Re: [PATCH v6 5/6] power: supply: max77759: add charger driver

Hi André,


On 2/17/26 5:14 AM, André Draszik wrote:

Hi Amit,

All below comments are only minor, feel free to ignore them.

On Sat, 2026-02-14 at 03:12 +0000, Amit Sunil Dhamne via B4 Relay wrote:

quoted

From: Amit Sunil Dhamne <amitsd@google.com>

Add support for MAX77759 battery charger driver. This is a 4A 1-Cell
Li+/LiPoly dual input switch mode charger. While the device can support
USB & wireless charger inputs, this implementation only supports USB
input. This implementation supports both buck and boost modes.

Signed-off-by: Amit Sunil Dhamne <amitsd@google.com>
---
 MAINTAINERS                             |   6 +
 drivers/power/supply/Kconfig            |  11 +
 drivers/power/supply/Makefile           |   1 +
 drivers/power/supply/max77759_charger.c | 768 ++++++++++++++++++++++++++++++++
 4 files changed, 786 insertions(+)

[...]

quoted

diff --git a/drivers/power/supply/max77759_charger.c b/drivers/power/supply/max77759_charger.c
new file mode 100644
index 000000000000..d4e02764ba04
--- /dev/null
+++ b/drivers/power/supply/max77759_charger.c

@@ -0,0 +1,768 @@

[...]

quoted

+
+/* USB input current limits (in uA) */
+static const struct linear_range chgin_ilim_ranges[] = {
+	LINEAR_RANGE(100000, 0x3, 0x7F, 25000),
+};

Shouldn't this one also have a entry for 0x00...0x02:
	LINEAR_RANGE(100000, 0x0, 0x2, 0),

Then you can also drop the umax() call in get_input_current_limit().

Ah, I see now there is no linear_range_get_selector_within_array(),
meaning the code is fine as-is, unless you want to add that as
well :-)


[...]

I would go with the code being as is for now.  :-)

quoted

+static int max77759_charger_init(struct max77759_charger *chg)
+{
+	struct power_supply_battery_info *info;
+	u32 regval, fast_chg_curr, fv;
+	int ret;
+
+	ret = regmap_read(chg->regmap, MAX77759_CHGR_REG_CHG_CNFG_00, &regval);
+	if (ret)
+		return ret;
+
+	chg->mode = FIELD_GET(MAX77759_CHGR_REG_CHG_CNFG_00_MODE, regval);
+	ret = charger_set_mode(chg, MAX77759_CHGR_MODE_OFF);
+	if (ret)
+		return ret;
+
+	if (power_supply_get_battery_info(chg->psy, &info)) {
+		fv = CHG_FV_DEFAULT_MV;
+		fast_chg_curr = CHG_CC_DEFAULT_UA;
+	} else {
+		fv = info->constant_charge_voltage_max_uv / 1000;
+		fast_chg_curr = info->constant_charge_current_max_ua;
+	}
+
+	ret = set_fast_charge_current_limit(chg, fast_chg_curr);
+	if (ret)
+		return ret;
+
+	ret = set_float_voltage_limit(chg, fv);
+	if (ret)
+		return ret;
+
+	ret = unlock_prot_regs(chg, true);
+	if (ret)
+		return ret;
+
+	/* Disable wireless charging input */
+	ret = regmap_update_bits(chg->regmap, MAX77759_CHGR_REG_CHG_CNFG_12,
+				 MAX77759_CHGR_REG_CHG_CNFG_12_WCINSEL, 0);
+	if (ret)
+		return ret;
+
+	ret = regmap_update_bits(chg->regmap, MAX77759_CHGR_REG_CHG_CNFG_18,
+				 MAX77759_CHGR_REG_CHG_CNFG_18_WDTEN, 0);
+	if (ret)
+		return ret;
+
+	return unlock_prot_regs(chg, false);

Should early error returns here try to lock the protection again? Something
like:

+	ret = unlock_prot_regs(chg, true);
+	if (ret)
+		return ret;
+
+	/* Disable wireless charging input */
+	ret = regmap_update_bits(chg->regmap, MAX77759_CHGR_REG_CHG_CNFG_12,
+				 MAX77759_CHGR_REG_CHG_CNFG_12_WCINSEL, 0);
+	if (ret)
+		goto relock;
+
+	ret = regmap_update_bits(chg->regmap, MAX77759_CHGR_REG_CHG_CNFG_18,
+				 MAX77759_CHGR_REG_CHG_CNFG_18_WDTEN, 0);
+	if (ret)
+		goto relock;
+
+	return unlock_prot_regs(chg, false);
+
+relock:
+	(void) unlock_prot_regs(chg, false);
+	return ret;

I guess if one of the regmap_update_bits() failed, then locking the
registers might not work either, so I have no strong opinion on
adding that.

Nice catch!

I need to send a next revision to keep the Linux Test Robot happy. I
will address this issue in that.

With or without updates:

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

LGTM! Thanks!


Regards,

Amit

Cheers,
Andre'