[PATCH v4 0/3] iio: adc: ti-ads112c14: continuous mode support

[PATCH v4 0/3] iio: adc: ti-ads112c14: continuous mode support

[David Lechner (TI)]

The TI ADS112C14 ADC has a continuous mode where it can trigger
conversions using its own clock and signals that data is ready using
the /DRDY line. This allows capturing data at higher rates since it
avoids the settling time between conversions. But it is limited to only
being able to sample one channel at a time.

Users will be able to select between continuous and single-shot mode
for buffered reads by selecting the trigger. If the "own" trigger from
the /DRDY interrupt is selected, continuous mode will be used. Or if
some other (software) trigger is selected, single-shot mode will be used
as was already implemented before this series.

Supporting open-drain /DRDY is deferred for future work. In simple cases
we have just done a `drive-open-drain` DT property for this, but on this
chip there is more than one pin (GPIO, FAULT interrupt) that could also
need to be open-drain and not all at the same time.

Signed-off-by: David Lechner (TI) <dlechner@baylibre.com>
---
Changes in v4:
- Check for the "interrupt-names" property instead of "interrupts" to
  detect the /DRDY interrupt so that it works on ACPI systems too.
- Add ads112c14_first_active_channel() helper instead of duplicating the
  find_first_bit() lookup.
- Use regmap_update_bits() with FIELD_PREP() instead of
  regmap_assign_bits() for the CONV_MODE field.
- Link to v3: https://patch.msgid.link/20260807-iio-adc-ti-ads112c14-continuous-mode-v3-0-76e0d30e6c6b@baylibre.com

Changes in v3:
- Leave DRDY interrupt always enabled.
- Link to v2: https://patch.msgid.link/20260731-iio-adc-ti-ads112c14-continuous-mode-v2-0-eb13da38e8fc@baylibre.com

Changes in v2:
- Split out irq and poll code paths into separate functions.
- Split refactoring into separate patch.
- Use iio_validate_scan_mask_onehot().
- Just return immediately on error in buffer_predisable().
- Don't set default trigger.
- Code style improvements.
- Fixed null pointer dereference bug when no trigger.
- Link to v1: https://patch.msgid.link/20260724-iio-adc-ti-ads112c14-continuous-mode-v1-0-9eb0b7a4f020@baylibre.com

---
David Lechner (TI) (3):
      iio: adc: ti-ads112c14: add DRDY interrupt support
      iio: adc: ti-ads112c14: create data read helper functions
      iio: adc: ti-ads112c14: add continuous mode support

 drivers/iio/adc/ti-ads112c14.c | 288 ++++++++++++++++++++++++++++++++++++-----
 1 file changed, 259 insertions(+), 29 deletions(-)
---
base-commit: 22359083a9e74d538ce383d5c0ee30cc20182187
change-id: 20260724-iio-adc-ti-ads112c14-continuous-mode-dd1580754609

Best regards,
--  
David Lechner (TI) <dlechner@baylibre.com>

[PATCH v4 1/3] iio: adc: ti-ads112c14: add DRDY interrupt support

[David Lechner (TI)]

Add handling for the DRDY interrupt to wait for data ready events rather
than polling (only when it is wired up).

Signed-off-by: David Lechner (TI) <dlechner@baylibre.com>
---

Small note: the hard-coded 100 ms timeout will be replaced in a future
series with a dynamic value, so I didn't bother with a macro or comments
to explain why the value was chosen.

And passing indio_dev instead of data to irq is intentional as it will
be used in the next patch.
---
 drivers/iio/adc/ti-ads112c14.c | 100 ++++++++++++++++++++++++++++++++++++-----
 1 file changed, 90 insertions(+), 10 deletions(-)

diff --git a/drivers/iio/adc/ti-ads112c14.c b/drivers/iio/adc/ti-ads112c14.c
index 60eab4852ba2..63b819339788 100644
--- a/drivers/iio/adc/ti-ads112c14.c
+++ b/drivers/iio/adc/ti-ads112c14.c
@@ -10,6 +10,7 @@
 
 #include <linux/bitfield.h>
 #include <linux/cleanup.h>
+#include <linux/completion.h>
 #include <linux/crc8.h>
 #include <linux/delay.h>
 #include <linux/dev_printk.h>
@@ -19,6 +20,7 @@
 #include <linux/iio/iio.h>
 #include <linux/iio/trigger_consumer.h>
 #include <linux/iio/triggered_buffer.h>
+#include <linux/interrupt.h>
 #include <linux/math64.h>
 #include <linux/minmax.h>
 #include <linux/module.h>
@@ -117,9 +119,15 @@
 #define   ADS112C14_GPIO_CFG_GPIO2_CFG			GENMASK(5, 4)
 #define   ADS112C14_GPIO_CFG_GPIO1_CFG			GENMASK(3, 2)
 #define   ADS112C14_GPIO_CFG_GPIO0_CFG			GENMASK(1, 0)
+#define     ADS112C14_GPIO_CFG_GPIO_CFG_DISABLED	  0
+#define     ADS112C14_GPIO_CFG_GPIO_CFG_INPUT		  1
+#define     ADS112C14_GPIO_CFG_GPIO_CFG_OUTPUT_PUSH_PULL  2
+#define     ADS112C14_GPIO_CFG_GPIO_CFG_OUTPUT_OPEN_DRAIN 3
 
 #define ADS112C14_REG_GPIO_DATA_OUTPUT			0x0C
 #define   ADS112C14_GPIO_DATA_OUTPUT_GPIO3_SRC		BIT(7)
+#define     ADS112C14_GPIO_DATA_OUTPUT_GPIO3_SRC_DAT_OUT  0
+#define     ADS112C14_GPIO_DATA_OUTPUT_GPIO3_SRC_DRDY	  1
 #define   ADS112C14_GPIO_DATA_OUTPUT_GPIO2_SRC		BIT(6)
 #define   ADS112C14_GPIO_DATA_OUTPUT_GPIO3_DAT_OUT	BIT(3)
 #define   ADS112C14_GPIO_DATA_OUTPUT_GPIO2_DAT_OUT	BIT(2)
@@ -251,6 +259,8 @@ struct ads112c14_data {
 	struct regmap *regmap;
 	/* Synchronizes access to register value fields. */
 	struct mutex lock;
+	int drdy_irq;
+	struct completion drdy_completion;
 	bool i2c_crc_enabled;
 	u32 avdd_uV;
 	u32 ext_ref_uV;
@@ -265,6 +275,16 @@ struct ads112c14_data {
 						 ARRAY_SIZE(ads112c14_sys_mon_channels));
 };
 
