[PATCH 00/14] extcon api extension + axp288_charger fixes

[PATCH 00/14] extcon api extension + axp288_charger fixes

[Hans de Goede]

Hi extcon and power-supply system maintainers,

This series really are 2 separate series, patches 1 - 2 add a new
extcon_get_extcon_dev_by_cable_id function to the extcon core.

Patches 3 - 15 consist of various fixes for the axp288_charger
driver, including one fix which uses the new extcon_get_extcon_dev_by_cable_id
making this series depend on the extcon patches.

So we will need some coordination between the merging of these 2
series. Since the extcon patches are quite simple, perhaps they can
still be merged into 4.10 ? Otherwise the extcon maintainers will need
to provide a stable branch with these 2 patches which the power-supply
maintainer can merge into power-supply/next .

Regards,

Hans

[PATCH 01/14] extcon: Add extcon_get_extcon_dev_by_cable_id function

[Hans de Goede]

extcon_register_notifier() allows passing in a NULL pointer for the
extcon_device, so that extcon consumers which want extcon events of a
certain type, but do not know the extcon device name (e.g. because
there are different implementation depending on the board), can still
get such events.

But most drivers will also want to know the initial state of the cable.
Rather then adding NULL edev argument support to extcon_get_state, which
would require looking up the right edev each call, this commit allows
drivers to get the first extcon device with a requested cable-id through
a new extcon_get_extcon_dev_by_cable_id function.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/extcon/extcon.c | 24 ++++++++++++++++++++++++
 include/linux/extcon.h  |  1 +
 2 files changed, 25 insertions(+)

diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
index 7829846..505c272 100644
--- a/drivers/extcon/extcon.c
+++ b/drivers/extcon/extcon.c
@@ -890,6 +890,30 @@ struct extcon_dev *extcon_get_extcon_dev(const char *extcon_name)
 EXPORT_SYMBOL_GPL(extcon_get_extcon_dev);
 
 /**
+ * extcon_get_extcon_dev_by_cable_id() - Get an extcon device by a cable id
+ * @id:		the unique id of each external connector in extcon enumeration.
+ */
+struct extcon_dev *extcon_get_extcon_dev_by_cable_id(unsigned int id)
+{
+	struct extcon_dev *extd;
+	int idx = -EINVAL;
+
+	mutex_lock(&extcon_dev_list_lock);
+	list_for_each_entry(extd, &extcon_dev_list, entry) {
+		idx = find_cable_index_by_id(extd, id);
+		if (idx >= 0)
+			break;
+	}
+	mutex_unlock(&extcon_dev_list_lock);
+
+	if (idx < 0)
+		return NULL;
+
+	return extd;
+}
+EXPORT_SYMBOL_GPL(extcon_get_extcon_dev_by_cable_id);
+
+/**
  * extcon_register_notifier() - Register a notifiee to get notified by
  *				any attach status changes from the extcon.
  * @edev:	the extcon device that has the external connecotr.
diff --git a/include/linux/extcon.h b/include/linux/extcon.h
index b871c0c..51abda8 100644
--- a/include/linux/extcon.h
+++ b/include/linux/extcon.h
@@ -230,6 +230,7 @@ extern int devm_extcon_dev_register(struct device *dev,
 extern void devm_extcon_dev_unregister(struct device *dev,
 				       struct extcon_dev *edev);
 extern struct extcon_dev *extcon_get_extcon_dev(const char *extcon_name);
+extern struct extcon_dev *extcon_get_extcon_dev_by_cable_id(unsigned int id);
 
 /*
  * Following APIs control the memory of extcon device.
-- 
2.9.3

[PATCH 02/14] extcon: Make extcon_register_notifier use extcon_get_extcon_dev_by_cable_id

[Hans de Goede]

Implement extcon_register_notifier's support for a NULL edev argument
using the new extcon_get_extcon_dev_by_cable_id function.

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

diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
index 505c272..2816187 100644
--- a/drivers/extcon/extcon.c
+++ b/drivers/extcon/extcon.c
@@ -933,32 +933,19 @@ int extcon_register_notifier(struct extcon_dev *edev, unsigned int id,
 	if (!nb)
 		return -EINVAL;
 
-	if (edev) {
-		idx = find_cable_index_by_id(edev, id);
-		if (idx < 0)
-			return idx;
+	if (!edev) {
+		edev = extcon_get_extcon_dev_by_cable_id(id);
+		if (!edev)
+			return -ENODEV;
+	}
 
-		spin_lock_irqsave(&edev->lock, flags);
-		ret = raw_notifier_chain_register(&edev->nh[idx], nb);
-		spin_unlock_irqrestore(&edev->lock, flags);
-	} else {
-		struct extcon_dev *extd;
-
-		mutex_lock(&extcon_dev_list_lock);
-		list_for_each_entry(extd, &extcon_dev_list, entry) {
-			idx = find_cable_index_by_id(extd, id);
-			if (idx >= 0)
-				break;
-		}
-		mutex_unlock(&extcon_dev_list_lock);
+	idx = find_cable_index_by_id(edev, id);
+	if (idx < 0)
+		return idx;
 
-		if (idx >= 0) {
-			edev = extd;
-			return extcon_register_notifier(extd, id, nb);
-		} else {
-			ret = -ENODEV;
-		}
-	}
+	spin_lock_irqsave(&edev->lock, flags);
+	ret = raw_notifier_chain_register(&edev->nh[idx], nb);
+	spin_unlock_irqrestore(&edev->lock, flags);
 
 	return ret;
 }
-- 
2.9.3

[PATCH 03/14] 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 75b8e0c..f1e3b0e 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)
@@ -916,7 +936,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 04/14] 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 f1e3b0e..76f7d29 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 05/14] power: supply: axp288_charger: use devm extcon / supply register

[Hans de Goede]

Use devm_extcon_register_notifier and devm_power_supply_register
instead of their non devm counterparts, this avoids the need to do
manual cleanup and results in quite a nice code cleanup.

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

diff --git a/drivers/power/supply/axp288_charger.c b/drivers/power/supply/axp288_charger.c
index 76f7d29..e435b1e 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 = {};
 
@@ -833,33 +834,27 @@ 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.notifier_call = axp288_charger_handle_cable_evt;
-	ret = extcon_register_notifier(info->cable.edev, EXTCON_CHG_USB_SDP,
-					&info->cable.nb);
+	ret = devm_extcon_register_notifier(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 = extcon_register_notifier(info->cable.edev, EXTCON_CHG_USB_CDP,
-					&info->cable.nb);
+	ret = devm_extcon_register_notifier(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);
-		extcon_unregister_notifier(info->cable.edev,
-				EXTCON_CHG_USB_SDP, &info->cable.nb);
 		return ret;
 	}
 
-	ret = extcon_register_notifier(info->cable.edev, EXTCON_CHG_USB_DCP,
-					&info->cable.nb);
+	ret = devm_extcon_register_notifier(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);
-		extcon_unregister_notifier(info->cable.edev,
-				EXTCON_CHG_USB_SDP, &info->cable.nb);
-		extcon_unregister_notifier(info->cable.edev,
-				EXTCON_CHG_USB_CDP, &info->cable.nb);
 		return ret;
 	}
 
@@ -868,19 +863,18 @@ 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;
+		return PTR_ERR(info->psy_usb);
 	}
 
 	/* 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;
-	ret = extcon_register_notifier(info->otg.cable, EXTCON_USB_HOST,
-				       &info->otg.id_nb);
+	ret = devm_extcon_register_notifier(dev, info->otg.cable,
+					    EXTCON_USB_HOST, &info->otg.id_nb);
 	if (ret)
 		dev_warn(&pdev->dev, "failed to register otg notifier\n");
 
@@ -895,8 +889,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,
@@ -904,53 +897,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:
-	if (info->otg.cable)
-		extcon_unregister_notifier(info->otg.cable, EXTCON_USB_HOST,
-					&info->otg.id_nb);
-	power_supply_unregister(info->psy_usb);
-psy_reg_failed:
-	extcon_unregister_notifier(info->cable.edev, EXTCON_CHG_USB_SDP,
-					&info->cable.nb);
-	extcon_unregister_notifier(info->cable.edev, EXTCON_CHG_USB_CDP,
-					&info->cable.nb);
-	extcon_unregister_notifier(info->cable.edev, EXTCON_CHG_USB_DCP,
-					&info->cable.nb);
-	return ret;
-}
-
-static int axp288_charger_remove(struct platform_device *pdev)
-{
-	struct axp288_chrg_info *info =  dev_get_drvdata(&pdev->dev);
-
-	if (info->otg.cable)
-		extcon_unregister_notifier(info->otg.cable, EXTCON_USB_HOST,
-					&info->otg.id_nb);
-
-	extcon_unregister_notifier(info->cable.edev, EXTCON_CHG_USB_SDP,
-					&info->cable.nb);
-	extcon_unregister_notifier(info->cable.edev, EXTCON_CHG_USB_CDP,
-					&info->cable.nb);
-	extcon_unregister_notifier(info->cable.edev, EXTCON_CHG_USB_DCP,
-					&info->cable.nb);
-	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 06/14] 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 e435b1e..5ed0a45 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(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(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(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);
 
@@ -870,6 +845,19 @@ static int axp288_charger_probe(struct platform_device *pdev)
 		return PTR_ERR(info->psy_usb);
 	}
 
+	/* 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 07/14] 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 5ed0a45..08a5dba 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,
@@ -889,10 +893,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 08/14] 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>
---
 drivers/power/supply/axp288_charger.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/power/supply/axp288_charger.c b/drivers/power/supply/axp288_charger.c
index 08a5dba..2b95db2 100644
--- a/drivers/power/supply/axp288_charger.c
+++ b/drivers/power/supply/axp288_charger.c
@@ -833,6 +833,12 @@ static int axp288_charger_probe(struct platform_device *pdev)
 		return -EPROBE_DEFER;
 	}
 
+	info->otg.cable = extcon_get_extcon_dev_by_cable_id(EXTCON_USB_HOST);
+	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);
 
@@ -867,12 +873,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(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_cable_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 09/14] 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 2b95db2..adeea2f 100644
--- a/drivers/power/supply/axp288_charger.c
+++ b/drivers/power/supply/axp288_charger.c
@@ -174,7 +174,6 @@ struct axp288_chrg_info {
 	int max_cv;
 	bool online;
 	bool present;
-	bool enable_charger;
 	bool is_charger_enabled;
 };
 
@@ -304,6 +303,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);
@@ -578,6 +580,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_cable_state_(edev, EXTCON_CHG_USB_SDP) > 0) {
@@ -600,7 +603,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)
@@ -608,14 +612,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;
@@ -634,17 +633,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 10/14] 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 adeea2f..f033a5e 100644
--- a/drivers/power/supply/axp288_charger.c
+++ b/drivers/power/supply/axp288_charger.c
@@ -166,14 +166,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;
 };
 
@@ -431,8 +428,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 */
@@ -443,8 +439,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);
@@ -577,7 +572,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;
@@ -603,11 +597,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);
@@ -642,13 +633,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 11/14] 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 f033a5e..b489289 100644
--- a/drivers/power/supply/axp288_charger.c
+++ b/drivers/power/supply/axp288_charger.c
@@ -171,7 +171,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)
@@ -300,7 +300,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)
@@ -653,7 +653,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_cable_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);
@@ -666,17 +676,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_cable_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;
@@ -807,6 +807,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) {
@@ -849,6 +851,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);
@@ -859,8 +862,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 12/14] 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 b489289..4a19666 100644
--- a/drivers/power/supply/axp288_charger.c
+++ b/drivers/power/supply/axp288_charger.c
@@ -705,7 +705,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 13/14] 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 4a19666..9b93453 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 */
@@ -712,24 +698,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 14/14] 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 9b93453..6165663 100644
--- a/drivers/power/supply/axp288_charger.c
+++ b/drivers/power/supply/axp288_charger.c
@@ -854,8 +854,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 05/14] power: supply: axp288_charger: use devm extcon / supply register

[Chanwoo Choi]

Hi Hans,

For the extcon part, I sent the patch[1]. 

[1]https://git.kernel.org/cgit/linux/kernel/git/sre/linux-power-supply.git/commit/?h=for-next-next&id=3164eef74088d9603a16b97e574a4e0067da083a

Regards,
Chanwoo Choi

On 2016년 12월 19일 09:07, Hans de Goede wrote:
> Use devm_extcon_register_notifier and devm_power_supply_register
> instead of their non devm counterparts, this avoids the need to do
> manual cleanup and results in quite a nice code cleanup.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/power/supply/axp288_charger.c | 71 ++++++++---------------------------
>  1 file changed, 15 insertions(+), 56 deletions(-)
> 
> diff --git a/drivers/power/supply/axp288_charger.c b/drivers/power/supply/axp288_charger.c
> index 76f7d29..e435b1e 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 = {};
>  
> @@ -833,33 +834,27 @@ 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.notifier_call = axp288_charger_handle_cable_evt;
> -	ret = extcon_register_notifier(info->cable.edev, EXTCON_CHG_USB_SDP,
> -					&info->cable.nb);
> +	ret = devm_extcon_register_notifier(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 = extcon_register_notifier(info->cable.edev, EXTCON_CHG_USB_CDP,
> -					&info->cable.nb);
> +	ret = devm_extcon_register_notifier(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);
> -		extcon_unregister_notifier(info->cable.edev,
> -				EXTCON_CHG_USB_SDP, &info->cable.nb);
>  		return ret;
>  	}
>  
> -	ret = extcon_register_notifier(info->cable.edev, EXTCON_CHG_USB_DCP,
> -					&info->cable.nb);
> +	ret = devm_extcon_register_notifier(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);
> -		extcon_unregister_notifier(info->cable.edev,
> -				EXTCON_CHG_USB_SDP, &info->cable.nb);
> -		extcon_unregister_notifier(info->cable.edev,
> -				EXTCON_CHG_USB_CDP, &info->cable.nb);
>  		return ret;
>  	}
>  
> @@ -868,19 +863,18 @@ 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;
> +		return PTR_ERR(info->psy_usb);
>  	}
>  
>  	/* 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;
> -	ret = extcon_register_notifier(info->otg.cable, EXTCON_USB_HOST,
> -				       &info->otg.id_nb);
> +	ret = devm_extcon_register_notifier(dev, info->otg.cable,
> +					    EXTCON_USB_HOST, &info->otg.id_nb);
>  	if (ret)
>  		dev_warn(&pdev->dev, "failed to register otg notifier\n");
>  
> @@ -895,8 +889,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,
> @@ -904,53 +897,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:
> -	if (info->otg.cable)
> -		extcon_unregister_notifier(info->otg.cable, EXTCON_USB_HOST,
> -					&info->otg.id_nb);
> -	power_supply_unregister(info->psy_usb);
> -psy_reg_failed:
> -	extcon_unregister_notifier(info->cable.edev, EXTCON_CHG_USB_SDP,
> -					&info->cable.nb);
> -	extcon_unregister_notifier(info->cable.edev, EXTCON_CHG_USB_CDP,
> -					&info->cable.nb);
> -	extcon_unregister_notifier(info->cable.edev, EXTCON_CHG_USB_DCP,
> -					&info->cable.nb);
> -	return ret;
> -}
> -
> -static int axp288_charger_remove(struct platform_device *pdev)
> -{
> -	struct axp288_chrg_info *info =  dev_get_drvdata(&pdev->dev);
> -
> -	if (info->otg.cable)
> -		extcon_unregister_notifier(info->otg.cable, EXTCON_USB_HOST,
> -					&info->otg.id_nb);
> -
> -	extcon_unregister_notifier(info->cable.edev, EXTCON_CHG_USB_SDP,
> -					&info->cable.nb);
> -	extcon_unregister_notifier(info->cable.edev, EXTCON_CHG_USB_CDP,
> -					&info->cable.nb);
> -	extcon_unregister_notifier(info->cable.edev, EXTCON_CHG_USB_DCP,
> -					&info->cable.nb);
> -	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",
>  	},
> 

Re: [PATCH 05/14] power: supply: axp288_charger: use devm extcon / supply register

[Hans de Goede]

Hi,

On 19-12-16 09:03, Chanwoo Choi wrote:
> Hi Hans,
>
> For the extcon part, I sent the patch[1].
>
> [1]https://git.kernel.org/cgit/linux/kernel/git/sre/linux-power-supply.git/commit/?h=for-next-next&id=3164eef74088d9603a16b97e574a4e0067da083a

Ok, I will cherry pick this one into my tree and rebase the patch-set
on top of it.

Regards,

Hans


>
> Regards,
> Chanwoo Choi
>
> On 2016년 12월 19일 09:07, Hans de Goede wrote:
>> Use devm_extcon_register_notifier and devm_power_supply_register
>> instead of their non devm counterparts, this avoids the need to do
>> manual cleanup and results in quite a nice code cleanup.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>  drivers/power/supply/axp288_charger.c | 71 ++++++++---------------------------
>>  1 file changed, 15 insertions(+), 56 deletions(-)
>>
>> diff --git a/drivers/power/supply/axp288_charger.c b/drivers/power/supply/axp288_charger.c
>> index 76f7d29..e435b1e 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 = {};
>>
>> @@ -833,33 +834,27 @@ 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.notifier_call = axp288_charger_handle_cable_evt;
>> -	ret = extcon_register_notifier(info->cable.edev, EXTCON_CHG_USB_SDP,
>> -					&info->cable.nb);
>> +	ret = devm_extcon_register_notifier(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 = extcon_register_notifier(info->cable.edev, EXTCON_CHG_USB_CDP,
>> -					&info->cable.nb);
>> +	ret = devm_extcon_register_notifier(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);
>> -		extcon_unregister_notifier(info->cable.edev,
>> -				EXTCON_CHG_USB_SDP, &info->cable.nb);
>>  		return ret;
>>  	}
>>
>> -	ret = extcon_register_notifier(info->cable.edev, EXTCON_CHG_USB_DCP,
>> -					&info->cable.nb);
>> +	ret = devm_extcon_register_notifier(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);
>> -		extcon_unregister_notifier(info->cable.edev,
>> -				EXTCON_CHG_USB_SDP, &info->cable.nb);
>> -		extcon_unregister_notifier(info->cable.edev,
>> -				EXTCON_CHG_USB_CDP, &info->cable.nb);
>>  		return ret;
>>  	}
>>
>> @@ -868,19 +863,18 @@ 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;
>> +		return PTR_ERR(info->psy_usb);
>>  	}
>>
>>  	/* 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;
>> -	ret = extcon_register_notifier(info->otg.cable, EXTCON_USB_HOST,
>> -				       &info->otg.id_nb);
>> +	ret = devm_extcon_register_notifier(dev, info->otg.cable,
>> +					    EXTCON_USB_HOST, &info->otg.id_nb);
>>  	if (ret)
>>  		dev_warn(&pdev->dev, "failed to register otg notifier\n");
>>
>> @@ -895,8 +889,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,
>> @@ -904,53 +897,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:
>> -	if (info->otg.cable)
>> -		extcon_unregister_notifier(info->otg.cable, EXTCON_USB_HOST,
>> -					&info->otg.id_nb);
>> -	power_supply_unregister(info->psy_usb);
>> -psy_reg_failed:
>> -	extcon_unregister_notifier(info->cable.edev, EXTCON_CHG_USB_SDP,
>> -					&info->cable.nb);
>> -	extcon_unregister_notifier(info->cable.edev, EXTCON_CHG_USB_CDP,
>> -					&info->cable.nb);
>> -	extcon_unregister_notifier(info->cable.edev, EXTCON_CHG_USB_DCP,
>> -					&info->cable.nb);
>> -	return ret;
>> -}
>> -
>> -static int axp288_charger_remove(struct platform_device *pdev)
>> -{
>> -	struct axp288_chrg_info *info =  dev_get_drvdata(&pdev->dev);
>> -
>> -	if (info->otg.cable)
>> -		extcon_unregister_notifier(info->otg.cable, EXTCON_USB_HOST,
>> -					&info->otg.id_nb);
>> -
>> -	extcon_unregister_notifier(info->cable.edev, EXTCON_CHG_USB_SDP,
>> -					&info->cable.nb);
>> -	extcon_unregister_notifier(info->cable.edev, EXTCON_CHG_USB_CDP,
>> -					&info->cable.nb);
>> -	extcon_unregister_notifier(info->cable.edev, EXTCON_CHG_USB_DCP,
>> -					&info->cable.nb);
>> -	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",
>>  	},
>>
>

Re: [PATCH 01/14] extcon: Add extcon_get_extcon_dev_by_cable_id function

[Chanwoo Choi]

Hi Hans,

On 2016년 12월 19일 09:07, Hans de Goede wrote:
> extcon_register_notifier() allows passing in a NULL pointer for the
> extcon_device, so that extcon consumers which want extcon events of a
> certain type, but do not know the extcon device name (e.g. because
> there are different implementation depending on the board), can still
> get such events.
> 
> But most drivers will also want to know the initial state of the cable.
> Rather then adding NULL edev argument support to extcon_get_state, which
> would require looking up the right edev each call, this commit allows
> drivers to get the first extcon device with a requested cable-id through
> a new extcon_get_extcon_dev_by_cable_id function.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/extcon/extcon.c | 24 ++++++++++++++++++++++++
>  include/linux/extcon.h  |  1 +
>  2 files changed, 25 insertions(+)
> 
> diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
> index 7829846..505c272 100644
> --- a/drivers/extcon/extcon.c
> +++ b/drivers/extcon/extcon.c
> @@ -890,6 +890,30 @@ struct extcon_dev *extcon_get_extcon_dev(const char *extcon_name)
>  EXPORT_SYMBOL_GPL(extcon_get_extcon_dev);
>  
>  /**
> + * extcon_get_extcon_dev_by_cable_id() - Get an extcon device by a cable id
> + * @id:		the unique id of each external connector in extcon enumeration.
> + */
> +struct extcon_dev *extcon_get_extcon_dev_by_cable_id(unsigned int id)
> +{
> +	struct extcon_dev *extd;
> +	int idx = -EINVAL;
> +
> +	mutex_lock(&extcon_dev_list_lock);
> +	list_for_each_entry(extd, &extcon_dev_list, entry) {
> +		idx = find_cable_index_by_id(extd, id);
> +		if (idx >= 0)
> +			break;
> +	}
> +	mutex_unlock(&extcon_dev_list_lock);
> +
> +	if (idx < 0)
> +		return NULL;
> +
> +	return extd;
> +}
> +EXPORT_SYMBOL_GPL(extcon_get_extcon_dev_by_cable_id);

