Support ROHM BD79124 ADC

drivers/base/property.c +46 -0

+/**
+ * fwnode_get_child_node_count_named - number of child nodes with given name
+ * @fwnode: Node which child nodes are counted.
+ * @name: String to match child node name against.
+ *
+ * Scan child nodes and count all the nodes with a specific name. Return the
+ * number of found nodes. Potential '@number' -ending for scanned names is
+ * ignored. Eg,
+ * device_get_child_node_count(dev, "channel");
+ * would match all the nodes:
+ * channel { }, channel@0 {}, channel@0xabba {}...
+ *
+ * Return: the number of child nodes with a matching name for a given device.
+ */
+unsigned int fwnode_get_child_node_count_named(const struct fwnode_handle *fwnode,
+					       const char *name)
+{
+	struct fwnode_handle *child;
+	unsigned int count = 0;
+
+	fwnode_for_each_child_node(fwnode, child)
+		if (fwnode_name_eq(child, name))
+			count++;
+
+	return count;
+}
+EXPORT_SYMBOL_GPL(fwnode_get_child_node_count_named);
+
+/**
+ * device_get_child_node_count_named - number of child nodes with given name
+ * @dev: Device to count the child nodes for.
+ * @name: String to match child node name against.
+ *
+ * Scan device's child nodes and find all the nodes with a specific name and
+ * return the number of found nodes. Potential '@number' -ending for scanned
+ * names is ignored. Eg,
+ * device_get_child_node_count(dev, "channel");
+ * would match all the nodes:
+ * channel { }, channel@0 {}, channel@0xabba {}...
+ *
+ * Return: the number of child nodes with a matching name for a given device.
+ */
+unsigned int device_get_child_node_count_named(const struct device *dev,
+					       const char *name)
+{
+	const struct fwnode_handle *fwnode = dev_fwnode(dev);

Quoted 10 lines

drivers/base/property.c +10 -0

+unsigned int device_get_child_node_count_named(const struct device *dev,
+					       const char *name)
+{
+	const struct fwnode_handle *fwnode = dev_fwnode(dev);
+
+	if (!fwnode)
+		return -EINVAL;
+
+	if (IS_ERR(fwnode))
+		return PTR_ERR(fwnode);

drivers/base/property.c unsigned int device_get_child_node_count(const struct device *dev) · line 1003

Quoted 4 lines

drivers/base/property.c +4 -0

+
+	return fwnode_get_child_node_count_named(fwnode, name);
+}
+EXPORT_SYMBOL_GPL(device_get_child_node_count_named);

Sorry if I missed something in the v4 thread, but why not do all the checks in fwnode_get_child_node_count_named(), and make this an inline function?

        static inline unsigned int
        device_get_child_node_count_named(const struct device *dev, const char *name)
        {
                return fwnode_get_child_node_count_named(dev_fwnode(fwnode), name);
        }

There are some use-cases where child nodes with a specific name need to be parsed. In a few cases the data from the found nodes is added to an array which is allocated based on the number of found nodes. One example of such use is the IIO subsystem's ADC channel nodes, where the relevant nodes are named as channel[@N].

Add a helpers for counting device's sub-nodes with certain name instead of open-coding this in every user.

drivers/base/property.c +2 -0

+	return fwnode_get_child_node_count_named(fwnode, name);
+}

On Mon, Mar 03, 2025 at 01:31:45PM +0200, Matti Vaittinen wrote:

drivers/base/property.c unsigned int device_get_child_node_count(const struct device *dev) · line 999

Quoted 9 lines

drivers/base/property.c +7 -0

+	return count;
+}
 
+	if (!fwnode)
+		return -EINVAL;
+
+	if (IS_ERR(fwnode))
+		return PTR_ERR(fwnode);

I expect that this will return 0 or number of nodes. Why do we need an error code? If it's really required, it should be in the fwnode API above.

Also do we care about secondary fwnodes?

Quoted 3 lines

drivers/base/property.c +2 -0

+	return fwnode_get_child_node_count_named(fwnode, name);
+}

...

Review

Quoted 2 lines

(quoted diff) +2 -0

+unsigned int fwnode_get_child_node_count_named(const struct fwnode_handle *fwnode,
+					       const char *name);

To me the following name sounds better: fwnode_get_named_child_node_count().

include/linux/property.h int fwnode_irq_get(const struct fwnode_handle *fwnode, unsigned int index); · line 215

Quoted 3 lines

include/linux/property.h +2 -0

+unsigned int device_get_child_node_count_named(const struct device *dev,
+					       const char *name);

In the similar way.

Quoted 1 line · Andy Shevchenko

On Mon, Mar 03, 2025 at 01:31:45PM +0200, Matti Vaittinen wrote:

Quoted 1 line

Also do we care about secondary fwnodes?

We have the device_get_child_node_count(). device_get_child_node_count_named() should follow the same logic.

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 <[email protected]>

---
Revision history:
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

drivers/iio/adc/Kconfig +3 -0

index 849c90203071..37b70a65da6f 100644
@@ -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

drivers/iio/adc/Makefile +2 -0

index ee19afba62b7..1c410f483029 100644
@@ -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

drivers/iio/adc/industrialio-adc.c new file +24 -0

new file mode 100644
index 000000000000..7bdae5330224
@@ -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 <[email protected]>
+ */
+
+#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.

drivers/iio/adc/industrialio-adc.c new file +24 -0

