Re: [PATCH v3 2/2] iio: adc: ti-ads112c04: Add support for TI ADS112C04

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Incorrect byte swapping for 16-bit I2C word read causes data corruption on Big-Endian architectures.
- [High] The hardirq handler does not mask the interrupt or clear the hardware interrupt source, leading to an infinite interrupt storm if a level-triggered IRQ is used.
- [Medium] The commit message falsely claims support for dynamic reference voltage scaling via `refn-supply`.
- [Medium] A delayed hardware interrupt from a previously timed-out conversion can prematurely complete the wait for a new conversion, causing a read of stale data.
--

--- Patch [2]: [PATCH v3 2/2] iio: adc: ti-ads112c04: Add support for TI ADS112C04 ---
commit bffeb9c9d9c618f12635f25aeb7cd1053228824c
Author: Kyle Hsieh <[email protected]>

iio: adc: ti-ads112c04: Add support for TI ADS112C04

Add IIO driver support for the Texas Instruments ADS112C04 (16-bit)
delta-sigma ADCs.

> - Single-shot conversions using the IIO raw read interface.
> - Dynamic parsing of single-ended and differential channels from
>   device tree child nodes.
> - Hardware interrupt support via the DRDY pin, falling back to
>   software polling if no IRQ is provided.
> - Scale calculation based on the internal 2.048V reference.
> - Dynamic reference voltage scaling via the regulator subsystem
>   (refp-supply/refn-supply), falling back to the internal 2.048V

[Severity: Medium]
The commit message mentions support for refn-supply, but does the code
actually support it?

Looking at ads112c04_probe(), it seems to explicitly reject refn-supply:

	if (device_property_present(dev, "refn-supply") ||
	    device_property_present(dev, "ti,refp-refn-resistor-ohms"))
		return dev_err_probe(dev, -EOPNOTSUPP,
				     "refn-supply and external resistors are not supported yet\n");

Could the commit message be updated to align with the implementation?

>   reference if not specified.
> - Hardware reset fallback using GPIO.
> 
> Signed-off-by: Kyle Hsieh <[email protected]>

[ ... ]

> diff --git a/drivers/iio/adc/ti-ads112c04.c b/drivers/iio/adc/ti-ads112c04.c
> new file mode 100644
> index 0000000000000..64af866f615de
> --- /dev/null
> +++ b/drivers/iio/adc/ti-ads112c04.c

[ ... ]

> +static int ads112c04_read_data(struct ads112c04_state *st, int *val)
> +{
> +	int ret;
> +
> +	ret = i2c_smbus_read_word_data(st->client, ADS112C04_CMD_RDATA);
> +	if (ret < 0)
> +		return ret;
> +
> +	*val = sign_extend32(be16_to_cpu(ret), 15);

[Severity: High]
Does this byte swapping work correctly on big-endian architectures?

i2c_smbus_read_word_data() returns the word already converted to host
endianness based on SMBus little-endian rules. If the device sends
big-endian data, using be16_to_cpu() on the result of
i2c_smbus_read_word_data() can cause data corruption on big-endian systems.

> +
> +	return 0;
> +}
> +
> +static int ads112c04_get_adc_result(struct ads112c04_state *st,
> +				    struct iio_chan_spec const *chan,
> +				    int *val)
> +{
> +	u8 new_config0;
> +	int ret;
> +
> +	new_config0 = st->config0 & ~ADS112C04_CONFIG0_MUX;
> +	new_config0 |= FIELD_PREP(ADS112C04_CONFIG0_MUX, chan->address);
> +
> +	if (st->config0 != new_config0) {
> +		ret = ads112c04_write_reg(st->client, ADS112C04_REG_CONFIG0, new_config0);
> +		if (ret < 0)
> +			return ret;
> +		st->config0 = new_config0;
> +	}
> +
> +	reinit_completion(&st->completion);
> +
> +	ret = ads112c04_write_cmd(st->client, ADS112C04_CMD_START_SYNC);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = ads112c04_wait_for_data(st);

[Severity: Medium]
Is there a race condition here if a previous conversion timed out?

If a previous read timed out but the hardware conversion was still running,
a delayed interrupt from that previous conversion could fire immediately after
reinit_completion() is called. 

Would this cause ads112c04_wait_for_data() to return prematurely and read
stale data?

> +	if (ret < 0)
> +		return ret;
> +
> +	return ads112c04_read_data(st, val);
> +}

[ ... ]

> +static irqreturn_t ads112c04_irq_handler(int irq, void *private)
> +{
> +	struct iio_dev *indio_dev = private;
> +	struct ads112c04_state *st = iio_priv(indio_dev);
> +
> +	complete(&st->completion);
> +
> +	return IRQ_HANDLED;
> +}

[Severity: High]
If the device tree configures this interrupt as level-triggered, could this
handler cause an interrupt storm?

The hardirq handler completes the wait but returns IRQ_HANDLED without
masking the interrupt line or clearing the hardware interrupt source.
Since devm_request_irq() is called in ads112c04_probe() with irqflags=0,
it allows level-triggered configurations from the device tree which would
continually fire until the data is read.

Should this use a threaded interrupt, or mask the IRQ until the data is read?

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