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
