Re: [PATCH 4/4] iio: adc: ti-ads112c14: add measurement channel support

[David Lechner <dlechner@baylibre.com>]

On 6/16/26 3:36 AM, Andy Shevchenko wrote:
> On Mon, Jun 15, 2026 at 05:00:02PM -0500, David Lechner (TI) wrote:
>> Add support for parsing devicetree properties for measurement channels
>> and doing direct reads on these.
>>
>> There are quite a lot of conditions that have to be met for each
>> measurement to be made, so quite a bit of state and algorithms are
>> required to handle it.
>>
>> Channels are created dynamically since the number of possibilities is
>> unreasonably large.
> 


>> +	/* measurement channels */
>> +	if (chan->channel < 100) {
>> +		struct ads112c14_measurement *measurement =
>> +			&data->measurements[chan->scan_index];
> 
>> +		if (!measurement->label)
>> +			return -EINVAL;
> 
> Hmm... Can it be true?

Yes. For some channels, label comes from the devicetree, which
may not have provided a label.

> 
>> +		return sysfs_emit(label, "%s\n", measurement->label);
>> +	}
> 

...

>> +		if (fwnode_property_present(child, "single-channel")) {
>> +			ret = fwnode_property_read_u32(child, "single-channel", &spec->channel);
>> +			if (ret)
>> +				return dev_err_probe(dev, ret,
>> +						     "failed to read single-channel property\n");
>> +
>> +			if (spec->channel >= 8)
>> +				return dev_err_probe(dev, -EINVAL,
>> +						     "single-channel value must be between 0 and 7\n");
>> +		} else if (fwnode_property_present(child, "diff-channels")) {
>> +			ret = fwnode_property_read_u32_array(child, "diff-channels", pair, ARRAY_SIZE(pair));
>> +			if (ret)
>> +				return dev_err_probe(dev, ret,
>> +						     "failed to read diff-channels property\n");
>> +
>> +			if (pair[0] >= 8 || pair[1] >= 8)
>> +				return dev_err_probe(dev, -EINVAL,
>> +						     "diff-channels values must be between 0 and 7\n");
>> +
>> +			spec->channel = pair[0];
>> +			spec->channel2 = pair[1];
>> +			spec->differential = 1;
>> +		} else {
>> +			return dev_err_probe(dev, -EINVAL,
>> +					     "channel node missing channel type property\n");
>> +		}
> 
> Looking how it's going to spread (I mean the above pattern), perhaps it's a time to introduce bunch of
> 
> 	fwnode_property_read_*_optional()
> 
> and the respective device_property_read_*_optional()?
> 
> Let's start from u32 case only, as it will be most used anyway.

I don't think that would be really any different from device_property_read_*
and checking for -EINVAL or ignoring the error completely. TBH, I really like
it this way with fwnode_property_present().

