Re: [PATCH v4 3/3] iio: adc: Add ti-ads1110 support to ti-ads1100 driver

[Jakub Szczudło]

wt., 23 cze 2026 o 11:23 Andy Shevchenko <andriy.shevchenko@xxxxxxxxx>
napisał(a):
>
> On Tue, Jun 23, 2026 at 12:15:50AM +0200, Jakub Szczudlo wrote:
> > Add ADS1110 support that have faster datarate than ADS1100, it also uses
> > internal voltage reference of 2.048V for measurement.
>
> ...
>
> >  config TI_ADS1100
> > -     tristate "Texas Instruments ADS1100 and ADS1000 ADC"
> > +     tristate "Texas Instruments ADS1100 and similar single channel I2C ADC"
> >       depends on I2C
> >       help
> > -       If you say yes here you get support for Texas Instruments ADS1100 and
> > -       ADS1000 ADC chips.
> > +       If you say yes here you get support TI ADS1100 and similar single
> > +       channel I2C Analog to Digital Converters.
>
> User won't know what similar are really supported. The rule of thumb is to add
> the list of supported here as
>
>           - ADS1000 (...perhaps some very short spec info...)
>           - ADS1100 (...perhaps some very short spec info...)
>
>
> >         This driver can also be built as a module. If so, the module will be
> >         called ti-ads1100.
>
> ...
>
> > +static int ads1100_get_vref_milivolts(struct ads1100_data *data)
> > +{
> > +     if (data->ads_config->has_internal_vref_only)
> > +             return ADS1110_INTERNAL_REF_mV;
> > +
> > +     return regulator_get_voltage(data->reg_vdd) / MILLI;
>
> For now we used "(MICRO / MILLI)" instead of "MILLI", to show the unit
> conversion.
>
> > +}
>
> ...
>
> >       if (ret < 0) {
> >               dev_err(&data->client->dev, "I2C read fail: %d\n", ret);
> >               return ret;
>
> > +     } else if (ret < 2) {
>
> Redundant 'else'.
>
> > +             dev_err(&data->client->dev, "Short I2C read\n");
> > +             return -EIO;
> >       }
>
> ...
>
> > -     microvolts = regulator_get_voltage(data->reg_vdd);
> > +     microvolts = ads1100_get_vref_milivolts(data) * (MICRO / MILLI);
>
> See above, here you correctly used the existing pattern, the above is
> inconsistent and needs to be addressed.
>
> ...
>
> > +     model = i2c_get_match_data(client);
> > +     if (!model)
> > +             return dev_err_probe(dev, -EINVAL,
> > +                                  "Can't get device data from firmware\n");
> > +
> > +     data->ads_config = (struct ads1100_config *)model;
>
> You can't drop const like this. If you need to apply modification,
> use devm_kmemdup(). Otherwise it won't work correctly if you have two different
> sensors of the same driver in the system.
>
> --
> With Best Regards,
> Andy Shevchenko
>
>


thanks for review, I will wait few days and start working on v5

best regards,
Jakub Szczudlo