devicetree battery support and client bq27xxx_battery

From: Liam Breck <[email protected]>

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 <[email protected]>

Cc [email protected]

Signed-off-by Matt Ranostay <[email protected]>

Signed-off-by Liam Breck <[email protected]>

---
 .../devicetree/bindings/power/supply/battery.txt   | 45 ++++++++++++++++++++++
 1 file changed, 45 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/power/supply/battery.txt

Documentation/devicetree/bindings/power/supply/battery.txt new file +0 -0

new file mode 100644
index 0000000..0278617

Documentation/devicetree/bindings/power/supply/battery.txt new file +20 -0

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

Signature · 1 line

2.9.3

Hi Liam & Matt,

Sorry for my absence in the discussion. I skipped the previous iterations for now and start with this version.

Quoted 21 lines · Liam Breck

From: Liam Breck <[email protected]>

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 <[email protected]>

Cc [email protected]

Signed-off-by Matt Ranostay <[email protected]>

Signed-off-by Liam Breck <[email protected]>

---
 .../devicetree/bindings/power/supply/battery.txt   | 45 ++++++++++++++++++++++
 1 file changed, 45 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/power/supply/battery.txt

Documentation/devicetree/bindings/power/supply/battery.txt new file +0 -0

new file mode 100644
index 0000000..0278617

Documentation/devicetree/bindings/power/supply/battery.txt line 1

Quoted 1 line

Documentation/devicetree/bindings/power/supply/battery.txt new file +1 -0

@@ -0,0 +1,45 @@
+Battery Characteristics

Maybe add something like

This device provides static battery information, that is usually available in the EEPROM of a smart battery. It's supposed to be used for batteries, which do not have their own EEPROM (or if its unusable).

Documentation/devicetree/bindings/power/supply/battery.txt line 4

Quoted 2 lines

Documentation/devicetree/bindings/power/supply/battery.txt new file +2 -0

+Required Properties:
+ - compatible: Must be "fixed-battery"

Documentation/devicetree/bindings/power/supply/battery.txt line 9

Quoted 4 lines

Documentation/devicetree/bindings/power/supply/battery.txt new file +4 -0

+Optional Properties:
+ - voltage-min-design-microvolt: drained battery voltage
+ - energy-full-design-microwatt-hours: battery design energy
+ - charge-full-design-microamp-hours: battery design capacity

Looks fine to me.

Documentation/devicetree/bindings/power/supply/battery.txt line 15

Quoted 5 lines

Documentation/devicetree/bindings/power/supply/battery.txt new file +5 -0

+Because drivers surface properties in sysfs using names derived
+from enum power_supply_property, e.g.
+/sys/class/power_supply/<device>/charge_full_design, our
+battery properties must be named for the corresponding elements in
+enum power_supply_property, defined in include/linux/power_supply.h.

This is Linux/implementation specific and does not belong into a DT binding document.

Documentation/devicetree/bindings/power/supply/battery.txt line 19

Quoted 3 lines

Documentation/devicetree/bindings/power/supply/battery.txt new file +3 -0

+Batteries must be referenced by chargers and/or fuel-gauges
+using a phandle. The phandle's property should be named
+"monitored-battery".

This looks fine.

Documentation/devicetree/bindings/power/supply/battery.txt line 24

Quoted 4 lines

Documentation/devicetree/bindings/power/supply/battery.txt new file +4 -0

+Driver code should call power_supply_get_battery_info() to obtain
+battery properties via monitored-battery. For details see:
+  drivers/power/supply/power_supply_core.c
+  drivers/power/supply/bq27xxx_battery.c

This is also Linux/implementation specific and should be dropped.

Quoted 15 lines

Documentation/devicetree/bindings/power/supply/battery.txt new file +15 -0

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

Quoted 8 lines

Documentation/devicetree/bindings/power/supply/battery.txt new file +5 -1

+     fuel_gauge: fuel-gauge@22 {
+             ....
+             monitored-battery = <&bat>;
+             ...
+     };
--

2.9.3

Hey Sebastian, welcome back :-)

I've taken over work on this patchset from Matt.

Quoted 31 lines

Hi Liam & Matt,

Sorry for my absence in the discussion. I skipped the previous iterations for now and start with this version.

Quoted 21 lines · Liam Breck

From: Liam Breck <[email protected]>

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 <[email protected]>

Cc [email protected]

Signed-off-by Matt Ranostay <[email protected]>

Signed-off-by Liam Breck <[email protected]>

---
 .../devicetree/bindings/power/supply/battery.txt   | 45 ++++++++++++++++++++++
 1 file changed, 45 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/power/supply/battery.txt

Documentation/devicetree/bindings/power/supply/battery.txt new file +0 -0

new file mode 100644
index 0000000..0278617

Documentation/devicetree/bindings/power/supply/battery.txt line 1

Quoted 1 line

Documentation/devicetree/bindings/power/supply/battery.txt new file +1 -0

@@ -0,0 +1,45 @@
+Battery Characteristics

Maybe add something like

Quoted 4 lines

This device provides static battery information, that is usually available in the EEPROM of a smart battery. It's supposed to be used for batteries, which do not have their own EEPROM (or if its unusable).

OK.

Quoted 2 lines

Documentation/devicetree/bindings/power/supply/battery.txt line 4

Quoted 2 lines

Documentation/devicetree/bindings/power/supply/battery.txt new file +2 -0

+Required Properties:
+ - compatible: Must be "fixed-battery"

Rob questioned the term "fixed". Shall we consider other terms -- simple-battery, plain-battery...?

Quoted 7 lines

Documentation/devicetree/bindings/power/supply/battery.txt line 9

Quoted 4 lines

Documentation/devicetree/bindings/power/supply/battery.txt new file +4 -0

+Optional Properties:
+ - voltage-min-design-microvolt: drained battery voltage
+ - energy-full-design-microwatt-hours: battery design energy
+ - charge-full-design-microamp-hours: battery design capacity

Looks fine to me.

Quoted 8 lines

Documentation/devicetree/bindings/power/supply/battery.txt line 15

Quoted 5 lines

Documentation/devicetree/bindings/power/supply/battery.txt new file +5 -0

+Because drivers surface properties in sysfs using names derived
+from enum power_supply_property, e.g.
+/sys/class/power_supply/<device>/charge_full_design, our
+battery properties must be named for the corresponding elements in
+enum power_supply_property, defined in include/linux/power_supply.h.

This is Linux/implementation specific and does not belong into a DT binding document.

I just wrote on a thread for an earlier version of this patch:

"Sebastian proposed DT:battery specifically to be consumed by power_supply_core. Allowing names in DT:battery and power_supply_property to diverge would cause confusion and wasted time, for no particular benefit. As there is no rationale to reconsider the names of these fields for DT:battery, let's write that into the docs."

Quoted 6 lines

Documentation/devicetree/bindings/power/supply/battery.txt line 19

Quoted 3 lines

Documentation/devicetree/bindings/power/supply/battery.txt new file +3 -0

+Batteries must be referenced by chargers and/or fuel-gauges
+using a phandle. The phandle's property should be named
+"monitored-battery".

This looks fine.

Quoted 6 lines

Documentation/devicetree/bindings/power/supply/battery.txt line 24

Quoted 4 lines

Documentation/devicetree/bindings/power/supply/battery.txt new file +4 -0

+Driver code should call power_supply_get_battery_info() to obtain
+battery properties via monitored-battery. For details see:
+  drivers/power/supply/power_supply_core.c
+  drivers/power/supply/bq27xxx_battery.c

This is also Linux/implementation specific and should be dropped.

We ought to mention how drivers are expected to consume DT:battery.

Quoted 23 lines

Quoted 23 lines

Documentation/devicetree/bindings/power/supply/battery.txt new file +20 -1

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

2.9.3

Hi,

Quoted 3 lines · Liam Breck

Hey Sebastian, welcome back :-)

I've taken over work on this patchset from Matt.

Ok.

Quoted 42 lines · Sebastian Reichel

Quoted 31 lines

Hi Liam & Matt,

Sorry for my absence in the discussion. I skipped the previous iterations for now and start with this version.

Quoted 21 lines · Liam Breck

From: Liam Breck <[email protected]>

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 <[email protected]>

Cc [email protected]

Signed-off-by Matt Ranostay <[email protected]>

Signed-off-by Liam Breck <[email protected]>

---
 .../devicetree/bindings/power/supply/battery.txt   | 45 ++++++++++++++++++++++
 1 file changed, 45 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/power/supply/battery.txt

Documentation/devicetree/bindings/power/supply/battery.txt new file +0 -0

new file mode 100644
index 0000000..0278617

Documentation/devicetree/bindings/power/supply/battery.txt line 1

Quoted 1 line

Documentation/devicetree/bindings/power/supply/battery.txt new file +1 -0

@@ -0,0 +1,45 @@
+Battery Characteristics

Maybe add something like

Quoted 4 lines

This device provides static battery information, that is usually available in the EEPROM of a smart battery. It's supposed to be used for batteries, which do not have their own EEPROM (or if its unusable).

OK.

Quoted 2 lines

Quoted 2 lines

Documentation/devicetree/bindings/power/supply/battery.txt new file +2 -0

+Required Properties:
+ - compatible: Must be "fixed-battery"

Quoted 2 lines

Rob questioned the term "fixed". Shall we consider other terms -- simple-battery, plain-battery...?

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

Quoted 16 lines

Quoted 7 lines

Documentation/devicetree/bindings/power/supply/battery.txt line 9

Quoted 4 lines

Documentation/devicetree/bindings/power/supply/battery.txt new file +4 -0

+Optional Properties:
+ - voltage-min-design-microvolt: drained battery voltage
+ - energy-full-design-microwatt-hours: battery design energy
+ - charge-full-design-microamp-hours: battery design capacity

Looks fine to me.

Quoted 8 lines

Documentation/devicetree/bindings/power/supply/battery.txt line 15

Quoted 5 lines

Documentation/devicetree/bindings/power/supply/battery.txt new file +5 -0

+Because drivers surface properties in sysfs using names derived
+from enum power_supply_property, e.g.
+/sys/class/power_supply/<device>/charge_full_design, our
+battery properties must be named for the corresponding elements in
+enum power_supply_property, defined in include/linux/power_supply.h.

This is Linux/implementation specific and does not belong into a DT binding document.

Quoted 8 lines

I just wrote on a thread for an earlier version of this patch:

"Sebastian proposed DT:battery specifically to be consumed by power_supply_core. Allowing names in DT:battery and power_supply_property to diverge would cause confusion and wasted time, for no particular benefit. As there is no rationale to reconsider the names of these fields for DT:battery, let's write that into the docs."

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

Quoted 6 lines

Quoted 6 lines

Documentation/devicetree/bindings/power/supply/battery.txt line 19

Quoted 3 lines

Documentation/devicetree/bindings/power/supply/battery.txt new file +3 -0

+Batteries must be referenced by chargers and/or fuel-gauges
+using a phandle. The phandle's property should be named
+"monitored-battery".

This looks fine.

Quoted 8 lines

Quoted 6 lines

Documentation/devicetree/bindings/power/supply/battery.txt line 24

Quoted 4 lines

Documentation/devicetree/bindings/power/supply/battery.txt new file +4 -0

+Driver code should call power_supply_get_battery_info() to obtain
+battery properties via monitored-battery. For details see:
+  drivers/power/supply/power_supply_core.c
+  drivers/power/supply/bq27xxx_battery.c

This is also Linux/implementation specific and should be dropped.

We ought to mention how drivers are expected to consume DT:battery.

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

Quoted 15 lines

Quoted 15 lines

Quoted 15 lines

Documentation/devicetree/bindings/power/supply/battery.txt new file +15 -0

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

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Documentation/devicetree/bindings/power/supply/battery.txt new file +5 -1

+     fuel_gauge: fuel-gauge@22 {
+             ....
+             monitored-battery = <&bat>;
+             ...
+     };
--

2.9.3

Quoted 51 lines · Sebastian Reichel

Hi,

Quoted 3 lines · Liam Breck

Hey Sebastian, welcome back :-)

I've taken over work on this patchset from Matt.

Ok.

Quoted 42 lines · Sebastian Reichel

Quoted 31 lines

Hi Liam & Matt,

Sorry for my absence in the discussion. I skipped the previous iterations for now and start with this version.

Quoted 21 lines · Liam Breck

From: Liam Breck <[email protected]>

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 <[email protected]>

Cc [email protected]

Signed-off-by Matt Ranostay <[email protected]>

Signed-off-by Liam Breck <[email protected]>

---
 .../devicetree/bindings/power/supply/battery.txt   | 45 ++++++++++++++++++++++
 1 file changed, 45 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/power/supply/battery.txt

Documentation/devicetree/bindings/power/supply/battery.txt new file +0 -0

new file mode 100644
index 0000000..0278617

Documentation/devicetree/bindings/power/supply/battery.txt line 1

Quoted 1 line

Documentation/devicetree/bindings/power/supply/battery.txt new file +1 -0

@@ -0,0 +1,45 @@
+Battery Characteristics

Maybe add something like

Quoted 4 lines

This device provides static battery information, that is usually available in the EEPROM of a smart battery. It's supposed to be used for batteries, which do not have their own EEPROM (or if its unusable).

OK.

Quoted 2 lines

Quoted 2 lines

Documentation/devicetree/bindings/power/supply/battery.txt new file +2 -0

+Required Properties:
+ - compatible: Must be "fixed-battery"

Quoted 9 lines

Quoted 2 lines

Rob questioned the term "fixed". Shall we consider other terms -- simple-battery, plain-battery...?

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

I rather like "fixed-battery", it matches the regulator and other power source DTs better. To answer Rob's question on what a non-fixed battery would look like we can think of a future battery that can change its physical properties (chemical or physical perhaps) in response to software. Although I've not seen such a battery I don't think it's too far-fetched.

All the properties described here are "fixed", if we say "simple" then a battery with more properties would be a "complex" battery..

Quoted 71 lines

Quoted 16 lines

Quoted 7 lines

Documentation/devicetree/bindings/power/supply/battery.txt line 9

Quoted 4 lines

Documentation/devicetree/bindings/power/supply/battery.txt new file +4 -0

+Optional Properties:
+ - voltage-min-design-microvolt: drained battery voltage
+ - energy-full-design-microwatt-hours: battery design energy
+ - charge-full-design-microamp-hours: battery design capacity

Looks fine to me.

Quoted 8 lines

Documentation/devicetree/bindings/power/supply/battery.txt line 15

Quoted 5 lines

Documentation/devicetree/bindings/power/supply/battery.txt new file +5 -0

+Because drivers surface properties in sysfs using names derived
+from enum power_supply_property, e.g.
+/sys/class/power_supply/<device>/charge_full_design, our
+battery properties must be named for the corresponding elements in
+enum power_supply_property, defined in include/linux/power_supply.h.

This is Linux/implementation specific and does not belong into a DT binding document.

Quoted 8 lines

I just wrote on a thread for an earlier version of this patch:

"Sebastian proposed DT:battery specifically to be consumed by power_supply_core. Allowing names in DT:battery and power_supply_property to diverge would cause confusion and wasted time, for no particular benefit. As there is no rationale to reconsider the names of these fields for DT:battery, let's write that into the docs."

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

Quoted 6 lines

Quoted 6 lines

Documentation/devicetree/bindings/power/supply/battery.txt line 19

Quoted 3 lines

Documentation/devicetree/bindings/power/supply/battery.txt new file +3 -0

+Batteries must be referenced by chargers and/or fuel-gauges
+using a phandle. The phandle's property should be named
+"monitored-battery".

This looks fine.

Quoted 8 lines

Quoted 6 lines

Documentation/devicetree/bindings/power/supply/battery.txt line 24

Quoted 4 lines

Documentation/devicetree/bindings/power/supply/battery.txt new file +4 -0

+Driver code should call power_supply_get_battery_info() to obtain
+battery properties via monitored-battery. For details see:
+  drivers/power/supply/power_supply_core.c
+  drivers/power/supply/bq27xxx_battery.c

This is also Linux/implementation specific and should be dropped.

We ought to mention how drivers are expected to consume DT:battery.

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

Quoted 23 lines

Quoted 23 lines

Quoted 23 lines

Documentation/devicetree/bindings/power/supply/battery.txt new file +20 -1

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

2.9.3

Quoted 62 lines · Andrew F. Davis

Quoted 51 lines · Sebastian Reichel

Hi,

Quoted 3 lines · Liam Breck

Hey Sebastian, welcome back :-)

I've taken over work on this patchset from Matt.

Ok.

Quoted 42 lines · Sebastian Reichel

Quoted 31 lines

Hi Liam & Matt,

Sorry for my absence in the discussion. I skipped the previous iterations for now and start with this version.

Quoted 21 lines · Liam Breck

From: Liam Breck <[email protected]>

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 <[email protected]>

Cc [email protected]

Signed-off-by Matt Ranostay <[email protected]>

Signed-off-by Liam Breck <[email protected]>

---
 .../devicetree/bindings/power/supply/battery.txt   | 45 ++++++++++++++++++++++
 1 file changed, 45 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/power/supply/battery.txt

Documentation/devicetree/bindings/power/supply/battery.txt new file +0 -0

new file mode 100644
index 0000000..0278617

Documentation/devicetree/bindings/power/supply/battery.txt line 1

Quoted 1 line

Documentation/devicetree/bindings/power/supply/battery.txt new file +1 -0

@@ -0,0 +1,45 @@
+Battery Characteristics

Maybe add something like

Quoted 4 lines

This device provides static battery information, that is usually available in the EEPROM of a smart battery. It's supposed to be used for batteries, which do not have their own EEPROM (or if its unusable).

OK.

Quoted 2 lines

Quoted 2 lines

Documentation/devicetree/bindings/power/supply/battery.txt new file +2 -0

+Required Properties:
+ - compatible: Must be "fixed-battery"

Quoted 9 lines

Quoted 2 lines

Rob questioned the term "fixed". Shall we consider other terms -- simple-battery, plain-battery...?

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

Quoted 6 lines

I rather like "fixed-battery", it matches the regulator and other power source DTs better. To answer Rob's question on what a non-fixed battery would look like we can think of a future battery that can change its physical properties (chemical or physical perhaps) in response to software. Although I've not seen such a battery I don't think it's too far-fetched.

A "fixed" battery sounds like a non-removable one to me. I'll switch to "simple-battery" in v11 unless a better idea bubbles up first.

Quoted 2 lines

All the properties described here are "fixed", if we say "simple" then a battery with more properties would be a "complex" battery..

Simple is not a boolean term; it really doesn't mandate a "complex-battery" type.

Quoted 63 lines

Quoted 63 lines

Quoted 16 lines

Quoted 7 lines

Documentation/devicetree/bindings/power/supply/battery.txt line 9

Quoted 4 lines

Documentation/devicetree/bindings/power/supply/battery.txt new file +4 -0

+Optional Properties:
+ - voltage-min-design-microvolt: drained battery voltage
+ - energy-full-design-microwatt-hours: battery design energy
+ - charge-full-design-microamp-hours: battery design capacity

Looks fine to me.

Quoted 8 lines

Documentation/devicetree/bindings/power/supply/battery.txt line 15

Quoted 5 lines

Documentation/devicetree/bindings/power/supply/battery.txt new file +5 -0

+Because drivers surface properties in sysfs using names derived
+from enum power_supply_property, e.g.
+/sys/class/power_supply/<device>/charge_full_design, our
+battery properties must be named for the corresponding elements in
+enum power_supply_property, defined in include/linux/power_supply.h.

This is Linux/implementation specific and does not belong into a DT binding document.

Quoted 8 lines

I just wrote on a thread for an earlier version of this patch:

"Sebastian proposed DT:battery specifically to be consumed by power_supply_core. Allowing names in DT:battery and power_supply_property to diverge would cause confusion and wasted time, for no particular benefit. As there is no rationale to reconsider the names of these fields for DT:battery, let's write that into the docs."

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

Quoted 6 lines

Quoted 6 lines

Documentation/devicetree/bindings/power/supply/battery.txt line 19

Quoted 3 lines

Documentation/devicetree/bindings/power/supply/battery.txt new file +3 -0

+Batteries must be referenced by chargers and/or fuel-gauges
+using a phandle. The phandle's property should be named
+"monitored-battery".

This looks fine.

Quoted 8 lines

Quoted 6 lines

Documentation/devicetree/bindings/power/supply/battery.txt line 24

Quoted 4 lines

Documentation/devicetree/bindings/power/supply/battery.txt new file +4 -0

+Driver code should call power_supply_get_battery_info() to obtain
+battery properties via monitored-battery. For details see:
+  drivers/power/supply/power_supply_core.c
+  drivers/power/supply/bq27xxx_battery.c

