* Re: [PATCH 3/3] iio: adc: Add ti-ads1110 support to ti-ads1100 driver
2026-06-07 18:35 ` [PATCH 3/3] iio: adc: Add ti-ads1110 support to ti-ads1100 driver Jakub Szczudlo
@ 2026-06-07 18:50 ` sashiko-bot
2026-06-08 7:39 ` Joshua Crofts
2026-06-08 18:23 ` Jonathan Cameron
2 siblings, 0 replies; 16+ messages in thread
From: sashiko-bot @ 2026-06-07 18:50 UTC (permalink / raw)
To: Jakub Szczudlo; +Cc: robh, conor+dt, devicetree
Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] I2C read errors are silently ignored due to implicit type conversion (signed `int` compared to `size_t`), causing the driver to process uninitialized stack data.
- [High] Partial I2C reads cause a positive return value to bypass caller error checks, leaking uninitialized kernel stack memory to userspace.
- [High] The `vdd` regulator is leaked during device unbind due to reversed devres execution order.
- [High] The data polling logic instantly terminates in continuous mode, failing to wait for readings and silently swallowing I2C communication failures.
- [High] Probe returns success (0) instead of an error code when device match data is missing, leading to an uninitialized device state and a subsequent NULL pointer dereference.
- [Medium] I2C write errors are ignored when changing the data rate or scale, silently leaving the software state out of sync with hardware.
--
commit 08fffdde41976a365855c8545a761a652c4906e0
Author: jszczudlo <jakubszczudlo40@gmail.com>
iio: adc: Add ti-ads1110 support to ti-ads1100 driver
add ADS1100 support
make changing gain and datarate wait for new reading
fix unbalanced regulator disable when removing in singleshot mode
> diff --git a/drivers/iio/adc/ti-ads1100.c b/drivers/iio/adc/ti-ads1100.c
> index aa8946063c7d6..11d6fe1e8abc2 100644
> --- a/drivers/iio/adc/ti-ads1100.c
> +++ b/drivers/iio/adc/ti-ads1100.c
[ ... ]
> @@ -85,6 +98,50 @@ static int ads1100_set_config_bits(struct ads1100_data *data, u8 mask, u8 value)
[ ... ]
> +static bool ads11x0_new_data_ready(struct ads1100_data *data)
> +{
> + int ret;
> + u8 buffer[3];
> +
> + ret = i2c_master_recv(data->client, (char *)&buffer, sizeof(buffer));
> + if (ret < sizeof(buffer)) {
[Severity: High]
Does this implicit type conversion mask negative error codes?
`sizeof(buffer)` evaluates to a `size_t`. When `ret` is compared to
`sizeof(buffer)`, it gets promoted to an unsigned value.
If `i2c_master_recv()` returns a negative error code like `-EIO`, it becomes
a massive positive number, bypassing the error check and allowing the driver
to process uninitialized stack data.
This same signedness comparison issue appears in `ads1100_get_adc_result()`
and `ads1100_setup()`.
> + dev_err(&data->client->dev, "I2C read fail: %d\n", ret);
> + return 0;
> + }
> +
> + int return_val = FIELD_GET(ADS1100_CFG_ST_BSY, buffer[2]);
> +
> + return return_val;
[Severity: High]
Will this polling logic ever actually wait?
According to the datasheet, the ST/BSY bit is always read as 0 in continuous
conversion mode. If `ads11x0_new_data_ready()` always extracts a 0,
`data_ready` will be 0.
When `read_poll_timeout()` checks `!data_ready`, it evaluates to true
immediately on the first iteration, causing the loop to exit without waiting.
> +}
[ ... ]
> @@ -105,9 +162,10 @@ static int ads1100_get_adc_result(struct ads1100_data *data, int chan, int *val)
[ ... ]
> ret = i2c_master_recv(data->client, (char *)&buffer, sizeof(buffer));
>
> + pm_runtime_mark_last_busy(&data->client->dev);
> pm_runtime_put_autosuspend(&data->client->dev);
>
> - if (ret < 0) {
> + if (ret < sizeof(buffer)) {
> dev_err(&data->client->dev, "I2C read fail: %d\n", ret);
> return ret;
[Severity: High]
If a partial read occurs, could this leak uninitialized stack memory to
userspace?
If `i2c_master_recv()` returns a positive short read length (like 1), this
error path correctly catches it and returns `ret` (which is 1).
However, the caller `ads1100_read_raw()` expects negative error codes:
drivers/iio/adc/ti-ads1100.c:ads1100_read_raw() {
...
ret = ads1100_get_adc_result(data, chan->address, val);
iio_device_release_direct(indio_dev);
if (ret < 0)
return ret;
return IIO_VAL_INT;
}
Because `ret` is positive, `ads1100_read_raw()` will bypass its error check
and return `IIO_VAL_INT` (success) while `*val` remains uninitialized.
> }
[ ... ]
> @@ -127,7 +185,7 @@ 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 = ads11x0_poll_data_ready(data);
[Severity: Medium]
Is it intentional to discard the return value of `ads1100_set_config_bits()`
here?
If the I2C write fails, the hardware and software state will be out of sync,
but the driver will proceed to poll and falsely report successful
configuration changes. The same issue appears in `ads1100_set_data_rate()`.
> +
> + pm_runtime_mark_last_busy(&data->client->dev);
> + pm_runtime_put_autosuspend(&data->client->dev);
> +
> + return ret;
> }
[ ... ]
> @@ -292,9 +364,13 @@ static int ads1100_setup(struct ads1100_data *data)
> return 0;
> }
>
> -static void ads1100_reg_disable(void *reg)
> +static void ads1100_reg_disable(void *data)
> {
> - regulator_disable(reg);
> + struct ads1100_data *ads1100_data = data;
> +
> + /* Disable when not already disabled by the driver */
> + if (!(ads1100_data->config & ADS1100_CFG_SC))
> + regulator_disable(ads1100_data->reg_vdd);
> }
[Severity: High]
Could this leak the `vdd` regulator during device unbind due to the devres
execution order?
Devres executes registered actions in reverse order (LIFO). During unbind,
`ads1100_disable_continuous()` executes first and unconditionally sets
`ADS1100_CFG_SC`.
When `ads1100_reg_disable()` runs immediately after, the `ADS1100_CFG_SC` bit
is already set, causing this condition to fail and permanently skipping the
`regulator_disable()` call.
>
> static void ads1100_disable_continuous(void *data)
[ ... ]
> @@ -334,10 +411,18 @@ static int ads1100_probe(struct i2c_client *client)
[ ... ]
> + model = device_get_match_data(dev);
> + if (!model)
> + return dev_err_probe(dev, ret,
> + "Can't set device data\n");
[Severity: High]
Does this return a success code when `device_get_match_data()` fails?
The preceding call to `devm_add_action_or_reset()` succeeds, leaving `ret`
as exactly 0.
Passing a 0 `ret` to `dev_err_probe()` will cause the probe function to
return 0 (success) instead of a valid error code (like `-ENODEV`). The
device registers successfully with uninitialized device data, leading to a
NULL pointer dereference when userspace tries to read from the device.
> +
> + data->data_rate = model->data_rate;
> + data->has_reference_voltage = model->has_reference_voltage;
> +
> ret = ads1100_setup(data);
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260607183542.368184-1-jakubszczudlo40@gmail.com?part=3
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH 3/3] iio: adc: Add ti-ads1110 support to ti-ads1100 driver
2026-06-07 18:35 ` [PATCH 3/3] iio: adc: Add ti-ads1110 support to ti-ads1100 driver Jakub Szczudlo
2026-06-07 18:50 ` sashiko-bot
@ 2026-06-08 7:39 ` Joshua Crofts
2026-06-08 18:03 ` Jonathan Cameron
2026-06-08 18:23 ` Jonathan Cameron
2 siblings, 1 reply; 16+ messages in thread
From: Joshua Crofts @ 2026-06-08 7:39 UTC (permalink / raw)
To: Jakub Szczudlo
Cc: linux-iio, jic23, dlechner, nuno.sa, andy, marcelo.schmitt, robh,
krzk+dt, conor+dt, mike.looijmans, devicetree, linux-kernel,
jorge.marques, antoniu.miclaus, mazziesaccount, jishnu.prakash,
duje, wens, sakari.ailus, linusw
Hi Jakub,
various comments inline, some nitpicks and some more serious.
Josh
On Sun, 7 Jun 2026 at 20:38, Jakub Szczudlo <jakubszczudlo40@gmail.com> wrote:
>
> From: jszczudlo <jakubszczudlo40@gmail.com>
>
> add ADS1100 support
Wrap the commit message to 72 characters per line, this is too
short.
> make changing gain and datarate wait for new reading
> fix unbalanced regulator disable when removing in singleshot mode
Additionally, write the commit messages as regular sentences, not
a list of changes.
>
> Signed-off-by: jszczudlo <jakubszczudlo40@gmail.com>
Ensure that your full name is in the Signed-off-by tag (this goes for all
patches in this series).
> ---
> drivers/iio/adc/ti-ads1100.c | 165 ++++++++++++++++++++++++++---------
> 1 file changed, 126 insertions(+), 39 deletions(-)
>
> diff --git a/drivers/iio/adc/ti-ads1100.c b/drivers/iio/adc/ti-ads1100.c
> index aa8946063c7d..11d6fe1e8abc 100644
> --- a/drivers/iio/adc/ti-ads1100.c
> +++ b/drivers/iio/adc/ti-ads1100.c
> @@ -5,18 +5,15 @@
> * 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, ADS1000 and ADS1110 ADC 16-bit I2C
> */
>
> #include <linux/bitfield.h>
> #include <linux/bits.h>
> -#include <linux/cleanup.h>
> -#include <linux/delay.h>
> -#include <linux/module.h>
> -#include <linux/init.h>
> #include <linux/i2c.h>
> +#include <linux/iopoll.h>
> +#include <linux/module.h>
> #include <linux/mutex.h>
> -#include <linux/property.h>
> #include <linux/pm_runtime.h>
> #include <linux/regulator/consumer.h>
> #include <linux/units.h>
This should go as a separate patch (or part of a cleanup series). Not only
it's undocumented in the commit message, it's also unrelated to adding
support for a new device.
> @@ -39,17 +36,39 @@
> #define ADS1100_SINGLESHOT ADS1100_CFG_SC
>
> #define ADS1100_SLEEP_DELAY_MS 2000
> +#define ADS1110_REFERENCE_VOLTAGE_MICROVOLT 2048000
> +
> +/* Timeout based on the minimum sample rate of 8 SPS (7500000us) */
> +#define ADS11x0_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 ads11x0_config {
> + const int *data_rate;
> + bool has_reference_voltage;
> +};
> +
> +static const struct ads11x0_config ads1100_config = {
> + .data_rate = ads1100_data_rate,
> + .has_reference_voltage = false,
> +};
> +
> +static const struct ads11x0_config ads1110_config = {
> + .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 */
> 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 */
> + bool has_reference_voltage; /* The ADS1110 has an internal reference, so fixed scale */
> + const int *data_rate;
> };
Have you used pahole to ensure this struct layout is ideal? I'd
move the data_rate pointer up to the larger types.
> static const struct iio_chan_spec ads1100_channel = {
> @@ -59,12 +78,6 @@ static const struct iio_chan_spec ads1100_channel = {
> BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_SAMP_FREQ),
> .info_mask_shared_by_all_available =
> BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_SAMP_FREQ),
> - .scan_type = {
> - .sign = 's',
> - .realbits = 16,
> - .storagebits = 16,
> - .endianness = IIO_CPU,
> - },
> .datasheet_name = "AIN",
> };
>
> @@ -85,6 +98,50 @@ static int ads1100_set_config_bits(struct ads1100_data *data, u8 mask, u8 value)
> return 0;
> };
>
> +static int ads11x0_get_voltage_microvolts(struct ads1100_data *data)
> +{
> + if (data->has_reference_voltage)
> + return ADS1110_REFERENCE_VOLTAGE_MICROVOLT;
> + else
> + return regulator_get_voltage(data->reg_vdd);
> +}
> +
> +static int ads11x0_get_voltage_milivolts(struct ads1100_data *data)
> +{
> + return ads11x0_get_voltage_microvolts(data) / (MICRO / MILLI);
> +}
> +
> +static bool ads11x0_new_data_ready(struct ads1100_data *data)
> +{
> + int ret;
> + u8 buffer[3];
> +
> + ret = i2c_master_recv(data->client, (char *)&buffer, sizeof(buffer));
> + if (ret < sizeof(buffer)) {
Sashiko raises an issue here. sizeof returns a size_t, therefore the compiler
will promote ret to a size_t, wrapping any potential negative error value to
a large positive value, throwing away the error.
> + dev_err(&data->client->dev, "I2C read fail: %d\n", ret);
> + return 0;
> + }
> +
> + int return_val = FIELD_GET(ADS1100_CFG_ST_BSY, buffer[2]);
> +
> + return return_val;
> +}
> +
> +static int ads11x0_poll_data_ready(struct ads1100_data *data)
> +{
> + bool data_ready;
> + u8 buffer[3];
> + int datarate = data->data_rate[FIELD_GET(ADS1100_DR_MASK, data->config)];
> + unsigned long wait_time = DIV_ROUND_CLOSEST(MICRO, 5 * datarate);
Reverse Christmas tree order please. This goes for all new functions
and variable
declarations.
> +
> + /* To be sure that polled value will have value after config change */
> + i2c_master_recv(data->client, (char *)&buffer, sizeof(buffer));
Add a check here as well maybe.
> + return read_poll_timeout(ads11x0_new_data_ready, data_ready,
> + !data_ready, wait_time,
> + ADS11x0_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)];
> @@ -105,9 +162,10 @@ static int ads1100_get_adc_result(struct ads1100_data *data, int chan, int *val)
>
> ret = i2c_master_recv(data->client, (char *)&buffer, sizeof(buffer));
>
> + pm_runtime_mark_last_busy(&data->client->dev);
AFAIK pm_runtime_put_autosuspend automatically calls mark_last_busy
when executed, so this is probably unnecessary. Additionally, this has nothing
to do with adding new device support.
> pm_runtime_put_autosuspend(&data->client->dev);
>
> - if (ret < 0) {
> + if (ret < sizeof(buffer)) {
Same issue with type promotion here.
> dev_err(&data->client->dev, "I2C read fail: %d\n", ret);
> return ret;
> }
> @@ -127,7 +185,7 @@ static int ads1100_set_scale(struct ads1100_data *data, int val, int val2)
> {
> int microvolts;
> int gain;
> -
> + int ret;
Newline here.
> /* With Vdd between 2.7 and 5V, the scale is always below 1 */
> if (val)
> return -EINVAL;
> @@ -135,7 +193,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 = ads11x0_get_voltage_microvolts(data);
> /*
> * val2 is in 'micro' units, n = val2 / 1000000
> * result must be millivolts, d = microvolts / 1000
> @@ -147,34 +205,49 @@ 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 = ads11x0_poll_data_ready(data);
> +
> + pm_runtime_mark_last_busy(&data->client->dev);
Same issue with mark_last_busy being called automatically.
> + 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 (ads1100_data_rate[i] == rate)
> - return ads1100_set_config_bits(data, ADS1100_DR_MASK,
> - FIELD_PREP(ADS1100_DR_MASK, i));
> + if (data->data_rate[i] == rate) {
> + 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 = ads11x0_poll_data_ready(data);
> +
> + pm_runtime_mark_last_busy(&data->client->dev);
> + pm_runtime_put_autosuspend(&data->client->dev);
> + return ret;
> + }
> }
>
> 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 = ads11x0_get_voltage_milivolts(data);
> unsigned int i;
>
> for (i = 0; i < ARRAY_SIZE(data->scale_avail) / 2; i++) {
> @@ -196,7 +269,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->data_rate;
> if (data->supports_data_rate)
> *length = ARRAY_SIZE(ads1100_data_rate);
> else
> @@ -233,12 +306,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 = ads11x0_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->data_rate[FIELD_GET(ADS1100_DR_MASK, data->config)];
> return IIO_VAL_INT;
> default:
> return -EINVAL;
> @@ -280,8 +352,8 @@ static int ads1100_setup(struct ads1100_data *data)
> return ret;
>
> ret = i2c_master_recv(data->client, buffer, sizeof(buffer));
> - if (ret < 0)
> - return ret;
> + if (ret < sizeof(buffer))
> + return -1;
Same type promotion issue here. Additionally, I'd rather return -EIO than -1.
>
> /* Config register returned in third byte, strip away the busy status */
> data->config = buffer[2] & ~ADS1100_CFG_ST_BSY;
> @@ -292,9 +364,13 @@ static int ads1100_setup(struct ads1100_data *data)
> return 0;
> }
>
> -static void ads1100_reg_disable(void *reg)
> +static void ads1100_reg_disable(void *data)
> {
> - regulator_disable(reg);
> + struct ads1100_data *ads1100_data = data;
> +
> + /* Disable when not already disabled by the driver */
> + if (!(ads1100_data->config & ADS1100_CFG_SC))
> + regulator_disable(ads1100_data->reg_vdd);
Sashiko has something to say:
Could this leak the `vdd` regulator during device unbind due to the devres
execution order?
Devres executes registered actions in reverse order (LIFO). During unbind,
`ads1100_disable_continuous()` executes first and unconditionally sets
`ADS1100_CFG_SC`.
When `ads1100_reg_disable()` runs immediately after, the `ADS1100_CFG_SC` bit
is already set, causing this condition to fail and permanently skipping the
`regulator_disable()` call.
> }
>
> static void ads1100_disable_continuous(void *data)
> @@ -307,6 +383,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 ads11x0_config *model;
> int ret;
>
> indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> @@ -334,10 +411,18 @@ static int ads1100_probe(struct i2c_client *client)
> return dev_err_probe(dev, ret,
> "Failed to enable vdd regulator\n");
>
> - ret = devm_add_action_or_reset(dev, ads1100_reg_disable, data->reg_vdd);
> + ret = devm_add_action_or_reset(dev, ads1100_reg_disable, data);
> if (ret)
> return ret;
>
> + model = device_get_match_data(dev);
> + if (!model)
> + return dev_err_probe(dev, ret,
> + "Can't set device data\n");
Hmm, if device_get_match_data fails, ret will still be 0 per previous
devm_add_action_or_reset() call, therefore you're returning a "successful
error". Additionally, the error message isn't aligned with the parenthesis.
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH 3/3] iio: adc: Add ti-ads1110 support to ti-ads1100 driver
2026-06-08 7:39 ` Joshua Crofts
@ 2026-06-08 18:03 ` Jonathan Cameron
0 siblings, 0 replies; 16+ messages in thread
From: Jonathan Cameron @ 2026-06-08 18:03 UTC (permalink / raw)
To: Joshua Crofts
Cc: Jakub Szczudlo, linux-iio, dlechner, nuno.sa, andy,
marcelo.schmitt, robh, krzk+dt, conor+dt, mike.looijmans,
devicetree, linux-kernel, jorge.marques, antoniu.miclaus,
mazziesaccount, jishnu.prakash, duje, wens, sakari.ailus, linusw
On Mon, 8 Jun 2026 09:39:07 +0200
Joshua Crofts <joshua.crofts1@gmail.com> wrote:
> Hi Jakub,
>
> various comments inline, some nitpicks and some more serious.
>
> Josh
>
> On Sun, 7 Jun 2026 at 20:38, Jakub Szczudlo <jakubszczudlo40@gmail.com> wrote:
> >
> > From: jszczudlo <jakubszczudlo40@gmail.com>
> >
> > add ADS1100 support
>
> Wrap the commit message to 72 characters per line, this is too
> short.
>
> > make changing gain and datarate wait for new reading
> > fix unbalanced regulator disable when removing in singleshot mode
>
> Additionally, write the commit messages as regular sentences, not
> a list of changes.
>
> >
> > Signed-off-by: jszczudlo <jakubszczudlo40@gmail.com>
>
> Ensure that your full name is in the Signed-off-by tag (this goes for all
> patches in this series).
A few follow ups to the good review you already have from Joshua.
> > @@ -85,6 +98,50 @@ static int ads1100_set_config_bits(struct ads1100_data *data, u8 mask, u8 value)
> > return 0;
> > };
> >
> > +static int ads11x0_get_voltage_microvolts(struct ads1100_data *data)
> > +{
> > + if (data->has_reference_voltage)
> > + return ADS1110_REFERENCE_VOLTAGE_MICROVOLT;
> > + else
> > + return regulator_get_voltage(data->reg_vdd);
> > +}
> > +
> > +static int ads11x0_get_voltage_milivolts(struct ads1100_data *data)
> > +{
> > + return ads11x0_get_voltage_microvolts(data) / (MICRO / MILLI);
> > +}
> > +
> > +static bool ads11x0_new_data_ready(struct ads1100_data *data)
> > +{
> > + int ret;
> > + u8 buffer[3];
> > +
> > + ret = i2c_master_recv(data->client, (char *)&buffer, sizeof(buffer));
> > + if (ret < sizeof(buffer)) {
>
> Sashiko raises an issue here. sizeof returns a size_t, therefore the compiler
> will promote ret to a size_t, wrapping any potential negative error value to
> a large positive value, throwing away the error.
>
> > + dev_err(&data->client->dev, "I2C read fail: %d\n", ret);
> > + return 0;
It gets thrown away here anyway which is also very wrong! If an error occurs
it should be propagated. If it makes sense to ignore it, do that at the
caller and add a comment on why.
> > + }
> > +
> > + int return_val = FIELD_GET(ADS1100_CFG_ST_BSY, buffer[2]);
When there isn't a good reason to do otherwise we still flow oldstyle c
where local variable declarations come at the top of scope.
However
return FIELD_GET();
should be fine.
> > +
> > + return return_val;
> > +}
> > static void ads1100_disable_continuous(void *data)
> > @@ -307,6 +383,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 ads11x0_config *model;
> > int ret;
> >
> > indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> > @@ -334,10 +411,18 @@ static int ads1100_probe(struct i2c_client *client)
> > return dev_err_probe(dev, ret,
> > "Failed to enable vdd regulator\n");
> >
> > - ret = devm_add_action_or_reset(dev, ads1100_reg_disable, data->reg_vdd);
> > + ret = devm_add_action_or_reset(dev, ads1100_reg_disable, data);
> > if (ret)
> > return ret;
> >
> > + model = device_get_match_data(dev);
> > + if (!model)
> > + return dev_err_probe(dev, ret,
> > + "Can't set device data\n");
>
> Hmm, if device_get_match_data fails, ret will still be 0 per previous
> devm_add_action_or_reset() call, therefore you're returning a "successful
> error". Additionally, the error message isn't aligned with the parenthesis.
Message also talks about 'setting' when it is 'getting' data from firmware.
So needs a rewrite.
Jonathan
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 3/3] iio: adc: Add ti-ads1110 support to ti-ads1100 driver
2026-06-07 18:35 ` [PATCH 3/3] iio: adc: Add ti-ads1110 support to ti-ads1100 driver Jakub Szczudlo
2026-06-07 18:50 ` sashiko-bot
2026-06-08 7:39 ` Joshua Crofts
@ 2026-06-08 18:23 ` Jonathan Cameron
2 siblings, 0 replies; 16+ messages in thread
From: Jonathan Cameron @ 2026-06-08 18:23 UTC (permalink / raw)
To: Jakub Szczudlo
Cc: linux-iio, dlechner, nuno.sa, andy, marcelo.schmitt, robh,
krzk+dt, conor+dt, mike.looijmans, devicetree, linux-kernel,
jorge.marques, antoniu.miclaus, mazziesaccount, jishnu.prakash,
duje, wens, sakari.ailus, linusw
On Sun, 7 Jun 2026 20:35:42 +0200
Jakub Szczudlo <jakubszczudlo40@gmail.com> wrote:
> From: jszczudlo <jakubszczudlo40@gmail.com>
>
> add ADS1100 support
> make changing gain and datarate wait for new reading
> fix unbalanced regulator disable when removing in singleshot mode
>
> Signed-off-by: jszczudlo <jakubszczudlo40@gmail.com>
Hi Jakub,
A few additional things for me.
Note that the IIO tree is closed for this cycle and even if it wasn't
don't rush on sending a new version (at least a few days for a patch like
this between versions). That will give more time for other reviewers
to spot things Joshua and I missed!
Thanks,
Jonathan
> ---
> drivers/iio/adc/ti-ads1100.c | 165 ++++++++++++++++++++++++++---------
> 1 file changed, 126 insertions(+), 39 deletions(-)
>
> diff --git a/drivers/iio/adc/ti-ads1100.c b/drivers/iio/adc/ti-ads1100.c
> index aa8946063c7d..11d6fe1e8abc 100644
> --- a/drivers/iio/adc/ti-ads1100.c
> +++ b/drivers/iio/adc/ti-ads1100.c
> @@ -5,18 +5,15 @@
> * 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, ADS1000 and ADS1110 ADC 16-bit I2C
> */
>
> #include <linux/bitfield.h>
> #include <linux/bits.h>
> -#include <linux/cleanup.h>
> -#include <linux/delay.h>
> -#include <linux/module.h>
> -#include <linux/init.h>
> #include <linux/i2c.h>
> +#include <linux/iopoll.h>
> +#include <linux/module.h>
> #include <linux/mutex.h>
> -#include <linux/property.h>
> #include <linux/pm_runtime.h>
> #include <linux/regulator/consumer.h>
> #include <linux/units.h>
> @@ -39,17 +36,39 @@
> #define ADS1100_SINGLESHOT ADS1100_CFG_SC
>
> #define ADS1100_SLEEP_DELAY_MS 2000
> +#define ADS1110_REFERENCE_VOLTAGE_MICROVOLT 2048000
Why not express this in milivolts and move the divisor for the
regulator into the place it is read?
> +
> +/* Timeout based on the minimum sample rate of 8 SPS (7500000us) */
> +#define ADS11x0_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 ads11x0_config {
> + const int *data_rate;
> + bool has_reference_voltage;
const char *name;
(see below)
> +};
> +
> +static const struct ads11x0_config ads1100_config = {
> + .data_rate = ads1100_data_rate,
> + .has_reference_voltage = false,
> +};
> +
> +static const struct ads11x0_config ads1110_config = {
> + .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 */
> 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 */
> + bool has_reference_voltage; /* The ADS1110 has an internal reference, so fixed scale */
As suggested below
struct ads1100_config *config;
rather than fields to copy.
> + const int *data_rate;
> };
>
> static const struct iio_chan_spec ads1100_channel = {
> @@ -59,12 +78,6 @@ static const struct iio_chan_spec ads1100_channel = {
> BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_SAMP_FREQ),
> .info_mask_shared_by_all_available =
> BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_SAMP_FREQ),
> - .scan_type = {
> - .sign = 's',
> - .realbits = 16,
> - .storagebits = 16,
> - .endianness = IIO_CPU,
> - },
Unrelated change so separate patch.
> .datasheet_name = "AIN",
> };
>
> @@ -85,6 +98,50 @@ static int ads1100_set_config_bits(struct ads1100_data *data, u8 mask, u8 value)
> return 0;
> };
>
> +static int ads11x0_get_voltage_microvolts(struct ads1100_data *data)
> +{
> + if (data->has_reference_voltage)
> + return ADS1110_REFERENCE_VOLTAGE_MICROVOLT;
> + else
> + return regulator_get_voltage(data->reg_vdd);
> +}
> +
> +static int ads11x0_get_voltage_milivolts(struct ads1100_data *data)
> +{
> + return ads11x0_get_voltage_microvolts(data) / (MICRO / MILLI);
> +}
> +
> +static bool ads11x0_new_data_ready(struct ads1100_data *data)
> +{
> + int ret;
> + u8 buffer[3];
> +
> + ret = i2c_master_recv(data->client, (char *)&buffer, sizeof(buffer));
> + if (ret < sizeof(buffer)) {
> + dev_err(&data->client->dev, "I2C read fail: %d\n", ret);
> + return 0;
> + }
> +
> + int return_val = FIELD_GET(ADS1100_CFG_ST_BSY, buffer[2]);
> +
> + return return_val;
Commented on this in Joshua's thread.
> +}
> +
> +static int ads11x0_poll_data_ready(struct ads1100_data *data)
> +{
> + bool data_ready;
> + u8 buffer[3];
> + int datarate = data->data_rate[FIELD_GET(ADS1100_DR_MASK, data->config)];
> + unsigned long wait_time = DIV_ROUND_CLOSEST(MICRO, 5 * datarate);
Why that number?
> +
> + /* To be sure that polled value will have value after config change */
> + i2c_master_recv(data->client, (char *)&buffer, sizeof(buffer));
Why no return value check? Even if it is just to ensure new data, nice
to know if the bus is falling over.
> +
> + return read_poll_timeout(ads11x0_new_data_ready, data_ready,
> + !data_ready, wait_time,
> + ADS11x0_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)];
> @@ -105,9 +162,10 @@ static int ads1100_get_adc_result(struct ads1100_data *data, int chan, int *val)
>
> ret = i2c_master_recv(data->client, (char *)&buffer, sizeof(buffer));
>
> + pm_runtime_mark_last_busy(&data->client->dev);
See below. This is probably an issue with forward porting from an old kernel.
Make sure to check for things like this in upstream as you should have
wondered why something so obvious was missing! If it were missing this
would have been a fix (it's not missing).
> pm_runtime_put_autosuspend(&data->client->dev);
>
> - if (ret < 0) {
> + if (ret < sizeof(buffer)) {
> dev_err(&data->client->dev, "I2C read fail: %d\n", ret);
> return ret;
> }
> @@ -127,7 +185,7 @@ static int ads1100_set_scale(struct ads1100_data *data, int val, int val2)
> {
> int microvolts;
> int gain;
> -
> + int ret;
Blank line here.
> /* With Vdd between 2.7 and 5V, the scale is always below 1 */
> if (val)
> return -EINVAL;
> @@ -135,7 +193,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 = ads11x0_get_voltage_microvolts(data);
> /*
> * val2 is in 'micro' units, n = val2 / 1000000
> * result must be millivolts, d = microvolts / 1000
> @@ -147,34 +205,49 @@ 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 = ads11x0_poll_data_ready(data);
As below - this applies to existing parts so if it makes sense, separate
patch before you add the new device support. That can have a description
that tells us why this is needed.
> +
> + pm_runtime_mark_last_busy(&data->client->dev);
> + 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 (ads1100_data_rate[i] == rate)
> - return ads1100_set_config_bits(data, ADS1100_DR_MASK,
> - FIELD_PREP(ADS1100_DR_MASK, i));
> + if (data->data_rate[i] == rate) {
Flip logic.
if (data->data_rate[i] != rate)
continue;
ret = ...
reduces indent and generally simplifies things.
> + 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 = ads11x0_poll_data_ready(data);
Why is the more complex handling needed for existing devices?
If it is then smells like it should be a precursor patch with that
well explained.
> +
> + pm_runtime_mark_last_busy(&data->client->dev);
> + pm_runtime_put_autosuspend(&data->client->dev);
Joshua called this out already. I guess you are forward porting form an old
kernel as for a while the mark_last_busy() has been called by
put_autosuspend().
> + return ret;
> + }
> }
>
> 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 = ads11x0_get_voltage_milivolts(data);
As below. No to the wild card. If you want to rename that's fine
but almost certainly wants to be a separate patch with a description
of why the rename makes sense.
> unsigned int i;
>
> for (i = 0; i < ARRAY_SIZE(data->scale_avail) / 2; i++) {
> @@ -196,7 +269,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->data_rate;
> if (data->supports_data_rate)
> *length = ARRAY_SIZE(ads1100_data_rate);
> else
> @@ -233,12 +306,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 = ads11x0_get_voltage_milivolts(data);
Nope. We don't use wild card naming in IIO drivers. It goes wrong
too often as manufacturers have very inconsistent naming schemes.
+ The wild card is already wrong for the ads1000.
Sticking to existing prefix that matches the driver name is the
way to go.
> *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->data_rate[FIELD_GET(ADS1100_DR_MASK, data->config)];
Unrelated. Check your patches for stuff like this. It is noise
that makes for lower quality reviews.
> return IIO_VAL_INT;
> default:
> return -EINVAL;
> @@ -280,8 +352,8 @@ static int ads1100_setup(struct ads1100_data *data)
> return ret;
>
> ret = i2c_master_recv(data->client, buffer, sizeof(buffer));
> - if (ret < 0)
> - return ret;
> + if (ret < sizeof(buffer))
> + return -1;
Proper error returns and first check for ret < 0 as if it is you should return
that rather than eating the more informative error. This check is then only
on postive but too small values - so the rare case of a short recieve.
>
> /* Config register returned in third byte, strip away the busy status */
> data->config = buffer[2] & ~ADS1100_CFG_ST_BSY;
> @@ -292,9 +364,13 @@ static int ads1100_setup(struct ads1100_data *data)
> return 0;
> }
>
> -static void ads1100_reg_disable(void *reg)
> +static void ads1100_reg_disable(void *data)
> {
> - regulator_disable(reg);
> + struct ads1100_data *ads1100_data = data;
> +
> + /* Disable when not already disabled by the driver */
> + if (!(ads1100_data->config & ADS1100_CFG_SC))
Not obvious why regulator power is correlated with that register(?)
bit? I'd imagine this is where issue Joshua called out that sashiko
reported is coming from.
> + regulator_disable(ads1100_data->reg_vdd);
> }
>
> static void ads1100_disable_continuous(void *data)
> @@ -307,6 +383,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 ads11x0_config *model;
> int ret;
>
> indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> @@ -334,10 +411,18 @@ static int ads1100_probe(struct i2c_client *client)
> return dev_err_probe(dev, ret,
> "Failed to enable vdd regulator\n");
>
> - ret = devm_add_action_or_reset(dev, ads1100_reg_disable, data->reg_vdd);
> + ret = devm_add_action_or_reset(dev, ads1100_reg_disable, data);
Unrelated change.
> if (ret)
> return ret;
>
> + model = device_get_match_data(dev);
> + if (!model)
> + return dev_err_probe(dev, ret,
> + "Can't set device data\n");
Discussion of this in Joshua's thread.
> +
The name should where possible reflect the actual device. Easy way to do
this is normally to put a string in the model data and use that for
iio_dev->name. There is a slightly ABI quirk that the driver doesn't do
this for the existing pair of support parts. We should probably leave
that alone, but we can at least do better for this one.
> + data->data_rate = model->data_rate;
> + data->has_reference_voltage = model->has_reference_voltage;
Probably just stash a pointer to model rather that copying elements.
The element copying route tends not to scale as a driver gets more complex.
> +
> ret = ads1100_setup(data);
> if (ret)
> return dev_err_probe(dev, ret,
> @@ -400,16 +485,18 @@ static DEFINE_RUNTIME_DEV_PM_OPS(ads1100_pm_ops,
> NULL);
>
> static const struct i2c_device_id ads1100_id[] = {
> - { "ads1100" },
> - { "ads1000" },
> + { .name = "ads1100", .driver_data = (kernel_ulong_t)&ads1100_config },
> + { .name = "ads1000", .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,ads1100", .data = &ads1100_config },
> + { .compatible = "ti,ads1000", .data = &ads1100_config },
> + { .compatible = "ti,ads1110", .data = &ads1110_config },
As with binding, fix the ordering to be alphanumeric whilst we are here
> { }
> };
>
^ permalink raw reply [flat|nested] 16+ messages in thread