[PATCH 0/4] power: supply: sbs-battery: Add delay when changing capacity mode

[PATCH 0/4] power: supply: sbs-battery: Add delay when changing capacity mode

[Phil Reid]

This is based on Michael Heinemann's patch:
"sbs-battery: correct capacity mode selection bits"

First two are housing keep / cleanup.
As reported by Michael with his patch he found the mode didn't always change.
I found adding a delay fixed the problem for several Insipered Energy SBS 
compatible batteries I have on hand.

The last patch may need some comment and testing by others.

The SBS spec says that capcaty mode bit:

Changes the return type for the following registers:
  RemainingCapacityAlarm()
  AtRate()
  RemainingCapacity()
  FullChargeCapacity()
  DesignCapacity()

and may affect the following calculations:
  AtRateOK()
  AtRateTimeToEmpty()
  RunTimeToEmpty()
  AverageTimeToEmpty()
  RemainingTimeAlarm()
  BatteryStatus()
  Optional: AtRateTimeToFull()

So far I wasn't able to detect any difference in say the AverageTimeTEmpty(). 
But if we are reseting the bit after the watt reading then perhaps it should 
be reset to the power up value (ie cleared) instead of reset to some other value.

I'm not sure what the best approach is here. 



Phil Reid (4):
  power: supply: sbs-battery: Remove FSF mailing address from comments
  power: supply: sbs-battery: sort includes
  power: supply: sbs-battery: Add delay when changing capacity mode bit
  power: supply: sbs-battery: Don't reset mode in get_battery_capacity

 drivers/power/supply/sbs-battery.c | 55 ++++++++++++++++----------------------
 1 file changed, 23 insertions(+), 32 deletions(-)

-- 
1.8.3.1

[PATCH 1/4] power: supply: sbs-battery: Remove FSF mailing address from comments

[Phil Reid]

checkpatch issued an error in having the FSF address in the comment.
As address may change and Linux already includes a copy.

Signed-off-by: Phil Reid <preid@electromag.com.au>
---
 drivers/power/supply/sbs-battery.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/power/supply/sbs-battery.c b/drivers/power/supply/sbs-battery.c
index f7351e0..15873df 100644
--- a/drivers/power/supply/sbs-battery.c
+++ b/drivers/power/supply/sbs-battery.c
@@ -12,10 +12,6 @@
  * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
  * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
  * more details.
- *
- * You should have received a copy of the GNU General Public License along
- * with this program; if not, write to the Free Software Foundation, Inc.,
- * 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
  */
 
 #include <linux/init.h>
-- 
1.8.3.1

[PATCH 2/4] power: supply: sbs-battery: sort includes

[Phil Reid]

Sort the header includes prior to adding to the list.

Signed-off-by: Phil Reid <preid@electromag.com.au>
---
 drivers/power/supply/sbs-battery.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/drivers/power/supply/sbs-battery.c b/drivers/power/supply/sbs-battery.c
index 15873df..4594ed1 100644
--- a/drivers/power/supply/sbs-battery.c
+++ b/drivers/power/supply/sbs-battery.c
@@ -14,19 +14,18 @@
  * more details.
  */
 
-#include <linux/init.h>
-#include <linux/module.h>
-#include <linux/kernel.h>
 #include <linux/err.h>
-#include <linux/power_supply.h>
+#include <linux/gpio/consumer.h>
 #include <linux/i2c.h>
-#include <linux/slab.h>
+#include <linux/init.h>
 #include <linux/interrupt.h>
-#include <linux/gpio/consumer.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
 #include <linux/of.h>
-#include <linux/stat.h>
-
 #include <linux/power/sbs-battery.h>
