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 <[email protected]>
---
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/[email protected]/T/#t
.../dts/qcom/msm8916-samsung-serranove.dts | 64 ++++++++++++++++++-
1 file changed, 63 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..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";
--
2.39.2
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,
--
Bjorn Andersson <[email protected]>
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 <[email protected]>
> ---
> 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
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/[email protected]/T/#m1f57a36d534e677f84158e6886c1340e036ab5c6
> [3]
> https://lore.kernel.org/linux-pm/[email protected]/T/#m7672ad05590e4123ba5622bc59a9b4dcc0f70e3a
> [4]
> https://lore.kernel.org/linux-pm/[email protected]/T/#m65db0709f0ad3feac6c289f65be5a351cacd2835
> [5]
> https://lore.kernel.org/linux-pm/[email protected]/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,
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/[email protected]/T/#m1f57a36d534e677f84158e6886c1340e036ab5c6
[3]
https://lore.kernel.org/linux-pm/[email protected]/T/#m7672ad05590e4123ba5622bc59a9b4dcc0f70e3a
[4]
https://lore.kernel.org/linux-pm/[email protected]/T/#m65db0709f0ad3feac6c289f65be5a351cacd2835
[5]
https://lore.kernel.org/linux-pm/[email protected]/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
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/[email protected]/T/#u
Kind regards,
Jakob