[PATCHv2 0/2] SBS battery PEC support

[PATCHv2 0/2] SBS battery PEC support

[Sebastian Reichel]

Hi,

Second try to enable PEC for SBS battery. Mainline currently
has 3 different platforms using sbs-battery with an I2C driver
not implementing I2C_M_RECV_LEN:

 * i2c-exynos5
 * i2c-rk3x
 * i2c-tegra

On those platforms PEC will be temporarly disabled for SBS
functions requesting strings. I considered moving the emulation
to I2C core, but it's specific to the SBS battery. The hack
only works because the strings are constant.

Changes since PATCHv1:
 * dropped all applied changes
 * rebased on top of power-supply's for-next branch
 * keep existing string reading method as fallback

-- Sebastian

Sebastian Reichel (2):
  power: supply: sbs-battery: use i2c_smbus_read_block_data()
  power: supply: sbs-battery: add PEC support"

 drivers/power/supply/sbs-battery.c | 89 +++++++++++++++++++++++++++++-
 1 file changed, 87 insertions(+), 2 deletions(-)

-- 
2.26.2

[PATCHv2 1/2] power: supply: sbs-battery: use i2c_smbus_read_block_data()

[Sebastian Reichel]

The SBS battery implements SMBus block reads. Currently the
driver "emulates" this by doing an I2C byte read for the
length followed by an I2C block read. The I2C subsystem
actually provides a proper API for doing SMBus block reads,
which can and should be used instead. The current implementation
does not properly handle packet error checking (PEC).

Not all upstream systems using sbs-battery have I2C bus drivers
supporting I2C_M_RECV_LEN, so old implementation is kept as
fallback to keep things working. But this prints a warning,
which hopefully results in people implementing support for it.

Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
 drivers/power/supply/sbs-battery.c | 31 ++++++++++++++++++++++++++++--
 1 file changed, 29 insertions(+), 2 deletions(-)

diff --git a/drivers/power/supply/sbs-battery.c b/drivers/power/supply/sbs-battery.c
index 83b9924033bd..74221b9279a9 100644
--- a/drivers/power/supply/sbs-battery.c
+++ b/drivers/power/supply/sbs-battery.c
@@ -263,8 +263,7 @@ static int sbs_read_word_data(struct i2c_client *client, u8 address)
 	return ret;
 }
 
-static int sbs_read_string_data(struct i2c_client *client, u8 address,
-				char *values)
+static int sbs_read_string_data_fallback(struct i2c_client *client, u8 address, char *values)
 {
 	struct sbs_info *chip = i2c_get_clientdata(client);
 	s32 ret = 0, block_length = 0;
@@ -274,6 +273,8 @@ static int sbs_read_string_data(struct i2c_client *client, u8 address,
 	retries_length = chip->i2c_retry_count;
 	retries_block = chip->i2c_retry_count;
 
+	dev_warn_once(&client->dev, "I2C adapter does not support I2C_FUNC_SMBUS_READ_BLOCK_DATA.\n");
+
 	/* Adapter needs to support these two functions */
 	if (!i2c_check_functionality(client->adapter,
 				     I2C_FUNC_SMBUS_BYTE_DATA |
@@ -329,6 +330,32 @@ static int sbs_read_string_data(struct i2c_client *client, u8 address,
 	return ret;
 }
 
+static int sbs_read_string_data(struct i2c_client *client, u8 address, char *values)
+{
+	struct sbs_info *chip = i2c_get_clientdata(client);
+	int retries = chip->i2c_retry_count;
+	int ret = 0;
+
+	if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_READ_BLOCK_DATA))
+		return sbs_read_string_data_fallback(client, address, values);
+
+	while (retries > 0) {
+		ret = i2c_smbus_read_block_data(client, address, values);
+		if (ret >= 0)
+			break;
+		retries--;
+	}
+
+	if (ret < 0) {
+		dev_dbg(&client->dev, "failed to read block 0x%x: %d\n", address, ret);
+		return ret;
+	}
+
+	/* add string termination */
+	values[ret] = '\0';
+	return ret;
+}
+
 static int sbs_write_word_data(struct i2c_client *client, u8 address,
 	u16 value)
 {
-- 
2.26.2

[PATCHv2 2/2] power: supply: sbs-battery: add PEC support

[Sebastian Reichel]

SBS batteries optionally have support for PEC. This enables
PEC handling based on the implemented SBS version as suggested
by the standard. The support for PEC is re-evaluated when the
battery is hotplugged into the system, since there might be
systems supporting batteries from different SBS generations.

Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
 drivers/power/supply/sbs-battery.c | 64 ++++++++++++++++++++++++++++--
 1 file changed, 61 insertions(+), 3 deletions(-)

diff --git a/drivers/power/supply/sbs-battery.c b/drivers/power/supply/sbs-battery.c
index 74221b9279a9..49c3508a6b79 100644
--- a/drivers/power/supply/sbs-battery.c
+++ b/drivers/power/supply/sbs-battery.c
@@ -51,6 +51,14 @@ enum {
 	REG_CHARGE_VOLTAGE,
 };
 
+#define REG_ADDR_SPEC_INFO		0x1A
+#define SPEC_INFO_VERSION_MASK		GENMASK(7, 4)
+#define SPEC_INFO_VERSION_SHIFT		4
+
+#define SBS_VERSION_1_0			1
+#define SBS_VERSION_1_1			2
+#define SBS_VERSION_1_1_WITH_PEC	3
+
 #define REG_ADDR_MANUFACTURE_DATE	0x1B
 
 /* Battery Mode defines */
@@ -224,14 +232,57 @@ static void sbs_disable_charger_broadcasts(struct sbs_info *chip)
 
 static int sbs_update_presence(struct sbs_info *chip, bool is_present)
 {
+	struct i2c_client *client = chip->client;
+	int retries = chip->i2c_retry_count;
+	s32 ret = 0;
+	u8 version;
+
 	if (chip->is_present == is_present)
 		return 0;
 
 	if (!is_present) {
 		chip->is_present = false;
+		/* Disable PEC when no device is present */
+		client->flags &= ~I2C_CLIENT_PEC;
 		return 0;
 	}
 
+	/* Check if device supports packet error checking and use it */
+	while (retries > 0) {
+		ret = i2c_smbus_read_word_data(client, REG_ADDR_SPEC_INFO);
+		if (ret >= 0)
+			break;
+
+		/*
+		 * Some batteries trigger the detection pin before the
+		 * I2C bus is properly connected. This works around the
+		 * issue.
+		 */
+		msleep(100);
+
+		retries--;
+	}
+
+	if (ret < 0) {
+		dev_dbg(&client->dev, "failed to read spec info: %d\n", ret);
+
+		/* fallback to old behaviour */
+		client->flags &= ~I2C_CLIENT_PEC;
+		chip->is_present = true;
+
+		return ret;
+	}
+
+	version = (ret & SPEC_INFO_VERSION_MASK) >> SPEC_INFO_VERSION_SHIFT;
+
+	if (version == SBS_VERSION_1_1_WITH_PEC)
+		client->flags |= I2C_CLIENT_PEC;
+	else
+		client->flags &= ~I2C_CLIENT_PEC;
+
+	dev_dbg(&client->dev, "PEC: %s\n", (client->flags & I2C_CLIENT_PEC) ?
+		"enabled" : "disabled");
+
 	if (!chip->is_present && is_present && !chip->charger_broadcasts)
 		sbs_disable_charger_broadcasts(chip);
 
@@ -273,7 +324,8 @@ static int sbs_read_string_data_fallback(struct i2c_client *client, u8 address,
 	retries_length = chip->i2c_retry_count;
 	retries_block = chip->i2c_retry_count;
 
-	dev_warn_once(&client->dev, "I2C adapter does not support I2C_FUNC_SMBUS_READ_BLOCK_DATA.\n");
+	dev_warn_once(&client->dev, "I2C adapter does not support I2C_FUNC_SMBUS_READ_BLOCK_DATA.\n"
+				    "Fallback method does not support PEC.\n");
 
 	/* Adapter needs to support these two functions */
 	if (!i2c_check_functionality(client->adapter,
@@ -336,8 +388,14 @@ static int sbs_read_string_data(struct i2c_client *client, u8 address, char *val
 	int retries = chip->i2c_retry_count;
 	int ret = 0;
 
-	if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_READ_BLOCK_DATA))
-		return sbs_read_string_data_fallback(client, address, values);
+	if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_READ_BLOCK_DATA)) {
+		bool pec = client->flags & I2C_CLIENT_PEC;
+		client->flags &= ~I2C_CLIENT_PEC;
+		ret = sbs_read_string_data_fallback(client, address, values);
+		if (pec)
+			client->flags |= I2C_CLIENT_PEC;
+		return ret;
+	}
 
 	while (retries > 0) {
 		ret = i2c_smbus_read_block_data(client, address, values);
-- 
2.26.2

Re: [PATCHv2 0/2] SBS battery PEC support

[Marek Szyprowski]

Hi Sebastian,

On 06.06.2020 01:06, Sebastian Reichel wrote:
> Second try to enable PEC for SBS battery. Mainline currently
> has 3 different platforms using sbs-battery with an I2C driver
> not implementing I2C_M_RECV_LEN:
>
>   * i2c-exynos5
>   * i2c-rk3x
>   * i2c-tegra
>
> On those platforms PEC will be temporarly disabled for SBS
> functions requesting strings. I considered moving the emulation
> to I2C core, but it's specific to the SBS battery. The hack
> only works because the strings are constant.

Thanks for the update. Works fine on Samsung Exynos-based Chromebooks: 
Snow, Peach-Pit and Peach-Pi.

Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>

> Changes since PATCHv1:
>   * dropped all applied changes
>   * rebased on top of power-supply's for-next branch
>   * keep existing string reading method as fallback
>
> -- Sebastian
>
> Sebastian Reichel (2):
>    power: supply: sbs-battery: use i2c_smbus_read_block_data()
>    power: supply: sbs-battery: add PEC support"
>
>   drivers/power/supply/sbs-battery.c | 89 +++++++++++++++++++++++++++++-
>   1 file changed, 87 insertions(+), 2 deletions(-)
>
Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCHv2 0/2] SBS battery PEC support

[Sebastian Reichel]

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

Hi,

On Mon, Jun 08, 2020 at 08:23:05AM +0200, Marek Szyprowski wrote:
> Hi Sebastian,
> 
> On 06.06.2020 01:06, Sebastian Reichel wrote:
> > Second try to enable PEC for SBS battery. Mainline currently
> > has 3 different platforms using sbs-battery with an I2C driver
> > not implementing I2C_M_RECV_LEN:
> >
> >   * i2c-exynos5
> >   * i2c-rk3x
> >   * i2c-tegra
> >
> > On those platforms PEC will be temporarly disabled for SBS
> > functions requesting strings. I considered moving the emulation
> > to I2C core, but it's specific to the SBS battery. The hack
> > only works because the strings are constant.
> 
> Thanks for the update. Works fine on Samsung Exynos-based Chromebooks: 
> Snow, Peach-Pit and Peach-Pi.
> 
> Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>

Thanks, queued.

-- Sebastian

> > Changes since PATCHv1:
> >   * dropped all applied changes
> >   * rebased on top of power-supply's for-next branch
> >   * keep existing string reading method as fallback
> >
> > -- Sebastian
> >
> > Sebastian Reichel (2):
> >    power: supply: sbs-battery: use i2c_smbus_read_block_data()
> >    power: supply: sbs-battery: add PEC support"
> >
> >   drivers/power/supply/sbs-battery.c | 89 +++++++++++++++++++++++++++++-
> >   1 file changed, 87 insertions(+), 2 deletions(-)

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