[{"id":1761850,"web_url":"http://patchwork.ozlabs.org/comment/1761850/","msgid":"<f07313a1-c609-f760-9578-4d6889fd89d9@kaod.org>","list_archive_url":null,"date":"2017-09-01T17:06:30","subject":"Re: [PATCH v2] leds: pca955x: Don't invert requested value in\n\tpca955x_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 09/01/2017 07:38 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 them. The reasons are here are a tangent but lead to the discovery\n> of the inversion, which manifested as the LEDs being set to full\n> brightness at boot when we expected them to be off.\n> \n> As we're driving the LEDs through leds-gpio, this means wending our way\n> through the gpiochip abstractions. So with that in mind we need to\n> describe an active-low GPIO configuration to drive the LEDs as though\n> 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\n> for 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 a\n> leds-gpio 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> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>\n\nReviewed-by: Cédric Le Goater <clg@kaod.org>\n\nThanks for digging into the code, that was a lot of inversions.\n\nC.\n\n> ---\n> \n> I've rebased on top of \"1591caf2d5ea leds: pca955x: check for I2C errors\" and\n> resolved the conflicts.\n> \n>  drivers/leds/leds-pca955x.c | 9 ++++++---\n>  1 file changed, 6 insertions(+), 3 deletions(-)\n> \n> diff --git a/drivers/leds/leds-pca955x.c b/drivers/leds/leds-pca955x.c\n> index 905729191d3e..6dcd2d7cc6a4 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> @@ -329,9 +332,9 @@ static int pca955x_set_value(struct gpio_chip *gc, unsigned int offset,\n>  \tstruct pca955x_led *led = &pca955x->leds[offset];\n>  \n>  \tif (val)\n> -\t\treturn pca955x_led_set(&led->led_cdev, LED_FULL);\n> -\telse\n> -\t\treturn pca955x_led_set(&led->led_cdev, LED_OFF);\n> +\t\treturn pca955x_led_set(&led->led_cdev, PCA955X_GPIO_HIGH);\n> +\n> +\treturn pca955x_led_set(&led->led_cdev, PCA955X_GPIO_LOW);\n>  }\n>  \n>  static void pca955x_gpio_set_value(struct gpio_chip *gc, unsigned int offset,\n>","headers":{"Return-Path":"<openbmc-bounces+incoming=patchwork.ozlabs.org@lists.ozlabs.org>","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 3xkQrq3hMCz9t2x\n\tfor <incoming@patchwork.ozlabs.org>;\n\tSat,  2 Sep 2017 03:16:43 +1000 (AEST)","from lists.ozlabs.org (lists.ozlabs.org [IPv6:2401:3900:2:1::3])\n\tby lists.ozlabs.org (Postfix) with ESMTP id 3xkQrm5jz6zDqkQ\n\tfor <incoming@patchwork.ozlabs.org>;\n\tSat,  2 Sep 2017 03:16:40 +1000 (AEST)","from 6.mo68.mail-out.ovh.net (6.mo68.mail-out.ovh.net\n\t[46.105.63.100])\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 3xkQrS6ytbzDqhL\n\tfor <openbmc@lists.ozlabs.org>; Sat,  2 Sep 2017 03:16:23 +1000 (AEST)","from player776.ha.ovh.net (b6.ovh.net [213.186.33.56])\n\tby mo68.mail-out.ovh.net (Postfix) with ESMTP id 4681974E03\n\tfor <openbmc@lists.ozlabs.org>; Fri,  1 Sep 2017 19:06:45 +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 player776.ha.ovh.net (Postfix) with ESMTPSA id 4AFC8400084;\n\tFri,  1 Sep 2017 19:06:35 +0200 (CEST)"],"Subject":"Re: [PATCH v2] leds: pca955x: Don't invert requested value in\n\tpca955x_gpio_set_value()","To":"Andrew Jeffery <andrew@aj.id.au>, linux-leds@vger.kernel.org","References":"<20170901053858.10070-1-andrew@aj.id.au>","From":"=?utf-8?q?C=C3=A9dric_Le_Goater?= <clg@kaod.org>","Message-ID":"<f07313a1-c609-f760-9578-4d6889fd89d9@kaod.org>","Date":"Fri, 1 Sep 2017 19:06:30 +0200","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.3.0","MIME-Version":"1.0","In-Reply-To":"<20170901053858.10070-1-andrew@aj.id.au>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-US","Content-Transfer-Encoding":"8bit","X-Ovh-Tracer-Id":"10638628221295037186","X-VR-SPAMSTATE":"OK","X-VR-SPAMSCORE":"-100","X-VR-SPAMCAUSE":"gggruggvucftvghtrhhoucdtuddrfeelledrvddtgdeludcutefuodetggdotefrodftvfcurfhrohhfihhlvgemucfqggfjpdevjffgvefmvefgnecuuegrihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmd","X-BeenThere":"openbmc@lists.ozlabs.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"Development list for OpenBMC <openbmc.lists.ozlabs.org>","List-Unsubscribe":"<https://lists.ozlabs.org/options/openbmc>,\n\t<mailto:openbmc-request@lists.ozlabs.org?subject=unsubscribe>","List-Archive":"<http://lists.ozlabs.org/pipermail/openbmc/>","List-Post":"<mailto:openbmc@lists.ozlabs.org>","List-Help":"<mailto:openbmc-request@lists.ozlabs.org?subject=help>","List-Subscribe":"<https://lists.ozlabs.org/listinfo/openbmc>,\n\t<mailto:openbmc-request@lists.ozlabs.org?subject=subscribe>","Cc":"openbmc@lists.ozlabs.org, linux-kernel@vger.kernel.org, rpurdie@rpsys.net,\n\tjacek.anaszewski@gmail.com, pavel@ucw.cz, bjwyman@gmail.com","Errors-To":"openbmc-bounces+incoming=patchwork.ozlabs.org@lists.ozlabs.org","Sender":"\"openbmc\"\n\t<openbmc-bounces+incoming=patchwork.ozlabs.org@lists.ozlabs.org>"}},{"id":1765093,"web_url":"http://patchwork.ozlabs.org/comment/1765093/","msgid":"<CACPK8XeoZknwJ8VqSgvbzgWVBgCxiB2GB6n--LXEUVDodND6DQ@mail.gmail.com>","list_archive_url":null,"date":"2017-09-08T05:53:11","subject":"Re: [PATCH v2] leds: pca955x: Don't invert requested value in\n\tpca955x_gpio_set_value()","submitter":{"id":48628,"url":"http://patchwork.ozlabs.org/api/people/48628/","name":"Joel Stanley","email":"joel@jms.id.au"},"content":"On Sat, Sep 2, 2017 at 2:36 AM, Cédric Le Goater <clg@kaod.org> wrote:\n> On 09/01/2017 07:38 AM, Andrew Jeffery wrote:\n\n>> Rework leds-pca955x so that we avoid the incorrect inversion and clarify\n>> the semantics with respect to GPIO.\n>>\n>> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>\n>\n> Reviewed-by: Cédric Le Goater <clg@kaod.org>\n\nI gave this a spin on our 'witherspoon' system and the LEDs responded\nas expected.\n\nTested-by: Joel Stanley <joel@jms.id.au>\n\nCheers,\n\nJoel","headers":{"Return-Path":"<openbmc-bounces+incoming=patchwork.ozlabs.org@lists.ozlabs.org>","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 3xpRQk2wNFz9sBZ\n\tfor <incoming@patchwork.ozlabs.org>;\n\tFri,  8 Sep 2017 15:56:30 +1000 (AEST)","from lists.ozlabs.org (lists.ozlabs.org [IPv6:2401:3900:2:1::3])\n\tby lists.ozlabs.org (Postfix) with ESMTP id 3xpRQk1RRkzDrYl\n\tfor <incoming@patchwork.ozlabs.org>;\n\tFri,  8 Sep 2017 15:56:30 +1000 (AEST)","from mail-lf0-x22d.google.com (mail-lf0-x22d.google.com\n\t[IPv6:2a00:1450:4010:c07::22d])\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 3xpRMN3GvLzDrVj\n\tfor <openbmc@lists.ozlabs.org>; Fri,  8 Sep 2017 15:53:35 +1000 (AEST)","by mail-lf0-x22d.google.com with SMTP id c80so3393104lfh.0\n\tfor <openbmc@lists.ozlabs.org>; Thu, 07 Sep 2017 22:53:35 -0700 (PDT)","by 10.25.79.70 with HTTP; Thu, 7 Sep 2017 22:53:11 -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=\"SUvQsEZT\"; 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=\"SUvQsEZT\"; dkim-atps=neutral","ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=gmail.com\n\t(client-ip=2a00:1450:4010:c07::22d; helo=mail-lf0-x22d.google.com;\n\tenvelope-from=joel.stan@gmail.com; receiver=<UNKNOWN>)","lists.ozlabs.org; dkim=pass (2048-bit key;\n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"SUvQsEZT\"; dkim-atps=neutral"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025;\n\th=mime-version:sender:in-reply-to:references:from:date:message-id\n\t:subject:to:cc:content-transfer-encoding;\n\tbh=f5YaPQT+TAW7p22vO4HnU3s8e9m37AX02RMYzEmkNig=;\n\tb=SUvQsEZT4FdsOOKzdbzXc5Ti55q7qT/iani2alVzI2duRN3F7Jc+UTmMi40Z/obzKr\n\thG6xf2n2FNSyWJlzZ4N3V2XdBGwhszteNnaOfv5qv9DCnDajxn5gfQbgdvy5N2BJo7Lv\n\t0giSetYiwcZEwSe55PaeUCMiDe3KsZGCt2iF8nDUtSL/GqKZpg0uAm2S5/YESglqV5Su\n\tsSTIUVV4FH/t3qn2yYqBPuNw8n2wDgjc5RSS/dir7zcr/CPeWrj9VFgFHYMzGjfnc4Wz\n\tvnGBWflUdXe0PjRLqW0Bjn8vaVNGKpDl0SqSk7gy9Qzvlq/1kDfcbYCxXNDUYovcrHPL\n\tWwrQ==","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:sender:in-reply-to:references:from\n\t:date:message-id:subject:to:cc:content-transfer-encoding;\n\tbh=f5YaPQT+TAW7p22vO4HnU3s8e9m37AX02RMYzEmkNig=;\n\tb=f5iK4yndNlpO942HvwWTZhvyKcrJjyLTVMfBSJ55UjNtK3ZG3TmT4Mfb/m3mrH3EFV\n\tvvRCmVJGdJaGiVi79CNr8B7Mz8DlVNNiJyUdvAHt7ewA4jp652fXzJtPy4g7zannBNjd\n\tMHc17gvHDh8r7/i2HhIxtOd7my54G7WK1lYWrbzvh1M+eHvuq1Ocsxhquss2zRsmg3Ew\n\t9pjhv5J5Dn2OozezXvzq/7blD7PdecezE1/Cf1jr4kTclw8QQEuAJC2jgjhA0703bEpT\n\tLY8147m1wyKSSmh8NxxXvCN3js2Youdi6hJrNLHFRv/BqXVX1dF6kcfYz4FWltESeqKR\n\t/gvg==","X-Gm-Message-State":"AHPjjUg3wD3Ko6SijCP4GghqqKncUj7ThgJLF40+R+DROcRZK1uihuNJ\n\tnOnNvbzDeiXwS9PYYjRj4I4djioabg==","X-Google-Smtp-Source":"AOwi7QAEvJGdgIQ5vcRpUSlVWzc2+9vrOzhEQfVQQYqOTB03hUwGmSj4g67LMIpUaVXUtlji+xRuMqHv0nm000gyEyM=","X-Received":"by 10.25.209.80 with SMTP id i77mr581182lfg.257.1504850012411;\n\tThu, 07 Sep 2017 22:53:32 -0700 (PDT)","MIME-Version":"1.0","In-Reply-To":"<f07313a1-c609-f760-9578-4d6889fd89d9@kaod.org>","References":"<20170901053858.10070-1-andrew@aj.id.au>\n\t<f07313a1-c609-f760-9578-4d6889fd89d9@kaod.org>","From":"Joel Stanley <joel@jms.id.au>","Date":"Fri, 8 Sep 2017 15:23:11 +0930","X-Google-Sender-Auth":"fX6ZxQ310f9mVfFOvzF13vPf8Xo","Message-ID":"<CACPK8XeoZknwJ8VqSgvbzgWVBgCxiB2GB6n--LXEUVDodND6DQ@mail.gmail.com>","Subject":"Re: [PATCH v2] leds: pca955x: Don't invert requested value in\n\tpca955x_gpio_set_value()","To":"=?utf-8?q?C=C3=A9dric_Le_Goater?= <clg@kaod.org>","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 <openbmc.lists.ozlabs.org>","List-Unsubscribe":"<https://lists.ozlabs.org/options/openbmc>,\n\t<mailto:openbmc-request@lists.ozlabs.org?subject=unsubscribe>","List-Archive":"<http://lists.ozlabs.org/pipermail/openbmc/>","List-Post":"<mailto:openbmc@lists.ozlabs.org>","List-Help":"<mailto:openbmc-request@lists.ozlabs.org?subject=help>","List-Subscribe":"<https://lists.ozlabs.org/listinfo/openbmc>,\n\t<mailto:openbmc-request@lists.ozlabs.org?subject=subscribe>","Cc":"Andrew Jeffery <andrew@aj.id.au>,\n\tOpenBMC Maillist <openbmc@lists.ozlabs.org>,\n\tLinux Kernel Mailing List <linux-kernel@vger.kernel.org>,\n\trpurdie@rpsys.net, jacek.anaszewski@gmail.com, pavel@ucw.cz,\n\tBrandon Wyman <bjwyman@gmail.com>, linux-leds@vger.kernel.org","Errors-To":"openbmc-bounces+incoming=patchwork.ozlabs.org@lists.ozlabs.org","Sender":"\"openbmc\"\n\t<openbmc-bounces+incoming=patchwork.ozlabs.org@lists.ozlabs.org>"}},{"id":1765615,"web_url":"http://patchwork.ozlabs.org/comment/1765615/","msgid":"<9cbbe7d6-027e-6fef-ca3d-bbec25042c96@gmail.com>","list_archive_url":null,"date":"2017-09-08T20:09:37","subject":"Re: [PATCH v2] leds: pca955x: Don't invert requested value in\n\tpca955x_gpio_set_value()","submitter":{"id":66885,"url":"http://patchwork.ozlabs.org/api/people/66885/","name":"Jacek Anaszewski","email":"jacek.anaszewski@gmail.com"},"content":"Hi Andrew,\n\nThanks for the patch.\n\nOn 09/01/2017 07:38 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 them. The reasons are here are a tangent but lead to the discovery\n> of the inversion, which manifested as the LEDs being set to full\n> brightness at boot when we expected them to be off.\n> \n> As we're driving the LEDs through leds-gpio, this means wending our way\n> through the gpiochip abstractions. So with that in mind we need to\n> describe an active-low GPIO configuration to drive the LEDs as though\n> 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\n> for 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 a\n> leds-gpio 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> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>\n> ---\n> \n> I've rebased on top of \"1591caf2d5ea leds: pca955x: check for I2C errors\" and\n> resolved the conflicts.\n> \n>  drivers/leds/leds-pca955x.c | 9 ++++++---\n>  1 file changed, 6 insertions(+), 3 deletions(-)\n> \n> diff --git a/drivers/leds/leds-pca955x.c b/drivers/leds/leds-pca955x.c\n> index 905729191d3e..6dcd2d7cc6a4 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> @@ -329,9 +332,9 @@ static int pca955x_set_value(struct gpio_chip *gc, unsigned int offset,\n>  \tstruct pca955x_led *led = &pca955x->leds[offset];\n>  \n>  \tif (val)\n> -\t\treturn pca955x_led_set(&led->led_cdev, LED_FULL);\n> -\telse\n> -\t\treturn pca955x_led_set(&led->led_cdev, LED_OFF);\n> +\t\treturn pca955x_led_set(&led->led_cdev, PCA955X_GPIO_HIGH);\n> +\n> +\treturn pca955x_led_set(&led->led_cdev, PCA955X_GPIO_LOW);\n>  }\n>  \n>  static void pca955x_gpio_set_value(struct gpio_chip *gc, unsigned int offset,\n> \n\nApplied to the for-4.15 branch of linux-leds.git.","headers":{"Return-Path":"<openbmc-bounces+incoming=patchwork.ozlabs.org@lists.ozlabs.org>","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 3xppNR6nkyz9s2G\n\tfor <incoming@patchwork.ozlabs.org>;\n\tSat,  9 Sep 2017 06:10:47 +1000 (AEST)","from lists.ozlabs.org (lists.ozlabs.org [IPv6:2401:3900:2:1::3])\n\tby lists.ozlabs.org (Postfix) with ESMTP id 3xppNR58VBzDrcf\n\tfor <incoming@patchwork.ozlabs.org>;\n\tSat,  9 Sep 2017 06:10:47 +1000 (AEST)","from mail-wr0-x243.google.com (mail-wr0-x243.google.com\n\t[IPv6:2a00:1450:400c:c0c::243])\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 3xppN96Wv0zDrcH\n\tfor <openbmc@lists.ozlabs.org>; Sat,  9 Sep 2017 06:10:33 +1000 (AEST)","by mail-wr0-x243.google.com with SMTP id u48so1668508wrf.4\n\tfor <openbmc@lists.ozlabs.org>; Fri, 08 Sep 2017 13:10:33 -0700 (PDT)","from [192.168.1.18] (dkt172.neoplus.adsl.tpnet.pl. [83.24.23.172])\n\tby smtp.gmail.com with ESMTPSA id\n\tl131sm3832430wmb.2.2017.09.08.13.10.28\n\t(version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128);\n\tFri, 08 Sep 2017 13:10:29 -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=\"EPzBQ0S8\"; 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=\"EPzBQ0S8\"; dkim-atps=neutral","ozlabs.org;\n\tspf=pass (mailfrom) smtp.mailfrom=gmail.com\n\t(client-ip=2a00:1450:400c:c0c::243; helo=mail-wr0-x243.google.com;\n\tenvelope-from=jacek.anaszewski@gmail.com; receiver=<UNKNOWN>)","lists.ozlabs.org; dkim=pass (2048-bit key;\n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"EPzBQ0S8\"; dkim-atps=neutral"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025;\n\th=subject:to:references:cc:from:message-id:date:user-agent\n\t:mime-version:in-reply-to:content-transfer-encoding;\n\tbh=eglMqBfjwqjUB9lWSgldtM2qbljJTrB7oO6kawrMpVg=;\n\tb=EPzBQ0S8ugkcO/0H8O6DPguocdbohqar3WEN+wdMmqvTJ4iDzcmw+hgHTE9oZ0QJFt\n\t4af9sWPQXwWrmefp8ENaFdGbD9vaIib536gCLQGdzXUlK8J9c2o/SZrZ4tOQ4/0sQgoE\n\tV5MhKu6k3QeXK7NvJF5ELxnOq3lfuA52vmV9V3MxNx1/vWsZBO+TJdtMQhEwdt6PBWBh\n\t7AgoNGVCroDgPu7zpBwyPlcu75ygjCCn98NBy4+ZSgMpmuaxTKqgVUEX8g3qN+jlduKI\n\tihzT6FcFJxddRZa8vEA5t5bn4MikZvRi2WyPOsgaa3juv6boANLBjEFN2uLNNLcQmMts\n\tZf/A==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:subject:to:references:cc:from:message-id:date\n\t:user-agent:mime-version:in-reply-to:content-transfer-encoding;\n\tbh=eglMqBfjwqjUB9lWSgldtM2qbljJTrB7oO6kawrMpVg=;\n\tb=i7xruRx9Ca2Eny9E3lwAygrx9qrXBYYI7sRtoJfIgI18eDQlmc4QQ304M1DkqT6C8Y\n\tkZnrDv0pPV0QGvnIm6xUviKMl59fmvEqCkmLfsoJLGMfM5X57TxVqm7M8M2ma9raBBW2\n\tyRvKBsVRSKdHU220yTFnKH3Opn2MnOWTSI84KZdXSuJdA1Reuni7VVwH1qgXmRqqltv3\n\tqmlGgfj9FLFZbgx4zDMlzGMuGfifyP8tv3J0n6tI7AAhrv/uPlM4c2tHDsX9maRD60tL\n\tzqKXmCw6Y1nRCq1/AVxdgmLh8K0cdhdaos8BFK2PdX0KQ/4qu2v9FccB+p1eyUrJedIh\n\tSXYg==","X-Gm-Message-State":"AHPjjUhNcgYzzQkNDqVFNEF6DaMOf1GE/MCu50DpYNXlARN+PTWTQvwD\n\t6dJ2H5kVVCqJ3xbPrtc=","X-Google-Smtp-Source":"ADKCNb4RWbGs3is07S2wTjv9ahkEbkeoTJEOvCUXgIBC/SzSd26Z9eUwTbDcZHijxj68v54e/YZi1A==","X-Received":"by 10.223.178.166 with SMTP id g35mr2930730wrd.262.1504901430183;\n\tFri, 08 Sep 2017 13:10:30 -0700 (PDT)","Subject":"Re: [PATCH v2] leds: pca955x: Don't invert requested value in\n\tpca955x_gpio_set_value()","To":"Andrew Jeffery <andrew@aj.id.au>, linux-leds@vger.kernel.org","References":"<20170901053858.10070-1-andrew@aj.id.au>","From":"Jacek Anaszewski <jacek.anaszewski@gmail.com>","Message-ID":"<9cbbe7d6-027e-6fef-ca3d-bbec25042c96@gmail.com>","Date":"Fri, 8 Sep 2017 22:09:37 +0200","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101\n\tThunderbird/45.8.0","MIME-Version":"1.0","In-Reply-To":"<20170901053858.10070-1-andrew@aj.id.au>","Content-Type":"text/plain; charset=utf-8","Content-Transfer-Encoding":"8bit","X-BeenThere":"openbmc@lists.ozlabs.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"Development list for OpenBMC <openbmc.lists.ozlabs.org>","List-Unsubscribe":"<https://lists.ozlabs.org/options/openbmc>,\n\t<mailto:openbmc-request@lists.ozlabs.org?subject=unsubscribe>","List-Archive":"<http://lists.ozlabs.org/pipermail/openbmc/>","List-Post":"<mailto:openbmc@lists.ozlabs.org>","List-Help":"<mailto:openbmc-request@lists.ozlabs.org?subject=help>","List-Subscribe":"<https://lists.ozlabs.org/listinfo/openbmc>,\n\t<mailto:openbmc-request@lists.ozlabs.org?subject=subscribe>","Cc":"openbmc@lists.ozlabs.org, linux-kernel@vger.kernel.org, rpurdie@rpsys.net,\n\tclg@kaod.org, pavel@ucw.cz, bjwyman@gmail.com","Errors-To":"openbmc-bounces+incoming=patchwork.ozlabs.org@lists.ozlabs.org","Sender":"\"openbmc\"\n\t<openbmc-bounces+incoming=patchwork.ozlabs.org@lists.ozlabs.org>"}}]