Re: [PATCH] media: i2c: imx274: implement enum_mbus_code

[<Eugen.Hristev@microchip.com>]

On 11/18/21 7:26 PM, Sakari Ailus wrote:
> Hi Luca,
> 
> On Thu, Nov 18, 2021 at 06:11:35PM +0100, Luca Ceresoli wrote:
>> Hi Eugen,
>>
>> On 18/11/21 16:40, Eugen Hristev wrote:
>>> Current driver supports only SRGGB 10 bit RAW bayer format.
>>> Add the enum_mbus_code implementation to report this format supported.
>>>
>>>   # v4l2-ctl -d /dev/v4l-subdev3 --list-subdev-mbus-codes
>>> ioctl: VIDIOC_SUBDEV_ENUM_MBUS_CODE (pad=0)
>>>          0x300f: MEDIA_BUS_FMT_SRGGB10_1X10
>>>   #
>>>
>>> Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
>>
>> Generally OK, but I have a few minor comments.
>>
>>> ---
>>>   drivers/media/i2c/imx274.c | 14 ++++++++++++++
>>>   1 file changed, 14 insertions(+)
>>>
>>> diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
>>> index 2e804e3b70c4..25a4ef8f6187 100644
>>> --- a/drivers/media/i2c/imx274.c
>>> +++ b/drivers/media/i2c/imx274.c
>>> @@ -1909,7 +1909,21 @@ static int imx274_set_frame_interval(struct stimx274 *priv,
>>>      return err;
>>>   }
>>>
>>> +static int imx274_enum_mbus_code(struct v4l2_subdev *sd,
>>> +                            struct v4l2_subdev_state *sd_state,
>>> +                            struct v4l2_subdev_mbus_code_enum *code)
>>> +{
>>> +   if (code->index > 0)
>>> +           return -EINVAL;
>>
>> Many driver do check code->pad too, so you might want to do
>>
>>        if (code->pad > 0 || code->index > 0)
>>                return -EINVAL;
> 
> The caller will have checked the pad exists, and there's a single one on
> the subdev I suppose.
> 
>>
>> However I don't think it is strictly necessary, thus
>>
>>> +
>>> +   /* only supported format in the driver is Raw 10 bits SRGGB */
>>> +   code->code = MEDIA_BUS_FMT_SRGGB10_1X10;
>>
>> Maybe better:
>>
>>    code->code =  to_imx274(sd)->format.code
> 
> Good idea.

Hi,

Initially I thought about this, but my idea was to keep it simple.
If we return format.code, we are not enumerating anything, just 
returning the current format and that's it.

If we want to be correct, I would rather add a struct with supported 
formats(currently just one ) and iterate through this structure.

If in the future we want to support more formats (I see this sensor 
could support SRGGB 12 bits ), then it would support 2 formats, and 
returning priv->format.code would be incorrect here (it would be correct 
for a g_fmt only )

So, how do you think I should proceed ?
1/ Create a struct with a single element and iterate through it
2/ Leave it like this and always return SRGGB10
3/ Do like Luca suggests and return format.code (which I am personally 
against )

Thanks for reviewing !

Eugen

> 
>>
>> just as a little guard for future format changes.
>>
>> With or without these I'm OK anyway with the patch, so:
>>
>> Reviewed-by: Luca Ceresoli <luca@lucaceresoli.net>
>>
>> --
>> Luca
> 
> --
> Sakari Ailus
>