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

[Steve Longerbeam <slongerbeam@gmail.com>]

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;
>>   }
>>