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

[PATCH v4 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>

---
V3 -> V4:
- make fixes patch the first change in the series
- correct error handling when short read
- use ACQUIRE macros from pm_runtime.h in new functions
- Link to v3: https://lore.kernel.org/linux-iio/20260613190957.654798-1-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):
  iio: adc: Fix incorrect reading when datarate changed in single mode
  dt-bindings: iio: adc: ti,ads1100: add support for ADS1110
  iio: adc: Add ti-ads1110 support to ti-ads1100 driver

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

-- 
2.47.3

[PATCH v4 1/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.

Fixes: 541880542f2b ("iio: adc: Add TI ADS1100 and ADS1000")
Signed-off-by: Jakub Szczudlo <jakubszczudlo40@gmail.com>
---
 drivers/iio/adc/ti-ads1100.c | 74 ++++++++++++++++++++++++++++++++++--
 1 file changed, 70 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/adc/ti-ads1100.c b/drivers/iio/adc/ti-ads1100.c
index 9fe8d54cce83..e3c801381434 100644
--- a/drivers/iio/adc/ti-ads1100.c
+++ b/drivers/iio/adc/ti-ads1100.c
@@ -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>
@@ -43,6 +44,9 @@
 static const int ads1100_data_rate[] = { 128, 32, 16, 8 };
 static const int ads1100_data_rate_bits[] = { 12, 14, 15, 16 };
 
+/* Timeout based on the minimum sample rate of 8 SPS (7.5s) */
+#define ADS1100_MAX_DRDY_TIMEOUT_US	7500000
+
 struct ads1100_data {
 	struct i2c_client *client;
 	struct regulator *reg_vdd;
@@ -123,10 +127,49 @@ static int ads1100_get_adc_result(struct ads1100_data *data, int chan, int *val)
 	return 0;
 }
 
+static bool ads1100_new_data_not_ready(struct ads1100_data *data)
+{
+	int ret;
+	u8 buffer[3];
+
+	ret = i2c_master_recv(data->client, (char *)&buffer, sizeof(buffer));
+	if (ret < 0) {
+		dev_err(&data->client->dev, "I2C read fail: %d\n", ret);
+		return true;
+	} else if (ret < 3) {
+		dev_err(&data->client->dev, "Short I2C read\n");
+		return true;
+	}
+
+	return FIELD_GET(ADS1100_CFG_ST_BSY, buffer[2]);
+}
+
+static int ads1100_poll_data_ready(struct ads1100_data *data)
+{
+	int ret;
+	u8 buffer[3];
+	bool data_ready;
+	int datarate = ads1100_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 */
+	ret = i2c_master_recv(data->client, (char *)&buffer, sizeof(buffer));
+	if (ret < 0) {
+		dev_err(&data->client->dev, "I2C read fail: %d\n", ret);
+		return ret;
+	}
+
+	return read_poll_timeout(ads1100_new_data_not_ready, data_ready,
+				 !data_ready, wait_time,
+				 ADS1100_MAX_DRDY_TIMEOUT_US, false, data);
+}
+
 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)
@@ -135,6 +178,12 @@ static int ads1100_set_scale(struct ads1100_data *data, int val, int val2)
 	if (!val2)
 		return -EINVAL;
 
+	PM_RUNTIME_ACQUIRE_AUTOSUSPEND(&data->client->dev, pm);
+
+	ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
+	if (ret)
+		return ret;
+
 	microvolts = regulator_get_voltage(data->reg_vdd);
 	/*
 	 * val2 is in 'micro' units, n = val2 / 1000000
@@ -149,19 +198,36 @@ static int ads1100_set_scale(struct ads1100_data *data, int val, int val2)
 
 	ads1100_set_config_bits(data, ADS1100_PGA_MASK, ffs(gain) - 1);
 
-	return 0;
+	ret = ads1100_poll_data_ready(data);
+
+	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 (ads1100_data_rate[i] == rate)
-			return ads1100_set_config_bits(data, ADS1100_DR_MASK,
-						       FIELD_PREP(ADS1100_DR_MASK, i));
+		if (ads1100_data_rate[i] != rate)
+			continue;
+
+		PM_RUNTIME_ACQUIRE_AUTOSUSPEND(&data->client->dev, pm);
+
+		ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
+		if (ret)
+			return ret;
+
+		ret = ads1100_set_config_bits(data, ADS1100_DR_MASK,
+					      FIELD_PREP(ADS1100_DR_MASK, i));
+		if (ret)
+			return ret;
+
+		ret = ads1100_poll_data_ready(data);
+
+		return ret;
 	}
 
 	return -EINVAL;
-- 
2.47.3

[PATCH v4 2/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
data rates and have internal voltage reference that is always 2.048V.
Also correct order of ads so they will be sorted alphabetically.

Signed-off-by: Jakub Szczudlo <jakubszczudlo40@gmail.com>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.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 v4 3/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 | 81 +++++++++++++++++++++++++++---------
 2 files changed, 64 insertions(+), 23 deletions(-)

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 1c663c98c6c9..30198335c63b 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -1765,11 +1765,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 e3c801381434..ec79a89464fb 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>
@@ -40,20 +40,44 @@
 #define	ADS1100_SINGLESHOT	ADS1100_CFG_SC
 
 #define ADS1100_SLEEP_DELAY_MS	2000
+#define ADS1110_INTERNAL_REF_mV 2048
 
 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 };
 
 /* Timeout based on the minimum sample rate of 8 SPS (7.5s) */
 #define ADS1100_MAX_DRDY_TIMEOUT_US	7500000
 
+struct ads1100_config {
+	const char *name;
+	const int *data_rate;
+	const int data_rate_count;
+	bool has_internal_vref_only;
+};
+
+static const struct ads1100_config ads1100_config = {
+	.name = "ads1100",
+	.data_rate = ads1100_data_rate,
+	.data_rate_count = ARRAY_SIZE(ads1100_data_rate),
+	.has_internal_vref_only = false,
+};
+
+static const struct ads1100_config ads1110_config = {
+	.name = "ads1110",
+	.data_rate = ads1110_data_rate,
+	.data_rate_count = ARRAY_SIZE(ads1110_data_rate),
+	.has_internal_vref_only = true,
+};
+
 struct ads1100_data {
 	struct i2c_client *client;
 	struct regulator *reg_vdd;
 	struct mutex lock;
 	int scale_avail[2 * 4]; /* 4 gain settings */
+	const struct ads1100_config *ads_config;
 	u8 config;
-	bool supports_data_rate; /* Only the ADS1100 can select the rate */
+	bool supports_data_rate;
 };
 
 static const struct iio_chan_spec ads1100_channel = {
@@ -89,6 +113,14 @@ static int ads1100_set_config_bits(struct ads1100_data *data, u8 mask, u8 value)
 	return 0;
 };
 
+static int ads1100_get_vref_milivolts(struct ads1100_data *data)
+{
+	if (data->ads_config->has_internal_vref_only)
+		return ADS1110_INTERNAL_REF_mV;
+
+	return regulator_get_voltage(data->reg_vdd) / MILLI;
+}
+
 static int ads1100_data_bits(struct ads1100_data *data)
 {
 	return ads1100_data_rate_bits[FIELD_GET(ADS1100_DR_MASK, data->config)];
@@ -114,6 +146,9 @@ static int ads1100_get_adc_result(struct ads1100_data *data, int chan, int *val)
 	if (ret < 0) {
 		dev_err(&data->client->dev, "I2C read fail: %d\n", ret);
 		return ret;
+	} else if (ret < 2) {
+		dev_err(&data->client->dev, "Short I2C read\n");
+		return -EIO;
 	}
 
 	/* Value is always 16-bit 2's complement */
@@ -184,7 +219,7 @@ static int ads1100_set_scale(struct ads1100_data *data, int val, int val2)
 	if (ret)
 		return ret;
 
-	microvolts = regulator_get_voltage(data->reg_vdd);
+	microvolts = ads1100_get_vref_milivolts(data) * (MICRO / MILLI);
 	/*
 	 * val2 is in 'micro' units, n = val2 / 1000000
 	 * result must be millivolts, d = microvolts / 1000
@@ -209,9 +244,9 @@ static int ads1100_set_data_rate(struct ads1100_data *data, int chan, int rate)
 	unsigned int size;
 	int ret;
 
-	size = data->supports_data_rate ? ARRAY_SIZE(ads1100_data_rate) : 1;
+	size = data->supports_data_rate ? data->ads_config->data_rate_count : 1;
 	for (i = 0; i < size; i++) {
-		if (ads1100_data_rate[i] != rate)
+		if (data->ads_config->data_rate[i] != rate)
 			continue;
 
 		PM_RUNTIME_ACQUIRE_AUTOSUSPEND(&data->client->dev, pm);
@@ -233,14 +268,9 @@ static int ads1100_set_data_rate(struct ads1100_data *data, int chan, int rate)
 	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_vref_milivolts(data);
 	unsigned int i;
 
 	for (i = 0; i < ARRAY_SIZE(data->scale_avail) / 2; i++) {
@@ -262,9 +292,9 @@ 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);
+			*length = data->ads_config->data_rate_count;
 		else
 			*length = 1;
 		return IIO_AVAIL_LIST;
@@ -283,6 +313,7 @@ static int ads1100_read_raw(struct iio_dev *indio_dev,
 			    int *val2, long mask)
 {
 	int ret;
+	int data_rate_index;
 	struct ads1100_data *data = iio_priv(indio_dev);
 
 	guard(mutex)(&data->lock);
@@ -299,12 +330,12 @@ 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_vref_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)];
+		data_rate_index = FIELD_GET(ADS1100_DR_MASK, data->config);
+		*val = data->ads_config->data_rate[data_rate_index];
 		return IIO_VAL_INT;
 	default:
 		return -EINVAL;
@@ -373,6 +404,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));
@@ -404,6 +436,13 @@ static int ads1100_probe(struct i2c_client *client)
 	if (ret)
 		return ret;
 
+	model = i2c_get_match_data(client);
+	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,
@@ -466,16 +505,18 @@ static DEFINE_RUNTIME_DEV_PM_OPS(ads1100_pm_ops,
 				 NULL);
 
 static const struct i2c_device_id ads1100_id[] = {
-	{ .name = "ads1100" },
-	{ .name = "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 v4 3/3] iio: adc: Add ti-ads1110 support to ti-ads1100 driver

[sashiko-bot]

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

New issues:
- [Medium] Hardcoded IIO device name ignores the newly added device configuration name.
- [Medium] Hardcoded ads1100_data_rate array used instead of config-specific data_rate in ads1100_poll_data_ready.

Pre-existing issues:
- [High] Missing PM runtime resume on device removal causes an unbalanced regulator_disable() call.
--

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

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 e3c801381434d..ec79a89464fb6 100644
> --- a/drivers/iio/adc/ti-ads1100.c
> +++ b/drivers/iio/adc/ti-ads1100.c
> @@ -40,20 +40,44 @@
>  #define	ADS1100_SINGLESHOT	ADS1100_CFG_SC
>  
>  #define ADS1100_SLEEP_DELAY_MS	2000
> +#define ADS1110_INTERNAL_REF_mV 2048
>  
>  static const int ads1100_data_rate[] = { 128, 32, 16, 8 };
> +static const int ads1110_data_rate[] = { 240, 60, 30, 15 };

[Severity: Medium]
Will this result in using the slower ads1100_data_rate array for the ADS1110
when polling for data readiness?

Looking at ads1100_poll_data_ready(), it still accesses the hardcoded
ads1100_data_rate array directly:

static int ads1100_poll_data_ready(struct ads1100_data *data)
{
	...
	int datarate = ads1100_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);
	...
}

Should this be updated to use data->ads_config->data_rate instead?

[ ... ]

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

[Severity: Medium]
Does this ignore the newly added device configuration name for the IIO device?

In ads1100_probe(), indio_dev->name is hardcoded earlier in the function:

	indio_dev->name = "ads1100";

Should this be updated to use data->ads_config->name so the user-space
sysfs name correctly identifies the ADS1110?

[Severity: High]
This is a pre-existing issue, but does the lack of a PM runtime resume on
device removal cause an unbalanced regulator disable?

If the device auto-suspends during runtime idle, the vdd regulator is disabled
via ads1100_runtime_suspend():

static int ads1100_runtime_suspend(struct device *dev)
{
	...
	regulator_disable(data->reg_vdd);
	...
}

When the driver is unbound, devres calls pm_runtime_disable() (which does not
resume the device), followed by ads1100_reg_disable() which attempts to call
regulator_disable() again on the already-disabled regulator, triggering a
kernel WARNING.

Could we ensure the device is resumed during driver unbind to prevent this?

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

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

[sashiko-bot]

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

New issues:
- [High] I2C read failures inside the polling loop are improperly mapped to 'device busy', causing a 7.5-second hang and massive dmesg log flooding.
- [High] The data ready polling logic fails to synchronize because the ST/BSY bit is likely static in continuous conversion mode.
- [Medium] The `ads1100_poll_data_ready` function performs a completely redundant I2C read that wastes bus bandwidth.

Pre-existing issues:
- [High] The return value of `ads1100_set_config_bits` is ignored in `ads1100_set_scale`, leading to silent configuration failures.
- [High] Reading the ADC result immediately after runtime resume returns an invalid or stale conversion.
- [High] I2C read functions allocate transfer buffers on the stack, which is unsafe for DMA operations.
--

commit 06506d1fcb678ffc0137dbc1df3913f95ce47327
Author: Jakub Szczudlo <jakubszczudlo40@gmail.com>
Subject: iio: adc: Fix incorrect reading when datarate changed in single mode

This commit attempts to fix incorrect ADC readings after changing
datarate or gain while suspended in single mode. It introduces a
polling loop to wait for new data before returning from the configuration
functions.

> diff --git a/drivers/iio/adc/ti-ads1100.c b/drivers/iio/adc/ti-ads1100.c
> index 9fe8d54cce83c..e3c801381434d 100644
> --- a/drivers/iio/adc/ti-ads1100.c
> +++ b/drivers/iio/adc/ti-ads1100.c
[ ... ]
> @@ -123,10 +127,49 @@ static int ads1100_get_adc_result(struct ads1100_data *data, int chan, int *val)
>  	return 0;
>  }

[Severity: High]
This is a pre-existing issue, but doesn't reading the ADC result immediately
after runtime resume return an invalid or stale conversion?

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

When the device auto-suspends, a subsequent sysfs read of in_voltage0_raw
calls pm_runtime_resume_and_get(), which transitions the ADC to continuous
mode and starts a new conversion. However, ads1100_get_adc_result()
immediately reads the I2C output register without waiting for the conversion
to complete, which can take up to 125ms at 8 SPS.

> +static bool ads1100_new_data_not_ready(struct ads1100_data *data)
> +{
> +	int ret;
> +	u8 buffer[3];
> +
> +	ret = i2c_master_recv(data->client, (char *)&buffer, sizeof(buffer));

[Severity: High]
This isn't a bug introduced by this patch, but is it safe to use
stack-allocated buffers for I2C transfers?

Using stack-allocated buffers for I2C transfers violates subsystem rules and
can lead to memory corruption or kernel panics on architectures where
CONFIG_VMAP_STACK is enabled and the underlying I2C controller utilizes DMA.

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

[Severity: High]
Does returning true on hard I2C errors trap the driver in an infinite polling
loop?

Returning true signals to the read_poll_timeout() macro that the device is
merely not ready, causing it to endlessly retry for up to 7.5 seconds instead
of aborting. This masks the actual I2C error code with -ETIMEDOUT and
generates massive dmesg log flooding.

> +	} else if (ret < 3) {
> +		dev_err(&data->client->dev, "Short I2C read\n");
> +		return true;
> +	}
> +
> +	return FIELD_GET(ADS1100_CFG_ST_BSY, buffer[2]);

[Severity: High]
Will this polling logic synchronize properly in continuous conversion mode?

When changing scale or datarate, PM_RUNTIME_ACQUIRE_AUTOSUSPEND() is called,
transitioning the ADC to continuous conversion mode. In continuous conversion
mode on these TI ADCs, the ST_BSY bit does not pulse to indicate new data;
it is typically static.

If ST_BSY reads as 0, ads1100_new_data_not_ready() returns false, breaking the
loop instantly. If it reads as 1, the driver would hang for 7.5 seconds.

> +}
> +
> +static int ads1100_poll_data_ready(struct ads1100_data *data)
> +{
> +	int ret;
> +	u8 buffer[3];
> +	bool data_ready;
> +	int datarate = ads1100_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 */
> +	ret = i2c_master_recv(data->client, (char *)&buffer, sizeof(buffer));

[Severity: Medium]
Is this initial I2C read redundant?

Executing ads1100_poll_data_ready() triggers this initial i2c_master_recv(),
discards the populated local buffer, and immediately enters
read_poll_timeout() with sleep_before_read = false. This instantly performs
another identical i2c_master_recv() inside ads1100_new_data_not_ready(),
needlessly occupying the I2C bus.

[ ... ]
> @@ -149,19 +198,36 @@ static int ads1100_set_scale(struct ads1100_data *data, int val, int val2)
>  
>  	ads1100_set_config_bits(data, ADS1100_PGA_MASK, ffs(gain) - 1);
>  
> -	return 0;
> +	ret = ads1100_poll_data_ready(data);
> +
> +	return ret;
>  }

[Severity: High]
This is a pre-existing issue, but does ads1100_set_scale() ignore the return
value of ads1100_set_config_bits()?

If the I2C write to update the hardware PGA scale fails, the error is
discarded. The function proceeds to poll for new data and may eventually
return success to userspace.

This leads to silently corrupted or miscalculated voltage measurements because
the sysfs attribute appears to have updated while the ADC is physically
operating with the old scale.

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

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

[Andy Shevchenko]

On Tue, Jun 23, 2026 at 12:15:48AM +0200, Jakub Szczudlo wrote:
> 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.

...

> +/* Timeout based on the minimum sample rate of 8 SPS (7.5s) */
> +#define ADS1100_MAX_DRDY_TIMEOUT_US	7500000

Not sure if the multiplier will look good here

#define ADS1100_MAX_DRDY_TIMEOUT_US	(7500 * USEC_PER_MSEC)

(comment might need an update to use 7500 ms, but see above).

...

> +static bool ads1100_new_data_not_ready(struct ads1100_data *data)
> +{
> +	int ret;
> +	u8 buffer[3];
> +
> +	ret = i2c_master_recv(data->client, (char *)&buffer, sizeof(buffer));
> +	if (ret < 0) {
> +		dev_err(&data->client->dev, "I2C read fail: %d\n", ret);
> +		return true;
> +	} else if (ret < 3) {

sizeof()

> +		dev_err(&data->client->dev, "Short I2C read\n");
> +		return true;
> +	}
> +
> +	return FIELD_GET(ADS1100_CFG_ST_BSY, buffer[2]);
> +}

...

> +static int ads1100_poll_data_ready(struct ads1100_data *data)
> +{
> +	int ret;
> +	u8 buffer[3];
> +	bool data_ready;
> +	int datarate = ads1100_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);

First of all, reversed xmas tree order can be better (especially
when something is assigned). Second, use units in the variable name,
wait_time_us. And at last, use USEC_PER_SEC instead of MICRO
(this will need time.h to be included if not yet).

> +	/* To be sure that polled value will have value after config change */
> +	ret = i2c_master_recv(data->client, (char *)&buffer, sizeof(buffer));
> +	if (ret < 0) {
> +		dev_err(&data->client->dev, "I2C read fail: %d\n", ret);
> +		return ret;
> +	}

> +	return read_poll_timeout(ads1100_new_data_not_ready, data_ready,
> +				 !data_ready, wait_time,
> +				 ADS1100_MAX_DRDY_TIMEOUT_US, false, data);

Why not readx_poll_timeout()? It's a short cut for the one-argument "read"
function.

> +}

