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

On Tue, Aug 11, 2026 at 10:18 PM David Lechner <[email protected]> wrote:
>
> On 8/10/26 9:48 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.
> > - Reference voltage scaling via the regulator subsystem (refp-supply),
> >   falling back to the internal 2.048V reference if not specified.
> >   refn-supply is not yet supported.
> > - Hardware reset fallback using GPIO.
> >
> > Signed-off-by: Kyle Hsieh <[email protected]>
> > ---
> >  MAINTAINERS                    |   1 +
> >  drivers/iio/adc/Kconfig        |  10 +
> >  drivers/iio/adc/Makefile       |   1 +
> >  drivers/iio/adc/ti-ads112c04.c | 425 +++++++++++++++++++++++++++++++++++++++++
> >  4 files changed, 437 insertions(+)
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 417d74b6d6cc..f51fbda9d4b9 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -26992,6 +26992,7 @@ M:    Kyle Hsieh <[email protected]>
> >  L:   [email protected]
> >  S:   Maintained
> >  F:   Documentation/devicetree/bindings/iio/adc/ti,ads112c04.yaml
> > +F:   drivers/iio/adc/ti-ads112c04.c
> >
> >  TI ADS112C14 ADC DRIVER
> >  M:   David Lechner <[email protected]>
> > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> > index 990e7b3e7212..0ee50828f9e4 100644
> > --- a/drivers/iio/adc/Kconfig
> > +++ b/drivers/iio/adc/Kconfig
> > @@ -1817,6 +1817,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_ADS112C14
> >       tristate "Texas Instruments ADS112C14/ADS122C14"
> >       depends on I2C
> > diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> > index dcec0abb03b7..d8acf2831fd2 100644
> > --- a/drivers/iio/adc/Makefile
> > +++ b/drivers/iio/adc/Makefile
> > @@ -155,6 +155,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_ADS112C14) += ti-ads112c14.o
> >  obj-$(CONFIG_TI_ADS124S08) += ti-ads124s08.o
> >  obj-$(CONFIG_TI_ADS1298) += ti-ads1298.o
> > diff --git a/drivers/iio/adc/ti-ads112c04.c b/drivers/iio/adc/ti-ads112c04.c
> > new file mode 100644
> > index 000000000000..3a775da2ae23
> > --- /dev/null
> > +++ b/drivers/iio/adc/ti-ads112c04.c
> > @@ -0,0 +1,425 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Texas Instruments ADS112C04 16-bit I2C ADC driver
> > + *
> > + * Copyright (c) 2026 Kyle Hsieh <[email protected]>
> > + *
> > + * Datasheet: https://www.ti.com/lit/ds/symlink/ads112c04.pdf
> > + * Based on TI Reference Code and standard Linux IIO framework.
> > + */
> > +
> > +#include <linux/bitfield.h>
> > +#include <linux/bitops.h>
> > +#include <linux/delay.h>
> > +#include <linux/err.h>
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/i2c.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/iopoll.h>
> > +#include <linux/jiffies.h>
> > +#include <linux/module.h>
> > +#include <linux/mutex.h>
> > +#include <linux/property.h>
> > +#include <linux/regulator/consumer.h>
> > +#include <linux/string.h>
> > +#include <linux/types.h>
> > +#include <linux/units.h>
> > +
> > +#include <linux/iio/iio.h>
> > +#include <linux/iio/sysfs.h>
> > +
> > +#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))
> > +
> > +#define ADS112C04_REG_CONFIG0       0x00
> > +#define ADS112C04_REG_CONFIG1       0x01
> > +#define ADS112C04_REG_CONFIG2       0x02
> > +#define ADS112C04_REG_CONFIG3       0x03
> > +
> > +#define ADS112C04_CONFIG0_MUX         GENMASK(7, 4)
> > +#define ADS112C04_CONFIG0_PGA_BYPASS  BIT(0)
> > +#define ADS112C04_CONFIG2_DRDY        BIT(7)
> > +
>
> I think I mentioned before the preferred way to orgainze the fields
> under the corresponding register. And as below, there are more
> fields we are using that need macros here.
>
> > +struct ads112c04_state {
> > +     struct i2c_client *client;
> > +     /* Protects concurrent ADC reads and device configuration */
> > +     struct mutex lock;
> > +     struct completion completion;
> > +     int vref_mV;
> > +     u8 config0;
> > +     u8 config1;
> > +     bool has_refp;
>
> This isn't used outside of probe. Can just be passed as a function argument.
>
> > +};
> > +
>
> ...
>
> > +static irqreturn_t ads112c04_irq_handler(int irq, void *private)
> > +{
> > +     struct iio_dev *indio_dev = private;
> > +     struct ads112c04_state *st = iio_priv(indio_dev);
> > +
> > +     disable_irq_nosync(irq);
>
> As mentioned in this recent discussion [1], we should not be disabling the IRQ.
> If there is a true need for it, we need lots of comments explaining why. Since
> we aren't doing buffered reads though, I can't see a reason why we would need
> it right now. Unless we are worried about spurious interrupts, in which case
> we should read the DRDY status bit before setting the completion here as that
> would be more robust.
>
> [1]: https://lore.kernel.org/linux-iio/20260802184126.04738251@jic23-huawei/
>
> Also, this ia a bug because we can't call syncronous code in an IRQ handler.
> We would need to change it to a threaded IRQ handler.
>
> > +     complete(&st->completion);
> > +
> > +     return IRQ_HANDLED;
> > +}
> > +
> > +static const struct iio_info ads112c04_info = {
> > +     .read_raw = ads112c04_read_raw,
> > +};
> > +
> > +static int ads112c04_parse_channels(struct iio_dev *indio_dev)
> > +{
> > +     struct device *dev = indio_dev->dev.parent;
> > +     struct ads112c04_state *st = iio_priv(indio_dev);
> > +     struct iio_chan_spec *channels;
> > +     u32 num_channels, pair[2];
> > +     int ret, i = 0;
> > +
> > +     num_channels = device_get_named_child_node_count(dev, "channel");
> > +     if (!num_channels)
> > +             return dev_err_probe(dev, -EINVAL, "no channel subnodes found\n");
> > +
> > +     if (num_channels > 12)
> > +             return dev_err_probe(dev, -EINVAL,
> > +                                  "num of channel nodes exceeds 12\n");
> > +
> > +     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];
> > +
> > +             if (fwnode_property_present(child, "excitation-channels"))
> > +                     return dev_err_probe(dev, -EOPNOTSUPP,
> > +                                          "excitation-channels is not supported yet\n");
> > +
> > +             if (fwnode_property_present(child, "reference-sources")) {
> > +                     const char *ref;
> > +
> > +                     ret = fwnode_property_read_string(child, "reference-sources", &ref);
> > +                     if (ret)
> > +                             return dev_err_probe(dev, ret,
> > +                                                  "failed to read reference-sources\n");
> > +
> > +                     if ((!strcmp(ref, "external") && !st->has_refp) ||
> > +                         (!strcmp(ref, "internal") && st->has_refp))
> > +                             return dev_err_probe(dev, -EINVAL,
> > +                                                  "reference-sources does not match refp-supply\n");
>
> As explained more below, this part needs comments to explain why it is
> the way it is since it is not implementing fully what is allowed by the
> devicetree bindings. And would return EOPNOTSUPP to be consistent that
> this is just something that is not implemented yet.
>
> > +             }
> > +
> > +             spec->type = IIO_VOLTAGE;
>
> I would add a REVISIT comment here to explain that when ti,refp-refn-resistor-ohms is implemented,
> then we have the possibility of the channel being resistance instead of votlage.
>
> > +             spec->indexed = 1;
> > +             spec->info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE);
> > +
> > +             if (fwnode_property_present(child, "single-channel")) {
> > +                     ret = fwnode_property_read_u32(child, "single-channel", &pair[0]);
> > +                     if (ret)
> > +                             return dev_err_probe(dev, ret,
> > +                                                  "failed to read single-channel property\n");
> > +
> > +                     if (pair[0] > 3)
> > +                             return dev_err_probe(dev, -EINVAL,
> > +                                                  "single-channel must be 0-3\n");
> > +
> > +                     spec->channel = pair[0];
> > +                     spec->address = 0x08 + pair[0];
> > +             } else if (fwnode_property_present(child, "diff-channels")) {
> > +                     ret = fwnode_property_read_u32_array(child, "diff-channels", pair, 2);
> > +                     if (ret)
> > +                             return dev_err_probe(dev, ret,
> > +                                                  "failed to read diff-channels property\n");
> > +
> > +                     if (pair[0] > 3 || pair[1] > 3)
> > +                             return dev_err_probe(dev, -EINVAL,
> > +                                                  "diff-channels must be 0-3\n");
> > +
> > +                     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 dev_err_probe(dev, -EINVAL,
> > +                                                  "invalid diff-channels combination\n");
> > +             } else {
> > +                     return dev_err_probe(dev, -EINVAL,
> > +                                          "channel node must have single-channel or diff-channels\n");
> > +             }
> > +
> > +             i++;
> > +     }
> > +
> > +     indio_dev->channels = channels;
> > +     indio_dev->num_channels = i;
> > +
> > +     return 0;
> > +}
> > +
> > +#define ADS112C04_VREF_INTERNAL_MV   2048
>
> Usually we put this at the top with other macro definitions.
>
> > +
> > +static int ads112c04_probe(struct i2c_client *client)
> > +{
> > +     struct device *dev = &client->dev;
> > +     struct iio_dev *indio_dev;
> > +     struct ads112c04_state *st;
> > +     struct gpio_desc *reset_gpio;
> > +     int ret;
> > +
> > +     indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
> > +     if (!indio_dev)
> > +             return -ENOMEM;
> > +
> > +     st = iio_priv(indio_dev);
> > +     st->client = client;
> > +
> > +     ret = devm_mutex_init(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;
> > +
> > +     /* Forward compatibility checks for unimplemented DT properties */
> > +     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");
> > +
> > +     st->has_refp = device_property_present(dev, "refp-supply");
> > +
> > +     ret = ads112c04_parse_channels(indio_dev);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = devm_regulator_get_enable(dev, "avdd");
> > +     if (ret)
> > +             return dev_err_probe(dev, ret, "failed to get avdd regulator\n");
> > +
> > +     ret = devm_regulator_get_enable(dev, "dvdd");
> > +     if (ret)
> > +             return dev_err_probe(dev, ret, "failed to get dvdd regulator\n");
> > +
> > +     if (device_property_present(dev, "refp-supply")) {
>
> This can just be st->has_refp now.
>
> > +             ret = devm_regulator_get_enable_read_voltage(dev, "refp");
> > +             if (ret < 0)
> > +                     return dev_err_probe(dev, ret,
> > +                                          "failed to get refp voltage\n");
> > +
> > +             st->vref_mV = ret / (MICRO / MILLI);
> > +             st->config1 = 0x02;
>
> I've asked before... these values should have macros to explain what
> they are and be using FIELD_PREP().
>
> > +     } else {
> > +             st->vref_mV = ADS112C04_VREF_INTERNAL_MV;
> > +             st->config1 = 0x00;
> > +     }
>
> I still don't think storing vref like this is a good idea since it should
> be per-channel. If you really are against implementing it in a more
> future-proof way, then it needs lots of comments explaining why it is
> implemented this way instead and how one would go about doing it the "right
> way" in the future.
>
> > +
> > +     /* Power-On Reset (POR) delay */
> > +     fsleep(50 * USEC_PER_MSEC);
> > +
> > +     /* Requesting OUT_HIGH asserts the active-low reset pin immediately */
> > +     reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> > +     if (IS_ERR(reset_gpio))
> > +             return PTR_ERR(reset_gpio);
> > +
> > +     if (reset_gpio) {
> > +             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);
> > +
> > +     /* Bypass PGA for now to allow full-scale single-ended measurements */
> > +     st->config0 = ADS112C04_CONFIG0_PGA_BYPASS;
> > +     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(dev, client->irq,
> > +                                    ads112c04_irq_handler,
> > +                                    0,
>
> I think I've asked twice now to move this 0 on the previous line.
> If you don't agree with that, it is fine, but we just ask that you
> reply to the suggestion with an explanation to state your reasoning.
> Otherwise, it comes across as carelessness that you continue to
> ignore suggestions. This is not the only suggestoin that has been
> silently ignored. You might want to go back to the previous revisions
> and see if you missed anything else.
Hi David,

Thank you for the review, and I'm sorry for the repeated oversights.

You're right, and I have no good excuse. I went back through the v1-v4
threads as you suggested and found several suggestions I silently
dropped:

- Moving the `0` in devm_request_irq() to the previous line
- Organizing register field macros indented under their register
- I even replied at the time saying I would do this, and then
  did not. Apologies.
- Using a macro and FIELD_PREP() for the CONFIG1 VREF value
- Making the reference voltage per-channel
- Using devm_reset_control_get_optional_exclusive() for the hardware
  reset (v1, and Andy raised the same point on v4)
- Your v1 suggestion to consider regmap with a register cache

All of these will be addressed in v5. For the per-channel reference
voltage, I will implement it properly following the ADS112C14 driver
rather than keeping the single stored value.

I'll also fix the IRQ handling - you're right that disabling the IRQ
was wrong, and that the handler needs to be threaded since it can't
call synchronous code. I'll drop disable_irq_nosync() and read the
DRDY status bit before completing instead.

Thanks for your patience with this series.

Best regards,
Kyle Hsieh
>
> > +                                    indio_dev->name, indio_dev);
> > +             if (ret)
> > +                     return ret;
> > +     }
> > +
> > +     return devm_iio_device_register(dev, indio_dev);
> > +}
> > +