diff mbox series

[linux,dev-5.7,2/2] rainier: Add LEDs that are controlled by ASPEED

Message ID 1596022243-8788-1-git-send-email-vishwa@linux.vnet.ibm.com
State New
Headers show
Series [linux,dev-5.7,1/2] rainier: Add leds that are off 9551 on Operator Panel | expand

Commit Message

vishwanatha subbanna July 29, 2020, 11:30 a.m. UTC 
From: Vishwanatha Subbanna <vishwa@linux.ibm.com>

These are the LEDs that have direct GPIO connection from ASPEED

Signed-off-by: Vishwanatha Subbanna <vishwa@linux.ibm.com>
Reviewed-by: Eddie James <eajames@linux.ibm.com>
---
 arch/arm/boot/dts/aspeed-bmc-ibm-rainier.dts | 24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

Comments

Joel Stanley Aug. 13, 2020, 3:15 a.m. UTC | #1 
On Wed, 29 Jul 2020 at 11:30, <vishwa@linux.vnet.ibm.com> wrote:
>
> From: Vishwanatha Subbanna <vishwa@linux.ibm.com>
>
> These are the LEDs that have direct GPIO connection from ASPEED

Do you mean directly connected to the BMC, and not via a GPIO expander?

Convetion is to name the patches with a prefix:

 ARM: dts: aspeed: rainier: Add directly controlled LEDs


