Support ROHM BD79124 ADC

There are a few use-cases where child nodes with a specific name need to be parsed. Code like:

fwnode_for_each_child_node()

	if (fwnode_name_eq())
		...

can be found from a various drivers/subsystems. Adding a macro for this can simplify things a bit.

In a few cases the data from the found nodes is later added to an array, which is allocated based on the number of found nodes. One example of such use is the IIO subsystem's ADC channel nodes, where the relevant nodes are named as channel[@N].

Add helpers for iterating and counting device's sub-nodes with certain name instead of open-coding this in every user.

There are a few use-cases where child nodes with a specific name need to be parsed. Code like:

fwnode_for_each_child_node()

	if (fwnode_name_eq())
		...

can be found from a various drivers/subsystems. Adding a macro for this can simplify things a bit.

In a few cases the data from the found nodes is later added to an array, which is allocated based on the number of found nodes. One example of such use is the IIO subsystem's ADC channel nodes, where the relevant nodes are named as channel[@N].

Add helpers for iterating and counting device's sub-nodes with certain name instead of open-coding this in every user.

Suggested-by Jonathan Cameron <[email protected]>

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

---

+/**
+ * fwnode_get_named_child_node_count - number of child nodes with given name
+ * @fwnode: Node which child nodes are counted.
+ * @name: String to match child node name against.
+ *
+ * Scan child nodes and count all the nodes with a specific name. Potential
+ * 'number' -ending after the 'at sign' for scanned names is ignored.
+ * E.g.::
+ * device_get_named_child_node_count(dev, "channel");

+ * would match all the nodes::
+ * channel { }, channel@0 {}, channel@0xabba {}...
+ *
+ * Return: the number of child nodes with a matching name for a given device.
+ */

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.

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

+	chan_array = devm_kcalloc(dev, num_chan, sizeof(*chan_array),
+				  GFP_KERNEL);
+	if (!chan_array)
+		return -ENOMEM;
+
+	chan = &chan_array[0];

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

+		*chan = *template;
+		chan->channel = ch;
+		chan++;
+	}
+
+	*cs = chan_array;
+
+	return num_chan;
+}

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

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

...

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

Quoted 7 lines

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

