[PATCH v3 0/9] Support ROHM BD79124 ADC

[PATCH v3 0/9] Support ROHM BD79124 ADC

[Matti Vaittinen]

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

Support ROHM BD79124 ADC.

This series adds also couple of helper functions for parsing the channel
information from the device tree. There has been some discussion about
how useful these are, and whether they should support also differential
and single ended channel configurations. This version adds support for
those - with the cost of added complexity and somewhat harder to use
API. I've babbled more about that in the patch 2/9. (And, I actually
wonder if I should've returned this to RFC?)

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

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

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

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

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

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

Revision history:
v2 => v3:
 - Restrict BD79124 channel numbers as suggested by Conor and add
   Conor's Reviewed-by tag.
 - Support differential and single-ended inputs
 - Convert couple of existing drivers to use the added ADC helpers
 - Minor fixes based on reviews
Link to v2:
https://lore.kernel.org/all/cover.1738761899.git.mazziesaccount@gmail.com/

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

---

Matti Vaittinen (9):
  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
  iio: adc: rzg2l_adc: Use adc-helpers
  iio: adc: sun20i-gpadc: Use adc-helpers
  iio: adc: ti-ads7924 Drop unnecessary function parameters
  iio: adc: ti-ads7924: Respect device tree config

 .../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            |  304 +++++
 drivers/iio/adc/rohm-bd79124.c                | 1162 +++++++++++++++++
 drivers/iio/adc/rzg2l_adc.c                   |   41 +-
 drivers/iio/adc/sun20i-gpadc-iio.c            |   42 +-
 drivers/iio/adc/ti-ads7924.c                  |   85 +-
 include/linux/iio/adc-helpers.h               |   56 +
 10 files changed, 1742 insertions(+), 91 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/rohm,bd79124.yaml
 create mode 100644 drivers/iio/adc/industrialio-adc.c
 create mode 100644 drivers/iio/adc/rohm-bd79124.c
 create mode 100644 include/linux/iio/adc-helpers.h


base-commit: 5bc55a333a2f7316b58edc7573e8e893f7acb532
-- 
2.48.1


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

[PATCH v3 1/9] dt-bindings: ROHM BD79124 ADC/GPO

[Matti Vaittinen]

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

Add binding document for the ROHM BD79124 ADC / GPO.

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

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

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


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

[PATCH v3 2/9] iio: adc: add helpers for parsing ADC nodes

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 15775 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:
v2 => v3: Mostly based on review comments by Jonathan
 - Support differential and single-ended channels(*)
 - Rename iio_adc_device_get_channels() as
 - Improve spelling
 - Drop support for cases where DT comes from parent device's node
 - Decrease loop indent by reverting node name check conditions
 - Don't set 'chan->indexed' by number of channels to keep the
   interface consistent no matter how many channels are connected.
 - Fix ID range check and related comment
RFC v1 => v2:
 - New patch

(*) The support for single-ended and differential channels is 100%
untested. I am not convinced it is actually an improvement over the
*_simple() helpers which only supported getting the ID from the "reg".
In theory they could be used. In practice, while I skimmed through the
in-tree ADC drivers which used the for_each_child_node() construct - it
seemed that most of those which supported differential inputs had also
some other per-channel properties to read. Those users would in any case
need to loop through the nodes to get those other properties.

If I am once more allowed to go back to proposing the _simple() variant
which only covers the case: "chan ID in 'reg' property"... Dropping
support for differential and single-ended channels would simplify this
helper a lot. It'd allow dropping the sanity check as well as the extra
parameters callers need to pass to tell what kind of properties they
expect. That'd (in my opinion) made the last patches (to actual ADC
drivers) in this series a much more lean and worthy ;)

Finally, we could add own functions for differential/single-ended/all
channels when the next driver which uses differential or single-ended
channels - and which does not need other per-channel properties - lands
in tree. That would still simplify the helper API usage for those
drivers touched at the end of this series.

I (still) 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 | 304 +++++++++++++++++++++++++++++
 include/linux/iio/adc-helpers.h    |  56 ++++++
 4 files changed, 364 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..0281d64ae112
--- /dev/null
+++ b/drivers/iio/adc/industrialio-adc.c
@@ -0,0 +1,304 @@
+// 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_device_num_channels(struct device *dev)
+{
+	int num_chan = 0;
+
+	device_for_each_child_node_scoped(dev, child)
+		if (fwnode_name_eq(child, "channel"))
+			num_chan++;
+
+	return num_chan;
+}
+EXPORT_SYMBOL_GPL(iio_adc_device_num_channels);
+
+static const char *iio_adc_type2prop(int type)
+{
+	switch (type) {
+	case IIO_ADC_CHAN_PROP_TYPE_REG:
+		return "reg";
+	case IIO_ADC_CHAN_PROP_TYPE_SINGLE_ENDED:
+		return "single-channel";
+	case IIO_ADC_CHAN_PROP_TYPE_DIFF:
+		return "diff-channels";
+	case IIO_ADC_CHAN_PROP_COMMON:
+		return "common-mode-channel";
+	default:
+		return "unknown";
+	}
+}
+
+/*
+ * Sanity check. Ensure that:
+ * - At least some type(s) are allowed
+ * - All types found are also expected
+ * - If plain "reg" is not allowed, either single-ended or differential
+ *   properties are found.
+ */
+static int iio_adc_prop_type_check_sanity(struct device *dev,
+		const struct iio_adc_props *expected_props, int found_types)
+{
+	unsigned long allowed_types = expected_props->allowed |
+				      expected_props->required;
+
+	if (!allowed_types || allowed_types & (~IIO_ADC_CHAN_PROP_TYPE_ALL)) {
+		dev_dbg(dev, "Invalid adc allowed prop types 0x%lx\n",
+			allowed_types);
+
+		return -EINVAL;
+	}
+	if (found_types & (~allowed_types)) {
+		long unknown_types = found_types & (~allowed_types);
+		int type;
+
+		for_each_set_bit(type, &unknown_types,
+				 IIO_ADC_CHAN_NUM_PROP_TYPES - 1) {
+			dev_err(dev, "Unsupported channel property %s\n",
+				iio_adc_type2prop(type));
+		}
+
+		return -EINVAL;
+	}
+
+	/*
+	 * The IIO_ADC_CHAN_PROP_TYPE_REG is special. We always require it to
+	 * be found in the dt. (If not, we'll error out before calling this
+	 * function.) However, listing it in 'allowed' types means the "reg"
+	 * alone can be used to indicate the channel ID.
+	 *
+	 * Thus, we don't add it in the found properties either - so check for
+	 * found and allowed properties passes even if user hasn't explicitly
+	 * added the 'IIO_ADC_CHAN_PROP_TYPE_REG' to be allowed. (This is the
+	 * case if either differential or single-ended property is required).
+	 *
+	 * Hence, for this check we need to explicitly add the
+	 * IIO_ADC_CHAN_PROP_TYPE_REG to 'found' properties to make the check
+	 * pass when "reg" is the property which is required to have the
+	 * channel ID.
+	 *
+	 * We could of course always add the IIO_ADC_CHAN_PROP_TYPE_REG in
+	 * allowed types and found types - but then we wouldn't catch the case
+	 * where user says the "reg" alone is not sufficient.
+	 */
+	if ((~(found_types | IIO_ADC_CHAN_PROP_TYPE_REG)) & expected_props->required) {
+		long missing_types;
+		int type;
+
+		missing_types = (~found_types) & expected_props->required;
+
+		for_each_set_bit(type, &missing_types,
+				 IIO_ADC_CHAN_NUM_PROP_TYPES - 1) {
+			dev_err(dev, "required channel specifier '%s' not found\n",
+				iio_adc_type2prop(type));
+		}
+
+		return -EINVAL;
+	}
+
+	/* Check if we require something else but the "reg" property */
+	if (!(allowed_types & IIO_ADC_CHAN_PROP_TYPE_REG)) {
+		if (found_types & IIO_ADC_CHAN_PROP_TYPE_SINGLE_ENDED ||
+				found_types & IIO_ADC_CHAN_PROP_TYPE_DIFF)
+			return 0;
+
+		dev_err(dev, "channel specifier not found\n");
+
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+/**
+ * iio_adc_device_channels_by_property - get ADC channel IDs
+ *
+ * Scan the device node for ADC channel information. Return an array of found
+ * IDs. Caller needs to provide 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.
+ * @expected_props:	Bitmaps of channel property types (for checking).
+ *
+ * Return:		Number of found channels on succes. 0 if no channels
+ *			was found. Negative value to indicate failure.
+ */
+int iio_adc_device_channels_by_property(struct device *dev, int *channels,
+		int max_channels, const struct iio_adc_props *expected_props)
+{
+	int num_chan = 0, ret;
+
+	device_for_each_child_node_scoped(dev, child) {
+		u32 ch, diff[2], se;
+		struct iio_adc_props tmp;
+		int chtypes_found = 0;
+
+		if (!fwnode_name_eq(child, "channel"))
+			continue;
+
+		if (num_chan == max_channels)
+			return -EINVAL;
+
+		ret = fwnode_property_read_u32(child, "reg", &ch);
+		if (ret)
+			return ret;
+
+		ret = fwnode_property_read_u32_array(child, "diff-channels",
+						     &diff[0], 2);
+		if (!ret)
+			chtypes_found |= IIO_ADC_CHAN_PROP_TYPE_DIFF;
+
+		ret = fwnode_property_read_u32(child, "single-channel", &se);
+		if (!ret)
+			chtypes_found |= IIO_ADC_CHAN_PROP_TYPE_SINGLE_ENDED;
+
+		tmp = *expected_props;
+		/*
+		 * We don't bother reading the "common-mode-channel" here as it
+		 * doesn't really affect on the primary channel ID. We remove
+		 * it from the required properties to allow the sanity check
+		 * pass here  also for drivers which require it.
+		 */
+		tmp.required &= (~BIT(IIO_ADC_CHAN_PROP_COMMON));
+
+		ret = iio_adc_prop_type_check_sanity(dev, &tmp, chtypes_found);
+		if (ret)
+			return ret;
+
+		if (chtypes_found & IIO_ADC_CHAN_PROP_TYPE_DIFF)
+			ch = diff[0];
+		else if (chtypes_found & IIO_ADC_CHAN_PROP_TYPE_SINGLE_ENDED)
+			ch = se;
+
+		/*
+		 * 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_channels_by_property);
+
+/**
+ * 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.
+ * @expected_props:	Bitmaps of channel property types (for checking).
+ *
+ * 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,
+				const struct iio_adc_props *expected_props)
+{
+	struct iio_chan_spec *chan;
+	int num_chan = 0, ret;
+
+	num_chan = iio_adc_device_num_channels(dev);
+	if (num_chan < 1)
+		return num_chan;
+
+	*cs = devm_kcalloc(dev, num_chan, sizeof(**cs), GFP_KERNEL);
+	if (!*cs)
+		return -ENOMEM;
+
+	chan = &(*cs)[0];
+
+	device_for_each_child_node_scoped(dev, child) {
+		u32 ch, diff[2], se, common;
+		int chtypes_found = 0;
+
+		if (!fwnode_name_eq(child, "channel"))
+			continue;
+
+		ret = fwnode_property_read_u32(child, "reg", &ch);
+		if (ret)
+			return ret;
+
+		ret = fwnode_property_read_u32_array(child, "diff-channels",
+						     &diff[0], 2);
+		if (!ret)
+			chtypes_found |= IIO_ADC_CHAN_PROP_TYPE_DIFF;
+
+		ret = fwnode_property_read_u32(child, "single-channel", &se);
+		if (!ret)
+			chtypes_found |= IIO_ADC_CHAN_PROP_TYPE_SINGLE_ENDED;
+
+		ret = fwnode_property_read_u32(child, "common-mode-channel",
+					       &common);
+		if (!ret)
+			chtypes_found |= BIT(IIO_ADC_CHAN_PROP_COMMON);
+
+		ret = iio_adc_prop_type_check_sanity(dev, expected_props,
+						     chtypes_found);
+		if (ret)
+			return ret;
+
+		*chan = *template;
+		chan->channel = ch;
+
+		if (chtypes_found & IIO_ADC_CHAN_PROP_TYPE_DIFF) {
+			chan->differential = 1;
+			chan->channel = diff[0];
+			chan->channel2 = diff[1];
+
+		} else if (chtypes_found & IIO_ADC_CHAN_PROP_TYPE_SINGLE_ENDED) {
+			chan->channel = se;
+			if (chtypes_found & BIT(IIO_ADC_CHAN_PROP_COMMON))
+				chan->channel2 = common;
+		}
+
+		/*
+		 * We assume the channel IDs start from 0. If it seems this is
+		 * not a sane assumption, then we have to add 'allowed ID ranges'
+		 * to the struct iio_adc_props because some of the callers may
+		 * rely on the IDs being in this range - and have arrays indexed
+		 * by the ID.
+		 */
+		if (chan->channel >= num_chan)
+			return -ERANGE;
+
+		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..f7791d45dbd2
--- /dev/null
+++ b/include/linux/iio/adc-helpers.h
@@ -0,0 +1,56 @@
+/* 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;
+
+enum {
+	IIO_ADC_CHAN_PROP_REG,
+	IIO_ADC_CHAN_PROP_SINGLE_ENDED,
+	IIO_ADC_CHAN_PROP_DIFF,
+	IIO_ADC_CHAN_PROP_COMMON,
+	IIO_ADC_CHAN_NUM_PROP_TYPES
+};
+
+/*
+ * Channel property types to be used with iio_adc_device_get_channels,
+ * devm_iio_adc_device_alloc_chaninfo, ...
+ */
+#define IIO_ADC_CHAN_PROP_TYPE_REG BIT(IIO_ADC_CHAN_PROP_REG)
+#define IIO_ADC_CHAN_PROP_TYPE_SINGLE_ENDED BIT(IIO_ADC_CHAN_PROP_SINGLE_ENDED)
+#define IIO_ADC_CHAN_PROP_TYPE_SINGLE_COMMON					\
+	(BIT(IIO_ADC_CHAN_PROP_SINGLE_ENDED) | BIT(IIO_ADC_CHAN_PROP_COMMON))
+#define IIO_ADC_CHAN_PROP_TYPE_DIFF BIT(IIO_ADC_CHAN_PROP_DIFF)
+#define IIO_ADC_CHAN_PROP_TYPE_ALL GENMASK(IIO_ADC_CHAN_NUM_PROP_TYPES - 1, 0)
+
+/**
+ * iio_adc_chan_props - information of expected device-tree channel properties
+ *
+ * @required:	Bitmask of property definitions of required channel properties
+ * @allowed:	Bitmask of property definitions of optional channel properties.
+ *		Listing of required properties is not needed here.
+ */
+struct iio_adc_props {
+	unsigned long required;
+	unsigned long allowed;
+};
+
+int iio_adc_device_num_channels(struct device *dev);
+int devm_iio_adc_device_alloc_chaninfo(struct device *dev,
+				const struct iio_chan_spec *template,
+				struct iio_chan_spec **cs,
+				const struct iio_adc_props *expected_props);
+
+int iio_adc_device_channels_by_property(struct device *dev, int *channels,
+				int max_channels,
+				const struct iio_adc_props *expected_props);
+#endif /* _INDUSTRIAL_IO_ADC_HELPERS_H_ */
-- 
2.48.1


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

[PATCH v3 3/9] iio: adc: Support ROHM BD79124 ADC

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 36782 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:
v2 => v3:
 - Fix uninitialized return value reported by the kernel test robot
 - Fix indent
 - Adapt to adc-helper changes supporting also single-ended and
   differential channels
RFC v1 => v2:
 - Add event throttling (constant delay of 1 sec)
 - rename variable 'd' to 'data'
 - Use ADC helpers to detect pins used for ADC
 - bd79124 drop MFD and pinmux && handle GPO in this driver
 - Drop adc suffix from the IIO file name
---
 drivers/iio/adc/Kconfig        |   12 +
 drivers/iio/adc/Makefile       |    1 +
 drivers/iio/adc/rohm-bd79124.c | 1162 ++++++++++++++++++++++++++++++++
 3 files changed, 1175 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..5e23bc8d9ce2
--- /dev/null
+++ b/drivers/iio/adc/rohm-bd79124.c
@@ -0,0 +1,1162 @@
+// 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
+
+/*
+ * We require the "reg" property and allow no other
+ * channel specifiers for the BD79124.
+ */
+static const struct iio_adc_props expected_props = {
+	.required = IIO_ADC_CHAN_PROP_TYPE_REG,
+};
+
+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_channels_by_property(gc->parent, adc_channels,
+						  BD79124_MAX_NUM_CHANNELS,
+						  &expected_props);
+	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_chans[BD79124_MAX_NUM_CHANNELS];
+	int num_adc, chan, regval = 0;
+
+	num_adc = iio_adc_device_channels_by_property(data->dev, &adc_chans[0],
+						      BD79124_MAX_NUM_CHANNELS,
+						      &expected_props);
+	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_chans[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");
+
+	ret = devm_regulator_get_enable_read_voltage(dev, "vdd");
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "Failed to get the Vdd\n");
+
+	data->vmax = ret;
+
+	ret = devm_regulator_get_enable(dev, "iovdd");
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "Failed to enable I/O voltage\n");
+
+	ret = devm_delayed_work_autocancel(dev, &data->alm_enable_work,
+					   bd79124_alm_enable_worker);
+	if (ret)
+		return ret;
+
+	if (i2c->irq) {
+		template = &bd79124_chan_template;
+	} else {
+		template = &bd79124_chan_template_noirq;
+		dev_dbg(dev, "No IRQ found, events disabled\n");
+	}
+	ret = devm_iio_adc_device_alloc_chaninfo(dev, template, &cs,
+						 &expected_props);
+	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 --]

[PATCH v3 4/9] 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 v3 5/9] 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 --]

[PATCH v3 6/9] iio: adc: rzg2l_adc: Use adc-helpers

[Matti Vaittinen]

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

The new devm_iio_adc_device_alloc_chaninfo() -helper is intended to help
drivers avoid open-coding the for_each_node -loop for getting the
channel IDs. The helper provides standard way to detect the ADC channel
nodes (by the node name), and a standard way to convert the "reg",
"diff-channels", "single-channel" and the "common-mode-channel" to
channel identification numbers used in the struct iio_chan_spec.
Furthermore, the helper checks the ID is in range of 0 ... num-channels.

