linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/3] power: supply: max17040: Correct voltage reading
       [not found] <20200504221300.3153-1-xc-racer2@live.ca>
@ 2020-05-04 22:12 ` Jonathan Bakker
  2020-05-10 20:09   ` Sebastian Reichel
  2020-05-04 22:12 ` [PATCH 2/3] dt-bindings: power: supply: Document maxim,rcomp-value for max17040 Jonathan Bakker
  2020-05-04 22:13 ` [PATCH 3/3] power: supply: max17040: Set rcomp value Jonathan Bakker
  2 siblings, 1 reply; 9+ messages in thread
From: Jonathan Bakker @ 2020-05-04 22:12 UTC (permalink / raw)
  To: sre, linux-pm, linux-kernel, robh+dt, devicetree; +Cc: Jonathan Bakker

According to the datasheet available at (1), the bottom four
bits are always zero and the actual voltage is 1.25x this value
in mV.  Since the kernel API specifies that voltages should be in
uV, it should report 1250x the shifted value.

1) https://datasheets.maximintegrated.com/en/ds/MAX17040-MAX17041.pdf

Signed-off-by: Jonathan Bakker <xc-racer2@live.ca>
---
 drivers/power/supply/max17040_battery.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/power/supply/max17040_battery.c b/drivers/power/supply/max17040_battery.c
index 8a1f0ee493aa..48aa44665e2f 100644
--- a/drivers/power/supply/max17040_battery.c
+++ b/drivers/power/supply/max17040_battery.c
@@ -126,7 +126,7 @@ static void max17040_get_vcell(struct i2c_client *client)
 
 	vcell = max17040_read_reg(client, MAX17040_VCELL);
 
-	chip->vcell = vcell;
+	chip->vcell = (vcell >> 4) * 1250;
 }
 
 static void max17040_get_soc(struct i2c_client *client)
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 2/3] dt-bindings: power: supply: Document maxim,rcomp-value for max17040
       [not found] <20200504221300.3153-1-xc-racer2@live.ca>
  2020-05-04 22:12 ` [PATCH 1/3] power: supply: max17040: Correct voltage reading Jonathan Bakker
@ 2020-05-04 22:12 ` Jonathan Bakker
  2020-05-13  2:16   ` Rob Herring
  2020-05-04 22:13 ` [PATCH 3/3] power: supply: max17040: Set rcomp value Jonathan Bakker
  2 siblings, 1 reply; 9+ messages in thread
From: Jonathan Bakker @ 2020-05-04 22:12 UTC (permalink / raw)
  To: sre, linux-pm, linux-kernel, robh+dt, devicetree; +Cc: Jonathan Bakker

The rcomp value is a device-specific value for configuration based
on specific chemistries.  There is no public documentation on how
to tune it.

Signed-off-by: Jonathan Bakker <xc-racer2@live.ca>
---
 .../devicetree/bindings/power/supply/max17040_battery.txt      | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/power/supply/max17040_battery.txt b/Documentation/devicetree/bindings/power/supply/max17040_battery.txt
index 4e0186b8380f..14097b3ed278 100644
--- a/Documentation/devicetree/bindings/power/supply/max17040_battery.txt
+++ b/Documentation/devicetree/bindings/power/supply/max17040_battery.txt
@@ -11,6 +11,9 @@ Optional properties :
 				generated. Can be configured from 1 up to 32
 				(%). If skipped the power up default value of
 				4 (%) will be used.
+- maxim,rcomp-value :		The value for specific lithium chemistry.  If
+				not present, the default value of 0x97 will be
+				used.
 - interrupts : 			Interrupt line see Documentation/devicetree/
 				bindings/interrupt-controller/interrupts.txt
 - wakeup-source :		This device has wakeup capabilities. Use this
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 3/3] power: supply: max17040: Set rcomp value
       [not found] <20200504221300.3153-1-xc-racer2@live.ca>
  2020-05-04 22:12 ` [PATCH 1/3] power: supply: max17040: Correct voltage reading Jonathan Bakker
  2020-05-04 22:12 ` [PATCH 2/3] dt-bindings: power: supply: Document maxim,rcomp-value for max17040 Jonathan Bakker
