Hi Hans
On Fri, 14 Jun 2013, Hans Verkuil wrote:
> On Fri 14 June 2013 09:14:48 Guennadi Liakhovetski wrote:
> > Hi Sylwester
> >
> > On Thu, 13 Jun 2013, Sylwester Nawrocki wrote:
> >
> > > Hi Guennadi,
> > >
> > > Overall it looks quite neat at this v10. :)
> >
> > Thanks :)
> >
> > > On 06/11/2013 10:23 AM, Guennadi Liakhovetski wrote:
> > > > Currently bridge device drivers register devices for all subdevices
> > > > synchronously, tupically, during their probing. E.g. if an I2C CMOS sensor
> > >
> > > s/tupically/typically
> > >
> > > > is attached to a video bridge device, the bridge driver will create an I2C
> > >
> > > > +/**
> > > > + * v4l2_async_subdev_list - provided by subdevices
> > > > + * @list: links struct v4l2_async_subdev_list objects to a global list
> > > > + * before probing, and onto notifier->done after probing
> > > > + * @asd: pointer to respective struct v4l2_async_subdev
> > > > + * @notifier: pointer to managing notifier
> > > > + */
> > > > +struct v4l2_async_subdev_list {
> > > > + struct list_head list;
> > > > + struct v4l2_async_subdev *asd;
> > > > + struct v4l2_async_notifier *notifier;
> > > > +};
> > >
> > > I have a patch for this patch, which embeds members of this struct directly
> > > into struct v4l2_subdev. My felling is that the code is simpler and easier
> > > to follow this way, I might be missing some important details though.
> >
> > Thanks, saw it. In principle I have nothing against it. I think, it's just
> > principle approach to this work, which seems to differ slightly from how
> > others see it. I tried to as little intrusive as possible, touching
> > current APIs only if absolutely necessary, keeping the async stuff largely
> > separated from the rest. If however the common feeling is, that we should
> > inject it directly in V4L2 core, I have nothing against it either. Still,
> > I would prefer to keep the .c and .h files separate for now at least to
> > reduce merge conflicts etc.
>
> I think it makes sense to move this to the core, but keep the .c and .h files
> separate.
Ok, we can apply Sylwester's patch on top of my series, sure.
> A general note: my experience is that if being being intrusive to existing
> APIs/data structures will simplify your code, then that's probable a good idea.
> Core APIs and data structures are not 'holy' and it is quite OK to change them.
> They will need careful code review, but other than that it is perfectly fine.
>
> >
> > > > +/**
> > > > + * v4l2_async_notifier - v4l2_device notifier data
> > > > + * @subdev_num: number of subdevices
> > > > + * @subdev: array of pointers to subdevices
> > >
> > > How about changing this to:
> > >
> > > @subdevs: array of pointers to the subdevice descriptors
> >
> > I'm sure every single line of comments and code in these (and all other)
> > patches can be improved :)
> >
> > > I think it would be more immediately clear this is the actual subdevs array
> > > pointer, and perhaps we could have subdev_num renamed to num_subdevs ?
> >
> > Sure, why not :)
> >
> > > > + * @v4l2_dev: pointer to struct v4l2_device
> > > > + * @waiting: list of struct v4l2_async_subdev, waiting for their drivers
> > > > + * @done: list of struct v4l2_async_subdev_list, already probed
> > > > + * @list: member in a global list of notifiers
> > > > + * @bound: a subdevice driver has successfully probed one of subdevices
> > > > + * @complete: all subdevices have been probed successfully
> > > > + * @unbind: a subdevice is leaving
> > > > + */
> > > > +struct v4l2_async_notifier {
> > > > + unsigned int subdev_num;
> > > > + struct v4l2_async_subdev **subdev;
> > > > + struct v4l2_device *v4l2_dev;
> > > > + struct list_head waiting;
> > > > + struct list_head done;
> > > > + struct list_head list;
> > > > + int (*bound)(struct v4l2_async_notifier *notifier,
> > > > + struct v4l2_subdev *subdev,
> > > > + struct v4l2_async_subdev *asd);
> > > > + int (*complete)(struct v4l2_async_notifier *notifier);
> > > > + void (*unbind)(struct v4l2_async_notifier *notifier,
> > > > + struct v4l2_subdev *subdev,
> > > > + struct v4l2_async_subdev *asd);
> > > > +};
> > > > +
> > > > +int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev,
> > > > + struct v4l2_async_notifier *notifier);
> > > > +void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier);
> > > > +int v4l2_async_register_subdev(struct v4l2_subdev *sd);
> > > > +void v4l2_async_unregister_subdev(struct v4l2_subdev *sd);
> > >
> > > I still think "async_" in this public API is unnecessary, since we register/
> > > unregister a subdev with the core and notifiers are intrinsically
> > > asynchronous.
> > > But your preference seems be otherwise, what could I do... :) At most it just
> > > means one less happy user of this interface.
>
> I think v4l2_register_subdev looks awfully similar to v4l2_device_register_subdev.
> It becomes very confusing naming it like that. I prefer v4l2_async where 'async'
> refers to the v4l2-async module.
And v4l2(_async)_notifier_(un)register()?
> > See above :) And another point - this is your opinion, which I certainly
> > respect and take into account. I think, Laurent somehow softly inclined in
> > the same direction. But I didn't hear any other opinions, so, in the end I
> > have to make a decision - is everyone more likely to like what has been
> > proposed by this specific reviewer, or would everyone object :) There are
> > obvious things - fixes etc., and there are less obvious ones - naming,
> > formulating and such. So, here again - I just would prefer all methods,
> > comprising this API to share the same namespace. To make it clearer, that
> > if you register subdevices asynchronously, you also need notifiers on the
> > host and the other way round. To make it easier to authors to match these
> > methods. Just my 2p :)
> >
> > > So except this bikeshedding I don't really have other comments, I'm going to
> > > test this series with the s3c-camif/ov9650 drivers and will report back soon.
> > >
> > > It would have been a shame to not have this series in 3.11. I guess three
> > > kernel cycles, since the initial implementation, time frame is sufficient
> > > for having finally working camera devices on a device tree enabled system
> > > in mainline.
> >
> > Great! So, let's get 1 or 2 opinions more, then I can make a v11 with just
> > a couple of cosmetic changes and kindly ask Mauro to pull this in :)
>
> There is one last thing that also needs to be done: document this API in
> Documentation/video4linux/v4l2-framework.txt.
Right, can do that too.
Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
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
