Re: [PATCH 5/6] v4l2-event: Add v4l2_subscribed_event_ops

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 10/27/2011 02:30 PM, Laurent Pinchart wrote:
> Hi Hans,
>
> On Thursday 27 October 2011 13:18:02 Hans de Goede wrote:
>> Just like with ctrl events, drivers may want to get called back on
>> listener add / remove for other event types too. Rather then special
>> casing all of this in subscribe / unsubscribe event it is better to
>> use ops for this.
>>
>> Signed-off-by: Hans de Goede<hdegoede@redhat.com>
>> ---
>>   drivers/media/video/ivtv/ivtv-ioctl.c  |    2 +-
>>   drivers/media/video/omap3isp/ispccdc.c |    2 +-
>>   drivers/media/video/omap3isp/ispstat.c |    2 +-
>>   drivers/media/video/pwc/pwc-v4l.c      |    2 +-
>>   drivers/media/video/v4l2-event.c       |   42 ++++++++++++++++++++++-------
>>   drivers/media/video/vivi.c             |    2 +-
>>   include/media/v4l2-event.h             |   24 +++++++++++++-----
>
> Haven't you forgotten to update Documentation/video4linux/v4l2-framework.txt ?
>

I think I have, I'll go and do another revision fixing this.

>>   7 files changed, 54 insertions(+), 22 deletions(-)
>
> [snip]
>
>> diff --git a/drivers/media/video/v4l2-event.c
>> b/drivers/media/video/v4l2-event.c index 3d27300..2dd9252 100644
>> --- a/drivers/media/video/v4l2-event.c
>> +++ b/drivers/media/video/v4l2-event.c
>> @@ -131,14 +131,14 @@ static void __v4l2_event_queue_fh(struct v4l2_fh *fh,
>> const struct v4l2_event *e sev->first = sev_pos(sev, 1);
>>   		fh->navailable--;
>>   		if (sev->elems == 1) {
>> -			if (sev->replace) {
>> -				sev->replace(&kev->event, ev);
>> +			if (sev->ops&&  sev->ops->replace) {
>> +				sev->ops->replace(&kev->event, ev);
>>   				copy_payload = false;
>>   			}
>> -		} else if (sev->merge) {
>> +		} else if (sev->ops&&  sev->ops->merge) {
>>   			struct v4l2_kevent *second_oldest =
>>   				sev->events + sev_pos(sev, 0);
>> -			sev->merge(&kev->event,&second_oldest->event);
>> +			sev->ops->merge(&kev->event,&second_oldest->event);
>>   		}
>>   	}
>>
>> @@ -207,8 +207,14 @@ static void ctrls_merge(const struct v4l2_event *old,
>> struct v4l2_event *new) new->u.ctrl.changes |= old->u.ctrl.changes;
>>   }
>>
>> +const struct v4l2_subscribed_event_ops ctrl_ops = {
>
> Shouldn't this be static const ?
>

You're right I will fix this in the next iteration.

>> +	.replace = ctrls_replace,
>> +	.merge = ctrls_merge,
>> +};
>> +
>>   int v4l2_event_subscribe(struct v4l2_fh *fh,
>> -			 struct v4l2_event_subscription *sub, unsigned elems)
>> +			 struct v4l2_event_subscription *sub, unsigned elems,
>> +			 const struct v4l2_subscribed_event_ops *ops)
>>   {
>>   	struct v4l2_subscribed_event *sev, *found_ev;
>>   	struct v4l2_ctrl *ctrl = NULL;
>> @@ -236,9 +242,9 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
>>   	sev->flags = sub->flags;
>>   	sev->fh = fh;
>>   	sev->elems = elems;
>> +	sev->ops = ops;
>>   	if (ctrl) {
>> -		sev->replace = ctrls_replace;
>> -		sev->merge = ctrls_merge;
>> +		sev->ops =&ctrl_ops;
>>   	}
>
> You can remove the brackets here.
>

I was aiming for minimal diff size here, since this bit will go away in the
next patch in the series anyways.

>>
>>   	spin_lock_irqsave(&fh->vdev->fh_lock, flags);
>> @@ -247,10 +253,22 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
>>   		list_add(&sev->list,&fh->subscribed);
>>   	spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
>>
>> -	/* v4l2_ctrl_add_event uses a mutex, so do this outside the spin lock */
>> -	if (found_ev)
>> +	if (found_ev) {
>>   		kfree(sev);
>> -	else if (ctrl)
>> +		return 0; /* Already listening */
>> +	}
>> +
>> +	if (sev->ops&&  sev->ops->add) {
>> +		int ret = sev->ops->add(sev);
>> +		if (ret) {
>> +			sev->ops = NULL;
>> +			v4l2_event_unsubscribe(fh, sub);
>> +			return ret;
>> +		}
>> +	}
>> +
>> +	/* v4l2_ctrl_add_event uses a mutex, so do this outside the spin lock */
>> +	if (ctrl)
>>   		v4l2_ctrl_add_event(ctrl, sev);
>>
>>   	return 0;
>> @@ -307,6 +325,10 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
>>   	}
>>
>>   	spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
>> +
>> +	if (sev&&  sev->ops&&  sev->ops->del)
>> +		sev->ops->del(sev);
>> +
>>   	if (sev&&  sev->type == V4L2_EVENT_CTRL) {
>>   		struct v4l2_ctrl *ctrl = v4l2_ctrl_find(fh->ctrl_handler, sev->id);
>>
>

Regards,

Hans