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

[PATCH v4 0/5] 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.

The voltage, current, capacity, temperature and charge have all been
tested and show coherent results. The charge full design and capacity
equal the ones seen on android, the ratio between average charge and
average current does predict pretty accurately the time to empty under
a constant workload and temperature is coherent with the dynamic state
of the device.

Health is not enabled as it always reports overheating. The time to empty
is wrong by about a factor 2 and is thus also disabled.

Signed-off-by: Thomas Antoine <t.antoine@uclouvain.be>
---
Changes in v4:
- Make first patch standalone
- Separate MAX77759 defines from MAX1720x defines (Dimitri Fedrau)
- Inline device name property (Dimitri Fedrau)
- Separate MAX77759 capacity lsb logic from the MAX1720x capacity
  computation (Dimitri Fedrau)
- Use device_property_read_u32 instead of of_property_read_u32
  (Sebastian Reichel)
- Removed leftover debugs
- Move shunt-resistor-micro-ohms to out of allOf:if: (Krzysztof Kozlowski)
- Fix reg-names constraints
- Fix style errors
- Link to v3: https://lore.kernel.org/r/20250421-b4-gs101_max77759_fg-v3-0-50cd8caf9017@uclouvain.be

Changes in v3:
- Update base tree to avoid conflicts
- Fix capacity computation for max1720x
- Add separate properties for the max7759 to disable non-functional ones
- Take TASKPERIOD into account for voltage computation of max77759
- Simplify vcell computation (Dimitri Fedrau)
- Switch has_nvmem to bool and keep it only in chip_data (Dimitri Fedrau)
- Drop the yes_range from the write table (Sebastian Reichel)
- Add test_power_supply_properties.sh to cover letter (Sebastian Reichel)
- Switch back some changes to binding and actually use allOf:if: to
  restrict constraints (Krzysztof Kozlowski)
- Fix style errors
- Link to v2: https://lore.kernel.org/r/20250102-b4-gs101_max77759_fg-v2-0-87959abeb7ff@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

tools/testing/selftests/power_supply/test_power_supply_properties.sh:
  # Testing device max77759-fg
  ok 1 max77759-fg.exists
  ok 2 max77759-fg.uevent.NAME
  ok 3 max77759-fg.sysfs.type
  ok 4 max77759-fg.uevent.TYPE
  ok 5 max77759-fg.sysfs.usb_type # SKIP
  ok 6 max77759-fg.sysfs.online # SKIP
  # Reported: '1' ()
  ok 7 max77759-fg.sysfs.present
  ok 8 max77759-fg.sysfs.status # SKIP
  # Reported: '78' % ()
  ok 9 max77759-fg.sysfs.capacity
  ok 10 max77759-fg.sysfs.capacity_level # SKIP
  # Reported: 'MAX77759' ()
  ok 11 max77759-fg.sysfs.model_name
  # Reported: 'Maxim Integrated' ()
  ok 12 max77759-fg.sysfs.manufacturer
  ok 13 max77759-fg.sysfs.serial_number # SKIP
  ok 14 max77759-fg.sysfs.technology # SKIP
  ok 15 max77759-fg.sysfs.cycle_count # SKIP
  ok 16 max77759-fg.sysfs.scope # SKIP
  ok 17 max77759-fg.sysfs.input_current_limit # SKIP
  ok 18 max77759-fg.sysfs.input_voltage_limit # SKIP
  # Reported: '4238593' uV (4.23859 V)
  ok 19 max77759-fg.sysfs.voltage_now
  ok 20 max77759-fg.sysfs.voltage_min # SKIP
  ok 21 max77759-fg.sysfs.voltage_max # SKIP
  ok 22 max77759-fg.sysfs.voltage_min_design # SKIP
  ok 23 max77759-fg.sysfs.voltage_max_design # SKIP
  # Reported: '-149689' uA ()
  ok 24 max77759-fg.sysfs.current_now
  ok 25 max77759-fg.sysfs.current_max # SKIP
  ok 26 max77759-fg.sysfs.charge_now # SKIP
  # Reported: '4716000' uAh (4.716 Ah)
  ok 27 max77759-fg.sysfs.charge_full
  # Reported: '4524000' uAh (4.524 Ah)
  ok 28 max77759-fg.sysfs.charge_full_design
  ok 29 max77759-fg.sysfs.power_now # SKIP
  ok 30 max77759-fg.sysfs.energy_now # SKIP
  ok 31 max77759-fg.sysfs.energy_full # SKIP
  ok 32 max77759-fg.sysfs.energy_full_design # SKIP
  ok 33 max77759-fg.sysfs.energy_full_design # SKIP

---
Thomas Antoine (5):
      power: supply: max1720x correct capacity computation
      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      |  42 +++-
 .../boot/dts/exynos/google/gs101-pixel-common.dtsi |  10 +
 arch/arm64/configs/defconfig                       |   1 +
 drivers/power/supply/max1720x_battery.c            | 276 ++++++++++++++++++---
 4 files changed, 294 insertions(+), 35 deletions(-)
---
base-commit: e48e99b6edf41c69c5528aa7ffb2daf3c59ee105
change-id: 20241202-b4-gs101_max77759_fg-402e231a4b33

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

[PATCH v4 1/5] power: supply: max1720x correct capacity computation

[Thomas Antoine via B4 Relay]

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

From the datasheet of the MAX17201/17205, the LSB should be
"5.0μVh/RSENSE". The current computation sets it at 0.5mAh=5.0μVh/10mOhm
which does not take into account the value of rsense which can be
different from 10mOhm.

Change the computation to fit the specs.

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

diff --git a/drivers/power/supply/max1720x_battery.c b/drivers/power/supply/max1720x_battery.c
index ea3912fd1de8bfd0d029c16f276316d06e1b105c..68b5314ecf3a234f906ec8fe400e586855b69cd9 100644
--- a/drivers/power/supply/max1720x_battery.c
+++ b/drivers/power/supply/max1720x_battery.c
@@ -288,9 +288,10 @@ static int max172xx_voltage_to_ps(unsigned int reg)
 	return reg * 1250;	/* in uV */
 }
 
-static int max172xx_capacity_to_ps(unsigned int reg)
+static int max172xx_capacity_to_ps(unsigned int reg,
+				   struct max1720x_device_info *info)
 {
-	return reg * 500;	/* in uAh */
+	return reg * (500000 / info->rsense);	/* in uAh */
 }
 
 /*
@@ -394,11 +395,11 @@ static int max1720x_battery_get_property(struct power_supply *psy,
 		break;
 	case POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN:
 		ret = regmap_read(info->regmap, MAX172XX_DESIGN_CAP, &reg_val);
-		val->intval = max172xx_capacity_to_ps(reg_val);
+		val->intval = max172xx_capacity_to_ps(reg_val, info);
 		break;
 	case POWER_SUPPLY_PROP_CHARGE_AVG:
 		ret = regmap_read(info->regmap, MAX172XX_REPCAP, &reg_val);
-		val->intval = max172xx_capacity_to_ps(reg_val);
+		val->intval = max172xx_capacity_to_ps(reg_val, info);
 		break;
 	case POWER_SUPPLY_PROP_TIME_TO_EMPTY_AVG:
 		ret = regmap_read(info->regmap, MAX172XX_TTE, &reg_val);
@@ -422,7 +423,7 @@ static int max1720x_battery_get_property(struct power_supply *psy,
 		break;
 	case POWER_SUPPLY_PROP_CHARGE_FULL:
 		ret = regmap_read(info->regmap, MAX172XX_FULL_CAP, &reg_val);
-		val->intval = max172xx_capacity_to_ps(reg_val);
+		val->intval = max172xx_capacity_to_ps(reg_val, info);
 		break;
 	case POWER_SUPPLY_PROP_MODEL_NAME:
 		ret = regmap_read(info->regmap, MAX172XX_DEV_NAME, &reg_val);

-- 
2.49.0

[PATCH v4 2/5] 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. A 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. The voltage must instead be read from V_CELL, the lowest voltage of
all cells. The mask to identify the chip is different. The computation of
the charge must also be changed to take into account TASKPERIOD, which
can add a factor 2 to the result.

Add support for the MAX77759 by taking into account all of those
differences based on chip type.

Do not advertise temp probes using the non-volatile memory as those are
not available.

The 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 | 265 ++++++++++++++++++++++++++++----
 1 file changed, 238 insertions(+), 27 deletions(-)

diff --git a/drivers/power/supply/max1720x_battery.c b/drivers/power/supply/max1720x_battery.c
index 68b5314ecf3a234f906ec8fe400e586855b69cd9..c9ad452ada9d0a2a51f37d04fd8c3260be522405 100644
--- a/drivers/power/supply/max1720x_battery.c
+++ b/drivers/power/supply/max1720x_battery.c
@@ -37,6 +37,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_FULL_CAP		0x10	/* Calculated full capacity */
@@ -54,15 +55,28 @@
 #define MAX172XX_BATT			0xDA	/* Battery voltage */
 #define MAX172XX_ATAVCAP		0xDF
 
+#define MAX77759_DEV_NAME_TYPE_MASK	GENMASK(15, 9)
+#define MAX77759_DEV_NAME_TYPE_MAX77759	0x31
+#define MAX77759_TASKPERIOD		0x3C
+#define MAX77759_TASKPERIOD_175MS	0x1680
+#define MAX77759_TASKPERIOD_351MS	0x2D00
+
 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;