@ 2020-05-04 22:13 ` Jonathan Bakker
  2020-05-10 20:08   ` Sebastian Reichel
  2020-05-28 17:02   ` Sebastian Reichel
  2 siblings, 2 replies; 9+ messages in thread
From: Jonathan Bakker @ 2020-05-04 22:13 UTC (permalink / raw)
  To: sre, linux-pm, linux-kernel, robh+dt, devicetree; +Cc: Jonathan Bakker

According to the datasheet (1), the rcomp parameter can
vary based on the typical operating temperature and the
battery chemistry.  If provided, make sure we set it after
we reset the chip on boot.

1) https://datasheets.maximintegrated.com/en/ds/MAX17040-MAX17041.pdf

Signed-off-by: Jonathan Bakker <xc-racer2@live.ca>
---
 drivers/power/supply/max17040_battery.c | 33 +++++++++++++++++++++----
 1 file changed, 28 insertions(+), 5 deletions(-)

diff --git a/drivers/power/supply/max17040_battery.c b/drivers/power/supply/max17040_battery.c
index 48aa44665e2f..f66e2fdc0a8a 100644
--- a/drivers/power/supply/max17040_battery.c
+++ b/drivers/power/supply/max17040_battery.c
@@ -10,6 +10,7 @@
 #include <linux/init.h>
 #include <linux/platform_device.h>
 #include <linux/mutex.h>
+#include <linux/property.h>
 #include <linux/err.h>
 #include <linux/i2c.h>
 #include <linux/delay.h>
@@ -31,6 +32,8 @@
 
 #define MAX17040_ATHD_MASK		0xFFC0
 #define MAX17040_ATHD_DEFAULT_POWER_UP	4
+#define MAX17040_RCOMP_MASK		0xFF
+#define MAX17040_RCOMP_DEFAULT_POWER_UP	0x97
 
 struct max17040_chip {
 	struct i2c_client		*client;
@@ -48,6 +51,8 @@ struct max17040_chip {
 	int status;
 	/* Low alert threshold from 32% to 1% of the State of Charge */
 	u32 low_soc_alert;
+	/* Optimization for specific chemistries */
+	u8 rcomp_value;
 };
 
 static int max17040_get_property(struct power_supply *psy,
@@ -119,6 +124,20 @@ static int max17040_set_low_soc_alert(struct i2c_client *client, u32 level)
 	return ret;
 }
 
+static int max17040_set_rcomp(struct i2c_client *client, u32 val)
+{
+	int ret;
+	u16 data;
+
+	data = max17040_read_reg(client, MAX17040_RCOMP);
+	/* clear the rcomp val and set MSb 8 bits */
+	data &= MAX17040_RCOMP_MASK;
+	data |= val << 8;
+	ret = max17040_write_reg(client, MAX17040_RCOMP, data);
+
+	return ret;
+}
+
 static void max17040_get_vcell(struct i2c_client *client)
 {
 	struct max17040_chip *chip = i2c_get_clientdata(client);
@@ -190,8 +209,14 @@ static int max17040_get_of_data(struct max17040_chip *chip)
 				 "maxim,alert-low-soc-level",
 				 &chip->low_soc_alert);
 
-	if (chip->low_soc_alert <= 0 || chip->low_soc_alert >= 33)
+	if (chip->low_soc_alert <= 0 || chip->low_soc_alert >= 33) {
+		dev_err(&client->dev,
+			"failed: low SOC alert OF data out of bounds\n");
 		return -EINVAL;
+	}
+
+	chip->rcomp_value = MAX17040_RCOMP_DEFAULT_POWER_UP;
+	device_property_read_u8(dev, "maxim,rcomp-value", &chip->rcomp_value);
 
 	return 0;
 }
@@ -289,11 +314,8 @@ static int max17040_probe(struct i2c_client *client,
 	chip->client = client;
 	chip->pdata = client->dev.platform_data;
 	ret = max17040_get_of_data(chip);
-	if (ret) {
-		dev_err(&client->dev,
-			"failed: low SOC alert OF data out of bounds\n");
+	if (ret)
 		return ret;
-	}
 
 	i2c_set_clientdata(client, chip);
 	psy_cfg.drv_data = chip;
@@ -307,6 +329,7 @@ static int max17040_probe(struct i2c_client *client,
 
 	max17040_reset(client);
 	max17040_get_version(client);
+	max17040_set_rcomp(client, chip->rcomp_value);
 
 	/* check interrupt */
 	if (client->irq && of_device_is_compatible(client->dev.of_node,
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH 3/3] power: supply: max17040: Set rcomp value
  2020-05-04 22:13 ` [PATCH 3/3] power: supply: max17040: Set rcomp value Jonathan Bakker
