linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/3] iio: core: Add devm_ APIs for iio_channel_{get,release}
@ 2016-04-06 10:31 Laxman Dewangan
  2016-04-06 10:31 ` [PATCH 2/3] iio: core: Add devm_ APIs for iio_channel_{get,release}_all Laxman Dewangan
                   ` (4 more replies)
  0 siblings, 5 replies; 13+ messages in thread
From: Laxman Dewangan @ 2016-04-06 10:31 UTC (permalink / raw)
  To: jic23, corbet, knaack.h, lars, pmeerw
  Cc: linux-doc, linux-kernel, linux-iio, Laxman Dewangan

Some of kernel driver uses the IIO framework to get the sensor
value via ADC or IIO HW driver. The client driver get iio channel
by iio_channel_get() and release it by calling iio_channel_release().

Add resource managed version (devm_*) of these APIs so that if client
calls the devm_iio_channel_get() then it need not to release it explicitly,
it can be done by managed device framework when driver get un-binded.

This reduces the code in error path and also need of .remove callback in
some cases.

Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
---
 drivers/iio/inkern.c         | 48 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/iio/consumer.h | 27 +++++++++++++++++++++++++
 2 files changed, 75 insertions(+)

diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
index 734a004..18e623f 100644
--- a/drivers/iio/inkern.c
+++ b/drivers/iio/inkern.c
@@ -356,6 +356,54 @@ void iio_channel_release(struct iio_channel *channel)
 }
 EXPORT_SYMBOL_GPL(iio_channel_release);
 
