Re: [RFC PATCH v2 09/10] power: supply: Initial support for ROHM BD70528 PMIC charger block

Hello Linus,

First of all - thanks for taking the time to help me once again.

I just some time ago did submit the RFC for BD99954 - and immediately
regretted doing so as only few minutes later I spotted the already
existing battery code/bindings. It's quite embarrassing to send out an
email for wide audience just to notice that I've written large amount
of nonsense :/ (I suggested adding common bindings and parsers and
didn't notice we already had bunch of them).

On Mon, 2020-02-10 at 14:12 +0100, Linus Walleij wrote:

On Thu, Feb 6, 2020 at 8:54 AM Vaittinen, Matti
[off-list ref] wrote:

quoted

quoted

They all seem to have trickle and fast charging, USB phy
interaction and AC plug interaction of some kind for detecting
those and in some cases temperature detection directly
or using an ADC.

Yes. That's pretty much what these ROHM chips have. But SW mostly
needs
to set the limit values - it does not need to participate in state
changing and most of these things are also something users do not
care
(I guess). Fuel gauging is other topic but I'm not planning to
implement it for in-tree drivers at least for now :/

The thing that comes to (my) mind is just a common DT properties
for
input current/voltage limits, battery voltages for switching to
next
charging state, current values for different charging states - and
parsing of these properties.

I've seen some comments about DT not being a good place for battery
specific properties as batteries may be replaced with another
having
different type - but I don't think this is valid problem for many
of
the devices today. Of course some devices may have batteries that
can
be changed - but many don't - and for those it is perfectly Ok to
have
battery data defined in DT, right?

If something is a property of the charging circuit, then it can be
derived
from the compatible value, so anything that is just peculiar for this
chip version doesn't go into device tree, just use a custom struct
and
the match data for that.

Agree. Assuming the limitation comes from charging circuit itself. But
I thought that for example some input current limits might come from
the external circuitry (neither from charger, nor from battery).

Then we have the battery. I suppose that could be defined as a DT
node for the battery itself, and not as part of the charging IC, and
we
should think about how to create a battery DT binding.

Oh, yes. This is my newest mistake. We already seem to have the
"simple-battery" bindings and a helper to retrieve given configs. After
I understood that I noticed that there is only couple of bindings I
might add there.

 What
characteristics are chargers really interested in? This could be some
work.

If different devices have different batteries then the boot loader
could ideally patch the DT with the right battery. This was the
solution
advocated for devices with different display panels, to a question
from Hans de Goede at one point IIRC.

Hmm. This sounds simpler than adding bunch of charger drivers in boot
loader. OTOH, if boot loader can detect the type of the battery, then I
see why linux couldn't? And if type of battery can be hard-coded in
boot, then I don't see why it couldn't be hard-coded in DT. But this
all goes far beyond my area of expertice ;)

quoted

But ... do we need to provide a way for user-space to configure
these
settings instead of the DT? Or any other possible sources for this
information?

What we usually have said is that "policy should be in userspace"
and "kernel manages hardware". This creates a not-so-small
grey area in between and it is unfortunately up to interpretation.

Today there is also a THIRD alternative, and that is to let
userspace insert a policy using a BPF program. This is the
approach taken by the network and tracing stacks and we look at
more applications. If the kernel needs to be able to handle charging
and emergencies even if userspace is not up or available, this or
other
userspace policies are not viable.

Just as a theoretical discussion this is interesting topic. I think the
fuel-gauge might be area where we could want to avoid waking up the
user-space when states change. But for my current crusade to power-
supply-world we are mostly limited to one-time configurations like
battery properties and system constraints for input currents etc.

I have a strong feeling that it should be a battery node in the
device
tree.

Yes. Now that I learned about it - mostly so. Except the input current
limits and VSYS regulation.

quoted

Hmm. If changing means state changes - then these ROHM chips
probably
play no role here as there the HW does most of the state changes
autonomously. (AFAIR, it's been a while since I looked at the other
but
BD99954).

Good it's a simpler hardware. We just need to think about making
the code reusable.

quoted

So ... If I extract the linear_range code from BD70528 driver (it
should be usable for the BD99954 as such) - should I place it to
some
common header/lib? (This is plain integer math so I don't like the
idea of placing it in fixp-arith.h).

Start with moving it to some separate file, like
power_supply_interpolation.c that we just always compile in
and we can take it from there.

Oh, that's pretty much what I did :) And that is pretty much the only
patch that makes sense in this first RFC version :|

Just some thoughts...

Thanks Linus. I _really_ appreciate your time and help! :)

Best Regards,
	Matti Vaittinen