arm64: dts: rockchip: misc. cleanups and improvements
Subsystems (3)
v2 cover arm64: dts: rockchip: misc. cleanups and improvements
From: Chen-Yu Tsai <[email protected]>
Hi,
Here is the remaining patch for roc-rk3399-pc, along with a few other new patches.
- Based on discussions from v1, patch one, newly added, drops the list of valid values for linux,default-triggers.
- Patch two is the same as in v1
- Patch three, new, sets dr_mode to "host" to the dwc2 OTG controller, matching what the board uses it for, a host port. This gets rid of a warning from the kernel.
Please have a look.
Regards
ChenYu
Chen-Yu Tsai (3):
dt-bindings: leds: common: Drop enumeration for linux,default-triggers
arm64: dts: rockchip: rk3399-roc-pc: Fix MMC numbering for LED
triggers
arm64: dts: rockchip: rk3328-roc-cc: Set dr_mode to "host" for OTG
.../devicetree/bindings/leds/common.yaml | 21 +------------------ .../arm64/boot/dts/rockchip/rk3328-roc-cc.dts | 1 + .../dts/rockchip/rk3399-roc-pc-mezzanine.dts | 8 +++++++ .../boot/dts/rockchip/rk3399-roc-pc.dtsi | 4 ++-- 4 files changed, 12 insertions(+), 22 deletions(-)
Signature · 1 line
2.26.0
PATCH v2 1/3 dt-bindings: leds: common: Drop enumeration for linux,default-triggers
From: Chen-Yu Tsai <[email protected]>
The bindings currently list a very small subset of valid triggers for LEDs. Since many drivers or subsystems in Linux register custom triggers, the list would become very hard to maintain.
Instead, just drop the list and allow free form strings.
--- .../devicetree/bindings/leds/common.yaml | 21 +------------------ 1 file changed, 1 insertion(+), 20 deletions(-)
Documentation/devicetree/bindings/leds/common.yaml +1 -20
@@ -79,26 +79,7 @@ properties: description: This parameter, if present, is a string defining the trigger assigned to the LED. - allOf: - - $ref: /schemas/types.yaml#definitions/string - enum: - # LED will act as a back-light, controlled by the framebuffer system - - backlight - # LED will turn on (but for leds-gpio see "default-state" property in - # Documentation/devicetree/bindings/leds/leds-gpio.yaml) - - default-on - # LED "double" flashes at a load average based rate - - heartbeat - # LED indicates disk activity - - disk-activity - # LED indicates IDE disk activity (deprecated), in new implementations - # use "disk-activity" - - ide-disk - # LED flashes at a fixed, configurable rate - - timer - # LED alters the brightness for the specified duration with one software - # timer (requires "led-pattern" property) - - pattern + $ref: /schemas/types.yaml#definitions/string led-pattern: description: |
Signature · 1 line
2.26.0
Re: [PATCH v2 1/3] dt-bindings: leds: common: Drop enumeration for linux,default-triggers
Hi Chen-Yu,
Quoted 32 lines
From: Chen-Yu Tsai <[email protected]>
The bindings currently list a very small subset of valid triggers for LEDs. Since many drivers or subsystems in Linux register custom triggers, the list would become very hard to maintain.
Instead, just drop the list and allow free form strings.
Signed-off-by Chen-Yu Tsai <[email protected]>--- .../devicetree/bindings/leds/common.yaml | 21 +------------------ 1 file changed, 1 insertion(+), 20 deletions(-)Documentation/devicetree/bindings/leds/common.yaml +0
-11@@ -79,26 +79,7 @@ properties: description: This parameter, if present, is a string defining the trigger assigned to the LED. - allOf: - - $ref: /schemas/types.yaml#definitions/string - enum: - # LED will act as a back-light, controlled by the framebuffer system - - backlight - # LED will turn on (but for leds-gpio see "default-state" property in - # Documentation/devicetree/bindings/leds/leds-gpio.yaml) - - default-on - # LED "double" flashes at a load average based rate - - heartbeat - # LED indicates disk activity
Documentation/devicetree/bindings/leds/common.yaml properties: · line 82
Quoted 10 lines
Documentation/devicetree/bindings/leds/common.yaml +1
-9- - disk-activity - # LED indicates IDE disk activity (deprecated), in new implementations - # use "disk-activity" - - ide-disk - # LED flashes at a fixed, configurable rate - - timer - # LED alters the brightness for the specified duration with one software - # timer (requires "led-pattern" property) - - pattern + $ref: /schemas/types.yaml#definitions/string
This makes it free form, but deletes the documentation of options that are standard available for people without custom driver. Where should that info go?
Quoted 5 lines
Documentation/devicetree/bindings/leds/common.yaml +0
-0led-pattern: description: |Signature · 1 line
2.26.0
Re: [PATCH v2 1/3] dt-bindings: leds: common: Drop enumeration for linux,default-triggers
Quoted 41 lines · Johan Jonker
Hi Chen-Yu,
Quoted 38 lines
From: Chen-Yu Tsai <[email protected]>
The bindings currently list a very small subset of valid triggers for LEDs. Since many drivers or subsystems in Linux register custom triggers, the list would become very hard to maintain.
Instead, just drop the list and allow free form strings.
Signed-off-by Chen-Yu Tsai <[email protected]>--- .../devicetree/bindings/leds/common.yaml | 21 +------------------ 1 file changed, 1 insertion(+), 20 deletions(-)Documentation/devicetree/bindings/leds/common.yaml +0
-17@@ -79,26 +79,7 @@ properties: description: This parameter, if present, is a string defining the trigger assigned to the LED. - allOf: - - $ref: /schemas/types.yaml#definitions/string - enum: - # LED will act as a back-light, controlled by the framebuffer system - - backlight - # LED will turn on (but for leds-gpio see "default-state" property in - # Documentation/devicetree/bindings/leds/leds-gpio.yaml) - - default-on - # LED "double" flashes at a load average based rate - - heartbeat - # LED indicates disk activity - - disk-activity - # LED indicates IDE disk activity (deprecated), in new implementations - # use "disk-activity" - - ide-disk - # LED flashes at a fixed, configurable rate - - timer
Quoted 8 lines
Documentation/devicetree/bindings/leds/common.yaml properties: · line 82
Quoted 4 lines
Documentation/devicetree/bindings/leds/common.yaml +1
-3- # LED alters the brightness for the specified duration with one software - # timer (requires "led-pattern" property) - - pattern + $ref: /schemas/types.yaml#definitions/stringThis makes it free form, but deletes the documentation of options that are standard available for people without custom driver. Where should that info go?
As far as I know, there is no canonical list of standard triggers. In addition, what triggers are available also depend on the kernel configuration, so there really is no "standard".
Since this is also configurable via sysfs, maybe it should be part of the sysfs ABI document? Either way I believe this will be up to the LED subsystem maintainers.
ChenYu
Quoted 6 lines
Quoted 5 lines
Documentation/devicetree/bindings/leds/common.yaml +0
-1led-pattern: description: | --2.26.0
Rob Herring
maintainerRe: [PATCH v2 1/3] dt-bindings: leds: common: Drop enumeration for linux,default-triggers
In reply to Chen-Yu TsaiQuoted 5 lines · Chen-Yu Tsai
From: Chen-Yu Tsai <[email protected]>
The bindings currently list a very small subset of valid triggers for LEDs. Since many drivers or subsystems in Linux register custom triggers, the list would become very hard to maintain.
The idea was we'll consolidate around standardized names and that new users should use 'function' instead.
There was discussion around this when adding 'function'.
Quoted 43 lines
Instead, just drop the list and allow free form strings.
Signed-off-by Chen-Yu Tsai <[email protected]>--- .../devicetree/bindings/leds/common.yaml | 21 +------------------ 1 file changed, 1 insertion(+), 20 deletions(-)Documentation/devicetree/bindings/leds/common.yaml +1
-20@@ -79,26 +79,7 @@ properties: description: This parameter, if present, is a string defining the trigger assigned to the LED. - allOf: - - $ref: /schemas/types.yaml#definitions/string - enum: - # LED will act as a back-light, controlled by the framebuffer system - - backlight - # LED will turn on (but for leds-gpio see "default-state" property in - # Documentation/devicetree/bindings/leds/leds-gpio.yaml) - - default-on - # LED "double" flashes at a load average based rate - - heartbeat - # LED indicates disk activity - - disk-activity - # LED indicates IDE disk activity (deprecated), in new implementations - # use "disk-activity" - - ide-disk - # LED flashes at a fixed, configurable rate - - timer - # LED alters the brightness for the specified duration with one software - # timer (requires "led-pattern" property) - - pattern + $ref: /schemas/types.yaml#definitions/string led-pattern: description: |Signature · 1 line
2.26.0
PATCH v2 2/3 arm64: dts: rockchip: rk3399-roc-pc: Fix MMC numbering for LED triggers
In reply to Chen-Yu TsaiFrom: Chen-Yu Tsai <[email protected]>
With SDIO now enabled, the numbering of the existing MMC host controllers gets incremented by 1, as the SDIO host is the first one.
Increment the numbering of the MMC LED triggers to match.
--- arch/arm64/boot/dts/rockchip/rk3399-roc-pc-mezzanine.dts | 8 ++++++++ arch/arm64/boot/dts/rockchip/rk3399-roc-pc.dtsi | 4 ++-- 2 files changed, 10 insertions(+), 2 deletions(-)
arch/arm64/boot/dts/rockchip/rk3399-roc-pc-mezzanine.dts +8 -0
@@ -38,6 +38,10 @@ vcc3v3_pcie: vcc3v3-pcie { }; }; +&diy_led { + linux,default-trigger = "mmc2"; +}; + &pcie_phy { status = "okay"; };
@@ -91,3 +95,7 @@ &uart0 { pinctrl-0 = <&uart0_xfer &uart0_cts &uart0_rts>; status = "okay"; }; + +&yellow_led { + linux,default-trigger = "mmc1"; +};
arch/arm64/boot/dts/rockchip/rk3399-roc-pc.dtsi +2 -2
@@ -70,14 +70,14 @@ work-led { linux,default-trigger = "heartbeat"; }; - diy-led { + diy_led: diy-led { label = "red:diy"; gpios = <&gpio0 RK_PB5 GPIO_ACTIVE_HIGH>; default-state = "off"; linux,default-trigger = "mmc1"; }; - yellow-led { + yellow_led: yellow-led { label = "yellow:yellow-led"; gpios = <&gpio0 RK_PA2 GPIO_ACTIVE_HIGH>; default-state = "off";
Signature · 1 line
2.26.0
Re: [PATCH v2 2/3] arm64: dts: rockchip: rk3399-roc-pc: Fix MMC numbering for LED triggers
Hi Chen-Yu,
Quoted 42 lines
From: Chen-Yu Tsai <[email protected]>
With SDIO now enabled, the numbering of the existing MMC host controllers gets incremented by 1, as the SDIO host is the first one.
Increment the numbering of the MMC LED triggers to match.
Fixes cf3c5397835f ("arm64: dts: rockchip: Enable sdio0 and uart0 on rk3399-roc-pc-mezzanine")Signed-off-by Chen-Yu Tsai <[email protected]>--- arch/arm64/boot/dts/rockchip/rk3399-roc-pc-mezzanine.dts | 8 ++++++++ arch/arm64/boot/dts/rockchip/rk3399-roc-pc.dtsi | 4 ++-- 2 files changed, 10 insertions(+), 2 deletions(-)arch/arm64/boot/dts/rockchip/rk3399-roc-pc-mezzanine.dts +8
-0@@ -38,6 +38,10 @@ vcc3v3_pcie: vcc3v3-pcie { }; }; +&diy_led { + linux,default-trigger = "mmc2"; +}; + &pcie_phy { status = "okay"; };@@ -91,3 +95,7 @@ &uart0 { pinctrl-0 = <&uart0_xfer &uart0_cts &uart0_rts>; status = "okay"; }; + +&yellow_led { + linux,default-trigger = "mmc1"; +};arch/arm64/boot/dts/rockchip/rk3399-roc-pc.dtsi +0
-0
arch/arm64/boot/dts/rockchip/rk3399-roc-pc.dtsi work-led { · line 74
Quoted 6 lines
arch/arm64/boot/dts/rockchip/rk3399-roc-pc.dtsi +1
-1@@ -70,14 +70,14 @@ work-led { linux,default-trigger = "heartbeat"; }; - diy-led { + diy_led: diy-led {
This changes an existing nodename into something that is still not the preferred way. In the current Rockchip dts there are nodenames like 'work', 'yellow' that causing warnings with the command:
make -k ARCH=arm dtbs_check
Could you give a generic guide line/example, so all these changes are treated the same way? As if the naming follows the preferred 'led-0' line.
Quoted 13 lines
arch/arm64/boot/dts/rockchip/rk3399-roc-pc.dtsi +1
-1label = "red:diy"; gpios = <&gpio0 RK_PB5 GPIO_ACTIVE_HIGH>; default-state = "off"; linux,default-trigger = "mmc1"; }; - yellow-led { + yellow_led: yellow-led { label = "yellow:yellow-led"; gpios = <&gpio0 RK_PA2 GPIO_ACTIVE_HIGH>;default-state = "off";
Signature · 1 line
2.26.0
Re: [PATCH v2 2/3] arm64: dts: rockchip: rk3399-roc-pc: Fix MMC numbering for LED triggers
Quoted 49 lines · Johan Jonker
Hi Chen-Yu,
Quoted 45 lines
From: Chen-Yu Tsai <[email protected]>
With SDIO now enabled, the numbering of the existing MMC host controllers gets incremented by 1, as the SDIO host is the first one.
Increment the numbering of the MMC LED triggers to match.
Fixes cf3c5397835f ("arm64: dts: rockchip: Enable sdio0 and uart0 on rk3399-roc-pc-mezzanine")Signed-off-by Chen-Yu Tsai <[email protected]>--- arch/arm64/boot/dts/rockchip/rk3399-roc-pc-mezzanine.dts | 8 ++++++++ arch/arm64/boot/dts/rockchip/rk3399-roc-pc.dtsi | 4 ++-- 2 files changed, 10 insertions(+), 2 deletions(-)arch/arm64/boot/dts/rockchip/rk3399-roc-pc-mezzanine.dts +8
-0@@ -38,6 +38,10 @@ vcc3v3_pcie: vcc3v3-pcie { }; }; +&diy_led { + linux,default-trigger = "mmc2"; +}; + &pcie_phy { status = "okay"; };@@ -91,3 +95,7 @@ &uart0 { pinctrl-0 = <&uart0_xfer &uart0_cts &uart0_rts>; status = "okay"; }; + +&yellow_led { + linux,default-trigger = "mmc1"; +};arch/arm64/boot/dts/rockchip/rk3399-roc-pc.dtsi +0
-0@@ -70,14 +70,14 @@ work-led { linux,default-trigger = "heartbeat"; };
Quoted 6 lines
arch/arm64/boot/dts/rockchip/rk3399-roc-pc.dtsi work-led { · line 73
Quoted 2 lines
arch/arm64/boot/dts/rockchip/rk3399-roc-pc.dtsi +1
-1- diy-led { + diy_led: diy-led {This changes an existing nodename into something that is still not the preferred way. In the current Rockchip dts there are nodenames like 'work', 'yellow' that causing warnings with the command:
This doesn't change the node name at all. It only adds a label. If it doesn't pass the check now, it didn't pass the check before.
I just realized that the footnote I added before is gone because I regenerated the patches. The original footnote was something along the lines of:
I opted to not change the node names nor the labels as the discussion had not concluded. The other reason being that people may have scripts or device tree overlays depending on the existing node names.
Previously I asked the following but got no response:
Is changing this after it has been in some kernel releases OK? Wouldn't it be considered a break of sysfs ABI?
Also, is there some guideline on how to name the labels? For sunxi we've been doing "${vendor}:${color}:${function}" since forever.
As far as I can tell, the hardware vendor [1] has no specific uses for these two (red and yellow) LEDs designed in. And their GPIO lines are simply labeled "DIY" (for the red one) and "YELLOW". So I'm not sure if putting "our" interpretations and the default-trigger into the label is wise.
For reference, the green one has its GPIO line labeled "WORK", and their intention from [1] is to have it as some sort of power / activity indicator. Hence it is named / labeled "work".
As for the node names, I think we can keep it as is for now. It's not the preferred form, but there's really no need to change it either. And some overlay or script might actually expect that name.
Quoted 4 lines
make -k ARCH=arm
dtbs_checkCould you give a generic guide line/example, so all these changes are treated the same way? As if the naming follows the preferred 'led-0' line.
I'm not sure what you are asking for.
ChenYu
Quoted 14 lines
Quoted 13 lines
arch/arm64/boot/dts/rockchip/rk3399-roc-pc.dtsi +1
-2label = "red:diy"; gpios = <&gpio0 RK_PB5 GPIO_ACTIVE_HIGH>; default-state = "off"; linux,default-trigger = "mmc1"; }; - yellow-led { + yellow_led: yellow-led { label = "yellow:yellow-led"; gpios = <&gpio0 RK_PA2 GPIO_ACTIVE_HIGH>; default-state = "off"; --2.26.0
Re: [PATCH v2 2/3] arm64: dts: rockchip: rk3399-roc-pc: Fix MMC numbering for LED triggers
Quoted 26 lines · Chen-Yu Tsai
Quoted 25 lines · Johan Jonker
Hi Chen-Yu,
Quoted 22 lines
From: Chen-Yu Tsai <[email protected]>
With SDIO now enabled, the numbering of the existing MMC host controllers gets incremented by 1, as the SDIO host is the first one.
Increment the numbering of the MMC LED triggers to match.
Fixes cf3c5397835f ("arm64: dts: rockchip: Enable sdio0 and uart0 on rk3399-roc-pc-mezzanine")Signed-off-by Chen-Yu Tsai <[email protected]>--- arch/arm64/boot/dts/rockchip/rk3399-roc-pc-mezzanine.dts | 8 ++++++++ arch/arm64/boot/dts/rockchip/rk3399-roc-pc.dtsi | 4 ++-- 2 files changed, 10 insertions(+), 2 deletions(-)arch/arm64/boot/dts/rockchip/rk3399-roc-pc-mezzanine.dts +0
-0@@ -38,6 +38,10 @@ vcc3v3_pcie: vcc3v3-pcie { }; };
Quoted 3 lines
Quoted 3 lines
arch/arm64/boot/dts/rockchip/rk3399-roc-pc-mezzanine.dts vcc3v3_pcie: vcc3v3-pcie { · line 42
Quoted 3 lines
arch/arm64/boot/dts/rockchip/rk3399-roc-pc-mezzanine.dts +2
-0+&diy_led { + linux,default-trigger = "mmc2";
If you decide to use a free form trigger that is not a 'standard' one, then it becomes a user property.
User defined properties should not go in a generic dts. It's up to the user what function he/she gives to that led!
Quoted 22 lines
Quoted 22 lines
Quoted 21 lines
arch/arm64/boot/dts/rockchip/rk3399-roc-pc-mezzanine.dts +2
-0+}; + &pcie_phy { status = "okay";};
arch/arm64/boot/dts/rockchip/rk3399-roc-pc-mezzanine.dts +4
-0@@ -91,3 +95,7 @@ &uart0 { pinctrl-0 = <&uart0_xfer &uart0_cts &uart0_rts>; status = "okay"; }; + +&yellow_led { + linux,default-trigger = "mmc1"; +};arch/arm64/boot/dts/rockchip/rk3399-roc-pc.dtsi +0
-0@@ -70,14 +70,14 @@ work-led { linux,default-trigger = "heartbeat"; };
Quoted 4 lines
Quoted 4 lines
arch/arm64/boot/dts/rockchip/rk3399-roc-pc.dtsi work-led { · line 73
Quoted 2 lines
arch/arm64/boot/dts/rockchip/rk3399-roc-pc.dtsi +1
-1- diy-led { + diy_led: diy-led {This changes an existing nodename into something that is still not the
Correction:
It takes an existing nodename and adds a label.
Quoted 36 lines
Quoted 2 lines
preferred way. In the current Rockchip dts there are nodenames like 'work', 'yellow' that causing warnings with the command:
This doesn't change the node name at all. It only adds a label. If it doesn't pass the check now, it didn't pass the check before.
I just realized that the footnote I added before is gone because I regenerated the patches. The original footnote was something along the lines of:
I opted to not change the node names nor the labels as the discussion had not concluded. The other reason being that people may have scripts or device tree overlays depending on the existing node names.
Previously I asked the following but got no response:
Is changing this after it has been in some kernel releases OK? Wouldn't it be considered a break of sysfs ABI?
Also, is there some guideline on how to name the labels? For sunxi we've been doing "${vendor}:${color}:${function}" since forever.
As far as I can tell, the hardware vendor [1] has no specific uses for these two (red and yellow) LEDs designed in. And their GPIO lines are simply labeled "DIY" (for the red one) and "YELLOW". So I'm not sure if putting "our" interpretations and the default-trigger into the label is wise.
For reference, the green one has its GPIO line labeled "WORK", and their intention from [1] is to have it as some sort of power / activity indicator. Hence it is named / labeled "work".
As for the node names, I think we can keep it as is for now. It's not the preferred form, but there's really no need to change it either. And some overlay or script might actually expect that name.
Quoted 6 lines
Quoted 4 lines
make -k ARCH=arm
dtbs_checkCould you give a generic guide line/example, so all these changes are treated the same way? As if the naming follows the preferred 'led-0' line.
I'm not sure what you are asking for.
Your nodename just happend to contain 'led' to pass the regex. There are many other names in use.
'If' the DT maintainer (=Heiko) decides the get rid of all the warnings for led nodes then that change would require all nodename to be handled to same (=preferred way):
Change this:
diy_led: diy-ledyellow_led: yellow-led
Into something like:
led_0: led-0led_1: led-1
Quoted 17 lines
ChenYu
Quoted 14 lines
Quoted 13 lines
arch/arm64/boot/dts/rockchip/rk3399-roc-pc.dtsi +1
-2label = "red:diy"; gpios = <&gpio0 RK_PB5 GPIO_ACTIVE_HIGH>; default-state = "off"; linux,default-trigger = "mmc1"; }; - yellow-led { + yellow_led: yellow-led { label = "yellow:yellow-led"; gpios = <&gpio0 RK_PA2 GPIO_ACTIVE_HIGH>; default-state = "off"; --2.26.0
Re: [PATCH v2 2/3] arm64: dts: rockchip: rk3399-roc-pc: Fix MMC numbering for LED triggers
Quoted 28 lines · Johan Jonker
Quoted 26 lines · Chen-Yu Tsai
Quoted 25 lines · Johan Jonker
Hi Chen-Yu,
Quoted 22 lines
From: Chen-Yu Tsai <[email protected]>
With SDIO now enabled, the numbering of the existing MMC host controllers gets incremented by 1, as the SDIO host is the first one.
Increment the numbering of the MMC LED triggers to match.
Fixes cf3c5397835f ("arm64: dts: rockchip: Enable sdio0 and uart0 on rk3399-roc-pc-mezzanine")Signed-off-by Chen-Yu Tsai <[email protected]>--- arch/arm64/boot/dts/rockchip/rk3399-roc-pc-mezzanine.dts | 8 ++++++++ arch/arm64/boot/dts/rockchip/rk3399-roc-pc.dtsi | 4 ++-- 2 files changed, 10 insertions(+), 2 deletions(-)arch/arm64/boot/dts/rockchip/rk3399-roc-pc-mezzanine.dts +0
-0@@ -38,6 +38,10 @@ vcc3v3_pcie: vcc3v3-pcie { }; };
Quoted 8 lines
Quoted 3 lines
Quoted 3 lines
arch/arm64/boot/dts/rockchip/rk3399-roc-pc-mezzanine.dts vcc3v3_pcie: vcc3v3-pcie { · line 42
Quoted 3 lines
arch/arm64/boot/dts/rockchip/rk3399-roc-pc-mezzanine.dts +2
-0+&diy_led { + linux,default-trigger = "mmc2";If you decide to use a free form trigger that is not a 'standard' one, then it becomes a user property.
User defined properties should not go in a generic dts. It's up to the user what function he/she gives to that led!
The original (in the base .dtsi file for this series of boards) trigger is already a non-standard, i.e. not listed in the bindings, one.
Now I would very much like to get rid of user specific stuff, and I also mentioned that in the previous round of discussions. No one said anything.
Quoted 22 lines
Quoted 22 lines
Quoted 22 lines
Quoted 21 lines
arch/arm64/boot/dts/rockchip/rk3399-roc-pc-mezzanine.dts +2
-0+}; + &pcie_phy { status = "okay";};
arch/arm64/boot/dts/rockchip/rk3399-roc-pc-mezzanine.dts +4
-0@@ -91,3 +95,7 @@ &uart0 { pinctrl-0 = <&uart0_xfer &uart0_cts &uart0_rts>; status = "okay"; }; + +&yellow_led { + linux,default-trigger = "mmc1"; +};arch/arm64/boot/dts/rockchip/rk3399-roc-pc.dtsi +0
-0@@ -70,14 +70,14 @@ work-led { linux,default-trigger = "heartbeat"; };
Quoted 7 lines
Quoted 4 lines
Quoted 4 lines
arch/arm64/boot/dts/rockchip/rk3399-roc-pc.dtsi work-led { · line 73
Quoted 2 lines
arch/arm64/boot/dts/rockchip/rk3399-roc-pc.dtsi +1
-1- diy-led { + diy_led: diy-led {This changes an existing nodename into something that is still not the
Correction:
It takes an existing nodename and adds a label.
OK.
Quoted 38 lines
Quoted 38 lines
Quoted 2 lines
preferred way. In the current Rockchip dts there are nodenames like 'work', 'yellow' that causing warnings with the command:
This doesn't change the node name at all. It only adds a label. If it doesn't pass the check now, it didn't pass the check before.
I just realized that the footnote I added before is gone because I regenerated the patches. The original footnote was something along the lines of:
I opted to not change the node names nor the labels as the discussion had not concluded. The other reason being that people may have scripts or device tree overlays depending on the existing node names.
Previously I asked the following but got no response:
Is changing this after it has been in some kernel releases OK? Wouldn't it be considered a break of sysfs ABI?
Also, is there some guideline on how to name the labels? For sunxi we've been doing "${vendor}:${color}:${function}" since forever.
As far as I can tell, the hardware vendor [1] has no specific uses for these two (red and yellow) LEDs designed in. And their GPIO lines are simply labeled "DIY" (for the red one) and "YELLOW". So I'm not sure if putting "our" interpretations and the default-trigger into the label is wise.
For reference, the green one has its GPIO line labeled "WORK", and their intention from [1] is to have it as some sort of power / activity indicator. Hence it is named / labeled "work".
As for the node names, I think we can keep it as is for now. It's not the preferred form, but there's really no need to change it either. And some overlay or script might actually expect that name.
Quoted 2 lines
make -k ARCH=arm
dtbs_check
Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
Could you give a generic guide line/example, so all these changes are treated the same way? As if the naming follows the preferred 'led-0' line.
I'm not sure what you are asking for.
Your nodename just happend to contain 'led' to pass the regex. There are many other names in use.
Right. So if it passes, what's the problem?
Quoted 6 lines
'If' the DT maintainer (=Heiko) decides the get rid of all the warnings for led nodes then that change would require all nodename to be handled to same (=preferred way):
Change this:
Quoted 7 lines
diy_led: diy-ledyellow_led: yellow-ledInto something like:
led_0: led-0led_1: led-1
As I already said, if the maintainers want to clean this up, I am happy to provide patches towards this. That is not the case right now. Furthermore, that cleanup is not directly related to what I'm trying to fix in this patch, which is, from the original submitter's point of view, incorrect triggers are used when the mezzanine board is added.
Also, DT labels "led_0" and "led_1" are useless. They have no relation to what is used in the schematics, which are "work", "diy", and "yellow". The board itself doesn't have anything silk-screened on, so on that end the only thing to go with is the color.
So for fixing up the LED node names, we'd probably want the following:
diy_led: led-0
yellow_led: led-1
work_led: led-2Is that what you're asking for?
ChenYu
Quoted 18 lines
Quoted 17 lines
ChenYu
Quoted 14 lines
Quoted 13 lines
arch/arm64/boot/dts/rockchip/rk3399-roc-pc.dtsi +1
-2label = "red:diy"; gpios = <&gpio0 RK_PB5 GPIO_ACTIVE_HIGH>; default-state = "off"; linux,default-trigger = "mmc1"; }; - yellow-led { + yellow_led: yellow-led { label = "yellow:yellow-led"; gpios = <&gpio0 RK_PA2 GPIO_ACTIVE_HIGH>; default-state = "off"; --2.26.0
Re: [PATCH v2 2/3] arm64: dts: rockchip: rk3399-roc-pc: Fix MMC numbering for LED triggers
Quoted 150 lines · Chen-Yu Tsai
Quoted 28 lines · Johan Jonker
Quoted 26 lines · Chen-Yu Tsai
Quoted 25 lines · Johan Jonker
Hi Chen-Yu,
Quoted 22 lines
From: Chen-Yu Tsai <[email protected]>
With SDIO now enabled, the numbering of the existing MMC host controllers gets incremented by 1, as the SDIO host is the first one.
Increment the numbering of the MMC LED triggers to match.
Fixes cf3c5397835f ("arm64: dts: rockchip: Enable sdio0 and uart0 on rk3399-roc-pc-mezzanine")Signed-off-by Chen-Yu Tsai <[email protected]>--- arch/arm64/boot/dts/rockchip/rk3399-roc-pc-mezzanine.dts | 8 ++++++++ arch/arm64/boot/dts/rockchip/rk3399-roc-pc.dtsi | 4 ++-- 2 files changed, 10 insertions(+), 2 deletions(-)arch/arm64/boot/dts/rockchip/rk3399-roc-pc-mezzanine.dts +0
-0@@ -38,6 +38,10 @@ vcc3v3_pcie: vcc3v3-pcie { }; };Quoted 8 lines
Quoted 3 lines
Quoted 3 lines
arch/arm64/boot/dts/rockchip/rk3399-roc-pc-mezzanine.dts vcc3v3_pcie: vcc3v3-pcie { · line 42
Quoted 3 lines
arch/arm64/boot/dts/rockchip/rk3399-roc-pc-mezzanine.dts +2
-0+&diy_led { + linux,default-trigger = "mmc2";If you decide to use a free form trigger that is not a 'standard' one, then it becomes a user property.
User defined properties should not go in a generic dts. It's up to the user what function he/she gives to that led!The original (in the base .dtsi file for this series of boards) trigger is already a non-standard, i.e. not listed in the bindings, one.
Now I would very much like to get rid of user specific stuff, and I also mentioned that in the previous round of discussions. No one said anything.
Quoted 22 lines
Quoted 22 lines
Quoted 22 lines
Quoted 21 lines
arch/arm64/boot/dts/rockchip/rk3399-roc-pc-mezzanine.dts +2
-0+}; + &pcie_phy { status = "okay";};
arch/arm64/boot/dts/rockchip/rk3399-roc-pc-mezzanine.dts +4
-0@@ -91,3 +95,7 @@ &uart0 { pinctrl-0 = <&uart0_xfer &uart0_cts &uart0_rts>; status = "okay"; }; + +&yellow_led { + linux,default-trigger = "mmc1"; +};arch/arm64/boot/dts/rockchip/rk3399-roc-pc.dtsi +0
-0@@ -70,14 +70,14 @@ work-led { linux,default-trigger = "heartbeat"; };Quoted 7 lines
Quoted 4 lines
Quoted 4 lines
arch/arm64/boot/dts/rockchip/rk3399-roc-pc.dtsi work-led { · line 73
Quoted 2 lines
arch/arm64/boot/dts/rockchip/rk3399-roc-pc.dtsi +1
-1- diy-led { + diy_led: diy-led {This changes an existing nodename into something that is still not the
Correction:
It takes an existing nodename and adds a label.OK.
Quoted 38 lines
Quoted 38 lines
Quoted 2 lines
preferred way. In the current Rockchip dts there are nodenames like 'work', 'yellow' that causing warnings with the command:
This doesn't change the node name at all. It only adds a label. If it doesn't pass the check now, it didn't pass the check before.
I just realized that the footnote I added before is gone because I regenerated the patches. The original footnote was something along the lines of:
I opted to not change the node names nor the labels as the discussion had not concluded. The other reason being that people may have scripts or device tree overlays depending on the existing node names.
Previously I asked the following but got no response:
Is changing this after it has been in some kernel releases OK? Wouldn't it be considered a break of sysfs ABI?
Also, is there some guideline on how to name the labels? For sunxi we've been doing "${vendor}:${color}:${function}" since forever.
As far as I can tell, the hardware vendor [1] has no specific uses for these two (red and yellow) LEDs designed in. And their GPIO lines are simply labeled "DIY" (for the red one) and "YELLOW". So I'm not sure if putting "our" interpretations and the default-trigger into the label is wise.
For reference, the green one has its GPIO line labeled "WORK", and their intention from [1] is to have it as some sort of power / activity indicator. Hence it is named / labeled "work".
As for the node names, I think we can keep it as is for now. It's not the preferred form, but there's really no need to change it either. And some overlay or script might actually expect that name.
Quoted 2 lines
make -k ARCH=arm
dtbs_checkQuoted 7 lines
Quoted 4 lines
Quoted 2 lines
Could you give a generic guide line/example, so all these changes are treated the same way? As if the naming follows the preferred 'led-0' line.
I'm not sure what you are asking for.
Your nodename just happend to contain 'led' to pass the regex. There are many other names in use.
Right. So if it passes, what's the problem?
Quoted 6 lines
'If' the DT maintainer (=Heiko) decides the get rid of all the warnings for led nodes then that change would require all nodename to be handled to same (=preferred way):
Change this:
Quoted 7 lines
diy_led: diy-ledyellow_led: yellow-ledInto something like:
led_0: led-0led_1: led-1As I already said, if the maintainers want to clean this up, I am happy to provide patches towards this. That is not the case right now. Furthermore, that cleanup is not directly related to what I'm trying to fix in this patch, which is, from the original submitter's point of view, incorrect triggers are used when the mezzanine board is added.
Also, DT labels "
led_0" and "led_1" are useless. They have no relation to what is used in the schematics, which are "work", "diy", and "yellow". The board itself doesn't have anything silk-screened on, so on that end the only thing to go with is the color.
Quoted 5 lines
So for fixing up the LED node names, we'd probably want the following:
diy_led: led-0 yellow_led: led-1 work_led: led-2
That doesn't look pretty either.
Would like to hear the maintainers view on how to handle other cases without 'led' like for example 'blue' for mk808.
Quoted 24 lines
Is that what you're asking for?
ChenYu
Quoted 18 lines
Quoted 17 lines
ChenYu
Quoted 14 lines
Quoted 13 lines
arch/arm64/boot/dts/rockchip/rk3399-roc-pc.dtsi +1
-2label = "red:diy"; gpios = <&gpio0 RK_PB5 GPIO_ACTIVE_HIGH>; default-state = "off"; linux,default-trigger = "mmc1"; }; - yellow-led { + yellow_led: yellow-led { label = "yellow:yellow-led"; gpios = <&gpio0 RK_PA2 GPIO_ACTIVE_HIGH>; default-state = "off"; --2.26.0
Re: [PATCH v2 2/3] arm64: dts: rockchip: rk3399-roc-pc: Fix MMC numbering for LED triggers
Hi,
Quoted 5 lines
Quoted 5 lines
So for fixing up the LED node names, we'd probably want the following:
diy_led: led-0 yellow_led: led-1 work_led: led-2
Change proposal for led nodes to comply with preexisting dts. Does this work?
diy_led: led_0: led-0yellow_led: led_1: led-1work_led: led_2: led-2
blue: led_0: led-0
A check does not give any warnings.
make -k ARCH=arm dtbs_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/leds/leds-gpio.yaml
Quoted 5 lines
That doesn't look pretty either.
Would like to hear the maintainers view on how to handle other cases without 'led' like for example 'blue' for mk808.
Re: [PATCH v2 2/3] arm64: dts: rockchip: rk3399-roc-pc: Fix MMC numbering for LED triggers
Quoted 17 lines · Johan Jonker
Hi,
Quoted 5 lines
Quoted 5 lines
So for fixing up the LED node names, we'd probably want the following:
diy_led: led-0 yellow_led: led-1 work_led: led-2Change proposal for led nodes to comply with preexisting dts. Does this work?
diy_led:led_0: led-0yellow_led:led_1: led-1work_led:led_2: led-2
Quoted 6 lines
blue:
led_0: led-0A check does not give any warnings.
make -k ARCH=arm
dtbs_checkDT_SCHEMA_FILES=Documentation/devicetree/bindings/leds/leds-gpio.yaml
IMO the led_N labels are kind of useless... they don't convey the information needed to identify the device.
A reader seeing
&led_0 {
linux,default_triggger = "activity";
};in some device tree (overlay) has no idea what this snippet is supposed to do. The person has to go back to the base dts / dtsi file to figure that out.
But seriously, you should start a separate thread to discuss this issue.
ChenYu
Quoted 5 lines
Quoted 5 lines
That doesn't look pretty either.
Would like to hear the maintainers view on how to handle other cases without 'led' like for example 'blue' for mk808.
Re: [PATCH v2 2/3] arm64: dts: rockchip: rk3399-roc-pc: Fix MMC numbering for LED triggers
In reply to Johan JonkerQuoted 8 lines · Johan Jonker
Hi,
Quoted 5 lines
Quoted 5 lines
So for fixing up the LED node names, we'd probably want the following:
diy_led: led-0 yellow_led: led-1 work_led: led-2
Quoted 6 lines
Change proposal for led nodes to comply with preexisting dts. Does this work?
diy_led:led_0: led-0yellow_led:led_1: led-1work_led:led_2: led-2
Yuck, why?
Labels are simply human-readable source annotations for the sake of referencing nodes more easily. Meaningful label names - that correlate to SoC or board components, schematic names, or physical labels on the board/device - make the DT sources easier to read, review, and maintain. There are a handful of cases where one node might have multiple labels, e.g. if two logical supply nets come from the same regulator on certain board variants, but there is zero point in defining redundant labels that just meaninglessly echo the DT's own structure.
Quoted 3 lines
blue:
led_0: led-0A check does not give any warnings.
I should hope not. Labels are there to be consumed by DT compilers (and whatever symbolic overlay handlers count as... DT linkers, maybe?) - they have no business being within the scope of the bindings that define a contract for system software consuming the final DTB.
Robin.
Quoted 14 lines
make -k ARCH=arm
dtbs_checkDT_SCHEMA_FILES=Documentation/devicetree/bindings/leds/leds-gpio.yamlQuoted 5 lines
That doesn't look pretty either.
Would like to hear the maintainers view on how to handle other cases without 'led' like for example 'blue' for mk808._______________________________________________
Linux-rockchip mailing list [email protected]
http://lists.infradead.org/mailman/listinfo/linux-rockchip
PATCH v2 3/3 arm64: dts: rockchip: rk3328-roc-cc: Set dr_mode to "host" for OTG
In reply to Chen-Yu TsaiFrom: Chen-Yu Tsai <[email protected]>
The board has a standard USB A female port connected to the USB OTG controller's data pins. Set dr_mode in the OTG controller node to indicate this usage, instead of having the implementation guess.
--- arch/arm64/boot/dts/rockchip/rk3328-roc-cc.dts | 1 + 1 file changed, 1 insertion(+)
arch/arm64/boot/dts/rockchip/rk3328-roc-cc.dts +1 -0
@@ -334,6 +334,7 @@ &uart2 { }; &usb20_otg { + dr_mode = "host"; status = "okay"; };
Signature · 1 line
2.26.0
Hi,
Quoted 11 lines · Chen-Yu Tsai
From: Chen-Yu Tsai <[email protected]>
Hi,
Here is the remaining patch for roc-rk3399-pc, along with a few other new patches.
- Based on discussions from v1, patch one, newly added, drops the list of valid values for linux,default-triggers.
Quoted 7 lines
- Patch two is the same as in v1
- Patch three, new, sets
dr_modeto "host" to the dwc2 OTG controller, matching what the board uses it for, a host port. This gets rid of a warning from the kernel.Please have a look.
Are there any comments from the maintainers?
I feel like this is getting needlessly sidetracked by the endless discussion about GPIO LED node names which I left untouched on purpose in this series.
ChenYu
Quoted 19 lines
Regards
ChenYuChen-Yu Tsai (3):
dt-bindings: leds: common: Drop enumeration for linux,default-triggers
arm64: dts: rockchip: rk3399-roc-pc: Fix MMC numbering for LED
triggers
arm64: dts: rockchip: rk3328-roc-cc: Setdr_modeto "host" for OTG.../devicetree/bindings/leds/common.yaml | 21 +------------------ .../arm64/boot/dts/rockchip/rk3328-roc-cc.dts | 1 + .../dts/rockchip/rk3399-roc-pc-mezzanine.dts | 8 +++++++ .../boot/dts/rockchip/rk3399-roc-pc.dtsi | 4 ++-- 4 files changed, 12 insertions(+), 22 deletions(-)--
2.26.0