+ *             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_child_node_scoped(dev, child) {
+               u32 ch;
+

drivers/iio/adc/industrialio-adc.c new file +4 -0

+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Matti Vaittinen <[email protected]>");
+MODULE_DESCRIPTION("IIO ADC fwnode parsing helpers");

include/linux/iio/adc-helpers.h new file +16 -0

new file mode 100644
index 000000000000..403a70b109ec
@@ -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 <[email protected]>
+ */
+
+#ifndef _INDUSTRIAL_IO_ADC_HELPERS_H_
+#define _INDUSTRIAL_IO_ADC_HELPERS_H_
+
+#include <linux/property.h>
+
+struct device;
+struct iio_chan_spec;
+

include/linux/iio/adc-helpers.h new file +1 -1

+#endif /* _INDUSTRIAL_IO_ADC_HELPERS_H_ */
--

2.48.1

On Mon, Mar 3, 2025 at 12:32 PM Matti Vaittinen
<[email protected]> wrote:

There are some different opinions on this, but on the last patch I did introducing a new namespace,

the consensus

seems to be that putting
the MODULE_IMPORT_NS() in the header file was convenient so that users of the API don't have to remember to both include the header and add the import macro.

Quoted 12 lines · Matti Vaittinen

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.

Quoted 6 lines

Signed-off-by Matti Vaittinen <[email protected]>

---

+ *
+ * Return: Number of found channels on succes. Negative value to indicate

s/succes/success/

Quoted 26 lines

drivers/iio/adc/industrialio-adc.c new file +26 -0

+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_child_node_scoped(dev, child) {
+               u32 ch;
+
+               if (!fwnode_name_eq(child, "channel"))
+                       continue;
+
+               ret = fwnode_property_read_u32(child, "reg", &ch);

drivers/iio/adc/industrialio-adc.c line 68

Quoted 6 lines

drivers/iio/adc/industrialio-adc.c new file +6 -0

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

Should we use return dev_err_probe() on these to help with debugging a bad dtb?

Quoted 6 lines

drivers/iio/adc/industrialio-adc.c new file +6 -0

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

drivers/iio/adc/industrialio-adc.c line 78

Quoted 4 lines

drivers/iio/adc/industrialio-adc.c new file +4 -0

+
+       return num_chan;
+}
+EXPORT_SYMBOL_NS_GPL(devm_iio_adc_device_alloc_chaninfo_se, "IIO_DRIVER");

We can make this less verbose by setting #define DEFAULT_SYMBOL_NAMESPACE at the start of the file. Then we can just do EXPORT_SYMBOL_GPL() throughout the rest of the file.

Also, I would prefer if the namespace matched config name (IIO_ADC_HELPER).

Quoted 4 lines

drivers/iio/adc/industrialio-adc.c new file +4 -0

+
+int devm_iio_adc_device_alloc_chaninfo_se(struct device *dev,
+                                         const struct iio_chan_spec *template,
+                                         int max_chan_id,

include/linux/iio/adc-helpers.h line 26

Quoted 2 lines

include/linux/iio/adc-helpers.h new file +2 -0

+                                         struct iio_chan_spec **cs);
+

There are some different opinions on this, but on the last patch I did introducing a new namespace, the consensus seems to be that putting the MODULE_IMPORT_NS() in the header file was convenient so that users of the API don't have to remember to both include the header and add the import macro.

Thanks for the review David.

Quoted 14 lines · David Lechner

Quoted 12 lines · Matti Vaittinen

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.

Quoted 8 lines

Quoted 6 lines

Signed-off-by Matti Vaittinen <[email protected]>

---

+ *
+ * Return: Number of found channels on succes. Negative value to indicate

s/succes/success/

Thanks!

Quoted 26 lines

Quoted 26 lines

drivers/iio/adc/industrialio-adc.c new file +26 -0

+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_child_node_scoped(dev, child) {
+               u32 ch;
+
+               if (!fwnode_name_eq(child, "channel"))
+                       continue;
+
+               ret = fwnode_property_read_u32(child, "reg", &ch);

Quoted 9 lines

drivers/iio/adc/industrialio-adc.c line 68

Quoted 6 lines

drivers/iio/adc/industrialio-adc.c new file +6 -0

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

Should we use return dev_err_probe() on these to help with debugging a bad dtb?

I am not fan of using dev_err_probe() in a 'library code'. This is because we never know if there'll be some odd use-case where this is not called from the probe.

All in all, I'd leave adding most of the debugs to the callers - especially because we do not expect to have bad device-trees after the initial 'development stage' of a board. The board 'development stage' should really reveal bugs which prevent the channels from being registered - and after the DT is correct, these debug prints become unnecessary (albeit minor) binary bloat.

Quoted 6 lines

Quoted 6 lines

drivers/iio/adc/industrialio-adc.c new file +6 -0

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

Quoted 8 lines

drivers/iio/adc/industrialio-adc.c line 78

Quoted 4 lines

drivers/iio/adc/industrialio-adc.c new file +4 -0

+
+       return num_chan;
+}
+EXPORT_SYMBOL_NS_GPL(devm_iio_adc_device_alloc_chaninfo_se, "IIO_DRIVER");  

We can make this less verbose by setting #define DEFAULT_SYMBOL_NAMESPACE at the start of the file. Then we can just do EXPORT_SYMBOL_GPL() throughout the rest of the file.

I am not sure what to think of this. I use the good old 'ctrl + ]' in my editor when I need to check how a function was supposed to be used. That jumps to the spot of code where the function is. I'd like to see the namespace mentioned there in order to not accidentally miss the fact the function belongs to one.

OTOH, I do like simplifications. Yet, the added simplification might not warrant the namespace not being visible in the function definition.

Quoted 1 line

Also, I would prefer if the namespace matched config name (IIO_ADC_HELPER).

I had some lengthy discussion about this with Andy and Jonathan during earlier review versions. In short, I don't like the idea of very fragmented namespaces in IIO, which will just complicate the drivers without providing any obvious benefit.

https://lore.kernel.org/all/20250222174842.57c091c5@jic23-huawei/

Quoted 4 lines

Quoted 4 lines

drivers/iio/adc/industrialio-adc.c new file +4 -0

+
+int devm_iio_adc_device_alloc_chaninfo_se(struct device *dev,
+                                         const struct iio_chan_spec *template,
+                                         int max_chan_id,

Quoted 9 lines

include/linux/iio/adc-helpers.h line 26

Quoted 2 lines

include/linux/iio/adc-helpers.h new file +2 -0

+                                         struct iio_chan_spec **cs);
+  

There are some different opinions on this, but on the last patch I did introducing a new namespace, the consensus seems to be that putting the MODULE_IMPORT_NS() in the header file was convenient so that users of the API don't have to remember to both include the header and add the import macro.

I do like this suggestion, and I believe this would be the balance between getting the benefit of hiding part of the symbols - while not unnecessarily complicating the callers. I know some people are opposing it though. My personal opinion is that having the MODULE_IMPORT_NS() in a header would be neatly simplifying the calling code with very little harm, especially here where including the header hardly has use-cases outside the IIO ADC.

Unfortunately, the "safety" seems to often be a synonym for just "making it intentionally hard". As Finnish people say: "Kärsi, kärsi, kirkkaamman kruunun saat". :)
(Roughly translated as "Suffer, suffer, you will get a brighter crown").

