[1/6] arm64: dts: rockchip: rk3399-roc-pc: Fix MMC numbering for LED triggers
diff mbox series

Message ID 20200327030414.5903-2-wens@kernel.org
State New
Headers show
Series
  • arm64: dts: rockchip: misc. cleanups
Related show

Commit Message

Chen-Yu Tsai March 27, 2020, 3:04 a.m. UTC
From: Chen-Yu Tsai <wens@csie.org>

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@csie.org>
---
 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(-)

Comments

Johan Jonker March 27, 2020, 9:58 a.m. UTC | #1
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@vger.kernel.org

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']

make -k ARCH=arm64 dtbs_check
DT_SCHEMA_FILES=Documentation/devicetree/bindings/leds/leds-gpio.yaml

> From: Chen-Yu Tsai <wens@csie.org>
> 
> 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@csie.org>
> ---
>  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
Chen-Yu Tsai March 29, 2020, 4:36 p.m. UTC | #2
On Fri, Mar 27, 2020 at 5:58 PM Johan Jonker <jbx6244@gmail.com> 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@vger.kernel.org

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?

Heiko?

ChenYu

> make -k ARCH=arm64 dtbs_check
> DT_SCHEMA_FILES=Documentation/devicetree/bindings/leds/leds-gpio.yaml
>
> > From: Chen-Yu Tsai <wens@csie.org>
> >
> > 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@csie.org>
> > ---
> >  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
>
Robin Murphy March 31, 2020, 11:07 a.m. UTC | #3
[ +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@gmail.com> 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@vger.kernel.org
> 
> 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 
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@csie.org>
>>>
>>> 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@csie.org>
>>> ---
>>>   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@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip
>
Jacek Anaszewski April 1, 2020, 8:05 p.m. UTC | #4
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@gmail.com> 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@vger.kernel.org
>>
>> 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@csie.org>
>>>>
>>>> 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@csie.org>
>>>> ---
>>>>   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@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-rockchip
>>
Pavel Machek April 6, 2020, 9:13 a.m. UTC | #5
Hi!

> > > 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

It is enough to keep it in sync with all the triggers we actually use :-).

> 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 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.

But freeform seems acceptable, too.

> > > > -             diy-led {
> > > > +             diy_led: diy-led {
> > > >                        label = "red:diy";
> > > >                        gpios = <&gpio0 RK_PB5 GPIO_ACTIVE_HIGH>;
> > > >                        default-state = "off";
> > > >                        linux,default-trigger = "mmc1";
> > > >                };

This label probably should be "mmc1:red:activity" or something like that.

> > > > -             yellow-led {
> > > > +             yellow_led: yellow-led {
> > > >                        label = "yellow:yellow-led";
> > > >                        gpios = <&gpio0 RK_PA2 GPIO_ACTIVE_HIGH>;
> > > >                        default-state = "off";

And this label should be changed, too.

Best regards,
									Pavel
Chen-Yu Tsai April 13, 2020, 5:33 a.m. UTC | #6
Hi Pavel,

On Mon, Apr 6, 2020 at 5:13 PM Pavel Machek <pavel@ucw.cz> wrote:
>
> Hi!
>
> > > > 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
>
> It is enough to keep it in sync with all the triggers we actually use :-).
>
> > 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 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.
>
> But freeform seems acceptable, too.

I'd say free form is easier to manage. Being in the list of valid triggers
doesn't mean the kernel actually has support for it enabled.

> > > > > -             diy-led {
> > > > > +             diy_led: diy-led {
> > > > >                        label = "red:diy";
> > > > >                        gpios = <&gpio0 RK_PB5 GPIO_ACTIVE_HIGH>;
> > > > >                        default-state = "off";
> > > > >                        linux,default-trigger = "mmc1";
> > > > >                };
>
> This label probably should be "mmc1:red:activity" or something like that.

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.

Regards
ChenYu


[1] http://wiki.t-firefly.com/en/ROC-RK3399-PC/driver_led.html

> > > > > -             yellow-led {
> > > > > +             yellow_led: yellow-led {
> > > > >                        label = "yellow:yellow-led";
> > > > >                        gpios = <&gpio0 RK_PA2 GPIO_ACTIVE_HIGH>;
> > > > >                        default-state = "off";
>
> And this label should be changed, too.
>
> Best regards,
>                                                                         Pavel
> --
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Patch
diff mbox series

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";