Re: [PATCH 14/15] power: supply: Add driver for Cherry Trail Whiskey Cove PMIC Fuel Gauge
Hi, On 17-03-17 18:58, Andy Shevchenko wrote: On Fri, 2017-03-17 at 10:55 +0100, Hans de Goede wrote: Add a driver for the Cherry Trail Whiskey Cove PMIC Fuel Gauge, note the Cherry Trail Whiskey Cove PMIC Fuel Gauge block is purely a fuel gauge and not a full battery controller. As such it offers a platform_data callback for extra power_supply properties for the actual external- charger ic driver and does not register a power_supply itself. ic -> IC Can we move to something like built-in device properties for additional properties instead of extending platform data? +config CHT_WC_FUEL_GAUGE I would use similar pattern: FUEL_GAUGE_INTEL_CHTWC (or FUEL_GAUGE_CHTWC, but this might be less obvious about vendor) Good point, although all the fuel-gauge's seem to use BATTERY as prefix, so I've gone with that. --- /dev/null +++ b/drivers/power/supply/cht_wc_fuel_gauge.c @@ -0,0 +1,209 @@ +/* + * Intel CHT Whiskey Cove Fuel Gauge driver CHT -> Cherry Trail Fixed. + * + * Cherrytrail Whiskey Cove devices have 2 functional blocks which interact + * with the battery. Cherry Trail? Since after discussion about how to deal with the charger / fuel_gauge comment v2 is going to be a stand-alone power_supply driver this comment has been dropped. +#define REG_CHARGE_NOW 0x05 +#define REG_VOLTAGE_NOW0x09 +#define REG_CURRENT_NOW0x0a +#define REG_CURRENT_AVG0x0b +#define REG_CHARGE_FULL0x10 +#define REG_CHARGE_DESIGN 0x18 +#define REG_VOLTAGE_AVG0x19 +#define REG_VOLTAGE_OCV0x1b /* Only updated during charging */ I think comment makes more sense where actual update is happening in the code. + +static int cht_wc_fg_read(struct cht_wc_fg_data *fg, u8 reg, + union power_supply_propval *val, int scale, + int sign_extend) +{ + int ret; + + ret = i2c_smbus_read_word_data(fg->client, reg); + if (ret < 0) + return ret; + + if (sign_extend) + ret = sign_extend32(ret, 15); Magic? Nope just simply dealing with i2c_smbus_read_word_data always returning 16 bit unsigned data (or a negative error code) and some of the registers being 16 bit signed. + + val->intval = ret * scale; + + return 0; +} + +int cht_wc_fg_get_property(enum power_supply_property prop, + union power_supply_propval *val) +{ + int ret = 0; Sounds like redundant assignment... No longer redundant in v2. + + mutex_lock(&cht_wc_fg_mutex); + + if (!cht_wc_fg) { + ret = -ENXIO; + goto out_unlock; + } ...otherwise maybe ret = cht_wc_fg ? 0 : -ENXIO; if (ret) goto ...; ? With the stand-alone power_supply driver version this ugliness is gone. + default: + ret = -ENODATA; + } +out_unlock: + mutex_unlock(&cht_wc_fg_mutex); + return ret; +} +EXPORT_SYMBOL_GPL(cht_wc_fg_get_property); + +static int cht_wc_fg_probe(struct i2c_client *client, + const struct i2c_device_id *i2c_id) +{ + struct device *dev = &client->dev; + struct cht_wc_fg_data *fg; + acpi_status status; + unsigned long long ptyp; + status = acpi_evaluate_integer(ACPI_HANDLE(dev), "PTYP", NULL, &ptyp); + if (ACPI_FAILURE(status)) { + dev_err(dev, "Failed to get PTYPE\n"); + return -ENODEV; + } + + /* +* The same ACPI HID is used with different PMICs check PTYP to +* ensure that we are dealing with a Whiskey Cove PMIC. +*/ + if (ptyp != CHT_WC_FG_PTYPE) + return -ENODEV; Logically I would split this part to be a main driver for device which would use actual driver based on this, though I think it too much churn for no benefit right now. Ack. + mutex_lock(&cht_wc_fg_mutex); + cht_wc_fg = fg; + mutex_unlock(&cht_wc_fg_mutex); It's pity we have no common storage of single possible present device drivers in the kernel. I would use some kind of framework rather then keeping all those global variables with locking. Perhaps radix / RB tree. With the stand-alone power_supply driver version this ugliness is gone. Regards, Hans
Re: [PATCH 14/15] power: supply: Add driver for Cherry Trail Whiskey Cove PMIC Fuel Gauge
Hi, On Fri, Mar 17, 2017 at 10:55:26AM +0100, Hans de Goede wrote: > Add a driver for the Cherry Trail Whiskey Cove PMIC Fuel Gauge, note > the Cherry Trail Whiskey Cove PMIC Fuel Gauge block is purely a fuel gauge > and not a full battery controller. As such it offers a platform_data > callback for extra power_supply properties for the actual external-charger > ic driver and does not register a power_supply itself. > > Signed-off-by: Hans de Goede I think this should become a normal battery-type power-supply driver. bq24190_charger driver should only expose a charger-type power-supply device on your system (and probably most others, its very rare, that systems have a programmable charger and no fuel-gauge). -- Sebastian > --- > drivers/power/supply/Kconfig | 9 ++ > drivers/power/supply/Makefile| 1 + > drivers/power/supply/cht_wc_fuel_gauge.c | 209 > +++ > include/linux/power/cht_wc_fuel_gauge.h | 21 > 4 files changed, 240 insertions(+) > create mode 100644 drivers/power/supply/cht_wc_fuel_gauge.c > create mode 100644 include/linux/power/cht_wc_fuel_gauge.h > > diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig > index fd93110..34ebfca 100644 > --- a/drivers/power/supply/Kconfig > +++ b/drivers/power/supply/Kconfig > @@ -538,4 +538,13 @@ config AXP20X_POWER > This driver provides support for the power supply features of > AXP20x PMIC. > > +config CHT_WC_FUEL_GAUGE > + tristate "Intel Cherry Trail Whiskey Cove PMIC Fuel Gauge" > + depends on INTEL_SOC_PMIC_CHTWC > + help > + This adds support for the battery fuel gauge found in the Intel > + Cherry Trail Whiskey Cove PMIC. This driver allows monitoring > + of the charge level of the battery on Intel Cherry Trail systems > + with a Whiskey Cove PMIC. > + > endif # POWER_SUPPLY > diff --git a/drivers/power/supply/Makefile b/drivers/power/supply/Makefile > index 3789a2c..702e28a 100644 > --- a/drivers/power/supply/Makefile > +++ b/drivers/power/supply/Makefile > @@ -74,3 +74,4 @@ obj-$(CONFIG_CHARGER_TPS65090) += tps65090-charger.o > obj-$(CONFIG_CHARGER_TPS65217) += tps65217_charger.o > obj-$(CONFIG_AXP288_FUEL_GAUGE) += axp288_fuel_gauge.o > obj-$(CONFIG_AXP288_CHARGER) += axp288_charger.o > +obj-$(CONFIG_CHT_WC_FUEL_GAUGE) += cht_wc_fuel_gauge.o > diff --git a/drivers/power/supply/cht_wc_fuel_gauge.c > b/drivers/power/supply/cht_wc_fuel_gauge.c > new file mode 100644 > index 000..56f6e5a > --- /dev/null > +++ b/drivers/power/supply/cht_wc_fuel_gauge.c > @@ -0,0 +1,209 @@ > +/* > + * Intel CHT Whiskey Cove Fuel Gauge driver > + * Copyright (C) 2017 Hans de Goede > + * > + * This program is free software; you can redistribute it and/or > + * modify it under the terms of the GNU General Public License version > + * 2 as published by the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + * Cherrytrail Whiskey Cove devices have 2 functional blocks which interact > + * with the battery. > + * > + * 1) The fuel-gauge which is build into the Whiskey Cove PMIC, but has its > + * own i2c bus and i2c client addresses separately from the rest of the PMIC. > + * That block is what this driver is for. > + * > + * 2) An external charger IC, which is connected to the SMBUS controller > + * which is part of the rest of the Whiskey Cove PMIC, mfd/intel_cht_wc.c > + * registers a platform device for the SMBUS controller and > + * i2c/busses/i2c-cht-wc.c contains the i2c-adapter driver for this. > + * > + * However we want to present this as a single power_supply device to > + * userspace. So this driver offers a callback to get the fuel-gauge > + * power_supply properties, which gets passed to the external charger > + * driver via i2c_board_info when i2c-cht-wc.c calls i2c_new_device(). > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#define REG_CHARGE_NOW 0x05 > +#define REG_VOLTAGE_NOW 0x09 > +#define REG_CURRENT_NOW 0x0a > +#define REG_CURRENT_AVG 0x0b > +#define REG_CHARGE_FULL 0x10 > +#define REG_CHARGE_DESIGN0x18 > +#define REG_VOLTAGE_AVG 0x19 > +#define REG_VOLTAGE_OCV 0x1b /* Only updated during charging */ > + > +#define CHT_WC_FG_PTYPE 4 > + > +struct cht_wc_fg_data { > + struct device *dev; > + struct i2c_client *client; > +}; > + > +static DEFINE_MUTEX(cht_wc_fg_mutex); > +static struct cht_wc_fg_data *cht_wc_fg; > + > +static int cht_wc_fg_read(struct cht_wc_fg_data *fg, u8 reg, > + union power_supply_propv
Re: [PATCH 14/15] power: supply: Add driver for Cherry Trail Whiskey Cove PMIC Fuel Gauge
On Fri, 2017-03-17 at 10:55 +0100, Hans de Goede wrote: > Add a driver for the Cherry Trail Whiskey Cove PMIC Fuel Gauge, note > the Cherry Trail Whiskey Cove PMIC Fuel Gauge block is purely a fuel > gauge > and not a full battery controller. As such it offers a platform_data > callback for extra power_supply properties for the actual external- > charger > ic driver and does not register a power_supply itself. ic -> IC Can we move to something like built-in device properties for additional properties instead of extending platform data? > +config CHT_WC_FUEL_GAUGE I would use similar pattern: FUEL_GAUGE_INTEL_CHTWC (or FUEL_GAUGE_CHTWC, but this might be less obvious about vendor) > --- /dev/null > +++ b/drivers/power/supply/cht_wc_fuel_gauge.c > @@ -0,0 +1,209 @@ > +/* > + * Intel CHT Whiskey Cove Fuel Gauge driver CHT -> Cherry Trail > + * > + * Cherrytrail Whiskey Cove devices have 2 functional blocks which > interact > + * with the battery. Cherry Trail? > +#define REG_CHARGE_NOW 0x05 > +#define REG_VOLTAGE_NOW 0x09 > +#define REG_CURRENT_NOW 0x0a > +#define REG_CURRENT_AVG 0x0b > +#define REG_CHARGE_FULL 0x10 > +#define REG_CHARGE_DESIGN0x18 > +#define REG_VOLTAGE_AVG 0x19 > +#define REG_VOLTAGE_OCV 0x1b /* Only updated during > charging */ I think comment makes more sense where actual update is happening in the code. > + > +static int cht_wc_fg_read(struct cht_wc_fg_data *fg, u8 reg, > + union power_supply_propval *val, int scale, > + int sign_extend) > +{ > + int ret; > + > + ret = i2c_smbus_read_word_data(fg->client, reg); > + if (ret < 0) > + return ret; > + > + if (sign_extend) > + ret = sign_extend32(ret, 15); Magic? > + > + val->intval = ret * scale; > + > + return 0; > +} > + > +int cht_wc_fg_get_property(enum power_supply_property prop, > + union power_supply_propval *val) > +{ > + int ret = 0; Sounds like redundant assignment... > + > + mutex_lock(&cht_wc_fg_mutex); > + > > + if (!cht_wc_fg) { > + ret = -ENXIO; > + goto out_unlock; > + } ...otherwise maybe ret = cht_wc_fg ? 0 : -ENXIO; if (ret) goto ...; ? > + default: > + ret = -ENODATA; > + } > +out_unlock: > + mutex_unlock(&cht_wc_fg_mutex); > + return ret; > +} > +EXPORT_SYMBOL_GPL(cht_wc_fg_get_property); > + > +static int cht_wc_fg_probe(struct i2c_client *client, > + const struct i2c_device_id *i2c_id) > +{ > + struct device *dev = &client->dev; > + struct cht_wc_fg_data *fg; > + acpi_status status; > + unsigned long long ptyp; > + status = acpi_evaluate_integer(ACPI_HANDLE(dev), "PTYP", > NULL, &ptyp); > + if (ACPI_FAILURE(status)) { > + dev_err(dev, "Failed to get PTYPE\n"); > + return -ENODEV; > + } > + > + /* > + * The same ACPI HID is used with different PMICs check PTYP > to > + * ensure that we are dealing with a Whiskey Cove PMIC. > + */ > + if (ptyp != CHT_WC_FG_PTYPE) > + return -ENODEV; Logically I would split this part to be a main driver for device which would use actual driver based on this, though I think it too much churn for no benefit right now. > + mutex_lock(&cht_wc_fg_mutex); > + cht_wc_fg = fg; > + mutex_unlock(&cht_wc_fg_mutex); It's pity we have no common storage of single possible present device drivers in the kernel. I would use some kind of framework rather then keeping all those global variables with locking. Perhaps radix / RB tree. > + > + return 0; > +} -- Andy Shevchenko Intel Finland Oy
[PATCH 14/15] power: supply: Add driver for Cherry Trail Whiskey Cove PMIC Fuel Gauge
Add a driver for the Cherry Trail Whiskey Cove PMIC Fuel Gauge, note the Cherry Trail Whiskey Cove PMIC Fuel Gauge block is purely a fuel gauge and not a full battery controller. As such it offers a platform_data callback for extra power_supply properties for the actual external-charger ic driver and does not register a power_supply itself. Signed-off-by: Hans de Goede --- drivers/power/supply/Kconfig | 9 ++ drivers/power/supply/Makefile| 1 + drivers/power/supply/cht_wc_fuel_gauge.c | 209 +++ include/linux/power/cht_wc_fuel_gauge.h | 21 4 files changed, 240 insertions(+) create mode 100644 drivers/power/supply/cht_wc_fuel_gauge.c create mode 100644 include/linux/power/cht_wc_fuel_gauge.h diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig index fd93110..34ebfca 100644 --- a/drivers/power/supply/Kconfig +++ b/drivers/power/supply/Kconfig @@ -538,4 +538,13 @@ config AXP20X_POWER This driver provides support for the power supply features of AXP20x PMIC. +config CHT_WC_FUEL_GAUGE + tristate "Intel Cherry Trail Whiskey Cove PMIC Fuel Gauge" + depends on INTEL_SOC_PMIC_CHTWC + help + This adds support for the battery fuel gauge found in the Intel + Cherry Trail Whiskey Cove PMIC. This driver allows monitoring + of the charge level of the battery on Intel Cherry Trail systems + with a Whiskey Cove PMIC. + endif # POWER_SUPPLY diff --git a/drivers/power/supply/Makefile b/drivers/power/supply/Makefile index 3789a2c..702e28a 100644 --- a/drivers/power/supply/Makefile +++ b/drivers/power/supply/Makefile @@ -74,3 +74,4 @@ obj-$(CONFIG_CHARGER_TPS65090)+= tps65090-charger.o obj-$(CONFIG_CHARGER_TPS65217) += tps65217_charger.o obj-$(CONFIG_AXP288_FUEL_GAUGE) += axp288_fuel_gauge.o obj-$(CONFIG_AXP288_CHARGER) += axp288_charger.o +obj-$(CONFIG_CHT_WC_FUEL_GAUGE)+= cht_wc_fuel_gauge.o diff --git a/drivers/power/supply/cht_wc_fuel_gauge.c b/drivers/power/supply/cht_wc_fuel_gauge.c new file mode 100644 index 000..56f6e5a --- /dev/null +++ b/drivers/power/supply/cht_wc_fuel_gauge.c @@ -0,0 +1,209 @@ +/* + * Intel CHT Whiskey Cove Fuel Gauge driver + * Copyright (C) 2017 Hans de Goede + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License version + * 2 as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * Cherrytrail Whiskey Cove devices have 2 functional blocks which interact + * with the battery. + * + * 1) The fuel-gauge which is build into the Whiskey Cove PMIC, but has its + * own i2c bus and i2c client addresses separately from the rest of the PMIC. + * That block is what this driver is for. + * + * 2) An external charger IC, which is connected to the SMBUS controller + * which is part of the rest of the Whiskey Cove PMIC, mfd/intel_cht_wc.c + * registers a platform device for the SMBUS controller and + * i2c/busses/i2c-cht-wc.c contains the i2c-adapter driver for this. + * + * However we want to present this as a single power_supply device to + * userspace. So this driver offers a callback to get the fuel-gauge + * power_supply properties, which gets passed to the external charger + * driver via i2c_board_info when i2c-cht-wc.c calls i2c_new_device(). + */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#define REG_CHARGE_NOW 0x05 +#define REG_VOLTAGE_NOW0x09 +#define REG_CURRENT_NOW0x0a +#define REG_CURRENT_AVG0x0b +#define REG_CHARGE_FULL0x10 +#define REG_CHARGE_DESIGN 0x18 +#define REG_VOLTAGE_AVG0x19 +#define REG_VOLTAGE_OCV0x1b /* Only updated during charging */ + +#define CHT_WC_FG_PTYPE4 + +struct cht_wc_fg_data { + struct device *dev; + struct i2c_client *client; +}; + +static DEFINE_MUTEX(cht_wc_fg_mutex); +static struct cht_wc_fg_data *cht_wc_fg; + +static int cht_wc_fg_read(struct cht_wc_fg_data *fg, u8 reg, + union power_supply_propval *val, int scale, + int sign_extend) +{ + int ret; + + ret = i2c_smbus_read_word_data(fg->client, reg); + if (ret < 0) + return ret; + + if (sign_extend) + ret = sign_extend32(ret, 15); + + val->intval = ret * scale; + + return 0; +} + +int cht_wc_fg_get_property(enum power_supply_property prop, + union power_supply_propval *val) +{ + int ret = 0; + + mutex_lock(&cht_wc_fg_mutex); + + if (!cht_wc_fg) { +