[PATCH v2 0/5] Support ROHM BD79124 ADC/GPO

[PATCH v2 0/5] Support ROHM BD79124 ADC/GPO

[Matti Vaittinen]

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

Support ROHM BD79124 ADC.

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:
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 (5):
  dt-bindings: ROHM BD79124 ADC/GPO
  iio: adc: add helpers for parsing ADC nodes
  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/iio/adc/Kconfig                       |   15 +
 drivers/iio/adc/Makefile                      |    2 +
 drivers/iio/adc/industrialio-adc.c            |  151 +++
 drivers/iio/adc/rohm-bd79124.c                | 1149 +++++++++++++++++
 include/linux/iio/adc-helpers.h               |   22 +
 7 files changed, 1465 insertions(+)
 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: 5bc55a333a2f7316b58edc7573e8e893f7acb532
-- 
2.48.1


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

[PATCH v2 1/5] dt-bindings: ROHM BD79124 ADC/GPO

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 3331 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>
---
Revision history:
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..50889dc6b9a8
--- /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-9a-f]+$":
+    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 --]

Re: [PATCH v2 1/5] dt-bindings: ROHM BD79124 ADC/GPO

[Conor Dooley]

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

On Wed, Feb 05, 2025 at 03:34:29PM +0200, Matti Vaittinen wrote:
> 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>
> ---
> Revision history:
> 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..50889dc6b9a8
> --- /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-9a-f]+$":

You can only have 8 channels, there's no need for this to be so
permissive, right?
Otherwise, this looks good enough to me.

> +    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: 228 bytes --]

Re: [PATCH v2 1/5] dt-bindings: ROHM BD79124 ADC/GPO

[Matti Vaittinen]

On 05/02/2025 22:03, Conor Dooley wrote:
> On Wed, Feb 05, 2025 at 03:34:29PM +0200, Matti Vaittinen wrote:
>> 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>
>> ---
>> Revision history:
>> RFC v1 => v2:
>>   - drop MFD and represent directly as ADC
>>   - drop pinmux and treat all non ADC channel pins as GPOs
>> ---

...

>> +patternProperties:
>> +  "^channel@[0-9a-f]+$":
> 
> You can only have 8 channels, there's no need for this to be so
> permissive, right?
> Otherwise, this looks good enough to me.

Thanks Conor!

Indeed, I think we only need to accept channel@0 .. channel@7. Thanks!
(And reason for this being permissive is ... drum roll ... copy-paste :) 
That's my main method of creating the yaml bindings. Swearing, sweating, 
copying and running the 'make dt_binding_check'
:rolleyes:

I'll wait for a while for comments to the other patches and fix this for 
v3 :)

Yours,
	-- Matti

Re: [PATCH v2 1/5] dt-bindings: ROHM BD79124 ADC/GPO

[Conor Dooley]

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

On Thu, Feb 06, 2025 at 10:39:19AM +0200, Matti Vaittinen wrote:
> On 05/02/2025 22:03, Conor Dooley wrote:
> > On Wed, Feb 05, 2025 at 03:34:29PM +0200, Matti Vaittinen wrote:
> > > 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>
> > > ---
> > > Revision history:
> > > RFC v1 => v2:
> > >   - drop MFD and represent directly as ADC
> > >   - drop pinmux and treat all non ADC channel pins as GPOs
> > > ---
> 
> ...
> 
> > > +patternProperties:
> > > +  "^channel@[0-9a-f]+$":
> > 
> > You can only have 8 channels, there's no need for this to be so
> > permissive, right?
> > Otherwise, this looks good enough to me.
> 
> Thanks Conor!
> 
> Indeed, I think we only need to accept channel@0 .. channel@7. Thanks!
> (And reason for this being permissive is ... drum roll ... copy-paste :)
> That's my main method of creating the yaml bindings. Swearing, sweating,
> copying and running the 'make dt_binding_check'
> :rolleyes:
> 
> I'll wait for a while for comments to the other patches and fix this for v3
> :)

with it reduced,
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>

Cheers,
Conor.

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

[PATCH v2 2/5] iio: adc: add helpers for parsing ADC nodes

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 7416 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:
RFC v1 => v2:
 - New patch

I think it might be nice to have helpers for fetching also the other
generic (non vendor specific) ADC properties (as listed in the
Documentation/devicetree/bindings/iio/adc/adc.yaml) - but as I don't
have use for those in BD79124 driver (at least not for now), I don't
imnplement them yet. Anyways, this commit creates a place for such
helpers.
---
 drivers/iio/adc/Kconfig            |   3 +
 drivers/iio/adc/Makefile           |   1 +
 drivers/iio/adc/industrialio-adc.c | 151 +++++++++++++++++++++++++++++
 include/linux/iio/adc-helpers.h    |  22 +++++
 4 files changed, 177 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..956c121a7544 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -57,6 +57,7 @@ obj-$(CONFIG_FSL_MX25_ADC) += fsl-imx25-gcq.o
 obj-$(CONFIG_GEHC_PMC_ADC) += gehc-pmc-adc.o
 obj-$(CONFIG_HI8435) += hi8435.o
 obj-$(CONFIG_HX711) += hx711.o
+obj-$(CONFIG_IIO_ADC_HELPER) += industrialio-adc.o
 obj-$(CONFIG_IMX7D_ADC) += imx7d_adc.o
 obj-$(CONFIG_IMX8QXP_ADC) += imx8qxp-adc.o
 obj-$(CONFIG_IMX93_ADC) += imx93_adc.o
diff --git a/drivers/iio/adc/industrialio-adc.c b/drivers/iio/adc/industrialio-adc.c
new file mode 100644
index 000000000000..366e4c8eb6c7
--- /dev/null
+++ b/drivers/iio/adc/industrialio-adc.c
@@ -0,0 +1,151 @@
+// 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/property.h>
+
+#include <linux/iio/adc-helpers.h>
+
+int iio_adc_fwnode_num_channels(struct fwnode_handle *fwnode)
+{
+	struct fwnode_handle *child;
+	int num_chan = 0;
+
+	fwnode_for_each_child_node(fwnode, child)
+		if (fwnode_name_eq(child, "channel"))
+			num_chan++;
+
+	return num_chan;
+}
+EXPORT_SYMBOL_GPL(iio_adc_fwnode_num_channels);
+
+/**
+ * iio_adc_device_get_channels - get ADC channel IDs
+ *
+ * Scan the device node for ADC channel information. Return an array of found
+ * IDs. Caller need to allocate the memory for the array and provide maximum
+ * number of IDs the array can store.
+ *
+ * @dev:		Pointer to the ADC device
+ * @channels:		Array where the found IDs will be stored.
+ * @max_channels:	Number of IDs that fit in the array.
+ *
+ * Return:		Number of found channels on succes. Negative value to
+ *			indicate failure.
+ */
+int iio_adc_device_get_channels(struct device *dev, int *channels,
+				int max_channels)
+{
+	struct fwnode_handle *fwnode, *child;
+	int num_chan = 0, ret;
+
+	fwnode = dev_fwnode(dev);
+	if (!fwnode) {
+		fwnode = dev_fwnode(dev->parent);
+		if (!fwnode)
+			return -ENODEV;
+	}
+	fwnode_for_each_child_node(fwnode, child) {
+		if (fwnode_name_eq(child, "channel")) {
+			u32 ch;
+
+			if (num_chan == max_channels)
+				return -EINVAL;
+
+			ret = fwnode_property_read_u32(child, "reg", &ch);
+			if (ret)
+				return ret;
+
+			/*
+			 * We assume the channel IDs start from 0. If it seems
+			 * this is not a sane assumption, then we can relax
+			 * this check or add 'allowed ID range' parameter.
+			 *
+			 * Let's just start with this simple assumption.
+			 */
+			if (ch > max_channels)
+				return -ERANGE;
+
+			channels[num_chan] = ch;
+			num_chan++;
+		}
+	}
+
+	return num_chan;
+
+}
+EXPORT_SYMBOL_GPL(iio_adc_device_get_channels);
+
+/**
+ * devm_iio_adc_device_alloc_chaninfo - allocate and fill iio_chan_spec for adc
+ *
+ * Scan the device node for ADC channel information. 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. Try parent
+ * device node if given device has no fwnode associated to cover also MFD
+ * devices.
+ *
+ * @dev:	Pointer to the ADC device
+ * @template:	Template iio_chan_spec from which the fields of all found and
+ *		allocated channels are initialized.
+ * @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(struct device *dev,
+				       const struct iio_chan_spec *template,
+				       struct iio_chan_spec **cs)
+{
+	struct fwnode_handle *fwnode, *child;
+	struct iio_chan_spec *chan;
+	int num_chan = 0, ret;
+
+	fwnode = dev_fwnode(dev);
+	if (!fwnode) {
+		fwnode = dev_fwnode(dev->parent);
+		if (!fwnode)
+			return -ENODEV;
+	}
+
+	num_chan = iio_adc_fwnode_num_channels(fwnode);
+	if (num_chan < 0)
+		return num_chan;
+
+	*cs = devm_kcalloc(dev, num_chan, sizeof(**cs), GFP_KERNEL);
+	if (!*cs)
+		return -ENOMEM;
+
+	chan = &(*cs)[0];
+
+	fwnode_for_each_child_node(fwnode, child) {
+		if (fwnode_name_eq(child, "channel")) {
+			u32 ch;
+
+			ret = fwnode_property_read_u32(child, "reg", &ch);
+			if (ret)
+				return ret;
+
+			*chan = *template;
+			chan->channel = ch;
+
+			if (num_chan > 1)
+				chan->indexed = 1;
+
+			chan++;
+		}
+	}
+
+	return num_chan;
+}
+EXPORT_SYMBOL_GPL(devm_iio_adc_device_alloc_chaninfo);
+
+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..3801b2d17517
--- /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 helpers
+ *
+ * Copyright (c) 2025 Matti Vaittinen <mazziesaccount@gmail.com>
+ */
+
+#ifndef _INDUSTRIAL_IO_ADC_HELPERS_H_
+#define _INDUSTRIAL_IO_ADC_HELPERS_H_
+
+#include <linux/iio/iio.h>
+
+struct device;
+struct fwnode_handle;
+
+int iio_adc_fwnode_num_channels(struct fwnode_handle *fwnode);
+int devm_iio_adc_device_alloc_chaninfo(struct device *dev,
+				       const struct iio_chan_spec *template,
+				       struct iio_chan_spec **cs);
+int iio_adc_device_get_channels(struct device *dev, int *channels,
+				int max_channels);
+#endif /* _INDUSTRIAL_IO_ADC_HELPERS_H_ */
-- 
2.48.1


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

Re: [PATCH v2 2/5] iio: adc: add helpers for parsing ADC nodes

[Jonathan Cameron]

On Wed, 5 Feb 2025 15:34:51 +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:
> RFC v1 => v2:
>  - New patch
> 
> I think it might be nice to have helpers for fetching also the other
> generic (non vendor specific) ADC properties (as listed in the
> Documentation/devicetree/bindings/iio/adc/adc.yaml) - but as I don't
> have use for those in BD79124 driver (at least not for now), I don't
> imnplement them yet. Anyways, this commit creates a place for such
> helpers.

There is often a mix of vendor specific and not in channel nodes.
Hence I'm not sure how widely this will be and it is driver
specific which of the standard things make sense.

So before I'd consider a helper like this I'd want to see it alongside
a bunch of users including some of the complex ones so that we know
it generalizes well enough.  It doesn't make sense to introduce
it otherwise - just keep the code in the specific drivers instead.

It's an interesting idea, but not a trivial one :)

Jonathan


> ---
>  drivers/iio/adc/Kconfig            |   3 +
>  drivers/iio/adc/Makefile           |   1 +
>  drivers/iio/adc/industrialio-adc.c | 151 +++++++++++++++++++++++++++++
>  include/linux/iio/adc-helpers.h    |  22 +++++
>  4 files changed, 177 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..956c121a7544 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -57,6 +57,7 @@ obj-$(CONFIG_FSL_MX25_ADC) += fsl-imx25-gcq.o
>  obj-$(CONFIG_GEHC_PMC_ADC) += gehc-pmc-adc.o
>  obj-$(CONFIG_HI8435) += hi8435.o
>  obj-$(CONFIG_HX711) += hx711.o
> +obj-$(CONFIG_IIO_ADC_HELPER) += industrialio-adc.o
>  obj-$(CONFIG_IMX7D_ADC) += imx7d_adc.o
>  obj-$(CONFIG_IMX8QXP_ADC) += imx8qxp-adc.o
>  obj-$(CONFIG_IMX93_ADC) += imx93_adc.o
> diff --git a/drivers/iio/adc/industrialio-adc.c b/drivers/iio/adc/industrialio-adc.c
> new file mode 100644
> index 000000000000..366e4c8eb6c7
> --- /dev/null
> +++ b/drivers/iio/adc/industrialio-adc.c
> @@ -0,0 +1,151 @@


