* Re: [PATCH linux dev-5.10 06/35] ARM: dts: aspeed: rainier: Add leds that are off PCA9552 [not found] ` <CACPK8Xf8iY5LOXgJLvBS0okokG-QNQJ3idPvmcF9eMaRQSBGzQ@mail.gmail.com> @ 2021-04-26 5:59 ` vishwanatha subbanna 2021-04-27 21:22 ` Jacek Anaszewski 0 siblings, 1 reply; 3+ messages in thread From: vishwanatha subbanna @ 2021-04-26 5:59 UTC (permalink / raw) To: Joel Stanley, Andrew Jeffery, Jacek Anaszewski, linux-leds Cc: vishwanatha subbanna, Milton Miller II, OpenBMC Maillist, Eddie James 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 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH linux dev-5.10 06/35] ARM: dts: aspeed: rainier: Add leds that are off PCA9552 2021-04-26 5:59 ` [PATCH linux dev-5.10 06/35] ARM: dts: aspeed: rainier: Add leds that are off PCA9552 vishwanatha subbanna @ 2021-04-27 21:22 ` Jacek Anaszewski 2021-04-28 7:30 ` vishwanatha subbanna 0 siblings, 1 reply; 3+ messages in thread From: Jacek Anaszewski @ 2021-04-27 21:22 UTC (permalink / raw) To: vishwanatha subbanna, Joel Stanley, Andrew Jeffery, linux-leds Cc: Milton Miller II, OpenBMC Maillist, Eddie James 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 ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH linux dev-5.10 06/35] ARM: dts: aspeed: rainier: Add leds that are off PCA9552 2021-04-27 21:22 ` Jacek Anaszewski @ 2021-04-28 7:30 ` vishwanatha subbanna 0 siblings, 0 replies; 3+ messages in thread From: vishwanatha subbanna @ 2021-04-28 7:30 UTC (permalink / raw) To: Jacek Anaszewski Cc: vishwanatha subbanna, Joel Stanley, Andrew Jeffery, linux-leds, Eddie James 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 ^ permalink raw reply related [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-04-28 7:30 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20210308225419.46530-1-eajames@linux.ibm.com>
[not found] ` <20210308225419.46530-7-eajames@linux.ibm.com>
[not found] ` <CACPK8Xc9XqM3UtpF0xywFwfj8anXWE1-TvbHCQskogrBBF_ZCQ@mail.gmail.com>
[not found] ` <OF39939D76.45BF746F-ON00258696.0001FF38-00258696.0001FF3E@notes.na.collabserv.com>
[not found] ` <CACPK8XfBu5_2xs_Eu=OtShNFQnAQ+Tc1Q1qM7Qgcaggd-yLumQ@mail.gmail.com>
[not found] ` <6ACEC474-8CFD-4BA9-B8FF-CCD41007AA67@linux.vnet.ibm.com>
[not found] ` <CACPK8Xf8iY5LOXgJLvBS0okokG-QNQJ3idPvmcF9eMaRQSBGzQ@mail.gmail.com>
2021-04-26 5:59 ` [PATCH linux dev-5.10 06/35] ARM: dts: aspeed: rainier: Add leds that are off PCA9552 vishwanatha subbanna
2021-04-27 21:22 ` Jacek Anaszewski
2021-04-28 7:30 ` vishwanatha subbanna
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).