[1/6,v4] media: V4L2: support asynchronous subdevice registration

Commit Message

Guennadi Liakhovetski Jan. 7, 2013, 10:23 a.m. UTC

>From 0e1eae338ba898dc25ec60e3dba99e5581edc199 Mon Sep 17 00:00:00 2001
From: Guennadi Liakhovetski <[e-mail]>
Date: Fri, 19 Oct 2012 23:40:44 +0200
Subject: [PATCH] media: V4L2: support asynchronous subdevice registration

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 <[e-mail]>
---

v4: Fixed v4l2_async_notifier_register() for the case, when subdevices 
probe successfully before the bridge, thanks to Prabhakar for reporting

 drivers/media/v4l2-core/Makefile     |    3 +-
 drivers/media/v4l2-core/v4l2-async.c |  284 ++++++++++++++++++++++++++++++++++
 include/media/v4l2-async.h           |  113 ++++++++++++++
 3 files changed, 399 insertions(+), 1 deletions(-)
 create mode 100644 drivers/media/v4l2-core/v4l2-async.c
 create mode 100644 include/media/v4l2-async.h

Comments

Laurent Pinchart Jan. 8, 2013, 8:10 a.m. UTC | #1

Hi Guennadi,

Thanks for the patch.

On Monday 07 January 2013 11:23:55 Guennadi Liakhovetski wrote:
> >From 0e1eae338ba898dc25ec60e3dba99e5581edc199 Mon Sep 17 00:00:00 2001
> 
> From: Guennadi Liakhovetski <[e-mail]>
> Date: Fri, 19 Oct 2012 23:40:44 +0200
> Subject: [PATCH] media: V4L2: support asynchronous subdevice registration
> 
> 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 <[e-mail]>
> ---
> 
> v4: Fixed v4l2_async_notifier_register() for the case, when subdevices
> probe successfully before the bridge, thanks to Prabhakar for reporting
> 
>  drivers/media/v4l2-core/Makefile     |    3 +-
>  drivers/media/v4l2-core/v4l2-async.c |  284 +++++++++++++++++++++++++++++++
>  include/media/v4l2-async.h           |  113 ++++++++++++++
>  3 files changed, 399 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/include/media/v4l2-async.h b/include/media/v4l2-async.h
> new file mode 100644
> index 0000000..91d436d
> --- /dev/null
> +++ b/include/media/v4l2-async.h
> @@ -0,0 +1,113 @@
> +/*
> + * V4L2 asynchronous subdevice registration API
> + *
> + * Copyright (C) 2012, Guennadi Liakhovetski <[e-mail]>
> + *
> + * 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,
> +	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;
> +};
> +
> +/**
> + * 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
> + * @bind:	a subdevice driver is about to probe one of your subdevices
> + * @bound:	a subdevice driver has successfully probed one of your
> subdevices + * @complete:	all your 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 (*bind)(struct v4l2_async_notifier *notifier,
> +		    struct v4l2_async_subdev_list *asdl);
> +	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);
> +/*
> + * If subdevice probing fails any time after v4l2_async_subdev_bind(), no
> + * clean up must be called. This function is only a message of intention.
> + */
> +int v4l2_async_subdev_bind(struct v4l2_async_subdev_list *asdl);
> +int v4l2_async_subdev_bound(struct v4l2_async_subdev_list *asdl);

Could you please explain why you need both a bind notifier and a bound 
notifier ? I was expecting a single v4l2_async_subdev_register() call in 
subdev drivers (and, thinking about it, I would probably name it 
v4l2_subdev_register()).

> +void v4l2_async_subdev_unbind(struct v4l2_async_subdev_list *asdl);
> +#endif

Guennadi Liakhovetski Jan. 8, 2013, 9:25 a.m. UTC | #2

Hi Laurent

On Tue, 8 Jan 2013, Laurent Pinchart wrote:

> Hi Guennadi,
> 
> Thanks for the patch.
> 
> On Monday 07 January 2013 11:23:55 Guennadi Liakhovetski wrote:
> > >From 0e1eae338ba898dc25ec60e3dba99e5581edc199 Mon Sep 17 00:00:00 2001
> > 
> > From: Guennadi Liakhovetski <[e-mail]>
> > Date: Fri, 19 Oct 2012 23:40:44 +0200
> > Subject: [PATCH] media: V4L2: support asynchronous subdevice registration
> > 
> > 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 <[e-mail]>
> > ---
> > 
> > v4: Fixed v4l2_async_notifier_register() for the case, when subdevices
> > probe successfully before the bridge, thanks to Prabhakar for reporting
> > 
> >  drivers/media/v4l2-core/Makefile     |    3 +-
> >  drivers/media/v4l2-core/v4l2-async.c |  284 +++++++++++++++++++++++++++++++
> >  include/media/v4l2-async.h           |  113 ++++++++++++++
> >  3 files changed, 399 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/include/media/v4l2-async.h b/include/media/v4l2-async.h
> > new file mode 100644
> > index 0000000..91d436d
> > --- /dev/null
> > +++ b/include/media/v4l2-async.h
> > @@ -0,0 +1,113 @@
> > +/*
> > + * V4L2 asynchronous subdevice registration API
> > + *
> > + * Copyright (C) 2012, Guennadi Liakhovetski <[e-mail]>
> > + *
> > + * 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,
> > +	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;
> > +};
> > +
> > +/**
> > + * 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
> > + * @bind:	a subdevice driver is about to probe one of your subdevices
> > + * @bound:	a subdevice driver has successfully probed one of your
> > subdevices + * @complete:	all your 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 (*bind)(struct v4l2_async_notifier *notifier,
> > +		    struct v4l2_async_subdev_list *asdl);
> > +	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);
> > +/*
> > + * If subdevice probing fails any time after v4l2_async_subdev_bind(), no
> > + * clean up must be called. This function is only a message of intention.
> > + */
> > +int v4l2_async_subdev_bind(struct v4l2_async_subdev_list *asdl);
> > +int v4l2_async_subdev_bound(struct v4l2_async_subdev_list *asdl);
> 
> Could you please explain why you need both a bind notifier and a bound 
> notifier ? I was expecting a single v4l2_async_subdev_register() call in 
> subdev drivers (and, thinking about it, I would probably name it 
> v4l2_subdev_register()).

I think I can, yes. Because between .bind() and .bound() the subdevice 
driver does the actual hardware probing. So, .bind() is used to make sure 
the hardware can be accessed, most importantly to provide a clock to the 
subdevice. You can look at soc_camera_async_bind(). There I'm registering 
the clock for the subdevice, about to bind. Why I cannot do it before, is 
because I need subdevice name for clock matching. With I2C subdevices the 
subdevice name contains the name of the driver, adapter number and i2c 
address. The latter 2 I've got from host subdevice list. But not the 
driver name. I thought about also passing the driver name there, but that 
seemed too limiting to me. I also request regulators there, because before 
->bound() the sensor driver, but that could be done on the first call to 
soc_camera_power_on(), although doing this "first call" thingie is kind of 
hackish too. I could add one more soc-camera-power helper like 
soc_camera_prepare() or similar too. So, the main problem is the clock 
subdevice name. Also see the comment in soc_camera.c:

	/*
	 * It is ok to keep the clock for the whole soc_camera_device life-time,
	 * in principle it would be more logical to register the clock on icd
	 * creation, the only problem is, that at that time we don't know the
	 * driver name yet.
	 */

Thanks
Guennadi

> > +void v4l2_async_subdev_unbind(struct v4l2_async_subdev_list *asdl);
> > +#endif
> -- 
> Regards,
> 
> Laurent Pinchart
> 

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Laurent Pinchart Jan. 8, 2013, 9:41 a.m. UTC | #3

Hi Guennadi,

