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

[PATCH v3 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 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:
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
ok 7 max77759-fg.sysfs.present
ok 8 max77759-fg.sysfs.status # SKIP
ok 9 max77759-fg.sysfs.capacity
ok 10 max77759-fg.sysfs.capacity_level # SKIP
ok 11 max77759-fg.sysfs.model_name
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(Dimitri Fedrau)
ok 18 max77759-fg.sysfs.input_voltage_limit # SKIP
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
ok 24 max77759-fg.sysfs.current_now
ok 25 max77759-fg.sysfs.current_max # SKIP
ok 26 max77759-fg.sysfs.charge_now # SKIP
ok 27 max77759-fg.sysfs.charge_full
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: 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      |  34 ++-
 .../boot/dts/exynos/google/gs101-pixel-common.dtsi |  10 +
 arch/arm64/configs/defconfig                       |   1 +
 drivers/power/supply/max1720x_battery.c            | 271 ++++++++++++++++++---
 4 files changed, 280 insertions(+), 36 deletions(-)
---
base-commit: e48e99b6edf41c69c5528aa7ffb2daf3c59ee105
change-id: 20241202-b4-gs101_max77759_fg-402e231a4b33

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

[PATCH v3 1/5] power: supply: 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 | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/power/supply/max1720x_battery.c b/drivers/power/supply/max1720x_battery.c
index ea3912fd1de8bfd0d029c16f276316d06e1b105c..cca5f8b5071fb731f9b60420239ea03d46cb1bf3 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 */
 }
 
 /*

-- 
2.49.0

[PATCH v3 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 | 270 ++++++++++++++++++++++++++++----
 1 file changed, 237 insertions(+), 33 deletions(-)

diff --git a/drivers/power/supply/max1720x_battery.c b/drivers/power/supply/max1720x_battery.c
index cca5f8b5071fb731f9b60420239ea03d46cb1bf3..969d3a7c2baa7e1d23c5175942d975b277c8554c 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 */
@@ -50,19 +51,32 @@
 #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 MAX77759_TASKPERIOD		0x3C
+#define MAX77759_TASKPERIOD_175MS	0x1680
+#define MAX77759_TASKPERIOD_351MS	0x2D00
 #define MAX172XX_BATT			0xDA	/* Battery voltage */
 #define MAX172XX_ATAVCAP		0xDF
 
 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 charge_full_design;
+	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,10 +376,36 @@ 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)
+				   struct max1720x_device_info *info,
+				   int *intval)
 {
-	return reg * (500000 / info->rsense);	/* in uAh */
+	int lsb = 1;
+	int reg_val;
+	int ret;
+
+	if (info->id == MAX77759_ID) {
+		ret = regmap_read(info->regmap, MAX77759_TASKPERIOD, &reg_val);
+		if (ret)
+			return ret;
+
+		switch (reg_val) {
+		case MAX77759_TASKPERIOD_175MS:
+			break;
+		case MAX77759_TASKPERIOD_351MS:
+			lsb = 2;
+			break;
+		default:
+			return -ENODEV;
+		}
+	}
+	*intval = reg * (500000 / info->rsense) * lsb;	/* in uAh */
+	return 0;
 }
 
 /*
@@ -306,6 +420,28 @@ static int max172xx_temperature_to_ps(unsigned int reg)
 	return val * 10 / 256; /* in tenths of deg. C */
 }
 
