Re: [PATCH 3/3] power: supply: max17040: Set rcomp value

[Jonathan Bakker]

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@xxxxxxx>
>> ---
>>  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
>>