Re: [V9, 2/2] media: i2c: ov02a10: Add OV02A10 image sensor driver

[Sakari Ailus <sakari.ailus@linux.intel.com>]

On Thu, Jun 11, 2020 at 11:57:43AM +0200, Tomasz Figa wrote:
> On Thu, Jun 11, 2020 at 11:53 AM Sakari Ailus
> <sakari.ailus@linux.intel.com> wrote:
> >
> > Hi Tomasz,
> >
> > On Wed, Jun 10, 2020 at 07:44:55PM +0000, Tomasz Figa wrote:
> > > Hi Dongchun,
> > >
> > > On Sat, May 23, 2020 at 04:41:03PM +0800, Dongchun Zhu wrote:
> > > > Add a V4L2 sub-device driver for OV02A10 image sensor.
> > > >
> > > > Signed-off-by: Dongchun Zhu <dongchun.zhu@mediatek.com>
> > > > ---
> > > >  MAINTAINERS                 |    1 +
> > > >  drivers/media/i2c/Kconfig   |   13 +
> > > >  drivers/media/i2c/Makefile  |    1 +
> > > >  drivers/media/i2c/ov02a10.c | 1025 +++++++++++++++++++++++++++++++++++++++++++
> > > >  4 files changed, 1040 insertions(+)
> > > >  create mode 100644 drivers/media/i2c/ov02a10.c
> > > >
> > >
> > > Thank you for the patch. Please see my comments inline.
> > >
> > > [snip]
> > > > diff --git a/drivers/media/i2c/ov02a10.c b/drivers/media/i2c/ov02a10.c
> > > > new file mode 100644
> > > > index 0000000..160a0b5
> > > > --- /dev/null
> > > > +++ b/drivers/media/i2c/ov02a10.c
> > > [snip]
> > > > +static const char * const ov02a10_test_pattern_menu[] = {
> > > > +   "Disabled",
> > > > +   "Color Bar",
> > >
> > > nit: We should normalize this to one of the standard names. What is the
> > > pattern on this sensor? Is it perhaps "Eight Vertical Colour Bars"?
> > >
> > > > +};
> > > [snip]
> > > > +static int ov02a10_set_fmt(struct v4l2_subdev *sd,
> > > > +                      struct v4l2_subdev_pad_config *cfg,
> > > > +                      struct v4l2_subdev_format *fmt)
> > > > +{
> > > > +   struct ov02a10 *ov02a10 = to_ov02a10(sd);
> > > > +   struct v4l2_mbus_framefmt *mbus_fmt = &fmt->format;
> > > > +
> > > > +   mutex_lock(&ov02a10->mutex);
> > > > +
> > >
> > >
> > > Don't we need to handle the case when fmt->which is V4L2_SUBDEV_FORMAT_TRY,
> > > which is used for trying the format, but not applying it to the hardware?
> >
> > Yes.
> >
> > >
> > > > +   if (ov02a10->streaming) {
> > > > +           mutex_unlock(&ov02a10->mutex);
> > > > +           return -EBUSY;
> > > > +   }
> > > > +
> > > > +   /* Only one sensor mode supported */
> > > > +   mbus_fmt->code = ov02a10->fmt.code;
> > > > +   ov02a10_fill_fmt(ov02a10->cur_mode, mbus_fmt);
> > > > +   ov02a10->fmt = fmt->format;
> > > > +
> > > > +   mutex_unlock(&ov02a10->mutex);
> > > > +
> > > > +   return 0;
> > > > +}
> > > > +
> > > > +static int ov02a10_get_fmt(struct v4l2_subdev *sd,
> > > > +                      struct v4l2_subdev_pad_config *cfg,
> > > > +                      struct v4l2_subdev_format *fmt)
> > > > +{
> > > > +   struct ov02a10 *ov02a10 = to_ov02a10(sd);
> > > > +   struct v4l2_mbus_framefmt *mbus_fmt = &fmt->format;
> > > > +
> > > > +   mutex_lock(&ov02a10->mutex);
> > > > +
> > > > +   fmt->format = ov02a10->fmt;
> > >
> > > Ditto.
> > >
> > > > +   mbus_fmt->code = ov02a10->fmt.code;
> > > > +   ov02a10_fill_fmt(ov02a10->cur_mode, mbus_fmt);
> > > > +
> > > > +   mutex_unlock(&ov02a10->mutex);
> > > > +
> > > > +   return 0;
> > > > +}
> > > > +
> > > > +static int ov02a10_enum_mbus_code(struct v4l2_subdev *sd,
> > > > +                             struct v4l2_subdev_pad_config *cfg,
> > > > +                             struct v4l2_subdev_mbus_code_enum *code)
> > > > +{
> > > > +   struct ov02a10 *ov02a10 = to_ov02a10(sd);
> > > > +
> > > > +   if (code->index >= ARRAY_SIZE(supported_modes))
> > > > +           return -EINVAL;
> > >
> > > Hmm, supported_modes[] doesn't seem to hold the information about mbus
> > > codes. Should this just perhaps be "!= 0"?
> > >
> > > > +
> > > > +   code->code = ov02a10->fmt.code;
> > > > +
> > > > +   return 0;
> > > > +}
> > > [snip]
> > > > +static int ov02a10_entity_init_cfg(struct v4l2_subdev *sd,
> > > > +                              struct v4l2_subdev_pad_config *cfg)
> > > > +{
> > > > +   struct v4l2_subdev_format fmt = {
> > > > +           .which = cfg ? V4L2_SUBDEV_FORMAT_TRY : V4L2_SUBDEV_FORMAT_ACTIVE,
> > > > +           .format = {
> > > > +                   .width = 1600,
> > > > +                   .height = 1200,
> > > > +           }
> > > > +   };
> > > > +
> > > > +   ov02a10_set_fmt(sd, cfg, &fmt);
> > > > +
> > > > +   return 0;
> > > > +}
> > > > +
> > >
> > > I'm not familiar with this init_cfg operation and the documentation is very
> > > sparse about it. Sakari, is this a correct implementation?
> >
> > The purpose is to initialise a pad configuration (format and selection
> > rectangles) to the device defaults. As there seem to be no selection
> > rectangles, this seems fine to me.
> 
> Thanks. I traced the code and could only see one place where the
> callback is being called and that was with cfg != NULL. Still, the
> code above uses "cfg ?" as a check to determine whether TRY or ACTIVE
> should be passed to which. Is that also correct?

It could be used in setting the active format in probe. That would probably
be cleaner than what it currently does.

But apart from that, the framework always calls init_cfg with cfg non-NULL.

-- 
Sakari Ailus