+	enum chip_id id;
 };
 
 /*
@@ -271,6 +285,80 @@ 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 = {
+	.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,
+};
+
+static const enum power_supply_property max77759_battery_props[] = {
+	POWER_SUPPLY_PROP_PRESENT,
+	POWER_SUPPLY_PROP_CAPACITY,
+	POWER_SUPPLY_PROP_VOLTAGE_NOW,
+	POWER_SUPPLY_PROP_CHARGE_FULL,
+	POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN,
+	POWER_SUPPLY_PROP_CHARGE_AVG,
+	POWER_SUPPLY_PROP_TEMP,
+	POWER_SUPPLY_PROP_CURRENT_NOW,
+	POWER_SUPPLY_PROP_CURRENT_AVG,
+	POWER_SUPPLY_PROP_MODEL_NAME,
+	POWER_SUPPLY_PROP_MANUFACTURER,
+};
+
+
+struct chip_data {
+	bool has_nvmem;
+	const struct regmap_config *regmap_cfg;
+	enum chip_id id;
+};
+
+static const struct chip_data max1720x_data  = {
+	.has_nvmem = true,
+	.regmap_cfg = &max1720x_regmap_cfg,
+	.id = MAX1720X_ID,
+};
+
+static const struct chip_data max77759_data = {
+	.has_nvmem = false,
+	.regmap_cfg = &max77759_regmap_cfg,
+	.id = MAX77759_ID,
+};
+
 /* Convert regs value to power_supply units */
 
 static int max172xx_time_to_ps(unsigned int reg)
@@ -288,12 +376,41 @@ static int max172xx_voltage_to_ps(unsigned int reg)
 	return reg * 1250;	/* in uV */
 }
 
+static int max172xx_cell_voltage_to_ps(unsigned int reg)
+{
+	return reg * 625 / 8;	/* in uV */
+}
+
 static int max172xx_capacity_to_ps(unsigned int reg,
 				   struct max1720x_device_info *info)
 {
 	return reg * (500000 / info->rsense);	/* in uAh */
 }
 
+static int max77759_capacity_lsb(struct max1720x_device_info *info,
+				 unsigned int *lsb)
+{
+	unsigned int reg_task_period;
+	int ret;
+
+	ret = regmap_read(info->regmap, MAX77759_TASKPERIOD, &reg_task_period);
+	if (ret < 0)
+		return ret;
+
+	switch (reg_task_period) {
+	case MAX77759_TASKPERIOD_175MS:
+		*lsb = 1;
+		break;
+	case MAX77759_TASKPERIOD_351MS:
+		*lsb = 2;
+		break;
+	default:
+		return -ENODEV;
+	}
+
+	return 0;
+}
+
 /*
  * Current and temperature is signed values, so unsigned regs
  * value must be converted to signed type
@@ -390,16 +507,36 @@ static int max1720x_battery_get_property(struct power_supply *psy,
 		val->intval = max172xx_percent_to_ps(reg_val);
 		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 (info->id == MAX1720X_ID) {
+			ret = regmap_read(info->regmap, MAX172XX_BATT, &reg_val);
+			val->intval = max172xx_voltage_to_ps(reg_val);
+		} else if (info->id == MAX77759_ID) {
+			ret = regmap_read(info->regmap, MAX172XX_VCELL, &reg_val);
+			val->intval = max172xx_cell_voltage_to_ps(reg_val);
+		} else
+			return -ENODEV;
 		break;
 	case POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN:
 		ret = regmap_read(info->regmap, MAX172XX_DESIGN_CAP, &reg_val);
+		if (ret < 0)
+			return ret;
+
 		val->intval = max172xx_capacity_to_ps(reg_val, info);
+		if (info->id == MAX77759_ID) {
+			ret = max77759_capacity_lsb(info, &reg_val);
+			val->intval *= reg_val;
+		}
 		break;
 	case POWER_SUPPLY_PROP_CHARGE_AVG:
 		ret = regmap_read(info->regmap, MAX172XX_REPCAP, &reg_val);
+		if (ret < 0)
+			return ret;
+
 		val->intval = max172xx_capacity_to_ps(reg_val, info);
+		if (info->id == MAX77759_ID) {
+			ret = max77759_capacity_lsb(info, &reg_val);
+			val->intval *= reg_val;
+		}
 		break;
 	case POWER_SUPPLY_PROP_TIME_TO_EMPTY_AVG:
 		ret = regmap_read(info->regmap, MAX172XX_TTE, &reg_val);
@@ -423,16 +560,35 @@ static int max1720x_battery_get_property(struct power_supply *psy,
 		break;
 	case POWER_SUPPLY_PROP_CHARGE_FULL:
 		ret = regmap_read(info->regmap, MAX172XX_FULL_CAP, &reg_val);
+		if (ret < 0)
+			return ret;
+
 		val->intval = max172xx_capacity_to_ps(reg_val, info);
+		if (info->id == MAX77759_ID) {
+			ret = max77759_capacity_lsb(info, &reg_val);
+			val->intval *= reg_val;
+		}
 		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
+		if (ret < 0)
+			return ret;
+
+		if (info->id == 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;
+		} else if (info->id == 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;
+		} else
 			return -ENODEV;
 		break;
 	case POWER_SUPPLY_PROP_MANUFACTURER:
@@ -527,7 +683,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);
@@ -549,18 +704,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!");
@@ -570,6 +713,38 @@ static int max1720x_probe_nvmem(struct i2c_client *client,
 	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) {
+		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 = device_property_read_u32(dev, "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,
@@ -579,32 +754,67 @@ static const struct power_supply_desc max1720x_bat_desc = {
 	.get_property = max1720x_battery_get_property,
 };
 
+static const struct power_supply_desc max77759_bat_desc = {
+	.name = "max77759-fg",
+	.no_thermal = true,
+	.type = POWER_SUPPLY_TYPE_BATTERY,
+	.properties = max77759_battery_props,
+	.num_properties = ARRAY_SIZE(max77759_battery_props),
+	.get_property = max1720x_battery_get_property,
+};
+
 static int max1720x_probe(struct i2c_client *client)
 {
 	struct power_supply_config psy_cfg = {};
 	struct device *dev = &client->dev;
 	struct max1720x_device_info *info;
 	struct power_supply *bat;
+	const struct chip_data *data;
+	const struct power_supply_desc *bat_desc;
 	int ret;
 
 	info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL);
 	if (!info)
 		return -ENOMEM;
 
+	data = device_get_match_data(dev);
+	if (!data)
+		return dev_err_probe(dev, -EINVAL, "Failed to get chip data\n");
+
 	psy_cfg.drv_data = info;
 	psy_cfg.fwnode = dev_fwnode(dev);
-	psy_cfg.attr_grp = max1720x_groups;
+
+	switch (data->id) {
+	case MAX1720X_ID:
+		psy_cfg.attr_grp = max1720x_groups;
+		bat_desc = &max1720x_bat_desc;
+		break;
+	case MAX77759_ID:
+		bat_desc = &max77759_bat_desc;
+		break;
+	default:
+		return dev_err_probe(dev, -EINVAL, "Unsupported chip\n");
+	}
+
 	i2c_set_clientdata(client, info);
-	info->regmap = devm_regmap_init_i2c(client, &max1720x_regmap_cfg);
+
+	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, data);
 	if (ret)
-		return dev_err_probe(dev, ret, "Failed to probe nvmem\n");
+		return ret;
 
-	bat = devm_power_supply_register(dev, &max1720x_bat_desc, &psy_cfg);
+	bat = devm_power_supply_register(dev, bat_desc, &psy_cfg);
 	if (IS_ERR(bat))
 		return dev_err_probe(dev, PTR_ERR(bat),
 				     "Failed to register power supply\n");
@@ -613,7 +823,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.49.0

[PATCH v4 3/5] dt-bindings: power: supply: add max77759-fg flavor

[Thomas Antoine via B4 Relay]

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

The Maxim 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 Maxim MAX77759 has no non-volatile memory so it doesn't require an
address and instead requires a value for the current sensing resistor.

Keep shunt-resistor-micro-ohms optional for the MAX17201/MAX17205 as it is
not be used at the moment but could be in the future. (e.g. as a default
value to be used in case of nvmem failure)

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

diff --git a/Documentation/devicetree/bindings/power/supply/maxim,max17201.yaml b/Documentation/devicetree/bindings/power/supply/maxim,max17201.yaml
index fe3dd9bd5585618e45220c51023391a5b21acfd2..2fb826b4b160eb9abe6604185ac2192447d90b8c 100644
--- a/Documentation/devicetree/bindings/power/supply/maxim,max17201.yaml
+++ b/Documentation/devicetree/bindings/power/supply/maxim,max17201.yaml
@@ -9,13 +9,11 @@ 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
@@ -25,11 +23,18 @@ properties:
     items:
       - description: ModelGauge m5 registers
       - description: Nonvolatile registers
+    minItems: 1
+    maxItems: 2
 
   reg-names:
     items:
       - const: m5
       - const: nvmem
+    minItems: 1
+    maxItems: 2
+
+  shunt-resistor-micro-ohms:
+    description: The value of current sense resistor in microohms.
 
   interrupts:
     maxItems: 1
@@ -39,6 +44,37 @@ required:
   - reg
   - reg-names
 
+allOf:
+  - $ref: power-supply.yaml#
+
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - maxim,max17201
+    then:
+      properties:
+        reg:
+          minItems: 2
+        reg-names:
+          minItems: 2
+
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - maxim,max77759-fg
+    then:
+      properties:
+        reg:
+          maxItems: 1
+        reg-names:
+          maxItems: 1
+      required:
+        - shunt-resistor-micro-ohms
+
 unevaluatedProperties: false
 
 examples:

-- 
2.49.0

[PATCH v4 4/5] 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 5bb8f09422a22116781169611482179b10798c14..4feaebca340b1bc613c4f4ea9f1ff3b6686b092d 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -687,6 +687,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.49.0

[PATCH v4 5/5] 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 Maxim MAX77759 fuel gauge as a slave of the i2c.

The TODO is still applicable given there are other slaves on the
bus (e.g. PCA9468, other MAX77759 functions and the MAX20339 OVP).

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

diff --git a/arch/arm64/boot/dts/exynos/google/gs101-pixel-common.dtsi b/arch/arm64/boot/dts/exynos/google/gs101-pixel-common.dtsi
index b25230495c64dce60916b7cd5dcb9a7cce5d0e4e..84fc10c3562958ab1621f24644709e85a9433b9b 100644
--- a/arch/arm64/boot/dts/exynos/google/gs101-pixel-common.dtsi
+++ b/arch/arm64/boot/dts/exynos/google/gs101-pixel-common.dtsi
@@ -10,6 +10,7 @@
 
 #include <dt-bindings/gpio/gpio.h>
 #include <dt-bindings/input/input.h>
+#include <dt-bindings/interrupt-controller/irq.h>
 #include <dt-bindings/usb/pd.h>
 #include "gs101-pinctrl.h"
 #include "gs101.dtsi"
@@ -188,6 +189,15 @@ usbc0_role_sw: endpoint {
 			};
 		};
 	};
+
+	fuel-gauge@36 {
+		compatible = "maxim,max77759-fg";
+		reg = <0x36>;
+		reg-names = "m5";
+		interrupt-parent = <&gpa9>;
+		interrupts = <3 IRQ_TYPE_LEVEL_LOW>;
+		shunt-resistor-micro-ohms = <5000>;
+	};
 };
 
 &pinctrl_far_alive {

-- 
2.49.0

Re: [PATCH v4 3/5] dt-bindings: power: supply: add max77759-fg flavor

[Rob Herring (Arm)]

On Fri, 23 May 2025 14:51:46 +0200, Thomas Antoine wrote:
> The Maxim 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 Maxim MAX77759 has no non-volatile memory so it doesn't require an
> address and instead requires a value for the current sensing resistor.
> 
> Keep shunt-resistor-micro-ohms optional for the MAX17201/MAX17205 as it is
> not be used at the moment but could be in the future. (e.g. as a default
> value to be used in case of nvmem failure)
> 
> Signed-off-by: Thomas Antoine <t.antoine@uclouvain.be>
> ---
>  .../bindings/power/supply/maxim,max17201.yaml      | 42 ++++++++++++++++++++--
>  1 file changed, 39 insertions(+), 3 deletions(-)
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/power/supply/maxim,max17201.yaml: properties:reg: {'items': [{'description': 'ModelGauge m5 registers'}, {'description': 'Nonvolatile registers'}], 'minItems': 1, 'maxItems': 2} should not be valid under {'required': ['maxItems']}
	hint: "maxItems" is not needed with an "items" list
	from schema $id: http://devicetree.org/meta-schemas/items.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/power/supply/maxim,max17201.yaml: properties:reg-names: {'items': [{'const': 'm5'}, {'const': 'nvmem'}], 'minItems': 1, 'maxItems': 2} should not be valid under {'required': ['maxItems']}
	hint: "maxItems" is not needed with an "items" list
	from schema $id: http://devicetree.org/meta-schemas/items.yaml#

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20250523-b4-gs101_max77759_fg-v4-3-b49904e35a34@uclouvain.be

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

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

[Peter Griffin]

Hi Thomas,

Thanks for your patch and work to enable fuel gauge on Pixel 6!

On Fri, 23 May 2025 at 13:52, Thomas Antoine via B4 Relay
<devnull+t.antoine.uclouvain.be@kernel.org> wrote:
>
> From: Thomas Antoine <t.antoine@uclouvain.be>
>
> Add the node for the Maxim MAX77759 fuel gauge as a slave of the i2c.
>
> The TODO is still applicable given there are other slaves on the
> bus (e.g. PCA9468, other MAX77759 functions and the MAX20339 OVP).
>
> Signed-off-by: Thomas Antoine <t.antoine@uclouvain.be>
> ---
>  arch/arm64/boot/dts/exynos/google/gs101-pixel-common.dtsi | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/exynos/google/gs101-pixel-common.dtsi b/arch/arm64/boot/dts/exynos/google/gs101-pixel-common.dtsi
> index b25230495c64dce60916b7cd5dcb9a7cce5d0e4e..84fc10c3562958ab1621f24644709e85a9433b9b 100644
> --- a/arch/arm64/boot/dts/exynos/google/gs101-pixel-common.dtsi
> +++ b/arch/arm64/boot/dts/exynos/google/gs101-pixel-common.dtsi
> @@ -10,6 +10,7 @@
>
>  #include <dt-bindings/gpio/gpio.h>
>  #include <dt-bindings/input/input.h>
> +#include <dt-bindings/interrupt-controller/irq.h>
>  #include <dt-bindings/usb/pd.h>
>  #include "gs101-pinctrl.h"
>  #include "gs101.dtsi"
> @@ -188,6 +189,15 @@ usbc0_role_sw: endpoint {
>                         };
>                 };
>         };
> +
> +       fuel-gauge@36 {
> +               compatible = "maxim,max77759-fg";
> +               reg = <0x36>;
> +               reg-names = "m5";
> +               interrupt-parent = <&gpa9>;
> +               interrupts = <3 IRQ_TYPE_LEVEL_LOW>;
> +               shunt-resistor-micro-ohms = <5000>;
> +       };
>  };
>

If gpa-9-3 is being used for the interrupt I think we should also add
the pinctrl configuration for it. Taking a look at downstream the pin
is defined as

&pinctrl_0 {
/* [MAX77759: FG_INTB] > FG_INT_L > [XEINT_23 : SC59845XWE] */
        if_pmic_fg_irq: if-pmic-fg-irq {
                samsung,pins = "gpa9-3"; /* XEINT_23 */
                samsung,pin-function = <EXYNOS_PIN_FUNC_EINT>;
                samsung,pin-pud = <EXYNOS_PIN_PULL_UP>;
                samsung,pin-drv = <GS101_PIN_DRV_2_5_MA>;
        };
};

and then the fuel-gauge node declares

/* FG_INT_L -> XEINT_23 */
pinctrl-names = "default";
pinctrl-0 = <&if_pmic_fg_irq>;

regards,

Peter

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

[Peter Griffin]

Hi Thomas,

On Fri, 23 May 2025 at 13:52, Thomas Antoine via B4 Relay
<devnull+t.antoine.uclouvain.be@kernel.org> 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: Peter Griffin <peter.griffin@linaro.org>

Re: [PATCH v4 2/5] power: supply: add support for max77759 fuel gauge

[Peter Griffin]

Hi Thomas,

Thanks for your patch and working to get fuel gauge functional on
Pixel 6! I've tried to do quite an in-depth review comparing with the
downstream driver.

On Fri, 23 May 2025 at 13:52, 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. A 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. The voltage must instead be read from V_CELL, the lowest voltage of
> all cells. The mask to identify the chip is different. The computation of
> the charge must also be changed to take into account TASKPERIOD, which
> can add a factor 2 to the result.
>
> Add support for the MAX77759 by taking into account all of those
> differences based on chip type.
>
> Do not advertise temp probes using the non-volatile memory as those are
> not available.
>
> The regmap was proposed by André Draszik in
>
> Link: https://lore.kernel.org/all/d1bade77b5281c1de6b2ddcb4dbbd033e455a116.camel@linaro.org/

I think it would be worth noting in the commit message this is basic
initial support for the M5 gauge in MAX77759, and things like loading
& saving the m5 model aren't implemented yet.

That's important as some values such as the REPSOC register value used
for POWER_SUPPLY_PROP_CAPACITY show the result after all processing
including ModelGauge mixing etc, so these values won't be as accurate
as downstream.

>
> Signed-off-by: Thomas Antoine <t.antoine@uclouvain.be>
> ---
>  drivers/power/supply/max1720x_battery.c | 265 ++++++++++++++++++++++++++++----
>  1 file changed, 238 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/power/supply/max1720x_battery.c b/drivers/power/supply/max1720x_battery.c
> index 68b5314ecf3a234f906ec8fe400e586855b69cd9..c9ad452ada9d0a2a51f37d04fd8c3260be522405 100644
> --- a/drivers/power/supply/max1720x_battery.c
> +++ b/drivers/power/supply/max1720x_battery.c
> @@ -37,6 +37,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_FULL_CAP              0x10    /* Calculated full capacity */
> @@ -54,15 +55,28 @@
>  #define MAX172XX_BATT                  0xDA    /* Battery voltage */
>  #define MAX172XX_ATAVCAP               0xDF
>
> +#define MAX77759_DEV_NAME_TYPE_MASK    GENMASK(15, 9)
> +#define MAX77759_DEV_NAME_TYPE_MAX77759        0x31
> +#define MAX77759_TASKPERIOD            0x3C
> +#define MAX77759_TASKPERIOD_175MS      0x1680
> +#define MAX77759_TASKPERIOD_351MS      0x2D00
> +
>  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;
> +       enum chip_id id;
>  };
>
>  /*
> @@ -271,6 +285,80 @@ 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 = {
> +       .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,
> +};
> +
> +static const enum power_supply_property max77759_battery_props[] = {
> +       POWER_SUPPLY_PROP_PRESENT,

I checked the register values match downstream - this looks correct

> +       POWER_SUPPLY_PROP_CAPACITY,

I checked the register offset matchs downstream. The value reported
varies a bit versus downstream. As mentioned above that is likely due
to the REPSOC register reporting after mixing with the m5 model which
is not loaded currently. Also the application specific values and cell
characterization information used by the model isn't configured
currently (see link below in _TEMP property below for the initial fuel
gauge params used by downstream.

> +       POWER_SUPPLY_PROP_VOLTAGE_NOW,

I checked the register offset matchs downstream. Values reported look sensible.

> +       POWER_SUPPLY_PROP_CHARGE_FULL,

Downstream has a slightly different implementation than upstream for
this property. See here
https://android.googlesource.com/kernel/google-modules/bms/+/1a68c36bef474573cc8629cc1d121eb6a81ab68c/max1720x_battery.c#2244

> +       POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN,

I checked the register offset value is correct. However this is
reporting 3000000 and downstream reports 4524000. I checked and it's
just converting the register reset value of DESIGNCAP which is 0xbb8.

This is listed as a "application specific" value, so it maybe we just
need to write the correct initial value to DESIGNCAP (see TEMP section
below)


> +       POWER_SUPPLY_PROP_CHARGE_AVG,

This property isn't reported downstream. The value is changing and not
just the reset value. I noticed REPSOC is an output of the ModelGauge
algorithm so it is likely not to be completely accurate.

> +       POWER_SUPPLY_PROP_TEMP,

I checked the register offset value is correct. However the
temperature is always being reported as the register reset value of
220. This is for obvious reasons quite an important one to report
correctly.

I started debugging this a bit, and it is caused by an incorrectly
configured CONFIG (0x1D) register. In particular the TEX[8] bit is 1
on reset in this register which means temperature measurements are
written from the host AP. When this bit is set to 0, measurements on
the AIN pin are converted to a temperature value and stored in the
Temperature register (I then saw values of 360 and the value
changing).

See here for the bits in that CONFIG register
https://android.googlesource.com/kernel/google-modules/bms/+/1a68c36bef474573cc8629cc1d121eb6a81ab68c/max_m5_reg.h#403

In downstream all these initial register settings are taken from DT
here  https://android.googlesource.com/kernel/google-modules/raviole-device/+/refs/heads/android14-gs-pixel-6.1/arch/arm64/boot/dts/google/gs101-fake-battery-data.dtsi#27

For temperature when TEX=0, TGAIN, TOFF and TCURVE registers should
also be configured to adjust the temperature measurement.

I think it would likely be worth initialising all the fuel gauge
registers referenced in maxim,fg-params as that includes some of the
application specific values for DESIGNCAP, also some of the cell
characterization information, and hopefully we will get more accurate
values from the fuel gauge generally.

> +       POWER_SUPPLY_PROP_CURRENT_NOW,

I checked the register offset matches downstream. Values reported look
reasonable.

> +       POWER_SUPPLY_PROP_CURRENT_AVG,

I checked the register offset matches downstream. Values reported look
reasonable.

> +       POWER_SUPPLY_PROP_MODEL_NAME,

This property isn't reported downstream.

> +       POWER_SUPPLY_PROP_MANUFACTURER,
> +};
> +
> +
> +struct chip_data {
> +       bool has_nvmem;
> +       const struct regmap_config *regmap_cfg;
> +       enum chip_id id;
> +};
> +
> +static const struct chip_data max1720x_data  = {
> +       .has_nvmem = true,
> +       .regmap_cfg = &max1720x_regmap_cfg,
> +       .id = MAX1720X_ID,
> +};
> +
> +static const struct chip_data max77759_data = {
> +       .has_nvmem = false,
> +       .regmap_cfg = &max77759_regmap_cfg,
> +       .id = MAX77759_ID,
> +};
> +
>  /* Convert regs value to power_supply units */
>
>  static int max172xx_time_to_ps(unsigned int reg)
> @@ -288,12 +376,41 @@ static int max172xx_voltage_to_ps(unsigned int reg)
>         return reg * 1250;      /* in uV */
>  }
>
> +static int max172xx_cell_voltage_to_ps(unsigned int reg)
> +{
> +       return reg * 625 / 8;   /* in uV */
> +}
> +
>  static int max172xx_capacity_to_ps(unsigned int reg,
>                                    struct max1720x_device_info *info)
>  {
>         return reg * (500000 / info->rsense);   /* in uAh */
>  }
>
> +static int max77759_capacity_lsb(struct max1720x_device_info *info,
> +                                unsigned int *lsb)
> +{
> +       unsigned int reg_task_period;
> +       int ret;
> +
> +       ret = regmap_read(info->regmap, MAX77759_TASKPERIOD, &reg_task_period);
> +       if (ret < 0)
> +               return ret;
> +
> +       switch (reg_task_period) {
> +       case MAX77759_TASKPERIOD_175MS:
> +               *lsb = 1;
> +               break;
> +       case MAX77759_TASKPERIOD_351MS:
> +               *lsb = 2;
> +               break;
> +       default:
> +               return -ENODEV;
> +       }
> +
> +       return 0;
> +}
> +
>  /*
>   * Current and temperature is signed values, so unsigned regs
>   * value must be converted to signed type
> @@ -390,16 +507,36 @@ static int max1720x_battery_get_property(struct power_supply *psy,
>                 val->intval = max172xx_percent_to_ps(reg_val);
>                 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 (info->id == MAX1720X_ID) {
> +                       ret = regmap_read(info->regmap, MAX172XX_BATT, &reg_val);
> +                       val->intval = max172xx_voltage_to_ps(reg_val);

I think MAX1720X using MAX172XX_BATT register is likely a bug as the
downstream driver uses MAX172XX_VCELL for that variant  see here
https://android.googlesource.com/kernel/google-modules/bms/+/1a68c36bef474573cc8629cc1d121eb6a81ab68c/max1720x.h#304

Having said that, if we do need to cope with differing register
offsets for the different fuel gauge variants it would be nicer to
abstract them in a way similar to the downstream driver. See here
https://android.googlesource.com/kernel/google-modules/bms/+/1a68c36bef474573cc8629cc1d121eb6a81ab68c/max_m5.c#1235.
I think that would be more scalable in supporting multiple variants in
one driver. Otherwise we will have an explosion of if(id==blah) else
if (id==blah) in the driver.

kind regards,

Peter

Re: [PATCH v4 2/5] power: supply: add support for max77759 fuel gauge

[Sebastian Reichel]

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

Hi,

On Fri, May 23, 2025 at 02:51:45PM +0200, 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. A 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. The voltage must instead be read from V_CELL, the lowest voltage of
> all cells. The mask to identify the chip is different. The computation of
> the charge must also be changed to take into account TASKPERIOD, which
> can add a factor 2 to the result.
> 
> Add support for the MAX77759 by taking into account all of those
> differences based on chip type.
> 
> Do not advertise temp probes using the non-volatile memory as those are
> not available.
> 
> The 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 | 265 ++++++++++++++++++++++++++++----
>  1 file changed, 238 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/power/supply/max1720x_battery.c b/drivers/power/supply/max1720x_battery.c
> index 68b5314ecf3a234f906ec8fe400e586855b69cd9..c9ad452ada9d0a2a51f37d04fd8c3260be522405 100644
> --- a/drivers/power/supply/max1720x_battery.c
> +++ b/drivers/power/supply/max1720x_battery.c
> @@ -37,6 +37,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 */
[...]
>  	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
[...]
> +			ret = regmap_read(info->regmap, MAX172XX_VCELL, &reg_val);
> +			val->intval = max172xx_cell_voltage_to_ps(reg_val);

I haven't reviewed this fully due to all the feedback you already
got from Peter Griffin and the DT binding being broken, but something
that catched my eye:

POWER_SUPPLY_PROP_VOLTAGE_NOW provides the voltage of the whole
battery and not of a single cell. E.g. a typical Li-Ion battery
with two serial cells has a nominal voltage of roughly 7.4V while
each cell has just 3.7V.

Greetings,

-- Sebastian

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

Re: (subset) [PATCH v4 0/5] Google Pixel 6 (oriole): max77759 fuel gauge enablement and driver support

[Sebastian Reichel]

On Fri, 23 May 2025 14:51:43 +0200, Thomas Antoine wrote:
> 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.
> 
> The voltage, current, capacity, temperature and charge have all been
> tested and show coherent results. The charge full design and capacity
> equal the ones seen on android, the ratio between average charge and
> average current does predict pretty accurately the time to empty under
> a constant workload and temperature is coherent with the dynamic state
> of the device.
> 
> [...]

Applied, thanks!

[1/5] power: supply: max1720x correct capacity computation
      commit: 58ae036172b5f051a19a32eba94a3e5eb37bf47e

Best regards,
-- 
Sebastian Reichel <sebastian.reichel@collabora.com>

Re: [PATCH v4 2/5] power: supply: add support for max77759 fuel gauge

[Thomas Antoine]

On 6/6/25 1:40 PM, Peter Griffin wrote:
> Hi Thomas,
> 
> Thanks for your patch and working to get fuel gauge functional on
> Pixel 6! I've tried to do quite an in-depth review comparing with the
> downstream driver.
Hi Peter,

Thank you very much for the in-depth review!

> On Fri, 23 May 2025 at 13:52, 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. A 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. The voltage must instead be read from V_CELL, the lowest voltage of
>> all cells. The mask to identify the chip is different. The computation of
>> the charge must also be changed to take into account TASKPERIOD, which
>> can add a factor 2 to the result.
>>
>> Add support for the MAX77759 by taking into account all of those
>> differences based on chip type.
>>
>> Do not advertise temp probes using the non-volatile memory as those are
>> not available.
>>
>> The regmap was proposed by André Draszik in
>>
>> Link: https://lore.kernel.org/all/d1bade77b5281c1de6b2ddcb4dbbd033e455a116.camel@linaro.org/
> 
> I think it would be worth noting in the commit message this is basic
> initial support for the M5 gauge in MAX77759, and things like loading
> & saving the m5 model aren't implemented yet.
> 
> That's important as some values such as the REPSOC register value used
> for POWER_SUPPLY_PROP_CAPACITY show the result after all processing
> including ModelGauge mixing etc, so these values won't be as accurate
> as downstream.

I will add that to the next version.

>>[...]
>> +static const enum power_supply_property max77759_battery_props[] = {
>> +       POWER_SUPPLY_PROP_PRESENT,
> 
> I checked the register values match downstream - this looks correct
> 
>> +       POWER_SUPPLY_PROP_CAPACITY,
> 
> I checked the register offset matchs downstream. The value reported
> varies a bit versus downstream. As mentioned above that is likely due
> to the REPSOC register reporting after mixing with the m5 model which
> is not loaded currently. Also the application specific values and cell
> characterization information used by the model isn't configured
> currently (see link below in _TEMP property below for the initial fuel
> gauge params used by downstream.
>

I have dumped the model written to my phone by a userdebug stock android.
If you think it is necessary, I can implement model loading where the
model is passed in the devicetree for next version.

>> +       POWER_SUPPLY_PROP_VOLTAGE_NOW,
> 
> I checked the register offset matchs downstream. Values reported look sensible.
> 
>> +       POWER_SUPPLY_PROP_CHARGE_FULL,
> 
> Downstream has a slightly different implementation than upstream for
> this property. See here
> https://android.googlesource.com/kernel/google-modules/bms/+/1a68c36bef474573cc8629cc1d121eb6a81ab68c/max1720x_battery.c#2244
>

Indeed, the main difference seems to be to use FULLCAPNOM instead of
FULLCAP. I will check out to see if both differ in value.

 
>> +       POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN,
> 
> I checked the register offset value is correct. However this is
> reporting 3000000 and downstream reports 4524000. I checked and it's
> just converting the register reset value of DESIGNCAP which is 0xbb8.
> 
> This is listed as a "application specific" value, so it maybe we just
> need to write the correct initial value to DESIGNCAP (see TEMP section
> below)
> 

The value  3000000 is the default value which will be set after a hardware
reset. I was able to extract the init sequence from a stock android.
It indeed writes the DESIGNCAP value. Here is a summary of the registers
written to upon a hardware reset. When (DTS) is written next to it,
it means that the value is exactly the same as given by the table in
maxim,cos-a1-2k at the link
https://android.googlesource.com/kernel/gs/+/refs/heads/android-gs-raviole-5.10-android15/arch/arm64/boot/dts/google/gs101-oriole-battery-data.dtsi

  "0x05 0x0000" #REPCAP
  "0x2a 0x0839" #RELAXCFG (DTS)
  "0x60 0x0080" #COMMAND UNLOCK_CFG
  "0x48 0x5722" #VFSOC0 (Value read from reg 0xff)
  "0x28 0x260e" #LEARNCFG (DTS)
  "0x1d 0x4217" #CONFIG (DTS)
  "0xbb 0x0090" #CONFIG2 (DTS)
  "0x13 0x5f00" #FULLSOCTHR (DTS)
  "0x35 0x08e8" #FULLCAPREP
  "0x18 0x08d6" #DESIGNCAP (DTS)
  "0x46 0x0c80" #DPACC
  "0x45 0x0094" #DQACC
  "0x00 0x0002" #STATUS
  "0x23 0x0905" #FULLCAPNOM
  "0x3a 0xa561" #VEMPTY (DTS)
  "0x12 0x2f80" #QRTABLE0 (DTS)
  "0x22 0x1400" #QRTABLE1 (DTS)
  "0x32 0x0680" #QRTABLE2 (DTS)
  "0x42 0x0580" #QRTABLE3 (DTS)
  "0x38 0x03bc" #RCOMP0
  "0x39 0x0c02" #TCOMPO
  "0x3c 0x2d00" #TASKPERIOD (DTS)
  "0x1e 0x05a0" #ICHGTERM (DTS)
  "0x2c 0xed51" #TGAIN (DTS)
  "0x2d 0x1eba" #TOFF (DTS)
  "0x2b 0x3870" #MISCCFG (DTS)
  "0x04 0x0000" #ATRATE (DTS)
  "0xb6 0x06c3" #CV_MIXCAP (value = 75% of FULLCAPNOM)
  "0xb7 0x0600" #CV_HALFTIME
  "0x49 0x2241" #CONVGCFG (DTS)
  "0x60 0x0000" #COMMAND LOCK_CFG
  "0xb9 0x0014" #CURVE (DTS)
  "0x29 0xc623" #FILTERCFG (DTS)
  "0x2e 0x0400" #CGAIN (hard coded)
  "0xbb 0x00b0" #CONFIG2 (DTS | 0x0020)
  "0x02 0x0780" #TALRTTH
  "0x00 0x0000" #STATUS
  "0x17 0x9320" #CYCLES

As can be seen, most values come directly from the devicetree but some
are not present in there or differ from the value given in the devicetree.

Without a similar init, charge and temperature will be non-functional
other values will most likely be wrong.
The fuel gauge stays powered through reboot so it doesn't reset even
when switching from android to linux, meaning that without any hardware
crash (e.g. empty batterry), the chip will look perfectly initialized.
A hardware reset of the fuel gauge can be forced by writing to
/proc/sysrq-trigger or by writing 0xf to 0x60.

I am unsure about what to do about this initalization, especially for values
which slightly differ from the devicetree. I think for next version, I
will have the same parameters be passed in the devicetree like android.
(except maybe IAvgEmpty which seems to be unused in the downsteam driver?)

>> +       POWER_SUPPLY_PROP_CHARGE_AVG,
> 
> This property isn't reported downstream. The value is changing and not
> just the reset value. I noticed REPSOC is an output of the ModelGauge
> algorithm so it is likely not to be completely accurate.
> 
>> +       POWER_SUPPLY_PROP_TEMP,
> 
> I checked the register offset value is correct. However the
> temperature is always being reported as the register reset value of
> 220. This is for obvious reasons quite an important one to report
> correctly.
> 
> I started debugging this a bit, and it is caused by an incorrectly
> configured CONFIG (0x1D) register. In particular the TEX[8] bit is 1
> on reset in this register which means temperature measurements are
> written from the host AP. When this bit is set to 0, measurements on
> the AIN pin are converted to a temperature value and stored in the
> Temperature register (I then saw values of 360 and the value
> changing).
> 
> See here for the bits in that CONFIG register
> https://android.googlesource.com/kernel/google-modules/bms/+/1a68c36bef474573cc8629cc1d121eb6a81ab68c/max_m5_reg.h#403
> 
> In downstream all these initial register settings are taken from DT
> here  https://android.googlesource.com/kernel/google-modules/raviole-device/+/refs/heads/android14-gs-pixel-6.1/arch/arm64/boot/dts/google/gs101-fake-battery-data.dtsi#27
> 
> For temperature when TEX=0, TGAIN, TOFF and TCURVE registers should
> also be configured to adjust the temperature measurement.
> 
> I think it would likely be worth initialising all the fuel gauge
> registers referenced in maxim,fg-params as that includes some of the
> application specific values for DESIGNCAP, also some of the cell
> characterization information, and hopefully we will get more accurate
> values from the fuel gauge generally.
>

See previous comment.

>> +       POWER_SUPPLY_PROP_CURRENT_NOW,
> 
> I checked the register offset matches downstream. Values reported look
> reasonable.
> 
>> +       POWER_SUPPLY_PROP_CURRENT_AVG,
> 
> I checked the register offset matches downstream. Values reported look
> reasonable.
> 
>> +       POWER_SUPPLY_PROP_MODEL_NAME,
> 
> This property isn't reported downstream.

Is this a problem?

>> [...]
>>  /*
>>   * Current and temperature is signed values, so unsigned regs
>>   * value must be converted to signed type
>> @@ -390,16 +507,36 @@ static int max1720x_battery_get_property(struct power_supply *psy,
>>                 val->intval = max172xx_percent_to_ps(reg_val);
>>                 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 (info->id == MAX1720X_ID) {
>> +                       ret = regmap_read(info->regmap, MAX172XX_BATT, &reg_val);
>> +                       val->intval = max172xx_voltage_to_ps(reg_val);
> 
> I think MAX1720X using MAX172XX_BATT register is likely a bug as the
> downstream driver uses MAX172XX_VCELL for that variant  see here
> https://android.googlesource.com/kernel/google-modules/bms/+/1a68c36bef474573cc8629cc1d121eb6a81ab68c/max1720x.h#304
>

Based on the comments from Sebastian Reichel, it seems that it is
downstream which is wrong:
https://lore.kernel.org/all/4cahu6dog7ly4ww6xyjmjigjfxs4m55mrnym2bjmzskscfvk34@guazy6wxbzfh/

> Having said that, if we do need to cope with differing register
> offsets for the different fuel gauge variants it would be nicer to
> abstract them in a way similar to the downstream driver. See here
> https://android.googlesource.com/kernel/google-modules/bms/+/1a68c36bef474573cc8629cc1d121eb6a81ab68c/max_m5.c#1235
> I think that would be more scalable in supporting multiple variants in
> one driver. Otherwise we will have an explosion of if(id==blah) else
> if (id==blah) in the driver.
>
> kind regards,
> 
> Peter


I completely agree about the need for abstraction if we want to keep both
chips in the same driver. I will try to implement that for next version.
Best regards,
Thomas

Re: [PATCH v4 2/5] power: supply: add support for max77759 fuel gauge

[Thomas Antoine]

Hi,

On 6/22/25 11:26 PM, Sebastian Reichel wrote:
> Hi,
> 
> On Fri, May 23, 2025 at 02:51:45PM +0200, 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. A 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. The voltage must instead be read from V_CELL, the lowest voltage of
>> all cells. The mask to identify the chip is different. The computation of
>> the charge must also be changed to take into account TASKPERIOD, which
>> can add a factor 2 to the result.
>>
>> Add support for the MAX77759 by taking into account all of those
>> differences based on chip type.
>>
>> Do not advertise temp probes using the non-volatile memory as those are
>> not available.
>>
>> The 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 | 265 ++++++++++++++++++++++++++++----
>>  1 file changed, 238 insertions(+), 27 deletions(-)
>>
>> diff --git a/drivers/power/supply/max1720x_battery.c b/drivers/power/supply/max1720x_battery.c
>> index 68b5314ecf3a234f906ec8fe400e586855b69cd9..c9ad452ada9d0a2a51f37d04fd8c3260be522405 100644
>> --- a/drivers/power/supply/max1720x_battery.c
>> +++ b/drivers/power/supply/max1720x_battery.c
>> @@ -37,6 +37,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 */
> [...]
>>  	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
> [...]
>> +			ret = regmap_read(info->regmap, MAX172XX_VCELL, &reg_val);
>> +			val->intval = max172xx_cell_voltage_to_ps(reg_val);
> 
> I haven't reviewed this fully due to all the feedback you already
> got from Peter Griffin and the DT binding being broken, but something
> that catched my eye:
> 
> POWER_SUPPLY_PROP_VOLTAGE_NOW provides the voltage of the whole
> battery and not of a single cell. E.g. a typical Li-Ion battery
> with two serial cells has a nominal voltage of roughly 7.4V while
> each cell has just 3.7V.
> 
> Greetings,
> 
> -- Sebastian

Downstream just reports this value which is usually a bit over 4V when I
record it.

Also from what I saw online, it seems that the battery does output
voltages around 3.86V as it is written on the battery:
https://cdn.shopify.com/s/files/1/0092/8133/9443/files/QeqxTLOL6eAp6OpZ.jpg?v=1728588266&width=2048

So I guess that there could only be a single cell in the battery? Which
would explain why they only report the lowest one.

It thus should be ok if we consider only the Google Pixel 6 (Pro). If we
need to take into account the possibility that the chip would be
used with other batteries, we could take from the devicetree the number
of cells and only provide the voltage if the number of cells is 1. Would
this be a good solution?

There is also a VSYS register (0xb1) but I couldn't find anything about it.
Taking a LSB of 156uV (twice the one of VCell), I see a clear correlation
with VCell, except for it being very slightly lower.
As it is mostly guesswork, I don't think it would be a good idea to use this
without any confirmation it is correct.

Best regards,
Thomas

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

[Thomas Antoine]

Hi,

On 6/5/25 3:00 PM, Peter Griffin wrote:
> Hi Thomas,
> 
> Thanks for your patch and work to enable fuel gauge on Pixel 6!
> 
> On Fri, 23 May 2025 at 13:52, Thomas Antoine via B4 Relay
> <devnull+t.antoine.uclouvain.be@kernel.org> wrote:
>>
>> From: Thomas Antoine <t.antoine@uclouvain.be>
>>
>> Add the node for the Maxim MAX77759 fuel gauge as a slave of the i2c.
>>
>> The TODO is still applicable given there are other slaves on the
>> bus (e.g. PCA9468, other MAX77759 functions and the MAX20339 OVP).
>>
>> Signed-off-by: Thomas Antoine <t.antoine@uclouvain.be>
>> ---
>>  arch/arm64/boot/dts/exynos/google/gs101-pixel-common.dtsi | 10 ++++++++++
>>  1 file changed, 10 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/exynos/google/gs101-pixel-common.dtsi b/arch/arm64/boot/dts/exynos/google/gs101-pixel-common.dtsi
>> index b25230495c64dce60916b7cd5dcb9a7cce5d0e4e..84fc10c3562958ab1621f24644709e85a9433b9b 100644
>> --- a/arch/arm64/boot/dts/exynos/google/gs101-pixel-common.dtsi
>> +++ b/arch/arm64/boot/dts/exynos/google/gs101-pixel-common.dtsi
>> @@ -10,6 +10,7 @@
>>
>>  #include <dt-bindings/gpio/gpio.h>
>>  #include <dt-bindings/input/input.h>
>> +#include <dt-bindings/interrupt-controller/irq.h>
>>  #include <dt-bindings/usb/pd.h>
>>  #include "gs101-pinctrl.h"
>>  #include "gs101.dtsi"
>> @@ -188,6 +189,15 @@ usbc0_role_sw: endpoint {
>>                         };
>>                 };
>>         };
>> +
>> +       fuel-gauge@36 {
>> +               compatible = "maxim,max77759-fg";
>> +               reg = <0x36>;
>> +               reg-names = "m5";
>> +               interrupt-parent = <&gpa9>;
>> +               interrupts = <3 IRQ_TYPE_LEVEL_LOW>;
>> +               shunt-resistor-micro-ohms = <5000>;
>> +       };
>>  };
>>
> 
> If gpa-9-3 is being used for the interrupt I think we should also add
> the pinctrl configuration for it. Taking a look at downstream the pin
> is defined as
> 
> &pinctrl_0 {
> /* [MAX77759: FG_INTB] > FG_INT_L > [XEINT_23 : SC59845XWE] */
>         if_pmic_fg_irq: if-pmic-fg-irq {
>                 samsung,pins = "gpa9-3"; /* XEINT_23 */
>                 samsung,pin-function = <EXYNOS_PIN_FUNC_EINT>;
>                 samsung,pin-pud = <EXYNOS_PIN_PULL_UP>;
>                 samsung,pin-drv = <GS101_PIN_DRV_2_5_MA>;
>         };
> };
> 
> and then the fuel-gauge node declares
> 
> /* FG_INT_L -> XEINT_23 */
> pinctrl-names = "default";
> pinctrl-0 = <&if_pmic_fg_irq>;
> 
> regards,
> 
> Peter


I will add this to the next version, thanks for the
catch.

Best regards,
Thomas

Re: [PATCH v4 2/5] power: supply: add support for max77759 fuel gauge

[Pavel Machek]

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

Hi!

> > Other differences include the lack of V_BATT register to read the battery
> > level. The voltage must instead be read from V_CELL, the lowest voltage of
> > all cells. The mask to identify the chip is different. The computation of
> > the charge must also be changed to take into account TASKPERIOD, which
> > can add a factor 2 to the result.
> > 
> > Add support for the MAX77759 by taking into account all of those
> > differences based on chip type.
> > 
> > Do not advertise temp probes using the non-volatile memory as those are
> > not available.
> > 
> > The 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 | 265 ++++++++++++++++++++++++++++----
> >  1 file changed, 238 insertions(+), 27 deletions(-)
> > 
> > diff --git a/drivers/power/supply/max1720x_battery.c b/drivers/power/supply/max1720x_battery.c
> > index 68b5314ecf3a234f906ec8fe400e586855b69cd9..c9ad452ada9d0a2a51f37d04fd8c3260be522405 100644
> > --- a/drivers/power/supply/max1720x_battery.c
> > +++ b/drivers/power/supply/max1720x_battery.c
> > @@ -37,6 +37,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 */
> [...]
> >  	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
> [...]
> > +			ret = regmap_read(info->regmap, MAX172XX_VCELL, &reg_val);
> > +			val->intval = max172xx_cell_voltage_to_ps(reg_val);
> 
> I haven't reviewed this fully due to all the feedback you already
> got from Peter Griffin and the DT binding being broken, but something
> that catched my eye:
> 
> POWER_SUPPLY_PROP_VOLTAGE_NOW provides the voltage of the whole
> battery and not of a single cell. E.g. a typical Li-Ion battery
> with two serial cells has a nominal voltage of roughly 7.4V while
> each cell has just 3.7V.

Phones normally only have one cell...

Best regards,
								Pavel
-- 
I don't work for Nazis and criminals, and neither should you.
Boycott Putin, Trump, and Musk!

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

Re: [PATCH v4 2/5] power: supply: add support for max77759 fuel gauge

[Sebastian Reichel]

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

Hi,

On Thu, Jul 03, 2025 at 10:12:28PM +0200, Pavel Machek wrote:
> > > Other differences include the lack of V_BATT register to read the battery
> > > level. The voltage must instead be read from V_CELL, the lowest voltage of
> > > all cells. The mask to identify the chip is different. The computation of
> > > the charge must also be changed to take into account TASKPERIOD, which
> > > can add a factor 2 to the result.
> > > 
> > > Add support for the MAX77759 by taking into account all of those
> > > differences based on chip type.
> > > 
> > > Do not advertise temp probes using the non-volatile memory as those are
> > > not available.
> > > 
> > > The 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 | 265 ++++++++++++++++++++++++++++----
> > >  1 file changed, 238 insertions(+), 27 deletions(-)
> > > 
> > > diff --git a/drivers/power/supply/max1720x_battery.c b/drivers/power/supply/max1720x_battery.c
> > > index 68b5314ecf3a234f906ec8fe400e586855b69cd9..c9ad452ada9d0a2a51f37d04fd8c3260be522405 100644
> > > --- a/drivers/power/supply/max1720x_battery.c
> > > +++ b/drivers/power/supply/max1720x_battery.c
> > > @@ -37,6 +37,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 */
> > [...]
> > >  	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
> > [...]
> > > +			ret = regmap_read(info->regmap, MAX172XX_VCELL, &reg_val);
> > > +			val->intval = max172xx_cell_voltage_to_ps(reg_val);
> > 
> > I haven't reviewed this fully due to all the feedback you already
> > got from Peter Griffin and the DT binding being broken, but something
> > that catched my eye:
> > 
> > POWER_SUPPLY_PROP_VOLTAGE_NOW provides the voltage of the whole
> > battery and not of a single cell. E.g. a typical Li-Ion battery
> > with two serial cells has a nominal voltage of roughly 7.4V while
> > each cell has just 3.7V.
> 
> Phones normally only have one cell...

... and chips often support more and are used in all kind of devices.

(Btw. some new high-end phones start to have 1S2P configuration)

Greetings,

-- Sebastian

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

Re: [PATCH v4 2/5] power: supply: add support for max77759 fuel gauge

[Sebastian Reichel]

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

Hi,

On Wed, Jun 25, 2025 at 02:12:21PM +0200, Thomas Antoine wrote:
> > On Fri, May 23, 2025 at 02:51:45PM +0200, 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. A 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. The voltage must instead be read from V_CELL, the lowest voltage of
> >> all cells. The mask to identify the chip is different. The computation of
> >> the charge must also be changed to take into account TASKPERIOD, which
> >> can add a factor 2 to the result.
> >>
> >> Add support for the MAX77759 by taking into account all of those
> >> differences based on chip type.
> >>
> >> Do not advertise temp probes using the non-volatile memory as those are
> >> not available.
> >>
> >> The 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 | 265 ++++++++++++++++++++++++++++----
> >>  1 file changed, 238 insertions(+), 27 deletions(-)
> >>
> >> diff --git a/drivers/power/supply/max1720x_battery.c b/drivers/power/supply/max1720x_battery.c
> >> index 68b5314ecf3a234f906ec8fe400e586855b69cd9..c9ad452ada9d0a2a51f37d04fd8c3260be522405 100644
> >> --- a/drivers/power/supply/max1720x_battery.c
> >> +++ b/drivers/power/supply/max1720x_battery.c
> >> @@ -37,6 +37,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 */
> > [...]
> >>  	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
> > [...]
> >> +			ret = regmap_read(info->regmap, MAX172XX_VCELL, &reg_val);
> >> +			val->intval = max172xx_cell_voltage_to_ps(reg_val);
> > 
> > I haven't reviewed this fully due to all the feedback you already
> > got from Peter Griffin and the DT binding being broken, but something
> > that catched my eye:
> > 
> > POWER_SUPPLY_PROP_VOLTAGE_NOW provides the voltage of the whole
> > battery and not of a single cell. E.g. a typical Li-Ion battery
> > with two serial cells has a nominal voltage of roughly 7.4V while
> > each cell has just 3.7V.
> > 
> > Greetings,
> > 
> > -- Sebastian
> 
> Downstream just reports this value which is usually a bit over 4V when I
> record it.
> 
> Also from what I saw online, it seems that the battery does output
> voltages around 3.86V as it is written on the battery:
> https://cdn.shopify.com/s/files/1/0092/8133/9443/files/QeqxTLOL6eAp6OpZ.jpg?v=1728588266&width=2048
> 
> So I guess that there could only be a single cell in the battery? Which
> would explain why they only report the lowest one.
> 
> It thus should be ok if we consider only the Google Pixel 6 (Pro). If we
> need to take into account the possibility that the chip would be
> used with other batteries, we could take from the devicetree the number
> of cells and only provide the voltage if the number of cells is 1. Would
> this be a good solution?

That's fine with me. But getting the DT binding prepared will take
some time (the DT property should definetly go via
Documentation/devicetree/bindings/power/supply/battery.yaml), we
had some prior discussions around this IIRC. It might be sensible
to move adding VOLTAGE_NOW support to an extra series.

> There is also a VSYS register (0xb1) but I couldn't find anything
> about it. Taking a LSB of 156uV (twice the one of VCell), I see a
> clear correlation with VCell, except for it being very slightly
> lower. As it is mostly guesswork, I don't think it would be a good
> idea to use this without any confirmation it is correct.

I see the datasheet does not have the register listed for < 0x10.
Have you tried contacting the original Google driver author to shed
some light to this?

Greetings,

-- Sebastian

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

Re: [PATCH v4 2/5] power: supply: add support for max77759 fuel gauge

[Peter Griffin]

Hi Thomas,

On Tue, 24 Jun 2025 at 16:45, Thomas Antoine <t.antoine@uclouvain.be> wrote:
>
>
>
> On 6/6/25 1:40 PM, Peter Griffin wrote:
> > Hi Thomas,
> >
> > Thanks for your patch and working to get fuel gauge functional on
> > Pixel 6! I've tried to do quite an in-depth review comparing with the
> > downstream driver.
> Hi Peter,
>
> Thank you very much for the in-depth review!
>
> > On Fri, 23 May 2025 at 13:52, 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. A 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. The voltage must instead be read from V_CELL, the lowest voltage of
> >> all cells. The mask to identify the chip is different. The computation of
> >> the charge must also be changed to take into account TASKPERIOD, which
> >> can add a factor 2 to the result.
> >>
> >> Add support for the MAX77759 by taking into account all of those
> >> differences based on chip type.
> >>
> >> Do not advertise temp probes using the non-volatile memory as those are
> >> not available.
> >>
> >> The regmap was proposed by André Draszik in
> >>
> >> Link: https://lore.kernel.org/all/d1bade77b5281c1de6b2ddcb4dbbd033e455a116.camel@linaro.org/
> >
> > I think it would be worth noting in the commit message this is basic
> > initial support for the M5 gauge in MAX77759, and things like loading
> > & saving the m5 model aren't implemented yet.
> >
> > That's important as some values such as the REPSOC register value used
> > for POWER_SUPPLY_PROP_CAPACITY show the result after all processing
> > including ModelGauge mixing etc, so these values won't be as accurate
> > as downstream.
>
> I will add that to the next version.

Ok great :)

>
> >>[...]
> >> +static const enum power_supply_property max77759_battery_props[] = {
> >> +       POWER_SUPPLY_PROP_PRESENT,
> >
> > I checked the register values match downstream - this looks correct
> >
> >> +       POWER_SUPPLY_PROP_CAPACITY,
> >
> > I checked the register offset matchs downstream. The value reported
> > varies a bit versus downstream. As mentioned above that is likely due
> > to the REPSOC register reporting after mixing with the m5 model which
> > is not loaded currently. Also the application specific values and cell
> > characterization information used by the model isn't configured
> > currently (see link below in _TEMP property below for the initial fuel
> > gauge params used by downstream.
> >
>
> I have dumped the model written to my phone by a userdebug stock android.
> If you think it is necessary, I can implement model loading where the
> model is passed in the devicetree for next version.

See comment a bit further down about using a dedicated compatible.
Also I think it's probably worth thinking about it conceptually as 3
things
1) Ensuring the application specific values and initial registers are
initialized like downstream does from DT (DesignCap, CONFIG register
etc)
2) Loading the M5 model gauge algorithm
3) Saving and restoring the "algorithm configuration" values to nvmem
and re-loading them on a fresh boot.

Most of the code for these features is in the following file
https://android.googlesource.com/kernel/google-modules/bms/+/1a68c36bef474573cc8629cc1d121eb6a81ab68c/max_m5.c
downstream.

>
> >> +       POWER_SUPPLY_PROP_VOLTAGE_NOW,
> >
> > I checked the register offset matchs downstream. Values reported look sensible.
> >
> >> +       POWER_SUPPLY_PROP_CHARGE_FULL,
> >
> > Downstream has a slightly different implementation than upstream for
> > this property. See here
> > https://android.googlesource.com/kernel/google-modules/bms/+/1a68c36bef474573cc8629cc1d121eb6a81ab68c/max1720x_battery.c#2244
> >
>
> Indeed, the main difference seems to be to use FULLCAPNOM instead of
> FULLCAP. I will check out to see if both differ in value.
>
>
> >> +       POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN,
> >
> > I checked the register offset value is correct. However this is
> > reporting 3000000 and downstream reports 4524000. I checked and it's
> > just converting the register reset value of DESIGNCAP which is 0xbb8.
> >
> > This is listed as a "application specific" value, so it maybe we just
> > need to write the correct initial value to DESIGNCAP (see TEMP section
> > below)
> >
>
> The value  3000000 is the default value which will be set after a hardware
> reset. I was able to extract the init sequence from a stock android.
> It indeed writes the DESIGNCAP value. Here is a summary of the registers
> written to upon a hardware reset. When (DTS) is written next to it,
> it means that the value is exactly the same as given by the table in
> maxim,cos-a1-2k at the link
> https://android.googlesource.com/kernel/gs/+/refs/heads/android-gs-raviole-5.10-android15/arch/arm64/boot/dts/google/gs101-oriole-battery-data.dtsi
>
>   "0x05 0x0000" #REPCAP
>   "0x2a 0x0839" #RELAXCFG (DTS)
>   "0x60 0x0080" #COMMAND UNLOCK_CFG
>   "0x48 0x5722" #VFSOC0 (Value read from reg 0xff)
>   "0x28 0x260e" #LEARNCFG (DTS)
>   "0x1d 0x4217" #CONFIG (DTS)
>   "0xbb 0x0090" #CONFIG2 (DTS)
>   "0x13 0x5f00" #FULLSOCTHR (DTS)
>   "0x35 0x08e8" #FULLCAPREP
>   "0x18 0x08d6" #DESIGNCAP (DTS)
>   "0x46 0x0c80" #DPACC
>   "0x45 0x0094" #DQACC
>   "0x00 0x0002" #STATUS
>   "0x23 0x0905" #FULLCAPNOM
>   "0x3a 0xa561" #VEMPTY (DTS)
>   "0x12 0x2f80" #QRTABLE0 (DTS)
>   "0x22 0x1400" #QRTABLE1 (DTS)
>   "0x32 0x0680" #QRTABLE2 (DTS)
>   "0x42 0x0580" #QRTABLE3 (DTS)
>   "0x38 0x03bc" #RCOMP0
>   "0x39 0x0c02" #TCOMPO
>   "0x3c 0x2d00" #TASKPERIOD (DTS)
>   "0x1e 0x05a0" #ICHGTERM (DTS)
>   "0x2c 0xed51" #TGAIN (DTS)
>   "0x2d 0x1eba" #TOFF (DTS)
>   "0x2b 0x3870" #MISCCFG (DTS)
>   "0x04 0x0000" #ATRATE (DTS)
>   "0xb6 0x06c3" #CV_MIXCAP (value = 75% of FULLCAPNOM)
>   "0xb7 0x0600" #CV_HALFTIME
>   "0x49 0x2241" #CONVGCFG (DTS)
>   "0x60 0x0000" #COMMAND LOCK_CFG
>   "0xb9 0x0014" #CURVE (DTS)
>   "0x29 0xc623" #FILTERCFG (DTS)
>   "0x2e 0x0400" #CGAIN (hard coded)
>   "0xbb 0x00b0" #CONFIG2 (DTS | 0x0020)

