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

["André Draszik" <andre.draszik@linaro.org>]

Hi Amit,

Just a quick comment below for something I noticed during a brief look.

On Sun, 2025-11-23 at 08:35 +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                             |   7 +
>  drivers/mfd/max77759.c                  |   3 +-
>  drivers/power/supply/Kconfig            |  11 +
>  drivers/power/supply/Makefile           |   1 +
>  drivers/power/supply/max77759_charger.c | 866 ++++++++++++++++++++++++++++++++
>  5 files changed, 887 insertions(+), 1 deletion(-)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index fed6cd812d79..f1b1015c08b5 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -15053,6 +15053,13 @@ 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:	Documentation/devicetree/bindings/power/supply/maxim,max77759-charger.yaml
> +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/mfd/max77759.c b/drivers/mfd/max77759.c
> index 5fe22884f362..8a22838be1b0 100644
> --- a/drivers/mfd/max77759.c
> +++ b/drivers/mfd/max77759.c
> @@ -349,7 +349,8 @@ static const struct mfd_cell max77759_maxq_cells[] = {
>  };
>  
>  static const struct mfd_cell max77759_charger_cells[] = {
> -	MFD_CELL_RES("max77759-charger", max77759_charger_resources),
> +	MFD_CELL_OF("max77759-charger", max77759_charger_resources, NULL, 0, 0,
> +		    "maxim,max77759-charger"),
>  };
>  
>  int max77759_maxq_command(struct max77759 *max77759,
> diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
> index 79ddb006e2da..b97990cc0b92 100644
> --- a/drivers/power/supply/Kconfig
> +++ b/drivers/power/supply/Kconfig
> @@ -1074,4 +1074,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
> +	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 f943c9150b32..12669734cfe3 100644
> --- a/drivers/power/supply/Makefile
> +++ b/drivers/power/supply/Makefile
> @@ -122,3 +122,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..51637e87182b
> --- /dev/null
> +++ b/drivers/power/supply/max77759_charger.c
> @@ -0,0 +1,866 @@
> +// 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>
> +
> +/* CHG_INT_OK */
> +#define CHG_INT_OK_AICL_OK			BIT(7)
> +#define CHG_INT_OK_CHGIN_OK			BIT(6)
> +#define CHG_INT_OK_CHG_OK			BIT(4)
> +#define CHG_INT_OK_INLIM_OK			BIT(2)
> +

[...]

> +static irqreturn_t irq_handler(int irq, void *data)
> +{
> +	struct max77759_charger *chg = data;
> +	u32 irq_status, chgint_ok, idx = 0;
> +	int ret;
> +
> +	if (irq == chg->irq[0])
> +		idx = 0;
> +	else
> +		idx = 1;
> +
> +	ret = regmap_read(chg->regmap, MAX77759_CHGR_REG_CHG_INT + idx,
> +			  &irq_status);
> +	if (ret) {
> +		dev_err(chg->dev, "regmap_read_error idx=%d ret=%d", idx, ret);
> +		return IRQ_HANDLED;
> +	}
> +
> +	regmap_write(chg->regmap, MAX77759_CHGR_REG_CHG_INT + idx,
> +		     irq_status);
> +	regmap_read(chg->regmap, MAX77759_CHGR_REG_CHG_INT_OK, &chgint_ok);
> +
> +	if (idx == 0) {
> +		if (irq_status & MAX77759_CHGR_REG_CHG_INT_AICL)
> +			dev_dbg(chg->dev, "AICL mode: %s",
> +				str_no_yes(chgint_ok & CHG_INT_OK_AICL_OK));
> +
> +		if (irq_status & MAX77759_CHGR_REG_CHG_INT_CHGIN)
> +			dev_dbg(chg->dev, "CHGIN input valid: %s",
> +				str_yes_no(chgint_ok & CHG_INT_OK_CHGIN_OK));
> +
> +		if (irq_status & MAX77759_CHGR_REG_CHG_INT_CHG)
> +			dev_dbg(chg->dev, "CHG status okay/off: %s",
> +				str_yes_no(chgint_ok & CHG_INT_OK_CHG_OK));
> +
> +		if (irq_status & MAX77759_CHGR_REG_CHG_INT_INLIM)
> +			dev_dbg(chg->dev, "Current Limit reached: %s",
> +				str_no_yes(chgint_ok & CHG_INT_OK_INLIM_OK));
> +	} else {
> +		if (irq_status & MAX77759_CHGR_REG_CHG_INT2_BAT_OILO)
> +			dev_dbg(chg->dev,
> +				"Battery over-current threshold crossed");
> +
> +		if (irq_status & MAX77759_CHGR_REG_CHG_INT2_CHG_STA_CC)
> +			dev_dbg(chg->dev, "Charger reached CC stage");
> +
> +		if (irq_status & MAX77759_CHGR_REG_CHG_INT2_CHG_STA_CV)
> +			dev_dbg(chg->dev, "Charger reached CV stage");
> +
> +		if (irq_status & MAX77759_CHGR_REG_CHG_INT2_CHG_STA_TO)
> +			dev_dbg(chg->dev, "Charger reached TO stage");
> +
> +		if (irq_status & MAX77759_CHGR_REG_CHG_INT2_CHG_STA_DONE)
> +			dev_dbg(chg->dev, "Charger reached Done stage");
> +	}
> +
> +	power_supply_changed(chg->psy);
> +	return IRQ_HANDLED;
> +}
> +
> +static int max77759_init_irqhandler(struct max77759_charger *chg)
> +{
> +	static const char * const irq_res_names[] = { "INT1", "INT2" };
> +	struct device *dev = chg->dev;
> +	unsigned long irq_flags;
> +	struct irq_data *irqd;
> +	int *irq = chg->irq;
> +	int ret, i;
> +
> +	for (i = 0; i < 2; i++) {
> +		irq[i] = platform_get_irq_byname(to_platform_device(dev),
> +						 irq_res_names[i]);
> +		if (irq[i] < 0) {
> +			dev_err(dev, "unable to find %s irq", irq_res_names[i]);
> +			return irq[i];
> +		}
> +
> +		irq_flags = IRQF_ONESHOT | IRQF_SHARED;
> +		irqd = irq_get_irq_data(irq[i]);
> +		if (irqd)
> +			irq_flags |= irqd_get_trigger_type(irqd);
> +
> +		ret = devm_request_threaded_irq(dev, irq[i], NULL, irq_handler,
> +						irq_flags, dev_name(dev), chg);
> +		if (ret) {
> +			dev_err(dev, "Unable to register threaded irq handler");
> +			return ret;
> +		}
> +	}
> +
> +	return 0;
> +}

The way you're requesting the IRQ here I don't think you need to modify
max77759_chgr_irqs[] in patch 4 at all. You're requesting the interrupt
and are manually demultiplexing and acknowledging the actual event in your
IRQ handler.

Your change in patch 4 would instead allow your charger driver to call
request_irq for each individual bit of the interrupt register(s), letting
regmap-irq demultiplex and acknowledge the event for you before calling
your handler(s). As an example, see how the ACPM result interrupt is
requested in max77759_add_chained_maxq().

It's probably cleaner to let regmap-irq deal with demultiplexing etc.


Cheers,
Andre