[RFC] Add of_path property for all devices with a node

[RFC] Add of_path property for all devices with a node

[Benjamin Herrenschmidt]

Hey folks ! This is not (yet) a formal patch submission but...

So I've been annoyed lately with having a bunch of devices such as i2c
eeproms (for use by VPDs, server world !) and other bits and pieces that
I want to be able to identify from userspace, and possibly provide
additional data about from FW.

Basically, it boils down to correlating the sysfs device with the OF
tree device node, so that user space can use device-tree info such as
additional "location" or "label" (or whatever else we can come up with)
propreties to identify a given device, or get some attributes of use
about it, etc...

Now, so far, we've done that in some subsystem in a fairly ad-hoc basis
using "devspec" properties. For example, PCI creates them if it can
correlate the probed device with a DT node. Some powerpc specific busses
do that too.

However, i2c doesn't and it would be nice to have something more generic
since technically any device can have a corresponding device tree node.

So I came up with this patch, it seems to work well for me. I'm adding
an "of_path" attribute to not conflict with the existing "devspec" one
just for the sake of this experiment (plus "devspec" sucks). Long run,
we might want to use of_path and leave a "devspec" symlink to of_path on
the few busses that currently have devspec (pci and some powerpc
specific ones).

Comments ?

Cheers,
Ben.

Re: [RFC] Add of_path property for all devices with a node

[Benjamin Herrenschmidt]

On Fri, 2014-11-07 at 17:33 +1100, Benjamin Herrenschmidt wrote:

> So I came up with this patch, 

And here is the actual patch, which might help :-) It's pretty trivial
and small...

diff --git a/drivers/base/core.c b/drivers/base/core.c
index 20da3ad..dd0ee1b 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -26,6 +26,7 @@
 #include <linux/pm_runtime.h>
 #include <linux/netdevice.h>
 #include <linux/sysfs.h>
+#include <linux/of.h>
 
 #include "base.h"
 #include "power/power.h"
@@ -454,6 +455,23 @@ static ssize_t online_store(struct device *dev, struct device_attribute *attr,
 }
 static DEVICE_ATTR_RW(online);
 
+#ifdef CONFIG_OF
+
+static ssize_t of_path_show(struct device *dev, struct device_attribute *attr,
+			    char *buf)
+{
+	ssize_t s = 0;
+
+	device_lock(dev);
+	if (dev->of_node)
+		s = sprintf(buf, "%s\n", dev->of_node->full_name);
+	device_unlock(dev);
+	return s;
+}
+static DEVICE_ATTR_RO(of_path);
+
+#endif /* CONFIG_OF */
+
 int device_add_groups(struct device *dev, const struct attribute_group **groups)
 {
 	return sysfs_create_groups(&dev->kobj, groups);
@@ -487,15 +505,27 @@ static int device_add_attrs(struct device *dev)
 	if (error)
 		goto err_remove_type_groups;
 
+#ifdef CONFIG_OF
+	if (dev->of_node) {
+		error = device_create_file(dev, &dev_attr_of_path);
+		if (error)
+			goto err_remove_dev_groups;
+	}
+#endif /* CONFIG_OF */
+
 	if (device_supports_offline(dev) && !dev->offline_disabled) {
 		error = device_create_file(dev, &dev_attr_online);
 		if (error)
-			goto err_remove_dev_groups;
+			goto err_remove_of_path;
 	}
 
 	return 0;
 
+ err_remove_of_path:
+#ifdef CONFIG_OF
+	device_remove_file(dev, &dev_attr_of_path);
  err_remove_dev_groups:
+#endif
 	device_remove_groups(dev, dev->groups);
  err_remove_type_groups:
 	if (type)

Re: [RFC] Add of_path property for all devices with a node

[Benjamin Herrenschmidt]

On Fri, 2014-11-07 at 17:35 +1100, Benjamin Herrenschmidt wrote:
> On Fri, 2014-11-07 at 17:33 +1100, Benjamin Herrenschmidt wrote:
> 
> > So I came up with this patch, 
> 
> And here is the actual patch, which might help :-) It's pretty trivial
> and small...

So not much reactions here .. a bit more on IRC, where Olof suggested
that rather than a file containing a path, we could use a symlink since
the devtree is now in sysfs, so we can do relative links.

That means keeping the legacy "devspec" files in the few places where
they exist, possibly behind a deprecated-default-y config option (though
we might never be able to deprecate them...), but there aren't many.

Any preference either way ?

Cheers,
Ben.

Re: [RFC] Add of_path property for all devices with a node

[Rob Herring]

Adding Grant.

On Sun, Nov 9, 2014 at 11:17 PM, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
> On Fri, 2014-11-07 at 17:35 +1100, Benjamin Herrenschmidt wrote:
>> On Fri, 2014-11-07 at 17:33 +1100, Benjamin Herrenschmidt wrote:
>>
>> > So I came up with this patch,
>>
>> And here is the actual patch, which might help :-) It's pretty trivial
>> and small...
>
> So not much reactions here .. a bit more on IRC, where Olof suggested
> that rather than a file containing a path, we could use a symlink since
> the devtree is now in sysfs, so we can do relative links.

+1 on this. That was part of the motivation to move DT into sysfs.

