[PATCH v3 0/3] iio: adc: Add support for TI ADS1110 to ti-ads1100 driver

[PATCH v3 0/3] iio: adc: Add support for TI ADS1110 to ti-ads1100 driver

[Jakub Szczudlo]

Add support for the TI ADS1110 to the existing ADS1100 ADC IIO driver.
The ADS1110 is pin-to-pin compatible with the ADS1100 while providing
higher resolution and an internal voltage reference. This patch series
extends driver support for ADS1110, updates device tree bindings and
Kconfig text, and improves the overall hardware description for the
TI ADS1100 family.

Tested on: Raspberry pi 3b+ with 7.0 stable kernel

Signed-off-by: Jakub Szczudlo <jakubszczudlo40@gmail.com>

---
V2 -> V3:
- clean patch from unreleated changes
- divide adding support for ads1110 into separate patch
- add missing changelog
- Link to v2: https://lore.kernel.org/linux-iio/20260607183542.368184-1-jakubszczudlo40@gmail.com/

V1 -> V2:
- go from creating new driver to extending ADS1100 driver to support ADS1110
- Link to v1: https://lore.kernel.org/linux-iio/20260527164312.355729-1-jakubszczudlo40@gmail.com/

Jakub Szczudlo (3):
  dt-bindings: iio: adc: ti,ads1100: add support for ADS1110
  iio: adc: Add ti-ads1110 support to ti-ads1100 driver
  iio: adc: Fix incorrect reading when datarate changed in single mode

 .../bindings/iio/adc/ti,ads1100.yaml          |  10 +-
 drivers/iio/adc/Kconfig                       |   6 +-
 drivers/iio/adc/ti-ads1100.c                  | 136 +++++++++++++++---
 3 files changed, 123 insertions(+), 29 deletions(-)

-- 
2.47.3

[PATCH v3 1/3] dt-bindings: iio: adc: ti,ads1100: add support for ADS1110

[Jakub Szczudlo]

Register layouts are the same as for ADS1100 but ADS1110 have different
datarates and have internal voltage reference that is always 2.048V

Signed-off-by: Jakub Szczudlo <jakubszczudlo40@gmail.com>
---
 .../devicetree/bindings/iio/adc/ti,ads1100.yaml        | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/iio/adc/ti,ads1100.yaml b/Documentation/devicetree/bindings/iio/adc/ti,ads1100.yaml
index 970ccab15e1e..28c5e2dd0ad6 100644
--- a/Documentation/devicetree/bindings/iio/adc/ti,ads1100.yaml
+++ b/Documentation/devicetree/bindings/iio/adc/ti,ads1100.yaml
@@ -4,19 +4,23 @@
 $id: http://devicetree.org/schemas/iio/adc/ti,ads1100.yaml#
 $schema: http://devicetree.org/meta-schemas/core.yaml#
 
-title: TI ADS1100/ADS1000 single channel I2C analog to digital converter
+title: TI ADS1100 and similar single channel I2C Analog to Digital Converters
 
 maintainers:
   - Mike Looijmans <mike.looijmans@topic.nl>
 
 description: |
-  Datasheet at: https://www.ti.com/lit/gpn/ads1100
+  Datasheets:
+    - https://www.ti.com/lit/gpn/ads1000
+    - https://www.ti.com/lit/gpn/ads1100
+    - https://www.ti.com/lit/gpn/ads1110
 
 properties:
   compatible:
     enum:
-      - ti,ads1100
       - ti,ads1000
+      - ti,ads1100
+      - ti,ads1110
 
   reg:
     maxItems: 1
-- 
2.47.3

[PATCH v3 2/3] iio: adc: Add ti-ads1110 support to ti-ads1100 driver

[Jakub Szczudlo]

Add ADS1110 support that have faster datarate than ADS1100, it also uses
internal voltage reference of 2.048V for measurement.

