[PATCH 01/16] power: supply: axp288_charger: Do not stop + restart charging at boot

[PATCH 01/16] power: supply: axp288_charger: Do not stop + restart charging at boot

[Hans de Goede]

Before this commit we were relying solely on the extcon interface for
cable detection, including to determine if a cable providing vbus is
connected at all. This caused us to turn off charging at boot, because
when we run the initial state processing the axp288-extcon driver is still
running charger-type detection most of the time, so all charger cable
types read as disconnected when we run the initial state processing.

This commit reworks the axp288_charger_extcon_evt_worker flow to use the
VBUS_VALID bit from the PWR_INPUT_STATUS register to determine if we
should turn charging on/off. Note this is the same bit as we use for the
online property.

If VBUS_VALID is set, but the extcon code has not completed the charger
type detection yet, we now simply bail leaving things as configured by
the BIOS (we will get a notifier call when the extcon code is done and
reschedule the axp288_charger_extcon_evt_worker).

The extcon code is the only one to trigger the worker (outside of the
initial run) and we can rely on it to only call us if things have changed,
so while we are completely refactoring axp288_charger_extcon_evt_worker,
also remove the code to check if the state has changed.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/power/supply/axp288_charger.c | 81 +++++++++++++----------------------
 1 file changed, 30 insertions(+), 51 deletions(-)

diff --git a/drivers/power/supply/axp288_charger.c b/drivers/power/supply/axp288_charger.c
index d51ebd1da65e..54a95d196306 100644
--- a/drivers/power/supply/axp288_charger.c
+++ b/drivers/power/supply/axp288_charger.c
@@ -1,6 +1,7 @@
 /*
  * axp288_charger.c - X-power AXP288 PMIC Charger driver
  *
+ * Copyright (C) 2016-2017 Hans de Goede <hdegoede@redhat.com>
  * Copyright (C) 2014 Intel Corporation
  * Author: Ramakrishna Pallala <ramakrishna.pallala@intel.com>
  *
@@ -152,8 +153,6 @@ struct axp288_chrg_info {
 	/* SDP/CDP/DCP USB charging cable notifications */
 	struct {
 		struct extcon_dev *edev;
-		bool connected;
-		enum power_supply_type chg_type;
 		struct notifier_block nb[ARRAY_SIZE(cable_ids)];
 		struct work_struct work;
 	} cable;
@@ -565,66 +564,47 @@ static void axp288_charger_extcon_evt_worker(struct work_struct *work)
 	    container_of(work, struct axp288_chrg_info, cable.work);
 	int ret, current_limit;
 	struct extcon_dev *edev = info->cable.edev;
-	bool old_connected = info->cable.connected;
-	enum power_supply_type old_chg_type = info->cable.chg_type;
+	unsigned int val;
+
+	ret = regmap_read(info->regmap, AXP20X_PWR_INPUT_STATUS, &val);
+	if (ret < 0) {
+		dev_err(&info->pdev->dev, "Error reading status (%d)\n", ret);
+		return;
+	}
+
+	/* Offline? Disable charging and bail */
+	if (!(val & PS_STAT_VBUS_VALID)) {
+		dev_dbg(&info->pdev->dev, "USB charger disconnected\n");
+		mutex_lock(&info->lock);
+		axp288_charger_enable_charger(info, false);
+		mutex_unlock(&info->lock);
+		power_supply_changed(info->psy_usb);
+		return;
+	}
 
 	/* Determine cable/charger type */
 	if (extcon_get_state(edev, EXTCON_CHG_USB_SDP) > 0) {
 		dev_dbg(&info->pdev->dev, "USB SDP charger  is connected");
-		info->cable.connected = true;
-		info->cable.chg_type = POWER_SUPPLY_TYPE_USB;
+		current_limit = ILIM_500MA;
 	} else if (extcon_get_state(edev, EXTCON_CHG_USB_CDP) > 0) {
 		dev_dbg(&info->pdev->dev, "USB CDP charger is connected");
-		info->cable.connected = true;
-		info->cable.chg_type = POWER_SUPPLY_TYPE_USB_CDP;
+		current_limit = ILIM_1500MA;
 	} else if (extcon_get_state(edev, EXTCON_CHG_USB_DCP) > 0) {
 		dev_dbg(&info->pdev->dev, "USB DCP charger is connected");
-		info->cable.connected = true;
-		info->cable.chg_type = POWER_SUPPLY_TYPE_USB_DCP;
+		current_limit = ILIM_2000MA;
 	} else {
-		if (old_connected)
-			dev_dbg(&info->pdev->dev, "USB charger disconnected");
-		info->cable.connected = false;
-		info->cable.chg_type = POWER_SUPPLY_TYPE_USB;
-	}
-
-	/* Cable status changed */
-	if (old_connected == info->cable.connected &&
-	    old_chg_type == info->cable.chg_type)
+		/* Charger type detection still in progress, bail. */
 		return;
-
-	mutex_lock(&info->lock);
-
-	if (info->cable.connected) {
-		axp288_charger_enable_charger(info, false);
-
-		switch (info->cable.chg_type) {
-		case POWER_SUPPLY_TYPE_USB:
-			current_limit = ILIM_500MA;
-			break;
-		case POWER_SUPPLY_TYPE_USB_CDP:
-			current_limit = ILIM_1500MA;
-			break;
-		case POWER_SUPPLY_TYPE_USB_DCP:
-			current_limit = ILIM_2000MA;
-			break;
-		default:
-			/* Unknown */
-			current_limit = 0;
-			break;
-		}
-
-		/* Set vbus current limit first, then enable charger */
-		ret = axp288_charger_set_vbus_inlmt(info, current_limit);
-		if (ret == 0)
-			axp288_charger_enable_charger(info, true);
-		else
-			dev_err(&info->pdev->dev,
-				"error setting current limit (%d)", ret);
-	} else {
-		axp288_charger_enable_charger(info, false);
 	}
 
+	mutex_lock(&info->lock);
+	/* Set vbus current limit first, then enable charger */
+	ret = axp288_charger_set_vbus_inlmt(info, current_limit);
+	if (ret == 0)
+		axp288_charger_enable_charger(info, true);
+	else
+		dev_err(&info->pdev->dev,
+			"error setting current limit (%d)\n", ret);
 	mutex_unlock(&info->lock);
 
 	power_supply_changed(info->psy_usb);
@@ -799,7 +779,6 @@ static int axp288_charger_probe(struct platform_device *pdev)
 	info->pdev = pdev;
 	info->regmap = axp20x->regmap;
 	info->regmap_irqc = axp20x->regmap_irqc;
-	info->cable.chg_type = -1;
 	info->is_charger_enabled = -1;
 
 	info->cable.edev = extcon_get_extcon_dev(AXP288_EXTCON_DEV_NAME);
-- 
2.14.3

[PATCH 02/16] power: supply: axp288_charger: Add missing newlines to some messages

[Hans de Goede]

