* [PATCH v1 1/1] power: bq24190_charger: Deprecate battery class and replicate its features in charger
@ 2017-04-16 23:06 Liam Breck
2017-04-18 8:05 ` Hans de Goede
2017-05-01 11:29 ` Sebastian Reichel
0 siblings, 2 replies; 7+ messages in thread
From: Liam Breck @ 2017-04-16 23:06 UTC (permalink / raw)
To: Sebastian Reichel, linux-pm; +Cc: Tony Lindgren, Hans de Goede, Liam Breck
From: Liam Breck <kernel@networkimprov.net>
The driver was registering two classes, bq24190-battery & -charger.
Because the power supply framework cannot surface features from multiple
drivers in a single class, a fuel gauge driver would create a third class,
which some power management utilities cannot see.
Deprecate the -battery class for future removal and replicate its features
in -charger. Set /sys/class...-charger/online = pg_stat && !batfet_disable.
If device_property "omit-battery-class" is set, don't register -battery.
Cc: Tony Lindgren <tony@atomide.com>
Cc: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Liam Breck <kernel@networkimprov.net>
---
drivers/power/supply/bq24190_charger.c | 146 +++++++++++++++++++++++++--------
1 file changed, 111 insertions(+), 35 deletions(-)
diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
index f581042..4197fdb 100644
--- a/drivers/power/supply/bq24190_charger.c
+++ b/drivers/power/supply/bq24190_charger.c
@@ -658,22 +658,25 @@ static int bq24190_charger_get_health(struct bq24190_dev_info *bdi,
v = bdi->f_reg;
mutex_unlock(&bdi->f_reg_lock);
- if (v & BQ24190_REG_F_BOOST_FAULT_MASK) {
- /*
- * This could be over-current or over-voltage but there's
- * no way to tell which. Return 'OVERVOLTAGE' since there
- * isn't an 'OVERCURRENT' value defined that we can return
- * even if it was over-current.
- */
- health = POWER_SUPPLY_HEALTH_OVERVOLTAGE;
- } else {
- v &= BQ24190_REG_F_CHRG_FAULT_MASK;
- v >>= BQ24190_REG_F_CHRG_FAULT_SHIFT;
-
- switch (v) {
- case 0x0: /* Normal */
- health = POWER_SUPPLY_HEALTH_GOOD;
+ if (v & BQ24190_REG_F_NTC_FAULT_MASK) {
+ switch (v >> BQ24190_REG_F_NTC_FAULT_SHIFT & 0x7) {
+ case 0x1: /* TS1 Cold */
+ case 0x3: /* TS2 Cold */
+ case 0x5: /* Both Cold */
+ health = POWER_SUPPLY_HEALTH_COLD;
break;
+ case 0x2: /* TS1 Hot */
+ case 0x4: /* TS2 Hot */
+ case 0x6: /* Both Hot */
+ health = POWER_SUPPLY_HEALTH_OVERHEAT;
+ break;
+ default:
+ health = POWER_SUPPLY_HEALTH_UNKNOWN;
+ }
+ } else if (v & BQ24190_REG_F_BAT_FAULT_MASK) {
+ health = POWER_SUPPLY_HEALTH_OVERVOLTAGE;
+ } else if (v & BQ24190_REG_F_CHRG_FAULT_MASK) {
+ switch (v >> BQ24190_REG_F_CHRG_FAULT_SHIFT & 0x3) {
case 0x1: /* Input Fault (VBUS OVP or VBAT<VBUS<3.8V) */
/*
* This could be over-voltage or under-voltage
@@ -690,9 +693,19 @@ static int bq24190_charger_get_health(struct bq24190_dev_info *bdi,
case 0x3: /* Charge Safety Timer Expiration */
health = POWER_SUPPLY_HEALTH_SAFETY_TIMER_EXPIRE;
break;
- default:
- health = POWER_SUPPLY_HEALTH_UNKNOWN;
+ default: /* prevent compiler warning */
+ health = -1;
}
+ } else if (v & BQ24190_REG_F_BOOST_FAULT_MASK) {
+ /*
+ * This could be over-current or over-voltage but there's
+ * no way to tell which. Return 'OVERVOLTAGE' since there
+ * isn't an 'OVERCURRENT' value defined that we can return
+ * even if it was over-current.
+ */
+ health = POWER_SUPPLY_HEALTH_OVERVOLTAGE;
+ } else {
+ health = POWER_SUPPLY_HEALTH_GOOD;
}
val->intval = health;
@@ -703,19 +716,59 @@ static int bq24190_charger_get_health(struct bq24190_dev_info *bdi,
static int bq24190_charger_get_online(struct bq24190_dev_info *bdi,
union power_supply_propval *val)
{
- u8 v;
+ u8 pg_stat, batfet_disable;
int ret;
ret = bq24190_read_mask(bdi, BQ24190_REG_SS,
BQ24190_REG_SS_PG_STAT_MASK,
- BQ24190_REG_SS_PG_STAT_SHIFT, &v);
+ BQ24190_REG_SS_PG_STAT_SHIFT, &pg_stat);
if (ret < 0)
return ret;
- val->intval = v;
+ ret = bq24190_read_mask(bdi, BQ24190_REG_MOC,
+ BQ24190_REG_MOC_BATFET_DISABLE_MASK,
+ BQ24190_REG_MOC_BATFET_DISABLE_SHIFT, &batfet_disable);
+ if (ret < 0)
+ return ret;
+
+ val->intval = pg_stat && !batfet_disable;
+
return 0;
}
+static int bq24190_battery_set_online(struct bq24190_dev_info *bdi,
+ const union power_supply_propval *val);
+static int bq24190_battery_get_status(struct bq24190_dev_info *bdi,
+ union power_supply_propval *val);
+static int bq24190_battery_get_temp_alert_max(struct bq24190_dev_info *bdi,
+ union power_supply_propval *val);
+static int bq24190_battery_set_temp_alert_max(struct bq24190_dev_info *bdi,
+ const union power_supply_propval *val);
+
+static int bq24190_charger_set_online(struct bq24190_dev_info *bdi,
+ const union power_supply_propval *val)
+{
+ return bq24190_battery_set_online(bdi, val);
+}
+
+static int bq24190_charger_get_status(struct bq24190_dev_info *bdi,
+ union power_supply_propval *val)
+{
+ return bq24190_battery_get_status(bdi, val);
+}
+
+static int bq24190_charger_get_temp_alert_max(struct bq24190_dev_info *bdi,
+ union power_supply_propval *val)
+{
+ return bq24190_battery_get_temp_alert_max(bdi, val);
+}
+
+static int bq24190_charger_set_temp_alert_max(struct bq24190_dev_info *bdi,
+ const union power_supply_propval *val)
+{
+ return bq24190_battery_set_temp_alert_max(bdi, val);
+}
+
static int bq24190_charger_get_current(struct bq24190_dev_info *bdi,
union power_supply_propval *val)
{
@@ -830,6 +883,12 @@ static int bq24190_charger_get_property(struct power_supply *psy,
case POWER_SUPPLY_PROP_ONLINE:
ret = bq24190_charger_get_online(bdi, val);
break;
+ case POWER_SUPPLY_PROP_STATUS:
+ ret = bq24190_charger_get_status(bdi, val);
+ break;
+ case POWER_SUPPLY_PROP_TEMP_ALERT_MAX:
+ ret = bq24190_charger_get_temp_alert_max(bdi, val);
+ break;
case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT:
ret = bq24190_charger_get_current(bdi, val);
break;
@@ -878,6 +937,12 @@ static int bq24190_charger_set_property(struct power_supply *psy,
return ret;
switch (psp) {
+ case POWER_SUPPLY_PROP_ONLINE:
+ ret = bq24190_charger_set_online(bdi, val);
+ break;
+ case POWER_SUPPLY_PROP_TEMP_ALERT_MAX:
+ ret = bq24190_charger_set_temp_alert_max(bdi, val);
+ break;
case POWER_SUPPLY_PROP_CHARGE_TYPE:
ret = bq24190_charger_set_charge_type(bdi, val);
break;
@@ -903,6 +968,8 @@ static int bq24190_charger_property_is_writeable(struct power_supply *psy,
int ret;
switch (psp) {
+ case POWER_SUPPLY_PROP_ONLINE:
+ case POWER_SUPPLY_PROP_TEMP_ALERT_MAX:
case POWER_SUPPLY_PROP_CHARGE_TYPE:
case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT:
case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE:
@@ -919,6 +986,8 @@ static enum power_supply_property bq24190_charger_properties[] = {
POWER_SUPPLY_PROP_CHARGE_TYPE,
POWER_SUPPLY_PROP_HEALTH,
POWER_SUPPLY_PROP_ONLINE,
+ POWER_SUPPLY_PROP_STATUS,
+ POWER_SUPPLY_PROP_TEMP_ALERT_MAX,
POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT,
POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX,
POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE,
@@ -1092,6 +1161,7 @@ static int bq24190_battery_get_property(struct power_supply *psy,
struct bq24190_dev_info *bdi = power_supply_get_drvdata(psy);
int ret;
+ dev_warn(bdi->dev, "warning: /sys/class/power_supply/bq24190-battery is deprecated\n");
dev_dbg(bdi->dev, "prop: %d\n", psp);
ret = pm_runtime_get_sync(bdi->dev);
@@ -1137,6 +1207,7 @@ static int bq24190_battery_set_property(struct power_supply *psy,
struct bq24190_dev_info *bdi = power_supply_get_drvdata(psy);
int ret;
+ dev_warn(bdi->dev, "warning: /sys/class/power_supply/bq24190-battery is deprecated\n");
dev_dbg(bdi->dev, "prop: %d\n", psp);
ret = pm_runtime_get_sync(bdi->dev);
@@ -1265,9 +1336,9 @@ static void bq24190_check_status(struct bq24190_dev_info *bdi)
bdi->ss_reg = ss_reg;
}
- if (alert_charger)
+ if (alert_charger || alert_battery)
power_supply_changed(bdi->charger);
- if (alert_battery)
+ if (alert_battery && bdi->battery)
power_supply_changed(bdi->battery);
dev_dbg(bdi->dev, "ss_reg: 0x%02x, f_reg: 0x%02x\n", ss_reg, f_reg);
@@ -1472,19 +1543,23 @@ static int bq24190_probe(struct i2c_client *client,
goto out_pmrt;
}
- battery_cfg.drv_data = bdi;
- bdi->battery = power_supply_register(dev, &bq24190_battery_desc,
- &battery_cfg);
- if (IS_ERR(bdi->battery)) {
- dev_err(dev, "Can't register battery\n");
- ret = PTR_ERR(bdi->battery);
- goto out_charger;
+ /* the battery class is deprecated and will be removed. */
+ /* in the interim, this property hides it. */
+ if (!device_property_read_bool(dev, "omit-battery-class")) {
+ battery_cfg.drv_data = bdi;
+ bdi->battery = power_supply_register(dev, &bq24190_battery_desc,
+ &battery_cfg);
+ if (IS_ERR(bdi->battery)) {
+ dev_err(dev, "Can't register battery\n");
+ ret = PTR_ERR(bdi->battery);
+ goto out_charger;
+ }
}
ret = bq24190_sysfs_create_group(bdi);
if (ret) {
dev_err(dev, "Can't create sysfs entries\n");
- goto out_battery;
+ goto out_charger;
}
bdi->initialized = true;
@@ -1522,10 +1597,9 @@ static int bq24190_probe(struct i2c_client *client,
out_sysfs:
bq24190_sysfs_remove_group(bdi);
-out_battery:
- power_supply_unregister(bdi->battery);
-
out_charger:
+ if (!IS_ERR_OR_NULL(bdi->battery))
+ power_supply_unregister(bdi->battery);
power_supply_unregister(bdi->charger);
out_pmrt:
@@ -1548,7 +1622,8 @@ static int bq24190_remove(struct i2c_client *client)
bq24190_register_reset(bdi);
bq24190_sysfs_remove_group(bdi);
- power_supply_unregister(bdi->battery);
+ if (bdi->battery)
+ power_supply_unregister(bdi->battery);
power_supply_unregister(bdi->charger);
if (error >= 0)
pm_runtime_put_sync(bdi->dev);
@@ -1635,7 +1710,8 @@ static __maybe_unused int bq24190_pm_resume(struct device *dev)
/* Things may have changed while suspended so alert upper layer */
power_supply_changed(bdi->charger);
- power_supply_changed(bdi->battery);
+ if (bdi->battery)
+ power_supply_changed(bdi->battery);
return 0;
}
--
2.9.3
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v1 1/1] power: bq24190_charger: Deprecate battery class and replicate its features in charger
2017-04-16 23:06 [PATCH v1 1/1] power: bq24190_charger: Deprecate battery class and replicate its features in charger Liam Breck
@ 2017-04-18 8:05 ` Hans de Goede
2017-04-18 9:42 ` Liam Breck
2017-05-01 11:29 ` Sebastian Reichel
1 sibling, 1 reply; 7+ messages in thread
From: Hans de Goede @ 2017-04-18 8:05 UTC (permalink / raw)
To: Liam Breck, Sebastian Reichel, linux-pm; +Cc: Tony Lindgren, Liam Breck
Hi Liam,
On 17-04-17 01:06, Liam Breck wrote:
> From: Liam Breck <kernel@networkimprov.net>
>
> The driver was registering two classes, bq24190-battery & -charger.
> Because the power supply framework cannot surface features from multiple
> drivers in a single class, a fuel gauge driver would create a third class,
> which some power management utilities cannot see.
>
> Deprecate the -battery class for future removal and replicate its features
> in -charger. Set /sys/class...-charger/online = pg_stat && !batfet_disable.
> If device_property "omit-battery-class" is set, don't register -battery.
>
> Cc: Tony Lindgren <tony@atomide.com>
> Cc: Hans de Goede <hdegoede@redhat.com>
> Signed-off-by: Liam Breck <kernel@networkimprov.net>
Thank you for writing this patch.
The patch looks good to me:
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Regards,
Hans
p.s.
Any chance you can find some time to review:
"[PATCH v6] power: supply: bq24190_charger: Add disable-reset device-property" ?
> ---
> drivers/power/supply/bq24190_charger.c | 146 +++++++++++++++++++++++++--------
> 1 file changed, 111 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
> index f581042..4197fdb 100644
> --- a/drivers/power/supply/bq24190_charger.c
> +++ b/drivers/power/supply/bq24190_charger.c
> @@ -658,22 +658,25 @@ static int bq24190_charger_get_health(struct bq24190_dev_info *bdi,
> v = bdi->f_reg;
> mutex_unlock(&bdi->f_reg_lock);
>
> - if (v & BQ24190_REG_F_BOOST_FAULT_MASK) {
> - /*
> - * This could be over-current or over-voltage but there's
> - * no way to tell which. Return 'OVERVOLTAGE' since there
> - * isn't an 'OVERCURRENT' value defined that we can return
> - * even if it was over-current.
> - */
> - health = POWER_SUPPLY_HEALTH_OVERVOLTAGE;
> - } else {
> - v &= BQ24190_REG_F_CHRG_FAULT_MASK;
> - v >>= BQ24190_REG_F_CHRG_FAULT_SHIFT;
> -
> - switch (v) {
> - case 0x0: /* Normal */
> - health = POWER_SUPPLY_HEALTH_GOOD;
> + if (v & BQ24190_REG_F_NTC_FAULT_MASK) {
> + switch (v >> BQ24190_REG_F_NTC_FAULT_SHIFT & 0x7) {
> + case 0x1: /* TS1 Cold */
> + case 0x3: /* TS2 Cold */
> + case 0x5: /* Both Cold */
> + health = POWER_SUPPLY_HEALTH_COLD;
> break;
> + case 0x2: /* TS1 Hot */
> + case 0x4: /* TS2 Hot */
> + case 0x6: /* Both Hot */
> + health = POWER_SUPPLY_HEALTH_OVERHEAT;
> + break;
> + default:
> + health = POWER_SUPPLY_HEALTH_UNKNOWN;
> + }
> + } else if (v & BQ24190_REG_F_BAT_FAULT_MASK) {
> + health = POWER_SUPPLY_HEALTH_OVERVOLTAGE;
> + } else if (v & BQ24190_REG_F_CHRG_FAULT_MASK) {
> + switch (v >> BQ24190_REG_F_CHRG_FAULT_SHIFT & 0x3) {
> case 0x1: /* Input Fault (VBUS OVP or VBAT<VBUS<3.8V) */
> /*
> * This could be over-voltage or under-voltage
> @@ -690,9 +693,19 @@ static int bq24190_charger_get_health(struct bq24190_dev_info *bdi,
> case 0x3: /* Charge Safety Timer Expiration */
> health = POWER_SUPPLY_HEALTH_SAFETY_TIMER_EXPIRE;
> break;
> - default:
> - health = POWER_SUPPLY_HEALTH_UNKNOWN;
> + default: /* prevent compiler warning */
> + health = -1;
> }
> + } else if (v & BQ24190_REG_F_BOOST_FAULT_MASK) {
> + /*
> + * This could be over-current or over-voltage but there's
> + * no way to tell which. Return 'OVERVOLTAGE' since there
> + * isn't an 'OVERCURRENT' value defined that we can return
> + * even if it was over-current.
> + */
> + health = POWER_SUPPLY_HEALTH_OVERVOLTAGE;
> + } else {
> + health = POWER_SUPPLY_HEALTH_GOOD;
> }
>
> val->intval = health;
> @@ -703,19 +716,59 @@ static int bq24190_charger_get_health(struct bq24190_dev_info *bdi,
> static int bq24190_charger_get_online(struct bq24190_dev_info *bdi,
> union power_supply_propval *val)
> {
> - u8 v;
> + u8 pg_stat, batfet_disable;
> int ret;
>
> ret = bq24190_read_mask(bdi, BQ24190_REG_SS,
> BQ24190_REG_SS_PG_STAT_MASK,
> - BQ24190_REG_SS_PG_STAT_SHIFT, &v);
> + BQ24190_REG_SS_PG_STAT_SHIFT, &pg_stat);
> if (ret < 0)
> return ret;
>
> - val->intval = v;
> + ret = bq24190_read_mask(bdi, BQ24190_REG_MOC,
> + BQ24190_REG_MOC_BATFET_DISABLE_MASK,
> + BQ24190_REG_MOC_BATFET_DISABLE_SHIFT, &batfet_disable);
> + if (ret < 0)
> + return ret;
> +
> + val->intval = pg_stat && !batfet_disable;
> +
> return 0;
> }
>
> +static int bq24190_battery_set_online(struct bq24190_dev_info *bdi,
> + const union power_supply_propval *val);
> +static int bq24190_battery_get_status(struct bq24190_dev_info *bdi,
> + union power_supply_propval *val);
> +static int bq24190_battery_get_temp_alert_max(struct bq24190_dev_info *bdi,
> + union power_supply_propval *val);
> +static int bq24190_battery_set_temp_alert_max(struct bq24190_dev_info *bdi,
> + const union power_supply_propval *val);
> +
> +static int bq24190_charger_set_online(struct bq24190_dev_info *bdi,
> + const union power_supply_propval *val)
> +{
> + return bq24190_battery_set_online(bdi, val);
> +}
> +
> +static int bq24190_charger_get_status(struct bq24190_dev_info *bdi,
> + union power_supply_propval *val)
> +{
> + return bq24190_battery_get_status(bdi, val);
> +}
> +
> +static int bq24190_charger_get_temp_alert_max(struct bq24190_dev_info *bdi,
> + union power_supply_propval *val)
> +{
> + return bq24190_battery_get_temp_alert_max(bdi, val);
> +}
> +
> +static int bq24190_charger_set_temp_alert_max(struct bq24190_dev_info *bdi,
> + const union power_supply_propval *val)
> +{
> + return bq24190_battery_set_temp_alert_max(bdi, val);
> +}
> +
> static int bq24190_charger_get_current(struct bq24190_dev_info *bdi,
> union power_supply_propval *val)
> {
> @@ -830,6 +883,12 @@ static int bq24190_charger_get_property(struct power_supply *psy,
> case POWER_SUPPLY_PROP_ONLINE:
> ret = bq24190_charger_get_online(bdi, val);
> break;
> + case POWER_SUPPLY_PROP_STATUS:
> + ret = bq24190_charger_get_status(bdi, val);
> + break;
> + case POWER_SUPPLY_PROP_TEMP_ALERT_MAX:
> + ret = bq24190_charger_get_temp_alert_max(bdi, val);
> + break;
> case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT:
> ret = bq24190_charger_get_current(bdi, val);
> break;
> @@ -878,6 +937,12 @@ static int bq24190_charger_set_property(struct power_supply *psy,
> return ret;
>
> switch (psp) {
> + case POWER_SUPPLY_PROP_ONLINE:
> + ret = bq24190_charger_set_online(bdi, val);
> + break;
> + case POWER_SUPPLY_PROP_TEMP_ALERT_MAX:
> + ret = bq24190_charger_set_temp_alert_max(bdi, val);
> + break;
> case POWER_SUPPLY_PROP_CHARGE_TYPE:
> ret = bq24190_charger_set_charge_type(bdi, val);
> break;
> @@ -903,6 +968,8 @@ static int bq24190_charger_property_is_writeable(struct power_supply *psy,
> int ret;
>
> switch (psp) {
> + case POWER_SUPPLY_PROP_ONLINE:
> + case POWER_SUPPLY_PROP_TEMP_ALERT_MAX:
> case POWER_SUPPLY_PROP_CHARGE_TYPE:
> case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT:
> case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE:
> @@ -919,6 +986,8 @@ static enum power_supply_property bq24190_charger_properties[] = {
> POWER_SUPPLY_PROP_CHARGE_TYPE,
> POWER_SUPPLY_PROP_HEALTH,
> POWER_SUPPLY_PROP_ONLINE,
> + POWER_SUPPLY_PROP_STATUS,
> + POWER_SUPPLY_PROP_TEMP_ALERT_MAX,
> POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT,
> POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX,
> POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE,
> @@ -1092,6 +1161,7 @@ static int bq24190_battery_get_property(struct power_supply *psy,
> struct bq24190_dev_info *bdi = power_supply_get_drvdata(psy);
> int ret;
>
> + dev_warn(bdi->dev, "warning: /sys/class/power_supply/bq24190-battery is deprecated\n");
> dev_dbg(bdi->dev, "prop: %d\n", psp);
>
> ret = pm_runtime_get_sync(bdi->dev);
> @@ -1137,6 +1207,7 @@ static int bq24190_battery_set_property(struct power_supply *psy,
> struct bq24190_dev_info *bdi = power_supply_get_drvdata(psy);
> int ret;
>
> + dev_warn(bdi->dev, "warning: /sys/class/power_supply/bq24190-battery is deprecated\n");
> dev_dbg(bdi->dev, "prop: %d\n", psp);
>
> ret = pm_runtime_get_sync(bdi->dev);
> @@ -1265,9 +1336,9 @@ static void bq24190_check_status(struct bq24190_dev_info *bdi)
> bdi->ss_reg = ss_reg;
> }
>
> - if (alert_charger)
> + if (alert_charger || alert_battery)
> power_supply_changed(bdi->charger);
> - if (alert_battery)
> + if (alert_battery && bdi->battery)
> power_supply_changed(bdi->battery);
>
> dev_dbg(bdi->dev, "ss_reg: 0x%02x, f_reg: 0x%02x\n", ss_reg, f_reg);
> @@ -1472,19 +1543,23 @@ static int bq24190_probe(struct i2c_client *client,
> goto out_pmrt;
> }
>
> - battery_cfg.drv_data = bdi;
> - bdi->battery = power_supply_register(dev, &bq24190_battery_desc,
> - &battery_cfg);
> - if (IS_ERR(bdi->battery)) {
> - dev_err(dev, "Can't register battery\n");
> - ret = PTR_ERR(bdi->battery);
> - goto out_charger;
> + /* the battery class is deprecated and will be removed. */
> + /* in the interim, this property hides it. */
> + if (!device_property_read_bool(dev, "omit-battery-class")) {
> + battery_cfg.drv_data = bdi;
> + bdi->battery = power_supply_register(dev, &bq24190_battery_desc,
> + &battery_cfg);
> + if (IS_ERR(bdi->battery)) {
> + dev_err(dev, "Can't register battery\n");
> + ret = PTR_ERR(bdi->battery);
> + goto out_charger;
> + }
> }
>
> ret = bq24190_sysfs_create_group(bdi);
> if (ret) {
> dev_err(dev, "Can't create sysfs entries\n");
> - goto out_battery;
> + goto out_charger;
> }
>
> bdi->initialized = true;
> @@ -1522,10 +1597,9 @@ static int bq24190_probe(struct i2c_client *client,
> out_sysfs:
> bq24190_sysfs_remove_group(bdi);
>
> -out_battery:
> - power_supply_unregister(bdi->battery);
> -
> out_charger:
> + if (!IS_ERR_OR_NULL(bdi->battery))
> + power_supply_unregister(bdi->battery);
> power_supply_unregister(bdi->charger);
>
> out_pmrt:
> @@ -1548,7 +1622,8 @@ static int bq24190_remove(struct i2c_client *client)
>
> bq24190_register_reset(bdi);
> bq24190_sysfs_remove_group(bdi);
> - power_supply_unregister(bdi->battery);
> + if (bdi->battery)
> + power_supply_unregister(bdi->battery);
> power_supply_unregister(bdi->charger);
> if (error >= 0)
> pm_runtime_put_sync(bdi->dev);
> @@ -1635,7 +1710,8 @@ static __maybe_unused int bq24190_pm_resume(struct device *dev)
>
> /* Things may have changed while suspended so alert upper layer */
> power_supply_changed(bdi->charger);
> - power_supply_changed(bdi->battery);
> + if (bdi->battery)
> + power_supply_changed(bdi->battery);
>
> return 0;
> }
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v1 1/1] power: bq24190_charger: Deprecate battery class and replicate its features in charger
2017-04-18 8:05 ` Hans de Goede
@ 2017-04-18 9:42 ` Liam Breck
2017-04-18 10:05 ` Hans de Goede
0 siblings, 1 reply; 7+ messages in thread
From: Liam Breck @ 2017-04-18 9:42 UTC (permalink / raw)
To: Hans de Goede; +Cc: Sebastian Reichel, linux-pm, Tony Lindgren, Liam Breck
Hi Hans,
On Tue, Apr 18, 2017 at 1:05 AM, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi Liam,
>
> On 17-04-17 01:06, Liam Breck wrote:
>>
>> From: Liam Breck <kernel@networkimprov.net>
>>
>> The driver was registering two classes, bq24190-battery & -charger.
>> Because the power supply framework cannot surface features from multiple
>> drivers in a single class, a fuel gauge driver would create a third class,
>> which some power management utilities cannot see.
>>
>> Deprecate the -battery class for future removal and replicate its features
>> in -charger. Set /sys/class...-charger/online = pg_stat &&
>> !batfet_disable.
>> If device_property "omit-battery-class" is set, don't register -battery.
>>
>> Cc: Tony Lindgren <tony@atomide.com>
>> Cc: Hans de Goede <hdegoede@redhat.com>
>> Signed-off-by: Liam Breck <kernel@networkimprov.net>
>
>
> Thank you for writing this patch.
Glad to help!
> The patch looks good to me:
Darn, I was hoping you'd see *something* amiss in all that clutter :-)
Do you think charger-powered and battery-connected is the right
definition of power_supply_prop_online?
Sebastian had suggested surfacing the batfet as a regulator, but altho
user settable, it's a binary widget.
> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
>
> Regards,
>
> Hans
>
> p.s.
>
> Any chance you can find some time to review:
>
> "[PATCH v6] power: supply: bq24190_charger: Add disable-reset
> device-property" ?
>
>
>
>
>
>> ---
>> drivers/power/supply/bq24190_charger.c | 146
>> +++++++++++++++++++++++++--------
>> 1 file changed, 111 insertions(+), 35 deletions(-)
>>
>> diff --git a/drivers/power/supply/bq24190_charger.c
>> b/drivers/power/supply/bq24190_charger.c
>> index f581042..4197fdb 100644
>> --- a/drivers/power/supply/bq24190_charger.c
>> +++ b/drivers/power/supply/bq24190_charger.c
>> @@ -658,22 +658,25 @@ static int bq24190_charger_get_health(struct
>> bq24190_dev_info *bdi,
>> v = bdi->f_reg;
>> mutex_unlock(&bdi->f_reg_lock);
>>
>> - if (v & BQ24190_REG_F_BOOST_FAULT_MASK) {
>> - /*
>> - * This could be over-current or over-voltage but there's
>> - * no way to tell which. Return 'OVERVOLTAGE' since there
>> - * isn't an 'OVERCURRENT' value defined that we can return
>> - * even if it was over-current.
>> - */
>> - health = POWER_SUPPLY_HEALTH_OVERVOLTAGE;
>> - } else {
>> - v &= BQ24190_REG_F_CHRG_FAULT_MASK;
>> - v >>= BQ24190_REG_F_CHRG_FAULT_SHIFT;
>> -
>> - switch (v) {
>> - case 0x0: /* Normal */
>> - health = POWER_SUPPLY_HEALTH_GOOD;
>> + if (v & BQ24190_REG_F_NTC_FAULT_MASK) {
>> + switch (v >> BQ24190_REG_F_NTC_FAULT_SHIFT & 0x7) {
>> + case 0x1: /* TS1 Cold */
>> + case 0x3: /* TS2 Cold */
>> + case 0x5: /* Both Cold */
>> + health = POWER_SUPPLY_HEALTH_COLD;
>> break;
>> + case 0x2: /* TS1 Hot */
>> + case 0x4: /* TS2 Hot */
>> + case 0x6: /* Both Hot */
>> + health = POWER_SUPPLY_HEALTH_OVERHEAT;
>> + break;
>> + default:
>> + health = POWER_SUPPLY_HEALTH_UNKNOWN;
>> + }
>> + } else if (v & BQ24190_REG_F_BAT_FAULT_MASK) {
>> + health = POWER_SUPPLY_HEALTH_OVERVOLTAGE;
>> + } else if (v & BQ24190_REG_F_CHRG_FAULT_MASK) {
>> + switch (v >> BQ24190_REG_F_CHRG_FAULT_SHIFT & 0x3) {
>> case 0x1: /* Input Fault (VBUS OVP or VBAT<VBUS<3.8V) */
>> /*
>> * This could be over-voltage or under-voltage
>> @@ -690,9 +693,19 @@ static int bq24190_charger_get_health(struct
>> bq24190_dev_info *bdi,
>> case 0x3: /* Charge Safety Timer Expiration */
>> health = POWER_SUPPLY_HEALTH_SAFETY_TIMER_EXPIRE;
>> break;
>> - default:
>> - health = POWER_SUPPLY_HEALTH_UNKNOWN;
>> + default: /* prevent compiler warning */
>> + health = -1;
>> }
>> + } else if (v & BQ24190_REG_F_BOOST_FAULT_MASK) {
>> + /*
>> + * This could be over-current or over-voltage but there's
>> + * no way to tell which. Return 'OVERVOLTAGE' since there
>> + * isn't an 'OVERCURRENT' value defined that we can return
>> + * even if it was over-current.
>> + */
>> + health = POWER_SUPPLY_HEALTH_OVERVOLTAGE;
>> + } else {
>> + health = POWER_SUPPLY_HEALTH_GOOD;
>> }
>>
>> val->intval = health;
>> @@ -703,19 +716,59 @@ static int bq24190_charger_get_health(struct
>> bq24190_dev_info *bdi,
>> static int bq24190_charger_get_online(struct bq24190_dev_info *bdi,
>> union power_supply_propval *val)
>> {
>> - u8 v;
>> + u8 pg_stat, batfet_disable;
>> int ret;
>>
>> ret = bq24190_read_mask(bdi, BQ24190_REG_SS,
>> BQ24190_REG_SS_PG_STAT_MASK,
>> - BQ24190_REG_SS_PG_STAT_SHIFT, &v);
>> + BQ24190_REG_SS_PG_STAT_SHIFT, &pg_stat);
>> if (ret < 0)
>> return ret;
>>
>> - val->intval = v;
>> + ret = bq24190_read_mask(bdi, BQ24190_REG_MOC,
>> + BQ24190_REG_MOC_BATFET_DISABLE_MASK,
>> + BQ24190_REG_MOC_BATFET_DISABLE_SHIFT,
>> &batfet_disable);
>> + if (ret < 0)
>> + return ret;
>> +
>> + val->intval = pg_stat && !batfet_disable;
>> +
>> return 0;
>> }
>>
>> +static int bq24190_battery_set_online(struct bq24190_dev_info *bdi,
>> + const union power_supply_propval
>> *val);
>> +static int bq24190_battery_get_status(struct bq24190_dev_info *bdi,
>> + union power_supply_propval *val);
>> +static int bq24190_battery_get_temp_alert_max(struct bq24190_dev_info
>> *bdi,
>> + union power_supply_propval
>> *val);
>> +static int bq24190_battery_set_temp_alert_max(struct bq24190_dev_info
>> *bdi,
>> + const union
>> power_supply_propval *val);
>> +
>> +static int bq24190_charger_set_online(struct bq24190_dev_info *bdi,
>> + const union power_supply_propval
>> *val)
>> +{
>> + return bq24190_battery_set_online(bdi, val);
>> +}
>> +
>> +static int bq24190_charger_get_status(struct bq24190_dev_info *bdi,
>> + union power_supply_propval *val)
>> +{
>> + return bq24190_battery_get_status(bdi, val);
>> +}
>> +
>> +static int bq24190_charger_get_temp_alert_max(struct bq24190_dev_info
>> *bdi,
>> + union power_supply_propval
>> *val)
>> +{
>> + return bq24190_battery_get_temp_alert_max(bdi, val);
>> +}
>> +
>> +static int bq24190_charger_set_temp_alert_max(struct bq24190_dev_info
>> *bdi,
>> + const union
>> power_supply_propval *val)
>> +{
>> + return bq24190_battery_set_temp_alert_max(bdi, val);
>> +}
>> +
>> static int bq24190_charger_get_current(struct bq24190_dev_info *bdi,
>> union power_supply_propval *val)
>> {
>> @@ -830,6 +883,12 @@ static int bq24190_charger_get_property(struct
>> power_supply *psy,
>> case POWER_SUPPLY_PROP_ONLINE:
>> ret = bq24190_charger_get_online(bdi, val);
>> break;
>> + case POWER_SUPPLY_PROP_STATUS:
>> + ret = bq24190_charger_get_status(bdi, val);
>> + break;
>> + case POWER_SUPPLY_PROP_TEMP_ALERT_MAX:
>> + ret = bq24190_charger_get_temp_alert_max(bdi, val);
>> + break;
>> case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT:
>> ret = bq24190_charger_get_current(bdi, val);
>> break;
>> @@ -878,6 +937,12 @@ static int bq24190_charger_set_property(struct
>> power_supply *psy,
>> return ret;
>>
>> switch (psp) {
>> + case POWER_SUPPLY_PROP_ONLINE:
>> + ret = bq24190_charger_set_online(bdi, val);
>> + break;
>> + case POWER_SUPPLY_PROP_TEMP_ALERT_MAX:
>> + ret = bq24190_charger_set_temp_alert_max(bdi, val);
>> + break;
>> case POWER_SUPPLY_PROP_CHARGE_TYPE:
>> ret = bq24190_charger_set_charge_type(bdi, val);
>> break;
>> @@ -903,6 +968,8 @@ static int
>> bq24190_charger_property_is_writeable(struct power_supply *psy,
>> int ret;
>>
>> switch (psp) {
>> + case POWER_SUPPLY_PROP_ONLINE:
>> + case POWER_SUPPLY_PROP_TEMP_ALERT_MAX:
>> case POWER_SUPPLY_PROP_CHARGE_TYPE:
>> case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT:
>> case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE:
>> @@ -919,6 +986,8 @@ static enum power_supply_property
>> bq24190_charger_properties[] = {
>> POWER_SUPPLY_PROP_CHARGE_TYPE,
>> POWER_SUPPLY_PROP_HEALTH,
>> POWER_SUPPLY_PROP_ONLINE,
>> + POWER_SUPPLY_PROP_STATUS,
>> + POWER_SUPPLY_PROP_TEMP_ALERT_MAX,
>> POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT,
>> POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX,
>> POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE,
>> @@ -1092,6 +1161,7 @@ static int bq24190_battery_get_property(struct
>> power_supply *psy,
>> struct bq24190_dev_info *bdi = power_supply_get_drvdata(psy);
>> int ret;
>>
>> + dev_warn(bdi->dev, "warning:
>> /sys/class/power_supply/bq24190-battery is deprecated\n");
>> dev_dbg(bdi->dev, "prop: %d\n", psp);
>>
>> ret = pm_runtime_get_sync(bdi->dev);
>> @@ -1137,6 +1207,7 @@ static int bq24190_battery_set_property(struct
>> power_supply *psy,
>> struct bq24190_dev_info *bdi = power_supply_get_drvdata(psy);
>> int ret;
>>
>> + dev_warn(bdi->dev, "warning:
>> /sys/class/power_supply/bq24190-battery is deprecated\n");
>> dev_dbg(bdi->dev, "prop: %d\n", psp);
>>
>> ret = pm_runtime_get_sync(bdi->dev);
>> @@ -1265,9 +1336,9 @@ static void bq24190_check_status(struct
>> bq24190_dev_info *bdi)
>> bdi->ss_reg = ss_reg;
>> }
>>
>> - if (alert_charger)
>> + if (alert_charger || alert_battery)
>> power_supply_changed(bdi->charger);
>> - if (alert_battery)
>> + if (alert_battery && bdi->battery)
>> power_supply_changed(bdi->battery);
>>
>> dev_dbg(bdi->dev, "ss_reg: 0x%02x, f_reg: 0x%02x\n", ss_reg,
>> f_reg);
>> @@ -1472,19 +1543,23 @@ static int bq24190_probe(struct i2c_client
>> *client,
>> goto out_pmrt;
>> }
>>
>> - battery_cfg.drv_data = bdi;
>> - bdi->battery = power_supply_register(dev, &bq24190_battery_desc,
>> - &battery_cfg);
>> - if (IS_ERR(bdi->battery)) {
>> - dev_err(dev, "Can't register battery\n");
>> - ret = PTR_ERR(bdi->battery);
>> - goto out_charger;
>> + /* the battery class is deprecated and will be removed. */
>> + /* in the interim, this property hides it. */
>> + if (!device_property_read_bool(dev, "omit-battery-class")) {
>> + battery_cfg.drv_data = bdi;
>> + bdi->battery = power_supply_register(dev,
>> &bq24190_battery_desc,
>> + &battery_cfg);
>> + if (IS_ERR(bdi->battery)) {
>> + dev_err(dev, "Can't register battery\n");
>> + ret = PTR_ERR(bdi->battery);
>> + goto out_charger;
>> + }
>> }
>>
>> ret = bq24190_sysfs_create_group(bdi);
>> if (ret) {
>> dev_err(dev, "Can't create sysfs entries\n");
>> - goto out_battery;
>> + goto out_charger;
>> }
>>
>> bdi->initialized = true;
>> @@ -1522,10 +1597,9 @@ static int bq24190_probe(struct i2c_client *client,
>> out_sysfs:
>> bq24190_sysfs_remove_group(bdi);
>>
>> -out_battery:
>> - power_supply_unregister(bdi->battery);
>> -
>> out_charger:
>> + if (!IS_ERR_OR_NULL(bdi->battery))
>> + power_supply_unregister(bdi->battery);
>> power_supply_unregister(bdi->charger);
>>
>> out_pmrt:
>> @@ -1548,7 +1622,8 @@ static int bq24190_remove(struct i2c_client *client)
>>
>> bq24190_register_reset(bdi);
>> bq24190_sysfs_remove_group(bdi);
>> - power_supply_unregister(bdi->battery);
>> + if (bdi->battery)
>> + power_supply_unregister(bdi->battery);
>> power_supply_unregister(bdi->charger);
>> if (error >= 0)
>> pm_runtime_put_sync(bdi->dev);
>> @@ -1635,7 +1710,8 @@ static __maybe_unused int bq24190_pm_resume(struct
>> device *dev)
>>
>> /* Things may have changed while suspended so alert upper layer */
>> power_supply_changed(bdi->charger);
>> - power_supply_changed(bdi->battery);
>> + if (bdi->battery)
>> + power_supply_changed(bdi->battery);
>>
>> return 0;
>> }
>>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v1 1/1] power: bq24190_charger: Deprecate battery class and replicate its features in charger
2017-04-18 9:42 ` Liam Breck
@ 2017-04-18 10:05 ` Hans de Goede
2017-04-18 10:27 ` Liam Breck
0 siblings, 1 reply; 7+ messages in thread
From: Hans de Goede @ 2017-04-18 10:05 UTC (permalink / raw)
To: Liam Breck; +Cc: Sebastian Reichel, linux-pm, Tony Lindgren, Liam Breck
Hi,
On 18-04-17 11:42, Liam Breck wrote:
> Hi Hans,
>
> On Tue, Apr 18, 2017 at 1:05 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>> Hi Liam,
>>
>> On 17-04-17 01:06, Liam Breck wrote:
>>>
>>> From: Liam Breck <kernel@networkimprov.net>
>>>
>>> The driver was registering two classes, bq24190-battery & -charger.
>>> Because the power supply framework cannot surface features from multiple
>>> drivers in a single class, a fuel gauge driver would create a third class,
>>> which some power management utilities cannot see.
>>>
>>> Deprecate the -battery class for future removal and replicate its features
>>> in -charger. Set /sys/class...-charger/online = pg_stat &&
>>> !batfet_disable.
>>> If device_property "omit-battery-class" is set, don't register -battery.
>>>
>>> Cc: Tony Lindgren <tony@atomide.com>
>>> Cc: Hans de Goede <hdegoede@redhat.com>
>>> Signed-off-by: Liam Breck <kernel@networkimprov.net>
>>
>>
>> Thank you for writing this patch.
>
> Glad to help!
>
>> The patch looks good to me:
>
> Darn, I was hoping you'd see *something* amiss in all that clutter :-)
Wel, I did not find the forward declaration of bq24190_battery_set_online
and friends pretty, but I can understand why you did this and we can
clean that up when we remove the battery interface altogether later.
> Do you think charger-powered and battery-connected is the right
> definition of power_supply_prop_online?
Now that you specifically ask I've been thinking a bit more this,
but I still can't find anything wrong with that as definition :)
> Sebastian had suggested surfacing the batfet as a regulator, but altho
> user settable, it's a binary widget.
Turning off the batfet typically will turn off the entire system,
I believe it is more of an emergency break style switch which gets
thrown by the charger under certain (very bad) conditions then
something which we would actually ever want to use / control from
within the kernel.
Regards,
Hans
>
>> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
>>
>> Regards,
>>
>> Hans
>>
>> p.s.
>>
>> Any chance you can find some time to review:
>>
>> "[PATCH v6] power: supply: bq24190_charger: Add disable-reset
>> device-property" ?
>>
>>
>>
>>
>>
>>> ---
>>> drivers/power/supply/bq24190_charger.c | 146
>>> +++++++++++++++++++++++++--------
>>> 1 file changed, 111 insertions(+), 35 deletions(-)
>>>
>>> diff --git a/drivers/power/supply/bq24190_charger.c
>>> b/drivers/power/supply/bq24190_charger.c
>>> index f581042..4197fdb 100644
>>> --- a/drivers/power/supply/bq24190_charger.c
>>> +++ b/drivers/power/supply/bq24190_charger.c
>>> @@ -658,22 +658,25 @@ static int bq24190_charger_get_health(struct
>>> bq24190_dev_info *bdi,
>>> v = bdi->f_reg;
>>> mutex_unlock(&bdi->f_reg_lock);
>>>
>>> - if (v & BQ24190_REG_F_BOOST_FAULT_MASK) {
>>> - /*
>>> - * This could be over-current or over-voltage but there's
>>> - * no way to tell which. Return 'OVERVOLTAGE' since there
>>> - * isn't an 'OVERCURRENT' value defined that we can return
>>> - * even if it was over-current.
>>> - */
>>> - health = POWER_SUPPLY_HEALTH_OVERVOLTAGE;
>>> - } else {
>>> - v &= BQ24190_REG_F_CHRG_FAULT_MASK;
>>> - v >>= BQ24190_REG_F_CHRG_FAULT_SHIFT;
>>> -
>>> - switch (v) {
>>> - case 0x0: /* Normal */
>>> - health = POWER_SUPPLY_HEALTH_GOOD;
>>> + if (v & BQ24190_REG_F_NTC_FAULT_MASK) {
>>> + switch (v >> BQ24190_REG_F_NTC_FAULT_SHIFT & 0x7) {
>>> + case 0x1: /* TS1 Cold */
>>> + case 0x3: /* TS2 Cold */
>>> + case 0x5: /* Both Cold */
>>> + health = POWER_SUPPLY_HEALTH_COLD;
>>> break;
>>> + case 0x2: /* TS1 Hot */
>>> + case 0x4: /* TS2 Hot */
>>> + case 0x6: /* Both Hot */
>>> + health = POWER_SUPPLY_HEALTH_OVERHEAT;
>>> + break;
>>> + default:
>>> + health = POWER_SUPPLY_HEALTH_UNKNOWN;
>>> + }
>>> + } else if (v & BQ24190_REG_F_BAT_FAULT_MASK) {
>>> + health = POWER_SUPPLY_HEALTH_OVERVOLTAGE;
>>> + } else if (v & BQ24190_REG_F_CHRG_FAULT_MASK) {
>>> + switch (v >> BQ24190_REG_F_CHRG_FAULT_SHIFT & 0x3) {
>>> case 0x1: /* Input Fault (VBUS OVP or VBAT<VBUS<3.8V) */
>>> /*
>>> * This could be over-voltage or under-voltage
>>> @@ -690,9 +693,19 @@ static int bq24190_charger_get_health(struct
>>> bq24190_dev_info *bdi,
>>> case 0x3: /* Charge Safety Timer Expiration */
>>> health = POWER_SUPPLY_HEALTH_SAFETY_TIMER_EXPIRE;
>>> break;
>>> - default:
>>> - health = POWER_SUPPLY_HEALTH_UNKNOWN;
>>> + default: /* prevent compiler warning */
>>> + health = -1;
>>> }
>>> + } else if (v & BQ24190_REG_F_BOOST_FAULT_MASK) {
>>> + /*
>>> + * This could be over-current or over-voltage but there's
>>> + * no way to tell which. Return 'OVERVOLTAGE' since there
>>> + * isn't an 'OVERCURRENT' value defined that we can return
>>> + * even if it was over-current.
>>> + */
>>> + health = POWER_SUPPLY_HEALTH_OVERVOLTAGE;
>>> + } else {
>>> + health = POWER_SUPPLY_HEALTH_GOOD;
>>> }
>>>
>>> val->intval = health;
>>> @@ -703,19 +716,59 @@ static int bq24190_charger_get_health(struct
>>> bq24190_dev_info *bdi,
>>> static int bq24190_charger_get_online(struct bq24190_dev_info *bdi,
>>> union power_supply_propval *val)
>>> {
>>> - u8 v;
>>> + u8 pg_stat, batfet_disable;
>>> int ret;
>>>
>>> ret = bq24190_read_mask(bdi, BQ24190_REG_SS,
>>> BQ24190_REG_SS_PG_STAT_MASK,
>>> - BQ24190_REG_SS_PG_STAT_SHIFT, &v);
>>> + BQ24190_REG_SS_PG_STAT_SHIFT, &pg_stat);
>>> if (ret < 0)
>>> return ret;
>>>
>>> - val->intval = v;
>>> + ret = bq24190_read_mask(bdi, BQ24190_REG_MOC,
>>> + BQ24190_REG_MOC_BATFET_DISABLE_MASK,
>>> + BQ24190_REG_MOC_BATFET_DISABLE_SHIFT,
>>> &batfet_disable);
>>> + if (ret < 0)
>>> + return ret;
>>> +
>>> + val->intval = pg_stat && !batfet_disable;
>>> +
>>> return 0;
>>> }
>>>
>>> +static int bq24190_battery_set_online(struct bq24190_dev_info *bdi,
>>> + const union power_supply_propval
>>> *val);
>>> +static int bq24190_battery_get_status(struct bq24190_dev_info *bdi,
>>> + union power_supply_propval *val);
>>> +static int bq24190_battery_get_temp_alert_max(struct bq24190_dev_info
>>> *bdi,
>>> + union power_supply_propval
>>> *val);
>>> +static int bq24190_battery_set_temp_alert_max(struct bq24190_dev_info
>>> *bdi,
>>> + const union
>>> power_supply_propval *val);
>>> +
>>> +static int bq24190_charger_set_online(struct bq24190_dev_info *bdi,
>>> + const union power_supply_propval
>>> *val)
>>> +{
>>> + return bq24190_battery_set_online(bdi, val);
>>> +}
>>> +
>>> +static int bq24190_charger_get_status(struct bq24190_dev_info *bdi,
>>> + union power_supply_propval *val)
>>> +{
>>> + return bq24190_battery_get_status(bdi, val);
>>> +}
>>> +
>>> +static int bq24190_charger_get_temp_alert_max(struct bq24190_dev_info
>>> *bdi,
>>> + union power_supply_propval
>>> *val)
>>> +{
>>> + return bq24190_battery_get_temp_alert_max(bdi, val);
>>> +}
>>> +
>>> +static int bq24190_charger_set_temp_alert_max(struct bq24190_dev_info
>>> *bdi,
>>> + const union
>>> power_supply_propval *val)
>>> +{
>>> + return bq24190_battery_set_temp_alert_max(bdi, val);
>>> +}
>>> +
>>> static int bq24190_charger_get_current(struct bq24190_dev_info *bdi,
>>> union power_supply_propval *val)
>>> {
>>> @@ -830,6 +883,12 @@ static int bq24190_charger_get_property(struct
>>> power_supply *psy,
>>> case POWER_SUPPLY_PROP_ONLINE:
>>> ret = bq24190_charger_get_online(bdi, val);
>>> break;
>>> + case POWER_SUPPLY_PROP_STATUS:
>>> + ret = bq24190_charger_get_status(bdi, val);
>>> + break;
>>> + case POWER_SUPPLY_PROP_TEMP_ALERT_MAX:
>>> + ret = bq24190_charger_get_temp_alert_max(bdi, val);
>>> + break;
>>> case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT:
>>> ret = bq24190_charger_get_current(bdi, val);
>>> break;
>>> @@ -878,6 +937,12 @@ static int bq24190_charger_set_property(struct
>>> power_supply *psy,
>>> return ret;
>>>
>>> switch (psp) {
>>> + case POWER_SUPPLY_PROP_ONLINE:
>>> + ret = bq24190_charger_set_online(bdi, val);
>>> + break;
>>> + case POWER_SUPPLY_PROP_TEMP_ALERT_MAX:
>>> + ret = bq24190_charger_set_temp_alert_max(bdi, val);
>>> + break;
>>> case POWER_SUPPLY_PROP_CHARGE_TYPE:
>>> ret = bq24190_charger_set_charge_type(bdi, val);
>>> break;
>>> @@ -903,6 +968,8 @@ static int
>>> bq24190_charger_property_is_writeable(struct power_supply *psy,
>>> int ret;
>>>
>>> switch (psp) {
>>> + case POWER_SUPPLY_PROP_ONLINE:
>>> + case POWER_SUPPLY_PROP_TEMP_ALERT_MAX:
>>> case POWER_SUPPLY_PROP_CHARGE_TYPE:
>>> case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT:
>>> case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE:
>>> @@ -919,6 +986,8 @@ static enum power_supply_property
>>> bq24190_charger_properties[] = {
>>> POWER_SUPPLY_PROP_CHARGE_TYPE,
>>> POWER_SUPPLY_PROP_HEALTH,
>>> POWER_SUPPLY_PROP_ONLINE,
>>> + POWER_SUPPLY_PROP_STATUS,
>>> + POWER_SUPPLY_PROP_TEMP_ALERT_MAX,
>>> POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT,
>>> POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX,
>>> POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE,
>>> @@ -1092,6 +1161,7 @@ static int bq24190_battery_get_property(struct
>>> power_supply *psy,
>>> struct bq24190_dev_info *bdi = power_supply_get_drvdata(psy);
>>> int ret;
>>>
>>> + dev_warn(bdi->dev, "warning:
>>> /sys/class/power_supply/bq24190-battery is deprecated\n");
>>> dev_dbg(bdi->dev, "prop: %d\n", psp);
>>>
>>> ret = pm_runtime_get_sync(bdi->dev);
>>> @@ -1137,6 +1207,7 @@ static int bq24190_battery_set_property(struct
>>> power_supply *psy,
>>> struct bq24190_dev_info *bdi = power_supply_get_drvdata(psy);
>>> int ret;
>>>
>>> + dev_warn(bdi->dev, "warning:
>>> /sys/class/power_supply/bq24190-battery is deprecated\n");
>>> dev_dbg(bdi->dev, "prop: %d\n", psp);
>>>
>>> ret = pm_runtime_get_sync(bdi->dev);
>>> @@ -1265,9 +1336,9 @@ static void bq24190_check_status(struct
>>> bq24190_dev_info *bdi)
>>> bdi->ss_reg = ss_reg;
>>> }
>>>
>>> - if (alert_charger)
>>> + if (alert_charger || alert_battery)
>>> power_supply_changed(bdi->charger);
>>> - if (alert_battery)
>>> + if (alert_battery && bdi->battery)
>>> power_supply_changed(bdi->battery);
>>>
>>> dev_dbg(bdi->dev, "ss_reg: 0x%02x, f_reg: 0x%02x\n", ss_reg,
>>> f_reg);
>>> @@ -1472,19 +1543,23 @@ static int bq24190_probe(struct i2c_client
>>> *client,
>>> goto out_pmrt;
>>> }
>>>
>>> - battery_cfg.drv_data = bdi;
>>> - bdi->battery = power_supply_register(dev, &bq24190_battery_desc,
>>> - &battery_cfg);
>>> - if (IS_ERR(bdi->battery)) {
>>> - dev_err(dev, "Can't register battery\n");
>>> - ret = PTR_ERR(bdi->battery);
>>> - goto out_charger;
>>> + /* the battery class is deprecated and will be removed. */
>>> + /* in the interim, this property hides it. */
>>> + if (!device_property_read_bool(dev, "omit-battery-class")) {
>>> + battery_cfg.drv_data = bdi;
>>> + bdi->battery = power_supply_register(dev,
>>> &bq24190_battery_desc,
>>> + &battery_cfg);
>>> + if (IS_ERR(bdi->battery)) {
>>> + dev_err(dev, "Can't register battery\n");
>>> + ret = PTR_ERR(bdi->battery);
>>> + goto out_charger;
>>> + }
>>> }
>>>
>>> ret = bq24190_sysfs_create_group(bdi);
>>> if (ret) {
>>> dev_err(dev, "Can't create sysfs entries\n");
>>> - goto out_battery;
>>> + goto out_charger;
>>> }
>>>
>>> bdi->initialized = true;
>>> @@ -1522,10 +1597,9 @@ static int bq24190_probe(struct i2c_client *client,
>>> out_sysfs:
>>> bq24190_sysfs_remove_group(bdi);
>>>
>>> -out_battery:
>>> - power_supply_unregister(bdi->battery);
>>> -
>>> out_charger:
>>> + if (!IS_ERR_OR_NULL(bdi->battery))
>>> + power_supply_unregister(bdi->battery);
>>> power_supply_unregister(bdi->charger);
>>>
>>> out_pmrt:
>>> @@ -1548,7 +1622,8 @@ static int bq24190_remove(struct i2c_client *client)
>>>
>>> bq24190_register_reset(bdi);
>>> bq24190_sysfs_remove_group(bdi);
>>> - power_supply_unregister(bdi->battery);
>>> + if (bdi->battery)
>>> + power_supply_unregister(bdi->battery);
>>> power_supply_unregister(bdi->charger);
>>> if (error >= 0)
>>> pm_runtime_put_sync(bdi->dev);
>>> @@ -1635,7 +1710,8 @@ static __maybe_unused int bq24190_pm_resume(struct
>>> device *dev)
>>>
>>> /* Things may have changed while suspended so alert upper layer */
>>> power_supply_changed(bdi->charger);
>>> - power_supply_changed(bdi->battery);
>>> + if (bdi->battery)
>>> + power_supply_changed(bdi->battery);
>>>
>>> return 0;
>>> }
>>>
>>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v1 1/1] power: bq24190_charger: Deprecate battery class and replicate its features in charger
2017-04-18 10:05 ` Hans de Goede
@ 2017-04-18 10:27 ` Liam Breck
2017-04-18 11:49 ` Hans de Goede
0 siblings, 1 reply; 7+ messages in thread
From: Liam Breck @ 2017-04-18 10:27 UTC (permalink / raw)
To: Hans de Goede; +Cc: Sebastian Reichel, linux-pm, Tony Lindgren, Liam Breck
On Tue, Apr 18, 2017 at 3:05 AM, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi,
>
> On 18-04-17 11:42, Liam Breck wrote:
>>
>> Hi Hans,
>>
>> On Tue, Apr 18, 2017 at 1:05 AM, Hans de Goede <hdegoede@redhat.com>
>> wrote:
>>>
>>> Hi Liam,
>>>
>>> On 17-04-17 01:06, Liam Breck wrote:
>>>>
>>>>
>>>> From: Liam Breck <kernel@networkimprov.net>
>>>>
>>>> The driver was registering two classes, bq24190-battery & -charger.
>>>> Because the power supply framework cannot surface features from multiple
>>>> drivers in a single class, a fuel gauge driver would create a third
>>>> class,
>>>> which some power management utilities cannot see.
>>>>
>>>> Deprecate the -battery class for future removal and replicate its
>>>> features
>>>> in -charger. Set /sys/class...-charger/online = pg_stat &&
>>>> !batfet_disable.
>>>> If device_property "omit-battery-class" is set, don't register -battery.
>>>>
>>>> Cc: Tony Lindgren <tony@atomide.com>
>>>> Cc: Hans de Goede <hdegoede@redhat.com>
>>>> Signed-off-by: Liam Breck <kernel@networkimprov.net>
>>>
>>>
>>>
>>> Thank you for writing this patch.
>>
>>
>> Glad to help!
>>
>>> The patch looks good to me:
>>
>>
>> Darn, I was hoping you'd see *something* amiss in all that clutter :-)
>
>
> Wel, I did not find the forward declaration of bq24190_battery_set_online
> and friends pretty, but I can understand why you did this and we can
> clean that up when we remove the battery interface altogether later.
Blame the C standards crew for that mess :-)
>> Do you think charger-powered and battery-connected is the right
>> definition of power_supply_prop_online?
>
>
> Now that you specifically ask I've been thinking a bit more this,
> but I still can't find anything wrong with that as definition :)
The power_supply_class docs really should tell us how to define this.
>> Sebastian had suggested surfacing the batfet as a regulator, but altho
>> user settable, it's a binary widget.
>
>
> Turning off the batfet typically will turn off the entire system,
> I believe it is more of an emergency break style switch which gets
> thrown by the charger under certain (very bad) conditions then
> something which we would actually ever want to use / control from
> within the kernel.
It's supposed to be disabled at the factory prior to shipping to avoid
depleting the battery before product reaches buyer. The same should
also be done for storage. So it can be an end-user switch.
>
>
>
>
>>
>>> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
>>>
>>> Regards,
>>>
>>> Hans
>>>
>>> p.s.
>>>
>>> Any chance you can find some time to review:
>>>
>>> "[PATCH v6] power: supply: bq24190_charger: Add disable-reset
>>> device-property" ?
>>>
>>>
>>>
>>>
>>>
>>>> ---
>>>> drivers/power/supply/bq24190_charger.c | 146
>>>> +++++++++++++++++++++++++--------
>>>> 1 file changed, 111 insertions(+), 35 deletions(-)
>>>>
>>>> diff --git a/drivers/power/supply/bq24190_charger.c
>>>> b/drivers/power/supply/bq24190_charger.c
>>>> index f581042..4197fdb 100644
>>>> --- a/drivers/power/supply/bq24190_charger.c
>>>> +++ b/drivers/power/supply/bq24190_charger.c
>>>> @@ -658,22 +658,25 @@ static int bq24190_charger_get_health(struct
>>>> bq24190_dev_info *bdi,
>>>> v = bdi->f_reg;
>>>> mutex_unlock(&bdi->f_reg_lock);
>>>>
>>>> - if (v & BQ24190_REG_F_BOOST_FAULT_MASK) {
>>>> - /*
>>>> - * This could be over-current or over-voltage but
>>>> there's
>>>> - * no way to tell which. Return 'OVERVOLTAGE' since
>>>> there
>>>> - * isn't an 'OVERCURRENT' value defined that we can
>>>> return
>>>> - * even if it was over-current.
>>>> - */
>>>> - health = POWER_SUPPLY_HEALTH_OVERVOLTAGE;
>>>> - } else {
>>>> - v &= BQ24190_REG_F_CHRG_FAULT_MASK;
>>>> - v >>= BQ24190_REG_F_CHRG_FAULT_SHIFT;
>>>> -
>>>> - switch (v) {
>>>> - case 0x0: /* Normal */
>>>> - health = POWER_SUPPLY_HEALTH_GOOD;
>>>> + if (v & BQ24190_REG_F_NTC_FAULT_MASK) {
>>>> + switch (v >> BQ24190_REG_F_NTC_FAULT_SHIFT & 0x7) {
>>>> + case 0x1: /* TS1 Cold */
>>>> + case 0x3: /* TS2 Cold */
>>>> + case 0x5: /* Both Cold */
>>>> + health = POWER_SUPPLY_HEALTH_COLD;
>>>> break;
>>>> + case 0x2: /* TS1 Hot */
>>>> + case 0x4: /* TS2 Hot */
>>>> + case 0x6: /* Both Hot */
>>>> + health = POWER_SUPPLY_HEALTH_OVERHEAT;
>>>> + break;
>>>> + default:
>>>> + health = POWER_SUPPLY_HEALTH_UNKNOWN;
>>>> + }
>>>> + } else if (v & BQ24190_REG_F_BAT_FAULT_MASK) {
>>>> + health = POWER_SUPPLY_HEALTH_OVERVOLTAGE;
>>>> + } else if (v & BQ24190_REG_F_CHRG_FAULT_MASK) {
>>>> + switch (v >> BQ24190_REG_F_CHRG_FAULT_SHIFT & 0x3) {
>>>> case 0x1: /* Input Fault (VBUS OVP or VBAT<VBUS<3.8V) */
>>>> /*
>>>> * This could be over-voltage or under-voltage
>>>> @@ -690,9 +693,19 @@ static int bq24190_charger_get_health(struct
>>>> bq24190_dev_info *bdi,
>>>> case 0x3: /* Charge Safety Timer Expiration */
>>>> health =
>>>> POWER_SUPPLY_HEALTH_SAFETY_TIMER_EXPIRE;
>>>> break;
>>>> - default:
>>>> - health = POWER_SUPPLY_HEALTH_UNKNOWN;
>>>> + default: /* prevent compiler warning */
>>>> + health = -1;
>>>> }
>>>> + } else if (v & BQ24190_REG_F_BOOST_FAULT_MASK) {
>>>> + /*
>>>> + * This could be over-current or over-voltage but
>>>> there's
>>>> + * no way to tell which. Return 'OVERVOLTAGE' since
>>>> there
>>>> + * isn't an 'OVERCURRENT' value defined that we can
>>>> return
>>>> + * even if it was over-current.
>>>> + */
>>>> + health = POWER_SUPPLY_HEALTH_OVERVOLTAGE;
>>>> + } else {
>>>> + health = POWER_SUPPLY_HEALTH_GOOD;
>>>> }
>>>>
>>>> val->intval = health;
>>>> @@ -703,19 +716,59 @@ static int bq24190_charger_get_health(struct
>>>> bq24190_dev_info *bdi,
>>>> static int bq24190_charger_get_online(struct bq24190_dev_info *bdi,
>>>> union power_supply_propval *val)
>>>> {
>>>> - u8 v;
>>>> + u8 pg_stat, batfet_disable;
>>>> int ret;
>>>>
>>>> ret = bq24190_read_mask(bdi, BQ24190_REG_SS,
>>>> BQ24190_REG_SS_PG_STAT_MASK,
>>>> - BQ24190_REG_SS_PG_STAT_SHIFT, &v);
>>>> + BQ24190_REG_SS_PG_STAT_SHIFT, &pg_stat);
>>>> if (ret < 0)
>>>> return ret;
>>>>
>>>> - val->intval = v;
>>>> + ret = bq24190_read_mask(bdi, BQ24190_REG_MOC,
>>>> + BQ24190_REG_MOC_BATFET_DISABLE_MASK,
>>>> + BQ24190_REG_MOC_BATFET_DISABLE_SHIFT,
>>>> &batfet_disable);
>>>> + if (ret < 0)
>>>> + return ret;
>>>> +
>>>> + val->intval = pg_stat && !batfet_disable;
>>>> +
>>>> return 0;
>>>> }
>>>>
>>>> +static int bq24190_battery_set_online(struct bq24190_dev_info *bdi,
>>>> + const union power_supply_propval
>>>> *val);
>>>> +static int bq24190_battery_get_status(struct bq24190_dev_info *bdi,
>>>> + union power_supply_propval *val);
>>>> +static int bq24190_battery_get_temp_alert_max(struct bq24190_dev_info
>>>> *bdi,
>>>> + union power_supply_propval
>>>> *val);
>>>> +static int bq24190_battery_set_temp_alert_max(struct bq24190_dev_info
>>>> *bdi,
>>>> + const union
>>>> power_supply_propval *val);
>>>> +
>>>> +static int bq24190_charger_set_online(struct bq24190_dev_info *bdi,
>>>> + const union power_supply_propval
>>>> *val)
>>>> +{
>>>> + return bq24190_battery_set_online(bdi, val);
>>>> +}
>>>> +
>>>> +static int bq24190_charger_get_status(struct bq24190_dev_info *bdi,
>>>> + union power_supply_propval *val)
>>>> +{
>>>> + return bq24190_battery_get_status(bdi, val);
>>>> +}
>>>> +
>>>> +static int bq24190_charger_get_temp_alert_max(struct bq24190_dev_info
>>>> *bdi,
>>>> + union power_supply_propval
>>>> *val)
>>>> +{
>>>> + return bq24190_battery_get_temp_alert_max(bdi, val);
>>>> +}
>>>> +
>>>> +static int bq24190_charger_set_temp_alert_max(struct bq24190_dev_info
>>>> *bdi,
>>>> + const union
>>>> power_supply_propval *val)
>>>> +{
>>>> + return bq24190_battery_set_temp_alert_max(bdi, val);
>>>> +}
>>>> +
>>>> static int bq24190_charger_get_current(struct bq24190_dev_info *bdi,
>>>> union power_supply_propval *val)
>>>> {
>>>> @@ -830,6 +883,12 @@ static int bq24190_charger_get_property(struct
>>>> power_supply *psy,
>>>> case POWER_SUPPLY_PROP_ONLINE:
>>>> ret = bq24190_charger_get_online(bdi, val);
>>>> break;
>>>> + case POWER_SUPPLY_PROP_STATUS:
>>>> + ret = bq24190_charger_get_status(bdi, val);
>>>> + break;
>>>> + case POWER_SUPPLY_PROP_TEMP_ALERT_MAX:
>>>> + ret = bq24190_charger_get_temp_alert_max(bdi, val);
>>>> + break;
>>>> case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT:
>>>> ret = bq24190_charger_get_current(bdi, val);
>>>> break;
>>>> @@ -878,6 +937,12 @@ static int bq24190_charger_set_property(struct
>>>> power_supply *psy,
>>>> return ret;
>>>>
>>>> switch (psp) {
>>>> + case POWER_SUPPLY_PROP_ONLINE:
>>>> + ret = bq24190_charger_set_online(bdi, val);
>>>> + break;
>>>> + case POWER_SUPPLY_PROP_TEMP_ALERT_MAX:
>>>> + ret = bq24190_charger_set_temp_alert_max(bdi, val);
>>>> + break;
>>>> case POWER_SUPPLY_PROP_CHARGE_TYPE:
>>>> ret = bq24190_charger_set_charge_type(bdi, val);
>>>> break;
>>>> @@ -903,6 +968,8 @@ static int
>>>> bq24190_charger_property_is_writeable(struct power_supply *psy,
>>>> int ret;
>>>>
>>>> switch (psp) {
>>>> + case POWER_SUPPLY_PROP_ONLINE:
>>>> + case POWER_SUPPLY_PROP_TEMP_ALERT_MAX:
>>>> case POWER_SUPPLY_PROP_CHARGE_TYPE:
>>>> case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT:
>>>> case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE:
>>>> @@ -919,6 +986,8 @@ static enum power_supply_property
>>>> bq24190_charger_properties[] = {
>>>> POWER_SUPPLY_PROP_CHARGE_TYPE,
>>>> POWER_SUPPLY_PROP_HEALTH,
>>>> POWER_SUPPLY_PROP_ONLINE,
>>>> + POWER_SUPPLY_PROP_STATUS,
>>>> + POWER_SUPPLY_PROP_TEMP_ALERT_MAX,
>>>> POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT,
>>>> POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX,
>>>> POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE,
>>>> @@ -1092,6 +1161,7 @@ static int bq24190_battery_get_property(struct
>>>> power_supply *psy,
>>>> struct bq24190_dev_info *bdi = power_supply_get_drvdata(psy);
>>>> int ret;
>>>>
>>>> + dev_warn(bdi->dev, "warning:
>>>> /sys/class/power_supply/bq24190-battery is deprecated\n");
>>>> dev_dbg(bdi->dev, "prop: %d\n", psp);
>>>>
>>>> ret = pm_runtime_get_sync(bdi->dev);
>>>> @@ -1137,6 +1207,7 @@ static int bq24190_battery_set_property(struct
>>>> power_supply *psy,
>>>> struct bq24190_dev_info *bdi = power_supply_get_drvdata(psy);
>>>> int ret;
>>>>
>>>> + dev_warn(bdi->dev, "warning:
>>>> /sys/class/power_supply/bq24190-battery is deprecated\n");
>>>> dev_dbg(bdi->dev, "prop: %d\n", psp);
>>>>
>>>> ret = pm_runtime_get_sync(bdi->dev);
>>>> @@ -1265,9 +1336,9 @@ static void bq24190_check_status(struct
>>>> bq24190_dev_info *bdi)
>>>> bdi->ss_reg = ss_reg;
>>>> }
>>>>
>>>> - if (alert_charger)
>>>> + if (alert_charger || alert_battery)
>>>> power_supply_changed(bdi->charger);
>>>> - if (alert_battery)
>>>> + if (alert_battery && bdi->battery)
>>>> power_supply_changed(bdi->battery);
>>>>
>>>> dev_dbg(bdi->dev, "ss_reg: 0x%02x, f_reg: 0x%02x\n", ss_reg,
>>>> f_reg);
>>>> @@ -1472,19 +1543,23 @@ static int bq24190_probe(struct i2c_client
>>>> *client,
>>>> goto out_pmrt;
>>>> }
>>>>
>>>> - battery_cfg.drv_data = bdi;
>>>> - bdi->battery = power_supply_register(dev, &bq24190_battery_desc,
>>>> - &battery_cfg);
>>>> - if (IS_ERR(bdi->battery)) {
>>>> - dev_err(dev, "Can't register battery\n");
>>>> - ret = PTR_ERR(bdi->battery);
>>>> - goto out_charger;
>>>> + /* the battery class is deprecated and will be removed. */
>>>> + /* in the interim, this property hides it. */
>>>> + if (!device_property_read_bool(dev, "omit-battery-class")) {
>>>> + battery_cfg.drv_data = bdi;
>>>> + bdi->battery = power_supply_register(dev,
>>>> &bq24190_battery_desc,
>>>> + &battery_cfg);
>>>> + if (IS_ERR(bdi->battery)) {
>>>> + dev_err(dev, "Can't register battery\n");
>>>> + ret = PTR_ERR(bdi->battery);
>>>> + goto out_charger;
>>>> + }
>>>> }
>>>>
>>>> ret = bq24190_sysfs_create_group(bdi);
>>>> if (ret) {
>>>> dev_err(dev, "Can't create sysfs entries\n");
>>>> - goto out_battery;
>>>> + goto out_charger;
>>>> }
>>>>
>>>> bdi->initialized = true;
>>>> @@ -1522,10 +1597,9 @@ static int bq24190_probe(struct i2c_client
>>>> *client,
>>>> out_sysfs:
>>>> bq24190_sysfs_remove_group(bdi);
>>>>
>>>> -out_battery:
>>>> - power_supply_unregister(bdi->battery);
>>>> -
>>>> out_charger:
>>>> + if (!IS_ERR_OR_NULL(bdi->battery))
>>>> + power_supply_unregister(bdi->battery);
>>>> power_supply_unregister(bdi->charger);
>>>>
>>>> out_pmrt:
>>>> @@ -1548,7 +1622,8 @@ static int bq24190_remove(struct i2c_client
>>>> *client)
>>>>
>>>> bq24190_register_reset(bdi);
>>>> bq24190_sysfs_remove_group(bdi);
>>>> - power_supply_unregister(bdi->battery);
>>>> + if (bdi->battery)
>>>> + power_supply_unregister(bdi->battery);
>>>> power_supply_unregister(bdi->charger);
>>>> if (error >= 0)
>>>> pm_runtime_put_sync(bdi->dev);
>>>> @@ -1635,7 +1710,8 @@ static __maybe_unused int bq24190_pm_resume(struct
>>>> device *dev)
>>>>
>>>> /* Things may have changed while suspended so alert upper layer
>>>> */
>>>> power_supply_changed(bdi->charger);
>>>> - power_supply_changed(bdi->battery);
>>>> + if (bdi->battery)
>>>> + power_supply_changed(bdi->battery);
>>>>
>>>> return 0;
>>>> }
>>>>
>>>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v1 1/1] power: bq24190_charger: Deprecate battery class and replicate its features in charger
2017-04-18 10:27 ` Liam Breck
@ 2017-04-18 11:49 ` Hans de Goede
0 siblings, 0 replies; 7+ messages in thread
From: Hans de Goede @ 2017-04-18 11:49 UTC (permalink / raw)
To: Liam Breck; +Cc: Sebastian Reichel, linux-pm, Tony Lindgren, Liam Breck
Hi,
On 18-04-17 12:27, Liam Breck wrote:
> On Tue, Apr 18, 2017 at 3:05 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>> Hi,
>>
>> On 18-04-17 11:42, Liam Breck wrote:
>>>
>>> Hi Hans,
>>>
>>> On Tue, Apr 18, 2017 at 1:05 AM, Hans de Goede <hdegoede@redhat.com>
>>> wrote:
>>>>
>>>> Hi Liam,
>>>>
>>>> On 17-04-17 01:06, Liam Breck wrote:
>>>>>
>>>>>
>>>>> From: Liam Breck <kernel@networkimprov.net>
>>>>>
>>>>> The driver was registering two classes, bq24190-battery & -charger.
>>>>> Because the power supply framework cannot surface features from multiple
>>>>> drivers in a single class, a fuel gauge driver would create a third
>>>>> class,
>>>>> which some power management utilities cannot see.
>>>>>
>>>>> Deprecate the -battery class for future removal and replicate its
>>>>> features
>>>>> in -charger. Set /sys/class...-charger/online = pg_stat &&
>>>>> !batfet_disable.
>>>>> If device_property "omit-battery-class" is set, don't register -battery.
>>>>>
>>>>> Cc: Tony Lindgren <tony@atomide.com>
>>>>> Cc: Hans de Goede <hdegoede@redhat.com>
>>>>> Signed-off-by: Liam Breck <kernel@networkimprov.net>
>>>>
>>>>
>>>>
>>>> Thank you for writing this patch.
>>>
>>>
>>> Glad to help!
>>>
>>>> The patch looks good to me:
>>>
>>>
>>> Darn, I was hoping you'd see *something* amiss in all that clutter :-)
>>
>>
>> Wel, I did not find the forward declaration of bq24190_battery_set_online
>> and friends pretty, but I can understand why you did this and we can
>> clean that up when we remove the battery interface altogether later.
>
> Blame the C standards crew for that mess :-)
>
>>> Do you think charger-powered and battery-connected is the right
>>> definition of power_supply_prop_online?
>>
>>
>> Now that you specifically ask I've been thinking a bit more this,
>> but I still can't find anything wrong with that as definition :)
>
> The power_supply_class docs really should tell us how to define this.
>
>>> Sebastian had suggested surfacing the batfet as a regulator, but altho
>>> user settable, it's a binary widget.
>>
>>
>> Turning off the batfet typically will turn off the entire system,
>> I believe it is more of an emergency break style switch which gets
>> thrown by the charger under certain (very bad) conditions then
>> something which we would actually ever want to use / control from
>> within the kernel.
>
> It's supposed to be disabled at the factory prior to shipping to avoid
> depleting the battery before product reaches buyer. The same should
> also be done for storage. So it can be an end-user switch.
True.
Regards,
Hans
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v1 1/1] power: bq24190_charger: Deprecate battery class and replicate its features in charger
2017-04-16 23:06 [PATCH v1 1/1] power: bq24190_charger: Deprecate battery class and replicate its features in charger Liam Breck
2017-04-18 8:05 ` Hans de Goede
@ 2017-05-01 11:29 ` Sebastian Reichel
1 sibling, 0 replies; 7+ messages in thread
From: Sebastian Reichel @ 2017-05-01 11:29 UTC (permalink / raw)
To: Liam Breck; +Cc: linux-pm, Tony Lindgren, Hans de Goede, Liam Breck
[-- Attachment #1: Type: text/plain, Size: 812 bytes --]
Hi,
On Sun, Apr 16, 2017 at 04:06:26PM -0700, Liam Breck wrote:
> From: Liam Breck <kernel@networkimprov.net>
>
> The driver was registering two classes, bq24190-battery & -charger.
> Because the power supply framework cannot surface features from multiple
> drivers in a single class, a fuel gauge driver would create a third class,
> which some power management utilities cannot see.
>
> Deprecate the -battery class for future removal and replicate its features
> in -charger. Set /sys/class...-charger/online = pg_stat && !batfet_disable.
> If device_property "omit-battery-class" is set, don't register -battery.
>
> Cc: Tony Lindgren <tony@atomide.com>
> Cc: Hans de Goede <hdegoede@redhat.com>
> Signed-off-by: Liam Breck <kernel@networkimprov.net>
Thanks, queued.
-- Sebastian
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2017-05-01 11:29 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-04-16 23:06 [PATCH v1 1/1] power: bq24190_charger: Deprecate battery class and replicate its features in charger Liam Breck
2017-04-18 8:05 ` Hans de Goede
2017-04-18 9:42 ` Liam Breck
2017-04-18 10:05 ` Hans de Goede
2017-04-18 10:27 ` Liam Breck
2017-04-18 11:49 ` Hans de Goede
2017-05-01 11:29 ` Sebastian Reichel
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox