[PATCH] of: Add .of suffix to device names from devicetree

[PATCH] of: Add .of suffix to device names from devicetree

[Sascha Hauer]

Previous implementation used to add a number to the device names
for devices registered from the device tree which did not have a 'reg'
property, thus a device node named "state" resulted in a device name
"state.<x>". Current implementation skips that number and we get a
device named "state". This conflicts with our barebox state
implementation which tries to register a device named "state" itself.
We could rename the state device nodes of all our device trees, but it
causes less trouble to rename the devices.

This adds a ".of" suffix to the device names for devices registered from
the device tree which also has the nice effect that they now can easily
be recognized.

Fixes: 7e497d48acbd11 ("of: Port latest of_device_make_bus_id() implementation")

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
Cc: Andrey Smirnov <andrew.smirnov@gmail.com>
Cc: Jan Lübbe <jlu@pengutronix.de>
---
 drivers/of/platform.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index 17052f4199..ef8969ca8b 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -61,14 +61,14 @@ static void of_device_make_bus_id(struct device_d *dev)
 		 */
 		reg = of_get_property(node, "reg", NULL);
 		if (reg && (addr = of_translate_address(node, reg)) != OF_BAD_ADDR) {
-			dev_set_name(dev, dev->name ? "%llx.%s:%s" : "%llx.%s",
+			dev_set_name(dev, dev->name ? "%llx.%s:%s" : "%llx.%s.of",
 				     (unsigned long long)addr, node->name,
 				     dev->name);
 			return;
 		}
 
 		/* format arguments only used if dev_name() resolves to NULL */
-		dev_set_name(dev, dev->name ? "%s:%s" : "%s",
+		dev_set_name(dev, dev->name ? "%s:%s" : "%s.of",
 			     kbasename(node->full_name), dev->name);
 		node = node->parent;
 	}
-- 
2.19.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] of: Add .of suffix to device names from devicetree

[Jan Lübbe]

On Wed, 2018-11-14 at 09:52 +0100, Sascha Hauer wrote:
> Previous implementation used to add a number to the device names
> for devices registered from the device tree which did not have a 'reg'
> property, thus a device node named "state" resulted in a device name
> "state.<x>". Current implementation skips that number and we get a
> device named "state". This conflicts with our barebox state
> implementation which tries to register a device named "state" itself.
> We could rename the state device nodes of all our device trees, but it
> causes less trouble to rename the devices.
> 
> This adds a ".of" suffix to the device names for devices registered from
> the device tree which also has the nice effect that they now can easily
> be recognized.
> 
> Fixes: 7e497d48acbd11 ("of: Port latest of_device_make_bus_id()
> implementation")

This looks like it should the fix the problem I saw in my integration
test.

Thanks,
Jan
-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] of: Add .of suffix to device names from devicetree

[Andrey Smirnov]

On Wed, Nov 14, 2018 at 12:52 AM Sascha Hauer <s.hauer@pengutronix.de> wrote:
>
> Previous implementation used to add a number to the device names
> for devices registered from the device tree which did not have a 'reg'
> property, thus a device node named "state" resulted in a device name
> "state.<x>". Current implementation skips that number and we get a
> device named "state". This conflicts with our barebox state
> implementation which tries to register a device named "state" itself.
> We could rename the state device nodes of all our device trees, but it
> causes less trouble to rename the devices.
>

State implementation will register a device named the same as alias
pointing to corresponding node, so the problem only arises if DT has
both a state node named "foo" and an alias to it named "foo" as well.
It seems that the whole alias/standalone device creation code in
common/state/state.c was written precisely because original DT naming
scheme was not producing "fixed/stable" names, so changing then naming
scheme from matching what Linux does in order to fit some assumptions
in state code and unfortunate DT naming, while the easiest solution,
seems a bit backwards.

Can we fix this using an early internal DT fixup that would rename
problematic node and maybe even print a warning urging users to rename
their state nodes?

Thanks,
Andrey Smirnov

> This adds a ".of" suffix to the device names for devices registered from
> the device tree which also has the nice effect that they now can easily
> be recognized.
>
> Fixes: 7e497d48acbd11 ("of: Port latest of_device_make_bus_id() implementation")
>
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> Cc: Andrey Smirnov <andrew.smirnov@gmail.com>
> Cc: Jan Lübbe <jlu@pengutronix.de>
> ---
>  drivers/of/platform.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> index 17052f4199..ef8969ca8b 100644
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -61,14 +61,14 @@ static void of_device_make_bus_id(struct device_d *dev)
>                  */
>                 reg = of_get_property(node, "reg", NULL);
>                 if (reg && (addr = of_translate_address(node, reg)) != OF_BAD_ADDR) {
> -                       dev_set_name(dev, dev->name ? "%llx.%s:%s" : "%llx.%s",
> +                       dev_set_name(dev, dev->name ? "%llx.%s:%s" : "%llx.%s.of",
>                                      (unsigned long long)addr, node->name,
>                                      dev->name);
>                         return;
>                 }
>
>                 /* format arguments only used if dev_name() resolves to NULL */
> -               dev_set_name(dev, dev->name ? "%s:%s" : "%s",
> +               dev_set_name(dev, dev->name ? "%s:%s" : "%s.of",
>                              kbasename(node->full_name), dev->name);
>                 node = node->parent;
>         }
> --
> 2.19.1
>

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] of: Add .of suffix to device names from devicetree

