Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH 1/2] power: supply: sbs-battery: don't assume
 MANUFACTURER_DATA formats
To:     Brian Norris <briannorris@chromium.org>,
        Sebastian Reichel <sre@kernel.org>
Cc:     linux-kernel@vger.kernel.org, Rob Herring <robh+dt@kernel.org>,
        linux-pm@vger.kernel.org, devicetree@vger.kernel.org,
        Alexandru Stan <amstan@chromium.org>,
        Doug Anderson <dianders@chromium.org>
References: <20180601003252.42810-1-briannorris@chromium.org>
From:   Guenter Roeck <linux@roeck-us.net>
Message-ID: <0b2a6fde-4b5a-bccb-d6bf-18a1feac7632@roeck-us.net>
Date:   Thu, 31 May 2018 18:20:27 -0700
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.8.0
MIME-Version: 1.0
In-Reply-To: <20180601003252.42810-1-briannorris@chromium.org>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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