Let's hear what Jonathan thinks of your suggestion.

Thanks!
-- Matti

Quoted 135 lines · On Wed, 5 Mar 2025 12:54:33 +0200 Matti Vaittinen <[email protected]>…

Thanks for the review David.

Quoted 14 lines · David Lechner

Quoted 12 lines · Matti Vaittinen

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.

Quoted 8 lines

Quoted 6 lines

Signed-off-by Matti Vaittinen <[email protected]>

---

+ *
+ * Return: Number of found channels on succes. Negative value to indicate

s/succes/success/

Thanks!

Quoted 26 lines

Quoted 26 lines

drivers/iio/adc/industrialio-adc.c new file +26 -0

+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_child_node_scoped(dev, child) {
+               u32 ch;
+
+               if (!fwnode_name_eq(child, "channel"))
+                       continue;
+
+               ret = fwnode_property_read_u32(child, "reg", &ch);

Quoted 9 lines

drivers/iio/adc/industrialio-adc.c line 68

Quoted 6 lines

drivers/iio/adc/industrialio-adc.c new file +6 -0

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

Should we use return dev_err_probe() on these to help with debugging a bad dtb?

I am not fan of using dev_err_probe() in a 'library code'. This is because we never know if there'll be some odd use-case where this is not called from the probe.

All in all, I'd leave adding most of the debugs to the callers - especially because we do not expect to have bad device-trees after the initial 'development stage' of a board. The board 'development stage' should really reveal bugs which prevent the channels from being registered - and after the DT is correct, these debug prints become unnecessary (albeit minor) binary bloat.

Quoted 6 lines

Quoted 6 lines

drivers/iio/adc/industrialio-adc.c new file +6 -0

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

Quoted 8 lines

drivers/iio/adc/industrialio-adc.c line 78

Quoted 4 lines

drivers/iio/adc/industrialio-adc.c new file +4 -0

+
+       return num_chan;
+}
+EXPORT_SYMBOL_NS_GPL(devm_iio_adc_device_alloc_chaninfo_se, "IIO_DRIVER");

We can make this less verbose by setting #define DEFAULT_SYMBOL_NAMESPACE at the start of the file. Then we can just do EXPORT_SYMBOL_GPL() throughout the rest of the file.

I am not sure what to think of this. I use the good old 'ctrl + ]' in my editor when I need to check how a function was supposed to be used. That jumps to the spot of code where the function is. I'd like to see the namespace mentioned there in order to not accidentally miss the fact the function belongs to one.

OTOH, I do like simplifications. Yet, the added simplification might not warrant the namespace not being visible in the function definition.

Quoted 1 line

Also, I would prefer if the namespace matched config name (IIO_ADC_HELPER).

I had some lengthy discussion about this with Andy and Jonathan during earlier review versions. In short, I don't like the idea of very fragmented namespaces in IIO, which will just complicate the drivers without providing any obvious benefit.

https://lore.kernel.org/all/20250222174842.57c091c5@jic23-huawei/

Quoted 4 lines

Quoted 4 lines

drivers/iio/adc/industrialio-adc.c new file +4 -0

+
+int devm_iio_adc_device_alloc_chaninfo_se(struct device *dev,
+                                         const struct iio_chan_spec *template,
+                                         int max_chan_id,

Quoted 9 lines

include/linux/iio/adc-helpers.h line 26

Quoted 2 lines

include/linux/iio/adc-helpers.h new file +2 -0

+                                         struct iio_chan_spec **cs);
+

There are some different opinions on this, but on the last patch I did introducing a new namespace, the consensus seems to be that putting the MODULE_IMPORT_NS() in the header file was convenient so that users of the API don't have to remember to both include the header and add the import macro.

