| Message ID | c8899e8c535a1d93cd7588b7c160eb0fae5d26d2.1741849323.git.mazziesaccount@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
On Thu, Mar 13, 2025 at 09:18:18AM +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. ... > +int devm_iio_adc_device_alloc_chaninfo_se(struct device *dev, > + const struct iio_chan_spec *template, > + int max_chan_id, > + struct iio_chan_spec **cs) > +{ > + struct iio_chan_spec *chan_array, *chan; > + int num_chan = 0, ret; Unneeded assignment. > + num_chan = iio_adc_device_num_channels(dev); > + if (num_chan < 1) > + return num_chan; This is really interesting code. So, if the above returns negative error code, we return it, if it returns 0, we return success (but 0 channels)? Shouldn't we do *cs = NULL; at the case of 0 channels if it's a success? (Under success I assume that returned values are okay to go with, and cs in your case will be left uninitialised or contain something we don't control. > + chan_array = devm_kcalloc(dev, num_chan, sizeof(*chan_array), > + GFP_KERNEL); > + if (!chan_array) > + return -ENOMEM; > + > + chan = &chan_array[0]; > + > + device_for_each_named_child_node_scoped(dev, child, "channel") { > + u32 ch; > + > + ret = fwnode_property_read_u32(child, "reg", &ch); > + if (ret) > + return ret; > + if (max_chan_id != -1 && ch > max_chan_id) > + return -ERANGE; Hmm... What if max_chan_id is equal to an error code? Or in other words, why -1 is special and not all negative numbers? Also note, you used unsigned type and compare it to int which, in case of being negative will give promotion. The ch will not be big enough in most cases (unless it's great than (INT_MAX + 1). TL;DR: you have a potential integer overflow here. > + *chan = *template; > + chan->channel = ch; > + chan++; > + } > + > + *cs = chan_array; > + > + return num_chan; > +}
On 13/03/2025 14:31, Andy Shevchenko wrote: > On Thu, Mar 13, 2025 at 09:18:18AM +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. > > ... > >> +int devm_iio_adc_device_alloc_chaninfo_se(struct device *dev, >> + const struct iio_chan_spec *template, >> + int max_chan_id, >> + struct iio_chan_spec **cs) >> +{ >> + struct iio_chan_spec *chan_array, *chan; >> + int num_chan = 0, ret; > > Unneeded assignment. Hmm. I have a deja-vu. Thanks for the reminder. > >> + num_chan = iio_adc_device_num_channels(dev); >> + if (num_chan < 1) >> + return num_chan; > > This is really interesting code. So, if the above returns negative error code, > we return it, if it returns 0, we return success (but 0 channels)? Yes. I don't think it's that interesting though. Checking the devicetree succeeded while no channels were found. I think returning 0 is very much aligned with this. > Shouldn't we do *cs = NULL; at the case of 0 channels if it's a success? I suppose you're right. But, as you pointed out in review of the 05/10: > Usually in other similar APIs we return -ENOENT. And user won't need > to have an additional check in case of 0 being considered as an error > case too. I don't know whether to agree with you here. For majority of the ADC drivers, having no channels in devicetree is indeed just another error, which I think is not in any ways special. However, for 33,3333% of the users added in this patch, the "no channels found" is not really an error condition ;) The BD79124 could have all channels used for GPO - although this would probably be very very unusual. (Why buying an ADC chip if you need just a GPO?). Still, this wouldn't be an error. (And I need to handle this better in BD79124 probe - so thanks). > (Under success I assume that returned values are okay to go with, and cs in > your case will be left uninitialised or contain something we don't control. I see your point although I wouldn't be concerned with cs not being NULL for as long as number of channels is zero. Anyway, I think it makes sense to simplify ~67% of callers by returning -ENODEV if there is no channels. The remaining ~33% can then check for the -ENODEV and handle it separately from other returned errors. So, thanks. >> + chan_array = devm_kcalloc(dev, num_chan, sizeof(*chan_array), >> + GFP_KERNEL); >> + if (!chan_array) >> + return -ENOMEM; >> + >> + chan = &chan_array[0]; >> + >> + device_for_each_named_child_node_scoped(dev, child, "channel") { >> + u32 ch; >> + >> + ret = fwnode_property_read_u32(child, "reg", &ch); >> + if (ret) >> + return ret; > >> + if (max_chan_id != -1 && ch > max_chan_id) >> + return -ERANGE; > > Hmm... What if max_chan_id is equal to an error code? > Or in other words, why -1 is special and not all negative numbers? -1 was just picked to represent a 'don't care' value. Old habit. In the old days I handled lots of code where -1 was defined as 'invalid' for APIs with unsigned ints as well. It works nicely on systems where it turns out to be maximum positive value - leaving most of the number space for valid values. I suppose saying any negative means "don't care" works well here. And, dropping all negatives here will also make the check below just work with unsigned comparison. > Also note, you used unsigned type and compare it to int which, > in case of being negative will give promotion. Right. I didn't thin negative IDs would make sense and trusted users to pass only positive ones. Treating all negatives as "don't care" is indeed better than trusting this. Thanks. > The ch will not be > big enough in most cases (unless it's great than (INT_MAX + 1). > > TL;DR: you have a potential integer overflow here. > >> + *chan = *template; >> + chan->channel = ch; >> + chan++; >> + } >> + >> + *cs = chan_array; >> + >> + return num_chan; >> +} >
On Thu, Mar 13, 2025 at 03:17:27PM +0200, Matti Vaittinen wrote: > On 13/03/2025 14:31, Andy Shevchenko wrote: > > On Thu, Mar 13, 2025 at 09:18:18AM +0200, Matti Vaittinen wrote: ... > > > + num_chan = iio_adc_device_num_channels(dev); > > > + if (num_chan < 1) > > > + return num_chan; > > > > This is really interesting code. So, if the above returns negative error code, > > we return it, if it returns 0, we return success (but 0 channels)? > > Yes. I don't think it's that interesting though. Checking the devicetree > succeeded while no channels were found. I think returning 0 is very much > aligned with this. Right, but as I suggested, let's follow already established APIs that return -ENOENT and never 0 in similar cases. > > Shouldn't we do *cs = NULL; at the case of 0 channels if it's a success? > > I suppose you're right. > > But, as you pointed out in review of the 05/10: > > Usually in other similar APIs we return -ENOENT. And user won't need > > to have an additional check in case of 0 being considered as an error > > case too. > > I don't know whether to agree with you here. For majority of the ADC > drivers, having no channels in devicetree is indeed just another error, > which I think is not in any ways special. So...? (I see below your answer :-) > However, for 33,3333% of the users added in this patch, the "no channels > found" is not really an error condition ;) The BD79124 could have all > channels used for GPO - although this would probably be very very unusual. > (Why buying an ADC chip if you need just a GPO?). Still, this wouldn't be an > error. (And I need to handle this better in BD79124 probe - so thanks). ENOENT check is again established for optional/not_found cases. > > (Under success I assume that returned values are okay to go with, and cs in > > your case will be left uninitialised or contain something we don't control. > > I see your point although I wouldn't be concerned with cs not being NULL for > as long as number of channels is zero. > > Anyway, I think it makes sense to simplify ~67% of callers by returning > -ENODEV if there is no channels. The remaining ~33% can then check for the > -ENODEV and handle it separately from other returned errors. So, thanks. Not at all!
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig index 849c90203071..37b70a65da6f 100644 --- a/drivers/iio/adc/Kconfig +++ b/drivers/iio/adc/Kconfig @@ -6,6 +6,9 @@ menu "Analog to digital converters" +config IIO_ADC_HELPER + tristate + config AB8500_GPADC bool "ST-Ericsson AB8500 GPADC driver" depends on AB8500_CORE && REGULATOR_AB8500 diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile index ee19afba62b7..1c410f483029 100644 --- a/drivers/iio/adc/Makefile +++ b/drivers/iio/adc/Makefile @@ -3,6 +3,8 @@ # Makefile for IIO ADC drivers # +obj-$(CONFIG_IIO_ADC_HELPER) += industrialio-adc.o + # When adding new entries keep the list in alphabetical order obj-$(CONFIG_AB8500_GPADC) += ab8500-gpadc.o obj-$(CONFIG_AD_SIGMA_DELTA) += ad_sigma_delta.o diff --git a/drivers/iio/adc/industrialio-adc.c b/drivers/iio/adc/industrialio-adc.c new file mode 100644 index 000000000000..6cfb001c74b9 --- /dev/null +++ b/drivers/iio/adc/industrialio-adc.c @@ -0,0 +1,79 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Helpers for parsing common ADC information from a firmware node. + * + * Copyright (c) 2025 Matti Vaittinen <mazziesaccount@gmail.com> + */ + +#include <linux/device.h> +#include <linux/errno.h> +#include <linux/export.h> +#include <linux/module.h> +#include <linux/property.h> +#include <linux/types.h> + +#include <linux/iio/adc-helpers.h> +#include <linux/iio/iio.h> + +/** + * devm_iio_adc_device_alloc_chaninfo_se - allocate and fill iio_chan_spec for ADC + * + * Scan the device node for single-ended ADC channel information. Channel ID is + * expected to be found from the "reg" property. 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. + * + * @dev: Pointer to the ADC device. + * @template: Template iio_chan_spec from which the fields of all + * found and allocated channels are initialized. + * @max_chan_id: Maximum value of a channel ID. Use -1 if no checking + * is required. + * @cs: Location where pointer to allocated iio_chan_spec + * should be stored. + * + * Return: Number of found channels on success. Negative value to indicate + * failure. + */ +int devm_iio_adc_device_alloc_chaninfo_se(struct device *dev, + const struct iio_chan_spec *template, + int max_chan_id, + struct iio_chan_spec **cs) +{ + struct iio_chan_spec *chan_array, *chan; + int num_chan = 0, ret; + + num_chan = iio_adc_device_num_channels(dev); + if (num_chan < 1) + return num_chan; + + chan_array = devm_kcalloc(dev, num_chan, sizeof(*chan_array), + GFP_KERNEL); + if (!chan_array) + return -ENOMEM; + + chan = &chan_array[0]; + + device_for_each_named_child_node_scoped(dev, child, "channel") { + u32 ch; + + ret = fwnode_property_read_u32(child, "reg", &ch); + if (ret) + return ret; + + if (max_chan_id != -1 && ch > max_chan_id) + return -ERANGE; + + *chan = *template; + chan->channel = ch; + chan++; + } + + *cs = chan_array; + + return num_chan; +} +EXPORT_SYMBOL_NS_GPL(devm_iio_adc_device_alloc_chaninfo_se, "IIO_DRIVER"); + +MODULE_LICENSE("GPL"); +MODULE_AUTHOR("Matti Vaittinen <mazziesaccount@gmail.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..56b092a2a4c4 --- /dev/null +++ b/include/linux/iio/adc-helpers.h @@ -0,0 +1,27 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +/* + * The industrial I/O ADC firmware property parsing helpers + * + * Copyright (c) 2025 Matti Vaittinen <mazziesaccount@gmail.com> + */ + +#ifndef _INDUSTRIAL_IO_ADC_HELPERS_H_ +#define _INDUSTRIAL_IO_ADC_HELPERS_H_ + +#include <linux/property.h> + +struct device; +struct iio_chan_spec; + +static inline int iio_adc_device_num_channels(struct device *dev) +{ + return device_get_named_child_node_count(dev, "channel"); +} + +int devm_iio_adc_device_alloc_chaninfo_se(struct device *dev, + const struct iio_chan_spec *template, + int max_chan_id, + struct iio_chan_spec **cs); + +#endif /* _INDUSTRIAL_IO_ADC_HELPERS_H_ */
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@gmail.com> --- Revision history: v6 => - No changes v5 => v6: - Adapt to changed fwnode helper names - Fix spelling v4 => v5: - Inline iio_adc_device_num_channels() - Fix Indenting function parameters - Combine the max channel ID checks. v3 => v4: - Drop diff-channel support - Drop iio_adc_device_channels_by_property() - Add IIO_DEVICE namespace - Move industrialio-adc.o to top of the Makefile - Some styling as suggested by Andy - Re-consider included headers v2 => v3: Mostly based on review comments by Jonathan - Support differential and single-ended channels - Rename iio_adc_device_get_channels() as iio_adc_device_channels_by_property() - 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 --- drivers/iio/adc/Kconfig | 3 ++ drivers/iio/adc/Makefile | 2 + drivers/iio/adc/industrialio-adc.c | 79 ++++++++++++++++++++++++++++++ include/linux/iio/adc-helpers.h | 27 ++++++++++ 4 files changed, 111 insertions(+) create mode 100644 drivers/iio/adc/industrialio-adc.c create mode 100644 include/linux/iio/adc-helpers.h