[PATCH v11 0/10] devicetree battery support and client bq27xxx_battery

[PATCH v11 0/10] 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 RAM/NVM for params
  essential to correct operation: energy-full-design-microwatt-hours,
  charge-full-design-microamp-hours, voltage-min-design-microvolt

Changes in v11:
  power_supply_core:
*   switch to compatible = "simple-battery"
*   add docs to power_supply_class.txt
  Documentation/devicetree/.../battery.txt:
*   add description, drop refs to Linux, "simple-battery"
  bq27xxx_battery:
*   reset flash chips after DM update
*   add bq27xxx_write/xfer()
*   polishing from Andrew's feedback
*   new patch to clean up error reporting
*   new patch to consolidate duplicate register/property arrays

Changes in v10:
  bq27xxx_battery:
*   pass actual chip ID into _setup()
*   add di->unseal_key & di->dm_regs; drop static arrays
*   support bq27425, 441, 621

Changes in v9:
  bq27xxx_battery:
*   fix set_cfgupdate()
*   support bq27500, 545, 421; defer others
*   drop print_dm_blocks() patch
*   minor polishing
  Documentation/devicetree/.../battery.txt:
*   describe rationale for enum power_supply_property names

Changes in v8:
  bq27xxx_battery:
*   wait on flag after set_cfgupdate & soft_reset
*   drop print_config(), report status in update_dm_block()
*   clarify error messages
*   cleanup from Andrew's feedback; minor polishing

Changes in v7:
  bq27xxx_battery:
*   support chips where terminate_voltage & design_* live in separate blocks
*   draft support for 421, 441, 621 chips
*   new patch to log chip memory fields
*   report bus I/O errors; return error code in bq27xxx_battery_i2c
*   verify checksum in read_dm_block()
*   use set_cfgupdate only if chip provides it, soft_reset on I/O error
*   block_data_control=0 only in write_dm_block()
*   note toxic code from TI bqtool in write_dm_block()
*   lots of functionally neutral polishing
  Documentation/devicetree/.../battery.txt:
*   mention power_supply_get_battery_info()

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

Liam Breck (8):
  devicetree: power: Add battery.txt
  devicetree: power: bq27xxx: Add monitored-battery documentation
  power: power_supply: Add power_supply_battery_info and API
  power: bq27xxx_battery: Make error reporting consistent
  power: bq27xxx_battery: Define access methods to write chip registers
  power: bq27xxx_battery: Keep track of specific chip id
  power: bq27xxx_battery: Add power_supply_battery_info support
  power: bq27xxx_battery: Remove duplicate register arrays

Matt Ranostay (2):
  devicetree: property-units: Add uWh and uAh units
  power: bq27xxx_battery_i2c: Add I2C bulk read/write functions

 .../devicetree/bindings/power/supply/battery.txt   |  43 ++
 .../devicetree/bindings/power/supply/bq27xxx.txt   |  11 +-
 .../devicetree/bindings/property-units.txt         |   2 +
 Documentation/power/power_supply_class.txt         |  12 +
 drivers/power/supply/bq27xxx_battery.c             | 707 +++++++++++++++++----
 drivers/power/supply/bq27xxx_battery_i2c.c         |  98 ++-
 drivers/power/supply/power_supply_core.c           |  45 ++
 include/linux/power/bq27xxx_battery.h              |  26 +-
 include/linux/power_supply.h                       |  18 +
 9 files changed, 806 insertions(+), 156 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/power/supply/battery.txt

-- 
2.9.3

[PATCH v11 01/10] devicetree: power: Add battery.txt

[Liam Breck]

From: Liam Breck <kernel-RYWXG+zxWwBdeoIcmNTgJF6hYfS7NtTn@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.

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   | 43 ++++++++++++++++++++++
 1 file changed, 43 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..53a68c0
--- /dev/null
+++ b/Documentation/devicetree/bindings/power/supply/battery.txt
@@ -0,0 +1,43 @@
+Battery Characteristics
+
+The devicetree battery node provides static battery characteristics. 
+In smart batteries, these are typically stored in non-volatile memory 
+on a fuel gauge chip. The battery node should be used where there is 
+no appropriate non-volatile memory, or it is unprogrammed/incorrect.
+
+Required Properties:
+ - compatible: Must be "simple-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
+
+Battery properties are named, where possible, for the corresponding 
+elements in enum power_supply_property, defined in
+https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/power_supply.h#n86
+
+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 = "simple-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

--
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 v11 01/10] devicetree: power: Add battery.txt

[Rob Herring]

On Mon, Mar 20, 2017 at 02:43:26AM -0700, Liam Breck wrote:
> From: Liam Breck <kernel@networkimprov.net>
> 
> Documentation of static battery characteristics that can be defined
> for batteries which cannot self-identify. This information is required
> by fuel-gauge and charger chips for proper handling of the battery.
> 
> Cc: Rob Herring <robh@kernel.org>
> Cc: devicetree@vger.kernel.org
> Signed-off-by: Matt Ranostay <matt@ranostay.consulting>
> Signed-off-by: Liam Breck <kernel@networkimprov.net>
> ---
>  .../devicetree/bindings/power/supply/battery.txt   | 43 ++++++++++++++++++++++
>  1 file changed, 43 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/power/supply/battery.txt

While "simple" is not generally something I like to see in a compatible 
string, we've beat this one to death.

Acked-by: Rob Herring <robh@kernel.org>

Re: [PATCH v11 01/10] devicetree: power: Add battery.txt

[Liam Breck]

Hi Rob,

On Fri, Mar 24, 2017 at 8:55 AM, Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> On Mon, Mar 20, 2017 at 02:43:26AM -0700, Liam Breck wrote:
>> From: Liam Breck <kernel-RYWXG+zxWwBdeoIcmNTgJF6hYfS7NtTn@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.
>>
>> 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   | 43 ++++++++++++++++++++++
>>  1 file changed, 43 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/power/supply/battery.txt
>
> While "simple" is not generally something I like to see in a compatible
> string, we've beat this one to death.
>
> Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>

Thanks. Are you in agreement with Sebastian on the following, or is
this OK to include?

On Thu, Mar 23, 2017 at 3:20 AM, Sebastian Reichel <sre-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
>> +Battery properties are named, where possible, for the corresponding
>> +elements in enum power_supply_property, defined in
>> +https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/power_supply.h#n86
>
>The above paragraph does not belong into DT bindings.
--
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 v11 01/10] devicetree: power: Add battery.txt

[Sebastian Reichel]

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

Hi,

On Mon, Mar 20, 2017 at 02:43:26AM -0700, Liam Breck wrote:
> From: Liam Breck <kernel-RYWXG+zxWwBdeoIcmNTgJF6hYfS7NtTn@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.
> 
> 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   | 43 ++++++++++++++++++++++
>  1 file changed, 43 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..53a68c0
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/supply/battery.txt
> @@ -0,0 +1,43 @@
> +Battery Characteristics
> +
> +The devicetree battery node provides static battery characteristics. 
> +In smart batteries, these are typically stored in non-volatile memory 
> +on a fuel gauge chip. The battery node should be used where there is 
> +no appropriate non-volatile memory, or it is unprogrammed/incorrect.
> +
> +Required Properties:
> + - compatible: Must be "simple-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
> +
> +Battery properties are named, where possible, for the corresponding 
> +elements in enum power_supply_property, defined in
> +https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/power_supply.h#n86

The above paragraph does not belong into DT bindings. Apart
from that

Acked-by: Sebastian Reichel <sre-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>

FYI: I will wait for Acked-by from Rob Herring on this patch.

-- Sebastian

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

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

Re: [PATCH v11 01/10] devicetree: power: Add battery.txt

[Liam Breck]

On Thu, Mar 23, 2017 at 3:20 AM, Sebastian Reichel <sre@kernel.org> wrote:
> Hi,
>
> On Mon, Mar 20, 2017 at 02:43:26AM -0700, Liam Breck wrote:
>> From: Liam Breck <kernel@networkimprov.net>
>>
>> Documentation of static battery characteristics that can be defined
>> for batteries which cannot self-identify. This information is required
>> by fuel-gauge and charger chips for proper handling of the battery.
>>
>> Cc: Rob Herring <robh@kernel.org>
>> Cc: devicetree@vger.kernel.org
>> Signed-off-by: Matt Ranostay <matt@ranostay.consulting>
>> Signed-off-by: Liam Breck <kernel@networkimprov.net>
>> ---
>>  .../devicetree/bindings/power/supply/battery.txt   | 43 ++++++++++++++++++++++
>>  1 file changed, 43 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..53a68c0
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/power/supply/battery.txt
>> @@ -0,0 +1,43 @@
>> +Battery Characteristics
>> +
>> +The devicetree battery node provides static battery characteristics.
>> +In smart batteries, these are typically stored in non-volatile memory
>> +on a fuel gauge chip. The battery node should be used where there is
>> +no appropriate non-volatile memory, or it is unprogrammed/incorrect.
>> +
>> +Required Properties:
>> + - compatible: Must be "simple-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
>> +
>> +Battery properties are named, where possible, for the corresponding
>> +elements in enum power_supply_property, defined in
>> +https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/power_supply.h#n86
>
> The above paragraph does not belong into DT bindings. Apart
> from that

How then should I indicate that there is a method to the madness of
the above names? More information in the docs is helpful than less!

> Acked-by: Sebastian Reichel <sre@kernel.org>
>
> FYI: I will wait for Acked-by from Rob Herring on this patch.
>
> -- Sebastian
>
>> +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 = "simple-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 v11 01/10] devicetree: power: Add battery.txt

[Sebastian Reichel]

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

Hi,

On Thu, Mar 23, 2017 at 03:30:42AM -0700, Liam Breck wrote:
> On Thu, Mar 23, 2017 at 3:20 AM, Sebastian Reichel <sre@kernel.org> wrote:
> > Hi,
> >
> > On Mon, Mar 20, 2017 at 02:43:26AM -0700, Liam Breck wrote:
> >> From: Liam Breck <kernel@networkimprov.net>
> >>
> >> Documentation of static battery characteristics that can be defined
> >> for batteries which cannot self-identify. This information is required
> >> by fuel-gauge and charger chips for proper handling of the battery.
> >>
> >> Cc: Rob Herring <robh@kernel.org>
> >> Cc: devicetree@vger.kernel.org
> >> Signed-off-by: Matt Ranostay <matt@ranostay.consulting>
> >> Signed-off-by: Liam Breck <kernel@networkimprov.net>
> >> ---
> >>  .../devicetree/bindings/power/supply/battery.txt   | 43 ++++++++++++++++++++++
> >>  1 file changed, 43 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..53a68c0
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/power/supply/battery.txt
> >> @@ -0,0 +1,43 @@
> >> +Battery Characteristics
> >> +
> >> +The devicetree battery node provides static battery characteristics.
> >> +In smart batteries, these are typically stored in non-volatile memory
> >> +on a fuel gauge chip. The battery node should be used where there is
> >> +no appropriate non-volatile memory, or it is unprogrammed/incorrect.
> >> +
> >> +Required Properties:
> >> + - compatible: Must be "simple-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
> >> +
> >> +Battery properties are named, where possible, for the corresponding
> >> +elements in enum power_supply_property, defined in
> >> +https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/power_supply.h#n86
> >
> > The above paragraph does not belong into DT bindings. Apart
> > from that
> >
> > Acked-by: Sebastian Reichel <sre@kernel.org>
> 
> How then should I indicate that there is a method to the madness of
> the above names? More information in the docs is helpful than less!

You don't. This does not belong into the DT binding document. DT
binding documents are _not_ Linux FW API. I thought we were through
this already. FWIW if anything is not named like the power-supply
subsystem names it, then that's not nice, but ok. Also if anybody
adds stuff he will 

1. send patches to the power-supply subsystem maintainer (me at
   the moment), who can reject patches.
2. implement the parsing in the driver, which asks for power-supply
   naming to be used if possible.

-- Sebastian

> > FYI: I will wait for Acked-by from Rob Herring on this patch.
> >
> > -- Sebastian
> >
> >> +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 = "simple-battery";
> >> +             voltage-min-design-microvolt = <3200000>;
> >> +             energy-full-design-microwatt-hours = <5290000>;
> >> +             charge-full-design-microamp-hours = <1430000>;
> >> +     };
> >> +
> >> +     charger: charger@11 {
> >> +             ....
> >> +             monitored-battery = <&bat>;
> >> +             ...
> >> +     };
> >> +
> >> +     fuel_gauge: fuel-gauge@22 {
> >> +             ....
> >> +             monitored-battery = <&bat>;
> >> +             ...
> >> +     };
> >> --
> >> 2.9.3
> >>

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

Re: [PATCH v11 01/10] devicetree: power: Add battery.txt

[Liam Breck]

[+CC Tony Lindgren]

Rob & Sebastian & Tony & Andrew,

With this binding we allow a DT to hard-code battery characteristics.
But mainline dts files for boards & boxes really should NOT include
immutable battery properties unless the battery is inseparable from
the electronics. We want to prevent unpredictable behavior due to
incorrect DT properties after a user changes a battery to a different
type.

It's OK for device vendors to hard-code battery characteristics in DT,
but mainline dts is another matter.

If this is agreed, how should we document it?


On Mon, Mar 20, 2017 at 2:43 AM, Liam Breck <liam-RYWXG+zxWwBdeoIcmNTgJF6hYfS7NtTn@public.gmane.org> wrote:
> From: Liam Breck <kernel-RYWXG+zxWwBdeoIcmNTgJF6hYfS7NtTn@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.
>
> 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   | 43 ++++++++++++++++++++++
>  1 file changed, 43 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..53a68c0
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/supply/battery.txt
> @@ -0,0 +1,43 @@
> +Battery Characteristics
> +
> +The devicetree battery node provides static battery characteristics.
> +In smart batteries, these are typically stored in non-volatile memory
> +on a fuel gauge chip. The battery node should be used where there is
> +no appropriate non-volatile memory, or it is unprogrammed/incorrect.
> +
> +Required Properties:
> + - compatible: Must be "simple-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
> +
> +Battery properties are named, where possible, for the corresponding
> +elements in enum power_supply_property, defined in
> +https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/power_supply.h#n86
> +
> +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 = "simple-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
>
--
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 v11 01/10] devicetree: power: Add battery.txt

[Sebastian Reichel]

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

Hi,

On Fri, Apr 07, 2017 at 12:23:29PM -0700, Liam Breck wrote:
> [+CC Tony Lindgren]
> 
> Rob & Sebastian & Tony & Andrew,
> 
> With this binding we allow a DT to hard-code battery characteristics.
> But mainline dts files for boards & boxes really should NOT include
> immutable battery properties unless the battery is inseparable from
> the electronics. We want to prevent unpredictable behavior due to
> incorrect DT properties after a user changes a battery to a different
> type.
> 
> It's OK for device vendors to hard-code battery characteristics in DT,
> but mainline dts is another matter.
> 
> If this is agreed, how should we document it?

That's not a problem of mainline vs vendor. The binding may only be
used for batteries, that cannot be exchanged with a different type.
That's why I originally suggested the "fixed-battery" name. IMHO
it would be ok to use the binding also for DTS files distributed
with the mainline kernel as long as the described device provides
some kind of protection against using batteries of a different type.

For example I would feel ok adding a simple-battery node for the
Droid 4 (which probably does not need one), since the battery has
a custom form-factor, is screwed to the device and has a custom
connector. So while it can be replaced, the new battery would have
the same technical specs.

I suggest to add something similar as the following to the binding:

Using incorrect battery parameters is a SAFETY HAZARD - If used
incorrectly, Lithium based batteries can explode. You should be
really sure, that all provided values are correct. Also if the
battery in your device can be replaced with a different battery
(without major hacks), you may not use this binding for safety
reasons.

-- Sebastian

> On Mon, Mar 20, 2017 at 2:43 AM, Liam Breck <liam-RYWXG+zxWwBdeoIcmNTgJF6hYfS7NtTn@public.gmane.org> wrote:
> > From: Liam Breck <kernel-RYWXG+zxWwBdeoIcmNTgJF6hYfS7NtTn@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.
> >
> > 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   | 43 ++++++++++++++++++++++
> >  1 file changed, 43 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..53a68c0
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/power/supply/battery.txt
> > @@ -0,0 +1,43 @@
> > +Battery Characteristics
> > +
> > +The devicetree battery node provides static battery characteristics.
> > +In smart batteries, these are typically stored in non-volatile memory
> > +on a fuel gauge chip. The battery node should be used where there is
> > +no appropriate non-volatile memory, or it is unprogrammed/incorrect.
> > +
> > +Required Properties:
> > + - compatible: Must be "simple-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
> > +
> > +Battery properties are named, where possible, for the corresponding
> > +elements in enum power_supply_property, defined in
> > +https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/power_supply.h#n86
> > +
> > +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 = "simple-battery";
> > +               voltage-min-design-microvolt = <3200000>;
> > +               energy-full-design-microwatt-hours = <5290000>;
> > +               charge-full-design-microamp-hours = <1430000>;
> > +       };
> > +
> > +       charger: charger@11 {
> > +               ....
> > +               monitored-battery = <&bat>;
> > +               ...
> > +       };
> > +
> > +       fuel_gauge: fuel-gauge@22 {
> > +               ....
> > +               monitored-battery = <&bat>;
> > +               ...
> > +       };
> > --
> > 2.9.3
> >

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

Re: [PATCH v11 01/10] devicetree: power: Add battery.txt

[Sebastian Reichel]

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

Hi,

On Mon, Mar 20, 2017 at 02:43:26AM -0700, Liam Breck wrote:
> From: Liam Breck <kernel-RYWXG+zxWwBdeoIcmNTgJF6hYfS7NtTn@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.
> 
> 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   | 43 ++++++++++++++++++++++
>  1 file changed, 43 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..53a68c0
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/supply/battery.txt
> @@ -0,0 +1,43 @@
> +Battery Characteristics
> +
> +The devicetree battery node provides static battery characteristics. 
> +In smart batteries, these are typically stored in non-volatile memory 
> +on a fuel gauge chip. The battery node should be used where there is 
> +no appropriate non-volatile memory, or it is unprogrammed/incorrect.
> +
> +Required Properties:
> + - compatible: Must be "simple-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
> +
> +Battery properties are named, where possible, for the corresponding 
> +elements in enum power_supply_property, defined in
> +https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/power_supply.h#n86

Since Rob did not reply, I queued this with this paragraph removed
and whitespace errors fixed to speed up things. You can always try
persuading Rob in an incremental patch ;)

-- Sebastian

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

[PATCH v11 03/10] devicetree: power: bq27xxx: Add monitored-battery documentation

[Liam Breck]

From: Liam Breck <kernel-RYWXG+zxWwBdeoIcmNTgJF6hYfS7NtTn@public.gmane.org>

Document monitored-battery = <&battery_node>

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>
Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Acked-by: Sebastian Reichel <sre-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 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

--
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 v11 03/10] devicetree: power: bq27xxx: Add monitored-battery documentation

[Sebastian Reichel]

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

Hi,

On Mon, Mar 20, 2017 at 02:43:28AM -0700, Liam Breck wrote:
> From: Liam Breck <kernel@networkimprov.net>
> 
> 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>
> Acked-by: Rob Herring <robh@kernel.org>
> Acked-by: Sebastian Reichel <sre@kernel.org>
> ---
>  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>;
>  };

Thanks, queued.

-- Sebastian

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

[PATCH v11 02/10] 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

Re: [PATCH v11 02/10] devicetree: property-units: Add uWh and uAh units

[Sebastian Reichel]

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

Hi,

On Mon, Mar 20, 2017 at 02:43:27AM -0700, Liam Breck wrote:
> 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

Thanks, queued.

-- Sebastian

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

[PATCH v11 04/10] power: power_supply: Add power_supply_battery_info and API

[Liam Breck]

From: Liam Breck <kernel@networkimprov.net>

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>
---
 Documentation/power/power_supply_class.txt | 12 ++++++++
 drivers/power/supply/power_supply_core.c   | 45 ++++++++++++++++++++++++++++++
 include/linux/power_supply.h               | 18 ++++++++++++
 3 files changed, 75 insertions(+)

diff --git a/Documentation/power/power_supply_class.txt b/Documentation/power/power_supply_class.txt
index 0c72588..dc92c77 100644
--- a/Documentation/power/power_supply_class.txt
+++ b/Documentation/power/power_supply_class.txt
@@ -174,6 +174,18 @@ issued by external power supply will notify supplicants via
 external_power_changed callback.
 
 
+Devicetree battery characteristics
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+Drivers should call power_supply_get_battery_info() to obtain battery 
+characteristics from a devicetree battery node, defined in
+Documentation/devicetree/bindings/power/supply/battery.txt. This is 
+implemented in drivers/power/supply/bq27xxx_battery.c. 
+
+Properties in struct power_supply_battery_info and their counterparts in the 
+battery node have names corresponding to elements in enum power_supply_property,
+for naming consistency between sysfs attributes and battery node properties.
+
+
 QA
 ~~
 Q: Where is POWER_SUPPLY_PROP_XYZ attribute?
diff --git a/drivers/power/supply/power_supply_core.c b/drivers/power/supply/power_supply_core.c
index a74d8ca..61e20b1 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,50 @@ 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("simple-battery", value))
+		return -ENODEV;
+
+	/* The property and field names below must correspond to elements
+	 * in enum power_supply_property. For reasoning, see
+	 * Documentation/power/power_supply_class.txt.
+	 */
+
+	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

Re: [PATCH v11 04/10] power: power_supply: Add power_supply_battery_info and API

[Liam Breck]

Hi Sebastian,

On Mon, Mar 20, 2017 at 2:43 AM, Liam Breck <liam@networkimprov.net> wrote:
> From: Liam Breck <kernel@networkimprov.net>
>
> 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>
> ---
>  Documentation/power/power_supply_class.txt | 12 ++++++++
>  drivers/power/supply/power_supply_core.c   | 45 ++++++++++++++++++++++++++++++
>  include/linux/power_supply.h               | 18 ++++++++++++
>  3 files changed, 75 insertions(+)
>
> diff --git a/Documentation/power/power_supply_class.txt b/Documentation/power/power_supply_class.txt
> index 0c72588..dc92c77 100644
> --- a/Documentation/power/power_supply_class.txt
> +++ b/Documentation/power/power_supply_class.txt
> @@ -174,6 +174,18 @@ issued by external power supply will notify supplicants via
>  external_power_changed callback.
>
>
> +Devicetree battery characteristics
> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> +Drivers should call power_supply_get_battery_info() to obtain battery
> +characteristics from a devicetree battery node, defined in
> +Documentation/devicetree/bindings/power/supply/battery.txt. This is
> +implemented in drivers/power/supply/bq27xxx_battery.c.
> +
> +Properties in struct power_supply_battery_info and their counterparts in the
> +battery node have names corresponding to elements in enum power_supply_property,
> +for naming consistency between sysfs attributes and battery node properties.
> +
> +
>  QA
>  ~~
>  Q: Where is POWER_SUPPLY_PROP_XYZ attribute?
> diff --git a/drivers/power/supply/power_supply_core.c b/drivers/power/supply/power_supply_core.c
> index a74d8ca..61e20b1 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,50 @@ 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("simple-battery", value))
> +               return -ENODEV;
> +
> +       /* The property and field names below must correspond to elements
> +        * in enum power_supply_property. For reasoning, see
> +        * Documentation/power/power_supply_class.txt.
> +        */
> +
> +       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);

Should we also check for an acpi_handle, and use
device_property_read_* here? Tho presumably the acpi property names
would be different...

I wouldn't have a way to test that however.

> +
> +       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
>

Re: [PATCH v11 04/10] power: power_supply: Add power_supply_battery_info and API

[Sebastian Reichel]

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

Hi,

On Mon, Mar 20, 2017 at 01:59:01PM -0700, Liam Breck wrote:
> On Mon, Mar 20, 2017 at 2:43 AM, Liam Breck <liam@networkimprov.net> wrote:
> > From: Liam Breck <kernel@networkimprov.net>
> >
> > 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>
> > ---
> >  Documentation/power/power_supply_class.txt | 12 ++++++++
> >  drivers/power/supply/power_supply_core.c   | 45 ++++++++++++++++++++++++++++++
> >  include/linux/power_supply.h               | 18 ++++++++++++
> >  3 files changed, 75 insertions(+)
> >
> > diff --git a/Documentation/power/power_supply_class.txt b/Documentation/power/power_supply_class.txt
> > index 0c72588..dc92c77 100644
> > --- a/Documentation/power/power_supply_class.txt
> > +++ b/Documentation/power/power_supply_class.txt
> > @@ -174,6 +174,18 @@ issued by external power supply will notify supplicants via
> >  external_power_changed callback.
> >
> >
> > +Devicetree battery characteristics
> > +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > +Drivers should call power_supply_get_battery_info() to obtain battery
> > +characteristics from a devicetree battery node, defined in
> > +Documentation/devicetree/bindings/power/supply/battery.txt. This is
> > +implemented in drivers/power/supply/bq27xxx_battery.c.
> > +
> > +Properties in struct power_supply_battery_info and their counterparts in the
> > +battery node have names corresponding to elements in enum power_supply_property,
> > +for naming consistency between sysfs attributes and battery node properties.
> > +
> > +
> >  QA
> >  ~~
> >  Q: Where is POWER_SUPPLY_PROP_XYZ attribute?
> > diff --git a/drivers/power/supply/power_supply_core.c b/drivers/power/supply/power_supply_core.c
> > index a74d8ca..61e20b1 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,50 @@ 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("simple-battery", value))
> > +               return -ENODEV;
> > +
> > +       /* The property and field names below must correspond to elements
> > +        * in enum power_supply_property. For reasoning, see
> > +        * Documentation/power/power_supply_class.txt.
> > +        */

Let's make this "should correspond".

> > +       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);

> Should we also check for an acpi_handle, and use
> device_property_read_* here? Tho presumably the acpi property names
> would be different...
> 
> I wouldn't have a way to test that however.

Yes, please use device_property_read_* instead of of_property_read_*.
Apart from that you do not have to care for ACPI for now. It can be
added once needed.

> > +
> > +       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
> >

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

Re: [PATCH v11 04/10] power: power_supply: Add power_supply_battery_info and API

[Liam Breck]

Hi Sebastian, some Qs re device_property_ api...

On Thu, Mar 23, 2017 at 3:17 AM, Sebastian Reichel <sre@kernel.org> wrote:
> Hi,
>
> On Mon, Mar 20, 2017 at 01:59:01PM -0700, Liam Breck wrote:
>> On Mon, Mar 20, 2017 at 2:43 AM, Liam Breck <liam@networkimprov.net> wrote:
>> > From: Liam Breck <kernel@networkimprov.net>
>> >
>> > 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>
>> > ---
>> >  Documentation/power/power_supply_class.txt | 12 ++++++++
>> >  drivers/power/supply/power_supply_core.c   | 45 ++++++++++++++++++++++++++++++
>> >  include/linux/power_supply.h               | 18 ++++++++++++
>> >  3 files changed, 75 insertions(+)
>> >
>> > diff --git a/Documentation/power/power_supply_class.txt b/Documentation/power/power_supply_class.txt
>> > index 0c72588..dc92c77 100644
>> > --- a/Documentation/power/power_supply_class.txt
>> > +++ b/Documentation/power/power_supply_class.txt
>> > @@ -174,6 +174,18 @@ issued by external power supply will notify supplicants via
>> >  external_power_changed callback.
>> >
>> >
>> > +Devicetree battery characteristics
>> > +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> > +Drivers should call power_supply_get_battery_info() to obtain battery
>> > +characteristics from a devicetree battery node, defined in
>> > +Documentation/devicetree/bindings/power/supply/battery.txt. This is
>> > +implemented in drivers/power/supply/bq27xxx_battery.c.
>> > +
>> > +Properties in struct power_supply_battery_info and their counterparts in the
>> > +battery node have names corresponding to elements in enum power_supply_property,
>> > +for naming consistency between sysfs attributes and battery node properties.
>> > +
>> > +
>> >  QA
>> >  ~~
>> >  Q: Where is POWER_SUPPLY_PROP_XYZ attribute?
>> > diff --git a/drivers/power/supply/power_supply_core.c b/drivers/power/supply/power_supply_core.c
>> > index a74d8ca..61e20b1 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,50 @@ 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) {

Do we still test that?

>> > +               dev_warn(&psy->dev, "%s currently only supports devicetree\n",
>> > +                        __func__);
>> > +               return -ENXIO;
>> > +       }
>> > +
>> > +       battery_np = of_parse_phandle(psy->of_node, "monitored-battery", 0);

Does that change? If not, will psy->dev work in device_property_read_* ?

>> > +       if (!battery_np)
>> > +               return -ENODEV;
>> > +
>> > +       err = of_property_read_string(battery_np, "compatible", &value);
>> > +       if (err)
>> > +               return err;
>> > +
>> > +       if (strcmp("simple-battery", value))
>> > +               return -ENODEV;
>> > +
>> > +       /* The property and field names below must correspond to elements
>> > +        * in enum power_supply_property. For reasoning, see
>> > +        * Documentation/power/power_supply_class.txt.
>> > +        */
>
> Let's make this "should correspond".
>
>> > +       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);

And these become: device_property_read_u32(psy->dev, ...) ?

>> Should we also check for an acpi_handle, and use
>> device_property_read_* here? Tho presumably the acpi property names
>> would be different...
>>
>> I wouldn't have a way to test that however.
>
> Yes, please use device_property_read_* instead of of_property_read_*.
> Apart from that you do not have to care for ACPI for now. It can be
> added once needed.
>
>> > +
>> > +       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
>> >

Re: [PATCH v11 04/10] power: power_supply: Add power_supply_battery_info and API

[Sebastian Reichel]

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

Hi,

On Sat, Mar 25, 2017 at 04:19:24PM -0700, Liam Breck wrote:
> >> > +               dev_warn(&psy->dev, "%s currently only supports devicetree\n",
> >> > +                        __func__);
> >> > +               return -ENXIO;
> >> > +       }
> >> > +
> >> > +       battery_np = of_parse_phandle(psy->of_node, "monitored-battery", 0);
> 
> Does that change? If not, will psy->dev work in device_property_read_* ?

Oh right, we don't have a device for usage with device_property_read_*.
Then don't mind for now. We can think about this once ACPI/platform based
init is required. Probably simple-battery should get a real driver
at that point.

-- Sebastian

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

Re: [PATCH v11 04/10] power: power_supply: Add power_supply_battery_info and API

[Sebastian Reichel]

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

Hi,

On Mon, Mar 20, 2017 at 02:43:29AM -0700, Liam Breck wrote:
> From: Liam Breck <kernel@networkimprov.net>
> 
> 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>

Thanks, queued.

-- Sebastian

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

[PATCH v11 05/10] power: bq27xxx_battery: Make error reporting consistent

[Liam Breck]

From: Liam Breck <kernel@networkimprov.net>

Previously errors were reported in a mix of styles via both dev_err() & dev_dbg().
Report all errors with dev_err() and include error code in message.
Report register ID separately in dev_dbg() message.

Signed-off-by: Liam Breck <kernel@networkimprov.net>
---
 drivers/power/supply/bq27xxx_battery.c | 35 ++++++++++++++++++----------------
 1 file changed, 19 insertions(+), 16 deletions(-)

diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
index 398801a..2e2a3a8 100644
--- a/drivers/power/supply/bq27xxx_battery.c
+++ b/drivers/power/supply/bq27xxx_battery.c
@@ -794,11 +794,17 @@ MODULE_PARM_DESC(poll_interval,
 static inline int bq27xxx_read(struct bq27xxx_device_info *di, int reg_index,
 			       bool single)
 {
-	/* Reports EINVAL for invalid/missing registers */
+	int ret;
+
 	if (!di || di->regs[reg_index] == INVALID_REG_ADDR)
 		return -EINVAL;
 
-	return di->bus.read(di, di->regs[reg_index], single);
+	ret = di->bus.read(di, di->regs[reg_index], single);
+	if (ret < 0)
+		dev_dbg(di->dev, "failed to read register 0x%02x (index %d)\n",
+			di->regs[reg_index], reg_index);
+
+	return ret;
 }
 
 /*
@@ -815,7 +821,7 @@ static int bq27xxx_battery_read_soc(struct bq27xxx_device_info *di)
 		soc = bq27xxx_read(di, BQ27XXX_REG_SOC, false);
 
 	if (soc < 0)
-		dev_dbg(di->dev, "error reading State-of-Charge\n");
+		dev_err(di->dev, "error %d reading state-of-charge\n", soc);
 
 	return soc;
 }
@@ -830,8 +836,7 @@ static int bq27xxx_battery_read_charge(struct bq27xxx_device_info *di, u8 reg)
 
 	charge = bq27xxx_read(di, reg, false);
 	if (charge < 0) {
-		dev_dbg(di->dev, "error reading charge register %02x: %d\n",
-			reg, charge);
+		dev_err(di->dev, "error %d reading charge\n", charge);
 		return charge;
 	}
 
@@ -883,7 +888,7 @@ static int bq27xxx_battery_read_dcap(struct bq27xxx_device_info *di)
 		dcap = bq27xxx_read(di, BQ27XXX_REG_DCAP, false);
 
 	if (dcap < 0) {
-		dev_dbg(di->dev, "error reading initial last measured discharge\n");
+		dev_err(di->dev, "error %d reading design capacity\n", dcap);
 		return dcap;
 	}
 
@@ -905,7 +910,7 @@ static int bq27xxx_battery_read_energy(struct bq27xxx_device_info *di)
 
 	ae = bq27xxx_read(di, BQ27XXX_REG_AE, false);
 	if (ae < 0) {
-		dev_dbg(di->dev, "error reading available energy\n");
+		dev_err(di->dev, "error %d reading available energy\n", ae);
 		return ae;
 	}
 
@@ -927,7 +932,7 @@ static int bq27xxx_battery_read_temperature(struct bq27xxx_device_info *di)
 
 	temp = bq27xxx_read(di, BQ27XXX_REG_TEMP, false);
 	if (temp < 0) {
-		dev_err(di->dev, "error reading temperature\n");
+		dev_err(di->dev, "error %d reading temperature\n", temp);
 		return temp;
 	}
 
@@ -947,7 +952,7 @@ static int bq27xxx_battery_read_cyct(struct bq27xxx_device_info *di)
 
 	cyct = bq27xxx_read(di, BQ27XXX_REG_CYCT, false);
 	if (cyct < 0)
-		dev_err(di->dev, "error reading cycle count total\n");
+		dev_err(di->dev, "error %d reading cycle count total\n", cyct);
 
 	return cyct;
 }
@@ -962,8 +967,7 @@ static int bq27xxx_battery_read_time(struct bq27xxx_device_info *di, u8 reg)
 
 	tval = bq27xxx_read(di, reg, false);
 	if (tval < 0) {
-		dev_dbg(di->dev, "error reading time register %02x: %d\n",
-			reg, tval);
+		dev_err(di->dev, "error %d reading time\n", tval);
 		return tval;
 	}
 
@@ -983,8 +987,7 @@ static int bq27xxx_battery_read_pwr_avg(struct bq27xxx_device_info *di)
 
 	tval = bq27xxx_read(di, BQ27XXX_REG_AP, false);
 	if (tval < 0) {
-		dev_err(di->dev, "error reading average power register  %02x: %d\n",
-			BQ27XXX_REG_AP, tval);
+		dev_err(di->dev, "error %d reading average power\n", tval);
 		return tval;
 	}
 
@@ -1054,7 +1057,7 @@ static int bq27xxx_battery_read_health(struct bq27xxx_device_info *di)
 
 	flags = bq27xxx_read(di, BQ27XXX_REG_FLAGS, has_singe_flag);
 	if (flags < 0) {
-		dev_err(di->dev, "error reading flag register:%d\n", flags);
+		dev_err(di->dev, "error %d reading flags\n", flags);
 		return flags;
 	}
 
@@ -1147,7 +1150,7 @@ static int bq27xxx_battery_current(struct bq27xxx_device_info *di,
 
 	curr = bq27xxx_read(di, BQ27XXX_REG_AI, false);
 	if (curr < 0) {
-		dev_err(di->dev, "error reading current\n");
+		dev_err(di->dev, "error %d reading current\n", curr);
 		return curr;
 	}
 
@@ -1236,7 +1239,7 @@ static int bq27xxx_battery_voltage(struct bq27xxx_device_info *di,
 
 	volt = bq27xxx_read(di, BQ27XXX_REG_VOLT, false);
 	if (volt < 0) {
-		dev_err(di->dev, "error reading voltage\n");
+		dev_err(di->dev, "error %d reading voltage\n", volt);
 		return volt;
 	}
 
-- 
2.9.3

Re: [PATCH v11 05/10] power: bq27xxx_battery: Make error reporting consistent

[Andrew F. Davis]

On 03/20/2017 04:43 AM, Liam Breck wrote:
> From: Liam Breck <kernel@networkimprov.net>
> 
> Previously errors were reported in a mix of styles via both dev_err() & dev_dbg().
> Report all errors with dev_err() and include error code in message.
> Report register ID separately in dev_dbg() message.
> 
> Signed-off-by: Liam Breck <kernel@networkimprov.net>
> ---
>  drivers/power/supply/bq27xxx_battery.c | 35 ++++++++++++++++++----------------
>  1 file changed, 19 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
> index 398801a..2e2a3a8 100644
> --- a/drivers/power/supply/bq27xxx_battery.c
> +++ b/drivers/power/supply/bq27xxx_battery.c
> @@ -794,11 +794,17 @@ MODULE_PARM_DESC(poll_interval,
>  static inline int bq27xxx_read(struct bq27xxx_device_info *di, int reg_index,
>  			       bool single)
>  {
> -	/* Reports EINVAL for invalid/missing registers */
> +	int ret;
> +
>  	if (!di || di->regs[reg_index] == INVALID_REG_ADDR)
>  		return -EINVAL;
>  
> -	return di->bus.read(di, di->regs[reg_index], single);
> +	ret = di->bus.read(di, di->regs[reg_index], single);
> +	if (ret < 0)
> +		dev_dbg(di->dev, "failed to read register 0x%02x (index %d)\n",
> +			di->regs[reg_index], reg_index);
> +

If we print out the error code here we don't have to do that in each of
the below printouts.

Otherwise looks good:
Acked-by: Andrew F. Davis <afd@ti.com>

> +	return ret;
>  }
>  
>  /*
> @@ -815,7 +821,7 @@ static int bq27xxx_battery_read_soc(struct bq27xxx_device_info *di)
>  		soc = bq27xxx_read(di, BQ27XXX_REG_SOC, false);
>  
>  	if (soc < 0)
> -		dev_dbg(di->dev, "error reading State-of-Charge\n");
> +		dev_err(di->dev, "error %d reading state-of-charge\n", soc);
>  
>  	return soc;
>  }
> @@ -830,8 +836,7 @@ static int bq27xxx_battery_read_charge(struct bq27xxx_device_info *di, u8 reg)
>  
>  	charge = bq27xxx_read(di, reg, false);
>  	if (charge < 0) {
> -		dev_dbg(di->dev, "error reading charge register %02x: %d\n",
> -			reg, charge);
> +		dev_err(di->dev, "error %d reading charge\n", charge);
>  		return charge;
>  	}
>  
> @@ -883,7 +888,7 @@ static int bq27xxx_battery_read_dcap(struct bq27xxx_device_info *di)
>  		dcap = bq27xxx_read(di, BQ27XXX_REG_DCAP, false);
>  
>  	if (dcap < 0) {
> -		dev_dbg(di->dev, "error reading initial last measured discharge\n");
> +		dev_err(di->dev, "error %d reading design capacity\n", dcap);
>  		return dcap;
>  	}
>  
> @@ -905,7 +910,7 @@ static int bq27xxx_battery_read_energy(struct bq27xxx_device_info *di)
>  
>  	ae = bq27xxx_read(di, BQ27XXX_REG_AE, false);
>  	if (ae < 0) {
> -		dev_dbg(di->dev, "error reading available energy\n");
> +		dev_err(di->dev, "error %d reading available energy\n", ae);
>  		return ae;
>  	}
>  
> @@ -927,7 +932,7 @@ static int bq27xxx_battery_read_temperature(struct bq27xxx_device_info *di)
>  
>  	temp = bq27xxx_read(di, BQ27XXX_REG_TEMP, false);
>  	if (temp < 0) {
> -		dev_err(di->dev, "error reading temperature\n");
> +		dev_err(di->dev, "error %d reading temperature\n", temp);
>  		return temp;
>  	}
>  
> @@ -947,7 +952,7 @@ static int bq27xxx_battery_read_cyct(struct bq27xxx_device_info *di)
>  
>  	cyct = bq27xxx_read(di, BQ27XXX_REG_CYCT, false);
>  	if (cyct < 0)
> -		dev_err(di->dev, "error reading cycle count total\n");
> +		dev_err(di->dev, "error %d reading cycle count total\n", cyct);
>  
>  	return cyct;
>  }
> @@ -962,8 +967,7 @@ static int bq27xxx_battery_read_time(struct bq27xxx_device_info *di, u8 reg)
>  
>  	tval = bq27xxx_read(di, reg, false);
>  	if (tval < 0) {
> -		dev_dbg(di->dev, "error reading time register %02x: %d\n",
> -			reg, tval);
> +		dev_err(di->dev, "error %d reading time\n", tval);
>  		return tval;
>  	}
>  
> @@ -983,8 +987,7 @@ static int bq27xxx_battery_read_pwr_avg(struct bq27xxx_device_info *di)
>  
>  	tval = bq27xxx_read(di, BQ27XXX_REG_AP, false);
>  	if (tval < 0) {
> -		dev_err(di->dev, "error reading average power register  %02x: %d\n",
> -			BQ27XXX_REG_AP, tval);
> +		dev_err(di->dev, "error %d reading average power\n", tval);
>  		return tval;
>  	}
>  
> @@ -1054,7 +1057,7 @@ static int bq27xxx_battery_read_health(struct bq27xxx_device_info *di)
>  
>  	flags = bq27xxx_read(di, BQ27XXX_REG_FLAGS, has_singe_flag);
>  	if (flags < 0) {
> -		dev_err(di->dev, "error reading flag register:%d\n", flags);
> +		dev_err(di->dev, "error %d reading flags\n", flags);
>  		return flags;
>  	}
>  
> @@ -1147,7 +1150,7 @@ static int bq27xxx_battery_current(struct bq27xxx_device_info *di,
>  
>  	curr = bq27xxx_read(di, BQ27XXX_REG_AI, false);
>  	if (curr < 0) {
> -		dev_err(di->dev, "error reading current\n");
> +		dev_err(di->dev, "error %d reading current\n", curr);
>  		return curr;
>  	}
>  
> @@ -1236,7 +1239,7 @@ static int bq27xxx_battery_voltage(struct bq27xxx_device_info *di,
>  
>  	volt = bq27xxx_read(di, BQ27XXX_REG_VOLT, false);
>  	if (volt < 0) {
> -		dev_err(di->dev, "error reading voltage\n");
> +		dev_err(di->dev, "error %d reading voltage\n", volt);
>  		return volt;
>  	}
>  
> 

Re: [PATCH v11 05/10] power: bq27xxx_battery: Make error reporting consistent

[Liam Breck]

On Mon, Mar 20, 2017 at 9:57 AM, Andrew F. Davis <afd@ti.com> wrote:
> On 03/20/2017 04:43 AM, Liam Breck wrote:
>> From: Liam Breck <kernel@networkimprov.net>
>>
>> Previously errors were reported in a mix of styles via both dev_err() & dev_dbg().
>> Report all errors with dev_err() and include error code in message.
>> Report register ID separately in dev_dbg() message.
>>
>> Signed-off-by: Liam Breck <kernel@networkimprov.net>
>> ---
>>  drivers/power/supply/bq27xxx_battery.c | 35 ++++++++++++++++++----------------
>>  1 file changed, 19 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
>> index 398801a..2e2a3a8 100644
>> --- a/drivers/power/supply/bq27xxx_battery.c
>> +++ b/drivers/power/supply/bq27xxx_battery.c
>> @@ -794,11 +794,17 @@ MODULE_PARM_DESC(poll_interval,
>>  static inline int bq27xxx_read(struct bq27xxx_device_info *di, int reg_index,
>>                              bool single)
>>  {
>> -     /* Reports EINVAL for invalid/missing registers */
>> +     int ret;
>> +
>>       if (!di || di->regs[reg_index] == INVALID_REG_ADDR)
>>               return -EINVAL;
>>
>> -     return di->bus.read(di, di->regs[reg_index], single);
>> +     ret = di->bus.read(di, di->regs[reg_index], single);
>> +     if (ret < 0)
>> +             dev_dbg(di->dev, "failed to read register 0x%02x (index %d)\n",
>> +                     di->regs[reg_index], reg_index);
>> +
>
> If we print out the error code here we don't have to do that in each of
> the below printouts.

This is a dev_dbg, the below are dev_err, and I think you want error
code in dev_err. BTW the above addresses your request to learn which
register was in play in the event of a DM read/write failure.

> Otherwise looks good:
> Acked-by: Andrew F. Davis <afd@ti.com>
>
>> +     return ret;
>>  }
>>
>>  /*
>> @@ -815,7 +821,7 @@ static int bq27xxx_battery_read_soc(struct bq27xxx_device_info *di)
>>               soc = bq27xxx_read(di, BQ27XXX_REG_SOC, false);
>>
>>       if (soc < 0)
>> -             dev_dbg(di->dev, "error reading State-of-Charge\n");
>> +             dev_err(di->dev, "error %d reading state-of-charge\n", soc);
>>
>>       return soc;
>>  }
>> @@ -830,8 +836,7 @@ static int bq27xxx_battery_read_charge(struct bq27xxx_device_info *di, u8 reg)
>>
>>       charge = bq27xxx_read(di, reg, false);
>>       if (charge < 0) {
>> -             dev_dbg(di->dev, "error reading charge register %02x: %d\n",
>> -                     reg, charge);
>> +             dev_err(di->dev, "error %d reading charge\n", charge);
>>               return charge;
>>       }
>>
>> @@ -883,7 +888,7 @@ static int bq27xxx_battery_read_dcap(struct bq27xxx_device_info *di)
>>               dcap = bq27xxx_read(di, BQ27XXX_REG_DCAP, false);
>>
>>       if (dcap < 0) {
>> -             dev_dbg(di->dev, "error reading initial last measured discharge\n");
>> +             dev_err(di->dev, "error %d reading design capacity\n", dcap);
>>               return dcap;
>>       }
>>
>> @@ -905,7 +910,7 @@ static int bq27xxx_battery_read_energy(struct bq27xxx_device_info *di)
>>
>>       ae = bq27xxx_read(di, BQ27XXX_REG_AE, false);
>>       if (ae < 0) {
>> -             dev_dbg(di->dev, "error reading available energy\n");
>> +             dev_err(di->dev, "error %d reading available energy\n", ae);
>>               return ae;
>>       }
>>
>> @@ -927,7 +932,7 @@ static int bq27xxx_battery_read_temperature(struct bq27xxx_device_info *di)
>>
>>       temp = bq27xxx_read(di, BQ27XXX_REG_TEMP, false);
>>       if (temp < 0) {
>> -             dev_err(di->dev, "error reading temperature\n");
>> +             dev_err(di->dev, "error %d reading temperature\n", temp);
>>               return temp;
>>       }
>>
>> @@ -947,7 +952,7 @@ static int bq27xxx_battery_read_cyct(struct bq27xxx_device_info *di)
>>
>>       cyct = bq27xxx_read(di, BQ27XXX_REG_CYCT, false);
>>       if (cyct < 0)
>> -             dev_err(di->dev, "error reading cycle count total\n");
>> +             dev_err(di->dev, "error %d reading cycle count total\n", cyct);
>>
>>       return cyct;
>>  }
>> @@ -962,8 +967,7 @@ static int bq27xxx_battery_read_time(struct bq27xxx_device_info *di, u8 reg)
>>
>>       tval = bq27xxx_read(di, reg, false);
>>       if (tval < 0) {
>> -             dev_dbg(di->dev, "error reading time register %02x: %d\n",
>> -                     reg, tval);
>> +             dev_err(di->dev, "error %d reading time\n", tval);
>>               return tval;
>>       }
>>
>> @@ -983,8 +987,7 @@ static int bq27xxx_battery_read_pwr_avg(struct bq27xxx_device_info *di)
>>
>>       tval = bq27xxx_read(di, BQ27XXX_REG_AP, false);
>>       if (tval < 0) {
>> -             dev_err(di->dev, "error reading average power register  %02x: %d\n",
>> -                     BQ27XXX_REG_AP, tval);
>> +             dev_err(di->dev, "error %d reading average power\n", tval);
>>               return tval;
>>       }
>>
>> @@ -1054,7 +1057,7 @@ static int bq27xxx_battery_read_health(struct bq27xxx_device_info *di)
>>
>>       flags = bq27xxx_read(di, BQ27XXX_REG_FLAGS, has_singe_flag);
>>       if (flags < 0) {
>> -             dev_err(di->dev, "error reading flag register:%d\n", flags);
>> +             dev_err(di->dev, "error %d reading flags\n", flags);
>>               return flags;
>>       }
>>
>> @@ -1147,7 +1150,7 @@ static int bq27xxx_battery_current(struct bq27xxx_device_info *di,
>>
>>       curr = bq27xxx_read(di, BQ27XXX_REG_AI, false);
>>       if (curr < 0) {
>> -             dev_err(di->dev, "error reading current\n");
>> +             dev_err(di->dev, "error %d reading current\n", curr);
>>               return curr;
>>       }
>>
>> @@ -1236,7 +1239,7 @@ static int bq27xxx_battery_voltage(struct bq27xxx_device_info *di,
>>
>>       volt = bq27xxx_read(di, BQ27XXX_REG_VOLT, false);
>>       if (volt < 0) {
>> -             dev_err(di->dev, "error reading voltage\n");
>> +             dev_err(di->dev, "error %d reading voltage\n", volt);
>>               return volt;
>>       }
>>
>>

Re: [PATCH v11 05/10] power: bq27xxx_battery: Make error reporting consistent

[Sebastian Reichel]

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

Hi,

On Mon, Mar 20, 2017 at 10:59:18AM -0700, Liam Breck wrote:
> On Mon, Mar 20, 2017 at 9:57 AM, Andrew F. Davis <afd@ti.com> wrote:
> > On 03/20/2017 04:43 AM, Liam Breck wrote:
> >> From: Liam Breck <kernel@networkimprov.net>
> >>
> >> Previously errors were reported in a mix of styles via both dev_err() & dev_dbg().
> >> Report all errors with dev_err() and include error code in message.
> >> Report register ID separately in dev_dbg() message.
> >>
> >> Signed-off-by: Liam Breck <kernel@networkimprov.net>
> >> ---
> >>  drivers/power/supply/bq27xxx_battery.c | 35 ++++++++++++++++++----------------
> >>  1 file changed, 19 insertions(+), 16 deletions(-)
> >>
> >> diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
> >> index 398801a..2e2a3a8 100644
> >> --- a/drivers/power/supply/bq27xxx_battery.c
> >> +++ b/drivers/power/supply/bq27xxx_battery.c
> >> @@ -794,11 +794,17 @@ MODULE_PARM_DESC(poll_interval,
> >>  static inline int bq27xxx_read(struct bq27xxx_device_info *di, int reg_index,
> >>                              bool single)
> >>  {
> >> -     /* Reports EINVAL for invalid/missing registers */
> >> +     int ret;
> >> +
> >>       if (!di || di->regs[reg_index] == INVALID_REG_ADDR)
> >>               return -EINVAL;
> >>
> >> -     return di->bus.read(di, di->regs[reg_index], single);
> >> +     ret = di->bus.read(di, di->regs[reg_index], single);
> >> +     if (ret < 0)
> >> +             dev_dbg(di->dev, "failed to read register 0x%02x (index %d)\n",
> >> +                     di->regs[reg_index], reg_index);
> >> +
> >
> > If we print out the error code here we don't have to do that in each of
> > the below printouts.
> 
> This is a dev_dbg, the below are dev_err, and I think you want error
> code in dev_err. BTW the above addresses your request to learn which
> register was in play in the event of a DM read/write failure.