Rob

> That means keeping the legacy "devspec" files in the few places where
> they exist, possibly behind a deprecated-default-y config option (though
> we might never be able to deprecate them...), but there aren't many.
>
> Any preference either way ?
>
> Cheers,
> Ben.
>
>

Re: [RFC] Add of_path property for all devices with a node

[Benjamin Herrenschmidt]

On Mon, 2014-11-10 at 08:06 -0600, Rob Herring wrote:
> Adding Grant.

Oh, I forgot him ? oops...

> > So not much reactions here .. a bit more on IRC, where Olof suggested
> > that rather than a file containing a path, we could use a symlink since
> > the devtree is now in sysfs, so we can do relative links.
> 
> +1 on this. That was part of the motivation to move DT into sysfs.

Yes, I like this too. I'll give it a go some time this week (I'm off for
a day or two) unless somebody beats me to it.

Cheers,
Ben.

> Rob
> 
> > That means keeping the legacy "devspec" files in the few places where
> > they exist, possibly behind a deprecated-default-y config option (though
> > we might never be able to deprecate them...), but there aren't many.
> >
> > Any preference either way ?
> >
> > Cheers,
> > Ben.
> >
> >
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] drivers/core/of: Add symlink to device-tree from devices with an OF node

[Benjamin Herrenschmidt]

So I've been annoyed lately with having a bunch of devices such as i2c
eeproms (for use by VPDs, server world !) and other bits and pieces that
I want to be able to identify from userspace, and possibly provide
additional data about from FW.

Basically, it boils down to correlating the sysfs device with the OF
tree device node, so that user space can use device-tree info such as
additional "location" or "label" (or whatever else we can come up with)
propreties to identify a given device, or get some attributes of use
about it, etc...

Now, so far, we've done that in some subsystem in a fairly ad-hoc basis
using "devspec" properties. For example, PCI creates them if it can
correlate the probed device with a DT node. Some powerpc specific busses
do that too.

However, i2c doesn't and it would be nice to have something more generic
since technically any device can have a corresponding device tree node.

This patch adds an "of_node" symlink to devices that have a non-NULL
dev->of_node pointer, the patch is pretty trivial and seems to work just
fine for me.

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
diff --git a/drivers/base/core.c b/drivers/base/core.c
index 20da3ad..8c7b607 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -493,6 +493,15 @@ static int device_add_attrs(struct device *dev)
 			goto err_remove_dev_groups;
 	}
 
+#ifdef CONFIG_OF
+	if (dev->of_node) {
+		error = sysfs_create_link(&dev->kobj, &dev->of_node->kobj,
+					  "of_node");
+		if (error)
+			dev_warn(dev, "Error %d creating of_node link\n", error);
+	}
+#endif /* CONFIG_OF */
+
 	return 0;
 
  err_remove_dev_groups:

Re: [PATCH] drivers/core/of: Add symlink to device-tree from devices with an OF node

[Rob Herring]

On Wed, Nov 12, 2014 at 7:10 PM, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
> So I've been annoyed lately with having a bunch of devices such as i2c
> eeproms (for use by VPDs, server world !) and other bits and pieces that
> I want to be able to identify from userspace, and possibly provide
> additional data about from FW.
>
> Basically, it boils down to correlating the sysfs device with the OF
> tree device node, so that user space can use device-tree info such as
> additional "location" or "label" (or whatever else we can come up with)
> propreties to identify a given device, or get some attributes of use
> about it, etc...
>
> Now, so far, we've done that in some subsystem in a fairly ad-hoc basis
> using "devspec" properties. For example, PCI creates them if it can
> correlate the probed device with a DT node. Some powerpc specific busses
> do that too.
>
> However, i2c doesn't and it would be nice to have something more generic
> since technically any device can have a corresponding device tree node.
>
> This patch adds an "of_node" symlink to devices that have a non-NULL
> dev->of_node pointer, the patch is pretty trivial and seems to work just
> fine for me.
>
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> ---
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 20da3ad..8c7b607 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -493,6 +493,15 @@ static int device_add_attrs(struct device *dev)
>                         goto err_remove_dev_groups;
>         }
>
> +#ifdef CONFIG_OF
> +       if (dev->of_node) {

if (IS_ENABLED(CONFIG_OF) && dev->of_node)

> +               error = sysfs_create_link(&dev->kobj, &dev->of_node->kobj,
> +                                         "of_node");
> +               if (error)
> +                       dev_warn(dev, "Error %d creating of_node link\n", error);
> +       }
> +#endif /* CONFIG_OF */
> +
>         return 0;
>
>   err_remove_dev_groups:
>
>

Re: [PATCH] drivers/core/of: Add symlink to device-tree from devices with an OF node

[Jeremy Kerr]

Hi Rob,

