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

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

[Andrzej Hajda]

This is the 2nd version of managed initializations for media/v4l2.
There are small changes documented in separate patches.

Additionally to advertise this solution I suggest to look at all *_remove
functions in drivers/media/i2c/ tree. After conversion to devm_* versions
most of the *_remove routines could be removed completely.
Below grep for showing all *_remove functions from drivers/media/i2c:
grep -rPzo "(?s)^(\s*)\N*_remove.*?{.*?^\1}" drivers/media/i2c/ --include='*.c'

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          |    6 +++
 include/media/v4l2-common.h           |    2 +
 include/media/v4l2-ctrls.h            |   31 +++++++++++++++
 include/media/v4l2-subdev.h           |    5 +++
 8 files changed, 224 insertions(+)

-- 
1.7.10.4

[PATCH RFC v2 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>
---
v2:
	- added missing struct device forward declaration
---
 drivers/media/media-entity.c |   70 ++++++++++++++++++++++++++++++++++++++++++
 include/media/media-entity.h |    6 ++++
 2 files changed, 76 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..e3f1604 100644
--- a/include/media/media-entity.h
+++ b/include/media/media-entity.h
@@ -26,6 +26,8 @@
 #include <linux/list.h>
 #include <linux/media.h>
 
+struct device;
+
 struct media_pipeline {
 };
 
@@ -126,6 +128,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 v2 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>
---
v2:
	- added missing struct device forward declaration,
	- corrected few comments
---
 drivers/media/v4l2-core/v4l2-ctrls.c |   48 ++++++++++++++++++++++++++++++++++
 include/media/v4l2-ctrls.h           |   31 ++++++++++++++++++++++
 2 files changed, 79 insertions(+)

diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
index ebb8e48..69c9b95 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -1421,6 +1421,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 7343a27..1986b90 100644
--- a/include/media/v4l2-ctrls.h
+++ b/include/media/v4l2-ctrls.h
@@ -25,6 +25,7 @@
 #include <linux/videodev2.h>
 
 /* forward references */
+struct device;
 struct file;
 struct v4l2_ctrl_handler;
 struct v4l2_ctrl_helper;
@@ -306,6 +307,36 @@ int v4l2_ctrl_handler_init_class(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 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.
+ *
+ * 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 the @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 v2 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>
---
v2:
	- changes of v4l2-ctrls.h moved to proper patch
---
 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-subdev.h           |    5 ++++
 4 files changed, 69 insertions(+)

diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c
index 3fed63f..96aac931 100644
--- a/drivers/media/v4l2-core/v4l2-common.c
+++ b/drivers/media/v4l2-core/v4l2-common.c
@@ -301,7 +301,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 1d93c48..da62e2b 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-subdev.h b/include/media/v4l2-subdev.h
index 5298d67..881abdd 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -657,6 +657,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 v2 3/3] media: added managed v4l2 subdevice initialization

[Hans Verkuil]

Hi Andrzej,

On Mon May 13 2013 10:34:46 Andrzej Hajda wrote:
> This patch adds managed versions of initialization
> functions for v4l2 subdevices.

I figured out what is bothering me about this patch: the fact that it is
tied to the v4l2_i2c_subdev_init/v4l2_subdev_init functions. Normally devm
functions are wrappers around functions that actually allocate some resource.
That's not the case with these subdev_init functions, they just initialize
fields in a struct.

Why not drop those wrappers and just provide the devm_v4l2_subdev_bind
function? That's actually the one that is doing the binding, and is a function
drivers can call explicitly.

The only thing you need to add to devm_v4l2_subdev_bind is a WARN_ON check that
sd->ops != NULL, verifying that v4l2_subdev_init was called before the
bind().

I would be much happier with that solution.

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>
> ---
> v2:
> 	- changes of v4l2-ctrls.h moved to proper patch
> ---
>  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-subdev.h           |    5 ++++
>  4 files changed, 69 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c
> index 3fed63f..96aac931 100644
> --- a/drivers/media/v4l2-core/v4l2-common.c
> +++ b/drivers/media/v4l2-core/v4l2-common.c
> @@ -301,7 +301,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 1d93c48..da62e2b 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-subdev.h b/include/media/v4l2-subdev.h
> index 5298d67..881abdd 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -657,6 +657,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 v2 3/3] media: added managed v4l2 subdevice initialization

[Andrzej Hajda]

On 13.05.2013 11:24, Hans Verkuil wrote:
> Hi Andrzej,
>
> On Mon May 13 2013 10:34:46 Andrzej Hajda wrote:
>> This patch adds managed versions of initialization
>> functions for v4l2 subdevices.
> I figured out what is bothering me about this patch: the fact that it is
> tied to the v4l2_i2c_subdev_init/v4l2_subdev_init functions. Normally devm
> functions are wrappers around functions that actually allocate some resource.
> That's not the case with these subdev_init functions, they just initialize
> fields in a struct.
clk_get do not perform any allocation for now, there is only requirement(?)
that it should be paired with clk_put, and that is what devm_clk_get
actually does.

>
> Why not drop those wrappers and just provide the devm_v4l2_subdev_bind
> function? That's actually the one that is doing the binding, and is a function
> drivers can call explicitly.

struct v4l2_subdev does not need allocation on initialization,
but an 'allocation' (subdev registration) usually happens during its
lifetime and is performed from outside (from v4l2 device), in fact it can
happen multiple times.
On v4l2_subdev de-initialization/cleanup/destruction we should ensure
it is not registered and eventually unregister it and all this is performed
by v4l2_device_unregister_subdev. So v4l2_device_unregister_subdev function
seems to be natural cleanup routine for v4l2_subdev and it is used this
way in most of the drivers.

All this lengthly explanation is to show that devm_v4l2*subdev_init just
initializes v4l2_subdev in a managed way - besides fields initialization
it adds automatic cleanup routine.
>
> The only thing you need to add to devm_v4l2_subdev_bind is a WARN_ON check that
> sd->ops != NULL, verifying that v4l2_subdev_init was called before the
> bind().
OK.
>
> I would be much happier with that solution.
I hope my reasoning above will convince you, if not I will adjust the
patch accordingly.

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>
>> ---
>> v2:
>> 	- changes of v4l2-ctrls.h moved to proper patch
>> ---
>>  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-subdev.h           |    5 ++++
>>  4 files changed, 69 insertions(+)
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c
>> index 3fed63f..96aac931 100644
>> --- a/drivers/media/v4l2-core/v4l2-common.c
>> +++ b/drivers/media/v4l2-core/v4l2-common.c
>> @@ -301,7 +301,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 1d93c48..da62e2b 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-subdev.h b/include/media/v4l2-subdev.h
>> index 5298d67..881abdd 100644
>> --- a/include/media/v4l2-subdev.h
>> +++ b/include/media/v4l2-subdev.h
>> @@ -657,6 +657,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 v2 2/3] media: added managed v4l2 control initialization

[Laurent Pinchart]

Hi Andrzej,

Thank you for the patch.

On Monday 13 May 2013 10:34:45 Andrzej Hajda wrote:
> 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>
> ---
> v2:
> 	- added missing struct device forward declaration,
> 	- corrected few comments
> ---
>  drivers/media/v4l2-core/v4l2-ctrls.c |   48 +++++++++++++++++++++++++++++++
>  include/media/v4l2-ctrls.h           |   31 ++++++++++++++++++++++
>  2 files changed, 79 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c
> b/drivers/media/v4l2-core/v4l2-ctrls.c index ebb8e48..69c9b95 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> @@ -1421,6 +1421,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);

I expect very few drivers to actually need devm_v4l2_ctrl_handler_free(), if 
any at all. Do you have a use case for that function at the moment ? If not, 
what about removing it for now and adding it later when (if) needed ?

