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.
> > +/**
> > + * 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.
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 :)
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
