Re: [PATCH] of: Add .of suffix to device names from devicetree
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.". 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.gad...@fffa4000.vbus} != 1 ] > +if [ ${fffa4000.gad...@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.gad...@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- | ___ 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
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.". 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.gad...@fffa4000.vbus} != 1 ] +if [ ${fffa4000.gad...@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
On Wed, Nov 14, 2018 at 07:59:25AM -0800, Andrey Smirnov wrote: > On Wed, Nov 14, 2018 at 12:52 AM 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.". 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- | ___ 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
On Wed, Nov 14, 2018 at 12:52 AM 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.". 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 > Cc: Andrey Smirnov > Cc: Jan Lübbe > --- > 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
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.". 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- | ___ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox
[PATCH] of: Add .of suffix to device names from devicetree
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.". 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 Cc: Andrey Smirnov Cc: Jan Lübbe --- 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