[RFC,3/3] media: added managed v4l2 subdevice initialization
Commit Message
This patch adds managed versions of initialization
functions for v4l2 subdevices.
Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
Reviewed-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
drivers/media/v4l2-core/v4l2-common.c | 10 +++++++
drivers/media/v4l2-core/v4l2-subdev.c | 52 +++++++++++++++++++++++++++++++++
include/media/v4l2-common.h | 2 ++
include/media/v4l2-ctrls.h | 7 +++--
include/media/v4l2-subdev.h | 5 ++++
5 files changed, 74 insertions(+), 2 deletions(-)
Comments
On Thu May 9 2013 14:52:44 Andrzej Hajda wrote:
> This patch adds managed versions of initialization
> functions for v4l2 subdevices.
I am not convinced of the usefulness of this. The *_subdev_init functions
are void functions and so do not return an error. So there isn't much to
manage.
It feels like the wrong approach to me.
The core problem is that you do not know what will be unregistered first:
the i2c adapter or the subdev. Depending on that the v4l2_device_unregister_subdev()
call is or is not needed.
Regards,
Hans
>
> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
> Reviewed-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
> drivers/media/v4l2-core/v4l2-common.c | 10 +++++++
> drivers/media/v4l2-core/v4l2-subdev.c | 52 +++++++++++++++++++++++++++++++++
> include/media/v4l2-common.h | 2 ++
> include/media/v4l2-ctrls.h | 7 +++--
> include/media/v4l2-subdev.h | 5 ++++
> 5 files changed, 74 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c
> index 614316f..714d07c 100644
> --- a/drivers/media/v4l2-core/v4l2-common.c
> +++ b/drivers/media/v4l2-core/v4l2-common.c
> @@ -302,7 +302,17 @@ void v4l2_i2c_subdev_init(struct v4l2_subdev *sd, struct i2c_client *client,
> }
> EXPORT_SYMBOL_GPL(v4l2_i2c_subdev_init);
>
> +int devm_v4l2_i2c_subdev_init(struct v4l2_subdev *sd, struct i2c_client *client,
> + const struct v4l2_subdev_ops *ops)
> +{
> + int ret;
>
> + ret = devm_v4l2_subdev_bind(&client->dev, sd);
> + if (!ret)
> + v4l2_i2c_subdev_init(sd, client, ops);
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(devm_v4l2_i2c_subdev_init);
>
> /* Load an i2c sub-device. */
> struct v4l2_subdev *v4l2_i2c_new_subdev_board(struct v4l2_device *v4l2_dev,
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index 996c248..87ce2f6 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -474,3 +474,55 @@ void v4l2_subdev_init(struct v4l2_subdev *sd, const struct v4l2_subdev_ops *ops)
> #endif
> }
> EXPORT_SYMBOL(v4l2_subdev_init);
> +
> +static void devm_v4l2_subdev_release(struct device *dev, void *res)
> +{
> + struct v4l2_subdev **sd = res;
> +
> + v4l2_device_unregister_subdev(*sd);
> +#if defined(CONFIG_MEDIA_CONTROLLER)
> + media_entity_cleanup(&(*sd)->entity);
> +#endif
> +}
> +
> +int devm_v4l2_subdev_bind(struct device *dev, struct v4l2_subdev *sd)
> +{
> + struct v4l2_subdev **dr;
> +
> + dr = devres_alloc(devm_v4l2_subdev_release, sizeof(*dr), GFP_KERNEL);
> + if (!dr)
> + return -ENOMEM;
> +
> + *dr = sd;
> + devres_add(dev, dr);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(devm_v4l2_subdev_bind);
> +
> +int devm_v4l2_subdev_init(struct device *dev, struct v4l2_subdev *sd,
> + const struct v4l2_subdev_ops *ops)
> +{
> + int ret;
> +
> + ret = devm_v4l2_subdev_bind(dev, sd);
> + if (!ret)
> + v4l2_subdev_init(sd, ops);
> + return ret;
> +}
> +EXPORT_SYMBOL(devm_v4l2_subdev_init);
> +
> +static int devm_v4l2_subdev_match(struct device *dev, void *res,
> + void *data)
> +{
> + struct v4l2_subdev **this = res, **sd = data;
> +
> + return *this == *sd;
> +}
> +
> +void devm_v4l2_subdev_free(struct device *dev, struct v4l2_subdev *sd)
> +{
> + WARN_ON(devres_release(dev, devm_v4l2_subdev_release,
> + devm_v4l2_subdev_match, &sd));
> +}
> +EXPORT_SYMBOL_GPL(devm_v4l2_subdev_free);
> diff --git a/include/media/v4l2-common.h b/include/media/v4l2-common.h
> index ec7c9c0..440d6b7 100644
> --- a/include/media/v4l2-common.h
> +++ b/include/media/v4l2-common.h
> @@ -136,6 +136,8 @@ struct v4l2_subdev *v4l2_i2c_new_subdev_board(struct v4l2_device *v4l2_dev,
> /* Initialize a v4l2_subdev with data from an i2c_client struct */
> void v4l2_i2c_subdev_init(struct v4l2_subdev *sd, struct i2c_client *client,
> const struct v4l2_subdev_ops *ops);
> +int devm_v4l2_i2c_subdev_init(struct v4l2_subdev *sd, struct i2c_client *client,
> + const struct v4l2_subdev_ops *ops);
> /* Return i2c client address of v4l2_subdev. */
> unsigned short v4l2_i2c_subdev_addr(struct v4l2_subdev *sd);
>
> diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
> index a1d06db..fe6dcef 100644
> --- a/include/media/v4l2-ctrls.h
> +++ b/include/media/v4l2-ctrls.h
> @@ -286,7 +286,10 @@ void v4l2_ctrl_handler_free(struct v4l2_ctrl_handler *hdl);
> /*
> * devm_v4l2_ctrl_handler_init - managed control handler initialization
> *
> - * @dev: Device for which @hdl belongs to.
> + * @dev: Device the @hdl belongs to.
> + * @hdl: The control handler.
> + * @nr_of_controls_hint: A hint of how many controls this handler is
> + * expected to refer to.
> *
> * This is a managed version of v4l2_ctrl_handler_init. Handler initialized with
> * this function will be automatically cleaned up on driver detach.
> @@ -301,7 +304,7 @@ int devm_v4l2_ctrl_handler_init(struct device *dev,
> /**
> * devm_v4l2_ctrl_handler_free - managed control handler free
> *
> - * @dev: Device for which @hdl belongs to.
> + * @dev: Device the @hdl belongs to.
> * @hdl: Handler to be cleaned up.
> *
> * This function should be used to manual free of an control handler
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index b137a5e..0eab5b0 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -673,6 +673,11 @@ int v4l2_subdev_link_validate(struct media_link *link);
> void v4l2_subdev_init(struct v4l2_subdev *sd,
> const struct v4l2_subdev_ops *ops);
>
> +int devm_v4l2_subdev_bind(struct device *dev, struct v4l2_subdev *sd);
> +int devm_v4l2_subdev_init(struct device *dev, struct v4l2_subdev *sd,
> + const struct v4l2_subdev_ops *ops);
> +void devm_v4l2_subdev_free(struct device *dev, struct v4l2_subdev *sd);
> +
> /* Call an ops of a v4l2_subdev, doing the right checks against
> NULL pointers.
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Hans,
Thank you for the review.
On 10.05.2013 14:31, Hans Verkuil wrote:
> On Thu May 9 2013 14:52:44 Andrzej Hajda wrote:
>> This patch adds managed versions of initialization
>> functions for v4l2 subdevices.
> I am not convinced of the usefulness of this. The *_subdev_init functions
> are void functions and so do not return an error. So there isn't much to
> manage.
>
> It feels like the wrong approach to me.
>
> The core problem is that you do not know what will be unregistered first:
> the i2c adapter or the subdev. Depending on that the v4l2_device_unregister_subdev()
> call is or is not needed.
v4l2_device_unregister_subdev() contains already necessary checks so it
is safe
to call it in i2c driver remove callback and currently this is a common
practice - most
drivers calls it unconditionally.
Typical code of i2c_driver::remove in most media/i2c devices currently
contains
some variations of the following sequence:
v4l2_device_unregister_subdev(sd);
media_entity_cleanup(&sd->entity);
v4l2_ctrl_handler_free(sd->ctrl_handler);
Using devm_* initialization will allow to remove the sequence above from
i2c_driver::remove callback,
in many cases of existing drivers i2c_driver::remove could be removed
completely.
Regards
Andrzej
>
> Regards,
>
> Hans
>
>> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
>> Reviewed-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>> ---
>> drivers/media/v4l2-core/v4l2-common.c | 10 +++++++
>> drivers/media/v4l2-core/v4l2-subdev.c | 52 +++++++++++++++++++++++++++++++++
>> include/media/v4l2-common.h | 2 ++
>> include/media/v4l2-ctrls.h | 7 +++--
>> include/media/v4l2-subdev.h | 5 ++++
>> 5 files changed, 74 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c
>> index 614316f..714d07c 100644
>> --- a/drivers/media/v4l2-core/v4l2-common.c
>> +++ b/drivers/media/v4l2-core/v4l2-common.c
>> @@ -302,7 +302,17 @@ void v4l2_i2c_subdev_init(struct v4l2_subdev *sd, struct i2c_client *client,
>> }
>> EXPORT_SYMBOL_GPL(v4l2_i2c_subdev_init);
>>
>> +int devm_v4l2_i2c_subdev_init(struct v4l2_subdev *sd, struct i2c_client *client,
>> + const struct v4l2_subdev_ops *ops)
>> +{
>> + int ret;
>>
>> + ret = devm_v4l2_subdev_bind(&client->dev, sd);
>> + if (!ret)
>> + v4l2_i2c_subdev_init(sd, client, ops);
>> + return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(devm_v4l2_i2c_subdev_init);
>>
>> /* Load an i2c sub-device. */
>> struct v4l2_subdev *v4l2_i2c_new_subdev_board(struct v4l2_device *v4l2_dev,
>> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
>> index 996c248..87ce2f6 100644
>> --- a/drivers/media/v4l2-core/v4l2-subdev.c
>> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
>> @@ -474,3 +474,55 @@ void v4l2_subdev_init(struct v4l2_subdev *sd, const struct v4l2_subdev_ops *ops)
>> #endif
>> }
>> EXPORT_SYMBOL(v4l2_subdev_init);
>> +
>> +static void devm_v4l2_subdev_release(struct device *dev, void *res)
>> +{
>> + struct v4l2_subdev **sd = res;
>> +
>> + v4l2_device_unregister_subdev(*sd);
>> +#if defined(CONFIG_MEDIA_CONTROLLER)
>> + media_entity_cleanup(&(*sd)->entity);
>> +#endif
>> +}
>> +
>> +int devm_v4l2_subdev_bind(struct device *dev, struct v4l2_subdev *sd)
>> +{
>> + struct v4l2_subdev **dr;
>> +
>> + dr = devres_alloc(devm_v4l2_subdev_release, sizeof(*dr), GFP_KERNEL);
>> + if (!dr)
>> + return -ENOMEM;
>> +
>> + *dr = sd;
>> + devres_add(dev, dr);
>> +
>> + return 0;
>> +}
>> +EXPORT_SYMBOL(devm_v4l2_subdev_bind);
>> +
>> +int devm_v4l2_subdev_init(struct device *dev, struct v4l2_subdev *sd,
>> + const struct v4l2_subdev_ops *ops)
>> +{
>> + int ret;
>> +
>> + ret = devm_v4l2_subdev_bind(dev, sd);
>> + if (!ret)
>> + v4l2_subdev_init(sd, ops);
>> + return ret;
>> +}
>> +EXPORT_SYMBOL(devm_v4l2_subdev_init);
>> +
>> +static int devm_v4l2_subdev_match(struct device *dev, void *res,
>> + void *data)
>> +{
>> + struct v4l2_subdev **this = res, **sd = data;
>> +
>> + return *this == *sd;
>> +}
>> +
>> +void devm_v4l2_subdev_free(struct device *dev, struct v4l2_subdev *sd)
>> +{
>> + WARN_ON(devres_release(dev, devm_v4l2_subdev_release,
>> + devm_v4l2_subdev_match, &sd));
>> +}
>> +EXPORT_SYMBOL_GPL(devm_v4l2_subdev_free);
>> diff --git a/include/media/v4l2-common.h b/include/media/v4l2-common.h
>> index ec7c9c0..440d6b7 100644
>> --- a/include/media/v4l2-common.h
>> +++ b/include/media/v4l2-common.h
>> @@ -136,6 +136,8 @@ struct v4l2_subdev *v4l2_i2c_new_subdev_board(struct v4l2_device *v4l2_dev,
>> /* Initialize a v4l2_subdev with data from an i2c_client struct */
>> void v4l2_i2c_subdev_init(struct v4l2_subdev *sd, struct i2c_client *client,
>> const struct v4l2_subdev_ops *ops);
>> +int devm_v4l2_i2c_subdev_init(struct v4l2_subdev *sd, struct i2c_client *client,
>> + const struct v4l2_subdev_ops *ops);
>> /* Return i2c client address of v4l2_subdev. */
>> unsigned short v4l2_i2c_subdev_addr(struct v4l2_subdev *sd);
>>
>> diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
>> index a1d06db..fe6dcef 100644
>> --- a/include/media/v4l2-ctrls.h
>> +++ b/include/media/v4l2-ctrls.h
>> @@ -286,7 +286,10 @@ void v4l2_ctrl_handler_free(struct v4l2_ctrl_handler *hdl);
>> /*
>> * devm_v4l2_ctrl_handler_init - managed control handler initialization
>> *
>> - * @dev: Device for which @hdl belongs to.
>> + * @dev: Device the @hdl belongs to.
>> + * @hdl: The control handler.
>> + * @nr_of_controls_hint: A hint of how many controls this handler is
>> + * expected to refer to.
>> *
>> * This is a managed version of v4l2_ctrl_handler_init. Handler initialized with
>> * this function will be automatically cleaned up on driver detach.
>> @@ -301,7 +304,7 @@ int devm_v4l2_ctrl_handler_init(struct device *dev,
>> /**
>> * devm_v4l2_ctrl_handler_free - managed control handler free
>> *
>> - * @dev: Device for which @hdl belongs to.
>> + * @dev: Device the @hdl belongs to.
>> * @hdl: Handler to be cleaned up.
>> *
>> * This function should be used to manual free of an control handler
>> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
>> index b137a5e..0eab5b0 100644
>> --- a/include/media/v4l2-subdev.h
>> +++ b/include/media/v4l2-subdev.h
>> @@ -673,6 +673,11 @@ int v4l2_subdev_link_validate(struct media_link *link);
>> void v4l2_subdev_init(struct v4l2_subdev *sd,
>> const struct v4l2_subdev_ops *ops);
>>
>> +int devm_v4l2_subdev_bind(struct device *dev, struct v4l2_subdev *sd);
>> +int devm_v4l2_subdev_init(struct device *dev, struct v4l2_subdev *sd,
>> + const struct v4l2_subdev_ops *ops);
>> +void devm_v4l2_subdev_free(struct device *dev, struct v4l2_subdev *sd);
>> +
>> /* Call an ops of a v4l2_subdev, doing the right checks against
>> NULL pointers.
>>
>>
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
@@ -302,7 +302,17 @@ void v4l2_i2c_subdev_init(struct v4l2_subdev *sd, struct i2c_client *client,
}
EXPORT_SYMBOL_GPL(v4l2_i2c_subdev_init);
+int devm_v4l2_i2c_subdev_init(struct v4l2_subdev *sd, struct i2c_client *client,
+ const struct v4l2_subdev_ops *ops)
+{
+ int ret;
+ ret = devm_v4l2_subdev_bind(&client->dev, sd);
+ if (!ret)
+ v4l2_i2c_subdev_init(sd, client, ops);
+ return ret;
+}
+EXPORT_SYMBOL_GPL(devm_v4l2_i2c_subdev_init);
/* Load an i2c sub-device. */
struct v4l2_subdev *v4l2_i2c_new_subdev_board(struct v4l2_device *v4l2_dev,
@@ -474,3 +474,55 @@ void v4l2_subdev_init(struct v4l2_subdev *sd, const struct v4l2_subdev_ops *ops)
#endif
}
EXPORT_SYMBOL(v4l2_subdev_init);
+
+static void devm_v4l2_subdev_release(struct device *dev, void *res)
+{
+ struct v4l2_subdev **sd = res;
+
+ v4l2_device_unregister_subdev(*sd);
+#if defined(CONFIG_MEDIA_CONTROLLER)
+ media_entity_cleanup(&(*sd)->entity);
+#endif
+}
+
+int devm_v4l2_subdev_bind(struct device *dev, struct v4l2_subdev *sd)
+{
+ struct v4l2_subdev **dr;
+
+ dr = devres_alloc(devm_v4l2_subdev_release, sizeof(*dr), GFP_KERNEL);
+ if (!dr)
+ return -ENOMEM;
+
+ *dr = sd;
+ devres_add(dev, dr);
+
+ return 0;
+}
+EXPORT_SYMBOL(devm_v4l2_subdev_bind);
+
+int devm_v4l2_subdev_init(struct device *dev, struct v4l2_subdev *sd,
+ const struct v4l2_subdev_ops *ops)
+{
+ int ret;
+
+ ret = devm_v4l2_subdev_bind(dev, sd);
+ if (!ret)
+ v4l2_subdev_init(sd, ops);
+ return ret;
+}
+EXPORT_SYMBOL(devm_v4l2_subdev_init);
+
+static int devm_v4l2_subdev_match(struct device *dev, void *res,
+ void *data)
+{
+ struct v4l2_subdev **this = res, **sd = data;
+
+ return *this == *sd;
+}
+
+void devm_v4l2_subdev_free(struct device *dev, struct v4l2_subdev *sd)
+{
+ WARN_ON(devres_release(dev, devm_v4l2_subdev_release,
+ devm_v4l2_subdev_match, &sd));
+}
+EXPORT_SYMBOL_GPL(devm_v4l2_subdev_free);
@@ -136,6 +136,8 @@ struct v4l2_subdev *v4l2_i2c_new_subdev_board(struct v4l2_device *v4l2_dev,
/* Initialize a v4l2_subdev with data from an i2c_client struct */
void v4l2_i2c_subdev_init(struct v4l2_subdev *sd, struct i2c_client *client,
const struct v4l2_subdev_ops *ops);
+int devm_v4l2_i2c_subdev_init(struct v4l2_subdev *sd, struct i2c_client *client,
+ const struct v4l2_subdev_ops *ops);
/* Return i2c client address of v4l2_subdev. */
unsigned short v4l2_i2c_subdev_addr(struct v4l2_subdev *sd);
@@ -286,7 +286,10 @@ void v4l2_ctrl_handler_free(struct v4l2_ctrl_handler *hdl);
/*
* devm_v4l2_ctrl_handler_init - managed control handler initialization
*
- * @dev: Device for which @hdl belongs to.
+ * @dev: Device the @hdl belongs to.
+ * @hdl: The control handler.
+ * @nr_of_controls_hint: A hint of how many controls this handler is
+ * expected to refer to.
*
* This is a managed version of v4l2_ctrl_handler_init. Handler initialized with
* this function will be automatically cleaned up on driver detach.
@@ -301,7 +304,7 @@ int devm_v4l2_ctrl_handler_init(struct device *dev,
/**
* devm_v4l2_ctrl_handler_free - managed control handler free
*
- * @dev: Device for which @hdl belongs to.
+ * @dev: Device the @hdl belongs to.
* @hdl: Handler to be cleaned up.
*
* This function should be used to manual free of an control handler
@@ -673,6 +673,11 @@ int v4l2_subdev_link_validate(struct media_link *link);
void v4l2_subdev_init(struct v4l2_subdev *sd,
const struct v4l2_subdev_ops *ops);
+int devm_v4l2_subdev_bind(struct device *dev, struct v4l2_subdev *sd);
+int devm_v4l2_subdev_init(struct device *dev, struct v4l2_subdev *sd,
+ const struct v4l2_subdev_ops *ops);
+void devm_v4l2_subdev_free(struct device *dev, struct v4l2_subdev *sd);
+
/* Call an ops of a v4l2_subdev, doing the right checks against
NULL pointers.