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

[PATCH v2 0/3] 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.

Changes from v2:
- power: supply: sbs-battery: sort includes
  - Remove errousnous chunk
- power: supply: sbs-battery: Don't reset mode in get_battery_capacity
  - drop patch as it needs more thought

Phil Reid (3):
  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

 drivers/power/supply/sbs-battery.c | 22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

-- 
1.8.3.1

[PATCH v2 1/3] 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 a7150a6..d06c4ac 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 v2 2/3] 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 | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/power/supply/sbs-battery.c b/drivers/power/supply/sbs-battery.c
index d06c4ac..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,
-- 
1.8.3.1

[PATCH v2 3/3] 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

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

[Michael Heinemann]

On 2017-07-11 11:43, Phil Reid wrote:
> 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>

Tested-by: Michael Heinemann <committed@heine.so>

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

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

[Phil Reid]

On 11/07/2017 19:31, Michael Heinemann wrote:
> On 2017-07-11 11:43, Phil Reid wrote:
>> 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>
> 
> Tested-by: Michael Heinemann <committed@heine.so>

I received some feedback from inspired energy on this and they confirmed they need a
delay between smbus transactions for their batteries.
This is a deviation from the standard for their batteries only.

Perhaps this should delay should only be added if we detect and inspired energy
battery. Could look at the manufacturer field to determine whether to add it.

WDYR?


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


-- 
Regards
Phil Reid

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

[Sebastian Reichel]

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

Hi,

On Tue, Jul 11, 2017 at 05:43:40PM +0800, Phil Reid wrote:
> 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.
> 
> Changes from v2:
> - power: supply: sbs-battery: sort includes
>   - Remove errousnous chunk
> - power: supply: sbs-battery: Don't reset mode in get_battery_capacity
>   - drop patch as it needs more thought
> 
> Phil Reid (3):
>   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
> 
>  drivers/power/supply/sbs-battery.c | 22 ++++++++++------------
>  1 file changed, 10 insertions(+), 12 deletions(-)

Thanks, whole patchset queued to power-supply's for-next branch.

-- Sebastian

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