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

[Jonathan Cameron <jic23@kernel.org>]

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.

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.    
> > 
> >   
>