This is also Linux/implementation specific and should be dropped.

We ought to mention how drivers are expected to consume DT:battery.

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

Quoted 15 lines

Quoted 15 lines

Quoted 15 lines

Documentation/devicetree/bindings/power/supply/battery.txt new file +15 -0

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

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Documentation/devicetree/bindings/power/supply/battery.txt new file +5 -1

+     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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 70 lines · Liam Breck

Quoted 62 lines · Andrew F. Davis

Quoted 51 lines · Sebastian Reichel

Hi,

Quoted 3 lines · Liam Breck

Hey Sebastian, welcome back :-)

I've taken over work on this patchset from Matt.

Ok.

Quoted 42 lines · Sebastian Reichel

Quoted 31 lines

Hi Liam & Matt,

Sorry for my absence in the discussion. I skipped the previous iterations for now and start with this version.

Quoted 21 lines · Liam Breck

From: Liam Breck <[email protected]>

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 <[email protected]>

Cc [email protected]

Signed-off-by Matt Ranostay <[email protected]>

Signed-off-by Liam Breck <[email protected]>

---
 .../devicetree/bindings/power/supply/battery.txt   | 45 ++++++++++++++++++++++
 1 file changed, 45 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/power/supply/battery.txt

Documentation/devicetree/bindings/power/supply/battery.txt new file +0 -0

new file mode 100644
index 0000000..0278617

Documentation/devicetree/bindings/power/supply/battery.txt line 1

Quoted 1 line

Documentation/devicetree/bindings/power/supply/battery.txt new file +1 -0

@@ -0,0 +1,45 @@
+Battery Characteristics

Maybe add something like

Quoted 4 lines

This device provides static battery information, that is usually available in the EEPROM of a smart battery. It's supposed to be used for batteries, which do not have their own EEPROM (or if its unusable).

OK.

Quoted 2 lines

Quoted 2 lines

Documentation/devicetree/bindings/power/supply/battery.txt new file +2 -0

+Required Properties:
+ - compatible: Must be "fixed-battery"

Quoted 9 lines

Quoted 2 lines

Rob questioned the term "fixed". Shall we consider other terms -- simple-battery, plain-battery...?

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

Quoted 6 lines

I rather like "fixed-battery", it matches the regulator and other power source DTs better. To answer Rob's question on what a non-fixed battery would look like we can think of a future battery that can change its physical properties (chemical or physical perhaps) in response to software. Although I've not seen such a battery I don't think it's too far-fetched.

Quoted 3 lines

A "fixed" battery sounds like a non-removable one to me. I'll switch to "simple-battery" in v11 unless a better idea bubbles up first.

Are fixed-regulators non-removable, or do most people understand. Also this is for non-removable batteries, if a battery is changeable then this DT is not valid as it would be describing the current configuration of the device. I guess it could be done with overlays for the current battery, not sure, but something to think about.

Quoted 6 lines

Quoted 2 lines

All the properties described here are "fixed", if we say "simple" then a battery with more properties would be a "complex" battery..

Simple is not a boolean term; it really doesn't mandate a "complex-battery" type.

Yes, but the spectrum would imply levels of simplicity, what would *you* call a "non-simple" battery?

Quoted 76 lines

Quoted 63 lines

Quoted 63 lines

Quoted 16 lines

Quoted 7 lines

Documentation/devicetree/bindings/power/supply/battery.txt line 7

Quoted 4 lines

Documentation/devicetree/bindings/power/supply/battery.txt new file +4 -0

+Optional Properties:
+ - voltage-min-design-microvolt: drained battery voltage
+ - energy-full-design-microwatt-hours: battery design energy
+ - charge-full-design-microamp-hours: battery design capacity

Looks fine to me.

Quoted 8 lines

Documentation/devicetree/bindings/power/supply/battery.txt line 12

Quoted 5 lines

Documentation/devicetree/bindings/power/supply/battery.txt new file +5 -0

+Because drivers surface properties in sysfs using names derived
+from enum power_supply_property, e.g.
+/sys/class/power_supply/<device>/charge_full_design, our
+battery properties must be named for the corresponding elements in
+enum power_supply_property, defined in include/linux/power_supply.h.

This is Linux/implementation specific and does not belong into a DT binding document.

Quoted 8 lines

I just wrote on a thread for an earlier version of this patch:

"Sebastian proposed DT:battery specifically to be consumed by power_supply_core. Allowing names in DT:battery and power_supply_property to diverge would cause confusion and wasted time, for no particular benefit. As there is no rationale to reconsider the names of these fields for DT:battery, let's write that into the docs."

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

Quoted 6 lines

Quoted 6 lines

Documentation/devicetree/bindings/power/supply/battery.txt line 15

Quoted 3 lines

Documentation/devicetree/bindings/power/supply/battery.txt new file +3 -0

+Batteries must be referenced by chargers and/or fuel-gauges
+using a phandle. The phandle's property should be named
+"monitored-battery".

This looks fine.

Quoted 8 lines

Quoted 6 lines

Documentation/devicetree/bindings/power/supply/battery.txt line 19

Quoted 4 lines

Documentation/devicetree/bindings/power/supply/battery.txt new file +4 -0

+Driver code should call power_supply_get_battery_info() to obtain
+battery properties via monitored-battery. For details see:
+  drivers/power/supply/power_supply_core.c
+  drivers/power/supply/bq27xxx_battery.c

This is also Linux/implementation specific and should be dropped.

We ought to mention how drivers are expected to consume DT:battery.

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

Quoted 15 lines

Quoted 15 lines

Quoted 15 lines

Documentation/devicetree/bindings/power/supply/battery.txt new file +15 -0

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

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Documentation/devicetree/bindings/power/supply/battery.txt new file +5 -1

+     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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 75 lines · Andrew F. Davis

Quoted 70 lines · Liam Breck

Quoted 62 lines · Andrew F. Davis

Quoted 51 lines · Sebastian Reichel

Hi,

Quoted 3 lines · Liam Breck

Hey Sebastian, welcome back :-)

I've taken over work on this patchset from Matt.

Ok.

Quoted 42 lines · Sebastian Reichel

Quoted 31 lines

Hi Liam & Matt,

Sorry for my absence in the discussion. I skipped the previous iterations for now and start with this version.

Quoted 21 lines · Liam Breck

From: Liam Breck <[email protected]>

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 <[email protected]>

Cc [email protected]

Signed-off-by Matt Ranostay <[email protected]>

Signed-off-by Liam Breck <[email protected]>

---
 .../devicetree/bindings/power/supply/battery.txt   | 45 ++++++++++++++++++++++
 1 file changed, 45 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/power/supply/battery.txt

Documentation/devicetree/bindings/power/supply/battery.txt new file +0 -0

new file mode 100644
index 0000000..0278617

Documentation/devicetree/bindings/power/supply/battery.txt line 1

Quoted 1 line

Documentation/devicetree/bindings/power/supply/battery.txt new file +1 -0

@@ -0,0 +1,45 @@
+Battery Characteristics

Maybe add something like

Quoted 4 lines

This device provides static battery information, that is usually available in the EEPROM of a smart battery. It's supposed to be used for batteries, which do not have their own EEPROM (or if its unusable).

OK.

Quoted 2 lines

Quoted 2 lines

Documentation/devicetree/bindings/power/supply/battery.txt new file +2 -0

+Required Properties:
+ - compatible: Must be "fixed-battery"

Quoted 9 lines

Quoted 2 lines

Rob questioned the term "fixed". Shall we consider other terms -- simple-battery, plain-battery...?

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

Quoted 6 lines

I rather like "fixed-battery", it matches the regulator and other power source DTs better. To answer Rob's question on what a non-fixed battery would look like we can think of a future battery that can change its physical properties (chemical or physical perhaps) in response to software. Although I've not seen such a battery I don't think it's too far-fetched.

Quoted 3 lines

A "fixed" battery sounds like a non-removable one to me. I'll switch to "simple-battery" in v11 unless a better idea bubbles up first.

Quoted 5 lines

Are fixed-regulators non-removable, or do most people understand. Also this is for non-removable batteries, if a battery is changeable then this DT is not valid as it would be describing the current configuration of the device. I guess it could be done with overlays for the current battery, not sure, but something to think about.

I know little about regulators, but wikipedia mentions fixed and variable/adjustable kinds. We're not contemplating an "adjustable battery" type, so the fixed/adjustable dichotomy isn't a natural fit here.

Quoted 7 lines

Quoted 6 lines

Quoted 2 lines

All the properties described here are "fixed", if we say "simple" then a battery with more properties would be a "complex" battery..

Simple is not a boolean term; it really doesn't mandate a "complex-battery" type.

Quoted 2 lines

Yes, but the spectrum would imply levels of simplicity, what would *you* call a "non-simple" battery?

Simple is not an integral term, either :-)

simple-battery, gauged-battery, coin-battery, bare-battery (no protection circuit)...

Quoted 68 lines

Quoted 68 lines

Quoted 68 lines

Quoted 68 lines

Quoted 16 lines

Quoted 7 lines

Documentation/devicetree/bindings/power/supply/battery.txt line 7

Quoted 4 lines

Documentation/devicetree/bindings/power/supply/battery.txt new file +4 -0

+Optional Properties:
+ - voltage-min-design-microvolt: drained battery voltage
+ - energy-full-design-microwatt-hours: battery design energy
+ - charge-full-design-microamp-hours: battery design capacity

Looks fine to me.

Quoted 8 lines

Documentation/devicetree/bindings/power/supply/battery.txt line 12

Quoted 5 lines

Documentation/devicetree/bindings/power/supply/battery.txt new file +5 -0

+Because drivers surface properties in sysfs using names derived
+from enum power_supply_property, e.g.
+/sys/class/power_supply/<device>/charge_full_design, our
+battery properties must be named for the corresponding elements in
+enum power_supply_property, defined in include/linux/power_supply.h.

This is Linux/implementation specific and does not belong into a DT binding document.

Quoted 8 lines

I just wrote on a thread for an earlier version of this patch:

"Sebastian proposed DT:battery specifically to be consumed by power_supply_core. Allowing names in DT:battery and power_supply_property to diverge would cause confusion and wasted time, for no particular benefit. As there is no rationale to reconsider the names of these fields for DT:battery, let's write that into the docs."

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

Quoted 6 lines

Quoted 6 lines

Documentation/devicetree/bindings/power/supply/battery.txt line 15

Quoted 3 lines

Documentation/devicetree/bindings/power/supply/battery.txt new file +3 -0

+Batteries must be referenced by chargers and/or fuel-gauges
+using a phandle. The phandle's property should be named
+"monitored-battery".

This looks fine.

Quoted 8 lines

Quoted 6 lines

Documentation/devicetree/bindings/power/supply/battery.txt line 19

Quoted 4 lines

Documentation/devicetree/bindings/power/supply/battery.txt new file +4 -0

+Driver code should call power_supply_get_battery_info() to obtain
+battery properties via monitored-battery. For details see:
+  drivers/power/supply/power_supply_core.c
+  drivers/power/supply/bq27xxx_battery.c

This is also Linux/implementation specific and should be dropped.

We ought to mention how drivers are expected to consume DT:battery.

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

Quoted 20 lines

Quoted 20 lines

Quoted 20 lines

Documentation/devicetree/bindings/power/supply/battery.txt new file +20 -0

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

Quoted 8 lines

Quoted 8 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

--
2.9.3

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 82 lines · Liam Breck

Quoted 75 lines · Andrew F. Davis

Quoted 70 lines · Liam Breck

Quoted 62 lines · Andrew F. Davis

Quoted 51 lines · Sebastian Reichel

Hi,

Quoted 3 lines · Liam Breck

Hey Sebastian, welcome back :-)

I've taken over work on this patchset from Matt.

Ok.

Quoted 42 lines · Sebastian Reichel

Quoted 31 lines

Hi Liam & Matt,

Sorry for my absence in the discussion. I skipped the previous iterations for now and start with this version.

Quoted 21 lines · Liam Breck

From: Liam Breck <[email protected]>

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 <[email protected]>

Cc [email protected]

Signed-off-by Matt Ranostay <[email protected]>

Signed-off-by Liam Breck <[email protected]>

---
 .../devicetree/bindings/power/supply/battery.txt   | 45 ++++++++++++++++++++++
 1 file changed, 45 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/power/supply/battery.txt

Documentation/devicetree/bindings/power/supply/battery.txt new file +0 -0

new file mode 100644
index 0000000..0278617

Documentation/devicetree/bindings/power/supply/battery.txt line 1

Quoted 1 line

Documentation/devicetree/bindings/power/supply/battery.txt new file +1 -0

@@ -0,0 +1,45 @@
+Battery Characteristics

Maybe add something like

Quoted 4 lines

This device provides static battery information, that is usually available in the EEPROM of a smart battery. It's supposed to be used for batteries, which do not have their own EEPROM (or if its unusable).

OK.

Quoted 2 lines

Quoted 2 lines

Documentation/devicetree/bindings/power/supply/battery.txt new file +2 -0

+Required Properties:
+ - compatible: Must be "fixed-battery"

Quoted 9 lines

Quoted 2 lines

Rob questioned the term "fixed". Shall we consider other terms -- simple-battery, plain-battery...?

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

Quoted 6 lines

I rather like "fixed-battery", it matches the regulator and other power source DTs better. To answer Rob's question on what a non-fixed battery would look like we can think of a future battery that can change its physical properties (chemical or physical perhaps) in response to software. Although I've not seen such a battery I don't think it's too far-fetched.

Quoted 3 lines

A "fixed" battery sounds like a non-removable one to me. I'll switch to "simple-battery" in v11 unless a better idea bubbles up first.

Quoted 5 lines

Are fixed-regulators non-removable, or do most people understand. Also this is for non-removable batteries, if a battery is changeable then this DT is not valid as it would be describing the current configuration of the device. I guess it could be done with overlays for the current battery, not sure, but something to think about.

Quoted 5 lines

I know little about regulators, but wikipedia mentions fixed and variable/adjustable kinds. We're not contemplating an "adjustable battery" type, so the fixed/adjustable dichotomy isn't a natural fit here.

I think we are though, you are saying this is a "simple" battery because it has fixed design parameters. Any battery that does not have fixed design parameters is some kinda futuristic adjustable battery, what else would you call a battery with non-fixed design parameters?

Quoted 7 lines

Quoted 7 lines

Quoted 6 lines

Quoted 2 lines

All the properties described here are "fixed", if we say "simple" then a battery with more properties would be a "complex" battery..

Simple is not a boolean term; it really doesn't mandate a "complex-battery" type.

Quoted 8 lines

Quoted 2 lines

Yes, but the spectrum would imply levels of simplicity, what would *you* call a "non-simple" battery?

Simple is not an integral term, either :-)

simple-battery, gauged-battery, coin-battery, bare-battery (no protection circuit)...

Fixed/simple/coin/bare batteries are all the same to DT and so do not need new node/compatible names. "Gauged-battery" would be described by a node representing the fuel-gauge and that node would then point to the battery that it gauges, no still no need for a new term.

In our world we still have only three things:
Charger -

  |      |-> Battery
  \/     |

Gauge - -

Sometimes we will have a battery, sometimes we will have a charger or a gauge with an attached battery, and sometimes we will have charger with embedded gauging functions. All these relations can be described in DT without adding extra compatibles.

Quoted 76 lines

Quoted 76 lines

Quoted 76 lines

Quoted 63 lines

Quoted 63 lines

Quoted 16 lines

Quoted 7 lines

Documentation/devicetree/bindings/power/supply/battery.txt line 7

Quoted 4 lines

Documentation/devicetree/bindings/power/supply/battery.txt new file +4 -0

+Optional Properties:
+ - voltage-min-design-microvolt: drained battery voltage
+ - energy-full-design-microwatt-hours: battery design energy
+ - charge-full-design-microamp-hours: battery design capacity

Looks fine to me.

Quoted 8 lines

Documentation/devicetree/bindings/power/supply/battery.txt line 12

Quoted 5 lines

Documentation/devicetree/bindings/power/supply/battery.txt new file +5 -0

+Because drivers surface properties in sysfs using names derived
+from enum power_supply_property, e.g.
+/sys/class/power_supply/<device>/charge_full_design, our
+battery properties must be named for the corresponding elements in
+enum power_supply_property, defined in include/linux/power_supply.h.

This is Linux/implementation specific and does not belong into a DT binding document.

Quoted 8 lines

I just wrote on a thread for an earlier version of this patch:

"Sebastian proposed DT:battery specifically to be consumed by power_supply_core. Allowing names in DT:battery and power_supply_property to diverge would cause confusion and wasted time, for no particular benefit. As there is no rationale to reconsider the names of these fields for DT:battery, let's write that into the docs."

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

Quoted 6 lines

Quoted 6 lines

Documentation/devicetree/bindings/power/supply/battery.txt line 15

Quoted 3 lines

Documentation/devicetree/bindings/power/supply/battery.txt new file +3 -0

+Batteries must be referenced by chargers and/or fuel-gauges
+using a phandle. The phandle's property should be named
+"monitored-battery".

This looks fine.

Quoted 8 lines

Quoted 6 lines

Documentation/devicetree/bindings/power/supply/battery.txt line 19

Quoted 4 lines

Documentation/devicetree/bindings/power/supply/battery.txt new file +4 -0

+Driver code should call power_supply_get_battery_info() to obtain
+battery properties via monitored-battery. For details see:
+  drivers/power/supply/power_supply_core.c
+  drivers/power/supply/bq27xxx_battery.c

This is also Linux/implementation specific and should be dropped.

We ought to mention how drivers are expected to consume DT:battery.

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

Quoted 15 lines

Quoted 15 lines

Quoted 15 lines

Documentation/devicetree/bindings/power/supply/battery.txt new file +15 -0

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

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Documentation/devicetree/bindings/power/supply/battery.txt new file +5 -1

+     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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

From: Liam Breck <[email protected]>

Document monitored-battery = <&battery_node>

Cc Rob Herring <[email protected]>

Cc [email protected]

Signed-off-by Matt Ranostay <[email protected]>

Signed-off-by Liam Breck <[email protected]>

Acked-by Rob Herring <[email protected]>

---
 Documentation/devicetree/bindings/power/supply/bq27xxx.txt | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

Documentation/devicetree/bindings/power/supply/bq27xxx.txt +10 -1

index b0c95ef..cf83371 100644
@@ -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>;
 };

Signature · 1 line

2.9.3

From: Liam Breck <[email protected]>

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 <[email protected]>

Signed-off-by Liam Breck <[email protected]>

---
 drivers/power/supply/power_supply_core.c | 40 ++++++++++++++++++++++++++++++++
 include/linux/power_supply.h             | 18 ++++++++++++++
 2 files changed, 58 insertions(+)

drivers/power/supply/power_supply_core.c +40 -0

index a74d8ca..c121931 100644
@@ -17,6 +17,7 @@
 #include <linux/device.h>
 #include <linux/notifier.h>
 #include <linux/err.h>
+#include <linux/of.h>
 #include <linux/power_supply.h>
 #include <linux/thermal.h>
 #include "power_supply.h"

@@ -487,6 +488,45 @@ struct power_supply *devm_power_supply_get_by_phandle(struct device *dev,
 EXPORT_SYMBOL_GPL(devm_power_supply_get_by_phandle);
 #endif /* CONFIG_OF */
 
+int power_supply_get_battery_info(struct power_supply *psy,
+				  struct power_supply_battery_info *info)
+{
+	struct device_node *battery_np;
+	const char *value;
+	int err;
+
+	info->energy_full_design_uwh = -EINVAL;
+	info->charge_full_design_uah = -EINVAL;
+	info->voltage_min_design_uv  = -EINVAL;
+
+	if (!psy->of_node) {
+		dev_warn(&psy->dev, "%s currently only supports devicetree\n",
+			 __func__);
+		return -ENXIO;
+	}
+
+	battery_np = of_parse_phandle(psy->of_node, "monitored-battery", 0);
+	if (!battery_np)
+		return -ENODEV;
+
+	err = of_property_read_string(battery_np, "compatible", &value);
+	if (err)
+		return err;
+
+	if (strcmp("fixed-battery", value))
+		return -ENODEV;
+
+	of_property_read_u32(battery_np, "energy-full-design-microwatt-hours",
+			     &info->energy_full_design_uwh);
+	of_property_read_u32(battery_np, "charge-full-design-microamp-hours",
+			     &info->charge_full_design_uah);
+	of_property_read_u32(battery_np, "voltage-min-design-microvolt",
+			     &info->voltage_min_design_uv);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(power_supply_get_battery_info);
+
 int power_supply_get_property(struct power_supply *psy,
 			    enum power_supply_property psp,
 			    union power_supply_propval *val)

include/linux/power_supply.h +18 -0

index 3965503..e84f1d3 100644
@@ -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);

Signature · 1 line

2.9.3

From: Liam Breck <[email protected]>

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 <[email protected]>

---

 drivers/power/supply/bq27xxx_battery.c     | 19 +++++++++++++++++++
 drivers/power/supply/bq27xxx_battery_i2c.c | 16 ++++++++--------
 include/linux/power/bq27xxx_battery.h      | 25 ++++++++++++++++++-------
 3 files changed, 45 insertions(+), 15 deletions(-)

drivers/power/supply/bq27xxx_battery.c +0 -0

index 7272d1e..d613d3d 100644
@@ -1020,6 +1020,25 @@ int bq27xxx_battery_setup(struct bq27xxx_device_info *di)
 	struct power_supply_desc *psy_desc;
 	struct power_supply_config psy_cfg = { .drv_data = di, };

drivers/power/supply/bq27xxx_battery.c +2 -0

+	}
+
 	INIT_DELAYED_WORK(&di->work, bq27xxx_battery_poll);
 	mutex_init(&di->lock);
 	di->regs = bq27xxx_regs[di->chip];

drivers/power/supply/bq27xxx_battery_i2c.c +8 -8

index 5c5c3a6..13def59 100644
@@ -150,18 +150,18 @@ static const struct i2c_device_id bq27xxx_i2c_id_table[] = {
 	{ "bq27210", BQ27010 },
 	{ "bq27500", BQ27500 },
 	{ "bq27510", BQ27510 },
-	{ "bq27520", BQ27510 },
+	{ "bq27520", BQ27520 },
 	{ "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);

include/linux/power/bq27xxx_battery.h +8 -7

index 92df553..90db1cf 100644
@@ -2,14 +2,25 @@
 #define __LINUX_BQ27X00_BATTERY_H__
 
 enum bq27xxx_chip {
+	/* categories; index for bq27xxx_regs[] */
 	BQ27000 = 1, /* bq27000, bq27200 */
-	BQ27010, /* bq27010, bq27210 */
-	BQ27500, /* bq27500 */
-	BQ27510, /* bq27510, bq27520 */
-	BQ27530, /* bq27530, bq27531 */
-	BQ27541, /* bq27541, bq27542, bq27546, bq27742 */
-	BQ27545, /* bq27545 */
-	BQ27421, /* bq27421, bq27425, bq27441, bq27621 */
+	BQ27010 = 2, /* bq27010, bq27210 */
+	BQ27500 = 3, /* bq27500 */
+	BQ27510 = 4, /* bq27510, bq27520 */
+	BQ27530 = 5, /* bq27530, bq27531 */
+	BQ27541 = 6, /* bq27541, bq27542, bq27546, bq27742 */
+	BQ27545 = 7, /* bq27545 */
+	BQ27421 = 8, /* bq27421, bq27425, bq27441, bq27621 */

include/linux/power/bq27xxx_battery.h +0 -0

 };
 
 /**

Quoted 8 lines · Liam Breck

From: Liam Breck <[email protected]>

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 <[email protected]>

---

What is this patch based on, it doesn't apply to v4.11-rc1 for me.

Quoted 13 lines

 drivers/power/supply/bq27xxx_battery.c     | 19 +++++++++++++++++++
 drivers/power/supply/bq27xxx_battery_i2c.c | 16 ++++++++--------
 include/linux/power/bq27xxx_battery.h      | 25 ++++++++++++++++++-------
 3 files changed, 45 insertions(+), 15 deletions(-)

drivers/power/supply/bq27xxx_battery.c +0 -0

index 7272d1e..d613d3d 100644
@@ -1020,6 +1020,25 @@ int bq27xxx_battery_setup(struct bq27xxx_device_info *di)
      struct power_supply_desc *psy_desc;
      struct power_supply_config psy_cfg = { .drv_data = di, };
 

drivers/power/supply/bq27xxx_battery.c int bq27xxx_battery_setup(struct bq27xxx_device_info *di) · line 1031

Quoted 9 lines

drivers/power/supply/bq27xxx_battery.c +9 -0

+     switch(di->chip) {
+     case BQ27000:
+     case BQ27010:
+     case BQ27500:
+     case BQ27510:
+     case BQ27530:
+     case BQ27541:
+     case BQ27545:
+     case BQ27421: break;

Why even have these cases then?

drivers/power/supply/bq27xxx_battery.c int bq27xxx_battery_setup(struct bq27xxx_device_info *di) · line 1039

Quoted 8 lines

drivers/power/supply/bq27xxx_battery.c +8 -0

+     case BQ27520: di->chip = BQ27510; 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;

Nope, don't like this at all, make a different variable, ->registers or something to map to the register table. Plus I think changing the variable you are switching on can cause undefined behavior.

Quoted 65 lines

drivers/power/supply/bq27xxx_battery.c +2 -0

+     }
+
      INIT_DELAYED_WORK(&di->work, bq27xxx_battery_poll);
      mutex_init(&di->lock);
      di->regs = bq27xxx_regs[di->chip];

drivers/power/supply/bq27xxx_battery_i2c.c +8 -8

index 5c5c3a6..13def59 100644
@@ -150,18 +150,18 @@ static const struct i2c_device_id bq27xxx_i2c_id_table[] = {
      { "bq27210", BQ27010 },
      { "bq27500", BQ27500 },
      { "bq27510", BQ27510 },
-     { "bq27520", BQ27510 },
+     { "bq27520", BQ27520 },
      { "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);

include/linux/power/bq27xxx_battery.h +13 -7

index 92df553..90db1cf 100644
@@ -2,14 +2,25 @@
 #define __LINUX_BQ27X00_BATTERY_H__
 
 enum bq27xxx_chip {
+     /* categories; index for bq27xxx_regs[] */
      BQ27000 = 1, /* bq27000, bq27200 */
-     BQ27010, /* bq27010, bq27210 */
-     BQ27500, /* bq27500 */
-     BQ27510, /* bq27510, bq27520 */
-     BQ27530, /* bq27530, bq27531 */
-     BQ27541, /* bq27541, bq27542, bq27546, bq27742 */
-     BQ27545, /* bq27545 */
-     BQ27421, /* bq27421, bq27425, bq27441, bq27621 */
+     BQ27010 = 2, /* bq27010, bq27210 */
+     BQ27500 = 3, /* bq27500 */
+     BQ27510 = 4, /* bq27510, bq27520 */
+     BQ27530 = 5, /* bq27530, bq27531 */
+     BQ27541 = 6, /* bq27541, bq27542, bq27546, bq27742 */
+     BQ27545 = 7, /* bq27545 */
+     BQ27421 = 8, /* bq27421, bq27425, bq27441, bq27621 */
+
+     /* members of categories; translate these to category in _setup() */
+     BQ27520 = 101,
+     BQ27531 = 102,
+     BQ27542 = 103,

include/linux/power/bq27xxx_battery.h line 23

Quoted 5 lines

include/linux/power/bq27xxx_battery.h +5 -0

+     BQ27546 = 104,
+     BQ27742 = 105,
+     BQ27425 = 106,
+     BQ27441 = 107,
+     BQ27621 = 108,

Get rid of this, just add new chip enum names if you need support for new chips.

Quoted 10 lines · Andrew F. Davis

Quoted 8 lines · Liam Breck

From: Liam Breck <[email protected]>

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 <[email protected]>

---

Quoted 1 line

What is this patch based on, it doesn't apply to v4.11-rc1 for me.

Sorry, I don't have all of Chris' patches here. It wasn't a factor until we attacked the register arrays. Could you send me his patchset with:
git format-patch --stdout HEAD~x..HEAD~y > bq27xxx-lapa.mbox

Quoted 13 lines

Quoted 13 lines

 drivers/power/supply/bq27xxx_battery.c     | 19 +++++++++++++++++++
 drivers/power/supply/bq27xxx_battery_i2c.c | 16 ++++++++--------
 include/linux/power/bq27xxx_battery.h      | 25 ++++++++++++++++++-------
 3 files changed, 45 insertions(+), 15 deletions(-)

drivers/power/supply/bq27xxx_battery.c +0 -0

index 7272d1e..d613d3d 100644
@@ -1020,6 +1020,25 @@ int bq27xxx_battery_setup(struct bq27xxx_device_info *di)
      struct power_supply_desc *psy_desc;
      struct power_supply_config psy_cfg = { .drv_data = di, };
 

Quoted 11 lines

drivers/power/supply/bq27xxx_battery.c int bq27xxx_battery_setup(struct bq27xxx_device_info *di) · line 1031

Quoted 9 lines

drivers/power/supply/bq27xxx_battery.c +9 -0

+     switch(di->chip) {
+     case BQ27000:
+     case BQ27010:
+     case BQ27500:
+     case BQ27510:
+     case BQ27530:
+     case BQ27541:
+     case BQ27545:
+     case BQ27421: break;

Why even have these cases then?

You'll get a compiler warning if any are missing. Helps when adding new chips.

Quoted 9 lines

drivers/power/supply/bq27xxx_battery.c int bq27xxx_battery_setup(struct bq27xxx_device_info *di) · line 1039

Quoted 8 lines

drivers/power/supply/bq27xxx_battery.c +8 -0

+     case BQ27520: di->chip = BQ27510; 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;

Quoted 3 lines

Nope, don't like this at all, make a different variable, ->registers or something to map to the register table. Plus I think changing the variable you are switching on can cause undefined behavior.

We had a different variable, .dmid, but you rejected a second ID field. I think this is better, as we eliminated the static arrays .dmid indexed. I didn't rename .chip to .category as that would cause trivial changes all over the file. I could do that in a final patch tho.

You can modify a variable after switching on it, I promise.

Quoted 65 lines

Quoted 65 lines

drivers/power/supply/bq27xxx_battery.c +2 -0

+     }
+
      INIT_DELAYED_WORK(&di->work, bq27xxx_battery_poll);
      mutex_init(&di->lock);
      di->regs = bq27xxx_regs[di->chip];

drivers/power/supply/bq27xxx_battery_i2c.c +8 -8

index 5c5c3a6..13def59 100644
@@ -150,18 +150,18 @@ static const struct i2c_device_id bq27xxx_i2c_id_table[] = {
      { "bq27210", BQ27010 },
      { "bq27500", BQ27500 },
      { "bq27510", BQ27510 },
-     { "bq27520", BQ27510 },
+     { "bq27520", BQ27520 },
      { "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);

include/linux/power/bq27xxx_battery.h +13 -7

index 92df553..90db1cf 100644
@@ -2,14 +2,25 @@
 #define __LINUX_BQ27X00_BATTERY_H__
 
 enum bq27xxx_chip {
+     /* categories; index for bq27xxx_regs[] */
      BQ27000 = 1, /* bq27000, bq27200 */
-     BQ27010, /* bq27010, bq27210 */
-     BQ27500, /* bq27500 */
-     BQ27510, /* bq27510, bq27520 */
-     BQ27530, /* bq27530, bq27531 */
-     BQ27541, /* bq27541, bq27542, bq27546, bq27742 */
-     BQ27545, /* bq27545 */
-     BQ27421, /* bq27421, bq27425, bq27441, bq27621 */
+     BQ27010 = 2, /* bq27010, bq27210 */
+     BQ27500 = 3, /* bq27500 */
+     BQ27510 = 4, /* bq27510, bq27520 */
+     BQ27530 = 5, /* bq27530, bq27531 */
+     BQ27541 = 6, /* bq27541, bq27542, bq27546, bq27742 */
+     BQ27545 = 7, /* bq27545 */
+     BQ27421 = 8, /* bq27421, bq27425, bq27441, bq27621 */
+
+     /* members of categories; translate these to category in _setup() */
+     BQ27520 = 101,
+     BQ27531 = 102,
+     BQ27542 = 103,

Quoted 8 lines

include/linux/power/bq27xxx_battery.h line 23

Quoted 5 lines

include/linux/power/bq27xxx_battery.h +5 -0

+     BQ27546 = 104,
+     BQ27742 = 105,
+     BQ27425 = 106,
+     BQ27441 = 107,
+     BQ27621 = 108,

Get rid of this, just add new chip enum names if you need support for new chips.

How does a non-DT config obtain chip ID? We want explicit enum values if they appear in external platform-data objects. I'll mention that in the next patch description.

The category (original) IDs index bq27xxx_regs[], and you'll probably extend that in time. So I placed the member (new) IDs well above the categories to allow permanent explicit values.

Quoted 13 lines · Liam Breck

Quoted 11 lines · Andrew F. Davis

Quoted 8 lines · Liam Breck

From: Liam Breck <[email protected]>

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 <[email protected]>

---

What is this patch based on, it doesn't apply to v4.11-rc1 for me.

Quoted 5 lines

Sorry, I don't have all of Chris' patches here. It wasn't a factor until we attacked the register arrays. Could you send me his patchset with:
git format-patch --stdout HEAD~x..HEAD~y > bq27xxx-lapa.mbox

Who are you asking?

Quoted 122 lines

Quoted 13 lines

Quoted 13 lines

 drivers/power/supply/bq27xxx_battery.c     | 19 +++++++++++++++++++
 drivers/power/supply/bq27xxx_battery_i2c.c | 16 ++++++++--------
 include/linux/power/bq27xxx_battery.h      | 25 ++++++++++++++++++-------
 3 files changed, 45 insertions(+), 15 deletions(-)

drivers/power/supply/bq27xxx_battery.c +0 -0

index 7272d1e..d613d3d 100644
@@ -1020,6 +1020,25 @@ int bq27xxx_battery_setup(struct bq27xxx_device_info *di)
      struct power_supply_desc *psy_desc;
      struct power_supply_config psy_cfg = { .drv_data = di, };
 

Quoted 11 lines

drivers/power/supply/bq27xxx_battery.c int bq27xxx_battery_setup(struct bq27xxx_device_info *di) · line 1031

Quoted 9 lines

drivers/power/supply/bq27xxx_battery.c +9 -0

+     switch(di->chip) {
+     case BQ27000:
+     case BQ27010:
+     case BQ27500:
+     case BQ27510:
+     case BQ27530:
+     case BQ27541:
+     case BQ27545:
+     case BQ27421: break;

Why even have these cases then?

You'll get a compiler warning if any are missing. Helps when adding new chips.

Quoted 9 lines

drivers/power/supply/bq27xxx_battery.c int bq27xxx_battery_setup(struct bq27xxx_device_info *di) · line 1039

Quoted 8 lines

drivers/power/supply/bq27xxx_battery.c +8 -0

+     case BQ27520: di->chip = BQ27510; 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;

Quoted 3 lines

Nope, don't like this at all, make a different variable, ->registers or something to map to the register table. Plus I think changing the variable you are switching on can cause undefined behavior.

We had a different variable, .dmid, but you rejected a second ID field. I think this is better, as we eliminated the static arrays .dmid indexed. I didn't rename .chip to .category as that would cause trivial changes all over the file. I could do that in a final patch tho.

You can modify a variable after switching on it, I promise.

Quoted 73 lines

Quoted 60 lines

drivers/power/supply/bq27xxx_battery.c +2 -0

+     }
+
      INIT_DELAYED_WORK(&di->work, bq27xxx_battery_poll);
      mutex_init(&di->lock);
      di->regs = bq27xxx_regs[di->chip];

drivers/power/supply/bq27xxx_battery_i2c.c +8 -8

index 5c5c3a6..13def59 100644
@@ -150,18 +150,18 @@ static const struct i2c_device_id bq27xxx_i2c_id_table[] = {
      { "bq27210", BQ27010 },
      { "bq27500", BQ27500 },
      { "bq27510", BQ27510 },
-     { "bq27520", BQ27510 },
+     { "bq27520", BQ27520 },
      { "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);

include/linux/power/bq27xxx_battery.h +8 -7

index 92df553..90db1cf 100644
@@ -2,14 +2,25 @@
 #define __LINUX_BQ27X00_BATTERY_H__
 
 enum bq27xxx_chip {
+     /* categories; index for bq27xxx_regs[] */
      BQ27000 = 1, /* bq27000, bq27200 */
-     BQ27010, /* bq27010, bq27210 */
-     BQ27500, /* bq27500 */
-     BQ27510, /* bq27510, bq27520 */
-     BQ27530, /* bq27530, bq27531 */
-     BQ27541, /* bq27541, bq27542, bq27546, bq27742 */
-     BQ27545, /* bq27545 */
-     BQ27421, /* bq27421, bq27425, bq27441, bq27621 */
+     BQ27010 = 2, /* bq27010, bq27210 */
+     BQ27500 = 3, /* bq27500 */
+     BQ27510 = 4, /* bq27510, bq27520 */
+     BQ27530 = 5, /* bq27530, bq27531 */
+     BQ27541 = 6, /* bq27541, bq27542, bq27546, bq27742 */
+     BQ27545 = 7, /* bq27545 */
+     BQ27421 = 8, /* bq27421, bq27425, bq27441, bq27621 */

include/linux/power/bq27xxx_battery.h line 23

Quoted 10 lines

include/linux/power/bq27xxx_battery.h +10 -0

+
+     /* members of categories; translate these to category in _setup() */
+     BQ27520 = 101,
+     BQ27531 = 102,
+     BQ27542 = 103,
+     BQ27546 = 104,
+     BQ27742 = 105,
+     BQ27425 = 106,
+     BQ27441 = 107,
+     BQ27621 = 108,

Get rid of this, just add new chip enum names if you need support for new chips.

Quoted 4 lines

How does a non-DT config obtain chip ID? We want explicit enum values if they appear in external platform-data objects. I'll mention that in the next patch description.

platform-data is going away, I'm not sure what you are saying here.

Quoted 4 lines

The category (original) IDs index bq27xxx_regs[], and you'll probably extend that in time. So I placed the member (new) IDs well above the categories to allow permanent explicit values.

Quoted 20 lines · Andrew F. Davis

Quoted 18 lines · Liam Breck

Quoted 11 lines · Andrew F. Davis

Quoted 8 lines · Liam Breck

From: Liam Breck <[email protected]>

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 <[email protected]>

---

What is this patch based on, it doesn't apply to v4.11-rc1 for me.

Sorry, I don't have all of Chris' patches here. It wasn't a factor until we attacked the register arrays. Could you send me his patchset with:
git format-patch --stdout HEAD~x..HEAD~y > bq27xxx-lapa.mbox

Quoted 1 line

Who are you asking?

You, if I may impose...

Quoted 127 lines

Quoted 126 lines

Quoted 13 lines

Quoted 13 lines

 drivers/power/supply/bq27xxx_battery.c     | 19 +++++++++++++++++++
 drivers/power/supply/bq27xxx_battery_i2c.c | 16 ++++++++--------
 include/linux/power/bq27xxx_battery.h      | 25 ++++++++++++++++++-------
 3 files changed, 45 insertions(+), 15 deletions(-)

drivers/power/supply/bq27xxx_battery.c +0 -0

index 7272d1e..d613d3d 100644
@@ -1020,6 +1020,25 @@ int bq27xxx_battery_setup(struct bq27xxx_device_info *di)
      struct power_supply_desc *psy_desc;
      struct power_supply_config psy_cfg = { .drv_data = di, };
 

Quoted 11 lines

drivers/power/supply/bq27xxx_battery.c int bq27xxx_battery_setup(struct bq27xxx_device_info *di) · line 1031

Quoted 9 lines

drivers/power/supply/bq27xxx_battery.c +9 -0

+     switch(di->chip) {
+     case BQ27000:
+     case BQ27010:
+     case BQ27500:
+     case BQ27510:
+     case BQ27530:
+     case BQ27541:
+     case BQ27545:
+     case BQ27421: break;

Why even have these cases then?

You'll get a compiler warning if any are missing. Helps when adding new chips.

Quoted 9 lines

drivers/power/supply/bq27xxx_battery.c int bq27xxx_battery_setup(struct bq27xxx_device_info *di) · line 1039

Quoted 8 lines

drivers/power/supply/bq27xxx_battery.c +8 -0

+     case BQ27520: di->chip = BQ27510; 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;

Quoted 3 lines

Nope, don't like this at all, make a different variable, ->registers or something to map to the register table. Plus I think changing the variable you are switching on can cause undefined behavior.

We had a different variable, .dmid, but you rejected a second ID field. I think this is better, as we eliminated the static arrays .dmid indexed. I didn't rename .chip to .category as that would cause trivial changes all over the file. I could do that in a final patch tho.

You can modify a variable after switching on it, I promise.

Quoted 65 lines

Quoted 65 lines

drivers/power/supply/bq27xxx_battery.c +2 -0

+     }
+
      INIT_DELAYED_WORK(&di->work, bq27xxx_battery_poll);
      mutex_init(&di->lock);
      di->regs = bq27xxx_regs[di->chip];

drivers/power/supply/bq27xxx_battery_i2c.c +8 -8

index 5c5c3a6..13def59 100644
@@ -150,18 +150,18 @@ static const struct i2c_device_id bq27xxx_i2c_id_table[] = {
      { "bq27210", BQ27010 },
      { "bq27500", BQ27500 },
      { "bq27510", BQ27510 },
-     { "bq27520", BQ27510 },
+     { "bq27520", BQ27520 },
      { "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);

include/linux/power/bq27xxx_battery.h +13 -7

index 92df553..90db1cf 100644
@@ -2,14 +2,25 @@
 #define __LINUX_BQ27X00_BATTERY_H__
 
 enum bq27xxx_chip {
+     /* categories; index for bq27xxx_regs[] */
      BQ27000 = 1, /* bq27000, bq27200 */
-     BQ27010, /* bq27010, bq27210 */
-     BQ27500, /* bq27500 */
-     BQ27510, /* bq27510, bq27520 */
-     BQ27530, /* bq27530, bq27531 */
-     BQ27541, /* bq27541, bq27542, bq27546, bq27742 */
-     BQ27545, /* bq27545 */
-     BQ27421, /* bq27421, bq27425, bq27441, bq27621 */
+     BQ27010 = 2, /* bq27010, bq27210 */
+     BQ27500 = 3, /* bq27500 */
+     BQ27510 = 4, /* bq27510, bq27520 */
+     BQ27530 = 5, /* bq27530, bq27531 */
+     BQ27541 = 6, /* bq27541, bq27542, bq27546, bq27742 */
+     BQ27545 = 7, /* bq27545 */
+     BQ27421 = 8, /* bq27421, bq27425, bq27441, bq27621 */
+
+     /* members of categories; translate these to category in _setup() */
+     BQ27520 = 101,
+     BQ27531 = 102,
+     BQ27542 = 103,

Quoted 8 lines

include/linux/power/bq27xxx_battery.h line 23

Quoted 5 lines

include/linux/power/bq27xxx_battery.h +5 -0

+     BQ27546 = 104,
+     BQ27742 = 105,
+     BQ27425 = 106,
+     BQ27441 = 107,
+     BQ27621 = 108,

Get rid of this, just add new chip enum names if you need support for new chips.

How does a non-DT config obtain chip ID? We want explicit enum values if they appear in external platform-data objects. I'll mention that in the next patch description.

Quoted 1 line

platform-data is going away, I'm not sure what you are saying here.

Don't 1-wire users have a platform-data config? You plan to force them to replace it with DT? I thought that wasn't kosher.

Quoted 4 lines

Quoted 4 lines

The category (original) IDs index bq27xxx_regs[], and you'll probably extend that in time. So I placed the member (new) IDs well above the categories to allow permanent explicit values.

Quoted 21 lines · Liam Breck

Quoted 20 lines · Andrew F. Davis

Quoted 18 lines · Liam Breck

Quoted 11 lines · Andrew F. Davis

Quoted 8 lines · Liam Breck

From: Liam Breck <[email protected]>

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 <[email protected]>

---

What is this patch based on, it doesn't apply to v4.11-rc1 for me.

Sorry, I don't have all of Chris' patches here. It wasn't a factor until we attacked the register arrays. Could you send me his patchset with:
git format-patch --stdout HEAD~x..HEAD~y > bq27xxx-lapa.mbox

Quoted 4 lines

Quoted 1 line

Who are you asking?

You, if I may impose...

Just rebase on v4.11-rc1...

Quoted 127 lines

Quoted 127 lines

Quoted 126 lines

Quoted 13 lines

Quoted 13 lines

 drivers/power/supply/bq27xxx_battery.c     | 19 +++++++++++++++++++
 drivers/power/supply/bq27xxx_battery_i2c.c | 16 ++++++++--------
 include/linux/power/bq27xxx_battery.h      | 25 ++++++++++++++++++-------
 3 files changed, 45 insertions(+), 15 deletions(-)

drivers/power/supply/bq27xxx_battery.c +0 -0

index 7272d1e..d613d3d 100644
@@ -1020,6 +1020,25 @@ int bq27xxx_battery_setup(struct bq27xxx_device_info *di)
      struct power_supply_desc *psy_desc;
      struct power_supply_config psy_cfg = { .drv_data = di, };
 

Quoted 11 lines

drivers/power/supply/bq27xxx_battery.c int bq27xxx_battery_setup(struct bq27xxx_device_info *di) · line 1031

Quoted 9 lines

drivers/power/supply/bq27xxx_battery.c +9 -0

+     switch(di->chip) {
+     case BQ27000:
+     case BQ27010:
+     case BQ27500:
+     case BQ27510:
+     case BQ27530:
+     case BQ27541:
+     case BQ27545:
+     case BQ27421: break;

Why even have these cases then?

You'll get a compiler warning if any are missing. Helps when adding new chips.

Quoted 9 lines

drivers/power/supply/bq27xxx_battery.c int bq27xxx_battery_setup(struct bq27xxx_device_info *di) · line 1039

Quoted 8 lines

drivers/power/supply/bq27xxx_battery.c +8 -0

+     case BQ27520: di->chip = BQ27510; 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;

Quoted 3 lines

Nope, don't like this at all, make a different variable, ->registers or something to map to the register table. Plus I think changing the variable you are switching on can cause undefined behavior.

We had a different variable, .dmid, but you rejected a second ID field. I think this is better, as we eliminated the static arrays .dmid indexed. I didn't rename .chip to .category as that would cause trivial changes all over the file. I could do that in a final patch tho.

You can modify a variable after switching on it, I promise.

Quoted 65 lines

Quoted 65 lines

drivers/power/supply/bq27xxx_battery.c +2 -0

+     }
+
      INIT_DELAYED_WORK(&di->work, bq27xxx_battery_poll);
      mutex_init(&di->lock);
      di->regs = bq27xxx_regs[di->chip];

drivers/power/supply/bq27xxx_battery_i2c.c +8 -8

index 5c5c3a6..13def59 100644
@@ -150,18 +150,18 @@ static const struct i2c_device_id bq27xxx_i2c_id_table[] = {
      { "bq27210", BQ27010 },
      { "bq27500", BQ27500 },
      { "bq27510", BQ27510 },
-     { "bq27520", BQ27510 },
+     { "bq27520", BQ27520 },
      { "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);

include/linux/power/bq27xxx_battery.h +13 -7

index 92df553..90db1cf 100644
@@ -2,14 +2,25 @@
 #define __LINUX_BQ27X00_BATTERY_H__
 
 enum bq27xxx_chip {
+     /* categories; index for bq27xxx_regs[] */
      BQ27000 = 1, /* bq27000, bq27200 */
-     BQ27010, /* bq27010, bq27210 */
-     BQ27500, /* bq27500 */
-     BQ27510, /* bq27510, bq27520 */
-     BQ27530, /* bq27530, bq27531 */
-     BQ27541, /* bq27541, bq27542, bq27546, bq27742 */
-     BQ27545, /* bq27545 */
-     BQ27421, /* bq27421, bq27425, bq27441, bq27621 */
+     BQ27010 = 2, /* bq27010, bq27210 */
+     BQ27500 = 3, /* bq27500 */
+     BQ27510 = 4, /* bq27510, bq27520 */
+     BQ27530 = 5, /* bq27530, bq27531 */
+     BQ27541 = 6, /* bq27541, bq27542, bq27546, bq27742 */
+     BQ27545 = 7, /* bq27545 */
+     BQ27421 = 8, /* bq27421, bq27425, bq27441, bq27621 */
+
+     /* members of categories; translate these to category in _setup() */
+     BQ27520 = 101,
+     BQ27531 = 102,
+     BQ27542 = 103,

Quoted 8 lines

include/linux/power/bq27xxx_battery.h line 23

Quoted 5 lines

include/linux/power/bq27xxx_battery.h +5 -0

+     BQ27546 = 104,
+     BQ27742 = 105,
+     BQ27425 = 106,
+     BQ27441 = 107,
+     BQ27621 = 108,

Get rid of this, just add new chip enum names if you need support for new chips.

How does a non-DT config obtain chip ID? We want explicit enum values if they appear in external platform-data objects. I'll mention that in the next patch description.

Quoted 5 lines

Quoted 1 line

platform-data is going away, I'm not sure what you are saying here.

Don't 1-wire users have a platform-data config? You plan to force them to replace it with DT? I thought that wasn't kosher.

The 1-wire driver generates this platform-data, 1-wire is a discoverable bus, they never needed to define DT or platform data and still will not.

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

The category (original) IDs index bq27xxx_regs[], and you'll probably extend that in time. So I placed the member (new) IDs well above the categories to allow permanent explicit values.

Quoted 158 lines · Andrew F. Davis

Quoted 21 lines · Liam Breck

Quoted 20 lines · Andrew F. Davis

Quoted 18 lines · Liam Breck

Quoted 11 lines · Andrew F. Davis

Quoted 8 lines · Liam Breck

From: Liam Breck <[email protected]>

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 <[email protected]>

---

What is this patch based on, it doesn't apply to v4.11-rc1 for me.

Sorry, I don't have all of Chris' patches here. It wasn't a factor until we attacked the register arrays. Could you send me his patchset with:
git format-patch --stdout HEAD~x..HEAD~y > bq27xxx-lapa.mbox

Quoted 4 lines

Quoted 1 line

Who are you asking?

You, if I may impose...

Just rebase on v4.11-rc1...

Quoted 129 lines

Quoted 128 lines

Quoted 122 lines

Quoted 13 lines

Quoted 13 lines

 drivers/power/supply/bq27xxx_battery.c     | 19 +++++++++++++++++++
 drivers/power/supply/bq27xxx_battery_i2c.c | 16 ++++++++--------
 include/linux/power/bq27xxx_battery.h      | 25 ++++++++++++++++++-------
 3 files changed, 45 insertions(+), 15 deletions(-)

drivers/power/supply/bq27xxx_battery.c +0 -0

index 7272d1e..d613d3d 100644
@@ -1020,6 +1020,25 @@ int bq27xxx_battery_setup(struct bq27xxx_device_info *di)
      struct power_supply_desc *psy_desc;
      struct power_supply_config psy_cfg = { .drv_data = di, };
 

Quoted 11 lines

drivers/power/supply/bq27xxx_battery.c int bq27xxx_battery_setup(struct bq27xxx_device_info *di) · line 1031

Quoted 9 lines

drivers/power/supply/bq27xxx_battery.c +9 -0

+     switch(di->chip) {
+     case BQ27000:
+     case BQ27010:
+     case BQ27500:
+     case BQ27510:
+     case BQ27530:
+     case BQ27541:
+     case BQ27545:
+     case BQ27421: break;

Why even have these cases then?

You'll get a compiler warning if any are missing. Helps when adding new chips.

Quoted 9 lines

drivers/power/supply/bq27xxx_battery.c int bq27xxx_battery_setup(struct bq27xxx_device_info *di) · line 1039

Quoted 8 lines

drivers/power/supply/bq27xxx_battery.c +8 -0

+     case BQ27520: di->chip = BQ27510; 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;

Quoted 3 lines

Nope, don't like this at all, make a different variable, ->registers or something to map to the register table. Plus I think changing the variable you are switching on can cause undefined behavior.

We had a different variable, .dmid, but you rejected a second ID field. I think this is better, as we eliminated the static arrays .dmid indexed. I didn't rename .chip to .category as that would cause trivial changes all over the file. I could do that in a final patch tho.

You can modify a variable after switching on it, I promise.

Quoted 65 lines

Quoted 65 lines

drivers/power/supply/bq27xxx_battery.c +2 -0

+     }
+
      INIT_DELAYED_WORK(&di->work, bq27xxx_battery_poll);
      mutex_init(&di->lock);
      di->regs = bq27xxx_regs[di->chip];

drivers/power/supply/bq27xxx_battery_i2c.c +8 -8

index 5c5c3a6..13def59 100644
@@ -150,18 +150,18 @@ static const struct i2c_device_id bq27xxx_i2c_id_table[] = {
      { "bq27210", BQ27010 },
      { "bq27500", BQ27500 },
      { "bq27510", BQ27510 },
-     { "bq27520", BQ27510 },
+     { "bq27520", BQ27520 },
      { "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);

include/linux/power/bq27xxx_battery.h +13 -7

index 92df553..90db1cf 100644
@@ -2,14 +2,25 @@
 #define __LINUX_BQ27X00_BATTERY_H__
 
 enum bq27xxx_chip {
+     /* categories; index for bq27xxx_regs[] */
      BQ27000 = 1, /* bq27000, bq27200 */
-     BQ27010, /* bq27010, bq27210 */
-     BQ27500, /* bq27500 */
-     BQ27510, /* bq27510, bq27520 */
-     BQ27530, /* bq27530, bq27531 */
-     BQ27541, /* bq27541, bq27542, bq27546, bq27742 */
-     BQ27545, /* bq27545 */
-     BQ27421, /* bq27421, bq27425, bq27441, bq27621 */
+     BQ27010 = 2, /* bq27010, bq27210 */
+     BQ27500 = 3, /* bq27500 */
+     BQ27510 = 4, /* bq27510, bq27520 */
+     BQ27530 = 5, /* bq27530, bq27531 */
+     BQ27541 = 6, /* bq27541, bq27542, bq27546, bq27742 */
+     BQ27545 = 7, /* bq27545 */
+     BQ27421 = 8, /* bq27421, bq27425, bq27441, bq27621 */
+
+     /* members of categories; translate these to category in _setup() */
+     BQ27520 = 101,
+     BQ27531 = 102,
+     BQ27542 = 103,

Quoted 8 lines

include/linux/power/bq27xxx_battery.h line 23

Quoted 5 lines

include/linux/power/bq27xxx_battery.h +5 -0

+     BQ27546 = 104,
+     BQ27742 = 105,
+     BQ27425 = 106,
+     BQ27441 = 107,
+     BQ27621 = 108,

Get rid of this, just add new chip enum names if you need support for new chips.

Quoted 4 lines

How does a non-DT config obtain chip ID? We want explicit enum values if they appear in external platform-data objects. I'll mention that in the next patch description.

platform-data is going away, I'm not sure what you are saying here.

Quoted 6 lines

Quoted 3 lines

Don't 1-wire users have a platform-data config? You plan to force them to replace it with DT? I thought that wasn't kosher.

The 1-wire driver generates this platform-data, 1-wire is a discoverable bus, they never needed to define DT or platform data and still will not.

How does it map a discovered chip type to a bq27xxx_chip enum?

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

The category (original) IDs index bq27xxx_regs[], and you'll probably extend that in time. So I placed the member (new) IDs well above the categories to allow permanent explicit values.

Quoted 163 lines · Liam Breck

Quoted 162 lines · Andrew F. Davis

Quoted 21 lines · Liam Breck

Quoted 20 lines · Andrew F. Davis

Quoted 18 lines · Liam Breck

Quoted 11 lines · Andrew F. Davis

Quoted 8 lines · Liam Breck

From: Liam Breck <[email protected]>

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 <[email protected]>

---

What is this patch based on, it doesn't apply to v4.11-rc1 for me.

Sorry, I don't have all of Chris' patches here. It wasn't a factor until we attacked the register arrays. Could you send me his patchset with:
git format-patch --stdout HEAD~x..HEAD~y > bq27xxx-lapa.mbox

Quoted 4 lines

Quoted 1 line

Who are you asking?

You, if I may impose...

Just rebase on v4.11-rc1...

Quoted 132 lines

Quoted 122 lines

Quoted 122 lines

Quoted 13 lines

Quoted 13 lines

 drivers/power/supply/bq27xxx_battery.c     | 19 +++++++++++++++++++
 drivers/power/supply/bq27xxx_battery_i2c.c | 16 ++++++++--------
 include/linux/power/bq27xxx_battery.h      | 25 ++++++++++++++++++-------
 3 files changed, 45 insertions(+), 15 deletions(-)

drivers/power/supply/bq27xxx_battery.c +0 -0

index 7272d1e..d613d3d 100644
@@ -1020,6 +1020,25 @@ int bq27xxx_battery_setup(struct bq27xxx_device_info *di)
      struct power_supply_desc *psy_desc;
      struct power_supply_config psy_cfg = { .drv_data = di, };
 

Quoted 11 lines

drivers/power/supply/bq27xxx_battery.c int bq27xxx_battery_setup(struct bq27xxx_device_info *di) · line 1031

Quoted 9 lines

drivers/power/supply/bq27xxx_battery.c +9 -0

+     switch(di->chip) {
+     case BQ27000:
+     case BQ27010:
+     case BQ27500:
+     case BQ27510:
+     case BQ27530:
+     case BQ27541:
+     case BQ27545:
+     case BQ27421: break;

Why even have these cases then?

You'll get a compiler warning if any are missing. Helps when adding new chips.

Quoted 9 lines

drivers/power/supply/bq27xxx_battery.c int bq27xxx_battery_setup(struct bq27xxx_device_info *di) · line 1039

Quoted 8 lines

drivers/power/supply/bq27xxx_battery.c +8 -0

+     case BQ27520: di->chip = BQ27510; 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;

Quoted 3 lines

Nope, don't like this at all, make a different variable, ->registers or something to map to the register table. Plus I think changing the variable you are switching on can cause undefined behavior.

We had a different variable, .dmid, but you rejected a second ID field. I think this is better, as we eliminated the static arrays .dmid indexed. I didn't rename .chip to .category as that would cause trivial changes all over the file. I could do that in a final patch tho.

You can modify a variable after switching on it, I promise.

Quoted 73 lines

Quoted 60 lines

drivers/power/supply/bq27xxx_battery.c +2 -0

+     }
+
      INIT_DELAYED_WORK(&di->work, bq27xxx_battery_poll);
      mutex_init(&di->lock);
      di->regs = bq27xxx_regs[di->chip];

drivers/power/supply/bq27xxx_battery_i2c.c +8 -8

index 5c5c3a6..13def59 100644
@@ -150,18 +150,18 @@ static const struct i2c_device_id bq27xxx_i2c_id_table[] = {
      { "bq27210", BQ27010 },
      { "bq27500", BQ27500 },
      { "bq27510", BQ27510 },
-     { "bq27520", BQ27510 },
+     { "bq27520", BQ27520 },
      { "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);

include/linux/power/bq27xxx_battery.h +8 -7

index 92df553..90db1cf 100644
@@ -2,14 +2,25 @@
 #define __LINUX_BQ27X00_BATTERY_H__
 
 enum bq27xxx_chip {
+     /* categories; index for bq27xxx_regs[] */
      BQ27000 = 1, /* bq27000, bq27200 */
-     BQ27010, /* bq27010, bq27210 */
-     BQ27500, /* bq27500 */
-     BQ27510, /* bq27510, bq27520 */
-     BQ27530, /* bq27530, bq27531 */
-     BQ27541, /* bq27541, bq27542, bq27546, bq27742 */
-     BQ27545, /* bq27545 */
-     BQ27421, /* bq27421, bq27425, bq27441, bq27621 */
+     BQ27010 = 2, /* bq27010, bq27210 */
+     BQ27500 = 3, /* bq27500 */
+     BQ27510 = 4, /* bq27510, bq27520 */
+     BQ27530 = 5, /* bq27530, bq27531 */
+     BQ27541 = 6, /* bq27541, bq27542, bq27546, bq27742 */
+     BQ27545 = 7, /* bq27545 */
+     BQ27421 = 8, /* bq27421, bq27425, bq27441, bq27621 */

include/linux/power/bq27xxx_battery.h line 23

Quoted 10 lines

include/linux/power/bq27xxx_battery.h +10 -0

+
+     /* members of categories; translate these to category in _setup() */
+     BQ27520 = 101,
+     BQ27531 = 102,
+     BQ27542 = 103,
+     BQ27546 = 104,
+     BQ27742 = 105,
+     BQ27425 = 106,
+     BQ27441 = 107,
+     BQ27621 = 108,

Get rid of this, just add new chip enum names if you need support for new chips.

Quoted 6 lines

Quoted 4 lines

How does a non-DT config obtain chip ID? We want explicit enum values if they appear in external platform-data objects. I'll mention that in the next patch description.

platform-data is going away, I'm not sure what you are saying here.

Don't 1-wire users have a platform-data config? You plan to force them to replace it with DT? I thought that wasn't kosher.

Quoted 5 lines

Quoted 2 lines

The 1-wire driver generates this platform-data, 1-wire is a discoverable bus, they never needed to define DT or platform data and still will not.

How does it map a discovered chip type to a bq27xxx_chip enum?

Right now it just assumes the chip is a BQ27000, see line 45 of drivers/w1/slaves/w1_bq27000.c

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

The category (original) IDs index bq27xxx_regs[], and you'll probably extend that in time. So I placed the member (new) IDs well above the categories to allow permanent explicit values.