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

[Andrew Jeffery]

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 at 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)
> > 
> 
> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: This is a digitally signed message part
URL: <http://lists.ozlabs.org/pipermail/openbmc/attachments/20170828/75370eca/attachment.sig>