| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <26d120ab-5506-f0f8-1d45-fd7c5c4a1bde@ti.com>
Date: Thu, 30 Apr 2020 07:43:37 -0500
From: Dan Murphy <dmurphy@...com>
To: Sebastian Reichel <sebastian.reichel@...labora.com>
CC: "Andrew F. Davis" <afd@...com>, <linux-pm@...r.kernel.org>,
<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2/2] power: supply: Introduce the BQ27561 Fuel gauge
driver
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@...com>
>> ---
>> 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@...com>
>> +
>> +#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
Powered by blists - more mailing lists