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

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

[vishwanatha subbanna]

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

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

[Jacek Anaszewski]

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.

-- 
Best regards,
Jacek Anaszewski

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

[vishwanatha subbanna]

Hi Jacek,

Thanks for reverting. The setup seems fine because, when we express these as PCA955X_TYPE_GPIO and then define the
LEDs under “compatibility=gpio-leds” section, then the LEDs do retain the states post BMC reboot with Andrew Jeffrey’s patch: https://github.com/torvalds/linux/commit/f5808ac158f2b16b686a3d3c0879c5d6048aba14

Problem is only when we express LEDs as PCA955X_TYPE_LED. With Andrew Jeffrey’s help now, I got some better understanding of load/unload. I then created this hack, but that does not do the trick as well.

--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -442,9 +442,9 @@ void led_classdev_unregister(struct led_classdev *led_cdev)
 	led_cdev->flags |= LED_UNREGISTERING;
 
 	/* Stop blinking */
-	led_stop_software_blink(led_cdev);
+    /* led_stop_software_blink(led_cdev); */
 
-	led_set_brightness(led_cdev, LED_OFF);
+	/*led_set_brightness(led_cdev, LED_OFF); */
 
 	flush_work(&led_cdev->set_brightness_work);
 
diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
index f1f718d..833a16b 100644
--- a/drivers/leds/led-core.c
+++ b/drivers/leds/led-core.c
@@ -121,8 +121,8 @@ static void set_brightness_delayed(struct work_struct *ws)
 	int ret = 0;
 
 	if (test_and_clear_bit(LED_BLINK_DISABLE, &led_cdev->work_flags)) {
-		led_cdev->delayed_set_value = LED_OFF;
-		led_stop_software_blink(led_cdev);
+		/* led_cdev->delayed_set_value = LED_OFF; */
+		/* led_stop_software_blink(led_cdev); */
 	}
 
 	ret = __led_set_brightness(led_cdev, led_cdev->delayed_set_value);
— 

Thank you,
!! Vishwa !!

> On 28-Apr-2021, at 2:52 AM, Jacek Anaszewski <jacek.anaszewski@gmail.com> wrote:
> 
> 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.
> 
> -- 
> Best regards,
> Jacek Anaszewski