diff mbox series

[6/6] arm64: dts: xilinx: ultra96: Standardize LED labels and triggers

Message ID 20181009140522.6492-7-manivannan.sadhasivam@linaro.org (mailing list archive)
State New, archived
Headers show
Series Standardize onboard LED support for 96Boards | expand

Commit Message

'Manivannan Sadhasivam' Oct. 9, 2018, 2:05 p.m. UTC
For all 96Boards, the following standard is used for onboard LEDs.

device-name:green:user1  default-trigger: heartbeat
device-name:green:user2  default-trigger: mmc0/disk-activity
                                          (onboard-storage)
device-name:green:user3  default-trigger: mmc1 (SD-card)
device-name:green:user4  default-trigger: none, panic-indicator
device-name:yellow:wlan  default-trigger: phy0tx
device-name:blue:bt      default-trigger: hci0-power

So lets adopt the same for Ultra96, which is one of the 96Boards
CE and AI platform. Since the WLAN and BT LEDs are hardwired onboard,
consolidate only User LEDs.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 arch/arm64/boot/dts/xilinx/zynqmp-zcu100-revC.dts | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

Comments

Michal Simek Oct. 10, 2018, 7:25 a.m. UTC | #1
On 9.10.2018 16:05, Manivannan Sadhasivam wrote:
> For all 96Boards, the following standard is used for onboard LEDs.
> 
> device-name:green:user1  default-trigger: heartbeat
> device-name:green:user2  default-trigger: mmc0/disk-activity
>                                           (onboard-storage)
> device-name:green:user3  default-trigger: mmc1 (SD-card)
> device-name:green:user4  default-trigger: none, panic-indicator
> device-name:yellow:wlan  default-trigger: phy0tx
> device-name:blue:bt      default-trigger: hci0-power
> 
> So lets adopt the same for Ultra96, which is one of the 96Boards
> CE and AI platform. Since the WLAN and BT LEDs are hardwired onboard,
> consolidate only User LEDs.
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>  arch/arm64/boot/dts/xilinx/zynqmp-zcu100-revC.dts | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/xilinx/zynqmp-zcu100-revC.dts b/arch/arm64/boot/dts/xilinx/zynqmp-zcu100-revC.dts
> index eb5e8bddb610..565ceb390f99 100644
> --- a/arch/arm64/boot/dts/xilinx/zynqmp-zcu100-revC.dts
> +++ b/arch/arm64/boot/dts/xilinx/zynqmp-zcu100-revC.dts
> @@ -57,29 +57,30 @@
>  	leds {
>  		compatible = "gpio-leds";
>  		ds2 {
> -			label = "ds2";
> +			label = "zynqmp-zcu100-revC:green:user1";

The purpose of this series should be standardize Led labels that's why I
would expect that leds will be called in the same way on all these
boards that's why I am not getting why there is board name.

green:user1 or just user1 should be enough.

Thanks,
Michal
'Manivannan Sadhasivam' Oct. 10, 2018, 7:35 a.m. UTC | #2
Hi Michal,

On Wed, Oct 10, 2018 at 09:25:24AM +0200, Michal Simek wrote:
> On 9.10.2018 16:05, Manivannan Sadhasivam wrote:
> > For all 96Boards, the following standard is used for onboard LEDs.
> > 
> > device-name:green:user1  default-trigger: heartbeat
> > device-name:green:user2  default-trigger: mmc0/disk-activity
> >                                           (onboard-storage)
> > device-name:green:user3  default-trigger: mmc1 (SD-card)
> > device-name:green:user4  default-trigger: none, panic-indicator
> > device-name:yellow:wlan  default-trigger: phy0tx
> > device-name:blue:bt      default-trigger: hci0-power
> > 
> > So lets adopt the same for Ultra96, which is one of the 96Boards
> > CE and AI platform. Since the WLAN and BT LEDs are hardwired onboard,
> > consolidate only User LEDs.
> > 
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > ---
> >  arch/arm64/boot/dts/xilinx/zynqmp-zcu100-revC.dts | 15 ++++++++-------
> >  1 file changed, 8 insertions(+), 7 deletions(-)
> > 
> > diff --git a/arch/arm64/boot/dts/xilinx/zynqmp-zcu100-revC.dts b/arch/arm64/boot/dts/xilinx/zynqmp-zcu100-revC.dts
> > index eb5e8bddb610..565ceb390f99 100644
> > --- a/arch/arm64/boot/dts/xilinx/zynqmp-zcu100-revC.dts
> > +++ b/arch/arm64/boot/dts/xilinx/zynqmp-zcu100-revC.dts
> > @@ -57,29 +57,30 @@
> >  	leds {
> >  		compatible = "gpio-leds";
> >  		ds2 {
> > -			label = "ds2";
> > +			label = "zynqmp-zcu100-revC:green:user1";
> 
> The purpose of this series should be standardize Led labels that's why I
> would expect that leds will be called in the same way on all these
> boards that's why I am not getting why there is board name.
> 
> green:user1 or just user1 should be enough.
> 

Hmm. That's a good point. My proposal to standardize the labels is inspired from
Dragonboard410c dts [1]. They had the device name in label and I decided to
go with that. But as you said, the board name is not needed and just 
`color:led-name` should be fine.

Will modify this in next interation.

Thanks,
Mani

[1] https://github.com/torvalds/linux/blob/master/arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi#L294

> Thanks,
> Michal
Michal Simek Oct. 10, 2018, 7:40 a.m. UTC | #3
On 10.10.2018 09:35, Manivannan Sadhasivam wrote:
> Hi Michal,
> 
> On Wed, Oct 10, 2018 at 09:25:24AM +0200, Michal Simek wrote:
>> On 9.10.2018 16:05, Manivannan Sadhasivam wrote:
>>> For all 96Boards, the following standard is used for onboard LEDs.
>>>
>>> device-name:green:user1  default-trigger: heartbeat
>>> device-name:green:user2  default-trigger: mmc0/disk-activity
>>>                                           (onboard-storage)
>>> device-name:green:user3  default-trigger: mmc1 (SD-card)
>>> device-name:green:user4  default-trigger: none, panic-indicator
>>> device-name:yellow:wlan  default-trigger: phy0tx
>>> device-name:blue:bt      default-trigger: hci0-power
>>>
>>> So lets adopt the same for Ultra96, which is one of the 96Boards
>>> CE and AI platform. Since the WLAN and BT LEDs are hardwired onboard,
>>> consolidate only User LEDs.
>>>
>>> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
>>> ---
>>>  arch/arm64/boot/dts/xilinx/zynqmp-zcu100-revC.dts | 15 ++++++++-------
>>>  1 file changed, 8 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/arch/arm64/boot/dts/xilinx/zynqmp-zcu100-revC.dts b/arch/arm64/boot/dts/xilinx/zynqmp-zcu100-revC.dts
>>> index eb5e8bddb610..565ceb390f99 100644
>>> --- a/arch/arm64/boot/dts/xilinx/zynqmp-zcu100-revC.dts
>>> +++ b/arch/arm64/boot/dts/xilinx/zynqmp-zcu100-revC.dts
>>> @@ -57,29 +57,30 @@
>>>  	leds {
>>>  		compatible = "gpio-leds";
>>>  		ds2 {
>>> -			label = "ds2";
>>> +			label = "zynqmp-zcu100-revC:green:user1";
>>
>> The purpose of this series should be standardize Led labels that's why I
>> would expect that leds will be called in the same way on all these
>> boards that's why I am not getting why there is board name.
>>
>> green:user1 or just user1 should be enough.
>>
> 
> Hmm. That's a good point. My proposal to standardize the labels is inspired from
> Dragonboard410c dts [1]. They had the device name in label and I decided to
> go with that. But as you said, the board name is not needed and just 
> `color:led-name` should be fine.
> 
> Will modify this in next interation.

The second part of this is that all current DTSes are using some names
and by changing it you will break compatibility which is something what
you should avoid.
That's why it is really a question if this change should be accepted.

Thanks,
Michal
'Manivannan Sadhasivam' Oct. 10, 2018, 7:46 a.m. UTC | #4
On Wed, Oct 10, 2018 at 09:40:22AM +0200, Michal Simek wrote:
> On 10.10.2018 09:35, Manivannan Sadhasivam wrote:
> > Hi Michal,
> > 
> > On Wed, Oct 10, 2018 at 09:25:24AM +0200, Michal Simek wrote:
> >> On 9.10.2018 16:05, Manivannan Sadhasivam wrote:
> >>> For all 96Boards, the following standard is used for onboard LEDs.
> >>>
> >>> device-name:green:user1  default-trigger: heartbeat
> >>> device-name:green:user2  default-trigger: mmc0/disk-activity
> >>>                                           (onboard-storage)
> >>> device-name:green:user3  default-trigger: mmc1 (SD-card)
> >>> device-name:green:user4  default-trigger: none, panic-indicator
> >>> device-name:yellow:wlan  default-trigger: phy0tx
> >>> device-name:blue:bt      default-trigger: hci0-power
> >>>
> >>> So lets adopt the same for Ultra96, which is one of the 96Boards
> >>> CE and AI platform. Since the WLAN and BT LEDs are hardwired onboard,
> >>> consolidate only User LEDs.
> >>>
> >>> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> >>> ---
> >>>  arch/arm64/boot/dts/xilinx/zynqmp-zcu100-revC.dts | 15 ++++++++-------
> >>>  1 file changed, 8 insertions(+), 7 deletions(-)
> >>>
> >>> diff --git a/arch/arm64/boot/dts/xilinx/zynqmp-zcu100-revC.dts b/arch/arm64/boot/dts/xilinx/zynqmp-zcu100-revC.dts
> >>> index eb5e8bddb610..565ceb390f99 100644
> >>> --- a/arch/arm64/boot/dts/xilinx/zynqmp-zcu100-revC.dts
> >>> +++ b/arch/arm64/boot/dts/xilinx/zynqmp-zcu100-revC.dts
> >>> @@ -57,29 +57,30 @@
> >>>  	leds {
> >>>  		compatible = "gpio-leds";
> >>>  		ds2 {
> >>> -			label = "ds2";
> >>> +			label = "zynqmp-zcu100-revC:green:user1";
> >>
> >> The purpose of this series should be standardize Led labels that's why I
> >> would expect that leds will be called in the same way on all these
> >> boards that's why I am not getting why there is board name.
> >>
> >> green:user1 or just user1 should be enough.
> >>
> > 
> > Hmm. That's a good point. My proposal to standardize the labels is inspired from
> > Dragonboard410c dts [1]. They had the device name in label and I decided to
> > go with that. But as you said, the board name is not needed and just 
> > `color:led-name` should be fine.
> > 
> > Will modify this in next interation.
> 
> The second part of this is that all current DTSes are using some names
> and by changing it you will break compatibility which is something what
> you should avoid.
> That's why it is really a question if this change should be accepted.
>

You mean userspace compatibility? If yes, then the affected boards are using
MRAA library for providing the userspace access and I will ensure that it will
get updated to the new labels.

But for other userspace scripts which users might be depending on, it will
break for sure. So, I would like to get some feedback here from Rob.

Thanks,
Mani
 
> Thanks,
> Michal
>
Pavel Machek Oct. 15, 2018, 7:56 p.m. UTC | #5
On Wed 2018-10-10 09:25:24, Michal Simek wrote:
> On 9.10.2018 16:05, Manivannan Sadhasivam wrote:
> > For all 96Boards, the following standard is used for onboard LEDs.
> > 
> > device-name:green:user1  default-trigger: heartbeat
> > device-name:green:user2  default-trigger: mmc0/disk-activity
> >                                           (onboard-storage)
> > device-name:green:user3  default-trigger: mmc1 (SD-card)
> > device-name:green:user4  default-trigger: none, panic-indicator

Actually, having one LED as heartbeat and second as panic indicator
only... Could those be merged and remaining LED used to something
useful?

> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > ---

> > index eb5e8bddb610..565ceb390f99 100644
> > --- a/arch/arm64/boot/dts/xilinx/zynqmp-zcu100-revC.dts
> > +++ b/arch/arm64/boot/dts/xilinx/zynqmp-zcu100-revC.dts
> > @@ -57,29 +57,30 @@
> >  	leds {
> >  		compatible = "gpio-leds";
> >  		ds2 {
> > -			label = "ds2";
> > +			label = "zynqmp-zcu100-revC:green:user1";
> 
> The purpose of this series should be standardize Led labels that's why I
> would expect that leds will be called in the same way on all these
> boards that's why I am not getting why there is board name.
> 
> green:user1 or just user1 should be enough.

green:user1 is better. Also... I guess this should go to the LEDs
list... and if you want to make names standard, maybe there's better
naming than user1..user4?

									Pavel
'Manivannan Sadhasivam' Oct. 22, 2018, 6:50 a.m. UTC | #6
Hi Pavel,

Sorry for the late reply! Not sure why your resonse was filtered by the
Spam filter :\

On Mon, Oct 15, 2018 at 09:56:01PM +0200, Pavel Machek wrote:
> On Wed 2018-10-10 09:25:24, Michal Simek wrote:
> > On 9.10.2018 16:05, Manivannan Sadhasivam wrote:
> > > For all 96Boards, the following standard is used for onboard LEDs.
> > > 
> > > device-name:green:user1  default-trigger: heartbeat
> > > device-name:green:user2  default-trigger: mmc0/disk-activity
> > >                                           (onboard-storage)
> > > device-name:green:user3  default-trigger: mmc1 (SD-card)
> > > device-name:green:user4  default-trigger: none, panic-indicator
> 
> Actually, having one LED as heartbeat and second as panic indicator
> only... Could those be merged and remaining LED used to something
> useful?
>

No. Our intention is to leave one LED as a panic indicator and a dedicated
LED for heartbeat. If user wants to override the defaults, he can always do
with any LEDs.
 
> > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > ---
> 
> > > index eb5e8bddb610..565ceb390f99 100644
> > > --- a/arch/arm64/boot/dts/xilinx/zynqmp-zcu100-revC.dts
> > > +++ b/arch/arm64/boot/dts/xilinx/zynqmp-zcu100-revC.dts
> > > @@ -57,29 +57,30 @@
> > >  	leds {
> > >  		compatible = "gpio-leds";
> > >  		ds2 {
> > > -			label = "ds2";
> > > +			label = "zynqmp-zcu100-revC:green:user1";
> > 
> > The purpose of this series should be standardize Led labels that's why I
> > would expect that leds will be called in the same way on all these
> > boards that's why I am not getting why there is board name.
> > 
> > green:user1 or just user1 should be enough.
> 
> green:user1 is better. Also... I guess this should go to the LEDs
> list... and if you want to make names standard, maybe there's better
> naming than user1..user4?
> 

These are user LEDs and that's why the `userN` naming convention comes into
place. Here I agree to move with "green:user1" format and sending it to the
LEDs list.

Thanks,
Mani

> 									Pavel
> -- 
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
diff mbox series

Patch

diff --git a/arch/arm64/boot/dts/xilinx/zynqmp-zcu100-revC.dts b/arch/arm64/boot/dts/xilinx/zynqmp-zcu100-revC.dts
index eb5e8bddb610..565ceb390f99 100644
--- a/arch/arm64/boot/dts/xilinx/zynqmp-zcu100-revC.dts
+++ b/arch/arm64/boot/dts/xilinx/zynqmp-zcu100-revC.dts
@@ -57,29 +57,30 @@ 
 	leds {
 		compatible = "gpio-leds";
 		ds2 {
-			label = "ds2";
+			label = "zynqmp-zcu100-revC:green:user1";
 			gpios = <&gpio 20 GPIO_ACTIVE_HIGH>;
 			linux,default-trigger = "heartbeat";
 		};
 
 		ds3 {
-			label = "ds3";
+			label = "zynqmp-zcu100-revC:green:user2";
 			gpios = <&gpio 19 GPIO_ACTIVE_HIGH>;
-			linux,default-trigger = "phy0tx"; /* WLAN tx */
+			linux,default-trigger = "mmc0";
 			default-state = "off";
 		};
 
 		ds4 {
-			label = "ds4";
+			label = "zynqmp-zcu100-revC:green:user3";
 			gpios = <&gpio 18 GPIO_ACTIVE_HIGH>;
-			linux,default-trigger = "phy0rx"; /* WLAN rx */
+			linux,default-trigger = "mmc1";
 			default-state = "off";
 		};
 
 		ds5 {
-			label = "ds5";
+			label = "zynqmp-zcu100-revC:green:user4";
 			gpios = <&gpio 17 GPIO_ACTIVE_HIGH>;
-			linux,default-trigger = "bluetooth-power";
+			linux,default-trigger = "none";
+			panic-indicator;
 		};
 
 		vbus_det { /* U5 USB5744 VBUS detection via MIO25 */