...

>  	ads1100_set_config_bits(data, ADS1100_PGA_MASK, ffs(gain) - 1);
>  
> -	return 0;
> +	ret = ads1100_poll_data_ready(data);
> +
> +	return ret;

Is it specifically done due to next patch? But this one is marked as Fix and
will go deep back in the releases. For them this will look unjustified. Just
use

	return ads1100_...;

here.


>  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 (ads1100_data_rate[i] == rate)
> -			return ads1100_set_config_bits(data, ADS1100_DR_MASK,
> -						       FIELD_PREP(ADS1100_DR_MASK, i));

> +		if (ads1100_data_rate[i] != rate)
> +			continue;

This will look better if you break here and add a check

	if (i == size)
		return -EINVAL;

	... then your new code...

	return ads1100_...;


> +		PM_RUNTIME_ACQUIRE_AUTOSUSPEND(&data->client->dev, pm);

> +

This blank line is not needed as they are coupled, but I don't know if we have
an agreed style in IIO for this.

> +		ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
> +		if (ret)
> +			return ret;
> +
> +		ret = ads1100_set_config_bits(data, ADS1100_DR_MASK,
> +					      FIELD_PREP(ADS1100_DR_MASK, i));
> +		if (ret)
> +			return ret;

> +		ret = ads1100_poll_data_ready(data);
> +
> +		return ret;

