diff mbox series

[linux,dev-5.10,06/35] ARM: dts: aspeed: rainier: Add leds that are off PCA9552

Message ID 20210308225419.46530-7-eajames@linux.ibm.com
State New
Headers show
Series Rainier and Everest system updates | expand

Commit Message

Eddie James March 8, 2021, 10:53 p.m. UTC 
From: Vishwanatha Subbanna <vishwa@linux.vnet.ibm.com>

These LEDs are on the fans and are connected via a
pca9551 i2c expander

Signed-off-by: Vishwanatha Subbanna <vishwa@linux.vnet.ibm.com>
---
 arch/arm/boot/dts/aspeed-bmc-ibm-rainier.dts | 41 ++++++++++++++++++++
 1 file changed, 41 insertions(+)

Comments

Joel Stanley March 12, 2021, 12:09 a.m. UTC | #1 
On Mon, 8 Mar 2021 at 22:54, Eddie James <eajames@linux.ibm.com> wrote:
>
> From: Vishwanatha Subbanna <vishwa@linux.vnet.ibm.com>
>
> These LEDs are on the fans and are connected via a
> pca9551 i2c expander

This change doesn't make sense. The pca9551 is an i2c LED expander, so
we don't need to expose the pins as GPIOs and then attach a gpio-leds
driver to them. We should instead simply configure the pca955x driver
to drive the LEDs as LEDs.

