| Message ID | 1371838198-7327-2-git-send-email-gsi@denx.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 Presumably the changes to this file simply seek to precisely document the HW that this binding supports, and do not intend to change the semantics of the binding at all. If that's the case, then they seem fine to me. Have you checked that all users of this binding do assume that the column GPIOs are output, and rows inputs, rather than the other way around for example? I suppose if the Linux driver is already implemented to assume that, then nobody can be successfully using this binding for HW where that isn't the case, so this change is fine. This change assumes it's OK to activate all columns at once, and presumably that drivers can request the GPIOs as IRQs. This need not be true on all HW. Should an additional property be added to describe whether it's legal to activate all columns at once? For the IRQ case, presumably the driver can simply try to request an IRQ for each GPIO, and fall back to not doing so if that doesn't work. > +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. > +driver's operation of activating all columns at the same time for most Saying "driver" in a DT binding is a slight red flag. The binding should really purely describe HW, rather than SW's use of it. Here, the use is probably generic enough not to be an issue, although it there's some way to reword it to avoid that word it might be good. > +- 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.
Let me first thank you for reviewing the set and for providing so much feedback! Then let me give a little background: The first part of the series with the doc update doesn't introduce anything new into the driver, it just makes the document catch up with what the driver already does. And the later steps of extending the driver and its getting configured by device tree properties was heavily driven by the desire to keep up strict backwards compatibility. Since breaking a means of input and taking away a UNIX user's keyboard isn't good for your health. :) While I can and do test the setup which the extension is targetting at, I cannot test the other setups due to lack of hardware. So when in doubt, I went with small and unintrusive steps and lots of remarks such that the default behaviour is strictly identical to before the change yet the desired new behaviour becomes available upon request, and all of it is documented. 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 > > Presumably the changes to this file simply seek to precisely document > the HW that this binding supports, and do not intend to change the > semantics of the binding at all. If that's the case, then they seem fine > to me. > > Have you checked that all users of this binding do assume that the > column GPIOs are output, and rows inputs, rather than the other way > around for example? I suppose if the Linux driver is already implemented > to assume that, then nobody can be successfully using this binding for > HW where that isn't the case, so this change is fine. I understand your concerns very well, since they crossed my mind too when I wrote that update. One way of keeping the driver's details away from the device tree doc might be to create another file under Documentation/input/, discussing all of the driver's operation and the assumed or supported hardware setups there, and reducing the device tree binding doc to just "mention the fact" that something is adjustable and which property to use. This would in addition reduce duplication with other approaches of configuring the driver (there's static platform data as well). Regarding the layering: I'm aware of the one 'GPIO matrix keypad' driver implemented in drivers/input/keyboard/matrix_keypad.c, which uses a pdata structure which can get filled in by means of static platform data provided with code or by parsing the device tree (the preferred method for future extensions). Other drivers may implement their own logic to scan matrices and detect changes, and share the pdata structure since they have to track similar conditions as well. That's the only reason why I had to touch other files than just keypad and keymap. But none of them are device tree aware AFAIK. So yes, from what I can see, the GPIO matrix keypad driver is the only instance which probes the device tree and implements what the binding describes. Which is why I updated the device tree binding's doc to provide motivation why something might need adjustment and what the consequences are of specifying a parameter. But this only applies if the binding is seen in concert with the Linux driver. When the binding gets used elsewhere in the kernel, or when code outside the kernel implements a binding, then the individual driver's details should be kept out of the binding doc. Unless all drivers implement similar logic in just individual ways, because they all need to drive a keypad matrix and thus will face similar problems. Oh well, let's see how others feel or think about it ... Another aspect to keep in mind here is that the current implementation already was layered into "the keypad" which drives the pins and detects presses and may get implemented in several drivers, and "the keymap" which translates key positions to key codes they generate and gets shared among many keypad drivers. I'm not certain about whether the corgi, spitz, palm, tnetv107x and qi_lb60 implementations just share the pdata layout and implement their own logic, or just fillin the pdata from constants provided with the code yet end up creating the same GPIO matrix keypad driver. If they brought their own logic, I could not see how they reference the individual settings, and when they don't inspect new settings then they end up with strictly the former behaviour. If they create the same matrix driver that I extended, and just provide the settings by other means, then they can optionally make use of the new features yet see strictly identical behaviour if they don't adjust what later got introduced. > This change assumes it's OK to activate all columns at once, and > presumably that drivers can request the GPIOs as IRQs. This need not be > true on all HW. Should an additional property be added to describe > whether it's legal to activate all columns at once? For the IRQ case, > presumably the driver can simply try to request an IRQ for each GPIO, > and fall back to not doing so if that doesn't work. > > > +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? > > +driver's operation of activating all columns at the same time for most > > Saying "driver" in a DT binding is a slight red flag. The binding should > really purely describe HW, rather than SW's use of it. Here, the use is > probably generic enough not to be an issue, although it there's some way > to reword it to avoid that word it might be good. All of this (and the other feedback) suggests that I should have another look into the separation of the specific drivers details (under Documentation/input/ and strictly bound to the Linux kernel driver implementation) and DT binding (merely discussing how to specify parameters and not discussing how they affect the driver's operation). > > +- 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. 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? virtually yours Gerhard Sittig
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.
On Mon, Jun 24, 2013 at 16:00 -0600, Stephen Warren wrote: > > On 06/22/2013 03:23 AM, Gerhard Sittig wrote: > ... > > [ device tree binding doc, discussing matrix hardware layouts ] > > 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. So the direction to go is - move the Linux driver specific discussion to Documentation/input/ including potential hardware setups and the background on the driver's theory of operation - just concentrate on "adjustables" in the binding document, merely listing the set and their units, while the motivation or background either "is obvious" or can get looked up in the driver's discussion Reducing the set of options is orthogonal to that. Breaking backwards compatibility by changing the default behaviour after introducing more generic approaches to the specific issue is an option that remains for future changes. > >>> +- 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:-( Don't worry about the 'AC14xx' board too long, its using the keypad matrix driver is new in mainline, and still can get adjusted without too much problems before more wide spread use. "Getting it right" is what I'm currently heading for, while nothing is set in stone yet. The actual issue I see is that - the device tree binding specs get mapped to platform data - the platform data is shared among the (device tree capable but as well used without a device tree) keypad matrix driver _and_ the other platform data providing machines (mostly ARM based) So the drivers' logic referenced the 'active low' flag which the platform data already carried, which the device tree parse routines happen to setup. So when you see a reference to the property in a .dts, it's just "the tip of the ice berg" since there are other ways to communicate or setup that property. virtually yours Gerhard Sittig
On 06/28/2013 02:24 AM, Gerhard Sittig wrote: > On Mon, Jun 24, 2013 at 16:00 -0600, Stephen Warren wrote: >> >> On 06/22/2013 03:23 AM, Gerhard Sittig wrote: ... > So the direction to go is > - move the Linux driver specific discussion to > Documentation/input/ including potential hardware setups and > the background on the driver's theory of operation > - just concentrate on "adjustables" in the binding document, > merely listing the set and their units, while the motivation or > background either "is obvious" or can get looked up in the > driver's discussion > > Reducing the set of options is orthogonal to that. Yup, sounds good. > Breaking > backwards compatibility by changing the default behaviour after > introducing more generic approaches to the specific issue is an > option that remains for future changes. Breaking backwards-compatibility in the DT bindings would be problematic. They're supposed to be an ABI, which if it evolves, does so entirely in a backwards-compatible fashion. This can usually be achieved by something like: * If new DT properties present, enable new behaviour. * If old DT properties present, or lack of new properties, enable old behaviour. This allows somebody to install a specific DT on a system, then continue booting arbitrary new kernels on it without loss of functionality. >>>> 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:-( > > Don't worry about the 'AC14xx' board too long, its using the > keypad matrix driver is new in mainline, and still can get > adjusted without too much problems before more wide spread use. > "Getting it right" is what I'm currently heading for, while > nothing is set in stone yet. Given the "ABI-ness" of DT, I'm not convinced we don't have to worry about the AC14xx. I think we'll have to continue supporting that property, even if there's a newer/better/more-flexible way of representing the information too.
On Fri, Jun 28, 2013 at 08:50 -0600, Stephen Warren wrote: > > [ ... just one reference in arch/powerpc/boot/dts/ac14xx.dts ... ] > > > > Don't worry about the 'AC14xx' board too long, its using the > > keypad matrix driver is new in mainline, and still can get > > adjusted without too much problems before more wide spread use. > > "Getting it right" is what I'm currently heading for, while > > nothing is set in stone yet. > > Given the "ABI-ness" of DT, I'm not convinced we don't have to worry > about the AC14xx. I think we'll have to continue supporting that > property, even if there's a newer/better/more-flexible way of > representing the information too. Don't get me wrong, I wasn't putting this lightly. From first hand experience I can tell that none of those boards in the field use current or even recent mainline kernels. All of them are running an older version which predates the introduction of 'AC14xx' support in kernel.org sources. Extending official support and "doing it the right way" is what I'm currently working on, before those boards may switch to running mainline kernels and compatibility with the first version that was used will become an issue. So to put it into other words: _If_ the 'AC14xx' board is the _only_ entity which references the property, then there's no problem at all. Incompatible changes for _this_one_board_ are acceptable since at the moment the current implementation is not used in the field. Of course I agree that breaking any other board isn't acceptable, which is why I followed the approach of strictly keeping backwards compatible behaviour even in those spots where a different approach or a different default would have looked more desireable. Which is why I'm still reluctant to more intrusively change the general working of the keypad matrix driver in contrast to introducing strictly local changes which only optionally add additional features which explicitly must get enabled before changed behaviour will occur. So I need some more time to think of which parts to change in which ways and how to make sure that nothings breaks ... virtually yours Gerhard Sittig
diff --git a/Documentation/devicetree/bindings/input/gpio-matrix-keypad.txt b/Documentation/devicetree/bindings/input/gpio-matrix-keypad.txt index ead641c..11d030e 100644 --- a/Documentation/devicetree/bindings/input/gpio-matrix-keypad.txt +++ b/Documentation/devicetree/bindings/input/gpio-matrix-keypad.txt @@ -2,9 +2,30 @@ GPIO driven matrix keypad is used to interface a SoC with a matrix keypad. The matrix keypad supports multiple row and column lines, a key can be -placed at each intersection of a unique row and a unique column. The matrix -keypad can sense a key-press and key-release by means of GPIO lines and -report the event using GPIO interrupts to the cpu. +placed at each intersection of a unique row and a unique column. + +Sampling the keypad status is done by individually activating column +lines (which thus are outputs) and reading back row lines (which are +inputs). Each combination of row and column is checked separately to +determine the status of a single key. + +To reduce load on the CPU, changes in the keys' status can get detected +by means of GPIO interrupts, which get generated upon changes in the pin +levels. This approach assumes that any change in the key press status +will result in a GPIO pin level change interrupt when all columns are +selected (active) at the same time. Which in turn assumes that all +column lines can get activated at the same time, and no harm is done to +the hardware when multiple keys are pressed simultaneously. + +Simple keypad matrix layouts just connect GPIO lines to mechanical +switches, with no other active components involved. Although due to the +driver's operation of activating all columns at the same time for most +of the time, the use of current limiting resistors may be desirable, or +column GPIO lines shall operate in open collector mode. + +More involved matrix layouts may include externally connected line +drivers and might influence signal polarity. The driver supports low +active signals, while high active line levels are assumed by default. Required Properties: - compatible: Should be "gpio-matrix-keypad" @@ -20,9 +41,18 @@ Required Properties: Optional Properties: - linux,no-autorepeat: do no enable autorepeat feature. - linux,wakeup: use any event on keypad as wakeup event. -- debounce-delay-ms: debounce interval in milliseconds -- col-scan-delay-us: delay, measured in microseconds, that is needed - before we can scan keypad after activating column gpio +- debounce-delay-ms: debounce interval in milliseconds, which needs + to pass between detecting a change in the key + press status and sampling the new status, e.g. + to support multi key presses without spurious + single key events, or to cope with bouncing + mechanical key switches +- col-scan-delay-us: delay interval in microseconds between selecting + a column line and reading back its row status, + such that pin levels can settle after + propagating through the matrix and its + associated hardware components +- gpio-activelow: row pins as well as column pins are active low Example: matrix-keypad {
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 Signed-off-by: Gerhard Sittig <gsi@denx.de> --- .../bindings/input/gpio-matrix-keypad.txt | 42 +++++++++++++++++--- 1 file changed, 36 insertions(+), 6 deletions(-)