As per above.

>  	}
>  
>  	return -EINVAL;

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Tue, Jun 23, 2026 at 12:15:50AM +0200, Jakub Szczudlo wrote:
> Add ADS1110 support that have faster datarate than ADS1100, it also uses
> internal voltage reference of 2.048V for measurement.

...

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

User won't know what similar are really supported. The rule of thumb is to add
the list of supported here as

	  - ADS1000 (...perhaps some very short spec info...)
	  - ADS1100 (...perhaps some very short spec info...)


>  	  This driver can also be built as a module. If so, the module will be
>  	  called ti-ads1100.

...

> +static int ads1100_get_vref_milivolts(struct ads1100_data *data)
> +{
> +	if (data->ads_config->has_internal_vref_only)
> +		return ADS1110_INTERNAL_REF_mV;
> +
> +	return regulator_get_voltage(data->reg_vdd) / MILLI;

For now we used "(MICRO / MILLI)" instead of "MILLI", to show the unit
conversion.

> +}

...

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

> +	} else if (ret < 2) {

Redundant 'else'.

> +		dev_err(&data->client->dev, "Short I2C read\n");
> +		return -EIO;
>  	}

...

> -	microvolts = regulator_get_voltage(data->reg_vdd);
> +	microvolts = ads1100_get_vref_milivolts(data) * (MICRO / MILLI);

