[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 at 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 at 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 at jms.id.au> wrote:
> 
> On Fri, 12 Mar 2021 at 07:05, vishwanatha subbanna
> <vishwa at linux.vnet.ibm.com> wrote:
>> 
>> 
>> 
>> On 12-Mar-2021, at 6:00 AM, Joel Stanley <joel at jms.id.au> wrote:
>> 
>> On Fri, 12 Mar 2021 at 00:21, Milton Miller II <miltonm at us.ibm.com> wrote:
>> 
>> 
>> 
>> 
>> -----"openbmc" <openbmc-bounces+miltonm=us.ibm.com at lists.ozlabs.org> wrote: -----
>> 
>> To: Eddie James <eajames at linux.ibm.com>
>> From: Joel Stanley
>> Sent by: "openbmc"
>> Date: 03/11/2021 06:09PM
>> Cc: OpenBMC Maillist <openbmc at 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 at linux.ibm.com>
>> wrote:
>> 
>> 
>> From: Vishwanatha Subbanna <vishwa at 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