Support ROHM BD79124 ADC/GPO

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

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

Documentation/devicetree/bindings/iio/adc/rohm,bd79124.yaml new file +38 -0

new file mode 100644
index 000000000000..50889dc6b9a8
@@ -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 <[email protected]>
+
+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
+

Documentation/devicetree/bindings/iio/adc/rohm,bd79124.yaml new file +66 -0

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

Signature · 1 line

2.48.1

Quoted 12 lines · Matti Vaittinen

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

---
Revision history:
RFC v1 => v2:
- drop MFD and represent directly as ADC
- drop pinmux and treat all non ADC channel pins as GPOs
---

Documentation/devicetree/bindings/iio/adc/rohm,bd79124.yaml line 48

Quoted 2 lines

Documentation/devicetree/bindings/iio/adc/rohm,bd79124.yaml new file +2 -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.

Quoted 13 lines · Conor Dooley

Quoted 12 lines · Matti Vaittinen

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

---
Revision history:
RFC v1 => v2:
- drop MFD and represent directly as ADC
- drop pinmux and treat all non ADC channel pins as GPOs
---

...

Quoted 6 lines

Documentation/devicetree/bindings/iio/adc/rohm,bd79124.yaml line 48

Quoted 2 lines

Documentation/devicetree/bindings/iio/adc/rohm,bd79124.yaml new file +2 -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.

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

There are ADC ICs which may have some of the AIN pins usable for other functions. These ICs may have some of the AIN pins wired so that they should not be used for ADC.

(Preferred?) way for marking pins which can be used as ADC inputs is to add corresponding channels@N nodes in the device tree as described in the ADC binding yaml.

Add couple of helper functions which can be used to retrieve the channel information from the device node.

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

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

drivers/iio/adc/Kconfig +3 -0

index 849c90203071..37b70a65da6f 100644
@@ -6,6 +6,9 @@
 
 menu "Analog to digital converters"
 
+config IIO_ADC_HELPER
+	tristate
+
 config AB8500_GPADC
 	bool "ST-Ericsson AB8500 GPADC driver"
 	depends on AB8500_CORE && REGULATOR_AB8500

drivers/iio/adc/Makefile +1 -0

index ee19afba62b7..956c121a7544 100644
@@ -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

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

new file mode 100644
index 000000000000..366e4c8eb6c7

+ *
+ * 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.
+ */

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

+			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

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

+		}
+	}
+
+	return num_chan;
+
+}

+ *
+ * @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.
+ */

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

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

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

+
+			chan++;
+		}
+	}
+
+	return num_chan;
+}
+EXPORT_SYMBOL_GPL(devm_iio_adc_device_alloc_chaninfo);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Matti Vaittinen <[email protected]>");
+MODULE_DESCRIPTION("IIO ADC fwnode parsing helpers");

Quoted 18 lines · On Wed, 5 Feb 2025 15:34:51 +0200 Matti Vaittinen <[email protected]>…

There are ADC ICs which may have some of the AIN pins usable for other functions. These ICs may have some of the AIN pins wired so that they should not be used for ADC.

(Preferred?) way for marking pins which can be used as ADC inputs is to add corresponding channels@N nodes in the device tree as described in the ADC binding yaml.

Add couple of helper functions which can be used to retrieve the channel information from the device node.

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

---
Revision history:
RFC v1 => v2:
- New patch

Quoted 6 lines

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

Quoted 41 lines

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

drivers/iio/adc/Kconfig +3 -0

index 849c90203071..37b70a65da6f 100644
@@ -6,6 +6,9 @@
 
  menu "Analog to digital converters"
 
+config IIO_ADC_HELPER
+	tristate
+
  config AB8500_GPADC
  	bool "ST-Ericsson AB8500 GPADC driver"
  	depends on AB8500_CORE && REGULATOR_AB8500

drivers/iio/adc/Makefile +1 -0

index ee19afba62b7..956c121a7544 100644
@@ -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

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

new file mode 100644
index 000000000000..366e4c8eb6c7
@@ -0,0 +1,151 @@

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

Quoted 3 lines

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

+
+/**
+ * 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.

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

Quoted 4 lines

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

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

Quoted 12 lines

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

+ * 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)

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

Quoted 6 lines

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

+{
+	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.

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

Quoted 7 lines

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

+		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

Quoted 12 lines

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

+			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

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

Quoted 10 lines

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

+			 * 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.

Quoted 14 lines

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

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

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

Quoted 3 lines

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

+ * 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.

Quoted 32 lines

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

+ *
+ * @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)

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

Quoted 6 lines

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

+		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).

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

Quoted 5 lines

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

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

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

Quoted 7 lines

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

+
+			*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.

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

Quoted 28 lines · Jonathan Cameron

Quoted 18 lines · On Wed, 5 Feb 2025 15:34:51 +0200 Matti Vaittinen <[email protected]>…

There are ADC ICs which may have some of the AIN pins usable for other functions. These ICs may have some of the AIN pins wired so that they should not be used for ADC.

(Preferred?) way for marking pins which can be used as ADC inputs is to add corresponding channels@N nodes in the device tree as described in the ADC binding yaml.

Add couple of helper functions which can be used to retrieve the channel information from the device node.

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

---
Revision history:
RFC v1 => v2:
- New patch

Quoted 6 lines

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.

Quoted 3 lines

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

Quoted 5 lines

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.

Quoted 1 line

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" ;)

Quoted 45 lines

Quoted 43 lines

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

drivers/iio/adc/Kconfig +3 -0

index 849c90203071..37b70a65da6f 100644
@@ -6,6 +6,9 @@
 
  menu "Analog to digital converters"
 
+config IIO_ADC_HELPER
+	tristate
+
  config AB8500_GPADC
  	bool "ST-Ericsson AB8500 GPADC driver"
  	depends on AB8500_CORE && REGULATOR_AB8500

drivers/iio/adc/Makefile +1 -0

index ee19afba62b7..956c121a7544 100644
@@ -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

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

new file mode 100644
index 000000000000..366e4c8eb6c7
@@ -0,0 +1,151 @@  

Quoted 6 lines

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

Quoted 3 lines

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

+
+/**
+ * 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.

Quoted 7 lines

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

Quoted 5 lines

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

+ *
+ * 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!

Quoted 12 lines

Quoted 12 lines

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

+ * 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)

Quoted 8 lines

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

Quoted 6 lines

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

+{
+	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).

Quoted 10 lines

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

Quoted 8 lines

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

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

Quoted 24 lines

Quoted 12 lines

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

+			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

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

Quoted 10 lines

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

+			 * 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;

Quoted 1 line

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.

Quoted 15 lines

Quoted 15 lines

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

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

Quoted 8 lines

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

Quoted 3 lines

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

+ * 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.

Quoted 32 lines

Quoted 32 lines

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

+ *
+ * @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)

Quoted 9 lines

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

Quoted 6 lines

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

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

Quoted 9 lines

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

Quoted 5 lines

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

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

Quoted 12 lines

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

Quoted 8 lines

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

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

Quoted 56 lines · On Tue, 11 Feb 2025 10:52:51 +0200 Matti Vaittinen <[email protected]>…

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

Quoted 28 lines · Jonathan Cameron

Quoted 18 lines · On Wed, 5 Feb 2025 15:34:51 +0200 Matti Vaittinen <[email protected]>…

There are ADC ICs which may have some of the AIN pins usable for other functions. These ICs may have some of the AIN pins wired so that they should not be used for ADC.

(Preferred?) way for marking pins which can be used as ADC inputs is to add corresponding channels@N nodes in the device tree as described in the ADC binding yaml.

Add couple of helper functions which can be used to retrieve the channel information from the device node.

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

---
Revision history:
RFC v1 => v2:
- New patch

Quoted 6 lines

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.

Quoted 3 lines

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

Quoted 5 lines

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.

Quoted 6 lines

Quoted 1 line

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.

Quoted 68 lines · Jonathan Cameron

Quoted 56 lines · On Tue, 11 Feb 2025 10:52:51 +0200 Matti Vaittinen <[email protected]>…

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

Quoted 28 lines · Jonathan Cameron

Quoted 18 lines · On Wed, 5 Feb 2025 15:34:51 +0200 Matti Vaittinen <[email protected]>…

There are ADC ICs which may have some of the AIN pins usable for other functions. These ICs may have some of the AIN pins wired so that they should not be used for ADC.

(Preferred?) way for marking pins which can be used as ADC inputs is to add corresponding channels@N nodes in the device tree as described in the ADC binding yaml.

Add couple of helper functions which can be used to retrieve the channel information from the device node.

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

---
Revision history:
RFC v1 => v2:
- New patch

Quoted 6 lines

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.

Quoted 3 lines

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

Quoted 5 lines

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.

Quoted 6 lines

Quoted 1 line

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

Quoted 3 lines

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.

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

Quoted 2 lines · On Tue, 11 Feb 2025 10:52:51 +0200 Matti Vaittinen <[email protected]>…

Hi Jonathan,

Quoted 7 lines

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

Quoted 25 lines

Right, thanks.

Quoted 9 lines

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

Quoted 5 lines

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

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

Quoted 4 lines

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.

On Tue, 11 Feb 2025 10:52:51 +0200
Matti Vaittinen <[email protected]> wrote:

Quoted 25 lines

Quoted 10 lines

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

Quoted 8 lines

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

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

Quoted 6 lines

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

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

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

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

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

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

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

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

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

new file mode 100644
index 000000000000..ea93762a24cc
@@ -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.

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

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

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

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

Quoted 25 lines · On Wed, 5 Feb 2025 15:38:16 +0200 Matti Vaittinen <[email protected]>…

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

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

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

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

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

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.

Quoted 1 line

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

Hi Matti,

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

Quoted 44 lines

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

new file mode 100644
index 000000000000..ea93762a24cc
@@ -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))

drivers/iio/adc/rohm-bd79124.c line 633

Quoted 7 lines

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

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

Quoted 33 lines

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

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

drivers/iio/adc/rohm-bd79124.c line 766

Quoted 6 lines

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

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

drivers/iio/adc/rohm-bd79124.c line 792

Quoted 7 lines

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

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

Quoted 15 lines

drivers/iio/adc/rohm-bd79124.c line 785

Quoted 13 lines

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

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

Quoted 10 lines

drivers/iio/adc/rohm-bd79124.c line 792

Quoted 9 lines

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

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