[PATCH 0/4] extcon: usb-gpio: fixes and improvements

[PATCH 0/4] extcon: usb-gpio: fixes and improvements

[Robert Baldyga]

Hello,

This patch set modifies extcon-usb-gpio driver fixing bugs, and adds
new features - VBUS pin detection support and 'debounce' property in
devicetree node. It also updates documentation with information about
new features.

More detailed description of changes can be found in commit messages.

Best regards,
Robert Baldyga

Robert Baldyga (4):
  extcon: usb-gpio: register extcon device before IRQ registration
  extcon: usb-gpio: add support for VBUS detection
  extcon: usb-gpio: make debounce value configurable in devicetree
  Documentation: extcon: usb-gpio: update usb-gpio binding description

 .../devicetree/bindings/extcon/extcon-usb-gpio.txt |  23 ++-
 drivers/extcon/extcon-usb-gpio.c                   | 171 +++++++++++++++------
 2 files changed, 147 insertions(+), 47 deletions(-)

-- 
1.9.1

[PATCH 1/4] extcon: usb-gpio: register extcon device before IRQ registration

[Robert Baldyga]

IRQ handler touches info->edev, so if interrupt occurs before extcon
device initialization it can cause NULL pointer dereference. Doing extcon
initialization before IRQ handler registration fixes this problem.

Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>
---
 drivers/extcon/extcon-usb-gpio.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/extcon/extcon-usb-gpio.c b/drivers/extcon/extcon-usb-gpio.c
index 3f0bad3..f6aa99d 100644
--- a/drivers/extcon/extcon-usb-gpio.c
+++ b/drivers/extcon/extcon-usb-gpio.c
@@ -119,6 +119,18 @@ static int usb_extcon_probe(struct platform_device *pdev)
 		return PTR_ERR(info->id_gpiod);
 	}
 
+	info->edev = devm_extcon_dev_allocate(dev, usb_extcon_cable);
+	if (IS_ERR(info->edev)) {
+		dev_err(dev, "failed to allocate extcon device\n");
+		return -ENOMEM;
+	}
+
+	ret = devm_extcon_dev_register(dev, info->edev);
+	if (ret < 0) {
+		dev_err(dev, "failed to register extcon device\n");
+		return ret;
+	}
+
 	ret = gpiod_set_debounce(info->id_gpiod,
 				 USB_GPIO_DEBOUNCE_MS * 1000);
 	if (ret < 0)
@@ -142,18 +154,6 @@ static int usb_extcon_probe(struct platform_device *pdev)
 		return ret;
 	}
 
-	info->edev = devm_extcon_dev_allocate(dev, usb_extcon_cable);
-	if (IS_ERR(info->edev)) {
-		dev_err(dev, "failed to allocate extcon device\n");
-		return -ENOMEM;
-	}
-
-	ret = devm_extcon_dev_register(dev, info->edev);
-	if (ret < 0) {
-		dev_err(dev, "failed to register extcon device\n");
-		return ret;
-	}
-
 	platform_set_drvdata(pdev, info);
 	device_init_wakeup(dev, 1);
 
-- 
1.9.1

[PATCH 2/4] extcon: usb-gpio: add support for VBUS detection

[Robert Baldyga]

This patch adds VBUS pin detection support to extcon-usb-gpio driver.
It allows to use this driver with boards which have both VBUS and ID
pins, or only one of them.

Following table of states presents relationship between this signals
and detected cable type:

 State        |    ID   |   VBUS
----------------------------------
 [1] USB      |    H    |    H
 [2] disconn. |    H    |    L
 [3] USB-HOST |    L    |    H
 [4] USB-HOST |    L    |    L

In case we have only one of these signals:
- ID pin only: we can distinguish between states [1] and [3].
- VBUS pin only: we can distinguish between states [1] and [2].

Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>
---
 drivers/extcon/extcon-usb-gpio.c | 159 +++++++++++++++++++++++++++++----------
 1 file changed, 118 insertions(+), 41 deletions(-)

diff --git a/drivers/extcon/extcon-usb-gpio.c b/drivers/extcon/extcon-usb-gpio.c
index f6aa99d..02ff40e 100644
--- a/drivers/extcon/extcon-usb-gpio.c
+++ b/drivers/extcon/extcon-usb-gpio.c
@@ -32,7 +32,9 @@ struct usb_extcon_info {
 	struct extcon_dev *edev;
 
 	struct gpio_desc *id_gpiod;
+	struct gpio_desc *vbus_gpiod;
 	int id_irq;
+	int vbus_irq;
 
 	unsigned long debounce_jiffies;
 	struct delayed_work wq_detcable;
@@ -52,21 +54,50 @@ static const char *usb_extcon_cable[] = {
 	NULL,
 };
 
+/*
+ *  State        |    ID   |   VBUS
+ * ----------------------------------
+ *  [1] USB      |    H    |    H
+ *  [2] disconn. |    H    |    L
+ *  [3] USB-HOST |    L    |    H
+ *  [4] USB-HOST |    L    |    L
+ *
+ */
+
 static void usb_extcon_detect_cable(struct work_struct *work)
 {
 	int id;
+	int vbus;
 	struct usb_extcon_info *info = container_of(to_delayed_work(work),
 						    struct usb_extcon_info,
 						    wq_detcable);
 
-	/* check ID and update cable state */
-	id = gpiod_get_value_cansleep(info->id_gpiod);
-	if (id) {
-		/*
-		 * ID = 1 means USB HOST cable detached.
-		 * As we don't have event for USB peripheral cable attached,
-		 * we simulate USB peripheral attach here.
-		 */
+	/* check ID and VBUS and update cable state */
+
+	/*
+	 * If we don't have id pin, we want to detect states [1] and [2],
+	 * so we set id to value 1.
+	 */
+	id = info->id_gpiod ?
+		gpiod_get_value_cansleep(info->id_gpiod) : 1;
+
+	/*
+	 * If we don't have vbus pin, we want to detect states [1] and [3],
+	 * so we set vbus to value 1.
+	 */
+	vbus = info->vbus_gpiod ?
+		gpiod_get_value_cansleep(info->vbus_gpiod) : 1;
+
+	if (id == 0) {
+		/* ID = 0 means USB HOST cable attached. */
+		extcon_set_cable_state(info->edev,
+				       usb_extcon_cable[EXTCON_CABLE_USB],
+				       false);
+		extcon_set_cable_state(info->edev,
+				       usb_extcon_cable[EXTCON_CABLE_USB_HOST],
+				       true);
+	} else if (vbus) {
+		/* ID = 1 and VBUS = 1 means USB cable attached. */
 		extcon_set_cable_state(info->edev,
 				       usb_extcon_cable[EXTCON_CABLE_USB_HOST],
 				       false);
@@ -74,17 +105,13 @@ static void usb_extcon_detect_cable(struct work_struct *work)
 				       usb_extcon_cable[EXTCON_CABLE_USB],
 				       true);
 	} else {
-		/*
-		 * ID = 0 means USB HOST cable attached.
-		 * As we don't have event for USB peripheral cable detached,
-		 * we simulate USB peripheral detach here.
-		 */
+		/* ID = 1 and VBUS = 0 means USB cable detached. */
 		extcon_set_cable_state(info->edev,
 				       usb_extcon_cable[EXTCON_CABLE_USB],
 				       false);
 		extcon_set_cable_state(info->edev,
 				       usb_extcon_cable[EXTCON_CABLE_USB_HOST],
-				       true);
+				       false);
 	}
 }
 
@@ -113,10 +140,12 @@ static int usb_extcon_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	info->dev = dev;
-	info->id_gpiod = devm_gpiod_get(&pdev->dev, "id");
-	if (IS_ERR(info->id_gpiod)) {
-		dev_err(dev, "failed to get ID GPIO\n");
-		return PTR_ERR(info->id_gpiod);
+	info->id_gpiod = devm_gpiod_get_optional(&pdev->dev, "id");
+	info->vbus_gpiod = devm_gpiod_get_optional(&pdev->dev, "vbus");
+
+	if (!info->id_gpiod && !info->vbus_gpiod) {
+		dev_err(dev, "failed to get gpios\n");
+		return -ENODEV;
 	}
 
 	info->edev = devm_extcon_dev_allocate(dev, usb_extcon_cable);
@@ -131,27 +160,51 @@ static int usb_extcon_probe(struct platform_device *pdev)
 		return ret;
 	}
 
-	ret = gpiod_set_debounce(info->id_gpiod,
-				 USB_GPIO_DEBOUNCE_MS * 1000);
+	if (info->id_gpiod)
+		ret = gpiod_set_debounce(info->id_gpiod,
+			USB_GPIO_DEBOUNCE_MS * 1000);
+	if (!ret && info->vbus_gpiod) {
+		ret = gpiod_set_debounce(info->vbus_gpiod,
+			USB_GPIO_DEBOUNCE_MS * 1000);
+		if (ret < 0 && info->id_gpiod)
+			gpiod_set_debounce(info->vbus_gpiod, 0);
+	}
 	if (ret < 0)
 		info->debounce_jiffies = msecs_to_jiffies(USB_GPIO_DEBOUNCE_MS);
 
 	INIT_DELAYED_WORK(&info->wq_detcable, usb_extcon_detect_cable);
 
-	info->id_irq = gpiod_to_irq(info->id_gpiod);
-	if (info->id_irq < 0) {
-		dev_err(dev, "failed to get ID IRQ\n");
-		return info->id_irq;
+	if (info->id_gpiod) {
+		info->id_irq = gpiod_to_irq(info->id_gpiod);
+		if (info->id_irq < 0) {
+			dev_err(dev, "failed to get ID IRQ\n");
+			return info->id_irq;
+		}
+		ret = devm_request_threaded_irq(dev, info->id_irq, NULL,
+				usb_irq_handler,
+				IRQF_TRIGGER_RISING |
+				IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
+				pdev->name, info);
+		if (ret < 0) {
+			dev_err(dev, "failed to request handler for ID IRQ\n");
+			return ret;
+		}
 	}
-
-	ret = devm_request_threaded_irq(dev, info->id_irq, NULL,
-					usb_irq_handler,
-					IRQF_TRIGGER_RISING |
-					IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
-					pdev->name, info);
-	if (ret < 0) {
-		dev_err(dev, "failed to request handler for ID IRQ\n");
-		return ret;
+	if (info->vbus_gpiod) {
+		info->vbus_irq = gpiod_to_irq(info->vbus_gpiod);
+		if (info->vbus_irq < 0) {
+			dev_err(dev, "failed to get VBUS IRQ\n");
+			return info->vbus_irq;
+		}
+		ret = devm_request_threaded_irq(dev, info->vbus_irq, NULL,
+				usb_irq_handler,
+				IRQF_TRIGGER_RISING |
+				IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
+				pdev->name, info);
+		if (ret < 0) {
+			dev_err(dev, "failed to request handler for VBUS IRQ\n");
+			return ret;
+		}
 	}
 
 	platform_set_drvdata(pdev, info);
@@ -179,9 +232,18 @@ static int usb_extcon_suspend(struct device *dev)
 	int ret = 0;
 
 	if (device_may_wakeup(dev)) {
-		ret = enable_irq_wake(info->id_irq);
-		if (ret)
-			return ret;
+		if (info->id_gpiod) {
+			ret = enable_irq_wake(info->id_irq);
+			if (ret)
+				return ret;
+		}
+		if (info->vbus_gpiod) {
+			ret = enable_irq_wake(info->vbus_irq);
+			if (ret) {
+				disable_irq_wake(info->id_irq);
+				return ret;
+			}
+		}
 	}
 
 	/*
@@ -189,7 +251,10 @@ static int usb_extcon_suspend(struct device *dev)
 	 * as GPIOs used behind I2C subsystem might not be
 	 * accessible until resume completes. So disable IRQ.
 	 */
-	disable_irq(info->id_irq);
+	if (info->id_gpiod)
+		disable_irq(info->id_irq);
+	if (info->vbus_gpiod)
+		disable_irq(info->vbus_irq);
 
 	return ret;
 }
@@ -200,12 +265,24 @@ static int usb_extcon_resume(struct device *dev)
 	int ret = 0;
 
 	if (device_may_wakeup(dev)) {
-		ret = disable_irq_wake(info->id_irq);
-		if (ret)
-			return ret;
+		if (info->id_gpiod) {
+			ret = disable_irq_wake(info->id_irq);
+			if (ret)
+				return ret;
+		}
+		if (info->vbus_gpiod) {
+			ret = disable_irq_wake(info->vbus_irq);
+			if (ret) {
+				enable_irq_wake(info->id_irq);
+				return ret;
+			}
+		}
 	}
 
-	enable_irq(info->id_irq);
+	if (info->id_gpiod)
+		enable_irq(info->id_irq);
+	if (info->vbus_gpiod)
+		enable_irq(info->vbus_irq);
 
 	return ret;
 }