>> diff --git a/drivers/base/core.c b/drivers/base/core.c
>> index 20da3ad..8c7b607 100644
>> --- a/drivers/base/core.c
>> +++ b/drivers/base/core.c
>> @@ -493,6 +493,15 @@ static int device_add_attrs(struct device *dev)
>>                         goto err_remove_dev_groups;
>>         }
>>
>> +#ifdef CONFIG_OF
>> +       if (dev->of_node) {
> 
> if (IS_ENABLED(CONFIG_OF) && dev->of_node)

struct device doesn't have an of_node member if !CONFIG_OF, so we'll
need to disable this block in the preprocessor.

Cheers,


Jeremy

Re: [PATCH] drivers/core/of: Add symlink to device-tree from devices with an OF node

[Jeremy Kerr]

Hi Rob,

> struct device doesn't have an of_node member if !CONFIG_OF, so we'll
> need to disable this block in the preprocessor.

Scratch that, I was looking at the wrong header - we do indeed have the
of_node available independently of CONFIG_OF, and this makes the logic a
little cleaner.

Cheers,


Jeremy

Re: [PATCH] drivers/core/of: Add symlink to device-tree from devices with an OF node

[Benjamin Herrenschmidt]

On Wed, 2014-11-19 at 10:39 +1100, Jeremy Kerr wrote:
> Hi Rob,
> 
> >> diff --git a/drivers/base/core.c b/drivers/base/core.c
> >> index 20da3ad..8c7b607 100644
> >> --- a/drivers/base/core.c
> >> +++ b/drivers/base/core.c
> >> @@ -493,6 +493,15 @@ static int device_add_attrs(struct device *dev)
> >>                         goto err_remove_dev_groups;
> >>         }
> >>
> >> +#ifdef CONFIG_OF
> >> +       if (dev->of_node) {
> > 
> > if (IS_ENABLED(CONFIG_OF) && dev->of_node)
> 
> struct device doesn't have an of_node member if !CONFIG_OF, so we'll
> need to disable this block in the preprocessor.

Actually that's no longer the case since 2.6.39 afaik :-)

Cheers,
Ben.

Re: [PATCH] drivers/core/of: Add symlink to device-tree from devices with an OF node

[Arnd Bergmann]

On Wednesday 19 November 2014 13:35:44 Benjamin Herrenschmidt wrote:
> On Wed, 2014-11-19 at 10:39 +1100, Jeremy Kerr wrote:
> > Hi Rob,
> > 
> > >> diff --git a/drivers/base/core.c b/drivers/base/core.c
> > >> index 20da3ad..8c7b607 100644
> > >> --- a/drivers/base/core.c
> > >> +++ b/drivers/base/core.c
> > >> @@ -493,6 +493,15 @@ static int device_add_attrs(struct device *dev)
> > >>                         goto err_remove_dev_groups;
> > >>         }
> > >>
> > >> +#ifdef CONFIG_OF
> > >> +       if (dev->of_node) {
> > > 
> > > if (IS_ENABLED(CONFIG_OF) && dev->of_node)
> > 
> > struct device doesn't have an of_node member if !CONFIG_OF, so we'll
> > need to disable this block in the preprocessor.
> 
> Actually that's no longer the case since 2.6.39 afaik 

I wonder if we should create a small helper for this, like

static inline struct device_node *dev_of_node(struct device *of_node)
{
	if (!IS_ENABLED(CONFIG_OF))
		return NULL;

	return dev->of_node;
}

Adding the IS_ENABLED() in a lot of drivers isn't horrible, but we seem
to be doing it a lot.

	Arnd

Re: [PATCH] drivers/core/of: Add symlink to device-tree from devices with an OF node

[Rob Herring]

On Wed, Nov 19, 2014 at 2:38 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Wednesday 19 November 2014 13:35:44 Benjamin Herrenschmidt wrote:
>> On Wed, 2014-11-19 at 10:39 +1100, Jeremy Kerr wrote:
>> > Hi Rob,
>> >
>> > >> diff --git a/drivers/base/core.c b/drivers/base/core.c
>> > >> index 20da3ad..8c7b607 100644
>> > >> --- a/drivers/base/core.c
>> > >> +++ b/drivers/base/core.c
>> > >> @@ -493,6 +493,15 @@ static int device_add_attrs(struct device *dev)
>> > >>                         goto err_remove_dev_groups;
>> > >>         }
>> > >>
>> > >> +#ifdef CONFIG_OF
>> > >> +       if (dev->of_node) {
>> > >
>> > > if (IS_ENABLED(CONFIG_OF) && dev->of_node)
>> >
>> > struct device doesn't have an of_node member if !CONFIG_OF, so we'll
>> > need to disable this block in the preprocessor.
>>
>> Actually that's no longer the case since 2.6.39 afaik
>
> I wonder if we should create a small helper for this, like
>
> static inline struct device_node *dev_of_node(struct device *of_node)
> {
>         if (!IS_ENABLED(CONFIG_OF))
>                 return NULL;
>
>         return dev->of_node;
> }
>
> Adding the IS_ENABLED() in a lot of drivers isn't horrible, but we seem
> to be doing it a lot.

I think you misread things. of_node is always present now, so it
should always be NULL for !CONFIG_OF.

Rob

Re: [PATCH] drivers/core/of: Add symlink to device-tree from devices with an OF node

[Arnd Bergmann]

On Wednesday 19 November 2014 08:45:58 Rob Herring wrote:
> > static inline struct device_node *dev_of_node(struct device *of_node)
> > {
> >         if (!IS_ENABLED(CONFIG_OF))
> >                 return NULL;
> >
> >         return dev->of_node;
> > }
> >
> > Adding the IS_ENABLED() in a lot of drivers isn't horrible, but we seem
> > to be doing it a lot.
> 
> I think you misread things. of_node is always present now, so it
> should always be NULL for !CONFIG_OF.
> 

No, I didn't misread it but I should have been clearer with the intention:
The idea is to tell the compiler that we know it will be NULL when CONFIG_OF
is unset, so it can optimize out all code that does

	struct device_node *dn = dev_of_node(dev);

	if (dn) {
		...
		/* complex code */
		...
	}

and we can avoid using an #ifdef or if(IS_ENABLED()) in the source to
compile out the DT-only sections of a driver.

	Arnd

Re: [PATCH] drivers/core/of: Add symlink to device-tree from devices with an OF node

[Rob Herring]

On Wed, Nov 19, 2014 at 8:49 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Wednesday 19 November 2014 08:45:58 Rob Herring wrote:
>> > static inline struct device_node *dev_of_node(struct device *of_node)
>> > {
>> >         if (!IS_ENABLED(CONFIG_OF))
>> >                 return NULL;
>> >
>> >         return dev->of_node;
>> > }
>> >
>> > Adding the IS_ENABLED() in a lot of drivers isn't horrible, but we seem
>> > to be doing it a lot.
>>
>> I think you misread things. of_node is always present now, so it
>> should always be NULL for !CONFIG_OF.
>>
>
> No, I didn't misread it but I should have been clearer with the intention:
> The idea is to tell the compiler that we know it will be NULL when CONFIG_OF
> is unset, so it can optimize out all code that does
>
>         struct device_node *dn = dev_of_node(dev);
>
>         if (dn) {
>                 ...
>                 /* complex code */
>                 ...
>         }
>
> and we can avoid using an #ifdef or if(IS_ENABLED()) in the source to
> compile out the DT-only sections of a driver.

Oh, right. That would definitely be worthwhile to do.

Rob

Re: [PATCH] drivers/core/of: Add symlink to device-tree from devices with an OF node

[Grant Likely]

On Wed, Nov 19, 2014 at 3:39 PM, Rob Herring <robherring2@gmail.com> wrote:
> On Wed, Nov 19, 2014 at 8:49 AM, Arnd Bergmann <arnd@arndb.de> wrote:
>> On Wednesday 19 November 2014 08:45:58 Rob Herring wrote:
>>> > static inline struct device_node *dev_of_node(struct device *of_node)
>>> > {
>>> >         if (!IS_ENABLED(CONFIG_OF))
>>> >                 return NULL;
>>> >
>>> >         return dev->of_node;
>>> > }
>>> >
>>> > Adding the IS_ENABLED() in a lot of drivers isn't horrible, but we seem
>>> > to be doing it a lot.
>>>
>>> I think you misread things. of_node is always present now, so it
>>> should always be NULL for !CONFIG_OF.
>>>
>>
>> No, I didn't misread it but I should have been clearer with the intention:
>> The idea is to tell the compiler that we know it will be NULL when CONFIG_OF
>> is unset, so it can optimize out all code that does
>>
>>         struct device_node *dn = dev_of_node(dev);
>>
>>         if (dn) {
>>                 ...
>>                 /* complex code */
>>                 ...
>>         }
>>
>> and we can avoid using an #ifdef or if(IS_ENABLED()) in the source to
>> compile out the DT-only sections of a driver.
>
> Oh, right. That would definitely be worthwhile to do.

Agreed.

g.

Re: [PATCH] drivers/core/of: Add symlink to device-tree from devices with an OF node

[Benjamin Herrenschmidt]

On Tue, 2014-11-18 at 10:37 -0600, Rob Herring wrote:
> On Wed, Nov 12, 2014 at 7:10 PM, Benjamin Herrenschmidt
> <benh@kernel.crashing.org> wrote:
> > So I've been annoyed lately with having a bunch of devices such as i2c
> > eeproms (for use by VPDs, server world !) and other bits and pieces that
> > I want to be able to identify from userspace, and possibly provide
> > additional data about from FW.
> >
> > Basically, it boils down to correlating the sysfs device with the OF
> > tree device node, so that user space can use device-tree info such as
> > additional "location" or "label" (or whatever else we can come up with)
> > propreties to identify a given device, or get some attributes of use
> > about it, etc...
> >
> > Now, so far, we've done that in some subsystem in a fairly ad-hoc basis
> > using "devspec" properties. For example, PCI creates them if it can
> > correlate the probed device with a DT node. Some powerpc specific busses
> > do that too.
> >
> > However, i2c doesn't and it would be nice to have something more generic
> > since technically any device can have a corresponding device tree node.
> >
> > This patch adds an "of_node" symlink to devices that have a non-NULL
> > dev->of_node pointer, the patch is pretty trivial and seems to work just
> > fine for me.
> >
> > Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> > ---
> > diff --git a/drivers/base/core.c b/drivers/base/core.c
> > index 20da3ad..8c7b607 100644
> > --- a/drivers/base/core.c
> > +++ b/drivers/base/core.c
> > @@ -493,6 +493,15 @@ static int device_add_attrs(struct device *dev)
> >                         goto err_remove_dev_groups;
> >         }
> >
> > +#ifdef CONFIG_OF
> > +       if (dev->of_node) {
> 
> if (IS_ENABLED(CONFIG_OF) && dev->of_node)

Ok, I didn't realize the of_node field existed in struct device even
without CONFIG_OF (otherwise that wouldn't have compiled). Grant, Rob,
do you want to take this patch (with the above fixed) or should I not
bother based on the fact that the info is in uevent ? I prefer still
doing the symlink but you tell me.

> > +               error = sysfs_create_link(&dev->kobj, &dev->of_node->kobj,
> > +                                         "of_node");
> > +               if (error)
> > +                       dev_warn(dev, "Error %d creating of_node link\n", error);
> > +       }
> > +#endif /* CONFIG_OF */
> > +
> >         return 0;
> >
> >   err_remove_dev_groups:
> >
> >
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] drivers/core/of: Add symlink to device-tree from devices with an OF node