+#include <linux/power_supply.h>
+#include <linux/slab.h>
+#include <linux/stat.h>
 
 enum {
 	REG_MANUFACTURER_DATA,
@@ -57,7 +56,7 @@ enum {
 #define BATTERY_MODE_MASK		0x8000
 enum sbs_battery_mode {
 	BATTERY_MODE_AMPS = 0,
-	BATTERY_MODE_WATTS = 0x800
+	BATTERY_MODE_WATTS = 0x8000
 };
 
 /* manufacturer access defines */
-- 
1.8.3.1

Re: [PATCH 2/4] power: supply: sbs-battery: sort includes

[Michael Heinemann]

On 2017-07-10 08:23, Phil Reid wrote:
> Sort the header includes prior to adding to the list.
> 
> Signed-off-by: Phil Reid <preid@electromag.com.au>
> ---
>  drivers/power/supply/sbs-battery.c | 17 ++++++++---------
>  1 file changed, 8 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/power/supply/sbs-battery.c
> b/drivers/power/supply/sbs-battery.c
> index 15873df..4594ed1 100644
> --- a/drivers/power/supply/sbs-battery.c
> +++ b/drivers/power/supply/sbs-battery.c
> @@ -14,19 +14,18 @@
>   * more details.
>   */
> 
> -#include <linux/init.h>
> -#include <linux/module.h>
> -#include <linux/kernel.h>
>  #include <linux/err.h>
> -#include <linux/power_supply.h>
> +#include <linux/gpio/consumer.h>
>  #include <linux/i2c.h>
> -#include <linux/slab.h>
> +#include <linux/init.h>
>  #include <linux/interrupt.h>
> -#include <linux/gpio/consumer.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
>  #include <linux/of.h>
> -#include <linux/stat.h>
> -
>  #include <linux/power/sbs-battery.h>
> +#include <linux/power_supply.h>
> +#include <linux/slab.h>
> +#include <linux/stat.h>
> 
>  enum {
>  	REG_MANUFACTURER_DATA,
> @@ -57,7 +56,7 @@ enum {
>  #define BATTERY_MODE_MASK		0x8000
>  enum sbs_battery_mode {
>  	BATTERY_MODE_AMPS = 0,
> -	BATTERY_MODE_WATTS = 0x800
> +	BATTERY_MODE_WATTS = 0x8000
>  };

I guess the second chunk is in there by accident.

> 
>  /* manufacturer access defines */

Re: [PATCH 2/4] power: supply: sbs-battery: sort includes

[Phil Reid]

On 11/07/2017 16:37, Michael Heinemann wrote:
> On 2017-07-10 08:23, Phil Reid wrote:
>> Sort the header includes prior to adding to the list.
>>
>> Signed-off-by: Phil Reid <preid@electromag.com.au>
>> ---
>>  drivers/power/supply/sbs-battery.c | 17 ++++++++---------
>>  1 file changed, 8 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/power/supply/sbs-battery.c
>> b/drivers/power/supply/sbs-battery.c
>> index 15873df..4594ed1 100644
>> --- a/drivers/power/supply/sbs-battery.c
>> +++ b/drivers/power/supply/sbs-battery.c
>> @@ -14,19 +14,18 @@
>>   * more details.
>>   */
>>
>> -#include <linux/init.h>
>> -#include <linux/module.h>
>> -#include <linux/kernel.h>
>>  #include <linux/err.h>
>> -#include <linux/power_supply.h>
>> +#include <linux/gpio/consumer.h>
>>  #include <linux/i2c.h>
>> -#include <linux/slab.h>
>> +#include <linux/init.h>
>>  #include <linux/interrupt.h>
>> -#include <linux/gpio/consumer.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>>  #include <linux/of.h>
>> -#include <linux/stat.h>
>> -
>>  #include <linux/power/sbs-battery.h>
>> +#include <linux/power_supply.h>
>> +#include <linux/slab.h>
>> +#include <linux/stat.h>
>>
>>  enum {
>>      REG_MANUFACTURER_DATA,
>> @@ -57,7 +56,7 @@ enum {
>>  #define BATTERY_MODE_MASK        0x8000
>>  enum sbs_battery_mode {
>>      BATTERY_MODE_AMPS = 0,
>> -    BATTERY_MODE_WATTS = 0x800
>> +    BATTERY_MODE_WATTS = 0x8000
>>  };
> 
> I guess the second chunk is in there by accident.
> 
>>
>>  /* manufacturer access defines */
> 
> 
Damn, Thought I'd fixed that before sending the email

-- 
Regards
Phil Reid

[PATCH 3/4] power: supply: sbs-battery: Add delay when changing capacity mode bit

[Phil Reid]

At least with the Inspired Energy compatible batteries a delay is required
after setting the capacity mode bit from amp to watts or the reverse.
Setting the bit and then immediately pooling the status register results
in an unknown error being returned in the register. Add the delay results
in and ok status being return. This was also seen when reading the charge
and energy registers where the wrong value was returned for the requested
mode.

Signed-off-by: Phil Reid <preid@electromag.com.au>
---
 drivers/power/supply/sbs-battery.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/power/supply/sbs-battery.c b/drivers/power/supply/sbs-battery.c
index 4594ed1..3473b45 100644
--- a/drivers/power/supply/sbs-battery.c
+++ b/drivers/power/supply/sbs-battery.c
@@ -14,6 +14,7 @@
  * more details.
  */
 
+#include <linux/delay.h>
 #include <linux/err.h>
 #include <linux/gpio/consumer.h>
 #include <linux/i2c.h>
@@ -511,6 +512,8 @@ static enum sbs_battery_mode sbs_set_battery_mode(struct i2c_client *client,
 	if (ret < 0)
 		return ret;
 
+	usleep_range(1000, 2000);
+
 	return original_val & BATTERY_MODE_MASK;
 }
 
-- 
1.8.3.1

[PATCH 4/4] power: supply: sbs-battery: Don't reset mode in get_battery_capacity

[Phil Reid]

There seems little point in resetting the capacity mode bit when
reading a capacity register that is affected by this bit. The
next call to the register read will set the bit if it's not in
the correct mode.

Signed-off-by: Phil Reid <preid@electromag.com.au>
---
 drivers/power/supply/sbs-battery.c | 31 ++++++++++++-------------------
 1 file changed, 12 insertions(+), 19 deletions(-)

diff --git a/drivers/power/supply/sbs-battery.c b/drivers/power/supply/sbs-battery.c
index 3473b45..441a576 100644
--- a/drivers/power/supply/sbs-battery.c
+++ b/drivers/power/supply/sbs-battery.c
@@ -491,22 +491,19 @@ static void  sbs_unit_adjustment(struct i2c_client *client,
 	}
 }
 
-static enum sbs_battery_mode sbs_set_battery_mode(struct i2c_client *client,
+static int sbs_set_battery_mode(struct i2c_client *client,
 	enum sbs_battery_mode mode)
 {
-	int ret, original_val;
+	int ret;
 
-	original_val = sbs_read_word_data(client, BATTERY_MODE_OFFSET);
-	if (original_val < 0)
-		return original_val;
+	ret = sbs_read_word_data(client, BATTERY_MODE_OFFSET);
+	if (ret < 0)
+		return ret;
 
-	if ((original_val & BATTERY_MODE_MASK) == mode)
-		return mode;
+	if ((ret & BATTERY_MODE_MASK) == mode)
+		return 0;
 
-	if (mode == BATTERY_MODE_AMPS)
-		ret = original_val & ~BATTERY_MODE_MASK;
-	else
-		ret = original_val | BATTERY_MODE_MASK;
+	ret = (ret & ~BATTERY_MODE_MASK) | mode;
 
 	ret = sbs_write_word_data(client, BATTERY_MODE_OFFSET, ret);
 	if (ret < 0)
@@ -514,7 +511,7 @@ static enum sbs_battery_mode sbs_set_battery_mode(struct i2c_client *client,
 
 	usleep_range(1000, 2000);
 
-	return original_val & BATTERY_MODE_MASK;
+	return 0;
 }
 
 static int sbs_get_battery_capacity(struct i2c_client *client,
@@ -527,9 +524,9 @@ static int sbs_get_battery_capacity(struct i2c_client *client,
 	if (power_supply_is_amp_property(psp))
 		mode = BATTERY_MODE_AMPS;
 
-	mode = sbs_set_battery_mode(client, mode);
-	if (mode < 0)
-		return mode;
+	ret = sbs_set_battery_mode(client, mode);
+	if (ret < 0)
+		return ret;
 
 	ret = sbs_read_word_data(client, sbs_data[reg_offset].addr);
 	if (ret < 0)
@@ -542,10 +539,6 @@ static int sbs_get_battery_capacity(struct i2c_client *client,
 	} else
 		val->intval = ret;
 
-	ret = sbs_set_battery_mode(client, mode);
-	if (ret < 0)
-		return ret;
-
 	return 0;
 }
 
-- 
1.8.3.1

Re: [PATCH 4/4] power: supply: sbs-battery: Don't reset mode in get_battery_capacity

[Phil Reid]

On 10/07/2017 14:23, Phil Reid wrote:
> There seems little point in resetting the capacity mode bit when
> reading a capacity register that is affected by this bit. The
> next call to the register read will set the bit if it's not in
> the correct mode.
> 

Thinking about this a bit more and I don't think this is a good idea.
Reading the spec again I'm guessing the runtime till empty calcs will
be different based on this mode bit.

ie:
in uW mode, the spec is making an assumption of constant power, ie Switch Mode regulator load.
in mA mode, the spec is make an assumption of constant current, ie linear regulator load

What it probably needs is a mode selection option and set that at probe or when a battery is detected.

I'm not sure what impact changing the mode has to data calcs that uses averaging.

Please drop this patch for now.



> Signed-off-by: Phil Reid <preid@electromag.com.au>
> ---
>   drivers/power/supply/sbs-battery.c | 31 ++++++++++++-------------------
>   1 file changed, 12 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/power/supply/sbs-battery.c b/drivers/power/supply/sbs-battery.c
> index 3473b45..441a576 100644
> --- a/drivers/power/supply/sbs-battery.c
> +++ b/drivers/power/supply/sbs-battery.c
> @@ -491,22 +491,19 @@ static void  sbs_unit_adjustment(struct i2c_client *client,
>   	}
>   }
>   
> -static enum sbs_battery_mode sbs_set_battery_mode(struct i2c_client *client,
> +static int sbs_set_battery_mode(struct i2c_client *client,
>   	enum sbs_battery_mode mode)
>   {
> -	int ret, original_val;
> +	int ret;
>   
> -	original_val = sbs_read_word_data(client, BATTERY_MODE_OFFSET);
> -	if (original_val < 0)
> -		return original_val;
> +	ret = sbs_read_word_data(client, BATTERY_MODE_OFFSET);
> +	if (ret < 0)
> +		return ret;
>   
> -	if ((original_val & BATTERY_MODE_MASK) == mode)
> -		return mode;
> +	if ((ret & BATTERY_MODE_MASK) == mode)
> +		return 0;
>   
> -	if (mode == BATTERY_MODE_AMPS)
> -		ret = original_val & ~BATTERY_MODE_MASK;
> -	else
> -		ret = original_val | BATTERY_MODE_MASK;
> +	ret = (ret & ~BATTERY_MODE_MASK) | mode;
>   
>   	ret = sbs_write_word_data(client, BATTERY_MODE_OFFSET, ret);
>   	if (ret < 0)
> @@ -514,7 +511,7 @@ static enum sbs_battery_mode sbs_set_battery_mode(struct i2c_client *client,
>   
>   	usleep_range(1000, 2000);
>   
> -	return original_val & BATTERY_MODE_MASK;
> +	return 0;
>   }
>   
>   static int sbs_get_battery_capacity(struct i2c_client *client,
> @@ -527,9 +524,9 @@ static int sbs_get_battery_capacity(struct i2c_client *client,
>   	if (power_supply_is_amp_property(psp))
>   		mode = BATTERY_MODE_AMPS;
>   
> -	mode = sbs_set_battery_mode(client, mode);
> -	if (mode < 0)
> -		return mode;
> +	ret = sbs_set_battery_mode(client, mode);
> +	if (ret < 0)
> +		return ret;
>   
>   	ret = sbs_read_word_data(client, sbs_data[reg_offset].addr);
>   	if (ret < 0)
> @@ -542,10 +539,6 @@ static int sbs_get_battery_capacity(struct i2c_client *client,
>   	} else
>   		val->intval = ret;
>   
> -	ret = sbs_set_battery_mode(client, mode);
> -	if (ret < 0)
> -		return ret;
> -
>   	return 0;
>   }
>   
> 


-- 
Regards
Phil Reid

Re: [PATCH 4/4] power: supply: sbs-battery: Don't reset mode in get_battery_capacity

[Michael Heinemann]

On 2017-07-11 06:57, Phil Reid wrote:
> On 10/07/2017 14:23, Phil Reid wrote:
>> There seems little point in resetting the capacity mode bit when
>> reading a capacity register that is affected by this bit. The
>> next call to the register read will set the bit if it's not in
>> the correct mode.
>> 
> 
> Thinking about this a bit more and I don't think this is a good idea.
> Reading the spec again I'm guessing the runtime till empty calcs will
> be different based on this mode bit.
> 
> ie:
> in uW mode, the spec is making an assumption of constant power, ie
> Switch Mode regulator load.
> in mA mode, the spec is make an assumption of constant current, ie
> linear regulator load
> 
> What it probably needs is a mode selection option and set that at
> probe or when a battery is detected.
> 
> I'm not sure what impact changing the mode has to data calcs that uses
> averaging.
> 
> Please drop this patch for now.
> 

I agree that changing the mode implictly might lead to trouble. So
getting back to the initial mode might be wise to do.

I applied your patches (without Patch 4/4) and made the following
observation:

Coming fresh from the charger the battery is in Amps mode.
But sometimes it is suddenly in Watts mode. I guess switching the
mode back failed at some point.
So maybe it would make sense to define a default mode to which it is
always set back. This might also be configurable (devicetree?). But
for a Battery I guess Amps should be the default Mode.

(Actually it confused me intially that WATTS mode is handled as the
default from code perspective.)

> 
> 
>> Signed-off-by: Phil Reid <preid@electromag.com.au>
>> ---
>>   drivers/power/supply/sbs-battery.c | 31 
>> ++++++++++++-------------------
>>   1 file changed, 12 insertions(+), 19 deletions(-)
>> 
>> diff --git a/drivers/power/supply/sbs-battery.c 
>> b/drivers/power/supply/sbs-battery.c
>> index 3473b45..441a576 100644
>> --- a/drivers/power/supply/sbs-battery.c
>> +++ b/drivers/power/supply/sbs-battery.c
>> @@ -491,22 +491,19 @@ static void  sbs_unit_adjustment(struct 
>> i2c_client *client,
>>   	}
>>   }
>>   -static enum sbs_battery_mode sbs_set_battery_mode(struct i2c_client 
>> *client,
>> +static int sbs_set_battery_mode(struct i2c_client *client,
>>   	enum sbs_battery_mode mode)
>>   {
>> -	int ret, original_val;
>> +	int ret;
>>   -	original_val = sbs_read_word_data(client, BATTERY_MODE_OFFSET);
>> -	if (original_val < 0)
>> -		return original_val;
>> +	ret = sbs_read_word_data(client, BATTERY_MODE_OFFSET);
>> +	if (ret < 0)
>> +		return ret;
>>   -	if ((original_val & BATTERY_MODE_MASK) == mode)
>> -		return mode;
>> +	if ((ret & BATTERY_MODE_MASK) == mode)
>> +		return 0;
>>   -	if (mode == BATTERY_MODE_AMPS)
>> -		ret = original_val & ~BATTERY_MODE_MASK;
>> -	else
>> -		ret = original_val | BATTERY_MODE_MASK;
>> +	ret = (ret & ~BATTERY_MODE_MASK) | mode;
>>     	ret = sbs_write_word_data(client, BATTERY_MODE_OFFSET, ret);
>>   	if (ret < 0)
>> @@ -514,7 +511,7 @@ static enum sbs_battery_mode 
>> sbs_set_battery_mode(struct i2c_client *client,
>>     	usleep_range(1000, 2000);
>>   -	return original_val & BATTERY_MODE_MASK;
>> +	return 0;
>>   }
>>     static int sbs_get_battery_capacity(struct i2c_client *client,
>> @@ -527,9 +524,9 @@ static int sbs_get_battery_capacity(struct 
>> i2c_client *client,
>>   	if (power_supply_is_amp_property(psp))
>>   		mode = BATTERY_MODE_AMPS;
>>   -	mode = sbs_set_battery_mode(client, mode);
>> -	if (mode < 0)
>> -		return mode;
>> +	ret = sbs_set_battery_mode(client, mode);
>> +	if (ret < 0)
>> +		return ret;
>>     	ret = sbs_read_word_data(client, sbs_data[reg_offset].addr);
>>   	if (ret < 0)
>> @@ -542,10 +539,6 @@ static int sbs_get_battery_capacity(struct 
>> i2c_client *client,
>>   	} else
>>   		val->intval = ret;
>>   -	ret = sbs_set_battery_mode(client, mode);
>> -	if (ret < 0)
>> -		return ret;
>> -
>>   	return 0;
>>   }
>> 

Regards,
Michael