Re: [PATCH 3/4] of/property: Introduce of_fwnode_name()

Re: [PATCH 3/4] of/property: Introduce of_fwnode_name()

[Rob Herring]

On Mon, Nov 5, 2018 at 3:17 AM Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
>
> Instead of always being forced to read the "name" property
> in fwnode_name() with of_nodes, implementing the fwnode
> operation meant for getting the node name.
>
> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> Cc: Rob Herring <robh@kernel.org>
> ---
>  drivers/of/property.c | 6 ++++++

Please Cc the DT list for DT changes.

>  1 file changed, 6 insertions(+)
>
> diff --git a/drivers/of/property.c b/drivers/of/property.c
> index f46828e3b082..ac7b0b6c2d4d 100644
> --- a/drivers/of/property.c
> +++ b/drivers/of/property.c
> @@ -823,6 +823,11 @@ static void of_fwnode_put(struct fwnode_handle *fwnode)
>         of_node_put(to_of_node(fwnode));
>  }
>
> +static const char *of_fwnode_name(const struct fwnode_handle *fwnode)
> +{
> +       return to_of_node(fwnode)->name;

I'm trying to get rid of the DT name ptr, so please don't add one. You
can use of_node_full_name() here instead if "<name>@<unit-address>"
instead of <name> is fine. Otherwise, you've got to allocate your own
storage and use "%pOFn" printf specifier.

Rob

Re: [PATCH 3/4] of/property: Introduce of_fwnode_name()

[Heikki Krogerus]

On Mon, Nov 05, 2018 at 12:50:02PM -0600, Rob Herring wrote:
> On Mon, Nov 5, 2018 at 3:17 AM Heikki Krogerus
> <heikki.krogerus@linux.intel.com> wrote:
> >
> > Instead of always being forced to read the "name" property
> > in fwnode_name() with of_nodes, implementing the fwnode
> > operation meant for getting the node name.
> >
> > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > Cc: Rob Herring <robh@kernel.org>
> > ---
> >  drivers/of/property.c | 6 ++++++
> 
> Please Cc the DT list for DT changes.

OK.

> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/drivers/of/property.c b/drivers/of/property.c
> > index f46828e3b082..ac7b0b6c2d4d 100644
> > --- a/drivers/of/property.c
> > +++ b/drivers/of/property.c
> > @@ -823,6 +823,11 @@ static void of_fwnode_put(struct fwnode_handle *fwnode)
> >         of_node_put(to_of_node(fwnode));
> >  }
> >
> > +static const char *of_fwnode_name(const struct fwnode_handle *fwnode)
> > +{
> > +       return to_of_node(fwnode)->name;
> 
> I'm trying to get rid of the DT name ptr, so please don't add one. You
> can use of_node_full_name() here instead if "<name>@<unit-address>"
> instead of <name> is fine. Otherwise, you've got to allocate your own
> storage and use "%pOFn" printf specifier.

OK, I'll use of_node_full_name().

thanks,

-- 
heikki

Re: [PATCH 3/4] of/property: Introduce of_fwnode_name()

[Andy Shevchenko]

On Mon, Nov 05, 2018 at 12:50:02PM -0600, Rob Herring wrote:
> On Mon, Nov 5, 2018 at 3:17 AM Heikki Krogerus
> <heikki.krogerus@linux.intel.com> wrote:

> > +static const char *of_fwnode_name(const struct fwnode_handle *fwnode)
> > +{
> > +       return to_of_node(fwnode)->name;
> 
> I'm trying to get rid of the DT name ptr, so please don't add one. You
> can use of_node_full_name() here instead if "<name>@<unit-address>"
> instead of <name> is fine. Otherwise, you've got to allocate your own
> storage and use "%pOFn" printf specifier.

If we do this here, we will change a behaviour of the entire set of
of_fwnode_get_named_child_node() users.

I think this is out of scope of the series.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/4] of/property: Introduce of_fwnode_name()

[Heikki Krogerus]

On Tue, Nov 06, 2018 at 12:58:03PM +0200, Andy Shevchenko wrote:
> On Mon, Nov 05, 2018 at 12:50:02PM -0600, Rob Herring wrote:
> > On Mon, Nov 5, 2018 at 3:17 AM Heikki Krogerus
> > <heikki.krogerus@linux.intel.com> wrote:
> 
> > > +static const char *of_fwnode_name(const struct fwnode_handle *fwnode)
> > > +{
> > > +       return to_of_node(fwnode)->name;
> > 
> > I'm trying to get rid of the DT name ptr, so please don't add one. You
> > can use of_node_full_name() here instead if "<name>@<unit-address>"
> > instead of <name> is fine. Otherwise, you've got to allocate your own
> > storage and use "%pOFn" printf specifier.
> 
> If we do this here, we will change a behaviour of the entire set of
> of_fwnode_get_named_child_node() users.
> 
> I think this is out of scope of the series.

You have a point. We must use the same member that was used in
of_fwnode_get_named_child_node().

The goal of this series if most likely not clear from this patch
alone, so I'll send a second version and make sure to CC the DT list
and Rob.

But in any case, I'll keep this part as it is.


thanks,

-- 
heikki

Re: [PATCH 3/4] of/property: Introduce of_fwnode_name()

[Rob Herring]

On Tue, Nov 6, 2018 at 6:27 AM Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
>
> On Tue, Nov 06, 2018 at 12:58:03PM +0200, Andy Shevchenko wrote:
> > On Mon, Nov 05, 2018 at 12:50:02PM -0600, Rob Herring wrote:
> > > On Mon, Nov 5, 2018 at 3:17 AM Heikki Krogerus
> > > <heikki.krogerus@linux.intel.com> wrote:
> >
> > > > +static const char *of_fwnode_name(const struct fwnode_handle *fwnode)
> > > > +{
> > > > +       return to_of_node(fwnode)->name;
> > >
> > > I'm trying to get rid of the DT name ptr, so please don't add one. You
> > > can use of_node_full_name() here instead if "<name>@<unit-address>"
> > > instead of <name> is fine. Otherwise, you've got to allocate your own
> > > storage and use "%pOFn" printf specifier.
> >
> > If we do this here, we will change a behaviour of the entire set of
> > of_fwnode_get_named_child_node() users.
> >
> > I think this is out of scope of the series.

No, because you are adding a firmware op for something that's going away.

> You have a point. We must use the same member that was used in
> of_fwnode_get_named_child_node().
>
> The goal of this series if most likely not clear from this patch
> alone, so I'll send a second version and make sure to CC the DT list
> and Rob.

Looking at patch 4, if matching the name is what you want to do, then
use the DT name matching functions. They were added in 4.19.

Rob

Re: [PATCH 3/4] of/property: Introduce of_fwnode_name()

[Andy Shevchenko]

On Tue, Nov 06, 2018 at 07:18:14AM -0600, Rob Herring wrote:
> On Tue, Nov 6, 2018 at 6:27 AM Heikki Krogerus
> <heikki.krogerus@linux.intel.com> wrote:
> >
> > On Tue, Nov 06, 2018 at 12:58:03PM +0200, Andy Shevchenko wrote:
> > > On Mon, Nov 05, 2018 at 12:50:02PM -0600, Rob Herring wrote:
> > > > On Mon, Nov 5, 2018 at 3:17 AM Heikki Krogerus
> > > > <heikki.krogerus@linux.intel.com> wrote:
> > >
> > > > > +static const char *of_fwnode_name(const struct fwnode_handle *fwnode)
> > > > > +{
> > > > > +       return to_of_node(fwnode)->name;
> > > >
> > > > I'm trying to get rid of the DT name ptr, so please don't add one. You
> > > > can use of_node_full_name() here instead if "<name>@<unit-address>"
> > > > instead of <name> is fine. Otherwise, you've got to allocate your own
> > > > storage and use "%pOFn" printf specifier.
> > >
> > > If we do this here, we will change a behaviour of the entire set of
> > > of_fwnode_get_named_child_node() users.
> > >
> > > I think this is out of scope of the series.
> 
> No, because you are adding a firmware op for something that's going away.

W/o your below explanation it wasn't obvious.

> 
> > You have a point. We must use the same member that was used in
> > of_fwnode_get_named_child_node().
> >
> > The goal of this series if most likely not clear from this patch
> > alone, so I'll send a second version and make sure to CC the DT list
> > and Rob.
> 
> Looking at patch 4, if matching the name is what you want to do, then
> use the DT name matching functions. They were added in 4.19.

Do you mean of_node_name_eq()?


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/4] of/property: Introduce of_fwnode_name()

[Rob Herring]

On Tue, Nov 6, 2018 at 8:28 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Tue, Nov 06, 2018 at 07:18:14AM -0600, Rob Herring wrote:
> > On Tue, Nov 6, 2018 at 6:27 AM Heikki Krogerus
> > <heikki.krogerus@linux.intel.com> wrote:
> > >
> > > On Tue, Nov 06, 2018 at 12:58:03PM +0200, Andy Shevchenko wrote:
> > > > On Mon, Nov 05, 2018 at 12:50:02PM -0600, Rob Herring wrote:
> > > > > On Mon, Nov 5, 2018 at 3:17 AM Heikki Krogerus
> > > > > <heikki.krogerus@linux.intel.com> wrote:
> > > >
> > > > > > +static const char *of_fwnode_name(const struct fwnode_handle *fwnode)
> > > > > > +{
> > > > > > +       return to_of_node(fwnode)->name;
> > > > >
> > > > > I'm trying to get rid of the DT name ptr, so please don't add one. You
> > > > > can use of_node_full_name() here instead if "<name>@<unit-address>"
> > > > > instead of <name> is fine. Otherwise, you've got to allocate your own
> > > > > storage and use "%pOFn" printf specifier.
> > > >
> > > > If we do this here, we will change a behaviour of the entire set of
> > > > of_fwnode_get_named_child_node() users.
> > > >
> > > > I think this is out of scope of the series.
> >
> > No, because you are adding a firmware op for something that's going away.
>
> W/o your below explanation it wasn't obvious.
>
> >
> > > You have a point. We must use the same member that was used in
> > > of_fwnode_get_named_child_node().
> > >
> > > The goal of this series if most likely not clear from this patch
> > > alone, so I'll send a second version and make sure to CC the DT list
> > > and Rob.
> >
> > Looking at patch 4, if matching the name is what you want to do, then
> > use the DT name matching functions. They were added in 4.19.
>
> Do you mean of_node_name_eq()?

Yes or maybe move further up and just retrieve child nodes by name.
There's a recent function Johan added for that too.
of_find_child_node_by_name IIRC.

Rob

Re: [PATCH 3/4] of/property: Introduce of_fwnode_name()

[Heikki Krogerus]

On Tue, Nov 06, 2018 at 07:18:14AM -0600, Rob Herring wrote:
> On Tue, Nov 6, 2018 at 6:27 AM Heikki Krogerus
> <heikki.krogerus@linux.intel.com> wrote:
> >
> > On Tue, Nov 06, 2018 at 12:58:03PM +0200, Andy Shevchenko wrote:
> > > On Mon, Nov 05, 2018 at 12:50:02PM -0600, Rob Herring wrote:
> > > > On Mon, Nov 5, 2018 at 3:17 AM Heikki Krogerus
> > > > <heikki.krogerus@linux.intel.com> wrote:
> > >
> > > > > +static const char *of_fwnode_name(const struct fwnode_handle *fwnode)
> > > > > +{
> > > > > +       return to_of_node(fwnode)->name;
> > > >
> > > > I'm trying to get rid of the DT name ptr, so please don't add one. You
> > > > can use of_node_full_name() here instead if "<name>@<unit-address>"
> > > > instead of <name> is fine. Otherwise, you've got to allocate your own
> > > > storage and use "%pOFn" printf specifier.
> > >
> > > If we do this here, we will change a behaviour of the entire set of
> > > of_fwnode_get_named_child_node() users.
> > >
> > > I think this is out of scope of the series.
> 
> No, because you are adding a firmware op for something that's going away.
> 
> > You have a point. We must use the same member that was used in
> > of_fwnode_get_named_child_node().
> >
> > The goal of this series if most likely not clear from this patch
> > alone, so I'll send a second version and make sure to CC the DT list
> > and Rob.
> 
> Looking at patch 4, if matching the name is what you want to do, then
> use the DT name matching functions. They were added in 4.19.

That is something that the of_fwnode_get_named_child_node() needs
to use (would have needed).

Regardless of what we do with that callback, fwnode_name() needs to
return the name in from that for example of_node_name_eq() takes as
the second parameter. So "node-name@unit-address" is not OK. Sorry for
not realizeing that before.

So I guess we need to either get the "node-name" from that full_name
member in of_fwnode_name() (Andy, are you OK with that?), or is there
already a helper that does it for us?


Thanks,

-- 
heikki

Re: [PATCH 3/4] of/property: Introduce of_fwnode_name()

[Andy Shevchenko]

On Tue, Nov 06, 2018 at 04:40:37PM +0200, Heikki Krogerus wrote:
> On Tue, Nov 06, 2018 at 07:18:14AM -0600, Rob Herring wrote:

> > Looking at patch 4, if matching the name is what you want to do, then
> > use the DT name matching functions. They were added in 4.19.
> 
> That is something that the of_fwnode_get_named_child_node() needs
> to use (would have needed).
> 
> Regardless of what we do with that callback, fwnode_name() needs to
> return the name in from that for example of_node_name_eq() takes as
> the second parameter. So "node-name@unit-address" is not OK. Sorry for
> not realizeing that before.
> 
> So I guess we need to either get the "node-name" from that full_name
> member in of_fwnode_name() (Andy, are you OK with that?), or is there
> already a helper that does it for us?

Looking into existing API I think we need something like

of_node_name_extract()

of_node_name_eq()
{
	name = of_node_name_extract();
	return strlen()...strncmp()...;
}

The question is who is going to allocate and free memory for the name out of it.

OTOH, of_fwnode_get_named_child_node() might need to copy that code which
brings the consistency issue (several places to maintain the same set of rules,
i.e. how we extract name out of full_name).

So, removal of name field shouldn't be done until we resolve the issue with
of_fwnode_get_named_child_node().

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/4] of/property: Introduce of_fwnode_name()

[Heikki Krogerus]

On Tue, Nov 06, 2018 at 04:55:37PM +0200, Andy Shevchenko wrote:
> On Tue, Nov 06, 2018 at 04:40:37PM +0200, Heikki Krogerus wrote:
> > On Tue, Nov 06, 2018 at 07:18:14AM -0600, Rob Herring wrote:
> 
> > > Looking at patch 4, if matching the name is what you want to do, then
> > > use the DT name matching functions. They were added in 4.19.
> > 
> > That is something that the of_fwnode_get_named_child_node() needs
> > to use (would have needed).
> > 
> > Regardless of what we do with that callback, fwnode_name() needs to
> > return the name in from that for example of_node_name_eq() takes as
> > the second parameter. So "node-name@unit-address" is not OK. Sorry for
> > not realizeing that before.
> > 
> > So I guess we need to either get the "node-name" from that full_name
> > member in of_fwnode_name() (Andy, are you OK with that?), or is there
> > already a helper that does it for us?
> 
> Looking into existing API I think we need something like
> 
> of_node_name_extract()
> 
> of_node_name_eq()
> {
> 	name = of_node_name_extract();
> 	return strlen()...strncmp()...;
> }
> 
> The question is who is going to allocate and free memory for the name out of it.

Maybe it would be best to just read the "name" device property in
fwnode_name() and not have of_fwnode_name at all.

> OTOH, of_fwnode_get_named_child_node() might need to copy that code which
> brings the consistency issue (several places to maintain the same set of rules,
> i.e. how we extract name out of full_name).
> 
> So, removal of name field shouldn't be done until we resolve the issue with
> of_fwnode_get_named_child_node().

thanks,

-- 
heikki

Re: [PATCH 3/4] of/property: Introduce of_fwnode_name()

[Andy Shevchenko]

On Tue, Nov 06, 2018 at 05:05:03PM +0200, Heikki Krogerus wrote:

> Maybe it would be best to just read the "name" device property in
> fwnode_name() and not have of_fwnode_name at all.

If it's a mandatory property or somehow its presence is guaranteed, it would work.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/4] of/property: Introduce of_fwnode_name()

[Rob Herring]

On Tue, Nov 6, 2018 at 9:53 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Tue, Nov 06, 2018 at 05:05:03PM +0200, Heikki Krogerus wrote:
>
> > Maybe it would be best to just read the "name" device property in
> > fwnode_name() and not have of_fwnode_name at all.
>
> If it's a mandatory property or somehow its presence is guaranteed, it would work.

It is currently, but after removing the name ptr, my current plan is
to remove the 'name' property too for FDT. On real OpenFirmware, it is
a real property so it will remain for sure in some cases.

Rob

Re: [PATCH 3/4] of/property: Introduce of_fwnode_name()

[Heikki Krogerus]

On Tue, Nov 06, 2018 at 12:13:30PM -0600, Rob Herring wrote:
> On Tue, Nov 6, 2018 at 9:53 AM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> >
> > On Tue, Nov 06, 2018 at 05:05:03PM +0200, Heikki Krogerus wrote:
> >
> > > Maybe it would be best to just read the "name" device property in
> > > fwnode_name() and not have of_fwnode_name at all.
> >
> > If it's a mandatory property or somehow its presence is guaranteed, it would work.
> 
> It is currently, but after removing the name ptr, my current plan is
> to remove the 'name' property too for FDT. On real OpenFirmware, it is
> a real property so it will remain for sure in some cases.

OK. I think we'll use the full_name after all.

thanks,

-- 
heikki