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

Brandon Wyman bjwyman at gmail.com
Tue Aug 29 07:16:18 AEST 2017


On Mon, Aug 28, 2017 at 12:16 AM, Andrew Jeffery <andrew at 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 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)
>> >
>>
>>

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


More information about the openbmc mailing list