-- 
1.9.1

[PATCH 3/4] extcon: usb-gpio: make debounce value configurable in devicetree

[Robert Baldyga]

This patch adds devicetree property for setting debounce value. It allows
to set debounce time shorter or longer depending on the needs of given
platform.

Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>
---
 drivers/extcon/extcon-usb-gpio.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/extcon/extcon-usb-gpio.c b/drivers/extcon/extcon-usb-gpio.c
index 02ff40e..86e3ece 100644
--- a/drivers/extcon/extcon-usb-gpio.c
+++ b/drivers/extcon/extcon-usb-gpio.c
@@ -130,6 +130,7 @@ static int usb_extcon_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	struct device_node *np = dev->of_node;
 	struct usb_extcon_info *info;
+	u32 debounce;
 	int ret;
 
 	if (!np)
@@ -140,6 +141,11 @@ static int usb_extcon_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	info->dev = dev;
+
+	ret = of_property_read_u32(np, "debounce", &debounce);
+	if (ret < 0)
+		debounce = USB_GPIO_DEBOUNCE_MS;
+
 	info->id_gpiod = devm_gpiod_get_optional(&pdev->dev, "id");
 	info->vbus_gpiod = devm_gpiod_get_optional(&pdev->dev, "vbus");
 
@@ -161,16 +167,14 @@ static int usb_extcon_probe(struct platform_device *pdev)
 	}
 
 	if (info->id_gpiod)
-		ret = gpiod_set_debounce(info->id_gpiod,
-			USB_GPIO_DEBOUNCE_MS * 1000);
+		ret = gpiod_set_debounce(info->id_gpiod, debounce * 1000);
 	if (!ret && info->vbus_gpiod) {
-		ret = gpiod_set_debounce(info->vbus_gpiod,
-			USB_GPIO_DEBOUNCE_MS * 1000);
+		ret = gpiod_set_debounce(info->vbus_gpiod, debounce * 1000);
 		if (ret < 0 && info->id_gpiod)
 			gpiod_set_debounce(info->vbus_gpiod, 0);
 	}
 	if (ret < 0)
-		info->debounce_jiffies = msecs_to_jiffies(USB_GPIO_DEBOUNCE_MS);
+		info->debounce_jiffies = msecs_to_jiffies(debounce);
 
 	INIT_DELAYED_WORK(&info->wq_detcable, usb_extcon_detect_cable);
 
-- 
1.9.1

[PATCH 4/4] Documentation: extcon: usb-gpio: update usb-gpio binding description

[Robert Baldyga]

Add information about VBUS pin detection support, 'debounce' property
and some other details.

Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>
---
 .../devicetree/bindings/extcon/extcon-usb-gpio.txt | 23 ++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/extcon/extcon-usb-gpio.txt b/Documentation/devicetree/bindings/extcon/extcon-usb-gpio.txt
index af0b903..d3fcf8b 100644
--- a/Documentation/devicetree/bindings/extcon/extcon-usb-gpio.txt
+++ b/Documentation/devicetree/bindings/extcon/extcon-usb-gpio.txt
@@ -1,16 +1,35 @@
 USB GPIO Extcon device
 
-This is a virtual device used to generate USB cable states from the USB ID pin
-connected to a GPIO pin.
+This is a virtual device used to generate USB cable states from the USB
+ID and VBUS signals connected to a GPIO pins.
+
+Some devices has only one of these GPIO pins, so we support cases when
+only one of them is present. Hence properties 'id-gpio' and 'vbus-gpio'
+are described as optional, but at least one of them has to be present
+in extcon-usb-gpio node.
+
+In general we have three cases:
+ 1. We have both VBUS and ID pin detection - we can detect USB, USB-HOST
+    and cable disconnection.
+ 2. We have only VBUS detection - we can detect USB and cable disconnection.
+ 3. We have ID pin only - we can distinguish between USB and USB-HOST
+    but without ability to detect cable disconnection.
 
 Required properties:
 - compatible: Should be "linux,extcon-usb-gpio"
+
+Optional properties
 - id-gpio: gpio for USB ID pin. See gpio binding.
+- vbus-gpio: gpio for USB VBUS pin. See gpio binding.
+- debounce: gpio debounce time in milliseconds (u32).
+
 
 Example: Examples of extcon-usb-gpio node in dra7-evm.dts as listed below:
 	extcon_usb1 {
 		compatible = "linux,extcon-usb-gpio";
 		id-gpio = <&gpio6 1 GPIO_ACTIVE_HIGH>;
+		vbus-gpio = <&gpio6 2 GPIO_ACTIVE_HIGH>;
+		debounce = <25>;
 	}
 
 	&omap_dwc3_1 {
-- 
1.9.1

Re: [PATCH 1/4] extcon: usb-gpio: register extcon device before IRQ registration

[Roger Quadros]

On 31/03/15 10:46, Robert Baldyga wrote:
> IRQ handler touches info->edev, so if interrupt occurs before extcon
> device initialization it can cause NULL pointer dereference. Doing extcon
> initialization before IRQ handler registration fixes this problem.
> 
> Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>

Acked-by: Roger Quadros <rogerq@ti.com>

cheers,
-roger

> ---
>  drivers/extcon/extcon-usb-gpio.c | 24 ++++++++++++------------
>  1 file changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/extcon/extcon-usb-gpio.c b/drivers/extcon/extcon-usb-gpio.c
> index 3f0bad3..f6aa99d 100644
> --- a/drivers/extcon/extcon-usb-gpio.c
> +++ b/drivers/extcon/extcon-usb-gpio.c
> @@ -119,6 +119,18 @@ static int usb_extcon_probe(struct platform_device *pdev)
>  		return PTR_ERR(info->id_gpiod);
>  	}
>  
> +	info->edev = devm_extcon_dev_allocate(dev, usb_extcon_cable);
> +	if (IS_ERR(info->edev)) {
> +		dev_err(dev, "failed to allocate extcon device\n");
> +		return -ENOMEM;
> +	}
> +
> +	ret = devm_extcon_dev_register(dev, info->edev);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to register extcon device\n");
> +		return ret;
> +	}
> +
>  	ret = gpiod_set_debounce(info->id_gpiod,
>  				 USB_GPIO_DEBOUNCE_MS * 1000);
>  	if (ret < 0)
> @@ -142,18 +154,6 @@ static int usb_extcon_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>  
> -	info->edev = devm_extcon_dev_allocate(dev, usb_extcon_cable);
> -	if (IS_ERR(info->edev)) {
> -		dev_err(dev, "failed to allocate extcon device\n");
> -		return -ENOMEM;
> -	}
> -
> -	ret = devm_extcon_dev_register(dev, info->edev);
> -	if (ret < 0) {
> -		dev_err(dev, "failed to register extcon device\n");
> -		return ret;
> -	}
> -
>  	platform_set_drvdata(pdev, info);
>  	device_init_wakeup(dev, 1);
>  
> 

Re: [PATCH 2/4] extcon: usb-gpio: add support for VBUS detection

[Roger Quadros]

Hi Robert,

