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

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

[Heiko Stübner]

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.

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>;
		};

...

       };

But I may be on the wrong track here, so I've included the devicetree-people 
for help, which I guess should've been included from the beginning.


Heiko

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

[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@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.

Regards,

Hans

-- 
You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/groups/opt_out.

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

[Maxime Ripard]

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

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.

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.

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.

I also wonder wether it would be possible in that case to use reg as
the "button" voltage, to get rid of both the chan0-step property, and
those big fat arrays in the driver.

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 --]

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

[Dmitry Torokhov]

On Thu, Jan 02, 2014 at 09:20:22PM +0100, 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.
> 
> This is also something that can really easily be made generic, since
> this is something that is rather common.

Except that button definition from gpio-keys does not use 'reg' property
but rather gpio. I'd rather we did not cram non-applicable attributes
into that definition just to make it "reusable" like that.

I'd be OK with having similar (but not claiming to be the same) mappings
though.

Thanks.

-- 
Dmitry

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

[Maxime Ripard]

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

On Thu, Jan 02, 2014 at 12:38:31PM -0800, Dmitry Torokhov wrote:
> On Thu, Jan 02, 2014 at 09:20:22PM +0100, 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.
> > 
> > This is also something that can really easily be made generic, since
> > this is something that is rather common.
> 
> Except that button definition from gpio-keys does not use 'reg' property
> but rather gpio. I'd rather we did not cram non-applicable attributes
> into that definition just to make it "reusable" like that.
> 
> I'd be OK with having similar (but not claiming to be the same) mappings
> though.

Yes, this is what I was meaning. Sorry if it was not clear enough.

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 --]

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

[Hans de Goede]

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.

>
> 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.

> 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.

And no boards I know of are using this as a generic analog input,
where as many boards are using it as designed.

Regards,

Hans

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

[Maxime Ripard]

[-- 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 --]

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

[Dmitry Torokhov]

On Fri, Jan 03, 2014 at 06:36:05PM +0100, Maxime Ripard wrote:
> 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.

How will it be shared? Surely not code-wise, but basically in spirit
only. It seems to me that the originally proposed binding is simple and
concise and works well for the driver.

> 
> > >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.

What is the benefit of the IIO layer if device can't really be used as
IIO? I am all for moving as many generic devices as we can to IIO but we
should recognize that sometimes the device is not an IIO device.

Thanks.

-- 
Dmitry

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

[Heiko Stübner]

Am Freitag, 3. Januar 2014, 10:23:50 schrieb Dmitry Torokhov:
> On Fri, Jan 03, 2014 at 06:36:05PM +0100, Maxime Ripard wrote:
> > 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.
> 
> How will it be shared? Surely not code-wise, but basically in spirit
> only. It seems to me that the originally proposed binding is simple and
> concise and works well for the driver.

I don't think "binding [...] works well for the driver" is the correct 
direction. From my understanding the binding should describe the hardware in 
an os-agnostic way (so "linux,foo" properties should stay the exception) and 
not the data structures used in the driver. The driver itself then implements 
the binding to convert the binding-data into a structure it wants to use.

The sharing would, as you suggested, be in spirit and in the use of already 
established dt-properties without introducing more non-standard ones.


Heiko

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

[Maxime Ripard]

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

On Fri, Jan 03, 2014 at 10:23:50AM -0800, Dmitry Torokhov wrote:
> On Fri, Jan 03, 2014 at 06:36:05PM +0100, Maxime Ripard wrote:
> > 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.
> 
> How will it be shared? Surely not code-wise, but basically in spirit
> only. It seems to me that the originally proposed binding is simple and
> concise and works well for the driver.

See Heiko's answer, but I do believe the code can be shared as well if
needs be.

> > > >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.
> 
> What is the benefit of the IIO layer if device can't really be used as
> IIO? I am all for moving as many generic devices as we can to IIO but we
> should recognize that sometimes the device is not an IIO device.

Yes, I've agreed to that. I was just clarifying Hans' statement.

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 --]