Hi Guennadi,
Thanks for the patch.
On Friday 15 March 2013 22:27:48 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@xxxxxx>
> ---
>
> v6:
> 1. clock name is now <I2C adapter ID>-<I2C address>
Is that related to this patch ? :-)
> 2. modified API: instead of bind-bound-unbind use register-unregister
> 3. removed the "bind" notifier callback
This version looks much better than the previous one, but I still think work
is needed. Sorry :-)
> drivers/media/v4l2-core/Makefile | 3 +-
> drivers/media/v4l2-core/v4l2-async.c | 272 +++++++++++++++++++++++++++++++
> include/media/v4l2-async.h | 105 +++++++++++++
> 3 files changed, 379 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..ce26043
> --- /dev/null
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -0,0 +1,272 @@
> +/*
> + * V4L2 asynchronous subdevice registration API
> + *
> + * Copyright (C) 2012, Guennadi Liakhovetski <g.liakhovetski@xxxxxx>
> + *
> + * 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 = 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_device
> *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_async_subdev *asd = NULL;
> + bool (*match)(struct device *,
> + struct v4l2_async_hw_device *);
> +
> + list_for_each_entry (asd, ¬ifier->waiting, list) {
> + struct v4l2_async_hw_device *hw = &asd->hw;
> + switch (hw->bus_type) {
> + case V4L2_ASYNC_BUS_SPECIAL:
> + match = hw->match.special.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:
> + /* Oops */
> + match = NULL;
> + dev_err(notifier->v4l2_dev ? notifier->v4l2_dev->dev : NULL,
> + "Invalid bus-type %u on %p\n", hw->bus_type, asd);
> + }
> +
> + if (match && match(asdl->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,
> + asdl->subdev);
> + 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)
> +{
> + v4l2_device_unregister_subdev(asdl->subdev);
> + /* Subdevice driver will reprobe and put asdl back onto the list */
> + list_del_init(&asdl->list);
> + asdl->asd = NULL;
> + asdl->dev = NULL;
> +}
> +
> +static struct device *v4l2_async_unregister(struct v4l2_async_subdev_list
> *asdl) +{
> + struct device *dev = asdl->dev;
> +
> + v4l2_async_cleanup(asdl);
> +
> + /* If we handled USB devices, we'd have to lock the parent too */
> + device_release_driver(dev);
> + return dev;
> +}
> +
> +int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev,
> + struct v4l2_async_notifier *notifier)
> +{
> + struct v4l2_async_subdev_list *asdl, *tmp;
> + 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++)
> + list_add_tail(¬ifier->subdev[i]->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) {
> + struct v4l2_async_subdev *asd = v4l2_async_belongs(notifier, asdl);
> + int ret;
> +
> + if (!asd)
> + continue;
> +
> + ret = v4l2_async_test_notify(notifier, asdl, asd);
> + if (ret < 0) {
> + mutex_unlock(&list_lock);
I like to 'goto done;' in case of error when a lock is held.
> + return ret;
> + }
> + }
> +
ret = 0;
done:
> + mutex_unlock(&list_lock);
> +
> + return 0;
return ret;
> +}
> +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)
> + dev[i++] = get_device(asdl->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_subdev_register(struct v4l2_async_subdev_list *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_subdev_register);
> +
> +void v4l2_async_subdev_unregister(struct v4l2_async_subdev_list *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 = asdl->dev;
> +
> + list_add(&asdl->asd->list, ¬ifier->waiting);
> +
> + dev = get_device(asdl->dev);
> +
> + v4l2_async_unregister(asdl);
There's something I don't get here. v4l2_async_subdev_unregister() is called
by the subdev driver, most probably in the driver release function. I don't
think calling device_release_driver() here (and reprobing below) is a good
idea. Why is that needed ?
> + if (notifier->unbind)
> + notifier->unbind(notifier, asdl);
> +
> + mutex_unlock(&list_lock);
> +
> + /* Re-probe with lock released - avoid a deadlock */
> + if (dev && device_attach(dev) < 0)
> + dev_err(dev, "Failed to re-probe to %s\n",
> + dev->driver ? dev->driver->name : "(none)");
> +
> + put_device(dev);
> +}
> +EXPORT_SYMBOL(v4l2_async_subdev_unregister);
> diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h
> new file mode 100644
> index 0000000..c0470c6
> --- /dev/null
> +++ b/include/media/v4l2-async.h
> @@ -0,0 +1,105 @@
> +/*
> + * V4L2 asynchronous subdevice registration API
> + *
> + * Copyright (C) 2012, Guennadi Liakhovetski <g.liakhovetski@xxxxxx>
> + *
> + * 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;
> +struct v4l2_async_notifier;
> +
> +enum v4l2_async_bus_type {
> + V4L2_ASYNC_BUS_SPECIAL,
I would have called that CUSTOM instead of SPECIAL. SPECIAL sounds a bit
negative to me (but it might just be me).
> + 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;
> + struct {
> + bool (*match)(struct device *,
> + struct v4l2_async_hw_device *);
> + void *priv;
> + } special;
> + } 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_device hw;
> + struct list_head list;
> +};
> +
> +/**
> + * v4l2_async_subdev_list - provided by subdevices
> + * @list: member in a list of subdevices
> + * @dev: hardware device
> + * @subdev: V4L2 subdevice
> + * @asd: pointer to respective struct v4l2_async_subdev
> + * @notifier: pointer to managing notifier
> + */
> +struct v4l2_async_subdev_list {
> + struct list_head list;
> + struct device *dev;
> + struct v4l2_subdev *subdev;
> + struct v4l2_async_subdev *asd;
> + struct v4l2_async_notifier *notifier;
> +};
I think we could get rid of this structure. The fields it contains could be
moved to struct v4l2_subdev, or alternatively struct v4l2_subdev could contain
a v4l2_async_subdev_list pointer dynamically allocated when registering the
subdev. That way the v4l2_async_subdev_(un)register() functions would take a
v4l2_subdev pointer, and the API would be very easy to use for subdev drivers.
> +/**
> + * v4l2_async_notifier - provided by bridges
> + * @subdev_num: number of subdevices
> + * @subdev: array of pointers to subdevices
> + * @v4l2_dev: pointer to sruct 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 {
> + 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_subdev_register(struct v4l2_async_subdev_list *asdl);
> +void v4l2_async_subdev_unregister(struct v4l2_async_subdev_list *asdl);
What about calling those two functions v4l2_async_register_subdev() and
v4l2_async_unregister_subdev() ? The names would then match the synchronous
API v4l2_device_register_subdev() and v4l2_device_unregister_subdev()
functions.
> +#endif
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
