[PATCH v7 03/10] iio: adc: add helpers for parsing ADC nodes

[PATCH v7 03/10] iio: adc: add helpers for parsing ADC nodes

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 6271 bytes --]

There are ADC ICs which may have some of the AIN pins usable for other
functions. These ICs may have some of the AIN pins wired so that they
should not be used for ADC.

(Preferred?) way for marking pins which can be used as ADC inputs is to
add corresponding channels@N nodes in the device tree as described in
the ADC binding yaml.

Add couple of helper functions which can be used to retrieve the channel
information from the device node.

Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>

---
Revision history:
v6 =>
 - No changes
v5 => v6:
 - Adapt to changed fwnode helper names
 - Fix spelling
v4 => v5:
 - Inline iio_adc_device_num_channels()
 - Fix Indenting function parameters
 - Combine the max channel ID checks.
v3 => v4:
 - Drop diff-channel support
 - Drop iio_adc_device_channels_by_property()
 - Add IIO_DEVICE namespace
 - Move industrialio-adc.o to top of the Makefile
 - Some styling as suggested by Andy
 - Re-consider included headers
v2 => v3: Mostly based on review comments by Jonathan
 - Support differential and single-ended channels
 - Rename iio_adc_device_get_channels() as
   iio_adc_device_channels_by_property()
 - Improve spelling
 - Drop support for cases where DT comes from parent device's node
 - Decrease loop indent by reverting node name check conditions
 - Don't set 'chan->indexed' by number of channels to keep the
   interface consistent no matter how many channels are connected.
 - Fix ID range check and related comment
RFC v1 => v2:
 - New patch
---
 drivers/iio/adc/Kconfig            |  3 ++
 drivers/iio/adc/Makefile           |  2 +
 drivers/iio/adc/industrialio-adc.c | 79 ++++++++++++++++++++++++++++++
 include/linux/iio/adc-helpers.h    | 27 ++++++++++
 4 files changed, 111 insertions(+)
 create mode 100644 drivers/iio/adc/industrialio-adc.c
 create mode 100644 include/linux/iio/adc-helpers.h

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 849c90203071..37b70a65da6f 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -6,6 +6,9 @@
 
 menu "Analog to digital converters"
 
+config IIO_ADC_HELPER
+	tristate
+
 config AB8500_GPADC
 	bool "ST-Ericsson AB8500 GPADC driver"
 	depends on AB8500_CORE && REGULATOR_AB8500
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index ee19afba62b7..1c410f483029 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -3,6 +3,8 @@
 # Makefile for IIO ADC drivers
 #
 
+obj-$(CONFIG_IIO_ADC_HELPER) += industrialio-adc.o
+
 # When adding new entries keep the list in alphabetical order
 obj-$(CONFIG_AB8500_GPADC) += ab8500-gpadc.o
 obj-$(CONFIG_AD_SIGMA_DELTA) += ad_sigma_delta.o
diff --git a/drivers/iio/adc/industrialio-adc.c b/drivers/iio/adc/industrialio-adc.c
new file mode 100644
index 000000000000..6cfb001c74b9
--- /dev/null
+++ b/drivers/iio/adc/industrialio-adc.c
@@ -0,0 +1,79 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Helpers for parsing common ADC information from a firmware node.
+ *
+ * Copyright (c) 2025 Matti Vaittinen <mazziesaccount@gmail.com>
+ */
+
+#include <linux/device.h>
+#include <linux/errno.h>
+#include <linux/export.h>
+#include <linux/module.h>
+#include <linux/property.h>
+#include <linux/types.h>
+
+#include <linux/iio/adc-helpers.h>
+#include <linux/iio/iio.h>
+
+/**
+ * devm_iio_adc_device_alloc_chaninfo_se - allocate and fill iio_chan_spec for ADC
+ *
+ * Scan the device node for single-ended ADC channel information. Channel ID is
+ * expected to be found from the "reg" property. Allocate and populate the
+ * iio_chan_spec structure corresponding to channels that are found. The memory
+ * for iio_chan_spec structure will be freed upon device detach.
+ *
+ * @dev:		Pointer to the ADC device.
+ * @template:		Template iio_chan_spec from which the fields of all
+ *			found and allocated channels are initialized.
+ * @max_chan_id:	Maximum value of a channel ID. Use -1 if no checking
+ *			is required.
+ * @cs:			Location where pointer to allocated iio_chan_spec
+ *			should be stored.
+ *
+ * Return:	Number of found channels on success. Negative value to indicate
+ *		failure.
+ */
+int devm_iio_adc_device_alloc_chaninfo_se(struct device *dev,
+					  const struct iio_chan_spec *template,
+					  int max_chan_id,
+					  struct iio_chan_spec **cs)
+{
+	struct iio_chan_spec *chan_array, *chan;
+	int num_chan = 0, ret;
+
+	num_chan = iio_adc_device_num_channels(dev);
+	if (num_chan < 1)
+		return num_chan;
+
+	chan_array = devm_kcalloc(dev, num_chan, sizeof(*chan_array),
+				  GFP_KERNEL);
+	if (!chan_array)
+		return -ENOMEM;
+
+	chan = &chan_array[0];
+
+	device_for_each_named_child_node_scoped(dev, child, "channel") {
+		u32 ch;
+
+		ret = fwnode_property_read_u32(child, "reg", &ch);
+		if (ret)
+			return ret;
+
+		if (max_chan_id != -1 && ch > max_chan_id)
+			return -ERANGE;
+
+		*chan = *template;
+		chan->channel = ch;
+		chan++;
+	}
+
+	*cs = chan_array;
+
+	return num_chan;
+}
+EXPORT_SYMBOL_NS_GPL(devm_iio_adc_device_alloc_chaninfo_se, "IIO_DRIVER");
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Matti Vaittinen <mazziesaccount@gmail.com>");
+MODULE_DESCRIPTION("IIO ADC fwnode parsing helpers");
diff --git a/include/linux/iio/adc-helpers.h b/include/linux/iio/adc-helpers.h
new file mode 100644
index 000000000000..56b092a2a4c4
--- /dev/null
+++ b/include/linux/iio/adc-helpers.h
@@ -0,0 +1,27 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+/*
+ * The industrial I/O ADC firmware property parsing helpers
+ *
+ * Copyright (c) 2025 Matti Vaittinen <mazziesaccount@gmail.com>
+ */
+
+#ifndef _INDUSTRIAL_IO_ADC_HELPERS_H_
+#define _INDUSTRIAL_IO_ADC_HELPERS_H_
+
+#include <linux/property.h>
+
+struct device;
+struct iio_chan_spec;
+
+static inline int iio_adc_device_num_channels(struct device *dev)
+{
+	return device_get_named_child_node_count(dev, "channel");
+}
+
+int devm_iio_adc_device_alloc_chaninfo_se(struct device *dev,
+					  const struct iio_chan_spec *template,
+					  int max_chan_id,
+					  struct iio_chan_spec **cs);
+
+#endif /* _INDUSTRIAL_IO_ADC_HELPERS_H_ */
-- 
2.48.1


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v7 03/10] iio: adc: add helpers for parsing ADC nodes