+static const char *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)
+			return max17201_model;
+		else if (reg_val == MAX172XX_DEV_NAME_TYPE_MAX17205)
+			return max17205_model;
+		return NULL;
+	case MAX77759_ID:
+		reg_val = FIELD_GET(MAX77759_DEV_NAME_TYPE_MASK, reg_val);
+		if (reg_val == MAX77759_DEV_NAME_TYPE_MAX77759)
+			return max77759_model;
+		return NULL;
+	default:
+		return NULL;
+	}
+}
+
 /*
  * Calculating current registers resolution:
  *
@@ -390,19 +526,31 @@ 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);
-		val->intval = max172xx_capacity_to_ps(reg_val);
+		if (ret)
+			break;
+		ret = max172xx_capacity_to_ps(reg_val, info, &val->intval);
 		break;
 	case POWER_SUPPLY_PROP_CHARGE_AVG:
 		ret = regmap_read(info->regmap, MAX172XX_REPCAP, &reg_val);
-		val->intval = max172xx_capacity_to_ps(reg_val);
+		if (ret)
+			break;
+
+		ret = max172xx_capacity_to_ps(reg_val, info, &val->intval);
 		break;
 	case POWER_SUPPLY_PROP_TIME_TO_EMPTY_AVG:
 		ret = regmap_read(info->regmap, MAX172XX_TTE, &reg_val);
+		pr_info("RAW TTE: %d", reg_val);
 		val->intval = max172xx_time_to_ps(reg_val);
 		break;
 	case POWER_SUPPLY_PROP_TIME_TO_FULL_AVG:
@@ -423,17 +571,15 @@ 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);
+		if (ret)
+			break;
+		ret = max172xx_capacity_to_ps(reg_val, info, &val->intval);
 		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;
+		val->strval = max1720x_devname_to_model(reg_val, val, info);
+		if (!val->strval)
+			ret = -ENODEV;
 		break;
 	case POWER_SUPPLY_PROP_MANUFACTURER:
 		val->strval = max1720x_manufacturer;
@@ -527,7 +673,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 +694,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 +703,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 = 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,
@@ -579,32 +744,70 @@ 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 = of_property_read_u32(dev->of_node,
+				   "charge-full-design-microamp-hours", &info->charge_full_design);
+	if (ret)
+		info->charge_full_design = 0;
+
+	ret = max1720x_get_rsense(dev, info, data);
 	if (ret)
-		return dev_err_probe(dev, ret, "Failed to probe nvmem\n");
+		return dev_err_probe(dev, ret, "Failed to get RSense\n");
 
-	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 +816,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 v3 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.

Signed-off-by: Thomas Antoine <t.antoine@uclouvain.be>
---
 .../bindings/power/supply/maxim,max17201.yaml      | 34 ++++++++++++++++++++--
 1 file changed, 31 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..4823021ff16b170db83abd0b974986a307c05089 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
@@ -39,6 +37,36 @@ required:
   - reg
   - reg-names
 
+allOf:
+  - $ref: power-supply.yaml#
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - maxim,max17201
+    then:
+      properties:
+        reg:
+          minItems: 2
+          maxItems: 2
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - maxim,max77759-fg
+    then:
+      properties:
+        reg:
+          items:
+            minItems: 1
+            maxItems: 1
+        shunt-resistor-micro-ohms:
+          description: The value of current sense resistor in microohms.
+      required:
+        - shunt-resistor-micro-ohms
+
 unevaluatedProperties: false
 
 examples:

-- 
2.49.0

[PATCH v3 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 v3 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 v3 0/5] Google Pixel 6 (oriole): max77759 fuel gauge enablement and driver support

[Rob Herring (Arm)]

On Mon, 21 Apr 2025 20:13:31 +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.
> 
> 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 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:
> 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
> ok 7 max77759-fg.sysfs.present
> ok 8 max77759-fg.sysfs.status # SKIP
> ok 9 max77759-fg.sysfs.capacity
> ok 10 max77759-fg.sysfs.capacity_level # SKIP
> ok 11 max77759-fg.sysfs.model_name
> 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(Dimitri Fedrau)
> ok 18 max77759-fg.sysfs.input_voltage_limit # SKIP
> 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
> ok 24 max77759-fg.sysfs.current_now
> ok 25 max77759-fg.sysfs.current_max # SKIP
> ok 26 max77759-fg.sysfs.charge_now # SKIP
> ok 27 max77759-fg.sysfs.charge_full
> 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: 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      |  34 ++-
>  .../boot/dts/exynos/google/gs101-pixel-common.dtsi |  10 +
>  arch/arm64/configs/defconfig                       |   1 +
>  drivers/power/supply/max1720x_battery.c            | 271 ++++++++++++++++++---
>  4 files changed, 280 insertions(+), 36 deletions(-)
> ---
> base-commit: e48e99b6edf41c69c5528aa7ffb2daf3c59ee105
> change-id: 20241202-b4-gs101_max77759_fg-402e231a4b33
> 
> Best regards,
> --
> Thomas Antoine <t.antoine@uclouvain.be>
> 
> 
> 


My bot found new DTB warnings on the .dts files added or changed in this
series.

Some warnings may be from an existing SoC .dtsi. Or perhaps the warnings
are fixed by another series. Ultimately, it is up to the platform
maintainer whether these warnings are acceptable or not. No need to reply
unless the platform maintainer has comments.

If you already ran DT checks and didn't see these error(s), then
make sure dt-schema is up to date:

  pip3 install dtschema --upgrade


This patch series was applied (using b4) to base:
 Base: using specified base-commit e48e99b6edf41c69c5528aa7ffb2daf3c59ee105

If this is not the correct base, please add 'base-commit' tag
(or use b4 which does this automatically)

New warnings running 'make CHECK_DTBS=y for arch/arm64/boot/dts/exynos/' for 20250421-b4-gs101_max77759_fg-v3-0-50cd8caf9017@uclouvain.be:

arch/arm64/boot/dts/exynos/google/gs101-raven.dtb: fuel-gauge@36 (maxim,max77759-fg): reg: [[54]] is too short
	from schema $id: http://devicetree.org/schemas/power/supply/maxim,max17201.yaml#
arch/arm64/boot/dts/exynos/google/gs101-raven.dtb: fuel-gauge@36 (maxim,max77759-fg): reg-names: ['m5'] is too short
	from schema $id: http://devicetree.org/schemas/power/supply/maxim,max17201.yaml#
arch/arm64/boot/dts/exynos/google/gs101-raven.dtb: fuel-gauge@36 (maxim,max77759-fg): Unevaluated properties are not allowed ('reg-names' was unexpected)
	from schema $id: http://devicetree.org/schemas/power/supply/maxim,max17201.yaml#
arch/arm64/boot/dts/exynos/google/gs101-oriole.dtb: fuel-gauge@36 (maxim,max77759-fg): reg: [[54]] is too short
	from schema $id: http://devicetree.org/schemas/power/supply/maxim,max17201.yaml#
arch/arm64/boot/dts/exynos/google/gs101-oriole.dtb: fuel-gauge@36 (maxim,max77759-fg): reg-names: ['m5'] is too short
	from schema $id: http://devicetree.org/schemas/power/supply/maxim,max17201.yaml#
arch/arm64/boot/dts/exynos/google/gs101-oriole.dtb: fuel-gauge@36 (maxim,max77759-fg): Unevaluated properties are not allowed ('reg-names' was unexpected)
	from schema $id: http://devicetree.org/schemas/power/supply/maxim,max17201.yaml#

Re: [PATCH v3 1/5] power: supply: correct capacity computation

[Dimitri Fedrau]

Hi Thomas,

On Mon, Apr 21, 2025 at 08:13:32PM +0200, Thomas Antoine via B4 Relay wrote:
> 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 | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/power/supply/max1720x_battery.c b/drivers/power/supply/max1720x_battery.c
> index ea3912fd1de8bfd0d029c16f276316d06e1b105c..cca5f8b5071fb731f9b60420239ea03d46cb1bf3 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 */
>  }
>  
>  /*
> 
> -- 
> 2.49.0
> 
> 
thanks for finding this.

Reviewed-by: Dimitri Fedrau <dimitri.fedrau@liebherr.com>

Best regards,
Dimitri Fedrau

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

[Krzysztof Kozlowski]

On Mon, Apr 21, 2025 at 08:13:34PM GMT, Thomas Antoine wrote:
> +allOf:
> +  - $ref: power-supply.yaml#
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - maxim,max17201
> +    then:
> +      properties:
> +        reg:
> +          minItems: 2
> +          maxItems: 2
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - maxim,max77759-fg
> +    then:
> +      properties:
> +        reg:
> +          items:
> +            minItems: 1

If there is going to be resend, drop minItems.

> +            maxItems: 1
> +        shunt-resistor-micro-ohms:
> +          description: The value of current sense resistor in microohms.

Property should be defined top-level list of properties and in other
variant if:then: you disallow it if it is not applicable at all
(shunt-resistor-micro-ohms: false).

Best regards,
Krzysztof

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

[Dimitri Fedrau]

Hi Thomas,

On Mon, Apr 21, 2025 at 08:13:33PM +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 | 270 ++++++++++++++++++++++++++++----
>  1 file changed, 237 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/power/supply/max1720x_battery.c b/drivers/power/supply/max1720x_battery.c
> index cca5f8b5071fb731f9b60420239ea03d46cb1bf3..969d3a7c2baa7e1d23c5175942d975b277c8554c 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 */
> @@ -50,19 +51,32 @@
>  #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 MAX77759_TASKPERIOD		0x3C
> +#define MAX77759_TASKPERIOD_175MS	0x1680
> +#define MAX77759_TASKPERIOD_351MS	0x2D00

I think it would be more readable if MAX77759_ defines are separated to
the MAX172XX defines instead of mixing them up.

>  #define MAX172XX_BATT			0xDA	/* Battery voltage */
>  #define MAX172XX_ATAVCAP		0xDF
>  
>  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 charge_full_design;

Don't see charge_full_design is used somewhere besides reading it from
device-tree and it isn't part of the bindings. If not needed, remove it.

> +	enum chip_id id;
>  };
>  
> 

[...]

> +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)
> +				   struct max1720x_device_info *info,
> +				   int *intval)
>  {
> -	return reg * (500000 / info->rsense);	/* in uAh */
> +	int lsb = 1;
> +	int reg_val;

The naming of reg_val is somehow confusing because of reg. Better rename
it to something like reg_task_period or similar and reg_val should be of
type unsigned int. 

> +	int ret;
> +
> +	if (info->id == MAX77759_ID) {
> +		ret = regmap_read(info->regmap, MAX77759_TASKPERIOD, &reg_val);
> +		if (ret)
> +			return ret;
> +
> +		switch (reg_val) {
> +		case MAX77759_TASKPERIOD_175MS:
> +			break;
> +		case MAX77759_TASKPERIOD_351MS:
> +			lsb = 2;
> +			break;
> +		default:
> +			return -ENODEV;
> +		}
> +	}
> +	*intval = reg * (500000 / info->rsense) * lsb;	/* in uAh */
> +	return 0;

nit: add newline before return.

>  }
>  
>  /*
> @@ -306,6 +420,28 @@ static int max172xx_temperature_to_ps(unsigned int reg)
>  	return val * 10 / 256; /* in tenths of deg. C */
>  }
>  
> +static const char *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)
> +			return max17201_model;
> +		else if (reg_val == MAX172XX_DEV_NAME_TYPE_MAX17205)
> +			return max17205_model;
> +		return NULL;

nit: return NULL in else case.

> +	case MAX77759_ID:
> +		reg_val = FIELD_GET(MAX77759_DEV_NAME_TYPE_MASK, reg_val);
> +		if (reg_val == MAX77759_DEV_NAME_TYPE_MAX77759)
> +			return max77759_model;
> +		return NULL;

nit: return NULL in else case.

> +	default:
> +		return NULL;
> +	}
> +}
> +
>  /*
>   * Calculating current registers resolution:
>   *
> @@ -390,19 +526,31 @@ 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);
> -		val->intval = max172xx_capacity_to_ps(reg_val);
> +		if (ret)
> +			break;

I would keep max172xx_capacity_to_ps as it was before and add the
calculation for the MAX77759 after handling the MAX1720X case. Creating
a function max77759_capacity_to_ps that further processes the value
calculated by max172xx_capacity_to_ps or just inline this code.
Otherwise the naming of the function is somehow confusing.

> +		ret = max172xx_capacity_to_ps(reg_val, info, &val->intval);
>  		break;
>  	case POWER_SUPPLY_PROP_CHARGE_AVG:
>  		ret = regmap_read(info->regmap, MAX172XX_REPCAP, &reg_val);
> -		val->intval = max172xx_capacity_to_ps(reg_val);
> +		if (ret)
> +			break;
> +

Same as above.

> +		ret = max172xx_capacity_to_ps(reg_val, info, &val->intval);
>  		break;
>  	case POWER_SUPPLY_PROP_TIME_TO_EMPTY_AVG:
>  		ret = regmap_read(info->regmap, MAX172XX_TTE, &reg_val);
> +		pr_info("RAW TTE: %d", reg_val);

Remove pr_info.

>  		val->intval = max172xx_time_to_ps(reg_val);
>  		break;
>  	case POWER_SUPPLY_PROP_TIME_TO_FULL_AVG:
> @@ -423,17 +571,15 @@ 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);

...

> +		if (ret)
> +			break;
> +		ret = max172xx_capacity_to_ps(reg_val, info, &val->intval);
>  		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;
> +		val->strval = max1720x_devname_to_model(reg_val, val, info);

Wouldn't it be better to just inline this function ?

> +		if (!val->strval)
> +			ret = -ENODEV;
>  {

[...]

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

nit: add empty line

>  	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 = of_property_read_u32(dev->of_node,
> +				   "charge-full-design-microamp-hours", &info->charge_full_design);
> +	if (ret)
> +		info->charge_full_design = 0;
> +
> +	ret = max1720x_get_rsense(dev, info, data);
>  	if (ret)
> -		return dev_err_probe(dev, ret, "Failed to probe nvmem\n");
> +		return dev_err_probe(dev, ret, "Failed to get RSense\n");
>  
> -	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 +816,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
> 
> 

Best regards,
Dimitri Fedrau

Re: [PATCH v3 1/5] power: supply: correct capacity computation

[Thomas Antoine]

Hi Dimitri,

On 4/22/25 11:46, Dimitri Fedrau wrote:
> Hi Thomas,
> 
> On Mon, Apr 21, 2025 at 08:13:32PM +0200, Thomas Antoine via B4 Relay wrote:
>> From: Thomas Antoine <t.antoine@uclouvain.be>
[...]
>> -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 */
>>  }
>>  
>>  /*
>>
>> -- 
>> 2.49.0
>>
>>
> thanks for finding this.
> 
> Reviewed-by: Dimitri Fedrau <dimitri.fedrau@liebherr.com>
> 
> Best regards,
> Dimitri Fedrau

I just realized I forgot to change the function calls in this patch, it is only
changed in the next patch. This will not compile as the function call does not
pass info as argument. I will change this in the next version.

Best regards,
Thomas Antoine

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

[Thomas Antoine]

Hi Dimitri,

On 4/22/25 20:48, Dimitri Fedrau wrote:
> Hi Thomas,
> 
> On Mon, Apr 21, 2025 at 08:13:33PM +0200, Thomas Antoine via B4 Relay wrote:
>> From: Thomas Antoine <t.antoine@uclouvain.be>

[...]

>>  #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 */
>> @@ -50,19 +51,32 @@
>>  #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 MAX77759_TASKPERIOD		0x3C
>> +#define MAX77759_TASKPERIOD_175MS	0x1680
>> +#define MAX77759_TASKPERIOD_351MS	0x2D00
> I think it would be more readable if MAX77759_ defines are separated to
> the MAX172XX defines instead of mixing them up.

Will fix in v4.

>>  #define MAX172XX_BATT			0xDA	/* Battery voltage */
>>  #define MAX172XX_ATAVCAP		0xDF
>>  
>>  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 charge_full_design;
> Don't see charge_full_design is used somewhere besides reading it from
> device-tree and it isn't part of the bindings. If not needed, remove it.
> 
Its a leftover of a previous experimentation, will remove in next version.

>> +	enum chip_id id;
>>  };
>>  
>>
> [...]
> 
>> +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)
>> +				   struct max1720x_device_info *info,
>> +				   int *intval)
>>  {
>> -	return reg * (500000 / info->rsense);	/* in uAh */
>> +	int lsb = 1;
>> +	int reg_val;
> The naming of reg_val is somehow confusing because of reg. Better rename
> it to something like reg_task_period or similar and reg_val should be of
> type unsigned int. 
>
Will change in v4.

>> +	int ret;
>> +
>> +	if (info->id == MAX77759_ID) {
>> +		ret = regmap_read(info->regmap, MAX77759_TASKPERIOD, &reg_val);
>> +		if (ret)
>> +			return ret;
>> +
>> +		switch (reg_val) {
>> +		case MAX77759_TASKPERIOD_175MS:
>> +			break;
>> +		case MAX77759_TASKPERIOD_351MS:
>> +			lsb = 2;
>> +			break;
>> +		default:
>> +			return -ENODEV;
>> +		}
>> +	}
>> +	*intval = reg * (500000 / info->rsense) * lsb;	/* in uAh */
>> +	return 0;
> nit: add newline before return.
>
Will fix in  v4

>>  }
>>  
>>  /*
>> @@ -306,6 +420,28 @@ static int max172xx_temperature_to_ps(unsigned int reg)
>>  	return val * 10 / 256; /* in tenths of deg. C */
>>  }
>>  
>> +static const char *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)
>> +			return max17201_model;
>> +		else if (reg_val == MAX172XX_DEV_NAME_TYPE_MAX17205)
>> +			return max17205_model;
>> +		return NULL;
> nit: return NULL in else case.
> 
>> +	case MAX77759_ID:
>> +		reg_val = FIELD_GET(MAX77759_DEV_NAME_TYPE_MASK, reg_val);
>> +		if (reg_val == MAX77759_DEV_NAME_TYPE_MAX77759)
>> +			return max77759_model;
>> +		return NULL;
> nit: return NULL in else case.
>
Will fix both in v4.

>> +	default:
>> +		return NULL;
>> +	}
>> +}
>> +
>>  /*
>>   * Calculating current registers resolution:
>>   *
>> @@ -390,19 +526,31 @@ 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);
>> -		val->intval = max172xx_capacity_to_ps(reg_val);
>> +		if (ret)
>> +			break;
> I would keep max172xx_capacity_to_ps as it was before and add the
> calculation for the MAX77759 after handling the MAX1720X case. Creating
> a function max77759_capacity_to_ps that further processes the value
> calculated by max172xx_capacity_to_ps or just inline this code.
> Otherwise the naming of the function is somehow confusing.
>
Will change for v4.

>> +		ret = max172xx_capacity_to_ps(reg_val, info, &val->intval);
>>  		break;
>>  	case POWER_SUPPLY_PROP_CHARGE_AVG:
>>  		ret = regmap_read(info->regmap, MAX172XX_REPCAP, &reg_val);
>> -		val->intval = max172xx_capacity_to_ps(reg_val);
>> +		if (ret)
>> +			break;
>> +
> Same as above.
> 
>> +		ret = max172xx_capacity_to_ps(reg_val, info, &val->intval);
>>  		break;
>>  	case POWER_SUPPLY_PROP_TIME_TO_EMPTY_AVG:
>>  		ret = regmap_read(info->regmap, MAX172XX_TTE, &reg_val);
>> +		pr_info("RAW TTE: %d", reg_val);
> Remove pr_info.
>
Once again debug I forgot, sorry for this.

>>  		val->intval = max172xx_time_to_ps(reg_val);
>>  		break;
>>  	case POWER_SUPPLY_PROP_TIME_TO_FULL_AVG:
>> @@ -423,17 +571,15 @@ 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);
> ...
> 
>> +		if (ret)
>> +			break;
>> +		ret = max172xx_capacity_to_ps(reg_val, info, &val->intval);
>>  		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;
>> +		val->strval = max1720x_devname_to_model(reg_val, val, info);
> Wouldn't it be better to just inline this function ?
>
I think my reason for this was that this case became quite long and indented
compared to all the others. If you think it is better, I will inline it for v4.

>> +		if (!val->strval)
>> +			ret = -ENODEV;
>>  {
> [...]
> 
>>  	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");
>> +	}
> nit: add empty line
>
Will add in v4.

[...]

Best regards,
Thomas Antoine

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

[Thomas Antoine]

On 4/22/25 12:05, Krzysztof Kozlowski wrote:
> On Mon, Apr 21, 2025 at 08:13:34PM GMT, Thomas Antoine wrote:
>> +allOf:
>> +  - $ref: power-supply.yaml#
>> +  - if:
>> +      properties:
>> +        compatible:
>> +          contains:
>> +            enum:
>> +              - maxim,max17201
>> +    then:
>> +      properties:
>> +        reg:
>> +          minItems: 2
>> +          maxItems: 2
>> +  - if:
>> +      properties:
>> +        compatible:
>> +          contains:
>> +            enum:
>> +              - maxim,max77759-fg
>> +    then:
>> +      properties:
>> +        reg:
>> +          items:
>> +            minItems: 1
> If there is going to be resend, drop minItems.
>
Will drop it in v4.

>> +            maxItems: 1
>> +        shunt-resistor-micro-ohms:
>> +          description: The value of current sense resistor in microohms.
> Property should be defined top-level list of properties and in other
> variant if:then: you disallow it if it is not applicable at all
> (shunt-resistor-micro-ohms: false).
>
Will change in v4.
> Best regards,
> Krzysztof
> 

Best regards,
Thomas Antoine

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

[Krzysztof Kozlowski]

On 23/04/2025 10:05, Thomas Antoine wrote:
> On 4/22/25 12:05, Krzysztof Kozlowski wrote:
>> On Mon, Apr 21, 2025 at 08:13:34PM GMT, Thomas Antoine wrote:
>>> +allOf:
>>> +  - $ref: power-supply.yaml#
>>> +  - if:
>>> +      properties:
>>> +        compatible:
>>> +          contains:
>>> +            enum:
>>> +              - maxim,max17201
>>> +    then:
>>> +      properties:
>>> +        reg:
>>> +          minItems: 2
>>> +          maxItems: 2
>>> +  - if:
>>> +      properties:
>>> +        compatible:
>>> +          contains:
>>> +            enum:
>>> +              - maxim,max77759-fg
>>> +    then:
>>> +      properties:
>>> +        reg:
>>> +          items:
>>> +            minItems: 1
>> If there is going to be resend, drop minItems.
>>
> Will drop it in v4.
> 
One more thing - your reg-names are now incorrectly constrained - where
are their constraints?

You need to test your bindings and DTS before you post. If by any chance
community robots found more issues (e.g. you sent something untested),
you should address it.


Best regards,
Krzysztof

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

[Sebastian Reichel]

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

Hi,

On Mon, Apr 21, 2025 at 08:13:33PM +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>
> ---

[...] (I actually skipped reviewing big parts here for now)

>
> +		ret = of_property_read_u32(dev->of_node,
> +					   "shunt-resistor-micro-ohms", &val);

device_property_read_u32(dev, ...)

> [...]
> +	ret = of_property_read_u32(dev->of_node,
> +				   "charge-full-design-microamp-hours", &info->charge_full_design);
> +	if (ret)
> +		info->charge_full_design = 0;

This is not in the DT binding and thus not allowed. Also I will NAK
adding it to the DT binding, since the following should be used:

Documentation/devicetree/bindings/power/supply/battery.yaml

followed by using power_supply_get_battery_info() in this driver.

Adding this support is probably a good idea, since it allows
providing all kind of static information from DT and you are
missing the non-volatile memory part from the existing chip.

Note that the power-supply core will pick up and expose any
of these properties automatically.

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

You can either drop this error print, or the ones in
max1720x_get_rsense(). No need to print two errors.

Greetings,

-- Sebastian

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

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

[Thomas Antoine]

Hi,

On 4/30/25 02:36, Sebastian Reichel wrote:
> Hi,
> 
> On Mon, Apr 21, 2025 at 08:13:33PM +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>
>> ---
> 
> [...] (I actually skipped reviewing big parts here for now)
> 
>>
>> +		ret = of_property_read_u32(dev->of_node,
>> +					   "shunt-resistor-micro-ohms", &val);
> 
> device_property_read_u32(dev, ...)
> 
>> [...]
>> +	ret = of_property_read_u32(dev->of_node,
>> +				   "charge-full-design-microamp-hours", &info->charge_full_design);
>> +	if (ret)
>> +		info->charge_full_design = 0;
> 
> This is not in the DT binding and thus not allowed. Also I will NAK
> adding it to the DT binding, since the following should be used:
> 
> Documentation/devicetree/bindings/power/supply/battery.yaml
> 
> followed by using power_supply_get_battery_info() in this driver.
> 
> Adding this support is probably a good idea, since it allows
> providing all kind of static information from DT and you are
> missing the non-volatile memory part from the existing chip.
> 
> Note that the power-supply core will pick up and expose any
> of these properties automatically.
>

As I said to Dimitri Fedrau, this is actually a mistake on my part coming from
an attempt at getting things working which failed. Charge full design should
be correctly computed now so I don't think it would be useful to get it from
DT.
 
>> +	ret = max1720x_get_rsense(dev, info, data);
>>  	if (ret)
>> -		return dev_err_probe(dev, ret, "Failed to probe nvmem\n");
>> +		return dev_err_probe(dev, ret, "Failed to get RSense\n");
> 
> You can either drop this error print, or the ones in
> max1720x_get_rsense(). No need to print two errors.
> 
> Greetings,
> 
> -- Sebastian

I will do that in v4.

Best regards,
Thomas

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

[Thomas Antoine]

Hello,

On 4/23/25 15:45, Krzysztof Kozlowski wrote:
> On 23/04/2025 10:05, Thomas Antoine wrote:
>> On 4/22/25 12:05, Krzysztof Kozlowski wrote:
>>> On Mon, Apr 21, 2025 at 08:13:34PM GMT, Thomas Antoine wrote:
>>>> +allOf:
>>>> +  - $ref: power-supply.yaml#
>>>> +  - if:
>>>> +      properties:
>>>> +        compatible:
>>>> +          contains:
>>>> +            enum:
>>>> +              - maxim,max17201
>>>> +    then:
>>>> +      properties:
>>>> +        reg:
>>>> +          minItems: 2
>>>> +          maxItems: 2
>>>> +  - if:
>>>> +      properties:
>>>> +        compatible:
>>>> +          contains:
>>>> +            enum:
>>>> +              - maxim,max77759-fg
>>>> +    then:
>>>> +      properties:
>>>> +        reg:
>>>> +          items:
>>>> +            minItems: 1
>>> If there is going to be resend, drop minItems.
>>>
>> Will drop it in v4.
>>
> One more thing - your reg-names are now incorrectly constrained - where
> are their constraints?
> 
> You need to test your bindings and DTS before you post. If by any chance
> community robots found more issues (e.g. you sent something untested),
> you should address it.
> 
> 
> Best regards,
> Krzysztof

I did run the test but I obviously must have done it wrong. I will check it out
such that it does not happen again.

Best regards,
Thomas