+int devm_iio_adc_device_alloc_chaninfo_se(struct device *dev,
+					  const struct iio_chan_spec *template,
+					  int max_chan_id,
+					  struct iio_chan_spec **cs)
+{
+	struct iio_chan_spec *chan_array, *chan;
+	int num_chan = 0, ret;

Unneeded assignment.

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

Quoted 4 lines

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

+	num_chan = iio_adc_device_num_channels(dev);
+	if (num_chan < 1)
+		return num_chan;

This is really interesting code. So, if the above returns negative error code, we return it, if it returns 0, we return success (but 0 channels)?

Shouldn't we do *cs = NULL; at the case of 0 channels if it's a success?

Usually in other similar APIs we return -ENOENT. And user won't need to have an additional check in case of 0 being considered as an error case too.

(Under success I assume that returned values are okay to go with, and cs in your case will be left uninitialised or contain something we don't control.

Quoted 13 lines

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

+	chan_array = devm_kcalloc(dev, num_chan, sizeof(*chan_array),
+				  GFP_KERNEL);
+	if (!chan_array)
+		return -ENOMEM;
+
+	chan = &chan_array[0];
+
+	device_for_each_named_child_node_scoped(dev, child, "channel") {
+		u32 ch;
+
+		ret = fwnode_property_read_u32(child, "reg", &ch);
+		if (ret)
+			return ret;

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

Quoted 2 lines

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

+		if (max_chan_id != -1 && ch > max_chan_id)
+			return -ERANGE;

Hmm... What if max_chan_id is equal to an error code? Or in other words, why -1 is special and not all negative numbers?

Also note, you used unsigned type and compare it to int which, in case of being negative will give promotion.

The ch will not be
big enough in most cases (unless it's great than (INT_MAX + 1).

TL;DR: you have a potential integer overflow here.

Quoted 9 lines

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

+		*chan = *template;
+		chan->channel = ch;
+		chan++;
+	}
+
+	*cs = chan_array;
+
+	return num_chan;
+}

Quoted 1 line · Andy Shevchenko

On Thu, Mar 13, 2025 at 09:18:18AM +0200, Matti Vaittinen wrote:

Quoted 6 lines

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

Quoted 3 lines

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

+	num_chan = iio_adc_device_num_channels(dev);
+	if (num_chan < 1)
+		return num_chan;

This is really interesting code. So, if the above returns negative error code, we return it, if it returns 0, we return success (but 0 channels)?

Yes. I don't think it's that interesting though. Checking the devicetree succeeded while no channels were found. I think returning 0 is very much aligned with this.

Quoted 1 line

Shouldn't we do *cs = NULL; at the case of 0 channels if it's a success?

I suppose you're right.

But, as you pointed out in review of the 05/10:

Quoted 3 lines

Usually in other similar APIs we return -ENOENT. And user won't need to have an additional check in case of 0 being considered as an error case too.

I don't know whether to agree with you here. For majority of the ADC drivers, having no channels in devicetree is indeed just another error, which I think is not in any ways special.

However, for 33,3333% of the users added in this patch, the "no channels found" is not really an error condition ;) The BD79124 could have all channels used for GPO - although this would probably be very very unusual. (Why buying an ADC chip if you need just a GPO?). Still, this wouldn't be an error. (And I need to handle this better in BD79124 probe - so thanks).

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.

Quoted 8 lines · On Thu, 13 Mar 2025 09:18:18 +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.

Quoted 2 lines

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

I suspect we'll need the addition of an optional trailing timestamp channel at somepoint. But we can add that when we need it as only matters for drivers doing iio_push_to_buffers_with_timestamp()

Otherwise no additional comments from me.

The new devm_iio_adc_device_alloc_chaninfo_se() -helper is intended to help drivers avoid open-coding the for_each_node -loop for getting the channel IDs. The helper provides standard way to detect the ADC channel nodes (by the node name), and a standard way to convert the "reg" -properties to channel identification numbers, used in the struct iio_chan_spec. Furthermore, the helper can optionally check the found channel IDs are smaller than given maximum. This is useful for callers which later use the IDs for example for indexing a channel data array.

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

Use the new helper.

Quoted 18 lines · Matti Vaittinen

The new devm_iio_adc_device_alloc_chaninfo_se() -helper is intended to help drivers avoid open-coding the for_each_node -loop for getting the channel IDs. The helper provides standard way to detect the ADC channel nodes (by the node name), and a standard way to convert the "reg" -properties to channel identification numbers, used in the struct iio_chan_spec. Furthermore, the helper can optionally check the found channel IDs are smaller than given maximum. This is useful for callers which later use the IDs for example for indexing a channel data array.

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

Use the new helper.

...

drivers/iio/adc/sun20i-gpadc-iio.c static void sun20i_gpadc_reset_assert(void *data) · line 172

Quoted 7 lines

drivers/iio/adc/sun20i-gpadc-iio.c +5 -0

+	num_channels = devm_iio_adc_device_alloc_chaninfo_se(dev,
+				&sun20i_gpadc_chan_template, -1, &channels);
+	if (num_channels < 0)
+		return num_channels;
+
 	if (num_channels == 0)
 		return dev_err_probe(dev, -ENODEV, "no channel children\n");  

Note, this what I would expected in your helper to see, i.e. separated cases for < 0 (error code) and == 0, no channels.

Also, are all users going to have this check? Usually in other similar APIs we return -ENOENT. And user won't need to have an additional check in case of 0 being considered as an error case too.

Quoted 33 lines · On Thu, 13 Mar 2025 14:34:24 +0200 Andy Shevchenko…

Quoted 18 lines · Matti Vaittinen

The new devm_iio_adc_device_alloc_chaninfo_se() -helper is intended to help drivers avoid open-coding the for_each_node -loop for getting the channel IDs. The helper provides standard way to detect the ADC channel nodes (by the node name), and a standard way to convert the "reg" -properties to channel identification numbers, used in the struct iio_chan_spec. Furthermore, the helper can optionally check the found channel IDs are smaller than given maximum. This is useful for callers which later use the IDs for example for indexing a channel data array.

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

Use the new helper.

...

drivers/iio/adc/sun20i-gpadc-iio.c static void sun20i_gpadc_reset_assert(void *data) · line 172

Quoted 7 lines

drivers/iio/adc/sun20i-gpadc-iio.c +5 -0

+	num_channels = devm_iio_adc_device_alloc_chaninfo_se(dev,
+				&sun20i_gpadc_chan_template, -1, &channels);
+	if (num_channels < 0)
+		return num_channels;
+
  	if (num_channels == 0)
  		return dev_err_probe(dev, -ENODEV, "no channel children\n");

Note, this what I would expected in your helper to see, i.e. separated cases for < 0 (error code) and == 0, no channels.

Quoted 3 lines

Also, are all users going to have this check? Usually in other similar APIs we return -ENOENT. And user won't need to have an additional check in case of 0 being considered as an error case too.

In a few cases we'll need to do the dance the other way in the caller. So specifically check for -ENOENT and not treat it as an error.

That stems from channel nodes being optionally added to drivers after they have been around a while (usually to add more specific configuration) and needing to maintain old behaviour of presenting all channels with default settings.

I agree that returning -ENOENT is a reasonable way to handle this.

Quoted 3 lines · Jonathan Cameron

Quoted 1 line · On Thu, 13 Mar 2025 14:34:24 +0200 Andy Shevchenko…

On Thu, Mar 13, 2025 at 09:18:49AM +0200, Matti Vaittinen wrote:

Quoted 22 lines

Quoted 11 lines

drivers/iio/adc/sun20i-gpadc-iio.c static void sun20i_gpadc_reset_assert(void *data) · line 172

Quoted 7 lines

drivers/iio/adc/sun20i-gpadc-iio.c +5 -0

+	num_channels = devm_iio_adc_device_alloc_chaninfo_se(dev,
+				&sun20i_gpadc_chan_template, -1, &channels);
+	if (num_channels < 0)
+		return num_channels;
+
  	if (num_channels == 0)
  		return dev_err_probe(dev, -ENODEV, "no channel children\n");

Note, this what I would expected in your helper to see, i.e. separated cases for < 0 (error code) and == 0, no channels.

Quoted 3 lines

Also, are all users going to have this check? Usually in other similar APIs we return -ENOENT. And user won't need to have an additional check in case of 0 being considered as an error case too.

In a few cases we'll need to do the dance the other way in the caller. So specifically check for -ENOENT and not treat it as an error.

That stems from channel nodes being optionally added to drivers after they have been around a while (usually to add more specific configuration) and needing to maintain old behaviour of presenting all channels with default settings.

Quoted 1 line

I agree that returning -ENOENT is a reasonable way to handle this.

I agree - but I'm going to use -ENODEV instead of -ENOENT because that's what the current callers return if they find no channels. That way the drivers can return the value directly without converting -ENOENT to -ENODEV.

Quoted 4 lines · Matti Vaittinen

Quoted 3 lines · Jonathan Cameron

Quoted 1 line · On Thu, 13 Mar 2025 14:34:24 +0200 Andy Shevchenko…

On Thu, Mar 13, 2025 at 09:18:49AM +0200, Matti Vaittinen wrote:

...

Quoted 22 lines

Quoted 22 lines

Quoted 11 lines

drivers/iio/adc/sun20i-gpadc-iio.c static void sun20i_gpadc_reset_assert(void *data) · line 172

Quoted 7 lines

drivers/iio/adc/sun20i-gpadc-iio.c +5 -0

+	num_channels = devm_iio_adc_device_alloc_chaninfo_se(dev,
+				&sun20i_gpadc_chan_template, -1, &channels);
+	if (num_channels < 0)
+		return num_channels;
+
   	if (num_channels == 0)
   		return dev_err_probe(dev, -ENODEV, "no channel children\n");

Note, this what I would expected in your helper to see, i.e. separated cases for < 0 (error code) and == 0, no channels.

Quoted 3 lines

Also, are all users going to have this check? Usually in other similar APIs we return -ENOENT. And user won't need to have an additional check in case of 0 being considered as an error case too.

In a few cases we'll need to do the dance the other way in the caller. So specifically check for -ENOENT and not treat it as an error.

That stems from channel nodes being optionally added to drivers after they have been around a while (usually to add more specific configuration) and needing to maintain old behaviour of presenting all channels with default settings.

Quoted 5 lines

Quoted 1 line

I agree that returning -ENOENT is a reasonable way to handle this.

I agree - but I'm going to use -ENODEV instead of -ENOENT because that's what the current callers return if they find no channels. That way the drivers can return the value directly without converting -ENOENT to -ENODEV.

ENODEV can be easily clashed with other irrelevant cases,

ENOENT is the correct
error code.

If drivers return this instead of another error code, nothing bad happen, it's not an ABI path, correct?

Quoted 5 lines · Andy Shevchenko

Quoted 4 lines · Matti Vaittinen

Quoted 3 lines · Jonathan Cameron

Quoted 1 line · On Thu, 13 Mar 2025 14:34:24 +0200 Andy Shevchenko…

On Thu, Mar 13, 2025 at 09:18:49AM +0200, Matti Vaittinen wrote:

Quoted 24 lines

Quoted 24 lines

Quoted 23 lines

Quoted 11 lines

drivers/iio/adc/sun20i-gpadc-iio.c static void sun20i_gpadc_reset_assert(void *data) · line 172

Quoted 7 lines

drivers/iio/adc/sun20i-gpadc-iio.c +5 -0

+	num_channels = devm_iio_adc_device_alloc_chaninfo_se(dev,
+				&sun20i_gpadc_chan_template, -1, &channels);
+	if (num_channels < 0)
+		return num_channels;
+
   	if (num_channels == 0)
   		return dev_err_probe(dev, -ENODEV, "no channel children\n");

Note, this what I would expected in your helper to see, i.e. separated cases for < 0 (error code) and == 0, no channels.

Quoted 3 lines

Also, are all users going to have this check? Usually in other similar APIs we return -ENOENT. And user won't need to have an additional check in case of 0 being considered as an error case too.

In a few cases we'll need to do the dance the other way in the caller. So specifically check for -ENOENT and not treat it as an error.

That stems from channel nodes being optionally added to drivers after they have been around a while (usually to add more specific configuration) and needing to maintain old behaviour of presenting all channels with default settings.

I agree that returning -ENOENT is a reasonable way to handle this.

Quoted 5 lines

Quoted 3 lines

I agree - but I'm going to use -ENODEV instead of -ENOENT because that's what the current callers return if they find no channels. That way the drivers can return the value directly without converting -ENOENT to -ENODEV.

ENODEV can be easily clashed with other irrelevant cases,

Can you please explain what cases?

Quoted 2 lines

ENOENT is the correct
error code.

I kind of agree if we look this from the fwnode perspective. But, when we look this from the intended user's perspective, I can very well understand the -ENODEV. Returning -ENODEV from ADC driver's probe which can't find any of the channels feels correct to me.

Quoted 2 lines

If drivers return this instead of another error code, nothing bad happen, it's not an ABI path, correct?

I don't know if failure returned from a probe is an ABI. I still feel -ENODEV is correct value,

and I don't want to change it for existing users -
and I think also new ADC drivers should use -ENODEV if they find no channels at all.

Besides that I think -ENODEV to be right, changing it to -ENOENT for existing callers requires a buy-in from Jonathan (and/or) the driver maintainers.

Quoted 6 lines · Matti Vaittinen

Quoted 5 lines · Andy Shevchenko

Quoted 4 lines · Matti Vaittinen

Quoted 3 lines · Jonathan Cameron

Quoted 1 line · On Thu, 13 Mar 2025 14:34:24 +0200 Andy Shevchenko…

On Thu, Mar 13, 2025 at 09:18:49AM +0200, Matti Vaittinen wrote:

...

Quoted 24 lines

Quoted 24 lines

Quoted 24 lines

Quoted 23 lines

Quoted 11 lines

Review

Quoted 7 lines

(quoted diff) +5 -0

+	num_channels = devm_iio_adc_device_alloc_chaninfo_se(dev,
+				&sun20i_gpadc_chan_template, -1, &channels);
+	if (num_channels < 0)
+		return num_channels;
+
   	if (num_channels == 0)
   		return dev_err_probe(dev, -ENODEV, "no channel children\n");  

Note, this what I would expected in your helper to see, i.e. separated cases for < 0 (error code) and == 0, no channels.

Quoted 3 lines

Also, are all users going to have this check? Usually in other similar APIs we return -ENOENT. And user won't need to have an additional check in case of 0 being considered as an error case too.

In a few cases we'll need to do the dance the other way in the caller. So specifically check for -ENOENT and not treat it as an error.

That stems from channel nodes being optionally added to drivers after they have been around a while (usually to add more specific configuration) and needing to maintain old behaviour of presenting all channels with default settings.

I agree that returning -ENOENT is a reasonable way to handle this.

Quoted 7 lines

Quoted 5 lines

Quoted 3 lines

I agree - but I'm going to use -ENODEV instead of -ENOENT because that's what the current callers return if they find no channels. That way the drivers can return the value directly without converting -ENOENT to -ENODEV.

ENODEV can be easily clashed with other irrelevant cases,

Can you please explain what cases?

When it's a code path that returns the same error code for something different.

Quoted 7 lines

Quoted 2 lines

ENOENT is the correct
error code.

I kind of agree if we look this from the fwnode perspective. But, when we look this from the intended user's perspective, I can very well understand the -ENODEV. Returning -ENODEV from ADC driver's probe which can't find any of the channels feels correct to me.

Okay, it seems we have got yet another disagreement as I think this has to be ENOENT. Because this is related to the firmware description and not real hardware discovery path. If it is the latter, I may fully agree on ENODEV choice. But AFAICS it's not the case here.

Quoted 6 lines

Quoted 3 lines

If drivers return this instead of another error code, nothing bad happen, it's not an ABI path, correct?

I don't know if failure returned from a probe is an ABI. I still feel -ENODEV is correct value,

And I have the opposite opinion. I think ENODEV is _incorrect_ choice in this case.

Quoted 3 lines

and I don't want to change it for existing users -
and I think also new ADC drivers should use -ENODEV if they find no channels at all.

Again, the problem is that you are trying to apply the error code for HW to the information that comes from FW.

Quoted 3 lines

Besides that I think -ENODEV to be right, changing it to -ENOENT for existing callers requires a buy-in from Jonathan (and/or) the driver maintainers.

Yeah, will wait for Jonathan to judge, but I think you can find rationale above.

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.

+#define BD79124_REG_SYSTEM_STATUS 0x0

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

+#define BD79124_REG_GEN_CFG		0x01
+#define BD79124_REG_OPMODE_CFG		0x04
+#define BD79124_REG_PINCFG		0x05
+#define BD79124_REG_GPO_VAL		0x0B
+#define BD79124_REG_SEQUENCE_CFG	0x10

+ * These macros return the address of the 1.st reg for the given channel

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

+/*
+ * The hysteresis for a channel is stored in the same register where the
+ * 4 bits of high limit reside.
+ */
+#define BD79124_GET_HYSTERESIS_REG(ch) BD79124_GET_HIGH_LIMIT_REG(ch)
+
+#define BD79124_MAX_NUM_CHANNELS 8
+
+struct bd79124_data {
+	s64 timestamp;
+	struct regmap *map;
+	struct device *dev;
+	int vmax;
+	/*
+	 * Keep measurement status so read_raw() knows if the measurement needs
+	 * to be started.
+	 */
+	int alarm_monitored[BD79124_MAX_NUM_CHANNELS];
+	/*
+	 * The BD79124 does not allow disabling/enabling limit separately for
+	 * one direction only. Hence, we do the disabling by changing the limit
+	 * to maximum/minimum measurable value. This means we need to cache
+	 * the limit in order to maintain it over the time limit is disabled.
+	 */
+	u16 alarm_r_limit[BD79124_MAX_NUM_CHANNELS];
+	u16 alarm_f_limit[BD79124_MAX_NUM_CHANNELS];
+	/* Bitmask of disabled events (for rate limiting) for each channel. */
+	int alarm_suppressed[BD79124_MAX_NUM_CHANNELS];
+	/*
+	 * The BD79124 is configured to run the measurements in the background.
+	 * This is done for the event monitoring as well as for the read_raw().
+	 * Protect the measurement starting/stopping using a mutex.
+	 */
+	struct mutex mutex;
+	struct delayed_work alm_enable_work;
+	struct gpio_chip gc;
+	u8 gpio_valid_mask;
+};

+static void bd79124gpo_set(struct gpio_chip *gc, unsigned int offset, int value)

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

+	struct bd79124_data *data = gpiochip_get_data(gc);
+
+	/* Ensure all GPIOs in 'mask' are set to be GPIOs */
+	ret = regmap_read(data->map, BD79124_REG_PINCFG, &val);
+	if (ret)
+		return;
+
+	if ((val & *mask) != *mask) {
+		dev_dbg(data->dev, "Invalid mux config. Can't set value.\n");
+		/* Do not set value for pins configured as ADC inputs */
+		*mask &= val;
+	}
+
+	regmap_update_bits(data->map, BD79124_REG_GPO_VAL, *mask, *bits);
+}

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