On Tuesday 08 January 2013 10:25:15 Guennadi Liakhovetski wrote:
> On Tue, 8 Jan 2013, Laurent Pinchart wrote:
> > On Monday 07 January 2013 11:23:55 Guennadi Liakhovetski wrote:
> > > >From 0e1eae338ba898dc25ec60e3dba99e5581edc199 Mon Sep 17 00:00:00 2001
> > > 
> > > From: Guennadi Liakhovetski <[e-mail]>
> > > Date: Fri, 19 Oct 2012 23:40:44 +0200
> > > Subject: [PATCH] media: V4L2: support asynchronous subdevice
> > > registration
> > > 
> > > 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 <[e-mail]>
> > > ---
> > > 
> > > v4: Fixed v4l2_async_notifier_register() for the case, when subdevices
> > > probe successfully before the bridge, thanks to Prabhakar for reporting
> > > 
> > >  drivers/media/v4l2-core/Makefile     |    3 +-
> > >  drivers/media/v4l2-core/v4l2-async.c |  284 +++++++++++++++++++++++++++
> > >  include/media/v4l2-async.h           |  113 ++++++++++++++
> > >  3 files changed, 399 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/include/media/v4l2-async.h b/include/media/v4l2-async.h
> > > new file mode 100644
> > > index 0000000..91d436d
> > > --- /dev/null
> > > +++ b/include/media/v4l2-async.h
> > > @@ -0,0 +1,113 @@
> > > +/*
> > > + * V4L2 asynchronous subdevice registration API
> > > + *
> > > + * Copyright (C) 2012, Guennadi Liakhovetski <[e-mail]>
> > > + *
> > > + * 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,
> > > +	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;
> > > +};
> > > +
> > > +/**
> > > + * 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
> > > + * @bind:	a subdevice driver is about to probe one of your subdevices
> > > + * @bound:	a subdevice driver has successfully probed one of your
> > > subdevices + * @complete:	all your 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 (*bind)(struct v4l2_async_notifier *notifier,
> > > +		    struct v4l2_async_subdev_list *asdl);
> > > +	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);
> > > +/*
> > > + * If subdevice probing fails any time after v4l2_async_subdev_bind(),
> > > no
> > > + * clean up must be called. This function is only a message of
> > > intention.
> > > + */
> > > +int v4l2_async_subdev_bind(struct v4l2_async_subdev_list *asdl);
> > > +int v4l2_async_subdev_bound(struct v4l2_async_subdev_list *asdl);
> > 
> > Could you please explain why you need both a bind notifier and a bound
> > notifier ? I was expecting a single v4l2_async_subdev_register() call in
> > subdev drivers (and, thinking about it, I would probably name it
> > v4l2_subdev_register()).
> 
> I think I can, yes. Because between .bind() and .bound() the subdevice
> driver does the actual hardware probing. So, .bind() is used to make sure
> the hardware can be accessed, most importantly to provide a clock to the
> subdevice. You can look at soc_camera_async_bind(). There I'm registering
> the clock for the subdevice, about to bind. Why I cannot do it before, is
> because I need subdevice name for clock matching. With I2C subdevices the
> subdevice name contains the name of the driver, adapter number and i2c
> address. The latter 2 I've got from host subdevice list. But not the
> driver name. I thought about also passing the driver name there, but that
> seemed too limiting to me. I also request regulators there, because before
> ->bound() the sensor driver, but that could be done on the first call to
> soc_camera_power_on(), although doing this "first call" thingie is kind of
> hackish too. I could add one more soc-camera-power helper like
> soc_camera_prepare() or similar too.

I think a soc_camera_power_init() function (or similar) would be a good idea, 
yes.

> So, the main problem is the clock
> subdevice name. Also see the comment in soc_camera.c:
> 
> 	/*
> 	 * It is ok to keep the clock for the whole soc_camera_device life-time,
> 	 * in principle it would be more logical to register the clock on icd
> 	 * creation, the only problem is, that at that time we don't know the
> 	 * driver name yet.
> 	 */

I think we should fix that problem instead of shaping the async API around a 
workaround :-)

From the subdevice point of view, the probe function should request resources, 
perform whatever initialization is needed (including verifying that the 
hardware is functional when possible), and the register the subdev with the 
code if everything succeeded. Splitting registration into bind() and bound() 
appears a bit as a workaround to me.

If we need a workaround, I'd rather pass the device name in addition to the 
I2C adapter number and address, instead of embedding the workaround in this 
new API.

> > > +void v4l2_async_subdev_unbind(struct v4l2_async_subdev_list *asdl);
> > > +#endif

Guennadi Liakhovetski Jan. 8, 2013, 9:56 a.m. UTC | #4

On Tue, 8 Jan 2013, Laurent Pinchart wrote:

> Hi Guennadi,
> 
> On Tuesday 08 January 2013 10:25:15 Guennadi Liakhovetski wrote:
> > On Tue, 8 Jan 2013, Laurent Pinchart wrote:
> > > On Monday 07 January 2013 11:23:55 Guennadi Liakhovetski wrote:
> > > > >From 0e1eae338ba898dc25ec60e3dba99e5581edc199 Mon Sep 17 00:00:00 2001

[snip]

> > > > +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);
> > > > +/*
> > > > + * If subdevice probing fails any time after v4l2_async_subdev_bind(),
> > > > no
> > > > + * clean up must be called. This function is only a message of
> > > > intention.
> > > > + */
> > > > +int v4l2_async_subdev_bind(struct v4l2_async_subdev_list *asdl);
> > > > +int v4l2_async_subdev_bound(struct v4l2_async_subdev_list *asdl);
> > > 
> > > Could you please explain why you need both a bind notifier and a bound
> > > notifier ? I was expecting a single v4l2_async_subdev_register() call in
> > > subdev drivers (and, thinking about it, I would probably name it
> > > v4l2_subdev_register()).
> > 
> > I think I can, yes. Because between .bind() and .bound() the subdevice
> > driver does the actual hardware probing. So, .bind() is used to make sure
> > the hardware can be accessed, most importantly to provide a clock to the
> > subdevice. You can look at soc_camera_async_bind(). There I'm registering
> > the clock for the subdevice, about to bind. Why I cannot do it before, is
> > because I need subdevice name for clock matching. With I2C subdevices the
> > subdevice name contains the name of the driver, adapter number and i2c
> > address. The latter 2 I've got from host subdevice list. But not the
> > driver name. I thought about also passing the driver name there, but that
> > seemed too limiting to me. I also request regulators there, because before
> > ->bound() the sensor driver, but that could be done on the first call to
> > soc_camera_power_on(), although doing this "first call" thingie is kind of
> > hackish too. I could add one more soc-camera-power helper like
> > soc_camera_prepare() or similar too.
> 
> I think a soc_camera_power_init() function (or similar) would be a good idea, 
> yes.
> 
> > So, the main problem is the clock
> > subdevice name. Also see the comment in soc_camera.c:
> > 
> > 	/*
> > 	 * It is ok to keep the clock for the whole soc_camera_device life-time,
> > 	 * in principle it would be more logical to register the clock on icd
> > 	 * creation, the only problem is, that at that time we don't know the
> > 	 * driver name yet.
> > 	 */
> 
> I think we should fix that problem instead of shaping the async API around a 
> workaround :-)
> 
> >From the subdevice point of view, the probe function should request resources, 
> perform whatever initialization is needed (including verifying that the 
> hardware is functional when possible), and the register the subdev with the 
> code if everything succeeded. Splitting registration into bind() and bound() 
> appears a bit as a workaround to me.
> 
> If we need a workaround, I'd rather pass the device name in addition to the 
> I2C adapter number and address, instead of embedding the workaround in this 
> new API.

...or we can change the I2C subdevice name format. The actual need to do

	snprintf(clk_name, sizeof(clk_name), "%s %d-%04x",
		 asdl->dev->driver->name,
		 i2c_adapter_id(client->adapter), client->addr);

in soc-camera now to exactly match the subdevice name, as created by 
v4l2_i2c_subdev_init(), doesn't make me specifically happy either. What if 
the latter changes at some point? Or what if one driver wishes to create 
several subdevices for one I2C device?

Thanks
Guennadi

> > > > +void v4l2_async_subdev_unbind(struct v4l2_async_subdev_list *asdl);
> > > > +#endif
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Laurent Pinchart Jan. 8, 2013, 10:21 a.m. UTC | #5

Hi Guennadi,

On Tuesday 08 January 2013 10:56:43 Guennadi Liakhovetski wrote:
> On Tue, 8 Jan 2013, Laurent Pinchart wrote:
> > On Tuesday 08 January 2013 10:25:15 Guennadi Liakhovetski wrote:
> > > On Tue, 8 Jan 2013, Laurent Pinchart wrote:
> > > > On Monday 07 January 2013 11:23:55 Guennadi Liakhovetski wrote:
> > > > > >From 0e1eae338ba898dc25ec60e3dba99e5581edc199 Mon Sep 17 00:00:00
> > > > > >2001
> 
> [snip]
> 
> > > > > +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);
> > > > > +/*
> > > > > + * If subdevice probing fails any time after
> > > > > v4l2_async_subdev_bind(),
> > > > > no
> > > > > + * clean up must be called. This function is only a message of
> > > > > intention.
> > > > > + */
> > > > > +int v4l2_async_subdev_bind(struct v4l2_async_subdev_list *asdl);
> > > > > +int v4l2_async_subdev_bound(struct v4l2_async_subdev_list *asdl);
> > > > 
> > > > Could you please explain why you need both a bind notifier and a bound
> > > > notifier ? I was expecting a single v4l2_async_subdev_register() call
> > > > in subdev drivers (and, thinking about it, I would probably name it
> > > > v4l2_subdev_register()).
> > > 
> > > I think I can, yes. Because between .bind() and .bound() the subdevice
> > > driver does the actual hardware probing. So, .bind() is used to make
> > > sure the hardware can be accessed, most importantly to provide a clock
> > > to the subdevice. You can look at soc_camera_async_bind(). There I'm
> > > registering the clock for the subdevice, about to bind. Why I cannot do
> > > it before, is because I need subdevice name for clock matching. With I2C
> > > subdevices the subdevice name contains the name of the driver, adapter
> > > number and i2c address. The latter 2 I've got from host subdevice list.
> > > But not the driver name. I thought about also passing the driver name
> > > there, but that seemed too limiting to me. I also request regulators
> > > there, because before ->bound() the sensor driver, but that could be
> > > done on the first call to soc_camera_power_on(), although doing this
> > > "first call" thingie is kind of hackish too. I could add one more soc-
> > > camera-power helper like soc_camera_prepare() or similar too.
> > 
> > I think a soc_camera_power_init() function (or similar) would be a good
> > idea, yes.
> > 
> > > So, the main problem is the clock
> > > 
> > > subdevice name. Also see the comment in soc_camera.c:
> > > 	/*
> > > 	 * It is ok to keep the clock for the whole soc_camera_device
> > > 	 life-time,
> > > 	 * in principle it would be more logical to register the clock on icd
> > > 	 * creation, the only problem is, that at that time we don't know the
> > > 	 * driver name yet.
> > > 	 */
> > 
> > I think we should fix that problem instead of shaping the async API around
> > a workaround :-)
> > 
> > From the subdevice point of view, the probe function should request
> > resources, perform whatever initialization is needed (including verifying
> > that the hardware is functional when possible), and the register the
> > subdev with the code if everything succeeded. Splitting registration into
> > bind() and bound() appears a bit as a workaround to me.
> > 
> > If we need a workaround, I'd rather pass the device name in addition to
> > the I2C adapter number and address, instead of embedding the workaround in
> > this new API.
> 
> ...or we can change the I2C subdevice name format. The actual need to do
> 
> 	snprintf(clk_name, sizeof(clk_name), "%s %d-%04x",
> 		 asdl->dev->driver->name,
> 		 i2c_adapter_id(client->adapter), client->addr);
> 
> in soc-camera now to exactly match the subdevice name, as created by
> v4l2_i2c_subdev_init(), doesn't make me specifically happy either. What if
> the latter changes at some point? Or what if one driver wishes to create
> several subdevices for one I2C device?

The common clock framework uses %d-%04x, maybe we could use that as well for 
clock names ?

> > > > > +void v4l2_async_subdev_unbind(struct v4l2_async_subdev_list *asdl);
> > > > > +#endif

Guennadi Liakhovetski Jan. 8, 2013, 10:26 a.m. UTC | #6

On Tue, 8 Jan 2013, Laurent Pinchart wrote:

> Hi Guennadi,
> 
> On Tuesday 08 January 2013 10:56:43 Guennadi Liakhovetski wrote:
> > On Tue, 8 Jan 2013, Laurent Pinchart wrote:
> > > On Tuesday 08 January 2013 10:25:15 Guennadi Liakhovetski wrote:
> > > > On Tue, 8 Jan 2013, Laurent Pinchart wrote:
> > > > > On Monday 07 January 2013 11:23:55 Guennadi Liakhovetski wrote:
> > > > > > >From 0e1eae338ba898dc25ec60e3dba99e5581edc199 Mon Sep 17 00:00:00
> > > > > > >2001
> > 
> > [snip]
> > 
> > > > > > +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);
> > > > > > +/*
> > > > > > + * If subdevice probing fails any time after
> > > > > > v4l2_async_subdev_bind(),
> > > > > > no
> > > > > > + * clean up must be called. This function is only a message of
> > > > > > intention.
> > > > > > + */
> > > > > > +int v4l2_async_subdev_bind(struct v4l2_async_subdev_list *asdl);
> > > > > > +int v4l2_async_subdev_bound(struct v4l2_async_subdev_list *asdl);
> > > > > 
> > > > > Could you please explain why you need both a bind notifier and a bound
> > > > > notifier ? I was expecting a single v4l2_async_subdev_register() call
> > > > > in subdev drivers (and, thinking about it, I would probably name it
> > > > > v4l2_subdev_register()).
> > > > 
> > > > I think I can, yes. Because between .bind() and .bound() the subdevice
> > > > driver does the actual hardware probing. So, .bind() is used to make
> > > > sure the hardware can be accessed, most importantly to provide a clock
> > > > to the subdevice. You can look at soc_camera_async_bind(). There I'm
> > > > registering the clock for the subdevice, about to bind. Why I cannot do
> > > > it before, is because I need subdevice name for clock matching. With I2C
> > > > subdevices the subdevice name contains the name of the driver, adapter
> > > > number and i2c address. The latter 2 I've got from host subdevice list.
> > > > But not the driver name. I thought about also passing the driver name
> > > > there, but that seemed too limiting to me. I also request regulators
> > > > there, because before ->bound() the sensor driver, but that could be
> > > > done on the first call to soc_camera_power_on(), although doing this
> > > > "first call" thingie is kind of hackish too. I could add one more soc-
> > > > camera-power helper like soc_camera_prepare() or similar too.
> > > 
> > > I think a soc_camera_power_init() function (or similar) would be a good
> > > idea, yes.
> > > 
> > > > So, the main problem is the clock
> > > > 
> > > > subdevice name. Also see the comment in soc_camera.c:
> > > > 	/*
> > > > 	 * It is ok to keep the clock for the whole soc_camera_device
> > > > 	 life-time,
> > > > 	 * in principle it would be more logical to register the clock on icd
> > > > 	 * creation, the only problem is, that at that time we don't know the
> > > > 	 * driver name yet.
> > > > 	 */
> > > 
> > > I think we should fix that problem instead of shaping the async API around
> > > a workaround :-)
> > > 
> > > From the subdevice point of view, the probe function should request
> > > resources, perform whatever initialization is needed (including verifying
> > > that the hardware is functional when possible), and the register the
> > > subdev with the code if everything succeeded. Splitting registration into
> > > bind() and bound() appears a bit as a workaround to me.
> > > 
> > > If we need a workaround, I'd rather pass the device name in addition to
> > > the I2C adapter number and address, instead of embedding the workaround in
> > > this new API.
> > 
> > ...or we can change the I2C subdevice name format. The actual need to do
> > 
> > 	snprintf(clk_name, sizeof(clk_name), "%s %d-%04x",
> > 		 asdl->dev->driver->name,
> > 		 i2c_adapter_id(client->adapter), client->addr);
> > 
> > in soc-camera now to exactly match the subdevice name, as created by
> > v4l2_i2c_subdev_init(), doesn't make me specifically happy either. What if
> > the latter changes at some point? Or what if one driver wishes to create
> > several subdevices for one I2C device?
> 
> The common clock framework uses %d-%04x, maybe we could use that as well for 
> clock names ?

And preserve the subdevice names? Then matching would be more difficult 
and less precise. Or change subdevice names too? I think, we can do the 
latter, since anyway at any time only one driver can be attached to an I2C 
device.

> > > > > > +void v4l2_async_subdev_unbind(struct v4l2_async_subdev_list *asdl);
> > > > > > +#endif

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-media" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Laurent Pinchart Jan. 8, 2013, 10:35 a.m. UTC | #7

On Tuesday 08 January 2013 11:26:57 Guennadi Liakhovetski wrote:
> On Tue, 8 Jan 2013, Laurent Pinchart wrote:
> > On Tuesday 08 January 2013 10:56:43 Guennadi Liakhovetski wrote:
> > > On Tue, 8 Jan 2013, Laurent Pinchart wrote:
> > > > On Tuesday 08 January 2013 10:25:15 Guennadi Liakhovetski wrote:
> > > > > On Tue, 8 Jan 2013, Laurent Pinchart wrote:

[snip]

> > > > > > Could you please explain why you need both a bind notifier and a
> > > > > > bound notifier ? I was expecting a single
> > > > > > v4l2_async_subdev_register() call in subdev drivers (and, thinking
> > > > > > about it, I would probably name it v4l2_subdev_register()).
> > > > > 
> > > > > I think I can, yes. Because between .bind() and .bound() the
> > > > > subdevice driver does the actual hardware probing. So, .bind() is
> > > > > used to make sure the hardware can be accessed, most importantly to
> > > > > provide a clock to the subdevice. You can look at
> > > > > soc_camera_async_bind(). There I'm registering the clock for the
> > > > > subdevice, about to bind. Why I cannot do it before, is because I
> > > > > need subdevice name for clock matching. With I2C subdevices the
> > > > > subdevice name contains the name of the driver, adapter number and
> > > > > i2c address. The latter 2 I've got from host subdevice list. But not
> > > > > the driver name. I thought about also passing the driver name there,
> > > > > but that seemed too limiting to me. I also request regulators there,
> > > > > because before ->bound() the sensor driver, but that could be done
> > > > > on the first call to soc_camera_power_on(), although doing this
> > > > > "first call" thingie is kind of hackish too. I could add one more
> > > > > soc-camera-power helper like soc_camera_prepare() or similar too.
> > > > 
> > > > I think a soc_camera_power_init() function (or similar) would be a
> > > > good idea, yes.
> > > > 
> > > > > So, the main problem is the clock
> > > > > 
> > > > > subdevice name. Also see the comment in soc_camera.c:
> > > > > 	/*
> > > > > 	 * It is ok to keep the clock for the whole soc_camera_device
> > > > > 	 * life-time, in principle it would be more logical to register
> > > > > 	 * the clock on icd creation, the only problem is, that at that
> > > > > 	 * time we don't know the driver name yet.
> > > > > 	 */
> > > > 
> > > > I think we should fix that problem instead of shaping the async API
> > > > around a workaround :-)
> > > > 
> > > > From the subdevice point of view, the probe function should request
> > > > resources, perform whatever initialization is needed (including
> > > > verifying that the hardware is functional when possible), and the
> > > > register the subdev with the code if everything succeeded. Splitting
> > > > registration into bind() and bound() appears a bit as a workaround to
> > > > me.
> > > > 
> > > > If we need a workaround, I'd rather pass the device name in addition
> > > > to the I2C adapter number and address, instead of embedding the
> > > > workaround in this new API.
> > > 
> > > ...or we can change the I2C subdevice name format. The actual need to do
> > > 
> > > 	snprintf(clk_name, sizeof(clk_name), "%s %d-%04x",
> > > 	
> > > 		 asdl->dev->driver->name,
> > > 		 i2c_adapter_id(client->adapter), client->addr);
> > > 
> > > in soc-camera now to exactly match the subdevice name, as created by
> > > v4l2_i2c_subdev_init(), doesn't make me specifically happy either. What
> > > if the latter changes at some point? Or what if one driver wishes to
> > > create several subdevices for one I2C device?
> > 
> > The common clock framework uses %d-%04x, maybe we could use that as well
> > for clock names ?
> 
> And preserve the subdevice names? Then matching would be more difficult
> and less precise. Or change subdevice names too? I think, we can do the
> latter, since anyway at any time only one driver can be attached to an I2C
> device.

That's right. Where else is the subdev name used ?

> > > > > > > +void v4l2_async_subdev_unbind(struct v4l2_async_subdev_list
> > > > > > > *asdl);
> > > > > > > +#endif

Sylwester Nawrocki Jan. 8, 2013, 11:37 a.m. UTC | #8

Hi,

On 01/08/2013 11:35 AM, Laurent Pinchart wrote:
>>>>> If we need a workaround, I'd rather pass the device name in addition
>>>>> to the I2C adapter number and address, instead of embedding the
>>>>> workaround in this new API.
>>>>
>>>> ...or we can change the I2C subdevice name format. The actual need to do
>>>>
>>>> 	snprintf(clk_name, sizeof(clk_name), "%s %d-%04x",
>>>> 	
>>>> 		 asdl->dev->driver->name,
>>>> 		 i2c_adapter_id(client->adapter), client->addr);
>>>>
>>>> in soc-camera now to exactly match the subdevice name, as created by
>>>> v4l2_i2c_subdev_init(), doesn't make me specifically happy either. What
>>>> if the latter changes at some point? Or what if one driver wishes to
>>>> create several subdevices for one I2C device?
>>>
>>> The common clock framework uses %d-%04x, maybe we could use that as well
>>> for clock names ?
>>
>> And preserve the subdevice names? Then matching would be more difficult
>> and less precise. Or change subdevice names too? I think, we can do the
>> latter, since anyway at any time only one driver can be attached to an I2C
>> device.
>
> That's right. Where else is the subdev name used ?

Subdev names are exposed to user space by the media controller API.
So they are really part of an ABI, aren't they ?

Also having I2C bus number or I2C slave address as part of the subdev
name makes it more difficult to write portable applications. Hence
in sensor drivers I used to overwrite subdev name to remove I2C bus
and slave address, as the format used v4l2_i2c_subdev_init() seemed
highly unsuitable..

--

Regards,
Sylwester
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Laurent Pinchart Jan. 8, 2013, 12:41 p.m. UTC | #9

Hi Sylwester,

On Tuesday 08 January 2013 12:37:36 Sylwester Nawrocki wrote:
> On 01/08/2013 11:35 AM, Laurent Pinchart wrote:
> >>>>> If we need a workaround, I'd rather pass the device name in addition
> >>>>> to the I2C adapter number and address, instead of embedding the
> >>>>> workaround in this new API.
> >>>> 
> >>>> ...or we can change the I2C subdevice name format. The actual need to
> >>>> do
> >>>> 
> >>>> 	snprintf(clk_name, sizeof(clk_name), "%s %d-%04x",
> >>>> 		 asdl->dev->driver->name,
> >>>> 		 i2c_adapter_id(client->adapter), client->addr);
> >>>> 
> >>>> in soc-camera now to exactly match the subdevice name, as created by
> >>>> v4l2_i2c_subdev_init(), doesn't make me specifically happy either. What
> >>>> if the latter changes at some point? Or what if one driver wishes to
> >>>> create several subdevices for one I2C device?
> >>> 
> >>> The common clock framework uses %d-%04x, maybe we could use that as well
> >>> for clock names ?
> >> 
> >> And preserve the subdevice names? Then matching would be more difficult
> >> and less precise. Or change subdevice names too? I think, we can do the
> >> latter, since anyway at any time only one driver can be attached to an
> >> I2C device.
> > 
> > That's right. Where else is the subdev name used ?
> 
> Subdev names are exposed to user space by the media controller API.
> So they are really part of an ABI, aren't they ?

They're used to construct the name exposed to userspace, but the media 
controller core could probably handle that internally by concatenating the 
driver name and the subdev name.

> Also having I2C bus number or I2C slave address as part of the subdev
> name makes it more difficult to write portable applications. Hence
> in sensor drivers I used to overwrite subdev name to remove I2C bus
> and slave address, as the format used v4l2_i2c_subdev_init() seemed
> highly unsuitable..

This clearly shows that we need to discuss the matter and agree on a common 
mode of operation.

Aren't applications that use the subdev name directly inherently non-portable 
anyway ? If you want your application to support different boards/sensors/SoCs 
you should discover the pipeline and find the sensor by iterating over 
entities, instead of using the sensor entity name.

Sakari Ailus Jan. 8, 2013, 1:15 p.m. UTC | #10

Hi Laurent,

On Tue, Jan 08, 2013 at 01:41:59PM +0100, Laurent Pinchart wrote:
> Hi Sylwester,
> 
> On Tuesday 08 January 2013 12:37:36 Sylwester Nawrocki wrote:
> > On 01/08/2013 11:35 AM, Laurent Pinchart wrote:
> > >>>>> If we need a workaround, I'd rather pass the device name in addition
> > >>>>> to the I2C adapter number and address, instead of embedding the
> > >>>>> workaround in this new API.
> > >>>> 
> > >>>> ...or we can change the I2C subdevice name format. The actual need to
> > >>>> do
> > >>>> 
> > >>>> 	snprintf(clk_name, sizeof(clk_name), "%s %d-%04x",
> > >>>> 		 asdl->dev->driver->name,
> > >>>> 		 i2c_adapter_id(client->adapter), client->addr);
> > >>>> 
> > >>>> in soc-camera now to exactly match the subdevice name, as created by
> > >>>> v4l2_i2c_subdev_init(), doesn't make me specifically happy either. What
> > >>>> if the latter changes at some point? Or what if one driver wishes to
> > >>>> create several subdevices for one I2C device?
> > >>> 
> > >>> The common clock framework uses %d-%04x, maybe we could use that as well
> > >>> for clock names ?
> > >> 
> > >> And preserve the subdevice names? Then matching would be more difficult
> > >> and less precise. Or change subdevice names too? I think, we can do the
> > >> latter, since anyway at any time only one driver can be attached to an
> > >> I2C device.
> > > 
> > > That's right. Where else is the subdev name used ?
> > 
> > Subdev names are exposed to user space by the media controller API.
> > So they are really part of an ABI, aren't they ?
> 
> They're used to construct the name exposed to userspace, but the media 
> controller core could probably handle that internally by concatenating the 
> driver name and the subdev name.
> 
> > Also having I2C bus number or I2C slave address as part of the subdev
> > name makes it more difficult to write portable applications. Hence
> > in sensor drivers I used to overwrite subdev name to remove I2C bus
> > and slave address, as the format used v4l2_i2c_subdev_init() seemed
> > highly unsuitable..
> 
> This clearly shows that we need to discuss the matter and agree on a common 
> mode of operation.
> 
> Aren't applications that use the subdev name directly inherently non-portable 
> anyway ? If you want your application to support different boards/sensors/SoCs 

Well, the name could come from a configuration file to distinguish e.g.
between primary and secondary sensors.

For what it's worth, the SMIA++ driver uses the actual name of the sensor
since there are about 10 sensors supported at the moment, and calling them
all smiapp-xxxx looks a bit insipid. So one has to talk to the sensor to
know what it's called.

This isn't strictly mandatory but a nice feature.

> you should discover the pipeline and find the sensor by iterating over 
> entities, instead of using the sensor entity name.

To be fully generic, yes.

Sylwester Nawrocki Jan. 8, 2013, 2:27 p.m. UTC | #11

Hi Laurent,

On 01/08/2013 01:41 PM, Laurent Pinchart wrote:
>> Subdev names are exposed to user space by the media controller API.
>> So they are really part of an ABI, aren't they ?
>
> They're used to construct the name exposed to userspace, but the media
> controller core could probably handle that internally by concatenating the
> driver name and the subdev name.
>
>> Also having I2C bus number or I2C slave address as part of the subdev
>> name makes it more difficult to write portable applications. Hence
>> in sensor drivers I used to overwrite subdev name to remove I2C bus
>> and slave address, as the format used v4l2_i2c_subdev_init() seemed
>> highly unsuitable..
>
> This clearly shows that we need to discuss the matter and agree on a common
> mode of operation.
>
> Aren't applications that use the subdev name directly inherently non-portable
> anyway ? If you want your application to support different boards/sensors/SoCs
> you should discover the pipeline and find the sensor by iterating over
> entities, instead of using the sensor entity name.

It depends on how we define the entity names :) It the names change from 
board
to board and are completely unreliable then user space applications 
using them
have no any chance to be generic. Nevertheless, struct 
media_entity_desc::name
[1] has currently no specific semantics defined, e.g. for V4L2.

It's likely way better for the kernel space to be no constrained by the 
subdev
user space name requirement. But having no clear definition of the 
entity names
brings more trouble to user space. E.g. when a sensor exposes multiple 
subdevs.
User space library/application could then reference them by entity name. It
seems difficult to me to handle such multiple subdev devices without 
somehow
reliable subdev names.

I imagine a system with multiple sensors of same type sitting on 
different I2C
busses, then appending I2C bus number/slave address to the name would be 
useful.

And is it always possible to discover the pipeline, as opposite to e.g. 
using
configuration file to activate all required links ? Configurations could be
of course per board file, but then at least we should keep subdev names
constant through the kernel releases.


[1] http://linuxtv.org/downloads/v4l-dvb-apis/media-ioc-enum-entities.html

--

Regards,
Sylwester
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Sylwester Nawrocki Jan. 8, 2013, 2:52 p.m. UTC | #12

Hi,

On 01/08/2013 09:10 AM, Laurent Pinchart wrote:
>> +/*
>> + * If subdevice probing fails any time after v4l2_async_subdev_bind(), no
>> + * clean up must be called. This function is only a message of intention.
>> + */
>> +int v4l2_async_subdev_bind(struct v4l2_async_subdev_list *asdl);
>> +int v4l2_async_subdev_bound(struct v4l2_async_subdev_list *asdl);
>
> Could you please explain why you need both a bind notifier and a bound
> notifier ? I was expecting a single v4l2_async_subdev_register() call in
> subdev drivers (and, thinking about it, I would probably name it
> v4l2_subdev_register()).