> 
>> +		if (fwnode_property_present(child, "excitation-channels")) {
>> +			ret = fwnode_property_count_u32(child, "excitation-channels");
>> +			if (ret < 0)
>> +				return dev_err_probe(dev, ret,
>> +						     "failed to read excitation-channels property\n");
>> +
>> +			if (ret < 1 || ret > 2)
>> +				return dev_err_probe(dev, -EINVAL,
>> +						     "excitation-channels property must have 1 or 2 values\n");
>> +
>> +			measurement->iadc_count = ret;
>> +			pair[1] = 0;
>> +
>> +			ret = fwnode_property_read_u32_array(child, "excitation-channels", pair, measurement->iadc_count);
>> +			if (ret)
>> +				return dev_err_probe(dev, ret,
>> +						     "failed to read excitation-channels property\n");
>> +
>> +			if (pair[0] >= 8 || pair[1] >= 8)
>> +				return dev_err_probe(dev, -EINVAL,
>> +						     "excitation-channels values must be between 0 and 7\n");
>> +
>> +			measurement->idac1_mux = pair[0];
>> +			measurement->idac2_mux = measurement->iadc_count > 1 ? pair[1] : 0;
>> +
>> +			ret = fwnode_property_read_u32(child, "excitation-current-microamp",
>> +						       &measurement->idac_current_uA);
>> +			if (ret)
>> +				return dev_err_probe(dev, ret,
>> +						     "failed to read excitation-current-microamp property\n");
>> +
>> +			measurement->current_chop = fwnode_property_read_bool(child, "current-chopping");
>> +		}
>> +
>> +		measurement->bipolar = fwnode_property_read_bool(child, "bipolar");
>> +
>> +		fwnode_property_read_u32(child, "ti,vref-source", &measurement->vref_source);
>> +		if (measurement->vref_source > ADS112C14_VREF_SOURCE_AVDD)
>> +			return dev_err_probe(dev, -EINVAL,
>> +					     "invalid vref-source value\n");
>> +
>> +		if (measurement->vref_source == ADS112C14_VREF_SOURCE_AVDD)
>> +			*need_avdd_ref = true;
>> +		if (measurement->vref_source == ADS112C14_VREF_SOURCE_EXTERNAL)
>> +			*need_ext_ref = true;
>> +
>> +		spec->info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE);
>> +		spec->info_mask_separate_available = BIT(IIO_CHAN_INFO_SCALE);
>> +
>> +		i++;
>> +	}
>> +
>> +	memcpy(channels + i, ads112c14_sys_mon_channels, sizeof(ads112c14_sys_mon_channels));
>> +
>> +	indio_dev->channels = channels;
>> +	indio_dev->num_channels = i + ARRAY_SIZE(ads112c14_sys_mon_channels);
>> +
>> +	return 0;
>> +}
> 
> ...
> 
>> +static void ads112c14_populate_scale_available(int scale_avail[][2],
>> +					       u32 vref_uV, u32 fsr_bits)
>> +{
>> +	for (u32 i = 0; i < ARRAY_SIZE(ads112c14_pga_gains_x10); i++) {
>> +		int *entry = scale_avail[i];
>> +		u32 gain_x10 = ads112c14_pga_gains_x10[i];
>> +
>> +		entry[0] = div_u64_rem(div64_u64((u64)(NANO * 10 /
>> +						       (MICRO / MILLI)) * vref_uV,
>> +						 (u64)gain_x10 * BIT(fsr_bits)),
> 
> Hmm... This differs from the previous implementation. Why?

Probably fixed it during testing and missed that I needed to fix
the original patch too.

> 
>> +				       NANO, &entry[1]);
>> +	}
>> +}
> 
> ...
> 
>> +	if (device_property_present(dev, "refp-refn-resistor-ohms")) {
>> +		if (refp_uV != 0 || refn_uV != 0)
>> +			return dev_err_probe(dev, -EINVAL,
>> +					     "refp-refn-resistor-ohms property should not be present when refp-supply or refn-supply is present\n");
>> +
>> +		ret = device_property_read_u32(dev, "refp-refn-resistor-ohms",
>> +					       &data->ext_ref_ohms);
>> +		if (ret)
>> +			return dev_err_probe(dev, ret,
>> +					     "failed to read refp-refn-resistor-ohms property\n");
> 
> Using
> 
> 	const char *propname;
> 	...
> 	propname = "refp-refn-resistor-ohms";
> 
> makes this
> 
> 	if (device_property_present(dev, propname)) {
> 		if (refp_uV != 0 || refn_uV != 0)
> 			return dev_err_probe(dev, -EINVAL,
> 					     "%s property should not be present when refp-supply or refn-supply is present\n",
> 					     propname);
> 
> 		ret = device_property_read_u32(dev, propname, &data->ext_ref_ohms);
> 		if (ret)
> 			return dev_err_probe(dev, ret, "failed to read %s property\n", propname);
> 
> Also the rest can be improved in the similar way.

Hmm... maybe less error prone, but makes the code harder to read IMHO.
Will think about it.

> 
>> +	} else {
>> +		if (need_ext_ref && data->ext_ref_uV == 0)
>> +			return dev_err_probe(dev, -EINVAL,
>> +					     "external reference measurements require either refp-supply or refp-refn-resistor-ohms property\n");
>> +	}
>