| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <6b37b88e9b7ee57eb1c006916fd995c813ab5e6e.camel@linaro.org>
Date: Tue, 13 Jan 2026 10:02:06 +0000
From: André Draszik <andre.draszik@...aro.org>
To: Amit Sunil Dhamne <amitsd@...gle.com>, 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
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?
> >
> > 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?
>
> Yes it does, as I can see a call to kobject_uevent() inside
> power_supply_changed_work(). Also, power_supply_changed() also notifies
> other subsystems that have registered their notifiers downstream of this
> power_supply object. So I believe we're good there.
If erroneous conditions are handled by other / upper layers, why print a
message in this interrupt handler in the first place?
Also, I just noticed there is a max77705 charger driver. It seems quite
similar to this one, maybe it can be leveraged / extended?
Cheers,
Andre'
Powered by blists - more mailing lists