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

[PATCH v2 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

Changelog:
v2:
- addressed Roger's comments

v1: https://lkml.org/lkml/2015/3/31/96

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 |  28 +++-
 drivers/extcon/extcon-usb-gpio.c                   | 183 ++++++++++++++-------
 2 files changed, 153 insertions(+), 58 deletions(-)

-- 
1.9.1

[PATCH v2 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>
Acked-by: Roger Quadros <rogerq@ti.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 v2 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] none           |    H    |    L
 [3] USB & USB-HOST |    L    |    H
 [4] USB-HOST       |    L    |    L

In case we have only one of these signals:
- VBUS only - we want to distinguish between [1] and [2], so ID is always 1.
- ID only - we want to distinguish between [1] and [4], so VBUS = ID.

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

diff --git a/drivers/extcon/extcon-usb-gpio.c b/drivers/extcon/extcon-usb-gpio.c
index f6aa99d..c842715 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,40 +54,49 @@ static const char *usb_extcon_cable[] = {
 	NULL,
 };
 
+/*
+ * "USB" = VBUS and "USB-HOST" = !ID, so we have:
+ *
+ *  State              |    ID   |   VBUS
+ * ----------------------------------------
+ *  [1] USB            |    H    |    H
+ *  [2] none           |    H    |    L
+ *  [3] USB & USB-HOST |    L    |    H
+ *  [4] USB-HOST       |    L    |    L
+ *
+ * In case we have only one of these signals:
+ * - VBUS only - we want to distinguish between [1] and [2], so ID is always 1.
+ * - ID only - we want to distinguish between [1] and [4], so VBUS = ID.
+ */
+
 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.
-		 */
-		extcon_set_cable_state(info->edev,
-				       usb_extcon_cable[EXTCON_CABLE_USB_HOST],
-				       false);
-		extcon_set_cable_state(info->edev,
-				       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.
-		 */
+	/* check ID and VBUS and update cable state */
+
+	id = info->id_gpiod ?
+		gpiod_get_value_cansleep(info->id_gpiod) : 1;
+
+	vbus = info->vbus_gpiod ?
+		gpiod_get_value_cansleep(info->vbus_gpiod) : id;
+
+	/* at first we clean states which are no longer active */
+	if (id)
 		extcon_set_cable_state(info->edev,
-				       usb_extcon_cable[EXTCON_CABLE_USB],
-				       false);
+			usb_extcon_cable[EXTCON_CABLE_USB_HOST], false);
+	if (!vbus)
 		extcon_set_cable_state(info->edev,
-				       usb_extcon_cable[EXTCON_CABLE_USB_HOST],
-				       true);
-	}
+			usb_extcon_cable[EXTCON_CABLE_USB], false);
+
+	extcon_set_cable_state(info->edev,
+		usb_extcon_cable[EXTCON_CABLE_USB_HOST], !id);
+	extcon_set_cable_state(info->edev,
+		usb_extcon_cable[EXTCON_CABLE_USB], vbus);
 }
 
 static irqreturn_t usb_irq_handler(int irq, void *dev_id)
@@ -113,10 +124,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 +144,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 +216,16 @@ 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)
+				goto err;
+		}
 	}
 
 	/*
@@ -189,8 +233,16 @@ 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;
 
+err:
+	if (info->id_gpiod)
+		disable_irq_wake(info->id_irq);
 	return ret;
 }
 
@@ -200,13 +252,28 @@ 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)
+				goto err;
+		}
 	}
 
-	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;
 
+err:
+	if (info->id_gpiod)
+		enable_irq_wake(info->id_irq);
 	return ret;
 }
 #endif
-- 
1.9.1

[PATCH v2 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 c842715..439ca99 100644
--- a/drivers/extcon/extcon-usb-gpio.c
+++ b/drivers/extcon/extcon-usb-gpio.c
@@ -114,6 +114,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)
@@ -124,6 +125,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");
 
@@ -145,16 +151,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 v2 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 | 28 ++++++++++++++++++++--
 1 file changed, 26 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..7096f39 100644
--- a/Documentation/devicetree/bindings/extcon/extcon-usb-gpio.txt
+++ b/Documentation/devicetree/bindings/extcon/extcon-usb-gpio.txt
@@ -1,16 +1,40 @@
 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 GPIO pins.
+
+The extcon cable states USB and USB_HOST are actually VBUS and !ID
+pin states and do not indicate what mode the USB needs to operate in.
+That decision is done by the USB stack.
+
+Some devices have 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. 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 {
-- 
1.9.1

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

[Roger Quadros]

Robert,

On 01/04/15 10:23, 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] none           |    H    |    L
>  [3] USB & USB-HOST |    L    |    H
>  [4] USB-HOST       |    L    |    L
> 
> In case we have only one of these signals:
> - VBUS only - we want to distinguish between [1] and [2], so ID is always 1.
> - ID only - we want to distinguish between [1] and [4], so VBUS = ID.
> 
> Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>
> ---
>  drivers/extcon/extcon-usb-gpio.c | 171 +++++++++++++++++++++++++++------------
>  1 file changed, 119 insertions(+), 52 deletions(-)
> 
> diff --git a/drivers/extcon/extcon-usb-gpio.c b/drivers/extcon/extcon-usb-gpio.c
> index f6aa99d..c842715 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,40 +54,49 @@ static const char *usb_extcon_cable[] = {
>  	NULL,
>  };
>  
> +/*
> + * "USB" = VBUS and "USB-HOST" = !ID, so we have:
> + *
> + *  State              |    ID   |   VBUS
> + * ----------------------------------------
> + *  [1] USB            |    H    |    H
> + *  [2] none           |    H    |    L
> + *  [3] USB & USB-HOST |    L    |    H
> + *  [4] USB-HOST       |    L    |    L
> + *
> + * In case we have only one of these signals:
> + * - VBUS only - we want to distinguish between [1] and [2], so ID is always 1.
> + * - ID only - we want to distinguish between [1] and [4], so VBUS = ID.
> + */
> +
>  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.
> -		 */
> -		extcon_set_cable_state(info->edev,
> -				       usb_extcon_cable[EXTCON_CABLE_USB_HOST],
> -				       false);
> -		extcon_set_cable_state(info->edev,
> -				       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.
> -		 */
> +	/* check ID and VBUS and update cable state */
> +
> +	id = info->id_gpiod ?
> +		gpiod_get_value_cansleep(info->id_gpiod) : 1;
> +
> +	vbus = info->vbus_gpiod ?
> +		gpiod_get_value_cansleep(info->vbus_gpiod) : id;
> +
> +	/* at first we clean states which are no longer active */
> +	if (id)
>  		extcon_set_cable_state(info->edev,
> -				       usb_extcon_cable[EXTCON_CABLE_USB],
> -				       false);
> +			usb_extcon_cable[EXTCON_CABLE_USB_HOST], false);
> +	if (!vbus)
>  		extcon_set_cable_state(info->edev,
> -				       usb_extcon_cable[EXTCON_CABLE_USB_HOST],
> -				       true);
> -	}
> +			usb_extcon_cable[EXTCON_CABLE_USB], false);
> +
> +	extcon_set_cable_state(info->edev,
> +		usb_extcon_cable[EXTCON_CABLE_USB_HOST], !id);
> +	extcon_set_cable_state(info->edev,
> +		usb_extcon_cable[EXTCON_CABLE_USB], vbus);

This approach has the following problems:
1) If ID is 1 then extcon_set_cable_state(USB_HOST, false) gets called twice.
2) If VBUS is 0 then extcon_set_cable_state(USB, false) gets called twice.
3) When both ID and VBUS are available, even if either one changes state then we unnecessarily
update the other pins cable state as well.

This is probably not an issue as extcon core might be ignoring duplicate set_cable_states,
but I think we should avoid them if we can. I wouldn't mind (3) as we unnecessarily need to
keep track of previous states, but (1) and (2) should be fixed.

cheers,
-roger