@ 2020-05-10 20:08   ` Sebastian Reichel
  2020-05-11  2:41     ` Jonathan Bakker
  2020-05-28 17:02   ` Sebastian Reichel
  1 sibling, 1 reply; 9+ messages in thread
From: Sebastian Reichel @ 2020-05-10 20:08 UTC (permalink / raw)
  To: Jonathan Bakker; +Cc: linux-pm, linux-kernel, robh+dt, devicetree

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

Hi,

On Mon, May 04, 2020 at 03:13:00PM -0700, Jonathan Bakker wrote:
> According to the datasheet (1), the rcomp parameter can
> vary based on the typical operating temperature and the
> battery chemistry.  If provided, make sure we set it after
> we reset the chip on boot.
> 
> 1) https://datasheets.maximintegrated.com/en/ds/MAX17040-MAX17041.pdf
> 
> Signed-off-by: Jonathan Bakker <xc-racer2@live.ca>
> ---
>  drivers/power/supply/max17040_battery.c | 33 +++++++++++++++++++++----
>  1 file changed, 28 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/power/supply/max17040_battery.c b/drivers/power/supply/max17040_battery.c
> index 48aa44665e2f..f66e2fdc0a8a 100644
> --- a/drivers/power/supply/max17040_battery.c
> +++ b/drivers/power/supply/max17040_battery.c
> @@ -10,6 +10,7 @@
>  #include <linux/init.h>
>  #include <linux/platform_device.h>
>  #include <linux/mutex.h>
> +#include <linux/property.h>
>  #include <linux/err.h>
>  #include <linux/i2c.h>
>  #include <linux/delay.h>
> @@ -31,6 +32,8 @@
>  
>  #define MAX17040_ATHD_MASK		0xFFC0
>  #define MAX17040_ATHD_DEFAULT_POWER_UP	4
> +#define MAX17040_RCOMP_MASK		0xFF
> +#define MAX17040_RCOMP_DEFAULT_POWER_UP	0x97

Why is this 8 bits? Quote from the datasheet, that you linked:

»RCOMP is a 16-bit value used to compensate the ModelGauge algorithm«

-- Sebastian

>  struct max17040_chip {
>  	struct i2c_client		*client;
> @@ -48,6 +51,8 @@ struct max17040_chip {
>  	int status;
>  	/* Low alert threshold from 32% to 1% of the State of Charge */
>  	u32 low_soc_alert;
> +	/* Optimization for specific chemistries */
> +	u8 rcomp_value;
>  };
>  
>  static int max17040_get_property(struct power_supply *psy,
> @@ -119,6 +124,20 @@ static int max17040_set_low_soc_alert(struct i2c_client *client, u32 level)
>  	return ret;
>  }
>  
> +static int max17040_set_rcomp(struct i2c_client *client, u32 val)
> +{
> +	int ret;
> +	u16 data;
> +
> +	data = max17040_read_reg(client, MAX17040_RCOMP);
> +	/* clear the rcomp val and set MSb 8 bits */
> +	data &= MAX17040_RCOMP_MASK;
> +	data |= val << 8;
> +	ret = max17040_write_reg(client, MAX17040_RCOMP, data);
> +
> +	return ret;
> +}
> +
>  static void max17040_get_vcell(struct i2c_client *client)
>  {
>  	struct max17040_chip *chip = i2c_get_clientdata(client);
> @@ -190,8 +209,14 @@ static int max17040_get_of_data(struct max17040_chip *chip)
>  				 "maxim,alert-low-soc-level",
>  				 &chip->low_soc_alert);
>  
> -	if (chip->low_soc_alert <= 0 || chip->low_soc_alert >= 33)
> +	if (chip->low_soc_alert <= 0 || chip->low_soc_alert >= 33) {
> +		dev_err(&client->dev,
> +			"failed: low SOC alert OF data out of bounds\n");
>  		return -EINVAL;
> +	}
> +
> +	chip->rcomp_value = MAX17040_RCOMP_DEFAULT_POWER_UP;
> +	device_property_read_u8(dev, "maxim,rcomp-value", &chip->rcomp_value);
>  
>  	return 0;
>  }
> @@ -289,11 +314,8 @@ static int max17040_probe(struct i2c_client *client,
>  	chip->client = client;
>  	chip->pdata = client->dev.platform_data;
>  	ret = max17040_get_of_data(chip);
> -	if (ret) {
> -		dev_err(&client->dev,
> -			"failed: low SOC alert OF data out of bounds\n");
> +	if (ret)
>  		return ret;
> -	}
>  
>  	i2c_set_clientdata(client, chip);
>  	psy_cfg.drv_data = chip;
> @@ -307,6 +329,7 @@ static int max17040_probe(struct i2c_client *client,
>  
>  	max17040_reset(client);
>  	max17040_get_version(client);
> +	max17040_set_rcomp(client, chip->rcomp_value);
>  
>  	/* check interrupt */
>  	if (client->irq && of_device_is_compatible(client->dev.of_node,
> -- 
> 2.20.1
> 

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/3] power: supply: max17040: Correct voltage reading
  2020-05-04 22:12 ` [PATCH 1/3] power: supply: max17040: Correct voltage reading Jonathan Bakker
@ 2020-05-10 20:09   ` Sebastian Reichel
  0 siblings, 0 replies; 9+ messages in thread
From: Sebastian Reichel @ 2020-05-10 20:09 UTC (permalink / raw)
  To: Jonathan Bakker; +Cc: linux-pm, linux-kernel, robh+dt, devicetree

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

Hi,

On Mon, May 04, 2020 at 03:12:58PM -0700, Jonathan Bakker wrote:
> According to the datasheet available at (1), the bottom four
> bits are always zero and the actual voltage is 1.25x this value
> in mV.  Since the kernel API specifies that voltages should be in
> uV, it should report 1250x the shifted value.
> 
> 1) https://datasheets.maximintegrated.com/en/ds/MAX17040-MAX17041.pdf
> 
> Signed-off-by: Jonathan Bakker <xc-racer2@live.ca>
> ---

Thanks, queued.

-- Sebastian

>  drivers/power/supply/max17040_battery.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/power/supply/max17040_battery.c b/drivers/power/supply/max17040_battery.c
> index 8a1f0ee493aa..48aa44665e2f 100644
> --- a/drivers/power/supply/max17040_battery.c
> +++ b/drivers/power/supply/max17040_battery.c
> @@ -126,7 +126,7 @@ static void max17040_get_vcell(struct i2c_client *client)
>  
>  	vcell = max17040_read_reg(client, MAX17040_VCELL);
>  
> -	chip->vcell = vcell;
> +	chip->vcell = (vcell >> 4) * 1250;
>  }
>  
>  static void max17040_get_soc(struct i2c_client *client)
> -- 
> 2.20.1
> 

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 3/3] power: supply: max17040: Set rcomp value
  2020-05-10 20:08   ` Sebastian Reichel
@ 2020-05-11  2:41     ` Jonathan Bakker
  0 siblings, 0 replies; 9+ messages in thread
From: Jonathan Bakker @ 2020-05-11  2:41 UTC (permalink / raw)
  To: Sebastian Reichel; +Cc: linux-pm, linux-kernel, robh+dt, devicetree

Hi Sebastian,

On 2020-05-10 1:08 p.m., Sebastian Reichel wrote:
> Hi,
> 
> On Mon, May 04, 2020 at 03:13:00PM -0700, Jonathan Bakker wrote:
>> According to the datasheet (1), the rcomp parameter can
>> vary based on the typical operating temperature and the
>> battery chemistry.  If provided, make sure we set it after
>> we reset the chip on boot.
>>
>> 1) https://datasheets.maximintegrated.com/en/ds/MAX17040-MAX17041.pdf
>>
>> Signed-off-by: Jonathan Bakker <xc-racer2@live.ca>
>> ---
>>  drivers/power/supply/max17040_battery.c | 33 +++++++++++++++++++++----
>>  1 file changed, 28 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/power/supply/max17040_battery.c b/drivers/power/supply/max17040_battery.c
>> index 48aa44665e2f..f66e2fdc0a8a 100644
>> --- a/drivers/power/supply/max17040_battery.c
>> +++ b/drivers/power/supply/max17040_battery.c
>> @@ -10,6 +10,7 @@
>>  #include <linux/init.h>
>>  #include <linux/platform_device.h>
>>  #include <linux/mutex.h>
>> +#include <linux/property.h>
>>  #include <linux/err.h>
>>  #include <linux/i2c.h>
>>  #include <linux/delay.h>
>> @@ -31,6 +32,8 @@
>>  
>>  #define MAX17040_ATHD_MASK		0xFFC0
>>  #define MAX17040_ATHD_DEFAULT_POWER_UP	4
>> +#define MAX17040_RCOMP_MASK		0xFF
>> +#define MAX17040_RCOMP_DEFAULT_POWER_UP	0x97
> 
> Why is this 8 bits? Quote from the datasheet, that you linked:
> 
> »RCOMP is a 16-bit value used to compensate the ModelGauge algorithm«

Well, the driver also supports the max17043 (datasheet at https://datasheets.maximintegrated.com/en/ds/MAX17043-MAX17044.pdf
here the register is named CONFIG), by the maxim,max77836-battery compatible.  The bottom 8 bits are for the alert config,
and I'm presuming it's the same on the max17040 (the vendor kernel for the device I'm testing on only sets the top 8 bits
and leaves the rest at 0).

If there's a better way of doing it (ie maybe explicitly making it a 16 bit value and checking if the bottom 8 bits are
set when the compatible is maxim,max77836-battery), then I'm happy to do it that way.

Thanks,
Jonathan

> 
> -- Sebastian
> 
>>  struct max17040_chip {
>>  	struct i2c_client		*client;
>> @@ -48,6 +51,8 @@ struct max17040_chip {
>>  	int status;
>>  	/* Low alert threshold from 32% to 1% of the State of Charge */
>>  	u32 low_soc_alert;
>> +	/* Optimization for specific chemistries */
>> +	u8 rcomp_value;
>>  };
>>  
>>  static int max17040_get_property(struct power_supply *psy,
>> @@ -119,6 +124,20 @@ static int max17040_set_low_soc_alert(struct i2c_client *client, u32 level)
>>  	return ret;
>>  }
>>  
>> +static int max17040_set_rcomp(struct i2c_client *client, u32 val)
>> +{
>> +	int ret;
>> +	u16 data;
>> +
>> +	data = max17040_read_reg(client, MAX17040_RCOMP);
>> +	/* clear the rcomp val and set MSb 8 bits */
>> +	data &= MAX17040_RCOMP_MASK;
>> +	data |= val << 8;
>> +	ret = max17040_write_reg(client, MAX17040_RCOMP, data);
>> +
>> +	return ret;
>> +}
>> +
>>  static void max17040_get_vcell(struct i2c_client *client)
>>  {
>>  	struct max17040_chip *chip = i2c_get_clientdata(client);
>> @@ -190,8 +209,14 @@ static int max17040_get_of_data(struct max17040_chip *chip)
>>  				 "maxim,alert-low-soc-level",
>>  				 &chip->low_soc_alert);
>>  
>> -	if (chip->low_soc_alert <= 0 || chip->low_soc_alert >= 33)
>> +	if (chip->low_soc_alert <= 0 || chip->low_soc_alert >= 33) {
>> +		dev_err(&client->dev,
>> +			"failed: low SOC alert OF data out of bounds\n");
>>  		return -EINVAL;
>> +	}
>> +
>> +	chip->rcomp_value = MAX17040_RCOMP_DEFAULT_POWER_UP;
>> +	device_property_read_u8(dev, "maxim,rcomp-value", &chip->rcomp_value);
>>  
>>  	return 0;
>>  }
>> @@ -289,11 +314,8 @@ static int max17040_probe(struct i2c_client *client,
>>  	chip->client = client;
>>  	chip->pdata = client->dev.platform_data;
>>  	ret = max17040_get_of_data(chip);
>> -	if (ret) {
>> -		dev_err(&client->dev,
>> -			"failed: low SOC alert OF data out of bounds\n");
>> +	if (ret)
>>  		return ret;
>> -	}
>>  
>>  	i2c_set_clientdata(client, chip);
>>  	psy_cfg.drv_data = chip;
>> @@ -307,6 +329,7 @@ static int max17040_probe(struct i2c_client *client,
>>  
>>  	max17040_reset(client);
>>  	max17040_get_version(client);
>> +	max17040_set_rcomp(client, chip->rcomp_value);
>>  
>>  	/* check interrupt */
>>  	if (client->irq && of_device_is_compatible(client->dev.of_node,
>> -- 
>> 2.20.1
>>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/3] dt-bindings: power: supply: Document maxim,rcomp-value for max17040
  2020-05-04 22:12 ` [PATCH 2/3] dt-bindings: power: supply: Document maxim,rcomp-value for max17040 Jonathan Bakker
