diff mbox series

[v2,3/3] arm64: dts: qcom: sdm845-oneplus: add tri-state-key

Message ID 20230209232556.91554-1-soyer@irl.hu
State New
Headers
Series Add tri-state-key for oneplus6 |

Commit Message

Gergo Koteles Feb. 9, 2023, 11:25 p.m. UTC 
  The tri-state-key is a sound profile switch found on the OnePlus 6,
Android maps the states to "mute", "vibrate" and "ring". Expose them as
ABS_SND_PROFILE events.
The previous GPIO numbers were wrong. Update them to the correct
ones.

Co-developed-by: Caleb Connolly <caleb@connolly.tech>
Signed-off-by: Caleb Connolly <caleb@connolly.tech>
Signed-off-by: Gergo Koteles <soyer@irl.hu>
---
 .../boot/dts/qcom/sdm845-oneplus-common.dtsi  | 39 ++++++++++++++++++-
 1 file changed, 38 insertions(+), 1 deletion(-)

Comments

Krzysztof Kozlowski Feb. 10, 2023, 11:33 a.m. UTC | #1 
On 10/02/2023 00:25, Gergo Koteles wrote:
> The tri-state-key is a sound profile switch found on the OnePlus 6,
> Android maps the states to "mute", "vibrate" and "ring". Expose them as
> ABS_SND_PROFILE events.
> The previous GPIO numbers were wrong. Update them to the correct
> ones.
> 
> Co-developed-by: Caleb Connolly <caleb@connolly.tech>
> Signed-off-by: Caleb Connolly <caleb@connolly.tech>
> Signed-off-by: Gergo Koteles <soyer@irl.hu>

Where are other patches? I got only 3/3.