> +
> +/**
> + * iio_adc_device_get_channels - get ADC channel IDs

This sounds far too like the inkern interfaces.  Need to associate it instead
with the fwnode / properties stuff.

> + *
> + * Scan the device node for ADC channel information. Return an array of found
> + * IDs. Caller need to allocate the memory for the array and provide maximum

Caller needs to provide the memory

> + * number of IDs the array can store.
> + *
> + * @dev:		Pointer to the ADC device
> + * @channels:		Array where the found IDs will be stored.
> + * @max_channels:	Number of IDs that fit in the array.
> + *
> + * Return:		Number of found channels on succes. Negative value to
> + *			indicate failure.
> + */
> +int iio_adc_device_get_channels(struct device *dev, int *channels,
> +				int max_channels)
> +{
> +	struct fwnode_handle *fwnode, *child;
> +	int num_chan = 0, ret;
> +
> +	fwnode = dev_fwnode(dev);
> +	if (!fwnode) {

As before, I'd relax this until we need to do it. We may never do so.

> +		fwnode = dev_fwnode(dev->parent);
> +		if (!fwnode)
> +			return -ENODEV;
> +	}
> +	fwnode_for_each_child_node(fwnode, child) {
> +		if (fwnode_name_eq(child, "channel")) {

As below. I'd flip the logic here and use a continue

> +			u32 ch;
> +
> +			if (num_chan == max_channels)
> +				return -EINVAL;
> +
> +			ret = fwnode_property_read_u32(child, "reg", &ch);
> +			if (ret)
> +				return ret;
> +
> +			/*
> +			 * We assume the channel IDs start from 0. If it seems
> +			 * this is not a sane assumption, then we can relax
> +			 * this check or add 'allowed ID range' parameter.
> +			 *
> +			 * Let's just start with this simple assumption.
> +			 */
> +			if (ch > max_channels)
> +				return -ERANGE;
> +
> +			channels[num_chan] = ch;
> +			num_chan++;
			channel[num_chan++] = ch;

So it is clear how the two are coupled.

> +		}
> +	}
> +
> +	return num_chan;
> +
> +}
> +EXPORT_SYMBOL_GPL(iio_adc_device_get_channels);
> +
> +/**
> + * devm_iio_adc_device_alloc_chaninfo - allocate and fill iio_chan_spec for adc
> + *
> + * Scan the device node for ADC channel information. 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. Try parent
> + * device node if given device has no fwnode associated to cover also MFD
> + * devices.

I'd leave that parent node check until we need it (unless you need it for
this one!).  Feels like infra structure that might never be used.
That would let us for now use the device_for_each_child_node()
handling.

> + *
> + * @dev:	Pointer to the ADC device
> + * @template:	Template iio_chan_spec from which the fields of all found and
> + *		allocated channels are initialized.
> + * @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(struct device *dev,
> +				       const struct iio_chan_spec *template,
> +				       struct iio_chan_spec **cs)
> +{
> +	struct fwnode_handle *fwnode, *child;
> +	struct iio_chan_spec *chan;
> +	int num_chan = 0, ret;
> +
> +	fwnode = dev_fwnode(dev);
> +	if (!fwnode) {
> +		fwnode = dev_fwnode(dev->parent);
> +		if (!fwnode)
> +			return -ENODEV;
> +	}
> +
> +	num_chan = iio_adc_fwnode_num_channels(fwnode);
> +	if (num_chan < 0)
> +		return num_chan;
> +
> +	*cs = devm_kcalloc(dev, num_chan, sizeof(**cs), GFP_KERNEL);
> +	if (!*cs)
> +		return -ENOMEM;
> +
> +	chan = &(*cs)[0];
> +
> +	fwnode_for_each_child_node(fwnode, child) {
> +		if (fwnode_name_eq(child, "channel")) {
Flip logic probably and use a continue to reduce indent of
the next bit (which may well get a lot more complex in future).

> +			u32 ch;
> +
> +			ret = fwnode_property_read_u32(child, "reg", &ch);
> +			if (ret)
> +				return ret;

In general the association between reg and channel is more complex.
We need to deal with a reasonable subset of cases (single-channel, diff-channels
etc) where it isn't the case that chan == chan->channel.

> +
> +			*chan = *template;
> +			chan->channel = ch;
> +
> +			if (num_chan > 1)
> +				chan->indexed = 1;

I think set this whatever, probably in the template.
I don't want to see the interface change just because a given DT says
only one channel is connected.

> +
> +			chan++;
> +		}
> +	}
> +
> +	return num_chan;
> +}
> +EXPORT_SYMBOL_GPL(devm_iio_adc_device_alloc_chaninfo);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Matti Vaittinen <mazziesaccount@gmail.com>");
> +MODULE_DESCRIPTION("IIO ADC fwnode parsing helpers");

Re: [PATCH v2 2/5] iio: adc: add helpers for parsing ADC nodes

[Matti Vaittinen]

Hi Jonathan,

Thanks for the review and all the comments!

Just a note that I am currently spending some quality time with 
rebuilding the floor of my house. Leaking floor drain can cause a bit of 
a work... :rolleyes: So, my time with upstream work is a bit limited - 
although writing an occasional bug or two can help one to keep the 
balance ;)

Anyways, my replies and new versions may be slower than usual..

On 08/02/2025 18:41, Jonathan Cameron wrote:
> On Wed, 5 Feb 2025 15:34:51 +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:
>> RFC v1 => v2:
>>   - New patch
>>
>> I think it might be nice to have helpers for fetching also the other
>> generic (non vendor specific) ADC properties (as listed in the
>> Documentation/devicetree/bindings/iio/adc/adc.yaml) - but as I don't
>> have use for those in BD79124 driver (at least not for now), I don't
>> imnplement them yet. Anyways, this commit creates a place for such
>> helpers.
> 
> There is often a mix of vendor specific and not in channel nodes.
> Hence I'm not sure how widely this will be and it is driver
> specific which of the standard things make sense.

I definitely agree. Still, in my experience, no written standard 
standardizes use as well as written helpers ;) More we support parsing 
the generic helpers by the (add subsystem here)-core, more the driver 
writes will use the generic properties (instead of brewing vendor 
specific ones).

> So before I'd consider a helper like this I'd want to see it alongside
> a bunch of users including some of the complex ones so that we know
> it generalizes well enough.  It doesn't make sense to introduce
> it otherwise - just keep the code in the specific drivers instead.
> 
> It's an interesting idea, but not a trivial one :)

I agree it's not trivial. But I believe adding helpers one-by-one to 
cover 'normal' use-cases guide the use of the properties. Those who need 
something more exotic can always implement their custom handlers - and 
then a reviewer of such handler can ask "why" ;)


>> ---
>>   drivers/iio/adc/Kconfig            |   3 +
>>   drivers/iio/adc/Makefile           |   1 +
>>   drivers/iio/adc/industrialio-adc.c | 151 +++++++++++++++++++++++++++++
>>   include/linux/iio/adc-helpers.h    |  22 +++++
>>   4 files changed, 177 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..956c121a7544 100644
>> --- a/drivers/iio/adc/Makefile
>> +++ b/drivers/iio/adc/Makefile
>> @@ -57,6 +57,7 @@ obj-$(CONFIG_FSL_MX25_ADC) += fsl-imx25-gcq.o
>>   obj-$(CONFIG_GEHC_PMC_ADC) += gehc-pmc-adc.o
>>   obj-$(CONFIG_HI8435) += hi8435.o
>>   obj-$(CONFIG_HX711) += hx711.o
>> +obj-$(CONFIG_IIO_ADC_HELPER) += industrialio-adc.o
>>   obj-$(CONFIG_IMX7D_ADC) += imx7d_adc.o
>>   obj-$(CONFIG_IMX8QXP_ADC) += imx8qxp-adc.o
>>   obj-$(CONFIG_IMX93_ADC) += imx93_adc.o
>> diff --git a/drivers/iio/adc/industrialio-adc.c b/drivers/iio/adc/industrialio-adc.c
>> new file mode 100644
>> index 000000000000..366e4c8eb6c7
>> --- /dev/null
>> +++ b/drivers/iio/adc/industrialio-adc.c
>> @@ -0,0 +1,151 @@
> 
> 
>> +
>> +/**
>> + * iio_adc_device_get_channels - get ADC channel IDs
> 
> This sounds far too like the inkern interfaces.  Need to associate it instead
> with the fwnode / properties stuff.

I was juggling between using the 'fwnode' and 'device'. I took the 
'device' approach in order to be consistent with the naming convention 
for the functions in property.h.

For example:
device_property_read_u<N>_array() and
fwnode_property_read_u<N>_array()

depending on if the node is found via device pointer, or if the fwnode 
is passed directly.

> 
>> + *
>> + * Scan the device node for ADC channel information. Return an array of found
>> + * IDs. Caller need to allocate the memory for the array and provide maximum
> 
> Caller needs to provide the memory

Thanks!

> 
>> + * number of IDs the array can store.
>> + *
>> + * @dev:		Pointer to the ADC device
>> + * @channels:		Array where the found IDs will be stored.
>> + * @max_channels:	Number of IDs that fit in the array.
>> + *
>> + * Return:		Number of found channels on succes. Negative value to
>> + *			indicate failure.
>> + */
>> +int iio_adc_device_get_channels(struct device *dev, int *channels,
>> +				int max_channels)
>> +{
>> +	struct fwnode_handle *fwnode, *child;
>> +	int num_chan = 0, ret;
>> +
>> +	fwnode = dev_fwnode(dev);
>> +	if (!fwnode) {
> 
> As before, I'd relax this until we need to do it. We may never do so.

The BD79124 does not require this as I dropped the MFD, so this is 
ultimately your call :) I, however, would like to humbly suggest adding 
it now ;) I have changed some APIs in the regulator subsystem and in the 
clk subsystem to support cases where the device-tree node is in the 
parent (usual MFD device-case), and it has been somewhat scary... (What 
if somewhere in some of the existing device-trees the parent happens to 
have interesting properties - but it actually is not correct node? This 
becomes a potential source of regression when adding support to an 
existing API).

