diff mbox series

[v10,3/8] iio: adc: add helpers for parsing ADC nodes

Message ID f1d8b3e15237947738912c0d297b3e1e21d8b03e.1742560649.git.mazziesaccount@gmail.com (mailing list archive)
State Accepted
Headers show
Series Support ROHM BD79124 ADC | expand

Commit Message

Matti Vaittinen March 24, 2025, 7:13 a.m. UTC
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>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

---
Revision history:
v8 =>
 - No changes
v7 => v8:
 devm_iio_adc_device_alloc_chaninfo_se():
 - Treat all negative values for the max ID as 'don't care'
 - Return -ENOENT instead of '0' if no channels were found.
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 | 82 ++++++++++++++++++++++++++++++
 include/linux/iio/adc-helpers.h    | 27 ++++++++++
 4 files changed, 114 insertions(+)
 create mode 100644 drivers/iio/adc/industrialio-adc.c
 create mode 100644 include/linux/iio/adc-helpers.h

Comments

Marcelo Schmitt March 30, 2025, 8:19 p.m. UTC | #1
Hi Matti,

The new helpers for ADC drivers look good to me.
I am now very late to complain about anything but am leaving some minor comments
below that can be completely ignored.

Reviewed-by: Marcelo Schmitt <marcelo.schmitt1@gmail.com>

Thanks,
Marcelo

On 03/24, 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.
Not sure it's preferred to have ADC channels always declared in dt. That
question was somewhat also raised during ADC doc review [1]. In short, ADC
channel may and may not be declared under ADC dt node. ADC bindings often don't
enforce channels to be declared. On IIO side of things, many ADC drivers just
populate channels even if they are not declared in dt.
The ADCs you are supporting in the other patches of this series seem to require 
dt declared channels though.

[1]: https://lore.kernel.org/linux-iio/20250118155153.2574dbe5@jic23-huawei/

Would something like

A common way of marking pins that can be used as ADC inputs is to add
corresponding channel@N nodes in the device tree as described in the ADC
binding yaml.

be a good rephrasing of the above paragraph?

> 
> 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>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
...
> +static inline int iio_adc_device_num_channels(struct device *dev)
> +{
> +	return device_get_named_child_node_count(dev, "channel");
> +}
I wonder if this function name can eventually become misleading.

In Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml we have
temperature sensor with channel nodes named after external hardware connected to
the sensor, leading to channels having different node names. Can anything like
that ever be accepted for ADC bindings?
Matti Vaittinen March 31, 2025, 5:39 a.m. UTC | #2
Hi Marcelo,

Thanks for the review!

On 30/03/2025 23:19, Marcelo Schmitt wrote:
> Hi Matti,
> 
> The new helpers for ADC drivers look good to me.
> I am now very late to complain about anything but am leaving some minor comments
> below that can be completely ignored.
> 
> Reviewed-by: Marcelo Schmitt <marcelo.schmitt1@gmail.com>
> 
> Thanks,
> Marcelo
> 
> On 03/24, 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.
> Not sure it's preferred to have ADC channels always declared in dt. That
> question was somewhat also raised during ADC doc review [1].

I had missed that doc and the review. Interesting read, thanks for 
pointing it :)

We did also do a bit discussion about this during the review of the 
earlier versions. I am not sure if we found an ultimate common consensus 
though :)

A recap as seen through my eyes:

- It is preferred to have either _all_ or _none_ of the channels 
described in the device tree.
https://lore.kernel.org/all/20250201162631.2eab9a9a@jic23-huawei/

- This, however, is not _always_ required to be followed, and it may be 
impractical in some cases:
https://lore.kernel.org/linux-iio/6f6e6550-5246-476f-9168-5e24151ab165@baylibre.com/#t

- We do have bunch of existing drivers which we need to support. With 
some very different approaches to bindings.
https://lore.kernel.org/linux-iio/20250302032054.1fb8a011@jic23-huawei/


My _personal_ thinking is that:

This means that we can't hide the binding parsing in the IIO-core. We 
can't go and change the channels in existing drivers.

But, we can provide helpers (like this one) for drivers to use. I also 
believe we should still try to have common (and preferred!) approach for 
the _new_ drivers. Eventually, the new ones will be majority. Some of 
the old ones die, and if we keep same practices for new ones, the old 
ones will become rare exceptions while majority follows same principles ;)

> In short, ADC
> channel may and may not be declared under ADC dt node. ADC bindings often don't
> enforce channels to be declared. On IIO side of things, many ADC drivers just
> populate channels even if they are not declared in dt.
> The ADCs you are supporting in the other patches of this series seem to require
> dt declared channels though.
> 
> [1]: https://lore.kernel.org/linux-iio/20250118155153.2574dbe5@jic23-huawei/
> 
> Would something like
> 
> A common way of marking pins that can be used as ADC inputs is to add
> corresponding channel@N nodes in the device tree as described in the ADC
> binding yaml.
> 
> be a good rephrasing of the above paragraph?

Yes, if we don't want to guide new drivers to either have all usable 
channels, or no channels in the device tree.

