[PATCH v6 0/8] devicetree battery support and client bq27xxx_battery

[PATCH v6 0/8] devicetree battery support and client bq27xxx_battery

[Liam Breck]

Overview:
* new devicetree battery node specifies static battery data
* fuel gauge and charger nodes shall use monitored-battery=<&battery_node>
* new power_supply_get_battery_info() reads battery data from devicetree
* new struct power_supply_battery_info provides battery data to drivers
* drivers surface battery data in sysfs via related power_supply_prop_* fields
* bq27xxx driver calls the above and writes battery data to NVM for params
  essential to correct operation: energy-full-design-microwatt-hours,
  charge-full-design-microamp-hours, voltage-min-design-microvolt

Changes in v6:
* Documentation/devictree/... fixes
* bq27xxx_battery: clarify names
* bq27xxx_battery: verify that selected registers are supported
* bq27xxx_battery: allocate NVM buffer on stack
* bq27xxx_battery_i2c: fix return code of bulk_read

Changes in v5:
* incorporate feedback into Documentation/devicetree/.../battery.txt
* use power_supply_prop_* names in devicetree and power_supply_battery_info
* default fields to -EINVAL in power_supply_battery_info
* power_supply_get_battery_info() always looks for "monitored-battery"
* power_supply_get_battery_info() emits a warning if !psy->of_node
* squash patches for power_supply_battery_info
* bq27xxx_battery: check power_supply_battery_info values
* bq27xxx_battery: note missing power_supply_prop_* features
* bq27xxx_battery: new patch for access methods

Changes in v4:
* add "fixed-battery" compatible field to be be more consistant with devicetree

Changes in v3:
* split i2c changes into respective patches
* add documentation for battery information for fuel gauge
* rebased documentation patches on change on the list
* abstracted the battery configuration for the state machine
  to an generic struct and platform data access function

Changes in v2:
* add documentation for uWh and uAh property units
* change devicetree entries to match new property units

Matt Ranostay (8):
  devicetree: power: Add battery.txt
  devicetree: property-units: Add uWh and uAh units
  devicetree: power: bq27xxx: Add monitored-battery documentation
  power: power_supply: Add power_supply_battery_info and API
  power: bq27xxx_battery: Define access methods to write chip registers
  power: bq27xxx_battery: Add BQ27425 chip id
  power: bq27xxx_battery: Add power_supply_battery_info support
  power: bq27xxx_battery_i2c: Add I2C bulk read/write functions

 .../devicetree/bindings/power/supply/battery.txt   |  37 +++
 .../devicetree/bindings/power/supply/bq27xxx.txt   |  11 +-
 .../devicetree/bindings/property-units.txt         |   2 +
 drivers/power/supply/bq27xxx_battery.c             | 325 ++++++++++++++++++++-
 drivers/power/supply/bq27xxx_battery_i2c.c         |  67 ++++-
 drivers/power/supply/power_supply_core.c           |  40 +++
 include/linux/power/bq27xxx_battery.h              |   6 +-
 include/linux/power_supply.h                       |  18 ++
 8 files changed, 501 insertions(+), 5 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/power/supply/battery.txt

-- 
2.9.3

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

[Liam Breck]

From: Matt Ranostay <matt@ranostay.consulting>

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   | 37 ++++++++++++++++++++++
 1 file changed, 37 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..d78a4aa
--- /dev/null
+++ b/Documentation/devicetree/bindings/power/supply/battery.txt
@@ -0,0 +1,37 @@
+Battery Characteristics
+
+Required Properties:
+ - compatible: Must be "fixed-battery"
+
+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
+
+Future Properties must be named for the corresponding elements in
+enum power_supply_property, defined in include/linux/power_supply.h.
+
+Batteries must be referenced by chargers and/or fuel-gauges
+using a phandle. The phandle's property should be named
+"monitored-battery".
+
+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

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

[Andrew F. Davis]

On 02/10/2017 08:43 PM, Liam Breck wrote:
> From: Matt Ranostay <matt-sk+viVC6FLCDq+mSdOJa79kegs52MxvZ@public.gmane.org>
> 
> 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.


I think some of the confusion about what a "smart-battery" binding would
look like is due to a miss-understanding what it means to
"self-identify". Originally the bq27xxx *was* the chip that lived its
whole life inside a battery (the iphone battery is one such example and
a good way to test the bq27000). This way it could be factory programmed
with the battery's basics and then learn about the individual pack over
time to provide better info on time to empty, discharge rates, etc..

Eventually, people wanted to have the battery become cheap and
replaceable, so fuel-gauges were made that are "system side", and ride
along with the device, not the battery. Now this has one big problem,
how can we learn about a battery when it may be a new battery every time
we boot up? So we added programmable memory to the devices, and
programming this memory looks to be what this series is about.

Only the battery design info is programmed in this series, but much more
battery info can be uploaded, including stuff that is not fixed and
therefor cannot be added to the DT. Even the design parameters may not
be fix if one changes the battery as different manufactures of the same
battery type may have different design capacities.

The ideal use-case in a system with a system side gauge is the OS should
periodically, or just before shutdown, read the battery's learned info
and when the device is powered back up, if the battery serial number
matches from before, re-upload this learned info to the gauge so it
doesn't have to re-learn everything. All of this will need to happen in
user-space, so what we really need is a user-space API for manipulating
these properties.

The properties added in this series are okay, and it's good to have some
sane defaults in DT, but looking forward this may not be enough to get
full use out of these devices.

Andrew

> 
> Cc: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Signed-off-by: Matt Ranostay <matt-sk+viVC6FLCDq+mSdOJa79kegs52MxvZ@public.gmane.org>
> Signed-off-by: Liam Breck <kernel-RYWXG+zxWwBdeoIcmNTgJF6hYfS7NtTn@public.gmane.org>
> ---
>  .../devicetree/bindings/power/supply/battery.txt   | 37 ++++++++++++++++++++++
>  1 file changed, 37 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..d78a4aa
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/supply/battery.txt
> @@ -0,0 +1,37 @@
> +Battery Characteristics
> +
> +Required Properties:
> + - compatible: Must be "fixed-battery"
> +
> +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
> +
> +Future Properties must be named for the corresponding elements in
> +enum power_supply_property, defined in include/linux/power_supply.h.
> +
> +Batteries must be referenced by chargers and/or fuel-gauges
> +using a phandle. The phandle's property should be named
> +"monitored-battery".
> +
> +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>;
> +		...
> +	};
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Liam Breck]

Hi Andrew, thanks for the drill-down on BQ27* chips.

On Mon, Feb 13, 2017 at 8:06 AM, Andrew F. Davis <afd-l0cyMroinI0@public.gmane.org> wrote:
> On 02/10/2017 08:43 PM, Liam Breck wrote:
>> From: Matt Ranostay <matt-sk+viVC6FLCDq+mSdOJa79kegs52MxvZ@public.gmane.org>
>>
>> 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.
>
>
> I think some of the confusion about what a "smart-battery" binding would
> look like is due to a miss-understanding what it means to
> "self-identify". Originally the bq27xxx *was* the chip that lived its
> whole life inside a battery (the iphone battery is one such example and
> a good way to test the bq27000). This way it could be factory programmed
> with the battery's basics and then learn about the individual pack over
> time to provide better info on time to empty, discharge rates, etc..
>
> Eventually, people wanted to have the battery become cheap and
> replaceable, so fuel-gauges were made that are "system side", and ride
> along with the device, not the battery. Now this has one big problem,
> how can we learn about a battery when it may be a new battery every time
> we boot up? So we added programmable memory to the devices, and
> programming this memory looks to be what this series is about.
>
> Only the battery design info is programmed in this series, but much more
> battery info can be uploaded, including stuff that is not fixed and
> therefor cannot be added to the DT. Even the design parameters may not
> be fix if one changes the battery as different manufactures of the same
> battery type may have different design capacities.
>
> The ideal use-case in a system with a system side gauge is the OS should
> periodically, or just before shutdown, read the battery's learned info
> and when the device is powered back up, if the battery serial number
> matches from before, re-upload this learned info to the gauge so it
> doesn't have to re-learn everything. All of this will need to happen in
> user-space, so what we really need is a user-space API for manipulating
> these properties.

We would need a feature in the driver to get/set the blob of pack- or
cell-unique data. The userspace api is one sysfs file.

However the DT fixed-battery node could indicate when/how-often the
object is stored, and where, e.g. filename. If the latter is given,
the driver should write/read the file itself.

Is there some documentation about what registers should go in this blob?

That concept is beyond the scope of this patchset (and doesn't
supersede it), but is something I'd like to implement for our chip,
the *425.


> The properties added in this series are okay, and it's good to have some
> sane defaults in DT, but looking forward this may not be enough to get
> full use out of these devices.

I'm relieved you're not opposed to our patch :-) Now I just need that
guidance I mentioned re the chip family...


>> Cc: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>> Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> Signed-off-by: Matt Ranostay <matt-sk+viVC6FLCDq+mSdOJa79kegs52MxvZ@public.gmane.org>
>> Signed-off-by: Liam Breck <kernel-RYWXG+zxWwBdeoIcmNTgJF6hYfS7NtTn@public.gmane.org>
>> ---
>>  .../devicetree/bindings/power/supply/battery.txt   | 37 ++++++++++++++++++++++
>>  1 file changed, 37 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..d78a4aa
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/power/supply/battery.txt
>> @@ -0,0 +1,37 @@
>> +Battery Characteristics
>> +
>> +Required Properties:
>> + - compatible: Must be "fixed-battery"
>> +
>> +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
>> +
>> +Future Properties must be named for the corresponding elements in
>> +enum power_supply_property, defined in include/linux/power_supply.h.
>> +
>> +Batteries must be referenced by chargers and/or fuel-gauges
>> +using a phandle. The phandle's property should be named
>> +"monitored-battery".
>> +
>> +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>;
>> +             ...
>> +     };
>>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Andrew F. Davis]

On 02/13/2017 02:58 PM, Liam Breck wrote:
> Hi Andrew, thanks for the drill-down on BQ27* chips.
> 
> On Mon, Feb 13, 2017 at 8:06 AM, Andrew F. Davis <afd-l0cyMroinI0@public.gmane.org> wrote:
>> On 02/10/2017 08:43 PM, Liam Breck wrote:
>>> From: Matt Ranostay <matt-sk+viVC6FLCDq+mSdOJa79kegs52MxvZ@public.gmane.org>
>>>
>>> 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.
>>
>>
>> I think some of the confusion about what a "smart-battery" binding would
>> look like is due to a miss-understanding what it means to
>> "self-identify". Originally the bq27xxx *was* the chip that lived its
>> whole life inside a battery (the iphone battery is one such example and
>> a good way to test the bq27000). This way it could be factory programmed
>> with the battery's basics and then learn about the individual pack over
>> time to provide better info on time to empty, discharge rates, etc..
>>
>> Eventually, people wanted to have the battery become cheap and
>> replaceable, so fuel-gauges were made that are "system side", and ride
>> along with the device, not the battery. Now this has one big problem,
>> how can we learn about a battery when it may be a new battery every time
>> we boot up? So we added programmable memory to the devices, and
>> programming this memory looks to be what this series is about.
>>
>> Only the battery design info is programmed in this series, but much more
>> battery info can be uploaded, including stuff that is not fixed and
>> therefor cannot be added to the DT. Even the design parameters may not
>> be fix if one changes the battery as different manufactures of the same
>> battery type may have different design capacities.
>>
>> The ideal use-case in a system with a system side gauge is the OS should
>> periodically, or just before shutdown, read the battery's learned info
>> and when the device is powered back up, if the battery serial number
>> matches from before, re-upload this learned info to the gauge so it
>> doesn't have to re-learn everything. All of this will need to happen in
>> user-space, so what we really need is a user-space API for manipulating
>> these properties.
> 
> We would need a feature in the driver to get/set the blob of pack- or
> cell-unique data. The userspace api is one sysfs file.
> 
> However the DT fixed-battery node could indicate when/how-often the
> object is stored, and where, e.g. filename. If the latter is given,
> the driver should write/read the file itself.
> 
> Is there some documentation about what registers should go in this blob?
> 

This should all be available in the parts' technical reference (looks
like page 24 for your device), but the blob isn't meant to be modified,
the information/format is mostly internal to the part, all we should do
is save blob / restore blob as needed.

> That concept is beyond the scope of this patchset (and doesn't
> supersede it), but is something I'd like to implement for our chip,
> the *425.
> 
> 
>> The properties added in this series are okay, and it's good to have some
>> sane defaults in DT, but looking forward this may not be enough to get
>> full use out of these devices.
> 
> I'm relieved you're not opposed to our patch :-) Now I just need that
> guidance I mentioned re the chip family...
> 

Still looking into it, I think for now would be easiest to add the
register info to each chip family struct. The un-seal sequence should be
generic enough already.

Andrew

> 
>>> Cc: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>>> Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>>> Signed-off-by: Matt Ranostay <matt-sk+viVC6FLCDq+mSdOJa79kegs52MxvZ@public.gmane.org>
>>> Signed-off-by: Liam Breck <kernel-RYWXG+zxWwBdeoIcmNTgJF6hYfS7NtTn@public.gmane.org>
>>> ---
>>>  .../devicetree/bindings/power/supply/battery.txt   | 37 ++++++++++++++++++++++
>>>  1 file changed, 37 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..d78a4aa
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/power/supply/battery.txt
>>> @@ -0,0 +1,37 @@
>>> +Battery Characteristics
>>> +
>>> +Required Properties:
>>> + - compatible: Must be "fixed-battery"
>>> +
>>> +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
>>> +
>>> +Future Properties must be named for the corresponding elements in
>>> +enum power_supply_property, defined in include/linux/power_supply.h.
>>> +
>>> +Batteries must be referenced by chargers and/or fuel-gauges
>>> +using a phandle. The phandle's property should be named
>>> +"monitored-battery".
>>> +
>>> +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>;
>>> +             ...
>>> +     };
>>>
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Liam Breck]

[Dropped devicetree from CC]

On Mon, Feb 13, 2017 at 1:12 PM, Andrew F. Davis <afd@ti.com> wrote:
> On 02/13/2017 02:58 PM, Liam Breck wrote:
>> Hi Andrew, thanks for the drill-down on BQ27* chips.
>>
>> On Mon, Feb 13, 2017 at 8:06 AM, Andrew F. Davis <afd@ti.com> wrote:
>>> On 02/10/2017 08:43 PM, Liam Breck wrote:
>>>> From: Matt Ranostay <matt@ranostay.consulting>
>>>>
>>>> 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.
>>>
>>>
>>> I think some of the confusion about what a "smart-battery" binding would
>>> look like is due to a miss-understanding what it means to
>>> "self-identify". Originally the bq27xxx *was* the chip that lived its
>>> whole life inside a battery (the iphone battery is one such example and
>>> a good way to test the bq27000). This way it could be factory programmed
>>> with the battery's basics and then learn about the individual pack over
>>> time to provide better info on time to empty, discharge rates, etc..
>>>
>>> Eventually, people wanted to have the battery become cheap and
>>> replaceable, so fuel-gauges were made that are "system side", and ride
>>> along with the device, not the battery. Now this has one big problem,
>>> how can we learn about a battery when it may be a new battery every time
>>> we boot up? So we added programmable memory to the devices, and
>>> programming this memory looks to be what this series is about.
>>>
>>> Only the battery design info is programmed in this series, but much more
>>> battery info can be uploaded, including stuff that is not fixed and
>>> therefor cannot be added to the DT. Even the design parameters may not
>>> be fix if one changes the battery as different manufactures of the same
>>> battery type may have different design capacities.
>>>
>>> The ideal use-case in a system with a system side gauge is the OS should
>>> periodically, or just before shutdown, read the battery's learned info
>>> and when the device is powered back up, if the battery serial number
>>> matches from before, re-upload this learned info to the gauge so it
>>> doesn't have to re-learn everything. All of this will need to happen in
>>> user-space, so what we really need is a user-space API for manipulating
>>> these properties.
>>
>> We would need a feature in the driver to get/set the blob of pack- or
>> cell-unique data. The userspace api is one sysfs file.
>>
>> However the DT fixed-battery node could indicate when/how-often the
>> object is stored, and where, e.g. filename. If the latter is given,
>> the driver should write/read the file itself.
>>
>> Is there some documentation about what registers should go in this blob?
>>
>
> This should all be available in the parts' technical reference (looks
> like page 24 for your device), but the blob isn't meant to be modified,
> the information/format is mostly internal to the part, all we should do
> is save blob / restore blob as needed.

Page 24 has "8.5.5 Data Block Summary" That?

Subclass IDs for NVM are 82, 88,104, 105. Do all those go in the blob?


>> That concept is beyond the scope of this patchset (and doesn't
>> supersede it), but is something I'd like to implement for our chip,
>> the *425.
>>
>>
>>> The properties added in this series are okay, and it's good to have some
>>> sane defaults in DT, but looking forward this may not be enough to get
>>> full use out of these devices.
>>
>> I'm relieved you're not opposed to our patch :-) Now I just need that
>> guidance I mentioned re the chip family...
>>
>
> Still looking into it, I think for now would be easiest to add the
> register info to each chip family struct. The un-seal sequence should be
> generic enough already.

I'll wait for your full reply on that thread.


>>>> 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   | 37 ++++++++++++++++++++++
>>>>  1 file changed, 37 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..d78a4aa
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/power/supply/battery.txt
>>>> @@ -0,0 +1,37 @@
>>>> +Battery Characteristics
>>>> +
>>>> +Required Properties:
>>>> + - compatible: Must be "fixed-battery"
>>>> +
>>>> +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
>>>> +
>>>> +Future Properties must be named for the corresponding elements in
>>>> +enum power_supply_property, defined in include/linux/power_supply.h.
>>>> +
>>>> +Batteries must be referenced by chargers and/or fuel-gauges
>>>> +using a phandle. The phandle's property should be named
>>>> +"monitored-battery".
>>>> +
>>>> +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>;
>>>> +             ...
>>>> +     };
>>>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe devicetree" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>

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

[Andrew F. Davis]

On 02/13/2017 03:31 PM, Liam Breck wrote:
> [Dropped devicetree from CC]
> 
> On Mon, Feb 13, 2017 at 1:12 PM, Andrew F. Davis <afd@ti.com> wrote:
>> On 02/13/2017 02:58 PM, Liam Breck wrote:
>>> Hi Andrew, thanks for the drill-down on BQ27* chips.
>>>
>>> On Mon, Feb 13, 2017 at 8:06 AM, Andrew F. Davis <afd@ti.com> wrote:
>>>> On 02/10/2017 08:43 PM, Liam Breck wrote:
>>>>> From: Matt Ranostay <matt@ranostay.consulting>
>>>>>
>>>>> 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.
>>>>
>>>>
>>>> I think some of the confusion about what a "smart-battery" binding would
>>>> look like is due to a miss-understanding what it means to
>>>> "self-identify". Originally the bq27xxx *was* the chip that lived its
>>>> whole life inside a battery (the iphone battery is one such example and
>>>> a good way to test the bq27000). This way it could be factory programmed
>>>> with the battery's basics and then learn about the individual pack over
>>>> time to provide better info on time to empty, discharge rates, etc..
>>>>
>>>> Eventually, people wanted to have the battery become cheap and
>>>> replaceable, so fuel-gauges were made that are "system side", and ride
>>>> along with the device, not the battery. Now this has one big problem,
>>>> how can we learn about a battery when it may be a new battery every time
>>>> we boot up? So we added programmable memory to the devices, and
>>>> programming this memory looks to be what this series is about.
>>>>
>>>> Only the battery design info is programmed in this series, but much more
>>>> battery info can be uploaded, including stuff that is not fixed and
>>>> therefor cannot be added to the DT. Even the design parameters may not
>>>> be fix if one changes the battery as different manufactures of the same
>>>> battery type may have different design capacities.
>>>>
>>>> The ideal use-case in a system with a system side gauge is the OS should
>>>> periodically, or just before shutdown, read the battery's learned info
>>>> and when the device is powered back up, if the battery serial number
>>>> matches from before, re-upload this learned info to the gauge so it
>>>> doesn't have to re-learn everything. All of this will need to happen in
>>>> user-space, so what we really need is a user-space API for manipulating
>>>> these properties.
>>>
>>> We would need a feature in the driver to get/set the blob of pack- or
>>> cell-unique data. The userspace api is one sysfs file.
>>>
>>> However the DT fixed-battery node could indicate when/how-often the
>>> object is stored, and where, e.g. filename. If the latter is given,
>>> the driver should write/read the file itself.
>>>
>>> Is there some documentation about what registers should go in this blob?
>>>
>>
>> This should all be available in the parts' technical reference (looks
>> like page 24 for your device), but the blob isn't meant to be modified,
>> the information/format is mostly internal to the part, all we should do
>> is save blob / restore blob as needed.
> 
> Page 24 has "8.5.5 Data Block Summary" That?
> 
> Subclass IDs for NVM are 82, 88,104, 105. Do all those go in the blob?
> 

