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

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

[Andrzej Hajda]

This is the 3rd version of managed initializations for media/v4l2.
Patches are adjusted according to Hans and Laurent suggestions.
Details of changes documented in individual patches.

*** BLURB HERE ***

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

 drivers/media/media-entity.c          |   44 +++++++++++++++++++++++++++++++++
 drivers/media/v4l2-core/v4l2-common.c |   10 ++++++++
 drivers/media/v4l2-core/v4l2-ctrls.c  |   32 ++++++++++++++++++++++++
 drivers/media/v4l2-core/v4l2-subdev.c |   25 +++++++++++++++++++
 include/media/media-entity.h          |    5 ++++
 include/media/v4l2-common.h           |    2 ++
 include/media/v4l2-ctrls.h            |   16 ++++++++++++
 include/media/v4l2-subdev.h           |    2 ++
 8 files changed, 136 insertions(+)

-- 
1.7.10.4

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

[Andrzej Hajda]

This patch adds managed versions of initialization
function 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>
---
v3:
	- removed managed cleanup
---
 drivers/media/media-entity.c |   44 ++++++++++++++++++++++++++++++++++++++++++
 include/media/media-entity.h |    5 +++++
 2 files changed, 49 insertions(+)

diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
index e1cd132..b1e29a7 100644
--- a/drivers/media/media-entity.c
+++ b/drivers/media/media-entity.c
@@ -82,9 +82,53 @@ 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.
+ */
+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);
+
 /* -----------------------------------------------------------------------------
  * Graph traversal
  */
diff --git a/include/media/media-entity.h b/include/media/media-entity.h
index 0c16f51..e25730e 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,9 @@ 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);
+
 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

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

[Laurent Pinchart]

Hi Andrzej,

Thank you for the patch.

On Thursday 16 May 2013 10:14:32 Andrzej Hajda wrote:
> This patch adds managed versions of initialization
> function 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>
> ---
> v3:
> 	- removed managed cleanup
> ---
>  drivers/media/media-entity.c |   44 +++++++++++++++++++++++++++++++++++++++
>  include/media/media-entity.h |    5 +++++
>  2 files changed, 49 insertions(+)
> 
> diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
> index e1cd132..b1e29a7 100644
> --- a/drivers/media/media-entity.c
> +++ b/drivers/media/media-entity.c
> @@ -82,9 +82,53 @@ 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.
> + */
> +int
> +devm_media_entity_init(struct device *dev, struct media_entity *entity,
> +		       u16 num_pads, struct media_pad *pads, u16 extra_links)

What kind of users do you see for this function ? Aren't subdev drivers 
supposed to use the devm_* functions from patch 3/3 instead ? We should at 
least make it clear in the documentation that drivers must not use both 
devm_media_entity_init() and devm_v4l2_subdev_i2c_init().

> +{
> +	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);
> +
>  /*
> ---------------------------------------------------------------------------
> -- * Graph traversal
>   */
> diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> index 0c16f51..e25730e 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,9 @@ 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);
> +
>  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);
-- 
Regards,

Laurent Pinchart

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

[Andrzej Hajda]

On 06/19/2013 12:03 AM, Laurent Pinchart wrote:
> Hi Andrzej,
>
> Thank you for the patch.
>
> On Thursday 16 May 2013 10:14:32 Andrzej Hajda wrote:
>> This patch adds managed versions of initialization
>> function 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>
>> ---
>> v3:
>> 	- removed managed cleanup
>> ---
>>  drivers/media/media-entity.c |   44 +++++++++++++++++++++++++++++++++++++++
>>  include/media/media-entity.h |    5 +++++
>>  2 files changed, 49 insertions(+)
>>
>> diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
>> index e1cd132..b1e29a7 100644
>> --- a/drivers/media/media-entity.c
>> +++ b/drivers/media/media-entity.c
>> @@ -82,9 +82,53 @@ 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.
>> + */
>> +int
>> +devm_media_entity_init(struct device *dev, struct media_entity *entity,
>> +		       u16 num_pads, struct media_pad *pads, u16 extra_links)
> What kind of users do you see for this function ? Aren't subdev drivers 
> supposed to use the devm_* functions from patch 3/3 instead ? We should at 
> least make it clear in the documentation that drivers must not use both 
> devm_media_entity_init() and devm_v4l2_subdev_i2c_init().
It can be used for media entities which are not part of subdev.
I will add statement about it.
Besides subdev, for now only video_device uses media entity.
I am not 100% sure about advantages of adding devm_media_entity_init -
currently only 7 drivers in kernel uses video_device and
media_entity_cleanup in those drivers is called not as straightforward
as in subdevs.
Replacing it with managed version would require deeper analysis :)
>
>> +{
>> +	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);
>> +
>>  /*
>> ---------------------------------------------------------------------------
>> -- * Graph traversal
>>   */
>> diff --git a/include/media/media-entity.h b/include/media/media-entity.h
>> index 0c16f51..e25730e 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,9 @@ 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);
>> +
>>  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);

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

