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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Incorrect polling condition in `readx_poll_timeout` causes premature loop termination and timeouts.
- [Medium] Negative error codes from `regulator_get_voltage` are propagated without checks, leaking to userspace as valid scale values.
- [Medium] The IIO device name is hardcoded to "ads1100", ignoring the newly introduced `name` field in `ads1100_config`.
- [Low] The Kconfig help text erroneously claims the device speeds are in 'MSPS' rather than 'SPS'.
- [Low] Typo in function name: `ads1100_get_vref_milivolts` instead of `millivolts`.
--

--- Patch [3]: [PATCH v6 3/3] iio: adc: Add ti-ads1110 support to ti-ads1100 driver ---
commit 28295190b9a9db57f2e01d15ded56ba73c22d69b
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/Kconfig b/drivers/iio/adc/Kconfig
> index 1c663c98c6c96..2459ff2af1050 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -1765,11 +1765,14 @@ 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 for TI single channel I2C Analog
> +	  Devices.
> +	  * ADS1000 12-Bit, 128 MSPS Analog-to-Digital Converter
> +	  * ADS1100 16-Bit, 128 MSPS Analog-to-Digital Converter
> +	  * ADS1110 16-Bit, 240 MSPS Analog-to-Digital Converter

[Severity: Low]
Does this code use the correct units for the datarate?

The ADS1100 and similar hardware typically operate at 8 to 240 SPS
(Samples Per Second). MSPS implies millions of samples per second,
which seems too high.

> diff --git a/drivers/iio/adc/ti-ads1100.c b/drivers/iio/adc/ti-ads1100.c
> index 6ad80d42d390a..06892ebc593d0 100644
> --- a/drivers/iio/adc/ti-ads1100.c
> +++ b/drivers/iio/adc/ti-ads1100.c
> @@ -90,6 +114,20 @@ 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)

[Severity: Low]
This isn't a bug, but there is a typo in the function name
ads1100_get_vref_milivolts (milivolts instead of millivolts).

> +{
> +	int voltage_uV;
> +
> +	if (data->ads_config->has_internal_vref_only)
> +		return ADS1110_INTERNAL_REF_mV;
> +
> +	voltage_uV = regulator_get_voltage(data->reg_vdd);
> +	if (voltage_uV < 0)
> +		return voltage_uV;

[Severity: Medium]
Does this leak negative error codes to callers?

If regulator_get_voltage() fails, it returns a negative error code
like -EINVAL. Callers like ads1100_read_raw() and ads1100_calc_scale_avail()
appear to use the return value from ads1100_get_vref_milivolts() without
checking for errors. This could expose negative scale values to userspace.

> +
> +	return voltage_uV / (MICRO / MILLI);
> +}
>  

> @@ -144,7 +182,8 @@ static bool ads1100_new_data_not_ready(struct ads1100_data *data)
>  
>  static int ads1100_poll_data_ready(struct ads1100_data *data)
>  {
> -	int data_rate_Hz = ads1100_data_rate[FIELD_GET(ADS1100_DR_MASK, data->config)];
> +	int data_rate_index = FIELD_GET(ADS1100_DR_MASK, data->config);
> +	int data_rate_Hz = data->ads_config->available_data_rate_hz[data_rate_index];
>  	/* To be sure we wait 5 times more than data rate */
>  	unsigned long wait_time_us = DIV_ROUND_CLOSEST(USEC_PER_SEC, 5 * data_rate_Hz);
>  	bool data_ready;

[Severity: High]
Does this function terminate polling prematurely?

When readx_poll_timeout() is called later in ads1100_poll_data_ready()
to wait for the conversion:

    return readx_poll_timeout(ads1100_new_data_not_ready, data,
                 data_ready, data_ready != 0,
                 wait_time_us, ADS1100_MAX_DRDY_TIMEOUT_US);

The termination condition used is data_ready != 0. However, the polled
function ads1100_new_data_not_ready() reads the busy bit and returns true
(non-zero) when the device is still busy, and false (zero) when the data
is ready.

Does this cause readx_poll_timeout() to immediately return success when
the device is busy, or loop until a timeout occurs if the device is ready?

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

[Severity: Medium]
Is the IIO device name hardcoded to ads1100 instead of using the
name from ads1100_config?

Earlier in ads1100_probe(), the code sets:

    indio_dev->name = "ads1100";

Since the newly introduced ads1100_config struct adds a name field
(e.g. ads1110), does hardcoding the name ignore the config and
potentially break userspace device matching for the ADS1110?

>  	ret = ads1100_setup(data);
>  	if (ret)
>  		return dev_err_probe(dev, ret,

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