Re: [PATCH 16/16] media: i2c: ov9282: Support event handlers

[Jacopo Mondi <jacopo@jmondi.org>]

On Fri, Oct 07, 2022 at 11:22:23AM +0100, Dave Stevenson wrote:
> Hi Jacopo
>
> + Hans for comment on compliance / controls framework
>
> On Thu, 6 Oct 2022 at 10:59, Jacopo Mondi <jacopo@jmondi.org> wrote:
> >
> > Hi Dave
> >
> > On Wed, Oct 05, 2022 at 04:28:09PM +0100, Dave Stevenson wrote:
> > > As noted in the headers for V4L2_SUBDEV_FL_HAS_EVENTS,
> > > "controls can send events, thus drivers exposing controls
> > > should set this flag".
> > >
> >
> > I was expecting this to only apply to drivers that actually generate
> > events...
> >
> > Not sure I can give a tag here as my understanding of this part is
> > limited, let's wait for other opinions :)
>
> I had to rack my memory on this one.
>
> It fixes a v4l2-compliance failure:
> fail: v4l2-test-controls.cpp(835): subscribe event for control 'User
> Controls' failed
> test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: FAIL
> (v4l-utils built at ToT from today, so fd544473800 "support INTEGER
> and INTEGER64 control arrays")
>
> So either it is required by all drivers that expose controls, or it's
> an issue in v4l2-compliance.
> I believe it's the former as all controls can create a V4L2_EVENT_CTRL
> event should the value or range change (very common for things like
> HBLANK and VBLANK in image sensor drivers).
>

It seems only 19 sensor drivers in total implement a .subscribe_event
function... let's say there's room for improvements :)

> Thanks
>   Dave
>
> > > This driver exposes controls, but didn't reflect that it
> > > could generate events. Correct this, and add the default
> > > event handler functions.
> > >
> > > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > > ---
> > >  drivers/media/i2c/ov9282.c | 10 +++++++++-
> > >  1 file changed, 9 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> > > index bc429455421e..416c9656e3ac 100644
> > > --- a/drivers/media/i2c/ov9282.c
> > > +++ b/drivers/media/i2c/ov9282.c
> > > @@ -14,6 +14,7 @@
> > >  #include <linux/regulator/consumer.h>
> > >
> > >  #include <media/v4l2-ctrls.h>
> > > +#include <media/v4l2-event.h>
> > >  #include <media/v4l2-fwnode.h>
> > >  #include <media/v4l2-subdev.h>
> > >
> > > @@ -1189,6 +1190,11 @@ static int ov9282_parse_hw_config(struct ov9282 *ov9282)
> > >  }
> > >
> > >  /* V4l2 subdevice ops */
> > > +static const struct v4l2_subdev_core_ops ov9282_core_ops = {
> > > +     .subscribe_event = v4l2_ctrl_subdev_subscribe_event,
> > > +     .unsubscribe_event = v4l2_event_subdev_unsubscribe,
> > > +};
> > > +
> > >  static const struct v4l2_subdev_video_ops ov9282_video_ops = {
> > >       .s_stream = ov9282_set_stream,
> > >  };
> > > @@ -1203,6 +1209,7 @@ static const struct v4l2_subdev_pad_ops ov9282_pad_ops = {
> > >  };
> > >
> > >  static const struct v4l2_subdev_ops ov9282_subdev_ops = {
> > > +     .core = &ov9282_core_ops,
> > >       .video = &ov9282_video_ops,
> > >       .pad = &ov9282_pad_ops,
> > >  };
> > > @@ -1419,7 +1426,8 @@ static int ov9282_probe(struct i2c_client *client)
> > >       }
> > >
> > >       /* Initialize subdev */
> > > -     ov9282->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> > > +     ov9282->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE |
> > > +                         V4L2_SUBDEV_FL_HAS_EVENTS;
> > >       ov9282->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
> > >
> > >       /* Initialize source pad */
> > > --
> > > 2.34.1
> > >