Message-ID: <255d7726-6758-43ed-b35f-db14726bcc9b@google.com>
Date: Tue, 6 Jan 2026 17:14:11 -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 v3 4/5] power: supply: max77759: add charger driver


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@...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?

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, &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.
>
> Thanks for the detailed review! I will fix all the flagged issues in 
> the next rev.
>
>
> BR,
>
> Amit
>
>>
>> Cheers,
>> Andre'

Powered by blists - more mailing lists