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@xxxxxxxxxxxxxxxx>
> ---
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
