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

[Kyle Hsieh]

On Sun, Aug 23, 2026 at 8:06 AM David Lechner <dlechner@xxxxxxxxxxxx> wrote:
>
> On 8/20/26 2:51 AM, Kyle Hsieh wrote:
> > Add IIO driver support for the Texas Instruments ADS112C04 (16-bit)
> > delta-sigma ADCs.
> >
>
> ...
>
> > +static int ads112c04_parse_channels(struct iio_dev *indio_dev,
> > +                                 bool *need_avdd_ref, bool *need_ext_ref)
> > +{
> > +     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], channel;
> > +     unsigned int i;
> > +     int ret;
> > +
> > +     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 > ADS112C04_MAX_CHANNELS)
> > +             return dev_err_probe(dev, -EINVAL,
> > +                                  "num of channel nodes exceeds %d\n",
> > +                                  ADS112C04_MAX_CHANNELS);
> > +
> > +     channels = devm_kcalloc(dev, num_channels, sizeof(*channels), GFP_KERNEL);
> > +     if (!channels)
> > +             return -ENOMEM;
> > +
> > +     st->vref_source = devm_kcalloc(dev, num_channels,
> > +                                    sizeof(*st->vref_source), GFP_KERNEL);
> > +     if (!st->vref_source)
> > +             return -ENOMEM;
> > +
> > +     i = 0;
> > +     device_for_each_named_child_node_scoped(dev, child, "channel") {
> > +             const char *sp = "single-channel", *dp = "diff-channels";
>
> What does the "p" mean? I would make the names the same as the string,
> e.g. sc, dc, or s_chan, d_chan since they are used quite far from here.

Renamed to s_chan and d_chan in v7.
>
> > +             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");
> > +
> > +             st->vref_source[i] = ADS112C04_VREF_SOURCE_INTERNAL;
> > +
> > +             if (fwnode_property_present(child, "reference-sources")) {
>
> ads112c04_parse_vref_source() already calls fwnode_property_present(child, "reference-sources")
> and returns ADS112C04_VREF_SOURCE_INTERNAL as default, so we are duplicating that
> logic here. Either drop the helper or drop the extra code here.

Dropped the extra code in the loop and now call the helper directly.
>
> > +                     ret = ads112c04_parse_vref_source(child);
> > +                     if (ret < 0)
> > +                             return dev_err_probe(dev, ret,
> > +                                                  "invalid reference-sources value\n");
> > +
> > +                     st->vref_source[i] = ret;
> > +             }
> > +
> > +             if (st->vref_source[i] == ADS112C04_VREF_SOURCE_EXTERNAL)
> > +                     *need_ext_ref = true;
>
> Can just assign these directly:
>
>                 *need_ext_ref = st->vref_source[i] == ADS112C04_VREF_SOURCE_EXTERNAL;

I used |= instead of = here: the flags accumulate across all channels,
so a plain assignment would let a later channel clear what an earlier
one set. For example with channel 0 on "external" and channel 1 on
"internal", *need_ext_ref would end up false and probe would skip
reading the refp voltage.
>
> > +             if (st->vref_source[i] == ADS112C04_VREF_SOURCE_AVDD)
> > +                     *need_avdd_ref = true;
> > +
>
> ...
>
> > +
> > +             i++;
> > +     }
> > +
> > +     indio_dev->channels = channels;
> > +     indio_dev->num_channels = i;
> > +
> > +     return 0;
> > +}
> > +
> > +static int ads112c04_probe(struct i2c_client *client)
> > +{
> > +     struct device *dev = &client->dev;
> > +     struct iio_dev *indio_dev;
> > +     struct ads112c04_state *st;
> > +     struct reset_control *reset;
> > +     bool need_avdd_ref = false, need_ext_ref = false;
> > +     int ret;
> > +
>
> ...
>
> > +     /* 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) {
>
> I'm pretty sure devm_reset_control_get_optional_exclusive() already asserts
> the reset. So the way this is usally implemented is that we get (assert)
> the rest, then wait chip-specific time (usually a few microsonds) then
> reset_control_deassert().
>
> I don't see any other IIO drivers that are using reset_control_reset().
> (And I think a lot might be calling reset_control_assert() unnecissaraly.)
>

You're right - reset-gpio requests the GPIO with GPIOD_OUT_HIGH, so the
reset is already asserted by the time we get the control. Changed to
waiting tw(RSL) and then calling reset_control_deassert(). That also
makes the pulse width explicit, rather than relying on the assert and
deassert inside reset_control_reset() happening to be far enough apart.
> > +             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;
> > +     }
> > +
>
> If it wasn't for the duplicate code, I might have said good enough. :-)

Thanks,
Kyle