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

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

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?

...

> 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?

>  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

...

> +EXPORT_SYMBOL_GPL(iio_adc_device_num_channels);

No namespace?

...

> +	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?

> +		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)


> +		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?

> +		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.

> +	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,

> +		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.

> +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...

> + */
> +#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_ */


-- 
With Best Regards,
Andy Shevchenko