[PATCH 1/3] media: imx: enable V4L2_PIX_FMT_XBGR32, _BGRX32, and _RGBX32

[PATCH 1/3] media: imx: enable V4L2_PIX_FMT_XBGR32, _BGRX32, and _RGBX32

[Philipp Zabel]

Now that proper V4L2 pixel formats exist for all 32-bit RGB
permutations, drop support for the poorly defined legacy format
V4L2_PIX_FMT_BGR32.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 drivers/staging/media/imx/imx-media-utils.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/media/imx/imx-media-utils.c b/drivers/staging/media/imx/imx-media-utils.c
index 4cc6a7462ae2..0788a1874557 100644
--- a/drivers/staging/media/imx/imx-media-utils.c
+++ b/drivers/staging/media/imx/imx-media-utils.c
@@ -184,7 +184,15 @@ static const struct imx_media_pixfmt rgb_formats[] = {
 		.cs     = IPUV3_COLORSPACE_RGB,
 		.bpp    = 24,
 	}, {
-		.fourcc	= V4L2_PIX_FMT_BGR32,
+		.fourcc	= V4L2_PIX_FMT_XBGR32,
+		.cs     = IPUV3_COLORSPACE_RGB,
+		.bpp    = 32,
+	}, {
+		.fourcc	= V4L2_PIX_FMT_BGRX32,
+		.cs     = IPUV3_COLORSPACE_RGB,
+		.bpp    = 32,
+	}, {
+		.fourcc	= V4L2_PIX_FMT_RGBX32,
 		.cs     = IPUV3_COLORSPACE_RGB,
 		.bpp    = 32,
 	},
-- 
2.20.1

[PATCH 2/3] media: imx: fix and simplify pixel format enumeration

[Philipp Zabel]

Merge yuv_formats and rgb_formats into a single array. Always loop over
all entries, skipping those that are incompatible with the requested
enumeration. This simplifies the code, lets us get rid of the manual
counting of array entries, and stops accidentally ignoring some non-mbus
RGB formats.

Before:

  $ v4l2-ctl -d /dev/video14 --list-formats-out
  ioctl: VIDIOC_ENUM_FMT
	Type: Video Output

	[0]: 'UYVY' (UYVY 4:2:2)
	[1]: 'YUYV' (YUYV 4:2:2)
	[2]: 'YU12' (Planar YUV 4:2:0)
	[3]: 'YV12' (Planar YVU 4:2:0)
	[4]: '422P' (Planar YUV 4:2:2)
	[5]: 'NV12' (Y/CbCr 4:2:0)
	[6]: 'NV16' (Y/CbCr 4:2:2)
	[7]: 'RGBP' (16-bit RGB 5-6-5)
	[8]: 'RGB3' (24-bit RGB 8-8-8)
	[9]: 'BX24' (32-bit XRGB 8-8-8-8)

After:

  $ v4l2-ctl -d /dev/video14 --list-formats-out
  ioctl: VIDIOC_ENUM_FMT
	Type: Video Output

	[0]: 'UYVY' (UYVY 4:2:2)
	[1]: 'YUYV' (YUYV 4:2:2)
	[2]: 'YU12' (Planar YUV 4:2:0)
	[3]: 'YV12' (Planar YVU 4:2:0)
	[4]: '422P' (Planar YUV 4:2:2)
	[5]: 'NV12' (Y/CbCr 4:2:0)
	[6]: 'NV16' (Y/CbCr 4:2:2)
	[7]: 'RGBP' (16-bit RGB 5-6-5)
	[8]: 'RGB3' (24-bit RGB 8-8-8)
	[9]: 'BGR3' (24-bit BGR 8-8-8)
	[10]: 'BX24' (32-bit XRGB 8-8-8-8)
	[11]: 'XR24' (32-bit BGRX 8-8-8-8)
	[12]: 'RX24' (32-bit XBGR 8-8-8-8)
	[13]: 'XB24' (32-bit RGBX 8-8-8-8)

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 drivers/staging/media/imx/imx-media-utils.c | 170 ++++++--------------
 1 file changed, 45 insertions(+), 125 deletions(-)

diff --git a/drivers/staging/media/imx/imx-media-utils.c b/drivers/staging/media/imx/imx-media-utils.c
index 0788a1874557..d61a8f4533dc 100644
--- a/drivers/staging/media/imx/imx-media-utils.c
+++ b/drivers/staging/media/imx/imx-media-utils.c
@@ -9,12 +9,9 @@
 
 /*
  * List of supported pixel formats for the subdevs.
- *
- * In all of these tables, the non-mbus formats (with no
- * mbus codes) must all fall at the end of the table.
  */
