input: Add new sun4i-lradc-keys driver

+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

Hi Hans,

Quoted 6 lines · Hans de Goede

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

Hi,

Quoted 10 lines · Dmitry Torokhov

Hi Hans,

Quoted 7 lines · Hans de Goede

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

Quoted 1 line

I think this should be "linux,chan0-keycodes".

Right, because the codes are Linux specific, will fix in v2.

Hi Hans, Dmitry,

Am Donnerstag, 2. Januar 2014, 10:37:47 schrieb Hans de Goede:

Quoted 13 lines

Hi,

Quoted 10 lines · Dmitry Torokhov

Hi Hans,

Quoted 7 lines · Hans de Goede

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

Quoted 3 lines

Quoted 1 line

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

Quoted 17 lines

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

Quoted 4 lines

		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.

Quoted 17 lines · Hans de Goede

Quoted 17 lines

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

Quoted 8 lines

Quoted 4 lines

		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.

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.

Quoted 27 lines · Maxime Ripard

Quoted 17 lines · Hans de Goede

Quoted 17 lines

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

Quoted 8 lines

Quoted 4 lines

		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.

Quoted 5 lines

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.

Quoted 22 lines · Hans de Goede

Quoted 21 lines

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

Quoted 3 lines

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.

Hi,

Quoted 27 lines · Maxime Ripard

Quoted 25 lines · Hans de Goede

Quoted 13 lines

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 {

Quoted 8 lines

			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.

Quoted 2 lines

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.

Quoted 8 lines

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.

Quoted 4 lines

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.

Quoted 3 lines

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.

Quoted 30 lines · Hans de Goede

Hi,

Quoted 27 lines · Maxime Ripard

Quoted 25 lines · Hans de Goede

Quoted 13 lines

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 {

Quoted 8 lines

			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.

Quoted 8 lines

Quoted 2 lines

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.

Quoted 22 lines

Quoted 9 lines

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.

Quoted 4 lines

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.

Quoted 4 lines

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.

Quoted 8 lines

Quoted 3 lines

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.

Quoted 5 lines

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.

Quoted 45 lines · Hans de Goede

Quoted 43 lines

Hi,

Quoted 26 lines · Maxime Ripard

Quoted 25 lines · Hans de Goede

Quoted 25 lines

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

Quoted 8 lines

Quoted 5 lines

Quoted 1 line

		};

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.

Quoted 4 lines

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.

Quoted 40 lines · Maxime Ripard

Quoted 38 lines · Hans de Goede

Hi,

Quoted 23 lines · Maxime Ripard

Quoted 22 lines · Hans de Goede

Quoted 21 lines

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

Quoted 6 lines

Quoted 3 lines

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.

Quoted 4 lines

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.

Quoted 42 lines

Quoted 21 lines

Quoted 8 lines

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.

Quoted 4 lines

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.

Quoted 4 lines

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.

Quoted 8 lines

Quoted 3 lines

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.

Quoted 5 lines

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.

Quoted 7 lines

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.

Signed-off-by Hans de Goede <[email protected]>

---
 arch/arm/boot/dts/sun5i-a10s-olinuxino-micro.dts | 7 +++++++
 arch/arm/boot/dts/sun5i-a10s.dtsi                | 7 +++++++
 arch/arm/boot/dts/sun5i-a13-olinuxino.dts        | 7 +++++++
 arch/arm/boot/dts/sun5i-a13.dtsi                 | 7 +++++++
 4 files changed, 28 insertions(+)

arch/arm/boot/dts/sun5i-a10s-olinuxino-micro.dts +0 -0

index e53fb12..c32162e 100644

Quoted 13 lines · Hans de Goede

Signed-off-by Hans de Goede <[email protected]>

---
  arch/arm/boot/dts/sun5i-a10s-olinuxino-micro.dts | 7 +++++++
  arch/arm/boot/dts/sun5i-a10s.dtsi                | 7 +++++++
  arch/arm/boot/dts/sun5i-a13-olinuxino.dts        | 7 +++++++
  arch/arm/boot/dts/sun5i-a13.dtsi                 | 7 +++++++
  4 files changed, 28 insertions(+)

arch/arm/boot/dts/sun5i-a10s-olinuxino-micro.dts +0 -0

index e53fb12..c32162e 100644

arch/arm/boot/dts/sun5i-a10s-olinuxino-micro.dts line 81

Quoted 7 lines

arch/arm/boot/dts/sun5i-a10s-olinuxino-micro.dts +4 -0

@@ -75,6 +75,13 @@
  			};
  		};
 
+		lradc: lradc at 01c22800 {
+			allwinner,chan0-step = <200>;
+			/* KEY_VOLUMEUP VOLUMEDOWN MENU ENTER HOME */
+			allwinner,chan0-keycodes = <115 114 139 28 102>;

Hi Hans

You might want to consider using arch/arm/boot/dts/include/dt-bindings/input/input.h

and then you could have the node:

arch/arm/boot/dts/sun5i-a10s-olinuxino-micro.dts

Quoted 3 lines

arch/arm/boot/dts/sun5i-a10s-olinuxino-micro.dts +3 -0

+		lradc: lradc at 01c22800 {
+			allwinner,chan0-step = <200>;
+			allwinner,chan0-keycodes = <KEY_VOLUMEUP VOLUMEDOWN MENU ENTER HOME>;

which is much more readable.