[{"id":1758215,"web_url":"http://patchwork.ozlabs.org/comment/1758215/","msgid":"<503ada3f-cabe-0f73-1907-f1b62470c5ed@kaod.org>","list_archive_url":null,"date":"2017-08-27T07:03:29","subject":"Re: [PATCH linux dev-4.10 3/5] leds: pca955x: Don't invert requested\n\tvalue in pca955x_gpio_set_value()","submitter":{"id":68548,"url":"http://patchwork.ozlabs.org/api/people/68548/","name":"Cédric Le Goater","email":"clg@kaod.org"},"content":"On 08/25/2017 08:52 AM, Andrew Jeffery wrote:\n> The PCA9552 lines can be used either for driving LEDs or as GPIOs. The\n> manual states that for LEDs, the operation is open-drain:\n> \n> The LSn LED select registers determine the source of the LED data.\n> \n> 00 = output is set LOW (LED on)\n> 01 = output is set high-impedance (LED off; default)\n> 10 = output blinks at PWM0 rate\n> 11 = output blinks at PWM1 rate\n> \n> For GPIOs it suggests a pull-up so that the open-case drives the line\n> high:\n> \n> For use as output, connect external pull-up resistor to the pin\n> and size it according to the DC recommended operating\n> characteristics. LED output pin is HIGH when the output is\n> programmed as high-impedance, and LOW when the output is\n> programmed LOW through the ‘LED selector’ register. The output\n> can be pulse-width controlled when PWM0 or PWM1 are used.\n> \n> Now, I have a hardware design that uses the LED controller to control\n> LEDs. However, for $reasons, we're using the leds-gpio driver to drive\n> the LEDs which means wending our way through the gpiochip abstractions.\n> So with that in mind we need to describe an active-low GPIO\n> configuration to drive the LEDs as though they were GPIOs.\n> \n> The set() gpiochip callback in leds-pca955x does the following:\n> \n> ...\n> if (val)\n> pca955x_led_set(&led->led_cdev, LED_FULL);\n> else\n> pca955x_led_set(&led->led_cdev, LED_OFF);\n> ...\n> \n> Where LED_FULL = 255. pca955x_led_set() in turn does:\n> \n> ...\n> switch (value) {\n> case LED_FULL:\n> ls = pca955x_ledsel(ls, ls_led, PCA955X_LS_LED_ON);\n> break;\n> ...\n> \n> Where PCA955X_LS_LED_ON is defined as:\n> \n> #define PCA955X_LS_LED_ON\t0x0\t/* Output LOW */\n> \n> So here we have some type confusion: We've crossed domains from GPIO\n> behaviour to LED behaviour without accounting for possible inversions\n> in the process.\n> \n> Stepping back to leds-gpio for a moment, during probe() we call\n> create_gpio_led(), which eventually executes:\n> \n> if (template->default_state == LEDS_GPIO_DEFSTATE_KEEP) {\n> state = gpiod_get_value_cansleep(led_dat->gpiod);\n> if (state < 0)\n> return state;\n> } else {\n> state = (template->default_state == LEDS_GPIO_DEFSTATE_ON);\n> }\n> ...\n> ret = gpiod_direction_output(led_dat->gpiod, state);\n> \n> In the devicetree the GPIO is annotated as active-low, and\n> gpiod_get_value_cansleep() handles this for us:\n> \n> int gpiod_get_value_cansleep(const struct gpio_desc *desc)\n> {\n> int value;\n> \n> might_sleep_if(extra_checks);\n> VALIDATE_DESC(desc);\n> value = _gpiod_get_raw_value(desc);\n> if (value < 0)\n> return value;\n> \n> if (test_bit(FLAG_ACTIVE_LOW, &desc->flags))\n> value = !value;\n> \n> return value;\n> }\n> \n> _gpiod_get_raw_value() in turn calls through the get() callback for the\n> gpiochip implementation, so returning to our get() implementation in\n> leds-pca955x we find we extract the raw value from hardware:\n> \n> static int pca955x_gpio_get_value(struct gpio_chip *gc, unsigned int offset)\n> {\n> struct pca955x *pca955x = gpiochip_get_data(gc);\n> struct pca955x_led *led = &pca955x->leds[offset];\n> u8 reg = pca955x_read_input(pca955x->client, led->led_num / 8);\n> \n> return !!(reg & (1 << (led->led_num % 8)));\n> }\n> \n> This behaviour is not symmetric with that of set(), where the val is\n> inverted by the driver.\n> \n> Closing the loop on the GPIO_ACTIVE_LOW inversions,\n> gpiod_direction_output(), like gpiod_get_value_cansleep, handles it for\n> us:\n> \n> int gpiod_direction_output(struct gpio_desc *desc, int value)\n> {\n> VALIDATE_DESC(desc);\n> if (test_bit(FLAG_ACTIVE_LOW, &desc->flags))\n> value = !value;\n> else\n> value = !!value;\n> return _gpiod_direction_output_raw(desc, value);\n> }\n> \n> All-in-all, with a value of 'keep' for default-state property in the\n> gpio-leds child node, the current state of the hardware will in-fact be\n> inverted; precisely the opposite of what was intended.\n> \n> Rework leds-pca955x so that we avoid the incorrect inversion and clarify\n> the semantics with respect to GPIO.\n\nI suppose this is correct. Have we made some tests on the pins directly\ndriven as GPIOs ? \n\nThanks,\n\nC.\n\n> \n> Signed-off-by: Andrew Jeffery \n> ---\n> drivers/leds/leds-pca955x.c | 7 +++++--\n> 1 file changed, 5 insertions(+), 2 deletions(-)\n> \n> diff --git a/drivers/leds/leds-pca955x.c b/drivers/leds/leds-pca955x.c\n> index 0217bac2f47b..a02c1fd5dee9 100644\n> --- a/drivers/leds/leds-pca955x.c\n> +++ b/drivers/leds/leds-pca955x.c\n> @@ -61,6 +61,9 @@\n> #define PCA955X_LS_BLINK0\t0x2\t/* Blink at PWM0 rate */\n> #define PCA955X_LS_BLINK1\t0x3\t/* Blink at PWM1 rate */\n> \n> +#define PCA955X_GPIO_HIGH\tLED_OFF\n> +#define PCA955X_GPIO_LOW\tLED_FULL\n> +\n> enum pca955x_type {\n> \tpca9550,\n> \tpca9551,\n> @@ -292,9 +295,9 @@ static void pca955x_gpio_set_value(struct gpio_chip *gc, unsigned int offset,\n> \tstruct pca955x_led *led = &pca955x->leds[offset];\n> \n> \tif (val)\n> -\t\tpca955x_led_set(&led->led_cdev, LED_FULL);\n> +\t\tpca955x_led_set(&led->led_cdev, PCA955X_GPIO_HIGH);\n> \telse\n> -\t\tpca955x_led_set(&led->led_cdev, LED_OFF);\n> +\t\tpca955x_led_set(&led->led_cdev, PCA955X_GPIO_LOW);\n> }\n> \n> static int pca955x_gpio_get_value(struct gpio_chip *gc, unsigned int offset)\n>","headers":{"Return-Path":"","X-Original-To":["incoming@patchwork.ozlabs.org","openbmc@lists.ozlabs.org"],"Delivered-To":["patchwork-incoming@bilbo.ozlabs.org","openbmc@lists.ozlabs.org"],"Received":["from lists.ozlabs.org (lists.ozlabs.org [IPv6:2401:3900:2:1::3])\n\t(using TLSv1.2 with cipher ADH-AES256-GCM-SHA384 (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xg5wg30NVz9s8J\n\tfor ;\n\tSun, 27 Aug 2017 17:23:31 +1000 (AEST)","from lists.ozlabs.org (lists.ozlabs.org [IPv6:2401:3900:2:1::3])\n\tby lists.ozlabs.org (Postfix) with ESMTP id 3xg5wf4JPwzDqgM\n\tfor ;\n\tSun, 27 Aug 2017 17:23:30 +1000 (AEST)","from 19.mo6.mail-out.ovh.net (19.mo6.mail-out.ovh.net\n\t[188.165.56.177])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256\n\tbits)) (No client certificate requested)\n\tby lists.ozlabs.org (Postfix) with ESMTPS id 3xg5vF1z7RzDqg5\n\tfor ; Sun, 27 Aug 2017 17:22:15 +1000 (AEST)","from player787.ha.ovh.net (b9.ovh.net [213.186.33.59])\n\tby mo6.mail-out.ovh.net (Postfix) with ESMTP id 3402F1038C2\n\tfor ; Sun, 27 Aug 2017 09:03:36 +0200 (CEST)","from zorba.kaod.org (LFbn-1-2231-173.w90-76.abo.wanadoo.fr\n\t[90.76.52.173]) (Authenticated sender: postmaster@kaod.org)\n\tby player787.ha.ovh.net (Postfix) with ESMTPSA id 121F1600076;\n\tSun, 27 Aug 2017 09:03:30 +0200 (CEST)"],"Subject":"Re: [PATCH linux dev-4.10 3/5] leds: pca955x: Don't invert requested\n\tvalue in pca955x_gpio_set_value()","To":"Andrew Jeffery , joel@jms.id.au","References":"<20170825065244.488-1-andrew@aj.id.au>\n\t<20170825065244.488-4-andrew@aj.id.au>","From":"=?utf-8?q?C=C3=A9dric_Le_Goater?= ","Message-ID":"<503ada3f-cabe-0f73-1907-f1b62470c5ed@kaod.org>","Date":"Sun, 27 Aug 2017 09:03:29 +0200","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.2.1","MIME-Version":"1.0","In-Reply-To":"<20170825065244.488-4-andrew@aj.id.au>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-US","Content-Transfer-Encoding":"8bit","X-Ovh-Tracer-Id":"7982630341510138626","X-VR-SPAMSTATE":"OK","X-VR-SPAMSCORE":"-100","X-VR-SPAMCAUSE":"gggruggvucftvghtrhhoucdtuddrfeelledrtdelgdejfecutefuodetggdotefrodftvfcurfhrohhfihhlvgemucfqggfjpdevjffgvefmvefgnecuuegrihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmd","X-BeenThere":"openbmc@lists.ozlabs.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"Development list for OpenBMC ","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"openbmc@lists.ozlabs.org, bjwyman@gmail.com, eajames@linux.vnet.ibm.com","Errors-To":"openbmc-bounces+incoming=patchwork.ozlabs.org@lists.ozlabs.org","Sender":"\"openbmc\"\n\t"}},{"id":1758364,"web_url":"http://patchwork.ozlabs.org/comment/1758364/","msgid":"<1503897371.32695.16.camel@aj.id.au>","list_archive_url":null,"date":"2017-08-28T05:16:11","subject":"Re: [PATCH linux dev-4.10 3/5] leds: pca955x: Don't invert\n\trequested value in pca955x_gpio_set_value()","submitter":{"id":68332,"url":"http://patchwork.ozlabs.org/api/people/68332/","name":"Andrew Jeffery","email":"andrew@aj.id.au"},"content":"On Sun, 2017-08-27 at 09:03 +0200, Cédric Le Goater wrote:\n> On 08/25/2017 08:52 AM, Andrew Jeffery wrote:\n> > The PCA9552 lines can be used either for driving LEDs or as GPIOs. The\n> > manual states that for LEDs, the operation is open-drain:\n> > \n> >          The LSn LED select registers determine the source of the LED data.\n> > \n> >            00 = output is set LOW (LED on)\n> >            01 = output is set high-impedance (LED off; default)\n> >            10 = output blinks at PWM0 rate\n> >            11 = output blinks at PWM1 rate\n> > \n> > For GPIOs it suggests a pull-up so that the open-case drives the line\n> > high:\n> > \n> >          For use as output, connect external pull-up resistor to the pin\n> >          and size it according to the DC recommended operating\n> >          characteristics.  LED output pin is HIGH when the output is\n> >          programmed as high-impedance, and LOW when the output is\n> >          programmed LOW through the ‘LED selector’ register.  The output\n> >          can be pulse-width controlled when PWM0 or PWM1 are used.\n> > \n> > Now, I have a hardware design that uses the LED controller to control\n> > LEDs. However, for $reasons, we're using the leds-gpio driver to drive\n> > the LEDs which means wending our way through the gpiochip abstractions.\n> > So with that in mind we need to describe an active-low GPIO\n> > configuration to drive the LEDs as though they were GPIOs.\n> > \n> > The set() gpiochip callback in leds-pca955x does the following:\n> > \n> >          ...\n> >          if (val)\n> >                 pca955x_led_set(&led->led_cdev, LED_FULL);\n> >          else\n> >                 pca955x_led_set(&led->led_cdev, LED_OFF);\n> >          ...\n> > \n> > Where LED_FULL = 255. pca955x_led_set() in turn does:\n> > \n> >          ...\n> >          switch (value) {\n> >          case LED_FULL:\n> >                 ls = pca955x_ledsel(ls, ls_led, PCA955X_LS_LED_ON);\n> >                 break;\n> >          ...\n> > \n> > Where PCA955X_LS_LED_ON is defined as:\n> > \n> > > > > >          #define PCA955X_LS_LED_ON\t0x0\t/* Output LOW */\n> > \n> > So here we have some type confusion: We've crossed domains from GPIO\n> > behaviour to LED behaviour without accounting for possible inversions\n> > in the process.\n> > \n> > Stepping back to leds-gpio for a moment, during probe() we call\n> > create_gpio_led(), which eventually executes:\n> > \n> >          if (template->default_state == LEDS_GPIO_DEFSTATE_KEEP) {\n> >                 state = gpiod_get_value_cansleep(led_dat->gpiod);\n> >                 if (state < 0)\n> >                         return state;\n> >          } else {\n> >                 state = (template->default_state == LEDS_GPIO_DEFSTATE_ON);\n> >          }\n> >          ...\n> >          ret = gpiod_direction_output(led_dat->gpiod, state);\n> > \n> > In the devicetree the GPIO is annotated as active-low, and\n> > gpiod_get_value_cansleep() handles this for us:\n> > \n> >          int gpiod_get_value_cansleep(const struct gpio_desc *desc)\n> >          {\n> >                  int value;\n> > \n> >                  might_sleep_if(extra_checks);\n> >                  VALIDATE_DESC(desc);\n> >                  value = _gpiod_get_raw_value(desc);\n> >                  if (value < 0)\n> >                          return value;\n> > \n> >                  if (test_bit(FLAG_ACTIVE_LOW, &desc->flags))\n> >                          value = !value;\n> > \n> >                  return value;\n> >          }\n> > \n> > _gpiod_get_raw_value() in turn calls through the get() callback for the\n> > gpiochip implementation, so returning to our get() implementation in\n> > leds-pca955x we find we extract the raw value from hardware:\n> > \n> >          static int pca955x_gpio_get_value(struct gpio_chip *gc, unsigned int offset)\n> >          {\n> >                  struct pca955x *pca955x = gpiochip_get_data(gc);\n> >                  struct pca955x_led *led = &pca955x->leds[offset];\n> >                  u8 reg = pca955x_read_input(pca955x->client, led->led_num / 8);\n> > \n> >                  return !!(reg & (1 << (led->led_num % 8)));\n> >          }\n> > \n> > This behaviour is not symmetric with that of set(), where the val is\n> > inverted by the driver.\n> > \n> > Closing the loop on the GPIO_ACTIVE_LOW inversions,\n> > gpiod_direction_output(), like gpiod_get_value_cansleep, handles it for\n> > us:\n> > \n> >          int gpiod_direction_output(struct gpio_desc *desc, int value)\n> >          {\n> >                   VALIDATE_DESC(desc);\n> >                   if (test_bit(FLAG_ACTIVE_LOW, &desc->flags))\n> >                            value = !value;\n> >                   else\n> >                            value = !!value;\n> >                   return _gpiod_direction_output_raw(desc, value);\n> >          }\n> > \n> > All-in-all, with a value of 'keep' for default-state property in the\n> > gpio-leds child node, the current state of the hardware will in-fact be\n> > inverted; precisely the opposite of what was intended.\n> > \n> > Rework leds-pca955x so that we avoid the incorrect inversion and clarify\n> > the semantics with respect to GPIO.\n> \n> I suppose this is correct. Have we made some tests on the pins directly\n> driven as GPIOs ? \n\nWell, nothing besides an LED. But is an LED not a satisfactory use-\ncase? It's still a GPIO. As for active-high/active-low, well they're\npretty thoroughly tested by the GPIO subsystem.\n\nBrandon Wyman has eyeballed the LEDs on a Witherspoon and confirms the\nproblem is resolved (with a caveat)[1].\n\nI've rebased on top of the leds tree without issue. I'll send this\npatch upstream to get some feedback.\n\nCheers,\n\nAndrew\n\n[1] https://github.com/openbmc/openbmc/issues/2156#issuecomment-325035185\n\n> \n> Thanks,\n> \n> C.\n> \n> > \n> > > > Signed-off-by: Andrew Jeffery \n> > ---\n> >  drivers/leds/leds-pca955x.c | 7 +++++--\n> >  1 file changed, 5 insertions(+), 2 deletions(-)\n> > \n> > diff --git a/drivers/leds/leds-pca955x.c b/drivers/leds/leds-pca955x.c\n> > index 0217bac2f47b..a02c1fd5dee9 100644\n> > --- a/drivers/leds/leds-pca955x.c\n> > +++ b/drivers/leds/leds-pca955x.c\n> > @@ -61,6 +61,9 @@\n> > > > > >  #define PCA955X_LS_BLINK0\t0x2\t/* Blink at PWM0 rate */\n> > > > > >  #define PCA955X_LS_BLINK1\t0x3\t/* Blink at PWM1 rate */\n> >  \n> > > > +#define PCA955X_GPIO_HIGH\tLED_OFF\n> > > > +#define PCA955X_GPIO_LOW\tLED_FULL\n> > +\n> >  enum pca955x_type {\n> > > >  \tpca9550,\n> > > >  \tpca9551,\n> > @@ -292,9 +295,9 @@ static void pca955x_gpio_set_value(struct gpio_chip *gc, unsigned int offset,\n> > > >  \tstruct pca955x_led *led = &pca955x->leds[offset];\n> >  \n> > > >  \tif (val)\n> > > > -\t\tpca955x_led_set(&led->led_cdev, LED_FULL);\n> > > > +\t\tpca955x_led_set(&led->led_cdev, PCA955X_GPIO_HIGH);\n> > > >  \telse\n> > > > -\t\tpca955x_led_set(&led->led_cdev, LED_OFF);\n> > > > +\t\tpca955x_led_set(&led->led_cdev, PCA955X_GPIO_LOW);\n> >  }\n> >  \n> >  static int pca955x_gpio_get_value(struct gpio_chip *gc, unsigned int offset)\n> > \n> \n>","headers":{"Return-Path":"","X-Original-To":["incoming@patchwork.ozlabs.org","openbmc@lists.ozlabs.org"],"Delivered-To":["patchwork-incoming@bilbo.ozlabs.org","openbmc@lists.ozlabs.org"],"Received":["from lists.ozlabs.org (lists.ozlabs.org [IPv6:2401:3900:2:1::3])\n\t(using TLSv1.2 with cipher ADH-AES256-GCM-SHA384 (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xgg3k49kHz9s81\n\tfor ;\n\tMon, 28 Aug 2017 15:16:34 +1000 (AEST)","from lists.ozlabs.org (lists.ozlabs.org [IPv6:2401:3900:2:1::3])\n\tby lists.ozlabs.org (Postfix) with ESMTP id 3xgg3k2yCpzDqgf\n\tfor ;\n\tMon, 28 Aug 2017 15:16:34 +1000 (AEST)","from out2-smtp.messagingengine.com (out2-smtp.messagingengine.com\n\t[66.111.4.26])\n\t(using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby lists.ozlabs.org (Postfix) with ESMTPS id 3xgg3V0cy5zDqZ0\n\tfor ; Mon, 28 Aug 2017 15:16:21 +1000 (AEST)","from compute4.internal (compute4.nyi.internal [10.202.2.44])\n\tby mailout.nyi.internal (Postfix) with ESMTP id AA12820DF0;\n\tMon, 28 Aug 2017 01:16:19 -0400 (EDT)","from frontend2 ([10.202.2.161])\n\tby compute4.internal (MEProxy); Mon, 28 Aug 2017 01:16:19 -0400","from keelia (ppp118-210-176-216.bras2.adl6.internode.on.net\n\t[118.210.176.216])\n\tby mail.messagingengine.com (Postfix) with ESMTPA id CDF67248E6;\n\tMon, 28 Aug 2017 01:16:16 -0400 (EDT)"],"Authentication-Results":["ozlabs.org;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=aj.id.au header.i=@aj.id.au header.b=\"YXcZJ1z0\";\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=messagingengine.com\n\theader.i=@messagingengine.com header.b=\"rJM+euGr\"; \n\tdkim-atps=neutral","lists.ozlabs.org;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=aj.id.au header.i=@aj.id.au header.b=\"YXcZJ1z0\";\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=messagingengine.com\n\theader.i=@messagingengine.com header.b=\"rJM+euGr\"; \n\tdkim-atps=neutral","lists.ozlabs.org; dkim=pass (2048-bit key;\n\tunprotected) header.d=aj.id.au header.i=@aj.id.au header.b=\"YXcZJ1z0\";\n\tdkim=pass (2048-bit key;\n\tunprotected) header.d=messagingengine.com\n\theader.i=@messagingengine.com\n\theader.b=\"rJM+euGr\"; dkim-atps=neutral"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/relaxed; d=aj.id.au; h=cc\n\t:content-type:date:from:in-reply-to:message-id:mime-version\n\t:references:subject:to:x-me-sender:x-me-sender:x-sasl-enc\n\t:x-sasl-enc; s=fm1; bh=/JWyY79hZLNJdXJY/ROIkD+oqaSsIDl8u8sydzB8A\n\tug=; b=YXcZJ1z0kjVSqRPXj2UOKuV5X3P6QtMKeFPpFjPrhOJfrZewDecTAxAAs\n\tGnKLqFphEeiGT8E1pQaL8depQ3RUq4OQ0U+kuqXrkt3KUT33L7ojpa5i99hObtSW\n\tdzreXaZ81rajw09/epdSzglLtdsxnjciDpIzznf6sDAtKW5jqcAVcbH1sObIfI/o\n\t1YHnkEQDk7hU6HCM7afiTmj/iflE7tu4QYcNA4FetIYCpdmTIRH/KnrYXSWv9lwH\n\tZwc8kiP43Z3T1e0g1z8ATVQJdXnc/EgFTs/BAdwTFV0XPg49uuUMqDuKPvSFQ7aM\n\tn/WDILNZah9rT5JiEAT7vOovzsPkQ==","v=1; a=rsa-sha256; c=relaxed/relaxed; d=\n\tmessagingengine.com; h=cc:content-type:date:from:in-reply-to\n\t:message-id:mime-version:references:subject:to:x-me-sender\n\t:x-me-sender:x-sasl-enc:x-sasl-enc; s=fm1; bh=/JWyY79hZLNJdXJY/R\n\tOIkD+oqaSsIDl8u8sydzB8Aug=; b=rJM+euGr0f//tZy3McEVG6MjaQ2ezKQcyH\n\tskxTbq1JUubdBvN7JacE8Eh5ftD7QmdMnioyshMUOH1o17Apzl1dqji7UWvxleoR\n\tNXHh0PeV8vjwm+whvhveNJEpMahio6jUsKle7TRIp83bm8q75X3LYC00dIp7mrN8\n\tofpw+eKeFaebPGDGCZ7gnWHvirulgHjwIDEcOpSow0/Fa7al9DVF7AZLVXt1jV9v\n\tfxEg97N75AW2KxweftJ52x17PFQOGWb2I4rBjqk8zHxbZM5e3f9F5cWNC38tsWfF\n\trw/uXLToSTlyXtqEb7v3F//rie7Tg/ugkFxahh6yeEQfTDRJ60UQ=="],"X-ME-Sender":"","X-Sasl-enc":"C1sahnY8R/LgcoAGSRyiLVx6P8EjZ+pO3byP1fcGbTM5 1503897378","Message-ID":"<1503897371.32695.16.camel@aj.id.au>","Subject":"Re: [PATCH linux dev-4.10 3/5] leds: pca955x: Don't invert\n\trequested value in pca955x_gpio_set_value()","From":"Andrew Jeffery ","To":"=?iso-8859-1?q?C=E9dric?= Le Goater , joel@jms.id.au","Date":"Mon, 28 Aug 2017 14:46:11 +0930","In-Reply-To":"<503ada3f-cabe-0f73-1907-f1b62470c5ed@kaod.org>","References":"<20170825065244.488-1-andrew@aj.id.au>\n\t<20170825065244.488-4-andrew@aj.id.au>\n\t<503ada3f-cabe-0f73-1907-f1b62470c5ed@kaod.org>","Content-Type":"multipart/signed; micalg=\"pgp-sha512\";\n\tprotocol=\"application/pgp-signature\";\n\tboundary=\"=-y/hSXVuRQoTbm5Ypiqxp\"","X-Mailer":"Evolution 3.22.6-1ubuntu1 ","Mime-Version":"1.0","X-BeenThere":"openbmc@lists.ozlabs.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"Development list for OpenBMC ","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"openbmc@lists.ozlabs.org, bjwyman@gmail.com, eajames@linux.vnet.ibm.com","Errors-To":"openbmc-bounces+incoming=patchwork.ozlabs.org@lists.ozlabs.org","Sender":"\"openbmc\"\n\t"}},{"id":1758874,"web_url":"http://patchwork.ozlabs.org/comment/1758874/","msgid":"","list_archive_url":null,"date":"2017-08-28T21:16:18","subject":"Re: [PATCH linux dev-4.10 3/5] leds: pca955x: Don't invert requested\n\tvalue in pca955x_gpio_set_value()","submitter":{"id":71862,"url":"http://patchwork.ozlabs.org/api/people/71862/","name":"Brandon Wyman","email":"bjwyman@gmail.com"},"content":"On Mon, Aug 28, 2017 at 12:16 AM, Andrew Jeffery wrote:\n> On Sun, 2017-08-27 at 09:03 +0200, Cédric Le Goater wrote:\n>> On 08/25/2017 08:52 AM, Andrew Jeffery wrote:\n>> > The PCA9552 lines can be used either for driving LEDs or as GPIOs. The\n>> > manual states that for LEDs, the operation is open-drain:\n>> >\n>> > The LSn LED select registers determine the source of the LED data.\n>> >\n>> > 00 = output is set LOW (LED on)\n>> > 01 = output is set high-impedance (LED off; default)\n>> > 10 = output blinks at PWM0 rate\n>> > 11 = output blinks at PWM1 rate\n>> >\n>> > For GPIOs it suggests a pull-up so that the open-case drives the line\n>> > high:\n>> >\n>> > For use as output, connect external pull-up resistor to the pin\n>> > and size it according to the DC recommended operating\n>> > characteristics. LED output pin is HIGH when the output is\n>> > programmed as high-impedance, and LOW when the output is\n>> > programmed LOW through the ‘LED selector’ register. The output\n>> > can be pulse-width controlled when PWM0 or PWM1 are used.\n>> >\n>> > Now, I have a hardware design that uses the LED controller to control\n>> > LEDs. However, for $reasons, we're using the leds-gpio driver to drive\n>> > the LEDs which means wending our way through the gpiochip abstractions.\n>> > So with that in mind we need to describe an active-low GPIO\n>> > configuration to drive the LEDs as though they were GPIOs.\n>> >\n>> > The set() gpiochip callback in leds-pca955x does the following:\n>> >\n>> > ...\n>> > if (val)\n>> > pca955x_led_set(&led->led_cdev, LED_FULL);\n>> > else\n>> > pca955x_led_set(&led->led_cdev, LED_OFF);\n>> > ...\n>> >\n>> > Where LED_FULL = 255. pca955x_led_set() in turn does:\n>> >\n>> > ...\n>> > switch (value) {\n>> > case LED_FULL:\n>> > ls = pca955x_ledsel(ls, ls_led, PCA955X_LS_LED_ON);\n>> > break;\n>> > ...\n>> >\n>> > Where PCA955X_LS_LED_ON is defined as:\n>> >\n>> > > > > > #define PCA955X_LS_LED_ON 0x0 /* Output LOW */\n>> >\n>> > So here we have some type confusion: We've crossed domains from GPIO\n>> > behaviour to LED behaviour without accounting for possible inversions\n>> > in the process.\n>> >\n>> > Stepping back to leds-gpio for a moment, during probe() we call\n>> > create_gpio_led(), which eventually executes:\n>> >\n>> > if (template->default_state == LEDS_GPIO_DEFSTATE_KEEP) {\n>> > state = gpiod_get_value_cansleep(led_dat->gpiod);\n>> > if (state < 0)\n>> > return state;\n>> > } else {\n>> > state = (template->default_state == LEDS_GPIO_DEFSTATE_ON);\n>> > }\n>> > ...\n>> > ret = gpiod_direction_output(led_dat->gpiod, state);\n>> >\n>> > In the devicetree the GPIO is annotated as active-low, and\n>> > gpiod_get_value_cansleep() handles this for us:\n>> >\n>> > int gpiod_get_value_cansleep(const struct gpio_desc *desc)\n>> > {\n>> > int value;\n>> >\n>> > might_sleep_if(extra_checks);\n>> > VALIDATE_DESC(desc);\n>> > value = _gpiod_get_raw_value(desc);\n>> > if (value < 0)\n>> > return value;\n>> >\n>> > if (test_bit(FLAG_ACTIVE_LOW, &desc->flags))\n>> > value = !value;\n>> >\n>> > return value;\n>> > }\n>> >\n>> > _gpiod_get_raw_value() in turn calls through the get() callback for the\n>> > gpiochip implementation, so returning to our get() implementation in\n>> > leds-pca955x we find we extract the raw value from hardware:\n>> >\n>> > static int pca955x_gpio_get_value(struct gpio_chip *gc, unsigned int offset)\n>> > {\n>> > struct pca955x *pca955x = gpiochip_get_data(gc);\n>> > struct pca955x_led *led = &pca955x->leds[offset];\n>> > u8 reg = pca955x_read_input(pca955x->client, led->led_num / 8);\n>> >\n>> > return !!(reg & (1 << (led->led_num % 8)));\n>> > }\n>> >\n>> > This behaviour is not symmetric with that of set(), where the val is\n>> > inverted by the driver.\n>> >\n>> > Closing the loop on the GPIO_ACTIVE_LOW inversions,\n>> > gpiod_direction_output(), like gpiod_get_value_cansleep, handles it for\n>> > us:\n>> >\n>> > int gpiod_direction_output(struct gpio_desc *desc, int value)\n>> > {\n>> > VALIDATE_DESC(desc);\n>> > if (test_bit(FLAG_ACTIVE_LOW, &desc->flags))\n>> > value = !value;\n>> > else\n>> > value = !!value;\n>> > return _gpiod_direction_output_raw(desc, value);\n>> > }\n>> >\n>> > All-in-all, with a value of 'keep' for default-state property in the\n>> > gpio-leds child node, the current state of the hardware will in-fact be\n>> > inverted; precisely the opposite of what was intended.\n>> >\n>> > Rework leds-pca955x so that we avoid the incorrect inversion and clarify\n>> > the semantics with respect to GPIO.\n>>\n>> I suppose this is correct. Have we made some tests on the pins directly\n>> driven as GPIOs ?\n>\n> Well, nothing besides an LED. But is an LED not a satisfactory use-\n> case? It's still a GPIO. As for active-high/active-low, well they're\n> pretty thoroughly tested by the GPIO subsystem.\n>\n> Brandon Wyman has eyeballed the LEDs on a Witherspoon and confirms the\n> problem is resolved (with a caveat)[1].\n>\n> I've rebased on top of the leds tree without issue. I'll send this\n> patch upstream to get some feedback.\n>\n> Cheers,\n>\n> Andrew\n>\n> [1] https://github.com/openbmc/openbmc/issues/2156#issuecomment-325035185\n>\n>>\n>> Thanks,\n>>\n>> C.\n>>\n>> >\n>> > > > Signed-off-by: Andrew Jeffery \n>> > ---\n>> > drivers/leds/leds-pca955x.c | 7 +++++--\n>> > 1 file changed, 5 insertions(+), 2 deletions(-)\n>> >\n>> > diff --git a/drivers/leds/leds-pca955x.c b/drivers/leds/leds-pca955x.c\n>> > index 0217bac2f47b..a02c1fd5dee9 100644\n>> > --- a/drivers/leds/leds-pca955x.c\n>> > +++ b/drivers/leds/leds-pca955x.c\n>> > @@ -61,6 +61,9 @@\n>> > > > > > #define PCA955X_LS_BLINK0 0x2 /* Blink at PWM0 rate */\n>> > > > > > #define PCA955X_LS_BLINK1 0x3 /* Blink at PWM1 rate */\n>> >\n>> > > > +#define PCA955X_GPIO_HIGH LED_OFF\n>> > > > +#define PCA955X_GPIO_LOW LED_FULL\n>> > +\n>> > enum pca955x_type {\n>> > > > pca9550,\n>> > > > pca9551,\n>> > @@ -292,9 +295,9 @@ static void pca955x_gpio_set_value(struct gpio_chip *gc, unsigned int offset,\n>> > > > struct pca955x_led *led = &pca955x->leds[offset];\n>> >\n>> > > > if (val)\n>> > > > - pca955x_led_set(&led->led_cdev, LED_FULL);\n>> > > > + pca955x_led_set(&led->led_cdev, PCA955X_GPIO_HIGH);\n>> > > > else\n>> > > > - pca955x_led_set(&led->led_cdev, LED_OFF);\n>> > > > + pca955x_led_set(&led->led_cdev, PCA955X_GPIO_LOW);\n>> > }\n>> >\n>> > static int pca955x_gpio_get_value(struct gpio_chip *gc, unsigned int offset)\n>> >\n>>\n>>\n\nTested-by: Brandon Wyman ","headers":{"Return-Path":"","X-Original-To":["incoming@patchwork.ozlabs.org","openbmc@lists.ozlabs.org"],"Delivered-To":["patchwork-incoming@bilbo.ozlabs.org","openbmc@lists.ozlabs.org"],"Received":["from lists.ozlabs.org (lists.ozlabs.org [103.22.144.68])\n\t(using TLSv1.2 with cipher ADH-AES256-GCM-SHA384 (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xh5FL61j5z9rxl\n\tfor ;\n\tTue, 29 Aug 2017 07:56:22 +1000 (AEST)","from lists.ozlabs.org (lists.ozlabs.org [IPv6:2401:3900:2:1::3])\n\tby lists.ozlabs.org (Postfix) with ESMTP id 3xh5FL41wYzDqFw\n\tfor ;\n\tTue, 29 Aug 2017 07:56:22 +1000 (AEST)","from mail-qt0-x244.google.com (mail-qt0-x244.google.com\n\t[IPv6:2607:f8b0:400d:c0d::244])\n\t(using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128\n\tbits)) (No client certificate requested)\n\tby lists.ozlabs.org (Postfix) with ESMTPS id 3xh4NT5JrpzDqD0\n\tfor ; Tue, 29 Aug 2017 07:16:21 +1000 (AEST)","by mail-qt0-x244.google.com with SMTP id e2so1481357qta.3\n\tfor ; Mon, 28 Aug 2017 14:16:21 -0700 (PDT)","by 10.140.82.232 with HTTP; Mon, 28 Aug 2017 14:16:18 -0700 (PDT)"],"Authentication-Results":["ozlabs.org;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"G32c1X+o\"; dkim-atps=neutral","lists.ozlabs.org;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"G32c1X+o\"; dkim-atps=neutral","lists.ozlabs.org; dkim=pass (2048-bit key;\n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"G32c1X+o\"; dkim-atps=neutral"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025;\n\th=mime-version:in-reply-to:references:from:date:message-id:subject:to\n\t:cc:content-transfer-encoding;\n\tbh=HsK+yYFu4rrRZed77mAtJUf4iDtqA+P5e+cmSroyvrU=;\n\tb=G32c1X+o1fLfPJU8iuzFjprnPDzk/LczQ+9N3zvZCOlZHVAfdeW6HA9grE5jsC0u6+\n\tEnFTTbRJhCFHM3SXWeyVNmJ2MrKcXhakkj/PcqnWdKz7DdzS23a0ezyRamb2ZcEmRpWN\n\tr2MY4SGGY7j+6lFjiz2t2+ctNddPAjwyRqr//DWxX/o65k/pIAQTeQqSuQ+1PbrgtcAI\n\tlRWUzQHFZmmHpVxeudi4SI+Vf6T4HEo9RiViFHKFyxccJiIVFhU0q+/4CzOmKVxWJmK+\n\tMk2Je+RLjJ9eSsqCRGeopllTrfJ77WOaz8JwZl+VZfLXPoITjJ9cvc2ncwGfq1njeYa2\n\tsybg==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:in-reply-to:references:from:date\n\t:message-id:subject:to:cc:content-transfer-encoding;\n\tbh=HsK+yYFu4rrRZed77mAtJUf4iDtqA+P5e+cmSroyvrU=;\n\tb=SehxwDn1qfV3BZ14wRe1WJ3IrMzT1yniHLDhIs9kHYEmpZdIptaUMfPcUyFWBs4NJ2\n\tRAYRxYZrTEALhpfbehS4XsNRvZY6Ra+9vDdTMqp2d86NxumM003B0PquQab+DLYd/LAa\n\tCtSrs2XrzotdciNZdGGsOpEpr/PcJ2kS+DIkChaf2cyS5gkcqtoCE9iNyrCZyTRizWbb\n\tsTGXJkXH3+mW5CSM85k89j+ReQsGwEOD8xjvQglu3sCZU0E30jLcaP2nXDolQDu9t0t7\n\tpQXbg1rvwRcHi+QZE7Br+2Xl8sOeroxt/m+1apxU5xkjv8PHSAKw4QwQC78wWBAay/im\n\tu7zA==","X-Gm-Message-State":"AHYfb5iVZjmBL6+qP1mgo3TYYum6Zx9vhd+lvoEJlZ5bkMLbvMkkHZH+\n\tBkjKiz5vtkEKhGjHfhwfCqcsGeebRw==","X-Received":"by 10.200.55.5 with SMTP id o5mr2964251qtb.228.1503954978655;\n\tMon, 28 Aug 2017 14:16:18 -0700 (PDT)","MIME-Version":"1.0","In-Reply-To":"<1503897371.32695.16.camel@aj.id.au>","References":"<20170825065244.488-1-andrew@aj.id.au>\n\t<20170825065244.488-4-andrew@aj.id.au>\n\t<503ada3f-cabe-0f73-1907-f1b62470c5ed@kaod.org>\n\t<1503897371.32695.16.camel@aj.id.au>","From":"Brandon Wyman ","Date":"Mon, 28 Aug 2017 16:16:18 -0500","Message-ID":"","Subject":"Re: [PATCH linux dev-4.10 3/5] leds: pca955x: Don't invert requested\n\tvalue in pca955x_gpio_set_value()","To":"Andrew Jeffery ","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","X-BeenThere":"openbmc@lists.ozlabs.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"Development list for OpenBMC ","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"OpenBMC Maillist , Eddie James\n\t, =?utf-8?q?C=C3=A9dric_Le_Goater?=\n\t","Errors-To":"openbmc-bounces+incoming=patchwork.ozlabs.org@lists.ozlabs.org","Sender":"\"openbmc\"\n\t"}}]