[PATCH v5 00/10] Support ROHM BD79124 ADC

[PATCH v5 00/10] Support ROHM BD79124 ADC

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 4904 bytes --]

Support ROHM BD79124 ADC.

This series adds also couple of IIO ADC helper functions for parsing the
channel information from the device tree. There are also two helpers
included for counting number of firmware child nodes with a specific name.

Series does also convert couple of drivers to use these helpers. The
rzg2l_adc and the sun20i-gpadc are converted to use the new ADC helper.

The gianfar driver under net is added as an RFC patch to use the newly
added firmware child node counting function.

There has been some discussion about how useful these ADC helpers are,
and whether they should support also differential and single ended channel
configurations. This version does not include support for those - with the
benefit of reduced complexity and easier to use API.

patch 6/10 is small simplification for the ti-ads7924, and it can be
taken independently from the rest of the series.

NOTE: Patches 4...6 and the patch 10 are untested as I lack of relevant HW.
They have been compile tested only.

The ROHM BD79124 ADC itself is quite usual stuff. 12-bit, 8-channel ADC
with threshold monitoring.

Except that:
 - each ADC input pin can be configured as a general purpose output.
 - manually starting an ADC conversion and reading the result would
   require the I2C _master_ to do clock stretching(!) for the duration
   of the conversion... Let's just say this is not well supported.
 - IC supports 'autonomous measurement mode' and storing latest results
   to the result registers. This mode is used by the driver due to the
   "peculiar" I2C when doing manual reads.

Furthermore, the ADC uses this continuous autonomous measuring,
and the IC keeps producing new 'out of window' IRQs if measurements are
out of window - the driver disables the event for 1 seconds when sending
it to user. This prevents generating storm of events

Revision history:
v4 => v5: Fixes as per various review comments. Most notably:
 - Drop the patch making the TI's ADC driver to respect device tree.
 - Add (RFC) patch converting gianfar driver to use new name child-node
   counting API as suggested by Andy.
 - Add fwnode_get_child_node_count_named() as suggested by Rob.
 Changes which were not proposed by reviewers:
 - rebase to v6.14-rc5
 - Do not include all recipients to all of the patches.
 More accurate changelog in individual patches.
v3 => v4:
 - Drop the ADC helper support for differential channels
 - Drop the ADC helper for getting only channel IDs by fwnode.
 - "Promote" the function counting the number of child nodes with a
   specific name to the property.h (As suggested by Jonathan).
 - Add ADC helpers to a namespace.
 - Rebase on v6.14-rc3
 - More minor changes described in individual patches.
v2 => v3:
 - Restrict BD79124 channel numbers as suggested by Conor and add
   Conor's Reviewed-by tag.
 - Support differential and single-ended inputs
 - Convert couple of existing drivers to use the added ADC helpers
 - Minor fixes based on reviews
Link to v2:
https://lore.kernel.org/all/cover.1738761899.git.mazziesaccount@gmail.com/

RFC v1 => v2:
 - Drop MFD and pinmux.
 - Automatically re-enable events after 1 second.
 - Export fwnode parsing helpers for finding the ADC channels.

---

Matti Vaittinen (10):
  dt-bindings: ROHM BD79124 ADC/GPO
  property: Add functions to count named child nodes
  iio: adc: add helpers for parsing ADC nodes
  iio: adc: rzg2l_adc: Use adc-helpers
  iio: adc: sun20i-gpadc: Use adc-helpers
  iio: adc: ti-ads7924 Drop unnecessary function parameters
  iio: adc: Support ROHM BD79124 ADC
  MAINTAINERS: Add IIO ADC helpers
  MAINTAINERS: Add ROHM BD79124 ADC/GPO
  net: gianfar: Use device_get_child_node_count_named()

 .../bindings/iio/adc/rohm,bd79124.yaml        |  114 ++
 MAINTAINERS                                   |   12 +
 drivers/base/property.c                       |   57 +
 drivers/iio/adc/Kconfig                       |   17 +
 drivers/iio/adc/Makefile                      |    3 +
 drivers/iio/adc/industrialio-adc.c            |   82 ++
 drivers/iio/adc/rohm-bd79124.c                | 1108 +++++++++++++++++
 drivers/iio/adc/rzg2l_adc.c                   |   38 +-
 drivers/iio/adc/sun20i-gpadc-iio.c            |   38 +-
 drivers/iio/adc/ti-ads7924.c                  |    7 +-
 drivers/net/ethernet/freescale/gianfar.c      |   17 +-
 include/linux/iio/adc-helpers.h               |   27 +
 include/linux/property.h                      |    4 +
 13 files changed, 1462 insertions(+), 62 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/rohm,bd79124.yaml
 create mode 100644 drivers/iio/adc/industrialio-adc.c
 create mode 100644 drivers/iio/adc/rohm-bd79124.c
 create mode 100644 include/linux/iio/adc-helpers.h


