| Message ID | 20230619203743.8136-1-jahau@rocketmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [v2] arm64: dts: qcom: msm8916-samsung-serranove: Add RT5033 PMIC with charger | expand |
On Mon, 19 Jun 2023 22:37:43 +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]. > > [...] Applied, thanks! [1/1] arm64: dts: qcom: msm8916-samsung-serranove: Add RT5033 PMIC with charger commit: 404d7f65767dde3a0eb3d6127b458b61ed7d7118 Best regards,
On 19/06/2023 22:37, 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. This appeared in today's next next-20230711 and causes new warnings msm8916-samsung-serranove.dtb: extcon@14: 'connector' does not match any of the regexes: 'pinctrl-[0-9]+' https://krzk.eu/#/builders/90/builds/40/steps/17/logs/stdio The commit mentions rt5033, but that is not the schema being here tested, so clearly this is wrong or bindings were not updated. Please fix (and test your future patches). Best regards, Krzysztof
Hi Krzysztof, Cc: Rob, Chanwoo & MyungJoo On 11.07.23 08:13, Krzysztof Kozlowski wrote: ... > This appeared in today's next next-20230711 and causes new warnings > > msm8916-samsung-serranove.dtb: extcon@14: 'connector' does not match any > of the regexes: 'pinctrl-[0-9]+' > https://krzk.eu/#/builders/90/builds/40/steps/17/logs/stdio > > The commit mentions rt5033, but that is not the schema being here > tested, so clearly this is wrong or bindings were not updated. > > Please fix (and test your future patches). The implementation you see in this patch follows the guidance of yours and Rob’s. I already expressed my discontent about it before. To solve the message, the dt-bindings of extcon device sm5502-muic [1] would need to be changed to allow a "connector" sub-node. That’s not the right approach. I still have the impression that the current implementation is based on misunderstandings. I do think Rob’s comment that excon phandle being deprecated [2] is valid for the USB subsystem. Your suggestion to check "ports graph", "orientation" and "usb-role-switch" applies to USB subsystem as well [3]. Rob took the time to add more explanation [4] but it’s still about handling connectors in the more strict sense, which is circling around UBS subsystem. These discussions led to a strangely mixed-up result. I was pushed to implement the USB subsystem connector approach upon an excton subsystem device. As the standard USB connector approach didn’t fit, we switched to a vendor-specific connector phandle [5]. In fact it’s kind of a workaround for the extcon phandle. The extcon device sm5504 is a real piece of hardware. It’s not handled by USB subsystem but by extcon subsystem. The excton subsystem has a method implemented to get the device by phandle [6]. I therefore propose to use the phandle of the extcon subsystem. I mean extcon subsystem, not USB subsystem. In case you disagree, I kindly ask you to take more time to answer in more detail and especially case-related. And specifically to Krzysztof I ask for more politeness in your way of communicating. I understand you’re answering hundreds of requests a day but the communication we had in the past weeks is really frustrating. [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/extcon/siliconmitus,sm5502-muic.yaml?h=v6.5-rc1 [2] https://lore.kernel.org/linux-pm/cover.1677620677.git.jahau@rocketmail.com/T/#m1f57a36d534e677f84158e6886c1340e036ab5c6 [3] https://lore.kernel.org/linux-pm/cover.1682636929.git.jahau@rocketmail.com/T/#m7672ad05590e4123ba5622bc59a9b4dcc0f70e3a [4] https://lore.kernel.org/linux-pm/cover.1682636929.git.jahau@rocketmail.com/T/#m65db0709f0ad3feac6c289f65be5a351cacd2835 [5] https://lore.kernel.org/linux-pm/20230506155435.3005-1-jahau@rocketmail.com/T/#m2aa652c41bad93d60042d831c6397e7838d3cbfc [6] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/extcon/extcon.c?h=v6.5-rc1#n1417 Kind regards, Jakob
On 12/07/2023 21:50, Jakob Hauser wrote: > Hi Krzysztof, > > Cc: Rob, Chanwoo & MyungJoo > > On 11.07.23 08:13, Krzysztof Kozlowski wrote: > ... >> This appeared in today's next next-20230711 and causes new warnings >> >> msm8916-samsung-serranove.dtb: extcon@14: 'connector' does not match any >> of the regexes: 'pinctrl-[0-9]+' >> https://krzk.eu/#/builders/90/builds/40/steps/17/logs/stdio >> >> The commit mentions rt5033, but that is not the schema being here >> tested, so clearly this is wrong or bindings were not updated. >> >> Please fix (and test your future patches). > > The implementation you see in this patch follows the guidance of yours > and Rob’s. I already expressed my discontent about it before. > > To solve the message, the dt-bindings of extcon device sm5502-muic [1] > would need to be changed to allow a "connector" sub-node. That’s not the > right approach. > > I still have the impression that the current implementation is based on > misunderstandings. I do think Rob’s comment that excon phandle being > deprecated [2] is valid for the USB subsystem. Your suggestion to check > "ports graph", "orientation" and "usb-role-switch" applies to USB > subsystem as well [3]. Rob took the time to add more explanation [4] but > it’s still about handling connectors in the more strict sense, which is > circling around UBS subsystem. > > These discussions led to a strangely mixed-up result. I was pushed to > implement the USB subsystem connector approach upon an excton subsystem > device. As the standard USB connector approach didn’t fit, we switched > to a vendor-specific connector phandle [5]. In fact it’s kind of a > workaround for the extcon phandle. > > The extcon device sm5504 is a real piece of hardware. It’s not handled > by USB subsystem but by extcon subsystem. The excton subsystem has a > method implemented to get the device by phandle [6]. I am not sure if we discuss the same problem. My email was about the DTS and bindings, not whether this works in Linux drivers. From your reply I feel that this patch might actually not work? This would be quite confusing... You added new child node "connector" to the siliconmitus,sm5504-muic, so all I would expect that we miss here only updating that binding. Assuming that your code was working... > > I therefore propose to use the phandle of the extcon subsystem. extcon in the bindings? Then we would be back to square one. > I mean > extcon subsystem, not USB subsystem. In case you disagree, I kindly ask > you to take more time to answer in more detail and especially > case-related. Assuming your patch works, I think above is quite specific answer - new property is missing in sm5504 binding. > And specifically to Krzysztof I ask for more politeness in > your way of communicating. I understand you’re answering hundreds of > requests a day but the communication we had in the past weeks is really > frustrating. Sorry to hear that, please accept my apologies. I went through all my replies to you in past few weeks and could not find any particular impolite behavior from my side. > > [1] > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/extcon/siliconmitus,sm5502-muic.yaml?h=v6.5-rc1 > [2] > https://lore.kernel.org/linux-pm/cover.1677620677.git.jahau@rocketmail.com/T/#m1f57a36d534e677f84158e6886c1340e036ab5c6 > [3] > https://lore.kernel.org/linux-pm/cover.1682636929.git.jahau@rocketmail.com/T/#m7672ad05590e4123ba5622bc59a9b4dcc0f70e3a > [4] > https://lore.kernel.org/linux-pm/cover.1682636929.git.jahau@rocketmail.com/T/#m65db0709f0ad3feac6c289f65be5a351cacd2835 > [5] > https://lore.kernel.org/linux-pm/20230506155435.3005-1-jahau@rocketmail.com/T/#m2aa652c41bad93d60042d831c6397e7838d3cbfc > [6] > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/extcon/extcon.c?h=v6.5-rc1#n1417 Best regards, Krzysztof
Hi Krzysztof, On 12.07.23 22:28, Krzysztof Kozlowski wrote: > On 12/07/2023 21:50, Jakob Hauser wrote: ... >> On 11.07.23 08:13, Krzysztof Kozlowski wrote: >> ... >>> This appeared in today's next next-20230711 and causes new warnings >>> >>> msm8916-samsung-serranove.dtb: extcon@14: 'connector' does not match any >>> of the regexes: 'pinctrl-[0-9]+' >>> https://krzk.eu/#/builders/90/builds/40/steps/17/logs/stdio >>> >>> The commit mentions rt5033, but that is not the schema being here >>> tested, so clearly this is wrong or bindings were not updated. >>> >>> Please fix (and test your future patches). >> >> The implementation you see in this patch follows the guidance of yours >> and Rob’s. I already expressed my discontent about it before. >> >> To solve the message, the dt-bindings of extcon device sm5502-muic [1] >> would need to be changed to allow a "connector" sub-node. That’s not the >> right approach. >> >> I still have the impression that the current implementation is based on >> misunderstandings. I do think Rob’s comment that excon phandle being >> deprecated [2] is valid for the USB subsystem. Your suggestion to check >> "ports graph", "orientation" and "usb-role-switch" applies to USB >> subsystem as well [3]. Rob took the time to add more explanation [4] but >> it’s still about handling connectors in the more strict sense, which is >> circling around UBS subsystem. >> >> These discussions led to a strangely mixed-up result. I was pushed to >> implement the USB subsystem connector approach upon an excton subsystem >> device. As the standard USB connector approach didn’t fit, we switched >> to a vendor-specific connector phandle [5]. In fact it’s kind of a >> workaround for the extcon phandle. >> >> The extcon device sm5504 is a real piece of hardware. It’s not handled >> by USB subsystem but by extcon subsystem. The excton subsystem has a >> method implemented to get the device by phandle [6]. > > I am not sure if we discuss the same problem. My email was about the DTS > and bindings, not whether this works in Linux drivers. From your reply I > feel that this patch might actually not work? This would be quite > confusing... > > You added new child node "connector" to the siliconmitus,sm5504-muic, so > all I would expect that we miss here only updating that binding. > Assuming that your code was working... The patch works. >> I therefore propose to use the phandle of the extcon subsystem. > > extcon in the bindings? Then we would be back to square one. If square one is a reasonable proposal, it should be considered. Discussions can go astray. It's a process. The extcon subsystem offers methods to access an excton device. If there is extcon hardware installed, using one of those methods is a pretty straight-forward and an obvious approach. What speaks against the use of this method? Rob argued that the complexity of connector implementation grew over time and therefore standard connector bindings should be used. I understand this and the example you linked in the previous discussion shows such a complexity. But this is about USB subsystem. USB subsystem is not involved here. Why involving it by force? I sure don't have the full picture. However, so far the whole discussion seems to be based on the confusion of different extcon phandles: "virtual" ones in USB subsystem and "real" ones in extcon subsystem. If that's the case, we've been drifting into the wrong direction all the time. >> I mean >> extcon subsystem, not USB subsystem. In case you disagree, I kindly ask >> you to take more time to answer in more detail and especially >> case-related. > > Assuming your patch works, I think above is quite specific answer - new > property is missing in sm5504 binding. I don't think this is the right way to get rid of the issue. Sure, technically the message disappears. Contentwise, however, we're sneaking the confusion of our discussion into the dt-bindings. Imagine what the description of that "connector" property in siliconmitus,sm5502-muic.yaml would look like: "Standard USB connector node according to usb-connector.yaml for accessing the extcon device via devicetree." A device in the extcon subsystem doesn't need this, the extcon subsystem already provides the method to access the extcon device via devicetree. Well, I guess we would silently skip a description like that by changing "additionalProperties" from false to true. Why do I make up such a big thing if the message could be made disappear that easily (and burning time of yours, sorry)? This mix-up we're implementing here is confusing. It's not helpful for further development and implementation of rt5033 and similar hardware arrangements. The issue that came up within the samsung-serranove dts patch here is a good indication of that. I can prepare a patchset to dissolve this USB/extcon mix-up (basically square one, as you called it). Alternatively, if all my tries to clarify a possible misunderstanding are in vain and no one else intervenes, I guess I have no other option than preparing a patch to change the dt-bindungs of siliconmitus,sm5502-muic.yaml. >> And specifically to Krzysztof I ask for more politeness in >> your way of communicating. I understand you’re answering hundreds of >> requests a day but the communication we had in the past weeks is really >> frustrating. > > Sorry to hear that, please accept my apologies. I went through all my > replies to you in past few weeks and could not find any particular > impolite behavior from my side. I'm not used to the fast-paced interaction on the kernel lists. Maybe I have mistaken some of your comments. In that case sorry for my accusation. ... Kind regards, Jakob
Hi all, To close this thread: On 14.07.23 00:26, Jakob Hauser wrote: ... > I can prepare a patchset to dissolve this USB/extcon mix-up (basically > square one, as you called it). > > Alternatively, if all my tries to clarify a possible misunderstanding > are in vain and no one else intervenes, I guess I have no other option > than preparing a patch to change the dt-bindungs of > siliconmitus,sm5502-muic.yaml. ... Krzysztof added a connector node to the dt-bindungs of siliconmitus,sm5502-muic.yaml. lore link: https://lore.kernel.org/linux-devicetree/20230720080141.56329-2-krzysztof.kozlowski@linaro.org/T/#u Kind regards, Jakob
diff --git a/arch/arm64/boot/dts/qcom/msm8916-samsung-serranove.dts b/arch/arm64/boot/dts/qcom/msm8916-samsung-serranove.dts index 15dc246e84e2..cbda25f2ad19 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,43 @@ 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 { + 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>; + }; + }; + + rt5033_charger: charger { + compatible = "richtek,rt5033-charger"; + monitored-battery = <&battery>; + richtek,usb-connector = <&usb_con>; + }; + }; +}; + &blsp_uart2 { status = "okay"; }; @@ -387,6 +441,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. Changes in v2: - Removed "regulator-name"s and changed regulator phandle labels as suggested by Stephan. v1: https://lore.kernel.org/linux-arm-msm/20230617002934.39408-1-jahau@rocketmail.com/T/#t .../dts/qcom/msm8916-samsung-serranove.dts | 64 ++++++++++++++++++- 1 file changed, 63 insertions(+), 1 deletion(-)