2019年1月3日(木) 22:47 Marco Felsch <m.felsch@xxxxxxxxxxxxxx>:
>
> On 19-01-01 02:07, Akinobu Mita wrote:
> > 2018年12月31日(月) 19:54 Marco Felsch <m.felsch@xxxxxxxxxxxxxx>:
> > >
> > > On 18-12-30 02:07, Akinobu Mita wrote:
> > > > The VIDIOC_SUBDEV_G_FMT ioctl for this driver doesn't recognize
> > > > V4L2_SUBDEV_FORMAT_TRY and always works as if V4L2_SUBDEV_FORMAT_ACTIVE
> > > > is specified.
> > > >
> > > > Cc: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> > > > Cc: Mauro Carvalho Chehab <mchehab@xxxxxxxxxx>
> > > > Signed-off-by: Akinobu Mita <akinobu.mita@xxxxxxxxx>
> > > > ---
> > > > drivers/media/i2c/mt9m111.c | 31 +++++++++++++++++++++++++++++++
> > > > 1 file changed, 31 insertions(+)
> > > >
> > > > diff --git a/drivers/media/i2c/mt9m111.c b/drivers/media/i2c/mt9m111.c
> > > > index f0e47fd..acb4dee 100644
> > > > --- a/drivers/media/i2c/mt9m111.c
> > > > +++ b/drivers/media/i2c/mt9m111.c
> > > > @@ -528,6 +528,16 @@ static int mt9m111_get_fmt(struct v4l2_subdev *sd,
> > > > if (format->pad)
> > > > return -EINVAL;
> > > >
> > > > + if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
> > > > +#ifdef CONFIG_VIDEO_V4L2_SUBDEV_API
> > >
> > > This ifdef is made in the include/media/v4l2-subdev.h, so I would drop
> > > it.
> >
> > I sent similar fix for ov2640 driver and kerel test robot reported
> > build test failure. So I think this ifdef is necessary.
> >
> > v1: https://www.mail-archive.com/linux-media@xxxxxxxxxxxxxxx/msg137098.html
> > v2: https://www.mail-archive.com/linux-media@xxxxxxxxxxxxxxx/msg141735.html
>
> You are absolutely true, sorry my mistake.. Unfortunately my patch [1] wasn't
> applied which fixes it commonly. This patch will avoid the 2nd ifdef in
> init_cfg() too.
>
> [1] https://www.spinics.net/lists/linux-media/msg138940.html
>
> >
> > > > + mf = v4l2_subdev_get_try_format(sd, cfg, 0);
> > >
> > > I would use format->pad instead of the static 0.
> >
> > OK.
> >
> > > > + format->format = *mf;
> > >
> > > Is this correct? I tought v4l2_subdev_get_try_format() will return the
> > > try_pad which we need to fill.
> >
> > I think this is correct. Other sensor drivers are doing the same thing in
> > get_fmt() callback.
>
> Yes, you're right.
>
> > > > + return 0;
> > > > +#else
> > > > + return -ENOTTY;
> > >
> > > Return this error is not specified in the API-Doc.
> >
> > I think this 'return -ENOTTY' is not reachable even if
> > CONFIG_VIDEO_V4L2_SUBDEV_API is not set, and can be replaced with any
> > return value.
>
> Sorry I didn't catched this. When it's not reachable why is it there and
> why isn't it reachable? If the format->which = V4L2_SUBDEV_FORMAT_TRY
> and we don't configure CONFIG_VIDEO_V4L2_SUBDEV_API, then this path will
> be reached, or overlooked I something?
As far as I can see, when CONFIG_VIDEO_V4L2_SUBDEV_API is not defined,
the get_fmt() callback is always called with
'format->which == V4L2_SUBDEV_FORMAT_ACTIVE'.
There is only one call site that the get_fmt() is called with
'format->which == V4L2_SUBDEV_FORMAT_TRY' in
drivers/media/v4l2-core/v4l2-subdev.c: subdev_do_ioctl().
But the call site is enclosed by ifdef CONFIG_VIDEO_V4L2_SUBDEV_API.
So the hunk of the patch can be changed to:
if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
#ifdef CONFIG_VIDEO_V4L2_SUBDEV_API
mf = v4l2_subdev_get_try_format(sd, cfg, format->pad);
format->format = *mf;
return 0;
#endif
}
