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

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

On Thu, Aug 13, 2026 at 11:06:03AM +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.
> - Per-channel reference source selection (internal 2.048V, external
>   REFP/REFN, or AVDD) via the reference-sources device tree property.
>   refn-supply is not yet supported.
> - Hardware reset via the reset controller framework, falling back to
>   the RESET command when no reset controller is present.

...

> +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) */
> +		if (!wait_for_completion_timeout(&st->completion, msecs_to_jiffies(100)))
> +			return -ETIMEDOUT;
> +
> +		return 0;
> +	}
> +
> +	ret = read_poll_timeout(ads112c04_read_reg, err,
> +				(err < 0 || (val & ADS112C04_CONF2_DRDY)),

Better to split logically, also the outer parentheses are redundant.

	ret = read_poll_timeout(ads112c04_read_reg,
				err, err < 0 || (val & ADS112C04_CONF2_DRDY),

> +				1 * USEC_PER_MSEC, 100 * USEC_PER_MSEC, false,
> +				st->client, ADS112C04_REG_CONFIG2, &val);
> +	if (err < 0)
> +		return err;
> +
> +	return ret;
> +}

...

> +	case IIO_CHAN_INFO_SCALE:
> +		switch (st->vref_source[idx]) {
> +		case ADS112C04_VREF_SOURCE_EXTERNAL:
> +			*val = st->ext_ref_mV;
> +			break;
> +		case ADS112C04_VREF_SOURCE_AVDD:
> +			*val = st->avdd_mV;
> +			break;
> +		default:
> +			*val = ADS112C04_INT_REF_mV;
> +			break;
> +		}
> +		*val2 = 15;

Seems like this being used in one of the above functions already. Perhaps you
want a defined constant? (I haven't checked if that 15 and this one are
semantically related, though.)

> +		return IIO_VAL_FRACTIONAL_LOG2;

...

With

	const char *sp = "single-channel", *dp = "diff-channels";

The below...

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

+ array_size.h

> +			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 (ads112c04_diff_mux[pair[0]][pair[1]] < 0)
> +				return dev_err_probe(dev, -EINVAL,
> +						     "invalid diff-channels combination\n");
> +
> +			spec->address = ads112c04_diff_mux[pair[0]][pair[1]];
> +		} else {
> +			return dev_err_probe(dev, -EINVAL,
> +					     "channel node must have single-channel or diff-channels\n");
> +		}

...can be written as

		if (fwnode_property_present(child, sp)) {
			ret = fwnode_property_read_u32(child, sp, &channel);
			if (ret)
				return dev_err_probe(dev, ret, "failed to read %s property\n", sp);

			if (channel > 3)
				return dev_err_probe(dev, -EINVAL, "%s must be 0-3\n", sp);

			spec->channel = channel;
			spec->address = ADS112C04_CONF0_MUX_AIN_SINGLE_BASE + channel;
		} else if (fwnode_property_present(child, dp)) {
			ret = fwnode_property_read_u32_array(child, dp, pair, ARRAY_SIZE(pair));
			if (ret)
				return dev_err_probe(dev, ret, "failed to read %s property\n", dp);

			if (pair[0] > 3 || pair[1] > 3)
				return dev_err_probe(dev, -EINVAL, "%s must be 0-3\n", dp);

			spec->channel = pair[0];
			spec->channel2 = pair[1];
			spec->differential = 1;

			if (ads112c04_diff_mux[pair[0]][pair[1]] < 0)
				return dev_err_probe(dev, -EINVAL, "invalid %s combination\n", dp);

			spec->address = ads112c04_diff_mux[pair[0]][pair[1]];
		} else {
			return dev_err_probe(dev, -EINVAL,
					     "channel node must have %s or %s\n", sp, dp);
		}

(but it also makes sense to check with bloat-o-meter to see how much code is
 added and how much data space is saved).

...

> +	/* Datasheet: POR releases ~500us after supplies are stable */
> +	fsleep(500);
> +
> +	reset = devm_reset_control_get_optional_exclusive(dev, NULL);
> +	if (IS_ERR(reset))
> +		return dev_err_probe(dev, PTR_ERR(reset), "failed to get reset\n");
> +
> +	if (reset) {
> +		ret = reset_control_reset(reset);
> +		if (ret)
> +			return dev_err_probe(dev, ret, "failed to reset device\n");
> +	} else {
> +		ret = ads112c04_write_cmd(client, ADS112C04_CMD_RESET);
> +		if (ret < 0)
> +			return ret;
> +	}

Also a comment here?

> +	fsleep(1 * USEC_PER_MSEC);

-- 
With Best Regards,
Andy Shevchenko