[PATCH v2] media: v4l2-subdev: Refactor events

[PATCH v2] media: v4l2-subdev: Refactor events

[Tommaso Merciai]

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.
If a control handler is set for the subdev then set the HAS_EVENTS
flag automatically into v4l2_subdev_init_finalize() and use
v4l2_ctrl_subdev_subscribe_event() and v4l2_event_subdev_unsubscribe()
as default if subdev don't have .(un)subscribe control operations.

Signed-off-by: Tommaso Merciai <tomm.merciai@gmail.com>
---
Changes since v1:
 - Aligned event subscription with unsubscription as suggested by LPinchart,
   SAilus

 drivers/media/v4l2-core/v4l2-subdev.c | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
index 3a4ba08810d2..fad8fa1f63e8 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -691,10 +691,25 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg,
 		return v4l2_event_dequeue(vfh, arg, file->f_flags & O_NONBLOCK);
 
 	case VIDIOC_SUBSCRIBE_EVENT:
-		return v4l2_subdev_call(sd, core, subscribe_event, vfh, arg);
+		if (v4l2_subdev_has_op(sd, core, subscribe_event))
+			return v4l2_subdev_call(sd, core, subscribe_event,
+						vfh, arg);
+
+		if ((sd->flags & V4L2_SUBDEV_FL_HAS_EVENTS) &&
+		    vfh->ctrl_handler)
+			return v4l2_ctrl_subdev_subscribe_event(sd, vfh, arg);
+
+		return -ENOIOCTLCMD;
 
 	case VIDIOC_UNSUBSCRIBE_EVENT:
-		return v4l2_subdev_call(sd, core, unsubscribe_event, vfh, arg);
+		if (v4l2_subdev_has_op(sd, core, unsubscribe_event))
+			return v4l2_subdev_call(sd, core, unsubscribe_event,
+						vfh, arg);
+
+		if (sd->flags & V4L2_SUBDEV_FL_HAS_EVENTS)
+			return v4l2_event_subdev_unsubscribe(sd, vfh, arg);
+
+		return -ENOIOCTLCMD;
 
 #ifdef CONFIG_VIDEO_ADV_DEBUG
 	case VIDIOC_DBG_G_REGISTER:
@@ -1641,6 +1656,9 @@ int __v4l2_subdev_init_finalize(struct v4l2_subdev *sd, const char *name,
 		}
 	}
 
+	if (sd->ctrl_handler)
+		sd->flags |= V4L2_SUBDEV_FL_HAS_EVENTS;
+
 	state = __v4l2_subdev_state_alloc(sd, name, key);
 	if (IS_ERR(state))
 		return PTR_ERR(state);
-- 
2.34.1

Re: [PATCH v2] media: v4l2-subdev: Refactor events

[Laurent Pinchart]

Hi Tommaso,

Thank you for the patch.

On Sun, Oct 20, 2024 at 06:35:32PM +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.
> If a control handler is set for the subdev then set the HAS_EVENTS
> flag automatically into v4l2_subdev_init_finalize() and use
> v4l2_ctrl_subdev_subscribe_event() and v4l2_event_subdev_unsubscribe()
> as default if subdev don't have .(un)subscribe control operations.

I would add here

This simplifies subdev drivers by avoiding the need to set the
V4L2_SUBDEV_FL_HAS_EVENTS flag and plug the event handlers, and ensures
consistency of the API exposed to userspace.

And you can also add

Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> Signed-off-by: Tommaso Merciai <tomm.merciai@gmail.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Now, can we simplify sensor drivers to drop the event handlers and the
flag ? :-)

