| Message ID | 20241202-b4-gs101_max77759_fg-v1-2-98d2fa7bfe30@uclouvain.be (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Google Pixel 6 (oriole): max77759 fuel gauge enablement and driver support | expand |
On 02/12/2024 14:07, Thomas Antoine via B4 Relay wrote: > From: Thomas Antoine <t.antoine@uclouvain.be> > > As the Maxim max77759 fuel gauge has no non-volatile memory slave address, s/max77759/MAX77759/ Please explain the device in general, e.g. fuel gauge is only one part of the PMIC chip. Otherwise 'fg' compatible suffix would not be justified. > make it non-obligatory. Except for this, the max77759 seems to behave the > same as the max1720x. > > Signed-off-by: Thomas Antoine <t.antoine@uclouvain.be> > --- > .../devicetree/bindings/power/supply/maxim,max17201.yaml | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > > diff --git a/Documentation/devicetree/bindings/power/supply/maxim,max17201.yaml b/Documentation/devicetree/bindings/power/supply/maxim,max17201.yaml > index fe3dd9bd5585618e45220c51023391a5b21acfd2..417fc2c4a1c1961654bc54ec1ac24602012f3335 100644 > --- a/Documentation/devicetree/bindings/power/supply/maxim,max17201.yaml > +++ b/Documentation/devicetree/bindings/power/supply/maxim,max17201.yaml > @@ -16,6 +16,7 @@ properties: > compatible: > oneOf: > - const: maxim,max17201 > + - const: maxim,max77759-fg > - items: > - enum: > - maxim,max17205 > @@ -25,11 +26,13 @@ properties: > items: > - description: ModelGauge m5 registers > - description: Nonvolatile registers > + minItems: 1 > > reg-names: > items: > - const: m5 > - const: nvmem > + minItems: 1 You need allOf:if:then section narrowing it per each variant. > > interrupts: > maxItems: 1 > @@ -56,3 +59,14 @@ examples: > interrupts = <31 IRQ_TYPE_LEVEL_LOW>; > }; > }; > + - | > + i2c { > + #address-cells = <1>; > + #size-cells = <0>; > + > + fuel-gauge@36 { > + compatible = "maxim,max77759-fg"; No need for new example if it differs with one property. Best regards, Krzysztof
On 12/2/24 14:39, Krzysztof Kozlowski wrote: > On 02/12/2024 14:07, Thomas Antoine via B4 Relay wrote: >> From: Thomas Antoine <t.antoine@uclouvain.be> >> >> As the Maxim max77759 fuel gauge has no non-volatile memory slave address, > > > s/max77759/MAX77759/ > > Please explain the device in general, e.g. fuel gauge is only one part > of the PMIC chip. Otherwise 'fg' compatible suffix would not be justified. The max77759 is an IC used to manage the power supply of the battery and the USB-C. Based on drivers from google, it contains at least a PMIC, a fuel gauge, a TPCI and a charger. Given I saw that the linked proposed patch, which adds a driver for the TCPCI, used the "maxim,max77759" compatible, I didn't want to create a possible eventual conflict. Link: https://lore.kernel.org/linux-devicetree/20241127-gs101-phy-lanes-orientation-dts-v1-0-5222d8508b71@linaro.org/ Will add this information to the commit description for v2. >> @@ -16,6 +16,7 @@ properties: >> compatible: >> oneOf: >> - const: maxim,max17201 >> + - const: maxim,max77759-fg >> - items: >> - enum: >> - maxim,max17205 >> @@ -25,11 +26,13 @@ properties: >> items: >> - description: ModelGauge m5 registers >> - description: Nonvolatile registers >> + minItems: 1 >> >> reg-names: >> items: >> - const: m5 >> - const: nvmem >> + minItems: 1 > > You need allOf:if:then section narrowing it per each variant. Will do in v2. >> interrupts: >> maxItems: 1 >> @@ -56,3 +59,14 @@ examples: >> interrupts = <31 IRQ_TYPE_LEVEL_LOW>; >> }; >> }; >> + - | >> + i2c { >> + #address-cells = <1>; >> + #size-cells = <0>; >> + >> + fuel-gauge@36 { >> + compatible = "maxim,max77759-fg"; > > > No need for new example if it differs with one property. Will remove in v2.
On Mon, 2024-12-02 at 14:07 +0100, Thomas Antoine via B4 Relay wrote: > From: Thomas Antoine <t.antoine@uclouvain.be> > > As the Maxim max77759 fuel gauge has no non-volatile memory slave address, > make it non-obligatory. Except for this, the max77759 seems to behave the > same as the max1720x. It also needs an interrupt line, and the previously mentioned shunt- resistor-micro-ohms, and probably a power supply. Cheers, Andre'
On Mon, 2024-12-02 at 14:07 +0100, Thomas Antoine via B4 Relay wrote: > From: Thomas Antoine <t.antoine@uclouvain.be> > > As the Maxim max77759 fuel gauge has no non-volatile memory slave address, > make it non-obligatory. Except for this, the max77759 seems to behave the > same as the max1720x. What about the battery characterization tables? Aren't they needed for correct reporting? Cheers, Andre'
On 12/3/24 07:57, André Draszik wrote: > On Mon, 2024-12-02 at 14:07 +0100, Thomas Antoine via B4 Relay wrote: >> From: Thomas Antoine <t.antoine@uclouvain.be> >> >> As the Maxim max77759 fuel gauge has no non-volatile memory slave address, >> make it non-obligatory. Except for this, the max77759 seems to behave the >> same as the max1720x. > > It also needs an interrupt line, and the previously mentioned shunt- > resistor-micro-ohms, and probably a power supply. I will try to add the interrupt line for v2. About the power supply, I didn't see anything about it in the devicetree from Google. Even it there is one, I guess it would be a single power supply for the whole max77759, not just the fuel gauge. Wouldn't it be more logical to have an mfd which activates the supply when other functions of the max77759 have been implemented? Best regards, Thomas
On Tue, 2024-12-03 at 10:32 +0100, Thomas Antoine wrote: > On 12/3/24 07:57, André Draszik wrote: > > On Mon, 2024-12-02 at 14:07 +0100, Thomas Antoine via B4 Relay wrote: > > > From: Thomas Antoine <t.antoine@uclouvain.be> > > > > > > As the Maxim max77759 fuel gauge has no non-volatile memory slave address, > > > make it non-obligatory. Except for this, the max77759 seems to behave the > > > same as the max1720x. > > > > It also needs an interrupt line, and the previously mentioned shunt- > > resistor-micro-ohms, and probably a power supply. > > I will try to add the interrupt line for v2. About the power supply, I > didn't see anything about it in the devicetree from Google. Even it > there is one, I guess it would be a single power supply for the whole > max77759, not just the fuel gauge. Wouldn't it be more logical to have an > mfd which activates the supply when other functions of the max77759 have > been implemented? I've looked it up now and for power supply nothing indeed seems necessary. Cheers, Andre'
On 12/3/24 08:12, André Draszik wrote: > On Mon, 2024-12-02 at 14:07 +0100, Thomas Antoine via B4 Relay wrote: >> From: Thomas Antoine <t.antoine@uclouvain.be> >> >> As the Maxim max77759 fuel gauge has no non-volatile memory slave address, >> make it non-obligatory. Except for this, the max77759 seems to behave the >> same as the max1720x. > > What about the battery characterization tables? Aren't they needed for > correct reporting? I checked some other patches which added fuel gauge and other bindings and I couldn't find such characterization table. Can you point me to an example or explain what it should contain if there needs one? Best regards, Thomas
On Tue, 2024-12-03 at 11:23 +0100, Thomas Antoine wrote: > On 12/3/24 08:12, André Draszik wrote: > > On Mon, 2024-12-02 at 14:07 +0100, Thomas Antoine via B4 Relay wrote: > > > From: Thomas Antoine <t.antoine@uclouvain.be> > > > > > > As the Maxim max77759 fuel gauge has no non-volatile memory slave address, > > > make it non-obligatory. Except for this, the max77759 seems to behave the > > > same as the max1720x. > > > > What about the battery characterization tables? Aren't they needed for > > correct reporting? > > I checked some other patches which added fuel gauge and other bindings and I > couldn't find such characterization table. Can you point me to an example or > explain what it should contain if there needs one? I haven't looked in detail, but there is https://android.googlesource.com/kernel/google-modules/raviole-device/+/refs/heads/android-gs-raviole-mainline/arch/arm64/boot/dts/google/gs101-oriole-battery.dtsi#13 https://android.googlesource.com/kernel/google-modules/raviole-device/+/refs/heads/android-gs-raviole-mainline/arch/arm64/boot/dts/google/gs101-raven-battery.dtsi#13 which include https://android.googlesource.com/kernel/google-modules/raviole-device/+/refs/heads/android-gs-raviole-mainline/arch/arm64/boot/dts/google/gs101-oriole-battery-data.dtsi https://android.googlesource.com/kernel/google-modules/raviole-device/+/refs/heads/android-gs-raviole-mainline/arch/arm64/boot/dts/google/gs101-raven-battery-data.dtsi respectively Both overlay https://android.googlesource.com/kernel/google-modules/raviole-device/+/refs/heads/android-gs-raviole-mainline/arch/arm64/boot/dts/google/gs101-raviole-battery.dtsi#177 Cheers, Andre'
On 12/3/24 11:40, André Draszik wrote: > On Tue, 2024-12-03 at 11:23 +0100, Thomas Antoine wrote: >> On 12/3/24 08:12, André Draszik wrote: >>> On Mon, 2024-12-02 at 14:07 +0100, Thomas Antoine via B4 Relay wrote: >>>> From: Thomas Antoine <t.antoine@uclouvain.be> >>>> >>>> As the Maxim max77759 fuel gauge has no non-volatile memory slave address, >>>> make it non-obligatory. Except for this, the max77759 seems to behave the >>>> same as the max1720x. >>> >>> What about the battery characterization tables? Aren't they needed for >>> correct reporting? >> >> I checked some other patches which added fuel gauge and other bindings and I >> couldn't find such characterization table. Can you point me to an example or >> explain what it should contain if there needs one? > > I haven't looked in detail, but there is > > > https://android.googlesource.com/kernel/google-modules/raviole-device/+/refs/heads/android-gs-raviole-mainline/arch/arm64/boot/dts/google/gs101-oriole-battery.dtsi#13 > https://android.googlesource.com/kernel/google-modules/raviole-device/+/refs/heads/android-gs-raviole-mainline/arch/arm64/boot/dts/google/gs101-raven-battery.dtsi#13 > > which include > https://android.googlesource.com/kernel/google-modules/raviole-device/+/refs/heads/android-gs-raviole-mainline/arch/arm64/boot/dts/google/gs101-oriole-battery-data.dtsi > https://android.googlesource.com/kernel/google-modules/raviole-device/+/refs/heads/android-gs-raviole-mainline/arch/arm64/boot/dts/google/gs101-raven-battery-data.dtsi > respectively > > Both overlay > https://android.googlesource.com/kernel/google-modules/raviole-device/+/refs/heads/android-gs-raviole-mainline/arch/arm64/boot/dts/google/gs101-raviole-battery.dtsi#177 I looked into it. The probe function launches a delay work max1720x_model_work which will try multiple times to run max1720x_model_load which leads to max_m5_load_gauge_model -> max_m5_update_custom_model This last function writes 0x0059 to 0x62 and 0x00c4 to 0x63 which unlocks the addresses from 0x80 to 0xaf. Those actually contain the model but are usually locked, returning only 0xff. It then writes the model and locks back the register. I tried it and I was indeed able to access this data on my device. The registers contained a model very close to the default-a1-0k fg-model contained in the downstream devicetree. The only diffrence is that all the 0x0100 are replaced with 0x0080. I think those registers are similar to the registers 180h to 1AFh of the max1720x ("ModelGauge m5 Algorithm Model registers" in the datasheet). The fg-params is used to set individual registers like CGAIN or CONFIG2 but from what I see, those are also on the max1720x. If it is indeed the case and that all of those are equivalent to their max1720x counterpart, I think the management of those values should be added in another patch which implements it for both the max1720x (and possibly the max77759) as the mainline driver does not do anything with those values currently. Best, Thomas
On Wed, 2024-12-04 at 14:13 +0100, Thomas Antoine wrote: > On 12/3/24 11:40, André Draszik wrote: > > On Tue, 2024-12-03 at 11:23 +0100, Thomas Antoine wrote: > > > On 12/3/24 08:12, André Draszik wrote: > > > > On Mon, 2024-12-02 at 14:07 +0100, Thomas Antoine via B4 Relay wrote: > > > > > From: Thomas Antoine <t.antoine@uclouvain.be> > > > > > > > > > > As the Maxim max77759 fuel gauge has no non-volatile memory slave address, > > > > > make it non-obligatory. Except for this, the max77759 seems to behave the > > > > > same as the max1720x. > > > > > > > > What about the battery characterization tables? Aren't they needed for > > > > correct reporting? [...] > > > I looked into it. The probe function launches a delay work > max1720x_model_work which will try multiple times to run > max1720x_model_load which leads to > max_m5_load_gauge_model -> max_m5_update_custom_model > > This last function writes 0x0059 to 0x62 and 0x00c4 to 0x63 which unlocks > the addresses from 0x80 to 0xaf. OK. The regmap I had proposed was excluding those based on the datasheet I have, but you probably noticed. [...] > If it is indeed the case and that all of those are equivalent to their > max1720x counterpart, I think the management of those values should be > added in another patch which implements it for both the max1720x (and possibly the > max77759) as the mainline driver does not do anything with those values > currently. Thanks for the analysis! And yes, I agree. Adding new required properties to a DT binding is an ABI break, therefore I was trying to ensure the binding is complete from the start. Cheers, Andre'
diff --git a/Documentation/devicetree/bindings/power/supply/maxim,max17201.yaml b/Documentation/devicetree/bindings/power/supply/maxim,max17201.yaml index fe3dd9bd5585618e45220c51023391a5b21acfd2..417fc2c4a1c1961654bc54ec1ac24602012f3335 100644 --- a/Documentation/devicetree/bindings/power/supply/maxim,max17201.yaml +++ b/Documentation/devicetree/bindings/power/supply/maxim,max17201.yaml @@ -16,6 +16,7 @@ properties: compatible: oneOf: - const: maxim,max17201 + - const: maxim,max77759-fg - items: - enum: - maxim,max17205 @@ -25,11 +26,13 @@ properties: items: - description: ModelGauge m5 registers - description: Nonvolatile registers + minItems: 1 reg-names: items: - const: m5 - const: nvmem + minItems: 1 interrupts: maxItems: 1 @@ -56,3 +59,14 @@ examples: interrupts = <31 IRQ_TYPE_LEVEL_LOW>; }; }; + - | + i2c { + #address-cells = <1>; + #size-cells = <0>; + + fuel-gauge@36 { + compatible = "maxim,max77759-fg"; + reg = <0x36>; + reg-names = "m5"; + }; + };