@ 2020-05-13  2:16   ` Rob Herring
  0 siblings, 0 replies; 9+ messages in thread
From: Rob Herring @ 2020-05-13  2:16 UTC (permalink / raw)
  To: Jonathan Bakker; +Cc: linux-kernel, devicetree, sre, linux-pm, robh+dt

On Mon,  4 May 2020 15:12:59 -0700, Jonathan Bakker wrote:
> The rcomp value is a device-specific value for configuration based
> on specific chemistries.  There is no public documentation on how
> to tune it.
> 
> Signed-off-by: Jonathan Bakker <xc-racer2@live.ca>
> ---
>  .../devicetree/bindings/power/supply/max17040_battery.txt      | 3 +++
>  1 file changed, 3 insertions(+)
> 

Acked-by: Rob Herring <robh@kernel.org>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 3/3] power: supply: max17040: Set rcomp value
  2020-05-04 22:13 ` [PATCH 3/3] power: supply: max17040: Set rcomp value Jonathan Bakker
  2020-05-10 20:08   ` Sebastian Reichel
@ 2020-05-28 17:02   ` Sebastian Reichel
  2020-05-29 20:09     ` Jonathan Bakker
  1 sibling, 1 reply; 9+ messages in thread
From: Sebastian Reichel @ 2020-05-28 17:02 UTC (permalink / raw)
  To: Jonathan Bakker; +Cc: linux-pm, linux-kernel, robh+dt, devicetree

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

Hi,

This patch does not even compile, how did you test it?

-- Sebastian

On Mon, May 04, 2020 at 03:13:00PM -0700, Jonathan Bakker wrote:
> According to the datasheet (1), the rcomp parameter can
> vary based on the typical operating temperature and the
> battery chemistry.  If provided, make sure we set it after
> we reset the chip on boot.
> 
> 1) https://datasheets.maximintegrated.com/en/ds/MAX17040-MAX17041.pdf
> 
> Signed-off-by: Jonathan Bakker <xc-racer2@live.ca>
> ---
>  drivers/power/supply/max17040_battery.c | 33 +++++++++++++++++++++----
>  1 file changed, 28 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/power/supply/max17040_battery.c b/drivers/power/supply/max17040_battery.c
> index 48aa44665e2f..f66e2fdc0a8a 100644
> --- a/drivers/power/supply/max17040_battery.c
> +++ b/drivers/power/supply/max17040_battery.c
> @@ -10,6 +10,7 @@
>  #include <linux/init.h>
>  #include <linux/platform_device.h>
>  #include <linux/mutex.h>
> +#include <linux/property.h>
>  #include <linux/err.h>
>  #include <linux/i2c.h>
>  #include <linux/delay.h>
> @@ -31,6 +32,8 @@
>  
>  #define MAX17040_ATHD_MASK		0xFFC0
>  #define MAX17040_ATHD_DEFAULT_POWER_UP	4
> +#define MAX17040_RCOMP_MASK		0xFF
> +#define MAX17040_RCOMP_DEFAULT_POWER_UP	0x97
>  
>  struct max17040_chip {
>  	struct i2c_client		*client;
> @@ -48,6 +51,8 @@ struct max17040_chip {
>  	int status;
>  	/* Low alert threshold from 32% to 1% of the State of Charge */
>  	u32 low_soc_alert;
> +	/* Optimization for specific chemistries */
> +	u8 rcomp_value;
>  };
>  
>  static int max17040_get_property(struct power_supply *psy,
> @@ -119,6 +124,20 @@ static int max17040_set_low_soc_alert(struct i2c_client *client, u32 level)
>  	return ret;
>  }
>  
> +static int max17040_set_rcomp(struct i2c_client *client, u32 val)
> +{
> +	int ret;
> +	u16 data;
> +
> +	data = max17040_read_reg(client, MAX17040_RCOMP);
> +	/* clear the rcomp val and set MSb 8 bits */
> +	data &= MAX17040_RCOMP_MASK;
> +	data |= val << 8;
> +	ret = max17040_write_reg(client, MAX17040_RCOMP, data);
> +
> +	return ret;
> +}
> +
>  static void max17040_get_vcell(struct i2c_client *client)
>  {
>  	struct max17040_chip *chip = i2c_get_clientdata(client);
> @@ -190,8 +209,14 @@ static int max17040_get_of_data(struct max17040_chip *chip)
>  				 "maxim,alert-low-soc-level",
>  				 &chip->low_soc_alert);
>  
> -	if (chip->low_soc_alert <= 0 || chip->low_soc_alert >= 33)
> +	if (chip->low_soc_alert <= 0 || chip->low_soc_alert >= 33) {
> +		dev_err(&client->dev,
> +			"failed: low SOC alert OF data out of bounds\n");
>  		return -EINVAL;
> +	}
> +
> +	chip->rcomp_value = MAX17040_RCOMP_DEFAULT_POWER_UP;
> +	device_property_read_u8(dev, "maxim,rcomp-value", &chip->rcomp_value);
>  
>  	return 0;
>  }
> @@ -289,11 +314,8 @@ static int max17040_probe(struct i2c_client *client,
>  	chip->client = client;
>  	chip->pdata = client->dev.platform_data;
>  	ret = max17040_get_of_data(chip);
> -	if (ret) {
> -		dev_err(&client->dev,
> -			"failed: low SOC alert OF data out of bounds\n");
> +	if (ret)
>  		return ret;
> -	}
>  
>  	i2c_set_clientdata(client, chip);
>  	psy_cfg.drv_data = chip;
> @@ -307,6 +329,7 @@ static int max17040_probe(struct i2c_client *client,
>  
>  	max17040_reset(client);
>  	max17040_get_version(client);
> +	max17040_set_rcomp(client, chip->rcomp_value);
>  
>  	/* check interrupt */
>  	if (client->irq && of_device_is_compatible(client->dev.of_node,
> -- 
> 2.20.1
> 

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 3/3] power: supply: max17040: Set rcomp value
  2020-05-28 17:02   ` Sebastian Reichel