> ---
> Changes since v1:
>  - Aligned event subscription with unsubscription as suggested by LPinchart,
>    SAilus
> 
>  drivers/media/v4l2-core/v4l2-subdev.c | 22 ++++++++++++++++++++--
>  1 file changed, 20 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index 3a4ba08810d2..fad8fa1f63e8 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -691,10 +691,25 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg,
>  		return v4l2_event_dequeue(vfh, arg, file->f_flags & O_NONBLOCK);
>  
>  	case VIDIOC_SUBSCRIBE_EVENT:
> -		return v4l2_subdev_call(sd, core, subscribe_event, vfh, arg);
> +		if (v4l2_subdev_has_op(sd, core, subscribe_event))
> +			return v4l2_subdev_call(sd, core, subscribe_event,
> +						vfh, arg);
> +
> +		if ((sd->flags & V4L2_SUBDEV_FL_HAS_EVENTS) &&
> +		    vfh->ctrl_handler)
> +			return v4l2_ctrl_subdev_subscribe_event(sd, vfh, arg);
> +
> +		return -ENOIOCTLCMD;
>  
>  	case VIDIOC_UNSUBSCRIBE_EVENT:
> -		return v4l2_subdev_call(sd, core, unsubscribe_event, vfh, arg);
> +		if (v4l2_subdev_has_op(sd, core, unsubscribe_event))
> +			return v4l2_subdev_call(sd, core, unsubscribe_event,
> +						vfh, arg);
> +
> +		if (sd->flags & V4L2_SUBDEV_FL_HAS_EVENTS)
> +			return v4l2_event_subdev_unsubscribe(sd, vfh, arg);
> +
> +		return -ENOIOCTLCMD;
>  
>  #ifdef CONFIG_VIDEO_ADV_DEBUG
>  	case VIDIOC_DBG_G_REGISTER:
> @@ -1641,6 +1656,9 @@ int __v4l2_subdev_init_finalize(struct v4l2_subdev *sd, const char *name,
>  		}
>  	}
>  
> +	if (sd->ctrl_handler)
> +		sd->flags |= V4L2_SUBDEV_FL_HAS_EVENTS;
> +
>  	state = __v4l2_subdev_state_alloc(sd, name, key);
>  	if (IS_ERR(state))
>  		return PTR_ERR(state);

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2] media: v4l2-subdev: Refactor events

[Tommaso Merciai]

Hi Laurent,
Thanks for your review.

On Sun, Oct 20, 2024 at 07:43:54PM +0300, Laurent Pinchart wrote:
> Hi Tommaso,
> 
> Thank you for the patch.
> 
> On Sun, Oct 20, 2024 at 06:35:32PM +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.
> > If a control handler is set for the subdev then set the HAS_EVENTS
> > flag automatically into v4l2_subdev_init_finalize() and use
> > v4l2_ctrl_subdev_subscribe_event() and v4l2_event_subdev_unsubscribe()
> > as default if subdev don't have .(un)subscribe control operations.
> 
> I would add here
> 
> This simplifies subdev drivers by avoiding the need to set the
> V4L2_SUBDEV_FL_HAS_EVENTS flag and plug the event handlers, and ensures
> consistency of the API exposed to userspace.
> 
> And you can also add
> 
> Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> > Signed-off-by: Tommaso Merciai <tomm.merciai@gmail.com>
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Oks, Thanks again.

> 
> Now, can we simplify sensor drivers to drop the event handlers and the
> flag ? :-)

Yep, plan is add all to support v4l2_subdev_init_finalize()
Removing:

 .subscribe_event = v4l2_ctrl_subdev_subscribe_event,
 .unsubscribe_event = v4l2_event_subdev_unsubscribe,

if are used. And ofc V4L2_SUBDEV_FL_HAS_EVENTS.

Meanwhile I think I will send v3 with your
suggestions. :)

Thanks & Regards,
Tommaso

> 
> > ---
> > Changes since v1:
> >  - Aligned event subscription with unsubscription as suggested by LPinchart,
> >    SAilus
> > 
> >  drivers/media/v4l2-core/v4l2-subdev.c | 22 ++++++++++++++++++++--
> >  1 file changed, 20 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> > index 3a4ba08810d2..fad8fa1f63e8 100644
> > --- a/drivers/media/v4l2-core/v4l2-subdev.c
> > +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> > @@ -691,10 +691,25 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg,
> >  		return v4l2_event_dequeue(vfh, arg, file->f_flags & O_NONBLOCK);
> >  
> >  	case VIDIOC_SUBSCRIBE_EVENT:
> > -		return v4l2_subdev_call(sd, core, subscribe_event, vfh, arg);
> > +		if (v4l2_subdev_has_op(sd, core, subscribe_event))
> > +			return v4l2_subdev_call(sd, core, subscribe_event,
> > +						vfh, arg);
> > +
> > +		if ((sd->flags & V4L2_SUBDEV_FL_HAS_EVENTS) &&
> > +		    vfh->ctrl_handler)
> > +			return v4l2_ctrl_subdev_subscribe_event(sd, vfh, arg);
> > +
> > +		return -ENOIOCTLCMD;
> >  
> >  	case VIDIOC_UNSUBSCRIBE_EVENT:
> > -		return v4l2_subdev_call(sd, core, unsubscribe_event, vfh, arg);
> > +		if (v4l2_subdev_has_op(sd, core, unsubscribe_event))
> > +			return v4l2_subdev_call(sd, core, unsubscribe_event,
> > +						vfh, arg);
> > +
> > +		if (sd->flags & V4L2_SUBDEV_FL_HAS_EVENTS)
> > +			return v4l2_event_subdev_unsubscribe(sd, vfh, arg);
> > +
> > +		return -ENOIOCTLCMD;
> >  
> >  #ifdef CONFIG_VIDEO_ADV_DEBUG
> >  	case VIDIOC_DBG_G_REGISTER:
> > @@ -1641,6 +1656,9 @@ int __v4l2_subdev_init_finalize(struct v4l2_subdev *sd, const char *name,
> >  		}
> >  	}
> >  
> > +	if (sd->ctrl_handler)
> > +		sd->flags |= V4L2_SUBDEV_FL_HAS_EVENTS;
> > +
> >  	state = __v4l2_subdev_state_alloc(sd, name, key);
> >  	if (IS_ERR(state))
> >  		return PTR_ERR(state);
> 
> -- 
> Regards,
> 
> Laurent Pinchart