I do like this suggestion, and I believe this would be the balance between getting the benefit of hiding part of the symbols - while not unnecessarily complicating the callers. I know some people are opposing it though. My personal opinion is that having the MODULE_IMPORT_NS() in a header would be neatly simplifying the calling code with very little harm, especially here where including the header hardly has use-cases outside the IIO ADC.

Unfortunately, the "safety" seems to often be a synonym for just "making it intentionally hard". As Finnish people say: "Kärsi, kärsi, kirkkaamman kruunun saat". :)
(Roughly translated as "Suffer, suffer, you will get a brighter crown").

Quoted 1 line

Let's hear what Jonathan thinks of your suggestion.

For this particular case my intent was that all the IIO exports that are suitable for use in simple IIO drives will be in this namespace, we just haven't started that conversion yet.

As such, having it defined from a header for this helper isn't a good thing to do.

Generally I prefer to see in driver code what namespaces
are involved but do understand the other viewpoint. In this case I definitely don't think it is appropriate unless we go for a specific namespace for just this helper.

Quoted 138 lines · Jonathan Cameron

Quoted 135 lines · On Wed, 5 Mar 2025 12:54:33 +0200 Matti Vaittinen <[email protected]>…

Thanks for the review David.

Quoted 14 lines · David Lechner

Quoted 12 lines · Matti Vaittinen

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.

Quoted 8 lines

Quoted 6 lines

Signed-off-by Matti Vaittinen <[email protected]>

---

+ *
+ * Return: Number of found channels on succes. Negative value to indicate

s/succes/success/

Thanks!

Quoted 26 lines

Quoted 26 lines

drivers/iio/adc/industrialio-adc.c new file +26 -0

+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_child_node_scoped(dev, child) {
+               u32 ch;
+
+               if (!fwnode_name_eq(child, "channel"))
+                       continue;
+
+               ret = fwnode_property_read_u32(child, "reg", &ch);

Quoted 9 lines

drivers/iio/adc/industrialio-adc.c line 68

Quoted 6 lines

drivers/iio/adc/industrialio-adc.c new file +6 -0

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

Should we use return dev_err_probe() on these to help with debugging a bad dtb?

I am not fan of using dev_err_probe() in a 'library code'. This is because we never know if there'll be some odd use-case where this is not called from the probe.

All in all, I'd leave adding most of the debugs to the callers - especially because we do not expect to have bad device-trees after the initial 'development stage' of a board. The board 'development stage' should really reveal bugs which prevent the channels from being registered - and after the DT is correct, these debug prints become unnecessary (albeit minor) binary bloat.

Quoted 6 lines

Quoted 6 lines

drivers/iio/adc/industrialio-adc.c new file +6 -0

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

Quoted 8 lines

drivers/iio/adc/industrialio-adc.c line 78

Quoted 4 lines

drivers/iio/adc/industrialio-adc.c new file +4 -0

+
+       return num_chan;
+}
+EXPORT_SYMBOL_NS_GPL(devm_iio_adc_device_alloc_chaninfo_se, "IIO_DRIVER");  

We can make this less verbose by setting #define DEFAULT_SYMBOL_NAMESPACE at the start of the file. Then we can just do EXPORT_SYMBOL_GPL() throughout the rest of the file.

I am not sure what to think of this. I use the good old 'ctrl + ]' in my editor when I need to check how a function was supposed to be used. That jumps to the spot of code where the function is. I'd like to see the namespace mentioned there in order to not accidentally miss the fact the function belongs to one.

OTOH, I do like simplifications. Yet, the added simplification might not warrant the namespace not being visible in the function definition.

Quoted 1 line

Also, I would prefer if the namespace matched config name (IIO_ADC_HELPER).

I had some lengthy discussion about this with Andy and Jonathan during earlier review versions. In short, I don't like the idea of very fragmented namespaces in IIO, which will just complicate the drivers without providing any obvious benefit.

https://lore.kernel.org/all/20250222174842.57c091c5@jic23-huawei/

Quoted 4 lines

Quoted 4 lines

drivers/iio/adc/industrialio-adc.c new file +4 -0

