[PATCH linux dev-4.10 0/5] Retain front LED state on Witherspoon

[PATCH linux dev-4.10 0/5] Retain front LED state on Witherspoon

[Andrew Jeffery]

Hello,

This series fixes witherspoon to retain its front LED state across BMC resets.
It was noted in issue 2156[1] that the front LEDs came on solid during a BMC
boot. This appears to be the result of several bugs which are addressed in the
following patches.

The changes to leds-gpio have been sent upstream:

  https://lkml.org/lkml/2017/8/25/44

The pca955x patch depends on changes we appear to only have in the dev-4.10
tree (i.e. they haven't been applied upstream). There's some discussion at [2]
but I'm not aware of a follow-up patch from Cédric. Certainly applying my patch
to upstream lead to signifiant conflicts, so I haven't yet tried to send it on
its way.

Cédric: I know you're undecided on how I've approached the pca955x patch - I
figured we could argue about it on the list in case anyone else had helpful
insights to settle the dispute :)

The final two patches clean up the leds nodes in the Witherspoon devicetree and
add the new retain-state-shutdown property to the necessary LEDs. I do wonder
if we want to retain the state on the rear LEDs as well, but that requires more
features to be added to the Aspeed GPIO controller.

The patches have been tested on a Witherspoon system, albeit remotely. I can't
put eyes on the LEDs in question so it would be good for someone who has the
ability to do so. I used the test outlined by Vishwa at [3].

Please review!

Andrew

[1] https://github.com/openbmc/openbmc/issues/2156
[2] http://patchwork.ozlabs.org/patch/760006/
[3] https://github.com/openbmc/openbmc/issues/2156#issuecomment-324101828

Andrew Jeffery (5):
  dt-bindings: leds: gpio: Add optional retain-state-shutdown property
  leds: gpio: Allow LED to retain state at shutdown
  leds: pca955x: Don't invert requested value in
    pca955x_gpio_set_value()
  aspeed: witherspoon: Tidy and unify LED nodes
  aspeed: witherspoon: leds: Retain state across BMC resets

 .../devicetree/bindings/leds/leds-gpio.txt         |  3 ++
 arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts   | 32 ++++++++++++----------
 drivers/leds/leds-gpio.c                           |  7 ++++-
 drivers/leds/leds-pca955x.c                        |  7 +++--
 include/linux/leds.h                               |  3 ++
 5 files changed, 34 insertions(+), 18 deletions(-)

-- 
2.11.0

[PATCH linux dev-4.10 1/5] dt-bindings: leds: gpio: Add optional retain-state-shutdown property

[Andrew Jeffery]

On BMC systems it's sometimes necessary for a LED to retain its state
across a BMC reset (which is independent of the host system state). Add
a devicetree property to describe this behaviour. The property would
typically be used in conjunction with 'default-state = "keep"'.

Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
 Documentation/devicetree/bindings/leds/leds-gpio.txt | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/leds/leds-gpio.txt b/Documentation/devicetree/bindings/leds/leds-gpio.txt
index 76535ca37120..a48dda268f81 100644
--- a/Documentation/devicetree/bindings/leds/leds-gpio.txt
+++ b/Documentation/devicetree/bindings/leds/leds-gpio.txt
@@ -18,6 +18,9 @@ LED sub-node properties:
   see Documentation/devicetree/bindings/leds/common.txt
 - retain-state-suspended: (optional) The suspend state can be retained.Such
   as charge-led gpio.
+- retain-state-shutdown: (optional) Retain the state of the LED on shutdown.
+  Useful in BMC systems, for example when the BMC is rebooted while the host
+  remains up.
 - panic-indicator : (optional)
   see Documentation/devicetree/bindings/leds/common.txt
 
-- 
2.11.0

[PATCH linux dev-4.10 2/5] leds: gpio: Allow LED to retain state at shutdown

[Andrew Jeffery]

In some systems, such as BMCs, we want to retain the state of LEDs
across a reboot of the BMC whilst the host remains up.

Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
 drivers/leds/leds-gpio.c | 7 ++++++-
 include/linux/leds.h     | 3 +++
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/leds/leds-gpio.c b/drivers/leds/leds-gpio.c
index d400dcaf4d29..061ab9220048 100644
--- a/drivers/leds/leds-gpio.c
+++ b/drivers/leds/leds-gpio.c
@@ -134,6 +134,8 @@ static int create_gpio_led(const struct gpio_led *template,
 		led_dat->cdev.flags |= LED_CORE_SUSPENDRESUME;
 	if (template->panic_indicator)
 		led_dat->cdev.flags |= LED_PANIC_INDICATOR;
+	if (template->retain_state_shutdown)
+		led_dat->cdev.flags |= LED_RETAIN_AT_SHUTDOWN;
 
 	ret = gpiod_direction_output(led_dat->gpiod, state);
 	if (ret < 0)
@@ -203,6 +205,8 @@ static struct gpio_leds_priv *gpio_leds_create(struct platform_device *pdev)
 
 		if (fwnode_property_present(child, "retain-state-suspended"))
 			led.retain_state_suspended = 1;
+		if (fwnode_property_present(child, "retain-state-shutdown"))
+			led.retain_state_shutdown = 1;
 		if (fwnode_property_present(child, "panic-indicator"))
 			led.panic_indicator = 1;
 
@@ -265,7 +269,8 @@ static void gpio_led_shutdown(struct platform_device *pdev)
 	for (i = 0; i < priv->num_leds; i++) {
 		struct gpio_led_data *led = &priv->leds[i];
 
-		gpio_led_set(&led->cdev, LED_OFF);
+		if (!(led->cdev.flags & LED_RETAIN_AT_SHUTDOWN))
+			gpio_led_set(&led->cdev, LED_OFF);
 	}
 }
 
diff --git a/include/linux/leds.h b/include/linux/leds.h
index 569cb531094c..4d39f767f34a 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -46,6 +46,8 @@ struct led_classdev {
 #define LED_DEV_CAP_FLASH	(1 << 18)
 #define LED_HW_PLUGGABLE	(1 << 19)
 #define LED_PANIC_INDICATOR	(1 << 20)
+#define LED_BRIGHT_HW_CHANGED	(1 << 21)
+#define LED_RETAIN_AT_SHUTDOWN	(1 << 22)
 
 	/* set_brightness_work / blink_timer flags, atomic, private. */
 	unsigned long		work_flags;
@@ -378,6 +380,7 @@ struct gpio_led {
 	unsigned	retain_state_suspended : 1;
 	unsigned	panic_indicator : 1;
 	unsigned	default_state : 2;
+	unsigned	retain_state_shutdown : 1;
 	/* default_state should be one of LEDS_GPIO_DEFSTATE_(ON|OFF|KEEP) */
 	struct gpio_desc *gpiod;
 };
-- 
2.11.0

Re: [PATCH linux dev-4.10 2/5] leds: gpio: Allow LED to retain state at shutdown

[Cédric Le Goater]

On 08/25/2017 08:52 AM, Andrew Jeffery wrote:
> In some systems, such as BMCs, we want to retain the state of LEDs
> across a reboot of the BMC whilst the host remains up.
> 
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> ---
>  drivers/leds/leds-gpio.c | 7 ++++++-
>  include/linux/leds.h     | 3 +++
>  2 files changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/leds/leds-gpio.c b/drivers/leds/leds-gpio.c
> index d400dcaf4d29..061ab9220048 100644
> --- a/drivers/leds/leds-gpio.c
> +++ b/drivers/leds/leds-gpio.c
> @@ -134,6 +134,8 @@ static int create_gpio_led(const struct gpio_led *template,
>  		led_dat->cdev.flags |= LED_CORE_SUSPENDRESUME;
>  	if (template->panic_indicator)
>  		led_dat->cdev.flags |= LED_PANIC_INDICATOR;
> +	if (template->retain_state_shutdown)
> +		led_dat->cdev.flags |= LED_RETAIN_AT_SHUTDOWN;
>  
>  	ret = gpiod_direction_output(led_dat->gpiod, state);
>  	if (ret < 0)
> @@ -203,6 +205,8 @@ static struct gpio_leds_priv *gpio_leds_create(struct platform_device *pdev)
>  
>  		if (fwnode_property_present(child, "retain-state-suspended"))
>  			led.retain_state_suspended = 1;
> +		if (fwnode_property_present(child, "retain-state-shutdown"))
> +			led.retain_state_shutdown = 1;
>  		if (fwnode_property_present(child, "panic-indicator"))
>  			led.panic_indicator = 1;
>  
> @@ -265,7 +269,8 @@ static void gpio_led_shutdown(struct platform_device *pdev)
>  	for (i = 0; i < priv->num_leds; i++) {
>  		struct gpio_led_data *led = &priv->leds[i];
>  
> -		gpio_led_set(&led->cdev, LED_OFF);
> +		if (!(led->cdev.flags & LED_RETAIN_AT_SHUTDOWN))
> +			gpio_led_set(&led->cdev, LED_OFF);
>  	}
>  }
>  
> diff --git a/include/linux/leds.h b/include/linux/leds.h
> index 569cb531094c..4d39f767f34a 100644
> --- a/include/linux/leds.h
> +++ b/include/linux/leds.h
> @@ -46,6 +46,8 @@ struct led_classdev {
>  #define LED_DEV_CAP_FLASH	(1 << 18)
>  #define LED_HW_PLUGGABLE	(1 << 19)
>  #define LED_PANIC_INDICATOR	(1 << 20)
> +#define LED_BRIGHT_HW_CHANGED	(1 << 21)

where does this come from ? is it used ?

Thanks,

C.

> +#define LED_RETAIN_AT_SHUTDOWN	(1 << 22)
>  
>  	/* set_brightness_work / blink_timer flags, atomic, private. */
>  	unsigned long		work_flags;
> @@ -378,6 +380,7 @@ struct gpio_led {
>  	unsigned	retain_state_suspended : 1;
>  	unsigned	panic_indicator : 1;
>  	unsigned	default_state : 2;
> +	unsigned	retain_state_shutdown : 1;
>  	/* default_state should be one of LEDS_GPIO_DEFSTATE_(ON|OFF|KEEP) */
>  	struct gpio_desc *gpiod;
>  };
> 

Re: [PATCH linux dev-4.10 2/5] leds: gpio: Allow LED to retain state at shutdown

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 3216 bytes --]

On Fri, 2017-08-25 at 09:40 +0200, Cédric Le Goater wrote:
> On 08/25/2017 08:52 AM, Andrew Jeffery wrote:
> > In some systems, such as BMCs, we want to retain the state of LEDs
> > across a reboot of the BMC whilst the host remains up.
> > 
> > > > Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> > ---
> >  drivers/leds/leds-gpio.c | 7 ++++++-
> >  include/linux/leds.h     | 3 +++
> >  2 files changed, 9 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/leds/leds-gpio.c b/drivers/leds/leds-gpio.c
> > index d400dcaf4d29..061ab9220048 100644
> > --- a/drivers/leds/leds-gpio.c
> > +++ b/drivers/leds/leds-gpio.c
> > @@ -134,6 +134,8 @@ static int create_gpio_led(const struct gpio_led *template,
> > > >  		led_dat->cdev.flags |= LED_CORE_SUSPENDRESUME;
> > > >  	if (template->panic_indicator)
> > > >  		led_dat->cdev.flags |= LED_PANIC_INDICATOR;
> > > > +	if (template->retain_state_shutdown)
> > > > +		led_dat->cdev.flags |= LED_RETAIN_AT_SHUTDOWN;
> >  
> > > >  	ret = gpiod_direction_output(led_dat->gpiod, state);
> > > >  	if (ret < 0)
> > @@ -203,6 +205,8 @@ static struct gpio_leds_priv *gpio_leds_create(struct platform_device *pdev)
> >  
> > > >  		if (fwnode_property_present(child, "retain-state-suspended"))
> > > >  			led.retain_state_suspended = 1;
> > > > +		if (fwnode_property_present(child, "retain-state-shutdown"))
> > > > +			led.retain_state_shutdown = 1;
> > > >  		if (fwnode_property_present(child, "panic-indicator"))
> > > >  			led.panic_indicator = 1;
> >  
> > @@ -265,7 +269,8 @@ static void gpio_led_shutdown(struct platform_device *pdev)
> > > >  	for (i = 0; i < priv->num_leds; i++) {
> > > >  		struct gpio_led_data *led = &priv->leds[i];
> >  
> > > > -		gpio_led_set(&led->cdev, LED_OFF);
> > > > +		if (!(led->cdev.flags & LED_RETAIN_AT_SHUTDOWN))
> > > > +			gpio_led_set(&led->cdev, LED_OFF);
> > > >  	}
> >  }
> >  
> > diff --git a/include/linux/leds.h b/include/linux/leds.h
> > index 569cb531094c..4d39f767f34a 100644
> > --- a/include/linux/leds.h
> > +++ b/include/linux/leds.h
> > @@ -46,6 +46,8 @@ struct led_classdev {
> > > >  #define LED_DEV_CAP_FLASH	(1 << 18)
> > > >  #define LED_HW_PLUGGABLE	(1 << 19)
> > > >  #define LED_PANIC_INDICATOR	(1 << 20)
> > +#define LED_BRIGHT_HW_CHANGED	(1 << 21)
> 
> where does this come from ? is it used ?

Ah, forgot to comment about that. It comes from upstream. I backported
the changes from on top of v4.13-rc6.

I kept it from the conflict to justify using (1 << 22) below, to remain
compatible with upstream.

Andrew

> 
> Thanks,
> 
> C.
> 
> > > > +#define LED_RETAIN_AT_SHUTDOWN	(1 << 22)
> >  
> > > >  	/* set_brightness_work / blink_timer flags, atomic, private. */
> > > > > >  	unsigned long		work_flags;
> > @@ -378,6 +380,7 @@ struct gpio_led {
> > > > > >  	unsigned	retain_state_suspended : 1;
> > > > > >  	unsigned	panic_indicator : 1;
> > > > > >  	unsigned	default_state : 2;
> > > > > > +	unsigned	retain_state_shutdown : 1;
> > > >  	/* default_state should be one of LEDS_GPIO_DEFSTATE_(ON|OFF|KEEP) */
> > > >  	struct gpio_desc *gpiod;
> >  };
> > 
> 
> 

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

[PATCH linux dev-4.10 3/5] leds: pca955x: Don't invert requested value in pca955x_gpio_set_value()

[Andrew Jeffery]

The PCA9552 lines can be used either for driving LEDs or as GPIOs. The
manual states that for LEDs, the operation is open-drain:

         The LSn LED select registers determine the source of the LED data.

           00 = output is set LOW (LED on)
           01 = output is set high-impedance (LED off; default)
           10 = output blinks at PWM0 rate
           11 = output blinks at PWM1 rate

For GPIOs it suggests a pull-up so that the open-case drives the line
high:

         For use as output, connect external pull-up resistor to the pin
         and size it according to the DC recommended operating
         characteristics.  LED output pin is HIGH when the output is
         programmed as high-impedance, and LOW when the output is
         programmed LOW through the ‘LED selector’ register.  The output
         can be pulse-width controlled when PWM0 or PWM1 are used.

Now, I have a hardware design that uses the LED controller to control
LEDs. However, for $reasons, we're using the leds-gpio driver to drive
the LEDs which means wending our way through the gpiochip abstractions.
So with that in mind we need to describe an active-low GPIO
configuration to drive the LEDs as though they were GPIOs.

The set() gpiochip callback in leds-pca955x does the following:

         ...
         if (val)
                pca955x_led_set(&led->led_cdev, LED_FULL);
         else
                pca955x_led_set(&led->led_cdev, LED_OFF);
         ...

Where LED_FULL = 255. pca955x_led_set() in turn does:

         ...
         switch (value) {
         case LED_FULL:
                ls = pca955x_ledsel(ls, ls_led, PCA955X_LS_LED_ON);
                break;
         ...

Where PCA955X_LS_LED_ON is defined as:

         #define PCA955X_LS_LED_ON	0x0	/* Output LOW */

So here we have some type confusion: We've crossed domains from GPIO
behaviour to LED behaviour without accounting for possible inversions
in the process.

Stepping back to leds-gpio for a moment, during probe() we call
create_gpio_led(), which eventually executes:

         if (template->default_state == LEDS_GPIO_DEFSTATE_KEEP) {
                state = gpiod_get_value_cansleep(led_dat->gpiod);
                if (state < 0)
                        return state;
         } else {
                state = (template->default_state == LEDS_GPIO_DEFSTATE_ON);
         }
         ...
         ret = gpiod_direction_output(led_dat->gpiod, state);

In the devicetree the GPIO is annotated as active-low, and
gpiod_get_value_cansleep() handles this for us:

         int gpiod_get_value_cansleep(const struct gpio_desc *desc)
         {
                 int value;

                 might_sleep_if(extra_checks);
                 VALIDATE_DESC(desc);
                 value = _gpiod_get_raw_value(desc);
                 if (value < 0)
                         return value;

                 if (test_bit(FLAG_ACTIVE_LOW, &desc->flags))
                         value = !value;

                 return value;
         }

_gpiod_get_raw_value() in turn calls through the get() callback for the
gpiochip implementation, so returning to our get() implementation in
leds-pca955x we find we extract the raw value from hardware:

         static int pca955x_gpio_get_value(struct gpio_chip *gc, unsigned int offset)
         {
                 struct pca955x *pca955x = gpiochip_get_data(gc);
                 struct pca955x_led *led = &pca955x->leds[offset];
                 u8 reg = pca955x_read_input(pca955x->client, led->led_num / 8);

                 return !!(reg & (1 << (led->led_num % 8)));
         }

This behaviour is not symmetric with that of set(), where the val is
inverted by the driver.

Closing the loop on the GPIO_ACTIVE_LOW inversions,
gpiod_direction_output(), like gpiod_get_value_cansleep, handles it for
us:

         int gpiod_direction_output(struct gpio_desc *desc, int value)
         {
                  VALIDATE_DESC(desc);
                  if (test_bit(FLAG_ACTIVE_LOW, &desc->flags))
                           value = !value;
                  else
                           value = !!value;
                  return _gpiod_direction_output_raw(desc, value);
         }

All-in-all, with a value of 'keep' for default-state property in the
gpio-leds child node, the current state of the hardware will in-fact be
inverted; precisely the opposite of what was intended.

Rework leds-pca955x so that we avoid the incorrect inversion and clarify
the semantics with respect to GPIO.

Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
 drivers/leds/leds-pca955x.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/leds/leds-pca955x.c b/drivers/leds/leds-pca955x.c
index 0217bac2f47b..a02c1fd5dee9 100644
--- a/drivers/leds/leds-pca955x.c
+++ b/drivers/leds/leds-pca955x.c
@@ -61,6 +61,9 @@
 #define PCA955X_LS_BLINK0	0x2	/* Blink at PWM0 rate */
 #define PCA955X_LS_BLINK1	0x3	/* Blink at PWM1 rate */
 
+#define PCA955X_GPIO_HIGH	LED_OFF
+#define PCA955X_GPIO_LOW	LED_FULL
+
 enum pca955x_type {
 	pca9550,
 	pca9551,
@@ -292,9 +295,9 @@ static void pca955x_gpio_set_value(struct gpio_chip *gc, unsigned int offset,
 	struct pca955x_led *led = &pca955x->leds[offset];
 
 	if (val)
-		pca955x_led_set(&led->led_cdev, LED_FULL);
+		pca955x_led_set(&led->led_cdev, PCA955X_GPIO_HIGH);
 	else
-		pca955x_led_set(&led->led_cdev, LED_OFF);
+		pca955x_led_set(&led->led_cdev, PCA955X_GPIO_LOW);
 }
 
 static int pca955x_gpio_get_value(struct gpio_chip *gc, unsigned int offset)
-- 
2.11.0

Re: [PATCH linux dev-4.10 3/5] leds: pca955x: Don't invert requested value in pca955x_gpio_set_value()

[Cédric Le Goater]

On 08/25/2017 08:52 AM, Andrew Jeffery wrote:
> The PCA9552 lines can be used either for driving LEDs or as GPIOs. The
> manual states that for LEDs, the operation is open-drain:
> 
>          The LSn LED select registers determine the source of the LED data.
> 
>            00 = output is set LOW (LED on)
>            01 = output is set high-impedance (LED off; default)
>            10 = output blinks at PWM0 rate
>            11 = output blinks at PWM1 rate
> 
> For GPIOs it suggests a pull-up so that the open-case drives the line
> high:
> 
>          For use as output, connect external pull-up resistor to the pin
>          and size it according to the DC recommended operating
>          characteristics.  LED output pin is HIGH when the output is
>          programmed as high-impedance, and LOW when the output is
>          programmed LOW through the ‘LED selector’ register.  The output
>          can be pulse-width controlled when PWM0 or PWM1 are used.
> 
> Now, I have a hardware design that uses the LED controller to control
> LEDs. However, for $reasons, we're using the leds-gpio driver to drive
> the LEDs which means wending our way through the gpiochip abstractions.
> So with that in mind we need to describe an active-low GPIO
> configuration to drive the LEDs as though they were GPIOs.
> 
> The set() gpiochip callback in leds-pca955x does the following:
> 
>          ...
>          if (val)
>                 pca955x_led_set(&led->led_cdev, LED_FULL);
>          else
>                 pca955x_led_set(&led->led_cdev, LED_OFF);
>          ...
> 
> Where LED_FULL = 255. pca955x_led_set() in turn does:
> 
>          ...
>          switch (value) {
>          case LED_FULL:
>                 ls = pca955x_ledsel(ls, ls_led, PCA955X_LS_LED_ON);
>                 break;
>          ...
> 
> Where PCA955X_LS_LED_ON is defined as:
> 
>          #define PCA955X_LS_LED_ON	0x0	/* Output LOW */
> 
> So here we have some type confusion: We've crossed domains from GPIO
> behaviour to LED behaviour without accounting for possible inversions
> in the process.
> 
> Stepping back to leds-gpio for a moment, during probe() we call
> create_gpio_led(), which eventually executes:
> 
>          if (template->default_state == LEDS_GPIO_DEFSTATE_KEEP) {
>                 state = gpiod_get_value_cansleep(led_dat->gpiod);
>                 if (state < 0)
>                         return state;
>          } else {
>                 state = (template->default_state == LEDS_GPIO_DEFSTATE_ON);
>          }
>          ...
>          ret = gpiod_direction_output(led_dat->gpiod, state);
> 
> In the devicetree the GPIO is annotated as active-low, and
> gpiod_get_value_cansleep() handles this for us:
> 
>          int gpiod_get_value_cansleep(const struct gpio_desc *desc)
>          {
>                  int value;
> 
>                  might_sleep_if(extra_checks);
>                  VALIDATE_DESC(desc);
>                  value = _gpiod_get_raw_value(desc);
>                  if (value < 0)
>                          return value;
> 
>                  if (test_bit(FLAG_ACTIVE_LOW, &desc->flags))
>                          value = !value;
> 
>                  return value;
>          }
> 
> _gpiod_get_raw_value() in turn calls through the get() callback for the
> gpiochip implementation, so returning to our get() implementation in
> leds-pca955x we find we extract the raw value from hardware:
> 
>          static int pca955x_gpio_get_value(struct gpio_chip *gc, unsigned int offset)
>          {
>                  struct pca955x *pca955x = gpiochip_get_data(gc);
>                  struct pca955x_led *led = &pca955x->leds[offset];
>                  u8 reg = pca955x_read_input(pca955x->client, led->led_num / 8);
> 
>                  return !!(reg & (1 << (led->led_num % 8)));
>          }
> 
> This behaviour is not symmetric with that of set(), where the val is
> inverted by the driver.
> 
> Closing the loop on the GPIO_ACTIVE_LOW inversions,
> gpiod_direction_output(), like gpiod_get_value_cansleep, handles it for
> us:
> 
>          int gpiod_direction_output(struct gpio_desc *desc, int value)
>          {
>                   VALIDATE_DESC(desc);
>                   if (test_bit(FLAG_ACTIVE_LOW, &desc->flags))
>                            value = !value;
>                   else
>                            value = !!value;
>                   return _gpiod_direction_output_raw(desc, value);
>          }
> 
> All-in-all, with a value of 'keep' for default-state property in the
> gpio-leds child node, the current state of the hardware will in-fact be
> inverted; precisely the opposite of what was intended.
> 
> Rework leds-pca955x so that we avoid the incorrect inversion and clarify
> the semantics with respect to GPIO.

I suppose this is correct. Have we made some tests on the pins directly
driven as GPIOs ? 

Thanks,

C.

> 
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> ---
>  drivers/leds/leds-pca955x.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/leds/leds-pca955x.c b/drivers/leds/leds-pca955x.c
> index 0217bac2f47b..a02c1fd5dee9 100644
> --- a/drivers/leds/leds-pca955x.c
> +++ b/drivers/leds/leds-pca955x.c
> @@ -61,6 +61,9 @@
>  #define PCA955X_LS_BLINK0	0x2	/* Blink at PWM0 rate */
>  #define PCA955X_LS_BLINK1	0x3	/* Blink at PWM1 rate */
>  
> +#define PCA955X_GPIO_HIGH	LED_OFF
> +#define PCA955X_GPIO_LOW	LED_FULL
> +
>  enum pca955x_type {
>  	pca9550,
>  	pca9551,
> @@ -292,9 +295,9 @@ static void pca955x_gpio_set_value(struct gpio_chip *gc, unsigned int offset,
>  	struct pca955x_led *led = &pca955x->leds[offset];
>  
>  	if (val)
> -		pca955x_led_set(&led->led_cdev, LED_FULL);
> +		pca955x_led_set(&led->led_cdev, PCA955X_GPIO_HIGH);
>  	else
> -		pca955x_led_set(&led->led_cdev, LED_OFF);
> +		pca955x_led_set(&led->led_cdev, PCA955X_GPIO_LOW);
>  }
>  
>  static int pca955x_gpio_get_value(struct gpio_chip *gc, unsigned int offset)
> 

Re: [PATCH linux dev-4.10 3/5] leds: pca955x: Don't invert requested value in pca955x_gpio_set_value()

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 7773 bytes --]

On Sun, 2017-08-27 at 09:03 +0200, Cédric Le Goater wrote:
> On 08/25/2017 08:52 AM, Andrew Jeffery wrote:
> > The PCA9552 lines can be used either for driving LEDs or as GPIOs. The
> > manual states that for LEDs, the operation is open-drain:
> > 
> >          The LSn LED select registers determine the source of the LED data.
> > 
> >            00 = output is set LOW (LED on)
> >            01 = output is set high-impedance (LED off; default)
> >            10 = output blinks at PWM0 rate
> >            11 = output blinks at PWM1 rate
> > 
> > For GPIOs it suggests a pull-up so that the open-case drives the line
> > high:
> > 
> >          For use as output, connect external pull-up resistor to the pin
> >          and size it according to the DC recommended operating
> >          characteristics.  LED output pin is HIGH when the output is
> >          programmed as high-impedance, and LOW when the output is
> >          programmed LOW through the ‘LED selector’ register.  The output
> >          can be pulse-width controlled when PWM0 or PWM1 are used.
> > 
> > Now, I have a hardware design that uses the LED controller to control
> > LEDs. However, for $reasons, we're using the leds-gpio driver to drive
> > the LEDs which means wending our way through the gpiochip abstractions.
> > So with that in mind we need to describe an active-low GPIO
> > configuration to drive the LEDs as though they were GPIOs.
> > 
> > The set() gpiochip callback in leds-pca955x does the following:
> > 
> >          ...
> >          if (val)
> >                 pca955x_led_set(&led->led_cdev, LED_FULL);
> >          else
> >                 pca955x_led_set(&led->led_cdev, LED_OFF);
> >          ...
> > 
> > Where LED_FULL = 255. pca955x_led_set() in turn does:
> > 
> >          ...
> >          switch (value) {
> >          case LED_FULL:
> >                 ls = pca955x_ledsel(ls, ls_led, PCA955X_LS_LED_ON);
> >                 break;
> >          ...
> > 
> > Where PCA955X_LS_LED_ON is defined as:
> > 
> > > > > >          #define PCA955X_LS_LED_ON	0x0	/* Output LOW */
> > 
> > So here we have some type confusion: We've crossed domains from GPIO
> > behaviour to LED behaviour without accounting for possible inversions
> > in the process.
> > 
> > Stepping back to leds-gpio for a moment, during probe() we call
> > create_gpio_led(), which eventually executes:
> > 
> >          if (template->default_state == LEDS_GPIO_DEFSTATE_KEEP) {
> >                 state = gpiod_get_value_cansleep(led_dat->gpiod);
> >                 if (state < 0)
> >                         return state;
> >          } else {
> >                 state = (template->default_state == LEDS_GPIO_DEFSTATE_ON);
> >          }
> >          ...
> >          ret = gpiod_direction_output(led_dat->gpiod, state);
> > 
> > In the devicetree the GPIO is annotated as active-low, and
> > gpiod_get_value_cansleep() handles this for us:
> > 
> >          int gpiod_get_value_cansleep(const struct gpio_desc *desc)
> >          {
> >                  int value;
> > 
> >                  might_sleep_if(extra_checks);
> >                  VALIDATE_DESC(desc);
> >                  value = _gpiod_get_raw_value(desc);
> >                  if (value < 0)
> >                          return value;
> > 
> >                  if (test_bit(FLAG_ACTIVE_LOW, &desc->flags))
> >                          value = !value;
> > 
> >                  return value;
> >          }
> > 
> > _gpiod_get_raw_value() in turn calls through the get() callback for the
> > gpiochip implementation, so returning to our get() implementation in
> > leds-pca955x we find we extract the raw value from hardware:
> > 
> >          static int pca955x_gpio_get_value(struct gpio_chip *gc, unsigned int offset)
> >          {
> >                  struct pca955x *pca955x = gpiochip_get_data(gc);
> >                  struct pca955x_led *led = &pca955x->leds[offset];
> >                  u8 reg = pca955x_read_input(pca955x->client, led->led_num / 8);
> > 
> >                  return !!(reg & (1 << (led->led_num % 8)));
> >          }
> > 
> > This behaviour is not symmetric with that of set(), where the val is
> > inverted by the driver.
> > 
> > Closing the loop on the GPIO_ACTIVE_LOW inversions,
> > gpiod_direction_output(), like gpiod_get_value_cansleep, handles it for
> > us:
> > 
> >          int gpiod_direction_output(struct gpio_desc *desc, int value)
> >          {
> >                   VALIDATE_DESC(desc);
> >                   if (test_bit(FLAG_ACTIVE_LOW, &desc->flags))
> >                            value = !value;
> >                   else
> >                            value = !!value;
> >                   return _gpiod_direction_output_raw(desc, value);
> >          }
> > 
> > All-in-all, with a value of 'keep' for default-state property in the
> > gpio-leds child node, the current state of the hardware will in-fact be
> > inverted; precisely the opposite of what was intended.
> > 
> > Rework leds-pca955x so that we avoid the incorrect inversion and clarify
> > the semantics with respect to GPIO.
> 
> I suppose this is correct. Have we made some tests on the pins directly
> driven as GPIOs ? 

Well, nothing besides an LED. But is an LED not a satisfactory use-
case? It's still a GPIO. As for active-high/active-low, well they're
pretty thoroughly tested by the GPIO subsystem.

Brandon Wyman has eyeballed the LEDs on a Witherspoon and confirms the
problem is resolved (with a caveat)[1].

I've rebased on top of the leds tree without issue. I'll send this
patch upstream to get some feedback.

Cheers,

Andrew

[1] https://github.com/openbmc/openbmc/issues/2156#issuecomment-325035185

> 
> Thanks,
> 
> C.
> 
> > 
> > > > Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> > ---
> >  drivers/leds/leds-pca955x.c | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/leds/leds-pca955x.c b/drivers/leds/leds-pca955x.c
> > index 0217bac2f47b..a02c1fd5dee9 100644
> > --- a/drivers/leds/leds-pca955x.c
> > +++ b/drivers/leds/leds-pca955x.c
> > @@ -61,6 +61,9 @@
> > > > > >  #define PCA955X_LS_BLINK0	0x2	/* Blink at PWM0 rate */
> > > > > >  #define PCA955X_LS_BLINK1	0x3	/* Blink at PWM1 rate */
> >  
> > > > +#define PCA955X_GPIO_HIGH	LED_OFF
> > > > +#define PCA955X_GPIO_LOW	LED_FULL
> > +
> >  enum pca955x_type {
> > > >  	pca9550,
> > > >  	pca9551,
> > @@ -292,9 +295,9 @@ static void pca955x_gpio_set_value(struct gpio_chip *gc, unsigned int offset,
> > > >  	struct pca955x_led *led = &pca955x->leds[offset];
> >  
> > > >  	if (val)
> > > > -		pca955x_led_set(&led->led_cdev, LED_FULL);
> > > > +		pca955x_led_set(&led->led_cdev, PCA955X_GPIO_HIGH);
> > > >  	else
> > > > -		pca955x_led_set(&led->led_cdev, LED_OFF);
> > > > +		pca955x_led_set(&led->led_cdev, PCA955X_GPIO_LOW);
> >  }
> >  
> >  static int pca955x_gpio_get_value(struct gpio_chip *gc, unsigned int offset)
> > 
> 
> 

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH linux dev-4.10 3/5] leds: pca955x: Don't invert requested value in pca955x_gpio_set_value()

[Brandon Wyman]

On Mon, Aug 28, 2017 at 12:16 AM, Andrew Jeffery <andrew@aj.id.au> wrote:
> On Sun, 2017-08-27 at 09:03 +0200, Cédric Le Goater wrote:
>> On 08/25/2017 08:52 AM, Andrew Jeffery wrote:
>> > The PCA9552 lines can be used either for driving LEDs or as GPIOs. The
>> > manual states that for LEDs, the operation is open-drain:
>> >
>> >          The LSn LED select registers determine the source of the LED data.
>> >
>> >            00 = output is set LOW (LED on)
>> >            01 = output is set high-impedance (LED off; default)
>> >            10 = output blinks at PWM0 rate
>> >            11 = output blinks at PWM1 rate
>> >
>> > For GPIOs it suggests a pull-up so that the open-case drives the line
>> > high:
>> >
>> >          For use as output, connect external pull-up resistor to the pin
>> >          and size it according to the DC recommended operating
>> >          characteristics.  LED output pin is HIGH when the output is
>> >          programmed as high-impedance, and LOW when the output is
>> >          programmed LOW through the ‘LED selector’ register.  The output
>> >          can be pulse-width controlled when PWM0 or PWM1 are used.
>> >
>> > Now, I have a hardware design that uses the LED controller to control
>> > LEDs. However, for $reasons, we're using the leds-gpio driver to drive
>> > the LEDs which means wending our way through the gpiochip abstractions.
>> > So with that in mind we need to describe an active-low GPIO
>> > configuration to drive the LEDs as though they were GPIOs.
>> >
>> > The set() gpiochip callback in leds-pca955x does the following:
>> >
>> >          ...
>> >          if (val)
>> >                 pca955x_led_set(&led->led_cdev, LED_FULL);
>> >          else
>> >                 pca955x_led_set(&led->led_cdev, LED_OFF);
>> >          ...
>> >
>> > Where LED_FULL = 255. pca955x_led_set() in turn does:
>> >
>> >          ...
>> >          switch (value) {
>> >          case LED_FULL:
>> >                 ls = pca955x_ledsel(ls, ls_led, PCA955X_LS_LED_ON);
>> >                 break;
>> >          ...
>> >
>> > Where PCA955X_LS_LED_ON is defined as:
>> >
>> > > > > >          #define PCA955X_LS_LED_ON  0x0     /* Output LOW */
>> >
>> > So here we have some type confusion: We've crossed domains from GPIO
>> > behaviour to LED behaviour without accounting for possible inversions
>> > in the process.
>> >
>> > Stepping back to leds-gpio for a moment, during probe() we call
>> > create_gpio_led(), which eventually executes:
>> >
>> >          if (template->default_state == LEDS_GPIO_DEFSTATE_KEEP) {
>> >                 state = gpiod_get_value_cansleep(led_dat->gpiod);
>> >                 if (state < 0)
>> >                         return state;
>> >          } else {
>> >                 state = (template->default_state == LEDS_GPIO_DEFSTATE_ON);
>> >          }
>> >          ...
>> >          ret = gpiod_direction_output(led_dat->gpiod, state);
>> >
>> > In the devicetree the GPIO is annotated as active-low, and
>> > gpiod_get_value_cansleep() handles this for us:
>> >
>> >          int gpiod_get_value_cansleep(const struct gpio_desc *desc)
>> >          {
>> >                  int value;
>> >
>> >                  might_sleep_if(extra_checks);
>> >                  VALIDATE_DESC(desc);
>> >                  value = _gpiod_get_raw_value(desc);
>> >                  if (value < 0)
>> >                          return value;
>> >
>> >                  if (test_bit(FLAG_ACTIVE_LOW, &desc->flags))
>> >                          value = !value;
>> >
>> >                  return value;
>> >          }
>> >
>> > _gpiod_get_raw_value() in turn calls through the get() callback for the
>> > gpiochip implementation, so returning to our get() implementation in
>> > leds-pca955x we find we extract the raw value from hardware:
>> >
>> >          static int pca955x_gpio_get_value(struct gpio_chip *gc, unsigned int offset)
>> >          {
>> >                  struct pca955x *pca955x = gpiochip_get_data(gc);
>> >                  struct pca955x_led *led = &pca955x->leds[offset];
>> >                  u8 reg = pca955x_read_input(pca955x->client, led->led_num / 8);
>> >
>> >                  return !!(reg & (1 << (led->led_num % 8)));
>> >          }
>> >
>> > This behaviour is not symmetric with that of set(), where the val is
>> > inverted by the driver.
>> >
>> > Closing the loop on the GPIO_ACTIVE_LOW inversions,
>> > gpiod_direction_output(), like gpiod_get_value_cansleep, handles it for
>> > us:
>> >
>> >          int gpiod_direction_output(struct gpio_desc *desc, int value)
>> >          {
>> >                   VALIDATE_DESC(desc);
>> >                   if (test_bit(FLAG_ACTIVE_LOW, &desc->flags))
>> >                            value = !value;
>> >                   else
>> >                            value = !!value;
>> >                   return _gpiod_direction_output_raw(desc, value);
>> >          }
>> >
>> > All-in-all, with a value of 'keep' for default-state property in the
>> > gpio-leds child node, the current state of the hardware will in-fact be
>> > inverted; precisely the opposite of what was intended.
>> >
>> > Rework leds-pca955x so that we avoid the incorrect inversion and clarify
>> > the semantics with respect to GPIO.
>>
>> I suppose this is correct. Have we made some tests on the pins directly
>> driven as GPIOs ?
>
> Well, nothing besides an LED. But is an LED not a satisfactory use-
> case? It's still a GPIO. As for active-high/active-low, well they're
> pretty thoroughly tested by the GPIO subsystem.
>
> Brandon Wyman has eyeballed the LEDs on a Witherspoon and confirms the
> problem is resolved (with a caveat)[1].
>
> I've rebased on top of the leds tree without issue. I'll send this
> patch upstream to get some feedback.
>
> Cheers,
>
> Andrew
>
> [1] https://github.com/openbmc/openbmc/issues/2156#issuecomment-325035185
>
>>
>> Thanks,
>>
>> C.
>>
>> >
>> > > > Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
>> > ---
>> >  drivers/leds/leds-pca955x.c | 7 +++++--
>> >  1 file changed, 5 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/drivers/leds/leds-pca955x.c b/drivers/leds/leds-pca955x.c
>> > index 0217bac2f47b..a02c1fd5dee9 100644
>> > --- a/drivers/leds/leds-pca955x.c
>> > +++ b/drivers/leds/leds-pca955x.c
>> > @@ -61,6 +61,9 @@
>> > > > > >  #define PCA955X_LS_BLINK0  0x2     /* Blink at PWM0 rate */
>> > > > > >  #define PCA955X_LS_BLINK1  0x3     /* Blink at PWM1 rate */
>> >
>> > > > +#define PCA955X_GPIO_HIGH      LED_OFF
>> > > > +#define PCA955X_GPIO_LOW       LED_FULL
>> > +
>> >  enum pca955x_type {
>> > > >         pca9550,
>> > > >         pca9551,
>> > @@ -292,9 +295,9 @@ static void pca955x_gpio_set_value(struct gpio_chip *gc, unsigned int offset,
>> > > >         struct pca955x_led *led = &pca955x->leds[offset];
>> >
>> > > >         if (val)
>> > > > -               pca955x_led_set(&led->led_cdev, LED_FULL);
>> > > > +               pca955x_led_set(&led->led_cdev, PCA955X_GPIO_HIGH);
>> > > >         else
>> > > > -               pca955x_led_set(&led->led_cdev, LED_OFF);
>> > > > +               pca955x_led_set(&led->led_cdev, PCA955X_GPIO_LOW);
>> >  }
>> >
>> >  static int pca955x_gpio_get_value(struct gpio_chip *gc, unsigned int offset)
>> >
>>
>>

Tested-by: Brandon Wyman <bjwyman@gmail.com>

[PATCH linux dev-4.10 4/5] aspeed: witherspoon: Tidy and unify LED nodes

[Andrew Jeffery]

There's no reason for these to be separate, the gpios property defines
the controller needed for each LED. Also fix the node names so we can
drop the label property.

Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
 arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts | 25 ++++++++++--------------
 1 file changed, 10 insertions(+), 15 deletions(-)

diff --git a/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts b/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts
index dd30c20c9fb2..81996f070818 100644
--- a/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts
+++ b/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts
@@ -87,48 +87,43 @@
 		};
 	};
 
-	pca_leds {
+	leds {
 		compatible = "gpio-leds";
 
 		fan0 {
-			label = "fan0";
 			default-state = "keep";
 			gpios = <&pca0 0 GPIO_ACTIVE_LOW>;
 		};
+
 		fan1 {
-			label = "fan1";
 			default-state = "keep";
 			gpios = <&pca0 1 GPIO_ACTIVE_LOW>;
 		};
+
 		fan2 {
-			label = "fan2";
 			default-state = "keep";
 			gpios = <&pca0 2 GPIO_ACTIVE_LOW>;
 		};
+
 		fan3 {
-			label = "fan3";
 			default-state = "keep";
 			gpios = <&pca0 3 GPIO_ACTIVE_LOW>;
 		};
-		front_fault {
-			label = "front-fault";
+
+		front-fault {
 			default-state = "keep";
 			gpios = <&pca0 13 GPIO_ACTIVE_LOW>;
 		};
-		front_power {
-			label = "front-power";
+
+		front-power {
 			default-state = "keep";
 			gpios = <&pca0 14 GPIO_ACTIVE_LOW>;
 		};
-		front_id {
-			label = "front-id";
+
+		front-id {
 			default-state = "keep";
 			gpios = <&pca0 15 GPIO_ACTIVE_LOW>;
 		};
-	};
-
-	leds {
-		compatible = "gpio-leds";
 
 		rear-fault {
 			gpios = <&gpio ASPEED_GPIO(N, 2) GPIO_ACTIVE_LOW>;
-- 
2.11.0

Re: [PATCH linux dev-4.10 4/5] aspeed: witherspoon: Tidy and unify LED nodes

[Brandon Wyman]

On Fri, Aug 25, 2017 at 1:52 AM, Andrew Jeffery <andrew@aj.id.au> wrote:
> There's no reason for these to be separate, the gpios property defines
> the controller needed for each LED. Also fix the node names so we can
> drop the label property.
>
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> ---
>  arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts | 25 ++++++++++--------------
>  1 file changed, 10 insertions(+), 15 deletions(-)
>
> diff --git a/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts b/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts
> index dd30c20c9fb2..81996f070818 100644
> --- a/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts
> +++ b/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts
> @@ -87,48 +87,43 @@
>                 };
>         };
>
> -       pca_leds {
> +       leds {
>                 compatible = "gpio-leds";
>
>                 fan0 {
> -                       label = "fan0";
>                         default-state = "keep";
>                         gpios = <&pca0 0 GPIO_ACTIVE_LOW>;
>                 };
> +
>                 fan1 {
> -                       label = "fan1";
>                         default-state = "keep";
>                         gpios = <&pca0 1 GPIO_ACTIVE_LOW>;
>                 };
> +
>                 fan2 {
> -                       label = "fan2";
>                         default-state = "keep";
>                         gpios = <&pca0 2 GPIO_ACTIVE_LOW>;
>                 };
> +
>                 fan3 {
> -                       label = "fan3";
>                         default-state = "keep";
>                         gpios = <&pca0 3 GPIO_ACTIVE_LOW>;
>                 };
> -               front_fault {
> -                       label = "front-fault";
> +
> +               front-fault {
>                         default-state = "keep";
>                         gpios = <&pca0 13 GPIO_ACTIVE_LOW>;
>                 };
> -               front_power {
> -                       label = "front-power";
> +
> +               front-power {
>                         default-state = "keep";
>                         gpios = <&pca0 14 GPIO_ACTIVE_LOW>;
>                 };
> -               front_id {
> -                       label = "front-id";
> +
> +               front-id {
>                         default-state = "keep";
>                         gpios = <&pca0 15 GPIO_ACTIVE_LOW>;
>                 };
> -       };
> -
> -       leds {
> -               compatible = "gpio-leds";
>
>                 rear-fault {
>                         gpios = <&gpio ASPEED_GPIO(N, 2) GPIO_ACTIVE_LOW>;
> --
> 2.11.0
>

Tested-by: Brandon Wyman <bjwyman@gmail.com>

[PATCH linux dev-4.10 5/5] aspeed: witherspoon: leds: Retain state across BMC resets

[Andrew Jeffery]

Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
 arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts b/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts
index 81996f070818..68058eca1559 100644
--- a/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts
+++ b/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts
@@ -91,36 +91,43 @@
 		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>;
 		};
 
 		front-fault {
+			retain-state-shutdown;
 			default-state = "keep";
 			gpios = <&pca0 13 GPIO_ACTIVE_LOW>;
 		};
 
 		front-power {
+			retain-state-shutdown;
 			default-state = "keep";
 			gpios = <&pca0 14 GPIO_ACTIVE_LOW>;
 		};
 
 		front-id {
+			retain-state-shutdown;
 			default-state = "keep";
 			gpios = <&pca0 15 GPIO_ACTIVE_LOW>;
 		};
-- 
2.11.0

Re: [PATCH linux dev-4.10 0/5] Retain front LED state on Witherspoon

[Brandon Wyman]

On Fri, Aug 25, 2017 at 1:52 AM, Andrew Jeffery <andrew@aj.id.au> wrote:
> Hello,
>
> This series fixes witherspoon to retain its front LED state across BMC resets.
> It was noted in issue 2156[1] that the front LEDs came on solid during a BMC
> boot. This appears to be the result of several bugs which are addressed in the
> following patches.
>
> The changes to leds-gpio have been sent upstream:
>
>   https://lkml.org/lkml/2017/8/25/44
>
> The pca955x patch depends on changes we appear to only have in the dev-4.10
> tree (i.e. they haven't been applied upstream). There's some discussion at [2]
> but I'm not aware of a follow-up patch from Cédric. Certainly applying my patch
> to upstream lead to signifiant conflicts, so I haven't yet tried to send it on
> its way.
>
> Cédric: I know you're undecided on how I've approached the pca955x patch - I
> figured we could argue about it on the list in case anyone else had helpful
> insights to settle the dispute :)
>
> The final two patches clean up the leds nodes in the Witherspoon devicetree and
> add the new retain-state-shutdown property to the necessary LEDs. I do wonder
> if we want to retain the state on the rear LEDs as well, but that requires more
> features to be added to the Aspeed GPIO controller.
>
> The patches have been tested on a Witherspoon system, albeit remotely. I can't
> put eyes on the LEDs in question so it would be good for someone who has the
> ability to do so. I used the test outlined by Vishwa at [3].
>
> Please review!
>
> Andrew
>
> [1] https://github.com/openbmc/openbmc/issues/2156
> [2] http://patchwork.ozlabs.org/patch/760006/
> [3] https://github.com/openbmc/openbmc/issues/2156#issuecomment-324101828
>
> Andrew Jeffery (5):
>   dt-bindings: leds: gpio: Add optional retain-state-shutdown property
>   leds: gpio: Allow LED to retain state at shutdown
>   leds: pca955x: Don't invert requested value in
>     pca955x_gpio_set_value()
>   aspeed: witherspoon: Tidy and unify LED nodes
>   aspeed: witherspoon: leds: Retain state across BMC resets
>
>  .../devicetree/bindings/leds/leds-gpio.txt         |  3 ++
>  arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts   | 32 ++++++++++++----------
>  drivers/leds/leds-gpio.c                           |  7 ++++-
>  drivers/leds/leds-pca955x.c                        |  7 +++--
>  include/linux/leds.h                               |  3 ++
>  5 files changed, 34 insertions(+), 18 deletions(-)
>
> --
> 2.11.0
>

Tested-by: Brandon Wyman <bjwyman@gmail.com>

Re: [PATCH linux dev-4.10 0/5] Retain front LED state on Witherspoon

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 2982 bytes --]

On Mon, 2017-08-28 at 16:11 -0500, Brandon Wyman wrote:
> > On Fri, Aug 25, 2017 at 1:52 AM, Andrew Jeffery <andrew@aj.id.au> wrote:
> > Hello,
> > 
> > This series fixes witherspoon to retain its front LED state across BMC resets.
> > It was noted in issue 2156[1] that the front LEDs came on solid during a BMC
> > boot. This appears to be the result of several bugs which are addressed in the
> > following patches.
> > 
> > The changes to leds-gpio have been sent upstream:
> > 
> >   https://lkml.org/lkml/2017/8/25/44
> > 
> > The pca955x patch depends on changes we appear to only have in the dev-4.10
> > tree (i.e. they haven't been applied upstream). There's some discussion at [2]
> > but I'm not aware of a follow-up patch from Cédric. Certainly applying my patch
> > to upstream lead to signifiant conflicts, so I haven't yet tried to send it on
> > its way.
> > 
> > Cédric: I know you're undecided on how I've approached the pca955x patch - I
> > figured we could argue about it on the list in case anyone else had helpful
> > insights to settle the dispute :)
> > 
> > The final two patches clean up the leds nodes in the Witherspoon devicetree and
> > add the new retain-state-shutdown property to the necessary LEDs. I do wonder
> > if we want to retain the state on the rear LEDs as well, but that requires more
> > features to be added to the Aspeed GPIO controller.
> > 
> > The patches have been tested on a Witherspoon system, albeit remotely. I can't
> > put eyes on the LEDs in question so it would be good for someone who has the
> > ability to do so. I used the test outlined by Vishwa at [3].
> > 
> > Please review!
> > 
> > Andrew
> > 
> > [1] https://github.com/openbmc/openbmc/issues/2156
> > [2] http://patchwork.ozlabs.org/patch/760006/
> > [3] https://github.com/openbmc/openbmc/issues/2156#issuecomment-324101828
> > 
> > Andrew Jeffery (5):
> >   dt-bindings: leds: gpio: Add optional retain-state-shutdown property
> >   leds: gpio: Allow LED to retain state at shutdown
> >   leds: pca955x: Don't invert requested value in
> >     pca955x_gpio_set_value()
> >   aspeed: witherspoon: Tidy and unify LED nodes
> >   aspeed: witherspoon: leds: Retain state across BMC resets
> > 
> >  .../devicetree/bindings/leds/leds-gpio.txt         |  3 ++
> >  arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts   | 32 ++++++++++++----------
> >  drivers/leds/leds-gpio.c                           |  7 ++++-
> >  drivers/leds/leds-pca955x.c                        |  7 +++--
> >  include/linux/leds.h                               |  3 ++
> >  5 files changed, 34 insertions(+), 18 deletions(-)
> > 
> > --
> > 2.11.0
> > 
> 
> > Tested-by: Brandon Wyman <bjwyman@gmail.com>

Thanks Brandon, I've applied the series to dev-4.10 with your Tested-by tags.

Andrew

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]