On 31/03/15 10:46, Robert Baldyga wrote:
> This patch adds VBUS pin detection support to extcon-usb-gpio driver.
> It allows to use this driver with boards which have both VBUS and ID
> pins, or only one of them.
> 
> Following table of states presents relationship between this signals
> and detected cable type:
> 
>  State        |    ID   |   VBUS
> ----------------------------------
>  [1] USB      |    H    |    H
>  [2] disconn. |    H    |    L
>  [3] USB-HOST |    L    |    H
>  [4] USB-HOST |    L    |    L
> 
> In case we have only one of these signals:
> - ID pin only: we can distinguish between states [1] and [3].
> - VBUS pin only: we can distinguish between states [1] and [2].
> 
> Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>
> ---
>  drivers/extcon/extcon-usb-gpio.c | 159 +++++++++++++++++++++++++++++----------
>  1 file changed, 118 insertions(+), 41 deletions(-)
> 
> diff --git a/drivers/extcon/extcon-usb-gpio.c b/drivers/extcon/extcon-usb-gpio.c
> index f6aa99d..02ff40e 100644
> --- a/drivers/extcon/extcon-usb-gpio.c
> +++ b/drivers/extcon/extcon-usb-gpio.c
> @@ -32,7 +32,9 @@ struct usb_extcon_info {
>  	struct extcon_dev *edev;
>  
>  	struct gpio_desc *id_gpiod;
> +	struct gpio_desc *vbus_gpiod;
>  	int id_irq;
> +	int vbus_irq;
>  
>  	unsigned long debounce_jiffies;
>  	struct delayed_work wq_detcable;
> @@ -52,21 +54,50 @@ static const char *usb_extcon_cable[] = {
>  	NULL,
>  };
>  
> +/*
> + *  State        |    ID   |   VBUS
> + * ----------------------------------
> + *  [1] USB      |    H    |    H
> + *  [2] disconn. |    H    |    L
> + *  [3] USB-HOST |    L    |    H
> + *  [4] USB-HOST |    L    |    L

This state table is misleading.
For the OTG case we're interested in ID and VBUS states as they are
without any decision making done by this driver.

The only state table you can have is
			ID	VBUS
	USB true	x	H
	USB false	x	L
	USB-HOST true	L	x
	USB-HOST false	H	x

So when both ID and VBUS pins are available, we must pass them
as is to the extcon framework.
	USB-HOST is ID status inverted
	USB is VBUS status

When only one pin is available we have to judge the other pin
state in the driver.

> + *
> + */
> +
>  static void usb_extcon_detect_cable(struct work_struct *work)
>  {
>  	int id;
> +	int vbus;
>  	struct usb_extcon_info *info = container_of(to_delayed_work(work),
>  						    struct usb_extcon_info,
>  						    wq_detcable);
>  
> -	/* check ID and update cable state */
> -	id = gpiod_get_value_cansleep(info->id_gpiod);
> -	if (id) {
> -		/*
> -		 * ID = 1 means USB HOST cable detached.
> -		 * As we don't have event for USB peripheral cable attached,
> -		 * we simulate USB peripheral attach here.
> -		 */
> +	/* check ID and VBUS and update cable state */
> +

Can we first tackle the case when both id_gpiod and vbus_gpiod are valid.
For that case you just need to pass on the states directly.
i.e.
	/* we have both ID and VBUS */
	if (info->id_gpiod && info->id_vbus) {
		EXTCON_CABLE_USB = vbus;
		EXTCON_CABLE_USB_HOST = !id;
	}

> +	/*
> +	 * If we don't have id pin, we want to detect states [1] and [2],
> +	 * so we set id to value 1.
> +	 */
> +	id = info->id_gpiod ?
> +		gpiod_get_value_cansleep(info->id_gpiod) : 1;
> +
> +	/*
> +	 * If we don't have vbus pin, we want to detect states [1] and [3],
> +	 * so we set vbus to value 1.
> +	 */
> +	vbus = info->vbus_gpiod ?
> +		gpiod_get_value_cansleep(info->vbus_gpiod) : 1;

All the below stuff can be simplified to

	/* we have only ID? */
	if (info->id_gpiod) {
		id = gpiod_get_value_cansleep(info->id_gpiod);
		if (id)
			EXCON_CABLE_USB_HOST = false;
			EXTCON_CABLE_USB = true;
		else
			EXTCON_CABLE_USB = false;
			EXTCON_CABLE_USB_HOST = true;

/* notice that we are first disabling the previously active state before enabling the next state */

	}

	/* we have only VBUS? */
	if (info->id_vbus) {
		vbus = gpiod_get_value_cansleep(info->vbus_gpiod);
		EXCON_CABLE_USB_HOST = false;
		EXTCON_CABLE_USB = vbus;
	}


> +
> +	if (id == 0) {
> +		/* ID = 0 means USB HOST cable attached. */
> +		extcon_set_cable_state(info->edev,
> +				       usb_extcon_cable[EXTCON_CABLE_USB],
> +				       false);

If we don't handle the case I mentioned above this will be wrong for
case id == 0, vbus == 1. We want CABLE_USB to be true in that case.

> +		extcon_set_cable_state(info->edev,
> +				       usb_extcon_cable[EXTCON_CABLE_USB_HOST],
> +				       true);


> +	} else if (vbus) {
> +		/* ID = 1 and VBUS = 1 means USB cable attached. */
>  		extcon_set_cable_state(info->edev,
>  				       usb_extcon_cable[EXTCON_CABLE_USB_HOST],
>  				       false);
> @@ -74,17 +105,13 @@ static void usb_extcon_detect_cable(struct work_struct *work)
>  				       usb_extcon_cable[EXTCON_CABLE_USB],
>  				       true);
>  	} else {
> -		/*
> -		 * ID = 0 means USB HOST cable attached.
> -		 * As we don't have event for USB peripheral cable detached,
> -		 * we simulate USB peripheral detach here.
> -		 */
> +		/* ID = 1 and VBUS = 0 means USB cable detached. */
>  		extcon_set_cable_state(info->edev,
>  				       usb_extcon_cable[EXTCON_CABLE_USB],
>  				       false);
>  		extcon_set_cable_state(info->edev,
>  				       usb_extcon_cable[EXTCON_CABLE_USB_HOST],
> -				       true);
> +				       false);
>  	}
>  }
>  
> @@ -113,10 +140,12 @@ static int usb_extcon_probe(struct platform_device *pdev)
>  		return -ENOMEM;
>  
>  	info->dev = dev;
> -	info->id_gpiod = devm_gpiod_get(&pdev->dev, "id");
> -	if (IS_ERR(info->id_gpiod)) {
> -		dev_err(dev, "failed to get ID GPIO\n");
> -		return PTR_ERR(info->id_gpiod);
> +	info->id_gpiod = devm_gpiod_get_optional(&pdev->dev, "id");
> +	info->vbus_gpiod = devm_gpiod_get_optional(&pdev->dev, "vbus");
> +
> +	if (!info->id_gpiod && !info->vbus_gpiod) {
> +		dev_err(dev, "failed to get gpios\n");
> +		return -ENODEV;
>  	}
>  
>  	info->edev = devm_extcon_dev_allocate(dev, usb_extcon_cable);
> @@ -131,27 +160,51 @@ static int usb_extcon_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>  
> -	ret = gpiod_set_debounce(info->id_gpiod,
> -				 USB_GPIO_DEBOUNCE_MS * 1000);
> +	if (info->id_gpiod)
> +		ret = gpiod_set_debounce(info->id_gpiod,
> +			USB_GPIO_DEBOUNCE_MS * 1000);
> +	if (!ret && info->vbus_gpiod) {
> +		ret = gpiod_set_debounce(info->vbus_gpiod,
> +			USB_GPIO_DEBOUNCE_MS * 1000);
> +		if (ret < 0 && info->id_gpiod)
> +			gpiod_set_debounce(info->vbus_gpiod, 0);
> +	}
>  	if (ret < 0)
>  		info->debounce_jiffies = msecs_to_jiffies(USB_GPIO_DEBOUNCE_MS);
>  
>  	INIT_DELAYED_WORK(&info->wq_detcable, usb_extcon_detect_cable);
>  
> -	info->id_irq = gpiod_to_irq(info->id_gpiod);
> -	if (info->id_irq < 0) {
> -		dev_err(dev, "failed to get ID IRQ\n");
> -		return info->id_irq;
> +	if (info->id_gpiod) {
> +		info->id_irq = gpiod_to_irq(info->id_gpiod);
> +		if (info->id_irq < 0) {
> +			dev_err(dev, "failed to get ID IRQ\n");
> +			return info->id_irq;
> +		}
> +		ret = devm_request_threaded_irq(dev, info->id_irq, NULL,
> +				usb_irq_handler,
> +				IRQF_TRIGGER_RISING |
> +				IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> +				pdev->name, info);
> +		if (ret < 0) {
> +			dev_err(dev, "failed to request handler for ID IRQ\n");
> +			return ret;
> +		}
>  	}
> -
> -	ret = devm_request_threaded_irq(dev, info->id_irq, NULL,
> -					usb_irq_handler,
> -					IRQF_TRIGGER_RISING |
> -					IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> -					pdev->name, info);
> -	if (ret < 0) {
> -		dev_err(dev, "failed to request handler for ID IRQ\n");
> -		return ret;
> +	if (info->vbus_gpiod) {
> +		info->vbus_irq = gpiod_to_irq(info->vbus_gpiod);
> +		if (info->vbus_irq < 0) {
> +			dev_err(dev, "failed to get VBUS IRQ\n");
> +			return info->vbus_irq;
> +		}
> +		ret = devm_request_threaded_irq(dev, info->vbus_irq, NULL,
> +				usb_irq_handler,
> +				IRQF_TRIGGER_RISING |
> +				IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> +				pdev->name, info);
> +		if (ret < 0) {
> +			dev_err(dev, "failed to request handler for VBUS IRQ\n");
> +			return ret;
> +		}
>  	}
>  
>  	platform_set_drvdata(pdev, info);
> @@ -179,9 +232,18 @@ static int usb_extcon_suspend(struct device *dev)
>  	int ret = 0;
>  
>  	if (device_may_wakeup(dev)) {
> -		ret = enable_irq_wake(info->id_irq);
> -		if (ret)
> -			return ret;
> +		if (info->id_gpiod) {
> +			ret = enable_irq_wake(info->id_irq);
> +			if (ret)
> +				return ret;
> +		}
> +		if (info->vbus_gpiod) {
> +			ret = enable_irq_wake(info->vbus_irq);
> +			if (ret) {
> +				disable_irq_wake(info->id_irq);

	if (info->id_gpiod)
		disable_irq_wake(info->id_irq);

> +				return ret;
> +			}
> +		}
>  	}
>  
>  	/*
> @@ -189,7 +251,10 @@ static int usb_extcon_suspend(struct device *dev)
>  	 * as GPIOs used behind I2C subsystem might not be
>  	 * accessible until resume completes. So disable IRQ.
>  	 */
> -	disable_irq(info->id_irq);
> +	if (info->id_gpiod)
> +		disable_irq(info->id_irq);
> +	if (info->vbus_gpiod)
> +		disable_irq(info->vbus_irq);
>  
>  	return ret;
>  }
> @@ -200,12 +265,24 @@ static int usb_extcon_resume(struct device *dev)
>  	int ret = 0;
>  
>  	if (device_may_wakeup(dev)) {
> -		ret = disable_irq_wake(info->id_irq);
> -		if (ret)
> -			return ret;
> +		if (info->id_gpiod) {
> +			ret = disable_irq_wake(info->id_irq);
> +			if (ret)
> +				return ret;
> +		}
> +		if (info->vbus_gpiod) {
> +			ret = disable_irq_wake(info->vbus_irq);
> +			if (ret) {
> +				enable_irq_wake(info->id_irq);

	if (info->id_gpiod)
		enable_irq_wake(info->id_irq);

> +				return ret;
> +			}
> +		}
>  	}
>  
> -	enable_irq(info->id_irq);
> +	if (info->id_gpiod)
> +		enable_irq(info->id_irq);
> +	if (info->vbus_gpiod)
> +		enable_irq(info->vbus_irq);
>  
>  	return ret;
>  }
> 

cheers,
-roger

Re: [PATCH 3/4] extcon: usb-gpio: make debounce value configurable in devicetree

[Roger Quadros]

On 31/03/15 10:46, Robert Baldyga wrote:
> This patch adds devicetree property for setting debounce value. It allows
> to set debounce time shorter or longer depending on the needs of given
> platform.
> 
> Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>
> ---
>  drivers/extcon/extcon-usb-gpio.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/extcon/extcon-usb-gpio.c b/drivers/extcon/extcon-usb-gpio.c
> index 02ff40e..86e3ece 100644
> --- a/drivers/extcon/extcon-usb-gpio.c
> +++ b/drivers/extcon/extcon-usb-gpio.c
> @@ -130,6 +130,7 @@ static int usb_extcon_probe(struct platform_device *pdev)
>  	struct device *dev = &pdev->dev;
>  	struct device_node *np = dev->of_node;
>  	struct usb_extcon_info *info;
> +	u32 debounce;
>  	int ret;
>  
>  	if (!np)
> @@ -140,6 +141,11 @@ static int usb_extcon_probe(struct platform_device *pdev)
>  		return -ENOMEM;
>  
>  	info->dev = dev;
> +
> +	ret = of_property_read_u32(np, "debounce", &debounce);
> +	if (ret < 0)
> +		debounce = USB_GPIO_DEBOUNCE_MS;
> +

Should we sanity check debounce value provided by device tree?
e.g. if it is greater than USB_GPIO_DEBOUNCE_MAX then we error out.
MAX could be 100 or 250ms.

>  	info->id_gpiod = devm_gpiod_get_optional(&pdev->dev, "id");
>  	info->vbus_gpiod = devm_gpiod_get_optional(&pdev->dev, "vbus");
>  
> @@ -161,16 +167,14 @@ static int usb_extcon_probe(struct platform_device *pdev)
>  	}
>  
>  	if (info->id_gpiod)
> -		ret = gpiod_set_debounce(info->id_gpiod,
> -			USB_GPIO_DEBOUNCE_MS * 1000);
> +		ret = gpiod_set_debounce(info->id_gpiod, debounce * 1000);
>  	if (!ret && info->vbus_gpiod) {
> -		ret = gpiod_set_debounce(info->vbus_gpiod,
> -			USB_GPIO_DEBOUNCE_MS * 1000);
> +		ret = gpiod_set_debounce(info->vbus_gpiod, debounce * 1000);
>  		if (ret < 0 && info->id_gpiod)
>  			gpiod_set_debounce(info->vbus_gpiod, 0);
>  	}
>  	if (ret < 0)
> -		info->debounce_jiffies = msecs_to_jiffies(USB_GPIO_DEBOUNCE_MS);
> +		info->debounce_jiffies = msecs_to_jiffies(debounce);
>  
>  	INIT_DELAYED_WORK(&info->wq_detcable, usb_extcon_detect_cable);
>  
> 