>
> Signed-off-by: Vishwanatha Subbanna <vishwa@linux.ibm.com>
> Reviewed-by: Eddie James <eajames@linux.ibm.com>
> ---
>  arch/arm/boot/dts/aspeed-bmc-ibm-rainier.dts | 24 ++++++++++++++++++++++--
>  1 file changed, 22 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm/boot/dts/aspeed-bmc-ibm-rainier.dts b/arch/arm/boot/dts/aspeed-bmc-ibm-rainier.dts
> index 9b28a30..d1a588f 100644
> --- a/arch/arm/boot/dts/aspeed-bmc-ibm-rainier.dts
> +++ b/arch/arm/boot/dts/aspeed-bmc-ibm-rainier.dts
> @@ -124,6 +124,26 @@
>         leds {
>                 compatible = "gpio-leds";
>
> +               /* BMC Card fault LED at the back */
> +               bmc-ingraham0 {
> +                       gpios = <&gpio0 ASPEED_GPIO(H, 1) GPIO_ACTIVE_LOW>;
> +               };
> +
> +               /* Enclosure ID LED at the back */
> +               rear-enc-id0 {
> +                       gpios = <&gpio0 ASPEED_GPIO(H, 2) GPIO_ACTIVE_LOW>;
> +               };
> +
> +               /* Enclosure fault LED at the back */
> +               rear-enc-fault0 {
> +                       gpios = <&gpio0 ASPEED_GPIO(H, 3) GPIO_ACTIVE_LOW>;
> +               };
> +
> +               /* PCIE slot power LED */
> +               pcieslot-power {
> +                       gpios = <&gpio0 ASPEED_GPIO(P, 4) GPIO_ACTIVE_LOW>;
> +               };
> +
>                 /* System ID LED that is at front on Op Panel */
>                 front-sys-id0 {
>                         retain-state-shutdown;
> @@ -167,7 +187,7 @@
>         /*E0-E7*/       "","","","","","","","",
>         /*F0-F7*/       "","","","","","","","",
>         /*G0-G7*/       "","","","","","","","",
> -       /*H0-H7*/       "","","","","","","","",
> +       /*H0-H7*/       "","bmc-ingraham0","rear-enc-id0","rear-enc-fault0","","","","",

I think the guideline is to name GPIOs based on their use, so
bmc-ingraham0 should be fault-bmc-ingraham0 if we follow the psu
presence GPIO convention of function-name.

I think we had some documentation written up on naming conventions.

Brad, do you have any input here?

>         /*I0-I7*/       "","","","","","","","",
>         /*J0-J7*/       "","","","","","","","",
>         /*K0-K7*/       "","","","","","","","",
> @@ -175,7 +195,7 @@
>         /*M0-M7*/       "","","","","","","","",
>         /*N0-N7*/       "","","","","","","","",
>         /*O0-O7*/       "","","","usb-power","","","","",
> -       /*P0-P7*/       "","","","","","","","",
> +       /*P0-P7*/       "","","","","pcieslot-power","","","",
>         /*Q0-Q7*/       "cfam-reset","","","","","","","",
>         /*R0-R7*/       "","","","","","","","",
>         /*S0-S7*/       "presence-ps0","presence-ps1","presence-ps2","presence-ps3",
> --
> 1.8.3.1
>
vishwanatha subbanna Aug. 13, 2020, 6:35 a.m. UTC | #2 
On 13/08/20, 8:45 AM, "Joel Stanley" <joel@jms.id.au> wrote:

    On Wed, 29 Jul 2020 at 11:30, <vishwa@linux.vnet.ibm.com> wrote:
    >
    > From: Vishwanatha Subbanna <vishwa@linux.ibm.com>
    >
    > These are the LEDs that have direct GPIO connection from ASPEED

    Do you mean directly connected to the BMC, and not via a GPIO expander?

Correct. Directly connected to ASPEED.

    Convetion is to name the patches with a prefix:

     ARM: dts: aspeed: rainier: Add directly controlled LEDs

I will update

    >
    > Signed-off-by: Vishwanatha Subbanna <vishwa@linux.ibm.com>
    > Reviewed-by: Eddie James <eajames@linux.ibm.com>
    > ---
    >  arch/arm/boot/dts/aspeed-bmc-ibm-rainier.dts | 24 ++++++++++++++++++++++--
    >  1 file changed, 22 insertions(+), 2 deletions(-)
    >
    > diff --git a/arch/arm/boot/dts/aspeed-bmc-ibm-rainier.dts b/arch/arm/boot/dts/aspeed-bmc-ibm-rainier.dts
    > index 9b28a30..d1a588f 100644
    > --- a/arch/arm/boot/dts/aspeed-bmc-ibm-rainier.dts
    > +++ b/arch/arm/boot/dts/aspeed-bmc-ibm-rainier.dts
    > @@ -124,6 +124,26 @@
    >         leds {
    >                 compatible = "gpio-leds";
    >
    > +               /* BMC Card fault LED at the back */
    > +               bmc-ingraham0 {
    > +                       gpios = <&gpio0 ASPEED_GPIO(H, 1) GPIO_ACTIVE_LOW>;
    > +               };
    > +
    > +               /* Enclosure ID LED at the back */
    > +               rear-enc-id0 {
    > +                       gpios = <&gpio0 ASPEED_GPIO(H, 2) GPIO_ACTIVE_LOW>;
    > +               };
    > +
    > +               /* Enclosure fault LED at the back */
    > +               rear-enc-fault0 {
    > +                       gpios = <&gpio0 ASPEED_GPIO(H, 3) GPIO_ACTIVE_LOW>;
    > +               };
    > +
    > +               /* PCIE slot power LED */
    > +               pcieslot-power {
    > +                       gpios = <&gpio0 ASPEED_GPIO(P, 4) GPIO_ACTIVE_LOW>;
    > +               };
    > +
    >                 /* System ID LED that is at front on Op Panel */
    >                 front-sys-id0 {
    >                         retain-state-shutdown;
    > @@ -167,7 +187,7 @@
    >         /*E0-E7*/       "","","","","","","","",
    >         /*F0-F7*/       "","","","","","","","",
    >         /*G0-G7*/       "","","","","","","","",
    > -       /*H0-H7*/       "","","","","","","","",
    > +       /*H0-H7*/       "","bmc-ingraham0","rear-enc-id0","rear-enc-fault0","","","","",

    I think the guideline is to name GPIOs based on their use, so
    bmc-ingraham0 should be fault-bmc-ingraham0 if we follow the psu
    presence GPIO convention of function-name.

This LED is used for Identification and Fault as well.

    I think we had some documentation written up on naming conventions.

    Brad, do you have any input here?

    >         /*I0-I7*/       "","","","","","","","",
    >         /*J0-J7*/       "","","","","","","","",
    >         /*K0-K7*/       "","","","","","","","",
    > @@ -175,7 +195,7 @@
    >         /*M0-M7*/       "","","","","","","","",
    >         /*N0-N7*/       "","","","","","","","",
    >         /*O0-O7*/       "","","","usb-power","","","","",
    > -       /*P0-P7*/       "","","","","","","","",
    > +       /*P0-P7*/       "","","","","pcieslot-power","","","",
    >         /*Q0-Q7*/       "cfam-reset","","","","","","","",
    >         /*R0-R7*/       "","","","","","","","",
    >         /*S0-S7*/       "presence-ps0","presence-ps1","presence-ps2","presence-ps3",
    > --
    > 1.8.3.1
    >
Deepak Kodihalli Aug. 13, 2020, 9:01 a.m. UTC | #3 
On 13/08/20 8:45 am, Joel Stanley wrote:
> On Wed, 29 Jul 2020 at 11:30, <vishwa@linux.vnet.ibm.com> wrote:
>>
>> From: Vishwanatha Subbanna <vishwa@linux.ibm.com>
>>
>> These are the LEDs that have direct GPIO connection from ASPEED
> 
> Do you mean directly connected to the BMC, and not via a GPIO expander?
> 
> Convetion is to name the patches with a prefix:
> 
>   ARM: dts: aspeed: rainier: Add directly controlled LEDs
> 
> 
>>
>> Signed-off-by: Vishwanatha Subbanna <vishwa@linux.ibm.com>
>> Reviewed-by: Eddie James <eajames@linux.ibm.com>
>> ---
>>   arch/arm/boot/dts/aspeed-bmc-ibm-rainier.dts | 24 ++++++++++++++++++++++--
>>   1 file changed, 22 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/arm/boot/dts/aspeed-bmc-ibm-rainier.dts b/arch/arm/boot/dts/aspeed-bmc-ibm-rainier.dts
>> index 9b28a30..d1a588f 100644
>> --- a/arch/arm/boot/dts/aspeed-bmc-ibm-rainier.dts
>> +++ b/arch/arm/boot/dts/aspeed-bmc-ibm-rainier.dts
>> @@ -124,6 +124,26 @@
>>          leds {
>>                  compatible = "gpio-leds";
>>
>> +               /* BMC Card fault LED at the back */
>> +               bmc-ingraham0 {
>> +                       gpios = <&gpio0 ASPEED_GPIO(H, 1) GPIO_ACTIVE_LOW>;
>> +               };
>> +
>> +               /* Enclosure ID LED at the back */
>> +               rear-enc-id0 {
>> +                       gpios = <&gpio0 ASPEED_GPIO(H, 2) GPIO_ACTIVE_LOW>;
>> +               };
>> +
>> +               /* Enclosure fault LED at the back */
>> +               rear-enc-fault0 {
>> +                       gpios = <&gpio0 ASPEED_GPIO(H, 3) GPIO_ACTIVE_LOW>;
>> +               };
>> +
>> +               /* PCIE slot power LED */
>> +               pcieslot-power {
>> +                       gpios = <&gpio0 ASPEED_GPIO(P, 4) GPIO_ACTIVE_LOW>;
>> +               };
>> +
>>                  /* System ID LED that is at front on Op Panel */
>>                  front-sys-id0 {
>>                          retain-state-shutdown;
>> @@ -167,7 +187,7 @@
>>          /*E0-E7*/       "","","","","","","","",
>>          /*F0-F7*/       "","","","","","","","",
>>          /*G0-G7*/       "","","","","","","","",
>> -       /*H0-H7*/       "","","","","","","","",
>> +       /*H0-H7*/       "","bmc-ingraham0","rear-enc-id0","rear-enc-fault0","","","","",
> 
> I think the guideline is to name GPIOs based on their use, so
> bmc-ingraham0 should be fault-bmc-ingraham0 if we follow the psu
> presence GPIO convention of function-name.
> 
> I think we had some documentation written up on naming conventions.

There is 
https://github.com/openbmc/docs/blob/master/designs/device-tree-gpio-naming.md.

Regards,
Deepak
diff mbox series

Patch

diff --git a/arch/arm/boot/dts/aspeed-bmc-ibm-rainier.dts b/arch/arm/boot/dts/aspeed-bmc-ibm-rainier.dts
index 9b28a30..d1a588f 100644
--- a/arch/arm/boot/dts/aspeed-bmc-ibm-rainier.dts
+++ b/arch/arm/boot/dts/aspeed-bmc-ibm-rainier.dts
@@ -124,6 +124,26 @@ 
 	leds {
 		compatible = "gpio-leds";
 
+		/* BMC Card fault LED at the back */
+		bmc-ingraham0 {
+			gpios = <&gpio0 ASPEED_GPIO(H, 1) GPIO_ACTIVE_LOW>;
+		};
+
+		/* Enclosure ID LED at the back */
+		rear-enc-id0 {
+			gpios = <&gpio0 ASPEED_GPIO(H, 2) GPIO_ACTIVE_LOW>;
+		};
+
+		/* Enclosure fault LED at the back */
+		rear-enc-fault0 {
+			gpios = <&gpio0 ASPEED_GPIO(H, 3) GPIO_ACTIVE_LOW>;
+		};
+
+		/* PCIE slot power LED */
+		pcieslot-power {
+			gpios = <&gpio0 ASPEED_GPIO(P, 4) GPIO_ACTIVE_LOW>;
+		};
+
 		/* System ID LED that is at front on Op Panel */
 		front-sys-id0 {
 			retain-state-shutdown;
@@ -167,7 +187,7 @@ 
 	/*E0-E7*/	"","","","","","","","",
 	/*F0-F7*/	"","","","","","","","",
 	/*G0-G7*/	"","","","","","","","",
-	/*H0-H7*/	"","","","","","","","",
+	/*H0-H7*/	"","bmc-ingraham0","rear-enc-id0","rear-enc-fault0","","","","",
 	/*I0-I7*/	"","","","","","","","",
 	/*J0-J7*/	"","","","","","","","",
 	/*K0-K7*/	"","","","","","","","",
@@ -175,7 +195,7 @@ 
 	/*M0-M7*/	"","","","","","","","",
 	/*N0-N7*/	"","","","","","","","",
 	/*O0-O7*/	"","","","usb-power","","","","",
-	/*P0-P7*/	"","","","","","","","",
+	/*P0-P7*/	"","","","","pcieslot-power","","","",
 	/*Q0-Q7*/	"cfam-reset","","","","","","","",
 	/*R0-R7*/	"","","","","","","","",
 	/*S0-S7*/	"presence-ps0","presence-ps1","presence-ps2","presence-ps3",