[Laurent Pinchart]

Hi Andrzej,

On Wednesday 19 June 2013 12:33:11 Andrzej Hajda wrote:
> On 06/19/2013 12:03 AM, Laurent Pinchart wrote:
> > On Thursday 16 May 2013 10:14:32 Andrzej Hajda wrote:
> >> This patch adds managed versions of initialization
> >> function 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>
> >> ---
> >> 
> >> v3:
> >> 	- removed managed cleanup
> >> 
> >> ---
> >> 
> >>  drivers/media/media-entity.c |   44 ++++++++++++++++++++++++++++++++++++
> >>  include/media/media-entity.h |    5 +++++
> >>  2 files changed, 49 insertions(+)
> >> 
> >> diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
> >> index e1cd132..b1e29a7 100644
> >> --- a/drivers/media/media-entity.c
> >> +++ b/drivers/media/media-entity.c
> >> @@ -82,9 +82,53 @@ 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.
> >> + */
> >> +int
> >> +devm_media_entity_init(struct device *dev, struct media_entity *entity,
> >> +		       u16 num_pads, struct media_pad *pads, u16 extra_links)
> > 
> > What kind of users do you see for this function ? Aren't subdev drivers
> > supposed to use the devm_* functions from patch 3/3 instead ? We should at
> > least make it clear in the documentation that drivers must not use both
> > devm_media_entity_init() and devm_v4l2_subdev_i2c_init().
> 
> It can be used for media entities which are not part of subdev.
> I will add statement about it.
> Besides subdev, for now only video_device uses media entity.
> I am not 100% sure about advantages of adding devm_media_entity_init -
> currently only 7 drivers in kernel uses video_device and
> media_entity_cleanup in those drivers is called not as straightforward
> as in subdevs.
> Replacing it with managed version would require deeper analysis :)

Thank you fhr the explanation. Maybe we should then delay introducing 
devm_media_entity_init until needed ?

-- 
Regards,

Laurent Pinchart

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

[Andrzej Hajda]

On 06/19/2013 12:36 PM, Laurent Pinchart wrote:
> Hi Andrzej,
>
> On Wednesday 19 June 2013 12:33:11 Andrzej Hajda wrote:
>> On 06/19/2013 12:03 AM, Laurent Pinchart wrote:
>>> On Thursday 16 May 2013 10:14:32 Andrzej Hajda wrote:
>>>> This patch adds managed versions of initialization
>>>> function 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>
>>>> ---
>>>>
>>>> v3:
>>>> 	- removed managed cleanup
>>>>
>>>> ---
>>>>
>>>>  drivers/media/media-entity.c |   44 ++++++++++++++++++++++++++++++++++++
>>>>  include/media/media-entity.h |    5 +++++
>>>>  2 files changed, 49 insertions(+)
>>>>
>>>> diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
>>>> index e1cd132..b1e29a7 100644
>>>> --- a/drivers/media/media-entity.c
>>>> +++ b/drivers/media/media-entity.c
>>>> @@ -82,9 +82,53 @@ 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.
>>>> + */
>>>> +int
>>>> +devm_media_entity_init(struct device *dev, struct media_entity *entity,
>>>> +		       u16 num_pads, struct media_pad *pads, u16 extra_links)
>>> What kind of users do you see for this function ? Aren't subdev drivers
>>> supposed to use the devm_* functions from patch 3/3 instead ? We should at
>>> least make it clear in the documentation that drivers must not use both
>>> devm_media_entity_init() and devm_v4l2_subdev_i2c_init().
>> It can be used for media entities which are not part of subdev.
>> I will add statement about it.
>> Besides subdev, for now only video_device uses media entity.
>> I am not 100% sure about advantages of adding devm_media_entity_init -
>> currently only 7 drivers in kernel uses video_device and
>> media_entity_cleanup in those drivers is called not as straightforward
>> as in subdevs.
>> Replacing it with managed version would require deeper analysis :)
> Thank you fhr the explanation. Maybe we should then delay introducing 
> devm_media_entity_init until needed ?
>
Yes, it can be delayed.

Regards
Andrzej

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

[Andrzej Hajda]

This patch adds managed version of initialization
function 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>
---
v3:
	- removed managed cleanup
v2:
	- added missing struct device forward declaration,
	- corrected few comments
---
 drivers/media/v4l2-core/v4l2-ctrls.c |   32 ++++++++++++++++++++++++++++++++
 include/media/v4l2-ctrls.h           |   16 ++++++++++++++++
 2 files changed, 48 insertions(+)

diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
index ebb8e48..f47ccfa 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -1421,6 +1421,38 @@ 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);
+
 /* 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..169443f 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,21 @@ 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.
+ */
+int devm_v4l2_ctrl_handler_init(struct device *dev,
+				struct v4l2_ctrl_handler *hdl,
+				unsigned nr_of_controls_hint);
+
 /** 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

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

[Sakari Ailus]

Hi Andrzej,

Thanks for the patchset!

On Thu, May 16, 2013 at 10:14:33AM +0200, Andrzej Hajda wrote:
> This patch adds managed version of initialization
> function 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>
> ---
> v3:
> 	- removed managed cleanup
> v2:
> 	- added missing struct device forward declaration,
> 	- corrected few comments
> ---
>  drivers/media/v4l2-core/v4l2-ctrls.c |   32 ++++++++++++++++++++++++++++++++
>  include/media/v4l2-ctrls.h           |   16 ++++++++++++++++
>  2 files changed, 48 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> index ebb8e48..f47ccfa 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> @@ -1421,6 +1421,38 @@ 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);

v4l2_ctrl_handler_free() acquires hdl->mutex which is independent of the
existence of hdl. By default hdl->lock is in the handler, but it may also be
elsewhere, e.g. in a driver-specific device struct such as struct
smiapp_sensor defined in drivers/media/i2c/smiapp/smiapp.h. I wonder if
anything guarantees that hdl->mutex still exists at the time the device is
removed.

I have to say I don't think it's neither meaningful to acquire that mutex in
v4l2_ctrl_handler_free(), though, since the whole going to be freed next
anyway: reference counting would be needed to prevent bad things from
happening, in case the drivers wouldn't take care of that.

> +}
> +

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

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

[Andrzej Hajda]

Hi Sakari,

Thanks for the review.


On 17.05.2013 00:34, Sakari Ailus wrote:
> Hi Andrzej,
>
> Thanks for the patchset!
>
> On Thu, May 16, 2013 at 10:14:33AM +0200, Andrzej Hajda wrote:
>> This patch adds managed version of initialization
>> function 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>
>> ---
>> v3:
>> 	- removed managed cleanup
>> v2:
>> 	- added missing struct device forward declaration,
>> 	- corrected few comments
>> ---
>>  drivers/media/v4l2-core/v4l2-ctrls.c |   32 ++++++++++++++++++++++++++++++++
>>  include/media/v4l2-ctrls.h           |   16 ++++++++++++++++
>>  2 files changed, 48 insertions(+)
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
>> index ebb8e48..f47ccfa 100644
>> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
>> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
>> @@ -1421,6 +1421,38 @@ 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);
> v4l2_ctrl_handler_free() acquires hdl->mutex which is independent of the
> existence of hdl. By default hdl->lock is in the handler, but it may also be
> elsewhere, e.g. in a driver-specific device struct such as struct
> smiapp_sensor defined in drivers/media/i2c/smiapp/smiapp.h. I wonder if
> anything guarantees that hdl->mutex still exists at the time the device is
> removed.
IMO in general if somebody provides custom mutex he becomes responsible
for validity of that mutex during v4l2_ctrl_handler usage.
In the particular case of smiapp if we replace v4l2_ctrl_handler_init with
devm version and remove v4l2_ctrl_handler_free calls it seems to be OK:
- mutex is in devm allocated memory chunk acquired at the beginning of
probe,
- mutex is initialized before devm_v4l2_ctrl_handler_init,
- there is no mutex_destroy call - ie the mutex is valid until the
memory is freed,
- memory free is called after v4l2_ctrl_handler_free - devm
'destructors' are
called in order reverse to devm_* 'constructor' calls.

Anyway in cases when devm_* usage would cause errors
we can still use non devm_* versions.
>
> I have to say I don't think it's neither meaningful to acquire that mutex in
> v4l2_ctrl_handler_free(), though, since the whole going to be freed next
> anyway: reference counting would be needed to prevent bad things from
> happening, in case the drivers wouldn't take care of that.
I do not understand what do you mean exactly. Could you please explain
it more?
What do you want to reference count?


Regards
Andrzej
>
>> +}
>> +

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

[Sakari Ailus]

Hi Andrzej,

Andrzej Hajda wrote:
>> I have to say I don't think it's neither meaningful to acquire that mutex in
>> v4l2_ctrl_handler_free(), though, since the whole going to be freed next
>> anyway: reference counting would be needed to prevent bad things from
>> happening, in case the drivers wouldn't take care of that.
> I do not understand what do you mean exactly. Could you please explain
> it more?
> What do you want to reference count?

I don't want to, but simply acquiring a lock which is a part of an 
object being destroyed makes no sense. If something else has a reference 
to the object you're screwed anyway; acquiring the lock does not help.

-- 
Sakari Ailus
sakari.ailus@iki.fi

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

[Hans Verkuil]

On Fri 17 May 2013 00:34:51 Sakari Ailus wrote:
> Hi Andrzej,
> 
> Thanks for the patchset!
> 
> On Thu, May 16, 2013 at 10:14:33AM +0200, Andrzej Hajda wrote:
> > This patch adds managed version of initialization
> > function 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>
> > ---
> > v3:
> > 	- removed managed cleanup
> > v2:
> > 	- added missing struct device forward declaration,
> > 	- corrected few comments
> > ---
> >  drivers/media/v4l2-core/v4l2-ctrls.c |   32 ++++++++++++++++++++++++++++++++
> >  include/media/v4l2-ctrls.h           |   16 ++++++++++++++++
> >  2 files changed, 48 insertions(+)
> > 
> > diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> > index ebb8e48..f47ccfa 100644
> > --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> > +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> > @@ -1421,6 +1421,38 @@ 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);
> 
> v4l2_ctrl_handler_free() acquires hdl->mutex which is independent of the
> existence of hdl. By default hdl->lock is in the handler, but it may also be
> elsewhere, e.g. in a driver-specific device struct such as struct
> smiapp_sensor defined in drivers/media/i2c/smiapp/smiapp.h. I wonder if
> anything guarantees that hdl->mutex still exists at the time the device is
> removed.

If it is a driver-managed lock, then the driver should also be responsible for
that lock during the life-time of the control handler. I think that is a fair
assumption.

> I have to say I don't think it's neither meaningful to acquire that mutex in
> v4l2_ctrl_handler_free(), though, since the whole going to be freed next
> anyway: reference counting would be needed to prevent bad things from
> happening, in case the drivers wouldn't take care of that.

It's probably not meaningful today, but it might become meaningful in the
future. And in any case, not taking the lock when manipulating internal
lists is very unexpected even though it might work with today's use cases.

Regards,

	Hans

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

[Sakari Ailus]

Hi Hans,

Hans Verkuil wrote:
> On Fri 17 May 2013 00:34:51 Sakari Ailus wrote:
>> Hi Andrzej,
>>
>> Thanks for the patchset!
>>
>> On Thu, May 16, 2013 at 10:14:33AM +0200, Andrzej Hajda wrote:
>>> This patch adds managed version of initialization
>>> function 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>
>>> ---
>>> v3:
>>> 	- removed managed cleanup
>>> v2:
>>> 	- added missing struct device forward declaration,
>>> 	- corrected few comments
>>> ---
>>>   drivers/media/v4l2-core/v4l2-ctrls.c |   32 ++++++++++++++++++++++++++++++++
>>>   include/media/v4l2-ctrls.h           |   16 ++++++++++++++++
>>>   2 files changed, 48 insertions(+)
>>>
>>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
>>> index ebb8e48..f47ccfa 100644
>>> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
>>> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
>>> @@ -1421,6 +1421,38 @@ 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);
>>
>> v4l2_ctrl_handler_free() acquires hdl->mutex which is independent of the
>> existence of hdl. By default hdl->lock is in the handler, but it may also be
>> elsewhere, e.g. in a driver-specific device struct such as struct
>> smiapp_sensor defined in drivers/media/i2c/smiapp/smiapp.h. I wonder if
>> anything guarantees that hdl->mutex still exists at the time the device is
>> removed.
>
> If it is a driver-managed lock, then the driver should also be responsible for
> that lock during the life-time of the control handler. I think that is a fair
> assumption.

Agreed.

>> I have to say I don't think it's neither meaningful to acquire that mutex in
>> v4l2_ctrl_handler_free(), though, since the whole going to be freed next
>> anyway: reference counting would be needed to prevent bad things from
>> happening, in case the drivers wouldn't take care of that.
>
> It's probably not meaningful today, but it might become meaningful in the
> future. And in any case, not taking the lock when manipulating internal
> lists is very unexpected even though it might work with today's use cases.

I simply don't think it's meaningful to acquire a lock related to an 
object when that object is being destroyed. If something else was 
holding that lock, you should not have begun destroying that object in 
the first place. This could be solved by reference counting the handler 
which I don't think is needed.

I'd just shout out loud about this rather than hiding such potential 
bug, i.e. replace mutex_lock() and mutex_unlock() in the function by 
WARN_ON(mutex_is_lock()).

But that should be a separate patch.

-- 
Sakari Ailus
sakari.ailus@iki.fi

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

[Hans Verkuil]

On Fri May 31 2013 03:08:33 Sakari Ailus wrote:
> Hi Hans,
> 
> Hans Verkuil wrote:
> > On Fri 17 May 2013 00:34:51 Sakari Ailus wrote:
> >> Hi Andrzej,
> >>
> >> Thanks for the patchset!
> >>
> >> On Thu, May 16, 2013 at 10:14:33AM +0200, Andrzej Hajda wrote:
> >>> This patch adds managed version of initialization
> >>> function 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>
> >>> ---
> >>> v3:
> >>> 	- removed managed cleanup
> >>> v2:
> >>> 	- added missing struct device forward declaration,
> >>> 	- corrected few comments
> >>> ---
> >>>   drivers/media/v4l2-core/v4l2-ctrls.c |   32 ++++++++++++++++++++++++++++++++
> >>>   include/media/v4l2-ctrls.h           |   16 ++++++++++++++++
> >>>   2 files changed, 48 insertions(+)
> >>>
> >>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> >>> index ebb8e48..f47ccfa 100644
> >>> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> >>> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> >>> @@ -1421,6 +1421,38 @@ 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);
> >>
> >> v4l2_ctrl_handler_free() acquires hdl->mutex which is independent of the
> >> existence of hdl. By default hdl->lock is in the handler, but it may also be
> >> elsewhere, e.g. in a driver-specific device struct such as struct
> >> smiapp_sensor defined in drivers/media/i2c/smiapp/smiapp.h. I wonder if
> >> anything guarantees that hdl->mutex still exists at the time the device is
> >> removed.
> >
> > If it is a driver-managed lock, then the driver should also be responsible for
> > that lock during the life-time of the control handler. I think that is a fair
> > assumption.
> 
> Agreed.
> 
> >> I have to say I don't think it's neither meaningful to acquire that mutex in
> >> v4l2_ctrl_handler_free(), though, since the whole going to be freed next
> >> anyway: reference counting would be needed to prevent bad things from
> >> happening, in case the drivers wouldn't take care of that.
> >
> > It's probably not meaningful today, but it might become meaningful in the
> > future. And in any case, not taking the lock when manipulating internal
> > lists is very unexpected even though it might work with today's use cases.
> 
> I simply don't think it's meaningful to acquire a lock related to an 
> object when that object is being destroyed. If something else was 
> holding that lock, you should not have begun destroying that object in 
> the first place. This could be solved by reference counting the handler 
> which I don't think is needed.

Right now the way controls are set up is very static, but in the future I
expect to see more dynamical behavior (I'm thinking of FPGAs supporting
partial reconfiguration). In cases like that it you do want to take the
lock preventing others from making modifications while the handler is
freed. I am well aware that much more work will have to be done if we want
to support such scenarios, but it is one reason why I would like to keep
the lock there.

Regards,

	Hans

> I'd just shout out loud about this rather than hiding such potential 
> bug, i.e. replace mutex_lock() and mutex_unlock() in the function by 
> WARN_ON(mutex_is_lock()).
> 
> But that should be a separate patch.
> 
> 

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

[Sakari Ailus]

Hi Andrzej and Hans,

On Fri, May 31, 2013 at 09:59:11AM +0200, Hans Verkuil wrote:
> On Fri May 31 2013 03:08:33 Sakari Ailus wrote:
> > Hi Hans,
> > 
> > Hans Verkuil wrote:
> > > On Fri 17 May 2013 00:34:51 Sakari Ailus wrote:
> > >> Hi Andrzej,
> > >>
> > >> Thanks for the patchset!
> > >>
> > >> On Thu, May 16, 2013 at 10:14:33AM +0200, Andrzej Hajda wrote:
> > >>> This patch adds managed version of initialization
> > >>> function 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>
> > >>> ---
> > >>> v3:
> > >>> 	- removed managed cleanup
> > >>> v2:
> > >>> 	- added missing struct device forward declaration,
> > >>> 	- corrected few comments
> > >>> ---
> > >>>   drivers/media/v4l2-core/v4l2-ctrls.c |   32 ++++++++++++++++++++++++++++++++
> > >>>   include/media/v4l2-ctrls.h           |   16 ++++++++++++++++
> > >>>   2 files changed, 48 insertions(+)
> > >>>
> > >>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> > >>> index ebb8e48..f47ccfa 100644
> > >>> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> > >>> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> > >>> @@ -1421,6 +1421,38 @@ 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);
> > >>
> > >> v4l2_ctrl_handler_free() acquires hdl->mutex which is independent of the
> > >> existence of hdl. By default hdl->lock is in the handler, but it may also be
> > >> elsewhere, e.g. in a driver-specific device struct such as struct
> > >> smiapp_sensor defined in drivers/media/i2c/smiapp/smiapp.h. I wonder if
> > >> anything guarantees that hdl->mutex still exists at the time the device is
> > >> removed.
> > >
> > > If it is a driver-managed lock, then the driver should also be responsible for
> > > that lock during the life-time of the control handler. I think that is a fair
> > > assumption.
> > 
> > Agreed.
> > 
> > >> I have to say I don't think it's neither meaningful to acquire that mutex in
> > >> v4l2_ctrl_handler_free(), though, since the whole going to be freed next
> > >> anyway: reference counting would be needed to prevent bad things from
> > >> happening, in case the drivers wouldn't take care of that.
> > >
> > > It's probably not meaningful today, but it might become meaningful in the
> > > future. And in any case, not taking the lock when manipulating internal
> > > lists is very unexpected even though it might work with today's use cases.
> > 
> > I simply don't think it's meaningful to acquire a lock related to an 
> > object when that object is being destroyed. If something else was 
> > holding that lock, you should not have begun destroying that object in 
> > the first place. This could be solved by reference counting the handler 
> > which I don't think is needed.
> 
> Right now the way controls are set up is very static, but in the future I
> expect to see more dynamical behavior (I'm thinking of FPGAs supporting
> partial reconfiguration). In cases like that it you do want to take the
> lock preventing others from making modifications while the handler is
> freed. I am well aware that much more work will have to be done if we want
> to support such scenarios, but it is one reason why I would like to keep
> the lock there.

I'm ok with that.

How about adding a note to the comment above devm_v4l2_ctrl_handler_init()
that the function cannot be used with drivers that wish to use their own
lock?

-- 
Regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

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

[Sylwester Nawrocki]

Hi Sakari,

On 06/06/2013 11:41 PM, Sakari Ailus wrote:
...
>>>>>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
>>>>>> index ebb8e48..f47ccfa 100644
>>>>>> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
>>>>>> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
>>>>>> @@ -1421,6 +1421,38 @@ 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);
>>>>>
>>>>> v4l2_ctrl_handler_free() acquires hdl->mutex which is independent of the
>>>>> existence of hdl. By default hdl->lock is in the handler, but it may also be
>>>>> elsewhere, e.g. in a driver-specific device struct such as struct
>>>>> smiapp_sensor defined in drivers/media/i2c/smiapp/smiapp.h. I wonder if
>>>>> anything guarantees that hdl->mutex still exists at the time the device is
>>>>> removed.
>>>>
>>>> If it is a driver-managed lock, then the driver should also be responsible for
>>>> that lock during the life-time of the control handler. I think that is a fair
>>>> assumption.
>>>
>>> Agreed.
>>>
>>>>> I have to say I don't think it's neither meaningful to acquire that mutex in
>>>>> v4l2_ctrl_handler_free(), though, since the whole going to be freed next
>>>>> anyway: reference counting would be needed to prevent bad things from
>>>>> happening, in case the drivers wouldn't take care of that.
>>>>
>>>> It's probably not meaningful today, but it might become meaningful in the
>>>> future. And in any case, not taking the lock when manipulating internal
>>>> lists is very unexpected even though it might work with today's use cases.
>>>
>>> I simply don't think it's meaningful to acquire a lock related to an
>>> object when that object is being destroyed. If something else was
>>> holding that lock, you should not have begun destroying that object in
>>> the first place. This could be solved by reference counting the handler
>>> which I don't think is needed.
>>
>> Right now the way controls are set up is very static, but in the future I
>> expect to see more dynamical behavior (I'm thinking of FPGAs supporting
>> partial reconfiguration). In cases like that it you do want to take the
>> lock preventing others from making modifications while the handler is
>> freed. I am well aware that much more work will have to be done if we want
>> to support such scenarios, but it is one reason why I would like to keep
>> the lock there.
>
> I'm ok with that.
>
> How about adding a note to the comment above devm_v4l2_ctrl_handler_init()
> that the function cannot be used with drivers that wish to use their own
> lock?

But wouldn't it be a false statement ? The driver can still control the
cleanup sequence by properly ordering the initialization sequence. So as
long as it is ensured the mutex is destroyed after the controls handler
(IOW, the mutex is created before the controls handler using the devm_*
API) everything should be fine, shouldn't it ?


Regards,
Sylwester

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

[Hans Verkuil]

On Sun June 9 2013 20:05:33 Sylwester Nawrocki wrote:
> Hi Sakari,
> 
> On 06/06/2013 11:41 PM, Sakari Ailus wrote:
> ...
> >>>>>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> >>>>>> index ebb8e48..f47ccfa 100644
> >>>>>> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> >>>>>> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> >>>>>> @@ -1421,6 +1421,38 @@ 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);
> >>>>>
> >>>>> v4l2_ctrl_handler_free() acquires hdl->mutex which is independent of the
> >>>>> existence of hdl. By default hdl->lock is in the handler, but it may also be
> >>>>> elsewhere, e.g. in a driver-specific device struct such as struct
> >>>>> smiapp_sensor defined in drivers/media/i2c/smiapp/smiapp.h. I wonder if
> >>>>> anything guarantees that hdl->mutex still exists at the time the device is
> >>>>> removed.
> >>>>
> >>>> If it is a driver-managed lock, then the driver should also be responsible for
> >>>> that lock during the life-time of the control handler. I think that is a fair
> >>>> assumption.
> >>>
> >>> Agreed.
> >>>
> >>>>> I have to say I don't think it's neither meaningful to acquire that mutex in
> >>>>> v4l2_ctrl_handler_free(), though, since the whole going to be freed next
> >>>>> anyway: reference counting would be needed to prevent bad things from
> >>>>> happening, in case the drivers wouldn't take care of that.
> >>>>
> >>>> It's probably not meaningful today, but it might become meaningful in the
> >>>> future. And in any case, not taking the lock when manipulating internal
> >>>> lists is very unexpected even though it might work with today's use cases.
> >>>
> >>> I simply don't think it's meaningful to acquire a lock related to an
> >>> object when that object is being destroyed. If something else was
> >>> holding that lock, you should not have begun destroying that object in
> >>> the first place. This could be solved by reference counting the handler
> >>> which I don't think is needed.
> >>
> >> Right now the way controls are set up is very static, but in the future I
> >> expect to see more dynamical behavior (I'm thinking of FPGAs supporting
> >> partial reconfiguration). In cases like that it you do want to take the
> >> lock preventing others from making modifications while the handler is
> >> freed. I am well aware that much more work will have to be done if we want
> >> to support such scenarios, but it is one reason why I would like to keep
> >> the lock there.
> >
> > I'm ok with that.
> >
> > How about adding a note to the comment above devm_v4l2_ctrl_handler_init()
> > that the function cannot be used with drivers that wish to use their own
> > lock?
> 
> But wouldn't it be a false statement ? The driver can still control the
> cleanup sequence by properly ordering the initialization sequence. So as
> long as it is ensured the mutex is destroyed after the controls handler
> (IOW, the mutex is created before the controls handler using the devm_*
> API) everything should be fine, shouldn't it ?

It should indeed. I really don't see the problem here.

Regards,

	Hans

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

[Hans Verkuil]

On Thu 16 May 2013 10:14:33 Andrzej Hajda wrote:
> This patch adds managed version of initialization
> function 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>

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

Regards,

	Hans

> ---
> v3:
> 	- removed managed cleanup
> v2:
> 	- added missing struct device forward declaration,
> 	- corrected few comments
> ---
>  drivers/media/v4l2-core/v4l2-ctrls.c |   32 ++++++++++++++++++++++++++++++++
>  include/media/v4l2-ctrls.h           |   16 ++++++++++++++++
>  2 files changed, 48 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> index ebb8e48..f47ccfa 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> @@ -1421,6 +1421,38 @@ 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);
> +
>  /* 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..169443f 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,21 @@ 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.
> + */
> +int devm_v4l2_ctrl_handler_init(struct device *dev,
> +				struct v4l2_ctrl_handler *hdl,
> +				unsigned nr_of_controls_hint);
> +
>  /** 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 v3 2/3] media: added managed v4l2 control initialization

[Laurent Pinchart]

Hi Andrzej,

Thank you for the patch.

On Thursday 16 May 2013 10:14:33 Andrzej Hajda wrote:
> This patch adds managed version of initialization
> function 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>

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
> v3:
> 	- removed managed cleanup
> v2:
> 	- added missing struct device forward declaration,
> 	- corrected few comments
> ---
>  drivers/media/v4l2-core/v4l2-ctrls.c |   32 +++++++++++++++++++++++++++++++
>  include/media/v4l2-ctrls.h           |   16 ++++++++++++++++
>  2 files changed, 48 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c
> b/drivers/media/v4l2-core/v4l2-ctrls.c index ebb8e48..f47ccfa 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> @@ -1421,6 +1421,38 @@ 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);
> +
>  /* 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..169443f 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,21 @@ 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. +
> */
> +int devm_v4l2_ctrl_handler_init(struct device *dev,
> +				struct v4l2_ctrl_handler *hdl,
> +				unsigned nr_of_controls_hint);
> +
>  /** 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

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

[Andrzej Hajda]

This patch adds managed version of initialization
function for v4l2/i2c 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>
---
v3:
	- removed devm_v4l2_subdev_(init|free),
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 |   25 +++++++++++++++++++++++++
 include/media/v4l2-common.h           |    2 ++
 include/media/v4l2-subdev.h           |    2 ++
 4 files changed, 39 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..d79ee22 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -474,3 +474,28 @@ 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);
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..e086cfe 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -657,6 +657,8 @@ 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);
+
 /* Call an ops of a v4l2_subdev, doing the right checks against
    NULL pointers.
 
-- 
1.7.10.4

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

[Hans Verkuil]

On Thu 16 May 2013 10:14:34 Andrzej Hajda wrote:
> This patch adds managed version of initialization
> function for v4l2/i2c 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>

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

Regards,

	Hans

> ---
> v3:
> 	- removed devm_v4l2_subdev_(init|free),
> 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 |   25 +++++++++++++++++++++++++
>  include/media/v4l2-common.h           |    2 ++
>  include/media/v4l2-subdev.h           |    2 ++
>  4 files changed, 39 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..d79ee22 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -474,3 +474,28 @@ 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);
> 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..e086cfe 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -657,6 +657,8 @@ 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);
> +
>  /* Call an ops of a v4l2_subdev, doing the right checks against
>     NULL pointers.
>  
> 

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

[Laurent Pinchart]

Hi Andrzej,

Thank you for the patch.

On Thursday 16 May 2013 10:14:34 Andrzej Hajda wrote:
> This patch adds managed version of initialization
> function for v4l2/i2c 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>
> ---
> v3:
> 	- removed devm_v4l2_subdev_(init|free),
> 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 |   25 +++++++++++++++++++++++++
>  include/media/v4l2-common.h           |    2 ++
>  include/media/v4l2-subdev.h           |    2 ++
>  4 files changed, 39 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..d79ee22 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -474,3 +474,28 @@ 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
> +}
> +

Could you add a brief kerneldoc comment here that explains the use cases of 
this function ? There's no v4l2_subdev_bind(), so it would probably not be 
straightforward for driver developers to know when to use this function.

Apart from that, the patch looks good.

> +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);
> 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..e086cfe 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -657,6 +657,8 @@ 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);
> +
>  /* Call an ops of a v4l2_subdev, doing the right checks against
>     NULL pointers.
-- 
Regards,

Laurent Pinchart

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

[Andrzej Hajda]

This patch adds managed version of initialization
function for v4l2 i2c 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>
---
v4:
	- added description to devm_v4l2_subdev_bind
v3:
	- removed devm_v4l2_subdev_(init|free),
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 | 35 +++++++++++++++++++++++++++++++++++
 include/media/v4l2-common.h           |  2 ++
 include/media/v4l2-subdev.h           |  2 ++
 4 files changed, 49 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..2242962 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -474,3 +474,38 @@ 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
+}
+
+/**
+ * devm_v4l2_subdev_bind - Add subdevice to device managed resource list
+ * @dev: Device to bind subdev to
+ * @sd:  Subdevice to bind
+ *
+ * Function adds device managed release code to the subdev.
+ * If the function succeedes then on driver detach subdev will be automatically
+ * unregistered and the media entity will be cleaned up. Function can be used
+ * with subdevs not initialized by devm_v4l2_i2c_subdev_init.
+ */
+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);
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..e086cfe 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -657,6 +657,8 @@ 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);
+
 /* Call an ops of a v4l2_subdev, doing the right checks against
    NULL pointers.
 
-- 
1.8.1.2

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

[Hans Verkuil]

This patch has been sitting around for quite some time now. Is there any reason
not to apply it?

Laurent, I see that these patches (part of the same patch set) are still pending
for you:

https://patchwork.linuxtv.org/patch/18447/
https://patchwork.linuxtv.org/patch/18449/

If you plan to apply those for 3.12, then it would make sense to apply this one
at the same time.

For this patch:

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

Regards,

	Hans

On Wed 19 June 2013 16:10:54 Andrzej Hajda wrote:
> This patch adds managed version of initialization
> function for v4l2 i2c 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>
> ---
> v4:
> 	- added description to devm_v4l2_subdev_bind
> v3:
> 	- removed devm_v4l2_subdev_(init|free),
> 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 | 35 +++++++++++++++++++++++++++++++++++
>  include/media/v4l2-common.h           |  2 ++
>  include/media/v4l2-subdev.h           |  2 ++
>  4 files changed, 49 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..2242962 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -474,3 +474,38 @@ 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
> +}
> +
> +/**
> + * devm_v4l2_subdev_bind - Add subdevice to device managed resource list
> + * @dev: Device to bind subdev to
> + * @sd:  Subdevice to bind
> + *
> + * Function adds device managed release code to the subdev.
> + * If the function succeedes then on driver detach subdev will be automatically
> + * unregistered and the media entity will be cleaned up. Function can be used
> + * with subdevs not initialized by devm_v4l2_i2c_subdev_init.
> + */
> +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);
> 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..e086cfe 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -657,6 +657,8 @@ 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);
> +
>  /* Call an ops of a v4l2_subdev, doing the right checks against
>     NULL pointers.
>  
> 

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

[Sylwester Nawrocki]

On 08/22/2013 01:10 PM, Hans Verkuil wrote:
> This patch has been sitting around for quite some time now. Is there any reason
> not to apply it?

We wanted to merge those patches together with some users of them.
We have already prepared relevant patches but those depend on other
ones (conversion to v4l2-async/DT, some pending review) and I didn't
find enough time to post everything. I won't find time to take care
of this for 3.12, sorry. I guess it could be postponed to 3.13.

--
Regards,
Sylwester