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

[Jonathan Cameron <jic23@kernel.org>]

On Mon, 8 Jun 2026 09:39:07 +0200
Joshua Crofts <joshua.crofts1@gmail.com> wrote:

> Hi Jakub,
> 
> various comments inline, some nitpicks and some more serious.
> 
> Josh
> 
> On Sun, 7 Jun 2026 at 20:38, Jakub Szczudlo <jakubszczudlo40@gmail.com> wrote:
> >
> > From: jszczudlo <jakubszczudlo40@gmail.com>
> >
> > add ADS1100 support  
> 
> Wrap the commit message to 72 characters per line, this is too
> short.
> 
> > make changing gain and datarate wait for new reading
> > fix unbalanced regulator disable when removing in singleshot mode  
> 
> Additionally, write the commit messages as regular sentences, not
> a list of changes.
> 
> >
> > Signed-off-by: jszczudlo <jakubszczudlo40@gmail.com>  
> 
> Ensure that your full name is in the Signed-off-by tag (this goes for all
> patches in this series).

A few follow ups to the good review you already have from Joshua.

> > @@ -85,6 +98,50 @@ static int ads1100_set_config_bits(struct ads1100_data *data, u8 mask, u8 value)
> >         return 0;
> >  };
> >
> > +static int ads11x0_get_voltage_microvolts(struct ads1100_data *data)
> > +{
> > +       if (data->has_reference_voltage)
> > +               return ADS1110_REFERENCE_VOLTAGE_MICROVOLT;
> > +       else
> > +               return regulator_get_voltage(data->reg_vdd);
> > +}
> > +
> > +static int ads11x0_get_voltage_milivolts(struct ads1100_data *data)
> > +{
> > +       return ads11x0_get_voltage_microvolts(data) / (MICRO / MILLI);
> > +}
> > +
> > +static bool ads11x0_new_data_ready(struct ads1100_data *data)
> > +{
> > +       int ret;
> > +       u8 buffer[3];
> > +
> > +       ret = i2c_master_recv(data->client, (char *)&buffer, sizeof(buffer));
> > +       if (ret < sizeof(buffer)) {  
> 
> Sashiko raises an issue here. sizeof returns a size_t, therefore the compiler
> will promote ret to a size_t, wrapping any potential negative error value to
> a large positive value, throwing away the error.
> 
> > +               dev_err(&data->client->dev, "I2C read fail: %d\n", ret);
> > +               return 0;
It gets thrown away here anyway which is also very wrong!  If an error occurs
it should be propagated. If it makes sense to ignore it, do that at the
caller and add a comment on why.
> > +       }
> > +
> > +       int return_val = FIELD_GET(ADS1100_CFG_ST_BSY, buffer[2]);

When there isn't a good reason to do otherwise we still flow oldstyle c
where local variable declarations come at the top of scope.
However

	return FIELD_GET();
should be fine.

> > +
> > +       return return_val;
> > +}


> >  static void ads1100_disable_continuous(void *data)
> > @@ -307,6 +383,7 @@ static int ads1100_probe(struct i2c_client *client)
> >         struct iio_dev *indio_dev;
> >         struct ads1100_data *data;
> >         struct device *dev = &client->dev;
> > +       const struct ads11x0_config *model;
> >         int ret;
> >
> >         indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> > @@ -334,10 +411,18 @@ static int ads1100_probe(struct i2c_client *client)
> >                 return dev_err_probe(dev, ret,
> >                                      "Failed to enable vdd regulator\n");
> >
> > -       ret = devm_add_action_or_reset(dev, ads1100_reg_disable, data->reg_vdd);
> > +       ret = devm_add_action_or_reset(dev, ads1100_reg_disable, data);
> >         if (ret)
> >                 return ret;
> >
> > +       model = device_get_match_data(dev);
> > +       if (!model)
> > +               return dev_err_probe(dev, ret,
> > +                               "Can't set device data\n");  
> 
> Hmm, if device_get_match_data fails, ret will still be 0 per previous
> devm_add_action_or_reset() call, therefore you're returning a "successful
> error". Additionally, the error message isn't aligned with the parenthesis.

Message also talks about 'setting' when it is 'getting' data from firmware.
So needs a rewrite.

Jonathan