All of lore.kernel.org
 help / color / mirror / Atom feed
From: Sebastian Reichel <sebastian.reichel@collabora.com>
To: Dmitry Osipenko <digetx@gmail.com>
Cc: Rob Herring <robh@kernel.org>, David Heidelberg <david@ixit.cz>,
	Jonghwa Lee <jonghwa3.lee@samsung.com>,
	Chanwoo Choi <cw00.choi@samsung.com>,
	Myungjoo Ham <myungjoo.ham@samsung.com>,
	Sumit Semwal <sumit.semwal@linaro.org>,
	John Stultz <john.stultz@linaro.org>,
	Vinay Simha BN <simhavcs@gmail.com>,
	Mika Westerberg <mika.westerberg@linux.intel.com>,
	ramakrishna.pallala@intel.com,
	"open list:THERMAL" <linux-pm@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	devicetree@vger.kernel.org
Subject: Re: [PATCH 4/9] dt-bindings: power: supply: Add device-tree binding for Summit SMB3xx
Date: Sat, 9 May 2020 03:14:06 +0200	[thread overview]
Message-ID: <20200509011406.hs7nj3g7f5pzetxp@earth.universe> (raw)
In-Reply-To: <687db60d-fea9-f157-d4ce-907189bb3cc7@gmail.com>

