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

[Andy Shevchenko <andriy.shevchenko@intel.com>]

On Tue, Aug 11, 2026 at 10:48:38AM +0800, Kyle Hsieh 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.

...

> +#include <linux/bitfield.h>
> +#include <linux/bitops.h>
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/i2c.h>
> +#include <linux/interrupt.h>
> +#include <linux/iopoll.h>
> +#include <linux/jiffies.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/property.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/string.h>
> +#include <linux/types.h>
> +#include <linux/units.h>

...

> +static int ads112c04_wait_for_data(struct ads112c04_state *st)
> +{
> +	int ret, err;
> +	u8 val;
> +
> +	if (st->client->irq > 0) {
> +		/* Timeout is 100ms (slowest data rate is 20 SPS) */
> +		ret = wait_for_completion_timeout(&st->completion,
> +						  msecs_to_jiffies(100));
> +		if (!ret)

In this case semantics of ret differs, that's why it's better to write as

		if (!wait_for_completion_timeout(&st->completion, msecs_to_jiffies(100)))

// and I would even dare to put on a single line.

> +			return -ETIMEDOUT;
> +
> +		return 0;
> +	}
> +
> +	err = read_poll_timeout(ads112c04_read_reg, ret,
> +				(ret < 0 || (val & ADS112C04_CONFIG2_DRDY)),
> +				1000, 100 * USEC_PER_MSEC, false,
> +				st->client, ADS112C04_REG_CONFIG2, &val);
> +
> +	if (ret < 0)
> +		return ret;
> +
> +	return err;

In this piece I would swap err and ret, so the ret is outer one and err is
the inner one. This will be consistent with other code pieces.

> +}

...

> +static int ads112c04_get_adc_result(struct ads112c04_state *st,
> +				    struct iio_chan_spec const *chan,
> +				    int *val)
> +{
> +	u8 new_config0;
> +	int ret;
> +
> +	new_config0 = st->config0;
> +	FIELD_MODIFY(ADS112C04_CONFIG0_MUX, &new_config0, chan->address);
> +
> +	if (st->config0 != new_config0) {
> +		ret = ads112c04_write_reg(st->client, ADS112C04_REG_CONFIG0, new_config0);
> +		if (ret < 0)
> +			return ret;
> +		st->config0 = new_config0;
> +	}
> +
> +	reinit_completion(&st->completion);
> +
> +	ret = ads112c04_write_cmd(st->client, ADS112C04_CMD_START_SYNC);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = ads112c04_wait_for_data(st);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = ads112c04_read_data(st, val);

> +	if (st->client->irq > 0)
> +		enable_irq(st->client->irq);

Why is it fine to leave IRQ enabled even in the error case?

> +	return ret;
> +}

...

> +static int ads112c04_read_raw(struct iio_dev *indio_dev,
> +			      struct iio_chan_spec const *chan,
> +			      int *val, int *val2, long mask)
> +{
> +	struct ads112c04_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		mutex_lock(&st->lock);
> +		ret = ads112c04_get_adc_result(st, chan, val);
> +		mutex_unlock(&st->lock);
> +
> +		if (ret < 0)
> +			return ret;

If IRQ is left enabled and we call it here, we end up with the unbalanced
depth counting.

> +		return IIO_VAL_INT;
> +
> +	case IIO_CHAN_INFO_SCALE:
> +		*val = st->vref_mV;
> +		*val2 = 15;
> +		return IIO_VAL_FRACTIONAL_LOG2;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +}

...

> +static irqreturn_t ads112c04_irq_handler(int irq, void *private)
> +{
> +	struct iio_dev *indio_dev = private;
> +	struct ads112c04_state *st = iio_priv(indio_dev);

> +	disable_irq_nosync(irq);

This is unconditionally called. Where is the guarantee that it becomes enabled
once again?

> +	complete(&st->completion);

> +	return IRQ_HANDLED;
> +}

...