I expected it to be done this way too, and I also used 
v4l2_subdev_register()
name in my early version of the subdev registration code where subdevs
were registering themselves to the v4l2 core.

BTW, this might not be most important thing here, but do we need separate
file, i.e. v4l2-async.c, instead of for example putting it in 
v4l2-device.c ?

>> +void v4l2_async_subdev_unbind(struct v4l2_async_subdev_list *asdl);
>> +#endif

--

Regards,
Sylwester
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Sylwester Nawrocki Jan. 8, 2013, 7:26 p.m. UTC | #13

Hi Guennadi,

Cc: LKML

On 01/08/2013 11:26 AM, Guennadi Liakhovetski wrote:
> On Tue, 8 Jan 2013, Laurent Pinchart wrote:
>> On Tuesday 08 January 2013 10:56:43 Guennadi Liakhovetski wrote:
>>> On Tue, 8 Jan 2013, Laurent Pinchart wrote:
>>>> On Tuesday 08 January 2013 10:25:15 Guennadi Liakhovetski wrote:
>>>>> On Tue, 8 Jan 2013, Laurent Pinchart wrote:
>>>>>> On Monday 07 January 2013 11:23:55 Guennadi Liakhovetski wrote:
>>>>>>> > From 0e1eae338ba898dc25ec60e3dba99e5581edc199 Mon Sep 17 00:00:00
>>>>>>>> 2001
>>>
>>> [snip]
>>>
>>>>>>> +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);
>>>>>>> +/*
>>>>>>> + * If subdevice probing fails any time after
>>>>>>> v4l2_async_subdev_bind(),
>>>>>>> no
>>>>>>> + * clean up must be called. This function is only a message of
>>>>>>> intention.
>>>>>>> + */
>>>>>>> +int v4l2_async_subdev_bind(struct v4l2_async_subdev_list *asdl);
>>>>>>> +int v4l2_async_subdev_bound(struct v4l2_async_subdev_list *asdl);
>>>>>>
>>>>>> Could you please explain why you need both a bind notifier and a bound
>>>>>> notifier ? I was expecting a single v4l2_async_subdev_register() call
>>>>>> in subdev drivers (and, thinking about it, I would probably name it
>>>>>> v4l2_subdev_register()).
>>>>>
>>>>> I think I can, yes. Because between .bind() and .bound() the subdevice
>>>>> driver does the actual hardware probing. So, .bind() is used to make
>>>>> sure the hardware can be accessed, most importantly to provide a clock
>>>>> to the subdevice. You can look at soc_camera_async_bind(). There I'm
>>>>> registering the clock for the subdevice, about to bind. Why I cannot do
>>>>> it before, is because I need subdevice name for clock matching. With I2C
>>>>> subdevices the subdevice name contains the name of the driver, adapter
>>>>> number and i2c address. The latter 2 I've got from host subdevice list.
>>>>> But not the driver name. I thought about also passing the driver name
>>>>> there, but that seemed too limiting to me. I also request regulators
>>>>> there, because before ->bound() the sensor driver, but that could be
>>>>> done on the first call to soc_camera_power_on(), although doing this
>>>>> "first call" thingie is kind of hackish too. I could add one more soc-
>>>>> camera-power helper like soc_camera_prepare() or similar too.
>>>>
>>>> I think a soc_camera_power_init() function (or similar) would be a good
>>>> idea, yes.
>>>>
>>>>> So, the main problem is the clock
>>>>>
>>>>> subdevice name. Also see the comment in soc_camera.c:
>>>>> 	/*
>>>>> 	 * It is ok to keep the clock for the whole soc_camera_device
>>>>> 	 life-time,
>>>>> 	 * in principle it would be more logical to register the clock on icd
>>>>> 	 * creation, the only problem is, that at that time we don't know the
>>>>> 	 * driver name yet.
>>>>> 	 */
>>>>
>>>> I think we should fix that problem instead of shaping the async API around
>>>> a workaround :-)
>>>>
>>>>  From the subdevice point of view, the probe function should request
>>>> resources, perform whatever initialization is needed (including verifying
>>>> that the hardware is functional when possible), and the register the
>>>> subdev with the code if everything succeeded. Splitting registration into
>>>> bind() and bound() appears a bit as a workaround to me.
>>>>
>>>> If we need a workaround, I'd rather pass the device name in addition to
>>>> the I2C adapter number and address, instead of embedding the workaround in
>>>> this new API.
>>>
>>> ...or we can change the I2C subdevice name format. The actual need to do
>>>
>>> 	snprintf(clk_name, sizeof(clk_name), "%s %d-%04x",
>>> 		 asdl->dev->driver->name,
>>> 		 i2c_adapter_id(client->adapter), client->addr);
>>>
>>> in soc-camera now to exactly match the subdevice name, as created by
>>> v4l2_i2c_subdev_init(), doesn't make me specifically happy either. What if
>>> the latter changes at some point? Or what if one driver wishes to create
>>> several subdevices for one I2C device?
>>
>> The common clock framework uses %d-%04x, maybe we could use that as well for
>> clock names ?
>
> And preserve the subdevice names? Then matching would be more difficult
> and less precise. Or change subdevice names too? I think, we can do the
> latter, since anyway at any time only one driver can be attached to an I2C
> device.

I'm just wondering why we can't associate the clock with relevant device,
rather than its driver ? This could eliminate the problem of unknown
sub-device name at the host driver, before sub-device driver is actually
probed, couldn't it ?

--

Thanks,
Sylwester
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Laurent Pinchart Jan. 8, 2013, 9:17 p.m. UTC | #14

Hi Sakari,

On Tuesday 08 January 2013 15:15:44 Sakari Ailus wrote:
> On Tue, Jan 08, 2013 at 01:41:59PM +0100, Laurent Pinchart wrote:
> > On Tuesday 08 January 2013 12:37:36 Sylwester Nawrocki wrote:
> > > On 01/08/2013 11:35 AM, Laurent Pinchart wrote:
> > > >>>>> If we need a workaround, I'd rather pass the device name in
> > > >>>>> addition to the I2C adapter number and address, instead of
> > > >>>>> embedding the workaround in this new API.
> > > >>>> 
> > > >>>> ...or we can change the I2C subdevice name format. The actual need
> > > >>>> to do
> > > >>>> 
> > > >>>> 	snprintf(clk_name, sizeof(clk_name), "%s %d-%04x",
> > > >>>> 		 asdl->dev->driver->name,
> > > >>>> 		 i2c_adapter_id(client->adapter), client->addr);
> > > >>>> 
> > > >>>> in soc-camera now to exactly match the subdevice name, as created
> > > >>>> by v4l2_i2c_subdev_init(), doesn't make me specifically happy
> > > >>>> either. What if the latter changes at some point? Or what if one
> > > >>>> driver wishes to create several subdevices for one I2C device?
> > > >>> 
> > > >>> The common clock framework uses %d-%04x, maybe we could use that as
> > > >>> well for clock names ?
> > > >> 
> > > >> And preserve the subdevice names? Then matching would be more
> > > >> difficult and less precise. Or change subdevice names too? I think,
> > > >> we can do the latter, since anyway at any time only one driver can be
> > > >> attached to an I2C device.
> > > > 
> > > > That's right. Where else is the subdev name used ?
> > > 
> > > Subdev names are exposed to user space by the media controller API.
> > > So they are really part of an ABI, aren't they ?
> > 
> > They're used to construct the name exposed to userspace, but the media
> > controller core could probably handle that internally by concatenating the
> > driver name and the subdev name.
> > 
> > > Also having I2C bus number or I2C slave address as part of the subdev
> > > name makes it more difficult to write portable applications. Hence
> > > in sensor drivers I used to overwrite subdev name to remove I2C bus
> > > and slave address, as the format used v4l2_i2c_subdev_init() seemed
> > > highly unsuitable..
> > 
> > This clearly shows that we need to discuss the matter and agree on a
> > common mode of operation.
> > 
> > Aren't applications that use the subdev name directly inherently
> > non-portable anyway ? If you want your application to support different
> > boards/sensors/SoCs
>
> Well, the name could come from a configuration file to distinguish e.g.
> between primary and secondary sensors.

