[2/2] iio:adc:ad7124: Convert to fwnode handling of child node parsing.

Commit Message

Jonathan Cameron July 25, 2021, 5:24 p.m. UTC

From: Jonathan Cameron <[e-mail]>

Also use device_get_match_data() rather than of specific variant.
These changes enable use of this binding on ACPI platforms via PRP0001.
Whilst it's possible no one will ever do so, this is part of a general
effort to clear out examples from IIO that might be copied into new
drivers.

It may appear that this change drops the check for status = disabled,
but in reality it does not because the of property code uses
of_get_next_available_child().  This driver may well fail to probe
if disabled is ever actually set though due to the need for
complete concurrent child nodes.  A future series might resolve
that restriction.

Signed-off-by: Jonathan Cameron <[e-mail]>
---
 drivers/iio/adc/ad7124.c | 35 ++++++++++++++++++-----------------
 1 file changed, 18 insertions(+), 17 deletions(-)

Comments

Andy Shevchenko July 25, 2021, 8:33 p.m. UTC | #1

On Sun, Jul 25, 2021 at 8:22 PM Jonathan Cameron <[e-mail]> wrote:
>
> From: Jonathan Cameron <[e-mail]>
>
> Also use device_get_match_data() rather than of specific variant.
> These changes enable use of this binding on ACPI platforms via PRP0001.
> Whilst it's possible no one will ever do so, this is part of a general
> effort to clear out examples from IIO that might be copied into new
> drivers.
>
> It may appear that this change drops the check for status = disabled,
> but in reality it does not because the of property code uses
> of_get_next_available_child().  This driver may well fail to probe
> if disabled is ever actually set though due to the need for
> complete concurrent child nodes.  A future series might resolve
> that restriction.

Perhaps we need to have

...

> +       device_for_each_child_node(dev, child)
> +               st->num_channels++;
> +

device_get_child_node_count() ?

...

> -       for_each_available_child_of_node(np, child) {
> +       device_for_each_child_node(dev, child) {

Isn't this
  fwnode_for_each_available_child_node()
better to use?

...

So the gaps I see are
  device_get_available_child_node_count()
and
  device_for_each_available_child_node()

Both of them I think are easy to add and avoid possible breakage.

Jonathan Cameron July 27, 2021, 1:51 p.m. UTC | #2

On Sun, 25 Jul 2021 23:33:12 +0300
Andy Shevchenko <[e-mail]> wrote:

> On Sun, Jul 25, 2021 at 8:22 PM Jonathan Cameron <[e-mail]> wrote:
> >
> > From: Jonathan Cameron <[e-mail]>
> >
> > Also use device_get_match_data() rather than of specific variant.
> > These changes enable use of this binding on ACPI platforms via PRP0001.
> > Whilst it's possible no one will ever do so, this is part of a general
> > effort to clear out examples from IIO that might be copied into new
> > drivers.
> >
> > It may appear that this change drops the check for status = disabled,
> > but in reality it does not because the of property code uses
> > of_get_next_available_child().  This driver may well fail to probe
> > if disabled is ever actually set though due to the need for
> > complete concurrent child nodes.  A future series might resolve
> > that restriction.  
> 
> Perhaps we need to have
> 
> ...
> 
> > +       device_for_each_child_node(dev, child)
> > +               st->num_channels++;
> > +  
> 
> device_get_child_node_count() ?
> 

Gah. Not sure how I missed that one when looking for it...

> ...
> 
> > -       for_each_available_child_of_node(np, child) {
> > +       device_for_each_child_node(dev, child) {  
> 
> Isn't this
>   fwnode_for_each_available_child_node()
> better to use?

Given we would be extracting the fwnode just to call this
loop, I'd say no, device version makes more sense..

> 
> ...
> 
> So the gaps I see are
>   device_get_available_child_node_count()
> and
>   device_for_each_available_child_node()

Do we then fix the fact that
device_for_each_child_node() will call the _available() form
for device tree?  That seems inconsistent currently and
I was assuming that was deliberate...

Jonathan


> 
> Both of them I think are easy to add and avoid possible breakage.
>

Andy Shevchenko July 27, 2021, 2:16 p.m. UTC | #3

On Tue, Jul 27, 2021 at 4:52 PM Jonathan Cameron
<[e-mail]> wrote:
> On Sun, 25 Jul 2021 23:33:12 +0300
> Andy Shevchenko <[e-mail]> wrote:
> > On Sun, Jul 25, 2021 at 8:22 PM Jonathan Cameron <[e-mail]> wrote:

...

> > > -       for_each_available_child_of_node(np, child) {
> > > +       device_for_each_child_node(dev, child) {
> >
> > Isn't this
> >   fwnode_for_each_available_child_node()
> > better to use?
>
> Given we would be extracting the fwnode just to call this
> loop, I'd say no, device version makes more sense..
>
> >
> > ...
> >
> > So the gaps I see are
> >   device_get_available_child_node_count()
> > and
> >   device_for_each_available_child_node()
>
> Do we then fix the fact that
> device_for_each_child_node() will call the _available() form
> for device tree?  That seems inconsistent currently and
> I was assuming that was deliberate...

I'm not sure I got your point. Mine (see below) is to add the APIs
that you want to use as a direct replacement of the corresponding OF
counterparts.

> > Both of them I think are easy to add and avoid possible breakage.

Jonathan Cameron July 27, 2021, 6:20 p.m. UTC | #4

On Tue, 27 Jul 2021 17:16:07 +0300
Andy Shevchenko <[e-mail]> wrote:

> On Tue, Jul 27, 2021 at 4:52 PM Jonathan Cameron
> <[e-mail]> wrote:
> > On Sun, 25 Jul 2021 23:33:12 +0300
> > Andy Shevchenko <[e-mail]> wrote:  
> > > On Sun, Jul 25, 2021 at 8:22 PM Jonathan Cameron <[e-mail]> wrote:  
> 
> ...
> 
> > > > -       for_each_available_child_of_node(np, child) {
> > > > +       device_for_each_child_node(dev, child) {  
> > >
> > > Isn't this
> > >   fwnode_for_each_available_child_node()
> > > better to use?  
> >
> > Given we would be extracting the fwnode just to call this
> > loop, I'd say no, device version makes more sense..
> >  
> > >
> > > ...
> > >
> > > So the gaps I see are
> > >   device_get_available_child_node_count()
> > > and
> > >   device_for_each_available_child_node()  
> >
> > Do we then fix the fact that
> > device_for_each_child_node() will call the _available() form
> > for device tree?  That seems inconsistent currently and
> > I was assuming that was deliberate...  
> 
> I'm not sure I got your point. Mine (see below) is to add the APIs
> that you want to use as a direct replacement of the corresponding OF
> counterparts.
+CC Rafael,

The oddity is that device_for_each_child_node() is a direct replacement
of the for_each_available_child_of_node() other than the obvious
use of device rather than the of node.

https://elixir.bootlin.com/linux/v5.14-rc3/source/drivers/of/property.c#L939

static struct fwnode_handle *
of_fwnode_get_next_child_node(const struct fwnode_handle *fwnode,
			      struct fwnode_handle *child)
{
	return of_fwnode_handle(of_get_next_available_child(to_of_node(fwnode),
							    to_of_node(child)));
}

So the question becomes whether there is any desire at all to have a
version of the device_for_each_child_node() that does not check
if it is available or not.

Looks like it goes all the way back.  Rafael, any comment on why the available
for is used here and whether it makes sense to introduce separate
versions for looping over children that cover the _available_ and everything
cases?

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/base/property.c?id=8a0662d9ed2968e1186208336a8e1fab3fdfea63

I'm kind of assuming this was deliberate as we don't want to encourage
accessing disabled firmware nodes.

Jonathan

> 
> > > Both of them I think are easy to add and avoid possible breakage.  
> 
>

Jonathan Cameron Aug. 15, 2021, 4:09 p.m. UTC | #5

On Tue, 27 Jul 2021 19:20:13 +0100
Jonathan Cameron <[e-mail]> wrote:

> On Tue, 27 Jul 2021 17:16:07 +0300
> Andy Shevchenko <[e-mail]> wrote:
> 
> > On Tue, Jul 27, 2021 at 4:52 PM Jonathan Cameron
> > <[e-mail]> wrote:  
> > > On Sun, 25 Jul 2021 23:33:12 +0300
> > > Andy Shevchenko <[e-mail]> wrote:    
> > > > On Sun, Jul 25, 2021 at 8:22 PM Jonathan Cameron <[e-mail]> wrote:    
> > 
> > ...
> >   
> > > > > -       for_each_available_child_of_node(np, child) {
> > > > > +       device_for_each_child_node(dev, child) {    
> > > >
> > > > Isn't this
> > > >   fwnode_for_each_available_child_node()
> > > > better to use?    
> > >
> > > Given we would be extracting the fwnode just to call this
> > > loop, I'd say no, device version makes more sense..
> > >    
> > > >
> > > > ...
> > > >
> > > > So the gaps I see are
> > > >   device_get_available_child_node_count()
> > > > and
> > > >   device_for_each_available_child_node()    
> > >
> > > Do we then fix the fact that
> > > device_for_each_child_node() will call the _available() form
> > > for device tree?  That seems inconsistent currently and
> > > I was assuming that was deliberate...    
> > 
> > I'm not sure I got your point. Mine (see below) is to add the APIs
> > that you want to use as a direct replacement of the corresponding OF
> > counterparts.  
> +CC Rafael,

Rafael, if you have a chance to give input on the questions below it would
be much appreciated.

Thanks,

Jonathan

> 
> The oddity is that device_for_each_child_node() is a direct replacement
> of the for_each_available_child_of_node() other than the obvious
> use of device rather than the of node.
> 
> https://elixir.bootlin.com/linux/v5.14-rc3/source/drivers/of/property.c#L939
> 
> static struct fwnode_handle *
> of_fwnode_get_next_child_node(const struct fwnode_handle *fwnode,
> 			      struct fwnode_handle *child)
> {
> 	return of_fwnode_handle(of_get_next_available_child(to_of_node(fwnode),
> 							    to_of_node(child)));
> }
> 
> So the question becomes whether there is any desire at all to have a
> version of the device_for_each_child_node() that does not check
> if it is available or not.
> 
> Looks like it goes all the way back.  Rafael, any comment on why the available
> for is used here and whether it makes sense to introduce separate
> versions for looping over children that cover the _available_ and everything
> cases?
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/base/property.c?id=8a0662d9ed2968e1186208336a8e1fab3fdfea63
> 
> I'm kind of assuming this was deliberate as we don't want to encourage
> accessing disabled firmware nodes.
> 
> Jonathan
> 
> >   
> > > > Both of them I think are easy to add and avoid possible breakage.    
> > 
> >   
>

Jonathan Cameron Oct. 3, 2021, 3:45 p.m. UTC | #6

On Sun, 15 Aug 2021 17:09:51 +0100
Jonathan Cameron <[e-mail]> wrote:

> On Tue, 27 Jul 2021 19:20:13 +0100
> Jonathan Cameron <[e-mail]> wrote:
> 
> > On Tue, 27 Jul 2021 17:16:07 +0300
> > Andy Shevchenko <[e-mail]> wrote:
> >   
> > > On Tue, Jul 27, 2021 at 4:52 PM Jonathan Cameron
> > > <[e-mail]> wrote:    
> > > > On Sun, 25 Jul 2021 23:33:12 +0300
> > > > Andy Shevchenko <[e-mail]> wrote:      
> > > > > On Sun, Jul 25, 2021 at 8:22 PM Jonathan Cameron <[e-mail]> wrote:      
> > > 
> > > ...
> > >     
> > > > > > -       for_each_available_child_of_node(np, child) {
> > > > > > +       device_for_each_child_node(dev, child) {      
> > > > >
> > > > > Isn't this
> > > > >   fwnode_for_each_available_child_node()
> > > > > better to use?      
> > > >
> > > > Given we would be extracting the fwnode just to call this
> > > > loop, I'd say no, device version makes more sense..
> > > >      
> > > > >
> > > > > ...
> > > > >
> > > > > So the gaps I see are
> > > > >   device_get_available_child_node_count()
> > > > > and
> > > > >   device_for_each_available_child_node()      
> > > >
> > > > Do we then fix the fact that
> > > > device_for_each_child_node() will call the _available() form
> > > > for device tree?  That seems inconsistent currently and
> > > > I was assuming that was deliberate...      
> > > 
> > > I'm not sure I got your point. Mine (see below) is to add the APIs
> > > that you want to use as a direct replacement of the corresponding OF
> > > counterparts.    
> > +CC Rafael,  
> 
> Rafael, if you have a chance to give input on the questions below it would
> be much appreciated.

Rafael, if you have a chance to look at this it would be great.
> 
> Thanks,
> 
> Jonathan
> 
> > 
> > The oddity is that device_for_each_child_node() is a direct replacement
> > of the for_each_available_child_of_node() other than the obvious
> > use of device rather than the of node.
> > 
> > https://elixir.bootlin.com/linux/v5.14-rc3/source/drivers/of/property.c#L939
> > 
> > static struct fwnode_handle *
> > of_fwnode_get_next_child_node(const struct fwnode_handle *fwnode,
> > 			      struct fwnode_handle *child)
> > {
> > 	return of_fwnode_handle(of_get_next_available_child(to_of_node(fwnode),
> > 							    to_of_node(child)));
> > }
> > 
> > So the question becomes whether there is any desire at all to have a
> > version of the device_for_each_child_node() that does not check
> > if it is available or not.
> > 
> > Looks like it goes all the way back.  Rafael, any comment on why the available
> > for is used here and whether it makes sense to introduce separate
> > versions for looping over children that cover the _available_ and everything
> > cases?
> > 
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/base/property.c?id=8a0662d9ed2968e1186208336a8e1fab3fdfea63
> > 
> > I'm kind of assuming this was deliberate as we don't want to encourage
> > accessing disabled firmware nodes.
> > 
> > Jonathan
> >   
> > >     
> > > > > Both of them I think are easy to add and avoid possible breakage.      
> > > 
> > >     
> >   
>

Jonathan Cameron Nov. 28, 2021, 6:15 p.m. UTC | #7

Rafael, looking for your input on this.

Just converted it again, having forgotten this patch set was outstanding until
I saw the same line of code and it all came back..

Thanks,

Jonathan



On Sun, 3 Oct 2021 16:45:41 +0100
Jonathan Cameron <[e-mail]> wrote:

> On Sun, 15 Aug 2021 17:09:51 +0100
> Jonathan Cameron <[e-mail]> wrote:
> 
> > On Tue, 27 Jul 2021 19:20:13 +0100
> > Jonathan Cameron <[e-mail]> wrote:
> >   
> > > On Tue, 27 Jul 2021 17:16:07 +0300
> > > Andy Shevchenko <[e-mail]> wrote:
> > >     
> > > > On Tue, Jul 27, 2021 at 4:52 PM Jonathan Cameron
> > > > <[e-mail]> wrote:      
> > > > > On Sun, 25 Jul 2021 23:33:12 +0300
> > > > > Andy Shevchenko <[e-mail]> wrote:        
> > > > > > On Sun, Jul 25, 2021 at 8:22 PM Jonathan Cameron <[e-mail]> wrote:        
> > > > 
> > > > ...
> > > >       
> > > > > > > -       for_each_available_child_of_node(np, child) {
> > > > > > > +       device_for_each_child_node(dev, child) {        
> > > > > >
> > > > > > Isn't this
> > > > > >   fwnode_for_each_available_child_node()
> > > > > > better to use?        
> > > > >
> > > > > Given we would be extracting the fwnode just to call this
> > > > > loop, I'd say no, device version makes more sense..
> > > > >        
> > > > > >
> > > > > > ...
> > > > > >
> > > > > > So the gaps I see are
> > > > > >   device_get_available_child_node_count()
> > > > > > and
> > > > > >   device_for_each_available_child_node()        
> > > > >
> > > > > Do we then fix the fact that
> > > > > device_for_each_child_node() will call the _available() form
> > > > > for device tree?  That seems inconsistent currently and
> > > > > I was assuming that was deliberate...        
> > > > 
> > > > I'm not sure I got your point. Mine (see below) is to add the APIs
> > > > that you want to use as a direct replacement of the corresponding OF
> > > > counterparts.      
> > > +CC Rafael,    
> > 
> > Rafael, if you have a chance to give input on the questions below it would
> > be much appreciated.  
> 
> Rafael, if you have a chance to look at this it would be great.



> > 
> > Thanks,
> > 
> > Jonathan
> >   
> > > 
> > > The oddity is that device_for_each_child_node() is a direct replacement
> > > of the for_each_available_child_of_node() other than the obvious
> > > use of device rather than the of node.
> > > 
> > > https://elixir.bootlin.com/linux/v5.14-rc3/source/drivers/of/property.c#L939
> > > 
> > > static struct fwnode_handle *
> > > of_fwnode_get_next_child_node(const struct fwnode_handle *fwnode,
> > > 			      struct fwnode_handle *child)
> > > {
> > > 	return of_fwnode_handle(of_get_next_available_child(to_of_node(fwnode),
> > > 							    to_of_node(child)));
> > > }
> > > 
> > > So the question becomes whether there is any desire at all to have a
> > > version of the device_for_each_child_node() that does not check
> > > if it is available or not.
> > > 
> > > Looks like it goes all the way back.  Rafael, any comment on why the available
> > > for is used here and whether it makes sense to introduce separate
> > > versions for looping over children that cover the _available_ and everything
> > > cases?
> > > 
> > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/base/property.c?id=8a0662d9ed2968e1186208336a8e1fab3fdfea63
> > > 
> > > I'm kind of assuming this was deliberate as we don't want to encourage
> > > accessing disabled firmware nodes.
> > > 
> > > Jonathan
> > >     
> > > >       
> > > > > > Both of them I think are easy to add and avoid possible breakage.        
> > > > 
> > > >       
> > >     
> >   
>

Patch

diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c
index b2e49386d7dc..bbb9830e13c2 100644
--- a/drivers/iio/adc/ad7124.c
+++ b/drivers/iio/adc/ad7124.c
@@ -14,7 +14,7 @@ 
 #include <linux/kernel.h>
 #include <linux/kfifo.h>
 #include <linux/module.h>
-#include <linux/of_device.h>
+#include <linux/property.h>
 #include <linux/regulator/consumer.h>
 #include <linux/spi/spi.h>
 
@@ -733,18 +733,20 @@  static int ad7124_check_chip_id(struct ad7124_state *st)
 	return 0;
 }
 
-static int ad7124_of_parse_channel_config(struct iio_dev *indio_dev,
-					  struct device_node *np)
+static int ad7124_parse_channel_config(struct iio_dev *indio_dev,
+				       struct device *dev)
 {
 	struct ad7124_state *st = iio_priv(indio_dev);
 	struct ad7124_channel_config *cfg;
 	struct ad7124_channel *channels;
-	struct device_node *child;
+	struct fwnode_handle *child;
 	struct iio_chan_spec *chan;
 	unsigned int ain[2], channel = 0, tmp;
 	int ret;
 
-	st->num_channels = of_get_available_child_count(np);
+	device_for_each_child_node(dev, child)
+		st->num_channels++;
+
 	if (!st->num_channels) {
 		dev_err(indio_dev->dev.parent, "no channel children\n");
 		return -ENODEV;
@@ -764,9 +766,8 @@  static int ad7124_of_parse_channel_config(struct iio_dev *indio_dev,
 	indio_dev->num_channels = st->num_channels;
 	st->channels = channels;
 
-	for_each_available_child_of_node(np, child) {
-
-		ret = of_property_read_u32(child, "reg", &channel);
+	device_for_each_child_node(dev, child) {
+		ret = fwnode_property_read_u32(child, "reg", &channel);
 		if (ret)
 			goto err;
 
@@ -779,8 +780,8 @@  static int ad7124_of_parse_channel_config(struct iio_dev *indio_dev,
 
 		cfg = &st->channels[channel].cfg;
 
-		ret = of_property_read_u32_array(child, "diff-channels",
-						 ain, 2);
+		ret = fwnode_property_read_u32_array(child, "diff-channels",
+						     ain, 2);
 		if (ret)
 			goto err;
 
@@ -788,16 +789,16 @@  static int ad7124_of_parse_channel_config(struct iio_dev *indio_dev,
 		st->channels[channel].ain = AD7124_CHANNEL_AINP(ain[0]) |
 						  AD7124_CHANNEL_AINM(ain[1]);
 
-		cfg->bipolar = of_property_read_bool(child, "bipolar");
+		cfg->bipolar = fwnode_property_read_bool(child, "bipolar");
 
-		ret = of_property_read_u32(child, "adi,reference-select", &tmp);
+		ret = fwnode_property_read_u32(child, "adi,reference-select", &tmp);
 		if (ret)
 			cfg->refsel = AD7124_INT_REF;
 		else
 			cfg->refsel = tmp;
 
-		cfg->buf_positive = of_property_read_bool(child, "adi,buffered-positive");
-		cfg->buf_negative = of_property_read_bool(child, "adi,buffered-negative");
+		cfg->buf_positive = fwnode_property_read_bool(child, "adi,buffered-positive");
+		cfg->buf_negative = fwnode_property_read_bool(child, "adi,buffered-negative");
 
 		chan[channel] = ad7124_channel_template;
 		chan[channel].address = channel;
@@ -808,7 +809,7 @@  static int ad7124_of_parse_channel_config(struct iio_dev *indio_dev,
 
 	return 0;
 err:
-	of_node_put(child);
+	fwnode_handle_put(child);
 
 	return ret;
 }
@@ -875,7 +876,7 @@  static int ad7124_probe(struct spi_device *spi)
 	struct iio_dev *indio_dev;
 	int i, ret;
 
-	info = of_device_get_match_data(&spi->dev);
+	info = device_get_match_data(&spi->dev);
 	if (!info)
 		return -ENODEV;
 
@@ -893,7 +894,7 @@  static int ad7124_probe(struct spi_device *spi)
 	indio_dev->modes = INDIO_DIRECT_MODE;
 	indio_dev->info = &ad7124_info;
 
-	ret = ad7124_of_parse_channel_config(indio_dev, spi->dev.of_node);
+	ret = ad7124_parse_channel_config(indio_dev, &spi->dev);
 	if (ret < 0)
 		return ret;