| Message ID | 1365781240-16149-3-git-send-email-g.liakhovetski@gmx.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
Hi Guennadi, On 04/12/2013 05:40 PM, Guennadi Liakhovetski 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> > --- > > v9: addressed Laurent's comments (thanks) > 1. moved valid hw->bus_type check > 2. made v4l2_async_unregister() void > 3. renamed struct v4l2_async_hw_device to struct v4l2_async_hw_info > 4. merged struct v4l2_async_subdev_list into struct v4l2_subdev > 5. fixed a typo > 6. made subdev_num unsigned > > drivers/media/v4l2-core/Makefile | 3 +- > drivers/media/v4l2-core/v4l2-async.c | 284 ++++++++++++++++++++++++++++++++++ > include/media/v4l2-async.h | 99 ++++++++++++ > include/media/v4l2-subdev.h | 10 ++ > 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 > > 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..98db2e0 > --- /dev/null > +++ b/drivers/media/v4l2-core/v4l2-async.c > @@ -0,0 +1,284 @@ > +/* > + * 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. > + */ > + [...] > +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; > +} > + > +static void v4l2_async_unregister(struct v4l2_async_subdev_list *asdl) > +{ > + struct v4l2_subdev *sd = v4l2_async_to_subdev(asdl); > + > + v4l2_async_cleanup(asdl); > + > + /* If we handled USB devices, we'd have to lock the parent too */ > + device_release_driver(sd->dev); > +} > + > +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; > + > + 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->hw.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->hw.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; > + int i = 0; > + struct device **dev = kcalloc(notifier->subdev_num, > + sizeof(*dev), GFP_KERNEL); > + if (!dev) > + dev_err(notifier->v4l2_dev->dev, > + "Failed to allocate device cache!\n"); > + > + mutex_lock(&list_lock); > + > + list_del(¬ifier->list); > + > + list_for_each_entry_safe(asdl, tmp, ¬ifier->done, list) { > + if (dev) { > + struct v4l2_subdev *sd = v4l2_async_to_subdev(asdl); > + dev[i++] = get_device(sd->dev); > + } > + v4l2_async_unregister(asdl); Hmm, couldn't we do without the **dev array ? Now when struct v42_subdev has struct device * embedded in it ? And if we can't get hold off struct device object is it safe to call v4l2_async_unregister(), which references it ? Why is get_device() optional ? Some comment might be useful here. > + > + if (notifier->unbind) > + notifier->unbind(notifier, asdl); > + } > + > + mutex_unlock(&list_lock); > + > + if (dev) { > + while (i--) { > + if (dev[i] && device_attach(dev[i]) < 0) > + dev_err(dev[i], "Failed to re-probe to %s\n", > + dev[i]->driver ? dev[i]->driver->name : "(none)"); Is it safe to reference dev->driver without holding struct device::mutex ? > + put_device(dev[i]); > + } > + kfree(dev); > + } > + /* > + * 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; > + struct device *dev; This variable appears unused, except a single assignment below. > + if (!asdl->asd) { > + if (!list_empty(&asdl->list)) > + v4l2_async_cleanup(asdl); > + return; > + } > + > + mutex_lock(&list_lock); > + > + dev = sd->dev; > + list_add(&asdl->asd->list, ¬ifier->waiting); > + > + v4l2_async_cleanup(asdl); > + > + if (notifier->unbind) > + notifier->unbind(notifier, asdl); > + > + 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..c638f5c > --- /dev/null > +++ b/include/media/v4l2-async.h > @@ -0,0 +1,99 @@ > +/* > + * 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; > + > +enum v4l2_async_bus_type { > + V4L2_ASYNC_BUS_CUSTOM, > + V4L2_ASYNC_BUS_PLATFORM, > + V4L2_ASYNC_BUS_I2C, > +}; > + > +struct v4l2_async_hw_info { > + 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_hw_info *); > + void *priv; > + } custom; > + } match; > +}; > + > +/** > + * struct v4l2_async_subdev - sub-device descriptor, as known to a bridge > + * @hw: this device descriptor > + * @list: member in a list of subdevices > + */ > +struct v4l2_async_subdev { > + struct v4l2_async_hw_info hw; > + struct list_head list; > +}; > + > +/** > + * v4l2_async_subdev_list - provided by subdevices > + * @list: member in a list of subdevices > + * @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 - provided by bridges It probably makes sense to just say e.g. v4l2_async_notifier - v4l2_device notifier data structure I mean at least "bridge" to me doesn't sound generic enough. > + * @subdev_num: number of subdevices > + * @subdev: array of pointers to subdevices > + * @v4l2_dev: pointer to struct v4l2_device > + * @waiting: list of subdevices, waiting for their drivers > + * @done: list of subdevices, 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_async_subdev_list *asdl); > + int (*complete)(struct v4l2_async_notifier *notifier); > + void (*unbind)(struct v4l2_async_notifier *notifier, > + struct v4l2_async_subdev_list *asdl); I would preffer to simply pass struct v4l2_subdev * to bound/unbind. Since it is about just one subdevice's status change, why do we need struct v4l2_async_subdev_list ? > +}; > + > +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); How about naming it v4l2_device_notifier_(un)register() ? > +int v4l2_async_register_subdev(struct v4l2_subdev *sd); > +void v4l2_async_unregister_subdev(struct v4l2_subdev *sd); Hopefully, one day it just becomes v4l2_(un)register_subdev() :-) > +#endif > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h > index 5298d67..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> > @@ -585,8 +586,17 @@ struct v4l2_subdev { > void *host_priv; > /* subdev device node */ > struct video_device *devnode; > + /* pointer to the physical device */ > + struct device *dev; > + struct v4l2_async_subdev_list asdl; Why not embed respective fields directly in struct v4l2_subdev, rather than adding this new data structure ? I find this all code pretty much convoluted, probably one of the reason is that there are multiple list_head objects at various levels. > }; > > +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) \ > 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
Hi Guennadi, Thanks for the patch! On Fri, Apr 12, 2013 at 9:10 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> with this https://patchwork.linuxtv.org/patch/18096/ patch applied, yo can add my Acked-by: Lad, Prabhakar <prabhakar.csengg@gmail.com> Tested-by: Lad, Prabhakar <prabhakar.csengg@gmail.com> Regards, --Prabhakar -- 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 and Sylwester, On Monday 15 April 2013 13:57:17 Sylwester Nawrocki wrote: > On 04/12/2013 05:40 PM, Guennadi Liakhovetski 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> > > --- > > > > v9: addressed Laurent's comments (thanks) > > 1. moved valid hw->bus_type check > > 2. made v4l2_async_unregister() void > > 3. renamed struct v4l2_async_hw_device to struct v4l2_async_hw_info > > 4. merged struct v4l2_async_subdev_list into struct v4l2_subdev > > 5. fixed a typo > > 6. made subdev_num unsigned Remembering the media controller days, I know how it feels to reach v9. Please keep on with the good work, we're getting there :-) > > drivers/media/v4l2-core/Makefile | 3 +- > > drivers/media/v4l2-core/v4l2-async.c | 284 +++++++++++++++++++++++++++++ > > include/media/v4l2-async.h | 99 ++++++++++++ > > include/media/v4l2-subdev.h | 10 ++ > > 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 [snip] > > diff --git a/drivers/media/v4l2-core/v4l2-async.c > > b/drivers/media/v4l2-core/v4l2-async.c new file mode 100644 > > index 0000000..98db2e0 > > --- /dev/null > > +++ b/drivers/media/v4l2-core/v4l2-async.c > > @@ -0,0 +1,284 @@ > > +/* > > + * 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. > > + */ > > + > > [...] > > > +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; > > +} > > + > > +static void v4l2_async_unregister(struct v4l2_async_subdev_list *asdl) > > +{ > > + struct v4l2_subdev *sd = v4l2_async_to_subdev(asdl); > > + > > + v4l2_async_cleanup(asdl); > > + > > + /* If we handled USB devices, we'd have to lock the parent too */ > > + device_release_driver(sd->dev); > > +} > > + > > +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; > > + > > + 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->hw.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->hw.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; > > + int i = 0; > > + struct device **dev = kcalloc(notifier->subdev_num, > > + sizeof(*dev), GFP_KERNEL); > > + if (!dev) > > + dev_err(notifier->v4l2_dev->dev, > > + "Failed to allocate device cache!\n"); > > + > > + mutex_lock(&list_lock); > > + > > + list_del(¬ifier->list); > > + > > + list_for_each_entry_safe(asdl, tmp, ¬ifier->done, list) { > > + if (dev) { > > + struct v4l2_subdev *sd = v4l2_async_to_subdev(asdl); > > + dev[i++] = get_device(sd->dev); > > + } > > + v4l2_async_unregister(asdl); > > Hmm, couldn't we do without the **dev array ? Now when struct v42_subdev has > struct device * embedded in it ? > > And if we can't get hold off struct device object is it safe to call > v4l2_async_unregister(), which references it ? > > Why is get_device() optional ? Some comment might be useful here. > > > + > > + if (notifier->unbind) > > + notifier->unbind(notifier, asdl); > > + } > > + > > + mutex_unlock(&list_lock); > > + > > + if (dev) { > > + while (i--) { > > + if (dev[i] && device_attach(dev[i]) < 0) This is my last major pain point. To avoid race conditions we need circular references (see http://www.mail-archive.com/linux-media@vger.kernel.org/msg61092.html). We will thus need a way to break the circle by explictly requesting the subdev to release its resources. I'm afraid I have no well-designed solution for that at the moment. > > + dev_err(dev[i], "Failed to re-probe to %s\n", > > + dev[i]->driver ? dev[i]->driver->name : "(none)"); > > Is it safe to reference dev->driver without holding struct device::mutex ? > > > + put_device(dev[i]); > > + } > > + kfree(dev); > > + } > > + /* > > + * 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; > > + struct device *dev; > > This variable appears unused, except a single assignment below. > > > + if (!asdl->asd) { > > + if (!list_empty(&asdl->list)) > > + v4l2_async_cleanup(asdl); > > + return; > > + } > > + > > + mutex_lock(&list_lock); > > + > > + dev = sd->dev; > > > > + list_add(&asdl->asd->list, ¬ifier->waiting); > > + > > + v4l2_async_cleanup(asdl); > > + > > + if (notifier->unbind) > > + notifier->unbind(notifier, asdl); > > + > > + 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..c638f5c > > --- /dev/null > > +++ b/include/media/v4l2-async.h > > @@ -0,0 +1,99 @@ > > +/* > > + * 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; > > + > > +enum v4l2_async_bus_type { > > + V4L2_ASYNC_BUS_CUSTOM, > > + V4L2_ASYNC_BUS_PLATFORM, > > + V4L2_ASYNC_BUS_I2C, > > +}; > > + > > +struct v4l2_async_hw_info { I think I'd go for dev_info instead of hw_info, as the structure doesn't contain much hardware information. > > + 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_hw_info *); > > + void *priv; > > + } custom; > > + } match; > > +}; > > + > > +/** > > + * struct v4l2_async_subdev - sub-device descriptor, as known to a bridge > > + * @hw: this device descriptor > > + * @list: member in a list of subdevices > > + */ > > +struct v4l2_async_subdev { > > + struct v4l2_async_hw_info hw; > > + struct list_head list; > > +}; > > + > > +/** > > + * v4l2_async_subdev_list - provided by subdevices > > + * @list: member in a list of subdevices Could you please extend this comment to tell which list of subdevices ? Same for the list in v4l2_async_subdev. > > + * @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 - provided by bridges > > It probably makes sense to just say e.g. > > v4l2_async_notifier - v4l2_device notifier data structure > > I mean at least "bridge" to me doesn't sound generic enough. > > > + * @subdev_num: number of subdevices > > + * @subdev: array of pointers to subdevices > > + * @v4l2_dev: pointer to struct v4l2_device > > + * @waiting: list of subdevices, waiting for their drivers s/subdevices/v4l2_async_subdev/ ? > > + * @done: list of subdevices, already probed s/subdevices/v4l2_async_subdev_list/ ? > > + * @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_async_subdev_list *asdl); > > + int (*complete)(struct v4l2_async_notifier *notifier); > > + void (*unbind)(struct v4l2_async_notifier *notifier, > > + struct v4l2_async_subdev_list *asdl); > > I would preffer to simply pass struct v4l2_subdev * to bound/unbind. Agreed. > Since it is about just one subdevice's status change, why do we need > struct v4l2_async_subdev_list ? The bridge will need to identify the subdev. The idea AFAIK is to do so through v4l2_async_hw_info, which can be accessed through asdl->asd->hw. As asd should be considered as private from the bridge point of view, I would rather pass the subdev pointer and the hw pointer to the bound and unbind functions. > > +}; > > + > > +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); > > How about naming it v4l2_device_notifier_(un)register() ? Or v4l2_subdev_notifier_(un)register ? > > +int v4l2_async_register_subdev(struct v4l2_subdev *sd); > > +void v4l2_async_unregister_subdev(struct v4l2_subdev *sd); > > Hopefully, one day it just becomes v4l2_(un)register_subdev() :-) > > > +#endif > > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h > > index 5298d67..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> > > @@ -585,8 +586,17 @@ struct v4l2_subdev { > > void *host_priv; > > /* subdev device node */ > > struct video_device *devnode; > > > > + /* pointer to the physical device */ > > + struct device *dev; > > + struct v4l2_async_subdev_list asdl; > > Why not embed respective fields directly in struct v4l2_subdev, rather > than adding this new data structure ? I find this all code pretty much > convoluted, probably one of the reason is that there are multiple > list_head objects at various levels. I agree, that should be at least tried. We could then merge the subdev list field with the asdl list field after switching to async registration. I was also wondering whether merging v4l2_async_subdev with v4l2_async_hw_info wouldn't produce simpler code. > > }; > > > > +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) \
Hi Laurent On Mon, 22 Apr 2013, Laurent Pinchart wrote: > Hi Guennadi and Sylwester, > > On Monday 15 April 2013 13:57:17 Sylwester Nawrocki wrote: > > On 04/12/2013 05:40 PM, Guennadi Liakhovetski wrote: [snip] > > > + > > > + if (notifier->unbind) > > > + notifier->unbind(notifier, asdl); > > > + } > > > + > > > + mutex_unlock(&list_lock); > > > + > > > + if (dev) { > > > + while (i--) { > > > + if (dev[i] && device_attach(dev[i]) < 0) > > This is my last major pain point. > > To avoid race conditions we need circular references (see http://www.mail-archive.com/linux-media@vger.kernel.org/msg61092.html). We will thus need a > way to break the circle by explictly requesting the subdev to release its > resources. I'm afraid I have no well-designed solution for that at the moment. I think we really can design the framework to allow a _safe_ unloading of the bridge driver. An rmmod run is not an immediate death - we have time to clean up and release all resources properly. As an example, I just had a network interface running, but rmmod-ing of one of hardware drivers just safely destroyed the interface. In our case, rmmod <bridge> should just signal the subdevice to release the clock reference. Whether we have the required - is a separate question. Currently a call to v4l2_clk_get() increments the clock owner use-count. This isn't a problem for soc-camera, since there the soc-camera core owns the clock. For other bridge drivers they would probably own the clock themselves, so, incrementing their use-count would block their modules in memory. To avoid that we have to remove that use-count incrementing. The crash, described in the referenced mail can happen if the subdevice driver calls (typically) v4l2_clk_enable() on a clock, that's already been freed. Wouldn't a locked look-up in the global clock list in v4l2-clk.c prevent such a crash? E.g. int v4l2_clk_enable(struct v4l2_clk *clk) { struct v4l2_clk *tmp; int ret = -ENODEV; mutex_lock(&clk_lock); list_for_each_entry(tmp, &clk_list, list) if (tmp == clk) { ret = !try_module_get(clk->ops->owner); if (ret) ret = -EFAULT; break; } mutex_unlock(&clk_lock); if (ret < 0) return ret; ... } We'd have to do a similar locked look-up in v4l2_clk_put(). 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 Guennadi, On Fri, Apr 12, 2013 at 05:40:22PM +0200, Guennadi Liakhovetski wrote: > + > +static bool match_i2c(struct device *dev, struct v4l2_async_hw_info *hw_dev) > +{ > + struct i2c_client *client = i2c_verify_client(dev); > + return client && > + hw_dev->bus_type == V4L2_ASYNC_BUS_I2C && > + hw_dev->match.i2c.adapter_id == client->adapter->nr && > + hw_dev->match.i2c.address == client->addr; > +} > + > +static bool match_platform(struct device *dev, struct v4l2_async_hw_info *hw_dev) > +{ > + return hw_dev->bus_type == V4L2_ASYNC_BUS_PLATFORM && > + !strcmp(hw_dev->match.platform.name, dev_name(dev)); > +} I recently solved the same problem without being aware of your series. How about registering the asynchronous subdevices with a 'void *key' instead of a bus specific matching function? With platform based devices the key could simply be a pointer to some dummy value which is used by both the subdevice and the device in its platform_data. for the shmobile patch you have later in this series this would become: static int csi2_r2025sd_key; static struct r2025sd_platform_data r2025sd_pdata { .key = &csi2_r2025sd_key, }; static struct i2c_board_info i2c1_devices[] = { { I2C_BOARD_INFO("r2025sd", 0x32), .platform_data = &r2025sd_pdata, }, }; static struct sh_csi2_pdata csi2_info = { .flags = SH_CSI2_ECC | SH_CSI2_CRC, .key = &csi2_r2025sd_key, }; For devicetree based devices the pointer to the subdevices devicenode could be used as key. I think this would make your matching code easier and also bus type agnostic. Sascha
Hi Guennadi, On 04/23/2013 03:01 PM, Guennadi Liakhovetski wrote: > On Mon, 22 Apr 2013, Laurent Pinchart wrote: >> On Monday 15 April 2013 13:57:17 Sylwester Nawrocki wrote: >>> On 04/12/2013 05:40 PM, Guennadi Liakhovetski wrote: >>>> + >>>> + if (notifier->unbind) >>>> + notifier->unbind(notifier, asdl); >>>> + } >>>> + >>>> + mutex_unlock(&list_lock); >>>> + >>>> + if (dev) { >>>> + while (i--) { >>>> + if (dev[i]&& device_attach(dev[i])< 0) >> >> This is my last major pain point. >> >> To avoid race conditions we need circular references (see >>http://www.mail-archive.com/linux-media@vger.kernel.org/msg61092.html). >>We will thus need a >> way to break the circle by explictly requesting the subdev to release its >> resources. I'm afraid I have no well-designed solution for that at the moment. > > I think we really can design the framework to allow a _safe_ unloading of > the bridge driver. An rmmod run is not an immediate death - we have time > to clean up and release all resources properly. As an example, I just had > a network interface running, but rmmod-ing of one of hardware drivers just > safely destroyed the interface. In our case, rmmod<bridge> should just > signal the subdevice to release the clock reference. Whether we have the > required - is a separate question. It sounds like a reasonable requirements. > Currently a call to v4l2_clk_get() increments the clock owner use-count. > This isn't a problem for soc-camera, since there the soc-camera core owns > the clock. For other bridge drivers they would probably own the clock > themselves, so, incrementing their use-count would block their modules in > memory. To avoid that we have to remove that use-count incrementing. > > The crash, described in the referenced mail can happen if the subdevice > driver calls (typically) v4l2_clk_enable() on a clock, that's already been > freed. Wouldn't a locked look-up in the global clock list in v4l2-clk.c > prevent such a crash? E.g. > > int v4l2_clk_enable(struct v4l2_clk *clk) > { > struct v4l2_clk *tmp; > int ret = -ENODEV; > > mutex_lock(&clk_lock); > list_for_each_entry(tmp,&clk_list, list) > if (tmp == clk) { > ret = !try_module_get(clk->ops->owner); > if (ret) > ret = -EFAULT; > break; > } > mutex_unlock(&clk_lock); > > if (ret< 0) > return ret; > > ... > } > > We'd have to do a similar locked look-up in v4l2_clk_put(). Sounds good. This way it will not be possible to unload modules when clock is enabled, which is expected. And it seems straightforward to ensure clk_prepare/ clk_unprepare, clk_enable/clk_disable are properly balanced. If module is gone before subdev driver calls v4l2_clk_put() the clock provider module will have to ensure any source clocks it uses are properly released (clk_unprepare/clk_put). I'm looking forward to try your v10. :-) 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 Sascha On Fri, 26 Apr 2013, Sascha Hauer wrote: > Hi Guennadi, > > On Fri, Apr 12, 2013 at 05:40:22PM +0200, Guennadi Liakhovetski wrote: > > + > > +static bool match_i2c(struct device *dev, struct v4l2_async_hw_info *hw_dev) > > +{ > > + struct i2c_client *client = i2c_verify_client(dev); > > + return client && > > + hw_dev->bus_type == V4L2_ASYNC_BUS_I2C && > > + hw_dev->match.i2c.adapter_id == client->adapter->nr && > > + hw_dev->match.i2c.address == client->addr; > > +} > > + > > +static bool match_platform(struct device *dev, struct v4l2_async_hw_info *hw_dev) > > +{ > > + return hw_dev->bus_type == V4L2_ASYNC_BUS_PLATFORM && > > + !strcmp(hw_dev->match.platform.name, dev_name(dev)); > > +} > > I recently solved the same problem without being aware of your series. > > How about registering the asynchronous subdevices with a 'void *key' > instead of a bus specific matching function? Personally I don't think adding dummy data is a good idea. You can of course use pointers to real data, even just to the device object itself. But I really was trying to unbind host and subdevice devices, similar how clocks or pinmux entries or regulators are matched to their users. In the DT case we already use phandles to bind entities / pads / in whatever terms you prefer to think;-) Thanks Guennadi > With platform based devices > the key could simply be a pointer to some dummy value which is used by > both the subdevice and the device in its platform_data. for the shmobile > patch you have later in this series this would become: > > static int csi2_r2025sd_key; > > static struct r2025sd_platform_data r2025sd_pdata { > .key = &csi2_r2025sd_key, > }; > > static struct i2c_board_info i2c1_devices[] = { > { > I2C_BOARD_INFO("r2025sd", 0x32), > .platform_data = &r2025sd_pdata, > }, > }; > > static struct sh_csi2_pdata csi2_info = { > .flags = SH_CSI2_ECC | SH_CSI2_CRC, > .key = &csi2_r2025sd_key, > }; > > For devicetree based devices the pointer to the subdevices devicenode > could be used as key. > > I think this would make your matching code easier and also bus type > agnostic. > > Sascha > > -- > Pengutronix e.K. | | > Industrial Linux Solutions | http://www.pengutronix.de/ | > Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | > Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | > --- 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, Apr 26, 2013 at 11:07:24PM +0200, Guennadi Liakhovetski wrote: > Hi Sascha > > On Fri, 26 Apr 2013, Sascha Hauer wrote: > > > Hi Guennadi, > > > > On Fri, Apr 12, 2013 at 05:40:22PM +0200, Guennadi Liakhovetski wrote: > > > + > > > +static bool match_i2c(struct device *dev, struct v4l2_async_hw_info *hw_dev) > > > +{ > > > + struct i2c_client *client = i2c_verify_client(dev); > > > + return client && > > > + hw_dev->bus_type == V4L2_ASYNC_BUS_I2C && > > > + hw_dev->match.i2c.adapter_id == client->adapter->nr && > > > + hw_dev->match.i2c.address == client->addr; > > > +} > > > + > > > +static bool match_platform(struct device *dev, struct v4l2_async_hw_info *hw_dev) > > > +{ > > > + return hw_dev->bus_type == V4L2_ASYNC_BUS_PLATFORM && > > > + !strcmp(hw_dev->match.platform.name, dev_name(dev)); > > > +} > > > > I recently solved the same problem without being aware of your series. > > > > How about registering the asynchronous subdevices with a 'void *key' > > instead of a bus specific matching function? > > Personally I don't think adding dummy data is a good idea. You can of > course use pointers to real data, even just to the device object itself. > But I really was trying to unbind host and subdevice devices, similar how > clocks or pinmux entries or regulators are matched to their users. In the > DT case we already use phandles to bind entities / pads / in whatever > terms you prefer to think;-) Do you have some preview patches for doing asynchronous subdevice registration with devicetree? I mean this series and the v4l2 of parser patches are not enough for the whole picture, right? Sascha
Hi Guennadi, On Fri, Apr 12, 2013 at 05:40:22PM +0200, Guennadi Liakhovetski 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> > --- > + > +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 = NULL; > + bool (*match)(struct device *, > + struct v4l2_async_hw_info *); > + > + list_for_each_entry (asd, ¬ifier->waiting, list) { > + struct v4l2_async_hw_info *hw = &asd->hw; > + > + /* bus_type has been verified valid before */ > + switch (hw->bus_type) { > + case V4L2_ASYNC_BUS_CUSTOM: > + match = hw->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; > + } > + > + if (match && match(sd->dev, hw)) > + break; > + } > + > + return asd; 'asd' can never be NULL here. You have to explicitly return NULL when leaving the loop without match. Sascha
Hi Sascha On Tue, 30 Apr 2013, Sascha Hauer wrote: > Hi Guennadi, > > On Fri, Apr 12, 2013 at 05:40:22PM +0200, Guennadi Liakhovetski 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> > > --- > > + > > +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 = NULL; > > + bool (*match)(struct device *, > > + struct v4l2_async_hw_info *); > > + > > + list_for_each_entry (asd, ¬ifier->waiting, list) { > > + struct v4l2_async_hw_info *hw = &asd->hw; > > + > > + /* bus_type has been verified valid before */ > > + switch (hw->bus_type) { > > + case V4L2_ASYNC_BUS_CUSTOM: > > + match = hw->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; > > + } > > + > > + if (match && match(sd->dev, hw)) > > + break; > > + } > > + > > + return asd; > > 'asd' can never be NULL here. You have to explicitly return NULL when > leaving the loop without match. I've already proposed a fix for this and Laurent has proposed a simplified version. Thanks Guennadi > > Sascha > > > -- > Pengutronix e.K. | | > Industrial Linux Solutions | http://www.pengutronix.de/ | > Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | > Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | > --- 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
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..98db2e0 --- /dev/null +++ b/drivers/media/v4l2-core/v4l2-async.c @@ -0,0 +1,284 @@ +/* + * 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_hw_info *hw_dev) +{ + struct i2c_client *client = i2c_verify_client(dev); + return client && + hw_dev->bus_type == V4L2_ASYNC_BUS_I2C && + hw_dev->match.i2c.adapter_id == client->adapter->nr && + hw_dev->match.i2c.address == client->addr; +} + +static bool match_platform(struct device *dev, struct v4l2_async_hw_info *hw_dev) +{ + return hw_dev->bus_type == V4L2_ASYNC_BUS_PLATFORM && + !strcmp(hw_dev->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 = NULL; + bool (*match)(struct device *, + struct v4l2_async_hw_info *); + + list_for_each_entry (asd, ¬ifier->waiting, list) { + struct v4l2_async_hw_info *hw = &asd->hw; + + /* bus_type has been verified valid before */ + switch (hw->bus_type) { + case V4L2_ASYNC_BUS_CUSTOM: + match = hw->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; + } + + if (match && match(sd->dev, hw)) + break; + } + + return asd; +} + +static int v4l2_async_test_notify(struct v4l2_async_notifier *notifier, + struct v4l2_async_subdev_list *asdl, + struct v4l2_async_subdev *asd) +{ + int ret; + + /* Remove from the waiting list */ + list_del(&asd->list); + asdl->asd = asd; + asdl->notifier = notifier; + + if (notifier->bound) { + ret = notifier->bound(notifier, asdl); + 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, + v4l2_async_to_subdev(asdl)); + if (ret < 0) { + if (notifier->unbind) + notifier->unbind(notifier, asdl); + 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; +} + +static void v4l2_async_unregister(struct v4l2_async_subdev_list *asdl) +{ + struct v4l2_subdev *sd = v4l2_async_to_subdev(asdl); + + v4l2_async_cleanup(asdl); + + /* If we handled USB devices, we'd have to lock the parent too */ + device_release_driver(sd->dev); +} + +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; + + 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->hw.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->hw.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; + int i = 0; + struct device **dev = kcalloc(notifier->subdev_num, + sizeof(*dev), GFP_KERNEL); + if (!dev) + dev_err(notifier->v4l2_dev->dev, + "Failed to allocate device cache!\n"); + + mutex_lock(&list_lock); + + list_del(¬ifier->list); + + list_for_each_entry_safe(asdl, tmp, ¬ifier->done, list) { + if (dev) { + struct v4l2_subdev *sd = v4l2_async_to_subdev(asdl); + dev[i++] = get_device(sd->dev); + } + v4l2_async_unregister(asdl); + + if (notifier->unbind) + notifier->unbind(notifier, asdl); + } + + mutex_unlock(&list_lock); + + if (dev) { + while (i--) { + if (dev[i] && device_attach(dev[i]) < 0) + dev_err(dev[i], "Failed to re-probe to %s\n", + dev[i]->driver ? dev[i]->driver->name : "(none)"); + put_device(dev[i]); + } + kfree(dev); + } + /* + * 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; + struct device *dev; + + if (!asdl->asd) { + if (!list_empty(&asdl->list)) + v4l2_async_cleanup(asdl); + return; + } + + mutex_lock(&list_lock); + + dev = sd->dev; + + list_add(&asdl->asd->list, ¬ifier->waiting); + + v4l2_async_cleanup(asdl); + + if (notifier->unbind) + notifier->unbind(notifier, asdl); + + 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..c638f5c --- /dev/null +++ b/include/media/v4l2-async.h @@ -0,0 +1,99 @@ +/* + * 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; + +enum v4l2_async_bus_type { + V4L2_ASYNC_BUS_CUSTOM, + V4L2_ASYNC_BUS_PLATFORM, + V4L2_ASYNC_BUS_I2C, +}; + +struct v4l2_async_hw_info { + 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_hw_info *); + void *priv; + } custom; + } match; +}; + +/** + * struct v4l2_async_subdev - sub-device descriptor, as known to a bridge + * @hw: this device descriptor + * @list: member in a list of subdevices + */ +struct v4l2_async_subdev { + struct v4l2_async_hw_info hw; + struct list_head list; +}; + +/** + * v4l2_async_subdev_list - provided by subdevices + * @list: member in a list of subdevices + * @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 - provided by bridges + * @subdev_num: number of subdevices + * @subdev: array of pointers to subdevices + * @v4l2_dev: pointer to struct v4l2_device + * @waiting: list of subdevices, waiting for their drivers + * @done: list of subdevices, 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_async_subdev_list *asdl); + int (*complete)(struct v4l2_async_notifier *notifier); + void (*unbind)(struct v4l2_async_notifier *notifier, + struct v4l2_async_subdev_list *asdl); +}; + +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 5298d67..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> @@ -585,8 +586,17 @@ struct v4l2_subdev { void *host_priv; /* subdev device node */ 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> --- v9: addressed Laurent's comments (thanks) 1. moved valid hw->bus_type check 2. made v4l2_async_unregister() void 3. renamed struct v4l2_async_hw_device to struct v4l2_async_hw_info 4. merged struct v4l2_async_subdev_list into struct v4l2_subdev 5. fixed a typo 6. made subdev_num unsigned drivers/media/v4l2-core/Makefile | 3 +- drivers/media/v4l2-core/v4l2-async.c | 284 ++++++++++++++++++++++++++++++++++ include/media/v4l2-async.h | 99 ++++++++++++ include/media/v4l2-subdev.h | 10 ++ 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