Some devices (bq27421) do not contain an NVM, and so rely on the host to
restore its "learned data" across power cycles with the same battery.

When using devices with NVM this "learned data" can be saved to the NVM
and it will automatically be copied over to the run-time RAM sections on
powerup.

So we should only ever need to save the RAM section.

Another thing to consider is that we (TI) have GUI tools to allow
manufacturers to input battery info and have it generate a blob that can
be deployed to these devices. The blob format is meant to be handled and
uploaded by custom user-space tools as it only really needs to be done
once (power-down events should only happen when changing batteries, in
which case you want to let the device forget what it has learned about
the old battery).

All of the details of this programming are probably not the job of the
kernel, in end-user hands the kernel only needs to know how to read from
the device, not program it.

Take everything I say with a grain of salt though, I'm not on the team
that makes these. I don't have much additional info not in the public
data-sheets.

Andrew

> 
>>> That concept is beyond the scope of this patchset (and doesn't
>>> supersede it), but is something I'd like to implement for our chip,
>>> the *425.
>>>
>>>
>>>> The properties added in this series are okay, and it's good to have some
>>>> sane defaults in DT, but looking forward this may not be enough to get
>>>> full use out of these devices.
>>>
>>> I'm relieved you're not opposed to our patch :-) Now I just need that
>>> guidance I mentioned re the chip family...
>>>
>>
>> Still looking into it, I think for now would be easiest to add the
>> register info to each chip family struct. The un-seal sequence should be
>> generic enough already.
> 
> I'll wait for your full reply on that thread.
> 
> 
>>>>> 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   | 37 ++++++++++++++++++++++
>>>>>  1 file changed, 37 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..d78a4aa
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/power/supply/battery.txt
>>>>> @@ -0,0 +1,37 @@
>>>>> +Battery Characteristics
>>>>> +
>>>>> +Required Properties:
>>>>> + - compatible: Must be "fixed-battery"
>>>>> +
>>>>> +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
>>>>> +
>>>>> +Future Properties must be named for the corresponding elements in
>>>>> +enum power_supply_property, defined in include/linux/power_supply.h.
>>>>> +
>>>>> +Batteries must be referenced by chargers and/or fuel-gauges
>>>>> +using a phandle. The phandle's property should be named
>>>>> +"monitored-battery".
>>>>> +
>>>>> +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>;
>>>>> +             ...
>>>>> +     };
>>>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe devicetree" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>

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

[Liam Breck]

On Tue, Feb 14, 2017 at 12:22 PM, Andrew F. Davis <afd@ti.com> wrote:
> On 02/13/2017 03:31 PM, Liam Breck wrote:
>> [Dropped devicetree from CC]
>>
>> On Mon, Feb 13, 2017 at 1:12 PM, Andrew F. Davis <afd@ti.com> wrote:
>>> On 02/13/2017 02:58 PM, Liam Breck wrote:
>>>> Hi Andrew, thanks for the drill-down on BQ27* chips.
>>>>
>>>> On Mon, Feb 13, 2017 at 8:06 AM, Andrew F. Davis <afd@ti.com> wrote:
>>>>> On 02/10/2017 08:43 PM, Liam Breck wrote:
>>>>>> From: Matt Ranostay <matt@ranostay.consulting>
>>>>>>
>>>>>> 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.
>>>>>
>>>>>
>>>>> I think some of the confusion about what a "smart-battery" binding would
>>>>> look like is due to a miss-understanding what it means to
>>>>> "self-identify". Originally the bq27xxx *was* the chip that lived its
>>>>> whole life inside a battery (the iphone battery is one such example and
>>>>> a good way to test the bq27000). This way it could be factory programmed
>>>>> with the battery's basics and then learn about the individual pack over
>>>>> time to provide better info on time to empty, discharge rates, etc..
>>>>>
>>>>> Eventually, people wanted to have the battery become cheap and
>>>>> replaceable, so fuel-gauges were made that are "system side", and ride
>>>>> along with the device, not the battery. Now this has one big problem,
>>>>> how can we learn about a battery when it may be a new battery every time
>>>>> we boot up? So we added programmable memory to the devices, and
>>>>> programming this memory looks to be what this series is about.
>>>>>
>>>>> Only the battery design info is programmed in this series, but much more
>>>>> battery info can be uploaded, including stuff that is not fixed and
>>>>> therefor cannot be added to the DT. Even the design parameters may not
>>>>> be fix if one changes the battery as different manufactures of the same
>>>>> battery type may have different design capacities.
>>>>>
>>>>> The ideal use-case in a system with a system side gauge is the OS should
>>>>> periodically, or just before shutdown, read the battery's learned info
>>>>> and when the device is powered back up, if the battery serial number
>>>>> matches from before, re-upload this learned info to the gauge so it
>>>>> doesn't have to re-learn everything. All of this will need to happen in
>>>>> user-space, so what we really need is a user-space API for manipulating
>>>>> these properties.
>>>>
>>>> We would need a feature in the driver to get/set the blob of pack- or
>>>> cell-unique data. The userspace api is one sysfs file.
>>>>
>>>> However the DT fixed-battery node could indicate when/how-often the
>>>> object is stored, and where, e.g. filename. If the latter is given,
>>>> the driver should write/read the file itself.
>>>>
>>>> Is there some documentation about what registers should go in this blob?
>>>>
>>>
>>> This should all be available in the parts' technical reference (looks
>>> like page 24 for your device), but the blob isn't meant to be modified,
>>> the information/format is mostly internal to the part, all we should do
>>> is save blob / restore blob as needed.
>>
>> Page 24 has "8.5.5 Data Block Summary" That?
>>
>> Subclass IDs for NVM are 82, 88,104, 105. Do all those go in the blob?
>>
>
> Some devices (bq27421) do not contain an NVM, and so rely on the host to
> restore its "learned data" across power cycles with the same battery.

Ah, so my concept for DT battery:nvm-storage: "filename" would apply
to these RAM-only chips.

> When using devices with NVM this "learned data" can be saved to the NVM
> and it will automatically be copied over to the run-time RAM sections on
> powerup. So we should only ever need to save the RAM section.

For the NVM chips, we'd want a userspace-accessible reset_nvm function
in driver which returns the pack-unique data to factory defaults. That
can be provided via a sysfs file to which you write a key (safer than
"echo 1 > /sys/class.../reset_nvm") to trigger the reset.

> Another thing to consider is that we (TI) have GUI tools to allow
> manufacturers to input battery info and have it generate a blob that can
> be deployed to these devices. The blob format is meant to be handled and
> uploaded by custom user-space tools as it only really needs to be done
> once (power-down events should only happen when changing batteries, in
> which case you want to let the device forget what it has learned about
> the old battery).
>
> All of the details of this programming are probably not the job of the
> kernel, in end-user hands the kernel only needs to know how to read from
> the device, not program it.

Were there an independently maintained battery manager app with wide
support for battery ICs, it would be a natural venue for BQ27*
reset/save/restore_nvm functions. But there is none that I know of,
and these features can be implemented in the driver with little new
code beyond the read/write_dm_block functions added by this patchset.

It's fine to ask hardware makers to rely on IC vendor tools in the
factory, but I don't think such tools belong in the end-user's stack.

> Take everything I say with a grain of salt though, I'm not on the team
> that makes these. I don't have much additional info not in the public
> data-sheets.
>
> Andrew
>
>>
>>>> That concept is beyond the scope of this patchset (and doesn't
>>>> supersede it), but is something I'd like to implement for our chip,
>>>> the *425.
>>>>
>>>>
>>>>> The properties added in this series are okay, and it's good to have some
>>>>> sane defaults in DT, but looking forward this may not be enough to get
>>>>> full use out of these devices.
>>>>
>>>> I'm relieved you're not opposed to our patch :-) Now I just need that
>>>> guidance I mentioned re the chip family...
>>>>
>>>
>>> Still looking into it, I think for now would be easiest to add the
>>> register info to each chip family struct. The un-seal sequence should be
>>> generic enough already.
>>
>> I'll wait for your full reply on that thread.
>>
>>
>>>>>> 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   | 37 ++++++++++++++++++++++
>>>>>>  1 file changed, 37 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..d78a4aa
>>>>>> --- /dev/null
>>>>>> +++ b/Documentation/devicetree/bindings/power/supply/battery.txt
>>>>>> @@ -0,0 +1,37 @@
>>>>>> +Battery Characteristics
>>>>>> +
>>>>>> +Required Properties:
>>>>>> + - compatible: Must be "fixed-battery"
>>>>>> +
>>>>>> +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
>>>>>> +
>>>>>> +Future Properties must be named for the corresponding elements in
>>>>>> +enum power_supply_property, defined in include/linux/power_supply.h.
>>>>>> +
>>>>>> +Batteries must be referenced by chargers and/or fuel-gauges
>>>>>> +using a phandle. The phandle's property should be named
>>>>>> +"monitored-battery".
>>>>>> +
>>>>>> +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>;
>>>>>> +             ...
>>>>>> +     };
>>>>>>
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe devicetree" in
>>>> the body of a message to majordomo@vger.kernel.org
>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>>

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

[Andrew F. Davis]

On 02/14/2017 04:38 PM, Liam Breck wrote:
> On Tue, Feb 14, 2017 at 12:22 PM, Andrew F. Davis <afd@ti.com> wrote:
>> On 02/13/2017 03:31 PM, Liam Breck wrote:
>>> [Dropped devicetree from CC]
>>>
>>> On Mon, Feb 13, 2017 at 1:12 PM, Andrew F. Davis <afd@ti.com> wrote:
>>>> On 02/13/2017 02:58 PM, Liam Breck wrote:
>>>>> Hi Andrew, thanks for the drill-down on BQ27* chips.
>>>>>
>>>>> On Mon, Feb 13, 2017 at 8:06 AM, Andrew F. Davis <afd@ti.com> wrote:
>>>>>> On 02/10/2017 08:43 PM, Liam Breck wrote:
>>>>>>> From: Matt Ranostay <matt@ranostay.consulting>
>>>>>>>
>>>>>>> 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.
>>>>>>
>>>>>>
>>>>>> I think some of the confusion about what a "smart-battery" binding would
>>>>>> look like is due to a miss-understanding what it means to
>>>>>> "self-identify". Originally the bq27xxx *was* the chip that lived its
>>>>>> whole life inside a battery (the iphone battery is one such example and
>>>>>> a good way to test the bq27000). This way it could be factory programmed
>>>>>> with the battery's basics and then learn about the individual pack over
>>>>>> time to provide better info on time to empty, discharge rates, etc..
>>>>>>
>>>>>> Eventually, people wanted to have the battery become cheap and
>>>>>> replaceable, so fuel-gauges were made that are "system side", and ride
>>>>>> along with the device, not the battery. Now this has one big problem,
>>>>>> how can we learn about a battery when it may be a new battery every time
>>>>>> we boot up? So we added programmable memory to the devices, and
>>>>>> programming this memory looks to be what this series is about.
>>>>>>
>>>>>> Only the battery design info is programmed in this series, but much more
>>>>>> battery info can be uploaded, including stuff that is not fixed and
>>>>>> therefor cannot be added to the DT. Even the design parameters may not
>>>>>> be fix if one changes the battery as different manufactures of the same
>>>>>> battery type may have different design capacities.
>>>>>>
>>>>>> The ideal use-case in a system with a system side gauge is the OS should
>>>>>> periodically, or just before shutdown, read the battery's learned info
>>>>>> and when the device is powered back up, if the battery serial number
>>>>>> matches from before, re-upload this learned info to the gauge so it
>>>>>> doesn't have to re-learn everything. All of this will need to happen in
>>>>>> user-space, so what we really need is a user-space API for manipulating
>>>>>> these properties.
>>>>>
>>>>> We would need a feature in the driver to get/set the blob of pack- or
>>>>> cell-unique data. The userspace api is one sysfs file.
>>>>>
>>>>> However the DT fixed-battery node could indicate when/how-often the
>>>>> object is stored, and where, e.g. filename. If the latter is given,
>>>>> the driver should write/read the file itself.
>>>>>
>>>>> Is there some documentation about what registers should go in this blob?
>>>>>
>>>>
>>>> This should all be available in the parts' technical reference (looks
>>>> like page 24 for your device), but the blob isn't meant to be modified,
>>>> the information/format is mostly internal to the part, all we should do
>>>> is save blob / restore blob as needed.
>>>
>>> Page 24 has "8.5.5 Data Block Summary" That?
>>>
>>> Subclass IDs for NVM are 82, 88,104, 105. Do all those go in the blob?
>>>
>>
>> Some devices (bq27421) do not contain an NVM, and so rely on the host to
>> restore its "learned data" across power cycles with the same battery.
> 
> Ah, so my concept for DT battery:nvm-storage: "filename" would apply
> to these RAM-only chips.
> 

I think so, I was going to do something similar, but let userspace take
care of the filename, or have it a fixed/module param like firmware. DT
is not really the right spot for filenames.

If you can find a kernel-friendly way to do this then I'm all for it!

>> When using devices with NVM this "learned data" can be saved to the NVM
>> and it will automatically be copied over to the run-time RAM sections on
>> powerup. So we should only ever need to save the RAM section.
> 
> For the NVM chips, we'd want a userspace-accessible reset_nvm function
> in driver which returns the pack-unique data to factory defaults. That
> can be provided via a sysfs file to which you write a key (safer than
> "echo 1 > /sys/class.../reset_nvm") to trigger the reset.
> 

Still not sure why one would need this, the only time you would want to
reset the gauge is when changing the battery, and that will already
cause it to power cycle (unless you have some kind of back-up battery,
but that doesn't make sense to me).

>> Another thing to consider is that we (TI) have GUI tools to allow
>> manufacturers to input battery info and have it generate a blob that can
>> be deployed to these devices. The blob format is meant to be handled and
>> uploaded by custom user-space tools as it only really needs to be done
>> once (power-down events should only happen when changing batteries, in
>> which case you want to let the device forget what it has learned about
>> the old battery).
>>
>> All of the details of this programming are probably not the job of the
>> kernel, in end-user hands the kernel only needs to know how to read from
>> the device, not program it.
> 
> Were there an independently maintained battery manager app with wide
> support for battery ICs, it would be a natural venue for BQ27*
> reset/save/restore_nvm functions. But there is none that I know of,
> and these features can be implemented in the driver with little new
> code beyond the read/write_dm_block functions added by this patchset.
> 
> It's fine to ask hardware makers to rely on IC vendor tools in the
> factory, but I don't think such tools belong in the end-user's stack.
> 

If they don't belong in the end-user's user-space tool-set then they
definitely don't belong in kernel-space. The other option is to define a
whole new API for interfacing with all these device specific parameters
and internal info tables. It's that or we accept that these are factory
settings and we don't need all too much kernel infrastructure for
handling them.

Andrew

>> Take everything I say with a grain of salt though, I'm not on the team
>> that makes these. I don't have much additional info not in the public
>> data-sheets.
>>
>> Andrew
>>
>>>
>>>>> That concept is beyond the scope of this patchset (and doesn't
>>>>> supersede it), but is something I'd like to implement for our chip,
>>>>> the *425.
>>>>>
>>>>>
>>>>>> The properties added in this series are okay, and it's good to have some
>>>>>> sane defaults in DT, but looking forward this may not be enough to get
>>>>>> full use out of these devices.
>>>>>
>>>>> I'm relieved you're not opposed to our patch :-) Now I just need that
>>>>> guidance I mentioned re the chip family...
>>>>>
>>>>
>>>> Still looking into it, I think for now would be easiest to add the
>>>> register info to each chip family struct. The un-seal sequence should be
>>>> generic enough already.
>>>
>>> I'll wait for your full reply on that thread.
>>>
>>>
>>>>>>> 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   | 37 ++++++++++++++++++++++
>>>>>>>  1 file changed, 37 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..d78a4aa
>>>>>>> --- /dev/null
>>>>>>> +++ b/Documentation/devicetree/bindings/power/supply/battery.txt
>>>>>>> @@ -0,0 +1,37 @@
>>>>>>> +Battery Characteristics
>>>>>>> +
>>>>>>> +Required Properties:
>>>>>>> + - compatible: Must be "fixed-battery"
>>>>>>> +
>>>>>>> +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
>>>>>>> +
>>>>>>> +Future Properties must be named for the corresponding elements in
>>>>>>> +enum power_supply_property, defined in include/linux/power_supply.h.
>>>>>>> +
>>>>>>> +Batteries must be referenced by chargers and/or fuel-gauges
>>>>>>> +using a phandle. The phandle's property should be named
>>>>>>> +"monitored-battery".
>>>>>>> +
>>>>>>> +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>;
>>>>>>> +             ...
>>>>>>> +     };
>>>>>>>
>>>>> --
>>>>> To unsubscribe from this list: send the line "unsubscribe devicetree" in
>>>>> the body of a message to majordomo@vger.kernel.org
>>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>>>

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

[Liam Breck]

On Wed, Feb 15, 2017 at 7:54 AM, Andrew F. Davis <afd@ti.com> wrote:
> On 02/14/2017 04:38 PM, Liam Breck wrote:
>> On Tue, Feb 14, 2017 at 12:22 PM, Andrew F. Davis <afd@ti.com> wrote:
>>> On 02/13/2017 03:31 PM, Liam Breck wrote:
>>>> [Dropped devicetree from CC]
>>>>
>>>> On Mon, Feb 13, 2017 at 1:12 PM, Andrew F. Davis <afd@ti.com> wrote:
>>>>> On 02/13/2017 02:58 PM, Liam Breck wrote:
>>>>>> Hi Andrew, thanks for the drill-down on BQ27* chips.
>>>>>>
>>>>>> On Mon, Feb 13, 2017 at 8:06 AM, Andrew F. Davis <afd@ti.com> wrote:
>>>>>>> On 02/10/2017 08:43 PM, Liam Breck wrote:
>>>>>>>> From: Matt Ranostay <matt@ranostay.consulting>
>>>>>>>>
>>>>>>>> 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.
>>>>>>>
>>>>>>>
>>>>>>> I think some of the confusion about what a "smart-battery" binding would
>>>>>>> look like is due to a miss-understanding what it means to
>>>>>>> "self-identify". Originally the bq27xxx *was* the chip that lived its
>>>>>>> whole life inside a battery (the iphone battery is one such example and
>>>>>>> a good way to test the bq27000). This way it could be factory programmed
>>>>>>> with the battery's basics and then learn about the individual pack over
>>>>>>> time to provide better info on time to empty, discharge rates, etc..
>>>>>>>
>>>>>>> Eventually, people wanted to have the battery become cheap and
>>>>>>> replaceable, so fuel-gauges were made that are "system side", and ride
>>>>>>> along with the device, not the battery. Now this has one big problem,
>>>>>>> how can we learn about a battery when it may be a new battery every time
>>>>>>> we boot up? So we added programmable memory to the devices, and
>>>>>>> programming this memory looks to be what this series is about.
>>>>>>>
>>>>>>> Only the battery design info is programmed in this series, but much more
>>>>>>> battery info can be uploaded, including stuff that is not fixed and
>>>>>>> therefor cannot be added to the DT. Even the design parameters may not
>>>>>>> be fix if one changes the battery as different manufactures of the same
>>>>>>> battery type may have different design capacities.
>>>>>>>
>>>>>>> The ideal use-case in a system with a system side gauge is the OS should
>>>>>>> periodically, or just before shutdown, read the battery's learned info
>>>>>>> and when the device is powered back up, if the battery serial number
>>>>>>> matches from before, re-upload this learned info to the gauge so it
>>>>>>> doesn't have to re-learn everything. All of this will need to happen in
>>>>>>> user-space, so what we really need is a user-space API for manipulating
>>>>>>> these properties.
>>>>>>
>>>>>> We would need a feature in the driver to get/set the blob of pack- or
>>>>>> cell-unique data. The userspace api is one sysfs file.
>>>>>>
>>>>>> However the DT fixed-battery node could indicate when/how-often the
>>>>>> object is stored, and where, e.g. filename. If the latter is given,
>>>>>> the driver should write/read the file itself.
>>>>>>
>>>>>> Is there some documentation about what registers should go in this blob?
>>>>>>
>>>>>
>>>>> This should all be available in the parts' technical reference (looks
>>>>> like page 24 for your device), but the blob isn't meant to be modified,
>>>>> the information/format is mostly internal to the part, all we should do
>>>>> is save blob / restore blob as needed.
>>>>
>>>> Page 24 has "8.5.5 Data Block Summary" That?
>>>>
>>>> Subclass IDs for NVM are 82, 88,104, 105. Do all those go in the blob?
>>>>
>>>
>>> Some devices (bq27421) do not contain an NVM, and so rely on the host to
>>> restore its "learned data" across power cycles with the same battery.
>>
>> Ah, so my concept for DT battery:nvm-storage: "filename" would apply
>> to these RAM-only chips.
>>
>
> I think so, I was going to do something similar, but let userspace take
> care of the filename, or have it a fixed/module param like firmware. DT
> is not really the right spot for filenames.
>
> If you can find a kernel-friendly way to do this then I'm all for it!

Hm, you have a point about filenames in DT. Maybe an inode number? (kidding :-)

We could hand the driver a filename via sysfs file or module param.
Perhaps use a filename in DT as a fallback.


>>> When using devices with NVM this "learned data" can be saved to the NVM
>>> and it will automatically be copied over to the run-time RAM sections on
>>> powerup. So we should only ever need to save the RAM section.
>>
>> For the NVM chips, we'd want a userspace-accessible reset_nvm function
>> in driver which returns the pack-unique data to factory defaults. That
>> can be provided via a sysfs file to which you write a key (safer than
>> "echo 1 > /sys/class.../reset_nvm") to trigger the reset.
>>
>
> Still not sure why one would need this, the only time you would want to
> reset the gauge is when changing the battery, and that will already
> cause it to power cycle (unless you have some kind of back-up battery,
> but that doesn't make sense to me).

This is for gauges with NVM, where power-cycle does not clear the
pack-specific stats. Or do those gauges recognize that the pack has
changed?


>>> Another thing to consider is that we (TI) have GUI tools to allow
>>> manufacturers to input battery info and have it generate a blob that can
>>> be deployed to these devices. The blob format is meant to be handled and
>>> uploaded by custom user-space tools as it only really needs to be done
>>> once (power-down events should only happen when changing batteries, in
>>> which case you want to let the device forget what it has learned about
>>> the old battery).
>>>
>>> All of the details of this programming are probably not the job of the
>>> kernel, in end-user hands the kernel only needs to know how to read from
>>> the device, not program it.
>>
>> Were there an independently maintained battery manager app with wide
>> support for battery ICs, it would be a natural venue for BQ27*
>> reset/save/restore_nvm functions. But there is none that I know of,
>> and these features can be implemented in the driver with little new
>> code beyond the read/write_dm_block functions added by this patchset.
>>
>> It's fine to ask hardware makers to rely on IC vendor tools in the
>> factory, but I don't think such tools belong in the end-user's stack.
>>
>
> If they don't belong in the end-user's user-space tool-set then they
> definitely don't belong in kernel-space. The other option is to define a
> whole new API for interfacing with all these device specific parameters
> and internal info tables. It's that or we accept that these are factory
> settings and we don't need all too much kernel infrastructure for
> handling them.

My point is that an IC vendor *codebase* does not belong in the
end-user's stack; it's a production tool. The functionality we're
discussing clearly belongs in both the device driver and the
production tool.


>>> Take everything I say with a grain of salt though, I'm not on the team
>>> that makes these. I don't have much additional info not in the public
>>> data-sheets.
>>>
>>> Andrew
>>>
>>>>
>>>>>> That concept is beyond the scope of this patchset (and doesn't
>>>>>> supersede it), but is something I'd like to implement for our chip,
>>>>>> the *425.
>>>>>>
>>>>>>
>>>>>>> The properties added in this series are okay, and it's good to have some
>>>>>>> sane defaults in DT, but looking forward this may not be enough to get
>>>>>>> full use out of these devices.
>>>>>>
>>>>>> I'm relieved you're not opposed to our patch :-) Now I just need that
>>>>>> guidance I mentioned re the chip family...
>>>>>>
>>>>>
>>>>> Still looking into it, I think for now would be easiest to add the
>>>>> register info to each chip family struct. The un-seal sequence should be
>>>>> generic enough already.
>>>>
>>>> I'll wait for your full reply on that thread.
>>>>
>>>>
>>>>>>>> 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   | 37 ++++++++++++++++++++++
>>>>>>>>  1 file changed, 37 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..d78a4aa
>>>>>>>> --- /dev/null
>>>>>>>> +++ b/Documentation/devicetree/bindings/power/supply/battery.txt
>>>>>>>> @@ -0,0 +1,37 @@
>>>>>>>> +Battery Characteristics
>>>>>>>> +
>>>>>>>> +Required Properties:
>>>>>>>> + - compatible: Must be "fixed-battery"
>>>>>>>> +
>>>>>>>> +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
>>>>>>>> +
>>>>>>>> +Future Properties must be named for the corresponding elements in
>>>>>>>> +enum power_supply_property, defined in include/linux/power_supply.h.
>>>>>>>> +
>>>>>>>> +Batteries must be referenced by chargers and/or fuel-gauges
>>>>>>>> +using a phandle. The phandle's property should be named
>>>>>>>> +"monitored-battery".
>>>>>>>> +
>>>>>>>> +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>;
>>>>>>>> +             ...
>>>>>>>> +     };
>>>>>>>>
>>>>>> --
>>>>>> To unsubscribe from this list: send the line "unsubscribe devicetree" in
>>>>>> the body of a message to majordomo@vger.kernel.org
>>>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>>>>

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

[Andrew F. Davis]

On 02/15/2017 02:05 PM, Liam Breck wrote:
> On Wed, Feb 15, 2017 at 7:54 AM, Andrew F. Davis <afd@ti.com> wrote:
>> On 02/14/2017 04:38 PM, Liam Breck wrote:
>>> On Tue, Feb 14, 2017 at 12:22 PM, Andrew F. Davis <afd@ti.com> wrote:
>>>> On 02/13/2017 03:31 PM, Liam Breck wrote:
>>>>> [Dropped devicetree from CC]
>>>>>
>>>>> On Mon, Feb 13, 2017 at 1:12 PM, Andrew F. Davis <afd@ti.com> wrote:
>>>>>> On 02/13/2017 02:58 PM, Liam Breck wrote:
>>>>>>> Hi Andrew, thanks for the drill-down on BQ27* chips.
>>>>>>>
>>>>>>> On Mon, Feb 13, 2017 at 8:06 AM, Andrew F. Davis <afd@ti.com> wrote:
>>>>>>>> On 02/10/2017 08:43 PM, Liam Breck wrote:
>>>>>>>>> From: Matt Ranostay <matt@ranostay.consulting>
>>>>>>>>>
>>>>>>>>> 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.
>>>>>>>>
>>>>>>>>
>>>>>>>> I think some of the confusion about what a "smart-battery" binding would
>>>>>>>> look like is due to a miss-understanding what it means to
>>>>>>>> "self-identify". Originally the bq27xxx *was* the chip that lived its
>>>>>>>> whole life inside a battery (the iphone battery is one such example and
>>>>>>>> a good way to test the bq27000). This way it could be factory programmed
>>>>>>>> with the battery's basics and then learn about the individual pack over
>>>>>>>> time to provide better info on time to empty, discharge rates, etc..
>>>>>>>>
>>>>>>>> Eventually, people wanted to have the battery become cheap and
>>>>>>>> replaceable, so fuel-gauges were made that are "system side", and ride
>>>>>>>> along with the device, not the battery. Now this has one big problem,
>>>>>>>> how can we learn about a battery when it may be a new battery every time
>>>>>>>> we boot up? So we added programmable memory to the devices, and
>>>>>>>> programming this memory looks to be what this series is about.
>>>>>>>>
>>>>>>>> Only the battery design info is programmed in this series, but much more
>>>>>>>> battery info can be uploaded, including stuff that is not fixed and
>>>>>>>> therefor cannot be added to the DT. Even the design parameters may not
>>>>>>>> be fix if one changes the battery as different manufactures of the same
>>>>>>>> battery type may have different design capacities.
>>>>>>>>
>>>>>>>> The ideal use-case in a system with a system side gauge is the OS should
>>>>>>>> periodically, or just before shutdown, read the battery's learned info
>>>>>>>> and when the device is powered back up, if the battery serial number
>>>>>>>> matches from before, re-upload this learned info to the gauge so it
>>>>>>>> doesn't have to re-learn everything. All of this will need to happen in
>>>>>>>> user-space, so what we really need is a user-space API for manipulating
>>>>>>>> these properties.
>>>>>>>
>>>>>>> We would need a feature in the driver to get/set the blob of pack- or
>>>>>>> cell-unique data. The userspace api is one sysfs file.
>>>>>>>
>>>>>>> However the DT fixed-battery node could indicate when/how-often the
>>>>>>> object is stored, and where, e.g. filename. If the latter is given,
>>>>>>> the driver should write/read the file itself.
>>>>>>>
>>>>>>> Is there some documentation about what registers should go in this blob?
>>>>>>>
>>>>>>
>>>>>> This should all be available in the parts' technical reference (looks
>>>>>> like page 24 for your device), but the blob isn't meant to be modified,
>>>>>> the information/format is mostly internal to the part, all we should do
>>>>>> is save blob / restore blob as needed.
>>>>>
>>>>> Page 24 has "8.5.5 Data Block Summary" That?
>>>>>
>>>>> Subclass IDs for NVM are 82, 88,104, 105. Do all those go in the blob?
>>>>>
>>>>
>>>> Some devices (bq27421) do not contain an NVM, and so rely on the host to
>>>> restore its "learned data" across power cycles with the same battery.
>>>
>>> Ah, so my concept for DT battery:nvm-storage: "filename" would apply
>>> to these RAM-only chips.
>>>
>>
>> I think so, I was going to do something similar, but let userspace take
>> care of the filename, or have it a fixed/module param like firmware. DT
>> is not really the right spot for filenames.
>>
>> If you can find a kernel-friendly way to do this then I'm all for it!
> 
> Hm, you have a point about filenames in DT. Maybe an inode number? (kidding :-)
> 

:)

> We could hand the driver a filename via sysfs file or module param.
> Perhaps use a filename in DT as a fallback.
> 

If you can convince Rob, then I'd say go for it.

> 
>>>> When using devices with NVM this "learned data" can be saved to the NVM
>>>> and it will automatically be copied over to the run-time RAM sections on
>>>> powerup. So we should only ever need to save the RAM section.
>>>
>>> For the NVM chips, we'd want a userspace-accessible reset_nvm function
>>> in driver which returns the pack-unique data to factory defaults. That
>>> can be provided via a sysfs file to which you write a key (safer than
>>> "echo 1 > /sys/class.../reset_nvm") to trigger the reset.
>>>
>>
>> Still not sure why one would need this, the only time you would want to
>> reset the gauge is when changing the battery, and that will already
>> cause it to power cycle (unless you have some kind of back-up battery,
>> but that doesn't make sense to me).
> 
> This is for gauges with NVM, where power-cycle does not clear the
> pack-specific stats. Or do those gauges recognize that the pack has
> changed?
> 

The NVM *is* the factory default, RAM tracks the  pack-specific stats,
when the device power-cycles NVM values are automatically moved over
into working RAM, so that is always cleared across all devices. There is
really no good way to recognize that a pack has changed, so simply
forgetting the pack-specific stats and re-loading the defaults is the
safest option after a power-cycle. If we want to keep this info in RAM
and re-use it, then it is up to the manufacturer to implement a solution
for this detection (as only they can know what kind of device they are
making and whether that detection makes sense or not for it).

> 
>>>> Another thing to consider is that we (TI) have GUI tools to allow
>>>> manufacturers to input battery info and have it generate a blob that can
>>>> be deployed to these devices. The blob format is meant to be handled and
>>>> uploaded by custom user-space tools as it only really needs to be done
>>>> once (power-down events should only happen when changing batteries, in
>>>> which case you want to let the device forget what it has learned about
>>>> the old battery).
>>>>
>>>> All of the details of this programming are probably not the job of the
>>>> kernel, in end-user hands the kernel only needs to know how to read from
>>>> the device, not program it.
>>>
>>> Were there an independently maintained battery manager app with wide
>>> support for battery ICs, it would be a natural venue for BQ27*
>>> reset/save/restore_nvm functions. But there is none that I know of,
>>> and these features can be implemented in the driver with little new
>>> code beyond the read/write_dm_block functions added by this patchset.
>>>
>>> It's fine to ask hardware makers to rely on IC vendor tools in the
>>> factory, but I don't think such tools belong in the end-user's stack.
>>>
>>
>> If they don't belong in the end-user's user-space tool-set then they
>> definitely don't belong in kernel-space. The other option is to define a
>> whole new API for interfacing with all these device specific parameters
>> and internal info tables. It's that or we accept that these are factory
>> settings and we don't need all too much kernel infrastructure for
>> handling them.
> 
> My point is that an IC vendor *codebase* does not belong in the
> end-user's stack; it's a production tool. The functionality we're
> discussing clearly belongs in both the device driver and the
> production tool.
> 

What functionality?, specifying your battery's design capacity, sure,
reprogramming the gauge's internal "Impedance Track™" algorithm data,
not so much. It's not up to me to draw the line in what goes into the
kernel driver vs user-space tools, at this point I think we can wait for
Sebastian to weigh in on this.

Andrew

> 
>>>> Take everything I say with a grain of salt though, I'm not on the team
>>>> that makes these. I don't have much additional info not in the public
>>>> data-sheets.
>>>>
>>>> Andrew
>>>>
>>>>>
>>>>>>> That concept is beyond the scope of this patchset (and doesn't
>>>>>>> supersede it), but is something I'd like to implement for our chip,
>>>>>>> the *425.
>>>>>>>
>>>>>>>
>>>>>>>> The properties added in this series are okay, and it's good to have some
>>>>>>>> sane defaults in DT, but looking forward this may not be enough to get
>>>>>>>> full use out of these devices.
>>>>>>>
>>>>>>> I'm relieved you're not opposed to our patch :-) Now I just need that
>>>>>>> guidance I mentioned re the chip family...
>>>>>>>
>>>>>>
>>>>>> Still looking into it, I think for now would be easiest to add the
>>>>>> register info to each chip family struct. The un-seal sequence should be
>>>>>> generic enough already.
>>>>>
>>>>> I'll wait for your full reply on that thread.
>>>>>
>>>>>
>>>>>>>>> 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   | 37 ++++++++++++++++++++++
>>>>>>>>>  1 file changed, 37 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..d78a4aa
>>>>>>>>> --- /dev/null
>>>>>>>>> +++ b/Documentation/devicetree/bindings/power/supply/battery.txt
>>>>>>>>> @@ -0,0 +1,37 @@
>>>>>>>>> +Battery Characteristics
>>>>>>>>> +
>>>>>>>>> +Required Properties:
>>>>>>>>> + - compatible: Must be "fixed-battery"
>>>>>>>>> +
>>>>>>>>> +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
>>>>>>>>> +
>>>>>>>>> +Future Properties must be named for the corresponding elements in
>>>>>>>>> +enum power_supply_property, defined in include/linux/power_supply.h.
>>>>>>>>> +
>>>>>>>>> +Batteries must be referenced by chargers and/or fuel-gauges
>>>>>>>>> +using a phandle. The phandle's property should be named
>>>>>>>>> +"monitored-battery".
>>>>>>>>> +
>>>>>>>>> +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>;
>>>>>>>>> +             ...
>>>>>>>>> +     };
>>>>>>>>>
>>>>>>> --
>>>>>>> To unsubscribe from this list: send the line "unsubscribe devicetree" in
>>>>>>> the body of a message to majordomo@vger.kernel.org
>>>>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>>>>>

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

[Liam Breck]

On Wed, Feb 15, 2017 at 3:21 PM, Andrew F. Davis <afd@ti.com> wrote:
> On 02/15/2017 02:05 PM, Liam Breck wrote:
>> On Wed, Feb 15, 2017 at 7:54 AM, Andrew F. Davis <afd@ti.com> wrote:
>>> On 02/14/2017 04:38 PM, Liam Breck wrote:
>>>> On Tue, Feb 14, 2017 at 12:22 PM, Andrew F. Davis <afd@ti.com> wrote:
>>>>> On 02/13/2017 03:31 PM, Liam Breck wrote:
>>>>>> [Dropped devicetree from CC]
>>>>>>
>>>>>> On Mon, Feb 13, 2017 at 1:12 PM, Andrew F. Davis <afd@ti.com> wrote:
>>>>>>> On 02/13/2017 02:58 PM, Liam Breck wrote:
>>>>>>>> Hi Andrew, thanks for the drill-down on BQ27* chips.
>>>>>>>>
>>>>>>>> On Mon, Feb 13, 2017 at 8:06 AM, Andrew F. Davis <afd@ti.com> wrote:
>>>>>>>>> On 02/10/2017 08:43 PM, Liam Breck wrote:
>>>>>>>>>> From: Matt Ranostay <matt@ranostay.consulting>
>>>>>>>>>>
>>>>>>>>>> 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.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> I think some of the confusion about what a "smart-battery" binding would
>>>>>>>>> look like is due to a miss-understanding what it means to
>>>>>>>>> "self-identify". Originally the bq27xxx *was* the chip that lived its
>>>>>>>>> whole life inside a battery (the iphone battery is one such example and
>>>>>>>>> a good way to test the bq27000). This way it could be factory programmed
>>>>>>>>> with the battery's basics and then learn about the individual pack over
>>>>>>>>> time to provide better info on time to empty, discharge rates, etc..
>>>>>>>>>
>>>>>>>>> Eventually, people wanted to have the battery become cheap and
>>>>>>>>> replaceable, so fuel-gauges were made that are "system side", and ride
>>>>>>>>> along with the device, not the battery. Now this has one big problem,
>>>>>>>>> how can we learn about a battery when it may be a new battery every time
>>>>>>>>> we boot up? So we added programmable memory to the devices, and
>>>>>>>>> programming this memory looks to be what this series is about.
>>>>>>>>>
>>>>>>>>> Only the battery design info is programmed in this series, but much more
>>>>>>>>> battery info can be uploaded, including stuff that is not fixed and
>>>>>>>>> therefor cannot be added to the DT. Even the design parameters may not
>>>>>>>>> be fix if one changes the battery as different manufactures of the same
>>>>>>>>> battery type may have different design capacities.
>>>>>>>>>
>>>>>>>>> The ideal use-case in a system with a system side gauge is the OS should
>>>>>>>>> periodically, or just before shutdown, read the battery's learned info
>>>>>>>>> and when the device is powered back up, if the battery serial number
>>>>>>>>> matches from before, re-upload this learned info to the gauge so it
>>>>>>>>> doesn't have to re-learn everything. All of this will need to happen in
>>>>>>>>> user-space, so what we really need is a user-space API for manipulating
>>>>>>>>> these properties.
>>>>>>>>
>>>>>>>> We would need a feature in the driver to get/set the blob of pack- or
>>>>>>>> cell-unique data. The userspace api is one sysfs file.
>>>>>>>>
>>>>>>>> However the DT fixed-battery node could indicate when/how-often the
>>>>>>>> object is stored, and where, e.g. filename. If the latter is given,
>>>>>>>> the driver should write/read the file itself.
>>>>>>>>
>>>>>>>> Is there some documentation about what registers should go in this blob?
>>>>>>>>
>>>>>>>
>>>>>>> This should all be available in the parts' technical reference (looks
>>>>>>> like page 24 for your device), but the blob isn't meant to be modified,
>>>>>>> the information/format is mostly internal to the part, all we should do
>>>>>>> is save blob / restore blob as needed.
>>>>>>
>>>>>> Page 24 has "8.5.5 Data Block Summary" That?
>>>>>>
>>>>>> Subclass IDs for NVM are 82, 88,104, 105. Do all those go in the blob?
>>>>>>
>>>>>
>>>>> Some devices (bq27421) do not contain an NVM, and so rely on the host to
>>>>> restore its "learned data" across power cycles with the same battery.

>From your above statement, I inferred that NVM retains pack-unique
stats on poweroff. Below you state that NVM only retains factory
defaults. Could you clarify?

>>>> Ah, so my concept for DT battery:nvm-storage: "filename" would apply
>>>> to these RAM-only chips.
>>>>
>>>
>>> I think so, I was going to do something similar, but let userspace take
>>> care of the filename, or have it a fixed/module param like firmware. DT
>>> is not really the right spot for filenames.
>>>
>>> If you can find a kernel-friendly way to do this then I'm all for it!
>>
>> Hm, you have a point about filenames in DT. Maybe an inode number? (kidding :-)
>>
>
> :)
>
>> We could hand the driver a filename via sysfs file or module param.
>> Perhaps use a filename in DT as a fallback.
>>
>
> If you can convince Rob, then I'd say go for it.

OK, I restate the proposal below.

>>>>> When using devices with NVM this "learned data" can be saved to the NVM
>>>>> and it will automatically be copied over to the run-time RAM sections on
>>>>> powerup. So we should only ever need to save the RAM section.
>>>>
>>>> For the NVM chips, we'd want a userspace-accessible reset_nvm function
>>>> in driver which returns the pack-unique data to factory defaults. That
>>>> can be provided via a sysfs file to which you write a key (safer than
>>>> "echo 1 > /sys/class.../reset_nvm") to trigger the reset.
>>>>
>>>
>>> Still not sure why one would need this, the only time you would want to
>>> reset the gauge is when changing the battery, and that will already
>>> cause it to power cycle (unless you have some kind of back-up battery,
>>> but that doesn't make sense to me).
>>
>> This is for gauges with NVM, where power-cycle does not clear the
>> pack-specific stats. Or do those gauges recognize that the pack has
>> changed?
>>
>
> The NVM *is* the factory default, RAM tracks the  pack-specific stats,
> when the device power-cycles NVM values are automatically moved over
> into working RAM, so that is always cleared across all devices. There is
> really no good way to recognize that a pack has changed, so simply
> forgetting the pack-specific stats and re-loading the defaults is the
> safest option after a power-cycle. If we want to keep this info in RAM
> and re-use it, then it is up to the manufacturer to implement a solution
> for this detection (as only they can know what kind of device they are
> making and whether that detection makes sense or not for it).
>
>>
>>>>> Another thing to consider is that we (TI) have GUI tools to allow
>>>>> manufacturers to input battery info and have it generate a blob that can
>>>>> be deployed to these devices. The blob format is meant to be handled and
>>>>> uploaded by custom user-space tools as it only really needs to be done
>>>>> once (power-down events should only happen when changing batteries, in
>>>>> which case you want to let the device forget what it has learned about
>>>>> the old battery).
>>>>>
>>>>> All of the details of this programming are probably not the job of the
>>>>> kernel, in end-user hands the kernel only needs to know how to read from
>>>>> the device, not program it.
>>>>
>>>> Were there an independently maintained battery manager app with wide
>>>> support for battery ICs, it would be a natural venue for BQ27*
>>>> reset/save/restore_nvm functions. But there is none that I know of,
>>>> and these features can be implemented in the driver with little new
>>>> code beyond the read/write_dm_block functions added by this patchset.
>>>>
>>>> It's fine to ask hardware makers to rely on IC vendor tools in the
>>>> factory, but I don't think such tools belong in the end-user's stack.
>>>>
>>>
>>> If they don't belong in the end-user's user-space tool-set then they
>>> definitely don't belong in kernel-space. The other option is to define a
>>> whole new API for interfacing with all these device specific parameters
>>> and internal info tables. It's that or we accept that these are factory
>>> settings and we don't need all too much kernel infrastructure for
>>> handling them.
>>
>> My point is that an IC vendor *codebase* does not belong in the
>> end-user's stack; it's a production tool. The functionality we're
>> discussing clearly belongs in both the device driver and the
>> production tool.
>>
>
> What functionality?, specifying your battery's design capacity, sure,
> reprogramming the gauge's internal "Impedance Track™" algorithm data,
> not so much. It's not up to me to draw the line in what goes into the
> kernel driver vs user-space tools, at this point I think we can wait for
> Sebastian to weigh in on this.

The functionality of save/restore pack-unique data!

Restating the pack-unique data (PUD) storage proposal:
- Allow driver to archive/install PUD from/to chip RAM
- Provide userspace api to above features via sysfs file
- If driver has a PUD storage policy, it writes/reads a blob/csv on disk
- PUD storage policy fields: bool poweroff, int period, char* filename
- PUD storage policy may be set via sysfs file or module param
- DT may give PUD storage policy as a fallback


>>>>> Take everything I say with a grain of salt though, I'm not on the team
>>>>> that makes these. I don't have much additional info not in the public
>>>>> data-sheets.
>>>>>
>>>>> Andrew
>>>>>
>>>>>>
>>>>>>>> That concept is beyond the scope of this patchset (and doesn't
>>>>>>>> supersede it), but is something I'd like to implement for our chip,
>>>>>>>> the *425.
>>>>>>>>
>>>>>>>>
>>>>>>>>> The properties added in this series are okay, and it's good to have some
>>>>>>>>> sane defaults in DT, but looking forward this may not be enough to get
>>>>>>>>> full use out of these devices.
>>>>>>>>
>>>>>>>> I'm relieved you're not opposed to our patch :-) Now I just need that
>>>>>>>> guidance I mentioned re the chip family...
>>>>>>>>
>>>>>>>
>>>>>>> Still looking into it, I think for now would be easiest to add the
>>>>>>> register info to each chip family struct. The un-seal sequence should be
>>>>>>> generic enough already.
>>>>>>
>>>>>> I'll wait for your full reply on that thread.
>>>>>>
>>>>>>
>>>>>>>>>> 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   | 37 ++++++++++++++++++++++
>>>>>>>>>>  1 file changed, 37 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..d78a4aa
>>>>>>>>>> --- /dev/null
>>>>>>>>>> +++ b/Documentation/devicetree/bindings/power/supply/battery.txt
>>>>>>>>>> @@ -0,0 +1,37 @@
>>>>>>>>>> +Battery Characteristics
>>>>>>>>>> +
>>>>>>>>>> +Required Properties:
>>>>>>>>>> + - compatible: Must be "fixed-battery"
>>>>>>>>>> +
>>>>>>>>>> +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
>>>>>>>>>> +
>>>>>>>>>> +Future Properties must be named for the corresponding elements in
>>>>>>>>>> +enum power_supply_property, defined in include/linux/power_supply.h.
>>>>>>>>>> +
>>>>>>>>>> +Batteries must be referenced by chargers and/or fuel-gauges
>>>>>>>>>> +using a phandle. The phandle's property should be named
>>>>>>>>>> +"monitored-battery".
>>>>>>>>>> +
>>>>>>>>>> +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>;
>>>>>>>>>> +             ...
>>>>>>>>>> +     };
>>>>>>>>>>
>>>>>>>> --
>>>>>>>> To unsubscribe from this list: send the line "unsubscribe devicetree" in
>>>>>>>> the body of a message to majordomo@vger.kernel.org
>>>>>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>>>>>>

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

[Andrew F. Davis]

On 02/15/2017 06:56 PM, Liam Breck wrote:
> On Wed, Feb 15, 2017 at 3:21 PM, Andrew F. Davis <afd@ti.com> wrote:
>> On 02/15/2017 02:05 PM, Liam Breck wrote:
>>> On Wed, Feb 15, 2017 at 7:54 AM, Andrew F. Davis <afd@ti.com> wrote:
>>>> On 02/14/2017 04:38 PM, Liam Breck wrote:
>>>>> On Tue, Feb 14, 2017 at 12:22 PM, Andrew F. Davis <afd@ti.com> wrote:
>>>>>> On 02/13/2017 03:31 PM, Liam Breck wrote:
>>>>>>> [Dropped devicetree from CC]
>>>>>>>
>>>>>>> On Mon, Feb 13, 2017 at 1:12 PM, Andrew F. Davis <afd@ti.com> wrote:
>>>>>>>> On 02/13/2017 02:58 PM, Liam Breck wrote:
>>>>>>>>> Hi Andrew, thanks for the drill-down on BQ27* chips.
>>>>>>>>>
>>>>>>>>> On Mon, Feb 13, 2017 at 8:06 AM, Andrew F. Davis <afd@ti.com> wrote:
>>>>>>>>>> On 02/10/2017 08:43 PM, Liam Breck wrote:
>>>>>>>>>>> From: Matt Ranostay <matt@ranostay.consulting>
>>>>>>>>>>>
>>>>>>>>>>> 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.
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> I think some of the confusion about what a "smart-battery" binding would
>>>>>>>>>> look like is due to a miss-understanding what it means to
>>>>>>>>>> "self-identify". Originally the bq27xxx *was* the chip that lived its
>>>>>>>>>> whole life inside a battery (the iphone battery is one such example and
>>>>>>>>>> a good way to test the bq27000). This way it could be factory programmed
>>>>>>>>>> with the battery's basics and then learn about the individual pack over
>>>>>>>>>> time to provide better info on time to empty, discharge rates, etc..
>>>>>>>>>>
>>>>>>>>>> Eventually, people wanted to have the battery become cheap and
>>>>>>>>>> replaceable, so fuel-gauges were made that are "system side", and ride
>>>>>>>>>> along with the device, not the battery. Now this has one big problem,
>>>>>>>>>> how can we learn about a battery when it may be a new battery every time
>>>>>>>>>> we boot up? So we added programmable memory to the devices, and
>>>>>>>>>> programming this memory looks to be what this series is about.
>>>>>>>>>>
>>>>>>>>>> Only the battery design info is programmed in this series, but much more
>>>>>>>>>> battery info can be uploaded, including stuff that is not fixed and
>>>>>>>>>> therefor cannot be added to the DT. Even the design parameters may not
>>>>>>>>>> be fix if one changes the battery as different manufactures of the same
>>>>>>>>>> battery type may have different design capacities.
>>>>>>>>>>
>>>>>>>>>> The ideal use-case in a system with a system side gauge is the OS should
>>>>>>>>>> periodically, or just before shutdown, read the battery's learned info
>>>>>>>>>> and when the device is powered back up, if the battery serial number
>>>>>>>>>> matches from before, re-upload this learned info to the gauge so it
>>>>>>>>>> doesn't have to re-learn everything. All of this will need to happen in
>>>>>>>>>> user-space, so what we really need is a user-space API for manipulating
>>>>>>>>>> these properties.
>>>>>>>>>
>>>>>>>>> We would need a feature in the driver to get/set the blob of pack- or
>>>>>>>>> cell-unique data. The userspace api is one sysfs file.
>>>>>>>>>
>>>>>>>>> However the DT fixed-battery node could indicate when/how-often the
>>>>>>>>> object is stored, and where, e.g. filename. If the latter is given,
>>>>>>>>> the driver should write/read the file itself.
>>>>>>>>>
>>>>>>>>> Is there some documentation about what registers should go in this blob?
>>>>>>>>>
>>>>>>>>
>>>>>>>> This should all be available in the parts' technical reference (looks
>>>>>>>> like page 24 for your device), but the blob isn't meant to be modified,
>>>>>>>> the information/format is mostly internal to the part, all we should do
>>>>>>>> is save blob / restore blob as needed.
>>>>>>>
>>>>>>> Page 24 has "8.5.5 Data Block Summary" That?
>>>>>>>
>>>>>>> Subclass IDs for NVM are 82, 88,104, 105. Do all those go in the blob?
>>>>>>>
>>>>>>
>>>>>> Some devices (bq27421) do not contain an NVM, and so rely on the host to
>>>>>> restore its "learned data" across power cycles with the same battery.
> 
> From your above statement, I inferred that NVM retains pack-unique
> stats on poweroff. Below you state that NVM only retains factory
> defaults. Could you clarify?
> 

This is a simple device, all that it knows is that it should move NVM
stats to RAM when it powers up, and then it uses and updates RAM stats
as it learns.

You can program the NVM values to the updated stats from RAM before
power-off manually if you would like, so that on the next power-on it
will move those updated values over to RAM. You should only do this if
you know you will have the same battery after a power-cycle.

Out of the box, the stats in NVM are the factory defaults, so if you
don't program NVM, and you power-cycle, you will get those factory
defaults in RAM. You are free to overwrite both the NVM and RAM with
updated values if you would like.

>>>>> Ah, so my concept for DT battery:nvm-storage: "filename" would apply
>>>>> to these RAM-only chips.
>>>>>
>>>>
>>>> I think so, I was going to do something similar, but let userspace take
>>>> care of the filename, or have it a fixed/module param like firmware. DT
>>>> is not really the right spot for filenames.
>>>>
>>>> If you can find a kernel-friendly way to do this then I'm all for it!
>>>
>>> Hm, you have a point about filenames in DT. Maybe an inode number? (kidding :-)
>>>
>>
>> :)
>>
>>> We could hand the driver a filename via sysfs file or module param.
>>> Perhaps use a filename in DT as a fallback.
>>>
>>
>> If you can convince Rob, then I'd say go for it.
> 
> OK, I restate the proposal below.
> 
>>>>>> When using devices with NVM this "learned data" can be saved to the NVM
>>>>>> and it will automatically be copied over to the run-time RAM sections on
>>>>>> powerup. So we should only ever need to save the RAM section.
>>>>>
>>>>> For the NVM chips, we'd want a userspace-accessible reset_nvm function
>>>>> in driver which returns the pack-unique data to factory defaults. That
>>>>> can be provided via a sysfs file to which you write a key (safer than
>>>>> "echo 1 > /sys/class.../reset_nvm") to trigger the reset.
>>>>>
>>>>
>>>> Still not sure why one would need this, the only time you would want to
>>>> reset the gauge is when changing the battery, and that will already
>>>> cause it to power cycle (unless you have some kind of back-up battery,
>>>> but that doesn't make sense to me).
>>>
>>> This is for gauges with NVM, where power-cycle does not clear the
>>> pack-specific stats. Or do those gauges recognize that the pack has
>>> changed?
>>>
>>
>> The NVM *is* the factory default, RAM tracks the  pack-specific stats,
>> when the device power-cycles NVM values are automatically moved over
>> into working RAM, so that is always cleared across all devices. There is
>> really no good way to recognize that a pack has changed, so simply
>> forgetting the pack-specific stats and re-loading the defaults is the
>> safest option after a power-cycle. If we want to keep this info in RAM
>> and re-use it, then it is up to the manufacturer to implement a solution
>> for this detection (as only they can know what kind of device they are
>> making and whether that detection makes sense or not for it).
>>
>>>
>>>>>> Another thing to consider is that we (TI) have GUI tools to allow
>>>>>> manufacturers to input battery info and have it generate a blob that can
>>>>>> be deployed to these devices. The blob format is meant to be handled and
>>>>>> uploaded by custom user-space tools as it only really needs to be done
>>>>>> once (power-down events should only happen when changing batteries, in
>>>>>> which case you want to let the device forget what it has learned about
>>>>>> the old battery).
>>>>>>
>>>>>> All of the details of this programming are probably not the job of the
>>>>>> kernel, in end-user hands the kernel only needs to know how to read from
>>>>>> the device, not program it.
>>>>>
>>>>> Were there an independently maintained battery manager app with wide
>>>>> support for battery ICs, it would be a natural venue for BQ27*
>>>>> reset/save/restore_nvm functions. But there is none that I know of,
>>>>> and these features can be implemented in the driver with little new
>>>>> code beyond the read/write_dm_block functions added by this patchset.
>>>>>
>>>>> It's fine to ask hardware makers to rely on IC vendor tools in the
>>>>> factory, but I don't think such tools belong in the end-user's stack.
>>>>>
>>>>
>>>> If they don't belong in the end-user's user-space tool-set then they
>>>> definitely don't belong in kernel-space. The other option is to define a
>>>> whole new API for interfacing with all these device specific parameters
>>>> and internal info tables. It's that or we accept that these are factory
>>>> settings and we don't need all too much kernel infrastructure for
>>>> handling them.
>>>
>>> My point is that an IC vendor *codebase* does not belong in the
>>> end-user's stack; it's a production tool. The functionality we're
>>> discussing clearly belongs in both the device driver and the
>>> production tool.
>>>
>>
>> What functionality?, specifying your battery's design capacity, sure,
>> reprogramming the gauge's internal "Impedance Track™" algorithm data,
>> not so much. It's not up to me to draw the line in what goes into the
>> kernel driver vs user-space tools, at this point I think we can wait for
>> Sebastian to weigh in on this.
> 
> The functionality of save/restore pack-unique data!
> 
> Restating the pack-unique data (PUD) storage proposal:
> - Allow driver to archive/install PUD from/to chip RAM
> - Provide userspace api to above features via sysfs file
> - If driver has a PUD storage policy, it writes/reads a blob/csv on disk
> - PUD storage policy fields: bool poweroff, int period, char* filename
> - PUD storage policy may be set via sysfs file or module param
> - DT may give PUD storage policy as a fallback
> 

I have no issue with any of this, but these are a bit out of the scope
of this patchset, right? Does anything in this set prevent the future
addition of those features?

Andrew

> 
>>>>>> Take everything I say with a grain of salt though, I'm not on the team
>>>>>> that makes these. I don't have much additional info not in the public
>>>>>> data-sheets.
>>>>>>
>>>>>> Andrew
>>>>>>
>>>>>>>
>>>>>>>>> That concept is beyond the scope of this patchset (and doesn't
>>>>>>>>> supersede it), but is something I'd like to implement for our chip,
>>>>>>>>> the *425.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>> The properties added in this series are okay, and it's good to have some
>>>>>>>>>> sane defaults in DT, but looking forward this may not be enough to get
>>>>>>>>>> full use out of these devices.
>>>>>>>>>
>>>>>>>>> I'm relieved you're not opposed to our patch :-) Now I just need that
>>>>>>>>> guidance I mentioned re the chip family...
>>>>>>>>>
>>>>>>>>
>>>>>>>> Still looking into it, I think for now would be easiest to add the
>>>>>>>> register info to each chip family struct. The un-seal sequence should be
>>>>>>>> generic enough already.
>>>>>>>
>>>>>>> I'll wait for your full reply on that thread.
>>>>>>>
>>>>>>>
>>>>>>>>>>> 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   | 37 ++++++++++++++++++++++
>>>>>>>>>>>  1 file changed, 37 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..d78a4aa
>>>>>>>>>>> --- /dev/null
>>>>>>>>>>> +++ b/Documentation/devicetree/bindings/power/supply/battery.txt
>>>>>>>>>>> @@ -0,0 +1,37 @@
>>>>>>>>>>> +Battery Characteristics
>>>>>>>>>>> +
>>>>>>>>>>> +Required Properties:
>>>>>>>>>>> + - compatible: Must be "fixed-battery"
>>>>>>>>>>> +
>>>>>>>>>>> +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
>>>>>>>>>>> +
>>>>>>>>>>> +Future Properties must be named for the corresponding elements in
>>>>>>>>>>> +enum power_supply_property, defined in include/linux/power_supply.h.
>>>>>>>>>>> +
>>>>>>>>>>> +Batteries must be referenced by chargers and/or fuel-gauges
>>>>>>>>>>> +using a phandle. The phandle's property should be named
>>>>>>>>>>> +"monitored-battery".
>>>>>>>>>>> +
>>>>>>>>>>> +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>;
>>>>>>>>>>> +             ...
>>>>>>>>>>> +     };
>>>>>>>>>>>
>>>>>>>>> --
>>>>>>>>> To unsubscribe from this list: send the line "unsubscribe devicetree" in
>>>>>>>>> the body of a message to majordomo@vger.kernel.org
>>>>>>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>>>>>>>

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

[Liam Breck]

