Re: [PATCH 1/6] arm64: dts: rockchip: rk3399-roc-pc: Fix MMC numbering for LED triggers

[Jacek Anaszewski]

On 3/31/20 1:07 PM, Robin Murphy wrote:
> [ +cc LED binding maintainers]
> 
> On 2020-03-29 5:36 pm, Chen-Yu Tsai wrote:
>> On Fri, Mar 27, 2020 at 5:58 PM Johan Jonker <jbx6244@xxxxxxxxx> wrote:
>>>
>>> Hi Chen-Yu Tsai,
>>>
>>> The led node names need some changes.
>>> 'linux,default-trigger' value does not fit.
>>>
>>> ÂFrom leds-gpio.yaml:
>>>
>>> patternProperties:
>>> ÂÂ # The first form is preferred, but fall back to just 'led'
>>> anywhere in the
>>> ÂÂ # node name to at least catch some child nodes.
>>> ÂÂ "(^led-[0-9a-f]$|led)":
>>> ÂÂÂÂ type: object
>>>
>>> Rename led nodenames to 'led-0' form
>>>
>>> Also include all mail lists found with:
>>> ./scripts/get_maintainer.pl --nogit-fallback --nogit
>>>
>>> devicetree@xxxxxxxxxxxxxxx
>>
>> Oops...
>>
>>> If you like change the rest of dts with leds as well...
>>>
>>> ÂÂ DTCÂÂÂÂ arch/arm64/boot/dts/rockchip/rk3399-roc-pc.dt.yaml
>>> ÂÂ CHECKÂÂ arch/arm64/boot/dts/rockchip/rk3399-roc-pc.dt.yaml
>>> arch/arm64/boot/dts/rockchip/rk3399-roc-pc.dt.yaml: leds:
>>> yellow-led:linux,default-trigger:0: 'mmc0' is not one of ['backlight',
>>> 'default-on', 'heartbeat', 'disk-activity', 'ide-disk', 'timer',
>>> 'pattern']
>>> arch/arm64/boot/dts/rockchip/rk3399-roc-pc.dt.yaml: leds:
>>> diy-led:linux,default-trigger:0: 'mmc1' is not one of ['backlight',
>>> 'default-on', 'heartbeat', 'disk-activity', 'ide-disk', 'timer',
>>> 'pattern']
>>> ÂÂ DTCÂÂÂÂ arch/arm64/boot/dts/rockchip/rk3399-roc-pc-mezzanine.dt.yaml
>>> ÂÂ CHECKÂÂ arch/arm64/boot/dts/rockchip/rk3399-roc-pc-mezzanine.dt.yaml
>>> arch/arm64/boot/dts/rockchip/rk3399-roc-pc-mezzanine.dt.yaml: leds:
>>> diy-led:linux,default-trigger:0: 'mmc2' is not one of ['backlight',
>>> 'default-on', 'heartbeat', 'disk-activity', 'ide-disk', 'timer',
>>> 'pattern']
>>> arch/arm64/boot/dts/rockchip/rk3399-roc-pc-mezzanine.dt.yaml: leds:
>>> yellow-led:linux,default-trigger:0: 'mmc1' is not one of ['backlight',
>>> 'default-on', 'heartbeat', 'disk-activity', 'ide-disk', 'timer',
>>> 'pattern']
>>
>> Maybe we should just get rid of linux,default-trigger then?
> 
> In this particular case, I'd say it's probably time to reevaluate the
> rather out-of-date binding. The apparent intent of the
> "linux,default-trigger" property seems to be to describe any trigger
> supported by Linux, so either the binding wants to be kept in sync with
> all the triggers Linux actually supports, or perhaps it should just be
> redefined as a free-form string. FWIW I'd be slightly inclined towards
> the latter, since the schema validator can't know whether the given
> trigger actually corresponds to the correct thing for whatever the LED

I agree. It is possible for LED class drivers to register their private
triggers, so generic bindings cannot use predefined set for validation.

I think that Rob just blindly converted common.txt to yaml and I acked
that but didn't envisage the implications for validation.
All in all, even that list in common.txt didn't include all existing
generic LED triggers.

Best regards,
Jacek Anaszewski

> is physically labelled on the board/case, nor whether the version(s) of
> Linux that people intend to use actually support that trigger (since it
> doesn't have to be the version contemporary with the schema definition),
> so strict validation of this particular property seems to be of limited
> value.
> 
> Robin.
> 
>>
>> Heiko?
>>
>> ChenYu
>>
>>> make -k ARCH=arm64 dtbs_check
>>> DT_SCHEMA_FILES=Documentation/devicetree/bindings/leds/leds-gpio.yaml
>>>
>>>> From: Chen-Yu Tsai <wens@xxxxxxxx>
>>>>
>>>> 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 <wens@xxxxxxxx>
>>>> ---
>>>> Â 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(-)
>>>>
>>>> diff --git
>>>> a/arch/arm64/boot/dts/rockchip/rk3399-roc-pc-mezzanine.dts
>>>> b/arch/arm64/boot/dts/rockchip/rk3399-roc-pc-mezzanine.dts
>>>> index 2acb3d500fb9..f0686fc276be 100644
>>>> --- a/arch/arm64/boot/dts/rockchip/rk3399-roc-pc-mezzanine.dts
>>>> +++ b/arch/arm64/boot/dts/rockchip/rk3399-roc-pc-mezzanine.dts
>>>> @@ -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";
>>>> +};
>>>> diff --git a/arch/arm64/boot/dts/rockchip/rk3399-roc-pc.dtsi
>>>> b/arch/arm64/boot/dts/rockchip/rk3399-roc-pc.dtsi
>>>> index 9f225e9c3d54..bc060ac7972d 100644
>>>> --- a/arch/arm64/boot/dts/rockchip/rk3399-roc-pc.dtsi
>>>> +++ b/arch/arm64/boot/dts/rockchip/rk3399-roc-pc.dtsi
>>>> @@ -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";
>>>> -- 
>>>> 2.25.1
>>>
>>
>> _______________________________________________
>> Linux-rockchip mailing list
>> Linux-rockchip@xxxxxxxxxxxxxxxxxxx
>> http://lists.infradead.org/mailman/listinfo/linux-rockchip
>>