| Message ID | BN6PR04MB066057B881DEFC0C48208589A3A60@BN6PR04MB0660.namprd04.prod.outlook.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | power: supply: Improve max17040 support | expand |
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 >
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 >>
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 >
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
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,
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(-)