I think Jonathan said he'll be rebasing this to rc1. I am a newcomer and 
I should not enforce my view over more experienced ones ;) So, feel free 
to reword the description as Marcelo suggests if you don't think we 
should prefer one direction or the other.

>>
>> 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>
>> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>>
> ...
>> +static inline int iio_adc_device_num_channels(struct device *dev)
>> +{
>> +	return device_get_named_child_node_count(dev, "channel");
>> +}
> I wonder if this function name can eventually become misleading.
> 
> In Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml we have
> temperature sensor with channel nodes named after external hardware connected to
> the sensor, leading to channels having different node names. Can anything like
> that ever be accepted for ADC bindings?

My initial thinking is that the hardware which is connected to the ADC 
should have it's own node - and there should be only a reference from 
the ADC to the other hardware's description. I think the connected 
hardware should not be a property of the ADC channel.

Anyways, the current ADC binding (bindings/iio/adc/adc.yaml) says the 
node name must be channel[@xxx] (which, I believe makes sense as it 
makes it easier to understand device-trees for ICs which may provide 
other nodes but ADC channels too).

properties:
   $nodename:
     pattern: "^channel(@[0-9a-f]+)?$"
     description:
       A channel index should match reg.

Yours,
	-- Matti
Jonathan Cameron March 31, 2025, 9:48 a.m. UTC | #3
On Mon, 31 Mar 2025 08:39:35 +0300
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> Hi Marcelo,
> 
> Thanks for the review!
> 
> On 30/03/2025 23:19, Marcelo Schmitt wrote:
> > Hi Matti,
> > 
> > The new helpers for ADC drivers look good to me.
> > I am now very late to complain about anything but am leaving some minor comments
> > below that can be completely ignored.
> > 
> > Reviewed-by: Marcelo Schmitt <marcelo.schmitt1@gmail.com>
> > 
> > Thanks,
> > Marcelo
> > 
> > On 03/24, 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.  
> > Not sure it's preferred to have ADC channels always declared in dt. That
> > question was somewhat also raised during ADC doc review [1].  
> 
> I had missed that doc and the review. Interesting read, thanks for 
> pointing it :)
> 
> We did also do a bit discussion about this during the review of the 
> earlier versions. I am not sure if we found an ultimate common consensus 
> though :)
> 
> A recap as seen through my eyes:
> 
> - It is preferred to have either _all_ or _none_ of the channels 
> described in the device tree.
> https://lore.kernel.org/all/20250201162631.2eab9a9a@jic23-huawei/
> 
> - This, however, is not _always_ required to be followed, and it may be 
> impractical in some cases:
> https://lore.kernel.org/linux-iio/6f6e6550-5246-476f-9168-5e24151ab165@baylibre.com/#t
> 
> - We do have bunch of existing drivers which we need to support. With 
> some very different approaches to bindings.
> https://lore.kernel.org/linux-iio/20250302032054.1fb8a011@jic23-huawei/
> 
> 
> My _personal_ thinking is that:
> 
> This means that we can't hide the binding parsing in the IIO-core. We 
> can't go and change the channels in existing drivers.
> 
> But, we can provide helpers (like this one) for drivers to use. I also 
> believe we should still try to have common (and preferred!) approach for 
> the _new_ drivers. Eventually, the new ones will be majority. Some of 
> the old ones die, and if we keep same practices for new ones, the old 
> ones will become rare exceptions while majority follows same principles ;)
> 
> > In short, ADC
> > channel may and may not be declared under ADC dt node. ADC bindings often don't
> > enforce channels to be declared. On IIO side of things, many ADC drivers just
> > populate channels even if they are not declared in dt.
> > The ADCs you are supporting in the other patches of this series seem to require
> > dt declared channels though.
> > 
> > [1]: https://lore.kernel.org/linux-iio/20250118155153.2574dbe5@jic23-huawei/
> > 
> > Would something like
> > 
> > A common way of marking pins that can be used as ADC inputs is to add
> > corresponding channel@N nodes in the device tree as described in the ADC
> > binding yaml.
> > 
> > be a good rephrasing of the above paragraph?  
> 
> Yes, if we don't want to guide new drivers to either have all usable 
> channels, or no channels in the device tree.
> 
> I think Jonathan said he'll be rebasing this to rc1. I am a newcomer and 
> I should not enforce my view over more experienced ones ;) So, feel free 
> to reword the description as Marcelo suggests if you don't think we 
> should prefer one direction or the other.

I've gone with Marcelo's suggestion because I don't want to be too specific
here given the complex history.   We can absolutely encourage the all or
nothing description going forwards though as it is logical in the vast
majority of cases.


