[PATCH 0/4] Google Pixel 6 (oriole): max77759 fuel gauge enablement and driver support

[PATCH 0/4] Google Pixel 6 (oriole): max77759 fuel gauge enablement and driver support

[Thomas Antoine via B4 Relay]

The Google Pixel 6 has a Maxim max77759 which provides a fuel gauge with
the same interface as the Maxim max1720x, except for the non-volatile
memory.

Modify the Maxim max1720x driver to be compatible with the Maxim max77759 and
enable it for the gs101-oriole board.

Signed-off-by: Thomas Antoine <t.antoine@uclouvain.be>
---
Thomas Antoine (4):
      power: supply: add support for max77759 fuel gauge
      dt-bindings: power: supply: add max77759-fg flavor and don't require nvme address
      arm64: defconfig: enable Maxim max1720x driver
      arm64: dts: exynos: gs101-oriole: enable Maxim max77759 fuel gauge

 .../bindings/power/supply/maxim,max17201.yaml      | 14 +++++
 arch/arm64/boot/dts/exynos/google/gs101-oriole.dts |  7 +++
 arch/arm64/configs/defconfig                       |  1 +
 drivers/power/supply/max1720x_battery.c            | 71 ++++++++++++++++++----
 4 files changed, 81 insertions(+), 12 deletions(-)
---
base-commit: 12e0a4072e8edc49c99418a4303bd7b96916de95
change-id: 20241202-b4-gs101_max77759_fg-402e231a4b33

Best regards,
-- 
Thomas Antoine <t.antoine@uclouvain.be>

[PATCH 1/4] power: supply: add support for max77759 fuel gauge

[Thomas Antoine via B4 Relay]

From: Thomas Antoine <t.antoine@uclouvain.be>

The Maxim max77759 fuel gauge has the same interface as the Maxim max1720x
except for the non-volatile memory slave address which is not available.
No slave is available at address 0xb of the i2c bus, which is coherent
with the following driver from google: line 5836 disables non-volatile
memory for m5 gauge.

Link: https://android.googlesource.com/kernel/google-modules/bms/+/1a68c36bef474573cc8629cc1d121eb6a81ab68c/max1720x_battery.c

Add support for the max77759 by allowing to use the non-volatile
memory or not based on the chip. Value for RSense comes from the following
stock devicetree:

Link: https://android.googlesource.com/kernel/devices/google/gs101/+/33eca36d43da6c2b6a546806eb3e7411bbe6d60d/dts/gs101-raviole-battery.dtsi

Signed-off-by: Thomas Antoine <t.antoine@uclouvain.be>
---
 drivers/power/supply/max1720x_battery.c | 71 +++++++++++++++++++++++++++------
 1 file changed, 59 insertions(+), 12 deletions(-)

diff --git a/drivers/power/supply/max1720x_battery.c b/drivers/power/supply/max1720x_battery.c
index 33105419e2427bb37963bda9948b647c239f8faa..faf336938dd4306dd2ceeb0a84b90ca80ad41a9f 100644
--- a/drivers/power/supply/max1720x_battery.c
+++ b/drivers/power/supply/max1720x_battery.c
@@ -13,6 +13,7 @@
 #include <linux/nvmem-provider.h>
 #include <linux/power_supply.h>
 #include <linux/regmap.h>
+#include <linux/types.h>
 
 #include <linux/unaligned.h>
 
@@ -39,6 +40,7 @@
 #define MAX172XX_DEV_NAME_TYPE_MASK	GENMASK(3, 0)
 #define MAX172XX_DEV_NAME_TYPE_MAX17201	BIT(0)
 #define MAX172XX_DEV_NAME_TYPE_MAX17205	(BIT(0) | BIT(2))
+#define MAX172XX_DEV_NAME_TYPE_MAX77759	0
 #define MAX172XX_QR_TABLE10		0x22
 #define MAX172XX_BATT			0xDA	/* Battery voltage */
 #define MAX172XX_ATAVCAP		0xDF
@@ -46,6 +48,7 @@
 static const char *const max1720x_manufacturer = "Maxim Integrated";
 static const char *const max17201_model = "MAX17201";
 static const char *const max17205_model = "MAX17205";
+static const char *const max77759_model = "MAX77759";
 
 struct max1720x_device_info {
 	struct regmap *regmap;
@@ -54,6 +57,21 @@ struct max1720x_device_info {
 	int rsense;
 };
 
+struct chip_data {
+	u16 default_nrsense; /* in regs in 10^-5 */
+	u8 has_nvmem;
+};
+
+static const struct chip_data max1720x_data  = {
+	.default_nrsense = 1000,
+	.has_nvmem = 1,
+};
+
+static const struct chip_data max77759_data = {
+	.default_nrsense = 500,
+	.has_nvmem = 0,
+};
+
 /*
  * Model Gauge M5 Algorithm output register
  * Volatile data (must not be cached)
@@ -369,6 +387,8 @@ static int max1720x_battery_get_property(struct power_supply *psy,
 			val->strval = max17201_model;
 		else if (reg_val == MAX172XX_DEV_NAME_TYPE_MAX17205)
 			val->strval = max17205_model;
+		else if (reg_val == MAX172XX_DEV_NAME_TYPE_MAX77759)
+			val->strval = max77759_model;
 		else
 			return -ENODEV;
 		break;
@@ -416,7 +436,6 @@ static int max1720x_probe_nvmem(struct i2c_client *client,
 		.priv = info,
 	};
 	struct nvmem_device *nvmem;
-	unsigned int val;
 	int ret;
 
 	info->ancillary = i2c_new_ancillary_device(client, "nvmem", 0xb);
@@ -438,6 +457,27 @@ static int max1720x_probe_nvmem(struct i2c_client *client,
 		return PTR_ERR(info->regmap_nv);
 	}
 
+	nvmem = devm_nvmem_register(dev, &nvmem_config);
+	if (IS_ERR(nvmem)) {
+		dev_err(dev, "Could not register nvmem!");
+		return PTR_ERR(nvmem);
+	}
+
+	return 0;
+}
+
+static int max1720x_get_rsense(struct device *dev,
+					 struct max1720x_device_info *info,
+					 const struct chip_data *data)
+{
+	unsigned int val;
+	int ret;
+
+	if (!data->has_nvmem) {
+		info->rsense = data->default_nrsense;
+		return 0;
+	}
+
 	ret = regmap_read(info->regmap_nv, MAX1720X_NRSENSE, &val);
 	if (ret < 0) {
 		dev_err(dev, "Failed to read sense resistor value\n");
@@ -446,14 +486,9 @@ static int max1720x_probe_nvmem(struct i2c_client *client,
 
 	info->rsense = val;
 	if (!info->rsense) {
-		dev_warn(dev, "RSense not calibrated, set 10 mOhms!\n");
-		info->rsense = 1000; /* in regs in 10^-5 */
-	}
-
-	nvmem = devm_nvmem_register(dev, &nvmem_config);
-	if (IS_ERR(nvmem)) {
-		dev_err(dev, "Could not register nvmem!");
-		return PTR_ERR(nvmem);
+		dev_warn(dev, "RSense not calibrated, set %d mOhms!\n",
+						data->default_nrsense/100);
+		info->rsense = data->default_nrsense;
 	}
 
 	return 0;
@@ -474,6 +509,7 @@ static int max1720x_probe(struct i2c_client *client)
 	struct device *dev = &client->dev;
 	struct max1720x_device_info *info;
 	struct power_supply *bat;
+	const struct chip_data *data;
 	int ret;
 
 	info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL);
@@ -488,9 +524,19 @@ static int max1720x_probe(struct i2c_client *client)
 		return dev_err_probe(dev, PTR_ERR(info->regmap),
 				     "regmap initialization failed\n");
 
-	ret = max1720x_probe_nvmem(client, info);
+	data = device_get_match_data(dev);
+	if (!data)
+		return dev_err_probe(dev, ret, "Failed to get chip data\n");
+
+	if (data->has_nvmem) {
+		ret = max1720x_probe_nvmem(client, info);
+		if (ret)
+			return dev_err_probe(dev, ret, "Failed to probe nvmem\n");
+	}
+
+	ret = max1720x_get_rsense(dev, info, data);
 	if (ret)
-		return dev_err_probe(dev, ret, "Failed to probe nvmem\n");
+		return dev_err_probe(dev, ret, "Failed to get RSense");
 
 	bat = devm_power_supply_register(dev, &max1720x_bat_desc, &psy_cfg);
 	if (IS_ERR(bat))
@@ -501,7 +547,8 @@ static int max1720x_probe(struct i2c_client *client)
 }
 
 static const struct of_device_id max1720x_of_match[] = {
-	{ .compatible = "maxim,max17201" },
+	{ .compatible = "maxim,max17201", .data = (void *) &max1720x_data },
+	{ .compatible = "maxim,max77759-fg", .data = (void *) &max77759_data},
 	{}
 };
 MODULE_DEVICE_TABLE(of, max1720x_of_match);

-- 
2.47.1

[PATCH 2/4] dt-bindings: power: supply: add max77759-fg flavor and don't require nvme address

[Thomas Antoine via B4 Relay]

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.

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
 
   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";
+      };
+    };

-- 
2.47.1

[PATCH 3/4] arm64: defconfig: enable Maxim max1720x driver

[Thomas Antoine via B4 Relay]

From: Thomas Antoine <t.antoine@uclouvain.be>

Enable the Maxim max1720x as it is used by the gs101-oriole
(Google Pixel 6) board.

Signed-off-by: Thomas Antoine <t.antoine@uclouvain.be>
---
 arch/arm64/configs/defconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
index d13218d0c30f458d9c555ab9771a1fd9139ce1aa..e7523e53f2ed26bab39e64f3b2d8c8afc7c1a1f0 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -675,6 +675,7 @@ CONFIG_NVMEM_REBOOT_MODE=m
 CONFIG_BATTERY_QCOM_BATTMGR=m
 CONFIG_BATTERY_SBS=m
 CONFIG_BATTERY_BQ27XXX=y
+CONFIG_BATTERY_MAX1720X=m
 CONFIG_BATTERY_MAX17042=m
 CONFIG_CHARGER_MT6360=m
 CONFIG_CHARGER_BQ25890=m

-- 
2.47.1

[PATCH 4/4] arm64: dts: exynos: gs101-oriole: enable Maxim max77759 fuel gauge

[Thomas Antoine via B4 Relay]

From: Thomas Antoine <t.antoine@uclouvain.be>

Add the node for the max77759 fuel gauge as a slave of the i2c.

The fuel gauge has been tested and seems to give coherent results.
Manual activation of the charger via i2cset shows that the sign of
the current does indicate charging/discharging status.

Signed-off-by: Thomas Antoine <t.antoine@uclouvain.be>
---
 arch/arm64/boot/dts/exynos/google/gs101-oriole.dts | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/arm64/boot/dts/exynos/google/gs101-oriole.dts b/arch/arm64/boot/dts/exynos/google/gs101-oriole.dts
index 387fb779bd29ea3812331a7951f03b181c5fe659..4c45dd6fd0173889234b7b04d7abb4b382c7706c 100644
--- a/arch/arm64/boot/dts/exynos/google/gs101-oriole.dts
+++ b/arch/arm64/boot/dts/exynos/google/gs101-oriole.dts
@@ -90,6 +90,13 @@ eeprom: eeprom@50 {
 &hsi2c_12 {
 	status = "okay";
 	/* TODO: add the devices once drivers exist */
+
+	fuel-gauge@36 {
+		compatible = "maxim,max77759-fg";
+		reg = <0x36>;
+		reg-names = "m5";
+	};
+
 };
 
 &pinctrl_far_alive {

-- 
2.47.1

Re: [PATCH 2/4] dt-bindings: power: supply: add max77759-fg flavor and don't require nvme address

[Krzysztof Kozlowski]

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

Re: [PATCH 3/4] arm64: defconfig: enable Maxim max1720x driver

[Krzysztof Kozlowski]

On 02/12/2024 14:07, Thomas Antoine via B4 Relay wrote:
> From: Thomas Antoine <t.antoine@uclouvain.be>
> 
> Enable the Maxim max1720x as it is used by the gs101-oriole
> (Google Pixel 6) board.
> 
> Signed-off-by: Thomas Antoine <t.antoine@uclouvain.be>
> ---
>  arch/arm64/configs/defconfig | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
> index d13218d0c30f458d9c555ab9771a1fd9139ce1aa..e7523e53f2ed26bab39e64f3b2d8c8afc7c1a1f0 100644
> --- a/arch/arm64/configs/defconfig
> +++ b/arch/arm64/configs/defconfig
> @@ -675,6 +675,7 @@ CONFIG_NVMEM_REBOOT_MODE=m
>  CONFIG_BATTERY_QCOM_BATTMGR=m
>  CONFIG_BATTERY_SBS=m
>  CONFIG_BATTERY_BQ27XXX=y
> +CONFIG_BATTERY_MAX1720X=m
>  CONFIG_BATTERY_MAX17042=m

Are you sure this is placed according to savedefconfig order?

Best regards,
Krzysztof

Re: [PATCH 4/4] arm64: dts: exynos: gs101-oriole: enable Maxim max77759 fuel gauge

[Krzysztof Kozlowski]

On 02/12/2024 14:07, Thomas Antoine via B4 Relay wrote:
> From: Thomas Antoine <t.antoine@uclouvain.be>
> 
> Add the node for the max77759 fuel gauge as a slave of the i2c.
> 
> The fuel gauge has been tested and seems to give coherent results.
> Manual activation of the charger via i2cset shows that the sign of
> the current does indicate charging/discharging status.
> 
> Signed-off-by: Thomas Antoine <t.antoine@uclouvain.be>
> ---
>  arch/arm64/boot/dts/exynos/google/gs101-oriole.dts | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/exynos/google/gs101-oriole.dts b/arch/arm64/boot/dts/exynos/google/gs101-oriole.dts
> index 387fb779bd29ea3812331a7951f03b181c5fe659..4c45dd6fd0173889234b7b04d7abb4b382c7706c 100644
> --- a/arch/arm64/boot/dts/exynos/google/gs101-oriole.dts
> +++ b/arch/arm64/boot/dts/exynos/google/gs101-oriole.dts
> @@ -90,6 +90,13 @@ eeprom: eeprom@50 {
>  &hsi2c_12 {
>  	status = "okay";
>  	/* TODO: add the devices once drivers exist */


Is this still applicable?

> +
> +	fuel-gauge@36 {
> +		compatible = "maxim,max77759-fg";
> +		reg = <0x36>;
> +		reg-names = "m5";


No interrupts?

> +	};
> +


Do not add stray blank lines.

Best regards,
Krzysztof

Re: [PATCH 2/4] dt-bindings: power: supply: add max77759-fg flavor and don't require nvme address

[Thomas Antoine]

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.

Re: [PATCH 3/4] arm64: defconfig: enable Maxim max1720x driver

[Thomas Antoine]

On 12/2/24 14:40, Krzysztof Kozlowski wrote:
> On 02/12/2024 14:07, Thomas Antoine via B4 Relay wrote:
>> From: Thomas Antoine <t.antoine@uclouvain.be>
>>  CONFIG_BATTERY_QCOM_BATTMGR=m
>>  CONFIG_BATTERY_SBS=m
>>  CONFIG_BATTERY_BQ27XXX=y
>> +CONFIG_BATTERY_MAX1720X=m
>>  CONFIG_BATTERY_MAX17042=m
> 
> Are you sure this is placed according to savedefconfig order?

I ran menuconfig and it put CONFIG_BATTERY_MAX1720X below
CONFIG_BATTERY_MAX17042 so I think it is an error on my part.
Will fix in v2.

Best regards,
Thomas Antoine

Re: [PATCH 4/4] arm64: dts: exynos: gs101-oriole: enable Maxim max77759 fuel gauge

[Thomas Antoine]

On 12/2/24 14:41, Krzysztof Kozlowski wrote:
> On 02/12/2024 14:07, Thomas Antoine via B4 Relay wrote:
>> From: Thomas Antoine <t.antoine@uclouvain.be>
>>  &hsi2c_12 {
>>       status = "okay";
>>       /* TODO: add the devices once drivers exist */
> 
> 
> Is this still applicable?

Yes, there are other devices on the bus (the Maxim max77759 pmic, charger
and TPCI, the Maxim max20339 OVP and the NXP PCA9468).

>> +
>> +     fuel-gauge@36 {
>> +             compatible = "maxim,max77759-fg";
>> +             reg = <0x36>;
>> +             reg-names = "m5";
> 
> 
> No interrupts?

There are interrupts in the stock devicetree but they didn't compile out
of the box when adding them to the node without any other modification and
I didn't try further given the device worked without them. I can try to
get them to work for v2.

>> +     };
>> +
> 
> 
> Do not add stray blank lines.

Will remove in v2.

Best regards,
Thomas Antoine

Re: [PATCH 1/4] power: supply: add support for max77759 fuel gauge

[André Draszik]

Hi Thomas,

Thanks for looking into this!

> From: Thomas Antoine <t.antoine@uclouvain.be>
> 
> The Maxim max77759 fuel gauge has the same interface as the Maxim max1720x
> except for the non-volatile memory slave address which is not available.

It is not fully compatible, and it also has a lot more registers.

For example, the voltage now is not in register 0xda as this driver assumes.
With these changes, POWER_SUPPLY_PROP_VOLTAGE_NOW just reads as 0. 0xda
doesn't exist in max77759

I haven't compared in depth yet, though.

> No slave is available at address 0xb of the i2c bus, which is coherent
> with the following driver from google: line 5836 disables non-volatile
> memory for m5 gauge.
> 
> Link:
> https://android.googlesource.com/kernel/google-modules/bms/+/1a68c36bef474573cc8629cc1d121eb6a81ab68c/max1720x_battery.c
> 
> Add support for the max77759 by allowing to use the non-volatile
> memory or not based on the chip. Value for RSense comes from the following
> stock devicetree:
> 
> Link:
> https://android.googlesource.com/kernel/devices/google/gs101/+/33eca36d43da6c2b6a546806eb3e7411bbe6d60d/dts/gs101-raviole-battery.dtsi
> 
> Signed-off-by: Thomas Antoine <t.antoine@uclouvain.be>
> ---
>  drivers/power/supply/max1720x_battery.c | 71 +++++++++++++++++++++++++++-
> -----
>  1 file changed, 59 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/power/supply/max1720x_battery.c
> b/drivers/power/supply/max1720x_battery.c
> index
> 33105419e2427bb37963bda9948b647c239f8faa..faf336938dd4306dd2ceeb0a84b90ca8
> 0ad41a9f 100644
> --- a/drivers/power/supply/max1720x_battery.c
> +++ b/drivers/power/supply/max1720x_battery.c
> @@ -13,6 +13,7 @@
>  #include <linux/nvmem-provider.h>
>  #include <linux/power_supply.h>
>  #include <linux/regmap.h>
> +#include <linux/types.h>

Please keep file names in alphabetical order.

>  
>  #include <linux/unaligned.h>
>  
> @@ -39,6 +40,7 @@
>  #define MAX172XX_DEV_NAME_TYPE_MASK	GENMASK(3, 0)
>  #define MAX172XX_DEV_NAME_TYPE_MAX17201	BIT(0)
>  #define MAX172XX_DEV_NAME_TYPE_MAX17205	(BIT(0) | BIT(2))
> +#define MAX172XX_DEV_NAME_TYPE_MAX77759	0
>  #define MAX172XX_QR_TABLE10		0x22
>  #define MAX172XX_BATT			0xDA	/* Battery voltage */
>  #define MAX172XX_ATAVCAP		0xDF
> @@ -46,6 +48,7 @@
>  static const char *const max1720x_manufacturer = "Maxim Integrated";
>  static const char *const max17201_model = "MAX17201";
>  static const char *const max17205_model = "MAX17205";
> +static const char *const max77759_model = "MAX77759";
>  
>  struct max1720x_device_info {
>  	struct regmap *regmap;
> @@ -54,6 +57,21 @@ struct max1720x_device_info {
>  	int rsense;
>  };
>  
> +struct chip_data {
> +	u16 default_nrsense; /* in regs in 10^-5 */
> +	u8 has_nvmem;
> +};
> +
> +static const struct chip_data max1720x_data  = {
> +	.default_nrsense = 1000,
> +	.has_nvmem = 1,
> +};
> +
> +static const struct chip_data max77759_data = {
> +	.default_nrsense = 500,
> +	.has_nvmem = 0,
> +};

This should be made a required devicetree property instead, at least for
max77759, as it's completely board dependent, 'shunt-resistor-micro-ohms'
is widely used.

I also don't think there should be a default. The driver should just fail
to probe if not specified in DT (for max77759).

> +
>  /*
>   * Model Gauge M5 Algorithm output register
>   * Volatile data (must not be cached)
> @@ -369,6 +387,8 @@ static int max1720x_battery_get_property(struct
> power_supply *psy,
>  			val->strval = max17201_model;
>  		else if (reg_val == MAX172XX_DEV_NAME_TYPE_MAX17205)
>  			val->strval = max17205_model;
> +		else if (reg_val == MAX172XX_DEV_NAME_TYPE_MAX77759)
> +			val->strval = max77759_model;
>  		else

This is a 16 bit register, and while yes, MAX172XX_DEV_NAME_TYPE_MASK only
cares about the bottom 4 bits, the register is described as 'Firmware
Version Information'.

But maybe it's ok to do it like that, at least for now.

>  			return -ENODEV;
>  		break;
> @@ -416,7 +436,6 @@ static int max1720x_probe_nvmem(struct i2c_client
> *client,
>  		.priv = info,
>  	};
>  	struct nvmem_device *nvmem;
> -	unsigned int val;
>  	int ret;
>  
>  	info->ancillary = i2c_new_ancillary_device(client, "nvmem", 0xb);
> @@ -438,6 +457,27 @@ static int max1720x_probe_nvmem(struct i2c_client
> *client,
>  		return PTR_ERR(info->regmap_nv);
>  	}
>  
> +	nvmem = devm_nvmem_register(dev, &nvmem_config);
> +	if (IS_ERR(nvmem)) {
> +		dev_err(dev, "Could not register nvmem!");
> +		return PTR_ERR(nvmem);
> +	}
> +
> +	return 0;
> +}
> +
> +static int max1720x_get_rsense(struct device *dev,
> +					 struct max1720x_device_info
> *info,
> +					 const struct chip_data *data)
> +{
> +	unsigned int val;
> +	int ret;
> +
> +	if (!data->has_nvmem) {
> +		info->rsense = data->default_nrsense;
> +		return 0;
> +	}
> +
>  	ret = regmap_read(info->regmap_nv, MAX1720X_NRSENSE, &val);
>  	if (ret < 0) {
>  		dev_err(dev, "Failed to read sense resistor value\n");
> @@ -446,14 +486,9 @@ static int max1720x_probe_nvmem(struct i2c_client
> *client,
>  
>  	info->rsense = val;
>  	if (!info->rsense) {
> -		dev_warn(dev, "RSense not calibrated, set 10 mOhms!\n");
> -		info->rsense = 1000; /* in regs in 10^-5 */
> -	}
> -
> -	nvmem = devm_nvmem_register(dev, &nvmem_config);
> -	if (IS_ERR(nvmem)) {
> -		dev_err(dev, "Could not register nvmem!");
> -		return PTR_ERR(nvmem);
> +		dev_warn(dev, "RSense not calibrated, set %d mOhms!\n",
> +						data-
> >default_nrsense/100);
> +		info->rsense = data->default_nrsense;
>  	}
>  
>  	return 0;
> @@ -474,6 +509,7 @@ static int max1720x_probe(struct i2c_client *client)
>  	struct device *dev = &client->dev;
>  	struct max1720x_device_info *info;
>  	struct power_supply *bat;
> +	const struct chip_data *data;
>  	int ret;
>  
>  	info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL);
> @@ -488,9 +524,19 @@ static int max1720x_probe(struct i2c_client *client)
>  		return dev_err_probe(dev, PTR_ERR(info->regmap),
>  				     "regmap initialization failed\n");
>  
> -	ret = max1720x_probe_nvmem(client, info);
> +	data = device_get_match_data(dev);
> +	if (!data)
> +		return dev_err_probe(dev, ret, "Failed to get chip
> data\n");
> +
> +	if (data->has_nvmem) {
> +		ret = max1720x_probe_nvmem(client, info);
> +		if (ret)
> +			return dev_err_probe(dev, ret, "Failed to probe
> nvmem\n");
> +	}
> +
> +	ret = max1720x_get_rsense(dev, info, data);
>  	if (ret)
> -		return dev_err_probe(dev, ret, "Failed to probe
> nvmem\n");
> +		return dev_err_probe(dev, ret, "Failed to get RSense");
>  
>  	bat = devm_power_supply_register(dev, &max1720x_bat_desc,
> &psy_cfg);
>  	if (IS_ERR(bat))
> @@ -501,7 +547,8 @@ static int max1720x_probe(struct i2c_client *client)
>  }
>  
>  static const struct of_device_id max1720x_of_match[] = {
> -	{ .compatible = "maxim,max17201" },
> +	{ .compatible = "maxim,max17201", .data = (void *) &max1720x_data
> },
> +	{ .compatible = "maxim,max77759-fg", .data = (void *)
> &max77759_data},

missing space before }

Cheers,
Andre'

>  	{}
>  };
>  MODULE_DEVICE_TABLE(of, max1720x_of_match);
> 

Re: [PATCH 2/4] dt-bindings: power: supply: add max77759-fg flavor and don't require nvme address

[André Draszik]

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'

Re: [PATCH 2/4] dt-bindings: power: supply: add max77759-fg flavor and don't require nvme address

[André Draszik]

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'

Re: [PATCH 1/4] power: supply: add support for max77759 fuel gauge

[André Draszik]

On Tue, 2024-12-03 at 06:47 +0000, André Draszik wrote:
> Hi Thomas,
> 
> Thanks for looking into this!
> 
> > From: Thomas Antoine <t.antoine@uclouvain.be>
> > 
> > The Maxim max77759 fuel gauge has the same interface as the Maxim max1720x
> > except for the non-volatile memory slave address which is not available.
> 
> It is not fully compatible, and it also has a lot more registers.
> 
> For example, the voltage now is not in register 0xda as this driver assumes.
> With these changes, POWER_SUPPLY_PROP_VOLTAGE_NOW just reads as 0. 0xda
> doesn't exist in max77759
> 
> I haven't compared in depth yet, though.

Regarding the regmap in this driver, please see below comparison I had
collected some time ago:

	regmap_reg_range(0x24, 0x26), // exists
	regmap_reg_range(0x30, 0x31), // exists
	regmap_reg_range(0x33, 0x34), // exists
	regmap_reg_range(0x37, 0x37), // exists
	regmap_reg_range(0x3B, 0x3C), // exists
	regmap_reg_range(0x40, 0x41), // exists
	regmap_reg_range(0x43, 0x44), // exists
	regmap_reg_range(0x47, 0x49), // exists
	regmap_reg_range(0x4B, 0x4C), // exists
	regmap_reg_range(0x4E, 0xAF), // 0x4e 0x4f exists
	regmap_reg_range(0xB1, 0xB3), // exists
	regmap_reg_range(0xB5, 0xB7), // exists
	regmap_reg_range(0xBF, 0xD0), // 0xd0 exists
	0xd1 .. 0xdb don't exist
	regmap_reg_range(0xDB, 0xDB),
	regmap_reg_range(0xE0, 0xFF), // 0xfb 0xff exist

the comments refer to whether or not the register exists in max77759

> 
> > No slave is available at address 0xb of the i2c bus, which is coherent
> > with the following driver from google: line 5836 disables non-volatile
> > memory for m5 gauge.
> > 
> > Link:
> > https://android.googlesource.com/kernel/google-modules/bms/+/1a68c36bef474573cc8629cc1d121eb6a81ab68c/max1720x_battery.c
> > 
> > Add support for the max77759 by allowing to use the non-volatile
> > memory or not based on the chip. Value for RSense comes from the following
> > stock devicetree:
> > 
> > Link:
> > https://android.googlesource.com/kernel/devices/google/gs101/+/33eca36d43da6c2b6a546806eb3e7411bbe6d60d/dts/gs101-raviole-battery.dtsi
> > 
> > Signed-off-by: Thomas Antoine <t.antoine@uclouvain.be>
> > ---
> >  drivers/power/supply/max1720x_battery.c | 71 +++++++++++++++++++++++++++-
> > -----
> >  1 file changed, 59 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/power/supply/max1720x_battery.c
> > b/drivers/power/supply/max1720x_battery.c
> > index
> > 33105419e2427bb37963bda9948b647c239f8faa..faf336938dd4306dd2ceeb0a84b90ca8
> > 0ad41a9f 100644
> > --- a/drivers/power/supply/max1720x_battery.c
> > +++ b/drivers/power/supply/max1720x_battery.c
> > @@ -13,6 +13,7 @@
> >  #include <linux/nvmem-provider.h>
> >  #include <linux/power_supply.h>
> >  #include <linux/regmap.h>
> > +#include <linux/types.h>
> 
> Please keep file names in alphabetical order.

Ignore that, it's too early :-)



Cheers,
Andre'

Re: [PATCH 1/4] power: supply: add support for max77759 fuel gauge

[Dimitri Fedrau]

Hi Antoine,

Am Mon, Dec 02, 2024 at 02:07:15PM +0100 schrieb Thomas Antoine via B4 Relay:
> From: Thomas Antoine <t.antoine@uclouvain.be>
> 
> The Maxim max77759 fuel gauge has the same interface as the Maxim max1720x
> except for the non-volatile memory slave address which is not available.
> No slave is available at address 0xb of the i2c bus, which is coherent
> with the following driver from google: line 5836 disables non-volatile
> memory for m5 gauge.
> 
> Link: https://android.googlesource.com/kernel/google-modules/bms/+/1a68c36bef474573cc8629cc1d121eb6a81ab68c/max1720x_battery.c
> 
> Add support for the max77759 by allowing to use the non-volatile
> memory or not based on the chip. Value for RSense comes from the following
> stock devicetree:
> 
> Link: https://android.googlesource.com/kernel/devices/google/gs101/+/33eca36d43da6c2b6a546806eb3e7411bbe6d60d/dts/gs101-raviole-battery.dtsi
> 
> Signed-off-by: Thomas Antoine <t.antoine@uclouvain.be>
> ---
>  drivers/power/supply/max1720x_battery.c | 71 +++++++++++++++++++++++++++------
>  1 file changed, 59 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/power/supply/max1720x_battery.c b/drivers/power/supply/max1720x_battery.c
> index 33105419e2427bb37963bda9948b647c239f8faa..faf336938dd4306dd2ceeb0a84b90ca80ad41a9f 100644
> --- a/drivers/power/supply/max1720x_battery.c
> +++ b/drivers/power/supply/max1720x_battery.c
> @@ -13,6 +13,7 @@
>  #include <linux/nvmem-provider.h>
>  #include <linux/power_supply.h>
>  #include <linux/regmap.h>
> +#include <linux/types.h>
>  
No need to include it, it is done by <linux/i2.c> which includes
<linux/device.h> which includes <linux/types.h>
>  #include <linux/unaligned.h>
>  
> @@ -39,6 +40,7 @@
>  #define MAX172XX_DEV_NAME_TYPE_MASK	GENMASK(3, 0)
>  #define MAX172XX_DEV_NAME_TYPE_MAX17201	BIT(0)
>  #define MAX172XX_DEV_NAME_TYPE_MAX17205	(BIT(0) | BIT(2))
> +#define MAX172XX_DEV_NAME_TYPE_MAX77759	0
>  #define MAX172XX_QR_TABLE10		0x22
>  #define MAX172XX_BATT			0xDA	/* Battery voltage */
>  #define MAX172XX_ATAVCAP		0xDF
> @@ -46,6 +48,7 @@
>  static const char *const max1720x_manufacturer = "Maxim Integrated";
>  static const char *const max17201_model = "MAX17201";
>  static const char *const max17205_model = "MAX17205";
> +static const char *const max77759_model = "MAX77759";
>  
>  struct max1720x_device_info {
>  	struct regmap *regmap;
> @@ -54,6 +57,21 @@ struct max1720x_device_info {
>  	int rsense;
>  };
>  
> +struct chip_data {
> +	u16 default_nrsense; /* in regs in 10^-5 */
> +	u8 has_nvmem;
> +};
> +
> +static const struct chip_data max1720x_data  = {
> +	.default_nrsense = 1000,
> +	.has_nvmem = 1,
> +};
> +
> +static const struct chip_data max77759_data = {
> +	.default_nrsense = 500,
> +	.has_nvmem = 0,
> +};
> +
You can get rid of chip_data by reading rsense from DT and moving
has_nvmem to max1720x_device_info. By doing so you don't have to rely on
default values. Either it is specified by DT or by rsense value in
nvmem.

Best regards,
Dimitri Fedrau

Re: [PATCH 1/4] power: supply: add support for max77759 fuel gauge

[Thomas Antoine]

On 12/3/24 07:47, André Draszik wrote:
> Hi Thomas,
> 
> Thanks for looking into this!

Hi,

With pleasure! This is my first time trying to contribute to the kernel
so sorry for any beginner mistakes I might do.
 
>> From: Thomas Antoine <t.antoine@uclouvain.be>
>>
>> The Maxim max77759 fuel gauge has the same interface as the Maxim max1720x
>> except for the non-volatile memory slave address which is not available.
> 
> It is not fully compatible, and it also has a lot more registers.
> 
> For example, the voltage now is not in register 0xda as this driver assumes.
> With these changes, POWER_SUPPLY_PROP_VOLTAGE_NOW just reads as 0. 0xda
> doesn't exist in max77759
> 
> I haven't compared in depth yet, though.

Is the voltage necessary for the driver? If so, we could just not
read the voltage. If it is necessary, I can try to kook into it and try
to find in which register it is located (if there is one).

>>  static const char *const max17205_model = "MAX17205";
>> +static const char *const max77759_model = "MAX77759";
>>
>>  struct max1720x_device_info {
>>       struct regmap *regmap;
>> @@ -54,6 +57,21 @@ struct max1720x_device_info {
>>       int rsense;
>>  };
>>
>> +struct chip_data {
>> +     u16 default_nrsense; /* in regs in 10^-5 */
>> +     u8 has_nvmem;
>> +};
>> +
>> +static const struct chip_data max1720x_data  = {
>> +     .default_nrsense = 1000,
>> +     .has_nvmem = 1,
>> +};
>> +
>> +static const struct chip_data max77759_data = {
>> +     .default_nrsense = 500,
>> +     .has_nvmem = 0,
>> +};
> 
> This should be made a required devicetree property instead, at least for
> max77759, as it's completely board dependent, 'shunt-resistor-micro-ohms'
> is widely used.
> 
> I also don't think there should be a default. The driver should just fail
> to probe if not specified in DT (for max77759).

I hesitated to do this but I didn't know what would be better. Will change
for v2.

>> +
>>  /*
>>   * Model Gauge M5 Algorithm output register
>>   * Volatile data (must not be cached)
>> @@ -369,6 +387,8 @@ static int max1720x_battery_get_property(struct
>> power_supply *psy,
>>                       val->strval = max17201_model;
>>               else if (reg_val == MAX172XX_DEV_NAME_TYPE_MAX17205)
>>                       val->strval = max17205_model;
>> +             else if (reg_val == MAX172XX_DEV_NAME_TYPE_MAX77759)
>> +                     val->strval = max77759_model;
>>               else
> 
> This is a 16 bit register, and while yes, MAX172XX_DEV_NAME_TYPE_MASK only
> cares about the bottom 4 bits, the register is described as 'Firmware
> Version Information'.
> 
> But maybe it's ok to do it like that, at least for now.

I thought this method would be ok as long as there is no collision on
values. I hesitated to change the model evaluation method based on chip
model, where the max77759 would thus have an hard-coded value and the
max1720x would still evaluate the register value. I did not do it because
it led to a lot more changes for no difference.

>> &max77759_data},
> 
> missing space before }

Will change for v2.

Best regards,
Thomas

Re: [PATCH 1/4] power: supply: add support for max77759 fuel gauge

[Thomas Antoine]

On 12/3/24 08:35, Dimitri Fedrau wrote:
> Hi Antoine,
> 
> Am Mon, Dec 02, 2024 at 02:07:15PM +0100 schrieb Thomas Antoine via B4 Relay:
>> From: Thomas Antoine <t.antoine@uclouvain.be>
>> +#include <linux/types.h>
>>  
> No need to include it, it is done by <linux/i2.c> which includes
> <linux/device.h> which includes <linux/types.h>

Hi,
Will remove for v2.

>>  static const char *const max17201_model = "MAX17201";
>>  static const char *const max17205_model = "MAX17205";
>> +static const char *const max77759_model = "MAX77759";
>>  
>>  struct max1720x_device_info {
>>  	struct regmap *regmap;
>> @@ -54,6 +57,21 @@ struct max1720x_device_info {
>>  	int rsense;
>>  };
>>  
>> +struct chip_data {
>> +	u16 default_nrsense; /* in regs in 10^-5 */
>> +	u8 has_nvmem;
>> +};
>> +
>> +static const struct chip_data max1720x_data  = {
>> +	.default_nrsense = 1000,
>> +	.has_nvmem = 1,
>> +};
>> +
>> +static const struct chip_data max77759_data = {
>> +	.default_nrsense = 500,
>> +	.has_nvmem = 0,
>> +};
>> +
> You can get rid of chip_data by reading rsense from DT and moving
> has_nvmem to max1720x_device_info. By doing so you don't have to rely on
> default values. Either it is specified by DT or by rsense value in
> nvmem.

I guess I can just get has_nvmem by seeing if of_property_read of the
rsense value fails. As long as the binding is well defined as to not
allow a rsense value for the max1720x, there should be no problem.
Will do that for v2.

Thanks, best regards,
Thomas

Re: [PATCH 1/4] power: supply: add support for max77759 fuel gauge

[André Draszik]

On Tue, 2024-12-03 at 10:08 +0100, Thomas Antoine wrote:
> On 12/3/24 07:47, André Draszik wrote:
> > Hi Thomas,
> > 
> > Thanks for looking into this!
> 
> Hi,
> 
> With pleasure! This is my first time trying to contribute to the kernel
> so sorry for any beginner mistakes I might do.

No worries :-)

>  
> > > From: Thomas Antoine <t.antoine@uclouvain.be>
> > > 
> > > The Maxim max77759 fuel gauge has the same interface as the Maxim max1720x
> > > except for the non-volatile memory slave address which is not available.
> > 
> > It is not fully compatible, and it also has a lot more registers.
> > 
> > For example, the voltage now is not in register 0xda as this driver assumes.
> > With these changes, POWER_SUPPLY_PROP_VOLTAGE_NOW just reads as 0. 0xda
> > doesn't exist in max77759
> > 
> > I haven't compared in depth yet, though.
> 
> Is the voltage necessary for the driver? If so, we could just not
> read the voltage. If it is necessary, I can try to kook into it and try
> to find in which register it is located (if there is one).

Downstream reports it in
https://android.googlesource.com/kernel/google-modules/bms/+/refs/heads/android-gs-raviole-mainline/max1720x_battery.c#2400

so upstream should do, too.

> > >  static const char *const max17205_model = "MAX17205";
> > > +static const char *const max77759_model = "MAX77759";
> > > 
> > >  struct max1720x_device_info {
> > >       struct regmap *regmap;
> > > @@ -54,6 +57,21 @@ struct max1720x_device_info {
> > >       int rsense;
> > >  };
> > > 
> > > +struct chip_data {
> > > +     u16 default_nrsense; /* in regs in 10^-5 */
> > > +     u8 has_nvmem;
> > > +};
> > > +
> > > +static const struct chip_data max1720x_data  = {
> > > +     .default_nrsense = 1000,
> > > +     .has_nvmem = 1,
> > > +};
> > > +
> > > +static const struct chip_data max77759_data = {
> > > +     .default_nrsense = 500,
> > > +     .has_nvmem = 0,
> > > +};
> > 
> > This should be made a required devicetree property instead, at least for
> > max77759, as it's completely board dependent, 'shunt-resistor-micro-ohms'
> > is widely used.
> > 
> > I also don't think there should be a default. The driver should just fail
> > to probe if not specified in DT (for max77759).
> 
> I hesitated to do this but I didn't know what would be better. Will change
> for v2.

Just to clarify, has_nvmem can stay here in the driver, just rsense should
go into DT is what I mean.

> > > +
> > >  /*
> > >   * Model Gauge M5 Algorithm output register
> > >   * Volatile data (must not be cached)
> > > @@ -369,6 +387,8 @@ static int max1720x_battery_get_property(struct
> > > power_supply *psy,
> > >                       val->strval = max17201_model;
> > >               else if (reg_val == MAX172XX_DEV_NAME_TYPE_MAX17205)
> > >                       val->strval = max17205_model;
> > > +             else if (reg_val == MAX172XX_DEV_NAME_TYPE_MAX77759)
> > > +                     val->strval = max77759_model;
> > >               else
> > 
> > This is a 16 bit register, and while yes, MAX172XX_DEV_NAME_TYPE_MASK only
> > cares about the bottom 4 bits, the register is described as 'Firmware
> > Version Information'.
> > 
> > But maybe it's ok to do it like that, at least for now.
> 
> I thought this method would be ok as long as there is no collision on
> values. I hesitated to change the model evaluation method based on chip
> model, where the max77759 would thus have an hard-coded value and the
> max1720x would still evaluate the register value. I did not do it because
> it led to a lot more changes for no difference.

Downstream uses the upper bits for max77759:
https://android.googlesource.com/kernel/google-modules/bms/+/refs/heads/android-gs-raviole-mainline/max_m5.h#135

I don't know what the original max17201/5 report in this register
for those bits, though. Given for max77759 this register returns
the firmware version, I assume the lower bits can change.

Cheers,
Andre'

Re: [PATCH 2/4] dt-bindings: power: supply: add max77759-fg flavor and don't require nvme address

[Thomas Antoine]

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

Re: [PATCH 1/4] power: supply: add support for max77759 fuel gauge

[André Draszik]

On Tue, 2024-12-03 at 07:23 +0000, André Draszik wrote:
> On Tue, 2024-12-03 at 06:47 +0000, André Draszik wrote:
> > Hi Thomas,
> > 
> > Thanks for looking into this!
> > 
> > > From: Thomas Antoine <t.antoine@uclouvain.be>
> > > 
> > > The Maxim max77759 fuel gauge has the same interface as the Maxim max1720x
> > > except for the non-volatile memory slave address which is not available.
> > 
> > It is not fully compatible, and it also has a lot more registers.
> > 
> > For example, the voltage now is not in register 0xda as this driver assumes.
> > With these changes, POWER_SUPPLY_PROP_VOLTAGE_NOW just reads as 0. 0xda
> > doesn't exist in max77759
> > 
> > I haven't compared in depth yet, though.
> 
> Regarding the regmap in this driver, please see below comparison I had
> collected some time ago:
> 
> 	regmap_reg_range(0x24, 0x26), // exists
> 	regmap_reg_range(0x30, 0x31), // exists
> 	regmap_reg_range(0x33, 0x34), // exists
> 	regmap_reg_range(0x37, 0x37), // exists
> 	regmap_reg_range(0x3B, 0x3C), // exists
> 	regmap_reg_range(0x40, 0x41), // exists
> 	regmap_reg_range(0x43, 0x44), // exists
> 	regmap_reg_range(0x47, 0x49), // exists
> 	regmap_reg_range(0x4B, 0x4C), // exists
> 	regmap_reg_range(0x4E, 0xAF), // 0x4e 0x4f exists
> 	regmap_reg_range(0xB1, 0xB3), // exists
> 	regmap_reg_range(0xB5, 0xB7), // exists
> 	regmap_reg_range(0xBF, 0xD0), // 0xd0 exists
> 	0xd1 .. 0xdb don't exist
> 	regmap_reg_range(0xDB, 0xDB),
> 	regmap_reg_range(0xE0, 0xFF), // 0xfb 0xff exist
> 
> the comments refer to whether or not the register exists in max77759

I think this should make it more clear:
allow:
	regmap_reg_range(0x00, 0xff),
deny:
	regmap_reg_range(0x50, 0xaf),
	regmap_reg_range(0xc0, 0xcf),
	regmap_reg_range(0xd1, 0xdb),
	regmap_reg_range(0xe0, 0xfa),
	regmap_reg_range(0xfc, 0xfe),

Cheers,
Andre'

Re: [PATCH 2/4] dt-bindings: power: supply: add max77759-fg flavor and don't require nvme address

[André Draszik]

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'

Re: [PATCH 1/4] power: supply: add support for max77759 fuel gauge

[Thomas Antoine]

On 12/3/24 10:31, André Draszik wrote:
> On Tue, 2024-12-03 at 10:08 +0100, Thomas Antoine wrote:
>> On 12/3/24 07:47, André Draszik wrote:
>>>> From: Thomas Antoine <t.antoine@uclouvain.be>
>>>>
>>>> The Maxim max77759 fuel gauge has the same interface as the Maxim max1720x
>>>> except for the non-volatile memory slave address which is not available.
>>>
>>> It is not fully compatible, and it also has a lot more registers.
>>>
>>> For example, the voltage now is not in register 0xda as this driver assumes.
>>> With these changes, POWER_SUPPLY_PROP_VOLTAGE_NOW just reads as 0. 0xda
>>> doesn't exist in max77759
>>>
>>> I haven't compared in depth yet, though.
>>
>> Is the voltage necessary for the driver? If so, we could just not
>> read the voltage. If it is necessary, I can try to kook into it and try
>> to find in which register it is located (if there is one).
> 
> Downstream reports it in
> https://android.googlesource.com/kernel/google-modules/bms/+/refs/heads/android-gs-raviole-mainline/max1720x_battery.c#2400
> 
> so upstream should do, too.

I should have checked before answering. Indeed, I will try to see the
best way to choose the register based on the chip. From what I see, it
will also be necessary to change the translation of the reg value to microvolt
as downstream does *78125/1000 when it is currently *1250 in mainline:
https://android.googlesource.com/kernel/google-modules/bms/+/refs/heads/android-gs-raviole-mainline/max1720x_battery.h#49
 
>>>>  static const char *const max17205_model = "MAX17205";
>>>> +static const char *const max77759_model = "MAX77759";
>>>>
>>>>  struct max1720x_device_info {
>>>>       struct regmap *regmap;
>>>> @@ -54,6 +57,21 @@ struct max1720x_device_info {
>>>>       int rsense;
>>>>  };
>>>>
>>>> +struct chip_data {
>>>> +     u16 default_nrsense; /* in regs in 10^-5 */
>>>> +     u8 has_nvmem;
>>>> +};
>>>> +
>>>> +static const struct chip_data max1720x_data  = {
>>>> +     .default_nrsense = 1000,
>>>> +     .has_nvmem = 1,
>>>> +};
>>>> +
>>>> +static const struct chip_data max77759_data = {
>>>> +     .default_nrsense = 500,
>>>> +     .has_nvmem = 0,
>>>> +};
>>>
>>> This should be made a required devicetree property instead, at least for
>>> max77759, as it's completely board dependent, 'shunt-resistor-micro-ohms'
>>> is widely used.
>>>
>>> I also don't think there should be a default. The driver should just fail
>>> to probe if not specified in DT (for max77759).
>>
>> I hesitated to do this but I didn't know what would be better. Will change
>> for v2.
> 
> Just to clarify, has_nvmem can stay here in the driver, just rsense should
> go into DT is what I mean.

It was clear don't worry. This answer is related to the same subject:
https://lore.kernel.org/linux-devicetree/20241202-b4-gs101_max77759_fg-v1-0-98d2fa7bfe30@uclouvain.be/T/#ma55f41d42bf39be826d6cbf8987138bcc4eb52e3

>>>> +
>>>>  /*
>>>>   * Model Gauge M5 Algorithm output register
>>>>   * Volatile data (must not be cached)
>>>> @@ -369,6 +387,8 @@ static int max1720x_battery_get_property(struct
>>>> power_supply *psy,
>>>>                       val->strval = max17201_model;
>>>>               else if (reg_val == MAX172XX_DEV_NAME_TYPE_MAX17205)
>>>>                       val->strval = max17205_model;
>>>> +             else if (reg_val == MAX172XX_DEV_NAME_TYPE_MAX77759)
>>>> +                     val->strval = max77759_model;
>>>>               else
>>>
>>> This is a 16 bit register, and while yes, MAX172XX_DEV_NAME_TYPE_MASK only
>>> cares about the bottom 4 bits, the register is described as 'Firmware
>>> Version Information'.
>>>
>>> But maybe it's ok to do it like that, at least for now.
>>
>> I thought this method would be ok as long as there is no collision on
>> values. I hesitated to change the model evaluation method based on chip
>> model, where the max77759 would thus have an hard-coded value and the
>> max1720x would still evaluate the register value. I did not do it because
>> it led to a lot more changes for no difference.
> 
> Downstream uses the upper bits for max77759:
> https://android.googlesource.com/kernel/google-modules/bms/+/refs/heads/android-gs-raviole-mainline/max_m5.h#135
> 
> I don't know what the original max17201/5 report in this register
> for those bits, though. Given for max77759 this register returns
> the firmware version, I assume the lower bits can change.

Based on this datasheet of the max1720x, the upper bits are the revision
and the four lower bits are device. So it could change.
https://www.analog.com/media/en/technical-documentation/data-sheets/MAX17201-MAX17215.pdf#MAX17201%20DS.indd%3A.213504%3A15892

If the four lower bits are not always 0 for the max77759 then I guess it
is necessary to change this as it wouldn't work with all max77759.


Best regards,
Thomas

Re: [PATCH 2/4] dt-bindings: power: supply: add max77759-fg flavor and don't require nvme address

[Thomas Antoine]

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

Re: [PATCH 1/4] power: supply: add support for max77759 fuel gauge

[Thomas Antoine]

On 12/3/24 10:45, André Draszik wrote:
> On Tue, 2024-12-03 at 07:23 +0000, André Draszik wrote:
>> On Tue, 2024-12-03 at 06:47 +0000, André Draszik wrote:
>>> Hi Thomas,
>>>
>>> Thanks for looking into this!
>>>
>>>> From: Thomas Antoine <t.antoine@uclouvain.be>
>>>>
>>>> The Maxim max77759 fuel gauge has the same interface as the Maxim max1720x
>>>> except for the non-volatile memory slave address which is not available.
>>>
>>> It is not fully compatible, and it also has a lot more registers.
>>>
>>> For example, the voltage now is not in register 0xda as this driver assumes.
>>> With these changes, POWER_SUPPLY_PROP_VOLTAGE_NOW just reads as 0. 0xda
>>> doesn't exist in max77759
>>>
>>> I haven't compared in depth yet, though.
>>
>> Regarding the regmap in this driver, please see below comparison I had
>> collected some time ago:
>>
>> 	regmap_reg_range(0x24, 0x26), // exists
>> 	regmap_reg_range(0x30, 0x31), // exists
>> 	regmap_reg_range(0x33, 0x34), // exists
>> 	regmap_reg_range(0x37, 0x37), // exists
>> 	regmap_reg_range(0x3B, 0x3C), // exists
>> 	regmap_reg_range(0x40, 0x41), // exists
>> 	regmap_reg_range(0x43, 0x44), // exists
>> 	regmap_reg_range(0x47, 0x49), // exists
>> 	regmap_reg_range(0x4B, 0x4C), // exists
>> 	regmap_reg_range(0x4E, 0xAF), // 0x4e 0x4f exists
>> 	regmap_reg_range(0xB1, 0xB3), // exists
>> 	regmap_reg_range(0xB5, 0xB7), // exists
>> 	regmap_reg_range(0xBF, 0xD0), // 0xd0 exists
>> 	0xd1 .. 0xdb don't exist
>> 	regmap_reg_range(0xDB, 0xDB),
>> 	regmap_reg_range(0xE0, 0xFF), // 0xfb 0xff exist
>>
>> the comments refer to whether or not the register exists in max77759
> 
> I think this should make it more clear:
> allow:
> 	regmap_reg_range(0x00, 0xff),
> deny:
> 	regmap_reg_range(0x50, 0xaf),
> 	regmap_reg_range(0xc0, 0xcf),
> 	regmap_reg_range(0xd1, 0xdb),
> 	regmap_reg_range(0xe0, 0xfa),
> 	regmap_reg_range(0xfc, 0xfe),

Should I explicitly deny their use in the code for the max77759 or is it
just for information?

Best regards,
Thomas

Re: [PATCH 2/4] dt-bindings: power: supply: add max77759-fg flavor and don't require nvme address

[André Draszik]

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'

Re: [PATCH 1/4] power: supply: add support for max77759 fuel gauge

[Peter Griffin]

Hi Thomas,

Thanks for your contribution :)

On Tue, 3 Dec 2024 at 10:12, Thomas Antoine <t.antoine@uclouvain.be> wrote:
>
> On 12/3/24 10:31, André Draszik wrote:
> > On Tue, 2024-12-03 at 10:08 +0100, Thomas Antoine wrote:
> >> On 12/3/24 07:47, André Draszik wrote:
> >>>> From: Thomas Antoine <t.antoine@uclouvain.be>
> >>>>
> >>>> The Maxim max77759 fuel gauge has the same interface as the Maxim max1720x
> >>>> except for the non-volatile memory slave address which is not available.
> >>>
> >>> It is not fully compatible, and it also has a lot more registers.
> >>>
> >>> For example, the voltage now is not in register 0xda as this driver assumes.
> >>> With these changes, POWER_SUPPLY_PROP_VOLTAGE_NOW just reads as 0. 0xda
> >>> doesn't exist in max77759
> >>>
> >>> I haven't compared in depth yet, though.
> >>
> >> Is the voltage necessary for the driver? If so, we could just not
> >> read the voltage. If it is necessary, I can try to kook into it and try
> >> to find in which register it is located (if there is one).
> >
> > Downstream reports it in
> > https://android.googlesource.com/kernel/google-modules/bms/+/refs/heads/android-gs-raviole-mainline/max1720x_battery.c#2400
> >
> > so upstream should do, too.
>
> I should have checked before answering. Indeed, I will try to see the
> best way to choose the register based on the chip. From what I see, it
> will also be necessary to change the translation of the reg value to microvolt
> as downstream does *78125/1000 when it is currently *1250 in mainline:
> https://android.googlesource.com/kernel/google-modules/bms/+/refs/heads/android-gs-raviole-mainline/max1720x_battery.h#49
>
> >>>>  static const char *const max17205_model = "MAX17205";
> >>>> +static const char *const max77759_model = "MAX77759";
> >>>>
> >>>>  struct max1720x_device_info {
> >>>>       struct regmap *regmap;
> >>>> @@ -54,6 +57,21 @@ struct max1720x_device_info {
> >>>>       int rsense;
> >>>>  };
> >>>>
> >>>> +struct chip_data {
> >>>> +     u16 default_nrsense; /* in regs in 10^-5 */
> >>>> +     u8 has_nvmem;
> >>>> +};
> >>>> +
> >>>> +static const struct chip_data max1720x_data  = {
> >>>> +     .default_nrsense = 1000,
> >>>> +     .has_nvmem = 1,
> >>>> +};
> >>>> +
> >>>> +static const struct chip_data max77759_data = {
> >>>> +     .default_nrsense = 500,
> >>>> +     .has_nvmem = 0,
> >>>> +};
> >>>
> >>> This should be made a required devicetree property instead, at least for
> >>> max77759, as it's completely board dependent, 'shunt-resistor-micro-ohms'
> >>> is widely used.
> >>>
> >>> I also don't think there should be a default. The driver should just fail
> >>> to probe if not specified in DT (for max77759).
> >>
> >> I hesitated to do this but I didn't know what would be better. Will change
> >> for v2.
> >
> > Just to clarify, has_nvmem can stay here in the driver, just rsense should
> > go into DT is what I mean.
>
> It was clear don't worry. This answer is related to the same subject:
> https://lore.kernel.org/linux-devicetree/20241202-b4-gs101_max77759_fg-v1-0-98d2fa7bfe30@uclouvain.be/T/#ma55f41d42bf39be826d6cbf8987138bcc4eb52e3
>
> >>>> +
> >>>>  /*
> >>>>   * Model Gauge M5 Algorithm output register
> >>>>   * Volatile data (must not be cached)
> >>>> @@ -369,6 +387,8 @@ static int max1720x_battery_get_property(struct
> >>>> power_supply *psy,
> >>>>                       val->strval = max17201_model;
> >>>>               else if (reg_val == MAX172XX_DEV_NAME_TYPE_MAX17205)
> >>>>                       val->strval = max17205_model;
> >>>> +             else if (reg_val == MAX172XX_DEV_NAME_TYPE_MAX77759)
> >>>> +                     val->strval = max77759_model;
> >>>>               else
> >>>
> >>> This is a 16 bit register, and while yes, MAX172XX_DEV_NAME_TYPE_MASK only
> >>> cares about the bottom 4 bits, the register is described as 'Firmware
> >>> Version Information'.
> >>>
> >>> But maybe it's ok to do it like that, at least for now.
> >>
> >> I thought this method would be ok as long as there is no collision on
> >> values. I hesitated to change the model evaluation method based on chip
> >> model, where the max77759 would thus have an hard-coded value and the
> >> max1720x would still evaluate the register value. I did not do it because
> >> it led to a lot more changes for no difference.
> >
> > Downstream uses the upper bits for max77759:
> > https://android.googlesource.com/kernel/google-modules/bms/+/refs/heads/android-gs-raviole-mainline/max_m5.h#135
> >
> > I don't know what the original max17201/5 report in this register
> > for those bits, though. Given for max77759 this register returns
> > the firmware version, I assume the lower bits can change.
>
> Based on this datasheet of the max1720x, the upper bits are the revision
> and the four lower bits are device. So it could change.
> https://www.analog.com/media/en/technical-documentation/data-sheets/MAX17201-MAX17215.pdf#MAX17201%20DS.indd%3A.213504%3A15892
>
> If the four lower bits are not always 0 for the max77759 then I guess it
> is necessary to change this as it wouldn't work with all max77759.

The definition of this register for max77759 is

Register name(addr): DevName (0x21)
Rest value: 0x6200
Bitfield: DevName Bits: 15:0 Description: Firmware Version Information

So I don't think you can rely on the bottom bits always being zero

regards,

Peter

Re: [PATCH 1/4] power: supply: add support for max77759 fuel gauge

[André Draszik]

On Tue, 2024-12-03 at 11:11 +0100, Thomas Antoine wrote:
> On 12/3/24 10:31, André Draszik wrote:
> > On Tue, 2024-12-03 at 10:08 +0100, Thomas Antoine wrote:
> > > On 12/3/24 07:47, André Draszik wrote:
> > > > > From: Thomas Antoine <t.antoine@uclouvain.be>
> > > > > 
[...]

> > > > >  /*
> > > > >   * Model Gauge M5 Algorithm output register
> > > > >   * Volatile data (must not be cached)
> > > > > @@ -369,6 +387,8 @@ static int max1720x_battery_get_property(struct
> > > > > power_supply *psy,
> > > > >                       val->strval = max17201_model;
> > > > >               else if (reg_val == MAX172XX_DEV_NAME_TYPE_MAX17205)
> > > > >                       val->strval = max17205_model;
> > > > > +             else if (reg_val == MAX172XX_DEV_NAME_TYPE_MAX77759)
> > > > > +                     val->strval = max77759_model;
> > > > >               else
> > > > 
> > > > This is a 16 bit register, and while yes, MAX172XX_DEV_NAME_TYPE_MASK only
> > > > cares about the bottom 4 bits, the register is described as 'Firmware
> > > > Version Information'.
> > > > 
> > > > But maybe it's ok to do it like that, at least for now.
> > > 
> > > I thought this method would be ok as long as there is no collision on
> > > values. I hesitated to change the model evaluation method based on chip
> > > model, where the max77759 would thus have an hard-coded value and the
> > > max1720x would still evaluate the register value. I did not do it because
> > > it led to a lot more changes for no difference.
> > 
> > Downstream uses the upper bits for max77759:
> > https://android.googlesource.com/kernel/google-modules/bms/+/refs/heads/android-gs-raviole-mainline/max_m5.h#135
> > 
> > I don't know what the original max17201/5 report in this register
> > for those bits, though. Given for max77759 this register returns
> > the firmware version, I assume the lower bits can change.
> 
> Based on this datasheet of the max1720x, the upper bits are the revision
> and the four lower bits are device. So it could change.
> https://www.analog.com/media/en/technical-documentation/data-sheets/MAX17201-MAX17215.pdf#MAX17201%20DS.indd%3A.213504%3A15892
> 
> If the four lower bits are not always 0 for the max77759 then I guess it
> is necessary to change this as it wouldn't work with all max77759.

Maybe the best way forward is to go by the compatible (from DT), and
if max77759 to then print a warning if the upper bits are != 0x62 and
!= 0x63. And maybe even refuse to load in that case.

Cheers,
Andre'

Re: [PATCH 1/4] power: supply: add support for max77759 fuel gauge

[André Draszik]

On Tue, 2024-12-03 at 11:30 +0100, Thomas Antoine wrote:
> 
> Should I explicitly deny their use in the code for the max77759 or is it
> just for information?

I'd probably do something like this, which will indeed deny their reading
and/or writing, both via debugfs, and also normal driver access via
readmap_read()/write() etc:

static const struct regmap_range max77759_registers[] = {
	regmap_reg_range(0x00, 0x4f),
	regmap_reg_range(0xb0, 0xbf),
	regmap_reg_range(0xd0, 0xd0),
	regmap_reg_range(0xdc, 0xdf),
	regmap_reg_range(0xfb, 0xfb),
	regmap_reg_range(0xff, 0xff),
};

static const struct regmap_range max77759_ro_registers[] = {
	regmap_reg_range(0x3d, 0x3d),
	regmap_reg_range(0xfb, 0xfb),
	regmap_reg_range(0xff, 0xff),
};

static const struct regmap_access_table max77759_write_table = {
	.yes_ranges = max77759_registers,
	.n_yes_ranges = ARRAY_SIZE(max77759_registers),
	.no_ranges = max77759_ro_registers,
	.n_no_ranges = ARRAY_SIZE(max77759_ro_registers),
};

static const struct regmap_access_table max77759_rd_table = {
	.yes_ranges = max77759_registers,
	.n_yes_ranges = ARRAY_SIZE(max77759_registers),
};

static const struct regmap_config max77759_regmap_config = {
	.reg_bits = 8,
	.val_bits = 8,
	.max_register = 0xff,
	.wr_table = &max77759_write_table,
	.rd_table = &max77759_rd_table,
	.cache_type = REGCACHE_NONE,
};

And maybe without cache for now. Most are probably not cacheable anyway.

Cheers,
Andre'

Re: [PATCH 1/4] power: supply: add support for max77759 fuel gauge

[Thomas Antoine]

On 12/3/24 12:06, André Draszik wrote:
> On Tue, 2024-12-03 at 11:30 +0100, Thomas Antoine wrote:
>> Should I explicitly deny their use in the code for the max77759 or is it
>> just for information?
> I'd probably do something like this, which will indeed deny their reading
> and/or writing, both via debugfs, and also normal driver access via
> readmap_read()/write() etc:
> 
> static const struct regmap_range max77759_registers[] = {
> 	regmap_reg_range(0x00, 0x4f),
> 	regmap_reg_range(0xb0, 0xbf),
> 	regmap_reg_range(0xd0, 0xd0),
> 	regmap_reg_range(0xdc, 0xdf),
> 	regmap_reg_range(0xfb, 0xfb),
> 	regmap_reg_range(0xff, 0xff),
> };
> 
> static const struct regmap_range max77759_ro_registers[] = {
> 	regmap_reg_range(0x3d, 0x3d),
> 	regmap_reg_range(0xfb, 0xfb),
> 	regmap_reg_range(0xff, 0xff),
> };
> 
> static const struct regmap_access_table max77759_write_table = {
> 	.yes_ranges = max77759_registers,
> 	.n_yes_ranges = ARRAY_SIZE(max77759_registers),
> 	.no_ranges = max77759_ro_registers,
> 	.n_no_ranges = ARRAY_SIZE(max77759_ro_registers),
> };
> 
> static const struct regmap_access_table max77759_rd_table = {
> 	.yes_ranges = max77759_registers,
> 	.n_yes_ranges = ARRAY_SIZE(max77759_registers),
> };
> 
> static const struct regmap_config max77759_regmap_config = {
> 	.reg_bits = 8,
> 	.val_bits = 8,
> 	.max_register = 0xff,
> 	.wr_table = &max77759_write_table,
> 	.rd_table = &max77759_rd_table,
> 	.cache_type = REGCACHE_NONE,
> };
> 
> And maybe without cache for now. Most are probably not cacheable anyway.

Thank you very much, I'll try this out.

Best regards,
Thomas

Re: [PATCH 1/4] power: supply: add support for max77759 fuel gauge

[Thomas Antoine]

On 12/3/24 12:02, André Draszik wrote:
> On Tue, 2024-12-03 at 11:11 +0100, Thomas Antoine wrote:
>> On 12/3/24 10:31, André Draszik wrote:
>>> On Tue, 2024-12-03 at 10:08 +0100, Thomas Antoine wrote:
>>>> On 12/3/24 07:47, André Draszik wrote:
>>>>>> From: Thomas Antoine <t.antoine@uclouvain.be>
>>>>>>
> [...]
> 
>>>>>>  /*
>>>>>>   * Model Gauge M5 Algorithm output register
>>>>>>   * Volatile data (must not be cached)
>>>>>> @@ -369,6 +387,8 @@ static int max1720x_battery_get_property(struct
>>>>>> power_supply *psy,
>>>>>>                       val->strval = max17201_model;
>>>>>>               else if (reg_val == MAX172XX_DEV_NAME_TYPE_MAX17205)
>>>>>>                       val->strval = max17205_model;
>>>>>> +             else if (reg_val == MAX172XX_DEV_NAME_TYPE_MAX77759)
>>>>>> +                     val->strval = max77759_model;
>>>>>>               else
>>>>>
>>>>> This is a 16 bit register, and while yes, MAX172XX_DEV_NAME_TYPE_MASK only
>>>>> cares about the bottom 4 bits, the register is described as 'Firmware
>>>>> Version Information'.
>>>>>
>>>>> But maybe it's ok to do it like that, at least for now.
>>>>
>>>> I thought this method would be ok as long as there is no collision on
>>>> values. I hesitated to change the model evaluation method based on chip
>>>> model, where the max77759 would thus have an hard-coded value and the
>>>> max1720x would still evaluate the register value. I did not do it because
>>>> it led to a lot more changes for no difference.
>>>
>>> Downstream uses the upper bits for max77759:
>>> https://android.googlesource.com/kernel/google-modules/bms/+/refs/heads/android-gs-raviole-mainline/max_m5.h#135
>>>
>>> I don't know what the original max17201/5 report in this register
>>> for those bits, though. Given for max77759 this register returns
>>> the firmware version, I assume the lower bits can change.
>>
>> Based on this datasheet of the max1720x, the upper bits are the revision
>> and the four lower bits are device. So it could change.
>> https://www.analog.com/media/en/technical-documentation/data-sheets/MAX17201-MAX17215.pdf#MAX17201%20DS.indd%3A.213504%3A15892
>>
>> If the four lower bits are not always 0 for the max77759 then I guess it
>> is necessary to change this as it wouldn't work with all max77759.
> 
> Maybe the best way forward is to go by the compatible (from DT), and
> if max77759 to then print a warning if the upper bits are != 0x62 and
> != 0x63. And maybe even refuse to load in that case.

Will implement this for v2, thank you.

Best regards,
Thomas

Re: [PATCH 2/4] dt-bindings: power: supply: add max77759-fg flavor and don't require nvme address

[Thomas Antoine]

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

Re: [PATCH 2/4] dt-bindings: power: supply: add max77759-fg flavor and don't require nvme address

[André Draszik]

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'