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

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

Hi Amit,

On Tue, 2026-01-06 at 17:14 -0800, Amit Sunil Dhamne wrote:
> 
> 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:
> > > > 
> > > > 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
> > > > [...]
> > > > +
> > > > +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?

I don't think dev_info() in an interrupt handler is appropriate. At
least it should be ratelimited.

If it's something special / unexpected that needs attention, having
a dev_dbg() message only will usually not be visible to anybody.

Also will the call to power_supply_changed() down below handle the
special conditions (e.g. convey to upper levels)? If not, can it be
made to do so?

Cheers,
Andre