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

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <bb9b9afa-0bfa-428e-9372-549d9ba8603c@google.com>
Date: Mon, 12 Jan 2026 11:37:40 -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

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.


>
> 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 all the above sounds good, I will proceed with sending the next 
revision including the fixes  :).


BR,

Amit

>
> Cheers,
> Andre
>