>  }
>  
>  static irqreturn_t usb_irq_handler(int irq, void *dev_id)
> @@ -113,10 +124,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 +144,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 +216,16 @@ 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)
> +				goto err;
> +		}
>  	}
>  
>  	/*
> @@ -189,8 +233,16 @@ 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;
>  
> +err:
> +	if (info->id_gpiod)
> +		disable_irq_wake(info->id_irq);
>  	return ret;
>  }
>  
> @@ -200,13 +252,28 @@ 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)
> +				goto err;
> +		}
>  	}
>  
> -	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;
>  
> +err:
> +	if (info->id_gpiod)
> +		enable_irq_wake(info->id_irq);
>  	return ret;
>  }
>  #endif
> 

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

[Roger Quadros]

On 01/04/15 10:23, 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>

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

cheers,
-roger

> ---
>  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 c842715..439ca99 100644
> --- a/drivers/extcon/extcon-usb-gpio.c
> +++ b/drivers/extcon/extcon-usb-gpio.c
> @@ -114,6 +114,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)
> @@ -124,6 +125,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");
>  
> @@ -145,16 +151,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);
>  
> 

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

[Roger Quadros]

On 01/04/15 10:23, 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>

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

cheers,
-roger

> ---
>  .../devicetree/bindings/extcon/extcon-usb-gpio.txt | 28 ++++++++++++++++++++--
>  1 file changed, 26 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..7096f39 100644
> --- a/Documentation/devicetree/bindings/extcon/extcon-usb-gpio.txt
> +++ b/Documentation/devicetree/bindings/extcon/extcon-usb-gpio.txt
> @@ -1,16 +1,40 @@
>  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 GPIO pins.
> +
> +The extcon cable states USB and USB_HOST are actually VBUS and !ID
> +pin states and do not indicate what mode the USB needs to operate in.
> +That decision is done by the USB stack.
> +
> +Some devices have 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. 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 {
> 

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

[Robert Baldyga]

Hi Roger,

On 04/01/2015 01:28 PM, Roger Quadros wrote:
> Robert,
> 
> On 01/04/15 10:23, 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] none           |    H    |    L
>>  [3] USB & USB-HOST |    L    |    H
>>  [4] USB-HOST       |    L    |    L
>>
>> In case we have only one of these signals:
>> - VBUS only - we want to distinguish between [1] and [2], so ID is always 1.
>> - ID only - we want to distinguish between [1] and [4], so VBUS = ID.
>>
>> Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>
>> ---
>>  drivers/extcon/extcon-usb-gpio.c | 171 +++++++++++++++++++++++++++------------
>>  1 file changed, 119 insertions(+), 52 deletions(-)
>>
>> diff --git a/drivers/extcon/extcon-usb-gpio.c b/drivers/extcon/extcon-usb-gpio.c
>> index f6aa99d..c842715 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,40 +54,49 @@ static const char *usb_extcon_cable[] = {
>>  	NULL,
>>  };
>>  
>> +/*
>> + * "USB" = VBUS and "USB-HOST" = !ID, so we have:
>> + *
>> + *  State              |    ID   |   VBUS
>> + * ----------------------------------------
>> + *  [1] USB            |    H    |    H
>> + *  [2] none           |    H    |    L
>> + *  [3] USB & USB-HOST |    L    |    H
>> + *  [4] USB-HOST       |    L    |    L
>> + *
>> + * In case we have only one of these signals:
>> + * - VBUS only - we want to distinguish between [1] and [2], so ID is always 1.
>> + * - ID only - we want to distinguish between [1] and [4], so VBUS = ID.
>> + */
>> +
>>  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.
>> -		 */
>> -		extcon_set_cable_state(info->edev,
>> -				       usb_extcon_cable[EXTCON_CABLE_USB_HOST],
>> -				       false);
>> -		extcon_set_cable_state(info->edev,
>> -				       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.
>> -		 */
>> +	/* check ID and VBUS and update cable state */
>> +
>> +	id = info->id_gpiod ?
>> +		gpiod_get_value_cansleep(info->id_gpiod) : 1;
>> +
>> +	vbus = info->vbus_gpiod ?
>> +		gpiod_get_value_cansleep(info->vbus_gpiod) : id;
>> +
>> +	/* at first we clean states which are no longer active */
>> +	if (id)
>>  		extcon_set_cable_state(info->edev,
>> -				       usb_extcon_cable[EXTCON_CABLE_USB],
>> -				       false);
>> +			usb_extcon_cable[EXTCON_CABLE_USB_HOST], false);
>> +	if (!vbus)
>>  		extcon_set_cable_state(info->edev,
>> -				       usb_extcon_cable[EXTCON_CABLE_USB_HOST],
>> -				       true);
>> -	}
>> +			usb_extcon_cable[EXTCON_CABLE_USB], false);
>> +
>> +	extcon_set_cable_state(info->edev,
>> +		usb_extcon_cable[EXTCON_CABLE_USB_HOST], !id);
>> +	extcon_set_cable_state(info->edev,
>> +		usb_extcon_cable[EXTCON_CABLE_USB], vbus);
> 
> This approach has the following problems:
> 1) If ID is 1 then extcon_set_cable_state(USB_HOST, false) gets called twice.
> 2) If VBUS is 0 then extcon_set_cable_state(USB, false) gets called twice.
> 3) When both ID and VBUS are available, even if either one changes state then we unnecessarily
> update the other pins cable state as well.
> 
> This is probably not an issue as extcon core might be ignoring duplicate set_cable_states,
> but I think we should avoid them if we can. I wouldn't mind (3) as we unnecessarily need to
> keep track of previous states, but (1) and (2) should be fixed.
> 

