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