The original driver treated all found child nodes as channel nodes. The
new helper requires channel nodes to be named channel[@N]. This should
help avoid problems with devices which may contain also other but ADC
child nodes. Quick grep from arch/* with the rzg2l_adc's compatible
string didn't reveal any in-tree .dts with channel nodes named
othervice. Also, same grep shows all the .dts seem to have channel IDs
between 0..num of channels.

Use the new helper.

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

---
Revision history:
v2 => v3:
 - New patch

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

The change is compile tested only!! Testing before applying is highly
appreciated (as always!).
---
 drivers/iio/adc/rzg2l_adc.c | 41 ++++++++++++++++++-------------------
 1 file changed, 20 insertions(+), 21 deletions(-)

diff --git a/drivers/iio/adc/rzg2l_adc.c b/drivers/iio/adc/rzg2l_adc.c
index cd3a7e46ea53..3e1c74019785 100644
--- a/drivers/iio/adc/rzg2l_adc.c
+++ b/drivers/iio/adc/rzg2l_adc.c
@@ -11,6 +11,7 @@
 #include <linux/clk.h>
 #include <linux/completion.h>
 #include <linux/delay.h>
+#include <linux/iio/adc-helpers.h>
 #include <linux/iio/iio.h>
 #include <linux/interrupt.h>
 #include <linux/io.h>
@@ -299,20 +300,34 @@ static irqreturn_t rzg2l_adc_isr(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
+static const struct iio_chan_spec rzg2l_adc_chan_template = {
+	.type = IIO_VOLTAGE,
+	.indexed = 1,
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+};
+
+static const struct iio_adc_props rzg2l_adc_chan_props = {
+	.required = IIO_ADC_CHAN_PROP_TYPE_REG,
+};
+
 static int rzg2l_adc_parse_properties(struct platform_device *pdev, struct rzg2l_adc *adc)
 {
 	struct iio_chan_spec *chan_array;
 	struct rzg2l_adc_data *data;
-	unsigned int channel;
 	int num_channels;
-	int ret;
 	u8 i;
 
 	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
 	if (!data)
 		return -ENOMEM;
 
-	num_channels = device_get_child_node_count(&pdev->dev);
+	num_channels = devm_iio_adc_device_alloc_chaninfo(&pdev->dev,
+					&rzg2l_adc_chan_template, &chan_array,
+					&rzg2l_adc_chan_props);
+
+	if (num_channels < 0)
+		return num_channels;
+
 	if (!num_channels) {
 		dev_err(&pdev->dev, "no channel children\n");
 		return -ENODEV;
@@ -323,26 +338,10 @@ static int rzg2l_adc_parse_properties(struct platform_device *pdev, struct rzg2l
 		return -EINVAL;
 	}
 
-	chan_array = devm_kcalloc(&pdev->dev, num_channels, sizeof(*chan_array),
-				  GFP_KERNEL);
-	if (!chan_array)
-		return -ENOMEM;
-
-	i = 0;
-	device_for_each_child_node_scoped(&pdev->dev, fwnode) {
-		ret = fwnode_property_read_u32(fwnode, "reg", &channel);
-		if (ret)
-			return ret;
-
-		if (channel >= RZG2L_ADC_MAX_CHANNELS)
-			return -EINVAL;
+	for (i = 0; i < num_channels; i++) {
+		int channel = chan_array[i].channel;
 
-		chan_array[i].type = IIO_VOLTAGE;
-		chan_array[i].indexed = 1;
-		chan_array[i].channel = channel;
-		chan_array[i].info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
 		chan_array[i].datasheet_name = rzg2l_adc_channel_name[channel];
-		i++;
 	}
 
 	data->num_channels = num_channels;
-- 
2.48.1


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

[PATCH v3 7/9] iio: adc: sun20i-gpadc: Use adc-helpers

[Matti Vaittinen]

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

The new devm_iio_adc_device_alloc_chaninfo() -helper is intended to help
drivers avoid open-coding the for_each_node -loop for getting the
channel IDs. The helper provides standard way to detect the ADC channel
nodes (by the node name), and a standard way to convert the "reg",
"diff-channels", "single-channel" and the "common-mode-channel" to
channel identification numbers used in the struct iio_chan_spec.
Furthermore, the helper checks the ID is in range of 0 ... num-channels.

The original driver treated all found child nodes as channel nodes. The
new helper requires channel nodes to be named channel[@N]. This should
help avoid problems with devices which may contain also other but ADC
child nodes. Quick grep from arch/* with the sun20i-gpadc's compatible
string didn't reveal any in-tree .dts with channel nodes named
othervice. Also, same grep shows all the in-tree .dts seem to have
channel IDs between 0..num of channels.

Use the new helper.

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

---
Revision history:
v2 => v3:
 - New patch

I picked the sun20i-gpadc in this series because it has a straightforward
approach for populating the struct iio_chan_spec. Everything else except
the .channel can use 'template'-data.

This makes the sun20i-gpadc well suited to be an example user of this new
helper. I hope this patch helps to evaluate whether these helpers are worth
the hassle.

The change is compile tested only!! Testing before applying is highly
appreciated (as always!). Also, even though I tried to audit the dts
files for the reg-properties in the channel nodes, use of references
didn't make it easy. I can't guarantee I didn't miss anything.
---
 drivers/iio/adc/sun20i-gpadc-iio.c | 42 ++++++++++++++----------------
 1 file changed, 19 insertions(+), 23 deletions(-)

diff --git a/drivers/iio/adc/sun20i-gpadc-iio.c b/drivers/iio/adc/sun20i-gpadc-iio.c
index 136b8d9c294f..36d48d95f029 100644
--- a/drivers/iio/adc/sun20i-gpadc-iio.c
+++ b/drivers/iio/adc/sun20i-gpadc-iio.c
@@ -15,6 +15,7 @@
 #include <linux/property.h>
 #include <linux/reset.h>
 
+#include <linux/iio/adc-helpers.h>
 #include <linux/iio/iio.h>
 
 #define SUN20I_GPADC_DRIVER_NAME	"sun20i-gpadc"
@@ -149,37 +150,32 @@ static void sun20i_gpadc_reset_assert(void *data)
 	reset_control_assert(rst);
 }
 
+static const struct iio_chan_spec sun20i_gpadc_chan_template = {
+	.type = IIO_VOLTAGE,
+	.indexed = 1,
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
+};
+
+static const struct iio_adc_props sun20i_gpadc_chan_props = {
+	.required = IIO_ADC_CHAN_PROP_TYPE_REG,
+};
+
 static int sun20i_gpadc_alloc_channels(struct iio_dev *indio_dev,
 				       struct device *dev)
 {
-	unsigned int channel;
-	int num_channels, i, ret;
+	int num_channels;
 	struct iio_chan_spec *channels;
 
-	num_channels = device_get_child_node_count(dev);
+	num_channels = devm_iio_adc_device_alloc_chaninfo(dev,
+					&sun20i_gpadc_chan_template, &channels,
+					&sun20i_gpadc_chan_props);
+	if (num_channels < 0)
+		return num_channels;
+
 	if (num_channels == 0)
 		return dev_err_probe(dev, -ENODEV, "no channel children\n");
 
-	channels = devm_kcalloc(dev, num_channels, sizeof(*channels),
-				GFP_KERNEL);
-	if (!channels)
-		return -ENOMEM;
-
-	i = 0;
-	device_for_each_child_node_scoped(dev, node) {
-		ret = fwnode_property_read_u32(node, "reg", &channel);
-		if (ret)
-			return dev_err_probe(dev, ret, "invalid channel number\n");
-
-		channels[i].type = IIO_VOLTAGE;
-		channels[i].indexed = 1;
-		channels[i].channel = channel;
-		channels[i].info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
-		channels[i].info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE);
-
-		i++;
-	}
-
 	indio_dev->channels = channels;
 	indio_dev->num_channels = num_channels;
 
-- 
2.48.1


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

[PATCH v3 8/9] iio: adc: ti-ads7924 Drop unnecessary function parameters

[Matti Vaittinen]

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

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

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

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

---
This commit is compile-tested only! All further testing is appreciated.
---
 drivers/iio/adc/ti-ads7924.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/adc/ti-ads7924.c b/drivers/iio/adc/ti-ads7924.c
index 66b54c0d75aa..b1f745f75dbe 100644
--- a/drivers/iio/adc/ti-ads7924.c
+++ b/drivers/iio/adc/ti-ads7924.c
@@ -251,11 +251,8 @@ static const struct iio_info ads7924_info = {
 	.read_raw = ads7924_read_raw,
 };
 
-static int ads7924_get_channels_config(struct i2c_client *client,
-				       struct iio_dev *indio_dev)
+static int ads7924_get_channels_config(struct device *dev)
 {
-	struct ads7924_data *priv = iio_priv(indio_dev);
-	struct device *dev = priv->dev;
 	struct fwnode_handle *node;
 	int num_channels = 0;
 
@@ -380,7 +377,7 @@ static int ads7924_probe(struct i2c_client *client)
 	indio_dev->num_channels = ARRAY_SIZE(ads7924_channels);
 	indio_dev->info = &ads7924_info;
 
-	ret = ads7924_get_channels_config(client, indio_dev);
+	ret = ads7924_get_channels_config(dev);
 	if (ret < 0)
 		return dev_err_probe(dev, ret,
 				     "failed to get channels configuration\n");
-- 
2.48.1


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

[PATCH v3 9/9] iio: adc: ti-ads7924: Respect device tree config

[Matti Vaittinen]

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

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

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

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

---
Revision history:
v2 => v3: New patch

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

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

This has only been compile tested! All testing is highly appreciated.
---
 drivers/iio/adc/ti-ads7924.c | 80 +++++++++++++++++-------------------
 1 file changed, 37 insertions(+), 43 deletions(-)

diff --git a/drivers/iio/adc/ti-ads7924.c b/drivers/iio/adc/ti-ads7924.c
index b1f745f75dbe..a5b8f7c81b8a 100644
--- a/drivers/iio/adc/ti-ads7924.c
+++ b/drivers/iio/adc/ti-ads7924.c
@@ -22,6 +22,7 @@
 #include <linux/regmap.h>
 #include <linux/regulator/consumer.h>
 
+#include <linux/iio/adc-helpers.h>
 #include <linux/iio/iio.h>
 #include <linux/iio/types.h>
 
@@ -119,15 +120,12 @@
 #define ADS7924_TOTAL_CONVTIME_US (ADS7924_PWRUPTIME_US + ADS7924_ACQTIME_US + \
 				   ADS7924_CONVTIME_US)
 
-#define ADS7924_V_CHAN(_chan, _addr) {				\
-	.type = IIO_VOLTAGE,					\
-	.indexed = 1,						\
-	.channel = _chan,					\
-	.address = _addr,					\
-	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW), 		\
-	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),	\
-	.datasheet_name = "AIN"#_chan,				\
-}
+static const struct iio_chan_spec ads7924_chan_template = {
+	.type = IIO_VOLTAGE,
+	.indexed = 1,
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
+};
 
 struct ads7924_data {
 	struct device *dev;
@@ -182,13 +180,6 @@ static const struct regmap_config ads7924_regmap_config = {
 	.writeable_reg = ads7924_is_writeable_reg,
 };
 
-static const struct iio_chan_spec ads7924_channels[] = {
-	ADS7924_V_CHAN(0, ADS7924_DATA0_U_REG),
-	ADS7924_V_CHAN(1, ADS7924_DATA1_U_REG),
-	ADS7924_V_CHAN(2, ADS7924_DATA2_U_REG),
-	ADS7924_V_CHAN(3, ADS7924_DATA3_U_REG),
-};
-
 static int ads7924_get_adc_result(struct ads7924_data *data,
 				  struct iio_chan_spec const *chan, int *val)
 {
@@ -251,32 +242,38 @@ static const struct iio_info ads7924_info = {
 	.read_raw = ads7924_read_raw,
 };
 
-static int ads7924_get_channels_config(struct device *dev)
+static const struct iio_adc_props ads7924_chan_props = {
+	.required = IIO_ADC_CHAN_PROP_TYPE_REG,
+};
+
+static int ads7924_get_channels_config(struct iio_dev *indio_dev,
+				       struct device *dev)
 {
-	struct fwnode_handle *node;
-	int num_channels = 0;
+	struct iio_chan_spec *chan_array;
+	int num_channels = 0, i;
 
-	device_for_each_child_node(dev, node) {
-		u32 pval;
-		unsigned int channel;
+	num_channels = devm_iio_adc_device_alloc_chaninfo(dev,
+					&ads7924_chan_template, &chan_array,
+					&ads7924_chan_props);
 
-		if (fwnode_property_read_u32(node, "reg", &pval)) {
-			dev_err(dev, "invalid reg on %pfw\n", node);
-			continue;
-		}
+	if (num_channels < 0)
+		return num_channels;
 
-		channel = pval;
-		if (channel >= ADS7924_CHANNELS) {
-			dev_err(dev, "invalid channel index %d on %pfw\n",
-				channel, node);
-			continue;
-		}
+	if (!num_channels)
+		return -EINVAL;
+
+	for (i = 0; i < num_channels; i++) {
+		static const char * const datasheet_names[] = {
+			"AIN0", "AIN1", "AIN2", "AIN3"
+		};
+		int ch_id = chan_array[i].channel;
 
-		num_channels++;
+		chan_array[i].address = ADS7924_DATA0_U_REG + ch_id;
+		chan_array[i].datasheet_name = datasheet_names[ch_id];
 	}
 
-	if (!num_channels)
-		return -EINVAL;
+	indio_dev->channels = chan_array;
+	indio_dev->num_channels = num_channels;
 
 	return 0;
 }
@@ -370,18 +367,15 @@ static int ads7924_probe(struct i2c_client *client)
 
 	mutex_init(&data->lock);
 
-	indio_dev->name = "ads7924";
-	indio_dev->modes = INDIO_DIRECT_MODE;
-
-	indio_dev->channels = ads7924_channels;
-	indio_dev->num_channels = ARRAY_SIZE(ads7924_channels);
-	indio_dev->info = &ads7924_info;
-
-	ret = ads7924_get_channels_config(dev);
+	ret = ads7924_get_channels_config(indio_dev, dev);
 	if (ret < 0)
 		return dev_err_probe(dev, ret,
 				     "failed to get channels configuration\n");
 
+	indio_dev->name = "ads7924";
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->info = &ads7924_info;
+
 	data->regmap = devm_regmap_init_i2c(client, &ads7924_regmap_config);
 	if (IS_ERR(data->regmap))
 		return dev_err_probe(dev, PTR_ERR(data->regmap),
-- 
2.48.1


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

Re: [PATCH v3 6/9] iio: adc: rzg2l_adc: Use adc-helpers

[Matti Vaittinen]

On 19/02/2025 14:31, Matti Vaittinen wrote:
> The new devm_iio_adc_device_alloc_chaninfo() -helper is intended to help
> drivers avoid open-coding the for_each_node -loop for getting the
> channel IDs. The helper provides standard way to detect the ADC channel
> nodes (by the node name), and a standard way to convert the "reg",
> "diff-channels", "single-channel" and the "common-mode-channel" to
> channel identification numbers used in the struct iio_chan_spec.
> Furthermore, the helper checks the ID is in range of 0 ... num-channels.
> 
> The original driver treated all found child nodes as channel nodes. The
> new helper requires channel nodes to be named channel[@N]. This should
> help avoid problems with devices which may contain also other but ADC
> child nodes. Quick grep from arch/* with the rzg2l_adc's compatible
> string didn't reveal any in-tree .dts with channel nodes named
> othervice. Also, same grep shows all the .dts seem to have channel IDs
> between 0..num of channels.
> 
> Use the new helper.
> 
> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
> 
> ---
> Revision history:
> v2 => v3:
>   - New patch
> 
> I picked the rzg2l_adc in this series because it has a straightforward
> approach for populating the struct iio_chan_spec. Only other member
> in the stuct besides the .channel, which can't use a 'template' -data,
> is the .datasheet_name. This makes the rzg2l_adc well suited for example
> user of this new helper. I hope this patch helps to evaluate whether these
> helpers are worth the hassle.
> 
> The change is compile tested only!! Testing before applying is highly
> appreciated (as always!).
> ---
>   drivers/iio/adc/rzg2l_adc.c | 41 ++++++++++++++++++-------------------
>   1 file changed, 20 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/iio/adc/rzg2l_adc.c b/drivers/iio/adc/rzg2l_adc.c
> index cd3a7e46ea53..3e1c74019785 100644
> --- a/drivers/iio/adc/rzg2l_adc.c
> +++ b/drivers/iio/adc/rzg2l_adc.c
> @@ -11,6 +11,7 @@

...

>   
> +static const struct iio_chan_spec rzg2l_adc_chan_template = {
> +	.type = IIO_VOLTAGE,

I just rebased this to v6.14-rc3 and noticed the channel type can no 
longer come from the template. There are also some other minor changes. 
I'll fix this in v4 if this same approach is kept.

> +	.indexed = 1,
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> +};
> +

Yours,
	-- Matti

Re: [PATCH v3 2/9] iio: adc: add helpers for parsing ADC nodes

[Andy Shevchenko]

On Wed, Feb 19, 2025 at 02:30:27PM +0200, Matti Vaittinen wrote:
> There are ADC ICs which may have some of the AIN pins usable for other
> functions. These ICs may have some of the AIN pins wired so that they
> should not be used for ADC.
> 
> (Preferred?) way for marking pins which can be used as ADC inputs is to
> add corresponding channels@N nodes in the device tree as described in
> the ADC binding yaml.
> 
> Add couple of helper functions which can be used to retrieve the channel
> information from the device node.

...

>  - Rename iio_adc_device_get_channels() as

as?

...

> 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

Shouldn't this be grouped with other IIO core related objects?

>  obj-$(CONFIG_IMX7D_ADC) += imx7d_adc.o
>  obj-$(CONFIG_IMX8QXP_ADC) += imx8qxp-adc.o
>  obj-$(CONFIG_IMX93_ADC) += imx93_adc.o

...


+ bitops.h

> +#include <linux/device.h>
> +#include <linux/errno.h>

+ export.h

+ module.h

> +#include <linux/property.h>

+ types.h

...

> +EXPORT_SYMBOL_GPL(iio_adc_device_num_channels);

No namespace?

...

> +	if (!allowed_types || allowed_types & (~IIO_ADC_CHAN_PROP_TYPE_ALL)) {

Unneeded parentheses around negated value.

> +		dev_dbg(dev, "Invalid adc allowed prop types 0x%lx\n",
> +			allowed_types);
> +
> +		return -EINVAL;
> +	}
> +	if (found_types & (~allowed_types)) {

Ditto.

> +		long unknown_types = found_types & (~allowed_types);

Ditto and so on...

Where did you get this style from? I think I see it first time in your
contributions. Is it a new preferences? Why?

> +		int type;
> +
> +		for_each_set_bit(type, &unknown_types,
> +				 IIO_ADC_CHAN_NUM_PROP_TYPES - 1) {
> +			dev_err(dev, "Unsupported channel property %s\n",
> +				iio_adc_type2prop(type));
> +		}
> +
> +		return -EINVAL;
> +	}

...

> +int iio_adc_device_channels_by_property(struct device *dev, int *channels,
> +		int max_channels, const struct iio_adc_props *expected_props)
> +{
> +	int num_chan = 0, ret;
> +
> +	device_for_each_child_node_scoped(dev, child) {
> +		u32 ch, diff[2], se;
> +		struct iio_adc_props tmp;
> +		int chtypes_found = 0;
> +
> +		if (!fwnode_name_eq(child, "channel"))
> +			continue;
> +
> +		if (num_chan == max_channels)
> +			return -EINVAL;
> +
> +		ret = fwnode_property_read_u32(child, "reg", &ch);
> +		if (ret)
> +			return ret;
> +
> +		ret = fwnode_property_read_u32_array(child, "diff-channels",
> +						     &diff[0], 2);

						     diff, ARRAY_SIZE(diff));

(will require array_size.h)


> +		if (!ret)
> +			chtypes_found |= IIO_ADC_CHAN_PROP_TYPE_DIFF;
> +
> +		ret = fwnode_property_read_u32(child, "single-channel", &se);
> +		if (!ret)
> +			chtypes_found |= IIO_ADC_CHAN_PROP_TYPE_SINGLE_ENDED;
> +
> +		tmp = *expected_props;
> +		/*
> +		 * We don't bother reading the "common-mode-channel" here as it
> +		 * doesn't really affect on the primary channel ID. We remove
> +		 * it from the required properties to allow the sanity check
> +		 * pass here  also for drivers which require it.
> +		 */
> +		tmp.required &= (~BIT(IIO_ADC_CHAN_PROP_COMMON));

Redundant outer parentheses. What's the point, please?

> +		ret = iio_adc_prop_type_check_sanity(dev, &tmp, chtypes_found);
> +		if (ret)
> +			return ret;
> +
> +		if (chtypes_found & IIO_ADC_CHAN_PROP_TYPE_DIFF)
> +			ch = diff[0];
> +		else if (chtypes_found & IIO_ADC_CHAN_PROP_TYPE_SINGLE_ENDED)
> +			ch = se;
> +
> +		/*
> +		 * 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;
> +
> +}

...

> +int devm_iio_adc_device_alloc_chaninfo(struct device *dev,
> +				const struct iio_chan_spec *template,
> +				struct iio_chan_spec **cs,
> +				const struct iio_adc_props *expected_props)
> +{
> +	struct iio_chan_spec *chan;
> +	int num_chan = 0, ret;
> +
> +	num_chan = iio_adc_device_num_channels(dev);
> +	if (num_chan < 1)
> +		return num_chan;
> +
> +	*cs = devm_kcalloc(dev, num_chan, sizeof(**cs), GFP_KERNEL);
> +	if (!*cs)
> +		return -ENOMEM;
> +
> +	chan = &(*cs)[0];

This and above and below will be easier to read if you introduce a temporary
variable which will be used locally and assigned to the output later on.
Also the current approach breaks the rule that infiltrates the output even in
the error cases.

> +	device_for_each_child_node_scoped(dev, child) {
> +		u32 ch, diff[2], se, common;
> +		int chtypes_found = 0;
> +
> +		if (!fwnode_name_eq(child, "channel"))
> +			continue;
> +
> +		ret = fwnode_property_read_u32(child, "reg", &ch);
> +		if (ret)
> +			return ret;
> +
> +		ret = fwnode_property_read_u32_array(child, "diff-channels",
> +						     &diff[0], 2);

As per above.

> +		if (!ret)
> +			chtypes_found |= IIO_ADC_CHAN_PROP_TYPE_DIFF;
> +
> +		ret = fwnode_property_read_u32(child, "single-channel", &se);
> +		if (!ret)
> +			chtypes_found |= IIO_ADC_CHAN_PROP_TYPE_SINGLE_ENDED;

> +		ret = fwnode_property_read_u32(child, "common-mode-channel",
> +					       &common);

I believe this is okay to have on a single line,

> +		if (!ret)
> +			chtypes_found |= BIT(IIO_ADC_CHAN_PROP_COMMON);
> +
> +		ret = iio_adc_prop_type_check_sanity(dev, expected_props,
> +						     chtypes_found);
> +		if (ret)
> +			return ret;
> +
> +		*chan = *template;
> +		chan->channel = ch;
> +
> +		if (chtypes_found & IIO_ADC_CHAN_PROP_TYPE_DIFF) {
> +			chan->differential = 1;
> +			chan->channel = diff[0];
> +			chan->channel2 = diff[1];
> +
> +		} else if (chtypes_found & IIO_ADC_CHAN_PROP_TYPE_SINGLE_ENDED) {
> +			chan->channel = se;
> +			if (chtypes_found & BIT(IIO_ADC_CHAN_PROP_COMMON))
> +				chan->channel2 = common;
> +		}
> +
> +		/*
> +		 * We assume the channel IDs start from 0. If it seems this is
> +		 * not a sane assumption, then we have to add 'allowed ID ranges'
> +		 * to the struct iio_adc_props because some of the callers may
> +		 * rely on the IDs being in this range - and have arrays indexed
> +		 * by the ID.
> +		 */
> +		if (chan->channel >= num_chan)
> +			return -ERANGE;
> +
> +		chan++;
> +	}
> +
> +	return num_chan;
> +}

...

> +#ifndef _INDUSTRIAL_IO_ADC_HELPERS_H_
> +#define _INDUSTRIAL_IO_ADC_HELPERS_H_

+ bits.h

> +#include <linux/iio/iio.h>

I'm failing to see how this is being used in this header.

> +struct device;
> +struct fwnode_handle;
> +
> +enum {
> +	IIO_ADC_CHAN_PROP_REG,
> +	IIO_ADC_CHAN_PROP_SINGLE_ENDED,
> +	IIO_ADC_CHAN_PROP_DIFF,
> +	IIO_ADC_CHAN_PROP_COMMON,
> +	IIO_ADC_CHAN_NUM_PROP_TYPES
> +};
> +
> +/*
> + * Channel property types to be used with iio_adc_device_get_channels,
> + * devm_iio_adc_device_alloc_chaninfo, ...

Looks like unfinished sentence...

> + */
> +#define IIO_ADC_CHAN_PROP_TYPE_REG BIT(IIO_ADC_CHAN_PROP_REG)
> +#define IIO_ADC_CHAN_PROP_TYPE_SINGLE_ENDED BIT(IIO_ADC_CHAN_PROP_SINGLE_ENDED)
> +#define IIO_ADC_CHAN_PROP_TYPE_SINGLE_COMMON					\
> +	(BIT(IIO_ADC_CHAN_PROP_SINGLE_ENDED) | BIT(IIO_ADC_CHAN_PROP_COMMON))
> +#define IIO_ADC_CHAN_PROP_TYPE_DIFF BIT(IIO_ADC_CHAN_PROP_DIFF)
> +#define IIO_ADC_CHAN_PROP_TYPE_ALL GENMASK(IIO_ADC_CHAN_NUM_PROP_TYPES - 1, 0)

> +int devm_iio_adc_device_alloc_chaninfo(struct device *dev,
> +				const struct iio_chan_spec *template,
> +				struct iio_chan_spec **cs,
> +				const struct iio_adc_props *expected_props);
> +
> +int iio_adc_device_channels_by_property(struct device *dev, int *channels,
> +				int max_channels,
> +				const struct iio_adc_props *expected_props);
> +#endif /* _INDUSTRIAL_IO_ADC_HELPERS_H_ */


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 2/9] iio: adc: add helpers for parsing ADC nodes

[Matti Vaittinen]

Hi Andy,

Long time no hear ;) First of all, thanks for the review!

On 19/02/2025 22:41, Andy Shevchenko wrote:
> On Wed, Feb 19, 2025 at 02:30:27PM +0200, Matti Vaittinen wrote:
>> There are ADC ICs which may have some of the AIN pins usable for other
>> functions. These ICs may have some of the AIN pins wired so that they
>> should not be used for ADC.
>>
>> (Preferred?) way for marking pins which can be used as ADC inputs is to
>> add corresponding channels@N nodes in the device tree as described in
>> the ADC binding yaml.
>>
>> Add couple of helper functions which can be used to retrieve the channel
>> information from the device node.
> 
> ...
> 
>>   - Rename iio_adc_device_get_channels() as
> 
> as?

Oh, looks like I got interrupted :) Thanks!

> 
> ...
> 
>> 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
> 
> Shouldn't this be grouped with other IIO core related objects?

I was unsure where to put this. The 'adc' subfolder contained no other 
IIO core files, so there really was no group. I did consider putting it 
on top of the file but then just decided to go with the alphabetical 
order. Not sure what is the right way though.

>>   obj-$(CONFIG_IMX7D_ADC) += imx7d_adc.o
>>   obj-$(CONFIG_IMX8QXP_ADC) += imx8qxp-adc.o
>>   obj-$(CONFIG_IMX93_ADC) += imx93_adc.o
> 
> ...
> 
> 
> + bitops.h
> 
>> +#include <linux/device.h>
>> +#include <linux/errno.h>
> 
> + export.h
> 
> + module.h
> 
>> +#include <linux/property.h>
> 
> + types.h

Thanks!

> ...
> 
>> +EXPORT_SYMBOL_GPL(iio_adc_device_num_channels);
> 
> No namespace?

I was considering also this. The IIO core functions don't belong into a 
namespace - so I followed the convention to keep these similar to other 
IIO core stuff.

(Sometimes I have a feeling that the trend today is to try make things 
intentionally difficult in the name of the safety. Like, "more difficult 
I make this, more experience points I gain in the name of the safety".)

Well, I suppose I could add a namespace for these functions - if this 
approach stays - but I'd really prefer having all IIO core stuff in some 
global IIO namespace and not to have dozens of fine-grained namespaces 
for an IIO driver to use...

> ...
> 
>> +	if (!allowed_types || allowed_types & (~IIO_ADC_CHAN_PROP_TYPE_ALL)) {
> 
> Unneeded parentheses around negated value.
> 
>> +		dev_dbg(dev, "Invalid adc allowed prop types 0x%lx\n",
>> +			allowed_types);
>> +
>> +		return -EINVAL;
>> +	}
>> +	if (found_types & (~allowed_types)) {
> 
> Ditto.
> 
>> +		long unknown_types = found_types & (~allowed_types);
> 
> Ditto and so on...
> 
> Where did you get this style from? I think I see it first time in your
> contributions. Is it a new preferences? Why?

Last autumn I found out my house was damaged by water. I had to empty 
half of the rooms and finally move out for 2.5 months. Now I'm finally 
back, but during the moves I lost my printed list of operator 
precedences which I used to have on my desk. I've been writing C for 25 
years or so, and I still don't remember the precedence rules for all 
bitwise operations - and I am fairly convinced I am not the only one.

What I understood is that I don't really have to have a printed list at 
home, or go googling when away from home. I can just make it very, very 
obvious :) Helps me a lot.

> 
>> +		int type;
>> +
>> +		for_each_set_bit(type, &unknown_types,
>> +				 IIO_ADC_CHAN_NUM_PROP_TYPES - 1) {
>> +			dev_err(dev, "Unsupported channel property %s\n",
>> +				iio_adc_type2prop(type));
>> +		}
>> +
>> +		return -EINVAL;
>> +	}
> 
> ...
> 
>> +int iio_adc_device_channels_by_property(struct device *dev, int *channels,
>> +		int max_channels, const struct iio_adc_props *expected_props)
>> +{
>> +	int num_chan = 0, ret;
>> +
>> +	device_for_each_child_node_scoped(dev, child) {
>> +		u32 ch, diff[2], se;
>> +		struct iio_adc_props tmp;
>> +		int chtypes_found = 0;
>> +
>> +		if (!fwnode_name_eq(child, "channel"))
>> +			continue;
>> +
>> +		if (num_chan == max_channels)
>> +			return -EINVAL;
>> +
>> +		ret = fwnode_property_read_u32(child, "reg", &ch);
>> +		if (ret)
>> +			return ret;
>> +
>> +		ret = fwnode_property_read_u32_array(child, "diff-channels",
>> +						     &diff[0], 2);
> 
> 						     diff, ARRAY_SIZE(diff));
> 
> (will require array_size.h)

thanks :) And thanks for being helpful with the header - and there is no 
sarcasm!

>> +		if (!ret)
>> +			chtypes_found |= IIO_ADC_CHAN_PROP_TYPE_DIFF;
>> +
>> +		ret = fwnode_property_read_u32(child, "single-channel", &se);
>> +		if (!ret)
>> +			chtypes_found |= IIO_ADC_CHAN_PROP_TYPE_SINGLE_ENDED;
>> +
>> +		tmp = *expected_props;
>> +		/*
>> +		 * We don't bother reading the "common-mode-channel" here as it
>> +		 * doesn't really affect on the primary channel ID. We remove
>> +		 * it from the required properties to allow the sanity check
>> +		 * pass here  also for drivers which require it.
>> +		 */
>> +		tmp.required &= (~BIT(IIO_ADC_CHAN_PROP_COMMON));
> 
> Redundant outer parentheses. What's the point, please?

Zero need to think of precedence.

>> +		ret = iio_adc_prop_type_check_sanity(dev, &tmp, chtypes_found);
>> +		if (ret)
>> +			return ret;
>> +
>> +		if (chtypes_found & IIO_ADC_CHAN_PROP_TYPE_DIFF)
>> +			ch = diff[0];
>> +		else if (chtypes_found & IIO_ADC_CHAN_PROP_TYPE_SINGLE_ENDED)
>> +			ch = se;
>> +
>> +		/*
>> +		 * 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;
>> +
>> +}
> 
> ...
> 
>> +int devm_iio_adc_device_alloc_chaninfo(struct device *dev,
>> +				const struct iio_chan_spec *template,
>> +				struct iio_chan_spec **cs,
>> +				const struct iio_adc_props *expected_props)
>> +{
>> +	struct iio_chan_spec *chan;
>> +	int num_chan = 0, ret;
>> +
>> +	num_chan = iio_adc_device_num_channels(dev);
>> +	if (num_chan < 1)
>> +		return num_chan;
>> +
>> +	*cs = devm_kcalloc(dev, num_chan, sizeof(**cs), GFP_KERNEL);
>> +	if (!*cs)
>> +		return -ENOMEM;
>> +
>> +	chan = &(*cs)[0];
> 
> This and above and below will be easier to read if you introduce a temporary
> variable which will be used locally and assigned to the output later on.
> Also the current approach breaks the rule that infiltrates the output even in
> the error cases.

Agree. Thanks.

> 
>> +	device_for_each_child_node_scoped(dev, child) {
>> +		u32 ch, diff[2], se, common;
>> +		int chtypes_found = 0;
>> +
>> +		if (!fwnode_name_eq(child, "channel"))
>> +			continue;
>> +
>> +		ret = fwnode_property_read_u32(child, "reg", &ch);
>> +		if (ret)
>> +			return ret;
>> +
>> +		ret = fwnode_property_read_u32_array(child, "diff-channels",
>> +						     &diff[0], 2);
> 
> As per above.
> 
>> +		if (!ret)
>> +			chtypes_found |= IIO_ADC_CHAN_PROP_TYPE_DIFF;
>> +
>> +		ret = fwnode_property_read_u32(child, "single-channel", &se);
>> +		if (!ret)
>> +			chtypes_found |= IIO_ADC_CHAN_PROP_TYPE_SINGLE_ENDED;
> 
>> +		ret = fwnode_property_read_u32(child, "common-mode-channel",
>> +					       &common);
> 
> I believe this is okay to have on a single line,

I try to keep things under 80 chars. It really truly helps me as I'd 
like to have 3 parallel terminals open when writing code. Furthermore, I 
hate to admit it but during the last two years my near vision has 
deteriorated... :/ 40 is getting more distant and 50 is approaching ;)

> 
>> +		if (!ret)
>> +			chtypes_found |= BIT(IIO_ADC_CHAN_PROP_COMMON);
>> +
>> +		ret = iio_adc_prop_type_check_sanity(dev, expected_props,
>> +						     chtypes_found);
>> +		if (ret)
>> +			return ret;
>> +
>> +		*chan = *template;
>> +		chan->channel = ch;
>> +
>> +		if (chtypes_found & IIO_ADC_CHAN_PROP_TYPE_DIFF) {
>> +			chan->differential = 1;
>> +			chan->channel = diff[0];
>> +			chan->channel2 = diff[1];
>> +
>> +		} else if (chtypes_found & IIO_ADC_CHAN_PROP_TYPE_SINGLE_ENDED) {
>> +			chan->channel = se;
>> +			if (chtypes_found & BIT(IIO_ADC_CHAN_PROP_COMMON))
>> +				chan->channel2 = common;
>> +		}
>> +
>> +		/*
>> +		 * We assume the channel IDs start from 0. If it seems this is
>> +		 * not a sane assumption, then we have to add 'allowed ID ranges'
>> +		 * to the struct iio_adc_props because some of the callers may
>> +		 * rely on the IDs being in this range - and have arrays indexed
>> +		 * by the ID.
>> +		 */
>> +		if (chan->channel >= num_chan)
>> +			return -ERANGE;
>> +
>> +		chan++;
>> +	}
>> +
>> +	return num_chan;
>> +}
> 
> ...
> 
>> +#ifndef _INDUSTRIAL_IO_ADC_HELPERS_H_
>> +#define _INDUSTRIAL_IO_ADC_HELPERS_H_
> 
> + bits.h
> 
>> +#include <linux/iio/iio.h>
> 
> I'm failing to see how this is being used in this header.

I suppose it was the struct iio_chan_spec. Yep, forward declaration 
could do, but I guess there would be no benefit because anyone using 
this header is more than likely to use the iio.h as well.

> 
>> +struct device;
>> +struct fwnode_handle;
>> +
>> +enum {
>> +	IIO_ADC_CHAN_PROP_REG,
>> +	IIO_ADC_CHAN_PROP_SINGLE_ENDED,
>> +	IIO_ADC_CHAN_PROP_DIFF,
>> +	IIO_ADC_CHAN_PROP_COMMON,
>> +	IIO_ADC_CHAN_NUM_PROP_TYPES
>> +};
>> +
>> +/*
>> + * Channel property types to be used with iio_adc_device_get_channels,
>> + * devm_iio_adc_device_alloc_chaninfo, ...
> 
> Looks like unfinished sentence...

Intention was to just give user an example of functions where this gets 
used, and leave room for more functions to be added. Reason is that 
lists like this tend to end up being incomplete anyways. Hence the ...

> 
>> + */
>> +#define IIO_ADC_CHAN_PROP_TYPE_REG BIT(IIO_ADC_CHAN_PROP_REG)
>> +#define IIO_ADC_CHAN_PROP_TYPE_SINGLE_ENDED BIT(IIO_ADC_CHAN_PROP_SINGLE_ENDED)
>> +#define IIO_ADC_CHAN_PROP_TYPE_SINGLE_COMMON					\
>> +	(BIT(IIO_ADC_CHAN_PROP_SINGLE_ENDED) | BIT(IIO_ADC_CHAN_PROP_COMMON))
>> +#define IIO_ADC_CHAN_PROP_TYPE_DIFF BIT(IIO_ADC_CHAN_PROP_DIFF)
>> +#define IIO_ADC_CHAN_PROP_TYPE_ALL GENMASK(IIO_ADC_CHAN_NUM_PROP_TYPES - 1, 0)
> 
>> +int devm_iio_adc_device_alloc_chaninfo(struct device *dev,
>> +				const struct iio_chan_spec *template,
>> +				struct iio_chan_spec **cs,
>> +				const struct iio_adc_props *expected_props);
>> +
>> +int iio_adc_device_channels_by_property(struct device *dev, int *channels,
>> +				int max_channels,
>> +				const struct iio_adc_props *expected_props);
>> +#endif /* _INDUSTRIAL_IO_ADC_HELPERS_H_ */
> 
> 

Re: [PATCH v3 2/9] iio: adc: add helpers for parsing ADC nodes

[Andy Shevchenko]

On Thu, Feb 20, 2025 at 09:13:00AM +0200, Matti Vaittinen wrote:
> On 19/02/2025 22:41, Andy Shevchenko wrote:
> > On Wed, Feb 19, 2025 at 02:30:27PM +0200, Matti Vaittinen wrote:

...

> > > 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
> > 
> > Shouldn't this be grouped with other IIO core related objects?
> 
> I was unsure where to put this. The 'adc' subfolder contained no other IIO
> core files, so there really was no group. I did consider putting it on top
> of the file but then just decided to go with the alphabetical order. Not
> sure what is the right way though.

I think it would be nice to have it grouped even if this one becomes
the first one.

> > >   obj-$(CONFIG_IMX7D_ADC) += imx7d_adc.o
> > >   obj-$(CONFIG_IMX8QXP_ADC) += imx8qxp-adc.o
> > >   obj-$(CONFIG_IMX93_ADC) += imx93_adc.o

...

> > > +EXPORT_SYMBOL_GPL(iio_adc_device_num_channels);
> > 
> > No namespace?
> 
> I was considering also this. The IIO core functions don't belong into a
> namespace - so I followed the convention to keep these similar to other IIO
> core stuff.

But it's historically. We have already started using namespaces
in the parts of IIO, haven't we?

> (Sometimes I have a feeling that the trend today is to try make things
> intentionally difficult in the name of the safety. Like, "more difficult I
> make this, more experience points I gain in the name of the safety".)
> 
> Well, I suppose I could add a namespace for these functions - if this
> approach stays - but I'd really prefer having all IIO core stuff in some
> global IIO namespace and not to have dozens of fine-grained namespaces for
> an IIO driver to use...

...

> > > +	if (!allowed_types || allowed_types & (~IIO_ADC_CHAN_PROP_TYPE_ALL)) {
> > 
> > Unneeded parentheses around negated value.
> > 
> > > +	if (found_types & (~allowed_types)) {
> > 
> > Ditto.
> > 
> > > +		long unknown_types = found_types & (~allowed_types);
> > 
> > Ditto and so on...
> > 
> > Where did you get this style from? I think I see it first time in your
> > contributions. Is it a new preferences? Why?
> 
> Last autumn I found out my house was damaged by water. I had to empty half
> of the rooms and finally move out for 2.5 months.

Sad to hear that... Hope that your house had been recovered (to some extent?).

> Now I'm finally back, but
> during the moves I lost my printed list of operator precedences which I used
> to have on my desk. I've been writing C for 25 years or so, and I still
> don't remember the precedence rules for all bitwise operations - and I am
> fairly convinced I am not the only one.

~ (a.k.a. negation) is higher priority in bitops and it's idiomatic
(at least in LK project).

> What I understood is that I don't really have to have a printed list at
> home, or go googling when away from home. I can just make it very, very
> obvious :) Helps me a lot.

Makes code harder to read, especially in the undoubtful cases like

	foo &= (~...);

> > > +		int type;
> > > +
> > > +		for_each_set_bit(type, &unknown_types,
> > > +				 IIO_ADC_CHAN_NUM_PROP_TYPES - 1) {
> > > +			dev_err(dev, "Unsupported channel property %s\n",
> > > +				iio_adc_type2prop(type));
> > > +		}
> > > +
> > > +		return -EINVAL;
> > > +	}

...

> > > +		tmp.required &= (~BIT(IIO_ADC_CHAN_PROP_COMMON));
> > 
> > Redundant outer parentheses. What's the point, please?
> 
> Zero need to think of precedence.

Huh? See above.
Everything with equal sign is less precedence than normal ops.

...

> > > +		ret = fwnode_property_read_u32(child, "common-mode-channel",
> > > +					       &common);
> > 
> > I believe this is okay to have on a single line,
> 
> I try to keep things under 80 chars. It really truly helps me as I'd like to
> have 3 parallel terminals open when writing code. Furthermore, I hate to
> admit it but during the last two years my near vision has deteriorated... :/
> 40 is getting more distant and 50 is approaching ;)

It's only 86 altogether with better readability.
We are in the second quarter of 21st century,
the 80 should be left in 80s...

(and yes, I deliberately put the above too short).

...

> > > +#include <linux/iio/iio.h>
> > 
> > I'm failing to see how this is being used in this header.
> 
> I suppose it was the struct iio_chan_spec. Yep, forward declaration could
> do, but I guess there would be no benefit because anyone using this header
> is more than likely to use the iio.h as well.

Still, it will be a beast to motivate people not thinking about what they are
doing. I strongly prefer avoiding the use of the "proxy" or dangling headers.

...

> > > +/*
> > > + * Channel property types to be used with iio_adc_device_get_channels,
> > > + * devm_iio_adc_device_alloc_chaninfo, ...
> > 
> > Looks like unfinished sentence...
> 
> Intention was to just give user an example of functions where this gets
> used, and leave room for more functions to be added. Reason is that lists
> like this tend to end up being incomplete anyways. Hence the ...

At least you may add ').' (without quotes) to that to make it clear.

> > > + */

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 2/9] iio: adc: add helpers for parsing ADC nodes

[Matti Vaittinen]

On 20/02/2025 14:41, Andy Shevchenko wrote:
> On Thu, Feb 20, 2025 at 09:13:00AM +0200, Matti Vaittinen wrote:
>> On 19/02/2025 22:41, Andy Shevchenko wrote:
>>> On Wed, Feb 19, 2025 at 02:30:27PM +0200, Matti Vaittinen wrote:
> 
> ...
> 
>>>> 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
>>>
>>> Shouldn't this be grouped with other IIO core related objects?
>>
>> I was unsure where to put this. The 'adc' subfolder contained no other IIO
>> core files, so there really was no group. I did consider putting it on top
>> of the file but then just decided to go with the alphabetical order. Not
>> sure what is the right way though.
> 
> I think it would be nice to have it grouped even if this one becomes
> the first one.

I will move this on top of the file. (If these helpers stay. I think I 
wrote somewhere - maybe in the cover letter - that people are not sure 
if this is worth or if every driver should just use the fwnode APIs. 
Reviewers may want to save energy and do more accurate review only to 
the next version...)

>>>>    obj-$(CONFIG_IMX7D_ADC) += imx7d_adc.o
>>>>    obj-$(CONFIG_IMX8QXP_ADC) += imx8qxp-adc.o
>>>>    obj-$(CONFIG_IMX93_ADC) += imx93_adc.o
> 
> ...
> 
>>>> +EXPORT_SYMBOL_GPL(iio_adc_device_num_channels);
>>>
>>> No namespace?
>>
>> I was considering also this. The IIO core functions don't belong into a
>> namespace - so I followed the convention to keep these similar to other IIO
>> core stuff.
> 
> But it's historically. We have already started using namespaces
> in the parts of IIO, haven't we?

Yes. But as I wrote, I don't think adding new namespaces for every 
helper file with a function or two exported will scale. We either need 
something common for IIO (or IIO "subsystems" like "adc", "accel", 
"light", ... ), or then we just keep these small helpers same as most of 
the IIO core.

>> (Sometimes I have a feeling that the trend today is to try make things
>> intentionally difficult in the name of the safety. Like, "more difficult I
>> make this, more experience points I gain in the name of the safety".)
>>
>> Well, I suppose I could add a namespace for these functions - if this
>> approach stays - but I'd really prefer having all IIO core stuff in some
>> global IIO namespace and not to have dozens of fine-grained namespaces for
>> an IIO driver to use...
> 
> ...
> 
>>>> +	if (!allowed_types || allowed_types & (~IIO_ADC_CHAN_PROP_TYPE_ALL)) {
>>>
>>> Unneeded parentheses around negated value.
>>>
>>>> +	if (found_types & (~allowed_types)) {
>>>
>>> Ditto.
>>>
>>>> +		long unknown_types = found_types & (~allowed_types);
>>>
>>> Ditto and so on...
>>>
>>> Where did you get this style from? I think I see it first time in your
>>> contributions. Is it a new preferences? Why?
>>
>> Last autumn I found out my house was damaged by water. I had to empty half
>> of the rooms and finally move out for 2.5 months.
> 
> Sad to hear that... Hope that your house had been recovered (to some extent?).

Thanks. I finalized rebuilding last weekend. Just moved back and now I'm 
trying to carry things back to right places... :rolleyes:

>> Now I'm finally back, but
>> during the moves I lost my printed list of operator precedences which I used
>> to have on my desk. I've been writing C for 25 years or so, and I still
>> don't remember the precedence rules for all bitwise operations - and I am
>> fairly convinced I am not the only one.
> 
> ~ (a.k.a. negation) is higher priority in bitops and it's idiomatic
> (at least in LK project).

I know there are well established, accurate rules. Problem is that I 
never remember these without looking.

>> What I understood is that I don't really have to have a printed list at
>> home, or go googling when away from home. I can just make it very, very
>> obvious :) Helps me a lot.
> 
> Makes code harder to read, especially in the undoubtful cases like
> 
> 	foo &= (~...);

This is not undoubtful case for me :) And believe me, reading and 
deciphering the

foo &= (~bar);

is _much_ faster than seeing:

foo &= ~bar;

and having to google the priorities.

>>>> +		int type;
>>>> +
>>>> +		for_each_set_bit(type, &unknown_types,
>>>> +				 IIO_ADC_CHAN_NUM_PROP_TYPES - 1) {
>>>> +			dev_err(dev, "Unsupported channel property %s\n",
>>>> +				iio_adc_type2prop(type));
>>>> +		}
>>>> +
>>>> +		return -EINVAL;
>>>> +	}
> 
> ...
> 
>>>> +		tmp.required &= (~BIT(IIO_ADC_CHAN_PROP_COMMON));
>>>
>>> Redundant outer parentheses. What's the point, please?
>>
>> Zero need to think of precedence.
> 
> Huh? See above.
> Everything with equal sign is less precedence than normal ops.

Sure. It's obvious if you remember that "Everything with equal sign is 
less precedence than normal ops". But as I said, I truly have hard time 
remembering these rules. When I try "going by memory" I end up having 
odd errors and suggestions to add parenthesis from the compiler...

By the way, do you know why anyone has bothered to add these 
warnings/suggestions about adding the parenthesis to the compiler? My 
guess is that I am not only one who needs the precedence charts ;)

> ...
> 
>>>> +		ret = fwnode_property_read_u32(child, "common-mode-channel",
>>>> +					       &common);
>>>
>>> I believe this is okay to have on a single line,
>>
>> I try to keep things under 80 chars. It really truly helps me as I'd like to
>> have 3 parallel terminals open when writing code. Furthermore, I hate to
>> admit it but during the last two years my near vision has deteriorated... :/
>> 40 is getting more distant and 50 is approaching ;)
> 
> It's only 86 altogether with better readability.
> We are in the second quarter of 21st century,
> the 80 should be left in 80s...
> 
> (and yes, I deliberately put the above too short).

I didn't even notice you had squeezed the lines :)

But yeah, I truly have problems fitting even 3 80 column terminals on 
screen with my current monitor. And when working on laptop screen it 
becomes impossible. Hence I strongly prefer keeping the 80 chars limit.

> ...
> 
>>>> +#include <linux/iio/iio.h>
>>>
>>> I'm failing to see how this is being used in this header.
>>
>> I suppose it was the struct iio_chan_spec. Yep, forward declaration could
>> do, but I guess there would be no benefit because anyone using this header
>> is more than likely to use the iio.h as well.
> 
> Still, it will be a beast to motivate people not thinking about what they are
> doing. I strongly prefer avoiding the use of the "proxy" or dangling headers.

Ehh. There will be no IIO user who does not include the iio.h. And, I 
need the iio_chan_spec here.

> ...
> 
>>>> +/*
>>>> + * Channel property types to be used with iio_adc_device_get_channels,
>>>> + * devm_iio_adc_device_alloc_chaninfo, ...
>>>
>>> Looks like unfinished sentence...
>>
>> Intention was to just give user an example of functions where this gets
>> used, and leave room for more functions to be added. Reason is that lists
>> like this tend to end up being incomplete anyways. Hence the ...
> 
> At least you may add ').' (without quotes) to that to make it clear.

Thanks. I agree, that's a good idea.

And as I said, I suggest saving some of the energy for reviewing the 
next version. I doubt the "property type" -flags and bitwise operations 
stay, and it may be all of this will be just meld in the bd79124 code - 
depending on what Jonathan & others think of it.

Yours,
	-- Matti

Re: [PATCH v3 2/9] iio: adc: add helpers for parsing ADC nodes

[Andy Shevchenko]

On Thu, Feb 20, 2025 at 03:40:30PM +0200, Matti Vaittinen wrote:
> On 20/02/2025 14:41, Andy Shevchenko wrote:
> > On Thu, Feb 20, 2025 at 09:13:00AM +0200, Matti Vaittinen wrote:
> > > On 19/02/2025 22:41, Andy Shevchenko wrote:
> > > > On Wed, Feb 19, 2025 at 02:30:27PM +0200, Matti Vaittinen wrote:

...

> > > > > +EXPORT_SYMBOL_GPL(iio_adc_device_num_channels);
> > > > 
> > > > No namespace?
> > > 
> > > I was considering also this. The IIO core functions don't belong into a
> > > namespace - so I followed the convention to keep these similar to other IIO
> > > core stuff.
> > 
> > But it's historically. We have already started using namespaces
> > in the parts of IIO, haven't we?
> 
> Yes. But as I wrote, I don't think adding new namespaces for every helper
> file with a function or two exported will scale. We either need something
> common for IIO (or IIO "subsystems" like "adc", "accel", "light", ... ), or
> then we just keep these small helpers same as most of the IIO core.

It can be still pushed to IIO_CORE namespace. Do you see an issue with that?

Or a new opaque namespace for the mentioned cases, something like IIO_HELPERS.

> > > (Sometimes I have a feeling that the trend today is to try make things
> > > intentionally difficult in the name of the safety. Like, "more difficult I
> > > make this, more experience points I gain in the name of the safety".)
> > > 
> > > Well, I suppose I could add a namespace for these functions - if this
> > > approach stays - but I'd really prefer having all IIO core stuff in some
> > > global IIO namespace and not to have dozens of fine-grained namespaces for
> > > an IIO driver to use...

...

> > > > > +	if (!allowed_types || allowed_types & (~IIO_ADC_CHAN_PROP_TYPE_ALL)) {
> > > > 
> > > > Unneeded parentheses around negated value.
> > > > 
> > > > > +	if (found_types & (~allowed_types)) {
> > > > 
> > > > Ditto.
> > > > 
> > > > > +		long unknown_types = found_types & (~allowed_types);
> > > > 
> > > > Ditto and so on...
> > > > 
> > > > Where did you get this style from? I think I see it first time in your
> > > > contributions. Is it a new preferences? Why?
> > > 
> > > Last autumn I found out my house was damaged by water. I had to empty half
> > > of the rooms and finally move out for 2.5 months.
> > 
> > Sad to hear that... Hope that your house had been recovered (to some extent?).
> 
> Thanks. I finalized rebuilding last weekend. Just moved back and now I'm
> trying to carry things back to right places... :rolleyes:
> 
> > > Now I'm finally back, but
> > > during the moves I lost my printed list of operator precedences which I used
> > > to have on my desk. I've been writing C for 25 years or so, and I still
> > > don't remember the precedence rules for all bitwise operations - and I am
> > > fairly convinced I am not the only one.
> > 
> > ~ (a.k.a. negation) is higher priority in bitops and it's idiomatic
> > (at least in LK project).
> 
> I know there are well established, accurate rules. Problem is that I never
> remember these without looking.

There are very obvious cases like below.

> > > What I understood is that I don't really have to have a printed list at
> > > home, or go googling when away from home. I can just make it very, very
> > > obvious :) Helps me a lot.
> > 
> > Makes code harder to read, especially in the undoubtful cases like
> > 
> > 	foo &= (~...);
> 
> This is not undoubtful case for me :) And believe me, reading and
> deciphering the
> 
> foo &= (~bar);
> 
> is _much_ faster than seeing:

Strongly disagree. One need to parse an additional pair of parentheses,
and especially when it's a big statement inside with nested ones along
with understanding what the heck is going on that you need them in the
first place.

On top of that, we have a common practices in the LK project and
with our history of communication it seems you are trying to do differently
from time to time. Sounds like a rebellion to me :-)

> foo &= ~bar;
> 
> and having to google the priorities.

Again, this is something a (regular) kernel developer keeps refreshed.
Or even wider, C-language developer.

> > > > > +		int type;
> > > > > +
> > > > > +		for_each_set_bit(type, &unknown_types,
> > > > > +				 IIO_ADC_CHAN_NUM_PROP_TYPES - 1) {
> > > > > +			dev_err(dev, "Unsupported channel property %s\n",
> > > > > +				iio_adc_type2prop(type));
> > > > > +		}
> > > > > +
> > > > > +		return -EINVAL;
> > > > > +	}

...

> > > > > +		tmp.required &= (~BIT(IIO_ADC_CHAN_PROP_COMMON));
> > > > 
> > > > Redundant outer parentheses. What's the point, please?
> > > 
> > > Zero need to think of precedence.
> > 
> > Huh? See above.
> > Everything with equal sign is less precedence than normal ops.
> 
> Sure. It's obvious if you remember that "Everything with equal sign is less
> precedence than normal ops". But as I said, I truly have hard time
> remembering these rules. When I try "going by memory" I end up having odd
> errors and suggestions to add parenthesis from the compiler...

The hardest to remember probably the

	foo && bar | baz

case and alike. These are the only ones that I totally agree on with you.
But negation.

> By the way, do you know why anyone has bothered to add these
> warnings/suggestions about adding the parenthesis to the compiler? My guess
> is that I am not only one who needs the precedence charts ;)

Maybe someone programmed too much in LISP?.. (it's a rhetorical one)

...

> > > > > +		ret = fwnode_property_read_u32(child, "common-mode-channel",
> > > > > +					       &common);
> > > > 
> > > > I believe this is okay to have on a single line,
> > > 
> > > I try to keep things under 80 chars. It really truly helps me as I'd like to
> > > have 3 parallel terminals open when writing code. Furthermore, I hate to
> > > admit it but during the last two years my near vision has deteriorated... :/
> > > 40 is getting more distant and 50 is approaching ;)
> > 
> > It's only 86 altogether with better readability.
> > We are in the second quarter of 21st century,
> > the 80 should be left in 80s...
> > 
> > (and yes, I deliberately put the above too short).
> 
> I didn't even notice you had squeezed the lines :)
> 
> But yeah, I truly have problems fitting even 3 80 column terminals on screen
> with my current monitor. And when working on laptop screen it becomes
> impossible. Hence I strongly prefer keeping the 80 chars limit.

Maybe you need a bigger monitor after all? (lurking now :-)

...

> > > > > +#include <linux/iio/iio.h>
> > > > 
> > > > I'm failing to see how this is being used in this header.
> > > 
> > > I suppose it was the struct iio_chan_spec. Yep, forward declaration could
> > > do, but I guess there would be no benefit because anyone using this header
> > > is more than likely to use the iio.h as well.
> > 
> > Still, it will be a beast to motivate people not thinking about what they are
> > doing. I strongly prefer avoiding the use of the "proxy" or dangling headers.
> 
> Ehh. There will be no IIO user who does not include the iio.h.

It's not your concern. That's the idea of making C units as much independent
and modular as possible (with common sense in mind). And in this case I see
no point of including this header. Again, the main problem is this will call
people to use the new header as a "proxy" and that's what I fully against to.

> And, I need the iio_chan_spec here.

Do you really need it or is it just a pointer?

...

> And as I said, I suggest saving some of the energy for reviewing the next
> version. I doubt the "property type" -flags and bitwise operations stay, and
> it may be all of this will be just meld in the bd79124 code - depending on
> what Jonathan & others think of it.

Whenever this code will be trying to land, the review comments still apply.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 2/9] iio: adc: add helpers for parsing ADC nodes

[Matti Vaittinen]

On 20/02/2025 16:04, Andy Shevchenko wrote:
> On Thu, Feb 20, 2025 at 03:40:30PM +0200, Matti Vaittinen wrote:
>> On 20/02/2025 14:41, Andy Shevchenko wrote:
>>> On Thu, Feb 20, 2025 at 09:13:00AM +0200, Matti Vaittinen wrote:
>>>> On 19/02/2025 22:41, Andy Shevchenko wrote:
>>>>> On Wed, Feb 19, 2025 at 02:30:27PM +0200, Matti Vaittinen wrote:
> 
> ...
> 
>>>>>> +EXPORT_SYMBOL_GPL(iio_adc_device_num_channels);
>>>>>
>>>>> No namespace?
>>>>
>>>> I was considering also this. The IIO core functions don't belong into a
>>>> namespace - so I followed the convention to keep these similar to other IIO
>>>> core stuff.
>>>
>>> But it's historically. We have already started using namespaces
>>> in the parts of IIO, haven't we?
>>
>> Yes. But as I wrote, I don't think adding new namespaces for every helper
>> file with a function or two exported will scale. We either need something
>> common for IIO (or IIO "subsystems" like "adc", "accel", "light", ... ), or
>> then we just keep these small helpers same as most of the IIO core.
> 
> It can be still pushed to IIO_CORE namespace. Do you see an issue with that?

No. I've missed the fact we have IIO_CORE O_o. Thanks for pointing it out!

> Or a new opaque namespace for the mentioned cases, something like IIO_HELPERS.

I am unsure if it really benefits to split this out of the IIO_CORE. 
I've a feeling it falls into the category of making things harder for 
user with no apparent reason. But yes, the IIO_CORE makes sense.

>>>> (Sometimes I have a feeling that the trend today is to try make things
>>>> intentionally difficult in the name of the safety. Like, "more difficult I
>>>> make this, more experience points I gain in the name of the safety".)
>>>>
>>>> Well, I suppose I could add a namespace for these functions - if this
>>>> approach stays - but I'd really prefer having all IIO core stuff in some
>>>> global IIO namespace and not to have dozens of fine-grained namespaces for
>>>> an IIO driver to use...
> 
> ...
> 
>>>>>> +	if (!allowed_types || allowed_types & (~IIO_ADC_CHAN_PROP_TYPE_ALL)) {
>>>>>
>>>>> Unneeded parentheses around negated value.
>>>>>
>>>>>> +	if (found_types & (~allowed_types)) {
>>>>>
>>>>> Ditto.
>>>>>
>>>>>> +		long unknown_types = found_types & (~allowed_types);
>>>>>
>>>>> Ditto and so on...
>>>>>

...

>>>> during the moves I lost my printed list of operator precedences which I used
>>>> to have on my desk. I've been writing C for 25 years or so, and I still
>>>> don't remember the precedence rules for all bitwise operations - and I am
>>>> fairly convinced I am not the only one.
>>>
>>> ~ (a.k.a. negation) is higher priority in bitops and it's idiomatic
>>> (at least in LK project).
>>
>> I know there are well established, accurate rules. Problem is that I never
>> remember these without looking.
> 
> There are very obvious cases like below.

I think we just disagree on if this is obvious.

>>>> What I understood is that I don't really have to have a printed list at
>>>> home, or go googling when away from home. I can just make it very, very
>>>> obvious :) Helps me a lot.
>>>
>>> Makes code harder to read, especially in the undoubtful cases like
>>>
>>> 	foo &= (~...);
>>
>> This is not undoubtful case for me :) And believe me, reading and
>> deciphering the
>>
>> foo &= (~bar);
>>
>> is _much_ faster than seeing:
> 
> Strongly disagree. One need to parse an additional pair of parentheses,
> and especially when it's a big statement inside with nested ones along
> with understanding what the heck is going on that you need them in the
> first place.
> 
> On top of that, we have a common practices in the LK project and
> with our history of communication it seems you are trying to do differently
> from time to time. Sounds like a rebellion to me :-)

I only rebel when I (in my opinion) have a solid reason :)

>> foo &= ~bar;
>>
>> and having to google the priorities.
> 
> Again, this is something a (regular) kernel developer keeps refreshed.
> Or even wider, C-language developer.

Ha. As I mentioned, I've been writing C on a daily bases for almost 25 
years. I wonder if you intent to say I am not a kernel/C-language 
developer? Bold claim.

>>>>>> +		int type;
>>>>>> +
>>>>>> +		for_each_set_bit(type, &unknown_types,
>>>>>> +				 IIO_ADC_CHAN_NUM_PROP_TYPES - 1) {
>>>>>> +			dev_err(dev, "Unsupported channel property %s\n",
>>>>>> +				iio_adc_type2prop(type));
>>>>>> +		}
>>>>>> +
>>>>>> +		return -EINVAL;
>>>>>> +	}
> 
> ...
> 
>>>>>> +		tmp.required &= (~BIT(IIO_ADC_CHAN_PROP_COMMON));
>>>>>
>>>>> Redundant outer parentheses. What's the point, please?
>>>>
>>>> Zero need to think of precedence.
>>>
>>> Huh? See above.
>>> Everything with equal sign is less precedence than normal ops.
>>
>> Sure. It's obvious if you remember that "Everything with equal sign is less
>> precedence than normal ops". But as I said, I truly have hard time
>> remembering these rules. When I try "going by memory" I end up having odd
>> errors and suggestions to add parenthesis from the compiler...
> 
> The hardest to remember probably the
> 
> 	foo && bar | baz
> 
> case and alike. These are the only ones that I totally agree on with you.
> But negation.
> 
>> By the way, do you know why anyone has bothered to add these
>> warnings/suggestions about adding the parenthesis to the compiler? My guess
>> is that I am not only one who needs the precedence charts ;)
> 
> Maybe someone programmed too much in LISP?.. (it's a rhetorical one)
> 
> ...
> 
>>>>>> +		ret = fwnode_property_read_u32(child, "common-mode-channel",
>>>>>> +					       &common);
>>>>>
>>>>> I believe this is okay to have on a single line,
>>>>
>>>> I try to keep things under 80 chars. It really truly helps me as I'd like to
>>>> have 3 parallel terminals open when writing code. Furthermore, I hate to
>>>> admit it but during the last two years my near vision has deteriorated... :/
>>>> 40 is getting more distant and 50 is approaching ;)
>>>
>>> It's only 86 altogether with better readability.
>>> We are in the second quarter of 21st century,
>>> the 80 should be left in 80s...
>>>
>>> (and yes, I deliberately put the above too short).
>>
>> I didn't even notice you had squeezed the lines :)
>>
>> But yeah, I truly have problems fitting even 3 80 column terminals on screen
>> with my current monitor. And when working on laptop screen it becomes
>> impossible. Hence I strongly prefer keeping the 80 chars limit.
> 
> Maybe you need a bigger monitor after all? (lurking now :-)

Wouldn't fit my table :)

> ...
> 
>>>>>> +#include <linux/iio/iio.h>
>>>>>
>>>>> I'm failing to see how this is being used in this header.
>>>>
>>>> I suppose it was the struct iio_chan_spec. Yep, forward declaration could
>>>> do, but I guess there would be no benefit because anyone using this header
>>>> is more than likely to use the iio.h as well.
>>>
>>> Still, it will be a beast to motivate people not thinking about what they are
>>> doing. I strongly prefer avoiding the use of the "proxy" or dangling headers.
>>
>> Ehh. There will be no IIO user who does not include the iio.h.
> 
> It's not your concern. That's the idea of making C units as much independent
> and modular as possible (with common sense in mind). And in this case I see
> no point of including this header. Again, the main problem is this will call
> people to use the new header as a "proxy" and that's what I fully against to.
> 
>> And, I need the iio_chan_spec here.
> 
> Do you really need it or is it just a pointer?

Just a pointer. Forward declaration could do it. Hmm. I didn't think of 
people using this header as a proxy. I guess you have a point here :)

> 
> ...
> 
>> And as I said, I suggest saving some of the energy for reviewing the next
>> version. I doubt the "property type" -flags and bitwise operations stay, and
>> it may be all of this will be just meld in the bd79124 code - depending on
>> what Jonathan & others think of it.
> 
> Whenever this code will be trying to land, the review comments still apply.

Sure! But chances are plenty of this code gets erased :) I just wanted 
to warn you that some of the effort on this version is likely to get 
wasted. I did consider reverting this back to a RFC - but going back'n 
forth with the RFC status felt odd...

Yours,
	-- Matti

Re: [PATCH v3 2/9] iio: adc: add helpers for parsing ADC nodes

[Andy Shevchenko]

On Thu, Feb 20, 2025 at 04:21:37PM +0200, Matti Vaittinen wrote:
> On 20/02/2025 16:04, Andy Shevchenko wrote:
> > On Thu, Feb 20, 2025 at 03:40:30PM +0200, Matti Vaittinen wrote:
> > > On 20/02/2025 14:41, Andy Shevchenko wrote:
> > > > On Thu, Feb 20, 2025 at 09:13:00AM +0200, Matti Vaittinen wrote:
> > > > > On 19/02/2025 22:41, Andy Shevchenko wrote:
> > > > > > On Wed, Feb 19, 2025 at 02:30:27PM +0200, Matti Vaittinen wrote:

...

> > > > > > > +EXPORT_SYMBOL_GPL(iio_adc_device_num_channels);
> > > > > > 
> > > > > > No namespace?
> > > > > 
> > > > > I was considering also this. The IIO core functions don't belong into a
> > > > > namespace - so I followed the convention to keep these similar to other IIO
> > > > > core stuff.
> > > > 
> > > > But it's historically. We have already started using namespaces
> > > > in the parts of IIO, haven't we?
> > > 
> > > Yes. But as I wrote, I don't think adding new namespaces for every helper
> > > file with a function or two exported will scale. We either need something
> > > common for IIO (or IIO "subsystems" like "adc", "accel", "light", ... ), or
> > > then we just keep these small helpers same as most of the IIO core.
> > 
> > It can be still pushed to IIO_CORE namespace. Do you see an issue with that?
> 
> No. I've missed the fact we have IIO_CORE O_o. Thanks for pointing it out!
> 
> > Or a new opaque namespace for the mentioned cases, something like IIO_HELPERS.
> 
> I am unsure if it really benefits to split this out of the IIO_CORE. I've a
> feeling it falls into the category of making things harder for user with no
> apparent reason. But yes, the IIO_CORE makes sense.

Probably I was not clear, I mean to put this under a given namespace. There is
no a such, we have currently:

IIO_BACKEND
IIO_DMA_BUFFER
IIO_DMAENGINE_BUFFER
IIO_GTS_HELPER
IIO_RESCALE

> > > > > (Sometimes I have a feeling that the trend today is to try make things
> > > > > intentionally difficult in the name of the safety. Like, "more difficult I
> > > > > make this, more experience points I gain in the name of the safety".)
> > > > > 
> > > > > Well, I suppose I could add a namespace for these functions - if this
> > > > > approach stays - but I'd really prefer having all IIO core stuff in some
> > > > > global IIO namespace and not to have dozens of fine-grained namespaces for
> > > > > an IIO driver to use...

...

> > > foo &= (~bar);
> > > 
> > > is _much_ faster than seeing:
> > 
> > Strongly disagree. One need to parse an additional pair of parentheses,
> > and especially when it's a big statement inside with nested ones along
> > with understanding what the heck is going on that you need them in the
> > first place.
> > 
> > On top of that, we have a common practices in the LK project and
> > with our history of communication it seems you are trying to do differently
> > from time to time. Sounds like a rebellion to me :-)
> 
> I only rebel when I (in my opinion) have a solid reason :)
> 
> > > foo &= ~bar;
> > > 
> > > and having to google the priorities.
> > 
> > Again, this is something a (regular) kernel developer keeps refreshed.
> > Or even wider, C-language developer.
> 
> Ha. As I mentioned, I've been writing C on a daily bases for almost 25
> years. I wonder if you intent to say I am not a kernel/C-language developer?
> Bold claim.

I'm just surprised by seeing that style from a 25y experienced C developer,
that's all.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 6/9] iio: adc: rzg2l_adc: Use adc-helpers

[kernel test robot]

Hi Matti,

kernel test robot noticed the following build errors:

[auto build test ERROR on 5bc55a333a2f7316b58edc7573e8e893f7acb532]

url:    https://github.com/intel-lab-lkp/linux/commits/Matti-Vaittinen/dt-bindings-ROHM-BD79124-ADC-GPO/20250219-203748
base:   5bc55a333a2f7316b58edc7573e8e893f7acb532
patch link:    https://lore.kernel.org/r/25c5d22f6f0cbd1355eee2e9d9103c3ee71cebdc.1739967040.git.mazziesaccount%40gmail.com
patch subject: [PATCH v3 6/9] iio: adc: rzg2l_adc: Use adc-helpers
config: i386-buildonly-randconfig-005-20250220 (https://download.01.org/0day-ci/archive/20250220/202502202332.jrEGhoBM-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/20250220/202502202332.jrEGhoBM-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/202502202332.jrEGhoBM-lkp@intel.com/

All errors (new ones prefixed by >>):

>> ld.lld: error: undefined symbol: devm_iio_adc_device_alloc_chaninfo
   >>> referenced by rzg2l_adc.c:324 (drivers/iio/adc/rzg2l_adc.c:324)
   >>>               drivers/iio/adc/rzg2l_adc.o:(rzg2l_adc_probe) in archive vmlinux.a

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

Re: [PATCH v3 7/9] iio: adc: sun20i-gpadc: Use adc-helpers

[kernel test robot]

Hi Matti,

kernel test robot noticed the following build errors:

[auto build test ERROR on 5bc55a333a2f7316b58edc7573e8e893f7acb532]

url:    https://github.com/intel-lab-lkp/linux/commits/Matti-Vaittinen/dt-bindings-ROHM-BD79124-ADC-GPO/20250219-203748
base:   5bc55a333a2f7316b58edc7573e8e893f7acb532
patch link:    https://lore.kernel.org/r/21b9af362b64a1d9c2a13cc46242dd6955996c46.1739967040.git.mazziesaccount%40gmail.com
patch subject: [PATCH v3 7/9] iio: adc: sun20i-gpadc: Use adc-helpers
config: i386-buildonly-randconfig-002-20250220 (https://download.01.org/0day-ci/archive/20250221/202502210037.162FN3PM-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250221/202502210037.162FN3PM-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/202502210037.162FN3PM-lkp@intel.com/

All errors (new ones prefixed by >>, old ones prefixed by <<):

>> ERROR: modpost: "devm_iio_adc_device_alloc_chaninfo" [drivers/iio/adc/sun20i-gpadc-iio.ko] undefined!

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

Re: [PATCH v3 2/9] iio: adc: add helpers for parsing ADC nodes

[Matti Vaittinen]

On 20/02/2025 16:56, Andy Shevchenko wrote:
> On Thu, Feb 20, 2025 at 04:21:37PM +0200, Matti Vaittinen wrote:
>> On 20/02/2025 16:04, Andy Shevchenko wrote:
>>> On Thu, Feb 20, 2025 at 03:40:30PM +0200, Matti Vaittinen wrote:
>>>> On 20/02/2025 14:41, Andy Shevchenko wrote:
>>>>> On Thu, Feb 20, 2025 at 09:13:00AM +0200, Matti Vaittinen wrote:
>>>>>> On 19/02/2025 22:41, Andy Shevchenko wrote:
>>>>>>> On Wed, Feb 19, 2025 at 02:30:27PM +0200, Matti Vaittinen wrote:
> 
> ...
> 
>>>>>>>> +EXPORT_SYMBOL_GPL(iio_adc_device_num_channels);
>>>>>>>
>>>>>>> No namespace?
>>>>>>
>>>>>> I was considering also this. The IIO core functions don't belong into a
>>>>>> namespace - so I followed the convention to keep these similar to other IIO
>>>>>> core stuff.
>>>>>
>>>>> But it's historically. We have already started using namespaces
>>>>> in the parts of IIO, haven't we?
>>>>
>>>> Yes. But as I wrote, I don't think adding new namespaces for every helper
>>>> file with a function or two exported will scale. We either need something
>>>> common for IIO (or IIO "subsystems" like "adc", "accel", "light", ... ), or
>>>> then we just keep these small helpers same as most of the IIO core.
>>>
>>> It can be still pushed to IIO_CORE namespace. Do you see an issue with that?
>>
>> No. I've missed the fact we have IIO_CORE O_o. Thanks for pointing it out!
>>
>>> Or a new opaque namespace for the mentioned cases, something like IIO_HELPERS.
>>
>> I am unsure if it really benefits to split this out of the IIO_CORE. I've a
>> feeling it falls into the category of making things harder for user with no
>> apparent reason. But yes, the IIO_CORE makes sense.
> 
> Probably I was not clear, I mean to put this under a given namespace. There is
> no a such, we have currently:
> 
> IIO_BACKEND
> IIO_DMA_BUFFER
> IIO_DMAENGINE_BUFFER
> IIO_GTS_HELPER
> IIO_RESCALE

Ah. So, the IIO core stuff is still not in a namespace. Those listed 
above are all too specific (I believe, in general, and definitely to 
carry ADC helpers).

Adding 'ADC_HELPERS' would just add yet another way too specific one. 
So, currently there is no suitable namespace for these helpers, and I 
still believe they fit best to where the rest of the IIO-core stuff is.

If we want really play the namespace game, then the existing IIO stuff 
should be put in a IIO_CORE-namespace instead of creating more new small 
ones. I am afraid that adding all existing IIO core to a IIO_CORE 
namespace and converting all existing users to use the IIO_CORE is not a 
reasonable request for a person trying to:

1. Write a driver
2. Add a small helper to aid others (instead of just melding it all in 
the given new driver - which does not benefit anyone else and just leads 
to code duplication in the long run...)

>>>>>> (Sometimes I have a feeling that the trend today is to try make things
>>>>>> intentionally difficult in the name of the safety. Like, "more difficult I
>>>>>> make this, more experience points I gain in the name of the safety".)
>>>>>>
>>>>>> Well, I suppose I could add a namespace for these functions - if this
>>>>>> approach stays - but I'd really prefer having all IIO core stuff in some
>>>>>> global IIO namespace and not to have dozens of fine-grained namespaces for
>>>>>> an IIO driver to use...
> 
> ...
> 
>>>> foo &= (~bar);
>>>>
>>>> is _much_ faster than seeing:
>>>
>>> Strongly disagree. One need to parse an additional pair of parentheses,
>>> and especially when it's a big statement inside with nested ones along
>>> with understanding what the heck is going on that you need them in the
>>> first place.
>>>
>>> On top of that, we have a common practices in the LK project and
>>> with our history of communication it seems you are trying to do differently
>>> from time to time. Sounds like a rebellion to me :-)
>>
>> I only rebel when I (in my opinion) have a solid reason :)
>>
>>>> foo &= ~bar;
>>>>
>>>> and having to google the priorities.
>>>
>>> Again, this is something a (regular) kernel developer keeps refreshed.
>>> Or even wider, C-language developer.
>>
>> Ha. As I mentioned, I've been writing C on a daily bases for almost 25
>> years. I wonder if you intent to say I am not a kernel/C-language developer?
>> Bold claim.
> 
> I'm just surprised by seeing that style from a 25y experienced C developer,
> that's all.

I am not. If something, these 25 years have taught me to understand that 
even if something is simple and obvious to me, it may not be simple and 
obvious to someone else. Similarly, something obvious to someone else, 
is not obvious to me. Hence, I am very careful when telling people that:

 >>> Again, this is something a (regular) kernel developer keeps refreshed.
 >>> Or even wider, C-language developer.

I may however say that "this is something _I_ keep refreshed (as a 
kernel/C-developer)".

As an example,

 >>>> foo &= (~bar);

This is something _I_ find very clear and exact, with zero doubt if 
negation is applied before &=. For _me_ the parenthesis there _help_, 
and for _me_ the parenthesis cause no confusion when reading the code.

I won't go and tell that I'd expect any C or kernel developer to be able 
to fluently parse "foo &= (~bar)". (Whether I think they should is 
another matter).

Oh well, let's wait and see what Jonathan thinks of these helpers in 
general. We can continue the parenthesis discussion when we know whether 
the code is going to stay.

Yours,
	-- Matti

Re: [PATCH v3 2/9] iio: adc: add helpers for parsing ADC nodes

[Andy Shevchenko]

On Fri, Feb 21, 2025 at 12:10:23PM +0200, Matti Vaittinen wrote:
> On 20/02/2025 16:56, Andy Shevchenko wrote:
> > On Thu, Feb 20, 2025 at 04:21:37PM +0200, Matti Vaittinen wrote:
> > > On 20/02/2025 16:04, Andy Shevchenko wrote:
> > > > On Thu, Feb 20, 2025 at 03:40:30PM +0200, Matti Vaittinen wrote:
> > > > > On 20/02/2025 14:41, Andy Shevchenko wrote:
> > > > > > On Thu, Feb 20, 2025 at 09:13:00AM +0200, Matti Vaittinen wrote:
> > > > > > > On 19/02/2025 22:41, Andy Shevchenko wrote:
> > > > > > > > On Wed, Feb 19, 2025 at 02:30:27PM +0200, Matti Vaittinen wrote:

...

> > > > > > > > > +EXPORT_SYMBOL_GPL(iio_adc_device_num_channels);
> > > > > > > > 
> > > > > > > > No namespace?
> > > > > > > 
> > > > > > > I was considering also this. The IIO core functions don't belong into a
> > > > > > > namespace - so I followed the convention to keep these similar to other IIO
> > > > > > > core stuff.
> > > > > > 
> > > > > > But it's historically. We have already started using namespaces
> > > > > > in the parts of IIO, haven't we?
> > > > > 
> > > > > Yes. But as I wrote, I don't think adding new namespaces for every helper
> > > > > file with a function or two exported will scale. We either need something
> > > > > common for IIO (or IIO "subsystems" like "adc", "accel", "light", ... ), or
> > > > > then we just keep these small helpers same as most of the IIO core.
> > > > 
> > > > It can be still pushed to IIO_CORE namespace. Do you see an issue with that?
> > > 
> > > No. I've missed the fact we have IIO_CORE O_o. Thanks for pointing it out!
> > > 
> > > > Or a new opaque namespace for the mentioned cases, something like IIO_HELPERS.
> > > 
> > > I am unsure if it really benefits to split this out of the IIO_CORE. I've a
> > > feeling it falls into the category of making things harder for user with no
> > > apparent reason. But yes, the IIO_CORE makes sense.
> > 
> > Probably I was not clear, I mean to put this under a given namespace. There is
> > no a such, we have currently:
> > 
> > IIO_BACKEND
> > IIO_DMA_BUFFER
> > IIO_DMAENGINE_BUFFER
> > IIO_GTS_HELPER
> > IIO_RESCALE
> 
> Ah. So, the IIO core stuff is still not in a namespace. Those listed above
> are all too specific (I believe, in general, and definitely to carry ADC
> helpers).
> 
> Adding 'ADC_HELPERS' would just add yet another way too specific one. So,
> currently there is no suitable namespace for these helpers, and I still
> believe they fit best to where the rest of the IIO-core stuff is.
> 
> If we want really play the namespace game, then the existing IIO stuff
> should be put in a IIO_CORE-namespace instead of creating more new small
> ones. I am afraid that adding all existing IIO core to a IIO_CORE namespace
> and converting all existing users to use the IIO_CORE is not a reasonable
> request for a person trying to:
> 
> 1. Write a driver
> 2. Add a small helper to aid others (instead of just melding it all in the
> given new driver - which does not benefit anyone else and just leads to code
> duplication in the long run...)

That's why more specific, but also a bit general might work, like IIO_HELPERS,
considering that they may be used by many drivers.

While it may be not your call, somebody should do the job. Jonathan? :-)

> > > > > > > (Sometimes I have a feeling that the trend today is to try make things
> > > > > > > intentionally difficult in the name of the safety. Like, "more difficult I
> > > > > > > make this, more experience points I gain in the name of the safety".)
> > > > > > > 
> > > > > > > Well, I suppose I could add a namespace for these functions - if this
> > > > > > > approach stays - but I'd really prefer having all IIO core stuff in some
> > > > > > > global IIO namespace and not to have dozens of fine-grained namespaces for
> > > > > > > an IIO driver to use...

...

> > > > > foo &= (~bar);
> > > > > 
> > > > > is _much_ faster than seeing:
> > > > 
> > > > Strongly disagree. One need to parse an additional pair of parentheses,
> > > > and especially when it's a big statement inside with nested ones along
> > > > with understanding what the heck is going on that you need them in the
> > > > first place.
> > > > 
> > > > On top of that, we have a common practices in the LK project and
> > > > with our history of communication it seems you are trying to do differently
> > > > from time to time. Sounds like a rebellion to me :-)
> > > 
> > > I only rebel when I (in my opinion) have a solid reason :)
> > > 
> > > > > foo &= ~bar;
> > > > > 
> > > > > and having to google the priorities.
> > > > 
> > > > Again, this is something a (regular) kernel developer keeps refreshed.
> > > > Or even wider, C-language developer.
> > > 
> > > Ha. As I mentioned, I've been writing C on a daily bases for almost 25
> > > years. I wonder if you intent to say I am not a kernel/C-language developer?
> > > Bold claim.
> > 
> > I'm just surprised by seeing that style from a 25y experienced C developer,
> > that's all.
> 
> I am not. If something, these 25 years have taught me to understand that
> even if something is simple and obvious to me, it may not be simple and
> obvious to someone else. Similarly, something obvious to someone else, is
> not obvious to me. Hence, I am very careful when telling people that:
> 
> >>> Again, this is something a (regular) kernel developer keeps refreshed.
> >>> Or even wider, C-language developer.
> 
> I may however say that "this is something _I_ keep refreshed (as a
> kernel/C-developer)".

True.

> As an example,
> 
> >>>> foo &= (~bar);
> 
> This is something _I_ find very clear and exact, with zero doubt if negation
> is applied before &=. For _me_ the parenthesis there _help_, and for _me_
> the parenthesis cause no confusion when reading the code.
> 
> I won't go and tell that I'd expect any C or kernel developer to be able to
> fluently parse "foo &= (~bar)". (Whether I think they should is another
> matter).

> Oh well, let's wait and see what Jonathan thinks of these helpers in
> general. We can continue the parenthesis discussion when we know whether the
> code is going to stay.

Sure, but it's not only about these helpers, it's about the style in general.
Spreading unneeded characters in the code seems to me as an attempt to put
_your_ rules over the subsytem's ones. Whatever, let's Jonathan to judge, we
will never agree on a keep growing list of things anyway...

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 2/9] iio: adc: add helpers for parsing ADC nodes

[Matti Vaittinen]

On 21/02/2025 18:41, Andy Shevchenko wrote:
> 
> Sure, but it's not only about these helpers, it's about the style in general.
> Spreading unneeded characters in the code seems to me as an attempt to put
> _your_ rules over the subsytem's ones. Whatever, let's Jonathan to judge, we
> will never agree on a keep growing list of things anyway...
> 

Hey, let's look at the bright side - there has been also a number of 
things we have agreed :) Besides, even if I don't agree with every thing 
you bring-up, I do still appreciate your reviews! I do think many of 
your comments have indeed improved the code. We may never agree on every 
point, but I am positive we can _understand_ each others stance. I do 
appreciate hearing your view on things.

Yours,
	-- Matti.

Re: [PATCH v3 2/9] iio: adc: add helpers for parsing ADC nodes

[Jonathan Cameron]

On Fri, 21 Feb 2025 18:41:05 +0200
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Fri, Feb 21, 2025 at 12:10:23PM +0200, Matti Vaittinen wrote:
> > On 20/02/2025 16:56, Andy Shevchenko wrote:  
> > > On Thu, Feb 20, 2025 at 04:21:37PM +0200, Matti Vaittinen wrote:  
> > > > On 20/02/2025 16:04, Andy Shevchenko wrote:  
> > > > > On Thu, Feb 20, 2025 at 03:40:30PM +0200, Matti Vaittinen wrote:  
> > > > > > On 20/02/2025 14:41, Andy Shevchenko wrote:  
> > > > > > > On Thu, Feb 20, 2025 at 09:13:00AM +0200, Matti Vaittinen wrote:  
> > > > > > > > On 19/02/2025 22:41, Andy Shevchenko wrote:  
> > > > > > > > > On Wed, Feb 19, 2025 at 02:30:27PM +0200, Matti Vaittinen wrote:  
> 
> ...
> 
> > > > > > > > > > +EXPORT_SYMBOL_GPL(iio_adc_device_num_channels);  
> > > > > > > > > 
> > > > > > > > > No namespace?  
> > > > > > > > 
> > > > > > > > I was considering also this. The IIO core functions don't belong into a
> > > > > > > > namespace - so I followed the convention to keep these similar to other IIO
> > > > > > > > core stuff.  
> > > > > > > 
> > > > > > > But it's historically. We have already started using namespaces
> > > > > > > in the parts of IIO, haven't we?  
> > > > > > 
> > > > > > Yes. But as I wrote, I don't think adding new namespaces for every helper
> > > > > > file with a function or two exported will scale. We either need something
> > > > > > common for IIO (or IIO "subsystems" like "adc", "accel", "light", ... ), or
> > > > > > then we just keep these small helpers same as most of the IIO core.  
> > > > > 
> > > > > It can be still pushed to IIO_CORE namespace. Do you see an issue with that?  
> > > > 
> > > > No. I've missed the fact we have IIO_CORE O_o. Thanks for pointing it out!
> > > >   
> > > > > Or a new opaque namespace for the mentioned cases, something like IIO_HELPERS.  
> > > > 
> > > > I am unsure if it really benefits to split this out of the IIO_CORE. I've a
> > > > feeling it falls into the category of making things harder for user with no
> > > > apparent reason. But yes, the IIO_CORE makes sense.  
> > > 
> > > Probably I was not clear, I mean to put this under a given namespace. There is
> > > no a such, we have currently:
> > > 
> > > IIO_BACKEND
> > > IIO_DMA_BUFFER
> > > IIO_DMAENGINE_BUFFER
> > > IIO_GTS_HELPER
> > > IIO_RESCALE  
> > 
> > Ah. So, the IIO core stuff is still not in a namespace. Those listed above
> > are all too specific (I believe, in general, and definitely to carry ADC
> > helpers).

Not yet. On todo list... Trick is working out what the correct break up is.

> > 
> > Adding 'ADC_HELPERS' would just add yet another way too specific one. So,
> > currently there is no suitable namespace for these helpers, and I still
> > believe they fit best to where the rest of the IIO-core stuff is.

Just add an IIO namespace.  That would be the one I'd expect a typical
driver to use.  We can move things into it later. The ones above are
more obscure functionality.  Avoid the IIO_CORE naming as I'd expect
that to be stuff the core alone should call and we shouldn't see in drivers.

> > 
> > If we want really play the namespace game, then the existing IIO stuff
> > should be put in a IIO_CORE-namespace instead of creating more new small
> > ones. I am afraid that adding all existing IIO core to a IIO_CORE namespace
> > and converting all existing users to use the IIO_CORE is not a reasonable
> > request for a person trying to:
> > 
> > 1. Write a driver
> > 2. Add a small helper to aid others (instead of just melding it all in the
> > given new driver - which does not benefit anyone else and just leads to code
> > duplication in the long run...)  
> 
> That's why more specific, but also a bit general might work, like IIO_HELPERS,
> considering that they may be used by many drivers.
> 
> While it may be not your call, somebody should do the job. Jonathan? :-)

It's on the list.  Not that near the top of it, but there to do at somepoint.

> 
> > > > > > > > (Sometimes I have a feeling that the trend today is to try make things
> > > > > > > > intentionally difficult in the name of the safety. Like, "more difficult I
> > > > > > > > make this, more experience points I gain in the name of the safety".)
> > > > > > > > 
> > > > > > > > Well, I suppose I could add a namespace for these functions - if this
> > > > > > > > approach stays - but I'd really prefer having all IIO core stuff in some
> > > > > > > > global IIO namespace and not to have dozens of fine-grained namespaces for
> > > > > > > > an IIO driver to use...  
> 
> ...
> 
> > > > > > foo &= (~bar);
> > > > > > 
> > > > > > is _much_ faster than seeing:  
> > > > > 
> > > > > Strongly disagree. One need to parse an additional pair of parentheses,
> > > > > and especially when it's a big statement inside with nested ones along
> > > > > with understanding what the heck is going on that you need them in the
> > > > > first place.
> > > > > 
> > > > > On top of that, we have a common practices in the LK project and
> > > > > with our history of communication it seems you are trying to do differently
> > > > > from time to time. Sounds like a rebellion to me :-)  
> > > > 
> > > > I only rebel when I (in my opinion) have a solid reason :)
> > > >   
> > > > > > foo &= ~bar;
> > > > > > 
> > > > > > and having to google the priorities.  
> > > > > 
> > > > > Again, this is something a (regular) kernel developer keeps refreshed.
> > > > > Or even wider, C-language developer.  
> > > > 
> > > > Ha. As I mentioned, I've been writing C on a daily bases for almost 25
> > > > years. I wonder if you intent to say I am not a kernel/C-language developer?
> > > > Bold claim.  
> > > 
> > > I'm just surprised by seeing that style from a 25y experienced C developer,
> > > that's all.  
> > 
> > I am not. If something, these 25 years have taught me to understand that
> > even if something is simple and obvious to me, it may not be simple and
> > obvious to someone else. Similarly, something obvious to someone else, is
> > not obvious to me. Hence, I am very careful when telling people that:
> >   
> > >>> Again, this is something a (regular) kernel developer keeps refreshed.
> > >>> Or even wider, C-language developer.  
> > 
> > I may however say that "this is something _I_ keep refreshed (as a
> > kernel/C-developer)".  
> 
> True.
> 
> > As an example,
> >   
> > >>>> foo &= (~bar);  
> > 
> > This is something _I_ find very clear and exact, with zero doubt if negation
> > is applied before &=. For _me_ the parenthesis there _help_, and for _me_
> > the parenthesis cause no confusion when reading the code.
> > 
> > I won't go and tell that I'd expect any C or kernel developer to be able to
> > fluently parse "foo &= (~bar)". (Whether I think they should is another
> > matter).  
> 
> > Oh well, let's wait and see what Jonathan thinks of these helpers in
> > general. We can continue the parenthesis discussion when we know whether the
> > code is going to stay.  
> 
> Sure, but it's not only about these helpers, it's about the style in general.
> Spreading unneeded characters in the code seems to me as an attempt to put
> _your_ rules over the subsytem's ones. Whatever, let's Jonathan to judge, we
> will never agree on a keep growing list of things anyway...
> 
I need to find some time to consider it a bit more and look at other users.
I'm not keen on this being 'general' if we only have one user in the short
term.

Jonathan

Re: [PATCH v3 2/9] iio: adc: add helpers for parsing ADC nodes

[Jonathan Cameron]

On Wed, 19 Feb 2025 14:30:27 +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:
> v2 => v3: Mostly based on review comments by Jonathan
>  - Support differential and single-ended channels(*)
>  - Rename iio_adc_device_get_channels() as
>  - Improve spelling
>  - Drop support for cases where DT comes from parent device's node
>  - Decrease loop indent by reverting node name check conditions
>  - Don't set 'chan->indexed' by number of channels to keep the
>    interface consistent no matter how many channels are connected.
>  - Fix ID range check and related comment
> RFC v1 => v2:
>  - New patch
> 
> (*) The support for single-ended and differential channels is 100%
> untested. I am not convinced it is actually an improvement over the
> *_simple() helpers which only supported getting the ID from the "reg

Currently it definitely feels too complex.  Partly, whilst I haven't
tried fleshing out the alternative, it feels like you've tried to make
it too general.  I really don't like the allowed bitmap as those
relationships are complex.

> In theory they could be used. In practice, while I skimmed through the
> in-tree ADC drivers which used the for_each_child_node() construct - it
> seemed that most of those which supported differential inputs had also
> some other per-channel properties to read. Those users would in any case
> need to loop through the nodes to get those other properties.
That doesn't surprise me that much. I'm still not sure there are enough
'simple' cases (i.e. more than maybe 3) to justify this being shared.

> 
> If I am once more allowed to go back to proposing the _simple() variant
> which only covers the case: "chan ID in 'reg' property"... Dropping
> support for differential and single-ended channels would simplify this
> helper a lot. It'd allow dropping the sanity check as well as the extra
> parameters callers need to pass to tell what kind of properties they
> expect. That'd (in my opinion) made the last patches (to actual ADC
> drivers) in this series a much more lean and worthy ;)

If you do, call it _se() or something like that.

> 
> Finally, we could add own functions for differential/single-ended/all
> channels when the next driver which uses differential or single-ended
> channels - and which does not need other per-channel properties - lands
> in tree. That would still simplify the helper API usage for those
> drivers touched at the end of this series.

This sounds reasonable approach but I still want more than one before
we bother.

> 
> I (still) 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.
Definitely need usecases.

So my current feeling is if you can find a second case that a
_se() variant works for then I don't mind this being a separate module.
If not put just what you need in your driver.

Various comments inline.
> ---
>  drivers/iio/adc/Kconfig            |   3 +
>  drivers/iio/adc/Makefile           |   1 +
>  drivers/iio/adc/industrialio-adc.c | 304 +++++++++++++++++++++++++++++
>  include/linux/iio/adc-helpers.h    |  56 ++++++
>  4 files changed, 364 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
As per other discussion. Move this to new block at the top.
>  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..0281d64ae112
> --- /dev/null
> +++ b/drivers/iio/adc/industrialio-adc.c
> @@ -0,0 +1,304 @@
> +// 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_device_num_channels(struct device *dev)
> +{
> +	int num_chan = 0;
> +
> +	device_for_each_child_node_scoped(dev, child)
> +		if (fwnode_name_eq(child, "channel"))
> +			num_chan++;
> +
> +	return num_chan;

This function seems easy to generalize to count nodes of particular
name.  So I'd promote this as a generic property.h helper and
just use that in here.


> +}
> +EXPORT_SYMBOL_GPL(iio_adc_device_num_channels);
> +
> +static const char *iio_adc_type2prop(int type)
> +{
> +	switch (type) {
> +	case IIO_ADC_CHAN_PROP_TYPE_REG:
> +		return "reg";
> +	case IIO_ADC_CHAN_PROP_TYPE_SINGLE_ENDED:
> +		return "single-channel";
> +	case IIO_ADC_CHAN_PROP_TYPE_DIFF:
> +		return "diff-channels";
> +	case IIO_ADC_CHAN_PROP_COMMON:
> +		return "common-mode-channel";
> +	default:
> +		return "unknown";
> +	}
> +}
> +
> +/*
> + * Sanity check. Ensure that:
> + * - At least some type(s) are allowed
> + * - All types found are also expected
> + * - If plain "reg" is not allowed, either single-ended or differential
> + *   properties are found.

I'd worry this is a combination of fragile and overly separate from
the parser.  I'd just encode this stuff down there based on accepted type
of channels.  Two flags, differential and single ended may be enough.
If single only then reg is expected solution but I don't see a reason to
ignore single-channel even then as meaning is well defined.
If differential only - then that property must be present for all channels.
If both single and differential then need either differential or single-channel.

> + */
> +static int iio_adc_prop_type_check_sanity(struct device *dev,
> +		const struct iio_adc_props *expected_props, int found_types)
> +{
> +	unsigned long allowed_types = expected_props->allowed |
> +				      expected_props->required;
> +
> +	if (!allowed_types || allowed_types & (~IIO_ADC_CHAN_PROP_TYPE_ALL)) {
> +		dev_dbg(dev, "Invalid adc allowed prop types 0x%lx\n",
> +			allowed_types);
> +
> +		return -EINVAL;
> +	}
> +	if (found_types & (~allowed_types)) {
> +		long unknown_types = found_types & (~allowed_types);
> +		int type;
> +
> +		for_each_set_bit(type, &unknown_types,
> +				 IIO_ADC_CHAN_NUM_PROP_TYPES - 1) {
> +			dev_err(dev, "Unsupported channel property %s\n",
> +				iio_adc_type2prop(type));
> +		}
> +
> +		return -EINVAL;
> +	}
> +
> +	/*
> +	 * The IIO_ADC_CHAN_PROP_TYPE_REG is special. We always require it to
> +	 * be found in the dt. (If not, we'll error out before calling this
> +	 * function.) However, listing it in 'allowed' types means the "reg"
> +	 * alone can be used to indicate the channel ID.
> +	 *
> +	 * Thus, we don't add it in the found properties either - so check for
> +	 * found and allowed properties passes even if user hasn't explicitly
> +	 * added the 'IIO_ADC_CHAN_PROP_TYPE_REG' to be allowed. (This is the
> +	 * case if either differential or single-ended property is required).
> +	 *
> +	 * Hence, for this check we need to explicitly add the
> +	 * IIO_ADC_CHAN_PROP_TYPE_REG to 'found' properties to make the check
> +	 * pass when "reg" is the property which is required to have the
> +	 * channel ID.
> +	 *
> +	 * We could of course always add the IIO_ADC_CHAN_PROP_TYPE_REG in
> +	 * allowed types and found types - but then we wouldn't catch the case
> +	 * where user says the "reg" alone is not sufficient.
> +	 */
> +	if ((~(found_types | IIO_ADC_CHAN_PROP_TYPE_REG)) & expected_props->required) {
> +		long missing_types;
> +		int type;
> +
> +		missing_types = (~found_types) & expected_props->required;
> +
> +		for_each_set_bit(type, &missing_types,
> +				 IIO_ADC_CHAN_NUM_PROP_TYPES - 1) {
> +			dev_err(dev, "required channel specifier '%s' not found\n",
> +				iio_adc_type2prop(type));
> +		}
> +
> +		return -EINVAL;
> +	}
> +
> +	/* Check if we require something else but the "reg" property */
> +	if (!(allowed_types & IIO_ADC_CHAN_PROP_TYPE_REG)) {
> +		if (found_types & IIO_ADC_CHAN_PROP_TYPE_SINGLE_ENDED ||
> +				found_types & IIO_ADC_CHAN_PROP_TYPE_DIFF)
> +			return 0;
> +
> +		dev_err(dev, "channel specifier not found\n");
> +
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * iio_adc_device_channels_by_property - get ADC channel IDs
> + *
> + * Scan the device node for ADC channel information. Return an array of found
> + * IDs. Caller needs to provide the memory for the array and provide maximum
> + * number of IDs the array can store.

I'm somewhat confused by this. Feels like you can get same info from the
iio_chan_spec array generated by the next function.

> + *
> + * @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.
> + * @expected_props:	Bitmaps of channel property types (for checking).
> + *
> + * Return:		Number of found channels on succes. 0 if no channels
> + *			was found. Negative value to indicate failure.
> + */
> +int iio_adc_device_channels_by_property(struct device *dev, int *channels,
> +		int max_channels, const struct iio_adc_props *expected_props)
> +{
> +	int num_chan = 0, ret;
> +
> +	device_for_each_child_node_scoped(dev, child) {
> +		u32 ch, diff[2], se;
> +		struct iio_adc_props tmp;
> +		int chtypes_found = 0;
> +
> +		if (!fwnode_name_eq(child, "channel"))
> +			continue;
> +
> +		if (num_chan == max_channels)
> +			return -EINVAL;
> +
> +		ret = fwnode_property_read_u32(child, "reg", &ch);
> +		if (ret)
> +			return ret;
> +
> +		ret = fwnode_property_read_u32_array(child, "diff-channels",
> +						     &diff[0], 2);
> +		if (!ret)
> +			chtypes_found |= IIO_ADC_CHAN_PROP_TYPE_DIFF;
> +
> +		ret = fwnode_property_read_u32(child, "single-channel", &se);
> +		if (!ret)
> +			chtypes_found |= IIO_ADC_CHAN_PROP_TYPE_SINGLE_ENDED;
> +
> +		tmp = *expected_props;
> +		/*
> +		 * We don't bother reading the "common-mode-channel" here as it
> +		 * doesn't really affect on the primary channel ID. We remove
> +		 * it from the required properties to allow the sanity check
> +		 * pass here  also for drivers which require it.
> +		 */
> +		tmp.required &= (~BIT(IIO_ADC_CHAN_PROP_COMMON));
> +
> +		ret = iio_adc_prop_type_check_sanity(dev, &tmp, chtypes_found);
> +		if (ret)
> +			return ret;
> +
> +		if (chtypes_found & IIO_ADC_CHAN_PROP_TYPE_DIFF)
> +			ch = diff[0];
> +		else if (chtypes_found & IIO_ADC_CHAN_PROP_TYPE_SINGLE_ENDED)
> +			ch = se;
> +
> +		/*
> +		 * 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_channels_by_property);
> +
> +/**
> + * devm_iio_adc_device_alloc_chaninfo - allocate and fill iio_chan_spec for adc

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
> + * @expected_props:	Bitmaps of channel property types (for checking).
Input parameter so should be after template.

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

I wonder if an alloc function would be better returning the pointer and
providing num_chans via a parameter.

> + */
> +int devm_iio_adc_device_alloc_chaninfo(struct device *dev,
> +				const struct iio_chan_spec *template,
> +				struct iio_chan_spec **cs,
> +				const struct iio_adc_props *expected_props)

I'm not sure this expected props thing works as often it's a case
of complex relationships 

> +{
> +	struct iio_chan_spec *chan;
> +	int num_chan = 0, ret;
Initialized just after this so don't set num_chan = 0.

> +
> +	num_chan = iio_adc_device_num_channels(dev);
> +	if (num_chan < 1)
> +		return num_chan;
> +
> +	*cs = devm_kcalloc(dev, num_chan, sizeof(**cs), GFP_KERNEL);
> +	if (!*cs)
> +		return -ENOMEM;
> +
> +	chan = &(*cs)[0];
> +
> +	device_for_each_child_node_scoped(dev, child) {
> +		u32 ch, diff[2], se, common;
> +		int chtypes_found = 0;
> +
> +		if (!fwnode_name_eq(child, "channel"))
> +			continue;
> +
> +		ret = fwnode_property_read_u32(child, "reg", &ch);
> +		if (ret)
> +			return ret;
> +

diff channels and single channel take precedence over reg, so check them
first. If present no need to look for reg can also read it then throw
it away giving simpler.

		*chan = *template;

		// reg should exist either way.
		ret = fwnode_property_read_u32(child, "reg", &reg);
		if (ret)
			return -EINVAL; //should be a reg whatever.

		ret = fwnode_property_read_u32_array(child, "diff-channels",
						     diff, ARRAY_SIZE(diff));
		if (ret == 0) {
			chan->differential = 1;
			chan->channel = diff[0];
			chan->channel2 = diff[1];
		} else {
			ret = fwnode_property_read_u32(child, "single-channel", &se);
			if (ret)
				se = reg;

			chan->channel = se;
			//IIRC common mode channel is rare. I'd skip it. That
			//also makes it a differential channel be it a weird one.
		}
					

> +		ret = fwnode_property_read_u32_array(child, "diff-channels",
> +						     &diff[0], 2);
> +		if (!ret)
> +			chtypes_found |= IIO_ADC_CHAN_PROP_TYPE_DIFF;
> +
> +		ret = fwnode_property_read_u32(child, "single-channel", &se);
> +		if (!ret)
> +			chtypes_found |= IIO_ADC_CHAN_PROP_TYPE_SINGLE_ENDED;
> +
> +		ret = fwnode_property_read_u32(child, "common-mode-channel",
> +					       &common);
> +		if (!ret)
> +			chtypes_found |= BIT(IIO_ADC_CHAN_PROP_COMMON);
> +
> +		ret = iio_adc_prop_type_check_sanity(dev, expected_props,
> +						     chtypes_found);

If we want to verify this (I'm not yet sure) then do it as you parse the
properties, not separately.

> +		if (ret)
> +			return ret;
> +
> +		*chan = *template;
> +		chan->channel = ch;
> +
> +		if (chtypes_found & IIO_ADC_CHAN_PROP_TYPE_DIFF) {
> +			chan->differential = 1;
> +			chan->channel = diff[0];
> +			chan->channel2 = diff[1];
> +
> +		} else if (chtypes_found & IIO_ADC_CHAN_PROP_TYPE_SINGLE_ENDED) {
> +			chan->channel = se;
> +			if (chtypes_found & BIT(IIO_ADC_CHAN_PROP_COMMON))
> +				chan->channel2 = common;
> +		}
> +
> +		/*
> +		 * We assume the channel IDs start from 0. If it seems this is
> +		 * not a sane assumption, then we have to add 'allowed ID ranges'
> +		 * to the struct iio_adc_props because some of the callers may
> +		 * rely on the IDs being in this range - and have arrays indexed
> +		 * by the ID.

Not a requirement in general.  It is more than possible to have a single channel
provided that is number 7.

> +		 */
> +		if (chan->channel >= num_chan)
> +			return -ERANGE;
> +
> +		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..f7791d45dbd2
> --- /dev/null
> +++ b/include/linux/iio/adc-helpers.h
> @@ -0,0 +1,56 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +
> +/* The industrial I/O ADC helpers
Comment syntax and make it more obvious that while this might be useful
it isn't something we necessarily expect to be useful to every driver.
/*
 * Industrial I/O ADC firmware property passing 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;
> +
> +enum {
> +	IIO_ADC_CHAN_PROP_REG,
> +	IIO_ADC_CHAN_PROP_SINGLE_ENDED,
> +	IIO_ADC_CHAN_PROP_DIFF,
> +	IIO_ADC_CHAN_PROP_COMMON,
> +	IIO_ADC_CHAN_NUM_PROP_TYPES
> +};
> +
> +/*
> + * Channel property types to be used with iio_adc_device_get_channels,
> + * devm_iio_adc_device_alloc_chaninfo, ...
> + */
> +#define IIO_ADC_CHAN_PROP_TYPE_REG BIT(IIO_ADC_CHAN_PROP_REG)
> +#define IIO_ADC_CHAN_PROP_TYPE_SINGLE_ENDED BIT(IIO_ADC_CHAN_PROP_SINGLE_ENDED)
> +#define IIO_ADC_CHAN_PROP_TYPE_SINGLE_COMMON					\
> +	(BIT(IIO_ADC_CHAN_PROP_SINGLE_ENDED) | BIT(IIO_ADC_CHAN_PROP_COMMON))
> +#define IIO_ADC_CHAN_PROP_TYPE_DIFF BIT(IIO_ADC_CHAN_PROP_DIFF)
> +#define IIO_ADC_CHAN_PROP_TYPE_ALL GENMASK(IIO_ADC_CHAN_NUM_PROP_TYPES - 1, 0)
> +
> +/**
> + * iio_adc_chan_props - information of expected device-tree channel properties
> + *
> + * @required:	Bitmask of property definitions of required channel properties
> + * @allowed:	Bitmask of property definitions of optional channel properties.
> + *		Listing of required properties is not needed here.
> + */
> +struct iio_adc_props {
> +	unsigned long required;
> +	unsigned long allowed;
> +};
> +
> +int iio_adc_device_num_channels(struct device *dev);
> +int devm_iio_adc_device_alloc_chaninfo(struct device *dev,
> +				const struct iio_chan_spec *template,
> +				struct iio_chan_spec **cs,
> +				const struct iio_adc_props *expected_props);
> +
> +int iio_adc_device_channels_by_property(struct device *dev, int *channels,
> +				int max_channels,
> +				const struct iio_adc_props *expected_props);
> +#endif /* _INDUSTRIAL_IO_ADC_HELPERS_H_ */

Re: [PATCH v3 3/9] iio: adc: Support ROHM BD79124 ADC

[Jonathan Cameron]

On Wed, 19 Feb 2025 14:30:43 +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,

Some fairly superficial review follows. I'm travelling for next few weeks
so not sure when I'll get time to take a more thorough look.

> diff --git a/drivers/iio/adc/rohm-bd79124.c b/drivers/iio/adc/rohm-bd79124.c
> new file mode 100644
> index 000000000000..5e23bc8d9ce2
> --- /dev/null
> +++ b/drivers/iio/adc/rohm-bd79124.c



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

As below.

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

I'd put reg and limit inline.  Local variables don't add much as
their meaning is obvious anyway from what you put in them.

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

This is shorter as straight line code rather than a loop. I'd unwind
it.  Fine to bring in a loop 'setter' like this once the benefit is
significant.

> +
> +	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_chans[BD79124_MAX_NUM_CHANNELS];
> +	int num_adc, chan, regval = 0;
> +
> +	num_adc = iio_adc_device_channels_by_property(data->dev, &adc_chans[0],
> +						      BD79124_MAX_NUM_CHANNELS,
> +						      &expected_props);
> +	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 this I would search the simpler iio_chan_spec array rather than passing
properties again.  Just look for gaps.  Or do it in the top level probe()
function and build a bitmap of which channels are ADC ones from the iio_chan_spec
array and pass that down here.

> +	 */
> +	for (chan = 0; chan < BD79124_MAX_NUM_CHANNELS; chan++)
> +		if (!bd79124_is_in_array(&adc_chans[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);

Where it doesn't make any other difference, align after (

If you are going shorter, single tab only.


> +	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);
What is this for?
> +
> +	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");
> +
> +	ret = devm_regulator_get_enable_read_voltage(dev, "vdd");
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret, "Failed to get the Vdd\n");
> +
> +	data->vmax = ret;
> +
> +	ret = devm_regulator_get_enable(dev, "iovdd");
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret, "Failed to enable I/O voltage\n");
> +
> +	ret = devm_delayed_work_autocancel(dev, &data->alm_enable_work,
> +					   bd79124_alm_enable_worker);
> +	if (ret)
> +		return ret;
> +
> +	if (i2c->irq) {
> +		template = &bd79124_chan_template;
> +	} else {
> +		template = &bd79124_chan_template_noirq;
> +		dev_dbg(dev, "No IRQ found, events disabled\n");
> +	}
> +	ret = devm_iio_adc_device_alloc_chaninfo(dev, template, &cs,
> +						 &expected_props);
> +	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);

Whilst it doesn't bring huge advantage, now we have devm_mutex_init()
it seems reasonable to use it and maybe catch a use after free for the lock.

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

Re: [PATCH v3 6/9] iio: adc: rzg2l_adc: Use adc-helpers

[Jonathan Cameron]

On Wed, 19 Feb 2025 14:31:38 +0200
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> The new devm_iio_adc_device_alloc_chaninfo() -helper is intended to help
> drivers avoid open-coding the for_each_node -loop for getting the
> channel IDs. The helper provides standard way to detect the ADC channel
> nodes (by the node name), and a standard way to convert the "reg",
> "diff-channels", "single-channel" and the "common-mode-channel" to
> channel identification numbers used in the struct iio_chan_spec.
> Furthermore, the helper checks the ID is in range of 0 ... num-channels.
> 
> The original driver treated all found child nodes as channel nodes. The
> new helper requires channel nodes to be named channel[@N]. This should
> help avoid problems with devices which may contain also other but ADC
> child nodes. Quick grep from arch/* with the rzg2l_adc's compatible
> string didn't reveal any in-tree .dts with channel nodes named
> othervice. Also, same grep shows all the .dts seem to have channel IDs
> between 0..num of channels.
> 
> Use the new helper.
> 
> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>

I should have read on.  Definitely more convincing with these usecases.
however drag them to start of series.  Better to add infrastructure
so some use and then on to your new driver.

Looks good to me.

Jonathan

Re: [PATCH v3 6/9] iio: adc: rzg2l_adc: Use adc-helpers

[Matti Vaittinen]

On 23/02/2025 18:30, Jonathan Cameron wrote:
> On Wed, 19 Feb 2025 14:31:38 +0200
> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> 
>> The new devm_iio_adc_device_alloc_chaninfo() -helper is intended to help
>> drivers avoid open-coding the for_each_node -loop for getting the
>> channel IDs. The helper provides standard way to detect the ADC channel
>> nodes (by the node name), and a standard way to convert the "reg",
>> "diff-channels", "single-channel" and the "common-mode-channel" to
>> channel identification numbers used in the struct iio_chan_spec.
>> Furthermore, the helper checks the ID is in range of 0 ... num-channels.
>>
>> The original driver treated all found child nodes as channel nodes. The
>> new helper requires channel nodes to be named channel[@N]. This should
>> help avoid problems with devices which may contain also other but ADC
>> child nodes. Quick grep from arch/* with the rzg2l_adc's compatible
>> string didn't reveal any in-tree .dts with channel nodes named
>> othervice. Also, same grep shows all the .dts seem to have channel IDs
>> between 0..num of channels.
>>
>> Use the new helper.
>>
>> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
> 
> I should have read on.  Definitely more convincing with these usecases.
> however drag them to start of series.  Better to add infrastructure
> so some use and then on to your new driver.

Ok. I'll reorder the series.

I'll drop the differential channels support in v4 - and thus also the 
expected property types parameters - which will simplify this and other 
callers.

Thanks for taking a look at this!

Yours,
	-- Matti

Re: [PATCH v3 3/9] iio: adc: Support ROHM BD79124 ADC

[Matti Vaittinen]

On 23/02/2025 18:28, Jonathan Cameron wrote:
> On Wed, 19 Feb 2025 14:30:43 +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,
> 
> Some fairly superficial review follows. I'm travelling for next few weeks
> so not sure when I'll get time to take a more thorough look.

Yeah, unfortunately people are allowed to have other life beyond the 
ROHM drivers :D
Enjoy your journey(s) ;)

...

>> +
>> +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);
> 
> As below.
> 
>> +}
>> +
>> +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);
> 
> I'd put reg and limit inline.  Local variables don't add much as
> their meaning is obvious anyway from what you put in them.

I can do this. The main purpose of those variables was to keep the 
function calls easier to read (on my limited monitor).

>> +}
>> +

...

>> +
>> +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;
>> +	}
> 
> This is shorter as straight line code rather than a loop. I'd unwind
> it.  Fine to bring in a loop 'setter' like this once the benefit is
> significant.

I suppose you're right. I think loops like this born out of a habit :)

>> +
>> +	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_chans[BD79124_MAX_NUM_CHANNELS];
>> +	int num_adc, chan, regval = 0;
>> +
>> +	num_adc = iio_adc_device_channels_by_property(data->dev, &adc_chans[0],
>> +						      BD79124_MAX_NUM_CHANNELS,
>> +						      &expected_props);
>> +	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 this I would search the simpler iio_chan_spec array rather than passing
> properties again.

I kind of agree. I did it like this because I thought that the 
'iio_adc_device_channels_by_property()' might be useful for other 
callers as well. And, if we had 'iio_adc_device_channels_by_property()' 
- then the code in this driver file becomes simple (as seen here). After 
I looked at the couple of other drivers I didn't easily spot any other 
driver needing the 'iio_adc_device_channels_by_property()' - so I 
suppose it is simpler to drop it and loop through the 'iio_chan_spec' as 
you suggest.

  Just look for gaps.  Or do it in the top level probe()
> function and build a bitmap of which channels are ADC ones from the iio_chan_spec
> array and pass that down here.
> 
>> +	 */
>> +	for (chan = 0; chan < BD79124_MAX_NUM_CHANNELS; chan++)
>> +		if (!bd79124_is_in_array(&adc_chans[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);
> 
> Where it doesn't make any other difference, align after (
> 
> If you are going shorter, single tab only.

Single tab only? You mean like:

ret = regmap_update_bits(data->map, BD79124_REG_OPMODE_CFG,
	BD79124_MASK_AUTO_INTERVAL, regval);

Do you prefer that even if the variable holding the return value was 
longer than 8 chars? To me it looks odd if arguments on the next line 
begin earlier than the function on previous line:

longvariable = regmap_update_bits(data->map, BD79124_REG_OPMODE_CFG,
	BD79124_MASK_AUTO_INTERVAL, regval);

(Just ensuring I understood your preference).

> 
> 
>> +	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);
> What is this for?

Thank's for pointing it out! It is supposed to be used in call below. 
The code works as it is just because the BD79124_CONV_MODE_MANSEQ 
happens to be '0', but it's still better to use FIELD_PREP() for the 
consistency. Below should change from:

>> +	return regmap_update_bits(data->map, BD79124_REG_OPMODE_CFG,
>> +			BD79124_MASK_CONV_MODE, BD79124_CONV_MODE_MANSEQ);
>> +
to:

	return regmap_update_bits(data->map, BD79124_REG_OPMODE_CFG,
				  BD79124_MASK_CONV_MODE, regval);

Good catch, thanks! :)

>> +}
>> +
>> +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");
>> +
>> +	ret = devm_regulator_get_enable_read_voltage(dev, "vdd");
>> +	if (ret < 0)
>> +		return dev_err_probe(dev, ret, "Failed to get the Vdd\n");
>> +
>> +	data->vmax = ret;
>> +
>> +	ret = devm_regulator_get_enable(dev, "iovdd");
>> +	if (ret < 0)
>> +		return dev_err_probe(dev, ret, "Failed to enable I/O voltage\n");
>> +
>> +	ret = devm_delayed_work_autocancel(dev, &data->alm_enable_work,
>> +					   bd79124_alm_enable_worker);
>> +	if (ret)
>> +		return ret;
>> +
>> +	if (i2c->irq) {
>> +		template = &bd79124_chan_template;
>> +	} else {
>> +		template = &bd79124_chan_template_noirq;
>> +		dev_dbg(dev, "No IRQ found, events disabled\n");
>> +	}
>> +	ret = devm_iio_adc_device_alloc_chaninfo(dev, template, &cs,
>> +						 &expected_props);
>> +	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);
> 
> Whilst it doesn't bring huge advantage, now we have devm_mutex_init()
> it seems reasonable to use it and maybe catch a use after free for the lock.

Ah, indeed. It's a good to learn to 'habitually' use devm_mutex_init().

Yours,
	-- Matti

Re: [PATCH v3 2/9] iio: adc: add helpers for parsing ADC nodes

[Matti Vaittinen]

On 23/02/2025 18:13, Jonathan Cameron wrote:
> On Wed, 19 Feb 2025 14:30:27 +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:
>> v2 => v3: Mostly based on review comments by Jonathan
>>   - Support differential and single-ended channels(*)
>>   - Rename iio_adc_device_get_channels() as
>>   - Improve spelling
>>   - Drop support for cases where DT comes from parent device's node
>>   - Decrease loop indent by reverting node name check conditions
>>   - Don't set 'chan->indexed' by number of channels to keep the
>>     interface consistent no matter how many channels are connected.
>>   - Fix ID range check and related comment
>> RFC v1 => v2:
>>   - New patch
>>
>> (*) The support for single-ended and differential channels is 100%
>> untested. I am not convinced it is actually an improvement over the
>> *_simple() helpers which only supported getting the ID from the "reg
> 
> Currently it definitely feels too complex.  Partly, whilst I haven't
> tried fleshing out the alternative, it feels like you've tried to make
> it too general.  I really don't like the allowed bitmap as those
> relationships are complex.
> 
>> In theory they could be used. In practice, while I skimmed through the
>> in-tree ADC drivers which used the for_each_child_node() construct - it
>> seemed that most of those which supported differential inputs had also
>> some other per-channel properties to read. Those users would in any case
>> need to loop through the nodes to get those other properties.
> That doesn't surprise me that much. I'm still not sure there are enough
> 'simple' cases (i.e. more than maybe 3) to justify this being shared.
> 
>>
>> If I am once more allowed to go back to proposing the _simple() variant
>> which only covers the case: "chan ID in 'reg' property"... Dropping
>> support for differential and single-ended channels would simplify this
>> helper a lot. It'd allow dropping the sanity check as well as the extra
>> parameters callers need to pass to tell what kind of properties they
>> expect. That'd (in my opinion) made the last patches (to actual ADC
>> drivers) in this series a much more lean and worthy ;)
> 
> If you do, call it _se() or something like that.

Due to the all above - I'll drop the differential channel support and 
used the _se() suffix while always accepting the single-ended property. 
This is a functional change to the existing callers later in the series 
though, but I doubt it causes problems.

>> +
>> +#include <linux/device.h>
>> +#include <linux/errno.h>
>> +#include <linux/property.h>
>> +
>> +#include <linux/iio/adc-helpers.h>
>> +
>> +int iio_adc_device_num_channels(struct device *dev)
>> +{
>> +	int num_chan = 0;
>> +
>> +	device_for_each_child_node_scoped(dev, child)
>> +		if (fwnode_name_eq(child, "channel"))
>> +			num_chan++;
>> +
>> +	return num_chan;
> 
> This function seems easy to generalize to count nodes of particular
> name.  So I'd promote this as a generic property.h helper and
> just use that in here.

I didn't think of that but now that you said it, I agree.

Oh well, that means another area impacted - let's see what the fwnode 
peeps say about adding it.

>> +}
>> +EXPORT_SYMBOL_GPL(iio_adc_device_num_channels);
>> +
>> +static const char *iio_adc_type2prop(int type)
>> +{
>> +	switch (type) {
>> +	case IIO_ADC_CHAN_PROP_TYPE_REG:
>> +		return "reg";
>> +	case IIO_ADC_CHAN_PROP_TYPE_SINGLE_ENDED:
>> +		return "single-channel";
>> +	case IIO_ADC_CHAN_PROP_TYPE_DIFF:
>> +		return "diff-channels";
>> +	case IIO_ADC_CHAN_PROP_COMMON:
>> +		return "common-mode-channel";
>> +	default:
>> +		return "unknown";
>> +	}
>> +}
>> +
>> +/*
>> + * Sanity check. Ensure that:
>> + * - At least some type(s) are allowed
>> + * - All types found are also expected
>> + * - If plain "reg" is not allowed, either single-ended or differential
>> + *   properties are found.
> 
> I'd worry this is a combination of fragile and overly separate from
> the parser.  I'd just encode this stuff down there based on accepted type
> of channels.  Two flags, differential and single ended may be enough.
> If single only then reg is expected solution but I don't see a reason to
> ignore single-channel even then as meaning is well defined.

I could accept both the single-channel and reg - but I think the binding 
is super clear telling that 'reg' is required and must match the channel 
ID. So, I feel supporting the single-channel withoout the 
common-mode-channel or diff-channels is kind of pointless?

Besides, reading both reg and single-channel leaves a question: "What if 
reg and single-channel have different values?" Should we error out? And, 
if they don't have different values, why bother checking the 
single-channel at all as reg is anyways required?

This sanity-check becomes obsolete when we only read the 'reg', and 
ignore everything which does not have (a valid) one.

>> +/**
>> + * iio_adc_device_channels_by_property - get ADC channel IDs
>> + *
>> + * Scan the device node for ADC channel information. Return an array of found
>> + * IDs. Caller needs to provide the memory for the array and provide maximum
>> + * number of IDs the array can store.
> 
> I'm somewhat confused by this. Feels like you can get same info from the
> iio_chan_spec array generated by the next function.

I used this in bd79124 for two purposes. First was setting the mux (ADC 
/ GPO) in the ADC portion of the code. This can indeed be done also by 
just iterating throuh the iio_chan_spec.

Second use-case was a call from the GPIO driver's valid_mask function. 
There I used this to detect which channels were reserved for ADC so I 
was able to build the valid-mask for GPIO core. Using the iio_chan_spec 
in GPIO code felt like a "no no" to me :) Having this available was 
quite handy - and I thought there might be other users as well.

Well, the ADC and GPIO now share the same private data (which is a bit 
ugly in my eyes, but maybe the simplest way to go) - so I can just build 
the valid-mask for the GPIOs in the ADC initialization where it reads 
the channels for the mux. That way I can avoid calling the GPIO's 
valid_mask callback and populate the info before registering the GPIO 
driver.

Long story short - I'll probably just drop this function.

>> + *
>> + * @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.
>> + * @expected_props:	Bitmaps of channel property types (for checking).
>> + *
>> + * Return:		Number of found channels on succes. 0 if no channels
>> + *			was found. Negative value to indicate failure.
>> + */
>> +int iio_adc_device_channels_by_property(struct device *dev, int *channels,
>> +		int max_channels, const struct iio_adc_props *expected_props)
>> +{
>> +	int num_chan = 0, ret;
>> +
>> +	device_for_each_child_node_scoped(dev, child) {
>> +		u32 ch, diff[2], se;
>> +		struct iio_adc_props tmp;
>> +		int chtypes_found = 0;
>> +
>> +		if (!fwnode_name_eq(child, "channel"))
>> +			continue;
>> +
>> +		if (num_chan == max_channels)
>> +			return -EINVAL;
>> +
>> +		ret = fwnode_property_read_u32(child, "reg", &ch);
>> +		if (ret)
>> +			return ret;
>> +
>> +		ret = fwnode_property_read_u32_array(child, "diff-channels",
>> +						     &diff[0], 2);
>> +		if (!ret)
>> +			chtypes_found |= IIO_ADC_CHAN_PROP_TYPE_DIFF;
>> +
>> +		ret = fwnode_property_read_u32(child, "single-channel", &se);
>> +		if (!ret)
>> +			chtypes_found |= IIO_ADC_CHAN_PROP_TYPE_SINGLE_ENDED;
>> +
>> +		tmp = *expected_props;
>> +		/*
>> +		 * We don't bother reading the "common-mode-channel" here as it
>> +		 * doesn't really affect on the primary channel ID. We remove
>> +		 * it from the required properties to allow the sanity check
>> +		 * pass here  also for drivers which require it.
>> +		 */
>> +		tmp.required &= (~BIT(IIO_ADC_CHAN_PROP_COMMON));
>> +
>> +		ret = iio_adc_prop_type_check_sanity(dev, &tmp, chtypes_found);
>> +		if (ret)
>> +			return ret;
>> +
>> +		if (chtypes_found & IIO_ADC_CHAN_PROP_TYPE_DIFF)
>> +			ch = diff[0];
>> +		else if (chtypes_found & IIO_ADC_CHAN_PROP_TYPE_SINGLE_ENDED)
>> +			ch = se;
>> +
>> +		/*
>> +		 * 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_channels_by_property);
>> +
>> +/**
>> + * devm_iio_adc_device_alloc_chaninfo - allocate and fill iio_chan_spec for adc
> 
> 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
>> + * @expected_props:	Bitmaps of channel property types (for checking).
> Input parameter so should be after template.
> 
>> + *
>> + * Return:	Number of found channels on succes. Negative value to indicate
>> + *		failure.
> 
> I wonder if an alloc function would be better returning the pointer and
> providing num_chans via a parameter.

I personally dislike returning pointer for anything which may fail other 
than by -ENOMEM. I dislike having to add all the the IS_ERR(), 
PTR_ERR(), ERR_PTR() - boilerplate when handling the errors. So, I'd 
rather kept the return value as is here if this is not a show stopper 
for you.

> 
>> + */
>> +int devm_iio_adc_device_alloc_chaninfo(struct device *dev,
>> +				const struct iio_chan_spec *template,
>> +				struct iio_chan_spec **cs,
>> +				const struct iio_adc_props *expected_props)
> 
> I'm not sure this expected props thing works as often it's a case
> of complex relationships

Luckily we can drop it altogether by having separate functions for cases 
where the channel ID is expected to be in "reg", "diff-channels" or 
"single-channel" [+ "common-mode-channel"].

And, we can introduce the more complex variants only if they are some 
day needed/usefull.

>> +{
>> +	struct iio_chan_spec *chan;
>> +	int num_chan = 0, ret;
> Initialized just after this so don't set num_chan = 0.
> 
>> +
>> +	num_chan = iio_adc_device_num_channels(dev);
>> +	if (num_chan < 1)
>> +		return num_chan;
>> +
>> +	*cs = devm_kcalloc(dev, num_chan, sizeof(**cs), GFP_KERNEL);
>> +	if (!*cs)
>> +		return -ENOMEM;
>> +
>> +	chan = &(*cs)[0];
>> +
>> +	device_for_each_child_node_scoped(dev, child) {
>> +		u32 ch, diff[2], se, common;
>> +		int chtypes_found = 0;
>> +
>> +		if (!fwnode_name_eq(child, "channel"))
>> +			continue;
>> +
>> +		ret = fwnode_property_read_u32(child, "reg", &ch);
>> +		if (ret)
>> +			return ret;
>> +
> 
> diff channels and single channel take precedence over reg, so check them
> first. If present no need to look for reg can also read it then throw
> it away giving simpler.

I liked to always read the "reg" because (AFAIR) it was marked as 
required in the binding doc.

> 
> 		*chan = *template;
> 
> 		// reg should exist either way.
> 		ret = fwnode_property_read_u32(child, "reg", &reg);
> 		if (ret)
> 			return -EINVAL; //should be a reg whatever.
> 
> 		ret = fwnode_property_read_u32_array(child, "diff-channels",
> 						     diff, ARRAY_SIZE(diff));
> 		if (ret == 0) {
> 			chan->differential = 1;
> 			chan->channel = diff[0];
> 			chan->channel2 = diff[1];
> 		} else {
> 			ret = fwnode_property_read_u32(child, "single-channel", &se);
> 			if (ret)
> 				se = reg;
> 
> 			chan->channel = se;
> 			//IIRC common mode channel is rare. I'd skip it. That
> 			//also makes it a differential channel be it a weird one.

Yes. There weren't many drivers I found using this. But I have a feeling 
that those which I checked and which did, didn't set the "differential" 
-flag in iio_chan_spec for single-ended + common - case.

***
Out of the curiosity (which means there is no need to reply if you're 
busy - I assume I can go and look the code to see what the 
'differential' and 'channel2' modifiers are used for)

What role does the "differential" Vs "single-ended" play for the users? 
I suppose it sure affects to what channels are reserved/free - but I 
would assume users were just interested to get the measurement result 
without caring about whether the signal is measured over differential 
pair or single-ended to gnd(?).
***

> 		}
> 					
> 
>> +		ret = fwnode_property_read_u32_array(child, "diff-channels",
>> +						     &diff[0], 2);
>> +		if (!ret)
>> +			chtypes_found |= IIO_ADC_CHAN_PROP_TYPE_DIFF;
>> +
>> +		ret = fwnode_property_read_u32(child, "single-channel", &se);
>> +		if (!ret)
>> +			chtypes_found |= IIO_ADC_CHAN_PROP_TYPE_SINGLE_ENDED;
>> +
>> +		ret = fwnode_property_read_u32(child, "common-mode-channel",
>> +					       &common);
>> +		if (!ret)
>> +			chtypes_found |= BIT(IIO_ADC_CHAN_PROP_COMMON);
>> +
>> +		ret = iio_adc_prop_type_check_sanity(dev, expected_props,
>> +						     chtypes_found);
> 
> If we want to verify this (I'm not yet sure) then do it as you parse the
> properties, not separately.
> 
>> +		if (ret)
>> +			return ret;
>> +
>> +		*chan = *template;
>> +		chan->channel = ch;
>> +
>> +		if (chtypes_found & IIO_ADC_CHAN_PROP_TYPE_DIFF) {
>> +			chan->differential = 1;
>> +			chan->channel = diff[0];
>> +			chan->channel2 = diff[1];
>> +
>> +		} else if (chtypes_found & IIO_ADC_CHAN_PROP_TYPE_SINGLE_ENDED) {
>> +			chan->channel = se;
>> +			if (chtypes_found & BIT(IIO_ADC_CHAN_PROP_COMMON))
>> +				chan->channel2 = common;
>> +		}
>> +
>> +		/*
>> +		 * We assume the channel IDs start from 0. If it seems this is
>> +		 * not a sane assumption, then we have to add 'allowed ID ranges'
>> +		 * to the struct iio_adc_props because some of the callers may
>> +		 * rely on the IDs being in this range - and have arrays indexed
>> +		 * by the ID.
> 
> Not a requirement in general.  It is more than possible to have a single channel
> provided that is number 7.

I think a few of the drivers did assume that the channel indexes start 
from zero, and go to num_channels - 1. It seemed quite usual that the 
channel IDs were used to index arrays with NUM_CHANNELS elements. A few 
did also check that the IDs were in a valid range.

I will add a max_id parameter to the API (and drop the 
'expected_props'). This should be useful for majority of the callers and 
help them to omit open-coding this particular check. It should also 
remind the driver to check the size of found IDs to avoid overflow bugs. 
As far as I remember at least one of the drivers which I encountered 
just trusted the DT channel IDs to be within valid range.

I'll allow omitting the check by setting the max_id to -1.

> 
>> +		 */
>> +		if (chan->channel >= num_chan)
>> +			return -ERANGE;
>> +
>> +		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..f7791d45dbd2
>> --- /dev/null
>> +++ b/include/linux/iio/adc-helpers.h
>> @@ -0,0 +1,56 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +
>> +/* The industrial I/O ADC helpers
> Comment syntax and make it more obvious that while this might be useful
> it isn't something we necessarily expect to be useful to every driver.
> /*
>   * Industrial I/O ADC firmware property passing helpers.
>   *
> 

Thanks :)

Yours,
	-- Matti

Re: [PATCH v3 9/9] iio: adc: ti-ads7924: Respect device tree config

[Jonathan Cameron]

On Wed, 19 Feb 2025 14:32:23 +0200
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

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

No to this one in it's current form.  The nodes are just there to provide
the labels.  Maybe it is fine if we fallback to registering them all if
none are provided.

> 
> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
> 
> ---
> Revision history:
> v2 => v3: New patch
> 
> Please note that this is potentially breaking existing users if they
> have wrong values in the device-tree. I believe the device-tree should
> ideally be respected, and if it says device X has only one channel, then
> we should believe it and not register 4. Well, we don't live in the
> ideal world, so even though I believe this is TheRightThingToDo - it may
> cause havoc because correct device-tree has not been required from the
> day 1. So, please review and test and apply at your own risk :)
> 
> As a side note, this might warrant a fixes tag but the adc-helper -stuff
> is hardly worth to be backported... (And I've already exceeded my time
> budget with this series - hence I'll leave crafting backportable fix to
> TI people ;) )
> 
> This has only been compile tested! All testing is highly appreciated.
> ---
>  drivers/iio/adc/ti-ads7924.c | 80 +++++++++++++++++-------------------
>  1 file changed, 37 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/iio/adc/ti-ads7924.c b/drivers/iio/adc/ti-ads7924.c
> index b1f745f75dbe..a5b8f7c81b8a 100644
> --- a/drivers/iio/adc/ti-ads7924.c
> +++ b/drivers/iio/adc/ti-ads7924.c
> @@ -22,6 +22,7 @@
>  #include <linux/regmap.h>
>  #include <linux/regulator/consumer.h>
>  
> +#include <linux/iio/adc-helpers.h>
>  #include <linux/iio/iio.h>
>  #include <linux/iio/types.h>
>  
> @@ -119,15 +120,12 @@
>  #define ADS7924_TOTAL_CONVTIME_US (ADS7924_PWRUPTIME_US + ADS7924_ACQTIME_US + \
>  				   ADS7924_CONVTIME_US)
>  
> -#define ADS7924_V_CHAN(_chan, _addr) {				\
> -	.type = IIO_VOLTAGE,					\
> -	.indexed = 1,						\
> -	.channel = _chan,					\
> -	.address = _addr,					\
> -	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW), 		\
> -	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),	\
> -	.datasheet_name = "AIN"#_chan,				\
> -}
> +static const struct iio_chan_spec ads7924_chan_template = {
> +	.type = IIO_VOLTAGE,
> +	.indexed = 1,
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> +	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
> +};
>  
>  struct ads7924_data {
>  	struct device *dev;
> @@ -182,13 +180,6 @@ static const struct regmap_config ads7924_regmap_config = {
>  	.writeable_reg = ads7924_is_writeable_reg,
>  };
>  
> -static const struct iio_chan_spec ads7924_channels[] = {
> -	ADS7924_V_CHAN(0, ADS7924_DATA0_U_REG),
> -	ADS7924_V_CHAN(1, ADS7924_DATA1_U_REG),
> -	ADS7924_V_CHAN(2, ADS7924_DATA2_U_REG),
> -	ADS7924_V_CHAN(3, ADS7924_DATA3_U_REG),
> -};
> -
>  static int ads7924_get_adc_result(struct ads7924_data *data,
>  				  struct iio_chan_spec const *chan, int *val)
>  {
> @@ -251,32 +242,38 @@ static const struct iio_info ads7924_info = {
>  	.read_raw = ads7924_read_raw,
>  };
>  
> -static int ads7924_get_channels_config(struct device *dev)
> +static const struct iio_adc_props ads7924_chan_props = {
> +	.required = IIO_ADC_CHAN_PROP_TYPE_REG,
> +};
> +
> +static int ads7924_get_channels_config(struct iio_dev *indio_dev,
> +				       struct device *dev)
>  {
> -	struct fwnode_handle *node;
> -	int num_channels = 0;
> +	struct iio_chan_spec *chan_array;
> +	int num_channels = 0, i;
>  
> -	device_for_each_child_node(dev, node) {
> -		u32 pval;
> -		unsigned int channel;
> +	num_channels = devm_iio_adc_device_alloc_chaninfo(dev,
> +					&ads7924_chan_template, &chan_array,
> +					&ads7924_chan_props);
>  
> -		if (fwnode_property_read_u32(node, "reg", &pval)) {
> -			dev_err(dev, "invalid reg on %pfw\n", node);
> -			continue;
> -		}
> +	if (num_channels < 0)
> +		return num_channels;
>  
> -		channel = pval;
> -		if (channel >= ADS7924_CHANNELS) {
> -			dev_err(dev, "invalid channel index %d on %pfw\n",
> -				channel, node);
> -			continue;
> -		}
> +	if (!num_channels)
> +		return -EINVAL;
> +
> +	for (i = 0; i < num_channels; i++) {
> +		static const char * const datasheet_names[] = {
> +			"AIN0", "AIN1", "AIN2", "AIN3"
> +		};
> +		int ch_id = chan_array[i].channel;
>  
> -		num_channels++;
> +		chan_array[i].address = ADS7924_DATA0_U_REG + ch_id;
> +		chan_array[i].datasheet_name = datasheet_names[ch_id];
>  	}
>  
> -	if (!num_channels)
> -		return -EINVAL;
> +	indio_dev->channels = chan_array;
> +	indio_dev->num_channels = num_channels;
>  
>  	return 0;
>  }
> @@ -370,18 +367,15 @@ static int ads7924_probe(struct i2c_client *client)
>  
>  	mutex_init(&data->lock);
>  
> -	indio_dev->name = "ads7924";
> -	indio_dev->modes = INDIO_DIRECT_MODE;
> -
> -	indio_dev->channels = ads7924_channels;
> -	indio_dev->num_channels = ARRAY_SIZE(ads7924_channels);
> -	indio_dev->info = &ads7924_info;
> -
> -	ret = ads7924_get_channels_config(dev);
> +	ret = ads7924_get_channels_config(indio_dev, dev);
>  	if (ret < 0)
>  		return dev_err_probe(dev, ret,
>  				     "failed to get channels configuration\n");
>  
> +	indio_dev->name = "ads7924";
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->info = &ads7924_info;
> +
>  	data->regmap = devm_regmap_init_i2c(client, &ads7924_regmap_config);
>  	if (IS_ERR(data->regmap))
>  		return dev_err_probe(dev, PTR_ERR(data->regmap),

Re: [PATCH v3 3/9] iio: adc: Support ROHM BD79124 ADC

[Jonathan Cameron]

On Mon, 24 Feb 2025 08:14:23 +0200
Matti Vaittinen <mazziesaccount@gmail.com> wrote:

> On 23/02/2025 18:28, Jonathan Cameron wrote:
> > On Wed, 19 Feb 2025 14:30:43 +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,
> > 
> > Some fairly superficial review follows. I'm travelling for next few weeks
> > so not sure when I'll get time to take a more thorough look.  
> 
> Yeah, unfortunately people are allowed to have other life beyond the 
> ROHM drivers :D
> Enjoy your journey(s) ;)

So far so good.  Hi from Shenzhen.  Obligatory pilgrimage to SEG market
done ;)

> >> +	/* 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);  
> > 
> > Where it doesn't make any other difference, align after (
> > 
> > If you are going shorter, single tab only.  
> 
> Single tab only? You mean like:
> 
> ret = regmap_update_bits(data->map, BD79124_REG_OPMODE_CFG,
> 	BD79124_MASK_AUTO_INTERVAL, regval);
> 
> Do you prefer that even if the variable holding the return value was 
> longer than 8 chars? To me it looks odd if arguments on the next line 
> begin earlier than the function on previous line:
> 
> longvariable = regmap_update_bits(data->map, BD79124_REG_OPMODE_CFG,
> 	BD79124_MASK_AUTO_INTERVAL, regval);
> 
> (Just ensuring I understood your preference).
It's hard to come up with an absolute policy / preference on this but
whilst I agree it looks a bit odd, I think it's easier to say one
tab as 'default' choice.  Obviously if it's really hideous for some
reason feel free to do something else ;)

Jonathan