Extcon core is responsible for storing current cable state to let us do
not worry about that. However if we really do want to get rid of
redundant usb_extcon_cable() calls, we can create separate interrupt
handler for VBUS.

Br,
Robert Baldyga

> 
>>  }
>>  
>>  static irqreturn_t usb_irq_handler(int irq, void *dev_id)
>> @@ -113,10 +124,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 +144,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 +216,16 @@ 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)
>> +				goto err;
>> +		}
>>  	}
>>  
>>  	/*
>> @@ -189,8 +233,16 @@ 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;
>>  
>> +err:
>> +	if (info->id_gpiod)
>> +		disable_irq_wake(info->id_irq);
>>  	return ret;
>>  }
>>  
>> @@ -200,13 +252,28 @@ 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)
>> +				goto err;
>> +		}
>>  	}
>>  
>> -	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;
>>  
>> +err:
>> +	if (info->id_gpiod)
>> +		enable_irq_wake(info->id_irq);
>>  	return ret;
>>  }
>>  #endif
>>
> 
> 

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

[Roger Quadros]

Hi Chanwoo,

On 01/04/15 10:23, 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>

Can you please pick this patch for 4.1 (or 4.1-rc) as it fixes a bug. Thanks.

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 v2 1/4] extcon: usb-gpio: register extcon device before IRQ registration

[Chanwoo Choi]

Hi Roger,

On 04/01/2015 08:58 PM, Roger Quadros wrote:
> Hi Chanwoo,
> 
> On 01/04/15 10:23, 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>
> 
> Can you please pick this patch for 4.1 (or 4.1-rc) as it fixes a bug. Thanks.

I already sent pull request for v4.1 and merged it.
I'll send this patch for 4.1-rc.

Thanks,
Chanwoo Choi

> 
> 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 v2 2/4] extcon: usb-gpio: add support for VBUS detection

[Roger Quadros]

Robert,

On 01/04/15 14:49, Robert Baldyga wrote:
> Hi Roger,
> 
> On 04/01/2015 01:28 PM, Roger Quadros wrote:
>> Robert,
>>
>> On 01/04/15 10:23, 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] none           |    H    |    L
>>>  [3] USB & USB-HOST |    L    |    H
>>>  [4] USB-HOST       |    L    |    L
>>>
>>> In case we have only one of these signals:
>>> - VBUS only - we want to distinguish between [1] and [2], so ID is always 1.
>>> - ID only - we want to distinguish between [1] and [4], so VBUS = ID.
>>>
>>> Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>
>>> ---
>>>  drivers/extcon/extcon-usb-gpio.c | 171 +++++++++++++++++++++++++++------------
>>>  1 file changed, 119 insertions(+), 52 deletions(-)
>>>
>>> diff --git a/drivers/extcon/extcon-usb-gpio.c b/drivers/extcon/extcon-usb-gpio.c
>>> index f6aa99d..c842715 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,40 +54,49 @@ static const char *usb_extcon_cable[] = {
>>>  	NULL,
>>>  };
>>>  
>>> +/*
>>> + * "USB" = VBUS and "USB-HOST" = !ID, so we have:
>>> + *
>>> + *  State              |    ID   |   VBUS
>>> + * ----------------------------------------
>>> + *  [1] USB            |    H    |    H
>>> + *  [2] none           |    H    |    L
>>> + *  [3] USB & USB-HOST |    L    |    H
>>> + *  [4] USB-HOST       |    L    |    L
>>> + *
>>> + * In case we have only one of these signals:
>>> + * - VBUS only - we want to distinguish between [1] and [2], so ID is always 1.
>>> + * - ID only - we want to distinguish between [1] and [4], so VBUS = ID.
>>> + */
>>> +
>>>  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.
>>> -		 */
>>> -		extcon_set_cable_state(info->edev,
>>> -				       usb_extcon_cable[EXTCON_CABLE_USB_HOST],
>>> -				       false);
>>> -		extcon_set_cable_state(info->edev,
>>> -				       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.
>>> -		 */
>>> +	/* check ID and VBUS and update cable state */
>>> +
>>> +	id = info->id_gpiod ?
>>> +		gpiod_get_value_cansleep(info->id_gpiod) : 1;
>>> +
>>> +	vbus = info->vbus_gpiod ?
>>> +		gpiod_get_value_cansleep(info->vbus_gpiod) : id;
>>> +
>>> +	/* at first we clean states which are no longer active */
>>> +	if (id)
>>>  		extcon_set_cable_state(info->edev,
>>> -				       usb_extcon_cable[EXTCON_CABLE_USB],
>>> -				       false);
>>> +			usb_extcon_cable[EXTCON_CABLE_USB_HOST], false);
>>> +	if (!vbus)
>>>  		extcon_set_cable_state(info->edev,
>>> -				       usb_extcon_cable[EXTCON_CABLE_USB_HOST],
>>> -				       true);
>>> -	}
>>> +			usb_extcon_cable[EXTCON_CABLE_USB], false);
>>> +
>>> +	extcon_set_cable_state(info->edev,
>>> +		usb_extcon_cable[EXTCON_CABLE_USB_HOST], !id);
>>> +	extcon_set_cable_state(info->edev,
>>> +		usb_extcon_cable[EXTCON_CABLE_USB], vbus);
>>
>> This approach has the following problems:
>> 1) If ID is 1 then extcon_set_cable_state(USB_HOST, false) gets called twice.
>> 2) If VBUS is 0 then extcon_set_cable_state(USB, false) gets called twice.
>> 3) When both ID and VBUS are available, even if either one changes state then we unnecessarily
>> update the other pins cable state as well.
>>
>> This is probably not an issue as extcon core might be ignoring duplicate set_cable_states,
>> but I think we should avoid them if we can. I wouldn't mind (3) as we unnecessarily need to
>> keep track of previous states, but (1) and (2) should be fixed.
>>
> 
> Extcon core is responsible for storing current cable state to let us do
> not worry about that. However if we really do want to get rid of
> redundant usb_extcon_cable() calls, we can create separate interrupt
> handler for VBUS.

I wouldn't bother for a separate handler. You can still fix (1) and (2) without a
separate handler.

cheers,
-roger

> 
>>
>>>  }
>>>  
>>>  static irqreturn_t usb_irq_handler(int irq, void *dev_id)
>>> @@ -113,10 +124,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 +144,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 +216,16 @@ 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)
>>> +				goto err;
>>> +		}
>>>  	}
>>>  
>>>  	/*
>>> @@ -189,8 +233,16 @@ 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;
>>>  
>>> +err:
>>> +	if (info->id_gpiod)
>>> +		disable_irq_wake(info->id_irq);
>>>  	return ret;
>>>  }
>>>  
>>> @@ -200,13 +252,28 @@ 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)
>>> +				goto err;
>>> +		}
>>>  	}
>>>  
>>> -	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;
>>>  
>>> +err:
>>> +	if (info->id_gpiod)
>>> +		enable_irq_wake(info->id_irq);
>>>  	return ret;
>>>  }
>>>  #endif
>>>
>>
>>
>