[Greg KH]

On Thu, Nov 13, 2014 at 12:10:47PM +1100, Benjamin Herrenschmidt wrote:
> So I've been annoyed lately with having a bunch of devices such as i2c
> eeproms (for use by VPDs, server world !) and other bits and pieces that
> I want to be able to identify from userspace, and possibly provide
> additional data about from FW.
> 
> Basically, it boils down to correlating the sysfs device with the OF
> tree device node, so that user space can use device-tree info such as
> additional "location" or "label" (or whatever else we can come up with)
> propreties to identify a given device, or get some attributes of use
> about it, etc...
> 
> Now, so far, we've done that in some subsystem in a fairly ad-hoc basis
> using "devspec" properties. For example, PCI creates them if it can
> correlate the probed device with a DT node. Some powerpc specific busses
> do that too.
> 
> However, i2c doesn't and it would be nice to have something more generic
> since technically any device can have a corresponding device tree node.
> 
> This patch adds an "of_node" symlink to devices that have a non-NULL
> dev->of_node pointer, the patch is pretty trivial and seems to work just
> fine for me.
> 
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> ---
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 20da3ad..8c7b607 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -493,6 +493,15 @@ static int device_add_attrs(struct device *dev)
>  			goto err_remove_dev_groups;
>  	}
>  
> +#ifdef CONFIG_OF
> +	if (dev->of_node) {
> +		error = sysfs_create_link(&dev->kobj, &dev->of_node->kobj,
> +					  "of_node");
> +		if (error)
> +			dev_warn(dev, "Error %d creating of_node link\n", error);
> +	}
> +#endif /* CONFIG_OF */
> +
>  	return 0;
>  
>   err_remove_dev_groups:
> 

