| Message ID | 20240910134428.795273-2-tomm.merciai@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | media: i2c: max96714/max96717 | expand |
Hi Tommaso, On Tue, Sep 10, 2024 at 03:44:27PM +0200, Tommaso Merciai wrote: > Controls can be exposed to userspace via a v4l-subdevX device, and > userspace has to be able to subscribe to control events so that it is > notified when the control changes value. Add missing HAS_EVENTS flag. How is this supposed to work, given that the driver doesn't implement .subscribe_event() ? > Signed-off-by: Tommaso Merciai <tomm.merciai@gmail.com> > --- > drivers/media/i2c/max96714.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/media/i2c/max96714.c b/drivers/media/i2c/max96714.c > index 159753b13777..94b1bc000e48 100644 > --- a/drivers/media/i2c/max96714.c > +++ b/drivers/media/i2c/max96714.c > @@ -602,7 +602,8 @@ static int max96714_create_subdev(struct max96714_priv *priv) > goto err_free_ctrl; > } > > - priv->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_STREAMS; > + priv->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | > + V4L2_SUBDEV_FL_HAS_EVENTS | V4L2_SUBDEV_FL_STREAMS; > priv->sd.entity.function = MEDIA_ENT_F_VID_IF_BRIDGE; > priv->sd.entity.ops = &max96714_entity_ops; >
Hi Laurent, On Thu, Sep 12, 2024 at 01:44:09PM +0300, Laurent Pinchart wrote: > Hi Tommaso, > > On Tue, Sep 10, 2024 at 03:44:27PM +0200, Tommaso Merciai wrote: > > Controls can be exposed to userspace via a v4l-subdevX device, and > > userspace has to be able to subscribe to control events so that it is > > notified when the control changes value. Add missing HAS_EVENTS flag. > > How is this supposed to work, given that the driver doesn't implement > .subscribe_event() ? You are completely right, sorry. I think in both cases I'm missing: diff --git a/drivers/media/i2c/max96714.c b/drivers/media/i2c/max96714.c index 94b1bc000e48..2257b6b807ea 100644 --- a/drivers/media/i2c/max96714.c +++ b/drivers/media/i2c/max96714.c @@ -17,6 +17,7 @@ #include <media/v4l2-cci.h> #include <media/v4l2-ctrls.h> +#include <media/v4l2-event.h> #include <media/v4l2-fwnode.h> #include <media/v4l2-subdev.h> @@ -488,6 +489,8 @@ static int max96714_log_status(struct v4l2_subdev *sd) static const struct v4l2_subdev_core_ops max96714_subdev_core_ops = { .log_status = max96714_log_status, + .subscribe_event = v4l2_ctrl_subdev_subscribe_event, + .unsubscribe_event = v4l2_event_subdev_unsubscribe, }; static const struct v4l2_subdev_video_ops max96714_video_ops = { Like you suggest. Or I'm wrong? Thanks & Regards, Tommaso > > > Signed-off-by: Tommaso Merciai <tomm.merciai@gmail.com> > > --- > > drivers/media/i2c/max96714.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/media/i2c/max96714.c b/drivers/media/i2c/max96714.c > > index 159753b13777..94b1bc000e48 100644 > > --- a/drivers/media/i2c/max96714.c > > +++ b/drivers/media/i2c/max96714.c > > @@ -602,7 +602,8 @@ static int max96714_create_subdev(struct max96714_priv *priv) > > goto err_free_ctrl; > > } > > > > - priv->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_STREAMS; > > + priv->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | > > + V4L2_SUBDEV_FL_HAS_EVENTS | V4L2_SUBDEV_FL_STREAMS; > > priv->sd.entity.function = MEDIA_ENT_F_VID_IF_BRIDGE; > > priv->sd.entity.ops = &max96714_entity_ops; > > > > -- > Regards, > > Laurent Pinchart
On Thu, Sep 12, 2024 at 01:11:13PM +0200, Tommaso Merciai wrote: > Hi Laurent, > > On Thu, Sep 12, 2024 at 01:44:09PM +0300, Laurent Pinchart wrote: > > Hi Tommaso, > > > > On Tue, Sep 10, 2024 at 03:44:27PM +0200, Tommaso Merciai wrote: > > > Controls can be exposed to userspace via a v4l-subdevX device, and > > > userspace has to be able to subscribe to control events so that it is > > > notified when the control changes value. Add missing HAS_EVENTS flag. > > > > How is this supposed to work, given that the driver doesn't implement > > .subscribe_event() ? > > You are completely right, sorry. > I think in both cases I'm missing: > > diff --git a/drivers/media/i2c/max96714.c b/drivers/media/i2c/max96714.c > index 94b1bc000e48..2257b6b807ea 100644 > --- a/drivers/media/i2c/max96714.c > +++ b/drivers/media/i2c/max96714.c > @@ -17,6 +17,7 @@ > > #include <media/v4l2-cci.h> > #include <media/v4l2-ctrls.h> > +#include <media/v4l2-event.h> > #include <media/v4l2-fwnode.h> > #include <media/v4l2-subdev.h> > > @@ -488,6 +489,8 @@ static int max96714_log_status(struct v4l2_subdev *sd) > > static const struct v4l2_subdev_core_ops max96714_subdev_core_ops = { > .log_status = max96714_log_status, > + .subscribe_event = v4l2_ctrl_subdev_subscribe_event, > + .unsubscribe_event = v4l2_event_subdev_unsubscribe, > }; > > static const struct v4l2_subdev_video_ops max96714_video_ops = { > > Like you suggest. Or I'm wrong? That looks better :-) Out of curiosity, what's your use case for control events ? > > > Signed-off-by: Tommaso Merciai <tomm.merciai@gmail.com> > > > --- > > > drivers/media/i2c/max96714.c | 3 ++- > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/media/i2c/max96714.c b/drivers/media/i2c/max96714.c > > > index 159753b13777..94b1bc000e48 100644 > > > --- a/drivers/media/i2c/max96714.c > > > +++ b/drivers/media/i2c/max96714.c > > > @@ -602,7 +602,8 @@ static int max96714_create_subdev(struct max96714_priv *priv) > > > goto err_free_ctrl; > > > } > > > > > > - priv->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_STREAMS; > > > + priv->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | > > > + V4L2_SUBDEV_FL_HAS_EVENTS | V4L2_SUBDEV_FL_STREAMS; > > > priv->sd.entity.function = MEDIA_ENT_F_VID_IF_BRIDGE; > > > priv->sd.entity.ops = &max96714_entity_ops; > > >
Hi Laurent, On Thu, Sep 12, 2024 at 03:05:10PM +0300, Laurent Pinchart wrote: > On Thu, Sep 12, 2024 at 01:11:13PM +0200, Tommaso Merciai wrote: > > Hi Laurent, > > > > On Thu, Sep 12, 2024 at 01:44:09PM +0300, Laurent Pinchart wrote: > > > Hi Tommaso, > > > > > > On Tue, Sep 10, 2024 at 03:44:27PM +0200, Tommaso Merciai wrote: > > > > Controls can be exposed to userspace via a v4l-subdevX device, and > > > > userspace has to be able to subscribe to control events so that it is > > > > notified when the control changes value. Add missing HAS_EVENTS flag. > > > > > > How is this supposed to work, given that the driver doesn't implement > > > .subscribe_event() ? > > > > You are completely right, sorry. > > I think in both cases I'm missing: > > > > diff --git a/drivers/media/i2c/max96714.c b/drivers/media/i2c/max96714.c > > index 94b1bc000e48..2257b6b807ea 100644 > > --- a/drivers/media/i2c/max96714.c > > +++ b/drivers/media/i2c/max96714.c > > @@ -17,6 +17,7 @@ > > > > #include <media/v4l2-cci.h> > > #include <media/v4l2-ctrls.h> > > +#include <media/v4l2-event.h> > > #include <media/v4l2-fwnode.h> > > #include <media/v4l2-subdev.h> > > > > @@ -488,6 +489,8 @@ static int max96714_log_status(struct v4l2_subdev *sd) > > > > static const struct v4l2_subdev_core_ops max96714_subdev_core_ops = { > > .log_status = max96714_log_status, > > + .subscribe_event = v4l2_ctrl_subdev_subscribe_event, > > + .unsubscribe_event = v4l2_event_subdev_unsubscribe, > > }; > > > > static const struct v4l2_subdev_video_ops max96714_video_ops = { > > > > Like you suggest. Or I'm wrong? > > That looks better :-) > > Out of curiosity, what's your use case for control events ? I'm not sure if there's a use case, however control event should be supported when a driver supports controls.
On Thu, Sep 12, 2024 at 12:14:52PM +0000, Sakari Ailus wrote: > On Thu, Sep 12, 2024 at 03:05:10PM +0300, Laurent Pinchart wrote: > > On Thu, Sep 12, 2024 at 01:11:13PM +0200, Tommaso Merciai wrote: > > > On Thu, Sep 12, 2024 at 01:44:09PM +0300, Laurent Pinchart wrote: > > > > On Tue, Sep 10, 2024 at 03:44:27PM +0200, Tommaso Merciai wrote: > > > > > Controls can be exposed to userspace via a v4l-subdevX device, and > > > > > userspace has to be able to subscribe to control events so that it is > > > > > notified when the control changes value. Add missing HAS_EVENTS flag. > > > > > > > > How is this supposed to work, given that the driver doesn't implement > > > > .subscribe_event() ? > > > > > > You are completely right, sorry. > > > I think in both cases I'm missing: > > > > > > diff --git a/drivers/media/i2c/max96714.c b/drivers/media/i2c/max96714.c > > > index 94b1bc000e48..2257b6b807ea 100644 > > > --- a/drivers/media/i2c/max96714.c > > > +++ b/drivers/media/i2c/max96714.c > > > @@ -17,6 +17,7 @@ > > > > > > #include <media/v4l2-cci.h> > > > #include <media/v4l2-ctrls.h> > > > +#include <media/v4l2-event.h> > > > #include <media/v4l2-fwnode.h> > > > #include <media/v4l2-subdev.h> > > > > > > @@ -488,6 +489,8 @@ static int max96714_log_status(struct v4l2_subdev *sd) > > > > > > static const struct v4l2_subdev_core_ops max96714_subdev_core_ops = { > > > .log_status = max96714_log_status, > > > + .subscribe_event = v4l2_ctrl_subdev_subscribe_event, > > > + .unsubscribe_event = v4l2_event_subdev_unsubscribe, > > > }; > > > > > > static const struct v4l2_subdev_video_ops max96714_video_ops = { > > > > > > Like you suggest. Or I'm wrong? > > > > That looks better :-) > > > > Out of curiosity, what's your use case for control events ? > > I'm not sure if there's a use case, however control event should be > supported when a driver supports controls. Not disputing that. That's why I asked "out of curiosity".
Hi Laurent, Sakari, Thanks for your comments. On Thu, Sep 12, 2024 at 03:18:16PM +0300, Laurent Pinchart wrote: > On Thu, Sep 12, 2024 at 12:14:52PM +0000, Sakari Ailus wrote: > > On Thu, Sep 12, 2024 at 03:05:10PM +0300, Laurent Pinchart wrote: > > > On Thu, Sep 12, 2024 at 01:11:13PM +0200, Tommaso Merciai wrote: > > > > On Thu, Sep 12, 2024 at 01:44:09PM +0300, Laurent Pinchart wrote: > > > > > On Tue, Sep 10, 2024 at 03:44:27PM +0200, Tommaso Merciai wrote: > > > > > > Controls can be exposed to userspace via a v4l-subdevX device, and > > > > > > userspace has to be able to subscribe to control events so that it is > > > > > > notified when the control changes value. Add missing HAS_EVENTS flag. > > > > > > > > > > How is this supposed to work, given that the driver doesn't implement > > > > > .subscribe_event() ? > > > > > > > > You are completely right, sorry. > > > > I think in both cases I'm missing: > > > > > > > > diff --git a/drivers/media/i2c/max96714.c b/drivers/media/i2c/max96714.c > > > > index 94b1bc000e48..2257b6b807ea 100644 > > > > --- a/drivers/media/i2c/max96714.c > > > > +++ b/drivers/media/i2c/max96714.c > > > > @@ -17,6 +17,7 @@ > > > > > > > > #include <media/v4l2-cci.h> > > > > #include <media/v4l2-ctrls.h> > > > > +#include <media/v4l2-event.h> > > > > #include <media/v4l2-fwnode.h> > > > > #include <media/v4l2-subdev.h> > > > > > > > > @@ -488,6 +489,8 @@ static int max96714_log_status(struct v4l2_subdev *sd) > > > > > > > > static const struct v4l2_subdev_core_ops max96714_subdev_core_ops = { > > > > .log_status = max96714_log_status, > > > > + .subscribe_event = v4l2_ctrl_subdev_subscribe_event, > > > > + .unsubscribe_event = v4l2_event_subdev_unsubscribe, > > > > }; > > > > > > > > static const struct v4l2_subdev_video_ops max96714_video_ops = { > > > > > > > > Like you suggest. Or I'm wrong? > > > > > > That looks better :-) > > > > > > Out of curiosity, what's your use case for control events ? > > > > I'm not sure if there's a use case, however control event should be > > supported when a driver supports controls. > > Not disputing that. That's why I asked "out of curiosity". No use case :'(. Sakari point me that we are missing events flag, some days ago. Thanks & Regards, Tommaso > > -- > Regards, > > Laurent Pinchart
diff --git a/drivers/media/i2c/max96714.c b/drivers/media/i2c/max96714.c index 159753b13777..94b1bc000e48 100644 --- a/drivers/media/i2c/max96714.c +++ b/drivers/media/i2c/max96714.c @@ -602,7 +602,8 @@ static int max96714_create_subdev(struct max96714_priv *priv) goto err_free_ctrl; } - priv->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_STREAMS; + priv->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | + V4L2_SUBDEV_FL_HAS_EVENTS | V4L2_SUBDEV_FL_STREAMS; priv->sd.entity.function = MEDIA_ENT_F_VID_IF_BRIDGE; priv->sd.entity.ops = &max96714_entity_ops;
Controls can be exposed to userspace via a v4l-subdevX device, and userspace has to be able to subscribe to control events so that it is notified when the control changes value. Add missing HAS_EVENTS flag. Signed-off-by: Tommaso Merciai <tomm.merciai@gmail.com> --- drivers/media/i2c/max96714.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)