* Re: [PATCH v2 2/2] iio: adc: ti-ads112c04: Add support for TI ADS112C04
2026-07-31 2:58 ` [PATCH v2 2/2] iio: adc: ti-ads112c04: Add support for TI ADS112C04 Kyle Hsieh
@ 2026-07-31 3:13 ` sashiko-bot
2026-07-31 9:27 ` Joshua Crofts
2026-07-31 14:54 ` David Lechner
2 siblings, 0 replies; 11+ messages in thread
From: sashiko-bot @ 2026-07-31 3:13 UTC (permalink / raw)
To: Kyle Hsieh; +Cc: robh, devicetree, conor+dt
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
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v2 2/2] iio: adc: ti-ads112c04: Add support for TI ADS112C04
2026-07-31 2:58 ` [PATCH v2 2/2] iio: adc: ti-ads112c04: Add support for TI ADS112C04 Kyle Hsieh
2026-07-31 3:13 ` sashiko-bot
@ 2026-07-31 9:27 ` Joshua Crofts
2026-08-01 22:59 ` Jonathan Cameron
2026-07-31 14:54 ` David Lechner
2 siblings, 1 reply; 11+ messages in thread
From: Joshua Crofts @ 2026-07-31 9:27 UTC (permalink / raw)
To: Kyle Hsieh
Cc: Jonathan Cameron, David Lechner, Nuno Sá, Andy Shevchenko,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Liam Girdwood,
Mark Brown, linux-iio, devicetree, linux-kernel
On Fri, 31 Jul 2026 10:58:25 +0800
Kyle Hsieh <kylehsieh1995@gmail.com> wrote:
> diff --git a/drivers/iio/adc/ti-ads112c04.c b/drivers/iio/adc/ti-ads112c04.c
> new file mode 100644
> index 000000000000..28d3be81934f
> --- /dev/null
> +++ b/drivers/iio/adc/ti-ads112c04.c
Hi Kyle, quick review from me, comments inline. Additionally, please
check Sashiko's review as there are some move severe issues (mostly
I2C stuff), see it here:
https://sashiko.dev/#/patchset/20260731-ti-ads112c04-driver-v2-0-aab0168c3c01%40gmail.com
> @@ -0,0 +1,378 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Texas Instruments ADS112C04 16-bit I2C ADC driver
> + * Based on TI Reference Code and standard Linux IIO framework.
Usually we'd add a Copyright (c) 2026 your_name_here your_email_here
and maybe a datasheet link for easy lookup.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/i2c.h>
> +#include <linux/delay.h>
> +#include <linux/interrupt.h>
> +#include <linux/mutex.h>
Please sort your headers alphabetically.
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
Group <linux/iio/*> headers separately and add them after the generic
<linux/*> headers.
> +#include <linux/regulator/consumer.h>
> +#include <linux/bitfield.h>
> +#include <linux/iopoll.h>
> +#include <linux/property.h>
> +#include <linux/gpio/consumer.h>
Additionally, you're missing jiffies.h, err.h, bitops.h, types.h
> +/* ADS112C04 Commands */
Unnecessary comment IMO, it's clear that these are commands from the
*_CMD_* part (same goes for your registers comment).
> +#define ADS112C04_CMD_RESET 0x06
> +#define ADS112C04_CMD_START_SYNC 0x08
> +#define ADS112C04_CMD_POWERDOWN 0x02
> +#define ADS112C04_CMD_RDATA 0x10
> +#define ADS112C04_CMD_RREG(reg) (0x20 | ((reg) << 2))
> +#define ADS112C04_CMD_WREG(reg) (0x40 | ((reg) << 2))
> +
> +/* Registers */
> +#define ADS112C04_REG_CONFIG0 0x00
> +#define ADS112C04_REG_CONFIG1 0x01
> +#define ADS112C04_REG_CONFIG2 0x02
> +#define ADS112C04_REG_CONFIG3 0x03
> +
> +#define ADS112C04_DRDY_MASK BIT(7)
> +#define ADS112C04_MUX_MASK GENMASK(7, 4)
> +
> +struct ads112c04_state {
> + struct i2c_client *client;
> + /* Protects concurrent ADC reads and device configuration */
> + struct mutex lock;
> + struct completion completion;
> + struct regulator *vref_reg;
> + int vref_mv;
vref_mV, this is a good exception to the no camelCase rule, as it's a SI
unit.
> + u8 config0;
> + u8 config1;
> +};
> +
> +static int ads112c04_write_cmd(struct i2c_client *client, u8 cmd)
> +{
> + int ret = i2c_master_send(client, &cmd, 1);
> +
> + return ret < 0 ? ret : 0;
> +}
> +
> +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);
i2c_master_send returns either the amount of bytes sent or an error
code. If the device NACKs, the function will return 0 (zero bytes sent)
but this will be interpreted as success.
> + if (ret < 0)
> + return ret;
> +
> + ret = i2c_master_recv(client, val, 1);
> + return ret < 0 ? ret : 0;
> +}
> +
> +static int ads112c04_write_reg(struct i2c_client *client, u8 reg, u8 val)
> +{
> + u8 buf[2] = { ADS112C04_CMD_WREG(reg), val };
> + int ret;
> +
> + ret = i2c_master_send(client, buf, 2);
Use sizeof, don't hardcode the buffer sizes.
> + 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,
Sashiko points out that read_poll_timeout discards any I2C read errors and returns
0. Remove the ret < 0 condition.
> + st->client, ADS112C04_REG_CONFIG2, &val);
> +}
> +
> +static int ads112c04_read_data(struct ads112c04_state *st, int *val)
> +{
> + u8 cmd = ADS112C04_CMD_RDATA;
> + __be16 buf;
> + int ret;
> +
> + ret = i2c_master_send(st->client, &cmd, 1);
> + if (ret < 0)
> + return ret;
> +
> + ret = i2c_master_recv(st->client, (u8 *)&buf, 2);
> + if (ret < 0)
> + return ret;
> +
> + *val = sign_extend32(be16_to_cpu(buf), 15);
> + return 0;
> +}
> +
> +static int ads112c04_get_adc_result(struct ads112c04_state *st,
> + struct iio_chan_spec const *chan,
> + int *val)
> +{
> + int ret;
> + u8 mux, new_config0;
Reverse xmas tree order please.
> +
> + mux = FIELD_PREP(ADS112C04_MUX_MASK, chan->address);
> + new_config0 = (st->config0 & 0x0F) | mux;
> +
> + 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);
> + if (ret < 0)
> + return ret;
> +
> + return ads112c04_read_data(st, val);
> +}
> +
> +static int ads112c04_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int *val, int *val2, long mask)
> +{
> + struct ads112c04_state *st = iio_priv(indio_dev);
> + int ret;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> + mutex_lock(&st->lock);
> + ret = ads112c04_get_adc_result(st, chan, val);
> + mutex_unlock(&st->lock);
> +
> + if (ret < 0)
> + return ret;
> + return IIO_VAL_INT;
> +
> + case IIO_CHAN_INFO_SCALE:
> + *val = st->vref_mv;
> + *val2 = 15;
> + return IIO_VAL_FRACTIONAL_LOG2;
> +
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +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;
> +}
> +
> +static const struct iio_info ads112c04_info = {
> + .read_raw = ads112c04_read_raw,
> +};
> +
> +static void ads112c04_regulator_disable(void *data)
> +{
> + regulator_disable(data);
> +}
> +
> +static int ads112c04_parse_channels(struct iio_dev *indio_dev)
> +{
> + struct device *dev = indio_dev->dev.parent;
> + struct iio_chan_spec *channels;
> + u32 num_channels, i = 0, pair[2];
> +
> + num_channels = device_get_named_child_node_count(dev, "channel");
> + if (!num_channels)
> + return -EINVAL;
> +
> + channels = devm_kcalloc(dev, num_channels, sizeof(*channels), GFP_KERNEL);
> + if (!channels)
> + return -ENOMEM;
> +
> + device_for_each_named_child_node_scoped(dev, child, "channel") {
> + struct iio_chan_spec *spec = &channels[i];
> +
> + spec->type = IIO_VOLTAGE;
> + spec->indexed = 1;
> + spec->info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE);
> + spec->scan_index = i;
> +
> + if (fwnode_property_present(child, "single-channel")) {
> + fwnode_property_read_u32(child, "single-channel", &pair[0]);
The return value of fwnode_property_read_u32 isn't checked, meaning that
pair[0] will contain stack garbage.
> + spec->channel = pair[0];
> + spec->differential = 0;
> + spec->address = 0x08 + pair[0];
> + } else if (fwnode_property_present(child, "diff-channels")) {
> + fwnode_property_read_u32_array(child, "diff-channels", pair, 2);
> + spec->channel = pair[0];
> + spec->channel2 = pair[1];
> + spec->differential = 1;
> +
> + if (pair[0] == 0 && pair[1] == 1)
> + spec->address = 0x00;
> + else if (pair[0] == 0 && pair[1] == 2)
> + spec->address = 0x01;
> + else if (pair[0] == 0 && pair[1] == 3)
> + spec->address = 0x02;
> + else if (pair[0] == 1 && pair[1] == 0)
> + spec->address = 0x03;
> + else if (pair[0] == 1 && pair[1] == 2)
> + spec->address = 0x04;
> + else if (pair[0] == 1 && pair[1] == 3)
> + spec->address = 0x05;
> + else if (pair[0] == 2 && pair[1] == 3)
> + spec->address = 0x06;
> + else if (pair[0] == 3 && pair[1] == 2)
> + spec->address = 0x07;
> + else
> + return -EINVAL;
> + } else {
> + return -EINVAL;
> + }
> + i++;
> + }
> +
> + indio_dev->channels = channels;
> + indio_dev->num_channels = num_channels;
> +
> + return 0;
> +}
> +
> +static int ads112c04_probe(struct i2c_client *client)
> +{
> + struct iio_dev *indio_dev;
> + struct ads112c04_state *st;
> + struct gpio_desc *reset_gpio;
Reverse xmas tree order here as well.
> + int ret;
> +
> + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*st));
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + st = iio_priv(indio_dev);
> + st->client = client;
> +
> + ret = devm_mutex_init(&client->dev, &st->lock);
> + if (ret)
> + return ret;
> +
> + init_completion(&st->completion);
> +
> + indio_dev->name = "ads112c04";
> + indio_dev->modes = INDIO_DIRECT_MODE;
> + indio_dev->info = &ads112c04_info;
> +
> + ret = ads112c04_parse_channels(indio_dev);
> + if (ret)
> + return ret;
> +
> + ret = devm_regulator_get_enable(&client->dev, "avdd");
> + if (ret)
> + return dev_err_probe(&client->dev, ret, "failed to get avdd regulator\n");
> +
> + ret = devm_regulator_get_enable(&client->dev, "dvdd");
> + if (ret)
> + return dev_err_probe(&client->dev, ret, "failed to get dvdd regulator\n");
> +
> + st->vref_reg = devm_regulator_get_optional(&client->dev, "refp");
> + if (IS_ERR(st->vref_reg)) {
> + ret = PTR_ERR(st->vref_reg);
> + if (ret == -ENODEV) {
> + st->vref_mv = 2048;
> + st->config1 = 0x00;
> + } else {
> + return ret;
> + }
> + } else {
> + ret = regulator_enable(st->vref_reg);
> + if (ret)
> + return ret;
> +
> + ret = devm_add_action_or_reset(&client->dev, ads112c04_regulator_disable,
> + st->vref_reg);
> + if (ret)
> + return ret;
> +
> + ret = regulator_get_voltage(st->vref_reg);
> + if (ret < 0)
> + return ret;
> +
> + st->vref_mv = ret / 1000;
> + st->config1 = 0x02;
> + }
> +
> + reset_gpio = devm_gpiod_get_optional(&client->dev, "reset", GPIOD_OUT_LOW);
> + if (IS_ERR(reset_gpio))
> + return PTR_ERR(reset_gpio);
> +
> + 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);
> + if (ret < 0)
> + return ret;
> + }
> +
> + fsleep(1000);
Why 1000? Add a comment that links to the datasheet or an explanation.
> +
> + 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,
> + indio_dev->name, indio_dev);
> + if (ret) {
> + dev_err(&client->dev, "Failed to request DRDY IRQ\n");
> + return ret;
Just return ret, devm_request_irq() already prints an error message on
failure.
--
Kind regards,
Joshua Crofts
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v2 2/2] iio: adc: ti-ads112c04: Add support for TI ADS112C04
2026-07-31 9:27 ` Joshua Crofts
@ 2026-08-01 22:59 ` Jonathan Cameron
0 siblings, 0 replies; 11+ messages in thread
From: Jonathan Cameron @ 2026-08-01 22:59 UTC (permalink / raw)
To: Joshua Crofts
Cc: Kyle Hsieh, David Lechner, Nuno Sá, Andy Shevchenko,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Liam Girdwood,
Mark Brown, linux-iio, devicetree, linux-kernel
On Fri, 31 Jul 2026 11:27:50 +0200
Joshua Crofts <joshua.crofts1@gmail.com> wrote:
> On Fri, 31 Jul 2026 10:58:25 +0800
> Kyle Hsieh <kylehsieh1995@gmail.com> wrote:
> > diff --git a/drivers/iio/adc/ti-ads112c04.c b/drivers/iio/adc/ti-ads112c04.c
> > new file mode 100644
> > index 000000000000..28d3be81934f
> > --- /dev/null
> > +++ b/drivers/iio/adc/ti-ads112c04.c
>
> Hi Kyle, quick review from me, comments inline. Additionally, please
> check Sashiko's review as there are some move severe issues (mostly
> I2C stuff), see it here:
> https://sashiko.dev/#/patchset/20260731-ti-ads112c04-driver-v2-0-aab0168c3c01%40gmail.com
Be careful with these. Some may be misleading or the correct
response may be in a very different place to sashiko suggests.
For example we wouldn't typically bother to defend against nonsense
interrupt types from DT, so if level isn't a plausible type then
state what is as a comment in the DT.
Also, multi master doesn't seems like something we should worry
too much about. That's not to say there isn't a better way to handle that
transaction.
> > +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);
>
> i2c_master_send returns either the amount of bytes sent or an error
> code. If the device NACKs, the function will return 0 (zero bytes sent)
> but this will be interpreted as success.
This one is fun and not necessarily that simple. (I used to assume it was!)
Mostly nacks will result in error codes, but there are other obscure
reasons it might return 0.
Anyhow, whatever the reason, agreed we annoyingly have to check these to see if
they are not 0.
Or, better, as per David's comment use the smbus command if that is possible.
>
> > + if (ret < 0)
> > + return ret;
> > +
> > + ret = i2c_master_recv(client, val, 1);
> > + return ret < 0 ? ret : 0;
> > +}
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/2] iio: adc: ti-ads112c04: Add support for TI ADS112C04
2026-07-31 2:58 ` [PATCH v2 2/2] iio: adc: ti-ads112c04: Add support for TI ADS112C04 Kyle Hsieh
2026-07-31 3:13 ` sashiko-bot
2026-07-31 9:27 ` Joshua Crofts
@ 2026-07-31 14:54 ` David Lechner
2026-07-31 15:09 ` David Lechner
2 siblings, 1 reply; 11+ messages in thread
From: David Lechner @ 2026-07-31 14:54 UTC (permalink / raw)
To: Kyle Hsieh, Jonathan Cameron, Nuno Sá, Andy Shevchenko,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Liam Girdwood,
Mark Brown
Cc: linux-iio, devicetree, linux-kernel
On 7/30/26 9:58 PM, Kyle Hsieh wrote:
> Add IIO driver support for the Texas Instruments ADS112C04 (16-bit)
> delta-sigma ADCs.
>
> The driver implements:
> - 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
> reference if not specified.
> - Hardware reset fallback using GPIO.
>
> Signed-off-by: Kyle Hsieh <kylehsieh1995@gmail.com>
> ---
> drivers/iio/adc/Kconfig | 10 ++
> drivers/iio/adc/Makefile | 1 +
> drivers/iio/adc/ti-ads112c04.c | 378 +++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 389 insertions(+)
>
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 3755a81c1efd..402e841bc083 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -1789,6 +1789,16 @@ config TI_ADS1119
> This driver can also be built as a module. If so, the module will be
> called ti-ads1119.
>
> +config TI_ADS112C04
> + tristate "Texas Instruments ADS112C04 ADC"
> + depends on I2C
> + help
> + If you say yes here you get support for Texas Instruments
> + ADS112C04 (16-bit) I2C analog to digital converters.
> +
> + This driver can also be built as a module. If so, the module will be
> + called ti-ads112c04.
> +
> config TI_ADS124S08
> tristate "Texas Instruments ADS124S08"
> depends on SPI
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index 707dd708912f..ebf9d4047a5a 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -153,6 +153,7 @@ obj-$(CONFIG_TI_ADS1015) += ti-ads1015.o
> obj-$(CONFIG_TI_ADS1018) += ti-ads1018.o
> obj-$(CONFIG_TI_ADS1100) += ti-ads1100.o
> obj-$(CONFIG_TI_ADS1119) += ti-ads1119.o
> +obj-$(CONFIG_TI_ADS112C04) += ti-ads112c04.o
> obj-$(CONFIG_TI_ADS124S08) += ti-ads124s08.o
> obj-$(CONFIG_TI_ADS1298) += ti-ads1298.o
> obj-$(CONFIG_TI_ADS131E08) += ti-ads131e08.o
> diff --git a/drivers/iio/adc/ti-ads112c04.c b/drivers/iio/adc/ti-ads112c04.c
> new file mode 100644
> index 000000000000..28d3be81934f
> --- /dev/null
> +++ b/drivers/iio/adc/ti-ads112c04.c
> @@ -0,0 +1,378 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Texas Instruments ADS112C04 16-bit I2C ADC driver
> + * Based on TI Reference Code and standard Linux IIO framework.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/i2c.h>
> +#include <linux/delay.h>
> +#include <linux/interrupt.h>
> +#include <linux/mutex.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/bitfield.h>
> +#include <linux/iopoll.h>
> +#include <linux/property.h>
> +#include <linux/gpio/consumer.h>
> +
> +/* ADS112C04 Commands */
> +#define ADS112C04_CMD_RESET 0x06
> +#define ADS112C04_CMD_START_SYNC 0x08
> +#define ADS112C04_CMD_POWERDOWN 0x02
> +#define ADS112C04_CMD_RDATA 0x10
> +#define ADS112C04_CMD_RREG(reg) (0x20 | ((reg) << 2))
> +#define ADS112C04_CMD_WREG(reg) (0x40 | ((reg) << 2))
> +
> +/* Registers */
> +#define ADS112C04_REG_CONFIG0 0x00
> +#define ADS112C04_REG_CONFIG1 0x01
> +#define ADS112C04_REG_CONFIG2 0x02
> +#define ADS112C04_REG_CONFIG3 0x03
> +
> +#define ADS112C04_DRDY_MASK BIT(7)
> +#define ADS112C04_MUX_MASK GENMASK(7, 4)
We like to include the name of the register in the mask names
(and usually don't bother with _MASK to keep it shorter) and
organize the fields under the registers with a bit of indent.
Something like:
#define ADS112C04_REG_CONFIG0 0x00
#define ADS112C04_CONFIG0_MUX GENMASK(7, 4)
#define ADS112C04_REG_CONFIG1 0x01
#define ADS112C04_REG_CONFIG2 0x02
#define ADS112C04_CONFIG2_DRDY BIT(7)
#define ADS112C04_REG_CONFIG3 0x03
> +
> +struct ads112c04_state {
> + struct i2c_client *client;
> + /* Protects concurrent ADC reads and device configuration */
> + struct mutex lock;
> + struct completion completion;
> + struct regulator *vref_reg;
> + int vref_mv;
> + u8 config0;
> + u8 config1;
> +};
> +
> +static int ads112c04_write_cmd(struct i2c_client *client, u8 cmd)
> +{
> + int ret = i2c_master_send(client, &cmd, 1);
> +
> + return ret < 0 ? ret : 0;
> +}
> +
> +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);
> + return ret < 0 ? ret : 0;
This can be done in one I2C call.
ret = i2c_smbus_read_byte_data(client, cmd);
if (ret < 0)
reutrn ret;
*val = ret;
return 0;
> +}
> +
> +static int ads112c04_write_reg(struct i2c_client *client, u8 reg, u8 val)
> +{
> + u8 buf[2] = { ADS112C04_CMD_WREG(reg), val };
> + int ret;
> +
> + ret = i2c_master_send(client, buf, 2);
> + return ret < 0 ? ret : 0;
And here it can be:
u8 cmd = ADS112C04_CMD_WREG(reg);
return i2c_smbus_write_byte_data(client, cmd, val);
> +}
> +
> +static int ads112c04_wait_for_data(struct ads112c04_state *st)
> +{
> + int ret;
> + u8 val;
The slowest data rate is 20 SPS, so wouldn't 100 ms timeout be more than
enough? 1 second seems a bit long.
> +
> + 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,
When there is more than 3 or 4 zeros, it is helpful to use macros, like
1 * MICRO.
> + st->client, ADS112C04_REG_CONFIG2, &val);
> +}
> +
> +static int ads112c04_read_data(struct ads112c04_state *st, int *val)
> +{
> + u8 cmd = ADS112C04_CMD_RDATA;
> + __be16 buf;
I would call this data instead of buf.
> + int ret;
> +
> + ret = i2c_master_send(st->client, &cmd, 1);
> + if (ret < 0)
> + return ret;
> +
> + ret = i2c_master_recv(st->client, (u8 *)&buf, 2);
sizeof(buf) instead of 2.
> + if (ret < 0)
> + return ret;
Or more simply ...
ret = i2c_smbus_read_word_data(client, cmd);
if (ret < 0)
return ret;
*val = sign_extend32(be16_to_cpu(ret), 15);
> +
> + *val = sign_extend32(be16_to_cpu(buf), 15);
> + return 0;
> +}
> +
> +static int ads112c04_get_adc_result(struct ads112c04_state *st,
> + struct iio_chan_spec const *chan,
> + int *val)
> +{
> + int ret;
> + u8 mux, new_config0;
> +
> + mux = FIELD_PREP(ADS112C04_MUX_MASK, chan->address);
> + new_config0 = (st->config0 & 0x0F) | mux;
~ADS112C04_MUX_MASK instead of 0x0F.
Or:
new_config0 = st->config0;
FIELD_MODIFY(ADS112C04_MUX_MASK, &new_config0, 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);
> + if (ret < 0)
> + return ret;
> +
> + return ads112c04_read_data(st, val);
> +}
> +
> +static int ads112c04_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int *val, int *val2, long mask)
> +{
> + struct ads112c04_state *st = iio_priv(indio_dev);
> + int ret;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> + mutex_lock(&st->lock);
> + ret = ads112c04_get_adc_result(st, chan, val);
> + mutex_unlock(&st->lock);
> +
> + if (ret < 0)
> + return ret;
> + return IIO_VAL_INT;
> +
> + case IIO_CHAN_INFO_SCALE:
> + *val = st->vref_mv;
> + *val2 = 15;
> + return IIO_VAL_FRACTIONAL_LOG2;
> +
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +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;
> +}
> +
> +static const struct iio_info ads112c04_info = {
> + .read_raw = ads112c04_read_raw,
> +};
> +
> +static void ads112c04_regulator_disable(void *data)
> +{
> + regulator_disable(data);
> +}
> +
> +static int ads112c04_parse_channels(struct iio_dev *indio_dev)
> +{
> + struct device *dev = indio_dev->dev.parent;
> + struct iio_chan_spec *channels;
> + u32 num_channels, i = 0, pair[2];
> +
> + num_channels = device_get_named_child_node_count(dev, "channel");
> + if (!num_channels)
> + return -EINVAL;
> +
> + channels = devm_kcalloc(dev, num_channels, sizeof(*channels), GFP_KERNEL);
> + if (!channels)
> + return -ENOMEM;
> +
> + device_for_each_named_child_node_scoped(dev, child, "channel") {
> + struct iio_chan_spec *spec = &channels[i];
> +
> + spec->type = IIO_VOLTAGE;
> + spec->indexed = 1;
> + spec->info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE);
> + spec->scan_index = i;
scan_index isn't currently used, so we could leave that out for now.
> +
> + if (fwnode_property_present(child, "single-channel")) {
> + fwnode_property_read_u32(child, "single-channel", &pair[0]);
> + spec->channel = pair[0];
> + spec->differential = 0;
differential is already 0, so don't need to set it here.
> + spec->address = 0x08 + pair[0];
> + } else if (fwnode_property_present(child, "diff-channels")) {
> + fwnode_property_read_u32_array(child, "diff-channels", pair, 2);
> + spec->channel = pair[0];
> + spec->channel2 = pair[1];
> + spec->differential = 1;
> +
> + if (pair[0] == 0 && pair[1] == 1)
> + spec->address = 0x00;
> + else if (pair[0] == 0 && pair[1] == 2)
> + spec->address = 0x01;
> + else if (pair[0] == 0 && pair[1] == 3)
> + spec->address = 0x02;
> + else if (pair[0] == 1 && pair[1] == 0)
> + spec->address = 0x03;
> + else if (pair[0] == 1 && pair[1] == 2)
> + spec->address = 0x04;
> + else if (pair[0] == 1 && pair[1] == 3)
> + spec->address = 0x05;
> + else if (pair[0] == 2 && pair[1] == 3)
> + spec->address = 0x06;
> + else if (pair[0] == 3 && pair[1] == 2)
> + spec->address = 0x07;
> + else
> + return -EINVAL;
Could be useful to use dev_err_probe() to print a message in this case. Helps
when you make a typo in the devicetree.
> + } else {
> + return -EINVAL;
> + }
I would also check if the other properties mentioned in the DT binding review
exist here and return error if they do since they aren't implemented. See
similar example below with refp-refn properties.
> + i++;
> + }
> +
> + indio_dev->channels = channels;
> + indio_dev->num_channels = num_channels;
This should be i. num_channels could actually be > i if any node has status = "disabled";.
> +
> + return 0;
> +}
> +
> +static int ads112c04_probe(struct i2c_client *client)
> +{
> + struct iio_dev *indio_dev;
> + struct ads112c04_state *st;
> + struct gpio_desc *reset_gpio;
> + int ret;
> +
Would be nice to make a local dev variable so we don't have to
write &clinet->dev so much.
> + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*st));
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + st = iio_priv(indio_dev);
> + st->client = client;
> +
> + ret = devm_mutex_init(&client->dev, &st->lock);
> + if (ret)
> + return ret;
> +
> + init_completion(&st->completion);
> +
> + indio_dev->name = "ads112c04";
> + indio_dev->modes = INDIO_DIRECT_MODE;
> + indio_dev->info = &ads112c04_info;
> +
> + ret = ads112c04_parse_channels(indio_dev);
> + if (ret)
> + return ret;
> +
> + ret = devm_regulator_get_enable(&client->dev, "avdd");
> + if (ret)
> + return dev_err_probe(&client->dev, ret, "failed to get avdd regulator\n");
> +
> + ret = devm_regulator_get_enable(&client->dev, "dvdd");
> + if (ret)
> + return dev_err_probe(&client->dev, ret, "failed to get dvdd regulator\n");
> +
We've been tending towards writing below like:
if (device_property_present(dev, "refp-supply") {
ret = devm_regulator_get_enable_read_voltage(dev, "refp");
if (ret < 0)
return dev_err_probe(dev, ret, "failed to read REFP voltage\n");
...
}
And I would check the other ref properties too even if we don't implement them.
if (device_property_present(dev, "refn-supply")
return dev_err_probe(dev, -EOPNOTSUPP, "refn-supply is not implemented\n");
if (device_property_present(dev, "ti,refp-refn-resistor-ohms")
return dev_err_probe(dev, -EOPNOTSUPP, "ti,refp-refn-resistor-ohms is not implemented\n");
> + st->vref_reg = devm_regulator_get_optional(&client->dev, "refp");
> + if (IS_ERR(st->vref_reg)) {
> + ret = PTR_ERR(st->vref_reg);
> + if (ret == -ENODEV) {
> + st->vref_mv = 2048;
> + st->config1 = 0x00;
> + } else {
> + return ret;
> + }
> + } else {
> + ret = regulator_enable(st->vref_reg);
> + if (ret)
> + return ret;
> +
> + ret = devm_add_action_or_reset(&client->dev, ads112c04_regulator_disable,
> + st->vref_reg);
> + if (ret)
> + return ret;
> +
> + ret = regulator_get_voltage(st->vref_reg);
> + if (ret < 0)
> + return ret;
> +
> + st->vref_mv = ret / 1000;
We've been writing this like:
st->vref_mv = ret / (MICRO / MILLI);
> + st->config1 = 0x02;
Add a macro and use FIELD_PREP().
> + }
And as in the DT bindings reply, we should go ahead and make the reference
voltage per channel. Even if we don't need it now, it would be hard to change
it in the future without breaking existing users.
> +
> + reset_gpio = devm_gpiod_get_optional(&client->dev, "reset", GPIOD_OUT_LOW);
If we make this GPIOD_OUT_HIGH, then we save a line later.
> + if (IS_ERR(reset_gpio))
> + return PTR_ERR(reset_gpio);
> +
> + 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);
> + if (ret < 0)
> + return ret;
> + }
> +
> + fsleep(1000);
> +
> + st->config0 = 0x01;
Needs a macro and FIELD_PREP().
Also, since we are parsing channels now, can/should we leave PGA enabled
for diff-channels? Otherwise add a comment explaining current choice.
> + 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,
Can put 0 on the previous line.
> + indio_dev->name, indio_dev);
> + if (ret) {
> + dev_err(&client->dev, "Failed to request DRDY IRQ\n");
> + return ret;
> + }
> + }
> +
> + return devm_iio_device_register(&client->dev, indio_dev);
> +}
> +
> +static const struct i2c_device_id ads112c04_id[] = {
> + { .name = "ads112c04", .driver_data = 0 },
No need for .driver_data since it is 0.
> + { }
> +};
> +MODULE_DEVICE_TABLE(i2c, ads112c04_id);
> +
> +static const struct of_device_id ads112c04_of_match[] = {
> + { .compatible = "ti,ads112c04" },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, ads112c04_of_match);
> +
> +static struct i2c_driver ads112c04_driver = {
> + .driver = {
> + .name = "ads112c04",
> + .of_match_table = ads112c04_of_match,
> + },
> + .probe = ads112c04_probe,
> + .id_table = ads112c04_id,
> +};
> +module_i2c_driver(ads112c04_driver);
> +
> +MODULE_AUTHOR("Kyle Hsieh <kylehsieh1995@gmail.com>");
> +MODULE_DESCRIPTION("Texas Instruments ADS112C04 ADC driver");
> +MODULE_LICENSE("GPL");
>
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v2 2/2] iio: adc: ti-ads112c04: Add support for TI ADS112C04
2026-07-31 14:54 ` David Lechner
@ 2026-07-31 15:09 ` David Lechner
0 siblings, 0 replies; 11+ messages in thread
From: David Lechner @ 2026-07-31 15:09 UTC (permalink / raw)
To: Kyle Hsieh, Jonathan Cameron, Nuno Sá, Andy Shevchenko,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Liam Girdwood,
Mark Brown
Cc: linux-iio, devicetree, linux-kernel
On 7/31/26 9:54 AM, David Lechner wrote:
> On 7/30/26 9:58 PM, Kyle Hsieh wrote:
>> Add IIO driver support for the Texas Instruments ADS112C04 (16-bit)
>> delta-sigma ADCs.
>>
...
>> + st->config0 = 0x01;
>
> Needs a macro and FIELD_PREP().
Correction: This one is a bit, so FIELD_PREP() is not needed.
>
> Also, since we are parsing channels now, can/should we leave PGA enabled
> for diff-channels? Otherwise add a comment explaining current choice.
>
^ permalink raw reply [flat|nested] 11+ messages in thread