Are you going to resend a changed version of this?

thanks,

greg k-h

Re: [PATCH] drivers/core/of: Add symlink to device-tree from devices with an OF node

[Benjamin Herrenschmidt]

On Wed, 2014-11-26 at 19:39 -0800, Greg KH wrote:

> Are you going to resend a changed version of this?

Yes, I've been distracted by a few other things, but I suppose I should
do it :-)

Possibly tomorrow. Arnd, are you doing that helper you suggested to get
to the of_node or should I ?

Cheers,
Ben.


> thanks,
> 
> greg k-h
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC] Add of_path property for all devices with a node

[Grant Likely]

On Mon, Nov 10, 2014 at 2:06 PM, Rob Herring <robherring2@gmail.com> wrote:
> Adding Grant.
>
> On Sun, Nov 9, 2014 at 11:17 PM, Benjamin Herrenschmidt
> <benh@kernel.crashing.org> wrote:
>> On Fri, 2014-11-07 at 17:35 +1100, Benjamin Herrenschmidt wrote:
>>> On Fri, 2014-11-07 at 17:33 +1100, Benjamin Herrenschmidt wrote:
>>>
>>> > So I came up with this patch,
>>>
>>> And here is the actual patch, which might help :-) It's pretty trivial
>>> and small...
>>
>> So not much reactions here .. a bit more on IRC, where Olof suggested
>> that rather than a file containing a path, we could use a symlink since
>> the devtree is now in sysfs, so we can do relative links.
>
> +1 on this. That was part of the motivation to move DT into sysfs.
>
> Rob

I had a patch that did exactly that. I wonder what happened to it...

But regardless, as Frank said in his reply, this data is already in
the uevent file for every device.

g.

Re: [RFC] Add of_path property for all devices with a node

[Benjamin Herrenschmidt]

On Tue, 2014-11-18 at 15:18 +0000, Grant Likely wrote:
> >> So not much reactions here .. a bit more on IRC, where Olof
> suggested
> >> that rather than a file containing a path, we could use a symlink
> since
> >> the devtree is now in sysfs, so we can do relative links.
> >
> > +1 on this. That was part of the motivation to move DT into sysfs.
> >
> > Rob
> 
> I had a patch that did exactly that. I wonder what happened to it...
> 
> But regardless, as Frank said in his reply, this data is already in
> the uevent file for every device.

I sent a patch doing the symlink days ago.... I think it's still
valuable, the patch is triviall and the symlink is a lot easier to deal
with than parsing the uevent file.