base-commit: 7eb172143d5508b4da468ed59ee857c6e5e01da6
-- 
2.48.1


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH v5 02/10] property: Add functions to count named child nodes

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 4178 bytes --]

There are some use-cases where child nodes with a specific name need to
be parsed. In a few cases the data from the found nodes is added to an
array which is allocated based on the number of found nodes. One example
of such use is the IIO subsystem's ADC channel nodes, where the relevant
nodes are named as channel[@N].

Add a helpers for counting device's sub-nodes with certain name instead
of open-coding this in every user.

Suggested-by: Jonathan Cameron <jic23@kernel.org>
Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
---
Revision history:
v4 => v5:
 - Use given name instead of string 'channel' when counting the nodes
 - Add also fwnode_get_child_node_count_named() as suggested by Rob.
v3 => v4:
 - New patch as suggested by Jonathan, see discussion in:
https://lore.kernel.org/lkml/20250223161338.5c896280@jic23-huawei/
---
 drivers/base/property.c  | 57 ++++++++++++++++++++++++++++++++++++++++
 include/linux/property.h |  4 +++
 2 files changed, 61 insertions(+)

diff --git a/drivers/base/property.c b/drivers/base/property.c
index c1392743df9c..3faf02b99cff 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -945,6 +945,63 @@ unsigned int device_get_child_node_count(const struct device *dev)
 }
 EXPORT_SYMBOL_GPL(device_get_child_node_count);
 
+/**
+ * fwnode_get_child_node_count_named - number of child nodes with given name
+ * @fwnode: Node which child nodes are counted.
+ * @name: String to match child node name against.
+ *
+ * Scan child nodes and count all the nodes with a specific name. Return the
+ * number of found nodes. Potential '@number' -ending for scanned names is
+ * ignored. Eg,
+ * device_get_child_node_count(dev, "channel");
+ * would match all the nodes:
+ * channel { }, channel@0 {}, channel@0xabba {}...
+ *
+ * Return: the number of child nodes with a matching name for a given device.
+ */
+unsigned int fwnode_get_child_node_count_named(const struct fwnode_handle *fwnode,
+					       const char *name)
+{
+	struct fwnode_handle *child;
+	unsigned int count = 0;
+
+	fwnode_for_each_child_node(fwnode, child)
+		if (fwnode_name_eq(child, name))
+			count++;
+
+	return count;
+}
+EXPORT_SYMBOL_GPL(fwnode_get_child_node_count_named);
+
+/**
+ * device_get_child_node_count_named - number of child nodes with given name
+ * @dev: Device to count the child nodes for.
+ * @name: String to match child node name against.
+ *
+ * Scan device's child nodes and find all the nodes with a specific name and
+ * return the number of found nodes. Potential '@number' -ending for scanned
+ * names is ignored. Eg,
+ * device_get_child_node_count(dev, "channel");
+ * would match all the nodes:
+ * channel { }, channel@0 {}, channel@0xabba {}...
+ *
+ * Return: the number of child nodes with a matching name for a given device.
+ */
+unsigned int device_get_child_node_count_named(const struct device *dev,
+					       const char *name)
+{
+	const struct fwnode_handle *fwnode = dev_fwnode(dev);
+
+	if (!fwnode)
+		return -EINVAL;
+
+	if (IS_ERR(fwnode))
+		return PTR_ERR(fwnode);
+
+	return fwnode_get_child_node_count_named(fwnode, name);
+}
+EXPORT_SYMBOL_GPL(device_get_child_node_count_named);
+
 bool device_dma_supported(const struct device *dev)
 {
 	return fwnode_call_bool_op(dev_fwnode(dev), device_dma_supported);
diff --git a/include/linux/property.h b/include/linux/property.h
index e214ecd241eb..269ab539515b 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -209,6 +209,10 @@ int fwnode_irq_get(const struct fwnode_handle *fwnode, unsigned int index);
 int fwnode_irq_get_byname(const struct fwnode_handle *fwnode, const char *name);
 
 unsigned int device_get_child_node_count(const struct device *dev);
+unsigned int fwnode_get_child_node_count_named(const struct fwnode_handle *fwnode,
+					       const char *name);
+unsigned int device_get_child_node_count_named(const struct device *dev,
+					       const char *name);
 
 static inline int device_property_read_u8(const struct device *dev,
 					  const char *propname, u8 *val)
-- 
2.48.1


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH RFC net-next v5 10/10] net: gianfar: Use device_get_child_node_count_named()

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 2231 bytes --]

