[PATCH v5] arm64: dts: qcom: glymur-crd: Enable keyboard, trackpad and touchscreen

Abel Vesa posted 1 patch 2 weeks, 3 days ago
There is a newer version of this series
arch/arm64/boot/dts/qcom/glymur-crd.dts | 117 ++++++++++++++++++++++++++++++++
1 file changed, 117 insertions(+)
[PATCH v5] arm64: dts: qcom: glymur-crd: Enable keyboard, trackpad and touchscreen
Posted by Abel Vesa 2 weeks, 3 days ago
On CRD, the keyboard, trackpad and touchscreen are connected over I2C
and all share a 3.3V regulator.

So describe the regulator and each input device along with their
pinctrl states.

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
Signed-off-by: Abel Vesa <abel.vesa@oss.qualcomm.com>
---
Changes in v5:
- Since this depends on Displat DT patchset and since that one
  had to be respun in order to drop the non-merging phy patch
  dependency, this one had to be respun as well so that the dependency
  tree is correct.
- Link to v4: https://patch.msgid.link/20260319-glymur-dts-crd-enable-kbd-tp-ts-v4-1-dfe67a134996@oss.qualcomm.com

Changes in v4:
- Rebased on next-20260318.
- Dropped all dependencies except the USB DT and Display DT patchesets,
  which are needed for this one to apply cleanly.
- Link to v3: https://patch.msgid.link/20260313-glymur-dts-crd-enable-kbd-tp-ts-v3-1-66c5ddfee97d@oss.qualcomm.com

Changes in v3:
- Picked up Dmitry's and Konrad's R-b tags.
- Drop the output-high and add bias-disable to the reset pin of the
  touchscreen default state.
- Link to v2: https://patch.msgid.link/20260312-glymur-dts-crd-enable-kbd-tp-ts-v2-1-2277bee4c564@oss.qualcomm.com

Changes in v2:
- Rebased on next-20260311
- Re-ordered pinctrl properties in vreg_misc_3p3, as Konrad suggested.
- Dropped next level dependency patchset.
- Link to v1: https://patch.msgid.link/20260309-glymur-dts-crd-enable-kbd-tp-ts-v1-1-56e03f769a76@oss.qualcomm.com
---
 arch/arm64/boot/dts/qcom/glymur-crd.dts | 117 ++++++++++++++++++++++++++++++++
 1 file changed, 117 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/glymur-crd.dts b/arch/arm64/boot/dts/qcom/glymur-crd.dts
index 38cdcf662ba7..5089ff7cdca3 100644
--- a/arch/arm64/boot/dts/qcom/glymur-crd.dts
+++ b/arch/arm64/boot/dts/qcom/glymur-crd.dts
@@ -13,6 +13,8 @@
 #include "pmk8850.dtsi"         /* SPMI0: SID-0                  */
 #include "smb2370.dtsi"         /* SPMI2: SID-9/10/11            */
 
