[PATCH 1/4] input: Add new sun4i-lradc-keys drivers

[hdegoede@redhat.com (Hans de Goede)]

Hi,

On 01/02/2014 12:59 PM, Heiko St?bner wrote:
> Hi Hans, Dmitry,
>
> Am Donnerstag, 2. Januar 2014, 10:37:47 schrieb Hans de Goede:
>> Hi,
>>
>> On 01/01/2014 09:56 PM, Dmitry Torokhov wrote:
>>> Hi Hans,
>>>
>>> On Wed, Jan 01, 2014 at 08:30:07PM +0100, Hans de Goede wrote:
>>>> +Required properties:
>>>> + - compatible: "allwinner,sun4i-lradc-keys"
>>>> + - reg: mmio address range of the chip
>>>> + - interrupts: interrupt to which the chip is connected
>>>> + - allwinner,chan0-step: step in mV between keys must be 150 or 200
>>>> + - allwinner,chan0-keycodes: array of include/uapi/linux/input.h KEY_
>>>> codes>
>>> I think this should be "linux,chan0-keycodes".
>>
>> Right, because the codes are Linux specific, will fix in v2.
>
> but the property with its "chan0-" thingy would be allwinner-specific if I'm
> not mistaken.

Correct, but denoting that this is linux only is more important, so as to
avoid namespace collisions.

>
> Also, instead of inventing yet another vendor-specific property, why not re-use
> a button binding similar to gpio-keys like:
>
>         lradc: lradc at 01c22800 {
>                 compatible = "allwinner,sun4i-lradc-keys";
>                 reg = <0x01c22800 0x100>;
>                 interrupts = <31>;
>                 allwinner,chan0-step = <200>;
>
> 		#address-cells = <1>;
> 		#size-cells = <0>;
>
> 		button at 0 {
> 			reg = <0>; /* your channel index from above */
> 			linux,code = <115>; /* already used as dt-property */
> 		};
>
> 		button at 1 {
> 			reg = <1>;
> 			linux,code = <114>;
> 		};

Ugh no. Having a vendor specific property which is KISS certainly beats this,
both wrt ease of writing dts files as well as wrt the dts parsing code in the driver.

Regards,

Hans