>
> Signed-off-by: Vishwanatha Subbanna <vishwa@linux.vnet.ibm.com>
> ---
>  arch/arm/boot/dts/aspeed-bmc-ibm-rainier.dts | 41 ++++++++++++++++++++
>  1 file changed, 41 insertions(+)
>
> diff --git a/arch/arm/boot/dts/aspeed-bmc-ibm-rainier.dts b/arch/arm/boot/dts/aspeed-bmc-ibm-rainier.dts
> index 6684485a2db0..514a14d3f914 100644
> --- a/arch/arm/boot/dts/aspeed-bmc-ibm-rainier.dts
> +++ b/arch/arm/boot/dts/aspeed-bmc-ibm-rainier.dts
> @@ -263,6 +263,47 @@ fan5-presence {
>                         linux,code = <11>;
>                 };
>         };
> +
> +       leds {
> +               compatible = "gpio-leds";
> +
> +               fan0 {
> +                       retain-state-shutdown;
> +                       default-state = "keep";
> +                       gpios = <&pca0 0 GPIO_ACTIVE_LOW>;
> +               };
> +
> +               fan1 {
> +                       retain-state-shutdown;
> +                       default-state = "keep";
> +                       gpios = <&pca0 1 GPIO_ACTIVE_LOW>;
> +               };
> +
> +               fan2 {
> +                       retain-state-shutdown;
> +                       default-state = "keep";
> +                       gpios = <&pca0 2 GPIO_ACTIVE_LOW>;
> +               };
> +
> +               fan3 {
> +                       retain-state-shutdown;
> +                       default-state = "keep";
> +                       gpios = <&pca0 3 GPIO_ACTIVE_LOW>;
> +               };
> +
> +               fan4 {
> +                       retain-state-shutdown;
> +                       default-state = "keep";
> +                       gpios = <&pca0 4 GPIO_ACTIVE_LOW>;
> +               };
> +
> +               fan5 {
> +                       retain-state-shutdown;
> +                       default-state = "keep";
> +                       gpios = <&pca0 5 GPIO_ACTIVE_LOW>;
> +               };
> +       };
> +
>  };
>
>  &ehci1 {
> --
> 2.27.0
>
Milton Miller II March 12, 2021, 12:21 a.m. UTC | #2 
-----"openbmc" <openbmc-bounces+miltonm=us.ibm.com@lists.ozlabs.org> wrote: -----

>To: Eddie James <eajames@linux.ibm.com>
>From: Joel Stanley 
>Sent by: "openbmc" 
>Date: 03/11/2021 06:09PM
>Cc: OpenBMC Maillist <openbmc@lists.ozlabs.org>
>Subject: [EXTERNAL] Re: [PATCH linux dev-5.10 06/35] ARM: dts:
>aspeed: rainier: Add leds that are off PCA9552
>
>On Mon, 8 Mar 2021 at 22:54, Eddie James <eajames@linux.ibm.com>
>wrote:
>>
>> From: Vishwanatha Subbanna <vishwa@linux.vnet.ibm.com>
>>
>> These LEDs are on the fans and are connected via a
>> pca9551 i2c expander
>
>This change doesn't make sense. The pca9551 is an i2c LED expander,
>so
>we don't need to expose the pins as GPIOs and then attach a gpio-leds
>driver to them. We should instead simply configure the pca955x driver
>to drive the LEDs as LEDs.

I'll refresh your memory on why we have been doing this in our 
devie trees and then let you consider if this is desired or not.

The led system insistes on creating a compact map (no holes) (as
does the reset subsystem).

However, this means the relative led number for a pin changes 
as the prior pins change from gpio to led configuration.

For example if pins 2 and 7 are leds, they become leds 0 and 1.  
Changing pin 5 to also be an led means that pin 7 is now led 2 
not led 1 on the led subsystem.

The workaround we have done for existing systems has been to use 
gpio leds for pca family devices.

milton

>
>>
>> Signed-off-by: Vishwanatha Subbanna <vishwa@linux.vnet.ibm.com>
>> ---
>>  arch/arm/boot/dts/aspeed-bmc-ibm-rainier.dts | 41
>++++++++++++++++++++
>>  1 file changed, 41 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/aspeed-bmc-ibm-rainier.dts
>b/arch/arm/boot/dts/aspeed-bmc-ibm-rainier.dts
>> index 6684485a2db0..514a14d3f914 100644
>> --- a/arch/arm/boot/dts/aspeed-bmc-ibm-rainier.dts
>> +++ b/arch/arm/boot/dts/aspeed-bmc-ibm-rainier.dts
>> @@ -263,6 +263,47 @@ fan5-presence {
>>                         linux,code = <11>;
>>                 };
>>         };
>> +
>> +       leds {
>> +               compatible = "gpio-leds";
>> +
>> +               fan0 {
>> +                       retain-state-shutdown;
>> +                       default-state = "keep";
>> +                       gpios = <&pca0 0 GPIO_ACTIVE_LOW>;
>> +               };
>> +
>> +               fan1 {
>> +                       retain-state-shutdown;
>> +                       default-state = "keep";
>> +                       gpios = <&pca0 1 GPIO_ACTIVE_LOW>;
>> +               };
>> +
>> +               fan2 {
>> +                       retain-state-shutdown;
>> +                       default-state = "keep";
>> +                       gpios = <&pca0 2 GPIO_ACTIVE_LOW>;
>> +               };
>> +
>> +               fan3 {
>> +                       retain-state-shutdown;
>> +                       default-state = "keep";
>> +                       gpios = <&pca0 3 GPIO_ACTIVE_LOW>;
>> +               };
>> +
>> +               fan4 {
>> +                       retain-state-shutdown;
>> +                       default-state = "keep";
>> +                       gpios = <&pca0 4 GPIO_ACTIVE_LOW>;
>> +               };
>> +
>> +               fan5 {
>> +                       retain-state-shutdown;
>> +                       default-state = "keep";
>> +                       gpios = <&pca0 5 GPIO_ACTIVE_LOW>;
>> +               };
>> +       };
>> +
>>  };
>>
>>  &ehci1 {
>> --
>> 2.27.0
>>
>
>
Joel Stanley March 12, 2021, 12:30 a.m. UTC | #3 
On Fri, 12 Mar 2021 at 00:21, Milton Miller II <miltonm@us.ibm.com> wrote:
>
>
>
> -----"openbmc" <openbmc-bounces+miltonm=us.ibm.com@lists.ozlabs.org> wrote: -----
>
> >To: Eddie James <eajames@linux.ibm.com>
> >From: Joel Stanley
> >Sent by: "openbmc"
> >Date: 03/11/2021 06:09PM
> >Cc: OpenBMC Maillist <openbmc@lists.ozlabs.org>
> >Subject: [EXTERNAL] Re: [PATCH linux dev-5.10 06/35] ARM: dts:
> >aspeed: rainier: Add leds that are off PCA9552
> >
> >On Mon, 8 Mar 2021 at 22:54, Eddie James <eajames@linux.ibm.com>
> >wrote:
> >>
> >> From: Vishwanatha Subbanna <vishwa@linux.vnet.ibm.com>
> >>
> >> These LEDs are on the fans and are connected via a
> >> pca9551 i2c expander
> >
> >This change doesn't make sense. The pca9551 is an i2c LED expander,
> >so
> >we don't need to expose the pins as GPIOs and then attach a gpio-leds
> >driver to them. We should instead simply configure the pca955x driver
> >to drive the LEDs as LEDs.
>
> I'll refresh your memory on why we have been doing this in our
> devie trees and then let you consider if this is desired or not.
>
> The led system insistes on creating a compact map (no holes) (as
> does the reset subsystem).
>
> However, this means the relative led number for a pin changes
> as the prior pins change from gpio to led configuration.
>
> For example if pins 2 and 7 are leds, they become leds 0 and 1.
> Changing pin 5 to also be an led means that pin 7 is now led 2
> not led 1 on the led subsystem.

Thanks for the rationale reminder.

Are these led numbers important to userspace, or does the renumbering
affect device tree changes only?
Joel Stanley March 24, 2021, 11:43 p.m. UTC | #4 
On Fri, 12 Mar 2021 at 07:05, vishwanatha subbanna
<vishwa@linux.vnet.ibm.com> wrote:
>
>
>
> On 12-Mar-2021, at 6:00 AM, Joel Stanley <joel@jms.id.au> wrote:
>
> On Fri, 12 Mar 2021 at 00:21, Milton Miller II <miltonm@us.ibm.com> wrote:
>
>
>
>
> -----"openbmc" <openbmc-bounces+miltonm=us.ibm.com@lists.ozlabs.org> wrote: -----
>
> To: Eddie James <eajames@linux.ibm.com>
> From: Joel Stanley
> Sent by: "openbmc"
> Date: 03/11/2021 06:09PM
> Cc: OpenBMC Maillist <openbmc@lists.ozlabs.org>
> Subject: [EXTERNAL] Re: [PATCH linux dev-5.10 06/35] ARM: dts:
> aspeed: rainier: Add leds that are off PCA9552
>
> On Mon, 8 Mar 2021 at 22:54, Eddie James <eajames@linux.ibm.com>
> wrote:
>
>
> From: Vishwanatha Subbanna <vishwa@linux.vnet.ibm.com>
>
> These LEDs are on the fans and are connected via a
> pca9551 i2c expander
>
>
> This change doesn't make sense. The pca9551 is an i2c LED expander,
> so
> we don't need to expose the pins as GPIOs and then attach a gpio-leds
> driver to them. We should instead simply configure the pca955x driver
> to drive the LEDs as LEDs.
>
>
> I'll refresh your memory on why we have been doing this in our
> devie trees and then let you consider if this is desired or not.
>
> The led system insistes on creating a compact map (no holes) (as
> does the reset subsystem).
>
> However, this means the relative led number for a pin changes
> as the prior pins change from gpio to led configuration.
>
> For example if pins 2 and 7 are leds, they become leds 0 and 1.
> Changing pin 5 to also be an led means that pin 7 is now led 2
> not led 1 on the led subsystem.
>
>
> Thanks for the rationale reminder.
>
> Are these led numbers important to userspace, or does the renumbering
> affect device tree changes only?
>
>
>
> Here are my technical needs.
> - I need these LEDs associated with names and this __must not__ change
> - I need those LEDs represented as `/sys/class/leds/<$name>`
>
> What can I do :
> - use `leds-gpio` like how it’s done today
>
> OR
>
> - Use “label” in PCA955X_TYPE_LED
>    - However, putting this label, it results in `/sys/class/leds/pca955x:<$label>`. As opposed to `/sys/class/leds/<$label>`.
>
> Is there a way where I can get `/sys/class/leds/<$label>` ?. I did not get this from the documentation. Seeing pca955x on 100 entries seems a noise

The prefix has been present in the driver since it was introduced in
2008. Is there any reason we can't have userspace ignore the pca955x
prefix?

Cheers,

Joel
vishwanatha subbanna April 26, 2021, 5:59 a.m. UTC | #5 
Joel,

With the experiments that I have done, I can not express LEDs with PCA955X_TYPE_LED predominantly because LEDs won’t
retain states after the BMC reboot. I cooked a patch and tried but it does not work. I did an experiment where
I put the patch and then did a reboot and saw that the LEDs were [OFF] in the very early stage of probe itself.

From a9fe9e956c624c15a455b88cc05262358519a541 Mon Sep 17 00:00:00 2001
From: Vishwanatha Subbanna <vishwa@linux.vnet.ibm.com>
Date: Fri, 23 Apr 2021 06:57:56 -0500
Subject: [PATCH 1/2] leds: pca955x: Add support for default-state

Signed-off-by: Vishwanatha Subbanna <vishwa@linux.vnet.ibm.com>
---
 drivers/leds/leds-pca955x.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/leds/leds-pca955x.c b/drivers/leds/leds-pca955x.c
index bf7ead4..987415b 100644
--- a/drivers/leds/leds-pca955x.c
+++ b/drivers/leds/leds-pca955x.c
@@ -130,6 +130,7 @@ struct pca955x_led {
 	char			name[32];
 	u32			type;
 	const char		*default_trigger;
+	const char		*default_state;
 };
 
 struct pca955x_platform_data {
@@ -408,6 +409,8 @@ static int pca955x_gpio_direction_output(struct gpio_chip *gc,
 		fwnode_property_read_u32(child, "type", &pdata->leds[reg].type);
 		fwnode_property_read_string(child, "linux,default-trigger",
 					&pdata->leds[reg].default_trigger);
+		fwnode_property_read_string(child, "default-state",
+					&pdata->leds[reg].default_state);
 	}
 
 	pdata->num_leds = chip->bits;
@@ -520,8 +523,13 @@ static int pca955x_probe(struct i2c_client *client,
 			if (err)
 				return err;
 
-			/* Turn off LED */
-			err = pca955x_led_set(&pca955x_led->led_cdev, LED_OFF);
+			/* If the default-state is "keep", don't change states */
+			if (strcmp(pdata->leds[i].default_state, "keep")) {
+				if (!strcmp(pdata->leds[i].default_state, "on"))
+					err = pca955x_led_set(&pca955x_led->led_cdev, LED_ON);
+				else
+					err = pca955x_led_set(&pca955x_led->led_cdev, LED_OFF);
+			}
 			if (err)
 				return err;
 		}
— 
1.8.3.1


For `leds-gpio`, Andrew had put a patch, but I don’t see how that can be mapped to PCA955X. https://github.com/torvalds/linux/commit/f5808ac158f2b16b686a3d3c0879c5d6048aba14

Jacek, 

Please could you help me here ?.. I need to express LEDs as PCA955X_TYPE_LED and also retain states post BMC reboot.

Thank you,
!! Vishwa !!

> On 25-Mar-2021, at 5:13 AM, Joel Stanley <joel@jms.id.au> wrote:
> 
> On Fri, 12 Mar 2021 at 07:05, vishwanatha subbanna
> <vishwa@linux.vnet.ibm.com> wrote:
>> 
>> 
>> 
>> On 12-Mar-2021, at 6:00 AM, Joel Stanley <joel@jms.id.au> wrote:
>> 
>> On Fri, 12 Mar 2021 at 00:21, Milton Miller II <miltonm@us.ibm.com> wrote:
>> 
>> 
>> 
>> 
>> -----"openbmc" <openbmc-bounces+miltonm=us.ibm.com@lists.ozlabs.org> wrote: -----
>> 
>> To: Eddie James <eajames@linux.ibm.com>
>> From: Joel Stanley
>> Sent by: "openbmc"
>> Date: 03/11/2021 06:09PM
>> Cc: OpenBMC Maillist <openbmc@lists.ozlabs.org>
>> Subject: [EXTERNAL] Re: [PATCH linux dev-5.10 06/35] ARM: dts:
>> aspeed: rainier: Add leds that are off PCA9552
>> 
>> On Mon, 8 Mar 2021 at 22:54, Eddie James <eajames@linux.ibm.com>
>> wrote:
>> 
>> 
>> From: Vishwanatha Subbanna <vishwa@linux.vnet.ibm.com>
>> 
>> These LEDs are on the fans and are connected via a
>> pca9551 i2c expander
>> 
>> 
>> This change doesn't make sense. The pca9551 is an i2c LED expander,
>> so
>> we don't need to expose the pins as GPIOs and then attach a gpio-leds
>> driver to them. We should instead simply configure the pca955x driver
>> to drive the LEDs as LEDs.
>> 
>> 
>> I'll refresh your memory on why we have been doing this in our
>> devie trees and then let you consider if this is desired or not.
>> 
>> The led system insistes on creating a compact map (no holes) (as
>> does the reset subsystem).
>> 
>> However, this means the relative led number for a pin changes
>> as the prior pins change from gpio to led configuration.
>> 
>> For example if pins 2 and 7 are leds, they become leds 0 and 1.
>> Changing pin 5 to also be an led means that pin 7 is now led 2
>> not led 1 on the led subsystem.
>> 
>> 
>> Thanks for the rationale reminder.
>> 
>> Are these led numbers important to userspace, or does the renumbering
>> affect device tree changes only?
>> 
>> 
>> 
>> Here are my technical needs.
>> - I need these LEDs associated with names and this __must not__ change
>> - I need those LEDs represented as `/sys/class/leds/<$name>`
>> 
>> What can I do :
>> - use `leds-gpio` like how it’s done today
>> 
>> OR
>> 
>> - Use “label” in PCA955X_TYPE_LED
>>   - However, putting this label, it results in `/sys/class/leds/pca955x:<$label>`. As opposed to `/sys/class/leds/<$label>`.
>> 
>> Is there a way where I can get `/sys/class/leds/<$label>` ?. I did not get this from the documentation. Seeing pca955x on 100 entries seems a noise
> 
> The prefix has been present in the driver since it was introduced in
> 2008. Is there any reason we can't have userspace ignore the pca955x
> prefix?
> 
> Cheers,
> 
> Joel
Jacek Anaszewski April 27, 2021, 9:22 p.m. UTC | #6 
Hi Vishwanatha,

On 4/26/21 7:59 AM, vishwanatha subbanna wrote:
> Joel,
> 
> With the experiments that I have done, I can not express LEDs with PCA955X_TYPE_LED predominantly because LEDs won’t
> retain states after the BMC reboot. I cooked a patch and tried but it does not work. I did an experiment where
> I put the patch and then did a reboot and saw that the LEDs were [OFF] in the very early stage of probe itself.
> 
>>From a9fe9e956c624c15a455b88cc05262358519a541 Mon Sep 17 00:00:00 2001
> From: Vishwanatha Subbanna <vishwa@linux.vnet.ibm.com>
> Date: Fri, 23 Apr 2021 06:57:56 -0500
> Subject: [PATCH 1/2] leds: pca955x: Add support for default-state
> 
> Signed-off-by: Vishwanatha Subbanna <vishwa@linux.vnet.ibm.com>
> ---
>   drivers/leds/leds-pca955x.c | 12 ++++++++++--
>   1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/leds/leds-pca955x.c b/drivers/leds/leds-pca955x.c
> index bf7ead4..987415b 100644
> --- a/drivers/leds/leds-pca955x.c
> +++ b/drivers/leds/leds-pca955x.c
> @@ -130,6 +130,7 @@ struct pca955x_led {
>   	char			name[32];
>   	u32			type;
>   	const char		*default_trigger;
> +	const char		*default_state;
>   };
>   
>   struct pca955x_platform_data {
> @@ -408,6 +409,8 @@ static int pca955x_gpio_direction_output(struct gpio_chip *gc,
>   		fwnode_property_read_u32(child, "type", &pdata->leds[reg].type);
>   		fwnode_property_read_string(child, "linux,default-trigger",
>   					&pdata->leds[reg].default_trigger);
> +		fwnode_property_read_string(child, "default-state",
> +					&pdata->leds[reg].default_state);
>   	}
>   
>   	pdata->num_leds = chip->bits;
> @@ -520,8 +523,13 @@ static int pca955x_probe(struct i2c_client *client,
>   			if (err)
>   				return err;
>   
> -			/* Turn off LED */
> -			err = pca955x_led_set(&pca955x_led->led_cdev, LED_OFF);
> +			/* If the default-state is "keep", don't change states */
> +			if (strcmp(pdata->leds[i].default_state, "keep")) {
> +				if (!strcmp(pdata->leds[i].default_state, "on"))
> +					err = pca955x_led_set(&pca955x_led->led_cdev, LED_ON);
> +				else
> +					err = pca955x_led_set(&pca955x_led->led_cdev, LED_OFF);
> +			}
>   			if (err)
>   				return err;
>   		}
> —
> 1.8.3.1
> 
> 
> For `leds-gpio`, Andrew had put a patch, but I don’t see how that can be mapped to PCA955X. https://github.com/torvalds/linux/commit/f5808ac158f2b16b686a3d3c0879c5d6048aba14
> 
> Jacek,
> 
> Please could you help me here ?.. I need to express LEDs as PCA955X_TYPE_LED and also retain states post BMC reboot.

If in your setup the LED controller loses power on reboot then there
is nothing you can do to retain the state.
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 6684485a2db0..514a14d3f914 100644
--- a/arch/arm/boot/dts/aspeed-bmc-ibm-rainier.dts
+++ b/arch/arm/boot/dts/aspeed-bmc-ibm-rainier.dts
@@ -263,6 +263,47 @@  fan5-presence {
 			linux,code = <11>;
 		};
 	};
+
+	leds {
+		compatible = "gpio-leds";
+
+		fan0 {
+			retain-state-shutdown;
+			default-state = "keep";
+			gpios = <&pca0 0 GPIO_ACTIVE_LOW>;
+		};
+
+		fan1 {
+			retain-state-shutdown;
+			default-state = "keep";
+			gpios = <&pca0 1 GPIO_ACTIVE_LOW>;
+		};
+
+		fan2 {
+			retain-state-shutdown;
+			default-state = "keep";
+			gpios = <&pca0 2 GPIO_ACTIVE_LOW>;
+		};
+
+		fan3 {
+			retain-state-shutdown;
+			default-state = "keep";
+			gpios = <&pca0 3 GPIO_ACTIVE_LOW>;
+		};
+
+		fan4 {
+			retain-state-shutdown;
+			default-state = "keep";
+			gpios = <&pca0 4 GPIO_ACTIVE_LOW>;
+		};
+
+		fan5 {
+			retain-state-shutdown;
+			default-state = "keep";
+			gpios = <&pca0 5 GPIO_ACTIVE_LOW>;
+		};
+	};
+
 };
 
 &ehci1 {