On Fri, Feb 17, 2017 at 12:12 PM, Andrew F. Davis <afd@ti.com> wrote:
> On 02/15/2017 06:56 PM, Liam Breck wrote:
>> On Wed, Feb 15, 2017 at 3:21 PM, Andrew F. Davis <afd@ti.com> wrote:
>>> On 02/15/2017 02:05 PM, Liam Breck wrote:
>>>> On Wed, Feb 15, 2017 at 7:54 AM, Andrew F. Davis <afd@ti.com> wrote:
>>>>> On 02/14/2017 04:38 PM, Liam Breck wrote:
>>>>>> On Tue, Feb 14, 2017 at 12:22 PM, Andrew F. Davis <afd@ti.com> wrote:
>>>>>>> On 02/13/2017 03:31 PM, Liam Breck wrote:
>>>>>>>> [Dropped devicetree from CC]
>>>>>>>>
>>>>>>>> On Mon, Feb 13, 2017 at 1:12 PM, Andrew F. Davis <afd@ti.com> wrote:
>>>>>>>>> On 02/13/2017 02:58 PM, Liam Breck wrote:
>>>>>>>>>> Hi Andrew, thanks for the drill-down on BQ27* chips.
>>>>>>>>>>
>>>>>>>>>> On Mon, Feb 13, 2017 at 8:06 AM, Andrew F. Davis <afd@ti.com> wrote:
>>>>>>>>>>> On 02/10/2017 08:43 PM, Liam Breck wrote:
>>>>>>>>>>>> From: Matt Ranostay <matt@ranostay.consulting>
>>>>>>>>>>>>
>>>>>>>>>>>> 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.
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> I think some of the confusion about what a "smart-battery" binding would
>>>>>>>>>>> look like is due to a miss-understanding what it means to
>>>>>>>>>>> "self-identify". Originally the bq27xxx *was* the chip that lived its
>>>>>>>>>>> whole life inside a battery (the iphone battery is one such example and
>>>>>>>>>>> a good way to test the bq27000). This way it could be factory programmed
>>>>>>>>>>> with the battery's basics and then learn about the individual pack over
>>>>>>>>>>> time to provide better info on time to empty, discharge rates, etc..
>>>>>>>>>>>
>>>>>>>>>>> Eventually, people wanted to have the battery become cheap and
>>>>>>>>>>> replaceable, so fuel-gauges were made that are "system side", and ride
>>>>>>>>>>> along with the device, not the battery. Now this has one big problem,
>>>>>>>>>>> how can we learn about a battery when it may be a new battery every time
>>>>>>>>>>> we boot up? So we added programmable memory to the devices, and
>>>>>>>>>>> programming this memory looks to be what this series is about.
>>>>>>>>>>>
>>>>>>>>>>> Only the battery design info is programmed in this series, but much more
>>>>>>>>>>> battery info can be uploaded, including stuff that is not fixed and
>>>>>>>>>>> therefor cannot be added to the DT. Even the design parameters may not
>>>>>>>>>>> be fix if one changes the battery as different manufactures of the same
>>>>>>>>>>> battery type may have different design capacities.
>>>>>>>>>>>
>>>>>>>>>>> The ideal use-case in a system with a system side gauge is the OS should
>>>>>>>>>>> periodically, or just before shutdown, read the battery's learned info
>>>>>>>>>>> and when the device is powered back up, if the battery serial number
>>>>>>>>>>> matches from before, re-upload this learned info to the gauge so it
>>>>>>>>>>> doesn't have to re-learn everything. All of this will need to happen in
>>>>>>>>>>> user-space, so what we really need is a user-space API for manipulating
>>>>>>>>>>> these properties.
>>>>>>>>>>
>>>>>>>>>> We would need a feature in the driver to get/set the blob of pack- or
>>>>>>>>>> cell-unique data. The userspace api is one sysfs file.
>>>>>>>>>>
>>>>>>>>>> However the DT fixed-battery node could indicate when/how-often the
>>>>>>>>>> object is stored, and where, e.g. filename. If the latter is given,
>>>>>>>>>> the driver should write/read the file itself.
>>>>>>>>>>
>>>>>>>>>> Is there some documentation about what registers should go in this blob?
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> This should all be available in the parts' technical reference (looks
>>>>>>>>> like page 24 for your device), but the blob isn't meant to be modified,
>>>>>>>>> the information/format is mostly internal to the part, all we should do
>>>>>>>>> is save blob / restore blob as needed.
>>>>>>>>
>>>>>>>> Page 24 has "8.5.5 Data Block Summary" That?
>>>>>>>>
>>>>>>>> Subclass IDs for NVM are 82, 88,104, 105. Do all those go in the blob?
>>>>>>>>
>>>>>>>
>>>>>>> Some devices (bq27421) do not contain an NVM, and so rely on the host to
>>>>>>> restore its "learned data" across power cycles with the same battery.
>>
>> From your above statement, I inferred that NVM retains pack-unique
>> stats on poweroff. Below you state that NVM only retains factory
>> defaults. Could you clarify?
>>
>
> This is a simple device, all that it knows is that it should move NVM
> stats to RAM when it powers up, and then it uses and updates RAM stats
> as it learns.
>
> You can program the NVM values to the updated stats from RAM before
> power-off manually if you would like, so that on the next power-on it
> will move those updated values over to RAM. You should only do this if
> you know you will have the same battery after a power-cycle.
>
> Out of the box, the stats in NVM are the factory defaults, so if you
> don't program NVM, and you power-cycle, you will get those factory
> defaults in RAM. You are free to overwrite both the NVM and RAM with
> updated values if you would like.

Looking at the 425 manual, 8.5.5 Data Block Summary, page 24, the only
RAM & NVM blocks which correspond are the Ra Tables, 88 & 89. It
doesn't appear that any other RAM block could be saved to NVM.
http://www.ti.com/lit/ds/symlink/bq27425-g2a.pdf#24

Is the Ra Table the only bq27* RAM data we should store on poweroff to
NVM or disk?

>>>>>> Ah, so my concept for DT battery:nvm-storage: "filename" would apply
>>>>>> to these RAM-only chips.
>>>>>>
>>>>>
>>>>> I think so, I was going to do something similar, but let userspace take
>>>>> care of the filename, or have it a fixed/module param like firmware. DT
>>>>> is not really the right spot for filenames.
>>>>>
>>>>> If you can find a kernel-friendly way to do this then I'm all for it!
>>>>
>>>> Hm, you have a point about filenames in DT. Maybe an inode number? (kidding :-)
>>>>
>>>
>>> :)
>>>
>>>> We could hand the driver a filename via sysfs file or module param.
>>>> Perhaps use a filename in DT as a fallback.
>>>>
>>>
>>> If you can convince Rob, then I'd say go for it.
>>
>> OK, I restate the proposal below.
>>
>>>>>>> When using devices with NVM this "learned data" can be saved to the NVM
>>>>>>> and it will automatically be copied over to the run-time RAM sections on
>>>>>>> powerup. So we should only ever need to save the RAM section.
>>>>>>
>>>>>> For the NVM chips, we'd want a userspace-accessible reset_nvm function
>>>>>> in driver which returns the pack-unique data to factory defaults. That
>>>>>> can be provided via a sysfs file to which you write a key (safer than
>>>>>> "echo 1 > /sys/class.../reset_nvm") to trigger the reset.
>>>>>>
>>>>>
>>>>> Still not sure why one would need this, the only time you would want to
>>>>> reset the gauge is when changing the battery, and that will already
>>>>> cause it to power cycle (unless you have some kind of back-up battery,
>>>>> but that doesn't make sense to me).
>>>>
>>>> This is for gauges with NVM, where power-cycle does not clear the
>>>> pack-specific stats. Or do those gauges recognize that the pack has
>>>> changed?
>>>>
>>>
>>> The NVM *is* the factory default, RAM tracks the  pack-specific stats,
>>> when the device power-cycles NVM values are automatically moved over
>>> into working RAM, so that is always cleared across all devices. There is
>>> really no good way to recognize that a pack has changed, so simply
>>> forgetting the pack-specific stats and re-loading the defaults is the
>>> safest option after a power-cycle. If we want to keep this info in RAM
>>> and re-use it, then it is up to the manufacturer to implement a solution
>>> for this detection (as only they can know what kind of device they are
>>> making and whether that detection makes sense or not for it).
>>>
>>>>
>>>>>>> Another thing to consider is that we (TI) have GUI tools to allow
>>>>>>> manufacturers to input battery info and have it generate a blob that can
>>>>>>> be deployed to these devices. The blob format is meant to be handled and
>>>>>>> uploaded by custom user-space tools as it only really needs to be done
>>>>>>> once (power-down events should only happen when changing batteries, in
>>>>>>> which case you want to let the device forget what it has learned about
>>>>>>> the old battery).
>>>>>>>
>>>>>>> All of the details of this programming are probably not the job of the
>>>>>>> kernel, in end-user hands the kernel only needs to know how to read from
>>>>>>> the device, not program it.
>>>>>>
>>>>>> Were there an independently maintained battery manager app with wide
>>>>>> support for battery ICs, it would be a natural venue for BQ27*
>>>>>> reset/save/restore_nvm functions. But there is none that I know of,
>>>>>> and these features can be implemented in the driver with little new
>>>>>> code beyond the read/write_dm_block functions added by this patchset.
>>>>>>
>>>>>> It's fine to ask hardware makers to rely on IC vendor tools in the
>>>>>> factory, but I don't think such tools belong in the end-user's stack.
>>>>>>
>>>>>
>>>>> If they don't belong in the end-user's user-space tool-set then they
>>>>> definitely don't belong in kernel-space. The other option is to define a
>>>>> whole new API for interfacing with all these device specific parameters
>>>>> and internal info tables. It's that or we accept that these are factory
>>>>> settings and we don't need all too much kernel infrastructure for
>>>>> handling them.
>>>>
>>>> My point is that an IC vendor *codebase* does not belong in the
>>>> end-user's stack; it's a production tool. The functionality we're
>>>> discussing clearly belongs in both the device driver and the
>>>> production tool.
>>>>
>>>
>>> What functionality?, specifying your battery's design capacity, sure,
>>> reprogramming the gauge's internal "Impedance Track™" algorithm data,
>>> not so much. It's not up to me to draw the line in what goes into the
>>> kernel driver vs user-space tools, at this point I think we can wait for
>>> Sebastian to weigh in on this.
>>
>> The functionality of save/restore pack-unique data!
>>
>> Restating the pack-unique data (PUD) storage proposal:
>> - Allow driver to archive/install PUD from/to chip RAM
>> - Provide userspace api to above features via sysfs file
>> - If driver has a PUD storage policy, it writes/reads a blob/csv on disk
>> - PUD storage policy fields: bool poweroff, int period, char* filename
>> - PUD storage policy may be set via sysfs file or module param
>> - DT may give PUD storage policy as a fallback

"Pack-unique data" should perhaps be "Ra Table" unless there's more to it.

One more feature for the above list:
- DT bq27xxx:save-ra-table saves table in NVM (if present) on poweroff
I'll tackle this first since our board's chip has NVM.

> I have no issue with any of this, but these are a bit out of the scope
> of this patchset, right? Does anything in this set prevent the future
> addition of those features?

Yes the above is outside scope of this patchset, but will use four
functions added in it.

>>>>>>> Take everything I say with a grain of salt though, I'm not on the team
>>>>>>> that makes these. I don't have much additional info not in the public
>>>>>>> data-sheets.
>>>>>>>
>>>>>>> Andrew
>>>>>>>
>>>>>>>>
>>>>>>>>>> That concept is beyond the scope of this patchset (and doesn't
>>>>>>>>>> supersede it), but is something I'd like to implement for our chip,
>>>>>>>>>> the *425.
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>> The properties added in this series are okay, and it's good to have some
>>>>>>>>>>> sane defaults in DT, but looking forward this may not be enough to get
>>>>>>>>>>> full use out of these devices.
>>>>>>>>>>
>>>>>>>>>> I'm relieved you're not opposed to our patch :-) Now I just need that
>>>>>>>>>> guidance I mentioned re the chip family...
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Still looking into it, I think for now would be easiest to add the
>>>>>>>>> register info to each chip family struct. The un-seal sequence should be
>>>>>>>>> generic enough already.
>>>>>>>>
>>>>>>>> I'll wait for your full reply on that thread.
>>>>>>>>
>>>>>>>>
>>>>>>>>>>>> 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   | 37 ++++++++++++++++++++++
>>>>>>>>>>>>  1 file changed, 37 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..d78a4aa
>>>>>>>>>>>> --- /dev/null
>>>>>>>>>>>> +++ b/Documentation/devicetree/bindings/power/supply/battery.txt
>>>>>>>>>>>> @@ -0,0 +1,37 @@
>>>>>>>>>>>> +Battery Characteristics
>>>>>>>>>>>> +
>>>>>>>>>>>> +Required Properties:
>>>>>>>>>>>> + - compatible: Must be "fixed-battery"
>>>>>>>>>>>> +
>>>>>>>>>>>> +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
>>>>>>>>>>>> +
>>>>>>>>>>>> +Future Properties must be named for the corresponding elements in
>>>>>>>>>>>> +enum power_supply_property, defined in include/linux/power_supply.h.
>>>>>>>>>>>> +
>>>>>>>>>>>> +Batteries must be referenced by chargers and/or fuel-gauges
>>>>>>>>>>>> +using a phandle. The phandle's property should be named
>>>>>>>>>>>> +"monitored-battery".
>>>>>>>>>>>> +
>>>>>>>>>>>> +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>;
>>>>>>>>>>>> +             ...
>>>>>>>>>>>> +     };
>>>>>>>>>>>>
>>>>>>>>>> --
>>>>>>>>>> To unsubscribe from this list: send the line "unsubscribe devicetree" in
>>>>>>>>>> the body of a message to majordomo@vger.kernel.org
>>>>>>>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>>>>>>>>

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

[Andrew F. Davis]

On 02/17/2017 10:10 PM, Liam Breck wrote:
> On Fri, Feb 17, 2017 at 12:12 PM, Andrew F. Davis <afd@ti.com> wrote:
>> On 02/15/2017 06:56 PM, Liam Breck wrote:
>>> On Wed, Feb 15, 2017 at 3:21 PM, Andrew F. Davis <afd@ti.com> wrote:
>>>> On 02/15/2017 02:05 PM, Liam Breck wrote:
>>>>> On Wed, Feb 15, 2017 at 7:54 AM, Andrew F. Davis <afd@ti.com> wrote:
>>>>>> On 02/14/2017 04:38 PM, Liam Breck wrote:
>>>>>>> On Tue, Feb 14, 2017 at 12:22 PM, Andrew F. Davis <afd@ti.com> wrote:
>>>>>>>> On 02/13/2017 03:31 PM, Liam Breck wrote:
>>>>>>>>> [Dropped devicetree from CC]
>>>>>>>>>
>>>>>>>>> On Mon, Feb 13, 2017 at 1:12 PM, Andrew F. Davis <afd@ti.com> wrote:
>>>>>>>>>> On 02/13/2017 02:58 PM, Liam Breck wrote:
>>>>>>>>>>> Hi Andrew, thanks for the drill-down on BQ27* chips.
>>>>>>>>>>>
>>>>>>>>>>> On Mon, Feb 13, 2017 at 8:06 AM, Andrew F. Davis <afd@ti.com> wrote:
>>>>>>>>>>>> On 02/10/2017 08:43 PM, Liam Breck wrote:
>>>>>>>>>>>>> From: Matt Ranostay <matt@ranostay.consulting>
>>>>>>>>>>>>>
>>>>>>>>>>>>> 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.
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> I think some of the confusion about what a "smart-battery" binding would
>>>>>>>>>>>> look like is due to a miss-understanding what it means to
>>>>>>>>>>>> "self-identify". Originally the bq27xxx *was* the chip that lived its
>>>>>>>>>>>> whole life inside a battery (the iphone battery is one such example and
>>>>>>>>>>>> a good way to test the bq27000). This way it could be factory programmed
>>>>>>>>>>>> with the battery's basics and then learn about the individual pack over
>>>>>>>>>>>> time to provide better info on time to empty, discharge rates, etc..
>>>>>>>>>>>>
>>>>>>>>>>>> Eventually, people wanted to have the battery become cheap and
>>>>>>>>>>>> replaceable, so fuel-gauges were made that are "system side", and ride
>>>>>>>>>>>> along with the device, not the battery. Now this has one big problem,
>>>>>>>>>>>> how can we learn about a battery when it may be a new battery every time
>>>>>>>>>>>> we boot up? So we added programmable memory to the devices, and
>>>>>>>>>>>> programming this memory looks to be what this series is about.
>>>>>>>>>>>>
>>>>>>>>>>>> Only the battery design info is programmed in this series, but much more
>>>>>>>>>>>> battery info can be uploaded, including stuff that is not fixed and
>>>>>>>>>>>> therefor cannot be added to the DT. Even the design parameters may not
>>>>>>>>>>>> be fix if one changes the battery as different manufactures of the same
>>>>>>>>>>>> battery type may have different design capacities.
>>>>>>>>>>>>
>>>>>>>>>>>> The ideal use-case in a system with a system side gauge is the OS should
>>>>>>>>>>>> periodically, or just before shutdown, read the battery's learned info
>>>>>>>>>>>> and when the device is powered back up, if the battery serial number
>>>>>>>>>>>> matches from before, re-upload this learned info to the gauge so it
>>>>>>>>>>>> doesn't have to re-learn everything. All of this will need to happen in
>>>>>>>>>>>> user-space, so what we really need is a user-space API for manipulating
>>>>>>>>>>>> these properties.
>>>>>>>>>>>
>>>>>>>>>>> We would need a feature in the driver to get/set the blob of pack- or
>>>>>>>>>>> cell-unique data. The userspace api is one sysfs file.
>>>>>>>>>>>
>>>>>>>>>>> However the DT fixed-battery node could indicate when/how-often the
>>>>>>>>>>> object is stored, and where, e.g. filename. If the latter is given,
>>>>>>>>>>> the driver should write/read the file itself.
>>>>>>>>>>>
>>>>>>>>>>> Is there some documentation about what registers should go in this blob?
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> This should all be available in the parts' technical reference (looks
>>>>>>>>>> like page 24 for your device), but the blob isn't meant to be modified,
>>>>>>>>>> the information/format is mostly internal to the part, all we should do
>>>>>>>>>> is save blob / restore blob as needed.
>>>>>>>>>
>>>>>>>>> Page 24 has "8.5.5 Data Block Summary" That?
>>>>>>>>>
>>>>>>>>> Subclass IDs for NVM are 82, 88,104, 105. Do all those go in the blob?
>>>>>>>>>
>>>>>>>>
>>>>>>>> Some devices (bq27421) do not contain an NVM, and so rely on the host to
>>>>>>>> restore its "learned data" across power cycles with the same battery.
>>>
>>> From your above statement, I inferred that NVM retains pack-unique
>>> stats on poweroff. Below you state that NVM only retains factory
>>> defaults. Could you clarify?
>>>
>>
>> This is a simple device, all that it knows is that it should move NVM
>> stats to RAM when it powers up, and then it uses and updates RAM stats
>> as it learns.
>>
>> You can program the NVM values to the updated stats from RAM before
>> power-off manually if you would like, so that on the next power-on it
>> will move those updated values over to RAM. You should only do this if
>> you know you will have the same battery after a power-cycle.
>>
>> Out of the box, the stats in NVM are the factory defaults, so if you
>> don't program NVM, and you power-cycle, you will get those factory
>> defaults in RAM. You are free to overwrite both the NVM and RAM with
>> updated values if you would like.
> 
> Looking at the 425 manual, 8.5.5 Data Block Summary, page 24, the only
> RAM & NVM blocks which correspond are the Ra Tables, 88 & 89. It
> doesn't appear that any other RAM block could be saved to NVM.
> http://www.ti.com/lit/ds/symlink/bq27425-g2a.pdf#24
> 
> Is the Ra Table the only bq27* RAM data we should store on poweroff to
> NVM or disk?
> 

See below

>>>>>>> Ah, so my concept for DT battery:nvm-storage: "filename" would apply
>>>>>>> to these RAM-only chips.
>>>>>>>
>>>>>>
>>>>>> I think so, I was going to do something similar, but let userspace take
>>>>>> care of the filename, or have it a fixed/module param like firmware. DT
>>>>>> is not really the right spot for filenames.
>>>>>>
>>>>>> If you can find a kernel-friendly way to do this then I'm all for it!
>>>>>
>>>>> Hm, you have a point about filenames in DT. Maybe an inode number? (kidding :-)
>>>>>
>>>>
>>>> :)
>>>>
>>>>> We could hand the driver a filename via sysfs file or module param.
>>>>> Perhaps use a filename in DT as a fallback.
>>>>>
>>>>
>>>> If you can convince Rob, then I'd say go for it.
>>>
>>> OK, I restate the proposal below.
>>>
>>>>>>>> When using devices with NVM this "learned data" can be saved to the NVM
>>>>>>>> and it will automatically be copied over to the run-time RAM sections on
>>>>>>>> powerup. So we should only ever need to save the RAM section.
>>>>>>>
>>>>>>> For the NVM chips, we'd want a userspace-accessible reset_nvm function
>>>>>>> in driver which returns the pack-unique data to factory defaults. That
>>>>>>> can be provided via a sysfs file to which you write a key (safer than
>>>>>>> "echo 1 > /sys/class.../reset_nvm") to trigger the reset.
>>>>>>>
>>>>>>
>>>>>> Still not sure why one would need this, the only time you would want to
>>>>>> reset the gauge is when changing the battery, and that will already
>>>>>> cause it to power cycle (unless you have some kind of back-up battery,
>>>>>> but that doesn't make sense to me).
>>>>>
>>>>> This is for gauges with NVM, where power-cycle does not clear the
>>>>> pack-specific stats. Or do those gauges recognize that the pack has
>>>>> changed?
>>>>>
>>>>
>>>> The NVM *is* the factory default, RAM tracks the  pack-specific stats,
>>>> when the device power-cycles NVM values are automatically moved over
>>>> into working RAM, so that is always cleared across all devices. There is
>>>> really no good way to recognize that a pack has changed, so simply
>>>> forgetting the pack-specific stats and re-loading the defaults is the
>>>> safest option after a power-cycle. If we want to keep this info in RAM
>>>> and re-use it, then it is up to the manufacturer to implement a solution
>>>> for this detection (as only they can know what kind of device they are
>>>> making and whether that detection makes sense or not for it).
>>>>
>>>>>
>>>>>>>> Another thing to consider is that we (TI) have GUI tools to allow
>>>>>>>> manufacturers to input battery info and have it generate a blob that can
>>>>>>>> be deployed to these devices. The blob format is meant to be handled and
>>>>>>>> uploaded by custom user-space tools as it only really needs to be done
>>>>>>>> once (power-down events should only happen when changing batteries, in
>>>>>>>> which case you want to let the device forget what it has learned about
>>>>>>>> the old battery).
>>>>>>>>
>>>>>>>> All of the details of this programming are probably not the job of the
>>>>>>>> kernel, in end-user hands the kernel only needs to know how to read from
>>>>>>>> the device, not program it.
>>>>>>>
>>>>>>> Were there an independently maintained battery manager app with wide
>>>>>>> support for battery ICs, it would be a natural venue for BQ27*
>>>>>>> reset/save/restore_nvm functions. But there is none that I know of,
>>>>>>> and these features can be implemented in the driver with little new
>>>>>>> code beyond the read/write_dm_block functions added by this patchset.
>>>>>>>
>>>>>>> It's fine to ask hardware makers to rely on IC vendor tools in the
>>>>>>> factory, but I don't think such tools belong in the end-user's stack.
>>>>>>>
>>>>>>
>>>>>> If they don't belong in the end-user's user-space tool-set then they
>>>>>> definitely don't belong in kernel-space. The other option is to define a
>>>>>> whole new API for interfacing with all these device specific parameters
>>>>>> and internal info tables. It's that or we accept that these are factory
>>>>>> settings and we don't need all too much kernel infrastructure for
>>>>>> handling them.
>>>>>
>>>>> My point is that an IC vendor *codebase* does not belong in the
>>>>> end-user's stack; it's a production tool. The functionality we're
>>>>> discussing clearly belongs in both the device driver and the
>>>>> production tool.
>>>>>
>>>>
>>>> What functionality?, specifying your battery's design capacity, sure,
>>>> reprogramming the gauge's internal "Impedance Track™" algorithm data,
>>>> not so much. It's not up to me to draw the line in what goes into the
>>>> kernel driver vs user-space tools, at this point I think we can wait for
>>>> Sebastian to weigh in on this.
>>>
>>> The functionality of save/restore pack-unique data!
>>>
>>> Restating the pack-unique data (PUD) storage proposal:
>>> - Allow driver to archive/install PUD from/to chip RAM
>>> - Provide userspace api to above features via sysfs file
>>> - If driver has a PUD storage policy, it writes/reads a blob/csv on disk
>>> - PUD storage policy fields: bool poweroff, int period, char* filename
>>> - PUD storage policy may be set via sysfs file or module param
>>> - DT may give PUD storage policy as a fallback
> 
> "Pack-unique data" should perhaps be "Ra Table" unless there's more to it.
> 

I'm not really sure on this one, (or even what the Ra Table does
exactly), this may be something that could be asked over in the fuel
gauge forum on e2e[0], someone over there may have more info on intended
uses of this.

Andrew

[0] http://e2e.ti.com/support/power_management/battery_management/f/180

> One more feature for the above list:
> - DT bq27xxx:save-ra-table saves table in NVM (if present) on poweroff
> I'll tackle this first since our board's chip has NVM.
> 
>> I have no issue with any of this, but these are a bit out of the scope
>> of this patchset, right? Does anything in this set prevent the future
>> addition of those features?
> 
> Yes the above is outside scope of this patchset, but will use four
> functions added in it.
> 
>>>>>>>> Take everything I say with a grain of salt though, I'm not on the team
>>>>>>>> that makes these. I don't have much additional info not in the public
>>>>>>>> data-sheets.
>>>>>>>>
>>>>>>>> Andrew
>>>>>>>>
>>>>>>>>>
>>>>>>>>>>> That concept is beyond the scope of this patchset (and doesn't
>>>>>>>>>>> supersede it), but is something I'd like to implement for our chip,
>>>>>>>>>>> the *425.
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>> The properties added in this series are okay, and it's good to have some
>>>>>>>>>>>> sane defaults in DT, but looking forward this may not be enough to get
>>>>>>>>>>>> full use out of these devices.
>>>>>>>>>>>
>>>>>>>>>>> I'm relieved you're not opposed to our patch :-) Now I just need that
>>>>>>>>>>> guidance I mentioned re the chip family...
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Still looking into it, I think for now would be easiest to add the
>>>>>>>>>> register info to each chip family struct. The un-seal sequence should be
>>>>>>>>>> generic enough already.
>>>>>>>>>
>>>>>>>>> I'll wait for your full reply on that thread.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>>>>> 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   | 37 ++++++++++++++++++++++
>>>>>>>>>>>>>  1 file changed, 37 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..d78a4aa
>>>>>>>>>>>>> --- /dev/null
>>>>>>>>>>>>> +++ b/Documentation/devicetree/bindings/power/supply/battery.txt
>>>>>>>>>>>>> @@ -0,0 +1,37 @@
>>>>>>>>>>>>> +Battery Characteristics
>>>>>>>>>>>>> +
>>>>>>>>>>>>> +Required Properties:
>>>>>>>>>>>>> + - compatible: Must be "fixed-battery"
>>>>>>>>>>>>> +
>>>>>>>>>>>>> +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
>>>>>>>>>>>>> +
>>>>>>>>>>>>> +Future Properties must be named for the corresponding elements in
>>>>>>>>>>>>> +enum power_supply_property, defined in include/linux/power_supply.h.
>>>>>>>>>>>>> +
>>>>>>>>>>>>> +Batteries must be referenced by chargers and/or fuel-gauges
>>>>>>>>>>>>> +using a phandle. The phandle's property should be named
>>>>>>>>>>>>> +"monitored-battery".
>>>>>>>>>>>>> +
>>>>>>>>>>>>> +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>;
>>>>>>>>>>>>> +             ...
>>>>>>>>>>>>> +     };
>>>>>>>>>>>>>
>>>>>>>>>>> --
>>>>>>>>>>> To unsubscribe from this list: send the line "unsubscribe devicetree" in
>>>>>>>>>>> the body of a message to majordomo@vger.kernel.org
>>>>>>>>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>>>>>>>>>

[PATCH v6 2/8] devicetree: property-units: Add uWh and uAh units

[Liam Breck]

From: Matt Ranostay <matt@ranostay.consulting>

Add entries for microwatt-hours and microamp-hours.

Cc: Rob Herring <robh@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: devicetree@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Matt Ranostay <matt@ranostay.consulting>
Signed-off-by: Liam Breck <kernel@networkimprov.net>
Acked-by: Sebastian Reichel <sre@kernel.org>
Acked-by: Rob Herring <robh@kernel.org>
---
 Documentation/devicetree/bindings/property-units.txt | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/property-units.txt b/Documentation/devicetree/bindings/property-units.txt
index 12278d7..0849618 100644
--- a/Documentation/devicetree/bindings/property-units.txt
+++ b/Documentation/devicetree/bindings/property-units.txt
@@ -25,8 +25,10 @@ Distance
 Electricity
 ----------------------------------------
 -microamp	: micro amps
+-microamp-hours : micro amp-hours
 -ohms		: Ohms
 -micro-ohms	: micro Ohms
+-microwatt-hours: micro Watt-hours
 -microvolt	: micro volts
 
 Temperature
-- 
2.9.3

[PATCH v6 2/8] devicetree: property-units: Add uWh and uAh units

[Liam Breck]

From: Matt Ranostay <matt-sk+viVC6FLCDq+mSdOJa79kegs52MxvZ@public.gmane.org>

Add entries for microwatt-hours and microamp-hours.

Cc: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Signed-off-by: Matt Ranostay <matt-sk+viVC6FLCDq+mSdOJa79kegs52MxvZ@public.gmane.org>
Signed-off-by: Liam Breck <kernel-RYWXG+zxWwBdeoIcmNTgJF6hYfS7NtTn@public.gmane.org>
Acked-by: Sebastian Reichel <sre-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 Documentation/devicetree/bindings/property-units.txt | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/property-units.txt b/Documentation/devicetree/bindings/property-units.txt
index 12278d7..0849618 100644
--- a/Documentation/devicetree/bindings/property-units.txt
+++ b/Documentation/devicetree/bindings/property-units.txt
@@ -25,8 +25,10 @@ Distance
 Electricity
 ----------------------------------------
 -microamp	: micro amps
+-microamp-hours : micro amp-hours
 -ohms		: Ohms
 -micro-ohms	: micro Ohms
+-microwatt-hours: micro Watt-hours
 -microvolt	: micro volts
 
 Temperature
-- 
2.9.3

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v6 3/8] devicetree: power: bq27xxx: Add monitored-battery documentation

[Liam Breck]

From: Matt Ranostay <matt@ranostay.consulting>

Document monitored-battery = <&battery_node>

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>
---
 Documentation/devicetree/bindings/power/supply/bq27xxx.txt | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/power/supply/bq27xxx.txt b/Documentation/devicetree/bindings/power/supply/bq27xxx.txt
index b0c95ef..cf83371 100644
--- a/Documentation/devicetree/bindings/power/supply/bq27xxx.txt
+++ b/Documentation/devicetree/bindings/power/supply/bq27xxx.txt
@@ -28,9 +28,18 @@ Required properties:
  * "ti,bq27621" - BQ27621
 - reg: integer, i2c address of the device.
 
+Optional properties:
+- monitored-battery: phandle of battery information devicetree node
+
+  See Documentation/devicetree/bindings/power/supply/battery.txt
+  If either of the referenced battery's *-full-design-*-hours properties are set,
+  then both must be.
+
 Example:
 
