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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] The `read_poll_timeout` macro silently swallows I2C read errors, incorrectly returning success and allowing the driver to read stale or premature data.
- [High] Level-triggered interrupts will cause an unhandled interrupt storm, locking up the CPU.
- [Medium] Missing return value checks for `fwnode_property_read_u32` leads to the use of uninitialized stack memory.
- [Medium] The driver claims support for differential reference voltage scaling but omits the negative reference (refn-supply).
- [Medium] Non-atomic I2C register reads lack a Repeated Start condition, risking multi-master interference.
- [Low] The driver accepts any value from the `single-channel` property without bounds validation, risking silent out-of-bounds MUX configuration.
- [Low] Missing Power-On Reset (POR) delay after enabling regulators.
--

--- Patch [2]: [PATCH v2 2/2] iio: adc: ti-ads112c04: Add support for TI ADS112C04 ---
commit 0ef91a5fb4e473da243372df8436c4501bbc780c
Author: Kyle Hsieh <kylehsieh1995@gmail.com>

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

This patch adds IIO driver support for the Texas Instruments ADS112C04 16-bit
delta-sigma ADC. It implements raw reads, device tree parsing, hardware
interrupts/polling, and regulator-based dynamic reference scaling.

> diff --git a/drivers/iio/adc/ti-ads112c04.c b/drivers/iio/adc/ti-ads112c04.c
> new file mode 100644
> index 0000000000000..28d3be81934f9
> --- /dev/null
> +++ b/drivers/iio/adc/ti-ads112c04.c
[ ... ]
> +static int ads112c04_read_reg(struct i2c_client *client, u8 reg, u8 *val)
> +{
> +	u8 cmd = ADS112C04_CMD_RREG(reg);
> +	int ret;
> +
> +	ret = i2c_master_send(client, &cmd, 1);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = i2c_master_recv(client, val, 1);

[Severity: Medium]
Will this sequence of i2c_master_send() followed by i2c_master_recv() cause
problems on a multi-master I2C bus?

Because this splits the read into two separate transfers with a STOP
condition in between, another master could initiate a transaction before the
read completes, disrupting the ADC's state. Could i2c_transfer() be used
instead to guarantee an atomic transaction with a Repeated Start?

> +	return ret < 0 ? ret : 0;
> +}
[ ... ]
> +static int ads112c04_wait_for_data(struct ads112c04_state *st)
> +{
> +	int ret;
> +	u8 val;
> +
> +	if (st->client->irq > 0) {
> +		ret = wait_for_completion_timeout(&st->completion, msecs_to_jiffies(1000));
> +		if (!ret)
> +			return -ETIMEDOUT;
> +		return 0;
> +	}
> +
> +	return read_poll_timeout(ads112c04_read_reg, ret,
> +				 (ret < 0 || (val & ADS112C04_DRDY_MASK)),
> +				 1000, 1000000, false,
> +				 st->client, ADS112C04_REG_CONFIG2, &val);

[Severity: High]
Does this polling loop properly handle I2C read errors?

When an I2C bus error occurs and ads112c04_read_reg() fails, ret becomes
negative. This makes the condition (ret < 0) evaluate to true, causing the
read_poll_timeout() macro to successfully break out of the loop and evaluate
to 0.

Since the driver directly returns the macro's result instead of checking if
ret contains an error, ads112c04_get_adc_result() will assume the conversion
is complete. This leads to reading and returning stale or corrupted data to
userspace.

> +}
[ ... ]
> +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]
Can this handler lead to a CPU hard lockup if the DRDY interrupt is configured
as level-triggered in the device tree?

Since the hardware interrupt source is not cleared before returning
IRQ_HANDLED, a level-triggered interrupt will remain asserted. The interrupt
controller will then immediately re-trigger this handler infinitely, starving
the process context.

> +}
[ ... ]
> +static int ads112c04_parse_channels(struct iio_dev *indio_dev)
> +{
[ ... ]
> +		if (fwnode_property_present(child, "single-channel")) {
> +			fwnode_property_read_u32(child, "single-channel", &pair[0]);

[Severity: Medium]
What happens if the single-channel property is present but malformed or empty
in the device tree?

Since the return value of fwnode_property_read_u32() is not checked, pair[0]
might remain uninitialized if the read fails. The driver would then proceed
to calculate spec->address using uninitialized stack memory.

> +			spec->channel = pair[0];
> +			spec->differential = 0;
> +			spec->address = 0x08 + pair[0];

[Severity: Low]
Is it possible for pair[0] to exceed the valid 0-3 range if the device tree is
misconfigured?

Without bounds validation, an out-of-bounds value here will be incorporated
into spec->address and eventually bit-truncated by FIELD_PREP() in
ads112c04_get_adc_result(), resulting in a silent and unpredictable hardware
multiplexer state.

> +		} else if (fwnode_property_present(child, "diff-channels")) {
[ ... ]
> +static int ads112c04_probe(struct i2c_client *client)
> +{
[ ... ]
> +	st->vref_reg = devm_regulator_get_optional(&client->dev, "refp");

[Severity: Medium]
The commit message states the driver supports dynamic reference scaling via
both refp-supply and refn-supply, but is refn-supply actually parsed?

Because only refp is requested and used to calculate st->vref_mv, the driver
implicitly assumes VREFN is always 0V. If a board uses a differential
reference voltage with a non-zero VREFN (for example, 3V and 1V), this will
calculate an incorrect ADC scale based on 3000mV instead of 2000mV.

> +	if (IS_ERR(st->vref_reg)) {
[ ... ]
> +	if (reset_gpio) {
> +		gpiod_set_value_cansleep(reset_gpio, 1);
> +		fsleep(1000);
> +		gpiod_set_value_cansleep(reset_gpio, 0);
> +	} else {
> +		ret = ads112c04_write_cmd(client, ADS112C04_CMD_RESET);

[Severity: Low]
Could the device NACK this I2C command if it hasn't finished its Power-On
Reset sequence?

The avdd and dvdd regulators were enabled earlier in ads112c04_probe(), but
there is no stabilization delay before this software reset command is issued.
This might cause intermittent driver probe failures on certain boards.

> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	fsleep(1000);
> +
> +	st->config0 = 0x01;
> +	ret = ads112c04_write_reg(client, ADS112C04_REG_CONFIG0, st->config0);
> +	if (ret)
> +		return ret;
> +
> +	ret = ads112c04_write_reg(client, ADS112C04_REG_CONFIG1, st->config1);
> +	if (ret)
> +		return ret;
> +
> +	if (client->irq > 0) {
> +		ret = devm_request_irq(&client->dev, client->irq,
> +				       ads112c04_irq_handler,
> +				       0,

[Severity: High]
Does passing 0 for irqflags safely handle all device tree configurations?

Passing 0 allows the interrupt to inherit its trigger type from the device
tree. However, as noted in the ads112c04_irq_handler() comments, the driver
does not clear the hardware interrupt source on level triggers. Should this
enforce IRQF_TRIGGER_FALLING to prevent potential CPU lockups if a level
trigger is specified in the device tree?

> +				       indio_dev->name, indio_dev);
> +		if (ret) {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260731-ti-ads112c04-driver-v2-0-aab0168c3c01@gmail.com?part=2