Support ROHM BD79124 ADC

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

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

+#include <linux/property.h>

+EXPORT_SYMBOL_GPL(iio_adc_device_num_channels);

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

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

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

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

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

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

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

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

drivers/iio/adc/industrialio-adc.c new file +7 -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)
+{
+	struct iio_chan_spec *chan;
+	int num_chan = 0, ret;

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

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

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

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

+struct device;
+struct fwnode_handle;
+
+enum {

+ */

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

+#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_ */

Quoted 8 lines · Matti Vaittinen

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

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

Quoted 2 lines

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

...

Quoted 1 line

- Rename iio_adc_device_get_channels() as

as?

...

Quoted 6 lines

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?

Quoted 3 lines

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

...

+ bitops.h

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

Quoted 2 lines

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

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

+ export.h

+ module.h

Quoted 1 line

+#include <linux/property.h>

+ types.h

...

Quoted 1 line

+EXPORT_SYMBOL_GPL(iio_adc_device_num_channels);

No namespace?

...

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

Quoted 1 line

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

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

Unneeded parentheses around negated value.

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

Quoted 6 lines

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

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

Ditto.

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

Quoted 1 line

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

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

Quoted 11 lines

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

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

...

Quoted 12 lines

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

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

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

Quoted 10 lines

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

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

Quoted 9 lines

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

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

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

Quoted 6 lines

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

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

Quoted 26 lines

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

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

...

Quoted 14 lines

drivers/iio/adc/industrialio-adc.c new file +14 -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)
+{
+	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)

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

Quoted 3 lines

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

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

Quoted 4 lines

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

+	device_for_each_child_node_scoped(dev, child) {
+		u32 ch, diff[2], se, common;
+		int chtypes_found = 0;

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

Quoted 10 lines

drivers/iio/adc/industrialio-adc.c new file +10 -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.

Quoted 6 lines

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

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

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

Quoted 2 lines

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

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

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

Quoted 38 lines

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

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

...

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

Quoted 2 lines

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

+#ifndef _INDUSTRIAL_IO_ADC_HELPERS_H_
+#define _INDUSTRIAL_IO_ADC_HELPERS_H_

+ bits.h

Quoted 1 line

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

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

Quoted 5 lines

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

+struct device;
+struct fwnode_handle;
+
+enum {

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

Quoted 10 lines

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

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

Quoted 18 lines

+ */

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

+#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_ */

Quoted 1 line · Andy Shevchenko

On Wed, Feb 19, 2025 at 02:30:27PM +0200, Matti Vaittinen wrote:

Quoted 5 lines

Quoted 4 lines

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

Quoted 3 lines

Quoted 1 line

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

Quoted 3 lines

Quoted 3 lines

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

Quoted 3 lines

Quoted 1 line

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

Quoted 12 lines

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

Quoted 1 line

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

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

Unneeded parentheses around negated value.

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

Quoted 1 line

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

+	if (found_types & (~allowed_types)) {

Ditto.

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

Quoted 1 line

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

+		long unknown_types = found_types & (~allowed_types);

Ditto and so on...

Quoted 2 lines

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.

Quoted 10 lines

Quoted 10 lines

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

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

Quoted 3 lines

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

Quoted 1 line

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

+		tmp.required &= (~BIT(IIO_ADC_CHAN_PROP_COMMON));

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

Zero need to think of precedence.

Quoted 4 lines

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

Quoted 2 lines

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

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

Quoted 3 lines

Quoted 1 line

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

Quoted 5 lines

Quoted 3 lines

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

Quoted 1 line

Quoted 1 line

+ */

Quoted 2 lines · Matti Vaittinen

Quoted 1 line · Andy Shevchenko

On Wed, Feb 19, 2025 at 02:30:27PM +0200, Matti Vaittinen wrote:

...

Quoted 9 lines

Quoted 8 lines

Quoted 6 lines

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?

Quoted 4 lines

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.

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

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

...

Quoted 2 lines

Quoted 2 lines

Quoted 1 line

+EXPORT_SYMBOL_GPL(iio_adc_device_num_channels);

Quoted 5 lines

Quoted 1 line

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?

Quoted 8 lines

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

...

Quoted 12 lines

Quoted 12 lines

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

Quoted 1 line

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

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

Unneeded parentheses around negated value.

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

Quoted 1 line

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

+	if (found_types & (~allowed_types)) {

Ditto.

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

Quoted 1 line

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

+		long unknown_types = found_types & (~allowed_types);

Ditto and so on...

Quoted 5 lines

Quoted 2 lines

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

Quoted 5 lines

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

Quoted 3 lines

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 &= (~...);

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

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

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

...

Quoted 5 lines

Quoted 3 lines

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

Quoted 1 line

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

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

...

Quoted 9 lines

Quoted 4 lines

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

Quoted 2 lines

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

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

...

Quoted 2 lines

Quoted 2 lines

Quoted 1 line

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

Quoted 5 lines

Quoted 1 line

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.

...

Quoted 4 lines

Quoted 4 lines

Quoted 3 lines

+/*
+ * Channel property types to be used with iio_adc_device_get_channels,
+ * devm_iio_adc_device_alloc_chaninfo, ...

Quoted 5 lines

Quoted 1 line

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.

Quoted 3 lines · Andy Shevchenko

Quoted 2 lines · Matti Vaittinen

Quoted 1 line · Andy Shevchenko

On Wed, Feb 19, 2025 at 02:30:27PM +0200, Matti Vaittinen wrote:

Quoted 8 lines

Quoted 7 lines

Quoted 3 lines

Quoted 1 line

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

Quoted 2 lines

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.

Quoted 8 lines

Quoted 8 lines

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

Quoted 12 lines

Quoted 12 lines

Quoted 12 lines

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

Quoted 1 line

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

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

Unneeded parentheses around negated value.

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

Quoted 1 line

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

+	if (found_types & (~allowed_types)) {

Ditto.

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

Quoted 1 line

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

+		long unknown_types = found_types & (~allowed_types);

Ditto and so on...

Quoted 7 lines

Quoted 5 lines

Quoted 2 lines

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:

Quoted 6 lines

Quoted 5 lines

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.

Quoted 2 lines

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

Quoted 6 lines

Quoted 3 lines

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

Quoted 1 line

	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.

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

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

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

Quoted 6 lines

Quoted 5 lines

Quoted 3 lines

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

Quoted 1 line

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

+		tmp.required &= (~BIT(IIO_ADC_CHAN_PROP_COMMON));

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

Zero need to think of precedence.

Quoted 2 lines

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

Quoted 14 lines

Quoted 9 lines

Quoted 4 lines

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

Quoted 2 lines

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

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

Quoted 1 line

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

Quoted 4 lines

Quoted 4 lines

Quoted 3 lines

Quoted 1 line

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

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

Quoted 6 lines

Quoted 3 lines

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.

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.

Quoted 4 lines · Matti Vaittinen

Quoted 3 lines · Andy Shevchenko

Quoted 2 lines · Matti Vaittinen

Quoted 1 line · Andy Shevchenko

On Wed, Feb 19, 2025 at 02:30:27PM +0200, Matti Vaittinen wrote:

...

Quoted 11 lines

Quoted 10 lines

Quoted 7 lines

Quoted 3 lines

Quoted 1 line

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

Quoted 4 lines

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.

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

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

...

Quoted 12 lines

Quoted 12 lines

Quoted 12 lines

Quoted 12 lines

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

Quoted 1 line

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

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

Unneeded parentheses around negated value.

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

Quoted 1 line

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

+	if (found_types & (~allowed_types)) {

Ditto.

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

Quoted 1 line

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

+		long unknown_types = found_types & (~allowed_types);

Ditto and so on...

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

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.

Quoted 5 lines

Quoted 2 lines

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

Quoted 6 lines

Quoted 6 lines

Quoted 3 lines

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

Quoted 8 lines

Quoted 1 line

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

Quoted 3 lines

foo &= ~bar;

and having to google the priorities.

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

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

Quoted 10 lines

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

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

...

Quoted 6 lines

Quoted 6 lines

Quoted 5 lines

Quoted 3 lines

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

Quoted 1 line

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

+		tmp.required &= (~BIT(IIO_ADC_CHAN_PROP_COMMON));

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

Zero need to think of precedence.

Quoted 7 lines

Quoted 2 lines

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.

Quoted 3 lines

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)

...

Quoted 16 lines

Quoted 15 lines

Quoted 9 lines

Quoted 4 lines

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

Quoted 2 lines

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

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

Quoted 5 lines

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

...

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 3 lines

Quoted 1 line

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

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

Quoted 8 lines

Quoted 6 lines

Quoted 3 lines

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.

Quoted 1 line

And, I need the iio_chan_spec here.

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

...

Quoted 4 lines

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.

Quoted 5 lines · Andy Shevchenko

Quoted 4 lines · Matti Vaittinen

Quoted 3 lines · Andy Shevchenko

Quoted 2 lines · Matti Vaittinen

Quoted 1 line · Andy Shevchenko

On Wed, Feb 19, 2025 at 02:30:27PM +0200, Matti Vaittinen wrote:

Quoted 11 lines

Quoted 11 lines

Quoted 10 lines

Quoted 7 lines

Quoted 3 lines

Quoted 1 line

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

Quoted 6 lines

Quoted 4 lines

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!

Quoted 1 line

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.

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

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

Quoted 4 lines

Quoted 3 lines

foo &= (~bar);

is _much_ faster than seeing:

Quoted 8 lines

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

Quoted 2 lines

Quoted 2 lines

foo &= ~bar;

Quoted 4 lines

Quoted 1 line

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.

Quoted 6 lines · Matti Vaittinen

Quoted 5 lines · Andy Shevchenko

Quoted 4 lines · Matti Vaittinen

Quoted 3 lines · Andy Shevchenko

Quoted 2 lines · Matti Vaittinen

Quoted 1 line · Andy Shevchenko

On Wed, Feb 19, 2025 at 02:30:27PM +0200, Matti Vaittinen wrote:

...

Quoted 18 lines

Quoted 11 lines

Quoted 11 lines

Quoted 10 lines

Quoted 7 lines

Quoted 3 lines

Quoted 1 line

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

Quoted 6 lines

Quoted 4 lines

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?

Quoted 7 lines

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

Quoted 1 line

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

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

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

...

Quoted 22 lines

Quoted 4 lines

Quoted 3 lines

foo &= (~bar);

is _much_ faster than seeing:

Quoted 8 lines

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

Quoted 6 lines

Quoted 3 lines

foo &= ~bar;

and having to google the priorities.

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

Quoted 3 lines

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.

Quoted 2 lines

Quoted 2 lines

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

Quoted 1 line

Quoted 1 line

Quoted 1 line

foo &= (~bar);

Quoted 7 lines · Andy Shevchenko

Quoted 6 lines · Matti Vaittinen

Quoted 5 lines · Andy Shevchenko

Quoted 4 lines · Matti Vaittinen

Quoted 3 lines · Andy Shevchenko

Quoted 2 lines · Matti Vaittinen

Quoted 1 line · Andy Shevchenko

On Wed, Feb 19, 2025 at 02:30:27PM +0200, Matti Vaittinen wrote:

Quoted 26 lines

Quoted 18 lines

Quoted 11 lines

Quoted 11 lines

Quoted 10 lines

Quoted 7 lines

Quoted 3 lines

Quoted 1 line

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

Quoted 6 lines

Quoted 4 lines

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?

Quoted 7 lines

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

Quoted 1 line

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.

Quoted 8 lines

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

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

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

Quoted 22 lines

Quoted 22 lines

Quoted 4 lines

Quoted 3 lines

foo &= (~bar);

is _much_ faster than seeing:

Quoted 8 lines

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

Quoted 6 lines

Quoted 3 lines

foo &= ~bar;

and having to google the priorities.

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

Quoted 6 lines

Quoted 3 lines

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:

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

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,

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

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

Quoted 8 lines · Matti Vaittinen

Quoted 7 lines · Andy Shevchenko

Quoted 6 lines · Matti Vaittinen

Quoted 5 lines · Andy Shevchenko

Quoted 4 lines · Matti Vaittinen

Quoted 3 lines · Andy Shevchenko

Quoted 2 lines · Matti Vaittinen

Quoted 1 line · Andy Shevchenko

On Wed, Feb 19, 2025 at 02:30:27PM +0200, Matti Vaittinen wrote:

...

Quoted 35 lines

Quoted 34 lines

Quoted 18 lines

Quoted 11 lines

Quoted 11 lines

Quoted 10 lines

Quoted 7 lines

Quoted 3 lines

Quoted 1 line

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

Quoted 6 lines

Quoted 4 lines

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?

Quoted 7 lines

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

Quoted 1 line

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

Quoted 3 lines

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

Quoted 4 lines

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.

Quoted 11 lines

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

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

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

...

Quoted 34 lines

Quoted 22 lines

Quoted 22 lines

Quoted 4 lines

Quoted 3 lines

foo &= (~bar);

is _much_ faster than seeing:

Quoted 8 lines

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

Quoted 6 lines

Quoted 3 lines

foo &= ~bar;

and having to google the priorities.

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

Quoted 6 lines

Quoted 3 lines

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:

Quoted 5 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

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.

Quoted 11 lines

As an example,

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

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

Quoted 3 lines

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

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

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

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

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

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

drivers/iio/adc/Kconfig +3 -0

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

drivers/iio/adc/Makefile +0 -0

index ee19afba62b7..956c121a7544 100644

drivers/iio/adc/Makefile +0 -0

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

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

new file mode 100644
index 000000000000..0281d64ae112
@@ -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 <[email protected]>
+ */
+
+#include <linux/device.h>
+#include <linux/errno.h>
+#include <linux/property.h>
+
+#include <linux/iio/adc-helpers.h>

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

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

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

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

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

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

drivers/iio/adc/industrialio-adc.c new file +12 -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) {

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

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

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

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

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

+		 */
+		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 <[email protected]>");
+MODULE_DESCRIPTION("IIO ADC fwnode parsing helpers");

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

new file mode 100644
index 000000000000..f7791d45dbd2

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

+ *
+ * Copyright (c) 2025 Matti Vaittinen <[email protected]>
+ */
+
+#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_ */

Quoted 27 lines · On Wed, 19 Feb 2025 14:30:27 +0200 Matti Vaittinen <[email protected]>…

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

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

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

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

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

Quoted 3 lines

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

Quoted 5 lines

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.

Quoted 8 lines

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.

Quoted 12 lines

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

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

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

Quoted 4 lines

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

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

Quoted 23 lines

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

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

Quoted 2 lines

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

Quoted 6 lines

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

Quoted 67 lines

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

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

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

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

Quoted 10 lines

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

+		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

Quoted 5 lines

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

Quoted 8 lines

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

Quoted 3 lines

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

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

Quoted 6 lines

+ */

drivers/iio/adc/industrialio-adc.c new file +4 -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)

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

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

Quoted 3 lines

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

+{
+	struct iio_chan_spec *chan;
+	int num_chan = 0, ret;

Initialized just after this so don't set num_chan = 0.

Quoted 18 lines

drivers/iio/adc/industrialio-adc.c new file +18 -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;
+

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

Quoted 4 lines

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

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

		}

Quoted 6 lines

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

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

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

Quoted 10 lines

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

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

Quoted 18 lines

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

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

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

Quoted 5 lines

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

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

Quoted 21 lines

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

+		 */
+		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 <[email protected]>");
+MODULE_DESCRIPTION("IIO ADC fwnode parsing helpers");

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

new file mode 100644
index 000000000000..f7791d45dbd2
@@ -0,0 +1,56 @@

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

Quoted 3 lines

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

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

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

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

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

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

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

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

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

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

new file mode 100644
index 000000000000..5e23bc8d9ce2
 
 
 
+
+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)

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

+}
+
+static int bd79124_event_ratelimit_lo(struct bd79124_data *data,
+				      unsigned int channel)

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

+}
+
+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[] = {

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

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

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

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

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

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

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

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

Quoted 25 lines · On Wed, 19 Feb 2025 14:30:43 +0200 Matti Vaittinen <[email protected]>…

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

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

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

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

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

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

Quoted 2 lines

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

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.

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

Quoted 13 lines

drivers/iio/adc/rohm-bd79124.c new file +13 -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.

Quoted 9 lines

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

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

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

Quoted 5 lines

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

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

Quoted 2 lines

(quoted diff) +2 -0

+}
+

Quoted 10 lines

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

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

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

Quoted 4 lines

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

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

Quoted 23 lines

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

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

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

Quoted 6 lines

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

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

function and build a bitmap of which channels are ADC ones from the iio_chan_spec array and pass that down here.

Quoted 38 lines

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

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

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

Quoted 10 lines

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

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

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

Quoted 13 lines

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

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

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

Quoted 3 lines

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

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

Quoted 53 lines

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

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