[PATCH RESEND] arm64: dts: qcom: msm8916-samsung-serranove: Add RT5033 PMIC with charger

[PATCH RESEND] arm64: dts: qcom: msm8916-samsung-serranove: Add RT5033 PMIC with charger

[Jakob Hauser]

For the regulators, apply the same settings as in the downstream
devicetree [1], including the "regulator-always-on" for the SAFE_LDO.
For the voltage of SAFE_LDO, however, there is only one voltage of 4.9 V
available in the mainline driver [2][3].

The values of the battery data evolve from following sources:
- precharge current: 450 mA corresponds to the default value of the chip. It
  doesn't get changed by the downstream Android driver. Therefore let's stick
  to this value.
- constant charge current: The 1000 mA are taken from the downstream devicetree
  of the serranove battery. It's not easy to spot. The value is in the line
  "input_current_limit" [4]. The rows are according to the power supply type,
  the 4th value stands for "main supply" [5]. That's the value used by the
  Android driver when a charging cable is plugged into the device.
- charge termination current: In the downstream devicetree of the battery
  that's the line "full_check_current_1st", which contains the 150 mA [6].
- precharge voltage: This one doesn't get set in the downstream Android driver.
  The chip's default is 2.8 V. That seemed too low to have a notable effect of
  handling the battery gentle. The chosen value of 3.5 V is a bit arbitrary
  and possibly rather high. As the device is already several years old and
  therefore most batteries too, a value on the safe side seems reasonable.
- constant charge voltage: The value of 4.35 V is set in the line
  "chg_float_voltage" of the downstream battery devicetree [7].

The "connector" sub-node in the extcon node, the "battery" node in the
general section and the line "power-supplies" in the fuel-gauge node result
from the way of implementation documented in the dt-bindings of
rt5033-charger [8] and mfd rt5033 [9].

[1] https://github.com/msm8916-mainline/linux-downstream/blob/GT-I9195I/arch/arm/boot/dts/samsung/msm8916/msm8916-sec-serranovelte-eur-r03.dtsi#L135-L181
[2] https://github.com/torvalds/linux/blob/v6.3/include/linux/mfd/rt5033-private.h#L211-L212
[3] https://github.com/torvalds/linux/blob/v6.3/drivers/regulator/rt5033-regulator.c#L83
[4] https://github.com/msm8916-mainline/linux-downstream/blob/GT-I9195I/arch/arm/boot/dts/samsung/msm8916/msm8916-sec-serranovelte-battery-r01.dtsi#L100
[5] https://github.com/msm8916-mainline/linux-downstream/blob/GT-I9195I/include/linux/power_supply.h#L173-L177
[6] https://github.com/msm8916-mainline/linux-downstream/blob/GT-I9195I/arch/arm/boot/dts/samsung/msm8916/msm8916-sec-serranovelte-battery-r01.dtsi#L102
[7] https://github.com/msm8916-mainline/linux-downstream/blob/GT-I9195I/arch/arm/boot/dts/samsung/msm8916/msm8916-sec-serranovelte-battery-r01.dtsi#L95
[8] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/Documentation/devicetree/bindings/power/supply/richtek,rt5033-charger.yaml?h=next-20230616
[9] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/Documentation/devicetree/bindings/mfd/richtek,rt5033.yaml?h=next-20230616

Signed-off-by: Jakob Hauser <jahau@rocketmail.com>
---
The patch is based on linux-next "next-20230616".

The driver rt5033-charger was just recently added to linux-next.

RESEND because I used an outdated e-mail address of Bjorn before.

 .../dts/qcom/msm8916-samsung-serranove.dts    | 67 ++++++++++++++++++-
 1 file changed, 66 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/qcom/msm8916-samsung-serranove.dts b/arch/arm64/boot/dts/qcom/msm8916-samsung-serranove.dts
