[PATCH] arm64: dts: rk3399-pinephone-pro: Add support for volume keys

[PATCH] arm64: dts: rk3399-pinephone-pro: Add support for volume keys

[Peter Robinson]

From: Ondrej Jirman <megi@xff.cz>

These are implemented via regular ADC, so regular polling is needed,
for these keys to work.

Signed-off-by: Martijn Braam <martijn@brixit.nl>
Co-developed-by: Kamil Trzciński <ayufan@ayufan.eu>
Signed-off-by: Ondrej Jirman <megi@xff.cz>
Signed-off-by: Peter Robinson <pbrobinson@gmail.com>
---
 .../dts/rockchip/rk3399-pinephone-pro.dts     | 26 +++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts b/arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts
index a0795a2b1cb1..ecd48040eb0c 100644
--- a/arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts
@@ -10,6 +10,7 @@
  */
 
 /dts-v1/;
+#include <dt-bindings/input/gpio-keys.h>
 #include <dt-bindings/input/linux-event-codes.h>
 #include "rk3399.dtsi"
 #include "rk3399-opp.dtsi"
@@ -29,6 +30,26 @@ chosen {
 		stdout-path = "serial2:115200n8";
 	};
 
+	adc-keys {
+		compatible = "adc-keys";
+		io-channels = <&saradc 1>;
+		io-channel-names = "buttons";
+		keyup-threshold-microvolt = <1600000>;
+		poll-interval = <100>;
+
+		button-up {
+			label = "Volume Up";
+			linux,code = <KEY_VOLUMEUP>;
+			press-threshold-microvolt = <100000>;
+		};
+
+		button-down {
+			label = "Volume Down";
+			linux,code = <KEY_VOLUMEDOWN>;
+			press-threshold-microvolt = <300000>;
+		};
+	};
+
 	gpio-keys {
 		compatible = "gpio-keys";
 		pinctrl-names = "default";
@@ -429,6 +450,11 @@ &sdio0 {
 	status = "okay";
 };
 
+&saradc {
+	vref-supply = <&vcca1v8_s3>;
+	status = "okay";
+};
+
 &sdmmc {
 	bus-width = <4>;
 	cap-sd-highspeed;
-- 
2.40.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64: dts: rk3399-pinephone-pro: Add support for volume keys

[Ondřej Jirman]

Hello Peter,

On Wed, Apr 05, 2023 at 01:38:13PM +0100, Peter Robinson wrote:
> From: Ondrej Jirman <megi@xff.cz>
> 
> These are implemented via regular ADC, so regular polling is needed,
> for these keys to work.
> 
> Signed-off-by: Martijn Braam <martijn@brixit.nl>
> Co-developed-by: Kamil Trzciński <ayufan@ayufan.eu>
> Signed-off-by: Ondrej Jirman <megi@xff.cz>
> Signed-off-by: Peter Robinson <pbrobinson@gmail.com>
> ---
>  .../dts/rockchip/rk3399-pinephone-pro.dts     | 26 +++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts b/arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts
> index a0795a2b1cb1..ecd48040eb0c 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts
> +++ b/arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts
> @@ -10,6 +10,7 @@
>   */
>  
>  /dts-v1/;
> +#include <dt-bindings/input/gpio-keys.h>
>  #include <dt-bindings/input/linux-event-codes.h>
>  #include "rk3399.dtsi"
>  #include "rk3399-opp.dtsi"
> @@ -29,6 +30,26 @@ chosen {
>  		stdout-path = "serial2:115200n8";
>  	};
>  
> +	adc-keys {
> +		compatible = "adc-keys";
> +		io-channels = <&saradc 1>;
> +		io-channel-names = "buttons";
> +		keyup-threshold-microvolt = <1600000>;
> +		poll-interval = <100>;
> +
> +		button-up {
> +			label = "Volume Up";
> +			linux,code = <KEY_VOLUMEUP>;
> +			press-threshold-microvolt = <100000>;
> +		};
> +
> +		button-down {
> +			label = "Volume Down";
> +			linux,code = <KEY_VOLUMEDOWN>;
> +			press-threshold-microvolt = <300000>;

I don't know about this... I've tried reading voltage values from:

  cd /sys/bus/iio/devices/iio:device0 (path may differ on your kernel)

  echo $((`cat in_voltage_scale`*`cat in_voltage1_raw`))

and I get various readings around the value 300 mV on both sides of the
threshold when pressing the vol down key. So this threshold may not be
good enough in practice.

Values I get for several different pushes of the button:

  293.5546875
  328.7109375
  332.2265625
  304.1015625
  297.0703125
  522.0703125

(I have to press quite hard to get bellow 300 and to get reliable detection
of volume down key press)

On development version of the phone, the value returned by sardac is less
variable. Basically either 298.828125 or 300.5859375 but it's also on
the edge.

I suggest raising the threshold to something like 600 and to do your own
testing, to get more data points. Unpressed value is ~1791.2109375 on both
phones, so 400 still gets a lot of headroom. And volume up is always < 15
in my tests.

Otherwise:

Tested-by: Ondrej Jirman <megi@xff.cz>

kind regards,
	o.

> +		};
> +	};
> +
>  	gpio-keys {
>  		compatible = "gpio-keys";
>  		pinctrl-names = "default";
> @@ -429,6 +450,11 @@ &sdio0 {
>  	status = "okay";
>  };
>  
> +&saradc {
> +	vref-supply = <&vcca1v8_s3>;
> +	status = "okay";
> +};
> +
>  &sdmmc {
>  	bus-width = <4>;
>  	cap-sd-highspeed;
> -- 
> 2.40.0
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64: dts: rk3399-pinephone-pro: Add support for volume keys

[Heiko Stübner]

Hi Peter, Ondrej,

Am Mittwoch, 5. April 2023, 15:53:39 CEST schrieb Ondřej Jirman:
> On Wed, Apr 05, 2023 at 01:38:13PM +0100, Peter Robinson wrote:
> > From: Ondrej Jirman <megi@xff.cz>
> > 
> > These are implemented via regular ADC, so regular polling is needed,
> > for these keys to work.
> > 
> > Signed-off-by: Martijn Braam <martijn@brixit.nl>
> > Co-developed-by: Kamil Trzciński <ayufan@ayufan.eu>
> > Signed-off-by: Ondrej Jirman <megi@xff.cz>
> > Signed-off-by: Peter Robinson <pbrobinson@gmail.com>
> > ---
> >  .../dts/rockchip/rk3399-pinephone-pro.dts     | 26 +++++++++++++++++++
> >  1 file changed, 26 insertions(+)
> > 
> > diff --git a/arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts b/arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts
> > index a0795a2b1cb1..ecd48040eb0c 100644
> > --- a/arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts
> > +++ b/arch/arm64/boot/dts/rockchip/rk3399-pinephone-pro.dts
> > @@ -10,6 +10,7 @@
> >   */
> >  
> >  /dts-v1/;
> > +#include <dt-bindings/input/gpio-keys.h>
> >  #include <dt-bindings/input/linux-event-codes.h>
> >  #include "rk3399.dtsi"
> >  #include "rk3399-opp.dtsi"
> > @@ -29,6 +30,26 @@ chosen {
> >  		stdout-path = "serial2:115200n8";
> >  	};
> >  
> > +	adc-keys {
> > +		compatible = "adc-keys";
> > +		io-channels = <&saradc 1>;
> > +		io-channel-names = "buttons";
> > +		keyup-threshold-microvolt = <1600000>;
> > +		poll-interval = <100>;
> > +
> > +		button-up {
> > +			label = "Volume Up";
> > +			linux,code = <KEY_VOLUMEUP>;
> > +			press-threshold-microvolt = <100000>;
> > +		};
> > +
> > +		button-down {
> > +			label = "Volume Down";
> > +			linux,code = <KEY_VOLUMEDOWN>;
> > +			press-threshold-microvolt = <300000>;
> 
> I don't know about this... I've tried reading voltage values from:
> 
>   cd /sys/bus/iio/devices/iio:device0 (path may differ on your kernel)
> 
>   echo $((`cat in_voltage_scale`*`cat in_voltage1_raw`))
> 
> and I get various readings around the value 300 mV on both sides of the
> threshold when pressing the vol down key. So this threshold may not be
> good enough in practice.
> 
> Values I get for several different pushes of the button:
> 
>   293.5546875
>   328.7109375
>   332.2265625
>   304.1015625
>   297.0703125
>   522.0703125
> 
> (I have to press quite hard to get bellow 300 and to get reliable detection
> of volume down key press)
> 
> On development version of the phone, the value returned by sardac is less
> variable. Basically either 298.828125 or 300.5859375 but it's also on
> the edge.
> 
> I suggest raising the threshold to something like 600 and to do your own
> testing, to get more data points. Unpressed value is ~1791.2109375 on both
> phones, so 400 still gets a lot of headroom. And volume up is always < 15
> in my tests.

did this get more attention meanwhile?

I don't have a Pinephone Pro myself, so you'll need to decide between you
about the value and the concern Ondrej raised here for the value.

Thanks
Heiko


> Otherwise:
> 
> Tested-by: Ondrej Jirman <megi@xff.cz>
> 
> kind regards,
> 	o.
> 
> > +		};
> > +	};
> > +
> >  	gpio-keys {
> >  		compatible = "gpio-keys";
> >  		pinctrl-names = "default";
> > @@ -429,6 +450,11 @@ &sdio0 {
> >  	status = "okay";
> >  };
> >  
> > +&saradc {
> > +	vref-supply = <&vcca1v8_s3>;
> > +	status = "okay";
> > +};
> > +
> >  &sdmmc {
> >  	bus-width = <4>;
> >  	cap-sd-highspeed;
> 





_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64: dts: rk3399-pinephone-pro: Add support for volume keys

[Ondřej Jirman]

Hello Heiko,

On Mon, Apr 17, 2023 at 10:34:20AM +0200, Heiko Stübner wrote:
> Hi Peter, Ondrej,
> 
> Am Mittwoch, 5. April 2023, 15:53:39 CEST schrieb Ondřej Jirman:
> > [...]
> >
> > (I have to press quite hard to get bellow 300 and to get reliable detection
> > of volume down key press)
> > 
> > On development version of the phone, the value returned by sardac is less
> > variable. Basically either 298.828125 or 300.5859375 but it's also on
> > the edge.
> > 
> > I suggest raising the threshold to something like 600 and to do your own
> > testing, to get more data points. Unpressed value is ~1791.2109375 on both
> > phones, so 400 still gets a lot of headroom. And volume up is always < 15
> > in my tests.
> 
> did this get more attention meanwhile?
> 
> I don't have a Pinephone Pro myself, so you'll need to decide between you
> about the value and the concern Ondrej raised here for the value.

It's safe and needed to use a higher value.

SAR ADC input is pulled high to 1.8V unless some key is pressed, so unpressed
value will always be around 1800 on all Pinephones, and pressed value will
depend on contact quality and tolerances. For volume down, SAR ADC input is fed
from a resistor divider of (10kOhm + 2kOhm) from 1.8V power rail. So that gives
2/12*1.8 = 0.3V. We can't have the press detection threshold right at this
voltage, because:

1) these resistors have tolerances that will randomly result in measured voltage
   being above or below the 0.3V on real devices (-1% on 10k and +1% on 2k =
   2*1.01/(10*0.99+2*1.01)*1.8 = 305 mV - already too high even without
   considering switch contact quality), and

2) those piddly membrane switches apparently have their own random resistance
   that is added to the bottom leg of the resistor divider, and depends on
   strenght of the press on some devices (and switches may develop higher
   resistance with age/use).

Schematic: https://megous.com/dl/tmp/1125d9248a8213b3.png

kind regards,
	o.

> Thanks
> Heiko

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64: dts: rk3399-pinephone-pro: Add support for volume keys

[Heiko Stübner]

Am Montag, 17. April 2023, 14:37:16 CEST schrieb Ondřej Jirman:
> Hello Heiko,
> 
> On Mon, Apr 17, 2023 at 10:34:20AM +0200, Heiko Stübner wrote:
> > Hi Peter, Ondrej,
> > 
> > Am Mittwoch, 5. April 2023, 15:53:39 CEST schrieb Ondřej Jirman:
> > > [...]
> > >
> > > (I have to press quite hard to get bellow 300 and to get reliable detection
> > > of volume down key press)
> > > 
> > > On development version of the phone, the value returned by sardac is less
> > > variable. Basically either 298.828125 or 300.5859375 but it's also on
> > > the edge.
> > > 
> > > I suggest raising the threshold to something like 600 and to do your own
> > > testing, to get more data points. Unpressed value is ~1791.2109375 on both
> > > phones, so 400 still gets a lot of headroom. And volume up is always < 15
> > > in my tests.
> > 
> > did this get more attention meanwhile?
> > 
> > I don't have a Pinephone Pro myself, so you'll need to decide between you
> > about the value and the concern Ondrej raised here for the value.
> 
> It's safe and needed to use a higher value.

so in a nutshell, if I change "Volume Down" to say 400000 instead of the
current 300000, the patch is good to go?

I.e. that was my main question :-), as you raised the objection to the value
in your initial reply

Thanks
Heiko



> SAR ADC input is pulled high to 1.8V unless some key is pressed, so unpressed
> value will always be around 1800 on all Pinephones, and pressed value will
> depend on contact quality and tolerances. For volume down, SAR ADC input is fed
> from a resistor divider of (10kOhm + 2kOhm) from 1.8V power rail. So that gives
> 2/12*1.8 = 0.3V. We can't have the press detection threshold right at this
> voltage, because:
> 
> 1) these resistors have tolerances that will randomly result in measured voltage
>    being above or below the 0.3V on real devices (-1% on 10k and +1% on 2k =
>    2*1.01/(10*0.99+2*1.01)*1.8 = 305 mV - already too high even without
>    considering switch contact quality), and
> 
> 2) those piddly membrane switches apparently have their own random resistance
>    that is added to the bottom leg of the resistor divider, and depends on
>    strenght of the press on some devices (and switches may develop higher
>    resistance with age/use).
> 
> Schematic: https://megous.com/dl/tmp/1125d9248a8213b3.png
> 
> kind regards,
> 	o.
> 
> > Thanks
> > Heiko
> 





_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64: dts: rk3399-pinephone-pro: Add support for volume keys

[Ondřej Jirman]

On Mon, Apr 17, 2023 at 04:36:55PM +0200, Heiko Stübner wrote:
> Am Montag, 17. April 2023, 14:37:16 CEST schrieb Ondřej Jirman:
> > Hello Heiko,
> > 
> > On Mon, Apr 17, 2023 at 10:34:20AM +0200, Heiko Stübner wrote:
> > > Hi Peter, Ondrej,
> > > 
> > > Am Mittwoch, 5. April 2023, 15:53:39 CEST schrieb Ondřej Jirman:
> > > > [...]
> > > >
> > > > (I have to press quite hard to get bellow 300 and to get reliable detection
> > > > of volume down key press)
> > > > 
> > > > On development version of the phone, the value returned by sardac is less
> > > > variable. Basically either 298.828125 or 300.5859375 but it's also on
> > > > the edge.
> > > > 
> > > > I suggest raising the threshold to something like 600 and to do your own
> > > > testing, to get more data points. Unpressed value is ~1791.2109375 on both
> > > > phones, so 400 still gets a lot of headroom. And volume up is always < 15
> > > > in my tests.
> > > 
> > > did this get more attention meanwhile?
> > > 
> > > I don't have a Pinephone Pro myself, so you'll need to decide between you
> > > about the value and the concern Ondrej raised here for the value.
> > 
> > It's safe and needed to use a higher value.
> 
> so in a nutshell, if I change "Volume Down" to say 400000 instead of the
> current 300000, the patch is good to go?

My suggestion was 600 mV so 600000. :) Otherwise, yes.

