Re: [PATCH 2/2] media: V4L2: support asynchronous subdevice registration

Guennadi Liakhovetski
2012-10-24T13:54:33Z

Hi Greg

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 <g.liakhovetski@gmx.de>

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 <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/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);
> +
> + /* 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 <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>
> +#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
>
>

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer