Re: [PATCH v3 2/9] iio: adc: add helpers for parsing ADC nodes

[Matti Vaittinen]

Hi Andy,

Long time no hear ;) First of all, thanks for the review!

On 19/02/2025 22:41, Andy Shevchenko wrote:
> On Wed, Feb 19, 2025 at 02:30:27PM +0200, Matti Vaittinen 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.
>
> ...
>
> >   - Rename iio_adc_device_get_channels() as
>
> as?

Oh, looks like I got interrupted :) Thanks!

> ...
>
> > obj-$(CONFIG_FSL_MX25_ADC) += fsl-imx25-gcq.o
> >   obj-$(CONFIG_GEHC_PMC_ADC) += gehc-pmc-adc.o
> >   obj-$(CONFIG_HI8435) += hi8435.o
> >   obj-$(CONFIG_HX711) += hx711.o
>
> > +obj-$(CONFIG_IIO_ADC_HELPER) += industrialio-adc.o
>
> Shouldn't this be grouped with other IIO core related objects?

I was unsure where to put this. The 'adc' subfolder contained no other 
IIO core files, so there really was no group. I did consider putting it 
on top of the file but then just decided to go with the alphabetical 
order. Not sure what is the right way though.

> >   obj-$(CONFIG_IMX7D_ADC) += imx7d_adc.o
> >   obj-$(CONFIG_IMX8QXP_ADC) += imx8qxp-adc.o
> >   obj-$(CONFIG_IMX93_ADC) += imx93_adc.o
>
> ...
>
>
> + bitops.h
>
> > +#include <linux/device.h>
> > +#include <linux/errno.h>
>
> + export.h
>
> + module.h
>
> > +#include <linux/property.h>
>
> + types.h

Thanks!

> ...
>
> > +EXPORT_SYMBOL_GPL(iio_adc_device_num_channels);
>
> No namespace?

I was considering also this. The IIO core functions don't belong into a 
namespace - so I followed the convention to keep these similar to other 
IIO core stuff.

(Sometimes I have a feeling that the trend today is to try make things 
intentionally difficult in the name of the safety. Like, "more difficult 
I make this, more experience points I gain in the name of the safety".)

Well, I suppose I could add a namespace for these functions - if this 
approach stays - but I'd really prefer having all IIO core stuff in some 
global IIO namespace and not to have dozens of fine-grained namespaces 
for an IIO driver to use...

> ...
>
> > +	if (!allowed_types || allowed_types & (~IIO_ADC_CHAN_PROP_TYPE_ALL)) {
>
> Unneeded parentheses around negated value.
>
> > +		dev_dbg(dev, "Invalid adc allowed prop types 0x%lx\n",
> > +			allowed_types);
> > +
> > +		return -EINVAL;
> > +	}
> > +	if (found_types & (~allowed_types)) {
>
> Ditto.
>
> > +		long unknown_types = found_types & (~allowed_types);
>
> Ditto and so on...
>
> Where did you get this style from? I think I see it first time in your
> contributions. Is it a new preferences? Why?

Last autumn I found out my house was damaged by water. I had to empty 
half of the rooms and finally move out for 2.5 months. Now I'm finally 
back, but during the moves I lost my printed list of operator 
precedences which I used to have on my desk. I've been writing C for 25 
years or so, and I still don't remember the precedence rules for all 
bitwise operations - and I am fairly convinced I am not the only one.

What I understood is that I don't really have to have a printed list at 
home, or go googling when away from home. I can just make it very, very 
obvious :) Helps me a lot.

> > +		int type;
> > +
> > +		for_each_set_bit(type, &unknown_types,
> > +				 IIO_ADC_CHAN_NUM_PROP_TYPES - 1) {
> > +			dev_err(dev, "Unsupported channel property %s\n",
> > +				iio_adc_type2prop(type));
> > +		}
> > +
> > +		return -EINVAL;
> > +	}
>
> ...
>
> > +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);
>
> 						     diff, ARRAY_SIZE(diff));
>
> (will require array_size.h)