This function do not guarantee the same operation on all of case.

For example,
The h/w board has multiple extcon provider which support the same external connector. When using this function, this function don't get the correct extcon instance. Just, this function returns the first extcon instance in the registered extcon list.

This function has the potential problem.

> +
> +/**
>   * extcon_register_notifier() - Register a notifiee to get notified by
>   *				any attach status changes from the extcon.
>   * @edev:	the extcon device that has the external connecotr.
> diff --git a/include/linux/extcon.h b/include/linux/extcon.h
> index b871c0c..51abda8 100644
> --- a/include/linux/extcon.h
> +++ b/include/linux/extcon.h
> @@ -230,6 +230,7 @@ extern int devm_extcon_dev_register(struct device *dev,
>  extern void devm_extcon_dev_unregister(struct device *dev,
>  				       struct extcon_dev *edev);
>  extern struct extcon_dev *extcon_get_extcon_dev(const char *extcon_name);
> +extern struct extcon_dev *extcon_get_extcon_dev_by_cable_id(unsigned int id);
>  
>  /*
>   * Following APIs control the memory of extcon device.
> 


-- 
Regards,
Chanwoo Choi

Re: [PATCH 08/14] power: supply: axp288_charger: Actually get and use the USB_HOST extcon device

[Chanwoo Choi]

Hi Hans,

On 2016년 12월 19일 09:07, Hans de Goede wrote:
> 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>
> ---
>  drivers/power/supply/axp288_charger.c | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/power/supply/axp288_charger.c b/drivers/power/supply/axp288_charger.c
> index 08a5dba..2b95db2 100644
> --- a/drivers/power/supply/axp288_charger.c
> +++ b/drivers/power/supply/axp288_charger.c
> @@ -833,6 +833,12 @@ static int axp288_charger_probe(struct platform_device *pdev)
>  		return -EPROBE_DEFER;
>  	}
>  
> +	info->otg.cable = extcon_get_extcon_dev_by_cable_id(EXTCON_USB_HOST);
> +	if (info->otg.cable == NULL) {
> +		dev_dbg(dev, "EXTCON_USB_HOST is not ready, probe deferred\n");
> +		return -EPROBE_DEFER;
> +	}
> +

I replied my opinion of extcon_get_extcon_dev_by_cable_id from patch1.

If you possible, I think that you better to use the id_table which
includes the correct name of extcon device which support the EXTCON_USB_HOST.

When changing the hardware design, I think you can add the new device_id
with new extcon device name.

>  	platform_set_drvdata(pdev, info);
>  	mutex_init(&info->lock);
>  
> @@ -867,12 +873,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(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_cable_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++) {
> 

-- 
Regards,
Chanwoo Choi

Re: [PATCH 01/14] extcon: Add extcon_get_extcon_dev_by_cable_id function

[Hans de Goede]

Hi,

On 19-12-16 11:12, Chanwoo Choi wrote:
> Hi Hans,
>
> On 2016년 12월 19일 09:07, Hans de Goede wrote:
>> extcon_register_notifier() allows passing in a NULL pointer for the
>> extcon_device, so that extcon consumers which want extcon events of a
>> certain type, but do not know the extcon device name (e.g. because
>> there are different implementation depending on the board), can still
>> get such events.
>>
>> But most drivers will also want to know the initial state of the cable.
>> Rather then adding NULL edev argument support to extcon_get_state, which
>> would require looking up the right edev each call, this commit allows
>> drivers to get the first extcon device with a requested cable-id through
>> a new extcon_get_extcon_dev_by_cable_id function.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>  drivers/extcon/extcon.c | 24 ++++++++++++++++++++++++
>>  include/linux/extcon.h  |  1 +
>>  2 files changed, 25 insertions(+)
>>
>> diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
>> index 7829846..505c272 100644
>> --- a/drivers/extcon/extcon.c
>> +++ b/drivers/extcon/extcon.c
>> @@ -890,6 +890,30 @@ struct extcon_dev *extcon_get_extcon_dev(const char *extcon_name)
>>  EXPORT_SYMBOL_GPL(extcon_get_extcon_dev);
>>
>>  /**
>> + * extcon_get_extcon_dev_by_cable_id() - Get an extcon device by a cable id
>> + * @id:		the unique id of each external connector in extcon enumeration.
>> + */
>> +struct extcon_dev *extcon_get_extcon_dev_by_cable_id(unsigned int id)
>> +{
>> +	struct extcon_dev *extd;
>> +	int idx = -EINVAL;
>> +
>> +	mutex_lock(&extcon_dev_list_lock);
>> +	list_for_each_entry(extd, &extcon_dev_list, entry) {
>> +		idx = find_cable_index_by_id(extd, id);
>> +		if (idx >= 0)
>> +			break;
>> +	}
>> +	mutex_unlock(&extcon_dev_list_lock);
>> +
>> +	if (idx < 0)
>> +		return NULL;
>> +
>> +	return extd;
>> +}
>> +EXPORT_SYMBOL_GPL(extcon_get_extcon_dev_by_cable_id);
>
> This function do not guarantee the same operation on all of case.
>
> For example,
> The h/w board has multiple extcon provider which support the same external connector. When using this function, this function don't get the correct extcon instance. Just, this function returns the first extcon instance in the registered extcon list.
>
> This function has the potential problem.

