On Mon October 1 2012 22:01:38 Mauro Carvalho Chehab wrote:
> Em Thu, 27 Sep 2012 08:44:25 +0200
> Hans Verkuil <hverkuil@xxxxxxxxx> 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.
Well, v4l2-controls.txt says:
"There can be only one notify function per control handler. Any attempt
to set another notify function will cause a WARN_ON."
And it is documented as well in the header, so what more do you want?
> > > > + 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.
No, it's not. Multiple controls share the same handler, so this would only work
for the first control and block any attempts to set the notifier for other
controls of the same handler.
The point is that in today's implementation the notifier is stored in the handler
because that was the easiest implementation and because it is all we need today.
But in the future we may have to support different notifiers and also more than
one notifier per control, and in that case the notifier would move to the control
data structure.
I want to be able to make such a change without having to change all drivers that
use today's implementation.
The API for setting a notifier should act on a control, not on a control handler.
The 'hack' above is just a check that ensures that you don't violate the constraints
of the current implementation. If anyone hits that warning and contacts the ml, then
I can see if there are good enough reasons to go the extra mile and remove this
constraint.
So I really don't want to change this patch.
Regards,
Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
