linux-kernel - Re: [PATCH] arm64: dts: rockchip: Add power button for RK3399 Puma

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <54c49375-cb2a-40fe-abcd-fc56c04d0c53@cherry.de>
Date: Thu, 26 Sep 2024 14:34:30 +0200
From: Quentin Schulz <quentin.schulz@...rry.de>
To: Daniel Semkowicz <dse@...umatec.com>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Heiko Stuebner <heiko@...ech.de>
Cc: Dragan Simic <dsimic@...jaro.org>,
 Farouk Bouabid <farouk.bouabid@...obroma-systems.com>,
 Quentin Schulz <quentin.schulz@...obroma-systems.com>,
 Vahe Grigoryan <vahe.grigoryan@...obroma-systems.com>,
 devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
 linux-kernel@...r.kernel.org, linux-rockchip@...ts.infradead.org
Subject: Re: [PATCH] arm64: dts: rockchip: Add power button for RK3399 Puma

Hi Daniel,

On 9/25/24 9:28 AM, Daniel Semkowicz wrote:
> There is a PWRBTN# input pin exposed on a Q7 connector. The pin
> is routed to a GPIO0_A1 through a diode. Q7 specification describes
> the PWRBTN# pin as a Power Button signal.
> Configure the pin as KEY_POWER, so it can function as power button and
> trigger device shutdown.
> Add the pin definition to RK3399 Puma dts, so it can be reused
> by derived platforms, but keep it disabled by default.
> 
> Enable the power button input on Haikou development board.
> 
> Signed-off-by: Daniel Semkowicz <dse@...umatec.com>

This works, thanks.

Tested-by: Quentin Schulz <quentin.schulz@...rry.de>

Now I have some questions I wasn't able to answer myself, maybe someone 
can provide some feedback on those :)

We already have a gpio-keys for buttons on Haikou, c.f. 
https://elixir.bootlin.com/linux/v6.11/source/arch/arm64/boot/dts/rockchip/rk3399-puma-haikou.dts#L22. 
Those signals are directly routed to the SoM and follow the Qseven standard.

The same applies to PWRBTN# signal.

However, here we have one gpio-keys for PWRBTN# in Puma DTSI and one 
gpio-keys for the buttons and sliders on Haikou devkit in Haikou DTS.

I'm a bit undecided on where this should go.

Having all button/slider signals following the Qseven standard in Puma 
DTSI and enable the gpio-keys only in the devkit would make sense to me, 
so that other baseboards could easily make use of it. However, things 
get complicated if the baseboard manufacturer decides to only implement 
**some** of the signals, for which we then need to remove some nodes 
from gpio-keys (and pinctrl entries) since gpio-keys doesn't check the 
"status" property in its child nodes (though that could be fixed). At 
which point, it's not entirely clear if having it in Puma DTSI is 
actually beneficial.

Someone has an opinion/recommendation on that?

Cheers,
Quentin

> ---
> 
>   .../boot/dts/rockchip/rk3399-puma-haikou.dts  |  4 ++++
>   arch/arm64/boot/dts/rockchip/rk3399-puma.dtsi | 22 +++++++++++++++++++
>   2 files changed, 26 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/rockchip/rk3399-puma-haikou.dts b/arch/arm64/boot/dts/rockchip/rk3399-puma-haikou.dts
> index f6f15946579e..0999026b16d0 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3399-puma-haikou.dts
> +++ b/arch/arm64/boot/dts/rockchip/rk3399-puma-haikou.dts
> @@ -143,6 +143,10 @@ vddd_codec: vddd-codec {
>   	};
>   };
>   
> +&gpio_key_power {
> +	status = "okay";
> +};
> +
>   &hdmi {
>   	ddc-i2c-bus = <&i2c3>;
>   	status = "okay";
> diff --git a/arch/arm64/boot/dts/rockchip/rk3399-puma.dtsi b/arch/arm64/boot/dts/rockchip/rk3399-puma.dtsi
> index 650b1ba9c192..389ffe604e74 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3399-puma.dtsi
> +++ b/arch/arm64/boot/dts/rockchip/rk3399-puma.dtsi
> @@ -3,6 +3,7 @@
>    * Copyright (c) 2017 Theobroma Systems Design und Consulting GmbH
>    */
>   
> +#include <dt-bindings/input/input.h>
>   #include <dt-bindings/pwm/pwm.h>
>   #include "rk3399.dtsi"
>   
> @@ -39,6 +40,19 @@ clkin_gmac: external-gmac-clock {
>   		#clock-cells = <0>;
>   	};
>   
> +	gpio_key_power: gpio-key-power {
> +		compatible = "gpio-keys";
> +		pinctrl-0 = <&pwrbtn_pin>;
> +		pinctrl-names = "default";
> +		status = "disabled";
> +
> +		button-pwrbtn-n {
> +			gpios = <&gpio0 RK_PA1 GPIO_ACTIVE_LOW>;
> +			label = "PWRBTN#";
> +			linux,code = <KEY_POWER>;
> +		};
> +	};
> +
>   	vcc1v2_phy: vcc1v2-phy {
>   		compatible = "regulator-fixed";
>   		regulator-name = "vcc1v2_phy";
> @@ -475,6 +489,14 @@ &pinctrl {
>   	pinctrl-names = "default";
>   	pinctrl-0 = <&q7_thermal_pin &bios_disable_override_hog_pin>;
>   
> +	buttons {
> +		pwrbtn_pin: pwrbtn-pin {
> +			rockchip,pins =
> +				/* PWRBTN# */
> +				<0 RK_PA1 RK_FUNC_GPIO &pcfg_pull_up>;
> +		};
> +	};
> +
>   	gpios {
>   		bios_disable_override_hog_pin: bios-disable-override-hog-pin {
>   			rockchip,pins =