Re: [PATCH] arm64: dts: qcom: sm8350-sagami: Rectify GPIO keys

[Konrad Dybcio <konrad.dybcio@linaro.org>]

On 13.12.2022 10:03, Marijn Suijten wrote:
> On 2022-12-10 14:38:50, Konrad Dybcio wrote:
>> With enough pins set properly, the hardware buttons now also work
>> like a charm.
>>
>> Fixes: c2721b0c23d9 ("arm64: dts: qcom: Add support for Xperia 1 III / 5 III")
>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> 
> Tested-by: Marijn Suijten <marijn.suijten@somainline.org> # On Xperia 5 III
> 
> However I also tested this on Xperia 1 III, and...
> 
>> ---
>>  .../qcom/sm8350-sony-xperia-sagami-pdx214.dts | 24 ++++++++++
>>  .../dts/qcom/sm8350-sony-xperia-sagami.dtsi   | 47 ++++++++++++++++++-
>>  2 files changed, 70 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/sm8350-sony-xperia-sagami-pdx214.dts b/arch/arm64/boot/dts/qcom/sm8350-sony-xperia-sagami-pdx214.dts
>> index e6824c8c2774..6fa830bdc6bd 100644
>> --- a/arch/arm64/boot/dts/qcom/sm8350-sony-xperia-sagami-pdx214.dts
>> +++ b/arch/arm64/boot/dts/qcom/sm8350-sony-xperia-sagami-pdx214.dts
>> @@ -10,6 +10,20 @@
>>  / {
>>  	model = "Sony Xperia 5 III";
>>  	compatible = "sony,pdx214-generic", "qcom,sm8350";
>> +
>> +	gpio-keys {
>> +		pinctrl-names = "default";
>> +		pinctrl-0 = <&focus_n &snapshot_n &vol_down_n &g_assist_n>;
>> +
>> +		key-google-assist {
>> +			label = "Google Assistant Key";
>> +			gpios = <&pm8350_gpios 9 GPIO_ACTIVE_LOW>;
>> +			linux,code = <KEY_LEFTMETA>;
>> +			debounce-interval = <15>;
>> +			linux,can-disable;
>> +			gpio-key,wakeup;
>> +		};
>> +	};
> 
> ... please move this to board DTS.  Xperia 1 III would also like to have
> it mapped.  Downstream DT indicates, and my local testing confirms, that
> it is identical to Xperia 5 III (i.e. common to the Sagami board).
> 
> The other buttons work great on both devices!
While I'd agree that having it like that would be nice for completeness,
it's:

- not necessary, as the button is not physically there, so the user will
never come into contact with it

- it will leak power, the plan is to park all unused pins after we get
"good enough" support for our devices (not now, so as not to mess with
ourselves in the dev process)

Konrad
> 
>>  };
>>  
>>  &framebuffer {
>> @@ -18,6 +32,16 @@ &framebuffer {
>>  	stride = <(1080 * 4)>;
>>  };
>>  
>> +&pm8350_gpios {
>> +	g_assist_n: g-assist-n-state {
>> +		pins = "gpio9";
>> +		function = "normal";
>> +		power-source = <1>;
>> +		bias-pull-up;
>> +		input-enable;
>> +	};
>> +};
>> +
> 
> (And this too, of course)
> 
> - Marijn
> 
>>  &pm8350b_gpios {
>>  	gpio-line-names = "NC", /* GPIO_1 */
>>  			  "NC",
>> diff --git a/arch/arm64/boot/dts/qcom/sm8350-sony-xperia-sagami.dtsi b/arch/arm64/boot/dts/qcom/sm8350-sony-xperia-sagami.dtsi
>> index d73e1f3fa501..557b4538a031 100644
>> --- a/arch/arm64/boot/dts/qcom/sm8350-sony-xperia-sagami.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sm8350-sony-xperia-sagami.dtsi
>> @@ -49,7 +49,26 @@ framebuffer: framebuffer@e1000000 {
>>  	gpio-keys {
>>  		compatible = "gpio-keys";
>>  
>> -		/* For reasons still unknown, GAssist key and Camera Focus/Shutter don't work.. */
>> +		pinctrl-names = "default";
>> +		pinctrl-0 = <&focus_n &snapshot_n &vol_down_n>;
>> +
>> +		key-camera-focus {
>> +			label = "Camera Focus";
>> +			linux,code = <KEY_CAMERA_FOCUS>;
>> +			gpios = <&pm8350b_gpios 8 GPIO_ACTIVE_LOW>;
>> +			debounce-interval = <15>;
>> +			linux,can-disable;
>> +			gpio-key,wakeup;
>> +		};
>> +
>> +		key-camera-snapshot {
>> +			label = "Camera Snapshot";
>> +			linux,code = <KEY_CAMERA>;
>> +			gpios = <&pm8350b_gpios 5 GPIO_ACTIVE_LOW>;
>> +			debounce-interval = <15>;
>> +			linux,can-disable;
>> +			gpio-key,wakeup;
>> +		};
>>  
>>  		key-vol-down {
>>  			label = "Volume Down";
>> @@ -547,11 +566,37 @@ &pm8350_gpios {
>>  			  "PM8350_OPTION"; /* GPIO_10 */
>>  };
>>  
>> +&pm8350b_gpios {
>> +	snapshot_n: snapshot-n-state {
>> +		pins = "gpio5";
>> +		function = "normal";
>> +		power-source = <0>;
>> +		bias-pull-up;
>> +		input-enable;
>> +	};
>> +
>> +	focus_n: focus-n-state {
>> +		pins = "gpio8";
>> +		function = "normal";
>> +		power-source = <0>;
>> +		input-enable;
>> +		bias-pull-up;
>> +	};
>> +};
>> +
>>  &pmk8350_gpios {
>>  	gpio-line-names = "NC", /* GPIO_1 */
>>  			  "NC",
>>  			  "VOL_DOWN_N",
>>  			  "PMK8350_OPTION";
>> +
>> +	vol_down_n: vol-down-n-state {
>> +		pins = "gpio3";
>> +		function = "normal";
>> +		power-source = <0>;
>> +		bias-pull-up;
>> +		input-enable;
>> +	};
>>  };
>>  
>>  &pmk8350_rtc {
>> -- 
>> 2.38.1
>>