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

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Missing error check for `ads1100_get_vref_millivolts()` return value propagates negative error codes to userspace.
- [Medium] Missing state restore on error paths in `ads1100_poll_data_ready` leaves the ADC permanently in single-shot mode.
--

--- Patch [3]: [PATCH v9 3/3] iio: adc: ti-ads1100: Add ti-ads1110 support to  ti-ads1100 driver ---
commit 1ff426338598760f136ce661b966962981800132
Author: Jakub Szczudlo <[email protected]>

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/ti-ads1100.c b/drivers/iio/adc/ti-ads1100.c
> index 288d209ecf92d..acbf9b96c545d 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_millivolts(struct ads1100_data *data)
> +{
> +	int voltage_uV;
> +
> +	if (data->chip_info->has_internal_vref_only)
> +		return ADS1110_INTERNAL_REF_mV;
> +
> +	voltage_uV = regulator_get_voltage(data->reg_vdd);
> +	if (voltage_uV < 0)
> +		return voltage_uV;
> +
> +	return voltage_uV / (MICRO / MILLI);
> +}
> +

[Severity: Medium]
Will this return a negative error code (like -EINVAL) to callers that don't
expect it?

If regulator_get_voltage() fails, ads1100_get_vref_millivolts() returns the
error code. However, callers like ads1100_calc_scale_avail() and
ads1100_read_raw() don't check for errors:

drivers/iio/adc/ti-ads1100.c:ads1100_calc_scale_avail() {
    int millivolts = ads1100_get_vref_millivolts(data);
    ...
    data->scale_avail[i * 2 + 0] = millivolts;
}

drivers/iio/adc/ti-ads1100.c:ads1100_read_raw() {
    case IIO_CHAN_INFO_SCALE:
        *val = ads1100_get_vref_millivolts(data);
        ...
}

Could this regression incorrectly expose a negative integer to userspace as a
valid voltage or scale value?

[ ... ]
> @@ -145,7 +183,7 @@ static int ads1100_conversion_busy(struct ads1100_data *data)
>  static int ads1100_wait_single_conversion(struct ads1100_data *data)
>  {
>  	int data_rate_index = FIELD_GET(ADS1100_DR_MASK, data->config);
> -	int data_rate_Hz = ads1100_data_rate[data_rate_index];
> +	int data_rate_Hz = data->chip_info->available_data_rate_hz[data_rate_index];
>  	unsigned long poll_us = DIV_ROUND_CLOSEST(USEC_PER_SEC, data_rate_Hz) / 4;
>  	int busy;
>  	int ret;

[Severity: Medium]
If ads1100_wait_single_conversion() returns an error here, does it leave the
ADC permanently stuck in single-shot mode?

Looking at the caller ads1100_poll_data_ready():

drivers/iio/adc/ti-ads1100.c:ads1100_poll_data_ready() {
    ...
    ret = ads1100_wait_single_conversion(data);
    if (ret)
        return ret;

    return ads1100_set_config_bits(data, ADS1100_CFG_SC, ADS1100_CONTINUOUS);
}

If ads1100_wait_single_conversion() fails, the function returns immediately,
bypassing the step that restores the hardware to continuous conversion mode.

Could this regression cause subsequent continuous reads to fetch stale data
because the ADC stopped converting?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3