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

[Steve Longerbeam <slongerbeam@gmail.com>]

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