kind regards,
	o.

> I.e. that was my main question :-), as you raised the objection to the value
> in your initial reply
> 
> Thanks
> Heiko
> 
> 
> 
> > SAR ADC input is pulled high to 1.8V unless some key is pressed, so unpressed
> > value will always be around 1800 on all Pinephones, and pressed value will
> > depend on contact quality and tolerances. For volume down, SAR ADC input is fed
> > from a resistor divider of (10kOhm + 2kOhm) from 1.8V power rail. So that gives
> > 2/12*1.8 = 0.3V. We can't have the press detection threshold right at this
> > voltage, because:
> > 
> > 1) these resistors have tolerances that will randomly result in measured voltage
> >    being above or below the 0.3V on real devices (-1% on 10k and +1% on 2k =
> >    2*1.01/(10*0.99+2*1.01)*1.8 = 305 mV - already too high even without
> >    considering switch contact quality), and
> > 
> > 2) those piddly membrane switches apparently have their own random resistance
> >    that is added to the bottom leg of the resistor divider, and depends on
> >    strenght of the press on some devices (and switches may develop higher
> >    resistance with age/use).
> > 
> > Schematic: https://megous.com/dl/tmp/1125d9248a8213b3.png
> > 
> > kind regards,
> > 	o.
> > 
> > > Thanks
> > > Heiko
> > 
> 
> 
> 
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64: dts: rk3399-pinephone-pro: Add support for volume keys

