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

[Jonathan Cameron <jic23@kernel.org>]

On Fri, 10 Jul 2026 17:50:39 -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>
> ---
> 
> A few other notes for review that didn't seem worth putting in the
> commit message:
> * I intentionally did not use bulk regmap because later we may need to
>   get the voltage of the avdd supply.
> * I left some comments in the code where the code might look funny (e.g.
>   to reduce future diff) or does not exactly match the datasheet, in
>   which case later changes will address that.
> 

A couple of really minor comments from a fresh read.

> diff --git a/drivers/iio/adc/ti-ads112c14.c b/drivers/iio/adc/ti-ads112c14.c
> new file mode 100644
> index 000000000000..4d2e7d37be82
> --- /dev/null
> +++ b/drivers/iio/adc/ti-ads112c14.c

...

> +static int ads112c14_read_raw(struct iio_dev *indio_dev,
> +			      struct iio_chan_spec const *chan, int *val,
> +			      int *val2, long mask)

I'd go with a logical split and bring val down a line.
Trivial however. I don't care much!

> +{
> +	struct ads112c14_data *data = iio_priv(indio_dev);

> +	case IIO_CHAN_INFO_SCALE:
> +		if (chan->type == IIO_TEMP) {
> +			/* TS_TC (typical) = 405 uV/°C */
> +			*val = MILLI * vref_uV / 405;
> +			*val2 = fsr_bits;
> +			return IIO_VAL_FRACTIONAL_LOG2;
> +		}
> +
> +		*val = vref_uV / (MICRO / MILLI);
> +		/*
> +		 * Last 3 SYS_MON channels (ext ref, AVDD, DVDD) need to be
> +		 * multiplied by 8 to account for internal attenuation of / 8.
> +		 */

I'd be tempted to make it an explicit match on those 3 channels.  A greater than
when other channels might turn up later for whatever reason seems flaky.

> +		*val2 = fsr_bits - (chan->address >= 3 ? 3 : 0);
> +		return IIO_VAL_FRACTIONAL_LOG2;

...

> +
> +static int ads112c14_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	const struct ads112c14_chip_info *info;

...
> +	/* Write magic reset value (0x16) to ensure known state. */
> +	ret = regmap_write(data->regmap, ADS112C14_REG_CONVERSION_CTRL,
> +			   FIELD_PREP(ADS112C14_CONVERSION_CTRL_RESET, 0x16));
> +	/*
> +	 * The reset may cause an -EREMOTEIO error because of failing to get the
> +	 * I2C ACK at the end of the message. The device still gets reset so it
> +	 * is safe to ignore this error.

Feels like a place where we might not always get the same error as it's surfacing
from each individual i2c controller.

I couldn't immediately spot anything in regmap or i2c function docs about this.
Where is that guarantee coming from?  In other cases we've simply not
checked regmap_write() return values at all.

> +	 */
> +	if (ret == -EREMOTEIO)
> +		ret = 0;
> +	if (ret)
> +		return ret;
> +
> +	fsleep(ADS112C14_DELAY_RESET_US);

...


> +	ret = regmap_read(data->regmap, ADS112C14_REG_DEVICE_ID, &reg_val);
> +	if (ret)
> +		return ret;
> +
> +	if (FIELD_GET(ADS112C14_DEVICE_ID_BITS, reg_val) != info->device_id)
> +		dev_info(dev, "device ID mismatch, expected 0xX%X, got 0x%lX\n",
0xX%X?  (sashiko)

> +			 info->device_id,
> +			 FIELD_GET(ADS112C14_DEVICE_ID_BITS, reg_val));
> +
> +	/* Place in single-shot conversion mode to make ready for raw read. */
Writing a bit called conv mode puts it in single-shot?  (sashiko) 

> +	ret = regmap_set_bits(data->regmap, ADS112C14_REG_DEVICE_CFG,
> +			      ADS112C14_DEVICE_CFG_CONV_MODE);

Thanks,

Jonathan