From: Stephen Warren <swarren@wwwdotorg.org> To: Dmitry Torokhov <dmitry.torokhov@gmail.com>, linux-input@vger.kernel.org, devicetree-discuss@lists.ozlabs.org, linux-arm-kernel@lists.infradead.org, Chao Xie <xiechao.mail@gmail.com>, Eric Miao <eric.y.miao@gmail.com>, Detlev Zundel <dzu@denx.de>, Sekhar Nori <nsekhar@ti.com>, Marek Vasut <marek.vasut@gmail.com>, Ralf Baechle <ralf@linux-mips.org> Subject: Re: [PATCH v1 01/12] input: matrix-keypad: update devicetree binding doc Date: Mon, 24 Jun 2013 16:00:26 -0600 [thread overview] Message-ID: <51C8C17A.1050700@wwwdotorg.org> (raw) In-Reply-To: <20130622092319.GE24305@book.gsilab.sittig.org> On 06/22/2013 03:23 AM, Gerhard Sittig wrote: ... > On Fri, Jun 21, 2013 at 15:31 -0600, Stephen Warren wrote: >> On 06/21/2013 12:09 PM, Gerhard Sittig wrote: >>> update the device tree binding documentation for the GPIO matrix keypad >>> driver: mention the driver's selecting all columns at once, reword the >>> delay descriptions, add the missing active low GPIO pin level property >> >>> diff --git a/Documentation/devicetree/bindings/input/gpio-matrix-keypad.txt b/Documentation/devicetree/bindings/input/gpio-matrix-keypad.txt ... >>> +Simple keypad matrix layouts just connect GPIO lines to mechanical >>> +switches, with no other active components involved. Although due to the >> >> I don't think that's always true. On some Tegra boards for example, >> multiple processes can "press" certain switches, so we actually have a >> wired-OR feed into a transistor which then connects a particular >> (column, row) combination. We do this e.g. for remote-control USB over >> the power button for example. I believe the effect is the same, but it >> does mean that statement above isn't quite true. > > That's why I wrote about "simple ... layouts", but couldn't tell > exactly when it was appropriate to discuss the more advanced > setups as well, and in how much depth to do that in the device > tree binding. > > So is there a better way of putting this? Is the "simple" or the > "mechanical" the issue in the description? I think both "simple" and "mechanical" should be removed. My reasoning: At the level of connection between rows/columns, aren't all matrix keypads set up such that something connects a row to a column to signify a keypress, not just "simple layouts". Similarly, "mechanical" should be removed since it's not always true, and even if it were, it would be irrelevant to anything outside the keyboard, perhaps aside from the need to debounce. In fact, thinking about this more, I think all four paragraphs of text that this patch adds would be better in Documentation/input/, as you mentioned elsewhere. When introducing any extra properties, you could mention their potential use by a driver, as explanation for why they're useful, but there's probably no need to describe the complete operation of the driver in the DT binding. >>> +- gpio-activelow: row pins as well as column pins are active low >> >> That one change is definitely wrong. Each entry in row-gpios and >> col-gpios should include GPIO flags (in a format defined by the binding >> for the DT node providing the GPIO). Those flags include an active-high >> vs. active-low bit. In Linux, you can use of_get_gpio_flags() to >> retrieve a standardized representation of the flags. > > See my introduction above. This isn't a "change", it's just > catching up in the documentation and adding what was omitted > before. Oh dear, that's quite unfortunate. I see that even though that property wasn't documented in the binding doc, arch/powerpc/boot/dts/ac14xx.dts has still already used it, so we probably can't fix it. I suppose we must simply document it, and ignore the shortcomings of that binding:-( > And I can see another issue here (maybe shadowed by the wording I > used, referring to "row pins"): The fact that a pin may be > inverted (within the SoC), and the fact that an externally > connected component might require low active stimulus over > otherwise high active pins/connections, might need to get > reflected well. Or am I too picky here? > > Are there other cases to learn from, where non-inverted physical > pins get attached to components which require inverted logical > information? Do we combine the overall polarity within the GPIO > pin's abstraction, or do logical drivers above GPIO handle the > inversion? In general, I would expect the binding to represent the overall effective polarity of the signal that must be programmed into the relevant GPIO controller. SW doesn't really care how/why the signal is inverted, simply that it needs to (or doesn't) invert the signal when it programs the GPIO controller.
WARNING: multiple messages have this Message-ID (diff)
From: swarren@wwwdotorg.org (Stephen Warren) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH v1 01/12] input: matrix-keypad: update devicetree binding doc Date: Mon, 24 Jun 2013 16:00:26 -0600 [thread overview] Message-ID: <51C8C17A.1050700@wwwdotorg.org> (raw) In-Reply-To: <20130622092319.GE24305@book.gsilab.sittig.org> On 06/22/2013 03:23 AM, Gerhard Sittig wrote: ... > On Fri, Jun 21, 2013 at 15:31 -0600, Stephen Warren wrote: >> On 06/21/2013 12:09 PM, Gerhard Sittig wrote: >>> update the device tree binding documentation for the GPIO matrix keypad >>> driver: mention the driver's selecting all columns at once, reword the >>> delay descriptions, add the missing active low GPIO pin level property >> >>> diff --git a/Documentation/devicetree/bindings/input/gpio-matrix-keypad.txt b/Documentation/devicetree/bindings/input/gpio-matrix-keypad.txt ... >>> +Simple keypad matrix layouts just connect GPIO lines to mechanical >>> +switches, with no other active components involved. Although due to the >> >> I don't think that's always true. On some Tegra boards for example, >> multiple processes can "press" certain switches, so we actually have a >> wired-OR feed into a transistor which then connects a particular >> (column, row) combination. We do this e.g. for remote-control USB over >> the power button for example. I believe the effect is the same, but it >> does mean that statement above isn't quite true. > > That's why I wrote about "simple ... layouts", but couldn't tell > exactly when it was appropriate to discuss the more advanced > setups as well, and in how much depth to do that in the device > tree binding. > > So is there a better way of putting this? Is the "simple" or the > "mechanical" the issue in the description? I think both "simple" and "mechanical" should be removed. My reasoning: At the level of connection between rows/columns, aren't all matrix keypads set up such that something connects a row to a column to signify a keypress, not just "simple layouts". Similarly, "mechanical" should be removed since it's not always true, and even if it were, it would be irrelevant to anything outside the keyboard, perhaps aside from the need to debounce. In fact, thinking about this more, I think all four paragraphs of text that this patch adds would be better in Documentation/input/, as you mentioned elsewhere. When introducing any extra properties, you could mention their potential use by a driver, as explanation for why they're useful, but there's probably no need to describe the complete operation of the driver in the DT binding. >>> +- gpio-activelow: row pins as well as column pins are active low >> >> That one change is definitely wrong. Each entry in row-gpios and >> col-gpios should include GPIO flags (in a format defined by the binding >> for the DT node providing the GPIO). Those flags include an active-high >> vs. active-low bit. In Linux, you can use of_get_gpio_flags() to >> retrieve a standardized representation of the flags. > > See my introduction above. This isn't a "change", it's just > catching up in the documentation and adding what was omitted > before. Oh dear, that's quite unfortunate. I see that even though that property wasn't documented in the binding doc, arch/powerpc/boot/dts/ac14xx.dts has still already used it, so we probably can't fix it. I suppose we must simply document it, and ignore the shortcomings of that binding:-( > And I can see another issue here (maybe shadowed by the wording I > used, referring to "row pins"): The fact that a pin may be > inverted (within the SoC), and the fact that an externally > connected component might require low active stimulus over > otherwise high active pins/connections, might need to get > reflected well. Or am I too picky here? > > Are there other cases to learn from, where non-inverted physical > pins get attached to components which require inverted logical > information? Do we combine the overall polarity within the GPIO > pin's abstraction, or do logical drivers above GPIO handle the > inversion? In general, I would expect the binding to represent the overall effective polarity of the signal that must be programmed into the relevant GPIO controller. SW doesn't really care how/why the signal is inverted, simply that it needs to (or doesn't) invert the signal when it programs the GPIO controller.
next prev parent reply other threads:[~2013-06-24 22:00 UTC|newest] Thread overview: 90+ messages / expand[flat|nested] mbox.gz Atom feed top 2013-06-21 18:09 [PATCH v1 00/12] input: keypad-matrix: doc update, hw separation, polling, binary columns Gerhard Sittig 2013-06-21 18:09 ` Gerhard Sittig 2013-06-21 18:09 ` [PATCH v1 01/12] input: matrix-keypad: update devicetree binding doc Gerhard Sittig 2013-06-21 18:09 ` Gerhard Sittig 2013-06-21 21:31 ` Stephen Warren 2013-06-21 21:31 ` Stephen Warren 2013-06-22 9:23 ` Gerhard Sittig 2013-06-22 9:23 ` Gerhard Sittig 2013-06-24 22:00 ` Stephen Warren [this message] 2013-06-24 22:00 ` Stephen Warren 2013-06-28 8:24 ` Gerhard Sittig 2013-06-28 8:24 ` Gerhard Sittig 2013-06-28 14:50 ` Stephen Warren 2013-06-28 14:50 ` Stephen Warren 2013-06-30 11:04 ` Gerhard Sittig 2013-06-30 11:04 ` Gerhard Sittig 2013-06-21 18:09 ` [PATCH v1 02/12] input: matrix-keymap: func call coding style nit Gerhard Sittig 2013-06-21 18:09 ` Gerhard Sittig 2013-06-22 2:18 ` Marek Vasut 2013-06-22 2:18 ` Marek Vasut 2013-06-22 8:22 ` Gerhard Sittig 2013-06-22 8:22 ` Gerhard Sittig 2013-06-22 13:23 ` Marek Vasut 2013-06-22 13:23 ` Marek Vasut 2013-06-21 18:09 ` [PATCH v1 03/12] input: matrix-keypad: rename variables and funcs Gerhard Sittig 2013-06-21 18:09 ` Gerhard Sittig 2013-06-21 18:09 ` [PATCH v1 04/12] input: matrix-keypad: push/pull, separate polarity Gerhard Sittig 2013-06-21 18:09 ` Gerhard Sittig 2013-06-21 21:34 ` Stephen Warren 2013-06-21 21:34 ` Stephen Warren 2013-06-22 9:36 ` Gerhard Sittig 2013-06-22 9:36 ` Gerhard Sittig 2013-06-24 23:14 ` Stephen Warren 2013-06-24 23:14 ` Stephen Warren 2013-06-28 8:33 ` Gerhard Sittig 2013-06-28 8:33 ` Gerhard Sittig 2013-06-28 15:01 ` Stephen Warren 2013-06-28 15:01 ` Stephen Warren 2013-06-30 11:43 ` Gerhard Sittig 2013-06-30 11:43 ` Gerhard Sittig 2013-06-21 18:09 ` [PATCH v1 05/12] input: matrix-keypad: update comments, diagnostics Gerhard Sittig 2013-06-21 18:09 ` Gerhard Sittig 2013-06-22 2:23 ` Marek Vasut 2013-06-22 2:23 ` Marek Vasut 2013-06-21 18:09 ` [PATCH v1 06/12] input: keypad-matrix: refactor matrix scan logic Gerhard Sittig 2013-06-21 18:09 ` Gerhard Sittig 2013-06-21 18:09 ` [PATCH v1 07/12] input: keypad-matrix: introduce polling support Gerhard Sittig 2013-06-21 18:09 ` Gerhard Sittig 2013-06-21 21:38 ` Stephen Warren 2013-06-21 21:38 ` Stephen Warren 2013-06-22 9:50 ` Gerhard Sittig 2013-06-22 9:50 ` Gerhard Sittig 2013-06-24 23:18 ` Stephen Warren 2013-06-24 23:18 ` Stephen Warren 2013-06-21 18:09 ` [PATCH v1 08/12] input: keypad-matrix: tell GPIO pins from matrix lines Gerhard Sittig 2013-06-21 18:09 ` Gerhard Sittig 2013-06-21 21:41 ` Stephen Warren 2013-06-21 21:41 ` Stephen Warren 2013-06-22 10:00 ` Gerhard Sittig 2013-06-22 10:00 ` Gerhard Sittig 2013-06-24 23:26 ` Stephen Warren 2013-06-24 23:26 ` Stephen Warren 2013-06-28 7:52 ` Gerhard Sittig 2013-06-28 7:52 ` Gerhard Sittig 2013-06-28 14:35 ` Stephen Warren 2013-06-28 14:35 ` Stephen Warren 2013-06-28 18:25 ` Dmitry Torokhov 2013-06-28 18:25 ` Dmitry Torokhov 2013-06-30 12:03 ` Gerhard Sittig 2013-06-30 12:03 ` Gerhard Sittig 2013-06-21 18:09 ` [PATCH v1 09/12] input: matrix-keypad: add binary column encoding Gerhard Sittig 2013-06-21 18:09 ` Gerhard Sittig 2013-06-21 21:58 ` Stephen Warren 2013-06-21 21:58 ` Stephen Warren 2013-06-21 18:09 ` [PATCH v1 10/12] input: keypad_matrix: use usleep_range() for scan delay Gerhard Sittig 2013-06-21 18:09 ` Gerhard Sittig 2013-06-21 22:00 ` Stephen Warren 2013-06-21 22:00 ` Stephen Warren 2013-06-22 10:17 ` Gerhard Sittig 2013-06-22 10:17 ` Gerhard Sittig 2013-06-24 23:27 ` Stephen Warren 2013-06-24 23:27 ` Stephen Warren 2013-06-21 18:09 ` [PATCH v1 11/12] input: keypad-matrix: AC14xx device tree update Gerhard Sittig 2013-06-21 18:09 ` Gerhard Sittig 2013-06-21 18:09 ` [PATCH v1 12/12] input: matrix-keypad: add diagnostics in probe() Gerhard Sittig 2013-06-21 18:09 ` Gerhard Sittig 2013-06-22 2:28 ` Marek Vasut 2013-06-22 2:28 ` Marek Vasut 2013-06-22 8:30 ` Gerhard Sittig 2013-06-22 8:30 ` Gerhard Sittig
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=51C8C17A.1050700@wwwdotorg.org \ --to=swarren@wwwdotorg.org \ --cc=devicetree-discuss@lists.ozlabs.org \ --cc=dmitry.torokhov@gmail.com \ --cc=dzu@denx.de \ --cc=eric.y.miao@gmail.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-input@vger.kernel.org \ --cc=marek.vasut@gmail.com \ --cc=nsekhar@ti.com \ --cc=ralf@linux-mips.org \ --cc=xiechao.mail@gmail.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.