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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Inverted hardware polling logic causes unconditional 7.5-second hang and timeout in continuous mode.
- [Medium] Missing error handling for regulator_get_voltage failure exposes raw negative error codes to userspace.
- [Low] Kconfig help text describes the ADC data rates in MSPS (Mega Samples Per Second) instead of SPS.
--

--- Patch [3]: [PATCH v7 3/3] iio: adc: ti-ads1100: Add ti-ads1110 support to ti-ads1100 driver ---
commit 48edf6dcf5a3e58bfda7f4b74908d7339f253938
Author: Jakub Szczudlo <jakubszczudlo40@gmail.com>

iio: adc: ti-ads1100: 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]
This isn't a bug, but are these units correct? The ADS1000/1100/1110 data
rates are typically listed in SPS (Samples Per Second), not MSPS (Mega
Samples Per Second).

> diff --git a/drivers/iio/adc/ti-ads1100.c b/drivers/iio/adc/ti-ads1100.c
> index f0a30ae139afa..ad9099aaf19c9 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)
> +{
> +	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]
Will this correctly handle a regulator failure?

If regulator_get_voltage() returns a negative error code (e.g. -EINVAL),
this function returns the raw error code.

Callers like ads1100_calc_scale_avail() and ads1100_read_raw() seem to
directly assign this value to userspace arrays or IIO physical scale
variables without checking for errors. Can this result in exposing raw
negative error codes as IIO scale values?

[ ... ]

> @@ -144,7 +182,8 @@ static int ads1100_new_data_is_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);
>  	int data_ready;

[Severity: High]
Does this polling logic hang the driver in continuous mode?

Looking at the rest of ads1100_poll_data_ready(), it calls:

ret = readx_poll_timeout(ads1100_new_data_is_ready, data,
                         data_ready, data_ready != 0,
                         wait_time_us, ADS1100_MAX_DRDY_TIMEOUT_US);

The break condition is data_ready != 0, but ads1100_new_data_is_ready()
returns 1 when the ST/BSY bit is set. This means the loop will incorrectly
break when the device is BUSY, rather than when it is ready.

Additionally, in continuous mode the ST/BSY bit is always 0, which means
data_ready != 0 is always false, causing an unconditional 7.5-second timeout
every time the data rate or scale is changed.

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