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

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

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

> 
> 
> 
>

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

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

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

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

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