[PATCH v2 00/12] power: supply: axp288_charger: Various fixes

[PATCH v2 00/12] power: supply: axp288_charger: Various fixes

[Hans de Goede]

Hi All,

Here is v2 of my axp288_charger series, which fixup the axp288_charger
code to actually work.

This implements the suggestion by Chanwoo Choi on how we can use the
existing extcon_get_extcon_dev instead of introducing a new
extcon_get_extcon_dev_by_id. So this series no longer depends on any
extcon sub-sys changes.

I've also rebased it on top of linux-power-supply/for-next-next to avoid
some conflicts with patches already there which Chanwoo pointed out,
so assuming review goes well merging these to for-next-next should go
smoothly :)

Regards,

Hans

[PATCH v2 01/12] power: supply: axp288_charger: Make charger_init_hw_regs propagate i2c errors

[Hans de Goede]

Make charger_init_hw_regs propagate i2c errors, instead of only warning
about them and then ignoring them.

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

diff --git a/drivers/power/supply/axp288_charger.c b/drivers/power/supply/axp288_charger.c
index 1115052..c0f5a49 100644
--- a/drivers/power/supply/axp288_charger.c
+++ b/drivers/power/supply/axp288_charger.c
@@ -701,59 +701,73 @@ static int axp288_charger_handle_otg_evt(struct notifier_block *nb,
 	return NOTIFY_OK;
 }
 