We can avoid open-coding the loop construct which counts firmware child
nodes with a specific name by using the newly added
device_get_child_node_count_named().

The gianfar driver has such open-coded loop. Replace it with the
device_get_child_node_count_named().

Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
---
It's fair to tell the pros and cons of this patch.
The simplification is there, but it's not a big one. It comes with a cost
of getting the property.h included in this driver which currently uses
exclusively the of_* APIs.

NOTE: This patch depends on the patch:
[2/10] "property: Add functions to count named child nodes"

Compile-tested only!
---
 drivers/net/ethernet/freescale/gianfar.c | 17 ++++-------------
 1 file changed, 4 insertions(+), 13 deletions(-)

diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c
index 435138f4699d..dfe012a5bc0a 100644
--- a/drivers/net/ethernet/freescale/gianfar.c
+++ b/drivers/net/ethernet/freescale/gianfar.c
@@ -97,6 +97,7 @@
 #include <linux/phy_fixed.h>
 #include <linux/of.h>
 #include <linux/of_net.h>
+#include <linux/property.h>
 
 #include "gianfar.h"
 
@@ -571,18 +572,6 @@ static int gfar_parse_group(struct device_node *np,
 	return 0;
 }
 
-static int gfar_of_group_count(struct device_node *np)
-{
-	struct device_node *child;
-	int num = 0;
-
-	for_each_available_child_of_node(np, child)
-		if (of_node_name_eq(child, "queue-group"))
-			num++;
-
-	return num;
-}
-
 /* Reads the controller's registers to determine what interface
  * connects it to the PHY.
  */
