arm64: dts: qcom: msm8916/39-samsung-a2015/fortuna: Add PMIC and charger #323

wonderfulShrineMaidenOfParadise · 2023-08-12T15:12:14Z

The phones listed below have Richteck RT5033 PMIC and charger.
Add them to the device trees.

Samsung Galaxy A3/A5/A7 2015
Samsung Galaxy E5/E7
Samsung Galaxy Grand Max
Samsung Galaxy Ace 4
Samsung Galaxy Core Prime LTE
Samsung Galaxy Grand Prime

Replaces #317

stephan-gh · 2023-08-12T15:19:39Z

@Jakko3 Do you mind taking a look at the rt5033 battery/charging part of these changes? Would appreciate your review :)

Jakko3 · 2023-08-15T01:50:06Z

Currently I'm on vacation, just sporadically online. Apologies to @wonderfulShrineMaidenOfParadise if my reply time is long and progress becomes slow because of this.

Cc: @Grimler91 (tested rt5033-charger on samsung-a5 and currently looks into rt5033-leds), @Minha-D (asked about rt5033-charger implementation in dts of samsung-fortuna3g elsewhere)

flash-led-controller

I see the compatible "richtek,rt5033-led" is now implemented in upstream drivers/leds/flash/leds-sgm3140.c [1]. I think this will conflict with the future rt5033-led driver implementation, no?

mfd, charger, battery

At the upstream implementation for samsung-serranove, Stephan suggested to drop the "regulator-name"s and change the label names [2]. The result is what's currently in linux-next [3].
The battery parameters differ between the devices. Therefore I suppose the battery node should not be in the "common" dtsi file but in the device-specific dts files instead. That way the battery parameters can be specified for each device independently. So I would place a complete battery node per device. I'm not yet fully sure if this is the best way of handling - however, I wouldn't recommend to put battery parameters in "common" dtsi files and override them in some of the device-specific dts files.
In gprime-common and gprimeltecan there are leftovers of the preliminary names for the charging parameters like "richtek,eoc-uamp". This needs to be replaced with a battery node that contains the charging parameters.
I have some pain with the nesting of the a2015-common/e2015-common dtsi files and gprime-common/cprime-common dtsi files. In a completely assembled dts files, there will be more than one "usb_con" label. This is used to find the extcon device. I'm not sure it works properly this way for all devices. But possibly I don't have a good understanding of the nesting.
At gprimeltecan, the label "i2c_nfc" looks strange to me. How come the mfd is subordinated there? How does it look in the downstream dts?

vbus-supply

I'm not familiar with this. What's the effect when assigning the SAFE_LDO regulator to vbus-supply?
In upstream, SAFE_LDO regulator currently has only one voltage value of 4.9 V implemented [4]. In downstream, SAFE_LDO regulator can provide four voltages 3.3 V, 4.85 V, 4.9 V, 4.95 V [5]. In case this would be implemented into upstream as well, would it still be appropriate for vbus-supply? (The question is hypothetical, I'm raising it because I'm not sure this association is correct.)

Sorry for the many comments and questions. The integration into all those devices is well appreciated.

Sources

wonderfulShrineMaidenOfParadise · 2023-08-15T07:12:51Z

@Jakko3

flash-led-controller

* I see the compatible "richtek,rt5033-led" is now implemented in upstream drivers/leds/flash/leds-sgm3140.c [1]. I think this will conflict with the future rt5033-led driver implementation, no?

Exactly. I would expect future rt5033-led driver implementation reverts and replaces my earlier patch, 7bd932d

mfd, charger, battery

* At the upstream implementation for samsung-serranove, Stephan suggested to drop the "regulator-name"s and change the label names [2]. The result is what's currently in linux-next [3].

I don't understand this. Do you mean adding a prefix rt5033_ to the labels?

* The battery parameters differ between the devices. Therefore I suppose the battery node should not be in the "common" dtsi file but in the device-specific dts files instead. That way the battery parameters can be specified for each device independently. So I would place a complete battery node per device. I'm not yet fully sure if this is the best way of handling - however, I wouldn't recommend to put battery parameters in "common" dtsi files and override them in some of the device-specific dts files.

You have the point. I would change it in a2015/e2015/grandmax.
However, the battery for all fortuna series are the same one, so the parameters are fine to be shared in fortuna-common dtsi, at this point?

* In gprime-common and gprimeltecan there are leftovers of the preliminary names for the charging parameters like "richtek,eoc-uamp". This needs to be replaced with a battery node that contains the charging parameters.

I missed it. It has been dropped.

* I have some pain with the nesting of the a2015-common/e2015-common dtsi files and gprime-common/cprime-common dtsi files. In a completely assembled dts files, there will be more than one "usb_con" label. This is used to find the extcon device. I'm not sure it works properly this way for all devices. But possibly I don't have a good understanding of the nesting.

After deletion of the muic node, the subnode usb_con: connector no longer exists, including label usb_con, so there is still only one usb_con, even after defining it again.

