[PATCH 1/8] extcon: axp288: Remove dependency on non-existing platform_data

[PATCH 1/8] extcon: axp288: Remove dependency on non-existing platform_data

[Hans de Goede]

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

However such another driver was never merged, so the extcon_axp288 as
merged upstream has never worked, its probe method simply always returns
-ENODEV.

This commit drops the dependency on the pdata always being there, instead
it treats not having pdata as the pdata having a NULL gpio_mux_control,
something which the code was already prepared to handle.

Note that the code for controlling the mux_control gpio is left in place,
as this may be necessary to allow the axp288 pmic to properly detect the
charger type (instead of assuming 500mA max charge current) on some
tablets. This will make it easier for future patches to add support for
this gpio by getting the gpio info from somewhere.

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

diff --git a/drivers/extcon/extcon-axp288.c b/drivers/extcon/extcon-axp288.c
index 42f41e8..a84fab8 100644
--- a/drivers/extcon/extcon-axp288.c
+++ b/drivers/extcon/extcon-axp288.c
@@ -112,7 +112,7 @@ struct axp288_extcon_info {
 	struct device *dev;
 	struct regmap *regmap;
 	struct regmap_irq_chip_data *regmap_irqc;
-	struct axp288_extcon_pdata *pdata;
+	struct gpio_desc *gpio_mux_cntl;
 	int irq[EXTCON_IRQ_END];
 	struct extcon_dev *edev;
 	struct notifier_block extcon_nb;
@@ -216,8 +216,8 @@ static int axp288_handle_chrg_det_event(struct axp288_extcon_info *info)
 		 * If VBUS is absent Connect D+/D- lines to PMIC for BC
 		 * detection. Else connect them to SOC for USB communication.
 		 */
-		if (info->pdata->gpio_mux_cntl)
-			gpiod_set_value(info->pdata->gpio_mux_cntl,
+		if (info->gpio_mux_cntl)
+			gpiod_set_value(info->gpio_mux_cntl,
 				vbus_attach ? EXTCON_GPIO_MUX_SEL_SOC
 						: EXTCON_GPIO_MUX_SEL_PMIC);
 
@@ -271,6 +271,7 @@ static int axp288_extcon_probe(struct platform_device *pdev)
 {
 	struct axp288_extcon_info *info;
 	struct axp20x_dev *axp20x = dev_get_drvdata(pdev->dev.parent);
+	struct axp288_extcon_pdata *pdata = pdev->dev.platform_data;
 	int ret, i, pirq, gpio;
 
 	info = devm_kzalloc(&pdev->dev, sizeof(*info), GFP_KERNEL);
@@ -280,15 +281,9 @@ static int axp288_extcon_probe(struct platform_device *pdev)
 	info->dev = &pdev->dev;
 	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_extcon_data\n"))
-			dev_err(&pdev->dev, "failed to get platform data\n");
-		return -ENODEV;
-	}
+	if (pdata)
+		info->gpio_mux_cntl = pdata->gpio_mux_cntl;
+
 	platform_set_drvdata(pdev, info);
 
 	axp288_extcon_log_rsi(info);
@@ -316,15 +311,15 @@ static int axp288_extcon_probe(struct platform_device *pdev)
 	}
 
 	/* Set up gpio control for USB Mux */
-	if (info->pdata->gpio_mux_cntl) {
-		gpio = desc_to_gpio(info->pdata->gpio_mux_cntl);
+	if (info->gpio_mux_cntl) {
+		gpio = desc_to_gpio(info->gpio_mux_cntl);
 		ret = devm_gpio_request(&pdev->dev, gpio, "USB_MUX");
 		if (ret < 0) {
 			dev_err(&pdev->dev,
 				"failed to request the gpio=%d\n", gpio);
 			return ret;
 		}
-		gpiod_direction_output(info->pdata->gpio_mux_cntl,
+		gpiod_direction_output(info->gpio_mux_cntl,
 						EXTCON_GPIO_MUX_SEL_PMIC);
 	}
 
-- 
2.9.3

[PATCH 2/8] extcon: axp288: Remove usb_phy notification code

[Hans de Goede]

The usb_phy based intel-usb-phy code never got merged into the
mainline kernel, so the devm_usb_get_phy() call will always fail,
blocking the driver from loading.

Since new drivers should use the generic-phy framework, not the
old-style usb_phy stuff, keeping this around is not useful.

Therefor this patch removes the usb_phy notification bits, which together
with the patch to remove the platform_data dependency, makes this driver
actually successfully probe on systems with an axp288 pmic.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/extcon/extcon-axp288.c | 12 ------------
 1 file changed, 12 deletions(-)

diff --git a/drivers/extcon/extcon-axp288.c b/drivers/extcon/extcon-axp288.c
index a84fab8..3d5e84e 100644
--- a/drivers/extcon/extcon-axp288.c
+++ b/drivers/extcon/extcon-axp288.c
@@ -21,7 +21,6 @@
 #include <linux/interrupt.h>
 #include <linux/platform_device.h>
 #include <linux/property.h>
-#include <linux/usb/phy.h>
 #include <linux/notifier.h>
 #include <linux/extcon.h>
 #include <linux/regmap.h>
@@ -116,7 +115,6 @@ struct axp288_extcon_info {
 	int irq[EXTCON_IRQ_END];
 	struct extcon_dev *edev;
 	struct notifier_block extcon_nb;
-	struct usb_phy *otg;
 };
 
 /* Power up/down reason string array */
@@ -220,9 +218,6 @@ static int axp288_handle_chrg_det_event(struct axp288_extcon_info *info)
 			gpiod_set_value(info->gpio_mux_cntl,
 				vbus_attach ? EXTCON_GPIO_MUX_SEL_SOC
 						: EXTCON_GPIO_MUX_SEL_PMIC);
-
-		atomic_notifier_call_chain(&info->otg->notifier,
-			vbus_attach ? USB_EVENT_VBUS : USB_EVENT_NONE, NULL);
 	}
 
 	if (notify_charger)
@@ -303,13 +298,6 @@ static int axp288_extcon_probe(struct platform_device *pdev)
 		return ret;
 	}
 
-	/* Get otg transceiver phy */
-	info->otg = devm_usb_get_phy(&pdev->dev, USB_PHY_TYPE_USB2);
-	if (IS_ERR(info->otg)) {
-		dev_err(&pdev->dev, "failed to get otg transceiver\n");
-		return PTR_ERR(info->otg);
-	}
-
 	/* Set up gpio control for USB Mux */
 	if (info->gpio_mux_cntl) {
 		gpio = desc_to_gpio(info->gpio_mux_cntl);
-- 
2.9.3

[PATCH 3/8] extcon: axp288: Simplify axp288_handle_chrg_det_event

[Hans de Goede]

axp288_handle_chrg_det_event only gets called on change interrupts
(so not that often), extcon_set_state_sync() checks itself if there are
any actual changes before notifying listeners, and gpiod_set_value is
not really expensive either.

So we can simply always do both on each interrupt removing a bunch of
somewhat magic looking code from axp288_handle_chrg_det_event.

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

diff --git a/drivers/extcon/extcon-axp288.c b/drivers/extcon/extcon-axp288.c
index 3d5e84e..43b3637 100644
--- a/drivers/extcon/extcon-axp288.c
+++ b/drivers/extcon/extcon-axp288.c
@@ -154,7 +154,6 @@ static void axp288_extcon_log_rsi(struct axp288_extcon_info *info)
 
 static int axp288_handle_chrg_det_event(struct axp288_extcon_info *info)
 {
-	static bool notify_otg, notify_charger;
 	static unsigned int cable;
 	int ret, stat, cfg, pwr_stat;
 	u8 chrg_type;
@@ -168,7 +167,7 @@ static int axp288_handle_chrg_det_event(struct axp288_extcon_info *info)
 
 	vbus_attach = (pwr_stat & PS_STAT_VBUS_PRESENT);
 	if (!vbus_attach)
-		goto notify_otg;
+		goto no_vbus;
 
 	/* Check charger detection completion status */
 	ret = regmap_read(info->regmap, AXP288_BC_GLOBAL_REG, &cfg);
@@ -188,19 +187,14 @@ static int axp288_handle_chrg_det_event(struct axp288_extcon_info *info)
 	switch (chrg_type) {
 	case DET_STAT_SDP:
 		dev_dbg(info->dev, "sdp cable is connected\n");
-		notify_otg = true;
-		notify_charger = true;
 		cable = EXTCON_CHG_USB_SDP;
 		break;
 	case DET_STAT_CDP:
 		dev_dbg(info->dev, "cdp cable is connected\n");
-		notify_otg = true;
-		notify_charger = true;
 		cable = EXTCON_CHG_USB_CDP;
 		break;
 	case DET_STAT_DCP:
 		dev_dbg(info->dev, "dcp cable is connected\n");
-		notify_charger = true;
 		cable = EXTCON_CHG_USB_DCP;
 		break;
 	default:
@@ -208,24 +202,17 @@ static int axp288_handle_chrg_det_event(struct axp288_extcon_info *info)
 			"disconnect or unknown or ID event\n");
 	}
 
-notify_otg:
-	if (notify_otg) {
-		/*
-		 * If VBUS is absent Connect D+/D- lines to PMIC for BC
-		 * detection. Else connect them to SOC for USB communication.
-		 */
-		if (info->gpio_mux_cntl)
-			gpiod_set_value(info->gpio_mux_cntl,
-				vbus_attach ? EXTCON_GPIO_MUX_SEL_SOC
-						: EXTCON_GPIO_MUX_SEL_PMIC);
-	}
-
-	if (notify_charger)
-		extcon_set_state_sync(info->edev, cable, vbus_attach);
-
-	/* Clear the flags on disconnect event */
-	if (!vbus_attach)
-		notify_otg = notify_charger = false;
+no_vbus:
+	/*
+	 * If VBUS is absent Connect D+/D- lines to PMIC for BC
+	 * detection. Else connect them to SOC for USB communication.
+	 */
+	if (info->gpio_mux_cntl)
+		gpiod_set_value(info->gpio_mux_cntl,
+			vbus_attach ? EXTCON_GPIO_MUX_SEL_SOC
+					: EXTCON_GPIO_MUX_SEL_PMIC);
+
+	extcon_set_state_sync(info->edev, cable, vbus_attach);
 
 	return 0;
 
-- 
2.9.3

[PATCH 4/8] extcon: axp288: Fix possibly reporting 2 cables in state true

[Hans de Goede]

When the charger type changes from e.g. SDP to CDP, without Vbus being
seen as low in between axp288_handle_chrg_det_event would set the state
for the new cable type to true, without clearing the state of the
previous cable type to false.

This commit fixes this and also gets rid of the function local static
cable variable, properly storing all drv state in the axp288_extcon_info
struct.

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

diff --git a/drivers/extcon/extcon-axp288.c b/drivers/extcon/extcon-axp288.c
index 43b3637..ded0bd9 100644
--- a/drivers/extcon/extcon-axp288.c
+++ b/drivers/extcon/extcon-axp288.c
@@ -115,6 +115,7 @@ struct axp288_extcon_info {
 	int irq[EXTCON_IRQ_END];
 	struct extcon_dev *edev;
 	struct notifier_block extcon_nb;
+	unsigned int previous_cable;
 };
 
 /* Power up/down reason string array */
@@ -154,9 +155,9 @@ static void axp288_extcon_log_rsi(struct axp288_extcon_info *info)
 
 static int axp288_handle_chrg_det_event(struct axp288_extcon_info *info)
 {
-	static unsigned int cable;
 	int ret, stat, cfg, pwr_stat;
 	u8 chrg_type;
+	unsigned int cable = info->previous_cable;
 	bool vbus_attach = false;
 
 	ret = regmap_read(info->regmap, AXP288_PS_STAT_REG, &pwr_stat);
@@ -212,7 +213,11 @@ static int axp288_handle_chrg_det_event(struct axp288_extcon_info *info)
 			vbus_attach ? EXTCON_GPIO_MUX_SEL_SOC
 					: EXTCON_GPIO_MUX_SEL_PMIC);
 
-	extcon_set_state_sync(info->edev, cable, vbus_attach);
+	extcon_set_state_sync(info->edev, info->previous_cable, false);
+	if (vbus_attach) {
+		extcon_set_state_sync(info->edev, cable, vbus_attach);
+		info->previous_cable = cable;
+	}
 
 	return 0;
 
@@ -263,6 +268,7 @@ static int axp288_extcon_probe(struct platform_device *pdev)
 	info->dev = &pdev->dev;
 	info->regmap = axp20x->regmap;
 	info->regmap_irqc = axp20x->regmap_irqc;
+	info->previous_cable = axp288_extcon_cables[0];
 	if (pdata)
 		info->gpio_mux_cntl = pdata->gpio_mux_cntl;
 
-- 
2.9.3

[PATCH 5/8] extcon: axp288: Make a couple of messages dev_info instead of dev_dbg

[Hans de Goede]

These messages are helpful for the user to check if their charger is
correctly detected, so make them dev_dbg instead of dev_info.

Also add a new message to indicate when the vbus is disconnected /
no cable is detected.

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

diff --git a/drivers/extcon/extcon-axp288.c b/drivers/extcon/extcon-axp288.c
index ded0bd9..fc636f6 100644
--- a/drivers/extcon/extcon-axp288.c
+++ b/drivers/extcon/extcon-axp288.c
@@ -167,8 +167,10 @@ static int axp288_handle_chrg_det_event(struct axp288_extcon_info *info)
 	}
 
 	vbus_attach = (pwr_stat & PS_STAT_VBUS_PRESENT);
-	if (!vbus_attach)
+	if (!vbus_attach) {
+		dev_info(info->dev, "vbus/cable disconnected\n");
 		goto no_vbus;
+	}
 
 	/* Check charger detection completion status */
 	ret = regmap_read(info->regmap, AXP288_BC_GLOBAL_REG, &cfg);
@@ -187,15 +189,15 @@ static int axp288_handle_chrg_det_event(struct axp288_extcon_info *info)
 
 	switch (chrg_type) {
 	case DET_STAT_SDP:
-		dev_dbg(info->dev, "sdp cable is connected\n");
+		dev_info(info->dev, "sdp cable is connected\n");
 		cable = EXTCON_CHG_USB_SDP;
 		break;
 	case DET_STAT_CDP:
-		dev_dbg(info->dev, "cdp cable is connected\n");
+		dev_info(info->dev, "cdp cable is connected\n");
 		cable = EXTCON_CHG_USB_CDP;
 		break;
 	case DET_STAT_DCP:
-		dev_dbg(info->dev, "dcp cable is connected\n");
+		dev_info(info->dev, "dcp cable is connected\n");
 		cable = EXTCON_CHG_USB_DCP;
 		break;
 	default:
-- 
2.9.3

[PATCH 6/8] extcon: axp288: Use vbus-valid instead of -present to determine cable presence

[Hans de Goede]

The vbus-present bit in the power status register also gets set to 1
when a usb-host cable (id-pin shorted to ground) is plugged in and a 5v
boost converter is supplying 5v to the otg usb bus.

This causes a "disconnect or unknown or ID event" warning in dmesg as
well as the extcon device to report the last detected charger cable
type as being connected even though none is connected.

This commit switches to checking the vbus-valid bit instead, which is
only 1 when both vbus is present and the vbus-path is enabled in the
vbus-path control register (the vbus-path gets disabled when a usb-host
cable is detected, to avoid the pmic drawing power from the 5v boost
converter).

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

diff --git a/drivers/extcon/extcon-axp288.c b/drivers/extcon/extcon-axp288.c
index fc636f6..7aec413 100644
--- a/drivers/extcon/extcon-axp288.c
+++ b/drivers/extcon/extcon-axp288.c
@@ -166,7 +166,7 @@ static int axp288_handle_chrg_det_event(struct axp288_extcon_info *info)
 		return ret;
 	}
 
-	vbus_attach = (pwr_stat & PS_STAT_VBUS_PRESENT);
+	vbus_attach = (pwr_stat & PS_STAT_VBUS_VALID);
 	if (!vbus_attach) {
 		dev_info(info->dev, "vbus/cable disconnected\n");
 		goto no_vbus;
-- 
2.9.3

[PATCH 7/8] extcon: axp288: 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/extcon/extcon-axp288.c | 19 +++----------------
 1 file changed, 3 insertions(+), 16 deletions(-)

diff --git a/drivers/extcon/extcon-axp288.c b/drivers/extcon/extcon-axp288.c
index 7aec413..a27ee68 100644
--- a/drivers/extcon/extcon-axp288.c
+++ b/drivers/extcon/extcon-axp288.c
@@ -70,12 +70,6 @@
 #define DET_STAT_CDP			2
 #define DET_STAT_DCP			3
 
-/* IRQ enable-1 register */
-#define PWRSRC_IRQ_CFG_MASK		(BIT(4)|BIT(3)|BIT(2))
-
-/* IRQ enable-6 register */
-#define BC12_IRQ_CFG_MASK		BIT(1)
-
 enum axp288_extcon_reg {
 	AXP288_PS_STAT_REG		= 0x00,
 	AXP288_PS_BOOT_REASON_REG	= 0x02,
@@ -83,8 +77,6 @@ enum axp288_extcon_reg {
 	AXP288_BC_VBUS_CNTL_REG		= 0x2d,
 	AXP288_BC_USB_STAT_REG		= 0x2e,
 	AXP288_BC_DET_STAT_REG		= 0x2f,
-	AXP288_PWRSRC_IRQ_CFG_REG	= 0x40,
-	AXP288_BC12_IRQ_CFG_REG		= 0x45,
 };
 
 enum axp288_mux_select {
@@ -242,15 +234,10 @@ static irqreturn_t axp288_extcon_isr(int irq, void *data)
 	return IRQ_HANDLED;
 }
 
-static void axp288_extcon_enable_irq(struct axp288_extcon_info *info)
+static void axp288_extcon_enable(struct axp288_extcon_info *info)
 {
-	/* Unmask VBUS interrupt */
-	regmap_write(info->regmap, AXP288_PWRSRC_IRQ_CFG_REG,
-						PWRSRC_IRQ_CFG_MASK);
 	regmap_update_bits(info->regmap, AXP288_BC_GLOBAL_REG,
 						BC_GLOBAL_RUN, 0);
-	/* Unmask the BC1.2 complete interrupts */
-	regmap_write(info->regmap, AXP288_BC12_IRQ_CFG_REG, BC12_IRQ_CFG_MASK);
 	/* Enable the charger detection logic */
 	regmap_update_bits(info->regmap, AXP288_BC_GLOBAL_REG,
 					BC_GLOBAL_RUN, BC_GLOBAL_RUN);
@@ -327,8 +314,8 @@ static int axp288_extcon_probe(struct platform_device *pdev)
 		}
 	}
 
-	/* Enable interrupts */
-	axp288_extcon_enable_irq(info);
+	/* Start charger cable type detection */
+	axp288_extcon_enable(info);
 
 	return 0;
 }
-- 
2.9.3

[PATCH 8/8] extcon: axp288: 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/extcon/extcon-axp288.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/extcon/extcon-axp288.c b/drivers/extcon/extcon-axp288.c
index a27ee68..509a5f9 100644
--- a/drivers/extcon/extcon-axp288.c
+++ b/drivers/extcon/extcon-axp288.c
@@ -320,8 +320,15 @@ static int axp288_extcon_probe(struct platform_device *pdev)
 	return 0;
 }
 
+static const struct platform_device_id axp288_extcon_table[] = {
+	{ .name = "axp288_extcon" },
+	{},
+};
+MODULE_DEVICE_TABLE(platform, axp288_extcon_table);
+
 static struct platform_driver axp288_extcon_driver = {
 	.probe = axp288_extcon_probe,
+	.id_table = axp288_extcon_table,
 	.driver = {
 		.name = "axp288_extcon",
 	},
-- 
2.9.3

Re: [PATCH 1/8] extcon: axp288: Remove dependency on non-existing platform_data

[Chanwoo Choi]

Hi Hans,

On 2016년 12월 19일 09:13, Hans de Goede wrote:
> When the extcon_axp288 driver was originally merged, it was merged with
> a dependency on some other driver providing platform data for it.
> 
> However such another driver was never merged, so the extcon_axp288 as
> merged upstream has never worked, its probe method simply always returns
> -ENODEV.
> 
> This commit drops the dependency on the pdata always being there, instead
> it treats not having pdata as the pdata having a NULL gpio_mux_control,
> something which the code was already prepared to handle.
> 
> Note that the code for controlling the mux_control gpio is left in place,
> as this may be necessary to allow the axp288 pmic to properly detect the
> charger type (instead of assuming 500mA max charge current) on some
> tablets. This will make it easier for future patches to add support for
> this gpio by getting the gpio info from somewhere.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/extcon/extcon-axp288.c | 25 ++++++++++---------------
>  1 file changed, 10 insertions(+), 15 deletions(-)

Looks good to me.
Acked-by: Chanwoo Choi <cw00.choi@samsung.com>

-- 
Regards,
Chanwoo Choi

Re: [PATCH 3/8] extcon: axp288: Simplify axp288_handle_chrg_det_event

[Chanwoo Choi]

Hi Hans,

On 2016년 12월 19일 09:13, Hans de Goede wrote:
> axp288_handle_chrg_det_event only gets called on change interrupts
> (so not that often), extcon_set_state_sync() checks itself if there are
> any actual changes before notifying listeners, and gpiod_set_value is
> not really expensive either.
> 
> So we can simply always do both on each interrupt removing a bunch of
> somewhat magic looking code from axp288_handle_chrg_det_event.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/extcon/extcon-axp288.c | 37 ++++++++++++-------------------------
>  1 file changed, 12 insertions(+), 25 deletions(-)

Looks good to me.
Acked-by: Chanwoo Choi <cw00.choi@samsung.com>

-- 
Regards,
Chanwoo Choi

Re: [PATCH 4/8] extcon: axp288: Fix possibly reporting 2 cables in state true

[Chanwoo Choi]

Hi Hans,

This patch looks good to me. But I have one comment
when setting the previous_cable in probe().

On 2016년 12월 19일 09:13, Hans de Goede wrote:
> When the charger type changes from e.g. SDP to CDP, without Vbus being
> seen as low in between axp288_handle_chrg_det_event would set the state
> for the new cable type to true, without clearing the state of the
> previous cable type to false.
> 
> This commit fixes this and also gets rid of the function local static
> cable variable, properly storing all drv state in the axp288_extcon_info
> struct.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/extcon/extcon-axp288.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/extcon/extcon-axp288.c b/drivers/extcon/extcon-axp288.c
> index 43b3637..ded0bd9 100644
> --- a/drivers/extcon/extcon-axp288.c
> +++ b/drivers/extcon/extcon-axp288.c
> @@ -115,6 +115,7 @@ struct axp288_extcon_info {
>  	int irq[EXTCON_IRQ_END];
>  	struct extcon_dev *edev;
>  	struct notifier_block extcon_nb;
> +	unsigned int previous_cable;
>  };
>  
>  /* Power up/down reason string array */
> @@ -154,9 +155,9 @@ static void axp288_extcon_log_rsi(struct axp288_extcon_info *info)
>  
>  static int axp288_handle_chrg_det_event(struct axp288_extcon_info *info)
>  {
> -	static unsigned int cable;
>  	int ret, stat, cfg, pwr_stat;
>  	u8 chrg_type;
> +	unsigned int cable = info->previous_cable;
>  	bool vbus_attach = false;
>  
>  	ret = regmap_read(info->regmap, AXP288_PS_STAT_REG, &pwr_stat);
> @@ -212,7 +213,11 @@ static int axp288_handle_chrg_det_event(struct axp288_extcon_info *info)
>  			vbus_attach ? EXTCON_GPIO_MUX_SEL_SOC
>  					: EXTCON_GPIO_MUX_SEL_PMIC);
>  
> -	extcon_set_state_sync(info->edev, cable, vbus_attach);
> +	extcon_set_state_sync(info->edev, info->previous_cable, false);
> +	if (vbus_attach) {
> +		extcon_set_state_sync(info->edev, cable, vbus_attach);
> +		info->previous_cable = cable;
> +	}
>  
>  	return 0;
>  
> @@ -263,6 +268,7 @@ static int axp288_extcon_probe(struct platform_device *pdev)
>  	info->dev = &pdev->dev;
>  	info->regmap = axp20x->regmap;
>  	info->regmap_irqc = axp20x->regmap_irqc;
> +	info->previous_cable = axp288_extcon_cables[0];

I think that you better to use "EXTCON_NONE" instead of
"axp288_extcon_cables[0]" as following:
	info->previous_cable = EXTCON_NONE;


>  	if (pdata)
>  		info->gpio_mux_cntl = pdata->gpio_mux_cntl;
>  
> 

-- 
Regards,
Chanwoo Choi

Re: [PATCH 5/8] extcon: axp288: Make a couple of messages dev_info instead of dev_dbg

[Chanwoo Choi]

Hi Hans,

I prefer to use the dev_dbg on the fly instead of dev_info.
If you want to check the change state, you can use the udev monitor tool
because extcon send the uevent when changing the state of connector.

Regards,
Chanwoo Choi

On 2016년 12월 19일 09:13, Hans de Goede wrote:
> These messages are helpful for the user to check if their charger is
> correctly detected, so make them dev_dbg instead of dev_info.
> 
> Also add a new message to indicate when the vbus is disconnected /
> no cable is detected.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/extcon/extcon-axp288.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/extcon/extcon-axp288.c b/drivers/extcon/extcon-axp288.c
> index ded0bd9..fc636f6 100644
> --- a/drivers/extcon/extcon-axp288.c
> +++ b/drivers/extcon/extcon-axp288.c
> @@ -167,8 +167,10 @@ static int axp288_handle_chrg_det_event(struct axp288_extcon_info *info)
>  	}
>  
>  	vbus_attach = (pwr_stat & PS_STAT_VBUS_PRESENT);
> -	if (!vbus_attach)
> +	if (!vbus_attach) {
> +		dev_info(info->dev, "vbus/cable disconnected\n");
>  		goto no_vbus;
> +	}
>  
>  	/* Check charger detection completion status */
>  	ret = regmap_read(info->regmap, AXP288_BC_GLOBAL_REG, &cfg);
> @@ -187,15 +189,15 @@ static int axp288_handle_chrg_det_event(struct axp288_extcon_info *info)
>  
>  	switch (chrg_type) {
>  	case DET_STAT_SDP:
> -		dev_dbg(info->dev, "sdp cable is connected\n");
> +		dev_info(info->dev, "sdp cable is connected\n");
>  		cable = EXTCON_CHG_USB_SDP;
>  		break;
>  	case DET_STAT_CDP:
> -		dev_dbg(info->dev, "cdp cable is connected\n");
> +		dev_info(info->dev, "cdp cable is connected\n");
>  		cable = EXTCON_CHG_USB_CDP;
>  		break;
>  	case DET_STAT_DCP:
> -		dev_dbg(info->dev, "dcp cable is connected\n");
> +		dev_info(info->dev, "dcp cable is connected\n");
>  		cable = EXTCON_CHG_USB_DCP;
>  		break;
>  	default:
> 

Re: [PATCH 6/8] extcon: axp288: Use vbus-valid instead of -present to determine cable presence

[Chanwoo Choi]

Hi Hans,

On 2016년 12월 19일 09:13, Hans de Goede wrote:
> The vbus-present bit in the power status register also gets set to 1
> when a usb-host cable (id-pin shorted to ground) is plugged in and a 5v
> boost converter is supplying 5v to the otg usb bus.
> 
> This causes a "disconnect or unknown or ID event" warning in dmesg as
> well as the extcon device to report the last detected charger cable
> type as being connected even though none is connected.
> 
> This commit switches to checking the vbus-valid bit instead, which is
> only 1 when both vbus is present and the vbus-path is enabled in the
> vbus-path control register (the vbus-path gets disabled when a usb-host
> cable is detected, to avoid the pmic drawing power from the 5v boost
> converter).
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/extcon/extcon-axp288.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/extcon/extcon-axp288.c b/drivers/extcon/extcon-axp288.c
> index fc636f6..7aec413 100644
> --- a/drivers/extcon/extcon-axp288.c
> +++ b/drivers/extcon/extcon-axp288.c
> @@ -166,7 +166,7 @@ static int axp288_handle_chrg_det_event(struct axp288_extcon_info *info)
>  		return ret;
>  	}
>  
> -	vbus_attach = (pwr_stat & PS_STAT_VBUS_PRESENT);
> +	vbus_attach = (pwr_stat & PS_STAT_VBUS_VALID);
>  	if (!vbus_attach) {
>  		dev_info(info->dev, "vbus/cable disconnected\n");
>  		goto no_vbus;
> 

Acked-by: Chanwoo Choi <cw00.choi@samsung.com>

-- 
Regards,
Chanwoo Choi

Re: [PATCH 7/8] extcon: axp288: Remove unnecessary irq?_en register writes

[Chanwoo Choi]

Hi Hans,

On 2016년 12월 19일 09:13, Hans de Goede wrote:
> 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/extcon/extcon-axp288.c | 19 +++----------------
>  1 file changed, 3 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/extcon/extcon-axp288.c b/drivers/extcon/extcon-axp288.c
> index 7aec413..a27ee68 100644
> --- a/drivers/extcon/extcon-axp288.c
> +++ b/drivers/extcon/extcon-axp288.c
> @@ -70,12 +70,6 @@
>  #define DET_STAT_CDP			2
>  #define DET_STAT_DCP			3
>  
> -/* IRQ enable-1 register */
> -#define PWRSRC_IRQ_CFG_MASK		(BIT(4)|BIT(3)|BIT(2))
> -
> -/* IRQ enable-6 register */
> -#define BC12_IRQ_CFG_MASK		BIT(1)
> -
>  enum axp288_extcon_reg {
>  	AXP288_PS_STAT_REG		= 0x00,
>  	AXP288_PS_BOOT_REASON_REG	= 0x02,
> @@ -83,8 +77,6 @@ enum axp288_extcon_reg {
>  	AXP288_BC_VBUS_CNTL_REG		= 0x2d,
>  	AXP288_BC_USB_STAT_REG		= 0x2e,
>  	AXP288_BC_DET_STAT_REG		= 0x2f,
> -	AXP288_PWRSRC_IRQ_CFG_REG	= 0x40,
> -	AXP288_BC12_IRQ_CFG_REG		= 0x45,
>  };
>  
>  enum axp288_mux_select {
> @@ -242,15 +234,10 @@ static irqreturn_t axp288_extcon_isr(int irq, void *data)
>  	return IRQ_HANDLED;
>  }
>  
> -static void axp288_extcon_enable_irq(struct axp288_extcon_info *info)
> +static void axp288_extcon_enable(struct axp288_extcon_info *info)
>  {
> -	/* Unmask VBUS interrupt */
> -	regmap_write(info->regmap, AXP288_PWRSRC_IRQ_CFG_REG,
> -						PWRSRC_IRQ_CFG_MASK);
>  	regmap_update_bits(info->regmap, AXP288_BC_GLOBAL_REG,
>  						BC_GLOBAL_RUN, 0);
> -	/* Unmask the BC1.2 complete interrupts */
> -	regmap_write(info->regmap, AXP288_BC12_IRQ_CFG_REG, BC12_IRQ_CFG_MASK);
>  	/* Enable the charger detection logic */
>  	regmap_update_bits(info->regmap, AXP288_BC_GLOBAL_REG,
>  					BC_GLOBAL_RUN, BC_GLOBAL_RUN);
> @@ -327,8 +314,8 @@ static int axp288_extcon_probe(struct platform_device *pdev)
>  		}
>  	}
>  
> -	/* Enable interrupts */
> -	axp288_extcon_enable_irq(info);
> +	/* Start charger cable type detection */
> +	axp288_extcon_enable(info);
>  
>  	return 0;
>  }
> 