Just make it dev_err and also print ret. Then we can remove all
other prints. That will result in some information-loss (name
of the register), but

1. it's not that important anyways. If you read battery voltage file
   in sysfs, then obviously reading voltage fails and so on.
2. the error should be very unlikely on productive systems, since
   it basically mean the i2c/1-wire transaction failed.

-- Sebastian

> > Otherwise looks good:
> > Acked-by: Andrew F. Davis <afd@ti.com>
> >
> >> +     return ret;
> >>  }
> >>
> >>  /*
> >> @@ -815,7 +821,7 @@ static int bq27xxx_battery_read_soc(struct bq27xxx_device_info *di)
> >>               soc = bq27xxx_read(di, BQ27XXX_REG_SOC, false);
> >>
> >>       if (soc < 0)
> >> -             dev_dbg(di->dev, "error reading State-of-Charge\n");
> >> +             dev_err(di->dev, "error %d reading state-of-charge\n", soc);
> >>
> >>       return soc;
> >>  }
> >> @@ -830,8 +836,7 @@ static int bq27xxx_battery_read_charge(struct bq27xxx_device_info *di, u8 reg)
> >>
> >>       charge = bq27xxx_read(di, reg, false);
> >>       if (charge < 0) {
> >> -             dev_dbg(di->dev, "error reading charge register %02x: %d\n",
> >> -                     reg, charge);
> >> +             dev_err(di->dev, "error %d reading charge\n", charge);
> >>               return charge;
> >>       }
> >>
> >> @@ -883,7 +888,7 @@ static int bq27xxx_battery_read_dcap(struct bq27xxx_device_info *di)
> >>               dcap = bq27xxx_read(di, BQ27XXX_REG_DCAP, false);
> >>
> >>       if (dcap < 0) {
> >> -             dev_dbg(di->dev, "error reading initial last measured discharge\n");
> >> +             dev_err(di->dev, "error %d reading design capacity\n", dcap);
> >>               return dcap;
> >>       }
> >>
> >> @@ -905,7 +910,7 @@ static int bq27xxx_battery_read_energy(struct bq27xxx_device_info *di)
> >>
> >>       ae = bq27xxx_read(di, BQ27XXX_REG_AE, false);
> >>       if (ae < 0) {
> >> -             dev_dbg(di->dev, "error reading available energy\n");
> >> +             dev_err(di->dev, "error %d reading available energy\n", ae);
> >>               return ae;
> >>       }
> >>
> >> @@ -927,7 +932,7 @@ static int bq27xxx_battery_read_temperature(struct bq27xxx_device_info *di)
> >>
> >>       temp = bq27xxx_read(di, BQ27XXX_REG_TEMP, false);
> >>       if (temp < 0) {
> >> -             dev_err(di->dev, "error reading temperature\n");
> >> +             dev_err(di->dev, "error %d reading temperature\n", temp);
> >>               return temp;
> >>       }
> >>
> >> @@ -947,7 +952,7 @@ static int bq27xxx_battery_read_cyct(struct bq27xxx_device_info *di)
> >>
> >>       cyct = bq27xxx_read(di, BQ27XXX_REG_CYCT, false);
> >>       if (cyct < 0)
> >> -             dev_err(di->dev, "error reading cycle count total\n");
> >> +             dev_err(di->dev, "error %d reading cycle count total\n", cyct);
> >>
> >>       return cyct;
> >>  }
> >> @@ -962,8 +967,7 @@ static int bq27xxx_battery_read_time(struct bq27xxx_device_info *di, u8 reg)
> >>
> >>       tval = bq27xxx_read(di, reg, false);
> >>       if (tval < 0) {
> >> -             dev_dbg(di->dev, "error reading time register %02x: %d\n",
> >> -                     reg, tval);
> >> +             dev_err(di->dev, "error %d reading time\n", tval);
> >>               return tval;
> >>       }
> >>
> >> @@ -983,8 +987,7 @@ static int bq27xxx_battery_read_pwr_avg(struct bq27xxx_device_info *di)
> >>
> >>       tval = bq27xxx_read(di, BQ27XXX_REG_AP, false);
> >>       if (tval < 0) {
> >> -             dev_err(di->dev, "error reading average power register  %02x: %d\n",
> >> -                     BQ27XXX_REG_AP, tval);
> >> +             dev_err(di->dev, "error %d reading average power\n", tval);
> >>               return tval;
> >>       }
> >>
> >> @@ -1054,7 +1057,7 @@ static int bq27xxx_battery_read_health(struct bq27xxx_device_info *di)
> >>
> >>       flags = bq27xxx_read(di, BQ27XXX_REG_FLAGS, has_singe_flag);
> >>       if (flags < 0) {
> >> -             dev_err(di->dev, "error reading flag register:%d\n", flags);
> >> +             dev_err(di->dev, "error %d reading flags\n", flags);
> >>               return flags;
> >>       }
> >>
> >> @@ -1147,7 +1150,7 @@ static int bq27xxx_battery_current(struct bq27xxx_device_info *di,
> >>
> >>       curr = bq27xxx_read(di, BQ27XXX_REG_AI, false);
> >>       if (curr < 0) {
> >> -             dev_err(di->dev, "error reading current\n");
> >> +             dev_err(di->dev, "error %d reading current\n", curr);
> >>               return curr;
> >>       }
> >>
> >> @@ -1236,7 +1239,7 @@ static int bq27xxx_battery_voltage(struct bq27xxx_device_info *di,
> >>
> >>       volt = bq27xxx_read(di, BQ27XXX_REG_VOLT, false);
> >>       if (volt < 0) {
> >> -             dev_err(di->dev, "error reading voltage\n");
> >> +             dev_err(di->dev, "error %d reading voltage\n", volt);
> >>               return volt;
> >>       }
> >>
> >>

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

Re: [PATCH v11 05/10] power: bq27xxx_battery: Make error reporting consistent

[Liam Breck]

On Thu, Mar 23, 2017 at 3:11 AM, Sebastian Reichel <sre@kernel.org> wrote:
> Hi,
>
> On Mon, Mar 20, 2017 at 10:59:18AM -0700, Liam Breck wrote:
>> On Mon, Mar 20, 2017 at 9:57 AM, Andrew F. Davis <afd@ti.com> wrote:
>> > On 03/20/2017 04:43 AM, Liam Breck wrote:
>> >> From: Liam Breck <kernel@networkimprov.net>
>> >>
>> >> Previously errors were reported in a mix of styles via both dev_err() & dev_dbg().
>> >> Report all errors with dev_err() and include error code in message.
>> >> Report register ID separately in dev_dbg() message.
>> >>
>> >> Signed-off-by: Liam Breck <kernel@networkimprov.net>
>> >> ---
>> >>  drivers/power/supply/bq27xxx_battery.c | 35 ++++++++++++++++++----------------
>> >>  1 file changed, 19 insertions(+), 16 deletions(-)
>> >>
>> >> diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
>> >> index 398801a..2e2a3a8 100644
>> >> --- a/drivers/power/supply/bq27xxx_battery.c
>> >> +++ b/drivers/power/supply/bq27xxx_battery.c
>> >> @@ -794,11 +794,17 @@ MODULE_PARM_DESC(poll_interval,
>> >>  static inline int bq27xxx_read(struct bq27xxx_device_info *di, int reg_index,
>> >>                              bool single)
>> >>  {
>> >> -     /* Reports EINVAL for invalid/missing registers */
>> >> +     int ret;
>> >> +
>> >>       if (!di || di->regs[reg_index] == INVALID_REG_ADDR)
>> >>               return -EINVAL;
>> >>
>> >> -     return di->bus.read(di, di->regs[reg_index], single);
>> >> +     ret = di->bus.read(di, di->regs[reg_index], single);
>> >> +     if (ret < 0)
>> >> +             dev_dbg(di->dev, "failed to read register 0x%02x (index %d)\n",
>> >> +                     di->regs[reg_index], reg_index);
>> >> +
>> >
>> > If we print out the error code here we don't have to do that in each of
>> > the below printouts.
>>
>> This is a dev_dbg, the below are dev_err, and I think you want error
>> code in dev_err. BTW the above addresses your request to learn which
>> register was in play in the event of a DM read/write failure.
>
> Just make it dev_err and also print ret. Then we can remove all
> other prints. That will result in some information-loss (name
> of the register), but

Andrew specifically asked for error reporting that gives the error
context elsewhere in the patchset. In complying with that request, I
filed this patch.

We could pass a context string into read() ...

> 1. it's not that important anyways. If you read battery voltage file
>    in sysfs, then obviously reading voltage fails and so on.
> 2. the error should be very unlikely on productive systems, since
>    it basically mean the i2c/1-wire transaction failed.
>
> -- Sebastian
>
>> > Otherwise looks good:
>> > Acked-by: Andrew F. Davis <afd@ti.com>
>> >
>> >> +     return ret;
>> >>  }
>> >>
>> >>  /*
>> >> @@ -815,7 +821,7 @@ static int bq27xxx_battery_read_soc(struct bq27xxx_device_info *di)
>> >>               soc = bq27xxx_read(di, BQ27XXX_REG_SOC, false);
>> >>
>> >>       if (soc < 0)
>> >> -             dev_dbg(di->dev, "error reading State-of-Charge\n");
>> >> +             dev_err(di->dev, "error %d reading state-of-charge\n", soc);
>> >>
>> >>       return soc;
>> >>  }
>> >> @@ -830,8 +836,7 @@ static int bq27xxx_battery_read_charge(struct bq27xxx_device_info *di, u8 reg)
>> >>
>> >>       charge = bq27xxx_read(di, reg, false);
>> >>       if (charge < 0) {
>> >> -             dev_dbg(di->dev, "error reading charge register %02x: %d\n",
>> >> -                     reg, charge);
>> >> +             dev_err(di->dev, "error %d reading charge\n", charge);
>> >>               return charge;
>> >>       }
>> >>
>> >> @@ -883,7 +888,7 @@ static int bq27xxx_battery_read_dcap(struct bq27xxx_device_info *di)
>> >>               dcap = bq27xxx_read(di, BQ27XXX_REG_DCAP, false);
>> >>
>> >>       if (dcap < 0) {
>> >> -             dev_dbg(di->dev, "error reading initial last measured discharge\n");
>> >> +             dev_err(di->dev, "error %d reading design capacity\n", dcap);
>> >>               return dcap;
>> >>       }
>> >>
>> >> @@ -905,7 +910,7 @@ static int bq27xxx_battery_read_energy(struct bq27xxx_device_info *di)
>> >>
>> >>       ae = bq27xxx_read(di, BQ27XXX_REG_AE, false);
>> >>       if (ae < 0) {
>> >> -             dev_dbg(di->dev, "error reading available energy\n");
>> >> +             dev_err(di->dev, "error %d reading available energy\n", ae);
>> >>               return ae;
>> >>       }
>> >>
>> >> @@ -927,7 +932,7 @@ static int bq27xxx_battery_read_temperature(struct bq27xxx_device_info *di)
>> >>
>> >>       temp = bq27xxx_read(di, BQ27XXX_REG_TEMP, false);
>> >>       if (temp < 0) {
>> >> -             dev_err(di->dev, "error reading temperature\n");
>> >> +             dev_err(di->dev, "error %d reading temperature\n", temp);
>> >>               return temp;
>> >>       }
>> >>
>> >> @@ -947,7 +952,7 @@ static int bq27xxx_battery_read_cyct(struct bq27xxx_device_info *di)
>> >>
>> >>       cyct = bq27xxx_read(di, BQ27XXX_REG_CYCT, false);
>> >>       if (cyct < 0)
>> >> -             dev_err(di->dev, "error reading cycle count total\n");
>> >> +             dev_err(di->dev, "error %d reading cycle count total\n", cyct);
>> >>
>> >>       return cyct;
>> >>  }
>> >> @@ -962,8 +967,7 @@ static int bq27xxx_battery_read_time(struct bq27xxx_device_info *di, u8 reg)
>> >>
>> >>       tval = bq27xxx_read(di, reg, false);
>> >>       if (tval < 0) {
>> >> -             dev_dbg(di->dev, "error reading time register %02x: %d\n",
>> >> -                     reg, tval);
>> >> +             dev_err(di->dev, "error %d reading time\n", tval);
>> >>               return tval;
>> >>       }
>> >>
>> >> @@ -983,8 +987,7 @@ static int bq27xxx_battery_read_pwr_avg(struct bq27xxx_device_info *di)
>> >>
>> >>       tval = bq27xxx_read(di, BQ27XXX_REG_AP, false);
>> >>       if (tval < 0) {
>> >> -             dev_err(di->dev, "error reading average power register  %02x: %d\n",
>> >> -                     BQ27XXX_REG_AP, tval);
>> >> +             dev_err(di->dev, "error %d reading average power\n", tval);
>> >>               return tval;
>> >>       }
>> >>
>> >> @@ -1054,7 +1057,7 @@ static int bq27xxx_battery_read_health(struct bq27xxx_device_info *di)
>> >>
>> >>       flags = bq27xxx_read(di, BQ27XXX_REG_FLAGS, has_singe_flag);
>> >>       if (flags < 0) {
>> >> -             dev_err(di->dev, "error reading flag register:%d\n", flags);
>> >> +             dev_err(di->dev, "error %d reading flags\n", flags);
>> >>               return flags;
>> >>       }
>> >>
>> >> @@ -1147,7 +1150,7 @@ static int bq27xxx_battery_current(struct bq27xxx_device_info *di,
>> >>
>> >>       curr = bq27xxx_read(di, BQ27XXX_REG_AI, false);
>> >>       if (curr < 0) {
>> >> -             dev_err(di->dev, "error reading current\n");
>> >> +             dev_err(di->dev, "error %d reading current\n", curr);
>> >>               return curr;
>> >>       }
>> >>
>> >> @@ -1236,7 +1239,7 @@ static int bq27xxx_battery_voltage(struct bq27xxx_device_info *di,
>> >>
>> >>       volt = bq27xxx_read(di, BQ27XXX_REG_VOLT, false);
>> >>       if (volt < 0) {
>> >> -             dev_err(di->dev, "error reading voltage\n");
>> >> +             dev_err(di->dev, "error %d reading voltage\n", volt);
>> >>               return volt;
>> >>       }
>> >>
>> >>

Re: [PATCH v11 05/10] power: bq27xxx_battery: Make error reporting consistent

[Sebastian Reichel]

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

Hi,

On Thu, Mar 23, 2017 at 03:24:40AM -0700, Liam Breck wrote:
> On Thu, Mar 23, 2017 at 3:11 AM, Sebastian Reichel <sre@kernel.org> wrote:
> > On Mon, Mar 20, 2017 at 10:59:18AM -0700, Liam Breck wrote:
> >> On Mon, Mar 20, 2017 at 9:57 AM, Andrew F. Davis <afd@ti.com> wrote:
> >> > On 03/20/2017 04:43 AM, Liam Breck wrote:
> >> >> From: Liam Breck <kernel@networkimprov.net>
> >> >>
> >> >> Previously errors were reported in a mix of styles via both dev_err() & dev_dbg().
> >> >> Report all errors with dev_err() and include error code in message.
> >> >> Report register ID separately in dev_dbg() message.
> >> >>
> >> >> Signed-off-by: Liam Breck <kernel@networkimprov.net>
> >> >> ---
> >> >>  drivers/power/supply/bq27xxx_battery.c | 35 ++++++++++++++++++----------------
> >> >>  1 file changed, 19 insertions(+), 16 deletions(-)
> >> >>
> >> >> diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
> >> >> index 398801a..2e2a3a8 100644
> >> >> --- a/drivers/power/supply/bq27xxx_battery.c
> >> >> +++ b/drivers/power/supply/bq27xxx_battery.c
> >> >> @@ -794,11 +794,17 @@ MODULE_PARM_DESC(poll_interval,
> >> >>  static inline int bq27xxx_read(struct bq27xxx_device_info *di, int reg_index,
> >> >>                              bool single)
> >> >>  {
> >> >> -     /* Reports EINVAL for invalid/missing registers */
> >> >> +     int ret;
> >> >> +
> >> >>       if (!di || di->regs[reg_index] == INVALID_REG_ADDR)
> >> >>               return -EINVAL;
> >> >>
> >> >> -     return di->bus.read(di, di->regs[reg_index], single);
> >> >> +     ret = di->bus.read(di, di->regs[reg_index], single);
> >> >> +     if (ret < 0)
> >> >> +             dev_dbg(di->dev, "failed to read register 0x%02x (index %d)\n",
> >> >> +                     di->regs[reg_index], reg_index);
> >> >> +
> >> >
> >> > If we print out the error code here we don't have to do that in each of
> >> > the below printouts.
> >>
> >> This is a dev_dbg, the below are dev_err, and I think you want error
> >> code in dev_err. BTW the above addresses your request to learn which
> >> register was in play in the event of a DM read/write failure.
> >
> > Just make it dev_err and also print ret. Then we can remove all
> > other prints. That will result in some information-loss (name
> > of the register), but
> 
> Andrew specifically asked for error reporting that gives the error
> context elsewhere in the patchset. In complying with that request, I
> filed this patch.
> 
> We could pass a context string into read() ...

That's fine with me.

-- Sebastian

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

Re: [PATCH v11 05/10] power: bq27xxx_battery: Make error reporting consistent

[Liam Breck]