Re: [PATCH v2] media: v4l2-subdev: Refactor events

[Laurent Pinchart]

On Mon, Oct 21, 2024 at 08:35:53AM +0200, Tommaso Merciai wrote:
> Hi Laurent,
> Thanks for your review.
> 
> On Sun, Oct 20, 2024 at 07:43:54PM +0300, Laurent Pinchart wrote:
> > Hi Tommaso,
> > 
> > Thank you for the patch.
> > 
> > On Sun, Oct 20, 2024 at 06:35:32PM +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.
> > > If a control handler is set for the subdev then set the HAS_EVENTS
> > > flag automatically into v4l2_subdev_init_finalize() and use
> > > v4l2_ctrl_subdev_subscribe_event() and v4l2_event_subdev_unsubscribe()
> > > as default if subdev don't have .(un)subscribe control operations.
> > 
> > I would add here
> > 
> > This simplifies subdev drivers by avoiding the need to set the
> > V4L2_SUBDEV_FL_HAS_EVENTS flag and plug the event handlers, and ensures
> > consistency of the API exposed to userspace.
> > 
> > And you can also add
> > 
> > Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > 
> > > Signed-off-by: Tommaso Merciai <tomm.merciai@gmail.com>
> > 
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> Oks, Thanks again.
> 
> > Now, can we simplify sensor drivers to drop the event handlers and the
> > flag ? :-)
> 
> Yep, plan is add all to support v4l2_subdev_init_finalize()
> Removing:
> 
>  .subscribe_event = v4l2_ctrl_subdev_subscribe_event,
>  .unsubscribe_event = v4l2_event_subdev_unsubscribe,
> 
> if are used. And ofc V4L2_SUBDEV_FL_HAS_EVENTS.

What I meant is looking at the I2C sensor drivers that currently

- call v4l2_subdev_init_finalize()
- set V4L2_SUBDEV_FL_HAS_EVENTS
- set the .subscribe_event() and .unsubscribe_event() handlers

and dropping the flag and handlers from them. Is that what you plan to
work on ?

