Re: [PATCH v2 3/3] media: atmel-isi: add sanity check for supported formats in set_fmt()

2015-08-20 Thread Josh Wu 

Hi, Laurent

Could you have time to review this patch? so that I can send a pull 
request with your reviewed-by tags.


Best Regards,
Josh Wu

On 8/5/2015 11:26 AM, Josh Wu wrote:

After adding the format check in set_fmt(), we don't need any format check
in configure_geometry(). So make configure_geometry() as void type.

Signed-off-by: Josh Wu josh...@atmel.com
---

Changes in v2:
- new added patch

  drivers/media/platform/soc_camera/atmel-isi.c | 39 +--
  1 file changed, 31 insertions(+), 8 deletions(-)

diff --git a/drivers/media/platform/soc_camera/atmel-isi.c 
b/drivers/media/platform/soc_camera/atmel-isi.c
index cb46aec..d0df518 100644
--- a/drivers/media/platform/soc_camera/atmel-isi.c
+++ b/drivers/media/platform/soc_camera/atmel-isi.c
@@ -103,17 +103,19 @@ static u32 isi_readl(struct atmel_isi *isi, u32 reg)
return readl(isi-regs + reg);
  }
  
-static int configure_geometry(struct atmel_isi *isi, u32 width,

+static void configure_geometry(struct atmel_isi *isi, u32 width,
u32 height, u32 code)
  {
u32 cfg2;
  
  	/* According to sensor's output format to set cfg2 */

switch (code) {
-   /* YUV, including grey */
+   default:
+   /* Grey */
case MEDIA_BUS_FMT_Y8_1X8:
cfg2 = ISI_CFG2_GRAYSCALE;
break;
+   /* YUV */
case MEDIA_BUS_FMT_VYUY8_2X8:
cfg2 = ISI_CFG2_YCC_SWAP_MODE_3;
break;
@@ -127,8 +129,6 @@ static int configure_geometry(struct atmel_isi *isi, u32 
width,
cfg2 = ISI_CFG2_YCC_SWAP_DEFAULT;
break;
/* RGB, TODO */
-   default:
-   return -EINVAL;
}
  
  	isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS);

@@ -139,8 +139,29 @@ static int configure_geometry(struct atmel_isi *isi, u32 
width,
cfg2 |= ((height - 1)  ISI_CFG2_IM_VSIZE_OFFSET)
 ISI_CFG2_IM_VSIZE_MASK;
isi_writel(isi, ISI_CFG2, cfg2);
+}
  
-	return 0;

+static bool is_supported(struct soc_camera_device *icd,
+   const struct soc_camera_format_xlate *xlate)
+{
+   bool ret = true;
+
+   switch (xlate-code) {
+   /* YUV, including grey */
+   case MEDIA_BUS_FMT_Y8_1X8:
+   case MEDIA_BUS_FMT_VYUY8_2X8:
+   case MEDIA_BUS_FMT_UYVY8_2X8:
+   case MEDIA_BUS_FMT_YVYU8_2X8:
+   case MEDIA_BUS_FMT_YUYV8_2X8:
+   break;
+   /* RGB, TODO */
+   default:
+   dev_err(icd-parent, not supported format: %d\n,
+   xlate-code);
+   ret = false;
+   }
+
+   return ret;
  }
  
  static irqreturn_t atmel_isi_handle_streaming(struct atmel_isi *isi)

@@ -391,10 +412,8 @@ static int start_streaming(struct vb2_queue *vq, unsigned 
int count)
/* Disable all interrupts */
isi_writel(isi, ISI_INTDIS, (u32)~0UL);
  
-	ret = configure_geometry(isi, icd-user_width, icd-user_height,

+   configure_geometry(isi, icd-user_width, icd-user_height,
icd-current_fmt-code);
-   if (ret  0)
-   return ret;
  
  	spin_lock_irq(isi-lock);

/* Clear any pending interrupt */
@@ -515,6 +534,10 @@ static int isi_camera_set_fmt(struct soc_camera_device 
*icd,
if (mf-code != xlate-code)
return -EINVAL;
  
+	/* check with atmel-isi support format */

+   if (!is_supported(icd, xlate))
+   return -EINVAL;
+
pix-width   = mf-width;
pix-height  = mf-height;
pix-field   = mf-field;


--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 3/3] media: atmel-isi: add sanity check for supported formats in set_fmt()

2015-08-20 Thread Laurent Pinchart 
Hi Josh,

Thank you for the patch.

On Wednesday 05 August 2015 11:26:29 Josh Wu wrote:
 After adding the format check in set_fmt(), we don't need any format check
 in configure_geometry(). So make configure_geometry() as void type.
 
 Signed-off-by: Josh Wu josh...@atmel.com
 ---
 
 Changes in v2:
 - new added patch
 
  drivers/media/platform/soc_camera/atmel-isi.c | 39 ++--
  1 file changed, 31 insertions(+), 8 deletions(-)
 
 diff --git a/drivers/media/platform/soc_camera/atmel-isi.c
 b/drivers/media/platform/soc_camera/atmel-isi.c index cb46aec..d0df518
 100644
 --- a/drivers/media/platform/soc_camera/atmel-isi.c
 +++ b/drivers/media/platform/soc_camera/atmel-isi.c
 @@ -103,17 +103,19 @@ static u32 isi_readl(struct atmel_isi *isi, u32 reg)
   return readl(isi-regs + reg);
  }
 
 -static int configure_geometry(struct atmel_isi *isi, u32 width,
 +static void configure_geometry(struct atmel_isi *isi, u32 width,
   u32 height, u32 code)
  {
   u32 cfg2;
 
   /* According to sensor's output format to set cfg2 */
   switch (code) {
 - /* YUV, including grey */
 + default:
 + /* Grey */
   case MEDIA_BUS_FMT_Y8_1X8:
   cfg2 = ISI_CFG2_GRAYSCALE;
   break;
 + /* YUV */
   case MEDIA_BUS_FMT_VYUY8_2X8:
   cfg2 = ISI_CFG2_YCC_SWAP_MODE_3;
   break;
 @@ -127,8 +129,6 @@ static int configure_geometry(struct atmel_isi *isi, u32
 width, cfg2 = ISI_CFG2_YCC_SWAP_DEFAULT;
   break;
   /* RGB, TODO */
 - default:
 - return -EINVAL;
   }
 
   isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS);
 @@ -139,8 +139,29 @@ static int configure_geometry(struct atmel_isi *isi,
 u32 width, cfg2 |= ((height - 1)  ISI_CFG2_IM_VSIZE_OFFSET)
ISI_CFG2_IM_VSIZE_MASK;
   isi_writel(isi, ISI_CFG2, cfg2);
 +}
 
 - return 0;
 +static bool is_supported(struct soc_camera_device *icd,
 + const struct soc_camera_format_xlate *xlate)
 +{
 + bool ret = true;
 +
 + switch (xlate-code) {
 + /* YUV, including grey */
 + case MEDIA_BUS_FMT_Y8_1X8:
 + case MEDIA_BUS_FMT_VYUY8_2X8:
 + case MEDIA_BUS_FMT_UYVY8_2X8:
 + case MEDIA_BUS_FMT_YVYU8_2X8:
 + case MEDIA_BUS_FMT_YUYV8_2X8:

I would just return true here and false below, and remove the ret variable.

 + break;
 + /* RGB, TODO */
 + default:
 + dev_err(icd-parent, not supported format: %d\n,
 + xlate-code);

If this can happen when userspace asks for an unsupported format I don't think 
you should print an error message to the kernel log.

 + ret = false;
 + }
 +
 + return ret;
  }
 
  static irqreturn_t atmel_isi_handle_streaming(struct atmel_isi *isi)
 @@ -391,10 +412,8 @@ static int start_streaming(struct vb2_queue *vq,
 unsigned int count) /* Disable all interrupts */
   isi_writel(isi, ISI_INTDIS, (u32)~0UL);
 
 - ret = configure_geometry(isi, icd-user_width, icd-user_height,
 + configure_geometry(isi, icd-user_width, icd-user_height,
   icd-current_fmt-code);
 - if (ret  0)
 - return ret;
 
   spin_lock_irq(isi-lock);
   /* Clear any pending interrupt */
 @@ -515,6 +534,10 @@ static int isi_camera_set_fmt(struct soc_camera_device
 *icd, if (mf-code != xlate-code)
   return -EINVAL;
 
 + /* check with atmel-isi support format */
 + if (!is_supported(icd, xlate))
 + return -EINVAL;
 +

S_FMT is supposed to pick a suitable default format when the requested format 
isn't supported. It shouldn't return an error.

   pix-width  = mf-width;
   pix-height = mf-height;
   pix-field  = mf-field;

-- 
Regards,

Laurent Pinchart

--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 3/3] media: atmel-isi: add sanity check for supported formats in set_fmt()

2015-08-20 Thread Josh Wu 

Hi, Laurent

Thanks for the review.

On 8/21/2015 2:30 AM, Laurent Pinchart wrote:

Hi Josh,

Thank you for the patch.

On Wednesday 05 August 2015 11:26:29 Josh Wu wrote:

After adding the format check in set_fmt(), we don't need any format check
in configure_geometry(). So make configure_geometry() as void type.

Signed-off-by: Josh Wu josh...@atmel.com
---

Changes in v2:
- new added patch

  drivers/media/platform/soc_camera/atmel-isi.c | 39 ++--
  1 file changed, 31 insertions(+), 8 deletions(-)

diff --git a/drivers/media/platform/soc_camera/atmel-isi.c
b/drivers/media/platform/soc_camera/atmel-isi.c index cb46aec..d0df518
100644
--- a/drivers/media/platform/soc_camera/atmel-isi.c
+++ b/drivers/media/platform/soc_camera/atmel-isi.c
@@ -103,17 +103,19 @@ static u32 isi_readl(struct atmel_isi *isi, u32 reg)
return readl(isi-regs + reg);
  }

-static int configure_geometry(struct atmel_isi *isi, u32 width,
+static void configure_geometry(struct atmel_isi *isi, u32 width,
u32 height, u32 code)
  {
u32 cfg2;

/* According to sensor's output format to set cfg2 */
switch (code) {
-   /* YUV, including grey */
+   default:
+   /* Grey */
case MEDIA_BUS_FMT_Y8_1X8:
cfg2 = ISI_CFG2_GRAYSCALE;
break;
+   /* YUV */
case MEDIA_BUS_FMT_VYUY8_2X8:
cfg2 = ISI_CFG2_YCC_SWAP_MODE_3;
break;
@@ -127,8 +129,6 @@ static int configure_geometry(struct atmel_isi *isi, u32
width, cfg2 = ISI_CFG2_YCC_SWAP_DEFAULT;
break;
/* RGB, TODO */
-   default:
-   return -EINVAL;
}

isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS);
@@ -139,8 +139,29 @@ static int configure_geometry(struct atmel_isi *isi,
u32 width, cfg2 |= ((height - 1)  ISI_CFG2_IM_VSIZE_OFFSET)
 ISI_CFG2_IM_VSIZE_MASK;
isi_writel(isi, ISI_CFG2, cfg2);
+}

-   return 0;
+static bool is_supported(struct soc_camera_device *icd,
+   const struct soc_camera_format_xlate *xlate)
+{
+   bool ret = true;
+
+   switch (xlate-code) {
+   /* YUV, including grey */
+   case MEDIA_BUS_FMT_Y8_1X8:
+   case MEDIA_BUS_FMT_VYUY8_2X8:
+   case MEDIA_BUS_FMT_UYVY8_2X8:
+   case MEDIA_BUS_FMT_YVYU8_2X8:
+   case MEDIA_BUS_FMT_YUYV8_2X8:

I would just return true here and false below, and remove the ret variable.

Ok.




+   break;
+   /* RGB, TODO */
+   default:
+   dev_err(icd-parent, not supported format: %d\n,
+   xlate-code);

If this can happen when userspace asks for an unsupported format I don't think
you should print an error message to the kernel log.


ok, I will remove this.




+   ret = false;
+   }
+
+   return ret;
  }

  static irqreturn_t atmel_isi_handle_streaming(struct atmel_isi *isi)
@@ -391,10 +412,8 @@ static int start_streaming(struct vb2_queue *vq,
unsigned int count) /* Disable all interrupts */
isi_writel(isi, ISI_INTDIS, (u32)~0UL);

-   ret = configure_geometry(isi, icd-user_width, icd-user_height,
+   configure_geometry(isi, icd-user_width, icd-user_height,
icd-current_fmt-code);
-   if (ret  0)
-   return ret;

spin_lock_irq(isi-lock);
/* Clear any pending interrupt */
@@ -515,6 +534,10 @@ static int isi_camera_set_fmt(struct soc_camera_device
*icd, if (mf-code != xlate-code)
return -EINVAL;

+   /* check with atmel-isi support format */
+   if (!is_supported(icd, xlate))
+   return -EINVAL;
+

S_FMT is supposed to pick a suitable default format when the requested format
isn't supported. It shouldn't return an error.


So I will move this check to beginning of S_FMT and if format not 
support, I will select a default format for it.


Best Regards,
Josh Wu




pix-width   = mf-width;
pix-height  = mf-height;
pix-field   = mf-field;


--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 3/3] media: atmel-isi: add sanity check for supported formats in set_fmt()

2015-08-04 Thread Josh Wu 
After adding the format check in set_fmt(), we don't need any format check
in configure_geometry(). So make configure_geometry() as void type.

Signed-off-by: Josh Wu josh...@atmel.com
---

Changes in v2:
- new added patch

 drivers/media/platform/soc_camera/atmel-isi.c | 39 +--
 1 file changed, 31 insertions(+), 8 deletions(-)

diff --git a/drivers/media/platform/soc_camera/atmel-isi.c 
b/drivers/media/platform/soc_camera/atmel-isi.c
index cb46aec..d0df518 100644
--- a/drivers/media/platform/soc_camera/atmel-isi.c
+++ b/drivers/media/platform/soc_camera/atmel-isi.c
@@ -103,17 +103,19 @@ static u32 isi_readl(struct atmel_isi *isi, u32 reg)
return readl(isi-regs + reg);
 }
 
-static int configure_geometry(struct atmel_isi *isi, u32 width,
+static void configure_geometry(struct atmel_isi *isi, u32 width,
u32 height, u32 code)
 {
u32 cfg2;
 
/* According to sensor's output format to set cfg2 */
switch (code) {
-   /* YUV, including grey */
+   default:
+   /* Grey */
case MEDIA_BUS_FMT_Y8_1X8:
cfg2 = ISI_CFG2_GRAYSCALE;
break;
+   /* YUV */
case MEDIA_BUS_FMT_VYUY8_2X8:
cfg2 = ISI_CFG2_YCC_SWAP_MODE_3;
break;
@@ -127,8 +129,6 @@ static int configure_geometry(struct atmel_isi *isi, u32 
width,
cfg2 = ISI_CFG2_YCC_SWAP_DEFAULT;
break;
/* RGB, TODO */
-   default:
-   return -EINVAL;
}
 
isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS);
@@ -139,8 +139,29 @@ static int configure_geometry(struct atmel_isi *isi, u32 
width,
cfg2 |= ((height - 1)  ISI_CFG2_IM_VSIZE_OFFSET)
 ISI_CFG2_IM_VSIZE_MASK;
isi_writel(isi, ISI_CFG2, cfg2);
+}
 
-   return 0;
+static bool is_supported(struct soc_camera_device *icd,
+   const struct soc_camera_format_xlate *xlate)
+{
+   bool ret = true;
+
+   switch (xlate-code) {
+   /* YUV, including grey */
+   case MEDIA_BUS_FMT_Y8_1X8:
+   case MEDIA_BUS_FMT_VYUY8_2X8:
+   case MEDIA_BUS_FMT_UYVY8_2X8:
+   case MEDIA_BUS_FMT_YVYU8_2X8:
+   case MEDIA_BUS_FMT_YUYV8_2X8:
+   break;
+   /* RGB, TODO */
+   default:
+   dev_err(icd-parent, not supported format: %d\n,
+   xlate-code);
+   ret = false;
+   }
+
+   return ret;
 }
 
 static irqreturn_t atmel_isi_handle_streaming(struct atmel_isi *isi)
@@ -391,10 +412,8 @@ static int start_streaming(struct vb2_queue *vq, unsigned 
int count)
/* Disable all interrupts */
isi_writel(isi, ISI_INTDIS, (u32)~0UL);
 
-   ret = configure_geometry(isi, icd-user_width, icd-user_height,
+   configure_geometry(isi, icd-user_width, icd-user_height,
icd-current_fmt-code);
-   if (ret  0)
-   return ret;
 
spin_lock_irq(isi-lock);
/* Clear any pending interrupt */
@@ -515,6 +534,10 @@ static int isi_camera_set_fmt(struct soc_camera_device 
*icd,
if (mf-code != xlate-code)
return -EINVAL;
 
+   /* check with atmel-isi support format */
+   if (!is_supported(icd, xlate))
+   return -EINVAL;
+
pix-width  = mf-width;
pix-height = mf-height;
pix-field  = mf-field;
-- 
1.9.1

--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html