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

On Mon, Aug 17, 2026 at 3:12 PM Andy Shevchenko
<[email protected]> wrote:
>
> On Thu, Aug 13, 2026 at 11:06:03AM +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.
> > - Per-channel reference source selection (internal 2.048V, external
> >   REFP/REFN, or AVDD) via the reference-sources device tree property.
> >   refn-supply is not yet supported.
> > - Hardware reset via the reset controller framework, falling back to
> >   the RESET command when no reset controller is present.
>
> ...
>
> > +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) */
> > +             if (!wait_for_completion_timeout(&st->completion, msecs_to_jiffies(100)))
> > +                     return -ETIMEDOUT;
> > +
> > +             return 0;
> > +     }
> > +
> > +     ret = read_poll_timeout(ads112c04_read_reg, err,
> > +                             (err < 0 || (val & ADS112C04_CONF2_DRDY)),
>
> Better to split logically, also the outer parentheses are redundant.

Fixed in v6.
>
>         ret = read_poll_timeout(ads112c04_read_reg,
>                                 err, err < 0 || (val & ADS112C04_CONF2_DRDY),
>
> > +                             1 * USEC_PER_MSEC, 100 * USEC_PER_MSEC, false,
> > +                             st->client, ADS112C04_REG_CONFIG2, &val);
> > +     if (err < 0)
> > +             return err;
> > +
> > +     return ret;
> > +}
>
> ...
>
> > +     case IIO_CHAN_INFO_SCALE:
> > +             switch (st->vref_source[idx]) {
> > +             case ADS112C04_VREF_SOURCE_EXTERNAL:
> > +                     *val = st->ext_ref_mV;
> > +                     break;
> > +             case ADS112C04_VREF_SOURCE_AVDD:
> > +                     *val = st->avdd_mV;
> > +                     break;
> > +             default:
> > +                     *val = ADS112C04_INT_REF_mV;
> > +                     break;
> > +             }
> > +             *val2 = 15;
>
> Seems like this being used in one of the above functions already. Perhaps you
> want a defined constant? (I haven't checked if that 15 and this one are
> semantically related, though.)

They are - both come from the 16-bit signed conversion result. Added
ADS112C04_RESOLUTION_BITS and used RESOLUTION_BITS - 1 in both places.
>
> > +             return IIO_VAL_FRACTIONAL_LOG2;
>
> ...
>
> With
>
>         const char *sp = "single-channel", *dp = "diff-channels";
>
> The below...
>
> > +             if (fwnode_property_present(child, "single-channel")) {
> > +                     ret = fwnode_property_read_u32(child, "single-channel", &channel);
> > +                     if (ret)
> > +                             return dev_err_probe(dev, ret,
> > +                                                  "failed to read single-channel property\n");
> > +
> > +                     if (channel > 3)
> > +                             return dev_err_probe(dev, -EINVAL,
> > +                                                  "single-channel must be 0-3\n");
> > +
> > +                     spec->channel = channel;
> > +                     spec->address = ADS112C04_CONF0_MUX_AIN_SINGLE_BASE + channel;
> > +             } else if (fwnode_property_present(child, "diff-channels")) {
> > +                     ret = fwnode_property_read_u32_array(child, "diff-channels",
> > +                                                          pair, ARRAY_SIZE(pair));
>
> + array_size.h

Added.
>
> > +                     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 (ads112c04_diff_mux[pair[0]][pair[1]] < 0)
> > +                             return dev_err_probe(dev, -EINVAL,
> > +                                                  "invalid diff-channels combination\n");
> > +
> > +                     spec->address = ads112c04_diff_mux[pair[0]][pair[1]];
> > +             } else {
> > +                     return dev_err_probe(dev, -EINVAL,
> > +                                          "channel node must have single-channel or diff-channels\n");
> > +             }
>
> ...can be written as
>
>                 if (fwnode_property_present(child, sp)) {
>                         ret = fwnode_property_read_u32(child, sp, &channel);
>                         if (ret)
>                                 return dev_err_probe(dev, ret, "failed to read %s property\n", sp);
>
>                         if (channel > 3)
>                                 return dev_err_probe(dev, -EINVAL, "%s must be 0-3\n", sp);
>
>                         spec->channel = channel;
>                         spec->address = ADS112C04_CONF0_MUX_AIN_SINGLE_BASE + channel;
>                 } else if (fwnode_property_present(child, dp)) {
>                         ret = fwnode_property_read_u32_array(child, dp, pair, ARRAY_SIZE(pair));
>                         if (ret)
>                                 return dev_err_probe(dev, ret, "failed to read %s property\n", dp);
>
>                         if (pair[0] > 3 || pair[1] > 3)
>                                 return dev_err_probe(dev, -EINVAL, "%s must be 0-3\n", dp);
>
>                         spec->channel = pair[0];
>                         spec->channel2 = pair[1];
>                         spec->differential = 1;
>
>                         if (ads112c04_diff_mux[pair[0]][pair[1]] < 0)
>                                 return dev_err_probe(dev, -EINVAL, "invalid %s combination\n", dp);
>
>                         spec->address = ads112c04_diff_mux[pair[0]][pair[1]];
>                 } else {
>                         return dev_err_probe(dev, -EINVAL,
>                                              "channel node must have %s or %s\n", sp, dp);
>                 }
>
> (but it also makes sense to check with bloat-o-meter to see how much code is
>  added and how much data space is saved).
>

Applied - it also keeps the error messages in sync with the property
names automatically.
> ...
>
> > +     /* Datasheet: POR releases ~500us after supplies are stable */
> > +     fsleep(500);
> > +
> > +     reset = devm_reset_control_get_optional_exclusive(dev, NULL);
> > +     if (IS_ERR(reset))
> > +             return dev_err_probe(dev, PTR_ERR(reset), "failed to get reset\n");
> > +
> > +     if (reset) {
> > +             ret = reset_control_reset(reset);
> > +             if (ret)
> > +                     return dev_err_probe(dev, ret, "failed to reset device\n");
> > +     } else {
> > +             ret = ads112c04_write_cmd(client, ADS112C04_CMD_RESET);
> > +             if (ret < 0)
> > +                     return ret;
> > +     }
>
> Also a comment here?
>
> > +     fsleep(1 * USEC_PER_MSEC);
>

Added.

Thanks,
Kyle Hsieh