[Andy Shevchenko]

On Thu, Mar 13, 2025 at 09:18:18AM +0200, Matti Vaittinen wrote:
> There are ADC ICs which may have some of the AIN pins usable for other
> functions. These ICs may have some of the AIN pins wired so that they
> should not be used for ADC.
> 
> (Preferred?) way for marking pins which can be used as ADC inputs is to
> add corresponding channels@N nodes in the device tree as described in
> the ADC binding yaml.
> 
> Add couple of helper functions which can be used to retrieve the channel
> information from the device node.

...

> +int devm_iio_adc_device_alloc_chaninfo_se(struct device *dev,
> +					  const struct iio_chan_spec *template,
> +					  int max_chan_id,
> +					  struct iio_chan_spec **cs)
> +{
> +	struct iio_chan_spec *chan_array, *chan;
> +	int num_chan = 0, ret;

Unneeded assignment.

> +	num_chan = iio_adc_device_num_channels(dev);
> +	if (num_chan < 1)
> +		return num_chan;

This is really interesting code. So, if the above returns negative error code,
we return it, if it returns 0, we return success (but 0 channels)?

Shouldn't we do *cs = NULL; at the case of 0 channels if it's a success?
(Under success I assume that returned values are okay to go with, and cs in
your case will be left uninitialised or contain something we don't control.

> +	chan_array = devm_kcalloc(dev, num_chan, sizeof(*chan_array),
> +				  GFP_KERNEL);
> +	if (!chan_array)
> +		return -ENOMEM;
> +
> +	chan = &chan_array[0];
> +
> +	device_for_each_named_child_node_scoped(dev, child, "channel") {
> +		u32 ch;
> +
> +		ret = fwnode_property_read_u32(child, "reg", &ch);
> +		if (ret)
> +			return ret;

> +		if (max_chan_id != -1 && ch > max_chan_id)
> +			return -ERANGE;

Hmm... What if max_chan_id is equal to an error code?
Or in other words, why -1 is special and not all negative numbers?

Also note, you used unsigned type and compare it to int which,
in case of being negative will give promotion. The ch will not be
big enough in most cases (unless it's great than (INT_MAX + 1).

TL;DR: you have a potential integer overflow here.

> +		*chan = *template;
> +		chan->channel = ch;
> +		chan++;
> +	}
> +
> +	*cs = chan_array;
> +
> +	return num_chan;
> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v7 03/10] iio: adc: add helpers for parsing ADC nodes

[Matti Vaittinen]

On 13/03/2025 14:31, Andy Shevchenko wrote:
> On Thu, Mar 13, 2025 at 09:18:18AM +0200, Matti Vaittinen wrote:
>> There are ADC ICs which may have some of the AIN pins usable for other
>> functions. These ICs may have some of the AIN pins wired so that they
>> should not be used for ADC.
>>
>> (Preferred?) way for marking pins which can be used as ADC inputs is to
>> add corresponding channels@N nodes in the device tree as described in
>> the ADC binding yaml.
>>
>> Add couple of helper functions which can be used to retrieve the channel
>> information from the device node.
> 
> ...
> 
>> +int devm_iio_adc_device_alloc_chaninfo_se(struct device *dev,
>> +					  const struct iio_chan_spec *template,
>> +					  int max_chan_id,
>> +					  struct iio_chan_spec **cs)
>> +{
>> +	struct iio_chan_spec *chan_array, *chan;
>> +	int num_chan = 0, ret;
> 
> Unneeded assignment.

Hmm. I have a deja-vu. Thanks for the reminder.

> 
>> +	num_chan = iio_adc_device_num_channels(dev);
>> +	if (num_chan < 1)
>> +		return num_chan;
> 
> This is really interesting code. So, if the above returns negative error code,
> we return it, if it returns 0, we return success (but 0 channels)?

Yes. I don't think it's that interesting though. Checking the devicetree 
succeeded while no channels were found. I think returning 0 is very much 
aligned with this.

> Shouldn't we do *cs = NULL; at the case of 0 channels if it's a success?

I suppose you're right.

But, as you pointed out in review of the 05/10:
 > Usually in other similar APIs we return -ENOENT. And user won't need
 > to have an additional check in case of 0 being considered as an error
 > case too.

I don't know whether to agree with you here. For majority of the ADC 
drivers, having no channels in devicetree is indeed just another error, 
which I think is not in any ways special.

However, for 33,3333% of the users added in this patch, the "no channels 
found" is not really an error condition ;) The BD79124 could have all 
channels used for GPO - although this would probably be very very 
unusual. (Why buying an ADC chip if you need just a GPO?). Still, this 
wouldn't be an error. (And I need to handle this better in BD79124 probe 
- so thanks).

> (Under success I assume that returned values are okay to go with, and cs in
> your case will be left uninitialised or contain something we don't control.

I see your point although I wouldn't be concerned with cs not being NULL 
for as long as number of channels is zero.

Anyway, I think it makes sense to simplify ~67% of callers by returning 
-ENODEV if there is no channels. The remaining ~33% can then check for 
the -ENODEV and handle it separately from other returned errors. So, thanks.

>> +	chan_array = devm_kcalloc(dev, num_chan, sizeof(*chan_array),
>> +				  GFP_KERNEL);
>> +	if (!chan_array)
>> +		return -ENOMEM;
>> +
>> +	chan = &chan_array[0];
>> +
>> +	device_for_each_named_child_node_scoped(dev, child, "channel") {
>> +		u32 ch;
>> +
>> +		ret = fwnode_property_read_u32(child, "reg", &ch);
>> +		if (ret)
>> +			return ret;
> 
>> +		if (max_chan_id != -1 && ch > max_chan_id)
>> +			return -ERANGE;
> 
> Hmm... What if max_chan_id is equal to an error code?
> Or in other words, why -1 is special and not all negative numbers?

-1 was just picked to represent a 'don't care' value. Old habit. In the 
old days I handled lots of code where -1 was defined as 'invalid' for 
APIs with unsigned ints as well. It works nicely on systems where it 
turns out to be maximum positive value - leaving most of the number 
space for valid values.

I suppose saying any negative means "don't care" works well here. And, 
dropping all negatives here will also make the check below just work 
with unsigned comparison.

> Also note, you used unsigned type and compare it to int which,
> in case of being negative will give promotion.

Right. I didn't thin negative IDs would make sense and trusted users to 
pass only positive ones. Treating all negatives as "don't care" is 
indeed better than trusting this.

Thanks.

> The ch will not be
> big enough in most cases (unless it's great than (INT_MAX + 1).
> 
> TL;DR: you have a potential integer overflow here.
> 
>> +		*chan = *template;
>> +		chan->channel = ch;
>> +		chan++;
>> +	}
>> +
>> +	*cs = chan_array;
>> +
>> +	return num_chan;
>> +}
> 

Re: [PATCH v7 03/10] iio: adc: add helpers for parsing ADC nodes

[Andy Shevchenko]

On Thu, Mar 13, 2025 at 03:17:27PM +0200, Matti Vaittinen wrote:
> On 13/03/2025 14:31, Andy Shevchenko wrote:
> > On Thu, Mar 13, 2025 at 09:18:18AM +0200, Matti Vaittinen wrote:

...

> > > +	num_chan = iio_adc_device_num_channels(dev);
> > > +	if (num_chan < 1)
> > > +		return num_chan;
> > 
> > This is really interesting code. So, if the above returns negative error code,
> > we return it, if it returns 0, we return success (but 0 channels)?
> 
> Yes. I don't think it's that interesting though. Checking the devicetree
> succeeded while no channels were found. I think returning 0 is very much
> aligned with this.

Right, but as I suggested, let's follow already established APIs that return
-ENOENT and never 0 in similar cases.

> > Shouldn't we do *cs = NULL; at the case of 0 channels if it's a success?
> 
> I suppose you're right.
> 
> But, as you pointed out in review of the 05/10:
> > Usually in other similar APIs we return -ENOENT. And user won't need
> > to have an additional check in case of 0 being considered as an error
> > case too.
> 
> I don't know whether to agree with you here. For majority of the ADC
> drivers, having no channels in devicetree is indeed just another error,
> which I think is not in any ways special.

So...? (I see below your answer :-)

> However, for 33,3333% of the users added in this patch, the "no channels
> found" is not really an error condition ;) The BD79124 could have all
> channels used for GPO - although this would probably be very very unusual.
> (Why buying an ADC chip if you need just a GPO?). Still, this wouldn't be an
> error. (And I need to handle this better in BD79124 probe - so thanks).

ENOENT check is again established for optional/not_found cases.

> > (Under success I assume that returned values are okay to go with, and cs in
> > your case will be left uninitialised or contain something we don't control.
> 
> I see your point although I wouldn't be concerned with cs not being NULL for
> as long as number of channels is zero.
> 
> Anyway, I think it makes sense to simplify ~67% of callers by returning
> -ENODEV if there is no channels. The remaining ~33% can then check for the
> -ENODEV and handle it separately from other returned errors. So, thanks.

Not at all!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v7 03/10] iio: adc: add helpers for parsing ADC nodes

[Jonathan Cameron]

On Thu, 13 Mar 2025 09:18:18 +0200
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> There are ADC ICs which may have some of the AIN pins usable for other
> functions. These ICs may have some of the AIN pins wired so that they
> should not be used for ADC.
> 
> (Preferred?) way for marking pins which can be used as ADC inputs is to
> add corresponding channels@N nodes in the device tree as described in
> the ADC binding yaml.
> 
> Add couple of helper functions which can be used to retrieve the channel
> information from the device node.
I suspect we'll need the addition of an optional trailing timestamp
channel at somepoint. But we can add that when we need it as only
matters for drivers doing iio_push_to_buffers_with_timestamp()

Otherwise no additional comments from me.

Re: [PATCH v7 03/10] iio: adc: add helpers for parsing ADC nodes

[Matti Vaittinen]

On 16/03/2025 11:38, Jonathan Cameron wrote:
> On Thu, 13 Mar 2025 09:18:18 +0200
> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> 
>> There are ADC ICs which may have some of the AIN pins usable for other
>> functions. These ICs may have some of the AIN pins wired so that they
>> should not be used for ADC.
>>
>> (Preferred?) way for marking pins which can be used as ADC inputs is to
>> add corresponding channels@N nodes in the device tree as described in
>> the ADC binding yaml.
>>
>> Add couple of helper functions which can be used to retrieve the channel
>> information from the device node.
> I suspect we'll need the addition of an optional trailing timestamp
> channel at somepoint. But we can add that when we need it as only
> matters for drivers doing iio_push_to_buffers_with_timestamp()

This is true. That'll enable using this for devices with buffers - which 
is not possible right now as most buffer users do timestamps. I'll leave 
adding the parameter to first buffered user though, but I think it's 
good to say out loud this is doable :) Thanks!

> Otherwise no additional comments from me.

Yours,
	-- Matti

[PATCH v7 05/10] iio: adc: sun20i-gpadc: Use adc-helpers

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 4932 bytes --]

The new devm_iio_adc_device_alloc_chaninfo_se() -helper is intended to
help drivers avoid open-coding the for_each_node -loop for getting the
channel IDs. The helper provides standard way to detect the ADC channel
nodes (by the node name), and a standard way to convert the "reg"
-properties to channel identification numbers, used in the struct
iio_chan_spec. Furthermore, the helper can optionally check the found
channel IDs are smaller than given maximum. This is useful for callers
which later use the IDs for example for indexing a channel data array.

The original driver treated all found child nodes as channel nodes. The
new helper requires channel nodes to be named channel[@N]. This should
help avoid problems with devices which may contain also other but ADC
child nodes. Quick grep from arch/* with the sun20i-gpadc's compatible
string didn't reveal any in-tree .dts with channel nodes named
otherwise. Also, same grep shows all the in-tree .dts seem to have
channel IDs between 0..num of channels.

Use the new helper.

Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>

---
Revision history:
v6 => v7:
 - Fix function name in the commit message
v5 => v6:
 - Commit message typofix
v4 => v5:
 - Drop the diff-channel stuff from the commit message
v3 => v4:
 - Adapt to 'drop diff-channel support' changes to ADC-helpers
 - select ADC helpers in the Kconfig
v2 => v3:
 - New patch

I picked the sun20i-gpadc in this series because it has a straightforward
approach for populating the struct iio_chan_spec. Everything else except
the .channel can use 'template'-data.

This makes the sun20i-gpadc well suited to be an example user of this new
helper. I hope this patch helps to evaluate whether these helpers are worth
the hassle.

The change is compile tested only!! Testing before applying is highly
appreciated (as always!). Also, even though I tried to audit the dts
files for the reg-properties in the channel nodes, use of references
didn't make it easy. I can't guarantee I didn't miss anything.
---
 drivers/iio/adc/Kconfig            |  1 +
 drivers/iio/adc/sun20i-gpadc-iio.c | 38 ++++++++++++------------------
 2 files changed, 16 insertions(+), 23 deletions(-)

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index e4933de0c366..0993008a1586 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -1357,6 +1357,7 @@ config SUN4I_GPADC
 config SUN20I_GPADC
 	tristate "Allwinner D1/T113s/T507/R329 and similar GPADCs driver"
 	depends on ARCH_SUNXI || COMPILE_TEST
+	select IIO_ADC_HELPER
 	help
 	  Say yes here to build support for Allwinner (D1, T113, T507 and R329)
 	  SoCs GPADC. This ADC provides up to 16 channels.
diff --git a/drivers/iio/adc/sun20i-gpadc-iio.c b/drivers/iio/adc/sun20i-gpadc-iio.c
index 136b8d9c294f..bf1db2a3de9b 100644
--- a/drivers/iio/adc/sun20i-gpadc-iio.c
+++ b/drivers/iio/adc/sun20i-gpadc-iio.c
@@ -15,6 +15,7 @@
 #include <linux/property.h>
 #include <linux/reset.h>
 
+#include <linux/iio/adc-helpers.h>
 #include <linux/iio/iio.h>
 
 #define SUN20I_GPADC_DRIVER_NAME	"sun20i-gpadc"
@@ -149,37 +150,27 @@ static void sun20i_gpadc_reset_assert(void *data)
 	reset_control_assert(rst);
 }
 
+static const struct iio_chan_spec sun20i_gpadc_chan_template = {
+	.type = IIO_VOLTAGE,
+	.indexed = 1,
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
+};
+
 static int sun20i_gpadc_alloc_channels(struct iio_dev *indio_dev,
 				       struct device *dev)
 {
-	unsigned int channel;
-	int num_channels, i, ret;
+	int num_channels;
 	struct iio_chan_spec *channels;
 
-	num_channels = device_get_child_node_count(dev);
+	num_channels = devm_iio_adc_device_alloc_chaninfo_se(dev,
+				&sun20i_gpadc_chan_template, -1, &channels);
+	if (num_channels < 0)
+		return num_channels;
+
 	if (num_channels == 0)
 		return dev_err_probe(dev, -ENODEV, "no channel children\n");
 
-	channels = devm_kcalloc(dev, num_channels, sizeof(*channels),
-				GFP_KERNEL);
-	if (!channels)
-		return -ENOMEM;
-
-	i = 0;
-	device_for_each_child_node_scoped(dev, node) {
-		ret = fwnode_property_read_u32(node, "reg", &channel);
-		if (ret)
-			return dev_err_probe(dev, ret, "invalid channel number\n");
-
-		channels[i].type = IIO_VOLTAGE;
-		channels[i].indexed = 1;
-		channels[i].channel = channel;
-		channels[i].info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
-		channels[i].info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE);
-
-		i++;
-	}
-
 	indio_dev->channels = channels;
 	indio_dev->num_channels = num_channels;
 
@@ -271,3 +262,4 @@ module_platform_driver(sun20i_gpadc_driver);
 MODULE_DESCRIPTION("ADC driver for sunxi platforms");
 MODULE_AUTHOR("Maksim Kiselev <bigunclemax@gmail.com>");
 MODULE_LICENSE("GPL");
+MODULE_IMPORT_NS("IIO_DRIVER");
-- 
2.48.1


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v7 05/10] iio: adc: sun20i-gpadc: Use adc-helpers

[Andy Shevchenko]

On Thu, Mar 13, 2025 at 09:18:49AM +0200, Matti Vaittinen wrote:
> The new devm_iio_adc_device_alloc_chaninfo_se() -helper is intended to
> help drivers avoid open-coding the for_each_node -loop for getting the
> channel IDs. The helper provides standard way to detect the ADC channel
> nodes (by the node name), and a standard way to convert the "reg"
> -properties to channel identification numbers, used in the struct
> iio_chan_spec. Furthermore, the helper can optionally check the found
> channel IDs are smaller than given maximum. This is useful for callers
> which later use the IDs for example for indexing a channel data array.
> 
> The original driver treated all found child nodes as channel nodes. The
> new helper requires channel nodes to be named channel[@N]. This should
> help avoid problems with devices which may contain also other but ADC
> child nodes. Quick grep from arch/* with the sun20i-gpadc's compatible
> string didn't reveal any in-tree .dts with channel nodes named
> otherwise. Also, same grep shows all the in-tree .dts seem to have
> channel IDs between 0..num of channels.
> 
> Use the new helper.

...

> +	num_channels = devm_iio_adc_device_alloc_chaninfo_se(dev,
> +				&sun20i_gpadc_chan_template, -1, &channels);
> +	if (num_channels < 0)
> +		return num_channels;
> +
>  	if (num_channels == 0)
>  		return dev_err_probe(dev, -ENODEV, "no channel children\n");

Note, this what I would expected in your helper to see, i.e. separated cases
for < 0 (error code) and == 0, no channels.

Also, are all users going to have this check? Usually in other similar APIs
we return -ENOENT. And user won't need to have an additional check in case of
0 being considered as an error case too.




-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v7 05/10] iio: adc: sun20i-gpadc: Use adc-helpers

[Jonathan Cameron]

On Thu, 13 Mar 2025 14:34:24 +0200
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Thu, Mar 13, 2025 at 09:18:49AM +0200, Matti Vaittinen wrote:
> > The new devm_iio_adc_device_alloc_chaninfo_se() -helper is intended to
> > help drivers avoid open-coding the for_each_node -loop for getting the
> > channel IDs. The helper provides standard way to detect the ADC channel
> > nodes (by the node name), and a standard way to convert the "reg"
> > -properties to channel identification numbers, used in the struct
> > iio_chan_spec. Furthermore, the helper can optionally check the found
> > channel IDs are smaller than given maximum. This is useful for callers
> > which later use the IDs for example for indexing a channel data array.
> > 
> > The original driver treated all found child nodes as channel nodes. The
> > new helper requires channel nodes to be named channel[@N]. This should
> > help avoid problems with devices which may contain also other but ADC
> > child nodes. Quick grep from arch/* with the sun20i-gpadc's compatible
> > string didn't reveal any in-tree .dts with channel nodes named
> > otherwise. Also, same grep shows all the in-tree .dts seem to have
> > channel IDs between 0..num of channels.
> > 
> > Use the new helper.  
> 
> ...
> 
> > +	num_channels = devm_iio_adc_device_alloc_chaninfo_se(dev,
> > +				&sun20i_gpadc_chan_template, -1, &channels);
> > +	if (num_channels < 0)
> > +		return num_channels;
> > +
> >  	if (num_channels == 0)
> >  		return dev_err_probe(dev, -ENODEV, "no channel children\n");  
> 
> Note, this what I would expected in your helper to see, i.e. separated cases
> for < 0 (error code) and == 0, no channels.
> 
> Also, are all users going to have this check? Usually in other similar APIs
> we return -ENOENT. And user won't need to have an additional check in case of
> 0 being considered as an error case too.
In a few cases we'll need to do the dance the other way in the caller.
So specifically check for -ENOENT and not treat it as an error.

That stems from channel nodes being optionally added to drivers after
they have been around a while (usually to add more specific configuration)
and needing to maintain old behaviour of presenting all channels with default
settings.

I agree that returning -ENOENT is a reasonable way to handle this.

Jonathan

> 
> 
> 
> 

Re: [PATCH v7 05/10] iio: adc: sun20i-gpadc: Use adc-helpers

[Matti Vaittinen]

On 16/03/2025 11:41, Jonathan Cameron wrote:
> On Thu, 13 Mar 2025 14:34:24 +0200
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> 
>> On Thu, Mar 13, 2025 at 09:18:49AM +0200, Matti Vaittinen wrote:
>>> The new devm_iio_adc_device_alloc_chaninfo_se() -helper is intended to
>>> help drivers avoid open-coding the for_each_node -loop for getting the
>>> channel IDs. The helper provides standard way to detect the ADC channel
>>> nodes (by the node name), and a standard way to convert the "reg"
>>> -properties to channel identification numbers, used in the struct
>>> iio_chan_spec. Furthermore, the helper can optionally check the found
>>> channel IDs are smaller than given maximum. This is useful for callers
>>> which later use the IDs for example for indexing a channel data array.
>>>
>>> The original driver treated all found child nodes as channel nodes. The
>>> new helper requires channel nodes to be named channel[@N]. This should
>>> help avoid problems with devices which may contain also other but ADC
>>> child nodes. Quick grep from arch/* with the sun20i-gpadc's compatible
>>> string didn't reveal any in-tree .dts with channel nodes named
>>> otherwise. Also, same grep shows all the in-tree .dts seem to have
>>> channel IDs between 0..num of channels.
>>>
>>> Use the new helper.
>>
>> ...
>>
>>> +	num_channels = devm_iio_adc_device_alloc_chaninfo_se(dev,
>>> +				&sun20i_gpadc_chan_template, -1, &channels);
>>> +	if (num_channels < 0)
>>> +		return num_channels;
>>> +
>>>   	if (num_channels == 0)
>>>   		return dev_err_probe(dev, -ENODEV, "no channel children\n");
>>
>> Note, this what I would expected in your helper to see, i.e. separated cases
>> for < 0 (error code) and == 0, no channels.
>>
>> Also, are all users going to have this check? Usually in other similar APIs
>> we return -ENOENT. And user won't need to have an additional check in case of
>> 0 being considered as an error case too.
> In a few cases we'll need to do the dance the other way in the caller.
> So specifically check for -ENOENT and not treat it as an error.
> 
> That stems from channel nodes being optionally added to drivers after
> they have been around a while (usually to add more specific configuration)
> and needing to maintain old behaviour of presenting all channels with default
> settings.
> 
> I agree that returning -ENOENT is a reasonable way to handle this.

I agree - but I'm going to use -ENODEV instead of -ENOENT because that's 
what the current callers return if they find no channels. That way the 
drivers can return the value directly without converting -ENOENT to -ENODEV.

Yours,
	-- Matti

Re: [PATCH v7 05/10] iio: adc: sun20i-gpadc: Use adc-helpers

[Andy Shevchenko]

On Mon, Mar 17, 2025 at 09:11:08AM +0200, Matti Vaittinen wrote:
> On 16/03/2025 11:41, Jonathan Cameron wrote:
> > On Thu, 13 Mar 2025 14:34:24 +0200
> > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> > > On Thu, Mar 13, 2025 at 09:18:49AM +0200, Matti Vaittinen wrote:

...

> > > > +	num_channels = devm_iio_adc_device_alloc_chaninfo_se(dev,
> > > > +				&sun20i_gpadc_chan_template, -1, &channels);
> > > > +	if (num_channels < 0)
> > > > +		return num_channels;
> > > > +
> > > >   	if (num_channels == 0)
> > > >   		return dev_err_probe(dev, -ENODEV, "no channel children\n");
> > > 
> > > Note, this what I would expected in your helper to see, i.e. separated cases
> > > for < 0 (error code) and == 0, no channels.
> > > 
> > > Also, are all users going to have this check? Usually in other similar APIs
> > > we return -ENOENT. And user won't need to have an additional check in case of
> > > 0 being considered as an error case too.
> > In a few cases we'll need to do the dance the other way in the caller.
> > So specifically check for -ENOENT and not treat it as an error.
> > 
> > That stems from channel nodes being optionally added to drivers after
> > they have been around a while (usually to add more specific configuration)
> > and needing to maintain old behaviour of presenting all channels with default
> > settings.
> > 
> > I agree that returning -ENOENT is a reasonable way to handle this.
> 
> I agree - but I'm going to use -ENODEV instead of -ENOENT because that's
> what the current callers return if they find no channels. That way the
> drivers can return the value directly without converting -ENOENT to -ENODEV.

ENODEV can be easily clashed with other irrelevant cases, ENOENT is the correct
error code. If drivers return this instead of another error code, nothing bad
happen, it's not an ABI path, correct?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v7 05/10] iio: adc: sun20i-gpadc: Use adc-helpers

[Matti Vaittinen]

On 17/03/2025 09:51, Andy Shevchenko wrote:
> On Mon, Mar 17, 2025 at 09:11:08AM +0200, Matti Vaittinen wrote:
>> On 16/03/2025 11:41, Jonathan Cameron wrote:
>>> On Thu, 13 Mar 2025 14:34:24 +0200
>>> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
>>>> On Thu, Mar 13, 2025 at 09:18:49AM +0200, Matti Vaittinen wrote:
> 
> ...
> 
>>>>> +	num_channels = devm_iio_adc_device_alloc_chaninfo_se(dev,
>>>>> +				&sun20i_gpadc_chan_template, -1, &channels);
>>>>> +	if (num_channels < 0)
>>>>> +		return num_channels;
>>>>> +
>>>>>    	if (num_channels == 0)
>>>>>    		return dev_err_probe(dev, -ENODEV, "no channel children\n");
>>>>
>>>> Note, this what I would expected in your helper to see, i.e. separated cases
>>>> for < 0 (error code) and == 0, no channels.
>>>>
>>>> Also, are all users going to have this check? Usually in other similar APIs
>>>> we return -ENOENT. And user won't need to have an additional check in case of
>>>> 0 being considered as an error case too.
>>> In a few cases we'll need to do the dance the other way in the caller.
>>> So specifically check for -ENOENT and not treat it as an error.
>>>
>>> That stems from channel nodes being optionally added to drivers after
>>> they have been around a while (usually to add more specific configuration)
>>> and needing to maintain old behaviour of presenting all channels with default
>>> settings.
>>>
>>> I agree that returning -ENOENT is a reasonable way to handle this.
>>
>> I agree - but I'm going to use -ENODEV instead of -ENOENT because that's
>> what the current callers return if they find no channels. That way the
>> drivers can return the value directly without converting -ENOENT to -ENODEV.
> 
> ENODEV can be easily clashed with other irrelevant cases,

Can you please explain what cases?

> ENOENT is the correct
> error code.

I kind of agree if we look this from the fwnode perspective. But, when 
we look this from the intended user's perspective, I can very well 
understand the -ENODEV. Returning -ENODEV from ADC driver's probe which 
can't find any of the channels feels correct to me.

> If drivers return this instead of another error code, nothing bad
> happen, it's not an ABI path, correct?

I don't know if failure returned from a probe is an ABI. I still feel 
-ENODEV is correct value, and I don't want to change it for existing 
users - and I think also new ADC drivers should use -ENODEV if they find 
no channels at all.

Besides that I think -ENODEV to be right, changing it to -ENOENT for 
existing callers requires a buy-in from Jonathan (and/or) the driver 
maintainers.

Yours,
	-- Matti

Re: [PATCH v7 05/10] iio: adc: sun20i-gpadc: Use adc-helpers

[Andy Shevchenko]

On Mon, Mar 17, 2025 at 10:42:07AM +0200, Matti Vaittinen wrote:
> On 17/03/2025 09:51, Andy Shevchenko wrote:
> > On Mon, Mar 17, 2025 at 09:11:08AM +0200, Matti Vaittinen wrote:
> > > On 16/03/2025 11:41, Jonathan Cameron wrote:
> > > > On Thu, 13 Mar 2025 14:34:24 +0200
> > > > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> > > > > On Thu, Mar 13, 2025 at 09:18:49AM +0200, Matti Vaittinen wrote:

...

> > > > > > +	num_channels = devm_iio_adc_device_alloc_chaninfo_se(dev,
> > > > > > +				&sun20i_gpadc_chan_template, -1, &channels);
> > > > > > +	if (num_channels < 0)
> > > > > > +		return num_channels;
> > > > > > +
> > > > > >    	if (num_channels == 0)
> > > > > >    		return dev_err_probe(dev, -ENODEV, "no channel children\n");
> > > > > 
> > > > > Note, this what I would expected in your helper to see, i.e. separated cases
> > > > > for < 0 (error code) and == 0, no channels.
> > > > > 
> > > > > Also, are all users going to have this check? Usually in other similar APIs
> > > > > we return -ENOENT. And user won't need to have an additional check in case of
> > > > > 0 being considered as an error case too.
> > > > In a few cases we'll need to do the dance the other way in the caller.
> > > > So specifically check for -ENOENT and not treat it as an error.
> > > > 
> > > > That stems from channel nodes being optionally added to drivers after
> > > > they have been around a while (usually to add more specific configuration)
> > > > and needing to maintain old behaviour of presenting all channels with default
> > > > settings.
> > > > 
> > > > I agree that returning -ENOENT is a reasonable way to handle this.
> > > 
> > > I agree - but I'm going to use -ENODEV instead of -ENOENT because that's
> > > what the current callers return if they find no channels. That way the
> > > drivers can return the value directly without converting -ENOENT to -ENODEV.
> > 
> > ENODEV can be easily clashed with other irrelevant cases,
> 
> Can you please explain what cases?

When it's a code path that returns the same error code for something different.

> > ENOENT is the correct
> > error code.
> 
> I kind of agree if we look this from the fwnode perspective. But, when we
> look this from the intended user's perspective, I can very well understand
> the -ENODEV. Returning -ENODEV from ADC driver's probe which can't find any
> of the channels feels correct to me.

Okay, it seems we have got yet another disagreement as I think this has to
be ENOENT. Because this is related to the firmware description and not real
hardware discovery path. If it is the latter, I may fully agree on ENODEV
choice. But AFAICS it's not the case here.

> > If drivers return this instead of another error code, nothing bad
> > happen, it's not an ABI path, correct?
> 
> I don't know if failure returned from a probe is an ABI. I still feel
> -ENODEV is correct value,

And I have the opposite opinion. I think ENODEV is _incorrect_ choice
in this case.

> and I don't want to change it for existing users -
> and I think also new ADC drivers should use -ENODEV if they find no channels
> at all.

Again, the problem is that you are trying to apply the error code for HW to the
information that comes from FW.

> Besides that I think -ENODEV to be right, changing it to -ENOENT for
> existing callers requires a buy-in from Jonathan (and/or) the driver
> maintainers.

Yeah, will wait for Jonathan to judge, but I think you can find rationale above.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v7 05/10] iio: adc: sun20i-gpadc: Use adc-helpers

[Jonathan Cameron]

On Mon, 17 Mar 2025 11:27:37 +0200
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Mon, Mar 17, 2025 at 10:42:07AM +0200, Matti Vaittinen wrote:
> > On 17/03/2025 09:51, Andy Shevchenko wrote:  
> > > On Mon, Mar 17, 2025 at 09:11:08AM +0200, Matti Vaittinen wrote:  
> > > > On 16/03/2025 11:41, Jonathan Cameron wrote:  
> > > > > On Thu, 13 Mar 2025 14:34:24 +0200
> > > > > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:  
> > > > > > On Thu, Mar 13, 2025 at 09:18:49AM +0200, Matti Vaittinen wrote:  
> 
> ...
> 
> > > > > > > +	num_channels = devm_iio_adc_device_alloc_chaninfo_se(dev,
> > > > > > > +				&sun20i_gpadc_chan_template, -1, &channels);
> > > > > > > +	if (num_channels < 0)
> > > > > > > +		return num_channels;
> > > > > > > +
> > > > > > >    	if (num_channels == 0)
> > > > > > >    		return dev_err_probe(dev, -ENODEV, "no channel children\n");  
> > > > > > 
> > > > > > Note, this what I would expected in your helper to see, i.e. separated cases
> > > > > > for < 0 (error code) and == 0, no channels.
> > > > > > 
> > > > > > Also, are all users going to have this check? Usually in other similar APIs
> > > > > > we return -ENOENT. And user won't need to have an additional check in case of
> > > > > > 0 being considered as an error case too.  
> > > > > In a few cases we'll need to do the dance the other way in the caller.
> > > > > So specifically check for -ENOENT and not treat it as an error.
> > > > > 
> > > > > That stems from channel nodes being optionally added to drivers after
> > > > > they have been around a while (usually to add more specific configuration)
> > > > > and needing to maintain old behaviour of presenting all channels with default
> > > > > settings.
> > > > > 
> > > > > I agree that returning -ENOENT is a reasonable way to handle this.  
> > > > 
> > > > I agree - but I'm going to use -ENODEV instead of -ENOENT because that's
> > > > what the current callers return if they find no channels. That way the
> > > > drivers can return the value directly without converting -ENOENT to -ENODEV.  
> > > 
> > > ENODEV can be easily clashed with other irrelevant cases,  
> > 
> > Can you please explain what cases?  
> 
> When it's a code path that returns the same error code for something different.
> 
> > > ENOENT is the correct
> > > error code.  
> > 
> > I kind of agree if we look this from the fwnode perspective. But, when we
> > look this from the intended user's perspective, I can very well understand
> > the -ENODEV. Returning -ENODEV from ADC driver's probe which can't find any
> > of the channels feels correct to me.  
> 
> Okay, it seems we have got yet another disagreement as I think this has to
> be ENOENT. Because this is related to the firmware description and not real
> hardware discovery path. If it is the latter, I may fully agree on ENODEV
> choice. But AFAICS it's not the case here.

I'd not rule so strongly but -ENOENT is fine and there should be no fall out from
standardizing around that.

> 
> > > If drivers return this instead of another error code, nothing bad
> > > happen, it's not an ABI path, correct?  
> > 
> > I don't know if failure returned from a probe is an ABI. I still feel
> > -ENODEV is correct value,  
> 
> And I have the opposite opinion. I think ENODEV is _incorrect_ choice
> in this case.
> 
> > and I don't want to change it for existing users -
> > and I think also new ADC drivers should use -ENODEV if they find no channels
> > at all.  
> 
> Again, the problem is that you are trying to apply the error code for HW to the
> information that comes from FW.
> 
> > Besides that I think -ENODEV to be right, changing it to -ENOENT for
> > existing callers requires a buy-in from Jonathan (and/or) the driver
> > maintainers.  
> 
> Yeah, will wait for Jonathan to judge, but I think you can find rationale above.

The distinction between hardware not there (-ENODEV) and missing stuff in 
FW (-ENOENT) seems reasonable to make. If anyone is relying on capturing
a specific error code and doing anything much with it beyond logging
for debug then they are very optimistic as this stuff is often
not particularly stable over kernel versions. That sort of error code specific
handling only makes sense very locally, not in probe() return values.

Hence my preference here is switch to -ENOENT and we'll go with that for
new code in general that hits this sort of problem.   I'd not consider
older code returning -ENODEV buggy as such, just slightly less than
ideal. So any changes for now probably belong in series touching the code
for other reasons rather than a mass cleanup.

Jonathan

[PATCH v7 07/10] MAINTAINERS: Add IIO ADC helpers

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 804 bytes --]

Add undersigned as a maintainer for the IIO ADC helpers.

Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
---
Revision history:
v2 =>
 - No changes
RFC v1 => v2:
 - New patch
---
 MAINTAINERS | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 8e0736dc2ee0..5b96fb864227 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11208,6 +11208,13 @@ L:	linux-media@vger.kernel.org
 S:	Maintained
 F:	drivers/media/rc/iguanair.c
 
+IIO ADC HELPERS
+M:	Matti Vaittinen <mazziesaccount@gmail.com>
+L:	linux-iio@vger.kernel.org
+S:	Maintained
+F:	drivers/iio/adc/industrialio-adc.c
+F:	include/linux/iio/adc-helpers.h
+
 IIO BACKEND FRAMEWORK
 M:	Nuno Sa <nuno.sa@analog.com>
 R:	Olivier Moysan <olivier.moysan@foss.st.com>
-- 
2.48.1


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]