+
+int devm_iio_adc_device_alloc_chaninfo_se(struct device *dev,
+                                         const struct iio_chan_spec *template,
+                                         int max_chan_id,

Quoted 9 lines

include/linux/iio/adc-helpers.h line 26

Quoted 2 lines

include/linux/iio/adc-helpers.h new file +2 -0

+                                         struct iio_chan_spec **cs);
+  

There are some different opinions on this, but on the last patch I did introducing a new namespace, the consensus seems to be that putting the MODULE_IMPORT_NS() in the header file was convenient so that users of the API don't have to remember to both include the header and add the import macro.

I do like this suggestion, and I believe this would be the balance between getting the benefit of hiding part of the symbols - while not unnecessarily complicating the callers. I know some people are opposing it though. My personal opinion is that having the MODULE_IMPORT_NS() in a header would be neatly simplifying the calling code with very little harm, especially here where including the header hardly has use-cases outside the IIO ADC.

Unfortunately, the "safety" seems to often be a synonym for just "making it intentionally hard". As Finnish people say: "Kärsi, kärsi, kirkkaamman kruunun saat". :)
(Roughly translated as "Suffer, suffer, you will get a brighter crown").

Quoted 8 lines

Quoted 1 line

Let's hear what Jonathan thinks of your suggestion.

For this particular case my intent was that all the IIO exports that are suitable for use in simple IIO drives will be in this namespace, we just haven't started that conversion yet.

As such, having it defined from a header for this helper isn't a good thing to do.

Hmm. I agree.

Quoted 4 lines

Generally I prefer to see in driver code what namespaces
are involved but do understand the other viewpoint. In this case I definitely don't think it is appropriate unless we go for a specific namespace for just this helper.

I suppose having the MODULE_IMPORT_NS() in the header would actually make the more fine-grained namespaces (like IIO_ADC_HELPERS, IIO_GTS, ...) much more usable. That'd relieved the drivers from explicitly listing multiple namespaces while nicely limiting the visibility.

If IIO was my territory, I might want to ask people to go with that approach - but I am quite happy being a freeloade.. errm, I mean, bystander ;)

Thanks!

Yours,
-- Matti

The new devm_iio_adc_device_alloc_chaninfo() -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" -propereties 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 rzg2l_adc's compatible string didn't reveal any in-tree .dts with channel nodes named otherwise. Also, same grep shows all the .dts seem to have channel IDs between 0..num of channels.

Use the new helper.

Signed-off-by Matti Vaittinen <[email protected]>

The new devm_iio_adc_device_alloc_chaninfo() -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" -propereties 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 <[email protected]>

Device pointer is the only variable which is used by the ads7924_get_channels_config() and which is declared outside this function. Still, the function gets the iio_device and i2c_client as parameters. The sole caller of this function (probe) already has the device pointer which it can directly pass to the function.

Simplify code by passing the device pointer directly as a parameter instead of digging it from the iio_device's private data.

Signed-off-by Matti Vaittinen <[email protected]>

---
Revision history:
v4 => No changes

This commit is compile-tested only! All further testing is appreciated.

---
 drivers/iio/adc/ti-ads7924.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

drivers/iio/adc/ti-ads7924.c +2 -5

index 66b54c0d75aa..b1f745f75dbe 100644
@@ -251,11 +251,8 @@ static const struct iio_info ads7924_info = {
 	.read_raw = ads7924_read_raw,
 };
 