+static irqreturn_t ads112c14_drdy_irq_handler(int irq, void *private)
+{
+	struct iio_dev *indio_dev = private;
+	struct ads112c14_data *data = iio_priv(indio_dev);
+
+	complete(&data->drdy_completion);
+
+	return IRQ_HANDLED;
+}
+
 static bool ads112c14_writeable_reg(struct device *dev, unsigned int reg)
 {
 	switch (reg) {
@@ -581,12 +601,45 @@ static int ads112c14_prepare_sys_mon_channel(struct ads112c14_data *data,
 	return 0;
 }
 
+static int ads112c14_wait_for_conversion_irq(struct ads112c14_data *data)
+{
+	unsigned long remaining;
+	int ret;
+
+	reinit_completion(&data->drdy_completion);
+
+	ret = regmap_write(data->regmap, ADS112C14_REG_CONVERSION_CTRL,
+			   ADS112C14_CONVERSION_CTRL_START);
+	if (ret)
+		return ret;
+
+	remaining = wait_for_completion_timeout(&data->drdy_completion,
+						msecs_to_jiffies(100));
+
+	return remaining ? 0 : -ETIMEDOUT;
+}
+
+static int ads112c14_wait_for_conversion_poll(struct ads112c14_data *data)
+{
+	u32 reg_val;
+	int ret;
+
+	ret = regmap_write(data->regmap, ADS112C14_REG_CONVERSION_CTRL,
+			   ADS112C14_CONVERSION_CTRL_START);
+	if (ret)
+		return ret;
+
+	return regmap_read_poll_timeout(data->regmap,
+					ADS112C14_REG_STATUS_MSB, reg_val,
+					FIELD_GET(ADS112C14_STATUS_MSB_DRDY, reg_val),
+					1 * USEC_PER_MSEC, 100 * USEC_PER_MSEC);
+}
+
 static int ads112c14_single_conversion(struct ads112c14_data *data,
 				       const struct iio_chan_spec *chan,
 				       u8 *buf, bool for_scan)
 {
 	struct i2c_client *client = to_i2c_client(regmap_get_device(data->regmap));
-	u32 reg_val;
 	int ret;
 
 	guard(mutex)(&data->lock);
@@ -601,15 +654,10 @@ static int ads112c14_single_conversion(struct ads112c14_data *data,
 			return ret;
 	}
 
-	ret = regmap_write(data->regmap, ADS112C14_REG_CONVERSION_CTRL,
-			   ADS112C14_CONVERSION_CTRL_START);
-	if (ret)
-		return ret;
-
-	ret = regmap_read_poll_timeout(data->regmap,
-				       ADS112C14_REG_STATUS_MSB, reg_val,
-				       FIELD_GET(ADS112C14_STATUS_MSB_DRDY, reg_val),
-				       1 * USEC_PER_MSEC, 100 * USEC_PER_MSEC);
+	if (data->drdy_irq)
+		ret = ads112c14_wait_for_conversion_irq(data);
+	else
+		ret = ads112c14_wait_for_conversion_poll(data);
 	if (ret)
 		return ret;
 
@@ -1391,6 +1439,38 @@ static int ads112c14_probe(struct i2c_client *client)
 	if (ret)
 		return ret;
 
+	if (device_property_present(dev, "interrupt-names")) {
+		data->drdy_irq = fwnode_irq_get_byname(dev_fwnode(dev), "drdy");
+		if (data->drdy_irq < 0)
+			return dev_err_probe(dev, data->drdy_irq,
+					     "failed to get drdy interrupt\n");
+
+		/*
+		 * REVISIT: would probably need to implement a pin controller in
+		 * order to support open drain option here.
+		 */
+		ret = regmap_update_bits(data->regmap, ADS112C14_REG_GPIO_CFG,
+					 ADS112C14_GPIO_CFG_GPIO3_CFG,
+					 FIELD_PREP(ADS112C14_GPIO_CFG_GPIO3_CFG,
+						    ADS112C14_GPIO_CFG_GPIO_CFG_OUTPUT_PUSH_PULL));
+		if (ret)
+			return ret;
+
+		ret = regmap_update_bits(data->regmap, ADS112C14_REG_GPIO_DATA_OUTPUT,
+					 ADS112C14_GPIO_DATA_OUTPUT_GPIO3_SRC,
+					 FIELD_PREP(ADS112C14_GPIO_DATA_OUTPUT_GPIO3_SRC,
+						    ADS112C14_GPIO_DATA_OUTPUT_GPIO3_SRC_DRDY));
+		if (ret)
+			return ret;
+
+		init_completion(&data->drdy_completion);
+
+		ret = devm_request_irq(dev, data->drdy_irq, ads112c14_drdy_irq_handler,
+				       0, dev_name(dev), indio_dev);
+		if (ret)
+			return ret;
+	}
+
 	ads112c14_populate_tables(data);
 
 	indio_dev->name = info->name;

-- 
2.43.0

Re: [PATCH v4 1/3] iio: adc: ti-ads112c14: add DRDY interrupt support

[Jonathan Cameron]

> Add handling for the DRDY interrupt to wait for data ready events rather
> than polling (only when it is wired up).
> 
> Signed-off-by: David Lechner (TI) <dlechner@baylibre.com>

I'm messing with "b4 review tui" to see if it works well for me and
given it has nice sashiko integration, we have comments from there
inline.  To me they all seem fine.

One request - I think we should add a little more detail
to the dt-binding about interrupt types that make sense.
That can be a follow up though if this series is otherwise ready
to go.

>
> diff --git a/drivers/iio/adc/ti-ads112c14.c b/drivers/iio/adc/ti-ads112c14.c
> index 60eab4852ba2..63b819339788 100644
> --- a/drivers/iio/adc/ti-ads112c14.c
> +++ b/drivers/iio/adc/ti-ads112c14.c
> @@ -10,6 +10,7 @@
>  
>  #include <linux/bitfield.h>
>  #include <linux/cleanup.h>
> +#include <linux/completion.h>
>  #include <linux/crc8.h>
>  #include <linux/delay.h>
>  #include <linux/dev_printk.h>
> @@ -19,6 +20,7 @@
>  #include <linux/iio/iio.h>
>  #include <linux/iio/trigger_consumer.h>
>  #include <linux/iio/triggered_buffer.h>
> +#include <linux/interrupt.h>
>  #include <linux/math64.h>
>  #include <linux/minmax.h>
>  #include <linux/module.h>
> @@ -117,9 +119,15 @@
>  #define   ADS112C14_GPIO_CFG_GPIO2_CFG			GENMASK(5, 4)
>  #define   ADS112C14_GPIO_CFG_GPIO1_CFG			GENMASK(3, 2)
>  #define   ADS112C14_GPIO_CFG_GPIO0_CFG			GENMASK(1, 0)
> +#define     ADS112C14_GPIO_CFG_GPIO_CFG_DISABLED	  0
> +#define     ADS112C14_GPIO_CFG_GPIO_CFG_INPUT		  1
> +#define     ADS112C14_GPIO_CFG_GPIO_CFG_OUTPUT_PUSH_PULL  2
> +#define     ADS112C14_GPIO_CFG_GPIO_CFG_OUTPUT_OPEN_DRAIN 3
>  
>  #define ADS112C14_REG_GPIO_DATA_OUTPUT			0x0C
>  #define   ADS112C14_GPIO_DATA_OUTPUT_GPIO3_SRC		BIT(7)
> +#define     ADS112C14_GPIO_DATA_OUTPUT_GPIO3_SRC_DAT_OUT  0
> +#define     ADS112C14_GPIO_DATA_OUTPUT_GPIO3_SRC_DRDY	  1
>  #define   ADS112C14_GPIO_DATA_OUTPUT_GPIO2_SRC		BIT(6)
>  #define   ADS112C14_GPIO_DATA_OUTPUT_GPIO3_DAT_OUT	BIT(3)
>  #define   ADS112C14_GPIO_DATA_OUTPUT_GPIO2_DAT_OUT	BIT(2)
> @@ -251,6 +259,8 @@ struct ads112c14_data {
>  	struct regmap *regmap;
>  	/* Synchronizes access to register value fields. */
>  	struct mutex lock;
> +	int drdy_irq;
> +	struct completion drdy_completion;
>  	bool i2c_crc_enabled;
>  	u32 avdd_uV;
>  	u32 ext_ref_uV;
> @@ -265,6 +275,16 @@ struct ads112c14_data {
>  						 ARRAY_SIZE(ads112c14_sys_mon_channels));
>  };

>  static bool ads112c14_writeable_reg(struct device *dev, unsigned int reg)
>  {
>  	switch (reg) {
> @@ -581,12 +601,45 @@ static int ads112c14_prepare_sys_mon_channel(struct ads112c14_data *data,
>  	return 0;
>  }
>  
> +static int ads112c14_wait_for_conversion_irq(struct ads112c14_data *data)
> +{
> +	unsigned long remaining;
> +	int ret;
> +
> +	reinit_completion(&data->drdy_completion);
> +
> +	ret = regmap_write(data->regmap, ADS112C14_REG_CONVERSION_CTRL,
> +			   ADS112C14_CONVERSION_CTRL_START);

 | sashiko.dev <sashiko@sashiko.dev>:
 |
 | [Severity: Medium]
 | Could this allow a stale DRDY interrupt from a previous timed-out conversion
 | to falsely complete the wait?
 |
 | Since regmap_write() performs a slow I2C transaction that can sleep, if a
 | previous conversion had timed out, the device might complete it and assert
 |  the DRDY interrupt exactly during this regmap_write(). The interrupt handler
 | would then call complete(), causing the subsequent
 | wait_for_completion_timeout() to return immediately and the driver to read
 | stale data.
 |
 | via: https://sashiko.dev/#/message/20260827-iio-adc-ti-ads112c14-continuous-mode-v4-1-1e51a6e20a69@baylibre.com

For this I don't think we care.  It is unlikely to happen and the chance
of getting stale data in that corner case probably doesn't bother us.

> +	if (ret)
> +		return ret;
> +
> +	remaining = wait_for_completion_timeout(&data->drdy_completion,
> +						msecs_to_jiffies(100));
> +
> +	return remaining ? 0 : -ETIMEDOUT;
> +}
> +
> +static int ads112c14_wait_for_conversion_poll(struct ads112c14_data *data)
> +{
> +	u32 reg_val;
> +	int ret;
> +
> +	ret = regmap_write(data->regmap, ADS112C14_REG_CONVERSION_CTRL,
> +			   ADS112C14_CONVERSION_CTRL_START);
> +	if (ret)
> +		return ret;
> +
> +	return regmap_read_poll_timeout(data->regmap,
> +					ADS112C14_REG_STATUS_MSB, reg_val,
> +					FIELD_GET(ADS112C14_STATUS_MSB_DRDY, reg_val),
> +					1 * USEC_PER_MSEC, 100 * USEC_PER_MSEC);
> +}
> +
>  static int ads112c14_single_conversion(struct ads112c14_data *data,
>  				       const struct iio_chan_spec *chan,
>  				       u8 *buf, bool for_scan)
>  {
>  	struct i2c_client *client = to_i2c_client(regmap_get_device(data->regmap));
> -	u32 reg_val;
>  	int ret;
>  
>  	guard(mutex)(&data->lock);
> @@ -601,15 +654,10 @@ static int ads112c14_single_conversion(struct ads112c14_data *data,
>  			return ret;
>  	}
>  
> -	ret = regmap_write(data->regmap, ADS112C14_REG_CONVERSION_CTRL,
> -			   ADS112C14_CONVERSION_CTRL_START);
> -	if (ret)
> -		return ret;
> -
> -	ret = regmap_read_poll_timeout(data->regmap,
> -				       ADS112C14_REG_STATUS_MSB, reg_val,
> -				       FIELD_GET(ADS112C14_STATUS_MSB_DRDY, reg_val),
> -				       1 * USEC_PER_MSEC, 100 * USEC_PER_MSEC);
> +	if (data->drdy_irq)
> +		ret = ads112c14_wait_for_conversion_irq(data);
> +	else
> +		ret = ads112c14_wait_for_conversion_poll(data);
>  	if (ret)
>  		return ret;
>  
> @@ -1391,6 +1439,38 @@ static int ads112c14_probe(struct i2c_client *client)
>  	if (ret)
>  		return ret;
>  
> +	if (device_property_present(dev, "interrupt-names")) {
> +		data->drdy_irq = fwnode_irq_get_byname(dev_fwnode(dev), "drdy");
> +		if (data->drdy_irq < 0)
> +			return dev_err_probe(dev, data->drdy_irq,
> +					     "failed to get drdy interrupt\n");
> +
> +		/*
> +		 * REVISIT: would probably need to implement a pin controller in
> +		 * order to support open drain option here.
> +		 */
> +		ret = regmap_update_bits(data->regmap, ADS112C14_REG_GPIO_CFG,
> +					 ADS112C14_GPIO_CFG_GPIO3_CFG,
> +					 FIELD_PREP(ADS112C14_GPIO_CFG_GPIO3_CFG,
> +						    ADS112C14_GPIO_CFG_GPIO_CFG_OUTPUT_PUSH_PULL));

 | sashiko.dev <sashiko@sashiko.dev>:
 |
 | [Severity: High]
 | Does this unconditionally force GPIO3 into an output even on board designs
 | that use the dedicated DRDY pin instead?
 |
 | For package variants (like the TSSOP) with a dedicated DRDY pin, a board
 | designer might route the dedicated pin to the interrupt controller and use
 | GPIO3 for another circuit. By forcing GPIO3 to a push-pull output here,
 | the driver will drive voltage into whatever is connected to it, which could
 | cause an electrical short if it is tied to GND/VCC or another output.
 |
 | via: https://sashiko.dev/#/message/20260827-iio-adc-ti-ads112c14-continuous-mode-v4-1-1e51a6e20a69@baylibre.com

This seems to be a datasheet misread. Kind of reasonable given
the datasheet talks about Data Ready (!DRDY) Pin. But that section goes
on to explain.

"GPIO3 can be configured as a dedicated DRDY output pin (GPIO3_CFG[1:0] = 10b or 11b, GPIO3_SRC = 1b)"

> +		if (ret)
> +			return ret;
> +
> +		ret = regmap_update_bits(data->regmap, ADS112C14_REG_GPIO_DATA_OUTPUT,
> +					 ADS112C14_GPIO_DATA_OUTPUT_GPIO3_SRC,
> +					 FIELD_PREP(ADS112C14_GPIO_DATA_OUTPUT_GPIO3_SRC,
> +						    ADS112C14_GPIO_DATA_OUTPUT_GPIO3_SRC_DRDY));
> +		if (ret)
> +			return ret;
> +
> +		init_completion(&data->drdy_completion);
> +
> +		ret = devm_request_irq(dev, data->drdy_irq, ads112c14_drdy_irq_handler,
> +				       0, dev_name(dev), indio_dev);

 | sashiko.dev <sashiko@sashiko.dev>:
 |
 | [Severity: High]
 | Could this lead to an interrupt storm on systems with level-triggered
 | interrupts, or fail completely on nested IRQ controllers?
 |
 | The IRQ is requested without a thread function and without IRQF_ONESHOT or
 | explicit edge-trigger flags. Since ads112c14_drdy_irq_handler() is a hardirq
 | handler, it acknowledges the OS interrupt but cannot perform a sleeping I2C
 | transaction to clear the physical line state on the device.

IRQF_ONESHOT in a non threaded interrupt handler would be pointless.

 |
 | On level-triggered systems, this will cause the IRQ controller to immediately
 | re-trigger the interrupt, starving the CPU. Additionally, if the interrupt
 | is routed through an I2C/SPI GPIO expander (nested IRQ controller),
 | devm_request_irq() will fail with -EINVAL because a threaded handler is
 | strictly required.
 |
 | Would it be better to use a threaded IRQ with IRQF_ONESHOT, or strictly
 | require edge-triggering?
 |
 | via: https://sashiko.dev/#/message/20260827-iio-adc-ti-ads112c14-continuous-mode-v4-1-1e51a6e20a69@baylibre.com

Personally I don't consider it the drivers job to handle broken firmware or
indeed the more complex cases of expanders that rely on threaded interrupts
only.  I do note that the dt file doesn't express that it expects
an edge interrupt though. Maybe that could be improved?  Not to mention
providing interrupts in one of the examples (that would at least
have hinted about this constraint).

-- 
Jonathan Cameron <jonathan.cameron@oss.qualcomm.com>

[PATCH v4 2/3] iio: adc: ti-ads112c14: create data read helper functions

[David Lechner (TI)]

Refactor a few bits of code into helper functions. These will be reused
when continuous mode support is added in a later patch.

Signed-off-by: David Lechner (TI) <dlechner@baylibre.com>
---
 drivers/iio/adc/ti-ads112c14.c | 51 +++++++++++++++++++++++++-----------------
 1 file changed, 31 insertions(+), 20 deletions(-)

diff --git a/drivers/iio/adc/ti-ads112c14.c b/drivers/iio/adc/ti-ads112c14.c
index 63b819339788..23f15be303fb 100644
--- a/drivers/iio/adc/ti-ads112c14.c
+++ b/drivers/iio/adc/ti-ads112c14.c
@@ -601,6 +601,32 @@ static int ads112c14_prepare_sys_mon_channel(struct ads112c14_data *data,
 	return 0;
 }
 
+static int ads112c14_prepare_channel(struct ads112c14_data *data,
+				     const struct iio_chan_spec *chan)
+{
+	if (chan->channel < ADS112C14_SYS_MON_CHANNEL_BASE)
+		return ads112c14_prepare_measurement_channel(data, chan);
+
+	return ads112c14_prepare_sys_mon_channel(data, chan);
+}
+
+static int ads112c14_scan_read(struct ads112c14_data *data, u8 *buf)
+{
+	struct i2c_client *client = to_i2c_client(regmap_get_device(data->regmap));
+	int ret;
+	u8 len;
+
+	len = BITS_TO_BYTES(data->chip_info->resolution_bits);
+	if (data->i2c_crc_enabled)
+		len += 1;
+
+	ret = i2c_smbus_read_i2c_block_data(client, ADS112C14_CMD_RDATA, len, buf);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
 static int ads112c14_wait_for_conversion_irq(struct ads112c14_data *data)
 {
 	unsigned long remaining;
@@ -644,15 +670,9 @@ static int ads112c14_single_conversion(struct ads112c14_data *data,
 
 	guard(mutex)(&data->lock);
 
-	if (chan->channel < ADS112C14_SYS_MON_CHANNEL_BASE) {
-		ret = ads112c14_prepare_measurement_channel(data, chan);
-		if (ret)
-			return ret;
-	} else {
-		ret = ads112c14_prepare_sys_mon_channel(data, chan);
-		if (ret)
-			return ret;
-	}
+	ret = ads112c14_prepare_channel(data, chan);
+	if (ret)
+		return ret;
 
 	if (data->drdy_irq)
 		ret = ads112c14_wait_for_conversion_irq(data);
@@ -667,17 +687,8 @@ static int ads112c14_single_conversion(struct ads112c14_data *data,
 	 * with CRC errors, but rather leave it to userspace to decide what to
 	 * do.
 	 */
-	if (for_scan) {
-		u8 len = BITS_TO_BYTES(data->chip_info->resolution_bits) +
-			 (data->i2c_crc_enabled ? 1 : 0);
-
-		ret = i2c_smbus_read_i2c_block_data(client, ADS112C14_CMD_RDATA,
-						    len, buf);
-		if (ret < 0)
-			return ret;
-
-		return 0;
-	}
+	if (for_scan)
+		return ads112c14_scan_read(data, buf);
 
 	return ads112c14_i2c_read_bytes(client, ADS112C14_CMD_RDATA, buf,
 					BITS_TO_BYTES(data->chip_info->resolution_bits),

-- 
2.43.0

[PATCH v4 3/3] iio: adc: ti-ads112c14: add continuous mode support

[David Lechner (TI)]

Add support for continuous mode in the TI ADS112C14 ADC driver. In this
mode the ADC itself is starting each conversion, so we add a trigger
based on the DRDY interrupt to read each sample. This mode is also
limited in that only one channel can be enabled at a time since the
chip does not have a sequencer or simultaneous sampling capability.
Continuous mode will only be used when this new trigger is the current
trigger.

Signed-off-by: David Lechner (TI) <dlechner@baylibre.com>
---
 drivers/iio/adc/ti-ads112c14.c | 143 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 141 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/adc/ti-ads112c14.c b/drivers/iio/adc/ti-ads112c14.c
index 23f15be303fb..878764deffc5 100644
--- a/drivers/iio/adc/ti-ads112c14.c
+++ b/drivers/iio/adc/ti-ads112c14.c
@@ -9,6 +9,7 @@
  */
 
 #include <linux/bitfield.h>
+#include <linux/bitmap.h>
 #include <linux/cleanup.h>
 #include <linux/completion.h>
 #include <linux/crc8.h>
@@ -18,6 +19,7 @@
 #include <linux/i2c.h>
 #include <linux/iio/buffer.h>
 #include <linux/iio/iio.h>
+#include <linux/iio/trigger.h>
 #include <linux/iio/trigger_consumer.h>
 #include <linux/iio/triggered_buffer.h>
 #include <linux/interrupt.h>
@@ -257,6 +259,7 @@ struct ads112c14_measurement {
 struct ads112c14_data {
 	const struct ads112c14_chip_info *chip_info;
 	struct regmap *regmap;
+	struct iio_trigger *drdy_trig;
 	/* Synchronizes access to register value fields. */
 	struct mutex lock;
 	int drdy_irq;
@@ -280,11 +283,18 @@ static irqreturn_t ads112c14_drdy_irq_handler(int irq, void *private)
 	struct iio_dev *indio_dev = private;
 	struct ads112c14_data *data = iio_priv(indio_dev);
 
-	complete(&data->drdy_completion);
+	if (indio_dev->trig && iio_trigger_using_own(indio_dev))
+		iio_trigger_poll(data->drdy_trig);
+	else
+		complete(&data->drdy_completion);
 
 	return IRQ_HANDLED;
 }
 
+static const struct iio_trigger_ops ads112c14_trigger_ops = {
+	.validate_device = iio_trigger_validate_own_device,
+};
+
 static bool ads112c14_writeable_reg(struct device *dev, unsigned int reg)
 {
 	switch (reg) {
@@ -952,6 +962,19 @@ static int ads112c14_read_label(struct iio_dev *indio_dev,
 	return sysfs_emit(label, "%s\n", label_source);
 }
 
+static const struct iio_chan_spec *
+ads112c14_first_active_channel(struct iio_dev *indio_dev)
+{
+	unsigned int scan_mask_len = iio_get_masklength(indio_dev);
+	unsigned int i;
+
+	i = find_first_bit(indio_dev->active_scan_mask, scan_mask_len);
+	if (i == scan_mask_len)
+		return NULL;
+
+	return &indio_dev->channels[i];
+}
+
 static irqreturn_t ads112c14_trigger_handler(int irq, void *private)
 {
 	struct iio_poll_func *pf = private;
@@ -961,6 +984,26 @@ static irqreturn_t ads112c14_trigger_handler(int irq, void *private)
 	u32 i;
 	int ret;
 
+	if (iio_trigger_using_own(indio_dev)) {
+		const struct iio_chan_spec *chan;
+
+		chan = ads112c14_first_active_channel(indio_dev);
+		if (!chan)
+			goto out;
+
+		ret = ads112c14_scan_read(data, (u8 *)&data->scan[0]);
+		if (ret) {
+			dev_err_once(indio_dev->dev.parent,
+				     "failed to read channel %d: %pe; additional errors will be suppressed\n",
+				     chan->channel, ERR_PTR(ret));
+			goto out;
+		}
+
+		iio_push_to_buffers_with_ts(indio_dev, data->scan,
+					    sizeof(data->scan), pf->timestamp);
+		goto out;
+	}
+
 	iio_for_each_active_channel(indio_dev, i) {
 		const struct iio_chan_spec *chan = &indio_dev->channels[i];
 
@@ -992,6 +1035,88 @@ static const struct iio_info ads112c14_info = {
 	.read_label = ads112c14_read_label,
 };
 
+static bool ads112c14_using_drdy_trigger(struct iio_dev *indio_dev)
+{
+	struct ads112c14_data *data = iio_priv(indio_dev);
+
+	return data->drdy_trig && indio_dev->trig == data->drdy_trig;
+}
+
+static bool ads112c14_validate_scan_mask(struct iio_dev *indio_dev,
+					 const unsigned long *mask)
+{
+	if (!ads112c14_using_drdy_trigger(indio_dev))
+		return true;
+
+	return iio_validate_scan_mask_onehot(indio_dev, mask);
+}
+
+static int ads112c14_buffer_postenable(struct iio_dev *indio_dev)
+{
+	struct ads112c14_data *data = iio_priv(indio_dev);
+	const struct iio_chan_spec *chan;
+	int ret;
+
+	if (!ads112c14_using_drdy_trigger(indio_dev))
+		return 0;
+
+	chan = ads112c14_first_active_channel(indio_dev);
+	if (!chan)
+		return -EINVAL;
+
+	guard(mutex)(&data->lock);
+
+	ret = ads112c14_prepare_channel(data, chan);
+	if (ret)
+		return ret;
+
+	ret = regmap_update_bits(data->regmap, ADS112C14_REG_DEVICE_CFG,
+				 ADS112C14_DEVICE_CFG_CONV_MODE,
+				 FIELD_PREP(ADS112C14_DEVICE_CFG_CONV_MODE,
+					    ADS112C14_DEVICE_CFG_CONV_MODE_CONTINUOUS));
+	if (ret)
+		return ret;
+
+	ret = regmap_write(data->regmap, ADS112C14_REG_CONVERSION_CTRL,
+			   ADS112C14_CONVERSION_CTRL_START);
+	if (ret) {
+		regmap_update_bits(data->regmap, ADS112C14_REG_DEVICE_CFG,
+				   ADS112C14_DEVICE_CFG_CONV_MODE,
+				   FIELD_PREP(ADS112C14_DEVICE_CFG_CONV_MODE,
+					      ADS112C14_DEVICE_CFG_CONV_MODE_SINGLE_SHOT));
+		return ret;
+	}
+
+	return 0;
+}
+
+static int ads112c14_buffer_predisable(struct iio_dev *indio_dev)
+{
+	struct ads112c14_data *data = iio_priv(indio_dev);
+	int ret;
+
+	if (!ads112c14_using_drdy_trigger(indio_dev))
+		return 0;
+
+	guard(mutex)(&data->lock);
+
+	ret = regmap_write(data->regmap, ADS112C14_REG_CONVERSION_CTRL,
+			   ADS112C14_CONVERSION_CTRL_STOP);
+	if (ret)
+		return ret;
+
+	return regmap_update_bits(data->regmap, ADS112C14_REG_DEVICE_CFG,
+				  ADS112C14_DEVICE_CFG_CONV_MODE,
+				  FIELD_PREP(ADS112C14_DEVICE_CFG_CONV_MODE,
+					     ADS112C14_DEVICE_CFG_CONV_MODE_SINGLE_SHOT));
+}
+
+static const struct iio_buffer_setup_ops ads112c14_buffer_setup_ops = {
+	.postenable = ads112c14_buffer_postenable,
+	.predisable = ads112c14_buffer_predisable,
+	.validate_scan_mask = ads112c14_validate_scan_mask,
+};
+
 static int ads112c14_populate_idac_mag(u32 current_nA, u8 *idac_mag)
 {
 	u32 current_uA = current_nA / (NANO / MICRO);
@@ -1480,6 +1605,19 @@ static int ads112c14_probe(struct i2c_client *client)
 				       0, dev_name(dev), indio_dev);
 		if (ret)
 			return ret;
+
+		data->drdy_trig = devm_iio_trigger_alloc(dev, "%s-dev%d-drdy",
+							 info->name,
+							 iio_device_id(indio_dev));
+		if (!data->drdy_trig)
+			return -ENOMEM;
+
+		data->drdy_trig->ops = &ads112c14_trigger_ops;
+		iio_trigger_set_drvdata(data->drdy_trig, indio_dev);
+
+		ret = devm_iio_trigger_register(dev, data->drdy_trig);
+		if (ret)
+			return ret;
 	}
 
 	ads112c14_populate_tables(data);
@@ -1490,7 +1628,8 @@ static int ads112c14_probe(struct i2c_client *client)
 
 	ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
 					      iio_pollfunc_store_time,
-					      ads112c14_trigger_handler, NULL);
+					      ads112c14_trigger_handler,
+					      &ads112c14_buffer_setup_ops);
 	if (ret)
 		return ret;
 

-- 
2.43.0

Re: [PATCH v4 3/3] iio: adc: ti-ads112c14: add continuous mode support

[Jonathan Cameron]

> Add support for continuous mode in the TI ADS112C14 ADC driver. In this
> mode the ADC itself is starting each conversion, so we add a trigger
> based on the DRDY interrupt to read each sample. This mode is also
> limited in that only one channel can be enabled at a time since the
> chip does not have a sequencer or simultaneous sampling capability.
> Continuous mode will only be used when this new trigger is the current
> trigger.
> 
> Signed-off-by: David Lechner (TI) <dlechner@baylibre.com>
Sashiko caught some interesting things in this one.

See inline.

>
> diff --git a/drivers/iio/adc/ti-ads112c14.c b/drivers/iio/adc/ti-ads112c14.c
> index 23f15be303fb..878764deffc5 100644
> --- a/drivers/iio/adc/ti-ads112c14.c
> +++ b/drivers/iio/adc/ti-ads112c14.c
> @@ -9,6 +9,7 @@
>   */
>  
>  #include <linux/bitfield.h>
> +#include <linux/bitmap.h>
>  #include <linux/cleanup.h>
>  #include <linux/completion.h>
>  #include <linux/crc8.h>
> @@ -18,6 +19,7 @@
>  #include <linux/i2c.h>
>  #include <linux/iio/buffer.h>
>  #include <linux/iio/iio.h>
> +#include <linux/iio/trigger.h>
>  #include <linux/iio/trigger_consumer.h>
>  #include <linux/iio/triggered_buffer.h>
>  #include <linux/interrupt.h>
> @@ -257,6 +259,7 @@ struct ads112c14_measurement {
>  struct ads112c14_data {
>  	const struct ads112c14_chip_info *chip_info;
>  	struct regmap *regmap;
> +	struct iio_trigger *drdy_trig;
>  	/* Synchronizes access to register value fields. */
>  	struct mutex lock;
>  	int drdy_irq;
> @@ -280,11 +283,18 @@ static irqreturn_t ads112c14_drdy_irq_handler(int irq, void *private)
>  	struct iio_dev *indio_dev = private;
>  	struct ads112c14_data *data = iio_priv(indio_dev);
>  
> -	complete(&data->drdy_completion);
> +	if (indio_dev->trig && iio_trigger_using_own(indio_dev))


This indeed needs more guarding given we hit these interrupts for
a normal read (to get to the completion)


Likewise seems valid.

> +		iio_trigger_poll(data->drdy_trig);
> +	else
> +		complete(&data->drdy_completion);
>  
>  	return IRQ_HANDLED;
>  }

...

>  static irqreturn_t ads112c14_trigger_handler(int irq, void *private)
>  {
>  	struct iio_poll_func *pf = private;
> @@ -961,6 +984,26 @@ static irqreturn_t ads112c14_trigger_handler(int irq, void *private)
>  	u32 i;
>  	int ret;
>  
> +	if (iio_trigger_using_own(indio_dev)) {

 | sashiko.dev <sashiko@sashiko.dev>:
 |
 | [Severity: High]
 | Will this check incorrectly enable continuous mode processing for any
 | external trigger that belongs to its own device?
 |
 | If a user assigns an external trigger that sets its attached_own_device
 | property to true, iio_trigger_using_own() will evaluate to true. Could this
 | misroute the interrupt handler into assuming continuous mode when it should
 | not?
 |
 | via: https://sashiko.dev/#/message/20260827-iio-adc-ti-ads112c14-continuous-mode-v4-3-1e51a6e20a69@baylibre.com

I think this is triggered by the more complex handling you have
to detect use of the dataready trigger below.  There is only one trigger
currently so they end up the same, but perhaps we should make the handling
more similar?

> +		const struct iio_chan_spec *chan;
> +
> +		chan = ads112c14_first_active_channel(indio_dev);
> +		if (!chan)
> +			goto out;
> +
> +		ret = ads112c14_scan_read(data, (u8 *)&data->scan[0]);

 | sashiko.dev <sashiko@sashiko.dev>:
 |
 | [Severity: High]
 | This is a pre-existing issue, but is there a missing length check in the
 | underlying I2C block read that leaves stack memory partially uninitialized?
 |
 | If i2c_smbus_read_i2c_block_data() (called by ads112c14_scan_read()) returns
 | a positive value smaller than the requested length, the condition if (ret < 0)
 | fails to catch the short read. Does this leak uninitialized stack memory to
 | userspace when data->scan is later pushed to the IIO buffers?
 |
 | via: https://sashiko.dev/#/message/20260827-iio-adc-ti-ads112c14-continuous-mode-v4-3-1e51a6e20a69@baylibre.com

I haven't chased this one through - but in a similar example in a review
I did yesterday I suggested just initializing the memory anyway
so we don't have to care one way or the other.

> +		if (ret) {
> +			dev_err_once(indio_dev->dev.parent,
> +				     "failed to read channel %d: %pe; additional errors will be suppressed\n",
> +				     chan->channel, ERR_PTR(ret));
> +			goto out;
> +		}
> +
> +		iio_push_to_buffers_with_ts(indio_dev, data->scan,
> +					    sizeof(data->scan), pf->timestamp);
> +		goto out;
> +	}
> +
>  	iio_for_each_active_channel(indio_dev, i) {
>  		const struct iio_chan_spec *chan = &indio_dev->channels[i];
>  
> @@ -992,6 +1035,88 @@ static const struct iio_info ads112c14_info = {
>  	.read_label = ads112c14_read_label,
>  };
>  

...

> +static int ads112c14_buffer_postenable(struct iio_dev *indio_dev)
> +{
> +	struct ads112c14_data *data = iio_priv(indio_dev);
> +	const struct iio_chan_spec *chan;
> +	int ret;
> +
> +	if (!ads112c14_using_drdy_trigger(indio_dev))
> +		return 0;
> +
> +	chan = ads112c14_first_active_channel(indio_dev);
> +	if (!chan)
> +		return -EINVAL;
> +
> +	guard(mutex)(&data->lock);
> +
> +	ret = ads112c14_prepare_channel(data, chan);


Hmm. Indeed an interesting corner...  From a quick look it is correct.
I'm not sure if this is in practice an existing bug - do other
drivers change acceptable channels based on another change such as
which trigger is in use?   Either way we need to close this.

We can't just go clearing the set bits on setting the trigger as
that might cause a regression.  So I think all we can do is add
a well commented additional check early in the buffer enable path.

Given the behaviour that is causing problems is present in this
driver we could either add the protection in fix and rely on that
going upstream first, or add it as first patch in this series and
let it work its way upstream with this patch.

Nice catch to sashiko!

-- 
Jonathan Cameron <jonathan.cameron@oss.qualcomm.com>

Re: [PATCH v4 3/3] iio: adc: ti-ads112c14: add continuous mode support

[Jonathan Cameron]

On Sun, 30 Aug 2026 22:00:34 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> > Add support for continuous mode in the TI ADS112C14 ADC driver. In this
> > mode the ADC itself is starting each conversion, so we add a trigger
> > based on the DRDY interrupt to read each sample. This mode is also
> > limited in that only one channel can be enabled at a time since the
> > chip does not have a sequencer or simultaneous sampling capability.
> > Continuous mode will only be used when this new trigger is the current
> > trigger.
> > 
> > Signed-off-by: David Lechner (TI) <dlechner@baylibre.com>  
> Sashiko caught some interesting things in this one.
> 
> See inline.

Sorry - messed up my use of b4 review tui and failed to include the
sashiko blobs I was talking about. I can't work out how to reply to review
I already did in there, so doing this via cut and paste.

> 
> >
> > diff --git a/drivers/iio/adc/ti-ads112c14.c b/drivers/iio/adc/ti-ads112c14.c
> > index 23f15be303fb..878764deffc5 100644
> > --- a/drivers/iio/adc/ti-ads112c14.c
> > +++ b/drivers/iio/adc/ti-ads112c14.c
> > @@ -9,6 +9,7 @@
> >   */
> >  
> >  #include <linux/bitfield.h>
> > +#include <linux/bitmap.h>
> >  #include <linux/cleanup.h>
> >  #include <linux/completion.h>
> >  #include <linux/crc8.h>
> > @@ -18,6 +19,7 @@
> >  #include <linux/i2c.h>
> >  #include <linux/iio/buffer.h>
> >  #include <linux/iio/iio.h>
> > +#include <linux/iio/trigger.h>
> >  #include <linux/iio/trigger_consumer.h>
> >  #include <linux/iio/triggered_buffer.h>
> >  #include <linux/interrupt.h>
> > @@ -257,6 +259,7 @@ struct ads112c14_measurement {
> >  struct ads112c14_data {
> >  	const struct ads112c14_chip_info *chip_info;
> >  	struct regmap *regmap;
> > +	struct iio_trigger *drdy_trig;
> >  	/* Synchronizes access to register value fields. */
> >  	struct mutex lock;
> >  	int drdy_irq;
> > @@ -280,11 +283,18 @@ static irqreturn_t ads112c14_drdy_irq_handler(int irq, void *private)
> >  	struct iio_dev *indio_dev = private;
> >  	struct ads112c14_data *data = iio_priv(indio_dev);
> >  
> > -	complete(&data->drdy_completion);
> > +	if (indio_dev->trig && iio_trigger_using_own(indio_dev))  
> 
> 
 | sashiko.dev <sashiko@sashiko.dev>:
 |
 | [Severity: Critical]
 | Can this sequence race with a concurrent sysfs write?
 |
 | If a direct read is initiated and indio_dev->trig is checked without a lock,
 | could a concurrent sysfs write to current_trigger nullify or free the
 | trigger right before iio_trigger_using_own() dereferences it, leading to a
 | NULL pointer dereference or use-after-free crash?

> This indeed needs more guarding given we hit these interrupts for
> a normal read (to get to the completion)
> 
> 
 |
 | [Severity: High]
 | Does this logic cause direct reads to hang?
 |
 | During a direct read (e.g., via sysfs) while the buffer is disabled but a
 | trigger is assigned, this condition evaluates to true. Will this route the
 | interrupt completion to iio_trigger_poll(), leaving the direct read waiting
 | indefinitely for data->drdy_completion and resulting in a timeout?
 |
 | via: https://sashiko.dev/#/message/20260827-iio-adc-ti-ads112c14-continuous-mode-v4-3-1e51a6e20a69@baylibre.com

> Likewise seems valid.
> 
> > +		iio_trigger_poll(data->drdy_trig);
> > +	else
> > +		complete(&data->drdy_completion);
> >  
> >  	return IRQ_HANDLED;
> >  }  
> 
> ...
> 
> >  static irqreturn_t ads112c14_trigger_handler(int irq, void *private)
> >  {
> >  	struct iio_poll_func *pf = private;
> > @@ -961,6 +984,26 @@ static irqreturn_t ads112c14_trigger_handler(int irq, void *private)
> >  	u32 i;
> >  	int ret;
> >  
> > +	if (iio_trigger_using_own(indio_dev)) {  
> 
>  | sashiko.dev <sashiko@sashiko.dev>:
>  |
>  | [Severity: High]
>  | Will this check incorrectly enable continuous mode processing for any
>  | external trigger that belongs to its own device?
>  |
>  | If a user assigns an external trigger that sets its attached_own_device
>  | property to true, iio_trigger_using_own() will evaluate to true. Could this
>  | misroute the interrupt handler into assuming continuous mode when it should
>  | not?
>  |
>  | via: https://sashiko.dev/#/message/20260827-iio-adc-ti-ads112c14-continuous-mode-v4-3-1e51a6e20a69@baylibre.com
> 
> I think this is triggered by the more complex handling you have
> to detect use of the dataready trigger below.  There is only one trigger
> currently so they end up the same, but perhaps we should make the handling
> more similar?
> 
> > +		const struct iio_chan_spec *chan;
> > +
> > +		chan = ads112c14_first_active_channel(indio_dev);
> > +		if (!chan)
> > +			goto out;
> > +
> > +		ret = ads112c14_scan_read(data, (u8 *)&data->scan[0]);  
> 
>  | sashiko.dev <sashiko@sashiko.dev>:
>  |
>  | [Severity: High]
>  | This is a pre-existing issue, but is there a missing length check in the
>  | underlying I2C block read that leaves stack memory partially uninitialized?
>  |
>  | If i2c_smbus_read_i2c_block_data() (called by ads112c14_scan_read()) returns
>  | a positive value smaller than the requested length, the condition if (ret < 0)
>  | fails to catch the short read. Does this leak uninitialized stack memory to
>  | userspace when data->scan is later pushed to the IIO buffers?
>  |
>  | via: https://sashiko.dev/#/message/20260827-iio-adc-ti-ads112c14-continuous-mode-v4-3-1e51a6e20a69@baylibre.com
> 
> I haven't chased this one through - but in a similar example in a review
> I did yesterday I suggested just initializing the memory anyway
> so we don't have to care one way or the other.
> 
> > +		if (ret) {
> > +			dev_err_once(indio_dev->dev.parent,
> > +				     "failed to read channel %d: %pe; additional errors will be suppressed\n",
> > +				     chan->channel, ERR_PTR(ret));
> > +			goto out;
> > +		}
> > +
> > +		iio_push_to_buffers_with_ts(indio_dev, data->scan,
> > +					    sizeof(data->scan), pf->timestamp);
> > +		goto out;
> > +	}
> > +
> >  	iio_for_each_active_channel(indio_dev, i) {
> >  		const struct iio_chan_spec *chan = &indio_dev->channels[i];
> >  
> > @@ -992,6 +1035,88 @@ static const struct iio_info ads112c14_info = {
> >  	.read_label = ads112c14_read_label,
> >  };
> >    
> 
> ...
> 
> > +static int ads112c14_buffer_postenable(struct iio_dev *indio_dev)
> > +{
> > +	struct ads112c14_data *data = iio_priv(indio_dev);
> > +	const struct iio_chan_spec *chan;
> > +	int ret;
> > +
> > +	if (!ads112c14_using_drdy_trigger(indio_dev))
> > +		return 0;
> > +
> > +	chan = ads112c14_first_active_channel(indio_dev);
> > +	if (!chan)
> > +		return -EINVAL;
> > +
> > +	guard(mutex)(&data->lock);
> > +
> > +	ret = ads112c14_prepare_channel(data, chan);  
> 
 | sashiko.dev <sashiko@sashiko.dev>:
 |
 | [Severity: High]
 | Can a user bypass the single-channel limitation and push corrupted data
 | to the ring buffer here?
 |
 | Since ads112c14_validate_scan_mask() allows multiple channels if a non-DRDY
 | trigger is currently selected, a user could enable multiple channels via
 | scan_elements, then switch current_trigger to the DRDY trigger before
 | enabling the IIO buffer.

> 
> Hmm. Indeed an interesting corner...  From a quick look it is correct.
> I'm not sure if this is in practice an existing bug - do other
> drivers change acceptable channels based on another change such as
> which trigger is in use?   Either way we need to close this.
> 
> We can't just go clearing the set bits on setting the trigger as
> that might cause a regression.  So I think all we can do is add
> a well commented additional check early in the buffer enable path.
> 
> Given the behaviour that is causing problems is present in this
> driver we could either add the protection in fix and rely on that
> going upstream first, or add it as first patch in this series and
> let it work its way upstream with this patch.
> 
> Nice catch to sashiko!
> 

 |
 | Because this driver does not provide an available_scan_masks array, the
 | mask isn't re-validated during buffer enablement. Since
 | ads112c14_buffer_postenable() only configures the first active channel,
 | will the trigger handler push uninitialized heap memory for the remaining
 | channels when the full array is sent to userspace?
 |
 | via: https://sashiko.dev/#/message/20260827-iio-adc-ti-ads112c14-continuous-mode-v4-3-1e51a6e20a69@baylibre.com

I think that's all the feedback I replied to...

J

Re: [PATCH v4 3/3] iio: adc: ti-ads112c14: add continuous mode support

[David Lechner]

On 8/30/26 5:24 PM, Jonathan Cameron wrote:
> On Sun, 30 Aug 2026 22:00:34 +0100
> Jonathan Cameron <jic23@kernel.org> wrote:
> 

...

>>
>>> +static int ads112c14_buffer_postenable(struct iio_dev *indio_dev)
>>> +{
>>> +	struct ads112c14_data *data = iio_priv(indio_dev);
>>> +	const struct iio_chan_spec *chan;
>>> +	int ret;
>>> +
>>> +	if (!ads112c14_using_drdy_trigger(indio_dev))
>>> +		return 0;
>>> +
>>> +	chan = ads112c14_first_active_channel(indio_dev);
>>> +	if (!chan)
>>> +		return -EINVAL;
>>> +
>>> +	guard(mutex)(&data->lock);
>>> +
>>> +	ret = ads112c14_prepare_channel(data, chan);  
>>
>  | sashiko.dev <sashiko@sashiko.dev>:
>  |
>  | [Severity: High]
>  | Can a user bypass the single-channel limitation and push corrupted data
>  | to the ring buffer here?
>  |
>  | Since ads112c14_validate_scan_mask() allows multiple channels if a non-DRDY
>  | trigger is currently selected, a user could enable multiple channels via
>  | scan_elements, then switch current_trigger to the DRDY trigger before
>  | enabling the IIO buffer.
> 
>>
>> Hmm. Indeed an interesting corner...  From a quick look it is correct.
>> I'm not sure if this is in practice an existing bug - do other
>> drivers change acceptable channels based on another change such as
>> which trigger is in use?   Either way we need to close this.
>>
>> We can't just go clearing the set bits on setting the trigger as
>> that might cause a regression.  So I think all we can do is add
>> a well commented additional check early in the buffer enable path.
>>
>> Given the behaviour that is causing problems is present in this
>> driver we could either add the protection in fix and rely on that
>> going upstream first, or add it as first patch in this series and
>> let it work its way upstream with this patch.
>>
>> Nice catch to sashiko!
>>
> 
>  |
>  | Because this driver does not provide an available_scan_masks array, the
>  | mask isn't re-validated during buffer enablement. Since
>  | ads112c14_buffer_postenable() only configures the first active channel,
>  | will the trigger handler push uninitialized heap memory for the remaining
>  | channels when the full array is sent to userspace?
>  |
>  | via: https://sashiko.dev/#/message/20260827-iio-adc-ti-ads112c14-continuous-mode-v4-3-1e51a6e20a69@baylibre.com
> 
Ah, I had it mixed up in my mind with update_scan_mode(), which would be
called at the appropriate time.

Re: [PATCH v4 3/3] iio: adc: ti-ads112c14: add continuous mode support

[David Lechner]

On 8/30/26 4:00 PM, Jonathan Cameron wrote:
>> Add support for continuous mode in the TI ADS112C14 ADC driver. In this
>> mode the ADC itself is starting each conversion, so we add a trigger
>> based on the DRDY interrupt to read each sample. This mode is also
>> limited in that only one channel can be enabled at a time since the
>> chip does not have a sequencer or simultaneous sampling capability.
>> Continuous mode will only be used when this new trigger is the current
>> trigger.
>>
>> Signed-off-by: David Lechner (TI) <dlechner@baylibre.com>
> Sashiko caught some interesting things in this one.
> 
> See inline.
> 
>>
>> diff --git a/drivers/iio/adc/ti-ads112c14.c b/drivers/iio/adc/ti-ads112c14.c
>> index 23f15be303fb..878764deffc5 100644
>> --- a/drivers/iio/adc/ti-ads112c14.c
>> +++ b/drivers/iio/adc/ti-ads112c14.c
>> @@ -9,6 +9,7 @@
>>   */
>>  
>>  #include <linux/bitfield.h>
>> +#include <linux/bitmap.h>
>>  #include <linux/cleanup.h>
>>  #include <linux/completion.h>
>>  #include <linux/crc8.h>
>> @@ -18,6 +19,7 @@
>>  #include <linux/i2c.h>
>>  #include <linux/iio/buffer.h>
>>  #include <linux/iio/iio.h>
>> +#include <linux/iio/trigger.h>
>>  #include <linux/iio/trigger_consumer.h>
>>  #include <linux/iio/triggered_buffer.h>
>>  #include <linux/interrupt.h>
>> @@ -257,6 +259,7 @@ struct ads112c14_measurement {
>>  struct ads112c14_data {
>>  	const struct ads112c14_chip_info *chip_info;
>>  	struct regmap *regmap;
>> +	struct iio_trigger *drdy_trig;
>>  	/* Synchronizes access to register value fields. */
>>  	struct mutex lock;
>>  	int drdy_irq;
>> @@ -280,11 +283,18 @@ static irqreturn_t ads112c14_drdy_irq_handler(int irq, void *private)
>>  	struct iio_dev *indio_dev = private;
>>  	struct ads112c14_data *data = iio_priv(indio_dev);
>>  
>> -	complete(&data->drdy_completion);
>> +	if (indio_dev->trig && iio_trigger_using_own(indio_dev))
> 
> 
> This indeed needs more guarding given we hit these interrupts for
> a normal read (to get to the completion)
> 
> 
> Likewise seems valid.

I figured it didn't need guarding since we are claiming direct mode. The
trigger can't be assigned until buffer mode is claimed. and we can't
switch to buffer mode if direct mode is claimed.


> 
>> +		iio_trigger_poll(data->drdy_trig);
>> +	else
>> +		complete(&data->drdy_completion);
>>  
>>  	return IRQ_HANDLED;
>>  }
> 
> ...
> 
>>  static irqreturn_t ads112c14_trigger_handler(int irq, void *private)
>>  {
>>  	struct iio_poll_func *pf = private;
>> @@ -961,6 +984,26 @@ static irqreturn_t ads112c14_trigger_handler(int irq, void *private)
>>  	u32 i;
>>  	int ret;
>>  
>> +	if (iio_trigger_using_own(indio_dev)) {
> 
>  | sashiko.dev <sashiko@sashiko.dev>:
>  |
>  | [Severity: High]
>  | Will this check incorrectly enable continuous mode processing for any
>  | external trigger that belongs to its own device?
>  |
>  | If a user assigns an external trigger that sets its attached_own_device
>  | property to true, iio_trigger_using_own() will evaluate to true. Could this
>  | misroute the interrupt handler into assuming continuous mode when it should
>  | not?
>  |
>  | via: https://sashiko.dev/#/message/20260827-iio-adc-ti-ads112c14-continuous-mode-v4-3-1e51a6e20a69@baylibre.com
> 
> I think this is triggered by the more complex handling you have
> to detect use of the dataready trigger below.  There is only one trigger
> currently so they end up the same, but perhaps we should make the handling
> more similar?

I'll just create our own local flag instead of relying on IIO internals.

> 
>> +		const struct iio_chan_spec *chan;
>> +
>> +		chan = ads112c14_first_active_channel(indio_dev);
>> +		if (!chan)
>> +			goto out;
>> +
>> +		ret = ads112c14_scan_read(data, (u8 *)&data->scan[0]);
> 
>  | sashiko.dev <sashiko@sashiko.dev>:
>  |
>  | [Severity: High]
>  | This is a pre-existing issue, but is there a missing length check in the
>  | underlying I2C block read that leaves stack memory partially uninitialized?
>  |
>  | If i2c_smbus_read_i2c_block_data() (called by ads112c14_scan_read()) returns
>  | a positive value smaller than the requested length, the condition if (ret < 0)
>  | fails to catch the short read. Does this leak uninitialized stack memory to
>  | userspace when data->scan is later pushed to the IIO buffers?
>  |
>  | via: https://sashiko.dev/#/message/20260827-iio-adc-ti-ads112c14-continuous-mode-v4-3-1e51a6e20a69@baylibre.com
> 
> I haven't chased this one through - but in a similar example in a review
> I did yesterday I suggested just initializing the memory anyway
> so we don't have to care one way or the other.

This uses i2c_smbus_xfer() in the end which returns 0 or negative error.
No partial reads. So I don't see how we could be leaking.

> 
>> +		if (ret) {
>> +			dev_err_once(indio_dev->dev.parent,
>> +				     "failed to read channel %d: %pe; additional errors will be suppressed\n",
>> +				     chan->channel, ERR_PTR(ret));
>> +			goto out;
>> +		}
>> +
>> +		iio_push_to_buffers_with_ts(indio_dev, data->scan,
>> +					    sizeof(data->scan), pf->timestamp);
>> +		goto out;
>> +	}
>> +
>>  	iio_for_each_active_channel(indio_dev, i) {
>>  		const struct iio_chan_spec *chan = &indio_dev->channels[i];
>>  

Re: [PATCH v4 3/3] iio: adc: ti-ads112c14: add continuous mode support

[Jonathan Cameron]

On Mon, 31 Aug 2026 16:31:13 -0500
David Lechner <dlechner@baylibre.com> wrote:

> On 8/30/26 4:00 PM, Jonathan Cameron wrote:
> >> Add support for continuous mode in the TI ADS112C14 ADC driver. In this
> >> mode the ADC itself is starting each conversion, so we add a trigger
> >> based on the DRDY interrupt to read each sample. This mode is also
> >> limited in that only one channel can be enabled at a time since the
> >> chip does not have a sequencer or simultaneous sampling capability.
> >> Continuous mode will only be used when this new trigger is the current
> >> trigger.
> >>
> >> Signed-off-by: David Lechner (TI) <dlechner@baylibre.com>  
> > Sashiko caught some interesting things in this one.
> > 
> > See inline.
> >   
> >>
> >> diff --git a/drivers/iio/adc/ti-ads112c14.c b/drivers/iio/adc/ti-ads112c14.c
> >> index 23f15be303fb..878764deffc5 100644
> >> --- a/drivers/iio/adc/ti-ads112c14.c
> >> +++ b/drivers/iio/adc/ti-ads112c14.c
> >> @@ -9,6 +9,7 @@
> >>   */
> >>  
> >>  #include <linux/bitfield.h>
> >> +#include <linux/bitmap.h>
> >>  #include <linux/cleanup.h>
> >>  #include <linux/completion.h>
> >>  #include <linux/crc8.h>
> >> @@ -18,6 +19,7 @@
> >>  #include <linux/i2c.h>
> >>  #include <linux/iio/buffer.h>
> >>  #include <linux/iio/iio.h>
> >> +#include <linux/iio/trigger.h>
> >>  #include <linux/iio/trigger_consumer.h>
> >>  #include <linux/iio/triggered_buffer.h>
> >>  #include <linux/interrupt.h>
> >> @@ -257,6 +259,7 @@ struct ads112c14_measurement {
> >>  struct ads112c14_data {
> >>  	const struct ads112c14_chip_info *chip_info;
> >>  	struct regmap *regmap;
> >> +	struct iio_trigger *drdy_trig;
> >>  	/* Synchronizes access to register value fields. */
> >>  	struct mutex lock;
> >>  	int drdy_irq;
> >> @@ -280,11 +283,18 @@ static irqreturn_t ads112c14_drdy_irq_handler(int irq, void *private)
> >>  	struct iio_dev *indio_dev = private;
> >>  	struct ads112c14_data *data = iio_priv(indio_dev);
> >>  
> >> -	complete(&data->drdy_completion);
> >> +	if (indio_dev->trig && iio_trigger_using_own(indio_dev))  
> > 
> > 
> > This indeed needs more guarding given we hit these interrupts for
> > a normal read (to get to the completion)
> > 
> > 
> > Likewise seems valid.  
> 
> I figured it didn't need guarding since we are claiming direct mode. The
> trigger can't be assigned until buffer mode is claimed. and we can't
> switch to buffer mode if direct mode is claimed.
> 

Hmm. Is indio_dev->trig guarded enough? Some of the set paths
are fine because they are things the driver calls directly. The one
that I think is a problem is current_trigger_store().  

That has some protections to stop the indio_dev->trig being changed
whilst we are in buffered mode (those looks suspiciously like the
have a time of check time of use bug though maybe a harmless one).
Anyhow, that's not relevant here as we care about the not in buffered
mode.  For that there is no guarding.  Maybe we can just claim
direct mode in there (which closes the toctou issue as well)?

It seems unlikely anyone will do a claim_direct in the validation
or set_trigger_state callbacks.  Would have to check that carefully
though.

> 
> >   
> >> +		iio_trigger_poll(data->drdy_trig);
> >> +	else
> >> +		complete(&data->drdy_completion);
> >>  
> >>  	return IRQ_HANDLED;
> >>  }  
> > 
> > ...
> >   
> >>  static irqreturn_t ads112c14_trigger_handler(int irq, void *private)
> >>  {
> >>  	struct iio_poll_func *pf = private;
> >> @@ -961,6 +984,26 @@ static irqreturn_t ads112c14_trigger_handler(int irq, void *private)
> >>  	u32 i;
> >>  	int ret;
> >>  
> >> +	if (iio_trigger_using_own(indio_dev)) {  
> > 
> >  | sashiko.dev <sashiko@sashiko.dev>:
> >  |
> >  | [Severity: High]
> >  | Will this check incorrectly enable continuous mode processing for any
> >  | external trigger that belongs to its own device?
> >  |
> >  | If a user assigns an external trigger that sets its attached_own_device
> >  | property to true, iio_trigger_using_own() will evaluate to true. Could this
> >  | misroute the interrupt handler into assuming continuous mode when it should
> >  | not?
> >  |
> >  | via: https://sashiko.dev/#/message/20260827-iio-adc-ti-ads112c14-continuous-mode-v4-3-1e51a6e20a69@baylibre.com
> > 
> > I think this is triggered by the more complex handling you have
> > to detect use of the dataready trigger below.  There is only one trigger
> > currently so they end up the same, but perhaps we should make the handling
> > more similar?  
> 
> I'll just create our own local flag instead of relying on IIO internals.

That would be safer.

> 
> >   
> >> +		const struct iio_chan_spec *chan;
> >> +
> >> +		chan = ads112c14_first_active_channel(indio_dev);
> >> +		if (!chan)
> >> +			goto out;
> >> +
> >> +		ret = ads112c14_scan_read(data, (u8 *)&data->scan[0]);  
> > 
> >  | sashiko.dev <sashiko@sashiko.dev>:
> >  |
> >  | [Severity: High]
> >  | This is a pre-existing issue, but is there a missing length check in the
> >  | underlying I2C block read that leaves stack memory partially uninitialized?
> >  |
> >  | If i2c_smbus_read_i2c_block_data() (called by ads112c14_scan_read()) returns
> >  | a positive value smaller than the requested length, the condition if (ret < 0)
> >  | fails to catch the short read. Does this leak uninitialized stack memory to
> >  | userspace when data->scan is later pushed to the IIO buffers?
> >  |
> >  | via: https://sashiko.dev/#/message/20260827-iio-adc-ti-ads112c14-continuous-mode-v4-3-1e51a6e20a69@baylibre.com
> > 
> > I haven't chased this one through - but in a similar example in a review
> > I did yesterday I suggested just initializing the memory anyway
> > so we don't have to care one way or the other.  
> 
> This uses i2c_smbus_xfer() in the end which returns 0 or negative error.
> No partial reads. So I don't see how we could be leaking.
Ok. Probably fine.  I still need to get back to that related i2c set that
makes the handling more expected.
> 
> >   
> >> +		if (ret) {
> >> +			dev_err_once(indio_dev->dev.parent,
> >> +				     "failed to read channel %d: %pe; additional errors will be suppressed\n",
> >> +				     chan->channel, ERR_PTR(ret));
> >> +			goto out;
> >> +		}
> >> +
> >> +		iio_push_to_buffers_with_ts(indio_dev, data->scan,
> >> +					    sizeof(data->scan), pf->timestamp);
> >> +		goto out;
> >> +	}
> >> +
> >>  	iio_for_each_active_channel(indio_dev, i) {
> >>  		const struct iio_chan_spec *chan = &indio_dev->channels[i];
> >>