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

Hi David,

Thank you for the detailed review and for pointing out the modern kernel APIs.

On Tue, Jul 28, 2026 at 11:21 PM David Lechner <[email protected]> wrote:
>
> On 7/28/26 3:01 AM, 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.
> > - MUX configuration for single-ended and differential channels.
> > - Hardware interrupt support via the DRDY pin, falling back to
> >   software polling of the configuration register if no IRQ is provided.
> > - Scale calculation based on the internal 2.048V reference.
> > - Dynamic reference voltage scaling via the regulator subsystem
> >   (vref-supply), falling back to the internal 2.048V reference
> >   if not specified.
>
> Nice to see it starting with a minimal feature set like this. Makes it easy
> to review.
>
> In the cover letter, it would be nice to know if this is all the features
> you need or if you plan to add more soon.
I will clarify this in the next version cover letter. This minimal set
covers our
current use case now.
>
> >
> > Signed-off-by: Kyle Hsieh <[email protected]>
> > ---
> >  drivers/iio/adc/Kconfig        |  10 ++
> >  drivers/iio/adc/Makefile       |   1 +
> >  drivers/iio/adc/ti-ads112c04.c | 317 +++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 328 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..0f1d415e3f21
> > --- /dev/null
> > +++ b/drivers/iio/adc/ti-ads112c04.c
> > @@ -0,0 +1,317 @@
> > +// SPDX-License-Identifier: GPL-2.0
>
> Prefer either GPL-2.0-only or GPL-2.0-or-later (your choice).
>
> > +/*
> > + * 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>
> > +
> > +/* 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
> > +
> > +/* Config2 Masks */
> > +#define ADS112C04_DRDY_MASK         BIT(7)
> > +
> > +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)
> > +{
> > +     return i2c_master_send(client, &cmd, 1);
> > +}
> > +
> > +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;
> > +}
> > +
> > +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;
> > +}
> > +
> > +static int ads112c04_wait_for_data(struct ads112c04_state *st)
> > +{
> > +     int ret, timeout = 50;
>
> Why is poll timeout 50x longer than interrupt timeout?
>
> > +     u8 val;
> > +
> > +     /* If IRQ is available, wait for DRDY interrupt completion */
> > +     if (st->client->irq > 0) {
> > +             ret = wait_for_completion_timeout(&st->completion, msecs_to_jiffies(1000));
> > +             if (!ret)
> > +                     return -ETIMEDOUT;
> > +             return 0;
> > +     }
> > +
> > +     /* Fallback: Poll DRDY bit in CONFIG2 */
>
> Use read_poll_timeout() or if you convert to use regmap, regmap_read_poll_timeout().
I will switch to `read_poll_timeout()` and align the timeout duration properly.
>
> > +     while (timeout--) {
> > +             ret = ads112c04_read_reg(st->client, ADS112C04_REG_CONFIG2, &val);
> > +             if (ret < 0)
> > +                     return ret;
> > +             if (val & ADS112C04_DRDY_MASK)
> > +                     return 0;
> > +             usleep_range(1000, 2000);
> > +     }
> > +
> > +     return -ETIMEDOUT;
> > +}
> > +
> > +static int ads112c04_read_data(struct ads112c04_state *st, int *val)
> > +{
> > +     u8 cmd = ADS112C04_CMD_RDATA;
> > +     u8 buf[2];
>
> Use __be16 buf;
>
> > +     int ret;
> > +
> > +     ret = i2c_master_send(st->client, &cmd, 1);
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     ret = i2c_master_recv(st->client, buf, 2);
> > +     if (ret < 0)
> > +             return ret;
>
> Can be simplified by using smbus APIs. It would just be one call.
>
> > +
> > +     /* 16-bit 2's complement conversion */
> > +     *val = (s16)((buf[0] << 8) | buf[1]);
>
> And here use be16_to_cpu() and sign_extend32(). Then no comments needed.
Thank you for the suggestion. I will update the data reading logic to use
`__be16`, `be16_to_cpu()`, and `sign_extend32()` for a much cleaner
implementation.
>
> > +     return 0;
> > +}
> > +
> > +static int ads112c04_get_adc_result(struct ads112c04_state *st,
> > +                                 struct iio_chan_spec const *chan,
> > +                                 int *val)
> > +{
> > +     int ret;
> > +     u8 mux;
> > +
> > +     mux = (chan->address << 4) & 0xF0;
>
> Use FIELD_PREP().
Got it.
>
> > +     if ((st->config0 & 0xF0) != mux) {
>
> Another option would be to use regmap with cache so that we don't
> have to keep track of the current config ourselves.
>
> > +             st->config0 = (st->config0 & 0x0F) | mux;
> > +             ads112c04_write_reg(st->client, ADS112C04_REG_CONFIG0, st->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, raw;
> > +
> > +     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; /* 2^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);
>
> nit: add blank line here.
Will fix.
>
> > +     return IRQ_HANDLED;
> > +}
> > +
> > +#define ADS112C04_V_CHAN(_chan, _addr) {                     \
> > +     .type = IIO_VOLTAGE,                                    \
> > +     .indexed = 1,                                           \
> > +     .channel = _chan,                                       \
> > +     .address = _addr,                                       \
> > +     .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),           \
> > +     .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),   \
> > +}
> > +
> > +#define ADS112C04_V_DIFF_CHAN(_chan, _chan2, _addr) {                \
> > +     .type = IIO_VOLTAGE,                                    \
> > +     .differential = 1,                                      \
> > +     .indexed = 1,                                           \
> > +     .channel = _chan,                                       \
> > +     .channel2 = _chan2,                                     \
> > +     .address = _addr,                                       \
> > +     .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),           \
> > +     .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),   \
> > +}
> > +
> > +static const struct iio_chan_spec ads112c04_channels[] = {
> > +     ADS112C04_V_DIFF_CHAN(0, 1, 0x00),
> > +     ADS112C04_V_DIFF_CHAN(0, 2, 0x01),
> > +     ADS112C04_V_DIFF_CHAN(0, 3, 0x02),
> > +     ADS112C04_V_DIFF_CHAN(1, 0, 0x03),
> > +     ADS112C04_V_DIFF_CHAN(1, 2, 0x04),
> > +     ADS112C04_V_DIFF_CHAN(1, 3, 0x05),
> > +     ADS112C04_V_DIFF_CHAN(2, 3, 0x06),
> > +     ADS112C04_V_DIFF_CHAN(3, 2, 0x07),
> > +     ADS112C04_V_CHAN(0, 0x08),
> > +     ADS112C04_V_CHAN(1, 0x09),
> > +     ADS112C04_V_CHAN(2, 0x0A),
> > +     ADS112C04_V_CHAN(3, 0x0B),
> > +};
> > +
> > +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_probe(struct i2c_client *client)
> > +{
> > +     struct iio_dev *indio_dev;
> > +     struct ads112c04_state *st;
> > +     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;
> > +     mutex_init(&st->lock);
>
>         ret = devm_mutex_init(&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;
> > +     indio_dev->channels = ads112c04_channels;
> > +     indio_dev->num_channels = ARRAY_SIZE(ads112c04_channels);
>
> We need more information to know how to configure each channel, so we
> probably won't be able to use fixed channels like this.
I will drop the fixed array and refactor the driver to parse the
channel configurations
dynamically from the devicetree child nodes, matching the dt-bindings update.
>
> > +
> > +     st->vref_reg = devm_regulator_get_optional(&client->dev, "vref");
>
> This is gonig to be more compilicated. See ti-ads112c14 driver.

>
> > +     if (IS_ERR(st->vref_reg)) {
> > +             ret = PTR_ERR(st->vref_reg);
> > +             if (ret == -ENODEV) {
> > +                     st->vref_mv = 2048;
> > +                     st->config1 = 0x00; /* VREF[1:0] = 00 */
> > +             } 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 device to defaults */
>
> There is a hardware reset, so the way we usually do that is to use
> devm_reset_control_get_optional_exclusive() to see if the gpio is
> wired up. If it is, use that to do the reset, otherwise fall back
> to the software reset.
I will implement the hardware reset fallback logic using
`devm_gpiod_get_optional()` as you suggested.
>
> > +     ads112c04_write_cmd(client, ADS112C04_CMD_RESET);
> > +     usleep_range(1000, 2000);
>
>         fsleep(1000);
>
> > +
> > +     /* Set defaults: PGA Disabled (For single-ended safety), Gain=1 */
> > +     st->config0 = 0x01;
> > +
> > +     ads112c04_write_reg(client, ADS112C04_REG_CONFIG1, st->config1);
>
> Check return value.
Will fix.
>
> > +
> > +     if (client->irq > 0) {
> > +             ret = devm_request_irq(&client->dev, client->irq,
> > +                                    ads112c04_irq_handler,
> > +                                    IRQF_TRIGGER_FALLING,
>
> This flag already comes from the devicetree and should not be hard-coded
> here.
I will change this to `0` to inherit the trigger type from the devicetree.
>
> > +                                    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[] = {
> > +     { "ads112c04", 0 },
>
>         { .name = "ads112c04" },
>
Will update to use the named initializer.
Thanks again for your time and the fantastic guidance!
> > +     { }
> > +};
> > +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 <[email protected]>");
> > +MODULE_DESCRIPTION("Texas Instruments ADS112C04 ADC driver");
> > +MODULE_LICENSE("GPL");
> >
>