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

On 8/20/26 2:51 AM, Kyle Hsieh wrote:
> Add IIO driver support for the Texas Instruments ADS112C04 (16-bit)
> delta-sigma ADCs.
> 

...

> +static int ads112c04_parse_channels(struct iio_dev *indio_dev,
> +				    bool *need_avdd_ref, bool *need_ext_ref)
> +{
> +	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], channel;
> +	unsigned int i;
> +	int ret;
> +
> +	num_channels = device_get_named_child_node_count(dev, "channel");
> +	if (!num_channels)
> +		return dev_err_probe(dev, -EINVAL, "no channel subnodes found\n");
> +
> +	if (num_channels > ADS112C04_MAX_CHANNELS)
> +		return dev_err_probe(dev, -EINVAL,
> +				     "num of channel nodes exceeds %d\n",
> +				     ADS112C04_MAX_CHANNELS);
> +
> +	channels = devm_kcalloc(dev, num_channels, sizeof(*channels), GFP_KERNEL);
> +	if (!channels)
> +		return -ENOMEM;
> +
> +	st->vref_source = devm_kcalloc(dev, num_channels,
> +				       sizeof(*st->vref_source), GFP_KERNEL);
> +	if (!st->vref_source)
> +		return -ENOMEM;
> +
> +	i = 0;
> +	device_for_each_named_child_node_scoped(dev, child, "channel") {
> +		const char *sp = "single-channel", *dp = "diff-channels";

What does the "p" mean? I would make the names the same as the string,
e.g. sc, dc, or s_chan, d_chan since they are used quite far from here.

> +		struct iio_chan_spec *spec = &channels[i];
> +
> +		if (fwnode_property_present(child, "excitation-channels"))
> +			return dev_err_probe(dev, -EOPNOTSUPP,
> +					     "excitation-channels is not supported yet\n");
> +
> +		st->vref_source[i] = ADS112C04_VREF_SOURCE_INTERNAL;
> +
> +		if (fwnode_property_present(child, "reference-sources")) {

ads112c04_parse_vref_source() already calls fwnode_property_present(child, "reference-sources")
and returns ADS112C04_VREF_SOURCE_INTERNAL as default, so we are duplicating that
logic here. Either drop the helper or drop the extra code here.

> +			ret = ads112c04_parse_vref_source(child);
> +			if (ret < 0)
> +				return dev_err_probe(dev, ret,
> +						     "invalid reference-sources value\n");
> +
> +			st->vref_source[i] = ret;
> +		}
> +
> +		if (st->vref_source[i] == ADS112C04_VREF_SOURCE_EXTERNAL)
> +			*need_ext_ref = true;

Can just assign these directly:

		*need_ext_ref = st->vref_source[i] == ADS112C04_VREF_SOURCE_EXTERNAL;

> +		if (st->vref_source[i] == ADS112C04_VREF_SOURCE_AVDD)
> +			*need_avdd_ref = true;
> +

...

> +
> +		i++;
> +	}
> +
> +	indio_dev->channels = channels;
> +	indio_dev->num_channels = i;
> +
> +	return 0;
> +}
> +
> +static int ads112c04_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	struct iio_dev *indio_dev;
> +	struct ads112c04_state *st;
> +	struct reset_control *reset;
> +	bool need_avdd_ref = false, need_ext_ref = false;
> +	int ret;
> +

...

> +	/* 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) {

I'm pretty sure devm_reset_control_get_optional_exclusive() already asserts
the reset. So the way this is usally implemented is that we get (assert)
the rest, then wait chip-specific time (usually a few microsonds) then
reset_control_deassert().

I don't see any other IIO drivers that are using reset_control_reset().
(And I think a lot might be calling reset_control_assert() unnecissaraly.)

> +		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;
> +	}
> +

If it wasn't for the duplicate code, I might have said good enough. :-)