-static int ads7924_get_channels_config(struct i2c_client *client,
-				       struct iio_dev *indio_dev)
+static int ads7924_get_channels_config(struct device *dev)
 {
-	struct ads7924_data *priv = iio_priv(indio_dev);
-	struct device *dev = priv->dev;
 	struct fwnode_handle *node;
 	int num_channels = 0;
 

@@ -380,7 +377,7 @@ static int ads7924_probe(struct i2c_client *client)
 	indio_dev->num_channels = ARRAY_SIZE(ads7924_channels);
 	indio_dev->info = &ads7924_info;
 
-	ret = ads7924_get_channels_config(client, indio_dev);
+	ret = ads7924_get_channels_config(dev);
 	if (ret < 0)
 		return dev_err_probe(dev, ret,
 				     "failed to get channels configuration\n");

The ROHM BD79124 is a 12-bit, 8-channel, SAR ADC. The ADC supports an automatic measurement mode, with an alarm interrupt for out-of-window measurements. The window is configurable for each channel.

The I2C protocol for manual start of the measurement and data reading is somewhat peculiar. It requires the master to do clock stretching after sending the I2C slave-address until the slave has captured the data. Needless to say this is not well suopported by the I2C controllers.

Thus the driver does not support the BD79124's manual measurement mode but implements the measurements using automatic measurement mode relying on the BD79124's ability of storing latest measurements into register.

The driver does also support configuring the threshold events for detecting the out-of-window events.

The BD79124 keeps asserting IRQ for as long as the measured voltage is out of the configured window. Thus the driver masks the received event for a fixed duration (1 second) when an event is handled. This prevents the user-space from choking on the events

The ADC input pins can be also configured as general purpose outputs. Those pins which don't have corresponding ADC channel node in the device-tree will be controllable as GPO.

Signed-off-by Matti Vaittinen <[email protected]>

obj-$(CONFIG_SC27XX_ADC) += sc27xx_adc.o

drivers/iio/adc/rohm-bd79124.c new file +0 -0

new file mode 100644
index 000000000000..466c7decf8fc
@@ -0,0 +1,1108 @@

drivers/iio/adc/rohm-bd79124.c new file +26 -0

+static const struct regmap_range bd79124_ro_ranges[] = {
 
 
 
+static int bd79124_read_event_value(struct iio_dev *iio_dev,
+				    const struct iio_chan_spec *chan,
+				    enum iio_event_type type,
+				    enum iio_event_direction dir,
+				    enum iio_event_info info, int *val,
+				    int *val2)
+{
+	struct bd79124_data *data = iio_priv(iio_dev);
+	int ret, reg;
+
+	if (chan->channel >= BD79124_MAX_NUM_CHANNELS)
+		return -EINVAL;
+
+	switch (info) {
+	case IIO_EV_INFO_VALUE:
+		if (dir == IIO_EV_DIR_RISING)
+			*val = data->alarm_r_limit[chan->channel];
+		else if (dir == IIO_EV_DIR_FALLING)
+			*val = data->alarm_f_limit[chan->channel];
+		else
+			return -EINVAL;
+
+		return IIO_VAL_INT;
+
+	case IIO_EV_INFO_HYSTERESIS:

drivers/iio/adc/rohm-bd79124.c new file +4 -0

+		 */
+		*val <<= 3;
+
+		return IIO_VAL_INT;

drivers/iio/adc/rohm-bd79124.c new file +2 -0

+
+}

Quoted 21 lines · On Mon, 3 Mar 2025 13:33:39 +0200 Matti Vaittinen <[email protected]>…

The ROHM BD79124 is a 12-bit, 8-channel, SAR ADC. The ADC supports an automatic measurement mode, with an alarm interrupt for out-of-window measurements. The window is configurable for each channel.

The I2C protocol for manual start of the measurement and data reading is somewhat peculiar. It requires the master to do clock stretching after sending the I2C slave-address until the slave has captured the data. Needless to say this is not well suopported by the I2C controllers.

Thus the driver does not support the BD79124's manual measurement mode but implements the measurements using automatic measurement mode relying on the BD79124's ability of storing latest measurements into register.

The driver does also support configuring the threshold events for detecting the out-of-window events.

The BD79124 keeps asserting IRQ for as long as the measured voltage is out of the configured window. Thus the driver masks the received event for a fixed duration (1 second) when an event is handled. This prevents the user-space from choking on the events

Quoted 5 lines

The ADC input pins can be also configured as general purpose outputs. Those pins which don't have corresponding ADC channel node in the device-tree will be controllable as GPO.

Signed-off-by Matti Vaittinen <[email protected]>

Hi Matti

Just a few really trivial comments. If all else in the set was resolved I'd probably have applied with a tweak or two

Thanks,

Jonathan

Quoted 7 lines

  obj-$(CONFIG_SC27XX_ADC) += sc27xx_adc.o

drivers/iio/adc/rohm-bd79124.c new file +0 -0

new file mode 100644
index 000000000000..466c7decf8fc
@@ -0,0 +1,1108 @@

...

drivers/iio/adc/rohm-bd79124.c

Quoted 2 lines

drivers/iio/adc/rohm-bd79124.c new file +2 -0

+
+/* Read-only regs */

Given naming this is pretty obvious.
I would drop the comment

Quoted 47 lines · Jonathan Cameron

Quoted 21 lines · On Mon, 3 Mar 2025 13:33:39 +0200 Matti Vaittinen <[email protected]>…

The ROHM BD79124 is a 12-bit, 8-channel, SAR ADC. The ADC supports an automatic measurement mode, with an alarm interrupt for out-of-window measurements. The window is configurable for each channel.

The I2C protocol for manual start of the measurement and data reading is somewhat peculiar. It requires the master to do clock stretching after sending the I2C slave-address until the slave has captured the data. Needless to say this is not well suopported by the I2C controllers.

Thus the driver does not support the BD79124's manual measurement mode but implements the measurements using automatic measurement mode relying on the BD79124's ability of storing latest measurements into register.

The driver does also support configuring the threshold events for detecting the out-of-window events.

The BD79124 keeps asserting IRQ for as long as the measured voltage is out of the configured window. Thus the driver masks the received event for a fixed duration (1 second) when an event is handled. This prevents the user-space from choking on the events

Quoted 5 lines

The ADC input pins can be also configured as general purpose outputs. Those pins which don't have corresponding ADC channel node in the device-tree will be controllable as GPO.

Signed-off-by Matti Vaittinen <[email protected]>

Hi Matti

Just a few really trivial comments. If all else in the set was resolved I'd probably have applied with a tweak or two

Thanks,

Jonathan

Quoted 7 lines

  obj-$(CONFIG_SC27XX_ADC) += sc27xx_adc.o

drivers/iio/adc/rohm-bd79124.c new file +0 -0

new file mode 100644
index 000000000000..466c7decf8fc
@@ -0,0 +1,1108 @@  

...

Quoted 5 lines

drivers/iio/adc/rohm-bd79124.c

Quoted 2 lines

drivers/iio/adc/rohm-bd79124.c new file +2 -0

+
+/* Read-only regs */  

Given naming this is pretty obvious.
I would drop the comment

I will drop this, although I am not sure it is as self explatonary as one thinks. I've seen people getting this wrong because the logic of regmap-ranges is kind of reversed. (Eg, read-only is done by adding range to wr_tables and not to rd_tables - as a no-range).

Thanks for the review, and rest of the comments just agreed with.

-- Matti

We can avoid open-coding the loop construct which counts firmware child nodes with a specific name by using the newly added device_get_child_node_count_named().

The gianfar driver has such open-coded loop. Replace it with the device_get_child_node_count_named().

It's fair to tell the pros and cons of this patch.
The simplification is there, but it's not a big one. It comes with a cost of getting the property.h included in this driver which currently uses exclusively the of_* APIs.

On Mon, Mar 03, 2025 at 01:34:49PM +0200, Matti Vaittinen wrote:

What about the second loop (in gfar_of_init)?
I mean perhaps we want to have fwnode_for_each_named_child_node() and its device variant that may be also reused in the IIO code and here.

Quoted 5 lines · Andy Shevchenko

On Mon, Mar 03, 2025 at 01:34:49PM +0200, Matti Vaittinen wrote:

What about the second loop (in gfar_of_init)?
I mean perhaps we want to have fwnode_for_each_named_child_node() and its device variant that may be also reused in the IIO code and here.

I agree the fwnode_for_each_named_child_node() would be useful. I think I said that already during the previous review rounds. There is plenty of code which could be converted to use it.

This, however, is far more than I am willing to do in the context of a simple IIO driver addition. The "BD79124 ADC suupport" is already now 10 patches, 2 of which are directly related to it.

I propose adding the for_each_named_child_node() as a separate series with bunch of users appended. That's be plenty of beans to count for those who like following the statistics :)