Hi,
On 10/24/2012 03:54 PM, Guennadi Liakhovetski wrote:
> On Sat, 20 Oct 2012, 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 request deferred probing as long as their
>> bridge driver hasn't probed. The bridge driver during its probing submits a
>> an arbitrary number of subdevice descriptor groups to the framework to
>> manage. After that it can add callbacks to each of those groups to be
>> called at various stages during subdevice probing, e.g. after completion.
>> Then the bridge driver can request single groups to be probed, finish its
>> own probing and continue its video subsystem configuration from its
>> callbacks.
>>
>> Signed-off-by: Guennadi Liakhovetski<[email protected]>
>
> Sorry, I did indeed forget to include you on CC for this patch as promised
> in https://lkml.org/lkml/2012/10/17/306. In the patch below just look for
> the only occurrance of the device_release_driver() / device_attach(). In
> your mail you said, that only bus drivers should do this. In fact this is
> indeed what is happening here. A device is attached to two busses:
> (typically) I2C and "media." And the code below is called when the device
> is detached from the media bus.
>
> Thanks
> Guennadi
>
>> ---
>>
>> One more thing to note about this patch. Subdevice drivers, supporting
>> asynchronous probing, and using this framework, need a unified way to
>> detect, whether their probing should succeed or they should request
>> deferred probing. I implement this using device platform data. This means,
>> that all subdevice drivers, wishing to use this API will have to use the
>> same platform data struct. I don't think this is a major inconvenience,
>> but if we decide against this, we'll have to add a V4L2 function to verify
>> "are you ready for me or not." The latter would be inconvenient, because
>> then we would have to look through all registered subdevice descriptor
>> groups for this specific subdevice.
>>
>> drivers/media/v4l2-core/Makefile | 3 +-
>> drivers/media/v4l2-core/v4l2-async.c | 249
>> +++++++++++++++++++++++++++++++++
>> drivers/media/v4l2-core/v4l2-device.c | 2 +
>> include/media/v4l2-async.h | 88 ++++++++++++
>> include/media/v4l2-device.h | 6 +
>> include/media/v4l2-subdev.h | 16 ++
>> 6 files changed, 363 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 cb5fede..074e01c 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..871f116
>> --- /dev/null
>> +++ b/drivers/media/v4l2-core/v4l2-async.c
>> @@ -0,0 +1,249 @@
>> +/*
>> + * V4L2 asynchronous subdevice registration API
>> + *
>> + * Copyright (C) 2012, Guennadi Liakhovetski<[email protected]>
>> + *
>> + * 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/notifier.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_device
>> *hw_dev)
>> +{
>> + struct i2c_client *client = to_i2c_client(dev);
>> + return 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_device
>> *hw_dev)
>> +{
>> + return hw_dev->bus_type == V4L2_ASYNC_BUS_PLATFORM&&
>> + !strcmp(hw_dev->match.platform.name, dev_name(dev));
>> +}
>> +
>> +/*
>> + * I think, notifiers on different busses can run concurrently, so, we have
>> to
>> + * protect common data, e.g. sub-device lists.
>> + */
>> +static int async_notifier_cb(struct v4l2_async_group *group,
>> + unsigned long action, struct device *dev,
>> + bool (*match)(struct device *, struct v4l2_async_hw_device *))
>> +{
>> + struct v4l2_device *v4l2_dev = group->v4l2_dev;
>> + struct v4l2_async_subdev *asd;
>> + bool done;
>> + int ret;
>> +
>> + if (action != BUS_NOTIFY_BOUND_DRIVER&&
>> + action != BUS_NOTIFY_BIND_DRIVER)
>> + return NOTIFY_DONE;
>> +
>> + /* Asynchronous: have to lock */
>> + mutex_lock(&v4l2_dev->group_lock);
>> +
>> + list_for_each_entry(asd,&group->group, list) {
>> + if (match(dev,&asd->hw))
>> + break;
>> + }
>> +
>> + if (&asd->list ==&group->group) {
>> + /* Not our device */
>> + mutex_unlock(&v4l2_dev->group_lock);
>> + return NOTIFY_DONE;
>> + }
>> +
>> + asd->dev = dev;
>> +
>> + if (action == BUS_NOTIFY_BIND_DRIVER) {
>> + /*
>> + * Provide platform data to the driver: it can complete probing
>> + * now.
>> + */
>> + dev->platform_data =&asd->sdpd;
>> + mutex_unlock(&v4l2_dev->group_lock);
>> + if (group->bind_cb)
>> + group->bind_cb(group, asd);
>> + return NOTIFY_OK;
>> + }
>> +
>> + /* BUS_NOTIFY_BOUND_DRIVER */
>> + if (asd->hw.bus_type == V4L2_ASYNC_BUS_I2C)
>> + asd->sdpd.subdev = i2c_get_clientdata(to_i2c_client(dev));
>> + /*
>> + * Non-I2C subdevice drivers should take care to assign their subdevice
>> + * pointers
>> + */
>> + ret = v4l2_device_register_subdev(v4l2_dev,
>> + asd->sdpd.subdev);
>> + if (ret< 0) {
>> + mutex_unlock(&v4l2_dev->group_lock);
>> + /* FIXME: error, clean up world? */
>> + dev_err(dev, "Failed registering a subdev: %d\n", ret);
>> + return NOTIFY_OK;
>> + }
>> + list_move(&asd->list,&group->done);
>> +
>> + /* Client probed& all subdev drivers collected */
>> + done = list_empty(&group->group);
>> +
>> + mutex_unlock(&v4l2_dev->group_lock);
>> +
>> + if (group->bound_cb)
>> + group->bound_cb(group, asd);
>> +
>> + if (done&& group->complete_cb)
>> + group->complete_cb(group);
>> +
>> + return NOTIFY_OK;
>> +}
>> +
>> +static int platform_cb(struct notifier_block *nb,
>> + unsigned long action, void *data)
>> +{
>> + struct device *dev = data;
>> + struct v4l2_async_group *group = container_of(nb, struct
>> v4l2_async_group,
>> + platform_notifier);
>> +
>> + return async_notifier_cb(group, action, dev, match_platform);
>> +}
>> +
>> +static int i2c_cb(struct notifier_block *nb,
>> + unsigned long action, void *data)
>> +{
>> + struct device *dev = data;
>> + struct v4l2_async_group *group = container_of(nb, struct
>> v4l2_async_group,
>> + i2c_notifier);
>> +
>> + return async_notifier_cb(group, action, dev, match_i2c);
>> +}
>> +
>> +/*
>> + * Typically this function will be called during bridge driver probing. It
>> + * installs bus notifiers to handle asynchronously probing subdevice
>> drivers.
>> + * Once the bridge driver probing completes, subdevice drivers, waiting in
>> + * EPROBE_DEFER state are re-probed, at which point they get their platform
>> + * data, which allows them to complete probing.
>> + */
>> +int v4l2_async_group_probe(struct v4l2_async_group *group)
>> +{
>> + struct v4l2_async_subdev *asd, *tmp;
>> + bool i2c_used = false, platform_used = false;
>> + int ret;
>> +
>> + /* This group is inactive so far - no notifiers yet */
>> + list_for_each_entry_safe(asd, tmp,&group->group, list) {
>> + if (asd->sdpd.subdev) {
>> + /* Simulate a BIND event */
>> + if (group->bind_cb)
>> + group->bind_cb(group, asd);
>> +
Still we can't be sure at this moment asd->sdpd.subdev's driver is
valid and not unloaded, can we ?
In the case when a sub-device driver is probed after the host driver
(a caller of this function) I assume doing
asd->sdpd.subdev = i2c_get_clientdata(to_i2c_client(dev));
...
ret = v4l2_device_register_subdev(v4l2_dev, asd->sdpd.subdev);
is safe, because it is done in the i2c bus notifier callback itself,
i.e. under device_lock(dev).
But for these already probed sub-devices, how do we prevent races from
subdev module unloading ? By not setting CONFIG_MODULE_UNLOAD?... ;)
>> + /* Already probed, don't wait for it */
>> + ret = v4l2_device_register_subdev(group->v4l2_dev,
>> + asd->sdpd.subdev);
>> +
>> + if (ret< 0)
>> + return ret;
>> +
>> + list_move(&asd->list,&group->done);
>> + continue;
>> + }
>> +
>> + switch (asd->hw.bus_type) {
>> + case V4L2_ASYNC_BUS_PLATFORM:
>> + platform_used = true;
>> + break;
>> + case V4L2_ASYNC_BUS_I2C:
>> + i2c_used = true;
>> + }
>> + }
>> +
>> + if (list_empty(&group->group)) {
>> + if (group->complete_cb)
>> + group->complete_cb(group);
>> + return 0;
>> + }
>> +
>> + /* TODO: so far bus_register_notifier() never fails */
>> + if (platform_used) {
>> + group->platform_notifier.notifier_call = platform_cb;
>> + bus_register_notifier(&platform_bus_type,
>> + &group->platform_notifier);
>> + }
>> +
>> + if (i2c_used) {
>> + group->i2c_notifier.notifier_call = i2c_cb;
>> + bus_register_notifier(&i2c_bus_type,
>> + &group->i2c_notifier);
>> + }
>> +
>> + return 0;
>> +}
>> +EXPORT_SYMBOL(v4l2_async_group_probe);
>> +
>> +int v4l2_async_group_init(struct v4l2_device *v4l2_dev,
>> + struct v4l2_async_group *group,
>> + struct v4l2_async_subdev *asd, int cnt)
>> +{
>> + int i;
>> +
>> + if (!group)
>> + return -EINVAL;
>> +
>> + INIT_LIST_HEAD(&group->group);
>> + INIT_LIST_HEAD(&group->done);
>> + group->v4l2_dev = v4l2_dev;
>> +
>> + for (i = 0; i< cnt; i++)
>> + list_add_tail(&asd[i].list,&group->group);
>> +
>> + /* Protect the global V4L2 device group list */
>> + mutex_lock(&v4l2_dev->group_lock);
>> + list_add_tail(&group->list,&v4l2_dev->group_head);
>> + mutex_unlock(&v4l2_dev->group_lock);
>> +
>> + return 0;
>> +}
>> +EXPORT_SYMBOL(v4l2_async_group_init);
>> +
>> +void v4l2_async_group_release(struct v4l2_async_group *group)
>> +{
>> + struct v4l2_async_subdev *asd, *tmp;
>> +
>> + /* Also no problem, if notifiers haven't been registered */
>> + bus_unregister_notifier(&platform_bus_type,
>> + &group->platform_notifier);
>> + bus_unregister_notifier(&i2c_bus_type,
>> + &group->i2c_notifier);
>> +
>> + mutex_lock(&group->v4l2_dev->group_lock);
>> + list_del(&group->list);
>> + mutex_unlock(&group->v4l2_dev->group_lock);
>> +
>> + list_for_each_entry_safe(asd, tmp,&group->done, list) {
>> + v4l2_device_unregister_subdev(asd->sdpd.subdev);
>> + /* If we handled USB devices, we'd have to lock the parent too
>> */
>> + device_release_driver(asd->dev);
>> + asd->dev->platform_data = NULL;
>> + if (device_attach(asd->dev)<= 0)
>> + dev_dbg(asd->dev, "Failed to re-probe to %s\n",
>> asd->dev->driver ?
>> + asd->dev->driver->name : "(none)");
>> + list_del(&asd->list);
>> + }
>> +}
>> +EXPORT_SYMBOL(v4l2_async_group_release);
>> diff --git a/drivers/media/v4l2-core/v4l2-device.c
>> b/drivers/media/v4l2-core/v4l2-device.c
>> index 513969f..52faf2f 100644
>> --- a/drivers/media/v4l2-core/v4l2-device.c
>> +++ b/drivers/media/v4l2-core/v4l2-device.c
>> @@ -40,6 +40,8 @@ int v4l2_device_register(struct device *dev, struct
>> v4l2_device *v4l2_dev)
>> mutex_init(&v4l2_dev->ioctl_lock);
>> v4l2_prio_init(&v4l2_dev->prio);
>> kref_init(&v4l2_dev->ref);
>> + INIT_LIST_HEAD(&v4l2_dev->group_head);
>> + mutex_init(&v4l2_dev->group_lock);
>> get_device(dev);
>> v4l2_dev->dev = dev;
>> if (dev == NULL) {
>> diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h
>> new file mode 100644
>> index 0000000..8f7bc06
>> --- /dev/null
>> +++ b/include/media/v4l2-async.h
>> @@ -0,0 +1,88 @@
>> +/*
>> + * V4L2 asynchronous subdevice registration API
>> + *
>> + * Copyright (C) 2012, Guennadi Liakhovetski<[email protected]>
>> + *
>> + * 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>
>> +#include<linux/notifier.h>
>> +
>> +#include<media/v4l2-subdev.h>
>> +
>> +struct device;
>> +struct v4l2_device;
>> +
>> +enum v4l2_async_bus_type {
>> + V4L2_ASYNC_BUS_PLATFORM,
>> + V4L2_ASYNC_BUS_I2C,
>> +};
>> +
>> +struct v4l2_async_hw_device {
>> + enum v4l2_async_bus_type bus_type;
>> + union {
>> + struct {
>> + const char *name;
>> + } platform;
>> + struct {
>> + int adapter_id;
>> + unsigned short address;
>> + } i2c;
>> + } match;
>> +};
>> +
>> +/**
>> + * struct v4l2_async_subdev - device descriptor
>> + * @hw: this device descriptor
>> + * @list: member in the group
>> + * @dev: corresponding hardware device (I2C, platform,...)
>> + * @sdpd: embedded subdevice platform data
>> + * @role: this subdevice role in the video pipeline
>> + */
>> +struct v4l2_async_subdev {
>> + struct v4l2_async_hw_device hw;
>> + struct list_head list;
>> + struct device *dev;
>> + struct v4l2_subdev_platform_data sdpd;
>> + enum v4l2_subdev_role role;
>> +};
>> +
>> +/**
>> + * struct v4l2_async_group - list of device descriptors
>> + * @list: member in the v4l2 group list
>> + * @group: head of device list
>> + * @done: head of probed device list
>> + * @platform_notifier: platform bus notifier block
>> + * @i2c_notifier: I2C bus notifier block
>> + * @v4l2_dev: link to the respective struct v4l2_device
>> + * @bind: callback, called upon BUS_NOTIFY_BIND_DRIVER for each
>> + * subdevice
>> + * @complete: callback, called once after all subdevices in
>> the group
>> + * have been bound
>> + */
>> +struct v4l2_async_group {
>> + struct list_head list;
>> + struct list_head group;
>> + struct list_head done;
>> + struct notifier_block platform_notifier;
>> + struct notifier_block i2c_notifier;
>> + struct v4l2_device *v4l2_dev;
>> + int (*bind)(struct v4l2_async_group *group,
>> + struct v4l2_async_subdev *asd);
>> + int (*complete)(struct v4l2_async_group *group);
>> +};
>> +
>> +int v4l2_async_group_init(struct v4l2_device *v4l2_dev,
>> + struct v4l2_async_group *group,
>> + struct v4l2_async_subdev *asd, int cnt);
>> +int v4l2_async_group_probe(struct v4l2_async_group *group);
>> +void v4l2_async_group_release(struct v4l2_async_group *group);
>> +
>> +#endif
>> diff --git a/include/media/v4l2-device.h b/include/media/v4l2-device.h
>> index d61febf..84b18c9 100644
>> --- a/include/media/v4l2-device.h
>> +++ b/include/media/v4l2-device.h
>> @@ -21,6 +21,9 @@
>> #ifndef _V4L2_DEVICE_H
>> #define _V4L2_DEVICE_H
>>
>> +#include<linux/list.h>
>> +#include<linux/mutex.h>
>> +
>> #include<media/media-device.h>
>> #include<media/v4l2-subdev.h>
>> #include<media/v4l2-dev.h>
>> @@ -60,6 +63,9 @@ struct v4l2_device {
>> struct v4l2_prio_state prio;
>> /* BKL replacement mutex. Temporary solution only. */
>> struct mutex ioctl_lock;
>> + /* Subdevice group handling */
>> + struct mutex group_lock;
>> + struct list_head group_head;
>> /* Keep track of the references to this struct. */
>> struct kref ref;
>> /* Release function that is called when the ref count goes to 0. */
>> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
>> index 2ecd737..1756c6c 100644
>> --- a/include/media/v4l2-subdev.h
>> +++ b/include/media/v4l2-subdev.h
>> @@ -574,6 +574,22 @@ struct v4l2_subdev_fh {
>> #endif
>> };
>>
>> +enum v4l2_subdev_role {
>> + V4L2_SUBDEV_DATA_SOURCE = 1,
>> + V4L2_SUBDEV_DATA_SINK,
>> + V4L2_SUBDEV_DATA_PROCESSOR,
>> +};
>> +
>> +/**
>> + * struct v4l2_subdev_platform_data - subdev platform data
>> + * @subdev: subdevice
>> + * @platform_data: subdevice driver platform data
>> + */
>> +struct v4l2_subdev_platform_data {
>> + struct v4l2_subdev *subdev;
>> + void *platform_data;
>> +};
>> +
>> #define to_v4l2_subdev_fh(fh) \
>> container_of(fh, struct v4l2_subdev_fh, vfh)
>>
>> --
>> 1.7.2.5
Thanks,
Sylwester
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
