boards: bus devices label names should include address on bus

[erwango]

Describe the bug samples/shields/x_nucleo_iks01a2/standard/sample.shields.x_nucleo_iks01a2.standard on b_l4s5i_iot01a generates the following dtc issue:

devicetree error: Label 'lsm6dsl' appears on /soc/i2c@40005400/lsm6dsl@6b and on /soc/i2c@40005800/lsm6dsl@6a

This is because same node label 'lsm6dsl' is used for both board and shield description: https://github.com/zephyrproject-rtos/zephyr/blob/eafc4f875b7443bceffe7f8bee14aa97d7ca4bd6/boards/shields/x_nucleo_iks01a2/x_nucleo_iks01a2.overlay#L28-L30 https://github.com/zephyrproject-rtos/zephyr/blob/eafc4f875b7443bceffe7f8bee14aa97d7ca4bd6/boards/arm/b_l4s5i_iot01a/b_l4s5i_iot01a.dts#L113-L115

These labels were added in https://github.com/zephyrproject-rtos/zephyr/pull/47944.

Since this use case is totally possible, the node labels provided should not conflict (at least not conflicting in dt when not conflicting in reality. In order to avoid this issue, I'm proposing to extend node label names with device address on bus (eg: lsm6dsl_6b), this way conflicts occurrences are minimized and conflicts occurences will reveal real hw conflicts.

So, we 'd get

 lsm6dsl_6b: lsm6dsl@6b { 
    compatible = "st,lsm6dsl"; 
    reg = <0x6b>; 

and

 lsm6dsl_6a: lsm6dsl@6a { 
    compatible = "st,lsm6dsl"; 
    reg = <0x6a>; 

[erwango]

@ycsin, @gmarull, @MaureenHelm, @soburi, @galak, @carlescufi: Aligned with this proposal ?

[soburi]

Hi, @erwango

It may not have been necessary to label all sensors.

It may be preferable to use the full path of the devicetree to define the alias. At least, We don't have to create a label to applicate generic test conventions.

Labels are convenient names that potentially conflict. I think that should use a full path of devicetree to solve this problem instead label. (And we should not label the node for creating the alias. We can use the 'alias' as a short name for the sensor node, even without the 'label')

I think we can solve the problem with the way shown in the following diffs.

diff --git a/boards/arm/bbc_microbit/bbc_microbit.dts b/boards/arm/bbc_microbit/bbc_microbit.dts
index 3398b47ccb..f76847bb28 100644
--- a/boards/arm/bbc_microbit/bbc_microbit.dts
+++ b/boards/arm/bbc_microbit/bbc_microbit.dts
@@ -17,7 +17,7 @@
                sw0 = &buttonA;
                sw1 = &buttonB;
                magn0 = &lsm303agr_magn;
-               accel0 = &mma8653fc;
+               accel0 = "/soc/i2c@40003000/mma8653fc@1d";
                watchdog0 = &wdt0;
        };

@@ -132,7 +132,7 @@
        pinctrl-0 = <&i2c0_default>;
        pinctrl-1 = <&i2c0_sleep>;
        pinctrl-names = "default", "sleep";
-       mma8653fc: mma8653fc@1d {
+       mma8653fc@1d {
                compatible = "nxp,fxos8700", "nxp,mma8653fc";
                status = "okay";
                reg = <0x1d>;

[erwango]

I'm not convinced about using the full path as we generally avoid this solution. But maybe that could be acceptable. Can we use full path for shields as well ? This would be something like: accel0 = "/soc/arduino_i2c/mma8653fc@1d

[soburi]

@erwango ,

It is difficult problem in a shield case.

arduino_i2c can't determine it placed just under the /soc. Shield's overlay defines "relative path" from the label path. The full-path is depends on that the arduino_i2c is where. It should use label in this case.

Heart of my opinion is...

• Don't set needless label to prevent (reduce the probability) conflict.
• If can determine full-path, use it to set alias. In this case no need to create labels.

I think it should improve situation If only apply the rule for the boards, even excluding sheilds. (I think devicetree is too complex system to solve such problem with single rule. But using the full-path is uniquely identify a node without conflicting in devicetree. It is appriate to this situation.)

[erwango]

• Don't set needless label to prevent (reduce the probability) conflict.

Well, I think this is what https://github.com/zephyrproject-rtos/zephyr/pull/47944 actually did, by introducing node labels that quickly created the issues we were trying to avoid by removing label properties.

• If can determine full-path, use it to set alias. In this case no need to create labels.

So the rule you propose would be:

• boards: use full paths to create aliases
• shields: add labels as full path can't be used. Note that, even in this case, labels should be in the form <device>_<address> in order to avoid creating naming conflicts when two compatible shields are stacked.

I'd be curious to get others opinion between these 2 options.

[gmarull]

I'm not convinced about appending the address to nodelabels, since I personally like nodelabels to be short and readable. Maybe shields could pre/append nodelabels with e.g. shield? Something like:

regular board:

 lsm6dsl: lsm6dsl@6b { 
    compatible = "st,lsm6dsl"; 
    reg = <0x6b>; 

shield:

 lsm6dsl_shield: lsm6dsl@6a { 
    compatible = "st,lsm6dsl"; 
    reg = <0x6a>; 

[erwango]

I'm not convinced about appending the address to nodelabels, since I personally like nodelabels to be short and readable. Maybe shields could pre/append nodelabels with e.g. shield? Something like:

regular board:

lsm6dsl: lsm6dsl@6b { 
  compatible = "st,lsm6dsl"; 
  reg = <0x6b>; 

shield:

 lsm6dsl_shield: lsm6dsl@6a { 
  compatible = "st,lsm6dsl"; 
  reg = <0x6a>; 

I though about it initially but it let a case open which is collision between compatible shields. Maybe lsm6dsl_<shield_name> (in this case lsm6dsl_x_nucleo_iks01a2) ?

[MaureenHelm]

I agree with @gmarull that nodelabels should be short and readable, and therefore don't think we should use full paths. I lean toward adding the address over "shield" for the same reason.

[erwango]

I agree with @gmarull that nodelabels should be short and readable, and therefore don't think we should use full paths. I lean toward adding the address over "shield" for the same reason.

So, no change in boards and <sensor>_<address> in shields ?

[gmarull]

I agree with @gmarull that nodelabels should be short and readable, and therefore don't think we should use full paths. I lean toward adding the address over "shield" for the same reason.

So, no change in boards and <sensor>_<address> in shields ?

That could be a good choice as well, only tweak shields with address.

[MaureenHelm]

@erwango Is there a reason to keep both this issue and #50040 open? I think we can close this one.

[erwango]

@erwango Is there a reason to keep both this issue and #50040 open? I think we can close this one.

Right, let me close this one. Thanks for noticing.