Add missing (terminating) "\n"-s to some dev_dbg messages.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/power/supply/axp288_charger.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/power/supply/axp288_charger.c b/drivers/power/supply/axp288_charger.c
index 54a95d196306..d398f8ee296d 100644
--- a/drivers/power/supply/axp288_charger.c
+++ b/drivers/power/supply/axp288_charger.c
@@ -584,13 +584,13 @@ static void axp288_charger_extcon_evt_worker(struct work_struct *work)
 
 	/* Determine cable/charger type */
 	if (extcon_get_state(edev, EXTCON_CHG_USB_SDP) > 0) {
-		dev_dbg(&info->pdev->dev, "USB SDP charger  is connected");
+		dev_dbg(&info->pdev->dev, "USB SDP charger is connected\n");
 		current_limit = ILIM_500MA;
 	} else if (extcon_get_state(edev, EXTCON_CHG_USB_CDP) > 0) {
-		dev_dbg(&info->pdev->dev, "USB CDP charger is connected");
+		dev_dbg(&info->pdev->dev, "USB CDP charger is connected\n");
 		current_limit = ILIM_1500MA;
 	} else if (extcon_get_state(edev, EXTCON_CHG_USB_DCP) > 0) {
-		dev_dbg(&info->pdev->dev, "USB DCP charger is connected");
+		dev_dbg(&info->pdev->dev, "USB DCP charger is connected\n");
 		current_limit = ILIM_2000MA;
 	} else {
 		/* Charger type detection still in progress, bail. */
-- 
2.14.3

[PATCH 03/16] power: supply: axp288_charger: Remove charger-enabled state tracking

[Hans de Goede]

The extcon code is the only one to trigger our worker (outside of the
initial run) and we can rely on it to only call us if things have
changed, so there is no need to track the charger-enabled state.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/power/supply/axp288_charger.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/drivers/power/supply/axp288_charger.c b/drivers/power/supply/axp288_charger.c
index d398f8ee296d..5d8308c8835d 100644
--- a/drivers/power/supply/axp288_charger.c
+++ b/drivers/power/supply/axp288_charger.c
@@ -162,7 +162,6 @@ struct axp288_chrg_info {
 	int cv;
 	int max_cc;
 	int max_cv;
-	int is_charger_enabled;
 };
 
 static inline int axp288_charger_set_cc(struct axp288_chrg_info *info, int cc)
@@ -291,9 +290,6 @@ static int axp288_charger_enable_charger(struct axp288_chrg_info *info,
 {
 	int ret;
 
-	if ((int)enable == info->is_charger_enabled)
-		return 0;
-
 	if (enable)
 		ret = regmap_update_bits(info->regmap, AXP20X_CHRG_CTRL1,
 				CHRG_CCCV_CHG_EN, CHRG_CCCV_CHG_EN);
@@ -302,8 +298,6 @@ static int axp288_charger_enable_charger(struct axp288_chrg_info *info,
 				CHRG_CCCV_CHG_EN, 0);
 	if (ret < 0)
 		dev_err(&info->pdev->dev, "axp288 enable charger %d\n", ret);
-	else
-		info->is_charger_enabled = enable;
 
 	return ret;
 }
@@ -779,7 +773,6 @@ static int axp288_charger_probe(struct platform_device *pdev)
 	info->pdev = pdev;
 	info->regmap = axp20x->regmap;
 	info->regmap_irqc = axp20x->regmap_irqc;
-	info->is_charger_enabled = -1;
 
 	info->cable.edev = extcon_get_extcon_dev(AXP288_EXTCON_DEV_NAME);
 	if (info->cable.edev == NULL) {
-- 
2.14.3

[PATCH 04/16] power: supply: axp288_charger: Cleanup some double empty lines

[Hans de Goede]

While we are doing cleanups, also remove some double blank lines.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/power/supply/axp288_charger.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/power/supply/axp288_charger.c b/drivers/power/supply/axp288_charger.c
index 5d8308c8835d..03f502e012c3 100644
--- a/drivers/power/supply/axp288_charger.c
+++ b/drivers/power/supply/axp288_charger.c
@@ -261,7 +261,6 @@ static inline int axp288_charger_set_vbus_inlmt(struct axp288_chrg_info *info,
 	else
 		dev_err(&info->pdev->dev, "charger BAK control %d\n", ret);
 
-
 set_inlmt_fail:
 	return ret;
 }
@@ -281,7 +280,6 @@ static int axp288_charger_vbus_path_select(struct axp288_chrg_info *info,
 	if (ret < 0)
 		dev_err(&info->pdev->dev, "axp288 vbus path select %d\n", ret);
 
-
 	return ret;
 }
 
-- 
2.14.3

[PATCH 05/16] power: supply: axp288_charger: Use regmap_update_bits to set the input limits

[Hans de Goede]

Use regmap_update_bits in axp288_charger_set_vbus_inlmt, instead of DIY
code.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/power/supply/axp288_charger.c | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/drivers/power/supply/axp288_charger.c b/drivers/power/supply/axp288_charger.c
index 03f502e012c3..7b48afca1a1f 100644
--- a/drivers/power/supply/axp288_charger.c
+++ b/drivers/power/supply/axp288_charger.c
@@ -222,14 +222,8 @@ static inline int axp288_charger_set_vbus_inlmt(struct axp288_chrg_info *info,
 					   int inlmt)
 {
 	int ret;
-	unsigned int val;
 	u8 reg_val;
 
-	/* Read in limit register */
-	ret = regmap_read(info->regmap, AXP20X_CHRG_BAK_CTRL, &val);
-	if (ret < 0)
-		goto set_inlmt_fail;
-
 	if (inlmt <= ILIM_100MA) {
 		reg_val = CHRG_VBUS_ILIM_100MA;
 		inlmt = ILIM_100MA;
@@ -253,15 +247,15 @@ static inline int axp288_charger_set_vbus_inlmt(struct axp288_chrg_info *info,
 		inlmt = ILIM_3000MA;
 	}
 
-	reg_val = (val & ~CHRG_VBUS_ILIM_MASK)
-			| (reg_val << CHRG_VBUS_ILIM_BIT_POS);
-	ret = regmap_write(info->regmap, AXP20X_CHRG_BAK_CTRL, reg_val);
+	reg_val = reg_val << CHRG_VBUS_ILIM_BIT_POS;
+
+	ret = regmap_update_bits(info->regmap, AXP20X_CHRG_BAK_CTRL,
+				 CHRG_VBUS_ILIM_MASK, reg_val);
 	if (ret >= 0)
 		info->inlmt = inlmt;
 	else
 		dev_err(&info->pdev->dev, "charger BAK control %d\n", ret);
 
-set_inlmt_fail:
 	return ret;
 }
 
-- 
2.14.3

[PATCH 06/16] power: supply: axp288_charger: Remove no longer needed locking

[Hans de Goede]

Now that we use regmap to do read-modify-write ops everywhere, we can
rely on the regmap lock and no longer need our own lock.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/power/supply/axp288_charger.c | 24 +++++-------------------
 1 file changed, 5 insertions(+), 19 deletions(-)

diff --git a/drivers/power/supply/axp288_charger.c b/drivers/power/supply/axp288_charger.c
index 7b48afca1a1f..070dd79e606f 100644
--- a/drivers/power/supply/axp288_charger.c
+++ b/drivers/power/supply/axp288_charger.c
@@ -140,7 +140,6 @@ struct axp288_chrg_info {
 	struct regmap_irq_chip_data *regmap_irqc;
 	int irq[CHRG_INTR_END];
 	struct power_supply *psy_usb;
-	struct mutex lock;
 
 	/* OTG/Host mode */
 	struct {
@@ -361,8 +360,6 @@ static int axp288_charger_usb_set_property(struct power_supply *psy,
 	int ret = 0;
 	int scaled_val;
 
-	mutex_lock(&info->lock);
-
 	switch (psp) {
 	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT:
 		scaled_val = min(val->intval, info->max_cc);
@@ -382,7 +379,6 @@ static int axp288_charger_usb_set_property(struct power_supply *psy,
 		ret = -EINVAL;
 	}
 
-	mutex_unlock(&info->lock);
 	return ret;
 }
 
@@ -391,9 +387,7 @@ static int axp288_charger_usb_get_property(struct power_supply *psy,
 				    union power_supply_propval *val)
 {
 	struct axp288_chrg_info *info = power_supply_get_drvdata(psy);
-	int ret = 0;
-
-	mutex_lock(&info->lock);
+	int ret;
 
 	switch (psp) {
 	case POWER_SUPPLY_PROP_PRESENT:
@@ -404,7 +398,7 @@ static int axp288_charger_usb_get_property(struct power_supply *psy,
 		}
 		ret = axp288_charger_is_present(info);
 		if (ret < 0)
-			goto psy_get_prop_fail;
+			return ret;
 		val->intval = ret;
 		break;
 	case POWER_SUPPLY_PROP_ONLINE:
@@ -415,7 +409,7 @@ static int axp288_charger_usb_get_property(struct power_supply *psy,
 		}
 		ret = axp288_charger_is_online(info);
 		if (ret < 0)
-			goto psy_get_prop_fail;
+			return ret;
 		val->intval = ret;
 		break;
 	case POWER_SUPPLY_PROP_HEALTH:
@@ -437,13 +431,10 @@ static int axp288_charger_usb_get_property(struct power_supply *psy,
 		val->intval = info->inlmt * 1000;
 		break;
 	default:
-		ret = -EINVAL;
-		goto psy_get_prop_fail;
+		return -EINVAL;
 	}
 
-psy_get_prop_fail:
-	mutex_unlock(&info->lock);
-	return ret;
+	return 0;
 }
 
 static int axp288_charger_property_is_writeable(struct power_supply *psy,
@@ -561,9 +552,7 @@ static void axp288_charger_extcon_evt_worker(struct work_struct *work)
 	/* Offline? Disable charging and bail */
 	if (!(val & PS_STAT_VBUS_VALID)) {
 		dev_dbg(&info->pdev->dev, "USB charger disconnected\n");
-		mutex_lock(&info->lock);
 		axp288_charger_enable_charger(info, false);
-		mutex_unlock(&info->lock);
 		power_supply_changed(info->psy_usb);
 		return;
 	}
@@ -583,7 +572,6 @@ static void axp288_charger_extcon_evt_worker(struct work_struct *work)
 		return;
 	}
 
-	mutex_lock(&info->lock);
 	/* Set vbus current limit first, then enable charger */
 	ret = axp288_charger_set_vbus_inlmt(info, current_limit);
 	if (ret == 0)
@@ -591,7 +579,6 @@ static void axp288_charger_extcon_evt_worker(struct work_struct *work)
 	else
 		dev_err(&info->pdev->dev,
 			"error setting current limit (%d)\n", ret);
-	mutex_unlock(&info->lock);
 
 	power_supply_changed(info->psy_usb);
 }
@@ -784,7 +771,6 @@ static int axp288_charger_probe(struct platform_device *pdev)
 	}
 
 	platform_set_drvdata(pdev, info);
-	mutex_init(&info->lock);
 
 	ret = charger_init_hw_regs(info);
 	if (ret)
-- 
2.14.3

[PATCH 07/16] power: supply: axp288_charger: Do not cache input current limit value

[Hans de Goede]

The hardware may change this underneath us.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/power/supply/axp288_charger.c | 48 ++++++++++++++++++++++++++---------
 1 file changed, 36 insertions(+), 12 deletions(-)

diff --git a/drivers/power/supply/axp288_charger.c b/drivers/power/supply/axp288_charger.c
index 070dd79e606f..2c7b0efbbf3d 100644
--- a/drivers/power/supply/axp288_charger.c
+++ b/drivers/power/supply/axp288_charger.c
@@ -156,7 +156,6 @@ struct axp288_chrg_info {
 		struct work_struct work;
 	} cable;
 
-	int inlmt;
 	int cc;
 	int cv;
 	int max_cc;
@@ -217,6 +216,37 @@ static inline int axp288_charger_set_cv(struct axp288_chrg_info *info, int cv)
 	return ret;
 }
 
+static int axp288_charger_get_vbus_inlmt(struct axp288_chrg_info *info)
+{
+	unsigned int val;
+	int ret;
+
+	ret = regmap_read(info->regmap, AXP20X_CHRG_BAK_CTRL, &val);
+	if (ret < 0)
+		return ret;
+
+	val >>= CHRG_VBUS_ILIM_BIT_POS;
+	switch (val) {
+	case CHRG_VBUS_ILIM_100MA:
+		return 100000;
+	case CHRG_VBUS_ILIM_500MA:
+		return 500000;
+	case CHRG_VBUS_ILIM_900MA:
+		return 900000;
+	case CHRG_VBUS_ILIM_1500MA:
+		return 1500000;
+	case CHRG_VBUS_ILIM_2000MA:
+		return 2000000;
+	case CHRG_VBUS_ILIM_2500MA:
+		return 2500000;
+	case CHRG_VBUS_ILIM_3000MA:
+		return 3000000;
+	default:
+		dev_warn(&info->pdev->dev, "Unknown ilim reg val: %d\n", val);
+		return 0;
+	}
+}
+
 static inline int axp288_charger_set_vbus_inlmt(struct axp288_chrg_info *info,
 					   int inlmt)
 {
@@ -225,34 +255,25 @@ static inline int axp288_charger_set_vbus_inlmt(struct axp288_chrg_info *info,
 
 	if (inlmt <= ILIM_100MA) {
 		reg_val = CHRG_VBUS_ILIM_100MA;
-		inlmt = ILIM_100MA;
 	} else if (inlmt <= ILIM_500MA) {
 		reg_val = CHRG_VBUS_ILIM_500MA;
-		inlmt = ILIM_500MA;
 	} else if (inlmt <= ILIM_900MA) {
 		reg_val = CHRG_VBUS_ILIM_900MA;
-		inlmt = ILIM_900MA;
 	} else if (inlmt <= ILIM_1500MA) {
 		reg_val = CHRG_VBUS_ILIM_1500MA;
-		inlmt = ILIM_1500MA;
 	} else if (inlmt <= ILIM_2000MA) {
 		reg_val = CHRG_VBUS_ILIM_2000MA;
-		inlmt = ILIM_2000MA;
 	} else if (inlmt <= ILIM_2500MA) {
 		reg_val = CHRG_VBUS_ILIM_2500MA;
-		inlmt = ILIM_2500MA;
 	} else {
 		reg_val = CHRG_VBUS_ILIM_3000MA;
-		inlmt = ILIM_3000MA;
 	}
 
 	reg_val = reg_val << CHRG_VBUS_ILIM_BIT_POS;
 
 	ret = regmap_update_bits(info->regmap, AXP20X_CHRG_BAK_CTRL,
 				 CHRG_VBUS_ILIM_MASK, reg_val);
-	if (ret >= 0)
-		info->inlmt = inlmt;
-	else
+	if (ret < 0)
 		dev_err(&info->pdev->dev, "charger BAK control %d\n", ret);
 
 	return ret;
@@ -428,7 +449,10 @@ static int axp288_charger_usb_get_property(struct power_supply *psy,
 		val->intval = info->max_cv * 1000;
 		break;
 	case POWER_SUPPLY_PROP_CHARGE_CONTROL_LIMIT:
-		val->intval = info->inlmt * 1000;
+		ret = axp288_charger_get_vbus_inlmt(info);
+		if (ret < 0)
+			return ret;
+		val->intval = ret;
 		break;
 	default:
 		return -EINVAL;
-- 
2.14.3

[PATCH 08/16] power: supply: axp288_charger: Pick lower input current limit not higher

[Hans de Goede]

The code before this commit would pick 900 mA when asking for an input
current limit of 600mA, rather then 500 mA, not good.

While touching almost all code using the silly xxxMA defines anyways,
also get rid of these simply typing out the numbers and switch the
unit to uA as that is the psy class standard unit for currents.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/power/supply/axp288_charger.c | 52 ++++++++++++-----------------------
 1 file changed, 17 insertions(+), 35 deletions(-)

diff --git a/drivers/power/supply/axp288_charger.c b/drivers/power/supply/axp288_charger.c
index 2c7b0efbbf3d..fdca0a4a1c8e 100644
--- a/drivers/power/supply/axp288_charger.c
+++ b/drivers/power/supply/axp288_charger.c
@@ -99,21 +99,6 @@
 #define CV_4200MV			4200	/* 4200mV */
 #define CV_4350MV			4350	/* 4350mV */
 
-#define CC_200MA			200	/*  200mA */
-#define CC_600MA			600	/*  600mA */
-#define CC_800MA			800	/*  800mA */
-#define CC_1000MA			1000	/* 1000mA */
-#define CC_1600MA			1600	/* 1600mA */
-#define CC_2000MA			2000	/* 2000mA */
-
-#define ILIM_100MA			100	/* 100mA */
-#define ILIM_500MA			500	/* 500mA */
-#define ILIM_900MA			900	/* 900mA */
-#define ILIM_1500MA			1500	/* 1500mA */
-#define ILIM_2000MA			2000	/* 2000mA */
-#define ILIM_2500MA			2500	/* 2500mA */
-#define ILIM_3000MA			3000	/* 3000mA */
-
 #define AXP288_EXTCON_DEV_NAME		"axp288_extcon"
 #define USB_HOST_EXTCON_HID		"INT3496"
 #define USB_HOST_EXTCON_NAME		"INT3496:00"
@@ -253,23 +238,20 @@ static inline int axp288_charger_set_vbus_inlmt(struct axp288_chrg_info *info,
 	int ret;
 	u8 reg_val;
 
-	if (inlmt <= ILIM_100MA) {
-		reg_val = CHRG_VBUS_ILIM_100MA;
-	} else if (inlmt <= ILIM_500MA) {
-		reg_val = CHRG_VBUS_ILIM_500MA;
-	} else if (inlmt <= ILIM_900MA) {
-		reg_val = CHRG_VBUS_ILIM_900MA;
-	} else if (inlmt <= ILIM_1500MA) {
-		reg_val = CHRG_VBUS_ILIM_1500MA;
-	} else if (inlmt <= ILIM_2000MA) {
-		reg_val = CHRG_VBUS_ILIM_2000MA;
-	} else if (inlmt <= ILIM_2500MA) {
-		reg_val = CHRG_VBUS_ILIM_2500MA;
-	} else {
-		reg_val = CHRG_VBUS_ILIM_3000MA;
-	}
-
-	reg_val = reg_val << CHRG_VBUS_ILIM_BIT_POS;
+	if (inlmt >= 3000000)
+		reg_val = CHRG_VBUS_ILIM_3000MA << CHRG_VBUS_ILIM_BIT_POS;
+	else if (inlmt >= 2500000)
+		reg_val = CHRG_VBUS_ILIM_2500MA << CHRG_VBUS_ILIM_BIT_POS;
+	else if (inlmt >= 2000000)
+		reg_val = CHRG_VBUS_ILIM_2000MA << CHRG_VBUS_ILIM_BIT_POS;
+	else if (inlmt >= 1500000)
+		reg_val = CHRG_VBUS_ILIM_1500MA << CHRG_VBUS_ILIM_BIT_POS;
+	else if (inlmt >= 900000)
+		reg_val = CHRG_VBUS_ILIM_900MA << CHRG_VBUS_ILIM_BIT_POS;
+	else if (inlmt >= 500000)
+		reg_val = CHRG_VBUS_ILIM_500MA << CHRG_VBUS_ILIM_BIT_POS;
+	else
+		reg_val = CHRG_VBUS_ILIM_100MA << CHRG_VBUS_ILIM_BIT_POS;
 
 	ret = regmap_update_bits(info->regmap, AXP20X_CHRG_BAK_CTRL,
 				 CHRG_VBUS_ILIM_MASK, reg_val);
@@ -584,13 +566,13 @@ static void axp288_charger_extcon_evt_worker(struct work_struct *work)
 	/* Determine cable/charger type */
 	if (extcon_get_state(edev, EXTCON_CHG_USB_SDP) > 0) {
 		dev_dbg(&info->pdev->dev, "USB SDP charger is connected\n");
-		current_limit = ILIM_500MA;
+		current_limit = 500000;
 	} else if (extcon_get_state(edev, EXTCON_CHG_USB_CDP) > 0) {
 		dev_dbg(&info->pdev->dev, "USB CDP charger is connected\n");
-		current_limit = ILIM_1500MA;
+		current_limit = 1500000;
 	} else if (extcon_get_state(edev, EXTCON_CHG_USB_DCP) > 0) {
 		dev_dbg(&info->pdev->dev, "USB DCP charger is connected\n");
-		current_limit = ILIM_2000MA;
+		current_limit = 2000000;
 	} else {
 		/* Charger type detection still in progress, bail. */
 		return;
-- 
2.14.3

[PATCH 09/16] power: supply: axp288_charger: Use the right property for the input current limit

[Hans de Goede]

Use the right property for the input current limit and make it writable.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/power/supply/axp288_charger.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/power/supply/axp288_charger.c b/drivers/power/supply/axp288_charger.c
index fdca0a4a1c8e..8f4ac2d38b3f 100644
--- a/drivers/power/supply/axp288_charger.c
+++ b/drivers/power/supply/axp288_charger.c
@@ -378,6 +378,11 @@ static int axp288_charger_usb_set_property(struct power_supply *psy,
 		if (ret < 0)
 			dev_warn(&info->pdev->dev, "set charge voltage failed\n");
 		break;
+	case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT:
+		ret = axp288_charger_set_vbus_inlmt(info, val->intval);
+		if (ret < 0)
+			dev_warn(&info->pdev->dev, "set input current limit failed\n");
+		break;
 	default:
 		ret = -EINVAL;
 	}
@@ -430,7 +435,7 @@ static int axp288_charger_usb_get_property(struct power_supply *psy,
 	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE_MAX:
 		val->intval = info->max_cv * 1000;
 		break;
-	case POWER_SUPPLY_PROP_CHARGE_CONTROL_LIMIT:
+	case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT:
 		ret = axp288_charger_get_vbus_inlmt(info);
 		if (ret < 0)
 			return ret;
@@ -451,6 +456,7 @@ static int axp288_charger_property_is_writeable(struct power_supply *psy,
 	switch (psp) {
 	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT:
 	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE:
+	case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT:
 		ret = 1;
 		break;
 	default:
@@ -469,7 +475,7 @@ static enum power_supply_property axp288_usb_props[] = {
 	POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT_MAX,
 	POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE,
 	POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE_MAX,
-	POWER_SUPPLY_PROP_CHARGE_CONTROL_LIMIT,
+	POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT,
 };
 
 static const struct power_supply_desc axp288_charger_desc = {
-- 
2.14.3

[PATCH 10/16] power: supply: axp288_charger: Simplify extcon cable handling

[Hans de Goede]

Simplify extcon cable handling using the new
devm_extcon_register_notifier_all function to listen to all cables
in one go.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/power/supply/axp288_charger.c | 51 +++++++----------------------------
 1 file changed, 10 insertions(+), 41 deletions(-)

diff --git a/drivers/power/supply/axp288_charger.c b/drivers/power/supply/axp288_charger.c
index 8f4ac2d38b3f..4ec7b023c881 100644
--- a/drivers/power/supply/axp288_charger.c
+++ b/drivers/power/supply/axp288_charger.c
@@ -103,9 +103,6 @@
 #define USB_HOST_EXTCON_HID		"INT3496"
 #define USB_HOST_EXTCON_NAME		"INT3496:00"
 
-static const unsigned int cable_ids[] =
-	{ EXTCON_CHG_USB_SDP, EXTCON_CHG_USB_CDP, EXTCON_CHG_USB_DCP };
-
 enum {
 	VBUS_OV_IRQ = 0,
 	CHARGE_DONE_IRQ,
@@ -137,7 +134,7 @@ struct axp288_chrg_info {
 	/* SDP/CDP/DCP USB charging cable notifications */
 	struct {
 		struct extcon_dev *edev;
-		struct notifier_block nb[ARRAY_SIZE(cable_ids)];
+		struct notifier_block nb;
 		struct work_struct work;
 	} cable;
 
@@ -595,34 +592,11 @@ static void axp288_charger_extcon_evt_worker(struct work_struct *work)
 	power_supply_changed(info->psy_usb);
 }
 
-/*
- * We need 3 copies of this, because there is no way to find out for which
- * cable id we are being called from the passed in arguments; and we must
- * have a separate nb for each extcon_register_notifier call.
- */
-static int axp288_charger_handle_cable0_evt(struct notifier_block *nb,
-					    unsigned long event, void *param)
-{
-	struct axp288_chrg_info *info =
-		container_of(nb, struct axp288_chrg_info, cable.nb[0]);
-	schedule_work(&info->cable.work);
-	return NOTIFY_OK;
-}
-
-static int axp288_charger_handle_cable1_evt(struct notifier_block *nb,
-					    unsigned long event, void *param)
+static int axp288_charger_handle_cable_evt(struct notifier_block *nb,
+					   unsigned long event, void *param)
 {
 	struct axp288_chrg_info *info =
-		container_of(nb, struct axp288_chrg_info, cable.nb[1]);
-	schedule_work(&info->cable.work);
-	return NOTIFY_OK;
-}
-
-static int axp288_charger_handle_cable2_evt(struct notifier_block *nb,
-					    unsigned long event, void *param)
-{
-	struct axp288_chrg_info *info =
-		container_of(nb, struct axp288_chrg_info, cable.nb[2]);
+		container_of(nb, struct axp288_chrg_info, cable.nb);
 	schedule_work(&info->cable.work);
 	return NOTIFY_OK;
 }
@@ -800,17 +774,12 @@ static int axp288_charger_probe(struct platform_device *pdev)
 
 	/* Register for extcon notification */
 	INIT_WORK(&info->cable.work, axp288_charger_extcon_evt_worker);
-	info->cable.nb[0].notifier_call = axp288_charger_handle_cable0_evt;
-	info->cable.nb[1].notifier_call = axp288_charger_handle_cable1_evt;
-	info->cable.nb[2].notifier_call = axp288_charger_handle_cable2_evt;
-	for (i = 0; i < ARRAY_SIZE(cable_ids); i++) {
-		ret = devm_extcon_register_notifier(dev, info->cable.edev,
-					  cable_ids[i], &info->cable.nb[i]);
-		if (ret) {
-			dev_err(dev, "failed to register extcon notifier for %u: %d\n",
-				cable_ids[i], ret);
-			return ret;
-		}
+	info->cable.nb.notifier_call = axp288_charger_handle_cable_evt;
+	ret = devm_extcon_register_notifier_all(dev, info->cable.edev,
+						&info->cable.nb);
+	if (ret) {
+		dev_err(dev, "failed to register cable extcon notifier\n");
+		return ret;
 	}
 	schedule_work(&info->cable.work);
 
-- 
2.14.3

[PATCH 11/16] power: supply: axp288_charger: Properly stop work on probe-error / remove

[Hans de Goede]

Properly stop any work we may have queued on probe-errors / remove.

Rather then adding a remove driver callback for this, and goto style
error handling to probe, use a devm_action for this.

The devm_action gets registered before we register any of the extcon
notifiers which may queue the work, devm does cleanup in reverse order,
so this ensures that the notifiers are removed before we cancel the work.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/power/supply/axp288_charger.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/power/supply/axp288_charger.c b/drivers/power/supply/axp288_charger.c
index 4ec7b023c881..9bfbde15b07d 100644
--- a/drivers/power/supply/axp288_charger.c
+++ b/drivers/power/supply/axp288_charger.c
@@ -724,6 +724,14 @@ static int charger_init_hw_regs(struct axp288_chrg_info *info)
 	return 0;
 }
 
+static void axp288_charger_cancel_work(void *data)
+{
+	struct axp288_chrg_info *info = data;
+
+	cancel_work_sync(&info->otg.work);
+	cancel_work_sync(&info->cable.work);
+}
+
 static int axp288_charger_probe(struct platform_device *pdev)
 {
 	int ret, i, pirq;
@@ -772,6 +780,11 @@ static int axp288_charger_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	/* Cancel our work on cleanup, register this before the notifiers */
+	ret = devm_add_action(dev, axp288_charger_cancel_work, info);
+	if (ret)
+		return ret;
+
 	/* Register for extcon notification */
 	INIT_WORK(&info->cable.work, axp288_charger_extcon_evt_worker);
 	info->cable.nb.notifier_call = axp288_charger_handle_cable_evt;
-- 
2.14.3

[PATCH 12/16] power: supply: axp288_fuel_gauge: Get iio-channels once during boot

[Hans de Goede]

Get iio-channels once during boot, delaying the probe if the axp288_adc
drivers has not loaded yet, instead of getting them on demand each time
we need them.

This fixes the following errors in dmesg:

axp288_fuel_gauge axp288_fuel_gauge: ADC charge current read failed:-19

Which were caused by the ondemand iio-channel read code not finding the
channel when the axp288_adc driver had not loaded yet.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/power/supply/axp288_fuel_gauge.c | 110 +++++++++++++++++++------------
 1 file changed, 67 insertions(+), 43 deletions(-)

diff --git a/drivers/power/supply/axp288_fuel_gauge.c b/drivers/power/supply/axp288_fuel_gauge.c
index a8dcabc32721..eb60f75f00d7 100644
--- a/drivers/power/supply/axp288_fuel_gauge.c
+++ b/drivers/power/supply/axp288_fuel_gauge.c
@@ -1,6 +1,7 @@
 /*
  * axp288_fuel_gauge.c - Xpower AXP288 PMIC Fuel Gauge Driver
  *
+ * Copyright (C) 2016-2017 Hans de Goede <hdegoede@redhat.com>
  * Copyright (C) 2014 Intel Corporation
  *
  * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
@@ -100,11 +101,22 @@ enum {
 	WL1_IRQ,
 };
 
+enum {
+	BAT_TEMP = 0,
+	PMIC_TEMP,
+	SYSTEM_TEMP,
+	BAT_CHRG_CURR,
+	BAT_D_CURR,
+	BAT_VOLT,
+	IIO_CHANNEL_NUM
+};
+
 struct axp288_fg_info {
 	struct platform_device *pdev;
 	struct regmap *regmap;
 	struct regmap_irq_chip_data *regmap_irqc;
 	int irq[AXP288_FG_INTR_NUM];
+	struct iio_channel *iio_channel[IIO_CHANNEL_NUM];
 	struct power_supply *bat;
 	struct mutex lock;
 	int status;
@@ -199,33 +211,6 @@ static int fuel_gauge_read_12bit_word(struct axp288_fg_info *info, int reg)
 	return (buf[0] << 4) | ((buf[1] >> 4) & 0x0f);
 }
 
-static int pmic_read_adc_val(const char *name, int *raw_val,
-		struct axp288_fg_info *info)
-{
-	int ret, val = 0;
-	struct iio_channel *indio_chan;
-
-	indio_chan = iio_channel_get(NULL, name);
-	if (IS_ERR_OR_NULL(indio_chan)) {
-		ret = PTR_ERR(indio_chan);
-		goto exit;
-	}
-	ret = iio_read_channel_raw(indio_chan, &val);
-	if (ret < 0) {
-		dev_err(&info->pdev->dev,
-			"IIO channel read error: %x, %x\n", ret, val);
-		goto err_exit;
-	}
-
-	dev_dbg(&info->pdev->dev, "adc raw val=%x\n", val);
-	*raw_val = val;
-
-err_exit:
-	iio_channel_release(indio_chan);
-exit:
-	return ret;
-}
-
 #ifdef CONFIG_DEBUG_FS
 static int fuel_gauge_debug_show(struct seq_file *s, void *data)
 {
@@ -296,22 +281,22 @@ static int fuel_gauge_debug_show(struct seq_file *s, void *data)
 		AXP288_FG_TUNE5,
 		fuel_gauge_reg_readb(info, AXP288_FG_TUNE5));
 
-	ret = pmic_read_adc_val("axp288-batt-temp", &raw_val, info);
+	ret = iio_read_channel_raw(info->iio_channel[BAT_TEMP], &raw_val);
 	if (ret >= 0)
 		seq_printf(s, "axp288-batttemp : %d\n", raw_val);
-	ret = pmic_read_adc_val("axp288-pmic-temp", &raw_val, info);
+	ret = iio_read_channel_raw(info->iio_channel[PMIC_TEMP], &raw_val);
 	if (ret >= 0)
 		seq_printf(s, "axp288-pmictemp : %d\n", raw_val);
-	ret = pmic_read_adc_val("axp288-system-temp", &raw_val, info);
+	ret = iio_read_channel_raw(info->iio_channel[SYSTEM_TEMP], &raw_val);
 	if (ret >= 0)
 		seq_printf(s, "axp288-systtemp : %d\n", raw_val);
-	ret = pmic_read_adc_val("axp288-chrg-curr", &raw_val, info);
+	ret = iio_read_channel_raw(info->iio_channel[BAT_CHRG_CURR], &raw_val);
 	if (ret >= 0)
 		seq_printf(s, "axp288-chrgcurr : %d\n", raw_val);
-	ret = pmic_read_adc_val("axp288-chrg-d-curr", &raw_val, info);
+	ret = iio_read_channel_raw(info->iio_channel[BAT_D_CURR], &raw_val);
 	if (ret >= 0)
 		seq_printf(s, "axp288-dchrgcur : %d\n", raw_val);
-	ret = pmic_read_adc_val("axp288-batt-volt", &raw_val, info);
+	ret = iio_read_channel_raw(info->iio_channel[BAT_VOLT], &raw_val);
 	if (ret >= 0)
 		seq_printf(s, "axp288-battvolt : %d\n", raw_val);
 
@@ -360,13 +345,13 @@ static void fuel_gauge_get_status(struct axp288_fg_info *info)
 			"PWR STAT read failed:%d\n", pwr_stat);
 		return;
 	}
-	ret = pmic_read_adc_val("axp288-chrg-curr", &charge, info);
+	ret = iio_read_channel_raw(info->iio_channel[BAT_CHRG_CURR], &charge);
 	if (ret < 0) {
 		dev_err(&info->pdev->dev,
 			"ADC charge current read failed:%d\n", ret);
 		return;
 	}
-	ret = pmic_read_adc_val("axp288-chrg-d-curr", &discharge, info);
+	ret = iio_read_channel_raw(info->iio_channel[BAT_D_CURR], &discharge);
 	if (ret < 0) {
 		dev_err(&info->pdev->dev,
 			"ADC discharge current read failed:%d\n", ret);
@@ -389,7 +374,7 @@ static int fuel_gauge_get_vbatt(struct axp288_fg_info *info, int *vbatt)
 {
 	int ret = 0, raw_val;
 
-	ret = pmic_read_adc_val("axp288-batt-volt", &raw_val, info);
+	ret = iio_read_channel_raw(info->iio_channel[BAT_VOLT], &raw_val);
 	if (ret < 0)
 		goto vbatt_read_fail;
 
@@ -403,10 +388,10 @@ static int fuel_gauge_get_current(struct axp288_fg_info *info, int *cur)
 	int ret, value = 0;
 	int charge, discharge;
 
-	ret = pmic_read_adc_val("axp288-chrg-curr", &charge, info);
+	ret = iio_read_channel_raw(info->iio_channel[BAT_CHRG_CURR], &charge);
 	if (ret < 0)
 		goto current_read_fail;
-	ret = pmic_read_adc_val("axp288-chrg-d-curr", &discharge, info);
+	ret = iio_read_channel_raw(info->iio_channel[BAT_D_CURR], &discharge);
 	if (ret < 0)
 		goto current_read_fail;
 
@@ -700,10 +685,18 @@ static void fuel_gauge_init_irq(struct axp288_fg_info *info)
 
 static int axp288_fuel_gauge_probe(struct platform_device *pdev)
 {
-	int ret = 0;
+	int i, ret = 0;
 	struct axp288_fg_info *info;
 	struct axp20x_dev *axp20x = dev_get_drvdata(pdev->dev.parent);
 	struct power_supply_config psy_cfg = {};
+	static const char * const iio_chan_name[] = {
+		[BAT_TEMP] = "axp288-batt-temp",
+		[PMIC_TEMP] = "axp288-pmic-temp",
+		[SYSTEM_TEMP] = "axp288-system-temp",
+		[BAT_CHRG_CURR] = "axp288-chrg-curr",
+		[BAT_D_CURR] = "axp288-chrg-d-curr",
+		[BAT_VOLT] = "axp288-batt-volt",
+	};
 
 	info = devm_kzalloc(&pdev->dev, sizeof(*info), GFP_KERNEL);
 	if (!info)
@@ -719,18 +712,39 @@ static int axp288_fuel_gauge_probe(struct platform_device *pdev)
 	mutex_init(&info->lock);
 	INIT_DELAYED_WORK(&info->status_monitor, fuel_gauge_status_monitor);
 
+	for (i = 0; i < IIO_CHANNEL_NUM; i++) {
+		/*
+		 * Note cannot use devm_iio_channel_get because x86 systems
+		 * lack the device<->channel maps which iio_channel_get will
+		 * try to use when passed a non NULL device pointer.
+		 */
+		info->iio_channel[i] =
+			iio_channel_get(NULL, iio_chan_name[i]);
+		if (IS_ERR(info->iio_channel[i])) {
+			ret = PTR_ERR(info->iio_channel[i]);
+			dev_dbg(&pdev->dev, "error getting iiochan %s: %d\n",
+				iio_chan_name[i], ret);
+			/* Wait for axp288_adc to load */
+			if (ret == -ENODEV)
+				ret = -EPROBE_DEFER;
+
+			goto out_free_iio_chan;
+		}
+	}
+
 	ret = fuel_gauge_reg_readb(info, AXP288_FG_DES_CAP1_REG);
 	if (ret < 0)
-		return ret;
+		goto out_free_iio_chan;
 
 	if (!(ret & FG_DES_CAP1_VALID)) {
 		dev_err(&pdev->dev, "axp288 not configured by firmware\n");
-		return -ENODEV;
+		ret = -ENODEV;
+		goto out_free_iio_chan;
 	}
 
 	ret = fuel_gauge_reg_readb(info, AXP20X_CHRG_CTRL1);
 	if (ret < 0)
-		return ret;
+		goto out_free_iio_chan;
 	switch ((ret & CHRG_CCCV_CV_MASK) >> CHRG_CCCV_CV_BIT_POS) {
 	case CHRG_CCCV_CV_4100MV:
 		info->max_volt = 4100;
@@ -751,7 +765,7 @@ static int axp288_fuel_gauge_probe(struct platform_device *pdev)
 	if (IS_ERR(info->bat)) {
 		ret = PTR_ERR(info->bat);
 		dev_err(&pdev->dev, "failed to register battery: %d\n", ret);
-		return ret;
+		goto out_free_iio_chan;
 	}
 
 	fuel_gauge_create_debugfs(info);
@@ -759,6 +773,13 @@ static int axp288_fuel_gauge_probe(struct platform_device *pdev)
 	schedule_delayed_work(&info->status_monitor, STATUS_MON_DELAY_JIFFIES);
 
 	return 0;
+
+out_free_iio_chan:
+	for (i = 0; i < IIO_CHANNEL_NUM; i++)
+		if (!IS_ERR_OR_NULL(info->iio_channel[i]))
+			iio_channel_release(info->iio_channel[i]);
+
+	return ret;
 }
 
 static const struct platform_device_id axp288_fg_id_table[] = {
@@ -780,6 +801,9 @@ static int axp288_fuel_gauge_remove(struct platform_device *pdev)
 		if (info->irq[i] >= 0)
 			free_irq(info->irq[i], info);
 
+	for (i = 0; i < IIO_CHANNEL_NUM; i++)
+		iio_channel_release(info->iio_channel[i]);
+
 	return 0;
 }
 
-- 
2.14.3

[PATCH 13/16] power: supply: axp288_fuel_gauge: Cache charge_full value

[Hans de Goede]

The charge_full value never changes on the axp288 (there is no
charge_full_design), so we can read it once and cache it.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/power/supply/axp288_fuel_gauge.c | 21 +++++++++------------
 1 file changed, 9 insertions(+), 12 deletions(-)

diff --git a/drivers/power/supply/axp288_fuel_gauge.c b/drivers/power/supply/axp288_fuel_gauge.c
index eb60f75f00d7..800abecef45c 100644
--- a/drivers/power/supply/axp288_fuel_gauge.c
+++ b/drivers/power/supply/axp288_fuel_gauge.c
@@ -120,6 +120,7 @@ struct axp288_fg_info {
 	struct power_supply *bat;
 	struct mutex lock;
 	int status;
+	int charge_full_raw;
 	int max_volt;
 	struct delayed_work status_monitor;
 	struct dentry *debug_file;
@@ -504,11 +505,7 @@ static int fuel_gauge_get_property(struct power_supply *ps,
 		val->intval = ret * FG_DES_CAP_RES_LSB;
 		break;
 	case POWER_SUPPLY_PROP_CHARGE_FULL:
-		ret = fuel_gauge_read_15bit_word(info, AXP288_FG_DES_CAP1_REG);
-		if (ret < 0)
-			goto fuel_gauge_read_err;
-
-		val->intval = ret * FG_DES_CAP_RES_LSB;
+		val->intval = info->charge_full_raw * FG_DES_CAP_RES_LSB;
 		break;
 	case POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN:
 		val->intval = PROP_VOLT(info->max_volt);
@@ -732,15 +729,15 @@ static int axp288_fuel_gauge_probe(struct platform_device *pdev)
 		}
 	}
 
-	ret = fuel_gauge_reg_readb(info, AXP288_FG_DES_CAP1_REG);
-	if (ret < 0)
-		goto out_free_iio_chan;
-
-	if (!(ret & FG_DES_CAP1_VALID)) {
-		dev_err(&pdev->dev, "axp288 not configured by firmware\n");
-		ret = -ENODEV;
+	ret = fuel_gauge_read_15bit_word(info, AXP288_FG_DES_CAP1_REG);
+	if (ret < 0) {
+		if (ret == -ENXIO) {
+			dev_err(&pdev->dev, "axp288 not configured by firmware\n");
+			ret = -ENODEV;
+		}
 		goto out_free_iio_chan;
 	}
+	info->charge_full_raw = ret;
 
 	ret = fuel_gauge_reg_readb(info, AXP20X_CHRG_CTRL1);
 	if (ret < 0)
-- 
2.14.3

[PATCH 14/16] power: supply: axp288_fuel_gauge: Remove bogus status reg check

[Hans de Goede]

Remove a bogus status reg check from fuel_gauge_get_status, the
register being read is the pwr-status reg, but the mask being used is
for the chgr-status reg.

The wrong mask checks for vbus-present, not valid but present, we could
change the check to use a proper mask definition for the vbus-valid mask,
but even then it makes no sense, if we don't have a valid vbus then we
must be running from battery and the discharge current thus must be non 0,
so we will then never reach the pwr_stat check.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/power/supply/axp288_fuel_gauge.c | 17 +++--------------
 1 file changed, 3 insertions(+), 14 deletions(-)

diff --git a/drivers/power/supply/axp288_fuel_gauge.c b/drivers/power/supply/axp288_fuel_gauge.c
index 800abecef45c..2fc144b1ec0a 100644
--- a/drivers/power/supply/axp288_fuel_gauge.c
+++ b/drivers/power/supply/axp288_fuel_gauge.c
@@ -337,15 +337,8 @@ static inline void fuel_gauge_remove_debugfs(struct axp288_fg_info *info)
 
 static void fuel_gauge_get_status(struct axp288_fg_info *info)
 {
-	int pwr_stat, ret;
-	int charge, discharge;
+	int ret, charge, discharge;
 
-	pwr_stat = fuel_gauge_reg_readb(info, AXP20X_PWR_INPUT_STATUS);
-	if (pwr_stat < 0) {
-		dev_err(&info->pdev->dev,
-			"PWR STAT read failed:%d\n", pwr_stat);
-		return;
-	}
 	ret = iio_read_channel_raw(info->iio_channel[BAT_CHRG_CURR], &charge);
 	if (ret < 0) {
 		dev_err(&info->pdev->dev,
@@ -363,12 +356,8 @@ static void fuel_gauge_get_status(struct axp288_fg_info *info)
 		info->status = POWER_SUPPLY_STATUS_CHARGING;
 	else if (discharge > 0)
 		info->status = POWER_SUPPLY_STATUS_DISCHARGING;
-	else {
-		if (pwr_stat & CHRG_STAT_BAT_PRESENT)
-			info->status = POWER_SUPPLY_STATUS_FULL;
-		else
-			info->status = POWER_SUPPLY_STATUS_NOT_CHARGING;
-	}
+	else
+		info->status = POWER_SUPPLY_STATUS_FULL;
 }
 
 static int fuel_gauge_get_vbatt(struct axp288_fg_info *info, int *vbatt)
-- 
2.14.3

[PATCH 15/16] power: supply: axp288_fuel_gauge: Fix FULL status reporting

[Hans de Goede]

Relying on the (dis)charge current reporting for reporting FULL back to
userspace does not work really well and often leads to the reported status
getting stuck at e.g. 98/99% (the fuelgauge is not perfect) for hours.

What happens is that when the battery is full the axp288 keeps charging it
with a very low current. Until it is really really full and once really
really full, some inaccuracies in the adc lead to it then sometimes
reporting a small discharging rate, even though an external pwr source is
used. So we end up with a status of "charging" for hours after the battery
is actually already full and sometimes this then flip-flops to discharging.

This commit fixes this by using the fuel-gauge to check if the battery is
approximately full and if it is then discount a (dis)charge current smaller
then 64mA.

If we encounter a 0 (dis)charge current when not full, then report this
as "not charging".

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/power/supply/axp288_fuel_gauge.c | 33 ++++++++++++++++++++++++++++----
 1 file changed, 29 insertions(+), 4 deletions(-)

diff --git a/drivers/power/supply/axp288_fuel_gauge.c b/drivers/power/supply/axp288_fuel_gauge.c
index 2fc144b1ec0a..5750b6caecd4 100644
--- a/drivers/power/supply/axp288_fuel_gauge.c
+++ b/drivers/power/supply/axp288_fuel_gauge.c
@@ -337,7 +337,8 @@ static inline void fuel_gauge_remove_debugfs(struct axp288_fg_info *info)
 
 static void fuel_gauge_get_status(struct axp288_fg_info *info)
 {
-	int ret, charge, discharge;
+	int ret, charge, discharge, charge_now_raw, threshold;
+	bool full;
 
 	ret = iio_read_channel_raw(info->iio_channel[BAT_CHRG_CURR], &charge);
 	if (ret < 0) {
@@ -352,12 +353,36 @@ static void fuel_gauge_get_status(struct axp288_fg_info *info)
 		return;
 	}
 
-	if (charge > 0)
+	ret = fuel_gauge_read_15bit_word(info, AXP288_FG_CC_MTR1_REG);
+	if (ret < 0) {
+		dev_err(&info->pdev->dev,
+			"ADC charge current read failed:%d\n", ret);
+		return;
+	}
+	charge_now_raw = ret;
+
+	/*
+	 * When the battery is full the axp288 keeps charging it with a
+	 * very low current. Until it is really really full and once really
+	 * really full, some inaccuracies in the adc lead to it then sometimes
+	 * reporting a small discharging rate, even though an external pwr
+	 * source is used.
+	 *
+	 * So when we the battery is (nearly) full we treat low (dis)charge
+	 * currents as 0.
+	 */
+
+	full = DIV_ROUND_UP(charge_now_raw * 100, info->charge_full_raw) >= 98;
+	threshold = full ? 64 : 0; /* mA */
+
+	if (charge > threshold)
 		info->status = POWER_SUPPLY_STATUS_CHARGING;
-	else if (discharge > 0)
+	else if (discharge > threshold)
 		info->status = POWER_SUPPLY_STATUS_DISCHARGING;
-	else
+	else if (full)
 		info->status = POWER_SUPPLY_STATUS_FULL;
+	else
+		info->status = POWER_SUPPLY_STATUS_NOT_CHARGING;
 }
 
 static int fuel_gauge_get_vbatt(struct axp288_fg_info *info, int *vbatt)
-- 
2.14.3

[PATCH 16/16] power: supply: axp288_fuel_gauge: Do not register our psy on (some) HDMI sticks

[Hans de Goede]

The Intel Compute Stick (Cherry Trail version) and the Meegopad T08 HDMI
stick, both use an axp288 PMIC.  They also both have this wired up in such
a way that the detection logic in the PMIC claims that a valid battery is
present, resuling in GNOME and KDE showing a full-battery in their status
bar and power-settings, while these devices do not have a battery.

For lack of a better fix add a DMI blacklist and do not register the
axp288_fuel_gauge psy on devices on the blacklist.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/power/supply/axp288_fuel_gauge.c | 35 ++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/drivers/power/supply/axp288_fuel_gauge.c b/drivers/power/supply/axp288_fuel_gauge.c
index 5750b6caecd4..94f93cd757ac 100644
--- a/drivers/power/supply/axp288_fuel_gauge.c
+++ b/drivers/power/supply/axp288_fuel_gauge.c
@@ -17,6 +17,7 @@
  *
  */
 
+#include <linux/dmi.h>
 #include <linux/module.h>
 #include <linux/kernel.h>
 #include <linux/device.h>
@@ -694,6 +695,37 @@ static void fuel_gauge_init_irq(struct axp288_fg_info *info)
 	}
 }
 
+/*
+ * Some devices have no battery (HDMI sticks) and the axp288 battery's
+ * detection reports one despite it not being there.
+ */
+static const struct dmi_system_id axp288_fuel_gauge_blacklist[] = {
+	{
+		/* Intel Cherry Trail Compute Stick, Windows version */
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Intel Corporation"),
+			DMI_MATCH(DMI_PRODUCT_NAME, "STK1AW32SC"),
+		},
+	},
+	{
+		/* Intel Cherry Trail Compute Stick, version without an OS */
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Intel Corporation"),
+			DMI_MATCH(DMI_PRODUCT_NAME, "STK1A32SC"),
+		},
+	},
+	{
+		/* Meegopad T08 */
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Default string"),
+			DMI_MATCH(DMI_BOARD_VENDOR, "To be filled by OEM."),
+			DMI_MATCH(DMI_BOARD_NAME, "T3 MRD"),
+			DMI_MATCH(DMI_BOARD_VERSION, "V1.1"),
+		},
+	},
+	{}
+};
+
 static int axp288_fuel_gauge_probe(struct platform_device *pdev)
 {
 	int i, ret = 0;
@@ -709,6 +741,9 @@ static int axp288_fuel_gauge_probe(struct platform_device *pdev)
 		[BAT_VOLT] = "axp288-batt-volt",
 	};
 
+	if (dmi_check_system(axp288_fuel_gauge_blacklist))
+		return -ENODEV;
+
 	info = devm_kzalloc(&pdev->dev, sizeof(*info), GFP_KERNEL);
 	if (!info)
 		return -ENOMEM;
-- 
2.14.3

Re: [PATCH 13/16] power: supply: axp288_fuel_gauge: Cache charge_full value

[Chen-Yu Tsai]

Hi,

On Fri, Dec 22, 2017 at 8:37 PM, Hans de Goede <hdegoede@redhat.com> wrote:
> The charge_full value never changes on the axp288 (there is no
> charge_full_design), so we can read it once and cache it.

The manual says on page 57:

    Additionally, the Fuel Gauge System is capable to learn the battery
    characteristic on each Full charge cycle.  Information such as Battery
    Maximum capacity (REG E0H, REG E1H) and Rdc (REG BAH, REG BBH) will be
    updated automatically over time.

Tablets might spend more time in suspend than actually being powered off,
meaning over time the value the kernel knows might be stale. But it looks
like the driver doesn't support suspend. I'd advise against this patch, or
at least leave some todo note to add a read-out call in the resume callback
(when its implemented).

ChenYu

> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/power/supply/axp288_fuel_gauge.c | 21 +++++++++------------
>  1 file changed, 9 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/power/supply/axp288_fuel_gauge.c b/drivers/power/supply/axp288_fuel_gauge.c
> index eb60f75f00d7..800abecef45c 100644
> --- a/drivers/power/supply/axp288_fuel_gauge.c
> +++ b/drivers/power/supply/axp288_fuel_gauge.c
> @@ -120,6 +120,7 @@ struct axp288_fg_info {
>         struct power_supply *bat;
>         struct mutex lock;
>         int status;
> +       int charge_full_raw;
>         int max_volt;
>         struct delayed_work status_monitor;
>         struct dentry *debug_file;
> @@ -504,11 +505,7 @@ static int fuel_gauge_get_property(struct power_supply *ps,
>                 val->intval = ret * FG_DES_CAP_RES_LSB;
>                 break;
>         case POWER_SUPPLY_PROP_CHARGE_FULL:
> -               ret = fuel_gauge_read_15bit_word(info, AXP288_FG_DES_CAP1_REG);
> -               if (ret < 0)
> -                       goto fuel_gauge_read_err;
> -
> -               val->intval = ret * FG_DES_CAP_RES_LSB;
> +               val->intval = info->charge_full_raw * FG_DES_CAP_RES_LSB;
>                 break;
>         case POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN:
>                 val->intval = PROP_VOLT(info->max_volt);
> @@ -732,15 +729,15 @@ static int axp288_fuel_gauge_probe(struct platform_device *pdev)
>                 }
>         }
>
> -       ret = fuel_gauge_reg_readb(info, AXP288_FG_DES_CAP1_REG);
> -       if (ret < 0)
> -               goto out_free_iio_chan;
> -
> -       if (!(ret & FG_DES_CAP1_VALID)) {
> -               dev_err(&pdev->dev, "axp288 not configured by firmware\n");
> -               ret = -ENODEV;
> +       ret = fuel_gauge_read_15bit_word(info, AXP288_FG_DES_CAP1_REG);
> +       if (ret < 0) {
> +               if (ret == -ENXIO) {
> +                       dev_err(&pdev->dev, "axp288 not configured by firmware\n");
> +                       ret = -ENODEV;
> +               }
>                 goto out_free_iio_chan;
>         }
> +       info->charge_full_raw = ret;
>
>         ret = fuel_gauge_reg_readb(info, AXP20X_CHRG_CTRL1);
>         if (ret < 0)
> --
> 2.14.3
>

Re: [PATCH 15/16] power: supply: axp288_fuel_gauge: Fix FULL status reporting

[Chen-Yu Tsai]

On Fri, Dec 22, 2017 at 8:37 PM, Hans de Goede <hdegoede@redhat.com> wrote:
> Relying on the (dis)charge current reporting for reporting FULL back to
> userspace does not work really well and often leads to the reported status
> getting stuck at e.g. 98/99% (the fuelgauge is not perfect) for hours.
>
> What happens is that when the battery is full the axp288 keeps charging it
> with a very low current. Until it is really really full and once really
> really full, some inaccuracies in the adc lead to it then sometimes
> reporting a small discharging rate, even though an external pwr source is
> used. So we end up with a status of "charging" for hours after the battery
> is actually already full and sometimes this then flip-flops to discharging.
>
> This commit fixes this by using the fuel-gauge to check if the battery is
> approximately full and if it is then discount a (dis)charge current smaller
> then 64mA.
>
> If we encounter a 0 (dis)charge current when not full, then report this
> as "not charging".

The trickle charge mode is called "safe mode" in the manual, which charges
the battery at 5mA until the battery is within 0.1V of the target voltage.
This mode can be checked by looking at REG 01h bit 3. See section 9.4.3
on page 50 for more information.

This might be better than the heuristic you're using. Also

> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/power/supply/axp288_fuel_gauge.c | 33 ++++++++++++++++++++++++++++----
>  1 file changed, 29 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/power/supply/axp288_fuel_gauge.c b/drivers/power/supply/axp288_fuel_gauge.c
> index 2fc144b1ec0a..5750b6caecd4 100644
> --- a/drivers/power/supply/axp288_fuel_gauge.c
> +++ b/drivers/power/supply/axp288_fuel_gauge.c
> @@ -337,7 +337,8 @@ static inline void fuel_gauge_remove_debugfs(struct axp288_fg_info *info)
>
>  static void fuel_gauge_get_status(struct axp288_fg_info *info)
>  {
> -       int ret, charge, discharge;
> +       int ret, charge, discharge, charge_now_raw, threshold;
> +       bool full;
>
>         ret = iio_read_channel_raw(info->iio_channel[BAT_CHRG_CURR], &charge);
>         if (ret < 0) {
> @@ -352,12 +353,36 @@ static void fuel_gauge_get_status(struct axp288_fg_info *info)
>                 return;
>         }
>
> -       if (charge > 0)
> +       ret = fuel_gauge_read_15bit_word(info, AXP288_FG_CC_MTR1_REG);
> +       if (ret < 0) {
> +               dev_err(&info->pdev->dev,
> +                       "ADC charge current read failed:%d\n", ret);
> +               return;
> +       }
> +       charge_now_raw = ret;
> +
> +       /*
> +        * When the battery is full the axp288 keeps charging it with a
> +        * very low current. Until it is really really full and once really
> +        * really full, some inaccuracies in the adc lead to it then sometimes
> +        * reporting a small discharging rate, even though an external pwr
> +        * source is used.
> +        *
> +        * So when we the battery is (nearly) full we treat low (dis)charge
> +        * currents as 0.
> +        */
> +
> +       full = DIV_ROUND_UP(charge_now_raw * 100, info->charge_full_raw) >= 98;

The contents of REG E5h contains what you are calculating here (charge
percentage).
You could just use that. I'm not sure what the relationship between REG B9h, E4H
and E5h is. B9h (overall capacity percentage) is another choice.

ChenYu

> +       threshold = full ? 64 : 0; /* mA */
> +
> +       if (charge > threshold)
>                 info->status = POWER_SUPPLY_STATUS_CHARGING;
> -       else if (discharge > 0)
> +       else if (discharge > threshold)
>                 info->status = POWER_SUPPLY_STATUS_DISCHARGING;
> -       else
> +       else if (full)
>                 info->status = POWER_SUPPLY_STATUS_FULL;
> +       else
> +               info->status = POWER_SUPPLY_STATUS_NOT_CHARGING;
>  }
>
>  static int fuel_gauge_get_vbatt(struct axp288_fg_info *info, int *vbatt)
> --
> 2.14.3
>

Re: [PATCH 01/16] power: supply: axp288_charger: Do not stop + restart charging at boot

[Chen-Yu Tsai]

On Fri, Dec 22, 2017 at 8:37 PM, Hans de Goede <hdegoede@redhat.com> wrote:
> Before this commit we were relying solely on the extcon interface for
> cable detection, including to determine if a cable providing vbus is
> connected at all. This caused us to turn off charging at boot, because
> when we run the initial state processing the axp288-extcon driver is still
> running charger-type detection most of the time, so all charger cable
> types read as disconnected when we run the initial state processing.
>
> This commit reworks the axp288_charger_extcon_evt_worker flow to use the
> VBUS_VALID bit from the PWR_INPUT_STATUS register to determine if we
> should turn charging on/off. Note this is the same bit as we use for the
> online property.
>
> If VBUS_VALID is set, but the extcon code has not completed the charger
> type detection yet, we now simply bail leaving things as configured by
> the BIOS (we will get a notifier call when the extcon code is done and
> reschedule the axp288_charger_extcon_evt_worker).
>
> The extcon code is the only one to trigger the worker (outside of the
> initial run) and we can rely on it to only call us if things have changed,
> so while we are completely refactoring axp288_charger_extcon_evt_worker,
> also remove the code to check if the state has changed.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Other than the two patches I replied to, this looks good overall.
For the remaining patches,

Reviewed-by: Chen-Yu Tsai <wens@csie.org>

Hans, I don't have any devices with this PMIC, so I'm unlikely to spend
much time on the axp288 specific drivers. Any chance you want to maintain
them?

Regards
ChenYu

Re: [PATCH 13/16] power: supply: axp288_fuel_gauge: Cache charge_full value

[Hans de Goede]

Hi,

On 26-12-17 08:04, Chen-Yu Tsai wrote:
> Hi,
> 
> On Fri, Dec 22, 2017 at 8:37 PM, Hans de Goede <hdegoede@redhat.com> wrote:
>> The charge_full value never changes on the axp288 (there is no
>> charge_full_design), so we can read it once and cache it.
> 
> The manual says on page 57:
> 
>      Additionally, the Fuel Gauge System is capable to learn the battery
>      characteristic on each Full charge cycle.  Information such as Battery
>      Maximum capacity (REG E0H, REG E1H) and Rdc (REG BAH, REG BBH) will be
>      updated automatically over time.
> 
> Tablets might spend more time in suspend than actually being powered off,
> meaning over time the value the kernel knows might be stale. But it looks
> like the driver doesn't support suspend. I'd advise against this patch, or
> at least leave some todo note to add a read-out call in the resume callback
> (when its implemented).

Ok, I will drop this patch from v2 of the patchset.

Regards,

Hans


> 
> ChenYu
> 
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>   drivers/power/supply/axp288_fuel_gauge.c | 21 +++++++++------------
>>   1 file changed, 9 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/power/supply/axp288_fuel_gauge.c b/drivers/power/supply/axp288_fuel_gauge.c
>> index eb60f75f00d7..800abecef45c 100644
>> --- a/drivers/power/supply/axp288_fuel_gauge.c
>> +++ b/drivers/power/supply/axp288_fuel_gauge.c
>> @@ -120,6 +120,7 @@ struct axp288_fg_info {
>>          struct power_supply *bat;
>>          struct mutex lock;
>>          int status;
>> +       int charge_full_raw;
>>          int max_volt;
>>          struct delayed_work status_monitor;
>>          struct dentry *debug_file;
>> @@ -504,11 +505,7 @@ static int fuel_gauge_get_property(struct power_supply *ps,
>>                  val->intval = ret * FG_DES_CAP_RES_LSB;
>>                  break;
>>          case POWER_SUPPLY_PROP_CHARGE_FULL:
>> -               ret = fuel_gauge_read_15bit_word(info, AXP288_FG_DES_CAP1_REG);
>> -               if (ret < 0)
>> -                       goto fuel_gauge_read_err;
>> -
>> -               val->intval = ret * FG_DES_CAP_RES_LSB;
>> +               val->intval = info->charge_full_raw * FG_DES_CAP_RES_LSB;
>>                  break;
>>          case POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN:
>>                  val->intval = PROP_VOLT(info->max_volt);
>> @@ -732,15 +729,15 @@ static int axp288_fuel_gauge_probe(struct platform_device *pdev)
>>                  }
>>          }
>>
>> -       ret = fuel_gauge_reg_readb(info, AXP288_FG_DES_CAP1_REG);
>> -       if (ret < 0)
>> -               goto out_free_iio_chan;
>> -
>> -       if (!(ret & FG_DES_CAP1_VALID)) {
>> -               dev_err(&pdev->dev, "axp288 not configured by firmware\n");
>> -               ret = -ENODEV;
>> +       ret = fuel_gauge_read_15bit_word(info, AXP288_FG_DES_CAP1_REG);
>> +       if (ret < 0) {
>> +               if (ret == -ENXIO) {
>> +                       dev_err(&pdev->dev, "axp288 not configured by firmware\n");
>> +                       ret = -ENODEV;
>> +               }
>>                  goto out_free_iio_chan;
>>          }
>> +       info->charge_full_raw = ret;
>>
>>          ret = fuel_gauge_reg_readb(info, AXP20X_CHRG_CTRL1);
>>          if (ret < 0)
>> --
>> 2.14.3
>>

Re: [PATCH 15/16] power: supply: axp288_fuel_gauge: Fix FULL status reporting

[Hans de Goede]

Hi,

On 26-12-17 08:26, Chen-Yu Tsai wrote:
> On Fri, Dec 22, 2017 at 8:37 PM, Hans de Goede <hdegoede@redhat.com> wrote:
>> Relying on the (dis)charge current reporting for reporting FULL back to
>> userspace does not work really well and often leads to the reported status
>> getting stuck at e.g. 98/99% (the fuelgauge is not perfect) for hours.
>>
>> What happens is that when the battery is full the axp288 keeps charging it
>> with a very low current. Until it is really really full and once really
>> really full, some inaccuracies in the adc lead to it then sometimes
>> reporting a small discharging rate, even though an external pwr source is
>> used. So we end up with a status of "charging" for hours after the battery
>> is actually already full and sometimes this then flip-flops to discharging.
>>
>> This commit fixes this by using the fuel-gauge to check if the battery is
>> approximately full and if it is then discount a (dis)charge current smaller
>> then 64mA.
>>
>> If we encounter a 0 (dis)charge current when not full, then report this
>> as "not charging".
> 
> The trickle charge mode is called "safe mode" in the manual, which charges
> the battery at 5mA until the battery is within 0.1V of the target voltage.
> This mode can be checked by looking at REG 01h bit 3. See section 9.4.3
> on page 50 for more information.

No safe mode is some something different, there is a timer for determining
the max fast charging time and if that expires then safe mode is entered.

Section 9.4.3 also says: "When the actual charge current is less than 20%
of the ICHRG, the timer will automatically hold." IOW during a normal
charge cycle the (actual charge-current) will drop to below 20% of the max
charge current before the safety timer expires and then the safety timer
stops running, so it will never expire.

Note that the "fast charge max time" is between 6 and 12 hours, so quite
long, so we should really never see the device entering safemode.

Reg 1 also has a "Charging Indication" bit but that is not really helpful
either, it turns off at around 90% or so and even then it just not turns
off, it bounces for a bit while it is around the threshold it uses.

> This might be better than the heuristic you're using. Also

See above, I'm afraid we really need to do our own heuristic, the reg
01 status bits are not usable.

>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>   drivers/power/supply/axp288_fuel_gauge.c | 33 ++++++++++++++++++++++++++++----
>>   1 file changed, 29 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/power/supply/axp288_fuel_gauge.c b/drivers/power/supply/axp288_fuel_gauge.c
>> index 2fc144b1ec0a..5750b6caecd4 100644
>> --- a/drivers/power/supply/axp288_fuel_gauge.c
>> +++ b/drivers/power/supply/axp288_fuel_gauge.c
>> @@ -337,7 +337,8 @@ static inline void fuel_gauge_remove_debugfs(struct axp288_fg_info *info)
>>
>>   static void fuel_gauge_get_status(struct axp288_fg_info *info)
>>   {
>> -       int ret, charge, discharge;
>> +       int ret, charge, discharge, charge_now_raw, threshold;
>> +       bool full;
>>
>>          ret = iio_read_channel_raw(info->iio_channel[BAT_CHRG_CURR], &charge);
>>          if (ret < 0) {
>> @@ -352,12 +353,36 @@ static void fuel_gauge_get_status(struct axp288_fg_info *info)
>>                  return;
>>          }
>>
>> -       if (charge > 0)
>> +       ret = fuel_gauge_read_15bit_word(info, AXP288_FG_CC_MTR1_REG);
>> +       if (ret < 0) {
>> +               dev_err(&info->pdev->dev,
>> +                       "ADC charge current read failed:%d\n", ret);
>> +               return;
>> +       }
>> +       charge_now_raw = ret;
>> +
>> +       /*
>> +        * When the battery is full the axp288 keeps charging it with a
>> +        * very low current. Until it is really really full and once really
>> +        * really full, some inaccuracies in the adc lead to it then sometimes
>> +        * reporting a small discharging rate, even though an external pwr
>> +        * source is used.
>> +        *
>> +        * So when we the battery is (nearly) full we treat low (dis)charge
>> +        * currents as 0.
>> +        */
>> +
>> +       full = DIV_ROUND_UP(charge_now_raw * 100, info->charge_full_raw) >= 98;
> 
> The contents of REG E5h contains what you are calculating here (charge
> percentage).
> You could just use that. I'm not sure what the relationship between REG B9h, E4H
> and E5h is. B9h (overall capacity percentage) is another choice.

Interesting I will run some tests with these, from the datasheet it seems that
E5 is just the coulumbmeter percentage, without the usual correction for the
OCV of the cell, so this will drift over time, I think B9"Battery capacity percentage for indication" is the one which we want as that
seems to be the final estimated charge percentage value.

Anyways thank you for the feedback, using one of the percentage registers
seems like a good idea. I will run some tests with this and then post a new
version.

Regards,

Hans



> ChenYu
> 
>> +       threshold = full ? 64 : 0; /* mA */
>> +
>> +       if (charge > threshold)
>>                  info->status = POWER_SUPPLY_STATUS_CHARGING;
>> -       else if (discharge > 0)
>> +       else if (discharge > threshold)
>>                  info->status = POWER_SUPPLY_STATUS_DISCHARGING;
>> -       else
>> +       else if (full)
>>                  info->status = POWER_SUPPLY_STATUS_FULL;
>> +       else
>> +               info->status = POWER_SUPPLY_STATUS_NOT_CHARGING;
>>   }
>>
>>   static int fuel_gauge_get_vbatt(struct axp288_fg_info *info, int *vbatt)
>> --
>> 2.14.3
>>

Re: [PATCH 01/16] power: supply: axp288_charger: Do not stop + restart charging at boot

[Hans de Goede]

Hi,

On 26-12-17 08:29, Chen-Yu Tsai wrote:
> On Fri, Dec 22, 2017 at 8:37 PM, Hans de Goede <hdegoede@redhat.com> wrote:
>> Before this commit we were relying solely on the extcon interface for
>> cable detection, including to determine if a cable providing vbus is
>> connected at all. This caused us to turn off charging at boot, because
>> when we run the initial state processing the axp288-extcon driver is still
>> running charger-type detection most of the time, so all charger cable
>> types read as disconnected when we run the initial state processing.
>>
>> This commit reworks the axp288_charger_extcon_evt_worker flow to use the
>> VBUS_VALID bit from the PWR_INPUT_STATUS register to determine if we
>> should turn charging on/off. Note this is the same bit as we use for the
>> online property.
>>
>> If VBUS_VALID is set, but the extcon code has not completed the charger
>> type detection yet, we now simply bail leaving things as configured by
>> the BIOS (we will get a notifier call when the extcon code is done and
>> reschedule the axp288_charger_extcon_evt_worker).
>>
>> The extcon code is the only one to trigger the worker (outside of the
>> initial run) and we can rely on it to only call us if things have changed,
>> so while we are completely refactoring axp288_charger_extcon_evt_worker,
>> also remove the code to check if the state has changed.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> 
> Other than the two patches I replied to, this looks good overall.
> For the remaining patches,
> 
> Reviewed-by: Chen-Yu Tsai <wens@csie.org>
> 
> Hans, I don't have any devices with this PMIC, so I'm unlikely to spend
> much time on the axp288 specific drivers. Any chance you want to maintain
> them?

Yes that probably is a good idea, I will add a patch to v2 of the series
adding a MAINTAINERS entry for this.

Regards,

Hans