> +
>  /* 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 7343a27..1986b90 100644
> --- a/include/media/v4l2-ctrls.h
> +++ b/include/media/v4l2-ctrls.h
> @@ -25,6 +25,7 @@
>  #include <linux/videodev2.h>
> 
>  /* forward references */
> +struct device;
>  struct file;
>  struct v4l2_ctrl_handler;
>  struct v4l2_ctrl_helper;
> @@ -306,6 +307,36 @@ int v4l2_ctrl_handler_init_class(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 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. +
> *
> + * 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 the @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.
-- 
Regards,

Laurent Pinchart

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

[Hans Verkuil]

On Wed 15 May 2013 10:29:16 Andrzej Hajda wrote:
> On 13.05.2013 11:24, Hans Verkuil wrote:
> > Hi Andrzej,
> >
> > On Mon May 13 2013 10:34:46 Andrzej Hajda wrote:
> >> This patch adds managed versions of initialization
> >> functions for v4l2 subdevices.
> > I figured out what is bothering me about this patch: the fact that it is
> > tied to the v4l2_i2c_subdev_init/v4l2_subdev_init functions. Normally devm
> > functions are wrappers around functions that actually allocate some resource.
> > That's not the case with these subdev_init functions, they just initialize
> > fields in a struct.
> clk_get do not perform any allocation for now, there is only requirement(?)
> that it should be paired with clk_put, and that is what devm_clk_get
> actually does.
> 
> >
> > Why not drop those wrappers and just provide the devm_v4l2_subdev_bind
> > function? That's actually the one that is doing the binding, and is a function
> > drivers can call explicitly.
> 
> struct v4l2_subdev does not need allocation on initialization,
> but an 'allocation' (subdev registration) usually happens during its
> lifetime and is performed from outside (from v4l2 device), in fact it can
> happen multiple times.
> On v4l2_subdev de-initialization/cleanup/destruction we should ensure
> it is not registered and eventually unregister it and all this is performed
> by v4l2_device_unregister_subdev. So v4l2_device_unregister_subdev function
> seems to be natural cleanup routine for v4l2_subdev and it is used this
> way in most of the drivers.
> 
> All this lengthly explanation is to show that devm_v4l2*subdev_init just
> initializes v4l2_subdev in a managed way - besides fields initialization
> it adds automatic cleanup routine.
> >
> > The only thing you need to add to devm_v4l2_subdev_bind is a WARN_ON check that
> > sd->ops != NULL, verifying that v4l2_subdev_init was called before the
> > bind().
> OK.
> >
> > I would be much happier with that solution.
> I hope my reasoning above will convince you, if not I will adjust the
> patch accordingly.

You have convinced me only partially. v4l2_subdev_init should *not* be managed:
it really is just an initialization function and for non-i2c and non-spi subdevs
there is generally no cleanup needed (or it is done automatically when the
v4l2_device is unregistered).

For v4l2_i2c/spi_subdev_init it makes sense to manage that since there cleanup
is needed. Actually, to be precise: cleanup is only needed if the i2c adapter
is released before the v4l2_device is unregistered. In general the order is
unknown, so cleanup is needed.

Regards,

	Hans

> 
> 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>
> >> ---
> >> v2:
> >> 	- changes of v4l2-ctrls.h moved to proper patch
> >> ---
> >>  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-subdev.h           |    5 ++++
> >>  4 files changed, 69 insertions(+)
> >>
> >> diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c
> >> index 3fed63f..96aac931 100644
> >> --- a/drivers/media/v4l2-core/v4l2-common.c
> >> +++ b/drivers/media/v4l2-core/v4l2-common.c
> >> @@ -301,7 +301,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 1d93c48..da62e2b 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-subdev.h b/include/media/v4l2-subdev.h
> >> index 5298d67..881abdd 100644
> >> --- a/include/media/v4l2-subdev.h
> >> +++ b/include/media/v4l2-subdev.h
> >> @@ -657,6 +657,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
>