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

[Abel Vesa]

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@xxxxxxxxxxxxxxxx>
> > Reviewed-by: Konrad Dybcio <konrad.dybcio@xxxxxxxxxxxxxxxx>
> > Signed-off-by: Abel Vesa <abel.vesa@xxxxxxxxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxxxxx
> > 
> > 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@xxxxxxxxxxxxxxxx
> > 
> > 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@xxxxxxxxxxxxxxxx
> > 
> > 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@xxxxxxxxxxxxxxxx
> > ---
> >  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!