[PATCH RFC 0/3] added managed media/v4l2 initialization

[PATCH RFC 0/3] added managed media/v4l2 initialization

[Andrzej Hajda]

Those three patches adds devm_* functions for initialization of:
- media entity,
- subdevice,
- v4l2 controls handler.

Converting current v4l2 (sub-)devices to use devm API should simplify
device cleanup routines.

Andrzej Hajda (3):
  media: added managed media entity initialization
  media: added managed v4l2 control initialization
  media: added managed v4l2 subdevice initialization

 drivers/media/media-entity.c          |   70 +++++++++++++++++++++++++++++++++
 drivers/media/v4l2-core/v4l2-common.c |   10 +++++
 drivers/media/v4l2-core/v4l2-ctrls.c  |   48 ++++++++++++++++++++++
 drivers/media/v4l2-core/v4l2-subdev.c |   52 ++++++++++++++++++++++++
 include/media/media-entity.h          |    4 ++
 include/media/v4l2-common.h           |    2 +
 include/media/v4l2-ctrls.h            |   30 ++++++++++++++
 include/media/v4l2-subdev.h           |    5 +++
 8 files changed, 221 insertions(+)

-- 
1.7.10.4

[PATCH RFC 1/3] media: added managed media entity initialization

[Andrzej Hajda]

This patch adds managed versions of initialization
and cleanup functions for media entity.

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/media-entity.c |   70 ++++++++++++++++++++++++++++++++++++++++++
 include/media/media-entity.h |    4 +++
 2 files changed, 74 insertions(+)

diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
index e1cd132..696de35 100644
--- a/drivers/media/media-entity.c
+++ b/drivers/media/media-entity.c
@@ -82,9 +82,79 @@ void
 media_entity_cleanup(struct media_entity *entity)
 {
 	kfree(entity->links);
+	entity->links = NULL;
 }
 EXPORT_SYMBOL_GPL(media_entity_cleanup);
 
+static void devm_media_entity_release(struct device *dev, void *res)
+{
+	struct media_entity **entity = res;
+
+	media_entity_cleanup(*entity);
+}
+
+/**
+ * devm_media_entity_init - managed media entity initialization
+ *
+ * @dev: Device for which @entity belongs to.
+ * @entity: Entity to be initialized.
+ * @num_pads: Total number of sink and source pads.
+ * @pads: Array of 'num_pads' pads.
+ * @extra_links: Initial estimate of the number of extra links.
+ *
+ * This is a managed version of media_entity_init. Entity initialized with
+ * this function will be automatically cleaned up on driver detach.
+ *
+ * If an entity initialized with this function needs to be cleaned up
+ * separately, devm_media_entity_cleanup() must be used.
+ */
+int
+devm_media_entity_init(struct device *dev, struct media_entity *entity,
+		       u16 num_pads, struct media_pad *pads, u16 extra_links)
+{
+	struct media_entity **dr;
+	int rc;
+
+	dr = devres_alloc(devm_media_entity_release, sizeof(*dr), GFP_KERNEL);
+	if (!dr)
+		return -ENOMEM;
+
+	rc = media_entity_init(entity, num_pads, pads, extra_links);
+	if (rc) {
+		devres_free(dr);
+		return rc;
+	}
+
+	*dr = entity;
+	devres_add(dev, dr);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(devm_media_entity_init);
+
+static int devm_media_entity_match(struct device *dev, void *res, void *data)
+{
+	struct media_entity **this = res, **entity = data;
+
+	return *this == *entity;
+}
+
+/**
+ * devm_media_entity_cleanup - managed media entity cleanup
+ *
+ * @dev: Device for which @entity belongs to.
+ * @entity: Entity to be cleaned up.
+ *
+ * This function should be used to manual cleanup of an media entity
+ * initialized with devm_media_entity_init().
+ */
+void devm_media_entity_cleanup(struct device *dev, struct media_entity *entity)
+{
+	WARN_ON(devres_release(dev, devm_media_entity_release,
+			       devm_media_entity_match, &entity));
+}
+EXPORT_SYMBOL_GPL(devm_media_entity_cleanup);
+
 /* -----------------------------------------------------------------------------
  * Graph traversal
  */
diff --git a/include/media/media-entity.h b/include/media/media-entity.h
index 0c16f51..af732ca 100644
--- a/include/media/media-entity.h
+++ b/include/media/media-entity.h
@@ -126,6 +126,10 @@ int media_entity_init(struct media_entity *entity, u16 num_pads,
 		struct media_pad *pads, u16 extra_links);
 void media_entity_cleanup(struct media_entity *entity);
 
+int devm_media_entity_init(struct device *dev, struct media_entity *entity,
+		u16 num_pads, struct media_pad *pads, u16 extra_links);
+void devm_media_entity_cleanup(struct device *dev, struct media_entity *entity);
+
 int media_entity_create_link(struct media_entity *source, u16 source_pad,
 		struct media_entity *sink, u16 sink_pad, u32 flags);
 int __media_entity_setup_link(struct media_link *link, u32 flags);
-- 
1.7.10.4

[PATCH RFC 2/3] media: added managed v4l2 control initialization

[Andrzej Hajda]

This patch adds managed versions of initialization
and cleanup functions for v4l2 control handler.

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-ctrls.c |   48 ++++++++++++++++++++++++++++++++++
 include/media/v4l2-ctrls.h           |   27 +++++++++++++++++++
 2 files changed, 75 insertions(+)

diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
index 6b56d7b..1f16405 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -1411,6 +1411,54 @@ void v4l2_ctrl_handler_free(struct v4l2_ctrl_handler *hdl)
 }
 EXPORT_SYMBOL(v4l2_ctrl_handler_free);
 
