Message ID | 1370939028-8352-17-git-send-email-g.liakhovetski@gmx.de (mailing list archive) |
---|---|
State | Superseded |
Headers | show |
Hi Guennadi, Thanks for the patch. Hopefully we get this in for 3.11 On Tue, Jun 11, 2013 at 1:53 PM, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote: > Currently bridge device drivers register devices for all subdevices > synchronously, tupically, during their probing. E.g. if an I2C CMOS sensor > is attached to a video bridge device, the bridge driver will create an I2C > device and wait for the respective I2C driver to probe. This makes linking > of devices straight forward, but this approach cannot be used with > intrinsically asynchronous and unordered device registration systems like > the Flattened Device Tree. To support such systems this patch adds an > asynchronous subdevice registration framework to V4L2. To use it respective > (e.g. I2C) subdevice drivers must register themselves with the framework. > A bridge driver on the other hand must register notification callbacks, > that will be called upon various related events. > > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de> Acked-by: Lad, Prabhakar <prabhakar.csengg@gmail.com> Tested-by: Lad, Prabhakar <prabhakar.csengg@gmail.com> Regards, --Prabhakar Lad -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Guennadi, Overall it looks quite neat at this v10. :) 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. > +/** > + * 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 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 ? > + * @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. 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. Thanks for the idea and patience during reviews! :) Regards, Sylwester -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. 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. > > 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. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On 06/14/2013 11:14 AM, Guennadi Liakhovetski wrote: > On Fri, 14 Jun 2013, Hans Verkuil wrote: >> On Fri 14 June 2013 09:14:48 Guennadi Liakhovetski wrote: >>> On Thu, 13 Jun 2013, Sylwester Nawrocki wrote: >>>> On 06/11/2013 10:23 AM, Guennadi Liakhovetski wrote: [...] >>>>> + * @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. Ok, let's leave v4l2_async then. > And v4l2(_async)_notifier_(un)register()? I guess it would be better to have all or none of the functions with that prefix. So either: v4l2_async_notifier_register v4l2_async_notifier_unregister v4l2_async_register_subdev v4l2_async_unregister_subdev or v4l2_subdev_notifier_register v4l2_subdev_notifier_unregister v4l2_subdev_register v4l2_subdev_unregister Thanks, Sylwester -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 14 Jun 2013, Sylwester Nawrocki wrote: > Hi, > > On 06/14/2013 11:14 AM, Guennadi Liakhovetski wrote: > > On Fri, 14 Jun 2013, Hans Verkuil wrote: > >> On Fri 14 June 2013 09:14:48 Guennadi Liakhovetski wrote: > >>> On Thu, 13 Jun 2013, Sylwester Nawrocki wrote: > >>>> On 06/11/2013 10:23 AM, Guennadi Liakhovetski wrote: > [...] > >>>>> + * @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. > > Ok, let's leave v4l2_async then. > > > And v4l2(_async)_notifier_(un)register()? > > I guess it would be better to have all or none of the functions > with that prefix. So either: Exactly. Thanks Guennadi > v4l2_async_notifier_register > v4l2_async_notifier_unregister > v4l2_async_register_subdev > v4l2_async_unregister_subdev > > or > > v4l2_subdev_notifier_register > v4l2_subdev_notifier_unregister > v4l2_subdev_register > v4l2_subdev_unregister > > Thanks, > Sylwester > --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Sylwester, On Friday 14 June 2013 11:40:12 Sylwester Nawrocki wrote: > On 06/14/2013 11:14 AM, Guennadi Liakhovetski wrote: > > On Fri, 14 Jun 2013, Hans Verkuil wrote: > >> On Fri 14 June 2013 09:14:48 Guennadi Liakhovetski wrote: > >>> On Thu, 13 Jun 2013, Sylwester Nawrocki wrote: > >>>> On 06/11/2013 10:23 AM, Guennadi Liakhovetski wrote: > [...] > > >>>>> + * @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. > > Ok, let's leave v4l2_async then. > > > And v4l2(_async)_notifier_(un)register()? > > I guess it would be better to have all or none of the functions > with that prefix. So either: > > v4l2_async_notifier_register > v4l2_async_notifier_unregister > v4l2_async_register_subdev > v4l2_async_unregister_subdev > > or > > v4l2_subdev_notifier_register > v4l2_subdev_notifier_unregister > v4l2_subdev_register > v4l2_subdev_unregister I prefer the section option, but I'm fine with the first one for now. As Hans pointed out, it would be pretty easy to confuse v4l2_subdev_register with v4l2_device_register_subdev. In the long term we want to move all drivers to asynchronous registration. When that happens it won't be difficult to revisit the API to remove unused functions and rename v4l2_aysnc_* to just v4l2_*.
Hi Guennadi, On Friday 14 June 2013 09:14:48 Guennadi Liakhovetski wrote: > 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 :) I have no further comment than those already posted by Hans and Sylwester on v10. I'll ack v11 (possibly with comments on the documentation though ;-)).
diff --git a/drivers/media/v4l2-core/Makefile b/drivers/media/v4l2-core/Makefile index 628c630..4c33b8d6 100644 --- a/drivers/media/v4l2-core/Makefile +++ b/drivers/media/v4l2-core/Makefile @@ -5,7 +5,8 @@ tuner-objs := tuner-core.o videodev-objs := v4l2-dev.o v4l2-ioctl.o v4l2-device.o v4l2-fh.o \ - v4l2-event.o v4l2-ctrls.o v4l2-subdev.o v4l2-clk.o + v4l2-event.o v4l2-ctrls.o v4l2-subdev.o v4l2-clk.o \ + v4l2-async.o ifeq ($(CONFIG_COMPAT),y) videodev-objs += v4l2-compat-ioctl32.o endif diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c new file mode 100644 index 0000000..3590ccc --- /dev/null +++ b/drivers/media/v4l2-core/v4l2-async.c @@ -0,0 +1,280 @@ +/* + * V4L2 asynchronous subdevice registration API + * + * Copyright (C) 2012-2013, Guennadi Liakhovetski <g.liakhovetski@gmx.de> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + */ + +#include <linux/device.h> +#include <linux/err.h> +#include <linux/i2c.h> +#include <linux/list.h> +#include <linux/module.h> +#include <linux/mutex.h> +#include <linux/platform_device.h> +#include <linux/slab.h> +#include <linux/types.h> + +#include <media/v4l2-async.h> +#include <media/v4l2-device.h> +#include <media/v4l2-subdev.h> + +static bool match_i2c(struct device *dev, struct v4l2_async_subdev *asd) +{ + struct i2c_client *client = i2c_verify_client(dev); + return client && + asd->bus_type == V4L2_ASYNC_BUS_I2C && + asd->match.i2c.adapter_id == client->adapter->nr && + asd->match.i2c.address == client->addr; +} + +static bool match_platform(struct device *dev, struct v4l2_async_subdev *asd) +{ + return asd->bus_type == V4L2_ASYNC_BUS_PLATFORM && + !strcmp(asd->match.platform.name, dev_name(dev)); +} + +static LIST_HEAD(subdev_list); +static LIST_HEAD(notifier_list); +static DEFINE_MUTEX(list_lock); + +static struct v4l2_async_subdev *v4l2_async_belongs(struct v4l2_async_notifier *notifier, + struct v4l2_async_subdev_list *asdl) +{ + struct v4l2_subdev *sd = v4l2_async_to_subdev(asdl); + struct v4l2_async_subdev *asd; + bool (*match)(struct device *, + struct v4l2_async_subdev *); + + list_for_each_entry(asd, ¬ifier->waiting, list) { + /* bus_type has been verified valid before */ + switch (asd->bus_type) { + case V4L2_ASYNC_BUS_CUSTOM: + match = asd->match.custom.match; + if (!match) + /* Match always */ + return asd; + break; + case V4L2_ASYNC_BUS_PLATFORM: + match = match_platform; + break; + case V4L2_ASYNC_BUS_I2C: + match = match_i2c; + break; + default: + /* Cannot happen, unless someone breaks us */ + WARN_ON(true); + return NULL; + } + + /* match cannot be NULL here */ + if (match(sd->dev, asd)) + return asd; + } + + return NULL; +} + +static int v4l2_async_test_notify(struct v4l2_async_notifier *notifier, + struct v4l2_async_subdev_list *asdl, + struct v4l2_async_subdev *asd) +{ + struct v4l2_subdev *sd = v4l2_async_to_subdev(asdl); + int ret; + + /* Remove from the waiting list */ + list_del(&asd->list); + asdl->asd = asd; + asdl->notifier = notifier; + + if (notifier->bound) { + ret = notifier->bound(notifier, sd, asd); + if (ret < 0) + return ret; + } + /* Move from the global subdevice list to notifier's done */ + list_move(&asdl->list, ¬ifier->done); + + ret = v4l2_device_register_subdev(notifier->v4l2_dev, sd); + if (ret < 0) { + if (notifier->unbind) + notifier->unbind(notifier, sd, asd); + return ret; + } + + if (list_empty(¬ifier->waiting) && notifier->complete) + return notifier->complete(notifier); + + return 0; +} + +static void v4l2_async_cleanup(struct v4l2_async_subdev_list *asdl) +{ + struct v4l2_subdev *sd = v4l2_async_to_subdev(asdl); + + v4l2_device_unregister_subdev(sd); + /* Subdevice driver will reprobe and put asdl back onto the list */ + list_del_init(&asdl->list); + asdl->asd = NULL; + sd->dev = NULL; +} + +int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev, + struct v4l2_async_notifier *notifier) +{ + struct v4l2_async_subdev_list *asdl, *tmp; + struct v4l2_async_subdev *asd; + int i; + + if (!notifier->subdev_num || notifier->subdev_num > V4L2_MAX_SUBDEVS) + return -EINVAL; + + notifier->v4l2_dev = v4l2_dev; + INIT_LIST_HEAD(¬ifier->waiting); + INIT_LIST_HEAD(¬ifier->done); + + for (i = 0; i < notifier->subdev_num; i++) { + asd = notifier->subdev[i]; + + switch (asd->bus_type) { + case V4L2_ASYNC_BUS_CUSTOM: + case V4L2_ASYNC_BUS_PLATFORM: + case V4L2_ASYNC_BUS_I2C: + break; + default: + dev_err(notifier->v4l2_dev ? notifier->v4l2_dev->dev : NULL, + "Invalid bus-type %u on %p\n", + asd->bus_type, asd); + return -EINVAL; + } + list_add_tail(&asd->list, ¬ifier->waiting); + } + + mutex_lock(&list_lock); + + /* Keep also completed notifiers on the list */ + list_add(¬ifier->list, ¬ifier_list); + + list_for_each_entry_safe(asdl, tmp, &subdev_list, list) { + int ret; + + asd = v4l2_async_belongs(notifier, asdl); + if (!asd) + continue; + + ret = v4l2_async_test_notify(notifier, asdl, asd); + if (ret < 0) { + mutex_unlock(&list_lock); + return ret; + } + } + + mutex_unlock(&list_lock); + + return 0; +} +EXPORT_SYMBOL(v4l2_async_notifier_register); + +void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier) +{ + struct v4l2_async_subdev_list *asdl, *tmp; + unsigned int notif_n_subdev = notifier->subdev_num; + unsigned int n_subdev = min(notif_n_subdev, V4L2_MAX_SUBDEVS); + struct device *dev[n_subdev]; + int i = 0; + + mutex_lock(&list_lock); + + list_del(¬ifier->list); + + list_for_each_entry_safe(asdl, tmp, ¬ifier->done, list) { + struct v4l2_subdev *sd = v4l2_async_to_subdev(asdl); + + dev[i] = get_device(sd->dev); + + v4l2_async_cleanup(asdl); + + /* If we handled USB devices, we'd have to lock the parent too */ + device_release_driver(dev[i++]); + + if (notifier->unbind) + notifier->unbind(notifier, sd, sd->asdl.asd); + } + + mutex_unlock(&list_lock); + + while (i--) { + struct device *d = dev[i]; + + if (d && device_attach(d) < 0) { + const char *name = "(none)"; + int lock = device_trylock(d); + + if (lock && d->driver) + name = d->driver->name; + dev_err(d, "Failed to re-probe to %s\n", name); + if (lock) + device_unlock(d); + } + put_device(d); + } + /* + * Don't care about the waiting list, it is initialised and populated + * upon notifier registration. + */ +} +EXPORT_SYMBOL(v4l2_async_notifier_unregister); + +int v4l2_async_register_subdev(struct v4l2_subdev *sd) +{ + struct v4l2_async_subdev_list *asdl = &sd->asdl; + struct v4l2_async_notifier *notifier; + + mutex_lock(&list_lock); + + INIT_LIST_HEAD(&asdl->list); + + list_for_each_entry(notifier, ¬ifier_list, list) { + struct v4l2_async_subdev *asd = v4l2_async_belongs(notifier, asdl); + if (asd) { + int ret = v4l2_async_test_notify(notifier, asdl, asd); + mutex_unlock(&list_lock); + return ret; + } + } + + /* None matched, wait for hot-plugging */ + list_add(&asdl->list, &subdev_list); + + mutex_unlock(&list_lock); + + return 0; +} +EXPORT_SYMBOL(v4l2_async_register_subdev); + +void v4l2_async_unregister_subdev(struct v4l2_subdev *sd) +{ + struct v4l2_async_subdev_list *asdl = &sd->asdl; + struct v4l2_async_notifier *notifier = asdl->notifier; + + if (!asdl->asd) { + if (!list_empty(&asdl->list)) + v4l2_async_cleanup(asdl); + return; + } + + mutex_lock(&list_lock); + + list_add(&asdl->asd->list, ¬ifier->waiting); + + v4l2_async_cleanup(asdl); + + if (notifier->unbind) + notifier->unbind(notifier, sd, sd->asdl.asd); + + mutex_unlock(&list_lock); +} +EXPORT_SYMBOL(v4l2_async_unregister_subdev); diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h new file mode 100644 index 0000000..12bd39c --- /dev/null +++ b/include/media/v4l2-async.h @@ -0,0 +1,105 @@ +/* + * V4L2 asynchronous subdevice registration API + * + * Copyright (C) 2012-2013, Guennadi Liakhovetski <g.liakhovetski@gmx.de> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + */ + +#ifndef V4L2_ASYNC_H +#define V4L2_ASYNC_H + +#include <linux/list.h> +#include <linux/mutex.h> + +struct device; +struct v4l2_device; +struct v4l2_subdev; +struct v4l2_async_notifier; + +/* A random max subdevice number, used to allocate an array on stack */ +#define V4L2_MAX_SUBDEVS 128U + +enum v4l2_async_bus_type { + V4L2_ASYNC_BUS_CUSTOM, + V4L2_ASYNC_BUS_PLATFORM, + V4L2_ASYNC_BUS_I2C, +}; + +/** + * struct v4l2_async_subdev - sub-device descriptor, as known to a bridge + * @bus_type: subdevice bus type to select the appropriate matching method + * @match: union of per-bus type matching data sets + * @list: used to link struct v4l2_async_subdev objects, waiting to be + * probed, to a notifier->waiting list + */ +struct v4l2_async_subdev { + enum v4l2_async_bus_type bus_type; + union { + struct { + const char *name; + } platform; + struct { + int adapter_id; + unsigned short address; + } i2c; + struct { + bool (*match)(struct device *, + struct v4l2_async_subdev *); + void *priv; + } custom; + } match; + + /* v4l2-async core private: not to be used by drivers */ + struct list_head list; +}; + +/** + * 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; +}; + +/** + * v4l2_async_notifier - v4l2_device notifier data + * @subdev_num: number of subdevices + * @subdev: array of pointers to subdevices + * @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); +#endif diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h index d8756fa..21174af 100644 --- a/include/media/v4l2-subdev.h +++ b/include/media/v4l2-subdev.h @@ -24,6 +24,7 @@ #include <linux/types.h> #include <linux/v4l2-subdev.h> #include <media/media-entity.h> +#include <media/v4l2-async.h> #include <media/v4l2-common.h> #include <media/v4l2-dev.h> #include <media/v4l2-fh.h> @@ -587,8 +588,15 @@ struct v4l2_subdev { struct video_device *devnode; /* pointer to the physical device */ struct device *dev; + struct v4l2_async_subdev_list asdl; }; +static inline struct v4l2_subdev *v4l2_async_to_subdev( + struct v4l2_async_subdev_list *asdl) +{ + return container_of(asdl, struct v4l2_subdev, asdl); +} + #define media_entity_to_v4l2_subdev(ent) \ container_of(ent, struct v4l2_subdev, entity) #define vdev_to_v4l2_subdev(vdev) \
Currently bridge device drivers register devices for all subdevices synchronously, tupically, during their probing. E.g. if an I2C CMOS sensor is attached to a video bridge device, the bridge driver will create an I2C device and wait for the respective I2C driver to probe. This makes linking of devices straight forward, but this approach cannot be used with intrinsically asynchronous and unordered device registration systems like the Flattened Device Tree. To support such systems this patch adds an asynchronous subdevice registration framework to V4L2. To use it respective (e.g. I2C) subdevice drivers must register themselves with the framework. A bridge driver on the other hand must register notification callbacks, that will be called upon various related events. Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de> --- v10: (a) remove dynamic dev[] allocation (b) merge struct v4l2_async_hw_info into struct v4l2_async_subdev (c) change .bound(), .unbind() prototypes - thanks to Laurent for a patch (d) improve struct comment (e) remove an unused variable in v4l2_async_unregister_subdev() (f) lock the device, when dereferencing dev->driver (g) fix a false success in v4l2_async_belongs() drivers/media/v4l2-core/Makefile | 3 +- drivers/media/v4l2-core/v4l2-async.c | 280 ++++++++++++++++++++++++++++++++++ include/media/v4l2-async.h | 105 +++++++++++++ include/media/v4l2-subdev.h | 8 + 4 files changed, 395 insertions(+), 1 deletions(-) create mode 100644 drivers/media/v4l2-core/v4l2-async.c create mode 100644 include/media/v4l2-async.h