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

[Sebastian Reichel]

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.

> 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 :)

> +	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!

> +	if (temp < 0)
> +		return temp;

so e.g. -EIO will become -0.5°C?

> +	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).

> +
> +		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?

> +	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.

> +	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

Attachment: signature.asc
Description: PGP signature