[PATCH 0/4] arm64: dts: qcom: improve Lenovo Miix 630 support

[PATCH 0/4] arm64: dts: qcom: improve Lenovo Miix 630 support

[Dmitry Baryshkov]

Enable some low-hanging fruits growing on the Lenovo Miix 630 laptop,
but not being enabled for some reason.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
Dmitry Baryshkov (4):
      arm64: dts: qcom: msm8998-lenovo-miix-630: enable touchscreen
      arm64: dts: qcom: msm8998-lenovo-miix-630: enable aDSP and SLPI
      arm64: dts: qcom: msm8998-lenovo-miix-630: enable VolumeUp button
      arm64: dts: qcom: msm8998-clamshell: enable resin/VolDown

 arch/arm64/boot/dts/qcom/msm8998-clamshell.dtsi    |  5 ++
 .../boot/dts/qcom/msm8998-lenovo-miix-630.dts      | 63 ++++++++++++++++++++++
 2 files changed, 68 insertions(+)
---
base-commit: 797012914d2d031430268fe512af0ccd7d8e46ef
change-id: 20240722-miix630-support-33b3acc3a396

Best regards,
-- 
Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

[PATCH 1/4] arm64: dts: qcom: msm8998-lenovo-miix-630: enable touchscreen

[Dmitry Baryshkov]

There is no point in keeping touchscreen disabled, enable corresponding
i2c-hid device.

04F3:2608 Touchscreen as /devices/platform/soc@0/c179000.i2c/i2c-0/0-0010/0018:04F3:2608.0001/input/input1
04F3:2608 as /devices/platform/soc@0/c179000.i2c/i2c-0/0-0010/0018:04F3:2608.0001/input/input2
04F3:2608 as /devices/platform/soc@0/c179000.i2c/i2c-0/0-0010/0018:04F3:2608.0001/input/input3
04F3:2608 Stylus as /devices/platform/soc@0/c179000.i2c/i2c-0/0-0010/0018:04F3:2608.0001/input/input4

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 .../boot/dts/qcom/msm8998-lenovo-miix-630.dts      | 28 ++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/msm8998-lenovo-miix-630.dts b/arch/arm64/boot/dts/qcom/msm8998-lenovo-miix-630.dts
index a105143bee4a..118c55f5bcfd 100644
--- a/arch/arm64/boot/dts/qcom/msm8998-lenovo-miix-630.dts
+++ b/arch/arm64/boot/dts/qcom/msm8998-lenovo-miix-630.dts
@@ -11,6 +11,24 @@ / {
 	chassis-type = "convertible";
 };
 
