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

Hi Andy,

Thanks for the detailed review.

On Tue, Aug 11, 2026 at 5:39 PM Andy Shevchenko
<[email protected]> wrote:
>
> On Tue, Aug 11, 2026 at 10:48:38AM +0800, 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.
>
> ...
>
> > +#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>
>
> ...
>
> > +static int ads112c04_wait_for_data(struct ads112c04_state *st)
> > +{
> > +     int ret, err;
> > +     u8 val;
> > +
> > +     if (st->client->irq > 0) {
> > +             /* Timeout is 100ms (slowest data rate is 20 SPS) */
> > +             ret = wait_for_completion_timeout(&st->completion,
> > +                                               msecs_to_jiffies(100));
> > +             if (!ret)
>
> In this case semantics of ret differs, that's why it's better to write as
>
>                 if (!wait_for_completion_timeout(&st->completion, msecs_to_jiffies(100)))
>
> // and I would even dare to put on a single line.
>
> > +                     return -ETIMEDOUT;
> > +
> > +             return 0;
> > +     }
> > +
> > +     err = read_poll_timeout(ads112c04_read_reg, ret,
> > +                             (ret < 0 || (val & ADS112C04_CONFIG2_DRDY)),
> > +                             1000, 100 * USEC_PER_MSEC, false,
> > +                             st->client, ADS112C04_REG_CONFIG2, &val);
> > +
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     return err;
>
> In this piece I would swap err and ret, so the ret is outer one and err is
> the inner one. This will be consistent with other code pieces.
>
> > +}
>
> ...
>
> > +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;
> > +     FIELD_MODIFY(ADS112C04_CONFIG0_MUX, &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;
> > +
> > +     ret = ads112c04_read_data(st, val);
>
> > +     if (st->client->irq > 0)
> > +             enable_irq(st->client->irq);
>
> Why is it fine to leave IRQ enabled even in the error case?
>
> > +     return ret;
> > +}
>
> ...
>
> > +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;
>
> If IRQ is left enabled and we call it here, we end up with the unbalanced
> depth counting.
>
> > +             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);
>
> > +     disable_irq_nosync(irq);
>
> This is unconditionally called. Where is the guarantee that it becomes enabled
> once again?
>
> > +     complete(&st->completion);
>
> > +     return IRQ_HANDLED;
> > +}
>
> ...
>
> > +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;
>
> Why is 'i' signed? And it's better to decouple definition and assignment, so
> the assignment will happen closer to when it's really needed.
>
> ...
>
> > +             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");
> > +             }
>
> Reinvention of fwnode_property_match_property_string() ?
>
> ...
>
> > +             if (fwnode_property_present(child, "single-channel")) {
> > +                     ret = fwnode_property_read_u32(child, "single-channel", &pair[0]);
>
> I don't like the (partial) pair reuse here. It's semantically wrong. Just add
> another temporary variable and let compiler to choose what to do with a stack
> frame in such a case.
>
> > +                     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);
>
> ARRAY_SIZE()
>
> > +                     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;
>
> I would do this as a 4x4 table
>
>         -1, 0, 1, 2,
>         3, -1, 4, 5,
>         -1, -1, -1, 6,
>         -1, -1, 7, -1,
>
> With that done you can even supported the swapped cases
>
>         -1, 0, 1, 2,
>         3, -1, 4, 5,
>         1, 4, -1, 6,
>         2, 5, 7, -1,
>
> (but I haven't studied the code if it's toughly relies on the pair[0]/pair[1]
>  values to be in a strong order after the address being assigned).
I will apply your first 4x4 table for diff-channels - it maps directly to
the MUX encoding and is much more readable than the if-else chain.

I did not apply the second (swapped) table. Entries like [2][0] would
map to MUX value 1, which is AINP=AIN0/AINN=AIN2, i.e. the opposite
polarity, so it would silently return negated values. Supporting swapped
pairs properly would need a per-channel inversion flag and negation
after conversion, which I'd rather leave out of this initial minimal
implementation.
>
> > +                     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
>
> _mV
>
> ...
>
> > +     if (device_property_present(dev, "refp-supply")) {
>
> A dup property check. if (st->has_refp) should suffice, no?
>
> > +             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;
> > +     } else {
> > +             st->vref_mV = ADS112C04_VREF_INTERNAL_MV;
> > +             st->config1 = 0x00;
> > +     }
>
> ...
>
> > +     /* 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);
>
> Why reset-gpio driver can't be used instead?
David suggested the reset controller framework on v1, but after I moved
to gpiod in v2 he suggested GPIOD_OUT_HIGH to simplify it, so I kept
gpiod. Switching now would mean changing the binding, which already has
a Reviewed-by - happy to do it if you both prefer.

Best regards,
Kyle Hsieh
>
> > +     if (reset_gpio) {
> > +             fsleep(1000);
>
> 1 * USEC_PER_MSEC
>
> > +             gpiod_set_value_cansleep(reset_gpio, 0);
> > +     } else {
> > +             ret = ads112c04_write_cmd(client, ADS112C04_CMD_RESET);
> > +             if (ret < 0)
> > +                     return ret;
> > +     }
> > +
> > +     fsleep(1000);
>
> Ditto.
>
> --
> With Best Regards,
> Andy Shevchenko
>
>