Re: [PATCH v4 6/8] iio: adc: add ti-ads112c14 driver

[Jonathan Cameron <jonathan.cameron@oss.qualcomm.com>]

On Mon, 20 Jul 2026 00:08:05 +0100
Jonathan Cameron <jonathan.cameron@oss.qualcomm.com> wrote:

> On Tue, 14 Jul 2026 18:21:28 -0500
> "David Lechner (TI)" <dlechner@baylibre.com> wrote:
> 
> > Add a new driver for the TI ADS112C14/ADS122C14 ADC chips.
> > 
> > This first step is adding a very basic driver that only supports power
> > on/reset and reading the system monitor channels.
> > 
> > ADS112C14_SYS_MON_CHANNEL_SHORT is the last channel rather than being in
> > logical order by address to keep the voltage channels together and in
> > case we find we need to add variants of this channel with different
> > voltage reference later.
> > 
> > Signed-off-by: David Lechner (TI) <dlechner@baylibre.com>  
> I think sashiko is sending us on a wild goose chase on this one.
> 
> "When ads112c14_single_conversion() calls i2c_smbus_read_i2c_block_data(), it
> returns the number of bytes read upon success. A short read (e.g. 1 or 2 bytes)
> would return a positive value, bypassing the (ret < 0) error check in
> ads112c14_read_raw()."
> 
> An i2c_smbus_read_i2c_block_data() response doesn't contain a length
> unlike i2c_smbus_read_block_data() which does. 
> 
> Maybe there is a controller driver out there that messes with block[0]
> rather than returning an error on failure to do the read part of
> the sequence. I checked a few and didn't find one.
> 
> It might make sense to make it clear this doesn't happen by
> adding checks in the i2c core.  One to consider after the other
> ones on my list!
> 
> However, one small related comment inline about keeping postive
> return values meaning success as local as possible in the code!
> 

I'm having a perhaps optimistic go at tweaking this to resolve remaining
tiny issues and avoid need for a v5.

Tweak here is:
diff --git a/drivers/iio/adc/ti-ads112c14.c b/drivers/iio/adc/ti-ads112c14.c
index a69c595ab518..bc0fd6839b72 100644
--- a/drivers/iio/adc/ti-ads112c14.c
+++ b/drivers/iio/adc/ti-ads112c14.c
@@ -322,9 +322,13 @@ static int ads112c14_single_conversion(struct ads112c14_data *data,
        if (ret)
                return ret;
 
-       return i2c_smbus_read_i2c_block_data(client, ADS112C14_CMD_RDATA,
-                                            BITS_TO_BYTES(data->chip_info->resolution_bits),
-                                            buf);
+       ret = i2c_smbus_read_i2c_block_data(client, ADS112C14_CMD_RDATA,
+                                           BITS_TO_BYTES(data->chip_info->resolution_bits),
+                                           buf);
+       if (ret < 0)
+               return ret;
+
+       return 0;
 }
 
 static int ads112c14_read_raw(struct iio_dev *indio_dev,
@@ -355,7 +359,7 @@ static int ads112c14_read_raw(struct iio_dev *indio_dev,
                        return -EBUSY;
 
                ret = ads112c14_single_conversion(data, chan, buf);
-               if (ret < 0)
+               if (ret)
                        return ret;
 
                switch (data->chip_info->resolution_bits) {


> Jonathan
> 
> 
> 
> 
> > diff --git a/drivers/iio/adc/ti-ads112c14.c b/drivers/iio/adc/ti-ads112c14.c
> > new file mode 100644
> > index 000000000000..a69c595ab518
> > --- /dev/null
> > +++ b/drivers/iio/adc/ti-ads112c14.c  
> 
> ...
> 
> > +
> > +static int ads112c14_single_conversion(struct ads112c14_data *data,
> > +				       const struct iio_chan_spec *chan,
> > +				       u8 *buf)
> > +{
> > +	struct i2c_client *client = to_i2c_client(regmap_get_device(data->regmap));
> > +	u32 reg_val;
> > +	int ret;
> > +
> > +	if (chan->channel < ADS112C14_SYS_MON_CHANNEL_BASE) {
> > +		/* Not implemented yet. */
> > +		return -EINVAL;
> > +	} else {
> > +		ret = ads112c14_prepare_sys_mon_channel(data, chan);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	ret = regmap_write(data->regmap, ADS112C14_REG_CONVERSION_CTRL,
> > +			   ADS112C14_CONVERSION_CTRL_START);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = regmap_read_poll_timeout(data->regmap,
> > +				       ADS112C14_REG_STATUS_MSB, reg_val,
> > +				       FIELD_GET(ADS112C14_STATUS_MSB_DRDY, reg_val),
> > +				       1 * USEC_PER_MSEC, 100 * USEC_PER_MSEC);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return i2c_smbus_read_i2c_block_data(client, ADS112C14_CMD_RDATA,
> > +					     BITS_TO_BYTES(data->chip_info->resolution_bits),
> > +					     buf);  
> With all that stuff above about this not returning short, I'd still be tempted to do
> an if (ret < 0) return ret; return 0; sequence in here so we don't propogate
> confusing positive returns beyond where we can see their source.
> 
> Having done that make the outer check if (ret) 
> 
> 
> > +}  
>