Re: [PATCH v3 4/5] power: supply: max77759: add charger driver
From: Amit Sunil Dhamne
Date: Wed Jan 14 2026 - 21:52:25 EST
On 1/14/26 2:20 AM, André Draszik wrote:
On Tue, 2026-01-13 at 16:47 -0800, Amit Sunil Dhamne wrote:
Hi Andre',Thanks for details. OK, they sound useful in this case, but should still
On 1/13/26 2:02 AM, André Draszik wrote:
Hi Amit,The logging will help in postmortem analysis which may or may not
On Mon, 2026-01-12 at 11:37 -0800, Amit Sunil Dhamne wrote:
Hi Andre',If it's an erroneous condition, maybe warn or even err are more appropriate?
On 1/12/26 5:47 AM, André Draszik wrote:
Hi Amit,I agree. I can change the prints to dev_info_ratelimited for the stuff
On Tue, 2026-01-06 at 17:14 -0800, Amit Sunil Dhamne wrote:
On 1/6/26 3:41 PM, Amit Sunil Dhamne wrote:I don't think dev_info() in an interrupt handler is appropriate. At
Hi Andre',I forgot to respond to this comment in my previous email.
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/MakefileYou might want to check the return value and return IRQ_NONE if it
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);
didn't
work?
+Are the above debug messages really all needed?
+ 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;
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?
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.
we care about.
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?
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.
be warn, not dbg.
Sounds good, will fix.
[...]
Also, I just noticed there is a max77705 charger driver. It seems quiteThanks for the feedback. I reviewed the max77705 charger driver. .
similar to this one, maybe it can be leveraged / extended?
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.
I double-checked the downstream drivers and datasheet. There exists no issue or WAR for max77759. Also, in case of max77759, the current limiting will be driven by the hardware and there's no need for software intervention. In case of max77705, the driver is explicitly reducing the current limit (in max77705_aicl_irq()), implying that hardware is just notifying the software to limit current due to (say) poor quality power source.
BR,
Amit
Cheers,
Andre'