Message-ID: <6341e004-880c-4a81-811d-a8b367aab39d@gmail.com>
Date: Fri, 29 Aug 2025 09:35:00 +0300
From: Matti Vaittinen <mazziesaccount@...il.com>
To: Linus Walleij <linus.walleij@...aro.org>,
 Andreas Kemnade <andreas@...nade.info>
Cc: Lee Jones <lee@...nel.org>, Sebastian Reichel <sre@...nel.org>,
 linux-kernel@...r.kernel.org, linux-pm@...r.kernel.org,
 Krzysztof Kozlowski <krzk@...nel.org>
Subject: Re: [PATCH v3 2/3] power: supply: Add bd718(15/28/78) charger driver

Hi deee Ho again peeps!

I'm back home from Amsterdam, and found the datasheets :)

On 28/08/2025 10:45, Linus Walleij wrote:
> Hi Andreas / Cong,
> 
> On Thu, Aug 21, 2025 at 8:25 PM Andreas Kemnade <andreas@...nade.info> wrote:
> 
>> Add charger driver for ROHM BD718(15/28/78) PMIC charger block.
>> It is a stripped down version of the driver here:
>> https://lore.kernel.org/lkml/dbd97c1b0d715aa35a8b4d79741e433d97c562aa.1637061794.git.matti.vaittinen@fi.rohmeurope.com/
>>
>> For the ease of review and to do a step-by-step approach remove all the
>> coloumb counter related stuff and do not sneak in BD71827 support. That
>> also avoids non-trivial rebasing of the above series.
>>
>> Changes besides that:
>> Replace the custom property by a standard one and do not use megaohms
>> for the current sense resistor.
>>
>> Signed-off-by: Andreas Kemnade <andreas@...nade.info>
>> Reviewed-by: Matti Vaittinen <mazziesaccount@...il.com>
> 
> I think it looks good to merge:
> Reviewed-by: Linus Walleij <linus.walleij@...aro.org>
> 
> It contains some interesting pointers for future work:
> 
>> +/* TODO: Verify the meaning of these interrupts */
>> +BD_ISR_BAT(rechg_det, "Recharging", true)
>> +BD_ISR_BAT(rechg_res, "Recharge ending", true)
>> +BD_ISR_DUMMY(temp_transit, "Temperature transition")
>> +BD_ISR_BAT(therm_rmv, "bd71815-therm-rmv", false)
>> +BD_ISR_BAT(therm_det, "bd71815-therm-det", true)
>> +BD_ISR_BAT(bat_dead, "bd71815-bat-dead", false)
>> +BD_ISR_BAT(bat_short_res, "bd71815-bat-short-res", true)
>> +BD_ISR_BAT(bat_short, "bd71815-bat-short-det", false)
>> +BD_ISR_BAT(bat_low_res, "bd71815-bat-low-res", true)
>> +BD_ISR_BAT(bat_low, "bd71815-bat-low-det", true)
>> +BD_ISR_BAT(bat_ov_res, "bd71815-bat-over-res", true)
> 
> Some of these look like they should immediately shut down the
> system, I suppose the battery charger does this autonomously
> but it should probably also trigger an emergency shutdown
> of Linux, right?

Yes. The shutdown for charging, or, in some cases for all power outputs, 
is automatically handled by the PMIC hardware. (Well, I am not sure 
about the 'over-current' IRQ, will see if I can find out more about it).

>> +/* What should we do here? */
>> +BD_ISR_BAT(bat_ov, "bd71815-bat-over-det", false)
> 
> At overvoltage all charging should stop, I guess the hardware
> does that autonomouslyd and this is just to inform us that
> this has happened.

Yes. That's my understanding.

>> +BD_ISR_BAT(bat_mon_res, "bd71815-bat-mon-res", true)
>> +BD_ISR_BAT(bat_mon, "bd71815-bat-mon-det", true)
>> +BD_ISR_BAT(bat_cc_mon, "bd71815-bat-cc-mon2", false)
>> +BD_ISR_BAT(bat_oc1_res, "bd71815-bat-oc1-res", true)
>> +BD_ISR_BAT(bat_oc1, "bd71815-bat-oc1-det", false)
>> +BD_ISR_BAT(bat_oc2_res, "bd71815-bat-oc2-res", true)
>> +BD_ISR_BAT(bat_oc2, "bd71815-bat-oc2-det", false)
>> +BD_ISR_BAT(bat_oc3_res, "bd71815-bat-oc3-res", true)
>> +BD_ISR_BAT(bat_oc3, "bd71815-bat-oc3-det", false)
>> +BD_ISR_BAT(temp_bat_low_res, "bd71815-temp-bat-low-res", true)
>> +BD_ISR_BAT(temp_bat_low, "bd71815-temp-bat-low-det", true)
>> +BD_ISR_BAT(temp_bat_hi_res, "bd71815-temp-bat-hi-res", true)
>> +BD_ISR_BAT(temp_bat_hi, "bd71815-temp-bat-hi-det", true)
> 
> The "oc" stuff is "open circuit" and probably different interrupts

I see I chose a confusing prefix, 'oc', sorry. In this case it doesn't 
come from "open circuit" but "over current" - and yes, I had to look it 
up from the MFD driver / data-sheet - which is a clear indication naming 
isn't as good as it could be. Anyways, it seems these over-current 
limits for the IRQ can be set by the software. I'm not sure if exceeding 
these limits causes hardware to do anything else but an IRQ though.

> to indicate that a measurement of the open circuit voltage
> is available in some register, which enables you do do more
> precise capacity estimation, right?

AFAIR, the ROHM fuel-gauge algorithm used OCV tables when battery was 
not really open, but 'relaxed', to adjust the coulomb counter based on 
the SOC estimated from the OCV. The 'relaxed' condition was met when the 
current consumption had been 'small', and battery had not been charged 
'recently'. I have a vague memory the BD71828 had some hardware support 
for knowing battery was 'relaxed', the BD71815 and BD71827 might have 
used coulomb counter 'history' for this. I can try to remember all this 
a bit better if Andreas continues to upstream also the fuel-gauging 
logic from the original RFC. But yeah, these interrupts were for over 
current.

> If it is useful depends on if the device is e.g. flashed with
> capacity tables vs OC voltage in the factory, and the charger
> then has "deep knowledge" of the battery it is handling, I guess
> this is how it works? Then this again is just informational, it
> informs you of what the device-internal charging algorithm
> is doing.
> 
> As for the temperature these are probably also just informational
> if all these states are already handled by the hardware itself.

Yes. AFAIR, the boundaries for the thermal areas can be adjusted by 
software though. [There are also the "thermal shutdown" limits when also 
the PMIC's other power outputs (not just charging) will be disabled. 
AFAIR, the TSD limit can't be configured though.] On certain temperature 
areas the charging is reduced, and the "amount of reduction" can also be 
set by the software.

State transitions are still handled by the hardware so these interrupts 
are indeed only informative.

> They all seem to be intended so that you can essentially put
> these into custom sysfs or debugfs entries and then get out
> a graphical state machine of the battery charger. That requires
> deep knowledge of how the charger works (a datasheet)
> and has limited use for a normal user.

Yours,
	-- Matti

Powered by blists - more mailing lists