https://github.com/msm8916-mainline/linux/pull/323/files#diff-a870c982d091ac61eae478bc6c3215140ef62c9422ac9b28d6b15cee294c8b86R18-L19
https://github.com/msm8916-mainline/linux/pull/323/files#diff-3feb05c5a6a405819c07a34a4c213f1830fa70b35e9872eadf68f455e7a2f28fL14-L15

* At gprimeltecan, the label "i2c_nfc" looks strange to me. How come the mfd is subordinated there? How does it look in the downstream dts?

The usage of I2C buses for NFC and MFD are completely swapped on gprimeltecan.
In other words, on gprimeltecan, I2C6 is used for NFC, and I2C GPIO with pin 0, 1 is used for MFD.

https://github.com/msm8916-mainline/linux-downstream/blob/SM-G530W/arch/arm/boot/dts/samsung/msm8916/msm8916-sec-fortuna-tmo-r02.dtsi#L50-L83
https://github.com/msm8916-mainline/linux-downstream/blob/SM-G530W/arch/arm/boot/dts/samsung/msm8916/msm8916-sec-fortuna-tmo-r02.dtsi#L110-L184

vbus-supply

* I'm not familiar with this. What's the effect when assigning the SAFE_LDO regulator to vbus-supply?

It's assigned to enable SAFE_LDO on demand, instead of keeping it always on.

* In upstream, SAFE_LDO regulator currently has only one voltage value of 4.9 V implemented [4]. In downstream, SAFE_LDO regulator can provide four voltages 3.3 V, 4.85 V, 4.9 V, 4.95 V [5]. In case this would be implemented into upstream as well, would it still be appropriate for vbus-supply? (The question is hypothetical, I'm raising it because I'm not sure this association is correct.)

I assume SAFE_LDO is for vbus-supply, because it's required to power up USB OTG devices.
Otherwise we will need a hub with external power port to power up.
However, I have no idea about the voltage, nor what happens if the voltage varies.

Jakko3 · 2023-08-16T01:00:15Z

Thanks for the accurate answers.

Exactly. I would expect future rt5033-led driver implementation reverts and replaces my earlier patch, 7bd932d

Sounds fine to me.

I don't understand this. Do you mean adding a prefix rt5033_ to the labels?

There are two things:

Remove the lines "regulator-name" like regulator-name = "SAFE_LDO"; (3x in each regulator node, one line per regulator). They are not neccessary because the regulators already get named that way by the rt5033-regulator driver if not specified otherwise. The lines "regulator-name" could be used to name them differently – though in our case this becomes too confusing when adding additional names. Removing the lines seems the better choice. I wasn't aware about this when I submitted the dt-bindings example.
For the labels, changing them to rt5033_reg_... is more clear. Like in the above link [3].

You have the point. I would change it in a2015/e2015/grandmax.
However, the battery for all fortuna series are the same one, so the parameters are fine to be shared in fortuna-common dtsi, at this point?

I guess this makes sense. As a "guideline" I would say 1) each battery node should be a complete one with all five values, 2) the battery nodes shouldn't be overriden. If battery nodes can be shared in "common" files without overriding them in device-specific files, it should be fine.

The precharge-current and precharge-upper-limit can always be the same. They are not defined in the downstream kernels. The value of 450 mA for precharge-current corresponds to the default value of the rt5033 chip. The 3.5 V for precharge-upper-limit is a conservative value suggested by me.

The other three values constant-charge-current-max, charge-term-current and constant-charge-voltage-max need to be looked up in the downstream kernels in the corresponding battery dtsi file. constant-charge-current-max is the 4th value in the line "battery,input_current_limit", charge-term-current is the line "battery,full_check_current_1st", constant-charge-voltage-max is the value in "battery,chg_float_voltage".

For A5/A5U constant-charge-current-max seems to be 1500 mA and charge-term-current 200 mA...
... whlie for A3/A3U it seems to be constant-charge-current-max 1000 mA and charge-term-current 150 mA.

I missed it. It has been dropped.

There is another set of five "richtek,..." battery parameters in the charger node of gprimeltecan.

After deletion of the muic node, the subnode usb_con: connector no longer exists, including label usb_con, so there is still only one usb_con, even after defining it again.

Sounds reasonable. If this works, I'm fine with that.

The usage of I2C buses for NFC and MFD are completely swapped on gprimeltecan.
In other words, on gprimeltecan, I2C6 is used for NFC, and I2C GPIO with pin 0, 1 is used for MFD.

I lost the overview ;) I don't have a specific opinion here. If it works, I'm ok with it.

It's assigned to enable SAFE_LDO on demand, instead of keeping it always on.

Sounds like a good thing.

I assume SAFE_LDO is for vbus-supply, because it's required to power up USB OTG devices.
Otherwise we will need a hub with external power port to power up.
However, I have no idea about the voltage, nor what happens if the voltage varies.

Powering the USB OTG devices works also without assining SAFE_LDO to vbus-supply. Therefore I wasn't sure if it's "needed". However, as long as it doesn't hurt, I don't mind adding it.

In case you upstream those patches at some point, could you apply the one for samsung-serranove as well? That way the devices are kept all on the same state, also upstream.

wonderfulShrineMaidenOfParadise · 2023-08-16T05:16:30Z

@Jakko3

There are two things:

* Remove the lines "regulator-name" like `regulator-name = "SAFE_LDO";` (3x in each regulator node, one line per regulator). They are not neccessary because the regulators already get named that way by the rt5033-regulator driver if not specified otherwise. The lines "regulator-name" could be used to name them differently – though in our case this becomes too confusing when adding additional names. Removing the lines seems the better choice. I wasn't aware about this when I submitted the dt-bindings example.

* For the labels, changing them to rt5033_reg_... is more clear. Like in the above link [3].

Applied.

I guess this makes sense. As a "guideline" I would say 1) each battery node should be a complete one with all five values, 2) the battery nodes shouldn't be overriden. If battery nodes can be shared in "common" files without overriding them in device-specific files, it should be fine.

The precharge-current and precharge-upper-limit can always be the same. They are not defined in the downstream kernels. The value of 450 mA for precharge-current corresponds to the default value of the rt5033 chip. The 3.5 V for precharge-upper-limit is a conservative value suggested by me.

The other three values constant-charge-current-max, charge-term-current and constant-charge-voltage-max need to be looked up in the downstream kernels in the corresponding battery dtsi file. constant-charge-current-max is the 4th value in the line "battery,input_current_limit", charge-term-current is the line "battery,full_check_current_1st", constant-charge-voltage-max is the value in "battery,chg_float_voltage".
* For [A5](https://github.com/msm8916-mainline/linux-downstream/blob/SM-A500F/arch/arm/boot/dts/samsung/msm8916/msm8916-sec-a5-eur-battery-r00.dtsi#L100-L105)/[A5U](https://github.com/msm8916-mainline/linux-downstream/blob/SM-A500FU/arch/arm/boot/dts/samsung/msm8916/msm8916-sec-a5u-eur-battery-r00.dtsi#L100-L111) `constant-charge-current-max` seems to be 1500 mA and `charge-term-current` 200 mA...

* ... whlie for [A3](https://github.com/msm8916-mainline/linux-downstream/blob/SM-A300F/arch/arm/boot/dts/samsung/msm8916/msm8916-sec-a3-battery-r00.dtsi#L100-L106)/[A3U](https://github.com/msm8916-mainline/linux-downstream/blob/SM-A300FU/arch/arm/boot/dts/samsung/msm8916/msm8916-sec-a3u-battery-r00.dtsi#L95-L101) it seems to be `constant-charge-current-max` 1000 mA and `charge-term-current` 150 mA.

Applied. But I am not sure about fortuna/rossa parts, since it's really a mess made by Samsung.

There is another set of five "richtek,..." battery parameters in the charger node of gprimeltecan.

Another miss :p
Dropped.

Powering the USB OTG devices works also without assining SAFE_LDO to vbus-supply. Therefore I wasn't sure if it's "needed". However, as long as it doesn't hurt, I don't mind adding it.

I am confused with this as well.
Actually I did a test on fortuna, with SAFE_LDO and vbus-supply disabled/dropped, and OTG still supplies power.
Perhaps it doesn't really have something to do with OTG power supply. Just in case, I will double-check it.
So I would drop vbus-supply at the moment. However, how SAFE_LDO is used is still unclear to me.
In my opinion if it's not worse or better with regulator-always-on, I would omit it for now.

In case you upstream those patches at some point, could you apply the one for samsung-serranove as well? That way the devices are kept all on the same state, also upstream.

There is nothing more to do with MFD on serranove, I guess. Could you test flash LED part?
Do let me know if it works, and if I should add a tag of Tested-by.

stephan-gh · 2023-08-16T18:09:51Z

The vbus-supply in this case would be the rt5033-charger, not the SAFE_LDO. However, the way @Jakko3 implemented it the rt5033-charger does not expose a regulator at the moment. It listens for the extcon events (USB detection) itself and enables the vbus-supply if an OTG adapter is connected. This means it doesn't make sense to specify any vbus-supply at the moment.

As far as I understand, the SAFE_LDO regulator is actually a special kind of regulator which gets enabled whenever the incoming VBUS (i.e. from an USB charging cable) is in valid bounds. It's from a totally different vendor but the datasheet of MAX77829 has some hints at page 37:

The safeout LDO is a linear regulator that provides an output voltage of 3.3V, 4.85V, 4.9V, or 4.95V and can be used to supply low voltage-rated USB systems. The SAFEOUT linear regulator turns on when VCHGIN ≥ 3.2V and SFOUT_EN = logic high (from MUIC), [...]. SAFEOUT is disabled when CHGIN is greater than the overvoltage threshold (5.90V typ).

On some Samsung devices this seems to be used for some special magic that makes the touchscreen aware that a charging cable is inserted (no idea why it needs to know that).

If SAFE_OUT was declared as regulator-always-on downstream please apply the same change on mainline as well.

Jakko3 · 2023-08-17T00:54:01Z

Applied. But I am not sure about fortuna/rossa parts, since it's really a mess made by Samsung.

Looking through the downstream sources, I find the following values. No guarantees this is correct, cross-checking is appreciated.

source link	constant-charge-current-max	charge-term-current	constant-charge-voltage-max	comments
A3	1000 mA	150 mA	4.35 V
A3U	1000 mA	150 mA	4.35 V
A5	1500 mA	200 mA	4.35 V	korean version "msm8916-sec-a5-kor-battery-r00.dtsi" contains same values
A5U	1500 mA	200 mA	4.35 V	canadian bell mobility version "msm8916-sec-a5u-canbmc-battery-r00.dtsi" contains same values
E5	1500 mA	200 mA	4.35 V	couldn't find a branch for E5, so I looked up the source in the branch of E7
E7	1500 mA	200 mA	4.35 V	lebanon version "msm8916-sec-e7-ctc-battery-r00.dtsi" contains same values
Grand Max	1000 mA	150 mA	4.4 V	korean version "msm8916-sec-grandmax-kor-battery-r00.dtsi" has a higher "constant-charge-current-max" ("battery,input_current_limit" 4th value) of 1500 mA
Ace 4	700 mA	150 mA	4.35 V	I couldn't find the proper downstream source, the link is actually to SM-G357FZ, which is a variant of "Samsung Galaxy Ace Style"
Core Prime	700 mA	150 mA	4.4 V	Not fully sure I got the right branch, however the file in SM-G360G branch contains the same values
Grand Prime	1000 mA	200 mA	4.35 V	I'm not sure about the variants of Grand Prime and what source exactly I picked here. Though I checked some other branches and found the same values.
A7	1500 mA	200 mA	4.35 V	chinese version "msm8939-sec-a7-chn-battery-r00.dtsi" contains a "constant-charge-voltage-max" of 4.3 V! lebanon version "msm8939-sec-a7-ctc-battery-r00.dtsi" contains same value like the european version.

A special comment on the "charge-term-current" values: When the downstream rt5033-charger driver passes that point, it charges additional approx. 42 mins beyond that. That way it reaches 100 % capacity for sure. Some more information on that I wrote in the cover sheet of the upstreaming patchset v1. Such fine-tuning tweaks are not implemented in the upstream driver (could be implemented as improvement, though). Therefore the "charge-term-current" is rather a bit too early. 150 mA is the lowest possible value by the rt5033 chip. The 200 mA values could be lowered to 150 mA to reach a higher capacity percentage and more relaxed re-charging cycles (longer not-charging/charging periods in "fully" charged state). For the time being, I would stick to the values provided by the downstream sources. That's easier to decide and possibly easier to argue when upstreaming. However, if someone of you tests those devices and confirms that 150 mA results in a nicer end-of-charge behavior (re-charging cycles should become visible when monitoring the voltage_avg), we can also change all of them to 150 mA.

There is nothing more to do with MFD on serranove, I guess. Could you test flash LED part?
Do let me know if it works, and if I should add a tag of Tested-by.

I haven't tested yet. Currently I'm on the road and can't test. When I'm back home and find some time, I'll test and let you know. However, don't wait for me.

Jakko3 · 2023-08-17T23:44:01Z

Some stuff I spotted in the current set:

e5

battery values charge-term-current-microamp to 200000 and constant-charge-current-max-microamp to 1500000
(alternatively the battery nodes in e5 and e7 could be removed and one for both could be added to e2015-common; however, I would suggest to keep a battery node with the three "variable" values in each single device, even if the values are the same... see also the following comments on the Grand Prime devices)

gprime-common

battery node: remove charge-term-current-microamp, constant-charge-current-max-microamp and onstant-charge-voltage-max-microvolt, because cprime-common goes on top of it and can't (or shouldn't) override these values

fortuna3g

add battery node with the 3 values

fortunaltezt

add battery node with the 3 values

gprimeltecan

add battery node with the 3 values
regulator node SAFE_LDO: add "regulator-always-on"

heatqlte

add battery node with value constant-charge-voltage-max-microvolt of 4350000
(suggestion: I would keep the three "variable" battery values together -> 3 battery values in cprime, 3 battery values in heatqlte, no battery node in cprime-common... but this is just my suggestion, you can also leave it splitted)
(off-topic: concerning the "model" line, SM-G357FZ is "Samsung Galaxy Ace Style LTE" with NFC, whereas "Samsung Galaxy Ace 4 LTE" with NFC is SM-G313F, see https://en.wikipedia.org/wiki/Samsung_Galaxy_Ace_4#Comparison_table)

a7

battery values charge-term-current-microamp to 200000 and constant-charge-current-max-microamp to 1500000

wonderfulShrineMaidenOfParadise · 2023-08-18T00:41:09Z

@Jakko3

Wrong values should be fixed now. Nice catches.

The 3 properties will stick together if they are set in &battery:

charge-term-current-microamp
constant-charge-current-max-microamp
constant-charge-voltage-max-microvolt

I am not sure what's the issue about overrides. My current idea is to share common part/properties for "host" devices (e2015 for example), and then override in "guest" (grandmax for example) devices.

Not sure if /delete-property/ solves the issue. I deleted those properties and set them again in guest devices.

About heatqlte model name, ~~I would send another patchset in upstream to fix it.~~

Samsung Galaxy Ace 4 LTE seems to be correct. References:
https://www.samsung.com/uk/support/model/SM-G357FZAZBTU/
https://www.samsung.com/uk/support/model/SM-G357FZWZBTU/

wonderfulShrineMaidenOfParadise

By the way would you feel like to use the code review feature?

arch/arm64/boot/dts/qcom/msm8916-samsung-gprimeltecan.dts

arch/arm64/boot/dts/qcom/msm8916-samsung-e5.dts

arch/arm64/boot/dts/qcom/msm8939-samsung-a7.dts

Jakko3 · 2023-08-18T10:12:32Z

I am not sure what's the issue about overrides.

With those nested dts(i) files over 3 levels and all kind of device code names, it's easy to loose overview which battery parameter goes into which device. It has a potential of being error-prone – which is a notable safety issue when it comes to Li-Ion battery charging.

Also if someone adds a new device based on one of the common dtsi files (also e.g. locally at home for initial device testing) and doesn't add yet a (partial) battery node to that device dts file: When the current override approach is implemented, that device gets charged right away with the values in the common dtsi file. Assume grandmax would be added newly on top of e2015-common, in the first tries without a battery node yet, that device would get charged with 1500 mA (but the battery can take only 1000 mA). Or assume a new device gets added on top of cprime-common and the person "forgets" or is unsure about the battery node, that battery will get charged to 4.4 V, even if the battery can only take 4.35 V like e.g. Ace 4.

Thinking about it, for safety reasons the "variable" battery values should always be in the file of the specific device dts file. That way it's clear and safe. If the (partial) battey node is missing, the charger driver fails.

Personally I would even go a step further and would place the complete battery node seperately in each specific device dts file (not splitting the battery node at all). Like placing a battery into the device. Maybe I'm asking too much by this.

About heatqlte model name, [...]

Good hint. It's not clear, web-searching for SM-G357FZ results partially in "Samsung Galaxy Ace Style" and partially in "Samsung Galaxy Ace 4". For sure it's part of the Ace 4 series. So there is no need to change it.

By the way would you feel like to use the code review feature?

Yes, right, thanks for the hint ;) I'm not familiar with it and once desperately failed on pmOS GitLab trying to do this.

As I'm currently on the road and short on time, I stick to the text comments for now. I'll try the code review feature at another occasion.

Comments on the current set:

As far as I can see, it technically looks good now. The only thing is the above mentioned safety doubts with using battery parameters in the "common" files. I would say that every "final" device dts file should get it's own battery node. Probably it hurts to add the exact same nodes to fortuna3g, fortunaltezt, gprimeltecan but overall this way it's more clear and safe. Preferably a complete battery node for each device; if it hurts the eyes too much, I'd also be fine with partial battery nodes containing the three "variable" battery parameters for each single device.

wonderfulShrineMaidenOfParadise · 2023-08-19T03:29:09Z

It seems a2015 LED parts have no obvious issue, and I will send it to upstream soon.

Also if someone adds a new device [...]

@Jakko3 I get your point, so it's about safety for future works. I understand your concern.

However, does it stop people from doing dangerous things?
There are 3 assumed situations:

A contributor writes a new dts without common dtsi included, then it doesn't matter much.
A contributor has a common dtsi included, so it reduces the risk when the 3 battery properties have been moved to boards.
A contributor boots existing dtb without further works.

For example, when I tried to port A7, I even booted it with huawei-kiwi dtb, instead of writing a new device tree for it.
Mostly the contributors are supposed to write a new dts.
If not, it would be their choice?

source link	charge-term-current	constant-charge-current-max	constant-charge-voltage-max	comments
Core Max	150 mA	1000 mA	4.4 V	Also known as kleoslte
Mega 2	200 mA	1500 mA	4.35 V	Also known as vastalte
J1 Verizon	100 mA	1500 mA	4.4 V	Also known as j1qltevzw. Uses `sm5701-charger` instead of `rt5033-charger`

As far as I know, there are some MSM8916 phones yet to be ported.
Maybe we can also take these devices into considerations, for their safety's sake.

fortuna series is not in upstream yet anyway, and we still have much time and room for discussions :D

Jakko3 · 2023-08-19T21:59:52Z

I think there are two things I'm worried about:

Transparency. It should be easy to spot what charging values are applied.
Incomplete new dts files. If someone doesn't provide explicitely charging values for a device, the rt5033-charging driver is supposed to fail and complain about the missing information in dmesg.

On the first point: In the current commit set some devices have values directly implemented in their dts files (like a7), some devices have overriden values (like grandmax, heatqlte values are even overriden twice) and some devices have no values in their dts (like fortuna3g, fortunaltezt, gprimeltecan). While I honor the efficiency of your implementation, it is hard keep track. Placing the values in each device dts is more clear and straight foreward.

On the second point: To inherit charging values is dangerous. Especially if it can happen without knowledge. The rt5033-charger driver fails if one of the five charging values is missing and states this in dmesg [edit: and falls back to the default charging values of the rt5033 chip, which is slow charging, unfortunately up to 4.4 V]. If someone doesn't provide charging values for a new device, the rt5033-charging driver should fail. The situation 2. you mentioned above would prevent this and increase the risk something is wrong while everything on rt5033 looks ok in dmesg.

If people do copy-paste, we cannot prevent them copying wrong values. However, refering to point one: If each device dts contains charging values, people at least see that they are copy-pasting charging values. In the current implementation this is not fully clear in all cases.

In downstream each device (even local variants) has a separate dtsi file for the battery charging values as such.

wonderfulShrineMaidenOfParadise · 2023-08-24T16:11:41Z

I don't see any point to insist on common dtsi, since it slows the progress
down.

@Jakko3 out of curious. If I pick up a Core Prime battery and use it
to power up Grand Prime, or doing vice versa, are the parameters still
valid? I would ask about this because the phones have been released for
years, and the batteries could be turned into spicy pillows, which makes
users choose to do so.

Jakko3 · 2023-08-27T23:53:21Z

Once again sorry for the delayed answer. I'm now back home.

I tested the flash LED patch on samsung-serranove. It works!

On the mfd/charger patches: Thanks for adding the variable battery parameters to every device seperately. I hope it's ok for you that way. I looked through the commits and it now looks good to me!

On your question of switching the batteries of Core Prime and Grand Prime: That must not be done and is quite dangerous! The charging parameters are not correct anymore. Though it's the same if you run the phones on Android: The charging parameters are set in the kernel, putting in a wrong battery leads to charging that battery with wrong parameters, which is hazardous.

The mainlined rt5033-charger driver could be extended and improved. E.g. there are some IRQ triggers available that could increase safety but are not implemented, see links 10-12 in the cover sheet of patchset v1. RT5033_CHTREGI_IRQ is thermal regulation IRQ, RT5033_VINOVPI_IRQ is overvoltage protection IRQ. In the Android driver, some of those things seem to be implemented in the general samsung battery infrastructure, see link 3 of the mentioned cover sheet. However, I don't know if they would help much when putting a completely different battery in the device, the charging parameters would still be wrong.

Jakko3 · 2023-08-31T06:38:17Z

It got a bit lost in my last comment: My part of the review is finished. I'm fine with the current implementation.

Jakko3 · 2023-08-31T06:44:40Z

And @wonderfulShrineMaidenOfParadise: Thanks for adding it to all these devices!

stephan-gh · 2023-08-31T09:24:56Z

Given the open questions with the flash LED on the mailing lists I would like to postpone that part until this is decided and just take the battery/charging stuff for now. Do you mind splitting up into two PRs again?

wonderfulShrineMaidenOfParadise · 2023-08-31T12:05:58Z

Opened #327 for RT5033 LED.

arch/arm64/boot/dts/qcom/msm8916-samsung-a2015-common.dtsi

arch/arm64/boot/dts/qcom/msm8916-samsung-gprime-common.dtsi

arch/arm64/boot/dts/qcom/msm8939-samsung-a7.dts

The phones listed below have Richteck RT5033 PMIC and charger. Add them to the device trees. - Samsung Galaxy A3/A5 2015 - Samsung Galaxy E5/E7 - Samsung Galaxy Grand Max Signed-off-by: Raymond Hackley <[email protected]>

Samsung Galaxy A7 has Richteck RT5033 PMIC and charger. Add them to the device tree. Signed-off-by: Raymond Hackley <[email protected]>

The phones listed below have Richteck RT5033 PMIC and charger. Add them to the device trees. - Samsung Galaxy Ace 4 - Samsung Galaxy Core Prime LTE - Samsung Galaxy Grand Prime Signed-off-by: Raymond Hackley <[email protected]>

Jakko3 · 2023-09-05T00:15:33Z

For A7 the "constant-charge-voltage-max" is now 4.3 V and the "charge-term-current" at 150 mA. For some of the other devices the "charge-term-current" is at 200 mA according to the table further above.

I'd say this is ok for a release.

Further testing of the charging behavior after the release is welcome, especially the recharging cycles behavior at "full" battery by monitoring the voltage in sysfs. For devices with "charge-term-current" at 200 mA, lowering this to 150 mA might improve the behavior. In case A7 doesn't show nice recharging cycles behavior, a "constant-charge-voltage-max" of 4.325 V could be tested and discussed.

wonderfulShrineMaidenOfParadise · 2023-12-05T09:41:50Z

Upstreaming a2015 series
Link: https://lore.kernel.org/lkml/[email protected]/

[ Upstream commit a853450 ] When calling crypto_finalize_request, BH should be disabled to avoid triggering the following calltrace: ------------[ cut here ]------------ WARNING: CPU: 2 PID: 74 at crypto/crypto_engine.c:58 crypto_finalize_request+0xa0/0x118 Modules linked in: cryptodev(O) CPU: 2 PID: 74 Comm: firmware:zynqmp Tainted: G O 6.8.0-rc1-yocto-standard msm8916-mainline#323 Hardware name: ZynqMP ZCU102 Rev1.0 (DT) pstate: 40000005 (nZcv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--) pc : crypto_finalize_request+0xa0/0x118 lr : crypto_finalize_request+0x104/0x118 sp : ffffffc085353ce0 x29: ffffffc085353ce0 x28: 0000000000000000 x27: ffffff8808ea8688 x26: ffffffc081715038 x25: 0000000000000000 x24: ffffff880100db00 x23: ffffff880100da80 x22: 0000000000000000 x21: 0000000000000000 x20: ffffff8805b14000 x19: ffffff880100da80 x18: 0000000000010450 x17: 0000000000000000 x16: 0000000000000000 x15: 0000000000000000 x14: 0000000000000003 x13: 0000000000000000 x12: ffffff880100dad0 x11: 0000000000000000 x10: ffffffc0832dcd08 x9 : ffffffc0812416d8 x8 : 00000000000001f4 x7 : ffffffc0830d2830 x6 : 0000000000000001 x5 : ffffffc082091000 x4 : ffffffc082091658 x3 : 0000000000000000 x2 : ffffffc7f9653000 x1 : 0000000000000000 x0 : ffffff8802d20000 Call trace: crypto_finalize_request+0xa0/0x118 crypto_finalize_aead_request+0x18/0x30 zynqmp_handle_aes_req+0xcc/0x388 crypto_pump_work+0x168/0x2d8 kthread_worker_fn+0xfc/0x3a0 kthread+0x118/0x138 ret_from_fork+0x10/0x20 irq event stamp: 40 hardirqs last enabled at (39): [<ffffffc0812416f8>] _raw_spin_unlock_irqrestore+0x70/0xb0 hardirqs last disabled at (40): [<ffffffc08122d208>] el1_dbg+0x28/0x90 softirqs last enabled at (36): [<ffffffc080017dec>] kernel_neon_begin+0x8c/0xf0 softirqs last disabled at (34): [<ffffffc080017dc0>] kernel_neon_begin+0x60/0xf0 ---[ end trace 0000000000000000 ]--- Fixes: 4d96f7d ("crypto: xilinx - Add Xilinx AES driver") Signed-off-by: Quanyang Wang <[email protected]> Signed-off-by: Herbert Xu <[email protected]> Signed-off-by: Sasha Levin <[email protected]>

wonderfulShrineMaidenOfParadise force-pushed the rt5033 branch from 82b2d62 to 414b336 Compare August 13, 2023 02:18

wonderfulShrineMaidenOfParadise changed the base branch from msm8916/6.5-rc5 to msm8916/6.5-rc6 August 14, 2023 13:00

wonderfulShrineMaidenOfParadise force-pushed the rt5033 branch from 0265501 to 291e753 Compare August 14, 2023 13:07

wonderfulShrineMaidenOfParadise force-pushed the rt5033 branch from 291e753 to 740d757 Compare August 15, 2023 03:35

wonderfulShrineMaidenOfParadise force-pushed the rt5033 branch 2 times, most recently from a85b318 to 17b79cc Compare August 16, 2023 04:08

wonderfulShrineMaidenOfParadise force-pushed the rt5033 branch from 17b79cc to 4495805 Compare August 16, 2023 05:17

wonderfulShrineMaidenOfParadise force-pushed the rt5033 branch from 4495805 to a08dac8 Compare August 17, 2023 02:27

wonderfulShrineMaidenOfParadise commented Aug 18, 2023

View reviewed changes

arch/arm64/boot/dts/qcom/msm8916-samsung-gprimeltecan.dts Outdated Show resolved Hide resolved

arch/arm64/boot/dts/qcom/msm8916-samsung-e5.dts Outdated Show resolved Hide resolved

arch/arm64/boot/dts/qcom/msm8939-samsung-a7.dts Outdated Show resolved Hide resolved

wonderfulShrineMaidenOfParadise force-pushed the rt5033 branch from a08dac8 to b0301ef Compare August 18, 2023 00:46

wonderfulShrineMaidenOfParadise changed the base branch from msm8916/6.5-rc6 to msm8916/6.5-rc7 August 22, 2023 13:50

wonderfulShrineMaidenOfParadise force-pushed the rt5033 branch from b0301ef to b53c4d7 Compare August 24, 2023 15:38

wonderfulShrineMaidenOfParadise force-pushed the rt5033 branch from b53c4d7 to e9aa5ea Compare August 31, 2023 11:59

wonderfulShrineMaidenOfParadise changed the title ~~arm64: dts: qcom: msm8916/39-samsung-a2015/fortuna/serranove: Add flash LED, MFD and charger~~ arm64: dts: qcom: msm8916/39-samsung-a2015/fortuna/serranove: Add MFD and charger Aug 31, 2023

wonderfulShrineMaidenOfParadise mentioned this pull request Aug 31, 2023

Add RT5033 charger device driver #176

Closed

stephan-gh reviewed Aug 31, 2023

View reviewed changes

arch/arm64/boot/dts/qcom/msm8916-samsung-a2015-common.dtsi Outdated Show resolved Hide resolved

arch/arm64/boot/dts/qcom/msm8916-samsung-a2015-common.dtsi Outdated Show resolved Hide resolved

arch/arm64/boot/dts/qcom/msm8916-samsung-a2015-common.dtsi Outdated Show resolved Hide resolved

wonderfulShrineMaidenOfParadise force-pushed the rt5033 branch 2 times, most recently from 6387cbd to ea6549f Compare August 31, 2023 15:03

wonderfulShrineMaidenOfParadise changed the title ~~arm64: dts: qcom: msm8916/39-samsung-a2015/fortuna/serranove: Add MFD and charger~~ arm64: dts: qcom: msm8916/39-samsung-a2015/fortuna/serranove: Add PMIC and charger Aug 31, 2023

stephan-gh reviewed Sep 1, 2023

View reviewed changes

wonderfulShrineMaidenOfParadise force-pushed the rt5033 branch 2 times, most recently from eadea38 to 629a4d1 Compare September 4, 2023 11:52

wonderfulShrineMaidenOfParadise added 3 commits September 4, 2023 12:00

arm64: dts: qcom: msm8916-samsung-a2015: Add PMIC and charger

92b3acf

The phones listed below have Richteck RT5033 PMIC and charger. Add them to the device trees. - Samsung Galaxy A3/A5 2015 - Samsung Galaxy E5/E7 - Samsung Galaxy Grand Max Signed-off-by: Raymond Hackley <[email protected]>

arm64: dts: qcom: msm8939-samsung-a7: Add PMIC and charger

a0f5471

Samsung Galaxy A7 has Richteck RT5033 PMIC and charger. Add them to the device tree. Signed-off-by: Raymond Hackley <[email protected]>

wonderfulShrineMaidenOfParadise force-pushed the rt5033 branch from 629a4d1 to 591fa91 Compare September 4, 2023 12:00

wonderfulShrineMaidenOfParadise changed the title ~~arm64: dts: qcom: msm8916/39-samsung-a2015/fortuna/serranove: Add PMIC and charger~~ arm64: dts: qcom: msm8916/39-samsung-a2015/fortuna: Add PMIC and charger Sep 4, 2023

stephan-gh merged commit 591fa91 into msm8916-mainline:msm8916/6.5-rc7 Sep 10, 2023

wonderfulShrineMaidenOfParadise deleted the rt5033 branch September 10, 2023 16:06

arm64: dts: qcom: msm8916/39-samsung-a2015/fortuna: Add PMIC and charger #323

arm64: dts: qcom: msm8916/39-samsung-a2015/fortuna: Add PMIC and charger #323

Uh oh!

Conversation

wonderfulShrineMaidenOfParadise commented Aug 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stephan-gh commented Aug 12, 2023

Uh oh!

Jakko3 commented Aug 15, 2023

Uh oh!

wonderfulShrineMaidenOfParadise commented Aug 15, 2023

Uh oh!

Jakko3 commented Aug 16, 2023

Uh oh!

wonderfulShrineMaidenOfParadise commented Aug 16, 2023

Uh oh!

stephan-gh commented Aug 16, 2023

Uh oh!

Jakko3 commented Aug 17, 2023

Uh oh!

Jakko3 commented Aug 17, 2023

Uh oh!

wonderfulShrineMaidenOfParadise commented Aug 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wonderfulShrineMaidenOfParadise left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Jakko3 commented Aug 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wonderfulShrineMaidenOfParadise commented Aug 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Jakko3 commented Aug 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wonderfulShrineMaidenOfParadise commented Aug 24, 2023

Uh oh!

Jakko3 commented Aug 27, 2023

Uh oh!

Jakko3 commented Aug 31, 2023

Uh oh!

Jakko3 commented Aug 31, 2023

Uh oh!

stephan-gh commented Aug 31, 2023

Uh oh!

wonderfulShrineMaidenOfParadise commented Aug 31, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Jakko3 commented Sep 5, 2023

Uh oh!

wonderfulShrineMaidenOfParadise commented Dec 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

wonderfulShrineMaidenOfParadise commented Aug 12, 2023 •

edited

Loading

wonderfulShrineMaidenOfParadise commented Aug 18, 2023 •

edited

Loading

Jakko3 commented Aug 18, 2023 •

edited

Loading

wonderfulShrineMaidenOfParadise commented Aug 19, 2023 •

edited

Loading

Jakko3 commented Aug 19, 2023 •

edited

Loading

wonderfulShrineMaidenOfParadise commented Dec 5, 2023 •

edited

Loading