+static void devm_v4l2_ctrl_handler_release(struct device *dev, void *res)
+{
+	struct v4l2_ctrl_handler **hdl = res;
+
+	v4l2_ctrl_handler_free(*hdl);
+}
+
+int devm_v4l2_ctrl_handler_init(struct device *dev,
+				struct v4l2_ctrl_handler *hdl,
+				unsigned nr_of_controls_hint)
+{
+	struct v4l2_ctrl_handler **dr;
+	int rc;
+
+	dr = devres_alloc(devm_v4l2_ctrl_handler_release, sizeof(*dr),
+			  GFP_KERNEL);
+	if (!dr)
+		return -ENOMEM;
+
+	rc = v4l2_ctrl_handler_init(hdl, nr_of_controls_hint);
+	if (rc) {
+		devres_free(dr);
+		return rc;
+	}
+
+	*dr = hdl;
+	devres_add(dev, dr);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(devm_v4l2_ctrl_handler_init);
+
+static int devm_v4l2_ctrl_handler_match(struct device *dev, void *res,
+					void *data)
+{
+	struct v4l2_ctrl_handler **this = res, **hdl = data;
+
+	return *this == *hdl;
+}
+
+void devm_v4l2_ctrl_handler_free(struct device *dev,
+				 struct v4l2_ctrl_handler *hdl)
+{
+	WARN_ON(devres_release(dev, devm_v4l2_ctrl_handler_release,
+			       devm_v4l2_ctrl_handler_match, &hdl));
+}
+EXPORT_SYMBOL_GPL(devm_v4l2_ctrl_handler_free);
+
 /* For backwards compatibility: V4L2_CID_PRIVATE_BASE should no longer
    be used except in G_CTRL, S_CTRL, QUERYCTRL and QUERYMENU when dealing
    with applications that do not use the NEXT_CTRL flag.
diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
index f00d42b..a1d06db 100644
--- a/include/media/v4l2-ctrls.h
+++ b/include/media/v4l2-ctrls.h
@@ -283,6 +283,33 @@ int v4l2_ctrl_handler_init(struct v4l2_ctrl_handler *hdl,
   */
 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.
+ *
+ * This is a managed version of v4l2_ctrl_handler_init. Handler initialized with
+ * this function will be automatically cleaned up on driver detach.
+ *
+ * If an handler initialized with this function needs to be cleaned up
+ * separately, devm_v4l2_ctrl_handler_free() must be used.
+ */
+int devm_v4l2_ctrl_handler_init(struct device *dev,
+				struct v4l2_ctrl_handler *hdl,
+				unsigned nr_of_controls_hint);
+
+/**
+ * devm_v4l2_ctrl_handler_free - managed control handler free
+ *
+ * @dev: Device for which @hdl belongs to.
+ * @hdl: Handler to be cleaned up.
+ *
+ * This function should be used to manual free of an control handler
+ * initialized with devm_v4l2_ctrl_handler_init().
+ */
+void devm_v4l2_ctrl_handler_free(struct device *dev,
+				 struct v4l2_ctrl_handler *hdl);
+
 /** v4l2_ctrl_handler_setup() - Call the s_ctrl op for all controls belonging
   * to the handler to initialize the hardware to the current control values.
   * @hdl:	The control handler.
-- 
1.7.10.4

[PATCH RFC 3/3] media: added managed v4l2 subdevice initialization

[Andrzej Hajda]

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(-)

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.
 
-- 
1.7.10.4

Re: [PATCH RFC 2/3] media: added managed v4l2 control initialization

[Hans Verkuil]

On Thu May 9 2013 14:52:43 Andrzej Hajda wrote:
> This patch adds managed versions of initialization
> and cleanup functions for v4l2 control handler.

Looks good, but please add the header changes needed for this to this
patch instead of patch 3/3.

With that change you can add my:

Acked-by: Hans Verkuil <hans.verkuil@cisco.com>

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-ctrls.c |   48 ++++++++++++++++++++++++++++++++++
>  include/media/v4l2-ctrls.h           |   27 +++++++++++++++++++
>  2 files changed, 75 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> index 6b56d7b..1f16405 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> @@ -1411,6 +1411,54 @@ void v4l2_ctrl_handler_free(struct v4l2_ctrl_handler *hdl)
>  }
>  EXPORT_SYMBOL(v4l2_ctrl_handler_free);
>  
> +static void devm_v4l2_ctrl_handler_release(struct device *dev, void *res)
> +{
> +	struct v4l2_ctrl_handler **hdl = res;
> +
> +	v4l2_ctrl_handler_free(*hdl);
> +}
> +
> +int devm_v4l2_ctrl_handler_init(struct device *dev,
> +				struct v4l2_ctrl_handler *hdl,
> +				unsigned nr_of_controls_hint)
> +{
> +	struct v4l2_ctrl_handler **dr;
> +	int rc;
> +
> +	dr = devres_alloc(devm_v4l2_ctrl_handler_release, sizeof(*dr),
> +			  GFP_KERNEL);
> +	if (!dr)
> +		return -ENOMEM;
> +
> +	rc = v4l2_ctrl_handler_init(hdl, nr_of_controls_hint);
> +	if (rc) {
> +		devres_free(dr);
> +		return rc;
> +	}
> +
> +	*dr = hdl;
> +	devres_add(dev, dr);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(devm_v4l2_ctrl_handler_init);
> +
> +static int devm_v4l2_ctrl_handler_match(struct device *dev, void *res,
> +					void *data)
> +{
> +	struct v4l2_ctrl_handler **this = res, **hdl = data;
> +
> +	return *this == *hdl;
> +}
> +
> +void devm_v4l2_ctrl_handler_free(struct device *dev,
> +				 struct v4l2_ctrl_handler *hdl)
> +{
> +	WARN_ON(devres_release(dev, devm_v4l2_ctrl_handler_release,
> +			       devm_v4l2_ctrl_handler_match, &hdl));
> +}
> +EXPORT_SYMBOL_GPL(devm_v4l2_ctrl_handler_free);
> +
>  /* For backwards compatibility: V4L2_CID_PRIVATE_BASE should no longer
>     be used except in G_CTRL, S_CTRL, QUERYCTRL and QUERYMENU when dealing
>     with applications that do not use the NEXT_CTRL flag.
> diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
> index f00d42b..a1d06db 100644
> --- a/include/media/v4l2-ctrls.h
> +++ b/include/media/v4l2-ctrls.h
> @@ -283,6 +283,33 @@ int v4l2_ctrl_handler_init(struct v4l2_ctrl_handler *hdl,
>    */
>  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.
> + *
> + * This is a managed version of v4l2_ctrl_handler_init. Handler initialized with
> + * this function will be automatically cleaned up on driver detach.
> + *
> + * If an handler initialized with this function needs to be cleaned up
> + * separately, devm_v4l2_ctrl_handler_free() must be used.
> + */
> +int devm_v4l2_ctrl_handler_init(struct device *dev,
> +				struct v4l2_ctrl_handler *hdl,
> +				unsigned nr_of_controls_hint);
> +
> +/**
> + * devm_v4l2_ctrl_handler_free - managed control handler free
> + *
> + * @dev: Device for which @hdl belongs to.
> + * @hdl: Handler to be cleaned up.
> + *
> + * This function should be used to manual free of an control handler
> + * initialized with devm_v4l2_ctrl_handler_init().
> + */
> +void devm_v4l2_ctrl_handler_free(struct device *dev,
> +				 struct v4l2_ctrl_handler *hdl);
> +
>  /** v4l2_ctrl_handler_setup() - Call the s_ctrl op for all controls belonging
>    * to the handler to initialize the hardware to the current control values.
>    * @hdl:	The control handler.
> 

Re: [PATCH RFC 3/3] media: added managed v4l2 subdevice initialization

[Hans Verkuil]

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.
>  
> 

Re: [PATCH RFC 3/3] media: added managed v4l2 subdevice initialization

[Andrzej Hajda]

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.
>>  
>>