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

New issues:
- [Medium] Hardcoded IIO device name ignores the newly added device configuration name.
- [Medium] Hardcoded ads1100_data_rate array used instead of config-specific data_rate in ads1100_poll_data_ready.

Pre-existing issues:
- [High] Missing PM runtime resume on device removal causes an unbalanced regulator_disable() call.
--

commit d015d54867f4b919a8e4d7c10d3727993b652f65
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 e3c801381434d..ec79a89464fb6 100644
> --- a/drivers/iio/adc/ti-ads1100.c
> +++ b/drivers/iio/adc/ti-ads1100.c
> @@ -40,20 +40,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]
Will this result in using the slower ads1100_data_rate array for the ADS1110
when polling for data readiness?

Looking at ads1100_poll_data_ready(), it still accesses the hardcoded
ads1100_data_rate array directly:

static int ads1100_poll_data_ready(struct ads1100_data *data)
{
	...
	int datarate = ads1100_data_rate[FIELD_GET(ADS1100_DR_MASK, data->config)];
	/* To be sure we wait 5 times more than datarate */
	unsigned long wait_time = DIV_ROUND_CLOSEST(MICRO, 5 * datarate);
	...
}

Should this be updated to use data->ads_config->data_rate instead?

[ ... ]

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

[Severity: Medium]
Does this ignore the newly added device configuration name for the IIO device?

In ads1100_probe(), indio_dev->name is hardcoded earlier in the function:

	indio_dev->name = "ads1100";

Should this be updated to use data->ads_config->name so the user-space
sysfs name correctly identifies the ADS1110?

[Severity: High]
This is a pre-existing issue, but does the lack of a PM runtime resume on
device removal cause an unbalanced regulator disable?

If the device auto-suspends during runtime idle, the vdd regulator is disabled
via ads1100_runtime_suspend():

static int ads1100_runtime_suspend(struct device *dev)
{
	...
	regulator_disable(data->reg_vdd);
	...
}

When the driver is unbound, devres calls pm_runtime_disable() (which does not
resume the device), followed by ads1100_reg_disable() which attempts to call
regulator_disable() again on the already-disabled regulator, triggering a
kernel WARNING.

Could we ensure the device is resumed during driver unbind to prevent this?

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