Re: [RFCv1 API PATCH 2/4] v4l2-ctrls: add a notify callback.

[Mauro Carvalho Chehab <mchehab@redhat.com>]

Em Thu, 27 Sep 2012 08:44:25 +0200
Hans Verkuil <hverkuil@xs4all.nl> escreveu:

> On Wed September 26 2012 12:50:11 Laurent Pinchart wrote:

> > > +	if (notify == NULL) {
> > > +		ctrl->call_notify = 0;
> > > +		return;
> > > +	}
> > > +	/* Only one notifier is allowed. Should we ever need to support
> > > +	   multiple notifiers, then some sort of linked list of notifiers
> > > +	   should be implemented. But I don't see any real reason to implement
> > > +	   that now. If you think you need multiple notifiers, then contact
> > > +	   the linux-media mailinglist. */

If only one notifier is allowed, then you should clearly state that at the
API documentation.

> > > +	if (WARN_ON(ctrl->handler->notify &&
> > > +			(ctrl->handler->notify != notify ||
> > > +			 ctrl->handler->notify_priv != priv)))
> > > +		return;
> > 
> > I'm not sure whether I like that. It feels a bit hackish. Wouldn't it be 
> > better to register the notifier with the handler explictly just once and then 
> > enable/disable notifications on a per-control basis ?
> 
> I thought about that, but I prefer this method because it allows me to switch
> to per-control notifiers in the future. In addition, different controls can have
> different handlers. If you have to set the notifier for handlers, then the
> driver needs to figure out which handlers are involved for the controls it wants
> to be notified on. It's much easier to do it like this.

That also sounded hackish on my eyes. If just one notifier is allowed, the
function should simply refuse any other call to it, as any other call to it
is a driver's bug. So:

	if (WARN_ON(ctrl->handler->notify))
		return;

seems to be enough.

Regards,
Mauro