> Meanwhile I think I will send v3 with your
> suggestions. :)
> 
> > > ---
> > > Changes since v1:
> > >  - Aligned event subscription with unsubscription as suggested by LPinchart,
> > >    SAilus
> > > 
> > >  drivers/media/v4l2-core/v4l2-subdev.c | 22 ++++++++++++++++++++--
> > >  1 file changed, 20 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> > > index 3a4ba08810d2..fad8fa1f63e8 100644
> > > --- a/drivers/media/v4l2-core/v4l2-subdev.c
> > > +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> > > @@ -691,10 +691,25 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg,
> > >  		return v4l2_event_dequeue(vfh, arg, file->f_flags & O_NONBLOCK);
> > >  
> > >  	case VIDIOC_SUBSCRIBE_EVENT:
> > > -		return v4l2_subdev_call(sd, core, subscribe_event, vfh, arg);
> > > +		if (v4l2_subdev_has_op(sd, core, subscribe_event))
> > > +			return v4l2_subdev_call(sd, core, subscribe_event,
> > > +						vfh, arg);
> > > +
> > > +		if ((sd->flags & V4L2_SUBDEV_FL_HAS_EVENTS) &&
> > > +		    vfh->ctrl_handler)
> > > +			return v4l2_ctrl_subdev_subscribe_event(sd, vfh, arg);
> > > +
> > > +		return -ENOIOCTLCMD;
> > >  
> > >  	case VIDIOC_UNSUBSCRIBE_EVENT:
> > > -		return v4l2_subdev_call(sd, core, unsubscribe_event, vfh, arg);
> > > +		if (v4l2_subdev_has_op(sd, core, unsubscribe_event))
> > > +			return v4l2_subdev_call(sd, core, unsubscribe_event,
> > > +						vfh, arg);
> > > +
> > > +		if (sd->flags & V4L2_SUBDEV_FL_HAS_EVENTS)
> > > +			return v4l2_event_subdev_unsubscribe(sd, vfh, arg);
> > > +
> > > +		return -ENOIOCTLCMD;
> > >  
> > >  #ifdef CONFIG_VIDEO_ADV_DEBUG
> > >  	case VIDIOC_DBG_G_REGISTER:
> > > @@ -1641,6 +1656,9 @@ int __v4l2_subdev_init_finalize(struct v4l2_subdev *sd, const char *name,
> > >  		}
> > >  	}
> > >  
> > > +	if (sd->ctrl_handler)
> > > +		sd->flags |= V4L2_SUBDEV_FL_HAS_EVENTS;
> > > +
> > >  	state = __v4l2_subdev_state_alloc(sd, name, key);
> > >  	if (IS_ERR(state))
> > >  		return PTR_ERR(state);

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2] media: v4l2-subdev: Refactor events

[Tommaso Merciai]

On Mon, Oct 21, 2024 at 10:30:34AM +0300, Laurent Pinchart wrote:
> On Mon, Oct 21, 2024 at 08:35:53AM +0200, Tommaso Merciai wrote:
> > Hi Laurent,
> > Thanks for your review.
> > 
> > On Sun, Oct 20, 2024 at 07:43:54PM +0300, Laurent Pinchart wrote:
> > > Hi Tommaso,
> > > 
> > > Thank you for the patch.
> > > 
> > > On Sun, Oct 20, 2024 at 06:35:32PM +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.
> > > > If a control handler is set for the subdev then set the HAS_EVENTS
> > > > flag automatically into v4l2_subdev_init_finalize() and use
> > > > v4l2_ctrl_subdev_subscribe_event() and v4l2_event_subdev_unsubscribe()
> > > > as default if subdev don't have .(un)subscribe control operations.
> > > 
> > > I would add here
> > > 
> > > This simplifies subdev drivers by avoiding the need to set the
> > > V4L2_SUBDEV_FL_HAS_EVENTS flag and plug the event handlers, and ensures
> > > consistency of the API exposed to userspace.
> > > 
> > > And you can also add
> > > 
> > > Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > 
> > > > Signed-off-by: Tommaso Merciai <tomm.merciai@gmail.com>
> > > 
> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > 
> > Oks, Thanks again.
> > 
> > > Now, can we simplify sensor drivers to drop the event handlers and the
> > > flag ? :-)
> > 
> > Yep, plan is add all to support v4l2_subdev_init_finalize()
> > Removing:
> > 
> >  .subscribe_event = v4l2_ctrl_subdev_subscribe_event,
> >  .unsubscribe_event = v4l2_event_subdev_unsubscribe,
> > 
> > if are used. And ofc V4L2_SUBDEV_FL_HAS_EVENTS.
> 
> What I meant is looking at the I2C sensor drivers that currently
> 
> - call v4l2_subdev_init_finalize()
> - set V4L2_SUBDEV_FL_HAS_EVENTS
> - set the .subscribe_event() and .unsubscribe_event() handlers
> 
> and dropping the flag and handlers from them. Is that what you plan to
> work on ?

Yep, I will take a look on that. :)

Thanks & Regards,
Tommaso

> 
> > Meanwhile I think I will send v3 with your
> > suggestions. :)
> > 
> > > > ---
> > > > Changes since v1:
> > > >  - Aligned event subscription with unsubscription as suggested by LPinchart,
> > > >    SAilus
> > > > 
> > > >  drivers/media/v4l2-core/v4l2-subdev.c | 22 ++++++++++++++++++++--
> > > >  1 file changed, 20 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> > > > index 3a4ba08810d2..fad8fa1f63e8 100644
> > > > --- a/drivers/media/v4l2-core/v4l2-subdev.c
> > > > +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> > > > @@ -691,10 +691,25 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg,
> > > >  		return v4l2_event_dequeue(vfh, arg, file->f_flags & O_NONBLOCK);
> > > >  
> > > >  	case VIDIOC_SUBSCRIBE_EVENT:
> > > > -		return v4l2_subdev_call(sd, core, subscribe_event, vfh, arg);
> > > > +		if (v4l2_subdev_has_op(sd, core, subscribe_event))
> > > > +			return v4l2_subdev_call(sd, core, subscribe_event,
> > > > +						vfh, arg);
> > > > +
> > > > +		if ((sd->flags & V4L2_SUBDEV_FL_HAS_EVENTS) &&
> > > > +		    vfh->ctrl_handler)
> > > > +			return v4l2_ctrl_subdev_subscribe_event(sd, vfh, arg);
> > > > +
> > > > +		return -ENOIOCTLCMD;
> > > >  
> > > >  	case VIDIOC_UNSUBSCRIBE_EVENT:
> > > > -		return v4l2_subdev_call(sd, core, unsubscribe_event, vfh, arg);
> > > > +		if (v4l2_subdev_has_op(sd, core, unsubscribe_event))
> > > > +			return v4l2_subdev_call(sd, core, unsubscribe_event,
> > > > +						vfh, arg);
> > > > +
> > > > +		if (sd->flags & V4L2_SUBDEV_FL_HAS_EVENTS)
> > > > +			return v4l2_event_subdev_unsubscribe(sd, vfh, arg);
> > > > +
> > > > +		return -ENOIOCTLCMD;
> > > >  
> > > >  #ifdef CONFIG_VIDEO_ADV_DEBUG
> > > >  	case VIDIOC_DBG_G_REGISTER:
> > > > @@ -1641,6 +1656,9 @@ int __v4l2_subdev_init_finalize(struct v4l2_subdev *sd, const char *name,
> > > >  		}
> > > >  	}
> > > >  
> > > > +	if (sd->ctrl_handler)
> > > > +		sd->flags |= V4L2_SUBDEV_FL_HAS_EVENTS;
> > > > +
> > > >  	state = __v4l2_subdev_state_alloc(sd, name, key);
> > > >  	if (IS_ERR(state))
> > > >  		return PTR_ERR(state);
> 
> -- 
> Regards,
> 
> Laurent Pinchart

Re: [PATCH v2] media: v4l2-subdev: Refactor events

[Tommaso Merciai]

Hi Laurent, Sakari,

Sorry for the delay.
Back on this topic.

On Mon, Oct 21, 2024 at 10:30:34AM +0300, Laurent Pinchart wrote:
> On Mon, Oct 21, 2024 at 08:35:53AM +0200, Tommaso Merciai wrote:
> > Hi Laurent,
> > Thanks for your review.
> > 
> > On Sun, Oct 20, 2024 at 07:43:54PM +0300, Laurent Pinchart wrote:
> > > Hi Tommaso,
> > > 
> > > Thank you for the patch.
> > > 
> > > On Sun, Oct 20, 2024 at 06:35:32PM +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.
> > > > If a control handler is set for the subdev then set the HAS_EVENTS
> > > > flag automatically into v4l2_subdev_init_finalize() and use
> > > > v4l2_ctrl_subdev_subscribe_event() and v4l2_event_subdev_unsubscribe()
> > > > as default if subdev don't have .(un)subscribe control operations.
> > > 
> > > I would add here
> > > 
> > > This simplifies subdev drivers by avoiding the need to set the
> > > V4L2_SUBDEV_FL_HAS_EVENTS flag and plug the event handlers, and ensures
> > > consistency of the API exposed to userspace.
> > > 
> > > And you can also add
> > > 
> > > Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > 
> > > > Signed-off-by: Tommaso Merciai <tomm.merciai@gmail.com>
> > > 
> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > 
> > Oks, Thanks again.
> > 
> > > Now, can we simplify sensor drivers to drop the event handlers and the
> > > flag ? :-)
> > 
> > Yep, plan is add all to support v4l2_subdev_init_finalize()
> > Removing:
> > 
> >  .subscribe_event = v4l2_ctrl_subdev_subscribe_event,
> >  .unsubscribe_event = v4l2_event_subdev_unsubscribe,
> > 
> > if are used. And ofc V4L2_SUBDEV_FL_HAS_EVENTS.
> 
> What I meant is looking at the I2C sensor drivers that currently
> 
> - call v4l2_subdev_init_finalize()
> - set V4L2_SUBDEV_FL_HAS_EVENTS
> - set the .subscribe_event() and .unsubscribe_event() handlers
> 
> and dropping the flag and handlers from them. Is that what you plan to
> work on ?

