Message-ID: <7435fb1f-1ecc-4806-b11e-1ec8e83a22a2@gmail.com>
Date: Mon, 24 Feb 2025 15:45:31 +0200
From: Matti Vaittinen <mazziesaccount@...il.com>
To: Jonathan Cameron <jic23@...nel.org>
Cc: Matti Vaittinen <matti.vaittinen@...rohmeurope.com>,
 Lars-Peter Clausen <lars@...afoo.de>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>,
 Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>,
 Chen-Yu Tsai <wens@...e.org>, Jernej Skrabec <jernej.skrabec@...il.com>,
 Samuel Holland <samuel@...lland.org>,
 Hugo Villeneuve <hvilleneuve@...onoff.com>, Nuno Sa <nuno.sa@...log.com>,
 David Lechner <dlechner@...libre.com>,
 Javier Carrasco <javier.carrasco.cruz@...il.com>,
 Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
 linux-iio@...r.kernel.org, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-renesas-soc@...r.kernel.org,
 linux-arm-kernel@...ts.infradead.org, linux-sunxi@...ts.linux.dev
Subject: Re: [PATCH v3 2/9] iio: adc: add helpers for parsing ADC nodes

On 23/02/2025 18:13, Jonathan Cameron wrote:
> On Wed, 19 Feb 2025 14:30:27 +0200
> Matti Vaittinen <mazziesaccount@...il.com> wrote:
> 
>> There are ADC ICs which may have some of the AIN pins usable for other
>> functions. These ICs may have some of the AIN pins wired so that they
>> should not be used for ADC.
>>
>> (Preferred?) way for marking pins which can be used as ADC inputs is to
>> add corresponding channels@N nodes in the device tree as described in
>> the ADC binding yaml.
>>
>> Add couple of helper functions which can be used to retrieve the channel
>> information from the device node.
>>
>> Signed-off-by: Matti Vaittinen <mazziesaccount@...il.com>
>>
>> ---
>> Revision history:
>> v2 => v3: Mostly based on review comments by Jonathan
>>   - Support differential and single-ended channels(*)
>>   - Rename iio_adc_device_get_channels() as
>>   - Improve spelling
>>   - Drop support for cases where DT comes from parent device's node
>>   - Decrease loop indent by reverting node name check conditions
>>   - Don't set 'chan->indexed' by number of channels to keep the
>>     interface consistent no matter how many channels are connected.
>>   - Fix ID range check and related comment
>> RFC v1 => v2:
>>   - New patch
>>
>> (*) The support for single-ended and differential channels is 100%
>> untested. I am not convinced it is actually an improvement over the
>> *_simple() helpers which only supported getting the ID from the "reg
> 
> Currently it definitely feels too complex.  Partly, whilst I haven't
> tried fleshing out the alternative, it feels like you've tried to make
> it too general.  I really don't like the allowed bitmap as those
> relationships are complex.
> 
>> In theory they could be used. In practice, while I skimmed through the
>> in-tree ADC drivers which used the for_each_child_node() construct - it
>> seemed that most of those which supported differential inputs had also
>> some other per-channel properties to read. Those users would in any case
>> need to loop through the nodes to get those other properties.
> That doesn't surprise me that much. I'm still not sure there are enough
> 'simple' cases (i.e. more than maybe 3) to justify this being shared.
> 
>>
>> If I am once more allowed to go back to proposing the _simple() variant
>> which only covers the case: "chan ID in 'reg' property"... Dropping
>> support for differential and single-ended channels would simplify this
>> helper a lot. It'd allow dropping the sanity check as well as the extra
>> parameters callers need to pass to tell what kind of properties they
>> expect. That'd (in my opinion) made the last patches (to actual ADC
>> drivers) in this series a much more lean and worthy ;)
> 
> If you do, call it _se() or something like that.

Due to the all above - I'll drop the differential channel support and 
used the _se() suffix while always accepting the single-ended property. 
This is a functional change to the existing callers later in the series 
though, but I doubt it causes problems.

>> +
>> +#include <linux/device.h>
>> +#include <linux/errno.h>
>> +#include <linux/property.h>
>> +
>> +#include <linux/iio/adc-helpers.h>
>> +
>> +int iio_adc_device_num_channels(struct device *dev)
>> +{
>> +	int num_chan = 0;
>> +
>> +	device_for_each_child_node_scoped(dev, child)
>> +		if (fwnode_name_eq(child, "channel"))
>> +			num_chan++;
>> +
>> +	return num_chan;
> 
> This function seems easy to generalize to count nodes of particular
> name.  So I'd promote this as a generic property.h helper and
> just use that in here.

I didn't think of that but now that you said it, I agree.

Oh well, that means another area impacted - let's see what the fwnode 
peeps say about adding it.

>> +}
>> +EXPORT_SYMBOL_GPL(iio_adc_device_num_channels);
>> +
>> +static const char *iio_adc_type2prop(int type)
>> +{
>> +	switch (type) {
>> +	case IIO_ADC_CHAN_PROP_TYPE_REG:
>> +		return "reg";
>> +	case IIO_ADC_CHAN_PROP_TYPE_SINGLE_ENDED:
>> +		return "single-channel";
>> +	case IIO_ADC_CHAN_PROP_TYPE_DIFF:
>> +		return "diff-channels";
>> +	case IIO_ADC_CHAN_PROP_COMMON:
>> +		return "common-mode-channel";
>> +	default:
>> +		return "unknown";
>> +	}
>> +}
>> +
>> +/*
>> + * Sanity check. Ensure that:
>> + * - At least some type(s) are allowed
>> + * - All types found are also expected
>> + * - If plain "reg" is not allowed, either single-ended or differential
>> + *   properties are found.
> 
> I'd worry this is a combination of fragile and overly separate from
> the parser.  I'd just encode this stuff down there based on accepted type
> of channels.  Two flags, differential and single ended may be enough.
> If single only then reg is expected solution but I don't see a reason to
> ignore single-channel even then as meaning is well defined.

I could accept both the single-channel and reg - but I think the binding 
is super clear telling that 'reg' is required and must match the channel 
ID. So, I feel supporting the single-channel withoout the 
common-mode-channel or diff-channels is kind of pointless?

Besides, reading both reg and single-channel leaves a question: "What if 
reg and single-channel have different values?" Should we error out? And, 
if they don't have different values, why bother checking the 
single-channel at all as reg is anyways required?

This sanity-check becomes obsolete when we only read the 'reg', and 
ignore everything which does not have (a valid) one.

>> +/**
>> + * iio_adc_device_channels_by_property - get ADC channel IDs
>> + *
>> + * Scan the device node for ADC channel information. Return an array of found
>> + * IDs. Caller needs to provide the memory for the array and provide maximum
>> + * number of IDs the array can store.
> 
> I'm somewhat confused by this. Feels like you can get same info from the
> iio_chan_spec array generated by the next function.

I used this in bd79124 for two purposes. First was setting the mux (ADC 
/ GPO) in the ADC portion of the code. This can indeed be done also by 
just iterating throuh the iio_chan_spec.

Second use-case was a call from the GPIO driver's valid_mask function. 
There I used this to detect which channels were reserved for ADC so I 
was able to build the valid-mask for GPIO core. Using the iio_chan_spec 
in GPIO code felt like a "no no" to me :) Having this available was 
quite handy - and I thought there might be other users as well.

Well, the ADC and GPIO now share the same private data (which is a bit 
ugly in my eyes, but maybe the simplest way to go) - so I can just build 
the valid-mask for the GPIOs in the ADC initialization where it reads 
the channels for the mux. That way I can avoid calling the GPIO's 
valid_mask callback and populate the info before registering the GPIO 
driver.

Long story short - I'll probably just drop this function.

>> + *
>> + * @dev:		Pointer to the ADC device
>> + * @channels:		Array where the found IDs will be stored.
>> + * @max_channels:	Number of IDs that fit in the array.
>> + * @expected_props:	Bitmaps of channel property types (for checking).
>> + *
>> + * Return:		Number of found channels on succes. 0 if no channels
>> + *			was found. Negative value to indicate failure.
>> + */
>> +int iio_adc_device_channels_by_property(struct device *dev, int *channels,
>> +		int max_channels, const struct iio_adc_props *expected_props)
>> +{
>> +	int num_chan = 0, ret;
>> +
>> +	device_for_each_child_node_scoped(dev, child) {
>> +		u32 ch, diff[2], se;
>> +		struct iio_adc_props tmp;
>> +		int chtypes_found = 0;
>> +
>> +		if (!fwnode_name_eq(child, "channel"))
>> +			continue;
>> +
>> +		if (num_chan == max_channels)
>> +			return -EINVAL;
>> +
>> +		ret = fwnode_property_read_u32(child, "reg", &ch);
>> +		if (ret)
>> +			return ret;
>> +
>> +		ret = fwnode_property_read_u32_array(child, "diff-channels",
>> +						     &diff[0], 2);
>> +		if (!ret)
>> +			chtypes_found |= IIO_ADC_CHAN_PROP_TYPE_DIFF;
>> +
>> +		ret = fwnode_property_read_u32(child, "single-channel", &se);
>> +		if (!ret)
>> +			chtypes_found |= IIO_ADC_CHAN_PROP_TYPE_SINGLE_ENDED;
>> +
>> +		tmp = *expected_props;
>> +		/*
>> +		 * We don't bother reading the "common-mode-channel" here as it
>> +		 * doesn't really affect on the primary channel ID. We remove
>> +		 * it from the required properties to allow the sanity check
>> +		 * pass here  also for drivers which require it.
>> +		 */
>> +		tmp.required &= (~BIT(IIO_ADC_CHAN_PROP_COMMON));
>> +
>> +		ret = iio_adc_prop_type_check_sanity(dev, &tmp, chtypes_found);
>> +		if (ret)
>> +			return ret;
>> +
>> +		if (chtypes_found & IIO_ADC_CHAN_PROP_TYPE_DIFF)
>> +			ch = diff[0];
>> +		else if (chtypes_found & IIO_ADC_CHAN_PROP_TYPE_SINGLE_ENDED)
>> +			ch = se;
>> +
>> +		/*
>> +		 * We assume the channel IDs start from 0. If it seems this is
>> +		 * not a sane assumption, then we can relax this check or add
>> +		 * 'allowed ID range' parameter.
>> +		 *
>> +		 * Let's just start with this simple assumption.
>> +		 */
>> +		if (ch >= max_channels)
>> +			return -ERANGE;
>> +
>> +		channels[num_chan] = ch;
>> +		num_chan++;
>> +	}
>> +
>> +	return num_chan;
>> +
>> +}
>> +EXPORT_SYMBOL_GPL(iio_adc_device_channels_by_property);
>> +
>> +/**
>> + * devm_iio_adc_device_alloc_chaninfo - allocate and fill iio_chan_spec for adc
> 
> ADC
> 
>> + *
>> + * Scan the device node for ADC channel information. Allocate and populate the
>> + * iio_chan_spec structure corresponding to channels that are found. The memory
>> + * for iio_chan_spec structure will be freed upon device detach. Try parent
>> + * device node if given device has no fwnode associated to cover also MFD
>> + * devices.
>> + *
>> + * @dev:		Pointer to the ADC device.
>> + * @template:		Template iio_chan_spec from which the fields of all
>> + *			found and allocated channels are initialized.
>> + * @cs:			Location where pointer to allocated iio_chan_spec
>> + *			should be stored
>> + * @expected_props:	Bitmaps of channel property types (for checking).
> Input parameter so should be after template.
> 
>> + *
>> + * Return:	Number of found channels on succes. Negative value to indicate
>> + *		failure.
> 
> I wonder if an alloc function would be better returning the pointer and
> providing num_chans via a parameter.

I personally dislike returning pointer for anything which may fail other 
than by -ENOMEM. I dislike having to add all the the IS_ERR(), 
PTR_ERR(), ERR_PTR() - boilerplate when handling the errors. So, I'd 
rather kept the return value as is here if this is not a show stopper 
for you.

> 
>> + */
>> +int devm_iio_adc_device_alloc_chaninfo(struct device *dev,
>> +				const struct iio_chan_spec *template,
>> +				struct iio_chan_spec **cs,
>> +				const struct iio_adc_props *expected_props)
> 
> I'm not sure this expected props thing works as often it's a case
> of complex relationships