Cheers,
Ben.

[PATCH 2/2 v3] drivers/core/of: Add symlink to device-tree from devices with an OF node

[Benjamin Herrenschmidt]

So I've been annoyed lately with having a bunch of devices such as i2c
eeproms (for use by VPDs, server world !) and other bits and pieces that
I want to be able to identify from userspace, and possibly provide
additional data about from FW.

Basically, it boils down to correlating the sysfs device with the OF
tree device node, so that user space can use device-tree info such as
additional "location" or "label" (or whatever else we can come up with)
propreties to identify a given device, or get some attributes of use
about it, etc...

Now, so far, we've done that in some subsystem in a fairly ad-hoc basis
using "devspec" properties. For example, PCI creates them if it can
correlate the probed device with a DT node. Some powerpc specific busses
do that too.

However, i2c doesn't and it would be nice to have something more generic
since technically any device can have a corresponding device tree node.

This patch adds an "of_node" symlink to devices that have a non-NULL
dev->of_node pointer, the patch is pretty trivial and seems to work just
fine for me.

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---

This addresses Greg's comments. Note that I'm not 100% certain about
using device_add_class_symlinks(), I had to put the code before the
test for dev->class, maybe we should rename that function to device_add_symlinks()
if it grows beyond the class bits ?

Also there was nothing that I could find in Documentation/ABI that
documented "core" device properties, it's all in
Documentation/sysfs-rules.txt, but as suggested by Greg (on IRC) I
went for ABI anyway, so I've added a file for "generic" properties
and added that one in. I'm happy to change it if you think that's not
right, just let me know where you want things.

I'm not resending patch 1/2, it should still be fine. Let me know if
you want a new copy.

 Documentation/ABI/stable/sysfs-devices | 10 ++++++++++
 drivers/base/core.c                    | 16 ++++++++++++++--
 2 files changed, 24 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/ABI/stable/sysfs-devices

diff --git a/Documentation/ABI/stable/sysfs-devices b/Documentation/ABI/stable/sysfs-devices
new file mode 100644
index 0000000..43f78b88d
--- /dev/null
+++ b/Documentation/ABI/stable/sysfs-devices
@@ -0,0 +1,10 @@
+# Note: This documents additional properties of any device beyond what
+# is documented in Documentation/sysfs-rules.txt
+
+What:		/sys/devices/*/of_path
+Date:		February 2015
+Contact:	Device Tree mailing list <devicetree@vger.kernel.org>
+Description:
+		Any device associated with a device-tree node will have
+		an of_path symlink pointing to the corresponding device
+		node in /sys/firmware/devicetree/
diff --git a/drivers/base/core.c b/drivers/base/core.c
index 97e2baf..2549805 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -805,8 +805,16 @@ static void cleanup_device_parent(struct device *dev)
 
 static int device_add_class_symlinks(struct device *dev)
 {
+	struct device_node *of_node = dev_of_node(dev);
 	int error;
 
+	if (of_node) {
+		error = sysfs_create_link(&dev->kobj, &of_node->kobj,"of_node");
+		if (error)
+			dev_warn(dev, "Error %d creating of_node link\n",error);
+		/* An error here doesn't warrant bringing down the device */
+	}
+
 	if (!dev->class)
 		return 0;
 
@@ -814,7 +822,7 @@ static int device_add_class_symlinks(struct device *dev)
 				  &dev->class->p->subsys.kobj,
 				  "subsystem");
 	if (error)