It's ok for you per/driver patch or you prefer a big single patch?

Meanwhile I've prepared something here:

https://gitlab.freedesktop.org/linux-media/users/tmerciai/-/compare/next...v6.12.0-rc1-nxp?from_project_id=22111

Let me know if you prefer (un)squashed version.
Thanks in advance. :)

Thanks & Regards,
Tommaso

> 
> > Meanwhile I think I will send v3 with your
> > suggestions. :)
> > 
> > > > ---
> > > > Changes since v1:
> > > >  - Aligned event subscription with unsubscription as suggested by LPinchart,
> > > >    SAilus
> > > > 
> > > >  drivers/media/v4l2-core/v4l2-subdev.c | 22 ++++++++++++++++++++--
> > > >  1 file changed, 20 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> > > > index 3a4ba08810d2..fad8fa1f63e8 100644
> > > > --- a/drivers/media/v4l2-core/v4l2-subdev.c
> > > > +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> > > > @@ -691,10 +691,25 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg,
> > > >  		return v4l2_event_dequeue(vfh, arg, file->f_flags & O_NONBLOCK);
> > > >  
> > > >  	case VIDIOC_SUBSCRIBE_EVENT:
> > > > -		return v4l2_subdev_call(sd, core, subscribe_event, vfh, arg);
> > > > +		if (v4l2_subdev_has_op(sd, core, subscribe_event))
> > > > +			return v4l2_subdev_call(sd, core, subscribe_event,
> > > > +						vfh, arg);
> > > > +
> > > > +		if ((sd->flags & V4L2_SUBDEV_FL_HAS_EVENTS) &&
> > > > +		    vfh->ctrl_handler)
> > > > +			return v4l2_ctrl_subdev_subscribe_event(sd, vfh, arg);
> > > > +
> > > > +		return -ENOIOCTLCMD;
> > > >  
> > > >  	case VIDIOC_UNSUBSCRIBE_EVENT:
> > > > -		return v4l2_subdev_call(sd, core, unsubscribe_event, vfh, arg);
> > > > +		if (v4l2_subdev_has_op(sd, core, unsubscribe_event))
> > > > +			return v4l2_subdev_call(sd, core, unsubscribe_event,
> > > > +						vfh, arg);
> > > > +
> > > > +		if (sd->flags & V4L2_SUBDEV_FL_HAS_EVENTS)
> > > > +			return v4l2_event_subdev_unsubscribe(sd, vfh, arg);
> > > > +
> > > > +		return -ENOIOCTLCMD;
> > > >  
> > > >  #ifdef CONFIG_VIDEO_ADV_DEBUG
> > > >  	case VIDIOC_DBG_G_REGISTER:
> > > > @@ -1641,6 +1656,9 @@ int __v4l2_subdev_init_finalize(struct v4l2_subdev *sd, const char *name,
> > > >  		}
> > > >  	}
> > > >  
> > > > +	if (sd->ctrl_handler)
> > > > +		sd->flags |= V4L2_SUBDEV_FL_HAS_EVENTS;
> > > > +
> > > >  	state = __v4l2_subdev_state_alloc(sd, name, key);
> > > >  	if (IS_ERR(state))
> > > >  		return PTR_ERR(state);
> 
> -- 
> Regards,
> 
> Laurent Pinchart

Re: [PATCH v2] media: v4l2-subdev: Refactor events

[Laurent Pinchart]

Hi Tommaso,

