subject:"Re\: \[PATCH v3 3\/3\] media\: atmel\-isi\: add sanity check for supported formats in try\/set

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

2015-08-24 Thread Josh Wu


Hi, Laurent

On 8/22/2015 2:22 AM, Laurent Pinchart wrote:

Hi Josh,

Thank you for the patch.

On Friday 21 August 2015 16:08:14 Josh Wu wrote:

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

Signed-off-by: Josh Wu 
---

Changes in v3:
- check the whether format is supported, if no then return a default
   format.
- misc changes according to Laurent's feedback.

Changes in v2:
- new added patch

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

diff --git a/drivers/media/platform/soc_camera/atmel-isi.c
b/drivers/media/platform/soc_camera/atmel-isi.c index fe9247a..84c91d3
100644
--- a/drivers/media/platform/soc_camera/atmel-isi.c
+++ b/drivers/media/platform/soc_camera/atmel-isi.c
@@ -102,17 +102,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;
@@ -126,8 +128,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);
@@ -138,8 +138,23 @@ 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 u32 pixformat)
+{
+   switch (pixformat) {
+   /* YUV, including grey */
+   case V4L2_PIX_FMT_GREY:
+   case V4L2_PIX_FMT_YUYV:
+   case V4L2_PIX_FMT_UYVY:
+   case V4L2_PIX_FMT_YVYU:
+   case V4L2_PIX_FMT_VYUY:
+   return true;
+   /* RGB, TODO */
+   default:
+   return false;
+   }
  }

  static irqreturn_t atmel_isi_handle_streaming(struct atmel_isi *isi)