Signed-off-by: Jakub Szczudlo <jakubszczudlo40@gmail.com>
---
 drivers/iio/adc/Kconfig      |  6 +--
 drivers/iio/adc/ti-ads1100.c | 83 +++++++++++++++++++++++++++---------
 2 files changed, 65 insertions(+), 24 deletions(-)

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index a9dedbb8eb46..54a0149a3838 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -1747,11 +1747,11 @@ config TI_ADS1018
          called ti-ads1018.
 
 config TI_ADS1100
-	tristate "Texas Instruments ADS1100 and ADS1000 ADC"
+	tristate "Texas Instruments ADS1100 and similar single channel I2C ADC"
 	depends on I2C
 	help
-	  If you say yes here you get support for Texas Instruments ADS1100 and
-	  ADS1000 ADC chips.
+	  If you say yes here you get support TI ADS1100 and similar single
+	  channel I2C Analog to Digital Converters
 
 	  This driver can also be built as a module. If so, the module will be
 	  called ti-ads1100.
diff --git a/drivers/iio/adc/ti-ads1100.c b/drivers/iio/adc/ti-ads1100.c
index aa8946063c7d..76de2466dc53 100644
--- a/drivers/iio/adc/ti-ads1100.c
+++ b/drivers/iio/adc/ti-ads1100.c
@@ -5,7 +5,7 @@
  * Copyright (c) 2023, Topic Embedded Products
  *
  * Datasheet: https://www.ti.com/lit/gpn/ads1100
- * IIO driver for ADS1100 and ADS1000 ADC 16-bit I2C
+ * IIO driver for ADS1100 and similar single channel ADC 16-bit I2C
  */
 
 #include <linux/bitfield.h>
@@ -15,6 +15,7 @@
 #include <linux/module.h>
 #include <linux/init.h>
 #include <linux/i2c.h>
+#include <linux/iopoll.h>
 #include <linux/mutex.h>
 #include <linux/property.h>
 #include <linux/pm_runtime.h>
@@ -39,17 +40,41 @@
 #define	ADS1100_SINGLESHOT	ADS1100_CFG_SC
 
 #define ADS1100_SLEEP_DELAY_MS	2000
+#define ADS1110_REFERENCE_VOLTAGE_MILIVOLTS 2048
+
+/* Timeout based on the minimum sample rate of 8 SPS (7500000us) */
+#define ADS1100_MAX_DRDY_TIMEOUT	7500000
 
 static const int ads1100_data_rate[] = { 128, 32, 16, 8 };
+static const int ads1110_data_rate[] = { 240, 60, 30, 15 };
 static const int ads1100_data_rate_bits[] = { 12, 14, 15, 16 };
 
+struct ads1100_config {
+	const char *name;
+	const int *data_rate;
+	bool has_reference_voltage;
+};
+
+static const struct ads1100_config ads1100_config = {
+	.name = "ads1100",
+	.data_rate = ads1100_data_rate,
+	.has_reference_voltage = false,
+};
+
+static const struct ads1100_config ads1110_config = {
+	.name = "ads1110",
+	.data_rate = ads1110_data_rate,
+	.has_reference_voltage = true,
+};
+
 struct ads1100_data {
 	struct i2c_client *client;
 	struct regulator *reg_vdd;
 	struct mutex lock;
 	int scale_avail[2 * 4]; /* 4 gain settings */
+	struct ads1100_config *ads_config;
 	u8 config;
-	bool supports_data_rate; /* Only the ADS1100 can select the rate */
+	bool supports_data_rate; /* Only the ADS1100/ADS1110 can select the rate */
 };
 
 static const struct iio_chan_spec ads1100_channel = {
@@ -85,6 +110,19 @@ static int ads1100_set_config_bits(struct ads1100_data *data, u8 mask, u8 value)
 	return 0;
 };
 