Luckily we can drop it altogether by having separate functions for cases 
where the channel ID is expected to be in "reg", "diff-channels" or 
"single-channel" [+ "common-mode-channel"].

And, we can introduce the more complex variants only if they are some 
day needed/usefull.

>> +{
>> +	struct iio_chan_spec *chan;
>> +	int num_chan = 0, ret;
> Initialized just after this so don't set num_chan = 0.
> 
>> +
>> +	num_chan = iio_adc_device_num_channels(dev);
>> +	if (num_chan < 1)
>> +		return num_chan;
>> +
>> +	*cs = devm_kcalloc(dev, num_chan, sizeof(**cs), GFP_KERNEL);
>> +	if (!*cs)
>> +		return -ENOMEM;
>> +
>> +	chan = &(*cs)[0];
>> +
>> +	device_for_each_child_node_scoped(dev, child) {
>> +		u32 ch, diff[2], se, common;
>> +		int chtypes_found = 0;
>> +
>> +		if (!fwnode_name_eq(child, "channel"))
>> +			continue;
>> +
>> +		ret = fwnode_property_read_u32(child, "reg", &ch);
>> +		if (ret)
>> +			return ret;
>> +
> 
> diff channels and single channel take precedence over reg, so check them
> first. If present no need to look for reg can also read it then throw
> it away giving simpler.

I liked to always read the "reg" because (AFAIR) it was marked as 
required in the binding doc.

> 
> 		*chan = *template;
> 
> 		// reg should exist either way.
> 		ret = fwnode_property_read_u32(child, "reg", &reg);
> 		if (ret)
> 			return -EINVAL; //should be a reg whatever.
> 
> 		ret = fwnode_property_read_u32_array(child, "diff-channels",
> 						     diff, ARRAY_SIZE(diff));
> 		if (ret == 0) {
> 			chan->differential = 1;
> 			chan->channel = diff[0];
> 			chan->channel2 = diff[1];
> 		} else {
> 			ret = fwnode_property_read_u32(child, "single-channel", &se);
> 			if (ret)
> 				se = reg;
> 
> 			chan->channel = se;
> 			//IIRC common mode channel is rare. I'd skip it. That
> 			//also makes it a differential channel be it a weird one.

Yes. There weren't many drivers I found using this. But I have a feeling 
that those which I checked and which did, didn't set the "differential" 
-flag in iio_chan_spec for single-ended + common - case.

***
Out of the curiosity (which means there is no need to reply if you're 
busy - I assume I can go and look the code to see what the 
'differential' and 'channel2' modifiers are used for)

What role does the "differential" Vs "single-ended" play for the users? 
I suppose it sure affects to what channels are reserved/free - but I 
would assume users were just interested to get the measurement result 
without caring about whether the signal is measured over differential 
pair or single-ended to gnd(?).
***

> 		}
> 					
> 
>> +		ret = fwnode_property_read_u32_array(child, "diff-channels",
>> +						     &diff[0], 2);
>> +		if (!ret)
>> +			chtypes_found |= IIO_ADC_CHAN_PROP_TYPE_DIFF;
>> +
>> +		ret = fwnode_property_read_u32(child, "single-channel", &se);
>> +		if (!ret)
>> +			chtypes_found |= IIO_ADC_CHAN_PROP_TYPE_SINGLE_ENDED;
>> +
>> +		ret = fwnode_property_read_u32(child, "common-mode-channel",
>> +					       &common);
>> +		if (!ret)
>> +			chtypes_found |= BIT(IIO_ADC_CHAN_PROP_COMMON);
>> +
>> +		ret = iio_adc_prop_type_check_sanity(dev, expected_props,
>> +						     chtypes_found);
> 
> If we want to verify this (I'm not yet sure) then do it as you parse the
> properties, not separately.
> 
>> +		if (ret)
>> +			return ret;
>> +
>> +		*chan = *template;
>> +		chan->channel = ch;
>> +
>> +		if (chtypes_found & IIO_ADC_CHAN_PROP_TYPE_DIFF) {
>> +			chan->differential = 1;
>> +			chan->channel = diff[0];
>> +			chan->channel2 = diff[1];
>> +
>> +		} else if (chtypes_found & IIO_ADC_CHAN_PROP_TYPE_SINGLE_ENDED) {
>> +			chan->channel = se;
>> +			if (chtypes_found & BIT(IIO_ADC_CHAN_PROP_COMMON))
>> +				chan->channel2 = common;
>> +		}
>> +
>> +		/*
>> +		 * We assume the channel IDs start from 0. If it seems this is
>> +		 * not a sane assumption, then we have to add 'allowed ID ranges'
>> +		 * to the struct iio_adc_props because some of the callers may
>> +		 * rely on the IDs being in this range - and have arrays indexed
>> +		 * by the ID.
> 
> Not a requirement in general.  It is more than possible to have a single channel
> provided that is number 7.

I think a few of the drivers did assume that the channel indexes start 
from zero, and go to num_channels - 1. It seemed quite usual that the 
channel IDs were used to index arrays with NUM_CHANNELS elements. A few 
did also check that the IDs were in a valid range.

I will add a max_id parameter to the API (and drop the 
'expected_props'). This should be useful for majority of the callers and 
help them to omit open-coding this particular check. It should also 
remind the driver to check the size of found IDs to avoid overflow bugs. 
As far as I remember at least one of the drivers which I encountered 
just trusted the DT channel IDs to be within valid range.

I'll allow omitting the check by setting the max_id to -1.

> 
>> +		 */
>> +		if (chan->channel >= num_chan)
>> +			return -ERANGE;
>> +
>> +		chan++;
>> +	}
>> +
>> +	return num_chan;
>> +}
>> +EXPORT_SYMBOL_GPL(devm_iio_adc_device_alloc_chaninfo);
>> +
>> +MODULE_LICENSE("GPL");
>> +MODULE_AUTHOR("Matti Vaittinen <mazziesaccount@...il.com>");
>> +MODULE_DESCRIPTION("IIO ADC fwnode parsing helpers");
>> diff --git a/include/linux/iio/adc-helpers.h b/include/linux/iio/adc-helpers.h
>> new file mode 100644
>> index 000000000000..f7791d45dbd2
>> --- /dev/null
>> +++ b/include/linux/iio/adc-helpers.h
>> @@ -0,0 +1,56 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +
>> +/* The industrial I/O ADC helpers
> Comment syntax and make it more obvious that while this might be useful
> it isn't something we necessarily expect to be useful to every driver.
> /*
>   * Industrial I/O ADC firmware property passing helpers.
>   *
> 

Thanks :)

Yours,
	-- Matti

Powered by blists - more mailing lists