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 <[email protected]>
---
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
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 <[email protected]>
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 <[email protected]>
--
Luca
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 <[email protected]>
>
> 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 <[email protected]>
>
> --
> Luca
--
Sakari Ailus
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 <[email protected]>
>>
>> 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 <[email protected]>
>>
>> --
>> Luca
>
> --
> Sakari Ailus
>
Hi,
On 18/11/21 18:34, [email protected] 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 <[email protected]>
>>>
>>> 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
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 <[email protected]>
>>
>> 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
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 <[email protected]>
> >>
> >> 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