+&blsp1_i2c5 {
+	clock-frequency = <400000>;
+	status = "okay";
+
+	tsc1: hid@10 {
+		compatible = "hid-over-i2c";
+		reg = <0x10>;
+		hid-descr-addr = <0x1>;
+
+		interrupts-extended = <&tlmm 125 IRQ_TYPE_LEVEL_LOW>;
+
+		pinctrl-0 = <&i2c5_hid_active>;
+		pinctrl-names = "default";
+
+		wakeup-source;
+	};
+};
+
 &blsp1_i2c6 {
 	status = "okay";
 
@@ -35,3 +53,13 @@ &remoteproc_mss {
 &sdhc2 {
 	cd-gpios = <&tlmm 95 GPIO_ACTIVE_HIGH>;
 };
+
+&tlmm {
+	i2c5_hid_active: i2c5-hid-active-state {
+		pins = "gpio125";
+		function = "gpio";
+
+		bias-pull-up;
+		drive-strength = <2>;
+	};
+};

-- 
2.39.2

[PATCH 2/4] arm64: dts: qcom: msm8998-lenovo-miix-630: enable aDSP and SLPI

[Dmitry Baryshkov]

Enable two other DSP instances on this platofm, aDSP and SLPI.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 arch/arm64/boot/dts/qcom/msm8998-lenovo-miix-630.dts | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/msm8998-lenovo-miix-630.dts b/arch/arm64/boot/dts/qcom/msm8998-lenovo-miix-630.dts
index 118c55f5bcfd..f585bc8ba2ce 100644
--- a/arch/arm64/boot/dts/qcom/msm8998-lenovo-miix-630.dts
+++ b/arch/arm64/boot/dts/qcom/msm8998-lenovo-miix-630.dts
@@ -45,11 +45,21 @@ keyboard@3a {
 	};
 };
 
+&remoteproc_adsp {
+	firmware-name = "qcom/msm8998/LENOVO/81F1/qcadsp8998.mbn";
+	status = "okay";
+};
+
 &remoteproc_mss {
 	firmware-name = "qcom/msm8998/LENOVO/81F1/qcdsp1v28998.mbn",
 			"qcom/msm8998/LENOVO/81F1/qcdsp28998.mbn";
 };
 
+&remoteproc_slpi {
+	firmware-name = "qcom/msm8998/LENOVO/81F1/qcslpi8998.mbn";
+	status = "okay";
+};
+
 &sdhc2 {
 	cd-gpios = <&tlmm 95 GPIO_ACTIVE_HIGH>;
 };

-- 
2.39.2

[PATCH 3/4] arm64: dts: qcom: msm8998-lenovo-miix-630: enable VolumeUp button

[Dmitry Baryshkov]

Add gpio-keys device, responsible for a single button: Volume Up.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 .../boot/dts/qcom/msm8998-lenovo-miix-630.dts      | 25 ++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/msm8998-lenovo-miix-630.dts b/arch/arm64/boot/dts/qcom/msm8998-lenovo-miix-630.dts
index f585bc8ba2ce..bca309ac0cb8 100644
--- a/arch/arm64/boot/dts/qcom/msm8998-lenovo-miix-630.dts
+++ b/arch/arm64/boot/dts/qcom/msm8998-lenovo-miix-630.dts
@@ -3,12 +3,27 @@
 
 /dts-v1/;
 
+#include <dt-bindings/pinctrl/qcom,pmic-gpio.h>
 #include "msm8998-clamshell.dtsi"
 
 / {
 	model = "Lenovo Miix 630";
 	compatible = "lenovo,miix-630", "qcom,msm8998";
 	chassis-type = "convertible";
+
+	gpio-keys {
+		compatible = "gpio-keys";
+		autorepeat;
+
+		pinctrl-names = "default";
+		pinctrl-0 = <&vol_up_pin_a>;
+
+		key-vol-up {
+			label = "Volume Up";
+			linux,code = <KEY_VOLUMEUP>;
+			gpios = <&pm8998_gpios 6 GPIO_ACTIVE_LOW>;
+		};
+	};
 };
 
 &blsp1_i2c5 {
@@ -45,6 +60,16 @@ keyboard@3a {
 	};
 };
 
+&pm8998_gpios {
+	vol_up_pin_a: vol-up-active-state {
+		pins = "gpio6";
+		function = "normal";
+		input-enable;
+		bias-pull-up;
+		qcom,drive-strength = <PMIC_GPIO_STRENGTH_NO>;
+	};
+};
+
 &remoteproc_adsp {
 	firmware-name = "qcom/msm8998/LENOVO/81F1/qcadsp8998.mbn";
 	status = "okay";

-- 
2.39.2

[PATCH 4/4] arm64: dts: qcom: msm8998-clamshell: enable resin/VolDown

[Dmitry Baryshkov]

Let resin device generate the VolumeDown key.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 arch/arm64/boot/dts/qcom/msm8998-clamshell.dtsi | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/msm8998-clamshell.dtsi b/arch/arm64/boot/dts/qcom/msm8998-clamshell.dtsi
index 3b7172aa4037..75958c4013b4 100644
--- a/arch/arm64/boot/dts/qcom/msm8998-clamshell.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8998-clamshell.dtsi
@@ -128,6 +128,11 @@ pm8005_s1: s1 { /* VDD_GFX supply */
 	};
 };
 
+&pm8998_resin {
+	linux,code = <KEY_VOLUMEDOWN>;
+	status = "okay";
+};
+
 &qusb2phy {
 	status = "okay";
 

-- 
2.39.2

Re: [PATCH 1/4] arm64: dts: qcom: msm8998-lenovo-miix-630: enable touchscreen

[Konrad Dybcio]

On 22.07.2024 1:57 PM, Dmitry Baryshkov wrote:
> There is no point in keeping touchscreen disabled, enable corresponding
> i2c-hid device.
> 
> 04F3:2608 Touchscreen as /devices/platform/soc@0/c179000.i2c/i2c-0/0-0010/0018:04F3:2608.0001/input/input1
> 04F3:2608 as /devices/platform/soc@0/c179000.i2c/i2c-0/0-0010/0018:04F3:2608.0001/input/input2
> 04F3:2608 as /devices/platform/soc@0/c179000.i2c/i2c-0/0-0010/0018:04F3:2608.0001/input/input3
> 04F3:2608 Stylus as /devices/platform/soc@0/c179000.i2c/i2c-0/0-0010/0018:04F3:2608.0001/input/input4
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  .../boot/dts/qcom/msm8998-lenovo-miix-630.dts      | 28 ++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/msm8998-lenovo-miix-630.dts b/arch/arm64/boot/dts/qcom/msm8998-lenovo-miix-630.dts
> index a105143bee4a..118c55f5bcfd 100644
> --- a/arch/arm64/boot/dts/qcom/msm8998-lenovo-miix-630.dts
> +++ b/arch/arm64/boot/dts/qcom/msm8998-lenovo-miix-630.dts
> @@ -11,6 +11,24 @@ / {
>  	chassis-type = "convertible";
>  };
>  
> +&blsp1_i2c5 {
> +	clock-frequency = <400000>;
> +	status = "okay";
> +
> +	tsc1: hid@10 {
weird (and unused label)

very non-specific node name too

> +		compatible = "hid-over-i2c";
> +		reg = <0x10>;
> +		hid-descr-addr = <0x1>;
> +
> +		interrupts-extended = <&tlmm 125 IRQ_TYPE_LEVEL_LOW>;
> +
> +		pinctrl-0 = <&i2c5_hid_active>;
> +		pinctrl-names = "default";
> +
> +		wakeup-source;

double tap to wake? tap to wake?

> +	};
> +};
> +
>  &blsp1_i2c6 {
>  	status = "okay";
>  
> @@ -35,3 +53,13 @@ &remoteproc_mss {
>  &sdhc2 {
>  	cd-gpios = <&tlmm 95 GPIO_ACTIVE_HIGH>;
>  };
> +
> +&tlmm {
> +	i2c5_hid_active: i2c5-hid-active-state {
> +		pins = "gpio125";
> +		function = "gpio";
> +
> +		bias-pull-up;
> +		drive-strength = <2>;

Since there are no other pin definitions, you can do better and not
copy the old rotten style ;)

Konrad

Re: [PATCH 2/4] arm64: dts: qcom: msm8998-lenovo-miix-630: enable aDSP and SLPI

[Konrad Dybcio]

On 22.07.2024 1:57 PM, Dmitry Baryshkov wrote:
> Enable two other DSP instances on this platofm, aDSP and SLPI.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---

newline before status please

Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Konrad

Re: [PATCH 3/4] arm64: dts: qcom: msm8998-lenovo-miix-630: enable VolumeUp button

[Konrad Dybcio]

On 22.07.2024 1:57 PM, Dmitry Baryshkov wrote:
> Add gpio-keys device, responsible for a single button: Volume Up.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  .../boot/dts/qcom/msm8998-lenovo-miix-630.dts      | 25 ++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/msm8998-lenovo-miix-630.dts b/arch/arm64/boot/dts/qcom/msm8998-lenovo-miix-630.dts
> index f585bc8ba2ce..bca309ac0cb8 100644
> --- a/arch/arm64/boot/dts/qcom/msm8998-lenovo-miix-630.dts
> +++ b/arch/arm64/boot/dts/qcom/msm8998-lenovo-miix-630.dts
> @@ -3,12 +3,27 @@
>  
>  /dts-v1/;
>  
> +#include <dt-bindings/pinctrl/qcom,pmic-gpio.h>
>  #include "msm8998-clamshell.dtsi"
>  
>  / {
>  	model = "Lenovo Miix 630";
>  	compatible = "lenovo,miix-630", "qcom,msm8998";
>  	chassis-type = "convertible";
> +
> +	gpio-keys {
> +		compatible = "gpio-keys";
> +		autorepeat;
> +
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&vol_up_pin_a>;

property-n
property-names
	
> +
> +		key-vol-up {
> +			label = "Volume Up";
> +			linux,code = <KEY_VOLUMEUP>;
> +			gpios = <&pm8998_gpios 6 GPIO_ACTIVE_LOW>;

No debounce-interval?

Konrad

Re: [PATCH 4/4] arm64: dts: qcom: msm8998-clamshell: enable resin/VolDown

[Konrad Dybcio]

On 22.07.2024 1:57 PM, Dmitry Baryshkov wrote:
> Let resin device generate the VolumeDown key.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  arch/arm64/boot/dts/qcom/msm8998-clamshell.dtsi | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/msm8998-clamshell.dtsi b/arch/arm64/boot/dts/qcom/msm8998-clamshell.dtsi
> index 3b7172aa4037..75958c4013b4 100644
> --- a/arch/arm64/boot/dts/qcom/msm8998-clamshell.dtsi
> +++ b/arch/arm64/boot/dts/qcom/msm8998-clamshell.dtsi
> @@ -128,6 +128,11 @@ pm8005_s1: s1 { /* VDD_GFX supply */
>  	};
>  };
>  
> +&pm8998_resin {
> +	linux,code = <KEY_VOLUMEDOWN>;
> +	status = "okay";

A newline before status, please

Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Konrad

Re: [PATCH 1/4] arm64: dts: qcom: msm8998-lenovo-miix-630: enable touchscreen

[Dmitry Baryshkov]

On Tue, 23 Jul 2024 at 02:22, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
>
> On 22.07.2024 1:57 PM, Dmitry Baryshkov wrote:
> > There is no point in keeping touchscreen disabled, enable corresponding
> > i2c-hid device.
> >
> > 04F3:2608 Touchscreen as /devices/platform/soc@0/c179000.i2c/i2c-0/0-0010/0018:04F3:2608.0001/input/input1
> > 04F3:2608 as /devices/platform/soc@0/c179000.i2c/i2c-0/0-0010/0018:04F3:2608.0001/input/input2
> > 04F3:2608 as /devices/platform/soc@0/c179000.i2c/i2c-0/0-0010/0018:04F3:2608.0001/input/input3
> > 04F3:2608 Stylus as /devices/platform/soc@0/c179000.i2c/i2c-0/0-0010/0018:04F3:2608.0001/input/input4
> >
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > ---
> >  .../boot/dts/qcom/msm8998-lenovo-miix-630.dts      | 28 ++++++++++++++++++++++
> >  1 file changed, 28 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/qcom/msm8998-lenovo-miix-630.dts b/arch/arm64/boot/dts/qcom/msm8998-lenovo-miix-630.dts
> > index a105143bee4a..118c55f5bcfd 100644
> > --- a/arch/arm64/boot/dts/qcom/msm8998-lenovo-miix-630.dts
> > +++ b/arch/arm64/boot/dts/qcom/msm8998-lenovo-miix-630.dts
> > @@ -11,6 +11,24 @@ / {
> >       chassis-type = "convertible";
> >  };
> >
> > +&blsp1_i2c5 {
> > +     clock-frequency = <400000>;
> > +     status = "okay";
> > +
> > +     tsc1: hid@10 {
> weird (and unused label)
>
> very non-specific node name too
>
> > +             compatible = "hid-over-i2c";
> > +             reg = <0x10>;
> > +             hid-descr-addr = <0x1>;
> > +
> > +             interrupts-extended = <&tlmm 125 IRQ_TYPE_LEVEL_LOW>;
> > +
> > +             pinctrl-0 = <&i2c5_hid_active>;
> > +             pinctrl-names = "default";
> > +
> > +             wakeup-source;
>
> double tap to wake? tap to wake?
>
> > +     };
> > +};
> > +
> >  &blsp1_i2c6 {
> >       status = "okay";
> >
> > @@ -35,3 +53,13 @@ &remoteproc_mss {
> >  &sdhc2 {
> >       cd-gpios = <&tlmm 95 GPIO_ACTIVE_HIGH>;
> >  };
> > +
> > +&tlmm {
> > +     i2c5_hid_active: i2c5-hid-active-state {
> > +             pins = "gpio125";
> > +             function = "gpio";
> > +
> > +             bias-pull-up;
> > +             drive-strength = <2>;
>
> Since there are no other pin definitions, you can do better and not
> copy the old rotten style ;)

Both node and pinctrl were c&p from c630. But was is rotten here?


-- 
With best wishes
Dmitry

Re: [PATCH 1/4] arm64: dts: qcom: msm8998-lenovo-miix-630: enable touchscreen

[Konrad Dybcio]

On 23.07.2024 3:09 AM, Dmitry Baryshkov wrote:
> On Tue, 23 Jul 2024 at 02:22, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
>>
>> On 22.07.2024 1:57 PM, Dmitry Baryshkov wrote:
>>> There is no point in keeping touchscreen disabled, enable corresponding
>>> i2c-hid device.
>>>
>>> 04F3:2608 Touchscreen as /devices/platform/soc@0/c179000.i2c/i2c-0/0-0010/0018:04F3:2608.0001/input/input1
>>> 04F3:2608 as /devices/platform/soc@0/c179000.i2c/i2c-0/0-0010/0018:04F3:2608.0001/input/input2
>>> 04F3:2608 as /devices/platform/soc@0/c179000.i2c/i2c-0/0-0010/0018:04F3:2608.0001/input/input3
>>> 04F3:2608 Stylus as /devices/platform/soc@0/c179000.i2c/i2c-0/0-0010/0018:04F3:2608.0001/input/input4
>>>
>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>> ---
>>>  .../boot/dts/qcom/msm8998-lenovo-miix-630.dts      | 28 ++++++++++++++++++++++
>>>  1 file changed, 28 insertions(+)
>>>
>>> diff --git a/arch/arm64/boot/dts/qcom/msm8998-lenovo-miix-630.dts b/arch/arm64/boot/dts/qcom/msm8998-lenovo-miix-630.dts
>>> index a105143bee4a..118c55f5bcfd 100644
>>> --- a/arch/arm64/boot/dts/qcom/msm8998-lenovo-miix-630.dts
>>> +++ b/arch/arm64/boot/dts/qcom/msm8998-lenovo-miix-630.dts
>>> @@ -11,6 +11,24 @@ / {
>>>       chassis-type = "convertible";
>>>  };
>>>
>>> +&blsp1_i2c5 {
>>> +     clock-frequency = <400000>;
>>> +     status = "okay";
>>> +
>>> +     tsc1: hid@10 {
>> weird (and unused label)
>>
>> very non-specific node name too
>>
>>> +             compatible = "hid-over-i2c";
>>> +             reg = <0x10>;
>>> +             hid-descr-addr = <0x1>;
>>> +
>>> +             interrupts-extended = <&tlmm 125 IRQ_TYPE_LEVEL_LOW>;
>>> +
>>> +             pinctrl-0 = <&i2c5_hid_active>;
>>> +             pinctrl-names = "default";
>>> +
>>> +             wakeup-source;
>>
>> double tap to wake? tap to wake?
>>
>>> +     };
>>> +};
>>> +
>>>  &blsp1_i2c6 {
>>>       status = "okay";
>>>
>>> @@ -35,3 +53,13 @@ &remoteproc_mss {
>>>  &sdhc2 {
>>>       cd-gpios = <&tlmm 95 GPIO_ACTIVE_HIGH>;
>>>  };
>>> +
>>> +&tlmm {
>>> +     i2c5_hid_active: i2c5-hid-active-state {
>>> +             pins = "gpio125";
>>> +             function = "gpio";
>>> +
>>> +             bias-pull-up;
>>> +             drive-strength = <2>;
>>
>> Since there are no other pin definitions, you can do better and not
>> copy the old rotten style ;)
> 
> Both node and pinctrl were c&p from c630. But was is rotten here?

yes, we stopped putting a random newline in there quite some years ago

Konrad

Re: [PATCH 1/4] arm64: dts: qcom: msm8998-lenovo-miix-630: enable touchscreen

[Dmitry Baryshkov]

On Tue, 23 Jul 2024 at 13:01, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
>
> On 23.07.2024 3:09 AM, Dmitry Baryshkov wrote:
> > On Tue, 23 Jul 2024 at 02:22, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
> >>
> >> On 22.07.2024 1:57 PM, Dmitry Baryshkov wrote:
> >>> There is no point in keeping touchscreen disabled, enable corresponding
> >>> i2c-hid device.
> >>>
> >>> 04F3:2608 Touchscreen as /devices/platform/soc@0/c179000.i2c/i2c-0/0-0010/0018:04F3:2608.0001/input/input1
> >>> 04F3:2608 as /devices/platform/soc@0/c179000.i2c/i2c-0/0-0010/0018:04F3:2608.0001/input/input2
> >>> 04F3:2608 as /devices/platform/soc@0/c179000.i2c/i2c-0/0-0010/0018:04F3:2608.0001/input/input3
> >>> 04F3:2608 Stylus as /devices/platform/soc@0/c179000.i2c/i2c-0/0-0010/0018:04F3:2608.0001/input/input4
> >>>
> >>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> >>> ---
> >>>  .../boot/dts/qcom/msm8998-lenovo-miix-630.dts      | 28 ++++++++++++++++++++++
> >>>  1 file changed, 28 insertions(+)
> >>>
> >>> diff --git a/arch/arm64/boot/dts/qcom/msm8998-lenovo-miix-630.dts b/arch/arm64/boot/dts/qcom/msm8998-lenovo-miix-630.dts
> >>> index a105143bee4a..118c55f5bcfd 100644
> >>> --- a/arch/arm64/boot/dts/qcom/msm8998-lenovo-miix-630.dts
> >>> +++ b/arch/arm64/boot/dts/qcom/msm8998-lenovo-miix-630.dts
> >>> @@ -11,6 +11,24 @@ / {
> >>>       chassis-type = "convertible";
> >>>  };
> >>>
> >>> +&blsp1_i2c5 {
> >>> +     clock-frequency = <400000>;
> >>> +     status = "okay";
> >>> +
> >>> +     tsc1: hid@10 {
> >> weird (and unused label)
> >>
> >> very non-specific node name too
> >>
> >>> +             compatible = "hid-over-i2c";
> >>> +             reg = <0x10>;
> >>> +             hid-descr-addr = <0x1>;
> >>> +
> >>> +             interrupts-extended = <&tlmm 125 IRQ_TYPE_LEVEL_LOW>;
> >>> +
> >>> +             pinctrl-0 = <&i2c5_hid_active>;
> >>> +             pinctrl-names = "default";
> >>> +
> >>> +             wakeup-source;
> >>
> >> double tap to wake? tap to wake?
> >>
> >>> +     };
> >>> +};
> >>> +
> >>>  &blsp1_i2c6 {
> >>>       status = "okay";
> >>>
> >>> @@ -35,3 +53,13 @@ &remoteproc_mss {
> >>>  &sdhc2 {
> >>>       cd-gpios = <&tlmm 95 GPIO_ACTIVE_HIGH>;
> >>>  };
> >>> +
> >>> +&tlmm {
> >>> +     i2c5_hid_active: i2c5-hid-active-state {
> >>> +             pins = "gpio125";
> >>> +             function = "gpio";
> >>> +
> >>> +             bias-pull-up;
> >>> +             drive-strength = <2>;
> >>
> >> Since there are no other pin definitions, you can do better and not
> >> copy the old rotten style ;)
> >
> > Both node and pinctrl were c&p from c630. But was is rotten here?
>
> yes, we stopped putting a random newline in there quite some years ago

Ack

-- 
With best wishes
Dmitry

Re: [PATCH 3/4] arm64: dts: qcom: msm8998-lenovo-miix-630: enable VolumeUp button

[Dmitry Baryshkov]

On Tue, 23 Jul 2024 at 02:24, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
>
> On 22.07.2024 1:57 PM, Dmitry Baryshkov wrote:
> > Add gpio-keys device, responsible for a single button: Volume Up.
> >
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > ---
> >  .../boot/dts/qcom/msm8998-lenovo-miix-630.dts      | 25 ++++++++++++++++++++++
> >  1 file changed, 25 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/qcom/msm8998-lenovo-miix-630.dts b/arch/arm64/boot/dts/qcom/msm8998-lenovo-miix-630.dts
> > index f585bc8ba2ce..bca309ac0cb8 100644
> > --- a/arch/arm64/boot/dts/qcom/msm8998-lenovo-miix-630.dts
> > +++ b/arch/arm64/boot/dts/qcom/msm8998-lenovo-miix-630.dts
> > @@ -3,12 +3,27 @@
> >
> >  /dts-v1/;
> >
> > +#include <dt-bindings/pinctrl/qcom,pmic-gpio.h>
> >  #include "msm8998-clamshell.dtsi"
> >
> >  / {
> >       model = "Lenovo Miix 630";
> >       compatible = "lenovo,miix-630", "qcom,msm8998";
> >       chassis-type = "convertible";
> > +
> > +     gpio-keys {
> > +             compatible = "gpio-keys";
> > +             autorepeat;
> > +
> > +             pinctrl-names = "default";
> > +             pinctrl-0 = <&vol_up_pin_a>;
>
> property-n
> property-names
>
> > +
> > +             key-vol-up {
> > +                     label = "Volume Up";
> > +                     linux,code = <KEY_VOLUMEUP>;
> > +                     gpios = <&pm8998_gpios 6 GPIO_ACTIVE_LOW>;
>
> No debounce-interval?

Strangely enough, no. See existing defines.



-- 
With best wishes
Dmitry

Re: [PATCH 3/4] arm64: dts: qcom: msm8998-lenovo-miix-630: enable VolumeUp button

[Dmitry Baryshkov]

On Tue, 23 Jul 2024 at 14:11, Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> On Tue, 23 Jul 2024 at 02:24, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
> >
> > On 22.07.2024 1:57 PM, Dmitry Baryshkov wrote:
> > > Add gpio-keys device, responsible for a single button: Volume Up.
> > >
> > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > ---
> > >  .../boot/dts/qcom/msm8998-lenovo-miix-630.dts      | 25 ++++++++++++++++++++++
> > >  1 file changed, 25 insertions(+)
> > >
> > > diff --git a/arch/arm64/boot/dts/qcom/msm8998-lenovo-miix-630.dts b/arch/arm64/boot/dts/qcom/msm8998-lenovo-miix-630.dts
> > > index f585bc8ba2ce..bca309ac0cb8 100644
> > > --- a/arch/arm64/boot/dts/qcom/msm8998-lenovo-miix-630.dts
> > > +++ b/arch/arm64/boot/dts/qcom/msm8998-lenovo-miix-630.dts
> > > +
> > > +             key-vol-up {
> > > +                     label = "Volume Up";
> > > +                     linux,code = <KEY_VOLUMEUP>;
> > > +                     gpios = <&pm8998_gpios 6 GPIO_ACTIVE_LOW>;
> >
> > No debounce-interval?
>
> Strangely enough, no. See existing defines.

After more digging I see it being used. Let's use it too.

-- 
With best wishes
Dmitry