[PATCH v3 4/5] power: supply: max77759: add charger driver
André Draszik
andre.draszik at linaro.org
Wed Jan 14 02:20:17 PST 2026
On Tue, 2026-01-13 at 16:47 -0800, Amit Sunil Dhamne wrote:
> Hi Andre',
>
> On 1/13/26 2:02 AM, André Draszik wrote:
> > Hi Amit,
> >
> > On Mon, 2026-01-12 at 11:37 -0800, Amit Sunil Dhamne wrote:
> > > Hi Andre',
> > >
> > > On 1/12/26 5:47 AM, André Draszik wrote:
> > > > 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.
> > > I agree. I can change the prints to dev_info_ratelimited for the stuff
> > > we care about.
> > If it's an erroneous condition, maybe warn or even err are more appropriate?
> >
> > But then, what is the expectation upon the user observing these messages?
> > What can or should they do? Who is going to look at these and can do
> > something sensible based on them?
>
> The logging will help in postmortem analysis which may or may not
> possible with just publishing uevents to userspace hoping that they log
> the psy properties. Illustrating a situation:
>
> 1. Over current situation happened where the Battery to System current
> exceeds the BAT_OILO threshold. This would also generate an interrupt.
>
> 2. The MAX77759 takes protective measures if the condition lasts for a
> certain specified time and reset. Resetting will cause Vsys to collapse
> to 0 if the system is only battery powered.
>
> 3. It'd be better that the BAT_OILO interrupt is logged in dmesg,
> instead of just delegating it to user space as user can debug this
> condition by looking at last_kmsg or pstore.
>
> 4. This signal can help the user debug conditions such as moisture (this
> signal + contaminant detection) or indicative of a mechanical failure.
>
> I do agree though that this is a hypothetical or very rare situation and
> if you have a strong opinion against this I am okay with removing the
> prints completely.
Thanks for details. OK, they sound useful in this case, but should still
be warn, not dbg.
> > >
>
>
> >
> > Also, I just noticed there is a max77705 charger driver. It seems quite
> > similar to this one, maybe it can be leveraged / extended?
>
> Thanks for the feedback. I reviewed the max77705 charger driver. .
>
> Here is a breakdown of why I believe a separate driver may be a better
> approach:
[...]
Thanks for the analysis, I agree with your conclusion. Mainly I noticed that
as part of AICL interrupt handling that driver does a bit of work, while here
we don't. I am wondering if that is applicable here is well.
Cheers,
Andre'
More information about the linux-arm-kernel
mailing list