Re: [RFC PATCH 0/3] rust: power_supply class abstraction and SMB347 charger driver

[Bruce Robertson <brucer42@gmail.com>]

[Resent as plain text -- my first attempt was rejected by the vger
lists for containing HTML, so it reached only the people Cc'd
directly. Apologies to those of you getting this twice; please reply
to this copy, as the first one never made it to the archives.]

On Sat, Jul 25, 2026 at 01:41:35 +0000, Sebastian Reichel wrote:
> Having Rust bindings for new drivers sounds good to me.

Thanks -- good to hear. That was the part I most wanted confirmed
before putting more time into it.

One status note first: patch 1/3 (the SMBus helpers) is dropped. Igor
Korotin pointed out that I2cClient is meant to implement
kernel::io::Io rather than grow standalone SMBus methods, and there is
an in-flight series from Muchamad Coirul Anwar doing exactly that [1].
The SMB347 driver will consume those accessors once they land.
Patches 2/3 and 3/3 are unaffected -- more on that at the end.

> Any reason for Summit SMB347 specifically? I would prefer not to
> have two drivers for the same hardware.

I wrote the C one. drivers/power/supply/smb347-charger.c came in via
commit ed1a230f96eb ("Add I2C driver for Summit Microelectronics
SMB347 Battery Charger.") in v3.4, back in 2012, and the file header
still carries my name. So this is me porting my own driver rather than
a drive-by rewrite of someone else's -- which I think also speaks to:

> All are ARM32 based, so Rust is at least available by all users.
> So I suppose replacing this C driver would be an option, OTOH I
> don't want to motivate people to rewrite things either :)

Agreed on both counts, and I don't want two drivers either.
Replacement is the goal. But I should be straight that the RFC is
nowhere near it yet: the C driver is ~1640 lines, the Rust one is 180,
with three properties, no IRQ, no set_property, no OTG/regulator
support and no DT charge-parameter configuration.

Two things I can do in the meantime:

 - Make the two mutually exclusive in Kconfig so that no configuration
   ever builds both. For v2 I was planning the ax88796b arrangement --
   the Rust option as a bool modifier on CHARGER_SMB347 with the
   Makefile selecting the object -- rather than a second tristate.

 - Build-test ARM32. So far I have only built and run this on x86_64
   against i2c-stub; I don't have any of the three boards.

What would you want to see before the C driver could actually go away
-- parity against a specific feature list, a hardware test from one of
the espresso10 / nexus7 / p4note users, a deprecation cycle? I can aim
at whatever the bar is, but I can't self-certify a swap on hardware I
don't have.

> A new driver supporting CHARGE_TYPE should also support CHARGE_TYPES.

Happy to add it. One question on how it should map here.

Reading the ABI and the in-tree users, charge_types is read-write: the
supported set comes from the desc->charge_types bitmask, the active
one from get_property, and writing selects an algorithm. On the SMB347
the charge type isn't selectable -- the progression from trickle/
pre-charge through fast to taper is autonomous hardware state that the
driver only reports out of STAT_C. That is why the C driver has never
had a set_property or property_is_writeable at all.

So: would you want CHARGE_TYPES exposed read-only here, with a
Trickle|Fast|N/A bitmask and the active value from STAT_C? Or do you
consider a read-only CHARGE_TYPE to be exactly the mis-modelling that
CHARGE_TYPES is meant to replace? I'll follow whichever you prefer.

Either way it lands on the abstraction rather than just the driver:
desc->charge_types is a descriptor field, not a property callback, so
the Rust Driver trait needs a way to express it, and the writable case
needs set_property and property_is_writeable plumbed through. Those
were on the cover letter's future-work list; this moves them up.

> If the idea is to just have a user for the Rust binding and
> something to test/play around: Maybe convert test_power instead? It
> would also decouple the power-supply Rust bindings from I2C/SMBus.

The decoupling is already there, as it happens. Patch 2/3 touches only
MAINTAINERS, rust/kernel/lib.rs, rust/bindings/bindings_helper.h and
the new rust/kernel/power_supply.rs -- no I2C dependency at all. It is
only the driver in 3/3 that consumes both. And with 1/3 dropped, the
SMBus layer leaves this series entirely.

> The disadvantage is, that this driver is a special-case, since it
> has no parent device :)

That is the part that gives me pause about it as the *first* consumer.
The open question I flagged in the cover letter -- how the get_property
trampoline recovers the driver's private data -- is specifically about
the parent-device path, and it is what reviewers have pushed on most.
A consumer with no parent device wouldn't exercise it at all, so I'd
risk shaping the abstraction around the special case.

For v2 I'm moving the private data into power_supply_config::drv_data
at registration time instead of recovering it from parent drvdata,
which removes the ordering dependency Alice and I discussed [2] -- and
that is also what a parentless supply would need. test_power becomes
straightforward once that's in. I'd be glad to do it as a second
consumer on top; I'd just rather it not be the only one.

[1] https://lore.kernel.org/all/20260707151542.91997-1-muchamadcoirulanwar@gmail.com/
[2] https://lore.kernel.org/all/ak9kAP1SZF8AchQT@google.com/

Bruce