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

[Stephen Warren <swarren@wwwdotorg.org>]

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.