-
-static const struct imx_media_pixfmt yuv_formats[] = {
+static const struct imx_media_pixfmt pixel_formats[] = {
+	/*** YUV formats start here ***/
 	{
 		.fourcc	= V4L2_PIX_FMT_UYVY,
 		.codes  = {
@@ -31,12 +28,7 @@ static const struct imx_media_pixfmt yuv_formats[] = {
 		},
 		.cs     = IPUV3_COLORSPACE_YUV,
 		.bpp    = 16,
-	},
-	/***
-	 * non-mbus YUV formats start here. NOTE! when adding non-mbus
-	 * formats, NUM_NON_MBUS_YUV_FORMATS must be updated below.
-	 ***/
-	{
+	}, {
 		.fourcc	= V4L2_PIX_FMT_YUV420,
 		.cs     = IPUV3_COLORSPACE_YUV,
 		.bpp    = 12,
@@ -62,13 +54,7 @@ static const struct imx_media_pixfmt yuv_formats[] = {
 		.bpp    = 16,
 		.planar = true,
 	},
-};
-
-#define NUM_NON_MBUS_YUV_FORMATS 5
-#define NUM_YUV_FORMATS ARRAY_SIZE(yuv_formats)
-#define NUM_MBUS_YUV_FORMATS (NUM_YUV_FORMATS - NUM_NON_MBUS_YUV_FORMATS)
-
-static const struct imx_media_pixfmt rgb_formats[] = {
+	/*** RGB formats start here ***/
 	{
 		.fourcc	= V4L2_PIX_FMT_RGB565,
 		.codes  = {MEDIA_BUS_FMT_RGB565_2X8_LE},
@@ -83,12 +69,28 @@ static const struct imx_media_pixfmt rgb_formats[] = {
 		},
 		.cs     = IPUV3_COLORSPACE_RGB,
 		.bpp    = 24,
+	}, {
+		.fourcc	= V4L2_PIX_FMT_BGR24,
+		.cs     = IPUV3_COLORSPACE_RGB,
+		.bpp    = 24,
 	}, {
 		.fourcc	= V4L2_PIX_FMT_XRGB32,
 		.codes  = {MEDIA_BUS_FMT_ARGB8888_1X32},
 		.cs     = IPUV3_COLORSPACE_RGB,
 		.bpp    = 32,
 		.ipufmt = true,
+	}, {
+		.fourcc	= V4L2_PIX_FMT_XBGR32,
+		.cs     = IPUV3_COLORSPACE_RGB,
+		.bpp    = 32,
+	}, {
+		.fourcc	= V4L2_PIX_FMT_BGRX32,
+		.cs     = IPUV3_COLORSPACE_RGB,
+		.bpp    = 32,
+	}, {
+		.fourcc	= V4L2_PIX_FMT_RGBX32,
+		.cs     = IPUV3_COLORSPACE_RGB,
+		.bpp    = 32,
 	},
 	/*** raw bayer and grayscale formats start here ***/
 	{
@@ -175,33 +177,8 @@ static const struct imx_media_pixfmt rgb_formats[] = {
 		.bpp    = 16,
 		.bayer  = true,
 	},
-	/***
-	 * non-mbus RGB formats start here. NOTE! when adding non-mbus
-	 * formats, NUM_NON_MBUS_RGB_FORMATS must be updated below.
-	 ***/
-	{
-		.fourcc	= V4L2_PIX_FMT_BGR24,
-		.cs     = IPUV3_COLORSPACE_RGB,
-		.bpp    = 24,
-	}, {
-		.fourcc	= V4L2_PIX_FMT_XBGR32,
-		.cs     = IPUV3_COLORSPACE_RGB,
-		.bpp    = 32,
-	}, {
-		.fourcc	= V4L2_PIX_FMT_BGRX32,
-		.cs     = IPUV3_COLORSPACE_RGB,
-		.bpp    = 32,
-	}, {
-		.fourcc	= V4L2_PIX_FMT_RGBX32,
-		.cs     = IPUV3_COLORSPACE_RGB,
-		.bpp    = 32,
-	},
 };
 
-#define NUM_NON_MBUS_RGB_FORMATS 2
-#define NUM_RGB_FORMATS ARRAY_SIZE(rgb_formats)
-#define NUM_MBUS_RGB_FORMATS (NUM_RGB_FORMATS - NUM_NON_MBUS_RGB_FORMATS)
-
 static const struct imx_media_pixfmt ipu_yuv_formats[] = {
 	{
 		.fourcc = V4L2_PIX_FMT_YUV32,
@@ -240,20 +217,20 @@ static void init_mbus_colorimetry(struct v4l2_mbus_framefmt *mbus,
 }
 
 static const
-struct imx_media_pixfmt *__find_format(u32 fourcc,
-				       u32 code,
-				       bool allow_non_mbus,
-				       bool allow_bayer,
-				       const struct imx_media_pixfmt *array,
-				       u32 array_size)
+struct imx_media_pixfmt *find_format(u32 fourcc,
+				     u32 code,
+				     enum codespace_sel cs_sel,
+				     bool allow_non_mbus,
+				     bool allow_bayer)
 {
 	const struct imx_media_pixfmt *fmt;
 	int i, j;
 
-	for (i = 0; i < array_size; i++) {
-		fmt = &array[i];
+	for (i = 0; i < ARRAY_SIZE(pixel_formats); i++) {
+		fmt = &pixel_formats[i];
 
-		if ((!allow_non_mbus && !fmt->codes[0]) ||
+		if ((cs_sel != CS_SEL_ANY && fmt->cs != cs_sel) ||
+		    (!allow_non_mbus && !fmt->codes[0]) ||
 		    (!allow_bayer && fmt->bayer))
 			continue;
 
@@ -263,7 +240,7 @@ struct imx_media_pixfmt *__find_format(u32 fourcc,
 		if (!code)
 			continue;
 
-		for (j = 0; fmt->codes[j]; j++) {
+		for (j = 0; j < ARRAY_SIZE(fmt->codes) && fmt->codes[j]; j++) {
 			if (code == fmt->codes[j])
 				return fmt;
 		}
@@ -271,86 +248,29 @@ struct imx_media_pixfmt *__find_format(u32 fourcc,
 	return NULL;
 }
 
-static const struct imx_media_pixfmt *find_format(u32 fourcc,
-						  u32 code,
-						  enum codespace_sel cs_sel,
-						  bool allow_non_mbus,
-						  bool allow_bayer)
-{
-	const struct imx_media_pixfmt *ret;
-
-	switch (cs_sel) {
-	case CS_SEL_YUV:
-		return __find_format(fourcc, code, allow_non_mbus, allow_bayer,
-				     yuv_formats, NUM_YUV_FORMATS);
-	case CS_SEL_RGB:
-		return __find_format(fourcc, code, allow_non_mbus, allow_bayer,
-				     rgb_formats, NUM_RGB_FORMATS);
-	case CS_SEL_ANY:
-		ret = __find_format(fourcc, code, allow_non_mbus, allow_bayer,
-				    yuv_formats, NUM_YUV_FORMATS);
-		if (ret)
-			return ret;
-		return __find_format(fourcc, code, allow_non_mbus, allow_bayer,
-				     rgb_formats, NUM_RGB_FORMATS);
-	default:
-		return NULL;
-	}
-}
-
 static int enum_format(u32 *fourcc, u32 *code, u32 index,
 		       enum codespace_sel cs_sel,
 		       bool allow_non_mbus,
 		       bool allow_bayer)
 {
 	const struct imx_media_pixfmt *fmt;
-	u32 mbus_yuv_sz = NUM_MBUS_YUV_FORMATS;
-	u32 mbus_rgb_sz = NUM_MBUS_RGB_FORMATS;
-	u32 yuv_sz = NUM_YUV_FORMATS;
-	u32 rgb_sz = NUM_RGB_FORMATS;
+	unsigned int i, j = 0;
 
-	switch (cs_sel) {
-	case CS_SEL_YUV:
-		if (index >= yuv_sz ||
-		    (!allow_non_mbus && index >= mbus_yuv_sz))
-			return -EINVAL;
-		fmt = &yuv_formats[index];
-		break;
-	case CS_SEL_RGB:
-		if (index >= rgb_sz ||
-		    (!allow_non_mbus && index >= mbus_rgb_sz))
-			return -EINVAL;
-		fmt = &rgb_formats[index];
-		if (!allow_bayer && fmt->bayer)
-			return -EINVAL;
-		break;
-	case CS_SEL_ANY:
-		if (!allow_non_mbus) {
-			if (index >= mbus_yuv_sz) {
-				index -= mbus_yuv_sz;
-				if (index >= mbus_rgb_sz)
-					return -EINVAL;
-				fmt = &rgb_formats[index];
-				if (!allow_bayer && fmt->bayer)
-					return -EINVAL;
-			} else {
-				fmt = &yuv_formats[index];
-			}
-		} else {
-			if (index >= yuv_sz + rgb_sz)
-				return -EINVAL;
-			if (index >= yuv_sz) {
-				fmt = &rgb_formats[index - yuv_sz];
-				if (!allow_bayer && fmt->bayer)
-					return -EINVAL;
-			} else {
-				fmt = &yuv_formats[index];
-			}
-		}
-		break;
-	default:
-		return -EINVAL;
+	for (i = 0; i < ARRAY_SIZE(pixel_formats); i++) {
+		fmt = &pixel_formats[i];
+
+		if ((cs_sel != CS_SEL_ANY && fmt->cs != cs_sel) ||
+		    (!allow_non_mbus && !fmt->codes[0]) ||
+		    (!allow_bayer && fmt->bayer))
+			continue;
+
+		if (index == j)
+			break;
+
+		j++;
 	}
+	if (i == ARRAY_SIZE(pixel_formats))
+		return -EINVAL;
 
 	if (fourcc)
 		*fourcc = fmt->fourcc;
-- 
2.20.1

Re: [PATCH 2/3] media: imx: fix and simplify pixel format enumeration

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2697 bytes --]

Hi Philipp,

I love your patch! Perhaps something to improve:

[auto build test WARNING on linuxtv-media/master]
[cannot apply to v5.3-rc8 next-20190904]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Philipp-Zabel/media-imx-enable-V4L2_PIX_FMT_XBGR32-_BGRX32-and-_RGBX32/20190914-085415
base:   git://linuxtv.org/media_tree.git master
config: ia64-allmodconfig (attached as .config)
compiler: ia64-linux-gcc (GCC) 7.4.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=7.4.0 make.cross ARCH=ia64 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   drivers/staging/media/imx/imx-media-utils.c: In function 'find_format':
>> drivers/staging/media/imx/imx-media-utils.c:232:40: warning: comparison between 'const enum ipu_color_space' and 'enum codespace_sel' [-Wenum-compare]
      if ((cs_sel != CS_SEL_ANY && fmt->cs != cs_sel) ||
                                           ^~
   drivers/staging/media/imx/imx-media-utils.c: In function 'enum_format':
   drivers/staging/media/imx/imx-media-utils.c:262:40: warning: comparison between 'const enum ipu_color_space' and 'enum codespace_sel' [-Wenum-compare]
      if ((cs_sel != CS_SEL_ANY && fmt->cs != cs_sel) ||
                                           ^~

vim +232 drivers/staging/media/imx/imx-media-utils.c

   218	
   219	static const
   220	struct imx_media_pixfmt *find_format(u32 fourcc,
   221					     u32 code,
   222					     enum codespace_sel cs_sel,
   223					     bool allow_non_mbus,
   224					     bool allow_bayer)
   225	{
   226		const struct imx_media_pixfmt *fmt;
   227		int i, j;
   228	
   229		for (i = 0; i < ARRAY_SIZE(pixel_formats); i++) {
   230			fmt = &pixel_formats[i];
   231	
 > 232			if ((cs_sel != CS_SEL_ANY && fmt->cs != cs_sel) ||
   233			    (!allow_non_mbus && !fmt->codes[0]) ||
   234			    (!allow_bayer && fmt->bayer))
   235				continue;
   236	
   237			if (fourcc && fmt->fourcc == fourcc)
   238				return fmt;
   239	
   240			if (!code)
   241				continue;
   242	
   243			for (j = 0; j < ARRAY_SIZE(fmt->codes) && fmt->codes[j]; j++) {
   244				if (code == fmt->codes[j])
   245					return fmt;
   246			}
   247		}
   248		return NULL;
   249	}
   250	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 54564 bytes --]

Re: [PATCH 2/3] media: imx: fix and simplify pixel format enumeration

[Hans Verkuil]

On 9/12/19 6:01 PM, Philipp Zabel wrote:
> Merge yuv_formats and rgb_formats into a single array. Always loop over
> all entries, skipping those that are incompatible with the requested
> enumeration. This simplifies the code, lets us get rid of the manual
> counting of array entries, and stops accidentally ignoring some non-mbus
> RGB formats.
> 
> Before:
> 
>   $ v4l2-ctl -d /dev/video14 --list-formats-out
>   ioctl: VIDIOC_ENUM_FMT
> 	Type: Video Output
> 
> 	[0]: 'UYVY' (UYVY 4:2:2)
> 	[1]: 'YUYV' (YUYV 4:2:2)
> 	[2]: 'YU12' (Planar YUV 4:2:0)
> 	[3]: 'YV12' (Planar YVU 4:2:0)
> 	[4]: '422P' (Planar YUV 4:2:2)
> 	[5]: 'NV12' (Y/CbCr 4:2:0)
> 	[6]: 'NV16' (Y/CbCr 4:2:2)
> 	[7]: 'RGBP' (16-bit RGB 5-6-5)
> 	[8]: 'RGB3' (24-bit RGB 8-8-8)
> 	[9]: 'BX24' (32-bit XRGB 8-8-8-8)
> 
> After:
> 
>   $ v4l2-ctl -d /dev/video14 --list-formats-out
>   ioctl: VIDIOC_ENUM_FMT
> 	Type: Video Output
> 
> 	[0]: 'UYVY' (UYVY 4:2:2)
> 	[1]: 'YUYV' (YUYV 4:2:2)
> 	[2]: 'YU12' (Planar YUV 4:2:0)
> 	[3]: 'YV12' (Planar YVU 4:2:0)
> 	[4]: '422P' (Planar YUV 4:2:2)
> 	[5]: 'NV12' (Y/CbCr 4:2:0)
> 	[6]: 'NV16' (Y/CbCr 4:2:2)
> 	[7]: 'RGBP' (16-bit RGB 5-6-5)
> 	[8]: 'RGB3' (24-bit RGB 8-8-8)
> 	[9]: 'BGR3' (24-bit BGR 8-8-8)
> 	[10]: 'BX24' (32-bit XRGB 8-8-8-8)
> 	[11]: 'XR24' (32-bit BGRX 8-8-8-8)
> 	[12]: 'RX24' (32-bit XBGR 8-8-8-8)
> 	[13]: 'XB24' (32-bit RGBX 8-8-8-8)
> 
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
>  drivers/staging/media/imx/imx-media-utils.c | 170 ++++++--------------
>  1 file changed, 45 insertions(+), 125 deletions(-)
> 
> diff --git a/drivers/staging/media/imx/imx-media-utils.c b/drivers/staging/media/imx/imx-media-utils.c
> index 0788a1874557..d61a8f4533dc 100644
> --- a/drivers/staging/media/imx/imx-media-utils.c
> +++ b/drivers/staging/media/imx/imx-media-utils.c
> @@ -9,12 +9,9 @@
>  
>  /*
>   * List of supported pixel formats for the subdevs.
> - *
> - * In all of these tables, the non-mbus formats (with no
> - * mbus codes) must all fall at the end of the table.
>   */
> -
> -static const struct imx_media_pixfmt yuv_formats[] = {
> +static const struct imx_media_pixfmt pixel_formats[] = {
> +	/*** YUV formats start here ***/
>  	{
>  		.fourcc	= V4L2_PIX_FMT_UYVY,
>  		.codes  = {
> @@ -31,12 +28,7 @@ static const struct imx_media_pixfmt yuv_formats[] = {
>  		},
>  		.cs     = IPUV3_COLORSPACE_YUV,
>  		.bpp    = 16,
> -	},
> -	/***
> -	 * non-mbus YUV formats start here. NOTE! when adding non-mbus
> -	 * formats, NUM_NON_MBUS_YUV_FORMATS must be updated below.
> -	 ***/
> -	{
> +	}, {
>  		.fourcc	= V4L2_PIX_FMT_YUV420,
>  		.cs     = IPUV3_COLORSPACE_YUV,
>  		.bpp    = 12,
> @@ -62,13 +54,7 @@ static const struct imx_media_pixfmt yuv_formats[] = {
>  		.bpp    = 16,
>  		.planar = true,
>  	},
> -};
> -
> -#define NUM_NON_MBUS_YUV_FORMATS 5
> -#define NUM_YUV_FORMATS ARRAY_SIZE(yuv_formats)
> -#define NUM_MBUS_YUV_FORMATS (NUM_YUV_FORMATS - NUM_NON_MBUS_YUV_FORMATS)
> -
> -static const struct imx_media_pixfmt rgb_formats[] = {
> +	/*** RGB formats start here ***/
>  	{
>  		.fourcc	= V4L2_PIX_FMT_RGB565,
>  		.codes  = {MEDIA_BUS_FMT_RGB565_2X8_LE},
> @@ -83,12 +69,28 @@ static const struct imx_media_pixfmt rgb_formats[] = {
>  		},
>  		.cs     = IPUV3_COLORSPACE_RGB,
>  		.bpp    = 24,
> +	}, {
> +		.fourcc	= V4L2_PIX_FMT_BGR24,
> +		.cs     = IPUV3_COLORSPACE_RGB,
> +		.bpp    = 24,
>  	}, {
>  		.fourcc	= V4L2_PIX_FMT_XRGB32,
>  		.codes  = {MEDIA_BUS_FMT_ARGB8888_1X32},
>  		.cs     = IPUV3_COLORSPACE_RGB,
>  		.bpp    = 32,
>  		.ipufmt = true,
> +	}, {
> +		.fourcc	= V4L2_PIX_FMT_XBGR32,
> +		.cs     = IPUV3_COLORSPACE_RGB,
> +		.bpp    = 32,
> +	}, {
> +		.fourcc	= V4L2_PIX_FMT_BGRX32,
> +		.cs     = IPUV3_COLORSPACE_RGB,
> +		.bpp    = 32,
> +	}, {
> +		.fourcc	= V4L2_PIX_FMT_RGBX32,
> +		.cs     = IPUV3_COLORSPACE_RGB,
> +		.bpp    = 32,
>  	},
>  	/*** raw bayer and grayscale formats start here ***/
>  	{
> @@ -175,33 +177,8 @@ static const struct imx_media_pixfmt rgb_formats[] = {
>  		.bpp    = 16,
>  		.bayer  = true,
>  	},
> -	/***
> -	 * non-mbus RGB formats start here. NOTE! when adding non-mbus
> -	 * formats, NUM_NON_MBUS_RGB_FORMATS must be updated below.
> -	 ***/
> -	{
> -		.fourcc	= V4L2_PIX_FMT_BGR24,
> -		.cs     = IPUV3_COLORSPACE_RGB,
> -		.bpp    = 24,
> -	}, {
> -		.fourcc	= V4L2_PIX_FMT_XBGR32,
> -		.cs     = IPUV3_COLORSPACE_RGB,
> -		.bpp    = 32,
> -	}, {
> -		.fourcc	= V4L2_PIX_FMT_BGRX32,
> -		.cs     = IPUV3_COLORSPACE_RGB,
> -		.bpp    = 32,
> -	}, {
> -		.fourcc	= V4L2_PIX_FMT_RGBX32,
> -		.cs     = IPUV3_COLORSPACE_RGB,
> -		.bpp    = 32,
> -	},
>  };
>  
> -#define NUM_NON_MBUS_RGB_FORMATS 2
> -#define NUM_RGB_FORMATS ARRAY_SIZE(rgb_formats)
> -#define NUM_MBUS_RGB_FORMATS (NUM_RGB_FORMATS - NUM_NON_MBUS_RGB_FORMATS)
> -
>  static const struct imx_media_pixfmt ipu_yuv_formats[] = {
>  	{
>  		.fourcc = V4L2_PIX_FMT_YUV32,
> @@ -240,20 +217,20 @@ static void init_mbus_colorimetry(struct v4l2_mbus_framefmt *mbus,
>  }
>  
>  static const
> -struct imx_media_pixfmt *__find_format(u32 fourcc,
> -				       u32 code,
> -				       bool allow_non_mbus,
> -				       bool allow_bayer,
> -				       const struct imx_media_pixfmt *array,
> -				       u32 array_size)
> +struct imx_media_pixfmt *find_format(u32 fourcc,
> +				     u32 code,
> +				     enum codespace_sel cs_sel,
> +				     bool allow_non_mbus,
> +				     bool allow_bayer)
>  {
>  	const struct imx_media_pixfmt *fmt;
>  	int i, j;
>  
> -	for (i = 0; i < array_size; i++) {
> -		fmt = &array[i];
> +	for (i = 0; i < ARRAY_SIZE(pixel_formats); i++) {
> +		fmt = &pixel_formats[i];
>  
> -		if ((!allow_non_mbus && !fmt->codes[0]) ||
> +		if ((cs_sel != CS_SEL_ANY && fmt->cs != cs_sel) ||

I'm getting this compiler warnings:

drivers/staging/media/imx/imx-media-utils.c: In function ‘find_format’:
drivers/staging/media/imx/imx-media-utils.c:232:40: warning: comparison between ‘const enum ipu_color_space’ and ‘enum codespace_sel’
[-Wenum-compare]
  232 |   if ((cs_sel != CS_SEL_ANY && fmt->cs != cs_sel) ||
      |                                        ^~

> +		    (!allow_non_mbus && !fmt->codes[0]) ||
>  		    (!allow_bayer && fmt->bayer))
>  			continue;
>  
> @@ -263,7 +240,7 @@ struct imx_media_pixfmt *__find_format(u32 fourcc,
>  		if (!code)
>  			continue;
>  
> -		for (j = 0; fmt->codes[j]; j++) {
> +		for (j = 0; j < ARRAY_SIZE(fmt->codes) && fmt->codes[j]; j++) {
>  			if (code == fmt->codes[j])
>  				return fmt;
>  		}
> @@ -271,86 +248,29 @@ struct imx_media_pixfmt *__find_format(u32 fourcc,
>  	return NULL;
>  }
>  
> -static const struct imx_media_pixfmt *find_format(u32 fourcc,
> -						  u32 code,
> -						  enum codespace_sel cs_sel,
> -						  bool allow_non_mbus,
> -						  bool allow_bayer)
> -{
> -	const struct imx_media_pixfmt *ret;
> -
> -	switch (cs_sel) {
> -	case CS_SEL_YUV:
> -		return __find_format(fourcc, code, allow_non_mbus, allow_bayer,
> -				     yuv_formats, NUM_YUV_FORMATS);
> -	case CS_SEL_RGB:
> -		return __find_format(fourcc, code, allow_non_mbus, allow_bayer,
> -				     rgb_formats, NUM_RGB_FORMATS);
> -	case CS_SEL_ANY:
> -		ret = __find_format(fourcc, code, allow_non_mbus, allow_bayer,
> -				    yuv_formats, NUM_YUV_FORMATS);
> -		if (ret)
> -			return ret;
> -		return __find_format(fourcc, code, allow_non_mbus, allow_bayer,
> -				     rgb_formats, NUM_RGB_FORMATS);
> -	default:
> -		return NULL;
> -	}
> -}
> -
>  static int enum_format(u32 *fourcc, u32 *code, u32 index,
>  		       enum codespace_sel cs_sel,
>  		       bool allow_non_mbus,
>  		       bool allow_bayer)
>  {
>  	const struct imx_media_pixfmt *fmt;
> -	u32 mbus_yuv_sz = NUM_MBUS_YUV_FORMATS;
> -	u32 mbus_rgb_sz = NUM_MBUS_RGB_FORMATS;
> -	u32 yuv_sz = NUM_YUV_FORMATS;
> -	u32 rgb_sz = NUM_RGB_FORMATS;
> +	unsigned int i, j = 0;
>  
> -	switch (cs_sel) {
> -	case CS_SEL_YUV:
> -		if (index >= yuv_sz ||
> -		    (!allow_non_mbus && index >= mbus_yuv_sz))
> -			return -EINVAL;
> -		fmt = &yuv_formats[index];
> -		break;
> -	case CS_SEL_RGB:
> -		if (index >= rgb_sz ||
> -		    (!allow_non_mbus && index >= mbus_rgb_sz))
> -			return -EINVAL;
> -		fmt = &rgb_formats[index];
> -		if (!allow_bayer && fmt->bayer)
> -			return -EINVAL;
> -		break;
> -	case CS_SEL_ANY:
> -		if (!allow_non_mbus) {
> -			if (index >= mbus_yuv_sz) {
> -				index -= mbus_yuv_sz;
> -				if (index >= mbus_rgb_sz)
> -					return -EINVAL;
> -				fmt = &rgb_formats[index];
> -				if (!allow_bayer && fmt->bayer)
> -					return -EINVAL;
> -			} else {
> -				fmt = &yuv_formats[index];
> -			}
> -		} else {
> -			if (index >= yuv_sz + rgb_sz)
> -				return -EINVAL;
> -			if (index >= yuv_sz) {
> -				fmt = &rgb_formats[index - yuv_sz];
> -				if (!allow_bayer && fmt->bayer)
> -					return -EINVAL;
> -			} else {
> -				fmt = &yuv_formats[index];
> -			}
> -		}
> -		break;
> -	default:
> -		return -EINVAL;
> +	for (i = 0; i < ARRAY_SIZE(pixel_formats); i++) {
> +		fmt = &pixel_formats[i];
> +
> +		if ((cs_sel != CS_SEL_ANY && fmt->cs != cs_sel) ||

Same warning here.

I'm dropping this patch, I'll only merge the first patch.

Regards,

	Hans

> +		    (!allow_non_mbus && !fmt->codes[0]) ||
> +		    (!allow_bayer && fmt->bayer))
> +			continue;
> +
> +		if (index == j)
> +			break;
> +
> +		j++;
>  	}
> +	if (i == ARRAY_SIZE(pixel_formats))
> +		return -EINVAL;
>  
>  	if (fourcc)
>  		*fourcc = fmt->fourcc;
> 

Re: [PATCH 2/3] media: imx: fix and simplify pixel format enumeration

[Steve Longerbeam]

Hi Philipp,

Thanks for this patch and the next. I agree it simplifies the code.

Besides the compile warning caught by Hans and the kbuild test robot, I 
have only one other suggestion. Can you change the name of the index j 
(which becomes k in the next patch), to something like "match_index", 
which makes it more clear that this is a count of the pixel_formats[] 
entries that match the requested search criteria.

For testing I made this change in my github fork of the media-tree:

git@github.com:slongerbeam/mediatree.git, branch imx/philipp-pixfmts-cleanup

In that branch I also fixed the compile warnings.

Finally, I added a patch that also moves the IPU-internal formats (YUV4 
and BX24) into pixel_formats[] that results in similar simplifications, 
with an additional search parameter to find_format() and enum_format() 
to search only for IPU-internal formats or only non-IPU-internal formats.

Let me know if you agree with those changes, and if so please include 
them in next version of this series.

I tested on i.MX6Q SabreSD and all is good.

Steve


On 9/12/19 9:01 AM, Philipp Zabel wrote:
> Merge yuv_formats and rgb_formats into a single array. Always loop over
> all entries, skipping those that are incompatible with the requested
> enumeration. This simplifies the code, lets us get rid of the manual
> counting of array entries, and stops accidentally ignoring some non-mbus
> RGB formats.
>
> Before:
>
>    $ v4l2-ctl -d /dev/video14 --list-formats-out
>    ioctl: VIDIOC_ENUM_FMT
> 	Type: Video Output
>
> 	[0]: 'UYVY' (UYVY 4:2:2)
> 	[1]: 'YUYV' (YUYV 4:2:2)
> 	[2]: 'YU12' (Planar YUV 4:2:0)
> 	[3]: 'YV12' (Planar YVU 4:2:0)
> 	[4]: '422P' (Planar YUV 4:2:2)
> 	[5]: 'NV12' (Y/CbCr 4:2:0)
> 	[6]: 'NV16' (Y/CbCr 4:2:2)
> 	[7]: 'RGBP' (16-bit RGB 5-6-5)
> 	[8]: 'RGB3' (24-bit RGB 8-8-8)
> 	[9]: 'BX24' (32-bit XRGB 8-8-8-8)
>
> After:
>
>    $ v4l2-ctl -d /dev/video14 --list-formats-out
>    ioctl: VIDIOC_ENUM_FMT
> 	Type: Video Output
>
> 	[0]: 'UYVY' (UYVY 4:2:2)
> 	[1]: 'YUYV' (YUYV 4:2:2)
> 	[2]: 'YU12' (Planar YUV 4:2:0)
> 	[3]: 'YV12' (Planar YVU 4:2:0)
> 	[4]: '422P' (Planar YUV 4:2:2)
> 	[5]: 'NV12' (Y/CbCr 4:2:0)
> 	[6]: 'NV16' (Y/CbCr 4:2:2)
> 	[7]: 'RGBP' (16-bit RGB 5-6-5)
> 	[8]: 'RGB3' (24-bit RGB 8-8-8)
> 	[9]: 'BGR3' (24-bit BGR 8-8-8)
> 	[10]: 'BX24' (32-bit XRGB 8-8-8-8)
> 	[11]: 'XR24' (32-bit BGRX 8-8-8-8)
> 	[12]: 'RX24' (32-bit XBGR 8-8-8-8)
> 	[13]: 'XB24' (32-bit RGBX 8-8-8-8)
>
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
>   drivers/staging/media/imx/imx-media-utils.c | 170 ++++++--------------
>   1 file changed, 45 insertions(+), 125 deletions(-)
>
> diff --git a/drivers/staging/media/imx/imx-media-utils.c b/drivers/staging/media/imx/imx-media-utils.c
> index 0788a1874557..d61a8f4533dc 100644
> --- a/drivers/staging/media/imx/imx-media-utils.c
> +++ b/drivers/staging/media/imx/imx-media-utils.c
> @@ -9,12 +9,9 @@
>   
>   /*
>    * List of supported pixel formats for the subdevs.
> - *
> - * In all of these tables, the non-mbus formats (with no
> - * mbus codes) must all fall at the end of the table.
>    */
> -
> -static const struct imx_media_pixfmt yuv_formats[] = {
> +static const struct imx_media_pixfmt pixel_formats[] = {
> +	/*** YUV formats start here ***/
>   	{
>   		.fourcc	= V4L2_PIX_FMT_UYVY,
>   		.codes  = {
> @@ -31,12 +28,7 @@ static const struct imx_media_pixfmt yuv_formats[] = {
>   		},
>   		.cs     = IPUV3_COLORSPACE_YUV,
>   		.bpp    = 16,
> -	},
> -	/***
> -	 * non-mbus YUV formats start here. NOTE! when adding non-mbus
> -	 * formats, NUM_NON_MBUS_YUV_FORMATS must be updated below.
> -	 ***/
> -	{
> +	}, {
>   		.fourcc	= V4L2_PIX_FMT_YUV420,
>   		.cs     = IPUV3_COLORSPACE_YUV,
>   		.bpp    = 12,
> @@ -62,13 +54,7 @@ static const struct imx_media_pixfmt yuv_formats[] = {
>   		.bpp    = 16,
>   		.planar = true,
>   	},
> -};
> -
> -#define NUM_NON_MBUS_YUV_FORMATS 5
> -#define NUM_YUV_FORMATS ARRAY_SIZE(yuv_formats)
> -#define NUM_MBUS_YUV_FORMATS (NUM_YUV_FORMATS - NUM_NON_MBUS_YUV_FORMATS)
> -
> -static const struct imx_media_pixfmt rgb_formats[] = {
> +	/*** RGB formats start here ***/
>   	{
>   		.fourcc	= V4L2_PIX_FMT_RGB565,
>   		.codes  = {MEDIA_BUS_FMT_RGB565_2X8_LE},
> @@ -83,12 +69,28 @@ static const struct imx_media_pixfmt rgb_formats[] = {
>   		},
>   		.cs     = IPUV3_COLORSPACE_RGB,
>   		.bpp    = 24,
> +	}, {
> +		.fourcc	= V4L2_PIX_FMT_BGR24,
> +		.cs     = IPUV3_COLORSPACE_RGB,
> +		.bpp    = 24,
>   	}, {
>   		.fourcc	= V4L2_PIX_FMT_XRGB32,
>   		.codes  = {MEDIA_BUS_FMT_ARGB8888_1X32},
>   		.cs     = IPUV3_COLORSPACE_RGB,
>   		.bpp    = 32,
>   		.ipufmt = true,
> +	}, {
> +		.fourcc	= V4L2_PIX_FMT_XBGR32,
> +		.cs     = IPUV3_COLORSPACE_RGB,
> +		.bpp    = 32,
> +	}, {
> +		.fourcc	= V4L2_PIX_FMT_BGRX32,
> +		.cs     = IPUV3_COLORSPACE_RGB,
> +		.bpp    = 32,
> +	}, {
> +		.fourcc	= V4L2_PIX_FMT_RGBX32,
> +		.cs     = IPUV3_COLORSPACE_RGB,
> +		.bpp    = 32,
>   	},
>   	/*** raw bayer and grayscale formats start here ***/
>   	{
> @@ -175,33 +177,8 @@ static const struct imx_media_pixfmt rgb_formats[] = {
>   		.bpp    = 16,
>   		.bayer  = true,
>   	},
> -	/***
> -	 * non-mbus RGB formats start here. NOTE! when adding non-mbus
> -	 * formats, NUM_NON_MBUS_RGB_FORMATS must be updated below.
> -	 ***/
> -	{
> -		.fourcc	= V4L2_PIX_FMT_BGR24,
> -		.cs     = IPUV3_COLORSPACE_RGB,
> -		.bpp    = 24,
> -	}, {
> -		.fourcc	= V4L2_PIX_FMT_XBGR32,
> -		.cs     = IPUV3_COLORSPACE_RGB,
> -		.bpp    = 32,
> -	}, {
> -		.fourcc	= V4L2_PIX_FMT_BGRX32,
> -		.cs     = IPUV3_COLORSPACE_RGB,
> -		.bpp    = 32,
> -	}, {
> -		.fourcc	= V4L2_PIX_FMT_RGBX32,
> -		.cs     = IPUV3_COLORSPACE_RGB,
> -		.bpp    = 32,
> -	},
>   };
>   
> -#define NUM_NON_MBUS_RGB_FORMATS 2
> -#define NUM_RGB_FORMATS ARRAY_SIZE(rgb_formats)
> -#define NUM_MBUS_RGB_FORMATS (NUM_RGB_FORMATS - NUM_NON_MBUS_RGB_FORMATS)
> -
>   static const struct imx_media_pixfmt ipu_yuv_formats[] = {
>   	{
>   		.fourcc = V4L2_PIX_FMT_YUV32,
> @@ -240,20 +217,20 @@ static void init_mbus_colorimetry(struct v4l2_mbus_framefmt *mbus,
>   }
>   
>   static const
> -struct imx_media_pixfmt *__find_format(u32 fourcc,
> -				       u32 code,
> -				       bool allow_non_mbus,
> -				       bool allow_bayer,
> -				       const struct imx_media_pixfmt *array,
> -				       u32 array_size)
> +struct imx_media_pixfmt *find_format(u32 fourcc,
> +				     u32 code,
> +				     enum codespace_sel cs_sel,
> +				     bool allow_non_mbus,
> +				     bool allow_bayer)
>   {
>   	const struct imx_media_pixfmt *fmt;
>   	int i, j;
>   
> -	for (i = 0; i < array_size; i++) {
> -		fmt = &array[i];
> +	for (i = 0; i < ARRAY_SIZE(pixel_formats); i++) {
> +		fmt = &pixel_formats[i];
>   
> -		if ((!allow_non_mbus && !fmt->codes[0]) ||
> +		if ((cs_sel != CS_SEL_ANY && fmt->cs != cs_sel) ||
> +		    (!allow_non_mbus && !fmt->codes[0]) ||
>   		    (!allow_bayer && fmt->bayer))
>   			continue;
>   
> @@ -263,7 +240,7 @@ struct imx_media_pixfmt *__find_format(u32 fourcc,
>   		if (!code)
>   			continue;
>   
> -		for (j = 0; fmt->codes[j]; j++) {
> +		for (j = 0; j < ARRAY_SIZE(fmt->codes) && fmt->codes[j]; j++) {
>   			if (code == fmt->codes[j])
>   				return fmt;
>   		}
> @@ -271,86 +248,29 @@ struct imx_media_pixfmt *__find_format(u32 fourcc,
>   	return NULL;
>   }
>   
> -static const struct imx_media_pixfmt *find_format(u32 fourcc,
> -						  u32 code,
> -						  enum codespace_sel cs_sel,
> -						  bool allow_non_mbus,
> -						  bool allow_bayer)
> -{
> -	const struct imx_media_pixfmt *ret;
> -
> -	switch (cs_sel) {
> -	case CS_SEL_YUV:
> -		return __find_format(fourcc, code, allow_non_mbus, allow_bayer,
> -				     yuv_formats, NUM_YUV_FORMATS);
> -	case CS_SEL_RGB:
> -		return __find_format(fourcc, code, allow_non_mbus, allow_bayer,
> -				     rgb_formats, NUM_RGB_FORMATS);
> -	case CS_SEL_ANY:
> -		ret = __find_format(fourcc, code, allow_non_mbus, allow_bayer,
> -				    yuv_formats, NUM_YUV_FORMATS);
> -		if (ret)
> -			return ret;
> -		return __find_format(fourcc, code, allow_non_mbus, allow_bayer,
> -				     rgb_formats, NUM_RGB_FORMATS);
> -	default:
> -		return NULL;
> -	}
> -}
> -
>   static int enum_format(u32 *fourcc, u32 *code, u32 index,
>   		       enum codespace_sel cs_sel,
>   		       bool allow_non_mbus,
>   		       bool allow_bayer)
>   {
>   	const struct imx_media_pixfmt *fmt;
> -	u32 mbus_yuv_sz = NUM_MBUS_YUV_FORMATS;
> -	u32 mbus_rgb_sz = NUM_MBUS_RGB_FORMATS;
> -	u32 yuv_sz = NUM_YUV_FORMATS;
> -	u32 rgb_sz = NUM_RGB_FORMATS;
> +	unsigned int i, j = 0;
>   
> -	switch (cs_sel) {
> -	case CS_SEL_YUV:
> -		if (index >= yuv_sz ||
> -		    (!allow_non_mbus && index >= mbus_yuv_sz))
> -			return -EINVAL;
> -		fmt = &yuv_formats[index];
> -		break;
> -	case CS_SEL_RGB:
> -		if (index >= rgb_sz ||
> -		    (!allow_non_mbus && index >= mbus_rgb_sz))
> -			return -EINVAL;
> -		fmt = &rgb_formats[index];
> -		if (!allow_bayer && fmt->bayer)
> -			return -EINVAL;
> -		break;
> -	case CS_SEL_ANY:
> -		if (!allow_non_mbus) {
> -			if (index >= mbus_yuv_sz) {
> -				index -= mbus_yuv_sz;
> -				if (index >= mbus_rgb_sz)
> -					return -EINVAL;
> -				fmt = &rgb_formats[index];
> -				if (!allow_bayer && fmt->bayer)
> -					return -EINVAL;
> -			} else {
> -				fmt = &yuv_formats[index];
> -			}
> -		} else {
> -			if (index >= yuv_sz + rgb_sz)
> -				return -EINVAL;
> -			if (index >= yuv_sz) {
> -				fmt = &rgb_formats[index - yuv_sz];
> -				if (!allow_bayer && fmt->bayer)
> -					return -EINVAL;
> -			} else {
> -				fmt = &yuv_formats[index];
> -			}
> -		}
> -		break;
> -	default:
> -		return -EINVAL;
> +	for (i = 0; i < ARRAY_SIZE(pixel_formats); i++) {
> +		fmt = &pixel_formats[i];
> +
> +		if ((cs_sel != CS_SEL_ANY && fmt->cs != cs_sel) ||
> +		    (!allow_non_mbus && !fmt->codes[0]) ||
> +		    (!allow_bayer && fmt->bayer))
> +			continue;
> +
> +		if (index == j)
> +			break;
> +
> +		j++;
>   	}
> +	if (i == ARRAY_SIZE(pixel_formats))
> +		return -EINVAL;
>   
>   	if (fourcc)
>   		*fourcc = fmt->fourcc;

[PATCH 3/3] media: imx: fix media bus format enumeration

[Philipp Zabel]

Iterate over all media bus formats, not just over the first format in
each imx_media_pixfmt entry.

Before:

  $ v4l2-ctl -d $(media-ctl -e ipu1_csi0) --list-subdev-mbus-codes 0
  ioctl: VIDIOC_SUBDEV_ENUM_MBUS_CODE (pad=0)
	0x2006: MEDIA_BUS_FMT_UYVY8_2X8
	0x2008: MEDIA_BUS_FMT_YUYV8_2X8
	0x1008: MEDIA_BUS_FMT_RGB565_2X8_LE
	0x100a: MEDIA_BUS_FMT_RGB888_1X24
	0x100d: MEDIA_BUS_FMT_ARGB8888_1X32
	0x3001: MEDIA_BUS_FMT_SBGGR8_1X8
	0x3013: MEDIA_BUS_FMT_SGBRG8_1X8
	0x3002: MEDIA_BUS_FMT_SGRBG8_1X8
	0x3014: MEDIA_BUS_FMT_SRGGB8_1X8
	0x3007: MEDIA_BUS_FMT_SBGGR10_1X10
	0x300e: MEDIA_BUS_FMT_SGBRG10_1X10
	0x300a: MEDIA_BUS_FMT_SGRBG10_1X10
	0x300f: MEDIA_BUS_FMT_SRGGB10_1X10
	0x2001: MEDIA_BUS_FMT_Y8_1X8
	0x200a: MEDIA_BUS_FMT_Y10_1X10

After:

  $ v4l2-ctl -d $(media-ctl -e ipu1_csi0) --list-subdev-mbus-codes 0
  ioctl: VIDIOC_SUBDEV_ENUM_MBUS_CODE (pad=0)
	0x2006: MEDIA_BUS_FMT_UYVY8_2X8
	0x200f: MEDIA_BUS_FMT_UYVY8_1X16
	0x2008: MEDIA_BUS_FMT_YUYV8_2X8
	0x2011: MEDIA_BUS_FMT_YUYV8_1X16
	0x1008: MEDIA_BUS_FMT_RGB565_2X8_LE
	0x100a: MEDIA_BUS_FMT_RGB888_1X24
	0x100c: MEDIA_BUS_FMT_RGB888_2X12_LE
	0x100d: MEDIA_BUS_FMT_ARGB8888_1X32
	0x3001: MEDIA_BUS_FMT_SBGGR8_1X8
	0x3013: MEDIA_BUS_FMT_SGBRG8_1X8
	0x3002: MEDIA_BUS_FMT_SGRBG8_1X8
	0x3014: MEDIA_BUS_FMT_SRGGB8_1X8
	0x3007: MEDIA_BUS_FMT_SBGGR10_1X10
	0x3008: MEDIA_BUS_FMT_SBGGR12_1X12
	0x3019: MEDIA_BUS_FMT_SBGGR14_1X14
	0x301d: MEDIA_BUS_FMT_SBGGR16_1X16
	0x300e: MEDIA_BUS_FMT_SGBRG10_1X10
	0x3010: MEDIA_BUS_FMT_SGBRG12_1X12
	0x301a: MEDIA_BUS_FMT_SGBRG14_1X14
	0x301e: MEDIA_BUS_FMT_SGBRG16_1X16
	0x300a: MEDIA_BUS_FMT_SGRBG10_1X10
	0x3011: MEDIA_BUS_FMT_SGRBG12_1X12
	0x301b: MEDIA_BUS_FMT_SGRBG14_1X14
	0x301f: MEDIA_BUS_FMT_SGRBG16_1X16
	0x300f: MEDIA_BUS_FMT_SRGGB10_1X10
	0x3012: MEDIA_BUS_FMT_SRGGB12_1X12
	0x301c: MEDIA_BUS_FMT_SRGGB14_1X14
	0x3020: MEDIA_BUS_FMT_SRGGB16_1X16
	0x2001: MEDIA_BUS_FMT_Y8_1X8
	0x200a: MEDIA_BUS_FMT_Y10_1X10
	0x2013: MEDIA_BUS_FMT_Y12_1X12

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 drivers/staging/media/imx/imx-media-utils.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/media/imx/imx-media-utils.c b/drivers/staging/media/imx/imx-media-utils.c
index d61a8f4533dc..5f8604db4dd4 100644
--- a/drivers/staging/media/imx/imx-media-utils.c
+++ b/drivers/staging/media/imx/imx-media-utils.c
@@ -254,7 +254,7 @@ static int enum_format(u32 *fourcc, u32 *code, u32 index,
 		       bool allow_bayer)
 {
 	const struct imx_media_pixfmt *fmt;
-	unsigned int i, j = 0;
+	unsigned int i, j, k = 0;
 
 	for (i = 0; i < ARRAY_SIZE(pixel_formats); i++) {
 		fmt = &pixel_formats[i];
@@ -264,18 +264,29 @@ static int enum_format(u32 *fourcc, u32 *code, u32 index,
 		    (!allow_bayer && fmt->bayer))
 			continue;
 
-		if (index == j)
+		if (fourcc && index == k)
 			break;
 
-		j++;
+		if (!code) {
+			k++;
+			continue;
+		}
+
+		for (j = 0; j < ARRAY_SIZE(fmt->codes) && fmt->codes[j]; j++) {
+			if (index == k)
+				goto out;
+
+			k++;
+		}
 	}
 	if (i == ARRAY_SIZE(pixel_formats))
 		return -EINVAL;
 
+out:
 	if (fourcc)
 		*fourcc = fmt->fourcc;
 	if (code)
-		*code = fmt->codes[0];
+		*code = fmt->codes[j];
 
 	return 0;
 }
-- 
2.20.1

Re: [PATCH 3/3] media: imx: fix media bus format enumeration

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 4113 bytes --]

Hi Philipp,

I love your patch! Perhaps something to improve:

[auto build test WARNING on linuxtv-media/master]
[cannot apply to v5.3-rc8 next-20190904]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Philipp-Zabel/media-imx-enable-V4L2_PIX_FMT_XBGR32-_BGRX32-and-_RGBX32/20190914-085415
base:   git://linuxtv.org/media_tree.git master
config: ia64-allmodconfig (attached as .config)
compiler: ia64-linux-gcc (GCC) 7.4.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=7.4.0 make.cross ARCH=ia64 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

Note: it may well be a FALSE warning. FWIW you are at least aware of it now.
http://gcc.gnu.org/wiki/Better_Uninitialized_Warnings

All warnings (new ones prefixed by >>):

   drivers/staging/media/imx/imx-media-utils.c: In function 'find_format':
   drivers/staging/media/imx/imx-media-utils.c:232:40: warning: comparison between 'const enum ipu_color_space' and 'enum codespace_sel' [-Wenum-compare]
      if ((cs_sel != CS_SEL_ANY && fmt->cs != cs_sel) ||
                                           ^~
   drivers/staging/media/imx/imx-media-utils.c: In function 'enum_format':
   drivers/staging/media/imx/imx-media-utils.c:262:40: warning: comparison between 'const enum ipu_color_space' and 'enum codespace_sel' [-Wenum-compare]
      if ((cs_sel != CS_SEL_ANY && fmt->cs != cs_sel) ||
                                           ^~
>> drivers/staging/media/imx/imx-media-utils.c:257:18: warning: 'j' may be used uninitialized in this function [-Wmaybe-uninitialized]
     unsigned int i, j, k = 0;
                     ^

vim +/j +257 drivers/staging/media/imx/imx-media-utils.c

   218	
   219	static const
   220	struct imx_media_pixfmt *find_format(u32 fourcc,
   221					     u32 code,
   222					     enum codespace_sel cs_sel,
   223					     bool allow_non_mbus,
   224					     bool allow_bayer)
   225	{
   226		const struct imx_media_pixfmt *fmt;
   227		int i, j;
   228	
   229		for (i = 0; i < ARRAY_SIZE(pixel_formats); i++) {
   230			fmt = &pixel_formats[i];
   231	
 > 232			if ((cs_sel != CS_SEL_ANY && fmt->cs != cs_sel) ||
   233			    (!allow_non_mbus && !fmt->codes[0]) ||
   234			    (!allow_bayer && fmt->bayer))
   235				continue;
   236	
   237			if (fourcc && fmt->fourcc == fourcc)
   238				return fmt;
   239	
   240			if (!code)
   241				continue;
   242	
   243			for (j = 0; j < ARRAY_SIZE(fmt->codes) && fmt->codes[j]; j++) {
   244				if (code == fmt->codes[j])
   245					return fmt;
   246			}
   247		}
   248		return NULL;
   249	}
   250	
   251	static int enum_format(u32 *fourcc, u32 *code, u32 index,
   252			       enum codespace_sel cs_sel,
   253			       bool allow_non_mbus,
   254			       bool allow_bayer)
   255	{
   256		const struct imx_media_pixfmt *fmt;
 > 257		unsigned int i, j, k = 0;
   258	
   259		for (i = 0; i < ARRAY_SIZE(pixel_formats); i++) {
   260			fmt = &pixel_formats[i];
   261	
   262			if ((cs_sel != CS_SEL_ANY && fmt->cs != cs_sel) ||
   263			    (!allow_non_mbus && !fmt->codes[0]) ||
   264			    (!allow_bayer && fmt->bayer))
   265				continue;
   266	
   267			if (fourcc && index == k)
   268				break;
   269	
   270			if (!code) {
   271				k++;
   272				continue;
   273			}
   274	
   275			for (j = 0; j < ARRAY_SIZE(fmt->codes) && fmt->codes[j]; j++) {
   276				if (index == k)
   277					goto out;
   278	
   279				k++;
   280			}
   281		}
   282		if (i == ARRAY_SIZE(pixel_formats))
   283			return -EINVAL;
   284	
   285	out:
   286		if (fourcc)
   287			*fourcc = fmt->fourcc;
   288		if (code)
   289			*code = fmt->codes[j];
   290	
   291		return 0;
   292	}
   293	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 54564 bytes --]

Re: [PATCH 3/3] media: imx: fix media bus format enumeration

[Hans Verkuil]

On 9/12/19 6:01 PM, Philipp Zabel wrote:
> Iterate over all media bus formats, not just over the first format in
> each imx_media_pixfmt entry.
> 
> Before:
> 
>   $ v4l2-ctl -d $(media-ctl -e ipu1_csi0) --list-subdev-mbus-codes 0
>   ioctl: VIDIOC_SUBDEV_ENUM_MBUS_CODE (pad=0)
> 	0x2006: MEDIA_BUS_FMT_UYVY8_2X8
> 	0x2008: MEDIA_BUS_FMT_YUYV8_2X8
> 	0x1008: MEDIA_BUS_FMT_RGB565_2X8_LE
> 	0x100a: MEDIA_BUS_FMT_RGB888_1X24
> 	0x100d: MEDIA_BUS_FMT_ARGB8888_1X32
> 	0x3001: MEDIA_BUS_FMT_SBGGR8_1X8
> 	0x3013: MEDIA_BUS_FMT_SGBRG8_1X8
> 	0x3002: MEDIA_BUS_FMT_SGRBG8_1X8
> 	0x3014: MEDIA_BUS_FMT_SRGGB8_1X8
> 	0x3007: MEDIA_BUS_FMT_SBGGR10_1X10
> 	0x300e: MEDIA_BUS_FMT_SGBRG10_1X10
> 	0x300a: MEDIA_BUS_FMT_SGRBG10_1X10
> 	0x300f: MEDIA_BUS_FMT_SRGGB10_1X10
> 	0x2001: MEDIA_BUS_FMT_Y8_1X8
> 	0x200a: MEDIA_BUS_FMT_Y10_1X10
> 
> After:
> 
>   $ v4l2-ctl -d $(media-ctl -e ipu1_csi0) --list-subdev-mbus-codes 0
>   ioctl: VIDIOC_SUBDEV_ENUM_MBUS_CODE (pad=0)
> 	0x2006: MEDIA_BUS_FMT_UYVY8_2X8
> 	0x200f: MEDIA_BUS_FMT_UYVY8_1X16
> 	0x2008: MEDIA_BUS_FMT_YUYV8_2X8
> 	0x2011: MEDIA_BUS_FMT_YUYV8_1X16
> 	0x1008: MEDIA_BUS_FMT_RGB565_2X8_LE
> 	0x100a: MEDIA_BUS_FMT_RGB888_1X24
> 	0x100c: MEDIA_BUS_FMT_RGB888_2X12_LE
> 	0x100d: MEDIA_BUS_FMT_ARGB8888_1X32
> 	0x3001: MEDIA_BUS_FMT_SBGGR8_1X8
> 	0x3013: MEDIA_BUS_FMT_SGBRG8_1X8
> 	0x3002: MEDIA_BUS_FMT_SGRBG8_1X8
> 	0x3014: MEDIA_BUS_FMT_SRGGB8_1X8
> 	0x3007: MEDIA_BUS_FMT_SBGGR10_1X10
> 	0x3008: MEDIA_BUS_FMT_SBGGR12_1X12
> 	0x3019: MEDIA_BUS_FMT_SBGGR14_1X14
> 	0x301d: MEDIA_BUS_FMT_SBGGR16_1X16
> 	0x300e: MEDIA_BUS_FMT_SGBRG10_1X10
> 	0x3010: MEDIA_BUS_FMT_SGBRG12_1X12
> 	0x301a: MEDIA_BUS_FMT_SGBRG14_1X14
> 	0x301e: MEDIA_BUS_FMT_SGBRG16_1X16
> 	0x300a: MEDIA_BUS_FMT_SGRBG10_1X10
> 	0x3011: MEDIA_BUS_FMT_SGRBG12_1X12
> 	0x301b: MEDIA_BUS_FMT_SGRBG14_1X14
> 	0x301f: MEDIA_BUS_FMT_SGRBG16_1X16
> 	0x300f: MEDIA_BUS_FMT_SRGGB10_1X10
> 	0x3012: MEDIA_BUS_FMT_SRGGB12_1X12
> 	0x301c: MEDIA_BUS_FMT_SRGGB14_1X14
> 	0x3020: MEDIA_BUS_FMT_SRGGB16_1X16
> 	0x2001: MEDIA_BUS_FMT_Y8_1X8
> 	0x200a: MEDIA_BUS_FMT_Y10_1X10
> 	0x2013: MEDIA_BUS_FMT_Y12_1X12
> 
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
>  drivers/staging/media/imx/imx-media-utils.c | 19 +++++++++++++++----
>  1 file changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/staging/media/imx/imx-media-utils.c b/drivers/staging/media/imx/imx-media-utils.c
> index d61a8f4533dc..5f8604db4dd4 100644
> --- a/drivers/staging/media/imx/imx-media-utils.c
> +++ b/drivers/staging/media/imx/imx-media-utils.c
> @@ -254,7 +254,7 @@ static int enum_format(u32 *fourcc, u32 *code, u32 index,
>  		       bool allow_bayer)
>  {

This function is becoming confusing. I think you should add some comments explaining
what the function does. Specifically the fourcc and code arguments.

Can both be non-NULL? Or only one of the two? I think that if fourcc is non-NULL you
enumerate over the V4L2 pixelformats, if code is non-NULL, then you enumerate over
the mediabus codes.

If so, then I think it would be easier to understand if you just make two functions:
enum_formats and enum_codes, rather than trying to merge them into one.

Patches 1 and 2 of this series look good, so I'll take those.

Regards,

	Hans

>  	const struct imx_media_pixfmt *fmt;
> -	unsigned int i, j = 0;
> +	unsigned int i, j, k = 0;
>  
>  	for (i = 0; i < ARRAY_SIZE(pixel_formats); i++) {
>  		fmt = &pixel_formats[i];
> @@ -264,18 +264,29 @@ static int enum_format(u32 *fourcc, u32 *code, u32 index,
>  		    (!allow_bayer && fmt->bayer))
>  			continue;
>  
> -		if (index == j)
> +		if (fourcc && index == k)
>  			break;
>  
> -		j++;
> +		if (!code) {
> +			k++;
> +			continue;
> +		}
> +
> +		for (j = 0; j < ARRAY_SIZE(fmt->codes) && fmt->codes[j]; j++) {
> +			if (index == k)
> +				goto out;
> +
> +			k++;
> +		}
>  	}
>  	if (i == ARRAY_SIZE(pixel_formats))
>  		return -EINVAL;
>  
> +out:
>  	if (fourcc)
>  		*fourcc = fmt->fourcc;
>  	if (code)
> -		*code = fmt->codes[0];
> +		*code = fmt->codes[j];
>  
>  	return 0;
>  }
> 

Re: [PATCH 3/3] media: imx: fix media bus format enumeration

[Steve Longerbeam]

Hi Hans,

On 9/27/19 12:33 AM, Hans Verkuil wrote:
> On 9/12/19 6:01 PM, Philipp Zabel wrote:
>> Iterate over all media bus formats, not just over the first format in
>> each imx_media_pixfmt entry.
>>
>> Before:
>>
>>    $ v4l2-ctl -d $(media-ctl -e ipu1_csi0) --list-subdev-mbus-codes 0
>>    ioctl: VIDIOC_SUBDEV_ENUM_MBUS_CODE (pad=0)
>> 	0x2006: MEDIA_BUS_FMT_UYVY8_2X8
>> 	0x2008: MEDIA_BUS_FMT_YUYV8_2X8
>> 	0x1008: MEDIA_BUS_FMT_RGB565_2X8_LE
>> 	0x100a: MEDIA_BUS_FMT_RGB888_1X24
>> 	0x100d: MEDIA_BUS_FMT_ARGB8888_1X32
>> 	0x3001: MEDIA_BUS_FMT_SBGGR8_1X8
>> 	0x3013: MEDIA_BUS_FMT_SGBRG8_1X8
>> 	0x3002: MEDIA_BUS_FMT_SGRBG8_1X8
>> 	0x3014: MEDIA_BUS_FMT_SRGGB8_1X8
>> 	0x3007: MEDIA_BUS_FMT_SBGGR10_1X10
>> 	0x300e: MEDIA_BUS_FMT_SGBRG10_1X10
>> 	0x300a: MEDIA_BUS_FMT_SGRBG10_1X10
>> 	0x300f: MEDIA_BUS_FMT_SRGGB10_1X10
>> 	0x2001: MEDIA_BUS_FMT_Y8_1X8
>> 	0x200a: MEDIA_BUS_FMT_Y10_1X10
>>
>> After:
>>
>>    $ v4l2-ctl -d $(media-ctl -e ipu1_csi0) --list-subdev-mbus-codes 0
>>    ioctl: VIDIOC_SUBDEV_ENUM_MBUS_CODE (pad=0)
>> 	0x2006: MEDIA_BUS_FMT_UYVY8_2X8
>> 	0x200f: MEDIA_BUS_FMT_UYVY8_1X16
>> 	0x2008: MEDIA_BUS_FMT_YUYV8_2X8
>> 	0x2011: MEDIA_BUS_FMT_YUYV8_1X16
>> 	0x1008: MEDIA_BUS_FMT_RGB565_2X8_LE
>> 	0x100a: MEDIA_BUS_FMT_RGB888_1X24
>> 	0x100c: MEDIA_BUS_FMT_RGB888_2X12_LE
>> 	0x100d: MEDIA_BUS_FMT_ARGB8888_1X32
>> 	0x3001: MEDIA_BUS_FMT_SBGGR8_1X8
>> 	0x3013: MEDIA_BUS_FMT_SGBRG8_1X8
>> 	0x3002: MEDIA_BUS_FMT_SGRBG8_1X8
>> 	0x3014: MEDIA_BUS_FMT_SRGGB8_1X8
>> 	0x3007: MEDIA_BUS_FMT_SBGGR10_1X10
>> 	0x3008: MEDIA_BUS_FMT_SBGGR12_1X12
>> 	0x3019: MEDIA_BUS_FMT_SBGGR14_1X14
>> 	0x301d: MEDIA_BUS_FMT_SBGGR16_1X16
>> 	0x300e: MEDIA_BUS_FMT_SGBRG10_1X10
>> 	0x3010: MEDIA_BUS_FMT_SGBRG12_1X12
>> 	0x301a: MEDIA_BUS_FMT_SGBRG14_1X14
>> 	0x301e: MEDIA_BUS_FMT_SGBRG16_1X16
>> 	0x300a: MEDIA_BUS_FMT_SGRBG10_1X10
>> 	0x3011: MEDIA_BUS_FMT_SGRBG12_1X12
>> 	0x301b: MEDIA_BUS_FMT_SGRBG14_1X14
>> 	0x301f: MEDIA_BUS_FMT_SGRBG16_1X16
>> 	0x300f: MEDIA_BUS_FMT_SRGGB10_1X10
>> 	0x3012: MEDIA_BUS_FMT_SRGGB12_1X12
>> 	0x301c: MEDIA_BUS_FMT_SRGGB14_1X14
>> 	0x3020: MEDIA_BUS_FMT_SRGGB16_1X16
>> 	0x2001: MEDIA_BUS_FMT_Y8_1X8
>> 	0x200a: MEDIA_BUS_FMT_Y10_1X10
>> 	0x2013: MEDIA_BUS_FMT_Y12_1X12
>>
>> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
>> ---
>>   drivers/staging/media/imx/imx-media-utils.c | 19 +++++++++++++++----
>>   1 file changed, 15 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/staging/media/imx/imx-media-utils.c b/drivers/staging/media/imx/imx-media-utils.c
>> index d61a8f4533dc..5f8604db4dd4 100644
>> --- a/drivers/staging/media/imx/imx-media-utils.c
>> +++ b/drivers/staging/media/imx/imx-media-utils.c
>> @@ -254,7 +254,7 @@ static int enum_format(u32 *fourcc, u32 *code, u32 index,
>>   		       bool allow_bayer)
>>   {
> This function is becoming confusing. I think you should add some comments explaining
> what the function does. Specifically the fourcc and code arguments.
>
> Can both be non-NULL? Or only one of the two? I think that if fourcc is non-NULL you
> enumerate over the V4L2 pixelformats, if code is non-NULL, then you enumerate over
> the mediabus codes.
>
> If so, then I think it would be easier to understand if you just make two functions:
> enum_formats and enum_codes, rather than trying to merge them into one.

I don't think the function is that confusing, but I'm fine with 
splitting it into enum_formats() and enum_codes().

I do agree it needs some comments describing how it works. I think my 
suggestion to rename the index that counts entries that match the search 
criteria to "match_index" will also help to follow the code.


Steve

>
> Patches 1 and 2 of this series look good, so I'll take those.
>
> Regards,
>
> 	Hans
>
>>   	const struct imx_media_pixfmt *fmt;
>> -	unsigned int i, j = 0;
>> +	unsigned int i, j, k = 0;
>>   
>>   	for (i = 0; i < ARRAY_SIZE(pixel_formats); i++) {
>>   		fmt = &pixel_formats[i];
>> @@ -264,18 +264,29 @@ static int enum_format(u32 *fourcc, u32 *code, u32 index,
>>   		    (!allow_bayer && fmt->bayer))
>>   			continue;
>>   
>> -		if (index == j)
>> +		if (fourcc && index == k)
>>   			break;
>>   
>> -		j++;
>> +		if (!code) {
>> +			k++;
>> +			continue;
>> +		}
>> +
>> +		for (j = 0; j < ARRAY_SIZE(fmt->codes) && fmt->codes[j]; j++) {
>> +			if (index == k)
>> +				goto out;
>> +
>> +			k++;
>> +		}
>>   	}
>>   	if (i == ARRAY_SIZE(pixel_formats))
>>   		return -EINVAL;
>>   
>> +out:
>>   	if (fourcc)
>>   		*fourcc = fmt->fourcc;
>>   	if (code)
>> -		*code = fmt->codes[0];
>> +		*code = fmt->codes[j];
>>   
>>   	return 0;
>>   }
>>

Re: [PATCH 3/3] media: imx: fix media bus format enumeration

[Steve Longerbeam]

Hi Hans, Philipp,

On 10/25/19 2:48 PM, Steve Longerbeam wrote:
> Hi Hans,
>
> On 9/27/19 12:33 AM, Hans Verkuil wrote:
>> On 9/12/19 6:01 PM, Philipp Zabel wrote:
>>> Iterate over all media bus formats, not just over the first format in
>>> each imx_media_pixfmt entry.
>>>
>>> Before:
>>>
>>>    $ v4l2-ctl -d $(media-ctl -e ipu1_csi0) --list-subdev-mbus-codes 0
>>>    ioctl: VIDIOC_SUBDEV_ENUM_MBUS_CODE (pad=0)
>>>     0x2006: MEDIA_BUS_FMT_UYVY8_2X8
>>>     0x2008: MEDIA_BUS_FMT_YUYV8_2X8
>>>     0x1008: MEDIA_BUS_FMT_RGB565_2X8_LE
>>>     0x100a: MEDIA_BUS_FMT_RGB888_1X24
>>>     0x100d: MEDIA_BUS_FMT_ARGB8888_1X32
>>>     0x3001: MEDIA_BUS_FMT_SBGGR8_1X8
>>>     0x3013: MEDIA_BUS_FMT_SGBRG8_1X8
>>>     0x3002: MEDIA_BUS_FMT_SGRBG8_1X8
>>>     0x3014: MEDIA_BUS_FMT_SRGGB8_1X8
>>>     0x3007: MEDIA_BUS_FMT_SBGGR10_1X10
>>>     0x300e: MEDIA_BUS_FMT_SGBRG10_1X10
>>>     0x300a: MEDIA_BUS_FMT_SGRBG10_1X10
>>>     0x300f: MEDIA_BUS_FMT_SRGGB10_1X10
>>>     0x2001: MEDIA_BUS_FMT_Y8_1X8
>>>     0x200a: MEDIA_BUS_FMT_Y10_1X10
>>>
>>> After:
>>>
>>>    $ v4l2-ctl -d $(media-ctl -e ipu1_csi0) --list-subdev-mbus-codes 0
>>>    ioctl: VIDIOC_SUBDEV_ENUM_MBUS_CODE (pad=0)
>>>     0x2006: MEDIA_BUS_FMT_UYVY8_2X8
>>>     0x200f: MEDIA_BUS_FMT_UYVY8_1X16
>>>     0x2008: MEDIA_BUS_FMT_YUYV8_2X8
>>>     0x2011: MEDIA_BUS_FMT_YUYV8_1X16
>>>     0x1008: MEDIA_BUS_FMT_RGB565_2X8_LE
>>>     0x100a: MEDIA_BUS_FMT_RGB888_1X24
>>>     0x100c: MEDIA_BUS_FMT_RGB888_2X12_LE
>>>     0x100d: MEDIA_BUS_FMT_ARGB8888_1X32
>>>     0x3001: MEDIA_BUS_FMT_SBGGR8_1X8
>>>     0x3013: MEDIA_BUS_FMT_SGBRG8_1X8
>>>     0x3002: MEDIA_BUS_FMT_SGRBG8_1X8
>>>     0x3014: MEDIA_BUS_FMT_SRGGB8_1X8
>>>     0x3007: MEDIA_BUS_FMT_SBGGR10_1X10
>>>     0x3008: MEDIA_BUS_FMT_SBGGR12_1X12
>>>     0x3019: MEDIA_BUS_FMT_SBGGR14_1X14
>>>     0x301d: MEDIA_BUS_FMT_SBGGR16_1X16
>>>     0x300e: MEDIA_BUS_FMT_SGBRG10_1X10
>>>     0x3010: MEDIA_BUS_FMT_SGBRG12_1X12
>>>     0x301a: MEDIA_BUS_FMT_SGBRG14_1X14
>>>     0x301e: MEDIA_BUS_FMT_SGBRG16_1X16
>>>     0x300a: MEDIA_BUS_FMT_SGRBG10_1X10
>>>     0x3011: MEDIA_BUS_FMT_SGRBG12_1X12
>>>     0x301b: MEDIA_BUS_FMT_SGRBG14_1X14
>>>     0x301f: MEDIA_BUS_FMT_SGRBG16_1X16
>>>     0x300f: MEDIA_BUS_FMT_SRGGB10_1X10
>>>     0x3012: MEDIA_BUS_FMT_SRGGB12_1X12
>>>     0x301c: MEDIA_BUS_FMT_SRGGB14_1X14
>>>     0x3020: MEDIA_BUS_FMT_SRGGB16_1X16
>>>     0x2001: MEDIA_BUS_FMT_Y8_1X8
>>>     0x200a: MEDIA_BUS_FMT_Y10_1X10
>>>     0x2013: MEDIA_BUS_FMT_Y12_1X12
>>>
>>> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
>>> ---
>>>   drivers/staging/media/imx/imx-media-utils.c | 19 +++++++++++++++----
>>>   1 file changed, 15 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/staging/media/imx/imx-media-utils.c 
>>> b/drivers/staging/media/imx/imx-media-utils.c
>>> index d61a8f4533dc..5f8604db4dd4 100644
>>> --- a/drivers/staging/media/imx/imx-media-utils.c
>>> +++ b/drivers/staging/media/imx/imx-media-utils.c
>>> @@ -254,7 +254,7 @@ static int enum_format(u32 *fourcc, u32 *code, 
>>> u32 index,
>>>                  bool allow_bayer)
>>>   {
>> This function is becoming confusing. I think you should add some 
>> comments explaining
>> what the function does. Specifically the fourcc and code arguments.
>>
>> Can both be non-NULL? Or only one of the two? I think that if fourcc 
>> is non-NULL you
>> enumerate over the V4L2 pixelformats, if code is non-NULL, then you 
>> enumerate over
>> the mediabus codes.
>>
>> If so, then I think it would be easier to understand if you just make 
>> two functions:
>> enum_formats and enum_codes, rather than trying to merge them into one.
>
> I don't think the function is that confusing, but I'm fine with 
> splitting it into enum_formats() and enum_codes().
>
> I do agree it needs some comments describing how it works. I think my 
> suggestion to rename the index that counts entries that match the 
> search criteria to "match_index" will also help to follow the code.
>

I added a comment block for find_format() and enum_format() in my 
media-tree github fork:

git@github.com:slongerbeam/mediatree.git, branch imx/philipp-pixfmts-cleanup

Steve