+	return regmap_bulk_write(data->map, reg, &raw, sizeof(raw));
+}

(quoted diff) +1 -0

+		 */

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

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

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

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

+	return IRQ_HANDLED;
+}

+}

(quoted diff) +1 -0

+	}

(quoted diff) +2 -0

+	{ }
+};

Quoted 16 lines · Matti Vaittinen

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.

Quoted 8 lines

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

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

...

Quoted 1 line

+#define BD79124_REG_SYSTEM_STATUS 0x0

I would expect to see 0x00

Quoted 6 lines

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

+#define BD79124_REG_GEN_CFG		0x01
+#define BD79124_REG_OPMODE_CFG		0x04
+#define BD79124_REG_PINCFG		0x05
+#define BD79124_REG_GPO_VAL		0x0B
+#define BD79124_REG_SEQUENCE_CFG	0x10

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

Quoted 10 lines

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

+#define BD79124_REG_MANUAL_CHANNELS	0x11
+#define BD79124_REG_AUTO_CHANNELS	0x12
+#define BD79124_REG_ALERT_CH_SEL	0x14
+#define BD79124_REG_EVENT_FLAG		0x18
+#define BD79124_REG_EVENT_FLAG_HI	0x1a
+#define BD79124_REG_EVENT_FLAG_LO	0x1c
+#define BD79124_REG_HYSTERESIS_CH0	0x20
+#define BD79124_REG_EVENTCOUNT_CH0	0x22
+#define BD79124_REG_RECENT_CH0_LSB	0xa0
+#define BD79124_REG_RECENT_CH7_MSB	0xaf

...

Wouldn't be better...

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

Quoted 5 lines

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

+#define BD79124_MASK_CONV_MODE GENMASK(6, 5)
+#define BD79124_MASK_AUTO_INTERVAL GENMASK(1, 0)
+#define BD79124_CONV_MODE_MANSEQ 0
+#define BD79124_CONV_MODE_AUTO 1
+#define BD79124_INTERVAL_750_US 0

To group masks and values of the same bitfields?

#define BD79124_MASK_CONV_MODE GENMASK(6, 5)
#define BD79124_CONV_MODE_MANSEQ 0
#define BD79124_CONV_MODE_AUTO 1
#define BD79124_MASK_AUTO_INTERVAL GENMASK(1, 0)
#define BD79124_INTERVAL_750_US 0

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

Quoted 11 lines

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

+#define BD79124_MASK_DWC_EN BIT(4)
+#define BD79124_MASK_STATS_EN BIT(5)
+#define BD79124_MASK_SEQ_START BIT(4)
+#define BD79124_MASK_SEQ_MODE GENMASK(1, 0)
+#define BD79124_MASK_SEQ_MANUAL 0
+#define BD79124_MASK_SEQ_SEQ 1
+
+#define BD79124_MASK_HYSTERESIS GENMASK(3, 0)
+#define BD79124_LOW_LIMIT_MIN 0
+#define BD79124_HIGH_LIMIT_MAX GENMASK(11, 0)

These masks are not named after registers (or I don't see it clearly),

it's
hard to understand which one relates to which register. Also, why not using bitfield.h?

...

Quoted 1 line

+ * These macros return the address of the 1.st reg for the given channel

first

(and missing period at the end).

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

Quoted 5 lines

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

+#define BD79124_GET_HIGH_LIMIT_REG(ch) (BD79124_REG_HYSTERESIS_CH0 + (ch) * 4)
+#define BD79124_GET_LOW_LIMIT_REG(ch) (BD79124_REG_EVENTCOUNT_CH0 + (ch) * 4)
+#define BD79124_GET_LIMIT_REG(ch, dir) ((dir) == IIO_EV_DIR_RISING ?		\
+		BD79124_GET_HIGH_LIMIT_REG(ch) : BD79124_GET_LOW_LIMIT_REG(ch))
+#define BD79124_GET_RECENT_RES_REG(ch) (BD79124_REG_RECENT_CH0_LSB + (ch) * 2)

Don't we want to have something in bitfield.h for arrays in the register, i.e. when register(s) is(are) split to 2+ bits per an element in an array of the same semantics. Would be nice to eliminate such a boilerplate here and in many other drivers.

Quoted 38 lines

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

+/*
+ * The hysteresis for a channel is stored in the same register where the
+ * 4 bits of high limit reside.
+ */
+#define BD79124_GET_HYSTERESIS_REG(ch) BD79124_GET_HIGH_LIMIT_REG(ch)
+
+#define BD79124_MAX_NUM_CHANNELS 8
+
+struct bd79124_data {
+	s64 timestamp;
+	struct regmap *map;
+	struct device *dev;
+	int vmax;
+	/*
+	 * Keep measurement status so read_raw() knows if the measurement needs
+	 * to be started.
+	 */
+	int alarm_monitored[BD79124_MAX_NUM_CHANNELS];
+	/*
+	 * The BD79124 does not allow disabling/enabling limit separately for
+	 * one direction only. Hence, we do the disabling by changing the limit
+	 * to maximum/minimum measurable value. This means we need to cache
+	 * the limit in order to maintain it over the time limit is disabled.
+	 */
+	u16 alarm_r_limit[BD79124_MAX_NUM_CHANNELS];
+	u16 alarm_f_limit[BD79124_MAX_NUM_CHANNELS];
+	/* Bitmask of disabled events (for rate limiting) for each channel. */
+	int alarm_suppressed[BD79124_MAX_NUM_CHANNELS];
+	/*
+	 * The BD79124 is configured to run the measurements in the background.
+	 * This is done for the event monitoring as well as for the read_raw().
+	 * Protect the measurement starting/stopping using a mutex.
+	 */
+	struct mutex mutex;
+	struct delayed_work alm_enable_work;
+	struct gpio_chip gc;
+	u8 gpio_valid_mask;
+};

...

Quoted 1 line

+static void bd79124gpo_set(struct gpio_chip *gc, unsigned int offset, int value)

You should use .set_rv()

...

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

Quoted 2 lines

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

+static void bd79124gpo_set_multiple(struct gpio_chip *gc, unsigned long *mask,
+				    unsigned long *bits)

Ditto, .set_multiple_rv().

Review

Quoted 3 lines

(quoted diff) +2 -0

+{
+	int ret, val;

Here and everywhere else, the returned value by regmap is unsigned int.

Quoted 16 lines

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

+	struct bd79124_data *data = gpiochip_get_data(gc);
+
+	/* Ensure all GPIOs in 'mask' are set to be GPIOs */
+	ret = regmap_read(data->map, BD79124_REG_PINCFG, &val);
+	if (ret)
+		return;
+
+	if ((val & *mask) != *mask) {
+		dev_dbg(data->dev, "Invalid mux config. Can't set value.\n");
+		/* Do not set value for pins configured as ADC inputs */
+		*mask &= val;
+	}
+
+	regmap_update_bits(data->map, BD79124_REG_GPO_VAL, *mask, *bits);
+}

...

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

Quoted 5 lines

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

+struct bd79124_raw {
+	u8 bit0_3; /* Is set in high bits of the byte */
+	u8 bit4_11;
+};
+#define BD79124_RAW_TO_INT(r) ((r.bit4_11 << 4) | (r.bit0_3 >> 4))

And how this is different from treating it as __le16? Needs a good comment about bit layout.

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

Quoted 4 lines

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

+struct bd79124_raw {
+	u8 bit0_3; /* Is set in high bits of the byte */
+	u8 bit4_11;
+};

...

Quoted 5 lines

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

+static int bd79124_write_int_to_reg(struct bd79124_data *data, int reg,
+				    unsigned int val)
+{
+	struct bd79124_raw raw;
+	int ret, tmp;

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

Quoted 2 lines

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

+	raw.bit4_11 = (u8)(val >> 4);
+	raw.bit0_3 = (u8)(val << 4);

Why casting?

Make sense to have a symmetrical macro instead of hiding it in the code.

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

Quoted 5 lines

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

+	ret = regmap_read(data->map, reg, &tmp);
+	if (ret)
+		return ret;
+
+	raw.bit0_3 |= (0xf & tmp);

GENMASK() ?

Quoted 2 lines

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

+	return regmap_bulk_write(data->map, reg, &raw, sizeof(raw));
+}

...

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

Quoted 1 line

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

+	return (data->alarm_monitored[chan->channel] & BIT(dir));

Unneeded parentheses.

...

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

Quoted 3 lines

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

+	case IIO_EV_INFO_HYSTERESIS:
+		reg = BD79124_GET_HYSTERESIS_REG(chan->channel);
+		val >>= 3;

Second time I see this. Perhaps you need a macro to explain this right shift?

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

Quoted 3 lines

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

+		return regmap_update_bits(data->map, reg, BD79124_MASK_HYSTERESIS,
+					  val);

Can be one line.

...

Quoted 15 lines

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

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

Quoted 5 lines

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

+		return IRQ_NONE;
+
+	ret = regmap_read(data->map, BD79124_REG_EVENT_FLAG_LO, &i_lo);
+	if (ret)
+		return IRQ_NONE;

Only I don't get why you can't use bulk read here. The registers seem to be sequential.

Quoted 39 lines

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

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

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

Quoted 6 lines

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

+	if (ret)
+		return IRQ_NONE;
+
+	ret = regmap_write(data->map, BD79124_REG_EVENT_FLAG_LO, i_lo);
+	if (ret)
+		return IRQ_NONE;

In the similar way bulk write.

Quoted 3 lines

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

+	return IRQ_HANDLED;
+}

...

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

Quoted 5 lines

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

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

BD79124_HIGH_LIMIT_MAX ?

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

Quoted 4 lines

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

+	if (ret)
+		return ret;
+
+	return regmap_write(data->map, BD79124_GET_LOW_LIMIT_REG(channel), 0);

BD79124_HIGH_LIMIT_MIN ?

Quoted 1 line

+}

...

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

Quoted 2 lines

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

