Re: [PATCH v3 1/2] mfd/axp20x: add support for extcon cell

Re: [PATCH v3 1/2] mfd/axp20x: add support for extcon cell

[Lee Jones]

When submitting patches, please observe the $SUBJECT line formatting
of the subsystem you're sending to.

You can do so by: `git log --oneline -- drivers/<subsystem>`

In the case of MFD it's:

mfd: <device>: <Description>

Note the ':' separators and the capitalisation of the first letter of
the description string.

> This patch adds the mfd cell info for axp288 extcon device.
> 
> Signed-off-by: Ramakrishna Pallala <ramakrishna.pallala@intel.com>
> ---
>  drivers/mfd/axp20x.c |   28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)

Nevertheless, patch applied, thanks.

> diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
> index b1b580a..4c8e0e9 100644
> --- a/drivers/mfd/axp20x.c
> +++ b/drivers/mfd/axp20x.c
> @@ -290,6 +290,29 @@ static struct resource axp288_adc_resources[] = {
>  	},
>  };
>  
> +static struct resource axp288_extcon_resources[] = {
> +	{
> +		.start = AXP288_IRQ_VBUS_FALL,
> +		.end   = AXP288_IRQ_VBUS_FALL,
> +		.flags = IORESOURCE_IRQ,
> +	},
> +	{
> +		.start = AXP288_IRQ_VBUS_RISE,
> +		.end   = AXP288_IRQ_VBUS_RISE,
> +		.flags = IORESOURCE_IRQ,
> +	},
> +	{
> +		.start = AXP288_IRQ_MV_CHNG,
> +		.end   = AXP288_IRQ_MV_CHNG,
> +		.flags = IORESOURCE_IRQ,
> +	},
> +	{
> +		.start = AXP288_IRQ_BC_USB_CHNG,
> +		.end   = AXP288_IRQ_BC_USB_CHNG,
> +		.flags = IORESOURCE_IRQ,
> +	},
> +};
> +
>  static struct resource axp288_charger_resources[] = {
>  	{
>  		.start = AXP288_IRQ_OV,
> @@ -345,6 +368,11 @@ static struct mfd_cell axp288_cells[] = {
>  		.resources = axp288_adc_resources,
>  	},
>  	{
> +		.name = "axp288_extcon",
> +		.num_resources = ARRAY_SIZE(axp288_extcon_resources),
> +		.resources = axp288_extcon_resources,
> +	},
> +	{
>  		.name = "axp288_charger",
>  		.num_resources = ARRAY_SIZE(axp288_charger_resources),
>  		.resources = axp288_charger_resources,

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

RE: [PATCH v3 1/2] mfd/axp20x: add support for extcon cell

[Pallala, Ramakrishna]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 846 bytes --]

> When submitting patches, please observe the $SUBJECT line formatting of the
> subsystem you're sending to.
> 
> You can do so by: `git log --oneline -- drivers/<subsystem>`
> 
> In the case of MFD it's:
> 
> mfd: <device>: <Description>
> 
> Note the ':' separators and the capitalisation of the first letter of the description
> string.
> 
> > This patch adds the mfd cell info for axp288 extcon device.
> >
> > Signed-off-by: Ramakrishna Pallala <ramakrishna.pallala@intel.com>
> > ---
> >  drivers/mfd/axp20x.c |   28 ++++++++++++++++++++++++++++
> >  1 file changed, 28 insertions(+)
> 
> Nevertheless, patch applied, thanks.

Thanks you. I will take care next time.

Thanks,
Ram
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

[PATCH v3 0/2] Add extcon support for AXP288 PMIC

[Ramakrishna Pallala]

This patch series adds the support for axp288 extcon driver
and also adds the cell info for extcon device in axp20x mfd driver.

Ramakrishna Pallala (2):
  mfd/axp20x: add support for extcon cell
  extcon-axp288: Add axp288 extcon driver support

 drivers/extcon/Kconfig         |    7 +
 drivers/extcon/Makefile        |    1 +
 drivers/extcon/extcon-axp288.c |  462 ++++++++++++++++++++++++++++++++++++++++
 drivers/mfd/axp20x.c           |   28 +++
 include/linux/mfd/axp20x.h     |    5 +
 5 files changed, 503 insertions(+)
 create mode 100644 drivers/extcon/extcon-axp288.c

-- 
1.7.9.5

[PATCH v3 1/2] mfd/axp20x: add support for extcon cell

[Ramakrishna Pallala]

This patch adds the mfd cell info for axp288 extcon device.

Signed-off-by: Ramakrishna Pallala <ramakrishna.pallala@intel.com>
---
 drivers/mfd/axp20x.c |   28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
index b1b580a..4c8e0e9 100644
--- a/drivers/mfd/axp20x.c
+++ b/drivers/mfd/axp20x.c
@@ -290,6 +290,29 @@ static struct resource axp288_adc_resources[] = {
 	},
 };
 
+static struct resource axp288_extcon_resources[] = {
+	{
+		.start = AXP288_IRQ_VBUS_FALL,
+		.end   = AXP288_IRQ_VBUS_FALL,
+		.flags = IORESOURCE_IRQ,
+	},
+	{
+		.start = AXP288_IRQ_VBUS_RISE,
+		.end   = AXP288_IRQ_VBUS_RISE,
+		.flags = IORESOURCE_IRQ,
+	},
+	{
+		.start = AXP288_IRQ_MV_CHNG,
+		.end   = AXP288_IRQ_MV_CHNG,
+		.flags = IORESOURCE_IRQ,
+	},
+	{
+		.start = AXP288_IRQ_BC_USB_CHNG,
+		.end   = AXP288_IRQ_BC_USB_CHNG,
+		.flags = IORESOURCE_IRQ,
+	},
+};
+
 static struct resource axp288_charger_resources[] = {
 	{
 		.start = AXP288_IRQ_OV,
@@ -345,6 +368,11 @@ static struct mfd_cell axp288_cells[] = {
 		.resources = axp288_adc_resources,
 	},
 	{
+		.name = "axp288_extcon",
+		.num_resources = ARRAY_SIZE(axp288_extcon_resources),
+		.resources = axp288_extcon_resources,
+	},
+	{
 		.name = "axp288_charger",
 		.num_resources = ARRAY_SIZE(axp288_charger_resources),
 		.resources = axp288_charger_resources,
-- 
1.7.9.5

[PATCH v3 2/2] extcon-axp288: Add axp288 extcon driver support

[Ramakrishna Pallala]

This patch adds the extcon support for AXP288 PMIC which
has the BC1.2 charger detection capability. Additionally
it also adds the USB mux switching support b/w SOC and PMIC
based on GPIO control.

Signed-off-by: Ramakrishna Pallala <ramakrishna.pallala@intel.com>
---
 drivers/extcon/Kconfig         |    7 +
 drivers/extcon/Makefile        |    1 +
 drivers/extcon/extcon-axp288.c |  462 ++++++++++++++++++++++++++++++++++++++++
 include/linux/mfd/axp20x.h     |    5 +
 4 files changed, 475 insertions(+)
 create mode 100644 drivers/extcon/extcon-axp288.c

diff --git a/drivers/extcon/Kconfig b/drivers/extcon/Kconfig
index 6a1f7de..f6e8b2a 100644
--- a/drivers/extcon/Kconfig
+++ b/drivers/extcon/Kconfig
@@ -28,6 +28,13 @@ config EXTCON_ARIZONA
 	  with Wolfson Arizona devices. These are audio CODECs with
 	  advanced audio accessory detection support.
 
+config EXTCON_AXP288
+	tristate "X-Power AXP288 EXTCON support"
+	depends on MFD_AXP20X && USB_PHY
+	help
+	  Say Y here to enable support for USB peripheral detection
+	  and USB MUX switching by X-Power AXP288 PMIC.
+
 config EXTCON_GPIO
 	tristate "GPIO extcon support"
 	depends on GPIOLIB
diff --git a/drivers/extcon/Makefile b/drivers/extcon/Makefile
index 0370b42..5429377 100644
--- a/drivers/extcon/Makefile
+++ b/drivers/extcon/Makefile
@@ -5,6 +5,7 @@
 obj-$(CONFIG_EXTCON)		+= extcon-class.o
 obj-$(CONFIG_EXTCON_ADC_JACK)	+= extcon-adc-jack.o
 obj-$(CONFIG_EXTCON_ARIZONA)	+= extcon-arizona.o
+obj-$(CONFIG_EXTCON_AXP288)	+= extcon-axp288.o
 obj-$(CONFIG_EXTCON_GPIO)	+= extcon-gpio.o
 obj-$(CONFIG_EXTCON_MAX14577)	+= extcon-max14577.o
 obj-$(CONFIG_EXTCON_MAX77693)	+= extcon-max77693.o
diff --git a/drivers/extcon/extcon-axp288.c b/drivers/extcon/extcon-axp288.c
new file mode 100644
index 0000000..363552c
--- /dev/null
+++ b/drivers/extcon/extcon-axp288.c
@@ -0,0 +1,462 @@
+/*
+ * extcon-axp288.c - X-Power AXP288 PMIC extcon cable detection driver
+ *
+ * Copyright (C) 2015 Intel Corporation
+ * Author: Ramakrishna Pallala <ramakrishna.pallala@intel.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/io.h>
+#include <linux/slab.h>
+#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>
+#include <linux/gpio.h>
+#include <linux/gpio/consumer.h>
+#include <linux/mfd/axp20x.h>
+
+/* Power source status register */
+#define PS_STAT_VBUS_TRIGGER		BIT(0)
+#define PS_STAT_BAT_CHRG_DIR		BIT(2)
+#define PS_STAT_VBUS_ABOVE_VHOLD	BIT(3)
+#define PS_STAT_VBUS_VALID		BIT(4)
+#define PS_STAT_VBUS_PRESENT		BIT(5)
+
+/* BC module global register */
+#define BC_GLOBAL_RUN			BIT(0)
+#define BC_GLOBAL_DET_STAT		BIT(2)
+#define BC_GLOBAL_DBP_TOUT		BIT(3)
+#define BC_GLOBAL_VLGC_COM_SEL		BIT(4)
+#define BC_GLOBAL_DCD_TOUT_MASK		(BIT(6)|BIT(5))
+#define BC_GLOBAL_DCD_TOUT_300MS	0
+#define BC_GLOBAL_DCD_TOUT_100MS	1
+#define BC_GLOBAL_DCD_TOUT_500MS	2
+#define BC_GLOBAL_DCD_TOUT_900MS	3
+#define BC_GLOBAL_DCD_DET_SEL		BIT(7)
+
+/* BC module vbus control and status register */
+#define VBUS_CNTL_DPDM_PD_EN		BIT(4)
+#define VBUS_CNTL_DPDM_FD_EN		BIT(5)
+#define VBUS_CNTL_FIRST_PO_STAT		BIT(6)
+
+/* BC USB status register */
+#define USB_STAT_BUS_STAT_MASK		(BIT(3)|BIT(2)|BIT(1)|BIT(0))
+#define USB_STAT_BUS_STAT_SHIFT		0
+#define USB_STAT_BUS_STAT_ATHD		0
+#define USB_STAT_BUS_STAT_CONN		1
+#define USB_STAT_BUS_STAT_SUSP		2
+#define USB_STAT_BUS_STAT_CONF		3
+#define USB_STAT_USB_SS_MODE		BIT(4)
+#define USB_STAT_DEAD_BAT_DET		BIT(6)
+#define USB_STAT_DBP_UNCFG		BIT(7)
+
+/* BC detect status register */
+#define DET_STAT_MASK			(BIT(7)|BIT(6)|BIT(5))
+#define DET_STAT_SHIFT			5
+#define DET_STAT_SDP			1
+#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,
+	AXP288_BC_GLOBAL_REG		= 0x2c,
+	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,
+};
+
+#define AXP288_DRV_NAME			"axp288_extcon"
+
+#define AXP288_EXTCON_CABLE_SDP		"Slow-charger"
+#define AXP288_EXTCON_CABLE_CDP		"Charge-downstream"
+#define AXP288_EXTCON_CABLE_DCP		"Fast-charger"
+
+enum axp288_mux_select {
+	EXTCON_GPIO_MUX_SEL_PMIC = 0,
+	EXTCON_GPIO_MUX_SEL_SOC,
+};
+
+enum axp288_extcon_irq {
+	VBUS_FALLING_IRQ = 0,
+	VBUS_RISING_IRQ,
+	MV_CHNG_IRQ,
+	BC_USB_CHNG_IRQ,
+	EXTCON_IRQ_END,
+};
+
+static const char *axp288_extcon_cables[] = {
+	AXP288_EXTCON_CABLE_SDP,
+	AXP288_EXTCON_CABLE_CDP,
+	AXP288_EXTCON_CABLE_DCP,
+	NULL,
+};
+
+struct axp288_extcon_info {
+	struct device *dev;
+	struct regmap *regmap;
+	struct regmap_irq_chip_data *regmap_irqc;
+	struct axp288_extcon_pdata *pdata;
+	int irq[EXTCON_IRQ_END];
+	struct extcon_dev *edev;
+	struct usb_phy *otg;
+	struct notifier_block extcon_nb;
+	struct extcon_specific_cable_nb cable;
+	/* detect OTG or ID events */
+	bool usb_id_short;
+};
+
+/* Power up/down reason string array */
+static char *axp288_pwr_up_down_info[] = {
+	"Last wake caused by user pressing the power button",
+	"Last wake caused by a charger insertion",
+	"Last wake caused by a battery insertion",
+	"Last wake caused by SOC initiated global reset",
+	"Last wake caused by cold reset",
+	"Last shutdown caused by PMIC UVLO threshold",
+	"Last shutdown caused by SOC initiated cold off",
+	"Last shutdown caused by user pressing the power button",
+	NULL,
+};
+
+/*
+ * Decode and log the given "reset source indicator" (rsi)
+ * register and then clear it.
+ */
+static void axp288_extcon_log_rsi(struct axp288_extcon_info *info,
+				char **pwrsrc_rsi_info, int reg)
+{
+	char **rsi;
+	unsigned int val, i, clear_mask = 0;
+	int ret;
+
+	ret = regmap_read(info->regmap, reg, &val);
+	for (i = 0, rsi = pwrsrc_rsi_info; *rsi; rsi++, i++) {
+		if (val & BIT(i)) {
+			dev_dbg(info->dev, "%s\n", *rsi);
+			clear_mask |= BIT(i);
+		}
+	}
+
+	/* Clear the register value for next reboot (write 1 to clear bit) */
+	regmap_write(info->regmap, reg, clear_mask);
+}
+
+static int handle_chrg_det_event(struct axp288_extcon_info *info)
+{
+	static bool notify_otg, notify_charger;
+	static char *cable;
+	int ret, stat, cfg, pwr_stat;
+	u8 chrg_type;
+	bool vbus_attach = false;
+
+	ret = regmap_read(info->regmap, AXP288_PS_STAT_REG, &pwr_stat);
+	if (ret < 0) {
+		dev_err(info->dev, "vbus status read error\n");
+		return ret;
+	}
+
+	vbus_attach = (pwr_stat & PS_STAT_VBUS_PRESENT) && !info->usb_id_short;
+	if (vbus_attach) {
+		dev_dbg(info->dev, "vbus present\n");
+	} else {
+		dev_dbg(info->dev, "vbus not present\n");
+		goto notify_otg;
+	}
+
+	/* Check charger detection completion status */
+	ret = regmap_read(info->regmap, AXP288_BC_GLOBAL_REG, &cfg);
+	if (ret < 0)
+		goto dev_det_ret;
+	if (cfg & BC_GLOBAL_DET_STAT) {
+		dev_dbg(info->dev, "charger detection not complete!!\n");
+		goto dev_det_ret;
+	}
+
+	ret = regmap_read(info->regmap, AXP288_BC_DET_STAT_REG, &stat);
+	if (ret < 0)
+		goto dev_det_ret;
+	dev_dbg(info->dev, "stat:%x, cfg:%x\n", stat, cfg);
+
+	chrg_type = (stat & DET_STAT_MASK) >> DET_STAT_SHIFT;
+
+	if (chrg_type == DET_STAT_SDP) {
+		dev_dbg(info->dev, "sdp cable connecetd\n");
+		notify_otg = true;
+		notify_charger = true;
+		cable = AXP288_EXTCON_CABLE_SDP;
+	} else if (chrg_type == DET_STAT_CDP) {
+		dev_dbg(info->dev, "cdp cable connecetd\n");
+		notify_otg = true;
+		notify_charger = true;
+		cable = AXP288_EXTCON_CABLE_CDP;
+	} else if (chrg_type == DET_STAT_DCP) {
+		dev_dbg(info->dev, "dcp cable connecetd\n");
+		notify_charger = true;
+		cable = AXP288_EXTCON_CABLE_DCP;
+	} else {
+		dev_warn(info->dev,
+			"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->pdata->gpio_mux_cntl != NULL)
+			gpiod_set_value(info->pdata->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)
+		extcon_set_cable_state(info->edev, cable, vbus_attach);
+
+	/* Clear the flags on disconnect event */
+	if (!vbus_attach) {
+		notify_otg = false;
+		notify_charger = false;
+	}
+
+	return 0;
+
+dev_det_ret:
+	if (ret < 0)
+		dev_warn(info->dev, "BC Mod detection error\n");
+
+	return ret;
+}
+
+static irqreturn_t axp288_extcon_isr(int irq, void *data)
+{
+	struct axp288_extcon_info *info = data;
+	unsigned int i;
+	int ret;
+
+	for (i = 0; i < EXTCON_IRQ_END; i++) {
+		if (info->irq[i] == irq)
+			break;
+	}
+
+	if (i == EXTCON_IRQ_END) {
+		dev_warn(info->dev, "spurious interrupt!!\n");
+		return IRQ_NONE;
+	}
+
+	ret = handle_chrg_det_event(info);
+	if (ret < 0)
+		dev_warn(info->dev, "error in PWRSRC INT handling\n");
+
+	return IRQ_HANDLED;
+}
+
+static int axp288_handle_extcon_event(struct notifier_block *nb,
+				   unsigned long event, void *param)
+{
+	struct axp288_extcon_info *info =
+	    container_of(nb, struct axp288_extcon_info, extcon_nb);
+	struct extcon_dev *edev = param;
+	int usb_host = extcon_get_cable_state(edev, "USB-Host");
+
+	dev_dbg(info->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->usb_id_short = usb_host;
+
+	/*
+	 * Connect the USB mux to SOC in case of usb host else connect
+	 * it to PMIC.
+	 */
+	if (info->pdata->gpio_mux_cntl != NULL) {
+		if (info->usb_id_short)
+			gpiod_set_value(info->pdata->gpio_mux_cntl,
+					EXTCON_GPIO_MUX_SEL_SOC);
+		else
+			gpiod_set_value(info->pdata->gpio_mux_cntl,
+					EXTCON_GPIO_MUX_SEL_PMIC);
+	}
+
+	return NOTIFY_OK;
+}
+
+static int axp288_init_gpio_mux_cntl(struct axp288_extcon_info *info)
+{
+	int ret;
+
+	ret = gpio_request(desc_to_gpio(info->pdata->gpio_mux_cntl), "USB_MUX");
+	if (ret < 0) {
+		dev_err(info->dev, "Failed to request the gpio=%d\n",
+			desc_to_gpio(info->pdata->gpio_mux_cntl));
+		return ret;
+	}
+	gpiod_direction_output(info->pdata->gpio_mux_cntl,
+					EXTCON_GPIO_MUX_SEL_PMIC);
+
+	info->extcon_nb.notifier_call = axp288_handle_extcon_event;
+	ret = extcon_register_interest(&info->cable, NULL,
+				"USB-Host", &info->extcon_nb);
+	if (ret) {
+		dev_err(info->dev, "failed to register extcon notifier\n");
+		return ret;
+	}
+
+	if (info->cable.edev)
+		info->usb_id_short =
+			extcon_get_cable_state(info->cable.edev, "USB-Host");
+
+	if (info->usb_id_short)
+		gpiod_set_value(info->pdata->gpio_mux_cntl,
+					EXTCON_GPIO_MUX_SEL_SOC);
+
+	return 0;
+}
+
+static int axp288_extcon_probe(struct platform_device *pdev)
+{
+	struct axp288_extcon_info *info;
+	struct axp20x_dev *axp20x = dev_get_drvdata(pdev->dev.parent);
+	int ret, i, pirq;
+
+	info = devm_kzalloc(&pdev->dev, sizeof(*info), GFP_KERNEL);
+	if (!info)
+		return -ENOMEM;
+
+	info->dev = &pdev->dev;
+	info->regmap = axp20x->regmap;
+	info->regmap_irqc = axp20x->regmap_irqc;
+	info->pdata = pdev->dev.platform_data;
+
+	if (!info->pdata) {
+		/* TODO: Try ACPI provided pdata via device properties */
+		if (!device_property_present(&pdev->dev,
+					"axp288_extcon_data\n"))
+			dev_err(&pdev->dev, "no platform data\n");
+		return -ENODEV;
+	}
+	platform_set_drvdata(pdev, info);
+
+	axp288_extcon_log_rsi(info, axp288_pwr_up_down_info,
+				AXP288_PS_BOOT_REASON_REG);
+
+	/* Initialize extcon device */
+	info->edev = devm_extcon_dev_allocate(&pdev->dev,
+					      axp288_extcon_cables);
+	if (IS_ERR(info->edev)) {
+		dev_err(&pdev->dev, "failed to allocate memory for extcon\n");
+		return -ENOMEM;
+	}
+
+	/* Register extcon device */
+	info->edev->name = dev_name(&pdev->dev);
+	ret = devm_extcon_dev_register(&pdev->dev, info->edev);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to register extcon device\n");
+		return ret;
+	}
+
+	/* Get otg transceiver phy */
+	info->otg = usb_get_phy(USB_PHY_TYPE_USB2);
+	if (IS_ERR(info->otg)) {
+		dev_warn(&pdev->dev, "Failed to get otg transceiver!!\n");
+		ret = PTR_ERR(info->otg);
+		return ret;
+	}
+
+	for (i = 0; i < EXTCON_IRQ_END; i++) {
+		pirq = platform_get_irq(pdev, i);
+		info->irq[i] = regmap_irq_get_virq(info->regmap_irqc, pirq);
+		if (info->irq[i] < 0) {
+			dev_warn(&pdev->dev,
+				"regmap_irq get virq failed for IRQ %d: %d\n",
+				pirq, info->irq[i]);
+			info->irq[i] = -1;
+			goto intr_reg_failed;
+		}
+		ret = devm_request_threaded_irq(&pdev->dev, info->irq[i],
+				NULL, axp288_extcon_isr,
+				IRQF_ONESHOT | IRQF_NO_SUSPEND,
+				pdev->name, info);
+		if (ret) {
+			dev_err(&pdev->dev, "request_irq fail :%d err:%d\n",
+							info->irq[i], ret);
+			goto intr_reg_failed;
+		}
+	}
+
+	/* Set up gpio control for USB Mux */
+	if (info->pdata->gpio_mux_cntl != NULL) {
+		ret = axp288_init_gpio_mux_cntl(info);
+		if (ret < 0)
+			goto intr_reg_failed;
+	}
+
+	/* 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 complte 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);
+
+	return 0;
+
+intr_reg_failed:
+	usb_put_phy(info->otg);
+	return ret;
+}
+
+static int axp288_extcon_remove(struct platform_device *pdev)
+{
+	struct axp288_extcon_info *info = platform_get_drvdata(pdev);
+
+	usb_put_phy(info->otg);
+	return 0;
+}
+
+static struct platform_driver axp288_extcon_driver = {
+	.probe = axp288_extcon_probe,
+	.remove = axp288_extcon_remove,
+	.driver = {
+		.name = "axp288_extcon",
+	},
+};
+module_platform_driver(axp288_extcon_driver);
+
+MODULE_AUTHOR("Ramakrishna Pallala <ramakrishna.pallala@intel.com>");
+MODULE_DESCRIPTION("X-Powers AXP288 extcon driver");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/mfd/axp20x.h b/include/linux/mfd/axp20x.h
index dfabd6d..4ed8071 100644
--- a/include/linux/mfd/axp20x.h
+++ b/include/linux/mfd/axp20x.h
@@ -275,4 +275,9 @@ struct axp20x_fg_pdata {
 	int thermistor_curve[MAX_THERM_CURVE_SIZE][2];
 };
 
+struct axp288_extcon_pdata {
+	/* GPIO pin control to switch D+/D- lines b/w PMIC and SOC */
+	struct gpio_desc *gpio_mux_cntl;
+};
+
 #endif /* __LINUX_MFD_AXP20X_H */
-- 
1.7.9.5

Re: [PATCH v3 2/2] extcon-axp288: Add axp288 extcon driver support

[Chanwoo Choi]

Hi Ramakrishna,

I'm sorry for late reply.

On 04/09/2015 02:12 AM, Ramakrishna Pallala wrote:
> This patch adds the extcon support for AXP288 PMIC which
> has the BC1.2 charger detection capability. Additionally
> it also adds the USB mux switching support b/w SOC and PMIC
> based on GPIO control.
> 
> Signed-off-by: Ramakrishna Pallala <ramakrishna.pallala@intel.com>
> ---
>  drivers/extcon/Kconfig         |    7 +
>  drivers/extcon/Makefile        |    1 +
>  drivers/extcon/extcon-axp288.c |  462 ++++++++++++++++++++++++++++++++++++++++
>  include/linux/mfd/axp20x.h     |    5 +
>  4 files changed, 475 insertions(+)
>  create mode 100644 drivers/extcon/extcon-axp288.c
> 
> diff --git a/drivers/extcon/Kconfig b/drivers/extcon/Kconfig
> index 6a1f7de..f6e8b2a 100644
> --- a/drivers/extcon/Kconfig
> +++ b/drivers/extcon/Kconfig
> @@ -28,6 +28,13 @@ config EXTCON_ARIZONA
>  	  with Wolfson Arizona devices. These are audio CODECs with
>  	  advanced audio accessory detection support.
>  
> +config EXTCON_AXP288
> +	tristate "X-Power AXP288 EXTCON support"
> +	depends on MFD_AXP20X && USB_PHY
> +	help
> +	  Say Y here to enable support for USB peripheral detection
> +	  and USB MUX switching by X-Power AXP288 PMIC.
> +
>  config EXTCON_GPIO
>  	tristate "GPIO extcon support"
>  	depends on GPIOLIB
> diff --git a/drivers/extcon/Makefile b/drivers/extcon/Makefile
> index 0370b42..5429377 100644
> --- a/drivers/extcon/Makefile
> +++ b/drivers/extcon/Makefile
> @@ -5,6 +5,7 @@
>  obj-$(CONFIG_EXTCON)		+= extcon-class.o
>  obj-$(CONFIG_EXTCON_ADC_JACK)	+= extcon-adc-jack.o
>  obj-$(CONFIG_EXTCON_ARIZONA)	+= extcon-arizona.o
> +obj-$(CONFIG_EXTCON_AXP288)	+= extcon-axp288.o
>  obj-$(CONFIG_EXTCON_GPIO)	+= extcon-gpio.o
>  obj-$(CONFIG_EXTCON_MAX14577)	+= extcon-max14577.o
>  obj-$(CONFIG_EXTCON_MAX77693)	+= extcon-max77693.o
> diff --git a/drivers/extcon/extcon-axp288.c b/drivers/extcon/extcon-axp288.c
> new file mode 100644
> index 0000000..363552c
> --- /dev/null
> +++ b/drivers/extcon/extcon-axp288.c
> @@ -0,0 +1,462 @@
> +/*
> + * extcon-axp288.c - X-Power AXP288 PMIC extcon cable detection driver
> + *
> + * Copyright (C) 2015 Intel Corporation
> + * Author: Ramakrishna Pallala <ramakrishna.pallala@intel.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/io.h>
> +#include <linux/slab.h>
> +#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>
> +#include <linux/gpio.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/mfd/axp20x.h>
> +
> +/* Power source status register */
> +#define PS_STAT_VBUS_TRIGGER		BIT(0)
> +#define PS_STAT_BAT_CHRG_DIR		BIT(2)
> +#define PS_STAT_VBUS_ABOVE_VHOLD	BIT(3)
> +#define PS_STAT_VBUS_VALID		BIT(4)
> +#define PS_STAT_VBUS_PRESENT		BIT(5)
> +
> +/* BC module global register */
> +#define BC_GLOBAL_RUN			BIT(0)
> +#define BC_GLOBAL_DET_STAT		BIT(2)
> +#define BC_GLOBAL_DBP_TOUT		BIT(3)
> +#define BC_GLOBAL_VLGC_COM_SEL		BIT(4)
> +#define BC_GLOBAL_DCD_TOUT_MASK		(BIT(6)|BIT(5))
> +#define BC_GLOBAL_DCD_TOUT_300MS	0
> +#define BC_GLOBAL_DCD_TOUT_100MS	1
> +#define BC_GLOBAL_DCD_TOUT_500MS	2
> +#define BC_GLOBAL_DCD_TOUT_900MS	3
> +#define BC_GLOBAL_DCD_DET_SEL		BIT(7)
> +
> +/* BC module vbus control and status register */
> +#define VBUS_CNTL_DPDM_PD_EN		BIT(4)
> +#define VBUS_CNTL_DPDM_FD_EN		BIT(5)
> +#define VBUS_CNTL_FIRST_PO_STAT		BIT(6)
> +
> +/* BC USB status register */
> +#define USB_STAT_BUS_STAT_MASK		(BIT(3)|BIT(2)|BIT(1)|BIT(0))
> +#define USB_STAT_BUS_STAT_SHIFT		0
> +#define USB_STAT_BUS_STAT_ATHD		0
> +#define USB_STAT_BUS_STAT_CONN		1
> +#define USB_STAT_BUS_STAT_SUSP		2
> +#define USB_STAT_BUS_STAT_CONF		3
> +#define USB_STAT_USB_SS_MODE		BIT(4)
> +#define USB_STAT_DEAD_BAT_DET		BIT(6)
> +#define USB_STAT_DBP_UNCFG		BIT(7)
> +
> +/* BC detect status register */
> +#define DET_STAT_MASK			(BIT(7)|BIT(6)|BIT(5))
> +#define DET_STAT_SHIFT			5
> +#define DET_STAT_SDP			1
> +#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,
> +	AXP288_BC_GLOBAL_REG		= 0x2c,
> +	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,
> +};
> +
> +#define AXP288_DRV_NAME			"axp288_extcon"

Use the '-' instead of '_'
- axp288_extcon -> axp288-extcon

> +
> +#define AXP288_EXTCON_CABLE_SDP		"Slow-charger"
> +#define AXP288_EXTCON_CABLE_CDP		"Charge-downstream"
> +#define AXP288_EXTCON_CABLE_DCP		"Fast-charger"

I'll rename the cable name by using capital letter instead of small letter.
So, I'd like you to rename cable as following:
- "Slow-charger" -> "SLOW-CHARGER"
- "Charge-downstream" ->"CHARGE-DOWNSTREAM"
- "Fast-charger" -> "FAST-CHARGER"

> +
> +enum axp288_mux_select {
> +	EXTCON_GPIO_MUX_SEL_PMIC = 0,
> +	EXTCON_GPIO_MUX_SEL_SOC,
> +};
> +
> +enum axp288_extcon_irq {
> +	VBUS_FALLING_IRQ = 0,
> +	VBUS_RISING_IRQ,
> +	MV_CHNG_IRQ,
> +	BC_USB_CHNG_IRQ,
> +	EXTCON_IRQ_END,
> +};
> +
> +static const char *axp288_extcon_cables[] = {
> +	AXP288_EXTCON_CABLE_SDP,
> +	AXP288_EXTCON_CABLE_CDP,
> +	AXP288_EXTCON_CABLE_DCP,
> +	NULL,
> +};
> +
> +struct axp288_extcon_info {
> +	struct device *dev;
> +	struct regmap *regmap;
> +	struct regmap_irq_chip_data *regmap_irqc;
> +	struct axp288_extcon_pdata *pdata;
> +	int irq[EXTCON_IRQ_END];
> +	struct extcon_dev *edev;
> +	struct usb_phy *otg;
> +	struct notifier_block extcon_nb;
> +	struct extcon_specific_cable_nb cable;
> +	/* detect OTG or ID events */

This driver have the feature of both extcon provider driver
and extcon consumer driver.

> +	bool usb_id_short;
> +};
> +
> +/* Power up/down reason string array */
> +static char *axp288_pwr_up_down_info[] = {
> +	"Last wake caused by user pressing the power button",
> +	"Last wake caused by a charger insertion",
> +	"Last wake caused by a battery insertion",
> +	"Last wake caused by SOC initiated global reset",
> +	"Last wake caused by cold reset",
> +	"Last shutdown caused by PMIC UVLO threshold",
> +	"Last shutdown caused by SOC initiated cold off",
> +	"Last shutdown caused by user pressing the power button",
> +	NULL,
> +};
> +
> +/*
> + * Decode and log the given "reset source indicator" (rsi)
> + * register and then clear it.
> + */
> +static void axp288_extcon_log_rsi(struct axp288_extcon_info *info,
> +				char **pwrsrc_rsi_info, int reg)

This function read only AXP288_PS_BOOT_REASON_REG register.
So, axp288_extcon_log_rsi() don't need the second parameter (int reg).

> +{
> +	char **rsi;
> +	unsigned int val, i, clear_mask = 0;
> +	int ret;
> +
> +	ret = regmap_read(info->regmap, reg, &val);
> +	for (i = 0, rsi = pwrsrc_rsi_info; *rsi; rsi++, i++) {
> +		if (val & BIT(i)) {
> +			dev_dbg(info->dev, "%s\n", *rsi);
> +			clear_mask |= BIT(i);
> +		}
> +	}
> +
> +	/* Clear the register value for next reboot (write 1 to clear bit) */
> +	regmap_write(info->regmap, reg, clear_mask);
> +}
> +
> +static int handle_chrg_det_event(struct axp288_extcon_info *info)
> +{
> +	static bool notify_otg, notify_charger;
> +	static char *cable;
> +	int ret, stat, cfg, pwr_stat;
> +	u8 chrg_type;
> +	bool vbus_attach = false;
> +
> +	ret = regmap_read(info->regmap, AXP288_PS_STAT_REG, &pwr_stat);
> +	if (ret < 0) {
> +		dev_err(info->dev, "vbus status read error\n");
> +		return ret;
> +	}
> +
> +	vbus_attach = (pwr_stat & PS_STAT_VBUS_PRESENT) && !info->usb_id_short;
> +	if (vbus_attach) {
> +		dev_dbg(info->dev, "vbus present\n");
> +	} else {
> +		dev_dbg(info->dev, "vbus not present\n");
> +		goto notify_otg;
> +	}
> +
> +	/* Check charger detection completion status */
> +	ret = regmap_read(info->regmap, AXP288_BC_GLOBAL_REG, &cfg);
> +	if (ret < 0)
> +		goto dev_det_ret;
> +	if (cfg & BC_GLOBAL_DET_STAT) {
> +		dev_dbg(info->dev, "charger detection not complete!!\n");
> +		goto dev_det_ret;
> +	}
> +
> +	ret = regmap_read(info->regmap, AXP288_BC_DET_STAT_REG, &stat);
> +	if (ret < 0)
> +		goto dev_det_ret;
> +	dev_dbg(info->dev, "stat:%x, cfg:%x\n", stat, cfg);
> +
> +	chrg_type = (stat & DET_STAT_MASK) >> DET_STAT_SHIFT;
> +
> +	if (chrg_type == DET_STAT_SDP) {
> +		dev_dbg(info->dev, "sdp cable connecetd\n");
> +		notify_otg = true;
> +		notify_charger = true;
> +		cable = AXP288_EXTCON_CABLE_SDP;
> +	} else if (chrg_type == DET_STAT_CDP) {
> +		dev_dbg(info->dev, "cdp cable connecetd\n");
> +		notify_otg = true;
> +		notify_charger = true;
> +		cable = AXP288_EXTCON_CABLE_CDP;
> +	} else if (chrg_type == DET_STAT_DCP) {
> +		dev_dbg(info->dev, "dcp cable connecetd\n");
> +		notify_charger = true;
> +		cable = AXP288_EXTCON_CABLE_DCP;
> +	} else {
> +		dev_warn(info->dev,
> +			"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->pdata->gpio_mux_cntl != NULL)
> +			gpiod_set_value(info->pdata->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)
> +		extcon_set_cable_state(info->edev, cable, vbus_attach);
> +
> +	/* Clear the flags on disconnect event */
> +	if (!vbus_attach) {
> +		notify_otg = false;
> +		notify_charger = false;
> +	}
> +
> +	return 0;
> +
> +dev_det_ret:
> +	if (ret < 0)
> +		dev_warn(info->dev, "BC Mod detection error\n");
> +
> +	return ret;
> +}
> +
> +static irqreturn_t axp288_extcon_isr(int irq, void *data)
> +{
> +	struct axp288_extcon_info *info = data;
> +	unsigned int i;
> +	int ret;
> +
> +	for (i = 0; i < EXTCON_IRQ_END; i++) {
> +		if (info->irq[i] == irq)
> +			break;
> +	}
> +
> +	if (i == EXTCON_IRQ_END) {
> +		dev_warn(info->dev, "spurious interrupt!!\n");
> +		return IRQ_NONE;
> +	}
> +
> +	ret = handle_chrg_det_event(info);
> +	if (ret < 0)
> +		dev_warn(info->dev, "error in PWRSRC INT handling\n");
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int axp288_handle_extcon_event(struct notifier_block *nb,
> +				   unsigned long event, void *param)
> +{
> +	struct axp288_extcon_info *info =
> +	    container_of(nb, struct axp288_extcon_info, extcon_nb);
> +	struct extcon_dev *edev = param;
> +	int usb_host = extcon_get_cable_state(edev, "USB-Host");
> +
> +	dev_dbg(info->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->usb_id_short = usb_host;
> +
> +	/*
> +	 * Connect the USB mux to SOC in case of usb host else connect
> +	 * it to PMIC.
> +	 */
> +	if (info->pdata->gpio_mux_cntl != NULL) {
> +		if (info->usb_id_short)
> +			gpiod_set_value(info->pdata->gpio_mux_cntl,
> +					EXTCON_GPIO_MUX_SEL_SOC);
> +		else
> +			gpiod_set_value(info->pdata->gpio_mux_cntl,
> +					EXTCON_GPIO_MUX_SEL_PMIC);
> +	}
> +
> +	return NOTIFY_OK;
> +}
> +
> +static int axp288_init_gpio_mux_cntl(struct axp288_extcon_info *info)
> +{
> +	int ret;
> +
> +	ret = gpio_request(desc_to_gpio(info->pdata->gpio_mux_cntl), "USB_MUX");
> +	if (ret < 0) {
> +		dev_err(info->dev, "Failed to request the gpio=%d\n",
> +			desc_to_gpio(info->pdata->gpio_mux_cntl));
> +		return ret;
> +	}
> +	gpiod_direction_output(info->pdata->gpio_mux_cntl,
> +					EXTCON_GPIO_MUX_SEL_PMIC);
> +
> +	info->extcon_nb.notifier_call = axp288_handle_extcon_event;
> +	ret = extcon_register_interest(&info->cable, NULL,
> +				"USB-Host", &info->extcon_nb);
> +	if (ret) {
> +		dev_err(info->dev, "failed to register extcon notifier\n");
> +		return ret;
> +	}
> +
> +	if (info->cable.edev)
> +		info->usb_id_short =
> +			extcon_get_cable_state(info->cable.edev, "USB-Host");
> +
> +	if (info->usb_id_short)
> +		gpiod_set_value(info->pdata->gpio_mux_cntl,
> +					EXTCON_GPIO_MUX_SEL_SOC);
> +
> +	return 0;
> +}

Currently, the only extcon provider driver are included in drivers/extcon.
I'm reruclant to support extcon consumer feature in extcon directory.
Also, this driver support both extcon provider feature and extcon consumer
feature in one driver. It is one reluctant reason.

So, I suggest that you includes only extcon provider feature in this patchset.
And then we will discuss the support of extcon consumer feature on later patch.

I will support the 'usb notifier chain' to detect both vbus and id pin state.
you can refet to this discussion on patch[1].
	#define EXTCON_USB_ID_LOW
	#define EXTCON_USB_ID_HIGH	
	#define EXTCON_USB_VBUS_LOW
	#define EXTCON_USB_VBUS_HIGH

[1] https://lkml.org/lkml/2015/4/16/116


> +
> +static int axp288_extcon_probe(struct platform_device *pdev)
> +{
> +	struct axp288_extcon_info *info;
> +	struct axp20x_dev *axp20x = dev_get_drvdata(pdev->dev.parent);
> +	int ret, i, pirq;
> +
> +	info = devm_kzalloc(&pdev->dev, sizeof(*info), GFP_KERNEL);
> +	if (!info)
> +		return -ENOMEM;
> +
> +	info->dev = &pdev->dev;
> +	info->regmap = axp20x->regmap;
> +	info->regmap_irqc = axp20x->regmap_irqc;
> +	info->pdata = pdev->dev.platform_data;
> +
> +	if (!info->pdata) {
> +		/* TODO: Try ACPI provided pdata via device properties */
> +		if (!device_property_present(&pdev->dev,
> +					"axp288_extcon_data\n"))
> +			dev_err(&pdev->dev, "no platform data\n");
> +		return -ENODEV;
> +	}
> +	platform_set_drvdata(pdev, info);
> +
> +	axp288_extcon_log_rsi(info, axp288_pwr_up_down_info,
> +				AXP288_PS_BOOT_REASON_REG);
> +
> +	/* Initialize extcon device */
> +	info->edev = devm_extcon_dev_allocate(&pdev->dev,
> +					      axp288_extcon_cables);
> +	if (IS_ERR(info->edev)) {
> +		dev_err(&pdev->dev, "failed to allocate memory for extcon\n");
> +		return -ENOMEM;

Use PTR_ERR(info->edev) instead of -ENOMEM

> +	}
> +
> +	/* Register extcon device */
> +	info->edev->name = dev_name(&pdev->dev);

Don't need to set the name of extcon device. extcon_dev_register()
set the name of extcon device. So, I'll move the extcon_dev structure
from extcon.h to extcon.c becaseu extcon_dev structure should be handled
in extcon core.

> +	ret = devm_extcon_dev_register(&pdev->dev, info->edev);
> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to register extcon device\n");
> +		return ret;
> +	}
> +
> +	/* Get otg transceiver phy */
> +	info->otg = usb_get_phy(USB_PHY_TYPE_USB2);
> +	if (IS_ERR(info->otg)) {
> +		dev_warn(&pdev->dev, "Failed to get otg transceiver!!\n");

'Failed' -> 'failed'. don't need the '!!' on end of debug log.

> +		ret = PTR_ERR(info->otg);
> +		return ret;
> +	}
> +
> +	for (i = 0; i < EXTCON_IRQ_END; i++) {
> +		pirq = platform_get_irq(pdev, i);
> +		info->irq[i] = regmap_irq_get_virq(info->regmap_irqc, pirq);
> +		if (info->irq[i] < 0) {
> +			dev_warn(&pdev->dev,
> +				"regmap_irq get virq failed for IRQ %d: %d\n",
> +				pirq, info->irq[i]);
> +			info->irq[i] = -1;
> +			goto intr_reg_failed;
> +		}

Need to add blank line

> +		ret = devm_request_threaded_irq(&pdev->dev, info->irq[i],
> +				NULL, axp288_extcon_isr,
> +				IRQF_ONESHOT | IRQF_NO_SUSPEND,
> +				pdev->name, info);
> +		if (ret) {
> +			dev_err(&pdev->dev, "request_irq fail :%d err:%d\n",
> +							info->irq[i], ret);
> +			goto intr_reg_failed;
> +		}
> +	}
> +
> +	/* Set up gpio control for USB Mux */
> +	if (info->pdata->gpio_mux_cntl != NULL) {

Modify if statement as following simply:
	if (info->pdata->gpio_mux_cntl)

> +		ret = axp288_init_gpio_mux_cntl(info);
> +		if (ret < 0)
> +			goto intr_reg_failed;
> +	}
> +
> +	/* 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 complte 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);

I think that you better to move the initialization code before extcon registration.
The initialization should be executed before extcon and interrupt registration.

> +
> +	return 0;
> +
> +intr_reg_failed:
> +	usb_put_phy(info->otg);
> +	return ret;
> +}
> +
> +static int axp288_extcon_remove(struct platform_device *pdev)
> +{
> +	struct axp288_extcon_info *info = platform_get_drvdata(pdev);
> +
> +	usb_put_phy(info->otg);
> +	return 0;
> +}
> +
> +static struct platform_driver axp288_extcon_driver = {
> +	.probe = axp288_extcon_probe,
> +	.remove = axp288_extcon_remove,
> +	.driver = {
> +		.name = "axp288_extcon",
> +	},

You should add the suspend/resume funciton to control the interrupt
during suspend state. I discussed the same issue on patch[2].
And you can refer to extcon-usb-gpio.c[3] for controlling interrupt
in suspend/resume function.

[2] https://lkml.org/lkml/2015/1/27/1069
[3] http://git.kernel.org/cgit/linux/kernel/git/chanwoo/extcon.git/commit/?h=extcon-next&id=e52817faae359ce95c93c2b6eb88b16d4b430181

> +};
> +module_platform_driver(axp288_extcon_driver);
> +
> +MODULE_AUTHOR("Ramakrishna Pallala <ramakrishna.pallala@intel.com>");
> +MODULE_DESCRIPTION("X-Powers AXP288 extcon driver");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/mfd/axp20x.h b/include/linux/mfd/axp20x.h
> index dfabd6d..4ed8071 100644
> --- a/include/linux/mfd/axp20x.h
> +++ b/include/linux/mfd/axp20x.h
> @@ -275,4 +275,9 @@ struct axp20x_fg_pdata {
>  	int thermistor_curve[MAX_THERM_CURVE_SIZE][2];
>  };
>  
> +struct axp288_extcon_pdata {
> +	/* GPIO pin control to switch D+/D- lines b/w PMIC and SOC */
> +	struct gpio_desc *gpio_mux_cntl;
> +};
> +
>  #endif /* __LINUX_MFD_AXP20X_H */
> 

RE: [PATCH v3 2/2] extcon-axp288: Add axp288 extcon driver support

[Pallala, Ramakrishna]

Hi Choi,

Please find my response below. 

> On 04/09/2015 02:12 AM, Ramakrishna Pallala wrote:
> > This patch adds the extcon support for AXP288 PMIC which has the BC1.2
> > charger detection capability. Additionally it also adds the USB mux
> > switching support b/w SOC and PMIC based on GPIO control.
> >
> > Signed-off-by: Ramakrishna Pallala <ramakrishna.pallala@intel.com>
> > ---
> >  drivers/extcon/Kconfig         |    7 +
> >  drivers/extcon/Makefile        |    1 +
> >  drivers/extcon/extcon-axp288.c |  462
> ++++++++++++++++++++++++++++++++++++++++
> >  include/linux/mfd/axp20x.h     |    5 +
> >  4 files changed, 475 insertions(+)
> >  create mode 100644 drivers/extcon/extcon-axp288.c
> >
> > diff --git a/drivers/extcon/Kconfig b/drivers/extcon/Kconfig index
> > 6a1f7de..f6e8b2a 100644
> > --- a/drivers/extcon/Kconfig
> > +++ b/drivers/extcon/Kconfig
> > @@ -28,6 +28,13 @@ config EXTCON_ARIZONA
> >  	  with Wolfson Arizona devices. These are audio CODECs with
> >  	  advanced audio accessory detection support.
> >
> > +config EXTCON_AXP288
> > +	tristate "X-Power AXP288 EXTCON support"
> > +	depends on MFD_AXP20X && USB_PHY
> > +	help
> > +	  Say Y here to enable support for USB peripheral detection
> > +	  and USB MUX switching by X-Power AXP288 PMIC.
> > +
> >  config EXTCON_GPIO
> >  	tristate "GPIO extcon support"
> >  	depends on GPIOLIB
> > diff --git a/drivers/extcon/Makefile b/drivers/extcon/Makefile index
> > 0370b42..5429377 100644
> > --- a/drivers/extcon/Makefile
> > +++ b/drivers/extcon/Makefile
> > @@ -5,6 +5,7 @@
> >  obj-$(CONFIG_EXTCON)		+= extcon-class.o
> >  obj-$(CONFIG_EXTCON_ADC_JACK)	+= extcon-adc-jack.o
> >  obj-$(CONFIG_EXTCON_ARIZONA)	+= extcon-arizona.o
> > +obj-$(CONFIG_EXTCON_AXP288)	+= extcon-axp288.o
> >  obj-$(CONFIG_EXTCON_GPIO)	+= extcon-gpio.o
> >  obj-$(CONFIG_EXTCON_MAX14577)	+= extcon-max14577.o
> >  obj-$(CONFIG_EXTCON_MAX77693)	+= extcon-max77693.o
> > diff --git a/drivers/extcon/extcon-axp288.c
> > b/drivers/extcon/extcon-axp288.c new file mode 100644 index
> > 0000000..363552c
> > --- /dev/null
> > +++ b/drivers/extcon/extcon-axp288.c
> > @@ -0,0 +1,462 @@
> > +/*
> > + * extcon-axp288.c - X-Power AXP288 PMIC extcon cable detection
> > +driver
> > + *
> > + * Copyright (C) 2015 Intel Corporation
> > + * Author: Ramakrishna Pallala <ramakrishna.pallala@intel.com>
> > + *
> > + * This program is free software; you can redistribute it and/or
> > +modify
> > + * it under the terms of the GNU General Public License as published
> > +by
> > + * the Free Software Foundation; either version 2 of the License, or
> > + * (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > + */
> > +
> > +#include <linux/module.h>
> > +#include <linux/kernel.h>
> > +#include <linux/io.h>
> > +#include <linux/slab.h>
> > +#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>
> > +#include <linux/gpio.h>
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/mfd/axp20x.h>
> > +
> > +/* Power source status register */
> > +#define PS_STAT_VBUS_TRIGGER		BIT(0)
> > +#define PS_STAT_BAT_CHRG_DIR		BIT(2)
> > +#define PS_STAT_VBUS_ABOVE_VHOLD	BIT(3)
> > +#define PS_STAT_VBUS_VALID		BIT(4)
> > +#define PS_STAT_VBUS_PRESENT		BIT(5)
> > +
> > +/* BC module global register */
> > +#define BC_GLOBAL_RUN			BIT(0)
> > +#define BC_GLOBAL_DET_STAT		BIT(2)
> > +#define BC_GLOBAL_DBP_TOUT		BIT(3)
> > +#define BC_GLOBAL_VLGC_COM_SEL		BIT(4)
> > +#define BC_GLOBAL_DCD_TOUT_MASK		(BIT(6)|BIT(5))
> > +#define BC_GLOBAL_DCD_TOUT_300MS	0
> > +#define BC_GLOBAL_DCD_TOUT_100MS	1
> > +#define BC_GLOBAL_DCD_TOUT_500MS	2
> > +#define BC_GLOBAL_DCD_TOUT_900MS	3
> > +#define BC_GLOBAL_DCD_DET_SEL		BIT(7)
> > +
> > +/* BC module vbus control and status register */
> > +#define VBUS_CNTL_DPDM_PD_EN		BIT(4)
> > +#define VBUS_CNTL_DPDM_FD_EN		BIT(5)
> > +#define VBUS_CNTL_FIRST_PO_STAT		BIT(6)
> > +
> > +/* BC USB status register */
> > +#define USB_STAT_BUS_STAT_MASK		(BIT(3)|BIT(2)|BIT(1)|BIT(0))
> > +#define USB_STAT_BUS_STAT_SHIFT		0
> > +#define USB_STAT_BUS_STAT_ATHD		0
> > +#define USB_STAT_BUS_STAT_CONN		1
> > +#define USB_STAT_BUS_STAT_SUSP		2
> > +#define USB_STAT_BUS_STAT_CONF		3
> > +#define USB_STAT_USB_SS_MODE		BIT(4)
> > +#define USB_STAT_DEAD_BAT_DET		BIT(6)
> > +#define USB_STAT_DBP_UNCFG		BIT(7)
> > +
> > +/* BC detect status register */
> > +#define DET_STAT_MASK			(BIT(7)|BIT(6)|BIT(5))
> > +#define DET_STAT_SHIFT			5
> > +#define DET_STAT_SDP			1
> > +#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,
> > +	AXP288_BC_GLOBAL_REG		= 0x2c,
> > +	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,
> > +};
> > +
> > +#define AXP288_DRV_NAME			"axp288_extcon"
> 
> Use the '-' instead of '_'
> - axp288_extcon -> axp288-extcon

All axp20x pmic mfd cell names are starting with "axp288_*" this one comment I got from Lee Jones on mfd patch.


> > +
> > +#define AXP288_EXTCON_CABLE_SDP		"Slow-charger"
> > +#define AXP288_EXTCON_CABLE_CDP		"Charge-downstream"
> > +#define AXP288_EXTCON_CABLE_DCP		"Fast-charger"
> 
> I'll rename the cable name by using capital letter instead of small letter.
> So, I'd like you to rename cable as following:
> - "Slow-charger" -> "SLOW-CHARGER"
> - "Charge-downstream" ->"CHARGE-DOWNSTREAM"
> - "Fast-charger" -> "FAST-CHARGER"
Ok I will change.

> > +
> > +enum axp288_mux_select {
> > +	EXTCON_GPIO_MUX_SEL_PMIC = 0,
> > +	EXTCON_GPIO_MUX_SEL_SOC,
> > +};
> > +
> > +enum axp288_extcon_irq {
> > +	VBUS_FALLING_IRQ = 0,
> > +	VBUS_RISING_IRQ,
> > +	MV_CHNG_IRQ,
> > +	BC_USB_CHNG_IRQ,
> > +	EXTCON_IRQ_END,
> > +};
> > +
> > +static const char *axp288_extcon_cables[] = {
> > +	AXP288_EXTCON_CABLE_SDP,
> > +	AXP288_EXTCON_CABLE_CDP,
> > +	AXP288_EXTCON_CABLE_DCP,
> > +	NULL,
> > +};
> > +
> > +struct axp288_extcon_info {
> > +	struct device *dev;
> > +	struct regmap *regmap;
> > +	struct regmap_irq_chip_data *regmap_irqc;
> > +	struct axp288_extcon_pdata *pdata;
> > +	int irq[EXTCON_IRQ_END];
> > +	struct extcon_dev *edev;
> > +	struct usb_phy *otg;
> > +	struct notifier_block extcon_nb;
> > +	struct extcon_specific_cable_nb cable;
> > +	/* detect OTG or ID events */
> 
> This driver have the feature of both extcon provider driver and extcon consumer
> driver.
Yes, it's acts as both...I will remove the consumer support from this patch as you suggested in later comments.

> 
> > +	bool usb_id_short;
> > +};
> > +
> > +/* Power up/down reason string array */ static char
> > +*axp288_pwr_up_down_info[] = {
> > +	"Last wake caused by user pressing the power button",
> > +	"Last wake caused by a charger insertion",
> > +	"Last wake caused by a battery insertion",
> > +	"Last wake caused by SOC initiated global reset",
> > +	"Last wake caused by cold reset",
> > +	"Last shutdown caused by PMIC UVLO threshold",
> > +	"Last shutdown caused by SOC initiated cold off",
> > +	"Last shutdown caused by user pressing the power button",
> > +	NULL,
> > +};
> > +
> > +/*
> > + * Decode and log the given "reset source indicator" (rsi)
> > + * register and then clear it.
> > + */
> > +static void axp288_extcon_log_rsi(struct axp288_extcon_info *info,
> > +				char **pwrsrc_rsi_info, int reg)
> 
> This function read only AXP288_PS_BOOT_REASON_REG register.
> So, axp288_extcon_log_rsi() don't need the second parameter (int reg).
Ok. I will fix this. 

> > +{
> > +	char **rsi;
> > +	unsigned int val, i, clear_mask = 0;
> > +	int ret;
> > +
> > +	ret = regmap_read(info->regmap, reg, &val);
> > +	for (i = 0, rsi = pwrsrc_rsi_info; *rsi; rsi++, i++) {
> > +		if (val & BIT(i)) {
> > +			dev_dbg(info->dev, "%s\n", *rsi);
> > +			clear_mask |= BIT(i);
> > +		}
> > +	}
> > +
> > +	/* Clear the register value for next reboot (write 1 to clear bit) */
> > +	regmap_write(info->regmap, reg, clear_mask); }
> > +
> > +static int handle_chrg_det_event(struct axp288_extcon_info *info) {
> > +	static bool notify_otg, notify_charger;
> > +	static char *cable;
> > +	int ret, stat, cfg, pwr_stat;
> > +	u8 chrg_type;
> > +	bool vbus_attach = false;
> > +
> > +	ret = regmap_read(info->regmap, AXP288_PS_STAT_REG, &pwr_stat);
> > +	if (ret < 0) {
> > +		dev_err(info->dev, "vbus status read error\n");
> > +		return ret;
> > +	}
> > +
> > +	vbus_attach = (pwr_stat & PS_STAT_VBUS_PRESENT) && !info-
> >usb_id_short;
> > +	if (vbus_attach) {
> > +		dev_dbg(info->dev, "vbus present\n");
> > +	} else {
> > +		dev_dbg(info->dev, "vbus not present\n");
> > +		goto notify_otg;
> > +	}
> > +
> > +	/* Check charger detection completion status */
> > +	ret = regmap_read(info->regmap, AXP288_BC_GLOBAL_REG, &cfg);
> > +	if (ret < 0)
> > +		goto dev_det_ret;
> > +	if (cfg & BC_GLOBAL_DET_STAT) {
> > +		dev_dbg(info->dev, "charger detection not complete!!\n");
> > +		goto dev_det_ret;
> > +	}
> > +
> > +	ret = regmap_read(info->regmap, AXP288_BC_DET_STAT_REG, &stat);
> > +	if (ret < 0)
> > +		goto dev_det_ret;
> > +	dev_dbg(info->dev, "stat:%x, cfg:%x\n", stat, cfg);
> > +
> > +	chrg_type = (stat & DET_STAT_MASK) >> DET_STAT_SHIFT;
> > +
> > +	if (chrg_type == DET_STAT_SDP) {
> > +		dev_dbg(info->dev, "sdp cable connecetd\n");
> > +		notify_otg = true;
> > +		notify_charger = true;
> > +		cable = AXP288_EXTCON_CABLE_SDP;
> > +	} else if (chrg_type == DET_STAT_CDP) {
> > +		dev_dbg(info->dev, "cdp cable connecetd\n");
> > +		notify_otg = true;
> > +		notify_charger = true;
> > +		cable = AXP288_EXTCON_CABLE_CDP;
> > +	} else if (chrg_type == DET_STAT_DCP) {
> > +		dev_dbg(info->dev, "dcp cable connecetd\n");
> > +		notify_charger = true;
> > +		cable = AXP288_EXTCON_CABLE_DCP;
> > +	} else {
> > +		dev_warn(info->dev,
> > +			"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->pdata->gpio_mux_cntl != NULL)
> > +			gpiod_set_value(info->pdata->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)
> > +		extcon_set_cable_state(info->edev, cable, vbus_attach);
> > +
> > +	/* Clear the flags on disconnect event */
> > +	if (!vbus_attach) {
> > +		notify_otg = false;
> > +		notify_charger = false;
> > +	}
> > +
> > +	return 0;
> > +
> > +dev_det_ret:
> > +	if (ret < 0)
> > +		dev_warn(info->dev, "BC Mod detection error\n");
> > +
> > +	return ret;
> > +}
> > +
> > +static irqreturn_t axp288_extcon_isr(int irq, void *data) {
> > +	struct axp288_extcon_info *info = data;
> > +	unsigned int i;
> > +	int ret;
> > +
> > +	for (i = 0; i < EXTCON_IRQ_END; i++) {
> > +		if (info->irq[i] == irq)
> > +			break;
> > +	}
> > +
> > +	if (i == EXTCON_IRQ_END) {
> > +		dev_warn(info->dev, "spurious interrupt!!\n");
> > +		return IRQ_NONE;
> > +	}
> > +
> > +	ret = handle_chrg_det_event(info);
> > +	if (ret < 0)
> > +		dev_warn(info->dev, "error in PWRSRC INT handling\n");
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +static int axp288_handle_extcon_event(struct notifier_block *nb,
> > +				   unsigned long event, void *param) {
> > +	struct axp288_extcon_info *info =
> > +	    container_of(nb, struct axp288_extcon_info, extcon_nb);
> > +	struct extcon_dev *edev = param;
> > +	int usb_host = extcon_get_cable_state(edev, "USB-Host");
> > +
> > +	dev_dbg(info->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->usb_id_short = usb_host;
> > +
> > +	/*
> > +	 * Connect the USB mux to SOC in case of usb host else connect
> > +	 * it to PMIC.
> > +	 */
> > +	if (info->pdata->gpio_mux_cntl != NULL) {
> > +		if (info->usb_id_short)
> > +			gpiod_set_value(info->pdata->gpio_mux_cntl,
> > +					EXTCON_GPIO_MUX_SEL_SOC);
> > +		else
> > +			gpiod_set_value(info->pdata->gpio_mux_cntl,
> > +					EXTCON_GPIO_MUX_SEL_PMIC);
> > +	}
> > +
> > +	return NOTIFY_OK;
> > +}
> > +
> > +static int axp288_init_gpio_mux_cntl(struct axp288_extcon_info *info)
> > +{
> > +	int ret;
> > +
> > +	ret = gpio_request(desc_to_gpio(info->pdata->gpio_mux_cntl),
> "USB_MUX");
> > +	if (ret < 0) {
> > +		dev_err(info->dev, "Failed to request the gpio=%d\n",
> > +			desc_to_gpio(info->pdata->gpio_mux_cntl));
> > +		return ret;
> > +	}
> > +	gpiod_direction_output(info->pdata->gpio_mux_cntl,
> > +					EXTCON_GPIO_MUX_SEL_PMIC);
> > +
> > +	info->extcon_nb.notifier_call = axp288_handle_extcon_event;
> > +	ret = extcon_register_interest(&info->cable, NULL,
> > +				"USB-Host", &info->extcon_nb);
> > +	if (ret) {
> > +		dev_err(info->dev, "failed to register extcon notifier\n");
> > +		return ret;
> > +	}
> > +
> > +	if (info->cable.edev)
> > +		info->usb_id_short =
> > +			extcon_get_cable_state(info->cable.edev, "USB-Host");
> > +
> > +	if (info->usb_id_short)
> > +		gpiod_set_value(info->pdata->gpio_mux_cntl,
> > +					EXTCON_GPIO_MUX_SEL_SOC);
> > +
> > +	return 0;
> > +}
> 
> Currently, the only extcon provider driver are included in drivers/extcon.
> I'm reruclant to support extcon consumer feature in extcon directory.
> Also, this driver support both extcon provider feature and extcon consumer
> feature in one driver. It is one reluctant reason.
> 
> So, I suggest that you includes only extcon provider feature in this patchset.
> And then we will discuss the support of extcon consumer feature on later patch.
> 
> I will support the 'usb notifier chain' to detect both vbus and id pin state.
> you can refet to this discussion on patch[1].
> 	#define EXTCON_USB_ID_LOW
> 	#define EXTCON_USB_ID_HIGH
> 	#define EXTCON_USB_VBUS_LOW
> 	#define EXTCON_USB_VBUS_HIGH
> 
> [1] https://lkml.org/lkml/2015/4/16/116
Ok..fine.

> > +
> > +static int axp288_extcon_probe(struct platform_device *pdev) {
> > +	struct axp288_extcon_info *info;
> > +	struct axp20x_dev *axp20x = dev_get_drvdata(pdev->dev.parent);
> > +	int ret, i, pirq;
> > +
> > +	info = devm_kzalloc(&pdev->dev, sizeof(*info), GFP_KERNEL);
> > +	if (!info)
> > +		return -ENOMEM;
> > +
> > +	info->dev = &pdev->dev;
> > +	info->regmap = axp20x->regmap;
> > +	info->regmap_irqc = axp20x->regmap_irqc;
> > +	info->pdata = pdev->dev.platform_data;
> > +
> > +	if (!info->pdata) {
> > +		/* TODO: Try ACPI provided pdata via device properties */
> > +		if (!device_property_present(&pdev->dev,
> > +					"axp288_extcon_data\n"))
> > +			dev_err(&pdev->dev, "no platform data\n");
> > +		return -ENODEV;
> > +	}
> > +	platform_set_drvdata(pdev, info);
> > +
> > +	axp288_extcon_log_rsi(info, axp288_pwr_up_down_info,
> > +				AXP288_PS_BOOT_REASON_REG);
> > +
> > +	/* Initialize extcon device */
> > +	info->edev = devm_extcon_dev_allocate(&pdev->dev,
> > +					      axp288_extcon_cables);
> > +	if (IS_ERR(info->edev)) {
> > +		dev_err(&pdev->dev, "failed to allocate memory for extcon\n");
> > +		return -ENOMEM;
> 
> Use PTR_ERR(info->edev) instead of -ENOMEM
Ok..

> 
> > +	}
> > +
> > +	/* Register extcon device */
> > +	info->edev->name = dev_name(&pdev->dev);
> 
> Don't need to set the name of extcon device. extcon_dev_register() set the
> name of extcon device. So, I'll move the extcon_dev structure from extcon.h to
> extcon.c becaseu extcon_dev structure should be handled in extcon core.
Ok..

> 
> > +	ret = devm_extcon_dev_register(&pdev->dev, info->edev);
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "failed to register extcon device\n");
> > +		return ret;
> > +	}
> > +
> > +	/* Get otg transceiver phy */
> > +	info->otg = usb_get_phy(USB_PHY_TYPE_USB2);
> > +	if (IS_ERR(info->otg)) {
> > +		dev_warn(&pdev->dev, "Failed to get otg transceiver!!\n");
> 
> 'Failed' -> 'failed'. don't need the '!!' on end of debug log.
Ok..

> 
> > +		ret = PTR_ERR(info->otg);
> > +		return ret;
> > +	}
> > +
> > +	for (i = 0; i < EXTCON_IRQ_END; i++) {
> > +		pirq = platform_get_irq(pdev, i);
> > +		info->irq[i] = regmap_irq_get_virq(info->regmap_irqc, pirq);
> > +		if (info->irq[i] < 0) {
> > +			dev_warn(&pdev->dev,
> > +				"regmap_irq get virq failed for IRQ %d: %d\n",
> > +				pirq, info->irq[i]);
> > +			info->irq[i] = -1;
> > +			goto intr_reg_failed;
> > +		}
> 
> Need to add blank line
Not sure how it came here...i don't see the line in vim

> 
> > +		ret = devm_request_threaded_irq(&pdev->dev, info->irq[i],
> > +				NULL, axp288_extcon_isr,
> > +				IRQF_ONESHOT | IRQF_NO_SUSPEND,
> > +				pdev->name, info);
> > +		if (ret) {
> > +			dev_err(&pdev->dev, "request_irq fail :%d err:%d\n",
> > +							info->irq[i], ret);
> > +			goto intr_reg_failed;
> > +		}
> > +	}
> > +
> > +	/* Set up gpio control for USB Mux */
> > +	if (info->pdata->gpio_mux_cntl != NULL) {
> 
> Modify if statement as following simply:
> 	if (info->pdata->gpio_mux_cntl)
Ok..

> 
> > +		ret = axp288_init_gpio_mux_cntl(info);
> > +		if (ret < 0)
> > +			goto intr_reg_failed;
> > +	}
> > +
> > +	/* 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 complte 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);
> 
> I think that you better to move the initialization code before extcon registration.
> The initialization should be executed before extcon and interrupt registration.
My intention is to enable the interrupts once after setting all the interrupt and extcon handlers.

> 
> > +
> > +	return 0;
> > +
> > +intr_reg_failed:
> > +	usb_put_phy(info->otg);
> > +	return ret;
> > +}
> > +
> > +static int axp288_extcon_remove(struct platform_device *pdev) {
> > +	struct axp288_extcon_info *info = platform_get_drvdata(pdev);
> > +
> > +	usb_put_phy(info->otg);
> > +	return 0;
> > +}
> > +
> > +static struct platform_driver axp288_extcon_driver = {
> > +	.probe = axp288_extcon_probe,
> > +	.remove = axp288_extcon_remove,
> > +	.driver = {
> > +		.name = "axp288_extcon",
> > +	},
> 
> You should add the suspend/resume funciton to control the interrupt during
> suspend state. I discussed the same issue on patch[2].
> And you can refer to extcon-usb-gpio.c[3] for controlling interrupt in
> suspend/resume function.
> 
> [2] https://lkml.org/lkml/2015/1/27/1069
> [3]
> http://git.kernel.org/cgit/linux/kernel/git/chanwoo/extcon.git/commit/?h=extc
> on-next&id=e52817faae359ce95c93c2b6eb88b16d4b430181
The actual HW interrupt is controlled/handled by mfd driver. This driver only gets the virtual interrupts.
I don't see the need to enable/disable the interrupts in this case...

Thanks,
Ram

Re: [PATCH v3 2/2] extcon-axp288: Add axp288 extcon driver support

[Chanwoo Choi]

Hi Pallala,

On 04/27/2015 07:44 PM, Pallala, Ramakrishna wrote:
> Hi Choi,
> 
> Please find my response below. 
> 
>> On 04/09/2015 02:12 AM, Ramakrishna Pallala wrote:
>>> This patch adds the extcon support for AXP288 PMIC which has the BC1.2
>>> charger detection capability. Additionally it also adds the USB mux
>>> switching support b/w SOC and PMIC based on GPIO control.
>>>
>>> Signed-off-by: Ramakrishna Pallala <ramakrishna.pallala@intel.com>
>>> ---
>>>  drivers/extcon/Kconfig         |    7 +
>>>  drivers/extcon/Makefile        |    1 +
>>>  drivers/extcon/extcon-axp288.c |  462
>> ++++++++++++++++++++++++++++++++++++++++
>>>  include/linux/mfd/axp20x.h     |    5 +
>>>  4 files changed, 475 insertions(+)
>>>  create mode 100644 drivers/extcon/extcon-axp288.c

[snip]

>>> +enum axp288_extcon_reg {
>>> +	AXP288_PS_STAT_REG		= 0x00,
>>> +	AXP288_PS_BOOT_REASON_REG	= 0x02,
>>> +	AXP288_BC_GLOBAL_REG		= 0x2c,
>>> +	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,
>>> +};
>>> +
>>> +#define AXP288_DRV_NAME			"axp288_extcon"
>>
>> Use the '-' instead of '_'
>> - axp288_extcon -> axp288-extcon
> 
> All axp20x pmic mfd cell names are starting with "axp288_*" this one comment I got from Lee Jones on mfd patch.

OK.

[snip]

>>> +struct axp288_extcon_info {
>>> +	struct device *dev;
>>> +	struct regmap *regmap;
>>> +	struct regmap_irq_chip_data *regmap_irqc;
>>> +	struct axp288_extcon_pdata *pdata;
>>> +	int irq[EXTCON_IRQ_END];
>>> +	struct extcon_dev *edev;
>>> +	struct usb_phy *otg;
>>> +	struct notifier_block extcon_nb;
>>> +	struct extcon_specific_cable_nb cable;
>>> +	/* detect OTG or ID events */
>>
>> This driver have the feature of both extcon provider driver and extcon consumer
>> driver.
> Yes, it's acts as both...I will remove the consumer support from this patch as you suggested in later comments.

OK.

[snip]
> 
>>
>>> +		ret = PTR_ERR(info->otg);
>>> +		return ret;
>>> +	}
>>> +
>>> +	for (i = 0; i < EXTCON_IRQ_END; i++) {
>>> +		pirq = platform_get_irq(pdev, i);
>>> +		info->irq[i] = regmap_irq_get_virq(info->regmap_irqc, pirq);
>>> +		if (info->irq[i] < 0) {
>>> +			dev_warn(&pdev->dev,
>>> +				"regmap_irq get virq failed for IRQ %d: %d\n",
>>> +				pirq, info->irq[i]);
>>> +			info->irq[i] = -1;
>>> +			goto intr_reg_failed;
>>> +		}
>>
>> Need to add blank line
> Not sure how it came here...i don't see the line in vim

I think that need the blank line between '}' and devm_request_threaded_irq() sentence.

> 
>>
>>> +		ret = devm_request_threaded_irq(&pdev->dev, info->irq[i],
>>> +				NULL, axp288_extcon_isr,
>>> +				IRQF_ONESHOT | IRQF_NO_SUSPEND,
>>> +				pdev->name, info);
>>> +		if (ret) {
>>> +			dev_err(&pdev->dev, "request_irq fail :%d err:%d\n",
>>> +							info->irq[i], ret);
>>> +			goto intr_reg_failed;
>>> +		}
>>> +	}
>>> +
>>> +	/* Set up gpio control for USB Mux */
>>> +	if (info->pdata->gpio_mux_cntl != NULL) {
>>
>> Modify if statement as following simply:
>> 	if (info->pdata->gpio_mux_cntl)
> Ok..
> 
>>
>>> +		ret = axp288_init_gpio_mux_cntl(info);
>>> +		if (ret < 0)
>>> +			goto intr_reg_failed;
>>> +	}
>>> +
>>> +	/* 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 complte 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);
>>
>> I think that you better to move the initialization code before extcon registration.
>> The initialization should be executed before extcon and interrupt registration.
> My intention is to enable the interrupts once after setting all the interrupt and extcon handlers.

OK.
But, I'd like you to add following functions to include the init code for enabling interrupt.
	axp288_extcon_enable_irq() or axp288_extcon_init()
	
> 
>>
>>> +
>>> +	return 0;
>>> +
>>> +intr_reg_failed:
>>> +	usb_put_phy(info->otg);
>>> +	return ret;
>>> +}
>>> +
>>> +static int axp288_extcon_remove(struct platform_device *pdev) {
>>> +	struct axp288_extcon_info *info = platform_get_drvdata(pdev);
>>> +
>>> +	usb_put_phy(info->otg);
>>> +	return 0;
>>> +}
>>> +
>>> +static struct platform_driver axp288_extcon_driver = {
>>> +	.probe = axp288_extcon_probe,
>>> +	.remove = axp288_extcon_remove,
>>> +	.driver = {
>>> +		.name = "axp288_extcon",
>>> +	},
>>
>> You should add the suspend/resume funciton to control the interrupt during
>> suspend state. I discussed the same issue on patch[2].
>> And you can refer to extcon-usb-gpio.c[3] for controlling interrupt in
>> suspend/resume function.
>>
>> [2] https://lkml.org/lkml/2015/1/27/1069
>> [3]
>> http://git.kernel.org/cgit/linux/kernel/git/chanwoo/extcon.git/commit/?h=extc
>> on-next&id=e52817faae359ce95c93c2b6eb88b16d4b430181
> The actual HW interrupt is controlled/handled by mfd driver. This driver only gets the virtual interrupts.
> I don't see the need to enable/disable the interrupts in this case...

OK. I understand.

Thanks,
Chanwoo Choi

RE: [PATCH v3 2/2] extcon-axp288: Add axp288 extcon driver support

[Pallala, Ramakrishna]

Hi Choi,

Please see my response below.

> >>
> >>> +		ret = PTR_ERR(info->otg);
> >>> +		return ret;
> >>> +	}
> >>> +
> >>> +	for (i = 0; i < EXTCON_IRQ_END; i++) {
> >>> +		pirq = platform_get_irq(pdev, i);
> >>> +		info->irq[i] = regmap_irq_get_virq(info->regmap_irqc, pirq);
> >>> +		if (info->irq[i] < 0) {
> >>> +			dev_warn(&pdev->dev,
> >>> +				"regmap_irq get virq failed for IRQ %d: %d\n",
> >>> +				pirq, info->irq[i]);
> >>> +			info->irq[i] = -1;
> >>> +			goto intr_reg_failed;
> >>> +		}
> >>
> >> Need to add blank line
> > Not sure how it came here...i don't see the line in vim
> 
> I think that need the blank line between '}' and devm_request_threaded_irq()
> sentence.
Ok.

> >>> +
> >>> +	/* 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 complte 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);
> >>
> >> I think that you better to move the initialization code before extcon
> registration.
> >> The initialization should be executed before extcon and interrupt registration.
> > My intention is to enable the interrupts once after setting all the interrupt and
> extcon handlers.
> 
> OK.
> But, I'd like you to add following functions to include the init code for enabling
> interrupt.
> 	axp288_extcon_enable_irq() or axp288_extcon_init()
> 
Ok I will move the interrupt enabling settings to axp288_extcon_enable_irq()

One more thing, mfd cell changes are already merged by Lee Jones so I will only push the
extcon-axp288 driver patch under v4.  Hope this is ok...

Thanks,
Ram

Re: [PATCH v3 2/2] extcon-axp288: Add axp288 extcon driver support

[Chanwoo Choi]

On Mon, Apr 27, 2015 at 10:19 PM, Pallala, Ramakrishna
<ramakrishna.pallala@intel.com> wrote:
> Hi Choi,
>
> Please see my response below.
>
>> >>
>> >>> +         ret = PTR_ERR(info->otg);
>> >>> +         return ret;
>> >>> + }
>> >>> +
>> >>> + for (i = 0; i < EXTCON_IRQ_END; i++) {
>> >>> +         pirq = platform_get_irq(pdev, i);
>> >>> +         info->irq[i] = regmap_irq_get_virq(info->regmap_irqc, pirq);
>> >>> +         if (info->irq[i] < 0) {
>> >>> +                 dev_warn(&pdev->dev,
>> >>> +                         "regmap_irq get virq failed for IRQ %d: %d\n",
>> >>> +                         pirq, info->irq[i]);
>> >>> +                 info->irq[i] = -1;
>> >>> +                 goto intr_reg_failed;
>> >>> +         }
>> >>
>> >> Need to add blank line
>> > Not sure how it came here...i don't see the line in vim
>>
>> I think that need the blank line between '}' and devm_request_threaded_irq()
>> sentence.
> Ok.
>
>> >>> +
>> >>> + /* 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 complte 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);
>> >>
>> >> I think that you better to move the initialization code before extcon
>> registration.
>> >> The initialization should be executed before extcon and interrupt registration.
>> > My intention is to enable the interrupts once after setting all the interrupt and
>> extcon handlers.
>>
>> OK.
>> But, I'd like you to add following functions to include the init code for enabling
>> interrupt.
>>       axp288_extcon_enable_irq() or axp288_extcon_init()
>>
> Ok I will move the interrupt enabling settings to axp288_extcon_enable_irq()
>
> One more thing, mfd cell changes are already merged by Lee Jones so I will only push the
> extcon-axp288 driver patch under v4.  Hope this is ok...

OK.

Thanks,
Chanwoo Choi