+static int ads1100_get_voltage_milivolts(struct ads1100_data *data)
+{
+	if (data->ads_config->has_reference_voltage)
+		return ADS1110_REFERENCE_VOLTAGE_MILIVOLTS;
+	else
+		return regulator_get_voltage(data->reg_vdd) / MILLI;
+}
+
+static int ads1100_get_voltage_microvolts(struct ads1100_data *data)
+{
+	return ads1100_get_voltage_milivolts(data) * MICRO / MILLI;
+}
+
 static int ads1100_data_bits(struct ads1100_data *data)
 {
 	return ads1100_data_rate_bits[FIELD_GET(ADS1100_DR_MASK, data->config)];
@@ -107,9 +145,9 @@ static int ads1100_get_adc_result(struct ads1100_data *data, int chan, int *val)
 
 	pm_runtime_put_autosuspend(&data->client->dev);
 
-	if (ret < 0) {
+	if (ret < 2) {
 		dev_err(&data->client->dev, "I2C read fail: %d\n", ret);
-		return ret;
+		return -EIO;
 	}
 
 	/* Value is always 16-bit 2's complement */
@@ -135,7 +173,7 @@ static int ads1100_set_scale(struct ads1100_data *data, int val, int val2)
 	if (!val2)
 		return -EINVAL;
 
-	microvolts = regulator_get_voltage(data->reg_vdd);
+	microvolts = ads1100_get_voltage_microvolts(data);
 	/*
 	 * val2 is in 'micro' units, n = val2 / 1000000
 	 * result must be millivolts, d = microvolts / 1000
@@ -159,22 +197,17 @@ static int ads1100_set_data_rate(struct ads1100_data *data, int chan, int rate)
 
 	size = data->supports_data_rate ? ARRAY_SIZE(ads1100_data_rate) : 1;
 	for (i = 0; i < size; i++) {
-		if (ads1100_data_rate[i] == rate)
+		if (data->ads_config->data_rate[i] == rate)
 			return ads1100_set_config_bits(data, ADS1100_DR_MASK,
-						       FIELD_PREP(ADS1100_DR_MASK, i));
+					FIELD_PREP(ADS1100_DR_MASK, i));
 	}
 
 	return -EINVAL;
 }
 
-static int ads1100_get_vdd_millivolts(struct ads1100_data *data)
-{
-	return regulator_get_voltage(data->reg_vdd) / (MICRO / MILLI);
-}
-
 static void ads1100_calc_scale_avail(struct ads1100_data *data)
 {
-	int millivolts = ads1100_get_vdd_millivolts(data);
+	int millivolts = ads1100_get_voltage_milivolts(data);
 	unsigned int i;
 
 	for (i = 0; i < ARRAY_SIZE(data->scale_avail) / 2; i++) {
@@ -196,7 +229,7 @@ static int ads1100_read_avail(struct iio_dev *indio_dev,
 	switch (mask) {
 	case IIO_CHAN_INFO_SAMP_FREQ:
 		*type = IIO_VAL_INT;
-		*vals = ads1100_data_rate;
+		*vals = data->ads_config->data_rate;
 		if (data->supports_data_rate)
 			*length = ARRAY_SIZE(ads1100_data_rate);
 		else
@@ -233,12 +266,11 @@ static int ads1100_read_raw(struct iio_dev *indio_dev,
 		return IIO_VAL_INT;
 	case IIO_CHAN_INFO_SCALE:
 		/* full-scale is the supply voltage in millivolts */
-		*val = ads1100_get_vdd_millivolts(data);
+		*val = ads1100_get_voltage_milivolts(data);
 		*val2 = 15 + FIELD_GET(ADS1100_PGA_MASK, data->config);
 		return IIO_VAL_FRACTIONAL_LOG2;
 	case IIO_CHAN_INFO_SAMP_FREQ:
-		*val = ads1100_data_rate[FIELD_GET(ADS1100_DR_MASK,
-						   data->config)];
+		*val = data->ads_config->data_rate[FIELD_GET(ADS1100_DR_MASK, data->config)];
 		return IIO_VAL_INT;
 	default:
 		return -EINVAL;
@@ -307,6 +339,7 @@ static int ads1100_probe(struct i2c_client *client)
 	struct iio_dev *indio_dev;
 	struct ads1100_data *data;
 	struct device *dev = &client->dev;
+	const struct ads1100_config *model;
 	int ret;
 
 	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
@@ -338,6 +371,12 @@ static int ads1100_probe(struct i2c_client *client)
 	if (ret)
 		return ret;
 
+	model = device_get_match_data(dev);
+	if (!model)
+		return dev_err_probe(dev, -EINVAL,
+				     "Can't get device data from firmware\n");
+
+	data->ads_config = (struct ads1100_config *)model;
 	ret = ads1100_setup(data);
 	if (ret)
 		return dev_err_probe(dev, ret,
@@ -400,16 +439,18 @@ static DEFINE_RUNTIME_DEV_PM_OPS(ads1100_pm_ops,
 				 NULL);
 
 static const struct i2c_device_id ads1100_id[] = {
-	{ "ads1100" },
-	{ "ads1000" },
+	{ .name = "ads1000", .driver_data = (kernel_ulong_t)&ads1100_config },
+	{ .name = "ads1100", .driver_data = (kernel_ulong_t)&ads1100_config },
+	{ .name = "ads1110", .driver_data = (kernel_ulong_t)&ads1110_config },
 	{ }
 };
 
 MODULE_DEVICE_TABLE(i2c, ads1100_id);
 
 static const struct of_device_id ads1100_of_match[] = {
-	{.compatible = "ti,ads1100" },
-	{.compatible = "ti,ads1000" },
+	{ .compatible = "ti,ads1000", .data = &ads1100_config },
+	{ .compatible = "ti,ads1100", .data = &ads1100_config },
+	{ .compatible = "ti,ads1110", .data = &ads1110_config },
 	{ }
 };
 
-- 
2.47.3

Re: [PATCH v3 2/3] iio: adc: Add ti-ads1110 support to ti-ads1100 driver

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:

New issues:
- [High] Improper casting of negative error codes to boolean causes an aggressive infinite loop during I2C read failures.
- [High] Integer overflow in `ads1100_get_voltage_microvolts` on 32-bit platforms due to missing parentheses.
- [Medium] I2C negative error codes are incorrectly discarded and squashed into a generic -EIO.
- [Low] The driver hardcodes `ARRAY_SIZE(ads1100_data_rate)` to determine the length of dynamically selected data rate arrays.
- [Low] Dropping the `const` qualifier when assigning the firmware device match data pointer.

Pre-existing issues:
- [High] Missing wait for ADC conversion completion after device power-on causes reads of invalid data.
--

commit 05622ebdba67f2ec5ab414bed153865ba64b4705
Author: Jakub Szczudlo <jakubszczudlo40@gmail.com>
Subject: iio: adc: Add ti-ads1110 support to ti-ads1100 driver

Add ADS1110 support that have faster datarate than ADS1100, it also uses
internal voltage reference of 2.048V for measurement.

[ ... ]

> diff --git a/drivers/iio/adc/ti-ads1100.c b/drivers/iio/adc/ti-ads1100.c
> index aa8946063c7d6..76de2466dc53b 100644
> --- a/drivers/iio/adc/ti-ads1100.c
> +++ b/drivers/iio/adc/ti-ads1100.c
> @@ -39,17 +40,41 @@
>  #define	ADS1100_SINGLESHOT	ADS1100_CFG_SC
>  
>  #define ADS1100_SLEEP_DELAY_MS	2000
> +#define ADS1110_REFERENCE_VOLTAGE_MILIVOLTS 2048
> +
> +/* Timeout based on the minimum sample rate of 8 SPS (7500000us) */
> +#define ADS1100_MAX_DRDY_TIMEOUT	7500000

[Severity: High]
Does an I2C read failure cause an aggressive infinite loop when polling for
data readiness?

Looking at ads1100_new_data_ready(), it casts the negative error code
returned from i2c_master_recv() to a boolean:

static bool ads1100_new_data_ready(struct ads1100_data *data)
{
    ...
    ret = i2c_master_recv(data->client, (char *)&buffer, sizeof(buffer));
    if (ret < 3) {
        dev_err(&data->client->dev, "I2C read fail: %d\n", ret);
        return ret;
    }
    ...
}

Because a negative value evaluates to true, ads1100_poll_data_ready()
receives true, which indicates the device is still busy. This causes
read_poll_timeout() to continue looping rather than breaking.

Consequently, any I2C communication error triggers a rapid polling storm
that hammers the bus repeatedly, blocking the thread and spamming the
kernel log until the 7.5-second timeout expires.

[ ... ]

> @@ -85,6 +110,19 @@ static int ads1100_set_config_bits(struct ads1100_data *data, u8 mask, u8 value)
>  	return 0;
>  };
>  
> +static int ads1100_get_voltage_milivolts(struct ads1100_data *data)
> +{
> +	if (data->ads_config->has_reference_voltage)
> +		return ADS1110_REFERENCE_VOLTAGE_MILIVOLTS;
> +	else
> +		return regulator_get_voltage(data->reg_vdd) / MILLI;
> +}
> +
> +static int ads1100_get_voltage_microvolts(struct ads1100_data *data)
> +{
> +	return ads1100_get_voltage_milivolts(data) * MICRO / MILLI;
> +}

[Severity: High]
Can this calculation overflow on 32-bit platforms?

Because MICRO is defined as 1000000UL in linux/units.h, evaluating this
left-to-right multiplies the millivolts by 1,000,000 first. For a typical
5V supply, this evaluates to 5,000,000,000UL.

On architectures where unsigned long is 32 bits, this overflows the
maximum limit and wraps around to 705,032,704. After division by MILLI,
the function incorrectly returns 705,032 microvolts instead of 5,000,000.

This completely breaks the IIO scale and gain math calculations. Would it
be better to add parentheses to divide MICRO by MILLI first?

[ ... ]

> @@ -107,9 +145,9 @@ static int ads1100_get_adc_result(struct ads1100_data *data, int chan, int *val)
>  
>  	pm_runtime_put_autosuspend(&data->client->dev);

[Severity: High]
This isn't a bug introduced by this patch, but does the I2C read happen
immediately after waking the device, entirely missing the ADC's hardware
conversion delay?

When the device is woken from runtime suspend via
pm_runtime_resume_and_get(), the ADC powers up and begins a new continuous
conversion. However, ads1100_get_adc_result() immediately reads from the
I2C data register without waiting for the conversion to complete (which
takes up to 125ms at 8 SPS).

If userspace periodically polls the ADC at intervals longer than the
2-second autosuspend delay, the device will suspend between every read.
According to the ADS1100 datasheet, reading before the first conversion
is complete returns 0, making the device completely unusable for periodic
polling.

>  
> -	if (ret < 0) {
> +	if (ret < 2) {
>  		dev_err(&data->client->dev, "I2C read fail: %d\n", ret);
> -		return ret;
> +		return -EIO;
>  	}

[Severity: Medium]
Is it intentional to discard negative I2C error codes and replace them
with -EIO?

When i2c_master_recv() fails with a negative error code (e.g., -EAGAIN,
-ENXIO), replacing it with a generic -EIO breaks proper error propagation
up the IIO stack. This prevents upper layers from correctly handling
recoverable errors like arbitration loss or restarts.

This is a regression introduced by the patch to fix unhandled short reads.

[ ... ]

> @@ -159,22 +197,17 @@ static int ads1100_set_data_rate(struct ads1100_data *data, int chan, int rate)
>  
>  	size = data->supports_data_rate ? ARRAY_SIZE(ads1100_data_rate) : 1;
>  	for (i = 0; i < size; i++) {
> -		if (ads1100_data_rate[i] == rate)
> +		if (data->ads_config->data_rate[i] == rate)
>  			return ads1100_set_config_bits(data, ADS1100_DR_MASK,
> -						       FIELD_PREP(ADS1100_DR_MASK, i));
> +					FIELD_PREP(ADS1100_DR_MASK, i));

[Severity: Low]
Is it safe to hardcode the loop iteration count to the size of the
ads1100_data_rate array?

While both ads1100_data_rate and ads1110_data_rate currently have 4
elements, bounding iteration over a pointer that can point to different
arrays using the compile-time size of a specific static array is fragile.
If a new chip variant with a different number of rates is added, this
could cause silent out-of-bounds reads or truncation.

[ ... ]

> @@ -196,7 +229,7 @@ static int ads1100_read_avail(struct iio_dev *indio_dev,
>  	switch (mask) {
>  	case IIO_CHAN_INFO_SAMP_FREQ:
>  		*type = IIO_VAL_INT;
> -		*vals = ads1100_data_rate;
> +		*vals = data->ads_config->data_rate;
>  		if (data->supports_data_rate)
>  			*length = ARRAY_SIZE(ads1100_data_rate);

[Severity: Low]
Similarly, does this hardcode the array length exposed to userspace to
the size of ads1100_data_rate regardless of the active hardware
configuration?

[ ... ]

> @@ -338,6 +371,12 @@ static int ads1100_probe(struct i2c_client *client)
>  	if (ret)
>  		return ret;
>  
> +	model = device_get_match_data(dev);
> +	if (!model)
> +		return dev_err_probe(dev, -EINVAL,
> +				     "Can't get device data from firmware\n");
> +
> +	data->ads_config = (struct ads1100_config *)model;

[Severity: Low]
Does this explicit cast drop the const qualifier from the static match
data?

The firmware match data returned by device_get_match_data() is a
const void * pointing to a static read-only .rodata structure
(ads1100_config or ads1110_config). Explicitly casting this to a mutable
struct ads1100_config * violates const correctness and risks undefined
behavior if the struct is ever modified by a future patch.

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260613190957.654798-1-jakubszczudlo40@gmail.com?part=2

[PATCH v3 3/3] iio: adc: Fix incorrect reading when datarate changed in single mode

[Jakub Szczudlo]

When device is suspended and it is in single mode then changing
datarate doesn't make it actual wait for new measurement, so to
be sure that read after change is correct functions that changes
datarate and gain will wait for new data.

Signed-off-by: Jakub Szczudlo <jakubszczudlo40@gmail.com>
---
 drivers/iio/adc/ti-ads1100.c | 55 ++++++++++++++++++++++++++++++++++--
 1 file changed, 52 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/adc/ti-ads1100.c b/drivers/iio/adc/ti-ads1100.c
index 76de2466dc53..195394665cd1 100644
--- a/drivers/iio/adc/ti-ads1100.c
+++ b/drivers/iio/adc/ti-ads1100.c
@@ -123,6 +123,36 @@ static int ads1100_get_voltage_microvolts(struct ads1100_data *data)
 	return ads1100_get_voltage_milivolts(data) * MICRO / MILLI;
 }
 
+static bool ads1100_new_data_ready(struct ads1100_data *data)
+{
+	int ret;
+	u8 buffer[3];
+
+	ret = i2c_master_recv(data->client, (char *)&buffer, sizeof(buffer));
+	if (ret < 3) {
+		dev_err(&data->client->dev, "I2C read fail: %d\n", ret);
+		return ret;
+	}
+
+	return FIELD_GET(ADS1100_CFG_ST_BSY, buffer[2]);
+}
+
+static int ads1100_poll_data_ready(struct ads1100_data *data)
+{
+	u8 buffer[3];
+	bool data_ready;
+	int datarate = data->ads_config->data_rate[FIELD_GET(ADS1100_DR_MASK, data->config)];
+   // To be sure we wait 5 times more than datarate
+	unsigned long wait_time = DIV_ROUND_CLOSEST(MICRO, 5 * datarate);
+
+	/* To be sure that polled value will have value after config change */
+	i2c_master_recv(data->client, (char *)&buffer, sizeof(buffer));
+
+	return read_poll_timeout(ads1100_new_data_ready, data_ready,
+				 !data_ready, wait_time,
+				 ADS1100_MAX_DRDY_TIMEOUT, false, data);
+}
+
 static int ads1100_data_bits(struct ads1100_data *data)
 {
 	return ads1100_data_rate_bits[FIELD_GET(ADS1100_DR_MASK, data->config)];
@@ -165,6 +194,7 @@ static int ads1100_set_scale(struct ads1100_data *data, int val, int val2)
 {
 	int microvolts;
 	int gain;
+	int ret;
 
 	/* With Vdd between 2.7 and 5V, the scale is always below 1 */
 	if (val)
@@ -185,21 +215,40 @@ static int ads1100_set_scale(struct ads1100_data *data, int val, int val2)
 	if (gain < BIT(0) || gain > BIT(3))
 		return -EINVAL;
 
+	ret = pm_runtime_resume_and_get(&data->client->dev);
+	if (ret < 0)
+		return ret;
+
 	ads1100_set_config_bits(data, ADS1100_PGA_MASK, ffs(gain) - 1);
 
-	return 0;
+	ret = ads1100_poll_data_ready(data);
+
+	pm_runtime_put_autosuspend(&data->client->dev);
+
+	return ret;
 }
 
 static int ads1100_set_data_rate(struct ads1100_data *data, int chan, int rate)
 {
 	unsigned int i;
 	unsigned int size;
+	int ret;
 
 	size = data->supports_data_rate ? ARRAY_SIZE(ads1100_data_rate) : 1;
 	for (i = 0; i < size; i++) {
-		if (data->ads_config->data_rate[i] == rate)
-			return ads1100_set_config_bits(data, ADS1100_DR_MASK,
+		if (data->ads_config->data_rate[i] != rate)
+			continue;
+
+		ret = pm_runtime_resume_and_get(&data->client->dev);
+		if (ret < 0)
+			return ret;
+
+		ads1100_set_config_bits(data, ADS1100_DR_MASK,
 					FIELD_PREP(ADS1100_DR_MASK, i));
+		ret = ads1100_poll_data_ready(data);
+
+		pm_runtime_put_autosuspend(&data->client->dev);
+		return ret;
 	}
 
 	return -EINVAL;
-- 
2.47.3

Re: [PATCH v3 3/3] iio: adc: Fix incorrect reading when datarate changed in single mode

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

New issues:
- [High] Incorrect boolean cast of I2C read errors causes infinite polling loop and log spam.
- [High] The return value of `ads1100_set_config_bits()` is ignored, masking I2C write failures.
- [High] Polling the ST/BSY bit will always time out because the device is placed in continuous conversion mode.
- [Medium] The initial dummy `i2c_master_recv()` ignores its return value.

Pre-existing issues:
- [High] The read path returns stale data after waking from autosuspend.
--

commit 4b2f1aca65a15af2d7f4a7cbbd2f3fc74aa691ce
Author: Jakub Szczudlo <jakubszczudlo40@gmail.com>

iio: adc: Fix incorrect reading when datarate changed in single mode

When device is suspended and it is in single mode then changing
datarate doesn't make it actual wait for new measurement, so to
be sure that read after change is correct functions that changes
datarate and gain will wait for new data.

> diff --git a/drivers/iio/adc/ti-ads1100.c b/drivers/iio/adc/ti-ads1100.c
> index 76de2466dc53b..2ec5c104afcf9 100644
> --- a/drivers/iio/adc/ti-ads1100.c
> +++ b/drivers/iio/adc/ti-ads1100.c
> @@ -123,6 +123,36 @@ static int ads1100_get_voltage_microvolts(struct ads1100_data *data)
>  	return ads1100_get_voltage_milivolts(data) * MICRO / MILLI;
>  }
>  
> +static bool ads1100_new_data_ready(struct ads1100_data *data)
> +{
> +	int ret;
> +	u8 buffer[3];
> +
> +	ret = i2c_master_recv(data->client, (char *)&buffer, sizeof(buffer));
> +	if (ret < 3) {
> +		dev_err(&data->client->dev, "I2C read fail: %d\n", ret);
> +		return ret;

[Severity: High]
Since this function returns a bool, doesn't returning a negative error
code implicitly cast it to true? If i2c_master_recv() fails, this will
cause the caller to interpret the error as data being ready.

> +	}
> +
> +	return FIELD_GET(ADS1100_CFG_ST_BSY, buffer[2]);
> +}
> +
> +static int ads1100_poll_data_ready(struct ads1100_data *data)
> +{
> +	u8 buffer[3];
> +	bool data_ready;
> +	int datarate = data->ads_config->data_rate[FIELD_GET(ADS1100_DR_MASK, data->config)];
> +   // To be sure we wait 5 times more than datarate
> +	unsigned long wait_time = DIV_ROUND_CLOSEST(MICRO, 5 * datarate);
> +
> +	/* To be sure that polled value will have value after config change */
> +	i2c_master_recv(data->client, (char *)&buffer, sizeof(buffer));

[Severity: Medium]
Is it safe to ignore the return value of this initial dummy read?

If the bus is inaccessible or the device is disconnected, the error is
silently masked before entering the polling loop.

> +
> +	return read_poll_timeout(ads1100_new_data_ready, data_ready,
> +				 !data_ready, wait_time,
> +				 ADS1100_MAX_DRDY_TIMEOUT, false, data);

[Severity: High]
If ads1100_new_data_ready() returns true due to the implicit error casting
mentioned above, !data_ready evaluates to false. Won't this cause the
read_poll_timeout() loop to never break, resulting in a full 7.5-second
timeout while flooding the kernel log with dev_err messages?

Also, the ST/BSY bit seems to permanently read as 1 (busy) when the device
is in continuous mode. Since pm_runtime_resume_and_get() places the device
in continuous mode via ads1100_runtime_resume(), won't this polling loop
always hit the timeout anyway?

> +}
> +
[ ... ]
> @@ -185,21 +216,40 @@ static int ads1100_set_scale(struct ads1100_data *data, int val, int val2)
>  	if (gain < BIT(0) || gain > BIT(3))
>  		return -EINVAL;
>  
> +	ret = pm_runtime_resume_and_get(&data->client->dev);
> +	if (ret < 0)
> +		return ret;
> +
>  	ads1100_set_config_bits(data, ADS1100_PGA_MASK, ffs(gain) - 1);

[Severity: High]
Should we check the return value of ads1100_set_config_bits() here?

If the I2C configuration write fails, the hardware configuration and the
internal state remain unmodified, but the code will still proceed to poll
for data using the old configuration. We should probably propagate the
error and ensure pm_runtime_put_autosuspend() is still called on the error
path.

>  
> -	return 0;
> +	ret = ads1100_poll_data_ready(data);
> +
> +	pm_runtime_put_autosuspend(&data->client->dev);
> +
> +	return ret;
>  }
>  
>  static int ads1100_set_data_rate(struct ads1100_data *data, int chan, int rate)
>  {
>  	unsigned int i;
>  	unsigned int size;
> +	int ret;
>  
>  	size = data->supports_data_rate ? ARRAY_SIZE(ads1100_data_rate) : 1;
>  	for (i = 0; i < size; i++) {
> -		if (data->ads_config->data_rate[i] == rate)
> -			return ads1100_set_config_bits(data, ADS1100_DR_MASK,
> +		if (data->ads_config->data_rate[i] != rate)
> +			continue;
> +
> +		ret = pm_runtime_resume_and_get(&data->client->dev);
> +		if (ret < 0)
> +			return ret;
> +
> +		ads1100_set_config_bits(data, ADS1100_DR_MASK,
>  					FIELD_PREP(ADS1100_DR_MASK, i));

[Severity: High]
Similar to ads1100_set_scale(), does ignoring the return value of
ads1100_set_config_bits() mask I2C write failures here?

> +		ret = ads1100_poll_data_ready(data);
> +
> +		pm_runtime_put_autosuspend(&data->client->dev);
> +		return ret;
>  	}
>  
>  	return -EINVAL;

[Severity: High]
This isn't a bug introduced by this patch, but there appears to be a
pre-existing issue in ads1100_get_adc_result().

When ads1100_get_adc_result() is called, it wakes the device with
pm_runtime_resume_and_get(), which places the device into continuous
conversion mode. However, the driver immediately issues an I2C read
without waiting for the first continuous conversion to complete:

ads1100_get_adc_result() {
    ret = pm_runtime_resume_and_get(&data->client->dev);
    if (ret < 0)
        return ret;

    ret = i2c_master_recv(data->client, (char *)&buffer, sizeof(buffer));
    ...
}

Doesn't this cause the read path to return stale data from the previous
active session when waking from autosuspend?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260613190957.654798-1-jakubszczudlo40@gmail.com?part=3