Re: [PATCH v4 2/2] iio: adc: ti-ads112c04: Add support for TI ADS112C04

[Jonathan Cameron <jic23@kernel.org>]

On Tue, 11 Aug 2026 10:48:38 +0800
Kyle Hsieh <kylehsieh1995@gmail.com> wrote:

> Add IIO driver support for the Texas Instruments ADS112C04 (16-bit)
> delta-sigma ADCs.
> 
> The driver implements:
> - Single-shot conversions using the IIO raw read interface.
> - Dynamic parsing of single-ended and differential channels from
>   device tree child nodes.
> - Hardware interrupt support via the DRDY pin, falling back to
>   software polling if no IRQ is provided.
> - Scale calculation based on the internal 2.048V reference.
> - Reference voltage scaling via the regulator subsystem (refp-supply),
>   falling back to the internal 2.048V reference if not specified.
>   refn-supply is not yet supported.
> - Hardware reset fallback using GPIO.
> 
> Signed-off-by: Kyle Hsieh <kylehsieh1995@gmail.com>
Hi Kyle,

Clearly you got plenty of good feedback already from Andy and David.
As such (and given I have 400 IIO messages unread right now :()
I only took a quick look and called out a few things that perhaps others have not
already raised.

Jonathan

> +
> +#define ADS112C04_VREF_INTERNAL_MV	2048

Probably better up top with the other defines.

> +
> +static int ads112c04_probe(struct i2c_client *client)
> +{

...

> +
> +	/* Bypass PGA for now to allow full-scale single-ended measurements */
Given there are several fields in each of these registers, it would be
nicer to fully fill it here (even though they all end up as zeros)

	st->config0 = ADS112C04_CONFIG0_PGA_BYPASS |
		      FIELD_PUT(ADS112C04_CONFIG0_GAIN, ADS112C04_CONFIG0_GAIN_X1) |
		      FIELD_PUT(ADS112C04_CONFIG0_MUX, ADS112C04_CONFIG0_MUX_AIN0_AIN1);

	st->config1 = FIELD_PUT(ADIS112C04_CONFIG1_TEMP_EN, 0) |
		...
		      FIELD_PUT(ADIS112C04_CONFIG1_CONVMODE, ADIS112C04_CONFIG1_CONVMODE_SINGLE)
etc
Long lines so maybe shorten CONFIG to CONF or CFG to help with that.

The aim of this explicit form is to make it obvious what the defaults actually mean.
That is lost if you write a 0 as done to config1.

> +	st->config0 = ADS112C04_CONFIG0_PGA_BYPASS;
> +	ret = ads112c04_write_reg(client, ADS112C04_REG_CONFIG0, st->config0);
> +	if (ret)
> +		return ret;
> +
> +	ret = ads112c04_write_reg(client, ADS112C04_REG_CONFIG1, st->config1);
> +	if (ret)
> +		return ret;