[RFC/PATCH 1/2] v4l: Add generic board subdev registration function

[RFC/PATCH 1/2] v4l: Add generic board subdev registration function

[Laurent Pinchart]

The new v4l2_new_subdev_board() function creates and register a subdev
based on generic board information. The board information structure
includes a bus type and bus type-specific information.

Only I2C and SPI busses are currently supported.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/video/v4l2-common.c |   70 +++++++++++++++++++++++++++++++++++++
 drivers/media/video/v4l2-device.c |    8 ++++
 include/media/v4l2-common.h       |   28 +++++++++++++++
 include/media/v4l2-subdev.h       |    3 ++
 4 files changed, 109 insertions(+), 0 deletions(-)

Hi everybody,

This approach has been briefly discussed during the Warsaw V4L meeting. Now
that support for platform subdevs has been requested, I'd like to move bus type
handling to the V4L2 core instead of duplicating the logic in every driver. As
usual, comments will be appreciated.

diff --git a/drivers/media/video/v4l2-common.c b/drivers/media/video/v4l2-common.c
index 06b9f9f..46aee94 100644
--- a/drivers/media/video/v4l2-common.c
+++ b/drivers/media/video/v4l2-common.c
@@ -474,6 +474,76 @@ EXPORT_SYMBOL_GPL(v4l2_spi_new_subdev);
 
 #endif /* defined(CONFIG_SPI) */
 
+/*
+ * v4l2_new_subdev_board - Register a subdevice based on board information
+ * @v4l2_dev: Parent V4L2 device
+ * @info: I2C subdevs board information array
+ *
+ * Register a subdevice identified by a geenric board information structure. The
+ * structure contains the bus type and bus type-specific information.
+ *
+ * Return a pointer to the subdevice if registration was successful, or NULL
+ * otherwise.
+ */
+struct v4l2_subdev *v4l2_new_subdev_board(struct v4l2_device *v4l2_dev,
+		struct v4l2_subdev_board_info *info)
+{
+	struct v4l2_subdev *subdev;
+
+	switch (info->type) {
+#if defined(CONFIG_I2C)
+	case V4L2_SUBDEV_BUS_TYPE_I2C: {
+		struct i2c_adapter *adapter;
+
+		adapter = i2c_get_adapter(info->info.i2c.i2c_adapter_id);
+		if (adapter == NULL) {
+			printk(KERN_ERR "%s: Unable to get I2C adapter %d for "
+				"device %s/%u\n", __func__,
+				info->info.i2c.i2c_adapter_id,
+				info->info.i2c.board_info->type,
+				info->info.i2c.board_info->addr);
+			return NULL;
+		}
+
+		subdev = v4l2_i2c_new_subdev_board(v4l2_dev, adapter,
+					info->info.i2c.board_info, NULL);
+		if (subdev == NULL) {
+			i2c_put_adapter(adapter);
+			return NULL;
+		}
+
+		subdev->flags |= V4L2_SUBDEV_FL_RELEASE_ADAPTER;
+		break;
+	}
+#endif /* defined(CONFIG_I2C) */
+#if defined(CONFIG_SPI)
+	case V4L2_SUBDEV_BUS_TYPE_SPI: {
+		struct spi_master *master;
+
+		master = spi_busnum_to_master(info->info.spi->bus_num);
+		if (master == NULL) {
+			printk(KERN_ERR "%s: Unable to get SPI master %u for "
+				"device %s/%u\n", __func__,
+				info->info.spi->bus_num,
+				info->info.spi->modalias,
+				info->info.spi->chip_select);
+			return NULL;
+		}
+
+		subdev = v4l2_spi_new_subdev(v4l2_dev, master, info->info.spi);
+		spi_master_put(master);
+		break;
+	}
+#endif /* defined(CONFIG_SPI) */
+	default:
+		subdev = NULL;
+		break;
+	}
+
+	return subdev;
+}
+EXPORT_SYMBOL_GPL(v4l2_new_subdev_board);
+
 /* Clamp x to be between min and max, aligned to a multiple of 2^align.  min
  * and max don't have to be aligned, but there must be at least one valid
  * value.  E.g., min=17,max=31,align=4 is not allowed as there are no multiples
diff --git a/drivers/media/video/v4l2-device.c b/drivers/media/video/v4l2-device.c
index 4aae501..cfd9caf 100644
--- a/drivers/media/video/v4l2-device.c
+++ b/drivers/media/video/v4l2-device.c
@@ -246,5 +246,13 @@ void v4l2_device_unregister_subdev(struct v4l2_subdev *sd)
 #endif
 	video_unregister_device(&sd->devnode);
 	module_put(sd->owner);
+
+#if defined(CONFIG_I2C) || (defined(CONFIG_I2C_MODULE) && defined(MODULE))
+	if ((sd->flags & V4L2_SUBDEV_FL_IS_I2C) &&
+	    (sd->flags & V4L2_SUBDEV_FL_RELEASE_ADAPTER)) {
+		struct i2c_client *client = v4l2_get_subdevdata(sd);
+		i2c_put_adapter(client->adapter);
+	}
+#endif
 }
 EXPORT_SYMBOL_GPL(v4l2_device_unregister_subdev);
diff --git a/include/media/v4l2-common.h b/include/media/v4l2-common.h
index a298ec4..88c38d9 100644
--- a/include/media/v4l2-common.h
+++ b/include/media/v4l2-common.h
@@ -171,6 +171,34 @@ void v4l2_spi_subdev_init(struct v4l2_subdev *sd, struct spi_device *spi,
 		const struct v4l2_subdev_ops *ops);
 #endif
 
+/* -------------------------------------------------------------------------- */
+
+/* Generic helper functions */
+
+struct v4l2_subdev_i2c_board_info {
+	struct i2c_board_info *board_info;
+	int i2c_adapter_id;
+};
+
+enum v4l2_subdev_bus_type {
+	V4L2_SUBDEV_BUS_TYPE_NONE,
+	V4L2_SUBDEV_BUS_TYPE_I2C,
+	V4L2_SUBDEV_BUS_TYPE_SPI,
+};
+
+struct v4l2_subdev_board_info {
+	enum v4l2_subdev_bus_type type;
+	union {
+		struct v4l2_subdev_i2c_board_info i2c;
+		struct spi_board_info *spi;
+	} info;
+};
+
+/* Create a subdevice and load its module. The info argumentidentifies the
+ * subdev bus type and the bus type-specific information. */
+struct v4l2_subdev *v4l2_new_subdev_board(struct v4l2_device *v4l2_dev,
+		struct v4l2_subdev_board_info *info);
+
 /* ------------------------------------------------------------------------- */
 
 /* Note: these remaining ioctls/structs should be removed as well, but they are
diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index 1562c4f..bc1c4d8 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -483,6 +483,9 @@ struct v4l2_subdev_internal_ops {
 #define V4L2_SUBDEV_FL_HAS_DEVNODE		(1U << 2)
 /* Set this flag if this subdev generates events. */
 #define V4L2_SUBDEV_FL_HAS_EVENTS		(1U << 3)
+/* Set by the core if the bus adapter needs to be released. Do NOT use in
+ * drivers. */
+#define V4L2_SUBDEV_FL_RELEASE_ADAPTER		(1U << 4)
 
 /* Each instance of a subdev driver should create this struct, either
    stand-alone or embedded in a larger struct.
-- 
Regards,

Laurent Pinchart

[RFC/PATCH 2/2] omap3isp: Use generic subdev registration function

[Laurent Pinchart]

Replace custom subdev registration with a call to
v4l2_new_subdev_board().

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/video/omap3isp/isp.c |   32 +++++++++-----------------------
 drivers/media/video/omap3isp/isp.h |    7 +------
 2 files changed, 10 insertions(+), 29 deletions(-)

diff --git a/drivers/media/video/omap3isp/isp.c b/drivers/media/video/omap3isp/isp.c
index 472a693..8280165 100644
--- a/drivers/media/video/omap3isp/isp.c
+++ b/drivers/media/video/omap3isp/isp.c
@@ -1642,9 +1642,9 @@ static void isp_unregister_entities(struct isp_device *isp)
 /*
  * isp_register_subdev_group - Register a group of subdevices
  * @isp: OMAP3 ISP device
- * @board_info: I2C subdevs board information array
+ * @board_info: V4L2 subdevs board information array
  *
- * Register all I2C subdevices in the board_info array. The array must be
+ * Register all V4L2 subdevices in the board_info array. The array must be
  * terminated by a NULL entry, and the first entry must be the sensor.
  *
  * Return a pointer to the sensor media entity if it has been successfully
@@ -1652,36 +1652,22 @@ static void isp_unregister_entities(struct isp_device *isp)
  */
 static struct v4l2_subdev *
 isp_register_subdev_group(struct isp_device *isp,
-		     struct isp_subdev_i2c_board_info *board_info)
+		     struct v4l2_subdev_board_info *board_info)
 {
 	struct v4l2_subdev *sensor = NULL;
-	unsigned int first;
-
-	if (board_info->board_info == NULL)
-		return NULL;
+	unsigned int i;
 
-	for (first = 1; board_info->board_info; ++board_info, first = 0) {
+	for (i = 0; board_info; ++board_info, ++i) {
 		struct v4l2_subdev *subdev;
-		struct i2c_adapter *adapter;
-
-		adapter = i2c_get_adapter(board_info->i2c_adapter_id);
-		if (adapter == NULL) {
-			printk(KERN_ERR "%s: Unable to get I2C adapter %d for "
-				"device %s\n", __func__,
-				board_info->i2c_adapter_id,
-				board_info->board_info->type);
-			continue;
-		}
 
-		subdev = v4l2_i2c_new_subdev_board(&isp->v4l2_dev, adapter,
-				board_info->board_info, NULL);
+		subdev = v4l2_new_subdev_board(&isp->v4l2_dev, board_info);
 		if (subdev == NULL) {
-			printk(KERN_ERR "%s: Unable to register subdev %s\n",
-				__func__, board_info->board_info->type);
+			printk(KERN_ERR "%s: Unable to register subdev %u\n",
+				__func__, i);
 			continue;
 		}
 
-		if (first)
+		if (i == 0)
 			sensor = subdev;
 	}
 
diff --git a/drivers/media/video/omap3isp/isp.h b/drivers/media/video/omap3isp/isp.h
index 2620c40..c3ecc36 100644
--- a/drivers/media/video/omap3isp/isp.h
+++ b/drivers/media/video/omap3isp/isp.h
@@ -180,13 +180,8 @@ struct isp_csi2_platform_data {
 	unsigned vpclk_div:2;
 };
 
-struct isp_subdev_i2c_board_info {
-	struct i2c_board_info *board_info;
-	int i2c_adapter_id;
-};
-
 struct isp_v4l2_subdevs_group {
-	struct isp_subdev_i2c_board_info *subdevs;
+	struct v4l2_subdev_board_info *subdevs;
 	enum isp_interface_type interface;
 	union {
 		struct isp_parallel_platform_data parallel;
-- 
1.7.3.4

Re: [RFC/PATCH 1/2] v4l: Add generic board subdev registration function

[Sylwester Nawrocki]

Hi Laurent,

On 05/19/2011 08:34 PM, Laurent Pinchart wrote:
> The new v4l2_new_subdev_board() function creates and register a subdev
> based on generic board information. The board information structure
> includes a bus type and bus type-specific information.
> 
> Only I2C and SPI busses are currently supported.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  drivers/media/video/v4l2-common.c |   70 +++++++++++++++++++++++++++++++++++++
>  drivers/media/video/v4l2-device.c |    8 ++++
>  include/media/v4l2-common.h       |   28 +++++++++++++++
>  include/media/v4l2-subdev.h       |    3 ++
>  4 files changed, 109 insertions(+), 0 deletions(-)
> 
> Hi everybody,
> 
> This approach has been briefly discussed during the Warsaw V4L meeting. Now
> that support for platform subdevs has been requested, I'd like to move bus type
> handling to the V4L2 core instead of duplicating the logic in every driver. As
> usual, comments will be appreciated.

Thanks for taking care of this.

> 
> diff --git a/drivers/media/video/v4l2-common.c b/drivers/media/video/v4l2-common.c
> index 06b9f9f..46aee94 100644
> --- a/drivers/media/video/v4l2-common.c
> +++ b/drivers/media/video/v4l2-common.c
> @@ -474,6 +474,76 @@ EXPORT_SYMBOL_GPL(v4l2_spi_new_subdev);
>  
>  #endif /* defined(CONFIG_SPI) */
>  
> +/*
> + * v4l2_new_subdev_board - Register a subdevice based on board information
> + * @v4l2_dev: Parent V4L2 device
> + * @info: I2C subdevs board information array

"info" doesn't appear to be a (I2C subdevs) array.

> + *
> + * Register a subdevice identified by a geenric board information structure. The

s/geenric/generic ?

> + * structure contains the bus type and bus type-specific information.
> + *
> + * Return a pointer to the subdevice if registration was successful, or NULL
> + * otherwise.
> + */
> +struct v4l2_subdev *v4l2_new_subdev_board(struct v4l2_device *v4l2_dev,
> +		struct v4l2_subdev_board_info *info)
> +{
> +	struct v4l2_subdev *subdev;
> +
> +	switch (info->type) {
> +#if defined(CONFIG_I2C)
> +	case V4L2_SUBDEV_BUS_TYPE_I2C: {
> +		struct i2c_adapter *adapter;
> +
> +		adapter = i2c_get_adapter(info->info.i2c.i2c_adapter_id);
> +		if (adapter == NULL) {
> +			printk(KERN_ERR "%s: Unable to get I2C adapter %d for "
> +				"device %s/%u\n", __func__,
> +				info->info.i2c.i2c_adapter_id,
> +				info->info.i2c.board_info->type,
> +				info->info.i2c.board_info->addr);
> +			return NULL;
> +		}
> +
> +		subdev = v4l2_i2c_new_subdev_board(v4l2_dev, adapter,
> +					info->info.i2c.board_info, NULL);
> +		if (subdev == NULL) {
> +			i2c_put_adapter(adapter);
> +			return NULL;
> +		}
> +
> +		subdev->flags |= V4L2_SUBDEV_FL_RELEASE_ADAPTER;
> +		break;
> +	}
> +#endif /* defined(CONFIG_I2C) */
> +#if defined(CONFIG_SPI)
> +	case V4L2_SUBDEV_BUS_TYPE_SPI: {
> +		struct spi_master *master;
> +
> +		master = spi_busnum_to_master(info->info.spi->bus_num);
> +		if (master == NULL) {
> +			printk(KERN_ERR "%s: Unable to get SPI master %u for "
> +				"device %s/%u\n", __func__,
> +				info->info.spi->bus_num,
> +				info->info.spi->modalias,
> +				info->info.spi->chip_select);
> +			return NULL;
> +		}
> +
> +		subdev = v4l2_spi_new_subdev(v4l2_dev, master, info->info.spi);
> +		spi_master_put(master);
> +		break;
> +	}
> +#endif /* defined(CONFIG_SPI) */
> +	default:
> +		subdev = NULL;
> +		break;
> +	}
> +
> +	return subdev;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_new_subdev_board);

I'm just wondering, while we are at it, if it would be worth to try to make
v4l2_i2c_new_subdev_board() and v4l2_spi_new_subdev() race-free. There has
been an attempt from Guennadi side to solve this issue,
http://thread.gmane.org/gmane.linux.kernel/1069603
After request_module there is nothing preventing subdev's driver module to be
unloaded and thus it is not safe to dereference dev->driver->owner.

> +
>  /* Clamp x to be between min and max, aligned to a multiple of 2^align.  min
>   * and max don't have to be aligned, but there must be at least one valid
>   * value.  E.g., min=17,max=31,align=4 is not allowed as there are no multiples
> diff --git a/drivers/media/video/v4l2-device.c b/drivers/media/video/v4l2-device.c
> index 4aae501..cfd9caf 100644
> --- a/drivers/media/video/v4l2-device.c
> +++ b/drivers/media/video/v4l2-device.c
> @@ -246,5 +246,13 @@ void v4l2_device_unregister_subdev(struct v4l2_subdev *sd)
>  #endif
>  	video_unregister_device(&sd->devnode);
>  	module_put(sd->owner);
> +
> +#if defined(CONFIG_I2C) || (defined(CONFIG_I2C_MODULE) && defined(MODULE))
> +	if ((sd->flags & V4L2_SUBDEV_FL_IS_I2C) &&
> +	    (sd->flags & V4L2_SUBDEV_FL_RELEASE_ADAPTER)) {
> +		struct i2c_client *client = v4l2_get_subdevdata(sd);
> +		i2c_put_adapter(client->adapter);
> +	}
> +#endif
>  }
>  EXPORT_SYMBOL_GPL(v4l2_device_unregister_subdev);
> diff --git a/include/media/v4l2-common.h b/include/media/v4l2-common.h
> index a298ec4..88c38d9 100644
> --- a/include/media/v4l2-common.h
> +++ b/include/media/v4l2-common.h
> @@ -171,6 +171,34 @@ void v4l2_spi_subdev_init(struct v4l2_subdev *sd, struct spi_device *spi,
>  		const struct v4l2_subdev_ops *ops);
>  #endif
>  
> +/* -------------------------------------------------------------------------- */
> +
> +/* Generic helper functions */
> +
> +struct v4l2_subdev_i2c_board_info {
> +	struct i2c_board_info *board_info;
> +	int i2c_adapter_id;
> +};
> +
> +enum v4l2_subdev_bus_type {
> +	V4L2_SUBDEV_BUS_TYPE_NONE,
> +	V4L2_SUBDEV_BUS_TYPE_I2C,
> +	V4L2_SUBDEV_BUS_TYPE_SPI,
> +};

I had an issue when tried to call request_module, to register subdev of platform
device type, in probe() of other platform device. Driver's probe() for devices
belonging same bus type cannot be nested as the bus lock is taken by the driver core
before entering probe(), so this would lead to a deadlock.
That exactly happens in __driver_attach().

For the same reason v4l2_new_subdev_board could not be called from probe()
of devices belonging to I2C or SPI bus, as request_module is called inside of it.
I'm not sure how to solve it, yet:)

> +
> +struct v4l2_subdev_board_info {
> +	enum v4l2_subdev_bus_type type;
> +	union {
> +		struct v4l2_subdev_i2c_board_info i2c;
> +		struct spi_board_info *spi;
> +	} info;
> +};
> +
> +/* Create a subdevice and load its module. The info argumentidentifies the
> + * subdev bus type and the bus type-specific information. */
> +struct v4l2_subdev *v4l2_new_subdev_board(struct v4l2_device *v4l2_dev,
> +		struct v4l2_subdev_board_info *info);
> +
>  /* ------------------------------------------------------------------------- */
>  
>  /* Note: these remaining ioctls/structs should be removed as well, but they are
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index 1562c4f..bc1c4d8 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -483,6 +483,9 @@ struct v4l2_subdev_internal_ops {
>  #define V4L2_SUBDEV_FL_HAS_DEVNODE		(1U << 2)
>  /* Set this flag if this subdev generates events. */
>  #define V4L2_SUBDEV_FL_HAS_EVENTS		(1U << 3)
> +/* Set by the core if the bus adapter needs to be released. Do NOT use in
> + * drivers. */
> +#define V4L2_SUBDEV_FL_RELEASE_ADAPTER		(1U << 4)
>  
>  /* Each instance of a subdev driver should create this struct, either
>     stand-alone or embedded in a larger struct.

Regards,
-- 
Sylwester Nawrocki
Samsung Poland R&D Center

Re: [RFC/PATCH 1/2] v4l: Add generic board subdev registration function

[Laurent Pinchart]

Hi Sylwester,

On Friday 20 May 2011 09:14:36 Sylwester Nawrocki wrote:
> On 05/19/2011 08:34 PM, Laurent Pinchart wrote:
> > The new v4l2_new_subdev_board() function creates and register a subdev
> > based on generic board information. The board information structure
> > includes a bus type and bus type-specific information.
> > 
> > Only I2C and SPI busses are currently supported.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > 
> >  drivers/media/video/v4l2-common.c |   70
> >  +++++++++++++++++++++++++++++++++++++ drivers/media/video/v4l2-device.c
> >  |    8 ++++
> >  include/media/v4l2-common.h       |   28 +++++++++++++++
> >  include/media/v4l2-subdev.h       |    3 ++
> >  4 files changed, 109 insertions(+), 0 deletions(-)
> > 
> > Hi everybody,
> > 
> > This approach has been briefly discussed during the Warsaw V4L meeting.
> > Now that support for platform subdevs has been requested, I'd like to
> > move bus type handling to the V4L2 core instead of duplicating the logic
> > in every driver. As usual, comments will be appreciated.
> 
> Thanks for taking care of this.

You're welcome. Thanks for the review.

> > diff --git a/drivers/media/video/v4l2-common.c
> > b/drivers/media/video/v4l2-common.c index 06b9f9f..46aee94 100644
> > --- a/drivers/media/video/v4l2-common.c
> > +++ b/drivers/media/video/v4l2-common.c
> > @@ -474,6 +474,76 @@ EXPORT_SYMBOL_GPL(v4l2_spi_new_subdev);
> > 
> >  #endif /* defined(CONFIG_SPI) */
> > 
> > +/*
> > + * v4l2_new_subdev_board - Register a subdevice based on board
> > information + * @v4l2_dev: Parent V4L2 device
> > + * @info: I2C subdevs board information array
> 
> "info" doesn't appear to be a (I2C subdevs) array.

Oops, I'll fix it.

> > + *
> > + * Register a subdevice identified by a geenric board information
> > structure. The
> 
> s/geenric/generic ?

Ditto.

> > + * structure contains the bus type and bus type-specific information.
> > + *
> > + * Return a pointer to the subdevice if registration was successful, or
> > NULL + * otherwise.
> > + */
> > +struct v4l2_subdev *v4l2_new_subdev_board(struct v4l2_device *v4l2_dev,
> > +		struct v4l2_subdev_board_info *info)
> > +{
> > +	struct v4l2_subdev *subdev;
> > +
> > +	switch (info->type) {
> > +#if defined(CONFIG_I2C)
> > +	case V4L2_SUBDEV_BUS_TYPE_I2C: {
> > +		struct i2c_adapter *adapter;
> > +
> > +		adapter = i2c_get_adapter(info->info.i2c.i2c_adapter_id);
> > +		if (adapter == NULL) {
> > +			printk(KERN_ERR "%s: Unable to get I2C adapter %d for "
> > +				"device %s/%u\n", __func__,
> > +				info->info.i2c.i2c_adapter_id,
> > +				info->info.i2c.board_info->type,
> > +				info->info.i2c.board_info->addr);
> > +			return NULL;
> > +		}
> > +
> > +		subdev = v4l2_i2c_new_subdev_board(v4l2_dev, adapter,
> > +					info->info.i2c.board_info, NULL);
> > +		if (subdev == NULL) {
> > +			i2c_put_adapter(adapter);
> > +			return NULL;
> > +		}
> > +
> > +		subdev->flags |= V4L2_SUBDEV_FL_RELEASE_ADAPTER;
> > +		break;
> > +	}
> > +#endif /* defined(CONFIG_I2C) */
> > +#if defined(CONFIG_SPI)
> > +	case V4L2_SUBDEV_BUS_TYPE_SPI: {
> > +		struct spi_master *master;
> > +
> > +		master = spi_busnum_to_master(info->info.spi->bus_num);
> > +		if (master == NULL) {
> > +			printk(KERN_ERR "%s: Unable to get SPI master %u for "
> > +				"device %s/%u\n", __func__,
> > +				info->info.spi->bus_num,
> > +				info->info.spi->modalias,
> > +				info->info.spi->chip_select);
> > +			return NULL;
> > +		}
> > +
> > +		subdev = v4l2_spi_new_subdev(v4l2_dev, master, info->info.spi);
> > +		spi_master_put(master);
> > +		break;
> > +	}
> > +#endif /* defined(CONFIG_SPI) */
> > +	default:
> > +		subdev = NULL;
> > +		break;
> > +	}
> > +
> > +	return subdev;
> > +}
> > +EXPORT_SYMBOL_GPL(v4l2_new_subdev_board);
> 
> I'm just wondering, while we are at it, if it would be worth to try to make
> v4l2_i2c_new_subdev_board() and v4l2_spi_new_subdev() race-free. There has
> been an attempt from Guennadi side to solve this issue,
> http://thread.gmane.org/gmane.linux.kernel/1069603
> After request_module there is nothing preventing subdev's driver module to
> be unloaded and thus it is not safe to dereference dev->driver->owner.

Please see below.

> > +
> > 
> >  /* Clamp x to be between min and max, aligned to a multiple of 2^align. 
> >  min
> >  
> >   * and max don't have to be aligned, but there must be at least one
> >   valid * value.  E.g., min=17,max=31,align=4 is not allowed as there
> >   are no multiples
> > 
> > diff --git a/drivers/media/video/v4l2-device.c
> > b/drivers/media/video/v4l2-device.c index 4aae501..cfd9caf 100644
> > --- a/drivers/media/video/v4l2-device.c
> > +++ b/drivers/media/video/v4l2-device.c
> > @@ -246,5 +246,13 @@ void v4l2_device_unregister_subdev(struct
> > v4l2_subdev *sd)
> > 
> >  #endif
> >  
> >  	video_unregister_device(&sd->devnode);
> >  	module_put(sd->owner);
> > 
> > +
> > +#if defined(CONFIG_I2C) || (defined(CONFIG_I2C_MODULE) &&
> > defined(MODULE)) +	if ((sd->flags & V4L2_SUBDEV_FL_IS_I2C) &&
> > +	    (sd->flags & V4L2_SUBDEV_FL_RELEASE_ADAPTER)) {
> > +		struct i2c_client *client = v4l2_get_subdevdata(sd);
> > +		i2c_put_adapter(client->adapter);
> > +	}
> > +#endif
> > 
> >  }
> >  EXPORT_SYMBOL_GPL(v4l2_device_unregister_subdev);
> > 
> > diff --git a/include/media/v4l2-common.h b/include/media/v4l2-common.h
> > index a298ec4..88c38d9 100644
> > --- a/include/media/v4l2-common.h
> > +++ b/include/media/v4l2-common.h
> > @@ -171,6 +171,34 @@ void v4l2_spi_subdev_init(struct v4l2_subdev *sd,
> > struct spi_device *spi,
> > 
> >  		const struct v4l2_subdev_ops *ops);
> >  
> >  #endif
> > 
> > +/*
> > ------------------------------------------------------------------------
> > -- */ +
> > +/* Generic helper functions */
> > +
> > +struct v4l2_subdev_i2c_board_info {
> > +	struct i2c_board_info *board_info;
> > +	int i2c_adapter_id;
> > +};
> > +
> > +enum v4l2_subdev_bus_type {
> > +	V4L2_SUBDEV_BUS_TYPE_NONE,
> > +	V4L2_SUBDEV_BUS_TYPE_I2C,
> > +	V4L2_SUBDEV_BUS_TYPE_SPI,
> > +};
> 
> I had an issue when tried to call request_module, to register subdev of
> platform device type, in probe() of other platform device. Driver's
> probe() for devices belonging same bus type cannot be nested as the bus
> lock is taken by the driver core before entering probe(), so this would
> lead to a deadlock.
> That exactly happens in __driver_attach().
> 
> For the same reason v4l2_new_subdev_board could not be called from probe()
> of devices belonging to I2C or SPI bus, as request_module is called inside
> of it. I'm not sure how to solve it, yet:)

Ouch. I wasn't aware of that issue. Looks like it's indeed time to fix the 
subdev registration issue, including the module load race condition. Michael, 
you said you have a patch to add platform subdev support, how have you avoided 
the race condition ?

I've been thinking for some time now about removing the module load code 
completely. I2C, SPI and platform subdevs would be registered either by board 
code (possibly through the device tree on platforms that suppport it) for 
embedded platforms, and by host drivers for pluggable hardware (PCI and USB). 
Module loading would be handled automatically by the kernel module auto 
loader, but asynchronously instead of synchronously. Bus notifiers would then 
be used by host drivers to wait for all subdevs to be registered.

I'm not sure yet if this approach is viable. Hans, I think we've briefly 
discussed this (possible quite a long time ago), do you have any opinion ? 
Guennadi, based on your previous experience trying to use bus notifiers to 
solve the module load race, what do you think about the idea ? Others, please 
comment as well :-)
 
> > +
> > +struct v4l2_subdev_board_info {
> > +	enum v4l2_subdev_bus_type type;
> > +	union {
> > +		struct v4l2_subdev_i2c_board_info i2c;
> > +		struct spi_board_info *spi;
> > +	} info;
> > +};
> > +
> > +/* Create a subdevice and load its module. The info argumentidentifies
> > the + * subdev bus type and the bus type-specific information. */
> > +struct v4l2_subdev *v4l2_new_subdev_board(struct v4l2_device *v4l2_dev,
> > +		struct v4l2_subdev_board_info *info);
> > +
> > 
> >  /*
> >  -----------------------------------------------------------------------
> >  -- */
> >  
> >  /* Note: these remaining ioctls/structs should be removed as well, but
> >  they are
> > 
> > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> > index 1562c4f..bc1c4d8 100644
> > --- a/include/media/v4l2-subdev.h
> > +++ b/include/media/v4l2-subdev.h
> > @@ -483,6 +483,9 @@ struct v4l2_subdev_internal_ops {
> > 
> >  #define V4L2_SUBDEV_FL_HAS_DEVNODE		(1U << 2)
> >  /* Set this flag if this subdev generates events. */
> >  #define V4L2_SUBDEV_FL_HAS_EVENTS		(1U << 3)
> > 
> > +/* Set by the core if the bus adapter needs to be released. Do NOT use
> > in + * drivers. */
> > +#define V4L2_SUBDEV_FL_RELEASE_ADAPTER		(1U << 4)
> > 
> >  /* Each instance of a subdev driver should create this struct, either
> >  
> >     stand-alone or embedded in a larger struct.
-
-- 
Regards,

Laurent Pinchart

Re: [RFC/PATCH 1/2] v4l: Add generic board subdev registration function

[Michael Jones]

On 05/20/2011 09:29 AM, Laurent Pinchart wrote:

[snip]

>> I had an issue when tried to call request_module, to register subdev of
>> platform device type, in probe() of other platform device. Driver's
>> probe() for devices belonging same bus type cannot be nested as the bus
>> lock is taken by the driver core before entering probe(), so this would
>> lead to a deadlock.
>> That exactly happens in __driver_attach().
>>
>> For the same reason v4l2_new_subdev_board could not be called from probe()
>> of devices belonging to I2C or SPI bus, as request_module is called inside
>> of it. I'm not sure how to solve it, yet:)
> 
> Ouch. I wasn't aware of that issue. Looks like it's indeed time to fix the 
> subdev registration issue, including the module load race condition. Michael, 
> you said you have a patch to add platform subdev support, how have you avoided 
> the race condition ?

I spoke too soon. This deadlock is staring me in the face right now,
too.  Ouch, indeed.

[snip]


MATRIX VISION GmbH, Talstrasse 16, DE-71570 Oppenweiler
Registergericht: Amtsgericht Stuttgart, HRB 271090
Geschaeftsfuehrer: Gerhard Thullner, Werner Armingeon, Uwe Furtner

Re: [RFC/PATCH 1/2] v4l: Add generic board subdev registration function

[Hans Verkuil]

On Friday, May 20, 2011 09:29:32 Laurent Pinchart wrote:
> Hi Sylwester,
> 
> On Friday 20 May 2011 09:14:36 Sylwester Nawrocki wrote:
> > On 05/19/2011 08:34 PM, Laurent Pinchart wrote:
> > > The new v4l2_new_subdev_board() function creates and register a subdev
> > > based on generic board information. The board information structure
> > > includes a bus type and bus type-specific information.
> > > 
> > > Only I2C and SPI busses are currently supported.
> > > 
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > ---
> > > 
> > >  drivers/media/video/v4l2-common.c |   70
> > >  +++++++++++++++++++++++++++++++++++++ drivers/media/video/v4l2-device.c
> > >  |    8 ++++
> > >  include/media/v4l2-common.h       |   28 +++++++++++++++
> > >  include/media/v4l2-subdev.h       |    3 ++
> > >  4 files changed, 109 insertions(+), 0 deletions(-)
> > > 
> > > Hi everybody,
> > > 
> > > This approach has been briefly discussed during the Warsaw V4L meeting.
> > > Now that support for platform subdevs has been requested, I'd like to
> > > move bus type handling to the V4L2 core instead of duplicating the logic
> > > in every driver. As usual, comments will be appreciated.
> > 
> > Thanks for taking care of this.
> 
> You're welcome. Thanks for the review.
> 
> > > diff --git a/drivers/media/video/v4l2-common.c
> > > b/drivers/media/video/v4l2-common.c index 06b9f9f..46aee94 100644
> > > --- a/drivers/media/video/v4l2-common.c
> > > +++ b/drivers/media/video/v4l2-common.c
> > > @@ -474,6 +474,76 @@ EXPORT_SYMBOL_GPL(v4l2_spi_new_subdev);
> > > 
> > >  #endif /* defined(CONFIG_SPI) */
> > > 
> > > +/*
> > > + * v4l2_new_subdev_board - Register a subdevice based on board
> > > information + * @v4l2_dev: Parent V4L2 device
> > > + * @info: I2C subdevs board information array
> > 
> > "info" doesn't appear to be a (I2C subdevs) array.
> 
> Oops, I'll fix it.
> 
> > > + *
> > > + * Register a subdevice identified by a geenric board information
> > > structure. The
> > 
> > s/geenric/generic ?
> 
> Ditto.
> 
> > > + * structure contains the bus type and bus type-specific information.
> > > + *
> > > + * Return a pointer to the subdevice if registration was successful, or
> > > NULL + * otherwise.
> > > + */
> > > +struct v4l2_subdev *v4l2_new_subdev_board(struct v4l2_device *v4l2_dev,
> > > +		struct v4l2_subdev_board_info *info)
> > > +{
> > > +	struct v4l2_subdev *subdev;
> > > +
> > > +	switch (info->type) {
> > > +#if defined(CONFIG_I2C)
> > > +	case V4L2_SUBDEV_BUS_TYPE_I2C: {
> > > +		struct i2c_adapter *adapter;
> > > +
> > > +		adapter = i2c_get_adapter(info->info.i2c.i2c_adapter_id);
> > > +		if (adapter == NULL) {
> > > +			printk(KERN_ERR "%s: Unable to get I2C adapter %d for "
> > > +				"device %s/%u\n", __func__,
> > > +				info->info.i2c.i2c_adapter_id,
> > > +				info->info.i2c.board_info->type,
> > > +				info->info.i2c.board_info->addr);
> > > +			return NULL;
> > > +		}
> > > +
> > > +		subdev = v4l2_i2c_new_subdev_board(v4l2_dev, adapter,
> > > +					info->info.i2c.board_info, NULL);
> > > +		if (subdev == NULL) {
> > > +			i2c_put_adapter(adapter);
> > > +			return NULL;
> > > +		}
> > > +
> > > +		subdev->flags |= V4L2_SUBDEV_FL_RELEASE_ADAPTER;
> > > +		break;
> > > +	}
> > > +#endif /* defined(CONFIG_I2C) */
> > > +#if defined(CONFIG_SPI)
> > > +	case V4L2_SUBDEV_BUS_TYPE_SPI: {
> > > +		struct spi_master *master;
> > > +
> > > +		master = spi_busnum_to_master(info->info.spi->bus_num);
> > > +		if (master == NULL) {
> > > +			printk(KERN_ERR "%s: Unable to get SPI master %u for "
> > > +				"device %s/%u\n", __func__,
> > > +				info->info.spi->bus_num,
> > > +				info->info.spi->modalias,
> > > +				info->info.spi->chip_select);
> > > +			return NULL;
> > > +		}
> > > +
> > > +		subdev = v4l2_spi_new_subdev(v4l2_dev, master, info->info.spi);
> > > +		spi_master_put(master);
> > > +		break;
> > > +	}
> > > +#endif /* defined(CONFIG_SPI) */
> > > +	default:
> > > +		subdev = NULL;
> > > +		break;
> > > +	}
> > > +
> > > +	return subdev;
> > > +}
> > > +EXPORT_SYMBOL_GPL(v4l2_new_subdev_board);
> > 
> > I'm just wondering, while we are at it, if it would be worth to try to 
make
> > v4l2_i2c_new_subdev_board() and v4l2_spi_new_subdev() race-free. There has
> > been an attempt from Guennadi side to solve this issue,
> > http://thread.gmane.org/gmane.linux.kernel/1069603
> > After request_module there is nothing preventing subdev's driver module to
> > be unloaded and thus it is not safe to dereference dev->driver->owner.
> 
> Please see below.
> 
> > > +
> > > 
> > >  /* Clamp x to be between min and max, aligned to a multiple of 2^align. 
> > >  min
> > >  
> > >   * and max don't have to be aligned, but there must be at least one
> > >   valid * value.  E.g., min=17,max=31,align=4 is not allowed as there
> > >   are no multiples
> > > 
> > > diff --git a/drivers/media/video/v4l2-device.c
> > > b/drivers/media/video/v4l2-device.c index 4aae501..cfd9caf 100644
> > > --- a/drivers/media/video/v4l2-device.c
> > > +++ b/drivers/media/video/v4l2-device.c
> > > @@ -246,5 +246,13 @@ void v4l2_device_unregister_subdev(struct
> > > v4l2_subdev *sd)
> > > 
> > >  #endif
> > >  
> > >  	video_unregister_device(&sd->devnode);
> > >  	module_put(sd->owner);
> > > 
> > > +
> > > +#if defined(CONFIG_I2C) || (defined(CONFIG_I2C_MODULE) &&
> > > defined(MODULE)) +	if ((sd->flags & V4L2_SUBDEV_FL_IS_I2C) &&
> > > +	    (sd->flags & V4L2_SUBDEV_FL_RELEASE_ADAPTER)) {
> > > +		struct i2c_client *client = v4l2_get_subdevdata(sd);
> > > +		i2c_put_adapter(client->adapter);
> > > +	}
> > > +#endif
> > > 
> > >  }
> > >  EXPORT_SYMBOL_GPL(v4l2_device_unregister_subdev);
> > > 
> > > diff --git a/include/media/v4l2-common.h b/include/media/v4l2-common.h
> > > index a298ec4..88c38d9 100644
> > > --- a/include/media/v4l2-common.h
> > > +++ b/include/media/v4l2-common.h
> > > @@ -171,6 +171,34 @@ void v4l2_spi_subdev_init(struct v4l2_subdev *sd,
> > > struct spi_device *spi,
> > > 
> > >  		const struct v4l2_subdev_ops *ops);
> > >  
> > >  #endif
> > > 
> > > +/*
> > > ------------------------------------------------------------------------
> > > -- */ +
> > > +/* Generic helper functions */
> > > +
> > > +struct v4l2_subdev_i2c_board_info {
> > > +	struct i2c_board_info *board_info;
> > > +	int i2c_adapter_id;
> > > +};
> > > +
> > > +enum v4l2_subdev_bus_type {
> > > +	V4L2_SUBDEV_BUS_TYPE_NONE,
> > > +	V4L2_SUBDEV_BUS_TYPE_I2C,
> > > +	V4L2_SUBDEV_BUS_TYPE_SPI,
> > > +};
> > 
> > I had an issue when tried to call request_module, to register subdev of
> > platform device type, in probe() of other platform device. Driver's
> > probe() for devices belonging same bus type cannot be nested as the bus
> > lock is taken by the driver core before entering probe(), so this would
> > lead to a deadlock.
> > That exactly happens in __driver_attach().
> > 
> > For the same reason v4l2_new_subdev_board could not be called from probe()
> > of devices belonging to I2C or SPI bus, as request_module is called inside
> > of it. I'm not sure how to solve it, yet:)
> 
> Ouch. I wasn't aware of that issue. Looks like it's indeed time to fix the 
> subdev registration issue, including the module load race condition. 
Michael, 
> you said you have a patch to add platform subdev support, how have you 
avoided 
> the race condition ?
> 
> I've been thinking for some time now about removing the module load code 
> completely. I2C, SPI and platform subdevs would be registered either by 
board 
> code (possibly through the device tree on platforms that suppport it) for 
> embedded platforms, and by host drivers for pluggable hardware (PCI and 
USB). 
> Module loading would be handled automatically by the kernel module auto 
> loader, but asynchronously instead of synchronously. Bus notifiers would 
then 
> be used by host drivers to wait for all subdevs to be registered.
> 
> I'm not sure yet if this approach is viable. Hans, I think we've briefly 
> discussed this (possible quite a long time ago), do you have any opinion ? 
> Guennadi, based on your previous experience trying to use bus notifiers to 
> solve the module load race, what do you think about the idea ? Others, 
please 
> comment as well :-)

It's definitely viable (I believe the required bus notification has been added 
some time ago), but I am not sure how to implement it in an efficient manner.

My initial idea would be to just wait in v4l2_new_subdev_board until you get 
the notification on the bus (with a timeout, of course). However, I suspect 
that that does not solve the deadlock, although it would solve the race.

As an aside: note that if the module is unloaded right after the 
request_module, then that will be detected by the code and it will just return 
an error. It won't oops or anything like that. Personally I don't believe it 
is worth the effort just to solve this race, since it is highly theoretical.

The problem of loading another bus module when in a bus probe function is a 
separate issue. My initial reaction is: why do you want to do this? Even if 
you use delayed module loads, you probably still have to wait for them to 
succeed at a higher-level function. For example: in the probe function of 
module A it will attempt to load module B. That probably can't succeed as long 
as you are in A's probe function due to the bus lock. So you can't check for a 
successful load of B until you return from that probe function and a higher-
level function (that likely loaded module A in the first place) does that 
check.

That's all pretty tricky code, and my suggestion would be to simply not do 
nested module loads from the same bus.

Regards,

      Hans

Re: [RFC/PATCH 1/2] v4l: Add generic board subdev registration function

[Laurent Pinchart]

Hi Hans,

On Friday 20 May 2011 10:53:32 Hans Verkuil wrote:
> On Friday, May 20, 2011 09:29:32 Laurent Pinchart wrote:
> > On Friday 20 May 2011 09:14:36 Sylwester Nawrocki wrote:
> > > On 05/19/2011 08:34 PM, Laurent Pinchart wrote:
> > > > The new v4l2_new_subdev_board() function creates and register a
> > > > subdev based on generic board information. The board information
> > > > structure includes a bus type and bus type-specific information.
> > > > 
> > > > Only I2C and SPI busses are currently supported.
> > > > 
> > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

[snip]

> > > I had an issue when tried to call request_module, to register subdev of
> > > platform device type, in probe() of other platform device. Driver's
> > > probe() for devices belonging same bus type cannot be nested as the bus
> > > lock is taken by the driver core before entering probe(), so this would
> > > lead to a deadlock.
> > > That exactly happens in __driver_attach().
> > > 
> > > For the same reason v4l2_new_subdev_board could not be called from
> > > probe() of devices belonging to I2C or SPI bus, as request_module is
> > > called inside of it. I'm not sure how to solve it, yet:)
> > 
> > Ouch. I wasn't aware of that issue. Looks like it's indeed time to fix
> > the subdev registration issue, including the module load race condition.
> > Michael, you said you have a patch to add platform subdev support, how
> > have you avoided the race condition ?
> > 
> > I've been thinking for some time now about removing the module load code
> > completely. I2C, SPI and platform subdevs would be registered either by
> > board code (possibly through the device tree on platforms that suppport
> > it) for embedded platforms, and by host drivers for pluggable hardware
> > (PCI and USB). Module loading would be handled automatically by the kernel
> > module auto loader, but asynchronously instead of synchronously. Bus
> > notifiers would then be used by host drivers to wait for all subdevs to be
> > registered.
> > 
> > I'm not sure yet if this approach is viable. Hans, I think we've briefly
> > discussed this (possible quite a long time ago), do you have any opinion
> > ? Guennadi, based on your previous experience trying to use bus notifiers
> > to solve the module load race, what do you think about the idea ? Others,
> > please comment as well :-)
> 
> It's definitely viable (I believe the required bus notification has been
> added some time ago), but I am not sure how to implement it in an
> efficient manner.
> 
> My initial idea would be to just wait in v4l2_new_subdev_board until you
> get the notification on the bus (with a timeout, of course). However, I
> suspect that that does not solve the deadlock, although it would solve the
> race.
> 
> As an aside: note that if the module is unloaded right after the
> request_module, then that will be detected by the code and it will just
> return an error. It won't oops or anything like that. Personally I don't
> believe it is worth the effort just to solve this race, since it is highly
> theoretical.
> 
> The problem of loading another bus module when in a bus probe function is a
> separate issue. My initial reaction is: why do you want to do this? Even if
> you use delayed module loads, you probably still have to wait for them to
> succeed at a higher-level function. For example: in the probe function of
> module A it will attempt to load module B. That probably can't succeed as
> long as you are in A's probe function due to the bus lock. So you can't
> check for a successful load of B until you return from that probe function
> and a higher- level function (that likely loaded module A in the first
> place) does that check.
> 
> That's all pretty tricky code, and my suggestion would be to simply not do
> nested module loads from the same bus.