thanks :) And thanks for being helpful with the header - and there is no 
sarcasm!

> > +		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));
>
> Redundant outer parentheses. What's the point, please?

Zero need to think of precedence.

> > +		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;
> > +
> > +}
>
> ...
>
> > +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)
> > +{
> > +	struct iio_chan_spec *chan;
> > +	int num_chan = 0, ret;
> > +
> > +	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];
>
> This and above and below will be easier to read if you introduce a temporary
> variable which will be used locally and assigned to the output later on.
> Also the current approach breaks the rule that infiltrates the output even in
> the error cases.

Agree. Thanks.

> > +	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;
> > +
> > +		ret = fwnode_property_read_u32_array(child, "diff-channels",
> > +						     &diff[0], 2);
>
> As per above.
>
> > +		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);
>
> I believe this is okay to have on a single line,

I try to keep things under 80 chars. It really truly helps me as I'd 
like to have 3 parallel terminals open when writing code. Furthermore, I 
hate to admit it but during the last two years my near vision has 
deteriorated... :/ 40 is getting more distant and 50 is approaching ;)

> > +		if (!ret)
> > +			chtypes_found |= BIT(IIO_ADC_CHAN_PROP_COMMON);
> > +
> > +		ret = iio_adc_prop_type_check_sanity(dev, expected_props,
> > +						     chtypes_found);
> > +		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.
> > +		 */
> > +		if (chan->channel >= num_chan)
> > +			return -ERANGE;
> > +
> > +		chan++;
> > +	}
> > +
> > +	return num_chan;
> > +}
>
> ...
>
> > +#ifndef _INDUSTRIAL_IO_ADC_HELPERS_H_
> > +#define _INDUSTRIAL_IO_ADC_HELPERS_H_
>
> + bits.h
>
> > +#include <linux/iio/iio.h>
>
> I'm failing to see how this is being used in this header.

I suppose it was the struct iio_chan_spec. Yep, forward declaration 
could do, but I guess there would be no benefit because anyone using 
this header is more than likely to use the iio.h as well.

> > +struct device;
> > +struct fwnode_handle;
> > +
> > +enum {
> > +	IIO_ADC_CHAN_PROP_REG,
> > +	IIO_ADC_CHAN_PROP_SINGLE_ENDED,
> > +	IIO_ADC_CHAN_PROP_DIFF,
> > +	IIO_ADC_CHAN_PROP_COMMON,
> > +	IIO_ADC_CHAN_NUM_PROP_TYPES
> > +};
> > +
> > +/*
> > + * Channel property types to be used with iio_adc_device_get_channels,
> > + * devm_iio_adc_device_alloc_chaninfo, ...
>
> Looks like unfinished sentence...

Intention was to just give user an example of functions where this gets 
used, and leave room for more functions to be added. Reason is that 
lists like this tend to end up being incomplete anyways. Hence the ...

> > + */
> > +#define IIO_ADC_CHAN_PROP_TYPE_REG BIT(IIO_ADC_CHAN_PROP_REG)
> > +#define IIO_ADC_CHAN_PROP_TYPE_SINGLE_ENDED BIT(IIO_ADC_CHAN_PROP_SINGLE_ENDED)
> > +#define IIO_ADC_CHAN_PROP_TYPE_SINGLE_COMMON					\
> > +	(BIT(IIO_ADC_CHAN_PROP_SINGLE_ENDED) | BIT(IIO_ADC_CHAN_PROP_COMMON))
> > +#define IIO_ADC_CHAN_PROP_TYPE_DIFF BIT(IIO_ADC_CHAN_PROP_DIFF)
> > +#define IIO_ADC_CHAN_PROP_TYPE_ALL GENMASK(IIO_ADC_CHAN_NUM_PROP_TYPES - 1, 0)
>
> > +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);
> > +
> > +int iio_adc_device_channels_by_property(struct device *dev, int *channels,
> > +				int max_channels,
> > +				const struct iio_adc_props *expected_props);
> > +#endif /* _INDUSTRIAL_IO_ADC_HELPERS_H_ */