On Mon, Oct 28, 2024 at 06:32:32PM +0100, Tommaso Merciai wrote:
> On Mon, Oct 21, 2024 at 10:30:34AM +0300, Laurent Pinchart wrote:
> > On Mon, Oct 21, 2024 at 08:35:53AM +0200, Tommaso Merciai wrote:
> > > On Sun, Oct 20, 2024 at 07:43:54PM +0300, Laurent Pinchart wrote:
> > > > On Sun, Oct 20, 2024 at 06:35:32PM +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.
> > > > > If a control handler is set for the subdev then set the HAS_EVENTS
> > > > > flag automatically into v4l2_subdev_init_finalize() and use
> > > > > v4l2_ctrl_subdev_subscribe_event() and v4l2_event_subdev_unsubscribe()
> > > > > as default if subdev don't have .(un)subscribe control operations.
> > > > 
> > > > I would add here
> > > > 
> > > > This simplifies subdev drivers by avoiding the need to set the
> > > > V4L2_SUBDEV_FL_HAS_EVENTS flag and plug the event handlers, and ensures
> > > > consistency of the API exposed to userspace.
> > > > 
> > > > And you can also add
> > > > 
> > > > Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > 
> > > > > Signed-off-by: Tommaso Merciai <tomm.merciai@gmail.com>
> > > > 
> > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > 
> > > Oks, Thanks again.
> > > 
> > > > Now, can we simplify sensor drivers to drop the event handlers and the
> > > > flag ? :-)
> > > 
> > > Yep, plan is add all to support v4l2_subdev_init_finalize()
> > > Removing:
> > > 
> > >  .subscribe_event = v4l2_ctrl_subdev_subscribe_event,
> > >  .unsubscribe_event = v4l2_event_subdev_unsubscribe,
> > > 
> > > if are used. And ofc V4L2_SUBDEV_FL_HAS_EVENTS.
> > 
> > What I meant is looking at the I2C sensor drivers that currently
> > 
> > - call v4l2_subdev_init_finalize()
> > - set V4L2_SUBDEV_FL_HAS_EVENTS
> > - set the .subscribe_event() and .unsubscribe_event() handlers
> > 
> > and dropping the flag and handlers from them. Is that what you plan to
> > work on ?
> 
> It's ok for you per/driver patch or you prefer a big single patch?

I'm fine either way. Maybe one large patch to address all the drivers
where the flag and handlers are simply dropped, and then one patch per
driver where changes are larger (such as adding calls to
v4l2_subdev_init_finalize()) ?

> Meanwhile I've prepared something here:
> 
> https://gitlab.freedesktop.org/linux-media/users/tmerciai/-/compare/next...v6.12.0-rc1-nxp?from_project_id=22111
> 
> Let me know if you prefer (un)squashed version.
> Thanks in advance. :)
> 
> Thanks & Regards,
> Tommaso
> 
> > 
> > > Meanwhile I think I will send v3 with your
> > > suggestions. :)
> > > 
> > > > > ---
> > > > > Changes since v1:
> > > > >  - Aligned event subscription with unsubscription as suggested by LPinchart,
> > > > >    SAilus
> > > > > 
> > > > >  drivers/media/v4l2-core/v4l2-subdev.c | 22 ++++++++++++++++++++--
> > > > >  1 file changed, 20 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> > > > > index 3a4ba08810d2..fad8fa1f63e8 100644
> > > > > --- a/drivers/media/v4l2-core/v4l2-subdev.c
> > > > > +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> > > > > @@ -691,10 +691,25 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg,
> > > > >  		return v4l2_event_dequeue(vfh, arg, file->f_flags & O_NONBLOCK);
> > > > >  
> > > > >  	case VIDIOC_SUBSCRIBE_EVENT:
> > > > > -		return v4l2_subdev_call(sd, core, subscribe_event, vfh, arg);
> > > > > +		if (v4l2_subdev_has_op(sd, core, subscribe_event))
> > > > > +			return v4l2_subdev_call(sd, core, subscribe_event,
> > > > > +						vfh, arg);
> > > > > +
> > > > > +		if ((sd->flags & V4L2_SUBDEV_FL_HAS_EVENTS) &&
> > > > > +		    vfh->ctrl_handler)
> > > > > +			return v4l2_ctrl_subdev_subscribe_event(sd, vfh, arg);
> > > > > +
> > > > > +		return -ENOIOCTLCMD;
> > > > >  
> > > > >  	case VIDIOC_UNSUBSCRIBE_EVENT:
> > > > > -		return v4l2_subdev_call(sd, core, unsubscribe_event, vfh, arg);
> > > > > +		if (v4l2_subdev_has_op(sd, core, unsubscribe_event))
> > > > > +			return v4l2_subdev_call(sd, core, unsubscribe_event,
> > > > > +						vfh, arg);
> > > > > +
> > > > > +		if (sd->flags & V4L2_SUBDEV_FL_HAS_EVENTS)
> > > > > +			return v4l2_event_subdev_unsubscribe(sd, vfh, arg);
> > > > > +
> > > > > +		return -ENOIOCTLCMD;
> > > > >  
> > > > >  #ifdef CONFIG_VIDEO_ADV_DEBUG
> > > > >  	case VIDIOC_DBG_G_REGISTER:
> > > > > @@ -1641,6 +1656,9 @@ int __v4l2_subdev_init_finalize(struct v4l2_subdev *sd, const char *name,
> > > > >  		}
> > > > >  	}
> > > > >  
> > > > > +	if (sd->ctrl_handler)
> > > > > +		sd->flags |= V4L2_SUBDEV_FL_HAS_EVENTS;
> > > > > +
> > > > >  	state = __v4l2_subdev_state_alloc(sd, name, key);
> > > > >  	if (IS_ERR(state))
> > > > >  		return PTR_ERR(state);

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2] media: v4l2-subdev: Refactor events