@ 2020-05-29 20:09     ` Jonathan Bakker
  0 siblings, 0 replies; 9+ messages in thread
From: Jonathan Bakker @ 2020-05-29 20:09 UTC (permalink / raw)
  To: Sebastian Reichel; +Cc: linux-pm, linux-kernel, robh+dt, devicetree

Hi Sebastian,

I'm sorry, I messed up my rebase on top of the low battery alert and it somehow
slipped through my pre-submit checklist.

Before resubmitting, do you want the rcomp changed in any manner (where the
datasheet doesn't specify if its the full 16 bits or only 8 bites for max17040
but does for the later max17043/max77836 where its only 8 bits)?

Thanks and sorry for the issues,
Jonathan

On 2020-05-28 10:02 a.m., Sebastian Reichel wrote:
> Hi,
> 
> This patch does not even compile, how did you test it?
> 
> -- Sebastian
> 
> On Mon, May 04, 2020 at 03:13:00PM -0700, Jonathan Bakker wrote:
>> According to the datasheet (1), the rcomp parameter can
>> vary based on the typical operating temperature and the
>> battery chemistry.  If provided, make sure we set it after
>> we reset the chip on boot.
>>
>> 1) https://datasheets.maximintegrated.com/en/ds/MAX17040-MAX17041.pdf
>>
>> Signed-off-by: Jonathan Bakker <xc-racer2@live.ca>
>> ---
>>  drivers/power/supply/max17040_battery.c | 33 +++++++++++++++++++++----
>>  1 file changed, 28 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/power/supply/max17040_battery.c b/drivers/power/supply/max17040_battery.c
>> index 48aa44665e2f..f66e2fdc0a8a 100644
>> --- a/drivers/power/supply/max17040_battery.c
>> +++ b/drivers/power/supply/max17040_battery.c
>> @@ -10,6 +10,7 @@
>>  #include <linux/init.h>
>>  #include <linux/platform_device.h>
>>  #include <linux/mutex.h>
>> +#include <linux/property.h>
>>  #include <linux/err.h>
>>  #include <linux/i2c.h>
>>  #include <linux/delay.h>
>> @@ -31,6 +32,8 @@
>>  
>>  #define MAX17040_ATHD_MASK		0xFFC0
>>  #define MAX17040_ATHD_DEFAULT_POWER_UP	4
>> +#define MAX17040_RCOMP_MASK		0xFF
>> +#define MAX17040_RCOMP_DEFAULT_POWER_UP	0x97
>>  
>>  struct max17040_chip {
>>  	struct i2c_client		*client;
>> @@ -48,6 +51,8 @@ struct max17040_chip {
>>  	int status;
>>  	/* Low alert threshold from 32% to 1% of the State of Charge */
>>  	u32 low_soc_alert;
>> +	/* Optimization for specific chemistries */
>> +	u8 rcomp_value;
>>  };
>>  
>>  static int max17040_get_property(struct power_supply *psy,
>> @@ -119,6 +124,20 @@ static int max17040_set_low_soc_alert(struct i2c_client *client, u32 level)
>>  	return ret;
>>  }
>>  
>> +static int max17040_set_rcomp(struct i2c_client *client, u32 val)
>> +{
>> +	int ret;
>> +	u16 data;
>> +
>> +	data = max17040_read_reg(client, MAX17040_RCOMP);
>> +	/* clear the rcomp val and set MSb 8 bits */
>> +	data &= MAX17040_RCOMP_MASK;
>> +	data |= val << 8;
>> +	ret = max17040_write_reg(client, MAX17040_RCOMP, data);
>> +
>> +	return ret;
>> +}
>> +
>>  static void max17040_get_vcell(struct i2c_client *client)
>>  {
>>  	struct max17040_chip *chip = i2c_get_clientdata(client);
>> @@ -190,8 +209,14 @@ static int max17040_get_of_data(struct max17040_chip *chip)
>>  				 "maxim,alert-low-soc-level",
>>  				 &chip->low_soc_alert);
>>  
>> -	if (chip->low_soc_alert <= 0 || chip->low_soc_alert >= 33)
>> +	if (chip->low_soc_alert <= 0 || chip->low_soc_alert >= 33) {
>> +		dev_err(&client->dev,
>> +			"failed: low SOC alert OF data out of bounds\n");
>>  		return -EINVAL;
>> +	}
>> +
>> +	chip->rcomp_value = MAX17040_RCOMP_DEFAULT_POWER_UP;
>> +	device_property_read_u8(dev, "maxim,rcomp-value", &chip->rcomp_value);
>>  
>>  	return 0;
>>  }
>> @@ -289,11 +314,8 @@ static int max17040_probe(struct i2c_client *client,
>>  	chip->client = client;
>>  	chip->pdata = client->dev.platform_data;
>>  	ret = max17040_get_of_data(chip);
>> -	if (ret) {
>> -		dev_err(&client->dev,
>> -			"failed: low SOC alert OF data out of bounds\n");
>> +	if (ret)
>>  		return ret;
>> -	}
>>  
>>  	i2c_set_clientdata(client, chip);
>>  	psy_cfg.drv_data = chip;
>> @@ -307,6 +329,7 @@ static int max17040_probe(struct i2c_client *client,
>>  
>>  	max17040_reset(client);
>>  	max17040_get_version(client);
>> +	max17040_set_rcomp(client, chip->rcomp_value);
>>  
>>  	/* check interrupt */
>>  	if (client->irq && of_device_is_compatible(client->dev.of_node,
>> -- 
>> 2.20.1

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2020-05-29 20:09 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <20200504221300.3153-1-xc-racer2@live.ca>
2020-05-04 22:12 ` [PATCH 1/3] power: supply: max17040: Correct voltage reading Jonathan Bakker
2020-05-10 20:09   ` Sebastian Reichel
2020-05-04 22:12 ` [PATCH 2/3] dt-bindings: power: supply: Document maxim,rcomp-value for max17040 Jonathan Bakker
2020-05-13  2:16   ` Rob Herring
2020-05-04 22:13 ` [PATCH 3/3] power: supply: max17040: Set rcomp value Jonathan Bakker
2020-05-10 20:08   ` Sebastian Reichel
2020-05-11  2:41     ` Jonathan Bakker
2020-05-28 17:02   ` Sebastian Reichel
2020-05-29 20:09     ` Jonathan Bakker

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).