That's unfortunately not an option. Most bridge/host devices in embedded 
systems are platform devices, and they will need to load platform subdevs. We 
need to fix that.

My idea was to use bus notifiers to delay the bridge/host device 
initialization. The bridge probe() function would pre-initialize the bridge 
and register notifiers. The driver would then wait until all subdevs are 
properly registered, and then proceed from to register V4L2 devices from the 
bus notifier callback (or possible a work queue). There would be no nested 
probe() calls.

-- 
Regards,

Laurent Pinchart

Re: [RFC/PATCH 1/2] v4l: Add generic board subdev registration function

[Hans Verkuil]

On Friday, May 20, 2011 11:05:00 Laurent Pinchart wrote:
> Hi Hans,
> 
> On Friday 20 May 2011 10:53:32 Hans Verkuil wrote:
> > On Friday, May 20, 2011 09:29:32 Laurent Pinchart wrote:
> > > On Friday 20 May 2011 09:14:36 Sylwester Nawrocki wrote:
> > > > On 05/19/2011 08:34 PM, Laurent Pinchart wrote:
> > > > > The new v4l2_new_subdev_board() function creates and register a
> > > > > subdev based on generic board information. The board information
> > > > > structure includes a bus type and bus type-specific information.
> > > > > 
> > > > > Only I2C and SPI busses are currently supported.
> > > > > 
> > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> [snip]
> 
> > > > I had an issue when tried to call request_module, to register subdev 
of
> > > > platform device type, in probe() of other platform device. Driver's
> > > > probe() for devices belonging same bus type cannot be nested as the 
bus
> > > > lock is taken by the driver core before entering probe(), so this 
would
> > > > lead to a deadlock.
> > > > That exactly happens in __driver_attach().
> > > > 
> > > > For the same reason v4l2_new_subdev_board could not be called from
> > > > probe() of devices belonging to I2C or SPI bus, as request_module is
> > > > called inside of it. I'm not sure how to solve it, yet:)
> > > 
> > > Ouch. I wasn't aware of that issue. Looks like it's indeed time to fix
> > > the subdev registration issue, including the module load race condition.
> > > Michael, you said you have a patch to add platform subdev support, how
> > > have you avoided the race condition ?
> > > 
> > > I've been thinking for some time now about removing the module load code
> > > completely. I2C, SPI and platform subdevs would be registered either by
> > > board code (possibly through the device tree on platforms that suppport
> > > it) for embedded platforms, and by host drivers for pluggable hardware
> > > (PCI and USB). Module loading would be handled automatically by the 
kernel
> > > module auto loader, but asynchronously instead of synchronously. Bus
> > > notifiers would then be used by host drivers to wait for all subdevs to 
be
> > > registered.
> > > 
> > > I'm not sure yet if this approach is viable. Hans, I think we've briefly
> > > discussed this (possible quite a long time ago), do you have any opinion
> > > ? Guennadi, based on your previous experience trying to use bus 
notifiers
> > > to solve the module load race, what do you think about the idea ? 
Others,
> > > please comment as well :-)
> > 
> > It's definitely viable (I believe the required bus notification has been
> > added some time ago), but I am not sure how to implement it in an
> > efficient manner.
> > 
> > My initial idea would be to just wait in v4l2_new_subdev_board until you
> > get the notification on the bus (with a timeout, of course). However, I
> > suspect that that does not solve the deadlock, although it would solve the
> > race.
> > 
> > As an aside: note that if the module is unloaded right after the
> > request_module, then that will be detected by the code and it will just
> > return an error. It won't oops or anything like that. Personally I don't
> > believe it is worth the effort just to solve this race, since it is highly
> > theoretical.
> > 
> > The problem of loading another bus module when in a bus probe function is 
a
> > separate issue. My initial reaction is: why do you want to do this? Even 
if
> > you use delayed module loads, you probably still have to wait for them to
> > succeed at a higher-level function. For example: in the probe function of
> > module A it will attempt to load module B. That probably can't succeed as
> > long as you are in A's probe function due to the bus lock. So you can't
> > check for a successful load of B until you return from that probe function
> > and a higher- level function (that likely loaded module A in the first
> > place) does that check.
> > 
> > That's all pretty tricky code, and my suggestion would be to simply not do
> > nested module loads from the same bus.
> 
> That's unfortunately not an option. Most bridge/host devices in embedded 
> systems are platform devices, and they will need to load platform subdevs. 
We 
> need to fix that.

Good point.

> My idea was to use bus notifiers to delay the bridge/host device 
> initialization. The bridge probe() function would pre-initialize the bridge 
> and register notifiers. The driver would then wait until all subdevs are 
> properly registered, and then proceed from to register V4L2 devices from the 
> bus notifier callback (or possible a work queue). There would be no nested 
> probe() calls.

Would it be an option to create a new platform bus for the subdevs? That would 
have its own lock. It makes sense from a hierarchical point of view, but I'm 
not certain about the amount of work involved.

Regards,

	Hans

Re: [RFC/PATCH 1/2] v4l: Add generic board subdev registration function

[Laurent Pinchart]

Hi Hans,

On Friday 20 May 2011 11:19:48 Hans Verkuil wrote:
> On Friday, May 20, 2011 11:05:00 Laurent Pinchart wrote:
> > On Friday 20 May 2011 10:53:32 Hans Verkuil wrote:
> > > On Friday, May 20, 2011 09:29:32 Laurent Pinchart wrote:
> > > > On Friday 20 May 2011 09:14:36 Sylwester Nawrocki wrote:
> > > > > On 05/19/2011 08:34 PM, Laurent Pinchart wrote:
> > > > > > The new v4l2_new_subdev_board() function creates and register a
> > > > > > subdev based on generic board information. The board information
> > > > > > structure includes a bus type and bus type-specific information.
> > > > > > 
> > > > > > Only I2C and SPI busses are currently supported.
> > > > > > 
> > > > > > Signed-off-by: Laurent Pinchart
> > > > > > <laurent.pinchart@ideasonboard.com>
> > 
> > [snip]
> > 
> > > > > I had an issue when tried to call request_module, to register subdev
> > > > > of platform device type, in probe() of other platform device.
> > > > > Driver's probe() for devices belonging same bus type cannot be
> > > > > nested as the bus lock is taken by the driver core before entering
> > > > > probe(), so this would lead to a deadlock.
> > > > > That exactly happens in __driver_attach().
> > > > > 
> > > > > For the same reason v4l2_new_subdev_board could not be called from
> > > > > probe() of devices belonging to I2C or SPI bus, as request_module
> > > > > is called inside of it. I'm not sure how to solve it, yet:)
> > > > 
> > > > Ouch. I wasn't aware of that issue. Looks like it's indeed time to
> > > > fix the subdev registration issue, including the module load race
> > > > condition. Michael, you said you have a patch to add platform subdev
> > > > support, how have you avoided the race condition ?
> > > > 
> > > > I've been thinking for some time now about removing the module load
> > > > code completely. I2C, SPI and platform subdevs would be registered
> > > > either by board code (possibly through the device tree on platforms
> > > > that suppport it) for embedded platforms, and by host drivers for
> > > > pluggable hardware (PCI and USB). Module loading would be handled
> > > > automatically by the kernel module auto loader, but asynchronously
> > > > instead of synchronously. Bus notifiers would then be used by host
> > > > drivers to wait for all subdevs to be registered.
> > > > 
> > > > I'm not sure yet if this approach is viable. Hans, I think we've
> > > > briefly discussed this (possible quite a long time ago), do you have
> > > > any opinion ? Guennadi, based on your previous experience trying to
> > > > use bus notifiers to solve the module load race, what do you think
> > > > about the idea ? Others, please comment as well :-)
> > > 
> > > It's definitely viable (I believe the required bus notification has
> > > been added some time ago), but I am not sure how to implement it in an
> > > efficient manner.
> > > 
> > > My initial idea would be to just wait in v4l2_new_subdev_board until
> > > you get the notification on the bus (with a timeout, of course).
> > > However, I suspect that that does not solve the deadlock, although it
> > > would solve the race.
> > > 
> > > As an aside: note that if the module is unloaded right after the
> > > request_module, then that will be detected by the code and it will just
> > > return an error. It won't oops or anything like that. Personally I
> > > don't believe it is worth the effort just to solve this race, since it
> > > is highly theoretical.
> > > 
> > > The problem of loading another bus module when in a bus probe function
> > > is a separate issue. My initial reaction is: why do you want to do this?
> > > Even if you use delayed module loads, you probably still have to wait
> > > for them to succeed at a higher-level function. For example: in the
> > > probe function of module A it will attempt to load module B. That
> > > probably can't succeed as long as you are in A's probe function due to
> > > the bus lock. So you can't check for a successful load of B until you
> > > return from that probe function and a higher- level function (that
> > > likely loaded module A in the first place) does that check.
> > > 
> > > That's all pretty tricky code, and my suggestion would be to simply not
> > > do nested module loads from the same bus.
> > 
> > That's unfortunately not an option. Most bridge/host devices in embedded
> > systems are platform devices, and they will need to load platform
> > subdevs. We need to fix that.
> 
> Good point.
> 
> > My idea was to use bus notifiers to delay the bridge/host device
> > initialization. The bridge probe() function would pre-initialize the
> > bridge and register notifiers. The driver would then wait until all
> > subdevs are properly registered, and then proceed from to register V4L2
> > devices from the bus notifier callback (or possible a work queue). There
> > would be no nested probe() calls.
> 
> Would it be an option to create a new platform bus for the subdevs? That
> would have its own lock. It makes sense from a hierarchical point of view,
> but I'm not certain about the amount of work involved.

Do you mean a subdev-platform bus for platform subdevs, or a V4L2 subdev bus 
for all subdevs ? The first option is possible, but it looks more like a hack 
to me. If the subdev really is a platform device, it should be handled by the 
platform bus.

I don't think the second option is possible, I2C and SPI subdevs need to sit 
on an I2C or SPI bus (I could be mistaken though, there's at least one example 
of a logical bus type in the kernel with the HID bus).

Let's also not forget about sub-sub-devices. We need to handle them at some 
point as well.

This being said, I think that the use of platform devices to solve the initial 
problem can also be considered a hack as well. What we really need is a way to 
handle subdevs that can't be controlled at all (a video source that 
continuously delivers data for instance), or that can be controlled through 
GPIO. What bus should we use for a bus-less subdev ? And for GPIO-based 
subdevs, should we create a GPIO bus ?

-- 
Regards,

Laurent Pinchart

Re: [RFC/PATCH 1/2] v4l: Add generic board subdev registration function

[Hans Verkuil]

On Friday, May 20, 2011 11:37:24 Laurent Pinchart wrote:
> Hi Hans,
> 
> On Friday 20 May 2011 11:19:48 Hans Verkuil wrote:
> > On Friday, May 20, 2011 11:05:00 Laurent Pinchart wrote:
> > > On Friday 20 May 2011 10:53:32 Hans Verkuil wrote:
> > > > On Friday, May 20, 2011 09:29:32 Laurent Pinchart wrote:
> > > > > On Friday 20 May 2011 09:14:36 Sylwester Nawrocki wrote:
> > > > > > On 05/19/2011 08:34 PM, Laurent Pinchart wrote:
> > > > > > > The new v4l2_new_subdev_board() function creates and register a
> > > > > > > subdev based on generic board information. The board information
> > > > > > > structure includes a bus type and bus type-specific information.
> > > > > > > 
> > > > > > > Only I2C and SPI busses are currently supported.
> > > > > > > 
> > > > > > > Signed-off-by: Laurent Pinchart
> > > > > > > <laurent.pinchart@ideasonboard.com>
> > > 
> > > [snip]
> > > 
> > > > > > I had an issue when tried to call request_module, to register 
subdev
> > > > > > of platform device type, in probe() of other platform device.
> > > > > > Driver's probe() for devices belonging same bus type cannot be
> > > > > > nested as the bus lock is taken by the driver core before entering
> > > > > > probe(), so this would lead to a deadlock.
> > > > > > That exactly happens in __driver_attach().
> > > > > > 
> > > > > > For the same reason v4l2_new_subdev_board could not be called from
> > > > > > probe() of devices belonging to I2C or SPI bus, as request_module
> > > > > > is called inside of it. I'm not sure how to solve it, yet:)
> > > > > 
> > > > > Ouch. I wasn't aware of that issue. Looks like it's indeed time to
> > > > > fix the subdev registration issue, including the module load race
> > > > > condition. Michael, you said you have a patch to add platform subdev
> > > > > support, how have you avoided the race condition ?
> > > > > 
> > > > > I've been thinking for some time now about removing the module load
> > > > > code completely. I2C, SPI and platform subdevs would be registered
> > > > > either by board code (possibly through the device tree on platforms
> > > > > that suppport it) for embedded platforms, and by host drivers for
> > > > > pluggable hardware (PCI and USB). Module loading would be handled
> > > > > automatically by the kernel module auto loader, but asynchronously
> > > > > instead of synchronously. Bus notifiers would then be used by host
> > > > > drivers to wait for all subdevs to be registered.
> > > > > 
> > > > > I'm not sure yet if this approach is viable. Hans, I think we've
> > > > > briefly discussed this (possible quite a long time ago), do you have
> > > > > any opinion ? Guennadi, based on your previous experience trying to
> > > > > use bus notifiers to solve the module load race, what do you think
> > > > > about the idea ? Others, please comment as well :-)
> > > > 
> > > > It's definitely viable (I believe the required bus notification has
> > > > been added some time ago), but I am not sure how to implement it in an
> > > > efficient manner.
> > > > 
> > > > My initial idea would be to just wait in v4l2_new_subdev_board until
> > > > you get the notification on the bus (with a timeout, of course).
> > > > However, I suspect that that does not solve the deadlock, although it
> > > > would solve the race.
> > > > 
> > > > As an aside: note that if the module is unloaded right after the
> > > > request_module, then that will be detected by the code and it will 
just
> > > > return an error. It won't oops or anything like that. Personally I
> > > > don't believe it is worth the effort just to solve this race, since it
> > > > is highly theoretical.
> > > > 
> > > > The problem of loading another bus module when in a bus probe function
> > > > is a separate issue. My initial reaction is: why do you want to do 
this?
> > > > Even if you use delayed module loads, you probably still have to wait
> > > > for them to succeed at a higher-level function. For example: in the
> > > > probe function of module A it will attempt to load module B. That
> > > > probably can't succeed as long as you are in A's probe function due to
> > > > the bus lock. So you can't check for a successful load of B until you
> > > > return from that probe function and a higher- level function (that
> > > > likely loaded module A in the first place) does that check.
> > > > 
> > > > That's all pretty tricky code, and my suggestion would be to simply 
not
> > > > do nested module loads from the same bus.
> > > 
> > > That's unfortunately not an option. Most bridge/host devices in embedded
> > > systems are platform devices, and they will need to load platform
> > > subdevs. We need to fix that.
> > 
> > Good point.
> > 
> > > My idea was to use bus notifiers to delay the bridge/host device
> > > initialization. The bridge probe() function would pre-initialize the
> > > bridge and register notifiers. The driver would then wait until all
> > > subdevs are properly registered, and then proceed from to register V4L2
> > > devices from the bus notifier callback (or possible a work queue). There
> > > would be no nested probe() calls.
> > 
> > Would it be an option to create a new platform bus for the subdevs? That
> > would have its own lock. It makes sense from a hierarchical point of view,
> > but I'm not certain about the amount of work involved.
> 
> Do you mean a subdev-platform bus for platform subdevs, or a V4L2 subdev bus 
> for all subdevs ? The first option is possible, but it looks more like a 
hack 
> to me. If the subdev really is a platform device, it should be handled by 
the 
> platform bus.

The first. So you have a 'top-level' platform device that wants to load 
platform subdevs (probably representing internal IP blocks). So it would 
create its own platform bus that is used to probe those platform subdevs.

It is similar to e.g. an I2C device that has internal I2C busses: you would
also create nested busses there.

BTW, why do these platform subdevs have to be on the platform bus? Why not 
have standalone subdev drivers that are not on any bus? That's for example 
what ivtv does for the internal GPIO audio subdev.

> I don't think the second option is possible, I2C and SPI subdevs need to sit 
> on an I2C or SPI bus (I could be mistaken though, there's at least one 
example 
> of a logical bus type in the kernel with the HID bus).
> 
> Let's also not forget about sub-sub-devices. We need to handle them at some 
> point as well.

Sub-sub-devices are not a problem by themselves. They are only a problem if 
they on the same bus.

> This being said, I think that the use of platform devices to solve the 
initial 
> problem can also be considered a hack as well. What we really need is a way 
to 
> handle subdevs that can't be controlled at all (a video source that 
> continuously delivers data for instance), or that can be controlled through 
> GPIO. What bus should we use for a bus-less subdev ? And for GPIO-based 
> subdevs, should we create a GPIO bus ?

It is perfectly possible to have bus-less subdevs. See ivtv (I think there are 
one or two other examples as well).

Regards,

	Hans

Re: [RFC/PATCH 1/2] v4l: Add generic board subdev registration function

[Laurent Pinchart]

Hi Hans,

On Friday 20 May 2011 11:52:17 Hans Verkuil wrote:
> On Friday, May 20, 2011 11:37:24 Laurent Pinchart wrote:
> > On Friday 20 May 2011 11:19:48 Hans Verkuil wrote:
> > > On Friday, May 20, 2011 11:05:00 Laurent Pinchart wrote:

[snip]

> > > > My idea was to use bus notifiers to delay the bridge/host device
> > > > initialization. The bridge probe() function would pre-initialize the
> > > > bridge and register notifiers. The driver would then wait until all
> > > > subdevs are properly registered, and then proceed from to register
> > > > V4L2 devices from the bus notifier callback (or possible a work
> > > > queue). There would be no nested probe() calls.
> > > 
> > > Would it be an option to create a new platform bus for the subdevs?
> > > That would have its own lock. It makes sense from a hierarchical point
> > > of view, but I'm not certain about the amount of work involved.
> > 
> > Do you mean a subdev-platform bus for platform subdevs, or a V4L2 subdev
> > bus for all subdevs ? The first option is possible, but it looks more
> > like a hack to me. If the subdev really is a platform device, it should be
> > handled by the platform bus.
> 
> The first. So you have a 'top-level' platform device that wants to load
> platform subdevs (probably representing internal IP blocks). So it would
> create its own platform bus that is used to probe those platform subdevs.
> 
> It is similar to e.g. an I2C device that has internal I2C busses: you would
> also create nested busses there.
> 
> BTW, why do these platform subdevs have to be on the platform bus? Why not
> have standalone subdev drivers that are not on any bus? That's for example
> what ivtv does for the internal GPIO audio subdev.

There's some misunderstanging here. Internal IP blocks don't need to sit on 
any bus. The host/bridge driver can create subdevs for those blocks directly, 
as it doesn't need to load a separate driver.

The issue comes from external subdevs that offer little control or even none 
at all. The best example is an FPGA that will feed video data to the bridge in 
a fixed format without any mean of control, or with just an on/off control 
through a GPIO. Support for such subdevices need to be handled by a separate 
driver, so we need a way to load it at runtime. I'm not sure on what bus that 
driver should sit.

> > I don't think the second option is possible, I2C and SPI subdevs need to
> > sit on an I2C or SPI bus (I could be mistaken though, there's at least
> > one example of a logical bus type in the kernel with the HID bus).
> > 
> > Let's also not forget about sub-sub-devices. We need to handle them at
> > some point as well.
> 
> Sub-sub-devices are not a problem by themselves. They are only a problem if
> they on the same bus.
> 
> > This being said, I think that the use of platform devices to solve the
> > initial problem can also be considered a hack as well. What we really need
> > is a way to handle subdevs that can't be controlled at all (a video source
> > that continuously delivers data for instance), or that can be controlled
> > through GPIO. What bus should we use for a bus-less subdev ? And for
> > GPIO-based subdevs, should we create a GPIO bus ?
> 
> It is perfectly possible to have bus-less subdevs. See ivtv (I think there
> are one or two other examples as well).

But how can we handle bus-less subdevs for embedded devices, where the host 
(e.g. OMAP3 ISP) doesn't know about the external subdevs (e.g. FPGA controlled 
by a couple of GPIOs) ?

-- 
Regards,

Laurent Pinchart

Re: [RFC/PATCH 1/2] v4l: Add generic board subdev registration function

[Hans Verkuil]

On Friday, May 20, 2011 12:12:23 Laurent Pinchart wrote:
> Hi Hans,
> 
> On Friday 20 May 2011 11:52:17 Hans Verkuil wrote:
> > On Friday, May 20, 2011 11:37:24 Laurent Pinchart wrote:
> > > On Friday 20 May 2011 11:19:48 Hans Verkuil wrote:
> > > > On Friday, May 20, 2011 11:05:00 Laurent Pinchart wrote:
> 
> [snip]
> 
> > > > > My idea was to use bus notifiers to delay the bridge/host device
> > > > > initialization. The bridge probe() function would pre-initialize the
> > > > > bridge and register notifiers. The driver would then wait until all
> > > > > subdevs are properly registered, and then proceed from to register
> > > > > V4L2 devices from the bus notifier callback (or possible a work
> > > > > queue). There would be no nested probe() calls.
> > > > 
> > > > Would it be an option to create a new platform bus for the subdevs?
> > > > That would have its own lock. It makes sense from a hierarchical point
> > > > of view, but I'm not certain about the amount of work involved.
> > > 
> > > Do you mean a subdev-platform bus for platform subdevs, or a V4L2 subdev
> > > bus for all subdevs ? The first option is possible, but it looks more
> > > like a hack to me. If the subdev really is a platform device, it should be
> > > handled by the platform bus.
> > 
> > The first. So you have a 'top-level' platform device that wants to load
> > platform subdevs (probably representing internal IP blocks). So it would
> > create its own platform bus that is used to probe those platform subdevs.
> > 
> > It is similar to e.g. an I2C device that has internal I2C busses: you would
> > also create nested busses there.
> > 
> > BTW, why do these platform subdevs have to be on the platform bus? Why not
> > have standalone subdev drivers that are not on any bus? That's for example
> > what ivtv does for the internal GPIO audio subdev.
> 
> There's some misunderstanging here. Internal IP blocks don't need to sit on 
> any bus. The host/bridge driver can create subdevs for those blocks directly, 
> as it doesn't need to load a separate driver.
> 
> The issue comes from external subdevs that offer little control or even none 
> at all. The best example is an FPGA that will feed video data to the bridge in 
> a fixed format without any mean of control, or with just an on/off control 
> through a GPIO. Support for such subdevices need to be handled by a separate 
> driver, so we need a way to load it at runtime. I'm not sure on what bus that 
> driver should sit.
> 
> > > I don't think the second option is possible, I2C and SPI subdevs need to
> > > sit on an I2C or SPI bus (I could be mistaken though, there's at least
> > > one example of a logical bus type in the kernel with the HID bus).
> > > 
> > > Let's also not forget about sub-sub-devices. We need to handle them at
> > > some point as well.
> > 
> > Sub-sub-devices are not a problem by themselves. They are only a problem if
> > they on the same bus.
> > 
> > > This being said, I think that the use of platform devices to solve the
> > > initial problem can also be considered a hack as well. What we really need
> > > is a way to handle subdevs that can't be controlled at all (a video source
> > > that continuously delivers data for instance), or that can be controlled
> > > through GPIO. What bus should we use for a bus-less subdev ? And for
> > > GPIO-based subdevs, should we create a GPIO bus ?
> > 
> > It is perfectly possible to have bus-less subdevs. See ivtv (I think there
> > are one or two other examples as well).
> 
> But how can we handle bus-less subdevs for embedded devices, where the host 
> (e.g. OMAP3 ISP) doesn't know about the external subdevs (e.g. FPGA controlled 
> by a couple of GPIOs) ?
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> 

I remembered that we had to do something similar and I found the patch below.
This was the result of some private discussions with Vaibhav Hiremath from TI.

It almost certainly doesn't apply to the current kernel, but it is clear
what happens.

We are actually using this with the dm6467 capture driver.

Hope this might at least give some ideas.

Regards,

	Hans

From: Mats Randgaard <mats.randgaard@cisco.com>
Subject: [PATCH 6/6] FPGA subdevice driver
Date: Thu,  2 Dec 2010 08:40:10 +0100

This subdevice driver can be used with custom made subdevices (e.g. FPGA's) that
need some or no configuration. The driver calls functions specified in the board
definition file.

Signed-off-by: Mats Randgaard <mats.randgaard@cisco.com>
---
 arch/arm/mach-davinci/board-dm646x-evm.c   |   38 +++++++++++
 drivers/media/video/Kconfig                |    8 ++
 drivers/media/video/Makefile               |    1 +
 drivers/media/video/davinci/vpif_capture.c |   21 ++++--
 drivers/media/video/fpga_subdev.c          |   97 ++++++++++++++++++++++++++++
 include/media/fpga_subdev.h                |   44 +++++++++++++
 6 files changed, 203 insertions(+), 6 deletions(-)
 create mode 100644 drivers/media/video/fpga_subdev.c
 create mode 100644 include/media/fpga_subdev.h

diff --git a/arch/arm/mach-davinci/board-dm646x-evm.c b/arch/arm/mach-davinci/board-dm646x-evm.c
index f6ac9ba..458f593 100644
--- a/arch/arm/mach-davinci/board-dm646x-evm.c
+++ b/arch/arm/mach-davinci/board-dm646x-evm.c
@@ -25,6 +25,7 @@
 #include <linux/i2c/at24.h>
 #include <linux/i2c/pcf857x.h>
 
+#include <media/fpga_subdev.h>
 #include <media/tvp514x.h>
 
 #include <linux/mtd/mtd.h>
@@ -485,12 +486,14 @@ static int set_vpif_clock(int mux_mode, int hd)
 }
 
 static struct vpif_subdev_info dm646x_vpif_subdev[] = {
+#if 0
 	{
 		.name	= "adv7343",
 		.board_info = {
 			I2C_BOARD_INFO("adv7343", 0x2a),
 		},
 	},
+#endif
 	{
 		.name	= "ths7303",
 		.board_info = {
@@ -595,10 +598,43 @@ static struct tvp514x_platform_data tvp5146_pdata = {
 	.vs_polarity = 1
 };
 
+int dm646x_subdev_s_power(struct fpga_subdev_platform_data *pdata,
+		struct v4l2_subdev *sd, int on)
+{
+	printk(KERN_DEBUG "%s: Power %s\n", __func__, on ? "on" : "off");
+
+	return 0;
+}
+
+int dm646x_subdev_s_stream(struct fpga_subdev_platform_data *pdata,
+		struct v4l2_subdev *sd, int enable)
+{
+	printk(KERN_DEBUG "%s: Stream %s\n", __func__,
+			enable ? "enabled" : "disabled");
+
+	return 0;
+}
+
+static struct fpga_subdev_ops dm646x_subdev_ops = {
+	.s_stream = dm646x_subdev_s_stream,
+	.s_power = dm646x_subdev_s_power,
+};
+
+struct fpga_subdev_platform_data dm646x_subdev_pdata = {
+	.ops = &dm646x_subdev_ops,
+};
+
 #define TVP514X_STD_ALL (V4L2_STD_NTSC | V4L2_STD_PAL)
 
 static struct vpif_subdev_info vpif_capture_sdev_info[] = {
 	{
+		.name	= "fpga_subdev",
+		.board_info = {
+			.platform_data = &dm646x_subdev_pdata,
+		},
+	},
+#if 0
+	{
 		.name	= TVP5147_CH0,
 		.board_info = {
 			I2C_BOARD_INFO("tvp5146", 0x5d),
@@ -630,6 +666,7 @@ static struct vpif_subdev_info vpif_capture_sdev_info[] = {
 			.fid_pol = 0,
 		},
 	},
+#endif
 };
 
 static const struct vpif_input dm6467_ch0_inputs[] = {
@@ -669,6 +706,7 @@ static struct vpif_capture_config dm646x_vpif_capture_cfg = {
 		.inputs = dm6467_ch1_inputs,
 		.input_count = ARRAY_SIZE(dm6467_ch1_inputs),
 	},
+	.card_name = "DM646x EVM",
 };
 
 static void __init evm_init_video(void)
diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig
index f6e4d04..49335b0 100644
--- a/drivers/media/video/Kconfig
+++ b/drivers/media/video/Kconfig
@@ -114,6 +114,14 @@ config VIDEO_IR_I2C
 menu "Encoders/decoders and other helper chips"
 	depends on !VIDEO_HELPER_CHIPS_AUTO
 
+config VIDEO_FPGA_SUBDEV
+	tristate "Subdevice for custom made chips"
+	depends on VIDEO_V4L2
+	---help---
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called fpga-subdev.
+
 comment "Audio decoders"
 
 config VIDEO_TVAUDIO
diff --git a/drivers/media/video/Makefile b/drivers/media/video/Makefile
index 40f98fb..3ce4434 100644
--- a/drivers/media/video/Makefile
+++ b/drivers/media/video/Makefile
@@ -86,6 +86,7 @@ obj-$(CONFIG_SOC_CAMERA_TW9910)		+= tw9910.o
 
 # And now the v4l2 drivers:
 
+obj-$(CONFIG_VIDEO_FPGA_SUBDEV) += fpga_subdev.o
 obj-$(CONFIG_VIDEO_BT848) += bt8xx/
 obj-$(CONFIG_VIDEO_ZORAN) += zoran/
 obj-$(CONFIG_VIDEO_CQCAM) += c-qcam.o
diff --git a/drivers/media/video/davinci/vpif_capture.c b/drivers/media/video/davinci/vpif_capture.c
index 14b9adb..1305c66 100644
--- a/drivers/media/video/davinci/vpif_capture.c
+++ b/drivers/media/video/davinci/vpif_capture.c
@@ -38,6 +38,7 @@
 #include <media/v4l2-device.h>
 #include <media/v4l2-ioctl.h>
 #include <media/v4l2-chip-ident.h>
+#include <media/fpga_subdev.h>
 
 #include "vpif_capture.h"
 #include "vpif.h"
@@ -2319,12 +2320,20 @@ static __init int vpif_probe(struct platform_device *pdev)
 
 	for (i = 0; i < subdev_count; i++) {
 		subdevdata = &config->subdev_info[i];
-		vpif_obj.sd[i] =
-			v4l2_i2c_new_subdev_board(&vpif_obj.v4l2_dev,
-						  i2c_adap,
-						  subdevdata->name,
-						  &subdevdata->board_info,
-						  NULL);
+		if (subdevdata->board_info.addr) {
+			/* i2c device */
+			vpif_obj.sd[i] =
+				v4l2_i2c_new_subdev_board(&vpif_obj.v4l2_dev,
+						i2c_adap,
+						NULL,
+						&subdevdata->board_info,
+						NULL);
+		} else {
+			/* custom platform device */
+			vpif_obj.sd[i] =
+				fpga_subdev_init(&vpif_obj.v4l2_dev,
+						subdevdata->board_info.platform_data);
+		}
 
 		if (!vpif_obj.sd[i]) {
 			vpif_err("Error registering v4l2 subdevice\n");
diff --git a/drivers/media/video/davinci/vpif_display.c b/drivers/media/video/davinci/vpif_display.c
index 14b9adb..1305c66 100644
--- a/drivers/media/video/davinci/vpif_display.c
+++ b/drivers/media/video/davinci/vpif_display.c
@@ -31,6 +31,7 @@
 #include <linux/io.h>
 #include <linux/version.h>
 #include <linux/slab.h>
+#include <media/fpga_subdev.h>
 
 #include <asm/irq.h>
 #include <asm/page.h>
@@ -1739,7 +1742,7 @@
 	/* Allocate memory for six channel objects */
 	for (i = 0; i < VPIF_DISPLAY_MAX_DEVICES; i++) {
 		vpif_obj.dev[i] =
-		    kmalloc(sizeof(struct channel_obj), GFP_KERNEL);
+		    kzalloc(sizeof(struct channel_obj), GFP_KERNEL);
 		/* If memory allocation fails, return error */
 		if (!vpif_obj.dev[i]) {
 			free_channel_objects_index = i;
@@ -1897,10 +1900,18 @@
 	}
 
 	for (i = 0; i < subdev_count; i++) {
-		vpif_obj.sd[i] = v4l2_i2c_new_subdev_board(&vpif_obj.v4l2_dev,
+		if (subdevdata[i].board_info.addr) {
+			/* i2c device */
+			vpif_obj.sd[i] = v4l2_i2c_new_subdev_board(&vpif_obj.v4l2_dev,
 						i2c_adap, subdevdata[i].name,
 						&subdevdata[i].board_info,
 						NULL);
+		} else {
+			/* custom platform device */
+			vpif_obj.sd[i] =
+				fpga_subdev_init(&vpif_obj.v4l2_dev,
+						subdevdata[i].board_info.platform_data);
+		}
 		if (!vpif_obj.sd[i]) {
 			vpif_err("Error registering v4l2 subdevice\n");
 			goto probe_subdev_out;
diff --git a/drivers/media/video/fpga_subdev.c b/drivers/media/video/fpga_subdev.c
new file mode 100644
index 0000000..b57fe81
--- /dev/null
+++ b/drivers/media/video/fpga_subdev.c
@@ -0,0 +1,97 @@
+/*
+ * This subdevice driver can be used with custom made subdevices (e.g. FPGA's)
+ * that need some or no configuration. The driver calls functions specified in
+ * the board definition file.
+ *
+ * Copyright (C) 2010 Cisco Systems AS
+ * Author: Mats Randgaard <mats.randgaard@cisco.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+ */
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <media/fpga_subdev.h>
+#include <media/v4l2-device.h>
+
+MODULE_DESCRIPTION("FPGA subdevice");
+MODULE_AUTHOR("Mats Randgaard <mats.randgaard@cisco.com>");
+MODULE_LICENSE("GPL");
+
+/* Debug functions */
+static int debug;
+module_param(debug, bool, 0644);
+MODULE_PARM_DESC(debug, "Debug level (0-2)");
+
+static int fpga_subdev_s_power(struct v4l2_subdev *sd, int on)
+{
+	struct fpga_subdev_platform_data *pdata = v4l2_get_subdevdata(sd);
+
+	if (!pdata || !pdata->ops || !pdata->ops->s_power)
+		return -ENOIOCTLCMD;
+
+	return pdata->ops->s_power(pdata, sd, on);
+}
+
+static int fpga_subdev_s_stream(struct v4l2_subdev *sd, int enable)
+{
+	struct fpga_subdev_platform_data *pdata = v4l2_get_subdevdata(sd);
+
+	if (!pdata || !pdata->ops || !pdata->ops->s_stream)
+		return -ENOIOCTLCMD;
+
+	return pdata->ops->s_stream(pdata, sd, enable);
+}
+
+static const struct v4l2_subdev_core_ops fpga_subdev_core_ops = {
+	.s_power = fpga_subdev_s_power,
+};
+
+static const struct v4l2_subdev_video_ops fpga_subdev_video_ops = {
+	.s_stream = fpga_subdev_s_stream,
+};
+
+static const struct v4l2_subdev_ops fpga_subdev_ops = {
+	.core = &fpga_subdev_core_ops,
+	.video = &fpga_subdev_video_ops,
+};
+
+
+struct v4l2_subdev *fpga_subdev_init(struct v4l2_device *v4l2_dev,
+		struct fpga_subdev_platform_data *platform_data)
+{
+	struct v4l2_subdev *sd;
+
+	sd = kzalloc(sizeof(struct v4l2_subdev), GFP_KERNEL);
+	if (!sd) {
+		v4l2_err(v4l2_dev, "unable to allocate memory for subdevice\n");
+		return NULL;
+	}
+
+	v4l2_subdev_init(sd, &fpga_subdev_ops);
+
+	v4l2_set_subdevdata(sd, platform_data);
+
+	snprintf(sd->name, sizeof(sd->name), "%s fpga-subdev", v4l2_dev->name);
+
+	if (v4l2_device_register_subdev(v4l2_dev, sd)) {
+		kfree(sd);
+		v4l2_err(v4l2_dev, "unable to register v4l2 device\n");
+		return NULL;
+	}
+
+	v4l2_info(sd, "%s driver registered\n", sd->name);
+
+	return sd;
+}
diff --git a/include/media/fpga_subdev.h b/include/media/fpga_subdev.h
new file mode 100644
index 0000000..0651c05
--- /dev/null
+++ b/include/media/fpga_subdev.h
@@ -0,0 +1,44 @@
+/*
+ * This subdevice driver can be used with custom made subdevices (e.g. FPGA's)
+ * that need some or no configuration. The driver calls functions specified in
+ * the board definition file.
+ *
+ * Copyright (C) 2010 Cisco Systems AS
+ * Author: Mats Randgaard <mats.randgaard@cisco.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+ */
+#ifndef _FPGA_SUBDEV_H_
+#define _FPGA_SUBDEV_H_
+
+#include <media/v4l2-subdev.h>
+
+struct fpga_subdev_platform_data;
+
+struct fpga_subdev_ops {
+	int (*s_stream)(struct fpga_subdev_platform_data *pdata,
+			struct v4l2_subdev *sd, int enable);
+	int (*s_power)(struct fpga_subdev_platform_data *pdata,
+			struct v4l2_subdev *sd, int on);
+};
+
+struct fpga_subdev_platform_data {
+	void *priv;
+	struct fpga_subdev_ops *ops;
+};
+
+struct v4l2_subdev *fpga_subdev_init(struct v4l2_device *v4l2_dev,
+		struct fpga_subdev_platform_data *platform_data);
+#endif
-- 
1.7.1

Re: [RFC/PATCH 1/2] v4l: Add generic board subdev registration function

[Laurent Pinchart]

Hi Hans,

On Friday 20 May 2011 13:14:42 Hans Verkuil wrote:
> On Friday, May 20, 2011 12:12:23 Laurent Pinchart wrote:
> > On Friday 20 May 2011 11:52:17 Hans Verkuil wrote:
> > > On Friday, May 20, 2011 11:37:24 Laurent Pinchart wrote:
> > > > On Friday 20 May 2011 11:19:48 Hans Verkuil wrote:
> > > > > On Friday, May 20, 2011 11:05:00 Laurent Pinchart wrote:
> > [snip]
> > 
> > > > > > My idea was to use bus notifiers to delay the bridge/host device
> > > > > > initialization. The bridge probe() function would pre-initialize
> > > > > > the bridge and register notifiers. The driver would then wait
> > > > > > until all subdevs are properly registered, and then proceed from
> > > > > > to register V4L2 devices from the bus notifier callback (or
> > > > > > possible a work queue). There would be no nested probe() calls.
> > > > > 
> > > > > Would it be an option to create a new platform bus for the subdevs?
> > > > > That would have its own lock. It makes sense from a hierarchical
> > > > > point of view, but I'm not certain about the amount of work
> > > > > involved.
> > > > 
> > > > Do you mean a subdev-platform bus for platform subdevs, or a V4L2
> > > > subdev bus for all subdevs ? The first option is possible, but it
> > > > looks more like a hack to me. If the subdev really is a platform
> > > > device, it should be handled by the platform bus.
> > > 
> > > The first. So you have a 'top-level' platform device that wants to load
> > > platform subdevs (probably representing internal IP blocks). So it
> > > would create its own platform bus that is used to probe those platform
> > > subdevs.
> > > 
> > > It is similar to e.g. an I2C device that has internal I2C busses: you
> > > would also create nested busses there.
> > > 
> > > BTW, why do these platform subdevs have to be on the platform bus? Why
> > > not have standalone subdev drivers that are not on any bus? That's for
> > > example what ivtv does for the internal GPIO audio subdev.
> > 
> > There's some misunderstanging here. Internal IP blocks don't need to sit
> > on any bus. The host/bridge driver can create subdevs for those blocks
> > directly, as it doesn't need to load a separate driver.
> > 
> > The issue comes from external subdevs that offer little control or even
> > none at all. The best example is an FPGA that will feed video data to
> > the bridge in a fixed format without any mean of control, or with just
> > an on/off control through a GPIO. Support for such subdevices need to be
> > handled by a separate driver, so we need a way to load it at runtime.
> > I'm not sure on what bus that driver should sit.
> > 
> > > > I don't think the second option is possible, I2C and SPI subdevs need
> > > > to sit on an I2C or SPI bus (I could be mistaken though, there's at
> > > > least one example of a logical bus type in the kernel with the HID
> > > > bus).
> > > > 
> > > > Let's also not forget about sub-sub-devices. We need to handle them
> > > > at some point as well.
> > > 
> > > Sub-sub-devices are not a problem by themselves. They are only a
> > > problem if they on the same bus.
> > > 
> > > > This being said, I think that the use of platform devices to solve
> > > > the initial problem can also be considered a hack as well. What we
> > > > really need is a way to handle subdevs that can't be controlled at
> > > > all (a video source that continuously delivers data for instance),
> > > > or that can be controlled through GPIO. What bus should we use for a
> > > > bus-less subdev ? And for GPIO-based subdevs, should we create a
> > > > GPIO bus ?
> > > 
> > > It is perfectly possible to have bus-less subdevs. See ivtv (I think
> > > there are one or two other examples as well).
> > 
> > But how can we handle bus-less subdevs for embedded devices, where the
> > host (e.g. OMAP3 ISP) doesn't know about the external subdevs (e.g. FPGA
> > controlled by a couple of GPIOs) ?
> 
> I remembered that we had to do something similar and I found the patch
> below. This was the result of some private discussions with Vaibhav
> Hiremath from TI.
> 
> It almost certainly doesn't apply to the current kernel, but it is clear
> what happens.
> 
> We are actually using this with the dm6467 capture driver.
> 
> Hope this might at least give some ideas.

Thanks for the code.

My goal is to avoid having bus-specific (or "bus-less-specific") code in 
bridge drivers. The bridge drivers should get a list of subdev information 
from board code through platform data (or possibly through the device tree) 
and call V4L2 core functions to instantiate and/or bind to the subdevs, 
regardless of the bus they're on.

My RFC patch implements such a V4L2 abstraction function that supports I2C and 
SPI devices. If I were to add support for this "FPGA subdev" to that core 
function, this would add a dependency between the V4L2 core and the FPGA 
subdev driver. The FPGA subdev driver could be dynamically linked to the V4L2 
core through symbol_get(), but that's still not an ideal solution.

This won't solve the platform subdev problem though. Loading a platform driver 
(for the subdev) from within the probe function of another platform driver 
(the bridge) will lead to a deadlock. We need to find a way to fix that.

My feeling is that we should try to replace our own device instantiation and 
module loading code and use the Linux device/driver model as much as possible. 
This would help the transition to the device tree for embedded platforms. 
That's why bus notifiers look like a possible solution to me.

-- 
Regards,

Laurent Pinchart

Re: [RFC/PATCH 1/2] v4l: Add generic board subdev registration function

[Sylwester Nawrocki]

Hi Hans,

On 05/20/2011 11:52 AM, Hans Verkuil wrote:
> On Friday, May 20, 2011 11:37:24 Laurent Pinchart wrote:
>> Hi Hans,
>>
>> On Friday 20 May 2011 11:19:48 Hans Verkuil wrote:
>>> On Friday, May 20, 2011 11:05:00 Laurent Pinchart wrote:
>>>> On Friday 20 May 2011 10:53:32 Hans Verkuil wrote:
>>>>> On Friday, May 20, 2011 09:29:32 Laurent Pinchart wrote:
>>>>>> On Friday 20 May 2011 09:14:36 Sylwester Nawrocki wrote:
>>>>>>> On 05/19/2011 08:34 PM, Laurent Pinchart wrote:
>>>>>>>> The new v4l2_new_subdev_board() function creates and register a
>>>>>>>> subdev based on generic board information. The board information
>>>>>>>> structure includes a bus type and bus type-specific information.
>>>>>>>>
>>>>>>>> Only I2C and SPI busses are currently supported.
>>>>>>>>
>>>>>>>> Signed-off-by: Laurent Pinchart
>>>>>>>> <laurent.pinchart@ideasonboard.com>
>>>>
>>>> [snip]
>>>>
>>>>>>> I had an issue when tried to call request_module, to register 
> subdev
>>>>>>> of platform device type, in probe() of other platform device.
>>>>>>> Driver's probe() for devices belonging same bus type cannot be
>>>>>>> nested as the bus lock is taken by the driver core before entering
>>>>>>> probe(), so this would lead to a deadlock.
>>>>>>> That exactly happens in __driver_attach().
>>>>>>>
>>>>>>> For the same reason v4l2_new_subdev_board could not be called from
>>>>>>> probe() of devices belonging to I2C or SPI bus, as request_module
>>>>>>> is called inside of it. I'm not sure how to solve it, yet:)
>>>>>>
>>>>>> Ouch. I wasn't aware of that issue. Looks like it's indeed time to
>>>>>> fix the subdev registration issue, including the module load race
>>>>>> condition. Michael, you said you have a patch to add platform subdev
>>>>>> support, how have you avoided the race condition ?
>>>>>>
>>>>>> I've been thinking for some time now about removing the module load
>>>>>> code completely. I2C, SPI and platform subdevs would be registered
>>>>>> either by board code (possibly through the device tree on platforms
>>>>>> that suppport it) for embedded platforms, and by host drivers for
>>>>>> pluggable hardware (PCI and USB). Module loading would be handled
>>>>>> automatically by the kernel module auto loader, but asynchronously
>>>>>> instead of synchronously. Bus notifiers would then be used by host
>>>>>> drivers to wait for all subdevs to be registered.
>>>>>>
>>>>>> I'm not sure yet if this approach is viable. Hans, I think we've
>>>>>> briefly discussed this (possible quite a long time ago), do you have
>>>>>> any opinion ? Guennadi, based on your previous experience trying to
>>>>>> use bus notifiers to solve the module load race, what do you think
>>>>>> about the idea ? Others, please comment as well :-)
>>>>>
>>>>> It's definitely viable (I believe the required bus notification has
>>>>> been added some time ago), but I am not sure how to implement it in an
>>>>> efficient manner.
>>>>>
>>>>> My initial idea would be to just wait in v4l2_new_subdev_board until
>>>>> you get the notification on the bus (with a timeout, of course).
>>>>> However, I suspect that that does not solve the deadlock, although it
>>>>> would solve the race.
>>>>>
>>>>> As an aside: note that if the module is unloaded right after the
>>>>> request_module, then that will be detected by the code and it will 
> just
>>>>> return an error. It won't oops or anything like that. Personally I
>>>>> don't believe it is worth the effort just to solve this race, since it
>>>>> is highly theoretical.
>>>>>
>>>>> The problem of loading another bus module when in a bus probe function
>>>>> is a separate issue. My initial reaction is: why do you want to do 
> this?
>>>>> Even if you use delayed module loads, you probably still have to wait
>>>>> for them to succeed at a higher-level function. For example: in the
>>>>> probe function of module A it will attempt to load module B. That
>>>>> probably can't succeed as long as you are in A's probe function due to
>>>>> the bus lock. So you can't check for a successful load of B until you
>>>>> return from that probe function and a higher- level function (that
>>>>> likely loaded module A in the first place) does that check.
>>>>>
>>>>> That's all pretty tricky code, and my suggestion would be to simply 
> not
>>>>> do nested module loads from the same bus.
>>>>
>>>> That's unfortunately not an option. Most bridge/host devices in embedded
>>>> systems are platform devices, and they will need to load platform
>>>> subdevs. We need to fix that.
>>>
>>> Good point.
>>>
>>>> My idea was to use bus notifiers to delay the bridge/host device
>>>> initialization. The bridge probe() function would pre-initialize the
>>>> bridge and register notifiers. The driver would then wait until all
>>>> subdevs are properly registered, and then proceed from to register V4L2
>>>> devices from the bus notifier callback (or possible a work queue). There
>>>> would be no nested probe() calls.
>>>
>>> Would it be an option to create a new platform bus for the subdevs? That
>>> would have its own lock. It makes sense from a hierarchical point of view,
>>> but I'm not certain about the amount of work involved.
>>
>> Do you mean a subdev-platform bus for platform subdevs, or a V4L2 subdev bus 
>> for all subdevs ? The first option is possible, but it looks more like a 
> hack 
>> to me. If the subdev really is a platform device, it should be handled by 
> the 
>> platform bus.
> 
> The first. So you have a 'top-level' platform device that wants to load 
> platform subdevs (probably representing internal IP blocks). So it would 
> create its own platform bus that is used to probe those platform subdevs.
> 
> It is similar to e.g. an I2C device that has internal I2C busses: you would
> also create nested busses there.
> 
> BTW, why do these platform subdevs have to be on the platform bus? Why not 
> have standalone subdev drivers that are not on any bus? That's for example 
> what ivtv does for the internal GPIO audio subdev.

Platform devices can have dependencies on their bus drivers. Power/clock domains
can be one of the examples. Mostly host and subdev driver will belong to same
power domain though. There still might be some other side effects from ripping
platform device off from it's bus I'm not aware of right now. 

> 
>> I don't think the second option is possible, I2C and SPI subdevs need to sit 
>> on an I2C or SPI bus (I could be mistaken though, there's at least one 
> example 
>> of a logical bus type in the kernel with the HID bus).
>>
>> Let's also not forget about sub-sub-devices. We need to handle them at some 
>> point as well.
> 
> Sub-sub-devices are not a problem by themselves. They are only a problem if 
> they on the same bus.
> 
>> This being said, I think that the use of platform devices to solve the 
> initial 
>> problem can also be considered a hack as well. What we really need is a way 
> to 
>> handle subdevs that can't be controlled at all (a video source that 
>> continuously delivers data for instance), or that can be controlled through 
>> GPIO. What bus should we use for a bus-less subdev ? And for GPIO-based 
>> subdevs, should we create a GPIO bus ?
> 
> It is perfectly possible to have bus-less subdevs. See ivtv (I think there are 
> one or two other examples as well).
> 

Regards,
-- 
Sylwester Nawrocki
Samsung Poland R&D Center

Re: [RFC/PATCH 1/2] v4l: Add generic board subdev registration function

[Guennadi Liakhovetski]

On Fri, 20 May 2011, Laurent Pinchart wrote:

> Hi Sylwester,
> 
> On Friday 20 May 2011 09:14:36 Sylwester Nawrocki wrote:

[snip]

> > I had an issue when tried to call request_module, to register subdev of
> > platform device type, in probe() of other platform device. Driver's
> > probe() for devices belonging same bus type cannot be nested as the bus
> > lock is taken by the driver core before entering probe(), so this would
> > lead to a deadlock.
> > That exactly happens in __driver_attach().
> > 
> > For the same reason v4l2_new_subdev_board could not be called from probe()
> > of devices belonging to I2C or SPI bus, as request_module is called inside
> > of it. I'm not sure how to solve it, yet:)
> 
> Ouch. I wasn't aware of that issue. Looks like it's indeed time to fix the 
> subdev registration issue, including the module load race condition. Michael, 
> you said you have a patch to add platform subdev support, how have you avoided 
> the race condition ?
> 
> I've been thinking for some time now about removing the module load code 
> completely. I2C, SPI and platform subdevs would be registered either by board 
> code (possibly through the device tree on platforms that suppport it) for 
> embedded platforms, and by host drivers for pluggable hardware (PCI and USB). 
> Module loading would be handled automatically by the kernel module auto 
> loader, but asynchronously instead of synchronously. Bus notifiers would then 
> be used by host drivers to wait for all subdevs to be registered.

Sorry, I'm probably missing something. The reason for this module loading 
was, that you cannot probe i2c sensors before the host is initialised and 
has turned the master clock on. If you want to go back to the traditional 
platform-based I2C device registration, you'll have to wait in your sensor 
(subdev) probe function for host registration, which wouldn't be a good 
thing to do, IMHO.

> I'm not sure yet if this approach is viable. Hans, I think we've briefly 
> discussed this (possible quite a long time ago), do you have any opinion ? 
> Guennadi, based on your previous experience trying to use bus notifiers to 
> solve the module load race, what do you think about the idea ? Others, please 
> comment as well :-)

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [RFC/PATCH 1/2] v4l: Add generic board subdev registration function

[Laurent Pinchart]

Hi Guennadi,

On Friday 20 May 2011 15:08:00 Guennadi Liakhovetski wrote:
> On Fri, 20 May 2011, Laurent Pinchart wrote:
> > On Friday 20 May 2011 09:14:36 Sylwester Nawrocki wrote:
> [snip]
> 
> > > I had an issue when tried to call request_module, to register subdev of
> > > platform device type, in probe() of other platform device. Driver's
> > > probe() for devices belonging same bus type cannot be nested as the bus
> > > lock is taken by the driver core before entering probe(), so this would
> > > lead to a deadlock.
> > > That exactly happens in __driver_attach().
> > > 
> > > For the same reason v4l2_new_subdev_board could not be called from
> > > probe() of devices belonging to I2C or SPI bus, as request_module is
> > > called inside of it. I'm not sure how to solve it, yet:)
> > 
> > Ouch. I wasn't aware of that issue. Looks like it's indeed time to fix
> > the subdev registration issue, including the module load race condition.
> > Michael, you said you have a patch to add platform subdev support, how
> > have you avoided the race condition ?
> > 
> > I've been thinking for some time now about removing the module load code
> > completely. I2C, SPI and platform subdevs would be registered either by
> > board code (possibly through the device tree on platforms that suppport
> > it) for embedded platforms, and by host drivers for pluggable hardware
> > (PCI and USB). Module loading would be handled automatically by the
> > kernel module auto loader, but asynchronously instead of synchronously.
> > Bus notifiers would then be used by host drivers to wait for all subdevs
> > to be registered.
> 
> Sorry, I'm probably missing something. The reason for this module loading
> was, that you cannot probe i2c sensors before the host is initialised and
> has turned the master clock on.

Only when the subdev clock is provided by the host. And worse than that, when 
clock configuration needs to go through board code, you often need to wait 
until the host has registered the subdev before the board code can be 
executed. That's why many subdev drivers only initialize a couple of 
structures in their probe() function, and don't access the hardware until the 
registered() callback is called.

> If you want to go back to the traditional platform-based I2C device
> registration, you'll have to wait in your sensor (subdev) probe function for
> host registration, which wouldn't be a good thing to do, IMHO.

Waiting for host initialization in the subdev probe function is definitely not 
a good option.

There are various dependencies between the host and the subdevs. The host 
can't obviously proceed before all subdevs are ready, and subdevs often depend 
on the host to provide clocks and power. Initializing the host first, making 
the host register the subdev devices and waiting synchronously for them to be 
initialized is the option we went for. Unfortunately this brings several 
issues, such as deadlocks when the host and the subdevs sit on the same bus.

Another option I'm proposing is to let Linux load modules and initialize 
devices without interfering with that. Host drivers would be notified that all 
subdevs are ready through bus notifiers, and subdev drivers would be notified 
that they can access host resources through the registered callback. This 
splits initialization in two parts.

I'm of course open to other options.

> > I'm not sure yet if this approach is viable. Hans, I think we've briefly
> > discussed this (possible quite a long time ago), do you have any opinion
> > ? Guennadi, based on your previous experience trying to use bus
> > notifiers to solve the module load race, what do you think about the
> > idea ? Others, please comment as well :-)

-- 
Regards,

Laurent Pinchart

Re: [RFC/PATCH 1/2] v4l: Add generic board subdev registration function

[Rick Bronson]

Hi All,

  I'm trying to add a SPI camera to 2.6.39 but ran into trouble with
isp_register_subdev_group() in drivers/media/video/omap3isp/isp.c
hardcoded to use i2c.  The platform is a BeagleBoardXM.  I tried these
two patches:

http://patchwork.linuxtv.org/patch/6651/mbox/
http://patchwork.linuxtv.org/patch/6650/raw/

  or:

RFC-PATCH-2-2-omap3isp-Use-generic-subdev-registration-function.patch
RFC-PATCH-1-2-v4l-Add-generic-board-subdev-registration-function.patch

  It crashes, from what I can tell, it seems to be coming into
isp_register_entities(), then iterating in:

---------------------------------
	/* Register external entities */
	for (subdevs = pdata->subdevs; subdevs->subdevs; ++subdevs) {
---------------------------------

  But it's iterating through ev76c560_camera_subdevs (see
http://efn.org/~rick/pub/board-portal7-camera.c), seems like
it should be iterating through portal7_camera_subdevs in the same
file.

  Any help would be greatly appreciated.

  Thanks,

  Rick Bronson

Re: [RFC/PATCH 1/2] v4l: Add generic board subdev registration function

[Rick Bronson]

Hi All,

  I'm trying to add a SPI camera to 2.6.39 but ran into trouble with
isp_register_subdev_group() in drivers/media/video/omap3isp/isp.c
hardcoded to use i2c.  The platform is a BeagleBoardXM.  I tried these
two patches:

http://patchwork.linuxtv.org/patch/6651/mbox/
http://patchwork.linuxtv.org/patch/6650/raw/

  or:

RFC-PATCH-2-2-omap3isp-Use-generic-subdev-registration-function.patch
RFC-PATCH-1-2-v4l-Add-generic-board-subdev-registration-function.patch

  It crashes, from what I can tell, it seems to be coming into
isp_register_entities(), then iterating in:

---------------------------------
	/* Register external entities */
	for (subdevs = pdata->subdevs; subdevs->subdevs; ++subdevs) {
---------------------------------

  But it's iterating through ev76c560_camera_subdevs (see
http://efn.org/~rick/pub/board-portal7-camera.c), seems like
it should be iterating through portal7_camera_subdevs in the same
file.

  Any help would be greatly appreciated.

  Thanks,

  Rick Bronson