Bit 5 is "LdMdl" see
https://android.googlesource.com/kernel/google-modules/bms/+/1a68c36bef474573cc8629cc1d121eb6a81ab68c/max_m5_reg.h#1489
It is set to 1 to initiate loading a model, and firmware clears the
bit to zero to indicate model loading is finished. You can see more
info about how this works
https://android.googlesource.com/kernel/google-modules/bms/+/1a68c36bef474573cc8629cc1d121eb6a81ab68c/max_m5.c#459

>   "0x02 0x0780" #TALRTTH
>   "0x00 0x0000" #STATUS
>   "0x17 0x9320" #CYCLES
>
> As can be seen, most values come directly from the devicetree but some
> are not present in there or differ from the value given in the devicetree.

Some of those registers like RepCap, Cycles, FullCapNom are ModelGauge
algorithm outputs.

Others like DQACC, DPACC, RCOMP0 are part of the values that should be
saved and restored to some nvmem. You can see the function here that
does this https://android.googlesource.com/kernel/google-modules/bms/+/1a68c36bef474573cc8629cc1d121eb6a81ab68c/max_m5.c#657

>
> Without a similar init, charge and temperature will be non-functional
> other values will most likely be wrong.
> The fuel gauge stays powered through reboot so it doesn't reset even
> when switching from android to linux, meaning that without any hardware
> crash (e.g. empty batterry), the chip will look perfectly initialized.
> A hardware reset of the fuel gauge can be forced by writing to
> /proc/sysrq-trigger or by writing 0xf to 0x60.

Ah that explains it then, as Andre mentioned he thought this reported
a correct value in a previous version. It might be worth adding that
info in the cover letter of future versions.

>
> I am unsure about what to do about this initalization, especially for values
> which slightly differ from the devicetree. I think for next version, I
> will have the same parameters be passed in the devicetree like android.

We don't really pass register values like the downstream driver is
doing in the device tree. I think you will likely need to add a
max77759-gs101-oriole compatible to the driver and then have the
application specific values, and m5 gauge model algorithm as static
info in the driver applied from the dedicated compatible. It would
also be worth checking whether any more of those register values can
be represented by the standard power-supply binding properties that
already exist.

> (except maybe IAvgEmpty which seems to be unused in the downsteam driver?)
>
> >> +       POWER_SUPPLY_PROP_CHARGE_AVG,
> >
> > This property isn't reported downstream. The value is changing and not
> > just the reset value. I noticed REPSOC is an output of the ModelGauge
> > algorithm so it is likely not to be completely accurate.
> >
> >> +       POWER_SUPPLY_PROP_TEMP,
> >
> > I checked the register offset value is correct. However the
> > temperature is always being reported as the register reset value of
> > 220. This is for obvious reasons quite an important one to report
> > correctly.
> >
> > I started debugging this a bit, and it is caused by an incorrectly
> > configured CONFIG (0x1D) register. In particular the TEX[8] bit is 1
> > on reset in this register which means temperature measurements are
> > written from the host AP. When this bit is set to 0, measurements on
> > the AIN pin are converted to a temperature value and stored in the
> > Temperature register (I then saw values of 360 and the value
> > changing).
> >
> > See here for the bits in that CONFIG register
> > https://android.googlesource.com/kernel/google-modules/bms/+/1a68c36bef474573cc8629cc1d121eb6a81ab68c/max_m5_reg.h#403
> >
> > In downstream all these initial register settings are taken from DT
> > here  https://android.googlesource.com/kernel/google-modules/raviole-device/+/refs/heads/android14-gs-pixel-6.1/arch/arm64/boot/dts/google/gs101-fake-battery-data.dtsi#27
> >
> > For temperature when TEX=0, TGAIN, TOFF and TCURVE registers should
> > also be configured to adjust the temperature measurement.
> >
> > I think it would likely be worth initialising all the fuel gauge
> > registers referenced in maxim,fg-params as that includes some of the
> > application specific values for DESIGNCAP, also some of the cell
> > characterization information, and hopefully we will get more accurate
> > values from the fuel gauge generally.
> >
>
> See previous comment.
>
> >> +       POWER_SUPPLY_PROP_CURRENT_NOW,
> >
> > I checked the register offset matches downstream. Values reported look
> > reasonable.
> >
> >> +       POWER_SUPPLY_PROP_CURRENT_AVG,
> >
> > I checked the register offset matches downstream. Values reported look
> > reasonable.
> >
> >> +       POWER_SUPPLY_PROP_MODEL_NAME,
> >
> > This property isn't reported downstream.
>
> Is this a problem?

Nope, that was more for my own benefit when doing the review :)

Thanks,

Peter


>
> >> [...]
> >>  /*
> >>   * Current and temperature is signed values, so unsigned regs
> >>   * value must be converted to signed type
> >> @@ -390,16 +507,36 @@ static int max1720x_battery_get_property(struct power_supply *psy,
> >>                 val->intval = max172xx_percent_to_ps(reg_val);
> >>                 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 (info->id == MAX1720X_ID) {
> >> +                       ret = regmap_read(info->regmap, MAX172XX_BATT, &reg_val);
> >> +                       val->intval = max172xx_voltage_to_ps(reg_val);
> >
> > I think MAX1720X using MAX172XX_BATT register is likely a bug as the
> > downstream driver uses MAX172XX_VCELL for that variant  see here
> > https://android.googlesource.com/kernel/google-modules/bms/+/1a68c36bef474573cc8629cc1d121eb6a81ab68c/max1720x.h#304
> >
>
> Based on the comments from Sebastian Reichel, it seems that it is
> downstream which is wrong:
> https://lore.kernel.org/all/4cahu6dog7ly4ww6xyjmjigjfxs4m55mrnym2bjmzskscfvk34@guazy6wxbzfh/
>
> > Having said that, if we do need to cope with differing register
> > offsets for the different fuel gauge variants it would be nicer to
> > abstract them in a way similar to the downstream driver. See here
> > https://android.googlesource.com/kernel/google-modules/bms/+/1a68c36bef474573cc8629cc1d121eb6a81ab68c/max_m5.c#1235
> > I think that would be more scalable in supporting multiple variants in
> > one driver. Otherwise we will have an explosion of if(id==blah) else
> > if (id==blah) in the driver.
> >
> > kind regards,
> >
> > Peter
>
>
> I completely agree about the need for abstraction if we want to keep both
> chips in the same driver. I will try to implement that for next version.
> Best regards,
> Thomas

Re: [PATCH v4 2/5] power: supply: add support for max77759 fuel gauge

[André Draszik]

Hi,

On Mon, 2025-07-07 at 08:16 +0100, Peter Griffin wrote:
> Hi Thomas,
> 
> On Tue, 24 Jun 2025 at 16:45, Thomas Antoine <t.antoine@uclouvain.be> wrote:
> 
> > I am unsure about what to do about this initalization, especially for values
> > which slightly differ from the devicetree. I think for next version, I
> > will have the same parameters be passed in the devicetree like android.
> 
> We don't really pass register values like the downstream driver is
> doing in the device tree. I think you will likely need to add a
> max77759-gs101-oriole compatible to the driver and then have the
> application specific values, and m5 gauge model algorithm as static
> info in the driver applied from the dedicated compatible. It would
> also be worth checking whether any more of those register values can
> be represented by the standard power-supply binding properties that
> already exist.

I believe these are likely battery specific values, and were obtained during
battery characterization by the vendor (or Maxim). They can change (with a
different battery supplier etc, hence I don't think basing this on a
max77759-gs101-oriole would be correct here.

As we learned from the Pixel 6a battery updates, the same phone may use
batteries (e.g. from different suppliers).

Either it needs to know about the specific battery model, or the values
should be passed from DT in some way.

Cheers,
Andre'

Re: [PATCH v4 2/5] power: supply: add support for max77759 fuel gauge

[Peter Griffin]

Hi André

On Mon, 7 Jul 2025 at 09:04, André Draszik <andre.draszik@linaro.org> wrote:
>
> Hi,
>
> On Mon, 2025-07-07 at 08:16 +0100, Peter Griffin wrote:
> > Hi Thomas,
> >
> > On Tue, 24 Jun 2025 at 16:45, Thomas Antoine <t.antoine@uclouvain.be> wrote:
> >
> > > I am unsure about what to do about this initalization, especially for values
> > > which slightly differ from the devicetree. I think for next version, I
> > > will have the same parameters be passed in the devicetree like android.
> >
> > We don't really pass register values like the downstream driver is
> > doing in the device tree. I think you will likely need to add a
> > max77759-gs101-oriole compatible to the driver and then have the
> > application specific values, and m5 gauge model algorithm as static
> > info in the driver applied from the dedicated compatible. It would
> > also be worth checking whether any more of those register values can
> > be represented by the standard power-supply binding properties that
> > already exist.
>
> I believe these are likely battery specific values, and were obtained during
> battery characterization by the vendor (or Maxim). They can change (with a
> different battery supplier etc, hence I don't think basing this on a
> max77759-gs101-oriole would be correct here.
>
> As we learned from the Pixel 6a battery updates, the same phone may use
> batteries (e.g. from different suppliers).
>
> Either it needs to know about the specific battery model, or the values
> should be passed from DT in some way.

Some of the fg-params values are characterization values like
QResidual, Rcomp0 and others. I hadn't really considered different
batteries for the same phone model. Given that information I think one
would need to investigate in more detail how downstream chooses what
battery it has and what set of values to use.

Thanks,

Peter