| Message ID | 20230405123813.2272919-1-pbrobinson@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: dts: rk3399-pinephone-pro: Add support for volume keys | expand |
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 >
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; >
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
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 >
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 > > > > > >
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,
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
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;