Re: [PATCH] of: property: Add device link support for interrupt-parent, dmas and -gpio(s)

[Saravana Kannan <saravanak@google.com>]

On Fri, Nov 22, 2019 at 5:35 AM Rob Herring <robh+dt@kernel.org> wrote:
>
> On Wed, Nov 20, 2019 at 1:13 AM Saravana Kannan <saravanak@google.com> wrote:
> >
> > Add support for creating device links out of more DT properties.
> >
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Cc: Vinod Koul <vkoul@kernel.org>
> > Cc: Linus Walleij <linus.walleij@linaro.org>
> > Signed-off-by: Saravana Kannan <saravanak@google.com>
> > ---
> >  drivers/of/property.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> >
> > diff --git a/drivers/of/property.c b/drivers/of/property.c
> > index 0fa04692e3cc..dedbf82da838 100644
> > --- a/drivers/of/property.c
> > +++ b/drivers/of/property.c
> > @@ -1188,7 +1188,11 @@ DEFINE_SIMPLE_PROP(interconnects, "interconnects", "#interconnect-cells")
> >  DEFINE_SIMPLE_PROP(iommus, "iommus", "#iommu-cells")
> >  DEFINE_SIMPLE_PROP(mboxes, "mboxes", "#mbox-cells")
> >  DEFINE_SIMPLE_PROP(io_channels, "io-channel", "#io-channel-cells")
> > +DEFINE_SIMPLE_PROP(interrupt_parent, "interrupt-parent", NULL)
>
> This one is not going to work most of the time (ignoring the fact that
> the primary controller doesn't have a struct device) because the
> interrupt-parent is typically in a parent node.

Just to make sure, I'm not parsing this property incorrectly, right?

Are you saying it's listed at the parent of a bunch of devices and the
interrupt-parent is inherited and won't really create device links for
those child devices?
I mainly added this to make sure IRQ controllers are probed in the
right order. Also, if this delays the parent device probe, by the time
the child devices are added, the interrupt parent most likely would
already be probed.

> You could make it work
> by specifying 'interrupt-parent' in every node, but that's not a
> pattern I want to encourage.

I'm trying to take care of the basic per-device properties first.
Adding support for inherited properties isn't too difficult, I just
need to get to those at some point. Also, for inherited properties, we
can't really block probing because the child device might not depend
on that resource. Inherited properties are mainly relevant only for
sync_state() callbacks.

> There's also all the other ways the
> parent can be determined. Any parent node with 'interrupt-controller'
> or 'interrupt-map' property is the parent. And there's
> 'interrupts-extended' too.

Now I'm confused. Not sure if you are referring to actual device
parent now or if you are talking about an "interrupt supplier".

-Saravana

>
> > +DEFINE_SIMPLE_PROP(dmas, "dmas", "#dma-cells")
> >  DEFINE_SUFFIX_PROP(regulators, "-supply", NULL)
> > +DEFINE_SUFFIX_PROP(gpio, "-gpio", "#gpio-cells")
> > +DEFINE_SUFFIX_PROP(gpios, "-gpios", "#gpio-cells")
> >
> >  static const struct supplier_bindings of_supplier_bindings[] = {
> >         { .parse_prop = parse_clocks, },
> > @@ -1196,7 +1200,11 @@ static const struct supplier_bindings of_supplier_bindings[] = {
> >         { .parse_prop = parse_iommus, },
> >         { .parse_prop = parse_mboxes, },
> >         { .parse_prop = parse_io_channels, },
> > +       { .parse_prop = parse_interrupt_parent, },
> > +       { .parse_prop = parse_dmas, },
> >         { .parse_prop = parse_regulators, },
> > +       { .parse_prop = parse_gpio, },
> > +       { .parse_prop = parse_gpios, },
> >         {}
> >  };
> >
> > --
> > 2.24.0.432.g9d3f5f5b63-goog
> >