| Message ID | 20181105091727.25544-4-heikki.krogerus@linux.intel.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | device property: Add fwnode_name() helper | expand |
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
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,
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.
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,
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
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()?
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,
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().
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,
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.
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
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
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,
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; +} + static bool of_fwnode_device_is_available(const struct fwnode_handle *fwnode) { return of_device_is_available(to_of_node(fwnode)); @@ -987,6 +992,7 @@ of_fwnode_device_get_match_data(const struct fwnode_handle *fwnode, const struct fwnode_operations of_fwnode_ops = { .get = of_fwnode_get, .put = of_fwnode_put, + .name = of_fwnode_name, .device_is_available = of_fwnode_device_is_available, .device_get_match_data = of_fwnode_device_get_match_data, .property_present = of_fwnode_property_present,
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 ++++++ 1 file changed, 6 insertions(+)