index 15dc246e84e2..2114d26548db 100644
--- a/arch/arm64/boot/dts/qcom/msm8916-samsung-serranove.dts
+++ b/arch/arm64/boot/dts/qcom/msm8916-samsung-serranove.dts
@@ -142,6 +142,12 @@ muic: extcon@14 {
 
 			pinctrl-names = "default";
 			pinctrl-0 = <&muic_irq_default>;
+
+			usb_con: connector {
+				compatible = "usb-b-connector";
+				label = "micro-USB";
+				type = "micro";
+			};
 		};
 	};
 
@@ -199,6 +205,15 @@ nfc@2b {
 			pinctrl-0 = <&nfc_default>;
 		};
 	};
+
+	battery: battery {
+		compatible = "simple-battery";
+		precharge-current-microamp = <450000>;
+		constant-charge-current-max-microamp = <1000000>;
+		charge-term-current-microamp = <150000>;
+		precharge-upper-limit-microvolt = <3500000>;
+		constant-charge-voltage-max-microvolt = <4350000>;
+	};
 };
 
 &blsp_i2c2 {
@@ -228,7 +243,7 @@ magnetometer@2e {
 &blsp_i2c4 {
 	status = "okay";
 
-	battery@35 {
+	fuel-gauge@35 {
 		compatible = "richtek,rt5033-battery";
 		reg = <0x35>;
 
@@ -237,6 +252,8 @@ battery@35 {
 
 		pinctrl-names = "default";
 		pinctrl-0 = <&fg_alert_default>;
+
+		power-supplies = <&rt5033_charger>;
 	};
 };
 
@@ -261,6 +278,46 @@ touchscreen@20 {
 	};
 };
 
+&blsp_i2c6 {
+	status = "okay";
+
+	pmic@34 {
+		compatible = "richtek,rt5033";
+		reg = <0x34>;
+
+		interrupt-parent = <&tlmm>;
+		interrupts = <62 IRQ_TYPE_EDGE_FALLING>;
+
+		pinctrl-names = "default";
+		pinctrl-0 = <&pmic_int_default>;
+
+		regulators {
+			safe_ldo_reg: SAFE_LDO {
+				regulator-name = "SAFE_LDO";
+				regulator-min-microvolt = <4900000>;
+				regulator-max-microvolt = <4900000>;
+				regulator-always-on;
+			};
+			ldo_reg: LDO {
+				regulator-name = "LDO";
+				regulator-min-microvolt = <2800000>;
+				regulator-max-microvolt = <2800000>;
+			};
+			buck_reg: BUCK {
+				regulator-name = "BUCK";
+				regulator-min-microvolt = <1200000>;
+				regulator-max-microvolt = <1200000>;
+			};
+		};
+
+		rt5033_charger: charger {
+			compatible = "richtek,rt5033-charger";
+			monitored-battery = <&battery>;
+			richtek,usb-connector = <&usb_con>;
+		};
+	};
+};
+
 &blsp_uart2 {
 	status = "okay";
 };
@@ -387,6 +444,14 @@ nfc_i2c_default: nfc-i2c-default-state {
 		bias-disable;
 	};
 
+	pmic_int_default: pmic-int-default-state {
+		pins = "gpio62";
+		function = "gpio";
+
+		drive-strength = <2>;
+		bias-disable;
+	};
+
 	tkey_default: tkey-default-state {
 		pins = "gpio98";
 		function = "gpio";
-- 
2.39.2

Re: [PATCH RESEND] arm64: dts: qcom: msm8916-samsung-serranove: Add RT5033 PMIC with charger

[Stephan Gerhold]

On Sat, Jun 17, 2023 at 02:29:34AM +0200, Jakob Hauser wrote:
> For the regulators, apply the same settings as in the downstream
> devicetree [1], including the "regulator-always-on" for the SAFE_LDO.
> For the voltage of SAFE_LDO, however, there is only one voltage of 4.9 V
> available in the mainline driver [2][3].
> 
> The values of the battery data evolve from following sources:
> - precharge current: 450 mA corresponds to the default value of the chip. It
>   doesn't get changed by the downstream Android driver. Therefore let's stick
>   to this value.
> - constant charge current: The 1000 mA are taken from the downstream devicetree
>   of the serranove battery. It's not easy to spot. The value is in the line
>   "input_current_limit" [4]. The rows are according to the power supply type,
>   the 4th value stands for "main supply" [5]. That's the value used by the
>   Android driver when a charging cable is plugged into the device.
> - charge termination current: In the downstream devicetree of the battery
>   that's the line "full_check_current_1st", which contains the 150 mA [6].
> - precharge voltage: This one doesn't get set in the downstream Android driver.
>   The chip's default is 2.8 V. That seemed too low to have a notable effect of
>   handling the battery gentle. The chosen value of 3.5 V is a bit arbitrary
>   and possibly rather high. As the device is already several years old and
>   therefore most batteries too, a value on the safe side seems reasonable.
> - constant charge voltage: The value of 4.35 V is set in the line
>   "chg_float_voltage" of the downstream battery devicetree [7].
> 
> The "connector" sub-node in the extcon node, the "battery" node in the
> general section and the line "power-supplies" in the fuel-gauge node result
> from the way of implementation documented in the dt-bindings of
> rt5033-charger [8] and mfd rt5033 [9].
> 
> [1] https://github.com/msm8916-mainline/linux-downstream/blob/GT-I9195I/arch/arm/boot/dts/samsung/msm8916/msm8916-sec-serranovelte-eur-r03.dtsi#L135-L181
> [2] https://github.com/torvalds/linux/blob/v6.3/include/linux/mfd/rt5033-private.h#L211-L212
> [3] https://github.com/torvalds/linux/blob/v6.3/drivers/regulator/rt5033-regulator.c#L83
> [4] https://github.com/msm8916-mainline/linux-downstream/blob/GT-I9195I/arch/arm/boot/dts/samsung/msm8916/msm8916-sec-serranovelte-battery-r01.dtsi#L100
> [5] https://github.com/msm8916-mainline/linux-downstream/blob/GT-I9195I/include/linux/power_supply.h#L173-L177
> [6] https://github.com/msm8916-mainline/linux-downstream/blob/GT-I9195I/arch/arm/boot/dts/samsung/msm8916/msm8916-sec-serranovelte-battery-r01.dtsi#L102
> [7] https://github.com/msm8916-mainline/linux-downstream/blob/GT-I9195I/arch/arm/boot/dts/samsung/msm8916/msm8916-sec-serranovelte-battery-r01.dtsi#L95
> [8] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/Documentation/devicetree/bindings/power/supply/richtek,rt5033-charger.yaml?h=next-20230616
> [9] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/Documentation/devicetree/bindings/mfd/richtek,rt5033.yaml?h=next-20230616
> 
> Signed-off-by: Jakob Hauser <jahau@rocketmail.com>
> ---
> The patch is based on linux-next "next-20230616".
> 
> The driver rt5033-charger was just recently added to linux-next.
> 
> RESEND because I used an outdated e-mail address of Bjorn before.
> 
>  .../dts/qcom/msm8916-samsung-serranove.dts    | 67 ++++++++++++++++++-
>  1 file changed, 66 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/msm8916-samsung-serranove.dts b/arch/arm64/boot/dts/qcom/msm8916-samsung-serranove.dts
> index 15dc246e84e2..2114d26548db 100644
> --- a/arch/arm64/boot/dts/qcom/msm8916-samsung-serranove.dts
> +++ b/arch/arm64/boot/dts/qcom/msm8916-samsung-serranove.dts
> [...]
> @@ -261,6 +278,46 @@ touchscreen@20 {
>  	};
>  };
>  
> +&blsp_i2c6 {
> +	status = "okay";
> +
> +	pmic@34 {
> +		compatible = "richtek,rt5033";
> +		reg = <0x34>;
> +
> +		interrupt-parent = <&tlmm>;
> +		interrupts = <62 IRQ_TYPE_EDGE_FALLING>;
> +
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&pmic_int_default>;
> +
> +		regulators {
> +			safe_ldo_reg: SAFE_LDO {
> +				regulator-name = "SAFE_LDO";
> +				regulator-min-microvolt = <4900000>;
> +				regulator-max-microvolt = <4900000>;
> +				regulator-always-on;
> +			};
> +			ldo_reg: LDO {
> +				regulator-name = "LDO";
> +				regulator-min-microvolt = <2800000>;
> +				regulator-max-microvolt = <2800000>;
> +			};
> +			buck_reg: BUCK {
> +				regulator-name = "BUCK";
> +				regulator-min-microvolt = <1200000>;
> +				regulator-max-microvolt = <1200000>;
> +			};

The "regulator-name"s here don't really seem useful, since they're just
the same as the ones already declared in the driver. Can you drop them?
Alternatively you could assign more useful board-specific names, such as
the CAM_SENSOR_A2.8V that was used downstream.

Also, I think it would be slightly clearer to prefix the regulator
labels (safe_ldo_reg, ldo_reg etc) with rt5033_. Perhaps
"rt5033_ldo_reg" or "rt5033_reg_ldo"?

Thanks,
Stephan

Re: [PATCH RESEND] arm64: dts: qcom: msm8916-samsung-serranove: Add RT5033 PMIC with charger

[Jakob Hauser]

Hi Stephan,

On 17.06.23 16:15, Stephan Gerhold wrote:
> On Sat, Jun 17, 2023 at 02:29:34AM +0200, Jakob Hauser wrote:

...

>> +		regulators {
>> +			safe_ldo_reg: SAFE_LDO {
>> +				regulator-name = "SAFE_LDO";
>> +				regulator-min-microvolt = <4900000>;
>> +				regulator-max-microvolt = <4900000>;
>> +				regulator-always-on;
>> +			};
>> +			ldo_reg: LDO {
>> +				regulator-name = "LDO";
>> +				regulator-min-microvolt = <2800000>;
>> +				regulator-max-microvolt = <2800000>;
>> +			};
>> +			buck_reg: BUCK {
>> +				regulator-name = "BUCK";
>> +				regulator-min-microvolt = <1200000>;
>> +				regulator-max-microvolt = <1200000>;
>> +			};
> 
> The "regulator-name"s here don't really seem useful, since they're just
> the same as the ones already declared in the driver. Can you drop them?
> Alternatively you could assign more useful board-specific names, such as
> the CAM_SENSOR_A2.8V that was used downstream.
> 
> Also, I think it would be slightly clearer to prefix the regulator
> labels (safe_ldo_reg, ldo_reg etc) with rt5033_. Perhaps
> "rt5033_ldo_reg" or "rt5033_reg_ldo"?

...
About the "regulator-name"s I wasn't really aware. I don't have a strong 
opinion on this.

With the downstream names, it would look like this:

regulators {
	rt5033_reg_safe_ldo: SAFE_LDO {
		regulator-name = "RT5033SafeLDO";
		regulator-min-microvolt = <4900000>;
		regulator-max-microvolt = <4900000>;
		regulator-always-on;
	};
	rt5033_reg_ldo: LDO {
		regulator-name = "CAM_SENSOR_A2.8V";
		regulator-min-microvolt = <2800000>;
		regulator-max-microvolt = <2800000>;
	};
	rt5033_reg_buck: BUCK {
		regulator-name = "CAM_SENSOR_CORE_1.25V";
		regulator-min-microvolt = <1200000>;
		regulator-max-microvolt = <1200000>;
	};

Dropping them would look like this:

regulators {
	rt5033_reg_safe_ldo: SAFE_LDO {
		regulator-min-microvolt = <4900000>;
		regulator-max-microvolt = <4900000>;
		regulator-always-on;
	};
	rt5033_reg_ldo: LDO {
		regulator-min-microvolt = <2800000>;
		regulator-max-microvolt = <2800000>;
	};
	rt5033_reg_buck: BUCK {
		regulator-min-microvolt = <1200000>;
		regulator-max-microvolt = <1200000>;
	};

I would rather drop them. The first name "RT5033SafeLDO" doesn't add 
much information. The other two I'm not fully sure if they provide the 
cam sensor only or if there might be other users as well. Also it add an 
additional set of names. When dropping them, the generic names SAFE_LDO, 
LDO and BUCK are taken from the rt5033-regulator driver.

Unfortunately, I added the example in the dt-bindings with the generic 
names. So this question might come up again when someone else adds 
rt5033-regulators to another device.

For the phandle labels I'd go for rt5033_reg_..., I already changed them 
in the examples above.

Kind regards,
Jakob

Re: [PATCH RESEND] arm64: dts: qcom: msm8916-samsung-serranove: Add RT5033 PMIC with charger

[Stephan Gerhold]

On Sun, Jun 18, 2023 at 06:49:16PM +0200, Jakob Hauser wrote:
> On 17.06.23 16:15, Stephan Gerhold wrote:
> > On Sat, Jun 17, 2023 at 02:29:34AM +0200, Jakob Hauser wrote:
> 
> ...
> 
> > > +		regulators {
> > > +			safe_ldo_reg: SAFE_LDO {
> > > +				regulator-name = "SAFE_LDO";
> > > +				regulator-min-microvolt = <4900000>;
> > > +				regulator-max-microvolt = <4900000>;
> > > +				regulator-always-on;
> > > +			};
> > > +			ldo_reg: LDO {
> > > +				regulator-name = "LDO";
> > > +				regulator-min-microvolt = <2800000>;
> > > +				regulator-max-microvolt = <2800000>;
> > > +			};
> > > +			buck_reg: BUCK {
> > > +				regulator-name = "BUCK";
> > > +				regulator-min-microvolt = <1200000>;
> > > +				regulator-max-microvolt = <1200000>;
> > > +			};
> > 
> > The "regulator-name"s here don't really seem useful, since they're just
> > the same as the ones already declared in the driver. Can you drop them?
> > Alternatively you could assign more useful board-specific names, such as
> > the CAM_SENSOR_A2.8V that was used downstream.
> > 
> > Also, I think it would be slightly clearer to prefix the regulator
> > labels (safe_ldo_reg, ldo_reg etc) with rt5033_. Perhaps
> > "rt5033_ldo_reg" or "rt5033_reg_ldo"?
> 
> ...
> About the "regulator-name"s I wasn't really aware. I don't have a strong
> opinion on this.
> 
> With the downstream names, it would look like this:
> 
> regulators {
> 	rt5033_reg_safe_ldo: SAFE_LDO {
> 		regulator-name = "RT5033SafeLDO";
> 		regulator-min-microvolt = <4900000>;
> 		regulator-max-microvolt = <4900000>;
> 		regulator-always-on;
> 	};
> 	rt5033_reg_ldo: LDO {
> 		regulator-name = "CAM_SENSOR_A2.8V";
> 		regulator-min-microvolt = <2800000>;
> 		regulator-max-microvolt = <2800000>;
> 	};
> 	rt5033_reg_buck: BUCK {
> 		regulator-name = "CAM_SENSOR_CORE_1.25V";
> 		regulator-min-microvolt = <1200000>;
> 		regulator-max-microvolt = <1200000>;
> 	};
> 
> Dropping them would look like this:
> 
> regulators {
> 	rt5033_reg_safe_ldo: SAFE_LDO {
> 		regulator-min-microvolt = <4900000>;
> 		regulator-max-microvolt = <4900000>;
> 		regulator-always-on;
> 	};
> 	rt5033_reg_ldo: LDO {
> 		regulator-min-microvolt = <2800000>;
> 		regulator-max-microvolt = <2800000>;
> 	};
> 	rt5033_reg_buck: BUCK {
> 		regulator-min-microvolt = <1200000>;
> 		regulator-max-microvolt = <1200000>;
> 	};
> 
> I would rather drop them. The first name "RT5033SafeLDO" doesn't add much
> information. The other two I'm not fully sure if they provide the cam sensor
> only or if there might be other users as well. Also it add an additional set
> of names. When dropping them, the generic names SAFE_LDO, LDO and BUCK are
> taken from the rt5033-regulator driver.
> 
> Unfortunately, I added the example in the dt-bindings with the generic
> names. So this question might come up again when someone else adds
> rt5033-regulators to another device.
> 
> For the phandle labels I'd go for rt5033_reg_..., I already changed them in
> the examples above.
> 

Sounds good to me, thanks!

Stephan