[PATCH] arm64: dts: x1e80100-crd: describe HID supplies

[PATCH] arm64: dts: x1e80100-crd: describe HID supplies

[Johan Hovold]

Add the missing HID supplies to avoid relying on other consumers to keep
them on.

This also avoids the following warnings on boot:

	i2c_hid_of 0-0010: supply vdd not found, using dummy regulator
	i2c_hid_of 0-0010: supply vddl not found, using dummy regulator
	i2c_hid_of 1-0015: supply vdd not found, using dummy regulator
	i2c_hid_of 1-0015: supply vddl not found, using dummy regulator
	i2c_hid_of 1-003a: supply vdd not found, using dummy regulator
	i2c_hid_of 1-003a: supply vddl not found, using dummy regulator

Note that VREG_MISC_3P3 is also used for things like the fingerprint
reader which are not yet fully described so mark the regulator as always
on for now.

Fixes: d7e03cce0400 ("arm64: dts: qcom: x1e80100-crd: Enable more support")
Cc: Abel Vesa <abel.vesa@linaro.org>
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 arch/arm64/boot/dts/qcom/x1e80100-crd.dts | 34 +++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/x1e80100-crd.dts b/arch/arm64/boot/dts/qcom/x1e80100-crd.dts
index 10b28d870f08..4ab7078f76e0 100644
--- a/arch/arm64/boot/dts/qcom/x1e80100-crd.dts
+++ b/arch/arm64/boot/dts/qcom/x1e80100-crd.dts
@@ -288,6 +288,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 = <&pm8550ve_8_gpios 6 GPIO_ACTIVE_HIGH>;
+		enable-active-high;
+
+		pinctrl-names = "default";
+		pinctrl-0 = <&misc_3p3_reg_en>;
+
+		regulator-boot-on;
+		regulator-always-on;
+	};
+
 	vreg_nvme: regulator-nvme {
 		compatible = "regulator-fixed";
 
@@ -689,6 +706,9 @@ touchpad@15 {
 		hid-descr-addr = <0x1>;
 		interrupts-extended = <&tlmm 3 IRQ_TYPE_LEVEL_LOW>;
 
+		vdd-supply = <&vreg_misc_3p3>;
+		vddl-supply = <&vreg_l12b_1p2>;
+
 		pinctrl-0 = <&tpad_default>;
 		pinctrl-names = "default";
 
@@ -702,6 +722,9 @@ keyboard@3a {
 		hid-descr-addr = <0x1>;
 		interrupts-extended = <&tlmm 67 IRQ_TYPE_LEVEL_LOW>;
 
+		vdd-supply = <&vreg_misc_3p3>;
+		vddl-supply = <&vreg_l12b_1p2>;
+
 		pinctrl-0 = <&kybd_default>;
 		pinctrl-names = "default";
 
@@ -721,6 +744,9 @@ touchscreen@10 {
 		hid-descr-addr = <0x1>;
 		interrupts-extended = <&tlmm 51 IRQ_TYPE_LEVEL_LOW>;
 
+		vdd-supply = <&vreg_misc_3p3>;
+		vddl-supply = <&vreg_l15b_1p8>;
+
 		pinctrl-0 = <&ts0_default>;
 		pinctrl-names = "default";
 	};
@@ -854,6 +880,14 @@ &pcie6a_phy {
 	status = "okay";
 };
 
+&pm8550ve_8_gpios {
+	misc_3p3_reg_en: misc-3p3-reg-en-state {
+		pins = "gpio6";
+		function = "normal";
+		bias-disable;
+	};
+};
+
 &pmc8380_3_gpios {
 	edp_bl_en: edp-bl-en-state {
 		pins = "gpio4";
-- 
2.45.2

Re: [PATCH] arm64: dts: x1e80100-crd: describe HID supplies

[Dmitry Baryshkov]

On Tue, Oct 15, 2024 at 02:24:27PM +0200, Johan Hovold wrote:
> Add the missing HID supplies to avoid relying on other consumers to keep
> them on.
> 
> This also avoids the following warnings on boot:
> 
> 	i2c_hid_of 0-0010: supply vdd not found, using dummy regulator
> 	i2c_hid_of 0-0010: supply vddl not found, using dummy regulator
> 	i2c_hid_of 1-0015: supply vdd not found, using dummy regulator
> 	i2c_hid_of 1-0015: supply vddl not found, using dummy regulator
> 	i2c_hid_of 1-003a: supply vdd not found, using dummy regulator
> 	i2c_hid_of 1-003a: supply vddl not found, using dummy regulator
> 
> Note that VREG_MISC_3P3 is also used for things like the fingerprint
> reader which are not yet fully described so mark the regulator as always
> on for now.
> 
> Fixes: d7e03cce0400 ("arm64: dts: qcom: x1e80100-crd: Enable more support")
> Cc: Abel Vesa <abel.vesa@linaro.org>
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
>  arch/arm64/boot/dts/qcom/x1e80100-crd.dts | 34 +++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
> 

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

-- 
With best wishes
Dmitry

Re: [PATCH] arm64: dts: x1e80100-crd: describe HID supplies

[Abel Vesa]

On 24-10-15 14:24:27, Johan Hovold wrote:
> Add the missing HID supplies to avoid relying on other consumers to keep
> them on.
> 
> This also avoids the following warnings on boot:
> 
> 	i2c_hid_of 0-0010: supply vdd not found, using dummy regulator
> 	i2c_hid_of 0-0010: supply vddl not found, using dummy regulator
> 	i2c_hid_of 1-0015: supply vdd not found, using dummy regulator
> 	i2c_hid_of 1-0015: supply vddl not found, using dummy regulator
> 	i2c_hid_of 1-003a: supply vdd not found, using dummy regulator
> 	i2c_hid_of 1-003a: supply vddl not found, using dummy regulator
> 
> Note that VREG_MISC_3P3 is also used for things like the fingerprint
> reader which are not yet fully described so mark the regulator as always
> on for now.
> 
> Fixes: d7e03cce0400 ("arm64: dts: qcom: x1e80100-crd: Enable more support")
> Cc: Abel Vesa <abel.vesa@linaro.org>
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
>  arch/arm64/boot/dts/qcom/x1e80100-crd.dts | 34 +++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/x1e80100-crd.dts b/arch/arm64/boot/dts/qcom/x1e80100-crd.dts
> index 10b28d870f08..4ab7078f76e0 100644
> --- a/arch/arm64/boot/dts/qcom/x1e80100-crd.dts
> +++ b/arch/arm64/boot/dts/qcom/x1e80100-crd.dts
> @@ -288,6 +288,23 @@ vreg_edp_3p3: regulator-edp-3p3 {
>  		regulator-boot-on;
>  	};
>  

[...]

>  
> +&pm8550ve_8_gpios {
> +	misc_3p3_reg_en: misc-3p3-reg-en-state {
> +		pins = "gpio6";
> +		function = "normal";
> +		bias-disable;

Maybe output-enable and input-disable are needed. Can you please check?

Otherwise, LGTM:

Reviewed-by: Abel Vesa <abel.vesa@linaro.org>

> +	};
> +};
> +
>  &pmc8380_3_gpios {
>  	edp_bl_en: edp-bl-en-state {
>  		pins = "gpio4";
> -- 
> 2.45.2
> 

Re: [PATCH] arm64: dts: x1e80100-crd: describe HID supplies

[Stephan Gerhold]

On Tue, Oct 15, 2024 at 04:41:13PM +0300, Abel Vesa wrote:
> On 24-10-15 14:24:27, Johan Hovold wrote:
> > Add the missing HID supplies to avoid relying on other consumers to keep
> > them on.
> > 
> > This also avoids the following warnings on boot:
> > 
> > 	i2c_hid_of 0-0010: supply vdd not found, using dummy regulator
> > 	i2c_hid_of 0-0010: supply vddl not found, using dummy regulator
> > 	i2c_hid_of 1-0015: supply vdd not found, using dummy regulator
> > 	i2c_hid_of 1-0015: supply vddl not found, using dummy regulator
> > 	i2c_hid_of 1-003a: supply vdd not found, using dummy regulator
> > 	i2c_hid_of 1-003a: supply vddl not found, using dummy regulator
> > 
> > Note that VREG_MISC_3P3 is also used for things like the fingerprint
> > reader which are not yet fully described so mark the regulator as always
> > on for now.
> > 
> > Fixes: d7e03cce0400 ("arm64: dts: qcom: x1e80100-crd: Enable more support")
> > Cc: Abel Vesa <abel.vesa@linaro.org>
> > Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> > ---
> >  arch/arm64/boot/dts/qcom/x1e80100-crd.dts | 34 +++++++++++++++++++++++
> >  1 file changed, 34 insertions(+)
> > 
> > diff --git a/arch/arm64/boot/dts/qcom/x1e80100-crd.dts b/arch/arm64/boot/dts/qcom/x1e80100-crd.dts
> > index 10b28d870f08..4ab7078f76e0 100644
> > --- a/arch/arm64/boot/dts/qcom/x1e80100-crd.dts
> > +++ b/arch/arm64/boot/dts/qcom/x1e80100-crd.dts
> > @@ -288,6 +288,23 @@ vreg_edp_3p3: regulator-edp-3p3 {
> >  		regulator-boot-on;
> >  	};
> >  
> 
> [...]
> 
> >  
> > +&pm8550ve_8_gpios {
> > +	misc_3p3_reg_en: misc-3p3-reg-en-state {
> > +		pins = "gpio6";
> > +		function = "normal";
> > +		bias-disable;
> 
> Maybe output-enable and input-disable are needed. Can you please check?
> 

FWIW, there is a reason behind explicitly describing the intended
direction of the pin for PMIC GPIOs with properties like "output-enable"
or "input-disable": On QC platforms, PMIC GPIOs can be either in "input"
mode, "output" mode, or "input+output" mode. If you don't specify
exactly what you want, then the pinctrl-spmi-gpio driver will only add
to the existing configuration.

For the configuration above this means:

 1. If GPIO6 is disabled or in "output" mode during boot, the resulting
    mode will be "output".

 2. If GPIO6 is in "input" mode during boot, the resulting mode will be
    "input+output".

I don't know if "input+output" mode has any negative impact compared to
pure "output" mode. We usually want to have the pins in a consistent
state though (i.e. independent of the boot up state).

Thanks,
Stephan

Re: [PATCH] arm64: dts: x1e80100-crd: describe HID supplies

[Johan Hovold]

On Wed, Oct 16, 2024 at 06:43:26PM +0200, Stephan Gerhold wrote:
> On Tue, Oct 15, 2024 at 04:41:13PM +0300, Abel Vesa wrote:
> > On 24-10-15 14:24:27, Johan Hovold wrote:
 
> > > +&pm8550ve_8_gpios {
> > > +	misc_3p3_reg_en: misc-3p3-reg-en-state {
> > > +		pins = "gpio6";
> > > +		function = "normal";
> > > +		bias-disable;
> > 
> > Maybe output-enable and input-disable are needed. Can you please check?
> 
> FWIW, there is a reason behind explicitly describing the intended
> direction of the pin for PMIC GPIOs with properties like "output-enable"
> or "input-disable": On QC platforms, PMIC GPIOs can be either in "input"
> mode, "output" mode, or "input+output" mode. If you don't specify
> exactly what you want, then the pinctrl-spmi-gpio driver will only add
> to the existing configuration.
> 
> For the configuration above this means:
> 
>  1. If GPIO6 is disabled or in "output" mode during boot, the resulting
>     mode will be "output".
> 
>  2. If GPIO6 is in "input" mode during boot, the resulting mode will be
>     "input+output".
> 
> I don't know if "input+output" mode has any negative impact compared to
> pure "output" mode. We usually want to have the pins in a consistent
> state though (i.e. independent of the boot up state).

Fair enough. I was worried that configuring the pin as an output without
setting the output value could cause trouble (e.g. always default to
low), but it seems at least the Linux driver handles that.

Johan

Re: [PATCH] arm64: dts: x1e80100-crd: describe HID supplies

[Stephan Gerhold]

On Tue, Oct 15, 2024 at 02:24:27PM +0200, Johan Hovold wrote:
> Add the missing HID supplies to avoid relying on other consumers to keep
> them on.
> 
> This also avoids the following warnings on boot:
> 
> 	i2c_hid_of 0-0010: supply vdd not found, using dummy regulator
> 	i2c_hid_of 0-0010: supply vddl not found, using dummy regulator
> 	i2c_hid_of 1-0015: supply vdd not found, using dummy regulator
> 	i2c_hid_of 1-0015: supply vddl not found, using dummy regulator
> 	i2c_hid_of 1-003a: supply vdd not found, using dummy regulator
> 	i2c_hid_of 1-003a: supply vddl not found, using dummy regulator
> 
> Note that VREG_MISC_3P3 is also used for things like the fingerprint
> reader which are not yet fully described so mark the regulator as always
> on for now.
> 
> Fixes: d7e03cce0400 ("arm64: dts: qcom: x1e80100-crd: Enable more support")
> Cc: Abel Vesa <abel.vesa@linaro.org>
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
>  arch/arm64/boot/dts/qcom/x1e80100-crd.dts | 34 +++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/x1e80100-crd.dts b/arch/arm64/boot/dts/qcom/x1e80100-crd.dts
> index 10b28d870f08..4ab7078f76e0 100644
> --- a/arch/arm64/boot/dts/qcom/x1e80100-crd.dts
> +++ b/arch/arm64/boot/dts/qcom/x1e80100-crd.dts
> @@ -288,6 +288,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 = <&pm8550ve_8_gpios 6 GPIO_ACTIVE_HIGH>;
> +		enable-active-high;
> +
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&misc_3p3_reg_en>;
> +
> +		regulator-boot-on;
> +		regulator-always-on;
> +	};
> +
>  	vreg_nvme: regulator-nvme {
>  		compatible = "regulator-fixed";
>  
> @@ -689,6 +706,9 @@ touchpad@15 {
>  		hid-descr-addr = <0x1>;
>  		interrupts-extended = <&tlmm 3 IRQ_TYPE_LEVEL_LOW>;
>  
> +		vdd-supply = <&vreg_misc_3p3>;
> +		vddl-supply = <&vreg_l12b_1p2>;
> +
>  		pinctrl-0 = <&tpad_default>;
>  		pinctrl-names = "default";
>  
> @@ -702,6 +722,9 @@ keyboard@3a {
>  		hid-descr-addr = <0x1>;
>  		interrupts-extended = <&tlmm 67 IRQ_TYPE_LEVEL_LOW>;
>  
> +		vdd-supply = <&vreg_misc_3p3>;
> +		vddl-supply = <&vreg_l12b_1p2>;
> +
>  		pinctrl-0 = <&kybd_default>;
>  		pinctrl-names = "default";
>  
> @@ -721,6 +744,9 @@ touchscreen@10 {
>  		hid-descr-addr = <0x1>;
>  		interrupts-extended = <&tlmm 51 IRQ_TYPE_LEVEL_LOW>;
>  
> +		vdd-supply = <&vreg_misc_3p3>;
> +		vddl-supply = <&vreg_l15b_1p8>;
> +
>  		pinctrl-0 = <&ts0_default>;
>  		pinctrl-names = "default";
>  	};
> @@ -854,6 +880,14 @@ &pcie6a_phy {
>  	status = "okay";
>  };
>  
> +&pm8550ve_8_gpios {
> +	misc_3p3_reg_en: misc-3p3-reg-en-state {
> +		pins = "gpio6";
> +		function = "normal";
> +		bias-disable;

Can we add a "power-source" here? PMIC GPIOs can be either ~3.7V
(VPH_PWR) or 1.8V, depending on which power-source is selected. Without
that, we rely on the firmware to set the voltage level for the GPIO
during boot.

I'm not sure which one would be suitable here. I guess we can just
replicate what the firmware configures during boot.

Thanks,
Stephan

Re: [PATCH] arm64: dts: x1e80100-crd: describe HID supplies

[Abel Vesa]

On 24-10-15 16:57:18, Stephan Gerhold wrote:
> On Tue, Oct 15, 2024 at 02:24:27PM +0200, Johan Hovold wrote:
> > Add the missing HID supplies to avoid relying on other consumers to keep
> > them on.
> > 
> > This also avoids the following warnings on boot:
> > 
> > 	i2c_hid_of 0-0010: supply vdd not found, using dummy regulator
> > 	i2c_hid_of 0-0010: supply vddl not found, using dummy regulator
> > 	i2c_hid_of 1-0015: supply vdd not found, using dummy regulator
> > 	i2c_hid_of 1-0015: supply vddl not found, using dummy regulator
> > 	i2c_hid_of 1-003a: supply vdd not found, using dummy regulator
> > 	i2c_hid_of 1-003a: supply vddl not found, using dummy regulator
> > 
> > Note that VREG_MISC_3P3 is also used for things like the fingerprint
> > reader which are not yet fully described so mark the regulator as always
> > on for now.
> > 
> > Fixes: d7e03cce0400 ("arm64: dts: qcom: x1e80100-crd: Enable more support")
> > Cc: Abel Vesa <abel.vesa@linaro.org>
> > Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> > ---
> >  arch/arm64/boot/dts/qcom/x1e80100-crd.dts | 34 +++++++++++++++++++++++
> >  1 file changed, 34 insertions(+)
> > 
> > diff --git a/arch/arm64/boot/dts/qcom/x1e80100-crd.dts b/arch/arm64/boot/dts/qcom/x1e80100-crd.dts
> > index 10b28d870f08..4ab7078f76e0 100644
> > --- a/arch/arm64/boot/dts/qcom/x1e80100-crd.dts
> > +++ b/arch/arm64/boot/dts/qcom/x1e80100-crd.dts
> > @@ -288,6 +288,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 = <&pm8550ve_8_gpios 6 GPIO_ACTIVE_HIGH>;
> > +		enable-active-high;
> > +
> > +		pinctrl-names = "default";
> > +		pinctrl-0 = <&misc_3p3_reg_en>;
> > +
> > +		regulator-boot-on;
> > +		regulator-always-on;
> > +	};
> > +
> >  	vreg_nvme: regulator-nvme {
> >  		compatible = "regulator-fixed";
> >  
> > @@ -689,6 +706,9 @@ touchpad@15 {
> >  		hid-descr-addr = <0x1>;
> >  		interrupts-extended = <&tlmm 3 IRQ_TYPE_LEVEL_LOW>;
> >  
> > +		vdd-supply = <&vreg_misc_3p3>;
> > +		vddl-supply = <&vreg_l12b_1p2>;
> > +
> >  		pinctrl-0 = <&tpad_default>;
> >  		pinctrl-names = "default";
> >  
> > @@ -702,6 +722,9 @@ keyboard@3a {
> >  		hid-descr-addr = <0x1>;
> >  		interrupts-extended = <&tlmm 67 IRQ_TYPE_LEVEL_LOW>;
> >  
> > +		vdd-supply = <&vreg_misc_3p3>;
> > +		vddl-supply = <&vreg_l12b_1p2>;
> > +
> >  		pinctrl-0 = <&kybd_default>;
> >  		pinctrl-names = "default";
> >  
> > @@ -721,6 +744,9 @@ touchscreen@10 {
> >  		hid-descr-addr = <0x1>;
> >  		interrupts-extended = <&tlmm 51 IRQ_TYPE_LEVEL_LOW>;
> >  
> > +		vdd-supply = <&vreg_misc_3p3>;
> > +		vddl-supply = <&vreg_l15b_1p8>;
> > +
> >  		pinctrl-0 = <&ts0_default>;
> >  		pinctrl-names = "default";
> >  	};
> > @@ -854,6 +880,14 @@ &pcie6a_phy {
> >  	status = "okay";
> >  };
> >  
> > +&pm8550ve_8_gpios {
> > +	misc_3p3_reg_en: misc-3p3-reg-en-state {
> > +		pins = "gpio6";
> > +		function = "normal";
> > +		bias-disable;
> 
> Can we add a "power-source" here? PMIC GPIOs can be either ~3.7V
> (VPH_PWR) or 1.8V, depending on which power-source is selected. Without
> that, we rely on the firmware to set the voltage level for the GPIO
> during boot.

AFAIU, the power-source here should be 0, which selects VPH_PWR which is
3.3V. In that case I think we can avoid explicitly setting it.

> 
> I'm not sure which one would be suitable here. I guess we can just
> replicate what the firmware configures during boot.
> 
> Thanks,
> Stephan

Re: [PATCH] arm64: dts: x1e80100-crd: describe HID supplies

[Johan Hovold]

On Fri, Oct 18, 2024 at 08:19:35PM +0300, Abel Vesa wrote:
> On 24-10-15 16:57:18, Stephan Gerhold wrote:
> > On Tue, Oct 15, 2024 at 02:24:27PM +0200, Johan Hovold wrote:

> > > +&pm8550ve_8_gpios {
> > > +	misc_3p3_reg_en: misc-3p3-reg-en-state {
> > > +		pins = "gpio6";
> > > +		function = "normal";
> > > +		bias-disable;
> > 
> > Can we add a "power-source" here? PMIC GPIOs can be either ~3.7V
> > (VPH_PWR) or 1.8V, depending on which power-source is selected. Without
> > that, we rely on the firmware to set the voltage level for the GPIO
> > during boot.
> 
> AFAIU, the power-source here should be 0, which selects VPH_PWR which is
> 3.3V. In that case I think we can avoid explicitly setting it.

The firmware uses 1.8 V here in fact, but it seems like 3.3 V would work
as well.

> > I'm not sure which one would be suitable here. I guess we can just
> > replicate what the firmware configures during boot.

Let's start with that at least.

Johan