| Message ID | 20230617002934.39408-1-jahau@rocketmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [RESEND] arm64: dts: qcom: msm8916-samsung-serranove: Add RT5033 PMIC with charger | expand |
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
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
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
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";
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(-)