[Sascha Hauer]

On Wed, Nov 14, 2018 at 07:59:25AM -0800, Andrey Smirnov wrote:
> On Wed, Nov 14, 2018 at 12:52 AM Sascha Hauer <s.hauer@pengutronix.de> wrote:
> >
> > Previous implementation used to add a number to the device names
> > for devices registered from the device tree which did not have a 'reg'
> > property, thus a device node named "state" resulted in a device name
> > "state.<x>". Current implementation skips that number and we get a
> > device named "state". This conflicts with our barebox state
> > implementation which tries to register a device named "state" itself.
> > We could rename the state device nodes of all our device trees, but it
> > causes less trouble to rename the devices.
> >
> 
> State implementation will register a device named the same as alias
> pointing to corresponding node, so the problem only arises if DT has
> both a state node named "foo" and an alias to it named "foo" as well.
> It seems that the whole alias/standalone device creation code in
> common/state/state.c was written precisely because original DT naming
> scheme was not producing "fixed/stable" names, so changing then naming
> scheme from matching what Linux does in order to fit some assumptions
> in state code and unfortunate DT naming, while the easiest solution,
> seems a bit backwards.
> 
> Can we fix this using an early internal DT fixup that would rename
> problematic node and maybe even print a warning urging users to rename
> their state nodes?

We have boards that have multiple state instances registered with
different names, but all follow the same pattern and have the same
problem, so it's not only "state" that causes problems, but could be
any string. Lucas noted there were problems with imx-thermal aswell.
We have the general problem that devices that have variables attached
to them shall have a specific name, so we must make sure these are
not used by devices registered from the device tree.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] of: Add .of suffix to device names from devicetree

[Ladislav Michl]

On Wed, Nov 14, 2018 at 09:52:33AM +0100, Sascha Hauer wrote:
> Previous implementation used to add a number to the device names
> for devices registered from the device tree which did not have a 'reg'
> property, thus a device node named "state" resulted in a device name
> "state.<x>". Current implementation skips that number and we get a
> device named "state". This conflicts with our barebox state
> implementation which tries to register a device named "state" itself.
> We could rename the state device nodes of all our device trees, but it
> causes less trouble to rename the devices.
> 
> This adds a ".of" suffix to the device names for devices registered from
> the device tree which also has the nice effect that they now can easily
> be recognized.

Well, that also breaks scripting based on device name (example):
-if [ ${fffa4000.gadget@fffa4000.vbus} != 1 ]
+if [ ${fffa4000.gadget@fffa4000.of.vbus} != 1 ]
 then
        echo "No USB Device cable plugged, normal boot"
        exit 0

I can live with that, but would welcome suggestions how to write above
code in some nicer way.

Thank you

> Fixes: 7e497d48acbd11 ("of: Port latest of_device_make_bus_id() implementation")

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] of: Add .of suffix to device names from devicetree

[Sascha Hauer]

On Mon, Dec 31, 2018 at 12:12:58PM +0100, Ladislav Michl wrote:
> On Wed, Nov 14, 2018 at 09:52:33AM +0100, Sascha Hauer wrote:
> > Previous implementation used to add a number to the device names
> > for devices registered from the device tree which did not have a 'reg'
> > property, thus a device node named "state" resulted in a device name
> > "state.<x>". Current implementation skips that number and we get a
> > device named "state". This conflicts with our barebox state
> > implementation which tries to register a device named "state" itself.
> > We could rename the state device nodes of all our device trees, but it
> > causes less trouble to rename the devices.
> > 
> > This adds a ".of" suffix to the device names for devices registered from
> > the device tree which also has the nice effect that they now can easily
> > be recognized.
> 
> Well, that also breaks scripting based on device name (example):
> -if [ ${fffa4000.gadget@fffa4000.vbus} != 1 ]
> +if [ ${fffa4000.gadget@fffa4000.of.vbus} != 1 ]
>  then
>         echo "No USB Device cable plugged, normal boot"
>         exit 0
> 
> I can live with that, but would welcome suggestions how to write above
> code in some nicer way.

This only becomes a problem when you peristently store an environment and
update barebox afterwards.

I generally avoid scripting as much as I can. It's good to be able to do
scripting, but it's even better to not have to. Scripting is great for
hacking and development but IMHO is bad in production. Correct error
handling is hard to archieve in scripts, even more so in a restricted
shell like hush. Then with scripts it's tempting to store them on the
device. When this is done you can run into surprising compatibility
issues like above, so you are doomed to maintain compatibility to some
ancient scripts that might be on your device.

With the example you have given I would rather write the corresponding C
code which is much easier to maintain.

Sidenote: Your script example only works because your devices base
address happens to be in the a-f hexadecimal range. Shell variables
cannot start with a number, so 800a4000.gadget@800a4000.vbus would not
work.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox