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

[PATCH v2 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
an interface with a lot in common with the Maxim max1720x.

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>
---
Changes in v2:
- Add fallback for voltage measurement (André Draszik)
- Add regmap for the max77759 (André Draszik)
- Add chip identification for the max77759 (André Draszik, Peter Griffin)
- Move RSense value to a devicetree property shunt-resistor-micro-ohms
  (Dimitri Fedrau, André Draszik)
- Use allOf:if to narrow binding per variant (Krzysztof Kozlowski)
- Remove binding example (Krzysztof Kozlowski)
- Change defconfig order to follow savedefconfig (Krzysztof Kozlowski)
- Fix style errors
- Link to v1: https://lore.kernel.org/r/20241202-b4-gs101_max77759_fg-v1-0-98d2fa7bfe30@uclouvain.be

---
Thomas Antoine (4):
      power: supply: add support for max77759 fuel gauge
      dt-bindings: power: supply: add max77759-fg flavor
      arm64: defconfig: enable Maxim max1720x driver
      arm64: dts: exynos: gs101-oriole: enable Maxim max77759 fuel gauge

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

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

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

[Thomas Antoine via B4 Relay]

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

The interface of the Maxim max77759 fuel gauge has a lot of common with the
Maxim max1720x. The major difference is the lack of non-volatile memory
slave address. 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

Other differences include the lack of V_BATT register to read the battery
level and a difference in the way to identify the chip (the same register
is used but not the same mask).

Add support for the max77759 by allowing to use the non-volatile
memory or not based on the chip. Also add the V_CELL regsister as a
fallback to read voltage value in the case where read of V_BATT fails.

The cast is necessary to avoid an overflow when the value of the register
is above 54975 (equivalent to a voltage around 4.29 V).

The regmap of the max77759 will lead the read to fail for V_BATT and to
correctly use V_CELL instead. This regmap was proposed by André Draszik in

Link: https://lore.kernel.org/all/d1bade77b5281c1de6b2ddcb4dbbd033e455a116.camel@linaro.org/

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

diff --git a/drivers/power/supply/max1720x_battery.c b/drivers/power/supply/max1720x_battery.c
index 33105419e2427bb37963bda9948b647c239f8faa..a41976679eded44fbd13120ad756a626d2867919 100644
--- a/drivers/power/supply/max1720x_battery.c
+++ b/drivers/power/supply/max1720x_battery.c
@@ -27,6 +27,7 @@
 #define MAX172XX_REPCAP			0x05	/* Average capacity */
 #define MAX172XX_REPSOC			0x06	/* Percentage of charge */
 #define MAX172XX_TEMP			0x08	/* Temperature */
+#define MAX172XX_VCELL			0x09	/* Lowest cell voltage */
 #define MAX172XX_CURRENT		0x0A	/* Actual current */
 #define MAX172XX_AVG_CURRENT		0x0B	/* Average current */
 #define MAX172XX_TTE			0x11	/* Time to empty */
@@ -39,6 +40,8 @@
 #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 MAX77759_DEV_NAME_TYPE_MASK	GENMASK(15, 9)
+#define MAX77759_DEV_NAME_TYPE_MAX77759	0x31
 #define MAX172XX_QR_TABLE10		0x22
 #define MAX172XX_BATT			0xDA	/* Battery voltage */
 #define MAX172XX_ATAVCAP		0xDF
@@ -46,12 +49,20 @@
 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";
+
+enum chip_id {
+	MAX1720X_ID,
+	MAX77759_ID,
+};
 
 struct max1720x_device_info {
 	struct regmap *regmap;
 	struct regmap *regmap_nv;
 	struct i2c_client *ancillary;
 	int rsense;
+	int has_nvmem;
+	enum chip_id id;
 };
 
 /*
@@ -254,6 +265,67 @@ static const enum power_supply_property max1720x_battery_props[] = {
 	POWER_SUPPLY_PROP_MANUFACTURER,
 };
 
+/*
+ * Registers 0x80 up to 0xaf which contain the model for the fuel gauge
+ * algorithm (stored in nvmem for the max1720x) are locked. They can
+ * be unlocked by writing 0x59 to 0x62 and 0xc4 to 0x63. They should be
+ * enabled in the regmap if the driver is extended to manage the model.
+ */
+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_cfg = {
+	.reg_bits = 8,
+	.val_bits = 16,
+	.max_register = 0xff,
+	.wr_table = &max77759_write_table,
+	.rd_table = &max77759_rd_table,
+	.val_format_endian = REGMAP_ENDIAN_LITTLE,
+	.cache_type = REGCACHE_NONE,
+};
+
+struct chip_data {
+	u8 has_nvmem;
+	const struct regmap_config *regmap_cfg;
+	enum chip_id id;
+};
+
+static const struct chip_data max1720x_data  = {
+	.has_nvmem = 1,
+	.regmap_cfg = &max1720x_regmap_cfg,
+	.id = MAX1720X_ID,
+};
+
+static const struct chip_data max77759_data = {
+	.has_nvmem = 0,
+	.regmap_cfg = &max77759_regmap_cfg,
+	.id = MAX77759_ID,
+};
+
 /* Convert regs value to power_supply units */
 
 static int max172xx_time_to_ps(unsigned int reg)
@@ -271,6 +343,14 @@ static int max172xx_voltage_to_ps(unsigned int reg)
 	return reg * 1250;	/* in uV */
 }
 
+// Use 64 bits because computation on 32 bits leads to an overflow
+static int max172xx_cell_voltage_to_ps(unsigned int reg)
+{
+	u64 val = reg;
+
+	return val * 78125 / 1000;	/* in uV */
+}
+
 static int max172xx_capacity_to_ps(unsigned int reg)
 {
 	return reg * 500;	/* in uAh */
@@ -303,6 +383,33 @@ static int max172xx_current_to_voltage(unsigned int reg)
 	return val * 156252;
 }
 
+static int max1720x_devname_to_model(unsigned int reg_val,
+					union power_supply_propval *val,
+					struct max1720x_device_info *info)
+{
+	switch (info->id) {
+	case MAX1720X_ID:
+		reg_val = FIELD_GET(MAX172XX_DEV_NAME_TYPE_MASK, reg_val);
+		if (reg_val == MAX172XX_DEV_NAME_TYPE_MAX17201)
+			val->strval = max17201_model;
+		else if (reg_val == MAX172XX_DEV_NAME_TYPE_MAX17205)
+			val->strval = max17205_model;
+		else
+			return -ENODEV;
+		break;
+	case MAX77759_ID:
+		reg_val = FIELD_GET(MAX77759_DEV_NAME_TYPE_MASK, reg_val);
+		if (reg_val == MAX77759_DEV_NAME_TYPE_MAX77759)
+			val->strval = max77759_model;
+		else
+			return -ENODEV;
+		break;
+	default:
+		return -ENODEV;
+	}
+	return 0;
+}
+
 static int max1720x_battery_get_property(struct power_supply *psy,
 					 enum power_supply_property psp,
 					 union power_supply_propval *val)
@@ -332,7 +439,12 @@ static int max1720x_battery_get_property(struct power_supply *psy,
 		break;
 	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
 		ret = regmap_read(info->regmap, MAX172XX_BATT, &reg_val);
-		val->intval = max172xx_voltage_to_ps(reg_val);
+		if (!ret)
+			val->intval = max172xx_voltage_to_ps(reg_val);
+		else {
+			ret = regmap_read(info->regmap, MAX172XX_VCELL, &reg_val);
+			val->intval = max172xx_cell_voltage_to_ps(reg_val);
+		}
 		break;
 	case POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN:
 		ret = regmap_read(info->regmap, MAX172XX_DESIGN_CAP, &reg_val);
@@ -364,13 +476,8 @@ static int max1720x_battery_get_property(struct power_supply *psy,
 		break;
 	case POWER_SUPPLY_PROP_MODEL_NAME:
 		ret = regmap_read(info->regmap, MAX172XX_DEV_NAME, &reg_val);
-		reg_val = FIELD_GET(MAX172XX_DEV_NAME_TYPE_MASK, reg_val);
-		if (reg_val == MAX172XX_DEV_NAME_TYPE_MAX17201)
-			val->strval = max17201_model;
-		else if (reg_val == MAX172XX_DEV_NAME_TYPE_MAX17205)
-			val->strval = max17205_model;
-		else
-			return -ENODEV;
+		if (!ret)
+			ret = max1720x_devname_to_model(reg_val, val, info);
 		break;
 	case POWER_SUPPLY_PROP_MANUFACTURER:
 		val->strval = max1720x_manufacturer;
@@ -416,7 +523,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,18 +544,6 @@ static int max1720x_probe_nvmem(struct i2c_client *client,
 		return PTR_ERR(info->regmap_nv);
 	}
 
-	ret = regmap_read(info->regmap_nv, MAX1720X_NRSENSE, &val);
-	if (ret < 0) {
-		dev_err(dev, "Failed to read sense resistor value\n");
-		return ret;
-	}
-
-	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!");
@@ -459,6 +553,36 @@ static int max1720x_probe_nvmem(struct i2c_client *client,
 	return 0;
 }
 
+static int max1720x_get_rsense(struct device *dev,
+					 struct max1720x_device_info *info)
+{
+	unsigned int val;
+	int ret;
+
+	if (info->has_nvmem) {
+		ret = regmap_read(info->regmap_nv, MAX1720X_NRSENSE, &val);
+		if (ret < 0) {
+			dev_err(dev, "Failed to read RSense from nvmem\n");
+			return ret;
+		}
+
+		info->rsense = val;
+		if (!info->rsense) {
+			dev_warn(dev, "RSense not calibrated, set 10 mOhms!\n");
+			info->rsense = 1000; /* in regs in 10^-5 */
+		}
+	} else {
+		ret = of_property_read_u32(dev->of_node,
+					"shunt-resistor-micro-ohms", &val);
+		if (ret) {
+			dev_err(dev, "Failed to read RSense from devicetree\n");
+			return ret;
+		}
+		info->rsense = val/10;
+	}
+	return 0;
+}
+
 static const struct power_supply_desc max1720x_bat_desc = {
 	.name = "max1720x",
 	.no_thermal = true,
@@ -474,6 +598,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);
@@ -483,14 +608,27 @@ static int max1720x_probe(struct i2c_client *client)
 	psy_cfg.drv_data = info;
 	psy_cfg.fwnode = dev_fwnode(dev);
 	i2c_set_clientdata(client, info);
-	info->regmap = devm_regmap_init_i2c(client, &max1720x_regmap_cfg);
+
+	data = device_get_match_data(dev);
+	if (!data)
+		return dev_err_probe(dev, ret, "Failed to get chip data\n");
+
+	info->has_nvmem = data->has_nvmem;
+	info->id = data->id;
+	info->regmap = devm_regmap_init_i2c(client, data->regmap_cfg);
 	if (IS_ERR(info->regmap))
 		return dev_err_probe(dev, PTR_ERR(info->regmap),
 				     "regmap initialization failed\n");
 
-	ret = max1720x_probe_nvmem(client, info);
+	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);
 	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 +639,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 v2 2/4] dt-bindings: power: supply: add max77759-fg flavor

[Thomas Antoine via B4 Relay]

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

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 TCPCI and a charger.

Use max77759-fg compatible to avoid conflict with drivers for other
functions.

The max77759 has no non-volatile memory so it doesn't require an address
and instead requires a value for the current sensing resistor.

Signed-off-by: Thomas Antoine <t.antoine@uclouvain.be>
---
 .../bindings/power/supply/maxim,max17201.yaml      | 56 +++++++++++++++++-----
 1 file changed, 43 insertions(+), 13 deletions(-)

diff --git a/Documentation/devicetree/bindings/power/supply/maxim,max17201.yaml b/Documentation/devicetree/bindings/power/supply/maxim,max17201.yaml
index fe3dd9bd5585618e45220c51023391a5b21acfd2..7e95314508c27d0d90ea92f61bca6b4a2fe0e88e 100644
--- a/Documentation/devicetree/bindings/power/supply/maxim,max17201.yaml
+++ b/Documentation/devicetree/bindings/power/supply/maxim,max17201.yaml
@@ -9,31 +9,61 @@ title: Maxim MAX17201 fuel gauge
 maintainers:
   - Dimitri Fedrau <dima.fedrau@gmail.com>
 
-allOf:
-  - $ref: power-supply.yaml#
-
 properties:
   compatible:
     oneOf:
       - const: maxim,max17201
+      - const: maxim,max77759-fg
       - items:
           - enum:
               - maxim,max17205
           - const: maxim,max17201
 
-  reg:
-    items:
-      - description: ModelGauge m5 registers
-      - description: Nonvolatile registers
-
-  reg-names:
-    items:
-      - const: m5
-      - const: nvmem
-
   interrupts:
     maxItems: 1
 
+allOf:
+  - $ref: power-supply.yaml#
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - maxim,max17201
+    then:
+      properties:
+        reg:
+          items:
+            - description: ModelGauge m5 registers
+            - description: Nonvolatile registers
+          minItems: 1
+
+        reg-names:
+          items:
+            - const: m5
+            - const: nvmem
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - maxim,max77759-fg
+    then:
+      properties:
+        reg:
+          items:
+            - description: ModelGauge m5 registers
+
+        reg-names:
+          items:
+            - const: m5
+
+        shunt-resistor-micro-ohms:
+          description: The value of current sense resistor.
+
+      required:
+        - shunt-resistor-micro-ohms
+
 required:
   - compatible
   - reg

-- 
2.47.1

[PATCH v2 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..7161387d1ebfea0d363e9413ef4350d83b581d96 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -676,6 +676,7 @@ CONFIG_BATTERY_QCOM_BATTMGR=m
 CONFIG_BATTERY_SBS=m
 CONFIG_BATTERY_BQ27XXX=y
 CONFIG_BATTERY_MAX17042=m
+CONFIG_BATTERY_MAX1720X=m
 CONFIG_CHARGER_MT6360=m
 CONFIG_CHARGER_BQ25890=m
 CONFIG_CHARGER_BQ25980=m

-- 
2.47.1

[PATCH v2 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 todo is still applicable given there are other slaves on the
bus (pca9468, other max77759 functions and the max20339 OVP).

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 | 10 ++++++++++
 1 file changed, 10 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..6c83ee6f8a6b0327c576573d03a8d2bcc93f9e16 100644
--- a/arch/arm64/boot/dts/exynos/google/gs101-oriole.dts
+++ b/arch/arm64/boot/dts/exynos/google/gs101-oriole.dts
@@ -10,6 +10,7 @@
 
 #include <dt-bindings/gpio/gpio.h>
 #include <dt-bindings/input/input.h>
+#include <dt-bindings/interrupt-controller/irq.h>
 #include "gs101-pinctrl.h"
 #include "gs101.dtsi"
 
@@ -90,6 +91,15 @@ 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";
+		shunt-resistor-micro-ohms = <5000>;
+		interrupt-parent = <&gpa9>;
+		interrupts = <3 IRQ_TYPE_LEVEL_LOW>;
+	};
 };
 
 &pinctrl_far_alive {

-- 
2.47.1

Re: [PATCH v2 2/4] dt-bindings: power: supply: add max77759-fg flavor

[Krzysztof Kozlowski]

On 02/01/2025 12:15, Thomas Antoine via B4 Relay wrote:
> From: Thomas Antoine <t.antoine@uclouvain.be>
> 
> The max77759 is an IC used to manage the power supply of the battery and

Still not the name I asked to use.

> the USB-C. Based on drivers from google, it contains at least a PMIC, a
> fuel gauge, a TCPCI and a charger.
> 
> Use max77759-fg compatible to avoid conflict with drivers for other
> functions.
> 
> The max77759 has no non-volatile memory so it doesn't require an address
> and instead requires a value for the current sensing resistor.
> 
> Signed-off-by: Thomas Antoine <t.antoine@uclouvain.be>
> ---
>  .../bindings/power/supply/maxim,max17201.yaml      | 56 +++++++++++++++++-----
>  1 file changed, 43 insertions(+), 13 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/power/supply/maxim,max17201.yaml b/Documentation/devicetree/bindings/power/supply/maxim,max17201.yaml
> index fe3dd9bd5585618e45220c51023391a5b21acfd2..7e95314508c27d0d90ea92f61bca6b4a2fe0e88e 100644
> --- a/Documentation/devicetree/bindings/power/supply/maxim,max17201.yaml
> +++ b/Documentation/devicetree/bindings/power/supply/maxim,max17201.yaml
> @@ -9,31 +9,61 @@ title: Maxim MAX17201 fuel gauge
>  maintainers:
>    - Dimitri Fedrau <dima.fedrau@gmail.com>
>  
> -allOf:
> -  - $ref: power-supply.yaml#
> -
>  properties:
>    compatible:
>      oneOf:
>        - const: maxim,max17201
> +      - const: maxim,max77759-fg
>        - items:
>            - enum:
>                - maxim,max17205
>            - const: maxim,max17201
>  
> -  reg:
> -    items:
> -      - description: ModelGauge m5 registers
> -      - description: Nonvolatile registers

Widest constraints always stay here.

See:
https://elixir.bootlin.com/linux/v6.11-rc6/source/Documentation/devicetree/bindings/ufs/qcom,ufs.yaml#L127

I did not say to remove it. I asked you to add allOf section restricting it.

> -
> -  reg-names:
> -    items:
> -      - const: m5
> -      - const: nvmem
> -
>    interrupts:
>      maxItems: 1
>  
> +allOf:

This goes after required: block. See example-schema.

> +  - $ref: power-supply.yaml#
> +  - if:
Best regards,
Krzysztof

Re: [PATCH v2 2/4] dt-bindings: power: supply: add max77759-fg flavor

[Thomas Antoine]

On 1/2/25 17:08, Krzysztof Kozlowski wrote:
> On 02/01/2025 12:15, Thomas Antoine via B4 Relay wrote:
>> From: Thomas Antoine <t.antoine@uclouvain.be>
>>
>> The max77759 is an IC used to manage the power supply of the battery and
> 
> Still not the name I asked to use.

Indeed, I missed that, I will fix this.

[...]
  
>> -allOf:
>> -  - $ref: power-supply.yaml#
>> -
>>  properties:
>>    compatible:
>>      oneOf:
>>        - const: maxim,max17201
>> +      - const: maxim,max77759-fg
>>        - items:
>>            - enum:
>>                - maxim,max17205
>>            - const: maxim,max17201
>>  
>> -  reg:
>> -    items:
>> -      - description: ModelGauge m5 registers
>> -      - description: Nonvolatile registers
> 
> Widest constraints always stay here.
> 
> See:
> https://elixir.bootlin.com/linux/v6.11-rc6/source/Documentation/devicetree/bindings/ufs/qcom,ufs.yaml#L127
> 
> I did not say to remove it. I asked you to add allOf section restricting it.

Thanks for the example. I think I understand now. I will put the reg section
back and use min/maxItems in the allOf:if: to set the number of reg/reg-names
to 1 for the MAX77759.

Do I keep shunt-resistor-micro-ohms as I did it here? I could move it in
properties: and only make it required by the max77759 in the allOf:if:. However, 
I think this would make it seem as if the MAX17201 optionally accepts it when
it would do nothing in practice so I'm not sure what is the best choice.

>> -
>> -  reg-names:
>> -    items:
>> -      - const: m5
>> -      - const: nvmem
>> -
>>    interrupts:
>>      maxItems: 1
>>  
>> +allOf:
> 
> This goes after required: block. See example-schema.
> 
>> +  - $ref: power-supply.yaml#
>> +  - if:
> Best regards,
> Krzysztof

Will fix.

Best regards,
Thomas

Re: [PATCH v2 2/4] dt-bindings: power: supply: add max77759-fg flavor

[Krzysztof Kozlowski]

On 03/01/2025 17:16, Thomas Antoine wrote:
>>> -  reg:
>>> -    items:
>>> -      - description: ModelGauge m5 registers
>>> -      - description: Nonvolatile registers
>>
>> Widest constraints always stay here.
>>
>> See:
>> https://elixir.bootlin.com/linux/v6.11-rc6/source/Documentation/devicetree/bindings/ufs/qcom,ufs.yaml#L127
>>
>> I did not say to remove it. I asked you to add allOf section restricting it.
> 
> Thanks for the example. I think I understand now. I will put the reg section
> back and use min/maxItems in the allOf:if: to set the number of reg/reg-names
> to 1 for the MAX77759.
> 
> Do I keep shunt-resistor-micro-ohms as I did it here? I could move it in


Depends, where does it come from? What does the other referenced schema say?


Best regards,
Krzysztof

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

[Dimitri Fedrau]

Hi Thomas,

Am Thu, Jan 02, 2025 at 12:15:03PM +0100 schrieb Thomas Antoine via B4 Relay:
> From: Thomas Antoine <t.antoine@uclouvain.be>
> 
> The interface of the Maxim max77759 fuel gauge has a lot of common with the
> Maxim max1720x. The major difference is the lack of non-volatile memory
> slave address. 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
> 
> Other differences include the lack of V_BATT register to read the battery
> level and a difference in the way to identify the chip (the same register
> is used but not the same mask).
> 
> Add support for the max77759 by allowing to use the non-volatile
> memory or not based on the chip. Also add the V_CELL regsister as a
> fallback to read voltage value in the case where read of V_BATT fails.
> 
> The cast is necessary to avoid an overflow when the value of the register
> is above 54975 (equivalent to a voltage around 4.29 V).
> 
> The regmap of the max77759 will lead the read to fail for V_BATT and to
> correctly use V_CELL instead. This regmap was proposed by André Draszik in
> 
> Link: https://lore.kernel.org/all/d1bade77b5281c1de6b2ddcb4dbbd033e455a116.camel@linaro.org/
> 
> Signed-off-by: Thomas Antoine <t.antoine@uclouvain.be>
> ---
>  drivers/power/supply/max1720x_battery.c | 189 +++++++++++++++++++++++++++-----
>  1 file changed, 164 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/power/supply/max1720x_battery.c b/drivers/power/supply/max1720x_battery.c
> index 33105419e2427bb37963bda9948b647c239f8faa..a41976679eded44fbd13120ad756a626d2867919 100644
> --- a/drivers/power/supply/max1720x_battery.c
> +++ b/drivers/power/supply/max1720x_battery.c
> @@ -27,6 +27,7 @@
>  #define MAX172XX_REPCAP			0x05	/* Average capacity */
>  #define MAX172XX_REPSOC			0x06	/* Percentage of charge */
>  #define MAX172XX_TEMP			0x08	/* Temperature */
> +#define MAX172XX_VCELL			0x09	/* Lowest cell voltage */
>  #define MAX172XX_CURRENT		0x0A	/* Actual current */
>  #define MAX172XX_AVG_CURRENT		0x0B	/* Average current */
>  #define MAX172XX_TTE			0x11	/* Time to empty */
> @@ -39,6 +40,8 @@
>  #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 MAX77759_DEV_NAME_TYPE_MASK	GENMASK(15, 9)
> +#define MAX77759_DEV_NAME_TYPE_MAX77759	0x31
>  #define MAX172XX_QR_TABLE10		0x22
>  #define MAX172XX_BATT			0xDA	/* Battery voltage */
>  #define MAX172XX_ATAVCAP		0xDF
> @@ -46,12 +49,20 @@
>  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";
> +
> +enum chip_id {
> +	MAX1720X_ID,
> +	MAX77759_ID,
> +};
>  
>  struct max1720x_device_info {
>  	struct regmap *regmap;
>  	struct regmap *regmap_nv;
>  	struct i2c_client *ancillary;
>  	int rsense;
> +	int has_nvmem;

Switch to bool and why is needed twice ? Here and in chip_data. Better
keep it in chip_data.

> +	enum chip_id id;
>  };
>  
>  /*
> @@ -254,6 +265,67 @@ static const enum power_supply_property max1720x_battery_props[] = {
>  	POWER_SUPPLY_PROP_MANUFACTURER,
>  };
>  
> +/*
> + * Registers 0x80 up to 0xaf which contain the model for the fuel gauge
> + * algorithm (stored in nvmem for the max1720x) are locked. They can
> + * be unlocked by writing 0x59 to 0x62 and 0xc4 to 0x63. They should be
> + * enabled in the regmap if the driver is extended to manage the model.
> + */
> +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_cfg = {
> +	.reg_bits = 8,
> +	.val_bits = 16,
> +	.max_register = 0xff,
> +	.wr_table = &max77759_write_table,
> +	.rd_table = &max77759_rd_table,
> +	.val_format_endian = REGMAP_ENDIAN_LITTLE,
> +	.cache_type = REGCACHE_NONE,
> +};
> +
> +struct chip_data {
> +	u8 has_nvmem;
> +	const struct regmap_config *regmap_cfg;
> +	enum chip_id id;
> +};
> +
> +static const struct chip_data max1720x_data  = {
> +	.has_nvmem = 1,
> +	.regmap_cfg = &max1720x_regmap_cfg,
> +	.id = MAX1720X_ID,
> +};
> +
> +static const struct chip_data max77759_data = {
> +	.has_nvmem = 0,
> +	.regmap_cfg = &max77759_regmap_cfg,
> +	.id = MAX77759_ID,
> +};
> +
>  /* Convert regs value to power_supply units */
>  
>  static int max172xx_time_to_ps(unsigned int reg)
> @@ -271,6 +343,14 @@ static int max172xx_voltage_to_ps(unsigned int reg)
>  	return reg * 1250;	/* in uV */
>  }
>  
> +// Use 64 bits because computation on 32 bits leads to an overflow
> +static int max172xx_cell_voltage_to_ps(unsigned int reg)
> +{
> +	u64 val = reg;
> +
> +	return val * 78125 / 1000;	/* in uV */

You could avoid the overflow with:
	return val * 625 / 8;	/* in uV */
> +}
> +
>  static int max172xx_capacity_to_ps(unsigned int reg)
>  {
>  	return reg * 500;	/* in uAh */
> @@ -303,6 +383,33 @@ static int max172xx_current_to_voltage(unsigned int reg)
>  	return val * 156252;
>  }
>  
> +static int max1720x_devname_to_model(unsigned int reg_val,
> +					union power_supply_propval *val,
> +					struct max1720x_device_info *info)

nit: Align with open parenthesis

> +{
> +	switch (info->id) {
> +	case MAX1720X_ID:
> +		reg_val = FIELD_GET(MAX172XX_DEV_NAME_TYPE_MASK, reg_val);
> +		if (reg_val == MAX172XX_DEV_NAME_TYPE_MAX17201)
> +			val->strval = max17201_model;
> +		else if (reg_val == MAX172XX_DEV_NAME_TYPE_MAX17205)
> +			val->strval = max17205_model;
> +		else
> +			return -ENODEV;
> +		break;
> +	case MAX77759_ID:
> +		reg_val = FIELD_GET(MAX77759_DEV_NAME_TYPE_MASK, reg_val);
> +		if (reg_val == MAX77759_DEV_NAME_TYPE_MAX77759)
> +			val->strval = max77759_model;
> +		else
> +			return -ENODEV;
> +		break;
> +	default:
> +		return -ENODEV;
> +	}
> +	return 0;
> +}
> +
>  static int max1720x_battery_get_property(struct power_supply *psy,
>  					 enum power_supply_property psp,
>  					 union power_supply_propval *val)
> @@ -332,7 +439,12 @@ static int max1720x_battery_get_property(struct power_supply *psy,
>  		break;
>  	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
>  		ret = regmap_read(info->regmap, MAX172XX_BATT, &reg_val);
> -		val->intval = max172xx_voltage_to_ps(reg_val);
> +		if (!ret)
> +			val->intval = max172xx_voltage_to_ps(reg_val);
> +		else {
> +			ret = regmap_read(info->regmap, MAX172XX_VCELL, &reg_val);
> +			val->intval = max172xx_cell_voltage_to_ps(reg_val);
> +		}

Would be better to read the right register, since we know which device
we are. You could differentiate like in max1720x_devname_to_model.

>  		break;
>  	case POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN:
>  		ret = regmap_read(info->regmap, MAX172XX_DESIGN_CAP, &reg_val);
> @@ -364,13 +476,8 @@ static int max1720x_battery_get_property(struct power_supply *psy,
>  		break;
>  	case POWER_SUPPLY_PROP_MODEL_NAME:
>  		ret = regmap_read(info->regmap, MAX172XX_DEV_NAME, &reg_val);
> -		reg_val = FIELD_GET(MAX172XX_DEV_NAME_TYPE_MASK, reg_val);
> -		if (reg_val == MAX172XX_DEV_NAME_TYPE_MAX17201)
> -			val->strval = max17201_model;
> -		else if (reg_val == MAX172XX_DEV_NAME_TYPE_MAX17205)
> -			val->strval = max17205_model;
> -		else
> -			return -ENODEV;
> +		if (!ret)
> +			ret = max1720x_devname_to_model(reg_val, val, info);
>  		break;
>  	case POWER_SUPPLY_PROP_MANUFACTURER:
>  		val->strval = max1720x_manufacturer;
> @@ -416,7 +523,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,18 +544,6 @@ static int max1720x_probe_nvmem(struct i2c_client *client,
>  		return PTR_ERR(info->regmap_nv);
>  	}
>  
> -	ret = regmap_read(info->regmap_nv, MAX1720X_NRSENSE, &val);
> -	if (ret < 0) {
> -		dev_err(dev, "Failed to read sense resistor value\n");
> -		return ret;
> -	}
> -
> -	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!");
> @@ -459,6 +553,36 @@ static int max1720x_probe_nvmem(struct i2c_client *client,
>  	return 0;
>  }
>  
> +static int max1720x_get_rsense(struct device *dev,
> +					 struct max1720x_device_info *info)

nit: Align with open parenthesis.

> +{
> +	unsigned int val;
> +	int ret;
> +
> +	if (info->has_nvmem) {
> +		ret = regmap_read(info->regmap_nv, MAX1720X_NRSENSE, &val);
> +		if (ret < 0) {
> +			dev_err(dev, "Failed to read RSense from nvmem\n");
> +			return ret;
> +		}
> +
> +		info->rsense = val;
> +		if (!info->rsense) {
> +			dev_warn(dev, "RSense not calibrated, set 10 mOhms!\n");
> +			info->rsense = 1000; /* in regs in 10^-5 */
> +		}
> +	} else {
> +		ret = of_property_read_u32(dev->of_node,
> +					"shunt-resistor-micro-ohms", &val);

nit: Align with open parenthesis.

> +		if (ret) {
> +			dev_err(dev, "Failed to read RSense from devicetree\n");
> +			return ret;
> +		}
> +		info->rsense = val/10;
> +	}
> +	return 0;
> +}
> 
[...]

Best regards,
Dimitri Fedrau

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

[André Draszik]

Hi Thomas,

Thanks for your patch!

On Thu, 2025-01-02 at 12:15 +0100, 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 todo is still applicable given there are other slaves on the
> bus (pca9468, other max77759 functions and the max20339 OVP).
> 
> 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 | 10 ++++++++++
>  1 file changed, 10 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..6c83ee6f8a6b0327c576573d03a8d2bcc93f9e16 100644
> --- a/arch/arm64/boot/dts/exynos/google/gs101-oriole.dts
> +++ b/arch/arm64/boot/dts/exynos/google/gs101-oriole.dts
> @@ -10,6 +10,7 @@
>  
>  #include <dt-bindings/gpio/gpio.h>
>  #include <dt-bindings/input/input.h>
> +#include <dt-bindings/interrupt-controller/irq.h>
>  #include "gs101-pinctrl.h"
>  #include "gs101.dtsi"
>  
> @@ -90,6 +91,15 @@ 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";
> +		shunt-resistor-micro-ohms = <5000>;
> +		interrupt-parent = <&gpa9>;
> +		interrupts = <3 IRQ_TYPE_LEVEL_LOW>;
> +	};

The order of properties within a node should follow
Documentation/devicetree/bindings/dts-coding-style.rst

In particular shunt-resistor-micro-ohms should come last in
this case.

Cheers,
Andre'

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

[André Draszik]

Hi Thomas,

Thanks for your patch!

On Thu, 2025-01-02 at 12:15 +0100, Thomas Antoine via B4 Relay wrote:
> From: Thomas Antoine <t.antoine@uclouvain.be>
> 
> The interface of the Maxim max77759 fuel gauge has a lot of common with the
> Maxim max1720x. The major difference is the lack of non-volatile memory
> slave address. 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
> 
> Other differences include the lack of V_BATT register to read the battery
> level and a difference in the way to identify the chip (the same register
> is used but not the same mask).

It also seems the reported POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN is
quite a bit off - on my Pixel 6, it reports ca. 1131mAh, but the downstream
stack reports a more reasonable 4524mAh. Interestingly, this is an exact
multiple of four.

POWER_SUPPLY_PROP_CHARGE_FULL is off in a similar way, and I suspect that
related properties like charge_avg, time_to_empty, time_to_full are
reported incorrectly as well.

[...]

> @@ -483,14 +608,27 @@ static int max1720x_probe(struct i2c_client *client)
>  	psy_cfg.drv_data = info;
>  	psy_cfg.fwnode = dev_fwnode(dev);
>  	i2c_set_clientdata(client, info);
> -	info->regmap = devm_regmap_init_i2c(client, &max1720x_regmap_cfg);
> +
> +	data = device_get_match_data(dev);
> +	if (!data)
> +		return dev_err_probe(dev, ret, "Failed to get chip data\n");
                                          ^^^
This should be -EINVAL.


Cheers,
Andre'

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

[André Draszik]

On Thu, 2025-01-02 at 12:15 +0100, 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>

Reviewed-by: André Draszik <andre.draszik@linaro.org>

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

[Christophe JAILLET]

Le 02/01/2025 à 12:15, Thomas Antoine via B4 Relay a écrit :
> From: Thomas Antoine <t.antoine@uclouvain.be>
> 
> The interface of the Maxim max77759 fuel gauge has a lot of common with the
> Maxim max1720x. The major difference is the lack of non-volatile memory
> slave address. 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.

Hi,

...

> +	ret = max1720x_get_rsense(dev, info);
>   	if (ret)
> -		return dev_err_probe(dev, ret, "Failed to probe nvmem\n");
> +		return dev_err_probe(dev, ret, "Failed to get RSense");

Missing ending \n.

>   
>   	bat = devm_power_supply_register(dev, &max1720x_bat_desc, &psy_cfg);
>   	if (IS_ERR(bat))

...

CJ

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

[Peter Griffin]

Hi Thomas,

On Thu, 2 Jan 2025 at 11:16, Thomas Antoine via B4 Relay
<devnull+t.antoine.uclouvain.be@kernel.org> wrote:
>
> From: Thomas Antoine <t.antoine@uclouvain.be>
>
> The interface of the Maxim max77759 fuel gauge has a lot of common with the
> Maxim max1720x. The major difference is the lack of non-volatile memory
> slave address. 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
>
> Other differences include the lack of V_BATT register to read the battery
> level and a difference in the way to identify the chip (the same register
> is used but not the same mask).
>
> Add support for the max77759 by allowing to use the non-volatile
> memory or not based on the chip. Also add the V_CELL regsister as a
> fallback to read voltage value in the case where read of V_BATT fails.
>
> The cast is necessary to avoid an overflow when the value of the register
> is above 54975 (equivalent to a voltage around 4.29 V).
>
> The regmap of the max77759 will lead the read to fail for V_BATT and to
> correctly use V_CELL instead. This regmap was proposed by André Draszik in
>
> Link: https://lore.kernel.org/all/d1bade77b5281c1de6b2ddcb4dbbd033e455a116.camel@linaro.org/

Firstly thanks for working on this :) Fuel gauge is a great addition
to the Pixel 6 upstream support.

It would be good if you can document what you consider "working" with
this series applied. I think it's fine for support to come
incrementally (say feature additions to the driver to support x,y,z).
But anything that is now reported in sysfs with this series applied
should be accurate (or alternatively not exposed for MAX77759 variant
if it's not accurate and it's unclear from checking the downstream
driver how to fix it).

Thanks,

Peter

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

[Thomas Antoine]

[...]  

>> @@ -90,6 +91,15 @@ eeprom: eeprom@50 {
>>  &hsi2c_12 {
>>  	status = "okay";
>>  	/* TODO: add th
>> +	fuel-gauge@36 {
>> +		compatible = "maxim,max77759-fg";
>> +		reg = <0x36>;
>> +		reg-names = "m5";
>> +		shunt-resistor-micro-ohms = <5000>;
>> +		interrupt-parent = <&gpa9>;
>> +		interrupts = <3 IRQ_TYPE_LEVEL_LOW>;
>> +	};
> 
> The order of properties within a node should follow
> Documentation/devicetree/bindings/dts-coding-style.rst
> 
> In particular shunt-resistor-micro-ohms should come last in
> this case.
> 
> Cheers,
> Andre'
> 

Hi,

Thank you, I will fix this.

Best regards,
Thomas

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

[Thomas Antoine]

On 1/7/25 19:10, Christophe JAILLET wrote:
> Le 02/01/2025 à 12:15, Thomas Antoine via B4 Relay a écrit :
>> From: Thomas Antoine <t.antoine@uclouvain.be>
>>
>> The interface of the Maxim max77759 fuel gauge has a lot of common with the
>> Maxim max1720x. The major difference is the lack of non-volatile memory
>> slave address. 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.
> 
> Hi,
> 
> ...
> 
>> +     ret = max1720x_get_rsense(dev, info);
>>       if (ret)
>> -             return dev_err_probe(dev, ret, "Failed to probe nvmem\n");
>> +             return dev_err_probe(dev, ret, "Failed to get RSense");
> 
> Missing ending \n.

Hi,

I will fix this, thank you.

Best regards,
Thomas

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

[Thomas Antoine]

Hi,

>> [...]
>>  
>>  struct max1720x_device_info {
>>  	struct regmap *regmap;
>>  	struct regmap *regmap_nv;
>>  	struct i2c_client *ancillary;
>>  	int rsense;
>> +	int has_nvmem;
> 
> Switch to bool and why is needed twice ? Here and in chip_data. Better
> keep it in chip_data.

It is useless in device_info at the moment, I'm not sure why I thought I
neeed it there at the time. I will remove it.

>> [...]
>> +// Use 64 bits because computation on 32 bits leads to an overflow
>> +static int max172xx_cell_voltage_to_ps(unsigned int reg)
>> +{
>> +	u64 val = reg;
>> +
>> +	return val * 78125 / 1000;	/* in uV */
> 
> You could avoid the overflow with:
> 	return val * 625 / 8;	/* in uV */

Indeed, I will change that.

>> +}
>> +
>>  static int max172xx_capacity_to_ps(unsigned int reg)
>>  {
>>  	return reg * 500;	/* in uAh */
>> @@ -303,6 +383,33 @@ static int max172xx_current_to_voltage(unsigned int reg)
>>  	return val * 156252;
>>  }
>>  
>> +static int max1720x_devname_to_model(unsigned int reg_val,
>> +					union power_supply_propval *val,
>> +					struct max1720x_device_info *info)
> 
> nit: Align with open parenthesis

Will fix.

>>  static int max1720x_battery_get_property(struct power_supply *psy,
>>  					 enum power_supply_property psp,
>>  					 union power_supply_propval *val)
>> @@ -332,7 +439,12 @@ static int max1720x_battery_get_property(struct power_supply *psy,
>>  		break;
>>  	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
>>  		ret = regmap_read(info->regmap, MAX172XX_BATT, &reg_val);
>> -		val->intval = max172xx_voltage_to_ps(reg_val);
>> +		if (!ret)
>> +			val->intval = max172xx_voltage_to_ps(reg_val);
>> +		else {
>> +			ret = regmap_read(info->regmap, MAX172XX_VCELL, &reg_val);
>> +			val->intval = max172xx_cell_voltage_to_ps(reg_val);
>> +		}
> 
> Would be better to read the right register, since we know which device
> we are. You could differentiate like in max1720x_devname_to_model.

Will do.

>> [...]
>> +static int max1720x_get_rsense(struct device *dev,
>> +					 struct max1720x_device_info *info)
> 
> nit: Align with open parenthesis.
> 
>> [...]
>> +		ret = of_property_read_u32(dev->of_node,
>> +					"shunt-resistor-micro-ohms", &val);
> 
> nit: Align with open parenthesis.

I will fix those too. Thanks for the feedback.

Best regards,
Thomas

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

[Thomas Antoine]

Hi,

Thanks for taking the time to test the system.

On 1/7/25 12:00, André Draszik wrote:
> Hi Thomas,
> 
> Thanks for your patch!
> 
> On Thu, 2025-01-02 at 12:15 +0100, Thomas Antoine via B4 Relay wrote:
>> From: Thomas Antoine <t.antoine@uclouvain.be>
>>
>> The interface of the Maxim max77759 fuel gauge has a lot of common with the
>> Maxim max1720x. The major difference is the lack of non-volatile memory
>> slave address. 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.
>>
>> https://android.googlesource.com/kernel/google-modules/bms/+/1a68c36bef474573cc8629cc1d121eb6a81ab68c/max1720x_battery.c
>>
>> Other differences include the lack of V_BATT register to read the battery
>> level and a difference in the way to identify the chip (the same register
>> is used but not the same mask).
> 
> It also seems the reported POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN is
> quite a bit off - on my Pixel 6, it reports ca. 1131mAh, but the downstream
> stack reports a more reasonable 4524mAh. Interestingly, this is an exact
> multiple of four.
> 
> POWER_SUPPLY_PROP_CHARGE_FULL is off in a similar way, and I suspect that
> related properties like charge_avg, time_to_empty, time_to_full are
> reported incorrectly as well.

Indeed, now that I check the code, the current computation is wrong.
In the downstream kernel, reg_to_capacity_uah is used to translate the register
value. In the end, it computes the value as follows:

div_s64((s64) val * 500000, rsense) * lsb;
Link: https://android.googlesource.com/kernel/google-modules/bms/+/refs/heads/android-gs-raviole-5.10-android15/max1720x_battery.h#36

whereas the mainline driver does val * 500.
Based on what I saw, lsb should be 1 to 2 based on the value of the register
MAX_M5_TASKPERIOD.

Basically, if lsb is 1 and given the default rsense of the mainline driver,
the two functions will return the same. 

From the datasheet of the max17201, capacity LSB is "5.0μVh/RSENSE".
So it seems that the current mainline driver is only right if rsense is equal
to 10mOhms.

The factor 4 that you see should thus come from
1. a factor 2 because we do 5.0μVh/10mOhm instead of 5.0μVh/5mOhm.
2. a factor 2 because we do not take into account lsb.

MAX_M5_TASKPERIOD is reg 0x3c which is not mentionned at all in the datasheet
of the max17201. I guess this might be another difference between the two
devices.

I think the best course of action is to correct the computation to take into
account rsense and to then multiply by lsb only for max77759.
This would make the behaviour of the max17201 follow the datasheet.

> [...]
> 
>> @@ -483,14 +608,27 @@ static int max1720x_probe(struct i2c_client *client)
>>  	psy_cfg.drv_data = info;
>>  	psy_cfg.fwnode = dev_fwnode(dev);
>>  	i2c_set_clientdata(client, info);
>> -	info->regmap = devm_regmap_init_i2c(client, &max1720x_regmap_cfg);
>> +
>> +	data = device_get_match_data(dev);
>> +	if (!data)
>> +		return dev_err_probe(dev, ret, "Failed to get chip data\n");
>                                           ^^^
> This should be -EINVAL.

Indeed, will fix.

Best regards,
Thomas

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

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 2041 bytes --]

Hi,

On Thu, Jan 02, 2025 at 12:15:03PM +0100, Thomas Antoine via B4 Relay wrote:
> From: Thomas Antoine <t.antoine@uclouvain.be>
> 
> The interface of the Maxim max77759 fuel gauge has a lot of common with the
> Maxim max1720x. The major difference is the lack of non-volatile memory
> slave address. 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
> 
> Other differences include the lack of V_BATT register to read the battery
> level and a difference in the way to identify the chip (the same register
> is used but not the same mask).
> 
> Add support for the max77759 by allowing to use the non-volatile
> memory or not based on the chip. Also add the V_CELL regsister as a
> fallback to read voltage value in the case where read of V_BATT fails.
> 
> The cast is necessary to avoid an overflow when the value of the register
> is above 54975 (equivalent to a voltage around 4.29 V).
> 
> The regmap of the max77759 will lead the read to fail for V_BATT and to
> correctly use V_CELL instead. This regmap was proposed by André Draszik in
> 
> Link: https://lore.kernel.org/all/d1bade77b5281c1de6b2ddcb4dbbd033e455a116.camel@linaro.org/
> 
> Signed-off-by: Thomas Antoine <t.antoine@uclouvain.be>
> ---

Please add output from to the cover letter to allow easily verifying
that all values are correctly scaled.

./tools/testing/selftests/power_supply/test_power_supply_properties.sh

> +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),
> +};

Drop the yes_range from the write table. It is wrong and confusing.

Greetings,

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[André Draszik]

Hi Sebastian,

On Wed, 2025-01-15 at 22:30 +0100, Sebastian Reichel wrote:
> Hi,
> 
> On Thu, Jan 02, 2025 at 12:15:03PM +0100, Thomas Antoine via B4 Relay wrote:
> > From: Thomas Antoine <t.antoine@uclouvain.be>
> > 
> > The interface of the Maxim max77759 fuel gauge has a lot of common with the
> > Maxim max1720x. The major difference is the lack of non-volatile memory
> > slave address. 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
> > 
> > Other differences include the lack of V_BATT register to read the battery
> > level and a difference in the way to identify the chip (the same register
> > is used but not the same mask).
> > 
> > Add support for the max77759 by allowing to use the non-volatile
> > memory or not based on the chip. Also add the V_CELL regsister as a
> > fallback to read voltage value in the case where read of V_BATT fails.
> > 
> > The cast is necessary to avoid an overflow when the value of the register
> > is above 54975 (equivalent to a voltage around 4.29 V).
> > 
> > The regmap of the max77759 will lead the read to fail for V_BATT and to
> > correctly use V_CELL instead. This regmap was proposed by André Draszik in
> > 
> > Link: https://lore.kernel.org/all/d1bade77b5281c1de6b2ddcb4dbbd033e455a116.camel@linaro.org/
> > 
> > Signed-off-by: Thomas Antoine <t.antoine@uclouvain.be>
> > ---
> 
> Please add output from to the cover letter to allow easily verifying
> that all values are correctly scaled.
> 
> ./tools/testing/selftests/power_supply/test_power_supply_properties.sh
> 
> > +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),
> > +};
> 
> Drop the yes_range from the write table. It is wrong and confusing.

Can you please clarify why having yes_ranges is wrong? Without yes_ranges,
all registers not in no_ranges are allowed to be written to.

Here, max77759_registers already specifies all the registers that exist
(and is also used in the max77759_read_table), and for write-access this is
further limited by the read-only registers in no_ranges.

As an example, register 0x50 doesn't exist, and without yes_ranges this
would allow write access to it.

If yes_ranges was dropped, all the information about non-existing registers
would have to be duplicated into no_ranges by inversing max77759_registers.
We already know the non-existing registers, and inversing that list just to
add to no_ranges seems non-ideal, error-prone, and just duplicated information.


Cheers,
Andre'