Re: [PATCH v1 04/12] input: matrix-keypad: push/pull, separate polarity

2013-06-30 Thread Gerhard Sittig 
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
-- 
DENX Software Engineering GmbH, MD: Wolfgang Denk  Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: off...@denx.de

Re: [PATCH v1 04/12] input: matrix-keypad: push/pull, separate polarity

2013-06-28 Thread Stephen Warren 
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().
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: [PATCH v1 04/12] input: matrix-keypad: push/pull, separate polarity

2013-06-24 Thread Stephen Warren 
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.
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: [PATCH v1 04/12] input: matrix-keypad: push/pull, separate polarity

2013-06-22 Thread Gerhard Sittig 
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
-- 
DENX Software Engineering GmbH, MD: Wolfgang Denk  Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: off...@denx.de
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

[PATCH v1 04/12] input: matrix-keypad: push/pull, separate polarity

2013-06-21 Thread Gerhard Sittig 
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 g...@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(-)

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  =

Re: [PATCH v1 04/12] input: matrix-keypad: push/pull, separate polarity

2013-06-21 Thread Stephen Warren 
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.
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss