[PATCH] media: i2c: imx274: implement enum_mbus_code

[PATCH] media: i2c: imx274: implement enum_mbus_code

[Eugen Hristev]

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>
---
 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;
+
+	/* only supported format in the driver is Raw 10 bits SRGGB */
+	code->code = MEDIA_BUS_FMT_SRGGB10_1X10;
+
+	return 0;
+}
+
 static const struct v4l2_subdev_pad_ops imx274_pad_ops = {
+	.enum_mbus_code = imx274_enum_mbus_code,
 	.get_fmt = imx274_get_fmt,
 	.set_fmt = imx274_set_fmt,
 	.get_selection = imx274_get_selection,
-- 
2.25.1

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

[Luca Ceresoli]

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;

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

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

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

[Sakari Ailus]

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.

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

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

[Eugen.Hristev]

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
> 

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

[Luca Ceresoli]

Hi,

On 18/11/21 18:34, Eugen.Hristev@microchip.com wrote:
> 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

I dislike adding complexity (albeit small) that adds no features. Let's
leave this idea to the day somebody adds another format.

> 2/ Leave it like this and always return SRGGB10
> 3/ Do like Luca suggests and return format.code (which I am personally 
> against )

No strong preference between 2 and 3.

-- 
Luca

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

[Luca Ceresoli]

Hi,

On 18/11/21 18:26, 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.

Thanks for your explanation. That's very reasonable indeed.

Now, why do many drivers do that? Old checks that later turned useless
and nobody ever removed?

-- 
Luca

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

[Sakari Ailus]

Hi Luca,

On Thu, Nov 18, 2021 at 06:39:09PM +0100, Luca Ceresoli wrote:
> Hi,
> 
> On 18/11/21 18:26, 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.
> 
> Thanks for your explanation. That's very reasonable indeed.
> 
> Now, why do many drivers do that? Old checks that later turned useless
> and nobody ever removed?

Some of those exists, that's for sure. The check was added to the callers a
few years ago I think. Many drivers also have more pads but then they
typically return something else than -EINVAL for the other pads.

-- 
Sakari Ailus