| Message ID | 20200618101340.2671020-1-iskren.chernev@gmail.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [v2,1/2] dt-bindings: power: supply: Document max17040 extensions | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success |
Hi, On Thu, Jun 18, 2020 at 01:13:39PM +0300, Iskren Chernev wrote: > Maxim max17040 is a fuel gauge from a larger family utilising the Model > Gauge technology. Document all different compatible strings that the > max17040 driver recognizes. > > Some devices in the wild report double the capacity. The > maxim,double-soc (from State-Of-Charge) property fixes that. > > Complete device reset might lead to very inaccurate readings. Specify > maxim,skip-reset to avoid that. > > To compensate for the battery chemistry and operating conditions the > chips support a compensation value. Specify one or two byte compensation > via the maxim,rcomp byte array. > > Signed-off-by: Iskren Chernev <iskren.chernev@gmail.com> > --- > v1: https://lkml.org/lkml/2020/6/8/682 > > Changes in v2: > - add maxim,skip-reset > - remove 2 byte rcomp from example, the specified compat string supports 1 byte > rcomp > > .../power/supply/max17040_battery.txt | 24 ++++++++++++++++++- > 1 file changed, 23 insertions(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/power/supply/max17040_battery.txt b/Documentation/devicetree/bindings/power/supply/max17040_battery.txt > index 4e0186b8380fa..3ee91c295027f 100644 > --- a/Documentation/devicetree/bindings/power/supply/max17040_battery.txt > +++ b/Documentation/devicetree/bindings/power/supply/max17040_battery.txt > @@ -2,7 +2,9 @@ max17040_battery > ~~~~~~~~~~~~~~~~ > > Required properties : > - - compatible : "maxim,max17040" or "maxim,max77836-battery" > + - compatible : "maxim,max17040", "maxim,max17041", "maxim,max17043", > + "maxim,max17044", "maxim,max17048", "maxim,max17049", > + "maxim,max17058", "maxim,max17059" or "maxim,max77836-battery" > - reg: i2c slave address > > Optional properties : > @@ -11,6 +13,18 @@ Optional properties : > generated. Can be configured from 1 up to 32 > (%). If skipped the power up default value of > 4 (%) will be used. > +- maxim,double-soc : Certain devices return double the capacity. > + Specify this boolean property to divide the > + reported value in 2 and thus normalize it. > + SOC == State of Charge == Capacity. Can this be derived from the compatible? > +- maxim,skip-reset : Do not reset device on driver initialization. > + Some devices report extremely inaccurately after > + a hard reset. Same question. -- Sebastian > +- maxim,rcomp : A value to compensate readings for various > + battery chemistries and operating temperatures. > + max17040,41 have 2 byte rcomp, default to > + 0x97 0x00. All other devices have one byte > + rcomp, default to 0x97. > - interrupts : Interrupt line see Documentation/devicetree/ > bindings/interrupt-controller/interrupts.txt > - wakeup-source : This device has wakeup capabilities. Use this > @@ -31,3 +45,11 @@ Example: > interrupts = <2 IRQ_TYPE_EDGE_FALLING>; > wakeup-source; > }; > + > + battery-fuel-gauge@36 { > + compatible = "maxim,max17048"; > + reg = <0x36>; > + maxim,rcomp = /bits/ 8 <0x97>; > + maxim,alert-low-soc-level = <10>; > + maxim,double-soc; > + }; > > base-commit: 1713116fa907cc7290020f0d8632ec646d2936f8 > -- > 2.27.0 >
Hi, On Thu, Jun 18, 2020 at 01:13:40PM +0300, Iskren Chernev wrote: > The max17040 fuel gauge is part of a family of 8 chips that have very > similar mode of operations and registers. > > This change adds: > - compatible strings for all supported devices and handles the minor > differences between them; > - handling for devices reporting double capacity via maxim,double-soc; > - handling for setting rcomp, a compensation value for more accurate > reading, affected by battery chemistry and operating temps; > - suppot for SOC alerts (capacity changes by +/- 1%), to prevent polling > every second; > - redo i2c read/writes via regmap Please split this into multiple patches as described in Documentation/process/submitting-patches.rst: 3) Separate your changes ------------------------ Separate each **logical change** into a separate patch. Thanks, -- Sebastian > The datasheets of the supported devices are linked [0] [1] [2] [3]. > > [0] https://datasheets.maximintegrated.com/en/ds/MAX17040-MAX17041.pdf > [1] https://datasheets.maximintegrated.com/en/ds/MAX17043-MAX17044.pdf > [2] https://datasheets.maximintegrated.com/en/ds/MAX17048-MAX17049.pdf > [3] https://datasheets.maximintegrated.com/en/ds/MAX17058-MAX17059.pdf > > Signed-off-by: Iskren Chernev <iskren.chernev@gmail.com> > --- > v1: https://lkml.org/lkml/2020/6/8/683 > > changes in v2: > - make max17040_family static > - fix rcomp len check > - implement maxim,skip-reset > - reword Kconfig text > > drivers/power/supply/Kconfig | 11 +- > drivers/power/supply/max17040_battery.c | 466 +++++++++++++++++------- > 2 files changed, 338 insertions(+), 139 deletions(-) > > diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig > index 44d3c8512fb8d..69e9eaa100d9f 100644 > --- a/drivers/power/supply/Kconfig > +++ b/drivers/power/supply/Kconfig > @@ -368,9 +368,14 @@ config BATTERY_MAX17040 > tristate "Maxim MAX17040 Fuel Gauge" > depends on I2C > help > - MAX17040 is fuel-gauge systems for lithium-ion (Li+) batteries > - in handheld and portable equipment. The MAX17040 is configured > - to operate with a single lithium cell > + Maxim models with ModelGauge are fuel-gauge systems for lithium-ion > + (Li+) batteries in handheld and portable equipment, including > + max17040, max17041, max17043, max17044, max17048, max17049, max17058, > + max17059. It is also included in some batteries like max77836. > + > + Driver supports reporting SOC (State of Charge, i.e capacity), > + voltage and configurable low-SOC wakeup interrupt. For certain models > + a SOC change interrupt is also supported. > > config BATTERY_MAX17042 > tristate "Maxim MAX17042/17047/17050/8997/8966 Fuel Gauge" > diff --git a/drivers/power/supply/max17040_battery.c b/drivers/power/supply/max17040_battery.c > index 48aa44665e2f1..1244982cacacb 100644 > --- a/drivers/power/supply/max17040_battery.c > +++ b/drivers/power/supply/max17040_battery.c > @@ -15,193 +15,342 @@ > #include <linux/delay.h> > #include <linux/interrupt.h> > #include <linux/power_supply.h> > +#include <linux/of_device.h> > #include <linux/max17040_battery.h> > +#include <linux/regmap.h> > #include <linux/slab.h> > > #define MAX17040_VCELL 0x02 > #define MAX17040_SOC 0x04 > #define MAX17040_MODE 0x06 > #define MAX17040_VER 0x08 > -#define MAX17040_RCOMP 0x0C > +#define MAX17040_CONFIG 0x0C > +#define MAX17040_STATUS 0x1A > #define MAX17040_CMD 0xFE > > > #define MAX17040_DELAY 1000 > #define MAX17040_BATTERY_FULL 95 > +#define MAX17040_RCOMP_DEFAULT 0x9700 > > -#define MAX17040_ATHD_MASK 0xFFC0 > +#define MAX17040_ATHD_MASK 0x3f > +#define MAX17040_ALSC_MASK 0x40 > #define MAX17040_ATHD_DEFAULT_POWER_UP 4 > +#define MAX17040_STATUS_HD_MASK 0x1000 > +#define MAX17040_STATUS_SC_MASK 0x2000 > +#define MAX17040_CFG_RCOMP_MASK 0xff00 > + > +enum chip_id { > + ID_MAX17040, > + ID_MAX17041, > + ID_MAX17043, > + ID_MAX17044, > + ID_MAX17048, > + ID_MAX17049, > + ID_MAX17058, > + ID_MAX17059, > +}; > + > +/* values that differ by chip_id */ > +struct chip_data { > + u16 reset_val; > + u16 vcell_shift; > + u16 vcell_mul; > + u16 vcell_div; > + u8 rcomp_bytes; > + u8 has_low_soc_alert; > + u8 has_soc_alert; > +}; > + > +static struct chip_data max17040_family[] = { > + [ID_MAX17040] = { > + .reset_val = 0x0054, > + .vcell_shift = 4, > + .vcell_mul = 1250, > + .vcell_div = 1, > + .rcomp_bytes = 2, > + .has_low_soc_alert = 0, > + .has_soc_alert = 0, > + }, > + [ID_MAX17041] = { > + .reset_val = 0x0054, > + .vcell_shift = 4, > + .vcell_mul = 2500, > + .vcell_div = 1, > + .rcomp_bytes = 2, > + .has_low_soc_alert = 0, > + .has_soc_alert = 0, > + }, > + [ID_MAX17043] = { > + .reset_val = 0x0054, > + .vcell_shift = 4, > + .vcell_mul = 1250, > + .vcell_div = 1, > + .rcomp_bytes = 1, > + .has_low_soc_alert = 1, > + .has_soc_alert = 0, > + }, > + [ID_MAX17044] = { > + .reset_val = 0x0054, > + .vcell_shift = 4, > + .vcell_mul = 2500, > + .vcell_div = 1, > + .rcomp_bytes = 1, > + .has_low_soc_alert = 1, > + .has_soc_alert = 0, > + }, > + [ID_MAX17048] = { > + .reset_val = 0x5400, > + .vcell_shift = 0, > + .vcell_mul = 625, > + .vcell_div = 8, > + .rcomp_bytes = 1, > + .has_low_soc_alert = 1, > + .has_soc_alert = 1, > + }, > + [ID_MAX17049] = { > + .reset_val = 0x5400, > + .vcell_shift = 0, > + .vcell_mul = 625, > + .vcell_div = 4, > + .rcomp_bytes = 1, > + .has_low_soc_alert = 1, > + .has_soc_alert = 1, > + }, > + [ID_MAX17058] = { > + .reset_val = 0x5400, > + .vcell_shift = 0, > + .vcell_mul = 625, > + .vcell_div = 8, > + .rcomp_bytes = 1, > + .has_low_soc_alert = 1, > + .has_soc_alert = 0, > + }, > + [ID_MAX17059] = { > + .reset_val = 0x5400, > + .vcell_shift = 0, > + .vcell_mul = 625, > + .vcell_div = 4, > + .rcomp_bytes = 1, > + .has_low_soc_alert = 1, > + .has_soc_alert = 0, > + }, > +}; > > struct max17040_chip { > struct i2c_client *client; > + struct regmap *regmap; > struct delayed_work work; > struct power_supply *battery; > struct max17040_platform_data *pdata; > + struct chip_data data; > > - /* State Of Connect */ > - int online; > - /* battery voltage */ > - int vcell; > /* battery capacity */ > int soc; > /* State Of Charge */ > int status; > + > /* Low alert threshold from 32% to 1% of the State of Charge */ > u32 low_soc_alert; > + /* some devices return twice the capacity */ > + bool quirk_double_soc; > + /* resetting causes some devices to show inaccurate reading */ > + bool quirk_skip_reset; > + /* higher 8 bits for 17043+, 16 bits for 17040,41 */ > + u16 rcomp; > }; > > -static int max17040_get_property(struct power_supply *psy, > - enum power_supply_property psp, > - union power_supply_propval *val) > +static int max17040_reset(struct max17040_chip *chip) > { > - struct max17040_chip *chip = power_supply_get_drvdata(psy); > - > - switch (psp) { > - case POWER_SUPPLY_PROP_STATUS: > - val->intval = chip->status; > - break; > - case POWER_SUPPLY_PROP_ONLINE: > - val->intval = chip->online; > - break; > - case POWER_SUPPLY_PROP_VOLTAGE_NOW: > - val->intval = chip->vcell; > - break; > - case POWER_SUPPLY_PROP_CAPACITY: > - val->intval = chip->soc; > - break; > - default: > - return -EINVAL; > - } > - return 0; > + return regmap_write(chip->regmap, MAX17040_CMD, chip->data.reset_val); > } > > -static int max17040_write_reg(struct i2c_client *client, int reg, u16 value) > +static int max17040_set_low_soc_alert(struct max17040_chip *chip, u32 level) > { > - int ret; > - > - ret = i2c_smbus_write_word_swapped(client, reg, value); > - > - if (ret < 0) > - dev_err(&client->dev, "%s: err %d\n", __func__, ret); > + level = 32 - level; > + return regmap_update_bits(chip->regmap, MAX17040_CONFIG, > + MAX17040_ATHD_MASK, level); > +} > > - return ret; > +static int max17040_set_soc_alert(struct max17040_chip *chip, bool enable) > +{ > + return regmap_update_bits(chip->regmap, MAX17040_CONFIG, > + MAX17040_ALSC_MASK, enable ? MAX17040_ALSC_MASK : 0); > } > > -static int max17040_read_reg(struct i2c_client *client, int reg) > +/* Returns true if alert cause was SOC change, not low SOC */ > +static bool max17040_handle_soc_alert(struct max17040_chip *chip) > { > - int ret; > + bool ret = true; > + u32 data; > > - ret = i2c_smbus_read_word_swapped(client, reg); > + regmap_read(chip->regmap, MAX17040_STATUS, &data); > > - if (ret < 0) > - dev_err(&client->dev, "%s: err %d\n", __func__, ret); > + if (data & MAX17040_STATUS_HD_MASK) { > + // this alert was caused by low soc > + ret = false; > + } > + if (data & MAX17040_STATUS_SC_MASK) { > + // soc change bit -- deassert to mark as handled > + regmap_write(chip->regmap, MAX17040_STATUS, > + data & ~MAX17040_STATUS_SC_MASK); > + } > > return ret; > } > > -static void max17040_reset(struct i2c_client *client) > +static int max17040_set_rcomp(struct max17040_chip *chip, u16 rcomp) > { > - max17040_write_reg(client, MAX17040_CMD, 0x0054); > + u16 mask = chip->data.rcomp_bytes == 2 ? > + 0xffff : MAX17040_CFG_RCOMP_MASK; > + > + return regmap_update_bits(chip->regmap, MAX17040_CONFIG, mask, rcomp); > } > > -static int max17040_set_low_soc_alert(struct i2c_client *client, u32 level) > +static int max17040_raw_vcell_to_uvolts(struct max17040_chip *chip, u16 vcell) > { > - int ret; > - u16 data; > - > - level = 32 - level; > - data = max17040_read_reg(client, MAX17040_RCOMP); > - /* clear the alrt bit and set LSb 5 bits */ > - data &= MAX17040_ATHD_MASK; > - data |= level; > - ret = max17040_write_reg(client, MAX17040_RCOMP, data); > + struct chip_data *d = &chip->data; > > - return ret; > + return (vcell >> d->vcell_shift) * d->vcell_mul / d->vcell_div; > } > > -static void max17040_get_vcell(struct i2c_client *client) > +static int max17040_get_vcell(struct max17040_chip *chip) > { > - struct max17040_chip *chip = i2c_get_clientdata(client); > - u16 vcell; > + u32 vcell; > > - vcell = max17040_read_reg(client, MAX17040_VCELL); > + regmap_read(chip->regmap, MAX17040_VCELL, &vcell); > > - chip->vcell = (vcell >> 4) * 1250; > + return max17040_raw_vcell_to_uvolts(chip, vcell); > } > > -static void max17040_get_soc(struct i2c_client *client) > +static int max17040_raw_soc_to_percent(struct max17040_chip *chip, u16 soc) > { > - struct max17040_chip *chip = i2c_get_clientdata(client); > - u16 soc; > + int shift = 8; > > - soc = max17040_read_reg(client, MAX17040_SOC); > + if (chip->quirk_double_soc) > + shift += 1; > > - chip->soc = (soc >> 8); > + return soc >> shift; > } > > -static void max17040_get_version(struct i2c_client *client) > +static int max17040_get_soc(struct max17040_chip *chip) > { > - u16 version; > + u32 soc; > > - version = max17040_read_reg(client, MAX17040_VER); > + regmap_read(chip->regmap, MAX17040_SOC, &soc); > > - dev_info(&client->dev, "MAX17040 Fuel-Gauge Ver 0x%x\n", version); > + return max17040_raw_soc_to_percent(chip, soc); > } > > -static void max17040_get_online(struct i2c_client *client) > +static void max17040_get_version(struct max17040_chip *chip) > { > - struct max17040_chip *chip = i2c_get_clientdata(client); > + u32 version; > > - if (chip->pdata && chip->pdata->battery_online) > - chip->online = chip->pdata->battery_online(); > - else > - chip->online = 1; > + regmap_read(chip->regmap, MAX17040_VER, &version); > + > + dev_dbg(&chip->client->dev, "MAX17040 Fuel-Gauge Ver 0x%x\n", version); > } > > -static void max17040_get_status(struct i2c_client *client) > +static int max17040_get_online(struct max17040_chip *chip) > { > - struct max17040_chip *chip = i2c_get_clientdata(client); > + return chip->pdata && chip->pdata->battery_online ? > + chip->pdata->battery_online() : 1; > +} > > +static int max17040_get_status(struct max17040_chip *chip) > +{ > if (!chip->pdata || !chip->pdata->charger_online > || !chip->pdata->charger_enable) { > - chip->status = POWER_SUPPLY_STATUS_UNKNOWN; > - return; > + return POWER_SUPPLY_STATUS_UNKNOWN; > } > > + if (max17040_get_soc(chip) > MAX17040_BATTERY_FULL) > + return POWER_SUPPLY_STATUS_FULL; > + > if (chip->pdata->charger_online()) { > if (chip->pdata->charger_enable()) > - chip->status = POWER_SUPPLY_STATUS_CHARGING; > + return POWER_SUPPLY_STATUS_CHARGING; > else > - chip->status = POWER_SUPPLY_STATUS_NOT_CHARGING; > + return POWER_SUPPLY_STATUS_NOT_CHARGING; > } else { > - chip->status = POWER_SUPPLY_STATUS_DISCHARGING; > + return POWER_SUPPLY_STATUS_DISCHARGING; > } > +} > + > +static int max17040_get_property(struct power_supply *psy, > + enum power_supply_property psp, > + union power_supply_propval *val) > +{ > + struct max17040_chip *chip = power_supply_get_drvdata(psy); > > - if (chip->soc > MAX17040_BATTERY_FULL) > - chip->status = POWER_SUPPLY_STATUS_FULL; > + switch (psp) { > + case POWER_SUPPLY_PROP_STATUS: > + val->intval = max17040_get_status(chip); > + break; > + case POWER_SUPPLY_PROP_ONLINE: > + val->intval = max17040_get_online(chip); > + break; > + case POWER_SUPPLY_PROP_VOLTAGE_NOW: > + val->intval = max17040_get_vcell(chip); > + break; > + case POWER_SUPPLY_PROP_CAPACITY: > + val->intval = max17040_get_soc(chip); > + break; > + default: > + return -EINVAL; > + } > + return 0; > } > > + > static int max17040_get_of_data(struct max17040_chip *chip) > { > struct device *dev = &chip->client->dev; > + struct chip_data *data = &max17040_family[ > + (enum chip_id) of_device_get_match_data(dev)]; > + int rcomp_len; > + u8 rcomp[2]; > > chip->low_soc_alert = MAX17040_ATHD_DEFAULT_POWER_UP; > device_property_read_u32(dev, > "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(dev, "maxim,alert-low-soc-level out of bounds\n"); > return -EINVAL; > + } > + > + chip->quirk_double_soc = device_property_read_bool(dev, > + "maxim,double-soc"); > + chip->quirk_skip_reset = device_property_read_bool(dev, > + "maxim,skip-reset"); > + rcomp_len = device_property_count_u8(dev, "maxim,rcomp"); > + chip->rcomp = MAX17040_RCOMP_DEFAULT; > + if (rcomp_len == data->rcomp_bytes) { > + device_property_read_u8_array(dev, "maxim,rcomp", > + rcomp, rcomp_len); > + chip->rcomp = rcomp_len == 2 ? > + rcomp[0] << 8 | rcomp[1] : > + rcomp[0] << 8; > + } else if (rcomp_len > 0) { > + dev_err(dev, "maxim,rcomp has incorrect length\n"); > + return -EINVAL; > + } > > return 0; > } > > -static void max17040_check_changes(struct i2c_client *client) > +static void max17040_check_changes(struct max17040_chip *chip) > { > - max17040_get_vcell(client); > - max17040_get_soc(client); > - max17040_get_online(client); > - max17040_get_status(client); > + chip->soc = max17040_get_soc(chip); > + chip->status = max17040_get_status(chip); > +} > + > +static void max17040_queue_work(struct max17040_chip *chip) > +{ > + queue_delayed_work(system_power_efficient_wq, &chip->work, > + MAX17040_DELAY); > } > > static void max17040_work(struct work_struct *work) > @@ -214,35 +363,42 @@ static void max17040_work(struct work_struct *work) > /* store SOC and status to check changes */ > last_soc = chip->soc; > last_status = chip->status; > - max17040_check_changes(chip->client); > + max17040_check_changes(chip); > > /* check changes and send uevent */ > if (last_soc != chip->soc || last_status != chip->status) > power_supply_changed(chip->battery); > > - queue_delayed_work(system_power_efficient_wq, &chip->work, > - MAX17040_DELAY); > + max17040_queue_work(chip); > +} > + > +static void max17040_stop_work(void *data) > +{ > + struct max17040_chip *chip = data; > + > + cancel_delayed_work_sync(&chip->work); > } > > static irqreturn_t max17040_thread_handler(int id, void *dev) > { > struct max17040_chip *chip = dev; > - struct i2c_client *client = chip->client; > > - dev_warn(&client->dev, "IRQ: Alert battery low level"); > + if (!(chip->data.has_soc_alert && max17040_handle_soc_alert(chip))) > + dev_warn(&chip->client->dev, "IRQ: Alert battery low level\n"); > + > /* read registers */ > - max17040_check_changes(chip->client); > + max17040_check_changes(chip); > > /* send uevent */ > power_supply_changed(chip->battery); > > - /* reset alert bit */ > - max17040_set_low_soc_alert(client, chip->low_soc_alert); > + /* reset low soc alert bit */ > + max17040_set_low_soc_alert(chip, chip->low_soc_alert); > > return IRQ_HANDLED; > } > > -static int max17040_enable_alert_irq(struct max17040_chip *chip) > +static int max17040_setup_irq(struct max17040_chip *chip) > { > struct i2c_client *client = chip->client; > unsigned int flags; > @@ -256,6 +412,13 @@ static int max17040_enable_alert_irq(struct max17040_chip *chip) > return ret; > } > > +static const struct regmap_config max17040_regmap = { > + .reg_bits = 8, > + .reg_stride = 2, > + .val_bits = 16, > + .val_format_endian = REGMAP_ENDIAN_BIG, > +}; > + > static enum power_supply_property max17040_battery_props[] = { > POWER_SUPPLY_PROP_STATUS, > POWER_SUPPLY_PROP_ONLINE, > @@ -277,6 +440,8 @@ static int max17040_probe(struct i2c_client *client, > struct i2c_adapter *adapter = client->adapter; > struct power_supply_config psy_cfg = {}; > struct max17040_chip *chip; > + enum chip_id chip_id; > + bool enable_irq = false; > int ret; > > if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE)) > @@ -287,38 +452,63 @@ static int max17040_probe(struct i2c_client *client, > return -ENOMEM; > > chip->client = client; > + chip->regmap = devm_regmap_init_i2c(client, &max17040_regmap); > 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"); > - return ret; > + chip_id = (enum chip_id) id->driver_data; > + if (client->dev.of_node) { > + ret = max17040_get_of_data(chip); > + if (ret) > + return ret; > + chip_id = (enum chip_id) of_device_get_match_data(&client->dev); > } > + chip->data = max17040_family[chip_id]; > > i2c_set_clientdata(client, chip); > psy_cfg.drv_data = chip; > > - chip->battery = power_supply_register(&client->dev, > - &max17040_battery_desc, &psy_cfg); > + chip->battery = devm_power_supply_register(&client->dev, > + &max17040_battery_desc, &psy_cfg); > if (IS_ERR(chip->battery)) { > dev_err(&client->dev, "failed: power supply register\n"); > return PTR_ERR(chip->battery); > } > > - max17040_reset(client); > - max17040_get_version(client); > + if (!chip->quirk_skip_reset) { > + max17040_reset(chip); > + } > + max17040_get_version(chip); > + max17040_set_rcomp(chip, chip->rcomp); > > /* check interrupt */ > - if (client->irq && of_device_is_compatible(client->dev.of_node, > - "maxim,max77836-battery")) { > - ret = max17040_set_low_soc_alert(client, chip->low_soc_alert); > + if (client->irq && chip->data.has_low_soc_alert) { > + ret = max17040_set_low_soc_alert(chip, chip->low_soc_alert); > if (ret) { > dev_err(&client->dev, > "Failed to set low SOC alert: err %d\n", ret); > return ret; > } > + enable_irq = true; > + } > + > + if (client->irq && chip->data.has_soc_alert) { > + ret = max17040_set_soc_alert(chip, 1); > + if (ret) { > + dev_err(&client->dev, > + "Failed to set SOC alert: err %d\n", ret); > + return ret; > + } > + enable_irq = true; > + } else { > + /* soc alerts negate the need for polling */ > + INIT_DEFERRABLE_WORK(&chip->work, max17040_work); > + ret = devm_add_action(&client->dev, max17040_stop_work, chip); > + if (ret) > + return ret; > + max17040_queue_work(chip); > + } > > - ret = max17040_enable_alert_irq(chip); > + if (enable_irq) { > + ret = max17040_setup_irq(chip); > if (ret) { > client->irq = 0; > dev_warn(&client->dev, > @@ -326,19 +516,6 @@ static int max17040_probe(struct i2c_client *client, > } > } > > - INIT_DEFERRABLE_WORK(&chip->work, max17040_work); > - queue_delayed_work(system_power_efficient_wq, &chip->work, > - MAX17040_DELAY); > - > - return 0; > -} > - > -static int max17040_remove(struct i2c_client *client) > -{ > - struct max17040_chip *chip = i2c_get_clientdata(client); > - > - power_supply_unregister(chip->battery); > - cancel_delayed_work(&chip->work); > return 0; > } > > @@ -349,10 +526,13 @@ static int max17040_suspend(struct device *dev) > struct i2c_client *client = to_i2c_client(dev); > struct max17040_chip *chip = i2c_get_clientdata(client); > > - cancel_delayed_work(&chip->work); > - > if (client->irq && device_may_wakeup(dev)) > enable_irq_wake(client->irq); > + if (client->irq && chip->data.has_soc_alert) > + // disable soc alert to prevent wakeup > + max17040_set_soc_alert(chip, 0); > + else > + cancel_delayed_work(&chip->work); > > return 0; > } > @@ -362,11 +542,12 @@ static int max17040_resume(struct device *dev) > struct i2c_client *client = to_i2c_client(dev); > struct max17040_chip *chip = i2c_get_clientdata(client); > > - queue_delayed_work(system_power_efficient_wq, &chip->work, > - MAX17040_DELAY); > - > if (client->irq && device_may_wakeup(dev)) > disable_irq_wake(client->irq); > + if (client->irq && chip->data.has_soc_alert) > + max17040_set_soc_alert(chip, 1); > + else > + max17040_queue_work(chip); > > return 0; > } > @@ -381,16 +562,30 @@ static SIMPLE_DEV_PM_OPS(max17040_pm_ops, max17040_suspend, max17040_resume); > #endif /* CONFIG_PM_SLEEP */ > > static const struct i2c_device_id max17040_id[] = { > - { "max17040" }, > - { "max77836-battery" }, > - { } > + { "max17040", ID_MAX17040 }, > + { "max17041", ID_MAX17041 }, > + { "max17043", ID_MAX17043 }, > + { "max77836-battery", ID_MAX17043 }, > + { "max17044", ID_MAX17044 }, > + { "max17048", ID_MAX17048 }, > + { "max17049", ID_MAX17049 }, > + { "max17058", ID_MAX17058 }, > + { "max17059", ID_MAX17059 }, > + { /* sentinel */ } > }; > MODULE_DEVICE_TABLE(i2c, max17040_id); > > static const struct of_device_id max17040_of_match[] = { > - { .compatible = "maxim,max17040" }, > - { .compatible = "maxim,max77836-battery" }, > - { }, > + { .compatible = "maxim,max17040", .data = (void *) ID_MAX17040 }, > + { .compatible = "maxim,max17041", .data = (void *) ID_MAX17041 }, > + { .compatible = "maxim,max17043", .data = (void *) ID_MAX17043 }, > + { .compatible = "maxim,max77836-battery", .data = (void *) ID_MAX17043 }, > + { .compatible = "maxim,max17044", .data = (void *) ID_MAX17044 }, > + { .compatible = "maxim,max17048", .data = (void *) ID_MAX17048 }, > + { .compatible = "maxim,max17049", .data = (void *) ID_MAX17049 }, > + { .compatible = "maxim,max17058", .data = (void *) ID_MAX17058 }, > + { .compatible = "maxim,max17059", .data = (void *) ID_MAX17059 }, > + { /* sentinel */ }, > }; > MODULE_DEVICE_TABLE(of, max17040_of_match); > > @@ -401,7 +596,6 @@ static struct i2c_driver max17040_i2c_driver = { > .pm = MAX17040_PM_OPS, > }, > .probe = max17040_probe, > - .remove = max17040_remove, > .id_table = max17040_id, > }; > module_i2c_driver(max17040_i2c_driver); > -- > 2.27.0 >
On 6/19/20 6:59 PM, Sebastian Reichel wrote: > Hi, > > On Thu, Jun 18, 2020 at 01:13:39PM +0300, Iskren Chernev wrote: >> Maxim max17040 is a fuel gauge from a larger family utilising the Model >> Gauge technology. Document all different compatible strings that the >> max17040 driver recognizes. >> >> Some devices in the wild report double the capacity. The >> maxim,double-soc (from State-Of-Charge) property fixes that. >> >> Complete device reset might lead to very inaccurate readings. Specify >> maxim,skip-reset to avoid that. >> >> To compensate for the battery chemistry and operating conditions the >> chips support a compensation value. Specify one or two byte compensation >> via the maxim,rcomp byte array. >> >> Signed-off-by: Iskren Chernev <iskren.chernev@gmail.com> >> --- >> v1: https://lkml.org/lkml/2020/6/8/682 >> >> Changes in v2: >> - add maxim,skip-reset >> - remove 2 byte rcomp from example, the specified compat string supports 1 byte >> rcomp >> >> .../power/supply/max17040_battery.txt | 24 ++++++++++++++++++- >> 1 file changed, 23 insertions(+), 1 deletion(-) >> >> diff --git a/Documentation/devicetree/bindings/power/supply/max17040_battery.txt b/Documentation/devicetree/bindings/power/supply/max17040_battery.txt >> index 4e0186b8380fa..3ee91c295027f 100644 >> --- a/Documentation/devicetree/bindings/power/supply/max17040_battery.txt >> +++ b/Documentation/devicetree/bindings/power/supply/max17040_battery.txt >> @@ -2,7 +2,9 @@ max17040_battery >> ~~~~~~~~~~~~~~~~ >> >> Required properties : >> - - compatible : "maxim,max17040" or "maxim,max77836-battery" >> + - compatible : "maxim,max17040", "maxim,max17041", "maxim,max17043", >> + "maxim,max17044", "maxim,max17048", "maxim,max17049", >> + "maxim,max17058", "maxim,max17059" or "maxim,max77836-battery" >> - reg: i2c slave address >> >> Optional properties : >> @@ -11,6 +13,18 @@ Optional properties : >> generated. Can be configured from 1 up to 32 >> (%). If skipped the power up default value of >> 4 (%) will be used. >> +- maxim,double-soc : Certain devices return double the capacity. >> + Specify this boolean property to divide the >> + reported value in 2 and thus normalize it. >> + SOC == State of Charge == Capacity. > > Can this be derived from the compatible? > So far, I'm aware of 2 devices using max17048 that need this setting. That would be 100% of the 17048 devices I know of [1]. At the same time, according to the max17048 documentation this is not the case. [1] These are the Samsung Galaxy S5 (klte) and the LG Nexus 5 (hammerhead). >> +- maxim,skip-reset : Do not reset device on driver initialization. >> + Some devices report extremely inaccurately after >> + a hard reset. > > Same question. > > -- Sebastian > This is even weirder. There is no mention in the documentation about whether the device should be reset on boot (because the fuelgauge is on even during device off times). Testing on 17040 and 17043 reveals no issues with resetting on boot, but on the 17048 I have; the readings are up to 2x wrong if you do reset on boot. Not doing a reset *might* leave the device in a state that is not 100% known by the driver, but I don't know of any real world problems stemming from that. So in a sense, I can apply both of these quirks for 17048, and it will work for all devices I have tested on, but it won't follow the spec. That is why I decided to mark them as special behavior needing configuration per use case. On the other hand, I can incorporate them in 17048, and if someone complains the bindings can be introduced later (because they are stable API and introducing them now is a bit over engineered). >> +- maxim,rcomp : A value to compensate readings for various >> + battery chemistries and operating temperatures. >> + max17040,41 have 2 byte rcomp, default to >> + 0x97 0x00. All other devices have one byte >> + rcomp, default to 0x97. >> - interrupts : Interrupt line see Documentation/devicetree/ >> bindings/interrupt-controller/interrupts.txt >> - wakeup-source : This device has wakeup capabilities. Use this >> @@ -31,3 +45,11 @@ Example: >> interrupts = <2 IRQ_TYPE_EDGE_FALLING>; >> wakeup-source; >> }; >> + >> + battery-fuel-gauge@36 { >> + compatible = "maxim,max17048"; >> + reg = <0x36>; >> + maxim,rcomp = /bits/ 8 <0x97>; >> + maxim,alert-low-soc-level = <10>; >> + maxim,double-soc; >> + }; >> >> base-commit: 1713116fa907cc7290020f0d8632ec646d2936f8 >> -- >> 2.27.0 >> P.S sorry for the double email, forgot reply-all
On 6/19/20 7:13 PM, Sebastian Reichel wrote: > Hi, > > On Thu, Jun 18, 2020 at 01:13:40PM +0300, Iskren Chernev wrote: >> The max17040 fuel gauge is part of a family of 8 chips that have very >> similar mode of operations and registers. >> >> This change adds: >> - compatible strings for all supported devices and handles the minor >> differences between them; >> - handling for devices reporting double capacity via maxim,double-soc; >> - handling for setting rcomp, a compensation value for more accurate >> reading, affected by battery chemistry and operating temps; >> - suppot for SOC alerts (capacity changes by +/- 1%), to prevent polling >> every second; >> - redo i2c read/writes via regmap > > Please split this into multiple patches as described in > Documentation/process/submitting-patches.rst: > > 3) Separate your changes > ------------------------ > > Separate each **logical change** into a separate patch. So I did some cleanup on the existing code (unified function interfaces, introduced regmap, and other small things). Can this be one commit? > Thanks, > > -- Sebastian > >> The datasheets of the supported devices are linked [0] [1] [2] [3]. >> >> [0] https://datasheets.maximintegrated.com/en/ds/MAX17040-MAX17041.pdf >> [1] https://datasheets.maximintegrated.com/en/ds/MAX17043-MAX17044.pdf >> [2] https://datasheets.maximintegrated.com/en/ds/MAX17048-MAX17049.pdf >> [3] https://datasheets.maximintegrated.com/en/ds/MAX17058-MAX17059.pdf >> >> Signed-off-by: Iskren Chernev <iskren.chernev@gmail.com> >> --- >> v1: https://lkml.org/lkml/2020/6/8/683 >> >> changes in v2: >> - make max17040_family static >> - fix rcomp len check >> - implement maxim,skip-reset >> - reword Kconfig text >> >> drivers/power/supply/Kconfig | 11 +- >> drivers/power/supply/max17040_battery.c | 466 +++++++++++++++++------- >> 2 files changed, 338 insertions(+), 139 deletions(-) >> >> diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig >> index 44d3c8512fb8d..69e9eaa100d9f 100644 >> --- a/drivers/power/supply/Kconfig >> +++ b/drivers/power/supply/Kconfig >> @@ -368,9 +368,14 @@ config BATTERY_MAX17040 >> tristate "Maxim MAX17040 Fuel Gauge" >> depends on I2C >> help >> - MAX17040 is fuel-gauge systems for lithium-ion (Li+) batteries >> - in handheld and portable equipment. The MAX17040 is configured >> - to operate with a single lithium cell >> + Maxim models with ModelGauge are fuel-gauge systems for lithium-ion >> + (Li+) batteries in handheld and portable equipment, including >> + max17040, max17041, max17043, max17044, max17048, max17049, max17058, >> + max17059. It is also included in some batteries like max77836. >> + >> + Driver supports reporting SOC (State of Charge, i.e capacity), >> + voltage and configurable low-SOC wakeup interrupt. For certain models >> + a SOC change interrupt is also supported. >> >> config BATTERY_MAX17042 >> tristate "Maxim MAX17042/17047/17050/8997/8966 Fuel Gauge" >> diff --git a/drivers/power/supply/max17040_battery.c b/drivers/power/supply/max17040_battery.c >> index 48aa44665e2f1..1244982cacacb 100644 >> --- a/drivers/power/supply/max17040_battery.c >> +++ b/drivers/power/supply/max17040_battery.c >> @@ -15,193 +15,342 @@ >> #include <linux/delay.h> >> #include <linux/interrupt.h> >> #include <linux/power_supply.h> >> +#include <linux/of_device.h> >> #include <linux/max17040_battery.h> >> +#include <linux/regmap.h> >> #include <linux/slab.h> >> >> #define MAX17040_VCELL 0x02 >> #define MAX17040_SOC 0x04 >> #define MAX17040_MODE 0x06 >> #define MAX17040_VER 0x08 >> -#define MAX17040_RCOMP 0x0C >> +#define MAX17040_CONFIG 0x0C >> +#define MAX17040_STATUS 0x1A >> #define MAX17040_CMD 0xFE >> >> >> #define MAX17040_DELAY 1000 >> #define MAX17040_BATTERY_FULL 95 >> +#define MAX17040_RCOMP_DEFAULT 0x9700 >> >> -#define MAX17040_ATHD_MASK 0xFFC0 >> +#define MAX17040_ATHD_MASK 0x3f >> +#define MAX17040_ALSC_MASK 0x40 >> #define MAX17040_ATHD_DEFAULT_POWER_UP 4 >> +#define MAX17040_STATUS_HD_MASK 0x1000 >> +#define MAX17040_STATUS_SC_MASK 0x2000 >> +#define MAX17040_CFG_RCOMP_MASK 0xff00 >> + >> +enum chip_id { >> + ID_MAX17040, >> + ID_MAX17041, >> + ID_MAX17043, >> + ID_MAX17044, >> + ID_MAX17048, >> + ID_MAX17049, >> + ID_MAX17058, >> + ID_MAX17059, >> +}; >> + >> +/* values that differ by chip_id */ >> +struct chip_data { >> + u16 reset_val; >> + u16 vcell_shift; >> + u16 vcell_mul; >> + u16 vcell_div; >> + u8 rcomp_bytes; >> + u8 has_low_soc_alert; >> + u8 has_soc_alert; >> +}; >> + >> +static struct chip_data max17040_family[] = { >> + [ID_MAX17040] = { >> + .reset_val = 0x0054, >> + .vcell_shift = 4, >> + .vcell_mul = 1250, >> + .vcell_div = 1, >> + .rcomp_bytes = 2, >> + .has_low_soc_alert = 0, >> + .has_soc_alert = 0, >> + }, >> + [ID_MAX17041] = { >> + .reset_val = 0x0054, >> + .vcell_shift = 4, >> + .vcell_mul = 2500, >> + .vcell_div = 1, >> + .rcomp_bytes = 2, >> + .has_low_soc_alert = 0, >> + .has_soc_alert = 0, >> + }, >> + [ID_MAX17043] = { >> + .reset_val = 0x0054, >> + .vcell_shift = 4, >> + .vcell_mul = 1250, >> + .vcell_div = 1, >> + .rcomp_bytes = 1, >> + .has_low_soc_alert = 1, >> + .has_soc_alert = 0, >> + }, >> + [ID_MAX17044] = { >> + .reset_val = 0x0054, >> + .vcell_shift = 4, >> + .vcell_mul = 2500, >> + .vcell_div = 1, >> + .rcomp_bytes = 1, >> + .has_low_soc_alert = 1, >> + .has_soc_alert = 0, >> + }, >> + [ID_MAX17048] = { >> + .reset_val = 0x5400, >> + .vcell_shift = 0, >> + .vcell_mul = 625, >> + .vcell_div = 8, >> + .rcomp_bytes = 1, >> + .has_low_soc_alert = 1, >> + .has_soc_alert = 1, >> + }, >> + [ID_MAX17049] = { >> + .reset_val = 0x5400, >> + .vcell_shift = 0, >> + .vcell_mul = 625, >> + .vcell_div = 4, >> + .rcomp_bytes = 1, >> + .has_low_soc_alert = 1, >> + .has_soc_alert = 1, >> + }, >> + [ID_MAX17058] = { >> + .reset_val = 0x5400, >> + .vcell_shift = 0, >> + .vcell_mul = 625, >> + .vcell_div = 8, >> + .rcomp_bytes = 1, >> + .has_low_soc_alert = 1, >> + .has_soc_alert = 0, >> + }, >> + [ID_MAX17059] = { >> + .reset_val = 0x5400, >> + .vcell_shift = 0, >> + .vcell_mul = 625, >> + .vcell_div = 4, >> + .rcomp_bytes = 1, >> + .has_low_soc_alert = 1, >> + .has_soc_alert = 0, >> + }, >> +}; >> >> struct max17040_chip { >> struct i2c_client *client; >> + struct regmap *regmap; >> struct delayed_work work; >> struct power_supply *battery; >> struct max17040_platform_data *pdata; >> + struct chip_data data; >> >> - /* State Of Connect */ >> - int online; >> - /* battery voltage */ >> - int vcell; >> /* battery capacity */ >> int soc; >> /* State Of Charge */ >> int status; >> + >> /* Low alert threshold from 32% to 1% of the State of Charge */ >> u32 low_soc_alert; >> + /* some devices return twice the capacity */ >> + bool quirk_double_soc; >> + /* resetting causes some devices to show inaccurate reading */ >> + bool quirk_skip_reset; >> + /* higher 8 bits for 17043+, 16 bits for 17040,41 */ >> + u16 rcomp; >> }; >> >> -static int max17040_get_property(struct power_supply *psy, >> - enum power_supply_property psp, >> - union power_supply_propval *val) >> +static int max17040_reset(struct max17040_chip *chip) >> { >> - struct max17040_chip *chip = power_supply_get_drvdata(psy); >> - >> - switch (psp) { >> - case POWER_SUPPLY_PROP_STATUS: >> - val->intval = chip->status; >> - break; >> - case POWER_SUPPLY_PROP_ONLINE: >> - val->intval = chip->online; >> - break; >> - case POWER_SUPPLY_PROP_VOLTAGE_NOW: >> - val->intval = chip->vcell; >> - break; >> - case POWER_SUPPLY_PROP_CAPACITY: >> - val->intval = chip->soc; >> - break; >> - default: >> - return -EINVAL; >> - } >> - return 0; >> + return regmap_write(chip->regmap, MAX17040_CMD, chip->data.reset_val); >> } >> >> -static int max17040_write_reg(struct i2c_client *client, int reg, u16 value) >> +static int max17040_set_low_soc_alert(struct max17040_chip *chip, u32 level) >> { >> - int ret; >> - >> - ret = i2c_smbus_write_word_swapped(client, reg, value); >> - >> - if (ret < 0) >> - dev_err(&client->dev, "%s: err %d\n", __func__, ret); >> + level = 32 - level; >> + return regmap_update_bits(chip->regmap, MAX17040_CONFIG, >> + MAX17040_ATHD_MASK, level); >> +} >> >> - return ret; >> +static int max17040_set_soc_alert(struct max17040_chip *chip, bool enable) >> +{ >> + return regmap_update_bits(chip->regmap, MAX17040_CONFIG, >> + MAX17040_ALSC_MASK, enable ? MAX17040_ALSC_MASK : 0); >> } >> >> -static int max17040_read_reg(struct i2c_client *client, int reg) >> +/* Returns true if alert cause was SOC change, not low SOC */ >> +static bool max17040_handle_soc_alert(struct max17040_chip *chip) >> { >> - int ret; >> + bool ret = true; >> + u32 data; >> >> - ret = i2c_smbus_read_word_swapped(client, reg); >> + regmap_read(chip->regmap, MAX17040_STATUS, &data); >> >> - if (ret < 0) >> - dev_err(&client->dev, "%s: err %d\n", __func__, ret); >> + if (data & MAX17040_STATUS_HD_MASK) { >> + // this alert was caused by low soc >> + ret = false; >> + } >> + if (data & MAX17040_STATUS_SC_MASK) { >> + // soc change bit -- deassert to mark as handled >> + regmap_write(chip->regmap, MAX17040_STATUS, >> + data & ~MAX17040_STATUS_SC_MASK); >> + } >> >> return ret; >> } >> >> -static void max17040_reset(struct i2c_client *client) >> +static int max17040_set_rcomp(struct max17040_chip *chip, u16 rcomp) >> { >> - max17040_write_reg(client, MAX17040_CMD, 0x0054); >> + u16 mask = chip->data.rcomp_bytes == 2 ? >> + 0xffff : MAX17040_CFG_RCOMP_MASK; >> + >> + return regmap_update_bits(chip->regmap, MAX17040_CONFIG, mask, rcomp); >> } >> >> -static int max17040_set_low_soc_alert(struct i2c_client *client, u32 level) >> +static int max17040_raw_vcell_to_uvolts(struct max17040_chip *chip, u16 vcell) >> { >> - int ret; >> - u16 data; >> - >> - level = 32 - level; >> - data = max17040_read_reg(client, MAX17040_RCOMP); >> - /* clear the alrt bit and set LSb 5 bits */ >> - data &= MAX17040_ATHD_MASK; >> - data |= level; >> - ret = max17040_write_reg(client, MAX17040_RCOMP, data); >> + struct chip_data *d = &chip->data; >> >> - return ret; >> + return (vcell >> d->vcell_shift) * d->vcell_mul / d->vcell_div; >> } >> >> -static void max17040_get_vcell(struct i2c_client *client) >> +static int max17040_get_vcell(struct max17040_chip *chip) >> { >> - struct max17040_chip *chip = i2c_get_clientdata(client); >> - u16 vcell; >> + u32 vcell; >> >> - vcell = max17040_read_reg(client, MAX17040_VCELL); >> + regmap_read(chip->regmap, MAX17040_VCELL, &vcell); >> >> - chip->vcell = (vcell >> 4) * 1250; >> + return max17040_raw_vcell_to_uvolts(chip, vcell); >> } >> >> -static void max17040_get_soc(struct i2c_client *client) >> +static int max17040_raw_soc_to_percent(struct max17040_chip *chip, u16 soc) >> { >> - struct max17040_chip *chip = i2c_get_clientdata(client); >> - u16 soc; >> + int shift = 8; >> >> - soc = max17040_read_reg(client, MAX17040_SOC); >> + if (chip->quirk_double_soc) >> + shift += 1; >> >> - chip->soc = (soc >> 8); >> + return soc >> shift; >> } >> >> -static void max17040_get_version(struct i2c_client *client) >> +static int max17040_get_soc(struct max17040_chip *chip) >> { >> - u16 version; >> + u32 soc; >> >> - version = max17040_read_reg(client, MAX17040_VER); >> + regmap_read(chip->regmap, MAX17040_SOC, &soc); >> >> - dev_info(&client->dev, "MAX17040 Fuel-Gauge Ver 0x%x\n", version); >> + return max17040_raw_soc_to_percent(chip, soc); >> } >> >> -static void max17040_get_online(struct i2c_client *client) >> +static void max17040_get_version(struct max17040_chip *chip) >> { >> - struct max17040_chip *chip = i2c_get_clientdata(client); >> + u32 version; >> >> - if (chip->pdata && chip->pdata->battery_online) >> - chip->online = chip->pdata->battery_online(); >> - else >> - chip->online = 1; >> + regmap_read(chip->regmap, MAX17040_VER, &version); >> + >> + dev_dbg(&chip->client->dev, "MAX17040 Fuel-Gauge Ver 0x%x\n", version); >> } >> >> -static void max17040_get_status(struct i2c_client *client) >> +static int max17040_get_online(struct max17040_chip *chip) >> { >> - struct max17040_chip *chip = i2c_get_clientdata(client); >> + return chip->pdata && chip->pdata->battery_online ? >> + chip->pdata->battery_online() : 1; >> +} >> >> +static int max17040_get_status(struct max17040_chip *chip) >> +{ >> if (!chip->pdata || !chip->pdata->charger_online >> || !chip->pdata->charger_enable) { >> - chip->status = POWER_SUPPLY_STATUS_UNKNOWN; >> - return; >> + return POWER_SUPPLY_STATUS_UNKNOWN; >> } >> >> + if (max17040_get_soc(chip) > MAX17040_BATTERY_FULL) >> + return POWER_SUPPLY_STATUS_FULL; >> + >> if (chip->pdata->charger_online()) { >> if (chip->pdata->charger_enable()) >> - chip->status = POWER_SUPPLY_STATUS_CHARGING; >> + return POWER_SUPPLY_STATUS_CHARGING; >> else >> - chip->status = POWER_SUPPLY_STATUS_NOT_CHARGING; >> + return POWER_SUPPLY_STATUS_NOT_CHARGING; >> } else { >> - chip->status = POWER_SUPPLY_STATUS_DISCHARGING; >> + return POWER_SUPPLY_STATUS_DISCHARGING; >> } >> +} >> + >> +static int max17040_get_property(struct power_supply *psy, >> + enum power_supply_property psp, >> + union power_supply_propval *val) >> +{ >> + struct max17040_chip *chip = power_supply_get_drvdata(psy); >> >> - if (chip->soc > MAX17040_BATTERY_FULL) >> - chip->status = POWER_SUPPLY_STATUS_FULL; >> + switch (psp) { >> + case POWER_SUPPLY_PROP_STATUS: >> + val->intval = max17040_get_status(chip); >> + break; >> + case POWER_SUPPLY_PROP_ONLINE: >> + val->intval = max17040_get_online(chip); >> + break; >> + case POWER_SUPPLY_PROP_VOLTAGE_NOW: >> + val->intval = max17040_get_vcell(chip); >> + break; >> + case POWER_SUPPLY_PROP_CAPACITY: >> + val->intval = max17040_get_soc(chip); >> + break; >> + default: >> + return -EINVAL; >> + } >> + return 0; >> } >> >> + >> static int max17040_get_of_data(struct max17040_chip *chip) >> { >> struct device *dev = &chip->client->dev; >> + struct chip_data *data = &max17040_family[ >> + (enum chip_id) of_device_get_match_data(dev)]; >> + int rcomp_len; >> + u8 rcomp[2]; >> >> chip->low_soc_alert = MAX17040_ATHD_DEFAULT_POWER_UP; >> device_property_read_u32(dev, >> "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(dev, "maxim,alert-low-soc-level out of bounds\n"); >> return -EINVAL; >> + } >> + >> + chip->quirk_double_soc = device_property_read_bool(dev, >> + "maxim,double-soc"); >> + chip->quirk_skip_reset = device_property_read_bool(dev, >> + "maxim,skip-reset"); >> + rcomp_len = device_property_count_u8(dev, "maxim,rcomp"); >> + chip->rcomp = MAX17040_RCOMP_DEFAULT; >> + if (rcomp_len == data->rcomp_bytes) { >> + device_property_read_u8_array(dev, "maxim,rcomp", >> + rcomp, rcomp_len); >> + chip->rcomp = rcomp_len == 2 ? >> + rcomp[0] << 8 | rcomp[1] : >> + rcomp[0] << 8; >> + } else if (rcomp_len > 0) { >> + dev_err(dev, "maxim,rcomp has incorrect length\n"); >> + return -EINVAL; >> + } >> >> return 0; >> } >> >> -static void max17040_check_changes(struct i2c_client *client) >> +static void max17040_check_changes(struct max17040_chip *chip) >> { >> - max17040_get_vcell(client); >> - max17040_get_soc(client); >> - max17040_get_online(client); >> - max17040_get_status(client); >> + chip->soc = max17040_get_soc(chip); >> + chip->status = max17040_get_status(chip); >> +} >> + >> +static void max17040_queue_work(struct max17040_chip *chip) >> +{ >> + queue_delayed_work(system_power_efficient_wq, &chip->work, >> + MAX17040_DELAY); >> } >> >> static void max17040_work(struct work_struct *work) >> @@ -214,35 +363,42 @@ static void max17040_work(struct work_struct *work) >> /* store SOC and status to check changes */ >> last_soc = chip->soc; >> last_status = chip->status; >> - max17040_check_changes(chip->client); >> + max17040_check_changes(chip); >> >> /* check changes and send uevent */ >> if (last_soc != chip->soc || last_status != chip->status) >> power_supply_changed(chip->battery); >> >> - queue_delayed_work(system_power_efficient_wq, &chip->work, >> - MAX17040_DELAY); >> + max17040_queue_work(chip); >> +} >> + >> +static void max17040_stop_work(void *data) >> +{ >> + struct max17040_chip *chip = data; >> + >> + cancel_delayed_work_sync(&chip->work); >> } >> >> static irqreturn_t max17040_thread_handler(int id, void *dev) >> { >> struct max17040_chip *chip = dev; >> - struct i2c_client *client = chip->client; >> >> - dev_warn(&client->dev, "IRQ: Alert battery low level"); >> + if (!(chip->data.has_soc_alert && max17040_handle_soc_alert(chip))) >> + dev_warn(&chip->client->dev, "IRQ: Alert battery low level\n"); >> + >> /* read registers */ >> - max17040_check_changes(chip->client); >> + max17040_check_changes(chip); >> >> /* send uevent */ >> power_supply_changed(chip->battery); >> >> - /* reset alert bit */ >> - max17040_set_low_soc_alert(client, chip->low_soc_alert); >> + /* reset low soc alert bit */ >> + max17040_set_low_soc_alert(chip, chip->low_soc_alert); >> >> return IRQ_HANDLED; >> } >> >> -static int max17040_enable_alert_irq(struct max17040_chip *chip) >> +static int max17040_setup_irq(struct max17040_chip *chip) >> { >> struct i2c_client *client = chip->client; >> unsigned int flags; >> @@ -256,6 +412,13 @@ static int max17040_enable_alert_irq(struct max17040_chip *chip) >> return ret; >> } >> >> +static const struct regmap_config max17040_regmap = { >> + .reg_bits = 8, >> + .reg_stride = 2, >> + .val_bits = 16, >> + .val_format_endian = REGMAP_ENDIAN_BIG, >> +}; >> + >> static enum power_supply_property max17040_battery_props[] = { >> POWER_SUPPLY_PROP_STATUS, >> POWER_SUPPLY_PROP_ONLINE, >> @@ -277,6 +440,8 @@ static int max17040_probe(struct i2c_client *client, >> struct i2c_adapter *adapter = client->adapter; >> struct power_supply_config psy_cfg = {}; >> struct max17040_chip *chip; >> + enum chip_id chip_id; >> + bool enable_irq = false; >> int ret; >> >> if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE)) >> @@ -287,38 +452,63 @@ static int max17040_probe(struct i2c_client *client, >> return -ENOMEM; >> >> chip->client = client; >> + chip->regmap = devm_regmap_init_i2c(client, &max17040_regmap); >> 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"); >> - return ret; >> + chip_id = (enum chip_id) id->driver_data; >> + if (client->dev.of_node) { >> + ret = max17040_get_of_data(chip); >> + if (ret) >> + return ret; >> + chip_id = (enum chip_id) of_device_get_match_data(&client->dev); >> } >> + chip->data = max17040_family[chip_id]; >> >> i2c_set_clientdata(client, chip); >> psy_cfg.drv_data = chip; >> >> - chip->battery = power_supply_register(&client->dev, >> - &max17040_battery_desc, &psy_cfg); >> + chip->battery = devm_power_supply_register(&client->dev, >> + &max17040_battery_desc, &psy_cfg); >> if (IS_ERR(chip->battery)) { >> dev_err(&client->dev, "failed: power supply register\n"); >> return PTR_ERR(chip->battery); >> } >> >> - max17040_reset(client); >> - max17040_get_version(client); >> + if (!chip->quirk_skip_reset) { >> + max17040_reset(chip); >> + } >> + max17040_get_version(chip); >> + max17040_set_rcomp(chip, chip->rcomp); >> >> /* check interrupt */ >> - if (client->irq && of_device_is_compatible(client->dev.of_node, >> - "maxim,max77836-battery")) { >> - ret = max17040_set_low_soc_alert(client, chip->low_soc_alert); >> + if (client->irq && chip->data.has_low_soc_alert) { >> + ret = max17040_set_low_soc_alert(chip, chip->low_soc_alert); >> if (ret) { >> dev_err(&client->dev, >> "Failed to set low SOC alert: err %d\n", ret); >> return ret; >> } >> + enable_irq = true; >> + } >> + >> + if (client->irq && chip->data.has_soc_alert) { >> + ret = max17040_set_soc_alert(chip, 1); >> + if (ret) { >> + dev_err(&client->dev, >> + "Failed to set SOC alert: err %d\n", ret); >> + return ret; >> + } >> + enable_irq = true; >> + } else { >> + /* soc alerts negate the need for polling */ >> + INIT_DEFERRABLE_WORK(&chip->work, max17040_work); >> + ret = devm_add_action(&client->dev, max17040_stop_work, chip); >> + if (ret) >> + return ret; >> + max17040_queue_work(chip); >> + } >> >> - ret = max17040_enable_alert_irq(chip); >> + if (enable_irq) { >> + ret = max17040_setup_irq(chip); >> if (ret) { >> client->irq = 0; >> dev_warn(&client->dev, >> @@ -326,19 +516,6 @@ static int max17040_probe(struct i2c_client *client, >> } >> } >> >> - INIT_DEFERRABLE_WORK(&chip->work, max17040_work); >> - queue_delayed_work(system_power_efficient_wq, &chip->work, >> - MAX17040_DELAY); >> - >> - return 0; >> -} >> - >> -static int max17040_remove(struct i2c_client *client) >> -{ >> - struct max17040_chip *chip = i2c_get_clientdata(client); >> - >> - power_supply_unregister(chip->battery); >> - cancel_delayed_work(&chip->work); >> return 0; >> } >> >> @@ -349,10 +526,13 @@ static int max17040_suspend(struct device *dev) >> struct i2c_client *client = to_i2c_client(dev); >> struct max17040_chip *chip = i2c_get_clientdata(client); >> >> - cancel_delayed_work(&chip->work); >> - >> if (client->irq && device_may_wakeup(dev)) >> enable_irq_wake(client->irq); >> + if (client->irq && chip->data.has_soc_alert) >> + // disable soc alert to prevent wakeup >> + max17040_set_soc_alert(chip, 0); >> + else >> + cancel_delayed_work(&chip->work); >> >> return 0; >> } >> @@ -362,11 +542,12 @@ static int max17040_resume(struct device *dev) >> struct i2c_client *client = to_i2c_client(dev); >> struct max17040_chip *chip = i2c_get_clientdata(client); >> >> - queue_delayed_work(system_power_efficient_wq, &chip->work, >> - MAX17040_DELAY); >> - >> if (client->irq && device_may_wakeup(dev)) >> disable_irq_wake(client->irq); >> + if (client->irq && chip->data.has_soc_alert) >> + max17040_set_soc_alert(chip, 1); >> + else >> + max17040_queue_work(chip); >> >> return 0; >> } >> @@ -381,16 +562,30 @@ static SIMPLE_DEV_PM_OPS(max17040_pm_ops, max17040_suspend, max17040_resume); >> #endif /* CONFIG_PM_SLEEP */ >> >> static const struct i2c_device_id max17040_id[] = { >> - { "max17040" }, >> - { "max77836-battery" }, >> - { } >> + { "max17040", ID_MAX17040 }, >> + { "max17041", ID_MAX17041 }, >> + { "max17043", ID_MAX17043 }, >> + { "max77836-battery", ID_MAX17043 }, >> + { "max17044", ID_MAX17044 }, >> + { "max17048", ID_MAX17048 }, >> + { "max17049", ID_MAX17049 }, >> + { "max17058", ID_MAX17058 }, >> + { "max17059", ID_MAX17059 }, >> + { /* sentinel */ } >> }; >> MODULE_DEVICE_TABLE(i2c, max17040_id); >> >> static const struct of_device_id max17040_of_match[] = { >> - { .compatible = "maxim,max17040" }, >> - { .compatible = "maxim,max77836-battery" }, >> - { }, >> + { .compatible = "maxim,max17040", .data = (void *) ID_MAX17040 }, >> + { .compatible = "maxim,max17041", .data = (void *) ID_MAX17041 }, >> + { .compatible = "maxim,max17043", .data = (void *) ID_MAX17043 }, >> + { .compatible = "maxim,max77836-battery", .data = (void *) ID_MAX17043 }, >> + { .compatible = "maxim,max17044", .data = (void *) ID_MAX17044 }, >> + { .compatible = "maxim,max17048", .data = (void *) ID_MAX17048 }, >> + { .compatible = "maxim,max17049", .data = (void *) ID_MAX17049 }, >> + { .compatible = "maxim,max17058", .data = (void *) ID_MAX17058 }, >> + { .compatible = "maxim,max17059", .data = (void *) ID_MAX17059 }, >> + { /* sentinel */ }, >> }; >> MODULE_DEVICE_TABLE(of, max17040_of_match); >> >> @@ -401,7 +596,6 @@ static struct i2c_driver max17040_i2c_driver = { >> .pm = MAX17040_PM_OPS, >> }, >> .probe = max17040_probe, >> - .remove = max17040_remove, >> .id_table = max17040_id, >> }; >> module_i2c_driver(max17040_i2c_driver); >> -- >> 2.27.0 >>
On 6/20/20 12:31 AM, Sebastian Reichel wrote: > Hi, > > On Fri, Jun 19, 2020 at 10:57:19PM +0300, Iskren Chernev wrote: >> On 6/19/20 6:59 PM, Sebastian Reichel wrote: >>> On Thu, Jun 18, 2020 at 01:13:39PM +0300, Iskren Chernev wrote: >>>> Maxim max17040 is a fuel gauge from a larger family utilising the Model >>>> Gauge technology. Document all different compatible strings that the >>>> max17040 driver recognizes. >>>> >>>> Some devices in the wild report double the capacity. The >>>> maxim,double-soc (from State-Of-Charge) property fixes that. >>>> >>>> Complete device reset might lead to very inaccurate readings. Specify >>>> maxim,skip-reset to avoid that. >>>> >>>> To compensate for the battery chemistry and operating conditions the >>>> chips support a compensation value. Specify one or two byte compensation >>>> via the maxim,rcomp byte array. >>>> >>>> Signed-off-by: Iskren Chernev <iskren.chernev@gmail.com> >>>> --- >>>> v1: https://lkml.org/lkml/2020/6/8/682 >>>> >>>> Changes in v2: >>>> - add maxim,skip-reset >>>> - remove 2 byte rcomp from example, the specified compat string supports 1 byte >>>> rcomp >>>> >>>> .../power/supply/max17040_battery.txt | 24 ++++++++++++++++++- >>>> 1 file changed, 23 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/Documentation/devicetree/bindings/power/supply/max17040_battery.txt b/Documentation/devicetree/bindings/power/supply/max17040_battery.txt >>>> index 4e0186b8380fa..3ee91c295027f 100644 >>>> --- a/Documentation/devicetree/bindings/power/supply/max17040_battery.txt >>>> +++ b/Documentation/devicetree/bindings/power/supply/max17040_battery.txt >>>> @@ -2,7 +2,9 @@ max17040_battery >>>> ~~~~~~~~~~~~~~~~ >>>> >>>> Required properties : >>>> - - compatible : "maxim,max17040" or "maxim,max77836-battery" >>>> + - compatible : "maxim,max17040", "maxim,max17041", "maxim,max17043", >>>> + "maxim,max17044", "maxim,max17048", "maxim,max17049", >>>> + "maxim,max17058", "maxim,max17059" or "maxim,max77836-battery" >>>> - reg: i2c slave address >>>> >>>> Optional properties : >>>> @@ -11,6 +13,18 @@ Optional properties : >>>> generated. Can be configured from 1 up to 32 >>>> (%). If skipped the power up default value of >>>> 4 (%) will be used. >>>> +- maxim,double-soc : Certain devices return double the capacity. >>>> + Specify this boolean property to divide the >>>> + reported value in 2 and thus normalize it. >>>> + SOC == State of Charge == Capacity. >>> >>> Can this be derived from the compatible? >> >> So far, I'm aware of 2 devices using max17048 that need this setting. That >> would be 100% of the 17048 devices I know of [1]. At the same time, according to >> the max17048 documentation this is not the case. >> >> [1] These are the Samsung Galaxy S5 (klte) and the LG Nexus 5 (hammerhead). > > Wouldn't be the first time, that documentation is wrong. Have you > checked how its handled in the downstream Android kernel? Yes, I have checked both klte[1] and hammerhead[2] downstream. About the state of charge -- the klte has the spec formula and the double formula, and the hammerhead has provisions in it's DTS to configure max/min raw values, effectively enabling arbitrary scaling of the value. About the reset on start, klte does a quick-start (0x4000 to MODE reg). Hammerhead resets the RI status bit (which in itself does not reset anything). I now also looked at downstream implementation of the other fuelgauges. Here are examples of 17040[3], 17043[4], 17058[5]. It looks like most of the family (except 17040) is prone to this doubling. [4] and [5] have both formulas toggleable with a macro. So I think leaving double-soc as an option in DTS is the right choice. About the reset -- looks like only 17040 is reset on start. The newer ones are either quick-started or nothing. So hard-resetting only the 17040 might be a good choice. [1] https://github.com/LineageOS/android_kernel_samsung_msm8974/blob/5dddbe776631650086c4491ec4037fbe73fa5795/drivers/battery/max17048_fuelgauge.c#L135 [2] https://github.com/LineageOS/android_kernel_lge_hammerhead/blob/62154f7111663d42655400f83c3c9d10a965571c/drivers/power/max17048_battery.c#L834 [3] https://github.com/LineageOS/android_kernel_lenovo_msm8916/blob/9a4a2ca2832653c90af7fae7503fc64687968700/drivers/power/max17040_battery.c [4] https://github.com/LineageOS/android_kernel_lge_v500/blob/b4fe00e1f8f09c173a6c28a42ca69ff9529cc13b/drivers/power/max17043_fuelgauge.c#L343 [5] https://github.com/LineageOS/android_kernel_asus_moorefield/blob/c3eae894ce8092c2a9a51f9a4924c8df714d6b3c/drivers/power/ASUS_BATTERY/max17058_battery.c#L551 >>>> +- maxim,skip-reset : Do not reset device on driver initialization. >>>> + Some devices report extremely inaccurately after >>>> + a hard reset. >>> >>> Same question. >>> >>> -- Sebastian >>> >> >> This is even weirder. There is no mention in the documentation about whether >> the device should be reset on boot (because the fuelgauge is on even during >> device off times). Testing on 17040 and 17043 reveals no issues with resetting >> on boot, but on the 17048 I have; the readings are up to 2x wrong if you do >> reset on boot. Not doing a reset *might* leave the device in a state that is >> not 100% known by the driver, but I don't know of any real world problems >> stemming from that. >> >> So in a sense, I can apply both of these quirks for 17048, and it will work for >> all devices I have tested on, but it won't follow the spec. That is why >> I decided to mark them as special behavior needing configuration per use case. >> On the other hand, I can incorporate them in 17048, and if someone complains >> the bindings can be introduced later (because they are stable API and >> introducing them now is a bit over engineered). > > Yes, just apply the quirks for 17048. As I mentioned in the previous paragraph, double-soc could benefit most of the family (excluding 17040), so I suggest we keep it, whereas doing a reset could remain for the 17040 only (removing the need for a flag in bindings). > -- Sebastian > >>>> +- maxim,rcomp : A value to compensate readings for various >>>> + battery chemistries and operating temperatures. >>>> + max17040,41 have 2 byte rcomp, default to >>>> + 0x97 0x00. All other devices have one byte >>>> + rcomp, default to 0x97. >>>> - interrupts : Interrupt line see Documentation/devicetree/ >>>> bindings/interrupt-controller/interrupts.txt >>>> - wakeup-source : This device has wakeup capabilities. Use this >>>> @@ -31,3 +45,11 @@ Example: >>>> interrupts = <2 IRQ_TYPE_EDGE_FALLING>; >>>> wakeup-source; >>>> }; >>>> + >>>> + battery-fuel-gauge@36 { >>>> + compatible = "maxim,max17048"; >>>> + reg = <0x36>; >>>> + maxim,rcomp = /bits/ 8 <0x97>; >>>> + maxim,alert-low-soc-level = <10>; >>>> + maxim,double-soc; >>>> + }; >>>> >>>> base-commit: 1713116fa907cc7290020f0d8632ec646d2936f8 >>>> -- >>>> 2.27.0 >>>> >>
diff --git a/Documentation/devicetree/bindings/power/supply/max17040_battery.txt b/Documentation/devicetree/bindings/power/supply/max17040_battery.txt index 4e0186b8380fa..3ee91c295027f 100644 --- a/Documentation/devicetree/bindings/power/supply/max17040_battery.txt +++ b/Documentation/devicetree/bindings/power/supply/max17040_battery.txt @@ -2,7 +2,9 @@ max17040_battery ~~~~~~~~~~~~~~~~ Required properties : - - compatible : "maxim,max17040" or "maxim,max77836-battery" + - compatible : "maxim,max17040", "maxim,max17041", "maxim,max17043", + "maxim,max17044", "maxim,max17048", "maxim,max17049", + "maxim,max17058", "maxim,max17059" or "maxim,max77836-battery" - reg: i2c slave address Optional properties : @@ -11,6 +13,18 @@ Optional properties : generated. Can be configured from 1 up to 32 (%). If skipped the power up default value of 4 (%) will be used. +- maxim,double-soc : Certain devices return double the capacity. + Specify this boolean property to divide the + reported value in 2 and thus normalize it. + SOC == State of Charge == Capacity. +- maxim,skip-reset : Do not reset device on driver initialization. + Some devices report extremely inaccurately after + a hard reset. +- maxim,rcomp : A value to compensate readings for various + battery chemistries and operating temperatures. + max17040,41 have 2 byte rcomp, default to + 0x97 0x00. All other devices have one byte + rcomp, default to 0x97. - interrupts : Interrupt line see Documentation/devicetree/ bindings/interrupt-controller/interrupts.txt - wakeup-source : This device has wakeup capabilities. Use this @@ -31,3 +45,11 @@ Example: interrupts = <2 IRQ_TYPE_EDGE_FALLING>; wakeup-source; }; + + battery-fuel-gauge@36 { + compatible = "maxim,max17048"; + reg = <0x36>; + maxim,rcomp = /bits/ 8 <0x97>; + maxim,alert-low-soc-level = <10>; + maxim,double-soc; + };
Maxim max17040 is a fuel gauge from a larger family utilising the Model Gauge technology. Document all different compatible strings that the max17040 driver recognizes. Some devices in the wild report double the capacity. The maxim,double-soc (from State-Of-Charge) property fixes that. Complete device reset might lead to very inaccurate readings. Specify maxim,skip-reset to avoid that. To compensate for the battery chemistry and operating conditions the chips support a compensation value. Specify one or two byte compensation via the maxim,rcomp byte array. Signed-off-by: Iskren Chernev <iskren.chernev@gmail.com> --- v1: https://lkml.org/lkml/2020/6/8/682 Changes in v2: - add maxim,skip-reset - remove 2 byte rcomp from example, the specified compat string supports 1 byte rcomp .../power/supply/max17040_battery.txt | 24 ++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-) base-commit: 1713116fa907cc7290020f0d8632ec646d2936f8