[2/2] of/platform: Disable generic device linking code for PowerPC

PowerPC platforms don't use the generic of/platform code to populate the
devices from DT.  Therefore the generic device linking code is never used
in PowerPC.  Compile it out to avoid warning about unused functions.

If a specific PowerPC platform wants to use this code in the future,
bringing this back for PowerPC would be trivial. We'll just need to export
of_link_to_suppliers() and then let the machine specific files do the
linking as they populate the devices from DT.

Signed-off-by: Saravana Kannan <saravanak@google.com>
---
 drivers/of/platform.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

On Tue, Aug 6, 2019 at 1:27 PM Saravana Kannan <saravanak@google.com> wrote:
>
> PowerPC platforms don't use the generic of/platform code to populate the
> devices from DT.

Yes, they do.

> Therefore the generic device linking code is never used
> in PowerPC.  Compile it out to avoid warning about unused functions.

I'd prefer this get disabled on PPC using 'if (IS_ENABLED(CONFIG_PPC))
return' rather than #ifdefs.

>
> If a specific PowerPC platform wants to use this code in the future,
> bringing this back for PowerPC would be trivial. We'll just need to export
> of_link_to_suppliers() and then let the machine specific files do the
> linking as they populate the devices from DT.
>
> Signed-off-by: Saravana Kannan <saravanak@google.com>
> ---
>  drivers/of/platform.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> index f68de5c4aeff..a2a4e4b79d43 100644
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -506,6 +506,7 @@ int of_platform_default_populate(struct device_node *root,
>  }
>  EXPORT_SYMBOL_GPL(of_platform_default_populate);
>
> +#ifndef CONFIG_PPC
>  static bool of_link_is_valid(struct device_node *con, struct device_node *sup)
>  {
>         of_node_get(sup);
> @@ -683,7 +684,6 @@ static int of_link_to_suppliers(struct device *dev)
>         return __of_link_to_suppliers(dev, dev->of_node);
>  }
>
> -#ifndef CONFIG_PPC
>  static const struct of_device_id reserved_mem_matches[] = {
>         { .compatible = "qcom,rmtfs-mem" },
>         { .compatible = "qcom,cmd-db" },
> --
> 2.22.0.770.g0f2c4a37fd-goog
>

On Tue, Aug 6, 2019 at 2:27 PM Rob Herring <robh+dt@kernel.org> wrote:
>
> On Tue, Aug 6, 2019 at 1:27 PM Saravana Kannan <saravanak@google.com> wrote:
> >
> > PowerPC platforms don't use the generic of/platform code to populate the
> > devices from DT.
>
> Yes, they do.

No they don't. My wording could be better, but they don't use
of_platform_default_populate_init()
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/of/platform.c#n511

>
> > Therefore the generic device linking code is never used
> > in PowerPC.  Compile it out to avoid warning about unused functions.
>
> I'd prefer this get disabled on PPC using 'if (IS_ENABLED(CONFIG_PPC))
> return' rather than #ifdefs.

I'm just moving the existing ifndef some lines above. I don't want to
go change existing #ifndef in this patch. Maybe that should be a
separate patch series that goes and fixes all such code in drivers/of/
or driver/

-Saravana

>
> >
> > If a specific PowerPC platform wants to use this code in the future,
> > bringing this back for PowerPC would be trivial. We'll just need to export
> > of_link_to_suppliers() and then let the machine specific files do the
> > linking as they populate the devices from DT.
> >
> > Signed-off-by: Saravana Kannan <saravanak@google.com>
> > ---
> >  drivers/of/platform.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> > index f68de5c4aeff..a2a4e4b79d43 100644
> > --- a/drivers/of/platform.c
> > +++ b/drivers/of/platform.c
> > @@ -506,6 +506,7 @@ int of_platform_default_populate(struct device_node *root,
> >  }
> >  EXPORT_SYMBOL_GPL(of_platform_default_populate);
> >
> > +#ifndef CONFIG_PPC
> >  static bool of_link_is_valid(struct device_node *con, struct device_node *sup)
> >  {
> >         of_node_get(sup);
> > @@ -683,7 +684,6 @@ static int of_link_to_suppliers(struct device *dev)
> >         return __of_link_to_suppliers(dev, dev->of_node);
> >  }
> >
> > -#ifndef CONFIG_PPC
> >  static const struct of_device_id reserved_mem_matches[] = {
> >         { .compatible = "qcom,rmtfs-mem" },
> >         { .compatible = "qcom,cmd-db" },
> > --
> > 2.22.0.770.g0f2c4a37fd-goog
> >

On Tue, Aug 6, 2019 at 3:04 PM Saravana Kannan <saravanak@google.com> wrote:
>
> On Tue, Aug 6, 2019 at 2:27 PM Rob Herring <robh+dt@kernel.org> wrote:
> >
> > On Tue, Aug 6, 2019 at 1:27 PM Saravana Kannan <saravanak@google.com> wrote:
> > >
> > > PowerPC platforms don't use the generic of/platform code to populate the
> > > devices from DT.
> >
> > Yes, they do.
>
> No they don't. My wording could be better, but they don't use
> of_platform_default_populate_init()
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/of/platform.c#n511
>
> >
> > > Therefore the generic device linking code is never used
> > > in PowerPC.  Compile it out to avoid warning about unused functions.
> >
> > I'd prefer this get disabled on PPC using 'if (IS_ENABLED(CONFIG_PPC))
> > return' rather than #ifdefs.
>
> I'm just moving the existing ifndef some lines above. I don't want to
> go change existing #ifndef in this patch. Maybe that should be a
> separate patch series that goes and fixes all such code in drivers/of/
> or driver/

Bump. Thoughts? I don't think changing the existing if(n)defs should
be part of this patch series.

-Saravana

On Tue, Aug 6, 2019 at 4:04 PM Saravana Kannan <saravanak@google.com> wrote:
>
> On Tue, Aug 6, 2019 at 2:27 PM Rob Herring <robh+dt@kernel.org> wrote:
> >
> > On Tue, Aug 6, 2019 at 1:27 PM Saravana Kannan <saravanak@google.com> wrote:
> > >
> > > PowerPC platforms don't use the generic of/platform code to populate the
> > > devices from DT.
> >
> > Yes, they do.
>
> No they don't. My wording could be better, but they don't use
> of_platform_default_populate_init()
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/of/platform.c#n511

Right, but the rest of the of/platform code is used (guess where it
got moved here from?).

> > > Therefore the generic device linking code is never used
> > > in PowerPC.  Compile it out to avoid warning about unused functions.
> >
> > I'd prefer this get disabled on PPC using 'if (IS_ENABLED(CONFIG_PPC))
> > return' rather than #ifdefs.
>
> I'm just moving the existing ifndef some lines above. I don't want to
> go change existing #ifndef in this patch. Maybe that should be a
> separate patch series that goes and fixes all such code in drivers/of/
> or driver/

So the initcall was originally just supposed to call
of_platform_default_populate(), but it's grown beyond that. That could
make things fragile as it is possible for platforms to call
of_platform_populate() (directly or indirectly) before
of_platform_default_populate_init(). That was supposed to work, but
now I think it's getting more fragile.

Anyways, I guess this patch is fine for now.

Rob

On Wed, Aug 14, 2019 at 4:41 PM Rob Herring <robh+dt@kernel.org> wrote:
>
> On Tue, Aug 6, 2019 at 4:04 PM Saravana Kannan <saravanak@google.com> wrote:
> >
> > On Tue, Aug 6, 2019 at 2:27 PM Rob Herring <robh+dt@kernel.org> wrote:
> > >
> > > On Tue, Aug 6, 2019 at 1:27 PM Saravana Kannan <saravanak@google.com> wrote:
> > > >
> > > > PowerPC platforms don't use the generic of/platform code to populate the
> > > > devices from DT.
> > >
> > > Yes, they do.
> >
> > No they don't. My wording could be better, but they don't use
> > of_platform_default_populate_init()
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/of/platform.c#n511
>
> Right, but the rest of the of/platform code is used (guess where it
> got moved here from?).
>
> > > > Therefore the generic device linking code is never used
> > > > in PowerPC.  Compile it out to avoid warning about unused functions.
> > >
> > > I'd prefer this get disabled on PPC using 'if (IS_ENABLED(CONFIG_PPC))
> > > return' rather than #ifdefs.
> >
> > I'm just moving the existing ifndef some lines above. I don't want to
> > go change existing #ifndef in this patch. Maybe that should be a
> > separate patch series that goes and fixes all such code in drivers/of/
> > or driver/
>
> So the initcall was originally just supposed to call
> of_platform_default_populate(), but it's grown beyond that. That could
> make things fragile as it is possible for platforms to call
> of_platform_populate() (directly or indirectly) before
> of_platform_default_populate_init(). That was supposed to work, but
> now I think it's getting more fragile.

Can you clarify what's wrong with of_platfrom_populate() being called
before of_platform_default_populate_init()? If that's what a platform
wants to do, they can do it? I have some thoughts of my own, but I
want to hear yours.
In any case, I'd be happy to help  clean up this initcall if you can
give me a direction to take it in.

> Anyways, I guess this patch is fine for now.

Thanks.

-Saravana

On Thu, Aug 15, 2019 at 9:04 PM Saravana Kannan <saravanak@google.com> wrote:
>
> On Wed, Aug 14, 2019 at 4:41 PM Rob Herring <robh+dt@kernel.org> wrote:
> >
> > On Tue, Aug 6, 2019 at 4:04 PM Saravana Kannan <saravanak@google.com> wrote:
> > >
> > > On Tue, Aug 6, 2019 at 2:27 PM Rob Herring <robh+dt@kernel.org> wrote:
> > > >
> > > > On Tue, Aug 6, 2019 at 1:27 PM Saravana Kannan <saravanak@google.com> wrote:
> > > > >
> > > > > PowerPC platforms don't use the generic of/platform code to populate the
> > > > > devices from DT.
> > > >
> > > > Yes, they do.
> > >
> > > No they don't. My wording could be better, but they don't use
> > > of_platform_default_populate_init()
> > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/of/platform.c#n511
> >
> > Right, but the rest of the of/platform code is used (guess where it
> > got moved here from?).
> >
> > > > > Therefore the generic device linking code is never used
> > > > > in PowerPC.  Compile it out to avoid warning about unused functions.
> > > >
> > > > I'd prefer this get disabled on PPC using 'if (IS_ENABLED(CONFIG_PPC))
> > > > return' rather than #ifdefs.
> > >
> > > I'm just moving the existing ifndef some lines above. I don't want to
> > > go change existing #ifndef in this patch. Maybe that should be a
> > > separate patch series that goes and fixes all such code in drivers/of/
> > > or driver/
> >
> > So the initcall was originally just supposed to call
> > of_platform_default_populate(), but it's grown beyond that. That could
> > make things fragile as it is possible for platforms to call
> > of_platform_populate() (directly or indirectly) before
> > of_platform_default_populate_init(). That was supposed to work, but
> > now I think it's getting more fragile.
>
> Can you clarify what's wrong with of_platfrom_populate() being called
> before of_platform_default_populate_init()? If that's what a platform
> wants to do, they can do it? I have some thoughts of my own, but I
> want to hear yours.

Really, I'd like to get rid of platforms doing their own calls. That's
mostly an arm32 issue. Most of what's left are either platforms using
auxdata which was supposed to be a transition thing or ones that set a
parent device (soc_device). The former takes work to finish converting
platforms to DT and I don't know what to do for the latter other than
always or never set a parent device. Also, I know there's an issue on
atmel where we can't remove their of_platform_populate call because it
changes the probe order and breaks their pinctrl and gpio driver (I
started a patch for that...).

Rob