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

[vishwanatha subbanna <vishwa@linux.vnet.ibm.com>]

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