Re: [PATCH v2 2/5] iio: adc: add helpers for parsing ADC nodes
From: Matti Vaittinen
Date: Tue Feb 11 2025 - 03:54:39 EST
Hi Jonathan,
Thanks for the review and all the comments!
Just a note that I am currently spending some quality time with
rebuilding the floor of my house. Leaking floor drain can cause a bit of
a work... :rolleyes: So, my time with upstream work is a bit limited -
although writing an occasional bug or two can help one to keep the
balance ;)
Anyways, my replies and new versions may be slower than usual..
On 08/02/2025 18:41, Jonathan Cameron wrote:
On Wed, 5 Feb 2025 15:34:51 +0200
Matti Vaittinen <mazziesaccount@xxxxxxxxx> 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@xxxxxxxxx>
---
Revision history:
RFC v1 => v2:
- New patch
I think it might be nice to have helpers for fetching also the other
generic (non vendor specific) ADC properties (as listed in the
Documentation/devicetree/bindings/iio/adc/adc.yaml) - but as I don't
have use for those in BD79124 driver (at least not for now), I don't
imnplement them yet. Anyways, this commit creates a place for such
helpers.
There is often a mix of vendor specific and not in channel nodes.
Hence I'm not sure how widely this will be and it is driver
specific which of the standard things make sense.
I definitely agree. Still, in my experience, no written standard
standardizes use as well as written helpers ;) More we support parsing
the generic helpers by the (add subsystem here)-core, more the driver
writes will use the generic properties (instead of brewing vendor
specific ones).
So before I'd consider a helper like this I'd want to see it alongside
a bunch of users including some of the complex ones so that we know
it generalizes well enough. It doesn't make sense to introduce
it otherwise - just keep the code in the specific drivers instead.
It's an interesting idea, but not a trivial one :)
I agree it's not trivial. But I believe adding helpers one-by-one to
cover 'normal' use-cases guide the use of the properties. Those who need
something more exotic can always implement their custom handlers - and
then a reviewer of such handler can ask "why" ;)
---
drivers/iio/adc/Kconfig | 3 +
drivers/iio/adc/Makefile | 1 +
drivers/iio/adc/industrialio-adc.c | 151 +++++++++++++++++++++++++++++
include/linux/iio/adc-helpers.h | 22 +++++
4 files changed, 177 insertions(+)
create mode 100644 drivers/iio/adc/industrialio-adc.c
create mode 100644 include/linux/iio/adc-helpers.h
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..956c121a7544 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -57,6 +57,7 @@ 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
obj-$(CONFIG_IMX7D_ADC) += imx7d_adc.o
obj-$(CONFIG_IMX8QXP_ADC) += imx8qxp-adc.o
obj-$(CONFIG_IMX93_ADC) += imx93_adc.o
diff --git a/drivers/iio/adc/industrialio-adc.c b/drivers/iio/adc/industrialio-adc.c
new file mode 100644
index 000000000000..366e4c8eb6c7
--- /dev/null
+++ b/drivers/iio/adc/industrialio-adc.c
@@ -0,0 +1,151 @@
+
+/**
+ * iio_adc_device_get_channels - get ADC channel IDs
This sounds far too like the inkern interfaces. Need to associate it instead
with the fwnode / properties stuff.
I was juggling between using the 'fwnode' and 'device'. I took the
'device' approach in order to be consistent with the naming convention
for the functions in property.h.
For example:
device_property_read_u<N>_array() and
fwnode_property_read_u<N>_array()
depending on if the node is found via device pointer, or if the fwnode
is passed directly.
+ *
+ * Scan the device node for ADC channel information. Return an array of found
+ * IDs. Caller need to allocate the memory for the array and provide maximum
Caller needs to provide the memory
Thanks!
+ * number of IDs the array can store.
+ *
+ * @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.
+ *
+ * Return: Number of found channels on succes. Negative value to
+ * indicate failure.
+ */
+int iio_adc_device_get_channels(struct device *dev, int *channels,
+ int max_channels)
+{
+ struct fwnode_handle *fwnode, *child;
+ int num_chan = 0, ret;
+
+ fwnode = dev_fwnode(dev);
+ if (!fwnode) {
As before, I'd relax this until we need to do it. We may never do so.
The BD79124 does not require this as I dropped the MFD, so this is
ultimately your call :) I, however, would like to humbly suggest adding
it now ;) I have changed some APIs in the regulator subsystem and in the
clk subsystem to support cases where the device-tree node is in the
parent (usual MFD device-case), and it has been somewhat scary... (What
if somewhere in some of the existing device-trees the parent happens to
have interesting properties - but it actually is not correct node? This
becomes a potential source of regression when adding support to an
existing API).
Furthermore, when the node is unconditionally taken from the given
device-pointer, it is tempting for the caller to just pass the parent
device as argument...
- If you have done this - please raise your hand. /me raises.
- If you have only later realized it can cause life-time issues when
devm is used - please raise your hand. /me raises.
- If you have tried to kick your own behind when fixing the issues -
please, raise your hand. /me raises. (and if you succeeded - congraz,
you aren't as old and clumsy as I am).
+ fwnode = dev_fwnode(dev->parent);
+ if (!fwnode)
+ return -ENODEV;
+ }
+ fwnode_for_each_child_node(fwnode, child) {
+ if (fwnode_name_eq(child, "channel")) {
As below. I'd flip the logic here and use a continue
Makes sense.
+ u32 ch;
+
+ if (num_chan == max_channels)
+ return -EINVAL;
+
+ ret = fwnode_property_read_u32(child, "reg", &ch);
+ if (ret)
+ return ret;
+
+ /*
+ * 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++;
channel[num_chan++] = ch;
So it is clear how the two are coupled.
Ouch. I am not fan of this. I have a personal issue as I always need to
wonder if this was the case where the ++foo and foo++ resulted different
functionality. I end up doing a test to see in which index the result
got stored. If you don't feel stronly about this, then I would like to
keep the index increase and assignment in separate rows. I believe the
coupling is still quite visible for as long as we keep the assignment
and increase next to each other. But yeah, if you do feel strongly about
this, then it can be implemented as you suggest.
+ }
+ }
+
+ return num_chan;
+
+}
+EXPORT_SYMBOL_GPL(iio_adc_device_get_channels);
+
+/**
+ * devm_iio_adc_device_alloc_chaninfo - allocate and fill iio_chan_spec for 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.
I'd leave that parent node check until we need it (unless you need it for
this one!). Feels like infra structure that might never be used.
That would let us for now use the device_for_each_child_node()
handling.
As above, I adviece to implement the parent device use right from the
beginning - but I can change this as BD79124 dropped MFD.
+ *
+ * @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
+ *
+ * Return: Number of found channels on succes. Negative value to indicate
+ * failure.
+ */
+int devm_iio_adc_device_alloc_chaninfo(struct device *dev,
+ const struct iio_chan_spec *template,
+ struct iio_chan_spec **cs)
+{
+ struct fwnode_handle *fwnode, *child;
+ struct iio_chan_spec *chan;
+ int num_chan = 0, ret;
+
+ fwnode = dev_fwnode(dev);
+ if (!fwnode) {
+ fwnode = dev_fwnode(dev->parent);
+ if (!fwnode)
+ return -ENODEV;
+ }
+
+ num_chan = iio_adc_fwnode_num_channels(fwnode);
+ if (num_chan < 0)
+ return num_chan;
+
+ *cs = devm_kcalloc(dev, num_chan, sizeof(**cs), GFP_KERNEL);
+ if (!*cs)
+ return -ENOMEM;
+
+ chan = &(*cs)[0];
+
+ fwnode_for_each_child_node(fwnode, child) {
+ if (fwnode_name_eq(child, "channel")) {
Flip logic probably and use a continue to reduce indent of
the next bit (which may well get a lot more complex in future).
Right, thanks.
+ u32 ch;
+
+ ret = fwnode_property_read_u32(child, "reg", &ch);
+ if (ret)
+ return ret;
In general the association between reg and channel is more complex.
We need to deal with a reasonable subset of cases (single-channel, diff-channels
etc) where it isn't the case that chan == chan->channel.
I guess this is right. I, however, lacked of knowledge on how the
differential channels etc. are handled :) Hence I just implemented what
I believe is "the most common" approach, while leaving the rest to be
implemented by those who need it.
Still, I agree that a generic looking API which silently produces bad
results for a few of the use-cases is not nice. By the very minimum we
should check if single-channel, diff-channels properties are present,
and then error out with a warning print. That should give a good hint
for those driver writers who are trying to use API for something
unsupported.
Also, restrictions should be mentioned in the documentation.
Do you think we should use some more specific function naming, like
"_simple" - suffix?
+
+ *chan = *template;
+ chan->channel = ch;
+
+ if (num_chan > 1)
+ chan->indexed = 1;
I think set this whatever, probably in the template.
I don't want to see the interface change just because a given DT says
only one channel is connected.
Ah. I didn't think of that! Good point.
Yours,
-- Matti