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

[Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 4230 bytes --]

On Thu, Jan 02, 2014 at 11:36:33PM +0100, Hans de Goede wrote:
> Hi,
> 
> On 01/02/2014 09:20 PM, Maxime Ripard wrote:
> >On Thu, Jan 02, 2014 at 02:45:29PM +0100, Hans de Goede wrote:
> >>>Also, instead of inventing yet another vendor-specific property, why not re-use
> >>>a button binding similar to gpio-keys like:
> >>>
> >>>        lradc: lradc@01c22800 {
> >>>                compatible = "allwinner,sun4i-lradc-keys";
> >>>                reg = <0x01c22800 0x100>;
> >>>                interrupts = <31>;
> >>>                allwinner,chan0-step = <200>;
> >>>
> >>>		#address-cells = <1>;
> >>>		#size-cells = <0>;
> >>>
> >>>		button@0 {
> >>>			reg = <0>; /* your channel index from above */
> >>>			linux,code = <115>; /* already used as dt-property */
> >>>		};
> >>>
> >>>		button@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.
> >
> >I'd agree with Heiko here. This is pretty much the same construct
> >that's already in use in other input drivers, like gpio-keys.
> 
> In the gpio case there is a 1 on 1 relation between a single hw
> entity (the gpio-pin) and a single keycode. Here there is 1 hw entity
> which maps to an array of key-codes, certainly using an array rather
> then a much more complicated construct is the correct data-structure
> to represent this.

You can build an array in your driver out of this very easily, it's 10
lines in your probe. And you gain from this something that is more
generic, can be shared by other similar drivers and is consistent with
what is already in use.

> >This is also something that can really easily be made generic,
> >since this is something that is rather common.
> >
> >Speaking of which. I believe this should actually come in two
> >different drivers:
> >   - The ADC driver itself, using IIO
> >   - A generic button handler driver on top of IIO.
> >
> > The fact that on most board this adc is used for buttons doesn't make
> > any difference, it's actually a hardware designer choice, we should
> > support that choice, but we should also be able to use it just as an
> > ADC.
> 
> No, this is not a generic adc, as mentioned in the commit msg, this
> adc is specifically designed to be used this way.
> 
> The adc won't start sampling data, and won't generate any interrupts
> until a button is pressed. That is until the input voltage drops below
> 2/3 of Vref, this is checked through a built-in analog comparator, which
> hooks into the control logic.
> 
> It has button down and button up interrupts, and can detect long
> presses (unused) and generate a second type of down interrupt for those.
> 
> This really is an input device, which happens to use an adc.

Hmm, yes, ok.

> >Carlo Caione already started to work on an IIO driver for the LRADC:
> >https://github.com/carlocaione/linux/tree/sunxi-lradc
> >maybe you can take over his work.
> 
> That won't work because the adc won't sample if the input gets above
> 2/3 of Vref. There may be some other mode which does not do that, but
> that is not clearly documented.
> 
> Even if an IIO driver turns out to be doable, I strongly believe that
> having a separate input driver for this is best, since this device
> was designed to be used as such. Building input on top of IIO would
> mean polling the adc, while with my driver it actually generates
> button down / up interrupts without any polling being involved.

Not really. iio_channel_read calls the read_raw function (in that
case) of your driver. If the read_raw function in your driver wants to
poll the device, fine, but most of the time, it will just block
waiting for an interrupt to come and return the data to the caller,
which is obviously the saner behaviour, and you don't actually end up
polling the device. Which is pretty much the architecture you're using
already, just with an intermediate layer in between.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]