+static const struct i2c_device_id bd79124_id[] = {
+	{ "bd79124", },

Redundant inner comma.

Quoted 17 lines · Andy Shevchenko

Quoted 6 lines · Matti Vaittinen

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

+static int bd79124_chan_init(struct bd79124_data *data, int channel)
+{
+    int ret;
+
+    ret = regmap_write(data->map, 

BD79124_GET_HIGH_LIMIT_REG(channel), 4095);

BD79124_HIGH_LIMIT_MAX ?

Quoted 5 lines

+    if (ret)
+        return ret;
+
+    return regmap_write(data->map,
BD79124_GET_LOW_LIMIT_REG(channel), 0);

Quoted 1 line

BD79124_HIGH_LIMIT_MIN ?

I think I originally had these defines but I was probably asked to drop the defines and use raw values instead.

Quoted 1 line · Andy Shevchenko

On Thu, Mar 13, 2025 at 09:19:03AM +0200, Matti Vaittinen wrote:

Quoted 5 lines

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

Quoted 3 lines

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

+#define BD79124_MASK_HYSTERESIS GENMASK(3, 0)
+#define BD79124_LOW_LIMIT_MIN 0
+#define BD79124_HIGH_LIMIT_MAX GENMASK(11, 0)

These masks are not named after registers (or I don't see it clearly),

Naming is hard. I usually try to make a balance between:

1) using names which explain the purpose when seen in the code (at call site)
2) keeping names short enough
3) following the naming convention in the data sheet
4) maintain relation to the register.

I put most emphasis to the 1, while 2 is important to me as well. 3 is _very_ nice to have, but it is often somehow contradicting with 1 and 2. 4 is IMO just nice to have. The register is usually clearly visible at call site, and if someone adds new functionality (or checks the correctness of the masks/registers), then 3 is way more important.

I am open to any concrete suggestions though.

Quoted 3 lines

it's
hard to understand which one relates to which register. Also, why not using bitfield.h?

I saw no need for it?

Quoted 3 lines

Quoted 1 line

+ * These macros return the address of the 1.st reg for the given channel

first

Huh?

Quoted 1 line

(and missing period at the end).

Ok.

Quoted 10 lines

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

Quoted 5 lines

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

+#define BD79124_GET_HIGH_LIMIT_REG(ch) (BD79124_REG_HYSTERESIS_CH0 + (ch) * 4)
+#define BD79124_GET_LOW_LIMIT_REG(ch) (BD79124_REG_EVENTCOUNT_CH0 + (ch) * 4)
+#define BD79124_GET_LIMIT_REG(ch, dir) ((dir) == IIO_EV_DIR_RISING ?		\
+		BD79124_GET_HIGH_LIMIT_REG(ch) : BD79124_GET_LOW_LIMIT_REG(ch))
+#define BD79124_GET_RECENT_RES_REG(ch) (BD79124_REG_RECENT_CH0_LSB + (ch) * 2)

Don't we want to have something in bitfield.h for arrays in the register, i.e. when register(s) is(are) split to 2+ bits per an element in an array of the same semantics. Would be nice to eliminate such a boilerplate here and in many other drivers.

Not necessarily a bad idea. Still, I am not willing to expand this series any more - currently there is 10 patches, 2 of which are directly related to the BD79124. I don't want to delay this driver for another cycle due to added helpers.

Quoted 3 lines

Quoted 1 line

+static void bd79124gpo_set(struct gpio_chip *gc, unsigned int offset, int value)

You should use .set_rv()

Quoted 4 lines

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

Quoted 2 lines

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

+static void bd79124gpo_set_multiple(struct gpio_chip *gc, unsigned long *mask,
+				    unsigned long *bits)

Ditto, .set_multiple_rv().

As you know, this started at v6.14 cycle which is still ongoing. I don't think .set_rv() and .set_multiple_rv() are available?

Quoted 8 lines

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

Quoted 5 lines

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

+struct bd79124_raw {
+	u8 bit0_3; /* Is set in high bits of the byte */
+	u8 bit4_11;
+};
+#define BD79124_RAW_TO_INT(r) ((r.bit4_11 << 4) | (r.bit0_3 >> 4))

And how this is different from treating it as __le16? Needs a good comment about bit layout.

You don't think:

Quoted 4 lines

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

Quoted 4 lines

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

+struct bd79124_raw {
+	u8 bit0_3; /* Is set in high bits of the byte */
+	u8 bit4_11;
+};

suffices? It's hard for me to think how to explain bit layout more explicitly.

This was discussed during the RFC review. I explained the rationale why I rather represent this as two 8 bit variables than le16 with (mysterious to me) shift. As a result, Jonathan told me he's not feeling (too) strong about this (but also warned we may see follow-up patches converting this to le and shift - which, by the way, is harder for me to understand).

Quoted 6 lines

Quoted 5 lines

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

+static int bd79124_write_int_to_reg(struct bd79124_data *data, int reg,
+				    unsigned int val)
+{
+	struct bd79124_raw raw;
+	int ret, tmp;

Quoted 4 lines

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

Quoted 2 lines

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

+	raw.bit4_11 = (u8)(val >> 4);
+	raw.bit0_3 = (u8)(val << 4);

Why casting?

To make it clear storing unsigned int to u8 is considered to be Ok. I can drop it though if you feel strong about it.

Quoted 7 lines

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

Quoted 5 lines

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

+	ret = regmap_read(data->map, reg, &tmp);
+	if (ret)
+		return ret;
+
+	raw.bit0_3 |= (0xf & tmp);

GENMASK() ?

For me 0xf is both shorter and clearer. (For me 0xf - or, 0x0f if you like, 0xf0 and 0xff are clear by a glance).

I can go for GENMASK() if it is absolutely required, but for me it is in this case just making this harder to read. I like GENMASK() for something like 0xe or 0x60 though.

Quoted 2 lines

Quoted 2 lines

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

+	return regmap_bulk_write(data->map, reg, &raw, sizeof(raw));
+}

Quoted 5 lines

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

Quoted 3 lines

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

+	case IIO_EV_INFO_HYSTERESIS:
+		reg = BD79124_GET_HYSTERESIS_REG(chan->channel);
+		val >>= 3;

Second time I see this. Perhaps you need a macro to explain this right shift?

Not really sure about this. I think it's kind of obvious why this is shifted
- and in case it's not, there is a comment explaining it.

I could have a macro like REGVAL2HYSTERESIS() - but I don't think it is a great idea, because then anyone interested in understanding the value read from register would need to dig out the macro to find how value needs to be converted. Doing the shift here shows the register format to a reader right away - and honestly, it should be pretty obvious to the reader that this shift converts register value to proper format.

Quoted 4 lines

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

Quoted 2 lines

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

+		return regmap_update_bits(data->map, reg, BD79124_MASK_HYSTERESIS,
+					  val);

Can be one line.

I still prefer to have lines < 80 to make them fit my terminal. I think we have discussed this before. I don't think we will agree on this as I have a very real reason for short lines. It does directly impact on my daily work. I don't think you're going to like it no matter how I explain this.

Quoted 2 lines · Matti Vaittinen

Quoted 1 line · Andy Shevchenko

On Thu, Mar 13, 2025 at 09:19:03AM +0200, Matti Vaittinen wrote:

...

Quoted 15 lines

Quoted 6 lines

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

Quoted 4 lines

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

+#define BD79124_MASK_HYSTERESIS GENMASK(3, 0)
+#define BD79124_LOW_LIMIT_MIN 0
+#define BD79124_HIGH_LIMIT_MAX GENMASK(11, 0)  

These masks are not named after registers (or I don't see it clearly),

Naming is hard. I usually try to make a balance between:

1) using names which explain the purpose when seen in the code (at call site)
2) keeping names short enough
3) following the naming convention in the data sheet
4) maintain relation to the register.

Quoted 7 lines

I put most emphasis to the 1, while 2 is important to me as well. 3 is _very_ nice to have, but it is often somehow contradicting with 1 and 2. 4 is IMO just nice to have. The register is usually clearly visible at call site, and if someone adds new functionality (or checks the correctness of the masks/registers), then 3 is way more important.

I am open to any concrete suggestions though.

From my point of view the starting point is 3, then one may apply 2 and 4, the 1 may mangle the name so much that register data field name becomes quite abstract.

Quoted 5 lines

Quoted 3 lines

it's
hard to understand which one relates to which register. Also, why not using bitfield.h?

I saw no need for it?

Hmm... Okay, I think Jonathan will ask that if really needed.

...

Quoted 5 lines

Quoted 4 lines

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

Quoted 2 lines

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

+static void bd79124gpo_set_multiple(struct gpio_chip *gc, unsigned long *mask,
+				    unsigned long *bits)  

Ditto, .set_multiple_rv().

Quoted 2 lines

As you know, this started at v6.14 cycle which is still ongoing. I don't think .set_rv() and .set_multiple_rv() are available?

You mean that you are still hope to have this series to be squeezed for v6.15-rc1? That's not me who decides, but if not, those APIs will be part of the v6.15-rc1 (at least they are pending in GPIO subsystem).

Quoted 5 lines · On Fri, 14 Mar 2025 16:33:13 +0200 Andy Shevchenko…

Quoted 2 lines · Matti Vaittinen

Quoted 1 line · Andy Shevchenko

On Thu, Mar 13, 2025 at 09:19:03AM +0200, Matti Vaittinen wrote:

...

Picking out a few things to comment on...

Quoted 31 lines

Quoted 15 lines

Quoted 6 lines

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

Quoted 4 lines

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

+#define BD79124_MASK_HYSTERESIS GENMASK(3, 0)
+#define BD79124_LOW_LIMIT_MIN 0
+#define BD79124_HIGH_LIMIT_MAX GENMASK(11, 0)    

These masks are not named after registers (or I don't see it clearly),

Naming is hard. I usually try to make a balance between:

1) using names which explain the purpose when seen in the code (at call site)
2) keeping names short enough
3) following the naming convention in the data sheet
4) maintain relation to the register.

Quoted 7 lines

I put most emphasis to the 1, while 2 is important to me as well. 3 is _very_ nice to have, but it is often somehow contradicting with 1 and 2. 4 is IMO just nice to have. The register is usually clearly visible at call site, and if someone adds new functionality (or checks the correctness of the masks/registers), then 3 is way more important.

I am open to any concrete suggestions though.

From my point of view the starting point is 3, then one may apply 2 and 4, the 1 may mangle the name so much that register data field name becomes quite abstract.

Quoted 4 lines

Quoted 3 lines

it's
hard to understand which one relates to which register. Also, why not using bitfield.h?

Quoted 4 lines

Quoted 1 line

I saw no need for it?

Hmm... Okay, I think Jonathan will ask that if really needed.

I won't as I'm not a huge fan of bitfield.h. In many cases they bloat the code

and increase the writes that go over the bus. Don't get me wrong, there are good usecases, but it's not a universal thing (unlike PREP_FIELD() etc which I love :)

I do favour burning a few characters to make field / register relationship clear though. A few things can help I think.

Structuring defines and naming:
I like using whitespace in subtle ways for this.

#define PREFIX_REGNAME_REG 0x00
#define PREFIX_REGNAME_FIELDNAME_MSK GENMASK(X, Y)
#define PREFIX_REGNAME_FIELDNAME_FILEVALNAME 0x3
etc

Makes it easy to see if we have a mismatch going on

However, I don't insist on this in all cases as it is one of those "don't let perfect be the enemy of good" cases I think.

So Matti, good to have one last look at the defines and see if they can be wrangled into a slightly better form.

.

Quoted 7 lines

...

Quoted 5 lines

Quoted 4 lines

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

Quoted 2 lines

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

+static void bd79124gpo_set_multiple(struct gpio_chip *gc, unsigned long *mask,
+				    unsigned long *bits)    

Ditto, .set_multiple_rv().

Quoted 7 lines

Quoted 2 lines

As you know, this started at v6.14 cycle which is still ongoing. I don't think .set_rv() and .set_multiple_rv() are available?

You mean that you are still hope to have this series to be squeezed for v6.15-rc1? That's not me who decides, but if not, those APIs will be part of the v6.15-rc1 (at least they are pending in GPIO subsystem).

I'd vote for a rebase on rc1 that should be really easy to for me to pick
up. I'd accept a follow up series though. Ultimately won't affect
when this series lands as very unlikely Linus will delay the release long enough for me to do another pull request this cycle,

Jonathan

Quoted 5 lines · On Fri, 14 Mar 2025 16:33:13 +0200 Andy Shevchenko…

Quoted 2 lines · Matti Vaittinen

Quoted 1 line · Andy Shevchenko

On Thu, Mar 13, 2025 at 09:19:03AM +0200, Matti Vaittinen wrote:

...

Picking out a few things to comment on...

Quoted 27 lines

Quoted 15 lines

Quoted 6 lines

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

Quoted 4 lines

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

+#define BD79124_MASK_HYSTERESIS GENMASK(3, 0)
+#define BD79124_LOW_LIMIT_MIN 0
+#define BD79124_HIGH_LIMIT_MAX GENMASK(11, 0)

These masks are not named after registers (or I don't see it clearly),

Naming is hard. I usually try to make a balance between:

1) using names which explain the purpose when seen in the code (at call site)
2) keeping names short enough
3) following the naming convention in the data sheet
4) maintain relation to the register.

Quoted 7 lines

I put most emphasis to the 1, while 2 is important to me as well. 3 is _very_ nice to have, but it is often somehow contradicting with 1 and 2. 4 is IMO just nice to have. The register is usually clearly visible at call site, and if someone adds new functionality (or checks the correctness of the masks/registers), then 3 is way more important.

I am open to any concrete suggestions though.

From my point of view the starting point is 3, then one may apply 2 and 4,
the 1 may mangle the name so much that register data field name becomes quite abstract.

Quoted 8 lines

Quoted 5 lines

Quoted 3 lines

it's
hard to understand which one relates to which register. Also, why not using bitfield.h?

I saw no need for it?

Hmm... Okay, I think Jonathan will ask that if really needed.

I won't as I'm not a huge fan of bitfield.h. In many cases they bloat the code and increase the writes that go over the bus. Don't get me wrong, there are good usecases, but it's not a universal thing (unlike PREP_FIELD() etc which I love :)

I do favour burning a few characters to make field / register relationship clear though. A few things can help I think.

Structuring defines and naming:
I like using whitespace in subtle ways for this.

#define PREFIX_REGNAME_REG 0x00
#define PREFIX_REGNAME_FIELDNAME_MSK GENMASK(X, Y)
#define PREFIX_REGNAME_FIELDNAME_FILEVALNAME 0x3
etc

Makes it easy to see if we have a mismatch going on

However, I don't insist on this in all cases as it is one of those "don't let perfect be the enemy of good" cases I think.

So Matti, good to have one last look at the defines and see if they can be wrangled into a slightly better form.

Quoted 10 lines

...

Quoted 7 lines

Quoted 4 lines

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

Quoted 2 lines

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

+static void bd79124gpo_set_multiple(struct gpio_chip *gc, unsigned long *mask,
+				    unsigned long *bits)

Ditto, .set_multiple_rv().

As you know, this started at v6.14 cycle which is still ongoing. I don't think .set_rv() and .set_multiple_rv() are available?

Quoted 4 lines

You mean that you are still hope to have this series to be squeezed for v6.15-rc1? That's not me who decides, but if not, those APIs will be part of the v6.15-rc1 (at least they are pending in GPIO subsystem).

I'd vote for a rebase on rc1 that should be really easy to for me to pick up.

I'd accept a follow up series though. Ultimately won't affect
when this series lands as very unlikely Linus will delay the release long enough for me to do another pull request this cycle,

Quoted 53 lines · Jonathan Cameron

Quoted 5 lines · On Fri, 14 Mar 2025 16:33:13 +0200 Andy Shevchenko…

Quoted 2 lines · Matti Vaittinen

Quoted 1 line · Andy Shevchenko

On Thu, Mar 13, 2025 at 09:19:03AM +0200, Matti Vaittinen wrote:

...

Picking out a few things to comment on...

Quoted 27 lines

Quoted 15 lines

Quoted 6 lines

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

Quoted 4 lines

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

+#define BD79124_MASK_HYSTERESIS GENMASK(3, 0)
+#define BD79124_LOW_LIMIT_MIN 0
+#define BD79124_HIGH_LIMIT_MAX GENMASK(11, 0)  

These masks are not named after registers (or I don't see it clearly),

Naming is hard. I usually try to make a balance between:

1) using names which explain the purpose when seen in the code (at call site)
2) keeping names short enough
3) following the naming convention in the data sheet
4) maintain relation to the register.

Quoted 7 lines

I put most emphasis to the 1, while 2 is important to me as well. 3 is _very_ nice to have, but it is often somehow contradicting with 1 and 2. 4 is IMO just nice to have. The register is usually clearly visible at call site, and if someone adds new functionality (or checks the correctness of the masks/registers), then 3 is way more important.

I am open to any concrete suggestions though.

From my point of view the starting point is 3, then one may apply 2 and 4,
the 1 may mangle the name so much that register data field name becomes quite abstract.

Quoted 8 lines

Quoted 5 lines

Quoted 3 lines

it's
hard to understand which one relates to which register. Also, why not using bitfield.h?

I saw no need for it?

Hmm... Okay, I think Jonathan will ask that if really needed.

I won't as I'm not a huge fan of bitfield.h. In many cases they bloat the code and increase the writes that go over the bus. Don't get me wrong, there are good usecases, but it's not a universal thing (unlike PREP_FIELD() etc which I love :)

I do favour burning a few characters to make field / register relationship clear though. A few things can help I think.

Quoted 7 lines

Structuring defines and naming:
I like using whitespace in subtle ways for this.

#define PREFIX_REGNAME_REG 0x00
#define PREFIX_REGNAME_FIELDNAME_MSK GENMASK(X, Y)
#define PREFIX_REGNAME_FIELDNAME_FILEVALNAME 0x3
etc

This is close to my usual way, but not quite. I most often do:

PREFIX_REG_REGNAME
PREFIX_MASK_FIELDNAME
PREFIX_FIELDNAME_FIELDVALUE.

Problem with
PREFIX_REGNAME_FIELDNAME_MSK
compared to
PREFIX_MASK_FIELDNAME

tends to be the length of the define. Also, sometimes registers contain field(s) which have nothing to do with the register name. Sometimes it results confusing definitions. (Confusing meaning that seeing the actual read/write makes one to wonder what the register field is supposed to do).

Quoted 8 lines

Makes it easy to see if we have a mismatch going on

However, I don't insist on this in all cases as it is one of those "don't let perfect be the enemy of good" cases I think.

So Matti, good to have one last look at the defines and see if they can be wrangled into a slightly better form.

I can at least change the MASK to MSK and save one letter :) What comes to the ordering, I see you prefer having MSK / REG suffix at the end - while I like having it right after the prefix (just because it makes the MSK / REG to stay aligned - which in my eyes looks a little bit better). So, I'm not sure if I change it to your preference (which may end up being more common in IIO if it's what you prefer), or if I keep it the way I am used to.

Quoted 11 lines

Quoted 11 lines

...

Quoted 7 lines

Quoted 4 lines

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

Quoted 2 lines

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

+static void bd79124gpo_set_multiple(struct gpio_chip *gc, unsigned long *mask,
+				    unsigned long *bits)  

Ditto, .set_multiple_rv().

As you know, this started at v6.14 cycle which is still ongoing. I don't think .set_rv() and .set_multiple_rv() are available?

Quoted 6 lines

Quoted 4 lines

You mean that you are still hope to have this series to be squeezed for v6.15-rc1? That's not me who decides, but if not, those APIs will be part of the v6.15-rc1 (at least they are pending in GPIO subsystem).

I'd vote for a rebase on rc1 that should be really easy to for me to pick up.

I sure can rebase if the series is not merged before the rc1 is out. I, however, rather not delay versions (unless explicitly asked to).

Sometimes postponing things to wait dependencies to get merged backfires due to some 'last minute' delays. Hence, I don't usually adapt to new APIs until they are in the rc1 or target subsystem's 'for-next' (or other suitable) -branch.

Quoted 4 lines

I'd accept a follow up series though. Ultimately won't affect
when this series lands as very unlikely Linus will delay the release long enough for me to do another pull request this cycle,

No problem. You'll pick this when you think it's ready - and I'll rebase if new rc1 is out before that (and convert to set_rv() and set_multiple_rv() if they are included in the rc1).

If you merge before the set_rv() and set_multiple_rv() are in your tree, then a follow-up will be done when they emerge. :)

Yours,
-- Matti

Quoted 9 lines · Matti Vaittinen

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

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

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

Quoted 5 lines

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

+		return IRQ_NONE;
+
+	ret = regmap_read(data->map, BD79124_REG_EVENT_FLAG_LO, &i_lo);
+	if (ret)
+		return IRQ_NONE;

Only I don't get why you can't use bulk read here. The registers seem to be sequential.

Quoted 39 lines

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

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

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

Quoted 6 lines

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

+	if (ret)
+		return IRQ_NONE;
+
+	ret = regmap_write(data->map, BD79124_REG_EVENT_FLAG_LO, i_lo);
+	if (ret)
+		return IRQ_NONE;

In the similar way bulk write.

Quoted 2 lines

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

+	return IRQ_HANDLED;
+}

Quoted 1 line · Andy Shevchenko

On Thu, Mar 13, 2025 at 09:19:03AM +0200, Matti Vaittinen wrote:

Quoted 10 lines

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

Quoted 7 lines

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

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

Only I don't get why you can't use bulk read here. The registers seem to be sequential.

After taking another look - there seems to be undocumented register (0x1b) between the BD79124_REG_EVENT_FLAG_HI (0x1a) and the BD79124_REG_EVENT_FLAG_LO (0x1c).

I won't touch it unless there is a real verified performance problem.

Quoted 1 line

In the similar way bulk write.

definitely not due to the 0x1b.

Quoted 2 lines · Matti Vaittinen

Quoted 1 line · Andy Shevchenko

On Thu, Mar 13, 2025 at 09:19:03AM +0200, Matti Vaittinen wrote:

...

Quoted 16 lines

Quoted 10 lines

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

Quoted 7 lines

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

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

Only I don't get why you can't use bulk read here. The registers seem to be sequential.

After taking another look - there seems to be undocumented register (0x1b) between the BD79124_REG_EVENT_FLAG_HI (0x1a) and the BD79124_REG_EVENT_FLAG_LO (0x1c).

I won't touch it unless there is a real verified performance problem.

...

Quoted 3 lines

Quoted 1 line

In the similar way bulk write.

definitely not due to the 0x1b.

Okay, it seems I misinterpreted the values you have in regmap configuration, I was under the impression that regmap is 16-bit data, but it is about address.

So, we need to know why the heck HW has sparse registers

for what is supposed
to be sequential. This needs a good comment.

Quoted 1 line · Andy Shevchenko

On Fri, Mar 14, 2025 at 11:22:37AM +0200, Matti Vaittinen wrote:

Quoted 4 lines

Okay, it seems I misinterpreted the values you have in regmap configuration, I was under the impression that regmap is 16-bit data, but it is about address.

So, we need to know why the heck HW has sparse registers

We don't know. And we really don't even need to know it. We can just use this device knowing there are some.

Quoted 2 lines

for what is supposed
to be sequential. This needs a good comment.

I think it is quite usual that devices contain undocumented registers. Not sure having a comment that this device also has some, adds much of value? I suppose I can add a comment that we can't use bulk_write because registers aren't in subsequent addresses - but having just removed bunch of unnecessary comments from the code (as requested), I'm not sure adding this one really improves situation... When one sees separate reads/writes for data spread to multiple registers, he is likely to assume addresses aren't subsequent.

Quoted 2 lines · Matti Vaittinen

Quoted 1 line · Andy Shevchenko

On Fri, Mar 14, 2025 at 11:22:37AM +0200, Matti Vaittinen wrote:

...

Quoted 3 lines

Quoted 3 lines

Okay, it seems I misinterpreted the values you have in regmap configuration, I was under the impression that regmap is 16-bit data, but it is about address.

Quoted 4 lines

Quoted 1 line

So, we need to know why the heck HW has sparse registers

We don't know. And we really don't even need to know it. We can just use this device knowing there are some.

Don't you have a channel to ask HW engineers about this?

Quoted 11 lines

Quoted 2 lines

for what is supposed
to be sequential. This needs a good comment.

I think it is quite usual that devices contain undocumented registers. Not sure having a comment that this device also has some, adds much of value? I suppose I can add a comment that we can't use bulk_write because registers aren't in subsequent addresses - but having just removed bunch of unnecessary comments from the code (as requested), I'm not sure adding this one really improves situation... When one sees separate reads/writes for data spread to multiple registers, he is likely to assume addresses aren't subsequent.

From HW design perspective it's silly to sparse hi and lo part of
the semantically same entity. So, either the (undocumented) register is also part of the soup, or the registers have different semantics.

This what needs to be commented in my opinion.

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

+/*
+ * The high and low limits as well as the recent result values are stored in
+ * the same way in 2 consequent registers. The first register contains 4 bits
+ * of the value. These bits are stored in the high bits [7:4] of register, but
+ * they represent the low bits [3:0] of the value.
+ * The value bits [11:4] are stored in the next regoster.
+ *
+ * Conver the integer to register format and write it using rmw cycle.

+ */

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

+static int bd79124_write_int_to_reg(struct bd79124_data *data, int reg,
+				    unsigned int val)

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

+}
 
 
 
+static int bd79124_probe(struct i2c_client *i2c)
+{

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

+
+	ret = devm_gpiochip_add_data(dev, &data->gc, data);
+	if (ret)
+		return dev_err_probe(dev, ret, "gpio init Failed\n");
+
+	if (i2c->irq > 0) {
+		ret = devm_request_threaded_irq(dev, i2c->irq,
+			bd79124_irq_handler, &bd79124_event_handler,
+			IRQF_ONESHOT, "adc-thresh-alert", iio_dev);
+		if (ret)
+			return dev_err_probe(data->dev, ret,
+					     "Failed to register IRQ\n");
+	}
+
+	return devm_iio_device_register(data->dev, iio_dev);
+}

Quoted 9 lines · On Thu, 13 Mar 2025 09:19:03 +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.

Quoted 1 line

Thus the driver does not support the BD79124's manual measurement mode

Given you are going to be doing a v8 and I'm bored on a train, so utterly trivial comments that you get as a frequent contributor as things to consider for future patches. (I'm sure it's just what you always wanted :)

In theory should be imperative though I don't care as much as some.

Hence, do not support the....

Quoted 5 lines

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.

Trivial editorial comment: that 'does' is not providing anything use in modern English (might have done in the past, no idea!)

"Also support configure the threshold..."

Quoted 11 lines

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

A few trivial things inline.

Jonathan

Quoted 8 lines

+/*
+ * The high and low limits as well as the recent result values are stored in
+ * the same way in 2 consequent registers. The first register contains 4 bits
+ * of the value. These bits are stored in the high bits [7:4] of register, but
+ * they represent the low bits [3:0] of the value.
+ * The value bits [11:4] are stored in the next regoster.
+ *
+ * Conver the integer to register format and write it using rmw cycle.

Convert?

On Thu, 13 Mar 2025 09:19:03 +0200
Matti Vaittinen <[email protected]> wrote:

Quoted 2 lines

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

+static int bd79124_write_int_to_reg(struct bd79124_data *data, int reg,
+				    unsigned int val)

..

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

Quoted 11 lines

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

+static int bd79124_read_event_config(struct iio_dev *iio_dev,
+				     const struct iio_chan_spec *chan,
+				     enum iio_event_type type,
+				     enum iio_event_direction dir)
+{
+	struct bd79124_data *data = iio_priv(iio_dev);
+
+	if (chan->channel >= BD79124_MAX_NUM_CHANNELS)
+		return -EINVAL;
+
+	return (data->alarm_monitored[chan->channel] & BIT(dir));

Drop the outer brackets as not adding anything.

Quoted 2 lines · Jonathan Cameron

On Thu, 13 Mar 2025 09:19:03 +0200
Matti Vaittinen <[email protected]> wrote:

...

Quoted 16 lines

Quoted 2 lines

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

+static int bd79124_write_int_to_reg(struct bd79124_data *data, int reg,
+				    unsigned int val)  

..

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

Quoted 11 lines

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

+static int bd79124_read_event_config(struct iio_dev *iio_dev,
+				     const struct iio_chan_spec *chan,
+				     enum iio_event_type type,
+				     enum iio_event_direction dir)
+{
+	struct bd79124_data *data = iio_priv(iio_dev);
+
+	if (chan->channel >= BD79124_MAX_NUM_CHANNELS)
+		return -EINVAL;
+
+	return (data->alarm_monitored[chan->channel] & BIT(dir));  

Drop the outer brackets as not adding anything.

I just noticed that the integer returned from here is directly provided to the user-space. I don't know the history, but it feels a bit off to me. I mean, I would expect the read from sysfs file "*_en" to return '1' or '0' - not 0x04.

Oh well, I suppose it's too late to change this in the IIO core - but I'll do:

	return !!(data->alarm_monitored[chan->channel] & BIT(dir));

in v8.

Yours,
-- Matti

Quoted 3 lines · On Mon, 17 Mar 2025 13:24:07 +0200 Matti Vaittinen <[email protected]>…

Quoted 2 lines · Jonathan Cameron

On Thu, 13 Mar 2025 09:19:03 +0200
Matti Vaittinen <[email protected]> wrote:

Quoted 4 lines

Quoted 3 lines

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

Quoted 1 line

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

+	return (data->alarm_monitored[chan->channel] & BIT(dir));  

Drop the outer brackets as not adding anything.

Quoted 8 lines

I just noticed that the integer returned from here is directly provided to the user-space. I don't know the history, but it feels a bit off to me. I mean, I would expect the read from sysfs file "*_en" to return '1' or '0' - not 0x04.

Oh well, I suppose it's too late to change this in the IIO core - but I'll do:

	return !!(data->alarm_monitored[chan->channel] & BIT(dir));

Agreed it should be returning 1 or 0.

This stuff is a little bit messy. I'd not be against that ABI cleanup if we squashed the values to 0,1 in the core as a follow up.

I doubt anyone relies on getting 0x4 as that would be very driver specific userspace code!

Quoted 5 lines · Jonathan Cameron

Quoted 3 lines · On Mon, 17 Mar 2025 13:24:07 +0200 Matti Vaittinen <[email protected]>…

Quoted 2 lines · Jonathan Cameron

On Thu, 13 Mar 2025 09:19:03 +0200
Matti Vaittinen <[email protected]> wrote:

...

Quoted 18 lines

Quoted 4 lines

Quoted 3 lines

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

Quoted 1 line

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

+	return (data->alarm_monitored[chan->channel] & BIT(dir));

Drop the outer brackets as not adding anything.

Quoted 8 lines

I just noticed that the integer returned from here is directly provided to the user-space. I don't know the history, but it feels a bit off to me. I mean, I would expect the read from sysfs file "*_en" to return '1' or '0' - not 0x04.

Oh well, I suppose it's too late to change this in the IIO core - but I'll do:

	return !!(data->alarm_monitored[chan->channel] & BIT(dir));

Agreed it should be returning 1 or 0.

This stuff is a little bit messy. I'd not be against that ABI cleanup if we squashed the values to 0,1 in the core as a follow up.

Quoted 2 lines

I doubt anyone relies on getting 0x4 as that would be very driver specific userspace code!

FWIW, the time this discussion was held, I looked at the code and it seems we have such drivers. I doubt we can change it without potential breakage of (some) user space tools.

We can avoid open-coding the loop construct which counts firmware child nodes with a specific name by using the newly added device_get_child_node_count_named().

The gianfar driver has such open-coded loop. Replace it with the device_get_child_node_count_named().