> ---
>  .../boot/dts/qcom/sdm845-oneplus-common.dtsi  | 39 ++++++++++++++++++-
>  1 file changed, 38 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sdm845-oneplus-common.dtsi b/arch/arm64/boot/dts/qcom/sdm845-oneplus-common.dtsi
> index 64638ea94db7..e45d4fdead82 100644
> --- a/arch/arm64/boot/dts/qcom/sdm845-oneplus-common.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sdm845-oneplus-common.dtsi
> @@ -52,6 +52,43 @@ key-vol-up {
>  		};
>  	};
>  
> +	tri-state-key {
> +		compatible = "gpio-keys";
> +		label = "Tri-state key";
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&tri_state_key_default>;

Missing blank line.

> +		state-top {

Does not look like you tested the DTS against bindings. Please run `make
dtbs_check` (see Documentation/devicetree/bindings/writing-schema.rst
for instructions).


Best regards,
Krzysztof
Gergo Koteles Feb. 10, 2023, 1:45 p.m. UTC | #2 
On 2023. 02. 10. 12:33, Krzysztof Kozlowski wrote:
> On 10/02/2023 00:25, Gergo Koteles wrote:
>> The tri-state-key is a sound profile switch found on the OnePlus 6,
>> Android maps the states to "mute", "vibrate" and "ring". Expose them as
>> ABS_SND_PROFILE events.
>> The previous GPIO numbers were wrong. Update them to the correct
>> ones.
>>
>> Co-developed-by: Caleb Connolly <caleb@connolly.tech>
>> Signed-off-by: Caleb Connolly <caleb@connolly.tech>
>> Signed-off-by: Gergo Koteles <soyer@irl.hu>
> 
> Where are other patches? I got only 3/3.
> 
Hi Krzysztof,

Sorry, I missed the --thread option for git format-patch.

>> ---
>>   .../boot/dts/qcom/sdm845-oneplus-common.dtsi  | 39 ++++++++++++++++++-
>>   1 file changed, 38 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/sdm845-oneplus-common.dtsi b/arch/arm64/boot/dts/qcom/sdm845-oneplus-common.dtsi
>> index 64638ea94db7..e45d4fdead82 100644
>> --- a/arch/arm64/boot/dts/qcom/sdm845-oneplus-common.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sdm845-oneplus-common.dtsi
>> @@ -52,6 +52,43 @@ key-vol-up {
>>   		};
>>   	};
>>   
>> +	tri-state-key {
>> +		compatible = "gpio-keys";
>> +		label = "Tri-state key";
>> +		pinctrl-names = "default";
>> +		pinctrl-0 = <&tri_state_key_default>;
> 
> Missing blank line.
> 

I'll add it to v3.

>> +		state-top {
> 
> Does not look like you tested the DTS against bindings. Please run `make
> dtbs_check` (see Documentation/devicetree/bindings/writing-schema.rst
> for instructions).
> 

I ran dtbs_check with 
DT_SCHEMA_FILES=Documentation/devicetree/bindings/arm/qcom.yaml. It only 
shows warnings for msm8996-oneplus3, but not for sdm845-oneplus phones. 
Is there anything else I need to check?


...
arch/arm64/boot/dts/qcom/msm8996-oneplus3.dtb: /: qcom,board-id: 'oneOf' 
conditional failed, one must be fixed:
	[8, 0, 15801, 15, 8, 0, 15801, 16] is too long
	From schema: /Documentation/devicetree/bindings/arm/qcom.yaml
...
   DTC_CHK arch/arm64/boot/dts/qcom/sdm845-oneplus-enchilada.dtb
   DTC_CHK arch/arm64/boot/dts/qcom/sdm845-oneplus-fajita.dtb


Thanks,
Gergo

> 
> Best regards,
> Krzysztof
>
Konrad Dybcio Feb. 10, 2023, 1:53 p.m. UTC | #3 
On 10.02.2023 14:45, Gergo Koteles wrote:
> On 2023. 02. 10. 12:33, Krzysztof Kozlowski wrote:
>> On 10/02/2023 00:25, Gergo Koteles wrote:
>>> The tri-state-key is a sound profile switch found on the OnePlus 6,
>>> Android maps the states to "mute", "vibrate" and "ring". Expose them as
>>> ABS_SND_PROFILE events.
>>> The previous GPIO numbers were wrong. Update them to the correct
>>> ones.
>>>
>>> Co-developed-by: Caleb Connolly <caleb@connolly.tech>
>>> Signed-off-by: Caleb Connolly <caleb@connolly.tech>
>>> Signed-off-by: Gergo Koteles <soyer@irl.hu>
>>
>> Where are other patches? I got only 3/3.
>>
> Hi Krzysztof,
> 
> Sorry, I missed the --thread option for git format-patch.
> 
>>> ---
>>>   .../boot/dts/qcom/sdm845-oneplus-common.dtsi  | 39 ++++++++++++++++++-
>>>   1 file changed, 38 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/arch/arm64/boot/dts/qcom/sdm845-oneplus-common.dtsi b/arch/arm64/boot/dts/qcom/sdm845-oneplus-common.dtsi
>>> index 64638ea94db7..e45d4fdead82 100644
>>> --- a/arch/arm64/boot/dts/qcom/sdm845-oneplus-common.dtsi
>>> +++ b/arch/arm64/boot/dts/qcom/sdm845-oneplus-common.dtsi
>>> @@ -52,6 +52,43 @@ key-vol-up {
>>>           };
>>>       };
>>>   +    tri-state-key {
>>> +        compatible = "gpio-keys";
>>> +        label = "Tri-state key";
>>> +        pinctrl-names = "default";
>>> +        pinctrl-0 = <&tri_state_key_default>;
>>
>> Missing blank line.
>>
> 
> I'll add it to v3.
While at it, please put pinctrl-names after pinctrl-0.

> 
>>> +        state-top {
>>
>> Does not look like you tested the DTS against bindings. Please run `make
>> dtbs_check` (see Documentation/devicetree/bindings/writing-schema.rst
>> for instructions).
>>
> 
> I ran dtbs_check with DT_SCHEMA_FILES=Documentation/devicetree/bindings/arm/qcom.yaml. It only shows warnings for msm8996-oneplus3, but not for sdm845-oneplus phones. Is there anything else I need to check?
You're only checking against a schema file which validates msm-id and
machine compatibles. The goal is to not introduce *any* new warnings.

You want to run:

make (your make args) CHECK_DTBS=1 qcom/sdm845-oneplus-enchilada.dtb

pre and post your patch.

Konrad

> 
> 
> ...
> arch/arm64/boot/dts/qcom/msm8996-oneplus3.dtb: /: qcom,board-id: 'oneOf' conditional failed, one must be fixed:
>     [8, 0, 15801, 15, 8, 0, 15801, 16] is too long
>     From schema: /Documentation/devicetree/bindings/arm/qcom.yaml
> ...
>   DTC_CHK arch/arm64/boot/dts/qcom/sdm845-oneplus-enchilada.dtb
>   DTC_CHK arch/arm64/boot/dts/qcom/sdm845-oneplus-fajita.dtb
> 
> 
> Thanks,
> Gergo
> 
>>
>> Best regards,
>> Krzysztof
>>
>
Pavel Machek Feb. 11, 2023, 4:40 p.m. UTC | #4 
Hi!

> +++ b/arch/arm64/boot/dts/qcom/sdm845-oneplus-common.dtsi
> @@ -52,6 +52,43 @@ key-vol-up {
>  		};
>  	};
>  
> +	tri-state-key {
> +		compatible = "gpio-keys";
> +		label = "Tri-state key";
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&tri_state_key_default>;
> +		state-top {
> +			label = "Tri-state key top";

"top/middle" is not too useful. Do we need the label at all? If so,
should it say "loud/vibrations only/mute"?

BR,
								Pavel
Caleb Connolly Feb. 12, 2023, 1:58 a.m. UTC | #5 
On 11/02/2023 16:40, Pavel Machek wrote:
> Hi!
>
>> +++ b/arch/arm64/boot/dts/qcom/sdm845-oneplus-common.dtsi
>> @@ -52,6 +52,43 @@ key-vol-up {
>>  		};
>>  	};
>>
>> +	tri-state-key {
>> +		compatible = "gpio-keys";
>> +		label = "Tri-state key";
>> +		pinctrl-names = "default";
>> +		pinctrl-0 = <&tri_state_key_default>;
>> +		state-top {
>> +			label = "Tri-state key top";
>
> "top/middle" is not too useful. Do we need the label at all? If so,
> should it say "loud/vibrations only/mute"?

"mute", "vibrate" and "ring" sound good to me.

Although it would be nice if users can easily map the physical key
position to the action when viewing the input device or remapping the
key in userspace.

Do you have any ideas or recommendations on how to do this?
>
> BR,
> 								Pavel

--
Kind Regards,
Caleb
Gergo Koteles Feb. 16, 2023, 3:32 a.m. UTC | #6 
Hi,

> 
> 
> On 11/02/2023 16:40, Pavel Machek wrote:
>> Hi!
>>
>>> +++ b/arch/arm64/boot/dts/qcom/sdm845-oneplus-common.dtsi
>>> @@ -52,6 +52,43 @@ key-vol-up {
>>>   		};
>>>   	};
>>>
>>> +	tri-state-key {
>>> +		compatible = "gpio-keys";
>>> +		label = "Tri-state key";
>>> +		pinctrl-names = "default";
>>> +		pinctrl-0 = <&tri_state_key_default>;
>>> +		state-top {
>>> +			label = "Tri-state key top";
>>
>> "top/middle" is not too useful. Do we need the label at all? If so,
>> should it say "loud/vibrations only/mute"?
> 
> "mute", "vibrate" and "ring" sound good to me.
> 

OnePlus uses the silent/vibrate/ring, iPhone the silent/ring names.
Maybe silent/vibrate/ring are more familiar.

Adding labels can document these modes here.
Should we also document these in input-event-codes.h?
#define ABS_SND_PROFILE		0x22 /* 0 = silent; 1 = vibrate; 2 = ring */


Thanks,
Gergo

> Although it would be nice if users can easily map the physical key
> position to the action when viewing the input device or remapping the
> key in userspace.
> 
> Do you have any ideas or recommendations on how to do this?
>>
>> BR,
>> 								Pavel
> 
> --
> Kind Regards,
> Caleb
>
Caleb Connolly Feb. 19, 2023, 3:30 p.m. UTC | #7 
On 16/02/2023 03:32, Gergo Koteles wrote:
> Hi,
>
>>
>>
>> On 11/02/2023 16:40, Pavel Machek wrote:
>>> Hi!
>>>
>>>> +++ b/arch/arm64/boot/dts/qcom/sdm845-oneplus-common.dtsi
>>>> @@ -52,6 +52,43 @@ key-vol-up {
>>>>   		};
>>>>   	};
>>>>
>>>> +	tri-state-key {
>>>> +		compatible = "gpio-keys";
>>>> +		label = "Tri-state key";
>>>> +		pinctrl-names = "default";
>>>> +		pinctrl-0 = <&tri_state_key_default>;
>>>> +		state-top {
>>>> +			label = "Tri-state key top";
>>>
>>> "top/middle" is not too useful. Do we need the label at all? If so,
>>> should it say "loud/vibrations only/mute"?
>>
>> "mute", "vibrate" and "ring" sound good to me.
>>
>
> OnePlus uses the silent/vibrate/ring, iPhone the silent/ring names.
> Maybe silent/vibrate/ring are more familiar.
>
> Adding labels can document these modes here.
> Should we also document these in input-event-codes.h?

Maybe it would be best to define macros for these rather than leave them
as magic numbers
> #define ABS_SND_PROFILE		0x22 /* 0 = silent; 1 = vibrate; 2 = ring */

#define ABS_SND_PROFILE_SILENT	0
#define ABS_SND_PROFILE_VIBRATE	1
#define ABS_SND_PROFILE_RING	2

>
>
> Thanks,
> Gergo
>
>> Although it would be nice if users can easily map the physical key
>> position to the action when viewing the input device or remapping the
>> key in userspace.
>>
>> Do you have any ideas or recommendations on how to do this?
>>>
>>> BR,
>>> 								Pavel
>>
>> --
>> Kind Regards,
>> Caleb
>>
>

--
Kind Regards,
Caleb
diff mbox series

Patch

diff --git a/arch/arm64/boot/dts/qcom/sdm845-oneplus-common.dtsi b/arch/arm64/boot/dts/qcom/sdm845-oneplus-common.dtsi
index 64638ea94db7..e45d4fdead82 100644
--- a/arch/arm64/boot/dts/qcom/sdm845-oneplus-common.dtsi
+++ b/arch/arm64/boot/dts/qcom/sdm845-oneplus-common.dtsi
@@ -52,6 +52,43 @@  key-vol-up {
 		};
 	};
 
+	tri-state-key {
+		compatible = "gpio-keys";
+		label = "Tri-state key";
+		pinctrl-names = "default";
+		pinctrl-0 = <&tri_state_key_default>;
+		state-top {
+			label = "Tri-state key top";
+			linux,input-type = <EV_ABS>;
+			linux,code = <ABS_SND_PROFILE>;
+			linux,input-value = <0>;
+			gpios = <&tlmm 126 GPIO_ACTIVE_LOW>;
+			debounce-interval = <50>;
+			linux,can-disable;
+		};
+
+		state-middle {
+			label = "Tri-state key middle";
+			linux,input-type = <EV_ABS>;
+			linux,code = <ABS_SND_PROFILE>;
+			linux,input-value = <1>;
+			gpios = <&tlmm 52 GPIO_ACTIVE_LOW>;
+			debounce-interval = <50>;
+			linux,can-disable;
+
+		};
+
+		state-bottom {
+			label = "Tri-state key bottom";
+			linux,input-type = <EV_ABS>;
+			linux,code = <ABS_SND_PROFILE>;
+			linux,input-value = <2>;
+			gpios = <&tlmm 24 GPIO_ACTIVE_LOW>;
+			debounce-interval = <50>;
+			linux,can-disable;
+		};
+	};
+
 	reserved-memory {
 		/*
 		 * The rmtfs_mem needs to be guarded due to "XPU limitations"
@@ -754,7 +791,7 @@  &tlmm {
 	gpio-reserved-ranges = <0 4>, <81 4>;
 
 	tri_state_key_default: tri-state-key-default-state {
-		pins = "gpio40", "gpio42", "gpio26";
+		pins = "gpio126", "gpio52", "gpio24";
 		function = "gpio";
 		drive-strength = <2>;
 		bias-disable;