* [PATCH 0/2] media: i2c: max96714/max96717 @ 2024-09-10 13:44 Tommaso Merciai 2024-09-10 13:44 ` [PATCH 1/2] media: i2c: max96714: add HAS_EVENTS subdev flag Tommaso Merciai 2024-09-10 13:44 ` [PATCH 2/2] media: i2c: max96717: " Tommaso Merciai 0 siblings, 2 replies; 9+ messages in thread From: Tommaso Merciai @ 2024-09-10 13:44 UTC (permalink / raw) Cc: linuxfancy, sakari.ailus, laurent.pinchart, tomm.merciai, julien.massot, Mauro Carvalho Chehab, linux-media, linux-kernel Hi All, This patchset add missing HAS_EVENTS flags to max96714/max96717 drivers. "If HAS_DEVNODE is set, and there are controls, then HAS_EVENTS must be set and you have to be able to (un)subscribe to events. Because the 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." References: - https://www.spinics.net/lists/linux-media/msg220721.html Regards, Tommaso Tommaso Merciai (2): media: i2c: max96714: add HAS_EVENTS subdev flag media: i2c: max96717: add HAS_EVENTS subdev flag drivers/media/i2c/max96714.c | 3 ++- drivers/media/i2c/max96717.c | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) -- 2.34.1 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 1/2] media: i2c: max96714: add HAS_EVENTS subdev flag 2024-09-10 13:44 [PATCH 0/2] media: i2c: max96714/max96717 Tommaso Merciai @ 2024-09-10 13:44 ` Tommaso Merciai 2024-09-12 10:44 ` Laurent Pinchart 2024-09-10 13:44 ` [PATCH 2/2] media: i2c: max96717: " Tommaso Merciai 1 sibling, 1 reply; 9+ messages in thread From: Tommaso Merciai @ 2024-09-10 13:44 UTC (permalink / raw) Cc: linuxfancy, sakari.ailus, laurent.pinchart, tomm.merciai, julien.massot, Mauro Carvalho Chehab, linux-media, linux-kernel 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(-) 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; -- 2.34.1 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] media: i2c: max96714: add HAS_EVENTS subdev flag 2024-09-10 13:44 ` [PATCH 1/2] media: i2c: max96714: add HAS_EVENTS subdev flag Tommaso Merciai @ 2024-09-12 10:44 ` Laurent Pinchart 2024-09-12 11:11 ` Tommaso Merciai 0 siblings, 1 reply; 9+ messages in thread From: Laurent Pinchart @ 2024-09-12 10:44 UTC (permalink / raw) To: Tommaso Merciai Cc: linuxfancy, sakari.ailus, julien.massot, Mauro Carvalho Chehab, linux-media, linux-kernel 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; > -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] media: i2c: max96714: add HAS_EVENTS subdev flag 2024-09-12 10:44 ` Laurent Pinchart @ 2024-09-12 11:11 ` Tommaso Merciai 2024-09-12 12:05 ` Laurent Pinchart 0 siblings, 1 reply; 9+ messages in thread From: Tommaso Merciai @ 2024-09-12 11:11 UTC (permalink / raw) To: Laurent Pinchart Cc: linuxfancy, sakari.ailus, julien.massot, Mauro Carvalho Chehab, linux-media, linux-kernel 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] media: i2c: max96714: add HAS_EVENTS subdev flag 2024-09-12 11:11 ` Tommaso Merciai @ 2024-09-12 12:05 ` Laurent Pinchart 2024-09-12 12:14 ` Sakari Ailus 0 siblings, 1 reply; 9+ messages in thread From: Laurent Pinchart @ 2024-09-12 12:05 UTC (permalink / raw) To: Tommaso Merciai Cc: linuxfancy, sakari.ailus, julien.massot, Mauro Carvalho Chehab, linux-media, linux-kernel 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; > > > -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] media: i2c: max96714: add HAS_EVENTS subdev flag 2024-09-12 12:05 ` Laurent Pinchart @ 2024-09-12 12:14 ` Sakari Ailus 2024-09-12 12:18 ` Laurent Pinchart 0 siblings, 1 reply; 9+ messages in thread From: Sakari Ailus @ 2024-09-12 12:14 UTC (permalink / raw) To: Laurent Pinchart Cc: Tommaso Merciai, linuxfancy, julien.massot, Mauro Carvalho Chehab, linux-media, linux-kernel 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. -- Regards, Sakari Ailus ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] media: i2c: max96714: add HAS_EVENTS subdev flag 2024-09-12 12:14 ` Sakari Ailus @ 2024-09-12 12:18 ` Laurent Pinchart 2024-09-12 12:52 ` Tommaso Merciai 0 siblings, 1 reply; 9+ messages in thread From: Laurent Pinchart @ 2024-09-12 12:18 UTC (permalink / raw) To: Sakari Ailus Cc: Tommaso Merciai, linuxfancy, julien.massot, Mauro Carvalho Chehab, linux-media, linux-kernel 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". -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] media: i2c: max96714: add HAS_EVENTS subdev flag 2024-09-12 12:18 ` Laurent Pinchart @ 2024-09-12 12:52 ` Tommaso Merciai 0 siblings, 0 replies; 9+ messages in thread From: Tommaso Merciai @ 2024-09-12 12:52 UTC (permalink / raw) To: Laurent Pinchart Cc: Sakari Ailus, linuxfancy, julien.massot, Mauro Carvalho Chehab, linux-media, linux-kernel 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 2/2] media: i2c: max96717: add HAS_EVENTS subdev flag 2024-09-10 13:44 [PATCH 0/2] media: i2c: max96714/max96717 Tommaso Merciai 2024-09-10 13:44 ` [PATCH 1/2] media: i2c: max96714: add HAS_EVENTS subdev flag Tommaso Merciai @ 2024-09-10 13:44 ` Tommaso Merciai 1 sibling, 0 replies; 9+ messages in thread From: Tommaso Merciai @ 2024-09-10 13:44 UTC (permalink / raw) Cc: linuxfancy, sakari.ailus, laurent.pinchart, tomm.merciai, julien.massot, Mauro Carvalho Chehab, linux-media, linux-kernel 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/max96717.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/media/i2c/max96717.c b/drivers/media/i2c/max96717.c index 4e85b8eb1e77..da534999e1ff 100644 --- a/drivers/media/i2c/max96717.c +++ b/drivers/media/i2c/max96717.c @@ -689,7 +689,8 @@ static int max96717_subdev_init(struct max96717_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 = &max96717_entity_ops; -- 2.34.1 ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2024-09-12 12:52 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2024-09-10 13:44 [PATCH 0/2] media: i2c: max96714/max96717 Tommaso Merciai 2024-09-10 13:44 ` [PATCH 1/2] media: i2c: max96714: add HAS_EVENTS subdev flag Tommaso Merciai 2024-09-12 10:44 ` Laurent Pinchart 2024-09-12 11:11 ` Tommaso Merciai 2024-09-12 12:05 ` Laurent Pinchart 2024-09-12 12:14 ` Sakari Ailus 2024-09-12 12:18 ` Laurent Pinchart 2024-09-12 12:52 ` Tommaso Merciai 2024-09-10 13:44 ` [PATCH 2/2] media: i2c: max96717: " Tommaso Merciai
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®