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

From: "Ondřej Jirman" <megi@xff.cz>
To: Peter Robinson <pbrobinson@gmail.com>
Cc: "Rob Herring" <robh+dt@kernel.org>,
	"Krzysztof Kozlowski" <krzysztof.kozlowski+dt@linaro.org>,
	"Heiko Stuebner" <heiko@sntech.de>,
	"Tom Fitzhenry" <tom@tom-fitzhenry.me.uk>,
	"Martijn Braam" <martijn@brixit.nl>,
	"Caleb Connolly" <kc@postmarketos.org>,
	"Jarrah Gosbell" <kernel@undef.tools>,
	devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	linux-rockchip@lists.infradead.org,
	"Kamil Trzciński" <ayufan@ayufan.eu>
Subject: Re: [PATCH] arm64: dts: rk3399-pinephone-pro: Add support for volume keys
Date: Wed, 5 Apr 2023 15:53:39 +0200	[thread overview]
Message-ID: <20230405135339.bcdyjdbtynuwf5yz@core> (raw)
In-Reply-To: <20230405123813.2272919-1-pbrobinson@gmail.com>

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