diff mbox

[v3,6/8] arm64: dts: rockchip: Add power key to GeekBox

Message ID 1457294038-14243-7-git-send-email-afaerber@suse.de (mailing list archive)
State New, archived
Headers show

Commit Message

Andreas Färber March 6, 2016, 7:53 p.m. UTC
Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 v2 -> v3:
 * Adopted wakeup-source instead of gpio-key,wakeup (Julien)
 * Dropped gpio-keys #address-cells and #size-cells properties (Julien)
 * Dropped power button reg property (Julien)
 * Adopted KEY_POWER (Julien)
 * Fixed power button pinctrl pull setting (Julien)
 
 v2: New
 
 arch/arm64/boot/dts/rockchip/rk3368-geekbox.dts | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

Comments

Julien Chauveau March 10, 2016, 11:04 p.m. UTC | #1
> Le 6 mars 2016 à 20:53, Andreas Färber <afaerber@suse.de> a écrit :
> 
> Signed-off-by: Andreas Färber <afaerber@suse.de>
> ---
> v2 -> v3:
> * Adopted wakeup-source instead of gpio-key,wakeup (Julien)
> * Dropped gpio-keys #address-cells and #size-cells properties (Julien)
> * Dropped power button reg property (Julien)
> * Adopted KEY_POWER (Julien)
> * Fixed power button pinctrl pull setting (Julien)
> 
> v2: New
> 
> arch/arm64/boot/dts/rockchip/rk3368-geekbox.dts | 20 ++++++++++++++++++++
> 1 file changed, 20 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/rockchip/rk3368-geekbox.dts b/arch/arm64/boot/dts/rockchip/rk3368-geekbox.dts
> index 098be3700a6f..7036b49c9206 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3368-geekbox.dts
> +++ b/arch/arm64/boot/dts/rockchip/rk3368-geekbox.dts
> @@ -42,6 +42,7 @@
> 
> /dts-v1/;
> #include "rk3368.dtsi"
> +#include <dt-bindings/input/input.h>
> 
> / {
> 	model = "GeekBox";
> @@ -70,6 +71,19 @@
> 		pinctrl-0 = <&ir_int>;
> 	};
> 
> +	keys: gpio-keys {

I think you don't need the "keys" label, because there’s no phandle that refers to this.

> +		compatible = "gpio-keys";
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&pwr_key>;
> +
> +		button@0 {

Here you should use "power" instead of "button@0".

> +			gpios = <&gpio0 2 GPIO_ACTIVE_LOW>;
> +			label = "GPIO Power";
> +			linux,code = <KEY_POWER>;

According to Documentation/input/event-codes.txt, there’s a special event type for the power button.
Should we use it here for that purpose?

			linux,input-type = <EV_PWR>

> +			wakeup-source;
> +		};
> +	};
> +
> 	leds: gpio-leds {
> 		compatible = "gpio-leds";
> 
> @@ -265,6 +279,12 @@
> 		};
> 	};
> 
> +	keys {
> +		pwr_key: pwr-key {
> +			rockchip,pins = <0 2 RK_FUNC_GPIO &pcfg_pull_none>;
> +		};
> +	};
> +
> 	pmic {
> 		pmic_sleep: pmic-sleep {
> 			rockchip,pins = <0 0 RK_FUNC_2 &pcfg_pull_none>;
> -- 
> 2.6.2
>
Julien Chauveau March 10, 2016, 11:09 p.m. UTC | #2
> Le 6 mars 2016 à 20:53, Andreas Färber <afaerber@suse.de> a écrit :
> 
> Signed-off-by: Andreas Färber <afaerber@suse.de>
> ---
> v2 -> v3:
> * Adopted wakeup-source instead of gpio-key,wakeup (Julien)
> * Dropped gpio-keys #address-cells and #size-cells properties (Julien)
> * Dropped power button reg property (Julien)
> * Adopted KEY_POWER (Julien)
> * Fixed power button pinctrl pull setting (Julien)
> 
> v2: New
> 
> arch/arm64/boot/dts/rockchip/rk3368-geekbox.dts | 20 ++++++++++++++++++++
> 1 file changed, 20 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/rockchip/rk3368-geekbox.dts b/arch/arm64/boot/dts/rockchip/rk3368-geekbox.dts
> index 098be3700a6f..7036b49c9206 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3368-geekbox.dts
> +++ b/arch/arm64/boot/dts/rockchip/rk3368-geekbox.dts
> @@ -42,6 +42,7 @@
> 
> /dts-v1/;
> #include "rk3368.dtsi"
> +#include <dt-bindings/input/input.h>
> 
> / {
> 	model = "GeekBox";
> @@ -70,6 +71,19 @@
> 		pinctrl-0 = <&ir_int>;
> 	};
> 
> +	keys: gpio-keys {

I think you don't need the "keys" label, because there’s no phandle that refers to this.

> +		compatible = "gpio-keys";
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&pwr_key>;
> +
> +		button@0 {

Here you should use "power" instead of "button@0".

> +			gpios = <&gpio0 2 GPIO_ACTIVE_LOW>;
> +			label = "GPIO Power";
> +			linux,code = <KEY_POWER>;

According to Documentation/input/event-codes.txt, there’s a special event type for the power button.
Should we use it here for that purpose?

			linux,input-type = <EV_PWR>

> +			wakeup-source;
> +		};
> +	};
> +
> 	leds: gpio-leds {
> 		compatible = "gpio-leds";
> 
> @@ -265,6 +279,12 @@
> 		};
> 	};
> 
> +	keys {
> +		pwr_key: pwr-key {
> +			rockchip,pins = <0 2 RK_FUNC_GPIO &pcfg_pull_none>;
> +		};
> +	};
> +
> 	pmic {
> 		pmic_sleep: pmic-sleep {
> 			rockchip,pins = <0 0 RK_FUNC_2 &pcfg_pull_none>;
> -- 
> 2.6.2
>
Andreas Färber March 16, 2016, 10:58 a.m. UTC | #3
Am 11.03.2016 um 00:04 schrieb Julien Chauveau:
>> Le 6 mars 2016 à 20:53, Andreas Färber <afaerber@suse.de> a écrit :
>>
>> Signed-off-by: Andreas Färber <afaerber@suse.de>
>> ---
>> v2 -> v3:
>> * Adopted wakeup-source instead of gpio-key,wakeup (Julien)
>> * Dropped gpio-keys #address-cells and #size-cells properties (Julien)
>> * Dropped power button reg property (Julien)
>> * Adopted KEY_POWER (Julien)
>> * Fixed power button pinctrl pull setting (Julien)
>>
>> v2: New
>>
>> arch/arm64/boot/dts/rockchip/rk3368-geekbox.dts | 20 ++++++++++++++++++++
>> 1 file changed, 20 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/rockchip/rk3368-geekbox.dts b/arch/arm64/boot/dts/rockchip/rk3368-geekbox.dts
>> index 098be3700a6f..7036b49c9206 100644
>> --- a/arch/arm64/boot/dts/rockchip/rk3368-geekbox.dts
>> +++ b/arch/arm64/boot/dts/rockchip/rk3368-geekbox.dts
>> @@ -42,6 +42,7 @@
>>
>> /dts-v1/;
>> #include "rk3368.dtsi"
>> +#include <dt-bindings/input/input.h>
>>
>> / {
>> 	model = "GeekBox";
>> @@ -70,6 +71,19 @@
>> 		pinctrl-0 = <&ir_int>;
>> 	};
>>
>> +	keys: gpio-keys {
> 
> I think you don't need the "keys" label, because there’s no phandle that refers to this.

As discussed elsewhere, there are four additional keys on the
Landingship (you proposed as sub-node names key1-key4).
I prefer preparing the label now over adding it in a later patch.

>> +		compatible = "gpio-keys";
>> +		pinctrl-names = "default";
>> +		pinctrl-0 = <&pwr_key>;
>> +
>> +		button@0 {
> 
> Here you should use "power" instead of "button@0".

Done.

>> +			gpios = <&gpio0 2 GPIO_ACTIVE_LOW>;
>> +			label = "GPIO Power";
>> +			linux,code = <KEY_POWER>;
> 
> According to Documentation/input/event-codes.txt, there’s a special event type for the power button.
> Should we use it here for that purpose?
> 
> 			linux,input-type = <EV_PWR>

The other RK3368 boards don't, so unless you can give a justification to
convert all boards yet again and test how this makes a difference, I'd
rather not do experiments here but leave that to someone who knows what
they're doing and then do it consistently...

Thanks for the detailed review,

Andreas

>> +			wakeup-source;
>> +		};
>> +	};
>> +
>> 	leds: gpio-leds {
>> 		compatible = "gpio-leds";
>>
>> @@ -265,6 +279,12 @@
>> 		};
>> 	};
>>
>> +	keys {
>> +		pwr_key: pwr-key {
>> +			rockchip,pins = <0 2 RK_FUNC_GPIO &pcfg_pull_none>;
>> +		};
>> +	};
>> +
>> 	pmic {
>> 		pmic_sleep: pmic-sleep {
>> 			rockchip,pins = <0 0 RK_FUNC_2 &pcfg_pull_none>;
Andreas Färber March 16, 2016, 1:52 p.m. UTC | #4
Am 16.03.2016 um 11:58 schrieb Andreas Färber:
> Am 11.03.2016 um 00:04 schrieb Julien Chauveau:
>>> @@ -70,6 +71,19 @@
>>> 		pinctrl-0 = <&ir_int>;
>>> 	};
>>>
>>> +	keys: gpio-keys {
[...]
>>> +		compatible = "gpio-keys";
>>> +		pinctrl-names = "default";
>>> +		pinctrl-0 = <&pwr_key>;
>>> +
>>> +		button@0 {
[...]
>>> +			gpios = <&gpio0 2 GPIO_ACTIVE_LOW>;
>>> +			label = "GPIO Power";
>>> +			linux,code = <KEY_POWER>;
>>
>> According to Documentation/input/event-codes.txt, there’s a special event type for the power button.
>> Should we use it here for that purpose?
>>
>> 			linux,input-type = <EV_PWR>
> 
> The other RK3368 boards don't, so unless you can give a justification to
> convert all boards yet again and test how this makes a difference, I'd
> rather not do experiments here but leave that to someone who knows what
> they're doing and then do it consistently...

For the record here's an evtest log:

geekbox:~ # evtest
No device specified, trying to scan all of /dev/input/event*
Available devices:
/dev/input/event0:      gpio_ir_recv
/dev/input/event1:      MCE IR Keyboard/Mouse (gpio-rc-recv)
/dev/input/event2:      gpio-keys
Select the device event number [0-2]: 2
Input driver version is 1.0.1
Input device ID: bus 0x19 vendor 0x1 product 0x1 version 0x100
Input device name: "gpio-keys"
Supported events:
  Event type 0 (EV_SYN)
  Event type 1 (EV_KEY)
    Event code 116 (KEY_POWER)
Properties:
Testing ... (interrupt to exit)
Event: time 1458136008.850429, type 1 (EV_KEY), code 116 (KEY_POWER),
value 1
Event: time 1458136008.850429, -------------- SYN_REPORT ------------

systemd then goes on to shut down the system cleanly.

Regards,
Andreas
diff mbox

Patch

diff --git a/arch/arm64/boot/dts/rockchip/rk3368-geekbox.dts b/arch/arm64/boot/dts/rockchip/rk3368-geekbox.dts
index 098be3700a6f..7036b49c9206 100644
--- a/arch/arm64/boot/dts/rockchip/rk3368-geekbox.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3368-geekbox.dts
@@ -42,6 +42,7 @@ 
 
 /dts-v1/;
 #include "rk3368.dtsi"
+#include <dt-bindings/input/input.h>
 
 / {
 	model = "GeekBox";
@@ -70,6 +71,19 @@ 
 		pinctrl-0 = <&ir_int>;
 	};
 
+	keys: gpio-keys {
+		compatible = "gpio-keys";
+		pinctrl-names = "default";
+		pinctrl-0 = <&pwr_key>;
+
+		button@0 {
+			gpios = <&gpio0 2 GPIO_ACTIVE_LOW>;
+			label = "GPIO Power";
+			linux,code = <KEY_POWER>;
+			wakeup-source;
+		};
+	};
+
 	leds: gpio-leds {
 		compatible = "gpio-leds";
 
@@ -265,6 +279,12 @@ 
 		};
 	};
 
+	keys {
+		pwr_key: pwr-key {
+			rockchip,pins = <0 2 RK_FUNC_GPIO &pcfg_pull_none>;
+		};
+	};
+
 	pmic {
 		pmic_sleep: pmic-sleep {
 			rockchip,pins = <0 0 RK_FUNC_2 &pcfg_pull_none>;