| Message ID | 1371838198-7327-5-git-send-email-gsi@denx.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 06/21/2013 12:09 PM, Gerhard Sittig wrote: > extend the device tree adjustable hardware configuration: > - allow for differing polarity of the row and column GPIO pins > - optionally fully drive column output pins instead of the former > unconditional open collector emulation approach > diff --git a/Documentation/devicetree/bindings/input/gpio-matrix-keypad.txt b/Documentation/devicetree/bindings/input/gpio-matrix-keypad.txt > +- row-gpios-activelow: row GPIO pins are active low > +- col-gpios-activelow: column GPIO pins are active low > - gpio-activelow: row pins as well as column pins are active low > + (provided for backward compatibility, and useful > + for matrix layouts of identical polarity for > + rows and columns) Those should all come from the existing GPIO flags, and may even differ for each GPIO. > +- col-gpios-pushpull: fully drive the column selection pins in either > + direction (high and low signals), the default > + behaviour is to actively drive low signals and > + be passive otherwise (emulates an open collector > + output driver) We don't actually have GPIO flags defined for pushpull-vs-open-collector etc. Perhaps we should do so. Then, we wouldn't need to invent custom properties to represent that in this binding.
On Fri, Jun 21, 2013 at 15:34 -0600, Stephen Warren wrote: > > On 06/21/2013 12:09 PM, Gerhard Sittig wrote: > > extend the device tree adjustable hardware configuration: > > - allow for differing polarity of the row and column GPIO pins > > - optionally fully drive column output pins instead of the former > > unconditional open collector emulation approach > > > diff --git a/Documentation/devicetree/bindings/input/gpio-matrix-keypad.txt b/Documentation/devicetree/bindings/input/gpio-matrix-keypad.txt > > > +- row-gpios-activelow: row GPIO pins are active low > > +- col-gpios-activelow: column GPIO pins are active low > > - gpio-activelow: row pins as well as column pins are active low > > + (provided for backward compatibility, and useful > > + for matrix layouts of identical polarity for > > + rows and columns) > > Those should all come from the existing GPIO flags, and may even differ > for each GPIO. > > > +- col-gpios-pushpull: fully drive the column selection pins in either > > + direction (high and low signals), the default > > + behaviour is to actively drive low signals and > > + be passive otherwise (emulates an open collector > > + output driver) > > We don't actually have GPIO flags defined for pushpull-vs-open-collector > etc. Perhaps we should do so. Then, we wouldn't need to invent custom > properties to represent that in this binding. I see your concerns and understand, but chose the above way to just extend the existing implementation in the least intrusive way without changing its nature. See the reply on the first patch for more details on the motivation. Of course I agree that pin properties belong to the GPIO layer and that "application drivers" on top should not worry about that. Were I allowed to break backwards compatibility, then I could have done more shuffling. But lacking the capability to test the affected setups puts more burdon on others, which I wanted to avoid. And then there's the question of whether the "active low" applies to the GPIO pin or to the matrix line (the above property suggests it's the pin, but I'm yet undecided), and whether the subsystems agree on a policy "where to put" the "overall polarity" of such a chain of driver logic and SoC pins and optionally involved inverting drivers and external components which the driver communicates to. The push-pull vs open-drain should never have made it into the keypad driver, but that fact wasn't introduced but did exist. Even if the kernel had no way to setup OD mode of a pin, the bootloader has, and the kernel just needs to setup "values" just like for any other pin regardless of push-pull or open-drain. virtually yours Gerhard Sittig
On 06/22/2013 03:36 AM, Gerhard Sittig wrote: > On Fri, Jun 21, 2013 at 15:34 -0600, Stephen Warren wrote: >> >> On 06/21/2013 12:09 PM, Gerhard Sittig wrote: >>> extend the device tree adjustable hardware configuration: >>> - allow for differing polarity of the row and column GPIO pins >>> - optionally fully drive column output pins instead of the former >>> unconditional open collector emulation approach >> >>> diff --git a/Documentation/devicetree/bindings/input/gpio-matrix-keypad.txt b/Documentation/devicetree/bindings/input/gpio-matrix-keypad.txt >> >>> +- row-gpios-activelow: row GPIO pins are active low >>> +- col-gpios-activelow: column GPIO pins are active low >>> - gpio-activelow: row pins as well as column pins are active low >>> + (provided for backward compatibility, and useful >>> + for matrix layouts of identical polarity for >>> + rows and columns) >> >> Those should all come from the existing GPIO flags, and may even differ >> for each GPIO. >> >>> +- col-gpios-pushpull: fully drive the column selection pins in either >>> + direction (high and low signals), the default >>> + behaviour is to actively drive low signals and >>> + be passive otherwise (emulates an open collector >>> + output driver) >> >> We don't actually have GPIO flags defined for pushpull-vs-open-collector >> etc. Perhaps we should do so. Then, we wouldn't need to invent custom >> properties to represent that in this binding. > > I see your concerns and understand, but chose the above way to > just extend the existing implementation in the least intrusive > way without changing its nature. See the reply on the first > patch for more details on the motivation. > > Of course I agree that pin properties belong to the GPIO layer > and that "application drivers" on top should not worry about > that. Were I allowed to break backwards compatibility, then I > could have done more shuffling. But lacking the capability to > test the affected setups puts more burdon on others, which I > wanted to avoid. Can't you add this enhancement as follows: Update the driver to look at the per-pin GPIO flags in all cases. Presumably in any existing cases, those flags all say "active high" anyway, since specifying anything else was useless. In the very unlikely case this isn't true, one could always add a quirk based on the HW-specific compatible value. If gpio-activelow is set, mark all pins as active-low. This way, any future extensions (i.e. what this patch implements: just rows active low, just columns active low) can be implemented using purely the standard GPIO flags, but the existing behaviour of the existing gpio-activelow property is maintained.
On Mon, Jun 24, 2013 at 17:14 -0600, Stephen Warren wrote: > > [ active low pins, individually for sets or even single pins ] > > Can't you add this enhancement as follows: > > Update the driver to look at the per-pin GPIO flags in all cases. > Presumably in any existing cases, those flags all say "active high" > anyway, since specifying anything else was useless. In the very unlikely > case this isn't true, one could always add a quirk based on the > HW-specific compatible value. Maybe I missed something, but - is "the 'active low' property of a GPIO pin" something platform specific? (maybe ARM inspired, introduced with pinctrl features) - is it only available for specific GPIO controller drivers ("chips" or "banks"), or is it in some layer on top of gpiolib? - is it in some other tree or branch than mainline? Documentation/gpio.txt suggests and eyeballing the gpiolib.c source supports (v3.10-rc7) that the in-kernel API doesn't have an 'active-low' for pins. There is one for the sysfs API and pins that were exported to user space, but not for in-kernel use. This fits with my observation that application drivers on top of gpiolib often take care of such a property which actually looks like it would belong to the physical attachment. I understand that a chip's driver will hide when a SoC's pin is inverted, but I cannot see where gpiolib provides a means to hide an externally connected inverting driver. virtually yours Gerhard Sittig
On 06/28/2013 02:33 AM, Gerhard Sittig wrote: > On Mon, Jun 24, 2013 at 17:14 -0600, Stephen Warren wrote: >> >> [ active low pins, individually for sets or even single pins ] >> >> Can't you add this enhancement as follows: >> >> Update the driver to look at the per-pin GPIO flags in all cases. >> Presumably in any existing cases, those flags all say "active high" >> anyway, since specifying anything else was useless. In the very unlikely >> case this isn't true, one could always add a quirk based on the >> HW-specific compatible value. > > Maybe I missed something, but > - is "the 'active low' property of a GPIO pin" something platform > specific? (maybe ARM inspired, introduced with pinctrl features) > - is it only available for specific GPIO controller drivers > ("chips" or "banks"), or is it in some layer on top of gpiolib? The way this property is represented is technically defined by each individual GPIO controller's DT binding. However, at least on ARM (and in fact I'd guess everywhere), many/most controllers do this in the same way; by adding an additional cell to #gpio-cells, which holds flags data, with values in that cell containing: include/linux/of_gpio.h: > /* > * This is Linux-specific flags. By default controllers' and Linux' mapping > * match, but GPIO controllers are free to translate their own flags to > * Linux-specific in their .xlate callback. Though, 1:1 mapping is recommended. > */ > enum of_gpio_flags { > OF_GPIO_ACTIVE_LOW = 0x1, > }; include/dt-bindings/gpio/gpio.h: > #define GPIO_ACTIVE_HIGH 0 > #define GPIO_ACTIVE_LOW 1 I'm not sure how new this is; it's possible older bindings don't have this extra cell. A quick grep of arch/*/boot/dts shows that almost all values of #gpio-cells are 2, so likely this is almost everywhere though. > - is it in some other tree or branch than mainline? It's been in mainline for quite a while now; a good number of kernel releases. > Documentation/gpio.txt suggests and eyeballing the gpiolib.c > source supports (v3.10-rc7) that the in-kernel API doesn't have > an 'active-low' for pins. There is one for the sysfs API and > pins that were exported to user space, but not for in-kernel use. > > This fits with my observation that application drivers on top of > gpiolib often take care of such a property which actually looks > like it would belong to the physical attachment. > > I understand that a chip's driver will hide when a SoC's pin is > inverted, but I cannot see where gpiolib provides a means to hide > an externally connected inverting driver. With the current gpiolib APIs, gpiolib only provides a way for GPIO clients to retrieve the GPIO flags from DT. It's then up to the GPIO client to actually make use of the flag, by inverting the value passed to gpio_set(), or returned from gpio_get(). Flags are retrieved by calling of_get_named_gpio_flags() rather than of_get_named_gpio(). There is a new GPIO API under development which I /think/ will hide this flag inside the GPIO core, so that GPIO clients don't have to care about it, and it all works automatically. I'm not sure what the status of this is though. Search Google for "gpio_get()".
On Fri, Jun 28, 2013 at 09:01 -0600, Stephen Warren wrote: > > On 06/28/2013 02:33 AM, Gerhard Sittig wrote: > > On Mon, Jun 24, 2013 at 17:14 -0600, Stephen Warren wrote: > >> > >> [ active low pins, individually for sets or even single pins ] > >> > >> Can't you add this enhancement as follows: > >> > >> Update the driver to look at the per-pin GPIO flags in all cases. > >> Presumably in any existing cases, those flags all say "active high" > >> anyway, since specifying anything else was useless. In the very unlikely > >> case this isn't true, one could always add a quirk based on the > >> HW-specific compatible value. > > > > Maybe I missed something, but > > - is "the 'active low' property of a GPIO pin" something platform > > specific? (maybe ARM inspired, introduced with pinctrl features) > > - is it only available for specific GPIO controller drivers > > ("chips" or "banks"), or is it in some layer on top of gpiolib? > > The way this property is represented is technically defined by each > individual GPIO controller's DT binding. However, at least on ARM (and > in fact I'd guess everywhere), many/most controllers do this in the same > way; by adding an additional cell to #gpio-cells, which holds flags > data, with values in that cell containing: > > include/linux/of_gpio.h: > > > /* > > * This is Linux-specific flags. By default controllers' and Linux' mapping > > * match, but GPIO controllers are free to translate their own flags to > > * Linux-specific in their .xlate callback. Though, 1:1 mapping is recommended. > > */ > > enum of_gpio_flags { > > OF_GPIO_ACTIVE_LOW = 0x1, > > }; > > include/dt-bindings/gpio/gpio.h: > > > #define GPIO_ACTIVE_HIGH 0 > > #define GPIO_ACTIVE_LOW 1 > > I'm not sure how new this is; it's possible older bindings don't have > this extra cell. A quick grep of arch/*/boot/dts shows that almost all > values of #gpio-cells are 2, so likely this is almost everywhere though. Ah, so it's not a feature of gpiolib in itself, but in parallel to gpiolib. In addition to the "chip and pin" spec in device tree bindings, one may specify additional flags, of which currently only 'active-low' exists. Thank you for the pointer. Yes I've seen "phandle plus two cell" specs everywhere, but haven't seen any non-zero value so far in the second cell. > > - is it in some other tree or branch than mainline? > > It's been in mainline for quite a while now; a good number of kernel > releases. > > > Documentation/gpio.txt suggests and eyeballing the gpiolib.c > > source supports (v3.10-rc7) that the in-kernel API doesn't have > > an 'active-low' for pins. There is one for the sysfs API and > > pins that were exported to user space, but not for in-kernel use. > > > > This fits with my observation that application drivers on top of > > gpiolib often take care of such a property which actually looks > > like it would belong to the physical attachment. > > > > I understand that a chip's driver will hide when a SoC's pin is > > inverted, but I cannot see where gpiolib provides a means to hide > > an externally connected inverting driver. > > With the current gpiolib APIs, gpiolib only provides a way for GPIO > clients to retrieve the GPIO flags from DT. It's then up to the GPIO > client to actually make use of the flag, by inverting the value passed > to gpio_set(), or returned from gpio_get(). > > Flags are retrieved by calling of_get_named_gpio_flags() rather than > of_get_named_gpio(). Ah, so optionally inverting the level between the logical request in software and controlling the physical pin remains the job of the application driver (here: the input driver that manages the matrix which just happens to be attached to GPIOs), since it's not transparently done within gpiolib or the gpio chip driver. But what the matrix driver shall do is not apply one 'invert everything' flag, but instead apply a flag that's specific to an individual GPIO pin. And this pin specific flag either gets setup from device tree information (from the flags cell of the GPIO spec), or gets derived from the platform data's 'invert everything' flag that applies to the non-DT case. And thus the individual polarity of rows and columns is a mere "byproduct" of respecting the individual pins' polarity. Nice. :) Thank you, this pointer is much appreciated. > There is a new GPIO API under development which I /think/ will hide this > flag inside the GPIO core, so that GPIO clients don't have to care about > it, and it all works automatically. I'm not sure what the status of this > is though. Search Google for "gpio_get()". Oh, I understand that this involves a huge sweep over both layers of GPIO chip drivers as well as GPIO API using drivers. And is completely orthogonal to whether and when more drivers are tought to respect the pin's active-low property. 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 11d030e..e6a01eb 100644 --- a/Documentation/devicetree/bindings/input/gpio-matrix-keypad.txt +++ b/Documentation/devicetree/bindings/input/gpio-matrix-keypad.txt @@ -27,6 +27,14 @@ 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. +Since output drivers and input drivers may exist independently from each +other and may or may not invert signals, polarity for row pins and for +column pins is adjustable individually. The initial implementation of +the driver used to emulate some kind of an open collector behaviour in +software, which makes signals float in the absence of pull up resistors. +To fully drive output signals in either direction, enable push/pull mode +for column pins. This option is off by default for compatibility. + Required Properties: - compatible: Should be "gpio-matrix-keypad" - row-gpios: List of gpios used as row lines. The gpio specifier @@ -52,7 +60,17 @@ Optional Properties: such that pin levels can settle after propagating through the matrix and its associated hardware components +- row-gpios-activelow: row GPIO pins are active low +- col-gpios-activelow: column GPIO pins are active low - gpio-activelow: row pins as well as column pins are active low + (provided for backward compatibility, and useful + for matrix layouts of identical polarity for + rows and columns) +- col-gpios-pushpull: fully drive the column selection pins in either + direction (high and low signals), the default + behaviour is to actively drive low signals and + be passive otherwise (emulates an open collector + output driver) Example: matrix-keypad { diff --git a/arch/arm/mach-davinci/board-tnetv107x-evm.c b/arch/arm/mach-davinci/board-tnetv107x-evm.c index ba79837..d41bd8a 100644 --- a/arch/arm/mach-davinci/board-tnetv107x-evm.c +++ b/arch/arm/mach-davinci/board-tnetv107x-evm.c @@ -199,7 +199,8 @@ static struct matrix_keypad_platform_data keypad_config = { .num_row_gpios = 6, .num_col_gpios = 5, .debounce_ms = 0, /* minimum */ - .active_low = 0, /* pull up realization */ + .col_gpios_active_low = 0, /* pull up realization */ + .row_gpios_active_low = 0, /* pull up realization */ .no_autorepeat = 0, }; diff --git a/arch/arm/mach-omap2/board-h4.c b/arch/arm/mach-omap2/board-h4.c index 69c0acf..78c91cd 100644 --- a/arch/arm/mach-omap2/board-h4.c +++ b/arch/arm/mach-omap2/board-h4.c @@ -97,7 +97,8 @@ static struct matrix_keypad_platform_data board_keypad_platform_data = { .num_row_gpios = ARRAY_SIZE(board_keypad_row_gpios), .col_gpios = board_keypad_col_gpios, .num_col_gpios = ARRAY_SIZE(board_keypad_col_gpios), - .active_low = 1, + .col_gpios_active_low = 1, + .row_gpios_active_low = 1, .debounce_ms = 20, .col_scan_delay_us = 5, diff --git a/arch/arm/mach-pxa/palmtc.c b/arch/arm/mach-pxa/palmtc.c index 100b176f..4767563 100644 --- a/arch/arm/mach-pxa/palmtc.c +++ b/arch/arm/mach-pxa/palmtc.c @@ -314,7 +314,8 @@ static struct matrix_keypad_platform_data palmtc_keypad_platform_data = { .num_row_gpios = ARRAY_SIZE(palmtc_keypad_row_gpios), .col_gpios = palmtc_keypad_col_gpios, .num_col_gpios = ARRAY_SIZE(palmtc_keypad_col_gpios), - .active_low = 1, + .col_gpios_active_low = 1, + .row_gpios_active_low = 1, .debounce_ms = 20, .col_scan_delay_us = 5, diff --git a/arch/mips/jz4740/board-qi_lb60.c b/arch/mips/jz4740/board-qi_lb60.c index be2b3de..4235ca2 100644 --- a/arch/mips/jz4740/board-qi_lb60.c +++ b/arch/mips/jz4740/board-qi_lb60.c @@ -250,7 +250,8 @@ static struct matrix_keypad_platform_data qi_lb60_pdata = { .col_scan_delay_us = 10, .debounce_ms = 10, .wakeup = 1, - .active_low = 1, + .col_gpios_active_low = 1, + .row_gpios_active_low = 1, }; static struct platform_device qi_lb60_keypad = { diff --git a/drivers/input/keyboard/matrix_keypad.c b/drivers/input/keyboard/matrix_keypad.c index 5b5f86d..4f22149 100644 --- a/drivers/input/keyboard/matrix_keypad.c +++ b/drivers/input/keyboard/matrix_keypad.c @@ -43,20 +43,37 @@ struct matrix_keypad { }; /* + * former comment before introduction of optional push/pull behaviour: + * <cite> * NOTE: normally the GPIO has to be put into HiZ when de-activated to cause * minmal side effect when scanning other columns, here it is configured to * be input, and it should work on most platforms. + * </cite> + * + * the above sounds a lot like emulating open collector (open drain) + * behaviour in software, which should only be required if the GPIO pin + * lacks such a mode, which should be rare and could be hidden by + * transparent support in the GPIO chip driver, or could be handled by + * attaching an external transistor to the pin, which is cheap and in + * addition avoids damage in the hardware caused by running the wrong + * software configuration + * + * applying the "turn to input" logic unconditionally breaks hardware + * setups which have no pull resistors (i.e. where outputs are directly + * connected to the matrix or to external matrix line drivers), and + * could make input signals float and become unreliable */ static void __activate_col_pin(const struct matrix_keypad_platform_data *pdata, int col, bool on) { - bool level_on = !pdata->active_low; + bool level_on = !pdata->col_gpios_active_low; if (on) { gpio_direction_output(pdata->col_gpios[col], level_on); } else { gpio_set_value_cansleep(pdata->col_gpios[col], !level_on); - gpio_direction_input(pdata->col_gpios[col]); + if (!pdata->col_gpios_push_pull) + gpio_direction_input(pdata->col_gpios[col]); } } @@ -82,7 +99,8 @@ static bool row_asserted(const struct matrix_keypad_platform_data *pdata, int row) { return gpio_get_value_cansleep(pdata->row_gpios[row]) ? - !pdata->active_low : pdata->active_low; + !pdata->row_gpios_active_low : + pdata->row_gpios_active_low; } static void enable_row_irqs(struct matrix_keypad *keypad) @@ -317,7 +335,8 @@ static int matrix_keypad_init_gpio(struct platform_device *pdev, goto err_free_cols; } - gpio_direction_output(pdata->col_gpios[i], !pdata->active_low); + gpio_direction_output(pdata->col_gpios[i], + !pdata->col_gpios_active_low); } for (i = 0; i < pdata->num_row_gpios; i++) { @@ -427,8 +446,16 @@ matrix_keypad_parse_dt(struct device *dev) pdata->no_autorepeat = true; if (of_get_property(np, "linux,wakeup", NULL)) pdata->wakeup = true; - if (of_get_property(np, "gpio-activelow", NULL)) - pdata->active_low = true; + if (of_get_property(np, "col-gpios-activelow", NULL)) + pdata->col_gpios_active_low = true; + if (of_get_property(np, "row-gpios-activelow", NULL)) + pdata->row_gpios_active_low = true; + if (of_get_property(np, "gpio-activelow", NULL)) { + pdata->row_gpios_active_low = true; + pdata->col_gpios_active_low = true; + } + if (of_get_property(np, "col-gpios-pushpull", NULL)) + pdata->col_gpios_push_pull = true; of_property_read_u32(np, "debounce-delay-ms", &pdata->debounce_ms); of_property_read_u32(np, "col-scan-delay-us", diff --git a/include/linux/input/matrix_keypad.h b/include/linux/input/matrix_keypad.h index 27e06ac..5496a00 100644 --- a/include/linux/input/matrix_keypad.h +++ b/include/linux/input/matrix_keypad.h @@ -45,7 +45,10 @@ struct matrix_keymap_data { * @clustered_irq: may be specified if interrupts of all row/column GPIOs * are bundled to one single irq * @clustered_irq_flags: flags that are needed for the clustered irq - * @active_low: gpio polarity + * @row_gpios_active_low: polarity of row gpio pins + * @col_gpios_active_low: polarity of column gpio pins + * @col_gpios_push_pull: whether column gpio pins emulate open drain + * behaviour or fully drive pin levels to either direction * @wakeup: controls whether the device should be set up as wakeup * source * @no_autorepeat: disable key autorepeat @@ -70,7 +73,9 @@ struct matrix_keypad_platform_data { unsigned int clustered_irq; unsigned int clustered_irq_flags; - bool active_low; + bool row_gpios_active_low; + bool col_gpios_active_low; + bool col_gpios_push_pull; bool wakeup; bool no_autorepeat; };
extend the device tree adjustable hardware configuration: - allow for differing polarity of the row and column GPIO pins - optionally fully drive column output pins instead of the former unconditional open collector emulation approach this change introduces individual polarity settings for the row pins as well as the column pins, to achieve this separate members in the platform data bundle get introduced, and all places which fill in the platform data receive an update -- the result is complete backwards compatibility with existing matrix drivers and an improvement for newly written code which needs the individual polarity settings this change allows optional support to fully drive the column selection pins, but by default sticks with the former behaviour of driving the active level and being passive for the inactive level, so - unadjusted configurations keep operating the hardware in an identical manner - adjusted or newly configured hardware will be operated according to the configuration, which now allows for either setups with pull resistors or mere connections between logic so this change doesn't break existing behaviour, is neutral for unadjusted code or configurations, and is an improvement in those areas where the former implementations was restricted Signed-off-by: Gerhard Sittig <gsi@denx.de> --- .../bindings/input/gpio-matrix-keypad.txt | 18 +++++++++ arch/arm/mach-davinci/board-tnetv107x-evm.c | 3 +- arch/arm/mach-omap2/board-h4.c | 3 +- arch/arm/mach-pxa/palmtc.c | 3 +- arch/mips/jz4740/board-qi_lb60.c | 3 +- drivers/input/keyboard/matrix_keypad.c | 39 +++++++++++++++++--- include/linux/input/matrix_keypad.h | 9 ++++- 7 files changed, 66 insertions(+), 12 deletions(-)