Patch Detail
get:
Show a patch.
patch:
Update a patch.
put:
Update a patch.
GET /api/patches/806369/?format=api
{ "id": 806369, "url": "http://patchwork.ozlabs.org/api/patches/806369/?format=api", "web_url": "http://patchwork.ozlabs.org/project/openbmc/patch/20170828054559.16654-1-andrew@aj.id.au/", "project": { "id": 56, "url": "http://patchwork.ozlabs.org/api/projects/56/?format=api", "name": "OpenBMC development", "link_name": "openbmc", "list_id": "openbmc.lists.ozlabs.org", "list_email": "openbmc@lists.ozlabs.org", "web_url": "http://github.com/openbmc/", "scm_url": "", "webscm_url": "", "list_archive_url": "", "list_archive_url_format": "", "commit_url_format": "" }, "msgid": "<20170828054559.16654-1-andrew@aj.id.au>", "list_archive_url": null, "date": "2017-08-28T05:45:59", "name": "leds: pca955x: Don't invert requested value in pca955x_gpio_set_value()", "commit_ref": null, "pull_url": null, "state": "not-applicable", "archived": true, "hash": "863b23ec122ec74cab03e87c81853cd6a43cfa20", "submitter": { "id": 68332, "url": "http://patchwork.ozlabs.org/api/people/68332/?format=api", "name": "Andrew Jeffery", "email": "andrew@aj.id.au" }, "delegate": null, "mbox": "http://patchwork.ozlabs.org/project/openbmc/patch/20170828054559.16654-1-andrew@aj.id.au/mbox/", "series": [ { "id": 74, "url": "http://patchwork.ozlabs.org/api/series/74/?format=api", "web_url": "http://patchwork.ozlabs.org/project/openbmc/list/?series=74", "date": "2017-08-28T05:45:59", "name": "leds: pca955x: Don't invert requested value in pca955x_gpio_set_value()", "version": 1, "mbox": "http://patchwork.ozlabs.org/series/74/mbox/" } ], "comments": "http://patchwork.ozlabs.org/api/patches/806369/comments/", "check": "pending", "checks": "http://patchwork.ozlabs.org/api/patches/806369/checks/", "tags": {}, "related": [], "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 3xggk55ZDLz9sP3\n\tfor <incoming@patchwork.ozlabs.org>;\n\tMon, 28 Aug 2017 15:46:21 +1000 (AEST)", "from lists.ozlabs.org (lists.ozlabs.org [IPv6:2401:3900:2:1::3])\n\tby lists.ozlabs.org (Postfix) with ESMTP id 3xggk54X7YzDqMM\n\tfor <incoming@patchwork.ozlabs.org>;\n\tMon, 28 Aug 2017 15:46:21 +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 3xggjy53fszDqF0\n\tfor <openbmc@lists.ozlabs.org>; Mon, 28 Aug 2017 15:46:14 +1000 (AEST)", "from compute4.internal (compute4.nyi.internal [10.202.2.44])\n\tby mailout.nyi.internal (Postfix) with ESMTP id A3834210DC;\n\tMon, 28 Aug 2017 01:46:11 -0400 (EDT)", "from frontend2 ([10.202.2.161])\n\tby compute4.internal (MEProxy); Mon, 28 Aug 2017 01:46:11 -0400", "from keelia.aj.id.au\n\t(ppp118-210-176-216.bras2.adl6.internode.on.net [118.210.176.216])\n\tby mail.messagingengine.com (Postfix) with ESMTPA id 2117A24605;\n\tMon, 28 Aug 2017 01:46:07 -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=\"nv/6PIY7\";\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=messagingengine.com\n\theader.i=@messagingengine.com header.b=\"CbUUUkwY\"; \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=\"nv/6PIY7\";\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=messagingengine.com\n\theader.i=@messagingengine.com header.b=\"CbUUUkwY\"; \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=\"nv/6PIY7\";\n\tdkim=pass (2048-bit key;\n\tunprotected) header.d=messagingengine.com\n\theader.i=@messagingengine.com\n\theader.b=\"CbUUUkwY\"; dkim-atps=neutral" ], "DKIM-Signature": [ "v=1; a=rsa-sha256; c=relaxed/relaxed; d=aj.id.au; h=cc\n\t:content-transfer-encoding:content-type:date:from:message-id\n\t:mime-version:subject:to:x-me-sender:x-me-sender:x-sasl-enc\n\t:x-sasl-enc; s=fm1; bh=XYRzJ5bfS3XZdTVW5SbLHwh9NPkOc7TkjtxOdz6Va\n\tw8=; b=nv/6PIY7gmEK3RV1aZUZk1p5Xv+nbqwS3BO+l8UCUUKWpsAbDWCoCiRMs\n\t6uAi5RZyyvN1TQtbJGyWrF6mTpRGHwcwbifldalVeMZ+hE016c97PPvNMeofWK3n\n\tXR8Sr1E4M5cA/30jIM80CjMPB9GrtDTHvhNAdeoLlVaSD2uONtsXM72RfYXe8hb9\n\tlOLtdWcOnYwjoiDu4thRWQ5xUGGJYv4gaZpxc7tfrnnYvD3A+5d2X1wZ0X6TmxWQ\n\ty+4tPCp2BZijaYNlXIGS7OkFmeuj2HL9EukcenwbU2vXIe/LFSjgY6tE+NdPK443\n\trbOB21X8kLSs48Uq8g75VPeAsGBuA==", "v=1; a=rsa-sha256; c=relaxed/relaxed; d=\n\tmessagingengine.com; h=cc:content-transfer-encoding:content-type\n\t:date:from:message-id:mime-version:subject:to:x-me-sender\n\t:x-me-sender:x-sasl-enc:x-sasl-enc; s=fm1; bh=XYRzJ5bfS3XZdTVW5S\n\tbLHwh9NPkOc7TkjtxOdz6Vaw8=; b=CbUUUkwY1ne/J2bAsjCWO+s9yLKmHE+LJn\n\tL/1MgJ7psZNwtLseKJaxBG0usJklT26QipZ0OenIuVqzAg2Ud8U8mhCV4OSFrb2S\n\tTvr3Tp9HkUz8B5jhjR73FL1n/Smsx1Fp7RlIsxnRtq7T8OnIWSwpqjtXyPnvLC1x\n\t1ZiCXc/J4zzXGAkYDYOPzceq/rrs/nwwitI+xNRuIF2s0C/4S2V71p3+6Jk8EKPy\n\tz/SH0hj4mySVNI/1ZT1CValKmUU/KQdBmuFp89XdBZs7hwQJCIXZl9mCkYNGXpRX\n\tpg7Sfu+YhEGjp1HZtnyRFm7G4WrBzH0Z/jVp6y5gaVO9PGX6uMwg==" ], "X-ME-Sender": "<xms:I66jWQi0QZuXnS2SFIMl3cpfJ5Z3dV_Y_LqzN_uXQy2bFcAwseu4zg>", "X-Sasl-enc": "2iBgbuwM06Z7acIVEFBCX2GDA8jGMalCBTaLQbRJxyPb 1503899170", "From": "Andrew Jeffery <andrew@aj.id.au>", "To": "linux-leds@vger.kernel.org", "Subject": "[PATCH] leds: pca955x: Don't invert requested value in\n\tpca955x_gpio_set_value()", "Date": "Mon, 28 Aug 2017 15:15:59 +0930", "Message-Id": "<20170828054559.16654-1-andrew@aj.id.au>", "X-Mailer": "git-send-email 2.11.0", "MIME-Version": "1.0", "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": "Andrew Jeffery <andrew@aj.id.au>, openbmc@lists.ozlabs.org,\n\tlinux-kernel@vger.kernel.org, rpurdie@rpsys.net, clg@kaod.org,\n\tpavel@ucw.cz, bjwyman@gmail.com, jacek.anaszewski@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>" }, "content": "The PCA9552 lines can be used either for driving LEDs or as GPIOs. The\nmanual 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\nFor GPIOs it suggests a pull-up so that the open-case drives the line\nhigh:\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\nNow, I have a hardware design that uses the LED controller to control\nLEDs. However, for $reasons, we're using the leds-gpio driver to drive\nthe them. The reasons are here are a tangent but lead to the discovery\nof the inversion, which manifested as the LEDs being set to full\nbrightness at boot when we expected them to be off.\n\nAs we're driving the LEDs through leds-gpio, this means wending our way\nthrough the gpiochip abstractions. So with that in mind we need to\ndescribe an active-low GPIO configuration to drive the LEDs as though\nthey were GPIOs.\n\nThe 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\nWhere 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\nWhere PCA955X_LS_LED_ON is defined as:\n\n #define PCA955X_LS_LED_ON\t0x0\t/* Output LOW */\n\nSo here we have some type confusion: We've crossed domains from GPIO\nbehaviour to LED behaviour without accounting for possible inversions\nin the process.\n\nStepping back to leds-gpio for a moment, during probe() we call\ncreate_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\nIn the devicetree the GPIO is annotated as active-low, and\ngpiod_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\ngpiochip implementation, so returning to our get() implementation in\nleds-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\nThis behaviour is not symmetric with that of set(), where the val is\ninverted by the driver.\n\nClosing the loop on the GPIO_ACTIVE_LOW inversions,\ngpiod_direction_output(), like gpiod_get_value_cansleep(), handles it\nfor 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\nAll-in-all, with a value of 'keep' for default-state property in a\nleds-gpio child node, the current state of the hardware will in-fact be\ninverted; precisely the opposite of what was intended.\n\nRework leds-pca955x so that we avoid the incorrect inversion and clarify\nthe semantics with respect to GPIO.\n\nSigned-off-by: Andrew Jeffery <andrew@aj.id.au>\n---\n drivers/leds/leds-pca955x.c | 7 +++++--\n 1 file changed, 5 insertions(+), 2 deletions(-)", "diff": "diff --git a/drivers/leds/leds-pca955x.c b/drivers/leds/leds-pca955x.c\nindex 09303fd1fdc6..8b8f81cf10cb 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", "prefixes": [] }