Re: [PATCH v5 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 4 potential issue(s) to consider:

New issues:
- [Medium] The IIO device name is hardcoded to 'ads1100', ignoring the dynamically added configuration name.
- [Medium] The polling timeout calculation ignores the device-specific data rates and always uses the ADS1100 data rates, leading to excessive sleep times for the ADS1110.

Pre-existing issues:
- [High] The `ads1100_set_data_rate()` function completely ignores the user-requested rate and always applies the maximum data rate.
- [High] Driver removal while the device is in runtime suspend causes I2C timeouts and an unbalanced regulator disable.
--

commit 97f3fd71176aef31ec01d7462b178bf5156c7ab6
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/ti-ads1100.c b/drivers/iio/adc/ti-ads1100.c
> index 71b31adeba3c6..c937e00e0a407 100644
> --- a/drivers/iio/adc/ti-ads1100.c
> +++ b/drivers/iio/adc/ti-ads1100.c
[ ... ]
> @@ -41,20 +41,44 @@
>  #define	ADS1100_SINGLESHOT	ADS1100_CFG_SC
>  
>  #define ADS1100_SLEEP_DELAY_MS	2000
> +#define ADS1110_INTERNAL_REF_mV 2048
>  
>  static const int ads1100_data_rate[] = { 128, 32, 16, 8 };
> +static const int ads1110_data_rate[] = { 240, 60, 30, 15 };

[Severity: Medium]
Does this new data rate array also need to be used in
ads1100_poll_data_ready()?

Looking at that function, it still calculates the wait time using the original
ads1100_data_rate array:

    int data_rate_hz = ads1100_data_rate[FIELD_GET(ADS1100_DR_MASK, data->config)];

If an ADS1110 is used, won't this result in calculating the timeout based on
the slower ADS1100 sample rates instead of the device's actual rate?

>  static const int ads1100_data_rate_bits[] = { 12, 14, 15, 16 };
>  
>  /* Timeout based on the minimum sample rate of 8 SPS (7500ms) */
>  #define ADS1100_MAX_DRDY_TIMEOUT_US	(7500 * USEC_PER_MSEC)
>  
> +struct ads1100_config {
> +	const char *name;
> +	const int *available_data_rate_hz;
> +	const int data_rate_count;
> +	bool has_internal_vref_only;
> +};

[Severity: Medium]
Is the name field in this struct intended to be used for the IIO device name?

Looking at ads1100_probe(), it appears the device name remains hardcoded:

    indio_dev->name = "ads1100";

Will the ADS1110 device be incorrectly exposed as "ads1100" in sysfs because
it ignores this new configuration name?

[ ... ]
> @@ -204,7 +242,7 @@ static int ads1100_set_data_rate(struct ads1100_data *data, int chan, int rate)
>  	unsigned int size;
>  	int ret;
>  
> -	size = data->supports_data_rate ? ARRAY_SIZE(ads1100_data_rate) : 1;
> +	size = data->supports_data_rate ? data->ads_config->data_rate_count : 1;
>  	for (i = 0; i < size; i++) {
>  		if (i == size)
>  			return -EINVAL;
> @@ -225,14 +263,9 @@ static int ads1100_set_data_rate(struct ads1100_data *data, int chan, int rate)
>  		PM_RUNTIME_ACQUIRE_IF_ENABLED_AUTOSUSPEND(&data->client->dev, pm);
>  		ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
>  		if (ret)
>  			return ret;
>  
>  		ret = ads1100_set_config_bits(data, ADS1100_DR_MASK,
>  					      FIELD_PREP(ADS1100_DR_MASK, i));
>  		if (ret)
>  			return ret;
>  
>  		return ads1100_poll_data_ready(data);
>  	}

[Severity: High]
This is a pre-existing issue, but the loop in ads1100_set_data_rate() lacks
any condition to check if the requested rate matches the available rates.

It seems the loop will always apply i = 0 on the first iteration and return.
Does this silently force the hardware to its maximum rate regardless of user
input?

[ ... ]
> @@ -458,16 +497,18 @@ static DEFINE_RUNTIME_DEV_PM_OPS(ads1100_pm_ops,
>  				 ads1100_runtime_suspend,
>  				 ads1100_runtime_resume,
>  				 NULL);

[Severity: High]
This is a pre-existing issue, but does driver removal while the device is in
runtime suspend cause teardown issues?

During driver unbind, the I2C core executes removal without automatically
resuming the device. Because there is no .remove callback to call
pm_runtime_get_sync() before teardown, devres will execute
ads1100_disable_continuous() followed by ads1100_reg_disable().

Since ads1100_runtime_suspend() already disabled the regulator, won't this
lead to an I2C timeout when trying to write to the unpowered device, followed
by an unbalanced regulator disable?

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