+static void devm_iio_channel_free(struct device *dev, void *res)
+{
+	struct iio_channel *channel = *(struct iio_channel **)res;
+
+	iio_channel_release(channel);
+}
+
+static int devm_iio_channel_match(struct device *dev, void *res, void *data)
+{
+	struct iio_channel **r = res;
+
+	if (!r || !*r) {
+		WARN_ON(!r || !*r);
+		return 0;
+	}
+
+	return *r == data;
+}
+
+struct iio_channel *devm_iio_channel_get(struct device *dev,
+					 const char *channel_name)
+{
+	struct iio_channel **ptr, *channel;
+
+	ptr = devres_alloc(devm_iio_channel_free, sizeof(*ptr), GFP_KERNEL);
+	if (!ptr)
+		return ERR_PTR(-ENOMEM);
+
+	channel = iio_channel_get(dev, channel_name);
+	if (IS_ERR(channel)) {
+		devres_free(ptr);
+		return channel;
+	}
+
+	*ptr = channel;
+	devres_add(dev, ptr);
+
+	return channel;
+}
+EXPORT_SYMBOL_GPL(devm_iio_channel_get);
+
+void devm_iio_channel_release(struct device *dev, struct iio_channel *channel)
+{
+	WARN_ON(devres_release(dev, devm_iio_channel_free,
+			       devm_iio_channel_match, channel));
+}
+EXPORT_SYMBOL_GPL(devm_iio_channel_release);
+
 struct iio_channel *iio_channel_get_all(struct device *dev)
 {
 	const char *name;
diff --git a/include/linux/iio/consumer.h b/include/linux/iio/consumer.h
index fad5867..e1e033d 100644
--- a/include/linux/iio/consumer.h
+++ b/include/linux/iio/consumer.h
@@ -49,6 +49,33 @@ struct iio_channel *iio_channel_get(struct device *dev,
 void iio_channel_release(struct iio_channel *chan);
 
 /**
+ * devm_iio_channel_get() - Resource managed version of iio_channel_get().
+ * @dev:		Pointer to consumer device. Device name must match
+ *			the name of the device as provided in the iio_map
+ *			with which the desired provider to consumer mapping
+ *			was registered.
+ * @consumer_channel:	Unique name to identify the channel on the consumer
+ *			side. This typically describes the channels use within
+ *			the consumer. E.g. 'battery_voltage'
+ *
+ * Returns a pointer to negative errno if it is not able to get the iio channel
+ * otherwise returns valid pointer for iio channel.
+ *
+ * The allocated iio channel is automatically released when the device is
+ * unbound.
+ */
+struct iio_channel *devm_iio_channel_get(struct device *dev,
+					 const char *consumer_channel);
+/**
+ * devm_iio_channel_release() - Resource managed version of
+ *				iio_channel_release().
+ * @dev:		Pointer to consumer device for which resource
+ *			is allocared.
+ * @chan:		The channel to be released.
+ */
+void devm_iio_channel_release(struct device *dev, struct iio_channel *chan);
+
+/**
  * iio_channel_get_all() - get all channels associated with a client
  * @dev:		Pointer to consumer device.
  *
-- 
2.1.4

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 2/3] iio: core: Add devm_ APIs for iio_channel_{get,release}_all
  2016-04-06 10:31 [PATCH 1/3] iio: core: Add devm_ APIs for iio_channel_{get,release} Laxman Dewangan
@ 2016-04-06 10:31 ` Laxman Dewangan
  2016-04-17 12:06   ` Jonathan Cameron
  2016-04-06 10:31 ` [PATCH 3/3] iio: Add resource managed APIs devm_iio_channel_{get,release) in devres Laxman Dewangan
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 13+ messages in thread
From: Laxman Dewangan @ 2016-04-06 10:31 UTC (permalink / raw)
  To: jic23, corbet, knaack.h, lars, pmeerw
  Cc: linux-doc, linux-kernel, linux-iio, Laxman Dewangan

Some of kernel driver uses the IIO framework to get the sensor
value via ADC or IIO HW driver. The client driver get iio channel
by iio_channel_get_all() and release it by calling
iio_channel_release_all().

Add resource managed version (devm_*) of these APIs so that if client
calls the devm_iio_channel_get_all() then it need not to release it
explicitly, it can be done by managed device framework when driver
get un-binded.

This reduces the code in error path and also need of .remove callback in
some cases.

Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
---
 drivers/iio/inkern.c         | 36 ++++++++++++++++++++++++++++++++++++
 include/linux/iio/consumer.h | 26 ++++++++++++++++++++++++++
 2 files changed, 62 insertions(+)

diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
index 18e623f..8c1abfe 100644
--- a/drivers/iio/inkern.c
+++ b/drivers/iio/inkern.c
@@ -489,6 +489,42 @@ void iio_channel_release_all(struct iio_channel *channels)
 }
 EXPORT_SYMBOL_GPL(iio_channel_release_all);
 
+static void devm_iio_channel_free_all(struct device *dev, void *res)
+{
+	struct iio_channel *channels = *(struct iio_channel **)res;
+
+	iio_channel_release_all(channels);
+}
+
+struct iio_channel *devm_iio_channel_get_all(struct device *dev)
+{
+	struct iio_channel **ptr, *channels;
+
+	ptr = devres_alloc(devm_iio_channel_free_all, sizeof(*ptr), GFP_KERNEL);
+	if (!ptr)
+		return ERR_PTR(-ENOMEM);
+
+	channels = iio_channel_get_all(dev);
+	if (IS_ERR(channels)) {
+		devres_free(ptr);
+		return channels;
+	}
+
+	*ptr = channels;
+	devres_add(dev, ptr);
+
+	return channels;
+}
+EXPORT_SYMBOL_GPL(devm_iio_channel_get_all);
+
+void devm_iio_channel_release_all(struct device *dev,
+				  struct iio_channel *channels)
+{
+	WARN_ON(devres_release(dev, devm_iio_channel_free_all,
+			       devm_iio_channel_match, channels));
+}
+EXPORT_SYMBOL_GPL(devm_iio_channel_release_all);
+
 static int iio_channel_read(struct iio_channel *chan, int *val, int *val2,
 	enum iio_chan_info_enum info)
 {
diff --git a/include/linux/iio/consumer.h b/include/linux/iio/consumer.h
index e1e033d..3d672f7 100644
--- a/include/linux/iio/consumer.h
+++ b/include/linux/iio/consumer.h
@@ -92,6 +92,32 @@ struct iio_channel *iio_channel_get_all(struct device *dev);
  */
 void iio_channel_release_all(struct iio_channel *chan);
 
+/**
+ * devm_iio_channel_get_all() - Resource managed version of
+ *				iio_channel_get_all().
+ * @dev: Pointer to consumer device.
+ *
+ * Returns a pointer to negative errno if it is not able to get the iio channel
+ * otherwise returns an array of iio_channel structures terminated with one with
+ * null iio_dev pointer.
+ *
+ * This function is used by fairly generic consumers to get all the
+ * channels registered as having this consumer.
+ *
+ * The allocated iio channels are automatically released when the device is
+ * unbounded.
+ */
+struct iio_channel *devm_iio_channel_get_all(struct device *dev);
+
+/**
+ * devm_iio_channel_release_all() - Resource managed version of
+ *				    iio_channel_release_all().
+ * @dev:		Pointer to consumer device for which resource
+ *			is allocared.
+ * @chan:		Array channel to be released.
+ */
+void devm_iio_channel_release_all(struct device *dev, struct iio_channel *chan);
+
 struct iio_cb_buffer;
 /**
  * iio_channel_get_all_cb() - register callback for triggered capture
-- 
2.1.4

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 3/3] iio: Add resource managed APIs devm_iio_channel_{get,release) in devres
  2016-04-06 10:31 [PATCH 1/3] iio: core: Add devm_ APIs for iio_channel_{get,release} Laxman Dewangan
  2016-04-06 10:31 ` [PATCH 2/3] iio: core: Add devm_ APIs for iio_channel_{get,release}_all Laxman Dewangan
@ 2016-04-06 10:31 ` Laxman Dewangan
  2016-04-06 13:49 ` [PATCH 1/3] iio: core: Add devm_ APIs for iio_channel_{get,release} Daniel Baluta
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 13+ messages in thread
From: Laxman Dewangan @ 2016-04-06 10:31 UTC (permalink / raw)
  To: jic23, corbet, knaack.h, lars, pmeerw
  Cc: linux-doc, linux-kernel, linux-iio, Laxman Dewangan

Add following APIs in the list of managed resources of IIO:
	devm_iio_channel_get()
	devm_iio_channel_get_all()
	devm_iio_channel_release()
	devm_iio_channel_release_all()

Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
---
 Documentation/driver-model/devres.txt | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/driver-model/devres.txt b/Documentation/driver-model/devres.txt
index 73b98df..bdc2dfb 100644
--- a/Documentation/driver-model/devres.txt
+++ b/Documentation/driver-model/devres.txt
@@ -267,6 +267,10 @@ IIO
   devm_iio_kfifo_free()
   devm_iio_trigger_alloc()
   devm_iio_trigger_free()
+  devm_iio_channel_get()
+  devm_iio_channel_release()
+  devm_iio_channel_get_all()
+  devm_iio_channel_release_all()
 
 IO region
   devm_release_mem_region()
-- 
2.1.4

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/3] iio: core: Add devm_ APIs for iio_channel_{get,release}
  2016-04-06 10:31 [PATCH 1/3] iio: core: Add devm_ APIs for iio_channel_{get,release} Laxman Dewangan
  2016-04-06 10:31 ` [PATCH 2/3] iio: core: Add devm_ APIs for iio_channel_{get,release}_all Laxman Dewangan
  2016-04-06 10:31 ` [PATCH 3/3] iio: Add resource managed APIs devm_iio_channel_{get,release) in devres Laxman Dewangan
@ 2016-04-06 13:49 ` Daniel Baluta
  2016-04-06 14:58   ` Laxman Dewangan
  2016-04-10 14:08 ` Jonathan Cameron
  2016-04-17 12:00 ` Jonathan Cameron
  4 siblings, 1 reply; 13+ messages in thread
From: Daniel Baluta @ 2016-04-06 13:49 UTC (permalink / raw)
  To: Laxman Dewangan
  Cc: Jonathan Cameron, Jonathan Corbet, Hartmut Knaack,
	Lars-Peter Clausen, Peter Meerwald-Stadler, linux-doc,
	Linux Kernel Mailing List, linux-iio

On Wed, Apr 6, 2016 at 1:31 PM, Laxman Dewangan <ldewangan@nvidia.com> wrote:
> Some of kernel driver uses the IIO framework to get the sensor
> value via ADC or IIO HW driver. The client driver get iio channel
> by iio_channel_get() and release it by calling iio_channel_release().
>
> Add resource managed version (devm_*) of these APIs so that if client
> calls the devm_iio_channel_get() then it need not to release it explicitly,
> it can be done by managed device framework when driver get un-binded.
>
> This reduces the code in error path and also need of .remove callback in
> some cases.
>

Please provide at least one example of code that uses this API.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/3] iio: core: Add devm_ APIs for iio_channel_{get,release}
  2016-04-06 13:49 ` [PATCH 1/3] iio: core: Add devm_ APIs for iio_channel_{get,release} Daniel Baluta
@ 2016-04-06 14:58   ` Laxman Dewangan
  2016-04-10 14:05     ` Jonathan Cameron
  0 siblings, 1 reply; 13+ messages in thread
From: Laxman Dewangan @ 2016-04-06 14:58 UTC (permalink / raw)
  To: Daniel Baluta
  Cc: Jonathan Cameron, Jonathan Corbet, Hartmut Knaack,
	Lars-Peter Clausen, Peter Meerwald-Stadler, linux-doc,
	Linux Kernel Mailing List, linux-iio

Hi Daniel,


On Wednesday 06 April 2016 07:19 PM, Daniel Baluta wrote:
> On Wed, Apr 6, 2016 at 1:31 PM, Laxman Dewangan <ldewangan@nvidia.com> wrote:
>> Some of kernel driver uses the IIO framework to get the sensor
>> value via ADC or IIO HW driver. The client driver get iio channel
>> by iio_channel_get() and release it by calling iio_channel_release().
>>
>> Add resource managed version (devm_*) of these APIs so that if client
>> calls the devm_iio_channel_get() then it need not to release it explicitly,
>> it can be done by managed device framework when driver get un-binded.
>>
>> This reduces the code in error path and also need of .remove callback in
>> some cases.
>>
> Please provide at least one example of code that uses this API.

Most of client for this APIs are in other subsystem.
When I was working on the patch
[PATCH 2/2] thermal: generic-adc: Add ADC based thermal sensor driver

if I have devm_iio_channel_get() then I can get .remove callback at all.

I did not use this new APIs in my patch because they are in different 
subsystem.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/3] iio: core: Add devm_ APIs for iio_channel_{get,release}
  2016-04-06 14:58   ` Laxman Dewangan
@ 2016-04-10 14:05     ` Jonathan Cameron
  2016-04-10 17:35       ` Laxman Dewangan
  0 siblings, 1 reply; 13+ messages in thread
From: Jonathan Cameron @ 2016-04-10 14:05 UTC (permalink / raw)
  To: Laxman Dewangan, Daniel Baluta
  Cc: Jonathan Corbet, Hartmut Knaack, Lars-Peter Clausen,
	Peter Meerwald-Stadler, linux-doc, Linux Kernel Mailing List,
	linux-iio

On 06/04/16 15:58, Laxman Dewangan wrote:
> Hi Daniel,
> 
> 
> On Wednesday 06 April 2016 07:19 PM, Daniel Baluta wrote:
>> On Wed, Apr 6, 2016 at 1:31 PM, Laxman Dewangan <ldewangan@nvidia.com> wrote:
>>> Some of kernel driver uses the IIO framework to get the sensor
>>> value via ADC or IIO HW driver. The client driver get iio channel
>>> by iio_channel_get() and release it by calling iio_channel_release().
>>>
>>> Add resource managed version (devm_*) of these APIs so that if client
>>> calls the devm_iio_channel_get() then it need not to release it explicitly,
>>> it can be done by managed device framework when driver get un-binded.
>>>
>>> This reduces the code in error path and also need of .remove callback in
>>> some cases.
>>>
>> Please provide at least one example of code that uses this API.
> 
> Most of client for this APIs are in other subsystem.
> When I was working on the patch
> [PATCH 2/2] thermal: generic-adc: Add ADC based thermal sensor driver
> 
> if I have devm_iio_channel_get() then I can get .remove callback at all.
> 
> I did not use this new APIs in my patch because they are in different subsystem.
It's actually worse than that having taken a quick look at the generic-adc thermal patch
you reference above.
(perhaps worth cc'ing linux-iio for next version of that).

Without this devm function set you have a race in remove in which I think you can
get attempts to access the channels after they have been released...

> +
> +	gti->tz_dev = devm_thermal_zone_of_sensor_register(&pdev->dev, 0,
> +				gti, &gadc_thermal_ops);
> +	if (IS_ERR(gti->tz_dev)) {
> +		ret = PTR_ERR(gti->tz_dev);
> +		dev_err(&pdev->dev, "Thermal zone sensor register failed: %d\n",
> +			ret);
> +		goto sensor_fail;
> +	}
This will get cleaned up in remove 'after' the iio_channels are released.
Hence you have a race in which they can probably be accessed after release
(unless some magic is going on that I've missed).
> +
> +	return 0;
> +
> +sensor_fail:
> +	iio_channel_release(gti->channel);
> +	return ret;
> +}
> +
> +static int gadc_thermal_remove(struct platform_device *pdev)
> +{
> +	struct gadc_thermal_info *gti = platform_get_drvdata(pdev);
> +
> +	iio_channel_release(gti->channel);
> +
> +	return 0;
> +}
> +


> 
> 
> 

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/3] iio: core: Add devm_ APIs for iio_channel_{get,release}
  2016-04-06 10:31 [PATCH 1/3] iio: core: Add devm_ APIs for iio_channel_{get,release} Laxman Dewangan
                   ` (2 preceding siblings ...)
  2016-04-06 13:49 ` [PATCH 1/3] iio: core: Add devm_ APIs for iio_channel_{get,release} Daniel Baluta
@ 2016-04-10 14:08 ` Jonathan Cameron
  2016-04-17 12:00 ` Jonathan Cameron
  4 siblings, 0 replies; 13+ messages in thread
From: Jonathan Cameron @ 2016-04-10 14:08 UTC (permalink / raw)
  To: Laxman Dewangan, corbet, knaack.h, lars, pmeerw
  Cc: linux-doc, linux-kernel, linux-iio

On 06/04/16 11:31, Laxman Dewangan wrote:
> Some of kernel driver uses the IIO framework to get the sensor
> value via ADC or IIO HW driver. The client driver get iio channel
> by iio_channel_get() and release it by calling iio_channel_release().
> 
> Add resource managed version (devm_*) of these APIs so that if client
> calls the devm_iio_channel_get() then it need not to release it explicitly,
> it can be done by managed device framework when driver get un-binded.
> 
> This reduces the code in error path and also need of .remove callback in
> some cases.
> 
> Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>

I'm fine with this - but would like it to sit for a few more days to see
if it gets any other feedback.
> ---
>  drivers/iio/inkern.c         | 48 ++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/iio/consumer.h | 27 +++++++++++++++++++++++++
>  2 files changed, 75 insertions(+)
> 
> diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
> index 734a004..18e623f 100644
> --- a/drivers/iio/inkern.c
> +++ b/drivers/iio/inkern.c
> @@ -356,6 +356,54 @@ void iio_channel_release(struct iio_channel *channel)
>  }
>  EXPORT_SYMBOL_GPL(iio_channel_release);
>  
> +static void devm_iio_channel_free(struct device *dev, void *res)
> +{
> +	struct iio_channel *channel = *(struct iio_channel **)res;
> +
> +	iio_channel_release(channel);
> +}
> +
> +static int devm_iio_channel_match(struct device *dev, void *res, void *data)
> +{
> +	struct iio_channel **r = res;
> +
> +	if (!r || !*r) {
> +		WARN_ON(!r || !*r);
> +		return 0;
> +	}
> +
> +	return *r == data;
> +}
> +
> +struct iio_channel *devm_iio_channel_get(struct device *dev,
> +					 const char *channel_name)
> +{
> +	struct iio_channel **ptr, *channel;
> +
> +	ptr = devres_alloc(devm_iio_channel_free, sizeof(*ptr), GFP_KERNEL);
> +	if (!ptr)
> +		return ERR_PTR(-ENOMEM);
> +
> +	channel = iio_channel_get(dev, channel_name);
> +	if (IS_ERR(channel)) {
> +		devres_free(ptr);
> +		return channel;
> +	}
> +
> +	*ptr = channel;
> +	devres_add(dev, ptr);
> +
> +	return channel;
> +}
> +EXPORT_SYMBOL_GPL(devm_iio_channel_get);
> +
> +void devm_iio_channel_release(struct device *dev, struct iio_channel *channel)
> +{
> +	WARN_ON(devres_release(dev, devm_iio_channel_free,
> +			       devm_iio_channel_match, channel));
> +}
> +EXPORT_SYMBOL_GPL(devm_iio_channel_release);
> +
>  struct iio_channel *iio_channel_get_all(struct device *dev)
>  {
>  	const char *name;
> diff --git a/include/linux/iio/consumer.h b/include/linux/iio/consumer.h
> index fad5867..e1e033d 100644
> --- a/include/linux/iio/consumer.h
> +++ b/include/linux/iio/consumer.h
> @@ -49,6 +49,33 @@ struct iio_channel *iio_channel_get(struct device *dev,
>  void iio_channel_release(struct iio_channel *chan);
>  
>  /**
> + * devm_iio_channel_get() - Resource managed version of iio_channel_get().
> + * @dev:		Pointer to consumer device. Device name must match
> + *			the name of the device as provided in the iio_map
> + *			with which the desired provider to consumer mapping
> + *			was registered.
> + * @consumer_channel:	Unique name to identify the channel on the consumer
> + *			side. This typically describes the channels use within
> + *			the consumer. E.g. 'battery_voltage'
> + *
> + * Returns a pointer to negative errno if it is not able to get the iio channel
> + * otherwise returns valid pointer for iio channel.
> + *
> + * The allocated iio channel is automatically released when the device is
> + * unbound.
> + */
> +struct iio_channel *devm_iio_channel_get(struct device *dev,
> +					 const char *consumer_channel);
> +/**
> + * devm_iio_channel_release() - Resource managed version of
> + *				iio_channel_release().
> + * @dev:		Pointer to consumer device for which resource
> + *			is allocared.
> + * @chan:		The channel to be released.
> + */
> +void devm_iio_channel_release(struct device *dev, struct iio_channel *chan);
> +
> +/**
>   * iio_channel_get_all() - get all channels associated with a client
>   * @dev:		Pointer to consumer device.
>   *
> 

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/3] iio: core: Add devm_ APIs for iio_channel_{get,release}
  2016-04-10 14:05     ` Jonathan Cameron
@ 2016-04-10 17:35       ` Laxman Dewangan
  2016-04-16 13:25         ` Jonathan Cameron
  0 siblings, 1 reply; 13+ messages in thread
From: Laxman Dewangan @ 2016-04-10 17:35 UTC (permalink / raw)
  To: Jonathan Cameron, Daniel Baluta
  Cc: Jonathan Corbet, Hartmut Knaack, Lars-Peter Clausen,
	Peter Meerwald-Stadler, linux-doc, Linux Kernel Mailing List,
	linux-iio


On Sunday 10 April 2016 07:35 PM, Jonathan Cameron wrote:
> On 06/04/16 15:58, Laxman Dewangan wrote:
>> Hi Daniel,
>>
>>
>> On Wednesday 06 April 2016 07:19 PM, Daniel Baluta wrote:
>>> On Wed, Apr 6, 2016 at 1:31 PM, Laxman Dewangan <ldewangan@nvidia.com> wrote:
>>>> Some of kernel driver uses the IIO framework to get the sensor
>>>> value via ADC or IIO HW driver. The client driver get iio channel
>>>> by iio_channel_get() and release it by calling iio_channel_release().
>>>>
>>>> Add resource managed version (devm_*) of these APIs so that if client
>>>> calls the devm_iio_channel_get() then it need not to release it explicitly,
>>>> it can be done by managed device framework when driver get un-binded.
>>>>
>>>> This reduces the code in error path and also need of .remove callback in
>>>> some cases.
>>>>
>>> Please provide at least one example of code that uses this API.
>> Most of client for this APIs are in other subsystem.
>> When I was working on the patch
>> [PATCH 2/2] thermal: generic-adc: Add ADC based thermal sensor driver
>>
>> if I have devm_iio_channel_get() then I can get .remove callback at all.
>>
>> I did not use this new APIs in my patch because they are in different subsystem.
> It's actually worse than that having taken a quick look at the generic-adc thermal patch
> you reference above.
> (perhaps worth cc'ing linux-iio for next version of that).
Sure. I will CC.

>
> Without this devm function set you have a race in remove in which I think you can
> get attempts to access the channels after they have been released...
Yaah, possibly race for very small time possible.

The limitation of devm_ api usage is that, we can keep using this till 
we have devm_ api continuous and if some resource are not there for 
devm_ then we can not use further.
Possibly, I need to wait for the devm_iio_channel_get() to merge and 
available for all subsystem to use (next release) and then only I can 
use devm_thermal_zone_of_sensor_register().

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/3] iio: core: Add devm_ APIs for iio_channel_{get,release}
  2016-04-10 17:35       ` Laxman Dewangan
@ 2016-04-16 13:25         ` Jonathan Cameron
  2016-04-16 15:42           ` Laxman Dewangan
  0 siblings, 1 reply; 13+ messages in thread
From: Jonathan Cameron @ 2016-04-16 13:25 UTC (permalink / raw)
  To: Laxman Dewangan, Daniel Baluta
  Cc: Jonathan Corbet, Hartmut Knaack, Lars-Peter Clausen,
	Peter Meerwald-Stadler, linux-doc, Linux Kernel Mailing List,
	linux-iio

On 10/04/16 18:35, Laxman Dewangan wrote:
> 
> On Sunday 10 April 2016 07:35 PM, Jonathan Cameron wrote:
>> On 06/04/16 15:58, Laxman Dewangan wrote:
>>> Hi Daniel,
>>>
>>>
>>> On Wednesday 06 April 2016 07:19 PM, Daniel Baluta wrote:
>>>> On Wed, Apr 6, 2016 at 1:31 PM, Laxman Dewangan <ldewangan@nvidia.com> wrote:
>>>>> Some of kernel driver uses the IIO framework to get the sensor
>>>>> value via ADC or IIO HW driver. The client driver get iio channel
>>>>> by iio_channel_get() and release it by calling iio_channel_release().
>>>>>
>>>>> Add resource managed version (devm_*) of these APIs so that if client
>>>>> calls the devm_iio_channel_get() then it need not to release it explicitly,
>>>>> it can be done by managed device framework when driver get un-binded.
>>>>>
>>>>> This reduces the code in error path and also need of .remove callback in
>>>>> some cases.
>>>>>
>>>> Please provide at least one example of code that uses this API.
>>> Most of client for this APIs are in other subsystem.
>>> When I was working on the patch
>>> [PATCH 2/2] thermal: generic-adc: Add ADC based thermal sensor driver
>>>
>>> if I have devm_iio_channel_get() then I can get .remove callback at all.
>>>
>>> I did not use this new APIs in my patch because they are in different subsystem.
>> It's actually worse than that having taken a quick look at the generic-adc thermal patch
>> you reference above.
>> (perhaps worth cc'ing linux-iio for next version of that).
> Sure. I will CC.
> 
>>
>> Without this devm function set you have a race in remove in which I think you can
>> get attempts to access the channels after they have been released...
> Yaah, possibly race for very small time possible.
> 
> The limitation of devm_ api usage is that, we can keep using this
> till we have devm_ api continuous and if some resource are not there
> for devm_ then we can not use further. Possibly, I need to wait for
> the devm_iio_channel_get() to merge and available for all subsystem
> to use (next release) and then only I can use
> devm_thermal_zone_of_sensor_register().
> 
The alternative would be to merge this devm_ support as a prerequisite for your thermal
patches and have it go through that tree.  As it's self contained I have
no particular problem with that if you'd prefer to do it that way.

Otherwise, you will need to do as you say above (not use
devm_thermal_zone_of_sensor_register) to make sure it isn't broken in the meantime.

Jonathan

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/3] iio: core: Add devm_ APIs for iio_channel_{get,release}
  2016-04-16 13:25         ` Jonathan Cameron
@ 2016-04-16 15:42           ` Laxman Dewangan
  0 siblings, 0 replies; 13+ messages in thread
From: Laxman Dewangan @ 2016-04-16 15:42 UTC (permalink / raw)
  To: Jonathan Cameron, Daniel Baluta
  Cc: Jonathan Corbet, Hartmut Knaack, Lars-Peter Clausen,
	Peter Meerwald-Stadler, linux-doc, Linux Kernel Mailing List,
	linux-iio


On Saturday 16 April 2016 06:55 PM, Jonathan Cameron wrote:
> On 10/04/16 18:35, Laxman Dewangan wrote:
>>
>> Yaah, possibly race for very small time possible.
>>
>> The limitation of devm_ api usage is that, we can keep using this
>> till we have devm_ api continuous and if some resource are not there
>> for devm_ then we can not use further. Possibly, I need to wait for
>> the devm_iio_channel_get() to merge and available for all subsystem
>> to use (next release) and then only I can use
>> devm_thermal_zone_of_sensor_register().
>>
> The alternative would be to merge this devm_ support as a prerequisite for your thermal
> patches and have it go through that tree.  As it's self contained I have
> no particular problem with that if you'd prefer to do it that way.
>
> Otherwise, you will need to do as you say above (not use
> devm_thermal_zone_of_sensor_register) to make sure it isn't broken in the meantime.
>
>

I have recycled patch of thermal driver to not use devm_ for sensor 
registration.

I will post another patch in future once these changes will be available 
for all, most probably next release.

I think there is no further comment now on the series so you can 
consider for next step.

Thanks,
Laxman

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/3] iio: core: Add devm_ APIs for iio_channel_{get,release}
  2016-04-06 10:31 [PATCH 1/3] iio: core: Add devm_ APIs for iio_channel_{get,release} Laxman Dewangan
                   ` (3 preceding siblings ...)
  2016-04-10 14:08 ` Jonathan Cameron
@ 2016-04-17 12:00 ` Jonathan Cameron
  2016-04-17 16:48   ` Laxman Dewangan
  4 siblings, 1 reply; 13+ messages in thread
From: Jonathan Cameron @ 2016-04-17 12:00 UTC (permalink / raw)
  To: Laxman Dewangan, corbet, knaack.h, lars, pmeerw
  Cc: linux-doc, linux-kernel, linux-iio

On 06/04/16 11:31, Laxman Dewangan wrote:
> Some of kernel driver uses the IIO framework to get the sensor
> value via ADC or IIO HW driver. The client driver get iio channel
> by iio_channel_get() and release it by calling iio_channel_release().
> 
> Add resource managed version (devm_*) of these APIs so that if client
> calls the devm_iio_channel_get() then it need not to release it explicitly,
> it can be done by managed device framework when driver get un-binded.
> 
> This reduces the code in error path and also need of .remove callback in
> some cases.
> 
> Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
Applied to the togreg branch of iio.git.  
I guess the thermal driver use case will hit a cycle or so behind this.

Thanks,

Jonathan
> ---
>  drivers/iio/inkern.c         | 48 ++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/iio/consumer.h | 27 +++++++++++++++++++++++++
>  2 files changed, 75 insertions(+)
> 
> diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
> index 734a004..18e623f 100644
> --- a/drivers/iio/inkern.c
> +++ b/drivers/iio/inkern.c
> @@ -356,6 +356,54 @@ void iio_channel_release(struct iio_channel *channel)
>  }
>  EXPORT_SYMBOL_GPL(iio_channel_release);
>  
> +static void devm_iio_channel_free(struct device *dev, void *res)
> +{
> +	struct iio_channel *channel = *(struct iio_channel **)res;
> +
> +	iio_channel_release(channel);
> +}
> +
> +static int devm_iio_channel_match(struct device *dev, void *res, void *data)
> +{
> +	struct iio_channel **r = res;
> +
> +	if (!r || !*r) {
> +		WARN_ON(!r || !*r);
> +		return 0;
> +	}
> +
> +	return *r == data;
> +}
> +
> +struct iio_channel *devm_iio_channel_get(struct device *dev,
> +					 const char *channel_name)
> +{
> +	struct iio_channel **ptr, *channel;
> +
> +	ptr = devres_alloc(devm_iio_channel_free, sizeof(*ptr), GFP_KERNEL);
> +	if (!ptr)
> +		return ERR_PTR(-ENOMEM);
> +
> +	channel = iio_channel_get(dev, channel_name);
> +	if (IS_ERR(channel)) {
> +		devres_free(ptr);
> +		return channel;
> +	}
> +
> +	*ptr = channel;
> +	devres_add(dev, ptr);
> +
> +	return channel;
> +}
> +EXPORT_SYMBOL_GPL(devm_iio_channel_get);
> +
> +void devm_iio_channel_release(struct device *dev, struct iio_channel *channel)
> +{
> +	WARN_ON(devres_release(dev, devm_iio_channel_free,
> +			       devm_iio_channel_match, channel));
> +}
> +EXPORT_SYMBOL_GPL(devm_iio_channel_release);
> +
>  struct iio_channel *iio_channel_get_all(struct device *dev)
>  {
>  	const char *name;
> diff --git a/include/linux/iio/consumer.h b/include/linux/iio/consumer.h
> index fad5867..e1e033d 100644
> --- a/include/linux/iio/consumer.h
> +++ b/include/linux/iio/consumer.h
> @@ -49,6 +49,33 @@ struct iio_channel *iio_channel_get(struct device *dev,
>  void iio_channel_release(struct iio_channel *chan);
>  
>  /**
> + * devm_iio_channel_get() - Resource managed version of iio_channel_get().
> + * @dev:		Pointer to consumer device. Device name must match
> + *			the name of the device as provided in the iio_map
> + *			with which the desired provider to consumer mapping
> + *			was registered.
> + * @consumer_channel:	Unique name to identify the channel on the consumer
> + *			side. This typically describes the channels use within
> + *			the consumer. E.g. 'battery_voltage'
> + *
> + * Returns a pointer to negative errno if it is not able to get the iio channel
> + * otherwise returns valid pointer for iio channel.
> + *
> + * The allocated iio channel is automatically released when the device is
> + * unbound.
> + */
> +struct iio_channel *devm_iio_channel_get(struct device *dev,
> +					 const char *consumer_channel);
> +/**
> + * devm_iio_channel_release() - Resource managed version of
> + *				iio_channel_release().
> + * @dev:		Pointer to consumer device for which resource
> + *			is allocared.
> + * @chan:		The channel to be released.
> + */
> +void devm_iio_channel_release(struct device *dev, struct iio_channel *chan);
> +
> +/**
>   * iio_channel_get_all() - get all channels associated with a client
>   * @dev:		Pointer to consumer device.
>   *
> 

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/3] iio: core: Add devm_ APIs for iio_channel_{get,release}_all
  2016-04-06 10:31 ` [PATCH 2/3] iio: core: Add devm_ APIs for iio_channel_{get,release}_all Laxman Dewangan
@ 2016-04-17 12:06   ` Jonathan Cameron
  0 siblings, 0 replies; 13+ messages in thread
From: Jonathan Cameron @ 2016-04-17 12:06 UTC (permalink / raw)
  To: Laxman Dewangan, corbet, knaack.h, lars, pmeerw
  Cc: linux-doc, linux-kernel, linux-iio

On 06/04/16 11:31, Laxman Dewangan wrote:
> Some of kernel driver uses the IIO framework to get the sensor
> value via ADC or IIO HW driver. The client driver get iio channel
> by iio_channel_get_all() and release it by calling
> iio_channel_release_all().
> 
> Add resource managed version (devm_*) of these APIs so that if client
> calls the devm_iio_channel_get_all() then it need not to release it
> explicitly, it can be done by managed device framework when driver
> get un-binded.
> 
> This reduces the code in error path and also need of .remove callback in
> some cases.
> 
> Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
Applied to the togreg branch of iio.git.  Thanks,

Jonathan
> ---
>  drivers/iio/inkern.c         | 36 ++++++++++++++++++++++++++++++++++++
>  include/linux/iio/consumer.h | 26 ++++++++++++++++++++++++++
>  2 files changed, 62 insertions(+)
> 
> diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
> index 18e623f..8c1abfe 100644
> --- a/drivers/iio/inkern.c
> +++ b/drivers/iio/inkern.c
> @@ -489,6 +489,42 @@ void iio_channel_release_all(struct iio_channel *channels)
>  }
>  EXPORT_SYMBOL_GPL(iio_channel_release_all);
>  
> +static void devm_iio_channel_free_all(struct device *dev, void *res)
> +{
> +	struct iio_channel *channels = *(struct iio_channel **)res;
> +
> +	iio_channel_release_all(channels);
> +}
> +
> +struct iio_channel *devm_iio_channel_get_all(struct device *dev)
> +{
> +	struct iio_channel **ptr, *channels;
> +
> +	ptr = devres_alloc(devm_iio_channel_free_all, sizeof(*ptr), GFP_KERNEL);
> +	if (!ptr)
> +		return ERR_PTR(-ENOMEM);
> +
> +	channels = iio_channel_get_all(dev);
> +	if (IS_ERR(channels)) {
> +		devres_free(ptr);
> +		return channels;
> +	}
> +
> +	*ptr = channels;
> +	devres_add(dev, ptr);
> +
> +	return channels;
> +}
> +EXPORT_SYMBOL_GPL(devm_iio_channel_get_all);
> +
> +void devm_iio_channel_release_all(struct device *dev,
> +				  struct iio_channel *channels)
> +{
> +	WARN_ON(devres_release(dev, devm_iio_channel_free_all,
> +			       devm_iio_channel_match, channels));
> +}
> +EXPORT_SYMBOL_GPL(devm_iio_channel_release_all);
> +
>  static int iio_channel_read(struct iio_channel *chan, int *val, int *val2,
>  	enum iio_chan_info_enum info)
>  {
> diff --git a/include/linux/iio/consumer.h b/include/linux/iio/consumer.h
> index e1e033d..3d672f7 100644
> --- a/include/linux/iio/consumer.h
> +++ b/include/linux/iio/consumer.h
> @@ -92,6 +92,32 @@ struct iio_channel *iio_channel_get_all(struct device *dev);
>   */
>  void iio_channel_release_all(struct iio_channel *chan);
>  
> +/**
> + * devm_iio_channel_get_all() - Resource managed version of
> + *				iio_channel_get_all().
> + * @dev: Pointer to consumer device.
> + *
> + * Returns a pointer to negative errno if it is not able to get the iio channel
> + * otherwise returns an array of iio_channel structures terminated with one with
> + * null iio_dev pointer.
> + *
> + * This function is used by fairly generic consumers to get all the
> + * channels registered as having this consumer.
> + *
> + * The allocated iio channels are automatically released when the device is
> + * unbounded.
> + */
> +struct iio_channel *devm_iio_channel_get_all(struct device *dev);
> +
> +/**
> + * devm_iio_channel_release_all() - Resource managed version of
> + *				    iio_channel_release_all().
> + * @dev:		Pointer to consumer device for which resource
> + *			is allocared.
> + * @chan:		Array channel to be released.
> + */
> +void devm_iio_channel_release_all(struct device *dev, struct iio_channel *chan);
> +
>  struct iio_cb_buffer;
>  /**
>   * iio_channel_get_all_cb() - register callback for triggered capture
> 

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/3] iio: core: Add devm_ APIs for iio_channel_{get,release}
  2016-04-17 12:00 ` Jonathan Cameron
@ 2016-04-17 16:48   ` Laxman Dewangan
  0 siblings, 0 replies; 13+ messages in thread
From: Laxman Dewangan @ 2016-04-17 16:48 UTC (permalink / raw)
  To: Jonathan Cameron, corbet, knaack.h, lars, pmeerw
  Cc: linux-doc, linux-kernel, linux-iio


On Sunday 17 April 2016 05:30 PM, Jonathan Cameron wrote:
> On 06/04/16 11:31, Laxman Dewangan wrote:
>> Some of kernel driver uses the IIO framework to get the sensor
>> value via ADC or IIO HW driver. The client driver get iio channel
>> by iio_channel_get() and release it by calling iio_channel_release().
>>
>> Add resource managed version (devm_*) of these APIs so that if client
>> calls the devm_iio_channel_get() then it need not to release it explicitly,
>> it can be done by managed device framework when driver get un-binded.
>>
>> This reduces the code in error path and also need of .remove callback in
>> some cases.
>>
>> Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
> Applied to the togreg branch of iio.git.
> I guess the thermal driver use case will hit a cycle or so behind this.
>

Thanks for applying this.

Yes, thermal driver will use this in future release. This is to avoid 
any syncup between subsystem.

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2016-04-17 17:00 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-04-06 10:31 [PATCH 1/3] iio: core: Add devm_ APIs for iio_channel_{get,release} Laxman Dewangan
2016-04-06 10:31 ` [PATCH 2/3] iio: core: Add devm_ APIs for iio_channel_{get,release}_all Laxman Dewangan
2016-04-17 12:06   ` Jonathan Cameron
2016-04-06 10:31 ` [PATCH 3/3] iio: Add resource managed APIs devm_iio_channel_{get,release) in devres Laxman Dewangan
2016-04-06 13:49 ` [PATCH 1/3] iio: core: Add devm_ APIs for iio_channel_{get,release} Daniel Baluta
2016-04-06 14:58   ` Laxman Dewangan
2016-04-10 14:05     ` Jonathan Cameron
2016-04-10 17:35       ` Laxman Dewangan
2016-04-16 13:25         ` Jonathan Cameron
2016-04-16 15:42           ` Laxman Dewangan
2016-04-10 14:08 ` Jonathan Cameron
2016-04-17 12:00 ` Jonathan Cameron
2016-04-17 16:48   ` Laxman Dewangan

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).