@@ -390,10 +405,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(>lock);
/* Clear any pending interrupt */
@@ -491,6 +504,10 @@ static int isi_camera_set_fmt(struct soc_camera_device
*icd, struct v4l2_mbus_framefmt *mf = 
int ret;

+   /* check with atmel-isi support format, if not support use UYVY */
+   if (!is_supported(icd, pix->pixelformat))
+   pix->pixelformat = V4L2_PIX_FMT_YUYV;

The comment mentions UYVY and the code uses YUYV.


Oops, forgotten to change the comments. I'll fix it.



+
xlate = soc_camera_xlate_by_fourcc(icd, pix->pixelformat);
if (!xlate) {
dev_warn(icd->parent, "Format %x not found\n",

Can this still happen ?


I think this warning should not happen if user call set_fmt() with the 
format that get from by get_formats().

But it is a common pattern in soc_camera host code to handle this error.




@@ -540,6 +557,10 @@ static int isi_camera_try_fmt(struct soc_camera_device
*icd, u32 pixfmt = pix->pixelformat;
int ret;

+   /* check with atmel-isi support format, if not support use UYVY */
+   if (!is_supported(icd, pix->pixelformat))
+   pix->pixelformat = V4L2_PIX_FMT_YUYV;
+
xlate = soc_camera_xlate_by_fourcc(icd, pixfmt);
if (pixfmt && !xlate) {
dev_warn(icd->parent, "Format %x not found\n", pixfmt);

Same comment here.

I wonder whether most of the content of isi_camera_set_fmt() and
isi_camera_try_fmt() could be factorized out into a shared function.


I agree. the current set_fmt() doesn't touch any hardware, so it's 
almost same as try_fmt().


Best Regards,
Josh Wu




--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2015-08-24 Thread Josh Wu


Hi, Laurent

On 8/22/2015 2:22 AM, Laurent Pinchart wrote:

Hi Josh,

Thank you for the patch.

On Friday 21 August 2015 16:08:14 Josh Wu wrote:

After adding the format check in try_fmt()/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 v3:
- check the whether format is supported, if no then return a default
   format.
- misc changes according to Laurent's feedback.

Changes in v2:
- new added patch

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

diff --git a/drivers/media/platform/soc_camera/atmel-isi.c
b/drivers/media/platform/soc_camera/atmel-isi.c index fe9247a..84c91d3
100644
--- a/drivers/media/platform/soc_camera/atmel-isi.c
+++ b/drivers/media/platform/soc_camera/atmel-isi.c
@@ -102,17 +102,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;
@@ -126,8 +128,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);
@@ -138,8 +138,23 @@ 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 u32 pixformat)
+{
+   switch (pixformat) {
+   /* YUV, including grey */
+   case V4L2_PIX_FMT_GREY:
+   case V4L2_PIX_FMT_YUYV:
+   case V4L2_PIX_FMT_UYVY:
+   case V4L2_PIX_FMT_YVYU:
+   case V4L2_PIX_FMT_VYUY:
+   return true;
+   /* RGB, TODO */
+   default:
+   return false;
+   }
  }

  static irqreturn_t atmel_isi_handle_streaming(struct atmel_isi *isi)
@@ -390,10 +405,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 */
@@ -491,6 +504,10 @@ static int isi_camera_set_fmt(struct soc_camera_device
*icd, struct v4l2_mbus_framefmt *mf = format.format;
int ret;

+   /* check with atmel-isi support format, if not support use UYVY */
+   if (!is_supported(icd, pix-pixelformat))
+   pix-pixelformat = V4L2_PIX_FMT_YUYV;

The comment mentions UYVY and the code uses YUYV.


Oops, forgotten to change the comments. I'll fix it.



+
xlate = soc_camera_xlate_by_fourcc(icd, pix-pixelformat);
if (!xlate) {
dev_warn(icd-parent, Format %x not found\n,

Can this still happen ?


I think this warning should not happen if user call set_fmt() with the 
format that get from by get_formats().

But it is a common pattern in soc_camera host code to handle this error.




@@ -540,6 +557,10 @@ static int isi_camera_try_fmt(struct soc_camera_device
*icd, u32 pixfmt = pix-pixelformat;
int ret;

+   /* check with atmel-isi support format, if not support use UYVY */
+   if (!is_supported(icd, pix-pixelformat))
+   pix-pixelformat = V4L2_PIX_FMT_YUYV;
+
xlate = soc_camera_xlate_by_fourcc(icd, pixfmt);
if (pixfmt  !xlate) {
dev_warn(icd-parent, Format %x not found\n, pixfmt);

Same comment here.

I wonder whether most of the content of isi_camera_set_fmt() and
isi_camera_try_fmt() could be factorized out into a shared function.


I agree. the current set_fmt() doesn't touch any hardware, so it's 
almost same as try_fmt().


Best Regards,
Josh Wu




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

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

2015-08-21 Thread Laurent Pinchart

Hi Josh,

Thank you for the patch.

On Friday 21 August 2015 16:08:14 Josh Wu wrote:
> After adding the format check in try_fmt()/set_fmt(), we don't need any
> format check in configure_geometry(). So make configure_geometry() as
> void type.
> 
> Signed-off-by: Josh Wu 
> ---
> 
> Changes in v3:
> - check the whether format is supported, if no then return a default
>   format.
> - misc changes according to Laurent's feedback.
> 
> Changes in v2:
> - new added patch
> 
>  drivers/media/platform/soc_camera/atmel-isi.c | 37 ++--
>  1 file changed, 29 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/media/platform/soc_camera/atmel-isi.c
> b/drivers/media/platform/soc_camera/atmel-isi.c index fe9247a..84c91d3
> 100644
> --- a/drivers/media/platform/soc_camera/atmel-isi.c
> +++ b/drivers/media/platform/soc_camera/atmel-isi.c
> @@ -102,17 +102,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;
> @@ -126,8 +128,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);
> @@ -138,8 +138,23 @@ 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 u32 pixformat)
> +{
> + switch (pixformat) {
> + /* YUV, including grey */
> + case V4L2_PIX_FMT_GREY:
> + case V4L2_PIX_FMT_YUYV:
> + case V4L2_PIX_FMT_UYVY:
> + case V4L2_PIX_FMT_YVYU:
> + case V4L2_PIX_FMT_VYUY:
> + return true;
> + /* RGB, TODO */
> + default:
> + return false;
> + }
>  }
> 
>  static irqreturn_t atmel_isi_handle_streaming(struct atmel_isi *isi)
> @@ -390,10 +405,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(>lock);
>   /* Clear any pending interrupt */
> @@ -491,6 +504,10 @@ static int isi_camera_set_fmt(struct soc_camera_device
> *icd, struct v4l2_mbus_framefmt *mf = 
>   int ret;
> 
> + /* check with atmel-isi support format, if not support use UYVY */
> + if (!is_supported(icd, pix->pixelformat))
> + pix->pixelformat = V4L2_PIX_FMT_YUYV;

The comment mentions UYVY and the code uses YUYV.

> +
>   xlate = soc_camera_xlate_by_fourcc(icd, pix->pixelformat);
>   if (!xlate) {
>   dev_warn(icd->parent, "Format %x not found\n",

Can this still happen ?

> @@ -540,6 +557,10 @@ static int isi_camera_try_fmt(struct soc_camera_device
> *icd, u32 pixfmt = pix->pixelformat;
>   int ret;
> 
> + /* check with atmel-isi support format, if not support use UYVY */
> + if (!is_supported(icd, pix->pixelformat))
> + pix->pixelformat = V4L2_PIX_FMT_YUYV;
> +
>   xlate = soc_camera_xlate_by_fourcc(icd, pixfmt);
>   if (pixfmt && !xlate) {
>   dev_warn(icd->parent, "Format %x not found\n", pixfmt);

Same comment here.

I wonder whether most of the content of isi_camera_set_fmt() and 
isi_camera_try_fmt() could be factorized out into a shared function.

-- 
Regards,

Laurent Pinchart

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2015-08-21 Thread Laurent Pinchart

Hi Josh,

Thank you for the patch.

On Friday 21 August 2015 16:08:14 Josh Wu wrote:
 After adding the format check in try_fmt()/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 v3:
 - check the whether format is supported, if no then return a default
   format.
 - misc changes according to Laurent's feedback.
 
 Changes in v2:
 - new added patch
 
  drivers/media/platform/soc_camera/atmel-isi.c | 37 ++--
  1 file changed, 29 insertions(+), 8 deletions(-)
 
 diff --git a/drivers/media/platform/soc_camera/atmel-isi.c
 b/drivers/media/platform/soc_camera/atmel-isi.c index fe9247a..84c91d3
 100644
 --- a/drivers/media/platform/soc_camera/atmel-isi.c
 +++ b/drivers/media/platform/soc_camera/atmel-isi.c
 @@ -102,17 +102,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;
 @@ -126,8 +128,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);
 @@ -138,8 +138,23 @@ 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 u32 pixformat)
 +{
 + switch (pixformat) {
 + /* YUV, including grey */
 + case V4L2_PIX_FMT_GREY:
 + case V4L2_PIX_FMT_YUYV:
 + case V4L2_PIX_FMT_UYVY:
 + case V4L2_PIX_FMT_YVYU:
 + case V4L2_PIX_FMT_VYUY:
 + return true;
 + /* RGB, TODO */
 + default:
 + return false;
 + }
  }
 
  static irqreturn_t atmel_isi_handle_streaming(struct atmel_isi *isi)
 @@ -390,10 +405,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 */
 @@ -491,6 +504,10 @@ static int isi_camera_set_fmt(struct soc_camera_device
 *icd, struct v4l2_mbus_framefmt *mf = format.format;
   int ret;
 
 + /* check with atmel-isi support format, if not support use UYVY */
 + if (!is_supported(icd, pix-pixelformat))
 + pix-pixelformat = V4L2_PIX_FMT_YUYV;

The comment mentions UYVY and the code uses YUYV.

 +
   xlate = soc_camera_xlate_by_fourcc(icd, pix-pixelformat);
   if (!xlate) {
   dev_warn(icd-parent, Format %x not found\n,

Can this still happen ?

 @@ -540,6 +557,10 @@ static int isi_camera_try_fmt(struct soc_camera_device
 *icd, u32 pixfmt = pix-pixelformat;
   int ret;
 
 + /* check with atmel-isi support format, if not support use UYVY */
 + if (!is_supported(icd, pix-pixelformat))
 + pix-pixelformat = V4L2_PIX_FMT_YUYV;
 +
   xlate = soc_camera_xlate_by_fourcc(icd, pixfmt);
   if (pixfmt  !xlate) {
   dev_warn(icd-parent, Format %x not found\n, pixfmt);

Same comment here.

I wonder whether most of the content of isi_camera_set_fmt() and 
isi_camera_try_fmt() could be factorized out into a shared function.

-- 
Regards,

Laurent Pinchart

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

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

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

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

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

4 matches

Site Navigation

Mail list logo

Footer information