[PATCH v2 0/4] usb: added palmas-usb driver and a few misc fixes

[PATCH v2 0/4] usb: added palmas-usb driver and a few misc fixes

[Kishon Vijay Abraham I]

Added palmas-usb driver which is mainly used as comparator driver to
detect vbus/id events when a USB cable is connected and passes on the
event information to omap glue (dwc3-omap.c)

The other fixes include setting dma_mask for dwc3 device since device
tree doesn't fill dma_mask, returning EPROBE_DEFER if probe has not yet
called and replace *_* with *-* in property names in musb glue since
that is the usual convention followed.

Developed this patches on
git://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git testing

Changes from v1:
* set the dma_mask for dwc3_omap (instead of setting dma_mask for dwc3
core from dwc3-omap.c)
* return '0' from dwc3_omap_mailbox on success (instead of hacky IRQ_HANDLED)
It is now handled using mailboxstat member in palmas_usb.
* Made compatible in palmas-usb to include *ti,twl6035-usb*

Graeme Gregory (1):
  USB: Palmas OTG Transceiver Driver

Kishon Vijay Abraham I (3):
  usb: dwc3: set dma_mask for dwc3_omap device
  usb: dwc3: dwc3-omap: return -EPROBE_DEFER if probe has not yet
    executed
  usb: musb: omap2430: replace *_* with *-* in property names

 Documentation/devicetree/bindings/usb/omap-usb.txt |   12 +-
 .../devicetree/bindings/usb/twlxxxx-usb.txt        |   15 +
 drivers/usb/dwc3/core.c                            |    4 +
 drivers/usb/dwc3/dwc3-omap.c                       |   10 +-
 drivers/usb/musb/omap2430.c                        |    6 +-
 drivers/usb/otg/Kconfig                            |    6 +
 drivers/usb/otg/Makefile                           |    1 +
 drivers/usb/otg/palmas-usb.c                       |  396 ++++++++++++++++++++
 include/linux/mfd/palmas.h                         |    7 +-
 include/linux/usb/dwc3-omap.h                      |    6 +-
 10 files changed, 448 insertions(+), 15 deletions(-)
 create mode 100644 drivers/usb/otg/palmas-usb.c

-- 
1.7.10.4

[PATCH v2 1/4] usb: dwc3: set dma_mask for dwc3_omap device

[Kishon Vijay Abraham I]

*dma_mask* is not set for devices created from dt data. So filled dma_mask
for dwc3_omap device here. And dwc3 core will copy the dma_mask from its
parent.

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 drivers/usb/dwc3/core.c      |    4 ++++
 drivers/usb/dwc3/dwc3-omap.c |    3 +++
 2 files changed, 7 insertions(+)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 66c0572..c845e70 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -454,6 +454,10 @@ static int dwc3_probe(struct platform_device *pdev)
 	dwc->regs_size	= resource_size(res);
 	dwc->dev	= dev;
 
+	dev->dma_mask	= dev->parent->dma_mask;
+	dev->dma_parms	= dev->parent->dma_parms;
+	dma_set_coherent_mask(dev, dev->parent->coherent_dma_mask);
+
 	if (!strncmp("super", maximum_speed, 5))
 		dwc->maximum_speed = DWC3_DCFG_SUPERSPEED;
 	else if (!strncmp("high", maximum_speed, 4))
diff --git a/drivers/usb/dwc3/dwc3-omap.c b/drivers/usb/dwc3/dwc3-omap.c
index 35b9673..546f1fd 100644
--- a/drivers/usb/dwc3/dwc3-omap.c
+++ b/drivers/usb/dwc3/dwc3-omap.c
@@ -277,6 +277,8 @@ static void dwc3_omap_disable_irqs(struct dwc3_omap *omap)
 	dwc3_omap_writel(omap->base, USBOTGSS_IRQENABLE_SET_0, 0x00);
 }
 
+static u64 dwc3_omap_dma_mask = DMA_BIT_MASK(32);
+
 static int dwc3_omap_probe(struct platform_device *pdev)
 {
 	struct device_node	*node = pdev->dev.of_node;
@@ -330,6 +332,7 @@ static int dwc3_omap_probe(struct platform_device *pdev)
 	omap->dev	= dev;
 	omap->irq	= irq;
 	omap->base	= base;
+	dev->dma_mask	= &dwc3_omap_dma_mask;
 
 	/*
 	 * REVISIT if we ever have two instances of the wrapper, we will be
-- 
1.7.10.4

[PATCH v2 2/4] usb: dwc3: dwc3-omap: return -EPROBE_DEFER if probe has not yet executed

[Kishon Vijay Abraham I]

return -EPROBE_DEFER from dwc3_omap_mailbox in dwc3-omap.c, if the probe of
dwc3-omap has not yet been executed or failed.

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 drivers/usb/dwc3/dwc3-omap.c  |    7 +++++--
 include/linux/usb/dwc3-omap.h |    6 +++---
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/dwc3/dwc3-omap.c b/drivers/usb/dwc3/dwc3-omap.c
index 546f1fd..2fe9723f 100644
--- a/drivers/usb/dwc3/dwc3-omap.c
+++ b/drivers/usb/dwc3/dwc3-omap.c
@@ -138,11 +138,14 @@ static inline void dwc3_omap_writel(void __iomem *base, u32 offset, u32 value)
 	writel(value, base + offset);
 }
 
-void dwc3_omap_mailbox(enum omap_dwc3_vbus_id_status status)
+int dwc3_omap_mailbox(enum omap_dwc3_vbus_id_status status)
 {
 	u32			val;
 	struct dwc3_omap	*omap = _omap;
 
+	if (!omap)
+		return -EPROBE_DEFER;
+
 	switch (status) {
 	case OMAP_DWC3_ID_GROUND:
 		dev_dbg(omap->dev, "ID GND\n");
@@ -185,7 +188,7 @@ void dwc3_omap_mailbox(enum omap_dwc3_vbus_id_status status)
 		dev_dbg(omap->dev, "ID float\n");
 	}
 
-	return;
+	return 0;
 }
 EXPORT_SYMBOL_GPL(dwc3_omap_mailbox);
 
diff --git a/include/linux/usb/dwc3-omap.h b/include/linux/usb/dwc3-omap.h
index 51eae14..5615f4d 100644
--- a/include/linux/usb/dwc3-omap.h
+++ b/include/linux/usb/dwc3-omap.h
@@ -19,11 +19,11 @@ enum omap_dwc3_vbus_id_status {
 };
 
 #if (defined(CONFIG_USB_DWC3) || defined(CONFIG_USB_DWC3_MODULE))
-extern void dwc3_omap_mailbox(enum omap_dwc3_vbus_id_status status);
+extern int dwc3_omap_mailbox(enum omap_dwc3_vbus_id_status status);
 #else
-static inline void dwc3_omap_mailbox(enum omap_dwc3_vbus_id_status status)
+static inline int dwc3_omap_mailbox(enum omap_dwc3_vbus_id_status status)
 {
-	return;
+	return -ENODEV;
 }
 #endif
 
-- 
1.7.10.4

[PATCH v2 3/4] USB: Palmas OTG Transceiver Driver

[Kishon Vijay Abraham I]

From: Graeme Gregory <gg@slimlogic.co.uk>

This is the driver for the OTG transceiver built into the Palmas chip. It
handles the various USB OTG events that can be generated by cable
insertion/removal.

Signed-off-by: Graeme Gregory <gg@slimlogic.co.uk>
Signed-off-by: Moiz Sonasath <m-sonasath@ti.com>
Signed-off-by: Ruchika Kharwar <ruchika@ti.com>
Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
Signed-off-by: Sebastien Guiriec <s-guiriec@ti.com>
---
 .../devicetree/bindings/usb/twlxxxx-usb.txt        |   15 +
 drivers/usb/otg/Kconfig                            |    6 +
 drivers/usb/otg/Makefile                           |    1 +
 drivers/usb/otg/palmas-usb.c                       |  396 ++++++++++++++++++++
 include/linux/mfd/palmas.h                         |    7 +-
 5 files changed, 424 insertions(+), 1 deletion(-)
 create mode 100644 drivers/usb/otg/palmas-usb.c

diff --git a/Documentation/devicetree/bindings/usb/twlxxxx-usb.txt b/Documentation/devicetree/bindings/usb/twlxxxx-usb.txt
index 36b9aed..17a9194 100644
--- a/Documentation/devicetree/bindings/usb/twlxxxx-usb.txt
+++ b/Documentation/devicetree/bindings/usb/twlxxxx-usb.txt
@@ -38,3 +38,18 @@ twl4030-usb {
 	usb3v1-supply = <&vusb3v1>;
 	usb_mode = <1>;
 };
+
+PALMAS USB COMPARATOR
+Required Properties:
+ - compatible : Should be "ti,palmas-usb" or "ti,twl6035-usb"
+ - vbus-supply : phandle to the regulator device tree node.
+
+Optional Properties:
+ - ti,wakeup : To enable the wakeup comparator in probe
+ - ti,no_control_vbus: if the platform wishes its own vbus control
+
+palmas-usb {
+	compatible = "ti,twl6035-usb", "ti,palmas-usb";
+	vbus-supply = <&smps10_reg>;
+	ti,wakeup;
+};
diff --git a/drivers/usb/otg/Kconfig b/drivers/usb/otg/Kconfig
index 37962c9..5b40e04 100644
--- a/drivers/usb/otg/Kconfig
+++ b/drivers/usb/otg/Kconfig
@@ -138,4 +138,10 @@ config USB_MV_OTG
 
 	  To compile this driver as a module, choose M here.
 
+config PALMAS_USB
+	tristate "Palmas USB Transceiver Driver"
+	depends on MFD_PALMAS
+	help
+	  Enable this to support the Palmas OTG transceiver
+
 endif # USB || OTG
diff --git a/drivers/usb/otg/Makefile b/drivers/usb/otg/Makefile
index a844b8d..7ae90ba 100644
--- a/drivers/usb/otg/Makefile
+++ b/drivers/usb/otg/Makefile
@@ -22,3 +22,4 @@ fsl_usb2_otg-objs		:= fsl_otg.o otg_fsm.o
 obj-$(CONFIG_FSL_USB2_OTG)	+= fsl_usb2_otg.o
 obj-$(CONFIG_USB_MXS_PHY)	+= mxs-phy.o
 obj-$(CONFIG_USB_MV_OTG)	+= mv_otg.o
+obj-$(CONFIG_PALMAS_USB)	+= palmas-usb.o
diff --git a/drivers/usb/otg/palmas-usb.c b/drivers/usb/otg/palmas-usb.c
new file mode 100644
index 0000000..8bdffe3
--- /dev/null
+++ b/drivers/usb/otg/palmas-usb.c
@@ -0,0 +1,396 @@
+/*
+ * Palmas USB transceiver driver
+ *
+ * Copyright (C) 2013 Texas Instruments Incorporated - http://www.ti.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.
+ *
+ * Author: Graeme Gregory <gg@slimlogic.co.uk>
+ * Author: Kishon Vijay Abraham I <kishon@ti.com>
+ *
+ * Based on twl6030_usb.c
+ *
+ * Author: Hema HK <hemahk@ti.com>
+ *
+ * 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/init.h>
+#include <linux/interrupt.h>
+#include <linux/platform_device.h>
+#include <linux/io.h>
+#include <linux/usb/otg.h>
+#include <linux/usb/phy_companion.h>
+#include <linux/usb/omap_usb.h>
+#include <linux/usb/dwc3-omap.h>
+#include <linux/regulator/consumer.h>
+#include <linux/err.h>
+#include <linux/notifier.h>
+#include <linux/slab.h>
+#include <linux/delay.h>
+#include <linux/mfd/palmas.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
+
+static int palmas_usb_read(struct palmas *palmas, unsigned int reg,
+		unsigned int *dest)
+{
+	unsigned int addr;
+	int slave;
+
+	slave = PALMAS_BASE_TO_SLAVE(PALMAS_USB_OTG_BASE);
+	addr = PALMAS_BASE_TO_REG(PALMAS_USB_OTG_BASE, reg);
+
+	return regmap_read(palmas->regmap[slave], addr, dest);
+}
+
+static int palmas_usb_write(struct palmas *palmas, unsigned int reg,
+		unsigned int data)
+{
+	unsigned int addr;
+	int slave;
+
+	slave = PALMAS_BASE_TO_SLAVE(PALMAS_USB_OTG_BASE);
+	addr = PALMAS_BASE_TO_REG(PALMAS_USB_OTG_BASE, reg);
+
+	return regmap_write(palmas->regmap[slave], addr, data);
+}
+
+static void palmas_usb_wakeup(struct palmas *palmas, int enable)
+{
+	if (enable)
+		palmas_usb_write(palmas, PALMAS_USB_WAKEUP,
+				PALMAS_USB_WAKEUP_ID_WK_UP_COMP);
+	else
+		palmas_usb_write(palmas, PALMAS_USB_WAKEUP, 0);
+}
+
+static ssize_t palmas_usb_vbus_show(struct device *dev,
+			struct device_attribute *attr, char *buf)
+{
+	unsigned long		flags;
+	int			ret = -EINVAL;
+	struct palmas_usb	*palmas_usb = dev_get_drvdata(dev);
+
+	spin_lock_irqsave(&palmas_usb->lock, flags);
+
+	switch (palmas_usb->linkstat) {
+	case OMAP_DWC3_VBUS_VALID:
+	       ret = snprintf(buf, PAGE_SIZE, "vbus\n");
+	       break;
+	case OMAP_DWC3_ID_GROUND:
+	       ret = snprintf(buf, PAGE_SIZE, "id\n");
+	       break;
+	case OMAP_DWC3_ID_FLOAT:
+	case OMAP_DWC3_VBUS_OFF:
+	       ret = snprintf(buf, PAGE_SIZE, "none\n");
+	       break;
+	default:
+	       ret = snprintf(buf, PAGE_SIZE, "UNKNOWN\n");
+	}
+	spin_unlock_irqrestore(&palmas_usb->lock, flags);
+
+	return ret;
+}
+static DEVICE_ATTR(vbus, 0444, palmas_usb_vbus_show, NULL);
+
+static irqreturn_t palmas_vbus_wakeup_irq(int irq, void *_palmas_usb)
+{
+	struct palmas_usb *palmas_usb = _palmas_usb;
+	enum omap_dwc3_vbus_id_status status = OMAP_DWC3_UNKNOWN;
+	int slave;
+	unsigned int vbus_line_state, addr;
+
+	slave = PALMAS_BASE_TO_SLAVE(PALMAS_INTERRUPT_BASE);
+	addr = PALMAS_BASE_TO_REG(PALMAS_INTERRUPT_BASE,
+			PALMAS_INT3_LINE_STATE);
+
+	regmap_read(palmas_usb->palmas->regmap[slave], addr, &vbus_line_state);
+
+	if (vbus_line_state & PALMAS_INT3_LINE_STATE_VBUS) {
+		if (palmas_usb->linkstat != OMAP_DWC3_VBUS_VALID) {
+			if (!IS_ERR_OR_NULL(palmas_usb->vbus_reg))
+				regulator_enable(palmas_usb->vbus_reg);
+			status = OMAP_DWC3_VBUS_VALID;
+		} else {
+			dev_dbg(palmas_usb->dev,
+				"Spurious connect event detected\n");
+		}
+	} else if (!(vbus_line_state & PALMAS_INT3_LINE_STATE_VBUS)) {
+		if (palmas_usb->linkstat == OMAP_DWC3_VBUS_VALID) {
+			if (!IS_ERR_OR_NULL(palmas_usb->vbus_reg))
+				regulator_disable(palmas_usb->vbus_reg);
+			status = OMAP_DWC3_VBUS_OFF;
+		} else {
+			dev_dbg(palmas_usb->dev,
+				"Spurious disconnect event detected\n");
+		}
+	}
+
+	if (status != OMAP_DWC3_UNKNOWN) {
+		palmas_usb->linkstat = status;
+		palmas_usb->mailboxstat = dwc3_omap_mailbox(status);
+		return IRQ_HANDLED;
+	}
+
+	return IRQ_NONE;
+}
+
+static irqreturn_t palmas_id_wakeup_irq(int irq, void *_palmas_usb)
+{
+	enum omap_dwc3_vbus_id_status status = OMAP_DWC3_UNKNOWN;
+	unsigned int		set;
+	struct palmas_usb	*palmas_usb = _palmas_usb;
+
+	palmas_usb_read(palmas_usb->palmas, PALMAS_USB_ID_INT_LATCH_SET, &set);
+
+	if (set & PALMAS_USB_ID_INT_SRC_ID_GND) {
+		if (!IS_ERR_OR_NULL(palmas_usb->vbus_reg))
+			regulator_enable(palmas_usb->vbus_reg);
+		palmas_usb_write(palmas_usb->palmas,
+					PALMAS_USB_ID_INT_EN_HI_SET,
+					PALMAS_USB_ID_INT_EN_HI_SET_ID_FLOAT);
+		palmas_usb_write(palmas_usb->palmas,
+					PALMAS_USB_ID_INT_EN_HI_CLR,
+					PALMAS_USB_ID_INT_EN_HI_CLR_ID_GND);
+		status = OMAP_DWC3_ID_GROUND;
+	} else if (set & PALMAS_USB_ID_INT_SRC_ID_FLOAT) {
+		palmas_usb_write(palmas_usb->palmas,
+					PALMAS_USB_ID_INT_EN_HI_SET,
+					PALMAS_USB_ID_INT_EN_HI_SET_ID_GND);
+		palmas_usb_write(palmas_usb->palmas,
+					PALMAS_USB_ID_INT_EN_HI_CLR,
+					PALMAS_USB_ID_INT_EN_HI_CLR_ID_FLOAT);
+		if (!IS_ERR_OR_NULL(palmas_usb->vbus_reg))
+			regulator_disable(palmas_usb->vbus_reg);
+		status = OMAP_DWC3_ID_FLOAT;
+	}
+
+	if (status != OMAP_DWC3_UNKNOWN) {
+		palmas_usb->linkstat = status;
+		palmas_usb->mailboxstat = dwc3_omap_mailbox(status);
+		return IRQ_HANDLED;
+	}
+
+	return IRQ_NONE;
+}
+
+static int palmas_enable_irq(struct palmas_usb *palmas_usb)
+{
+	palmas_usb_write(palmas_usb->palmas, PALMAS_USB_VBUS_CTRL_SET,
+			PALMAS_USB_VBUS_CTRL_SET_VBUS_ACT_COMP);
+
+	palmas_usb_write(palmas_usb->palmas, PALMAS_USB_ID_CTRL_SET,
+			PALMAS_USB_ID_CTRL_SET_ID_ACT_COMP);
+
+	palmas_usb_write(palmas_usb->palmas, PALMAS_USB_ID_INT_EN_HI_SET,
+			PALMAS_USB_ID_INT_EN_HI_SET_ID_GND);
+
+	palmas_vbus_wakeup_irq(palmas_usb->irq4, palmas_usb);
+	palmas_id_wakeup_irq(palmas_usb->irq2, palmas_usb);
+
+	return palmas_usb->mailboxstat;
+}
+
+static void palmas_set_vbus_work(struct work_struct *data)
+{
+	struct palmas_usb *palmas_usb = container_of(data, struct palmas_usb,
+								set_vbus_work);
+
+	if (IS_ERR_OR_NULL(palmas_usb->vbus_reg)) {
+		dev_err(palmas_usb->dev, "invalid regulator\n");
+		return;
+	}
+
+	/*
+	 * Start driving VBUS. Set OPA_MODE bit in CHARGERUSB_CTRL1
+	 * register. This enables boost mode.
+	 */
+
+	if (palmas_usb->vbus_enable)
+		regulator_enable(palmas_usb->vbus_reg);
+	else
+		regulator_disable(palmas_usb->vbus_reg);
+}
+
+static int palmas_set_vbus(struct phy_companion *comparator, bool enabled)
+{
+	struct palmas_usb *palmas_usb = comparator_to_palmas(comparator);
+
+	palmas_usb->vbus_enable = enabled;
+	schedule_work(&palmas_usb->set_vbus_work);
+
+	return 0;
+}
+
+static int palmas_start_srp(struct phy_companion *comparator)
+{
+	struct palmas_usb *palmas_usb = comparator_to_palmas(comparator);
+
+	palmas_usb_write(palmas_usb->palmas, PALMAS_USB_VBUS_CTRL_SET,
+			PALMAS_USB_VBUS_CTRL_SET_VBUS_DISCHRG |
+			PALMAS_USB_VBUS_CTRL_SET_VBUS_IADP_SINK);
+	palmas_usb_write(palmas_usb->palmas, PALMAS_USB_VBUS_CTRL_SET,
+			PALMAS_USB_VBUS_CTRL_SET_VBUS_CHRG_VSYS |
+			PALMAS_USB_VBUS_CTRL_SET_VBUS_IADP_SINK);
+
+	mdelay(100);
+
+	palmas_usb_write(palmas_usb->palmas, PALMAS_USB_VBUS_CTRL_CLR,
+			PALMAS_USB_VBUS_CTRL_SET_VBUS_CHRG_VSYS |
+			PALMAS_USB_VBUS_CTRL_SET_VBUS_CHRG_VSYS);
+
+	return 0;
+}
+
+static void palmas_dt_to_pdata(struct device_node *node,
+		struct palmas_usb_platform_data *pdata)
+{
+	pdata->no_control_vbus = of_property_read_bool(node,
+					"ti,no_control_vbus");
+	pdata->wakeup = of_property_read_bool(node, "ti,wakeup");
+}
+
+static int palmas_usb_probe(struct platform_device *pdev)
+{
+	u32 ret;
+	struct palmas *palmas = dev_get_drvdata(pdev->dev.parent);
+	struct palmas_usb_platform_data	*pdata = pdev->dev.platform_data;
+	struct device_node *node = pdev->dev.of_node;
+	struct palmas_usb *palmas_usb;
+	int status;
+
+	if (node && !pdata) {
+		pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
+
+		if (!pdata)
+			return -ENOMEM;
+
+		palmas_dt_to_pdata(node, pdata);
+	}
+
+	if (!pdata)
+		return -EINVAL;
+
+	palmas_usb = devm_kzalloc(&pdev->dev, sizeof(*palmas_usb), GFP_KERNEL);
+	if (!palmas_usb)
+		return -ENOMEM;
+
+	palmas->usb		= palmas_usb;
+	palmas_usb->palmas	= palmas;
+
+	palmas_usb->dev		= &pdev->dev;
+
+	palmas_usb->irq1 = regmap_irq_get_virq(palmas->irq_data,
+						PALMAS_ID_OTG_IRQ);
+	palmas_usb->irq2 = regmap_irq_get_virq(palmas->irq_data,
+						PALMAS_ID_IRQ);
+	palmas_usb->irq3 = regmap_irq_get_virq(palmas->irq_data,
+						PALMAS_VBUS_OTG_IRQ);
+	palmas_usb->irq4 = regmap_irq_get_virq(palmas->irq_data,
+						PALMAS_VBUS_IRQ);
+
+	palmas_usb->comparator.set_vbus	= palmas_set_vbus;
+	palmas_usb->comparator.start_srp = palmas_start_srp;
+
+	ret = omap_usb2_set_comparator(&palmas_usb->comparator);
+	if (ret == -ENODEV)
+		return -EPROBE_DEFER;
+
+	palmas_usb_wakeup(palmas, pdata->wakeup);
+
+	/* init spinlock for workqueue */
+	spin_lock_init(&palmas_usb->lock);
+
+	if (!pdata->no_control_vbus) {
+		palmas_usb->vbus_reg = devm_regulator_get(&pdev->dev, "vbus");
+		if (IS_ERR(palmas_usb->vbus_reg)) {
+			dev_err(&pdev->dev, "vbus init failed\n");
+			return PTR_ERR(palmas_usb->vbus_reg);
+		}
+	}
+
+	platform_set_drvdata(pdev, palmas_usb);
+
+	if (device_create_file(&pdev->dev, &dev_attr_vbus))
+		dev_warn(&pdev->dev, "could not create sysfs file\n");
+
+	/* init spinlock for workqueue */
+	spin_lock_init(&palmas_usb->lock);
+
+	INIT_WORK(&palmas_usb->set_vbus_work, palmas_set_vbus_work);
+
+	status = devm_request_threaded_irq(palmas_usb->dev, palmas_usb->irq2,
+			NULL, palmas_id_wakeup_irq,
+			IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING,
+			"palmas_usb", palmas_usb);
+	if (status < 0) {
+		dev_err(&pdev->dev, "can't get IRQ %d, err %d\n",
+					palmas_usb->irq2, status);
+		goto fail_irq;
+	}
+
+	status = devm_request_threaded_irq(palmas_usb->dev, palmas_usb->irq4,
+			NULL, palmas_vbus_wakeup_irq,
+			IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING,
+			"palmas_usb", palmas_usb);
+	if (status < 0) {
+		dev_err(&pdev->dev, "can't get IRQ %d, err %d\n",
+					palmas_usb->irq4, status);
+		goto fail_irq;
+	}
+
+	status = palmas_enable_irq(palmas_usb);
+	if (status < 0) {
+		dev_dbg(&pdev->dev, "enable irq failed\n");
+		goto fail_irq;
+	}
+
+	return 0;
+
+fail_irq:
+	cancel_work_sync(&palmas_usb->set_vbus_work);
+	device_remove_file(palmas_usb->dev, &dev_attr_vbus);
+
+	return status;
+}
+
+static int palmas_usb_remove(struct platform_device *pdev)
+{
+	struct palmas_usb *palmas_usb = platform_get_drvdata(pdev);
+
+	device_remove_file(palmas_usb->dev, &dev_attr_vbus);
+	cancel_work_sync(&palmas_usb->set_vbus_work);
+
+	return 0;
+}
+
+static struct of_device_id of_palmas_match_tbl[] = {
+	{ .compatible = "ti,palmas-usb", },
+	{ .compatible = "ti,twl6035-usb", },
+	{ /* end */ }
+};
+
+static struct platform_driver palmas_usb_driver = {
+	.probe = palmas_usb_probe,
+	.remove = palmas_usb_remove,
+	.driver = {
+		.name = "palmas-usb",
+		.of_match_table = of_palmas_match_tbl,
+		.owner = THIS_MODULE,
+	},
+};
+
+module_platform_driver(palmas_usb_driver);
+
+MODULE_ALIAS("platform:palmas-usb");
+MODULE_AUTHOR("Graeme Gregory <gg@slimlogic.co.uk>");
+MODULE_DESCRIPTION("Palmas USB transceiver driver");
+MODULE_LICENSE("GPL");
+MODULE_DEVICE_TABLE(of, of_palmas_match_tbl);
diff --git a/include/linux/mfd/palmas.h b/include/linux/mfd/palmas.h
index a4d13d7..3342017 100644
--- a/include/linux/mfd/palmas.h
+++ b/include/linux/mfd/palmas.h
@@ -19,6 +19,8 @@
 #include <linux/leds.h>
 #include <linux/regmap.h>
 #include <linux/regulator/driver.h>
+#include <linux/usb/phy_companion.h>
+#include <linux/usb/dwc3-omap.h>
 
 #define PALMAS_NUM_CLIENTS		3
 
@@ -341,6 +343,8 @@ struct palmas_usb {
 	struct palmas *palmas;
 	struct device *dev;
 
+	struct phy_companion comparator;
+
 	/* for vbus reporting with irqs disabled */
 	spinlock_t lock;
 
@@ -356,7 +360,8 @@ struct palmas_usb {
 
 	int vbus_enable;
 
-	u8 linkstat;
+	int mailboxstat;
+	enum omap_dwc3_vbus_id_status linkstat;
 };
 
 #define comparator_to_palmas(x) container_of((x), struct palmas_usb, comparator)
-- 
1.7.10.4

[PATCH v2 4/4] usb: musb: omap2430: replace *_* with *-* in property names

[Kishon Vijay Abraham I]

No functional change. Replace *_* with *-* in property names of otg to
follow the general convention.

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 Documentation/devicetree/bindings/usb/omap-usb.txt |   12 ++++++------
 drivers/usb/musb/omap2430.c                        |    6 +++---
 2 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/Documentation/devicetree/bindings/usb/omap-usb.txt b/Documentation/devicetree/bindings/usb/omap-usb.txt
index 1b9f55f..662f0f1 100644
--- a/Documentation/devicetree/bindings/usb/omap-usb.txt
+++ b/Documentation/devicetree/bindings/usb/omap-usb.txt
@@ -8,10 +8,10 @@ OMAP MUSB GLUE
    and disconnect.
  - multipoint : Should be "1" indicating the musb controller supports
    multipoint. This is a MUSB configuration-specific setting.
- - num_eps : Specifies the number of endpoints. This is also a
+ - num-eps : Specifies the number of endpoints. This is also a
    MUSB configuration-specific setting. Should be set to "16"
- - ram_bits : Specifies the ram address size. Should be set to "12"
- - interface_type : This is a board specific setting to describe the type of
+ - ram-bits : Specifies the ram address size. Should be set to "12"
+ - interface-type : This is a board specific setting to describe the type of
    interface between the controller and the phy. It should be "0" or "1"
    specifying ULPI and UTMI respectively.
  - mode : Should be "3" to represent OTG. "1" signifies HOST and "2"
@@ -29,14 +29,14 @@ usb_otg_hs: usb_otg_hs@4a0ab000 {
 	ti,hwmods = "usb_otg_hs";
 	ti,has-mailbox;
 	multipoint = <1>;
-	num_eps = <16>;
-	ram_bits = <12>;
+	num-eps = <16>;
+	ram-bits = <12>;
 	ctrl-module = <&omap_control_usb>;
 };
 
 Board specific device node entry
 &usb_otg_hs {
-	interface_type = <1>;
+	interface-type = <1>;
 	mode = <3>;
 	power = <50>;
 };
diff --git a/drivers/usb/musb/omap2430.c b/drivers/usb/musb/omap2430.c
index 1762354..dde2802 100644
--- a/drivers/usb/musb/omap2430.c
+++ b/drivers/usb/musb/omap2430.c
@@ -522,10 +522,10 @@ static int omap2430_probe(struct platform_device *pdev)
 		}
 
 		of_property_read_u32(np, "mode", (u32 *)&pdata->mode);
-		of_property_read_u32(np, "interface_type",
+		of_property_read_u32(np, "interface-type",
 						(u32 *)&data->interface_type);
-		of_property_read_u32(np, "num_eps", (u32 *)&config->num_eps);
-		of_property_read_u32(np, "ram_bits", (u32 *)&config->ram_bits);
+		of_property_read_u32(np, "num-eps", (u32 *)&config->num_eps);
+		of_property_read_u32(np, "ram-bits", (u32 *)&config->ram_bits);
 		of_property_read_u32(np, "power", (u32 *)&pdata->power);
 		config->multipoint = of_property_read_bool(np, "multipoint");
 		pdata->has_mailbox = of_property_read_bool(np,
-- 
1.7.10.4

Re: [PATCH v2 3/4] USB: Palmas OTG Transceiver Driver

[Felipe Balbi]

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

On Thu, Mar 07, 2013 at 06:51:45PM +0530, Kishon Vijay Abraham I wrote:
> From: Graeme Gregory <gg@slimlogic.co.uk>
> 
> This is the driver for the OTG transceiver built into the Palmas chip. It
> handles the various USB OTG events that can be generated by cable
> insertion/removal.
> 
> Signed-off-by: Graeme Gregory <gg@slimlogic.co.uk>
> Signed-off-by: Moiz Sonasath <m-sonasath@ti.com>
> Signed-off-by: Ruchika Kharwar <ruchika@ti.com>
> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> Signed-off-by: Sebastien Guiriec <s-guiriec@ti.com>
> ---
>  .../devicetree/bindings/usb/twlxxxx-usb.txt        |   15 +
>  drivers/usb/otg/Kconfig                            |    6 +
>  drivers/usb/otg/Makefile                           |    1 +
>  drivers/usb/otg/palmas-usb.c                       |  396 ++++++++++++++++++++

this has to go to drivers/usb/phy/ and should be named phy-palmas.c or
phy-twl$(whatever number palmas is).c :-)

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v2 4/4] usb: musb: omap2430: replace *_* with *-* in property names

[Felipe Balbi]

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

On Thu, Mar 07, 2013 at 06:51:46PM +0530, Kishon Vijay Abraham I wrote:
> No functional change. Replace *_* with *-* in property names of otg to
> follow the general convention.
> 
> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>

this has been pending for quite a while, since nobody complained, I'm
taking it for v3.10.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v2 3/4] USB: Palmas OTG Transceiver Driver

[kishon]

On Thursday 14 March 2013 07:26 PM, Felipe Balbi wrote:
> On Thu, Mar 07, 2013 at 06:51:45PM +0530, Kishon Vijay Abraham I wrote:
>> From: Graeme Gregory <gg@slimlogic.co.uk>
>>
>> This is the driver for the OTG transceiver built into the Palmas chip. It
>> handles the various USB OTG events that can be generated by cable
>> insertion/removal.
>>
>> Signed-off-by: Graeme Gregory <gg@slimlogic.co.uk>
>> Signed-off-by: Moiz Sonasath <m-sonasath@ti.com>
>> Signed-off-by: Ruchika Kharwar <ruchika@ti.com>
>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>> Signed-off-by: Sebastien Guiriec <s-guiriec@ti.com>
>> ---
>>   .../devicetree/bindings/usb/twlxxxx-usb.txt        |   15 +
>>   drivers/usb/otg/Kconfig                            |    6 +
>>   drivers/usb/otg/Makefile                           |    1 +
>>   drivers/usb/otg/palmas-usb.c                       |  396 ++++++++++++++++++++
>
> this has to go to drivers/usb/phy/ and should be named phy-palmas.c or
> phy-twl$(whatever number palmas is).c :-)

Ok. I'll fix this and send

Thanks
Kishon

[PATCH v3] USB: PHY: Palmas USB Transceiver Driver

[Kishon Vijay Abraham I]

From: Graeme Gregory <gg@slimlogic.co.uk>

This is the driver for the OTG transceiver built into the Palmas chip. It
handles the various USB OTG events that can be generated by cable
insertion/removal.

Signed-off-by: Graeme Gregory <gg@slimlogic.co.uk>
Signed-off-by: Moiz Sonasath <m-sonasath@ti.com>
Signed-off-by: Ruchika Kharwar <ruchika@ti.com>
Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
Signed-off-by: Sebastien Guiriec <s-guiriec@ti.com>
---
Changes from v2:
* Moved the driver to drivers/usb/phy/
* Added a bit more explanation in Kconfig

 .../devicetree/bindings/usb/twlxxxx-usb.txt        |   15 +
 drivers/usb/phy/Kconfig                            |    9 +
 drivers/usb/phy/Makefile                           |    1 +
 drivers/usb/phy/phy-palmas-usb.c                   |  396 ++++++++++++++++++++
 include/linux/mfd/palmas.h                         |    7 +-
 5 files changed, 427 insertions(+), 1 deletion(-)
 create mode 100644 drivers/usb/phy/phy-palmas-usb.c

diff --git a/Documentation/devicetree/bindings/usb/twlxxxx-usb.txt b/Documentation/devicetree/bindings/usb/twlxxxx-usb.txt
index 36b9aed..17a9194 100644
--- a/Documentation/devicetree/bindings/usb/twlxxxx-usb.txt
+++ b/Documentation/devicetree/bindings/usb/twlxxxx-usb.txt
@@ -38,3 +38,18 @@ twl4030-usb {
 	usb3v1-supply = <&vusb3v1>;
 	usb_mode = <1>;
 };
+
+PALMAS USB COMPARATOR
+Required Properties:
+ - compatible : Should be "ti,palmas-usb" or "ti,twl6035-usb"
+ - vbus-supply : phandle to the regulator device tree node.
+
+Optional Properties:
+ - ti,wakeup : To enable the wakeup comparator in probe
+ - ti,no_control_vbus: if the platform wishes its own vbus control
+
+palmas-usb {
+	compatible = "ti,twl6035-usb", "ti,palmas-usb";
+	vbus-supply = <&smps10_reg>;
+	ti,wakeup;
+};
diff --git a/drivers/usb/phy/Kconfig b/drivers/usb/phy/Kconfig
index 7e8fe0f..4107136 100644
--- a/drivers/usb/phy/Kconfig
+++ b/drivers/usb/phy/Kconfig
@@ -85,6 +85,15 @@ config OMAP_USB3
 	  This driver interacts with the "OMAP Control USB Driver" to power
 	  on/off the PHY.
 
+config PALMAS_USB
+	tristate "Palmas USB Transceiver Driver"
+	depends on MFD_PALMAS
+	help
+	  Enable this to support the Palmas USB transceiver driver. This
+	  palmas transceiver has the VBUS and ID GND and OTG SRP events
+	  capabilities. For all other transceiver functionality
+	  UTMI PHY is embedded in OMAP.
+
 config SAMSUNG_USBPHY
 	tristate "Samsung USB PHY Driver"
 	help
diff --git a/drivers/usb/phy/Makefile b/drivers/usb/phy/Makefile
index 33863c0..0e01790 100644
--- a/drivers/usb/phy/Makefile
+++ b/drivers/usb/phy/Makefile
@@ -17,6 +17,7 @@ obj-$(CONFIG_NOP_USB_XCEIV)		+= phy-nop.o
 obj-$(CONFIG_OMAP_CONTROL_USB)		+= phy-omap-control.o
 obj-$(CONFIG_OMAP_USB2)			+= phy-omap-usb2.o
 obj-$(CONFIG_OMAP_USB3)			+= phy-omap-usb3.o
+obj-$(CONFIG_PALMAS_USB)		+= phy-palmas-usb.o
 obj-$(CONFIG_SAMSUNG_USBPHY)		+= phy-samsung-usb.o
 obj-$(CONFIG_SAMSUNG_USB2PHY)		+= phy-samsung-usb2.o
 obj-$(CONFIG_SAMSUNG_USB3PHY)		+= phy-samsung-usb3.o
diff --git a/drivers/usb/phy/phy-palmas-usb.c b/drivers/usb/phy/phy-palmas-usb.c
new file mode 100644
index 0000000..8bdffe3
--- /dev/null
+++ b/drivers/usb/phy/phy-palmas-usb.c
@@ -0,0 +1,396 @@
+/*
+ * Palmas USB transceiver driver
+ *
+ * Copyright (C) 2013 Texas Instruments Incorporated - http://www.ti.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.
+ *
+ * Author: Graeme Gregory <gg@slimlogic.co.uk>
+ * Author: Kishon Vijay Abraham I <kishon@ti.com>
+ *
+ * Based on twl6030_usb.c
+ *
+ * Author: Hema HK <hemahk@ti.com>
+ *
+ * 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/init.h>
+#include <linux/interrupt.h>
+#include <linux/platform_device.h>
+#include <linux/io.h>
+#include <linux/usb/otg.h>
+#include <linux/usb/phy_companion.h>
+#include <linux/usb/omap_usb.h>
+#include <linux/usb/dwc3-omap.h>
+#include <linux/regulator/consumer.h>
+#include <linux/err.h>
+#include <linux/notifier.h>
+#include <linux/slab.h>
+#include <linux/delay.h>
+#include <linux/mfd/palmas.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
+
+static int palmas_usb_read(struct palmas *palmas, unsigned int reg,
+		unsigned int *dest)
+{
+	unsigned int addr;
+	int slave;
+
+	slave = PALMAS_BASE_TO_SLAVE(PALMAS_USB_OTG_BASE);
+	addr = PALMAS_BASE_TO_REG(PALMAS_USB_OTG_BASE, reg);
+
+	return regmap_read(palmas->regmap[slave], addr, dest);
+}
+
+static int palmas_usb_write(struct palmas *palmas, unsigned int reg,
+		unsigned int data)
+{
+	unsigned int addr;
+	int slave;
+
+	slave = PALMAS_BASE_TO_SLAVE(PALMAS_USB_OTG_BASE);
+	addr = PALMAS_BASE_TO_REG(PALMAS_USB_OTG_BASE, reg);
+
+	return regmap_write(palmas->regmap[slave], addr, data);
+}
+
+static void palmas_usb_wakeup(struct palmas *palmas, int enable)
+{
+	if (enable)
+		palmas_usb_write(palmas, PALMAS_USB_WAKEUP,
+				PALMAS_USB_WAKEUP_ID_WK_UP_COMP);
+	else
+		palmas_usb_write(palmas, PALMAS_USB_WAKEUP, 0);
+}
+
+static ssize_t palmas_usb_vbus_show(struct device *dev,
+			struct device_attribute *attr, char *buf)
+{
+	unsigned long		flags;
+	int			ret = -EINVAL;
+	struct palmas_usb	*palmas_usb = dev_get_drvdata(dev);
+
+	spin_lock_irqsave(&palmas_usb->lock, flags);
+
+	switch (palmas_usb->linkstat) {
+	case OMAP_DWC3_VBUS_VALID:
+	       ret = snprintf(buf, PAGE_SIZE, "vbus\n");
+	       break;
+	case OMAP_DWC3_ID_GROUND:
+	       ret = snprintf(buf, PAGE_SIZE, "id\n");
+	       break;
+	case OMAP_DWC3_ID_FLOAT:
+	case OMAP_DWC3_VBUS_OFF:
+	       ret = snprintf(buf, PAGE_SIZE, "none\n");
+	       break;
+	default:
+	       ret = snprintf(buf, PAGE_SIZE, "UNKNOWN\n");
+	}
+	spin_unlock_irqrestore(&palmas_usb->lock, flags);
+
+	return ret;
+}
+static DEVICE_ATTR(vbus, 0444, palmas_usb_vbus_show, NULL);
+
+static irqreturn_t palmas_vbus_wakeup_irq(int irq, void *_palmas_usb)
+{
+	struct palmas_usb *palmas_usb = _palmas_usb;
+	enum omap_dwc3_vbus_id_status status = OMAP_DWC3_UNKNOWN;
+	int slave;
+	unsigned int vbus_line_state, addr;
+
+	slave = PALMAS_BASE_TO_SLAVE(PALMAS_INTERRUPT_BASE);
+	addr = PALMAS_BASE_TO_REG(PALMAS_INTERRUPT_BASE,
+			PALMAS_INT3_LINE_STATE);
+
+	regmap_read(palmas_usb->palmas->regmap[slave], addr, &vbus_line_state);
+
+	if (vbus_line_state & PALMAS_INT3_LINE_STATE_VBUS) {
+		if (palmas_usb->linkstat != OMAP_DWC3_VBUS_VALID) {
+			if (!IS_ERR_OR_NULL(palmas_usb->vbus_reg))
+				regulator_enable(palmas_usb->vbus_reg);
+			status = OMAP_DWC3_VBUS_VALID;
+		} else {
+			dev_dbg(palmas_usb->dev,
+				"Spurious connect event detected\n");
+		}
+	} else if (!(vbus_line_state & PALMAS_INT3_LINE_STATE_VBUS)) {
+		if (palmas_usb->linkstat == OMAP_DWC3_VBUS_VALID) {
+			if (!IS_ERR_OR_NULL(palmas_usb->vbus_reg))
+				regulator_disable(palmas_usb->vbus_reg);
+			status = OMAP_DWC3_VBUS_OFF;
+		} else {
+			dev_dbg(palmas_usb->dev,
+				"Spurious disconnect event detected\n");
+		}
+	}
+
+	if (status != OMAP_DWC3_UNKNOWN) {
+		palmas_usb->linkstat = status;
+		palmas_usb->mailboxstat = dwc3_omap_mailbox(status);
+		return IRQ_HANDLED;
+	}
+
+	return IRQ_NONE;
+}
+
+static irqreturn_t palmas_id_wakeup_irq(int irq, void *_palmas_usb)
+{
+	enum omap_dwc3_vbus_id_status status = OMAP_DWC3_UNKNOWN;
+	unsigned int		set;
+	struct palmas_usb	*palmas_usb = _palmas_usb;
+
+	palmas_usb_read(palmas_usb->palmas, PALMAS_USB_ID_INT_LATCH_SET, &set);
+
+	if (set & PALMAS_USB_ID_INT_SRC_ID_GND) {
+		if (!IS_ERR_OR_NULL(palmas_usb->vbus_reg))
+			regulator_enable(palmas_usb->vbus_reg);
+		palmas_usb_write(palmas_usb->palmas,
+					PALMAS_USB_ID_INT_EN_HI_SET,
+					PALMAS_USB_ID_INT_EN_HI_SET_ID_FLOAT);
+		palmas_usb_write(palmas_usb->palmas,
+					PALMAS_USB_ID_INT_EN_HI_CLR,
+					PALMAS_USB_ID_INT_EN_HI_CLR_ID_GND);
+		status = OMAP_DWC3_ID_GROUND;
+	} else if (set & PALMAS_USB_ID_INT_SRC_ID_FLOAT) {
+		palmas_usb_write(palmas_usb->palmas,
+					PALMAS_USB_ID_INT_EN_HI_SET,
+					PALMAS_USB_ID_INT_EN_HI_SET_ID_GND);
+		palmas_usb_write(palmas_usb->palmas,
+					PALMAS_USB_ID_INT_EN_HI_CLR,
+					PALMAS_USB_ID_INT_EN_HI_CLR_ID_FLOAT);
+		if (!IS_ERR_OR_NULL(palmas_usb->vbus_reg))
+			regulator_disable(palmas_usb->vbus_reg);
+		status = OMAP_DWC3_ID_FLOAT;
+	}
+
+	if (status != OMAP_DWC3_UNKNOWN) {
+		palmas_usb->linkstat = status;
+		palmas_usb->mailboxstat = dwc3_omap_mailbox(status);
+		return IRQ_HANDLED;
+	}
+
+	return IRQ_NONE;
+}
+
+static int palmas_enable_irq(struct palmas_usb *palmas_usb)
+{
+	palmas_usb_write(palmas_usb->palmas, PALMAS_USB_VBUS_CTRL_SET,
+			PALMAS_USB_VBUS_CTRL_SET_VBUS_ACT_COMP);
+
+	palmas_usb_write(palmas_usb->palmas, PALMAS_USB_ID_CTRL_SET,
+			PALMAS_USB_ID_CTRL_SET_ID_ACT_COMP);
+
+	palmas_usb_write(palmas_usb->palmas, PALMAS_USB_ID_INT_EN_HI_SET,
+			PALMAS_USB_ID_INT_EN_HI_SET_ID_GND);
+
+	palmas_vbus_wakeup_irq(palmas_usb->irq4, palmas_usb);
+	palmas_id_wakeup_irq(palmas_usb->irq2, palmas_usb);
+
+	return palmas_usb->mailboxstat;
+}
+
+static void palmas_set_vbus_work(struct work_struct *data)
+{
+	struct palmas_usb *palmas_usb = container_of(data, struct palmas_usb,
+								set_vbus_work);
+
+	if (IS_ERR_OR_NULL(palmas_usb->vbus_reg)) {
+		dev_err(palmas_usb->dev, "invalid regulator\n");
+		return;
+	}
+
+	/*
+	 * Start driving VBUS. Set OPA_MODE bit in CHARGERUSB_CTRL1
+	 * register. This enables boost mode.
+	 */
+
+	if (palmas_usb->vbus_enable)
+		regulator_enable(palmas_usb->vbus_reg);
+	else
+		regulator_disable(palmas_usb->vbus_reg);
+}
+
+static int palmas_set_vbus(struct phy_companion *comparator, bool enabled)
+{
+	struct palmas_usb *palmas_usb = comparator_to_palmas(comparator);
+
+	palmas_usb->vbus_enable = enabled;
+	schedule_work(&palmas_usb->set_vbus_work);
+
+	return 0;
+}
+
+static int palmas_start_srp(struct phy_companion *comparator)
+{
+	struct palmas_usb *palmas_usb = comparator_to_palmas(comparator);
+
+	palmas_usb_write(palmas_usb->palmas, PALMAS_USB_VBUS_CTRL_SET,
+			PALMAS_USB_VBUS_CTRL_SET_VBUS_DISCHRG |
+			PALMAS_USB_VBUS_CTRL_SET_VBUS_IADP_SINK);
+	palmas_usb_write(palmas_usb->palmas, PALMAS_USB_VBUS_CTRL_SET,
+			PALMAS_USB_VBUS_CTRL_SET_VBUS_CHRG_VSYS |
+			PALMAS_USB_VBUS_CTRL_SET_VBUS_IADP_SINK);
+
+	mdelay(100);
+
+	palmas_usb_write(palmas_usb->palmas, PALMAS_USB_VBUS_CTRL_CLR,
+			PALMAS_USB_VBUS_CTRL_SET_VBUS_CHRG_VSYS |
+			PALMAS_USB_VBUS_CTRL_SET_VBUS_CHRG_VSYS);
+
+	return 0;
+}
+
+static void palmas_dt_to_pdata(struct device_node *node,
+		struct palmas_usb_platform_data *pdata)
+{
+	pdata->no_control_vbus = of_property_read_bool(node,
+					"ti,no_control_vbus");
+	pdata->wakeup = of_property_read_bool(node, "ti,wakeup");
+}
+
+static int palmas_usb_probe(struct platform_device *pdev)
+{
+	u32 ret;
+	struct palmas *palmas = dev_get_drvdata(pdev->dev.parent);
+	struct palmas_usb_platform_data	*pdata = pdev->dev.platform_data;
+	struct device_node *node = pdev->dev.of_node;
+	struct palmas_usb *palmas_usb;
+	int status;
+
+	if (node && !pdata) {
+		pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
+
+		if (!pdata)
+			return -ENOMEM;
+
+		palmas_dt_to_pdata(node, pdata);
+	}
+
+	if (!pdata)
+		return -EINVAL;
+
+	palmas_usb = devm_kzalloc(&pdev->dev, sizeof(*palmas_usb), GFP_KERNEL);
+	if (!palmas_usb)
+		return -ENOMEM;
+
+	palmas->usb		= palmas_usb;
+	palmas_usb->palmas	= palmas;
+
+	palmas_usb->dev		= &pdev->dev;
+
+	palmas_usb->irq1 = regmap_irq_get_virq(palmas->irq_data,
+						PALMAS_ID_OTG_IRQ);
+	palmas_usb->irq2 = regmap_irq_get_virq(palmas->irq_data,
+						PALMAS_ID_IRQ);
+	palmas_usb->irq3 = regmap_irq_get_virq(palmas->irq_data,
+						PALMAS_VBUS_OTG_IRQ);
+	palmas_usb->irq4 = regmap_irq_get_virq(palmas->irq_data,
+						PALMAS_VBUS_IRQ);
+
+	palmas_usb->comparator.set_vbus	= palmas_set_vbus;
+	palmas_usb->comparator.start_srp = palmas_start_srp;
+
+	ret = omap_usb2_set_comparator(&palmas_usb->comparator);
+	if (ret == -ENODEV)
+		return -EPROBE_DEFER;
+
+	palmas_usb_wakeup(palmas, pdata->wakeup);
+
+	/* init spinlock for workqueue */
+	spin_lock_init(&palmas_usb->lock);
+
+	if (!pdata->no_control_vbus) {
+		palmas_usb->vbus_reg = devm_regulator_get(&pdev->dev, "vbus");
+		if (IS_ERR(palmas_usb->vbus_reg)) {
+			dev_err(&pdev->dev, "vbus init failed\n");
+			return PTR_ERR(palmas_usb->vbus_reg);
+		}
+	}
+
+	platform_set_drvdata(pdev, palmas_usb);
+
+	if (device_create_file(&pdev->dev, &dev_attr_vbus))
+		dev_warn(&pdev->dev, "could not create sysfs file\n");
+
+	/* init spinlock for workqueue */
+	spin_lock_init(&palmas_usb->lock);
+
+	INIT_WORK(&palmas_usb->set_vbus_work, palmas_set_vbus_work);
+
+	status = devm_request_threaded_irq(palmas_usb->dev, palmas_usb->irq2,
+			NULL, palmas_id_wakeup_irq,
+			IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING,
+			"palmas_usb", palmas_usb);
+	if (status < 0) {
+		dev_err(&pdev->dev, "can't get IRQ %d, err %d\n",
+					palmas_usb->irq2, status);
+		goto fail_irq;
+	}
+
+	status = devm_request_threaded_irq(palmas_usb->dev, palmas_usb->irq4,
+			NULL, palmas_vbus_wakeup_irq,
+			IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING,
+			"palmas_usb", palmas_usb);
+	if (status < 0) {
+		dev_err(&pdev->dev, "can't get IRQ %d, err %d\n",
+					palmas_usb->irq4, status);
+		goto fail_irq;
+	}
+
+	status = palmas_enable_irq(palmas_usb);
+	if (status < 0) {
+		dev_dbg(&pdev->dev, "enable irq failed\n");
+		goto fail_irq;
+	}
+
+	return 0;
+
+fail_irq:
+	cancel_work_sync(&palmas_usb->set_vbus_work);
+	device_remove_file(palmas_usb->dev, &dev_attr_vbus);
+
+	return status;
+}
+
+static int palmas_usb_remove(struct platform_device *pdev)
+{
+	struct palmas_usb *palmas_usb = platform_get_drvdata(pdev);
+
+	device_remove_file(palmas_usb->dev, &dev_attr_vbus);
+	cancel_work_sync(&palmas_usb->set_vbus_work);
+
+	return 0;
+}
+
+static struct of_device_id of_palmas_match_tbl[] = {
+	{ .compatible = "ti,palmas-usb", },
+	{ .compatible = "ti,twl6035-usb", },
+	{ /* end */ }
+};
+
+static struct platform_driver palmas_usb_driver = {
+	.probe = palmas_usb_probe,
+	.remove = palmas_usb_remove,
+	.driver = {
+		.name = "palmas-usb",
+		.of_match_table = of_palmas_match_tbl,
+		.owner = THIS_MODULE,
+	},
+};
+
+module_platform_driver(palmas_usb_driver);
+
+MODULE_ALIAS("platform:palmas-usb");
+MODULE_AUTHOR("Graeme Gregory <gg@slimlogic.co.uk>");
+MODULE_DESCRIPTION("Palmas USB transceiver driver");
+MODULE_LICENSE("GPL");
+MODULE_DEVICE_TABLE(of, of_palmas_match_tbl);
diff --git a/include/linux/mfd/palmas.h b/include/linux/mfd/palmas.h
index 3bbda22..b93b8a3 100644
--- a/include/linux/mfd/palmas.h
+++ b/include/linux/mfd/palmas.h
@@ -19,6 +19,8 @@
 #include <linux/leds.h>
 #include <linux/regmap.h>
 #include <linux/regulator/driver.h>
+#include <linux/usb/phy_companion.h>
+#include <linux/usb/dwc3-omap.h>
 
 #define PALMAS_NUM_CLIENTS		3
 
@@ -342,6 +344,8 @@ struct palmas_usb {
 	struct palmas *palmas;
 	struct device *dev;
 
+	struct phy_companion comparator;
+
 	/* for vbus reporting with irqs disabled */
 	spinlock_t lock;
 
@@ -357,7 +361,8 @@ struct palmas_usb {
 
 	int vbus_enable;
 
-	u8 linkstat;
+	int mailboxstat;
+	enum omap_dwc3_vbus_id_status linkstat;
 };
 
 #define comparator_to_palmas(x) container_of((x), struct palmas_usb, comparator)
-- 
1.7.10.4

Re: [PATCH v3] USB: PHY: Palmas USB Transceiver Driver

[Laxman Dewangan]

On Monday 25 March 2013 03:02 PM, Kishon Vijay Abraham I wrote:
> From: Graeme Gregory <gg@slimlogic.co.uk>
>
> This is the driver for the OTG transceiver built into the Palmas chip. It
> handles the various USB OTG events that can be generated by cable
> insertion/removal.
>
> Signed-off-by: Graeme Gregory <gg@slimlogic.co.uk>
> Signed-off-by: Moiz Sonasath <m-sonasath@ti.com>
> Signed-off-by: Ruchika Kharwar <ruchika@ti.com>
> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> Signed-off-by: Sebastien Guiriec <s-guiriec@ti.com>
> ---

I think this driver is more over the cable connection like vbus 
detetcion or ID pin detection.
Then why not it is implemented based on extcon framework?

That way, generic usb driver (like tegra_usb driver) will get 
notification through extcon.

We need this cable detection through extcon on our tegra solution 
through the Palmas.


+#include <linux/of.h>
+#include <linux/of_platform.h>
+
+static int palmas_usb_read(struct palmas *palmas, unsigned int reg,
+               unsigned int *dest)
+{
+       unsigned int addr;
+       int slave;
+
+       slave = PALMAS_BASE_TO_SLAVE(PALMAS_USB_OTG_BASE);
+       addr = PALMAS_BASE_TO_REG(PALMAS_USB_OTG_BASE, reg);
+
+       return regmap_read(palmas->regmap[slave], addr, dest);


Please use the generic api for palmas_read()/palmas_write(0 as it will 
be ease on debugging on register access.
Direct regmap_read() does not help much on this.

> +}
> +
> +static int palmas_usb_write(struct palmas *palmas, unsigned int reg,
> +               unsigned int data)
> +{
> +       unsigned int addr;
> +       int slave;
> +
> +       slave = PALMAS_BASE_TO_SLAVE(PALMAS_USB_OTG_BASE);
> +       addr = PALMAS_BASE_TO_REG(PALMAS_USB_OTG_BASE, reg);
> +
> +       return regmap_write(palmas->regmap[slave], addr, data);

Same as above.



> +
> +       if (status != OMAP_DWC3_UNKNOWN) {
> +               palmas_usb->linkstat = status;
> +               palmas_usb->mailboxstat = dwc3_omap_mailbox(status);
Omap specific call, why? This is generic palma driver.


> +

> +       palmas_usb->dev         = &pdev->dev;
> +
> +       palmas_usb->irq1 = regmap_irq_get_virq(palmas->irq_data,
> +                                               PALMAS_ID_OTG_IRQ);
> +       palmas_usb->irq2 = regmap_irq_get_virq(palmas->irq_data,
> +                                               PALMAS_ID_IRQ);
> +       palmas_usb->irq3 = regmap_irq_get_virq(palmas->irq_data,
> +                                               PALMAS_VBUS_OTG_IRQ);
> +       palmas_usb->irq4 = regmap_irq_get_virq(palmas->irq_data,
> +                                               PALMAS_VBUS_IRQ);

Should be come from platform_get_irq() through platform driver.

Re: [PATCH v3] USB: PHY: Palmas USB Transceiver Driver

[Kishon Vijay Abraham I]

Hi,

On Monday 25 March 2013 03:16 PM, Laxman Dewangan wrote:
> On Monday 25 March 2013 03:02 PM, Kishon Vijay Abraham I wrote:
>> From: Graeme Gregory <gg@slimlogic.co.uk>
>>
>> This is the driver for the OTG transceiver built into the Palmas chip. It
>> handles the various USB OTG events that can be generated by cable
>> insertion/removal.
>>
>> Signed-off-by: Graeme Gregory <gg@slimlogic.co.uk>
>> Signed-off-by: Moiz Sonasath <m-sonasath@ti.com>
>> Signed-off-by: Ruchika Kharwar <ruchika@ti.com>
>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>> Signed-off-by: Sebastien Guiriec <s-guiriec@ti.com>
>> ---
>
> I think this driver is more over the cable connection like vbus
> detetcion or ID pin detection.
> Then why not it is implemented based on extcon framework?

extcon framework uses notification mechanism and Felipe dint like using 
notification here. right Felipe?
>
> That way, generic usb driver (like tegra_usb driver) will get
> notification through extcon.
>
> We need this cable detection through extcon on our tegra solution
> through the Palmas.
>
>
> +#include <linux/of.h>
> +#include <linux/of_platform.h>
> +
> +static int palmas_usb_read(struct palmas *palmas, unsigned int reg,
> +               unsigned int *dest)
> +{
> +       unsigned int addr;
> +       int slave;
> +
> +       slave = PALMAS_BASE_TO_SLAVE(PALMAS_USB_OTG_BASE);
> +       addr = PALMAS_BASE_TO_REG(PALMAS_USB_OTG_BASE, reg);
> +
> +       return regmap_read(palmas->regmap[slave], addr, dest);
>
>
> Please use the generic api for palmas_read()/palmas_write(0 as it will
> be ease on debugging on register access.
> Direct regmap_read() does not help much on this.

Graeme,
Any reason why you dint use palmas_read()/palmas_write here?
Btw palmas_read()/palmas_write() internally uses regmap APIs.
>
>> +}
>> +
>> +static int palmas_usb_write(struct palmas *palmas, unsigned int reg,
>> +               unsigned int data)
>> +{
>> +       unsigned int addr;
>> +       int slave;
>> +
>> +       slave = PALMAS_BASE_TO_SLAVE(PALMAS_USB_OTG_BASE);
>> +       addr = PALMAS_BASE_TO_REG(PALMAS_USB_OTG_BASE, reg);
>> +
>> +       return regmap_write(palmas->regmap[slave], addr, data);
>
> Same as above.
>
>
>
>> +
>> +       if (status != OMAP_DWC3_UNKNOWN) {
>> +               palmas_usb->linkstat = status;
>> +               palmas_usb->mailboxstat = dwc3_omap_mailbox(status);
> Omap specific call, why? This is generic palma driver.

hmm.. I think we should either fall-back to the notification mechanism 
or have the client drivers pass function pointer here. Felipe?
>
>
>> +
>
>> +       palmas_usb->dev         = &pdev->dev;
>> +
>> +       palmas_usb->irq1 = regmap_irq_get_virq(palmas->irq_data,
>> +                                               PALMAS_ID_OTG_IRQ);
>> +       palmas_usb->irq2 = regmap_irq_get_virq(palmas->irq_data,
>> +                                               PALMAS_ID_IRQ);
>> +       palmas_usb->irq3 = regmap_irq_get_virq(palmas->irq_data,
>> +                                               PALMAS_VBUS_OTG_IRQ);
>> +       palmas_usb->irq4 = regmap_irq_get_virq(palmas->irq_data,
>> +                                               PALMAS_VBUS_IRQ);
>
> Should be come from platform_get_irq() through platform driver.

No. It can be obtained from regmap too.

Thanks
Kishon

Re: [PATCH v3] USB: PHY: Palmas USB Transceiver Driver

[Graeme Gregory]

On 26/03/13 06:03, Kishon Vijay Abraham I wrote:
> Hi,
>
> On Monday 25 March 2013 03:16 PM, Laxman Dewangan wrote:
>> On Monday 25 March 2013 03:02 PM, Kishon Vijay Abraham I wrote:
>>> From: Graeme Gregory <gg@slimlogic.co.uk>
>>>
>>> This is the driver for the OTG transceiver built into the Palmas
>>> chip. It
>>> handles the various USB OTG events that can be generated by cable
>>> insertion/removal.
>>>
>>> Signed-off-by: Graeme Gregory <gg@slimlogic.co.uk>
>>> Signed-off-by: Moiz Sonasath <m-sonasath@ti.com>
>>> Signed-off-by: Ruchika Kharwar <ruchika@ti.com>
>>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>>> Signed-off-by: Sebastien Guiriec <s-guiriec@ti.com>
>>> ---
>>
>> I think this driver is more over the cable connection like vbus
>> detetcion or ID pin detection.
>> Then why not it is implemented based on extcon framework?
>
> extcon framework uses notification mechanism and Felipe dint like
> using notification here. right Felipe?
>>
>> That way, generic usb driver (like tegra_usb driver) will get
>> notification through extcon.
>>
>> We need this cable detection through extcon on our tegra solution
>> through the Palmas.
>>
>>
>> +#include <linux/of.h>
>> +#include <linux/of_platform.h>
>> +
>> +static int palmas_usb_read(struct palmas *palmas, unsigned int reg,
>> +               unsigned int *dest)
>> +{
>> +       unsigned int addr;
>> +       int slave;
>> +
>> +       slave = PALMAS_BASE_TO_SLAVE(PALMAS_USB_OTG_BASE);
>> +       addr = PALMAS_BASE_TO_REG(PALMAS_USB_OTG_BASE, reg);
>> +
>> +       return regmap_read(palmas->regmap[slave], addr, dest);
>>
>>
>> Please use the generic api for palmas_read()/palmas_write(0 as it will
>> be ease on debugging on register access.
>> Direct regmap_read() does not help much on this.
>
> Graeme,
> Any reason why you dint use palmas_read()/palmas_write here?
> Btw palmas_read()/palmas_write() internally uses regmap APIs.
Because I was not a fan of tightly coupling the child devices to the
parent MFD. palmas_read/write were added by Laxman.

>>
>>> +}
>>> +
>>> +static int palmas_usb_write(struct palmas *palmas, unsigned int reg,
>>> +               unsigned int data)
>>> +{
>>> +       unsigned int addr;
>>> +       int slave;
>>> +
>>> +       slave = PALMAS_BASE_TO_SLAVE(PALMAS_USB_OTG_BASE);
>>> +       addr = PALMAS_BASE_TO_REG(PALMAS_USB_OTG_BASE, reg);
>>> +
>>> +       return regmap_write(palmas->regmap[slave], addr, data);
>>
>> Same as above.
>>
>>
>>
>>> +
>>> +       if (status != OMAP_DWC3_UNKNOWN) {
>>> +               palmas_usb->linkstat = status;
>>> +               palmas_usb->mailboxstat = dwc3_omap_mailbox(status);
>> Omap specific call, why? This is generic palma driver.
>
> hmm.. I think we should either fall-back to the notification mechanism
> or have the client drivers pass function pointer here. Felipe?
>>
>>
>>> +
>>
>>> +       palmas_usb->dev         = &pdev->dev;
>>> +
>>> +       palmas_usb->irq1 = regmap_irq_get_virq(palmas->irq_data,
>>> +                                               PALMAS_ID_OTG_IRQ);
>>> +       palmas_usb->irq2 = regmap_irq_get_virq(palmas->irq_data,
>>> +                                               PALMAS_ID_IRQ);
>>> +       palmas_usb->irq3 = regmap_irq_get_virq(palmas->irq_data,
>>> +                                               PALMAS_VBUS_OTG_IRQ);
>>> +       palmas_usb->irq4 = regmap_irq_get_virq(palmas->irq_data,
>>> +                                               PALMAS_VBUS_IRQ);
>>
>> Should be come from platform_get_irq() through platform driver.
>
> No. It can be obtained from regmap too.
>
> Thanks
> Kishon

Re: [PATCH v3] USB: PHY: Palmas USB Transceiver Driver

[Laxman Dewangan]

On Tuesday 26 March 2013 02:31 PM, Graeme Gregory wrote:
> On 26/03/13 06:03, Kishon Vijay Abraham I wrote:
>>> +static int palmas_usb_read(struct palmas *palmas, unsigned int reg,
>>> +               unsigned int *dest)
>>> +{
>>> +       unsigned int addr;
>>> +       int slave;
>>> +
>>> +       slave = PALMAS_BASE_TO_SLAVE(PALMAS_USB_OTG_BASE);
>>> +       addr = PALMAS_BASE_TO_REG(PALMAS_USB_OTG_BASE, reg);
>>> +
>>> +       return regmap_read(palmas->regmap[slave], addr, dest);
>>>
>>>
>>> Please use the generic api for palmas_read()/palmas_write(0 as it will
>>> be ease on debugging on register access.
>>> Direct regmap_read() does not help much on this.
>> Graeme,
>> Any reason why you dint use palmas_read()/palmas_write here?
>> Btw palmas_read()/palmas_write() internally uses regmap APIs.
> Because I was not a fan of tightly coupling the child devices to the
> parent MFD. palmas_read/write were added by Laxman.


But still you are using the PALMAS macro here and indirectly it is tied 
up. It is not completely independent.
If need to be independent then regmap pointer with address need to be 
passed as independt header and no where on code whould refer the PALMA.
I think as per current code, it is not possible although it is your big 
plan what I understand from some time back in one of patch discussion.


>>>> +       palmas_usb->dev         = &pdev->dev;
>>>> +
>>>> +       palmas_usb->irq1 = regmap_irq_get_virq(palmas->irq_data,
>>>> +                                               PALMAS_ID_OTG_IRQ);
>>>> +       palmas_usb->irq2 = regmap_irq_get_virq(palmas->irq_data,
>>>> +                                               PALMAS_ID_IRQ);
>>>> +       palmas_usb->irq3 = regmap_irq_get_virq(palmas->irq_data,
>>>> +                                               PALMAS_VBUS_OTG_IRQ);
>>>> +       palmas_usb->irq4 = regmap_irq_get_virq(palmas->irq_data,
>>>> +                                               PALMAS_VBUS_IRQ);
>>> Should be come from platform_get_irq() through platform driver.
>> No. It can be obtained from regmap too.
Kishon,
I think it is very much possible. You can pass the interrupt throough 
IRQ_RESOURCE and populate it from DT. If you provide proper interrupt 
parent and irq number then irq framework take care of every thing.  
already tested this with RTC interrupt of plama and it worked very well.

Re: [PATCH v3] USB: PHY: Palmas USB Transceiver Driver

[Graeme Gregory]

On 26/03/13 09:12, Laxman Dewangan wrote:
> On Tuesday 26 March 2013 02:31 PM, Graeme Gregory wrote:
>> On 26/03/13 06:03, Kishon Vijay Abraham I wrote:
>>>> +static int palmas_usb_read(struct palmas *palmas, unsigned int reg,
>>>> +               unsigned int *dest)
>>>> +{
>>>> +       unsigned int addr;
>>>> +       int slave;
>>>> +
>>>> +       slave = PALMAS_BASE_TO_SLAVE(PALMAS_USB_OTG_BASE);
>>>> +       addr = PALMAS_BASE_TO_REG(PALMAS_USB_OTG_BASE, reg);
>>>> +
>>>> +       return regmap_read(palmas->regmap[slave], addr, dest);
>>>>
>>>>
>>>> Please use the generic api for palmas_read()/palmas_write(0 as it will
>>>> be ease on debugging on register access.
>>>> Direct regmap_read() does not help much on this.
>>> Graeme,
>>> Any reason why you dint use palmas_read()/palmas_write here?
>>> Btw palmas_read()/palmas_write() internally uses regmap APIs.
>> Because I was not a fan of tightly coupling the child devices to the
>> parent MFD. palmas_read/write were added by Laxman.
>
>
> But still you are using the PALMAS macro here and indirectly it is
> tied up. It is not completely independent.
> If need to be independent then regmap pointer with address need to be
> passed as independt header and no where on code whould refer the PALMA.
> I think as per current code, it is not possible although it is your
> big plan what I understand from some time back in one of patch
> discussion.
>
It is actually almost possible, but it is something I gave up looking
at. You can get the regmap of your parent i2c device without having
knowledge of the type of parent.

>
>>>>> +       palmas_usb->dev         = &pdev->dev;
>>>>> +
>>>>> +       palmas_usb->irq1 = regmap_irq_get_virq(palmas->irq_data,
>>>>> +                                               PALMAS_ID_OTG_IRQ);
>>>>> +       palmas_usb->irq2 = regmap_irq_get_virq(palmas->irq_data,
>>>>> +                                               PALMAS_ID_IRQ);
>>>>> +       palmas_usb->irq3 = regmap_irq_get_virq(palmas->irq_data,
>>>>> +                                               PALMAS_VBUS_OTG_IRQ);
>>>>> +       palmas_usb->irq4 = regmap_irq_get_virq(palmas->irq_data,
>>>>> +                                               PALMAS_VBUS_IRQ);
>>>> Should be come from platform_get_irq() through platform driver.
>>> No. It can be obtained from regmap too.
> Kishon,
> I think it is very much possible. You can pass the interrupt throough
> IRQ_RESOURCE and populate it from DT. If you provide proper interrupt
> parent and irq number then irq framework take care of every thing. 
> already tested this with RTC interrupt of plama and it worked very well.
>
If we are tightly coupling as above then using platform_irq is an extra
inefficiency. You both have to populate this then parse it afterwards.
Why not just use the regmap helper? Ill admit this code is like this as
there was a period where platform irqs in DT just was not working right!

We should really agree now if we are going for loose or tight coupling
now rather than keep switching?

Graeme

Re: [PATCH v3] USB: PHY: Palmas USB Transceiver Driver

[Laxman Dewangan]

On Tuesday 26 March 2013 02:57 PM, Graeme Gregory wrote:
> On 26/03/13 09:12, Laxman Dewangan wrote:
>> On Tuesday 26 March 2013 02:31 PM, Graeme Gregory wrote:
>>
>> But still you are using the PALMAS macro here and indirectly it is
>> tied up. It is not completely independent.
>> If need to be independent then regmap pointer with address need to be
>> passed as independt header and no where on code whould refer the PALMA.
>> I think as per current code, it is not possible although it is your
>> big plan what I understand from some time back in one of patch
>> discussion.
>>
> It is actually almost possible, but it is something I gave up looking
> at. You can get the regmap of your parent i2c device without having
> knowledge of the type of parent.

There is multiple regmap of parent and hence getting correct regmap is 
really issue.  May be RTC require regmap[0] and gpio require regmap[1].


>
>>>>>> +       palmas_usb->dev         = &pdev->dev;
>>>>>> +
>>>>>> +       palmas_usb->irq1 = regmap_irq_get_virq(palmas->irq_data,
>>>>>> +                                               PALMAS_ID_OTG_IRQ);
>>>>>> +       palmas_usb->irq2 = regmap_irq_get_virq(palmas->irq_data,
>>>>>> +                                               PALMAS_ID_IRQ);
>>>>>> +       palmas_usb->irq3 = regmap_irq_get_virq(palmas->irq_data,
>>>>>> +                                               PALMAS_VBUS_OTG_IRQ);
>>>>>> +       palmas_usb->irq4 = regmap_irq_get_virq(palmas->irq_data,
>>>>>> +                                               PALMAS_VBUS_IRQ);
>>>>> Should be come from platform_get_irq() through platform driver.
>>>> No. It can be obtained from regmap too.
>> Kishon,
>> I think it is very much possible. You can pass the interrupt throough
>> IRQ_RESOURCE and populate it from DT. If you provide proper interrupt
>> parent and irq number then irq framework take care of every thing.
>> already tested this with RTC interrupt of plama and it worked very well.
>>
> If we are tightly coupling as above then using platform_irq is an extra
> inefficiency. You both have to populate this then parse it afterwards.
> Why not just use the regmap helper? Ill admit this code is like this as
> there was a period where platform irqs in DT just was not working right!
>
> We should really agree now if we are going for loose or tight coupling
> now rather than keep switching?

Here we are hardcoding for PALMAS_ID_OTG_IRQ and so on. If we take data 
from platform then it need not and it will be completely independent of 
palma atleast on this front.
We need to populate just as:
palmas: palmas {
:::::::
     palams_usb_phy {
         compatile = ...
         interrupt-parent = <& palmas>;
         interrupt = < 10, 0,
                                 21, 0,
                             22, 0,
                             23, 0>;
}


and in code, we just need to do
irq1 = platform_get_irq(pdev, 0);
irq2 = platform_get_irq(pdev, 1);
etc..


So here, actually we do not need to use palmas one and it is completely 
independent.

Also the way you define the DT od palmas, the above one looks more 
appropriate.

Re: [PATCH v3] USB: PHY: Palmas USB Transceiver Driver

[Graeme Gregory]

On 26/03/13 09:34, Laxman Dewangan wrote:
> On Tuesday 26 March 2013 02:57 PM, Graeme Gregory wrote:
>> On 26/03/13 09:12, Laxman Dewangan wrote:
>>> On Tuesday 26 March 2013 02:31 PM, Graeme Gregory wrote:
>>>
>>> But still you are using the PALMAS macro here and indirectly it is
>>> tied up. It is not completely independent.
>>> If need to be independent then regmap pointer with address need to be
>>> passed as independt header and no where on code whould refer the PALMA.
>>> I think as per current code, it is not possible although it is your
>>> big plan what I understand from some time back in one of patch
>>> discussion.
>>>
>> It is actually almost possible, but it is something I gave up looking
>> at. You can get the regmap of your parent i2c device without having
>> knowledge of the type of parent.
>
> There is multiple regmap of parent and hence getting correct regmap is
> really issue.  May be RTC require regmap[0] and gpio require regmap[1].
>
If you notice each regmap is connected to the correct dummy. Its
possible to create the correct children per dummy. The twl6030 driver
does this. But this is pointless now as I never intend to work on it so
we shall go with the tightly coupled.

>
>>
>>>>>>> +       palmas_usb->dev         = &pdev->dev;
>>>>>>> +
>>>>>>> +       palmas_usb->irq1 = regmap_irq_get_virq(palmas->irq_data,
>>>>>>> +                                               PALMAS_ID_OTG_IRQ);
>>>>>>> +       palmas_usb->irq2 = regmap_irq_get_virq(palmas->irq_data,
>>>>>>> +                                               PALMAS_ID_IRQ);
>>>>>>> +       palmas_usb->irq3 = regmap_irq_get_virq(palmas->irq_data,
>>>>>>> +                                              
>>>>>>> PALMAS_VBUS_OTG_IRQ);
>>>>>>> +       palmas_usb->irq4 = regmap_irq_get_virq(palmas->irq_data,
>>>>>>> +                                               PALMAS_VBUS_IRQ);
>>>>>> Should be come from platform_get_irq() through platform driver.
>>>>> No. It can be obtained from regmap too.
>>> Kishon,
>>> I think it is very much possible. You can pass the interrupt throough
>>> IRQ_RESOURCE and populate it from DT. If you provide proper interrupt
>>> parent and irq number then irq framework take care of every thing.
>>> already tested this with RTC interrupt of plama and it worked very
>>> well.
>>>
>> If we are tightly coupling as above then using platform_irq is an extra
>> inefficiency. You both have to populate this then parse it afterwards.
>> Why not just use the regmap helper? Ill admit this code is like this as
>> there was a period where platform irqs in DT just was not working right!
>>
>> We should really agree now if we are going for loose or tight coupling
>> now rather than keep switching?
>
> Here we are hardcoding for PALMAS_ID_OTG_IRQ and so on. If we take
> data from platform then it need not and it will be completely
> independent of palma atleast on this front.
> We need to populate just as:
> palmas: palmas {
> :::::::
>     palams_usb_phy {
>         compatile = ...
>         interrupt-parent = <& palmas>;
>         interrupt = < 10, 0,
>                                 21, 0,
>                             22, 0,
>                             23, 0>;
> }
>
>
> and in code, we just need to do
> irq1 = platform_get_irq(pdev, 0);
> irq2 = platform_get_irq(pdev, 1);
> etc..
>
>
> So here, actually we do not need to use palmas one and it is
> completely independent.
>
> Also the way you define the DT od palmas, the above one looks more
> appropriate.
>
Ok that makes sense if you are actually planning to feed non palmas IRQs
to the usb via either palmas GPIO or even directly! I did not know there
was such a use case!

Graeme

Re: [PATCH v3] USB: PHY: Palmas USB Transceiver Driver

[Felipe Balbi]

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

Hi,

On Tue, Mar 26, 2013 at 11:33:44AM +0530, Kishon Vijay Abraham I wrote:
> >>+static int palmas_usb_write(struct palmas *palmas, unsigned int reg,
> >>+               unsigned int data)
> >>+{
> >>+       unsigned int addr;
> >>+       int slave;
> >>+
> >>+       slave = PALMAS_BASE_TO_SLAVE(PALMAS_USB_OTG_BASE);
> >>+       addr = PALMAS_BASE_TO_REG(PALMAS_USB_OTG_BASE, reg);
> >>+
> >>+       return regmap_write(palmas->regmap[slave], addr, data);
> >
> >Same as above.
> >
> >
> >
> >>+
> >>+       if (status != OMAP_DWC3_UNKNOWN) {
> >>+               palmas_usb->linkstat = status;
> >>+               palmas_usb->mailboxstat = dwc3_omap_mailbox(status);
> >Omap specific call, why? This is generic palma driver.
> 
> hmm.. I think we should either fall-back to the notification
> mechanism or have the client drivers pass function pointer here.
> Felipe?

hmmm, if palmas is being used outside of OMAP world, then I guess extcon
is the way to go... too bad

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v3] USB: PHY: Palmas USB Transceiver Driver

[Felipe Balbi]

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

Hi,

On Tue, Mar 26, 2013 at 09:01:42AM +0000, Graeme Gregory wrote:
> >>> From: Graeme Gregory <gg@slimlogic.co.uk>
> >>>
> >>> This is the driver for the OTG transceiver built into the Palmas
> >>> chip. It
> >>> handles the various USB OTG events that can be generated by cable
> >>> insertion/removal.
> >>>
> >>> Signed-off-by: Graeme Gregory <gg@slimlogic.co.uk>
> >>> Signed-off-by: Moiz Sonasath <m-sonasath@ti.com>
> >>> Signed-off-by: Ruchika Kharwar <ruchika@ti.com>
> >>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> >>> Signed-off-by: Sebastien Guiriec <s-guiriec@ti.com>
> >>> ---
> >>
> >> I think this driver is more over the cable connection like vbus
> >> detetcion or ID pin detection.
> >> Then why not it is implemented based on extcon framework?
> >
> > extcon framework uses notification mechanism and Felipe dint like
> > using notification here. right Felipe?
> >>
> >> That way, generic usb driver (like tegra_usb driver) will get
> >> notification through extcon.
> >>
> >> We need this cable detection through extcon on our tegra solution
> >> through the Palmas.
> >>
> >>
> >> +#include <linux/of.h>
> >> +#include <linux/of_platform.h>
> >> +
> >> +static int palmas_usb_read(struct palmas *palmas, unsigned int reg,
> >> +               unsigned int *dest)
> >> +{
> >> +       unsigned int addr;
> >> +       int slave;
> >> +
> >> +       slave = PALMAS_BASE_TO_SLAVE(PALMAS_USB_OTG_BASE);
> >> +       addr = PALMAS_BASE_TO_REG(PALMAS_USB_OTG_BASE, reg);
> >> +
> >> +       return regmap_read(palmas->regmap[slave], addr, dest);
> >>
> >>
> >> Please use the generic api for palmas_read()/palmas_write(0 as it will
> >> be ease on debugging on register access.
> >> Direct regmap_read() does not help much on this.
> >
> > Graeme,
> > Any reason why you dint use palmas_read()/palmas_write here?
> > Btw palmas_read()/palmas_write() internally uses regmap APIs.
> Because I was not a fan of tightly coupling the child devices to the
> parent MFD. palmas_read/write were added by Laxman.

I guess regmap would also help abstracting SPI versus I2C connection.
IMHO, palmas_read/write should be removed.

Laxman's complaint that it doesn't help with debugging is utter
nonsense.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v3] USB: PHY: Palmas USB Transceiver Driver

[Laxman Dewangan]

On Tuesday 26 March 2013 03:51 PM, Felipe Balbi wrote:
> * PGP Signed by an unknown key
>
> Hi,
>
> On Tue, Mar 26, 2013 at 09:01:42AM +0000, Graeme Gregory wrote:
>>>>> From: Graeme Gregory <gg@slimlogic.co.uk>
>>>>>
>>>>> This is the driver for the OTG transceiver built into the Palmas
>>>>> chip. It
>>>>> handles the various USB OTG events that can be generated by cable
>>>>> insertion/removal.
>>>>>
>>>>> Signed-off-by: Graeme Gregory <gg@slimlogic.co.uk>
>>>>> Signed-off-by: Moiz Sonasath <m-sonasath@ti.com>
>>>>> Signed-off-by: Ruchika Kharwar <ruchika@ti.com>
>>>>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>>>>> Signed-off-by: Sebastien Guiriec <s-guiriec@ti.com>
>>>>> ---
>>>> I think this driver is more over the cable connection like vbus
>>>> detetcion or ID pin detection.
>>>> Then why not it is implemented based on extcon framework?
>>> extcon framework uses notification mechanism and Felipe dint like
>>> using notification here. right Felipe?
>>>> That way, generic usb driver (like tegra_usb driver) will get
>>>> notification through extcon.
>>>>
>>>> We need this cable detection through extcon on our tegra solution
>>>> through the Palmas.
>>>>
>>>>
>>>> +#include <linux/of.h>
>>>> +#include <linux/of_platform.h>
>>>> +
>>>> +static int palmas_usb_read(struct palmas *palmas, unsigned int reg,
>>>> +               unsigned int *dest)
>>>> +{
>>>> +       unsigned int addr;
>>>> +       int slave;
>>>> +
>>>> +       slave = PALMAS_BASE_TO_SLAVE(PALMAS_USB_OTG_BASE);
>>>> +       addr = PALMAS_BASE_TO_REG(PALMAS_USB_OTG_BASE, reg);
>>>> +
>>>> +       return regmap_read(palmas->regmap[slave], addr, dest);
>>>>
>>>>
>>>> Please use the generic api for palmas_read()/palmas_write(0 as it will
>>>> be ease on debugging on register access.
>>>> Direct regmap_read() does not help much on this.
>>> Graeme,
>>> Any reason why you dint use palmas_read()/palmas_write here?
>>> Btw palmas_read()/palmas_write() internally uses regmap APIs.
>> Because I was not a fan of tightly coupling the child devices to the
>> parent MFD. palmas_read/write were added by Laxman.
> I guess regmap would also help abstracting SPI versus I2C connection.
> IMHO, palmas_read/write should be removed.
>
> Laxman's complaint that it doesn't help with debugging is utter
> nonsense.
palams read/write api uses the regmap only and hence not break anything 
on abstraction.
in place of doing the following three statement in whole word, it 
provides wrapper of palmas_read()
which actually does the same.

slave = PALMAS_BASE_TO_SLAVE(PALMAS_USB_OTG_BASE);
addr = PALMAS_BASE_TO_REG(PALMAS_USB_OTG_BASE, reg);

regmap_read(palmas->regmap[slave], addr, dest);

Above 3 lines in all the places for resgister access or make single call:
palmas_read(palmas, PALMAS_USB_OTG_BASE, PALMAS_USB_OTG_BASE, dest).

This function implement the above 3 lines.

Re: [PATCH v3] USB: PHY: Palmas USB Transceiver Driver

[Laxman Dewangan]

On Tuesday 26 March 2013 03:21 PM, Graeme Gregory wrote:
> On 26/03/13 09:34, Laxman Dewangan wrote:
>>>>
>>>> Kishon,
>>>> I think it is very much possible. You can pass the interrupt throough
>>>> IRQ_RESOURCE and populate it from DT. If you provide proper interrupt
>>>> parent and irq number then irq framework take care of every thing.
>>>> already tested this with RTC interrupt of plama and it worked very
>>>> well.
>>>>
>>> If we are tightly coupling as above then using platform_irq is an extra
>>> inefficiency. You both have to populate this then parse it afterwards.
>>> Why not just use the regmap helper? Ill admit this code is like this as
>>> there was a period where platform irqs in DT just was not working right!
>>>
>>> We should really agree now if we are going for loose or tight coupling
>>> now rather than keep switching?
>> Here we are hardcoding for PALMAS_ID_OTG_IRQ and so on. If we take
>> data from platform then it need not and it will be completely
>> independent of palma atleast on this front.
>> We need to populate just as:
>> palmas: palmas {
>> :::::::
>>      palams_usb_phy {
>>          compatile = ...
>>          interrupt-parent = <& palmas>;
>>          interrupt = < 10, 0,
>>                                  21, 0,
>>                              22, 0,
>>                              23, 0>;
>> }
>>
>>
>> and in code, we just need to do
>> irq1 = platform_get_irq(pdev, 0);
>> irq2 = platform_get_irq(pdev, 1);
>> etc..
>>
>>
>> So here, actually we do not need to use palmas one and it is
>> completely independent.
>>
>> Also the way you define the DT od palmas, the above one looks more
>> appropriate.
>>
> Ok that makes sense if you are actually planning to feed non palmas IRQs
> to the usb via either palmas GPIO or even directly! I did not know there
> was such a use case!
>
> Graeme
>

Hi Graeme,
There is multiple reqson for requesting this change:
- When we register the device through non-dt, the irq number come as 
IRQ_RESOURCE when we add mfd sub devices. We added the same irq number 
on mfd/palma.c
- So if that is true then irq should get from platform_irq_get() for 
having proper transfer of infomration.
- Same thing can be populated through dt. If any change then change will 
be on the driver which si registerung in place of on driver which is 
implementing.

Another important point is: we have tps80036 (called palams-charger in 
some of places) which support extended gpios and interrupts. The 
extended interrupt register is not properly offsetted and in current 
regmp-irq framework, it can ot be accomodate. For that the palma need to 
implement the local irq implementation.
In this case, really regmap will not help much as the registration will 
not be through regmap-irq as irq domain will be created locally.

Re: [PATCH v3] USB: PHY: Palmas USB Transceiver Driver

[Felipe Balbi]

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

Hi,

On Tue, Mar 26, 2013 at 03:58:41PM +0530, Laxman Dewangan wrote:
> On Tuesday 26 March 2013 03:51 PM, Felipe Balbi wrote:
> >* PGP Signed by an unknown key
> >
> >Hi,
> >
> >On Tue, Mar 26, 2013 at 09:01:42AM +0000, Graeme Gregory wrote:
> >>>>>From: Graeme Gregory <gg@slimlogic.co.uk>
> >>>>>
> >>>>>This is the driver for the OTG transceiver built into the Palmas
> >>>>>chip. It
> >>>>>handles the various USB OTG events that can be generated by cable
> >>>>>insertion/removal.
> >>>>>
> >>>>>Signed-off-by: Graeme Gregory <gg@slimlogic.co.uk>
> >>>>>Signed-off-by: Moiz Sonasath <m-sonasath@ti.com>
> >>>>>Signed-off-by: Ruchika Kharwar <ruchika@ti.com>
> >>>>>Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> >>>>>Signed-off-by: Sebastien Guiriec <s-guiriec@ti.com>
> >>>>>---
> >>>>I think this driver is more over the cable connection like vbus
> >>>>detetcion or ID pin detection.
> >>>>Then why not it is implemented based on extcon framework?
> >>>extcon framework uses notification mechanism and Felipe dint like
> >>>using notification here. right Felipe?
> >>>>That way, generic usb driver (like tegra_usb driver) will get
> >>>>notification through extcon.
> >>>>
> >>>>We need this cable detection through extcon on our tegra solution
> >>>>through the Palmas.
> >>>>
> >>>>
> >>>>+#include <linux/of.h>
> >>>>+#include <linux/of_platform.h>
> >>>>+
> >>>>+static int palmas_usb_read(struct palmas *palmas, unsigned int reg,
> >>>>+               unsigned int *dest)
> >>>>+{
> >>>>+       unsigned int addr;
> >>>>+       int slave;
> >>>>+
> >>>>+       slave = PALMAS_BASE_TO_SLAVE(PALMAS_USB_OTG_BASE);
> >>>>+       addr = PALMAS_BASE_TO_REG(PALMAS_USB_OTG_BASE, reg);
> >>>>+
> >>>>+       return regmap_read(palmas->regmap[slave], addr, dest);
> >>>>
> >>>>
> >>>>Please use the generic api for palmas_read()/palmas_write(0 as it will
> >>>>be ease on debugging on register access.
> >>>>Direct regmap_read() does not help much on this.
> >>>Graeme,
> >>>Any reason why you dint use palmas_read()/palmas_write here?
> >>>Btw palmas_read()/palmas_write() internally uses regmap APIs.
> >>Because I was not a fan of tightly coupling the child devices to the
> >>parent MFD. palmas_read/write were added by Laxman.
> >I guess regmap would also help abstracting SPI versus I2C connection.
> >IMHO, palmas_read/write should be removed.
> >
> >Laxman's complaint that it doesn't help with debugging is utter
> >nonsense.
> palams read/write api uses the regmap only and hence not break
> anything on abstraction.
> in place of doing the following three statement in whole word, it
> provides wrapper of palmas_read()
> which actually does the same.
> 
> slave = PALMAS_BASE_TO_SLAVE(PALMAS_USB_OTG_BASE);
> addr = PALMAS_BASE_TO_REG(PALMAS_USB_OTG_BASE, reg);
> 
> regmap_read(palmas->regmap[slave], addr, dest);
> 
> Above 3 lines in all the places for resgister access or make single call:
> palmas_read(palmas, PALMAS_USB_OTG_BASE, PALMAS_USB_OTG_BASE, dest).
> 
> This function implement the above 3 lines.

now you have explained what the problem is. Makes much more sense to use
palmas_read() indeed. Duplicating the same thing all over the place
isn't very nice.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v3] USB: PHY: Palmas USB Transceiver Driver

[Stephen Warren]

On 03/26/2013 04:28 AM, Laxman Dewangan wrote:
> On Tuesday 26 March 2013 03:51 PM, Felipe Balbi wrote:
>> On Tue, Mar 26, 2013 at 09:01:42AM +0000, Graeme Gregory wrote:
...
>>>>> +       return regmap_read(palmas->regmap[slave], addr, dest);
>>>>>
>>>>>
>>>>> Please use the generic api for palmas_read()/palmas_write(0 as it will
>>>>> be ease on debugging on register access.
>>>>> Direct regmap_read() does not help much on this.
>>>>
>>>> Any reason why you dint use palmas_read()/palmas_write here?
>>>> Btw palmas_read()/palmas_write() internally uses regmap APIs.
>>>
>>> Because I was not a fan of tightly coupling the child devices to the
>>> parent MFD. palmas_read/write were added by Laxman.
>>
>> I guess regmap would also help abstracting SPI versus I2C connection.
>> IMHO, palmas_read/write should be removed.
>>
>> Laxman's complaint that it doesn't help with debugging is utter
>> nonsense.
>
> palams read/write api uses the regmap only and hence not break anything
> on abstraction.
> in place of doing the following three statement in whole word, it
> provides wrapper of palmas_read()
> which actually does the same.
> 
> slave = PALMAS_BASE_TO_SLAVE(PALMAS_USB_OTG_BASE);
> addr = PALMAS_BASE_TO_REG(PALMAS_USB_OTG_BASE, reg);
> regmap_read(palmas->regmap[slave], addr, dest);

Why would you ever do that? Surely each module's probe should create or
obtain a regmap object somehow, and then all other code in that driver
should simply use regmap_read()/regmap_write() without having to perform
any kind of calculations at all.

If the MFD components truly are pluggable mix/match IP blocks, then all
the register offset #defines must all be relative to the base of the IP
block, so there would be no need for any calculations there.

The I2C address and base register address for the regmap object should
come from DT or platform data, and be used one time in probe() to create
the regmap object.

Then, there would be no need for palmas_read()/palmas_write().

> Above 3 lines in all the places for resgister access or make single call:
> palmas_read(palmas, PALMAS_USB_OTG_BASE, PALMAS_USB_OTG_BASE, dest).
> 
> This function implement the above 3 lines.

Re: [PATCH v3] USB: PHY: Palmas USB Transceiver Driver

[Stephen Warren]

On 03/26/2013 03:27 AM, Graeme Gregory wrote:
...
> If we are tightly coupling as above then using platform_irq is an extra
> inefficiency. You both have to populate this then parse it afterwards.
> Why not just use the regmap helper? Ill admit this code is like this as
> there was a period where platform irqs in DT just was not working right!
> 
> We should really agree now if we are going for loose or tight coupling
> now rather than keep switching?

Yes, this is something that I think needs to be fully resolved before
any more Palmas patches are discussed.

So you can have the MFD components fully coupled or completely
standalone. We should fully pick one or the other, not some mish-mash of
the two.

In practical terms, this means that:

a) Tightly coupled

The top-level MFD device knows exactly which child devices are present.
It has an internal table to defined the set of child devices, and
instantiate them. It provides them with IRQs, I2C addresses and register
base addresses (or regmaps), etc. etc., using purely Palmas-internal
APIs. If using device tree, the DT won't include any representation of
which child devices are present, nor their I2C addresses, nor their
register addresses, nor their IRQs, etc. That's all inside the driver.

b) Completely decoupled.

The top-level MFD device knows nothing about its children. It simply
provides a bus into which they can be instantiated, and a generic IRQ
controller into which they can hook.

Platform data or device tree is solely used to define which children
exist, which of the top-level MFD's I2C addresses is used for each
child, the base register address for each child device, the IRQs used by
each child device, etc.


Which of those two models are different people expecting?

(b) appears to be the most flexible, and since you have defined the DT
bindings to contain a separate node for each MFD child device, each with
its own compatible value, seems to be what you're aiming for. In this
scenario, there should be no private APIs between the top-level MFD
device and the children though; everything should be using standard bus
APIs.

Re: [PATCH v3] USB: PHY: Palmas USB Transceiver Driver

[Graeme Gregory]

On 26/03/13 16:22, Stephen Warren wrote:
> On 03/26/2013 03:27 AM, Graeme Gregory wrote:
> ...
>> If we are tightly coupling as above then using platform_irq is an extra
>> inefficiency. You both have to populate this then parse it afterwards.
>> Why not just use the regmap helper? Ill admit this code is like this as
>> there was a period where platform irqs in DT just was not working right!
>>
>> We should really agree now if we are going for loose or tight coupling
>> now rather than keep switching?
> Yes, this is something that I think needs to be fully resolved before
> any more Palmas patches are discussed.
>
> So you can have the MFD components fully coupled or completely
> standalone. We should fully pick one or the other, not some mish-mash of
> the two.
>
> In practical terms, this means that:
>
> a) Tightly coupled
>
> The top-level MFD device knows exactly which child devices are present.
> It has an internal table to defined the set of child devices, and
> instantiate them. It provides them with IRQs, I2C addresses and register
> base addresses (or regmaps), etc. etc., using purely Palmas-internal
> APIs. If using device tree, the DT won't include any representation of
> which child devices are present, nor their I2C addresses, nor their
> register addresses, nor their IRQs, etc. That's all inside the driver.
>
> b) Completely decoupled.
>
> The top-level MFD device knows nothing about its children. It simply
> provides a bus into which they can be instantiated, and a generic IRQ
> controller into which they can hook.
>
> Platform data or device tree is solely used to define which children
> exist, which of the top-level MFD's I2C addresses is used for each
> child, the base register address for each child device, the IRQs used by
> each child device, etc.
>
>
> Which of those two models are different people expecting?
>
> (b) appears to be the most flexible, and since you have defined the DT
> bindings to contain a separate node for each MFD child device, each with
> its own compatible value, seems to be what you're aiming for. In this
> scenario, there should be no private APIs between the top-level MFD
> device and the children though; everything should be using standard bus
> APIs.
I was aiming towards (b) which would allow drivers for IP blocks that I
know are shared between multiple devices such as RTC which is shared by
twl6030, twl6032, tps80032, tps65910, tps65911, tps65912, tps80035,
tps80036 and probably many more.

Finishing (b) implementation is currently beyond the time I have
available I think.

If we choose to ignore path (b) and ignore the code duplication of half
a dozen RTC drivers for the same IP than the path to (a) is much quicker
and easier. Can just rip out a lot of the DT stuff, use mfd_add_devices.
Makes the binding much simpler. Means we don't have to use platform
resources for IRQs. Makes palmas and palmas-charger just 2 black boxes
which is in line with other MFDs.

So I think I have come around to just do it the easy way and select (a)

I shall work on the main palmas series to implement (a).

This will obviously invalidate some comments on this patch and the main
series.

Graeme

Re: [PATCH v3] USB: PHY: Palmas USB Transceiver Driver

[Stephen Warren]

On 03/26/2013 10:57 AM, Graeme Gregory wrote:
> On 26/03/13 16:22, Stephen Warren wrote:
>> On 03/26/2013 03:27 AM, Graeme Gregory wrote:
>> ...
>>> If we are tightly coupling as above then using platform_irq is an extra
>>> inefficiency. You both have to populate this then parse it afterwards.
>>> Why not just use the regmap helper? Ill admit this code is like this as
>>> there was a period where platform irqs in DT just was not working right!
>>>
>>> We should really agree now if we are going for loose or tight coupling
>>> now rather than keep switching?
>> Yes, this is something that I think needs to be fully resolved before
>> any more Palmas patches are discussed.
>>
>> So you can have the MFD components fully coupled or completely
>> standalone. We should fully pick one or the other, not some mish-mash of
>> the two.
>>
>> In practical terms, this means that:
>>
>> a) Tightly coupled
>>
>> The top-level MFD device knows exactly which child devices are present.
>> It has an internal table to defined the set of child devices, and
>> instantiate them. It provides them with IRQs, I2C addresses and register
>> base addresses (or regmaps), etc. etc., using purely Palmas-internal
>> APIs. If using device tree, the DT won't include any representation of
>> which child devices are present, nor their I2C addresses, nor their
>> register addresses, nor their IRQs, etc. That's all inside the driver.
>>
>> b) Completely decoupled.
>>
>> The top-level MFD device knows nothing about its children. It simply
>> provides a bus into which they can be instantiated, and a generic IRQ
>> controller into which they can hook.
>>
>> Platform data or device tree is solely used to define which children
>> exist, which of the top-level MFD's I2C addresses is used for each
>> child, the base register address for each child device, the IRQs used by
>> each child device, etc.
>>
>>
>> Which of those two models are different people expecting?
>>
>> (b) appears to be the most flexible, and since you have defined the DT
>> bindings to contain a separate node for each MFD child device, each with
>> its own compatible value, seems to be what you're aiming for. In this
>> scenario, there should be no private APIs between the top-level MFD
>> device and the children though; everything should be using standard bus
>> APIs.
>
> I was aiming towards (b) which would allow drivers for IP blocks that I
> know are shared between multiple devices such as RTC which is shared by
> twl6030, twl6032, tps80032, tps65910, tps65911, tps65912, tps80035,
> tps80036 and probably many more.
> 
> Finishing (b) implementation is currently beyond the time I have
> available I think.
> 
> If we choose to ignore path (b) and ignore the code duplication of half
> a dozen RTC drivers for the same IP than the path to (a) is much quicker
> and easier. Can just rip out a lot of the DT stuff, use mfd_add_devices.
> Makes the binding much simpler. Means we don't have to use platform
> resources for IRQs. Makes palmas and palmas-charger just 2 black boxes
> which is in line with other MFDs.
> 
> So I think I have come around to just do it the easy way and select (a)
> 
> I shall work on the main palmas series to implement (a).
> 
> This will obviously invalidate some comments on this patch and the main
> series.

Well, my question was more directed at which way we want to model the HW
in the device tree, rather than how we want to implement the driver. The
driver implementation style might end up being derived from the DT
structure, but it shouldn't be the other way around.

I think if people think (b) is the right way to go, we should just do
it, and ignore any time issues; if it takes a while to get it right
upstream, what is the issue with that?

Re: [PATCH v3] USB: PHY: Palmas USB Transceiver Driver

[Graeme Gregory]

On 26/03/13 20:23, Stephen Warren wrote:
> On 03/26/2013 10:57 AM, Graeme Gregory wrote:
>> On 26/03/13 16:22, Stephen Warren wrote:
>>> On 03/26/2013 03:27 AM, Graeme Gregory wrote:
>>> ...
>>>> If we are tightly coupling as above then using platform_irq is an extra
>>>> inefficiency. You both have to populate this then parse it afterwards.
>>>> Why not just use the regmap helper? Ill admit this code is like this as
>>>> there was a period where platform irqs in DT just was not working right!
>>>>
>>>> We should really agree now if we are going for loose or tight coupling
>>>> now rather than keep switching?
>>> Yes, this is something that I think needs to be fully resolved before
>>> any more Palmas patches are discussed.
>>>
>>> So you can have the MFD components fully coupled or completely
>>> standalone. We should fully pick one or the other, not some mish-mash of
>>> the two.
>>>
>>> In practical terms, this means that:
>>>
>>> a) Tightly coupled
>>>
>>> The top-level MFD device knows exactly which child devices are present.
>>> It has an internal table to defined the set of child devices, and
>>> instantiate them. It provides them with IRQs, I2C addresses and register
>>> base addresses (or regmaps), etc. etc., using purely Palmas-internal
>>> APIs. If using device tree, the DT won't include any representation of
>>> which child devices are present, nor their I2C addresses, nor their
>>> register addresses, nor their IRQs, etc. That's all inside the driver.
>>>
>>> b) Completely decoupled.
>>>
>>> The top-level MFD device knows nothing about its children. It simply
>>> provides a bus into which they can be instantiated, and a generic IRQ
>>> controller into which they can hook.
>>>
>>> Platform data or device tree is solely used to define which children
>>> exist, which of the top-level MFD's I2C addresses is used for each
>>> child, the base register address for each child device, the IRQs used by
>>> each child device, etc.
>>>
>>>
>>> Which of those two models are different people expecting?
>>>
>>> (b) appears to be the most flexible, and since you have defined the DT
>>> bindings to contain a separate node for each MFD child device, each with
>>> its own compatible value, seems to be what you're aiming for. In this
>>> scenario, there should be no private APIs between the top-level MFD
>>> device and the children though; everything should be using standard bus
>>> APIs.
>> I was aiming towards (b) which would allow drivers for IP blocks that I
>> know are shared between multiple devices such as RTC which is shared by
>> twl6030, twl6032, tps80032, tps65910, tps65911, tps65912, tps80035,
>> tps80036 and probably many more.
>>
>> Finishing (b) implementation is currently beyond the time I have
>> available I think.
>>
>> If we choose to ignore path (b) and ignore the code duplication of half
>> a dozen RTC drivers for the same IP than the path to (a) is much quicker
>> and easier. Can just rip out a lot of the DT stuff, use mfd_add_devices.
>> Makes the binding much simpler. Means we don't have to use platform
>> resources for IRQs. Makes palmas and palmas-charger just 2 black boxes
>> which is in line with other MFDs.
>>
>> So I think I have come around to just do it the easy way and select (a)
>>
>> I shall work on the main palmas series to implement (a).
>>
>> This will obviously invalidate some comments on this patch and the main
>> series.
> Well, my question was more directed at which way we want to model the HW
> in the device tree, rather than how we want to implement the driver. The
> driver implementation style might end up being derived from the DT
> structure, but it shouldn't be the other way around.
>
> I think if people think (b) is the right way to go, we should just do
> it, and ignore any time issues; if it takes a while to get it right
> upstream, what is the issue with that?
I'm going to take a timeout now, I have to travel on an emergency
tonight and not sure when I will be back.

Graeme

[PATCH v4] extcon: Palmas Extcon Driver

[Kishon Vijay Abraham I]

From: Graeme Gregory <gg@slimlogic.co.uk>

This is the driver for the USB comparator built into the palmas chip. It
handles the various USB OTG events that can be generated by cable
insertion/removal.

Signed-off-by: Graeme Gregory <gg@slimlogic.co.uk>
Signed-off-by: Moiz Sonasath <m-sonasath@ti.com>
Signed-off-by: Ruchika Kharwar <ruchika@ti.com>
Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
[kishon@ti.com: adapted palmas usb driver to use the extcon framework]
Signed-off-by: Sebastien Guiriec <s-guiriec@ti.com>
---
Changes from v3:
* adapted the driver to extcon framework (so moved to drivers/extcon)
* removed palmas_usb_(write/read) and replaced all calls with
  palmas_(read/write).
* ignored a checkpatch warning in the line 
	static const char *palmas_extcon_cable[] = {
  as it seemed to be incorrect?
* removed all references to OMAP in this driver.
* couldn't test this driver with mainline as omap5 panda is not booting
  with mainline.
* A comment to change to platform_get_irq from regmap is not done as I felt
  the data should come from regmap in this case. Graeme?
Changes from v2:
* Moved the driver to drivers/usb/phy/
* Added a bit more explanation in Kconfig
 .../devicetree/bindings/extcon/extcon-twl.txt      |   17 +
 drivers/extcon/Kconfig                             |    7 +
 drivers/extcon/Makefile                            |    1 +
 drivers/extcon/extcon-palmas.c                     |  389 ++++++++++++++++++++
 include/linux/extcon/extcon_palmas.h               |   26 ++
 include/linux/mfd/palmas.h                         |    8 +-
 6 files changed, 447 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/extcon/extcon-twl.txt
 create mode 100644 drivers/extcon/extcon-palmas.c
 create mode 100644 include/linux/extcon/extcon_palmas.h

diff --git a/Documentation/devicetree/bindings/extcon/extcon-twl.txt b/Documentation/devicetree/bindings/extcon/extcon-twl.txt
new file mode 100644
index 0000000..a7f6527
--- /dev/null
+++ b/Documentation/devicetree/bindings/extcon/extcon-twl.txt
@@ -0,0 +1,17 @@
+EXTCON FOR TWL CHIPS
+
+PALMAS USB COMPARATOR
+Required Properties:
+ - compatible : Should be "ti,palmas-usb" or "ti,twl6035-usb"
+ - vbus-supply : phandle to the regulator device tree node.
+
+Optional Properties:
+ - ti,wakeup : To enable the wakeup comparator in probe
+ - ti,no_control_vbus: if the platform wishes its own vbus control
+
+palmas-usb {
+       compatible = "ti,twl6035-usb", "ti,palmas-usb";
+       vbus-supply = <&smps10_reg>;
+       ti,wakeup;
+};
+
diff --git a/drivers/extcon/Kconfig b/drivers/extcon/Kconfig
index 5168a13..c881899 100644
--- a/drivers/extcon/Kconfig
+++ b/drivers/extcon/Kconfig
@@ -53,4 +53,11 @@ config EXTCON_ARIZONA
 	  with Wolfson Arizona devices. These are audio CODECs with
 	  advanced audio accessory detection support.
 
+config EXTCON_PALMAS
+	tristate "Palmas USB EXTCON support"
+	depends on MFD_PALMAS
+	help
+	  Say Y here to enable support for USB peripheral and USB host
+	  detection by palmas usb.
+
 endif # MULTISTATE_SWITCH
diff --git a/drivers/extcon/Makefile b/drivers/extcon/Makefile
index f98a3c4..540e2c3 100644
--- a/drivers/extcon/Makefile
+++ b/drivers/extcon/Makefile
@@ -8,3 +8,4 @@ obj-$(CONFIG_EXTCON_ADC_JACK)	+= extcon-adc-jack.o
 obj-$(CONFIG_EXTCON_MAX77693)	+= extcon-max77693.o
 obj-$(CONFIG_EXTCON_MAX8997)	+= extcon-max8997.o
 obj-$(CONFIG_EXTCON_ARIZONA)	+= extcon-arizona.o
+obj-$(CONFIG_EXTCON_PALMAS)	+= extcon-palmas.o
diff --git a/drivers/extcon/extcon-palmas.c b/drivers/extcon/extcon-palmas.c
new file mode 100644
index 0000000..3ef042f
--- /dev/null
+++ b/drivers/extcon/extcon-palmas.c
@@ -0,0 +1,389 @@
+/*
+ * Palmas USB transceiver driver
+ *
+ * Copyright (C) 2013 Texas Instruments Incorporated - http://www.ti.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.
+ *
+ * Author: Graeme Gregory <gg@...mlogic.co.uk>
+ * Author: Kishon Vijay Abraham I <kishon@...com>
+ *
+ * Based on twl6030_usb.c
+ *
+ * Author: Hema HK <hemahk@...com>
+ *
+ * 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/init.h>
+#include <linux/interrupt.h>
+#include <linux/platform_device.h>
+#include <linux/io.h>
+#include <linux/usb/phy_companion.h>
+#include <linux/regulator/consumer.h>
+#include <linux/err.h>
+#include <linux/notifier.h>
+#include <linux/slab.h>
+#include <linux/delay.h>
+#include <linux/mfd/palmas.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
+#include <linux/extcon/extcon_palmas.h>
+
+static const char *palmas_extcon_cable[] = {
+	[0] = "USB",
+	[1] = "USB-HOST",
+	NULL,
+};
+
+static const int mutually_exclusive[] = {0x3, 0x0};
+
+static void palmas_usb_wakeup(struct palmas *palmas, int enable)
+{
+	if (enable)
+		palmas_write(palmas, PALMAS_USB_OTG_BASE, PALMAS_USB_WAKEUP,
+			PALMAS_USB_WAKEUP_ID_WK_UP_COMP);
+	else
+		palmas_write(palmas, PALMAS_USB_OTG_BASE, PALMAS_USB_WAKEUP, 0);
+}
+
+static ssize_t palmas_usb_vbus_show(struct device *dev,
+			struct device_attribute *attr, char *buf)
+{
+	unsigned long		flags;
+	int			ret = -EINVAL;
+	struct palmas_usb	*palmas_usb = dev_get_drvdata(dev);
+
+	spin_lock_irqsave(&palmas_usb->lock, flags);
+
+	switch (palmas_usb->linkstat) {
+	case PALMAS_USB_STATE_VBUS:
+	       ret = snprintf(buf, PAGE_SIZE, "vbus\n");
+	       break;
+	case PALMAS_USB_STATE_ID:
+	       ret = snprintf(buf, PAGE_SIZE, "id\n");
+	       break;
+	case PALMAS_USB_STATE_DISCONNECT:
+	       ret = snprintf(buf, PAGE_SIZE, "none\n");
+	       break;
+	default:
+	       ret = snprintf(buf, PAGE_SIZE, "UNKNOWN\n");
+	}
+	spin_unlock_irqrestore(&palmas_usb->lock, flags);
+
+	return ret;
+}
+static DEVICE_ATTR(vbus, 0444, palmas_usb_vbus_show, NULL);
+
+static irqreturn_t palmas_vbus_wakeup_irq(int irq, void *_palmas_usb)
+{
+	int ret;
+	struct palmas_usb *palmas_usb = _palmas_usb;
+	unsigned int vbus_line_state;
+
+	palmas_read(palmas_usb->palmas, PALMAS_INTERRUPT_BASE,
+		PALMAS_INT3_LINE_STATE, &vbus_line_state);
+
+	if (vbus_line_state & PALMAS_INT3_LINE_STATE_VBUS) {
+		if (palmas_usb->linkstat != PALMAS_USB_STATE_VBUS) {
+			if (palmas_usb->vbus_reg) {
+				ret = regulator_enable(palmas_usb->vbus_reg);
+				if (ret) {
+					dev_dbg(palmas_usb->dev,
+						"regulator enable failed\n");
+					goto ret0;
+				}
+			}
+			palmas_usb->linkstat = PALMAS_USB_STATE_VBUS;
+			extcon_set_cable_state(&palmas_usb->edev, "USB", true);
+		} else {
+			dev_dbg(palmas_usb->dev,
+				"Spurious connect event detected\n");
+		}
+	} else if (!(vbus_line_state & PALMAS_INT3_LINE_STATE_VBUS)) {
+		if (palmas_usb->linkstat == PALMAS_USB_STATE_VBUS) {
+			if (palmas_usb->vbus_reg)
+				regulator_disable(palmas_usb->vbus_reg);
+			palmas_usb->linkstat = PALMAS_USB_STATE_DISCONNECT;
+			extcon_set_cable_state(&palmas_usb->edev, "USB", false);
+		} else {
+			dev_dbg(palmas_usb->dev,
+				"Spurious disconnect event detected\n");
+		}
+	}
+
+ret0:
+	return IRQ_HANDLED;
+}
+
+static irqreturn_t palmas_id_wakeup_irq(int irq, void *_palmas_usb)
+{
+	int			ret;
+	unsigned int		set;
+	struct palmas_usb	*palmas_usb = _palmas_usb;
+
+	palmas_read(palmas_usb->palmas, PALMAS_USB_OTG_BASE,
+		PALMAS_USB_ID_INT_LATCH_SET, &set);
+
+	if (set & PALMAS_USB_ID_INT_SRC_ID_GND) {
+		if (palmas_usb->vbus_reg) {
+			ret = regulator_enable(palmas_usb->vbus_reg);
+			if (ret) {
+				dev_dbg(palmas_usb->dev,
+					"regulator enable failed\n");
+				goto ret0;
+			}
+		}
+		palmas_write(palmas_usb->palmas, PALMAS_USB_OTG_BASE,
+			PALMAS_USB_ID_INT_EN_HI_SET,
+			PALMAS_USB_ID_INT_EN_HI_SET_ID_FLOAT);
+		palmas_write(palmas_usb->palmas, PALMAS_USB_OTG_BASE,
+			PALMAS_USB_ID_INT_EN_HI_CLR,
+			PALMAS_USB_ID_INT_EN_HI_CLR_ID_GND);
+		palmas_usb->linkstat = PALMAS_USB_STATE_ID;
+		extcon_set_cable_state(&palmas_usb->edev, "USB-HOST", true);
+	} else if (set & PALMAS_USB_ID_INT_SRC_ID_FLOAT) {
+		palmas_write(palmas_usb->palmas, PALMAS_USB_OTG_BASE,
+			PALMAS_USB_ID_INT_EN_HI_SET,
+			PALMAS_USB_ID_INT_EN_HI_SET_ID_GND);
+		palmas_write(palmas_usb->palmas, PALMAS_USB_OTG_BASE,
+			PALMAS_USB_ID_INT_EN_HI_CLR,
+			PALMAS_USB_ID_INT_EN_HI_CLR_ID_FLOAT);
+		if (palmas_usb->vbus_reg)
+			regulator_disable(palmas_usb->vbus_reg);
+		palmas_usb->linkstat = PALMAS_USB_STATE_DISCONNECT;
+		extcon_set_cable_state(&palmas_usb->edev, "USB-HOST", false);
+	}
+
+ret0:
+	return IRQ_HANDLED;
+}
+
+static void palmas_enable_irq(struct palmas_usb *palmas_usb)
+{
+	palmas_write(palmas_usb->palmas, PALMAS_USB_OTG_BASE,
+		PALMAS_USB_VBUS_CTRL_SET,
+		PALMAS_USB_VBUS_CTRL_SET_VBUS_ACT_COMP);
+
+	palmas_write(palmas_usb->palmas, PALMAS_USB_OTG_BASE,
+		PALMAS_USB_ID_CTRL_SET, PALMAS_USB_ID_CTRL_SET_ID_ACT_COMP);
+
+	palmas_write(palmas_usb->palmas, PALMAS_USB_OTG_BASE,
+		PALMAS_USB_ID_INT_EN_HI_SET,
+		PALMAS_USB_ID_INT_EN_HI_SET_ID_GND);
+
+	palmas_vbus_wakeup_irq(palmas_usb->irq4, palmas_usb);
+	palmas_id_wakeup_irq(palmas_usb->irq2, palmas_usb);
+}
+
+static void palmas_set_vbus_work(struct work_struct *data)
+{
+	int ret;
+	struct palmas_usb *palmas_usb = container_of(data, struct palmas_usb,
+								set_vbus_work);
+
+	if (IS_ERR_OR_NULL(palmas_usb->vbus_reg)) {
+		dev_err(palmas_usb->dev, "invalid regulator\n");
+		return;
+	}
+
+	/*
+	 * Start driving VBUS. Set OPA_MODE bit in CHARGERUSB_CTRL1
+	 * register. This enables boost mode.
+	 */
+
+	if (palmas_usb->vbus_enable) {
+		ret = regulator_enable(palmas_usb->vbus_reg);
+		if (ret)
+			dev_dbg(palmas_usb->dev, "regulator enable failed\n");
+	} else {
+		regulator_disable(palmas_usb->vbus_reg);
+	}
+}
+
+static int palmas_set_vbus(struct phy_companion *comparator, bool enabled)
+{
+	struct palmas_usb *palmas_usb = comparator_to_palmas(comparator);
+
+	palmas_usb->vbus_enable = enabled;
+	schedule_work(&palmas_usb->set_vbus_work);
+
+	return 0;
+}
+
+static int palmas_start_srp(struct phy_companion *comparator)
+{
+	struct palmas_usb *palmas_usb = comparator_to_palmas(comparator);
+
+	palmas_write(palmas_usb->palmas, PALMAS_USB_OTG_BASE,
+		PALMAS_USB_VBUS_CTRL_SET, PALMAS_USB_VBUS_CTRL_SET_VBUS_DISCHRG
+		| PALMAS_USB_VBUS_CTRL_SET_VBUS_IADP_SINK);
+	palmas_write(palmas_usb->palmas, PALMAS_USB_OTG_BASE,
+		PALMAS_USB_VBUS_CTRL_SET,
+		PALMAS_USB_VBUS_CTRL_SET_VBUS_CHRG_VSYS |
+		PALMAS_USB_VBUS_CTRL_SET_VBUS_IADP_SINK);
+
+	mdelay(100);
+
+	palmas_write(palmas_usb->palmas, PALMAS_USB_OTG_BASE,
+		PALMAS_USB_VBUS_CTRL_CLR,
+		PALMAS_USB_VBUS_CTRL_SET_VBUS_CHRG_VSYS |
+		PALMAS_USB_VBUS_CTRL_SET_VBUS_CHRG_VSYS);
+
+	return 0;
+}
+
+static void palmas_dt_to_pdata(struct device_node *node,
+		struct palmas_usb_platform_data *pdata)
+{
+	pdata->no_control_vbus = of_property_read_bool(node,
+					"ti,no_control_vbus");
+	pdata->wakeup = of_property_read_bool(node, "ti,wakeup");
+}
+
+static int palmas_usb_probe(struct platform_device *pdev)
+{
+	u32 ret;
+	struct palmas *palmas = dev_get_drvdata(pdev->dev.parent);
+	struct palmas_usb_platform_data	*pdata = pdev->dev.platform_data;
+	struct device_node *node = pdev->dev.of_node;
+	struct palmas_usb *palmas_usb;
+	int status;
+
+	if (node && !pdata) {
+		pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
+
+		if (!pdata)
+			return -ENOMEM;
+
+		palmas_dt_to_pdata(node, pdata);
+	}
+
+	if (!pdata)
+		return -EINVAL;
+
+	palmas_usb = devm_kzalloc(&pdev->dev, sizeof(*palmas_usb), GFP_KERNEL);
+	if (!palmas_usb)
+		return -ENOMEM;
+
+	palmas->usb		= palmas_usb;
+	palmas_usb->palmas	= palmas;
+
+	palmas_usb->dev		= &pdev->dev;
+
+	palmas_usb->irq1 = regmap_irq_get_virq(palmas->irq_data,
+						PALMAS_ID_OTG_IRQ);
+	palmas_usb->irq2 = regmap_irq_get_virq(palmas->irq_data,
+						PALMAS_ID_IRQ);
+	palmas_usb->irq3 = regmap_irq_get_virq(palmas->irq_data,
+						PALMAS_VBUS_OTG_IRQ);
+	palmas_usb->irq4 = regmap_irq_get_virq(palmas->irq_data,
+						PALMAS_VBUS_IRQ);
+
+	palmas_usb->comparator.set_vbus	= palmas_set_vbus;
+	palmas_usb->comparator.start_srp = palmas_start_srp;
+
+	palmas_usb_wakeup(palmas, pdata->wakeup);
+
+	/* init spinlock for workqueue */
+	spin_lock_init(&palmas_usb->lock);
+
+	if (!pdata->no_control_vbus) {
+		palmas_usb->vbus_reg = devm_regulator_get(&pdev->dev, "vbus");
+		if (IS_ERR(palmas_usb->vbus_reg)) {
+			dev_err(&pdev->dev, "vbus init failed\n");
+			return PTR_ERR(palmas_usb->vbus_reg);
+		}
+	}
+
+	platform_set_drvdata(pdev, palmas_usb);
+
+	if (device_create_file(&pdev->dev, &dev_attr_vbus))
+		dev_warn(&pdev->dev, "could not create sysfs file\n");
+
+	palmas_usb->edev.name = "palmas_usb";
+	palmas_usb->edev.supported_cable = palmas_extcon_cable;
+	palmas_usb->edev.mutually_exclusive = mutually_exclusive;
+
+	ret = extcon_dev_register(&palmas_usb->edev, palmas_usb->dev);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to register extcon device\n");
+		return ret;
+	}
+
+	/* init spinlock for workqueue */
+	spin_lock_init(&palmas_usb->lock);
+
+	INIT_WORK(&palmas_usb->set_vbus_work, palmas_set_vbus_work);
+
+	status = devm_request_threaded_irq(palmas_usb->dev, palmas_usb->irq2,
+			NULL, palmas_id_wakeup_irq,
+			IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING,
+			"palmas_usb", palmas_usb);
+	if (status < 0) {
+		dev_err(&pdev->dev, "can't get IRQ %d, err %d\n",
+					palmas_usb->irq2, status);
+		goto fail_irq;
+	}
+
+	status = devm_request_threaded_irq(palmas_usb->dev, palmas_usb->irq4,
+			NULL, palmas_vbus_wakeup_irq,
+			IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING,
+			"palmas_usb", palmas_usb);
+	if (status < 0) {
+		dev_err(&pdev->dev, "can't get IRQ %d, err %d\n",
+					palmas_usb->irq4, status);
+		goto fail_irq;
+	}
+
+	palmas_enable_irq(palmas_usb);
+
+	return 0;
+
+fail_irq:
+	cancel_work_sync(&palmas_usb->set_vbus_work);
+	device_remove_file(palmas_usb->dev, &dev_attr_vbus);
+
+	return status;
+}
+
+static int palmas_usb_remove(struct platform_device *pdev)
+{
+	struct palmas_usb *palmas_usb = platform_get_drvdata(pdev);
+
+	device_remove_file(palmas_usb->dev, &dev_attr_vbus);
+	cancel_work_sync(&palmas_usb->set_vbus_work);
+	extcon_dev_unregister(&palmas_usb->edev);
+
+	return 0;
+}
+
+static struct of_device_id of_palmas_match_tbl[] = {
+	{ .compatible = "ti,palmas-usb", },
+	{ .compatible = "ti,twl6035-usb", },
+	{ /* end */ }
+};
+
+static struct platform_driver palmas_usb_driver = {
+	.probe = palmas_usb_probe,
+	.remove = palmas_usb_remove,
+	.driver = {
+		.name = "palmas-usb",
+		.of_match_table = of_palmas_match_tbl,
+		.owner = THIS_MODULE,
+	},
+};
+
+module_platform_driver(palmas_usb_driver);
+
+MODULE_ALIAS("platform:palmas-usb");
+MODULE_AUTHOR("Graeme Gregory <gg@...mlogic.co.uk>");
+MODULE_DESCRIPTION("Palmas USB transceiver driver");
+MODULE_LICENSE("GPL");
+MODULE_DEVICE_TABLE(of, of_palmas_match_tbl);
diff --git a/include/linux/extcon/extcon_palmas.h b/include/linux/extcon/extcon_palmas.h
new file mode 100644
index 0000000..a5119c9
--- /dev/null
+++ b/include/linux/extcon/extcon_palmas.h
@@ -0,0 +1,26 @@
+/*
+ * extcon_palmas.h - palmas extcon driver to detect VBUS or ID events
+ *
+ * Copyright (C) 2013 Texas Instruments Incorporated - http://www.ti.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.
+ *
+ * Author: Kishon Vijay Abraham I <kishon@ti.com>
+ *
+ * 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.
+ *
+ */
+
+#ifndef __EXTCON_PALMAS_H__
+#define __EXTCON_PALMAS_H__
+
+#define	PALMAS_USB_STATE_DISCONNECT	0x0
+#define	PALMAS_USB_STATE_VBUS		BIT(0)
+#define	PALMAS_USB_STATE_ID		BIT(1)
+
+#endif	/* __EXTCON_PALMAS_H__ */
diff --git a/include/linux/mfd/palmas.h b/include/linux/mfd/palmas.h
index 8f21daf..d671679 100644
--- a/include/linux/mfd/palmas.h
+++ b/include/linux/mfd/palmas.h
@@ -18,8 +18,10 @@
 
 #include <linux/usb/otg.h>
 #include <linux/leds.h>
+#include <linux/extcon.h>
 #include <linux/regmap.h>
 #include <linux/regulator/driver.h>
+#include <linux/usb/phy_companion.h>
 
 #define PALMAS_NUM_CLIENTS		3
 
@@ -349,6 +351,9 @@ struct palmas_resource {
 struct palmas_usb {
 	struct palmas *palmas;
 	struct device *dev;
+	struct extcon_dev edev;
+
+	struct phy_companion comparator;
 
 	/* for vbus reporting with irqs disabled */
 	spinlock_t lock;
@@ -365,7 +370,8 @@ struct palmas_usb {
 
 	int vbus_enable;
 
-	u8 linkstat;
+	int mailboxstat;
+	int linkstat;
 };
 
 #define comparator_to_palmas(x) container_of((x), struct palmas_usb, comparator)
-- 
1.7.10.4

Re: [PATCH v4] extcon: Palmas Extcon Driver

[Laxman Dewangan]

On Monday 06 May 2013 06:47 PM, Kishon Vijay Abraham I wrote:
> +
> +static irqreturn_t palmas_vbus_wakeup_irq(int irq, void *_palmas_usb)

Can we name the function to palams_vbus_irq_handler() for better 
understanding? Reserve the wakeup word for the suspend-wakeups.


> +
> +
> +static irqreturn_t palmas_id_wakeup_irq(int irq, void *_palmas_usb)

Same here for better name.


> +
> +static void palmas_set_vbus_work(struct work_struct *data)
> +{
> +       int ret;
> +       struct palmas_usb *palmas_usb = container_of(data, struct palmas_usb,
> +                                                               set_vbus_work);
> +
> +       if (IS_ERR_OR_NULL(palmas_usb->vbus_reg)) {
> +               dev_err(palmas_usb->dev, "invalid regulator\n");
> +               return;
> +       }

This error will keep coming if the vbus is not require as workqueue get 
scheduled always. I think we should remove it.


> +

> +static void palmas_dt_to_pdata(struct device_node *node,
> +               struct palmas_usb_platform_data *pdata)
> +{
> +       pdata->no_control_vbus = of_property_read_bool(node,
> +                                       "ti,no_control_vbus");


Can we change the variable names to enable_control_bus and logic 
accordingly as it looks more appropriate and easy to understand?


> +
> +       palmas_usb->irq1 = regmap_irq_get_virq(palmas->irq_data,
> +                                               PALMAS_ID_OTG_IRQ);
> +       palmas_usb->irq2 = regmap_irq_get_virq(palmas->irq_data,
> +                                               PALMAS_ID_IRQ);
> +       palmas_usb->irq3 = regmap_irq_get_virq(palmas->irq_data,
> +                                               PALMAS_VBUS_OTG_IRQ);
> +       palmas_usb->irq4 = regmap_irq_get_virq(palmas->irq_data,
> +                                               PALMAS_VBUS_IRQ);
> +

Better to name irq1, irq2 in more logical names for easy understanding.


> +
> +       if (device_create_file(&pdev->dev, &dev_attr_vbus))
> +               dev_warn(&pdev->dev, "could not create sysfs file\n");
> +
> +       palmas_usb->edev.name = "palmas_usb";
> +       palmas_usb->edev.supported_cable = palmas_extcon_cable;
> +       palmas_usb->edev.mutually_exclusive = mutually_exclusive;
> +
> +       ret = extcon_dev_register(&palmas_usb->edev, palmas_usb->dev);
> +       if (ret) {
> +               dev_err(&pdev->dev, "failed to register extcon device\n");
> +               return ret;

It need to destroy sysfs also.

> +       }
> +
> +       /* init spinlock for workqueue */
> +       spin_lock_init(&palmas_usb->lock);

It is already done above.

> +
> +       INIT_WORK(&palmas_usb->set_vbus_work, palmas_set_vbus_work);

Better to create the workqueu when control_vbus is require.


> +
> +
> diff --git a/include/linux/extcon/extcon_palmas.h b/include/linux/extcon/extcon_palmas.h
> new file mode 100644
> index 0000000..a5119c9
> --- /dev/null
> +++ b/include/linux/extcon/extcon_palmas.h

I think it can be use palama.h only. No need to have one more header for 
this.

> @@ -0,0 +1,26 @@
>
>
> -       u8 linkstat;
> +       int mailboxstat;
Do we really require mailboxstat?

Re: [PATCH v4] extcon: Palmas Extcon Driver

[Mark Brown]

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

On Mon, May 06, 2013 at 06:47:04PM +0530, Kishon Vijay Abraham I wrote:

> +		if (palmas_usb->linkstat != PALMAS_USB_STATE_VBUS) {
> +			if (palmas_usb->vbus_reg) {
> +				ret = regulator_enable(palmas_usb->vbus_reg);
> +				if (ret) {
> +					dev_dbg(palmas_usb->dev,
> +						"regulator enable failed\n");
> +					goto ret0;

This is very bad karma, why is the regulator optional?

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

RE: [PATCH v4] extcon: Palmas Extcon Driver

[myungjoo.ham]

> From: Graeme Gregory <gg@slimlogic.co.uk>
> 
> This is the driver for the USB comparator built into the palmas chip. It
> handles the various USB OTG events that can be generated by cable
> insertion/removal.
> 
> Signed-off-by: Graeme Gregory <gg@slimlogic.co.uk>
> Signed-off-by: Moiz Sonasath <m-sonasath@ti.com>
> Signed-off-by: Ruchika Kharwar <ruchika@ti.com>
> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> [kishon@ti.com: adapted palmas usb driver to use the extcon framework]
> Signed-off-by: Sebastien Guiriec <s-guiriec@ti.com>

Here goes some comments in the code:

[]

> diff --git a/drivers/extcon/extcon-palmas.c
b/drivers/extcon/extcon-palmas.c
> new file mode 100644
> index 0000000..3ef042f
> --- /dev/null
> +++ b/drivers/extcon/extcon-palmas.c
> @@ -0,0 +1,389 @@
> +/*
> + * Palmas USB transceiver driver
> + *
> + * Copyright (C) 2013 Texas Instruments Incorporated - http://www.ti.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.
> + *
> + * Author: Graeme Gregory <gg@...mlogic.co.uk>
> + * Author: Kishon Vijay Abraham I <kishon@...com>
> + *
> + * Based on twl6030_usb.c
> + *
> + * Author: Hema HK <hemahk@...com>
> + *
> + * 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.
> + */

Why the email addresses are masked in the source code?
(I'm just curious as this is the first time to see such in kernel source)

> +
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/platform_device.h>
> +#include <linux/io.h>
> +#include <linux/usb/phy_companion.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/err.h>
> +#include <linux/notifier.h>
> +#include <linux/slab.h>
> +#include <linux/delay.h>
> +#include <linux/mfd/palmas.h>
> +#include <linux/of.h>
> +#include <linux/of_platform.h>
> +#include <linux/extcon/extcon_palmas.h>
> +
> +static const char *palmas_extcon_cable[] = {
> +	[0] = "USB",
> +	[1] = "USB-HOST",

	[1] = "USB-Host",

> +	NULL,
> +};
> +
> +static const int mutually_exclusive[] = {0x3, 0x0};
> +
> +static void palmas_usb_wakeup(struct palmas *palmas, int enable)
> +{
> +	if (enable)
> +		palmas_write(palmas, PALMAS_USB_OTG_BASE, PALMAS_USB_WAKEUP,
> +			PALMAS_USB_WAKEUP_ID_WK_UP_COMP);
> +	else
> +		palmas_write(palmas, PALMAS_USB_OTG_BASE, PALMAS_USB_WAKEUP,
0);
> +}
> +
> +static ssize_t palmas_usb_vbus_show(struct device *dev,
> +			struct device_attribute *attr, char *buf)
> +{
> +	unsigned long		flags;
> +	int			ret = -EINVAL;
> +	struct palmas_usb	*palmas_usb = dev_get_drvdata(dev);
> +
> +	spin_lock_irqsave(&palmas_usb->lock, flags);

This spinlock is used in this sysfs-show function only.
Is this really needed?
The only protected value here is "linkstat", which is "readonly"
in this protected context.
Thus, removing the spin_lock and updating like the following should
be the same with less overhead:

	int linkstat = palmas_usb->linkstat;
	switch(linkstat) {


> +
> +	switch (palmas_usb->linkstat) {
> +	case PALMAS_USB_STATE_VBUS:
> +	       ret = snprintf(buf, PAGE_SIZE, "vbus\n");
> +	       break;
> +	case PALMAS_USB_STATE_ID:
> +	       ret = snprintf(buf, PAGE_SIZE, "id\n");
> +	       break;
> +	case PALMAS_USB_STATE_DISCONNECT:
> +	       ret = snprintf(buf, PAGE_SIZE, "none\n");
> +	       break;
> +	default:
> +	       ret = snprintf(buf, PAGE_SIZE, "UNKNOWN\n");
> +	}
> +	spin_unlock_irqrestore(&palmas_usb->lock, flags);
> +
> +	return ret;
> +}
> +static DEVICE_ATTR(vbus, 0444, palmas_usb_vbus_show, NULL);
> +

[]

> +
> +static void palmas_set_vbus_work(struct work_struct *data)
> +{
> +	int ret;
> +	struct palmas_usb *palmas_usb = container_of(data, struct
palmas_usb,
> +
set_vbus_work);
> +
> +	if (IS_ERR_OR_NULL(palmas_usb->vbus_reg)) {
> +		dev_err(palmas_usb->dev, "invalid regulator\n");
> +		return;
> +	}
> +
> +	/*
> +	 * Start driving VBUS. Set OPA_MODE bit in CHARGERUSB_CTRL1
> +	 * register. This enables boost mode.
> +	 */
> +
> +	if (palmas_usb->vbus_enable) {
> +		ret = regulator_enable(palmas_usb->vbus_reg);
> +		if (ret)
> +			dev_dbg(palmas_usb->dev, "regulator enable
failed\n");
> +	} else {
> +		regulator_disable(palmas_usb->vbus_reg);
> +	}
> +}
> +
> +static int palmas_set_vbus(struct phy_companion *comparator, bool
enabled)
> +{
> +	struct palmas_usb *palmas_usb = comparator_to_palmas(comparator);
> +
> +	palmas_usb->vbus_enable = enabled;
> +	schedule_work(&palmas_usb->set_vbus_work);
> +
> +	return 0;
> +}
> +
> +static int palmas_start_srp(struct phy_companion *comparator)
> +{
> +	struct palmas_usb *palmas_usb = comparator_to_palmas(comparator);
> +
> +	palmas_write(palmas_usb->palmas, PALMAS_USB_OTG_BASE,
> +		PALMAS_USB_VBUS_CTRL_SET,
PALMAS_USB_VBUS_CTRL_SET_VBUS_DISCHRG
> +		| PALMAS_USB_VBUS_CTRL_SET_VBUS_IADP_SINK);
> +	palmas_write(palmas_usb->palmas, PALMAS_USB_OTG_BASE,
> +		PALMAS_USB_VBUS_CTRL_SET,
> +		PALMAS_USB_VBUS_CTRL_SET_VBUS_CHRG_VSYS |
> +		PALMAS_USB_VBUS_CTRL_SET_VBUS_IADP_SINK);
> +
> +	mdelay(100);

Why not msleep()? It's long enough to consider sleep.
Is this in an atomic context? (if so, 100msec seems even more awful)

> +
> +	palmas_write(palmas_usb->palmas, PALMAS_USB_OTG_BASE,
> +		PALMAS_USB_VBUS_CTRL_CLR,
> +		PALMAS_USB_VBUS_CTRL_SET_VBUS_CHRG_VSYS |
> +		PALMAS_USB_VBUS_CTRL_SET_VBUS_CHRG_VSYS);
> +
> +	return 0;
> +}
> +
> +static void palmas_dt_to_pdata(struct device_node *node,
> +		struct palmas_usb_platform_data *pdata)
> +{
> +	pdata->no_control_vbus = of_property_read_bool(node,
> +					"ti,no_control_vbus");
> +	pdata->wakeup = of_property_read_bool(node, "ti,wakeup");
> +}
> +
> +static int palmas_usb_probe(struct platform_device *pdev)
> +{

[]

> +	/* init spinlock for workqueue */
> +	spin_lock_init(&palmas_usb->lock);

[]

> +	/* init spinlock for workqueue */
> +	spin_lock_init(&palmas_usb->lock);

Why this lock is initialized again?

> +
> +	INIT_WORK(&palmas_usb->set_vbus_work, palmas_set_vbus_work);
> +

[]

> +
> +module_platform_driver(palmas_usb_driver);
> +
> +MODULE_ALIAS("platform:palmas-usb");
> +MODULE_AUTHOR("Graeme Gregory <gg@...mlogic.co.uk>");

Is this intentional?

> +MODULE_DESCRIPTION("Palmas USB transceiver driver");
> +MODULE_LICENSE("GPL");
> +MODULE_DEVICE_TABLE(of, of_palmas_match_tbl);
>


Cheers,
MyungJoo

Re: [PATCH v4] extcon: Palmas Extcon Driver

[Kishon Vijay Abraham I]

Hi,

On Monday 06 May 2013 07:56 PM, Laxman Dewangan wrote:
> On Monday 06 May 2013 06:47 PM, Kishon Vijay Abraham I wrote:
>> +
>> +static irqreturn_t palmas_vbus_wakeup_irq(int irq, void *_palmas_usb)
>
> Can we name the function to palams_vbus_irq_handler() for better
> understanding? Reserve the wakeup word for the suspend-wakeups.
>
>
>> +
>> +
>> +static irqreturn_t palmas_id_wakeup_irq(int irq, void *_palmas_usb)
>
> Same here for better name.
>
>
>> +
>> +static void palmas_set_vbus_work(struct work_struct *data)
>> +{
>> +       int ret;
>> +       struct palmas_usb *palmas_usb = container_of(data, struct
>> palmas_usb,
>> +
>> set_vbus_work);
>> +
>> +       if (IS_ERR_OR_NULL(palmas_usb->vbus_reg)) {
>> +               dev_err(palmas_usb->dev, "invalid regulator\n");
>> +               return;
>> +       }
>
> This error will keep coming if the vbus is not require as workqueue get
> scheduled always. I think we should remove it.
>
>
>> +
>
>> +static void palmas_dt_to_pdata(struct device_node *node,
>> +               struct palmas_usb_platform_data *pdata)
>> +{
>> +       pdata->no_control_vbus = of_property_read_bool(node,
>> +                                       "ti,no_control_vbus");
>
>
> Can we change the variable names to enable_control_bus and logic
> accordingly as it looks more appropriate and easy to understand?
>
>
>> +
>> +       palmas_usb->irq1 = regmap_irq_get_virq(palmas->irq_data,
>> +                                               PALMAS_ID_OTG_IRQ);
>> +       palmas_usb->irq2 = regmap_irq_get_virq(palmas->irq_data,
>> +                                               PALMAS_ID_IRQ);
>> +       palmas_usb->irq3 = regmap_irq_get_virq(palmas->irq_data,
>> +                                               PALMAS_VBUS_OTG_IRQ);
>> +       palmas_usb->irq4 = regmap_irq_get_virq(palmas->irq_data,
>> +                                               PALMAS_VBUS_IRQ);
>> +
>
> Better to name irq1, irq2 in more logical names for easy understanding.
>
>
>> +
>> +       if (device_create_file(&pdev->dev, &dev_attr_vbus))
>> +               dev_warn(&pdev->dev, "could not create sysfs file\n");
>> +
>> +       palmas_usb->edev.name = "palmas_usb";
>> +       palmas_usb->edev.supported_cable = palmas_extcon_cable;
>> +       palmas_usb->edev.mutually_exclusive = mutually_exclusive;
>> +
>> +       ret = extcon_dev_register(&palmas_usb->edev, palmas_usb->dev);
>> +       if (ret) {
>> +               dev_err(&pdev->dev, "failed to register extcon
>> device\n");
>> +               return ret;
>
> It need to destroy sysfs also.
>
>> +       }
>> +
>> +       /* init spinlock for workqueue */
>> +       spin_lock_init(&palmas_usb->lock);
>
> It is already done above.
>
>> +
>> +       INIT_WORK(&palmas_usb->set_vbus_work, palmas_set_vbus_work);
>
> Better to create the workqueu when control_vbus is require.
>
>
>> +
>> +
>> diff --git a/include/linux/extcon/extcon_palmas.h
>> b/include/linux/extcon/extcon_palmas.h
>> new file mode 100644
>> index 0000000..a5119c9
>> --- /dev/null
>> +++ b/include/linux/extcon/extcon_palmas.h
>
> I think it can be use palama.h only. No need to have one more header for
> this.
>
>> @@ -0,0 +1,26 @@
>>
>>
>> -       u8 linkstat;
>> +       int mailboxstat;
> Do we really require mailboxstat?

Will fix your comments.

Thanks
Kishon

Re: [PATCH v4] extcon: Palmas Extcon Driver

[Kishon Vijay Abraham I]

Hi,

On Monday 06 May 2013 08:10 PM, Mark Brown wrote:
> On Mon, May 06, 2013 at 06:47:04PM +0530, Kishon Vijay Abraham I wrote:
>
>> +		if (palmas_usb->linkstat != PALMAS_USB_STATE_VBUS) {
>> +			if (palmas_usb->vbus_reg) {
>> +				ret = regulator_enable(palmas_usb->vbus_reg);
>> +				if (ret) {
>> +					dev_dbg(palmas_usb->dev,
>> +						"regulator enable failed\n");
>> +					goto ret0;
>
> This is very bad karma, why is the regulator optional?

The platform can provide it's own vbus control in which case this is not 
needed.

Thanks
Kishon

Re: [PATCH v4] extcon: Palmas Extcon Driver

[Kishon Vijay Abraham I]

Hi,

On Tuesday 07 May 2013 06:13 AM, myungjoo.ham wrote:
>> From: Graeme Gregory <gg@slimlogic.co.uk>
>>
>> This is the driver for the USB comparator built into the palmas chip. It
>> handles the various USB OTG events that can be generated by cable
>> insertion/removal.
>>
>> Signed-off-by: Graeme Gregory <gg@slimlogic.co.uk>
>> Signed-off-by: Moiz Sonasath <m-sonasath@ti.com>
>> Signed-off-by: Ruchika Kharwar <ruchika@ti.com>
>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>> [kishon@ti.com: adapted palmas usb driver to use the extcon framework]
>> Signed-off-by: Sebastien Guiriec <s-guiriec@ti.com>
>
> Here goes some comments in the code:
>
> []
>
>> diff --git a/drivers/extcon/extcon-palmas.c
> b/drivers/extcon/extcon-palmas.c
>> new file mode 100644
>> index 0000000..3ef042f
>> --- /dev/null
>> +++ b/drivers/extcon/extcon-palmas.c
>> @@ -0,0 +1,389 @@
>> +/*
>> + * Palmas USB transceiver driver
>> + *
>> + * Copyright (C) 2013 Texas Instruments Incorporated - http://www.ti.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.
>> + *
>> + * Author: Graeme Gregory <gg@...mlogic.co.uk>
>> + * Author: Kishon Vijay Abraham I <kishon@...com>
>> + *
>> + * Based on twl6030_usb.c
>> + *
>> + * Author: Hema HK <hemahk@...com>
>> + *
>> + * 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.
>> + */
>
> Why the email addresses are masked in the source code?
> (I'm just curious as this is the first time to see such in kernel source)

That was not intentional. Took the previous version from the net. My bad.
>
>> +
>> +#include <linux/module.h>
>> +#include <linux/init.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/io.h>
>> +#include <linux/usb/phy_companion.h>
>> +#include <linux/regulator/consumer.h>
>> +#include <linux/err.h>
>> +#include <linux/notifier.h>
>> +#include <linux/slab.h>
>> +#include <linux/delay.h>
>> +#include <linux/mfd/palmas.h>
>> +#include <linux/of.h>
>> +#include <linux/of_platform.h>
>> +#include <linux/extcon/extcon_palmas.h>
>> +
>> +static const char *palmas_extcon_cable[] = {
>> +	[0] = "USB",
>> +	[1] = "USB-HOST",
>
> 	[1] = "USB-Host",
>
>> +	NULL,
>> +};
>> +
>> +static const int mutually_exclusive[] = {0x3, 0x0};
>> +
>> +static void palmas_usb_wakeup(struct palmas *palmas, int enable)
>> +{
>> +	if (enable)
>> +		palmas_write(palmas, PALMAS_USB_OTG_BASE, PALMAS_USB_WAKEUP,
>> +			PALMAS_USB_WAKEUP_ID_WK_UP_COMP);
>> +	else
>> +		palmas_write(palmas, PALMAS_USB_OTG_BASE, PALMAS_USB_WAKEUP,
> 0);
>> +}
>> +
>> +static ssize_t palmas_usb_vbus_show(struct device *dev,
>> +			struct device_attribute *attr, char *buf)
>> +{
>> +	unsigned long		flags;
>> +	int			ret = -EINVAL;
>> +	struct palmas_usb	*palmas_usb = dev_get_drvdata(dev);
>> +
>> +	spin_lock_irqsave(&palmas_usb->lock, flags);
>
> This spinlock is used in this sysfs-show function only.
> Is this really needed?
> The only protected value here is "linkstat", which is "readonly"
> in this protected context.
> Thus, removing the spin_lock and updating like the following should
> be the same with less overhead:
>
> 	int linkstat = palmas_usb->linkstat;
> 	switch(linkstat) {

hmm.. ok.
>
>
>> +
>> +	switch (palmas_usb->linkstat) {
>> +	case PALMAS_USB_STATE_VBUS:
>> +	       ret = snprintf(buf, PAGE_SIZE, "vbus\n");
>> +	       break;
>> +	case PALMAS_USB_STATE_ID:
>> +	       ret = snprintf(buf, PAGE_SIZE, "id\n");
>> +	       break;
>> +	case PALMAS_USB_STATE_DISCONNECT:
>> +	       ret = snprintf(buf, PAGE_SIZE, "none\n");
>> +	       break;
>> +	default:
>> +	       ret = snprintf(buf, PAGE_SIZE, "UNKNOWN\n");
>> +	}
>> +	spin_unlock_irqrestore(&palmas_usb->lock, flags);
>> +
>> +	return ret;
>> +}
>> +static DEVICE_ATTR(vbus, 0444, palmas_usb_vbus_show, NULL);
>> +
>
> []
>
>> +
>> +static void palmas_set_vbus_work(struct work_struct *data)
>> +{
>> +	int ret;
>> +	struct palmas_usb *palmas_usb = container_of(data, struct
> palmas_usb,
>> +
> set_vbus_work);
>> +
>> +	if (IS_ERR_OR_NULL(palmas_usb->vbus_reg)) {
>> +		dev_err(palmas_usb->dev, "invalid regulator\n");
>> +		return;
>> +	}
>> +
>> +	/*
>> +	 * Start driving VBUS. Set OPA_MODE bit in CHARGERUSB_CTRL1
>> +	 * register. This enables boost mode.
>> +	 */
>> +
>> +	if (palmas_usb->vbus_enable) {
>> +		ret = regulator_enable(palmas_usb->vbus_reg);
>> +		if (ret)
>> +			dev_dbg(palmas_usb->dev, "regulator enable
> failed\n");
>> +	} else {
>> +		regulator_disable(palmas_usb->vbus_reg);
>> +	}
>> +}
>> +
>> +static int palmas_set_vbus(struct phy_companion *comparator, bool
> enabled)
>> +{
>> +	struct palmas_usb *palmas_usb = comparator_to_palmas(comparator);
>> +
>> +	palmas_usb->vbus_enable = enabled;
>> +	schedule_work(&palmas_usb->set_vbus_work);
>> +
>> +	return 0;
>> +}
>> +
>> +static int palmas_start_srp(struct phy_companion *comparator)
>> +{
>> +	struct palmas_usb *palmas_usb = comparator_to_palmas(comparator);
>> +
>> +	palmas_write(palmas_usb->palmas, PALMAS_USB_OTG_BASE,
>> +		PALMAS_USB_VBUS_CTRL_SET,
> PALMAS_USB_VBUS_CTRL_SET_VBUS_DISCHRG
>> +		| PALMAS_USB_VBUS_CTRL_SET_VBUS_IADP_SINK);
>> +	palmas_write(palmas_usb->palmas, PALMAS_USB_OTG_BASE,
>> +		PALMAS_USB_VBUS_CTRL_SET,
>> +		PALMAS_USB_VBUS_CTRL_SET_VBUS_CHRG_VSYS |
>> +		PALMAS_USB_VBUS_CTRL_SET_VBUS_IADP_SINK);
>> +
>> +	mdelay(100);
>
> Why not msleep()? It's long enough to consider sleep.
> Is this in an atomic context? (if so, 100msec seems even more awful)

I guess it shouldn't be called from atomic context. Actually srp hasn't 
been tested because the controller driver we use with palmas dont have 
the infrastructure yet.
>
>> +
>> +	palmas_write(palmas_usb->palmas, PALMAS_USB_OTG_BASE,
>> +		PALMAS_USB_VBUS_CTRL_CLR,
>> +		PALMAS_USB_VBUS_CTRL_SET_VBUS_CHRG_VSYS |
>> +		PALMAS_USB_VBUS_CTRL_SET_VBUS_CHRG_VSYS);
>> +
>> +	return 0;
>> +}
>> +
>> +static void palmas_dt_to_pdata(struct device_node *node,
>> +		struct palmas_usb_platform_data *pdata)
>> +{
>> +	pdata->no_control_vbus = of_property_read_bool(node,
>> +					"ti,no_control_vbus");
>> +	pdata->wakeup = of_property_read_bool(node, "ti,wakeup");
>> +}
>> +
>> +static int palmas_usb_probe(struct platform_device *pdev)
>> +{
>
> []
>
>> +	/* init spinlock for workqueue */
>> +	spin_lock_init(&palmas_usb->lock);
>
> []
>
>> +	/* init spinlock for workqueue */
>> +	spin_lock_init(&palmas_usb->lock);
>
> Why this lock is initialized again?

Will remove it.

Thanks
Kishon

Re: [PATCH v4] extcon: Palmas Extcon Driver

[Chanwoo Choi]

Hi Kishon,

I add some opinion about this patch.

On 05/06/2013 10:17 PM, Kishon Vijay Abraham I wrote:
> From: Graeme Gregory <gg@slimlogic.co.uk>
>
> This is the driver for the USB comparator built into the palmas chip. It
> handles the various USB OTG events that can be generated by cable
> insertion/removal.
>
> Signed-off-by: Graeme Gregory <gg@slimlogic.co.uk>
> Signed-off-by: Moiz Sonasath <m-sonasath@ti.com>
> Signed-off-by: Ruchika Kharwar <ruchika@ti.com>
> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> [kishon@ti.com: adapted palmas usb driver to use the extcon framework]
> Signed-off-by: Sebastien Guiriec <s-guiriec@ti.com>
> ---
> Changes from v3:
> * adapted the driver to extcon framework (so moved to drivers/extcon)
> * removed palmas_usb_(write/read) and replaced all calls with
>   palmas_(read/write).
> * ignored a checkpatch warning in the line 
> 	static const char *palmas_extcon_cable[] = {
>   as it seemed to be incorrect?
> * removed all references to OMAP in this driver.
> * couldn't test this driver with mainline as omap5 panda is not booting
>   with mainline.
> * A comment to change to platform_get_irq from regmap is not done as I felt
>   the data should come from regmap in this case. Graeme?
> Changes from v2:
> * Moved the driver to drivers/usb/phy/
> * Added a bit more explanation in Kconfig
>  .../devicetree/bindings/extcon/extcon-twl.txt      |   17 +
>  drivers/extcon/Kconfig                             |    7 +
>  drivers/extcon/Makefile                            |    1 +
>  drivers/extcon/extcon-palmas.c                     |  389 ++++++++++++++++++++
>  include/linux/extcon/extcon_palmas.h               |   26 ++
>  include/linux/mfd/palmas.h                         |    8 +-
>  6 files changed, 447 insertions(+), 1 deletion(-)
>  create mode 100644 Documentation/devicetree/bindings/extcon/extcon-twl.txt
>  create mode 100644 drivers/extcon/extcon-palmas.c
>  create mode 100644 include/linux/extcon/extcon_palmas.h
>
> diff --git a/Documentation/devicetree/bindings/extcon/extcon-twl.txt b/Documentation/devicetree/bindings/extcon/extcon-twl.txt
> new file mode 100644
> index 0000000..a7f6527
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/extcon/extcon-twl.txt
> @@ -0,0 +1,17 @@
> +EXTCON FOR TWL CHIPS
> +
> +PALMAS USB COMPARATOR
> +Required Properties:
> + - compatible : Should be "ti,palmas-usb" or "ti,twl6035-usb"
> + - vbus-supply : phandle to the regulator device tree node.
> +
> +Optional Properties:
> + - ti,wakeup : To enable the wakeup comparator in probe
> + - ti,no_control_vbus: if the platform wishes its own vbus control
> +
> +palmas-usb {
> +       compatible = "ti,twl6035-usb", "ti,palmas-usb";
> +       vbus-supply = <&smps10_reg>;
> +       ti,wakeup;
> +};
> +
> diff --git a/drivers/extcon/Kconfig b/drivers/extcon/Kconfig
> index 5168a13..c881899 100644
> --- a/drivers/extcon/Kconfig
> +++ b/drivers/extcon/Kconfig
> @@ -53,4 +53,11 @@ config EXTCON_ARIZONA
>  	  with Wolfson Arizona devices. These are audio CODECs with
>  	  advanced audio accessory detection support.
>  
> +config EXTCON_PALMAS
> +	tristate "Palmas USB EXTCON support"
> +	depends on MFD_PALMAS
> +	help
> +	  Say Y here to enable support for USB peripheral and USB host
> +	  detection by palmas usb.
> +
>  endif # MULTISTATE_SWITCH
> diff --git a/drivers/extcon/Makefile b/drivers/extcon/Makefile
> index f98a3c4..540e2c3 100644
> --- a/drivers/extcon/Makefile
> +++ b/drivers/extcon/Makefile
> @@ -8,3 +8,4 @@ obj-$(CONFIG_EXTCON_ADC_JACK)	+= extcon-adc-jack.o
>  obj-$(CONFIG_EXTCON_MAX77693)	+= extcon-max77693.o
>  obj-$(CONFIG_EXTCON_MAX8997)	+= extcon-max8997.o
>  obj-$(CONFIG_EXTCON_ARIZONA)	+= extcon-arizona.o
> +obj-$(CONFIG_EXTCON_PALMAS)	+= extcon-palmas.o
> diff --git a/drivers/extcon/extcon-palmas.c b/drivers/extcon/extcon-palmas.c
> new file mode 100644
> index 0000000..3ef042f
> --- /dev/null
> +++ b/drivers/extcon/extcon-palmas.c
> @@ -0,0 +1,389 @@
> +/*
> + * Palmas USB transceiver driver
> + *
> + * Copyright (C) 2013 Texas Instruments Incorporated - http://www.ti.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.
> + *
> + * Author: Graeme Gregory <gg@...mlogic.co.uk>
> + * Author: Kishon Vijay Abraham I <kishon@...com>
> + *
> + * Based on twl6030_usb.c
> + *
> + * Author: Hema HK <hemahk@...com>
> + *
> + * 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/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/platform_device.h>
> +#include <linux/io.h>
> +#include <linux/usb/phy_companion.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/err.h>
> +#include <linux/notifier.h>
> +#include <linux/slab.h>
> +#include <linux/delay.h>
> +#include <linux/mfd/palmas.h>
> +#include <linux/of.h>
> +#include <linux/of_platform.h>
> +#include <linux/extcon/extcon_palmas.h>
> +
Remove unnecessary header file. When I remove following header file,
I completed kernel build without any problem.

linux/init.h
linux/io.h
linux/regulator/consumer.h
linux/notifier.h
linux/slab.h
linux/delay.h
> +static const char *palmas_extcon_cable[] = {
> +	[0] = "USB",
> +	[1] = "USB-HOST",
> +	NULL,
> +};
> +
> +static const int mutually_exclusive[] = {0x3, 0x0};
> +
> +static void palmas_usb_wakeup(struct palmas *palmas, int enable)
> +{
> +	if (enable)
> +		palmas_write(palmas, PALMAS_USB_OTG_BASE, PALMAS_USB_WAKEUP,
> +			PALMAS_USB_WAKEUP_ID_WK_UP_COMP);
> +	else
> +		palmas_write(palmas, PALMAS_USB_OTG_BASE, PALMAS_USB_WAKEUP, 0);
> +}
> +
> +static ssize_t palmas_usb_vbus_show(struct device *dev,
> +			struct device_attribute *attr, char *buf)
> +{
> +	unsigned long		flags;
> +	int			ret = -EINVAL;
> +	struct palmas_usb	*palmas_usb = dev_get_drvdata(dev);
> +
> +	spin_lock_irqsave(&palmas_usb->lock, flags);
> +
> +	switch (palmas_usb->linkstat) {
> +	case PALMAS_USB_STATE_VBUS:
> +	       ret = snprintf(buf, PAGE_SIZE, "vbus\n");
> +	       break;
> +	case PALMAS_USB_STATE_ID:
> +	       ret = snprintf(buf, PAGE_SIZE, "id\n");
> +	       break;
> +	case PALMAS_USB_STATE_DISCONNECT:
> +	       ret = snprintf(buf, PAGE_SIZE, "none\n");
> +	       break;
> +	default:
> +	       ret = snprintf(buf, PAGE_SIZE, "UNKNOWN\n");
> +	}
> +	spin_unlock_irqrestore(&palmas_usb->lock, flags);
> +
> +	return ret;
> +}
> +static DEVICE_ATTR(vbus, 0444, palmas_usb_vbus_show, NULL);
I think that 'palmas_usb_vbus' device attribute isn't standard node
of extcon framework. If you want to check USB/USB-HOST state
on user-space, user process should use following standard node
of extcon instead of 'palmas_usb_vbus' node.

- User can get the state of USB/USB-HOST through following node:
/sys/class/extcon/palmas_usb/cable.0/state
    if state is 1, PALMAS_USB_STATE_VBUS
    if state is 0, PALMAS_USB_STATE_DISCONNECT

/sys/class/extcon/palmas_usb/cable.1/state
    if state is 1, PALMAS_USB_STATE_ID
    if state is 0, PALMAS_USB_STATE_DISCONNECT

Certainly, extcon driver have to provide specific information of external connector through
standard sysfs entry.
> +
> +static irqreturn_t palmas_vbus_wakeup_irq(int irq, void *_palmas_usb)
> +{
> +	int ret;
> +	struct palmas_usb *palmas_usb = _palmas_usb;
> +	unsigned int vbus_line_state;
> +
> +	palmas_read(palmas_usb->palmas, PALMAS_INTERRUPT_BASE,
> +		PALMAS_INT3_LINE_STATE, &vbus_line_state);
> +
> +	if (vbus_line_state & PALMAS_INT3_LINE_STATE_VBUS) {
> +		if (palmas_usb->linkstat != PALMAS_USB_STATE_VBUS) {
> +			if (palmas_usb->vbus_reg) {
> +				ret = regulator_enable(palmas_usb->vbus_reg);
> +				if (ret) {
> +					dev_dbg(palmas_usb->dev,
> +						"regulator enable failed\n");
> +					goto ret0;
> +				}
> +			}
> +			palmas_usb->linkstat = PALMAS_USB_STATE_VBUS;
> +			extcon_set_cable_state(&palmas_usb->edev, "USB", true);
> +		} else {
> +			dev_dbg(palmas_usb->dev,
> +				"Spurious connect event detected\n");
> +		}
> +	} else if (!(vbus_line_state & PALMAS_INT3_LINE_STATE_VBUS)) {
> +		if (palmas_usb->linkstat == PALMAS_USB_STATE_VBUS) {
> +			if (palmas_usb->vbus_reg)
> +				regulator_disable(palmas_usb->vbus_reg);
> +			palmas_usb->linkstat = PALMAS_USB_STATE_DISCONNECT;
> +			extcon_set_cable_state(&palmas_usb->edev, "USB", false);
> +		} else {
> +			dev_dbg(palmas_usb->dev,
> +				"Spurious disconnect event detected\n");
> +		}
> +	}
> +
> +ret0:
> +	return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t palmas_id_wakeup_irq(int irq, void *_palmas_usb)
> +{
> +	int			ret;
> +	unsigned int		set;
> +	struct palmas_usb	*palmas_usb = _palmas_usb;
> +
> +	palmas_read(palmas_usb->palmas, PALMAS_USB_OTG_BASE,
> +		PALMAS_USB_ID_INT_LATCH_SET, &set);
> +
> +	if (set & PALMAS_USB_ID_INT_SRC_ID_GND) {
> +		if (palmas_usb->vbus_reg) {
> +			ret = regulator_enable(palmas_usb->vbus_reg);
> +			if (ret) {
> +				dev_dbg(palmas_usb->dev,
> +					"regulator enable failed\n");
> +				goto ret0;
> +			}
> +		}
> +		palmas_write(palmas_usb->palmas, PALMAS_USB_OTG_BASE,
> +			PALMAS_USB_ID_INT_EN_HI_SET,
> +			PALMAS_USB_ID_INT_EN_HI_SET_ID_FLOAT);
> +		palmas_write(palmas_usb->palmas, PALMAS_USB_OTG_BASE,
> +			PALMAS_USB_ID_INT_EN_HI_CLR,
> +			PALMAS_USB_ID_INT_EN_HI_CLR_ID_GND);
> +		palmas_usb->linkstat = PALMAS_USB_STATE_ID;
> +		extcon_set_cable_state(&palmas_usb->edev, "USB-HOST", true);
> +	} else if (set & PALMAS_USB_ID_INT_SRC_ID_FLOAT) {
> +		palmas_write(palmas_usb->palmas, PALMAS_USB_OTG_BASE,
> +			PALMAS_USB_ID_INT_EN_HI_SET,
> +			PALMAS_USB_ID_INT_EN_HI_SET_ID_GND);
> +		palmas_write(palmas_usb->palmas, PALMAS_USB_OTG_BASE,
> +			PALMAS_USB_ID_INT_EN_HI_CLR,
> +			PALMAS_USB_ID_INT_EN_HI_CLR_ID_FLOAT);
> +		if (palmas_usb->vbus_reg)
> +			regulator_disable(palmas_usb->vbus_reg);
> +		palmas_usb->linkstat = PALMAS_USB_STATE_DISCONNECT;
> +		extcon_set_cable_state(&palmas_usb->edev, "USB-HOST", false);
> +	}
> +
> +ret0:
> +	return IRQ_HANDLED;
> +}
> +
> +static void palmas_enable_irq(struct palmas_usb *palmas_usb)
> +{
> +	palmas_write(palmas_usb->palmas, PALMAS_USB_OTG_BASE,
> +		PALMAS_USB_VBUS_CTRL_SET,
> +		PALMAS_USB_VBUS_CTRL_SET_VBUS_ACT_COMP);
> +
> +	palmas_write(palmas_usb->palmas, PALMAS_USB_OTG_BASE,
> +		PALMAS_USB_ID_CTRL_SET, PALMAS_USB_ID_CTRL_SET_ID_ACT_COMP);
> +
> +	palmas_write(palmas_usb->palmas, PALMAS_USB_OTG_BASE,
> +		PALMAS_USB_ID_INT_EN_HI_SET,
> +		PALMAS_USB_ID_INT_EN_HI_SET_ID_GND);
> +
> +	palmas_vbus_wakeup_irq(palmas_usb->irq4, palmas_usb);
> +	palmas_id_wakeup_irq(palmas_usb->irq2, palmas_usb);
> +}
> +
> +static void palmas_set_vbus_work(struct work_struct *data)
> +{
> +	int ret;
> +	struct palmas_usb *palmas_usb = container_of(data, struct palmas_usb,
> +								set_vbus_work);
> +
> +	if (IS_ERR_OR_NULL(palmas_usb->vbus_reg)) {
> +		dev_err(palmas_usb->dev, "invalid regulator\n");
> +		return;
> +	}
> +
> +	/*
> +	 * Start driving VBUS. Set OPA_MODE bit in CHARGERUSB_CTRL1
> +	 * register. This enables boost mode.
> +	 */
> +
> +	if (palmas_usb->vbus_enable) {
> +		ret = regulator_enable(palmas_usb->vbus_reg);
> +		if (ret)
> +			dev_dbg(palmas_usb->dev, "regulator enable failed\n");
> +	} else {
> +		regulator_disable(palmas_usb->vbus_reg);
> +	}
> +}
> +
> +static int palmas_set_vbus(struct phy_companion *comparator, bool enabled)
> +{
> +	struct palmas_usb *palmas_usb = comparator_to_palmas(comparator);
> +
> +	palmas_usb->vbus_enable = enabled;
> +	schedule_work(&palmas_usb->set_vbus_work);
> +
> +	return 0;
> +}
> +
> +static int palmas_start_srp(struct phy_companion *comparator)
> +{
> +	struct palmas_usb *palmas_usb = comparator_to_palmas(comparator);
> +
> +	palmas_write(palmas_usb->palmas, PALMAS_USB_OTG_BASE,
> +		PALMAS_USB_VBUS_CTRL_SET, PALMAS_USB_VBUS_CTRL_SET_VBUS_DISCHRG
> +		| PALMAS_USB_VBUS_CTRL_SET_VBUS_IADP_SINK);
> +	palmas_write(palmas_usb->palmas, PALMAS_USB_OTG_BASE,
> +		PALMAS_USB_VBUS_CTRL_SET,
> +		PALMAS_USB_VBUS_CTRL_SET_VBUS_CHRG_VSYS |
> +		PALMAS_USB_VBUS_CTRL_SET_VBUS_IADP_SINK);
> +
> +	mdelay(100);
> +
> +	palmas_write(palmas_usb->palmas, PALMAS_USB_OTG_BASE,
> +		PALMAS_USB_VBUS_CTRL_CLR,
> +		PALMAS_USB_VBUS_CTRL_SET_VBUS_CHRG_VSYS |
> +		PALMAS_USB_VBUS_CTRL_SET_VBUS_CHRG_VSYS);
> +
> +	return 0;
> +}
> +
> +static void palmas_dt_to_pdata(struct device_node *node,
> +		struct palmas_usb_platform_data *pdata)
> +{
> +	pdata->no_control_vbus = of_property_read_bool(node,
> +					"ti,no_control_vbus");
> +	pdata->wakeup = of_property_read_bool(node, "ti,wakeup");
> +}
> +
> +static int palmas_usb_probe(struct platform_device *pdev)
> +{
> +	u32 ret;
> +	struct palmas *palmas = dev_get_drvdata(pdev->dev.parent);
> +	struct palmas_usb_platform_data	*pdata = pdev->dev.platform_data;
> +	struct device_node *node = pdev->dev.of_node;
> +	struct palmas_usb *palmas_usb;
> +	int status;
> +
> +	if (node && !pdata) {
> +		pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
> +
> +		if (!pdata)
> +			return -ENOMEM;
> +
> +		palmas_dt_to_pdata(node, pdata);
> +	}
> +
> +	if (!pdata)
> +		return -EINVAL;
> +
> +	palmas_usb = devm_kzalloc(&pdev->dev, sizeof(*palmas_usb), GFP_KERNEL);
> +	if (!palmas_usb)
> +		return -ENOMEM;
> +
> +	palmas->usb		= palmas_usb;
> +	palmas_usb->palmas	= palmas;
> +
> +	palmas_usb->dev		= &pdev->dev;
> +
> +	palmas_usb->irq1 = regmap_irq_get_virq(palmas->irq_data,
> +						PALMAS_ID_OTG_IRQ);
> +	palmas_usb->irq2 = regmap_irq_get_virq(palmas->irq_data,
> +						PALMAS_ID_IRQ);
> +	palmas_usb->irq3 = regmap_irq_get_virq(palmas->irq_data,
> +						PALMAS_VBUS_OTG_IRQ);
> +	palmas_usb->irq4 = regmap_irq_get_virq(palmas->irq_data,
> +						PALMAS_VBUS_IRQ);
> +
> +	palmas_usb->comparator.set_vbus	= palmas_set_vbus;
> +	palmas_usb->comparator.start_srp = palmas_start_srp;
> +
> +	palmas_usb_wakeup(palmas, pdata->wakeup);
> +
> +	/* init spinlock for workqueue */
> +	spin_lock_init(&palmas_usb->lock);
> +
> +	if (!pdata->no_control_vbus) {
> +		palmas_usb->vbus_reg = devm_regulator_get(&pdev->dev, "vbus");
> +		if (IS_ERR(palmas_usb->vbus_reg)) {
> +			dev_err(&pdev->dev, "vbus init failed\n");
> +			return PTR_ERR(palmas_usb->vbus_reg);
> +		}
> +	}
> +
> +	platform_set_drvdata(pdev, palmas_usb);
> +
> +	if (device_create_file(&pdev->dev, &dev_attr_vbus))
> +		dev_warn(&pdev->dev, "could not create sysfs file\n");
device_create_file() isn't needed if remove 'palmas_usb_vbus' sysfs entry.
> +
> +	palmas_usb->edev.name = "palmas_usb";
I prefer '-' instead of '_'. change from "palmas_usb" to "palmas-usb".
> +	palmas_usb->edev.supported_cable = palmas_extcon_cable;
> +	palmas_usb->edev.mutually_exclusive = mutually_exclusive;
> +
> +	ret = extcon_dev_register(&palmas_usb->edev, palmas_usb->dev);
> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to register extcon device\n");
> +		return ret;
> +	}
> +
> +	/* init spinlock for workqueue */
> +	spin_lock_init(&palmas_usb->lock);
> +
> +	INIT_WORK(&palmas_usb->set_vbus_work, palmas_set_vbus_work);
> +
> +	status = devm_request_threaded_irq(palmas_usb->dev, palmas_usb->irq2,
> +			NULL, palmas_id_wakeup_irq,
> +			IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING,
> +			"palmas_usb", palmas_usb);
> +	if (status < 0) {
> +		dev_err(&pdev->dev, "can't get IRQ %d, err %d\n",
> +					palmas_usb->irq2, status);
> +		goto fail_irq;
> +	}
> +
> +	status = devm_request_threaded_irq(palmas_usb->dev, palmas_usb->irq4,
> +			NULL, palmas_vbus_wakeup_irq,
> +			IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING,
> +			"palmas_usb", palmas_usb);
> +	if (status < 0) {
> +		dev_err(&pdev->dev, "can't get IRQ %d, err %d\n",
> +					palmas_usb->irq4, status);
> +		goto fail_irq;
> +	}
> +
> +	palmas_enable_irq(palmas_usb);
> +
> +	return 0;
> +
> +fail_irq:
> +	cancel_work_sync(&palmas_usb->set_vbus_work);
> +	device_remove_file(palmas_usb->dev, &dev_attr_vbus);
ditto.
> +
> +	return status;
> +}
> +
> +static int palmas_usb_remove(struct platform_device *pdev)
> +{
> +	struct palmas_usb *palmas_usb = platform_get_drvdata(pdev);
> +
> +	device_remove_file(palmas_usb->dev, &dev_attr_vbus);
ditto.
> +	cancel_work_sync(&palmas_usb->set_vbus_work);
> +	extcon_dev_unregister(&palmas_usb->edev);
> +
> +	return 0;
> +}
> +
> +static struct of_device_id of_palmas_match_tbl[] = {
> +	{ .compatible = "ti,palmas-usb", },
> +	{ .compatible = "ti,twl6035-usb", },
> +	{ /* end */ }
> +};
> +
> +static struct platform_driver palmas_usb_driver = {
> +	.probe = palmas_usb_probe,
> +	.remove = palmas_usb_remove,
> +	.driver = {
> +		.name = "palmas-usb",
> +		.of_match_table = of_palmas_match_tbl,
> +		.owner = THIS_MODULE,
> +	},
> +};
> +
> +module_platform_driver(palmas_usb_driver);
> +
> +MODULE_ALIAS("platform:palmas-usb");
> +MODULE_AUTHOR("Graeme Gregory <gg@...mlogic.co.uk>");
> +MODULE_DESCRIPTION("Palmas USB transceiver driver");
> +MODULE_LICENSE("GPL");
> +MODULE_DEVICE_TABLE(of, of_palmas_match_tbl);
> diff --git a/include/linux/extcon/extcon_palmas.h b/include/linux/extcon/extcon_palmas.h
> new file mode 100644
> index 0000000..a5119c9
> --- /dev/null
> +++ b/include/linux/extcon/extcon_palmas.h
> @@ -0,0 +1,26 @@
> +/*
> + * extcon_palmas.h - palmas extcon driver to detect VBUS or ID events
> + *
> + * Copyright (C) 2013 Texas Instruments Incorporated - http://www.ti.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.
> + *
> + * Author: Kishon Vijay Abraham I <kishon@ti.com>
> + *
> + * 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.
> + *
> + */
> +
> +#ifndef __EXTCON_PALMAS_H__
> +#define __EXTCON_PALMAS_H__
> +
> +#define	PALMAS_USB_STATE_DISCONNECT	0x0
> +#define	PALMAS_USB_STATE_VBUS		BIT(0)
> +#define	PALMAS_USB_STATE_ID		BIT(1)
> +
The defined variable in extcon_palmas.h is used only on extcon-palmas.c.
So, I would like to move definition from extcon_palmas.h to extcon-palmas.c
and remove extcon_palmas.h header file.
> +#endif	/* __EXTCON_PALMAS_H__ */
> diff --git a/include/linux/mfd/palmas.h b/include/linux/mfd/palmas.h
> index 8f21daf..d671679 100644
> --- a/include/linux/mfd/palmas.h
> +++ b/include/linux/mfd/palmas.h
> @@ -18,8 +18,10 @@
>  
>  #include <linux/usb/otg.h>
>  #include <linux/leds.h>
> +#include <linux/extcon.h>
>  #include <linux/regmap.h>
>  #include <linux/regulator/driver.h>
> +#include <linux/usb/phy_companion.h>
>  
>  #define PALMAS_NUM_CLIENTS		3
>  
> @@ -349,6 +351,9 @@ struct palmas_resource {
>  struct palmas_usb {
>  	struct palmas *palmas;
>  	struct device *dev;
> +	struct extcon_dev edev;
> +
> +	struct phy_companion comparator;
>  
>  	/* for vbus reporting with irqs disabled */
>  	spinlock_t lock;
> @@ -365,7 +370,8 @@ struct palmas_usb {
>  
>  	int vbus_enable;
>  
> -	u8 linkstat;
> +	int mailboxstat;
> +	int linkstat;
>  };
>  
>  #define comparator_to_palmas(x) container_of((x), struct palmas_usb, comparator)

Thanks,
Chanwoo Choi

Re: [PATCH v4] extcon: Palmas Extcon Driver

[Kishon Vijay Abraham I]

Hi,

On Tuesday 07 May 2013 11:40 AM, Chanwoo Choi wrote:
> Hi Kishon,
>
> I add some opinion about this patch.
>
> On 05/06/2013 10:17 PM, Kishon Vijay Abraham I wrote:
>> From: Graeme Gregory <gg@slimlogic.co.uk>
>>
>> This is the driver for the USB comparator built into the palmas chip. It
>> handles the various USB OTG events that can be generated by cable
>> insertion/removal.
>>
>> Signed-off-by: Graeme Gregory <gg@slimlogic.co.uk>
>> Signed-off-by: Moiz Sonasath <m-sonasath@ti.com>
>> Signed-off-by: Ruchika Kharwar <ruchika@ti.com>
>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>> [kishon@ti.com: adapted palmas usb driver to use the extcon framework]
>> Signed-off-by: Sebastien Guiriec <s-guiriec@ti.com>
>> ---
>> Changes from v3:
>> * adapted the driver to extcon framework (so moved to drivers/extcon)
>> * removed palmas_usb_(write/read) and replaced all calls with
>>    palmas_(read/write).
>> * ignored a checkpatch warning in the line
>> 	static const char *palmas_extcon_cable[] = {
>>    as it seemed to be incorrect?
>> * removed all references to OMAP in this driver.
>> * couldn't test this driver with mainline as omap5 panda is not booting
>>    with mainline.
>> * A comment to change to platform_get_irq from regmap is not done as I felt
>>    the data should come from regmap in this case. Graeme?
>> Changes from v2:
>> * Moved the driver to drivers/usb/phy/
>> * Added a bit more explanation in Kconfig
>>   .../devicetree/bindings/extcon/extcon-twl.txt      |   17 +
>>   drivers/extcon/Kconfig                             |    7 +
>>   drivers/extcon/Makefile                            |    1 +
>>   drivers/extcon/extcon-palmas.c                     |  389 ++++++++++++++++++++
>>   include/linux/extcon/extcon_palmas.h               |   26 ++
>>   include/linux/mfd/palmas.h                         |    8 +-
>>   6 files changed, 447 insertions(+), 1 deletion(-)
>>   create mode 100644 Documentation/devicetree/bindings/extcon/extcon-twl.txt
>>   create mode 100644 drivers/extcon/extcon-palmas.c
>>   create mode 100644 include/linux/extcon/extcon_palmas.h
>>
>> diff --git a/Documentation/devicetree/bindings/extcon/extcon-twl.txt b/Documentation/devicetree/bindings/extcon/extcon-twl.txt
>> new file mode 100644
>> index 0000000..a7f6527
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/extcon/extcon-twl.txt
>> @@ -0,0 +1,17 @@
>> +EXTCON FOR TWL CHIPS
>> +
>> +PALMAS USB COMPARATOR
>> +Required Properties:
>> + - compatible : Should be "ti,palmas-usb" or "ti,twl6035-usb"
>> + - vbus-supply : phandle to the regulator device tree node.
>> +
>> +Optional Properties:
>> + - ti,wakeup : To enable the wakeup comparator in probe
>> + - ti,no_control_vbus: if the platform wishes its own vbus control
>> +
>> +palmas-usb {
>> +       compatible = "ti,twl6035-usb", "ti,palmas-usb";
>> +       vbus-supply = <&smps10_reg>;
>> +       ti,wakeup;
>> +};
>> +
>> diff --git a/drivers/extcon/Kconfig b/drivers/extcon/Kconfig
>> index 5168a13..c881899 100644
>> --- a/drivers/extcon/Kconfig
>> +++ b/drivers/extcon/Kconfig
>> @@ -53,4 +53,11 @@ config EXTCON_ARIZONA
>>   	  with Wolfson Arizona devices. These are audio CODECs with
>>   	  advanced audio accessory detection support.
>>
>> +config EXTCON_PALMAS
>> +	tristate "Palmas USB EXTCON support"
>> +	depends on MFD_PALMAS
>> +	help
>> +	  Say Y here to enable support for USB peripheral and USB host
>> +	  detection by palmas usb.
>> +
>>   endif # MULTISTATE_SWITCH
>> diff --git a/drivers/extcon/Makefile b/drivers/extcon/Makefile
>> index f98a3c4..540e2c3 100644
>> --- a/drivers/extcon/Makefile
>> +++ b/drivers/extcon/Makefile
>> @@ -8,3 +8,4 @@ obj-$(CONFIG_EXTCON_ADC_JACK)	+= extcon-adc-jack.o
>>   obj-$(CONFIG_EXTCON_MAX77693)	+= extcon-max77693.o
>>   obj-$(CONFIG_EXTCON_MAX8997)	+= extcon-max8997.o
>>   obj-$(CONFIG_EXTCON_ARIZONA)	+= extcon-arizona.o
>> +obj-$(CONFIG_EXTCON_PALMAS)	+= extcon-palmas.o
>> diff --git a/drivers/extcon/extcon-palmas.c b/drivers/extcon/extcon-palmas.c
>> new file mode 100644
>> index 0000000..3ef042f
>> --- /dev/null
>> +++ b/drivers/extcon/extcon-palmas.c
>> @@ -0,0 +1,389 @@
>> +/*
>> + * Palmas USB transceiver driver
>> + *
>> + * Copyright (C) 2013 Texas Instruments Incorporated - http://www.ti.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.
>> + *
>> + * Author: Graeme Gregory <gg@...mlogic.co.uk>
>> + * Author: Kishon Vijay Abraham I <kishon@...com>
>> + *
>> + * Based on twl6030_usb.c
>> + *
>> + * Author: Hema HK <hemahk@...com>
>> + *
>> + * 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/init.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/io.h>
>> +#include <linux/usb/phy_companion.h>
>> +#include <linux/regulator/consumer.h>
>> +#include <linux/err.h>
>> +#include <linux/notifier.h>
>> +#include <linux/slab.h>
>> +#include <linux/delay.h>
>> +#include <linux/mfd/palmas.h>
>> +#include <linux/of.h>
>> +#include <linux/of_platform.h>
>> +#include <linux/extcon/extcon_palmas.h>
>> +
> Remove unnecessary header file. When I remove following header file,
> I completed kernel build without any problem.
>
> linux/init.h
> linux/io.h
> linux/regulator/consumer.h
> linux/notifier.h
> linux/slab.h
> linux/delay.h

hmm.. ok.
>> +static const char *palmas_extcon_cable[] = {
>> +	[0] = "USB",
>> +	[1] = "USB-HOST",
>> +	NULL,
>> +};
>> +
>> +static const int mutually_exclusive[] = {0x3, 0x0};
>> +
>> +static void palmas_usb_wakeup(struct palmas *palmas, int enable)
>> +{
>> +	if (enable)
>> +		palmas_write(palmas, PALMAS_USB_OTG_BASE, PALMAS_USB_WAKEUP,
>> +			PALMAS_USB_WAKEUP_ID_WK_UP_COMP);
>> +	else
>> +		palmas_write(palmas, PALMAS_USB_OTG_BASE, PALMAS_USB_WAKEUP, 0);
>> +}
>> +
>> +static ssize_t palmas_usb_vbus_show(struct device *dev,
>> +			struct device_attribute *attr, char *buf)
>> +{
>> +	unsigned long		flags;
>> +	int			ret = -EINVAL;
>> +	struct palmas_usb	*palmas_usb = dev_get_drvdata(dev);
>> +
>> +	spin_lock_irqsave(&palmas_usb->lock, flags);
>> +
>> +	switch (palmas_usb->linkstat) {
>> +	case PALMAS_USB_STATE_VBUS:
>> +	       ret = snprintf(buf, PAGE_SIZE, "vbus\n");
>> +	       break;
>> +	case PALMAS_USB_STATE_ID:
>> +	       ret = snprintf(buf, PAGE_SIZE, "id\n");
>> +	       break;
>> +	case PALMAS_USB_STATE_DISCONNECT:
>> +	       ret = snprintf(buf, PAGE_SIZE, "none\n");
>> +	       break;
>> +	default:
>> +	       ret = snprintf(buf, PAGE_SIZE, "UNKNOWN\n");
>> +	}
>> +	spin_unlock_irqrestore(&palmas_usb->lock, flags);
>> +
>> +	return ret;
>> +}
>> +static DEVICE_ATTR(vbus, 0444, palmas_usb_vbus_show, NULL);
> I think that 'palmas_usb_vbus' device attribute isn't standard node
> of extcon framework. If you want to check USB/USB-HOST state
> on user-space, user process should use following standard node
> of extcon instead of 'palmas_usb_vbus' node.

Ok. Makes sense.
>
> - User can get the state of USB/USB-HOST through following node:
> /sys/class/extcon/palmas_usb/cable.0/state
>      if state is 1, PALMAS_USB_STATE_VBUS
>      if state is 0, PALMAS_USB_STATE_DISCONNECT
>
> /sys/class/extcon/palmas_usb/cable.1/state
>      if state is 1, PALMAS_USB_STATE_ID
>      if state is 0, PALMAS_USB_STATE_DISCONNECT
>
> Certainly, extcon driver have to provide specific information of external connector through
> standard sysfs entry.
>> +
>> +static irqreturn_t palmas_vbus_wakeup_irq(int irq, void *_palmas_usb)
>> +{
>> +	int ret;
>> +	struct palmas_usb *palmas_usb = _palmas_usb;
>> +	unsigned int vbus_line_state;
>> +
>> +	palmas_read(palmas_usb->palmas, PALMAS_INTERRUPT_BASE,
>> +		PALMAS_INT3_LINE_STATE, &vbus_line_state);
>> +
>> +	if (vbus_line_state & PALMAS_INT3_LINE_STATE_VBUS) {
>> +		if (palmas_usb->linkstat != PALMAS_USB_STATE_VBUS) {
>> +			if (palmas_usb->vbus_reg) {
>> +				ret = regulator_enable(palmas_usb->vbus_reg);
>> +				if (ret) {
>> +					dev_dbg(palmas_usb->dev,
>> +						"regulator enable failed\n");
>> +					goto ret0;
>> +				}
>> +			}
>> +			palmas_usb->linkstat = PALMAS_USB_STATE_VBUS;
>> +			extcon_set_cable_state(&palmas_usb->edev, "USB", true);
>> +		} else {
>> +			dev_dbg(palmas_usb->dev,
>> +				"Spurious connect event detected\n");
>> +		}
>> +	} else if (!(vbus_line_state & PALMAS_INT3_LINE_STATE_VBUS)) {
>> +		if (palmas_usb->linkstat == PALMAS_USB_STATE_VBUS) {
>> +			if (palmas_usb->vbus_reg)
>> +				regulator_disable(palmas_usb->vbus_reg);
>> +			palmas_usb->linkstat = PALMAS_USB_STATE_DISCONNECT;
>> +			extcon_set_cable_state(&palmas_usb->edev, "USB", false);
>> +		} else {
>> +			dev_dbg(palmas_usb->dev,
>> +				"Spurious disconnect event detected\n");
>> +		}
>> +	}
>> +
>> +ret0:
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +static irqreturn_t palmas_id_wakeup_irq(int irq, void *_palmas_usb)
>> +{
>> +	int			ret;
>> +	unsigned int		set;
>> +	struct palmas_usb	*palmas_usb = _palmas_usb;
>> +
>> +	palmas_read(palmas_usb->palmas, PALMAS_USB_OTG_BASE,
>> +		PALMAS_USB_ID_INT_LATCH_SET, &set);
>> +
>> +	if (set & PALMAS_USB_ID_INT_SRC_ID_GND) {
>> +		if (palmas_usb->vbus_reg) {
>> +			ret = regulator_enable(palmas_usb->vbus_reg);
>> +			if (ret) {
>> +				dev_dbg(palmas_usb->dev,
>> +					"regulator enable failed\n");
>> +				goto ret0;
>> +			}
>> +		}
>> +		palmas_write(palmas_usb->palmas, PALMAS_USB_OTG_BASE,
>> +			PALMAS_USB_ID_INT_EN_HI_SET,
>> +			PALMAS_USB_ID_INT_EN_HI_SET_ID_FLOAT);
>> +		palmas_write(palmas_usb->palmas, PALMAS_USB_OTG_BASE,
>> +			PALMAS_USB_ID_INT_EN_HI_CLR,
>> +			PALMAS_USB_ID_INT_EN_HI_CLR_ID_GND);
>> +		palmas_usb->linkstat = PALMAS_USB_STATE_ID;
>> +		extcon_set_cable_state(&palmas_usb->edev, "USB-HOST", true);
>> +	} else if (set & PALMAS_USB_ID_INT_SRC_ID_FLOAT) {
>> +		palmas_write(palmas_usb->palmas, PALMAS_USB_OTG_BASE,
>> +			PALMAS_USB_ID_INT_EN_HI_SET,
>> +			PALMAS_USB_ID_INT_EN_HI_SET_ID_GND);
>> +		palmas_write(palmas_usb->palmas, PALMAS_USB_OTG_BASE,
>> +			PALMAS_USB_ID_INT_EN_HI_CLR,
>> +			PALMAS_USB_ID_INT_EN_HI_CLR_ID_FLOAT);
>> +		if (palmas_usb->vbus_reg)
>> +			regulator_disable(palmas_usb->vbus_reg);
>> +		palmas_usb->linkstat = PALMAS_USB_STATE_DISCONNECT;
>> +		extcon_set_cable_state(&palmas_usb->edev, "USB-HOST", false);
>> +	}
>> +
>> +ret0:
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +static void palmas_enable_irq(struct palmas_usb *palmas_usb)
>> +{
>> +	palmas_write(palmas_usb->palmas, PALMAS_USB_OTG_BASE,
>> +		PALMAS_USB_VBUS_CTRL_SET,
>> +		PALMAS_USB_VBUS_CTRL_SET_VBUS_ACT_COMP);
>> +
>> +	palmas_write(palmas_usb->palmas, PALMAS_USB_OTG_BASE,
>> +		PALMAS_USB_ID_CTRL_SET, PALMAS_USB_ID_CTRL_SET_ID_ACT_COMP);
>> +
>> +	palmas_write(palmas_usb->palmas, PALMAS_USB_OTG_BASE,
>> +		PALMAS_USB_ID_INT_EN_HI_SET,
>> +		PALMAS_USB_ID_INT_EN_HI_SET_ID_GND);
>> +
>> +	palmas_vbus_wakeup_irq(palmas_usb->irq4, palmas_usb);
>> +	palmas_id_wakeup_irq(palmas_usb->irq2, palmas_usb);
>> +}
>> +
>> +static void palmas_set_vbus_work(struct work_struct *data)
>> +{
>> +	int ret;
>> +	struct palmas_usb *palmas_usb = container_of(data, struct palmas_usb,
>> +								set_vbus_work);
>> +
>> +	if (IS_ERR_OR_NULL(palmas_usb->vbus_reg)) {
>> +		dev_err(palmas_usb->dev, "invalid regulator\n");
>> +		return;
>> +	}
>> +
>> +	/*
>> +	 * Start driving VBUS. Set OPA_MODE bit in CHARGERUSB_CTRL1
>> +	 * register. This enables boost mode.
>> +	 */
>> +
>> +	if (palmas_usb->vbus_enable) {
>> +		ret = regulator_enable(palmas_usb->vbus_reg);
>> +		if (ret)
>> +			dev_dbg(palmas_usb->dev, "regulator enable failed\n");
>> +	} else {
>> +		regulator_disable(palmas_usb->vbus_reg);
>> +	}
>> +}
>> +
>> +static int palmas_set_vbus(struct phy_companion *comparator, bool enabled)
>> +{
>> +	struct palmas_usb *palmas_usb = comparator_to_palmas(comparator);
>> +
>> +	palmas_usb->vbus_enable = enabled;
>> +	schedule_work(&palmas_usb->set_vbus_work);
>> +
>> +	return 0;
>> +}
>> +
>> +static int palmas_start_srp(struct phy_companion *comparator)
>> +{
>> +	struct palmas_usb *palmas_usb = comparator_to_palmas(comparator);
>> +
>> +	palmas_write(palmas_usb->palmas, PALMAS_USB_OTG_BASE,
>> +		PALMAS_USB_VBUS_CTRL_SET, PALMAS_USB_VBUS_CTRL_SET_VBUS_DISCHRG
>> +		| PALMAS_USB_VBUS_CTRL_SET_VBUS_IADP_SINK);
>> +	palmas_write(palmas_usb->palmas, PALMAS_USB_OTG_BASE,
>> +		PALMAS_USB_VBUS_CTRL_SET,
>> +		PALMAS_USB_VBUS_CTRL_SET_VBUS_CHRG_VSYS |
>> +		PALMAS_USB_VBUS_CTRL_SET_VBUS_IADP_SINK);
>> +
>> +	mdelay(100);
>> +
>> +	palmas_write(palmas_usb->palmas, PALMAS_USB_OTG_BASE,
>> +		PALMAS_USB_VBUS_CTRL_CLR,
>> +		PALMAS_USB_VBUS_CTRL_SET_VBUS_CHRG_VSYS |
>> +		PALMAS_USB_VBUS_CTRL_SET_VBUS_CHRG_VSYS);
>> +
>> +	return 0;
>> +}
>> +
>> +static void palmas_dt_to_pdata(struct device_node *node,
>> +		struct palmas_usb_platform_data *pdata)
>> +{
>> +	pdata->no_control_vbus = of_property_read_bool(node,
>> +					"ti,no_control_vbus");
>> +	pdata->wakeup = of_property_read_bool(node, "ti,wakeup");
>> +}
>> +
>> +static int palmas_usb_probe(struct platform_device *pdev)
>> +{
>> +	u32 ret;
>> +	struct palmas *palmas = dev_get_drvdata(pdev->dev.parent);
>> +	struct palmas_usb_platform_data	*pdata = pdev->dev.platform_data;
>> +	struct device_node *node = pdev->dev.of_node;
>> +	struct palmas_usb *palmas_usb;
>> +	int status;
>> +
>> +	if (node && !pdata) {
>> +		pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
>> +
>> +		if (!pdata)
>> +			return -ENOMEM;
>> +
>> +		palmas_dt_to_pdata(node, pdata);
>> +	}
>> +
>> +	if (!pdata)
>> +		return -EINVAL;
>> +
>> +	palmas_usb = devm_kzalloc(&pdev->dev, sizeof(*palmas_usb), GFP_KERNEL);
>> +	if (!palmas_usb)
>> +		return -ENOMEM;
>> +
>> +	palmas->usb		= palmas_usb;
>> +	palmas_usb->palmas	= palmas;
>> +
>> +	palmas_usb->dev		= &pdev->dev;
>> +
>> +	palmas_usb->irq1 = regmap_irq_get_virq(palmas->irq_data,
>> +						PALMAS_ID_OTG_IRQ);
>> +	palmas_usb->irq2 = regmap_irq_get_virq(palmas->irq_data,
>> +						PALMAS_ID_IRQ);
>> +	palmas_usb->irq3 = regmap_irq_get_virq(palmas->irq_data,
>> +						PALMAS_VBUS_OTG_IRQ);
>> +	palmas_usb->irq4 = regmap_irq_get_virq(palmas->irq_data,
>> +						PALMAS_VBUS_IRQ);
>> +
>> +	palmas_usb->comparator.set_vbus	= palmas_set_vbus;
>> +	palmas_usb->comparator.start_srp = palmas_start_srp;
>> +
>> +	palmas_usb_wakeup(palmas, pdata->wakeup);
>> +
>> +	/* init spinlock for workqueue */
>> +	spin_lock_init(&palmas_usb->lock);
>> +
>> +	if (!pdata->no_control_vbus) {
>> +		palmas_usb->vbus_reg = devm_regulator_get(&pdev->dev, "vbus");
>> +		if (IS_ERR(palmas_usb->vbus_reg)) {
>> +			dev_err(&pdev->dev, "vbus init failed\n");
>> +			return PTR_ERR(palmas_usb->vbus_reg);
>> +		}
>> +	}
>> +
>> +	platform_set_drvdata(pdev, palmas_usb);
>> +
>> +	if (device_create_file(&pdev->dev, &dev_attr_vbus))
>> +		dev_warn(&pdev->dev, "could not create sysfs file\n");
> device_create_file() isn't needed if remove 'palmas_usb_vbus' sysfs entry.

right.

>> +
>> +	palmas_usb->edev.name = "palmas_usb";
> I prefer '-' instead of '_'. change from "palmas_usb" to "palmas-usb".
>> +	palmas_usb->edev.supported_cable = palmas_extcon_cable;
>> +	palmas_usb->edev.mutually_exclusive = mutually_exclusive;
>> +
>> +	ret = extcon_dev_register(&palmas_usb->edev, palmas_usb->dev);
>> +	if (ret) {
>> +		dev_err(&pdev->dev, "failed to register extcon device\n");
>> +		return ret;
>> +	}
>> +
>> +	/* init spinlock for workqueue */
>> +	spin_lock_init(&palmas_usb->lock);
>> +
>> +	INIT_WORK(&palmas_usb->set_vbus_work, palmas_set_vbus_work);
>> +
>> +	status = devm_request_threaded_irq(palmas_usb->dev, palmas_usb->irq2,
>> +			NULL, palmas_id_wakeup_irq,
>> +			IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING,
>> +			"palmas_usb", palmas_usb);
>> +	if (status < 0) {
>> +		dev_err(&pdev->dev, "can't get IRQ %d, err %d\n",
>> +					palmas_usb->irq2, status);
>> +		goto fail_irq;
>> +	}
>> +
>> +	status = devm_request_threaded_irq(palmas_usb->dev, palmas_usb->irq4,
>> +			NULL, palmas_vbus_wakeup_irq,
>> +			IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING,
>> +			"palmas_usb", palmas_usb);
>> +	if (status < 0) {
>> +		dev_err(&pdev->dev, "can't get IRQ %d, err %d\n",
>> +					palmas_usb->irq4, status);
>> +		goto fail_irq;
>> +	}
>> +
>> +	palmas_enable_irq(palmas_usb);
>> +
>> +	return 0;
>> +
>> +fail_irq:
>> +	cancel_work_sync(&palmas_usb->set_vbus_work);
>> +	device_remove_file(palmas_usb->dev, &dev_attr_vbus);
> ditto.
>> +
>> +	return status;
>> +}
>> +
>> +static int palmas_usb_remove(struct platform_device *pdev)
>> +{
>> +	struct palmas_usb *palmas_usb = platform_get_drvdata(pdev);
>> +
>> +	device_remove_file(palmas_usb->dev, &dev_attr_vbus);
> ditto.
>> +	cancel_work_sync(&palmas_usb->set_vbus_work);
>> +	extcon_dev_unregister(&palmas_usb->edev);
>> +
>> +	return 0;
>> +}
>> +
>> +static struct of_device_id of_palmas_match_tbl[] = {
>> +	{ .compatible = "ti,palmas-usb", },
>> +	{ .compatible = "ti,twl6035-usb", },
>> +	{ /* end */ }
>> +};
>> +
>> +static struct platform_driver palmas_usb_driver = {
>> +	.probe = palmas_usb_probe,
>> +	.remove = palmas_usb_remove,
>> +	.driver = {
>> +		.name = "palmas-usb",
>> +		.of_match_table = of_palmas_match_tbl,
>> +		.owner = THIS_MODULE,
>> +	},
>> +};
>> +
>> +module_platform_driver(palmas_usb_driver);
>> +
>> +MODULE_ALIAS("platform:palmas-usb");
>> +MODULE_AUTHOR("Graeme Gregory <gg@...mlogic.co.uk>");
>> +MODULE_DESCRIPTION("Palmas USB transceiver driver");
>> +MODULE_LICENSE("GPL");
>> +MODULE_DEVICE_TABLE(of, of_palmas_match_tbl);
>> diff --git a/include/linux/extcon/extcon_palmas.h b/include/linux/extcon/extcon_palmas.h
>> new file mode 100644
>> index 0000000..a5119c9
>> --- /dev/null
>> +++ b/include/linux/extcon/extcon_palmas.h
>> @@ -0,0 +1,26 @@
>> +/*
>> + * extcon_palmas.h - palmas extcon driver to detect VBUS or ID events
>> + *
>> + * Copyright (C) 2013 Texas Instruments Incorporated - http://www.ti.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.
>> + *
>> + * Author: Kishon Vijay Abraham I <kishon@ti.com>
>> + *
>> + * 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.
>> + *
>> + */
>> +
>> +#ifndef __EXTCON_PALMAS_H__
>> +#define __EXTCON_PALMAS_H__
>> +
>> +#define	PALMAS_USB_STATE_DISCONNECT	0x0
>> +#define	PALMAS_USB_STATE_VBUS		BIT(0)
>> +#define	PALMAS_USB_STATE_ID		BIT(1)
>> +
> The defined variable in extcon_palmas.h is used only on extcon-palmas.c.
> So, I would like to move definition from extcon_palmas.h to extcon-palmas.c
> and remove extcon_palmas.h header file.

Actually it has to be used in dwc3-omap.c (that was in a different patch).

Thanks
Kishon

Re: [PATCH v4] extcon: Palmas Extcon Driver

[Chanwoo Choi]

On 05/07/2013 03:25 PM, Kishon Vijay Abraham I wrote:
> Hi,
>
> On Tuesday 07 May 2013 11:40 AM, Chanwoo Choi wrote:
>> Hi Kishon,
>>
>> I add some opinion about this patch.
>>
>> On 05/06/2013 10:17 PM, Kishon Vijay Abraham I wrote:
>>> From: Graeme Gregory <gg@slimlogic.co.uk>
>>>
>>> This is the driver for the USB comparator built into the palmas chip. It
>>> handles the various USB OTG events that can be generated by cable
>>> insertion/removal.
>>>
>>> Signed-off-by: Graeme Gregory <gg@slimlogic.co.uk>
>>> Signed-off-by: Moiz Sonasath <m-sonasath@ti.com>
>>> Signed-off-by: Ruchika Kharwar <ruchika@ti.com>
>>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>>> [kishon@ti.com: adapted palmas usb driver to use the extcon framework]
>>> Signed-off-by: Sebastien Guiriec <s-guiriec@ti.com>
>>> ---
>>> Changes from v3:
>>> * adapted the driver to extcon framework (so moved to drivers/extcon)
>>> * removed palmas_usb_(write/read) and replaced all calls with
>>>    palmas_(read/write).
>>> * ignored a checkpatch warning in the line
>>>     static const char *palmas_extcon_cable[] = {
>>>    as it seemed to be incorrect?
>>> * removed all references to OMAP in this driver.
>>> * couldn't test this driver with mainline as omap5 panda is not booting
>>>    with mainline.
>>> * A comment to change to platform_get_irq from regmap is not done as I felt
>>>    the data should come from regmap in this case. Graeme?
>>> Changes from v2:
>>> * Moved the driver to drivers/usb/phy/
>>> * Added a bit more explanation in Kconfig
>>>   .../devicetree/bindings/extcon/extcon-twl.txt      |   17 +
>>>   drivers/extcon/Kconfig                             |    7 +
>>>   drivers/extcon/Makefile                            |    1 +
>>>   drivers/extcon/extcon-palmas.c                     |  389 ++++++++++++++++++++
>>>   include/linux/extcon/extcon_palmas.h               |   26 ++
>>>   include/linux/mfd/palmas.h                         |    8 +-
>>>   6 files changed, 447 insertions(+), 1 deletion(-)
>>>   create mode 100644 Documentation/devicetree/bindings/extcon/extcon-twl.txt
>>>   create mode 100644 drivers/extcon/extcon-palmas.c
>>>   create mode 100644 include/linux/extcon/extcon_palmas.h
>>>
>>> diff --git a/Documentation/devicetree/bindings/extcon/extcon-twl.txt b/Documentation/devicetree/bindings/extcon/extcon-twl.txt
>>> new file mode 100644
>>> index 0000000..a7f6527
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/extcon/extcon-twl.txt
>>> @@ -0,0 +1,17 @@
>>> +EXTCON FOR TWL CHIPS
>>> +
>>> +PALMAS USB COMPARATOR
>>> +Required Properties:
>>> + - compatible : Should be "ti,palmas-usb" or "ti,twl6035-usb"
>>> + - vbus-supply : phandle to the regulator device tree node.
>>> +
>>> +Optional Properties:
>>> + - ti,wakeup : To enable the wakeup comparator in probe
>>> + - ti,no_control_vbus: if the platform wishes its own vbus control
>>> +
>>> +palmas-usb {
>>> +       compatible = "ti,twl6035-usb", "ti,palmas-usb";
>>> +       vbus-supply = <&smps10_reg>;
>>> +       ti,wakeup;
>>> +};
>>> +
>>> diff --git a/drivers/extcon/Kconfig b/drivers/extcon/Kconfig
>>> index 5168a13..c881899 100644
>>> --- a/drivers/extcon/Kconfig
>>> +++ b/drivers/extcon/Kconfig
>>> @@ -53,4 +53,11 @@ config EXTCON_ARIZONA
>>>         with Wolfson Arizona devices. These are audio CODECs with
>>>         advanced audio accessory detection support.
>>>
>>> +config EXTCON_PALMAS
>>> +    tristate "Palmas USB EXTCON support"
>>> +    depends on MFD_PALMAS
>>> +    help
>>> +      Say Y here to enable support for USB peripheral and USB host
>>> +      detection by palmas usb.
>>> +
>>>   endif # MULTISTATE_SWITCH
>>> diff --git a/drivers/extcon/Makefile b/drivers/extcon/Makefile
>>> index f98a3c4..540e2c3 100644
>>> --- a/drivers/extcon/Makefile
>>> +++ b/drivers/extcon/Makefile
>>> @@ -8,3 +8,4 @@ obj-$(CONFIG_EXTCON_ADC_JACK)    += extcon-adc-jack.o
>>>   obj-$(CONFIG_EXTCON_MAX77693)    += extcon-max77693.o
>>>   obj-$(CONFIG_EXTCON_MAX8997)    += extcon-max8997.o
>>>   obj-$(CONFIG_EXTCON_ARIZONA)    += extcon-arizona.o
>>> +obj-$(CONFIG_EXTCON_PALMAS)    += extcon-palmas.o
>>> diff --git a/drivers/extcon/extcon-palmas.c b/drivers/extcon/extcon-palmas.c
>>> new file mode 100644
>>> index 0000000..3ef042f
>>> --- /dev/null
>>> +++ b/drivers/extcon/extcon-palmas.c
>>> @@ -0,0 +1,389 @@
>>> +/*
>>> + * Palmas USB transceiver driver
>>> + *
>>> + * Copyright (C) 2013 Texas Instruments Incorporated - http://www.ti.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.
>>> + *
>>> + * Author: Graeme Gregory <gg@...mlogic.co.uk>
>>> + * Author: Kishon Vijay Abraham I <kishon@...com>
>>> + *
>>> + * Based on twl6030_usb.c
>>> + *
>>> + * Author: Hema HK <hemahk@...com>
>>> + *
>>> + * 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/init.h>
>>> +#include <linux/interrupt.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/io.h>
>>> +#include <linux/usb/phy_companion.h>
>>> +#include <linux/regulator/consumer.h>
>>> +#include <linux/err.h>
>>> +#include <linux/notifier.h>
>>> +#include <linux/slab.h>
>>> +#include <linux/delay.h>
>>> +#include <linux/mfd/palmas.h>
>>> +#include <linux/of.h>
>>> +#include <linux/of_platform.h>
>>> +#include <linux/extcon/extcon_palmas.h>
>>> +
>> Remove unnecessary header file. When I remove following header file,
>> I completed kernel build without any problem.
>>
>> linux/init.h
>> linux/io.h
>> linux/regulator/consumer.h
>> linux/notifier.h
>> linux/slab.h
>> linux/delay.h
>
> hmm.. ok.
>>> +static const char *palmas_extcon_cable[] = {
>>> +    [0] = "USB",
>>> +    [1] = "USB-HOST",
>>> +    NULL,
>>> +};
>>> +
>>> +static const int mutually_exclusive[] = {0x3, 0x0};
>>> +
>>> +static void palmas_usb_wakeup(struct palmas *palmas, int enable)
>>> +{
>>> +    if (enable)
>>> +        palmas_write(palmas, PALMAS_USB_OTG_BASE, PALMAS_USB_WAKEUP,
>>> +            PALMAS_USB_WAKEUP_ID_WK_UP_COMP);
>>> +    else
>>> +        palmas_write(palmas, PALMAS_USB_OTG_BASE, PALMAS_USB_WAKEUP, 0);
>>> +}
>>> +
>>> +static ssize_t palmas_usb_vbus_show(struct device *dev,
>>> +            struct device_attribute *attr, char *buf)
>>> +{
>>> +    unsigned long        flags;
>>> +    int            ret = -EINVAL;
>>> +    struct palmas_usb    *palmas_usb = dev_get_drvdata(dev);
>>> +
>>> +    spin_lock_irqsave(&palmas_usb->lock, flags);
>>> +
>>> +    switch (palmas_usb->linkstat) {
>>> +    case PALMAS_USB_STATE_VBUS:
>>> +           ret = snprintf(buf, PAGE_SIZE, "vbus\n");
>>> +           break;
>>> +    case PALMAS_USB_STATE_ID:
>>> +           ret = snprintf(buf, PAGE_SIZE, "id\n");
>>> +           break;
>>> +    case PALMAS_USB_STATE_DISCONNECT:
>>> +           ret = snprintf(buf, PAGE_SIZE, "none\n");
>>> +           break;
>>> +    default:
>>> +           ret = snprintf(buf, PAGE_SIZE, "UNKNOWN\n");
>>> +    }
>>> +    spin_unlock_irqrestore(&palmas_usb->lock, flags);
>>> +
>>> +    return ret;
>>> +}
>>> +static DEVICE_ATTR(vbus, 0444, palmas_usb_vbus_show, NULL);
>> I think that 'palmas_usb_vbus' device attribute isn't standard node
>> of extcon framework. If you want to check USB/USB-HOST state
>> on user-space, user process should use following standard node
>> of extcon instead of 'palmas_usb_vbus' node.
>
> Ok. Makes sense.
>>
>> - User can get the state of USB/USB-HOST through following node:
>> /sys/class/extcon/palmas_usb/cable.0/state
>>      if state is 1, PALMAS_USB_STATE_VBUS
>>      if state is 0, PALMAS_USB_STATE_DISCONNECT
>>
>> /sys/class/extcon/palmas_usb/cable.1/state
>>      if state is 1, PALMAS_USB_STATE_ID
>>      if state is 0, PALMAS_USB_STATE_DISCONNECT
>>
>> Certainly, extcon driver have to provide specific information of external connector through
>> standard sysfs entry.
>>> +
>>> +static irqreturn_t palmas_vbus_wakeup_irq(int irq, void *_palmas_usb)
>>> +{
>>> +    int ret;
>>> +    struct palmas_usb *palmas_usb = _palmas_usb;
>>> +    unsigned int vbus_line_state;
>>> +
>>> +    palmas_read(palmas_usb->palmas, PALMAS_INTERRUPT_BASE,
>>> +        PALMAS_INT3_LINE_STATE, &vbus_line_state);
>>> +
>>> +    if (vbus_line_state & PALMAS_INT3_LINE_STATE_VBUS) {
>>> +        if (palmas_usb->linkstat != PALMAS_USB_STATE_VBUS) {
>>> +            if (palmas_usb->vbus_reg) {
>>> +                ret = regulator_enable(palmas_usb->vbus_reg);
>>> +                if (ret) {
>>> +                    dev_dbg(palmas_usb->dev,
>>> +                        "regulator enable failed\n");
>>> +                    goto ret0;
>>> +                }
>>> +            }
>>> +            palmas_usb->linkstat = PALMAS_USB_STATE_VBUS;
>>> +            extcon_set_cable_state(&palmas_usb->edev, "USB", true);
>>> +        } else {
>>> +            dev_dbg(palmas_usb->dev,
>>> +                "Spurious connect event detected\n");
>>> +        }
>>> +    } else if (!(vbus_line_state & PALMAS_INT3_LINE_STATE_VBUS)) {
>>> +        if (palmas_usb->linkstat == PALMAS_USB_STATE_VBUS) {
>>> +            if (palmas_usb->vbus_reg)
>>> +                regulator_disable(palmas_usb->vbus_reg);
>>> +            palmas_usb->linkstat = PALMAS_USB_STATE_DISCONNECT;
>>> +            extcon_set_cable_state(&palmas_usb->edev, "USB", false);
>>> +        } else {
>>> +            dev_dbg(palmas_usb->dev,
>>> +                "Spurious disconnect event detected\n");
>>> +        }
>>> +    }
>>> +
>>> +ret0:
>>> +    return IRQ_HANDLED;
>>> +}
>>> +
>>> +static irqreturn_t palmas_id_wakeup_irq(int irq, void *_palmas_usb)
>>> +{
>>> +    int            ret;
>>> +    unsigned int        set;
>>> +    struct palmas_usb    *palmas_usb = _palmas_usb;
>>> +
>>> +    palmas_read(palmas_usb->palmas, PALMAS_USB_OTG_BASE,
>>> +        PALMAS_USB_ID_INT_LATCH_SET, &set);
>>> +
>>> +    if (set & PALMAS_USB_ID_INT_SRC_ID_GND) {
>>> +        if (palmas_usb->vbus_reg) {
>>> +            ret = regulator_enable(palmas_usb->vbus_reg);
>>> +            if (ret) {
>>> +                dev_dbg(palmas_usb->dev,
>>> +                    "regulator enable failed\n");
>>> +                goto ret0;
>>> +            }
>>> +        }
>>> +        palmas_write(palmas_usb->palmas, PALMAS_USB_OTG_BASE,
>>> +            PALMAS_USB_ID_INT_EN_HI_SET,
>>> +            PALMAS_USB_ID_INT_EN_HI_SET_ID_FLOAT);
>>> +        palmas_write(palmas_usb->palmas, PALMAS_USB_OTG_BASE,
>>> +            PALMAS_USB_ID_INT_EN_HI_CLR,
>>> +            PALMAS_USB_ID_INT_EN_HI_CLR_ID_GND);
>>> +        palmas_usb->linkstat = PALMAS_USB_STATE_ID;
>>> +        extcon_set_cable_state(&palmas_usb->edev, "USB-HOST", true);
>>> +    } else if (set & PALMAS_USB_ID_INT_SRC_ID_FLOAT) {
>>> +        palmas_write(palmas_usb->palmas, PALMAS_USB_OTG_BASE,
>>> +            PALMAS_USB_ID_INT_EN_HI_SET,
>>> +            PALMAS_USB_ID_INT_EN_HI_SET_ID_GND);
>>> +        palmas_write(palmas_usb->palmas, PALMAS_USB_OTG_BASE,
>>> +            PALMAS_USB_ID_INT_EN_HI_CLR,
>>> +            PALMAS_USB_ID_INT_EN_HI_CLR_ID_FLOAT);
>>> +        if (palmas_usb->vbus_reg)
>>> +            regulator_disable(palmas_usb->vbus_reg);
>>> +        palmas_usb->linkstat = PALMAS_USB_STATE_DISCONNECT;
>>> +        extcon_set_cable_state(&palmas_usb->edev, "USB-HOST", false);
>>> +    }
>>> +
>>> +ret0:
>>> +    return IRQ_HANDLED;
>>> +}
>>> +
>>> +static void palmas_enable_irq(struct palmas_usb *palmas_usb)
>>> +{
>>> +    palmas_write(palmas_usb->palmas, PALMAS_USB_OTG_BASE,
>>> +        PALMAS_USB_VBUS_CTRL_SET,
>>> +        PALMAS_USB_VBUS_CTRL_SET_VBUS_ACT_COMP);
>>> +
>>> +    palmas_write(palmas_usb->palmas, PALMAS_USB_OTG_BASE,
>>> +        PALMAS_USB_ID_CTRL_SET, PALMAS_USB_ID_CTRL_SET_ID_ACT_COMP);
>>> +
>>> +    palmas_write(palmas_usb->palmas, PALMAS_USB_OTG_BASE,
>>> +        PALMAS_USB_ID_INT_EN_HI_SET,
>>> +        PALMAS_USB_ID_INT_EN_HI_SET_ID_GND);
>>> +
>>> +    palmas_vbus_wakeup_irq(palmas_usb->irq4, palmas_usb);
>>> +    palmas_id_wakeup_irq(palmas_usb->irq2, palmas_usb);
>>> +}
>>> +
>>> +static void palmas_set_vbus_work(struct work_struct *data)
>>> +{
>>> +    int ret;
>>> +    struct palmas_usb *palmas_usb = container_of(data, struct palmas_usb,
>>> +                                set_vbus_work);
>>> +
>>> +    if (IS_ERR_OR_NULL(palmas_usb->vbus_reg)) {
>>> +        dev_err(palmas_usb->dev, "invalid regulator\n");
>>> +        return;
>>> +    }
>>> +
>>> +    /*
>>> +     * Start driving VBUS. Set OPA_MODE bit in CHARGERUSB_CTRL1
>>> +     * register. This enables boost mode.
>>> +     */
>>> +
>>> +    if (palmas_usb->vbus_enable) {
>>> +        ret = regulator_enable(palmas_usb->vbus_reg);
>>> +        if (ret)
>>> +            dev_dbg(palmas_usb->dev, "regulator enable failed\n");
>>> +    } else {
>>> +        regulator_disable(palmas_usb->vbus_reg);
>>> +    }
>>> +}
>>> +
>>> +static int palmas_set_vbus(struct phy_companion *comparator, bool enabled)
>>> +{
>>> +    struct palmas_usb *palmas_usb = comparator_to_palmas(comparator);
>>> +
>>> +    palmas_usb->vbus_enable = enabled;
>>> +    schedule_work(&palmas_usb->set_vbus_work);
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static int palmas_start_srp(struct phy_companion *comparator)
>>> +{
>>> +    struct palmas_usb *palmas_usb = comparator_to_palmas(comparator);
>>> +
>>> +    palmas_write(palmas_usb->palmas, PALMAS_USB_OTG_BASE,
>>> +        PALMAS_USB_VBUS_CTRL_SET, PALMAS_USB_VBUS_CTRL_SET_VBUS_DISCHRG
>>> +        | PALMAS_USB_VBUS_CTRL_SET_VBUS_IADP_SINK);
>>> +    palmas_write(palmas_usb->palmas, PALMAS_USB_OTG_BASE,
>>> +        PALMAS_USB_VBUS_CTRL_SET,
>>> +        PALMAS_USB_VBUS_CTRL_SET_VBUS_CHRG_VSYS |
>>> +        PALMAS_USB_VBUS_CTRL_SET_VBUS_IADP_SINK);
>>> +
>>> +    mdelay(100);
>>> +
>>> +    palmas_write(palmas_usb->palmas, PALMAS_USB_OTG_BASE,
>>> +        PALMAS_USB_VBUS_CTRL_CLR,
>>> +        PALMAS_USB_VBUS_CTRL_SET_VBUS_CHRG_VSYS |
>>> +        PALMAS_USB_VBUS_CTRL_SET_VBUS_CHRG_VSYS);
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static void palmas_dt_to_pdata(struct device_node *node,
>>> +        struct palmas_usb_platform_data *pdata)
>>> +{
>>> +    pdata->no_control_vbus = of_property_read_bool(node,
>>> +                    "ti,no_control_vbus");
>>> +    pdata->wakeup = of_property_read_bool(node, "ti,wakeup");
>>> +}
>>> +
>>> +static int palmas_usb_probe(struct platform_device *pdev)
>>> +{
>>> +    u32 ret;
>>> +    struct palmas *palmas = dev_get_drvdata(pdev->dev.parent);
>>> +    struct palmas_usb_platform_data    *pdata = pdev->dev.platform_data;
>>> +    struct device_node *node = pdev->dev.of_node;
>>> +    struct palmas_usb *palmas_usb;
>>> +    int status;
>>> +
>>> +    if (node && !pdata) {
>>> +        pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
>>> +
>>> +        if (!pdata)
>>> +            return -ENOMEM;
>>> +
>>> +        palmas_dt_to_pdata(node, pdata);
>>> +    }
>>> +
>>> +    if (!pdata)
>>> +        return -EINVAL;
>>> +
>>> +    palmas_usb = devm_kzalloc(&pdev->dev, sizeof(*palmas_usb), GFP_KERNEL);
>>> +    if (!palmas_usb)
>>> +        return -ENOMEM;
>>> +
>>> +    palmas->usb        = palmas_usb;
>>> +    palmas_usb->palmas    = palmas;
>>> +
>>> +    palmas_usb->dev        = &pdev->dev;
>>> +
>>> +    palmas_usb->irq1 = regmap_irq_get_virq(palmas->irq_data,
>>> +                        PALMAS_ID_OTG_IRQ);
>>> +    palmas_usb->irq2 = regmap_irq_get_virq(palmas->irq_data,
>>> +                        PALMAS_ID_IRQ);
>>> +    palmas_usb->irq3 = regmap_irq_get_virq(palmas->irq_data,
>>> +                        PALMAS_VBUS_OTG_IRQ);
>>> +    palmas_usb->irq4 = regmap_irq_get_virq(palmas->irq_data,
>>> +                        PALMAS_VBUS_IRQ);
>>> +
>>> +    palmas_usb->comparator.set_vbus    = palmas_set_vbus;
>>> +    palmas_usb->comparator.start_srp = palmas_start_srp;
>>> +
>>> +    palmas_usb_wakeup(palmas, pdata->wakeup);
>>> +
>>> +    /* init spinlock for workqueue */
>>> +    spin_lock_init(&palmas_usb->lock);
>>> +
>>> +    if (!pdata->no_control_vbus) {
>>> +        palmas_usb->vbus_reg = devm_regulator_get(&pdev->dev, "vbus");
>>> +        if (IS_ERR(palmas_usb->vbus_reg)) {
>>> +            dev_err(&pdev->dev, "vbus init failed\n");
>>> +            return PTR_ERR(palmas_usb->vbus_reg);
>>> +        }
>>> +    }
>>> +
>>> +    platform_set_drvdata(pdev, palmas_usb);
>>> +
>>> +    if (device_create_file(&pdev->dev, &dev_attr_vbus))
>>> +        dev_warn(&pdev->dev, "could not create sysfs file\n");
>> device_create_file() isn't needed if remove 'palmas_usb_vbus' sysfs entry.
>
> right.
>
>>> +
>>> +    palmas_usb->edev.name = "palmas_usb";
>> I prefer '-' instead of '_'. change from "palmas_usb" to "palmas-usb".
>>> +    palmas_usb->edev.supported_cable = palmas_extcon_cable;
>>> +    palmas_usb->edev.mutually_exclusive = mutually_exclusive;
>>> +
>>> +    ret = extcon_dev_register(&palmas_usb->edev, palmas_usb->dev);
>>> +    if (ret) {
>>> +        dev_err(&pdev->dev, "failed to register extcon device\n");
>>> +        return ret;
>>> +    }
>>> +
>>> +    /* init spinlock for workqueue */
>>> +    spin_lock_init(&palmas_usb->lock);
>>> +
>>> +    INIT_WORK(&palmas_usb->set_vbus_work, palmas_set_vbus_work);
>>> +
>>> +    status = devm_request_threaded_irq(palmas_usb->dev, palmas_usb->irq2,
>>> +            NULL, palmas_id_wakeup_irq,
>>> +            IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING,
>>> +            "palmas_usb", palmas_usb);
>>> +    if (status < 0) {
>>> +        dev_err(&pdev->dev, "can't get IRQ %d, err %d\n",
>>> +                    palmas_usb->irq2, status);
>>> +        goto fail_irq;
>>> +    }
>>> +
>>> +    status = devm_request_threaded_irq(palmas_usb->dev, palmas_usb->irq4,
>>> +            NULL, palmas_vbus_wakeup_irq,
>>> +            IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING,
>>> +            "palmas_usb", palmas_usb);
>>> +    if (status < 0) {
>>> +        dev_err(&pdev->dev, "can't get IRQ %d, err %d\n",
>>> +                    palmas_usb->irq4, status);
>>> +        goto fail_irq;
>>> +    }
>>> +
>>> +    palmas_enable_irq(palmas_usb);
>>> +
>>> +    return 0;
>>> +
>>> +fail_irq:
>>> +    cancel_work_sync(&palmas_usb->set_vbus_work);
>>> +    device_remove_file(palmas_usb->dev, &dev_attr_vbus);
>> ditto.
>>> +
>>> +    return status;
>>> +}
>>> +
>>> +static int palmas_usb_remove(struct platform_device *pdev)
>>> +{
>>> +    struct palmas_usb *palmas_usb = platform_get_drvdata(pdev);
>>> +
>>> +    device_remove_file(palmas_usb->dev, &dev_attr_vbus);
>> ditto.
>>> +    cancel_work_sync(&palmas_usb->set_vbus_work);
>>> +    extcon_dev_unregister(&palmas_usb->edev);
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static struct of_device_id of_palmas_match_tbl[] = {
>>> +    { .compatible = "ti,palmas-usb", },
>>> +    { .compatible = "ti,twl6035-usb", },
>>> +    { /* end */ }
>>> +};
>>> +
>>> +static struct platform_driver palmas_usb_driver = {
>>> +    .probe = palmas_usb_probe,
>>> +    .remove = palmas_usb_remove,
>>> +    .driver = {
>>> +        .name = "palmas-usb",
>>> +        .of_match_table = of_palmas_match_tbl,
>>> +        .owner = THIS_MODULE,
>>> +    },
>>> +};
>>> +
>>> +module_platform_driver(palmas_usb_driver);
>>> +
>>> +MODULE_ALIAS("platform:palmas-usb");
>>> +MODULE_AUTHOR("Graeme Gregory <gg@...mlogic.co.uk>");
>>> +MODULE_DESCRIPTION("Palmas USB transceiver driver");
>>> +MODULE_LICENSE("GPL");
>>> +MODULE_DEVICE_TABLE(of, of_palmas_match_tbl);
>>> diff --git a/include/linux/extcon/extcon_palmas.h b/include/linux/extcon/extcon_palmas.h
>>> new file mode 100644
>>> index 0000000..a5119c9
>>> --- /dev/null
>>> +++ b/include/linux/extcon/extcon_palmas.h
>>> @@ -0,0 +1,26 @@
>>> +/*
>>> + * extcon_palmas.h - palmas extcon driver to detect VBUS or ID events
>>> + *
>>> + * Copyright (C) 2013 Texas Instruments Incorporated - http://www.ti.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.
>>> + *
>>> + * Author: Kishon Vijay Abraham I <kishon@ti.com>
>>> + *
>>> + * 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.
>>> + *
>>> + */
>>> +
>>> +#ifndef __EXTCON_PALMAS_H__
>>> +#define __EXTCON_PALMAS_H__
>>> +
>>> +#define    PALMAS_USB_STATE_DISCONNECT    0x0
>>> +#define    PALMAS_USB_STATE_VBUS        BIT(0)
>>> +#define    PALMAS_USB_STATE_ID        BIT(1)
>>> +
>> The defined variable in extcon_palmas.h is used only on extcon-palmas.c.
>> So, I would like to move definition from extcon_palmas.h to extcon-palmas.c
>> and remove extcon_palmas.h header file.
>
> Actually it has to be used in dwc3-omap.c (that was in a different patch).
>

Should detect the state of USB/USB-HOST on dwc3-omap driver?

If yes, dwc3-omap driver can immediately detect the changed state of USB/USB-HOST
by using excon_register_interest() function which is defined in extcon-class.c

I explain simple usage of extcon_register_interest()
to receive newly state of USB cable on dwc3-omap driver.
-----------
    struct extcon_specific_cable_nb extcon_notifier
    struct notifier_block extcon_notifier;

     /* ... */

    extcon_notifier.notifier_call = omap_extcon_notifier;
    ret = extcon_register_interest(&extcon_dev, "USB", &extcon_notifier);
    /* ... */

    int omap_extcon_notifier(struct notifier_block *self,
                                                unsigned long event, void *ptr)
    {
            int usb_state;

            usb_state = event;                                   

            /* if usb_state is 1, PALMAS_USB_STATE_VBUS  */
            /* if usb_state is 0, PALMAS_USB_STATE_DISCONNECT */

            /* TODO */

    }
-----------

If dwc3-omap driver use extcon_register_interest(), following defined variables
are able to be removed.
    PALMAS_USB_STATE_DISCONNECT
    PALMAS_USB_STATE_VBUS
    PALMAS_USB_STATE_ID

Thanks,
Chanwoo Choi

Re: [PATCH v4] extcon: Palmas Extcon Driver

[Chanwoo Choi]

On 05/07/2013 03:57 PM, Chanwoo Choi wrote:
> diff --git a/include/linux/extcon/extcon_palmas.h b/include/linux/extcon/extcon_palmas.h
> new file mode 100644
> index 0000000..a5119c9
> --- /dev/null
> +++ b/include/linux/extcon/extcon_palmas.h
> @@ -0,0 +1,26 @@
> +/*
> + * extcon_palmas.h - palmas extcon driver to detect VBUS or ID events
> + *
> + * Copyright (C) 2013 Texas Instruments Incorporated - http://www.ti.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.
> + *
> + * Author: Kishon Vijay Abraham I <kishon@ti.com>
> + *
> + * 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.
> + *
> + */
> +
> +#ifndef __EXTCON_PALMAS_H__
> +#define __EXTCON_PALMAS_H__
> +
> +#define    PALMAS_USB_STATE_DISCONNECT    0x0
> +#define    PALMAS_USB_STATE_VBUS        BIT(0)
> +#define    PALMAS_USB_STATE_ID        BIT(1)
> +
>>> The defined variable in extcon_palmas.h is used only on extcon-palmas.c.
>>> So, I would like to move definition from extcon_palmas.h to extcon-palmas.c
>>> and remove extcon_palmas.h header file.
>> Actually it has to be used in dwc3-omap.c (that was in a different patch).
>>
> Should detect the state of USB/USB-HOST on dwc3-omap driver?
>
> If yes, dwc3-omap driver can immediately detect the changed state of USB/USB-HOST
> by using excon_register_interest() function which is defined in extcon-class.c
>
> I explain simple usage of extcon_register_interest()
> to receive newly state of USB cable on dwc3-omap driver.
> -----------
>     struct extcon_specific_cable_nb extcon_notifier
>     struct notifier_block extcon_notifier;
>
>      /* ... */
>
>     extcon_notifier.notifier_call = omap_extcon_notifier;
>     ret = extcon_register_interest(&extcon_dev, "USB", &extcon_notifier);
    Fix usage of extcon_register_interest() as following:

    ret = extcon_register_interest(&extcon_dev, NULL, "USB", &extcon_notifier); or
    ret = extcon_register_interest(&extcon_dev, "palmas-usb", "USB", &extcon_notifier);
>     /* ... */
>
>     int omap_extcon_notifier(struct notifier_block *self,
>                                                 unsigned long event, void *ptr)
>     {
>             int usb_state;
>
>             usb_state = event;                                   
>
>             /* if usb_state is 1, PALMAS_USB_STATE_VBUS  */
>             /* if usb_state is 0, PALMAS_USB_STATE_DISCONNECT */
>
>             /* TODO */
>
>     }
> -----------
>
> If dwc3-omap driver use extcon_register_interest(), following defined variables
> are able to be removed.
>     PALMAS_USB_STATE_DISCONNECT
>     PALMAS_USB_STATE_VBUS
>     PALMAS_USB_STATE_ID
>
> Thanks,
> Chanwoo Choi
>

Re: [PATCH v4] extcon: Palmas Extcon Driver

[Mark Brown]

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

On Tue, May 07, 2013 at 10:42:53AM +0530, Kishon Vijay Abraham I wrote:
> On Monday 06 May 2013 08:10 PM, Mark Brown wrote:
> >On Mon, May 06, 2013 at 06:47:04PM +0530, Kishon Vijay Abraham I wrote:
> >
> >>+		if (palmas_usb->linkstat != PALMAS_USB_STATE_VBUS) {
> >>+			if (palmas_usb->vbus_reg) {
> >>+				ret = regulator_enable(palmas_usb->vbus_reg);
> >>+				if (ret) {
> >>+					dev_dbg(palmas_usb->dev,
> >>+						"regulator enable failed\n");
> >>+					goto ret0;

> >This is very bad karma, why is the regulator optional?

> The platform can provide it's own vbus control in which case this is
> not needed.

So why is there no interaction with this external VBUS control and how
does the driver distinguish between that and an error getting the
regulator?

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v4] extcon: Palmas Extcon Driver

[Kishon Vijay Abraham I]

On Tuesday 07 May 2013 12:35 PM, Chanwoo Choi wrote:
> On 05/07/2013 03:57 PM, Chanwoo Choi wrote:
>> diff --git a/include/linux/extcon/extcon_palmas.h b/include/linux/extcon/extcon_palmas.h
>> new file mode 100644
>> index 0000000..a5119c9
>> --- /dev/null
>> +++ b/include/linux/extcon/extcon_palmas.h
>> @@ -0,0 +1,26 @@
>> +/*
>> + * extcon_palmas.h - palmas extcon driver to detect VBUS or ID events
>> + *
>> + * Copyright (C) 2013 Texas Instruments Incorporated - http://www.ti.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.
>> + *
>> + * Author: Kishon Vijay Abraham I <kishon@ti.com>
>> + *
>> + * 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.
>> + *
>> + */
>> +
>> +#ifndef __EXTCON_PALMAS_H__
>> +#define __EXTCON_PALMAS_H__
>> +
>> +#define    PALMAS_USB_STATE_DISCONNECT    0x0
>> +#define    PALMAS_USB_STATE_VBUS        BIT(0)
>> +#define    PALMAS_USB_STATE_ID        BIT(1)
>> +
>>>> The defined variable in extcon_palmas.h is used only on extcon-palmas.c.
>>>> So, I would like to move definition from extcon_palmas.h to extcon-palmas.c
>>>> and remove extcon_palmas.h header file.
>>> Actually it has to be used in dwc3-omap.c (that was in a different patch).
>>>
>> Should detect the state of USB/USB-HOST on dwc3-omap driver?
>>
>> If yes, dwc3-omap driver can immediately detect the changed state of USB/USB-HOST
>> by using excon_register_interest() function which is defined in extcon-class.c
>>
>> I explain simple usage of extcon_register_interest()
>> to receive newly state of USB cable on dwc3-omap driver.
>> -----------
>>      struct extcon_specific_cable_nb extcon_notifier
>>      struct notifier_block extcon_notifier;
>>
>>       /* ... */
>>
>>      extcon_notifier.notifier_call = omap_extcon_notifier;
>>      ret = extcon_register_interest(&extcon_dev, "USB", &extcon_notifier);
>      Fix usage of extcon_register_interest() as following:
>
>      ret = extcon_register_interest(&extcon_dev, NULL, "USB", &extcon_notifier); or
>      ret = extcon_register_interest(&extcon_dev, "palmas-usb", "USB", &extcon_notifier);

By this we have to define 2 notifiers (one for USB and the other for 
USB-HOST). I thought of having a single notifier and handling it using 
states. It was something like this http://pastebin.com/Nv7q9swz.

Thanks
Kishon

Re: [PATCH v4] extcon: Palmas Extcon Driver

[Kishon Vijay Abraham I]

Hi,

On Tuesday 07 May 2013 01:28 PM, Mark Brown wrote:
> On Tue, May 07, 2013 at 10:42:53AM +0530, Kishon Vijay Abraham I wrote:
>> On Monday 06 May 2013 08:10 PM, Mark Brown wrote:
>>> On Mon, May 06, 2013 at 06:47:04PM +0530, Kishon Vijay Abraham I wrote:
>>>
>>>> +		if (palmas_usb->linkstat != PALMAS_USB_STATE_VBUS) {
>>>> +			if (palmas_usb->vbus_reg) {
>>>> +				ret = regulator_enable(palmas_usb->vbus_reg);
>>>> +				if (ret) {
>>>> +					dev_dbg(palmas_usb->dev,
>>>> +						"regulator enable failed\n");
>>>> +					goto ret0;
>
>>> This is very bad karma, why is the regulator optional?
>
>> The platform can provide it's own vbus control in which case this is
>> not needed.
>
> So why is there no interaction with this external VBUS control and how
> does the driver distinguish between that and an error getting the
> regulator?

The platform specifies if it provides its own VBUS control by the dt 
property ti,no_control_vbus. So the driver will give an error only when 
*ti,no_control_vbus* is not set. Graeme?

Thanks
Kishon

Re: [PATCH v4] extcon: Palmas Extcon Driver

[Graeme Gregory]

On 07/05/13 10:47, Kishon Vijay Abraham I wrote:
> Hi,
>
> On Tuesday 07 May 2013 01:28 PM, Mark Brown wrote:
>> On Tue, May 07, 2013 at 10:42:53AM +0530, Kishon Vijay Abraham I wrote:
>>> On Monday 06 May 2013 08:10 PM, Mark Brown wrote:
>>>> On Mon, May 06, 2013 at 06:47:04PM +0530, Kishon Vijay Abraham I
>>>> wrote:
>>>>
>>>>> +        if (palmas_usb->linkstat != PALMAS_USB_STATE_VBUS) {
>>>>> +            if (palmas_usb->vbus_reg) {
>>>>> +                ret = regulator_enable(palmas_usb->vbus_reg);
>>>>> +                if (ret) {
>>>>> +                    dev_dbg(palmas_usb->dev,
>>>>> +                        "regulator enable failed\n");
>>>>> +                    goto ret0;
>>
>>>> This is very bad karma, why is the regulator optional?
>>
>>> The platform can provide it's own vbus control in which case this is
>>> not needed.
>>
>> So why is there no interaction with this external VBUS control and how
>> does the driver distinguish between that and an error getting the
>> regulator?
>
> The platform specifies if it provides its own VBUS control by the dt
> property ti,no_control_vbus. So the driver will give an error only
> when *ti,no_control_vbus* is not set. Graeme?
>
> Thanks
> Kishon
That is correct, but as currently there are only two users I guess this
depends on Laxmans usecase. If he doesn't need this extra complexity I
would remove it for now. It was originally done with another SoC in mind!

Graeme

Re: [PATCH v4] extcon: Palmas Extcon Driver

[Mark Brown]

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

On Tue, May 07, 2013 at 03:17:08PM +0530, Kishon Vijay Abraham I wrote:
> On Tuesday 07 May 2013 01:28 PM, Mark Brown wrote:

> >>The platform can provide it's own vbus control in which case this is
> >>not needed.

> >So why is there no interaction with this external VBUS control and how
> >does the driver distinguish between that and an error getting the
> >regulator?

> The platform specifies if it provides its own VBUS control by the dt
> property ti,no_control_vbus. So the driver will give an error only
> when *ti,no_control_vbus* is not set. Graeme?

That doesn't answer the question about why there's no interaction with
the external control...

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v4] extcon: Palmas Extcon Driver

[Kishon Vijay Abraham I]

On Tuesday 07 May 2013 04:15 PM, Mark Brown wrote:
> On Tue, May 07, 2013 at 03:17:08PM +0530, Kishon Vijay Abraham I wrote:
>> On Tuesday 07 May 2013 01:28 PM, Mark Brown wrote:
>
>>>> The platform can provide it's own vbus control in which case this is
>>>> not needed.
>
>>> So why is there no interaction with this external VBUS control and how
>>> does the driver distinguish between that and an error getting the
>>> regulator?
>
>> The platform specifies if it provides its own VBUS control by the dt
>> property ti,no_control_vbus. So the driver will give an error only
>> when *ti,no_control_vbus* is not set. Graeme?
>
> That doesn't answer the question about why there's no interaction with
> the external control...
>

Graeme?

-Kishon

Re: [PATCH v4] extcon: Palmas Extcon Driver

[Graeme Gregory]

On Tue, May 14, 2013 at 02:48:53PM +0530, Kishon Vijay Abraham I wrote:
> On Tuesday 07 May 2013 04:15 PM, Mark Brown wrote:
> >On Tue, May 07, 2013 at 03:17:08PM +0530, Kishon Vijay Abraham I wrote:
> >>On Tuesday 07 May 2013 01:28 PM, Mark Brown wrote:
> >
> >>>>The platform can provide it's own vbus control in which case this is
> >>>>not needed.
> >
> >>>So why is there no interaction with this external VBUS control and how
> >>>does the driver distinguish between that and an error getting the
> >>>regulator?
> >
> >>The platform specifies if it provides its own VBUS control by the dt
> >>property ti,no_control_vbus. So the driver will give an error only
> >>when *ti,no_control_vbus* is not set. Graeme?
> >
> >That doesn't answer the question about why there's no interaction with
> >the external control...
> >
> 
> Graeme?
> 
I already partially answered this, it was written for a SoC where VBUS
generation was in hardware on the SoC with no information.

I would say if nvidia/Laxman do not need this remove it, if someone ever
uses it then they can re-add it easy enough!

Graeme

Re: [PATCH v4] extcon: Palmas Extcon Driver

[Laxman Dewangan]

On Tuesday 14 May 2013 03:24 PM, Graeme Gregory wrote:
> On Tue, May 14, 2013 at 02:48:53PM +0530, Kishon Vijay Abraham I wrote:
>> On Tuesday 07 May 2013 04:15 PM, Mark Brown wrote:
>>> On Tue, May 07, 2013 at 03:17:08PM +0530, Kishon Vijay Abraham I wrote:
>>>> On Tuesday 07 May 2013 01:28 PM, Mark Brown wrote:
>>>>>> The platform can provide it's own vbus control in which case this is
>>>>>> not needed.
>>>>> So why is there no interaction with this external VBUS control and how
>>>>> does the driver distinguish between that and an error getting the
>>>>> regulator?
>>>> The platform specifies if it provides its own VBUS control by the dt
>>>> property ti,no_control_vbus. So the driver will give an error only
>>>> when *ti,no_control_vbus* is not set. Graeme?
>>> That doesn't answer the question about why there's no interaction with
>>> the external control...
>>>
>> Graeme?
>>
> I already partially answered this, it was written for a SoC where VBUS
> generation was in hardware on the SoC with no information.
>
> I would say if nvidia/Laxman do not need this remove it, if someone ever
> uses it then they can re-add it easy enough!

I think we really do not require this. Just we need the notification 
through extcon about the cable connection. We should remove it for 
avoiding complexity for now.

Re: [PATCH v4] extcon: Palmas Extcon Driver

[Kishon Vijay Abraham I]

Hi,

On Tuesday 07 May 2013 10:51 AM, Kishon Vijay Abraham I wrote:
> Hi,
>
> On Tuesday 07 May 2013 06:13 AM, myungjoo.ham wrote:
>>> From: Graeme Gregory <gg@slimlogic.co.uk>
>>>
>>> This is the driver for the USB comparator built into the palmas chip. It
>>> handles the various USB OTG events that can be generated by cable
>>> insertion/removal.
>>>
>>> Signed-off-by: Graeme Gregory <gg@slimlogic.co.uk>
>>> Signed-off-by: Moiz Sonasath <m-sonasath@ti.com>
>>> Signed-off-by: Ruchika Kharwar <ruchika@ti.com>
>>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>>> [kishon@ti.com: adapted palmas usb driver to use the extcon framework]
>>> Signed-off-by: Sebastien Guiriec <s-guiriec@ti.com>
>>
>> Here goes some comments in the code:
>>
>> []
>>
>>> diff --git a/drivers/extcon/extcon-palmas.c
>> b/drivers/extcon/extcon-palmas.c
>>> new file mode 100644
>>> index 0000000..3ef042f
>>> --- /dev/null
>>> +++ b/drivers/extcon/extcon-palmas.c
>>> @@ -0,0 +1,389 @@
>>> +/*
>>> + * Palmas USB transceiver driver
>>> + *
>>> + * Copyright (C) 2013 Texas Instruments Incorporated -
>>> http://www.ti.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.
>>> + *
>>> + * Author: Graeme Gregory <gg@...mlogic.co.uk>
>>> + * Author: Kishon Vijay Abraham I <kishon@...com>
>>> + *
>>> + * Based on twl6030_usb.c
>>> + *
>>> + * Author: Hema HK <hemahk@...com>
>>> + *
>>> + * 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.
>>> + */
>>
>> Why the email addresses are masked in the source code?
>> (I'm just curious as this is the first time to see such in kernel source)
>
> That was not intentional. Took the previous version from the net. My bad.
>>
>>> +
>>> +#include <linux/module.h>
>>> +#include <linux/init.h>
>>> +#include <linux/interrupt.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/io.h>
>>> +#include <linux/usb/phy_companion.h>
>>> +#include <linux/regulator/consumer.h>
>>> +#include <linux/err.h>
>>> +#include <linux/notifier.h>
>>> +#include <linux/slab.h>
>>> +#include <linux/delay.h>
>>> +#include <linux/mfd/palmas.h>
>>> +#include <linux/of.h>
>>> +#include <linux/of_platform.h>
>>> +#include <linux/extcon/extcon_palmas.h>
>>> +
>>> +static const char *palmas_extcon_cable[] = {
>>> +    [0] = "USB",
>>> +    [1] = "USB-HOST",
>>
>>     [1] = "USB-Host",
>>
>>> +    NULL,
>>> +};
>>> +
>>> +static const int mutually_exclusive[] = {0x3, 0x0};
>>> +
>>> +static void palmas_usb_wakeup(struct palmas *palmas, int enable)
>>> +{
>>> +    if (enable)
>>> +        palmas_write(palmas, PALMAS_USB_OTG_BASE, PALMAS_USB_WAKEUP,
>>> +            PALMAS_USB_WAKEUP_ID_WK_UP_COMP);
>>> +    else
>>> +        palmas_write(palmas, PALMAS_USB_OTG_BASE, PALMAS_USB_WAKEUP,
>> 0);
>>> +}
>>> +
>>> +static ssize_t palmas_usb_vbus_show(struct device *dev,
>>> +            struct device_attribute *attr, char *buf)
>>> +{
>>> +    unsigned long        flags;
>>> +    int            ret = -EINVAL;
>>> +    struct palmas_usb    *palmas_usb = dev_get_drvdata(dev);
>>> +
>>> +    spin_lock_irqsave(&palmas_usb->lock, flags);
>>
>> This spinlock is used in this sysfs-show function only.
>> Is this really needed?
>> The only protected value here is "linkstat", which is "readonly"
>> in this protected context.
>> Thus, removing the spin_lock and updating like the following should
>> be the same with less overhead:
>>
>>     int linkstat = palmas_usb->linkstat;
>>     switch(linkstat) {
>
> hmm.. ok.
I think this is to do with disabling interrupts while reporting the VBUS 
state. (linkstat is updated in irq handler). So this should be fine I guess?

Thanks
Kishon