[Sakari Ailus]

On Mon, Oct 28, 2024 at 08:36:57PM +0200, Laurent Pinchart wrote:
> > > What I meant is looking at the I2C sensor drivers that currently
> > > 
> > > - call v4l2_subdev_init_finalize()
> > > - set V4L2_SUBDEV_FL_HAS_EVENTS
> > > - set the .subscribe_event() and .unsubscribe_event() handlers
> > > 
> > > and dropping the flag and handlers from them. Is that what you plan to
> > > work on ?
> > 
> > It's ok for you per/driver patch or you prefer a big single patch?
> 
> I'm fine either way. Maybe one large patch to address all the drivers
> where the flag and handlers are simply dropped, and then one patch per
> driver where changes are larger (such as adding calls to
> v4l2_subdev_init_finalize()) ?

Sounds good to me.

-- 
Sakari Ailus

Re: [PATCH v2] media: v4l2-subdev: Refactor events

[Sakari Ailus]

Hi Laurent, Tommaso,

On Sun, Oct 20, 2024 at 07:43:54PM +0300, Laurent Pinchart wrote:
> Hi Tommaso,
> 
> Thank you for the patch.
> 
> On Sun, Oct 20, 2024 at 06:35:32PM +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.
> > If a control handler is set for the subdev then set the HAS_EVENTS
> > flag automatically into v4l2_subdev_init_finalize() and use
> > v4l2_ctrl_subdev_subscribe_event() and v4l2_event_subdev_unsubscribe()
> > as default if subdev don't have .(un)subscribe control operations.
> 
> I would add here
> 
> This simplifies subdev drivers by avoiding the need to set the
> V4L2_SUBDEV_FL_HAS_EVENTS flag and plug the event handlers, and ensures
> consistency of the API exposed to userspace.
> 
> And you can also add
> 
> Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> > Signed-off-by: Tommaso Merciai <tomm.merciai@gmail.com>
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Thanks!

I've picked this to my tree.

Please try to properly wrap the commit message the next time, most editors
can do that automatically.

-- 
Sakari Ailus

Re: [PATCH v2] media: v4l2-subdev: Refactor events

[Tommaso Merciai]

Hi Sakari,

On Mon, Oct 21, 2024 at 07:26:41AM +0000, Sakari Ailus wrote:
> Hi Laurent, Tommaso,
> 
> On Sun, Oct 20, 2024 at 07:43:54PM +0300, Laurent Pinchart wrote:
> > Hi Tommaso,
> > 
> > Thank you for the patch.
> > 
> > On Sun, Oct 20, 2024 at 06:35:32PM +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.
> > > If a control handler is set for the subdev then set the HAS_EVENTS
> > > flag automatically into v4l2_subdev_init_finalize() and use
> > > v4l2_ctrl_subdev_subscribe_event() and v4l2_event_subdev_unsubscribe()
> > > as default if subdev don't have .(un)subscribe control operations.
> > 
> > I would add here
> > 
> > This simplifies subdev drivers by avoiding the need to set the
> > V4L2_SUBDEV_FL_HAS_EVENTS flag and plug the event handlers, and ensures
> > consistency of the API exposed to userspace.
> > 
> > And you can also add
> > 
> > Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > 
> > > Signed-off-by: Tommaso Merciai <tomm.merciai@gmail.com>
> > 
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> Thanks!
> 
> I've picked this to my tree.
> 
> Please try to properly wrap the commit message the next time, most editors
> can do that automatically.

Mmm..
In theory I have this cfg for my vim:

filetype plugin indent on
syntax on
set title
set tabstop=8
set softtabstop=8
set shiftwidth=8
set noexpandtab
set number
set hidden

Thanks & Regards,
Tommaso

> 
> -- 
> Sakari Ailus