Message-ID: <298ca35590d2180fdcf334f94964b6110e17c606.camel@linaro.org>
Date: Mon, 05 Jan 2026 17:32:24 +0000
From: André Draszik <andre.draszik@...aro.org>
To: amitsd@...gle.com, Sebastian Reichel <sre@...nel.org>, Rob Herring	
 <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley	
 <conor+dt@...nel.org>, Lee Jones <lee@...nel.org>, Greg Kroah-Hartman	
 <gregkh@...uxfoundation.org>, Badhri Jagan Sridharan <badhri@...gle.com>, 
 Heikki Krogerus <heikki.krogerus@...ux.intel.com>, Peter Griffin
 <peter.griffin@...aro.org>, Tudor Ambarus	 <tudor.ambarus@...aro.org>, Alim
 Akhtar <alim.akhtar@...sung.com>
Cc: linux-kernel@...r.kernel.org, linux-pm@...r.kernel.org, 
	devicetree@...r.kernel.org, linux-usb@...r.kernel.org, 
	linux-arm-kernel@...ts.infradead.org, linux-samsung-soc@...r.kernel.org, RD
 Babiera <rdbabiera@...gle.com>, Kyle Tso <kyletso@...gle.com>
Subject: Re: [PATCH v3 4/5] power: supply: max77759: add charger driver

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@...gle.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@...gle.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@...gle.com>
> +L:	linux-kernel@...r.kernel.org
> +S:	Maintained
> +F:	drivers/power/supply/max77759_charger.c
> +
>  MAXIM MAX77802 PMIC REGULATOR DEVICE DRIVER
>  M:	Javier Martinez Canillas <javier@...hile0.org>
>  L:	linux-kernel@...r.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, &regval);
> +	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,
> +				  &current_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'

Powered by blists - more mailing lists