diff mbox series

[v6,3/3] of/property: Add of_fwnode_name()

Message ID 20181211131451.52832-4-heikki.krogerus@linux.intel.com
State Rejected, archived
Headers show
Series device property: Add fwnode_get_name() helper | expand

Checks

Context Check Description
robh/checkpatch success

Commit Message

Heikki Krogerus Dec. 11, 2018, 1:14 p.m. UTC
This implements get_name fwnode op for DT.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/of/property.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

Comments

Rafael J. Wysocki Dec. 14, 2018, 9:36 a.m. UTC | #1
On Tue, Dec 11, 2018 at 2:15 PM Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
>
> This implements get_name fwnode op for DT.
>
> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

Rob, any objections here?

> ---
>  drivers/of/property.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
>
> diff --git a/drivers/of/property.c b/drivers/of/property.c
> index 08430031bd28..5c10fdded473 100644
> --- a/drivers/of/property.c
> +++ b/drivers/of/property.c
> @@ -823,6 +823,18 @@ static void of_fwnode_put(struct fwnode_handle *fwnode)
>         of_node_put(to_of_node(fwnode));
>  }
>
> +static int of_fwnode_get_name(const struct fwnode_handle *fwnode, char *buf,
> +                             size_t len)
> +{
> +       const struct device_node *node = to_of_node(fwnode);
> +
> +       if (len < snprintf(NULL, 0, "%pOFn", node) + 1)
> +               return -EOVERFLOW;
> +
> +       sprintf(buf, "%pOFn", node);
> +       return 0;
> +}
> +
>  static bool of_fwnode_device_is_available(const struct fwnode_handle *fwnode)
>  {
>         return of_device_is_available(to_of_node(fwnode));
> @@ -987,6 +999,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,
> +       .get_name = of_fwnode_get_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,
> --
> 2.19.2
>
Rob Herring (Arm) Dec. 14, 2018, 5:46 p.m. UTC | #2
On Fri, Dec 14, 2018 at 3:36 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Tue, Dec 11, 2018 at 2:15 PM Heikki Krogerus
> <heikki.krogerus@linux.intel.com> wrote:
> >
> > This implements get_name fwnode op for DT.
> >
> > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
>
> Rob, any objections here?

Yes, what I said in v5.

At one point this series had a user which was finding a matching node
by name. Now there is no user, so I can't really say whether this API
makes sense or not.

There's generally only 2 uses for 'name' at least for DT. The first is
node matching by name. ICYMI, every case of that in the tree is now
replaced with of_node_name_eq(). Now the details of matching by node
name are contained within the DT API. The 2nd use of node names is
just informational purposes or to fill some other structure's name
string (e.g. struct resource). In these cases, device_node.full_name
can be used. The only difference is full_name includes the unit
address ('@1234' part) if there is one. For these uses, the kernel
shouldn't generally care what the name actually is, but that's not
really enforceable.

Rob
Heikki Krogerus Dec. 17, 2018, 12:15 p.m. UTC | #3
On Fri, Dec 14, 2018 at 11:46:02AM -0600, Rob Herring wrote:
> On Fri, Dec 14, 2018 at 3:36 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> >
> > On Tue, Dec 11, 2018 at 2:15 PM Heikki Krogerus
> > <heikki.krogerus@linux.intel.com> wrote:
> > >
> > > This implements get_name fwnode op for DT.
> > >
> > > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> >
> > Rob, any objections here?
> 
> Yes, what I said in v5.

I did address your comment in v5? The helper no longer tries to read
"name" property if the callback fails.

> At one point this series had a user which was finding a matching node
> by name. Now there is no user, so I can't really say whether this API
> makes sense or not.

That is a fair point. I choose to not to convert
fwnode_get_named_child_node() to use this helper because it would mean
we would have to allocate the buffer there, and I don't know if it's
worth it.

Let's forget about this for now. I was planning to use this helper for
matching a requested remote-endpoint (graph) device, but I'm not sure
if the node name is usable there.

I'll resend these as part of a real user for the API, if I ever have
one.


thanks,
diff mbox series

Patch

diff --git a/drivers/of/property.c b/drivers/of/property.c
index 08430031bd28..5c10fdded473 100644
--- a/drivers/of/property.c
+++ b/drivers/of/property.c
@@ -823,6 +823,18 @@  static void of_fwnode_put(struct fwnode_handle *fwnode)
 	of_node_put(to_of_node(fwnode));
 }
 
+static int of_fwnode_get_name(const struct fwnode_handle *fwnode, char *buf,
+			      size_t len)
+{
+	const struct device_node *node = to_of_node(fwnode);
+
+	if (len < snprintf(NULL, 0, "%pOFn", node) + 1)
+		return -EOVERFLOW;
+
+	sprintf(buf, "%pOFn", node);
+	return 0;
+}
+
 static bool of_fwnode_device_is_available(const struct fwnode_handle *fwnode)
 {
 	return of_device_is_available(to_of_node(fwnode));
@@ -987,6 +999,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,
+	.get_name = of_fwnode_get_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,