> +static int ads112c04_parse_channels(struct iio_dev *indio_dev)
> +{
> +	struct device *dev = indio_dev->dev.parent;
> +	struct ads112c04_state *st = iio_priv(indio_dev);
> +	struct iio_chan_spec *channels;
> +	u32 num_channels, pair[2];

> +	int ret, i = 0;

Why is 'i' signed? And it's better to decouple definition and assignment, so
the assignment will happen closer to when it's really needed.

...

> +		if (fwnode_property_present(child, "reference-sources")) {
> +			const char *ref;
> +
> +			ret = fwnode_property_read_string(child, "reference-sources", &ref);
> +			if (ret)
> +				return dev_err_probe(dev, ret,
> +						     "failed to read reference-sources\n");
> +
> +			if ((!strcmp(ref, "external") && !st->has_refp) ||
> +			    (!strcmp(ref, "internal") && st->has_refp))
> +				return dev_err_probe(dev, -EINVAL,
> +						     "reference-sources does not match refp-supply\n");
> +		}

Reinvention of fwnode_property_match_property_string() ?

...

> +		if (fwnode_property_present(child, "single-channel")) {
> +			ret = fwnode_property_read_u32(child, "single-channel", &pair[0]);

I don't like the (partial) pair reuse here. It's semantically wrong. Just add
another temporary variable and let compiler to choose what to do with a stack
frame in such a case.

> +			if (ret)
> +				return dev_err_probe(dev, ret,
> +						     "failed to read single-channel property\n");
> +
> +			if (pair[0] > 3)
> +				return dev_err_probe(dev, -EINVAL,
> +						     "single-channel must be 0-3\n");
> +
> +			spec->channel = pair[0];
> +			spec->address = 0x08 + pair[0];
> +		} else if (fwnode_property_present(child, "diff-channels")) {
> +			ret = fwnode_property_read_u32_array(child, "diff-channels", pair, 2);

ARRAY_SIZE()

> +			if (ret)
> +				return dev_err_probe(dev, ret,
> +						     "failed to read diff-channels property\n");
> +
> +			if (pair[0] > 3 || pair[1] > 3)
> +				return dev_err_probe(dev, -EINVAL,
> +						     "diff-channels must be 0-3\n");
> +
> +			spec->channel = pair[0];
> +			spec->channel2 = pair[1];
> +			spec->differential = 1;

> +			if (pair[0] == 0 && pair[1] == 1)
> +				spec->address = 0x00;
> +			else if (pair[0] == 0 && pair[1] == 2)
> +				spec->address = 0x01;
> +			else if (pair[0] == 0 && pair[1] == 3)
> +				spec->address = 0x02;
> +			else if (pair[0] == 1 && pair[1] == 0)
> +				spec->address = 0x03;
> +			else if (pair[0] == 1 && pair[1] == 2)
> +				spec->address = 0x04;
> +			else if (pair[0] == 1 && pair[1] == 3)
> +				spec->address = 0x05;
> +			else if (pair[0] == 2 && pair[1] == 3)
> +				spec->address = 0x06;
> +			else if (pair[0] == 3 && pair[1] == 2)
> +				spec->address = 0x07;

I would do this as a 4x4 table

	-1, 0, 1, 2,
	3, -1, 4, 5,
	-1, -1, -1, 6,
	-1, -1, 7, -1,

With that done you can even supported the swapped cases

	-1, 0, 1, 2,
	3, -1, 4, 5,
	1, 4, -1, 6,
	2, 5, 7, -1,

(but I haven't studied the code if it's toughly relies on the pair[0]/pair[1]
 values to be in a strong order after the address being assigned).

> +			else
> +				return dev_err_probe(dev, -EINVAL,
> +						     "invalid diff-channels combination\n");

> +		} else {
> +			return dev_err_probe(dev, -EINVAL,
> +					     "channel node must have single-channel or diff-channels\n");
> +		}
> +
> +		i++;
> +	}
> +
> +	indio_dev->channels = channels;
> +	indio_dev->num_channels = i;
> +
> +	return 0;
> +}

...

> +#define ADS112C04_VREF_INTERNAL_MV	2048

_mV

...

> +	if (device_property_present(dev, "refp-supply")) {

A dup property check. if (st->has_refp) should suffice, no?

> +		ret = devm_regulator_get_enable_read_voltage(dev, "refp");
> +		if (ret < 0)
> +			return dev_err_probe(dev, ret,
> +					     "failed to get refp voltage\n");
> +
> +		st->vref_mV = ret / (MICRO / MILLI);
> +		st->config1 = 0x02;
> +	} else {
> +		st->vref_mV = ADS112C04_VREF_INTERNAL_MV;
> +		st->config1 = 0x00;
> +	}

...

> +	/* Requesting OUT_HIGH asserts the active-low reset pin immediately */
> +	reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> +	if (IS_ERR(reset_gpio))
> +		return PTR_ERR(reset_gpio);

Why reset-gpio driver can't be used instead?

> +	if (reset_gpio) {
> +		fsleep(1000);

1 * USEC_PER_MSEC

> +		gpiod_set_value_cansleep(reset_gpio, 0);
> +	} else {
> +		ret = ads112c04_write_cmd(client, ADS112C04_CMD_RESET);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	fsleep(1000);

Ditto.

-- 
With Best Regards,
Andy Shevchenko