cheers,
-roger

Re: [PATCH 4/4] Documentation: extcon: usb-gpio: update usb-gpio binding description

[Roger Quadros]

On 31/03/15 10:46, Robert Baldyga wrote:
> Add information about VBUS pin detection support, 'debounce' property
> and some other details.
> 
> Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>
> ---
>  .../devicetree/bindings/extcon/extcon-usb-gpio.txt | 23 ++++++++++++++++++++--
>  1 file changed, 21 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/extcon/extcon-usb-gpio.txt b/Documentation/devicetree/bindings/extcon/extcon-usb-gpio.txt
> index af0b903..d3fcf8b 100644
> --- a/Documentation/devicetree/bindings/extcon/extcon-usb-gpio.txt
> +++ b/Documentation/devicetree/bindings/extcon/extcon-usb-gpio.txt
> @@ -1,16 +1,35 @@
>  USB GPIO Extcon device
>  
> -This is a virtual device used to generate USB cable states from the USB ID pin
> -connected to a GPIO pin.
> +This is a virtual device used to generate USB cable states from the USB
> +ID and VBUS signals connected to a GPIO pins.

s/to a GPIO/to GPIO/

> +
> +Some devices has only one of these GPIO pins, so we support cases when
s/has/have/

> +only one of them is present. Hence properties 'id-gpio' and 'vbus-gpio'
> +are described as optional, but at least one of them has to be present
> +in extcon-usb-gpio node.
> +
> +In general we have three cases:
> + 1. We have both VBUS and ID pin detection - we can detect USB, USB-HOST
> +    and cable disconnection.