@@ -654,8 +643,10 @@ static int gfar_of_init(struct platform_device *ofdev, struct net_device **pdev)
 		num_rx_qs = 1;
 	} else { /* MQ_MG_MODE */
 		/* get the actual number of supported groups */
-		unsigned int num_grps = gfar_of_group_count(np);
+		unsigned int num_grps;
 
+		num_grps = device_get_child_node_count_named(&ofdev->dev,
+							     "queue-group");
 		if (num_grps == 0 || num_grps > MAXGROUPS) {
 			dev_err(&ofdev->dev, "Invalid # of int groups(%d)\n",
 				num_grps);
-- 
2.48.1


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v5 02/10] property: Add functions to count named child nodes

[Heikki Krogerus]

Hi,

> +/**
> + * fwnode_get_child_node_count_named - number of child nodes with given name
> + * @fwnode: Node which child nodes are counted.
> + * @name: String to match child node name against.
> + *
> + * Scan child nodes and count all the nodes with a specific name. Return the
> + * number of found nodes. Potential '@number' -ending for scanned names is
> + * ignored. Eg,
> + * device_get_child_node_count(dev, "channel");
> + * would match all the nodes:
> + * channel { }, channel@0 {}, channel@0xabba {}...
> + *
> + * Return: the number of child nodes with a matching name for a given device.
> + */
> +unsigned int fwnode_get_child_node_count_named(const struct fwnode_handle *fwnode,
> +					       const char *name)
> +{
> +	struct fwnode_handle *child;
> +	unsigned int count = 0;
> +
> +	fwnode_for_each_child_node(fwnode, child)
> +		if (fwnode_name_eq(child, name))
> +			count++;
> +
> +	return count;
> +}
> +EXPORT_SYMBOL_GPL(fwnode_get_child_node_count_named);
> +
> +/**
> + * device_get_child_node_count_named - number of child nodes with given name
> + * @dev: Device to count the child nodes for.
> + * @name: String to match child node name against.
> + *
> + * Scan device's child nodes and find all the nodes with a specific name and
> + * return the number of found nodes. Potential '@number' -ending for scanned
> + * names is ignored. Eg,
> + * device_get_child_node_count(dev, "channel");
> + * would match all the nodes:
> + * channel { }, channel@0 {}, channel@0xabba {}...
> + *
> + * Return: the number of child nodes with a matching name for a given device.
> + */
> +unsigned int device_get_child_node_count_named(const struct device *dev,
> +					       const char *name)
> +{
> +	const struct fwnode_handle *fwnode = dev_fwnode(dev);
> +
> +	if (!fwnode)
> +		return -EINVAL;
> +
> +	if (IS_ERR(fwnode))
> +		return PTR_ERR(fwnode);
> +
> +	return fwnode_get_child_node_count_named(fwnode, name);
> +}
> +EXPORT_SYMBOL_GPL(device_get_child_node_count_named);

Sorry if I missed something in the v4 thread, but why not do all the
checks in fwnode_get_child_node_count_named(), and make this an inline
function?

        static inline unsigned int
        device_get_child_node_count_named(const struct device *dev, const char *name)
        {
                return fwnode_get_child_node_count_named(dev_fwnode(fwnode), name);
        }

thanks,

-- 
heikki

Re: [PATCH RFC net-next v5 10/10] net: gianfar: Use device_get_child_node_count_named()

[Andy Shevchenko]

On Mon, Mar 03, 2025 at 01:34:49PM +0200, Matti Vaittinen wrote:
> We can avoid open-coding the loop construct which counts firmware child
> nodes with a specific name by using the newly added
> device_get_child_node_count_named().
> 
> The gianfar driver has such open-coded loop. Replace it with the
> device_get_child_node_count_named().

...

> It's fair to tell the pros and cons of this patch.
> The simplification is there, but it's not a big one. It comes with a cost
> of getting the property.h included in this driver which currently uses
> exclusively the of_* APIs.

I think it's a good step to the right direction. We might convert the rest
(at least I don't see much impediments while briefly looking into the code).

...

What about the second loop (in gfar_of_init)?
I mean perhaps we want to have fwnode_for_each_named_child_node()
and its device variant that may be also reused in the IIO code and here.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 02/10] property: Add functions to count named child nodes

[Andy Shevchenko]

On Mon, Mar 03, 2025 at 01:31:45PM +0200, Matti Vaittinen wrote:
> There are some use-cases where child nodes with a specific name need to
> be parsed. In a few cases the data from the found nodes is added to an
> array which is allocated based on the number of found nodes. One example
> of such use is the IIO subsystem's ADC channel nodes, where the relevant
> nodes are named as channel[@N].
> 
> Add a helpers for counting device's sub-nodes with certain name instead
> of open-coding this in every user.

...

> +unsigned int fwnode_get_child_node_count_named(const struct fwnode_handle *fwnode,
> +					       const char *name)
> +{
> +	struct fwnode_handle *child;
> +	unsigned int count = 0;

> +	fwnode_for_each_child_node(fwnode, child)
> +		if (fwnode_name_eq(child, name))

I would expect this to be a separate macro

	fwnode_for_each_named_child_node()

(and its device variant) that gives us more consistent approach.

> +			count++;

And the above looks like missing {}, which won't be needed with the other
suggestion in place.

> +	return count;
> +}

> +	if (!fwnode)
> +		return -EINVAL;
> +
> +	if (IS_ERR(fwnode))
> +		return PTR_ERR(fwnode);

I expect that this will return 0 or number of nodes. Why do we need an error code?
If it's really required, it should be in the fwnode API above.

Also do we care about secondary fwnodes?

> +	return fwnode_get_child_node_count_named(fwnode, name);
> +}

...

> +unsigned int fwnode_get_child_node_count_named(const struct fwnode_handle *fwnode,
> +					       const char *name);

To me the following name sounds better: fwnode_get_named_child_node_count().

> +unsigned int device_get_child_node_count_named(const struct device *dev,
> +					       const char *name);

In the similar way.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 02/10] property: Add functions to count named child nodes

[Andy Shevchenko]

On Mon, Mar 03, 2025 at 01:50:13PM +0200, Heikki Krogerus wrote:

...

> > +unsigned int device_get_child_node_count_named(const struct device *dev,
> > +					       const char *name)
> > +{
> > +	const struct fwnode_handle *fwnode = dev_fwnode(dev);
> > +
> > +	if (!fwnode)
> > +		return -EINVAL;
> > +
> > +	if (IS_ERR(fwnode))
> > +		return PTR_ERR(fwnode);
> > +
> > +	return fwnode_get_child_node_count_named(fwnode, name);
> > +}
> > +EXPORT_SYMBOL_GPL(device_get_child_node_count_named);
> 
> Sorry if I missed something in the v4 thread, but why not do all the
> checks in fwnode_get_child_node_count_named(), and make this an inline
> function?