In that case having the I2C bus number and address in the name doesn't create 
an extra portability issue, does it ?
 
> For what it's worth, the SMIA++ driver uses the actual name of the sensor
> since there are about 10 sensors supported at the moment, and calling them
> all smiapp-xxxx looks a bit insipid. So one has to talk to the sensor to
> know what it's called.
> 
> This isn't strictly mandatory but a nice feature.
> 
> > you should discover the pipeline and find the sensor by iterating over
> > entities, instead of using the sensor entity name.
> 
> To be fully generic, yes.

Laurent Pinchart Jan. 8, 2013, 9:20 p.m. UTC | #15

Hi Sylwester,

On Tuesday 08 January 2013 15:27:09 Sylwester Nawrocki wrote:
> On 01/08/2013 01:41 PM, Laurent Pinchart wrote:
> >> Subdev names are exposed to user space by the media controller API.
> >> So they are really part of an ABI, aren't they ?
> > 
> > They're used to construct the name exposed to userspace, but the media
> > controller core could probably handle that internally by concatenating the
> > driver name and the subdev name.
> > 
> >> Also having I2C bus number or I2C slave address as part of the subdev
> >> name makes it more difficult to write portable applications. Hence
> >> in sensor drivers I used to overwrite subdev name to remove I2C bus
> >> and slave address, as the format used v4l2_i2c_subdev_init() seemed
> >> highly unsuitable..
> > 
> > This clearly shows that we need to discuss the matter and agree on a
> > common mode of operation.
> > 
> > Aren't applications that use the subdev name directly inherently
> > non-portable anyway ? If you want your application to support different
> > boards/sensors/SoCs you should discover the pipeline and find the sensor
> > by iterating over entities, instead of using the sensor entity name.
> 
> It depends on how we define the entity names :) It the names change from
> board to board and are completely unreliable then user space applications
> using them have no any chance to be generic. Nevertheless, struct
> media_entity_desc::name [1] has currently no specific semantics defined,
> e.g. for V4L2.
> 
> It's likely way better for the kernel space to be no constrained by the
> subdev user space name requirement. But having no clear definition of the
> entity names brings more trouble to user space. E.g. when a sensor exposes
> multiple subdevs. User space library/application could then reference them
> by entity name. It seems difficult to me to handle such multiple subdev
> devices without somehow reliable subdev names.

I agree, I think naming rules are required.

> I imagine a system with multiple sensors of same type sitting on different
> I2C busses, then appending I2C bus number/slave address to the name would be
> useful.

And an application can always strip off the I2C bus number and address and use 
the sensor name only if needed (or use the sensor name in addition to the I2C 
information).

> And is it always possible to discover the pipeline, as opposite to e.g.
> using configuration file to activate all required links ? Configurations
> could be of course per board file, but then at least we should keep subdev
> names constant through the kernel releases.

I'm fine with applications more or less hardcoding the pipeline, and I agree 
that subdev names need to be kept constant across kernel releases (and, very 
importantly, well-defined). Now we "just" need to agree on naming rules :-)

> [1] http://linuxtv.org/downloads/v4l-dvb-apis/media-ioc-enum-entities.html

Laurent Pinchart Jan. 8, 2013, 9:23 p.m. UTC | #16

Hi Sylwester,

On Tuesday 08 January 2013 15:52:21 Sylwester Nawrocki wrote:
> On 01/08/2013 09:10 AM, Laurent Pinchart wrote:
> >> +/*
> >> + * If subdevice probing fails any time after v4l2_async_subdev_bind(),
> >> + * no clean up must be called. This function is only a message of
> >> + * intention.
> >> + */
> >> +int v4l2_async_subdev_bind(struct v4l2_async_subdev_list *asdl);
> >> +int v4l2_async_subdev_bound(struct v4l2_async_subdev_list *asdl);
> > 
> > Could you please explain why you need both a bind notifier and a bound
> > notifier ? I was expecting a single v4l2_async_subdev_register() call in
> > subdev drivers (and, thinking about it, I would probably name it
> > v4l2_subdev_register()).
> 
> I expected it to be done this way too, and I also used
> v4l2_subdev_register() name in my early version of the subdev registration
> code where subdevs were registering themselves to the v4l2 core.

I think we can switch back to v4l2_subdev_register() if we can solve the clock 
name issue. This doesn't seem impossible at first sight.

> BTW, this might not be most important thing here, but do we need separate
> file, i.e. v4l2-async.c, instead of for example putting it in v4l2-device.c
> ?

I'm fine with both, but I tend to try and keep source files not too large for 
ease of reading. Depending on the amount of code we end up adding, moving the 
functions to v4l2-device.c might be a good idea.

> >> +void v4l2_async_subdev_unbind(struct v4l2_async_subdev_list *asdl);
> >> +#endif

Guennadi Liakhovetski March 12, 2013, 4:59 p.m. UTC | #17

Hi all

(this is a _conscious_ case of top-posting, hopefully a justified one :-) )

The discussion, I'd like to continue here is pretty old (2 months), so, 
instead of asking everyone to go and re-read the thread, I'll try to 
summarise the problem here and encourage everyone to contribute to a final 
solution :-)

The last version of my "asynchronous V4L2 subdevice registration" patch 
stumbled upon a problem of V4L2 clock naming, which has to match a 
subdevice name. Currently on I2C subdevice names are produced per

	snprintf(clk_name, sizeof(clk_name), "%s %d-%04x",
		 client->dev.driver->name,
		 i2c_adapter_id(client->adapter), client->addr);

The problem with this is, that to match this name in a V4L2 clock, the 
clock provider (typically a V4L2 bridge driver) has to know the name of a 
driver, that is going to bind to the I2C device. Normally bridge drivers 
don't have that information, so, it either has to be provided to them 
additionally beforehand, or they have to wait until an I2C driver has 
been bound to the I2C device. Both these options are inconvenient.

We can now decide to drop the driver name from the subdevice name, but 
subdevice names are exposed to the user-space. It is possible, that 
user-space software exists, that relies on specific subdevice names. If we 
change them, that software can break. A possible solution would be to 
remove the driver name from the subdevice name for internal purposes, but 
to re-add it, when exposing to the user-space.

We can also leave the name as is and relax the clock matching criteria: 
instead of a full string match we can match a substring and verify, that 
the matching part is at the end of the name. We could even actually 
produce the complete name by prepending the driver name in the V4L2 clock 
helpers, but that would make the helpers aware of V4L2 subdevice 
internals, which isn't very good either.

Also note, that non-standard subdevice names are possible, there are also 
non-I2C subdevices, multiple subdevices per I2C device are also a 
possibility. So, maybe explicitly passing a device name from board data, 
similarly to how the clock API does that, is indeed the best option.

I'll keep an original message quoted below for now, in case someone wants 
to through a glance, but feel free to remove it when replying.

Thanks
Guennadi

On Tue, 8 Jan 2013, Laurent Pinchart wrote:

> Hi Sakari,
> 
> On Tuesday 08 January 2013 15:15:44 Sakari Ailus wrote:
> > On Tue, Jan 08, 2013 at 01:41:59PM +0100, Laurent Pinchart wrote:
> > > On Tuesday 08 January 2013 12:37:36 Sylwester Nawrocki wrote:
> > > > On 01/08/2013 11:35 AM, Laurent Pinchart wrote:
> > > > >>>>> If we need a workaround, I'd rather pass the device name in
> > > > >>>>> addition to the I2C adapter number and address, instead of
> > > > >>>>> embedding the workaround in this new API.
> > > > >>>> 
> > > > >>>> ...or we can change the I2C subdevice name format. The actual need
> > > > >>>> to do
> > > > >>>> 
> > > > >>>> 	snprintf(clk_name, sizeof(clk_name), "%s %d-%04x",
> > > > >>>> 		 asdl->dev->driver->name,
> > > > >>>> 		 i2c_adapter_id(client->adapter), client->addr);
> > > > >>>> 
> > > > >>>> in soc-camera now to exactly match the subdevice name, as created
> > > > >>>> by v4l2_i2c_subdev_init(), doesn't make me specifically happy
> > > > >>>> either. What if the latter changes at some point? Or what if one
> > > > >>>> driver wishes to create several subdevices for one I2C device?
> > > > >>> 
> > > > >>> The common clock framework uses %d-%04x, maybe we could use that as
> > > > >>> well for clock names ?
> > > > >> 
> > > > >> And preserve the subdevice names? Then matching would be more
> > > > >> difficult and less precise. Or change subdevice names too? I think,
> > > > >> we can do the latter, since anyway at any time only one driver can be
> > > > >> attached to an I2C device.
> > > > > 
> > > > > That's right. Where else is the subdev name used ?
> > > > 
> > > > Subdev names are exposed to user space by the media controller API.
> > > > So they are really part of an ABI, aren't they ?
> > > 
> > > They're used to construct the name exposed to userspace, but the media
> > > controller core could probably handle that internally by concatenating the
> > > driver name and the subdev name.
> > > 
> > > > Also having I2C bus number or I2C slave address as part of the subdev
> > > > name makes it more difficult to write portable applications. Hence
> > > > in sensor drivers I used to overwrite subdev name to remove I2C bus
> > > > and slave address, as the format used v4l2_i2c_subdev_init() seemed
> > > > highly unsuitable..
> > > 
> > > This clearly shows that we need to discuss the matter and agree on a
> > > common mode of operation.
> > > 
> > > Aren't applications that use the subdev name directly inherently
> > > non-portable anyway ? If you want your application to support different
> > > boards/sensors/SoCs
> >
> > Well, the name could come from a configuration file to distinguish e.g.
> > between primary and secondary sensors.
> 
> In that case having the I2C bus number and address in the name doesn't create 
> an extra portability issue, does it ?
>  
> > For what it's worth, the SMIA++ driver uses the actual name of the sensor
> > since there are about 10 sensors supported at the moment, and calling them
> > all smiapp-xxxx looks a bit insipid. So one has to talk to the sensor to
> > know what it's called.
> > 
> > This isn't strictly mandatory but a nice feature.
> > 
> > > you should discover the pipeline and find the sensor by iterating over
> > > entities, instead of using the sensor entity name.
> > 
> > To be fully generic, yes.
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sh" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/media/v4l2-core/Makefile b/drivers/media/v4l2-core/Makefile
index d065c01..b667ced 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..55c2ad0
--- /dev/null
+++ b/drivers/media/v4l2-core/v4l2-async.c
@@ -0,0 +1,284 @@ 
+/*
+ * V4L2 asynchronous subdevice registration API
+ *
+ * Copyright (C) 2012, Guennadi Liakhovetski <[e-mail]>
+ *
+ * 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));
+}
+
+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, &notifier->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 = v4l2_async_belongs(notifier, asdl);
+	if (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, &notifier->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(&notifier->waiting) && notifier->complete)
+			return notifier->complete(notifier);
+
+		return 0;
+	}
+
+	return -EPROBE_DEFER;
+}
+
+static struct device *v4l2_async_unbind(struct v4l2_async_subdev_list *asdl)
+{
+	struct device *dev = asdl->dev;
+	v4l2_device_unregister_subdev(asdl->subdev);
+	/* Subdevice driver will reprobe and put asdl back onto the list */
+	list_del(&asdl->list);
+	asdl->asd = NULL;
+	/* 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;
+	int i;
+
+	notifier->v4l2_dev = v4l2_dev;
+	INIT_LIST_HEAD(&notifier->waiting);
+	INIT_LIST_HEAD(&notifier->done);
+
+	for (i = 0; i < notifier->subdev_num; i++)
+		list_add_tail(&notifier->subdev[i]->list, &notifier->waiting);
+
+	mutex_lock(&list_lock);
+
+	/* Keep also completed notifiers on the list */
+	list_add(&notifier->list, &notifier_list);
+
+	list_for_each_entry(asdl, &subdev_list, list) {
+		int ret = v4l2_async_test_notify(notifier, asdl);
+		if (ret < 0 && ret != -EPROBE_DEFER) {
+			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(&notifier->list);
+
+	list_for_each_entry_safe(asdl, tmp, &notifier->done, list) {
+		if (dev)
+			dev[i++] = get_device(asdl->dev);
+		v4l2_async_unbind(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_bind(struct v4l2_async_subdev_list *asdl)
+{
+	struct v4l2_async_notifier *notifier;
+	int ret = 0;
+
+	mutex_lock(&list_lock);
+
+	list_for_each_entry(notifier, &notifier_list, list) {
+		struct v4l2_async_subdev *asd = v4l2_async_belongs(notifier,
+								   asdl);
+		/*
+		 * Whether or not probing succeeds - this is the right hardware
+		 * subdevice descriptor and we can provide it to the notifier
+		 */
+		if (asd) {
+			asdl->asd = asd;
+			if (notifier->bind)
+				ret = notifier->bind(notifier, asdl);
+			break;
+		}
+	}
+
+	mutex_unlock(&list_lock);
+
+	return ret;
+}
+EXPORT_SYMBOL(v4l2_async_subdev_bind);
+
+int v4l2_async_subdev_bound(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, &notifier_list, list) {
+		int ret = v4l2_async_test_notify(notifier, asdl);
+		if (ret != -EPROBE_DEFER) {
+			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_bound);
+
+void v4l2_async_subdev_unbind(struct v4l2_async_subdev_list *asdl)
+{
+	struct v4l2_async_notifier *notifier = asdl->notifier;
+	struct device *dev;
+
+	if (!asdl->asd)
+		return;
+
+	mutex_lock(&list_lock);
+
+	dev = asdl->dev;
+
+	list_add(&asdl->asd->list, &notifier->waiting);
+
+	dev = get_device(asdl->dev);
+
+	v4l2_async_unbind(asdl);
+
+	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_unbind);
diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h
new file mode 100644
index 0000000..91d436d
--- /dev/null
+++ b/include/media/v4l2-async.h
@@ -0,0 +1,113 @@ 
+/*
+ * V4L2 asynchronous subdevice registration API
+ *
+ * Copyright (C) 2012, Guennadi Liakhovetski <[e-mail]>
+ *
+ * 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,
+	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;
+};
+
+/**
+ * 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
+ * @bind:	a subdevice driver is about to probe one of your subdevices
+ * @bound:	a subdevice driver has successfully probed one of your subdevices
+ * @complete:	all your 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 (*bind)(struct v4l2_async_notifier *notifier,
+		    struct v4l2_async_subdev_list *asdl);
+	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);
+/*
+ * If subdevice probing fails any time after v4l2_async_subdev_bind(), no clean
+ * up must be called. This function is only a message of intention.
+ */
+int v4l2_async_subdev_bind(struct v4l2_async_subdev_list *asdl);
+int v4l2_async_subdev_bound(struct v4l2_async_subdev_list *asdl);
+void v4l2_async_subdev_unbind(struct v4l2_async_subdev_list *asdl);
+#endif