[PATCH v4 00/10] Support ROHM BD79124 ADC

[PATCH v4 00/10] Support ROHM BD79124 ADC

[Matti Vaittinen]

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

Support ROHM BD79124 ADC.

This series adds also couple of helper functions for parsing the channel
information from the device tree. There has been some discussion about
how useful these are, and whether they should support also differential
and single ended channel configurations. This version drops support for
those - with the benefit of reduced complexity and easier to use
API.

A few of the patches are examples of drivers which could utilize
these added helpers:
 - 4/9 converts rzg2l_adc to use helpers
 - 5/9 converts sun20i-gpadc to use helpers
 - 6,7/9 makes the ti-ads7924 to respect the channel specification give in
   the device-tree using these helpers.

patch 8/9 is small simplification for the ti-ads7924, and it can be
taken independently from the rest of the series.

NOTE: Patches 4...7 are untested as I lack of relevant HW. They have
been compile tested only.

The ROHM BD79124 ADC itself is quite usual stuff. 12-bit, 8-channel ADC
with threshold monitoring.

Except that:
 - each ADC input pin can be configured as a general purpose output.
 - manually starting an ADC conversion and reading the result would
   require the I2C _master_ to do clock stretching(!) for the duration
   of the conversion... Let's just say this is not well supported.
 - IC supports 'autonomous measurement mode' and storing latest results
   to the result registers. This mode is used by the driver due to the
   "peculiar" I2C when doing manual reads.

Furthermore, the ADC uses this continuous autonomous measuring,
and the IC keeps producing new 'out of window' IRQs if measurements are
out of window - the driver disables the event for 1 seconds when sending
it to user. This prevents generating storm of events

Revision history:
v3 => v4:
 - Drop the ADC helper support for differential channels
 - Drop the ADC helper for getting only channel IDs by fwnode.
 - "Promote" the function counting the number of child nodes with a
   specific name to the property.h (As suggested by Jonathan).
 - Add ADC helpers to a namespace.
 - Rebase on v6.14-rc3
 - More minor changes described in individual patches.
v2 => v3:
 - Restrict BD79124 channel numbers as suggested by Conor and add
   Conor's Reviewed-by tag.
 - Support differential and single-ended inputs
 - Convert couple of existing drivers to use the added ADC helpers
 - Minor fixes based on reviews
Link to v2:
https://lore.kernel.org/all/cover.1738761899.git.mazziesaccount@gmail.com/

RFC v1 => v2:
 - Drop MFD and pinmux.
 - Automatically re-enable events after 1 second.
 - Export fwnode parsing helpers for finding the ADC channels.

---

Matti Vaittinen (10):
  dt-bindings: ROHM BD79124 ADC/GPO
  property: Add device_get_child_node_count_named()
  iio: adc: add helpers for parsing ADC nodes
  iio: adc: rzg2l_adc: Use adc-helpers
  iio: adc: sun20i-gpadc: Use adc-helpers
  iio: adc: ti-ads7924 Drop unnecessary function parameters
  iio: adc: ti-ads7924: Respect device tree config
  iio: adc: Support ROHM BD79124 ADC
  MAINTAINERS: Add IIO ADC helpers
  MAINTAINERS: Add ROHM BD79124 ADC/GPO

 .../bindings/iio/adc/rohm,bd79124.yaml        |  114 ++
 MAINTAINERS                                   |   12 +
 drivers/base/property.c                       |   28 +
 drivers/iio/adc/Kconfig                       |   18 +
 drivers/iio/adc/Makefile                      |    3 +
 drivers/iio/adc/industrialio-adc.c            |   89 ++
 drivers/iio/adc/rohm-bd79124.c                | 1114 +++++++++++++++++
 drivers/iio/adc/rzg2l_adc.c                   |   38 +-
 drivers/iio/adc/sun20i-gpadc-iio.c            |   38 +-
 drivers/iio/adc/ti-ads7924.c                  |   83 +-
 include/linux/iio/adc-helpers.h               |   22 +
 include/linux/property.h                      |    2 +
 12 files changed, 1470 insertions(+), 91 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/rohm,bd79124.yaml
 create mode 100644 drivers/iio/adc/industrialio-adc.c
 create mode 100644 drivers/iio/adc/rohm-bd79124.c
 create mode 100644 include/linux/iio/adc-helpers.h


base-commit: 0ad2507d5d93f39619fc42372c347d6006b64319
-- 
2.48.1


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

[PATCH v4 01/10] dt-bindings: ROHM BD79124 ADC/GPO

[Matti Vaittinen]

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

Add binding document for the ROHM BD79124 ADC / GPO.

ROHM BD79124 is a 8-channel, 12-bit ADC. The input pins can also be used
as general purpose outputs.

Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
---
Revision history:
v2 => v3:
 - Restrict channel numbers to 0-7 as suggested by Conor
RFC v1 => v2:
 - drop MFD and represent directly as ADC
 - drop pinmux and treat all non ADC channel pins as GPOs
---
 .../bindings/iio/adc/rohm,bd79124.yaml        | 114 ++++++++++++++++++
 1 file changed, 114 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/rohm,bd79124.yaml

diff --git a/Documentation/devicetree/bindings/iio/adc/rohm,bd79124.yaml b/Documentation/devicetree/bindings/iio/adc/rohm,bd79124.yaml
new file mode 100644
index 000000000000..503285823376
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/rohm,bd79124.yaml
@@ -0,0 +1,114 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/adc/rohm,bd79124.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: ROHM BD79124 ADC/GPO
+
+maintainers:
+  - Matti Vaittinen <mazziesaccount@gmail.com>
+
+description: |
+  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. ADC input pins can be also configured as general purpose
+  outputs.
+
+properties:
+  compatible:
+    const: rohm,bd79124
+
+  reg:
+    description:
+      I2C slave address.
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  gpio-controller: true
+
+  "#gpio-cells":
+    const: 1
+    description:
+      The pin number.
+
+  vdd-supply: true
+
+  iovdd-supply: true
+
+  "#address-cells":
+    const: 1
+
+  "#size-cells":
+    const: 0
+
+patternProperties:
+  "^channel@[0-7]+$":
+    type: object
+    $ref: /schemas/iio/adc/adc.yaml#
+    description: Represents ADC channel.
+
+    properties:
+      reg:
+        description: AIN pin number
+        minimum: 0
+        maximum: 7
+
+    required:
+      - reg
+
+    additionalProperties: false
+
+required:
+  - compatible
+  - reg
+  - iovdd-supply
+  - vdd-supply
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+    #include <dt-bindings/leds/common.h>
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+        adc: adc@10 {
+            compatible = "rohm,bd79124";
+            reg = <0x10>;
+
+            interrupt-parent = <&gpio1>;
+            interrupts = <29 8>;
+
+            vdd-supply = <&dummyreg>;
+            iovdd-supply = <&dummyreg>;
+
+            #address-cells = <1>;
+            #size-cells = <0>;
+
+            channel@0 {
+                reg = <0>;
+            };
+            channel@1 {
+                reg = <1>;
+            };
+            channel@2 {
+                reg = <2>;
+            };
+            channel@3 {
+                reg = <3>;
+            };
+            channel@4 {
+                reg = <4>;
+            };
+            channel@5 {
+                reg = <5>;
+            };
+            channel@6 {
+                reg = <6>;
+            };
+        };
+    };
-- 
2.48.1


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

[PATCH v4 02/10] property: Add device_get_child_node_count_named()

[Matti Vaittinen]

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

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 helper for counting device's sub-nodes with certain name instead
of open-coding this in every user.

Suggested-by: Jonathan Cameron <jic23@kernel.org>
Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
---
Revision history:
v3 => v4:
 - New patch as suggested by Jonathan, see discussion in:
https://lore.kernel.org/lkml/20250223161338.5c896280@jic23-huawei/
---
 drivers/base/property.c  | 28 ++++++++++++++++++++++++++++
 include/linux/property.h |  2 ++
 2 files changed, 30 insertions(+)

diff --git a/drivers/base/property.c b/drivers/base/property.c
index c1392743df9c..3f85818183cd 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -945,6 +945,34 @@ unsigned int device_get_child_node_count(const struct device *dev)
 }
 EXPORT_SYMBOL_GPL(device_get_child_node_count);
 
