Re: [PATCH 2/4] iio: adc: add ti-ads112c14 driver

[Jonathan Cameron <jic23@kernel.org>]

On Mon, 15 Jun 2026 17:00:00 -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 few really small things inline.

> diff --git a/drivers/iio/adc/ti-ads112c14.c b/drivers/iio/adc/ti-ads112c14.c
> new file mode 100644
> index 000000000000..97097ae2a487
> --- /dev/null
> +++ b/drivers/iio/adc/ti-ads112c14.c
> @@ -0,0 +1,536 @@

> +
> +#define ADS112C14_REG_IDAC_MUX_CFG			0x0E
> +#define ADS112C14_IDAC_MUX_CFG_IUNIT			BIT(7)
> +#define ADS112C14_IDAC_MUX_CFG_I2MUX			GENMASK(6, 4)
> +#define ADS112C14_IDAC_MUX_CFG_I1MUX			GENMASK(2, 0)

Maybe tweak the indent so the register vs fields is slightly easier
to see.
#define ADS112C14_REG_IDAC_MUX_CFG			0x0E
#define   ADS112C14_IDAC_MUX_CFG_IUNIT			BIT(7)
#define   ADS112C14_IDAC_MUX_CFG_I2MUX			GENMASK(6, 4)
#define   ADS112C14_IDAC_MUX_CFG_I1MUX			GENMASK(2, 0)

And deeper still for values where appropriate.

> +static const struct reg_default ads112c14_reg_defaults[] = {
> +	{ ADS112C14_REG_DEVICE_CFG, 0x00 },
> +	{ ADS112C14_REG_DATA_RATE_CFG, 0x00 },
> +	{ ADS112C14_REG_MUX_CFG, 0x00 },
> +	{ ADS112C14_REG_GAIN_CFG, 0x01 },
> +	{ ADS112C14_REG_REFERENCE_CFG, 0x00 },
> +	{ ADS112C14_REG_DIGITAL_CFG, 0x00 },
> +	{ ADS112C14_REG_GPIO_CFG, 0x00 },
> +	{ ADS112C14_REG_GPIO_DATA_OUTPUT, 0x00 },
> +	{ ADS112C14_REG_IDAC_MAG_CFG, 0x00 },
> +	{ ADS112C14_REG_IDAC_MUX_CFG, 0x10 },

If it is plausible to define these in terms of fields that make them
up I'd prefer that.  It isn't always sensible to do so though as
they can get very messy.

> +};
>

> +
> +static int ads112c14_read_raw(struct iio_dev *indio_dev,
> +			      struct iio_chan_spec const *chan, int *val,
> +			      int *val2, long mask)
> +{
> +	struct ads112c14_data *data = iio_priv(indio_dev);
> +	u32 vref_uV, fsr_bits;
> +
> +	/* Selecting V_REF source is not implemented yet. */
> +	vref_uV = ads112c14_internal_ref_uV[ADS112C14_REFERENCE_CFG_REF_VAL_2_5V];
> +
> +	/* Currently, everything is using signed data. */
> +	fsr_bits = data->chip_info->resolution_bits - 1;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW: {
> +		u8 buf[3];
> +		int ret;
> +
> +		if (!iio_device_claim_direct(indio_dev))
> +			return -EBUSY;
> +
> +		ret = ads112c14_single_conversion(data, chan, buf);
> +		iio_device_release_direct(indio_dev);
> +		if (ret < 0)
> +			return ret;
> +
> +		switch (data->chip_info->resolution_bits) {
> +		case 16:
> +			*val = get_unaligned_be16(buf);
> +			break;
> +		case 24:
> +			*val = get_unaligned_be24(buf);
> +			break;
> +		default:
> +			return -EINVAL;
> +		}
> +
> +		*val = sign_extend32(*val, fsr_bits);
> +
> +		return IIO_VAL_INT;
> +	}
> +	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.
> +		 */
> +		*val2 = fsr_bits - (chan->address >= 3 ? 3 : 0);
> +		return IIO_VAL_FRACTIONAL_LOG2;
> +	case IIO_CHAN_INFO_OFFSET:
> +		/* Only the temperature channel has an offset. */
> +		if (chan->type != IIO_TEMP)
> +			return -EINVAL;
> +		/* Die temperature [°C] = 25°C + (Measured voltage – TS_Offset) / TS_TC */
> +		/* TS_TC (typical) = 405 uV/°C */
> +		/* TS_Offset (typical) = 119.5 mV */

Trivial but make that a multiline comment block given it is all related stuff.

> +		*val = div_s64((s64)(25 * 405 - 119500) * BIT(fsr_bits), vref_uV);
> +		return IIO_VAL_INT;
> +	default:
> +		return -EINVAL;
> +	}
> +}


> +
> +static int ads112c14_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	const struct ads112c14_chip_info *info;
> +	struct iio_dev *indio_dev;
> +	struct ads112c14_data *data;
> +	u32 reg_val;
> +	int ret;
> +
> +	info = i2c_get_match_data(client);

We currently (I think) still need to protect against a manual driver
override and that being NULL. Just error out if that happens as we
don't care about trying to support that.

> +
> +	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> +	if (!indio_dev)