See above, here you correctly used the existing pattern, the above is
inconsistent and needs to be addressed.

...

> +	model = i2c_get_match_data(client);
> +	if (!model)
> +		return dev_err_probe(dev, -EINVAL,
> +				     "Can't get device data from firmware\n");
> +
> +	data->ads_config = (struct ads1100_config *)model;

You can't drop const like this. If you need to apply modification,
use devm_kmemdup(). Otherwise it won't work correctly if you have two different
sensors of the same driver in the system.

-- 
With Best Regards,
Andy Shevchenko

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

[Joshua Crofts]

On Tue, 23 Jun 2026 12:16:39 +0300
Andy Shevchenko <andriy.shevchenko@intel.com> wrote:

> > +	return read_poll_timeout(ads1100_new_data_not_ready, data_ready,
> > +				 !data_ready, wait_time,

I'd actually be all for using `data_ready != 0`, to make the condition more
readable.

...

> > +		PM_RUNTIME_ACQUIRE_AUTOSUSPEND(&data->client->dev, pm);  
> 
> > +  
> 
> This blank line is not needed as they are coupled, but I don't know if we have
> an agreed style in IIO for this.

I'd be surprised if there was an agreed style, as there aren't any IIO drivers
that use this specific macro (not in mainline at least). Additionally, might I
suggest using `PM_RUNTIME_ACQUIRE_IF_ENABLED_AUTOSUSPEND` as it is more generic?

-- 
Kind regards

CJD

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

[Andy Shevchenko]

On Tue, Jun 23, 2026 at 11:29:53AM +0200, Joshua Crofts wrote:
> On Tue, 23 Jun 2026 12:16:39 +0300
> Andy Shevchenko <andriy.shevchenko@intel.com> wrote:

...

> > > +	return read_poll_timeout(ads1100_new_data_not_ready, data_ready,
> > > +				 !data_ready, wait_time,
> 
> I'd actually be all for using `data_ready != 0`, to make the condition more
> readable.

I am okay with either. It might be slightly clearer if the comparison is done
for some dynamic counting or so, when 0 is not special.

...

> > > +		PM_RUNTIME_ACQUIRE_AUTOSUSPEND(&data->client->dev, pm);  
> > 
> > > +  
> > 
> > This blank line is not needed as they are coupled, but I don't know if we have
> > an agreed style in IIO for this.
> 
> I'd be surprised if there was an agreed style, as there aren't any IIO drivers
> that use this specific macro (not in mainline at least). Additionally, might I
> suggest using `PM_RUNTIME_ACQUIRE_IF_ENABLED_AUTOSUSPEND` as it is more generic?

We have other PM_ACQUIRE_*() macros in the drivers in IIO, so we have some style,
but I haven't checked what is that.

-- 
With Best Regards,
Andy Shevchenko