+/**
+ * device_get_child_node_count_named - number of child nodes with given name
+ *
+ * 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 {}...
+ *
+ * @dev: Device to count the child nodes for
+ *
+ * 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)
+{
+	struct fwnode_handle *child;
+	unsigned int count = 0;
+
+	device_for_each_child_node(dev, child)
+		if (fwnode_name_eq(child, "channel"))
+			count++;
+
+	return count;
+}
+EXPORT_SYMBOL_GPL(device_get_child_node_count_named);
+
 bool device_dma_supported(const struct device *dev)
 {
 	return fwnode_call_bool_op(dev_fwnode(dev), device_dma_supported);
diff --git a/include/linux/property.h b/include/linux/property.h
index e214ecd241eb..a2770197f76b 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -209,6 +209,8 @@ int fwnode_irq_get(const struct fwnode_handle *fwnode, unsigned int index);
 int fwnode_irq_get_byname(const struct fwnode_handle *fwnode, const char *name);
 
 unsigned int device_get_child_node_count(const struct device *dev);
+unsigned int device_get_child_node_count_named(const struct device *dev,
+					       const char *name);
 
 static inline int device_property_read_u8(const struct device *dev,
 					  const char *propname, u8 *val)
-- 
2.48.1


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

Re: [PATCH v4 02/10] property: Add device_get_child_node_count_named()

[Heikki Krogerus]

Hi,

> +/**
> + * device_get_child_node_count_named - number of child nodes with given name
> + *
> + * 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 {}...
> + *
> + * @dev: Device to count the child nodes for
> + *
> + * 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)
> +{
> +	struct fwnode_handle *child;
> +	unsigned int count = 0;
> +
> +	device_for_each_child_node(dev, child)
> +		if (fwnode_name_eq(child, "channel"))

s/"channel"/name/ ?

> +			count++;
> +
> +	return count;
> +}
> +EXPORT_SYMBOL_GPL(device_get_child_node_count_named);

I did not check how many users are you proposing for this, but if
there's only one, then IMO this should not be a global function yet.
It just feels to special case to me. But let's see what the others
think.

thanks,

-- 
heikki

Re: [PATCH v4 02/10] property: Add device_get_child_node_count_named()

[Matti Vaittinen]

On 25/02/2025 11:40, Heikki Krogerus wrote:
> Hi,
> 
>> +/**
>> + * device_get_child_node_count_named - number of child nodes with given name
>> + *
>> + * 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 {}...
>> + *
>> + * @dev: Device to count the child nodes for
>> + *
>> + * 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)
>> +{
>> +	struct fwnode_handle *child;
>> +	unsigned int count = 0;
>> +
>> +	device_for_each_child_node(dev, child)
>> +		if (fwnode_name_eq(child, "channel"))
> 
> s/"channel"/name/ ?

Thanks Heikki for spotting this brainfart! :)

> 
>> +			count++;
>> +
>> +	return count;
>> +}
>> +EXPORT_SYMBOL_GPL(device_get_child_node_count_named);
> 
> I did not check how many users are you proposing for this, but if
> there's only one, then IMO this should not be a global function yet.

I have no strong opinion on this. It starts with just 1 user (IIO ADC 
channel stuff), but I've a feeling there are other areas which do 
look-up nodes by name. I suppose "channels" are looked-up in other areas 
of IIO as well. Lookups are probably done outside the IIO as well. I 
haven't audited this, but I wouldn't be surprized if at least LEDs (and 
perhaps clks/regulators?) could find this useful too.

> It just feels to special case to me. But let's see what the others
> think.

Yeah :) And thanks for spotting the "channel" -thing :)

Yours,
	-- Matti

Re: [PATCH v4 02/10] property: Add device_get_child_node_count_named()

[Andy Shevchenko]

On Tue, Feb 25, 2025 at 11:40:16AM +0200, Heikki Krogerus wrote:
> > +/**
> > + * device_get_child_node_count_named - number of child nodes with given name
> > + *
> > + * 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 {}...
> > + *
> > + * @dev: Device to count the child nodes for

This has an inconsistent kernel doc structure in comparison to the rest in this
file.

> > + * 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)
> > +{
> > +	struct fwnode_handle *child;
> > +	unsigned int count = 0;
> > +
> > +	device_for_each_child_node(dev, child)
> > +		if (fwnode_name_eq(child, "channel"))
> 
> s/"channel"/name/ ?
> 
> > +			count++;
> > +
> > +	return count;
> > +}
> > +EXPORT_SYMBOL_GPL(device_get_child_node_count_named);
> 
> I did not check how many users are you proposing for this, but if
> there's only one, then IMO this should not be a global function yet.
> It just feels to special case to me. But let's see what the others
> think.

The problem is that if somebody hides it, we might potentially see
a duplication in the future. So I _slightly_ prefer to publish and
then drop that after a few cycles if no users appear.

Also this misses the test cases.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 02/10] property: Add device_get_child_node_count_named()

[Matti Vaittinen]

On 25/02/2025 12:21, Andy Shevchenko wrote:
> On Tue, Feb 25, 2025 at 11:40:16AM +0200, Heikki Krogerus wrote:
>>> +/**
>>> + * device_get_child_node_count_named - number of child nodes with given name
>>> + *
>>> + * 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 {}...
>>> + *
>>> + * @dev: Device to count the child nodes for
> 
> This has an inconsistent kernel doc structure in comparison to the rest in this
> file.
> 
>>> + * 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)
>>> +{
>>> +	struct fwnode_handle *child;
>>> +	unsigned int count = 0;
>>> +
>>> +	device_for_each_child_node(dev, child)
>>> +		if (fwnode_name_eq(child, "channel"))
>>
>> s/"channel"/name/ ?
>>
>>> +			count++;
>>> +
>>> +	return count;
>>> +}
>>> +EXPORT_SYMBOL_GPL(device_get_child_node_count_named);
>>
>> I did not check how many users are you proposing for this, but if
>> there's only one, then IMO this should not be a global function yet.
>> It just feels to special case to me. But let's see what the others
>> think.
> 
> The problem is that if somebody hides it, we might potentially see
> a duplication in the future. So I _slightly_ prefer to publish and
> then drop that after a few cycles if no users appear.

After taking a very quick grep I spotted one other existing place where 
we might be able to do direct conversion to use this function.

drivers/net/ethernet/freescale/gianfar.c

That'd be 2 users.

While I looked at it, it seems that a 
'device_for_each_named_child_node()' -construct would have a few users.

Yours,
	-- Matti

Re: [PATCH v4 02/10] property: Add device_get_child_node_count_named()

[Andy Shevchenko]

On Tue, Feb 25, 2025 at 12:29:31PM +0200, Matti Vaittinen wrote:
> On 25/02/2025 12:21, Andy Shevchenko wrote:
> > On Tue, Feb 25, 2025 at 11:40:16AM +0200, Heikki Krogerus wrote:
> > > > +/**
> > > > + * device_get_child_node_count_named - number of child nodes with given name
> > > > + *
> > > > + * 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 {}...
> > > > + *
> > > > + * @dev: Device to count the child nodes for
> > 
> > This has an inconsistent kernel doc structure in comparison to the rest in this
> > file.
> > 
> > > > + * 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)
> > > > +{
> > > > +	struct fwnode_handle *child;
> > > > +	unsigned int count = 0;
> > > > +
> > > > +	device_for_each_child_node(dev, child)
> > > > +		if (fwnode_name_eq(child, "channel"))
> > > 
> > > s/"channel"/name/ ?
> > > 
> > > > +			count++;
> > > > +
> > > > +	return count;
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(device_get_child_node_count_named);
> > > 
> > > I did not check how many users are you proposing for this, but if
> > > there's only one, then IMO this should not be a global function yet.
> > > It just feels to special case to me. But let's see what the others
> > > think.
> > 
> > The problem is that if somebody hides it, we might potentially see
> > a duplication in the future. So I _slightly_ prefer to publish and
> > then drop that after a few cycles if no users appear.
> 
> After taking a very quick grep I spotted one other existing place where we
> might be able to do direct conversion to use this function.
> 
> drivers/net/ethernet/freescale/gianfar.c
> 
> That'd be 2 users.

I haven't checked myself, I believe your judgement, but can you add a (rfc?)
patch at the end of this series to show that? With the luckily event of acking
by the network people we may have it already done.

> While I looked at it, it seems that a 'device_for_each_named_child_node()'
> -construct would have a few users.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 02/10] property: Add device_get_child_node_count_named()

[Matti Vaittinen]

On 25/02/2025 12:39, Andy Shevchenko wrote:
> On Tue, Feb 25, 2025 at 12:29:31PM +0200, Matti Vaittinen wrote:
>> On 25/02/2025 12:21, Andy Shevchenko wrote:
>>> On Tue, Feb 25, 2025 at 11:40:16AM +0200, Heikki Krogerus wrote:
>>>>> +/**
>>>>> + * device_get_child_node_count_named - number of child nodes with given name
>>>>> + *
>>>>> + * 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 {}...
>>>>> + *
>>>>> + * @dev: Device to count the child nodes for

...

>>>> I did not check how many users are you proposing for this, but if
>>>> there's only one, then IMO this should not be a global function yet.
>>>> It just feels to special case to me. But let's see what the others
>>>> think.
>>>
>>> The problem is that if somebody hides it, we might potentially see
>>> a duplication in the future. So I _slightly_ prefer to publish and
>>> then drop that after a few cycles if no users appear.
>>
>> After taking a very quick grep I spotted one other existing place where we
>> might be able to do direct conversion to use this function.
>>
>> drivers/net/ethernet/freescale/gianfar.c
>>
>> That'd be 2 users.
> 
> I haven't checked myself, I believe your judgement, but can you add a (rfc?)
> patch at the end of this series to show that? With the luckily event of acking
> by the network people we may have it already done.

Sure. I can add a patch to gianfar when sending the v5 - if this new 
function is not completely NACK'd before that :)

Yours,
	-- Matti

Re: [PATCH v4 02/10] property: Add device_get_child_node_count_named()

[Matti Vaittinen]

On 25/02/2025 12:39, Andy Shevchenko wrote:
> On Tue, Feb 25, 2025 at 12:29:31PM +0200, Matti Vaittinen wrote:
>> On 25/02/2025 12:21, Andy Shevchenko wrote:
>>> On Tue, Feb 25, 2025 at 11:40:16AM +0200, Heikki Krogerus wrote:

...

>>>>
>>>> I did not check how many users are you proposing for this, but if
>>>> there's only one, then IMO this should not be a global function yet.
>>>> It just feels to special case to me. But let's see what the others
>>>> think.
>>>
>>> The problem is that if somebody hides it, we might potentially see
>>> a duplication in the future. So I _slightly_ prefer to publish and
>>> then drop that after a few cycles if no users appear.
>>
>> After taking a very quick grep I spotted one other existing place where we
>> might be able to do direct conversion to use this function.
>>
>> drivers/net/ethernet/freescale/gianfar.c
>>
>> That'd be 2 users.
> 
> I haven't checked myself, I believe your judgement,

I took a better look and you obviously shouldn't believe :) The gianfar 
used of_node instead of the fwnode. So, it'd be a single caller at starters.

Yours,
	-- Matti

Re: [PATCH v4 02/10] property: Add device_get_child_node_count_named()

[Andy Shevchenko]

On Tue, Feb 25, 2025 at 03:29:17PM +0200, Matti Vaittinen wrote:
> On 25/02/2025 12:39, Andy Shevchenko wrote:
> > On Tue, Feb 25, 2025 at 12:29:31PM +0200, Matti Vaittinen wrote:
> > > On 25/02/2025 12:21, Andy Shevchenko wrote:
> > > > On Tue, Feb 25, 2025 at 11:40:16AM +0200, Heikki Krogerus wrote:

...

> > > > > 
> > > > > I did not check how many users are you proposing for this, but if
> > > > > there's only one, then IMO this should not be a global function yet.
> > > > > It just feels to special case to me. But let's see what the others
> > > > > think.
> > > > 
> > > > The problem is that if somebody hides it, we might potentially see
> > > > a duplication in the future. So I _slightly_ prefer to publish and
> > > > then drop that after a few cycles if no users appear.
> > > 
> > > After taking a very quick grep I spotted one other existing place where we
> > > might be able to do direct conversion to use this function.
> > > 
> > > drivers/net/ethernet/freescale/gianfar.c
> > > 
> > > That'd be 2 users.
> > 
> > I haven't checked myself, I believe your judgement,
> 
> I took a better look and you obviously shouldn't believe :) The gianfar used
> of_node instead of the fwnode. So, it'd be a single caller at starters.

...which is the same as dev_of_node(), which means that you can use your
function there.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 02/10] property: Add device_get_child_node_count_named()

[Matti Vaittinen]

On 25/02/2025 15:59, Andy Shevchenko wrote:
> On Tue, Feb 25, 2025 at 03:29:17PM +0200, Matti Vaittinen wrote:
>> On 25/02/2025 12:39, Andy Shevchenko wrote:
>>> On Tue, Feb 25, 2025 at 12:29:31PM +0200, Matti Vaittinen wrote:
>>>> On 25/02/2025 12:21, Andy Shevchenko wrote:
>>>>> On Tue, Feb 25, 2025 at 11:40:16AM +0200, Heikki Krogerus wrote:
> 
> ...
> 
>>>>>>
>>>>>> I did not check how many users are you proposing for this, but if
>>>>>> there's only one, then IMO this should not be a global function yet.
>>>>>> It just feels to special case to me. But let's see what the others
>>>>>> think.
>>>>>
>>>>> The problem is that if somebody hides it, we might potentially see
>>>>> a duplication in the future. So I _slightly_ prefer to publish and
>>>>> then drop that after a few cycles if no users appear.
>>>>
>>>> After taking a very quick grep I spotted one other existing place where we
>>>> might be able to do direct conversion to use this function.
>>>>
>>>> drivers/net/ethernet/freescale/gianfar.c
>>>>
>>>> That'd be 2 users.
>>>
>>> I haven't checked myself, I believe your judgement,
>>
>> I took a better look and you obviously shouldn't believe :) The gianfar used
>> of_node instead of the fwnode. So, it'd be a single caller at starters.
> 
> ...which is the same as dev_of_node(), which means that you can use your
> function there.

I'm unsure what you mean. The proposed function 
device_get_child_node_count_named() takes device pointer. I don't see 
how dev_of_node() helps converting node to device?

I think I could actually kill the whole gfar_of_group_count() function 
and replace it with a direct call to the 
device_get_child_node_count_named() - but I am not at all convinced 
that'd be worth including the property.h to a file which is currently 
using only of_* -stuff. Well, I suppose it can be asked from netdev 
peeps but I am not convinced they see it as a great idea.

If I misunderstood your meaning - please elaborate.

Yours,
	-- Matti

Re: [PATCH v4 02/10] property: Add device_get_child_node_count_named()

[Andy Shevchenko]

On Wed, Feb 26, 2025 at 04:04:02PM +0200, Matti Vaittinen wrote:
> On 25/02/2025 15:59, Andy Shevchenko wrote:
> > On Tue, Feb 25, 2025 at 03:29:17PM +0200, Matti Vaittinen wrote:
> > > On 25/02/2025 12:39, Andy Shevchenko wrote:
> > > > On Tue, Feb 25, 2025 at 12:29:31PM +0200, Matti Vaittinen wrote:
> > > > > On 25/02/2025 12:21, Andy Shevchenko wrote:
> > > > > > On Tue, Feb 25, 2025 at 11:40:16AM +0200, Heikki Krogerus wrote:

...

> > > > > > > 
> > > > > > > I did not check how many users are you proposing for this, but if
> > > > > > > there's only one, then IMO this should not be a global function yet.
> > > > > > > It just feels to special case to me. But let's see what the others
> > > > > > > think.
> > > > > > 
> > > > > > The problem is that if somebody hides it, we might potentially see
> > > > > > a duplication in the future. So I _slightly_ prefer to publish and
> > > > > > then drop that after a few cycles if no users appear.
> > > > > 
> > > > > After taking a very quick grep I spotted one other existing place where we
> > > > > might be able to do direct conversion to use this function.
> > > > > 
> > > > > drivers/net/ethernet/freescale/gianfar.c
> > > > > 
> > > > > That'd be 2 users.
> > > > 
> > > > I haven't checked myself, I believe your judgement,
> > > 
> > > I took a better look and you obviously shouldn't believe :) The gianfar used
> > > of_node instead of the fwnode. So, it'd be a single caller at starters.
> > 
> > ...which is the same as dev_of_node(), which means that you can use your
> > function there.
> 
> I'm unsure what you mean. The proposed function
> device_get_child_node_count_named() takes device pointer. I don't see how
> dev_of_node() helps converting node to device?

dev_of_node() takes the device pointer and dev_fwnode() takes that as well,
it means that there is no difference which one to use OF-centric or fwnode
API in this particular case. Just make sure that the function (and there
is also a second loop AFAICS) takes struct device *dev instead of struct
device_node *np as a parameter.

> I think I could actually kill the whole gfar_of_group_count() function and
> replace it with a direct call to the device_get_child_node_count_named() -
> but I am not at all convinced that'd be worth including the property.h to a
> file which is currently using only of_* -stuff. Well, I suppose it can be
> asked from netdev peeps but I am not convinced they see it as a great idea.
> 
> If I misunderstood your meaning - please elaborate.

The driver is quite old and has a lot of room to improve. Briefly looking it
may be almost fully converted to fwnode, but it's not your call (only if you
wish). Nevertheless, using agnostic APIs if they reduce code base is fine.
We have drivers that do OF and fwnode mixed approach (for various reasons,
one of which is the new API that is absent in OF realm.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 02/10] property: Add device_get_child_node_count_named()

[Matti Vaittinen]

On 26/02/2025 16:11, Andy Shevchenko wrote:
> On Wed, Feb 26, 2025 at 04:04:02PM +0200, Matti Vaittinen wrote:
>> On 25/02/2025 15:59, Andy Shevchenko wrote:
>>> On Tue, Feb 25, 2025 at 03:29:17PM +0200, Matti Vaittinen wrote:
>>>> On 25/02/2025 12:39, Andy Shevchenko wrote:
>>>>> On Tue, Feb 25, 2025 at 12:29:31PM +0200, Matti Vaittinen wrote:
>>>>>> On 25/02/2025 12:21, Andy Shevchenko wrote:
>>>>>>> On Tue, Feb 25, 2025 at 11:40:16AM +0200, Heikki Krogerus wrote:
> 
> ...
> 
>>>>>>>>
>>>>>>>> I did not check how many users are you proposing for this, but if
>>>>>>>> there's only one, then IMO this should not be a global function yet.
>>>>>>>> It just feels to special case to me. But let's see what the others
>>>>>>>> think.
>>>>>>>
>>>>>>> The problem is that if somebody hides it, we might potentially see
>>>>>>> a duplication in the future. So I _slightly_ prefer to publish and
>>>>>>> then drop that after a few cycles if no users appear.
>>>>>>
>>>>>> After taking a very quick grep I spotted one other existing place where we
>>>>>> might be able to do direct conversion to use this function.
>>>>>>
>>>>>> drivers/net/ethernet/freescale/gianfar.c
>>>>>>
>>>>>> That'd be 2 users.
>>>>>
>>>>> I haven't checked myself, I believe your judgement,
>>>>
>>>> I took a better look and you obviously shouldn't believe :) The gianfar used
>>>> of_node instead of the fwnode. So, it'd be a single caller at starters.
>>>
>>> ...which is the same as dev_of_node(), which means that you can use your
>>> function there.
>>
>> I'm unsure what you mean. The proposed function
>> device_get_child_node_count_named() takes device pointer. I don't see how
>> dev_of_node() helps converting node to device?
> 
> dev_of_node() takes the device pointer and dev_fwnode() takes that as well,
> it means that there is no difference which one to use OF-centric or fwnode

The proposed device_get_child_node_count_named() takes a device pointer. 
I don't see how dev_of_node() helps if there is just of_node and no 
device pointer available in the calling code. (Well, as I wrote below, I 
could alter the gianfar code by dropping the gfar_of_group_count(), so 
that I have the device pointer in caller). Anyways, I don't see how 
dev_of_node() should help unless you're proposing I add a 
of_get_child_node_count_named() or somesuch - which I don't think makes 
sense.

> API in this particular case. Just make sure that the function (and there
> is also a second loop AFAICS) takes struct device *dev instead of struct
> device_node *np as a parameter.

I think I lost the track here :)

>> I think I could actually kill the whole gfar_of_group_count() function and
>> replace it with a direct call to the device_get_child_node_count_named() -
>> but I am not at all convinced that'd be worth including the property.h to a
>> file which is currently using only of_* -stuff. Well, I suppose it can be
>> asked from netdev peeps but I am not convinced they see it as a great idea.
>>
>> If I misunderstood your meaning - please elaborate.
> 
> The driver is quite old

I remember having to modify this driver somewhere around 2010 or so. :) 
Time flies.

> and has a lot of room to improve. Briefly looking it
> may be almost fully converted to fwnode, but it's not your call (only if you
> wish). Nevertheless, using agnostic APIs if they reduce code base is fine.
> We have drivers that do OF and fwnode mixed approach (for various reasons,
> one of which is the new API that is absent in OF realm.

Well, we can propose this to netdev people but I wouldn't be surprized 
if they requested full of_node => fwnode rewrite instead of removing 
simple looking loop and bringing mixture of fwnode and of_node in driver.

Yours,
	-- Matti

Re: [PATCH v4 02/10] property: Add device_get_child_node_count_named()

[Andy Shevchenko]

On Thu, Feb 27, 2025 at 10:01:49AM +0200, Matti Vaittinen wrote:
> On 26/02/2025 16:11, Andy Shevchenko wrote:
> > On Wed, Feb 26, 2025 at 04:04:02PM +0200, Matti Vaittinen wrote:
> > > On 25/02/2025 15:59, Andy Shevchenko wrote:
> > > > On Tue, Feb 25, 2025 at 03:29:17PM +0200, Matti Vaittinen wrote:
> > > > > On 25/02/2025 12:39, Andy Shevchenko wrote:
> > > > > > On Tue, Feb 25, 2025 at 12:29:31PM +0200, Matti Vaittinen wrote:
> > > > > > > On 25/02/2025 12:21, Andy Shevchenko wrote:
> > > > > > > > On Tue, Feb 25, 2025 at 11:40:16AM +0200, Heikki Krogerus wrote:

...

> > > > > > > > > 
> > > > > > > > > I did not check how many users are you proposing for this, but if
> > > > > > > > > there's only one, then IMO this should not be a global function yet.
> > > > > > > > > It just feels to special case to me. But let's see what the others
> > > > > > > > > think.
> > > > > > > > 
> > > > > > > > The problem is that if somebody hides it, we might potentially see
> > > > > > > > a duplication in the future. So I _slightly_ prefer to publish and
> > > > > > > > then drop that after a few cycles if no users appear.
> > > > > > > 
> > > > > > > After taking a very quick grep I spotted one other existing place where we
> > > > > > > might be able to do direct conversion to use this function.
> > > > > > > 
> > > > > > > drivers/net/ethernet/freescale/gianfar.c
> > > > > > > 
> > > > > > > That'd be 2 users.
> > > > > > 
> > > > > > I haven't checked myself, I believe your judgement,
> > > > > 
> > > > > I took a better look and you obviously shouldn't believe :) The gianfar used
> > > > > of_node instead of the fwnode. So, it'd be a single caller at starters.
> > > > 
> > > > ...which is the same as dev_of_node(), which means that you can use your
> > > > function there.
> > > 
> > > I'm unsure what you mean. The proposed function
> > > device_get_child_node_count_named() takes device pointer. I don't see how
> > > dev_of_node() helps converting node to device?
> > 
> > dev_of_node() takes the device pointer and dev_fwnode() takes that as well,
> > it means that there is no difference which one to use OF-centric or fwnode
> 
> The proposed device_get_child_node_count_named() takes a device pointer. I
> don't see how dev_of_node() helps if there is just of_node and no device
> pointer available in the calling code.

???

The loops are working on

	struct device_node *np = pdev->dev.np;

which is the equivalent to

	struct device_node *np = dev_of_node(&pdev->dev);

which takes device pointer.

> (Well, as I wrote below, I could
> alter the gianfar code by dropping the gfar_of_group_count(), so that I have
> the device pointer in caller). Anyways, I don't see how dev_of_node() should
> help unless you're proposing I add a of_get_child_node_count_named() or
> somesuch - which I don't think makes sense.

Are you forbidding yourself to change the function prototype to take a device
pointer instead of device_node one? :-)

> > API in this particular case. Just make sure that the function (and there
> > is also a second loop AFAICS) takes struct device *dev instead of struct
> > device_node *np as a parameter.
> 
> I think I lost the track here :)

Make gfar_of_group_count() to take device pointer. As simple as that.

> > > I think I could actually kill the whole gfar_of_group_count() function and
> > > replace it with a direct call to the device_get_child_node_count_named() -
> > > but I am not at all convinced that'd be worth including the property.h to a
> > > file which is currently using only of_* -stuff. Well, I suppose it can be
> > > asked from netdev peeps but I am not convinced they see it as a great idea.
> > > 
> > > If I misunderstood your meaning - please elaborate.
> > 
> > The driver is quite old
> 
> I remember having to modify this driver somewhere around 2010 or so. :) Time
> flies.
> 
> > and has a lot of room to improve. Briefly looking it
> > may be almost fully converted to fwnode, but it's not your call (only if you
> > wish). Nevertheless, using agnostic APIs if they reduce code base is fine.
> > We have drivers that do OF and fwnode mixed approach (for various reasons,
> > one of which is the new API that is absent in OF realm.
> 
> Well, we can propose this to netdev people but I wouldn't be surprized if
> they requested full of_node => fwnode rewrite instead of removing simple
> looking loop and bringing mixture of fwnode and of_node in driver.

Without doing the proposal we will never know what they will think of all
this...

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 02/10] property: Add device_get_child_node_count_named()

[Matti Vaittinen]

On 27/02/2025 16:49, Andy Shevchenko wrote:
> On Thu, Feb 27, 2025 at 10:01:49AM +0200, Matti Vaittinen wrote:
>> On 26/02/2025 16:11, Andy Shevchenko wrote:
>>> On Wed, Feb 26, 2025 at 04:04:02PM +0200, Matti Vaittinen wrote:
>>>> On 25/02/2025 15:59, Andy Shevchenko wrote:
>>>>> On Tue, Feb 25, 2025 at 03:29:17PM +0200, Matti Vaittinen wrote:
>>>>>> On 25/02/2025 12:39, Andy Shevchenko wrote:
>>>>>>> On Tue, Feb 25, 2025 at 12:29:31PM +0200, Matti Vaittinen wrote:
>>>>>>>> On 25/02/2025 12:21, Andy Shevchenko wrote:
>>>>>>>>> On Tue, Feb 25, 2025 at 11:40:16AM +0200, Heikki Krogerus wrote:
> 
> ...
> 
>>>>>>>>>>
>>>>>>>>>> I did not check how many users are you proposing for this, but if
>>>>>>>>>> there's only one, then IMO this should not be a global function yet.
>>>>>>>>>> It just feels to special case to me. But let's see what the others
>>>>>>>>>> think.
>>>>>>>>>
>>>>>>>>> The problem is that if somebody hides it, we might potentially see
>>>>>>>>> a duplication in the future. So I _slightly_ prefer to publish and
>>>>>>>>> then drop that after a few cycles if no users appear.
>>>>>>>>
>>>>>>>> After taking a very quick grep I spotted one other existing place where we
>>>>>>>> might be able to do direct conversion to use this function.
>>>>>>>>
>>>>>>>> drivers/net/ethernet/freescale/gianfar.c
>>>>>>>>
>>>>>>>> That'd be 2 users.
>>>>>>>
>>>>>>> I haven't checked myself, I believe your judgement,
>>>>>>
>>>>>> I took a better look and you obviously shouldn't believe :) The gianfar used
>>>>>> of_node instead of the fwnode. So, it'd be a single caller at starters.
>>>>>
>>>>> ...which is the same as dev_of_node(), which means that you can use your
>>>>> function there.
>>>>
>>>> I'm unsure what you mean. The proposed function
>>>> device_get_child_node_count_named() takes device pointer. I don't see how
>>>> dev_of_node() helps converting node to device?
>>>
>>> dev_of_node() takes the device pointer and dev_fwnode() takes that as well,
>>> it means that there is no difference which one to use OF-centric or fwnode
>>
>> The proposed device_get_child_node_count_named() takes a device pointer. I
>> don't see how dev_of_node() helps if there is just of_node and no device
>> pointer available in the calling code.
> 
> ???
> 
> The loops are working on
> 
> 	struct device_node *np = pdev->dev.np;
> 
> which is the equivalent to
> 
> 	struct device_node *np = dev_of_node(&pdev->dev);
> 
> which takes device pointer.
> 
>> (Well, as I wrote below, I could
>> alter the gianfar code by dropping the gfar_of_group_count(), so that I have
>> the device pointer in caller). Anyways, I don't see how dev_of_node() should
>> help unless you're proposing I add a of_get_child_node_count_named() or
>> somesuch - which I don't think makes sense.
> 
> Are you forbidding yourself to change the function prototype to take a device
> pointer instead of device_node one? :-)
> 

This is our point of misunderstanding. As I wrote, and as you can see 
from the prototype, the function _is_ taking the device pointer. Hence I 
didn't understand how dev_of_node() should help us.

>>> API in this particular case. Just make sure that the function (and there
>>> is also a second loop AFAICS) takes struct device *dev instead of struct
>>> device_node *np as a parameter.
>>
>> I think I lost the track here :)
> 
> Make gfar_of_group_count() to take device pointer. As simple as that.

that'd just make the gfar_of_group_count() a wrapper of the 
of_get_child_node_count_named(). I prefer killing whole 
gfar_of_group_count().


Yours,
	-- Matti

Re: [PATCH v4 02/10] property: Add device_get_child_node_count_named()

[Rob Herring]

On Thu, Feb 27, 2025 at 9:06 AM Matti Vaittinen
<mazziesaccount@gmail.com> wrote:
>
> On 27/02/2025 16:49, Andy Shevchenko wrote:
> > On Thu, Feb 27, 2025 at 10:01:49AM +0200, Matti Vaittinen wrote:
> >> On 26/02/2025 16:11, Andy Shevchenko wrote:
> >>> On Wed, Feb 26, 2025 at 04:04:02PM +0200, Matti Vaittinen wrote:
> >>>> On 25/02/2025 15:59, Andy Shevchenko wrote:
> >>>>> On Tue, Feb 25, 2025 at 03:29:17PM +0200, Matti Vaittinen wrote:
> >>>>>> On 25/02/2025 12:39, Andy Shevchenko wrote:
> >>>>>>> On Tue, Feb 25, 2025 at 12:29:31PM +0200, Matti Vaittinen wrote:
> >>>>>>>> On 25/02/2025 12:21, Andy Shevchenko wrote:
> >>>>>>>>> On Tue, Feb 25, 2025 at 11:40:16AM +0200, Heikki Krogerus wrote:
> >
> > ...
> >
> >>>>>>>>>>
> >>>>>>>>>> I did not check how many users are you proposing for this, but if
> >>>>>>>>>> there's only one, then IMO this should not be a global function yet.
> >>>>>>>>>> It just feels to special case to me. But let's see what the others
> >>>>>>>>>> think.
> >>>>>>>>>
> >>>>>>>>> The problem is that if somebody hides it, we might potentially see
> >>>>>>>>> a duplication in the future. So I _slightly_ prefer to publish and
> >>>>>>>>> then drop that after a few cycles if no users appear.
> >>>>>>>>
> >>>>>>>> After taking a very quick grep I spotted one other existing place where we
> >>>>>>>> might be able to do direct conversion to use this function.
> >>>>>>>>
> >>>>>>>> drivers/net/ethernet/freescale/gianfar.c
> >>>>>>>>
> >>>>>>>> That'd be 2 users.
> >>>>>>>
> >>>>>>> I haven't checked myself, I believe your judgement,
> >>>>>>
> >>>>>> I took a better look and you obviously shouldn't believe :) The gianfar used
> >>>>>> of_node instead of the fwnode. So, it'd be a single caller at starters.
> >>>>>
> >>>>> ...which is the same as dev_of_node(), which means that you can use your
> >>>>> function there.
> >>>>
> >>>> I'm unsure what you mean. The proposed function
> >>>> device_get_child_node_count_named() takes device pointer. I don't see how
> >>>> dev_of_node() helps converting node to device?
> >>>
> >>> dev_of_node() takes the device pointer and dev_fwnode() takes that as well,
> >>> it means that there is no difference which one to use OF-centric or fwnode
> >>
> >> The proposed device_get_child_node_count_named() takes a device pointer. I
> >> don't see how dev_of_node() helps if there is just of_node and no device
> >> pointer available in the calling code.
> >
> > ???
> >
> > The loops are working on
> >
> >       struct device_node *np = pdev->dev.np;
> >
> > which is the equivalent to
> >
> >       struct device_node *np = dev_of_node(&pdev->dev);
> >
> > which takes device pointer.
> >
> >> (Well, as I wrote below, I could
> >> alter the gianfar code by dropping the gfar_of_group_count(), so that I have
> >> the device pointer in caller). Anyways, I don't see how dev_of_node() should
> >> help unless you're proposing I add a of_get_child_node_count_named() or
> >> somesuch - which I don't think makes sense.
> >
> > Are you forbidding yourself to change the function prototype to take a device
> > pointer instead of device_node one? :-)
> >
>
> This is our point of misunderstanding. As I wrote, and as you can see
> from the prototype, the function _is_ taking the device pointer. Hence I
> didn't understand how dev_of_node() should help us.
>
> >>> API in this particular case. Just make sure that the function (and there
> >>> is also a second loop AFAICS) takes struct device *dev instead of struct
> >>> device_node *np as a parameter.
> >>
> >> I think I lost the track here :)
> >
> > Make gfar_of_group_count() to take device pointer. As simple as that.
>
> that'd just make the gfar_of_group_count() a wrapper of the
> of_get_child_node_count_named(). I prefer killing whole
> gfar_of_group_count().

Sigh. This is not that hard.

- unsigned int num_grps = gfar_of_group_count(np);
+ unsigned int num_grps =
device_get_child_node_count_named(&ofdev->dev, "queue-groups");

And remove gfar_of_group_count() of course.

Rob

Re: [PATCH v4 02/10] property: Add device_get_child_node_count_named()

[Matti Vaittinen]

On 28/02/2025 18:59, Rob Herring wrote:
> On Thu, Feb 27, 2025 at 9:06 AM Matti Vaittinen
> <mazziesaccount@gmail.com> wrote:
>>
>>>
> Sigh. This is not that hard.
> 
> - unsigned int num_grps = gfar_of_group_count(np);
> + unsigned int num_grps =
> device_get_child_node_count_named(&ofdev->dev, "queue-groups");
> 
> And remove gfar_of_group_count() of course.

Thanks Rob. That's what I (already twice) wrote I'll do:

 >>>> alter the gianfar code by dropping the gfar_of_group_count(),

and

 >>  I prefer killing whole
 >> gfar_of_group_count().

I just wanted to understand what Andy suggested.

Yours,
	-- Matti

Re: [PATCH v4 02/10] property: Add device_get_child_node_count_named()

[Matti Vaittinen]

On 25/02/2025 12:21, Andy Shevchenko wrote:
> On Tue, Feb 25, 2025 at 11:40:16AM +0200, Heikki Krogerus wrote:
>>> +/**
>>> + * device_get_child_node_count_named - number of child nodes with given name
>>> + *
>>> + * 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 {}...
>>> + *
>>> + * @dev: Device to count the child nodes for
> 
> This has an inconsistent kernel doc structure in comparison to the rest in this
> file.

Hmm. I'll take a look at the differences for v5.

>>> + * 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)
>>> +{
>>> +	struct fwnode_handle *child;
>>> +	unsigned int count = 0;
>>> +
>>> +	device_for_each_child_node(dev, child)
>>> +		if (fwnode_name_eq(child, "channel"))
>>
>> s/"channel"/name/ ?
>>
>>> +			count++;
>>> +
>>> +	return count;
>>> +}
>>> +EXPORT_SYMBOL_GPL(device_get_child_node_count_named);
>>
>> I did not check how many users are you proposing for this, but if
>> there's only one, then IMO this should not be a global function yet.
>> It just feels to special case to me. But let's see what the others
>> think.
> 
> The problem is that if somebody hides it, we might potentially see
> a duplication in the future. So I _slightly_ prefer to publish and
> then drop that after a few cycles if no users appear.
> 
> Also this misses the test cases.

I'll also take a look at the tests, but I have a bit of an attitude 
problem what comes to unit testing. Adding tests for the sake of having 
tests just hinders the development. It makes improving functions less 
appealing (as tests need to be changed as well) and adds bunch of 
inertia & maintenance cost. Sure, on complex functions having tests 
increases the confidence that changes work - but I don't see much value 
here.

Do we have tests for all the property.h functions?

Yours,
	-- Matti

Re: [PATCH v4 02/10] property: Add device_get_child_node_count_named()

[Rob Herring]

On Mon, Feb 24, 2025 at 12:33 PM Matti Vaittinen
<mazziesaccount@gmail.com> wrote:
>
> 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 helper for counting device's sub-nodes with certain name instead
> of open-coding this in every user.
>
> Suggested-by: Jonathan Cameron <jic23@kernel.org>
> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
> ---
> Revision history:
> v3 => v4:
>  - New patch as suggested by Jonathan, see discussion in:
> https://lore.kernel.org/lkml/20250223161338.5c896280@jic23-huawei/
> ---
>  drivers/base/property.c  | 28 ++++++++++++++++++++++++++++
>  include/linux/property.h |  2 ++
>  2 files changed, 30 insertions(+)
>
> diff --git a/drivers/base/property.c b/drivers/base/property.c
> index c1392743df9c..3f85818183cd 100644
> --- a/drivers/base/property.c
> +++ b/drivers/base/property.c
> @@ -945,6 +945,34 @@ unsigned int device_get_child_node_count(const struct device *dev)
>  }
>  EXPORT_SYMBOL_GPL(device_get_child_node_count);
>
> +/**
> + * device_get_child_node_count_named - number of child nodes with given name
> + *
> + * 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 {}...
> + *
> + * @dev: Device to count the child nodes for
> + *
> + * 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)

I think this should be implemented as
fwnode_get_child_node_count_named() with the device variant being just
a wrapper.

Rob

Re: [PATCH v4 02/10] property: Add device_get_child_node_count_named()

[Andy Shevchenko]

On Fri, Feb 28, 2025 at 11:07:24AM -0600, Rob Herring wrote:
> On Mon, Feb 24, 2025 at 12:33 PM Matti Vaittinen
> <mazziesaccount@gmail.com> wrote:

...

> > +unsigned int device_get_child_node_count_named(const struct device *dev,
> > +                                              const char *name)
> 
> I think this should be implemented as
> fwnode_get_child_node_count_named() with the device variant being just
> a wrapper.

Good catch! That's also added to our misunderstanding with Matti who didn't get
the role of dev_of_node() vs. dev_fwnode() in the first place.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 02/10] property: Add device_get_child_node_count_named()

[Matti Vaittinen]

On 28/02/2025 19:07, Rob Herring wrote:
> On Mon, Feb 24, 2025 at 12:33 PM Matti Vaittinen
> <mazziesaccount@gmail.com> wrote:
>>
>> 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 helper for counting device's sub-nodes with certain name instead
>> of open-coding this in every user.
>>
>> Suggested-by: Jonathan Cameron <jic23@kernel.org>
>> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
>> ---
>> Revision history:
>> v3 => v4:
>>   - New patch as suggested by Jonathan, see discussion in:
>> https://lore.kernel.org/lkml/20250223161338.5c896280@jic23-huawei/
>> ---
>>   drivers/base/property.c  | 28 ++++++++++++++++++++++++++++
>>   include/linux/property.h |  2 ++
>>   2 files changed, 30 insertions(+)
>>
>> diff --git a/drivers/base/property.c b/drivers/base/property.c
>> index c1392743df9c..3f85818183cd 100644
>> --- a/drivers/base/property.c
>> +++ b/drivers/base/property.c
>> @@ -945,6 +945,34 @@ unsigned int device_get_child_node_count(const struct device *dev)
>>   }
>>   EXPORT_SYMBOL_GPL(device_get_child_node_count);
>>
>> +/**
>> + * device_get_child_node_count_named - number of child nodes with given name
>> + *
>> + * 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 {}...
>> + *
>> + * @dev: Device to count the child nodes for
>> + *
>> + * 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)
> 
> I think this should be implemented as
> fwnode_get_child_node_count_named() with the device variant being just
> a wrapper.

I thought of that but it'll mean we had two very little used APIs 
instead of just one. Well, perhaps we see new users though so I'll 
follow this suggestion for v5.

Yours,
	-- Matti

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

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 6197 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:
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

iio: adc: helper: drop headers
---
 drivers/iio/adc/Kconfig            |  3 +
 drivers/iio/adc/Makefile           |  2 +
 drivers/iio/adc/industrialio-adc.c | 89 ++++++++++++++++++++++++++++++
 include/linux/iio/adc-helpers.h    | 22 ++++++++
 4 files changed, 116 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..d8e9e6825d2b
--- /dev/null
+++ b/drivers/iio/adc/industrialio-adc.c
@@ -0,0 +1,89 @@
+// 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>
+
+int iio_adc_device_num_channels(struct device *dev)
+{
+	return device_get_child_node_count_named(dev, "channel");
+}
+EXPORT_SYMBOL_GPL(iio_adc_device_num_channels);
+
+/**
+ * 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 succes. 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_child_node_scoped(dev, child) {
+		u32 ch;
+
+		if (!fwnode_name_eq(child, "channel"))
+			continue;
+
+		ret = fwnode_property_read_u32(child, "reg", &ch);
+		if (ret)
+			return ret;
+
+		if (max_chan_id != -1)
+			if (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..5d64244f1552
--- /dev/null
+++ b/include/linux/iio/adc-helpers.h
@@ -0,0 +1,22 @@
+/* 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_
+
+struct device;
+struct fwnode_handle;
+struct iio_chan_spec;
+
+int iio_adc_device_num_channels(struct device *dev);
+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 v4 03/10] iio: adc: add helpers for parsing ADC nodes

[David Lechner]

On 2/24/25 12:33 PM, 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.

I think "preferred?" is the key question here. Currently, it is assumed
that basically all IIO bindings have channels implicitly even if the
binding doesn't call them out. It just means that there is nothing
special about the channel that needs to be documented, but the channel
is still there.

Similarly, on several drivers we added recently that make use of adc.yaml
(adi,ad7380, adi,ad4695) we wrote the bindings with the intention that
if a channel was wired in the default configuration, then you would just
omit the channel node for that input pin. Therefore, this helper couldn't
be used by these drivers since we always have a fixed number of channels
used in the driver regardless of if there are explicit channel nodes in
the devicetree or not.

In my experience, the only time we don't populate all available channels
on an ADC, even if not used, is in cases like differential chips where
any two inputs can be mixed and matched to form a channel. Some of these,
like adi,ad7173-8 would have 100s or 1000s of channels if we tried to
include all possible channels. In those cases, we make an exception and
use a dynamic number of channels based on the devicetree. But for chips
that have less than 20 total possible channels or so we've always
provided all possible channels to userspace. It makes writing userspace
software for a specific chip easier if we can always assume that chip
has the same number of channels.

> 
> 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>
> 

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

[Matti Vaittinen]

Hi David,

Thanks for taking a look at this :)

On 26/02/2025 02:26, David Lechner wrote:
> On 2/24/25 12:33 PM, 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.
> 
> I think "preferred?" is the key question here. Currently, it is assumed
> that basically all IIO bindings have channels implicitly even if the
> binding doesn't call them out. It just means that there is nothing
> special about the channel that needs to be documented, but the channel
> is still there.

I think this works well with the ADCs which have no other purpose for 
the pins but the ADC. The BD79124 (and some others) do allow muxing the 
ADC input pins for other purposes. There the DT bindings with nothing 
but the "reg" are relevant, and channels can't be trusted to just be 
there without those..

> Similarly, on several drivers we added recently that make use of adc.yaml
> (adi,ad7380, adi,ad4695) we wrote the bindings with the intention that
> if a channel was wired in the default configuration, then you would just
> omit the channel node for that input pin. Therefore, this helper couldn't
> be used by these drivers since we always have a fixed number of channels
> used in the driver regardless of if there are explicit channel nodes in
> the devicetree or not.

I think this works with the ICs where channels, indeed, always are 
there. But this is not the case with _all_ ICs. And in order to keep the 
consistency I'd actually required that if channels are listed in the DT, 
then _all_ the channels must be listed. Else it becomes less 
straightforward for people to understand how many channels there are 
based on the device tree. I believe this was also proposed by Jonathan 
during the v1 review:

 > > Hmm. That'd mean the ADC channels _must_ be defined in DT in order 
to be
 > > usable(?) Well, if this is the usual way, then it should be well known
 > > by users. Thanks.
 >
 > Yes. We basically have two types of binding wrt to channels.
 > 1) Always there - no explicit binding, but also no way to describe
 >    anything specific about the channels.
 > 2) Subnode per channel with stuff from adc.yaml and anything device
 >    specific.  Only channels that that have a node are enabled.
 >
 > There are a few drivers that for historical reasons support both
 > options with 'no channels' meaning 'all channels'.

https://lore.kernel.org/all/20250201162631.2eab9a9a@jic23-huawei/

> In my experience, the only time we don't populate all available channels
> on an ADC, even if not used, is in cases like differential chips where
> any two inputs can be mixed and matched to form a channel. Some of these,
> like adi,ad7173-8 would have 100s or 1000s of channels if we tried to
> include all possible channels. In those cases, we make an exception and
> use a dynamic number of channels based on the devicetree. But for chips
> that have less than 20 total possible channels or so we've always
> provided all possible channels to userspace. It makes writing userspace
> software for a specific chip easier if we can always assume that chip
> has the same number of channels.

In any exception to this rule of describing all channels in DT should 
just avoid using these helpers and do things as they're done now. No one 
is forced to use them. But I am not really sure why would you not 
describe all the channels in the device-tree for ICs with less than 20 
channels? I'd assume that if the channels are unconditionally usable in 
the hardware, then they should be in DT as well(?)

>> 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>
>>

Yours,
	-- Matti

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

[David Lechner]

On 2/26/25 12:28 AM, Matti Vaittinen wrote:
> Hi David,
> 
> Thanks for taking a look at this :)
> 
> On 26/02/2025 02:26, David Lechner wrote:
>> On 2/24/25 12:33 PM, 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.
>>
>> I think "preferred?" is the key question here. Currently, it is assumed
>> that basically all IIO bindings have channels implicitly even if the
>> binding doesn't call them out. It just means that there is nothing
>> special about the channel that needs to be documented, but the channel
>> is still there.
> 
> I think this works well with the ADCs which have no other purpose for the pins but the ADC. The BD79124 (and some others) do allow muxing the ADC input pins for other purposes. There the DT bindings with nothing but the "reg" are relevant, and channels can't be trusted to just be there without those..

Makes sense.

> 
>> Similarly, on several drivers we added recently that make use of adc.yaml
>> (adi,ad7380, adi,ad4695) we wrote the bindings with the intention that
>> if a channel was wired in the default configuration, then you would just
>> omit the channel node for that input pin. Therefore, this helper couldn't
>> be used by these drivers since we always have a fixed number of channels
>> used in the driver regardless of if there are explicit channel nodes in
>> the devicetree or not.
> 
> I think this works with the ICs where channels, indeed, always are there. But this is not the case with _all_ ICs. And in order to keep the consistency I'd actually required that if channels are listed in the DT, then _all_ the channels must be listed. Else it becomes less straightforward for people to understand how many channels there are based on the device tree. I believe this was also proposed by Jonathan during the v1 review:
> 
>> > Hmm. That'd mean the ADC channels _must_ be defined in DT in order to be
>> > usable(?) Well, if this is the usual way, then it should be well known
>> > by users. Thanks.
>>
>> Yes. We basically have two types of binding wrt to channels.
>> 1) Always there - no explicit binding, but also no way to describe
>>    anything specific about the channels.
>> 2) Subnode per channel with stuff from adc.yaml and anything device
>>    specific.  Only channels that that have a node are enabled.
>>

Hmm... does that mean we implemented it wrong on ad7380 and ad4695?

>> There are a few drivers that for historical reasons support both
>> options with 'no channels' meaning 'all channels'.
> 
> https://lore.kernel.org/all/20250201162631.2eab9a9a@jic23-huawei/
> 
>> In my experience, the only time we don't populate all available channels
>> on an ADC, even if not used, is in cases like differential chips where
>> any two inputs can be mixed and matched to form a channel. Some of these,
>> like adi,ad7173-8 would have 100s or 1000s of channels if we tried to
>> include all possible channels. In those cases, we make an exception and
>> use a dynamic number of channels based on the devicetree. But for chips
>> that have less than 20 total possible channels or so we've always
>> provided all possible channels to userspace. It makes writing userspace
>> software for a specific chip easier if we can always assume that chip
>> has the same number of channels.
> 
> In any exception to this rule of describing all channels in DT should just avoid using these helpers and do things as they're done now. No one is forced to use them. But I am not really sure why would you not describe all the channels in the device-tree for ICs with less than 20 channels? I'd assume that if the channels are unconditionally usable in the hardware, then they should be in DT as well(?)

I devicetree, I think the tendency is to be less verbose and only add
properties/nodes when there is something that is not the usual case.
Default values are chosen to be the most usual case so we don't have
to write so much in the .dts.

> 
>>> 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>
>>>
> 
> Yours,
>     -- Matti

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

[Matti Vaittinen]

On 26/02/2025 18:10, David Lechner wrote:
> On 2/26/25 12:28 AM, Matti Vaittinen wrote:
>> Hi David,
>>
>> Thanks for taking a look at this :)
>>
>> On 26/02/2025 02:26, David Lechner wrote:
>>> On 2/24/25 12:33 PM, Matti Vaittinen wrote:

...

>>
>>> Similarly, on several drivers we added recently that make use of adc.yaml
>>> (adi,ad7380, adi,ad4695) we wrote the bindings with the intention that
>>> if a channel was wired in the default configuration, then you would just
>>> omit the channel node for that input pin. Therefore, this helper couldn't
>>> be used by these drivers since we always have a fixed number of channels
>>> used in the driver regardless of if there are explicit channel nodes in
>>> the devicetree or not.
>>
>> I think this works with the ICs where channels, indeed, always are there. But this is not the case with _all_ ICs. And in order to keep the consistency I'd actually required that if channels are listed in the DT, then _all_ the channels must be listed. Else it becomes less straightforward for people to understand how many channels there are based on the device tree. I believe this was also proposed by Jonathan during the v1 review:
>>
>>>> Hmm. That'd mean the ADC channels _must_ be defined in DT in order to be
>>>> usable(?) Well, if this is the usual way, then it should be well known
>>>> by users. Thanks.
>>>
>>> Yes. We basically have two types of binding wrt to channels.
>>> 1) Always there - no explicit binding, but also no way to describe
>>>      anything specific about the channels.
>>> 2) Subnode per channel with stuff from adc.yaml and anything device
>>>      specific.  Only channels that that have a node are enabled.
>>>
> 
> Hmm... does that mean we implemented it wrong on ad7380 and ad4695?

I believe this is a question to Jonathan. With my ADC-driver experience 
I am not the person to answer this :)

_If_ I commented something to this, I would say that: "I believe, this 
question is a good example of why providing helpers is so powerful. In 
my experience, when we provide helpers, then there will be a 'de facto' 
way of doing things, which improves consistency". But as I feel I'm on 
the verge of stepping on someones toes (and I am really the novice on 
this area), I won't say that comment out loud.

>>> There are a few drivers that for historical reasons support both
>>> options with 'no channels' meaning 'all channels'.
>>
>> https://lore.kernel.org/all/20250201162631.2eab9a9a@jic23-huawei/
>>
>>> In my experience, the only time we don't populate all available channels
>>> on an ADC, even if not used, is in cases like differential chips where
>>> any two inputs can be mixed and matched to form a channel. Some of these,
>>> like adi,ad7173-8 would have 100s or 1000s of channels if we tried to
>>> include all possible channels. In those cases, we make an exception and
>>> use a dynamic number of channels based on the devicetree. But for chips
>>> that have less than 20 total possible channels or so we've always
>>> provided all possible channels to userspace. It makes writing userspace
>>> software for a specific chip easier if we can always assume that chip
>>> has the same number of channels.
>>
>> In any exception to this rule of describing all channels in DT should just avoid using these helpers and do things as they're done now. No one is forced to use them. But I am not really sure why would you not describe all the channels in the device-tree for ICs with less than 20 channels? I'd assume that if the channels are unconditionally usable in the hardware, then they should be in DT as well(?)
> 
> I devicetree, I think the tendency is to be less verbose and only add
> properties/nodes when there is something that is not the usual case.
> Default values are chosen to be the most usual case so we don't have
> to write so much in the .dts.

On the other hand, I've received comments from the DTS people to expose 
all HW blocks in the bindings. AFAIR, for example, marking 
power-supplies as 'optional' in bindings is frowned upon, because they 
are in the HW whether the SW needs to control them or not. Hence I think 
marking either all or no channels in dt should be the way to go - but my 
thinking is not done based on the years of experience on ADCs!

>>>> 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>
>>>>
>>
>> Yours,
>>      -- Matti
> 

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

[Jonathan Cameron]

On Thu, 27 Feb 2025 09:46:06 +0200
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> On 26/02/2025 18:10, David Lechner wrote:
> > On 2/26/25 12:28 AM, Matti Vaittinen wrote:  
> >> Hi David,
> >>
> >> Thanks for taking a look at this :)
> >>
> >> On 26/02/2025 02:26, David Lechner wrote:  
> >>> On 2/24/25 12:33 PM, Matti Vaittinen wrote:  
> 
> ...
> 
> >>  
> >>> Similarly, on several drivers we added recently that make use of adc.yaml
> >>> (adi,ad7380, adi,ad4695) we wrote the bindings with the intention that
> >>> if a channel was wired in the default configuration, then you would just
> >>> omit the channel node for that input pin. Therefore, this helper couldn't
> >>> be used by these drivers since we always have a fixed number of channels
> >>> used in the driver regardless of if there are explicit channel nodes in
> >>> the devicetree or not.  
> >>
> >> I think this works with the ICs where channels, indeed, always are there. But this is not the case with _all_ ICs. And in order to keep the consistency I'd actually required that if channels are listed in the DT, then _all_ the channels must be listed. Else it becomes less straightforward for people to understand how many channels there are based on the device tree. I believe this was also proposed by Jonathan during the v1 review:
> >>  
> >>>> Hmm. That'd mean the ADC channels _must_ be defined in DT in order to be
> >>>> usable(?) Well, if this is the usual way, then it should be well known
> >>>> by users. Thanks.  

So there is some history here that complicates things.

1) Originally we always provided all channels.  Easy case :)
2) Along came SoC ADC users who were unhappy with this not so much because
   of the case Matti hit where the channel can be something else but more
   because it's not unusual to either not wire up some pins on an SoC
   or there are multiple packages that we don't otherwise distinguish
   (as no software differences really) in which some internal pins never
   reach the ones on the package.   Various solutions initially existed
   for this (you can find things like xxx,channels properties in some bindings.)
3) Then along came devices where we wanted per channel config.
   There were some 'interesting' bindings for that as well for a while but
   eventually we decided on channel nodes when needed.  Those always allowed
   drivers to supply extra channels that didn't have nodes though (that's
   a driver /binding choice and motivated somewhat by whether the unwired
   pin thing matters - there are ADC package variants where this happens
   but it is rare unlike for SoCs where it seems to be common).
   From this discussion it occurs to me that we maybe want to make sure
   that binding docs state what is expected here clearly.  If there is
   a concept of a 'default' for missing channel nodes then we need to say
   what it is.  Property defaults will give us most of that but don't cover
   everything.
4) Now we had channel nodes we can also use them for (2).  In those cases
   on a device specific case we allow for channels that don't have nodes
   to be hidden.   There is often a fallback for this which is more about
   how bindings evolved (sure they shouldn't evolve but they do unfortunately).
   In those cases, no channel nodes == all channel nodes with default settings.


> >>>
> >>> Yes. We basically have two types of binding wrt to channels.
> >>> 1) Always there - no explicit binding, but also no way to describe
> >>>      anything specific about the channels.
> >>> 2) Subnode per channel with stuff from adc.yaml and anything device
> >>>      specific.  Only channels that that have a node are enabled.
> >>>  
> > 
> > Hmm... does that mean we implemented it wrong on ad7380 and ad4695?  
> 
> I believe this is a question to Jonathan. With my ADC-driver experience 
> I am not the person to answer this :)
> 
> _If_ I commented something to this, I would say that: "I believe, this 
> question is a good example of why providing helpers is so powerful. In 
> my experience, when we provide helpers, then there will be a 'de facto' 
> way of doing things, which improves consistency". But as I feel I'm on 
> the verge of stepping on someones toes (and I am really the novice on 
> this area), I won't say that comment out loud.

Problem is always 'history'.  We already have a bunch of drivers
doing what the parts David called out do.  The bindings are clear and
ultimately it is a bit device specific to whether missing nodes logically
should default to default parameters or be hidden. In some cases there are
natural defaults, in others not even close as we have fully flexible
MUXes in front of differential ADCs and can in theory configure far more
combinations than we even have pins for.

So today the situation is we have all the options in tree and we aren't
really in a position to drop any of them:

a) custom bindings to configure channels - lots of these :(
b) everything on if no channel nodes.  Maybe everything on always.
c) channel nodes necessary for a channel to exist.

If I were starting all this again we'd probably reduce the options but
too late now :(

Only thing I'd request is if a binding uses channel nodes at all.
It should be possible to provide all nodes - whether or not some are
just the defaults.  That way we can advise writers of bindings to
provide all the channels they want to use.  The other cases then
become a case of whether they get more channels than expected, but
never that some they want aren't there!  A binding that didn't do
this wouldn't be wrong, it would just mean the writer read the binding
doc more carefully and knows what is expected for this device rather
than more generally.

There are some 'interesting' is it broken ABI backwards compatibility
questions when we retrofit channel nodes into a binding.  In those cases
we can't hide non specified nodes as it would mean channels disappear that
in an earlier kernel were present.  In theory that should never be a
problem but not all userspace code is going to be sufficient careful
to not be disrupted by channel number changes.  Even this I think we
broke once or twice because of cases like the one Matti has where they
are multipurpose pins on some chip variant we didn't know about when
the driver was written.

Jonathan

> 
> >>> There are a few drivers that for historical reasons support both
> >>> options with 'no channels' meaning 'all channels'.  
> >>
> >> https://lore.kernel.org/all/20250201162631.2eab9a9a@jic23-huawei/
> >>  
> >>> In my experience, the only time we don't populate all available channels
> >>> on an ADC, even if not used, is in cases like differential chips where
> >>> any two inputs can be mixed and matched to form a channel. Some of these,
> >>> like adi,ad7173-8 would have 100s or 1000s of channels if we tried to
> >>> include all possible channels. In those cases, we make an exception and
> >>> use a dynamic number of channels based on the devicetree. But for chips
> >>> that have less than 20 total possible channels or so we've always
> >>> provided all possible channels to userspace. It makes writing userspace
> >>> software for a specific chip easier if we can always assume that chip
> >>> has the same number of channels.  
> >>
> >> In any exception to this rule of describing all channels in DT should just avoid using these helpers and do things as they're done now. No one is forced to use them. But I am not really sure why would you not describe all the channels in the device-tree for ICs with less than 20 channels? I'd assume that if the channels are unconditionally usable in the hardware, then they should be in DT as well(?)  
> > 
> > I devicetree, I think the tendency is to be less verbose and only add
> > properties/nodes when there is something that is not the usual case.
> > Default values are chosen to be the most usual case so we don't have
> > to write so much in the .dts.  
> 
> On the other hand, I've received comments from the DTS people to expose 
> all HW blocks in the bindings. AFAIR, for example, marking 
> power-supplies as 'optional' in bindings is frowned upon, because they 
> are in the HW whether the SW needs to control them or not. Hence I think 
> marking either all or no channels in dt should be the way to go - but my 
> thinking is not done based on the years of experience on ADCs!

Even for power supplies there is a difference between the binding doc
saying they are there and what we do if they aren't (which is assume
a stub regulator representing an non controllable / unknowable power supply
is sufficient).    Also for power supplies there isn't really a 'default'
to use so it doesn't really work as a comparison.

Hindsight is a wonderful thing.  I'm not sure on what policy we should have
gone for, but now we are kind of stuck with this slightly messy situation.

Helper wise if it expands usefulness we may want a bool parameter to say
if we skip the missing or not + make sure a max expected channel is provided
(might already be - I didn't check!)

Jonathan

> 
> >>>> 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>
> >>>>  
> >>
> >> Yours,
> >>      -- Matti  
> >   
> 

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

[Matti Vaittinen]

On 02/03/2025 05:20, Jonathan Cameron wrote:
> On Thu, 27 Feb 2025 09:46:06 +0200
> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> 
>> On 26/02/2025 18:10, David Lechner wrote:
>>> On 2/26/25 12:28 AM, Matti Vaittinen wrote:

...

> So today the situation is we have all the options in tree and we aren't
> really in a position to drop any of them:

Sure. I am only really interested whether we want to prefer some 
approach for (majority of) new drivers. Furthermore, I believe there 
will always be corner cases and oddities which won't fit to the 'de 
facto' model. That doesn't mean we shouldn't help those which don't have 
such 'oddities' to work with some generic code.

> Hindsight is a wonderful thing.  I'm not sure on what policy we should have
> gone for, but now we are kind of stuck with this slightly messy situation.

Sorry if my comments came out as criticism. It was not intention, I just 
try to justify the helpers by trying to think what new drivers should 
prefer.

> Helper wise if it expands usefulness we may want a bool parameter to say
> if we skip the missing or not + make sure a max expected channel is provided
> (might already be - I didn't check!)

This far it only had (optional) maximum channel ID for sanity checking 
(useful for callers which use the ID to index an array). The bool 
parameter would also require a parameter specifying the number of 
expected channels. That'd make 3 parameters which may be used or unused.

I don't think I saw existing code which would have used these 
parameters. It might be cleaner to add new APIs when we get such 
use-cases. That should simplify the use for current cases.

Thank You for the long explanation of current system + the history :) I 
appreciate your guidance!

Yours,
	-- Matti

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

[Jonathan Cameron]

On Sun, 2 Mar 2025 14:54:16 +0200
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> On 02/03/2025 05:20, Jonathan Cameron wrote:
> > On Thu, 27 Feb 2025 09:46:06 +0200
> > Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> >   
> >> On 26/02/2025 18:10, David Lechner wrote:  
> >>> On 2/26/25 12:28 AM, Matti Vaittinen wrote:  
> 
> ...
> 
> > So today the situation is we have all the options in tree and we aren't
> > really in a position to drop any of them:  
> 
> Sure. I am only really interested whether we want to prefer some 
> approach for (majority of) new drivers. Furthermore, I believe there 
> will always be corner cases and oddities which won't fit to the 'de 
> facto' model. That doesn't mean we shouldn't help those which don't have 
> such 'oddities' to work with some generic code.
Absolutely but we also can't apply this to existing drivers that don't
work quite that way.  So if we want to make more use of it we end up
providing variants long term.

> 
> > Hindsight is a wonderful thing.  I'm not sure on what policy we should have
> > gone for, but now we are kind of stuck with this slightly messy situation.  
> 
> Sorry if my comments came out as criticism. 

No problem - that was just me being wistful about wanting a crystal ball :)

> It was not intention, I just 
> try to justify the helpers by trying to think what new drivers should 
> prefer.
> 
> > Helper wise if it expands usefulness we may want a bool parameter to say
> > if we skip the missing or not + make sure a max expected channel is provided
> > (might already be - I didn't check!)  
> 
> This far it only had (optional) maximum channel ID for sanity checking 
> (useful for callers which use the ID to index an array). The bool 
> parameter would also require a parameter specifying the number of 
> expected channels. That'd make 3 parameters which may be used or unused.
> 
> I don't think I saw existing code which would have used these 
> parameters. It might be cleaner to add new APIs when we get such 
> use-cases. That should simplify the use for current cases.
That's fair enough.   Ultimately my guess is we'll end up with a complex
internal function and a bunch of wrappers that elide the majority of
the parameters.  We can get there once it's needed though!

Jonathan

> 
> Thank You for the long explanation of current system + the history :) I 
> appreciate your guidance!
> 
> Yours,
> 	-- Matti
> 

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

[Jonathan Cameron]

On Mon, 24 Feb 2025 20:33:16 +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.
I think it's worth exploring if we can tweak this slightly to make
that something a driver specifies.  Either skip the unspecified or
fill them with default values depending on a parameter.

Would make this code cover the existing cases better. 
Might be a little fiddly as we'd want to maintain ordering so
the code would need to index slightly differently. I've not tried it
so maybe not worth it for now.


> 
> 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:
> 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
> 
> iio: adc: helper: drop headers
> ---
>  drivers/iio/adc/Kconfig            |  3 +
>  drivers/iio/adc/Makefile           |  2 +
>  drivers/iio/adc/industrialio-adc.c | 89 ++++++++++++++++++++++++++++++
>  include/linux/iio/adc-helpers.h    | 22 ++++++++
>  4 files changed, 116 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..d8e9e6825d2b
> --- /dev/null
> +++ b/drivers/iio/adc/industrialio-adc.c
> @@ -0,0 +1,89 @@
> +// 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>
> +
> +int iio_adc_device_num_channels(struct device *dev)
> +{
> +	return device_get_child_node_count_named(dev, "channel");
> +}
> +EXPORT_SYMBOL_GPL(iio_adc_device_num_channels);

Maybe one to promote to a static inline in the header and avoid need for
the export given it is very simple.

> +
> +/**
> + * 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 succes. 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_child_node_scoped(dev, child) {
> +		u32 ch;
> +
> +		if (!fwnode_name_eq(child, "channel"))
> +			continue;
> +
> +		ret = fwnode_property_read_u32(child, "reg", &ch);
> +		if (ret)
> +			return ret;
> +
> +		if (max_chan_id != -1)
> +			if (ch > max_chan_id)
> +				return -ERANGE;

Might as well combine.
		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");

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

[Matti Vaittinen]

On 02/03/2025 05:35, Jonathan Cameron wrote:
> On Mon, 24 Feb 2025 20:33:16 +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.
> I think it's worth exploring if we can tweak this slightly to make
> that something a driver specifies.  Either skip the unspecified or
> fill them with default values depending on a parameter.
> 
> Would make this code cover the existing cases better.
> Might be a little fiddly as we'd want to maintain ordering so
> the code would need to index slightly differently. I've not tried it
> so maybe not worth it for now.

Thanks for the review!

I don't remember seeing users which would have benefited from this (but 
maybe I just quickly discarded them as unsuitable for this API and 
forgot them). Anyways, I think it might be cleaner (from the caller's 
perspective) to have own function for supporting such cases.


>> +
>> +int iio_adc_device_num_channels(struct device *dev)
>> +{
>> +	return device_get_child_node_count_named(dev, "channel");
>> +}
>> +EXPORT_SYMBOL_GPL(iio_adc_device_num_channels);
> 
> Maybe one to promote to a static inline in the header and avoid need for
> the export given it is very simple.

Makes sense, thanks.

...

>> +
>> +		if (max_chan_id != -1)
>> +			if (ch > max_chan_id)
>> +				return -ERANGE;
> 
> Might as well combine.
> 		if (max_chan_id != -1 && ch > max_chan_id)
> 			return -ERANGE;

Ack.

Yours,
	-- Matti

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

[Jonathan Cameron]

On Mon, 24 Feb 2025 20:33:16 +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.
> 
> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
> 
> ---
> Revision history:
> 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
> 
> iio: adc: helper: drop headers
> ---
>  drivers/iio/adc/Kconfig            |  3 +
>  drivers/iio/adc/Makefile           |  2 +
>  drivers/iio/adc/industrialio-adc.c | 89 ++++++++++++++++++++++++++++++
>  include/linux/iio/adc-helpers.h    | 22 ++++++++
>  4 files changed, 116 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..d8e9e6825d2b
> --- /dev/null
> +++ b/drivers/iio/adc/industrialio-adc.c
> @@ -0,0 +1,89 @@
> +// 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>
> +
> +int iio_adc_device_num_channels(struct device *dev)
> +{
> +	return device_get_child_node_count_named(dev, "channel");
> +}
> +EXPORT_SYMBOL_GPL(iio_adc_device_num_channels);
Just noticed, this isn't namespaces which is a bit odd.  I'd drop
the export anyway in favour of static inline but if you don't match
the namespace of the next one.

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

[Matti Vaittinen]

On 02/03/2025 05:48, Jonathan Cameron wrote:
> On Mon, 24 Feb 2025 20:33:16 +0200
> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> 

...

>> +int iio_adc_device_num_channels(struct device *dev)
>> +{
>> +	return device_get_child_node_count_named(dev, "channel");
>> +}
>> +EXPORT_SYMBOL_GPL(iio_adc_device_num_channels);
> Just noticed, this isn't namespaces which is a bit odd.  I'd drop
> the export anyway in favour of static inline but if you don't match
> the namespace of the next one.

Indeed, thanks. I'll inline this in v5.

Yours,
	-- Matti

[PATCH v4 04/10] iio: adc: rzg2l_adc: Use adc-helpers

[Matti Vaittinen]

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

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",
"diff-channels", "single-channel" and the "common-mode-channel" to
channel identification numbers used in the struct iio_chan_spec.
Furthermore, the helper checks the ID is in range of 0 ... num-channels.

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
othervice. 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 <mazziesaccount@gmail.com>

---
Revision history:
v3 => v4:
 - Adapt to 'drop diff-channel support' changes to ADC-helpers
 - select ADC helpers in the Kconfig
 - Rebased to 6.14-rc3 => channel type can no longer come from the
   template.

v2 => v3:
 - New patch

I picked the rzg2l_adc in this series because it has a straightforward
approach for populating the struct iio_chan_spec. Only other member
in the stuct besides the .channel, which can't use a 'template' -data,
is the .datasheet_name. This makes the rzg2l_adc well suited for 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!).
---
 drivers/iio/adc/Kconfig     |  1 +
 drivers/iio/adc/rzg2l_adc.c | 38 +++++++++++++++++--------------------
 2 files changed, 18 insertions(+), 21 deletions(-)

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 37b70a65da6f..e4933de0c366 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -1222,6 +1222,7 @@ config RICHTEK_RTQ6056
 config RZG2L_ADC
 	tristate "Renesas RZ/G2L ADC driver"
 	depends on ARCH_RZG2L || COMPILE_TEST
+	select IIO_ADC_HELPER
 	help
 	  Say yes here to build support for the ADC found in Renesas
 	  RZ/G2L family.
diff --git a/drivers/iio/adc/rzg2l_adc.c b/drivers/iio/adc/rzg2l_adc.c
index 883c167c0670..51c87b1bdc98 100644
--- a/drivers/iio/adc/rzg2l_adc.c
+++ b/drivers/iio/adc/rzg2l_adc.c
@@ -11,6 +11,7 @@
 #include <linux/cleanup.h>
 #include <linux/completion.h>
 #include <linux/delay.h>
+#include <linux/iio/adc-helpers.h>
 #include <linux/iio/iio.h>
 #include <linux/interrupt.h>
 #include <linux/io.h>
@@ -324,21 +325,30 @@ static irqreturn_t rzg2l_adc_isr(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
+static const struct iio_chan_spec rzg2l_adc_chan_template = {
+	.indexed = 1,
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+};
+
 static int rzg2l_adc_parse_properties(struct platform_device *pdev, struct rzg2l_adc *adc)
 {
 	const struct rzg2l_adc_hw_params *hw_params = adc->hw_params;
 	struct iio_chan_spec *chan_array;
 	struct rzg2l_adc_data *data;
-	unsigned int channel;
 	int num_channels;
-	int ret;
 	u8 i;
 
 	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
 	if (!data)
 		return -ENOMEM;
 
-	num_channels = device_get_child_node_count(&pdev->dev);
+	num_channels = devm_iio_adc_device_alloc_chaninfo_se(&pdev->dev,
+						&rzg2l_adc_chan_template,
+						hw_params->num_channels - 1,
+						&chan_array);
+	if (num_channels < 0)
+		return num_channels;
+
 	if (!num_channels)
 		return dev_err_probe(&pdev->dev, -ENODEV, "no channel children\n");
 
@@ -346,26 +356,11 @@ static int rzg2l_adc_parse_properties(struct platform_device *pdev, struct rzg2l
 		return dev_err_probe(&pdev->dev, -EINVAL,
 				     "num of channel children out of range\n");
 
-	chan_array = devm_kcalloc(&pdev->dev, num_channels, sizeof(*chan_array),
-				  GFP_KERNEL);
-	if (!chan_array)
-		return -ENOMEM;
-
-	i = 0;
-	device_for_each_child_node_scoped(&pdev->dev, fwnode) {
-		ret = fwnode_property_read_u32(fwnode, "reg", &channel);
-		if (ret)
-			return ret;
-
-		if (channel >= hw_params->num_channels)
-			return -EINVAL;
+	for (i = 0; i < num_channels; i++) {
+		int channel = chan_array[i].channel;
 
-		chan_array[i].type = rzg2l_adc_channels[channel].type;
-		chan_array[i].indexed = 1;
-		chan_array[i].channel = channel;
-		chan_array[i].info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
 		chan_array[i].datasheet_name = rzg2l_adc_channels[channel].name;
-		i++;
+		chan_array[i].type = rzg2l_adc_channels[channel].type;
 	}
 
 	data->num_channels = num_channels;
@@ -626,3 +621,4 @@ module_platform_driver(rzg2l_adc_driver);
 MODULE_AUTHOR("Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>");
 MODULE_DESCRIPTION("Renesas RZ/G2L ADC driver");
 MODULE_LICENSE("GPL v2");
+MODULE_IMPORT_NS("IIO_DRIVER");
-- 
2.48.1


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

Re: [PATCH v4 04/10] iio: adc: rzg2l_adc: Use adc-helpers

[Jonathan Cameron]

On Mon, 24 Feb 2025 20:33:29 +0200
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> 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",
> "diff-channels", "single-channel" and the "common-mode-channel" to
> channel identification numbers used in the struct iio_chan_spec.

Needs an update to reflecting naming and functionality simplifications.

> Furthermore, the helper checks the ID is in range of 0 ... num-channels.
> 
> 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
> othervice. Also, same grep shows all the .dts seem to have channel IDs
otherwise

(othervice does sound cool though ;)

> between 0..num of channels.
> 
> Use the new helper.
> 
> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
> 
> ---
> Revision history:
> v3 => v4:
>  - Adapt to 'drop diff-channel support' changes to ADC-helpers
>  - select ADC helpers in the Kconfig
>  - Rebased to 6.14-rc3 => channel type can no longer come from the
>    template.
> 
> v2 => v3:
>  - New patch
> 
> I picked the rzg2l_adc in this series because it has a straightforward
> approach for populating the struct iio_chan_spec. Only other member
> in the stuct besides the .channel, which can't use a 'template' -data,
> is the .datasheet_name. This makes the rzg2l_adc well suited for example
> user of this new helper. I hope this patch helps to evaluate whether these
> helpers are worth the hassle.
This doesn't seem to match driver. It is messing with channel type.

> 
> The change is compile tested only!! Testing before applying is highly
> appreciated (as always!).
> ---
>  drivers/iio/adc/Kconfig     |  1 +
>  drivers/iio/adc/rzg2l_adc.c | 38 +++++++++++++++++--------------------
>  2 files changed, 18 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 37b70a65da6f..e4933de0c366 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -1222,6 +1222,7 @@ config RICHTEK_RTQ6056
>  config RZG2L_ADC
>  	tristate "Renesas RZ/G2L ADC driver"
>  	depends on ARCH_RZG2L || COMPILE_TEST
> +	select IIO_ADC_HELPER
>  	help
>  	  Say yes here to build support for the ADC found in Renesas
>  	  RZ/G2L family.
> diff --git a/drivers/iio/adc/rzg2l_adc.c b/drivers/iio/adc/rzg2l_adc.c
> index 883c167c0670..51c87b1bdc98 100644
> --- a/drivers/iio/adc/rzg2l_adc.c
> +++ b/drivers/iio/adc/rzg2l_adc.c
> @@ -11,6 +11,7 @@
>  #include <linux/cleanup.h>
>  #include <linux/completion.h>
>  #include <linux/delay.h>
> +#include <linux/iio/adc-helpers.h>
>  #include <linux/iio/iio.h>
>  #include <linux/interrupt.h>
>  #include <linux/io.h>
> @@ -324,21 +325,30 @@ static irqreturn_t rzg2l_adc_isr(int irq, void *dev_id)
>  	return IRQ_HANDLED;
>  }
>  
> +static const struct iio_chan_spec rzg2l_adc_chan_template = {
> +	.indexed = 1,
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> +};
> +
>  static int rzg2l_adc_parse_properties(struct platform_device *pdev, struct rzg2l_adc *adc)
>  {
>  	const struct rzg2l_adc_hw_params *hw_params = adc->hw_params;
>  	struct iio_chan_spec *chan_array;
>  	struct rzg2l_adc_data *data;
> -	unsigned int channel;
>  	int num_channels;
> -	int ret;
>  	u8 i;
>  
>  	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
>  	if (!data)
>  		return -ENOMEM;
>  
> -	num_channels = device_get_child_node_count(&pdev->dev);
> +	num_channels = devm_iio_adc_device_alloc_chaninfo_se(&pdev->dev,
> +						&rzg2l_adc_chan_template,
> +						hw_params->num_channels - 1,
> +						&chan_array);
> +	if (num_channels < 0)
> +		return num_channels;
> +
>  	if (!num_channels)
>  		return dev_err_probe(&pdev->dev, -ENODEV, "no channel children\n");
>  
> @@ -346,26 +356,11 @@ static int rzg2l_adc_parse_properties(struct platform_device *pdev, struct rzg2l
>  		return dev_err_probe(&pdev->dev, -EINVAL,
>  				     "num of channel children out of range\n");
>  
> -	chan_array = devm_kcalloc(&pdev->dev, num_channels, sizeof(*chan_array),
> -				  GFP_KERNEL);
> -	if (!chan_array)
> -		return -ENOMEM;
> -
> -	i = 0;
> -	device_for_each_child_node_scoped(&pdev->dev, fwnode) {
> -		ret = fwnode_property_read_u32(fwnode, "reg", &channel);
> -		if (ret)
> -			return ret;
> -
> -		if (channel >= hw_params->num_channels)
> -			return -EINVAL;
> +	for (i = 0; i < num_channels; i++) {
> +		int channel = chan_array[i].channel;
>  
> -		chan_array[i].type = rzg2l_adc_channels[channel].type;
> -		chan_array[i].indexed = 1;
> -		chan_array[i].channel = channel;
> -		chan_array[i].info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
>  		chan_array[i].datasheet_name = rzg2l_adc_channels[channel].name;
> -		i++;
> +		chan_array[i].type = rzg2l_adc_channels[channel].type;
>  	}
>  
>  	data->num_channels = num_channels;
> @@ -626,3 +621,4 @@ module_platform_driver(rzg2l_adc_driver);
>  MODULE_AUTHOR("Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>");
>  MODULE_DESCRIPTION("Renesas RZ/G2L ADC driver");
>  MODULE_LICENSE("GPL v2");
> +MODULE_IMPORT_NS("IIO_DRIVER");

Re: [PATCH v4 04/10] iio: adc: rzg2l_adc: Use adc-helpers

[Matti Vaittinen]

On 02/03/2025 05:40, Jonathan Cameron wrote:
> On Mon, 24 Feb 2025 20:33:29 +0200
> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> 
>> 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",
>> "diff-channels", "single-channel" and the "common-mode-channel" to
>> channel identification numbers used in the struct iio_chan_spec.
> 
> Needs an update to reflecting naming and functionality simplifications.

Thanks! I should've noticed this. I'll re-read and alter all the commits 
for the v5.

> 
>> Furthermore, the helper checks the ID is in range of 0 ... num-channels.
>>
>> 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
>> othervice. Also, same grep shows all the .dts seem to have channel IDs
> otherwise
> 
> (othervice does sound cool though ;)

Thanks :D For some reason I always have difficulties spelling it!

...

>> I picked the rzg2l_adc in this series because it has a straightforward
>> approach for populating the struct iio_chan_spec. Only other member
>> in the stuct besides the .channel, which can't use a 'template' -data,
>> is the .datasheet_name. This makes the rzg2l_adc well suited for example
>> user of this new helper. I hope this patch helps to evaluate whether these
>> helpers are worth the hassle.
> This doesn't seem to match driver. It is messing with channel type.

Ah. Yes. It was simple until I rebased to never version. I'll change 
this. Thanks!

Yours,
	-- Matti

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

[Matti Vaittinen]

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

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",
"diff-channels", "single-channel" and the "common-mode-channel" to
channel identification numbers used in the struct iio_chan_spec.
Furthermore, the helper checks the ID is in range of 0 ... num-channels.

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
othervice. 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:
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 v4 05/10] iio: adc: sun20i-gpadc: Use adc-helpers

[Jonathan Cameron]

On Mon, 24 Feb 2025 20:33:46 +0200
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> 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",
> "diff-channels", "single-channel" and the "common-mode-channel" to
> channel identification numbers used in the struct iio_chan_spec.
> Furthermore, the helper checks the ID is in range of 0 ... num-channels.
Needs update.

> 
> 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
> othervice. 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>
> 
Otherwise looks good to me.

Jonathan

[PATCH v4 06/10] iio: adc: ti-ads7924 Drop unnecessary function parameters

[Matti Vaittinen]

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

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 <mazziesaccount@gmail.com>

---
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(-)

diff --git a/drivers/iio/adc/ti-ads7924.c b/drivers/iio/adc/ti-ads7924.c
index 66b54c0d75aa..b1f745f75dbe 100644
--- a/drivers/iio/adc/ti-ads7924.c
+++ b/drivers/iio/adc/ti-ads7924.c
@@ -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");
-- 
2.48.1


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

Re: [PATCH v4 06/10] iio: adc: ti-ads7924 Drop unnecessary function parameters

[Jonathan Cameron]

On Mon, 24 Feb 2025 20:34:01 +0200
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> 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 <mazziesaccount@gmail.com>
Looking again at this function it doesn't seem to be doing anything
useful at all.  It checks the channel nodes are in range, but
does nothing with that data. I'd just drop it entirely.

Ah. I see David suggested the same.

We can't really 'fix' what this was perhaps intended to do now
as what it does has become ABI :(


Jonathan

> 
> ---
> 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(-)
> 
> diff --git a/drivers/iio/adc/ti-ads7924.c b/drivers/iio/adc/ti-ads7924.c
> index 66b54c0d75aa..b1f745f75dbe 100644
> --- a/drivers/iio/adc/ti-ads7924.c
> +++ b/drivers/iio/adc/ti-ads7924.c
> @@ -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");

Re: [PATCH v4 06/10] iio: adc: ti-ads7924 Drop unnecessary function parameters

[Matti Vaittinen]

Hi dee Ho again Jonathan (and all),

On 02/03/2025 05:46, Jonathan Cameron wrote:
> On Mon, 24 Feb 2025 20:34:01 +0200
> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> 
>> 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 <mazziesaccount@gmail.com>
> Looking again at this function it doesn't seem to be doing anything
> useful at all.  It checks the channel nodes are in range, but
> does nothing with that data. I'd just drop it entirely.
> 
> Ah. I see David suggested the same.
> 
> We can't really 'fix' what this was perhaps intended to do now
> as what it does has become ABI :(


I took another look at this.
The logic in the ads7924 driver (without this patch) is actually:

ads7924_get_channels_config(...)
{
	device_for_each_child_node(dev, node) {
		if (fwnode_property_read_u32(node, "reg", &pval)) ..
			continue;

		if (channel >= ADS7924_CHANNELS)
			continue;

		num_channels++;
	}

if (!num_channels)
	return -EINVAL;
}

...

ads7924_probe()
{
	ret = ads7924_get_channels_config(...);
	if (ret < 0)
		return dev_err_probe(...);
}

So, it still returns an error, if no channels with valid 'reg' property 
were found from the DT. It will also fail the probe().

Thus, this change is not quite as likely to cause things to break as it 
seemed. Still, for now anything with even single valid 'channel' has 
been Ok, even if all the rest were garbage. This new variant would fail 
if any of the 'channel' nodes contained no or bad 'reg'. Thus this can 
still break things.

Anyways, I'll follow your suggestion and drop this patch (unless you 
have second thoughts) - but I will keep the function so it still 
requires at least 1 valid channel node to be found.

Yours,
	-- Matti

[PATCH v4 07/10] iio: adc: ti-ads7924: Respect device tree config

[Matti Vaittinen]

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

The ti-ads7924 driver ignores the device-tree ADC channel specification
and always exposes all 4 channels to users whether they are present in
the device-tree or not. Additionally, the "reg" values in the channel
nodes are ignored, although an error is printed if they are out of range.

Register only the channels described in the device-tree, and use the reg
property as a channel ID.

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

---
Revision history:
v3 => v4:
 - Adapt to 'drop diff-channel support' changes to ADC-helpers
 - select ADC helpers in the Kconfig
v2 => v3: New patch

Please note that this is potentially breaking existing users if they
have wrong values in the device-tree. I believe the device-tree should
ideally be respected, and if it says device X has only one channel, then
we should believe it and not register 4. Well, we don't live in the
ideal world, so even though I believe this is TheRightThingToDo - it may
cause havoc because correct device-tree has not been required from the
day 1. So, please review and test and apply at your own risk :)

As a side note, this might warrant a fixes tag but the adc-helper -stuff
is hardly worth to be backported... (And I've already exceeded my time
budget with this series - hence I'll leave crafting backportable fix to
TI people ;) )

This has only been compile tested! All testing is highly appreciated.
---
 drivers/iio/adc/Kconfig      |  1 +
 drivers/iio/adc/ti-ads7924.c | 78 ++++++++++++++++--------------------
 2 files changed, 36 insertions(+), 43 deletions(-)

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 0993008a1586..d16082ac2e1f 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -1476,6 +1476,7 @@ config TI_ADS7924
 	tristate "Texas Instruments ADS7924 ADC"
 	depends on I2C
 	select REGMAP_I2C
+	select IIO_ADC_HELPER
 	help
 	  If you say yes here you get support for Texas Instruments ADS7924
 	  4 channels, 12-bit I2C ADC chip.
diff --git a/drivers/iio/adc/ti-ads7924.c b/drivers/iio/adc/ti-ads7924.c
index b1f745f75dbe..e3ac170c73f4 100644
--- a/drivers/iio/adc/ti-ads7924.c
+++ b/drivers/iio/adc/ti-ads7924.c
@@ -22,6 +22,7 @@
 #include <linux/regmap.h>
 #include <linux/regulator/consumer.h>
 
+#include <linux/iio/adc-helpers.h>
 #include <linux/iio/iio.h>
 #include <linux/iio/types.h>
 
@@ -119,15 +120,12 @@
 #define ADS7924_TOTAL_CONVTIME_US (ADS7924_PWRUPTIME_US + ADS7924_ACQTIME_US + \
 				   ADS7924_CONVTIME_US)
 
-#define ADS7924_V_CHAN(_chan, _addr) {				\
-	.type = IIO_VOLTAGE,					\
-	.indexed = 1,						\
-	.channel = _chan,					\
-	.address = _addr,					\
-	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW), 		\
-	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),	\
-	.datasheet_name = "AIN"#_chan,				\
-}
+static const struct iio_chan_spec ads7924_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),
+};
 
 struct ads7924_data {
 	struct device *dev;
@@ -182,13 +180,6 @@ static const struct regmap_config ads7924_regmap_config = {
 	.writeable_reg = ads7924_is_writeable_reg,
 };
 
-static const struct iio_chan_spec ads7924_channels[] = {
-	ADS7924_V_CHAN(0, ADS7924_DATA0_U_REG),
-	ADS7924_V_CHAN(1, ADS7924_DATA1_U_REG),
-	ADS7924_V_CHAN(2, ADS7924_DATA2_U_REG),
-	ADS7924_V_CHAN(3, ADS7924_DATA3_U_REG),
-};
-
 static int ads7924_get_adc_result(struct ads7924_data *data,
 				  struct iio_chan_spec const *chan, int *val)
 {
@@ -251,32 +242,35 @@ static const struct iio_info ads7924_info = {
 	.read_raw = ads7924_read_raw,
 };
 
-static int ads7924_get_channels_config(struct device *dev)
+static int ads7924_get_channels_config(struct iio_dev *indio_dev,
+				       struct device *dev)
 {
-	struct fwnode_handle *node;
-	int num_channels = 0;
+	struct iio_chan_spec *chan_array;
+	int num_channels = 0, i;
+	static const char * const datasheet_names[] = {
+		"AIN0", "AIN1", "AIN2", "AIN3"
+	};
 
-	device_for_each_child_node(dev, node) {
-		u32 pval;
-		unsigned int channel;
+	num_channels = devm_iio_adc_device_alloc_chaninfo_se(dev,
+						&ads7924_chan_template,
+						ARRAY_SIZE(datasheet_names) - 1,
+						&chan_array);
 
-		if (fwnode_property_read_u32(node, "reg", &pval)) {
-			dev_err(dev, "invalid reg on %pfw\n", node);
-			continue;
-		}
+	if (num_channels < 0)
+		return num_channels;
 
-		channel = pval;
-		if (channel >= ADS7924_CHANNELS) {
-			dev_err(dev, "invalid channel index %d on %pfw\n",
-				channel, node);
-			continue;
-		}
+	if (!num_channels)
+		return -EINVAL;
 
-		num_channels++;
+	for (i = 0; i < num_channels; i++) {
+		int ch_id = chan_array[i].channel;
+
+		chan_array[i].address = ADS7924_DATA0_U_REG + ch_id;
+		chan_array[i].datasheet_name = datasheet_names[ch_id];
 	}
 
-	if (!num_channels)
-		return -EINVAL;
+	indio_dev->channels = chan_array;
+	indio_dev->num_channels = num_channels;
 
 	return 0;
 }
@@ -370,18 +364,15 @@ static int ads7924_probe(struct i2c_client *client)
 
 	mutex_init(&data->lock);
 
-	indio_dev->name = "ads7924";
-	indio_dev->modes = INDIO_DIRECT_MODE;
-
-	indio_dev->channels = ads7924_channels;
-	indio_dev->num_channels = ARRAY_SIZE(ads7924_channels);
-	indio_dev->info = &ads7924_info;
-
-	ret = ads7924_get_channels_config(dev);
+	ret = ads7924_get_channels_config(indio_dev, dev);
 	if (ret < 0)
 		return dev_err_probe(dev, ret,
 				     "failed to get channels configuration\n");
 
+	indio_dev->name = "ads7924";
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->info = &ads7924_info;
+
 	data->regmap = devm_regmap_init_i2c(client, &ads7924_regmap_config);
 	if (IS_ERR(data->regmap))
 		return dev_err_probe(dev, PTR_ERR(data->regmap),
@@ -469,3 +460,4 @@ module_i2c_driver(ads7924_driver);
 MODULE_AUTHOR("Hugo Villeneuve <hvilleneuve@dimonoff.com>");
 MODULE_DESCRIPTION("Texas Instruments ADS7924 ADC I2C driver");
 MODULE_LICENSE("GPL");
+MODULE_IMPORT_NS("IIO_DRIVER");
-- 
2.48.1


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

Re: [PATCH v4 07/10] iio: adc: ti-ads7924: Respect device tree config

[David Lechner]

On 2/24/25 12:34 PM, Matti Vaittinen wrote:
> The ti-ads7924 driver ignores the device-tree ADC channel specification
> and always exposes all 4 channels to users whether they are present in
> the device-tree or not. Additionally, the "reg" values in the channel
> nodes are ignored, although an error is printed if they are out of range.
> 
> Register only the channels described in the device-tree, and use the reg
> property as a channel ID.
> 
> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
> 
> ---
> Revision history:
> v3 => v4:
>  - Adapt to 'drop diff-channel support' changes to ADC-helpers
>  - select ADC helpers in the Kconfig
> v2 => v3: New patch
> 
> Please note that this is potentially breaking existing users if they
> have wrong values in the device-tree. I believe the device-tree should
> ideally be respected, and if it says device X has only one channel, then
> we should believe it and not register 4. Well, we don't live in the
> ideal world, so even though I believe this is TheRightThingToDo - it may
> cause havoc because correct device-tree has not been required from the
> day 1. So, please review and test and apply at your own risk :)

The DT bindings on this one are a little weird. Usually, if we don't
use any extra properties from adc.yaml, we leave out the channels. In
this case it does seem kind of like the original intention was to work
like you are suggesting, but hard to say since the driver wasn't actually
implemented that way. I would be more inclined to actually not make the
breaking change here and instead relax the bindings to make channel nodes
optional and just have the driver ignore the channel nodes by dropping
the ads7924_get_channels_config() function completely. This would make
the driver simpler instead of more complex like this patch does.

> 
> As a side note, this might warrant a fixes tag but the adc-helper -stuff
> is hardly worth to be backported... (And I've already exceeded my time
> budget with this series - hence I'll leave crafting backportable fix to
> TI people ;) )
> 
> This has only been compile tested! All testing is highly appreciated.
> ---

...

> -static int ads7924_get_channels_config(struct device *dev)
> +static int ads7924_get_channels_config(struct iio_dev *indio_dev,
> +				       struct device *dev)

Could get dev from indio_dev->dev.parent and keep only one parameter
to this function.

>  {
> -	struct fwnode_handle *node;
> -	int num_channels = 0;
> +	struct iio_chan_spec *chan_array;
> +	int num_channels = 0, i;

Don't need initialization here.

> +	static const char * const datasheet_names[] = {
> +		"AIN0", "AIN1", "AIN2", "AIN3"
> +	};
>  

Re: [PATCH v4 07/10] iio: adc: ti-ads7924: Respect device tree config

[Matti Vaittinen]

On 26/02/2025 02:09, David Lechner wrote:
> On 2/24/25 12:34 PM, Matti Vaittinen wrote:
>> The ti-ads7924 driver ignores the device-tree ADC channel specification
>> and always exposes all 4 channels to users whether they are present in
>> the device-tree or not. Additionally, the "reg" values in the channel
>> nodes are ignored, although an error is printed if they are out of range.
>>
>> Register only the channels described in the device-tree, and use the reg
>> property as a channel ID.
>>
>> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
>>
>> ---
>> Revision history:
>> v3 => v4:
>>   - Adapt to 'drop diff-channel support' changes to ADC-helpers
>>   - select ADC helpers in the Kconfig
>> v2 => v3: New patch
>>
>> Please note that this is potentially breaking existing users if they
>> have wrong values in the device-tree. I believe the device-tree should
>> ideally be respected, and if it says device X has only one channel, then
>> we should believe it and not register 4. Well, we don't live in the
>> ideal world, so even though I believe this is TheRightThingToDo - it may
>> cause havoc because correct device-tree has not been required from the
>> day 1. So, please review and test and apply at your own risk :)
> 
> The DT bindings on this one are a little weird. Usually, if we don't
> use any extra properties from adc.yaml, we leave out the channels. In
> this case it does seem kind of like the original intention was to work
> like you are suggesting, but hard to say since the driver wasn't actually
> implemented that way. I would be more inclined to actually not make the
> breaking change here and instead relax the bindings to make channel nodes
> optional and just have the driver ignore the channel nodes by dropping
> the ads7924_get_channels_config() function completely. This would make
> the driver simpler instead of more complex like this patch does.

I have no strong opinion on this. I see this driver says 'Supported' in 
MAINTAINERS. Maybe Hugo is able to provide some insight?

>> As a side note, this might warrant a fixes tag but the adc-helper -stuff
>> is hardly worth to be backported... (And I've already exceeded my time
>> budget with this series - hence I'll leave crafting backportable fix to
>> TI people ;) )
>>
>> This has only been compile tested! All testing is highly appreciated.
>> ---
> 
> ...
> 
>> -static int ads7924_get_channels_config(struct device *dev)
>> +static int ads7924_get_channels_config(struct iio_dev *indio_dev,
>> +				       struct device *dev)
> 
> Could get dev from indio_dev->dev.parent and keep only one parameter
> to this function.
> 
>>   {
>> -	struct fwnode_handle *node;
>> -	int num_channels = 0;
>> +	struct iio_chan_spec *chan_array;
>> +	int num_channels = 0, i;
> 
> Don't need initialization here.
> 
>> +	static const char * const datasheet_names[] = {
>> +		"AIN0", "AIN1", "AIN2", "AIN3"
>> +	};

Thanks for the review David! I do agree with the comments to the code.

Yours,
	-- Matti

Re: [PATCH v4 07/10] iio: adc: ti-ads7924: Respect device tree config

[Jonathan Cameron]

On Wed, 26 Feb 2025 08:39:11 +0200
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> On 26/02/2025 02:09, David Lechner wrote:
> > On 2/24/25 12:34 PM, Matti Vaittinen wrote:  
> >> The ti-ads7924 driver ignores the device-tree ADC channel specification
> >> and always exposes all 4 channels to users whether they are present in
> >> the device-tree or not. Additionally, the "reg" values in the channel
> >> nodes are ignored, although an error is printed if they are out of range.
> >>
> >> Register only the channels described in the device-tree, and use the reg
> >> property as a channel ID.
> >>
> >> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
> >>
> >> ---
> >> Revision history:
> >> v3 => v4:
> >>   - Adapt to 'drop diff-channel support' changes to ADC-helpers
> >>   - select ADC helpers in the Kconfig
> >> v2 => v3: New patch
> >>
> >> Please note that this is potentially breaking existing users if they
> >> have wrong values in the device-tree. I believe the device-tree should
> >> ideally be respected, and if it says device X has only one channel, then
> >> we should believe it and not register 4. Well, we don't live in the
> >> ideal world, so even though I believe this is TheRightThingToDo - it may
> >> cause havoc because correct device-tree has not been required from the
> >> day 1. So, please review and test and apply at your own risk :)  
> > 
> > The DT bindings on this one are a little weird. Usually, if we don't
> > use any extra properties from adc.yaml, we leave out the channels. In
> > this case it does seem kind of like the original intention was to work
> > like you are suggesting, but hard to say since the driver wasn't actually
> > implemented that way. I would be more inclined to actually not make the
> > breaking change here and instead relax the bindings to make channel nodes
> > optional and just have the driver ignore the channel nodes by dropping
> > the ads7924_get_channels_config() function completely. This would make
> > the driver simpler instead of more complex like this patch does.  
> 
> I have no strong opinion on this. I see this driver says 'Supported' in 
> MAINTAINERS. Maybe Hugo is able to provide some insight?
> 
This seems to be ABI breakage.  Never something we can take if there is
a significant chance of anyone noticing.  Here it looks like risk
is too high.

Anyhow, as discussed there has never been a clear statement on what default
is and whether the channels should be presented or not. It's always
been binding dependent, but it seems not as explicitly stated as it should
have been.  

Maybe there is some DT binding magic we can do to close this ambiguity but
I'm not sure what it is.  If not documentation may be the only way.


> >> As a side note, this might warrant a fixes tag but the adc-helper -stuff
> >> is hardly worth to be backported... (And I've already exceeded my time
> >> budget with this series - hence I'll leave crafting backportable fix to
> >> TI people ;) )
> >>
> >> This has only been compile tested! All testing is highly appreciated.
> >> ---  
> > 
> > ...
> >   
> >> -static int ads7924_get_channels_config(struct device *dev)
> >> +static int ads7924_get_channels_config(struct iio_dev *indio_dev,
> >> +				       struct device *dev)  
> > 
> > Could get dev from indio_dev->dev.parent and keep only one parameter
> > to this function.
> >   
> >>   {
> >> -	struct fwnode_handle *node;
> >> -	int num_channels = 0;
> >> +	struct iio_chan_spec *chan_array;
> >> +	int num_channels = 0, i;  
> > 
> > Don't need initialization here.
> >   
> >> +	static const char * const datasheet_names[] = {
> >> +		"AIN0", "AIN1", "AIN2", "AIN3"
> >> +	};  
> 
> Thanks for the review David! I do agree with the comments to the code.
> 
> Yours,
> 	-- Matti
> 
> 

Re: [PATCH v4 07/10] iio: adc: ti-ads7924: Respect device tree config

[Matti Vaittinen]

On 02/03/2025 05:27, Jonathan Cameron wrote:
> On Wed, 26 Feb 2025 08:39:11 +0200
> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> 
>> On 26/02/2025 02:09, David Lechner wrote:
>>> On 2/24/25 12:34 PM, Matti Vaittinen wrote:
>>>> The ti-ads7924 driver ignores the device-tree ADC channel specification
>>>> and always exposes all 4 channels to users whether they are present in
>>>> the device-tree or not. Additionally, the "reg" values in the channel
>>>> nodes are ignored, although an error is printed if they are out of range.
>>>>
>>>> Register only the channels described in the device-tree, and use the reg
>>>> property as a channel ID.
>>>>
>>>> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
>>>>
>>>> ---
>>>> Revision history:
>>>> v3 => v4:
>>>>    - Adapt to 'drop diff-channel support' changes to ADC-helpers
>>>>    - select ADC helpers in the Kconfig
>>>> v2 => v3: New patch
>>>>
>>>> Please note that this is potentially breaking existing users if they
>>>> have wrong values in the device-tree. I believe the device-tree should
>>>> ideally be respected, and if it says device X has only one channel, then
>>>> we should believe it and not register 4. Well, we don't live in the
>>>> ideal world, so even though I believe this is TheRightThingToDo - it may
>>>> cause havoc because correct device-tree has not been required from the
>>>> day 1. So, please review and test and apply at your own risk :)
>>>
>>> The DT bindings on this one are a little weird. Usually, if we don't
>>> use any extra properties from adc.yaml, we leave out the channels. In
>>> this case it does seem kind of like the original intention was to work
>>> like you are suggesting, but hard to say since the driver wasn't actually
>>> implemented that way. I would be more inclined to actually not make the
>>> breaking change here and instead relax the bindings to make channel nodes
>>> optional and just have the driver ignore the channel nodes by dropping
>>> the ads7924_get_channels_config() function completely. This would make
>>> the driver simpler instead of more complex like this patch does.
>>
>> I have no strong opinion on this. I see this driver says 'Supported' in
>> MAINTAINERS. Maybe Hugo is able to provide some insight?
>>
> This seems to be ABI breakage.  Never something we can take if there is
> a significant chance of anyone noticing.  Here it looks like risk
> is too high.

Ok. I'll just drop this patch then. Thanks David & Jonathan :)

Yours,
	-- Matti

Re: [PATCH v4 07/10] iio: adc: ti-ads7924: Respect device tree config

[Hugo Villeneuve]

On Sun, 2 Mar 2025 15:10:12 +0200
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> On 02/03/2025 05:27, Jonathan Cameron wrote:
> > On Wed, 26 Feb 2025 08:39:11 +0200
> > Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> > 
> >> On 26/02/2025 02:09, David Lechner wrote:
> >>> On 2/24/25 12:34 PM, Matti Vaittinen wrote:
> >>>> The ti-ads7924 driver ignores the device-tree ADC channel specification
> >>>> and always exposes all 4 channels to users whether they are present in
> >>>> the device-tree or not. Additionally, the "reg" values in the channel
> >>>> nodes are ignored, although an error is printed if they are out of range.
> >>>>
> >>>> Register only the channels described in the device-tree, and use the reg
> >>>> property as a channel ID.
> >>>>
> >>>> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
> >>>>
> >>>> ---
> >>>> Revision history:
> >>>> v3 => v4:
> >>>>    - Adapt to 'drop diff-channel support' changes to ADC-helpers
> >>>>    - select ADC helpers in the Kconfig
> >>>> v2 => v3: New patch
> >>>>
> >>>> Please note that this is potentially breaking existing users if they
> >>>> have wrong values in the device-tree. I believe the device-tree should
> >>>> ideally be respected, and if it says device X has only one channel, then
> >>>> we should believe it and not register 4. Well, we don't live in the
> >>>> ideal world, so even though I believe this is TheRightThingToDo - it may
> >>>> cause havoc because correct device-tree has not been required from the
> >>>> day 1. So, please review and test and apply at your own risk :)
> >>>
> >>> The DT bindings on this one are a little weird. Usually, if we don't
> >>> use any extra properties from adc.yaml, we leave out the channels. In
> >>> this case it does seem kind of like the original intention was to work
> >>> like you are suggesting, but hard to say since the driver wasn't actually
> >>> implemented that way. I would be more inclined to actually not make the
> >>> breaking change here and instead relax the bindings to make channel nodes
> >>> optional and just have the driver ignore the channel nodes by dropping
> >>> the ads7924_get_channels_config() function completely. This would make
> >>> the driver simpler instead of more complex like this patch does.
> >>
> >> I have no strong opinion on this. I see this driver says 'Supported' in
> >> MAINTAINERS. Maybe Hugo is able to provide some insight?
> >>
> > This seems to be ABI breakage.  Never something we can take if there is
> > a significant chance of anyone noticing.  Here it looks like risk
> > is too high.
> 
> Ok. I'll just drop this patch then. Thanks David & Jonathan :)

Hi Matti,
I haven't really been able to follow all discussions, but as an
historic note I developped this driver for a prototype that never
materialized. So any changes would not have an impact, as far as I am
concerned.

Hugo.

[PATCH v4 08/10] iio: adc: Support ROHM BD79124 ADC

[Matti Vaittinen]

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

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 <mazziesaccount@gmail.com>

---
Revision history:
v3 => v4:
 - Adapt to 'drop diff-channel support' changes to ADC-helpers
 - Don't parse fwnode in GPIO valid-mask callback but use pin config
   cached at probe()
 - Drop use of iio_adc_device_channels_by_property()
 - Open code the bd79124_reg_init loop (as suggested by Jonathan)
 - Use devm variant of mutex_init()
 - Styling
v2 => v3:
 - Fix uninitialized return value reported by the kernel test robot
 - Fix indent
 - Adapt to adc-helper changes supporting also single-ended and
   differential channels
RFC v1 => v2:
 - Add event throttling (constant delay of 1 sec)
 - rename variable 'd' to 'data'
 - Use ADC helpers to detect pins used for ADC
 - bd79124 drop MFD and pinmux && handle GPO in this driver
 - Drop adc suffix from the IIO file name
---
 drivers/iio/adc/Kconfig        |   12 +
 drivers/iio/adc/Makefile       |    1 +
 drivers/iio/adc/rohm-bd79124.c | 1114 ++++++++++++++++++++++++++++++++
 3 files changed, 1127 insertions(+)
 create mode 100644 drivers/iio/adc/rohm-bd79124.c

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index d16082ac2e1f..357214bc5024 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -1191,6 +1191,18 @@ config RN5T618_ADC
 	  This driver can also be built as a module. If so, the module
 	  will be called rn5t618-adc.
 
+config ROHM_BD79124
+	tristate "Rohm BD79124 ADC driver"
+	depends on I2C
+	select REGMAP_I2C
+	select IIO_ADC_HELPER
+	help
+	  Say yes here to build support for the ROHM BD79124 ADC. The
+	  ROHM BD79124 is a 12-bit, 8-channel, SAR ADC. The ADC supports
+	  also an automatic measurement mode, with an alarm interrupt for
+	  out-of-window measurements. The window is configurable for each
+	  channel.
+
 config ROCKCHIP_SARADC
 	tristate "Rockchip SARADC driver"
 	depends on ARCH_ROCKCHIP || COMPILE_TEST
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index 1c410f483029..3e10af9ec4c4 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -109,6 +109,7 @@ obj-$(CONFIG_QCOM_VADC_COMMON) += qcom-vadc-common.o
 obj-$(CONFIG_RCAR_GYRO_ADC) += rcar-gyroadc.o
 obj-$(CONFIG_RICHTEK_RTQ6056) += rtq6056.o
 obj-$(CONFIG_RN5T618_ADC) += rn5t618-adc.o
+obj-$(CONFIG_ROHM_BD79124) += rohm-bd79124.o
 obj-$(CONFIG_ROCKCHIP_SARADC) += rockchip_saradc.o
 obj-$(CONFIG_RZG2L_ADC) += rzg2l_adc.o
 obj-$(CONFIG_SC27XX_ADC) += sc27xx_adc.o
diff --git a/drivers/iio/adc/rohm-bd79124.c b/drivers/iio/adc/rohm-bd79124.c
new file mode 100644
index 000000000000..b3ef0e0c9a3a
--- /dev/null
+++ b/drivers/iio/adc/rohm-bd79124.c
@@ -0,0 +1,1114 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * ROHM ADC driver for BD79124 ADC/GPO device
+ * https://fscdn.rohm.com/en/products/databook/datasheet/ic/data_converter/dac/bd79124muf-c-e.pdf
+ *
+ * Copyright (c) 2025, ROHM Semiconductor.
+ */
+
+#include <linux/bitfield.h>
+#include <linux/bitmap.h>
+#include <linux/bits.h>
+#include <linux/byteorder/generic.h>
+#include <linux/device.h>
+#include <linux/delay.h>
+#include <linux/devm-helpers.h>
+#include <linux/err.h>
+#include <linux/gpio/driver.h>
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <linux/irqreturn.h>
+#include <linux/module.h>
+#include <linux/mod_devicetable.h>
+#include <linux/regmap.h>
+#include <linux/types.h>
+
+#include <linux/iio/events.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/adc-helpers.h>
+
+#define BD79124_I2C_MULTI_READ		0x30
+#define BD79124_I2C_MULTI_WRITE		0x28
+#define BD79124_REG_MAX			0xaf
+
+#define BD79124_REG_SYSTEM_STATUS	0x0
+#define BD79124_REG_GEN_CFG		0x01
+#define BD79124_REG_OPMODE_CFG		0x04
+#define BD79124_REG_PINCFG		0x05
+#define BD79124_REG_GPO_VAL		0x0B
+#define BD79124_REG_SEQUENCE_CFG	0x10
+#define BD79124_REG_MANUAL_CHANNELS	0x11
+#define BD79124_REG_AUTO_CHANNELS	0x12
+#define BD79124_REG_ALERT_CH_SEL	0x14
+#define BD79124_REG_EVENT_FLAG		0x18
+#define BD79124_REG_EVENT_FLAG_HI	0x1a
+#define BD79124_REG_EVENT_FLAG_LO	0x1c
+#define BD79124_REG_HYSTERESIS_CH0	0x20
+#define BD79124_REG_EVENTCOUNT_CH0	0x22
+#define BD79124_REG_RECENT_CH0_LSB	0xa0
+#define BD79124_REG_RECENT_CH7_MSB	0xaf
+
+#define BD79124_ADC_BITS 12
+#define BD79124_MASK_CONV_MODE GENMASK(6, 5)
+#define BD79124_MASK_AUTO_INTERVAL GENMASK(1, 0)
+#define BD79124_CONV_MODE_MANSEQ 0
+#define BD79124_CONV_MODE_AUTO 1
+#define BD79124_INTERVAL_075 0
+#define BD79124_INTERVAL_150 1
+#define BD79124_INTERVAL_300 2
+#define BD79124_INTERVAL_600 3
+
+#define BD79124_MASK_DWC_EN BIT(4)
+#define BD79124_MASK_STATS_EN BIT(5)
+#define BD79124_MASK_SEQ_START BIT(4)
+#define BD79124_MASK_SEQ_MODE GENMASK(1, 0)
+#define BD79124_MASK_SEQ_MANUAL 0
+#define BD79124_MASK_SEQ_SEQ 1
+
+#define BD79124_MASK_HYSTERESIS GENMASK(3, 0)
+#define BD79124_LOW_LIMIT_MIN 0
+#define BD79124_HIGH_LIMIT_MAX GENMASK(11, 0)
+
+/*
+ * The high limit, low limit and last measurement result are each stored in
+ * 2 consequtive registers. 4 bits are in the high bits of the 1.st register
+ * and 8 bits in the next register.
+ *
+ * These macros return the address of the 1.st reg for the given channel
+ */
+#define BD79124_GET_HIGH_LIMIT_REG(ch) (BD79124_REG_HYSTERESIS_CH0 + (ch) * 4)
+#define BD79124_GET_LOW_LIMIT_REG(ch) (BD79124_REG_EVENTCOUNT_CH0 + (ch) * 4)
+#define BD79124_GET_LIMIT_REG(ch, dir) ((dir) == IIO_EV_DIR_RISING ?		\
+		BD79124_GET_HIGH_LIMIT_REG(ch) : BD79124_GET_LOW_LIMIT_REG(ch))
+#define BD79124_GET_RECENT_RES_REG(ch) (BD79124_REG_RECENT_CH0_LSB + (ch) * 2)
+
+/*
+ * The hysteresis for a channel is stored in the same register where the
+ * 4 bits of high limit reside.
+ */
+#define BD79124_GET_HYSTERESIS_REG(ch) BD79124_GET_HIGH_LIMIT_REG(ch)
+
+#define BD79124_MAX_NUM_CHANNELS 8
+
+struct bd79124_data {
+	s64 timestamp;
+	struct regmap *map;
+	struct device *dev;
+	int vmax;
+	/*
+	 * Keep measurement status so read_raw() knows if the measurement needs
+	 * to be started.
+	 */
+	int alarm_monitored[BD79124_MAX_NUM_CHANNELS];
+	/*
+	 * The BD79124 does not allow disabling/enabling limit separately for
+	 * one direction only. Hence, we do the disabling by changing the limit
+	 * to maximum/minimum measurable value. This means we need to cache
+	 * the limit in order to maintain it over the time limit is disabled.
+	 */
+	u16 alarm_r_limit[BD79124_MAX_NUM_CHANNELS];
+	u16 alarm_f_limit[BD79124_MAX_NUM_CHANNELS];
+	/* Bitmask of disabled events (for rate limiting) for each channel. */
+	int alarm_suppressed[BD79124_MAX_NUM_CHANNELS];
+	/*
+	 * The BD79124 is configured to run the measurements in the background.
+	 * This is done for the event monitoring as well as for the read_raw().
+	 * Protect the measurement starting/stopping using a mutex.
+	 */
+	struct mutex mutex;
+	struct delayed_work alm_enable_work;
+	struct gpio_chip gc;
+	u8 gpio_valid_mask;
+};
+
+/* Read-only regs */
+static const struct regmap_range bd79124_ro_ranges[] = {
+	{
+		.range_min = BD79124_REG_EVENT_FLAG,
+		.range_max = BD79124_REG_EVENT_FLAG,
+	}, {
+		.range_min = BD79124_REG_RECENT_CH0_LSB,
+		.range_max = BD79124_REG_RECENT_CH7_MSB,
+	},
+};
+
+static const struct regmap_access_table bd79124_ro_regs = {
+	.no_ranges	= &bd79124_ro_ranges[0],
+	.n_no_ranges	= ARRAY_SIZE(bd79124_ro_ranges),
+};
+
+static const struct regmap_range bd79124_volatile_ranges[] = {
+	{
+		.range_min = BD79124_REG_RECENT_CH0_LSB,
+		.range_max = BD79124_REG_RECENT_CH7_MSB,
+	}, {
+		.range_min = BD79124_REG_EVENT_FLAG,
+		.range_max = BD79124_REG_EVENT_FLAG,
+	}, {
+		.range_min = BD79124_REG_EVENT_FLAG_HI,
+		.range_max = BD79124_REG_EVENT_FLAG_HI,
+	}, {
+		.range_min = BD79124_REG_EVENT_FLAG_LO,
+		.range_max = BD79124_REG_EVENT_FLAG_LO,
+	}, {
+		.range_min = BD79124_REG_SYSTEM_STATUS,
+		.range_max = BD79124_REG_SYSTEM_STATUS,
+	},
+};
+
+static const struct regmap_access_table bd79124_volatile_regs = {
+	.yes_ranges	= &bd79124_volatile_ranges[0],
+	.n_yes_ranges	= ARRAY_SIZE(bd79124_volatile_ranges),
+};
+
+static const struct regmap_range bd79124_precious_ranges[] = {
+	{
+		.range_min = BD79124_REG_EVENT_FLAG_HI,
+		.range_max = BD79124_REG_EVENT_FLAG_HI,
+	}, {
+		.range_min = BD79124_REG_EVENT_FLAG_LO,
+		.range_max = BD79124_REG_EVENT_FLAG_LO,
+	},
+};
+
+static const struct regmap_access_table bd79124_precious_regs = {
+	.yes_ranges	= &bd79124_precious_ranges[0],
+	.n_yes_ranges	= ARRAY_SIZE(bd79124_precious_ranges),
+};
+
+static const struct regmap_config bd79124_regmap = {
+	.reg_bits		= 16,
+	.val_bits		= 8,
+	.read_flag_mask		= BD79124_I2C_MULTI_READ,
+	.write_flag_mask	= BD79124_I2C_MULTI_WRITE,
+	.max_register		= BD79124_REG_MAX,
+	.cache_type		= REGCACHE_MAPLE,
+	.volatile_table		= &bd79124_volatile_regs,
+	.wr_table		= &bd79124_ro_regs,
+	.precious_table		= &bd79124_precious_regs,
+};
+
+static int bd79124gpo_direction_get(struct gpio_chip *gc, unsigned int offset)
+{
+	return GPIO_LINE_DIRECTION_OUT;
+}
+
+static void bd79124gpo_set(struct gpio_chip *gc, unsigned int offset, int value)
+{
+	struct bd79124_data *data = gpiochip_get_data(gc);
+
+	if (value)
+		regmap_set_bits(data->map, BD79124_REG_GPO_VAL, BIT(offset));
+	else
+		regmap_clear_bits(data->map, BD79124_REG_GPO_VAL, BIT(offset));
+}
+
+static void bd79124gpo_set_multiple(struct gpio_chip *gc, unsigned long *mask,
+				   unsigned long *bits)
+{
+	int ret, val;
+	struct bd79124_data *data = gpiochip_get_data(gc);
+
+	/* Ensure all GPIOs in 'mask' are set to be GPIOs */
+	ret = regmap_read(data->map, BD79124_REG_PINCFG, &val);
+	if (ret)
+		return;
+
+	if ((val & *mask) != *mask) {
+		dev_dbg(data->dev, "Invalid mux config. Can't set value.\n");
+		/* Do not set value for pins configured as ADC inputs */
+		*mask &= val;
+	}
+
+	regmap_update_bits(data->map, BD79124_REG_GPO_VAL, *mask, *bits);
+}
+
+static int bd79124_init_valid_mask(struct gpio_chip *gc,
+				   unsigned long *valid_mask,
+				   unsigned int ngpios)
+{
+	struct bd79124_data *data = gpiochip_get_data(gc);
+
+	*valid_mask = data->gpio_valid_mask;
+
+	return 0;
+}
+
+/* Template for GPIO chip */
+static const struct gpio_chip bd79124gpo_chip = {
+	.label			= "bd79124-gpo",
+	.get_direction		= bd79124gpo_direction_get,
+	.set			= bd79124gpo_set,
+	.set_multiple		= bd79124gpo_set_multiple,
+	.init_valid_mask	= bd79124_init_valid_mask,
+	.can_sleep		= true,
+	.ngpio			= 8,
+	.base			= -1,
+};
+
+struct bd79124_raw {
+	u8 bit0_3; /* Is set in high bits of the byte */
+	u8 bit4_11;
+};
+#define BD79124_RAW_TO_INT(r) ((r.bit4_11 << 4) | (r.bit0_3 >> 4))
+
+/*
+ * The high and low limits as well as the recent result values are stored in
+ * the same way in 2 consequent registers. The first register contains 4 bits
+ * of the value. These bits are stored in the high bits [7:4] of register, but
+ * they represent the low bits [3:0] of the value.
+ * The value bits [11:4] are stored in the next register.
+ *
+ * Read data from register and convert to integer.
+ */
+static int bd79124_read_reg_to_int(struct bd79124_data *data, int reg,
+				   unsigned int *val)
+{
+	int ret;
+	struct bd79124_raw raw;
+
+	ret = regmap_bulk_read(data->map, reg, &raw, sizeof(raw));
+	if (ret) {
+		dev_dbg(data->dev, "bulk_read failed %d\n", ret);
+
+		return ret;
+	}
+
+	*val = BD79124_RAW_TO_INT(raw);
+
+	return 0;
+}
+
+/*
+ * The high and low limits as well as the recent result values are stored in
+ * the same way in 2 consequent registers. The first register contains 4 bits
+ * of the value. These bits are stored in the high bits [7:4] of register, but
+ * they represent the low bits [3:0] of the value.
+ * The value bits [11:4] are stored in the next regoster.
+ *
+ * Conver the integer to register format and write it using rmw cycle.
+ */
+static int bd79124_write_int_to_reg(struct bd79124_data *data, int reg,
+				    unsigned int val)
+{
+	struct bd79124_raw raw;
+	int ret, tmp;
+
+	raw.bit4_11 = (u8)(val >> 4);
+	raw.bit0_3 = (u8)(val << 4);
+
+	ret = regmap_read(data->map, reg, &tmp);
+	if (ret)
+		return ret;
+
+	raw.bit0_3 |= (0xf & tmp);
+
+	return regmap_bulk_write(data->map, reg, &raw, sizeof(raw));
+}
+
+static const struct iio_event_spec bd79124_events[] = {
+	{
+		.type = IIO_EV_TYPE_THRESH,
+		.dir = IIO_EV_DIR_RISING,
+		.mask_separate = BIT(IIO_EV_INFO_VALUE) |
+				 BIT(IIO_EV_INFO_ENABLE),
+	},
+	{
+		.type = IIO_EV_TYPE_THRESH,
+		.dir = IIO_EV_DIR_FALLING,
+		.mask_separate = BIT(IIO_EV_INFO_VALUE) |
+				 BIT(IIO_EV_INFO_ENABLE),
+	},
+	{
+		.type = IIO_EV_TYPE_THRESH,
+		.dir = IIO_EV_DIR_EITHER,
+		.mask_separate = BIT(IIO_EV_INFO_HYSTERESIS),
+	},
+};
+
+static const struct iio_chan_spec bd79124_chan_template_noirq = {
+	.type = IIO_VOLTAGE,
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
+	.indexed = 1,
+};
+
+static const struct iio_chan_spec bd79124_chan_template = {
+	.type = IIO_VOLTAGE,
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
+	.indexed = 1,
+	.event_spec = bd79124_events,
+	.num_event_specs = ARRAY_SIZE(bd79124_events),
+};
+
+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:
+		reg = BD79124_GET_HYSTERESIS_REG(chan->channel);
+		ret = regmap_read(data->map, reg, val);
+		if (ret)
+			return ret;
+		/* Mask the non hysteresis bits */
+		*val &= BD79124_MASK_HYSTERESIS;
+		/*
+		 * The data-sheet says the hysteresis register value needs to be
+		 * sifted left by 3 (or multiplied by 8, depending on the
+		 * page :] )
+		 */
+		*val <<= 3;
+
+		return IIO_VAL_INT;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int bd79124_start_measurement(struct bd79124_data *data, int chan)
+{
+	int val, ret, regval;
+
+	/* See if already started */
+	ret = regmap_read(data->map, BD79124_REG_AUTO_CHANNELS, &val);
+	if (val & BIT(chan))
+		return 0;
+
+	ret = regmap_clear_bits(data->map, BD79124_REG_SEQUENCE_CFG,
+				BD79124_MASK_SEQ_START);
+	if (ret)
+		return ret;
+
+	/* Add the channel to measured channels */
+	ret = regmap_write(data->map, BD79124_REG_AUTO_CHANNELS, val | BIT(chan));
+	if (ret)
+		return ret;
+
+	ret = regmap_set_bits(data->map, BD79124_REG_SEQUENCE_CFG,
+			      BD79124_MASK_SEQ_START);
+	if (ret)
+		return ret;
+
+	/*
+	 * Start the measurement at the background. Don't bother checking if
+	 * it was started, regmap has cache.
+	 */
+	regval = FIELD_PREP(BD79124_MASK_CONV_MODE, BD79124_CONV_MODE_AUTO);
+
+	return regmap_update_bits(data->map, BD79124_REG_OPMODE_CFG,
+				BD79124_MASK_CONV_MODE, regval);
+}
+
+static int bd79124_stop_measurement(struct bd79124_data *data, int chan)
+{
+	int val, ret;
+
+	/* See if already stopped */
+	ret = regmap_read(data->map, BD79124_REG_AUTO_CHANNELS, &val);
+	if (!(val & BIT(chan)))
+		return 0;
+
+	ret = regmap_clear_bits(data->map, BD79124_REG_SEQUENCE_CFG,
+				BD79124_MASK_SEQ_START);
+
+	/* Clear the channel from the measured channels */
+	ret = regmap_write(data->map, BD79124_REG_AUTO_CHANNELS,
+			   (~BIT(chan)) & val);
+	if (ret)
+		return ret;
+
+	/*
+	 * Stop background conversion for power saving if it was the last
+	 * channel
+	 */
+	if (!((~BIT(chan)) & val)) {
+		int regval = FIELD_PREP(BD79124_MASK_CONV_MODE,
+					BD79124_CONV_MODE_MANSEQ);
+
+		ret = regmap_update_bits(data->map, BD79124_REG_OPMODE_CFG,
+					 BD79124_MASK_CONV_MODE, regval);
+		if (ret)
+			return ret;
+	}
+
+	return regmap_set_bits(data->map, BD79124_REG_SEQUENCE_CFG,
+			       BD79124_MASK_SEQ_START);
+}
+
+static int bd79124_read_event_config(struct iio_dev *iio_dev,
+				     const struct iio_chan_spec *chan,
+				     enum iio_event_type type,
+				     enum iio_event_direction dir)
+{
+	struct bd79124_data *data = iio_priv(iio_dev);
+
+	if (chan->channel >= BD79124_MAX_NUM_CHANNELS)
+		return -EINVAL;
+
+	return (data->alarm_monitored[chan->channel] & BIT(dir));
+}
+
+static int bd79124_disable_event(struct bd79124_data *data,
+			enum iio_event_direction dir, int channel)
+{
+	int dir_bit = BIT(dir), reg;
+	unsigned int limit;
+
+	guard(mutex)(&data->mutex);
+	/*
+	 * Set thresholds either to 0 or to 2^12 - 1 as appropriate to prevent
+	 * alerts and thus disable event generation.
+	 */
+	if (dir == IIO_EV_DIR_RISING) {
+		reg = BD79124_GET_HIGH_LIMIT_REG(channel);
+		limit = BD79124_HIGH_LIMIT_MAX;
+	} else if (dir == IIO_EV_DIR_FALLING) {
+		reg = BD79124_GET_LOW_LIMIT_REG(channel);
+		limit = BD79124_LOW_LIMIT_MIN;
+	} else {
+		return -EINVAL;
+	}
+
+	data->alarm_monitored[channel] &= (~dir_bit);
+	/*
+	 * Stop measurement if there is no more events to monitor.
+	 * We don't bother checking the retval because the limit
+	 * setting should in any case effectively disable the alarm.
+	 */
+	if (!data->alarm_monitored[channel]) {
+		bd79124_stop_measurement(data, channel);
+		regmap_clear_bits(data->map, BD79124_REG_ALERT_CH_SEL,
+			       BIT(channel));
+	}
+
+	return bd79124_write_int_to_reg(data, reg, limit);
+}
+
+static int bd79124_enable_event(struct bd79124_data *data,
+		enum iio_event_direction dir, unsigned int channel)
+{
+	int dir_bit = BIT(dir);
+	int reg;
+	u16 *limit;
+	int ret;
+
+	guard(mutex)(&data->mutex);
+	/* Set channel to be measured */
+	ret = bd79124_start_measurement(data, channel);
+	if (ret)
+		return ret;
+
+	data->alarm_monitored[channel] |= dir_bit;
+
+	/* Add the channel to the list of monitored channels */
+	ret = regmap_set_bits(data->map, BD79124_REG_ALERT_CH_SEL,
+			      BIT(channel));
+	if (ret)
+		return ret;
+
+	if (dir == IIO_EV_DIR_RISING) {
+		limit = &data->alarm_f_limit[channel];
+		reg = BD79124_GET_HIGH_LIMIT_REG(channel);
+	} else {
+		limit = &data->alarm_f_limit[channel];
+		reg = BD79124_GET_LOW_LIMIT_REG(channel);
+	}
+	/* Don't write the new limit to the hardware if we are in the
+	 * rate-limit period. The timer which re-enables the event will set
+	 * the limit.
+	 */
+	if (!(data->alarm_suppressed[channel] & dir_bit)) {
+		ret = bd79124_write_int_to_reg(data, reg, *limit);
+		if (ret)
+			return ret;
+	}
+
+	/*
+	 * Enable comparator. Trust the regmap cache, no need to check
+	 * if it was already enabled.
+	 *
+	 * We could do this in the hw-init, but there may be users who
+	 * never enable alarms and for them it makes sense to not
+	 * enable the comparator at probe.
+	 */
+	return regmap_set_bits(data->map, BD79124_REG_GEN_CFG,
+				      BD79124_MASK_DWC_EN);
+
+}
+
+static int bd79124_write_event_config(struct iio_dev *iio_dev,
+				      const struct iio_chan_spec *chan,
+				      enum iio_event_type type,
+				      enum iio_event_direction dir, bool state)
+{
+	struct bd79124_data *data = iio_priv(iio_dev);
+
+	if (chan->channel >= BD79124_MAX_NUM_CHANNELS)
+		return -EINVAL;
+
+	if (state)
+		return bd79124_enable_event(data, dir, chan->channel);
+
+	return bd79124_disable_event(data, dir, chan->channel);
+}
+
+static int bd79124_write_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 reg;
+
+	if (chan->channel >= BD79124_MAX_NUM_CHANNELS)
+		return -EINVAL;
+
+	switch (info) {
+	case IIO_EV_INFO_VALUE:
+		if (dir == IIO_EV_DIR_RISING) {
+			guard(mutex)(&data->mutex);
+
+			data->alarm_r_limit[chan->channel] = val;
+			reg = BD79124_GET_HIGH_LIMIT_REG(chan->channel);
+		} else if (dir == IIO_EV_DIR_FALLING) {
+			guard(mutex)(&data->mutex);
+
+			data->alarm_f_limit[chan->channel] = val;
+			reg = BD79124_GET_LOW_LIMIT_REG(chan->channel);
+		} else {
+			return -EINVAL;
+		}
+		/*
+		 * We don't want to enable the alarm if it is not enabled or
+		 * if it is suppressed. In that case skip writing to the
+		 * register.
+		 */
+		if (!(data->alarm_monitored[chan->channel] & BIT(dir)) ||
+		    data->alarm_suppressed[chan->channel] & BIT(dir))
+			return 0;
+
+		return bd79124_write_int_to_reg(data, reg, val);
+
+	case IIO_EV_INFO_HYSTERESIS:
+		reg = BD79124_GET_HYSTERESIS_REG(chan->channel);
+		val >>= 3;
+
+		return regmap_update_bits(data->map, reg, BD79124_MASK_HYSTERESIS,
+					  val);
+	default:
+		return -EINVAL;
+	}
+}
+
+static int bd79124_single_chan_seq(struct bd79124_data *data, int chan, int *old)
+{
+	int ret;
+
+	ret = regmap_clear_bits(data->map, BD79124_REG_SEQUENCE_CFG,
+				BD79124_MASK_SEQ_START);
+	if (ret)
+		return ret;
+
+	/*
+	 * It may be we have some channels monitored for alarms so we want to
+	 * cache the old config and return it when the single channel
+	 * measurement has been completed.
+	 */
+	ret = regmap_read(data->map, BD79124_REG_AUTO_CHANNELS, old);
+	if (ret)
+		return ret;
+
+	ret = regmap_write(data->map, BD79124_REG_AUTO_CHANNELS, BIT(chan));
+	if (ret)
+		return ret;
+
+	/* Restart the sequencer */
+	return regmap_set_bits(data->map, BD79124_REG_SEQUENCE_CFG,
+			      BD79124_MASK_SEQ_START);
+}
+
+static int bd79124_single_chan_seq_end(struct bd79124_data *data, int old)
+{
+	int ret;
+
+	ret = regmap_clear_bits(data->map, BD79124_REG_SEQUENCE_CFG,
+				BD79124_MASK_SEQ_START);
+	if (ret)
+		return ret;
+
+	ret = regmap_write(data->map, BD79124_REG_AUTO_CHANNELS, old);
+	if (ret)
+		return ret;
+
+	return regmap_set_bits(data->map, BD79124_REG_SEQUENCE_CFG,
+			      BD79124_MASK_SEQ_START);
+}
+
+static int bd79124_read_raw(struct iio_dev *iio_dev,
+			    struct iio_chan_spec const *chan,
+			    int *val, int *val2, long m)
+{
+	struct bd79124_data *data = iio_priv(iio_dev);
+	int ret;
+
+	if (chan->channel >= BD79124_MAX_NUM_CHANNELS)
+		return -EINVAL;
+
+	switch (m) {
+	case IIO_CHAN_INFO_RAW:
+	{
+		int old_chan_cfg, tmp;
+		int regval;
+
+		guard(mutex)(&data->mutex);
+
+		/*
+		 * Start the automatic conversion. This is needed here if no
+		 * events have been enabled.
+		 */
+		regval = FIELD_PREP(BD79124_MASK_CONV_MODE,
+				    BD79124_CONV_MODE_AUTO);
+		ret = regmap_update_bits(data->map, BD79124_REG_OPMODE_CFG,
+					 BD79124_MASK_CONV_MODE, regval);
+		if (ret)
+			return ret;
+
+		ret = bd79124_single_chan_seq(data, chan->channel, &old_chan_cfg);
+		if (ret)
+			return ret;
+
+		/* The maximum conversion time is 6 uS. */
+		udelay(6);
+
+		ret = bd79124_read_reg_to_int(data,
+				BD79124_GET_RECENT_RES_REG(chan->channel),
+				val);
+		/*
+		 * Return the old chan config even if data reading failed in
+		 * order to re-enable the event monitoring.
+		 */
+		tmp = bd79124_single_chan_seq_end(data, old_chan_cfg);
+		if (tmp)
+			dev_err(data->dev,
+				"Failed to return config. Alarms may be disabled\n");
+
+		if (ret)
+			return ret;
+
+		return IIO_VAL_INT;
+	}
+	case IIO_CHAN_INFO_SCALE:
+		*val = data->vmax / 1000;
+		*val2 = BD79124_ADC_BITS;
+		return IIO_VAL_FRACTIONAL_LOG2;
+	default:
+		return -EINVAL;
+	}
+}
+
+static const struct iio_info bd79124_info = {
+	.read_raw = bd79124_read_raw,
+	.read_event_config = &bd79124_read_event_config,
+	.write_event_config = &bd79124_write_event_config,
+	.read_event_value = &bd79124_read_event_value,
+	.write_event_value = &bd79124_write_event_value,
+};
+
+static void bd79124_re_enable_lo(struct bd79124_data *data, unsigned int channel)
+{
+	int ret, evbit = BIT(IIO_EV_DIR_FALLING);
+
+	if (!(data->alarm_suppressed[channel] & evbit))
+		return;
+
+	data->alarm_suppressed[channel] &= (~evbit);
+
+	if (!(data->alarm_monitored[channel] & evbit))
+		return;
+
+	ret = bd79124_write_int_to_reg(data, BD79124_GET_LOW_LIMIT_REG(channel),
+				       data->alarm_f_limit[channel]);
+	if (ret)
+		dev_warn(data->dev, "Low limit enabling failed for channel%d\n",
+			 channel);
+}
+
+static void bd79124_re_enable_hi(struct bd79124_data *data, unsigned int channel)
+{
+	int ret, evbit = BIT(IIO_EV_DIR_RISING);
+
+	if (!(data->alarm_suppressed[channel] & evbit))
+		return;
+
+	data->alarm_suppressed[channel] &= (~evbit);
+
+	if (!(data->alarm_monitored[channel] & evbit))
+		return;
+
+	ret = bd79124_write_int_to_reg(data, BD79124_GET_HIGH_LIMIT_REG(channel),
+				       data->alarm_r_limit[channel]);
+	if (ret)
+		dev_warn(data->dev, "High limit enabling failed for channel%d\n",
+			 channel);
+}
+
+static void bd79124_alm_enable_worker(struct work_struct *work)
+{
+	int i;
+	struct bd79124_data *data = container_of(work, struct bd79124_data,
+						 alm_enable_work.work);
+
+	guard(mutex)(&data->mutex);
+	/*
+	 * We should not re-enable the event if user has disabled it while
+	 * rate-limiting was enabled.
+	 */
+	for (i = 0; i < BD79124_MAX_NUM_CHANNELS; i++) {
+		bd79124_re_enable_hi(data, i);
+		bd79124_re_enable_lo(data, i);
+	}
+}
+
+static int __bd79124_event_ratelimit(struct bd79124_data *data, int reg,
+				     unsigned int limit)
+{
+	int ret;
+
+	if (limit > BD79124_HIGH_LIMIT_MAX)
+		return -EINVAL;
+
+	ret = bd79124_write_int_to_reg(data, reg, limit);
+	if (ret)
+		return ret;
+
+	/*
+	 * We use 1 sec 'grace period'. At the moment I see no reason to make
+	 * this user configurable. We need an ABI for this if configuration is
+	 * needed.
+	 */
+	schedule_delayed_work(&data->alm_enable_work,
+			      msecs_to_jiffies(1000));
+
+	return 0;
+}
+
+static int bd79124_event_ratelimit_hi(struct bd79124_data *data,
+				      unsigned int channel)
+{
+	guard(mutex)(&data->mutex);
+	data->alarm_suppressed[channel] |= BIT(IIO_EV_DIR_RISING);
+
+	return __bd79124_event_ratelimit(data,
+					 BD79124_GET_HIGH_LIMIT_REG(channel),
+					 BD79124_HIGH_LIMIT_MAX);
+}
+
+static int bd79124_event_ratelimit_lo(struct bd79124_data *data,
+				      unsigned int channel)
+{
+	guard(mutex)(&data->mutex);
+	data->alarm_suppressed[channel] |= BIT(IIO_EV_DIR_FALLING);
+
+	return __bd79124_event_ratelimit(data,
+					 BD79124_GET_LOW_LIMIT_REG(channel),
+					 BD79124_LOW_LIMIT_MIN);
+}
+
+static irqreturn_t bd79124_event_handler(int irq, void *priv)
+{
+	int ret, i_hi, i_lo, i;
+	struct iio_dev *iio_dev = priv;
+	struct bd79124_data *data = iio_priv(iio_dev);
+
+	/*
+	 * Return IRQ_NONE if bailing-out without acking. This allows the IRQ
+	 * subsystem to disable the offending IRQ line if we get a hardware
+	 * problem. This behaviour has saved my poor bottom a few times in the
+	 * past as, instead of getting unusably unresponsive, the system has
+	 * spilled out the magic words "...nobody cared".
+	 */
+	ret = regmap_read(data->map, BD79124_REG_EVENT_FLAG_HI, &i_hi);
+	if (ret)
+		return IRQ_NONE;
+
+	ret = regmap_read(data->map, BD79124_REG_EVENT_FLAG_LO, &i_lo);
+	if (ret)
+		return IRQ_NONE;
+
+	if (!i_lo && !i_hi)
+		return IRQ_NONE;
+
+	for (i = 0; i < BD79124_MAX_NUM_CHANNELS; i++) {
+		u64 ecode;
+
+		if (BIT(i) & i_hi) {
+			ecode = IIO_UNMOD_EVENT_CODE(IIO_VOLTAGE, i,
+					IIO_EV_TYPE_THRESH, IIO_EV_DIR_RISING);
+
+			iio_push_event(iio_dev, ecode, data->timestamp);
+			/*
+			 * The BD79124 keeps the IRQ asserted for as long as
+			 * the voltage exceeds the threshold. It causes the IRQ
+			 * to keep firing.
+			 *
+			 * Disable the event for the channel and schedule the
+			 * re-enabling the event later to prevent storm of
+			 * events.
+			 */
+			ret = bd79124_event_ratelimit_hi(data, i);
+			if (ret)
+				return IRQ_NONE;
+		}
+		if (BIT(i) & i_lo) {
+			ecode = IIO_UNMOD_EVENT_CODE(IIO_VOLTAGE, i,
+					IIO_EV_TYPE_THRESH, IIO_EV_DIR_FALLING);
+
+			iio_push_event(iio_dev, ecode, data->timestamp);
+			ret = bd79124_event_ratelimit_lo(data, i);
+			if (ret)
+				return IRQ_NONE;
+		}
+	}
+
+	ret = regmap_write(data->map, BD79124_REG_EVENT_FLAG_HI, i_hi);
+	if (ret)
+		return IRQ_NONE;
+
+	ret = regmap_write(data->map, BD79124_REG_EVENT_FLAG_LO, i_lo);
+	if (ret)
+		return IRQ_NONE;
+
+	return IRQ_HANDLED;
+}
+
+static irqreturn_t bd79124_irq_handler(int irq, void *priv)
+{
+	struct iio_dev *iio_dev = priv;
+	struct bd79124_data *data = iio_priv(iio_dev);
+
+	data->timestamp = iio_get_time_ns(iio_dev);
+
+	return IRQ_WAKE_THREAD;
+}
+
+struct bd79124_reg_init {
+	int reg;
+	int val;
+};
+
+static int bd79124_chan_init(struct bd79124_data *data, int channel)
+{
+	int ret;
+
+	ret = regmap_write(data->map, BD79124_GET_HIGH_LIMIT_REG(channel), 4095);
+	if (ret)
+		return ret;
+
+	return regmap_write(data->map, BD79124_GET_LOW_LIMIT_REG(channel), 0);
+}
+
+static int bd79124_get_gpio_pins(const struct iio_chan_spec *cs, int num_channels)
+{
+	int i, gpio_channels;
+
+	/*
+	 * Let's initialize the mux config to say that all 8 channels are
+	 * GPIOs. Then we can just loop through the iio_chan_spec and clear the
+	 * bits for found ADC channels.
+	 */
+	gpio_channels = GENMASK(7, 0);
+	for (i = 0; i < num_channels; i++)
+		gpio_channels &= (~BIT(cs[i].channel));
+
+	return gpio_channels;
+}
+
+static int bd79124_init_mux(struct bd79124_data *data, int gpio_pins)
+{
+	return regmap_write(data->map, BD79124_REG_PINCFG, gpio_pins);
+}
+
+static int bd79124_hw_init(struct bd79124_data *data, int gpio_pins)
+{
+	int ret, regval, i;
+
+	ret = bd79124_init_mux(data, gpio_pins);
+	if (ret)
+		return ret;
+
+	for (i = 0; i < BD79124_MAX_NUM_CHANNELS; i++) {
+		ret = bd79124_chan_init(data, i);
+		if (ret)
+			return ret;
+		data->alarm_r_limit[i] = 4095;
+	}
+	/* Stop auto sequencer */
+	ret = regmap_clear_bits(data->map, BD79124_REG_SEQUENCE_CFG,
+				BD79124_MASK_SEQ_START);
+	if (ret)
+		return ret;
+
+	/* Enable writing the measured values to the regsters */
+	ret = regmap_set_bits(data->map, BD79124_REG_GEN_CFG,
+			      BD79124_MASK_STATS_EN);
+	if (ret)
+		return ret;
+
+	/* Set no channels to be auto-measured */
+	ret = regmap_write(data->map, BD79124_REG_AUTO_CHANNELS, 0x0);
+	if (ret)
+		return ret;
+
+	/* Set no channels to be manually measured */
+	ret = regmap_write(data->map, BD79124_REG_MANUAL_CHANNELS, 0x0);
+	if (ret)
+		return ret;
+
+	/* Set the measurement interval to 0.75 mS */
+	regval = FIELD_PREP(BD79124_MASK_AUTO_INTERVAL, BD79124_INTERVAL_075);
+	ret = regmap_update_bits(data->map, BD79124_REG_OPMODE_CFG,
+				 BD79124_MASK_AUTO_INTERVAL, regval);
+	if (ret)
+		return ret;
+
+	/* Sequencer mode to auto */
+	ret = regmap_set_bits(data->map, BD79124_REG_SEQUENCE_CFG,
+			      BD79124_MASK_SEQ_SEQ);
+	if (ret)
+		return ret;
+
+	/* Don't start the measurement */
+	regval = FIELD_PREP(BD79124_MASK_CONV_MODE, BD79124_CONV_MODE_MANSEQ);
+	return regmap_update_bits(data->map, BD79124_REG_OPMODE_CFG,
+				  BD79124_MASK_CONV_MODE, regval);
+
+}
+
+static int bd79124_probe(struct i2c_client *i2c)
+{
+	struct bd79124_data *data;
+	struct iio_dev *iio_dev;
+	const struct iio_chan_spec *template;
+	struct iio_chan_spec *cs;
+	struct device *dev = &i2c->dev;
+	int gpio_pins, ret;
+
+	iio_dev = devm_iio_device_alloc(dev, sizeof(*data));
+	if (!iio_dev)
+		return -ENOMEM;
+
+	data = iio_priv(iio_dev);
+	data->dev = dev;
+	data->map = devm_regmap_init_i2c(i2c, &bd79124_regmap);
+	if (IS_ERR(data->map))
+		return dev_err_probe(dev, PTR_ERR(data->map),
+				     "Failed to initialize Regmap\n");
+
+	ret = devm_regulator_get_enable_read_voltage(dev, "vdd");
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "Failed to get the Vdd\n");
+
+	data->vmax = ret;
+
+	ret = devm_regulator_get_enable(dev, "iovdd");
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "Failed to enable I/O voltage\n");
+
+	ret = devm_delayed_work_autocancel(dev, &data->alm_enable_work,
+					   bd79124_alm_enable_worker);
+	if (ret)
+		return ret;
+
+	if (i2c->irq) {
+		template = &bd79124_chan_template;
+	} else {
+		template = &bd79124_chan_template_noirq;
+		dev_dbg(dev, "No IRQ found, events disabled\n");
+	}
+	ret = devm_iio_adc_device_alloc_chaninfo_se(dev, template,
+					BD79124_MAX_NUM_CHANNELS - 1, &cs);
+	if (ret < 0)
+		return ret;
+
+	iio_dev->channels = cs;
+	iio_dev->num_channels = ret;
+	iio_dev->info = &bd79124_info;
+	iio_dev->name = "bd79124";
+	iio_dev->modes = INDIO_DIRECT_MODE;
+
+	gpio_pins = bd79124_get_gpio_pins(iio_dev->channels,
+					  iio_dev->num_channels);
+	ret = bd79124_hw_init(data, gpio_pins);
+	if (ret)
+		return ret;
+
+	data->gpio_valid_mask = gpio_pins;
+	data->gc = bd79124gpo_chip;
+	data->gc.parent = dev;
+	devm_mutex_init(dev, &data->mutex);
+
+	ret = devm_gpiochip_add_data(dev, &data->gc, data);
+	if (ret)
+		return dev_err_probe(dev, ret, "gpio init Failed\n");
+
+	if (i2c->irq > 0) {
+		ret = devm_request_threaded_irq(dev, i2c->irq,
+				bd79124_irq_handler, &bd79124_event_handler,
+				IRQF_ONESHOT, "adc-thresh-alert", iio_dev);
+		if (ret)
+			return dev_err_probe(data->dev, ret,
+					     "Failed to register IRQ\n");
+	}
+
+	return devm_iio_device_register(data->dev, iio_dev);
+}
+
+static const struct of_device_id bd79124_of_match[] = {
+	{ .compatible = "rohm,bd79124" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, bd79124_of_match);
+
+static const struct i2c_device_id bd79124_id[] = {
+	{ "bd79124", },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, bd79124_id);
+
+static struct i2c_driver bd79124_driver = {
+	.driver = {
+		.name = "bd79124",
+		.of_match_table = bd79124_of_match,
+	},
+	.probe = bd79124_probe,
+	.id_table = bd79124_id,
+};
+module_i2c_driver(bd79124_driver);
+
+MODULE_AUTHOR("Matti Vaittinen <mazziesaccount@gmail.com>");
+MODULE_DESCRIPTION("Driver for ROHM BD79124 ADC");
+MODULE_LICENSE("GPL");
+MODULE_IMPORT_NS("IIO_DRIVER");
-- 
2.48.1


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

Re: [PATCH v4 08/10] iio: adc: Support ROHM BD79124 ADC

[Jonathan Cameron]

On Mon, 24 Feb 2025 20:34:30 +0200
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> 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 <mazziesaccount@gmail.com>
Some minor stuff inline.

Thanks,

Jonathan

> +
> +#define BD79124_REG_SYSTEM_STATUS	0x0
> +#define BD79124_REG_GEN_CFG		0x01
> +#define BD79124_REG_OPMODE_CFG		0x04
> +#define BD79124_REG_PINCFG		0x05
> +#define BD79124_REG_GPO_VAL		0x0B
> +#define BD79124_REG_SEQUENCE_CFG	0x10
> +#define BD79124_REG_MANUAL_CHANNELS	0x11
> +#define BD79124_REG_AUTO_CHANNELS	0x12
> +#define BD79124_REG_ALERT_CH_SEL	0x14
> +#define BD79124_REG_EVENT_FLAG		0x18
> +#define BD79124_REG_EVENT_FLAG_HI	0x1a
> +#define BD79124_REG_EVENT_FLAG_LO	0x1c
> +#define BD79124_REG_HYSTERESIS_CH0	0x20
> +#define BD79124_REG_EVENTCOUNT_CH0	0x22
> +#define BD79124_REG_RECENT_CH0_LSB	0xa0
> +#define BD79124_REG_RECENT_CH7_MSB	0xaf
> +
> +#define BD79124_ADC_BITS 12
> +#define BD79124_MASK_CONV_MODE GENMASK(6, 5)
> +#define BD79124_MASK_AUTO_INTERVAL GENMASK(1, 0)
> +#define BD79124_CONV_MODE_MANSEQ 0
> +#define BD79124_CONV_MODE_AUTO 1
> +#define BD79124_INTERVAL_075 0

Can we make these units in these explicit?
#define BD79124_INTERVAL_MS_0_75 
maybe?  Nice to avoid need for comments on what the units are where
you use these.

> +#define BD79124_INTERVAL_150 1
> +#define BD79124_INTERVAL_300 2
> +#define BD79124_INTERVAL_600 3

> +
> +static int bd79124_enable_event(struct bd79124_data *data,
> +		enum iio_event_direction dir, unsigned int channel)
> +{
> +	int dir_bit = BIT(dir);
> +	int reg;
> +	u16 *limit;
> +	int ret;
> +
> +	guard(mutex)(&data->mutex);
> +	/* Set channel to be measured */
> +	ret = bd79124_start_measurement(data, channel);
> +	if (ret)
> +		return ret;
> +
> +	data->alarm_monitored[channel] |= dir_bit;
> +
> +	/* Add the channel to the list of monitored channels */
> +	ret = regmap_set_bits(data->map, BD79124_REG_ALERT_CH_SEL,
> +			      BIT(channel));
> +	if (ret)
> +		return ret;
> +
> +	if (dir == IIO_EV_DIR_RISING) {
> +		limit = &data->alarm_f_limit[channel];
> +		reg = BD79124_GET_HIGH_LIMIT_REG(channel);
> +	} else {
> +		limit = &data->alarm_f_limit[channel];
> +		reg = BD79124_GET_LOW_LIMIT_REG(channel);
> +	}
> +	/* Don't write the new limit to the hardware if we are in the
> +	 * rate-limit period. The timer which re-enables the event will set
> +	 * the limit.
> +	 */
/*
 * Don't

Check for other cases of this...


> +static void bd79124_re_enable_hi(struct bd79124_data *data, unsigned int channel)
> +{
> +	int ret, evbit = BIT(IIO_EV_DIR_RISING);
> +
> +	if (!(data->alarm_suppressed[channel] & evbit))
> +		return;
> +
> +	data->alarm_suppressed[channel] &= (~evbit);

No brackets around the ~evbit.
Check for other cases of this.
Otherwise we'll get some script written 'cleanup'.


> +
> +	if (!(data->alarm_monitored[channel] & evbit))
> +		return;
> +
> +	ret = bd79124_write_int_to_reg(data, BD79124_GET_HIGH_LIMIT_REG(channel),
> +				       data->alarm_r_limit[channel]);
> +	if (ret)
> +		dev_warn(data->dev, "High limit enabling failed for channel%d\n",
> +			 channel);
> +}
> +
> +static void bd79124_alm_enable_worker(struct work_struct *work)
> +{
> +	int i;
> +	struct bd79124_data *data = container_of(work, struct bd79124_data,
> +						 alm_enable_work.work);
> +
> +	guard(mutex)(&data->mutex);
> +	/*
> +	 * We should not re-enable the event if user has disabled it while
> +	 * rate-limiting was enabled.
> +	 */

Is this comment suggesting something that isn't done or referring to specific
code?  I think it wants to be in the function above where the decision is made.

> +	for (i = 0; i < BD79124_MAX_NUM_CHANNELS; i++) {
> +		bd79124_re_enable_hi(data, i);
> +		bd79124_re_enable_lo(data, i);
> +	}
> +}
> +

> +

> +static irqreturn_t bd79124_event_handler(int irq, void *priv)
> +{
> +	int ret, i_hi, i_lo, i;
> +	struct iio_dev *iio_dev = priv;
> +	struct bd79124_data *data = iio_priv(iio_dev);
> +
> +	/*
> +	 * Return IRQ_NONE if bailing-out without acking. This allows the IRQ
> +	 * subsystem to disable the offending IRQ line if we get a hardware
> +	 * problem. This behaviour has saved my poor bottom a few times in the
> +	 * past as, instead of getting unusably unresponsive, the system has
> +	 * spilled out the magic words "...nobody cared".
> +	 */
> +	ret = regmap_read(data->map, BD79124_REG_EVENT_FLAG_HI, &i_hi);
> +	if (ret)
> +		return IRQ_NONE;
> +
> +	ret = regmap_read(data->map, BD79124_REG_EVENT_FLAG_LO, &i_lo);
> +	if (ret)
> +		return IRQ_NONE;
> +
> +	if (!i_lo && !i_hi)
> +		return IRQ_NONE;
> +
> +	for (i = 0; i < BD79124_MAX_NUM_CHANNELS; i++) {
> +		u64 ecode;
> +
> +		if (BIT(i) & i_hi) {
> +			ecode = IIO_UNMOD_EVENT_CODE(IIO_VOLTAGE, i,

> +					IIO_EV_TYPE_THRESH, IIO_EV_DIR_RISING);
Align this to less tabs as per discussion on previous version.

> +
> +
> +struct bd79124_reg_init {
> +	int reg;
> +	int val;
> +};

Not used any more.

> +
> +static int bd79124_chan_init(struct bd79124_data *data, int channel)
> +{
> +	int ret;
> +
> +	ret = regmap_write(data->map, BD79124_GET_HIGH_LIMIT_REG(channel), 4095);
> +	if (ret)
> +		return ret;
> +
> +	return regmap_write(data->map, BD79124_GET_LOW_LIMIT_REG(channel), 0);
> +}

> +
> +static int bd79124_init_mux(struct bd79124_data *data, int gpio_pins)
> +{
> +	return regmap_write(data->map, BD79124_REG_PINCFG, gpio_pins);

Maybe squash this inline.  Doesn't seem to add a lot to have the helper.

> +}
> +
> +static int bd79124_hw_init(struct bd79124_data *data, int gpio_pins)
> +{
...

> +
> +	/* Set the measurement interval to 0.75 mS */

This lead me to comment on defines.  I'd rather code was fully self
documenting and remove the comment if we can.  Makes for less chance of it
becoming out of sync over time.

> +	regval = FIELD_PREP(BD79124_MASK_AUTO_INTERVAL, BD79124_INTERVAL_075);
> +	ret = regmap_update_bits(data->map, BD79124_REG_OPMODE_CFG,
> +				 BD79124_MASK_AUTO_INTERVAL, regval);
> +	if (ret)
> +		return ret;

Re: [PATCH v4 08/10] iio: adc: Support ROHM BD79124 ADC

[Matti Vaittinen]

On 02/03/2025 06:10, Jonathan Cameron wrote:
> On Mon, 24 Feb 2025 20:34:30 +0200
> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> 
>> 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 <mazziesaccount@gmail.com>
> Some minor stuff inline.

...

>> +#define BD79124_INTERVAL_075 0
> 
> Can we make these units in these explicit?
> #define BD79124_INTERVAL_MS_0_75
> maybe?  Nice to avoid need for comments on what the units are where
> you use these.
> 

Sure, thanks.

>> +#define BD79124_INTERVAL_150 1
>> +#define BD79124_INTERVAL_300 2
>> +#define BD79124_INTERVAL_600 3

...

> 
>> +static void bd79124_re_enable_hi(struct bd79124_data *data, unsigned int channel)
>> +{
>> +	int ret, evbit = BIT(IIO_EV_DIR_RISING);
>> +
>> +	if (!(data->alarm_suppressed[channel] & evbit))
>> +		return;
>> +
>> +	data->alarm_suppressed[channel] &= (~evbit);
> 
> No brackets around the ~evbit.
> Check for other cases of this.
> Otherwise we'll get some script written 'cleanup'.

Sigh. I had a lengthy discussion about this with Andy explaining why I 
like having the parenthesis to avoid any confusion. Well, I suppose I 
have no options if you're strongly opposing them.

...

>> +static void bd79124_alm_enable_worker(struct work_struct *work)
>> +{
>> +	int i;
>> +	struct bd79124_data *data = container_of(work, struct bd79124_data,
>> +						 alm_enable_work.work);
>> +
>> +	guard(mutex)(&data->mutex);
>> +	/*
>> +	 * We should not re-enable the event if user has disabled it while
>> +	 * rate-limiting was enabled.
>> +	 */
> 
> Is this comment suggesting something that isn't done or referring to specific
> code?  I think it wants to be in the function above where the decision is made.

I have to take another look but it seems it got misplaced during the 
road. Thanks!

Agreeing with all the rest, thanks!

Yours,
	-- Matti

[PATCH v4 09/10] MAINTAINERS: Add IIO ADC helpers

[Matti Vaittinen]

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

Add undersigned as a maintainer for the IIO ADC helpers.

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

diff --git a/MAINTAINERS b/MAINTAINERS
index efee40ea589f..c1821fa8099b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11200,6 +11200,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 --]

[PATCH v4 10/10] MAINTAINERS: Add ROHM BD79124 ADC/GPO

[Matti Vaittinen]

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

Add undersigned as a maintainer for the ROHM BD79124 ADC/GPO driver.

Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
---
Revision history:
RFC v1 => v2:
 - Drop MFD and pinmux drivers
---
 MAINTAINERS | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index c1821fa8099b..b116a497d905 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -20533,6 +20533,11 @@ S:	Supported
 F:	drivers/power/supply/bd99954-charger.c
 F:	drivers/power/supply/bd99954-charger.h
 
+ROHM BD79124 ADC / GPO IC
+M:	Matti Vaittinen <mazziesaccount@gmail.com>
+S:	Supported
+F:	drivers/iio/adc/rohm-bd79124.c
+
 ROHM BH1745 COLOUR SENSOR
 M:	Mudit Sharma <muditsharma.info@gmail.com>
 L:	linux-iio@vger.kernel.org
-- 
2.48.1


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