+#include <dt-bindings/pinctrl/qcom,pmic-gpio.h>
+
 / {
 	model = "Qualcomm Technologies, Inc. Glymur CRD";
 	compatible = "qcom,glymur-crd", "qcom,glymur";
@@ -139,6 +141,23 @@ vreg_edp_3p3: regulator-edp-3p3 {
 		regulator-boot-on;
 	};
 
+	vreg_misc_3p3: regulator-misc-3p3 {
+		 compatible = "regulator-fixed";
+
+		regulator-name = "VREG_MISC_3P3";
+		regulator-min-microvolt = <3300000>;
+		regulator-max-microvolt = <3300000>;
+
+		gpio = <&pmh0110_f_e0_gpios 6 GPIO_ACTIVE_HIGH>;
+		enable-active-high;
+
+		pinctrl-0 = <&misc_3p3_reg_en>;
+		pinctrl-names = "default";
+
+		regulator-boot-on;
+		regulator-always-on;
+	};
+
 	vreg_nvme: regulator-nvme {
 		compatible = "regulator-fixed";
 
@@ -446,6 +465,64 @@ vreg_l4h_e0_1p2: ldo4 {
 	};
 };
 
+&i2c0 {
+	clock-frequency = <400000>;
+
+	status = "okay";
+
+	touchpad@2c {
+		compatible = "hid-over-i2c";
+		reg = <0x2c>;
+
+		hid-descr-addr = <0x20>;
+		interrupts-extended = <&tlmm 3 IRQ_TYPE_LEVEL_LOW>;
+
+		vdd-supply = <&vreg_misc_3p3>;
+		vddl-supply = <&vreg_l15b_e0_1p8>;
+
+		pinctrl-0 = <&tpad_default>;
+		pinctrl-names = "default";
+
+		wakeup-source;
+	};
+
+	keyboard@3a {
+		compatible = "hid-over-i2c";
+		reg = <0x3a>;
+
+		hid-descr-addr = <0x1>;
+		interrupts-extended = <&tlmm 67 IRQ_TYPE_LEVEL_LOW>;
+
+		vdd-supply = <&vreg_misc_3p3>;
+		vddl-supply = <&vreg_l15b_e0_1p8>;
+
+		pinctrl-0 = <&kybd_default>;
+		pinctrl-names = "default";
+
+		wakeup-source;
+	};
+};
+
+&i2c8 {
+	clock-frequency = <400000>;
+
+	status = "okay";
+
+	touchscreen@38 {
+		compatible = "hid-over-i2c";
+		reg = <0x38>;
+
+		hid-descr-addr = <0x1>;
+		interrupts-extended = <&tlmm 51 IRQ_TYPE_LEVEL_LOW>;
+
+		vdd-supply = <&vreg_misc_3p3>;
+		vddl-supply = <&vreg_l15b_e0_1p8>;
+
+		pinctrl-0 = <&ts0_default>;
+		pinctrl-names = "default";
+	};
+};
+
 &i2c5 {
 	clock-frequency = <400000>;
 
@@ -626,6 +703,19 @@ key_vol_up_default: key-vol-up-default-state {
 	};
 };
 
+&pmh0110_f_e0_gpios {
+	misc_3p3_reg_en: misc-3p3-reg-en-state {
+		pins = "gpio6";
+		function = "normal";
+		bias-disable;
+		input-disable;
+		output-enable;
+		drive-push-pull;
+		power-source = <1>; /* 1.8 V */
+		qcom,drive-strength = <PMIC_GPIO_STRENGTH_LOW>;
+	};
+};
+
 &pmk8850_rtc {
 	qcom,no-alarm;
 };
@@ -664,6 +754,33 @@ edp_reg_en: edp-reg-en-state {
 		bias-disable;
 	};
 
+	kybd_default: kybd-default-state {
+		pins = "gpio67";
+		function = "gpio";
+		bias-disable;
+	};
+
+	tpad_default: tpad-default-state {
+		pins = "gpio3";
+		function = "gpio";
+		bias-disable;
+	};
+
+	ts0_default: ts0-default-state {
+		int-n-pins {
+			pins = "gpio51";
+			function = "gpio";
+			bias-disable;
+		};
+
+		reset-n-pins {
+			pins = "gpio48";
+			function = "gpio";
+			drive-strength = <16>;
+			bias-disable;
+		};
+	};
+
 	pcie4_default: pcie4-default-state {
 		clkreq-n-pins {
 			pins = "gpio147";

---
base-commit: 54526d6c29ce58d5399cd4e2237d631266ebaaef
change-id: 20260309-glymur-dts-crd-enable-kbd-tp-ts-c80c0cb78940
prerequisite-change-id:  20260109-dts-qcom-glymur-add-usb-support-617b6d9d032c:v6
prerequisite-patch-id: 7ec5f802a334d96421d8f95d4d9e9773655cc947
prerequisite-patch-id: 8d9e016b49979fa817cf9eab70b809fdb9d4656f
prerequisite-change-id: 20260109-dts-qcom-glymur-crd-add-edp-03f0adde9750:v6
prerequisite-patch-id: 7ec5f802a334d96421d8f95d4d9e9773655cc947
prerequisite-patch-id: 8d9e016b49979fa817cf9eab70b809fdb9d4656f
prerequisite-patch-id: 346f2db0933c551a039f63b945f989a5c8320657
prerequisite-patch-id: 919020405b70d588fa4356a5cbfb44e67006102e

Best regards,
--  
Abel Vesa <abel.vesa@oss.qualcomm.com>
Re: [PATCH v5] arm64: dts: qcom: glymur-crd: Enable keyboard, trackpad and touchscreen
Posted by Dmitry Baryshkov 2 weeks, 3 days ago
On Thu, Mar 19, 2026 at 05:30:48PM +0200, Abel Vesa wrote:
> On CRD, the keyboard, trackpad and touchscreen are connected over I2C
> and all share a 3.3V regulator.
> 
> So describe the regulator and each input device along with their
> pinctrl states.
> 
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
> Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> Signed-off-by: Abel Vesa <abel.vesa@oss.qualcomm.com>
> ---
> Changes in v5:
> - Since this depends on Displat DT patchset and since that one
>   had to be respun in order to drop the non-merging phy patch
>   dependency, this one had to be respun as well so that the dependency
>   tree is correct.

Where do the dependencies come from? Would it be easier to merge this
one first? Or are there overlapping supplies?

> - Link to v4: https://patch.msgid.link/20260319-glymur-dts-crd-enable-kbd-tp-ts-v4-1-dfe67a134996@oss.qualcomm.com
> 
> Changes in v4:
> - Rebased on next-20260318.
> - Dropped all dependencies except the USB DT and Display DT patchesets,
>   which are needed for this one to apply cleanly.
> - Link to v3: https://patch.msgid.link/20260313-glymur-dts-crd-enable-kbd-tp-ts-v3-1-66c5ddfee97d@oss.qualcomm.com
> 
> Changes in v3:
> - Picked up Dmitry's and Konrad's R-b tags.
> - Drop the output-high and add bias-disable to the reset pin of the
>   touchscreen default state.
> - Link to v2: https://patch.msgid.link/20260312-glymur-dts-crd-enable-kbd-tp-ts-v2-1-2277bee4c564@oss.qualcomm.com
> 
> Changes in v2:
> - Rebased on next-20260311
> - Re-ordered pinctrl properties in vreg_misc_3p3, as Konrad suggested.
> - Dropped next level dependency patchset.
> - Link to v1: https://patch.msgid.link/20260309-glymur-dts-crd-enable-kbd-tp-ts-v1-1-56e03f769a76@oss.qualcomm.com
> ---
>  arch/arm64/boot/dts/qcom/glymur-crd.dts | 117 ++++++++++++++++++++++++++++++++
>  1 file changed, 117 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/glymur-crd.dts b/arch/arm64/boot/dts/qcom/glymur-crd.dts
> index 38cdcf662ba7..5089ff7cdca3 100644
> --- a/arch/arm64/boot/dts/qcom/glymur-crd.dts
> +++ b/arch/arm64/boot/dts/qcom/glymur-crd.dts
> @@ -13,6 +13,8 @@
>  #include "pmk8850.dtsi"         /* SPMI0: SID-0                  */
>  #include "smb2370.dtsi"         /* SPMI2: SID-9/10/11            */
>  
> +#include <dt-bindings/pinctrl/qcom,pmic-gpio.h>
> +
>  / {
>  	model = "Qualcomm Technologies, Inc. Glymur CRD";
>  	compatible = "qcom,glymur-crd", "qcom,glymur";
> @@ -139,6 +141,23 @@ vreg_edp_3p3: regulator-edp-3p3 {
>  		regulator-boot-on;
>  	};
>  
> +	vreg_misc_3p3: regulator-misc-3p3 {
> +		 compatible = "regulator-fixed";

Extra whitespaces before the 'compatible'

> +
> +		regulator-name = "VREG_MISC_3P3";
> +		regulator-min-microvolt = <3300000>;
> +		regulator-max-microvolt = <3300000>;
> +
> +		gpio = <&pmh0110_f_e0_gpios 6 GPIO_ACTIVE_HIGH>;
> +		enable-active-high;
> +
> +		pinctrl-0 = <&misc_3p3_reg_en>;
> +		pinctrl-names = "default";
> +
> +		regulator-boot-on;
> +		regulator-always-on;

Why is it always on? Should it be on only if the HID is used?

> +	};
> +
>  	vreg_nvme: regulator-nvme {
>  		compatible = "regulator-fixed";
>  
> @@ -446,6 +465,64 @@ vreg_l4h_e0_1p2: ldo4 {
>  	};
>  };
>  
> +&i2c0 {
> +	clock-frequency = <400000>;
> +
> +	status = "okay";
> +
> +	touchpad@2c {
> +		compatible = "hid-over-i2c";
> +		reg = <0x2c>;
> +
> +		hid-descr-addr = <0x20>;
> +		interrupts-extended = <&tlmm 3 IRQ_TYPE_LEVEL_LOW>;
> +
> +		vdd-supply = <&vreg_misc_3p3>;
> +		vddl-supply = <&vreg_l15b_e0_1p8>;
> +
> +		pinctrl-0 = <&tpad_default>;
> +		pinctrl-names = "default";
> +
> +		wakeup-source;
> +	};
> +
> +	keyboard@3a {
> +		compatible = "hid-over-i2c";
> +		reg = <0x3a>;
> +
> +		hid-descr-addr = <0x1>;
> +		interrupts-extended = <&tlmm 67 IRQ_TYPE_LEVEL_LOW>;
> +
> +		vdd-supply = <&vreg_misc_3p3>;
> +		vddl-supply = <&vreg_l15b_e0_1p8>;
> +
> +		pinctrl-0 = <&kybd_default>;
> +		pinctrl-names = "default";
> +
> +		wakeup-source;
> +	};
> +};
> +
> +&i2c8 {
> +	clock-frequency = <400000>;
> +
> +	status = "okay";
> +
> +	touchscreen@38 {
> +		compatible = "hid-over-i2c";
> +		reg = <0x38>;
> +
> +		hid-descr-addr = <0x1>;
> +		interrupts-extended = <&tlmm 51 IRQ_TYPE_LEVEL_LOW>;
> +
> +		vdd-supply = <&vreg_misc_3p3>;
> +		vddl-supply = <&vreg_l15b_e0_1p8>;
> +
> +		pinctrl-0 = <&ts0_default>;
> +		pinctrl-names = "default";
> +	};
> +};
> +
>  &i2c5 {
>  	clock-frequency = <400000>;
>  
> @@ -626,6 +703,19 @@ key_vol_up_default: key-vol-up-default-state {
>  	};
>  };
>  
> +&pmh0110_f_e0_gpios {
> +	misc_3p3_reg_en: misc-3p3-reg-en-state {
> +		pins = "gpio6";
> +		function = "normal";
> +		bias-disable;
> +		input-disable;
> +		output-enable;
> +		drive-push-pull;
> +		power-source = <1>; /* 1.8 V */
> +		qcom,drive-strength = <PMIC_GPIO_STRENGTH_LOW>;
> +	};
> +};
> +
>  &pmk8850_rtc {
>  	qcom,no-alarm;
>  };
> @@ -664,6 +754,33 @@ edp_reg_en: edp-reg-en-state {
>  		bias-disable;
>  	};
>  
> +	kybd_default: kybd-default-state {
> +		pins = "gpio67";
> +		function = "gpio";
> +		bias-disable;
> +	};
> +
> +	tpad_default: tpad-default-state {
> +		pins = "gpio3";
> +		function = "gpio";
> +		bias-disable;
> +	};
> +
> +	ts0_default: ts0-default-state {
> +		int-n-pins {
> +			pins = "gpio51";

What was the sorting order here? I assume you had one.

> +			function = "gpio";
> +			bias-disable;
> +		};
> +
> +		reset-n-pins {
> +			pins = "gpio48";
> +			function = "gpio";
> +			drive-strength = <16>;
> +			bias-disable;
> +		};
> +	};
> +
>  	pcie4_default: pcie4-default-state {
>  		clkreq-n-pins {
>  			pins = "gpio147";
> 
> ---
> base-commit: 54526d6c29ce58d5399cd4e2237d631266ebaaef
> change-id: 20260309-glymur-dts-crd-enable-kbd-tp-ts-c80c0cb78940
> prerequisite-change-id:  20260109-dts-qcom-glymur-add-usb-support-617b6d9d032c:v6
> prerequisite-patch-id: 7ec5f802a334d96421d8f95d4d9e9773655cc947
> prerequisite-patch-id: 8d9e016b49979fa817cf9eab70b809fdb9d4656f
> prerequisite-change-id: 20260109-dts-qcom-glymur-crd-add-edp-03f0adde9750:v6
> prerequisite-patch-id: 7ec5f802a334d96421d8f95d4d9e9773655cc947
> prerequisite-patch-id: 8d9e016b49979fa817cf9eab70b809fdb9d4656f
> prerequisite-patch-id: 346f2db0933c551a039f63b945f989a5c8320657
> prerequisite-patch-id: 919020405b70d588fa4356a5cbfb44e67006102e
> 
> Best regards,
> --  
> Abel Vesa <abel.vesa@oss.qualcomm.com>
> 

-- 
With best wishes
Dmitry
Re: [PATCH v5] arm64: dts: qcom: glymur-crd: Enable keyboard, trackpad and touchscreen
Posted by Abel Vesa 2 weeks, 3 days ago
On 26-03-19 21:49:07, Dmitry Baryshkov wrote:
> On Thu, Mar 19, 2026 at 05:30:48PM +0200, Abel Vesa wrote:
> > On CRD, the keyboard, trackpad and touchscreen are connected over I2C
> > and all share a 3.3V regulator.
> > 
> > So describe the regulator and each input device along with their
> > pinctrl states.
> > 
> > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
> > Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> > Signed-off-by: Abel Vesa <abel.vesa@oss.qualcomm.com>
> > ---
> > Changes in v5:
> > - Since this depends on Displat DT patchset and since that one
> >   had to be respun in order to drop the non-merging phy patch
> >   dependency, this one had to be respun as well so that the dependency
> >   tree is correct.
> 
> Where do the dependencies come from? Would it be easier to merge this
> one first? Or are there overlapping supplies?

The USB and DT patchsets were on the list first, so it makes sense to be
merged first. If this one was to be merged first, the other two would
have to be reworked due to conflicts. Also this is the order in which the
support was brought up. Also, keyboard, trackpad and touchscreen don't
really make sense without display.

> 
> > - Link to v4: https://patch.msgid.link/20260319-glymur-dts-crd-enable-kbd-tp-ts-v4-1-dfe67a134996@oss.qualcomm.com
> > 
> > Changes in v4:
> > - Rebased on next-20260318.
> > - Dropped all dependencies except the USB DT and Display DT patchesets,
> >   which are needed for this one to apply cleanly.
> > - Link to v3: https://patch.msgid.link/20260313-glymur-dts-crd-enable-kbd-tp-ts-v3-1-66c5ddfee97d@oss.qualcomm.com
> > 
> > Changes in v3:
> > - Picked up Dmitry's and Konrad's R-b tags.
> > - Drop the output-high and add bias-disable to the reset pin of the
> >   touchscreen default state.
> > - Link to v2: https://patch.msgid.link/20260312-glymur-dts-crd-enable-kbd-tp-ts-v2-1-2277bee4c564@oss.qualcomm.com
> > 
> > Changes in v2:
> > - Rebased on next-20260311
> > - Re-ordered pinctrl properties in vreg_misc_3p3, as Konrad suggested.
> > - Dropped next level dependency patchset.
> > - Link to v1: https://patch.msgid.link/20260309-glymur-dts-crd-enable-kbd-tp-ts-v1-1-56e03f769a76@oss.qualcomm.com
> > ---
> >  arch/arm64/boot/dts/qcom/glymur-crd.dts | 117 ++++++++++++++++++++++++++++++++
> >  1 file changed, 117 insertions(+)
> > 
> > diff --git a/arch/arm64/boot/dts/qcom/glymur-crd.dts b/arch/arm64/boot/dts/qcom/glymur-crd.dts
> > index 38cdcf662ba7..5089ff7cdca3 100644
> > --- a/arch/arm64/boot/dts/qcom/glymur-crd.dts
> > +++ b/arch/arm64/boot/dts/qcom/glymur-crd.dts
> > @@ -13,6 +13,8 @@
> >  #include "pmk8850.dtsi"         /* SPMI0: SID-0                  */
> >  #include "smb2370.dtsi"         /* SPMI2: SID-9/10/11            */
> >  
> > +#include <dt-bindings/pinctrl/qcom,pmic-gpio.h>
> > +
> >  / {
> >  	model = "Qualcomm Technologies, Inc. Glymur CRD";
> >  	compatible = "qcom,glymur-crd", "qcom,glymur";
> > @@ -139,6 +141,23 @@ vreg_edp_3p3: regulator-edp-3p3 {
> >  		regulator-boot-on;
> >  	};
> >  
> > +	vreg_misc_3p3: regulator-misc-3p3 {
> > +		 compatible = "regulator-fixed";
> 
> Extra whitespaces before the 'compatible'

Will fix and respin tomorrow.

> 
> > +
> > +		regulator-name = "VREG_MISC_3P3";
> > +		regulator-min-microvolt = <3300000>;
> > +		regulator-max-microvolt = <3300000>;
> > +
> > +		gpio = <&pmh0110_f_e0_gpios 6 GPIO_ACTIVE_HIGH>;
> > +		enable-active-high;
> > +
> > +		pinctrl-0 = <&misc_3p3_reg_en>;
> > +		pinctrl-names = "default";
> > +
> > +		regulator-boot-on;
> > +		regulator-always-on;
> 
> Why is it always on? Should it be on only if the HID is used?

Yeah, I think this should be dropped.

Double checked the schematics and, unlike Hamoa CRD, this one doesn't
use this regulator for fingerprint.

Will do in next version.

> 
> > +	};
> > +
> >  	vreg_nvme: regulator-nvme {
> >  		compatible = "regulator-fixed";
> >  
> > @@ -446,6 +465,64 @@ vreg_l4h_e0_1p2: ldo4 {
> >  	};
> >  };
> >  
> > +&i2c0 {
> > +	clock-frequency = <400000>;
> > +
> > +	status = "okay";
> > +
> > +	touchpad@2c {
> > +		compatible = "hid-over-i2c";
> > +		reg = <0x2c>;
> > +
> > +		hid-descr-addr = <0x20>;
> > +		interrupts-extended = <&tlmm 3 IRQ_TYPE_LEVEL_LOW>;
> > +
> > +		vdd-supply = <&vreg_misc_3p3>;
> > +		vddl-supply = <&vreg_l15b_e0_1p8>;
> > +
> > +		pinctrl-0 = <&tpad_default>;
> > +		pinctrl-names = "default";
> > +
> > +		wakeup-source;
> > +	};
> > +
> > +	keyboard@3a {
> > +		compatible = "hid-over-i2c";
> > +		reg = <0x3a>;
> > +
> > +		hid-descr-addr = <0x1>;
> > +		interrupts-extended = <&tlmm 67 IRQ_TYPE_LEVEL_LOW>;
> > +
> > +		vdd-supply = <&vreg_misc_3p3>;
> > +		vddl-supply = <&vreg_l15b_e0_1p8>;
> > +
> > +		pinctrl-0 = <&kybd_default>;
> > +		pinctrl-names = "default";
> > +
> > +		wakeup-source;
> > +	};
> > +};
> > +
> > +&i2c8 {
> > +	clock-frequency = <400000>;
> > +
> > +	status = "okay";
> > +
> > +	touchscreen@38 {
> > +		compatible = "hid-over-i2c";
> > +		reg = <0x38>;
> > +
> > +		hid-descr-addr = <0x1>;
> > +		interrupts-extended = <&tlmm 51 IRQ_TYPE_LEVEL_LOW>;
> > +
> > +		vdd-supply = <&vreg_misc_3p3>;
> > +		vddl-supply = <&vreg_l15b_e0_1p8>;
> > +
> > +		pinctrl-0 = <&ts0_default>;
> > +		pinctrl-names = "default";
> > +	};
> > +};
> > +
> >  &i2c5 {
> >  	clock-frequency = <400000>;
> >  
> > @@ -626,6 +703,19 @@ key_vol_up_default: key-vol-up-default-state {
> >  	};
> >  };
> >  
> > +&pmh0110_f_e0_gpios {
> > +	misc_3p3_reg_en: misc-3p3-reg-en-state {
> > +		pins = "gpio6";
> > +		function = "normal";
> > +		bias-disable;
> > +		input-disable;
> > +		output-enable;
> > +		drive-push-pull;
> > +		power-source = <1>; /* 1.8 V */
> > +		qcom,drive-strength = <PMIC_GPIO_STRENGTH_LOW>;
> > +	};
> > +};
> > +
> >  &pmk8850_rtc {
> >  	qcom,no-alarm;
> >  };
> > @@ -664,6 +754,33 @@ edp_reg_en: edp-reg-en-state {
> >  		bias-disable;
> >  	};
> >  
> > +	kybd_default: kybd-default-state {
> > +		pins = "gpio67";
> > +		function = "gpio";
> > +		bias-disable;
> > +	};
> > +
> > +	tpad_default: tpad-default-state {
> > +		pins = "gpio3";
> > +		function = "gpio";
> > +		bias-disable;
> > +	};
> > +
> > +	ts0_default: ts0-default-state {
> > +		int-n-pins {
> > +			pins = "gpio51";
> 
> What was the sorting order here? I assume you had one.

The way I see it, it should be based on state subnode name.
Which currently it is.

Do you suggest some other sorting order though ?

Thanks for reviewing!
Re: [PATCH v5] arm64: dts: qcom: glymur-crd: Enable keyboard, trackpad and touchscreen
Posted by Dmitry Baryshkov 2 weeks, 3 days ago
On Thu, Mar 19, 2026 at 11:11:18PM +0200, Abel Vesa wrote:
> On 26-03-19 21:49:07, Dmitry Baryshkov wrote:
> > On Thu, Mar 19, 2026 at 05:30:48PM +0200, Abel Vesa wrote:
> > > On CRD, the keyboard, trackpad and touchscreen are connected over I2C
> > > and all share a 3.3V regulator.
> > > 
> > > So describe the regulator and each input device along with their
> > > pinctrl states.
> > > 
> > > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
> > > Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> > > Signed-off-by: Abel Vesa <abel.vesa@oss.qualcomm.com>
> > > ---
> > > Changes in v5:
> > > - Since this depends on Displat DT patchset and since that one
> > >   had to be respun in order to drop the non-merging phy patch
> > >   dependency, this one had to be respun as well so that the dependency
> > >   tree is correct.
> > 
> > Where do the dependencies come from? Would it be easier to merge this
> > one first? Or are there overlapping supplies?
> 
> The USB and DT patchsets were on the list first, so it makes sense to be
> merged first. If this one was to be merged first, the other two would
> have to be reworked due to conflicts. Also this is the order in which the
> support was brought up. Also, keyboard, trackpad and touchscreen don't
> really make sense without display.

Well, up to you. Let's hope that there are no additional delays with USB
and display

> 
> > > +
> > > +	ts0_default: ts0-default-state {
> > > +		int-n-pins {
> > > +			pins = "gpio51";
> > 
> > What was the sorting order here? I assume you had one.
> 
> The way I see it, it should be based on state subnode name.
> Which currently it is.
> 
> Do you suggest some other sorting order though ?
> 
> Thanks for reviewing!

Then ts0-default-state > pcie0

I think the recent recommendation was to sort on the pin number, but I
didn't switch myself to it too.

-- 
With best wishes
Dmitry
Re: [PATCH v5] arm64: dts: qcom: glymur-crd: Enable keyboard, trackpad and touchscreen
Posted by Abel Vesa 2 weeks, 3 days ago
On 26-03-20 01:52:06, Dmitry Baryshkov wrote:
> On Thu, Mar 19, 2026 at 11:11:18PM +0200, Abel Vesa wrote:
> > On 26-03-19 21:49:07, Dmitry Baryshkov wrote:
> > > On Thu, Mar 19, 2026 at 05:30:48PM +0200, Abel Vesa wrote:
> > > > On CRD, the keyboard, trackpad and touchscreen are connected over I2C
> > > > and all share a 3.3V regulator.
> > > > 
> > > > So describe the regulator and each input device along with their
> > > > pinctrl states.
> > > > 
> > > > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
> > > > Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> > > > Signed-off-by: Abel Vesa <abel.vesa@oss.qualcomm.com>
> > > > ---
> > > > Changes in v5:
> > > > - Since this depends on Displat DT patchset and since that one
> > > >   had to be respun in order to drop the non-merging phy patch
> > > >   dependency, this one had to be respun as well so that the dependency
> > > >   tree is correct.
> > > 
> > > Where do the dependencies come from? Would it be easier to merge this
> > > one first? Or are there overlapping supplies?
> > 
> > The USB and DT patchsets were on the list first, so it makes sense to be
> > merged first. If this one was to be merged first, the other two would
> > have to be reworked due to conflicts. Also this is the order in which the
> > support was brought up. Also, keyboard, trackpad and touchscreen don't
> > really make sense without display.
> 
> Well, up to you. Let's hope that there are no additional delays with USB
> and display

The latest version of those two patchsets are ready to be merged and
have no other dependency (anymore) than between them, that is Display
depends on USB.

> 
> > 
> > > > +
> > > > +	ts0_default: ts0-default-state {
> > > > +		int-n-pins {
> > > > +			pins = "gpio51";
> > > 
> > > What was the sorting order here? I assume you had one.
> > 
> > The way I see it, it should be based on state subnode name.
> > Which currently it is.
> > 
> > Do you suggest some other sorting order though ?
> > 
> > Thanks for reviewing!
> 
> Then ts0-default-state > pcie0

Oh, right. Will fix that.

> 
> I think the recent recommendation was to sort on the pin number, but I
> didn't switch myself to it too.

Doing that looks odd to me. When you review DT, it seems more logical
that sorting by node name should take precedence over the node properties 
or even worse, their values. Not to mention that that would become just
another weird exception.

But maybe it's just my OCD that is unwilling to accept such an exception...

So unless someone NACKs it, I intend to respin with the sorting done by
the node/subnode name.
Re: [PATCH v5] arm64: dts: qcom: glymur-crd: Enable keyboard, trackpad and touchscreen
Posted by Konrad Dybcio 1 week, 6 days ago
On 3/20/26 9:57 AM, Abel Vesa wrote:
> On 26-03-20 01:52:06, Dmitry Baryshkov wrote:
>> On Thu, Mar 19, 2026 at 11:11:18PM +0200, Abel Vesa wrote:
>>> On 26-03-19 21:49:07, Dmitry Baryshkov wrote:
>>>> On Thu, Mar 19, 2026 at 05:30:48PM +0200, Abel Vesa wrote:
>>>>> On CRD, the keyboard, trackpad and touchscreen are connected over I2C
>>>>> and all share a 3.3V regulator.
>>>>>
>>>>> So describe the regulator and each input device along with their
>>>>> pinctrl states.
>>>>>
>>>>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
>>>>> Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
>>>>> Signed-off-by: Abel Vesa <abel.vesa@oss.qualcomm.com>
>>>>> ---
>>>>> Changes in v5:
>>>>> - Since this depends on Displat DT patchset and since that one
>>>>>   had to be respun in order to drop the non-merging phy patch
>>>>>   dependency, this one had to be respun as well so that the dependency
>>>>>   tree is correct.

[...]

>>>>> +	ts0_default: ts0-default-state {
>>>>> +		int-n-pins {
>>>>> +			pins = "gpio51";
>>>>
>>>> What was the sorting order here? I assume you had one.
>>>
>>> The way I see it, it should be based on state subnode name.
>>> Which currently it is.
>>>
>>> Do you suggest some other sorting order though ?
>>>
>>> Thanks for reviewing!
>>
>> Then ts0-default-state > pcie0
> 
> Oh, right. Will fix that.

+Krzysztof dts-coding-style should clarify this

(pin state subnode sorting, IMO it would make sense to have
both top-level and the child nodes sorted by the pinidx but I don't
really care that much)

Konrad