Message-ID: <606f3de66ec16accfb82d4ea77d0ca9b7e8790d8.camel@linaro.org>
Date: Thu, 22 Jan 2026 12:52:44 +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 v4 4/5] power: supply: max77759: add charger driver

On Thu, 2026-01-22 at 12:47 +0000, André Draszik wrote:
> Hi Amit,
> 
> Thanks for your patches, just a few minor comments below.
> 
> On Wed, 2026-01-21 at 00:59 +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 | 737 ++++++++++++++++++++++++++++++++
> >  4 files changed, 755 insertions(+)
> > 
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 0d044a58cbfe0f2b97f3682a86708e1ece108e9f..38354964a85c34611b1b54e20651b360f3b9c11e 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -15546,6 +15546,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 92f9f7aae92f249aa165e68dbcd4cebb569286ea..3a2cdb95c98e44324151ac2b86d740ae2923ee77 100644
> > --- a/drivers/power/supply/Kconfig
> > +++ b/drivers/power/supply/Kconfig
> > @@ -631,6 +631,17 @@ config CHARGER_MAX77705
> >  	help
> >  	  Say Y to enable support for the Maxim MAX77705 battery charger.
> >  
> > +config CHARGER_MAX77759
> > +	tristate "Maxim MAX77759 battery charger driver"
> > +	depends on MFD_MAX77759 && REGULATOR
> > +	default MFD_MAX77759
> > +	help
> > +	  Say M or Y here to enable the MAX77759 battery charger. 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.
> > +
> >  config CHARGER_MAX77976
> >  	tristate "Maxim MAX77976 battery charger driver"
> >  	depends on I2C
> > diff --git a/drivers/power/supply/Makefile b/drivers/power/supply/Makefile
> > index 4b79d5abc49a7fd1e37a26d0c89f94d9fe3a916f..6af905875ad5e3b393a7030405355b9a975870f6 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 0000000000000000000000000000000000000000..34b5ea0967eb7b4716e81ee1a55227ac872493b0
> > --- /dev/null
> > +++ b/drivers/power/supply/max77759_charger.c
> > @@ -0,0 +1,737 @@
> > +// 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)
> > +};
> > +
> > +#define NUM_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;
> > +	int irqs[NUM_IRQS];
> > +	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;
> 
> Does it really make an impact on performance to specify unlikely? Also, I seem to
> remember that the if branch is treated as unlikely anyway, but can't find any hard
> evidence on that right now.
> 
> > +
> > +	dividend = (s64)val - minval;
> > +	return DIV_S64_ROUND_CLOSEST(dividend, step) + reg_offset;
> > +}
> 
> For these two functions above, have you considered using the APIs from
> include/linux/linear_range.h instead of duplicating in this driver? The
> implementations of the above match linear_range_get_value() and
> linear_range_get_selector_low() quite nicely.
> 
> > +
> > +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 irqreturn_t irq_handler(int irq, void *data)
> > +{
> > +	struct max77759_charger *chg = data;
> > +	struct device *dev = chg->dev;
> > +	int i;
> > +
> > +	for (i = 0; i < NUM_IRQS && chg->irqs[i] != irq; i++)
> > +		;
> > +
> > +	if (i == NUM_IRQS) {
> > +		dev_err(dev, "Unable to handle irq=%d", irq);
> > +		return IRQ_NONE;
> > +	} else if (i == BAT_OILO) {
> > +		dev_warn(dev, "Battery over-current threshold crossed");
> > +	}
> 
> Generally, no 'else' is required after return.
> 
> > +
> > +	power_supply_changed(chg->psy);
> > +	return IRQ_HANDLED;
> > +}
> > +
> 
> [...]
> 
> > +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, online;
> > +	int ret;
> > +
> > +	ret = power_supply_get_property(chg->tcpm_psy,
> > +					POWER_SUPPLY_PROP_CURRENT_MAX,
> > +					&current_limit);
> > +	if (ret) {
> > +		dev_err(chg->dev,
> > +			"Failed to get CURRENT_MAX psy property, ret=%d",
> > +			ret);
> > +		return;
> > +	}
> > +
> > +	ret = power_supply_get_property(chg->tcpm_psy, POWER_SUPPLY_PROP_ONLINE,
> > +					&online);
> > +	if (ret) {
> > +		dev_err(chg->dev,
> > +			"Failed to get ONLINE psy property, ret=%d",
> > +			ret);
> > +		return;
> > +	}
> > +
> > +	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);
> > +	}
> 
> For all the possible errors in this function, should the driver try a bit
> harder, even if unlikely to occur? What if the current limit needed to be
> reduced, e.g. due to thermal or any other reasons?
> 
> Could rescheduling the work be something to consider?
> 
> > +}
> > +
> > +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";
> 
> This can be static const char.
> 
> > +	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 dev_err_probe(dev, ret,
> > +				     "Failed to add devm action to unregister psy notifier");
> > +
> > +	return max77759_init_irqhandler(chg);
> > +}
> > +
> > +static const struct platform_device_id max77759_charger_id[] = {
> > +	{"max77759-charger",},
> 
> Minor formatting nit - I believe common practice is to use named initializers:
> 
> +	{ .compatible = "max77759-charger", },

.name of course for platform_device_id

A.

> 
> > +	{ }
> > +};
> > +MODULE_DEVICE_TABLE(platform, max77759_charger_id);
> > +
> > +static struct platform_driver max77759_charger_driver = {
> > +	.driver = {
> > +		.name = "max77759-charger",
> 
> Can it be async, or are there issues with that?
> 
>                 .probe_type = PROBE_PREFER_ASYNCHRONOUS,
> 
> 
> Thanks again Amit!
> 
> Cheers,
> Andre'
> 
> > +	},
> > +	.probe = max77759_charger_probe,
> > +	.id_table = max77759_charger_id,
> > +};
> > +module_platform_driver(max77759_charger_driver);
> > +
> > +MODULE_AUTHOR("Amit Sunil Dhamne <amitsd@...gle.com>");
> > +MODULE_DESCRIPTION("Maxim MAX77759 charger driver");
> > +MODULE_LICENSE("GPL");

Powered by blists - more mailing lists