-bq27510g3 {
+bq27510g3 : fuel-gauge@55 {
     compatible = "ti,bq27510g3";
     reg = <0x55>;
+
+    monitored-battery = <&bat>;
 };
-- 
2.9.3

[PATCH v6 4/8] power: power_supply: Add power_supply_battery_info and API

[Liam Breck]

From: Matt Ranostay <matt@ranostay.consulting>

power_supply_get_battery_info() reads battery data from devicetree.
struct power_supply_battery_info provides battery data to drivers.
Drivers may surface battery data in sysfs via corresponding
POWER_SUPPLY_PROP_* fields.

Signed-off-by: Matt Ranostay <matt@ranostay.consulting>
Signed-off-by: Liam Breck <kernel@networkimprov.net>
---
 drivers/power/supply/power_supply_core.c | 40 ++++++++++++++++++++++++++++++++
 include/linux/power_supply.h             | 18 ++++++++++++++
 2 files changed, 58 insertions(+)

diff --git a/drivers/power/supply/power_supply_core.c b/drivers/power/supply/power_supply_core.c
index a74d8ca..c121931 100644
--- a/drivers/power/supply/power_supply_core.c
+++ b/drivers/power/supply/power_supply_core.c
@@ -17,6 +17,7 @@
 #include <linux/device.h>
 #include <linux/notifier.h>
 #include <linux/err.h>
+#include <linux/of.h>
 #include <linux/power_supply.h>
 #include <linux/thermal.h>
 #include "power_supply.h"
@@ -487,6 +488,45 @@ struct power_supply *devm_power_supply_get_by_phandle(struct device *dev,
 EXPORT_SYMBOL_GPL(devm_power_supply_get_by_phandle);
 #endif /* CONFIG_OF */
 
+int power_supply_get_battery_info(struct power_supply *psy,
+				  struct power_supply_battery_info *info)
+{
+	struct device_node *battery_np;
+	const char *value;
+	int err;
+
+	info->energy_full_design_uwh = -EINVAL;
+	info->charge_full_design_uah = -EINVAL;
+	info->voltage_min_design_uv  = -EINVAL;
+
+	if (!psy->of_node) {
+		dev_warn(&psy->dev, "%s currently only supports devicetree\n",
+			 __func__);
+		return -ENXIO;
+	}
+
+	battery_np = of_parse_phandle(psy->of_node, "monitored-battery", 0);
+	if (!battery_np)
+		return -ENODEV;
+
+	err = of_property_read_string(battery_np, "compatible", &value);
+	if (err)
+		return err;
+
+	if (strcmp("fixed-battery", value))
+		return -ENODEV;
+
+	of_property_read_u32(battery_np, "energy-full-design-microwatt-hours",
+			     &info->energy_full_design_uwh);
+	of_property_read_u32(battery_np, "charge-full-design-microamp-hours",
+			     &info->charge_full_design_uah);
+	of_property_read_u32(battery_np, "voltage-min-design-microvolt",
+			     &info->voltage_min_design_uv);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(power_supply_get_battery_info);
+
 int power_supply_get_property(struct power_supply *psy,
 			    enum power_supply_property psp,
 			    union power_supply_propval *val)
diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
index 3965503..e84f1d3 100644
--- a/include/linux/power_supply.h
+++ b/include/linux/power_supply.h
@@ -288,6 +288,21 @@ struct power_supply_info {
 	int use_for_apm;
 };
 
+/*
+ * This is the recommended struct to manage static battery parameters,
+ * populated by power_supply_get_battery_info(). Most platform drivers should
+ * use these for consistency.
+ * Its field names must correspond to elements in enum power_supply_property.
+ * The default field value is -EINVAL.
+ * Power supply class itself doesn't use this.
+ */
+
+struct power_supply_battery_info {
+	int energy_full_design_uwh;	/* microWatt-hours */
+	int charge_full_design_uah;	/* microAmp-hours */
+	int voltage_min_design_uv;	/* microVolts */
+};
+
 extern struct atomic_notifier_head power_supply_notifier;
 extern int power_supply_reg_notifier(struct notifier_block *nb);
 extern void power_supply_unreg_notifier(struct notifier_block *nb);
@@ -306,6 +321,9 @@ static inline struct power_supply *
 devm_power_supply_get_by_phandle(struct device *dev, const char *property)
 { return NULL; }
 #endif /* CONFIG_OF */
+
+extern int power_supply_get_battery_info(struct power_supply *psy,
+					 struct power_supply_battery_info *info);
 extern void power_supply_changed(struct power_supply *psy);
 extern int power_supply_am_i_supplied(struct power_supply *psy);
 extern int power_supply_set_battery_charged(struct power_supply *psy);
-- 
2.9.3

[PATCH v6 5/8] power: bq27xxx_battery: Define access methods to write chip registers

[Liam Breck]

From: Matt Ranostay <matt@ranostay.consulting>

write(), read_bulk(), write_bulk() support setting chip registers,
e.g. with data obtained from power_supply_battery_info.

Signed-off-by: Matt Ranostay <matt@ranostay.consulting>
Signed-off-by: Liam Breck <kernel@networkimprov.net>
---
 include/linux/power/bq27xxx_battery.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/linux/power/bq27xxx_battery.h b/include/linux/power/bq27xxx_battery.h
index bed9557..92df553 100644
--- a/include/linux/power/bq27xxx_battery.h
+++ b/include/linux/power/bq27xxx_battery.h
@@ -32,6 +32,9 @@ struct bq27xxx_platform_data {
 struct bq27xxx_device_info;
 struct bq27xxx_access_methods {
 	int (*read)(struct bq27xxx_device_info *di, u8 reg, bool single);
+	int (*write)(struct bq27xxx_device_info *di, u8 reg, int value, bool single);
+	int (*read_bulk)(struct bq27xxx_device_info *di, u8 reg, u8 *data, int len);
+	int (*write_bulk)(struct bq27xxx_device_info *di, u8 reg, u8 *data, int len);
 };
 
 struct bq27xxx_reg_cache {
-- 
2.9.3

[PATCH v6 6/8] power: bq27xxx_battery: Add BQ27425 chip id

[Liam Breck]

From: Matt Ranostay <matt@ranostay.consulting>

This patch does not alter the function of the driver.
This chip ID is referenced in the devicetree code.

Signed-off-by: Matt Ranostay <matt@ranostay.consulting>
Signed-off-by: Liam Breck <kernel@networkimprov.net>
---
 drivers/power/supply/bq27xxx_battery.c     | 23 ++++++++++++++++++++++-
 drivers/power/supply/bq27xxx_battery_i2c.c |  2 +-
 include/linux/power/bq27xxx_battery.h      |  3 ++-
 3 files changed, 25 insertions(+), 3 deletions(-)

diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
index 7272d1e..7475a5f 100644
--- a/drivers/power/supply/bq27xxx_battery.c
+++ b/drivers/power/supply/bq27xxx_battery.c
@@ -259,6 +259,25 @@ static u8 bq27xxx_regs[][BQ27XXX_REG_MAX] = {
 		[BQ27XXX_REG_DCAP] = 0x3c,
 		[BQ27XXX_REG_AP] = 0x18,
 	},
+	[BQ27425] = {
+		[BQ27XXX_REG_CTRL] = 0x00,
+		[BQ27XXX_REG_TEMP] = 0x02,
+		[BQ27XXX_REG_INT_TEMP] = 0x1e,
+		[BQ27XXX_REG_VOLT] = 0x04,
+		[BQ27XXX_REG_AI] = 0x10,
+		[BQ27XXX_REG_FLAGS] = 0x06,
+		[BQ27XXX_REG_TTE] = INVALID_REG_ADDR,
+		[BQ27XXX_REG_TTF] = INVALID_REG_ADDR,
+		[BQ27XXX_REG_TTES] = INVALID_REG_ADDR,
+		[BQ27XXX_REG_TTECP] = INVALID_REG_ADDR,
+		[BQ27XXX_REG_NAC] = 0x08,
+		[BQ27XXX_REG_FCC] = 0x0e,
+		[BQ27XXX_REG_CYCT] = INVALID_REG_ADDR,
+		[BQ27XXX_REG_AE] = INVALID_REG_ADDR,
+		[BQ27XXX_REG_SOC] = 0x1c,
+		[BQ27XXX_REG_DCAP] = 0x3c,
+		[BQ27XXX_REG_AP] = 0x18,
+	},
 };
 
 static enum power_supply_property bq27000_battery_props[] = {
@@ -427,6 +446,7 @@ static struct {
 	BQ27XXX_PROP(BQ27541, bq27541_battery_props),
 	BQ27XXX_PROP(BQ27545, bq27545_battery_props),
 	BQ27XXX_PROP(BQ27421, bq27421_battery_props),
+	BQ27XXX_PROP(BQ27425, bq27421_battery_props),
 };
 
 static DEFINE_MUTEX(bq27xxx_list_lock);
@@ -682,6 +702,7 @@ static bool bq27xxx_battery_overtemp(struct bq27xxx_device_info *di, u16 flags)
 		return flags & (BQ27XXX_FLAG_OTC | BQ27XXX_FLAG_OTD);
 	case BQ27530:
 	case BQ27421:
+	case BQ27425:
 		return flags & BQ27XXX_FLAG_OT;
 	default:
 		return false;
@@ -693,7 +714,7 @@ static bool bq27xxx_battery_overtemp(struct bq27xxx_device_info *di, u16 flags)
  */
 static bool bq27xxx_battery_undertemp(struct bq27xxx_device_info *di, u16 flags)
 {
-	if (di->chip == BQ27530 || di->chip == BQ27421)
+	if (di->chip == BQ27530 || di->chip == BQ27421 || di->chip == BQ27425)
 		return flags & BQ27XXX_FLAG_UT;
 
 	return false;
diff --git a/drivers/power/supply/bq27xxx_battery_i2c.c b/drivers/power/supply/bq27xxx_battery_i2c.c
index 5c5c3a6..d7da535 100644
--- a/drivers/power/supply/bq27xxx_battery_i2c.c
+++ b/drivers/power/supply/bq27xxx_battery_i2c.c
@@ -159,7 +159,7 @@ static const struct i2c_device_id bq27xxx_i2c_id_table[] = {
 	{ "bq27742", BQ27541 },
 	{ "bq27545", BQ27545 },
 	{ "bq27421", BQ27421 },
-	{ "bq27425", BQ27421 },
+	{ "bq27425", BQ27425 },
 	{ "bq27441", BQ27421 },
 	{ "bq27621", BQ27421 },
 	{},
diff --git a/include/linux/power/bq27xxx_battery.h b/include/linux/power/bq27xxx_battery.h
index 92df553..4925478 100644
--- a/include/linux/power/bq27xxx_battery.h
+++ b/include/linux/power/bq27xxx_battery.h
@@ -9,7 +9,8 @@ enum bq27xxx_chip {
 	BQ27530, /* bq27530, bq27531 */
 	BQ27541, /* bq27541, bq27542, bq27546, bq27742 */
 	BQ27545, /* bq27545 */
-	BQ27421, /* bq27421, bq27425, bq27441, bq27621 */
+	BQ27421, /* bq27421, bq27441, bq27621 */
+	BQ27425, /* bq27425 */
 };
 
 /**
-- 
2.9.3

[PATCH v6 7/8] power: bq27xxx_battery: Add power_supply_battery_info support

[Liam Breck]

From: Matt Ranostay <matt@ranostay.consulting>

Previously there was no way to set chip registers in the event that the
defaults didn't match the battery in question.

BQ27xxx driver now calls power_supply_get_battery_info, checks the inputs,
and writes battery data to non-volatile memory.

Signed-off-by: Matt Ranostay <matt@ranostay.consulting>
Signed-off-by: Liam Breck <kernel@networkimprov.net>
---
 drivers/power/supply/bq27xxx_battery.c | 302 ++++++++++++++++++++++++++++++++-
 1 file changed, 301 insertions(+), 1 deletion(-)

diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
index 7475a5f..70d0142 100644
--- a/drivers/power/supply/bq27xxx_battery.c
+++ b/drivers/power/supply/bq27xxx_battery.c
@@ -37,6 +37,7 @@
  * http://www.ti.com/product/bq27621-g1
  */
 
+#include <linux/delay.h>
 #include <linux/device.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
@@ -452,6 +453,59 @@ static struct {
 static DEFINE_MUTEX(bq27xxx_list_lock);
 static LIST_HEAD(bq27xxx_battery_devices);
 
+#define BQ27XXX_TERM_V_MIN	2800
+#define BQ27XXX_TERM_V_MAX	3700
+
+#define BQ27XXX_BLOCK_DATA_CLASS	0x3E
+#define BQ27XXX_DATA_BLOCK		0x3F
+#define BQ27XXX_BLOCK_DATA		0x40
+#define BQ27XXX_BLOCK_DATA_CHECKSUM	0x60
+#define BQ27XXX_BLOCK_DATA_CONTROL	0x61
+#define BQ27XXX_SET_CFGUPDATE		0x13
+#define BQ27XXX_SOFT_RESET		0x42
+#define BQ27XXX_SUBCLASS_STATE_NVM	82
+
+struct bq27xxx_dm_buf {
+	u8 a[32];
+};
+
+struct bq27xxx_dm_reg {
+	unsigned int subclass_id;
+	unsigned int offset;
+	unsigned int bytes;
+	char *name;
+};
+
+#define BQ27XXX_DM_SUPPORTED(r) ( \
+	   r->subclass_id == BQ27XXX_SUBCLASS_STATE_NVM \
+	&& r->offset <= 30 \
+	&& r->bytes == 2 \
+)
+
+enum bq27xxx_dm_reg_id {
+	BQ27XXX_DM_DESIGN_CAPACITY = 0,
+	BQ27XXX_DM_DESIGN_ENERGY,
+	BQ27XXX_DM_TERMINATE_VOLTAGE,
+	BQ27XXX_DM_END,
+};
+
+static struct bq27xxx_dm_reg bq27425_dm_regs[] = {
+	[BQ27XXX_DM_DESIGN_CAPACITY] =
+		{ BQ27XXX_SUBCLASS_STATE_NVM, 12, 2, "design-capacity" },
+	[BQ27XXX_DM_DESIGN_ENERGY] =
+		{ BQ27XXX_SUBCLASS_STATE_NVM, 14, 2, "design-energy" },
+	[BQ27XXX_DM_TERMINATE_VOLTAGE] =
+		{ BQ27XXX_SUBCLASS_STATE_NVM, 18, 2, "terminate-voltage" },
+};
+
+static struct bq27xxx_dm_reg *bq27xxx_dm_regs[] = {
+	[BQ27425] = bq27425_dm_regs,
+};
+
+static unsigned int bq27xxx_unseal_keys[] = {
+	[BQ27425] = 0x04143672,
+};
+
 static int poll_interval_param_set(const char *val, const struct kernel_param *kp)
 {
 	struct bq27xxx_device_info *di;
@@ -496,6 +550,183 @@ static inline int bq27xxx_read(struct bq27xxx_device_info *di, int reg_index,
 	return di->bus.read(di, di->regs[reg_index], single);
 }
 
+static int bq27xxx_battery_set_seal_state(struct bq27xxx_device_info *di,
+					  bool state)
+{
+	unsigned int key = bq27xxx_unseal_keys[di->chip];
+	int ret;
+
+	if (state)
+		return di->bus.write(di, BQ27XXX_REG_CTRL, 0x20, false);
+
+	ret = di->bus.write(di, BQ27XXX_REG_CTRL, (key >> 16) & 0xffff, false);
+	if (ret < 0)
+		return ret;
+
+	return di->bus.write(di, BQ27XXX_REG_CTRL, key & 0xffff, false);
+}
+
+static int bq27xxx_battery_read_dm_block(struct bq27xxx_device_info *di,
+					 int subclass,
+					 struct bq27xxx_dm_buf *buf)
+{
+	int ret;
+
+	ret = di->bus.write(di, BQ27XXX_REG_CTRL, 0, false);
+	if (ret < 0)
+		return ret;
+
+	ret = di->bus.write(di, BQ27XXX_BLOCK_DATA_CONTROL, 0, true);
+	if (ret < 0)
+		return ret;
+
+	ret = di->bus.write(di, BQ27XXX_BLOCK_DATA_CLASS, subclass, true);
+	if (ret < 0)
+		return ret;
+
+	ret = di->bus.write(di, BQ27XXX_DATA_BLOCK, 0, true);
+	if (ret < 0)
+		return ret;
+
+	usleep_range(1000, 1500);
+
+	return di->bus.read_bulk(di, BQ27XXX_BLOCK_DATA, buf->a, sizeof buf->a);
+}
+
+static int bq27xxx_battery_print_config(struct bq27xxx_device_info *di)
+{
+	struct bq27xxx_dm_buf buf;
+	struct bq27xxx_dm_reg *reg = bq27xxx_dm_regs[di->chip];
+	int ret, i;
+
+	ret = bq27xxx_battery_read_dm_block(di, BQ27XXX_SUBCLASS_STATE_NVM,
+					    &buf);
+	if (ret < 0)
+		return ret;
+
+	for (i = 0; i < BQ27XXX_DM_END; i++, reg++) {
+		int val;
+
+		if (!BQ27XXX_DM_SUPPORTED(reg)) {
+			dev_warn(di->dev, "unsupported config register %s\n", reg->name);
+			continue;
+		}
+
+		val = be16_to_cpup((u16 *) &buf.a[reg->offset]);
+
+		dev_info(di->dev, "config register %s set at %d\n", reg->name, val);
+	}
+
+	return 0;
+}
+
+static bool bq27xxx_battery_update_dm_block(struct bq27xxx_device_info *di,
+					    struct bq27xxx_dm_buf *buf,
+					    enum bq27xxx_dm_reg_id reg_id,
+					    unsigned int val)
+{
+	struct bq27xxx_dm_reg *reg = &bq27xxx_dm_regs[di->chip][reg_id];
+	u16 *prev;
+
+	if (!BQ27XXX_DM_SUPPORTED(reg))
+		return false;
+
+	prev = (u16 *) &buf->a[reg->offset];
+
+	if (be16_to_cpup(prev) == val)
+		return false;
+
+	*prev = cpu_to_be16(val);
+
+	return true;
+}
+
+static u8 bq27xxx_battery_checksum(struct bq27xxx_dm_buf *buf)
+{
+	u16 sum = 0;
+	int i;
+
+	for (i = 0; i < sizeof buf->a; i++) {
+		sum += buf->a[i];
+		sum &= 0xff;
+	}
+
+	return 0xff - sum;
+}
+
+static int bq27xxx_battery_write_dm_block(struct bq27xxx_device_info *di,
+					  int subclass,
+					  struct bq27xxx_dm_buf *buf)
+{
+	int ret;
+
+	ret = di->bus.write(di, BQ27XXX_REG_CTRL, BQ27XXX_SET_CFGUPDATE, false);
+	if (ret < 0)
+		return ret;
+
+	ret = di->bus.write(di, BQ27XXX_BLOCK_DATA_CONTROL, 0, true);
+	if (ret < 0)
+		return ret;
+
+	ret = di->bus.write(di, BQ27XXX_BLOCK_DATA_CLASS, subclass, true);
+	if (ret < 0)
+		return ret;
+
+	ret = di->bus.write(di, BQ27XXX_DATA_BLOCK, 0, true);
+	if (ret < 0)
+		return ret;
+
+	ret = di->bus.write_bulk(di, BQ27XXX_BLOCK_DATA, buf->a, sizeof buf->a);
+	if (ret < 0)
+		return ret;
+
+	usleep_range(1000, 1500);
+
+	ret = di->bus.write(di, BQ27XXX_BLOCK_DATA_CHECKSUM,
+			    bq27xxx_battery_checksum(buf), true);
+	if (ret < 0)
+		return ret;
+
+	usleep_range(1000, 1500);
+
+	return di->bus.write(di, BQ27XXX_REG_CTRL, BQ27XXX_SOFT_RESET, false);
+}
+
+static int bq27xxx_battery_set_config(struct bq27xxx_device_info *di,
+				      struct power_supply_battery_info *info)
+{
+	struct bq27xxx_dm_buf buf;
+	int ret;
+
+	ret = bq27xxx_battery_read_dm_block(di, BQ27XXX_SUBCLASS_STATE_NVM,
+					    &buf);
+	if (ret < 0)
+		return ret;
+
+	if (info->charge_full_design_uah != -EINVAL
+	 && info->energy_full_design_uwh != -EINVAL) {
+		ret |= bq27xxx_battery_update_dm_block(di, &buf,
+					BQ27XXX_DM_DESIGN_CAPACITY,
+					info->charge_full_design_uah / 1000);
+		ret |= bq27xxx_battery_update_dm_block(di, &buf,
+					BQ27XXX_DM_DESIGN_ENERGY,
+					info->energy_full_design_uwh / 1000);
+	}
+
+	if (info->voltage_min_design_uv != -EINVAL)
+		ret |= bq27xxx_battery_update_dm_block(di, &buf,
+					BQ27XXX_DM_TERMINATE_VOLTAGE,
+					info->voltage_min_design_uv / 1000);
+
+	if (ret) {
+		dev_info(di->dev, "updating NVM settings\n");
+		return bq27xxx_battery_write_dm_block(di, BQ27XXX_SUBCLASS_STATE_NVM,
+						      &buf);
+	}
+
+	return 0;
+}
+
 /*
  * Return the battery State-of-Charge
  * Or < 0 if something fails.
@@ -757,6 +988,64 @@ static int bq27xxx_battery_read_health(struct bq27xxx_device_info *di)
 	return POWER_SUPPLY_HEALTH_GOOD;
 }
 
+void bq27xxx_battery_settings(struct bq27xxx_device_info *di)
+{
+	struct power_supply_battery_info info = {};
+
+	/* functions don't exist for writing data so abort */
+	if (!di->bus.write || !di->bus.write_bulk)
+		return;
+
+	/* no settings to be set for this chipset so abort */
+	if (!bq27xxx_dm_regs[di->chip])
+		return;
+
+	bq27xxx_battery_set_seal_state(di, false);
+
+	if (power_supply_get_battery_info(di->bat, &info) < 0)
+		goto out;
+
+	if (info.energy_full_design_uwh != info.charge_full_design_uah) {
+		if (info.energy_full_design_uwh == -EINVAL)
+			dev_warn(di->dev,
+				"missing battery:energy-full-design-microwatt-hours\n");
+		else if (info.charge_full_design_uah == -EINVAL)
+			dev_warn(di->dev,
+				"missing battery:charge-full-design-microamp-hours\n");
+	}
+
+	if (info.energy_full_design_uwh > 0x7fff * 1000) {
+		dev_err(di->dev, "invalid battery:energy-full-design-microwatt-hours %d\n",
+			info.energy_full_design_uwh);
+		info.energy_full_design_uwh = -EINVAL;
+	}
+
+	if (info.charge_full_design_uah > 0x7fff * 1000) {
+		dev_err(di->dev, "invalid battery:charge-full-design-microamp-hours %d\n",
+			info.charge_full_design_uah);
+		info.charge_full_design_uah = -EINVAL;
+	}
+
+	if ((info.voltage_min_design_uv < BQ27XXX_TERM_V_MIN * 1000
+	  || info.voltage_min_design_uv > BQ27XXX_TERM_V_MAX * 1000)
+	  && info.voltage_min_design_uv != -EINVAL) {
+		dev_err(di->dev, "invalid battery:voltage-min-design-microvolt %d\n",
+			info.voltage_min_design_uv);
+		info.voltage_min_design_uv = -EINVAL;
+	}
+
+	if ((info.energy_full_design_uwh == -EINVAL
+	  || info.charge_full_design_uah == -EINVAL)
+	  && info.voltage_min_design_uv  == -EINVAL)
+		goto out;
+
+	bq27xxx_battery_set_config(di, &info);
+
+out:
+	bq27xxx_battery_print_config(di);
+	bq27xxx_battery_set_seal_state(di, true);
+}
+
 void bq27xxx_battery_update(struct bq27xxx_device_info *di)
 {
 	struct bq27xxx_reg_cache cache = {0, };
@@ -1006,6 +1295,13 @@ static int bq27xxx_battery_get_property(struct power_supply *psy,
 	case POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN:
 		ret = bq27xxx_simple_value(di->charge_design_full, val);
 		break;
+	/*
+	 * TODO: Implement these to make registers set from
+	 * power_supply_battery_info visible in sysfs.
+	 */
+	case POWER_SUPPLY_PROP_ENERGY_FULL_DESIGN:
+	case POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN:
+		return -EINVAL;
 	case POWER_SUPPLY_PROP_CYCLE_COUNT:
 		ret = bq27xxx_simple_value(di->cache.cycle_count, val);
 		break;
@@ -1039,7 +1335,10 @@ static void bq27xxx_external_power_changed(struct power_supply *psy)
 int bq27xxx_battery_setup(struct bq27xxx_device_info *di)
 {
 	struct power_supply_desc *psy_desc;
-	struct power_supply_config psy_cfg = { .drv_data = di, };
+	struct power_supply_config psy_cfg = {
+		.of_node = di->dev->of_node,
+		.drv_data = di,
+	};
 
 	INIT_DELAYED_WORK(&di->work, bq27xxx_battery_poll);
 	mutex_init(&di->lock);
@@ -1064,6 +1363,7 @@ int bq27xxx_battery_setup(struct bq27xxx_device_info *di)
 
 	dev_info(di->dev, "support ver. %s enabled\n", DRIVER_VERSION);
 
+	bq27xxx_battery_settings(di);
 	bq27xxx_battery_update(di);
 
 	mutex_lock(&bq27xxx_list_lock);
-- 
2.9.3

Re: [PATCH v6 7/8] power: bq27xxx_battery: Add power_supply_battery_info support

[Liam Breck]

Hi Andrew,

Based on Sebastian's feedback on v1-4 of this patchset, I believe it
will go upstream. (He hasn't written anything on linux-pm since before
I posted v5 a week ago.)

Could we get your feedback on making this adaptable to the rest of the
BQ27 family? Specifically...

Is the op sequence in these functions common across the chips?

bq27xxx_battery_set_seal_state()
bq27xxx_battery_read_dm_block()
bq27xxx_battery_write_dm_block()

Should we move these constants to bq27xxx_regs[chip][name]?

/* writable registers */
#define BQ27XXX_DATA_CLASS             0x3E
#define BQ27XXX_DATA_BLOCK             0x3F
#define BQ27XXX_BLOCK_DATA             0x40
#define BQ27XXX_BLOCK_DATA_CHECKSUM    0x60
#define BQ27XXX_BLOCK_DATA_CONTROL     0x61

/* writable register inputs */
#define BQ27XXX_SET_CFGUPDATE          0x13
#define BQ27XXX_SOFT_RESET             0x42
#define BQ27XXX_SUBCLASS_STATE_NVM     82

If you are dead set against including this in bq27xxx_battery, we
could site the code in a companion module loaded by the I2C module.
Then the main module calls just one function.

Thanks,
Liam


On Fri, Feb 10, 2017 at 6:43 PM, Liam Breck <liam@networkimprov.net> wrote:
> From: Matt Ranostay <matt@ranostay.consulting>
>
> Previously there was no way to set chip registers in the event that the
> defaults didn't match the battery in question.
>
> BQ27xxx driver now calls power_supply_get_battery_info, checks the inputs,
> and writes battery data to non-volatile memory.
>
> Signed-off-by: Matt Ranostay <matt@ranostay.consulting>
> Signed-off-by: Liam Breck <kernel@networkimprov.net>
> ---
>  drivers/power/supply/bq27xxx_battery.c | 302 ++++++++++++++++++++++++++++++++-
>  1 file changed, 301 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
> index 7475a5f..70d0142 100644
> --- a/drivers/power/supply/bq27xxx_battery.c
> +++ b/drivers/power/supply/bq27xxx_battery.c
> @@ -37,6 +37,7 @@
>   * http://www.ti.com/product/bq27621-g1
>   */
>
> +#include <linux/delay.h>
>  #include <linux/device.h>
>  #include <linux/module.h>
>  #include <linux/mutex.h>
> @@ -452,6 +453,59 @@ static struct {
>  static DEFINE_MUTEX(bq27xxx_list_lock);
>  static LIST_HEAD(bq27xxx_battery_devices);
>
> +#define BQ27XXX_TERM_V_MIN     2800
> +#define BQ27XXX_TERM_V_MAX     3700
> +
> +#define BQ27XXX_BLOCK_DATA_CLASS       0x3E
> +#define BQ27XXX_DATA_BLOCK             0x3F
> +#define BQ27XXX_BLOCK_DATA             0x40
> +#define BQ27XXX_BLOCK_DATA_CHECKSUM    0x60
> +#define BQ27XXX_BLOCK_DATA_CONTROL     0x61
> +#define BQ27XXX_SET_CFGUPDATE          0x13
> +#define BQ27XXX_SOFT_RESET             0x42
> +#define BQ27XXX_SUBCLASS_STATE_NVM     82
> +
> +struct bq27xxx_dm_buf {
> +       u8 a[32];
> +};
> +
> +struct bq27xxx_dm_reg {
> +       unsigned int subclass_id;
> +       unsigned int offset;
> +       unsigned int bytes;
> +       char *name;
> +};
> +
> +#define BQ27XXX_DM_SUPPORTED(r) ( \
> +          r->subclass_id == BQ27XXX_SUBCLASS_STATE_NVM \
> +       && r->offset <= 30 \
> +       && r->bytes == 2 \
> +)
> +
> +enum bq27xxx_dm_reg_id {
> +       BQ27XXX_DM_DESIGN_CAPACITY = 0,
> +       BQ27XXX_DM_DESIGN_ENERGY,
> +       BQ27XXX_DM_TERMINATE_VOLTAGE,
> +       BQ27XXX_DM_END,
> +};
> +
> +static struct bq27xxx_dm_reg bq27425_dm_regs[] = {
> +       [BQ27XXX_DM_DESIGN_CAPACITY] =
> +               { BQ27XXX_SUBCLASS_STATE_NVM, 12, 2, "design-capacity" },
> +       [BQ27XXX_DM_DESIGN_ENERGY] =
> +               { BQ27XXX_SUBCLASS_STATE_NVM, 14, 2, "design-energy" },
> +       [BQ27XXX_DM_TERMINATE_VOLTAGE] =
> +               { BQ27XXX_SUBCLASS_STATE_NVM, 18, 2, "terminate-voltage" },
> +};
> +
> +static struct bq27xxx_dm_reg *bq27xxx_dm_regs[] = {
> +       [BQ27425] = bq27425_dm_regs,
> +};
> +
> +static unsigned int bq27xxx_unseal_keys[] = {
> +       [BQ27425] = 0x04143672,
> +};
> +
>  static int poll_interval_param_set(const char *val, const struct kernel_param *kp)
>  {
>         struct bq27xxx_device_info *di;
> @@ -496,6 +550,183 @@ static inline int bq27xxx_read(struct bq27xxx_device_info *di, int reg_index,
>         return di->bus.read(di, di->regs[reg_index], single);
>  }
>
> +static int bq27xxx_battery_set_seal_state(struct bq27xxx_device_info *di,
> +                                         bool state)
> +{
> +       unsigned int key = bq27xxx_unseal_keys[di->chip];
> +       int ret;
> +
> +       if (state)
> +               return di->bus.write(di, BQ27XXX_REG_CTRL, 0x20, false);
> +
> +       ret = di->bus.write(di, BQ27XXX_REG_CTRL, (key >> 16) & 0xffff, false);
> +       if (ret < 0)
> +               return ret;
> +
> +       return di->bus.write(di, BQ27XXX_REG_CTRL, key & 0xffff, false);
> +}
> +
> +static int bq27xxx_battery_read_dm_block(struct bq27xxx_device_info *di,
> +                                        int subclass,
> +                                        struct bq27xxx_dm_buf *buf)
> +{
> +       int ret;
> +
> +       ret = di->bus.write(di, BQ27XXX_REG_CTRL, 0, false);
> +       if (ret < 0)
> +               return ret;
> +
> +       ret = di->bus.write(di, BQ27XXX_BLOCK_DATA_CONTROL, 0, true);
> +       if (ret < 0)
> +               return ret;
> +
> +       ret = di->bus.write(di, BQ27XXX_BLOCK_DATA_CLASS, subclass, true);
> +       if (ret < 0)
> +               return ret;
> +
> +       ret = di->bus.write(di, BQ27XXX_DATA_BLOCK, 0, true);
> +       if (ret < 0)
> +               return ret;
> +
> +       usleep_range(1000, 1500);
> +
> +       return di->bus.read_bulk(di, BQ27XXX_BLOCK_DATA, buf->a, sizeof buf->a);
> +}
> +
> +static int bq27xxx_battery_print_config(struct bq27xxx_device_info *di)
> +{
> +       struct bq27xxx_dm_buf buf;
> +       struct bq27xxx_dm_reg *reg = bq27xxx_dm_regs[di->chip];
> +       int ret, i;
> +
> +       ret = bq27xxx_battery_read_dm_block(di, BQ27XXX_SUBCLASS_STATE_NVM,
> +                                           &buf);
> +       if (ret < 0)
> +               return ret;
> +
> +       for (i = 0; i < BQ27XXX_DM_END; i++, reg++) {
> +               int val;
> +
> +               if (!BQ27XXX_DM_SUPPORTED(reg)) {
> +                       dev_warn(di->dev, "unsupported config register %s\n", reg->name);
> +                       continue;
> +               }
> +
> +               val = be16_to_cpup((u16 *) &buf.a[reg->offset]);
> +
> +               dev_info(di->dev, "config register %s set at %d\n", reg->name, val);
> +       }
> +
> +       return 0;
> +}
> +
> +static bool bq27xxx_battery_update_dm_block(struct bq27xxx_device_info *di,
> +                                           struct bq27xxx_dm_buf *buf,
> +                                           enum bq27xxx_dm_reg_id reg_id,
> +                                           unsigned int val)
> +{
> +       struct bq27xxx_dm_reg *reg = &bq27xxx_dm_regs[di->chip][reg_id];
> +       u16 *prev;
> +
> +       if (!BQ27XXX_DM_SUPPORTED(reg))
> +               return false;
> +
> +       prev = (u16 *) &buf->a[reg->offset];
> +
> +       if (be16_to_cpup(prev) == val)
> +               return false;
> +
> +       *prev = cpu_to_be16(val);
> +
> +       return true;
> +}
> +
> +static u8 bq27xxx_battery_checksum(struct bq27xxx_dm_buf *buf)
> +{
> +       u16 sum = 0;
> +       int i;
> +
> +       for (i = 0; i < sizeof buf->a; i++) {
> +               sum += buf->a[i];
> +               sum &= 0xff;
> +       }
> +
> +       return 0xff - sum;
> +}
> +
> +static int bq27xxx_battery_write_dm_block(struct bq27xxx_device_info *di,
> +                                         int subclass,
> +                                         struct bq27xxx_dm_buf *buf)
> +{
> +       int ret;
> +
> +       ret = di->bus.write(di, BQ27XXX_REG_CTRL, BQ27XXX_SET_CFGUPDATE, false);
> +       if (ret < 0)
> +               return ret;
> +
> +       ret = di->bus.write(di, BQ27XXX_BLOCK_DATA_CONTROL, 0, true);
> +       if (ret < 0)
> +               return ret;
> +
> +       ret = di->bus.write(di, BQ27XXX_BLOCK_DATA_CLASS, subclass, true);
> +       if (ret < 0)
> +               return ret;
> +
> +       ret = di->bus.write(di, BQ27XXX_DATA_BLOCK, 0, true);
> +       if (ret < 0)
> +               return ret;
> +
> +       ret = di->bus.write_bulk(di, BQ27XXX_BLOCK_DATA, buf->a, sizeof buf->a);
> +       if (ret < 0)
> +               return ret;
> +
> +       usleep_range(1000, 1500);
> +
> +       ret = di->bus.write(di, BQ27XXX_BLOCK_DATA_CHECKSUM,
> +                           bq27xxx_battery_checksum(buf), true);
> +       if (ret < 0)
> +               return ret;
> +
> +       usleep_range(1000, 1500);
> +
> +       return di->bus.write(di, BQ27XXX_REG_CTRL, BQ27XXX_SOFT_RESET, false);
> +}
> +
> +static int bq27xxx_battery_set_config(struct bq27xxx_device_info *di,
> +                                     struct power_supply_battery_info *info)
> +{
> +       struct bq27xxx_dm_buf buf;
> +       int ret;
> +
> +       ret = bq27xxx_battery_read_dm_block(di, BQ27XXX_SUBCLASS_STATE_NVM,
> +                                           &buf);
> +       if (ret < 0)
> +               return ret;
> +
> +       if (info->charge_full_design_uah != -EINVAL
> +        && info->energy_full_design_uwh != -EINVAL) {
> +               ret |= bq27xxx_battery_update_dm_block(di, &buf,
> +                                       BQ27XXX_DM_DESIGN_CAPACITY,
> +                                       info->charge_full_design_uah / 1000);
> +               ret |= bq27xxx_battery_update_dm_block(di, &buf,
> +                                       BQ27XXX_DM_DESIGN_ENERGY,
> +                                       info->energy_full_design_uwh / 1000);
> +       }
> +
> +       if (info->voltage_min_design_uv != -EINVAL)
> +               ret |= bq27xxx_battery_update_dm_block(di, &buf,
> +                                       BQ27XXX_DM_TERMINATE_VOLTAGE,
> +                                       info->voltage_min_design_uv / 1000);
> +
> +       if (ret) {
> +               dev_info(di->dev, "updating NVM settings\n");
> +               return bq27xxx_battery_write_dm_block(di, BQ27XXX_SUBCLASS_STATE_NVM,
> +                                                     &buf);
> +       }
> +
> +       return 0;
> +}
> +
>  /*
>   * Return the battery State-of-Charge
>   * Or < 0 if something fails.
> @@ -757,6 +988,64 @@ static int bq27xxx_battery_read_health(struct bq27xxx_device_info *di)
>         return POWER_SUPPLY_HEALTH_GOOD;
>  }
>
> +void bq27xxx_battery_settings(struct bq27xxx_device_info *di)
> +{
> +       struct power_supply_battery_info info = {};
> +
> +       /* functions don't exist for writing data so abort */
> +       if (!di->bus.write || !di->bus.write_bulk)
> +               return;
> +
> +       /* no settings to be set for this chipset so abort */
> +       if (!bq27xxx_dm_regs[di->chip])
> +               return;
> +
> +       bq27xxx_battery_set_seal_state(di, false);
> +
> +       if (power_supply_get_battery_info(di->bat, &info) < 0)
> +               goto out;
> +
> +       if (info.energy_full_design_uwh != info.charge_full_design_uah) {
> +               if (info.energy_full_design_uwh == -EINVAL)
> +                       dev_warn(di->dev,
> +                               "missing battery:energy-full-design-microwatt-hours\n");
> +               else if (info.charge_full_design_uah == -EINVAL)
> +                       dev_warn(di->dev,
> +                               "missing battery:charge-full-design-microamp-hours\n");
> +       }
> +
> +       if (info.energy_full_design_uwh > 0x7fff * 1000) {
> +               dev_err(di->dev, "invalid battery:energy-full-design-microwatt-hours %d\n",
> +                       info.energy_full_design_uwh);
> +               info.energy_full_design_uwh = -EINVAL;
> +       }
> +
> +       if (info.charge_full_design_uah > 0x7fff * 1000) {
> +               dev_err(di->dev, "invalid battery:charge-full-design-microamp-hours %d\n",
> +                       info.charge_full_design_uah);
> +               info.charge_full_design_uah = -EINVAL;
> +       }
> +
> +       if ((info.voltage_min_design_uv < BQ27XXX_TERM_V_MIN * 1000
> +         || info.voltage_min_design_uv > BQ27XXX_TERM_V_MAX * 1000)
> +         && info.voltage_min_design_uv != -EINVAL) {
> +               dev_err(di->dev, "invalid battery:voltage-min-design-microvolt %d\n",
> +                       info.voltage_min_design_uv);
> +               info.voltage_min_design_uv = -EINVAL;
> +       }
> +
> +       if ((info.energy_full_design_uwh == -EINVAL
> +         || info.charge_full_design_uah == -EINVAL)
> +         && info.voltage_min_design_uv  == -EINVAL)
> +               goto out;
> +
> +       bq27xxx_battery_set_config(di, &info);
> +
> +out:
> +       bq27xxx_battery_print_config(di);
> +       bq27xxx_battery_set_seal_state(di, true);
> +}
> +
>  void bq27xxx_battery_update(struct bq27xxx_device_info *di)
>  {
>         struct bq27xxx_reg_cache cache = {0, };
> @@ -1006,6 +1295,13 @@ static int bq27xxx_battery_get_property(struct power_supply *psy,
>         case POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN:
>                 ret = bq27xxx_simple_value(di->charge_design_full, val);
>                 break;
> +       /*
> +        * TODO: Implement these to make registers set from
> +        * power_supply_battery_info visible in sysfs.
> +        */
> +       case POWER_SUPPLY_PROP_ENERGY_FULL_DESIGN:
> +       case POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN:
> +               return -EINVAL;
>         case POWER_SUPPLY_PROP_CYCLE_COUNT:
>                 ret = bq27xxx_simple_value(di->cache.cycle_count, val);
>                 break;
> @@ -1039,7 +1335,10 @@ static void bq27xxx_external_power_changed(struct power_supply *psy)
>  int bq27xxx_battery_setup(struct bq27xxx_device_info *di)
>  {
>         struct power_supply_desc *psy_desc;
> -       struct power_supply_config psy_cfg = { .drv_data = di, };
> +       struct power_supply_config psy_cfg = {
> +               .of_node = di->dev->of_node,
> +               .drv_data = di,
> +       };
>
>         INIT_DELAYED_WORK(&di->work, bq27xxx_battery_poll);
>         mutex_init(&di->lock);
> @@ -1064,6 +1363,7 @@ int bq27xxx_battery_setup(struct bq27xxx_device_info *di)
>
>         dev_info(di->dev, "support ver. %s enabled\n", DRIVER_VERSION);
>
> +       bq27xxx_battery_settings(di);
>         bq27xxx_battery_update(di);
>
>         mutex_lock(&bq27xxx_list_lock);
> --
> 2.9.3
>

Re: [PATCH v6 7/8] power: bq27xxx_battery: Add power_supply_battery_info support

[Liam Breck]

On Sun, Feb 12, 2017 at 6:32 PM, Liam Breck <liam@networkimprov.net> wrote:
> Hi Andrew,
>
> Based on Sebastian's feedback on v1-4 of this patchset, I believe it
> will go upstream. (He hasn't written anything on linux-pm since before
> I posted v5 a week ago.)
>
> Could we get your feedback on making this adaptable to the rest of the
> BQ27 family? Specifically...
>
> Is the op sequence in these functions common across the chips?
>
> bq27xxx_battery_set_seal_state()
> bq27xxx_battery_read_dm_block()
> bq27xxx_battery_write_dm_block()
>
> Should we move these constants to bq27xxx_regs[chip][name]?
>
> /* writable registers */
> #define BQ27XXX_DATA_CLASS             0x3E
> #define BQ27XXX_DATA_BLOCK             0x3F
> #define BQ27XXX_BLOCK_DATA             0x40
> #define BQ27XXX_BLOCK_DATA_CHECKSUM    0x60
> #define BQ27XXX_BLOCK_DATA_CONTROL     0x61
>
> /* writable register inputs */
> #define BQ27XXX_SET_CFGUPDATE          0x13
> #define BQ27XXX_SOFT_RESET             0x42
> #define BQ27XXX_SUBCLASS_STATE_NVM     82

I dug into bqtool:gauge.c. I learned a couple things which I've added
to the patch, e.g. retrieve and compare checksum after read_dm_block.

Re command ids and params, only subclass, field offsets, and unseal
key differ among chips, which I planned for. However the driver's
current 9 enumerated chips won't accommodate all the diff unseal keys.
But I shall leave that for others to solve.

Re op sequence, bqtool apparently uses set_cfgupdate for the 441 & 621
chips, but not others, whereas the 425 manual implies that it's nec.
And bqtool uses exit_cfgupdate where 425 manual indicates soft_reset.
See open/close_dm_rom (441 & 621) vs. open/close_dm_flash. We need
some guidance on that issue.

Bqtool also uses loops to poll for begin & end of cfgupdate mode, and
to retry read/write_dm_block with increasing delays. We could add that
if nec...

http://git.ti.com/bms-linux/bqtool/blobs/master/gauge.c

Thanks!

Re: [PATCH v6 7/8] power: bq27xxx_battery: Add power_supply_battery_info support

[Liam Breck]

On Wed, Feb 15, 2017 at 11:45 AM, Liam Breck <liam@networkimprov.net> wrote:
> On Sun, Feb 12, 2017 at 6:32 PM, Liam Breck <liam@networkimprov.net> wrote:
>> Hi Andrew,
>>
>> Based on Sebastian's feedback on v1-4 of this patchset, I believe it
>> will go upstream. (He hasn't written anything on linux-pm since before
>> I posted v5 a week ago.)
>>
>> Could we get your feedback on making this adaptable to the rest of the
>> BQ27 family? Specifically...
>>
>> Is the op sequence in these functions common across the chips?
>>
>> bq27xxx_battery_set_seal_state()
>> bq27xxx_battery_read_dm_block()
>> bq27xxx_battery_write_dm_block()
>>
>> Should we move these constants to bq27xxx_regs[chip][name]?
>>
>> /* writable registers */
>> #define BQ27XXX_DATA_CLASS             0x3E
>> #define BQ27XXX_DATA_BLOCK             0x3F
>> #define BQ27XXX_BLOCK_DATA             0x40
>> #define BQ27XXX_BLOCK_DATA_CHECKSUM    0x60
>> #define BQ27XXX_BLOCK_DATA_CONTROL     0x61
>>
>> /* writable register inputs */
>> #define BQ27XXX_SET_CFGUPDATE          0x13
>> #define BQ27XXX_SOFT_RESET             0x42
>> #define BQ27XXX_SUBCLASS_STATE_NVM     82
>
> I dug into bqtool:gauge.c. I learned a couple things which I've added
> to the patch, e.g. retrieve and compare checksum after read_dm_block.
>
> Re command ids and params, only subclass, field offsets, and unseal
> key differ among chips, which I planned for. However the driver's
> current 9 enumerated chips won't accommodate all the diff unseal keys.
> But I shall leave that for others to solve.
>
> Re op sequence, bqtool apparently uses set_cfgupdate for the 441 & 621
> chips, but not others, whereas the 425 manual implies that it's nec.
> And bqtool uses exit_cfgupdate where 425 manual indicates soft_reset.
> See open/close_dm_rom (441 & 621) vs. open/close_dm_flash. We need
> some guidance on that issue.

I checked datasheets for one chip in each family defined in bqtool,
and all chips in the 421-621 & 425 families. Only the latter 2
families use set_cfgupdate & soft_reset. I've rev'd patch accordingly;
will post this evening.

http://git.ti.com/bms-linux/bqtool/blobs/master/gauge.c#line732

Bqtool may have a bug on 425 since it doesn't set_cfgupdate in that
case, which the manual says is required.

[PATCH v6 8/8] power: bq27xxx_battery_i2c: Add I2C bulk read/write functions

[Liam Breck]

From: Matt Ranostay <matt@ranostay.consulting>

write(), read_bulk(), write_bulk() are required by bq27xxx_battery
devicetree code.

Signed-off-by: Matt Ranostay <matt@ranostay.consulting>
Signed-off-by: Liam Breck <kernel@networkimprov.net>
---
 drivers/power/supply/bq27xxx_battery_i2c.c | 65 ++++++++++++++++++++++++++++++
 1 file changed, 65 insertions(+)

diff --git a/drivers/power/supply/bq27xxx_battery_i2c.c b/drivers/power/supply/bq27xxx_battery_i2c.c
index d7da535..c1638ad 100644
--- a/drivers/power/supply/bq27xxx_battery_i2c.c
+++ b/drivers/power/supply/bq27xxx_battery_i2c.c
@@ -68,6 +68,67 @@ static int bq27xxx_battery_i2c_read(struct bq27xxx_device_info *di, u8 reg,
 	return ret;
 }
 
+static int bq27xxx_battery_i2c_write(struct bq27xxx_device_info *di, u8 reg,
+				     int value, bool single)
+{
+	struct i2c_client *client = to_i2c_client(di->dev);
+	struct i2c_msg msg;
+	unsigned char data[4];
+
+	if (!client->adapter)
+		return -ENODEV;
+
+	data[0] = reg;
+	if (single) {
+		data[1] = (unsigned char) value;
+		msg.len = 2;
+	} else {
+		put_unaligned_le16(value, &data[1]);
+		msg.len = 3;
+	}
+
+	msg.buf = data;
+	msg.addr = client->addr;
+	msg.flags = 0;
+
+	return i2c_transfer(client->adapter, &msg, 1) == 1 ?
+		0 : -EINVAL;
+}
+
+static int bq27xxx_battery_i2c_bulk_read(struct bq27xxx_device_info *di, u8 reg,
+					 u8 *data, int len)
+{
+	struct i2c_client *client = to_i2c_client(di->dev);
+
+	if (!client->adapter)
+		return -ENODEV;
+
+	return i2c_smbus_read_i2c_block_data(client, reg, len, data) == len ?
+		0 : -EINVAL;
+}
+
+static int bq27xxx_battery_i2c_bulk_write(struct bq27xxx_device_info *di,
+					  u8 reg, u8 *data, int len)
+{
+	struct i2c_client *client = to_i2c_client(di->dev);
+	struct i2c_msg msg;
+	u8 buf[33];
+
+	if (!client->adapter)
+		return -ENODEV;
+
+	buf[0] = reg;
+	memcpy(&buf[1], data, len);
+
+	msg.buf = buf;
+	msg.addr = client->addr;
+	msg.flags = 0;
+	msg.len = len + 1;
+
+	return i2c_transfer(client->adapter, &msg, 1) == 1 ?
+		0 : -EINVAL;
+}
+
 static int bq27xxx_battery_i2c_probe(struct i2c_client *client,
 				     const struct i2c_device_id *id)
 {
@@ -95,7 +156,11 @@ static int bq27xxx_battery_i2c_probe(struct i2c_client *client,
 	di->dev = &client->dev;
 	di->chip = id->driver_data;
 	di->name = name;
+
 	di->bus.read = bq27xxx_battery_i2c_read;
+	di->bus.write = bq27xxx_battery_i2c_write;
+	di->bus.read_bulk = bq27xxx_battery_i2c_bulk_read;
+	di->bus.write_bulk = bq27xxx_battery_i2c_bulk_write;
 
 	ret = bq27xxx_battery_setup(di);
 	if (ret)
-- 
2.9.3