The interpretation of "cable disconnect" might not be always true.
ID may be 1 and VBUS 0 but cable might still not be disconnected.
e.g. if both are OTG devices.
That's why we have ADP to detect cable connect/disconnect status for OTG case.

So let's leave cable disconnection interpretation to the USB stack and
just deal with passing ID/VBUS status. I must admit that the extcon cable
state names are misleading. They should really have been named
USB-ID and USB-VBUS :).

The driver doesn't do connect/disconnect detection but only infers the other
pin state if only one of the ID/VBUS is available.

> + 2. We have only VBUS detection - we can detect USB and cable disconnection.
> + 3. We have ID pin only - we can distinguish between USB and USB-HOST
> +    but without ability to detect cable disconnection.

how about rewording these 3 points like so with a short header about
clarification of extcon USB/USB_HOST states.

The extcon cable states USB and USB_HOST are actually VBUS and (inverted) ID
pin states and do not indicate what mode the USB needs to operate in.
That decision is done by the USB stack.

1. If VBUS and ID gpios are present we pass them as is
	USB-HOST = !ID, USB = VBUS
2. If only VBUS gpio is present we assume that ID pin is always High.
	USB-HOST = false, USB = VBUS.
3. If only ID pin is available we infer the VBUS pin states based on ID.
	USB-HOST = !ID, USB = ID

>  
>  Required properties:
>  - compatible: Should be "linux,extcon-usb-gpio"
> +
> +Optional properties
>  - id-gpio: gpio for USB ID pin. See gpio binding.
> +- vbus-gpio: gpio for USB VBUS pin. See gpio binding.
> +- debounce: gpio debounce time in milliseconds (u32).
> +
>  
>  Example: Examples of extcon-usb-gpio node in dra7-evm.dts as listed below:
>  	extcon_usb1 {
>  		compatible = "linux,extcon-usb-gpio";
>  		id-gpio = <&gpio6 1 GPIO_ACTIVE_HIGH>;
> +		vbus-gpio = <&gpio6 2 GPIO_ACTIVE_HIGH>;
> +		debounce = <25>;
>  	}
>  
>  	&omap_dwc3_1 {
>
 
cheers,
-roger

Re: [PATCH 4/4] Documentation: extcon: usb-gpio: update usb-gpio binding description

[Robert Baldyga]

Hi,

On 03/31/2015 12:20 PM, Roger Quadros wrote:
> On 31/03/15 10:46, Robert Baldyga wrote:
>> Add information about VBUS pin detection support, 'debounce' property
>> and some other details.
>>
>> Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>
>> ---
>>  .../devicetree/bindings/extcon/extcon-usb-gpio.txt | 23 ++++++++++++++++++++--
>>  1 file changed, 21 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/extcon/extcon-usb-gpio.txt b/Documentation/devicetree/bindings/extcon/extcon-usb-gpio.txt
>> index af0b903..d3fcf8b 100644
>> --- a/Documentation/devicetree/bindings/extcon/extcon-usb-gpio.txt
>> +++ b/Documentation/devicetree/bindings/extcon/extcon-usb-gpio.txt
>> @@ -1,16 +1,35 @@
>>  USB GPIO Extcon device
>>  
>> -This is a virtual device used to generate USB cable states from the USB ID pin
>> -connected to a GPIO pin.
>> +This is a virtual device used to generate USB cable states from the USB
>> +ID and VBUS signals connected to a GPIO pins.
> 
> s/to a GPIO/to GPIO/
> 
>> +
>> +Some devices has only one of these GPIO pins, so we support cases when
> s/has/have/
> 
>> +only one of them is present. Hence properties 'id-gpio' and 'vbus-gpio'
>> +are described as optional, but at least one of them has to be present
>> +in extcon-usb-gpio node.
>> +
>> +In general we have three cases:
>> + 1. We have both VBUS and ID pin detection - we can detect USB, USB-HOST
>> +    and cable disconnection.
> 
> The interpretation of "cable disconnect" might not be always true.
> ID may be 1 and VBUS 0 but cable might still not be disconnected.
> e.g. if both are OTG devices.
> That's why we have ADP to detect cable connect/disconnect status for OTG case.
> 
> So let's leave cable disconnection interpretation to the USB stack and
> just deal with passing ID/VBUS status. I must admit that the extcon cable
> state names are misleading. They should really have been named
> USB-ID and USB-VBUS :).

I thought the same.

Chanwoo, what do you think about such naming convention change? USB
cable detection in general is needed mainly for OTG, so having cable
state names clearly related to OTG statemachine states seems to be good
idea.

> 
> The driver doesn't do connect/disconnect detection but only infers the other
> pin state if only one of the ID/VBUS is available.
> 
>> + 2. We have only VBUS detection - we can detect USB and cable disconnection.
>> + 3. We have ID pin only - we can distinguish between USB and USB-HOST
>> +    but without ability to detect cable disconnection.
> 
> how about rewording these 3 points like so with a short header about
> clarification of extcon USB/USB_HOST states.
> 
> The extcon cable states USB and USB_HOST are actually VBUS and (inverted) ID
> pin states and do not indicate what mode the USB needs to operate in.
> That decision is done by the USB stack.
> 
> 1. If VBUS and ID gpios are present we pass them as is
> 	USB-HOST = !ID, USB = VBUS
> 2. If only VBUS gpio is present we assume that ID pin is always High.
> 	USB-HOST = false, USB = VBUS.
> 3. If only ID pin is available we infer the VBUS pin states based on ID.
> 	USB-HOST = !ID, USB = ID
> 

Thanks for comments. I will try to fix it up.

Best regards,
Robert Baldyga

Re: [PATCH 3/4] extcon: usb-gpio: make debounce value configurable in devicetree

[Robert Baldyga]

On 03/31/2015 11:30 AM, Roger Quadros wrote:
> On 31/03/15 10:46, Robert Baldyga wrote:
>> This patch adds devicetree property for setting debounce value. It allows
>> to set debounce time shorter or longer depending on the needs of given
>> platform.
>>
>> Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>
>> ---
>>  drivers/extcon/extcon-usb-gpio.c | 14 +++++++++-----
>>  1 file changed, 9 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/extcon/extcon-usb-gpio.c b/drivers/extcon/extcon-usb-gpio.c
>> index 02ff40e..86e3ece 100644
>> --- a/drivers/extcon/extcon-usb-gpio.c
>> +++ b/drivers/extcon/extcon-usb-gpio.c
>> @@ -130,6 +130,7 @@ static int usb_extcon_probe(struct platform_device *pdev)
>>  	struct device *dev = &pdev->dev;
>>  	struct device_node *np = dev->of_node;
>>  	struct usb_extcon_info *info;
>> +	u32 debounce;
>>  	int ret;
>>  
>>  	if (!np)
>> @@ -140,6 +141,11 @@ static int usb_extcon_probe(struct platform_device *pdev)
>>  		return -ENOMEM;
>>  
>>  	info->dev = dev;
>> +
>> +	ret = of_property_read_u32(np, "debounce", &debounce);
>> +	if (ret < 0)
>> +		debounce = USB_GPIO_DEBOUNCE_MS;
>> +
> 
> Should we sanity check debounce value provided by device tree?
> e.g. if it is greater than USB_GPIO_DEBOUNCE_MAX then we error out.
> MAX could be 100 or 250ms.
> 

None of extcon drivers do such thing, so I suppose it's up to devicetree
to supply reasonable value here.

>>  	info->id_gpiod = devm_gpiod_get_optional(&pdev->dev, "id");
>>  	info->vbus_gpiod = devm_gpiod_get_optional(&pdev->dev, "vbus");
>>  
>> @@ -161,16 +167,14 @@ static int usb_extcon_probe(struct platform_device *pdev)
>>  	}
>>  
>>  	if (info->id_gpiod)
>> -		ret = gpiod_set_debounce(info->id_gpiod,
>> -			USB_GPIO_DEBOUNCE_MS * 1000);
>> +		ret = gpiod_set_debounce(info->id_gpiod, debounce * 1000);
>>  	if (!ret && info->vbus_gpiod) {
>> -		ret = gpiod_set_debounce(info->vbus_gpiod,
>> -			USB_GPIO_DEBOUNCE_MS * 1000);
>> +		ret = gpiod_set_debounce(info->vbus_gpiod, debounce * 1000);
>>  		if (ret < 0 && info->id_gpiod)
>>  			gpiod_set_debounce(info->vbus_gpiod, 0);
>>  	}
>>  	if (ret < 0)
>> -		info->debounce_jiffies = msecs_to_jiffies(USB_GPIO_DEBOUNCE_MS);
>> +		info->debounce_jiffies = msecs_to_jiffies(debounce);
>>  
>>  	INIT_DELAYED_WORK(&info->wq_detcable, usb_extcon_detect_cable);
>>  
>>

Best regards,
Robert Baldyga