[-- Attachment #1: Type: text/plain, Size: 5893 bytes --]

Hi,

On Wed, Apr 15, 2020 at 06:30:02PM +0300, Dmitry Osipenko wrote:
> 15.04.2020 17:27, Rob Herring пишет:
> > On Fri, Apr 10, 2020 at 2:02 PM Dmitry Osipenko <digetx@gmail.com> wrote:
> >>
> >> 10.04.2020 19:49, Rob Herring пишет:
> >> ...
> >>>> +  summit,max-chg-curr:
> >>>> +    description: Maximum current for charging (in uA)
> >>>> +    allOf:
> >>>> +      - $ref: /schemas/types.yaml#/definitions/uint32
> >>>> +
> >>>> +  summit,max-chg-volt:
> >>>> +    description: Maximum voltage for charging (in uV)
> >>>> +    allOf:
> >>>> +      - $ref: /schemas/types.yaml#/definitions/uint32
> >>>> +    minimum: 3500000
> >>>> +    maximum: 4500000
> >>>> +
> >>>> +  summit,pre-chg-curr:
> >>>> +    description: Pre-charging current for charging (in uA)
> >>>> +    allOf:
> >>>> +      - $ref: /schemas/types.yaml#/definitions/uint32
> >>>> +
> >>>> +  summit,term-curr:
> >>>> +    description: Charging cycle termination current (in uA)
> >>>> +    allOf:
> >>>> +      - $ref: /schemas/types.yaml#/definitions/uint32
> >> ...
> >>> These are all properties of the battery attached and we have standard
> >>> properties for some/all of these.
> >>
> >> Looks like only four properties seem to be matching the properties of
> >> the battery.txt binding.
> >>
> >> Are you suggesting that these matching properties should be renamed
> >> after the properties in battery.txt?
> > 
> > Yes, and that there should be a battery node.
> 
> Usually, it's a battery that has a phandle to the power-supply. Isn't it?

There are two things: The infrastructure described by 
Documentation/devicetree/bindings/power/supply/power-supply.yaml is
used for telling the operating system, that a battery is charged
by some charger. This is done by adding a power-supplies = <&phandle>
in the battery fuel gauge node referencing the charger and probably
what you mean here.

Then we have the infrastructure described by 
Documentation/devicetree/bindings/power/supply/battery.txt, which
provides data about the battery cell. In an ideal world we would
have only smart batteries providing this data, but we don't live
in such a world. So what we currently have is a binding looking
like this:

bat: dumb-battery {
    compatible = "simple-battery";

    // data about battery cell(s)
};

fuel-gauge {
    // fuel-gauge specific data

    supplies = <&charger>;
    monitored-battery = <&bat>;
};

charger: charger {
    // charger specific data

    monitored-battery = <&bat>;
};

In an ideal world, charger should possibly reference fuel-gauge
node, which could provide combined data. Right now we do not have
the infrastructure for that, so it needs to directly reference
the simple-battery node.

> > Possibly you should add
> > new properties battery.txt. It's curious that different properties are
> > needed.
> 
> I guess it should be possible to make all these properties generic.
> 
> Sebastian, will you be okay if we will add all the required properties
> to the power_supply_core?

Extending battery.txt is possible when something is missing. As Rob
mentioned quite a few are already described, though:

summit,max-chg-curr => constant-charge-current-max-microamp
summit,max-chg-volt => constant-charge-voltage-max-microvolt
summit,pre-chg-curr => precharge-current-microamp
summit,term-curr => charge-term-current-microamp

I think at least the battery temperature limits are something, that
should be added to the generic code.

> > Ultimately, for a given battery technology I would expect
> > there's a fixed set of properties needed to describe how to charge
> > them.
> 
> Please notice that the charger doesn't "only charge" the battery,
> usually it also supplies power to the whole device.
> 
> For example, when battery is fully-charged and charger is connected to
> the power source (USB or mains), then battery may not draw any current
> at all.

It is also a question of how good the charging process should be.
Technically I can charge a single cell Li-ion battery without
knowing much, but it can reduce battery life and/or be very slow.
It might even be dangerous, if charging is done at high
temperatures. Also some of the properties in the battery binding
are not about charging, but about gauging. Some devices basically
have only options to measure voltage and voltage drop over a
resistor and everything else must be done by the operating system.

> > Perhaps some of these properties can just be derived from other
> > properties and folks are just picking what a specific charger wants.
> 
> Could be so, but I don't know for sure.

I don't think we have things, that can be derived with a reasonable
amount of effort in the existing simple-battery binding, except for
energy-full-design-microwatt-hours & charge-full-design-microamp-hours.

> Even if some properties could be derived from the others, it won't hurt
> if we will specify everything explicitly in the device-tree.
> 
> > Unfortunately, we have just a mess of stuff made up for each charger
> > out there. I don't have the time nor the experience in this area to do
> > much more than say do better.
>
> I don't think it's a mess in the kernel. For example, it's common that
> embedded controllers are exposed to the system as "just a battery",
> while in fact it's a combined charger + battery controller and the
> charger parameters just couldn't be changed by SW.

A good EC driver exposes a charger and a battery device, so that
userspace can easily identify if a charger is connected.

> In a case of the Nexus 7 devices, the battery controller and charger
> controller are two separate entities in the system. The battery
> controller (bq27541) only monitors status of the battery (charge level,
> temperature and etc).

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

  reply	other threads:[~2020-05-09  1:14 UTC|newest]

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-03-29 16:15 [PATCH 0/9] Summit SMB3xx driver & device-tree David Heidelberg
2020-03-29 16:15 ` [PATCH 1/9] power: supply: smb347-charger: IRQSTAT_D is volatile David Heidelberg
2020-05-10 16:28   ` Sebastian Reichel
2020-03-29 16:15 ` [PATCH 2/9] power: supply: smb347-charger: Add delay before getting IRQSTAT David Heidelberg
2020-05-10 16:28   ` Sebastian Reichel
2020-03-29 16:15 ` [PATCH 3/9] power: supply: smb347-charger: Use resource-managed API David Heidelberg
2020-05-10 16:33   ` Sebastian Reichel
2020-03-29 16:21 ` [PATCH 4/9] dt-bindings: power: supply: Add device-tree binding for Summit SMB3xx David Heidelberg
2020-04-10 16:49   ` Rob Herring
2020-04-10 19:02     ` Dmitry Osipenko
2020-04-15 14:27       ` Rob Herring
2020-04-15 15:30         ` Dmitry Osipenko
2020-05-09  1:14           ` Sebastian Reichel [this message]
2020-05-14 19:34             ` Dmitry Osipenko
2020-03-29 16:21 ` [PATCH 5/9] power: supply: smb347-charger: Implement device-tree support David Heidelberg
2020-03-31  0:25   ` Jonghwa Lee
2020-03-31 17:20     ` Dmitry Osipenko
2020-03-29 16:21 ` [PATCH 6/9] power: supply: smb347-charger: Support SMB345 and SMB358 David Heidelberg
2020-05-10 16:40   ` Sebastian Reichel
2020-03-29 16:21 ` [PATCH 7/9] power: supply: smb347-charger: Remove virtual smb347-battery David Heidelberg
2020-03-29 16:21 ` [PATCH 8/9] power: supply: smb347-charger: Replace mutex with IRQ disable/enable David Heidelberg
2020-03-29 16:21 ` [PATCH 9/9] arm: dts: qcom: apq8064-nexus7: Add smb345 charger node David Heidelberg

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200509011406.hs7nj3g7f5pzetxp@earth.universe \
    --to=sebastian.reichel@collabora.com \
    --cc=cw00.choi@samsung.com \
    --cc=david@ixit.cz \
    --cc=devicetree@vger.kernel.org \
    --cc=digetx@gmail.com \
    --cc=john.stultz@linaro.org \
    --cc=jonghwa3.lee@samsung.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pm@vger.kernel.org \
    --cc=mika.westerberg@linux.intel.com \
    --cc=myungjoo.ham@samsung.com \
    --cc=ramakrishna.pallala@intel.com \
    --cc=robh@kernel.org \
    --cc=simhavcs@gmail.com \
    --cc=sumit.semwal@linaro.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.