Furthermore, when the node is unconditionally taken from the given 
device-pointer, it is tempting for the caller to just pass the parent 
device as argument...

  - If you have done this - please raise your hand. /me raises.
  - If you have only later realized it can cause life-time issues when
    devm is used - please raise your hand. /me raises.
  - If you have tried to kick your own behind when fixing the issues -
    please, raise your hand. /me raises. (and if you succeeded - congraz,
    you aren't as old and clumsy as I am).

> 
>> +		fwnode = dev_fwnode(dev->parent);
>> +		if (!fwnode)
>> +			return -ENODEV;
>> +	}
>> +	fwnode_for_each_child_node(fwnode, child) {
>> +		if (fwnode_name_eq(child, "channel")) {
> 
> As below. I'd flip the logic here and use a continue

Makes sense.

> 
>> +			u32 ch;
>> +
>> +			if (num_chan == max_channels)
>> +				return -EINVAL;
>> +
>> +			ret = fwnode_property_read_u32(child, "reg", &ch);
>> +			if (ret)
>> +				return ret;
>> +
>> +			/*
>> +			 * We assume the channel IDs start from 0. If it seems
>> +			 * this is not a sane assumption, then we can relax
>> +			 * this check or add 'allowed ID range' parameter.
>> +			 *
>> +			 * Let's just start with this simple assumption.
>> +			 */
>> +			if (ch > max_channels)
>> +				return -ERANGE;
>> +
>> +			channels[num_chan] = ch;
>> +			num_chan++;
> 			channel[num_chan++] = ch;
> 
> So it is clear how the two are coupled.

Ouch. I am not fan of this. I have a personal issue as I always need to 
wonder if this was the case where the ++foo and foo++ resulted different 
functionality. I end up doing a test to see in which index the result 
got stored. If you don't feel stronly about this, then I would like to 
keep the index increase and assignment in separate rows. I believe the 
coupling is still quite visible for as long as we keep the assignment 
and increase next to each other. But yeah, if you do feel strongly about 
this, then it can be implemented as you suggest.

> 
>> +		}
>> +	}
>> +
>> +	return num_chan;
>> +
>> +}
>> +EXPORT_SYMBOL_GPL(iio_adc_device_get_channels);
>> +
>> +/**
>> + * devm_iio_adc_device_alloc_chaninfo - allocate and fill iio_chan_spec for adc
>> + *
>> + * Scan the device node for ADC channel information. 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. Try parent
>> + * device node if given device has no fwnode associated to cover also MFD
>> + * devices.
> 
> I'd leave that parent node check until we need it (unless you need it for
> this one!).  Feels like infra structure that might never be used.
> That would let us for now use the device_for_each_child_node()
> handling.

As above, I adviece to implement the parent device use right from the 
beginning - but I can change this as BD79124 dropped MFD.

> 
>> + *
>> + * @dev:	Pointer to the ADC device
>> + * @template:	Template iio_chan_spec from which the fields of all found and
>> + *		allocated channels are initialized.
>> + * @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(struct device *dev,
>> +				       const struct iio_chan_spec *template,
>> +				       struct iio_chan_spec **cs)
>> +{
>> +	struct fwnode_handle *fwnode, *child;
>> +	struct iio_chan_spec *chan;
>> +	int num_chan = 0, ret;
>> +
>> +	fwnode = dev_fwnode(dev);
>> +	if (!fwnode) {
>> +		fwnode = dev_fwnode(dev->parent);
>> +		if (!fwnode)
>> +			return -ENODEV;
>> +	}
>> +
>> +	num_chan = iio_adc_fwnode_num_channels(fwnode);
>> +	if (num_chan < 0)
>> +		return num_chan;
>> +
>> +	*cs = devm_kcalloc(dev, num_chan, sizeof(**cs), GFP_KERNEL);
>> +	if (!*cs)
>> +		return -ENOMEM;
>> +
>> +	chan = &(*cs)[0];
>> +
>> +	fwnode_for_each_child_node(fwnode, child) {
>> +		if (fwnode_name_eq(child, "channel")) {
> Flip logic probably and use a continue to reduce indent of
> the next bit (which may well get a lot more complex in future).
> 

Right, thanks.

>> +			u32 ch;
>> +
>> +			ret = fwnode_property_read_u32(child, "reg", &ch);
>> +			if (ret)
>> +				return ret;
> 
> In general the association between reg and channel is more complex.
> We need to deal with a reasonable subset of cases (single-channel, diff-channels
> etc) where it isn't the case that chan == chan->channel.

I guess this is right. I, however, lacked of knowledge on how the 
differential channels etc. are handled :) Hence I just implemented what 
I believe is "the most common" approach, while leaving the rest to be 
implemented by those who need it.

Still, I agree that a generic looking API which silently produces bad 
results for a few of the use-cases is not nice. By the very minimum we 
should check if single-channel, diff-channels properties are present, 
and then error out with a warning print. That should give a good hint 
for those driver writers who are trying to use API for something 
unsupported.

Also, restrictions should be mentioned in the documentation.

Do you think we should use some more specific function naming, like 
"_simple" - suffix?

> 
>> +
>> +			*chan = *template;
>> +			chan->channel = ch;
>> +
>> +			if (num_chan > 1)
>> +				chan->indexed = 1;
> 
> I think set this whatever, probably in the template.
> I don't want to see the interface change just because a given DT says
> only one channel is connected.

Ah. I didn't think of that! Good point.


Yours,
   -- Matti

Re: [PATCH v2 2/5] iio: adc: add helpers for parsing ADC nodes

[Jonathan Cameron]

On Tue, 11 Feb 2025 10:52:51 +0200
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> Hi Jonathan,
> 
> Thanks for the review and all the comments!
> 
> Just a note that I am currently spending some quality time with 
> rebuilding the floor of my house. Leaking floor drain can cause a bit of 
> a work... :rolleyes: So, my time with upstream work is a bit limited - 
> although writing an occasional bug or two can help one to keep the 
> balance ;)
> 
> Anyways, my replies and new versions may be slower than usual..
> 
> On 08/02/2025 18:41, Jonathan Cameron wrote:
> > On Wed, 5 Feb 2025 15:34:51 +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:
> >> RFC v1 => v2:
> >>   - New patch
> >>
> >> I think it might be nice to have helpers for fetching also the other
> >> generic (non vendor specific) ADC properties (as listed in the
> >> Documentation/devicetree/bindings/iio/adc/adc.yaml) - but as I don't
> >> have use for those in BD79124 driver (at least not for now), I don't
> >> imnplement them yet. Anyways, this commit creates a place for such
> >> helpers.  
> > 
> > There is often a mix of vendor specific and not in channel nodes.
> > Hence I'm not sure how widely this will be and it is driver
> > specific which of the standard things make sense.  
> 
> I definitely agree. Still, in my experience, no written standard 
> standardizes use as well as written helpers ;) More we support parsing 
> the generic helpers by the (add subsystem here)-core, more the driver 
> writes will use the generic properties (instead of brewing vendor 
> specific ones).
> 
> > So before I'd consider a helper like this I'd want to see it alongside
> > a bunch of users including some of the complex ones so that we know
> > it generalizes well enough.  It doesn't make sense to introduce
> > it otherwise - just keep the code in the specific drivers instead.
> > 
> > It's an interesting idea, but not a trivial one :)  
> 
> I agree it's not trivial. But I believe adding helpers one-by-one to 
> cover 'normal' use-cases guide the use of the properties. Those who need 
> something more exotic can always implement their custom handlers - and 
> then a reviewer of such handler can ask "why" ;)
I'd be fine with a series taking on the task of converting handling of
all the documented properties in adc.yaml

If we do less than that it may never get wide adoption and we end
up with a bit of generic looking infrastructure that isn't generic.

> 
> 
> >> ---
> >>   drivers/iio/adc/Kconfig            |   3 +
> >>   drivers/iio/adc/Makefile           |   1 +
> >>   drivers/iio/adc/industrialio-adc.c | 151 +++++++++++++++++++++++++++++
> >>   include/linux/iio/adc-helpers.h    |  22 +++++
> >>   4 files changed, 177 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..956c121a7544 100644
> >> --- a/drivers/iio/adc/Makefile
> >> +++ b/drivers/iio/adc/Makefile
> >> @@ -57,6 +57,7 @@ obj-$(CONFIG_FSL_MX25_ADC) += fsl-imx25-gcq.o
> >>   obj-$(CONFIG_GEHC_PMC_ADC) += gehc-pmc-adc.o
> >>   obj-$(CONFIG_HI8435) += hi8435.o
> >>   obj-$(CONFIG_HX711) += hx711.o
> >> +obj-$(CONFIG_IIO_ADC_HELPER) += industrialio-adc.o
> >>   obj-$(CONFIG_IMX7D_ADC) += imx7d_adc.o
> >>   obj-$(CONFIG_IMX8QXP_ADC) += imx8qxp-adc.o
> >>   obj-$(CONFIG_IMX93_ADC) += imx93_adc.o
> >> diff --git a/drivers/iio/adc/industrialio-adc.c b/drivers/iio/adc/industrialio-adc.c
> >> new file mode 100644
> >> index 000000000000..366e4c8eb6c7
> >> --- /dev/null
> >> +++ b/drivers/iio/adc/industrialio-adc.c
> >> @@ -0,0 +1,151 @@  
> > 
> >   
> >> +
> >> +/**
> >> + * iio_adc_device_get_channels - get ADC channel IDs  
> > 
> > This sounds far too like the inkern interfaces.  Need to associate it instead
> > with the fwnode / properties stuff.  
> 
> I was juggling between using the 'fwnode' and 'device'. I took the 
> 'device' approach in order to be consistent with the naming convention 
> for the functions in property.h.
> 
> For example:
> device_property_read_u<N>_array() and
> fwnode_property_read_u<N>_array()
> 
> depending on if the node is found via device pointer, or if the fwnode 
> is passed directly.
hmm.  The bit to avoid is 'get'.  

iio_adc_device_channels_from_properties() maybe?

> 
> >   
> >> + *
> >> + * Scan the device node for ADC channel information. Return an array of found
> >> + * IDs. Caller need to allocate the memory for the array and provide maximum  
> > 
> > Caller needs to provide the memory  
> 
> Thanks!
> 
> >   
> >> + * number of IDs the array can store.
> >> + *
> >> + * @dev:		Pointer to the ADC device
> >> + * @channels:		Array where the found IDs will be stored.
> >> + * @max_channels:	Number of IDs that fit in the array.
> >> + *
> >> + * Return:		Number of found channels on succes. Negative value to
> >> + *			indicate failure.
> >> + */
> >> +int iio_adc_device_get_channels(struct device *dev, int *channels,
> >> +				int max_channels)
> >> +{
> >> +	struct fwnode_handle *fwnode, *child;
> >> +	int num_chan = 0, ret;
> >> +
> >> +	fwnode = dev_fwnode(dev);
> >> +	if (!fwnode) {  
> > 
> > As before, I'd relax this until we need to do it. We may never do so.  
> 
> The BD79124 does not require this as I dropped the MFD, so this is 
> ultimately your call :) I, however, would like to humbly suggest adding 
> it now ;) I have changed some APIs in the regulator subsystem and in the 
> clk subsystem to support cases where the device-tree node is in the 
> parent (usual MFD device-case), and it has been somewhat scary... (What 
> if somewhere in some of the existing device-trees the parent happens to 
> have interesting properties - but it actually is not correct node? This 
> becomes a potential source of regression when adding support to an 
> existing API).
> 
> Furthermore, when the node is unconditionally taken from the given 
> device-pointer, it is tempting for the caller to just pass the parent 
> device as argument...
> 
>   - If you have done this - please raise your hand. /me raises.
>   - If you have only later realized it can cause life-time issues when
>     devm is used - please raise your hand. /me raises.
>   - If you have tried to kick your own behind when fixing the issues -
>     please, raise your hand. /me raises. (and if you succeeded - congraz,
>     you aren't as old and clumsy as I am).

Maybe. I'd be happier if there was a single user included
with a patch set doing this.  I'm not sure this applies to
any of the SoC ADCs which are MFD hosted but maybe it does.

> 
> >   
> >> +		fwnode = dev_fwnode(dev->parent);
> >> +		if (!fwnode)
> >> +			return -ENODEV;
> >> +	}
> >> +	fwnode_for_each_child_node(fwnode, child) {
> >> +		if (fwnode_name_eq(child, "channel")) {  
> > 
> > As below. I'd flip the logic here and use a continue  
> 
> Makes sense.
> 
> >   
> >> +			u32 ch;
> >> +
> >> +			if (num_chan == max_channels)
> >> +				return -EINVAL;
> >> +
> >> +			ret = fwnode_property_read_u32(child, "reg", &ch);
> >> +			if (ret)
> >> +				return ret;
> >> +
> >> +			/*
> >> +			 * We assume the channel IDs start from 0. If it seems
> >> +			 * this is not a sane assumption, then we can relax
> >> +			 * this check or add 'allowed ID range' parameter.
> >> +			 *
> >> +			 * Let's just start with this simple assumption.
> >> +			 */
> >> +			if (ch > max_channels)
> >> +				return -ERANGE;
> >> +
> >> +			channels[num_chan] = ch;
> >> +			num_chan++;  
> > 			channel[num_chan++] = ch;
> > 
> > So it is clear how the two are coupled.  
> 
> Ouch. I am not fan of this. I have a personal issue as I always need to 
> wonder if this was the case where the ++foo and foo++ resulted different 
> functionality. I end up doing a test to see in which index the result 
> got stored. If you don't feel stronly about this, then I would like to 
> keep the index increase and assignment in separate rows. I believe the 
> coupling is still quite visible for as long as we keep the assignment 
> and increase next to each other. But yeah, if you do feel strongly about 
> this, then it can be implemented as you suggest.

It's that question of them staying next to each other that is where
the included increment helps.  I don't mind that much on this.

> 
> >   
> >> +		}
> >> +	}
> >> +
> >> +	return num_chan;
> >> +
> >> +}
> >> +EXPORT_SYMBOL_GPL(iio_adc_device_get_channels);
> >> +
> >> +/**
> >> + * devm_iio_adc_device_alloc_chaninfo - allocate and fill iio_chan_spec for adc
> >> + *
> >> + * Scan the device node for ADC channel information. 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. Try parent
> >> + * device node if given device has no fwnode associated to cover also MFD
> >> + * devices.  
> > 
> > I'd leave that parent node check until we need it (unless you need it for
> > this one!).  Feels like infra structure that might never be used.
> > That would let us for now use the device_for_each_child_node()
> > handling.  
> 
> As above, I adviece to implement the parent device use right from the 
> beginning - but I can change this as BD79124 dropped MFD.
> 
> >   
> >> + *
> >> + * @dev:	Pointer to the ADC device
> >> + * @template:	Template iio_chan_spec from which the fields of all found and
> >> + *		allocated channels are initialized.
> >> + * @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(struct device *dev,
> >> +				       const struct iio_chan_spec *template,
> >> +				       struct iio_chan_spec **cs)
> >> +{
> >> +	struct fwnode_handle *fwnode, *child;
> >> +	struct iio_chan_spec *chan;
> >> +	int num_chan = 0, ret;
> >> +
> >> +	fwnode = dev_fwnode(dev);
> >> +	if (!fwnode) {
> >> +		fwnode = dev_fwnode(dev->parent);
> >> +		if (!fwnode)
> >> +			return -ENODEV;
> >> +	}
> >> +
> >> +	num_chan = iio_adc_fwnode_num_channels(fwnode);
> >> +	if (num_chan < 0)
> >> +		return num_chan;
> >> +
> >> +	*cs = devm_kcalloc(dev, num_chan, sizeof(**cs), GFP_KERNEL);
> >> +	if (!*cs)
> >> +		return -ENOMEM;
> >> +
> >> +	chan = &(*cs)[0];
> >> +
> >> +	fwnode_for_each_child_node(fwnode, child) {
> >> +		if (fwnode_name_eq(child, "channel")) {  
> > Flip logic probably and use a continue to reduce indent of
> > the next bit (which may well get a lot more complex in future).
> >   
> 
> Right, thanks.
> 
> >> +			u32 ch;
> >> +
> >> +			ret = fwnode_property_read_u32(child, "reg", &ch);
> >> +			if (ret)
> >> +				return ret;  
> > 
> > In general the association between reg and channel is more complex.
> > We need to deal with a reasonable subset of cases (single-channel, diff-channels
> > etc) where it isn't the case that chan == chan->channel.  
> 
> I guess this is right. I, however, lacked of knowledge on how the 
> differential channels etc. are handled :) Hence I just implemented what 
> I believe is "the most common" approach, while leaving the rest to be 
> implemented by those who need it.
> 
> Still, I agree that a generic looking API which silently produces bad 
> results for a few of the use-cases is not nice. By the very minimum we 
> should check if single-channel, diff-channels properties are present, 
> and then error out with a warning print. That should give a good hint 
> for those driver writers who are trying to use API for something 
> unsupported.
> 
> Also, restrictions should be mentioned in the documentation.
> 
> Do you think we should use some more specific function naming, like 
> "_simple" - suffix?

No to _simple. I think this needs to handle those cases before we
take it at all  They should all have enough docs in adc.yaml to
work out what happens.

The most complex cases are all Analog Devices parts and those
folk are very active on linux-iio so it should be easy to get
them to review a series using it.

I don't think it is going to be particularly hard to generalise
this.

We may end up with a variant that takes a 'per channel' callback
to fill in more data + a private pointer of some type as often
those are putting data in a parallel array of extra info about
the channels.  Let's see what it looks like.

Jonathan

> 
> >   
> >> +
> >> +			*chan = *template;
> >> +			chan->channel = ch;
> >> +
> >> +			if (num_chan > 1)
> >> +				chan->indexed = 1;  
> > 
> > I think set this whatever, probably in the template.
> > I don't want to see the interface change just because a given DT says
> > only one channel is connected.  
> 
> Ah. I didn't think of that! Good point.
> 
> 
> Yours,
>    -- Matti
> 

Re: [PATCH v2 2/5] iio: adc: add helpers for parsing ADC nodes

[David Lechner]

On 2/11/25 1:07 PM, Jonathan Cameron wrote:
> On Tue, 11 Feb 2025 10:52:51 +0200
> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> 
>> Hi Jonathan,
>>
>> Thanks for the review and all the comments!
>>
>> Just a note that I am currently spending some quality time with 
>> rebuilding the floor of my house. Leaking floor drain can cause a bit of 
>> a work... :rolleyes: So, my time with upstream work is a bit limited - 
>> although writing an occasional bug or two can help one to keep the 
>> balance ;)
>>
>> Anyways, my replies and new versions may be slower than usual..
>>
>> On 08/02/2025 18:41, Jonathan Cameron wrote:
>>> On Wed, 5 Feb 2025 15:34:51 +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:
>>>> RFC v1 => v2:
>>>>   - New patch
>>>>
>>>> I think it might be nice to have helpers for fetching also the other
>>>> generic (non vendor specific) ADC properties (as listed in the
>>>> Documentation/devicetree/bindings/iio/adc/adc.yaml) - but as I don't
>>>> have use for those in BD79124 driver (at least not for now), I don't
>>>> imnplement them yet. Anyways, this commit creates a place for such
>>>> helpers.  
>>>
>>> There is often a mix of vendor specific and not in channel nodes.
>>> Hence I'm not sure how widely this will be and it is driver
>>> specific which of the standard things make sense.  
>>
>> I definitely agree. Still, in my experience, no written standard 
>> standardizes use as well as written helpers ;) More we support parsing 
>> the generic helpers by the (add subsystem here)-core, more the driver 
>> writes will use the generic properties (instead of brewing vendor 
>> specific ones).
>>
>>> So before I'd consider a helper like this I'd want to see it alongside
>>> a bunch of users including some of the complex ones so that we know
>>> it generalizes well enough.  It doesn't make sense to introduce
>>> it otherwise - just keep the code in the specific drivers instead.
>>>
>>> It's an interesting idea, but not a trivial one :)  
>>
>> I agree it's not trivial. But I believe adding helpers one-by-one to 
>> cover 'normal' use-cases guide the use of the properties. Those who need 
>> something more exotic can always implement their custom handlers - and 
>> then a reviewer of such handler can ask "why" ;)
> I'd be fine with a series taking on the task of converting handling of
> all the documented properties in adc.yaml
> 
> If we do less than that it may never get wide adoption and we end
> up with a bit of generic looking infrastructure that isn't generic.
> 
Having reviewed quite a few patches recently that make use of these
generic channel properties (and writing one driver myself), I don't
really see how we could make generic functions for these that would
be any simpler than calling the fwnode_property functions directly
other than maybe saving a few arguments. The "normal" operation for
these properties usually involves poking some registers on the chip
(could be immediately or deferred) to configure it. So the only thing
we could generalize is reading the property value, but not doing
anything with that information.

Re: [PATCH v2 2/5] iio: adc: add helpers for parsing ADC nodes

[Matti Vaittinen]

Hi David!

Thanks for the input!

On 16/02/2025 19:50, David Lechner wrote:
> On 2/11/25 1:07 PM, Jonathan Cameron wrote:
>> On Tue, 11 Feb 2025 10:52:51 +0200
>> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
>>
>>> Hi Jonathan,
>>>
>>> Thanks for the review and all the comments!
>>>
>>> Just a note that I am currently spending some quality time with
>>> rebuilding the floor of my house. Leaking floor drain can cause a bit of
>>> a work... :rolleyes: So, my time with upstream work is a bit limited -
>>> although writing an occasional bug or two can help one to keep the
>>> balance ;)
>>>
>>> Anyways, my replies and new versions may be slower than usual..
>>>
>>> On 08/02/2025 18:41, Jonathan Cameron wrote:
>>>> On Wed, 5 Feb 2025 15:34:51 +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:
>>>>> RFC v1 => v2:
>>>>>    - New patch
>>>>>
>>>>> I think it might be nice to have helpers for fetching also the other
>>>>> generic (non vendor specific) ADC properties (as listed in the
>>>>> Documentation/devicetree/bindings/iio/adc/adc.yaml) - but as I don't
>>>>> have use for those in BD79124 driver (at least not for now), I don't
>>>>> imnplement them yet. Anyways, this commit creates a place for such
>>>>> helpers.
>>>>
>>>> There is often a mix of vendor specific and not in channel nodes.
>>>> Hence I'm not sure how widely this will be and it is driver
>>>> specific which of the standard things make sense.
>>>
>>> I definitely agree. Still, in my experience, no written standard
>>> standardizes use as well as written helpers ;) More we support parsing
>>> the generic helpers by the (add subsystem here)-core, more the driver
>>> writes will use the generic properties (instead of brewing vendor
>>> specific ones).
>>>
>>>> So before I'd consider a helper like this I'd want to see it alongside
>>>> a bunch of users including some of the complex ones so that we know
>>>> it generalizes well enough.  It doesn't make sense to introduce
>>>> it otherwise - just keep the code in the specific drivers instead.
>>>>
>>>> It's an interesting idea, but not a trivial one :)
>>>
>>> I agree it's not trivial. But I believe adding helpers one-by-one to
>>> cover 'normal' use-cases guide the use of the properties. Those who need
>>> something more exotic can always implement their custom handlers - and
>>> then a reviewer of such handler can ask "why" ;)
>> I'd be fine with a series taking on the task of converting handling of
>> all the documented properties in adc.yaml
>>
>> If we do less than that it may never get wide adoption and we end
>> up with a bit of generic looking infrastructure that isn't generic.
>>
> Having reviewed quite a few patches recently that make use of these
> generic channel properties (and writing one driver myself), I don't
> really see how we could make generic functions for these that would
> be any simpler than calling the fwnode_property functions directly
> other than maybe saving a few arguments.

I started this for the BD79124 - which is very simple ADC. What it 
requires from the devicetree is the channel ID. What needs to be done in 
oreder to get it?

1. Locate the ADC node (device-node)
2. Loop through the sub-nodes and identify subnodes that denote
    channels.
3. Get the channel number (in this case from the reg-property).

I assume this is very usual need for the simple ADC drivers, and none of 
this needs to be hardware specific (when we have simple input channels 
(no need for the differential channel support).

Providing a simple helper doing just this should remove code duplication 
from drivers. Furthermore, this could help standardize the mechanism for 
identifying the channel nodes - in my patch this would require channel 
node to be named channel@<N>. (I see some drivers which don't require 
this but just assume any sub-node is for channel information. This works 
badly with chips which may provide multiple functions and thus I think 
it'd be nice to guide towards naming the nodes to channel@N when possible).

> The "normal" operation for
> these properties usually involves poking some registers on the chip
> (could be immediately or deferred) to configure it. So the only thing
> we could generalize is reading the property value, but not doing
> anything with that information.

Absolutely. I had no intention to do anything hardware specific with 
these helpers. What I'd love to see is IIO-helpers which allowed passing 
device pointer and a pointer to iio_info - and getting the (known by the 
helper) device-tree properties parsed and filled in iio_info.

This sure does not help more complex drivers which require some 'not 
easily parsed' data from the device tree. But for simple drivers like 
the BD79124 this would allow drivers to omit open-coding the loop 
locating the channel nodes and getting the channel data.

Maybe a small improvement but one can't deny it still is an improvement, 
right? :)

Yours,
  -- Matti

Re: [PATCH v2 2/5] iio: adc: add helpers for parsing ADC nodes

[David Lechner]

On 2/17/25 12:29 AM, Matti Vaittinen wrote:
> Hi David!
> 
> Thanks for the input!
> 

...

> Absolutely. I had no intention to do anything hardware specific with these helpers. What I'd love to see is IIO-helpers which allowed passing device pointer and a pointer to iio_info - and getting the (known by the helper) device-tree properties parsed and filled in iio_info.
> 
> This sure does not help more complex drivers which require some 'not easily parsed' data from the device tree. But for simple drivers like the BD79124 this would allow drivers to omit open-coding the loop locating the channel nodes and getting the channel data.
> 
> Maybe a small improvement but one can't deny it still is an improvement, right? :)
> 
> Yours,
>  -- Matti
> 

If there are a decent number of drivers that can make use of it,
then sure. I'm a bit biased because I've been working on some
rather complex ADCs lately and not any simple ones. :-)

Re: [PATCH v2 2/5] iio: adc: add helpers for parsing ADC nodes

[Matti Vaittinen]

On 11/02/2025 21:07, Jonathan Cameron wrote:
> On Tue, 11 Feb 2025 10:52:51 +0200
> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> 
>> Hi Jonathan,
>>
>> Thanks for the review and all the comments!
>>
>> Just a note that I am currently spending some quality time with
>> rebuilding the floor of my house. Leaking floor drain can cause a bit of
>> a work... :rolleyes: So, my time with upstream work is a bit limited -
>> although writing an occasional bug or two can help one to keep the
>> balance ;)

The good thing when doing manual labour is that one can run thinking 
processes at the background - and has no time to reply instantly. XD So, 
I've been thinking about this quite a bit while installing new floor...

...

>>
>> Right, thanks.
>>
>>>> +			u32 ch;
>>>> +
>>>> +			ret = fwnode_property_read_u32(child, "reg", &ch);
>>>> +			if (ret)
>>>> +				return ret;
>>>
>>> In general the association between reg and channel is more complex.
>>> We need to deal with a reasonable subset of cases (single-channel, diff-channels
>>> etc) where it isn't the case that chan == chan->channel.
>>
>> I guess this is right. I, however, lacked of knowledge on how the
>> differential channels etc. are handled :) Hence I just implemented what
>> I believe is "the most common" approach, while leaving the rest to be
>> implemented by those who need it.
>>
>> Still, I agree that a generic looking API which silently produces bad
>> results for a few of the use-cases is not nice. By the very minimum we
>> should check if single-channel, diff-channels properties are present,
>> and then error out with a warning print. That should give a good hint
>> for those driver writers who are trying to use API for something
>> unsupported.
>>
>> Also, restrictions should be mentioned in the documentation.
>>
>> Do you think we should use some more specific function naming, like
>> "_simple" - suffix?
> 
> No to _simple. I think this needs to handle those cases before we
> take it at all  They should all have enough docs in adc.yaml to
> work out what happens.
> 

I am thinking it would be conceptually better to provide small helper 
(like the *_simpe() ) - which handled only specific properties. We can 
additionally provide '*_differential()' helper and potentially some 
'*_full()' helper which covers both cases.

Rationale is that:
  - writing _simple() helper is simpler. So is understanding it.
  - Devices may not support differential inputs. Drivers for those
   devices would want to call helper which does not accept the
   'differential' channel config. Parsing the differential channel
   information successfully and filling it in iio_chan_spec would be an
   error for those drivers.

> The most complex cases are all Analog Devices parts and those
> folk are very active on linux-iio so it should be easy to get
> them to review a series using it.
> 
> I don't think it is going to be particularly hard to generalise
> this.
> 
> We may end up with a variant that takes a 'per channel' callback
> to fill in more data + a private pointer of some type as often
> those are putting data in a parallel array of extra info about
> the channels.  Let's see what it looks like.

I was playing with this thought for a bit. I do love the regulator 
framework's '.of_parse_cp' which the driver can fill in the regulator 
desc. It's a pointer to function which is called by a regulator core 
after it locates the regulator node so the driver can handle regulator 
specific properties and do relevant configs in the registers.

I some sense the use case is similar - PMICs usually contain multiple 
regulators, each having own node. Being able to 'offload' locating the 
node to the regulator-core is often very handy for drivers.

I thought we could maybe add .fwnode_parse_chan_cp field in the iio_info 
structure. Idea being that it'd be a function which would be called for 
each channel - and it would probably be useful on other areas in IIO 
besides the ADCs.

Problem is that the number of channels may not be known prior parsing 
the device-tree. So, here is kind of a chicken-egg problem with 
populating the iio_chan_spec. Furthermore, locating the channel node in 
device-tree may not be 'standard enough' throughout all the different 
IIO areas.

What comes to a devicetree parsing helper which takes a pointer to a 
device specific configuration function as argument - my initial feeling 
is that it might get a bit overly complex to be user-friendly.

Hence, I'd keep this simple and provided small-but-usefull _simple() ;)

Well, if others aren't convinced by the usefulness of these helpers - 
then I'll probably just squash it into bd79124 driver and be done with 
this :) Later if I work for another ADC variant I might export it as 
'rohm_adc_helper' with an internal to IIO header :)

Thanks for the opinions / discussion!

Yours,
  -- Matti

Re: [PATCH v2 2/5] iio: adc: add helpers for parsing ADC nodes

[Matti Vaittinen]

On 11/02/2025 21:07, Jonathan Cameron wrote:
> On Tue, 11 Feb 2025 10:52:51 +0200
> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> 

...

>>>> +int iio_adc_device_get_channels(struct device *dev, int *channels,
>>>> +				int max_channels)
>>>> +{
>>>> +	struct fwnode_handle *fwnode, *child;
>>>> +	int num_chan = 0, ret;
>>>> +
>>>> +	fwnode = dev_fwnode(dev);
>>>> +	if (!fwnode) {
>>>
>>> As before, I'd relax this until we need to do it. We may never do so.
>>
>> The BD79124 does not require this as I dropped the MFD, so this is
>> ultimately your call :) I, however, would like to humbly suggest adding
>> it now ;) I have changed some APIs in the regulator subsystem and in the
>> clk subsystem to support cases where the device-tree node is in the
>> parent (usual MFD device-case), and it has been somewhat scary... (What
>> if somewhere in some of the existing device-trees the parent happens to
>> have interesting properties - but it actually is not correct node? This
>> becomes a potential source of regression when adding support to an
>> existing API).
>>
>> Furthermore, when the node is unconditionally taken from the given
>> device-pointer, it is tempting for the caller to just pass the parent
>> device as argument...
>>
>>    - If you have done this - please raise your hand. /me raises.
>>    - If you have only later realized it can cause life-time issues when
>>      devm is used - please raise your hand. /me raises.
>>    - If you have tried to kick your own behind when fixing the issues -
>>      please, raise your hand. /me raises. (and if you succeeded - congraz,
>>      you aren't as old and clumsy as I am).
> 
> Maybe. I'd be happier if there was a single user included
> with a patch set doing this.  I'm not sure this applies to
> any of the SoC ADCs which are MFD hosted but maybe it does.
> 

I did a quick "grep powered audit" of the ADC drivers out of the 
curiosity. It seems to me that most of the platform drivers under ADC do 
have the of_match table populated. I suppose they have own node for the 
ADC stuff with ADC specific compatibles, so they're safe from this problem.

(I think we should still also consider cases where the ADC block does 
not have own compatible/node. This sure is just an opinion but I think 
it is kind of artificial to have own node for ADC block when it is just 
a part of a smallish device. Also, having multiple compatibles for one 
device feels "quirky" to me).

The exception to a rule seems to be the 'sun4i-gpadc-iio.c'

And, if I am not misreading the code, the thermal zone registration may 
be causing some problems. It seems to me the data of the thermal zone is 
allocated by the devm_iio_device_alloc() - and bound to the lifetime of 
the platform device.

The thermal zone in MFD branch is bound to the life time of the parent 
(MFD) device.

(in MFD case):
	info->sensor_device = pdev->dev.parent;
...

devm_thermal_of_zone_register(info->sensor_device, ...

I believe that releasing the IIO device will free the thermal zone data 
- but the thermal zone stays there until MFD is released. I haven't 
checked when the thermal zone uses the data though - but I'd be a bit 
wary of this design. Furthermore, removing and re-registering the IIO 
driver may cause some collision with the thermal zone which has stayed 
there. (Again, I didn't check the thermal zone implementation so I'm not 
sure this really is a problem).

In any case, it seems to me most of the current IIO ADC MFD devices have 
dt node "bound" to the platform device and don't use the parent node.

Yours,
   -- Matti

[PATCH v2 3/5] iio: adc: Support ROHM BD79124 ADC

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 36285 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:
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 | 1149 ++++++++++++++++++++++++++++++++
 3 files changed, 1162 insertions(+)
 create mode 100644 drivers/iio/adc/rohm-bd79124.c

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 37b70a65da6f..a2a36a4ec644 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 956c121a7544..43f159aba390 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -108,6 +108,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..ea93762a24cc
--- /dev/null
+++ b/drivers/iio/adc/rohm-bd79124.c
@@ -0,0 +1,1149 @@
+// 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;
+};
+
+/* 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)
+{
+	int adc_channels[BD79124_MAX_NUM_CHANNELS];
+	int ret, num_channels, gpo_chan, j;
+
+	*valid_mask = 0;
+
+	ret = iio_adc_device_get_channels(gc->parent, adc_channels,
+					  BD79124_MAX_NUM_CHANNELS);
+	if (ret < 0)
+		return ret;
+
+	num_channels = ret;
+
+	for (gpo_chan = 0; gpo_chan < BD79124_MAX_NUM_CHANNELS; gpo_chan++) {
+		for (j = 0; j < num_channels; j++) {
+			if (adc_channels[j] == gpo_chan)
+				break;
+		}
+		if (j == num_channels)
+			*valid_mask |= BIT(gpo_chan);
+	}
+
+	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)
+{
+	int reg, limit;
+
+	guard(mutex)(&data->mutex);
+	data->alarm_suppressed[channel] |= BIT(IIO_EV_DIR_RISING);
+
+	reg = BD79124_GET_HIGH_LIMIT_REG(channel);
+	limit = BD79124_HIGH_LIMIT_MAX;
+
+	return __bd79124_event_ratelimit(data, reg, limit);
+}
+
+static int bd79124_event_ratelimit_lo(struct bd79124_data *data,
+				      unsigned int channel)
+{
+	int reg, limit;
+
+	guard(mutex)(&data->mutex);
+	data->alarm_suppressed[channel] |= BIT(IIO_EV_DIR_FALLING);
+
+	reg = BD79124_GET_LOW_LIMIT_REG(channel);
+	limit = BD79124_LOW_LIMIT_MIN;
+
+	return __bd79124_event_ratelimit(data, reg, limit);
+}
+
+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)
+{
+	struct bd79124_reg_init inits[] = {
+		{ .reg = BD79124_GET_HIGH_LIMIT_REG(channel), .val = 4095 },
+		{ .reg = BD79124_GET_LOW_LIMIT_REG(channel), .val = 0 },
+	};
+	int i, ret;
+
+	for (i = 0; i < ARRAY_SIZE(inits); i++) {
+		ret = regmap_write(data->map, inits[i].reg, inits[i].val);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+static bool bd79124_is_in_array(int *arr, int num_items, int val)
+{
+	int i;
+
+	for (i = 0; i < num_items; i++)
+		if (arr[i] == val)
+			return true;
+
+	return false;
+}
+
+static int bd79124_mux_init(struct bd79124_data *data)
+{
+	int adc_channels[BD79124_MAX_NUM_CHANNELS];
+	int num_adc, chan, regval = 0;
+
+	num_adc = iio_adc_device_get_channels(data->dev, &adc_channels[0],
+					      BD79124_MAX_NUM_CHANNELS);
+	if (num_adc < 0)
+		return num_adc;
+
+	/*
+	 * Set a mux register bit for each pin which is free to be used as
+	 * a GPO.
+	 */
+	for (chan = 0; chan < BD79124_MAX_NUM_CHANNELS; chan++)
+		if (!bd79124_is_in_array(&adc_channels[0], num_adc, chan))
+			regval |= BIT(chan);
+
+	return regmap_write(data->map, BD79124_REG_PINCFG, regval);
+}
+
+static int bd79124_hw_init(struct bd79124_data *data)
+{
+	int ret, regval, i;
+
+	ret = bd79124_mux_init(data);
+	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, BD79124_CONV_MODE_MANSEQ);
+
+}
+
+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 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");
+
+	data->vmax = devm_regulator_get_enable_read_voltage(dev, "vdd");
+	if (data->vmax < 0)
+		return dev_err_probe(dev, ret, "Failed to get the Vdd\n");
+
+	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(dev, template, &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;
+
+	data->gc = bd79124gpo_chip;
+	data->gc.parent = dev;
+
+	mutex_init(&data->mutex);
+
+	ret = bd79124_hw_init(data);
+	if (ret)
+		return ret;
+
+	ret = devm_gpiochip_add_data(data->dev, &data->gc, data);
+	if (ret)
+		return dev_err_probe(data->dev, ret, "gpio init Failed\n");
+
+	if (i2c->irq > 0) {
+		ret = devm_request_threaded_irq(data->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");
-- 
2.48.1


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

Re: [PATCH v2 3/5] iio: adc: Support ROHM BD79124 ADC

[kernel test robot]

Hi Matti,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 5bc55a333a2f7316b58edc7573e8e893f7acb532]

url:    https://github.com/intel-lab-lkp/linux/commits/Matti-Vaittinen/dt-bindings-ROHM-BD79124-ADC-GPO/20250205-214127
base:   5bc55a333a2f7316b58edc7573e8e893f7acb532
patch link:    https://lore.kernel.org/r/4781e1b1f074ca6c84ecc084b152885d08e826cc.1738761899.git.mazziesaccount%40gmail.com
patch subject: [PATCH v2 3/5] iio: adc: Support ROHM BD79124 ADC
config: i386-randconfig-004-20250207 (https://download.01.org/0day-ci/archive/20250207/202502070654.5T9Nk6dN-lkp@intel.com/config)
compiler: clang version 19.1.3 (https://github.com/llvm/llvm-project ab51eccf88f5321e7c60591c5546b254b6afab99)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250207/202502070654.5T9Nk6dN-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202502070654.5T9Nk6dN-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/iio/adc/rohm-bd79124.c:1072:29: warning: variable 'ret' is uninitialized when used here [-Wuninitialized]
    1072 |                 return dev_err_probe(dev, ret, "Failed to get the Vdd\n");
         |                                           ^~~
   drivers/iio/adc/rohm-bd79124.c:1057:9: note: initialize the variable 'ret' to silence this warning
    1057 |         int ret;
         |                ^
         |                 = 0
   1 warning generated.


vim +/ret +1072 drivers/iio/adc/rohm-bd79124.c

  1049	
  1050	static int bd79124_probe(struct i2c_client *i2c)
  1051	{
  1052		struct bd79124_data *data;
  1053		struct iio_dev *iio_dev;
  1054		const struct iio_chan_spec *template;
  1055		struct iio_chan_spec *cs;
  1056		struct device *dev = &i2c->dev;
  1057		int ret;
  1058	
  1059		iio_dev = devm_iio_device_alloc(dev, sizeof(*data));
  1060		if (!iio_dev)
  1061			return -ENOMEM;
  1062	
  1063		data = iio_priv(iio_dev);
  1064		data->dev = dev;
  1065		data->map = devm_regmap_init_i2c(i2c, &bd79124_regmap);
  1066		if (IS_ERR(data->map))
  1067			return dev_err_probe(dev, PTR_ERR(data->map),
  1068					     "Failed to initialize Regmap\n");
  1069	
  1070		data->vmax = devm_regulator_get_enable_read_voltage(dev, "vdd");
  1071		if (data->vmax < 0)
> 1072			return dev_err_probe(dev, ret, "Failed to get the Vdd\n");
  1073	
  1074		ret = devm_regulator_get_enable(dev, "iovdd");
  1075		if (ret < 0)
  1076			return dev_err_probe(dev, ret, "Failed to enable I/O voltage\n");
  1077	
  1078		ret = devm_delayed_work_autocancel(dev, &data->alm_enable_work,
  1079						   bd79124_alm_enable_worker);
  1080		if (ret)
  1081			return ret;
  1082	
  1083		if (i2c->irq) {
  1084			template = &bd79124_chan_template;
  1085		} else {
  1086			template = &bd79124_chan_template_noirq;
  1087			dev_dbg(dev, "No IRQ found, events disabled\n");
  1088		}
  1089		ret = devm_iio_adc_device_alloc_chaninfo(dev, template, &cs);
  1090		if (ret < 0)
  1091			return ret;
  1092	
  1093		iio_dev->channels = cs;
  1094		iio_dev->num_channels = ret;
  1095		iio_dev->info = &bd79124_info;
  1096		iio_dev->name = "bd79124";
  1097		iio_dev->modes = INDIO_DIRECT_MODE;
  1098	
  1099		data->gc = bd79124gpo_chip;
  1100		data->gc.parent = dev;
  1101	
  1102		mutex_init(&data->mutex);
  1103	
  1104		ret = bd79124_hw_init(data);
  1105		if (ret)
  1106			return ret;
  1107	
  1108		ret = devm_gpiochip_add_data(data->dev, &data->gc, data);
  1109		if (ret)
  1110			return dev_err_probe(data->dev, ret, "gpio init Failed\n");
  1111	
  1112		if (i2c->irq > 0) {
  1113			ret = devm_request_threaded_irq(data->dev, i2c->irq,
  1114						bd79124_irq_handler,
  1115						&bd79124_event_handler, IRQF_ONESHOT,
  1116						"adc-thresh-alert", iio_dev);
  1117			if (ret)
  1118				return dev_err_probe(data->dev, ret,
  1119						     "Failed to register IRQ\n");
  1120		}
  1121	
  1122		return devm_iio_device_register(data->dev, iio_dev);
  1123	}
  1124	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v2 3/5] iio: adc: Support ROHM BD79124 ADC

[Jonathan Cameron]

On Wed, 5 Feb 2025 15:38:16 +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>

Hi Matti,

Just a few really trivial comments though this wasn't my most thorough
of reviews as ran out of time / energy today!

Jonathan

> diff --git a/drivers/iio/adc/rohm-bd79124.c b/drivers/iio/adc/rohm-bd79124.c
> new file mode 100644
> index 000000000000..ea93762a24cc
> --- /dev/null
> +++ b/drivers/iio/adc/rohm-bd79124.c
> @@ -0,0 +1,1149 @@


> +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;
Odd indent.
> +
> +		return regmap_update_bits(data->map, reg, BD79124_MASK_HYSTERESIS,
> +					  val);
> +	default:
> +		return -EINVAL;
> +	}
> +}


> +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;
This lot is very similar to the lo variant. Can we combine them or
use some helper for both?
> +
> +	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);
> +}

Re: [PATCH v2 3/5] iio: adc: Support ROHM BD79124 ADC

[Matti Vaittinen]

On 08/02/2025 18:52, Jonathan Cameron wrote:
> On Wed, 5 Feb 2025 15:38:16 +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>
> 
> Hi Matti,
> 
> Just a few really trivial comments though this wasn't my most thorough
> of reviews as ran out of time / energy today!


Thanks :) I appreciate the time and energy invested here :)
> 
>> diff --git a/drivers/iio/adc/rohm-bd79124.c b/drivers/iio/adc/rohm-bd79124.c
>> new file mode 100644
>> index 000000000000..ea93762a24cc
>> --- /dev/null
>> +++ b/drivers/iio/adc/rohm-bd79124.c
>> @@ -0,0 +1,1149 @@
> 
> 
>> +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;
> Odd indent.

Oh, indeed. Thanks!

>> +
>> +		return regmap_update_bits(data->map, reg, BD79124_MASK_HYSTERESIS,
>> +					  val);
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +}
> 
> 
>> +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;
> This lot is very similar to the lo variant. Can we combine them or
> use some helper for both?

Initially I did this.

But the code looked a bit dull because the evbitm, alarm-limit array and 
prints depend on the direction. Furthermore, the caller already knows 
the direction (as the caller does also handle directions separately), so 
doing:

foo(dir)
{
	if (dir == bar)
		...
	else
		...
}

...

if (dir == bar)
	foo(dir);
else
	foo(dir);

started to feel just a bit, meh. Hence I separated the stuff to own _lo 
and _hi functions.


>> +
>> +	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);
>> +}
> 

Yours,
   -- Matti

Re: [PATCH v2 3/5] iio: adc: Support ROHM BD79124 ADC

[Jonathan Cameron]

> >> +
> >> +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;  
> > This lot is very similar to the lo variant. Can we combine them or
> > use some helper for both?  
> 
> Initially I did this.
> 
> But the code looked a bit dull because the evbitm, alarm-limit array and 
> prints depend on the direction. Furthermore, the caller already knows 
> the direction (as the caller does also handle directions separately), so 
> doing:
> 
> foo(dir)
> {
> 	if (dir == bar)
> 		...
> 	else
> 		...
> }
> 
> ...
> 
> if (dir == bar)
> 	foo(dir);
> else
> 	foo(dir);
> 
> started to feel just a bit, meh. Hence I separated the stuff to own _lo 
> and _hi functions.
Hmm. I was thinking of something like

static void bd79124_re_enable_xx(struct bd79124_data *data, unsigned int channel,
				 unsigned int reg, u16 limit,
				 enum iio_event_dir dir)
{
	int ret, evbit = BIT(dir);

	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, reg, limit);
	if (ret)
		dev_warn(data->dev, "Low limit enabling failed for channel%d\n",
			 channel);
}

static void bd71924_reenable_hi(struct bd79124_data *data, unsigned int channel)
{
	return bd71924_reenable_x(data, channel, BD79124_GET_HIGH_LIMIT_REG(channel)
				  data->alarm_r_limit[channel], IIO_EV_DIR_RISING);
}

static void bd71924_reenable_lo(struct bd79124_data *data, unsigned int channel)
{
	return bd71924_reenable_x(data, channel, BD79124_GET_LOW_LIMIT_REG(channel)
				  data->alarm_f_limit[channel], IIO_EV_DIR_FALLING);
}

But I guess not really a saving in the end.

> 
> 
> >> +
> >> +	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);
> >> +}  
> >   
> 
> Yours,
>    -- Matti
> 
> 

[PATCH v2 4/5] 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 a87ddad78e26..bfe2f53fa74d 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11099,6 +11099,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 v2 5/5] 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 bfe2f53fa74d..2021327e665e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -20299,6 +20299,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 --]