-static void charger_init_hw_regs(struct axp288_chrg_info *info)
+static int charger_init_hw_regs(struct axp288_chrg_info *info)
 {
 	int ret, cc, cv;
 	unsigned int val;
 
 	/* Program temperature thresholds */
 	ret = regmap_write(info->regmap, AXP20X_V_LTF_CHRG, CHRG_VLTFC_0C);
-	if (ret < 0)
-		dev_warn(&info->pdev->dev, "register(%x) write error(%d)\n",
+	if (ret < 0) {
+		dev_err(&info->pdev->dev, "register(%x) write error(%d)\n",
 							AXP20X_V_LTF_CHRG, ret);
+		return ret;
+	}
 
 	ret = regmap_write(info->regmap, AXP20X_V_HTF_CHRG, CHRG_VHTFC_45C);
-	if (ret < 0)
-		dev_warn(&info->pdev->dev, "register(%x) write error(%d)\n",
+	if (ret < 0) {
+		dev_err(&info->pdev->dev, "register(%x) write error(%d)\n",
 							AXP20X_V_HTF_CHRG, ret);
+		return ret;
+	}
 
 	/* Do not turn-off charger o/p after charge cycle ends */
 	ret = regmap_update_bits(info->regmap,
 				AXP20X_CHRG_CTRL2,
 				CNTL2_CHG_OUT_TURNON, 1);
-	if (ret < 0)
-		dev_warn(&info->pdev->dev, "register(%x) write error(%d)\n",
+	if (ret < 0) {
+		dev_err(&info->pdev->dev, "register(%x) write error(%d)\n",
 						AXP20X_CHRG_CTRL2, ret);
+		return ret;
+	}
 
 	/* Enable interrupts */
 	ret = regmap_update_bits(info->regmap,
 				AXP20X_IRQ2_EN,
 				BAT_IRQ_CFG_BAT_MASK, 1);
-	if (ret < 0)
-		dev_warn(&info->pdev->dev, "register(%x) write error(%d)\n",
+	if (ret < 0) {
+		dev_err(&info->pdev->dev, "register(%x) write error(%d)\n",
 						AXP20X_IRQ2_EN, ret);
+		return ret;
+	}
 
 	ret = regmap_update_bits(info->regmap, AXP20X_IRQ3_EN,
 				TEMP_IRQ_CFG_MASK, 1);
-	if (ret < 0)
-		dev_warn(&info->pdev->dev, "register(%x) write error(%d)\n",
+	if (ret < 0) {
+		dev_err(&info->pdev->dev, "register(%x) write error(%d)\n",
 						AXP20X_IRQ3_EN, ret);
+		return ret;
+	}
 
 	/* Setup ending condition for charging to be 10% of I(chrg) */
 	ret = regmap_update_bits(info->regmap,
 				AXP20X_CHRG_CTRL1,
 				CHRG_CCCV_ITERM_20P, 0);
-	if (ret < 0)
-		dev_warn(&info->pdev->dev, "register(%x) write error(%d)\n",
+	if (ret < 0) {
+		dev_err(&info->pdev->dev, "register(%x) write error(%d)\n",
 						AXP20X_CHRG_CTRL1, ret);
+		return ret;
+	}
 
 	/* Disable OCV-SOC curve calibration */
 	ret = regmap_update_bits(info->regmap,
 				AXP20X_CC_CTRL,
 				FG_CNTL_OCV_ADJ_EN, 0);
-	if (ret < 0)
-		dev_warn(&info->pdev->dev, "register(%x) write error(%d)\n",
+	if (ret < 0) {
+		dev_err(&info->pdev->dev, "register(%x) write error(%d)\n",
 						AXP20X_CC_CTRL, ret);
+		return ret;
+	}
 
 	/* Init charging current and voltage */
 	info->max_cc = info->pdata->max_cc;
@@ -796,15 +810,21 @@ static void charger_init_hw_regs(struct axp288_chrg_info *info)
 		cv = min(info->pdata->def_cv, info->max_cv);
 
 		ret = axp288_charger_set_cc(info, cc);
-		if (ret < 0)
-			dev_warn(&info->pdev->dev,
+		if (ret < 0) {
+			dev_err(&info->pdev->dev,
 					"error(%d) in setting CC\n", ret);
+			return ret;
+		}
 
 		ret = axp288_charger_set_cv(info, cv);
-		if (ret < 0)
-			dev_warn(&info->pdev->dev,
+		if (ret < 0) {
+			dev_err(&info->pdev->dev,
 					"error(%d) in setting CV\n", ret);
+			return ret;
+		}
 	}
+
+	return 0;
 }
 
 static int axp288_charger_probe(struct platform_device *pdev)
@@ -910,7 +930,9 @@ static int axp288_charger_probe(struct platform_device *pdev)
 		}
 	}
 
-	charger_init_hw_regs(info);
+	ret = charger_init_hw_regs(info);
+	if (ret)
+		goto intr_reg_failed;
 
 	return 0;
 
-- 
2.9.3

[PATCH v2 02/12] power: supply: axp288_charger: Drop platform_data dependency

[Hans de Goede]

When the axp288_charger driver was originally merged, it was merged with
a dependency on some other driver providing platform data for it.

However the battery-data-framework which should provide that data never
got merged, so the axp288_charger as merged upstream has never worked,
its probe method simply always returns -ENODEV.

This commit removes the dependency on the platform_data instead reading
back the charging current and charging voltage that the firmware has set
and using those values as the maximum values the user may set.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/power/supply/axp288_charger.c | 88 ++++++++++++-----------------------
 include/linux/mfd/axp20x.h            |  7 ---
 2 files changed, 30 insertions(+), 65 deletions(-)

diff --git a/drivers/power/supply/axp288_charger.c b/drivers/power/supply/axp288_charger.c
index c0f5a49..5caa154 100644
--- a/drivers/power/supply/axp288_charger.c
+++ b/drivers/power/supply/axp288_charger.c
@@ -143,7 +143,6 @@ enum {
 
 struct axp288_chrg_info {
 	struct platform_device *pdev;
-	struct axp20x_chrg_pdata *pdata;
 	struct regmap *regmap;
 	struct regmap_irq_chip_data *regmap_irqc;
 	int irq[CHRG_INTR_END];
@@ -769,60 +768,42 @@ static int charger_init_hw_regs(struct axp288_chrg_info *info)
 		return ret;
 	}
 
-	/* Init charging current and voltage */
-	info->max_cc = info->pdata->max_cc;
-	info->max_cv = info->pdata->max_cv;
-
 	/* Read current charge voltage and current limit */
 	ret = regmap_read(info->regmap, AXP20X_CHRG_CTRL1, &val);
 	if (ret < 0) {
-		/* Assume default if cannot read */
-		info->cc = info->pdata->def_cc;
-		info->cv = info->pdata->def_cv;
-	} else {
-		/* Determine charge voltage */
-		cv = (val & CHRG_CCCV_CV_MASK) >> CHRG_CCCV_CV_BIT_POS;
-		switch (cv) {
-		case CHRG_CCCV_CV_4100MV:
-			info->cv = CV_4100MV;
-			break;
-		case CHRG_CCCV_CV_4150MV:
-			info->cv = CV_4150MV;
-			break;
-		case CHRG_CCCV_CV_4200MV:
-			info->cv = CV_4200MV;
-			break;
-		case CHRG_CCCV_CV_4350MV:
-			info->cv = CV_4350MV;
-			break;
-		default:
-			info->cv = INT_MAX;
-			break;
-		}
-
-		/* Determine charge current limit */
-		cc = (ret & CHRG_CCCV_CC_MASK) >> CHRG_CCCV_CC_BIT_POS;
-		cc = (cc * CHRG_CCCV_CC_LSB_RES) + CHRG_CCCV_CC_OFFSET;
-		info->cc = cc;
+		dev_err(&info->pdev->dev, "register(%x) read error(%d)\n",
+			AXP20X_CHRG_CTRL1, ret);
+		return ret;
+	}
 
-		/* Program default charging voltage and current */
-		cc = min(info->pdata->def_cc, info->max_cc);
-		cv = min(info->pdata->def_cv, info->max_cv);
+	/* Determine charge voltage */
+	cv = (val & CHRG_CCCV_CV_MASK) >> CHRG_CCCV_CV_BIT_POS;
+	switch (cv) {
+	case CHRG_CCCV_CV_4100MV:
+		info->cv = CV_4100MV;
+		break;
+	case CHRG_CCCV_CV_4150MV:
+		info->cv = CV_4150MV;
+		break;
+	case CHRG_CCCV_CV_4200MV:
+		info->cv = CV_4200MV;
+		break;
+	case CHRG_CCCV_CV_4350MV:
+		info->cv = CV_4350MV;
+		break;
+	}
 
-		ret = axp288_charger_set_cc(info, cc);
-		if (ret < 0) {
-			dev_err(&info->pdev->dev,
-					"error(%d) in setting CC\n", ret);
-			return ret;
-		}
+	/* Determine charge current limit */
+	cc = (ret & CHRG_CCCV_CC_MASK) >> CHRG_CCCV_CC_BIT_POS;
+	cc = (cc * CHRG_CCCV_CC_LSB_RES) + CHRG_CCCV_CC_OFFSET;
+	info->cc = cc;
 
-		ret = axp288_charger_set_cv(info, cv);
-		if (ret < 0) {
-			dev_err(&info->pdev->dev,
-					"error(%d) in setting CV\n", ret);
-			return ret;
-		}
-	}
+	/*
+	 * Do not allow the user to configure higher settings then those
+	 * set by the firmware
+	 */
+	info->max_cv = info->cv;
+	info->max_cc = info->cc;
 
 	return 0;
 }
@@ -841,15 +822,6 @@ static int axp288_charger_probe(struct platform_device *pdev)
 	info->pdev = pdev;
 	info->regmap = axp20x->regmap;
 	info->regmap_irqc = axp20x->regmap_irqc;
-	info->pdata = pdev->dev.platform_data;
-
-	if (!info->pdata) {
-		/* Try ACPI provided pdata via device properties */
-		if (!device_property_present(&pdev->dev,
-						"axp288_charger_data\n"))
-			dev_err(&pdev->dev, "failed to get platform data\n");
-		return -ENODEV;
-	}
 
 	info->cable.edev = extcon_get_extcon_dev(AXP288_EXTCON_DEV_NAME);
 	if (info->cable.edev == NULL) {
diff --git a/include/linux/mfd/axp20x.h b/include/linux/mfd/axp20x.h
index 1d07ada..6715df3 100644
--- a/include/linux/mfd/axp20x.h
+++ b/include/linux/mfd/axp20x.h
@@ -532,13 +532,6 @@ struct axp20x_dev {
 	const struct regmap_irq_chip	*regmap_irq_chip;
 };
 
-struct axp20x_chrg_pdata {
-	int max_cc;
-	int max_cv;
-	int def_cc;
-	int def_cv;
-};
-
 struct axp288_extcon_pdata {
 	/* GPIO pin control to switch D+/D- lines b/w PMIC and SOC */
 	struct gpio_desc *gpio_mux_cntl;
-- 
2.9.3

[PATCH v2 03/12] power: supply: axp288_charger: Use devm_power_supply_register

[Hans de Goede]

Use devm_power_supply_register instead of power_supply_register,
this avoids the need to do manual cleanup and results in quite
a nice code cleanup.

Note it may seem excessive to add a "struct device *dev" helper local
variable for the 1 time it is used in this patch, but future patches
in this series also use it.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
-Rebase on top of for-next-next
-Only switch to devm_power_supply_register, for-next-next has already switched
 to devm_extcon_register_notifier
---
 drivers/power/supply/axp288_charger.c | 31 ++++++++-----------------------
 1 file changed, 8 insertions(+), 23 deletions(-)

diff --git a/drivers/power/supply/axp288_charger.c b/drivers/power/supply/axp288_charger.c
index 5caa154..d929742 100644
--- a/drivers/power/supply/axp288_charger.c
+++ b/drivers/power/supply/axp288_charger.c
@@ -812,6 +812,7 @@ static int axp288_charger_probe(struct platform_device *pdev)
 {
 	int ret, i, pirq;
 	struct axp288_chrg_info *info;
+	struct device *dev = &pdev->dev;
 	struct axp20x_dev *axp20x = dev_get_drvdata(pdev->dev.parent);
 	struct power_supply_config charger_cfg = {};
 
@@ -862,12 +863,12 @@ static int axp288_charger_probe(struct platform_device *pdev)
 
 	/* Register with power supply class */
 	charger_cfg.drv_data = info;
-	info->psy_usb = power_supply_register(&pdev->dev, &axp288_charger_desc,
-						&charger_cfg);
+	info->psy_usb = devm_power_supply_register(dev, &axp288_charger_desc,
+						   &charger_cfg);
 	if (IS_ERR(info->psy_usb)) {
-		dev_err(&pdev->dev, "failed to register power supply charger\n");
 		ret = PTR_ERR(info->psy_usb);
-		goto psy_reg_failed;
+		dev_err(dev, "failed to register power supply: %d\n", ret);
+		return ret;
 	}
 
 	/* Register for OTG notification */
@@ -889,8 +890,7 @@ static int axp288_charger_probe(struct platform_device *pdev)
 		if (info->irq[i] < 0) {
 			dev_warn(&info->pdev->dev,
 				"failed to get virtual interrupt=%d\n", pirq);
-			ret = info->irq[i];
-			goto intr_reg_failed;
+			return info->irq[i];
 		}
 		ret = devm_request_threaded_irq(&info->pdev->dev, info->irq[i],
 					NULL, axp288_charger_irq_thread_handler,
@@ -898,34 +898,19 @@ static int axp288_charger_probe(struct platform_device *pdev)
 		if (ret) {
 			dev_err(&pdev->dev, "failed to request interrupt=%d\n",
 								info->irq[i]);
-			goto intr_reg_failed;
+			return ret;
 		}
 	}
 
 	ret = charger_init_hw_regs(info);
 	if (ret)
-		goto intr_reg_failed;
-
-	return 0;
-
-intr_reg_failed:
-	power_supply_unregister(info->psy_usb);
-psy_reg_failed:
-	return ret;
-}
-
-static int axp288_charger_remove(struct platform_device *pdev)
-{
-	struct axp288_chrg_info *info =  dev_get_drvdata(&pdev->dev);
-
-	power_supply_unregister(info->psy_usb);
+		return ret;
 
 	return 0;
 }
 
 static struct platform_driver axp288_charger_driver = {
 	.probe = axp288_charger_probe,
-	.remove = axp288_charger_remove,
 	.driver = {
 		.name = "axp288_charger",
 	},
-- 
2.9.3

[PATCH v2 04/12] power: supply: axp288_charger: Register extcon notifers after power_supply

[Hans de Goede]

The extcon notifier work calls power_supply_changed on the power_supply
we register, so the extcon notifiers should be registered after we
register the power_supply.

While touching this code anyways, refactor the code for the 3 cable types
into a loop to avoid code repetition.

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

diff --git a/drivers/power/supply/axp288_charger.c b/drivers/power/supply/axp288_charger.c
index d929742..250dd70 100644
--- a/drivers/power/supply/axp288_charger.c
+++ b/drivers/power/supply/axp288_charger.c
@@ -815,6 +815,8 @@ static int axp288_charger_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	struct axp20x_dev *axp20x = dev_get_drvdata(pdev->dev.parent);
 	struct power_supply_config charger_cfg = {};
+	unsigned int cable_ids[] = { EXTCON_CHG_USB_SDP, EXTCON_CHG_USB_CDP,
+				     EXTCON_CHG_USB_DCP };
 
 	info = devm_kzalloc(&pdev->dev, sizeof(*info), GFP_KERNEL);
 	if (!info)
@@ -831,33 +833,6 @@ static int axp288_charger_probe(struct platform_device *pdev)
 		return -EPROBE_DEFER;
 	}
 
-	/* Register for extcon notification */
-	INIT_WORK(&info->cable.work, axp288_charger_extcon_evt_worker);
-	info->cable.nb.notifier_call = axp288_charger_handle_cable_evt;
-	ret = devm_extcon_register_notifier(&pdev->dev, info->cable.edev,
-					EXTCON_CHG_USB_SDP, &info->cable.nb);
-	if (ret) {
-		dev_err(&info->pdev->dev,
-			"failed to register extcon notifier for SDP %d\n", ret);
-		return ret;
-	}
-
-	ret = devm_extcon_register_notifier(&pdev->dev, info->cable.edev,
-					EXTCON_CHG_USB_CDP, &info->cable.nb);
-	if (ret) {
-		dev_err(&info->pdev->dev,
-			"failed to register extcon notifier for CDP %d\n", ret);
-		return ret;
-	}
-
-	ret = devm_extcon_register_notifier(&pdev->dev, info->cable.edev,
-					EXTCON_CHG_USB_DCP, &info->cable.nb);
-	if (ret) {
-		dev_err(&info->pdev->dev,
-			"failed to register extcon notifier for DCP %d\n", ret);
-		return ret;
-	}
-
 	platform_set_drvdata(pdev, info);
 	mutex_init(&info->lock);
 
@@ -871,6 +846,19 @@ static int axp288_charger_probe(struct platform_device *pdev)
 		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;
+	for (i = 0; i < ARRAY_SIZE(cable_ids); i++) {
+		ret = devm_extcon_register_notifier(dev, info->cable.edev,
+						cable_ids[i], &info->cable.nb);
+		if (ret) {
+			dev_err(dev, "failed to register extcon notifier for %u: %d\n",
+				cable_ids[i], ret);
+			return ret;
+		}
+	}
+
 	/* Register for OTG notification */
 	INIT_WORK(&info->otg.work, axp288_charger_otg_evt_worker);
 	info->otg.id_nb.notifier_call = axp288_charger_handle_otg_evt;
-- 
2.9.3

[PATCH v2 05/12] power: supply: axp288_charger: Move init_hw_regs call before supply registration

[Hans de Goede]

Move the charger_init_hw_regs() above the power_supply_register call,
the axp288_charger_usb_set_property() uses axp288_chrg_info.max_cv and
.max_cc which get set by charger_init_hw_regs().

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

diff --git a/drivers/power/supply/axp288_charger.c b/drivers/power/supply/axp288_charger.c
index 250dd70..1588efd 100644
--- a/drivers/power/supply/axp288_charger.c
+++ b/drivers/power/supply/axp288_charger.c
@@ -836,6 +836,10 @@ 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)
+		return ret;
+
 	/* Register with power supply class */
 	charger_cfg.drv_data = info;
 	info->psy_usb = devm_power_supply_register(dev, &axp288_charger_desc,
@@ -890,10 +894,6 @@ static int axp288_charger_probe(struct platform_device *pdev)
 		}
 	}
 
-	ret = charger_init_hw_regs(info);
-	if (ret)
-		return ret;
-
 	return 0;
 }
 
-- 
2.9.3

[PATCH v2 06/12] power: supply: axp288_charger: Actually get and use the USB_HOST extcon device

[Hans de Goede]

Nothing was setting info->otg.cable, so the extcon_get_cable_state_
calls on it would always return -EINVAL.

This commit fixes this by actually setting info->otg.cable using the new
extcon_get_extcon_dev_by_cable_id function.

This commit also makes failing to register the extcon notifier for the
USB_HOST cable an error rather then a warning, because we MUST have this
notfier to properly disable the VBUS path when in host mode so that we're
not drawing current from the 5V boost converter which is supplying power
to the otg port when in host mode.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
-Use extcon_get_extcon_dev instead of the new extcon_get_extcon_dev_by_id
 which was not accepted by the extcon upstream maintainer
---
 drivers/power/supply/axp288_charger.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/drivers/power/supply/axp288_charger.c b/drivers/power/supply/axp288_charger.c
index 1588efd..4443f11 100644
--- a/drivers/power/supply/axp288_charger.c
+++ b/drivers/power/supply/axp288_charger.c
@@ -127,6 +127,7 @@
 #define ILIM_3000MA			3000	/* 3000mA */
 
 #define AXP288_EXTCON_DEV_NAME		"axp288_extcon"
+#define USB_HOST_EXTCON_DEV_NAME	"INT3496:00"
 
 enum {
 	VBUS_OV_IRQ = 0,
@@ -833,6 +834,12 @@ static int axp288_charger_probe(struct platform_device *pdev)
 		return -EPROBE_DEFER;
 	}
 
+	info->otg.cable = extcon_get_extcon_dev(USB_HOST_EXTCON_DEV_NAME);
+	if (info->otg.cable == NULL) {
+		dev_dbg(dev, "EXTCON_USB_HOST is not ready, probe deferred\n");
+		return -EPROBE_DEFER;
+	}
+
 	platform_set_drvdata(pdev, info);
 	mutex_init(&info->lock);
 
@@ -868,12 +875,12 @@ static int axp288_charger_probe(struct platform_device *pdev)
 	info->otg.id_nb.notifier_call = axp288_charger_handle_otg_evt;
 	ret = devm_extcon_register_notifier(&pdev->dev, info->otg.cable,
 					EXTCON_USB_HOST, &info->otg.id_nb);
-	if (ret)
-		dev_warn(&pdev->dev, "failed to register otg notifier\n");
-
-	if (info->otg.cable)
-		info->otg.id_short = extcon_get_state(
-					info->otg.cable, EXTCON_USB_HOST);
+	if (ret) {
+		dev_err(dev, "failed to register EXTCON_USB_HOST notifier\n");
+		return ret;
+	}
+	info->otg.id_short = extcon_get_cable_state_(info->otg.cable,
+						     EXTCON_USB_HOST);
 
 	/* Register charger interrupts */
 	for (i = 0; i < CHRG_INTR_END; i++) {
-- 
2.9.3

[PATCH v2 07/12] power: supply: axp288_charger: Handle charger type changing without disconnect

[Hans de Goede]

Deal with the charger type changing without a vbus-disconnect being
reported in between the 2 charger type states:

-Do not return from axp288_charger_extcon_evt_worker early in this case
 (track old_chg_type)
-Make calling axp288_charger_enable_charger with the same value as before
 a nop, to avoid the need for the caller to check this
-Do no do a dev_err when axp288_charger_enable_charger returns an error,
 axp288_charger_enable_charger already returns an error itself
-Disable the charger before changing the charge-current setting (nop if
 vbus was seen as disconnected before the change)

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

diff --git a/drivers/power/supply/axp288_charger.c b/drivers/power/supply/axp288_charger.c
index 4443f11..03395d2 100644
--- a/drivers/power/supply/axp288_charger.c
+++ b/drivers/power/supply/axp288_charger.c
@@ -175,7 +175,6 @@ struct axp288_chrg_info {
 	int max_cv;
 	bool online;
 	bool present;
-	bool enable_charger;
 	bool is_charger_enabled;
 };
 
@@ -305,6 +304,9 @@ static int axp288_charger_enable_charger(struct axp288_chrg_info *info,
 {
 	int ret;
 
+	if (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);
@@ -579,6 +581,7 @@ static void axp288_charger_extcon_evt_worker(struct work_struct *work)
 	bool changed = false;
 	struct extcon_dev *edev = info->cable.edev;
 	bool old_connected = info->cable.connected;
+	enum power_supply_type old_chg_type = info->cable.chg_type;
 
 	/* Determine cable/charger type */
 	if (extcon_get_state(edev, EXTCON_CHG_USB_SDP) > 0) {
@@ -601,7 +604,8 @@ static void axp288_charger_extcon_evt_worker(struct work_struct *work)
 	}
 
 	/* Cable status changed */
-	if (old_connected != info->cable.connected)
+	if (old_connected != info->cable.connected ||
+	    old_chg_type != info->cable.chg_type)
 		changed = true;
 
 	if (!changed)
@@ -609,14 +613,9 @@ static void axp288_charger_extcon_evt_worker(struct work_struct *work)
 
 	mutex_lock(&info->lock);
 
-	if (info->is_charger_enabled && !info->cable.connected) {
-		info->enable_charger = false;
-		ret = axp288_charger_enable_charger(info, info->enable_charger);
-		if (ret < 0)
-			dev_err(&info->pdev->dev,
-				"cannot disable charger (%d)", ret);
+	if (info->cable.connected) {
+		axp288_charger_enable_charger(info, false);
 
-	} else if (!info->is_charger_enabled && info->cable.connected) {
 		switch (info->cable.chg_type) {
 		case POWER_SUPPLY_TYPE_USB:
 			current_limit = ILIM_500MA;
@@ -635,17 +634,13 @@ static void axp288_charger_extcon_evt_worker(struct work_struct *work)
 
 		/* Set vbus current limit first, then enable charger */
 		ret = axp288_charger_set_vbus_inlmt(info, current_limit);
-		if (ret < 0) {
+		if (ret == 0)
+			axp288_charger_enable_charger(info, true);
+		else
 			dev_err(&info->pdev->dev,
 				"error setting current limit (%d)", ret);
-		} else {
-			info->enable_charger = (current_limit > 0);
-			ret = axp288_charger_enable_charger(info,
-							info->enable_charger);
-			if (ret < 0)
-				dev_err(&info->pdev->dev,
-					"cannot enable charger (%d)", ret);
-		}
+	} else {
+		axp288_charger_enable_charger(info, false);
 	}
 
 	if (changed)
-- 
2.9.3

[PATCH v2 08/12] power: supply: axp288_charger: Some minor cleanups

[Hans de Goede]

Remove info->health, info->present and info->online caching, as no code
is reading the cached values.

Remove if (changed) check before calling power_supply_changed(), we
return early from axp288_charger_extcon_evt_worker if nothing has
changed, so the check is not needed.

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

diff --git a/drivers/power/supply/axp288_charger.c b/drivers/power/supply/axp288_charger.c
index 03395d2..4dd1a5f 100644
--- a/drivers/power/supply/axp288_charger.c
+++ b/drivers/power/supply/axp288_charger.c
@@ -167,14 +167,11 @@ struct axp288_chrg_info {
 		struct work_struct work;
 	} cable;
 
-	int health;
 	int inlmt;
 	int cc;
 	int cv;
 	int max_cc;
 	int max_cv;
-	bool online;
-	bool present;
 	bool is_charger_enabled;
 };
 
@@ -432,8 +429,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;
-		info->present = ret;
-		val->intval = info->present;
+		val->intval = ret;
 		break;
 	case POWER_SUPPLY_PROP_ONLINE:
 		/* Check for OTG case first */
@@ -444,8 +440,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;
-		info->online = ret;
-		val->intval = info->online;
+		val->intval = ret;
 		break;
 	case POWER_SUPPLY_PROP_HEALTH:
 		val->intval = axp288_get_charger_health(info);
@@ -578,7 +573,6 @@ static void axp288_charger_extcon_evt_worker(struct work_struct *work)
 	struct axp288_chrg_info *info =
 	    container_of(work, struct axp288_chrg_info, cable.work);
 	int ret, current_limit;
-	bool changed = false;
 	struct extcon_dev *edev = info->cable.edev;
 	bool old_connected = info->cable.connected;
 	enum power_supply_type old_chg_type = info->cable.chg_type;
@@ -604,11 +598,8 @@ static void axp288_charger_extcon_evt_worker(struct work_struct *work)
 	}
 
 	/* Cable status changed */
-	if (old_connected != info->cable.connected ||
-	    old_chg_type != info->cable.chg_type)
-		changed = true;
-
-	if (!changed)
+	if (old_connected == info->cable.connected &&
+	    old_chg_type == info->cable.chg_type)
 		return;
 
 	mutex_lock(&info->lock);
@@ -643,13 +634,9 @@ static void axp288_charger_extcon_evt_worker(struct work_struct *work)
 		axp288_charger_enable_charger(info, false);
 	}
 
-	if (changed)
-		info->health = axp288_get_charger_health(info);
-
 	mutex_unlock(&info->lock);
 
-	if (changed)
-		power_supply_changed(info->psy_usb);
+	power_supply_changed(info->psy_usb);
 }
 
 static int axp288_charger_handle_cable_evt(struct notifier_block *nb,
-- 
2.9.3

[PATCH v2 09/12] power: supply: axp288_charger: Get and process initial hardware-state

[Hans de Goede]

Do not wait for an extcon notification before processing the cable
states, instead queue the otg / cable work on probe to make sure we
immediately process the initial hardware state.

Note this also requiree moving the getting of the USB_HOST cable state
from the extcon notifier to the workqueue function.

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

diff --git a/drivers/power/supply/axp288_charger.c b/drivers/power/supply/axp288_charger.c
index 4dd1a5f..42a6b92 100644
--- a/drivers/power/supply/axp288_charger.c
+++ b/drivers/power/supply/axp288_charger.c
@@ -172,7 +172,7 @@ struct axp288_chrg_info {
 	int cv;
 	int max_cc;
 	int max_cv;
-	bool is_charger_enabled;
+	int is_charger_enabled;
 };
 
 static inline int axp288_charger_set_cc(struct axp288_chrg_info *info, int cc)
@@ -301,7 +301,7 @@ static int axp288_charger_enable_charger(struct axp288_chrg_info *info,
 {
 	int ret;
 
-	if (enable == info->is_charger_enabled)
+	if ((int)enable == info->is_charger_enabled)
 		return 0;
 
 	if (enable)
@@ -654,7 +654,17 @@ static void axp288_charger_otg_evt_worker(struct work_struct *work)
 {
 	struct axp288_chrg_info *info =
 	    container_of(work, struct axp288_chrg_info, otg.work);
-	int ret;
+	struct extcon_dev *edev = info->otg.cable;
+	int ret, usb_host = extcon_get_state(edev, EXTCON_USB_HOST);
+
+	dev_dbg(&info->pdev->dev, "external connector USB-Host is %s\n",
+				usb_host ? "attached" : "detached");
+
+	/*
+	 * Set usb_id_short flag to avoid running charger detection logic
+	 * in case usb host.
+	 */
+	info->otg.id_short = usb_host;
 
 	/* Disable VBUS path before enabling the 5V boost */
 	ret = axp288_charger_vbus_path_select(info, !info->otg.id_short);
@@ -667,17 +677,7 @@ static int axp288_charger_handle_otg_evt(struct notifier_block *nb,
 {
 	struct axp288_chrg_info *info =
 	    container_of(nb, struct axp288_chrg_info, otg.id_nb);
-	struct extcon_dev *edev = info->otg.cable;
-	int usb_host = extcon_get_state(edev, EXTCON_USB_HOST);
-
-	dev_dbg(&info->pdev->dev, "external connector USB-Host is %s\n",
-				usb_host ? "attached" : "detached");
 
-	/*
-	 * Set usb_id_short flag to avoid running charger detection logic
-	 * in case usb host.
-	 */
-	info->otg.id_short = usb_host;
 	schedule_work(&info->otg.work);
 
 	return NOTIFY_OK;
@@ -808,6 +808,8 @@ 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);
 	if (info->cable.edev == NULL) {
@@ -851,6 +853,7 @@ static int axp288_charger_probe(struct platform_device *pdev)
 			return ret;
 		}
 	}
+	schedule_work(&info->cable.work);
 
 	/* Register for OTG notification */
 	INIT_WORK(&info->otg.work, axp288_charger_otg_evt_worker);
@@ -861,8 +864,7 @@ static int axp288_charger_probe(struct platform_device *pdev)
 		dev_err(dev, "failed to register EXTCON_USB_HOST notifier\n");
 		return ret;
 	}
-	info->otg.id_short = extcon_get_cable_state_(info->otg.cable,
-						     EXTCON_USB_HOST);
+	schedule_work(&info->otg.work);
 
 	/* Register charger interrupts */
 	for (i = 0; i < CHRG_INTR_END; i++) {
-- 
2.9.3

[PATCH v2 10/12] power: supply: axp288_charger: Fix wrong regmap_update_bits

[Hans de Goede]

To set a bit to 1 one needs to pass the mask for the bit to set
as second argument into regmap_update_bits, not "1".

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

diff --git a/drivers/power/supply/axp288_charger.c b/drivers/power/supply/axp288_charger.c
index 42a6b92..d3932cd 100644
--- a/drivers/power/supply/axp288_charger.c
+++ b/drivers/power/supply/axp288_charger.c
@@ -706,7 +706,7 @@ static int charger_init_hw_regs(struct axp288_chrg_info *info)
 	/* Do not turn-off charger o/p after charge cycle ends */
 	ret = regmap_update_bits(info->regmap,
 				AXP20X_CHRG_CTRL2,
-				CNTL2_CHG_OUT_TURNON, 1);
+				CNTL2_CHG_OUT_TURNON, CNTL2_CHG_OUT_TURNON);
 	if (ret < 0) {
 		dev_err(&info->pdev->dev, "register(%x) write error(%d)\n",
 						AXP20X_CHRG_CTRL2, ret);
-- 
2.9.3

[PATCH v2 11/12] power: supply: axp288_charger: Remove unnecessary irq?_en register writes

[Hans de Goede]

Setting the irq_enable bits is taken care of by the irq chip when we
request the irqs and the driver should not be meddling with the
irq?_en registers itself.

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

diff --git a/drivers/power/supply/axp288_charger.c b/drivers/power/supply/axp288_charger.c
index d3932cd..6e588e6 100644
--- a/drivers/power/supply/axp288_charger.c
+++ b/drivers/power/supply/axp288_charger.c
@@ -90,20 +90,6 @@
 #define CHRG_VLTFC_0C			0xA5	/* 0 DegC */
 #define CHRG_VHTFC_45C			0x1F	/* 45 DegC */
 
-#define BAT_IRQ_CFG_CHRG_DONE		(1 << 2)
-#define BAT_IRQ_CFG_CHRG_START		(1 << 3)
-#define BAT_IRQ_CFG_BAT_SAFE_EXIT	(1 << 4)
-#define BAT_IRQ_CFG_BAT_SAFE_ENTER	(1 << 5)
-#define BAT_IRQ_CFG_BAT_DISCON		(1 << 6)
-#define BAT_IRQ_CFG_BAT_CONN		(1 << 7)
-#define BAT_IRQ_CFG_BAT_MASK		0xFC
-
-#define TEMP_IRQ_CFG_QCBTU		(1 << 4)
-#define TEMP_IRQ_CFG_CBTU		(1 << 5)
-#define TEMP_IRQ_CFG_QCBTO		(1 << 6)
-#define TEMP_IRQ_CFG_CBTO		(1 << 7)
-#define TEMP_IRQ_CFG_MASK		0xF0
-
 #define FG_CNTL_OCV_ADJ_EN		(1 << 3)
 
 #define CV_4100MV			4100	/* 4100mV */
@@ -713,24 +699,6 @@ static int charger_init_hw_regs(struct axp288_chrg_info *info)
 		return ret;
 	}
 
-	/* Enable interrupts */
-	ret = regmap_update_bits(info->regmap,
-				AXP20X_IRQ2_EN,
-				BAT_IRQ_CFG_BAT_MASK, 1);
-	if (ret < 0) {
-		dev_err(&info->pdev->dev, "register(%x) write error(%d)\n",
-						AXP20X_IRQ2_EN, ret);
-		return ret;
-	}
-
-	ret = regmap_update_bits(info->regmap, AXP20X_IRQ3_EN,
-				TEMP_IRQ_CFG_MASK, 1);
-	if (ret < 0) {
-		dev_err(&info->pdev->dev, "register(%x) write error(%d)\n",
-						AXP20X_IRQ3_EN, ret);
-		return ret;
-	}
-
 	/* Setup ending condition for charging to be 10% of I(chrg) */
 	ret = regmap_update_bits(info->regmap,
 				AXP20X_CHRG_CTRL1,
-- 
2.9.3

[PATCH v2 12/12] power: supply: axp288_charger: Fix the module not auto-loading

[Hans de Goede]

Add a MODULE_DEVICE_TABLE to fix the module not auto-loading.

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

diff --git a/drivers/power/supply/axp288_charger.c b/drivers/power/supply/axp288_charger.c
index 6e588e6..ca4fa51 100644
--- a/drivers/power/supply/axp288_charger.c
+++ b/drivers/power/supply/axp288_charger.c
@@ -856,8 +856,15 @@ static int axp288_charger_probe(struct platform_device *pdev)
 	return 0;
 }
 
+static const struct platform_device_id axp288_charger_id_table[] = {
+	{ .name = "axp288_charger" },
+	{},
+};
+MODULE_DEVICE_TABLE(platform, axp288_charger_id_table);
+
 static struct platform_driver axp288_charger_driver = {
 	.probe = axp288_charger_probe,
+	.id_table = axp288_charger_id_table,
 	.driver = {
 		.name = "axp288_charger",
 	},
-- 
2.9.3

Re: [PATCH v2 00/12] power: supply: axp288_charger: Various fixes

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 684 bytes --]

Hi,

On Wed, Dec 21, 2016 at 03:36:45PM +0100, Hans de Goede wrote:
> Here is v2 of my axp288_charger series, which fixup the axp288_charger
> code to actually work.
> 
> This implements the suggestion by Chanwoo Choi on how we can use the
> existing extcon_get_extcon_dev instead of introducing a new
> extcon_get_extcon_dev_by_id. So this series no longer depends on any
> extcon sub-sys changes.
> 
> I've also rebased it on top of linux-power-supply/for-next-next to avoid
> some conflicts with patches already there which Chanwoo pointed out,
> so assuming review goes well merging these to for-next-next should go
> smoothly :)

Thanks, queued.

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]