On Thu, Mar 23, 2017 at 3:33 AM, Sebastian Reichel <sre@kernel.org> wrote:
> Hi,
>
> On Thu, Mar 23, 2017 at 03:24:40AM -0700, Liam Breck wrote:
>> On Thu, Mar 23, 2017 at 3:11 AM, Sebastian Reichel <sre@kernel.org> wrote:
>> > On Mon, Mar 20, 2017 at 10:59:18AM -0700, Liam Breck wrote:
>> >> On Mon, Mar 20, 2017 at 9:57 AM, Andrew F. Davis <afd@ti.com> wrote:
>> >> > On 03/20/2017 04:43 AM, Liam Breck wrote:
>> >> >> From: Liam Breck <kernel@networkimprov.net>
>> >> >>
>> >> >> Previously errors were reported in a mix of styles via both dev_err() & dev_dbg().
>> >> >> Report all errors with dev_err() and include error code in message.
>> >> >> Report register ID separately in dev_dbg() message.
>> >> >>
>> >> >> Signed-off-by: Liam Breck <kernel@networkimprov.net>
>> >> >> ---
>> >> >>  drivers/power/supply/bq27xxx_battery.c | 35 ++++++++++++++++++----------------
>> >> >>  1 file changed, 19 insertions(+), 16 deletions(-)
>> >> >>
>> >> >> diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
>> >> >> index 398801a..2e2a3a8 100644
>> >> >> --- a/drivers/power/supply/bq27xxx_battery.c
>> >> >> +++ b/drivers/power/supply/bq27xxx_battery.c
>> >> >> @@ -794,11 +794,17 @@ MODULE_PARM_DESC(poll_interval,
>> >> >>  static inline int bq27xxx_read(struct bq27xxx_device_info *di, int reg_index,
>> >> >>                              bool single)
>> >> >>  {
>> >> >> -     /* Reports EINVAL for invalid/missing registers */
>> >> >> +     int ret;
>> >> >> +
>> >> >>       if (!di || di->regs[reg_index] == INVALID_REG_ADDR)
>> >> >>               return -EINVAL;
>> >> >>
>> >> >> -     return di->bus.read(di, di->regs[reg_index], single);
>> >> >> +     ret = di->bus.read(di, di->regs[reg_index], single);
>> >> >> +     if (ret < 0)
>> >> >> +             dev_dbg(di->dev, "failed to read register 0x%02x (index %d)\n",
>> >> >> +                     di->regs[reg_index], reg_index);
>> >> >> +
>> >> >
>> >> > If we print out the error code here we don't have to do that in each of
>> >> > the below printouts.
>> >>
>> >> This is a dev_dbg, the below are dev_err, and I think you want error
>> >> code in dev_err. BTW the above addresses your request to learn which
>> >> register was in play in the event of a DM read/write failure.
>> >
>> > Just make it dev_err and also print ret. Then we can remove all
>> > other prints. That will result in some information-loss (name
>> > of the register), but
>>
>> Andrew specifically asked for error reporting that gives the error
>> context elsewhere in the patchset. In complying with that request, I
>> filed this patch.
>>
>> We could pass a context string into read() ...
>
> That's fine with me.

And it's really outside the scope of this patchset. Let Andrew do that
after this goes in :-)

[PATCH v11 06/10] power: bq27xxx_battery: Define access methods to write chip registers

[Liam Breck]

From: Liam Breck <kernel@networkimprov.net>

Declare bus.write/read_bulk/write_bulk() to support get/set of chip registers,
e.g. with inputs from power_supply_battery_info.

Add bq27xxx_write() & bq27xxx_xfer() helper functions to call the above.

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

diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
index 2e2a3a8..f36a6f1 100644
--- a/drivers/power/supply/bq27xxx_battery.c
+++ b/drivers/power/supply/bq27xxx_battery.c
@@ -807,6 +807,41 @@ static inline int bq27xxx_read(struct bq27xxx_device_info *di, int reg_index,
 	return ret;
 }
 
+static inline int bq27xxx_write(struct bq27xxx_device_info *di, int reg_index,
+				u16 value, bool single)
+{
+	int ret;
+
+	if (!di || di->regs[reg_index] == INVALID_REG_ADDR)
+		return -EINVAL;
+
+	ret = di->bus.write(di, di->regs[reg_index], value, single);
+	if (ret < 0)
+		dev_dbg(di->dev, "failed to write register 0x%02x (index %d)\n",
+			di->regs[reg_index], reg_index);
+
+	return ret;
+}
+
+static inline int bq27xxx_xfer(struct bq27xxx_device_info *di,
+			       struct bq27xxx_dm_buf *buf, bool read)
+{
+	int ret;
+	int (*xfer)(struct bq27xxx_device_info*, u8, u8*, int) =
+		read ? di->bus.read_bulk : di->bus.write_bulk;
+
+	if (!di || di->regs[BQ27XXX_DM_DATA] == INVALID_REG_ADDR)
+		return -EINVAL;
+
+	ret = xfer(di, di->regs[BQ27XXX_DM_DATA], buf->a, BQ27XXX_DM_SZ);
+	if (ret < 0)
+		dev_dbg(di->dev, "failed to %s_bulk register 0x%02x (index %d)\n",
+			read ? "read" : "write",
+			di->regs[BQ27XXX_DM_DATA], BQ27XXX_DM_DATA);
+
+	return ret;
+}
+
 /*
  * Return the battery State-of-Charge
  * Or < 0 if something fails.
diff --git a/include/linux/power/bq27xxx_battery.h b/include/linux/power/bq27xxx_battery.h
index b312bce..c3369fa 100644
--- a/include/linux/power/bq27xxx_battery.h
+++ b/include/linux/power/bq27xxx_battery.h
@@ -40,6 +40,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

Re: [PATCH v11 06/10] power: bq27xxx_battery: Define access methods to write chip registers

[kbuild test robot]

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

Hi Liam,

[auto build test ERROR on linus/master]
[also build test ERROR on v4.11-rc3 next-20170321]
[cannot apply to battery/master]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Liam-Breck/devicetree-battery-support-and-client-bq27xxx_battery/20170322-041314
config: i386-randconfig-x002-201712 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

Note: the linux-review/Liam-Breck/devicetree-battery-support-and-client-bq27xxx_battery/20170322-041314 HEAD 0e1428c6f71feb34dbef1ed69f5612af92cd1dbb builds fine.
      It only hurts bisectibility.

All error/warnings (new ones prefixed by >>):

>> drivers/power/supply/bq27xxx_battery.c:827:18: warning: 'struct bq27xxx_dm_buf' declared inside parameter list will not be visible outside of this definition or declaration
              struct bq27xxx_dm_buf *buf, bool read)
                     ^~~~~~~~~~~~~~
   drivers/power/supply/bq27xxx_battery.c: In function 'bq27xxx_xfer':
>> drivers/power/supply/bq27xxx_battery.c:833:22: error: 'BQ27XXX_DM_DATA' undeclared (first use in this function)
     if (!di || di->regs[BQ27XXX_DM_DATA] == INVALID_REG_ADDR)
                         ^~~~~~~~~~~~~~~
   drivers/power/supply/bq27xxx_battery.c:833:22: note: each undeclared identifier is reported only once for each function it appears in
>> drivers/power/supply/bq27xxx_battery.c:836:47: error: dereferencing pointer to incomplete type 'struct bq27xxx_dm_buf'
     ret = xfer(di, di->regs[BQ27XXX_DM_DATA], buf->a, BQ27XXX_DM_SZ);
                                                  ^~
>> drivers/power/supply/bq27xxx_battery.c:836:52: error: 'BQ27XXX_DM_SZ' undeclared (first use in this function)
     ret = xfer(di, di->regs[BQ27XXX_DM_DATA], buf->a, BQ27XXX_DM_SZ);
                                                       ^~~~~~~~~~~~~

vim +/BQ27XXX_DM_DATA +833 drivers/power/supply/bq27xxx_battery.c

   821				di->regs[reg_index], reg_index);
   822	
   823		return ret;
   824	}
   825	
   826	static inline int bq27xxx_xfer(struct bq27xxx_device_info *di,
 > 827				       struct bq27xxx_dm_buf *buf, bool read)
   828	{
   829		int ret;
   830		int (*xfer)(struct bq27xxx_device_info*, u8, u8*, int) =
   831			read ? di->bus.read_bulk : di->bus.write_bulk;
   832	
 > 833		if (!di || di->regs[BQ27XXX_DM_DATA] == INVALID_REG_ADDR)
   834			return -EINVAL;
   835	
 > 836		ret = xfer(di, di->regs[BQ27XXX_DM_DATA], buf->a, BQ27XXX_DM_SZ);
   837		if (ret < 0)
   838			dev_dbg(di->dev, "failed to %s_bulk register 0x%02x (index %d)\n",
   839				read ? "read" : "write",

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 21215 bytes --]

Re: [PATCH v11 06/10] power: bq27xxx_battery: Define access methods to write chip registers

[Andrew F. Davis]

On 03/20/2017 04:43 AM, Liam Breck wrote:
> From: Liam Breck <kernel@networkimprov.net>
> 
> Declare bus.write/read_bulk/write_bulk() to support get/set of chip registers,
> e.g. with inputs from power_supply_battery_info.
> 
> Add bq27xxx_write() & bq27xxx_xfer() helper functions to call the above.
> 
> Signed-off-by: Matt Ranostay <matt@ranostay.consulting>
> Signed-off-by: Liam Breck <kernel@networkimprov.net>
> ---
>  drivers/power/supply/bq27xxx_battery.c | 35 ++++++++++++++++++++++++++++++++++
>  include/linux/power/bq27xxx_battery.h  |  3 +++
>  2 files changed, 38 insertions(+)
> 
> diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
> index 2e2a3a8..f36a6f1 100644
> --- a/drivers/power/supply/bq27xxx_battery.c
> +++ b/drivers/power/supply/bq27xxx_battery.c
> @@ -807,6 +807,41 @@ static inline int bq27xxx_read(struct bq27xxx_device_info *di, int reg_index,
>  	return ret;
>  }
>  
> +static inline int bq27xxx_write(struct bq27xxx_device_info *di, int reg_index,
> +				u16 value, bool single)
> +{
> +	int ret;
> +
> +	if (!di || di->regs[reg_index] == INVALID_REG_ADDR)
> +		return -EINVAL;
> +
> +	ret = di->bus.write(di, di->regs[reg_index], value, single);

You should check that .write has been defined, it will not be for the
hdq bus.

> +	if (ret < 0)
> +		dev_dbg(di->dev, "failed to write register 0x%02x (index %d)\n",
> +			di->regs[reg_index], reg_index);
> +
> +	return ret;
> +}
> +
> +static inline int bq27xxx_xfer(struct bq27xxx_device_info *di,
> +			       struct bq27xxx_dm_buf *buf, bool read)
> +{
> +	int ret;
> +	int (*xfer)(struct bq27xxx_device_info*, u8, u8*, int) =
> +		read ? di->bus.read_bulk : di->bus.write_bulk;
> +
> +	if (!di || di->regs[BQ27XXX_DM_DATA] == INVALID_REG_ADDR)
> +		return -EINVAL;
> +
> +	ret = xfer(di, di->regs[BQ27XXX_DM_DATA], buf->a, BQ27XXX_DM_SZ);
> +	if (ret < 0)
> +		dev_dbg(di->dev, "failed to %s_bulk register 0x%02x (index %d)\n",
> +			read ? "read" : "write",
> +			di->regs[BQ27XXX_DM_DATA], BQ27XXX_DM_DATA);
> +
> +	return ret;
> +}
> +
>  /*
>   * Return the battery State-of-Charge
>   * Or < 0 if something fails.
> diff --git a/include/linux/power/bq27xxx_battery.h b/include/linux/power/bq27xxx_battery.h
> index b312bce..c3369fa 100644
> --- a/include/linux/power/bq27xxx_battery.h
> +++ b/include/linux/power/bq27xxx_battery.h
> @@ -40,6 +40,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 {
> 

[PATCH v11 07/10] power: bq27xxx_battery: Keep track of specific chip id

[Liam Breck]

From: Liam Breck <kernel@networkimprov.net>

Pass actual chip ID into _setup(), which translates it to a group ID,
to allow support for all chips by the power_supply_battery_info code.
There are no functional changes to the driver.

Signed-off-by: Liam Breck <kernel@networkimprov.net>
---
 drivers/power/supply/bq27xxx_battery.c     | 27 +++++++++++++++++++++++++++
 drivers/power/supply/bq27xxx_battery_i2c.c | 16 ++++++++--------
 include/linux/power/bq27xxx_battery.h      | 11 +++++++++++
 3 files changed, 46 insertions(+), 8 deletions(-)

diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
index f36a6f1..db1a388 100644
--- a/drivers/power/supply/bq27xxx_battery.c
+++ b/drivers/power/supply/bq27xxx_battery.c
@@ -1391,6 +1391,33 @@ int bq27xxx_battery_setup(struct bq27xxx_device_info *di)
 	struct power_supply_desc *psy_desc;
 	struct power_supply_config psy_cfg = { .drv_data = di, };
 
+	switch(di->chip) {
+	case BQ27000:
+	case BQ27010:
+	case BQ2750X:
+	case BQ2751X:
+	case BQ27500:
+	case BQ27510G1:
+	case BQ27510G2:
+	case BQ27510G3:
+	case BQ27520G1:
+	case BQ27520G2:
+	case BQ27520G3:
+	case BQ27520G4:
+	case BQ27530:
+	case BQ27541:
+	case BQ27545:
+	case BQ27421: break;
+	case BQ2752X: di->chip = BQ2751X; break;
+	case BQ27531: di->chip = BQ27530; break;
+	case BQ27542: di->chip = BQ27541; break;
+	case BQ27546: di->chip = BQ27541; break;
+	case BQ27742: di->chip = BQ27541; break;
+	case BQ27425: di->chip = BQ27421; break;
+	case BQ27441: di->chip = BQ27421; break;
+	case BQ27621: di->chip = BQ27421; break;
+	}
+
 	INIT_DELAYED_WORK(&di->work, bq27xxx_battery_poll);
 	mutex_init(&di->lock);
 	di->regs = bq27xxx_regs[di->chip];
diff --git a/drivers/power/supply/bq27xxx_battery_i2c.c b/drivers/power/supply/bq27xxx_battery_i2c.c
index c68fbc3..b3f2494 100644
--- a/drivers/power/supply/bq27xxx_battery_i2c.c
+++ b/drivers/power/supply/bq27xxx_battery_i2c.c
@@ -150,7 +150,7 @@ static const struct i2c_device_id bq27xxx_i2c_id_table[] = {
 	{ "bq27210", BQ27010 },
 	{ "bq27500", BQ2750X },
 	{ "bq27510", BQ2751X },
-	{ "bq27520", BQ2751X },
+	{ "bq27520", BQ2752X },
 	{ "bq27500-1", BQ27500 },
 	{ "bq27510g1", BQ27510G1 },
 	{ "bq27510g2", BQ27510G2 },
@@ -160,16 +160,16 @@ static const struct i2c_device_id bq27xxx_i2c_id_table[] = {
 	{ "bq27520g3", BQ27520G3 },
 	{ "bq27520g4", BQ27520G4 },
 	{ "bq27530", BQ27530 },
-	{ "bq27531", BQ27530 },
+	{ "bq27531", BQ27531 },
 	{ "bq27541", BQ27541 },
-	{ "bq27542", BQ27541 },
-	{ "bq27546", BQ27541 },
-	{ "bq27742", BQ27541 },
+	{ "bq27542", BQ27542 },
+	{ "bq27546", BQ27546 },
+	{ "bq27742", BQ27742 },
 	{ "bq27545", BQ27545 },
 	{ "bq27421", BQ27421 },
-	{ "bq27425", BQ27421 },
-	{ "bq27441", BQ27421 },
-	{ "bq27621", BQ27421 },
+	{ "bq27425", BQ27425 },
+	{ "bq27441", BQ27441 },
+	{ "bq27621", BQ27621 },
 	{},
 };
 MODULE_DEVICE_TABLE(i2c, bq27xxx_i2c_id_table);
diff --git a/include/linux/power/bq27xxx_battery.h b/include/linux/power/bq27xxx_battery.h
index c3369fa..96cec17 100644
--- a/include/linux/power/bq27xxx_battery.h
+++ b/include/linux/power/bq27xxx_battery.h
@@ -2,6 +2,7 @@
 #define __LINUX_BQ27X00_BATTERY_H__
 
 enum bq27xxx_chip {
+	/* categories; index for bq27xxx_regs[] */
 	BQ27000 = 1, /* bq27000, bq27200 */
 	BQ27010, /* bq27010, bq27210 */
 	BQ2750X, /* bq27500 deprecated alias */
@@ -18,6 +19,16 @@ enum bq27xxx_chip {
 	BQ27541, /* bq27541, bq27542, bq27546, bq27742 */
 	BQ27545, /* bq27545 */
 	BQ27421, /* bq27421, bq27425, bq27441, bq27621 */
+
+	/* members of categories; translate these to category in _setup() */
+	BQ2752X, /* deprecated alias */
+	BQ27531,
+	BQ27542,
+	BQ27546,
+	BQ27742,
+	BQ27425,
+	BQ27441,
+	BQ27621,
 };
 
 /**
-- 
2.9.3

Re: [PATCH v11 07/10] power: bq27xxx_battery: Keep track of specific chip id

[Sebastian Reichel]

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

Hi,

On Mon, Mar 20, 2017 at 02:43:32AM -0700, Liam Breck wrote:
> From: Liam Breck <kernel@networkimprov.net>
> 
> Pass actual chip ID into _setup(), which translates it to a group ID,
> to allow support for all chips by the power_supply_battery_info code.
> There are no functional changes to the driver.
> 
> Signed-off-by: Liam Breck <kernel@networkimprov.net>

This is really ugly. If we need chip ID and group ID let's store
them in different variables. For example put the detailed chipid
into di->realchip and then do

switch(di->realchip) {
case FOO:
    di->chip = GRP_FOO;
    break;
case BAR:
    di->chip = GRP_BAR;
    break;
default:
    di->chip = di->realchip;
    break;
}

-- Sebastian

>  drivers/power/supply/bq27xxx_battery.c     | 27 +++++++++++++++++++++++++++
>  drivers/power/supply/bq27xxx_battery_i2c.c | 16 ++++++++--------
>  include/linux/power/bq27xxx_battery.h      | 11 +++++++++++
>  3 files changed, 46 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
> index f36a6f1..db1a388 100644
> --- a/drivers/power/supply/bq27xxx_battery.c
> +++ b/drivers/power/supply/bq27xxx_battery.c
> @@ -1391,6 +1391,33 @@ int bq27xxx_battery_setup(struct bq27xxx_device_info *di)
>  	struct power_supply_desc *psy_desc;
>  	struct power_supply_config psy_cfg = { .drv_data = di, };
>  
> +	switch(di->chip) {
> +	case BQ27000:
> +	case BQ27010:
> +	case BQ2750X:
> +	case BQ2751X:
> +	case BQ27500:
> +	case BQ27510G1:
> +	case BQ27510G2:
> +	case BQ27510G3:
> +	case BQ27520G1:
> +	case BQ27520G2:
> +	case BQ27520G3:
> +	case BQ27520G4:
> +	case BQ27530:
> +	case BQ27541:
> +	case BQ27545:
> +	case BQ27421: break;
> +	case BQ2752X: di->chip = BQ2751X; break;
> +	case BQ27531: di->chip = BQ27530; break;
> +	case BQ27542: di->chip = BQ27541; break;
> +	case BQ27546: di->chip = BQ27541; break;
> +	case BQ27742: di->chip = BQ27541; break;
> +	case BQ27425: di->chip = BQ27421; break;
> +	case BQ27441: di->chip = BQ27421; break;
> +	case BQ27621: di->chip = BQ27421; break;
> +	}
> +
>  	INIT_DELAYED_WORK(&di->work, bq27xxx_battery_poll);
>  	mutex_init(&di->lock);
>  	di->regs = bq27xxx_regs[di->chip];
> diff --git a/drivers/power/supply/bq27xxx_battery_i2c.c b/drivers/power/supply/bq27xxx_battery_i2c.c
> index c68fbc3..b3f2494 100644
> --- a/drivers/power/supply/bq27xxx_battery_i2c.c
> +++ b/drivers/power/supply/bq27xxx_battery_i2c.c
> @@ -150,7 +150,7 @@ static const struct i2c_device_id bq27xxx_i2c_id_table[] = {
>  	{ "bq27210", BQ27010 },
>  	{ "bq27500", BQ2750X },
>  	{ "bq27510", BQ2751X },
> -	{ "bq27520", BQ2751X },
> +	{ "bq27520", BQ2752X },
>  	{ "bq27500-1", BQ27500 },
>  	{ "bq27510g1", BQ27510G1 },
>  	{ "bq27510g2", BQ27510G2 },
> @@ -160,16 +160,16 @@ static const struct i2c_device_id bq27xxx_i2c_id_table[] = {
>  	{ "bq27520g3", BQ27520G3 },
>  	{ "bq27520g4", BQ27520G4 },
>  	{ "bq27530", BQ27530 },
> -	{ "bq27531", BQ27530 },
> +	{ "bq27531", BQ27531 },
>  	{ "bq27541", BQ27541 },
> -	{ "bq27542", BQ27541 },
> -	{ "bq27546", BQ27541 },
> -	{ "bq27742", BQ27541 },
> +	{ "bq27542", BQ27542 },
> +	{ "bq27546", BQ27546 },
> +	{ "bq27742", BQ27742 },
>  	{ "bq27545", BQ27545 },
>  	{ "bq27421", BQ27421 },
> -	{ "bq27425", BQ27421 },
> -	{ "bq27441", BQ27421 },
> -	{ "bq27621", BQ27421 },
> +	{ "bq27425", BQ27425 },
> +	{ "bq27441", BQ27441 },
> +	{ "bq27621", BQ27621 },
>  	{},
>  };
>  MODULE_DEVICE_TABLE(i2c, bq27xxx_i2c_id_table);
> diff --git a/include/linux/power/bq27xxx_battery.h b/include/linux/power/bq27xxx_battery.h
> index c3369fa..96cec17 100644
> --- a/include/linux/power/bq27xxx_battery.h
> +++ b/include/linux/power/bq27xxx_battery.h
> @@ -2,6 +2,7 @@
>  #define __LINUX_BQ27X00_BATTERY_H__
>  
>  enum bq27xxx_chip {
> +	/* categories; index for bq27xxx_regs[] */
>  	BQ27000 = 1, /* bq27000, bq27200 */
>  	BQ27010, /* bq27010, bq27210 */
>  	BQ2750X, /* bq27500 deprecated alias */
> @@ -18,6 +19,16 @@ enum bq27xxx_chip {
>  	BQ27541, /* bq27541, bq27542, bq27546, bq27742 */
>  	BQ27545, /* bq27545 */
>  	BQ27421, /* bq27421, bq27425, bq27441, bq27621 */
> +
> +	/* members of categories; translate these to category in _setup() */
> +	BQ2752X, /* deprecated alias */
> +	BQ27531,
> +	BQ27542,
> +	BQ27546,
> +	BQ27742,
> +	BQ27425,
> +	BQ27441,
> +	BQ27621,
>  };
>  
>  /**
> -- 
> 2.9.3
> 

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

Re: [PATCH v11 07/10] power: bq27xxx_battery: Keep track of specific chip id

[Liam Breck]

On Thu, Mar 23, 2017 at 3:28 AM, Sebastian Reichel <sre@kernel.org> wrote:
> Hi,
>
> On Mon, Mar 20, 2017 at 02:43:32AM -0700, Liam Breck wrote:
>> From: Liam Breck <kernel@networkimprov.net>
>>
>> Pass actual chip ID into _setup(), which translates it to a group ID,
>> to allow support for all chips by the power_supply_battery_info code.
>> There are no functional changes to the driver.
>>
>> Signed-off-by: Liam Breck <kernel@networkimprov.net>
>
> This is really ugly. If we need chip ID and group ID let's store
> them in different variables. For example put the detailed chipid
> into di->realchip and then do

I tried this in a previous version, and Andrew rejected it as
confusing to have two IDs. That was while you were away on business
:-)

> switch(di->realchip) {
> case FOO:
>     di->chip = GRP_FOO;
>     break;
> case BAR:
>     di->chip = GRP_BAR;
>     break;
> default:
>     di->chip = di->realchip;
>     break;
> }
>
> -- Sebastian
>
>>  drivers/power/supply/bq27xxx_battery.c     | 27 +++++++++++++++++++++++++++
>>  drivers/power/supply/bq27xxx_battery_i2c.c | 16 ++++++++--------
>>  include/linux/power/bq27xxx_battery.h      | 11 +++++++++++
>>  3 files changed, 46 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
>> index f36a6f1..db1a388 100644
>> --- a/drivers/power/supply/bq27xxx_battery.c
>> +++ b/drivers/power/supply/bq27xxx_battery.c
>> @@ -1391,6 +1391,33 @@ int bq27xxx_battery_setup(struct bq27xxx_device_info *di)
>>       struct power_supply_desc *psy_desc;
>>       struct power_supply_config psy_cfg = { .drv_data = di, };
>>
>> +     switch(di->chip) {
>> +     case BQ27000:
>> +     case BQ27010:
>> +     case BQ2750X:
>> +     case BQ2751X:
>> +     case BQ27500:
>> +     case BQ27510G1:
>> +     case BQ27510G2:
>> +     case BQ27510G3:
>> +     case BQ27520G1:
>> +     case BQ27520G2:
>> +     case BQ27520G3:
>> +     case BQ27520G4:
>> +     case BQ27530:
>> +     case BQ27541:
>> +     case BQ27545:
>> +     case BQ27421: break;
>> +     case BQ2752X: di->chip = BQ2751X; break;
>> +     case BQ27531: di->chip = BQ27530; break;
>> +     case BQ27542: di->chip = BQ27541; break;
>> +     case BQ27546: di->chip = BQ27541; break;
>> +     case BQ27742: di->chip = BQ27541; break;
>> +     case BQ27425: di->chip = BQ27421; break;
>> +     case BQ27441: di->chip = BQ27421; break;
>> +     case BQ27621: di->chip = BQ27421; break;
>> +     }
>> +
>>       INIT_DELAYED_WORK(&di->work, bq27xxx_battery_poll);
>>       mutex_init(&di->lock);
>>       di->regs = bq27xxx_regs[di->chip];
>> diff --git a/drivers/power/supply/bq27xxx_battery_i2c.c b/drivers/power/supply/bq27xxx_battery_i2c.c
>> index c68fbc3..b3f2494 100644
>> --- a/drivers/power/supply/bq27xxx_battery_i2c.c
>> +++ b/drivers/power/supply/bq27xxx_battery_i2c.c
>> @@ -150,7 +150,7 @@ static const struct i2c_device_id bq27xxx_i2c_id_table[] = {
>>       { "bq27210", BQ27010 },
>>       { "bq27500", BQ2750X },
>>       { "bq27510", BQ2751X },
>> -     { "bq27520", BQ2751X },
>> +     { "bq27520", BQ2752X },
>>       { "bq27500-1", BQ27500 },
>>       { "bq27510g1", BQ27510G1 },
>>       { "bq27510g2", BQ27510G2 },
>> @@ -160,16 +160,16 @@ static const struct i2c_device_id bq27xxx_i2c_id_table[] = {
>>       { "bq27520g3", BQ27520G3 },
>>       { "bq27520g4", BQ27520G4 },
>>       { "bq27530", BQ27530 },
>> -     { "bq27531", BQ27530 },
>> +     { "bq27531", BQ27531 },
>>       { "bq27541", BQ27541 },
>> -     { "bq27542", BQ27541 },
>> -     { "bq27546", BQ27541 },
>> -     { "bq27742", BQ27541 },
>> +     { "bq27542", BQ27542 },
>> +     { "bq27546", BQ27546 },
>> +     { "bq27742", BQ27742 },
>>       { "bq27545", BQ27545 },
>>       { "bq27421", BQ27421 },
>> -     { "bq27425", BQ27421 },
>> -     { "bq27441", BQ27421 },
>> -     { "bq27621", BQ27421 },
>> +     { "bq27425", BQ27425 },
>> +     { "bq27441", BQ27441 },
>> +     { "bq27621", BQ27621 },
>>       {},
>>  };
>>  MODULE_DEVICE_TABLE(i2c, bq27xxx_i2c_id_table);
>> diff --git a/include/linux/power/bq27xxx_battery.h b/include/linux/power/bq27xxx_battery.h
>> index c3369fa..96cec17 100644
>> --- a/include/linux/power/bq27xxx_battery.h
>> +++ b/include/linux/power/bq27xxx_battery.h
>> @@ -2,6 +2,7 @@
>>  #define __LINUX_BQ27X00_BATTERY_H__
>>
>>  enum bq27xxx_chip {
>> +     /* categories; index for bq27xxx_regs[] */
>>       BQ27000 = 1, /* bq27000, bq27200 */
>>       BQ27010, /* bq27010, bq27210 */
>>       BQ2750X, /* bq27500 deprecated alias */
>> @@ -18,6 +19,16 @@ enum bq27xxx_chip {
>>       BQ27541, /* bq27541, bq27542, bq27546, bq27742 */
>>       BQ27545, /* bq27545 */
>>       BQ27421, /* bq27421, bq27425, bq27441, bq27621 */
>> +
>> +     /* members of categories; translate these to category in _setup() */
>> +     BQ2752X, /* deprecated alias */
>> +     BQ27531,
>> +     BQ27542,
>> +     BQ27546,
>> +     BQ27742,
>> +     BQ27425,
>> +     BQ27441,
>> +     BQ27621,
>>  };
>>
>>  /**
>> --
>> 2.9.3
>>

Re: [PATCH v11 07/10] power: bq27xxx_battery: Keep track of specific chip id

[Sebastian Reichel]

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

Hi,

On Thu, Mar 23, 2017 at 03:35:33AM -0700, Liam Breck wrote:
> On Thu, Mar 23, 2017 at 3:28 AM, Sebastian Reichel <sre@kernel.org> wrote:
> > Hi,
> >
> > On Mon, Mar 20, 2017 at 02:43:32AM -0700, Liam Breck wrote:
> >> From: Liam Breck <kernel@networkimprov.net>
> >>
> >> Pass actual chip ID into _setup(), which translates it to a group ID,
> >> to allow support for all chips by the power_supply_battery_info code.
> >> There are no functional changes to the driver.
> >>
> >> Signed-off-by: Liam Breck <kernel@networkimprov.net>
> >
> > This is really ugly. If we need chip ID and group ID let's store
> > them in different variables. For example put the detailed chipid
> > into di->realchip and then do
> 
> I tried this in a previous version, and Andrew rejected it as
> confusing to have two IDs. That was while you were away on business
> :-)

now we have two IDs in the same variable.
That definitely more confusing. We can name the second variable
group to reduce confusion.

-- Sebastian

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

Re: [PATCH v11 07/10] power: bq27xxx_battery: Keep track of specific chip id

[Liam Breck]

On Thu, Mar 23, 2017 at 5:30 AM, Sebastian Reichel <sre@kernel.org> wrote:
> Hi,
>
> On Thu, Mar 23, 2017 at 03:35:33AM -0700, Liam Breck wrote:
>> On Thu, Mar 23, 2017 at 3:28 AM, Sebastian Reichel <sre@kernel.org> wrote:
>> > Hi,
>> >
>> > On Mon, Mar 20, 2017 at 02:43:32AM -0700, Liam Breck wrote:
>> >> From: Liam Breck <kernel@networkimprov.net>
>> >>
>> >> Pass actual chip ID into _setup(), which translates it to a group ID,
>> >> to allow support for all chips by the power_supply_battery_info code.
>> >> There are no functional changes to the driver.
>> >>
>> >> Signed-off-by: Liam Breck <kernel@networkimprov.net>
>> >
>> > This is really ugly. If we need chip ID and group ID let's store
>> > them in different variables. For example put the detailed chipid
>> > into di->realchip and then do
>>
>> I tried this in a previous version, and Andrew rejected it as
>> confusing to have two IDs. That was while you were away on business
>> :-)
>
> now we have two IDs in the same variable.
> That definitely more confusing. We can name the second variable
> group to reduce confusion.

I was trying not to rename every instance of di->chip; there's a lot.

But we can pass real_chip as an argument to this function, so that
gets you what you wanted.

Re: [PATCH v11 07/10] power: bq27xxx_battery: Keep track of specific chip id

[Sebastian Reichel]

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

Hi,

On Thu, Mar 23, 2017 at 05:39:09AM -0700, Liam Breck wrote:
> On Thu, Mar 23, 2017 at 5:30 AM, Sebastian Reichel <sre@kernel.org> wrote:
> > Hi,
> >
> > On Thu, Mar 23, 2017 at 03:35:33AM -0700, Liam Breck wrote:
> >> On Thu, Mar 23, 2017 at 3:28 AM, Sebastian Reichel <sre@kernel.org> wrote:
> >> > Hi,
> >> >
> >> > On Mon, Mar 20, 2017 at 02:43:32AM -0700, Liam Breck wrote:
> >> >> From: Liam Breck <kernel@networkimprov.net>
> >> >>
> >> >> Pass actual chip ID into _setup(), which translates it to a group ID,
> >> >> to allow support for all chips by the power_supply_battery_info code.
> >> >> There are no functional changes to the driver.
> >> >>
> >> >> Signed-off-by: Liam Breck <kernel@networkimprov.net>
> >> >
> >> > This is really ugly. If we need chip ID and group ID let's store
> >> > them in different variables. For example put the detailed chipid
> >> > into di->realchip and then do
> >>
> >> I tried this in a previous version, and Andrew rejected it as
> >> confusing to have two IDs. That was while you were away on business
> >> :-)
> >
> > now we have two IDs in the same variable.
> > That definitely more confusing. We can name the second variable
> > group to reduce confusion.
> 
> I was trying not to rename every instance of di->chip; there's a lot.
> 
> But we can pass real_chip as an argument to this function, so that
> gets you what you wanted.

That's fine with me.

-- Sebastian

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

Re: [PATCH v11 07/10] power: bq27xxx_battery: Keep track of specific chip id

[Andrew F. Davis]

On 03/23/2017 05:35 AM, Liam Breck wrote:
> On Thu, Mar 23, 2017 at 3:28 AM, Sebastian Reichel <sre@kernel.org> wrote:
>> Hi,
>>
>> On Mon, Mar 20, 2017 at 02:43:32AM -0700, Liam Breck wrote:
>>> From: Liam Breck <kernel@networkimprov.net>
>>>
>>> Pass actual chip ID into _setup(), which translates it to a group ID,
>>> to allow support for all chips by the power_supply_battery_info code.
>>> There are no functional changes to the driver.
>>>
>>> Signed-off-by: Liam Breck <kernel@networkimprov.net>
>>
>> This is really ugly. If we need chip ID and group ID let's store
>> them in different variables. For example put the detailed chipid
>> into di->realchip and then do
> 
> I tried this in a previous version, and Andrew rejected it as
> confusing to have two IDs. That was while you were away on business
> :-)
> 

I rejected two tables, to indexes is what I've been pushing for a while.

>> switch(di->realchip) {
>> case FOO:
>>     di->chip = GRP_FOO;
>>     break;
>> case BAR:
>>     di->chip = GRP_BAR;
>>     break;
>> default:
>>     di->chip = di->realchip;
>>     break;
>> }
>>
>> -- Sebastian
>>
>>>  drivers/power/supply/bq27xxx_battery.c     | 27 +++++++++++++++++++++++++++
>>>  drivers/power/supply/bq27xxx_battery_i2c.c | 16 ++++++++--------
>>>  include/linux/power/bq27xxx_battery.h      | 11 +++++++++++
>>>  3 files changed, 46 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
>>> index f36a6f1..db1a388 100644
>>> --- a/drivers/power/supply/bq27xxx_battery.c
>>> +++ b/drivers/power/supply/bq27xxx_battery.c
>>> @@ -1391,6 +1391,33 @@ int bq27xxx_battery_setup(struct bq27xxx_device_info *di)
>>>       struct power_supply_desc *psy_desc;
>>>       struct power_supply_config psy_cfg = { .drv_data = di, };
>>>
>>> +     switch(di->chip) {
>>> +     case BQ27000:
>>> +     case BQ27010:
>>> +     case BQ2750X:
>>> +     case BQ2751X:
>>> +     case BQ27500:
>>> +     case BQ27510G1:
>>> +     case BQ27510G2:
>>> +     case BQ27510G3:
>>> +     case BQ27520G1:
>>> +     case BQ27520G2:
>>> +     case BQ27520G3:
>>> +     case BQ27520G4:
>>> +     case BQ27530:
>>> +     case BQ27541:
>>> +     case BQ27545:
>>> +     case BQ27421: break;
>>> +     case BQ2752X: di->chip = BQ2751X; break;
>>> +     case BQ27531: di->chip = BQ27530; break;
>>> +     case BQ27542: di->chip = BQ27541; break;
>>> +     case BQ27546: di->chip = BQ27541; break;
>>> +     case BQ27742: di->chip = BQ27541; break;
>>> +     case BQ27425: di->chip = BQ27421; break;
>>> +     case BQ27441: di->chip = BQ27421; break;
>>> +     case BQ27621: di->chip = BQ27421; break;
>>> +     }
>>> +
>>>       INIT_DELAYED_WORK(&di->work, bq27xxx_battery_poll);
>>>       mutex_init(&di->lock);
>>>       di->regs = bq27xxx_regs[di->chip];
>>> diff --git a/drivers/power/supply/bq27xxx_battery_i2c.c b/drivers/power/supply/bq27xxx_battery_i2c.c
>>> index c68fbc3..b3f2494 100644
>>> --- a/drivers/power/supply/bq27xxx_battery_i2c.c
>>> +++ b/drivers/power/supply/bq27xxx_battery_i2c.c
>>> @@ -150,7 +150,7 @@ static const struct i2c_device_id bq27xxx_i2c_id_table[] = {
>>>       { "bq27210", BQ27010 },
>>>       { "bq27500", BQ2750X },
>>>       { "bq27510", BQ2751X },
>>> -     { "bq27520", BQ2751X },
>>> +     { "bq27520", BQ2752X },
>>>       { "bq27500-1", BQ27500 },
>>>       { "bq27510g1", BQ27510G1 },
>>>       { "bq27510g2", BQ27510G2 },
>>> @@ -160,16 +160,16 @@ static const struct i2c_device_id bq27xxx_i2c_id_table[] = {
>>>       { "bq27520g3", BQ27520G3 },
>>>       { "bq27520g4", BQ27520G4 },
>>>       { "bq27530", BQ27530 },
>>> -     { "bq27531", BQ27530 },
>>> +     { "bq27531", BQ27531 },
>>>       { "bq27541", BQ27541 },
>>> -     { "bq27542", BQ27541 },
>>> -     { "bq27546", BQ27541 },
>>> -     { "bq27742", BQ27541 },
>>> +     { "bq27542", BQ27542 },
>>> +     { "bq27546", BQ27546 },
>>> +     { "bq27742", BQ27742 },
>>>       { "bq27545", BQ27545 },
>>>       { "bq27421", BQ27421 },
>>> -     { "bq27425", BQ27421 },
>>> -     { "bq27441", BQ27421 },
>>> -     { "bq27621", BQ27421 },
>>> +     { "bq27425", BQ27425 },
>>> +     { "bq27441", BQ27441 },
>>> +     { "bq27621", BQ27621 },
>>>       {},
>>>  };
>>>  MODULE_DEVICE_TABLE(i2c, bq27xxx_i2c_id_table);
>>> diff --git a/include/linux/power/bq27xxx_battery.h b/include/linux/power/bq27xxx_battery.h
>>> index c3369fa..96cec17 100644
>>> --- a/include/linux/power/bq27xxx_battery.h
>>> +++ b/include/linux/power/bq27xxx_battery.h
>>> @@ -2,6 +2,7 @@
>>>  #define __LINUX_BQ27X00_BATTERY_H__
>>>
>>>  enum bq27xxx_chip {
>>> +     /* categories; index for bq27xxx_regs[] */
>>>       BQ27000 = 1, /* bq27000, bq27200 */
>>>       BQ27010, /* bq27010, bq27210 */
>>>       BQ2750X, /* bq27500 deprecated alias */
>>> @@ -18,6 +19,16 @@ enum bq27xxx_chip {
>>>       BQ27541, /* bq27541, bq27542, bq27546, bq27742 */
>>>       BQ27545, /* bq27545 */
>>>       BQ27421, /* bq27421, bq27425, bq27441, bq27621 */
>>> +
>>> +     /* members of categories; translate these to category in _setup() */
>>> +     BQ2752X, /* deprecated alias */
>>> +     BQ27531,
>>> +     BQ27542,
>>> +     BQ27546,
>>> +     BQ27742,
>>> +     BQ27425,
>>> +     BQ27441,
>>> +     BQ27621,
>>>  };
>>>
>>>  /**
>>> --
>>> 2.9.3
>>>

Re: [PATCH v11 07/10] power: bq27xxx_battery: Keep track of specific chip id

[Liam Breck]

On Thu, Mar 23, 2017 at 7:49 AM, Andrew F. Davis <afd@ti.com> wrote:
> On 03/23/2017 05:35 AM, Liam Breck wrote:
>> On Thu, Mar 23, 2017 at 3:28 AM, Sebastian Reichel <sre@kernel.org> wrote:
>>> Hi,
>>>
>>> On Mon, Mar 20, 2017 at 02:43:32AM -0700, Liam Breck wrote:
>>>> From: Liam Breck <kernel@networkimprov.net>
>>>>
>>>> Pass actual chip ID into _setup(), which translates it to a group ID,
>>>> to allow support for all chips by the power_supply_battery_info code.
>>>> There are no functional changes to the driver.
>>>>
>>>> Signed-off-by: Liam Breck <kernel@networkimprov.net>
>>>
>>> This is really ugly. If we need chip ID and group ID let's store
>>> them in different variables. For example put the detailed chipid
>>> into di->realchip and then do
>>
>> I tried this in a previous version, and Andrew rejected it as
>> confusing to have two IDs. That was while you were away on business
>> :-)
>>
>
> I rejected two tables, to indexes is what I've been pushing for a while.

On March 9, you wrote, "each chip should have one unique ID, I think
what ever way you prefer would work for the table structure, lets just
keep the chip ID enums clean"

Anyway, we have a solution - realid via argument.

>>> switch(di->realchip) {
>>> case FOO:
>>>     di->chip = GRP_FOO;
>>>     break;
>>> case BAR:
>>>     di->chip = GRP_BAR;
>>>     break;
>>> default:
>>>     di->chip = di->realchip;
>>>     break;
>>> }
>>>
>>> -- Sebastian
>>>
>>>>  drivers/power/supply/bq27xxx_battery.c     | 27 +++++++++++++++++++++++++++
>>>>  drivers/power/supply/bq27xxx_battery_i2c.c | 16 ++++++++--------
>>>>  include/linux/power/bq27xxx_battery.h      | 11 +++++++++++
>>>>  3 files changed, 46 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
>>>> index f36a6f1..db1a388 100644
>>>> --- a/drivers/power/supply/bq27xxx_battery.c
>>>> +++ b/drivers/power/supply/bq27xxx_battery.c
>>>> @@ -1391,6 +1391,33 @@ int bq27xxx_battery_setup(struct bq27xxx_device_info *di)
>>>>       struct power_supply_desc *psy_desc;
>>>>       struct power_supply_config psy_cfg = { .drv_data = di, };
>>>>
>>>> +     switch(di->chip) {
>>>> +     case BQ27000:
>>>> +     case BQ27010:
>>>> +     case BQ2750X:
>>>> +     case BQ2751X:
>>>> +     case BQ27500:
>>>> +     case BQ27510G1:
>>>> +     case BQ27510G2:
>>>> +     case BQ27510G3:
>>>> +     case BQ27520G1:
>>>> +     case BQ27520G2:
>>>> +     case BQ27520G3:
>>>> +     case BQ27520G4:
>>>> +     case BQ27530:
>>>> +     case BQ27541:
>>>> +     case BQ27545:
>>>> +     case BQ27421: break;
>>>> +     case BQ2752X: di->chip = BQ2751X; break;
>>>> +     case BQ27531: di->chip = BQ27530; break;
>>>> +     case BQ27542: di->chip = BQ27541; break;
>>>> +     case BQ27546: di->chip = BQ27541; break;
>>>> +     case BQ27742: di->chip = BQ27541; break;
>>>> +     case BQ27425: di->chip = BQ27421; break;
>>>> +     case BQ27441: di->chip = BQ27421; break;
>>>> +     case BQ27621: di->chip = BQ27421; break;
>>>> +     }
>>>> +
>>>>       INIT_DELAYED_WORK(&di->work, bq27xxx_battery_poll);
>>>>       mutex_init(&di->lock);
>>>>       di->regs = bq27xxx_regs[di->chip];
>>>> diff --git a/drivers/power/supply/bq27xxx_battery_i2c.c b/drivers/power/supply/bq27xxx_battery_i2c.c
>>>> index c68fbc3..b3f2494 100644
>>>> --- a/drivers/power/supply/bq27xxx_battery_i2c.c
>>>> +++ b/drivers/power/supply/bq27xxx_battery_i2c.c
>>>> @@ -150,7 +150,7 @@ static const struct i2c_device_id bq27xxx_i2c_id_table[] = {
>>>>       { "bq27210", BQ27010 },
>>>>       { "bq27500", BQ2750X },
>>>>       { "bq27510", BQ2751X },
>>>> -     { "bq27520", BQ2751X },
>>>> +     { "bq27520", BQ2752X },
>>>>       { "bq27500-1", BQ27500 },
>>>>       { "bq27510g1", BQ27510G1 },
>>>>       { "bq27510g2", BQ27510G2 },
>>>> @@ -160,16 +160,16 @@ static const struct i2c_device_id bq27xxx_i2c_id_table[] = {
>>>>       { "bq27520g3", BQ27520G3 },
>>>>       { "bq27520g4", BQ27520G4 },
>>>>       { "bq27530", BQ27530 },
>>>> -     { "bq27531", BQ27530 },
>>>> +     { "bq27531", BQ27531 },
>>>>       { "bq27541", BQ27541 },
>>>> -     { "bq27542", BQ27541 },
>>>> -     { "bq27546", BQ27541 },
>>>> -     { "bq27742", BQ27541 },
>>>> +     { "bq27542", BQ27542 },
>>>> +     { "bq27546", BQ27546 },
>>>> +     { "bq27742", BQ27742 },
>>>>       { "bq27545", BQ27545 },
>>>>       { "bq27421", BQ27421 },
>>>> -     { "bq27425", BQ27421 },
>>>> -     { "bq27441", BQ27421 },
>>>> -     { "bq27621", BQ27421 },
>>>> +     { "bq27425", BQ27425 },
>>>> +     { "bq27441", BQ27441 },
>>>> +     { "bq27621", BQ27621 },
>>>>       {},
>>>>  };
>>>>  MODULE_DEVICE_TABLE(i2c, bq27xxx_i2c_id_table);
>>>> diff --git a/include/linux/power/bq27xxx_battery.h b/include/linux/power/bq27xxx_battery.h
>>>> index c3369fa..96cec17 100644
>>>> --- a/include/linux/power/bq27xxx_battery.h
>>>> +++ b/include/linux/power/bq27xxx_battery.h
>>>> @@ -2,6 +2,7 @@
>>>>  #define __LINUX_BQ27X00_BATTERY_H__
>>>>
>>>>  enum bq27xxx_chip {
>>>> +     /* categories; index for bq27xxx_regs[] */
>>>>       BQ27000 = 1, /* bq27000, bq27200 */
>>>>       BQ27010, /* bq27010, bq27210 */
>>>>       BQ2750X, /* bq27500 deprecated alias */
>>>> @@ -18,6 +19,16 @@ enum bq27xxx_chip {
>>>>       BQ27541, /* bq27541, bq27542, bq27546, bq27742 */
>>>>       BQ27545, /* bq27545 */
>>>>       BQ27421, /* bq27421, bq27425, bq27441, bq27621 */
>>>> +
>>>> +     /* members of categories; translate these to category in _setup() */
>>>> +     BQ2752X, /* deprecated alias */
>>>> +     BQ27531,
>>>> +     BQ27542,
>>>> +     BQ27546,
>>>> +     BQ27742,
>>>> +     BQ27425,
>>>> +     BQ27441,
>>>> +     BQ27621,
>>>>  };
>>>>
>>>>  /**
>>>> --
>>>> 2.9.3
>>>>

Re: [PATCH v11 07/10] power: bq27xxx_battery: Keep track of specific chip id

[Andrew F. Davis]

On 03/23/2017 04:44 PM, Liam Breck wrote:
> On Thu, Mar 23, 2017 at 7:49 AM, Andrew F. Davis <afd@ti.com> wrote:
>> On 03/23/2017 05:35 AM, Liam Breck wrote:
>>> On Thu, Mar 23, 2017 at 3:28 AM, Sebastian Reichel <sre@kernel.org> wrote:
>>>> Hi,
>>>>
>>>> On Mon, Mar 20, 2017 at 02:43:32AM -0700, Liam Breck wrote:
>>>>> From: Liam Breck <kernel@networkimprov.net>
>>>>>
>>>>> Pass actual chip ID into _setup(), which translates it to a group ID,
>>>>> to allow support for all chips by the power_supply_battery_info code.
>>>>> There are no functional changes to the driver.
>>>>>
>>>>> Signed-off-by: Liam Breck <kernel@networkimprov.net>
>>>>
>>>> This is really ugly. If we need chip ID and group ID let's store
>>>> them in different variables. For example put the detailed chipid
>>>> into di->realchip and then do
>>>
>>> I tried this in a previous version, and Andrew rejected it as
>>> confusing to have two IDs. That was while you were away on business
>>> :-)
>>>
>>
>> I rejected two tables, to indexes is what I've been pushing for a while.
> 
> On March 9, you wrote, "each chip should have one unique ID, I think
> what ever way you prefer would work for the table structure, lets just
> keep the chip ID enums clean"
> 

And I still want this, the chip id enum is *not* clean right now, a
BQ27531 for instance could be BQ27530 or BQ27531. enum bq27xxx_chip
should not have "categories" and "members of categories" in one enum,
it's a hack to reduce the number of table indexes, just add di->realchip
or similar as Sebastian suggested and problem solved.

> Anyway, we have a solution - realid via argument.
> 
>>>> switch(di->realchip) {
>>>> case FOO:
>>>>     di->chip = GRP_FOO;
>>>>     break;
>>>> case BAR:
>>>>     di->chip = GRP_BAR;
>>>>     break;
>>>> default:
>>>>     di->chip = di->realchip;
>>>>     break;
>>>> }
>>>>
>>>> -- Sebastian
>>>>
>>>>>  drivers/power/supply/bq27xxx_battery.c     | 27 +++++++++++++++++++++++++++
>>>>>  drivers/power/supply/bq27xxx_battery_i2c.c | 16 ++++++++--------
>>>>>  include/linux/power/bq27xxx_battery.h      | 11 +++++++++++
>>>>>  3 files changed, 46 insertions(+), 8 deletions(-)
>>>>>
>>>>> diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
>>>>> index f36a6f1..db1a388 100644
>>>>> --- a/drivers/power/supply/bq27xxx_battery.c
>>>>> +++ b/drivers/power/supply/bq27xxx_battery.c
>>>>> @@ -1391,6 +1391,33 @@ int bq27xxx_battery_setup(struct bq27xxx_device_info *di)
>>>>>       struct power_supply_desc *psy_desc;
>>>>>       struct power_supply_config psy_cfg = { .drv_data = di, };
>>>>>
>>>>> +     switch(di->chip) {
>>>>> +     case BQ27000:
>>>>> +     case BQ27010:
>>>>> +     case BQ2750X:
>>>>> +     case BQ2751X:
>>>>> +     case BQ27500:
>>>>> +     case BQ27510G1:
>>>>> +     case BQ27510G2:
>>>>> +     case BQ27510G3:
>>>>> +     case BQ27520G1:
>>>>> +     case BQ27520G2:
>>>>> +     case BQ27520G3:
>>>>> +     case BQ27520G4:
>>>>> +     case BQ27530:
>>>>> +     case BQ27541:
>>>>> +     case BQ27545:
>>>>> +     case BQ27421: break;
>>>>> +     case BQ2752X: di->chip = BQ2751X; break;
>>>>> +     case BQ27531: di->chip = BQ27530; break;
>>>>> +     case BQ27542: di->chip = BQ27541; break;
>>>>> +     case BQ27546: di->chip = BQ27541; break;
>>>>> +     case BQ27742: di->chip = BQ27541; break;
>>>>> +     case BQ27425: di->chip = BQ27421; break;
>>>>> +     case BQ27441: di->chip = BQ27421; break;
>>>>> +     case BQ27621: di->chip = BQ27421; break;
>>>>> +     }
>>>>> +
>>>>>       INIT_DELAYED_WORK(&di->work, bq27xxx_battery_poll);
>>>>>       mutex_init(&di->lock);
>>>>>       di->regs = bq27xxx_regs[di->chip];
>>>>> diff --git a/drivers/power/supply/bq27xxx_battery_i2c.c b/drivers/power/supply/bq27xxx_battery_i2c.c
>>>>> index c68fbc3..b3f2494 100644
>>>>> --- a/drivers/power/supply/bq27xxx_battery_i2c.c
>>>>> +++ b/drivers/power/supply/bq27xxx_battery_i2c.c
>>>>> @@ -150,7 +150,7 @@ static const struct i2c_device_id bq27xxx_i2c_id_table[] = {
>>>>>       { "bq27210", BQ27010 },
>>>>>       { "bq27500", BQ2750X },
>>>>>       { "bq27510", BQ2751X },
>>>>> -     { "bq27520", BQ2751X },
>>>>> +     { "bq27520", BQ2752X },
>>>>>       { "bq27500-1", BQ27500 },
>>>>>       { "bq27510g1", BQ27510G1 },
>>>>>       { "bq27510g2", BQ27510G2 },
>>>>> @@ -160,16 +160,16 @@ static const struct i2c_device_id bq27xxx_i2c_id_table[] = {
>>>>>       { "bq27520g3", BQ27520G3 },
>>>>>       { "bq27520g4", BQ27520G4 },
>>>>>       { "bq27530", BQ27530 },
>>>>> -     { "bq27531", BQ27530 },
>>>>> +     { "bq27531", BQ27531 },
>>>>>       { "bq27541", BQ27541 },
>>>>> -     { "bq27542", BQ27541 },
>>>>> -     { "bq27546", BQ27541 },
>>>>> -     { "bq27742", BQ27541 },
>>>>> +     { "bq27542", BQ27542 },
>>>>> +     { "bq27546", BQ27546 },
>>>>> +     { "bq27742", BQ27742 },
>>>>>       { "bq27545", BQ27545 },
>>>>>       { "bq27421", BQ27421 },
>>>>> -     { "bq27425", BQ27421 },
>>>>> -     { "bq27441", BQ27421 },
>>>>> -     { "bq27621", BQ27421 },
>>>>> +     { "bq27425", BQ27425 },
>>>>> +     { "bq27441", BQ27441 },
>>>>> +     { "bq27621", BQ27621 },
>>>>>       {},
>>>>>  };
>>>>>  MODULE_DEVICE_TABLE(i2c, bq27xxx_i2c_id_table);
>>>>> diff --git a/include/linux/power/bq27xxx_battery.h b/include/linux/power/bq27xxx_battery.h
>>>>> index c3369fa..96cec17 100644
>>>>> --- a/include/linux/power/bq27xxx_battery.h
>>>>> +++ b/include/linux/power/bq27xxx_battery.h
>>>>> @@ -2,6 +2,7 @@
>>>>>  #define __LINUX_BQ27X00_BATTERY_H__
>>>>>
>>>>>  enum bq27xxx_chip {
>>>>> +     /* categories; index for bq27xxx_regs[] */
>>>>>       BQ27000 = 1, /* bq27000, bq27200 */
>>>>>       BQ27010, /* bq27010, bq27210 */
>>>>>       BQ2750X, /* bq27500 deprecated alias */
>>>>> @@ -18,6 +19,16 @@ enum bq27xxx_chip {
>>>>>       BQ27541, /* bq27541, bq27542, bq27546, bq27742 */
>>>>>       BQ27545, /* bq27545 */
>>>>>       BQ27421, /* bq27421, bq27425, bq27441, bq27621 */
>>>>> +
>>>>> +     /* members of categories; translate these to category in _setup() */
>>>>> +     BQ2752X, /* deprecated alias */
>>>>> +     BQ27531,
>>>>> +     BQ27542,
>>>>> +     BQ27546,
>>>>> +     BQ27742,
>>>>> +     BQ27425,
>>>>> +     BQ27441,
>>>>> +     BQ27621,
>>>>>  };
>>>>>
>>>>>  /**
>>>>> --
>>>>> 2.9.3
>>>>>

Re: [PATCH v11 07/10] power: bq27xxx_battery: Keep track of specific chip id

[Liam Breck]

On Fri, Mar 24, 2017 at 4:48 AM, Andrew F. Davis <afd@ti.com> wrote:
> On 03/23/2017 04:44 PM, Liam Breck wrote:
>> On Thu, Mar 23, 2017 at 7:49 AM, Andrew F. Davis <afd@ti.com> wrote:
>>> On 03/23/2017 05:35 AM, Liam Breck wrote:
>>>> On Thu, Mar 23, 2017 at 3:28 AM, Sebastian Reichel <sre@kernel.org> wrote:
>>>>> Hi,
>>>>>
>>>>> On Mon, Mar 20, 2017 at 02:43:32AM -0700, Liam Breck wrote:
>>>>>> From: Liam Breck <kernel@networkimprov.net>
>>>>>>
>>>>>> Pass actual chip ID into _setup(), which translates it to a group ID,
>>>>>> to allow support for all chips by the power_supply_battery_info code.
>>>>>> There are no functional changes to the driver.
>>>>>>
>>>>>> Signed-off-by: Liam Breck <kernel@networkimprov.net>
>>>>>
>>>>> This is really ugly. If we need chip ID and group ID let's store
>>>>> them in different variables. For example put the detailed chipid
>>>>> into di->realchip and then do
>>>>
>>>> I tried this in a previous version, and Andrew rejected it as
>>>> confusing to have two IDs. That was while you were away on business
>>>> :-)
>>>>
>>>
>>> I rejected two tables, to indexes is what I've been pushing for a while.
>>
>> On March 9, you wrote, "each chip should have one unique ID, I think
>> what ever way you prefer would work for the table structure, lets just
>> keep the chip ID enums clean"
>>
>
> And I still want this, the chip id enum is *not* clean right now, a
> BQ27531 for instance could be BQ27530 or BQ27531. enum bq27xxx_chip
> should not have "categories" and "members of categories" in one enum,
> it's a hack to reduce the number of table indexes, just add di->realchip
> or similar as Sebastian suggested and problem solved.

Alternative to the "members" subset of the enum is a second enum which
replicates the "categories" subset. The names in the first enum will
be different (BQC27nnn) , so we have originals in the second. When
adding a new chip, you'd usually have to add to both enums. Is that
better?

We don't need di->realchip in any case; we only need the real ID in
setup() to set unseal_key and dm_regs fields.

>> Anyway, we have a solution - realid via argument.
>>
>>>>> switch(di->realchip) {
>>>>> case FOO:
>>>>>     di->chip = GRP_FOO;
>>>>>     break;
>>>>> case BAR:
>>>>>     di->chip = GRP_BAR;
>>>>>     break;
>>>>> default:
>>>>>     di->chip = di->realchip;
>>>>>     break;
>>>>> }
>>>>>
>>>>> -- Sebastian
>>>>>
>>>>>>  drivers/power/supply/bq27xxx_battery.c     | 27 +++++++++++++++++++++++++++
>>>>>>  drivers/power/supply/bq27xxx_battery_i2c.c | 16 ++++++++--------
>>>>>>  include/linux/power/bq27xxx_battery.h      | 11 +++++++++++
>>>>>>  3 files changed, 46 insertions(+), 8 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
>>>>>> index f36a6f1..db1a388 100644
>>>>>> --- a/drivers/power/supply/bq27xxx_battery.c
>>>>>> +++ b/drivers/power/supply/bq27xxx_battery.c
>>>>>> @@ -1391,6 +1391,33 @@ int bq27xxx_battery_setup(struct bq27xxx_device_info *di)
>>>>>>       struct power_supply_desc *psy_desc;
>>>>>>       struct power_supply_config psy_cfg = { .drv_data = di, };
>>>>>>
>>>>>> +     switch(di->chip) {
>>>>>> +     case BQ27000:
>>>>>> +     case BQ27010:
>>>>>> +     case BQ2750X:
>>>>>> +     case BQ2751X:
>>>>>> +     case BQ27500:
>>>>>> +     case BQ27510G1:
>>>>>> +     case BQ27510G2:
>>>>>> +     case BQ27510G3:
>>>>>> +     case BQ27520G1:
>>>>>> +     case BQ27520G2:
>>>>>> +     case BQ27520G3:
>>>>>> +     case BQ27520G4:
>>>>>> +     case BQ27530:
>>>>>> +     case BQ27541:
>>>>>> +     case BQ27545:
>>>>>> +     case BQ27421: break;
>>>>>> +     case BQ2752X: di->chip = BQ2751X; break;
>>>>>> +     case BQ27531: di->chip = BQ27530; break;
>>>>>> +     case BQ27542: di->chip = BQ27541; break;
>>>>>> +     case BQ27546: di->chip = BQ27541; break;
>>>>>> +     case BQ27742: di->chip = BQ27541; break;
>>>>>> +     case BQ27425: di->chip = BQ27421; break;
>>>>>> +     case BQ27441: di->chip = BQ27421; break;
>>>>>> +     case BQ27621: di->chip = BQ27421; break;
>>>>>> +     }
>>>>>> +
>>>>>>       INIT_DELAYED_WORK(&di->work, bq27xxx_battery_poll);
>>>>>>       mutex_init(&di->lock);
>>>>>>       di->regs = bq27xxx_regs[di->chip];
>>>>>> diff --git a/drivers/power/supply/bq27xxx_battery_i2c.c b/drivers/power/supply/bq27xxx_battery_i2c.c
>>>>>> index c68fbc3..b3f2494 100644
>>>>>> --- a/drivers/power/supply/bq27xxx_battery_i2c.c
>>>>>> +++ b/drivers/power/supply/bq27xxx_battery_i2c.c
>>>>>> @@ -150,7 +150,7 @@ static const struct i2c_device_id bq27xxx_i2c_id_table[] = {
>>>>>>       { "bq27210", BQ27010 },
>>>>>>       { "bq27500", BQ2750X },
>>>>>>       { "bq27510", BQ2751X },
>>>>>> -     { "bq27520", BQ2751X },
>>>>>> +     { "bq27520", BQ2752X },
>>>>>>       { "bq27500-1", BQ27500 },
>>>>>>       { "bq27510g1", BQ27510G1 },
>>>>>>       { "bq27510g2", BQ27510G2 },
>>>>>> @@ -160,16 +160,16 @@ static const struct i2c_device_id bq27xxx_i2c_id_table[] = {
>>>>>>       { "bq27520g3", BQ27520G3 },
>>>>>>       { "bq27520g4", BQ27520G4 },
>>>>>>       { "bq27530", BQ27530 },
>>>>>> -     { "bq27531", BQ27530 },
>>>>>> +     { "bq27531", BQ27531 },
>>>>>>       { "bq27541", BQ27541 },
>>>>>> -     { "bq27542", BQ27541 },
>>>>>> -     { "bq27546", BQ27541 },
>>>>>> -     { "bq27742", BQ27541 },
>>>>>> +     { "bq27542", BQ27542 },
>>>>>> +     { "bq27546", BQ27546 },
>>>>>> +     { "bq27742", BQ27742 },
>>>>>>       { "bq27545", BQ27545 },
>>>>>>       { "bq27421", BQ27421 },
>>>>>> -     { "bq27425", BQ27421 },
>>>>>> -     { "bq27441", BQ27421 },
>>>>>> -     { "bq27621", BQ27421 },
>>>>>> +     { "bq27425", BQ27425 },
>>>>>> +     { "bq27441", BQ27441 },
>>>>>> +     { "bq27621", BQ27621 },
>>>>>>       {},
>>>>>>  };
>>>>>>  MODULE_DEVICE_TABLE(i2c, bq27xxx_i2c_id_table);
>>>>>> diff --git a/include/linux/power/bq27xxx_battery.h b/include/linux/power/bq27xxx_battery.h
>>>>>> index c3369fa..96cec17 100644
>>>>>> --- a/include/linux/power/bq27xxx_battery.h
>>>>>> +++ b/include/linux/power/bq27xxx_battery.h
>>>>>> @@ -2,6 +2,7 @@
>>>>>>  #define __LINUX_BQ27X00_BATTERY_H__
>>>>>>
>>>>>>  enum bq27xxx_chip {
>>>>>> +     /* categories; index for bq27xxx_regs[] */
>>>>>>       BQ27000 = 1, /* bq27000, bq27200 */
>>>>>>       BQ27010, /* bq27010, bq27210 */
>>>>>>       BQ2750X, /* bq27500 deprecated alias */
>>>>>> @@ -18,6 +19,16 @@ enum bq27xxx_chip {
>>>>>>       BQ27541, /* bq27541, bq27542, bq27546, bq27742 */
>>>>>>       BQ27545, /* bq27545 */
>>>>>>       BQ27421, /* bq27421, bq27425, bq27441, bq27621 */
>>>>>> +
>>>>>> +     /* members of categories; translate these to category in _setup() */
>>>>>> +     BQ2752X, /* deprecated alias */
>>>>>> +     BQ27531,
>>>>>> +     BQ27542,
>>>>>> +     BQ27546,
>>>>>> +     BQ27742,
>>>>>> +     BQ27425,
>>>>>> +     BQ27441,
>>>>>> +     BQ27621,
>>>>>>  };
>>>>>>
>>>>>>  /**
>>>>>> --
>>>>>> 2.9.3
>>>>>>

[PATCH v11 08/10] power: bq27xxx_battery: Add power_supply_battery_info support

[Liam Breck]

From: Liam Breck <kernel@networkimprov.net>

Previously there was no way to configure 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 chip RAM or non-volatile memory.

Supports chips BQ27500, 545, 421, 425, 441, 621. Others may be added.

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

diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
index db1a388..7f08f36 100644
--- a/drivers/power/supply/bq27xxx_battery.c
+++ b/drivers/power/supply/bq27xxx_battery.c
@@ -57,7 +57,7 @@
 
 #include <linux/power/bq27xxx_battery.h>
 
-#define DRIVER_VERSION		"1.2.0"
+#define DRIVER_VERSION		"1.3.0"
 
 #define BQ27XXX_MANUFACTURER	"Texas Instruments"
 
@@ -65,6 +65,7 @@
 #define BQ27XXX_FLAG_DSC	BIT(0)
 #define BQ27XXX_FLAG_SOCF	BIT(1) /* State-of-Charge threshold final */
 #define BQ27XXX_FLAG_SOC1	BIT(2) /* State-of-Charge threshold 1 */
+#define BQ27XXX_FLAG_CFGUP	BIT(4)
 #define BQ27XXX_FLAG_FC		BIT(9)
 #define BQ27XXX_FLAG_OTD	BIT(14)
 #define BQ27XXX_FLAG_OTC	BIT(15)
@@ -78,6 +79,12 @@
 #define BQ27000_FLAG_FC		BIT(5)
 #define BQ27000_FLAG_CHGS	BIT(7) /* Charge state flag */
 
+/* control register params */
+#define BQ27XXX_SEALED			0x20
+#define BQ27XXX_SET_CFGUPDATE		0x13
+#define BQ27XXX_SOFT_RESET		0x42
+#define BQ27XXX_RESET			0x41
+
 #define BQ27XXX_RS			(20) /* Resistor sense mOhm */
 #define BQ27XXX_POWER_CONSTANT		(29200) /* 29.2 µV^2 * 1000 */
 #define BQ27XXX_CURRENT_CONSTANT	(3570) /* 3.57 µV * 1000 */
@@ -108,6 +115,11 @@ enum bq27xxx_reg_index {
 	BQ27XXX_REG_SOC,	/* State-of-Charge */
 	BQ27XXX_REG_DCAP,	/* Design Capacity */
 	BQ27XXX_REG_AP,		/* Average Power */
+	BQ27XXX_DM_CTRL,	/* BlockDataControl() */
+	BQ27XXX_DM_CLASS,	/* DataClass() */
+	BQ27XXX_DM_BLOCK,	/* DataBlock() */
+	BQ27XXX_DM_DATA,	/* BlockData() */
+	BQ27XXX_DM_CKSUM,	/* BlockDataChecksum() */
 	BQ27XXX_REG_MAX,	/* sentinel */
 };
 
@@ -131,6 +143,11 @@ static u8 bq27xxx_regs[][BQ27XXX_REG_MAX] = {
 		[BQ27XXX_REG_SOC] = 0x0b,
 		[BQ27XXX_REG_DCAP] = 0x76,
 		[BQ27XXX_REG_AP] = 0x24,
+		[BQ27XXX_DM_CTRL] = INVALID_REG_ADDR,
+		[BQ27XXX_DM_CLASS] = INVALID_REG_ADDR,
+		[BQ27XXX_DM_BLOCK] = INVALID_REG_ADDR,
+		[BQ27XXX_DM_DATA] = INVALID_REG_ADDR,
+		[BQ27XXX_DM_CKSUM] = INVALID_REG_ADDR,
 	},
 	[BQ27010] = {
 		[BQ27XXX_REG_CTRL] = 0x00,
@@ -150,6 +167,11 @@ static u8 bq27xxx_regs[][BQ27XXX_REG_MAX] = {
 		[BQ27XXX_REG_SOC] = 0x0b,
 		[BQ27XXX_REG_DCAP] = 0x76,
 		[BQ27XXX_REG_AP] = INVALID_REG_ADDR,
+		[BQ27XXX_DM_CTRL] = INVALID_REG_ADDR,
+		[BQ27XXX_DM_CLASS] = INVALID_REG_ADDR,
+		[BQ27XXX_DM_BLOCK] = INVALID_REG_ADDR,
+		[BQ27XXX_DM_DATA] = INVALID_REG_ADDR,
+		[BQ27XXX_DM_CKSUM] = INVALID_REG_ADDR,
 	},
 	[BQ2750X] = {
 		[BQ27XXX_REG_CTRL] = 0x00,
@@ -169,6 +191,11 @@ static u8 bq27xxx_regs[][BQ27XXX_REG_MAX] = {
 		[BQ27XXX_REG_SOC] = 0x2c,
 		[BQ27XXX_REG_DCAP] = 0x3c,
 		[BQ27XXX_REG_AP] = INVALID_REG_ADDR,
+		[BQ27XXX_DM_CTRL] = 0x61,
+		[BQ27XXX_DM_CLASS] = 0x3e,
+		[BQ27XXX_DM_BLOCK] = 0x3f,
+		[BQ27XXX_DM_DATA] = 0x40,
+		[BQ27XXX_DM_CKSUM] = 0x60,
 	},
 	[BQ2751X] = {
 		[BQ27XXX_REG_CTRL] = 0x00,
@@ -188,6 +215,11 @@ static u8 bq27xxx_regs[][BQ27XXX_REG_MAX] = {
 		[BQ27XXX_REG_SOC] = 0x20,
 		[BQ27XXX_REG_DCAP] = 0x2e,
 		[BQ27XXX_REG_AP] = INVALID_REG_ADDR,
+		[BQ27XXX_DM_CTRL] = 0x61,
+		[BQ27XXX_DM_CLASS] = 0x3e,
+		[BQ27XXX_DM_BLOCK] = 0x3f,
+		[BQ27XXX_DM_DATA] = 0x40,
+		[BQ27XXX_DM_CKSUM] = 0x60,
 	},
 	[BQ27500] = {
 		[BQ27XXX_REG_CTRL] = 0x00,
@@ -207,6 +239,11 @@ static u8 bq27xxx_regs[][BQ27XXX_REG_MAX] = {
 		[BQ27XXX_REG_SOC] = 0x2c,
 		[BQ27XXX_REG_DCAP] = 0x3c,
 		[BQ27XXX_REG_AP] = 0x24,
+		[BQ27XXX_DM_CTRL] = 0x61,
+		[BQ27XXX_DM_CLASS] = 0x3e,
+		[BQ27XXX_DM_BLOCK] = 0x3f,
+		[BQ27XXX_DM_DATA] = 0x40,
+		[BQ27XXX_DM_CKSUM] = 0x60,
 	},
 	[BQ27510G1] = {
 		[BQ27XXX_REG_CTRL] = 0x00,
@@ -226,6 +263,11 @@ static u8 bq27xxx_regs[][BQ27XXX_REG_MAX] = {
 		[BQ27XXX_REG_SOC] = 0x2c,
 		[BQ27XXX_REG_DCAP] = 0x3c,
 		[BQ27XXX_REG_AP] = 0x24,
+		[BQ27XXX_DM_CTRL] = 0x61,
+		[BQ27XXX_DM_CLASS] = 0x3e,
+		[BQ27XXX_DM_BLOCK] = 0x3f,
+		[BQ27XXX_DM_DATA] = 0x40,
+		[BQ27XXX_DM_CKSUM] = 0x60,
 	},
 	[BQ27510G2] = {
 		[BQ27XXX_REG_CTRL] = 0x00,
@@ -245,6 +287,11 @@ static u8 bq27xxx_regs[][BQ27XXX_REG_MAX] = {
 		[BQ27XXX_REG_SOC] = 0x2c,
 		[BQ27XXX_REG_DCAP] = 0x3c,
 		[BQ27XXX_REG_AP] = 0x24,
+		[BQ27XXX_DM_CTRL] = 0x61,
+		[BQ27XXX_DM_CLASS] = 0x3e,
+		[BQ27XXX_DM_BLOCK] = 0x3f,
+		[BQ27XXX_DM_DATA] = 0x40,
+		[BQ27XXX_DM_CKSUM] = 0x60,
 	},
 	[BQ27510G3] = {
 		[BQ27XXX_REG_CTRL] = 0x00,
@@ -264,6 +311,11 @@ static u8 bq27xxx_regs[][BQ27XXX_REG_MAX] = {
 		[BQ27XXX_REG_SOC] = 0x20,
 		[BQ27XXX_REG_DCAP] = 0x2e,
 		[BQ27XXX_REG_AP] = INVALID_REG_ADDR,
+		[BQ27XXX_DM_CTRL] = 0x61,
+		[BQ27XXX_DM_CLASS] = 0x3e,
+		[BQ27XXX_DM_BLOCK] = 0x3f,
+		[BQ27XXX_DM_DATA] = 0x40,
+		[BQ27XXX_DM_CKSUM] = 0x60,
 	},
 	[BQ27520G1] = {
 		[BQ27XXX_REG_CTRL] = 0x00,
@@ -283,6 +335,11 @@ static u8 bq27xxx_regs[][BQ27XXX_REG_MAX] = {
 		[BQ27XXX_REG_SOC] = 0x2c,
 		[BQ27XXX_REG_DCAP] = 0x3c,
 		[BQ27XXX_REG_AP] = 0x24,
+		[BQ27XXX_DM_CTRL] = 0x61,
+		[BQ27XXX_DM_CLASS] = 0x3e,
+		[BQ27XXX_DM_BLOCK] = 0x3f,
+		[BQ27XXX_DM_DATA] = 0x40,
+		[BQ27XXX_DM_CKSUM] = 0x60,
 	},
 	[BQ27520G2] = {
 		[BQ27XXX_REG_CTRL] = 0x00,
@@ -302,6 +359,11 @@ static u8 bq27xxx_regs[][BQ27XXX_REG_MAX] = {
 		[BQ27XXX_REG_SOC] = 0x2c,
 		[BQ27XXX_REG_DCAP] = 0x3c,
 		[BQ27XXX_REG_AP] = 0x24,
+		[BQ27XXX_DM_CTRL] = 0x61,
+		[BQ27XXX_DM_CLASS] = 0x3e,
+		[BQ27XXX_DM_BLOCK] = 0x3f,
+		[BQ27XXX_DM_DATA] = 0x40,
+		[BQ27XXX_DM_CKSUM] = 0x60,
 	},
 	[BQ27520G3] = {
 		[BQ27XXX_REG_CTRL] = 0x00,
@@ -321,6 +383,11 @@ static u8 bq27xxx_regs[][BQ27XXX_REG_MAX] = {
 		[BQ27XXX_REG_SOC] = 0x2c,
 		[BQ27XXX_REG_DCAP] = 0x3c,
 		[BQ27XXX_REG_AP] = 0x24,
+		[BQ27XXX_DM_CTRL] = 0x61,
+		[BQ27XXX_DM_CLASS] = 0x3e,
+		[BQ27XXX_DM_BLOCK] = 0x3f,
+		[BQ27XXX_DM_DATA] = 0x40,
+		[BQ27XXX_DM_CKSUM] = 0x60,
 	},
 	[BQ27520G4] = {
 		[BQ27XXX_REG_CTRL] = 0x00,
@@ -340,6 +407,11 @@ static u8 bq27xxx_regs[][BQ27XXX_REG_MAX] = {
 		[BQ27XXX_REG_SOC] = 0x20,
 		[BQ27XXX_REG_DCAP] = INVALID_REG_ADDR,
 		[BQ27XXX_REG_AP] = INVALID_REG_ADDR,
+		[BQ27XXX_DM_CTRL] = 0x61,
+		[BQ27XXX_DM_CLASS] = 0x3e,
+		[BQ27XXX_DM_BLOCK] = 0x3f,
+		[BQ27XXX_DM_DATA] = 0x40,
+		[BQ27XXX_DM_CKSUM] = 0x60,
 	},
 	[BQ27530] = {
 		[BQ27XXX_REG_CTRL] = 0x00,
@@ -359,6 +431,11 @@ static u8 bq27xxx_regs[][BQ27XXX_REG_MAX] = {
 		[BQ27XXX_REG_SOC] = 0x2c,
 		[BQ27XXX_REG_DCAP] = INVALID_REG_ADDR,
 		[BQ27XXX_REG_AP] = 0x24,
+		[BQ27XXX_DM_CTRL] = 0x61,
+		[BQ27XXX_DM_CLASS] = 0x3e,
+		[BQ27XXX_DM_BLOCK] = 0x3f,
+		[BQ27XXX_DM_DATA] = 0x40,
+		[BQ27XXX_DM_CKSUM] = 0x60,
 	},
 	[BQ27541] = {
 		[BQ27XXX_REG_CTRL] = 0x00,
@@ -378,6 +455,11 @@ static u8 bq27xxx_regs[][BQ27XXX_REG_MAX] = {
 		[BQ27XXX_REG_SOC] = 0x2c,
 		[BQ27XXX_REG_DCAP] = 0x3c,
 		[BQ27XXX_REG_AP] = 0x24,
+		[BQ27XXX_DM_CTRL] = 0x61,
+		[BQ27XXX_DM_CLASS] = 0x3e,
+		[BQ27XXX_DM_BLOCK] = 0x3f,
+		[BQ27XXX_DM_DATA] = 0x40,
+		[BQ27XXX_DM_CKSUM] = 0x60,
 	},
 	[BQ27545] = {
 		[BQ27XXX_REG_CTRL] = 0x00,
@@ -397,6 +479,11 @@ static u8 bq27xxx_regs[][BQ27XXX_REG_MAX] = {
 		[BQ27XXX_REG_SOC] = 0x2c,
 		[BQ27XXX_REG_DCAP] = INVALID_REG_ADDR,
 		[BQ27XXX_REG_AP] = 0x24,
+		[BQ27XXX_DM_CTRL] = 0x61,
+		[BQ27XXX_DM_CLASS] = 0x3e,
+		[BQ27XXX_DM_BLOCK] = 0x3f,
+		[BQ27XXX_DM_DATA] = 0x40,
+		[BQ27XXX_DM_CKSUM] = 0x60,
 	},
 	[BQ27421] = {
 		[BQ27XXX_REG_CTRL] = 0x00,
@@ -416,6 +503,11 @@ static u8 bq27xxx_regs[][BQ27XXX_REG_MAX] = {
 		[BQ27XXX_REG_SOC] = 0x1c,
 		[BQ27XXX_REG_DCAP] = 0x3c,
 		[BQ27XXX_REG_AP] = 0x18,
+		[BQ27XXX_DM_CTRL] = 0x61,
+		[BQ27XXX_DM_CLASS] = 0x3e,
+		[BQ27XXX_DM_BLOCK] = 0x3f,
+		[BQ27XXX_DM_DATA] = 0x40,
+		[BQ27XXX_DM_CKSUM] = 0x60,
 	},
 };
 
@@ -757,6 +849,82 @@ static struct {
 static DEFINE_MUTEX(bq27xxx_list_lock);
 static LIST_HEAD(bq27xxx_battery_devices);
 
+#define BQ27XXX_DM_SZ	32
+
+#define BQ27XXX_MSLEEP(i) usleep_range((i)*1000, (i)*1000+500)
+
+struct bq27xxx_dm_reg {
+	u8 subclass_id;
+	u8 offset;
+	u8 bytes;
+	u16 min, max;
+};
+
+struct bq27xxx_dm_buf {
+	u8 class;
+	u8 block;
+	u8 a[BQ27XXX_DM_SZ];
+	bool full, updt;
+};
+
+#define BQ27XXX_DM_BUF(di, i) { \
+	.class = (di)->dm_regs[i].subclass_id, \
+	.block = (di)->dm_regs[i].offset / BQ27XXX_DM_SZ, \
+}
+
+static inline u16* bq27xxx_dm_reg_ptr(struct bq27xxx_dm_buf *buf,
+				      struct bq27xxx_dm_reg *reg)
+{
+	if (buf->class == reg->subclass_id
+	&&  buf->block == reg->offset / BQ27XXX_DM_SZ)
+		return (u16*) (buf->a + reg->offset % BQ27XXX_DM_SZ);
+
+	return NULL;
+}
+
+enum bq27xxx_dm_reg_id {
+	BQ27XXX_DM_DESIGN_CAPACITY = 0,
+	BQ27XXX_DM_DESIGN_ENERGY,
+	BQ27XXX_DM_TERMINATE_VOLTAGE,
+};
+
+static const char* bq27xxx_dm_reg_name[] = {
+	[BQ27XXX_DM_DESIGN_CAPACITY] = "design-capacity",
+	[BQ27XXX_DM_DESIGN_ENERGY] = "design-energy",
+	[BQ27XXX_DM_TERMINATE_VOLTAGE] = "terminate-voltage",
+};
+
+static struct bq27xxx_dm_reg bq27500_dm_regs[] = {
+	[BQ27XXX_DM_DESIGN_CAPACITY]   = { 48, 10, 2,    0, 65535 },
+	[BQ27XXX_DM_DESIGN_ENERGY]     = { }, /* missing on chip */
+	[BQ27XXX_DM_TERMINATE_VOLTAGE] = { 80, 48, 2, 1000, 32767 },
+};
+
+static struct bq27xxx_dm_reg bq27545_dm_regs[] = {
+	[BQ27XXX_DM_DESIGN_CAPACITY]   = { 48, 23, 2,    0, 32767 },
+	[BQ27XXX_DM_DESIGN_ENERGY]     = { 48, 25, 2,    0, 32767 },
+	[BQ27XXX_DM_TERMINATE_VOLTAGE] = { 80, 67, 2, 2800,  3700 },
+};
+
+static struct bq27xxx_dm_reg bq27421_dm_regs[] = {
+	[BQ27XXX_DM_DESIGN_CAPACITY]   = { 82, 10, 2,    0,  8000 },
+	[BQ27XXX_DM_DESIGN_ENERGY]     = { 82, 12, 2,    0, 32767 },
+	[BQ27XXX_DM_TERMINATE_VOLTAGE] = { 82, 16, 2, 2500,  3700 },
+};
+
+static struct bq27xxx_dm_reg bq27425_dm_regs[] = {
+	[BQ27XXX_DM_DESIGN_CAPACITY]   = { 82, 12, 2,    0, 32767 },
+	[BQ27XXX_DM_DESIGN_ENERGY]     = { 82, 14, 2,    0, 32767 },
+	[BQ27XXX_DM_TERMINATE_VOLTAGE] = { 82, 18, 2, 2800,  3700 },
+};
+
+static struct bq27xxx_dm_reg bq27621_dm_regs[] = {
+	[BQ27XXX_DM_DESIGN_CAPACITY]   = { 82, 3, 2,    0,  8000 },
+	[BQ27XXX_DM_DESIGN_ENERGY]     = { 82, 5, 2,    0, 32767 },
+	[BQ27XXX_DM_TERMINATE_VOLTAGE] = { 82, 9, 2, 2500,  3700 },
+};
+
+
 static int poll_interval_param_set(const char *val, const struct kernel_param *kp)
 {
 	struct bq27xxx_device_info *di;
@@ -842,6 +1010,320 @@ static inline int bq27xxx_xfer(struct bq27xxx_device_info *di,
 	return ret;
 }
 
+static int bq27xxx_battery_set_seal_state(struct bq27xxx_device_info *di,
+					  bool state)
+{
+	int ret;
+
+	if (state) {
+		ret = bq27xxx_write(di, BQ27XXX_REG_CTRL, BQ27XXX_SEALED, false);
+		if (ret < 0)
+			goto out;
+	} else {
+		ret = bq27xxx_write(di, BQ27XXX_REG_CTRL, (u16)(di->unseal_key >> 16), false);
+		if (ret < 0)
+			goto out;
+
+		ret = bq27xxx_write(di, BQ27XXX_REG_CTRL, (u16)di->unseal_key, false);
+		if (ret < 0)
+			goto out;
+	}
+	return 0;
+
+out:
+	dev_err(di->dev, "bus error on %s: %d\n", state ? "seal" : "unseal", ret);
+	return ret;
+}
+
+static u8 bq27xxx_battery_checksum_dm_block(struct bq27xxx_dm_buf *buf)
+{
+	u16 sum = 0;
+	int i;
+
+	for (i = 0; i < BQ27XXX_DM_SZ; i++)
+		sum += buf->a[i];
+	sum &= 0xff;
+
+	return 0xff - sum;
+}
+
+static int bq27xxx_battery_read_dm_block(struct bq27xxx_device_info *di,
+					 struct bq27xxx_dm_buf *buf)
+{
+	int ret;
+
+	ret = bq27xxx_write(di, BQ27XXX_DM_CLASS, buf->class, true);
+	if (ret < 0)
+		goto out;
+
+	ret = bq27xxx_write(di, BQ27XXX_DM_BLOCK, buf->block, true);
+	if (ret < 0)
+		goto out;
+
+	BQ27XXX_MSLEEP(1);
+
+	ret = bq27xxx_xfer(di, buf, true);
+	if (ret < 0)
+		goto out;
+
+	ret = bq27xxx_read(di, BQ27XXX_DM_CKSUM, true);
+	if (ret < 0)
+		goto out;
+
+	if ((u8)ret != bq27xxx_battery_checksum_dm_block(buf)) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	buf->full = true;
+	buf->updt = false;
+	return 0;
+
+out:
+	dev_err(di->dev, "bus error reading chip memory: %d\n", ret);
+	return ret;
+}
+
+static void 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 = &di->dm_regs[reg_id];
+	const char* str = bq27xxx_dm_reg_name[reg_id];
+	u16 *prev = bq27xxx_dm_reg_ptr(buf, reg);
+
+	if (prev == NULL) {
+		dev_warn(di->dev, "buffer does not match %s dm spec\n", str);
+		return;
+	}
+
+	if (reg->bytes != 2) {
+		dev_warn(di->dev, "%s dm spec has unsupported byte size\n", str);
+		return;
+	}
+
+	if (!buf->full)
+		return;
+
+	if (be16_to_cpup(prev) == val) {
+		dev_info(di->dev, "%s has %u\n", str, val);
+		return;
+	}
+
+	dev_info(di->dev, "update %s to %u\n", str, val);
+
+	*prev = cpu_to_be16(val);
+	buf->updt = true;
+}
+
+static int bq27xxx_battery_set_cfgupdate(struct bq27xxx_device_info *di, u16 flag)
+{
+	const int limit = 100;
+	int ret, try = limit;
+
+	ret = bq27xxx_write(di, BQ27XXX_REG_CTRL,
+			    flag ? BQ27XXX_SET_CFGUPDATE : BQ27XXX_SOFT_RESET,
+			    false);
+	if (ret < 0)
+		goto out;
+
+	do {
+		BQ27XXX_MSLEEP(25);
+		ret = di->bus.read(di, di->regs[BQ27XXX_REG_FLAGS], false);
+		if (ret < 0)
+			goto out;
+	} while ((ret & BQ27XXX_FLAG_CFGUP) != flag && --try);
+
+	if (!try) {
+		dev_err(di->dev, "timed out waiting for cfgupdate flag %d\n", !!flag);
+		return -EINVAL;
+	}
+
+	if (limit-try > 3)
+		dev_warn(di->dev, "cfgupdate %d, retries %d\n", !!flag, limit-try);
+
+	return 0;
+
+out:
+	dev_err(di->dev, "bus error on %s: %d\n", flag ? "set_cfgupdate" : "soft_reset", ret);
+	return ret;
+}
+
+static int bq27xxx_battery_write_dm_block(struct bq27xxx_device_info *di,
+					  struct bq27xxx_dm_buf *buf)
+{
+	bool cfgup = di->chip == BQ27421; /* assume group supports cfgupdate */
+	int ret;
+
+	if (!buf->updt)
+		return 0;
+
+	if (cfgup) {
+		ret = bq27xxx_battery_set_cfgupdate(di, BQ27XXX_FLAG_CFGUP);
+		if (ret < 0)
+			return ret;
+	}
+
+	ret = bq27xxx_write(di, BQ27XXX_DM_CTRL, 0, true);
+	if (ret < 0)
+		goto out;
+
+	ret = bq27xxx_write(di, BQ27XXX_DM_CLASS, buf->class, true);
+	if (ret < 0)
+		goto out;
+
+	ret = bq27xxx_write(di, BQ27XXX_DM_BLOCK, buf->block, true);
+	if (ret < 0)
+		goto out;
+
+	BQ27XXX_MSLEEP(1);
+
+	ret = bq27xxx_xfer(di, buf, false);
+	if (ret < 0)
+		goto out;
+
+	ret = bq27xxx_write(di, BQ27XXX_DM_CKSUM,
+			    bq27xxx_battery_checksum_dm_block(buf), true);
+	if (ret < 0)
+		goto out;
+
+	/* THE FOLLOWING SEQUENCE IS TOXIC. DO NOT USE!
+	 * If the 'time' delay is insufficient, NVM corruption results on
+	 * the '425 chip (and perhaps others), which could damage the chip.
+	 * It was suggested in this TI tool:
+	 *   http://git.ti.com/bms-linux/bqtool/blobs/master/gauge.c#line328
+	 *
+	 * 1. MSLEEP(time) after above write(BQ27XXX_DM_CKSUM, ...)
+	 * 2. write(BQ27XXX_DM_BLOCK, buf->block)
+	 * 3. sum = read(BQ27XXX_DM_CKSUM)
+	 * 4. if (sum != bq27xxx_battery_checksum_dm_block(buf))
+	 *      report error
+	 */
+
+	if (cfgup) {
+		BQ27XXX_MSLEEP(1);
+		ret = bq27xxx_battery_set_cfgupdate(di, 0);
+		if (ret < 0)
+			return ret;
+	} else {
+		BQ27XXX_MSLEEP(100); /* flash DM updates in <100ms */
+	}
+
+	buf->updt = false;
+	return 0;
+
+out:
+	if (cfgup)
+		bq27xxx_battery_set_cfgupdate(di, 0);
+
+	dev_err(di->dev, "bus error writing chip memory: %d\n", ret);
+	return ret;
+}
+
+static void bq27xxx_battery_set_config(struct bq27xxx_device_info *di,
+				       struct power_supply_battery_info *info)
+{
+	struct bq27xxx_dm_buf bd = BQ27XXX_DM_BUF(di, BQ27XXX_DM_DESIGN_CAPACITY);
+	struct bq27xxx_dm_buf bt = BQ27XXX_DM_BUF(di, BQ27XXX_DM_TERMINATE_VOLTAGE);
+
+	if (info->charge_full_design_uah != -EINVAL
+	 && info->energy_full_design_uwh != -EINVAL) {
+		bq27xxx_battery_read_dm_block(di, &bd);
+		/* assume design energy & capacity are in same block */
+		bq27xxx_battery_update_dm_block(di, &bd,
+					BQ27XXX_DM_DESIGN_CAPACITY,
+					info->charge_full_design_uah / 1000);
+		bq27xxx_battery_update_dm_block(di, &bd,
+					BQ27XXX_DM_DESIGN_ENERGY,
+					info->energy_full_design_uwh / 1000);
+	}
+
+	if (info->voltage_min_design_uv != -EINVAL) {
+		bool same = bd.class == bt.class && bd.block == bt.block;
+		if (!same)
+			bq27xxx_battery_read_dm_block(di, &bt);
+		bq27xxx_battery_update_dm_block(di, same ? &bd : &bt,
+					BQ27XXX_DM_TERMINATE_VOLTAGE,
+					info->voltage_min_design_uv / 1000);
+	}
+
+	bq27xxx_battery_write_dm_block(di, &bd);
+	bq27xxx_battery_write_dm_block(di, &bt);
+
+	if (di->chip != BQ27421) { /* not a cfgupdate chip, so reset */
+		bq27xxx_write(di, BQ27XXX_REG_CTRL, BQ27XXX_RESET, false);
+		BQ27XXX_MSLEEP(300); /* reset time is not documented */
+	}
+	/* assume bq27xxx_battery_update() is called hereafter */
+}
+
+void bq27xxx_battery_settings(struct bq27xxx_device_info *di)
+{
+	struct power_supply_battery_info info = {};
+	unsigned int min, max;
+
+	/* 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 (!di->dm_regs)
+		return;
+
+	if (bq27xxx_battery_set_seal_state(di, false) < 0)
+		return;
+
+	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");
+	}
+
+	/* assume min == 0 */
+	max = di->dm_regs[BQ27XXX_DM_DESIGN_ENERGY].max;
+	if (info.energy_full_design_uwh > max * 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;
+	}
+
+	/* assume min == 0 */
+	max = di->dm_regs[BQ27XXX_DM_DESIGN_CAPACITY].max;
+	if (info.charge_full_design_uah > max * 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;
+	}
+
+	min = di->dm_regs[BQ27XXX_DM_TERMINATE_VOLTAGE].min;
+	max = di->dm_regs[BQ27XXX_DM_TERMINATE_VOLTAGE].max;
+	if ((info.voltage_min_design_uv < min * 1000
+	  || info.voltage_min_design_uv > 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)
+		bq27xxx_battery_set_config(di, &info);
+
+out:
+	bq27xxx_battery_set_seal_state(di, true);
+}
+
 /*
  * Return the battery State-of-Charge
  * Or < 0 if something fails.
@@ -1356,6 +1838,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;
@@ -1386,17 +1875,25 @@ static void bq27xxx_external_power_changed(struct power_supply *psy)
 	schedule_delayed_work(&di->work, 0);
 }
 
+#define BQ27XXX_INIT(c,d,k)   \
+	di->chip       = (c); \
+	di->dm_regs    = (d); \
+	di->unseal_key = (k)
+
 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,
+	};
 
 	switch(di->chip) {
 	case BQ27000:
 	case BQ27010:
 	case BQ2750X:
-	case BQ2751X:
-	case BQ27500:
+	case BQ2751X: break;
+	case BQ27500: BQ27XXX_INIT(BQ27500, bq27500_dm_regs, 0x04143672); break;
 	case BQ27510G1:
 	case BQ27510G2:
 	case BQ27510G3:
@@ -1405,17 +1902,17 @@ int bq27xxx_battery_setup(struct bq27xxx_device_info *di)
 	case BQ27520G3:
 	case BQ27520G4:
 	case BQ27530:
-	case BQ27541:
-	case BQ27545:
-	case BQ27421: break;
+	case BQ27541: break;
+	case BQ27545: BQ27XXX_INIT(BQ27545, bq27545_dm_regs, 0x04143672); break;
+	case BQ27421: BQ27XXX_INIT(BQ27421, bq27421_dm_regs, 0x80008000); break;
 	case BQ2752X: di->chip = BQ2751X; break;
 	case BQ27531: di->chip = BQ27530; break;
 	case BQ27542: di->chip = BQ27541; break;
 	case BQ27546: di->chip = BQ27541; break;
 	case BQ27742: di->chip = BQ27541; break;
-	case BQ27425: di->chip = BQ27421; break;
-	case BQ27441: di->chip = BQ27421; break;
-	case BQ27621: di->chip = BQ27421; break;
+	case BQ27425: BQ27XXX_INIT(BQ27421, bq27425_dm_regs, 0x04143672); break;
+	case BQ27441: BQ27XXX_INIT(BQ27421, bq27421_dm_regs, 0x80008000); break;
+	case BQ27621: BQ27XXX_INIT(BQ27421, bq27621_dm_regs, 0x80008000); break;
 	}
 
 	INIT_DELAYED_WORK(&di->work, bq27xxx_battery_poll);
@@ -1441,6 +1938,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);
diff --git a/include/linux/power/bq27xxx_battery.h b/include/linux/power/bq27xxx_battery.h
index 96cec17..014e59f 100644
--- a/include/linux/power/bq27xxx_battery.h
+++ b/include/linux/power/bq27xxx_battery.h
@@ -75,6 +75,8 @@ struct bq27xxx_device_info {
 	int id;
 	enum bq27xxx_chip chip;
 	const char *name;
+	struct bq27xxx_dm_reg *dm_regs;
+	u32 unseal_key;
 	struct bq27xxx_access_methods bus;
 	struct bq27xxx_reg_cache cache;
 	int charge_design_full;
-- 
2.9.3

[PATCH v11 09/10] 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
power_supply_battery_info code.

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

diff --git a/drivers/power/supply/bq27xxx_battery_i2c.c b/drivers/power/supply/bq27xxx_battery_i2c.c
index b3f2494..0b11ed4 100644
--- a/drivers/power/supply/bq27xxx_battery_i2c.c
+++ b/drivers/power/supply/bq27xxx_battery_i2c.c
@@ -38,7 +38,7 @@ static int bq27xxx_battery_i2c_read(struct bq27xxx_device_info *di, u8 reg,
 {
 	struct i2c_client *client = to_i2c_client(di->dev);
 	struct i2c_msg msg[2];
-	unsigned char data[2];
+	u8 data[2];
 	int ret;
 
 	if (!client->adapter)
@@ -68,6 +68,82 @@ 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;
+	u8 data[4];
+	int ret;
+
+	if (!client->adapter)
+		return -ENODEV;
+
+	data[0] = reg;
+	if (single) {
+		data[1] = (u8) value;
+		msg.len = 2;
+	} else {
+		put_unaligned_le16(value, &data[1]);
+		msg.len = 3;
+	}
+
+	msg.buf = data;
+	msg.addr = client->addr;
+	msg.flags = 0;
+
+	ret = i2c_transfer(client->adapter, &msg, 1);
+	if (ret < 0)
+		return ret;
+	if (ret != 1)
+		return -EINVAL;
+	return 0;
+}
+
+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);
+	int ret;
+
+	if (!client->adapter)
+		return -ENODEV;
+
+	ret = i2c_smbus_read_i2c_block_data(client, reg, len, data);
+	if (ret < 0)
+		return ret;
+	if (ret != len)
+		return -EINVAL;
+	return 0;
+}
+
+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];
+	int ret;
+
+	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;
+
+	ret = i2c_transfer(client->adapter, &msg, 1);
+	if (ret < 0)
+		return ret;
+	if (ret != 1)
+		return -EINVAL;
+	return 0;
+}
+
 static int bq27xxx_battery_i2c_probe(struct i2c_client *client,
 				     const struct i2c_device_id *id)
 {
@@ -95,7 +171,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

[PATCH v11 10/10] power: bq27xxx_battery: Remove duplicate register arrays

[Liam Breck]

From: Liam Breck <kernel@networkimprov.net>

BQ2751X & BQ27510G3 are identical.
BQ27500 & BQ27510G1 & BQ27510G2 are identical.
Make BQ2751X & BQ27510G1/2 category members.
Remove the duplicate arrays, etc.

Signed-off-by: Liam Breck <kernel@networkimprov.net>
---
 drivers/power/supply/bq27xxx_battery.c | 148 ++-------------------------------
 include/linux/power/bq27xxx_battery.h  |  12 +--
 2 files changed, 11 insertions(+), 149 deletions(-)

diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
index 7f08f36..ccc74e6 100644
--- a/drivers/power/supply/bq27xxx_battery.c
+++ b/drivers/power/supply/bq27xxx_battery.c
@@ -197,30 +197,6 @@ static u8 bq27xxx_regs[][BQ27XXX_REG_MAX] = {
 		[BQ27XXX_DM_DATA] = 0x40,
 		[BQ27XXX_DM_CKSUM] = 0x60,
 	},
-	[BQ2751X] = {
-		[BQ27XXX_REG_CTRL] = 0x00,
-		[BQ27XXX_REG_TEMP] = 0x06,
-		[BQ27XXX_REG_INT_TEMP] = 0x28,
-		[BQ27XXX_REG_VOLT] = 0x08,
-		[BQ27XXX_REG_AI] = 0x14,
-		[BQ27XXX_REG_FLAGS] = 0x0a,
-		[BQ27XXX_REG_TTE] = 0x16,
-		[BQ27XXX_REG_TTF] = INVALID_REG_ADDR,
-		[BQ27XXX_REG_TTES] = 0x1a,
-		[BQ27XXX_REG_TTECP] = INVALID_REG_ADDR,
-		[BQ27XXX_REG_NAC] = 0x0c,
-		[BQ27XXX_REG_FCC] = 0x12,
-		[BQ27XXX_REG_CYCT] = 0x1e,
-		[BQ27XXX_REG_AE] = INVALID_REG_ADDR,
-		[BQ27XXX_REG_SOC] = 0x20,
-		[BQ27XXX_REG_DCAP] = 0x2e,
-		[BQ27XXX_REG_AP] = INVALID_REG_ADDR,
-		[BQ27XXX_DM_CTRL] = 0x61,
-		[BQ27XXX_DM_CLASS] = 0x3e,
-		[BQ27XXX_DM_BLOCK] = 0x3f,
-		[BQ27XXX_DM_DATA] = 0x40,
-		[BQ27XXX_DM_CKSUM] = 0x60,
-	},
 	[BQ27500] = {
 		[BQ27XXX_REG_CTRL] = 0x00,
 		[BQ27XXX_REG_TEMP] = 0x06,
@@ -245,54 +221,6 @@ static u8 bq27xxx_regs[][BQ27XXX_REG_MAX] = {
 		[BQ27XXX_DM_DATA] = 0x40,
 		[BQ27XXX_DM_CKSUM] = 0x60,
 	},
-	[BQ27510G1] = {
-		[BQ27XXX_REG_CTRL] = 0x00,
-		[BQ27XXX_REG_TEMP] = 0x06,
-		[BQ27XXX_REG_INT_TEMP] = INVALID_REG_ADDR,
-		[BQ27XXX_REG_VOLT] = 0x08,
-		[BQ27XXX_REG_AI] = 0x14,
-		[BQ27XXX_REG_FLAGS] = 0x0a,
-		[BQ27XXX_REG_TTE] = 0x16,
-		[BQ27XXX_REG_TTF] = 0x18,
-		[BQ27XXX_REG_TTES] = 0x1c,
-		[BQ27XXX_REG_TTECP] = 0x26,
-		[BQ27XXX_REG_NAC] = 0x0c,
-		[BQ27XXX_REG_FCC] = 0x12,
-		[BQ27XXX_REG_CYCT] = 0x2a,
-		[BQ27XXX_REG_AE] = 0x22,
-		[BQ27XXX_REG_SOC] = 0x2c,
-		[BQ27XXX_REG_DCAP] = 0x3c,
-		[BQ27XXX_REG_AP] = 0x24,
-		[BQ27XXX_DM_CTRL] = 0x61,
-		[BQ27XXX_DM_CLASS] = 0x3e,
-		[BQ27XXX_DM_BLOCK] = 0x3f,
-		[BQ27XXX_DM_DATA] = 0x40,
-		[BQ27XXX_DM_CKSUM] = 0x60,
-	},
-	[BQ27510G2] = {
-		[BQ27XXX_REG_CTRL] = 0x00,
-		[BQ27XXX_REG_TEMP] = 0x06,
-		[BQ27XXX_REG_INT_TEMP] = INVALID_REG_ADDR,
-		[BQ27XXX_REG_VOLT] = 0x08,
-		[BQ27XXX_REG_AI] = 0x14,
-		[BQ27XXX_REG_FLAGS] = 0x0a,
-		[BQ27XXX_REG_TTE] = 0x16,
-		[BQ27XXX_REG_TTF] = 0x18,
-		[BQ27XXX_REG_TTES] = 0x1c,
-		[BQ27XXX_REG_TTECP] = 0x26,
-		[BQ27XXX_REG_NAC] = 0x0c,
-		[BQ27XXX_REG_FCC] = 0x12,
-		[BQ27XXX_REG_CYCT] = 0x2a,
-		[BQ27XXX_REG_AE] = 0x22,
-		[BQ27XXX_REG_SOC] = 0x2c,
-		[BQ27XXX_REG_DCAP] = 0x3c,
-		[BQ27XXX_REG_AP] = 0x24,
-		[BQ27XXX_DM_CTRL] = 0x61,
-		[BQ27XXX_DM_CLASS] = 0x3e,
-		[BQ27XXX_DM_BLOCK] = 0x3f,
-		[BQ27XXX_DM_DATA] = 0x40,
-		[BQ27XXX_DM_CKSUM] = 0x60,
-	},
 	[BQ27510G3] = {
 		[BQ27XXX_REG_CTRL] = 0x00,
 		[BQ27XXX_REG_TEMP] = 0x06,
@@ -571,24 +499,6 @@ static enum power_supply_property bq2750x_battery_props[] = {
 	POWER_SUPPLY_PROP_MANUFACTURER,
 };
 
-static enum power_supply_property bq2751x_battery_props[] = {
-	POWER_SUPPLY_PROP_STATUS,
-	POWER_SUPPLY_PROP_PRESENT,
-	POWER_SUPPLY_PROP_VOLTAGE_NOW,
-	POWER_SUPPLY_PROP_CURRENT_NOW,
-	POWER_SUPPLY_PROP_CAPACITY,
-	POWER_SUPPLY_PROP_CAPACITY_LEVEL,
-	POWER_SUPPLY_PROP_TEMP,
-	POWER_SUPPLY_PROP_TIME_TO_EMPTY_NOW,
-	POWER_SUPPLY_PROP_TECHNOLOGY,
-	POWER_SUPPLY_PROP_CHARGE_FULL,
-	POWER_SUPPLY_PROP_CHARGE_NOW,
-	POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN,
-	POWER_SUPPLY_PROP_CYCLE_COUNT,
-	POWER_SUPPLY_PROP_HEALTH,
-	POWER_SUPPLY_PROP_MANUFACTURER,
-};
-
 static enum power_supply_property bq27500_battery_props[] = {
 	POWER_SUPPLY_PROP_STATUS,
 	POWER_SUPPLY_PROP_PRESENT,
@@ -610,48 +520,6 @@ static enum power_supply_property bq27500_battery_props[] = {
 	POWER_SUPPLY_PROP_MANUFACTURER,
 };
 
-static enum power_supply_property bq27510g1_battery_props[] = {
-	POWER_SUPPLY_PROP_STATUS,
-	POWER_SUPPLY_PROP_PRESENT,
-	POWER_SUPPLY_PROP_VOLTAGE_NOW,
-	POWER_SUPPLY_PROP_CURRENT_NOW,
-	POWER_SUPPLY_PROP_CAPACITY,
-	POWER_SUPPLY_PROP_CAPACITY_LEVEL,
-	POWER_SUPPLY_PROP_TEMP,
-	POWER_SUPPLY_PROP_TIME_TO_EMPTY_NOW,
-	POWER_SUPPLY_PROP_TIME_TO_FULL_NOW,
-	POWER_SUPPLY_PROP_TECHNOLOGY,
-	POWER_SUPPLY_PROP_CHARGE_FULL,
-	POWER_SUPPLY_PROP_CHARGE_NOW,
-	POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN,
-	POWER_SUPPLY_PROP_CYCLE_COUNT,
-	POWER_SUPPLY_PROP_ENERGY_NOW,
-	POWER_SUPPLY_PROP_POWER_AVG,
-	POWER_SUPPLY_PROP_HEALTH,
-	POWER_SUPPLY_PROP_MANUFACTURER,
-};
-
-static enum power_supply_property bq27510g2_battery_props[] = {
-	POWER_SUPPLY_PROP_STATUS,
-	POWER_SUPPLY_PROP_PRESENT,
-	POWER_SUPPLY_PROP_VOLTAGE_NOW,
-	POWER_SUPPLY_PROP_CURRENT_NOW,
-	POWER_SUPPLY_PROP_CAPACITY,
-	POWER_SUPPLY_PROP_CAPACITY_LEVEL,
-	POWER_SUPPLY_PROP_TEMP,
-	POWER_SUPPLY_PROP_TIME_TO_EMPTY_NOW,
-	POWER_SUPPLY_PROP_TIME_TO_FULL_NOW,
-	POWER_SUPPLY_PROP_TECHNOLOGY,
-	POWER_SUPPLY_PROP_CHARGE_FULL,
-	POWER_SUPPLY_PROP_CHARGE_NOW,
-	POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN,
-	POWER_SUPPLY_PROP_CYCLE_COUNT,
-	POWER_SUPPLY_PROP_ENERGY_NOW,
-	POWER_SUPPLY_PROP_POWER_AVG,
-	POWER_SUPPLY_PROP_HEALTH,
-	POWER_SUPPLY_PROP_MANUFACTURER,
-};
-
 static enum power_supply_property bq27510g3_battery_props[] = {
 	POWER_SUPPLY_PROP_STATUS,
 	POWER_SUPPLY_PROP_PRESENT,
@@ -831,10 +699,7 @@ static struct {
 	BQ27XXX_PROP(BQ27000, bq27000_battery_props),
 	BQ27XXX_PROP(BQ27010, bq27010_battery_props),
 	BQ27XXX_PROP(BQ2750X, bq2750x_battery_props),
-	BQ27XXX_PROP(BQ2751X, bq2751x_battery_props),
 	BQ27XXX_PROP(BQ27500, bq27500_battery_props),
-	BQ27XXX_PROP(BQ27510G1, bq27510g1_battery_props),
-	BQ27XXX_PROP(BQ27510G2, bq27510g2_battery_props),
 	BQ27XXX_PROP(BQ27510G3, bq27510g3_battery_props),
 	BQ27XXX_PROP(BQ27520G1, bq27520g1_battery_props),
 	BQ27XXX_PROP(BQ27520G2, bq27520g2_battery_props),
@@ -1521,10 +1386,7 @@ static bool bq27xxx_battery_overtemp(struct bq27xxx_device_info *di, u16 flags)
 {
 	switch (di->chip) {
 	case BQ2750X:
-	case BQ2751X:
 	case BQ27500:
-	case BQ27510G1:
-	case BQ27510G2:
 	case BQ27510G3:
 	case BQ27520G1:
 	case BQ27520G2:
@@ -1891,11 +1753,8 @@ int bq27xxx_battery_setup(struct bq27xxx_device_info *di)
 	switch(di->chip) {
 	case BQ27000:
 	case BQ27010:
-	case BQ2750X:
-	case BQ2751X: break;
+	case BQ2750X: break;
 	case BQ27500: BQ27XXX_INIT(BQ27500, bq27500_dm_regs, 0x04143672); break;
-	case BQ27510G1:
-	case BQ27510G2:
 	case BQ27510G3:
 	case BQ27520G1:
 	case BQ27520G2:
@@ -1905,7 +1764,10 @@ int bq27xxx_battery_setup(struct bq27xxx_device_info *di)
 	case BQ27541: break;
 	case BQ27545: BQ27XXX_INIT(BQ27545, bq27545_dm_regs, 0x04143672); break;
 	case BQ27421: BQ27XXX_INIT(BQ27421, bq27421_dm_regs, 0x80008000); break;
-	case BQ2752X: di->chip = BQ2751X; break;
+	case BQ2751X: di->chip = BQ27510G3; break;
+	case BQ2752X: di->chip = BQ27510G3; break;
+	case BQ27510G1: di->chip = BQ27500; break;
+	case BQ27510G2: di->chip = BQ27500; break;
 	case BQ27531: di->chip = BQ27530; break;
 	case BQ27542: di->chip = BQ27541; break;
 	case BQ27546: di->chip = BQ27541; break;
diff --git a/include/linux/power/bq27xxx_battery.h b/include/linux/power/bq27xxx_battery.h
index 014e59f..81f7ffa 100644
--- a/include/linux/power/bq27xxx_battery.h
+++ b/include/linux/power/bq27xxx_battery.h
@@ -6,11 +6,8 @@ enum bq27xxx_chip {
 	BQ27000 = 1, /* bq27000, bq27200 */
 	BQ27010, /* bq27010, bq27210 */
 	BQ2750X, /* bq27500 deprecated alias */
-	BQ2751X, /* bq27510, bq27520 deprecated alias */
-	BQ27500, /* bq27500/1 */
-	BQ27510G1, /* bq27510G1 */
-	BQ27510G2, /* bq27510G2 */
-	BQ27510G3, /* bq27510G3 */
+	BQ27500, /* bq27500/1, bq27510G1, bq27510G2 */
+	BQ27510G3, /* bq27510G3, bq27510, bq27520 */
 	BQ27520G1, /* bq27520G1 */
 	BQ27520G2, /* bq27520G2 */
 	BQ27520G3, /* bq27520G3 */
@@ -21,7 +18,10 @@ enum bq27xxx_chip {
 	BQ27421, /* bq27421, bq27425, bq27441, bq27621 */
 
 	/* members of categories; translate these to category in _setup() */
-	BQ2752X, /* deprecated alias */
+	BQ2751X, /* bq27510 deprecated alias */
+	BQ2752X, /* bq27520 deprecated alias */
+	BQ27510G1,
+	BQ27510G2,
 	BQ27531,
 	BQ27542,
 	BQ27546,
-- 
2.9.3

Fwd: [PATCH v11 10/10] power: bq27xxx_battery: Remove duplicate register arrays

[Liam Breck]

Resend to list...


---------- Forwarded message ----------
Date: Mon, Mar 20, 2017 at 6:23 PM
To: Chris@lapa.com.au
Cc: Matt Ranostay <matt@ranostay.consulting>,
linux-pm@vger.kernel.org, "Andrew F. Davis" <afd@ti.com>, Sebastian
Reichel <sre@kernel.org>

Chris & Andrew,

Can we make id bq2750x an alias for bq27500 and remove the associated arrays?

This patch does that for three IDs which are dups, although bq2750x &
bq27500 are not exact dups.


On Mar 20, 2017 2:44 AM, "Liam Breck" <liam@networkimprov.net> wrote:
>
> From: Liam Breck <kernel@networkimprov.net>
>
> BQ2751X & BQ27510G3 are identical.
> BQ27500 & BQ27510G1 & BQ27510G2 are identical.
> Make BQ2751X & BQ27510G1/2 category members.
> Remove the duplicate arrays, etc.
>
> Signed-off-by: Liam Breck <kernel@networkimprov.net>
> ---
>  drivers/power/supply/bq27xxx_battery.c | 148 ++-------------------------------
>  include/linux/power/bq27xxx_battery.h  |  12 +--
>  2 files changed, 11 insertions(+), 149 deletions(-)
>
> diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
> index 7f08f36..ccc74e6 100644
> --- a/drivers/power/supply/bq27xxx_battery.c
> +++ b/drivers/power/supply/bq27xxx_battery.c
> @@ -197,30 +197,6 @@ static u8 bq27xxx_regs[][BQ27XXX_REG_MAX] = {
>                 [BQ27XXX_DM_DATA] = 0x40,
>                 [BQ27XXX_DM_CKSUM] = 0x60,
>         },
> -       [BQ2751X] = {
> -               [BQ27XXX_REG_CTRL] = 0x00,
> -               [BQ27XXX_REG_TEMP] = 0x06,
> -               [BQ27XXX_REG_INT_TEMP] = 0x28,
> -               [BQ27XXX_REG_VOLT] = 0x08,
> -               [BQ27XXX_REG_AI] = 0x14,
> -               [BQ27XXX_REG_FLAGS] = 0x0a,
> -               [BQ27XXX_REG_TTE] = 0x16,
> -               [BQ27XXX_REG_TTF] = INVALID_REG_ADDR,
> -               [BQ27XXX_REG_TTES] = 0x1a,
> -               [BQ27XXX_REG_TTECP] = INVALID_REG_ADDR,
> -               [BQ27XXX_REG_NAC] = 0x0c,
> -               [BQ27XXX_REG_FCC] = 0x12,
> -               [BQ27XXX_REG_CYCT] = 0x1e,
> -               [BQ27XXX_REG_AE] = INVALID_REG_ADDR,
> -               [BQ27XXX_REG_SOC] = 0x20,
> -               [BQ27XXX_REG_DCAP] = 0x2e,
> -               [BQ27XXX_REG_AP] = INVALID_REG_ADDR,
> -               [BQ27XXX_DM_CTRL] = 0x61,
> -               [BQ27XXX_DM_CLASS] = 0x3e,
> -               [BQ27XXX_DM_BLOCK] = 0x3f,
> -               [BQ27XXX_DM_DATA] = 0x40,
> -               [BQ27XXX_DM_CKSUM] = 0x60,
> -       },
>         [BQ27500] = {
>                 [BQ27XXX_REG_CTRL] = 0x00,
>                 [BQ27XXX_REG_TEMP] = 0x06,
> @@ -245,54 +221,6 @@ static u8 bq27xxx_regs[][BQ27XXX_REG_MAX] = {
>                 [BQ27XXX_DM_DATA] = 0x40,
>                 [BQ27XXX_DM_CKSUM] = 0x60,
>         },
> -       [BQ27510G1] = {
> -               [BQ27XXX_REG_CTRL] = 0x00,
> -               [BQ27XXX_REG_TEMP] = 0x06,
> -               [BQ27XXX_REG_INT_TEMP] = INVALID_REG_ADDR,
> -               [BQ27XXX_REG_VOLT] = 0x08,
> -               [BQ27XXX_REG_AI] = 0x14,
> -               [BQ27XXX_REG_FLAGS] = 0x0a,
> -               [BQ27XXX_REG_TTE] = 0x16,
> -               [BQ27XXX_REG_TTF] = 0x18,
> -               [BQ27XXX_REG_TTES] = 0x1c,
> -               [BQ27XXX_REG_TTECP] = 0x26,
> -               [BQ27XXX_REG_NAC] = 0x0c,
> -               [BQ27XXX_REG_FCC] = 0x12,
> -               [BQ27XXX_REG_CYCT] = 0x2a,
> -               [BQ27XXX_REG_AE] = 0x22,
> -               [BQ27XXX_REG_SOC] = 0x2c,
> -               [BQ27XXX_REG_DCAP] = 0x3c,
> -               [BQ27XXX_REG_AP] = 0x24,
> -               [BQ27XXX_DM_CTRL] = 0x61,
> -               [BQ27XXX_DM_CLASS] = 0x3e,
> -               [BQ27XXX_DM_BLOCK] = 0x3f,
> -               [BQ27XXX_DM_DATA] = 0x40,
> -               [BQ27XXX_DM_CKSUM] = 0x60,
> -       },
> -       [BQ27510G2] = {
> -               [BQ27XXX_REG_CTRL] = 0x00,
> -               [BQ27XXX_REG_TEMP] = 0x06,
> -               [BQ27XXX_REG_INT_TEMP] = INVALID_REG_ADDR,
> -               [BQ27XXX_REG_VOLT] = 0x08,
> -               [BQ27XXX_REG_AI] = 0x14,
> -               [BQ27XXX_REG_FLAGS] = 0x0a,
> -               [BQ27XXX_REG_TTE] = 0x16,
> -               [BQ27XXX_REG_TTF] = 0x18,
> -               [BQ27XXX_REG_TTES] = 0x1c,
> -               [BQ27XXX_REG_TTECP] = 0x26,
> -               [BQ27XXX_REG_NAC] = 0x0c,
> -               [BQ27XXX_REG_FCC] = 0x12,
> -               [BQ27XXX_REG_CYCT] = 0x2a,
> -               [BQ27XXX_REG_AE] = 0x22,
> -               [BQ27XXX_REG_SOC] = 0x2c,
> -               [BQ27XXX_REG_DCAP] = 0x3c,
> -               [BQ27XXX_REG_AP] = 0x24,
> -               [BQ27XXX_DM_CTRL] = 0x61,
> -               [BQ27XXX_DM_CLASS] = 0x3e,
> -               [BQ27XXX_DM_BLOCK] = 0x3f,
> -               [BQ27XXX_DM_DATA] = 0x40,
> -               [BQ27XXX_DM_CKSUM] = 0x60,
> -       },
>         [BQ27510G3] = {
>                 [BQ27XXX_REG_CTRL] = 0x00,
>                 [BQ27XXX_REG_TEMP] = 0x06,
> @@ -571,24 +499,6 @@ static enum power_supply_property bq2750x_battery_props[] = {
>         POWER_SUPPLY_PROP_MANUFACTURER,
>  };
>
> -static enum power_supply_property bq2751x_battery_props[] = {
> -       POWER_SUPPLY_PROP_STATUS,
> -       POWER_SUPPLY_PROP_PRESENT,
> -       POWER_SUPPLY_PROP_VOLTAGE_NOW,
> -       POWER_SUPPLY_PROP_CURRENT_NOW,
> -       POWER_SUPPLY_PROP_CAPACITY,
> -       POWER_SUPPLY_PROP_CAPACITY_LEVEL,
> -       POWER_SUPPLY_PROP_TEMP,
> -       POWER_SUPPLY_PROP_TIME_TO_EMPTY_NOW,
> -       POWER_SUPPLY_PROP_TECHNOLOGY,
> -       POWER_SUPPLY_PROP_CHARGE_FULL,
> -       POWER_SUPPLY_PROP_CHARGE_NOW,
> -       POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN,
> -       POWER_SUPPLY_PROP_CYCLE_COUNT,
> -       POWER_SUPPLY_PROP_HEALTH,
> -       POWER_SUPPLY_PROP_MANUFACTURER,
> -};
> -
>  static enum power_supply_property bq27500_battery_props[] = {
>         POWER_SUPPLY_PROP_STATUS,
>         POWER_SUPPLY_PROP_PRESENT,
> @@ -610,48 +520,6 @@ static enum power_supply_property bq27500_battery_props[] = {
>         POWER_SUPPLY_PROP_MANUFACTURER,
>  };
>
> -static enum power_supply_property bq27510g1_battery_props[] = {
> -       POWER_SUPPLY_PROP_STATUS,
> -       POWER_SUPPLY_PROP_PRESENT,
> -       POWER_SUPPLY_PROP_VOLTAGE_NOW,
> -       POWER_SUPPLY_PROP_CURRENT_NOW,
> -       POWER_SUPPLY_PROP_CAPACITY,
> -       POWER_SUPPLY_PROP_CAPACITY_LEVEL,
> -       POWER_SUPPLY_PROP_TEMP,
> -       POWER_SUPPLY_PROP_TIME_TO_EMPTY_NOW,
> -       POWER_SUPPLY_PROP_TIME_TO_FULL_NOW,
> -       POWER_SUPPLY_PROP_TECHNOLOGY,
> -       POWER_SUPPLY_PROP_CHARGE_FULL,
> -       POWER_SUPPLY_PROP_CHARGE_NOW,
> -       POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN,
> -       POWER_SUPPLY_PROP_CYCLE_COUNT,
> -       POWER_SUPPLY_PROP_ENERGY_NOW,
> -       POWER_SUPPLY_PROP_POWER_AVG,
> -       POWER_SUPPLY_PROP_HEALTH,
> -       POWER_SUPPLY_PROP_MANUFACTURER,
> -};
> -
> -static enum power_supply_property bq27510g2_battery_props[] = {
> -       POWER_SUPPLY_PROP_STATUS,
> -       POWER_SUPPLY_PROP_PRESENT,
> -       POWER_SUPPLY_PROP_VOLTAGE_NOW,
> -       POWER_SUPPLY_PROP_CURRENT_NOW,
> -       POWER_SUPPLY_PROP_CAPACITY,
> -       POWER_SUPPLY_PROP_CAPACITY_LEVEL,
> -       POWER_SUPPLY_PROP_TEMP,
> -       POWER_SUPPLY_PROP_TIME_TO_EMPTY_NOW,
> -       POWER_SUPPLY_PROP_TIME_TO_FULL_NOW,
> -       POWER_SUPPLY_PROP_TECHNOLOGY,
> -       POWER_SUPPLY_PROP_CHARGE_FULL,
> -       POWER_SUPPLY_PROP_CHARGE_NOW,
> -       POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN,
> -       POWER_SUPPLY_PROP_CYCLE_COUNT,
> -       POWER_SUPPLY_PROP_ENERGY_NOW,
> -       POWER_SUPPLY_PROP_POWER_AVG,
> -       POWER_SUPPLY_PROP_HEALTH,
> -       POWER_SUPPLY_PROP_MANUFACTURER,
> -};
> -
>  static enum power_supply_property bq27510g3_battery_props[] = {
>         POWER_SUPPLY_PROP_STATUS,
>         POWER_SUPPLY_PROP_PRESENT,
> @@ -831,10 +699,7 @@ static struct {
>         BQ27XXX_PROP(BQ27000, bq27000_battery_props),
>         BQ27XXX_PROP(BQ27010, bq27010_battery_props),
>         BQ27XXX_PROP(BQ2750X, bq2750x_battery_props),
> -       BQ27XXX_PROP(BQ2751X, bq2751x_battery_props),
>         BQ27XXX_PROP(BQ27500, bq27500_battery_props),
> -       BQ27XXX_PROP(BQ27510G1, bq27510g1_battery_props),
> -       BQ27XXX_PROP(BQ27510G2, bq27510g2_battery_props),
>         BQ27XXX_PROP(BQ27510G3, bq27510g3_battery_props),
>         BQ27XXX_PROP(BQ27520G1, bq27520g1_battery_props),
>         BQ27XXX_PROP(BQ27520G2, bq27520g2_battery_props),
> @@ -1521,10 +1386,7 @@ static bool bq27xxx_battery_overtemp(struct bq27xxx_device_info *di, u16 flags)
>  {
>         switch (di->chip) {
>         case BQ2750X:
> -       case BQ2751X:
>         case BQ27500:
> -       case BQ27510G1:
> -       case BQ27510G2:
>         case BQ27510G3:
>         case BQ27520G1:
>         case BQ27520G2:
> @@ -1891,11 +1753,8 @@ int bq27xxx_battery_setup(struct bq27xxx_device_info *di)
>         switch(di->chip) {
>         case BQ27000:
>         case BQ27010:
> -       case BQ2750X:
> -       case BQ2751X: break;
> +       case BQ2750X: break;
>         case BQ27500: BQ27XXX_INIT(BQ27500, bq27500_dm_regs, 0x04143672); break;
> -       case BQ27510G1:
> -       case BQ27510G2:
>         case BQ27510G3:
>         case BQ27520G1:
>         case BQ27520G2:
> @@ -1905,7 +1764,10 @@ int bq27xxx_battery_setup(struct bq27xxx_device_info *di)
>         case BQ27541: break;
>         case BQ27545: BQ27XXX_INIT(BQ27545, bq27545_dm_regs, 0x04143672); break;
>         case BQ27421: BQ27XXX_INIT(BQ27421, bq27421_dm_regs, 0x80008000); break;
> -       case BQ2752X: di->chip = BQ2751X; break;
> +       case BQ2751X: di->chip = BQ27510G3; break;
> +       case BQ2752X: di->chip = BQ27510G3; break;
> +       case BQ27510G1: di->chip = BQ27500; break;
> +       case BQ27510G2: di->chip = BQ27500; break;
>         case BQ27531: di->chip = BQ27530; break;
>         case BQ27542: di->chip = BQ27541; break;
>         case BQ27546: di->chip = BQ27541; break;
> diff --git a/include/linux/power/bq27xxx_battery.h b/include/linux/power/bq27xxx_battery.h
> index 014e59f..81f7ffa 100644
> --- a/include/linux/power/bq27xxx_battery.h
> +++ b/include/linux/power/bq27xxx_battery.h
> @@ -6,11 +6,8 @@ enum bq27xxx_chip {
>         BQ27000 = 1, /* bq27000, bq27200 */
>         BQ27010, /* bq27010, bq27210 */
>         BQ2750X, /* bq27500 deprecated alias */
> -       BQ2751X, /* bq27510, bq27520 deprecated alias */
> -       BQ27500, /* bq27500/1 */
> -       BQ27510G1, /* bq27510G1 */
> -       BQ27510G2, /* bq27510G2 */
> -       BQ27510G3, /* bq27510G3 */
> +       BQ27500, /* bq27500/1, bq27510G1, bq27510G2 */
> +       BQ27510G3, /* bq27510G3, bq27510, bq27520 */
>         BQ27520G1, /* bq27520G1 */
>         BQ27520G2, /* bq27520G2 */
>         BQ27520G3, /* bq27520G3 */
> @@ -21,7 +18,10 @@ enum bq27xxx_chip {
>         BQ27421, /* bq27421, bq27425, bq27441, bq27621 */
>
>         /* members of categories; translate these to category in _setup() */
> -       BQ2752X, /* deprecated alias */
> +       BQ2751X, /* bq27510 deprecated alias */
> +       BQ2752X, /* bq27520 deprecated alias */
> +       BQ27510G1,
> +       BQ27510G2,
>         BQ27531,
>         BQ27542,
>         BQ27546,
> --
> 2.9.3