[PATCH 1/2] power: supply: sbs-battery: don't assume MANUFACTURER_DATA formats

[PATCH 1/2] power: supply: sbs-battery: don't assume MANUFACTURER_DATA formats

[Brian Norris]

This driver was originally submitted for the TI BQ20Z75 battery IC
(commit a7640bfa10c5 ("power_supply: Add driver for TI BQ20Z75 gas gauge
IC")) and later renamed to express generic SBS support. While it's
mostly true that this driver implemented a standard SBS command set, it
takes liberties with the REG_MANUFACTURER_DATA register. This register
is specified in the SBS spec, but it doesn't make any mention of what
its actual contents are.

We've sort of noticed this optionality previously, with commit
17c6d3979e5b ("sbs-battery: make writes to ManufacturerAccess
optional"), where we found that some batteries NAK writes to this
register.

What this really means is that so far, we've just been lucky that most
batteries have either been compatible with the TI chip, or else at least
haven't reported highly-unexpected values.

For instance, one battery I have here seems to report either 0x0000 or
0x0100 to the MANUFACTURER_ACCESS_STATUS command -- while this seems to
match either Wake Up (bits[11:8] = 0000b) or Normal Discharge
(bits[11:8] = 0001b) status for the TI part [1], they don't seem to
actually correspond to real states (for instance, I never see 0101b =
Charge, even when charging).

On other batteries, I'm getting apparently random data in return, which
means that occasionally, we interpret this as "battery not present" or
"battery is not healthy".

All in all, it seems to be a really bad idea to make assumptions about
REG_MANUFACTURER_DATA, unless we already know what battery we're using.
Therefore, this patch reimplements the "present" and "health" checks to
the following on most SBS batteries:

1. HEALTH: report "unknown" -- I couldn't find a standard SBS command
   that gives us much useful here
2. PRESENT: just send a REG_STATUS command; if it succeeds, then the
   battery is present

Also, we stop sending MANUFACTURER_ACCESS_SLEEP to non-TI parts. I have
no proof that this is useful and supported.

If someone explicitly provided a 'ti,bq20z75' compatible property, then
we continue to use the existing TI command behaviors, and we effectively
revert commit 17c6d3979e5b ("sbs-battery: make writes to
ManufacturerAccess optional") to again make these commands required.

[1] http://www.ti.com/lit/er/sluu265a/sluu265a.pdf

Signed-off-by: Brian Norris <briannorris@chromium.org>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
Acked-by: Rhyland Klein <rklein@nvidia.com>
---
v2:
 * don't stub out POWER_SUPPLY_PROP_PRESENT from sbs_data[]
 * use if/else instead of switch/case

v3:
 * pull 'return 0' out of if/else, to satisfy braindead tooling

v4:
 * make ManufacturerAccess non-optional for TI parts again

v5: no change
---
 drivers/power/supply/sbs-battery.c | 67 ++++++++++++++++++++++++------
 1 file changed, 54 insertions(+), 13 deletions(-)

diff --git a/drivers/power/supply/sbs-battery.c b/drivers/power/supply/sbs-battery.c
index 83d7b4115857..8ba6abf584de 100644
--- a/drivers/power/supply/sbs-battery.c
+++ b/drivers/power/supply/sbs-battery.c
@@ -23,6 +23,7 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/of.h>
+#include <linux/of_device.h>
 #include <linux/power/sbs-battery.h>
 #include <linux/power_supply.h>
 #include <linux/slab.h>
@@ -156,6 +157,9 @@ static enum power_supply_property sbs_properties[] = {
 	POWER_SUPPLY_PROP_MODEL_NAME
 };
 
+/* Supports special manufacturer commands from TI BQ20Z75 IC. */
+#define SBS_FLAGS_TI_BQ20Z75		BIT(0)
+
 struct sbs_info {
 	struct i2c_client		*client;
 	struct power_supply		*power_supply;
@@ -168,6 +172,7 @@ struct sbs_info {
 	u32				poll_retry_count;
 	struct delayed_work		work;
 	struct mutex			mode_lock;
+	u32				flags;
 };
 
 static char model_name[I2C_SMBUS_BLOCK_MAX + 1];
@@ -315,17 +320,41 @@ static int sbs_status_correct(struct i2c_client *client, int *intval)
 static int sbs_get_battery_presence_and_health(
 	struct i2c_client *client, enum power_supply_property psp,
 	union power_supply_propval *val)
+{
+	int ret;
+
+	if (psp == POWER_SUPPLY_PROP_PRESENT) {
+		/* Dummy command; if it succeeds, battery is present. */
+		ret = sbs_read_word_data(client, sbs_data[REG_STATUS].addr);
+		if (ret < 0)
+			val->intval = 0; /* battery disconnected */
+		else
+			val->intval = 1; /* battery present */
+	} else { /* POWER_SUPPLY_PROP_HEALTH */
+		/* SBS spec doesn't have a general health command. */
+		val->intval = POWER_SUPPLY_HEALTH_UNKNOWN;
+	}
+
+	return 0;
+}
+
+static int sbs_get_ti_battery_presence_and_health(
+	struct i2c_client *client, enum power_supply_property psp,
+	union power_supply_propval *val)
 {
 	s32 ret;
 
 	/*
 	 * Write to ManufacturerAccess with ManufacturerAccess command
-	 * and then read the status. Do not check for error on the write
-	 * since not all batteries implement write access to this command,
-	 * while others mandate it.
+	 * and then read the status.
 	 */
-	sbs_write_word_data(client, sbs_data[REG_MANUFACTURER_DATA].addr,
-			    MANUFACTURER_ACCESS_STATUS);
+	ret = sbs_write_word_data(client, sbs_data[REG_MANUFACTURER_DATA].addr,
+				  MANUFACTURER_ACCESS_STATUS);
+	if (ret < 0) {
+		if (psp == POWER_SUPPLY_PROP_PRESENT)
+			val->intval = 0; /* battery removed */
+		return ret;
+	}
 
 	ret = sbs_read_word_data(client, sbs_data[REG_MANUFACTURER_DATA].addr);
 	if (ret < 0) {
@@ -600,7 +629,12 @@ static int sbs_get_property(struct power_supply *psy,
 	switch (psp) {
 	case POWER_SUPPLY_PROP_PRESENT:
 	case POWER_SUPPLY_PROP_HEALTH:
-		ret = sbs_get_battery_presence_and_health(client, psp, val);
+		if (client->flags & SBS_FLAGS_TI_BQ20Z75)
+			ret = sbs_get_ti_battery_presence_and_health(client,
+								     psp, val);
+		else
+			ret = sbs_get_battery_presence_and_health(client, psp,
+								  val);
 		if (psp == POWER_SUPPLY_PROP_PRESENT)
 			return 0;
 		break;
@@ -806,6 +840,7 @@ static int sbs_probe(struct i2c_client *client,
 	if (!chip)
 		return -ENOMEM;
 
+	chip->flags = (u32)(uintptr_t)of_device_get_match_data(&client->dev);
 	chip->client = client;
 	chip->enable_detection = false;
 	psy_cfg.of_node = client->dev.of_node;
@@ -911,16 +946,19 @@ static int sbs_suspend(struct device *dev)
 {
 	struct i2c_client *client = to_i2c_client(dev);
 	struct sbs_info *chip = i2c_get_clientdata(client);
+	int ret;
 
 	if (chip->poll_time > 0)
 		cancel_delayed_work_sync(&chip->work);
 
-	/*
-	 * Write to manufacturer access with sleep command.
-	 * Support is manufacturer dependend, so ignore errors.
-	 */
-	sbs_write_word_data(client, sbs_data[REG_MANUFACTURER_DATA].addr,
-		MANUFACTURER_ACCESS_SLEEP);
+	if (chip->flags & SBS_FLAGS_TI_BQ20Z75) {
+		/* Write to manufacturer access with sleep command. */
+		ret = sbs_write_word_data(client,
+					  sbs_data[REG_MANUFACTURER_DATA].addr,
+					  MANUFACTURER_ACCESS_SLEEP);
+		if (chip->is_present && ret < 0)
+			return ret;
+	}
 
 	return 0;
 }
@@ -941,7 +979,10 @@ MODULE_DEVICE_TABLE(i2c, sbs_id);
 
 static const struct of_device_id sbs_dt_ids[] = {
 	{ .compatible = "sbs,sbs-battery" },
-	{ .compatible = "ti,bq20z75" },
+	{
+		.compatible = "ti,bq20z75",
+		.data = (void *)SBS_FLAGS_TI_BQ20Z75,
+	},
 	{ }
 };
 MODULE_DEVICE_TABLE(of, sbs_dt_ids);
-- 
2.18.0.rc1.242.g61856ae69a-goog

[PATCH 1/2] power: supply: sbs-battery: don't assume MANUFACTURER_DATA formats

[Brian Norris]

This driver was originally submitted for the TI BQ20Z75 battery IC
(commit a7640bfa10c5 ("power_supply: Add driver for TI BQ20Z75 gas gauge
IC")) and later renamed to express generic SBS support. While it's
mostly true that this driver implemented a standard SBS command set, it
takes liberties with the REG_MANUFACTURER_DATA register. This register
is specified in the SBS spec, but it doesn't make any mention of what
its actual contents are.

We've sort of noticed this optionality previously, with commit
17c6d3979e5b ("sbs-battery: make writes to ManufacturerAccess
optional"), where we found that some batteries NAK writes to this
register.

What this really means is that so far, we've just been lucky that most
batteries have either been compatible with the TI chip, or else at least
haven't reported highly-unexpected values.

For instance, one battery I have here seems to report either 0x0000 or
0x0100 to the MANUFACTURER_ACCESS_STATUS command -- while this seems to
match either Wake Up (bits[11:8] = 0000b) or Normal Discharge
(bits[11:8] = 0001b) status for the TI part [1], they don't seem to
actually correspond to real states (for instance, I never see 0101b =
Charge, even when charging).

On other batteries, I'm getting apparently random data in return, which
means that occasionally, we interpret this as "battery not present" or
"battery is not healthy".

All in all, it seems to be a really bad idea to make assumptions about
REG_MANUFACTURER_DATA, unless we already know what battery we're using.
Therefore, this patch reimplements the "present" and "health" checks to
the following on most SBS batteries:

1. HEALTH: report "unknown" -- I couldn't find a standard SBS command
   that gives us much useful here
2. PRESENT: just send a REG_STATUS command; if it succeeds, then the
   battery is present

Also, we stop sending MANUFACTURER_ACCESS_SLEEP to non-TI parts. I have
no proof that this is useful and supported.

If someone explicitly provided a 'ti,bq20z75' compatible property, then
we continue to use the existing TI command behaviors, and we effectively
revert commit 17c6d3979e5b ("sbs-battery: make writes to
ManufacturerAccess optional") to again make these commands required.

[1] http://www.ti.com/lit/er/sluu265a/sluu265a.pdf

Signed-off-by: Brian Norris <briannorris@chromium.org>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
Acked-by: Rhyland Klein <rklein@nvidia.com>
---
v2:
 * don't stub out POWER_SUPPLY_PROP_PRESENT from sbs_data[]
 * use if/else instead of switch/case

v3:
 * pull 'return 0' out of if/else, to satisfy braindead tooling

v4:
 * make ManufacturerAccess non-optional for TI parts again

v5: no change
---
 drivers/power/supply/sbs-battery.c | 67 ++++++++++++++++++++++++------
 1 file changed, 54 insertions(+), 13 deletions(-)

diff --git a/drivers/power/supply/sbs-battery.c b/drivers/power/supply/sbs-battery.c
index 83d7b4115857..8ba6abf584de 100644
--- a/drivers/power/supply/sbs-battery.c
+++ b/drivers/power/supply/sbs-battery.c
@@ -23,6 +23,7 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/of.h>
+#include <linux/of_device.h>
 #include <linux/power/sbs-battery.h>
 #include <linux/power_supply.h>
 #include <linux/slab.h>
@@ -156,6 +157,9 @@ static enum power_supply_property sbs_properties[] = {
 	POWER_SUPPLY_PROP_MODEL_NAME
 };
 
+/* Supports special manufacturer commands from TI BQ20Z75 IC. */
+#define SBS_FLAGS_TI_BQ20Z75		BIT(0)
+
 struct sbs_info {
 	struct i2c_client		*client;
 	struct power_supply		*power_supply;
@@ -168,6 +172,7 @@ struct sbs_info {
 	u32				poll_retry_count;
 	struct delayed_work		work;
 	struct mutex			mode_lock;
+	u32				flags;
 };
 
 static char model_name[I2C_SMBUS_BLOCK_MAX + 1];
@@ -315,17 +320,41 @@ static int sbs_status_correct(struct i2c_client *client, int *intval)
 static int sbs_get_battery_presence_and_health(
 	struct i2c_client *client, enum power_supply_property psp,
 	union power_supply_propval *val)
+{
+	int ret;
+
+	if (psp == POWER_SUPPLY_PROP_PRESENT) {
+		/* Dummy command; if it succeeds, battery is present. */
+		ret = sbs_read_word_data(client, sbs_data[REG_STATUS].addr);
+		if (ret < 0)
+			val->intval = 0; /* battery disconnected */
+		else
+			val->intval = 1; /* battery present */
+	} else { /* POWER_SUPPLY_PROP_HEALTH */
+		/* SBS spec doesn't have a general health command. */
+		val->intval = POWER_SUPPLY_HEALTH_UNKNOWN;
+	}
+
+	return 0;
+}
+
+static int sbs_get_ti_battery_presence_and_health(
+	struct i2c_client *client, enum power_supply_property psp,
+	union power_supply_propval *val)
 {
 	s32 ret;
 
 	/*
 	 * Write to ManufacturerAccess with ManufacturerAccess command
-	 * and then read the status. Do not check for error on the write
-	 * since not all batteries implement write access to this command,
-	 * while others mandate it.
+	 * and then read the status.
 	 */
-	sbs_write_word_data(client, sbs_data[REG_MANUFACTURER_DATA].addr,
-			    MANUFACTURER_ACCESS_STATUS);
+	ret = sbs_write_word_data(client, sbs_data[REG_MANUFACTURER_DATA].addr,
+				  MANUFACTURER_ACCESS_STATUS);
+	if (ret < 0) {
+		if (psp == POWER_SUPPLY_PROP_PRESENT)
+			val->intval = 0; /* battery removed */
+		return ret;
+	}
 
 	ret = sbs_read_word_data(client, sbs_data[REG_MANUFACTURER_DATA].addr);
 	if (ret < 0) {
@@ -600,7 +629,12 @@ static int sbs_get_property(struct power_supply *psy,
 	switch (psp) {
 	case POWER_SUPPLY_PROP_PRESENT:
 	case POWER_SUPPLY_PROP_HEALTH:
-		ret = sbs_get_battery_presence_and_health(client, psp, val);
+		if (client->flags & SBS_FLAGS_TI_BQ20Z75)
+			ret = sbs_get_ti_battery_presence_and_health(client,
+								     psp, val);
+		else
+			ret = sbs_get_battery_presence_and_health(client, psp,
+								  val);
 		if (psp == POWER_SUPPLY_PROP_PRESENT)
 			return 0;
 		break;
@@ -806,6 +840,7 @@ static int sbs_probe(struct i2c_client *client,
 	if (!chip)
 		return -ENOMEM;
 
+	chip->flags = (u32)(uintptr_t)of_device_get_match_data(&client->dev);
 	chip->client = client;
 	chip->enable_detection = false;
 	psy_cfg.of_node = client->dev.of_node;
@@ -911,16 +946,19 @@ static int sbs_suspend(struct device *dev)
 {
 	struct i2c_client *client = to_i2c_client(dev);
 	struct sbs_info *chip = i2c_get_clientdata(client);
+	int ret;
 
 	if (chip->poll_time > 0)
 		cancel_delayed_work_sync(&chip->work);
 
-	/*
-	 * Write to manufacturer access with sleep command.
-	 * Support is manufacturer dependend, so ignore errors.
-	 */
-	sbs_write_word_data(client, sbs_data[REG_MANUFACTURER_DATA].addr,
-		MANUFACTURER_ACCESS_SLEEP);
+	if (chip->flags & SBS_FLAGS_TI_BQ20Z75) {
+		/* Write to manufacturer access with sleep command. */
+		ret = sbs_write_word_data(client,
+					  sbs_data[REG_MANUFACTURER_DATA].addr,
+					  MANUFACTURER_ACCESS_SLEEP);
+		if (chip->is_present && ret < 0)
+			return ret;
+	}
 
 	return 0;
 }
@@ -941,7 +979,10 @@ MODULE_DEVICE_TABLE(i2c, sbs_id);
 
 static const struct of_device_id sbs_dt_ids[] = {
 	{ .compatible = "sbs,sbs-battery" },
-	{ .compatible = "ti,bq20z75" },
+	{
+		.compatible = "ti,bq20z75",
+		.data = (void *)SBS_FLAGS_TI_BQ20Z75,
+	},
 	{ }
 };
 MODULE_DEVICE_TABLE(of, sbs_dt_ids);
-- 
2.18.0.rc1.242.g61856ae69a-goog

[PATCH 2/2] dt-bindings: power: sbs-battery: re-document "ti,bq20z75"

[Brian Norris]

This compatible property was documented before the driver was renamed to
"SBS" (see commit e57f1b68c406 ("devicetree-bindings: Propagate
bq20z75->sbs rename to dt bindings")). The driver has continued to
support this property as an alternative to "sbs,sbs-battery", and
because we've noticed there are some lingering TI specifics (in the
manufacturer-specific portion of the SBS spec), we'd like to start using
this property again to differentiate.

In typical DT fashion, the <vendor>,<part-number> specifics should be
used ahead of the generic "sbs,sbs-battery" string, so we can handle
vendor specifics -- so document this. Language borrowed mostly from
Documentation/devicetree/bindings/power/supply/sbs_sbs-charger.txt

Also fixup the example to use this property (it's already implying that
it's "bq20z75@b"); fixup the node name to be generic ("battery", not
"<part-number>"); and fixup some whitespace.

Signed-off-by: Brian Norris <briannorris@chromium.org>
Acked-by: Rhyland Klein <rklein@nvidia.com>
Reviewed-by: Rob Herring <robh@kernel.org>
---
v2: add Rhyland's Acked-by
v3: no change
v4: add Rob's Reviewed-by
v5:
 * note "sbs,sbs-battery" as a fallback for vendor,part-number
 * improve the Example
 * I still kept the {Acked,Reviewed}-by, since the substance of the
   change is essentially the same. Apologies if that is not proper.
---
 .../bindings/power/supply/sbs_sbs-battery.txt        | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/power/supply/sbs_sbs-battery.txt b/Documentation/devicetree/bindings/power/supply/sbs_sbs-battery.txt
index c40e8926facf..4e78e51018eb 100644
--- a/Documentation/devicetree/bindings/power/supply/sbs_sbs-battery.txt
+++ b/Documentation/devicetree/bindings/power/supply/sbs_sbs-battery.txt
@@ -2,7 +2,11 @@ SBS sbs-battery
 ~~~~~~~~~~
 
 Required properties :
- - compatible : "sbs,sbs-battery"
+ - compatible: "<vendor>,<part-number>", "sbs,sbs-battery" as fallback. The
+     part number compatible string might be used in order to take care of
+     vendor specific registers.
+     Known <vendor>,<part-number>:
+       ti,bq20z75
 
 Optional properties :
  - sbs,i2c-retry-count : The number of times to retry i2c transactions on i2c
@@ -14,9 +18,9 @@ Optional properties :
 
 Example:
 
-	bq20z75@b {
-		compatible = "sbs,sbs-battery";
-		reg = < 0xb >;
+	battery@b {
+		compatible = "ti,bq20z75", "sbs,sbs-battery";
+		reg = <0xb>;
 		sbs,i2c-retry-count = <2>;
 		sbs,poll-retry-count = <10>;
 		sbs,battery-detect-gpios = <&gpio-controller 122 1>;
-- 
2.18.0.rc1.242.g61856ae69a-goog

[PATCH 2/2] dt-bindings: power: sbs-battery: re-document "ti,bq20z75"

[Brian Norris]

This compatible property was documented before the driver was renamed to
"SBS" (see commit e57f1b68c406 ("devicetree-bindings: Propagate
bq20z75->sbs rename to dt bindings")). The driver has continued to
support this property as an alternative to "sbs,sbs-battery", and
because we've noticed there are some lingering TI specifics (in the
manufacturer-specific portion of the SBS spec), we'd like to start using
this property again to differentiate.

In typical DT fashion, the <vendor>,<part-number> specifics should be
used ahead of the generic "sbs,sbs-battery" string, so we can handle
vendor specifics -- so document this. Language borrowed mostly from
Documentation/devicetree/bindings/power/supply/sbs_sbs-charger.txt

Also fixup the example to use this property (it's already implying that
it's "bq20z75@b"); fixup the node name to be generic ("battery", not
"<part-number>"); and fixup some whitespace.

Signed-off-by: Brian Norris <briannorris@chromium.org>
Acked-by: Rhyland Klein <rklein@nvidia.com>
Reviewed-by: Rob Herring <robh@kernel.org>
---
v2: add Rhyland's Acked-by
v3: no change
v4: add Rob's Reviewed-by
v5:
 * note "sbs,sbs-battery" as a fallback for vendor,part-number
 * improve the Example
 * I still kept the {Acked,Reviewed}-by, since the substance of the
   change is essentially the same. Apologies if that is not proper.
---
 .../bindings/power/supply/sbs_sbs-battery.txt        | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/power/supply/sbs_sbs-battery.txt b/Documentation/devicetree/bindings/power/supply/sbs_sbs-battery.txt
index c40e8926facf..4e78e51018eb 100644
--- a/Documentation/devicetree/bindings/power/supply/sbs_sbs-battery.txt
+++ b/Documentation/devicetree/bindings/power/supply/sbs_sbs-battery.txt
@@ -2,7 +2,11 @@ SBS sbs-battery
 ~~~~~~~~~~
 
 Required properties :
- - compatible : "sbs,sbs-battery"
+ - compatible: "<vendor>,<part-number>", "sbs,sbs-battery" as fallback. The
+     part number compatible string might be used in order to take care of
+     vendor specific registers.
+     Known <vendor>,<part-number>:
+       ti,bq20z75
 
 Optional properties :
  - sbs,i2c-retry-count : The number of times to retry i2c transactions on i2c
@@ -14,9 +18,9 @@ Optional properties :
 
 Example:
 
-	bq20z75@b {
-		compatible = "sbs,sbs-battery";
-		reg = < 0xb >;
+	battery@b {
+		compatible = "ti,bq20z75", "sbs,sbs-battery";
+		reg = <0xb>;
 		sbs,i2c-retry-count = <2>;
 		sbs,poll-retry-count = <10>;
 		sbs,battery-detect-gpios = <&gpio-controller 122 1>;
-- 
2.18.0.rc1.242.g61856ae69a-goog

Re: [PATCH 1/2] power: supply: sbs-battery: don't assume MANUFACTURER_DATA formats

[Brian Norris]

Eek, sorry this series should have subjects "[PATCH v5 X/2] ...". I
can resend if really needed, but hopefully by now this is ready to
go...

Brian

On Tue, Jun 12, 2018 at 1:20 PM, Brian Norris <briannorris@chromium.org> wrote:
> This driver was originally submitted for the TI BQ20Z75 battery IC
> (commit a7640bfa10c5 ("power_supply: Add driver for TI BQ20Z75 gas gauge
> IC")) and later renamed to express generic SBS support. While it's
> mostly true that this driver implemented a standard SBS command set, it
> takes liberties with the REG_MANUFACTURER_DATA register. This register
> is specified in the SBS spec, but it doesn't make any mention of what
> its actual contents are.

...

> ---
> v2:
>  * don't stub out POWER_SUPPLY_PROP_PRESENT from sbs_data[]
>  * use if/else instead of switch/case
>
> v3:
>  * pull 'return 0' out of if/else, to satisfy braindead tooling
>
> v4:
>  * make ManufacturerAccess non-optional for TI parts again
>
> v5: no change
> ---
>  drivers/power/supply/sbs-battery.c | 67 ++++++++++++++++++++++++------
>  1 file changed, 54 insertions(+), 13 deletions(-)

Re: [PATCH 1/2] power: supply: sbs-battery: don't assume MANUFACTURER_DATA formats

[Sebastian Reichel]

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

Hi Brian,

On Tue, Jun 12, 2018 at 01:23:25PM -0700, Brian Norris wrote:
> Eek, sorry this series should have subjects "[PATCH v5 X/2] ...". I
> can resend if really needed, but hopefully by now this is ready to
> go...

No need to resend. Patches look fine to me, but we are currently
in the merge window. I will queue them once 4.18-rc1 has been
tagged and linux-next is open for 4.19 content.

-- Sebastian

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

Re: [PATCH 1/2] power: supply: sbs-battery: don't assume MANUFACTURER_DATA formats

[Sebastian Reichel]

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

Hi,

On Tue, Jun 12, 2018 at 01:20:41PM -0700, Brian Norris wrote:
> This driver was originally submitted for the TI BQ20Z75 battery IC
> (commit a7640bfa10c5 ("power_supply: Add driver for TI BQ20Z75 gas gauge
> IC")) and later renamed to express generic SBS support. While it's
> mostly true that this driver implemented a standard SBS command set, it
> takes liberties with the REG_MANUFACTURER_DATA register. This register
> is specified in the SBS spec, but it doesn't make any mention of what
> its actual contents are.
> 
> We've sort of noticed this optionality previously, with commit
> 17c6d3979e5b ("sbs-battery: make writes to ManufacturerAccess
> optional"), where we found that some batteries NAK writes to this
> register.
> 
> What this really means is that so far, we've just been lucky that most
> batteries have either been compatible with the TI chip, or else at least
> haven't reported highly-unexpected values.
> 
> For instance, one battery I have here seems to report either 0x0000 or
> 0x0100 to the MANUFACTURER_ACCESS_STATUS command -- while this seems to
> match either Wake Up (bits[11:8] = 0000b) or Normal Discharge
> (bits[11:8] = 0001b) status for the TI part [1], they don't seem to
> actually correspond to real states (for instance, I never see 0101b =
> Charge, even when charging).
> 
> On other batteries, I'm getting apparently random data in return, which
> means that occasionally, we interpret this as "battery not present" or
> "battery is not healthy".
> 
> All in all, it seems to be a really bad idea to make assumptions about
> REG_MANUFACTURER_DATA, unless we already know what battery we're using.
> Therefore, this patch reimplements the "present" and "health" checks to
> the following on most SBS batteries:
> 
> 1. HEALTH: report "unknown" -- I couldn't find a standard SBS command
>    that gives us much useful here
> 2. PRESENT: just send a REG_STATUS command; if it succeeds, then the
>    battery is present
> 
> Also, we stop sending MANUFACTURER_ACCESS_SLEEP to non-TI parts. I have
> no proof that this is useful and supported.
> 
> If someone explicitly provided a 'ti,bq20z75' compatible property, then
> we continue to use the existing TI command behaviors, and we effectively
> revert commit 17c6d3979e5b ("sbs-battery: make writes to
> ManufacturerAccess optional") to again make these commands required.
> 
> [1] http://www.ti.com/lit/er/sluu265a/sluu265a.pdf
> 
> Signed-off-by: Brian Norris <briannorris@chromium.org>
> Reviewed-by: Guenter Roeck <linux@roeck-us.net>
> Acked-by: Rhyland Klein <rklein@nvidia.com>
> ---

Thanks, queued to power-supply-next.

-- Sebastian

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

Re: [PATCH 2/2] dt-bindings: power: sbs-battery: re-document "ti,bq20z75"

[Sebastian Reichel]

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

Hi,

On Tue, Jun 12, 2018 at 01:20:42PM -0700, Brian Norris wrote:
> This compatible property was documented before the driver was renamed to
> "SBS" (see commit e57f1b68c406 ("devicetree-bindings: Propagate
> bq20z75->sbs rename to dt bindings")). The driver has continued to
> support this property as an alternative to "sbs,sbs-battery", and
> because we've noticed there are some lingering TI specifics (in the
> manufacturer-specific portion of the SBS spec), we'd like to start using
> this property again to differentiate.
> 
> In typical DT fashion, the <vendor>,<part-number> specifics should be
> used ahead of the generic "sbs,sbs-battery" string, so we can handle
> vendor specifics -- so document this. Language borrowed mostly from
> Documentation/devicetree/bindings/power/supply/sbs_sbs-charger.txt
> 
> Also fixup the example to use this property (it's already implying that
> it's "bq20z75@b"); fixup the node name to be generic ("battery", not
> "<part-number>"); and fixup some whitespace.
> 
> Signed-off-by: Brian Norris <briannorris@chromium.org>
> Acked-by: Rhyland Klein <rklein@nvidia.com>
> Reviewed-by: Rob Herring <robh@kernel.org>
> ---

Thanks, queued to power-supply-next.

-- Sebastian

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

Re: [PATCH 1/2] power: supply: sbs-battery: don't assume MANUFACTURER_DATA formats

[Guenter Roeck]

Hi Brian,

On 05/31/2018 05:32 PM, Brian Norris wrote:
> This driver was originally submitted for the TI BQ20Z75 battery IC
> (commit a7640bfa10c5 ("power_supply: Add driver for TI BQ20Z75 gas gauge
> IC")) and later renamed to express generic SBS support. While it's
> mostly true that this driver implemented a standard SBS command set, it
> takes liberties with the REG_MANUFACTURER_DATA register. This register
> is specified in the SBS spec, but it doesn't make any mention of what
> its actual contents are.
> 
> We've sort of noticed this optionality previously, with commit
> 17c6d3979e5b ("sbs-battery: make writes to ManufacturerAccess
> optional"), where we found that some batteries NAK writes to this
> register.
> 
> What this really means is that so far, we've just been lucky that most
> batteries have either been compatible with the TI chip, or else at least
> haven't reported highly-unexpected values.
> 
> For instance, one battery I have here seems to report either 0x0000 or
> 0x0100 to the MANUFACTURER_ACCESS_STATUS command -- while this seems to
> match either Wake Up (bits[11:8] = 0000b) or Normal Discharge
> (bits[11:8] = 0001b) status for the TI part [1], they don't seem to
> actually correspond to real states (for instance, I never see 0101b =
> Charge, even when charging).
> 
> On other batteries, I'm getting apparently random data in return, which
> means that occasionally, we interpret this as "battery not present" or
> "battery is not healthy".
> 
> All in all, it seems to be a really bad idea to make assumptions about
> REG_MANUFACTURER_DATA, unless we already know what battery we're using.
> Therefore, this patch reimplements the "present" and "health" checks to
> the following on most SBS batteries:
> 
> 1. HEALTH: report "unknown" -- I couldn't find a standard SBS command
>     that gives us much useful here
> 2. PRESENT: just send a REG_STATUS command; if it succeeds, then the
>     battery is present
> 
> Also, we stop sending MANUFACTURER_ACCESS_SLEEP to non-TI parts. I have
> no proof that this is useful and supported.
> 
> If someone explicitly provided a 'ti,bq20z75' compatible property, then
> we retain the existing TI command behaviors.
> 
> [1] http://www.ti.com/lit/er/sluu265a/sluu265a.pdf
> 

Excellent idea. Couple of comments below.

Thanks,
Guenter

> Signed-off-by: Brian Norris <briannorris@chromium.org>
> ---
>   drivers/power/supply/sbs-battery.c | 61 +++++++++++++++++++++++++-----
>   1 file changed, 52 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/power/supply/sbs-battery.c b/drivers/power/supply/sbs-battery.c
> index 83d7b4115857..e15d0ca4729d 100644
> --- a/drivers/power/supply/sbs-battery.c
> +++ b/drivers/power/supply/sbs-battery.c
> @@ -23,6 +23,7 @@
>   #include <linux/kernel.h>
>   #include <linux/module.h>
>   #include <linux/of.h>
> +#include <linux/of_device.h>
>   #include <linux/power/sbs-battery.h>
>   #include <linux/power_supply.h>
>   #include <linux/slab.h>
> @@ -84,8 +85,9 @@ static const struct chip_data {
>   	int min_value;
>   	int max_value;
>   } sbs_data[] = {
> +	/* Manuf. data isn't directly useful as a POWER_SUPPLY_PROP */
>   	[REG_MANUFACTURER_DATA] =
> -		SBS_DATA(POWER_SUPPLY_PROP_PRESENT, 0x00, 0, 65535),
> +		SBS_DATA(-1, 0x00, 0, 65535),

I don't think this change is necessary. For example, POWER_SUPPLY_PROP_SERIAL_NUMBER
is also not directly accessed, yet the REG_SERIAL_NUMBER array entry includes
POWER_SUPPLY_PROP_SERIAL_NUMBER.

>   	[REG_TEMPERATURE] =
>   		SBS_DATA(POWER_SUPPLY_PROP_TEMP, 0x08, 0, 65535),
>   	[REG_VOLTAGE] =
> @@ -156,6 +158,9 @@ static enum power_supply_property sbs_properties[] = {
>   	POWER_SUPPLY_PROP_MODEL_NAME
>   };
>   
> +/* Supports special manufacturer commands from TI BQ20Z75 IC. */
> +#define SBS_FLAGS_TI_BQ20Z75		BIT(0)
> +
>   struct sbs_info {
>   	struct i2c_client		*client;
>   	struct power_supply		*power_supply;
> @@ -168,6 +173,7 @@ struct sbs_info {
>   	u32				poll_retry_count;
>   	struct delayed_work		work;
>   	struct mutex			mode_lock;
> +	u32				flags;
>   };
>   
>   static char model_name[I2C_SMBUS_BLOCK_MAX + 1];
> @@ -315,6 +321,31 @@ static int sbs_status_correct(struct i2c_client *client, int *intval)
>   static int sbs_get_battery_presence_and_health(
>   	struct i2c_client *client, enum power_supply_property psp,
>   	union power_supply_propval *val)
> +{
> +	int ret;
> +
> +	switch (psp) {
> +	case POWER_SUPPLY_PROP_PRESENT:
> +		/* Dummy command; if it succeeds, battery is present. */
> +		ret = sbs_read_word_data(client, sbs_data[REG_STATUS].addr);
> +		if (ret < 0)
> +			val->intval = 0; /* battery disconnected */

I don't know the relevance, but the original code returns the error
in this situation.

> +		else
> +			val->intval = 1; /* battery present */
> +		return 0;
> +	case POWER_SUPPLY_PROP_HEALTH:
> +		/* SBS spec doesn't have a general health command. */
> +		val->intval = POWER_SUPPLY_HEALTH_UNKNOWN;
> +		return 0;
> +	default:
> +		dev_err(&client->dev, "unexpected prop %d\n", psp);
> +		return -EINVAL;

Personally I would prefer if/else here, for the simple reason that the default case
will never be executed and just wastes a bit of code space.

> +	}
> +}
> +
> +static int sbs_get_ti_battery_presence_and_health(
> +	struct i2c_client *client, enum power_supply_property psp,
> +	union power_supply_propval *val)
>   {
>   	s32 ret;
>   
> @@ -600,7 +631,12 @@ static int sbs_get_property(struct power_supply *psy,
>   	switch (psp) {
>   	case POWER_SUPPLY_PROP_PRESENT:
>   	case POWER_SUPPLY_PROP_HEALTH:
> -		ret = sbs_get_battery_presence_and_health(client, psp, val);
> +		if (client->flags & SBS_FLAGS_TI_BQ20Z75)
> +			ret = sbs_get_ti_battery_presence_and_health(client,
> +								     psp, val);
> +		else
> +			ret = sbs_get_battery_presence_and_health(client, psp,
> +								  val);
>   		if (psp == POWER_SUPPLY_PROP_PRESENT)
>   			return 0;
>   		break;
> @@ -806,6 +842,7 @@ static int sbs_probe(struct i2c_client *client,
>   	if (!chip)
>   		return -ENOMEM;
>   
> +	chip->flags = (u32)(uintptr_t)of_device_get_match_data(&client->dev);
>   	chip->client = client;
>   	chip->enable_detection = false;
>   	psy_cfg.of_node = client->dev.of_node;
> @@ -915,12 +952,15 @@ static int sbs_suspend(struct device *dev)
>   	if (chip->poll_time > 0)
>   		cancel_delayed_work_sync(&chip->work);
>   
> -	/*
> -	 * Write to manufacturer access with sleep command.
> -	 * Support is manufacturer dependend, so ignore errors.
> -	 */
> -	sbs_write_word_data(client, sbs_data[REG_MANUFACTURER_DATA].addr,
> -		MANUFACTURER_ACCESS_SLEEP);
> +	if (chip->flags & SBS_FLAGS_TI_BQ20Z75) {
> +		/*
> +		 * Write to manufacturer access with sleep command.
> +		 * Support is manufacturer dependent, so ignore errors.
> +		 */
> +		sbs_write_word_data(client,
> +				    sbs_data[REG_MANUFACTURER_DATA].addr,
> +				    MANUFACTURER_ACCESS_SLEEP);
> +	}
>   
>   	return 0;
>   }
> @@ -941,7 +981,10 @@ MODULE_DEVICE_TABLE(i2c, sbs_id);
>   
>   static const struct of_device_id sbs_dt_ids[] = {
>   	{ .compatible = "sbs,sbs-battery" },
> -	{ .compatible = "ti,bq20z75" },
> +	{
> +		.compatible = "ti,bq20z75",
> +		.data = (void *)SBS_FLAGS_TI_BQ20Z75,
> +	},
>   	{ }
>   };
>   MODULE_DEVICE_TABLE(of, sbs_dt_ids);
> 

[PATCH 1/2] power: supply: sbs-battery: don't assume MANUFACTURER_DATA formats

[Brian Norris]

This driver was originally submitted for the TI BQ20Z75 battery IC
(commit a7640bfa10c5 ("power_supply: Add driver for TI BQ20Z75 gas gauge
IC")) and later renamed to express generic SBS support. While it's
mostly true that this driver implemented a standard SBS command set, it
takes liberties with the REG_MANUFACTURER_DATA register. This register
is specified in the SBS spec, but it doesn't make any mention of what
its actual contents are.

We've sort of noticed this optionality previously, with commit
17c6d3979e5b ("sbs-battery: make writes to ManufacturerAccess
optional"), where we found that some batteries NAK writes to this
register.

What this really means is that so far, we've just been lucky that most
batteries have either been compatible with the TI chip, or else at least
haven't reported highly-unexpected values.

For instance, one battery I have here seems to report either 0x0000 or
0x0100 to the MANUFACTURER_ACCESS_STATUS command -- while this seems to
match either Wake Up (bits[11:8] = 0000b) or Normal Discharge
(bits[11:8] = 0001b) status for the TI part [1], they don't seem to
actually correspond to real states (for instance, I never see 0101b =
Charge, even when charging).

On other batteries, I'm getting apparently random data in return, which
means that occasionally, we interpret this as "battery not present" or
"battery is not healthy".

All in all, it seems to be a really bad idea to make assumptions about
REG_MANUFACTURER_DATA, unless we already know what battery we're using.
Therefore, this patch reimplements the "present" and "health" checks to
the following on most SBS batteries:

1. HEALTH: report "unknown" -- I couldn't find a standard SBS command
   that gives us much useful here
2. PRESENT: just send a REG_STATUS command; if it succeeds, then the
   battery is present

Also, we stop sending MANUFACTURER_ACCESS_SLEEP to non-TI parts. I have
no proof that this is useful and supported.

If someone explicitly provided a 'ti,bq20z75' compatible property, then
we retain the existing TI command behaviors.

[1] http://www.ti.com/lit/er/sluu265a/sluu265a.pdf

Signed-off-by: Brian Norris <briannorris@chromium.org>
---
 drivers/power/supply/sbs-battery.c | 61 +++++++++++++++++++++++++-----
 1 file changed, 52 insertions(+), 9 deletions(-)

diff --git a/drivers/power/supply/sbs-battery.c b/drivers/power/supply/sbs-battery.c
index 83d7b4115857..e15d0ca4729d 100644
--- a/drivers/power/supply/sbs-battery.c
+++ b/drivers/power/supply/sbs-battery.c
@@ -23,6 +23,7 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/of.h>
+#include <linux/of_device.h>
 #include <linux/power/sbs-battery.h>
 #include <linux/power_supply.h>
 #include <linux/slab.h>
@@ -84,8 +85,9 @@ static const struct chip_data {
 	int min_value;
 	int max_value;
 } sbs_data[] = {
+	/* Manuf. data isn't directly useful as a POWER_SUPPLY_PROP */
 	[REG_MANUFACTURER_DATA] =
-		SBS_DATA(POWER_SUPPLY_PROP_PRESENT, 0x00, 0, 65535),
+		SBS_DATA(-1, 0x00, 0, 65535),
 	[REG_TEMPERATURE] =
 		SBS_DATA(POWER_SUPPLY_PROP_TEMP, 0x08, 0, 65535),
 	[REG_VOLTAGE] =
@@ -156,6 +158,9 @@ static enum power_supply_property sbs_properties[] = {
 	POWER_SUPPLY_PROP_MODEL_NAME
 };
 
+/* Supports special manufacturer commands from TI BQ20Z75 IC. */
+#define SBS_FLAGS_TI_BQ20Z75		BIT(0)
+
 struct sbs_info {
 	struct i2c_client		*client;
 	struct power_supply		*power_supply;
@@ -168,6 +173,7 @@ struct sbs_info {
 	u32				poll_retry_count;
 	struct delayed_work		work;
 	struct mutex			mode_lock;
+	u32				flags;
 };
 
 static char model_name[I2C_SMBUS_BLOCK_MAX + 1];
@@ -315,6 +321,31 @@ static int sbs_status_correct(struct i2c_client *client, int *intval)
 static int sbs_get_battery_presence_and_health(
 	struct i2c_client *client, enum power_supply_property psp,
 	union power_supply_propval *val)
+{
+	int ret;
+
+	switch (psp) {
+	case POWER_SUPPLY_PROP_PRESENT:
+		/* Dummy command; if it succeeds, battery is present. */
+		ret = sbs_read_word_data(client, sbs_data[REG_STATUS].addr);
+		if (ret < 0)
+			val->intval = 0; /* battery disconnected */
+		else
+			val->intval = 1; /* battery present */
+		return 0;
+	case POWER_SUPPLY_PROP_HEALTH:
+		/* SBS spec doesn't have a general health command. */
+		val->intval = POWER_SUPPLY_HEALTH_UNKNOWN;
+		return 0;
+	default:
+		dev_err(&client->dev, "unexpected prop %d\n", psp);
+		return -EINVAL;
+	}
+}
+
+static int sbs_get_ti_battery_presence_and_health(
+	struct i2c_client *client, enum power_supply_property psp,
+	union power_supply_propval *val)
 {
 	s32 ret;
 
@@ -600,7 +631,12 @@ static int sbs_get_property(struct power_supply *psy,
 	switch (psp) {
 	case POWER_SUPPLY_PROP_PRESENT:
 	case POWER_SUPPLY_PROP_HEALTH:
-		ret = sbs_get_battery_presence_and_health(client, psp, val);
+		if (client->flags & SBS_FLAGS_TI_BQ20Z75)
+			ret = sbs_get_ti_battery_presence_and_health(client,
+								     psp, val);
+		else
+			ret = sbs_get_battery_presence_and_health(client, psp,
+								  val);
 		if (psp == POWER_SUPPLY_PROP_PRESENT)
 			return 0;
 		break;
@@ -806,6 +842,7 @@ static int sbs_probe(struct i2c_client *client,
 	if (!chip)
 		return -ENOMEM;
 
+	chip->flags = (u32)(uintptr_t)of_device_get_match_data(&client->dev);
 	chip->client = client;
 	chip->enable_detection = false;
 	psy_cfg.of_node = client->dev.of_node;
@@ -915,12 +952,15 @@ static int sbs_suspend(struct device *dev)
 	if (chip->poll_time > 0)
 		cancel_delayed_work_sync(&chip->work);
 
-	/*
-	 * Write to manufacturer access with sleep command.
-	 * Support is manufacturer dependend, so ignore errors.
-	 */
-	sbs_write_word_data(client, sbs_data[REG_MANUFACTURER_DATA].addr,
-		MANUFACTURER_ACCESS_SLEEP);
+	if (chip->flags & SBS_FLAGS_TI_BQ20Z75) {
+		/*
+		 * Write to manufacturer access with sleep command.
+		 * Support is manufacturer dependent, so ignore errors.
+		 */
+		sbs_write_word_data(client,
+				    sbs_data[REG_MANUFACTURER_DATA].addr,
+				    MANUFACTURER_ACCESS_SLEEP);
+	}
 
 	return 0;
 }
@@ -941,7 +981,10 @@ MODULE_DEVICE_TABLE(i2c, sbs_id);
 
 static const struct of_device_id sbs_dt_ids[] = {
 	{ .compatible = "sbs,sbs-battery" },
-	{ .compatible = "ti,bq20z75" },
+	{
+		.compatible = "ti,bq20z75",
+		.data = (void *)SBS_FLAGS_TI_BQ20Z75,
+	},
 	{ }
 };
 MODULE_DEVICE_TABLE(of, sbs_dt_ids);
-- 
2.17.0.921.gf22659ad46-goog

[PATCH 1/2] power: supply: sbs-battery: don't assume MANUFACTURER_DATA formats

[Brian Norris]

This driver was originally submitted for the TI BQ20Z75 battery IC
(commit a7640bfa10c5 ("power_supply: Add driver for TI BQ20Z75 gas gauge
IC")) and later renamed to express generic SBS support. While it's
mostly true that this driver implemented a standard SBS command set, it
takes liberties with the REG_MANUFACTURER_DATA register. This register
is specified in the SBS spec, but it doesn't make any mention of what
its actual contents are.

We've sort of noticed this optionality previously, with commit
17c6d3979e5b ("sbs-battery: make writes to ManufacturerAccess
optional"), where we found that some batteries NAK writes to this
register.

What this really means is that so far, we've just been lucky that most
batteries have either been compatible with the TI chip, or else at least
haven't reported highly-unexpected values.

For instance, one battery I have here seems to report either 0x0000 or
0x0100 to the MANUFACTURER_ACCESS_STATUS command -- while this seems to
match either Wake Up (bits[11:8] = 0000b) or Normal Discharge
(bits[11:8] = 0001b) status for the TI part [1], they don't seem to
actually correspond to real states (for instance, I never see 0101b =
Charge, even when charging).

On other batteries, I'm getting apparently random data in return, which
means that occasionally, we interpret this as "battery not present" or
"battery is not healthy".

All in all, it seems to be a really bad idea to make assumptions about
REG_MANUFACTURER_DATA, unless we already know what battery we're using.
Therefore, this patch reimplements the "present" and "health" checks to
the following on most SBS batteries:

1. HEALTH: report "unknown" -- I couldn't find a standard SBS command
   that gives us much useful here
2. PRESENT: just send a REG_STATUS command; if it succeeds, then the
   battery is present

Also, we stop sending MANUFACTURER_ACCESS_SLEEP to non-TI parts. I have
no proof that this is useful and supported.

If someone explicitly provided a 'ti,bq20z75' compatible property, then
we retain the existing TI command behaviors.

[1] http://www.ti.com/lit/er/sluu265a/sluu265a.pdf

Signed-off-by: Brian Norris <briannorris@chromium.org>
---
 drivers/power/supply/sbs-battery.c | 61 +++++++++++++++++++++++++-----
 1 file changed, 52 insertions(+), 9 deletions(-)

diff --git a/drivers/power/supply/sbs-battery.c b/drivers/power/supply/sbs-battery.c
index 83d7b4115857..e15d0ca4729d 100644
--- a/drivers/power/supply/sbs-battery.c
+++ b/drivers/power/supply/sbs-battery.c
@@ -23,6 +23,7 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/of.h>
+#include <linux/of_device.h>
 #include <linux/power/sbs-battery.h>
 #include <linux/power_supply.h>
 #include <linux/slab.h>
@@ -84,8 +85,9 @@ static const struct chip_data {
 	int min_value;
 	int max_value;
 } sbs_data[] = {
+	/* Manuf. data isn't directly useful as a POWER_SUPPLY_PROP */
 	[REG_MANUFACTURER_DATA] =
-		SBS_DATA(POWER_SUPPLY_PROP_PRESENT, 0x00, 0, 65535),
+		SBS_DATA(-1, 0x00, 0, 65535),
 	[REG_TEMPERATURE] =
 		SBS_DATA(POWER_SUPPLY_PROP_TEMP, 0x08, 0, 65535),
 	[REG_VOLTAGE] =
@@ -156,6 +158,9 @@ static enum power_supply_property sbs_properties[] = {
 	POWER_SUPPLY_PROP_MODEL_NAME
 };
 
+/* Supports special manufacturer commands from TI BQ20Z75 IC. */
+#define SBS_FLAGS_TI_BQ20Z75		BIT(0)
+
 struct sbs_info {
 	struct i2c_client		*client;
 	struct power_supply		*power_supply;
@@ -168,6 +173,7 @@ struct sbs_info {
 	u32				poll_retry_count;
 	struct delayed_work		work;
 	struct mutex			mode_lock;
+	u32				flags;
 };
 
 static char model_name[I2C_SMBUS_BLOCK_MAX + 1];
@@ -315,6 +321,31 @@ static int sbs_status_correct(struct i2c_client *client, int *intval)
 static int sbs_get_battery_presence_and_health(
 	struct i2c_client *client, enum power_supply_property psp,
 	union power_supply_propval *val)
+{
+	int ret;
+
+	switch (psp) {
+	case POWER_SUPPLY_PROP_PRESENT:
+		/* Dummy command; if it succeeds, battery is present. */
+		ret = sbs_read_word_data(client, sbs_data[REG_STATUS].addr);
+		if (ret < 0)
+			val->intval = 0; /* battery disconnected */
+		else
+			val->intval = 1; /* battery present */
+		return 0;
+	case POWER_SUPPLY_PROP_HEALTH:
+		/* SBS spec doesn't have a general health command. */
+		val->intval = POWER_SUPPLY_HEALTH_UNKNOWN;
+		return 0;
+	default:
+		dev_err(&client->dev, "unexpected prop %d\n", psp);
+		return -EINVAL;
+	}
+}
+
+static int sbs_get_ti_battery_presence_and_health(
+	struct i2c_client *client, enum power_supply_property psp,
+	union power_supply_propval *val)
 {
 	s32 ret;
 
@@ -600,7 +631,12 @@ static int sbs_get_property(struct power_supply *psy,
 	switch (psp) {
 	case POWER_SUPPLY_PROP_PRESENT:
 	case POWER_SUPPLY_PROP_HEALTH:
-		ret = sbs_get_battery_presence_and_health(client, psp, val);
+		if (client->flags & SBS_FLAGS_TI_BQ20Z75)
+			ret = sbs_get_ti_battery_presence_and_health(client,
+								     psp, val);
+		else
+			ret = sbs_get_battery_presence_and_health(client, psp,
+								  val);
 		if (psp == POWER_SUPPLY_PROP_PRESENT)
 			return 0;
 		break;
@@ -806,6 +842,7 @@ static int sbs_probe(struct i2c_client *client,
 	if (!chip)
 		return -ENOMEM;
 
+	chip->flags = (u32)(uintptr_t)of_device_get_match_data(&client->dev);
 	chip->client = client;
 	chip->enable_detection = false;
 	psy_cfg.of_node = client->dev.of_node;
@@ -915,12 +952,15 @@ static int sbs_suspend(struct device *dev)
 	if (chip->poll_time > 0)
 		cancel_delayed_work_sync(&chip->work);
 
-	/*
-	 * Write to manufacturer access with sleep command.
-	 * Support is manufacturer dependend, so ignore errors.
-	 */
-	sbs_write_word_data(client, sbs_data[REG_MANUFACTURER_DATA].addr,
-		MANUFACTURER_ACCESS_SLEEP);
+	if (chip->flags & SBS_FLAGS_TI_BQ20Z75) {
+		/*
+		 * Write to manufacturer access with sleep command.
+		 * Support is manufacturer dependent, so ignore errors.
+		 */
+		sbs_write_word_data(client,
+				    sbs_data[REG_MANUFACTURER_DATA].addr,
+				    MANUFACTURER_ACCESS_SLEEP);
+	}
 
 	return 0;
 }
@@ -941,7 +981,10 @@ MODULE_DEVICE_TABLE(i2c, sbs_id);
 
 static const struct of_device_id sbs_dt_ids[] = {
 	{ .compatible = "sbs,sbs-battery" },
-	{ .compatible = "ti,bq20z75" },
+	{
+		.compatible = "ti,bq20z75",
+		.data = (void *)SBS_FLAGS_TI_BQ20Z75,
+	},
 	{ }
 };
 MODULE_DEVICE_TABLE(of, sbs_dt_ids);
-- 
2.17.0.921.gf22659ad46-goog