Re: [PATCH 2/2] power: supply: Introduce the BQ27561 Fuel gauge driver

From: Dan Murphy
Date: Thu Apr 30 2020 - 08:52:19 EST

Sebastian

On 4/29/20 5:05 PM, Sebastian Reichel wrote:
Hi,

On Fri, Apr 17, 2020 at 12:22:27PM -0500, Dan Murphy wrote:
Introduce the BQ27561 Fuel gauge driver from Texas Instruments. The
driver also supports the BQ27750 as it has the same register map and
bit mask.
Why a separate driver instead of adding support to bq27xxx? The register
map seems to be similar enough.

I will need to see if I can integrate this into the bq27xxx.  I have to look at these 2 devices as well as another device that we need to integrate

But preliminary analysis suggests that we can integrate these 2 devices plus the third one into the driver.

Signed-off-by: Dan Murphy <dmurphy@xxxxxx>
---
drivers/power/supply/Kconfig | 9 +
drivers/power/supply/Makefile | 1 +
drivers/power/supply/bq27561_fg.c | 304 ++++++++++++++++++++++++++++++
3 files changed, 314 insertions(+)
create mode 100644 drivers/power/supply/bq27561_fg.c

diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
index f3424fdce341..febf6bf8c4ea 100644
--- a/drivers/power/supply/Kconfig
+++ b/drivers/power/supply/Kconfig
@@ -597,6 +597,15 @@ config CHARGER_BQ25890
help
Say Y to enable support for the TI BQ25890 battery charger.
+config FUEL_GAUGE_BQ27561
+ tristate "Texas Instruments bq27561 fuel gauge driver"
+ depends on I2C
+ select REGMAP_I2C
good :)
Ack

+ depends on GPIOLIB || COMPILE_TEST
+ help
+ Say Y here to enable support for fuel gauge with TI bq27561
+ fuel gauge chips.
+
config CHARGER_SMB347
tristate "Summit Microelectronics SMB347 Battery Charger"
depends on I2C
diff --git a/drivers/power/supply/Makefile b/drivers/power/supply/Makefile
index 6c7da920ea83..4e402292b276 100644
--- a/drivers/power/supply/Makefile
+++ b/drivers/power/supply/Makefile
@@ -81,6 +81,7 @@ obj-$(CONFIG_CHARGER_BQ24190) += bq24190_charger.o
obj-$(CONFIG_CHARGER_BQ24257) += bq24257_charger.o
obj-$(CONFIG_CHARGER_BQ24735) += bq24735-charger.o
obj-$(CONFIG_CHARGER_BQ25890) += bq25890_charger.o
+obj-$(CONFIG_FUEL_GAUGE_BQ27561) += bq27561_fg.o
obj-$(CONFIG_CHARGER_SMB347) += smb347-charger.o
obj-$(CONFIG_CHARGER_TPS65090) += tps65090-charger.o
obj-$(CONFIG_CHARGER_TPS65217) += tps65217_charger.o
diff --git a/drivers/power/supply/bq27561_fg.c b/drivers/power/supply/bq27561_fg.c
new file mode 100644
index 000000000000..b9f7cdc00d2e
--- /dev/null
+++ b/drivers/power/supply/bq27561_fg.c
@@ -0,0 +1,304 @@
+// SPDX-License-Identifier: GPL-2.0
+// BQ27561 Fuel Gauge Driver
+// Copyright (C) 2020 Texas Instruments Incorporated - http://www.ti.com/
+// Author: Dan Murphy <dmurphy@xxxxxx>
+
+#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/power_supply.h>
+#include <linux/types.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/moduleparam.h>
+#include <linux/slab.h>
+#include <linux/of.h>
+
+#define BQ27561_REG_CNTL 0x00
+#define BQ27561_REG_AR 0x02
+#define BQ27561_REG_TEMP 0x06
+#define BQ27561_REG_VOLT 0x08
+#define BQ27561_REG_REMAIN_CAP 0x10
+#define BQ27561_REG_BATTSTAT 0x0a
+#define BQ27561_REG_CURRENT 0x0c
+#define BQ27561_REG_FCC 0x12
+#define BQ27561_REG_AVG_CURRENT 0x14
+#define BQ27561_REG_TTE 0x16
+#define BQ27561_REG_TTF 0x18
+#define BQ27561_REG_CYCLECNT 0x2a
+#define BQ27561_REG_RSOC 0x2c
+
+#define BQ27561_VAL_MULTIPLIER 1000
+#define BQ27561_TEMP_K_TO_C 2732
+#define BQ27561_TIME_MIN_TO_S 60
+
+#define BQ27561_BATT_DEPLETED BIT(4)
+#define BQ27561_BATT_CHARGED BIT(5)
+#define BQ27561_BATT_DISCHARGING BIT(6)
+
+enum {
+ BQ27561,
+ BQ27750,
+};
+
+struct bq27561_device {
+ struct device *dev;
+ struct i2c_client *client;
+ struct power_supply *battery;
+};
+
+static enum power_supply_property bq27561_properties[] = {
+ POWER_SUPPLY_PROP_STATUS,
+ POWER_SUPPLY_PROP_CAPACITY,
+ POWER_SUPPLY_PROP_TEMP,
+ POWER_SUPPLY_PROP_VOLTAGE_NOW,
+ POWER_SUPPLY_PROP_CURRENT_NOW,
+ POWER_SUPPLY_PROP_CHARGE_NOW,
+ POWER_SUPPLY_PROP_CHARGE_FULL,
+ POWER_SUPPLY_PROP_TIME_TO_EMPTY_NOW,
+};
+
+static int bq27561_get_temp(struct bq27561_device *bq)
+{
+ unsigned int temp;
+
+ temp = i2c_smbus_read_word_data(bq->client, BQ27561_REG_TEMP);
please use regmap instead of direct i2c read/write commands!

OK I did not look at regmap close enough to see if it supports smbus.  But apparently it does.

I just need to make sure.


+ if (temp < 0)
+ return temp;
so e.g. -EIO will become -0.5°C?
Ack.  It is hard to get a negative temp to be recorded by the thermister in testing.

+ return temp - BQ27561_TEMP_K_TO_C;
+}
+
+static int bq27561_get_voltage(struct bq27561_device *bq)
+{
+ unsigned int batt_volt;
+
+ batt_volt = i2c_smbus_read_word_data(bq->client, BQ27561_REG_VOLT);
+ if (batt_volt < 0)
+ return batt_volt;
+
+ return batt_volt * BQ27561_VAL_MULTIPLIER;
+}
+
+static int bq27561_get_current(struct bq27561_device *bq)
+{
+ unsigned int batt_curr;
+
+ batt_curr = i2c_smbus_read_word_data(bq->client, BQ27561_REG_CURRENT);
+ if (batt_curr < 0)
+ return batt_curr;
+
+ return batt_curr * BQ27561_VAL_MULTIPLIER;
+}
+
+static int bq27561_get_charge_capacity(struct bq27561_device *bq)
+{
+ unsigned int charge_cap;
+
+ charge_cap = i2c_smbus_read_word_data(bq->client, BQ27561_REG_FCC);
+ if (charge_cap < 0)
+ return charge_cap;
+
+ return charge_cap * BQ27561_VAL_MULTIPLIER;
+}
+
+static int bq27561_get_batt_status(struct bq27561_device *bq)
+{
+ unsigned int batt_stat;
+ unsigned int curr;
+ int ret;
+
+ batt_stat = i2c_smbus_read_word_data(bq->client, BQ27561_REG_BATTSTAT);
+ if (batt_stat < 0)
+ return batt_stat;
+
+ curr = i2c_smbus_read_word_data(bq->client, BQ27561_REG_CURRENT);
+ if (curr < 0)
+ return curr;
+
+ if (batt_stat & BQ27561_BATT_CHARGED)
+ ret = POWER_SUPPLY_STATUS_FULL;
+ else if (batt_stat & BQ27561_BATT_DISCHARGING || curr == 0)
+ ret = POWER_SUPPLY_STATUS_DISCHARGING;
+ else if (curr > 0)
+ ret = POWER_SUPPLY_STATUS_CHARGING;
+ else
+ ret = POWER_SUPPLY_STATUS_UNKNOWN;
+
+ return ret;
+}
+
+static int bq27561_get_empty_time(struct bq27561_device *bq)
+{
+ unsigned int empty_time;
+
+ empty_time = i2c_smbus_read_word_data(bq->client, BQ27561_REG_TTE);
+ if (empty_time < 0)
+ return empty_time;
+
+ return empty_time * BQ27561_TIME_MIN_TO_S;
+}
+
+static int bq27561_get_property(struct power_supply *psy,
+ enum power_supply_property psp,
+ union power_supply_propval *val)
+{
+ struct bq27561_device *bq = power_supply_get_drvdata(psy);
+ int ret = 0;
+
+ switch (psp) {
+ case POWER_SUPPLY_PROP_VOLTAGE_NOW:
+ ret = bq27561_get_voltage(bq);
+ if (ret < 0)
+ return ret;
+
+ val->intval = ret;
+ break;
+ case POWER_SUPPLY_PROP_CURRENT_NOW:
+ ret = bq27561_get_current(bq);
+ if (ret < 0)
+ return ret;
current can be negative (charging vs discharging).
Ack

+
+ val->intval = ret;
+ break;
+ case POWER_SUPPLY_PROP_STATUS:
+ ret = bq27561_get_batt_status(bq);
+ if (ret < 0)
+ return ret;
+
+ val->intval = ret;
+ break;
+ case POWER_SUPPLY_PROP_TEMP:
+ val->intval = bq27561_get_temp(bq);
+ break;
no error handling?
Ack

+ case POWER_SUPPLY_PROP_CAPACITY:
+ ret = i2c_smbus_read_word_data(bq->client, BQ27561_REG_RSOC);
+ if (ret < 0)
+ return ret;
+
+ val->intval = ret;
+ break;
+ case POWER_SUPPLY_PROP_CHARGE_FULL:
+ ret = bq27561_get_charge_capacity(bq);
+ if (ret < 0)
+ return ret;
+
+ val->intval = ret;
+ break;
+ case POWER_SUPPLY_PROP_TIME_TO_EMPTY_NOW:
+ ret = bq27561_get_empty_time(bq);
+ if (ret < 0)
+ return ret;
+
+ val->intval = ret;
+ break;
+ case POWER_SUPPLY_PROP_CHARGE_NOW:
+ ret = i2c_smbus_read_word_data(bq->client, BQ27561_REG_AR);
+ if (ret < 0)
+ return ret;
+
+ val->intval = ret;
+ break;
+ default:
+ return -EINVAL;
+ };
+
+ return 0;
+}
+
+static int bq27561_set_property(struct power_supply *psy,
+ enum power_supply_property psp,
+ const union power_supply_propval *val)
+{
+ struct bq27561_device *bq = power_supply_get_drvdata(psy);
+ int ret = 0;
+
+ switch (psp) {
+ case POWER_SUPPLY_PROP_CHARGE_NOW:
+ ret = i2c_smbus_write_word_data(bq->client, BQ27561_REG_AR,
+ val->intval);
+ break;
+ default:
+ return -EINVAL;
+ };
+
+ return ret;
+}
+
+static int bq27561_prop_is_writeable(struct power_supply *psy,
+ enum power_supply_property prop)
+{
+ switch (prop) {
+ case POWER_SUPPLY_PROP_CHARGE_NOW:
+ return 1;
+ default:
+ return 0;
+ };
+}
+
+static struct power_supply_desc bq27561_power_supply_desc = {
+ .type = POWER_SUPPLY_TYPE_BATTERY,
+ .properties = bq27561_properties,
+ .num_properties = ARRAY_SIZE(bq27561_properties),
+ .get_property = bq27561_get_property,
+ .set_property = bq27561_set_property,
+ .property_is_writeable = bq27561_prop_is_writeable,
+};
+
+static int bq27561_probe(struct i2c_client *client,
+ const struct i2c_device_id *id)
+{
+ struct bq27561_device *bq;
+ struct power_supply_config psy_cfg;
+
+ bq = devm_kzalloc(&client->dev, sizeof(*bq), GFP_KERNEL);
+ if (!bq)
+ return -ENOMEM;
+
+ bq->dev = &client->dev;
+ bq->client = client;
+ i2c_set_clientdata(client, bq);
+
+ psy_cfg.drv_data = bq;
You should setup psy_cfg.of_node, so that it can be referenced
in DT.

Ack
+ bq27561_power_supply_desc.name = id->name;
+ bq->battery = devm_power_supply_register(bq->dev,
+ &bq27561_power_supply_desc,
+ &psy_cfg);
+
+ if (IS_ERR(bq->battery))
+ return -EINVAL;
+
+ return 0;
+}
+
+static const struct of_device_id bq27561_match_table[] = {
+ {.compatible = "ti,bq27561",},
+ {.compatible = "ti,bq27750",},
+ {},
+};
+MODULE_DEVICE_TABLE(of, bq_fg_match_table);
+
+static const struct i2c_device_id bq27561_id[] = {
+ {"bq27561", BQ27561},
+ {"bq27750", BQ27750},
+ {},
+};
+MODULE_DEVICE_TABLE(i2c, bq27561_id);
+
+static struct i2c_driver bq27561_battery_driver = {
+ .driver = {
+ .name = "bq27561-fuel-gauge",
+ .of_match_table = bq27561_match_table,
+ .owner = THIS_MODULE,
i2c_register_driver() sets it owner automatically, so
drop this.

+ },
+ .probe = bq27561_probe,
+ .id_table = bq27561_id,
+};
+module_i2c_driver(bq27561_battery_driver);
+
+MODULE_AUTHOR("Texas Instruments");
+MODULE_DESCRIPTION("bq27561 battery monitor driver");
+MODULE_LICENSE("GPL v2");
-- Sebastian