Re: [PATCH v10 1/8] devicetree: power: Add battery.txt

[Sebastian Reichel <sre@kernel.org>]

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

Hi,

On Wed, Mar 15, 2017 at 03:18:17PM -0700, Liam Breck wrote:
> Hey Sebastian, welcome back :-)
> 
> I've taken over work on this patchset from Matt.

Ok.

> On Wed, Mar 15, 2017 at 3:04 PM, Sebastian Reichel <sre@kernel.org> wrote:
> > Hi Liam & Matt,
> >
> > Sorry for my absence in the discussion. I skipped the previous
> > iterations for now and start with this version.
> >
> > On Wed, Mar 15, 2017 at 12:26:46PM -0700, Liam Breck wrote:
> >> From: Liam Breck <kernel@networkimprov.net>
> >>
> >> Documentation of static battery characteristics that can be defined
> >> for batteries which cannot self-identify. This information is required
> >> by fuel-gauge and charger chips for proper handling of the battery.
> >>
> >> Cc: Rob Herring <robh@kernel.org>
> >> Cc: devicetree@vger.kernel.org
> >> Signed-off-by: Matt Ranostay <matt@ranostay.consulting>
> >> Signed-off-by: Liam Breck <kernel@networkimprov.net>
> >> ---
> >>  .../devicetree/bindings/power/supply/battery.txt   | 45 ++++++++++++++++++++++
> >>  1 file changed, 45 insertions(+)
> >>  create mode 100644 Documentation/devicetree/bindings/power/supply/battery.txt
> >>
> >> diff --git a/Documentation/devicetree/bindings/power/supply/battery.txt b/Documentation/devicetree/bindings/power/supply/battery.txt
> >> new file mode 100644
> >> index 0000000..0278617
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/power/supply/battery.txt
> >> @@ -0,0 +1,45 @@
> >> +Battery Characteristics
> >
> > Maybe add something like
> >
> > This device provides static battery information, that is usually
> > available in the EEPROM of a smart battery. It's supposed to be
> > used for batteries, which do not have their own EEPROM (or if its
> > unusable).
> 
> OK.
> 
> >> +Required Properties:
> >> + - compatible: Must be "fixed-battery"
> 
> Rob questioned the term "fixed". Shall we consider other terms --
> simple-battery, plain-battery...?

I don't like the term either, but it was the best I came up with.
They are known as "dumb" batteries, but that term looks too
colloquial for DT usage. While I think "simple-battery" is not
perfect either, it's better than "fixed-battery", so please switch
to that in the next revision.

> >> +Optional Properties:
> >> + - voltage-min-design-microvolt: drained battery voltage
> >> + - energy-full-design-microwatt-hours: battery design energy
> >> + - charge-full-design-microamp-hours: battery design capacity
> >
> > Looks fine to me.
> >
> >> +Because drivers surface properties in sysfs using names derived
> >> +from enum power_supply_property, e.g.
> >> +/sys/class/power_supply/<device>/charge_full_design, our
> >> +battery properties must be named for the corresponding elements in
> >> +enum power_supply_property, defined in include/linux/power_supply.h.
> >
> > This is Linux/implementation specific and does not belong
> > into a DT binding document.
> 
> I just wrote on a thread for an earlier version of this patch:
> 
> "Sebastian proposed DT:battery specifically to be consumed by
> power_supply_core. Allowing names in DT:battery and
> power_supply_property to diverge would cause confusion and wasted
> time, for no particular benefit. As there is no rationale to
> reconsider the names of these fields for DT:battery, let's write that
> into the docs."

DT bindings are not "Linux hardware information bindings". Of course
there is no need to diverge without a good reason, but that kind of
Documentation just does not belong into the DT bindings.

> >> +Batteries must be referenced by chargers and/or fuel-gauges
> >> +using a phandle. The phandle's property should be named
> >> +"monitored-battery".
> >
> > This looks fine.
> >
> >> +Driver code should call power_supply_get_battery_info() to obtain
> >> +battery properties via monitored-battery. For details see:
> >> +  drivers/power/supply/power_supply_core.c
> >> +  drivers/power/supply/bq27xxx_battery.c
> >
> > This is also Linux/implementation specific and should be dropped.
> 
> We ought to mention how drivers are expected to consume DT:battery.

That kind of documentation does not belong into
Documentation/devicetree/bindings. We can add something to
Documentation/power/power_supply_class.txt instead.

> >> +Example:
> >> +
> >> +     bat: battery {
> >> +             compatible = "fixed-battery";
> >> +             voltage-min-design-microvolt = <3200000>;
> >> +             energy-full-design-microwatt-hours = <5290000>;
> >> +             charge-full-design-microamp-hours = <1430000>;
> >> +     };
> >> +
> >> +     charger: charger@11 {
> >> +             ....
> >> +             monitored-battery = <&bat>;
> >> +             ...
> >> +     };
> >> +
> >> +     fuel_gauge: fuel-gauge@22 {
> >> +             ....
> >> +             monitored-battery = <&bat>;
> >> +             ...
> >> +     };
> >> --
> >> 2.9.3
> >>

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