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 | 764 ++++++++++++++++++++++++++++++++
4 files changed, 782 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index dc731d37c8fe..26a9654ab75e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -15539,6 +15539,12 @@ F: drivers/mfd/max77759.c
F: drivers/nvmem/max77759-nvmem.c
F: include/linux/mfd/max77759.h
+MAXIM MAX77759 BATTERY CHARGER DRIVER
+M: Amit Sunil Dhamne <amitsd@google.com>
+L: linux-kernel@vger.kernel.org
+S: Maintained
+F: drivers/power/supply/max77759_charger.c
+
MAXIM MAX77802 PMIC REGULATOR DEVICE DRIVER
M: Javier Martinez Canillas <javier@dowhile0.org>
L: linux-kernel@vger.kernel.org
diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
index 92f9f7aae92f..e172fd980fde 100644
--- a/drivers/power/supply/Kconfig
+++ b/drivers/power/supply/Kconfig
@@ -1132,4 +1132,15 @@ config FUEL_GAUGE_MM8013
the state of charge, temperature, cycle count, actual and design
capacity, etc.
+config CHARGER_MAX77759
+ tristate "MAX77759 Charger Driver"
+ depends on MFD_MAX77759 && REGULATOR
+ default MFD_MAX77759
+ help
+ Say M or Y here to enable the MAX77759 Charger Driver. MAX77759
+ charger is a function of the MAX77759 PMIC. This is a dual input
+ switch-mode charger. This driver supports buck and OTG boost modes.
+
+ If built as a module, it will be called max77759_charger.
+
endif # POWER_SUPPLY
diff --git a/drivers/power/supply/Makefile b/drivers/power/supply/Makefile
index 4b79d5abc49a..6af905875ad5 100644
--- a/drivers/power/supply/Makefile
+++ b/drivers/power/supply/Makefile
@@ -128,3 +128,4 @@ obj-$(CONFIG_CHARGER_SURFACE) += surface_charger.o
obj-$(CONFIG_BATTERY_UG3105) += ug3105_battery.o
obj-$(CONFIG_CHARGER_QCOM_SMB2) += qcom_smbx.o
obj-$(CONFIG_FUEL_GAUGE_MM8013) += mm8013.o
+obj-$(CONFIG_CHARGER_MAX77759) += max77759_charger.o
diff --git a/drivers/power/supply/max77759_charger.c b/drivers/power/supply/max77759_charger.c
new file mode 100644
index 000000000000..3d255b069fb9
--- /dev/null
+++ b/drivers/power/supply/max77759_charger.c
@@ -0,0 +1,764 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * max77759_charger.c - Battery charger driver for MAX77759 charger device.
+ *
+ * Copyright 2025 Google LLC.
+ */
+
+#include <linux/bitfield.h>
+#include <linux/cleanup.h>
+#include <linux/device.h>
+#include <linux/devm-helpers.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/math64.h>
+#include <linux/mfd/max77759.h>
+#include <linux/module.h>
+#include <linux/mod_devicetable.h>
+#include <linux/mutex.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/power_supply.h>
+#include <linux/regmap.h>
+#include <linux/regulator/driver.h>
+#include <linux/string_choices.h>
+
+/* Default values for Fast Charge Current & Float Voltage */
+#define CHG_CC_DEFAULT_UA 2266770
+#define CHG_FV_DEFAULT_MV 4300
+
+#define FOREACH_IRQ(S) \
+ S(AICL), \
+ S(CHGIN), \
+ S(CHG), \
+ S(INLIM), \
+ S(BAT_OILO), \
+ S(CHG_STA_CC), \
+ S(CHG_STA_CV), \
+ S(CHG_STA_TO), \
+ S(CHG_STA_DONE)
+
+#define GENERATE_ENUM(e) e
+#define GENERATE_STRING(s) #s
+
+enum {
+ FOREACH_IRQ(GENERATE_ENUM)
+};
+
+static const char *const chgr_irqs_str[] = {
+ FOREACH_IRQ(GENERATE_STRING)
+};
+
+static int irqs[ARRAY_SIZE(chgr_irqs_str)];
+
+struct max77759_charger {
+ struct device *dev;
+ struct regmap *regmap;
+ struct power_supply *psy;
+ struct regulator_dev *chgin_otg_rdev;
+ struct notifier_block nb;
+ struct power_supply *tcpm_psy;
+ struct work_struct psy_work;
+ struct mutex lock; /* protects the state below */
+ enum max77759_chgr_mode mode;
+};
+
+static inline int regval_to_val(int reg, int reg_offset, int step, int minval)
+{
+ return ((reg - reg_offset) * step) + minval;
+}
+
+static inline int val_to_regval(int val, int minval, int step, int reg_offset)
+{
+ s64 dividend;
+
+ if (unlikely(step == 0))
+ return reg_offset;
+
+ dividend = (s64)val - minval;
+ return DIV_S64_ROUND_CLOSEST(dividend, step) + reg_offset;
+}
+
+static inline int unlock_prot_regs(struct max77759_charger *chg, bool unlock)
+{
+ return regmap_update_bits(chg->regmap, MAX77759_CHGR_REG_CHG_CNFG_06,
+ MAX77759_CHGR_REG_CHG_CNFG_06_CHGPROT, unlock
+ ? MAX77759_CHGR_REG_CHG_CNFG_06_CHGPROT : 0);
+}
+
+static int charger_input_valid(struct max77759_charger *chg)
+{
+ u32 val;
+ int ret;
+
+ ret = regmap_read(chg->regmap, MAX77759_CHGR_REG_CHG_INT_OK, &val);
+ if (ret)
+ return ret;
+
+ return (val & MAX77759_CHGR_REG_CHG_INT_CHG) &&
+ (val & MAX77759_CHGR_REG_CHG_INT_CHGIN);
+}
+
+static int get_online(struct max77759_charger *chg)
+{
+ u32 val;
+ int ret;
+
+ ret = charger_input_valid(chg);
+ if (ret <= 0)
+ return ret;
+
+ ret = regmap_read(chg->regmap, MAX77759_CHGR_REG_CHG_DETAILS_02, &val);
+ if (ret)
+ return ret;
+
+ guard(mutex)(&chg->lock);
+ return (val & MAX77759_CHGR_REG_CHG_DETAILS_02_CHGIN_STS) &&
+ (chg->mode == MAX77759_CHGR_MODE_CHG_BUCK_ON);
+}
+
+static int get_status(struct max77759_charger *chg)
+{
+ u32 val;
+ int ret;
+
+ ret = regmap_read(chg->regmap, MAX77759_CHGR_REG_CHG_DETAILS_01, &val);
+ if (ret)
+ return ret;
+
+ switch (FIELD_GET(MAX77759_CHGR_REG_CHG_DETAILS_01_CHG_DTLS, val)) {
+ case MAX77759_CHGR_CHG_DTLS_PREQUAL:
+ case MAX77759_CHGR_CHG_DTLS_CC:
+ case MAX77759_CHGR_CHG_DTLS_CV:
+ case MAX77759_CHGR_CHG_DTLS_TO:
+ return POWER_SUPPLY_STATUS_CHARGING;
+ case MAX77759_CHGR_CHG_DTLS_DONE:
+ return POWER_SUPPLY_STATUS_FULL;
+ case MAX77759_CHGR_CHG_DTLS_TIMER_FAULT:
+ case MAX77759_CHGR_CHG_DTLS_SUSP_BATT_THM:
+ case MAX77759_CHGR_CHG_DTLS_OFF_WDOG_TIMER:
+ case MAX77759_CHGR_CHG_DTLS_SUSP_JEITA:
+ return POWER_SUPPLY_STATUS_NOT_CHARGING;
+ case MAX77759_CHGR_CHG_DTLS_OFF:
+ return POWER_SUPPLY_STATUS_DISCHARGING;
+ default:
+ break;
+ }
+
+ return POWER_SUPPLY_STATUS_UNKNOWN;
+}
+
+static int get_charge_type(struct max77759_charger *chg)
+{
+ u32 val;
+ int ret;
+
+ ret = regmap_read(chg->regmap, MAX77759_CHGR_REG_CHG_DETAILS_01, &val);
+ if (ret)
+ return ret;
+
+ switch (FIELD_GET(MAX77759_CHGR_REG_CHG_DETAILS_01_CHG_DTLS, val)) {
+ case MAX77759_CHGR_CHG_DTLS_PREQUAL:
+ return POWER_SUPPLY_CHARGE_TYPE_TRICKLE;
+ case MAX77759_CHGR_CHG_DTLS_CC:
+ case MAX77759_CHGR_CHG_DTLS_CV:
+ return POWER_SUPPLY_CHARGE_TYPE_FAST;
+ case MAX77759_CHGR_CHG_DTLS_TO:
+ return POWER_SUPPLY_CHARGE_TYPE_STANDARD;
+ case MAX77759_CHGR_CHG_DTLS_DONE:
+ case MAX77759_CHGR_CHG_DTLS_TIMER_FAULT:
+ case MAX77759_CHGR_CHG_DTLS_SUSP_BATT_THM:
+ case MAX77759_CHGR_CHG_DTLS_OFF_WDOG_TIMER:
+ case MAX77759_CHGR_CHG_DTLS_SUSP_JEITA:
+ case MAX77759_CHGR_CHG_DTLS_OFF:
+ return POWER_SUPPLY_CHARGE_TYPE_NONE;
+ default:
+ break;
+ }
+
+ return POWER_SUPPLY_CHARGE_TYPE_UNKNOWN;
+}
+
+static int get_chg_health(struct max77759_charger *chg)
+{
+ u32 val;
+ int ret;
+
+ ret = regmap_read(chg->regmap, MAX77759_CHGR_REG_CHG_DETAILS_00, &val);
+ if (ret)
+ return ret;
+
+ switch (FIELD_GET(MAX77759_CHGR_REG_CHG_DETAILS_OO_CHGIN_DTLS, val)) {
+ case MAX77759_CHGR_CHGIN_DTLS_VBUS_UNDERVOLTAGE:
+ case MAX77759_CHGR_CHGIN_DTLS_VBUS_MARGINAL_VOLTAGE:
+ return POWER_SUPPLY_HEALTH_UNDERVOLTAGE;
+ case MAX77759_CHGR_CHGIN_DTLS_VBUS_OVERVOLTAGE:
+ return POWER_SUPPLY_HEALTH_OVERVOLTAGE;
+ case MAX77759_CHGR_CHGIN_DTLS_VBUS_VALID:
+ return POWER_SUPPLY_HEALTH_GOOD;
+ default:
+ break;
+ }
+
+ return POWER_SUPPLY_HEALTH_UNKNOWN;
+}
+
+static int get_batt_health(struct max77759_charger *chg)
+{
+ u32 val;
+ int ret;
+
+ ret = regmap_read(chg->regmap, MAX77759_CHGR_REG_CHG_DETAILS_01, &val);
+ if (ret)
+ return ret;
+
+ switch (FIELD_GET(MAX77759_CHGR_REG_CHG_DETAILS_01_BAT_DTLS, val)) {
+ case MAX77759_CHGR_BAT_DTLS_NO_BATT_CHG_SUSP:
+ return POWER_SUPPLY_HEALTH_NO_BATTERY;
+ case MAX77759_CHGR_BAT_DTLS_DEAD_BATTERY:
+ return POWER_SUPPLY_HEALTH_DEAD;
+ case MAX77759_CHGR_BAT_DTLS_BAT_CHG_TIMER_FAULT:
+ return POWER_SUPPLY_HEALTH_SAFETY_TIMER_EXPIRE;
+ case MAX77759_CHGR_BAT_DTLS_BAT_OKAY:
+ case MAX77759_CHGR_BAT_DTLS_BAT_ONLY_MODE:
+ return POWER_SUPPLY_HEALTH_GOOD;
+ case MAX77759_CHGR_BAT_DTLS_BAT_UNDERVOLTAGE:
+ return POWER_SUPPLY_HEALTH_UNDERVOLTAGE;
+ case MAX77759_CHGR_BAT_DTLS_BAT_OVERVOLTAGE:
+ return POWER_SUPPLY_HEALTH_OVERVOLTAGE;
+ case MAX77759_CHGR_BAT_DTLS_BAT_OVERCURRENT:
+ return POWER_SUPPLY_HEALTH_OVERCURRENT;
+ default:
+ break;
+ }
+
+ return POWER_SUPPLY_HEALTH_UNKNOWN;
+}
+
+static int get_health(struct max77759_charger *chg)
+{
+ int ret;
+
+ ret = get_online(chg);
+ if (ret < 0)
+ return ret;
+
+ if (ret) {
+ ret = get_chg_health(chg);
+ if (ret < 0 || ret != POWER_SUPPLY_HEALTH_GOOD)
+ return ret;
+ }
+
+ return get_batt_health(chg);
+}
+
+static int get_fast_charge_current(struct max77759_charger *chg)
+{
+ u32 regval;
+ int ret;
+
+ ret = regmap_read(chg->regmap, MAX77759_CHGR_REG_CHG_CNFG_02, ®val);
+ if (ret)
+ return ret;
+
+ ret = FIELD_GET(MAX77759_CHGR_REG_CHG_CNFG_02_CHGCC, regval);
+ if (ret <= MAX77759_CHGR_CHGCC_REG_OFFSET)
+ return MAX77759_CHGR_CHGCC_MIN_UA;
+
+ return regval_to_val(ret, MAX77759_CHGR_CHGCC_REG_OFFSET,
+ MAX77759_CHGR_CHGCC_STEP_UA,
+ MAX77759_CHGR_CHGCC_MIN_UA);
+}
+
+static int set_fast_charge_current_limit(struct max77759_charger *chg,
+ u32 cc_max_ua)
+{
+ u32 val;
+
+ if (cc_max_ua < MAX77759_CHGR_CHGCC_MIN_UA ||
+ cc_max_ua > MAX77759_CHGR_CHGCC_MAX_UA)
+ return -EINVAL;
+
+ val = val_to_regval(cc_max_ua, MAX77759_CHGR_CHGCC_MIN_UA,
+ MAX77759_CHGR_CHGCC_STEP_UA,
+ MAX77759_CHGR_CHGCC_REG_OFFSET);
+ return regmap_update_bits(chg->regmap, MAX77759_CHGR_REG_CHG_CNFG_02,
+ MAX77759_CHGR_REG_CHG_CNFG_02_CHGCC, val);
+}
+
+static int get_float_voltage(struct max77759_charger *chg)
+{
+ u32 regval;
+ int ret;
+
+ ret = regmap_read(chg->regmap, MAX77759_CHGR_REG_CHG_CNFG_04, ®val);
+ if (ret)
+ return ret;
+
+ ret = FIELD_GET(MAX77759_CHGR_REG_CHG_CNFG_04_CHG_CV_PRM, regval);
+ switch (ret) {
+ case MAX77759_CHGR_CHG_CV_PRM_HI_MIN_REG ... MAX77759_CHGR_CHG_CV_PRM_HI_MAX_REG:
+ return regval_to_val(ret, MAX77759_CHGR_CHG_CV_PRM_HI_MIN_REG,
+ MAX77759_CHGR_CHG_CV_PRM_HI_STEP_MV,
+ MAX77759_CHGR_CHG_CV_PRM_HI_MIN_MV);
+ case MAX77759_CHGR_CHG_CV_PRM_LO_MIN_REG ... MAX77759_CHGR_CHG_CV_PRM_LO_MAX_REG:
+ return regval_to_val(ret, MAX77759_CHGR_CHG_CV_PRM_LO_MIN_REG,
+ MAX77759_CHGR_CHG_CV_PRM_LO_STEP_MV,
+ MAX77759_CHGR_CHG_CV_PRM_LO_MIN_MV);
+ default:
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static int set_float_voltage_limit(struct max77759_charger *chg, u32 fv_mv)
+{
+ u32 regval;
+
+ if (fv_mv >= MAX77759_CHGR_CHG_CV_PRM_LO_MIN_MV &&
+ fv_mv <= MAX77759_CHGR_CHG_CV_PRM_LO_MAX_MV) {
+ regval = val_to_regval(fv_mv,
+ MAX77759_CHGR_CHG_CV_PRM_LO_MIN_MV,
+ MAX77759_CHGR_CHG_CV_PRM_LO_STEP_MV,
+ MAX77759_CHGR_CHG_CV_PRM_LO_MIN_REG);
+ } else if (fv_mv >= MAX77759_CHGR_CHG_CV_PRM_HI_MIN_MV &&
+ fv_mv <= MAX77759_CHGR_CHG_CV_PRM_HI_MAX_MV) {
+ regval = val_to_regval(fv_mv,
+ MAX77759_CHGR_CHG_CV_PRM_HI_MIN_MV,
+ MAX77759_CHGR_CHG_CV_PRM_HI_STEP_MV,
+ MAX77759_CHGR_CHG_CV_PRM_HI_MIN_REG);
+ } else {
+ return -EINVAL;
+ }
+
+ return regmap_update_bits(chg->regmap, MAX77759_CHGR_REG_CHG_CNFG_04,
+ MAX77759_CHGR_REG_CHG_CNFG_04_CHG_CV_PRM,
+ regval);
+}
+
+static int get_input_current_limit(struct max77759_charger *chg)
+{
+ u32 regval;
+ int ret;
+
+ ret = regmap_read(chg->regmap, MAX77759_CHGR_REG_CHG_CNFG_09, ®val);
+ if (ret)
+ return ret;
+
+ ret = FIELD_GET(MAX77759_CHGR_REG_CHG_CNFG_09_CHGIN_ILIM, regval);
+ if (ret <= MAX77759_CHGR_CHGIN_ILIM_REG_OFFSET)
+ return MAX77759_CHGR_CHGIN_ILIM_MIN_UA;
+
+ return regval_to_val(ret, MAX77759_CHGR_CHGIN_ILIM_REG_OFFSET,
+ MAX77759_CHGR_CHGIN_ILIM_STEP_UA,
+ MAX77759_CHGR_CHGIN_ILIM_MIN_UA);
+}
+
+static int set_input_current_limit(struct max77759_charger *chg, int ilim_ua)
+{
+ u32 regval;
+
+ if (ilim_ua < 0)
+ return -EINVAL;
+
+ if (ilim_ua == 0)
+ ilim_ua = MAX77759_CHGR_CHGIN_ILIM_MIN_UA;
+ else if (ilim_ua > MAX77759_CHGR_CHGIN_ILIM_MAX_UA)
+ ilim_ua = MAX77759_CHGR_CHGIN_ILIM_MAX_UA;
+
+ regval = val_to_regval(ilim_ua, MAX77759_CHGR_CHGIN_ILIM_MIN_UA,
+ MAX77759_CHGR_CHGIN_ILIM_STEP_UA,
+ MAX77759_CHGR_CHGIN_ILIM_REG_OFFSET);
+ return regmap_update_bits(chg->regmap, MAX77759_CHGR_REG_CHG_CNFG_09,
+ MAX77759_CHGR_REG_CHG_CNFG_09_CHGIN_ILIM,
+ regval);
+}
+
+static const enum power_supply_property max77759_charger_props[] = {
+ POWER_SUPPLY_PROP_ONLINE,
+ POWER_SUPPLY_PROP_PRESENT,
+ POWER_SUPPLY_PROP_STATUS,
+ POWER_SUPPLY_PROP_CHARGE_TYPE,
+ POWER_SUPPLY_PROP_HEALTH,
+ POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX,
+ POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE_MAX,
+ POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT,
+};
+
+static int max77759_charger_get_property(struct power_supply *psy,
+ enum power_supply_property psp,
+ union power_supply_propval *pval)
+{
+ struct max77759_charger *chg = power_supply_get_drvdata(psy);
+ int ret;
+
+ switch (psp) {
+ case POWER_SUPPLY_PROP_ONLINE:
+ ret = get_online(chg);
+ break;
+ case POWER_SUPPLY_PROP_PRESENT:
+ ret = charger_input_valid(chg);
+ break;
+ case POWER_SUPPLY_PROP_STATUS:
+ ret = get_status(chg);
+ break;
+ case POWER_SUPPLY_PROP_CHARGE_TYPE:
+ ret = get_charge_type(chg);
+ break;
+ case POWER_SUPPLY_PROP_HEALTH:
+ ret = get_health(chg);
+ break;
+ case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX:
+ ret = get_fast_charge_current(chg);
+ break;
+ case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE_MAX:
+ ret = get_float_voltage(chg);
+ break;
+ case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT:
+ ret = get_input_current_limit(chg);
+ break;
+ default:
+ ret = -EINVAL;
+ }
+
+ pval->intval = ret;
+ return ret < 0 ? ret : 0;
+}
+
+static const struct power_supply_desc max77759_charger_desc = {
+ .name = "max77759-charger",
+ .type = POWER_SUPPLY_TYPE_USB,
+ .properties = max77759_charger_props,
+ .num_properties = ARRAY_SIZE(max77759_charger_props),
+ .get_property = max77759_charger_get_property,
+};
+
+static int charger_set_mode(struct max77759_charger *chg,
+ enum max77759_chgr_mode mode)
+{
+ int ret;
+
+ guard(mutex)(&chg->lock);
+
+ if (chg->mode == mode)
+ return 0;
+
+ if ((mode == MAX77759_CHGR_MODE_CHG_BUCK_ON ||
+ mode == MAX77759_CHGR_MODE_OTG_BOOST_ON) &&
+ chg->mode != MAX77759_CHGR_MODE_OFF) {
+ dev_err(chg->dev, "Invalid mode transition from %d to %d",
+ chg->mode, mode);
+ return -EINVAL;
+ }
+
+ ret = regmap_update_bits(chg->regmap, MAX77759_CHGR_REG_CHG_CNFG_00,
+ MAX77759_CHGR_REG_CHG_CNFG_00_MODE, mode);
+ if (ret)
+ return ret;
+
+ chg->mode = mode;
+ return 0;
+}
+
+static int enable_chgin_otg(struct regulator_dev *rdev)
+{
+ struct max77759_charger *chg = rdev_get_drvdata(rdev);
+
+ return charger_set_mode(chg, MAX77759_CHGR_MODE_OTG_BOOST_ON);
+}
+
+static int disable_chgin_otg(struct regulator_dev *rdev)
+{
+ struct max77759_charger *chg = rdev_get_drvdata(rdev);
+
+ return charger_set_mode(chg, MAX77759_CHGR_MODE_OFF);
+}
+
+static int chgin_otg_status(struct regulator_dev *rdev)
+{
+ struct max77759_charger *chg = rdev_get_drvdata(rdev);
+
+ guard(mutex)(&chg->lock);
+ return chg->mode == MAX77759_CHGR_MODE_OTG_BOOST_ON;
+}
+
+static const struct regulator_ops chgin_otg_reg_ops = {
+ .enable = enable_chgin_otg,
+ .disable = disable_chgin_otg,
+ .is_enabled = chgin_otg_status,
+};
+
+static const struct regulator_desc chgin_otg_reg_desc = {
+ .name = "chgin-otg",
+ .of_match = of_match_ptr("chgin-otg-regulator"),
+ .owner = THIS_MODULE,
+ .ops = &chgin_otg_reg_ops,
+ .fixed_uV = 5000000,
+ .n_voltages = 1,
+};
+
+static irqreturn_t irq_handler(int irq, void *data)
+{
+ struct max77759_charger *chg = data;
+ struct device *dev = chg->dev;
+ u32 chgint_ok;
+ int i;
+
+ regmap_read(chg->regmap, MAX77759_CHGR_REG_CHG_INT_OK, &chgint_ok);
+
+ for (i = 0; i < ARRAY_SIZE(irqs); i++) {
+ if (irqs[i] == irq)
+ break;
+ }
+
+ switch (i) {
+ case AICL:
+ dev_dbg(dev, "AICL mode: %s",
+ str_no_yes(chgint_ok & MAX77759_CHGR_REG_CHG_INT_AICL));
+ break;
+ case CHGIN:
+ dev_dbg(dev, "CHGIN input valid: %s",
+ str_yes_no(chgint_ok & MAX77759_CHGR_REG_CHG_INT_CHGIN));
+ break;
+ case CHG:
+ dev_dbg(dev, "CHG status okay/off: %s",
+ str_yes_no(chgint_ok & MAX77759_CHGR_REG_CHG_INT_CHG));
+ break;
+ case INLIM:
+ dev_dbg(dev, "Current Limit reached: %s",
+ str_no_yes(chgint_ok & MAX77759_CHGR_REG_CHG_INT_INLIM));
+ break;
+ case BAT_OILO:
+ dev_dbg(dev, "Battery over-current threshold crossed");
+ break;
+ case CHG_STA_CC:
+ dev_dbg(dev, "Charger reached CC stage");
+ break;
+ case CHG_STA_CV:
+ dev_dbg(dev, "Charger reached CV stage");
+ break;
+ case CHG_STA_TO:
+ dev_dbg(dev, "Charger reached TO stage");
+ break;
+ case CHG_STA_DONE:
+ dev_dbg(dev, "Charger reached TO stage");
+ break;
+ default:
+ dev_err(dev, "Unrecognized irq: %d", i);
+ return IRQ_HANDLED;
+ }
+
+ power_supply_changed(chg->psy);
+ return IRQ_HANDLED;
+}
+
+static int max77759_init_irqhandler(struct max77759_charger *chg)
+{
+ struct device *dev = chg->dev;
+ unsigned long irq_flags;
+ struct irq_data *irqd;
+ int i, ret;
+
+ for (i = 0; i < ARRAY_SIZE(chgr_irqs_str); i++) {
+ ret = platform_get_irq_byname(to_platform_device(dev),
+ chgr_irqs_str[i]);
+ if (ret < 0) {
+ dev_err(dev,
+ "Failed to get irq resource for %s, ret=%d",
+ chgr_irqs_str[i], ret);
+ return ret;
+ }
+
+ irqs[i] = ret;
+ irq_flags = IRQF_ONESHOT;
+ irqd = irq_get_irq_data(irqs[i]);
+ if (irqd)
+ irq_flags |= irqd_get_trigger_type(irqd);
+
+ ret = devm_request_threaded_irq(dev, irqs[i], NULL, irq_handler,
+ irq_flags, dev_name(dev), chg);
+ if (ret) {
+ dev_err(dev,
+ "Unable to register irq handler for %s, ret=%d",
+ chgr_irqs_str[i], ret);
+ return ret;
+ }
+ }
+
+ return 0;
+}
+
+static int max77759_charger_init(struct max77759_charger *chg)
+{
+ struct power_supply_battery_info *info;
+ u32 regval, fast_chg_curr, fv;
+ int ret;
+
+ regmap_read(chg->regmap, MAX77759_CHGR_REG_CHG_CNFG_00, ®val);
+ 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 */
+ regmap_update_bits(chg->regmap, MAX77759_CHGR_REG_CHG_CNFG_12,
+ MAX77759_CHGR_REG_CHG_CNFG_12_WCINSEL, 0);
+
+ regmap_update_bits(chg->regmap, MAX77759_CHGR_REG_CHG_CNFG_18,
+ MAX77759_CHGR_REG_CHG_CNFG_18_WDTEN, 0);
+
+ return unlock_prot_regs(chg, false);
+}
+
+static void psy_work_item(struct work_struct *work)
+{
+ struct max77759_charger *chg =
+ container_of(work, struct max77759_charger, psy_work);
+ union power_supply_propval current_limit = { 0 }, online = { 0 };
+ int ret;
+
+ power_supply_get_property(chg->tcpm_psy, POWER_SUPPLY_PROP_CURRENT_MAX,
+ ¤t_limit);
+ power_supply_get_property(chg->tcpm_psy, POWER_SUPPLY_PROP_ONLINE,
+ &online);
+
+ if (online.intval && current_limit.intval) {
+ ret = set_input_current_limit(chg, current_limit.intval);
+ if (ret)
+ dev_err(chg->dev,
+ "Unable to set current limit, ret=%d", ret);
+
+ charger_set_mode(chg, MAX77759_CHGR_MODE_CHG_BUCK_ON);
+ } else {
+ charger_set_mode(chg, MAX77759_CHGR_MODE_OFF);
+ }
+}
+
+static int psy_changed(struct notifier_block *nb, unsigned long evt, void *data)
+{
+ struct max77759_charger *chg = container_of(nb, struct max77759_charger,
+ nb);
+ const char *psy_name = "tcpm-source";
+ struct power_supply *psy = data;
+
+ if (!strnstr(psy->desc->name, psy_name, strlen(psy_name)) ||
+ evt != PSY_EVENT_PROP_CHANGED)
+ return NOTIFY_OK;
+
+ chg->tcpm_psy = psy;
+ schedule_work(&chg->psy_work);
+ return NOTIFY_OK;
+}
+
+static void max_tcpci_unregister_psy_notifier(void *nb)
+{
+ power_supply_unreg_notifier(nb);
+}
+
+static int max77759_charger_probe(struct platform_device *pdev)
+{
+ struct regulator_config chgin_otg_reg_cfg;
+ struct power_supply_config psy_cfg;
+ struct device *dev = &pdev->dev;
+ struct max77759_charger *chg;
+ int ret;
+
+ device_set_of_node_from_dev(dev, dev->parent);
+ chg = devm_kzalloc(dev, sizeof(*chg), GFP_KERNEL);
+ if (!chg)
+ return -ENOMEM;
+
+ platform_set_drvdata(pdev, chg);
+ chg->dev = dev;
+ chg->regmap = dev_get_regmap(dev->parent, "charger");
+ if (!chg->regmap)
+ return dev_err_probe(dev, -ENODEV, "Missing regmap");
+
+ ret = devm_mutex_init(dev, &chg->lock);
+ if (ret)
+ return dev_err_probe(dev, ret, "Failed to initialize lock");
+
+ psy_cfg.fwnode = dev_fwnode(dev);
+ psy_cfg.drv_data = chg;
+ chg->psy = devm_power_supply_register(dev, &max77759_charger_desc,
+ &psy_cfg);
+ if (IS_ERR(chg->psy))
+ return dev_err_probe(dev, -EPROBE_DEFER,
+ "Failed to register psy, ret=%ld",
+ PTR_ERR(chg->psy));
+
+ ret = max77759_charger_init(chg);
+ if (ret)
+ return dev_err_probe(dev, ret,
+ "Failed to initialize max77759 charger");
+
+ chgin_otg_reg_cfg.dev = dev;
+ chgin_otg_reg_cfg.driver_data = chg;
+ chgin_otg_reg_cfg.of_node = dev_of_node(dev);
+ chg->chgin_otg_rdev = devm_regulator_register(dev, &chgin_otg_reg_desc,
+ &chgin_otg_reg_cfg);
+ if (IS_ERR(chg->chgin_otg_rdev))
+ return dev_err_probe(dev, PTR_ERR(chg->chgin_otg_rdev),
+ "Failed to register chgin otg regulator");
+
+ ret = devm_work_autocancel(dev, &chg->psy_work, psy_work_item);
+ if (ret)
+ return dev_err_probe(dev, ret, "Failed to initialize psy work");
+
+ chg->nb.notifier_call = psy_changed;
+ ret = power_supply_reg_notifier(&chg->nb);
+ if (ret)
+ return dev_err_probe(dev, ret,
+ "Unable to register psy notifier");
+
+ ret = devm_add_action_or_reset(dev, max_tcpci_unregister_psy_notifier,
+ &chg->nb);
+ if (ret)
+ return ret;
+
+ ret = max77759_init_irqhandler(chg);
+ if (ret)
+ return dev_err_probe(dev, ret,
+ "Unable to initialize irq handler");
+ return 0;
+}
+
+static const struct platform_device_id max77759_charger_id[] = {
+ {"max77759-charger",},
+ { }
+};
+MODULE_DEVICE_TABLE(platform, max77759_charger_id);
+
+static struct platform_driver max77759_charger_driver = {
+ .driver = {
+ .name = "max77759-charger",
+ },
+ .probe = max77759_charger_probe,
+ .id_table = max77759_charger_id,
+};
+module_platform_driver(max77759_charger_driver);
+
+MODULE_AUTHOR("Amit Sunil Dhamne <amitsd@google.com>");
+MODULE_DESCRIPTION("Maxim MAX77759 charger driver");
+MODULE_LICENSE("GPL");
--
2.52.0.351.gbe84eed79e-googHi Amit,
I haven't done a full review, but a few things caught my eye.
On Sat, 2025-12-27 at 00:04 +0000, Amit Sunil Dhamne via B4 Relay wrote:
> 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 | 764 ++++++++++++++++++++++++++++++++
> 4 files changed, 782 insertions(+)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index dc731d37c8fe..26a9654ab75e 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -15539,6 +15539,12 @@ F: drivers/mfd/max77759.c
> F: drivers/nvmem/max77759-nvmem.c
> F: include/linux/mfd/max77759.h
>
> +MAXIM MAX77759 BATTERY CHARGER DRIVER
> +M: Amit Sunil Dhamne <amitsd@google.com>
> +L: linux-kernel@vger.kernel.org
> +S: Maintained
> +F: drivers/power/supply/max77759_charger.c
> +
> MAXIM MAX77802 PMIC REGULATOR DEVICE DRIVER
> M: Javier Martinez Canillas <javier@dowhile0.org>
> L: linux-kernel@vger.kernel.org
> diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
> index 92f9f7aae92f..e172fd980fde 100644
> --- a/drivers/power/supply/Kconfig
> +++ b/drivers/power/supply/Kconfig
> @@ -1132,4 +1132,15 @@ config FUEL_GAUGE_MM8013
> the state of charge, temperature, cycle count, actual and design
> capacity, etc.
>
> +config CHARGER_MAX77759
> + tristate "MAX77759 Charger Driver"
> + depends on MFD_MAX77759 && REGULATOR
> + default MFD_MAX77759
> + help
> + Say M or Y here to enable the MAX77759 Charger Driver. MAX77759
> + charger is a function of the MAX77759 PMIC. This is a dual input
> + switch-mode charger. This driver supports buck and OTG boost modes.
> +
> + If built as a module, it will be called max77759_charger.
> +
It might make sense to add this block near the existing MAX77... charger drivers,
while updating the tristate string and keeping alphabetical order of entries.
> endif # POWER_SUPPLY
> diff --git a/drivers/power/supply/Makefile b/drivers/power/supply/Makefile
> index 4b79d5abc49a..6af905875ad5 100644
> --- a/drivers/power/supply/Makefile
> +++ b/drivers/power/supply/Makefile
> @@ -128,3 +128,4 @@ obj-$(CONFIG_CHARGER_SURFACE) += surface_charger.o
> obj-$(CONFIG_BATTERY_UG3105) += ug3105_battery.o
> obj-$(CONFIG_CHARGER_QCOM_SMB2) += qcom_smbx.o
> obj-$(CONFIG_FUEL_GAUGE_MM8013) += mm8013.o
> +obj-$(CONFIG_CHARGER_MAX77759) += max77759_charger.o
> diff --git a/drivers/power/supply/max77759_charger.c b/drivers/power/supply/max77759_charger.c
> new file mode 100644
> index 000000000000..3d255b069fb9
> --- /dev/null
> +++ b/drivers/power/supply/max77759_charger.c
> @@ -0,0 +1,764 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * max77759_charger.c - Battery charger driver for MAX77759 charger device.
> + *
> + * Copyright 2025 Google LLC.
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/cleanup.h>
> +#include <linux/device.h>
> +#include <linux/devm-helpers.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> +#include <linux/math64.h>
> +#include <linux/mfd/max77759.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/mutex.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/power_supply.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/driver.h>
> +#include <linux/string_choices.h>
> +
> +/* Default values for Fast Charge Current & Float Voltage */
> +#define CHG_CC_DEFAULT_UA 2266770
> +#define CHG_FV_DEFAULT_MV 4300
> +
> +#define FOREACH_IRQ(S) \
> + S(AICL), \
> + S(CHGIN), \
> + S(CHG), \
> + S(INLIM), \
> + S(BAT_OILO), \
> + S(CHG_STA_CC), \
> + S(CHG_STA_CV), \
> + S(CHG_STA_TO), \
> + S(CHG_STA_DONE)
> +
> +#define GENERATE_ENUM(e) e
> +#define GENERATE_STRING(s) #s
> +
> +enum {
> + FOREACH_IRQ(GENERATE_ENUM)
> +};
> +
> +static const char *const chgr_irqs_str[] = {
> + FOREACH_IRQ(GENERATE_STRING)
> +};
> +
> +static int irqs[ARRAY_SIZE(chgr_irqs_str)];
No global variables please, this is not a singleton.
> [...]
>
> +static int set_input_current_limit(struct max77759_charger *chg, int ilim_ua)
> +{
> + u32 regval;
> +
> + if (ilim_ua < 0)
> + return -EINVAL;
> +
> + if (ilim_ua == 0)
> + ilim_ua = MAX77759_CHGR_CHGIN_ILIM_MIN_UA;
> + else if (ilim_ua > MAX77759_CHGR_CHGIN_ILIM_MAX_UA)
> + ilim_ua = MAX77759_CHGR_CHGIN_ILIM_MAX_UA;
What if ilim_ua == 1 (or any other value < min_uA)? You could use clamp()
instead of open-coding.
> +
> + regval = val_to_regval(ilim_ua, MAX77759_CHGR_CHGIN_ILIM_MIN_UA,
> + MAX77759_CHGR_CHGIN_ILIM_STEP_UA,
> + MAX77759_CHGR_CHGIN_ILIM_REG_OFFSET);
> + return regmap_update_bits(chg->regmap, MAX77759_CHGR_REG_CHG_CNFG_09,
> + MAX77759_CHGR_REG_CHG_CNFG_09_CHGIN_ILIM,
> + regval);
> +}
> +
> +static const enum power_supply_property max77759_charger_props[] = {
> + POWER_SUPPLY_PROP_ONLINE,
> + POWER_SUPPLY_PROP_PRESENT,
> + POWER_SUPPLY_PROP_STATUS,
> + POWER_SUPPLY_PROP_CHARGE_TYPE,
> + POWER_SUPPLY_PROP_HEALTH,
> + POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX,
> + POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE_MAX,
> + POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT,
> +};
> +
> +static int max77759_charger_get_property(struct power_supply *psy,
> + enum power_supply_property psp,
> + union power_supply_propval *pval)
> +{
> + struct max77759_charger *chg = power_supply_get_drvdata(psy);
> + int ret;
> +
> + switch (psp) {
> + case POWER_SUPPLY_PROP_ONLINE:
> + ret = get_online(chg);
> + break;
> + case POWER_SUPPLY_PROP_PRESENT:
> + ret = charger_input_valid(chg);
> + break;
> + case POWER_SUPPLY_PROP_STATUS:
> + ret = get_status(chg);
> + break;
> + case POWER_SUPPLY_PROP_CHARGE_TYPE:
> + ret = get_charge_type(chg);
> + break;
> + case POWER_SUPPLY_PROP_HEALTH:
> + ret = get_health(chg);
> + break;
> + case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX:
> + ret = get_fast_charge_current(chg);
> + break;
> + case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE_MAX:
> + ret = get_float_voltage(chg);
> + break;
> + case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT:
> + ret = get_input_current_limit(chg);
> + break;
> + default:
> + ret = -EINVAL;
> + }
> +
> + pval->intval = ret;
> + return ret < 0 ? ret : 0;
> +}
> +
> +static const struct power_supply_desc max77759_charger_desc = {
> + .name = "max77759-charger",
> + .type = POWER_SUPPLY_TYPE_USB,
> + .properties = max77759_charger_props,
> + .num_properties = ARRAY_SIZE(max77759_charger_props),
> + .get_property = max77759_charger_get_property,
> +};
> +
> +static int charger_set_mode(struct max77759_charger *chg,
> + enum max77759_chgr_mode mode)
> +{
> + int ret;
> +
> + guard(mutex)(&chg->lock);
> +
> + if (chg->mode == mode)
> + return 0;
> +
> + if ((mode == MAX77759_CHGR_MODE_CHG_BUCK_ON ||
> + mode == MAX77759_CHGR_MODE_OTG_BOOST_ON) &&
> + chg->mode != MAX77759_CHGR_MODE_OFF) {
> + dev_err(chg->dev, "Invalid mode transition from %d to %d",
> + chg->mode, mode);
> + return -EINVAL;
> + }
> +
> + ret = regmap_update_bits(chg->regmap, MAX77759_CHGR_REG_CHG_CNFG_00,
> + MAX77759_CHGR_REG_CHG_CNFG_00_MODE, mode);
> + if (ret)
> + return ret;
> +
> + chg->mode = mode;
> + return 0;
> +}
> +
> +static int enable_chgin_otg(struct regulator_dev *rdev)
> +{
> + struct max77759_charger *chg = rdev_get_drvdata(rdev);
> +
> + return charger_set_mode(chg, MAX77759_CHGR_MODE_OTG_BOOST_ON);
> +}
> +
> +static int disable_chgin_otg(struct regulator_dev *rdev)
> +{
> + struct max77759_charger *chg = rdev_get_drvdata(rdev);
> +
> + return charger_set_mode(chg, MAX77759_CHGR_MODE_OFF);
> +}
> +
> +static int chgin_otg_status(struct regulator_dev *rdev)
> +{
> + struct max77759_charger *chg = rdev_get_drvdata(rdev);
> +
> + guard(mutex)(&chg->lock);
> + return chg->mode == MAX77759_CHGR_MODE_OTG_BOOST_ON;
> +}
> +
> +static const struct regulator_ops chgin_otg_reg_ops = {
> + .enable = enable_chgin_otg,
> + .disable = disable_chgin_otg,
> + .is_enabled = chgin_otg_status,
> +};
> +
> +static const struct regulator_desc chgin_otg_reg_desc = {
> + .name = "chgin-otg",
> + .of_match = of_match_ptr("chgin-otg-regulator"),
> + .owner = THIS_MODULE,
> + .ops = &chgin_otg_reg_ops,
> + .fixed_uV = 5000000,
> + .n_voltages = 1,
> +};
> +
> +static irqreturn_t irq_handler(int irq, void *data)
> +{
> + struct max77759_charger *chg = data;
> + struct device *dev = chg->dev;
> + u32 chgint_ok;
> + int i;
> +
> + regmap_read(chg->regmap, MAX77759_CHGR_REG_CHG_INT_OK, &chgint_ok);
You might want to check the return value and return IRQ_NONE if it didn't
work?
> +
> + for (i = 0; i < ARRAY_SIZE(irqs); i++) {
> + if (irqs[i] == irq)
> + break;
> + }
> +
> + switch (i) {
> + case AICL:
> + dev_dbg(dev, "AICL mode: %s",
> + str_no_yes(chgint_ok & MAX77759_CHGR_REG_CHG_INT_AICL));
> + break;
> + case CHGIN:
> + dev_dbg(dev, "CHGIN input valid: %s",
> + str_yes_no(chgint_ok & MAX77759_CHGR_REG_CHG_INT_CHGIN));
> + break;
> + case CHG:
> + dev_dbg(dev, "CHG status okay/off: %s",
> + str_yes_no(chgint_ok & MAX77759_CHGR_REG_CHG_INT_CHG));
> + break;
> + case INLIM:
> + dev_dbg(dev, "Current Limit reached: %s",
> + str_no_yes(chgint_ok & MAX77759_CHGR_REG_CHG_INT_INLIM));
> + break;
> + case BAT_OILO:
> + dev_dbg(dev, "Battery over-current threshold crossed");
> + break;
> + case CHG_STA_CC:
> + dev_dbg(dev, "Charger reached CC stage");
> + break;
> + case CHG_STA_CV:
> + dev_dbg(dev, "Charger reached CV stage");
> + break;
> + case CHG_STA_TO:
> + dev_dbg(dev, "Charger reached TO stage");
> + break;
> + case CHG_STA_DONE:
> + dev_dbg(dev, "Charger reached TO stage");
> + break;
Are the above debug messages really all needed?
> + default:
> + dev_err(dev, "Unrecognized irq: %d", i);
> + return IRQ_HANDLED;
I'm not sure it should return IRQ_HANDLED in this case.
> + }
> +
> + power_supply_changed(chg->psy);
> + return IRQ_HANDLED;
> +}
> +
> +static int max77759_init_irqhandler(struct max77759_charger *chg)
> +{
> + struct device *dev = chg->dev;
> + unsigned long irq_flags;
> + struct irq_data *irqd;
> + int i, ret;
> +
> + for (i = 0; i < ARRAY_SIZE(chgr_irqs_str); i++) {
> + ret = platform_get_irq_byname(to_platform_device(dev),
> + chgr_irqs_str[i]);
> + if (ret < 0) {
> + dev_err(dev,
> + "Failed to get irq resource for %s, ret=%d",
> + chgr_irqs_str[i], ret);
> + return ret;
> + }
You should use return dev_err_probe() here, and drop the additional dev_err_probe()
in max77759_charger_probe().
> +
> + irqs[i] = ret;
> + irq_flags = IRQF_ONESHOT;
> + irqd = irq_get_irq_data(irqs[i]);
> + if (irqd)
> + irq_flags |= irqd_get_trigger_type(irqd);
The above three lines are not needed, and then you can also drop irq_flags and
use its value in the below call directly.
> +
> + ret = devm_request_threaded_irq(dev, irqs[i], NULL, irq_handler,
> + irq_flags, dev_name(dev), chg);
> + if (ret) {
> + dev_err(dev,
> + "Unable to register irq handler for %s, ret=%d",
> + chgr_irqs_str[i], ret);
> + return ret;
dev_err_probe() please.
> + }
> + }
> +
> + return 0;
> +}
> +
> +static int max77759_charger_init(struct max77759_charger *chg)
> +{
> + struct power_supply_battery_info *info;
> + u32 regval, fast_chg_curr, fv;
> + int ret;
> +
> + regmap_read(chg->regmap, MAX77759_CHGR_REG_CHG_CNFG_00, ®val);
> + 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 */
> + regmap_update_bits(chg->regmap, MAX77759_CHGR_REG_CHG_CNFG_12,
> + MAX77759_CHGR_REG_CHG_CNFG_12_WCINSEL, 0);
> +
> + regmap_update_bits(chg->regmap, MAX77759_CHGR_REG_CHG_CNFG_18,
> + MAX77759_CHGR_REG_CHG_CNFG_18_WDTEN, 0);
I think it's good practice to check return values.
> +
> + return unlock_prot_regs(chg, false);
> +}
> +
> +static void psy_work_item(struct work_struct *work)
> +{
> + struct max77759_charger *chg =
> + container_of(work, struct max77759_charger, psy_work);
> + union power_supply_propval current_limit = { 0 }, online = { 0 };
> + int ret;
> +
> + power_supply_get_property(chg->tcpm_psy, POWER_SUPPLY_PROP_CURRENT_MAX,
> + ¤t_limit);
> + power_supply_get_property(chg->tcpm_psy, POWER_SUPPLY_PROP_ONLINE,
> + &online);
Would it make sense to rework this and check the return values? Then you can also
drop the greedy init at function entry.
> +
> + if (online.intval && current_limit.intval) {
> + ret = set_input_current_limit(chg, current_limit.intval);
> + if (ret)
> + dev_err(chg->dev,
> + "Unable to set current limit, ret=%d", ret);
> +
> + charger_set_mode(chg, MAX77759_CHGR_MODE_CHG_BUCK_ON);
> + } else {
> + charger_set_mode(chg, MAX77759_CHGR_MODE_OFF);
> + }
> +}
> +
> +static int psy_changed(struct notifier_block *nb, unsigned long evt, void *data)
> +{
> + struct max77759_charger *chg = container_of(nb, struct max77759_charger,
> + nb);
> + const char *psy_name = "tcpm-source";
> + struct power_supply *psy = data;
> +
> + if (!strnstr(psy->desc->name, psy_name, strlen(psy_name)) ||
> + evt != PSY_EVENT_PROP_CHANGED)
> + return NOTIFY_OK;
> +
> + chg->tcpm_psy = psy;
> + schedule_work(&chg->psy_work);
Maybe add a newline here.
> + return NOTIFY_OK;
> +}
> +
> +static void max_tcpci_unregister_psy_notifier(void *nb)
> +{
> + power_supply_unreg_notifier(nb);
> +}
> +
> +static int max77759_charger_probe(struct platform_device *pdev)
> +{
> + struct regulator_config chgin_otg_reg_cfg;
> + struct power_supply_config psy_cfg;
> + struct device *dev = &pdev->dev;
> + struct max77759_charger *chg;
> + int ret;
> +
> + device_set_of_node_from_dev(dev, dev->parent);
> + chg = devm_kzalloc(dev, sizeof(*chg), GFP_KERNEL);
> + if (!chg)
> + return -ENOMEM;
> +
> + platform_set_drvdata(pdev, chg);
> + chg->dev = dev;
> + chg->regmap = dev_get_regmap(dev->parent, "charger");
> + if (!chg->regmap)
> + return dev_err_probe(dev, -ENODEV, "Missing regmap");
> +
> + ret = devm_mutex_init(dev, &chg->lock);
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to initialize lock");
> +
> + psy_cfg.fwnode = dev_fwnode(dev);
> + psy_cfg.drv_data = chg;
> + chg->psy = devm_power_supply_register(dev, &max77759_charger_desc,
> + &psy_cfg);
> + if (IS_ERR(chg->psy))
> + return dev_err_probe(dev, -EPROBE_DEFER,
> + "Failed to register psy, ret=%ld",
> + PTR_ERR(chg->psy));
> +
> + ret = max77759_charger_init(chg);
> + if (ret)
> + return dev_err_probe(dev, ret,
> + "Failed to initialize max77759 charger");
> +
> + chgin_otg_reg_cfg.dev = dev;
> + chgin_otg_reg_cfg.driver_data = chg;
> + chgin_otg_reg_cfg.of_node = dev_of_node(dev);
> + chg->chgin_otg_rdev = devm_regulator_register(dev, &chgin_otg_reg_desc,
> + &chgin_otg_reg_cfg);
> + if (IS_ERR(chg->chgin_otg_rdev))
> + return dev_err_probe(dev, PTR_ERR(chg->chgin_otg_rdev),
> + "Failed to register chgin otg regulator");
> +
> + ret = devm_work_autocancel(dev, &chg->psy_work, psy_work_item);
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to initialize psy work");
> +
> + chg->nb.notifier_call = psy_changed;
> + ret = power_supply_reg_notifier(&chg->nb);
> + if (ret)
> + return dev_err_probe(dev, ret,
> + "Unable to register psy notifier");
> +
> + ret = devm_add_action_or_reset(dev, max_tcpci_unregister_psy_notifier,
> + &chg->nb);
> + if (ret)
> + return ret;
You could print a message here as well.
Cheers,
Andre'
Hi Andre',
On 1/5/26 9:32 AM, André Draszik wrote:
> Hi Amit,
>
> I haven't done a full review, but a few things caught my eye.
>
> On Sat, 2025-12-27 at 00:04 +0000, Amit Sunil Dhamne via B4 Relay wrote:
>> 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 | 764 ++++++++++++++++++++++++++++++++
>> 4 files changed, 782 insertions(+)
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index dc731d37c8fe..26a9654ab75e 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -15539,6 +15539,12 @@ F: drivers/mfd/max77759.c
>> F: drivers/nvmem/max77759-nvmem.c
>> F: include/linux/mfd/max77759.h
>>
>> +MAXIM MAX77759 BATTERY CHARGER DRIVER
>> +M: Amit Sunil Dhamne <amitsd@google.com>
>> +L: linux-kernel@vger.kernel.org
>> +S: Maintained
>> +F: drivers/power/supply/max77759_charger.c
>> +
>> MAXIM MAX77802 PMIC REGULATOR DEVICE DRIVER
>> M: Javier Martinez Canillas <javier@dowhile0.org>
>> L: linux-kernel@vger.kernel.org
>> diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
>> index 92f9f7aae92f..e172fd980fde 100644
>> --- a/drivers/power/supply/Kconfig
>> +++ b/drivers/power/supply/Kconfig
>> @@ -1132,4 +1132,15 @@ config FUEL_GAUGE_MM8013
>> the state of charge, temperature, cycle count, actual and design
>> capacity, etc.
>>
>> +config CHARGER_MAX77759
>> + tristate "MAX77759 Charger Driver"
>> + depends on MFD_MAX77759 && REGULATOR
>> + default MFD_MAX77759
>> + help
>> + Say M or Y here to enable the MAX77759 Charger Driver. MAX77759
>> + charger is a function of the MAX77759 PMIC. This is a dual input
>> + switch-mode charger. This driver supports buck and OTG boost modes.
>> +
>> + If built as a module, it will be called max77759_charger.
>> +
> It might make sense to add this block near the existing MAX77... charger drivers,
> while updating the tristate string and keeping alphabetical order of entries.
>
>> endif # POWER_SUPPLY
>> diff --git a/drivers/power/supply/Makefile b/drivers/power/supply/Makefile
>> index 4b79d5abc49a..6af905875ad5 100644
>> --- a/drivers/power/supply/Makefile
>> +++ b/drivers/power/supply/Makefile
>> @@ -128,3 +128,4 @@ obj-$(CONFIG_CHARGER_SURFACE) += surface_charger.o
>> obj-$(CONFIG_BATTERY_UG3105) += ug3105_battery.o
>> obj-$(CONFIG_CHARGER_QCOM_SMB2) += qcom_smbx.o
>> obj-$(CONFIG_FUEL_GAUGE_MM8013) += mm8013.o
>> +obj-$(CONFIG_CHARGER_MAX77759) += max77759_charger.o
>> diff --git a/drivers/power/supply/max77759_charger.c b/drivers/power/supply/max77759_charger.c
>> new file mode 100644
>> index 000000000000..3d255b069fb9
>> --- /dev/null
>> +++ b/drivers/power/supply/max77759_charger.c
>> @@ -0,0 +1,764 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * max77759_charger.c - Battery charger driver for MAX77759 charger device.
>> + *
>> + * Copyright 2025 Google LLC.
>> + */
>> +
>> +#include <linux/bitfield.h>
>> +#include <linux/cleanup.h>
>> +#include <linux/device.h>
>> +#include <linux/devm-helpers.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/irq.h>
>> +#include <linux/math64.h>
>> +#include <linux/mfd/max77759.h>
>> +#include <linux/module.h>
>> +#include <linux/mod_devicetable.h>
>> +#include <linux/mutex.h>
>> +#include <linux/of.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/power_supply.h>
>> +#include <linux/regmap.h>
>> +#include <linux/regulator/driver.h>
>> +#include <linux/string_choices.h>
>> +
>> +/* Default values for Fast Charge Current & Float Voltage */
>> +#define CHG_CC_DEFAULT_UA 2266770
>> +#define CHG_FV_DEFAULT_MV 4300
>> +
>> +#define FOREACH_IRQ(S) \
>> + S(AICL), \
>> + S(CHGIN), \
>> + S(CHG), \
>> + S(INLIM), \
>> + S(BAT_OILO), \
>> + S(CHG_STA_CC), \
>> + S(CHG_STA_CV), \
>> + S(CHG_STA_TO), \
>> + S(CHG_STA_DONE)
>> +
>> +#define GENERATE_ENUM(e) e
>> +#define GENERATE_STRING(s) #s
>> +
>> +enum {
>> + FOREACH_IRQ(GENERATE_ENUM)
>> +};
>> +
>> +static const char *const chgr_irqs_str[] = {
>> + FOREACH_IRQ(GENERATE_STRING)
>> +};
>> +
>> +static int irqs[ARRAY_SIZE(chgr_irqs_str)];
> No global variables please, this is not a singleton.
>
>> [...]
>>
>> +static int set_input_current_limit(struct max77759_charger *chg, int ilim_ua)
>> +{
>> + u32 regval;
>> +
>> + if (ilim_ua < 0)
>> + return -EINVAL;
>> +
>> + if (ilim_ua == 0)
>> + ilim_ua = MAX77759_CHGR_CHGIN_ILIM_MIN_UA;
>> + else if (ilim_ua > MAX77759_CHGR_CHGIN_ILIM_MAX_UA)
>> + ilim_ua = MAX77759_CHGR_CHGIN_ILIM_MAX_UA;
> What if ilim_ua == 1 (or any other value < min_uA)? You could use clamp()
> instead of open-coding.
>
>> +
>> + regval = val_to_regval(ilim_ua, MAX77759_CHGR_CHGIN_ILIM_MIN_UA,
>> + MAX77759_CHGR_CHGIN_ILIM_STEP_UA,
>> + MAX77759_CHGR_CHGIN_ILIM_REG_OFFSET);
>> + return regmap_update_bits(chg->regmap, MAX77759_CHGR_REG_CHG_CNFG_09,
>> + MAX77759_CHGR_REG_CHG_CNFG_09_CHGIN_ILIM,
>> + regval);
>> +}
>> +
>> +static const enum power_supply_property max77759_charger_props[] = {
>> + POWER_SUPPLY_PROP_ONLINE,
>> + POWER_SUPPLY_PROP_PRESENT,
>> + POWER_SUPPLY_PROP_STATUS,
>> + POWER_SUPPLY_PROP_CHARGE_TYPE,
>> + POWER_SUPPLY_PROP_HEALTH,
>> + POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX,
>> + POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE_MAX,
>> + POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT,
>> +};
>> +
>> +static int max77759_charger_get_property(struct power_supply *psy,
>> + enum power_supply_property psp,
>> + union power_supply_propval *pval)
>> +{
>> + struct max77759_charger *chg = power_supply_get_drvdata(psy);
>> + int ret;
>> +
>> + switch (psp) {
>> + case POWER_SUPPLY_PROP_ONLINE:
>> + ret = get_online(chg);
>> + break;
>> + case POWER_SUPPLY_PROP_PRESENT:
>> + ret = charger_input_valid(chg);
>> + break;
>> + case POWER_SUPPLY_PROP_STATUS:
>> + ret = get_status(chg);
>> + break;
>> + case POWER_SUPPLY_PROP_CHARGE_TYPE:
>> + ret = get_charge_type(chg);
>> + break;
>> + case POWER_SUPPLY_PROP_HEALTH:
>> + ret = get_health(chg);
>> + break;
>> + case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX:
>> + ret = get_fast_charge_current(chg);
>> + break;
>> + case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE_MAX:
>> + ret = get_float_voltage(chg);
>> + break;
>> + case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT:
>> + ret = get_input_current_limit(chg);
>> + break;
>> + default:
>> + ret = -EINVAL;
>> + }
>> +
>> + pval->intval = ret;
>> + return ret < 0 ? ret : 0;
>> +}
>> +
>> +static const struct power_supply_desc max77759_charger_desc = {
>> + .name = "max77759-charger",
>> + .type = POWER_SUPPLY_TYPE_USB,
>> + .properties = max77759_charger_props,
>> + .num_properties = ARRAY_SIZE(max77759_charger_props),
>> + .get_property = max77759_charger_get_property,
>> +};
>> +
>> +static int charger_set_mode(struct max77759_charger *chg,
>> + enum max77759_chgr_mode mode)
>> +{
>> + int ret;
>> +
>> + guard(mutex)(&chg->lock);
>> +
>> + if (chg->mode == mode)
>> + return 0;
>> +
>> + if ((mode == MAX77759_CHGR_MODE_CHG_BUCK_ON ||
>> + mode == MAX77759_CHGR_MODE_OTG_BOOST_ON) &&
>> + chg->mode != MAX77759_CHGR_MODE_OFF) {
>> + dev_err(chg->dev, "Invalid mode transition from %d to %d",
>> + chg->mode, mode);
>> + return -EINVAL;
>> + }
>> +
>> + ret = regmap_update_bits(chg->regmap, MAX77759_CHGR_REG_CHG_CNFG_00,
>> + MAX77759_CHGR_REG_CHG_CNFG_00_MODE, mode);
>> + if (ret)
>> + return ret;
>> +
>> + chg->mode = mode;
>> + return 0;
>> +}
>> +
>> +static int enable_chgin_otg(struct regulator_dev *rdev)
>> +{
>> + struct max77759_charger *chg = rdev_get_drvdata(rdev);
>> +
>> + return charger_set_mode(chg, MAX77759_CHGR_MODE_OTG_BOOST_ON);
>> +}
>> +
>> +static int disable_chgin_otg(struct regulator_dev *rdev)
>> +{
>> + struct max77759_charger *chg = rdev_get_drvdata(rdev);
>> +
>> + return charger_set_mode(chg, MAX77759_CHGR_MODE_OFF);
>> +}
>> +
>> +static int chgin_otg_status(struct regulator_dev *rdev)
>> +{
>> + struct max77759_charger *chg = rdev_get_drvdata(rdev);
>> +
>> + guard(mutex)(&chg->lock);
>> + return chg->mode == MAX77759_CHGR_MODE_OTG_BOOST_ON;
>> +}
>> +
>> +static const struct regulator_ops chgin_otg_reg_ops = {
>> + .enable = enable_chgin_otg,
>> + .disable = disable_chgin_otg,
>> + .is_enabled = chgin_otg_status,
>> +};
>> +
>> +static const struct regulator_desc chgin_otg_reg_desc = {
>> + .name = "chgin-otg",
>> + .of_match = of_match_ptr("chgin-otg-regulator"),
>> + .owner = THIS_MODULE,
>> + .ops = &chgin_otg_reg_ops,
>> + .fixed_uV = 5000000,
>> + .n_voltages = 1,
>> +};
>> +
>> +static irqreturn_t irq_handler(int irq, void *data)
>> +{
>> + struct max77759_charger *chg = data;
>> + struct device *dev = chg->dev;
>> + u32 chgint_ok;
>> + int i;
>> +
>> + regmap_read(chg->regmap, MAX77759_CHGR_REG_CHG_INT_OK, &chgint_ok);
> You might want to check the return value and return IRQ_NONE if it didn't
> work?
>
>> +
>> + for (i = 0; i < ARRAY_SIZE(irqs); i++) {
>> + if (irqs[i] == irq)
>> + break;
>> + }
>> +
>> + switch (i) {
>> + case AICL:
>> + dev_dbg(dev, "AICL mode: %s",
>> + str_no_yes(chgint_ok & MAX77759_CHGR_REG_CHG_INT_AICL));
>> + break;
>> + case CHGIN:
>> + dev_dbg(dev, "CHGIN input valid: %s",
>> + str_yes_no(chgint_ok & MAX77759_CHGR_REG_CHG_INT_CHGIN));
>> + break;
>> + case CHG:
>> + dev_dbg(dev, "CHG status okay/off: %s",
>> + str_yes_no(chgint_ok & MAX77759_CHGR_REG_CHG_INT_CHG));
>> + break;
>> + case INLIM:
>> + dev_dbg(dev, "Current Limit reached: %s",
>> + str_no_yes(chgint_ok & MAX77759_CHGR_REG_CHG_INT_INLIM));
>> + break;
>> + case BAT_OILO:
>> + dev_dbg(dev, "Battery over-current threshold crossed");
>> + break;
>> + case CHG_STA_CC:
>> + dev_dbg(dev, "Charger reached CC stage");
>> + break;
>> + case CHG_STA_CV:
>> + dev_dbg(dev, "Charger reached CV stage");
>> + break;
>> + case CHG_STA_TO:
>> + dev_dbg(dev, "Charger reached TO stage");
>> + break;
>> + case CHG_STA_DONE:
>> + dev_dbg(dev, "Charger reached TO stage");
>> + break;
> Are the above debug messages really all needed?
>
>> + default:
>> + dev_err(dev, "Unrecognized irq: %d", i);
>> + return IRQ_HANDLED;
> I'm not sure it should return IRQ_HANDLED in this case.
>
>> + }
>> +
>> + power_supply_changed(chg->psy);
>> + return IRQ_HANDLED;
>> +}
>> +
>> +static int max77759_init_irqhandler(struct max77759_charger *chg)
>> +{
>> + struct device *dev = chg->dev;
>> + unsigned long irq_flags;
>> + struct irq_data *irqd;
>> + int i, ret;
>> +
>> + for (i = 0; i < ARRAY_SIZE(chgr_irqs_str); i++) {
>> + ret = platform_get_irq_byname(to_platform_device(dev),
>> + chgr_irqs_str[i]);
>> + if (ret < 0) {
>> + dev_err(dev,
>> + "Failed to get irq resource for %s, ret=%d",
>> + chgr_irqs_str[i], ret);
>> + return ret;
>> + }
> You should use return dev_err_probe() here, and drop the additional dev_err_probe()
> in max77759_charger_probe().
>
>> +
>> + irqs[i] = ret;
>> + irq_flags = IRQF_ONESHOT;
>> + irqd = irq_get_irq_data(irqs[i]);
>> + if (irqd)
>> + irq_flags |= irqd_get_trigger_type(irqd);
> The above three lines are not needed, and then you can also drop irq_flags and
> use its value in the below call directly.
>
>> +
>> + ret = devm_request_threaded_irq(dev, irqs[i], NULL, irq_handler,
>> + irq_flags, dev_name(dev), chg);
>> + if (ret) {
>> + dev_err(dev,
>> + "Unable to register irq handler for %s, ret=%d",
>> + chgr_irqs_str[i], ret);
>> + return ret;
> dev_err_probe() please.
>
>> + }
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int max77759_charger_init(struct max77759_charger *chg)
>> +{
>> + struct power_supply_battery_info *info;
>> + u32 regval, fast_chg_curr, fv;
>> + int ret;
>> +
>> + regmap_read(chg->regmap, MAX77759_CHGR_REG_CHG_CNFG_00, ®val);
>> + 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 */
>> + regmap_update_bits(chg->regmap, MAX77759_CHGR_REG_CHG_CNFG_12,
>> + MAX77759_CHGR_REG_CHG_CNFG_12_WCINSEL, 0);
>> +
>> + regmap_update_bits(chg->regmap, MAX77759_CHGR_REG_CHG_CNFG_18,
>> + MAX77759_CHGR_REG_CHG_CNFG_18_WDTEN, 0);
> I think it's good practice to check return values.
>
>> +
>> + return unlock_prot_regs(chg, false);
>> +}
>> +
>> +static void psy_work_item(struct work_struct *work)
>> +{
>> + struct max77759_charger *chg =
>> + container_of(work, struct max77759_charger, psy_work);
>> + union power_supply_propval current_limit = { 0 }, online = { 0 };
>> + int ret;
>> +
>> + power_supply_get_property(chg->tcpm_psy, POWER_SUPPLY_PROP_CURRENT_MAX,
>> + ¤t_limit);
>> + power_supply_get_property(chg->tcpm_psy, POWER_SUPPLY_PROP_ONLINE,
>> + &online);
> Would it make sense to rework this and check the return values? Then you can also
> drop the greedy init at function entry.
>
>> +
>> + if (online.intval && current_limit.intval) {
>> + ret = set_input_current_limit(chg, current_limit.intval);
>> + if (ret)
>> + dev_err(chg->dev,
>> + "Unable to set current limit, ret=%d", ret);
>> +
>> + charger_set_mode(chg, MAX77759_CHGR_MODE_CHG_BUCK_ON);
>> + } else {
>> + charger_set_mode(chg, MAX77759_CHGR_MODE_OFF);
>> + }
>> +}
>> +
>> +static int psy_changed(struct notifier_block *nb, unsigned long evt, void *data)
>> +{
>> + struct max77759_charger *chg = container_of(nb, struct max77759_charger,
>> + nb);
>> + const char *psy_name = "tcpm-source";
>> + struct power_supply *psy = data;
>> +
>> + if (!strnstr(psy->desc->name, psy_name, strlen(psy_name)) ||
>> + evt != PSY_EVENT_PROP_CHANGED)
>> + return NOTIFY_OK;
>> +
>> + chg->tcpm_psy = psy;
>> + schedule_work(&chg->psy_work);
> Maybe add a newline here.
>
>> + return NOTIFY_OK;
>> +}
>> +
>> +static void max_tcpci_unregister_psy_notifier(void *nb)
>> +{
>> + power_supply_unreg_notifier(nb);
>> +}
>> +
>> +static int max77759_charger_probe(struct platform_device *pdev)
>> +{
>> + struct regulator_config chgin_otg_reg_cfg;
>> + struct power_supply_config psy_cfg;
>> + struct device *dev = &pdev->dev;
>> + struct max77759_charger *chg;
>> + int ret;
>> +
>> + device_set_of_node_from_dev(dev, dev->parent);
>> + chg = devm_kzalloc(dev, sizeof(*chg), GFP_KERNEL);
>> + if (!chg)
>> + return -ENOMEM;
>> +
>> + platform_set_drvdata(pdev, chg);
>> + chg->dev = dev;
>> + chg->regmap = dev_get_regmap(dev->parent, "charger");
>> + if (!chg->regmap)
>> + return dev_err_probe(dev, -ENODEV, "Missing regmap");
>> +
>> + ret = devm_mutex_init(dev, &chg->lock);
>> + if (ret)
>> + return dev_err_probe(dev, ret, "Failed to initialize lock");
>> +
>> + psy_cfg.fwnode = dev_fwnode(dev);
>> + psy_cfg.drv_data = chg;
>> + chg->psy = devm_power_supply_register(dev, &max77759_charger_desc,
>> + &psy_cfg);
>> + if (IS_ERR(chg->psy))
>> + return dev_err_probe(dev, -EPROBE_DEFER,
>> + "Failed to register psy, ret=%ld",
>> + PTR_ERR(chg->psy));
>> +
>> + ret = max77759_charger_init(chg);
>> + if (ret)
>> + return dev_err_probe(dev, ret,
>> + "Failed to initialize max77759 charger");
>> +
>> + chgin_otg_reg_cfg.dev = dev;
>> + chgin_otg_reg_cfg.driver_data = chg;
>> + chgin_otg_reg_cfg.of_node = dev_of_node(dev);
>> + chg->chgin_otg_rdev = devm_regulator_register(dev, &chgin_otg_reg_desc,
>> + &chgin_otg_reg_cfg);
>> + if (IS_ERR(chg->chgin_otg_rdev))
>> + return dev_err_probe(dev, PTR_ERR(chg->chgin_otg_rdev),
>> + "Failed to register chgin otg regulator");
>> +
>> + ret = devm_work_autocancel(dev, &chg->psy_work, psy_work_item);
>> + if (ret)
>> + return dev_err_probe(dev, ret, "Failed to initialize psy work");
>> +
>> + chg->nb.notifier_call = psy_changed;
>> + ret = power_supply_reg_notifier(&chg->nb);
>> + if (ret)
>> + return dev_err_probe(dev, ret,
>> + "Unable to register psy notifier");
>> +
>> + ret = devm_add_action_or_reset(dev, max_tcpci_unregister_psy_notifier,
>> + &chg->nb);
>> + if (ret)
>> + return ret;
> You could print a message here as well.
Thanks for the detailed review! I will fix all the flagged issues in the
next rev.
BR,
Amit
>
> Cheers,
> Andre'
On 1/6/26 3:41 PM, Amit Sunil Dhamne wrote:
> Hi Andre',
>
> On 1/5/26 9:32 AM, André Draszik wrote:
>> Hi Amit,
>>
>> I haven't done a full review, but a few things caught my eye.
>>
>> On Sat, 2025-12-27 at 00:04 +0000, Amit Sunil Dhamne via B4 Relay wrote:
>>> 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 | 764
>>> ++++++++++++++++++++++++++++++++
>>> 4 files changed, 782 insertions(+)
>>>
>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>> index dc731d37c8fe..26a9654ab75e 100644
>>> --- a/MAINTAINERS
>>> +++ b/MAINTAINERS
>>> @@ -15539,6 +15539,12 @@ F: drivers/mfd/max77759.c
>>> F: drivers/nvmem/max77759-nvmem.c
>>> F: include/linux/mfd/max77759.h
>>> +MAXIM MAX77759 BATTERY CHARGER DRIVER
>>> +M: Amit Sunil Dhamne <amitsd@google.com>
>>> +L: linux-kernel@vger.kernel.org
>>> +S: Maintained
>>> +F: drivers/power/supply/max77759_charger.c
>>> +
>>> MAXIM MAX77802 PMIC REGULATOR DEVICE DRIVER
>>> M: Javier Martinez Canillas <javier@dowhile0.org>
>>> L: linux-kernel@vger.kernel.org
>>> diff --git a/drivers/power/supply/Kconfig
>>> b/drivers/power/supply/Kconfig
>>> index 92f9f7aae92f..e172fd980fde 100644
>>> --- a/drivers/power/supply/Kconfig
>>> +++ b/drivers/power/supply/Kconfig
>>> @@ -1132,4 +1132,15 @@ config FUEL_GAUGE_MM8013
>>> the state of charge, temperature, cycle count, actual and
>>> design
>>> capacity, etc.
>>> +config CHARGER_MAX77759
>>> + tristate "MAX77759 Charger Driver"
>>> + depends on MFD_MAX77759 && REGULATOR
>>> + default MFD_MAX77759
>>> + help
>>> + Say M or Y here to enable the MAX77759 Charger Driver. MAX77759
>>> + charger is a function of the MAX77759 PMIC. This is a dual input
>>> + switch-mode charger. This driver supports buck and OTG boost
>>> modes.
>>> +
>>> + If built as a module, it will be called max77759_charger.
>>> +
>> It might make sense to add this block near the existing MAX77...
>> charger drivers,
>> while updating the tristate string and keeping alphabetical order of
>> entries.
>>
>>> endif # POWER_SUPPLY
>>> diff --git a/drivers/power/supply/Makefile
>>> b/drivers/power/supply/Makefile
>>> index 4b79d5abc49a..6af905875ad5 100644
>>> --- a/drivers/power/supply/Makefile
>>> +++ b/drivers/power/supply/Makefile
>>> @@ -128,3 +128,4 @@ obj-$(CONFIG_CHARGER_SURFACE) +=
>>> surface_charger.o
>>> obj-$(CONFIG_BATTERY_UG3105) += ug3105_battery.o
>>> obj-$(CONFIG_CHARGER_QCOM_SMB2) += qcom_smbx.o
>>> obj-$(CONFIG_FUEL_GAUGE_MM8013) += mm8013.o
>>> +obj-$(CONFIG_CHARGER_MAX77759) += max77759_charger.o
>>> diff --git a/drivers/power/supply/max77759_charger.c
>>> b/drivers/power/supply/max77759_charger.c
>>> new file mode 100644
>>> index 000000000000..3d255b069fb9
>>> --- /dev/null
>>> +++ b/drivers/power/supply/max77759_charger.c
>>> @@ -0,0 +1,764 @@
>>> +// SPDX-License-Identifier: GPL-2.0-only
>>> +/*
>>> + * max77759_charger.c - Battery charger driver for MAX77759 charger
>>> device.
>>> + *
>>> + * Copyright 2025 Google LLC.
>>> + */
>>> +
>>> +#include <linux/bitfield.h>
>>> +#include <linux/cleanup.h>
>>> +#include <linux/device.h>
>>> +#include <linux/devm-helpers.h>
>>> +#include <linux/interrupt.h>
>>> +#include <linux/irq.h>
>>> +#include <linux/math64.h>
>>> +#include <linux/mfd/max77759.h>
>>> +#include <linux/module.h>
>>> +#include <linux/mod_devicetable.h>
>>> +#include <linux/mutex.h>
>>> +#include <linux/of.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/power_supply.h>
>>> +#include <linux/regmap.h>
>>> +#include <linux/regulator/driver.h>
>>> +#include <linux/string_choices.h>
>>> +
>>> +/* Default values for Fast Charge Current & Float Voltage */
>>> +#define CHG_CC_DEFAULT_UA 2266770
>>> +#define CHG_FV_DEFAULT_MV 4300
>>> +
>>> +#define FOREACH_IRQ(S) \
>>> + S(AICL), \
>>> + S(CHGIN), \
>>> + S(CHG), \
>>> + S(INLIM), \
>>> + S(BAT_OILO), \
>>> + S(CHG_STA_CC), \
>>> + S(CHG_STA_CV), \
>>> + S(CHG_STA_TO), \
>>> + S(CHG_STA_DONE)
>>> +
>>> +#define GENERATE_ENUM(e) e
>>> +#define GENERATE_STRING(s) #s
>>> +
>>> +enum {
>>> + FOREACH_IRQ(GENERATE_ENUM)
>>> +};
>>> +
>>> +static const char *const chgr_irqs_str[] = {
>>> + FOREACH_IRQ(GENERATE_STRING)
>>> +};
>>> +
>>> +static int irqs[ARRAY_SIZE(chgr_irqs_str)];
>> No global variables please, this is not a singleton.
>>
>>> [...]
>>>
>>> +static int set_input_current_limit(struct max77759_charger *chg,
>>> int ilim_ua)
>>> +{
>>> + u32 regval;
>>> +
>>> + if (ilim_ua < 0)
>>> + return -EINVAL;
>>> +
>>> + if (ilim_ua == 0)
>>> + ilim_ua = MAX77759_CHGR_CHGIN_ILIM_MIN_UA;
>>> + else if (ilim_ua > MAX77759_CHGR_CHGIN_ILIM_MAX_UA)
>>> + ilim_ua = MAX77759_CHGR_CHGIN_ILIM_MAX_UA;
>> What if ilim_ua == 1 (or any other value < min_uA)? You could use
>> clamp()
>> instead of open-coding.
>>
>>> +
>>> + regval = val_to_regval(ilim_ua, MAX77759_CHGR_CHGIN_ILIM_MIN_UA,
>>> + MAX77759_CHGR_CHGIN_ILIM_STEP_UA,
>>> + MAX77759_CHGR_CHGIN_ILIM_REG_OFFSET);
>>> + return regmap_update_bits(chg->regmap,
>>> MAX77759_CHGR_REG_CHG_CNFG_09,
>>> + MAX77759_CHGR_REG_CHG_CNFG_09_CHGIN_ILIM,
>>> + regval);
>>> +}
>>> +
>>> +static const enum power_supply_property max77759_charger_props[] = {
>>> + POWER_SUPPLY_PROP_ONLINE,
>>> + POWER_SUPPLY_PROP_PRESENT,
>>> + POWER_SUPPLY_PROP_STATUS,
>>> + POWER_SUPPLY_PROP_CHARGE_TYPE,
>>> + POWER_SUPPLY_PROP_HEALTH,
>>> + POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX,
>>> + POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE_MAX,
>>> + POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT,
>>> +};
>>> +
>>> +static int max77759_charger_get_property(struct power_supply *psy,
>>> + enum power_supply_property psp,
>>> + union power_supply_propval *pval)
>>> +{
>>> + struct max77759_charger *chg = power_supply_get_drvdata(psy);
>>> + int ret;
>>> +
>>> + switch (psp) {
>>> + case POWER_SUPPLY_PROP_ONLINE:
>>> + ret = get_online(chg);
>>> + break;
>>> + case POWER_SUPPLY_PROP_PRESENT:
>>> + ret = charger_input_valid(chg);
>>> + break;
>>> + case POWER_SUPPLY_PROP_STATUS:
>>> + ret = get_status(chg);
>>> + break;
>>> + case POWER_SUPPLY_PROP_CHARGE_TYPE:
>>> + ret = get_charge_type(chg);
>>> + break;
>>> + case POWER_SUPPLY_PROP_HEALTH:
>>> + ret = get_health(chg);
>>> + break;
>>> + case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX:
>>> + ret = get_fast_charge_current(chg);
>>> + break;
>>> + case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE_MAX:
>>> + ret = get_float_voltage(chg);
>>> + break;
>>> + case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT:
>>> + ret = get_input_current_limit(chg);
>>> + break;
>>> + default:
>>> + ret = -EINVAL;
>>> + }
>>> +
>>> + pval->intval = ret;
>>> + return ret < 0 ? ret : 0;
>>> +}
>>> +
>>> +static const struct power_supply_desc max77759_charger_desc = {
>>> + .name = "max77759-charger",
>>> + .type = POWER_SUPPLY_TYPE_USB,
>>> + .properties = max77759_charger_props,
>>> + .num_properties = ARRAY_SIZE(max77759_charger_props),
>>> + .get_property = max77759_charger_get_property,
>>> +};
>>> +
>>> +static int charger_set_mode(struct max77759_charger *chg,
>>> + enum max77759_chgr_mode mode)
>>> +{
>>> + int ret;
>>> +
>>> + guard(mutex)(&chg->lock);
>>> +
>>> + if (chg->mode == mode)
>>> + return 0;
>>> +
>>> + if ((mode == MAX77759_CHGR_MODE_CHG_BUCK_ON ||
>>> + mode == MAX77759_CHGR_MODE_OTG_BOOST_ON) &&
>>> + chg->mode != MAX77759_CHGR_MODE_OFF) {
>>> + dev_err(chg->dev, "Invalid mode transition from %d to %d",
>>> + chg->mode, mode);
>>> + return -EINVAL;
>>> + }
>>> +
>>> + ret = regmap_update_bits(chg->regmap,
>>> MAX77759_CHGR_REG_CHG_CNFG_00,
>>> + MAX77759_CHGR_REG_CHG_CNFG_00_MODE, mode);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + chg->mode = mode;
>>> + return 0;
>>> +}
>>> +
>>> +static int enable_chgin_otg(struct regulator_dev *rdev)
>>> +{
>>> + struct max77759_charger *chg = rdev_get_drvdata(rdev);
>>> +
>>> + return charger_set_mode(chg, MAX77759_CHGR_MODE_OTG_BOOST_ON);
>>> +}
>>> +
>>> +static int disable_chgin_otg(struct regulator_dev *rdev)
>>> +{
>>> + struct max77759_charger *chg = rdev_get_drvdata(rdev);
>>> +
>>> + return charger_set_mode(chg, MAX77759_CHGR_MODE_OFF);
>>> +}
>>> +
>>> +static int chgin_otg_status(struct regulator_dev *rdev)
>>> +{
>>> + struct max77759_charger *chg = rdev_get_drvdata(rdev);
>>> +
>>> + guard(mutex)(&chg->lock);
>>> + return chg->mode == MAX77759_CHGR_MODE_OTG_BOOST_ON;
>>> +}
>>> +
>>> +static const struct regulator_ops chgin_otg_reg_ops = {
>>> + .enable = enable_chgin_otg,
>>> + .disable = disable_chgin_otg,
>>> + .is_enabled = chgin_otg_status,
>>> +};
>>> +
>>> +static const struct regulator_desc chgin_otg_reg_desc = {
>>> + .name = "chgin-otg",
>>> + .of_match = of_match_ptr("chgin-otg-regulator"),
>>> + .owner = THIS_MODULE,
>>> + .ops = &chgin_otg_reg_ops,
>>> + .fixed_uV = 5000000,
>>> + .n_voltages = 1,
>>> +};
>>> +
>>> +static irqreturn_t irq_handler(int irq, void *data)
>>> +{
>>> + struct max77759_charger *chg = data;
>>> + struct device *dev = chg->dev;
>>> + u32 chgint_ok;
>>> + int i;
>>> +
>>> + regmap_read(chg->regmap, MAX77759_CHGR_REG_CHG_INT_OK,
>>> &chgint_ok);
>> You might want to check the return value and return IRQ_NONE if it
>> didn't
>> work?
>>
>>> +
>>> + for (i = 0; i < ARRAY_SIZE(irqs); i++) {
>>> + if (irqs[i] == irq)
>>> + break;
>>> + }
>>> +
>>> + switch (i) {
>>> + case AICL:
>>> + dev_dbg(dev, "AICL mode: %s",
>>> + str_no_yes(chgint_ok & MAX77759_CHGR_REG_CHG_INT_AICL));
>>> + break;
>>> + case CHGIN:
>>> + dev_dbg(dev, "CHGIN input valid: %s",
>>> + str_yes_no(chgint_ok & MAX77759_CHGR_REG_CHG_INT_CHGIN));
>>> + break;
>>> + case CHG:
>>> + dev_dbg(dev, "CHG status okay/off: %s",
>>> + str_yes_no(chgint_ok & MAX77759_CHGR_REG_CHG_INT_CHG));
>>> + break;
>>> + case INLIM:
>>> + dev_dbg(dev, "Current Limit reached: %s",
>>> + str_no_yes(chgint_ok & MAX77759_CHGR_REG_CHG_INT_INLIM));
>>> + break;
>>> + case BAT_OILO:
>>> + dev_dbg(dev, "Battery over-current threshold crossed");
>>> + break;
>>> + case CHG_STA_CC:
>>> + dev_dbg(dev, "Charger reached CC stage");
>>> + break;
>>> + case CHG_STA_CV:
>>> + dev_dbg(dev, "Charger reached CV stage");
>>> + break;
>>> + case CHG_STA_TO:
>>> + dev_dbg(dev, "Charger reached TO stage");
>>> + break;
>>> + case CHG_STA_DONE:
>>> + dev_dbg(dev, "Charger reached TO stage");
>>> + break;
>> Are the above debug messages really all needed?
I forgot to respond to this comment in my previous email.
I think we can keep AICL, BAT_OILO, INLIM. They're either special
conditions (AICL) or faulty conditions (like BAT_OILO) and we can in
fact keep them at dev_info level. Rest can be removed and a
power_supply_changed() is sufficient.
Let me know what you think?
BR,
Amit
>>
>>> + default:
>>> + dev_err(dev, "Unrecognized irq: %d", i);
>>> + return IRQ_HANDLED;
>> I'm not sure it should return IRQ_HANDLED in this case.
>>
>>> + }
>>> +
>>> + power_supply_changed(chg->psy);
>>> + return IRQ_HANDLED;
>>> +}
>>> +
>>> +static int max77759_init_irqhandler(struct max77759_charger *chg)
>>> +{
>>> + struct device *dev = chg->dev;
>>> + unsigned long irq_flags;
>>> + struct irq_data *irqd;
>>> + int i, ret;
>>> +
>>> + for (i = 0; i < ARRAY_SIZE(chgr_irqs_str); i++) {
>>> + ret = platform_get_irq_byname(to_platform_device(dev),
>>> + chgr_irqs_str[i]);
>>> + if (ret < 0) {
>>> + dev_err(dev,
>>> + "Failed to get irq resource for %s, ret=%d",
>>> + chgr_irqs_str[i], ret);
>>> + return ret;
>>> + }
>> You should use return dev_err_probe() here, and drop the additional
>> dev_err_probe()
>> in max77759_charger_probe().
>>
>>> +
>>> + irqs[i] = ret;
>>> + irq_flags = IRQF_ONESHOT;
>>> + irqd = irq_get_irq_data(irqs[i]);
>>> + if (irqd)
>>> + irq_flags |= irqd_get_trigger_type(irqd);
>> The above three lines are not needed, and then you can also drop
>> irq_flags and
>> use its value in the below call directly.
>>
>>> +
>>> + ret = devm_request_threaded_irq(dev, irqs[i], NULL,
>>> irq_handler,
>>> + irq_flags, dev_name(dev), chg);
>>> + if (ret) {
>>> + dev_err(dev,
>>> + "Unable to register irq handler for %s, ret=%d",
>>> + chgr_irqs_str[i], ret);
>>> + return ret;
>> dev_err_probe() please.
>>
>>> + }
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int max77759_charger_init(struct max77759_charger *chg)
>>> +{
>>> + struct power_supply_battery_info *info;
>>> + u32 regval, fast_chg_curr, fv;
>>> + int ret;
>>> +
>>> + regmap_read(chg->regmap, MAX77759_CHGR_REG_CHG_CNFG_00, ®val);
>>> + 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 */
>>> + regmap_update_bits(chg->regmap, MAX77759_CHGR_REG_CHG_CNFG_12,
>>> + MAX77759_CHGR_REG_CHG_CNFG_12_WCINSEL, 0);
>>> +
>>> + regmap_update_bits(chg->regmap, MAX77759_CHGR_REG_CHG_CNFG_18,
>>> + MAX77759_CHGR_REG_CHG_CNFG_18_WDTEN, 0);
>> I think it's good practice to check return values.
>>
>>> +
>>> + return unlock_prot_regs(chg, false);
>>> +}
>>> +
>>> +static void psy_work_item(struct work_struct *work)
>>> +{
>>> + struct max77759_charger *chg =
>>> + container_of(work, struct max77759_charger, psy_work);
>>> + union power_supply_propval current_limit = { 0 }, online = { 0 };
>>> + int ret;
>>> +
>>> + power_supply_get_property(chg->tcpm_psy,
>>> POWER_SUPPLY_PROP_CURRENT_MAX,
>>> + ¤t_limit);
>>> + power_supply_get_property(chg->tcpm_psy, POWER_SUPPLY_PROP_ONLINE,
>>> + &online);
>> Would it make sense to rework this and check the return values? Then
>> you can also
>> drop the greedy init at function entry.
>>
>>> +
>>> + if (online.intval && current_limit.intval) {
>>> + ret = set_input_current_limit(chg, current_limit.intval);
>>> + if (ret)
>>> + dev_err(chg->dev,
>>> + "Unable to set current limit, ret=%d", ret);
>>> +
>>> + charger_set_mode(chg, MAX77759_CHGR_MODE_CHG_BUCK_ON);
>>> + } else {
>>> + charger_set_mode(chg, MAX77759_CHGR_MODE_OFF);
>>> + }
>>> +}
>>> +
>>> +static int psy_changed(struct notifier_block *nb, unsigned long
>>> evt, void *data)
>>> +{
>>> + struct max77759_charger *chg = container_of(nb, struct
>>> max77759_charger,
>>> + nb);
>>> + const char *psy_name = "tcpm-source";
>>> + struct power_supply *psy = data;
>>> +
>>> + if (!strnstr(psy->desc->name, psy_name, strlen(psy_name)) ||
>>> + evt != PSY_EVENT_PROP_CHANGED)
>>> + return NOTIFY_OK;
>>> +
>>> + chg->tcpm_psy = psy;
>>> + schedule_work(&chg->psy_work);
>> Maybe add a newline here.
>>
>>> + return NOTIFY_OK;
>>> +}
>>> +
>>> +static void max_tcpci_unregister_psy_notifier(void *nb)
>>> +{
>>> + power_supply_unreg_notifier(nb);
>>> +}
>>> +
>>> +static int max77759_charger_probe(struct platform_device *pdev)
>>> +{
>>> + struct regulator_config chgin_otg_reg_cfg;
>>> + struct power_supply_config psy_cfg;
>>> + struct device *dev = &pdev->dev;
>>> + struct max77759_charger *chg;
>>> + int ret;
>>> +
>>> + device_set_of_node_from_dev(dev, dev->parent);
>>> + chg = devm_kzalloc(dev, sizeof(*chg), GFP_KERNEL);
>>> + if (!chg)
>>> + return -ENOMEM;
>>> +
>>> + platform_set_drvdata(pdev, chg);
>>> + chg->dev = dev;
>>> + chg->regmap = dev_get_regmap(dev->parent, "charger");
>>> + if (!chg->regmap)
>>> + return dev_err_probe(dev, -ENODEV, "Missing regmap");
>>> +
>>> + ret = devm_mutex_init(dev, &chg->lock);
>>> + if (ret)
>>> + return dev_err_probe(dev, ret, "Failed to initialize lock");
>>> +
>>> + psy_cfg.fwnode = dev_fwnode(dev);
>>> + psy_cfg.drv_data = chg;
>>> + chg->psy = devm_power_supply_register(dev, &max77759_charger_desc,
>>> + &psy_cfg);
>>> + if (IS_ERR(chg->psy))
>>> + return dev_err_probe(dev, -EPROBE_DEFER,
>>> + "Failed to register psy, ret=%ld",
>>> + PTR_ERR(chg->psy));
>>> +
>>> + ret = max77759_charger_init(chg);
>>> + if (ret)
>>> + return dev_err_probe(dev, ret,
>>> + "Failed to initialize max77759 charger");
>>> +
>>> + chgin_otg_reg_cfg.dev = dev;
>>> + chgin_otg_reg_cfg.driver_data = chg;
>>> + chgin_otg_reg_cfg.of_node = dev_of_node(dev);
>>> + chg->chgin_otg_rdev = devm_regulator_register(dev,
>>> &chgin_otg_reg_desc,
>>> + &chgin_otg_reg_cfg);
>>> + if (IS_ERR(chg->chgin_otg_rdev))
>>> + return dev_err_probe(dev, PTR_ERR(chg->chgin_otg_rdev),
>>> + "Failed to register chgin otg regulator");
>>> +
>>> + ret = devm_work_autocancel(dev, &chg->psy_work, psy_work_item);
>>> + if (ret)
>>> + return dev_err_probe(dev, ret, "Failed to initialize psy
>>> work");
>>> +
>>> + chg->nb.notifier_call = psy_changed;
>>> + ret = power_supply_reg_notifier(&chg->nb);
>>> + if (ret)
>>> + return dev_err_probe(dev, ret,
>>> + "Unable to register psy notifier");
>>> +
>>> + ret = devm_add_action_or_reset(dev,
>>> max_tcpci_unregister_psy_notifier,
>>> + &chg->nb);
>>> + if (ret)
>>> + return ret;
>> You could print a message here as well.
>
> Thanks for the detailed review! I will fix all the flagged issues in
> the next rev.
>
>
> BR,
>
> Amit
>
>>
>> Cheers,
>> Andre'
Hi Amit,
On Tue, 2026-01-06 at 17:14 -0800, Amit Sunil Dhamne wrote:
>
> On 1/6/26 3:41 PM, Amit Sunil Dhamne wrote:
> > Hi Andre',
> >
> > On 1/5/26 9:32 AM, André Draszik wrote:
> > > Hi Amit,
> > >
> > > I haven't done a full review, but a few things caught my eye.
> > >
> > > On Sat, 2025-12-27 at 00:04 +0000, Amit Sunil Dhamne via B4 Relay wrote:
> > > >
> > > > diff --git a/drivers/power/supply/Makefile
> > > > b/drivers/power/supply/Makefile
> > > > index 4b79d5abc49a..6af905875ad5 100644
> > > > --- a/drivers/power/supply/Makefile
> > > > +++ b/drivers/power/supply/Makefile
> > > > [...]
> > > > +
> > > > +static irqreturn_t irq_handler(int irq, void *data)
> > > > +{
> > > > + struct max77759_charger *chg = data;
> > > > + struct device *dev = chg->dev;
> > > > + u32 chgint_ok;
> > > > + int i;
> > > > +
> > > > + regmap_read(chg->regmap, MAX77759_CHGR_REG_CHG_INT_OK,
> > > > &chgint_ok);
> > > You might want to check the return value and return IRQ_NONE if it
> > > didn't
> > > work?
> > >
> > > > +
> > > > + for (i = 0; i < ARRAY_SIZE(irqs); i++) {
> > > > + if (irqs[i] == irq)
> > > > + break;
> > > > + }
> > > > +
> > > > + switch (i) {
> > > > + case AICL:
> > > > + dev_dbg(dev, "AICL mode: %s",
> > > > + str_no_yes(chgint_ok & MAX77759_CHGR_REG_CHG_INT_AICL));
> > > > + break;
> > > > + case CHGIN:
> > > > + dev_dbg(dev, "CHGIN input valid: %s",
> > > > + str_yes_no(chgint_ok & MAX77759_CHGR_REG_CHG_INT_CHGIN));
> > > > + break;
> > > > + case CHG:
> > > > + dev_dbg(dev, "CHG status okay/off: %s",
> > > > + str_yes_no(chgint_ok & MAX77759_CHGR_REG_CHG_INT_CHG));
> > > > + break;
> > > > + case INLIM:
> > > > + dev_dbg(dev, "Current Limit reached: %s",
> > > > + str_no_yes(chgint_ok & MAX77759_CHGR_REG_CHG_INT_INLIM));
> > > > + break;
> > > > + case BAT_OILO:
> > > > + dev_dbg(dev, "Battery over-current threshold crossed");
> > > > + break;
> > > > + case CHG_STA_CC:
> > > > + dev_dbg(dev, "Charger reached CC stage");
> > > > + break;
> > > > + case CHG_STA_CV:
> > > > + dev_dbg(dev, "Charger reached CV stage");
> > > > + break;
> > > > + case CHG_STA_TO:
> > > > + dev_dbg(dev, "Charger reached TO stage");
> > > > + break;
> > > > + case CHG_STA_DONE:
> > > > + dev_dbg(dev, "Charger reached TO stage");
> > > > + break;
> > > Are the above debug messages really all needed?
>
> I forgot to respond to this comment in my previous email.
>
> I think we can keep AICL, BAT_OILO, INLIM. They're either special
> conditions (AICL) or faulty conditions (like BAT_OILO) and we can in
> fact keep them at dev_info level. Rest can be removed and a
> power_supply_changed() is sufficient.
>
> Let me know what you think?
I don't think dev_info() in an interrupt handler is appropriate. At
least it should be ratelimited.
If it's something special / unexpected that needs attention, having
a dev_dbg() message only will usually not be visible to anybody.
Also will the call to power_supply_changed() down below handle the
special conditions (e.g. convey to upper levels)? If not, can it be
made to do so?
Cheers,
Andre
Hi Andre',
On 1/12/26 5:47 AM, André Draszik wrote:
> Hi Amit,
>
> On Tue, 2026-01-06 at 17:14 -0800, Amit Sunil Dhamne wrote:
>> On 1/6/26 3:41 PM, Amit Sunil Dhamne wrote:
>>> Hi Andre',
>>>
>>> On 1/5/26 9:32 AM, André Draszik wrote:
>>>> Hi Amit,
>>>>
>>>> I haven't done a full review, but a few things caught my eye.
>>>>
>>>> On Sat, 2025-12-27 at 00:04 +0000, Amit Sunil Dhamne via B4 Relay wrote:
>>>>> diff --git a/drivers/power/supply/Makefile
>>>>> b/drivers/power/supply/Makefile
>>>>> index 4b79d5abc49a..6af905875ad5 100644
>>>>> --- a/drivers/power/supply/Makefile
>>>>> +++ b/drivers/power/supply/Makefile
>>>>> [...]
>>>>> +
>>>>> +static irqreturn_t irq_handler(int irq, void *data)
>>>>> +{
>>>>> + struct max77759_charger *chg = data;
>>>>> + struct device *dev = chg->dev;
>>>>> + u32 chgint_ok;
>>>>> + int i;
>>>>> +
>>>>> + regmap_read(chg->regmap, MAX77759_CHGR_REG_CHG_INT_OK,
>>>>> &chgint_ok);
>>>> You might want to check the return value and return IRQ_NONE if it
>>>> didn't
>>>> work?
>>>>
>>>>> +
>>>>> + for (i = 0; i < ARRAY_SIZE(irqs); i++) {
>>>>> + if (irqs[i] == irq)
>>>>> + break;
>>>>> + }
>>>>> +
>>>>> + switch (i) {
>>>>> + case AICL:
>>>>> + dev_dbg(dev, "AICL mode: %s",
>>>>> + str_no_yes(chgint_ok & MAX77759_CHGR_REG_CHG_INT_AICL));
>>>>> + break;
>>>>> + case CHGIN:
>>>>> + dev_dbg(dev, "CHGIN input valid: %s",
>>>>> + str_yes_no(chgint_ok & MAX77759_CHGR_REG_CHG_INT_CHGIN));
>>>>> + break;
>>>>> + case CHG:
>>>>> + dev_dbg(dev, "CHG status okay/off: %s",
>>>>> + str_yes_no(chgint_ok & MAX77759_CHGR_REG_CHG_INT_CHG));
>>>>> + break;
>>>>> + case INLIM:
>>>>> + dev_dbg(dev, "Current Limit reached: %s",
>>>>> + str_no_yes(chgint_ok & MAX77759_CHGR_REG_CHG_INT_INLIM));
>>>>> + break;
>>>>> + case BAT_OILO:
>>>>> + dev_dbg(dev, "Battery over-current threshold crossed");
>>>>> + break;
>>>>> + case CHG_STA_CC:
>>>>> + dev_dbg(dev, "Charger reached CC stage");
>>>>> + break;
>>>>> + case CHG_STA_CV:
>>>>> + dev_dbg(dev, "Charger reached CV stage");
>>>>> + break;
>>>>> + case CHG_STA_TO:
>>>>> + dev_dbg(dev, "Charger reached TO stage");
>>>>> + break;
>>>>> + case CHG_STA_DONE:
>>>>> + dev_dbg(dev, "Charger reached TO stage");
>>>>> + break;
>>>> Are the above debug messages really all needed?
>> I forgot to respond to this comment in my previous email.
>>
>> I think we can keep AICL, BAT_OILO, INLIM. They're either special
>> conditions (AICL) or faulty conditions (like BAT_OILO) and we can in
>> fact keep them at dev_info level. Rest can be removed and a
>> power_supply_changed() is sufficient.
>>
>> Let me know what you think?
> I don't think dev_info() in an interrupt handler is appropriate. At
> least it should be ratelimited.
>
> If it's something special / unexpected that needs attention, having
> a dev_dbg() message only will usually not be visible to anybody.
I agree. I can change the prints to dev_info_ratelimited for the stuff
we care about.
>
> Also will the call to power_supply_changed() down below handle the
> special conditions (e.g. convey to upper levels)? If not, can it be
> made to do so?
Yes it does, as I can see a call to kobject_uevent() inside
power_supply_changed_work(). Also, power_supply_changed() also notifies
other subsystems that have registered their notifiers downstream of this
power_supply object. So I believe we're good there.
If all the above sounds good, I will proceed with sending the next
revision including the fixes :).
BR,
Amit
>
> Cheers,
> Andre
>
Hi Amit,
On Mon, 2026-01-12 at 11:37 -0800, Amit Sunil Dhamne wrote:
> Hi Andre',
>
> On 1/12/26 5:47 AM, André Draszik wrote:
> > Hi Amit,
> >
> > On Tue, 2026-01-06 at 17:14 -0800, Amit Sunil Dhamne wrote:
> > > On 1/6/26 3:41 PM, Amit Sunil Dhamne wrote:
> > > > Hi Andre',
> > > >
> > > > On 1/5/26 9:32 AM, André Draszik wrote:
> > > > > Hi Amit,
> > > > >
> > > > > I haven't done a full review, but a few things caught my eye.
> > > > >
> > > > > On Sat, 2025-12-27 at 00:04 +0000, Amit Sunil Dhamne via B4 Relay wrote:
> > > > > > diff --git a/drivers/power/supply/Makefile
> > > > > > b/drivers/power/supply/Makefile
> > > > > > index 4b79d5abc49a..6af905875ad5 100644
> > > > > > --- a/drivers/power/supply/Makefile
> > > > > > +++ b/drivers/power/supply/Makefile
> > > > > > [...]
> > > > > > +
> > > > > > +static irqreturn_t irq_handler(int irq, void *data)
> > > > > > +{
> > > > > > + struct max77759_charger *chg = data;
> > > > > > + struct device *dev = chg->dev;
> > > > > > + u32 chgint_ok;
> > > > > > + int i;
> > > > > > +
> > > > > > + regmap_read(chg->regmap, MAX77759_CHGR_REG_CHG_INT_OK,
> > > > > > &chgint_ok);
> > > > > You might want to check the return value and return IRQ_NONE if it
> > > > > didn't
> > > > > work?
> > > > >
> > > > > > +
> > > > > > + for (i = 0; i < ARRAY_SIZE(irqs); i++) {
> > > > > > + if (irqs[i] == irq)
> > > > > > + break;
> > > > > > + }
> > > > > > +
> > > > > > + switch (i) {
> > > > > > + case AICL:
> > > > > > + dev_dbg(dev, "AICL mode: %s",
> > > > > > + str_no_yes(chgint_ok & MAX77759_CHGR_REG_CHG_INT_AICL));
> > > > > > + break;
> > > > > > + case CHGIN:
> > > > > > + dev_dbg(dev, "CHGIN input valid: %s",
> > > > > > + str_yes_no(chgint_ok & MAX77759_CHGR_REG_CHG_INT_CHGIN));
> > > > > > + break;
> > > > > > + case CHG:
> > > > > > + dev_dbg(dev, "CHG status okay/off: %s",
> > > > > > + str_yes_no(chgint_ok & MAX77759_CHGR_REG_CHG_INT_CHG));
> > > > > > + break;
> > > > > > + case INLIM:
> > > > > > + dev_dbg(dev, "Current Limit reached: %s",
> > > > > > + str_no_yes(chgint_ok & MAX77759_CHGR_REG_CHG_INT_INLIM));
> > > > > > + break;
> > > > > > + case BAT_OILO:
> > > > > > + dev_dbg(dev, "Battery over-current threshold crossed");
> > > > > > + break;
> > > > > > + case CHG_STA_CC:
> > > > > > + dev_dbg(dev, "Charger reached CC stage");
> > > > > > + break;
> > > > > > + case CHG_STA_CV:
> > > > > > + dev_dbg(dev, "Charger reached CV stage");
> > > > > > + break;
> > > > > > + case CHG_STA_TO:
> > > > > > + dev_dbg(dev, "Charger reached TO stage");
> > > > > > + break;
> > > > > > + case CHG_STA_DONE:
> > > > > > + dev_dbg(dev, "Charger reached TO stage");
> > > > > > + break;
> > > > > Are the above debug messages really all needed?
> > > I forgot to respond to this comment in my previous email.
> > >
> > > I think we can keep AICL, BAT_OILO, INLIM. They're either special
> > > conditions (AICL) or faulty conditions (like BAT_OILO) and we can in
> > > fact keep them at dev_info level. Rest can be removed and a
> > > power_supply_changed() is sufficient.
> > >
> > > Let me know what you think?
> > I don't think dev_info() in an interrupt handler is appropriate. At
> > least it should be ratelimited.
> >
> > If it's something special / unexpected that needs attention, having
> > a dev_dbg() message only will usually not be visible to anybody.
>
> I agree. I can change the prints to dev_info_ratelimited for the stuff
> we care about.
If it's an erroneous condition, maybe warn or even err are more appropriate?
But then, what is the expectation upon the user observing these messages?
What can or should they do? Who is going to look at these and can do
something sensible based on them?
> >
> > Also will the call to power_supply_changed() down below handle the
> > special conditions (e.g. convey to upper levels)? If not, can it be
> > made to do so?
>
> Yes it does, as I can see a call to kobject_uevent() inside
> power_supply_changed_work(). Also, power_supply_changed() also notifies
> other subsystems that have registered their notifiers downstream of this
> power_supply object. So I believe we're good there.
If erroneous conditions are handled by other / upper layers, why print a
message in this interrupt handler in the first place?
Also, I just noticed there is a max77705 charger driver. It seems quite
similar to this one, maybe it can be leveraged / extended?
Cheers,
Andre'
Hi Andre',
On 1/13/26 2:02 AM, André Draszik wrote:
> Hi Amit,
>
> On Mon, 2026-01-12 at 11:37 -0800, Amit Sunil Dhamne wrote:
>> Hi Andre',
>>
>> On 1/12/26 5:47 AM, André Draszik wrote:
>>> Hi Amit,
>>>
>>> On Tue, 2026-01-06 at 17:14 -0800, Amit Sunil Dhamne wrote:
>>>> On 1/6/26 3:41 PM, Amit Sunil Dhamne wrote:
>>>>> Hi Andre',
>>>>>
>>>>> On 1/5/26 9:32 AM, André Draszik wrote:
>>>>>> Hi Amit,
>>>>>>
>>>>>> I haven't done a full review, but a few things caught my eye.
>>>>>>
>>>>>> On Sat, 2025-12-27 at 00:04 +0000, Amit Sunil Dhamne via B4 Relay wrote:
>>>>>>> diff --git a/drivers/power/supply/Makefile
>>>>>>> b/drivers/power/supply/Makefile
>>>>>>> index 4b79d5abc49a..6af905875ad5 100644
>>>>>>> --- a/drivers/power/supply/Makefile
>>>>>>> +++ b/drivers/power/supply/Makefile
>>>>>>> [...]
>>>>>>> +
>>>>>>> +static irqreturn_t irq_handler(int irq, void *data)
>>>>>>> +{
>>>>>>> + struct max77759_charger *chg = data;
>>>>>>> + struct device *dev = chg->dev;
>>>>>>> + u32 chgint_ok;
>>>>>>> + int i;
>>>>>>> +
>>>>>>> + regmap_read(chg->regmap, MAX77759_CHGR_REG_CHG_INT_OK,
>>>>>>> &chgint_ok);
>>>>>> You might want to check the return value and return IRQ_NONE if it
>>>>>> didn't
>>>>>> work?
>>>>>>
>>>>>>> +
>>>>>>> + for (i = 0; i < ARRAY_SIZE(irqs); i++) {
>>>>>>> + if (irqs[i] == irq)
>>>>>>> + break;
>>>>>>> + }
>>>>>>> +
>>>>>>> + switch (i) {
>>>>>>> + case AICL:
>>>>>>> + dev_dbg(dev, "AICL mode: %s",
>>>>>>> + str_no_yes(chgint_ok & MAX77759_CHGR_REG_CHG_INT_AICL));
>>>>>>> + break;
>>>>>>> + case CHGIN:
>>>>>>> + dev_dbg(dev, "CHGIN input valid: %s",
>>>>>>> + str_yes_no(chgint_ok & MAX77759_CHGR_REG_CHG_INT_CHGIN));
>>>>>>> + break;
>>>>>>> + case CHG:
>>>>>>> + dev_dbg(dev, "CHG status okay/off: %s",
>>>>>>> + str_yes_no(chgint_ok & MAX77759_CHGR_REG_CHG_INT_CHG));
>>>>>>> + break;
>>>>>>> + case INLIM:
>>>>>>> + dev_dbg(dev, "Current Limit reached: %s",
>>>>>>> + str_no_yes(chgint_ok & MAX77759_CHGR_REG_CHG_INT_INLIM));
>>>>>>> + break;
>>>>>>> + case BAT_OILO:
>>>>>>> + dev_dbg(dev, "Battery over-current threshold crossed");
>>>>>>> + break;
>>>>>>> + case CHG_STA_CC:
>>>>>>> + dev_dbg(dev, "Charger reached CC stage");
>>>>>>> + break;
>>>>>>> + case CHG_STA_CV:
>>>>>>> + dev_dbg(dev, "Charger reached CV stage");
>>>>>>> + break;
>>>>>>> + case CHG_STA_TO:
>>>>>>> + dev_dbg(dev, "Charger reached TO stage");
>>>>>>> + break;
>>>>>>> + case CHG_STA_DONE:
>>>>>>> + dev_dbg(dev, "Charger reached TO stage");
>>>>>>> + break;
>>>>>> Are the above debug messages really all needed?
>>>> I forgot to respond to this comment in my previous email.
>>>>
>>>> I think we can keep AICL, BAT_OILO, INLIM. They're either special
>>>> conditions (AICL) or faulty conditions (like BAT_OILO) and we can in
>>>> fact keep them at dev_info level. Rest can be removed and a
>>>> power_supply_changed() is sufficient.
>>>>
>>>> Let me know what you think?
>>> I don't think dev_info() in an interrupt handler is appropriate. At
>>> least it should be ratelimited.
>>>
>>> If it's something special / unexpected that needs attention, having
>>> a dev_dbg() message only will usually not be visible to anybody.
>> I agree. I can change the prints to dev_info_ratelimited for the stuff
>> we care about.
> If it's an erroneous condition, maybe warn or even err are more appropriate?
>
> But then, what is the expectation upon the user observing these messages?
> What can or should they do? Who is going to look at these and can do
> something sensible based on them?
The logging will help in postmortem analysis which may or may not
possible with just publishing uevents to userspace hoping that they log
the psy properties. Illustrating a situation:
1. Over current situation happened where the Battery to System current
exceeds the BAT_OILO threshold. This would also generate an interrupt.
2. The MAX77759 takes protective measures if the condition lasts for a
certain specified time and reset. Resetting will cause Vsys to collapse
to 0 if the system is only battery powered.
3. It'd be better that the BAT_OILO interrupt is logged in dmesg,
instead of just delegating it to user space as user can debug this
condition by looking at last_kmsg or pstore.
4. This signal can help the user debug conditions such as moisture (this
signal + contaminant detection) or indicative of a mechanical failure.
I do agree though that this is a hypothetical or very rare situation and
if you have a strong opinion against this I am okay with removing the
prints completely.
>
>>> Also will the call to power_supply_changed() down below handle the
>>> special conditions (e.g. convey to upper levels)? If not, can it be
>>> made to do so?
>> Yes it does, as I can see a call to kobject_uevent() inside
>> power_supply_changed_work(). Also, power_supply_changed() also notifies
>> other subsystems that have registered their notifiers downstream of this
>> power_supply object. So I believe we're good there.
> If erroneous conditions are handled by other / upper layers, why print a
> message in this interrupt handler in the first place?
I tried illustrating an example above.
>
> Also, I just noticed there is a max77705 charger driver. It seems quite
> similar to this one, maybe it can be leveraged / extended?
Thanks for the feedback. I reviewed the max77705 charger driver. .
Here is a breakdown of why I believe a separate driver may be a better
approach:
Similarities:
1. Helper Functions: We could potentially leverage common logic for
get_charge_type, get_status, get_health, and get_input_current.
2. Register Access: MAX77705 uses regfield abstractions to handle
register operations which can also be potentially leveraged.
3. Initialization: Some hardware initialization steps appear similar,
though about 60% of the max77705 initialization (e.g., switching
frequency, WCIN regulation voltage, top-off time) is irrelevant for the
max77759 configuration I need.
Differences:
1. OTG Support: The max77759 driver supports OTG boost mode, which is a
key requirement for my use case. While the max77705 hardware might
support OTG based on its registers, the current driver implementation
does not support it.
2. TCPCI/TCPM Integration: The max77759 driver is explicitly architected
to work with a TCPCI/TCPM-compliant Type-C controller to set input
current limits dynamically. It is ambiguous whether the max77705 device
uses a standard TCPCI/TCPM model or a proprietary one.
3. Register Incompatibility: There are distinct register differences.
For example, the max77705 driver relies on BATP and BATP_DTLS registers,
which do not exist in the max77759. Conversely, the max77759 has a
dedicated second interrupt register (CHG_INT2) that reports critical
signals like BAT_OILO, SYS_UVLO, and charging stages, which appear
absent or handled differently in the max77705. Additionally, MAX77759
has input selection (wireless, usb) and uses it in the driver but it's
not evident from register definitions whether max77705 has it.
4. Parameter Calculations: The formulas for calculating parameters like
Fast Charge Current (CHGCC) and Float Voltage are different between the
two chips. Merging the drivers would require separate, chip-specific
getter/setter functions for these core properties.
5. Device-Specific Workarounds: The max77705 driver includes a
workaround in max77705_aicl_irq that is not relevant to the max77759.
There may also be future workarounds which may not be applicable to one
or the other.
Logistical Constraints: I don't have access to max77705 hardware or its
full datasheet. This makes it impossible for me to test any shared code
changes to ensure I haven't introduced regressions on the max77705. IMO,
given these constraints and technical divergences, maintaining separate
drivers maybe a better choice. Please let me know wdyt?
BR,
Amit
>
>
> Cheers,
> Andre'
On Tue, 2026-01-13 at 16:47 -0800, Amit Sunil Dhamne wrote:
> Hi Andre',
>
> On 1/13/26 2:02 AM, André Draszik wrote:
> > Hi Amit,
> >
> > On Mon, 2026-01-12 at 11:37 -0800, Amit Sunil Dhamne wrote:
> > > Hi Andre',
> > >
> > > On 1/12/26 5:47 AM, André Draszik wrote:
> > > > Hi Amit,
> > > >
> > > > On Tue, 2026-01-06 at 17:14 -0800, Amit Sunil Dhamne wrote:
> > > > > On 1/6/26 3:41 PM, Amit Sunil Dhamne wrote:
> > > > > > Hi Andre',
> > > > > >
> > > > > > On 1/5/26 9:32 AM, André Draszik wrote:
> > > > > > > Hi Amit,
> > > > > > >
> > > > > > > I haven't done a full review, but a few things caught my eye.
> > > > > > >
> > > > > > > On Sat, 2025-12-27 at 00:04 +0000, Amit Sunil Dhamne via B4 Relay wrote:
> > > > > > > > diff --git a/drivers/power/supply/Makefile
> > > > > > > > b/drivers/power/supply/Makefile
> > > > > > > > index 4b79d5abc49a..6af905875ad5 100644
> > > > > > > > --- a/drivers/power/supply/Makefile
> > > > > > > > +++ b/drivers/power/supply/Makefile
> > > > > > > > [...]
> > > > > > > > +
> > > > > > > > +static irqreturn_t irq_handler(int irq, void *data)
> > > > > > > > +{
> > > > > > > > + struct max77759_charger *chg = data;
> > > > > > > > + struct device *dev = chg->dev;
> > > > > > > > + u32 chgint_ok;
> > > > > > > > + int i;
> > > > > > > > +
> > > > > > > > + regmap_read(chg->regmap, MAX77759_CHGR_REG_CHG_INT_OK,
> > > > > > > > &chgint_ok);
> > > > > > > You might want to check the return value and return IRQ_NONE if it
> > > > > > > didn't
> > > > > > > work?
> > > > > > >
> > > > > > > > +
> > > > > > > > + for (i = 0; i < ARRAY_SIZE(irqs); i++) {
> > > > > > > > + if (irqs[i] == irq)
> > > > > > > > + break;
> > > > > > > > + }
> > > > > > > > +
> > > > > > > > + switch (i) {
> > > > > > > > + case AICL:
> > > > > > > > + dev_dbg(dev, "AICL mode: %s",
> > > > > > > > + str_no_yes(chgint_ok & MAX77759_CHGR_REG_CHG_INT_AICL));
> > > > > > > > + break;
> > > > > > > > + case CHGIN:
> > > > > > > > + dev_dbg(dev, "CHGIN input valid: %s",
> > > > > > > > + str_yes_no(chgint_ok & MAX77759_CHGR_REG_CHG_INT_CHGIN));
> > > > > > > > + break;
> > > > > > > > + case CHG:
> > > > > > > > + dev_dbg(dev, "CHG status okay/off: %s",
> > > > > > > > + str_yes_no(chgint_ok & MAX77759_CHGR_REG_CHG_INT_CHG));
> > > > > > > > + break;
> > > > > > > > + case INLIM:
> > > > > > > > + dev_dbg(dev, "Current Limit reached: %s",
> > > > > > > > + str_no_yes(chgint_ok & MAX77759_CHGR_REG_CHG_INT_INLIM));
> > > > > > > > + break;
> > > > > > > > + case BAT_OILO:
> > > > > > > > + dev_dbg(dev, "Battery over-current threshold crossed");
> > > > > > > > + break;
> > > > > > > > + case CHG_STA_CC:
> > > > > > > > + dev_dbg(dev, "Charger reached CC stage");
> > > > > > > > + break;
> > > > > > > > + case CHG_STA_CV:
> > > > > > > > + dev_dbg(dev, "Charger reached CV stage");
> > > > > > > > + break;
> > > > > > > > + case CHG_STA_TO:
> > > > > > > > + dev_dbg(dev, "Charger reached TO stage");
> > > > > > > > + break;
> > > > > > > > + case CHG_STA_DONE:
> > > > > > > > + dev_dbg(dev, "Charger reached TO stage");
> > > > > > > > + break;
> > > > > > > Are the above debug messages really all needed?
> > > > > I forgot to respond to this comment in my previous email.
> > > > >
> > > > > I think we can keep AICL, BAT_OILO, INLIM. They're either special
> > > > > conditions (AICL) or faulty conditions (like BAT_OILO) and we can in
> > > > > fact keep them at dev_info level. Rest can be removed and a
> > > > > power_supply_changed() is sufficient.
> > > > >
> > > > > Let me know what you think?
> > > > I don't think dev_info() in an interrupt handler is appropriate. At
> > > > least it should be ratelimited.
> > > >
> > > > If it's something special / unexpected that needs attention, having
> > > > a dev_dbg() message only will usually not be visible to anybody.
> > > I agree. I can change the prints to dev_info_ratelimited for the stuff
> > > we care about.
> > If it's an erroneous condition, maybe warn or even err are more appropriate?
> >
> > But then, what is the expectation upon the user observing these messages?
> > What can or should they do? Who is going to look at these and can do
> > something sensible based on them?
>
> The logging will help in postmortem analysis which may or may not
> possible with just publishing uevents to userspace hoping that they log
> the psy properties. Illustrating a situation:
>
> 1. Over current situation happened where the Battery to System current
> exceeds the BAT_OILO threshold. This would also generate an interrupt.
>
> 2. The MAX77759 takes protective measures if the condition lasts for a
> certain specified time and reset. Resetting will cause Vsys to collapse
> to 0 if the system is only battery powered.
>
> 3. It'd be better that the BAT_OILO interrupt is logged in dmesg,
> instead of just delegating it to user space as user can debug this
> condition by looking at last_kmsg or pstore.
>
> 4. This signal can help the user debug conditions such as moisture (this
> signal + contaminant detection) or indicative of a mechanical failure.
>
> I do agree though that this is a hypothetical or very rare situation and
> if you have a strong opinion against this I am okay with removing the
> prints completely.
Thanks for details. OK, they sound useful in this case, but should still
be warn, not dbg.
> > >
>
>
> >
> > Also, I just noticed there is a max77705 charger driver. It seems quite
> > similar to this one, maybe it can be leveraged / extended?
>
> Thanks for the feedback. I reviewed the max77705 charger driver. .
>
> Here is a breakdown of why I believe a separate driver may be a better
> approach:
[...]
Thanks for the analysis, I agree with your conclusion. Mainly I noticed that
as part of AICL interrupt handling that driver does a bit of work, while here
we don't. I am wondering if that is applicable here is well.
Cheers,
Andre'
On 1/14/26 2:20 AM, André Draszik wrote:
> On Tue, 2026-01-13 at 16:47 -0800, Amit Sunil Dhamne wrote:
>> Hi Andre',
>>
>> On 1/13/26 2:02 AM, André Draszik wrote:
>>> Hi Amit,
>>>
>>> On Mon, 2026-01-12 at 11:37 -0800, Amit Sunil Dhamne wrote:
>>>> Hi Andre',
>>>>
>>>> On 1/12/26 5:47 AM, André Draszik wrote:
>>>>> Hi Amit,
>>>>>
>>>>> On Tue, 2026-01-06 at 17:14 -0800, Amit Sunil Dhamne wrote:
>>>>>> On 1/6/26 3:41 PM, Amit Sunil Dhamne wrote:
>>>>>>> Hi Andre',
>>>>>>>
>>>>>>> On 1/5/26 9:32 AM, André Draszik wrote:
>>>>>>>> Hi Amit,
>>>>>>>>
>>>>>>>> I haven't done a full review, but a few things caught my eye.
>>>>>>>>
>>>>>>>> On Sat, 2025-12-27 at 00:04 +0000, Amit Sunil Dhamne via B4 Relay wrote:
>>>>>>>>> diff --git a/drivers/power/supply/Makefile
>>>>>>>>> b/drivers/power/supply/Makefile
>>>>>>>>> index 4b79d5abc49a..6af905875ad5 100644
>>>>>>>>> --- a/drivers/power/supply/Makefile
>>>>>>>>> +++ b/drivers/power/supply/Makefile
>>>>>>>>> [...]
>>>>>>>>> +
>>>>>>>>> +static irqreturn_t irq_handler(int irq, void *data)
>>>>>>>>> +{
>>>>>>>>> + struct max77759_charger *chg = data;
>>>>>>>>> + struct device *dev = chg->dev;
>>>>>>>>> + u32 chgint_ok;
>>>>>>>>> + int i;
>>>>>>>>> +
>>>>>>>>> + regmap_read(chg->regmap, MAX77759_CHGR_REG_CHG_INT_OK,
>>>>>>>>> &chgint_ok);
>>>>>>>> You might want to check the return value and return IRQ_NONE if it
>>>>>>>> didn't
>>>>>>>> work?
>>>>>>>>
>>>>>>>>> +
>>>>>>>>> + for (i = 0; i < ARRAY_SIZE(irqs); i++) {
>>>>>>>>> + if (irqs[i] == irq)
>>>>>>>>> + break;
>>>>>>>>> + }
>>>>>>>>> +
>>>>>>>>> + switch (i) {
>>>>>>>>> + case AICL:
>>>>>>>>> + dev_dbg(dev, "AICL mode: %s",
>>>>>>>>> + str_no_yes(chgint_ok & MAX77759_CHGR_REG_CHG_INT_AICL));
>>>>>>>>> + break;
>>>>>>>>> + case CHGIN:
>>>>>>>>> + dev_dbg(dev, "CHGIN input valid: %s",
>>>>>>>>> + str_yes_no(chgint_ok & MAX77759_CHGR_REG_CHG_INT_CHGIN));
>>>>>>>>> + break;
>>>>>>>>> + case CHG:
>>>>>>>>> + dev_dbg(dev, "CHG status okay/off: %s",
>>>>>>>>> + str_yes_no(chgint_ok & MAX77759_CHGR_REG_CHG_INT_CHG));
>>>>>>>>> + break;
>>>>>>>>> + case INLIM:
>>>>>>>>> + dev_dbg(dev, "Current Limit reached: %s",
>>>>>>>>> + str_no_yes(chgint_ok & MAX77759_CHGR_REG_CHG_INT_INLIM));
>>>>>>>>> + break;
>>>>>>>>> + case BAT_OILO:
>>>>>>>>> + dev_dbg(dev, "Battery over-current threshold crossed");
>>>>>>>>> + break;
>>>>>>>>> + case CHG_STA_CC:
>>>>>>>>> + dev_dbg(dev, "Charger reached CC stage");
>>>>>>>>> + break;
>>>>>>>>> + case CHG_STA_CV:
>>>>>>>>> + dev_dbg(dev, "Charger reached CV stage");
>>>>>>>>> + break;
>>>>>>>>> + case CHG_STA_TO:
>>>>>>>>> + dev_dbg(dev, "Charger reached TO stage");
>>>>>>>>> + break;
>>>>>>>>> + case CHG_STA_DONE:
>>>>>>>>> + dev_dbg(dev, "Charger reached TO stage");
>>>>>>>>> + break;
>>>>>>>> Are the above debug messages really all needed?
>>>>>> I forgot to respond to this comment in my previous email.
>>>>>>
>>>>>> I think we can keep AICL, BAT_OILO, INLIM. They're either special
>>>>>> conditions (AICL) or faulty conditions (like BAT_OILO) and we can in
>>>>>> fact keep them at dev_info level. Rest can be removed and a
>>>>>> power_supply_changed() is sufficient.
>>>>>>
>>>>>> Let me know what you think?
>>>>> I don't think dev_info() in an interrupt handler is appropriate. At
>>>>> least it should be ratelimited.
>>>>>
>>>>> If it's something special / unexpected that needs attention, having
>>>>> a dev_dbg() message only will usually not be visible to anybody.
>>>> I agree. I can change the prints to dev_info_ratelimited for the stuff
>>>> we care about.
>>> If it's an erroneous condition, maybe warn or even err are more appropriate?
>>>
>>> But then, what is the expectation upon the user observing these messages?
>>> What can or should they do? Who is going to look at these and can do
>>> something sensible based on them?
>> The logging will help in postmortem analysis which may or may not
>> possible with just publishing uevents to userspace hoping that they log
>> the psy properties. Illustrating a situation:
>>
>> 1. Over current situation happened where the Battery to System current
>> exceeds the BAT_OILO threshold. This would also generate an interrupt.
>>
>> 2. The MAX77759 takes protective measures if the condition lasts for a
>> certain specified time and reset. Resetting will cause Vsys to collapse
>> to 0 if the system is only battery powered.
>>
>> 3. It'd be better that the BAT_OILO interrupt is logged in dmesg,
>> instead of just delegating it to user space as user can debug this
>> condition by looking at last_kmsg or pstore.
>>
>> 4. This signal can help the user debug conditions such as moisture (this
>> signal + contaminant detection) or indicative of a mechanical failure.
>>
>> I do agree though that this is a hypothetical or very rare situation and
>> if you have a strong opinion against this I am okay with removing the
>> prints completely.
> Thanks for details. OK, they sound useful in this case, but should still
> be warn, not dbg.
Sounds good, will fix.
>>
>>> Also, I just noticed there is a max77705 charger driver. It seems quite
>>> similar to this one, maybe it can be leveraged / extended?
>> Thanks for the feedback. I reviewed the max77705 charger driver. .
>>
>> Here is a breakdown of why I believe a separate driver may be a better
>> approach:
> [...]
>
> Thanks for the analysis, I agree with your conclusion. Mainly I noticed that
> as part of AICL interrupt handling that driver does a bit of work, while here
> we don't. I am wondering if that is applicable here is well.
I double-checked the downstream drivers and datasheet. There exists no
issue or WAR for max77759. Also, in case of max77759, the current
limiting will be driven by the hardware and there's no need for software
intervention. In case of max77705, the driver is explicitly reducing the
current limit (in max77705_aicl_irq()), implying that hardware is just
notifying the software to limit current due to (say) poor quality power
source.
BR,
Amit
>
> Cheers,
> Andre'
© 2016 - 2026 Red Hat, Inc.