+1, or drop the checks and make it return 0 depending on the follow up use cases.

>         static inline unsigned int
>         device_get_child_node_count_named(const struct device *dev, const char *name)
>         {
>                 return fwnode_get_child_node_count_named(dev_fwnode(fwnode), name);
>         }

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH RFC net-next v5 10/10] net: gianfar: Use device_get_child_node_count_named()

[Matti Vaittinen]

On 03/03/2025 13:51, Andy Shevchenko wrote:
> On Mon, Mar 03, 2025 at 01:34:49PM +0200, Matti Vaittinen wrote:

> 
> What about the second loop (in gfar_of_init)?
> I mean perhaps we want to have fwnode_for_each_named_child_node()
> and its device variant that may be also reused in the IIO code and here.
> 

I agree the fwnode_for_each_named_child_node() would be useful. I think 
I said that already during the previous review rounds. There is plenty 
of code which could be converted to use it.

This, however, is far more than I am willing to do in the context of a 
simple IIO driver addition. The "BD79124 ADC suupport" is already now 10 
patches, 2 of which are directly related to it.

I propose adding the for_each_named_child_node() as a separate series 
with bunch of users appended. That's be plenty of beans to count for 
those who like following the statistics :)

Yours,
	-- Matti

Re: [PATCH RFC net-next v5 10/10] net: gianfar: Use device_get_child_node_count_named()

[Andy Shevchenko]

On Mon, Mar 03, 2025 at 02:13:30PM +0200, Matti Vaittinen wrote:
> On 03/03/2025 13:51, Andy Shevchenko wrote:
> > On Mon, Mar 03, 2025 at 01:34:49PM +0200, Matti Vaittinen wrote:
> 
> > What about the second loop (in gfar_of_init)?
> > I mean perhaps we want to have fwnode_for_each_named_child_node()
> > and its device variant that may be also reused in the IIO code and here.
> 
> I agree the fwnode_for_each_named_child_node() would be useful. I think I
> said that already during the previous review rounds. There is plenty of code
> which could be converted to use it.


> This, however, is far more than I am willing to do in the context of a
> simple IIO driver addition. The "BD79124 ADC suupport" is already now 10
> patches, 2 of which are directly related to it.

But you already will have at least one user (IIO code) and second as in RFC.
I do not ask you to _add_ patches.

> I propose adding the for_each_named_child_node() as a separate series with
> bunch of users appended. That's be plenty of beans to count for those who
> like following the statistics :)

It would sound like an unneeded churn as we first introduce something that we
already know needs a refactoring.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 02/10] property: Add functions to count named child nodes

[Matti Vaittinen]

On 03/03/2025 13:59, Andy Shevchenko wrote:
> On Mon, Mar 03, 2025 at 01:31:45PM +0200, Matti Vaittinen wrote:

...

> 
>> +	return count;
>> +}
> 
>> +	if (!fwnode)
>> +		return -EINVAL;
>> +
>> +	if (IS_ERR(fwnode))
>> +		return PTR_ERR(fwnode);
> 
> I expect that this will return 0 or number of nodes. Why do we need an error code?
> If it's really required, it should be in the fwnode API above.
> 
> Also do we care about secondary fwnodes?

We have the device_get_child_node_count(). 
device_get_child_node_count_named() should follow the same logic.

> 
>> +	return fwnode_get_child_node_count_named(fwnode, name);
>> +}
> 
> ...
> 
>> +unsigned int fwnode_get_child_node_count_named(const struct fwnode_handle *fwnode,
>> +					       const char *name);
> 
> To me the following name sounds better: fwnode_get_named_child_node_count().

Agree.

> 
>> +unsigned int device_get_child_node_count_named(const struct device *dev,
>> +					       const char *name);
> 
> In the similar way.
> 

Re: [PATCH v5 02/10] property: Add functions to count named child nodes

[Andy Shevchenko]

On Mon, Mar 10, 2025 at 08:23:15AM +0200, Matti Vaittinen wrote:
> On 03/03/2025 13:59, Andy Shevchenko wrote:
> > On Mon, Mar 03, 2025 at 01:31:45PM +0200, Matti Vaittinen wrote:

...

> > Also do we care about secondary fwnodes?
> 
> We have the device_get_child_node_count().
> device_get_child_node_count_named() should follow the same logic.

Okay, so we don't care about them right now.

-- 
With Best Regards,
Andy Shevchenko