True, I wanted this function because I'm afraid that on some
boards using the axp288_charger.c driver the USB_HOST extcon
cable may be provided by a different extcon device then the
"intel-int3496" device, but as you said in your reply to

"[PATCH 08/14] power: supply: axp288_charger: Actually get and use the USB_HOST extcon device"

If that happens we can add an array of extcon names to try,
in axp288_charger.c.

So I'll modify this patch to use the existing
extcon_get_extcon_dev with a name argument of "intel-int3496".

Note that currently extcon_register_notifier accepts a NULL
edev argument and in that case does pick the first edev with
a matching cable-id, which has the same problem as you pointed
out. So perhaps see if anyone actually passes in NULL, and if
not drop support for a NULL edev argument passed to
extcon_register_notifier ?

Regards,

Hans


>
>> +
>> +/**
>>   * extcon_register_notifier() - Register a notifiee to get notified by
>>   *				any attach status changes from the extcon.
>>   * @edev:	the extcon device that has the external connecotr.
>> diff --git a/include/linux/extcon.h b/include/linux/extcon.h
>> index b871c0c..51abda8 100644
>> --- a/include/linux/extcon.h
>> +++ b/include/linux/extcon.h
>> @@ -230,6 +230,7 @@ extern int devm_extcon_dev_register(struct device *dev,
>>  extern void devm_extcon_dev_unregister(struct device *dev,
>>  				       struct extcon_dev *edev);
>>  extern struct extcon_dev *extcon_get_extcon_dev(const char *extcon_name);
>> +extern struct extcon_dev *extcon_get_extcon_dev_by_cable_id(unsigned int id);
>>
>>  /*
>>   * Following APIs control the memory of extcon device.
>>
>
>

Re: [PATCH 01/14] extcon: Add extcon_get_extcon_dev_by_cable_id function

[Chanwoo Choi]

Hi,

On 2016년 12월 19일 20:42, Hans de Goede wrote:
> Hi,
> 
> On 19-12-16 11:12, Chanwoo Choi wrote:
>> Hi Hans,
>>
>> On 2016년 12월 19일 09:07, Hans de Goede wrote:
>>> extcon_register_notifier() allows passing in a NULL pointer for the
>>> extcon_device, so that extcon consumers which want extcon events of a
>>> certain type, but do not know the extcon device name (e.g. because
>>> there are different implementation depending on the board), can still
>>> get such events.
>>>
>>> But most drivers will also want to know the initial state of the cable.
>>> Rather then adding NULL edev argument support to extcon_get_state, which
>>> would require looking up the right edev each call, this commit allows
>>> drivers to get the first extcon device with a requested cable-id through
>>> a new extcon_get_extcon_dev_by_cable_id function.
>>>
>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>> ---
>>>  drivers/extcon/extcon.c | 24 ++++++++++++++++++++++++
>>>  include/linux/extcon.h  |  1 +
>>>  2 files changed, 25 insertions(+)
>>>
>>> diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
>>> index 7829846..505c272 100644
>>> --- a/drivers/extcon/extcon.c
>>> +++ b/drivers/extcon/extcon.c
>>> @@ -890,6 +890,30 @@ struct extcon_dev *extcon_get_extcon_dev(const char *extcon_name)
>>>  EXPORT_SYMBOL_GPL(extcon_get_extcon_dev);
>>>
>>>  /**
>>> + * extcon_get_extcon_dev_by_cable_id() - Get an extcon device by a cable id
>>> + * @id:        the unique id of each external connector in extcon enumeration.
>>> + */
>>> +struct extcon_dev *extcon_get_extcon_dev_by_cable_id(unsigned int id)
>>> +{
>>> +    struct extcon_dev *extd;
>>> +    int idx = -EINVAL;
>>> +
>>> +    mutex_lock(&extcon_dev_list_lock);
>>> +    list_for_each_entry(extd, &extcon_dev_list, entry) {
>>> +        idx = find_cable_index_by_id(extd, id);
>>> +        if (idx >= 0)
>>> +            break;
>>> +    }
>>> +    mutex_unlock(&extcon_dev_list_lock);
>>> +
>>> +    if (idx < 0)
>>> +        return NULL;
>>> +
>>> +    return extd;
>>> +}
>>> +EXPORT_SYMBOL_GPL(extcon_get_extcon_dev_by_cable_id);
>>
>> This function do not guarantee the same operation on all of case.
>>
>> For example,
>> The h/w board has multiple extcon provider which support the same external connector. When using this function, this function don't get the correct extcon instance. Just, this function returns the first extcon instance in the registered extcon list.
>>
>> This function has the potential problem.
> 
> True, I wanted this function because I'm afraid that on some
> boards using the axp288_charger.c driver the USB_HOST extcon
> cable may be provided by a different extcon device then the
> "intel-int3496" device, but as you said in your reply to
> 
> "[PATCH 08/14] power: supply: axp288_charger: Actually get and use the USB_HOST extcon device"
> 
> If that happens we can add an array of extcon names to try,
> in axp288_charger.c.
> 
> So I'll modify this patch to use the existing
> extcon_get_extcon_dev with a name argument of "intel-int3496".
> 
> Note that currently extcon_register_notifier accepts a NULL
> edev argument and in that case does pick the first edev with
> a matching cable-id, which has the same problem as you pointed
> out. So perhaps see if anyone actually passes in NULL, and if
> not drop support for a NULL edev argument passed to
> extcon_register_notifier ?

Right. To remove the potential problem,
I'll remove the code in the extcon_register_notfier() function when first parameter is NULL.

-- 
Regards,
Chanwoo Choi