Message-ID: <ff8d1c62-3bb3-4026-b69e-db26e939aa28@google.com>
Date: Mon, 26 Jan 2026 17:08:04 -0800
From: Amit Sunil Dhamne <amitsd@...gle.com>
To: André Draszik <andre.draszik@...aro.org>,
 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

Hi Andre',

On 1/22/26 4:07 PM, Amit Sunil Dhamne wrote:
> Hi Andre',
>
> On 1/22/26 4:47 AM, André Draszik wrote:
>> Hi Amit,
>>
>> Thanks for your patches, just a few minor comments below.
>
> Thanks for your feedback!
>
>
>>
>> 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.
>
> I was hoping it's performance improving even if negligible, though I 
> don't have evidence to support either arguments. In any case, I kept 
> it for readability.
>
>
>>
>>> +
>>> +    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.
>
> I think it looks useful, will check it out.
>
>
>>> +
>>> +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.
>
> I will refactor it in the next rev.
>
>
>>> +
>>> +    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?
>
> Maybe we can do this:
>
> On failure of either of the power_supply_get_property() calls or 
> set_input_current_limit(), we should first turn off the charger (as 
> that's the safest choice) and reschedule this work for a certain 
> number of times (maybe 3). Obviously, if we recover from this state, 
> we reset the error limit and don't reschedule this work anymore. Do 
> you concur?
>
>
>> What if the current limit needed to be
>> reduced, e.g. due to thermal or any other reasons?
>
> This specific piece of code is for setting current limit that is 
> driven by USB Type-C subsystem. If the Type-C subsystem re-negotiates 
> a PD contract this piece of code will re-run as this subsystem will be 
> re-notified by TCPM. For cases where we need to cap the current limit 
> due to thermal management, that's a hardware driven feature. It is 
> enabled by enabling JEITA management. That's not in the scope atm.
>
>
>>
>> Could rescheduling the work be something to consider?
>>
> Responded in the previous comment.. 


Following up on this comment, in case you missed it :)


Thanks,

Amit

Powered by blists - more mailing lists