-		goto out;
+		goto out_devnode;
 
 	if (dev->parent && device_is_not_partition(dev)) {
 		error = sysfs_create_link(&dev->kobj, &dev->parent->kobj,
@@ -842,12 +850,16 @@ out_device:
 
 out_subsys:
 	sysfs_remove_link(&dev->kobj, "subsystem");
-out:
+out_devnode:
+	sysfs_remove_link(&dev->kobj, "of_node");
 	return error;
 }
 
 static void device_remove_class_symlinks(struct device *dev)
 {
+	if (dev_of_node(dev))
+		sysfs_remove_link(&dev->kobj, "of_node");
+
 	if (!dev->class)
 		return;
 

Re: [PATCH 2/2 v3] drivers/core/of: Add symlink to device-tree from devices with an OF node

[Rob Herring]

On Tue, Feb 17, 2015 at 6:25 PM, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
> So I've been annoyed lately with having a bunch of devices such as i2c
> eeproms (for use by VPDs, server world !) and other bits and pieces that
> I want to be able to identify from userspace, and possibly provide
> additional data about from FW.
>
> Basically, it boils down to correlating the sysfs device with the OF
> tree device node, so that user space can use device-tree info such as
> additional "location" or "label" (or whatever else we can come up with)
> propreties to identify a given device, or get some attributes of use
> about it, etc...
>
> Now, so far, we've done that in some subsystem in a fairly ad-hoc basis
> using "devspec" properties. For example, PCI creates them if it can
> correlate the probed device with a DT node. Some powerpc specific busses
> do that too.
>
> However, i2c doesn't and it would be nice to have something more generic
> since technically any device can have a corresponding device tree node.
>
> This patch adds an "of_node" symlink to devices that have a non-NULL
> dev->of_node pointer, the patch is pretty trivial and seems to work just
> fine for me.
>
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>

Acked-by: Rob Herring <robh@kernel.org>

> ---
>
> This addresses Greg's comments. Note that I'm not 100% certain about
> using device_add_class_symlinks(), I had to put the code before the
> test for dev->class, maybe we should rename that function to device_add_symlinks()
> if it grows beyond the class bits ?
>
> Also there was nothing that I could find in Documentation/ABI that
> documented "core" device properties, it's all in
> Documentation/sysfs-rules.txt, but as suggested by Greg (on IRC) I
> went for ABI anyway, so I've added a file for "generic" properties
> and added that one in. I'm happy to change it if you think that's not
> right, just let me know where you want things.
>
> I'm not resending patch 1/2, it should still be fine. Let me know if
> you want a new copy.
>
>  Documentation/ABI/stable/sysfs-devices | 10 ++++++++++
>  drivers/base/core.c                    | 16 ++++++++++++++--
>  2 files changed, 24 insertions(+), 2 deletions(-)
>  create mode 100644 Documentation/ABI/stable/sysfs-devices
>
> diff --git a/Documentation/ABI/stable/sysfs-devices b/Documentation/ABI/stable/sysfs-devices
> new file mode 100644
> index 0000000..43f78b88d
> --- /dev/null
> +++ b/Documentation/ABI/stable/sysfs-devices
> @@ -0,0 +1,10 @@
> +# Note: This documents additional properties of any device beyond what
> +# is documented in Documentation/sysfs-rules.txt
> +
> +What:          /sys/devices/*/of_path
> +Date:          February 2015
> +Contact:       Device Tree mailing list <devicetree@vger.kernel.org>
> +Description:
> +               Any device associated with a device-tree node will have
> +               an of_path symlink pointing to the corresponding device
> +               node in /sys/firmware/devicetree/
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 97e2baf..2549805 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -805,8 +805,16 @@ static void cleanup_device_parent(struct device *dev)
>
>  static int device_add_class_symlinks(struct device *dev)
>  {
> +       struct device_node *of_node = dev_of_node(dev);
>         int error;
>
> +       if (of_node) {
> +               error = sysfs_create_link(&dev->kobj, &of_node->kobj,"of_node");
> +               if (error)
> +                       dev_warn(dev, "Error %d creating of_node link\n",error);
> +               /* An error here doesn't warrant bringing down the device */
> +       }
> +
>         if (!dev->class)
>                 return 0;
>
> @@ -814,7 +822,7 @@ static int device_add_class_symlinks(struct device *dev)
>                                   &dev->class->p->subsys.kobj,
>                                   "subsystem");
>         if (error)
> -               goto out;
> +               goto out_devnode;
>
>         if (dev->parent && device_is_not_partition(dev)) {
>                 error = sysfs_create_link(&dev->kobj, &dev->parent->kobj,
> @@ -842,12 +850,16 @@ out_device:
>
>  out_subsys:
>         sysfs_remove_link(&dev->kobj, "subsystem");
> -out:
> +out_devnode:
> +       sysfs_remove_link(&dev->kobj, "of_node");
>         return error;
>  }
>
>  static void device_remove_class_symlinks(struct device *dev)
>  {
> +       if (dev_of_node(dev))
> +               sysfs_remove_link(&dev->kobj, "of_node");
> +
>         if (!dev->class)
>                 return;
>
>
>

Re: [PATCH 2/2 v3] drivers/core/of: Add symlink to device-tree from devices with an OF node

[Greg Kroah-Hartman]

On Tue, Feb 17, 2015 at 07:07:14PM -0600, Rob Herring wrote:
> On Tue, Feb 17, 2015 at 6:25 PM, Benjamin Herrenschmidt
> <benh@kernel.crashing.org> wrote:
> > So I've been annoyed lately with having a bunch of devices such as i2c
> > eeproms (for use by VPDs, server world !) and other bits and pieces that
> > I want to be able to identify from userspace, and possibly provide
> > additional data about from FW.
> >
> > Basically, it boils down to correlating the sysfs device with the OF
> > tree device node, so that user space can use device-tree info such as
> > additional "location" or "label" (or whatever else we can come up with)
> > propreties to identify a given device, or get some attributes of use
> > about it, etc...
> >
> > Now, so far, we've done that in some subsystem in a fairly ad-hoc basis
> > using "devspec" properties. For example, PCI creates them if it can
> > correlate the probed device with a DT node. Some powerpc specific busses
> > do that too.
> >
> > However, i2c doesn't and it would be nice to have something more generic
> > since technically any device can have a corresponding device tree node.
> >
> > This patch adds an "of_node" symlink to devices that have a non-NULL
> > dev->of_node pointer, the patch is pretty trivial and seems to work just
> > fine for me.
> >
> > Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> 
> Acked-by: Rob Herring <robh@kernel.org>

Thanks, I'll queue these up after 3.20-rc1 is out.

greg k-h

Re: [PATCH 2/2 v3] drivers/core/of: Add symlink to device-tree from devices with an OF node

[Benjamin Herrenschmidt]

On Tue, 2015-02-17 at 20:57 -0800, Greg Kroah-Hartman wrote:

> > Acked-by: Rob Herring <robh@kernel.org>
> 
> Thanks, I'll queue these up after 3.20-rc1 is out.

Awesome, thanks !

Cheers,
Ben.

Re: [PATCH 2/2 v3] drivers/core/of: Add symlink to device-tree from devices with an OF node

[Rob Herring]

On Wed, Feb 18, 2015 at 3:50 AM, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
> On Tue, 2015-02-17 at 20:57 -0800, Greg Kroah-Hartman wrote:
>
>> > Acked-by: Rob Herring <robh@kernel.org>
>>
>> Thanks, I'll queue these up after 3.20-rc1 is out.

It doesn't look like these 2 patches got applied.

Rob

Re: [PATCH 2/2 v3] drivers/core/of: Add symlink to device-tree from devices with an OF node

[Greg Kroah-Hartman]

On Tue, Mar 10, 2015 at 09:22:51AM -0500, Rob Herring wrote:
> On Wed, Feb 18, 2015 at 3:50 AM, Benjamin Herrenschmidt
> <benh@kernel.crashing.org> wrote:
> > On Tue, 2015-02-17 at 20:57 -0800, Greg Kroah-Hartman wrote:
> >
> >> > Acked-by: Rob Herring <robh@kernel.org>
> >>
> >> Thanks, I'll queue these up after 3.20-rc1 is out.
> 
> It doesn't look like these 2 patches got applied.

My queue is still large, I'm still digging through it...

Re: [PATCH 2/2 v3] drivers/core/of: Add symlink to device-tree from devices with an OF node

[Greg Kroah-Hartman]

On Wed, Feb 18, 2015 at 11:25:18AM +1100, Benjamin Herrenschmidt wrote:
> So I've been annoyed lately with having a bunch of devices such as i2c
> eeproms (for use by VPDs, server world !) and other bits and pieces that
> I want to be able to identify from userspace, and possibly provide
> additional data about from FW.
> 
> Basically, it boils down to correlating the sysfs device with the OF
> tree device node, so that user space can use device-tree info such as
> additional "location" or "label" (or whatever else we can come up with)
> propreties to identify a given device, or get some attributes of use
> about it, etc...
> 
> Now, so far, we've done that in some subsystem in a fairly ad-hoc basis
> using "devspec" properties. For example, PCI creates them if it can
> correlate the probed device with a DT node. Some powerpc specific busses
> do that too.
> 
> However, i2c doesn't and it would be nice to have something more generic
> since technically any device can have a corresponding device tree node.
> 
> This patch adds an "of_node" symlink to devices that have a non-NULL
> dev->of_node pointer, the patch is pretty trivial and seems to work just
> fine for me.
> 
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> ---
> 
> This addresses Greg's comments. Note that I'm not 100% certain about
> using device_add_class_symlinks(), I had to put the code before the
> test for dev->class, maybe we should rename that function to device_add_symlinks()
> if it grows beyond the class bits ?

That would probably be a good idea, but we can leave it for now.

> Also there was nothing that I could find in Documentation/ABI that
> documented "core" device properties, it's all in
> Documentation/sysfs-rules.txt, but as suggested by Greg (on IRC) I
> went for ABI anyway, so I've added a file for "generic" properties
> and added that one in. I'm happy to change it if you think that's not
> right, just let me know where you want things.

It looks good as-is, thanks.

greg k-h

Re: [RFC] Add of_path property for all devices with a node

[Frank Rowand]

On 11/6/2014 10:33 PM, Benjamin Herrenschmidt wrote:
> Hey folks ! This is not (yet) a formal patch submission but...
> 
> So I've been annoyed lately with having a bunch of devices such as i2c
> eeproms (for use by VPDs, server world !) and other bits and pieces that
> I want to be able to identify from userspace, and possibly provide
> additional data about from FW.
> 
> Basically, it boils down to correlating the sysfs device with the OF
> tree device node, so that user space can use device-tree info such as
> additional "location" or "label" (or whatever else we can come up with)
> propreties to identify a given device, or get some attributes of use
> about it, etc...
> 
> Now, so far, we've done that in some subsystem in a fairly ad-hoc basis
> using "devspec" properties. For example, PCI creates them if it can
> correlate the probed device with a DT node. Some powerpc specific busses
> do that too.
> 
> However, i2c doesn't and it would be nice to have something more generic
> since technically any device can have a corresponding device tree node.
> 
> So I came up with this patch, it seems to work well for me. I'm adding
> an "of_path" attribute to not conflict with the existing "devspec" one
> just for the sake of this experiment (plus "devspec" sucks). Long run,
> we might want to use of_path and leave a "devspec" symlink to of_path on
> the few busses that currently have devspec (pci and some powerpc
> specific ones).
> 
> Comments ?
> 
> Cheers,
> Ben.

If I understand correctly, that information is already available in
the file uevent.  For example, if I apply your patch, at least
for a simple path, I see the same path name in uevent as in of_path:

   $ cd /sys/devices/soc/f9824900.sdhci
   $ cat of_path
   /soc/sdhci@f9824900
   $ grep OF_FULLNAME uevent | cut -d"=" -f2
   /soc/sdhci@f9824900

-Frank