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

[Jonathan Cameron <jic23@jic23.retrosnub.co.uk>]

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 <jic23@kernel.org> wrote:

> On Sun, 15 Aug 2021 17:09:51 +0100
> Jonathan Cameron <jic23@kernel.org> wrote:
> 
> > On Tue, 27 Jul 2021 19:20:13 +0100
> > Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:
> >   
> > > On Tue, 27 Jul 2021 17:16:07 +0300
> > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > >     
> > > > On Tue, Jul 27, 2021 at 4:52 PM Jonathan Cameron
> > > > <Jonathan.Cameron@huawei.com> wrote:      
> > > > > On Sun, 25 Jul 2021 23:33:12 +0300
> > > > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:        
> > > > > > On Sun, Jul 25, 2021 at 8:22 PM Jonathan Cameron <jic23@kernel.org> 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.        
> > > > 
> > > >       
> > >     
> >   
>