Looks good to me.
Acked-by: Chanwoo Choi <cw00.choi@samsung.com>

-- 
Regards,
Chanwoo Choi

Re: [PATCH 8/8] extcon: axp288: Fix the module not auto-loading

[Chanwoo Choi]

Hi Hans,

On 2016년 12월 19일 09:13, Hans de Goede wrote:
> Add a MODULE_DEVICE_TABLE to fix the module not auto-loading.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/extcon/extcon-axp288.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/extcon/extcon-axp288.c b/drivers/extcon/extcon-axp288.c
> index a27ee68..509a5f9 100644
> --- a/drivers/extcon/extcon-axp288.c
> +++ b/drivers/extcon/extcon-axp288.c
> @@ -320,8 +320,15 @@ static int axp288_extcon_probe(struct platform_device *pdev)
>  	return 0;
>  }
>  
> +static const struct platform_device_id axp288_extcon_table[] = {
> +	{ .name = "axp288_extcon" },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(platform, axp288_extcon_table);
> +
>  static struct platform_driver axp288_extcon_driver = {
>  	.probe = axp288_extcon_probe,
> +	.id_table = axp288_extcon_table,
>  	.driver = {
>  		.name = "axp288_extcon",
>  	},
> 

Looks good to me.
Acked-by: Chanwoo Choi <cw00.choi@samsung.com>

-- 
Regards,
Chanwoo Choi

Re: [PATCH 2/8] extcon: axp288: Remove usb_phy notification code

[Chanwoo Choi]

Hi Hans,

On 2016년 12월 19일 09:13, Hans de Goede wrote:
> The usb_phy based intel-usb-phy code never got merged into the
> mainline kernel, so the devm_usb_get_phy() call will always fail,
> blocking the driver from loading.
> 
> Since new drivers should use the generic-phy framework, not the
> old-style usb_phy stuff, keeping this around is not useful.
> 
> Therefor this patch removes the usb_phy notification bits, which together
> with the patch to remove the platform_data dependency, makes this driver
> actually successfully probe on systems with an axp288 pmic.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/extcon/extcon-axp288.c | 12 ------------
>  1 file changed, 12 deletions(-)
> 
> diff --git a/drivers/extcon/extcon-axp288.c b/drivers/extcon/extcon-axp288.c
> index a84fab8..3d5e84e 100644
> --- a/drivers/extcon/extcon-axp288.c
> +++ b/drivers/extcon/extcon-axp288.c
> @@ -21,7 +21,6 @@
>  #include <linux/interrupt.h>
>  #include <linux/platform_device.h>
>  #include <linux/property.h>
> -#include <linux/usb/phy.h>
>  #include <linux/notifier.h>
>  #include <linux/extcon.h>
>  #include <linux/regmap.h>
> @@ -116,7 +115,6 @@ struct axp288_extcon_info {
>  	int irq[EXTCON_IRQ_END];
>  	struct extcon_dev *edev;
>  	struct notifier_block extcon_nb;
> -	struct usb_phy *otg;
>  };
>  
>  /* Power up/down reason string array */
> @@ -220,9 +218,6 @@ static int axp288_handle_chrg_det_event(struct axp288_extcon_info *info)
>  			gpiod_set_value(info->gpio_mux_cntl,
>  				vbus_attach ? EXTCON_GPIO_MUX_SEL_SOC
>  						: EXTCON_GPIO_MUX_SEL_PMIC);
> -
> -		atomic_notifier_call_chain(&info->otg->notifier,
> -			vbus_attach ? USB_EVENT_VBUS : USB_EVENT_NONE, NULL);
>  	}
>  
>  	if (notify_charger)
> @@ -303,13 +298,6 @@ static int axp288_extcon_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>  
> -	/* Get otg transceiver phy */
> -	info->otg = devm_usb_get_phy(&pdev->dev, USB_PHY_TYPE_USB2);
> -	if (IS_ERR(info->otg)) {
> -		dev_err(&pdev->dev, "failed to get otg transceiver\n");
> -		return PTR_ERR(info->otg);
> -	}
> -
>  	/* Set up gpio control for USB Mux */
>  	if (info->gpio_mux_cntl) {
>  		gpio = desc_to_gpio(info->gpio_mux_cntl);
> 

Looks good to me.
Acked-by: Chanwoo Choi <cw00.choi@samsung.com>

-- 
Regards,
Chanwoo Choi

Re: [PATCH 1/8] extcon: axp288: Remove dependency on non-existing platform_data

[Chanwoo Choi]

Hi Hans,

This series look good to me. I added the comment
for patch4/5. If you agree following two comment, I'll merge these series for 4.11.

- patch4 uses the EXTCON_NONE when setting the previous_cable in probe()
- patch5, I don't want to use the dev_info on the fly. So, I want to drop the patch5.

Regards,
Chanwoo Choi

On 2016년 12월 19일 15:10, Chanwoo Choi wrote:
> Hi Hans,
> 
> On 2016년 12월 19일 09:13, Hans de Goede wrote:
>> When the extcon_axp288 driver was originally merged, it was merged with
>> a dependency on some other driver providing platform data for it.
>>
>> However such another driver was never merged, so the extcon_axp288 as
>> merged upstream has never worked, its probe method simply always returns
>> -ENODEV.
>>
>> This commit drops the dependency on the pdata always being there, instead
>> it treats not having pdata as the pdata having a NULL gpio_mux_control,
>> something which the code was already prepared to handle.
>>
>> Note that the code for controlling the mux_control gpio is left in place,
>> as this may be necessary to allow the axp288 pmic to properly detect the
>> charger type (instead of assuming 500mA max charge current) on some
>> tablets. This will make it easier for future patches to add support for
>> this gpio by getting the gpio info from somewhere.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>  drivers/extcon/extcon-axp288.c | 25 ++++++++++---------------
>>  1 file changed, 10 insertions(+), 15 deletions(-)
> 
> Looks good to me.
> Acked-by: Chanwoo Choi <cw00.choi@samsung.com>
> 

Re: [PATCH 1/8] extcon: axp288: Remove dependency on non-existing platform_data

[Hans de Goede]

Hi,

On 19-12-16 07:58, Chanwoo Choi wrote:
> Hi Hans,
>
> This series look good to me. I added the comment
> for patch4/5. If you agree following two comment, I'll merge these series for 4.11.
>
> - patch4 uses the EXTCON_NONE when setting the previous_cable in probe()
> - patch5, I don't want to use the dev_info on the fly. So, I want to drop the patch5.

Sounds good to me, thank you for reviewing these patches so quickly.

Regards,

Hans


>
> Regards,
> Chanwoo Choi
>
> On 2016년 12월 19일 15:10, Chanwoo Choi wrote:
>> Hi Hans,
>>
>> On 2016년 12월 19일 09:13, Hans de Goede wrote:
>>> When the extcon_axp288 driver was originally merged, it was merged with
>>> a dependency on some other driver providing platform data for it.
>>>
>>> However such another driver was never merged, so the extcon_axp288 as
>>> merged upstream has never worked, its probe method simply always returns
>>> -ENODEV.
>>>
>>> This commit drops the dependency on the pdata always being there, instead
>>> it treats not having pdata as the pdata having a NULL gpio_mux_control,
>>> something which the code was already prepared to handle.
>>>
>>> Note that the code for controlling the mux_control gpio is left in place,
>>> as this may be necessary to allow the axp288 pmic to properly detect the
>>> charger type (instead of assuming 500mA max charge current) on some
>>> tablets. This will make it easier for future patches to add support for
>>> this gpio by getting the gpio info from somewhere.
>>>
>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>> ---
>>>  drivers/extcon/extcon-axp288.c | 25 ++++++++++---------------
>>>  1 file changed, 10 insertions(+), 15 deletions(-)
>>
>> Looks good to me.
>> Acked-by: Chanwoo Choi <cw00.choi@samsung.com>
>>
>

Re: [PATCH 1/8] extcon: axp288: Remove dependency on non-existing platform_data

[Chanwoo Choi]

Hi Hans,

On 2016년 12월 19일 17:18, Hans de Goede wrote:
> Hi,
> 
> On 19-12-16 07:58, Chanwoo Choi wrote:
>> Hi Hans,
>>
>> This series look good to me. I added the comment
>> for patch4/5. If you agree following two comment, I'll merge these series for 4.11.
>>
>> - patch4 uses the EXTCON_NONE when setting the previous_cable in probe()
>> - patch5, I don't want to use the dev_info on the fly. So, I want to drop the patch5.
> 
> Sounds good to me, thank you for reviewing these patches so quickly.

Applied these series except for patch5 for 4.11.

Regards,
Chanwoo Choi

> 
> 
>>
>> Regards,
>> Chanwoo Choi
>>
>> On 2016년 12월 19일 15:10, Chanwoo Choi wrote:
>>> Hi Hans,
>>>
>>> On 2016년 12월 19일 09:13, Hans de Goede wrote:
>>>> When the extcon_axp288 driver was originally merged, it was merged with
>>>> a dependency on some other driver providing platform data for it.
>>>>
>>>> However such another driver was never merged, so the extcon_axp288 as
>>>> merged upstream has never worked, its probe method simply always returns
>>>> -ENODEV.
>>>>
>>>> This commit drops the dependency on the pdata always being there, instead
>>>> it treats not having pdata as the pdata having a NULL gpio_mux_control,
>>>> something which the code was already prepared to handle.
>>>>
>>>> Note that the code for controlling the mux_control gpio is left in place,
>>>> as this may be necessary to allow the axp288 pmic to properly detect the
>>>> charger type (instead of assuming 500mA max charge current) on some
>>>> tablets. This will make it easier for future patches to add support for
>>>> this gpio by getting the gpio info from somewhere.
>>>>
>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>> ---
>>>>  drivers/extcon/extcon-axp288.c | 25 ++++++++++---------------
>>>>  1 file changed, 10 insertions(+), 15 deletions(-)
>>>
>>> Looks good to me.
>>> Acked-by: Chanwoo Choi <cw00.choi@samsung.com>
>>>
>>
> 
> 
>