> 
> >>
> >> 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>
> >> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> >>  
> > ...  
> >> +static inline int iio_adc_device_num_channels(struct device *dev)
> >> +{
> >> +	return device_get_named_child_node_count(dev, "channel");
> >> +}  
> > I wonder if this function name can eventually become misleading.
> > 
> > In Documentation/devicetree/bindings/iio/temperature/adi,ltc2983.yaml we have
> > temperature sensor with channel nodes named after external hardware connected to
> > the sensor, leading to channels having different node names. Can anything like
> > that ever be accepted for ADC bindings?  
> 
> My initial thinking is that the hardware which is connected to the ADC 
> should have it's own node - and there should be only a reference from 
> the ADC to the other hardware's description. I think the connected 
> hardware should not be a property of the ADC channel.
> 
> Anyways, the current ADC binding (bindings/iio/adc/adc.yaml) says the 
> node name must be channel[@xxx] (which, I believe makes sense as it 
> makes it easier to understand device-trees for ICs which may provide 
> other nodes but ADC channels too).
> 
> properties:
>    $nodename:
>      pattern: "^channel(@[0-9a-f]+)?$"
>      description:
>        A channel index should match reg.
> 
> Yours,
> 	-- Matti
Matti Vaittinen March 31, 2025, 9:57 a.m. UTC | #4
On 31/03/2025 12:48, Jonathan Cameron wrote:
> On Mon, 31 Mar 2025 08:39:35 +0300
> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> 
>> Hi Marcelo,
>>
>> Thanks for the review!
>>
>> On 30/03/2025 23:19, Marcelo Schmitt wrote:
>>> Hi Matti,
>>>
>>> The new helpers for ADC drivers look good to me.
>>> I am now very late to complain about anything but am leaving some minor comments
>>> below that can be completely ignored.
>>>
>>> Reviewed-by: Marcelo Schmitt <marcelo.schmitt1@gmail.com>
>>>
>>> Thanks,
>>> Marcelo
>>>
>>> On 03/24, 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.
>>> Not sure it's preferred to have ADC channels always declared in dt. That
>>> question was somewhat also raised during ADC doc review [1].
>>
>> I had missed that doc and the review. Interesting read, thanks for
>> pointing it :)
>>
>> We did also do a bit discussion about this during the review of the
>> earlier versions. I am not sure if we found an ultimate common consensus
>> though :)
>>
>> A recap as seen through my eyes:
>>
>> - It is preferred to have either _all_ or _none_ of the channels
>> described in the device tree.
>> https://lore.kernel.org/all/20250201162631.2eab9a9a@jic23-huawei/
>>
>> - This, however, is not _always_ required to be followed, and it may be
>> impractical in some cases:
>> https://lore.kernel.org/linux-iio/6f6e6550-5246-476f-9168-5e24151ab165@baylibre.com/#t
>>
>> - We do have bunch of existing drivers which we need to support. With
>> some very different approaches to bindings.
>> https://lore.kernel.org/linux-iio/20250302032054.1fb8a011@jic23-huawei/
>>
>>
>> My _personal_ thinking is that:
>>
>> This means that we can't hide the binding parsing in the IIO-core. We
>> can't go and change the channels in existing drivers.
>>
>> But, we can provide helpers (like this one) for drivers to use. I also
>> believe we should still try to have common (and preferred!) approach for
>> the _new_ drivers. Eventually, the new ones will be majority. Some of
>> the old ones die, and if we keep same practices for new ones, the old
>> ones will become rare exceptions while majority follows same principles ;)
>>
>>> In short, ADC
>>> channel may and may not be declared under ADC dt node. ADC bindings often don't
>>> enforce channels to be declared. On IIO side of things, many ADC drivers just
>>> populate channels even if they are not declared in dt.
>>> The ADCs you are supporting in the other patches of this series seem to require
>>> dt declared channels though.
>>>
>>> [1]: https://lore.kernel.org/linux-iio/20250118155153.2574dbe5@jic23-huawei/
>>>
>>> Would something like
>>>
>>> A common way of marking pins that can be used as ADC inputs is to add
>>> corresponding channel@N nodes in the device tree as described in the ADC
>>> binding yaml.
>>>
>>> be a good rephrasing of the above paragraph?
>>
>> Yes, if we don't want to guide new drivers to either have all usable
>> channels, or no channels in the device tree.
>>
>> I think Jonathan said he'll be rebasing this to rc1. I am a newcomer and
>> I should not enforce my view over more experienced ones ;) So, feel free
>> to reword the description as Marcelo suggests if you don't think we
>> should prefer one direction or the other.
> 
> I've gone with Marcelo's suggestion because I don't want to be too specific
> here given the complex history.   We can absolutely encourage the all or
> nothing description going forwards though as it is logical in the vast
> majority of cases.

Thanks for taking care of it :)

Yours,
	-- Matti
diff mbox series

Patch

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..b4057230e749
--- /dev/null
+++ b/drivers/iio/adc/industrialio-adc.c
@@ -0,0 +1,82 @@ 
+// 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 negative value 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. Specifically, -ENOENT if no channel nodes were found.
+ */
+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, ret;
+
+	num_chan = iio_adc_device_num_channels(dev);
+	if (num_chan < 0)
+		return num_chan;
+
+	if (!num_chan)
+		return -ENOENT;
+
+	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 >= 0 && 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_ */