[Heiko Stuebner]

On Wed, 5 Apr 2023 13:38:13 +0100, Peter Robinson wrote:
> From: Ondrej Jirman <megi@xff.cz>
> 
> These are implemented via regular ADC, so regular polling is needed,
> for these keys to work.
> 
> 

Applied, thanks!

[1/1] arm64: dts: rk3399-pinephone-pro: Add support for volume keys
      commit: d3150ed535805403291b95fd84b00b0b5ef41096

With Volume Down increased to 600mV as suggested by Ondrej

Best regards,
-- 
Heiko Stuebner <heiko@sntech.de>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64: dts: rk3399-pinephone-pro: Add support for volume keys

[Peter Robinson]

On Mon, Apr 17, 2023 at 11:43 PM Heiko Stuebner <heiko@sntech.de> wrote:
>
> On Wed, 5 Apr 2023 13:38:13 +0100, Peter Robinson wrote:
> > From: Ondrej Jirman <megi@xff.cz>
> >
> > These are implemented via regular ADC, so regular polling is needed,
> > for these keys to work.
> >
> >
>
> Applied, thanks!
>
> [1/1] arm64: dts: rk3399-pinephone-pro: Add support for volume keys
>       commit: d3150ed535805403291b95fd84b00b0b5ef41096
>
> With Volume Down increased to 600mV as suggested by Ondrej

Thanks folks

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel