linux-kernel - Re: [PATCH v3 2/3] power: supply: Add bd718(15/28/78) 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: <35172e32-cecd-4d6e-9ff1-68dfa409f0ef@gmail.com>
Date: Thu, 28 Aug 2025 16:01:45 +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

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?
> 
>> +/* 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.
> 
>> +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
> 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?

I'll try to clarify this. No promises though ;)

> 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?

I am not 100% sure as I never had the whole setup with me. AFAICS, the 
used battery (at least in Kobo project) was a dummy one, and the device 
driver did fuel-gauge computation(s) based on the battery parameters and 
the CC.

The OCV and VDR tables were indeed measured (but AFAICS, not during 
manufacturing for each independent device. It was instead done during 
the R&D phase, for the type of battery that was used in these devices. 
Or, at least this is how I think it was done - based on discussion with 
HQ colleagues). Anyways, I think Andreas had these extracted from the 
downstream driver.

> 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.
> 
> 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.

Thanks for sharing your insight Linus! It's always great to gain a bit 
more understanding :)

Yours,
	-- Matti