| Message ID | 20211118154009.307430-1-eugen.hristev@microchip.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: i2c: imx274: implement enum_mbus_code | expand |
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>
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
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 >
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.
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?
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.
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,
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(+)