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

[sashiko-bot@kernel.org]

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