[PATCH v3 0/3] Add Generic USB VBUS/ID detection via GPIO using extcon

[PATCH v3 0/3] Add Generic USB VBUS/ID detection via GPIO using extcon

[George Cherian]

Hi,

These patches add generic support for USB VBUS/ID pin detection using extcon framework.
The USB ID pin on DRA7xx is connected via the gpio expander pcf8575.
The interrupt line of the same is connected to the gpio 11 of bank 6.
The following driver relies on  the  gpio interrupt to notify the actual
ID pin values by which the dwc3 driver determines the HOST/Peripheral roles.


These patches are on top of following merges.
v3.11-rc3 dra7 baseport tree [1] , with balbi/next ,control-usb multi instance support [2],
chanwoo/extcon-next, roger's patches for USB host adaptation[3],Laurent Pinchart's patch to
Enable pcf857x GPIO expander for Device Tree[4] and pcf857x cleanup patch [5].

Patches are available at
extcon_gpio_usbvid-v1
in git tree
git://git.ti.com/~georgecherian/ti-linux-kernel/georgec-connectivity-linux-feature-tree.git

[1] - dra7 base tree
        https://github.com/lokeshvutla/linux/tree/dra7-3.11-rc3-base
[2] - multiple control-usb instances
        https://github.com/rogerq/linux/tree/usb-control-module
[3] - [PATCH 0/4] ARM: DRA7-evm: USB host adaptation
        https://lkml.org/lkml/2013/8/1/335
[4] - [PATCH v5] gpio: pcf857x: Add OF support
        https://lkml.org/lkml/2013/8/27/70
[5] - [PATCH] gpio: pcf857x: cleanup irq_demux_work and use threaded irq
        https://lkml.org/lkml/2013/8/27/207

George Cherian (3):
  extcon: extcon-gpio-usbvid: Generic USB VBUS/ID detection via GPIO
  drivers: Makefile: Extcon is a framework so bump it up
  ARM: dts: dra7-evm: Add extcon nodes for USB ID pin detection

 .../bindings/extcon/extcon-gpio-usbvid.txt         |  20 ++
 arch/arm/boot/dts/dra7-evm.dts                     |  50 +++-
 drivers/Makefile                                   |   2 +-
 drivers/extcon/Kconfig                             |   6 +
 drivers/extcon/Makefile                            |   1 +
 drivers/extcon/extcon-gpio-usbvid.c                | 286 +++++++++++++++++++++
 6 files changed, 363 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/extcon/extcon-gpio-usbvid.txt
 create mode 100644 drivers/extcon/extcon-gpio-usbvid.c

-- 
1.8.1.4

[PATCH v3 1/3] extcon: extcon-gpio-usbvid: Generic USB VBUS/ID detection via GPIO

[George Cherian]

Add a generic USB VBUS/ID detection EXTCON driver. This driver expects
the ID/VBUS pin are connected via GPIOs. This driver is tested on
DRA7x board were the ID pin is routed via GPIOs. The driver supports
both VBUS and ID pin configuration and ID pin only configuration.

Signed-off-by: George Cherian <george.cherian@ti.com>
---
 .../bindings/extcon/extcon-gpio-usbvid.txt         |  20 ++
 drivers/extcon/Kconfig                             |   6 +
 drivers/extcon/Makefile                            |   1 +
 drivers/extcon/extcon-gpio-usbvid.c                | 286 +++++++++++++++++++++
 4 files changed, 313 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/extcon/extcon-gpio-usbvid.txt
 create mode 100644 drivers/extcon/extcon-gpio-usbvid.c

diff --git a/Documentation/devicetree/bindings/extcon/extcon-gpio-usbvid.txt b/Documentation/devicetree/bindings/extcon/extcon-gpio-usbvid.txt
new file mode 100644
index 0000000..eea0741
--- /dev/null
+++ b/Documentation/devicetree/bindings/extcon/extcon-gpio-usbvid.txt
@@ -0,0 +1,20 @@
+EXTCON FOR USB VIA GPIO
+
+Required Properties:
+ - compatible : Should be "ti,gpio-usb-vid" for USB VBUS-ID detector
+		using gpios or "ti,gpio-usb-id" for USB ID pin detector
+ - gpios : phandle and args ID pin gpio and VBUS gpio.
+	   The first gpio used  for ID pin detection
+	   and the second used for VBUS detection.
+	   ID pin gpio is mandatory and VBUS is optional
+	   depending on implementation.
+
+Please refer to ../gpio/gpio.txt for details of the common GPIO bindings
+
+Example:
+
+	gpio_usbvid_extcon1 {
+		compatible = "ti,gpio-usb-vid";
+		gpios = <&gpio1 1 0>,
+			<&gpio2 2 0>;
+	};
diff --git a/drivers/extcon/Kconfig b/drivers/extcon/Kconfig
index f1d54a3..8097398 100644
--- a/drivers/extcon/Kconfig
+++ b/drivers/extcon/Kconfig
@@ -64,4 +64,10 @@ config EXTCON_PALMAS
 	  Say Y here to enable support for USB peripheral and USB host
 	  detection by palmas usb.
 
+config EXTCON_GPIO_USBVID
+	tristate "Generic USB VBUS/ID detection using GPIO EXTCON support"
+	help
+	  Say Y here to enable support for USB VBUS/ID deetction by GPIO.
+
+
 endif # MULTISTATE_SWITCH
diff --git a/drivers/extcon/Makefile b/drivers/extcon/Makefile
index e4fa8ba..0451f698 100644
--- a/drivers/extcon/Makefile
+++ b/drivers/extcon/Makefile
@@ -10,3 +10,4 @@ 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
+obj-$(CONFIG_EXTCON_GPIO_USBVID)	+= extcon-gpio-usbvid.o
diff --git a/drivers/extcon/extcon-gpio-usbvid.c b/drivers/extcon/extcon-gpio-usbvid.c
new file mode 100644
index 0000000..e9bc2a97
--- /dev/null
+++ b/drivers/extcon/extcon-gpio-usbvid.c
@@ -0,0 +1,286 @@
+/*
+ * Generic  USB VBUS-ID pin detection 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: George Cherian <george.cherian@ti.com>
+ *
+ * Based on extcon-palmas.c
+ *
+ * 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.
+ */
+
+#include <linux/module.h>
+#include <linux/interrupt.h>
+#include <linux/kthread.h>
+#include <linux/freezer.h>
+#include <linux/platform_device.h>
+#include <linux/extcon.h>
+#include <linux/err.h>
+#include <linux/of.h>
+#include <linux/gpio.h>
+#include <linux/of_gpio.h>
+#include <linux/of_platform.h>
+
+struct gpio_usbvid {
+	struct device *dev;
+
+	struct extcon_dev edev;
+
+	/*GPIO pin */
+	int id_gpio;
+	int vbus_gpio;
+
+	int id_irq;
+	int vbus_irq;
+	int type;
+};
+
+static const char *dra7xx_extcon_cable[] = {
+	[0] = "USB",
+	[1] = "USB-HOST",
+	NULL,
+};
+
+static const int mutually_exclusive[] = {0x3, 0x0};
+
+/* Two types of support are provided.
+ * Systems which has
+ *	1) VBUS and ID pin connected via GPIO
+ *	2)  only ID pin connected via GPIO
+ *  For Case 1 both the gpios should be provided via DT
+ *  Always the first GPIO in dt is considered ID pin GPIO
+ */
+
+enum {
+	UNKNOWN = 0,
+	ID_DETECT,
+	VBUS_ID_DETECT,
+};
+
+#define ID_GND		0
+#define ID_FLOAT	1
+#define VBUS_OFF	0
+#define VBUS_ON		1
+
+
+static irqreturn_t id_irq_handler(int irq, void *data)
+{
+	struct gpio_usbvid *gpio_usbvid = (struct gpio_usbvid *) data;
+	int id_current;
+
+	id_current = gpio_get_value_cansleep(gpio_usbvid->id_gpio);
+	if (id_current == ID_GND) {
+		if (gpio_usbvid->type == ID_DETECT)
+			extcon_set_cable_state(&gpio_usbvid->edev,
+							"USB", false);
+		extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", true);
+	} else {
+		extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", false);
+		if (gpio_usbvid->type == ID_DETECT)
+			extcon_set_cable_state(&gpio_usbvid->edev,
+							"USB", true);
+	}
+	return IRQ_HANDLED;
+}
+
+static irqreturn_t vbus_irq_handler(int irq, void *data)
+{
+	struct gpio_usbvid *gpio_usbvid = (struct gpio_usbvid *) data;
+	int vbus_current;
+
+	vbus_current = gpio_get_value_cansleep(gpio_usbvid->vbus_gpio);
+	if (vbus_current == VBUS_OFF)
+		extcon_set_cable_state(&gpio_usbvid->edev, "USB", false);
+	else
+		extcon_set_cable_state(&gpio_usbvid->edev, "USB", true);
+
+	return IRQ_HANDLED;
+}
+
+
+static void gpio_usbvid_set_initial_state(struct gpio_usbvid *gpio_usbvid)
+{
+	int id_current;
+	int vbus_current;
+
+	switch (gpio_usbvid->type) {
+	case ID_DETECT:
+		id_current = gpio_get_value_cansleep(gpio_usbvid->id_gpio);
+		if (!!id_current == ID_FLOAT) {
+			extcon_set_cable_state(&gpio_usbvid->edev,
+							"USB-HOST", false);
+			extcon_set_cable_state(&gpio_usbvid->edev,
+							"USB", true);
+		} else {
+			extcon_set_cable_state(&gpio_usbvid->edev,
+							"USB", false);
+			extcon_set_cable_state(&gpio_usbvid->edev,
+							"USB-HOST", true);
+		}
+		break;
+
+	case VBUS_ID_DETECT:
+		id_current = gpio_get_value_cansleep(gpio_usbvid->id_gpio);
+		if (!!id_current == ID_FLOAT)
+			extcon_set_cable_state(&gpio_usbvid->edev,
+							"USB-HOST", false);
+		else
+			extcon_set_cable_state(&gpio_usbvid->edev,
+							"USB-HOST", true);
+
+		vbus_current = gpio_get_value_cansleep(gpio_usbvid->vbus_gpio);
+		if (!!vbus_current == VBUS_ON)
+			extcon_set_cable_state(&gpio_usbvid->edev,
+							"USB", true);
+		else
+			extcon_set_cable_state(&gpio_usbvid->edev,
+							"USB", false);
+		break;
+
+	default:
+		dev_err(gpio_usbvid->dev, "Unknown VBUS-ID type\n");
+	}
+}
+
+static int gpio_usbvid_request_irq(struct gpio_usbvid *gpio_usbvid)
+{
+	int ret;
+	ret = devm_request_threaded_irq(gpio_usbvid->dev, gpio_usbvid->id_irq,
+					NULL, id_irq_handler,
+					IRQF_ONESHOT | IRQF_TRIGGER_FALLING,
+					dev_name(gpio_usbvid->dev),
+					(void *) gpio_usbvid);
+	if (ret) {
+		dev_err(gpio_usbvid->dev, "failed to request id irq #%d\n",
+					gpio_usbvid->id_irq);
+		return ret;
+	}
+	if (gpio_usbvid->type == VBUS_ID_DETECT) {
+		ret = devm_request_threaded_irq(gpio_usbvid->dev,
+					gpio_usbvid->vbus_irq, NULL,
+					vbus_irq_handler,
+					IRQF_ONESHOT | IRQF_TRIGGER_FALLING,
+					dev_name(gpio_usbvid->dev),
+					(void *) gpio_usbvid);
+		if (ret)
+			dev_err(gpio_usbvid->dev, "failed to request vbus irq #%d\n",
+						gpio_usbvid->vbus_irq);
+	}
+	return ret;
+}
+
+static int gpio_usbvid_probe(struct platform_device *pdev)
+{
+	struct device_node *node = pdev->dev.of_node;
+	struct gpio_usbvid *gpio_usbvid;
+	int ret;
+	int gpio;
+
+	gpio_usbvid = devm_kzalloc(&pdev->dev, sizeof(*gpio_usbvid),
+								GFP_KERNEL);
+	if (!gpio_usbvid)
+		return -ENOMEM;
+
+
+	gpio_usbvid->dev	 = &pdev->dev;
+
+	platform_set_drvdata(pdev, gpio_usbvid);
+
+	gpio_usbvid->edev.name = dev_name(&pdev->dev);
+	gpio_usbvid->edev.supported_cable = dra7xx_extcon_cable;
+	gpio_usbvid->edev.mutually_exclusive = mutually_exclusive;
+
+	if (of_device_is_compatible(node, "ti,gpio-usb-id"))
+		gpio_usbvid->type = ID_DETECT;
+
+	gpio = of_get_gpio(node, 0);
+	if (gpio_is_valid(gpio)) {
+		gpio_usbvid->id_gpio = gpio;
+		ret = devm_gpio_request(&pdev->dev, gpio_usbvid->id_gpio,
+					"id_gpio");
+		if (ret)
+			return ret;
+		gpio_usbvid->id_irq = gpio_to_irq(gpio_usbvid->id_gpio);
+	} else {
+		dev_err(&pdev->dev, "failed to get id gpio\n");
+		return -ENODEV;
+	}
+
+	if (of_device_is_compatible(node, "ti,gpio-usb-vid")) {
+		gpio_usbvid->type = VBUS_ID_DETECT;
+		gpio = of_get_gpio(node, 1);
+		if (gpio_is_valid(gpio)) {
+			gpio_usbvid->vbus_gpio = gpio;
+			ret = devm_gpio_request(&pdev->dev,
+						gpio_usbvid->vbus_gpio,
+						"vbus_gpio");
+			if (ret)
+				return ret;
+			gpio_usbvid->vbus_irq =
+					gpio_to_irq(gpio_usbvid->vbus_gpio);
+		} else {
+			dev_err(&pdev->dev, "failed to get vbus gpio\n");
+			return -ENODEV;
+		}
+	}
+
+	ret = extcon_dev_register(&gpio_usbvid->edev, gpio_usbvid->dev);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to register extcon device\n");
+		return ret;
+	}
+
+	gpio_usbvid_set_initial_state(gpio_usbvid);
+	ret = gpio_usbvid_request_irq(gpio_usbvid);
+	if (ret)
+		goto err0;
+
+	return 0;
+
+err0:
+	extcon_dev_unregister(&gpio_usbvid->edev);
+
+	return ret;
+}
+
+static int gpio_usbvid_remove(struct platform_device *pdev)
+{
+	struct gpio_usbvid *gpio_usbvid = platform_get_drvdata(pdev);
+
+	extcon_dev_unregister(&gpio_usbvid->edev);
+	return 0;
+}
+
+static struct of_device_id of_gpio_usbvid_match_tbl[] = {
+	{ .compatible = "ti,gpio-usb-vid", },
+	{ .compatible = "ti,gpio-usb-id", },
+	{ /* end */ }
+};
+
+static struct platform_driver gpio_usbvid_driver = {
+	.probe = gpio_usbvid_probe,
+	.remove = gpio_usbvid_remove,
+	.driver = {
+		.name = "gpio-usbvid",
+		.of_match_table = of_gpio_usbvid_match_tbl,
+		.owner = THIS_MODULE,
+	},
+};
+
+module_platform_driver(gpio_usbvid_driver);
+
+MODULE_ALIAS("platform:gpio-usbvid");
+MODULE_AUTHOR("George Cherian <george.cherian@ti.com>");
+MODULE_DESCRIPTION("GPIO based USB Connector driver");
+MODULE_LICENSE("GPL");
+MODULE_DEVICE_TABLE(of, of_gpio_usbvid_match_tbl);
-- 
1.8.1.4

[PATCH v3 2/3] drivers: Makefile: Extcon is a framework so bump it up

[George Cherian]

Bump up the order, since extcon is a framework and needed by
other drivers. With the previous order it failed to detect
extcon device in DWC3 when both were compiled built-in.

Signed-off-by: George Cherian <george.cherian@ti.com>
---
 drivers/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/Makefile b/drivers/Makefile
index ab93de8..4573b8d 100644
--- a/drivers/Makefile
+++ b/drivers/Makefile
@@ -49,6 +49,7 @@ obj-y				+= char/
 obj-y				+= gpu/
 
 obj-$(CONFIG_CONNECTOR)		+= connector/
+obj-$(CONFIG_EXTCON)		+= extcon/
 
 # i810fb and intelfb depend on char/agp/
 obj-$(CONFIG_FB_I810)           += video/i810/
@@ -145,7 +146,6 @@ obj-$(CONFIG_VIRT_DRIVERS)	+= virt/
 obj-$(CONFIG_HYPERV)		+= hv/
 
 obj-$(CONFIG_PM_DEVFREQ)	+= devfreq/
-obj-$(CONFIG_EXTCON)		+= extcon/
 obj-$(CONFIG_MEMORY)		+= memory/
 obj-$(CONFIG_IIO)		+= iio/
 obj-$(CONFIG_VME_BUS)		+= vme/
-- 
1.8.1.4

[PATCH v3 3/3] ARM: dts: dra7-evm: Add extcon nodes for USB ID pin detection

[George Cherian]

Add
	-extcon nodes for USB ID pin detection.
	-i2c nodes.
	-pcf nodes to which USB ID pin is connected.

Signed-off-by: George Cherian <george.cherian@ti.com>
---
 arch/arm/boot/dts/dra7-evm.dts | 50 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 49 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/dra7-evm.dts b/arch/arm/boot/dts/dra7-evm.dts
index acd3c09..8b0738a 100644
--- a/arch/arm/boot/dts/dra7-evm.dts
+++ b/arch/arm/boot/dts/dra7-evm.dts
@@ -17,6 +17,17 @@
 		device_type = "memory";
 		reg = <0x80000000 0x60000000>; /* 1536 MB */
 	};
+
+	extcon1: gpio_usbvid_extcon1 {
+		compatible = "ti,gpio-usb-id";
+		gpios = <&gpio21 1 0>;
+	};
+
+	extcon2: gpio_usbvid_extcon2 {
+		compatible = "ti,gpio-usb-id";
+		gpios = <&gpio21 2 0>;
+	};
+
 };
 
 &dra7_pmx_core {
@@ -33,10 +44,47 @@
         };
 };
 
+&i2c1 {
+	clock-frequency = <400000>;
+
+	gpio20: pcf8575@20 {
+		compatible = "ti,pcf8575";
+		reg = <0x20>;
+		n_latch = <0x4000>;
+		gpio-controller;
+		#gpio-cells = <2>;
+		interrupt-parent = <&gpio6>;
+		interrupts = <11 2>;
+		interrupt-controller;
+		#interrupt-cells = <2>;
+	};
+
+	gpio21: pcf8575@21 {
+		compatible = "ti,pcf8575";
+		reg = <0x21>;
+		n_latch = <0x1408>;
+		gpio-controller;
+		#gpio-cells = <2>;
+		interrupt-parent = <&pcf_20>;
+		interrupts = <14 2>;
+		interrupt-controller;
+		#interrupt-cells = <2>;
+	};
+
+};
+
 &dwc3_1 {
-	dr_mode = "otg";
+	dr_mode = "host";
 };
 
 &dwc3_2 {
 	dr_mode = "host";
 };
+
+&usb1 {
+	extcon = <&extcon1>;
+};
+
+&usb2 {
+	extcon = <&extcon2>;
+};
-- 
1.8.1.4

Re: [PATCH v3 3/3] ARM: dts: dra7-evm: Add extcon nodes for USB ID pin detection

[Sergei Shtylyov]

On 08/28/2013 09:33 PM, George Cherian wrote:

> Add
> 	-extcon nodes for USB ID pin detection.
> 	-i2c nodes.
> 	-pcf nodes to which USB ID pin is connected.

> Signed-off-by: George Cherian <george.cherian@ti.com>
> ---
>   arch/arm/boot/dts/dra7-evm.dts | 50 +++++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 49 insertions(+), 1 deletion(-)

> diff --git a/arch/arm/boot/dts/dra7-evm.dts b/arch/arm/boot/dts/dra7-evm.dts
> index acd3c09..8b0738a 100644
> --- a/arch/arm/boot/dts/dra7-evm.dts
> +++ b/arch/arm/boot/dts/dra7-evm.dts
[...]
> @@ -33,10 +44,47 @@
>           };
>   };
>
> +&i2c1 {
> +	clock-frequency = <400000>;
> +
> +	gpio20: pcf8575@20 {

     ePAPR was talking about the node naming, not about labelling. Back to the 
drawing board. ;-)

> +		compatible = "ti,pcf8575";
> +		reg = <0x20>;
> +		n_latch = <0x4000>;
> +		gpio-controller;
> +		#gpio-cells = <2>;
> +		interrupt-parent = <&gpio6>;
> +		interrupts = <11 2>;
> +		interrupt-controller;
> +		#interrupt-cells = <2>;
> +	};
> +
> +	gpio21: pcf8575@21 {
> +		compatible = "ti,pcf8575";
> +		reg = <0x21>;
> +		n_latch = <0x1408>;
> +		gpio-controller;
> +		#gpio-cells = <2>;
> +		interrupt-parent = <&pcf_20>;
> +		interrupts = <14 2>;
> +		interrupt-controller;
> +		#interrupt-cells = <2>;
> +	};
> +
> +};
> +

WBR, Sergei

Re: [PATCH v3 1/3] extcon: extcon-gpio-usbvid: Generic USB VBUS/ID detection via GPIO

[Chanwoo Choi]

Hi George,

You didn't modify this patchset about my comment on v1 patchset.
Please pay attention to comment.

On 08/29/2013 02:33 AM, George Cherian wrote:
> Add a generic USB VBUS/ID detection EXTCON driver. This driver expects
> the ID/VBUS pin are connected via GPIOs. This driver is tested on
> DRA7x board were the ID pin is routed via GPIOs. The driver supports
> both VBUS and ID pin configuration and ID pin only configuration.
> 
> Signed-off-by: George Cherian <george.cherian@ti.com>
> ---
>  .../bindings/extcon/extcon-gpio-usbvid.txt         |  20 ++
>  drivers/extcon/Kconfig                             |   6 +
>  drivers/extcon/Makefile                            |   1 +
>  drivers/extcon/extcon-gpio-usbvid.c                | 286 +++++++++++++++++++++

You should keep following naming stlye. extcon-gpio-usbvid.c is wrong naming style.
- extcon-[device name].c
- extcon-gpio-usbvid.c -> extcon-dra7xx.c or etc.

Also, you should change the file name of extcon-gpio-usbvid.txt.

Finally, You used 'gpio_usbvid' prefix in extcon-gpio-usbvid.c.
It has caused the confusion that user would think extcon-gpio-usbvid.c driver
support all of extcon driver using gpio irq pin. So I'd like you to use
proper prefix including device name.

>  4 files changed, 313 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/extcon/extcon-gpio-usbvid.txt
>  create mode 100644 drivers/extcon/extcon-gpio-usbvid.c
> 
> diff --git a/Documentation/devicetree/bindings/extcon/extcon-gpio-usbvid.txt b/Documentation/devicetree/bindings/extcon/extcon-gpio-usbvid.txt
> new file mode 100644
> index 0000000..eea0741
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/extcon/extcon-gpio-usbvid.txt
> @@ -0,0 +1,20 @@
> +EXTCON FOR USB VIA GPIO
> +
> +Required Properties:
> + - compatible : Should be "ti,gpio-usb-vid" for USB VBUS-ID detector
> +		using gpios or "ti,gpio-usb-id" for USB ID pin detector
> + - gpios : phandle and args ID pin gpio and VBUS gpio.
> +	   The first gpio used  for ID pin detection
> +	   and the second used for VBUS detection.
> +	   ID pin gpio is mandatory and VBUS is optional
> +	   depending on implementation.
> +
> +Please refer to ../gpio/gpio.txt for details of the common GPIO bindings
> +
> +Example:
> +
> +	gpio_usbvid_extcon1 {
> +		compatible = "ti,gpio-usb-vid";
> +		gpios = <&gpio1 1 0>,
> +			<&gpio2 2 0>;
> +	};
> diff --git a/drivers/extcon/Kconfig b/drivers/extcon/Kconfig
> index f1d54a3..8097398 100644
> --- a/drivers/extcon/Kconfig
> +++ b/drivers/extcon/Kconfig
> @@ -64,4 +64,10 @@ config EXTCON_PALMAS
>  	  Say Y here to enable support for USB peripheral and USB host
>  	  detection by palmas usb.
>  
> +config EXTCON_GPIO_USBVID
> +	tristate "Generic USB VBUS/ID detection using GPIO EXTCON support"
> +	help
> +	  Say Y here to enable support for USB VBUS/ID deetction by GPIO.
> +
> +

Remove blank line.

>  endif # MULTISTATE_SWITCH
> diff --git a/drivers/extcon/Makefile b/drivers/extcon/Makefile
> index e4fa8ba..0451f698 100644
> --- a/drivers/extcon/Makefile
> +++ b/drivers/extcon/Makefile
> @@ -10,3 +10,4 @@ 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
> +obj-$(CONFIG_EXTCON_GPIO_USBVID)	+= extcon-gpio-usbvid.o
> diff --git a/drivers/extcon/extcon-gpio-usbvid.c b/drivers/extcon/extcon-gpio-usbvid.c
> new file mode 100644
> index 0000000..e9bc2a97
> --- /dev/null
> +++ b/drivers/extcon/extcon-gpio-usbvid.c
> @@ -0,0 +1,286 @@
> +/*
> + * Generic  USB VBUS-ID pin detection 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: George Cherian <george.cherian@ti.com>
> + *
> + * Based on extcon-palmas.c
> + *
> + * 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.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/interrupt.h>
> +#include <linux/kthread.h>
> +#include <linux/freezer.h>

kthead.h, freezer.h headerfile is used in this file?

> +#include <linux/platform_device.h>
> +#include <linux/extcon.h>
> +#include <linux/err.h>
> +#include <linux/of.h>
> +#include <linux/gpio.h>
> +#include <linux/of_gpio.h>
> +#include <linux/of_platform.h>

Order headerfile alphabetically.

> +
> +struct gpio_usbvid {
> +	struct device *dev;
> +
> +	struct extcon_dev edev;
> +
> +	/*GPIO pin */

I commented previous your patch about this wrong coding style.
Why did not you fix this coding style?

> +	int id_gpio;
> +	int vbus_gpio;
> +
> +	int id_irq;
> +	int vbus_irq;
> +	int type;
> +};
> +
> +static const char *dra7xx_extcon_cable[] = {
> +	[0] = "USB",
> +	[1] = "USB-HOST",
> +	NULL,
> +};
> +
> +static const int mutually_exclusive[] = {0x3, 0x0};
> +
> +/* Two types of support are provided.
> + * Systems which has
> + *	1) VBUS and ID pin connected via GPIO
> + *	2)  only ID pin connected via GPIO

Remove blank between '2)' and 'only'.

> + *  For Case 1 both the gpios should be provided via DT
> + *  Always the first GPIO in dt is considered ID pin GPIO
> + */
> +
> +enum {
> +	UNKNOWN = 0,
> +	ID_DETECT,
> +	VBUS_ID_DETECT,
> +};
> +
> +#define ID_GND		0
> +#define ID_FLOAT	1
> +#define VBUS_OFF	0
> +#define VBUS_ON		1

I think you could only use two constant instead of four constant definition.

> +
> +

This blank line isn't necessary.

> +static irqreturn_t id_irq_handler(int irq, void *data)
> +{
> +	struct gpio_usbvid *gpio_usbvid = (struct gpio_usbvid *) data;

You should delete blank between ')' and 'data' as follwong: 
	- (struct gpio_usbvid *)data;

> +	int id_current;
> +
> +	id_current = gpio_get_value_cansleep(gpio_usbvid->id_gpio);
> +	if (id_current == ID_GND) {
> +		if (gpio_usbvid->type == ID_DETECT)
> +			extcon_set_cable_state(&gpio_usbvid->edev,
> +							"USB", false);
> +		extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", true);

As else statement, you should set "USB-HOST" cable state to improve readability.

		extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", true);
		if (gpio_usbvid->type == ID_DETECT)
			extcon_set_cable_state(&gpio_usbvid->edev,
							"USB", false);

> +	} else {
> +		extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", false);
> +		if (gpio_usbvid->type == ID_DETECT)
> +			extcon_set_cable_state(&gpio_usbvid->edev,
> +							"USB", true);
> +	}

Add blank line.

> +	return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t vbus_irq_handler(int irq, void *data)
> +{
> +	struct gpio_usbvid *gpio_usbvid = (struct gpio_usbvid *) data;

ditto.

> +	int vbus_current;
> +
> +	vbus_current = gpio_get_value_cansleep(gpio_usbvid->vbus_gpio);
> +	if (vbus_current == VBUS_OFF)
> +		extcon_set_cable_state(&gpio_usbvid->edev, "USB", false);
> +	else
> +		extcon_set_cable_state(&gpio_usbvid->edev, "USB", true);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +

This blank line isn't necessary.
I commented unnecessary blank line on previous review.

> +static void gpio_usbvid_set_initial_state(struct gpio_usbvid *gpio_usbvid)
> +{
> +	int id_current;
> +	int vbus_current;

Define loacal variable on one line as following:
	int id_current, vbus_current;

> +
> +	switch (gpio_usbvid->type) {
> +	case ID_DETECT:
> +		id_current = gpio_get_value_cansleep(gpio_usbvid->id_gpio);
> +		if (!!id_current == ID_FLOAT) {
> +			extcon_set_cable_state(&gpio_usbvid->edev,
> +							"USB-HOST", false);
> +			extcon_set_cable_state(&gpio_usbvid->edev,
> +							"USB", true);
> +		} else {
> +			extcon_set_cable_state(&gpio_usbvid->edev,
> +							"USB", false);
> +			extcon_set_cable_state(&gpio_usbvid->edev,
> +							"USB-HOST", true);
> +		}
> +		break;
> +
> +	case VBUS_ID_DETECT:
> +		id_current = gpio_get_value_cansleep(gpio_usbvid->id_gpio);
> +		if (!!id_current == ID_FLOAT)
> +			extcon_set_cable_state(&gpio_usbvid->edev,
> +							"USB-HOST", false);
> +		else
> +			extcon_set_cable_state(&gpio_usbvid->edev,
> +							"USB-HOST", true);
> +
> +		vbus_current = gpio_get_value_cansleep(gpio_usbvid->vbus_gpio);
> +		if (!!vbus_current == VBUS_ON)
> +			extcon_set_cable_state(&gpio_usbvid->edev,
> +							"USB", true);
> +		else
> +			extcon_set_cable_state(&gpio_usbvid->edev,
> +							"USB", false);
> +		break;
> +
> +	default:
> +		dev_err(gpio_usbvid->dev, "Unknown VBUS-ID type\n");
> +	}
> +}
> +
> +static int gpio_usbvid_request_irq(struct gpio_usbvid *gpio_usbvid)
> +{
> +	int ret;

Add blank line.

> +	ret = devm_request_threaded_irq(gpio_usbvid->dev, gpio_usbvid->id_irq,
> +					NULL, id_irq_handler,
> +					IRQF_ONESHOT | IRQF_TRIGGER_FALLING,
> +					dev_name(gpio_usbvid->dev),
> +					(void *) gpio_usbvid);
> +	if (ret) {
> +		dev_err(gpio_usbvid->dev, "failed to request id irq #%d\n",
> +					gpio_usbvid->id_irq);
> +		return ret;
> +	}
> +	if (gpio_usbvid->type == VBUS_ID_DETECT) {
> +		ret = devm_request_threaded_irq(gpio_usbvid->dev,
> +					gpio_usbvid->vbus_irq, NULL,
> +					vbus_irq_handler,
> +					IRQF_ONESHOT | IRQF_TRIGGER_FALLING,
> +					dev_name(gpio_usbvid->dev),

Why do you use the same interrupt name both gpio_usbvid->id_irq and gpio_usbvid->vbus_irq?
You should use characteristic interrupt name.

> +					(void *) gpio_usbvid);
> +		if (ret)
> +			dev_err(gpio_usbvid->dev, "failed to request vbus irq #%d\n",
> +						gpio_usbvid->vbus_irq);
> +	}

Add blank line.

> +	return ret;
> +}
> +
> +static int gpio_usbvid_probe(struct platform_device *pdev)
> +{
> +	struct device_node *node = pdev->dev.of_node;
> +	struct gpio_usbvid *gpio_usbvid;
> +	int ret;
> +	int gpio;

Define loacal variable on one line as following:
	int ret, gpio;

> +
> +	gpio_usbvid = devm_kzalloc(&pdev->dev, sizeof(*gpio_usbvid),
> +								GFP_KERNEL);

If this statement over 80 line, you have to keep proper indentation as following:
	gpio_usbvid = devm_kzalloc(&pdev->dev, sizeof(*gpio_usbvid),
				  GFP_KERNEL);

> +	if (!gpio_usbvid)
> +		return -ENOMEM;
> +
> +

Remove blank line.

> +	gpio_usbvid->dev	 = &pdev->dev;

Use space instead of tab.

> +
> +	platform_set_drvdata(pdev, gpio_usbvid);
> +
> +	gpio_usbvid->edev.name = dev_name(&pdev->dev);

I add as follwong comment about your v1 patchset

	"If edev name is equal with device name, this line is unnecessary.
	Because extcon_dev_register() use dev_name(&pdev->dev) as edev name
	in extcon-class.c"

Why did not apply for my comment to v3 patchset?
Plesae pay attention for previous comment.

> +	gpio_usbvid->edev.supported_cable = dra7xx_extcon_cable;
> +	gpio_usbvid->edev.mutually_exclusive = mutually_exclusive;
> +
> +	if (of_device_is_compatible(node, "ti,gpio-usb-id"))
> +		gpio_usbvid->type = ID_DETECT;
> +
> +	gpio = of_get_gpio(node, 0);
> +	if (gpio_is_valid(gpio)) {
> +		gpio_usbvid->id_gpio = gpio;
> +		ret = devm_gpio_request(&pdev->dev, gpio_usbvid->id_gpio,
> +					"id_gpio");
> +		if (ret)
> +			return ret;

Add blank line.

> +		gpio_usbvid->id_irq = gpio_to_irq(gpio_usbvid->id_gpio);
> +	} else {
> +		dev_err(&pdev->dev, "failed to get id gpio\n");
> +		return -ENODEV;
> +	}
> +
> +	if (of_device_is_compatible(node, "ti,gpio-usb-vid")) {
> +		gpio_usbvid->type = VBUS_ID_DETECT;
> +		gpio = of_get_gpio(node, 1);
> +		if (gpio_is_valid(gpio)) {
> +			gpio_usbvid->vbus_gpio = gpio;
> +			ret = devm_gpio_request(&pdev->dev,
> +						gpio_usbvid->vbus_gpio,
> +						"vbus_gpio");
> +			if (ret)
> +				return ret;

Add blank line.

> +			gpio_usbvid->vbus_irq =
> +					gpio_to_irq(gpio_usbvid->vbus_gpio);
> +		} else {
> +			dev_err(&pdev->dev, "failed to get vbus gpio\n");
> +			return -ENODEV;
> +		}
> +	}
> +
> +	ret = extcon_dev_register(&gpio_usbvid->edev, gpio_usbvid->dev);
> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to register extcon device\n");
> +		return ret;
> +	}
> +
> +	gpio_usbvid_set_initial_state(gpio_usbvid);
> +	ret = gpio_usbvid_request_irq(gpio_usbvid);

You should move gpio_usbvid_request_irq() call before extcon_dev_register().

> +	if (ret)
> +		goto err0;

? As following previous comment about v1 patchset:
	I need correct meaning name as err_thread or etc ...

> +
> +	return 0;
> +
> +err0:

ditto.

> +	extcon_dev_unregister(&gpio_usbvid->edev);
> +
> +	return ret;
> +}
> +
> +static int gpio_usbvid_remove(struct platform_device *pdev)
> +{
> +	struct gpio_usbvid *gpio_usbvid = platform_get_drvdata(pdev);
> +
> +	extcon_dev_unregister(&gpio_usbvid->edev);
> +	return 0;
> +}
> +
> +static struct of_device_id of_gpio_usbvid_match_tbl[] = {
> +	{ .compatible = "ti,gpio-usb-vid", },
> +	{ .compatible = "ti,gpio-usb-id", },
> +	{ /* end */ }
> +};
> +
> +static struct platform_driver gpio_usbvid_driver = {
> +	.probe = gpio_usbvid_probe,
> +	.remove = gpio_usbvid_remove,
> +	.driver = {
> +		.name = "gpio-usbvid",
> +		.of_match_table = of_gpio_usbvid_match_tbl,
> +		.owner = THIS_MODULE,
> +	},
> +};
> +
> +module_platform_driver(gpio_usbvid_driver);
> +
> +MODULE_ALIAS("platform:gpio-usbvid");
> +MODULE_AUTHOR("George Cherian <george.cherian@ti.com>");
> +MODULE_DESCRIPTION("GPIO based USB Connector driver");
> +MODULE_LICENSE("GPL");
> +MODULE_DEVICE_TABLE(of, of_gpio_usbvid_match_tbl);
> 

Cheers,
Chanwoo Choi

Re: [PATCH v3 1/3] extcon: extcon-gpio-usbvid: Generic USB VBUS/ID detection via GPIO

[George Cherian]

Hi Chanwoo,

Thanks for the review and sorry for all the trivial mistakes.

On 8/29/2013 7:05 AM, Chanwoo Choi wrote:
> Hi George,
>
> You didn't modify this patchset about my comment on v1 patchset.
> Please pay attention to comment.
>
> On 08/29/2013 02:33 AM, George Cherian wrote:
>> Add a generic USB VBUS/ID detection EXTCON driver. This driver expects
>> the ID/VBUS pin are connected via GPIOs. This driver is tested on
>> DRA7x board were the ID pin is routed via GPIOs. The driver supports
>> both VBUS and ID pin configuration and ID pin only configuration.
>>
>> Signed-off-by: George Cherian <george.cherian@ti.com>
>> ---
>>   .../bindings/extcon/extcon-gpio-usbvid.txt         |  20 ++
>>   drivers/extcon/Kconfig                             |   6 +
>>   drivers/extcon/Makefile                            |   1 +
>>   drivers/extcon/extcon-gpio-usbvid.c                | 286 +++++++++++++++++++++
> You should keep following naming stlye. extcon-gpio-usbvid.c is wrong naming style.
> - extcon-[device name].c
> - extcon-gpio-usbvid.c -> extcon-dra7xx.c or etc.
Actually dra7xx is the SoC name and the USB VBUS/ID detection is not 
specific to SoC.
It uses gpios to detect the VBUS/ID change. So i thought it would be 
better to have generic
gpio based VBUS/ID detection rather than making dra7xx specific. Stephen 
Warren had this opinion
with patch v1.
>
> Also, you should change the file name of extcon-gpio-usbvid.txt.
>
> Finally, You used 'gpio_usbvid' prefix in extcon-gpio-usbvid.c.
> It has caused the confusion that user would think extcon-gpio-usbvid.c driver
> support all of extcon driver using gpio irq pin. So I'd like you to use
> proper prefix including device name.
I meant to support all of extcon driver using gpio for USB VBUS/ID 
detection.
>
>>   4 files changed, 313 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/extcon/extcon-gpio-usbvid.txt
>>   create mode 100644 drivers/extcon/extcon-gpio-usbvid.c
[snip]
>> index f1d54a3..8097398 100644
>> --- a/drivers/extcon/Kconfig
>> +++ b/drivers/extcon/Kconfig
>> @@ -64,4 +64,10 @@ config EXTCON_PALMAS
>>   	  Say Y here to enable support for USB peripheral and USB host
>>   	  detection by palmas usb.
>>   
>> +config EXTCON_GPIO_USBVID
>> +	tristate "Generic USB VBUS/ID detection using GPIO EXTCON support"
>> +	help
>> +	  Say Y here to enable support for USB VBUS/ID deetction by GPIO.
>> +
>> +
> Remove blank line.
okay
>>   endif # MULTISTATE_SWITCH
[snip]
>> diff --git a/drivers/extcon/extcon-gpio-usbvid.c b/drivers/extcon/extcon-gpio-usbvid.c
>> new file mode 100644
>> index 0000000..e9bc2a97
>> --- /dev/null
>> +++ b/drivers/extcon/extcon-gpio-usbvid.c
>> @@ -0,0 +1,286 @@
>> +/*
>> + * Generic  USB VBUS-ID pin detection 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: George Cherian <george.cherian@ti.com>
>> + *
>> + * Based on extcon-palmas.c
>> + *
>> + * 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.
>> + */
>> +
>> +#include <linux/module.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/kthread.h>
>> +#include <linux/freezer.h>
> kthead.h, freezer.h headerfile is used in this file?
okay
>> +#include <linux/platform_device.h>
>> +#include <linux/extcon.h>
>> +#include <linux/err.h>
>> +#include <linux/of.h>
>> +#include <linux/gpio.h>
>> +#include <linux/of_gpio.h>
>> +#include <linux/of_platform.h>
> Order headerfile alphabetically.
okay
>
>> +
>> +struct gpio_usbvid {
>> +	struct device *dev;
>> +
>> +	struct extcon_dev edev;
>> +
>> +	/*GPIO pin */
> I commented previous your patch about this wrong coding style.
> Why did not you fix this coding style?
okay
>> +	int id_gpio;
>> +	int vbus_gpio;
>> +
>> +	int id_irq;
>> +	int vbus_irq;
>> +	int type;
>> +};
>> +
>> +static const char *dra7xx_extcon_cable[] = {
>> +	[0] = "USB",
>> +	[1] = "USB-HOST",
>> +	NULL,
>> +};
>> +
>> +static const int mutually_exclusive[] = {0x3, 0x0};
>> +
>> +/* Two types of support are provided.
>> + * Systems which has
>> + *	1) VBUS and ID pin connected via GPIO
>> + *	2)  only ID pin connected via GPIO
> Remove blank between '2)' and 'only'.
okay
>> + *  For Case 1 both the gpios should be provided via DT
>> + *  Always the first GPIO in dt is considered ID pin GPIO
>> + */
>> +
>> +enum {
>> +	UNKNOWN = 0,
>> +	ID_DETECT,
>> +	VBUS_ID_DETECT,
>> +};
>> +
>> +#define ID_GND		0
>> +#define ID_FLOAT	1
>> +#define VBUS_OFF	0
>> +#define VBUS_ON		1
> I think you could only use two constant instead of four constant definition.
you mean only ID_GND and VBUS_OFF?
>> +
>> +
> This blank line isn't necessary.
>
>> +static irqreturn_t id_irq_handler(int irq, void *data)
>> +{
>> +	struct gpio_usbvid *gpio_usbvid = (struct gpio_usbvid *) data;
> You should delete blank between ')' and 'data' as follwong:
> 	- (struct gpio_usbvid *)data;
okay
>
>> +	int id_current;
>> +
>> +	id_current = gpio_get_value_cansleep(gpio_usbvid->id_gpio);
>> +	if (id_current == ID_GND) {
>> +		if (gpio_usbvid->type == ID_DETECT)
>> +			extcon_set_cable_state(&gpio_usbvid->edev,
>> +							"USB", false);
>> +		extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", true);
> As else statement, you should set "USB-HOST" cable state to improve readability.
>
> 		extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", true);
> 		if (gpio_usbvid->type == ID_DETECT)
> 			extcon_set_cable_state(&gpio_usbvid->edev,
> 							"USB", false);
Actually, USB-HOST state should be set in the id_irq handler. But in 
cases were only ID pin is routed to gpio
and VBUS is not used we set the USB state too. The gpio_usbvid->type 
differentiates whether its an ID only or
VBUS and ID.
>> +	} else {
>> +		extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", false);
>> +		if (gpio_usbvid->type == ID_DETECT)
>> +			extcon_set_cable_state(&gpio_usbvid->edev,
>> +							"USB", true);
>> +	}
> Add blank line.
okay
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +static irqreturn_t vbus_irq_handler(int irq, void *data)
>> +{
>> +	struct gpio_usbvid *gpio_usbvid = (struct gpio_usbvid *) data;
> ditto.
okay
>> +	int vbus_current;
>> +
>> +	vbus_current = gpio_get_value_cansleep(gpio_usbvid->vbus_gpio);
>> +	if (vbus_current == VBUS_OFF)
>> +		extcon_set_cable_state(&gpio_usbvid->edev, "USB", false);
>> +	else
>> +		extcon_set_cable_state(&gpio_usbvid->edev, "USB", true);
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +
> This blank line isn't necessary.
> I commented unnecessary blank line on previous review.
okay
>> +static void gpio_usbvid_set_initial_state(struct gpio_usbvid *gpio_usbvid)
>> +{
>> +	int id_current;
>> +	int vbus_current;
> Define loacal variable on one line as following:
> 	int id_current, vbus_current;
okay
>> +
>> +	switch (gpio_usbvid->type) {
>> +	case ID_DETECT:
>> +		id_current = gpio_get_value_cansleep(gpio_usbvid->id_gpio);
>> +		if (!!id_current == ID_FLOAT) {
>> +			extcon_set_cable_state(&gpio_usbvid->edev,
>> +							"USB-HOST", false);
>> +			extcon_set_cable_state(&gpio_usbvid->edev,
>> +							"USB", true);
>> +		} else {
>> +			extcon_set_cable_state(&gpio_usbvid->edev,
>> +							"USB", false);
>> +			extcon_set_cable_state(&gpio_usbvid->edev,
>> +							"USB-HOST", true);
>> +		}
>> +		break;
>> +
>> +	case VBUS_ID_DETECT:
>> +		id_current = gpio_get_value_cansleep(gpio_usbvid->id_gpio);
>> +		if (!!id_current == ID_FLOAT)
>> +			extcon_set_cable_state(&gpio_usbvid->edev,
>> +							"USB-HOST", false);
>> +		else
>> +			extcon_set_cable_state(&gpio_usbvid->edev,
>> +							"USB-HOST", true);
>> +
>> +		vbus_current = gpio_get_value_cansleep(gpio_usbvid->vbus_gpio);
>> +		if (!!vbus_current == VBUS_ON)
>> +			extcon_set_cable_state(&gpio_usbvid->edev,
>> +							"USB", true);
>> +		else
>> +			extcon_set_cable_state(&gpio_usbvid->edev,
>> +							"USB", false);
>> +		break;
>> +
>> +	default:
>> +		dev_err(gpio_usbvid->dev, "Unknown VBUS-ID type\n");
>> +	}
>> +}
>> +
>> +static int gpio_usbvid_request_irq(struct gpio_usbvid *gpio_usbvid)
>> +{
>> +	int ret;
> Add blank line.
okay
>> +	ret = devm_request_threaded_irq(gpio_usbvid->dev, gpio_usbvid->id_irq,
>> +					NULL, id_irq_handler,
>> +					IRQF_ONESHOT | IRQF_TRIGGER_FALLING,
>> +					dev_name(gpio_usbvid->dev),
>> +					(void *) gpio_usbvid);
>> +	if (ret) {
>> +		dev_err(gpio_usbvid->dev, "failed to request id irq #%d\n",
>> +					gpio_usbvid->id_irq);
>> +		return ret;
>> +	}
>> +	if (gpio_usbvid->type == VBUS_ID_DETECT) {
>> +		ret = devm_request_threaded_irq(gpio_usbvid->dev,
>> +					gpio_usbvid->vbus_irq, NULL,
>> +					vbus_irq_handler,
>> +					IRQF_ONESHOT | IRQF_TRIGGER_FALLING,
>> +					dev_name(gpio_usbvid->dev),
> Why do you use the same interrupt name both gpio_usbvid->id_irq and gpio_usbvid->vbus_irq?
> You should use characteristic interrupt name.
if I use dev_name it gives me 2 same name interrupts associated with a 
single extcon device.
Where as if i use characteristic (for eg: VBUS or ID) names then I will 
get multiple with same name
since there will be 2 instances of extcon being used as of now.
>> +					(void *) gpio_usbvid);
>> +		if (ret)
>> +			dev_err(gpio_usbvid->dev, "failed to request vbus irq #%d\n",
>> +						gpio_usbvid->vbus_irq);
>> +	}
> Add blank line.
okay
>> +	return ret;
>> +}
>> +
>> +static int gpio_usbvid_probe(struct platform_device *pdev)
>> +{
>> +	struct device_node *node = pdev->dev.of_node;
>> +	struct gpio_usbvid *gpio_usbvid;
>> +	int ret;
>> +	int gpio;
> Define loacal variable on one line as following:
> 	int ret, gpio;
>
>> +
>> +	gpio_usbvid = devm_kzalloc(&pdev->dev, sizeof(*gpio_usbvid),
>> +								GFP_KERNEL);
> If this statement over 80 line, you have to keep proper indentation as following:
> 	gpio_usbvid = devm_kzalloc(&pdev->dev, sizeof(*gpio_usbvid),
> 				  GFP_KERNEL);
okay
>
>> +	if (!gpio_usbvid)
>> +		return -ENOMEM;
>> +
>> +
> Remove blank line.
okay
>> +	gpio_usbvid->dev	 = &pdev->dev;
> Use space instead of tab.
okay
>> +
>> +	platform_set_drvdata(pdev, gpio_usbvid);
>> +
>> +	gpio_usbvid->edev.name = dev_name(&pdev->dev);
> I add as follwong comment about your v1 patchset
>
> 	"If edev name is equal with device name, this line is unnecessary.
> 	Because extcon_dev_register() use dev_name(&pdev->dev) as edev name
> 	in extcon-class.c"
>
> Why did not apply for my comment to v3 patchset?
> Plesae pay attention for previous comment.
I removed it but it gave me a  NULL pointer dereference  in 
extcon_get_extcon_dev (strcmp the sd->name was NULL).
I am based on v3.11-rc3, did you have any fix for this in later rc's? 
probably I would rebase to your latest and check.
>> +	gpio_usbvid->edev.supported_cable = dra7xx_extcon_cable;
>> +	gpio_usbvid->edev.mutually_exclusive = mutually_exclusive;
>> +
>> +	if (of_device_is_compatible(node, "ti,gpio-usb-id"))
>> +		gpio_usbvid->type = ID_DETECT;
>> +
>> +	gpio = of_get_gpio(node, 0);
>> +	if (gpio_is_valid(gpio)) {
>> +		gpio_usbvid->id_gpio = gpio;
>> +		ret = devm_gpio_request(&pdev->dev, gpio_usbvid->id_gpio,
>> +					"id_gpio");
>> +		if (ret)
>> +			return ret;
> Add blank line.
>
>> +		gpio_usbvid->id_irq = gpio_to_irq(gpio_usbvid->id_gpio);
>> +	} else {
>> +		dev_err(&pdev->dev, "failed to get id gpio\n");
>> +		return -ENODEV;
>> +	}
>> +
>> +	if (of_device_is_compatible(node, "ti,gpio-usb-vid")) {
>> +		gpio_usbvid->type = VBUS_ID_DETECT;
>> +		gpio = of_get_gpio(node, 1);
>> +		if (gpio_is_valid(gpio)) {
>> +			gpio_usbvid->vbus_gpio = gpio;
>> +			ret = devm_gpio_request(&pdev->dev,
>> +						gpio_usbvid->vbus_gpio,
>> +						"vbus_gpio");
>> +			if (ret)
>> +				return ret;
> Add blank line.
>
>> +			gpio_usbvid->vbus_irq =
>> +					gpio_to_irq(gpio_usbvid->vbus_gpio);
>> +		} else {
>> +			dev_err(&pdev->dev, "failed to get vbus gpio\n");
>> +			return -ENODEV;
>> +		}
>> +	}
>> +
>> +	ret = extcon_dev_register(&gpio_usbvid->edev, gpio_usbvid->dev);
>> +	if (ret) {
>> +		dev_err(&pdev->dev, "failed to register extcon device\n");
>> +		return ret;
>> +	}
>> +
>> +	gpio_usbvid_set_initial_state(gpio_usbvid);
>> +	ret = gpio_usbvid_request_irq(gpio_usbvid);
> You should move gpio_usbvid_request_irq() call before extcon_dev_register().
>
>> +	if (ret)
>> +		goto err0;
> ? As following previous comment about v1 patchset:
> 	I need correct meaning name as err_thread or etc ...
okay
>
>> +
>> +	return 0;
>> +
>> +err0:
> ditto.
okay
>> +	extcon_dev_unregister(&gpio_usbvid->edev);
>> +
>> +	return ret;
>> +}
>> +
>> +static int gpio_usbvid_remove(struct platform_device *pdev)
>> +{
>> +	struct gpio_usbvid *gpio_usbvid = platform_get_drvdata(pdev);
>> +
>> +	extcon_dev_unregister(&gpio_usbvid->edev);
>> +	return 0;
>> +}
>> +
>> +static struct of_device_id of_gpio_usbvid_match_tbl[] = {
>> +	{ .compatible = "ti,gpio-usb-vid", },
>> +	{ .compatible = "ti,gpio-usb-id", },
>> +	{ /* end */ }
>> +};
>> +
>> +static struct platform_driver gpio_usbvid_driver = {
>> +	.probe = gpio_usbvid_probe,
>> +	.remove = gpio_usbvid_remove,
>> +	.driver = {
>> +		.name = "gpio-usbvid",
>> +		.of_match_table = of_gpio_usbvid_match_tbl,
>> +		.owner = THIS_MODULE,
>> +	},
>> +};
>> +
>> +module_platform_driver(gpio_usbvid_driver);
>> +
>> +MODULE_ALIAS("platform:gpio-usbvid");
>> +MODULE_AUTHOR("George Cherian <george.cherian@ti.com>");
>> +MODULE_DESCRIPTION("GPIO based USB Connector driver");
>> +MODULE_LICENSE("GPL");
>> +MODULE_DEVICE_TABLE(of, of_gpio_usbvid_match_tbl);
>>
> Cheers,
> Chanwoo Choi


-- 
-George

Re: [PATCH v3 3/3] ARM: dts: dra7-evm: Add extcon nodes for USB ID pin detection

[George Cherian]

On 8/28/2013 11:24 PM, Sergei Shtylyov wrote:
> On 08/28/2013 09:33 PM, George Cherian wrote:
>
>> Add
>>     -extcon nodes for USB ID pin detection.
>>     -i2c nodes.
>>     -pcf nodes to which USB ID pin is connected.
>
>> Signed-off-by: George Cherian <george.cherian@ti.com>
>> ---
>>   arch/arm/boot/dts/dra7-evm.dts | 50 
>> +++++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 49 insertions(+), 1 deletion(-)
>
>> diff --git a/arch/arm/boot/dts/dra7-evm.dts 
>> b/arch/arm/boot/dts/dra7-evm.dts
>> index acd3c09..8b0738a 100644
>> --- a/arch/arm/boot/dts/dra7-evm.dts
>> +++ b/arch/arm/boot/dts/dra7-evm.dts
> [...]
>> @@ -33,10 +44,47 @@
>>           };
>>   };
>>
>> +&i2c1 {
>> +    clock-frequency = <400000>;
>> +
>> +    gpio20: pcf8575@20 {
>
>     ePAPR was talking about the node naming, not about labelling. Back 
> to the drawing board. ;-)
>
ha..... :-/
>> +        compatible = "ti,pcf8575";
>> +        reg = <0x20>;
>> +        n_latch = <0x4000>;
>> +        gpio-controller;
>> +        #gpio-cells = <2>;
>> +        interrupt-parent = <&gpio6>;
>> +        interrupts = <11 2>;
>> +        interrupt-controller;
>> +        #interrupt-cells = <2>;
>> +    };
>> +
>> +    gpio21: pcf8575@21 {
>> +        compatible = "ti,pcf8575";
>> +        reg = <0x21>;
>> +        n_latch = <0x1408>;
>> +        gpio-controller;
>> +        #gpio-cells = <2>;
>> +        interrupt-parent = <&pcf_20>;
>> +        interrupts = <14 2>;
>> +        interrupt-controller;
>> +        #interrupt-cells = <2>;
>> +    };
>> +
>> +};
>> +
>
> WBR, Sergei
>


-- 
-George

Re: [PATCH v3 1/3] extcon: extcon-gpio-usbvid: Generic USB VBUS/ID detection via GPIO

[Chanwoo Choi]

Hi George,

On 08/29/2013 11:21 AM, George Cherian wrote:
> Hi Chanwoo,
> 
> Thanks for the review and sorry for all the trivial mistakes.
> 
> On 8/29/2013 7:05 AM, Chanwoo Choi wrote:
>> Hi George,
>>
>> You didn't modify this patchset about my comment on v1 patchset.
>> Please pay attention to comment.
>>
>> On 08/29/2013 02:33 AM, George Cherian wrote:
>>> Add a generic USB VBUS/ID detection EXTCON driver. This driver expects
>>> the ID/VBUS pin are connected via GPIOs. This driver is tested on
>>> DRA7x board were the ID pin is routed via GPIOs. The driver supports
>>> both VBUS and ID pin configuration and ID pin only configuration.
>>>
>>> Signed-off-by: George Cherian <george.cherian@ti.com>
>>> ---
>>>   .../bindings/extcon/extcon-gpio-usbvid.txt         |  20 ++
>>>   drivers/extcon/Kconfig                             |   6 +
>>>   drivers/extcon/Makefile                            |   1 +
>>>   drivers/extcon/extcon-gpio-usbvid.c                | 286 +++++++++++++++++++++
>> You should keep following naming stlye. extcon-gpio-usbvid.c is wrong naming style.
>> - extcon-[device name].c
>> - extcon-gpio-usbvid.c -> extcon-dra7xx.c or etc.
> Actually dra7xx is the SoC name and the USB VBUS/ID detection is not specific to SoC.
> It uses gpios to detect the VBUS/ID change. So i thought it would be better to have generic
> gpio based VBUS/ID detection rather than making dra7xx specific. Stephen Warren had this opinion
> with patch v1.

Would you guarantee that this driver support all of extcon devices using gpio pin?

I can't agree. This driver has specific dependency on dra7x SoC.

First,
vbus_irq_handler() determine USB cable state according to "gpio_usbvid->vbus_gpio" state.
If "gpio_usbvid->vbus_gpio" value is VBUS_OFF(0), this patch set USB cable state as 'false'
But, it include potential problems. Other extcon device using gpio would set USB cable state
as 'true' when gpio state is 1. This way has dependency on specific SoC.

Second,
gpio_usbvid_set_initial_state() dertermine both "USB-HOST" and "USB" cable state at same time
in 'case ID_DETCT' according to 'gpio_usbvid->id_gpio'. But, I think that other extcon devices 
would not control both "USB-HOST" and "USB" cable state at same time. 

Other extcon devices would support either "USB-HOST" or "USB" cable.

Third,
gpio_usbvid_probe() get both gpio_usbvid->id_irq and gpio_usbvid->vbus_irq by using DT helper function.
and gpio_usbvid_request_irq() register each interrupt handler(id_irq and vbus_irq) according to DT data.
In result, 
	id_irq_handler() would control both "USB-HOST" and "USB" cable state.
	vbus_irq_handler() would control only "USB" cable state.

Two interrupt handler(id_irq_handler()/vbus_irq_handler()) would control "USB" cable state
according to each gpio state at same time. Also, It include critical problem.


>>
>> Also, you should change the file name of extcon-gpio-usbvid.txt.
>>
>> Finally, You used 'gpio_usbvid' prefix in extcon-gpio-usbvid.c.
>> It has caused the confusion that user would think extcon-gpio-usbvid.c driver
>> support all of extcon driver using gpio irq pin. So I'd like you to use
>> proper prefix including device name.
> I meant to support all of extcon driver using gpio for USB VBUS/ID detection.
>>
>>>   4 files changed, 313 insertions(+)
>>>   create mode 100644 Documentation/devicetree/bindings/extcon/extcon-gpio-usbvid.txt
>>>   create mode 100644 drivers/extcon/extcon-gpio-usbvid.c
> [snip]
>>> index f1d54a3..8097398 100644
>>> --- a/drivers/extcon/Kconfig
>>> +++ b/drivers/extcon/Kconfig
>>> @@ -64,4 +64,10 @@ config EXTCON_PALMAS
>>>         Say Y here to enable support for USB peripheral and USB host
>>>         detection by palmas usb.
>>>   +config EXTCON_GPIO_USBVID
>>> +    tristate "Generic USB VBUS/ID detection using GPIO EXTCON support"
>>> +    help
>>> +      Say Y here to enable support for USB VBUS/ID deetction by GPIO.
>>> +
>>> +
>> Remove blank line.
> okay
>>>   endif # MULTISTATE_SWITCH
> [snip]
>>> diff --git a/drivers/extcon/extcon-gpio-usbvid.c b/drivers/extcon/extcon-gpio-usbvid.c
>>> new file mode 100644
>>> index 0000000..e9bc2a97
>>> --- /dev/null
>>> +++ b/drivers/extcon/extcon-gpio-usbvid.c
>>> @@ -0,0 +1,286 @@
>>> +/*
>>> + * Generic  USB VBUS-ID pin detection 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: George Cherian <george.cherian@ti.com>
>>> + *
>>> + * Based on extcon-palmas.c
>>> + *
>>> + * 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.
>>> + */
>>> +
>>> +#include <linux/module.h>
>>> +#include <linux/interrupt.h>
>>> +#include <linux/kthread.h>
>>> +#include <linux/freezer.h>
>> kthead.h, freezer.h headerfile is used in this file?
> okay
>>> +#include <linux/platform_device.h>
>>> +#include <linux/extcon.h>
>>> +#include <linux/err.h>
>>> +#include <linux/of.h>
>>> +#include <linux/gpio.h>
>>> +#include <linux/of_gpio.h>
>>> +#include <linux/of_platform.h>
>> Order headerfile alphabetically.
> okay
>>
>>> +
>>> +struct gpio_usbvid {
>>> +    struct device *dev;
>>> +
>>> +    struct extcon_dev edev;
>>> +
>>> +    /*GPIO pin */
>> I commented previous your patch about this wrong coding style.
>> Why did not you fix this coding style?
> okay
>>> +    int id_gpio;
>>> +    int vbus_gpio;
>>> +
>>> +    int id_irq;
>>> +    int vbus_irq;
>>> +    int type;
>>> +};
>>> +
>>> +static const char *dra7xx_extcon_cable[] = {
>>> +    [0] = "USB",
>>> +    [1] = "USB-HOST",
>>> +    NULL,
>>> +};
>>> +
>>> +static const int mutually_exclusive[] = {0x3, 0x0};
>>> +
>>> +/* Two types of support are provided.
>>> + * Systems which has
>>> + *    1) VBUS and ID pin connected via GPIO
>>> + *    2)  only ID pin connected via GPIO
>> Remove blank between '2)' and 'only'.
> okay
>>> + *  For Case 1 both the gpios should be provided via DT
>>> + *  Always the first GPIO in dt is considered ID pin GPIO
>>> + */
>>> +
>>> +enum {
>>> +    UNKNOWN = 0,
>>> +    ID_DETECT,
>>> +    VBUS_ID_DETECT,
>>> +};
>>> +
>>> +#define ID_GND        0
>>> +#define ID_FLOAT    1
>>> +#define VBUS_OFF    0
>>> +#define VBUS_ON        1
>> I think you could only use two constant instead of four constant definition.
> you mean only ID_GND and VBUS_OFF?

you could only define two contant (0 and 1) to detect gpio state.

>>> +
>>> +
>> This blank line isn't necessary.
>>
>>> +static irqreturn_t id_irq_handler(int irq, void *data)
>>> +{
>>> +    struct gpio_usbvid *gpio_usbvid = (struct gpio_usbvid *) data;
>> You should delete blank between ')' and 'data' as follwong:
>>     - (struct gpio_usbvid *)data;
> okay
>>
>>> +    int id_current;
>>> +
>>> +    id_current = gpio_get_value_cansleep(gpio_usbvid->id_gpio);
>>> +    if (id_current == ID_GND) {
>>> +        if (gpio_usbvid->type == ID_DETECT)
>>> +            extcon_set_cable_state(&gpio_usbvid->edev,
>>> +                            "USB", false);
>>> +        extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", true);
>> As else statement, you should set "USB-HOST" cable state to improve readability.
>>
>>         extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", true);
>>         if (gpio_usbvid->type == ID_DETECT)
>>             extcon_set_cable_state(&gpio_usbvid->edev,
>>                             "USB", false);
> Actually, USB-HOST state should be set in the id_irq handler. But in cases were only ID pin is routed to gpio
> and VBUS is not used we set the USB state too. The gpio_usbvid->type differentiates whether its an ID only or
> VBUS and ID.

I don't understand. Wht does not you change the order of function call as following?

	Before:
	        if (gpio_usbvid->type == ID_DETECT)
			extcon_set_cable_state(&gpio_usbvid->edev,
	                            	"USB", false);
		extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", true);

	After:
		extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", true);
		if (gpio_usbvid->type == ID_DETECT)
			extcon_set_cable_state(&gpio_usbvid->edev,
					"USB", false);

id_irq_handler() determine the state of "USB-HOST" cable according to 'id_current' value.
And this function dermine the state of "USB" cable accordign to "gpio_usbvid->type" value.

>>> +    } else {
>>> +        extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", false);
>>> +        if (gpio_usbvid->type == ID_DETECT)
>>> +            extcon_set_cable_state(&gpio_usbvid->edev,
>>> +                            "USB", true);
>>> +    }
>> Add blank line.
> okay
>>> +    return IRQ_HANDLED;
>>> +}
>>> +
>>> +static irqreturn_t vbus_irq_handler(int irq, void *data)
>>> +{
>>> +    struct gpio_usbvid *gpio_usbvid = (struct gpio_usbvid *) data;
>> ditto.
> okay
>>> +    int vbus_current;
>>> +
>>> +    vbus_current = gpio_get_value_cansleep(gpio_usbvid->vbus_gpio);
>>> +    if (vbus_current == VBUS_OFF)
>>> +        extcon_set_cable_state(&gpio_usbvid->edev, "USB", false);
>>> +    else
>>> +        extcon_set_cable_state(&gpio_usbvid->edev, "USB", true);

First,
vbus_irq_handler() determine USB cable state according to "gpio_usbvid->vbus_gpio" state.
If "gpio_usbvid->vbus_gpio" value is VBUS_OFF(0), this patch set USB cable state as 'false'
But, it include potential problems. Other extcon device using gpio would set USB cable state
as 'true' when gpio state is 1. This way has dependency on specific SoC.

>>> +
>>> +    return IRQ_HANDLED;
>>> +}
>>> +
>>> +
>> This blank line isn't necessary.
>> I commented unnecessary blank line on previous review.
> okay
>>> +static void gpio_usbvid_set_initial_state(struct gpio_usbvid *gpio_usbvid)
>>> +{
>>> +    int id_current;
>>> +    int vbus_current;
>> Define loacal variable on one line as following:
>>     int id_current, vbus_current;
> okay
>>> +
>>> +    switch (gpio_usbvid->type) {
>>> +    case ID_DETECT:
>>> +        id_current = gpio_get_value_cansleep(gpio_usbvid->id_gpio);
>>> +        if (!!id_current == ID_FLOAT) {
>>> +            extcon_set_cable_state(&gpio_usbvid->edev,
>>> +                            "USB-HOST", false);
>>> +            extcon_set_cable_state(&gpio_usbvid->edev,
>>> +                            "USB", true);
>>> +        } else {
>>> +            extcon_set_cable_state(&gpio_usbvid->edev,
>>> +                            "USB", false);
>>> +            extcon_set_cable_state(&gpio_usbvid->edev,
>>> +                            "USB-HOST", true);
>>> +        }

Second,
gpio_usbvid_set_initial_state() dertermine both "USB-HOST" and "USB" cable state at same time
in 'case ID_DETCT' according to 'gpio_usbvid->id_gpio'. But, I think that other extcon devices 
would not control both "USB-HOST" and "USB" cable state at same time. 

Other extcon devices would support either "USB-HOST" or "USB" cable.

>>> +        break;
>>> +
>>> +    case VBUS_ID_DETECT:
>>> +        id_current = gpio_get_value_cansleep(gpio_usbvid->id_gpio);
>>> +        if (!!id_current == ID_FLOAT)
>>> +            extcon_set_cable_state(&gpio_usbvid->edev,
>>> +                            "USB-HOST", false);
>>> +        else
>>> +            extcon_set_cable_state(&gpio_usbvid->edev,
>>> +                            "USB-HOST", true);
>>> +
>>> +        vbus_current = gpio_get_value_cansleep(gpio_usbvid->vbus_gpio);
>>> +        if (!!vbus_current == VBUS_ON)
>>> +            extcon_set_cable_state(&gpio_usbvid->edev,
>>> +                            "USB", true);
>>> +        else
>>> +            extcon_set_cable_state(&gpio_usbvid->edev,
>>> +                            "USB", false);
>>> +        break;
>>> +
>>> +    default:
>>> +        dev_err(gpio_usbvid->dev, "Unknown VBUS-ID type\n");
>>> +    }
>>> +}
>>> +
>>> +static int gpio_usbvid_request_irq(struct gpio_usbvid *gpio_usbvid)
>>> +{
>>> +    int ret;
>> Add blank line.
> okay
>>> +    ret = devm_request_threaded_irq(gpio_usbvid->dev, gpio_usbvid->id_irq,
>>> +                    NULL, id_irq_handler,
>>> +                    IRQF_ONESHOT | IRQF_TRIGGER_FALLING,
>>> +                    dev_name(gpio_usbvid->dev),
>>> +                    (void *) gpio_usbvid);
>>> +    if (ret) {
>>> +        dev_err(gpio_usbvid->dev, "failed to request id irq #%d\n",
>>> +                    gpio_usbvid->id_irq);
>>> +        return ret;
>>> +    }
>>> +    if (gpio_usbvid->type == VBUS_ID_DETECT) {
>>> +        ret = devm_request_threaded_irq(gpio_usbvid->dev,
>>> +                    gpio_usbvid->vbus_irq, NULL,
>>> +                    vbus_irq_handler,
>>> +                    IRQF_ONESHOT | IRQF_TRIGGER_FALLING,
>>> +                    dev_name(gpio_usbvid->dev),
>> Why do you use the same interrupt name both gpio_usbvid->id_irq and gpio_usbvid->vbus_irq?
>> You should use characteristic interrupt name.
> if I use dev_name it gives me 2 same name interrupts associated with a single extcon device.
> Where as if i use characteristic (for eg: VBUS or ID) names then I will get multiple with same name
> since there will be 2 instances of extcon being used as of now.

I can't agree. Single extcon driver can have various interrupt.
If you use same interrupt name about different two interrupt,
can we know the kind of interrupt which is happened on /proc/interrupts?
We cannot count the number of each interrupt occurrences.

>>> +                    (void *) gpio_usbvid);
>>> +        if (ret)
>>> +            dev_err(gpio_usbvid->dev, "failed to request vbus irq #%d\n",
>>> +                        gpio_usbvid->vbus_irq);
>>> +    }
>> Add blank line.
> okay
>>> +    return ret;
>>> +}
>>> +
>>> +static int gpio_usbvid_probe(struct platform_device *pdev)
>>> +{
>>> +    struct device_node *node = pdev->dev.of_node;
>>> +    struct gpio_usbvid *gpio_usbvid;
>>> +    int ret;
>>> +    int gpio;
>> Define loacal variable on one line as following:
>>     int ret, gpio;
>>
>>> +
>>> +    gpio_usbvid = devm_kzalloc(&pdev->dev, sizeof(*gpio_usbvid),
>>> +                                GFP_KERNEL);
>> If this statement over 80 line, you have to keep proper indentation as following:
>>     gpio_usbvid = devm_kzalloc(&pdev->dev, sizeof(*gpio_usbvid),
>>                   GFP_KERNEL);
> okay
>>
>>> +    if (!gpio_usbvid)
>>> +        return -ENOMEM;
>>> +
>>> +
>> Remove blank line.
> okay
>>> +    gpio_usbvid->dev     = &pdev->dev;
>> Use space instead of tab.
> okay
>>> +
>>> +    platform_set_drvdata(pdev, gpio_usbvid);
>>> +
>>> +    gpio_usbvid->edev.name = dev_name(&pdev->dev);
>> I add as follwong comment about your v1 patchset
>>
>>     "If edev name is equal with device name, this line is unnecessary.
>>     Because extcon_dev_register() use dev_name(&pdev->dev) as edev name
>>     in extcon-class.c"
>>
>> Why did not apply for my comment to v3 patchset?
>> Plesae pay attention for previous comment.
> I removed it but it gave me a  NULL pointer dereference  in extcon_get_extcon_dev (strcmp the sd->name was NULL).
> I am based on v3.11-rc3, did you have any fix for this in later rc's? probably I would rebase to your latest and check.

Always, you have to develop patch based on extcon-next or extcon-linux branch according to patch content.

This feature related to your NULL pointer issue will include v3.12.
- http://git.kernel.org/cgit/linux/kernel/git/gregkh/char-misc.git/commit/?h=char-misc-next&id=6eee5b3b493824731ed34ade0299241f91f04096

>>> +    gpio_usbvid->edev.supported_cable = dra7xx_extcon_cable;
>>> +    gpio_usbvid->edev.mutually_exclusive = mutually_exclusive;
>>> +
>>> +    if (of_device_is_compatible(node, "ti,gpio-usb-id"))
>>> +        gpio_usbvid->type = ID_DETECT;
>>> +
>>> +    gpio = of_get_gpio(node, 0);
>>> +    if (gpio_is_valid(gpio)) {
>>> +        gpio_usbvid->id_gpio = gpio;
>>> +        ret = devm_gpio_request(&pdev->dev, gpio_usbvid->id_gpio,
>>> +                    "id_gpio");
>>> +        if (ret)
>>> +            return ret;
>> Add blank line.
>>
>>> +        gpio_usbvid->id_irq = gpio_to_irq(gpio_usbvid->id_gpio);
>>> +    } else {
>>> +        dev_err(&pdev->dev, "failed to get id gpio\n");
>>> +        return -ENODEV;
>>> +    }
>>> +
>>> +    if (of_device_is_compatible(node, "ti,gpio-usb-vid")) {
>>> +        gpio_usbvid->type = VBUS_ID_DETECT;
>>> +        gpio = of_get_gpio(node, 1);
>>> +        if (gpio_is_valid(gpio)) {
>>> +            gpio_usbvid->vbus_gpio = gpio;
>>> +            ret = devm_gpio_request(&pdev->dev,
>>> +                        gpio_usbvid->vbus_gpio,
>>> +                        "vbus_gpio");
>>> +            if (ret)
>>> +                return ret;
>> Add blank line.
>>
>>> +            gpio_usbvid->vbus_irq =
>>> +                    gpio_to_irq(gpio_usbvid->vbus_gpio);
>>> +        } else {
>>> +            dev_err(&pdev->dev, "failed to get vbus gpio\n");
>>> +            return -ENODEV;
>>> +        }
>>> +    }
>>> +
>>> +    ret = extcon_dev_register(&gpio_usbvid->edev, gpio_usbvid->dev);
>>> +    if (ret) {
>>> +        dev_err(&pdev->dev, "failed to register extcon device\n");
>>> +        return ret;
>>> +    }
>>> +
>>> +    gpio_usbvid_set_initial_state(gpio_usbvid);
>>> +    ret = gpio_usbvid_request_irq(gpio_usbvid);

Third,
gpio_usbvid_probe() get both gpio_usbvid->id_irq and gpio_usbvid->vbus_irq by using DT helper function.
and gpio_usbvid_request_irq() register each interrupt handler(id_irq and vbus_irq) according to DT data.
In result, 
id_irq_handler() would control both "USB-HOST" and "USB" cable state.
vbus_irq_handler() would control only "USB" cable state.

Two interrupt handler(id_irq_handler()/vbus_irq_handler()) would control "USB" cable state
according to each gpio state at same time. Also, It include critical problem.

>> You should move gpio_usbvid_request_irq() call before extcon_dev_register().
>>
>>> +    if (ret)
>>> +        goto err0;
>> ? As following previous comment about v1 patchset:
>>     I need correct meaning name as err_thread or etc ...
> okay
>>
>>> +
>>> +    return 0;
>>> +
>>> +err0:
>> ditto.
> okay
>>> +    extcon_dev_unregister(&gpio_usbvid->edev);
>>> +
>>> +    return ret;
>>> +}
>>> +
>>> +static int gpio_usbvid_remove(struct platform_device *pdev)
>>> +{
>>> +    struct gpio_usbvid *gpio_usbvid = platform_get_drvdata(pdev);
>>> +
>>> +    extcon_dev_unregister(&gpio_usbvid->edev);
>>> +    return 0;
>>> +}
>>> +
>>> +static struct of_device_id of_gpio_usbvid_match_tbl[] = {
>>> +    { .compatible = "ti,gpio-usb-vid", },
>>> +    { .compatible = "ti,gpio-usb-id", },
>>> +    { /* end */ }
>>> +};
>>> +
>>> +static struct platform_driver gpio_usbvid_driver = {
>>> +    .probe = gpio_usbvid_probe,
>>> +    .remove = gpio_usbvid_remove,
>>> +    .driver = {
>>> +        .name = "gpio-usbvid",
>>> +        .of_match_table = of_gpio_usbvid_match_tbl,
>>> +        .owner = THIS_MODULE,
>>> +    },
>>> +};
>>> +
>>> +module_platform_driver(gpio_usbvid_driver);
>>> +
>>> +MODULE_ALIAS("platform:gpio-usbvid");
>>> +MODULE_AUTHOR("George Cherian <george.cherian@ti.com>");
>>> +MODULE_DESCRIPTION("GPIO based USB Connector driver");
>>> +MODULE_LICENSE("GPL");
>>> +MODULE_DEVICE_TABLE(of, of_gpio_usbvid_match_tbl);
>>>
>> Cheers,
>> Chanwoo Choi
> 
> 

Thanks,
Chanwoo Choi

Re: [PATCH v3 1/3] extcon: extcon-gpio-usbvid: Generic USB VBUS/ID detection via GPIO

[George Cherian]

Hi Chanwoo,

On 8/29/2013 11:53 AM, Chanwoo Choi wrote:
[snip]
> You should keep following naming stlye. extcon-gpio-usbvid.c is wrong naming style.
> - extcon-[device name].c
> - extcon-gpio-usbvid.c -> extcon-dra7xx.c or etc.
>> Actually dra7xx is the SoC name and the USB VBUS/ID detection is not specific to SoC.
>> It uses gpios to detect the VBUS/ID change. So i thought it would be better to have generic
>> gpio based VBUS/ID detection rather than making dra7xx specific. Stephen Warren had this opinion
>> with patch v1.
> Would you guarantee that this driver support all of extcon devices using gpio pin?
This driver would guarantee extcon devices using gpio pins for USB VBUS 
and ID detection.
Following is the basic assumption made, correct me if I am wrong.
ID pin ground means -> USB-HOST is true (this happens when a device is 
connected to USB port and we act as HOST )
ID pin Float means -> USB-HOST is false (this happens when nothing is 
connected or when we act as a peripheral under a HOST)
VBUS ON means -> USB is true (this happens when we are connected under a 
HOST as a peripheral)
VBUS OFF means -> USB is false ( this happens when we are either 
disconnected from a HOST or when we are in HOST mode).

So normally USB is in peripheral mode is enabled when ID pin is float 
and VBUS is ON.
and USB is in HOST mode when ID pin is ground and VBUS is OFF.

In all above cases VBUS is referred to the external VBUS supply from an 
external HOST.

> I can't agree. This driver has specific dependency on dra7x SoC.
>
> First,
> vbus_irq_handler() determine USB cable state according to "gpio_usbvid->vbus_gpio" state.
> If "gpio_usbvid->vbus_gpio" value is VBUS_OFF(0), this patch set USB cable state as 'false'
> But, it include potential problems. Other extcon device using gpio would set USB cable state
> as 'true' when gpio state is 1. This way has dependency on specific SoC.
no this is not SoC specific. VBUS is referred to the external VBUS 
supply from an external HOST.
so if VBUS is zero means its definitely not in connected state.
>
> Second,
> gpio_usbvid_set_initial_state() dertermine both "USB-HOST" and "USB" cable state at same time
> in 'case ID_DETCT' according to 'gpio_usbvid->id_gpio'. But, I think that other extcon devices
> would not control both "USB-HOST" and "USB" cable state at same time.
>
> Other extcon devices would support either "USB-HOST" or "USB" cable.
The driver has 2 configurations.
1) supports implementations with both VBUS and ID pin are routed via 
gpio's for swicthing roles(compatible  gpio-usb-vid).
2) supports implementations with only ID pin routed via gpio for 
swicthing roles(compatible gpio-usb-id).
So if you take type as VBUS_ID_DETECT then it is as what you meant.
>
> Third,
> gpio_usbvid_probe() get both gpio_usbvid->id_irq and gpio_usbvid->vbus_irq by using DT helper function.
> and gpio_usbvid_request_irq() register each interrupt handler(id_irq and vbus_irq) according to DT data.
> In result,
> 	id_irq_handler() would control both "USB-HOST" and "USB" cable state.
only if type is ID_DETECT id_irq_handler control both USB-HOST and USB 
cable states
if type is VBUS_ID_DETECT then id_irq_handler only controls USB-HOST.
> 	vbus_irq_handler() would control only "USB" cable state.
>
> Two interrupt handler(id_irq_handler()/vbus_irq_handler()) would control "USB" cable state
> according to each gpio state at same time. Also, It include critical problem.
No it depends on the configuration as explained above.

[snip]

> +
> +#define ID_GND        0
> +#define ID_FLOAT    1
> +#define VBUS_OFF    0
> +#define VBUS_ON        1
>>> I think you could only use two constant instead of four constant definition.
>> you mean only ID_GND and VBUS_OFF?
> you could only define two contant (0 and 1) to detect gpio state.
okay
>
>>>> +
>>>> +
>>> This blank line isn't necessary.
>>>
>>>> +static irqreturn_t id_irq_handler(int irq, void *data)
>>>> +{
>>>> +    struct gpio_usbvid *gpio_usbvid = (struct gpio_usbvid *) data;
>>> You should delete blank between ')' and 'data' as follwong:
>>>      - (struct gpio_usbvid *)data;
>> okay
>>>> +    int id_current;
>>>> +
>>>> +    id_current = gpio_get_value_cansleep(gpio_usbvid->id_gpio);
>>>> +    if (id_current == ID_GND) {
>>>> +        if (gpio_usbvid->type == ID_DETECT)
>>>> +            extcon_set_cable_state(&gpio_usbvid->edev,
>>>> +                            "USB", false);
>>>> +        extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", true);
>>> As else statement, you should set "USB-HOST" cable state to improve readability.
>>>
>>>          extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", true);
>>>          if (gpio_usbvid->type == ID_DETECT)
>>>              extcon_set_cable_state(&gpio_usbvid->edev,
>>>                              "USB", false);
>> Actually, USB-HOST state should be set in the id_irq handler. But in cases were only ID pin is routed to gpio
>> and VBUS is not used we set the USB state too. The gpio_usbvid->type differentiates whether its an ID only or
>> VBUS and ID.
> I don't understand. Wht does not you change the order of function call as following?
>
> 	Before:
> 	        if (gpio_usbvid->type == ID_DETECT)
> 			extcon_set_cable_state(&gpio_usbvid->edev,
> 	                            	"USB", false);
> 		extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", true);
Basically these notifiers would go and change the UTMI modes which is 
s/w controlled.
so we want to gracefully exit Device mode first and then enter HOST mode.
this is only with type=ID_DETECT.
> 	After:
> 		extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", true);
> 		if (gpio_usbvid->type == ID_DETECT)
> 			extcon_set_cable_state(&gpio_usbvid->edev,
> 					"USB", false);
>
> id_irq_handler() determine the state of "USB-HOST" cable according to 'id_current' value.
> And this function dermine the state of "USB" cable accordign to "gpio_usbvid->type" value.
>
>>>> +    } else {
>>>> +        extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", false);
>>>> +        if (gpio_usbvid->type == ID_DETECT)
>>>> +            extcon_set_cable_state(&gpio_usbvid->edev,
>>>> +                            "USB", true);
>>>> +    }
>>> Add blank line.
>> okay
>>>> +    return IRQ_HANDLED;
>>>> +}
>>>> +
>>>> +static irqreturn_t vbus_irq_handler(int irq, void *data)
>>>> +{
>>>> +    struct gpio_usbvid *gpio_usbvid = (struct gpio_usbvid *) data;
>>> ditto.
>> okay
>>>> +    int vbus_current;
>>>> +
>>>> +    vbus_current = gpio_get_value_cansleep(gpio_usbvid->vbus_gpio);
>>>> +    if (vbus_current == VBUS_OFF)
>>>> +        extcon_set_cable_state(&gpio_usbvid->edev, "USB", false);
>>>> +    else
>>>> +        extcon_set_cable_state(&gpio_usbvid->edev, "USB", true);
> First,
> vbus_irq_handler() determine USB cable state according to "gpio_usbvid->vbus_gpio" state.
> If "gpio_usbvid->vbus_gpio" value is VBUS_OFF(0), this patch set USB cable state as 'false'
> But, it include potential problems. Other extcon device using gpio would set USB cable state
> as 'true' when gpio state is 1. This way has dependency on specific SoC.
see above
[snip]
>>> +
>>> +    switch (gpio_usbvid->type) {
>>> +    case ID_DETECT:
>>> +        id_current = gpio_get_value_cansleep(gpio_usbvid->id_gpio);
>>> +        if (!!id_current == ID_FLOAT) {
>>> +            extcon_set_cable_state(&gpio_usbvid->edev,
>>> +                            "USB-HOST", false);
>>> +            extcon_set_cable_state(&gpio_usbvid->edev,
>>> +                            "USB", true);
>>> +        } else {
>>> +            extcon_set_cable_state(&gpio_usbvid->edev,
>>> +                            "USB", false);
>>> +            extcon_set_cable_state(&gpio_usbvid->edev,
>>> +                            "USB-HOST", true);
>>> +        }
> Second,
> gpio_usbvid_set_initial_state() dertermine both "USB-HOST" and "USB" cable state at same time
> in 'case ID_DETCT' according to 'gpio_usbvid->id_gpio'. But, I think that other extcon devices
> would not control both "USB-HOST" and "USB" cable state at same time.
>
> Other extcon devices would support either "USB-HOST" or "USB" cable.
see above
[snip]
> Why do you use the same interrupt name both gpio_usbvid->id_irq and gpio_usbvid->vbus_irq?
> You should use characteristic interrupt name.
>> if I use dev_name it gives me 2 same name interrupts associated with a single extcon device.
>> Where as if i use characteristic (for eg: VBUS or ID) names then I will get multiple with same name
>> since there will be 2 instances of extcon being used as of now.
> I can't agree. Single extcon driver can have various interrupt.
> If you use same interrupt name about different two interrupt,
> can we know the kind of interrupt which is happened on /proc/interrupts?
> We cannot count the number of each interrupt occurrences.
ya so i will use something like "extcon_devname + VBUS/ID"
otherwise if i have 3 instance of extcon device in type VBUS_ID_DETECT 
then i will have
2 entries named VBUS and 2 named ID.
I will change it.
[snip]
> +
> +    platform_set_drvdata(pdev, gpio_usbvid);
> +
> +    gpio_usbvid->edev.name = dev_name(&pdev->dev);
>>> I add as follwong comment about your v1 patchset
>>>
>>>      "If edev name is equal with device name, this line is unnecessary.
>>>      Because extcon_dev_register() use dev_name(&pdev->dev) as edev name
>>>      in extcon-class.c"
>>>
>>> Why did not apply for my comment to v3 patchset?
>>> Plesae pay attention for previous comment.
>> I removed it but it gave me a  NULL pointer dereference  in extcon_get_extcon_dev (strcmp the sd->name was NULL).
>> I am based on v3.11-rc3, did you have any fix for this in later rc's? probably I would rebase to your latest and check.
> Always, you have to develop patch based on extcon-next or extcon-linux branch according to patch content.
>
> This feature related to your NULL pointer issue will include v3.12.
> - http://git.kernel.org/cgit/linux/kernel/git/gregkh/char-misc.git/commit/?h=char-misc-next&id=6eee5b3b493824731ed34ade0299241f91f04096
guilty as charged i will rebase.
>>>> +    gpio_usbvid->edev.supported_cable = dra7xx_extcon_cable;
>>>> +    gpio_usbvid->edev.mutually_exclusive = mutually_exclusive;
>>>> +
>>>> +    if (of_device_is_compatible(node, "ti,gpio-usb-id"))
>>>> +        gpio_usbvid->type = ID_DETECT;
>>>> +
>>>> +    gpio = of_get_gpio(node, 0);
>>>> +    if (gpio_is_valid(gpio)) {
>>>> +        gpio_usbvid->id_gpio = gpio;
>>>> +        ret = devm_gpio_request(&pdev->dev, gpio_usbvid->id_gpio,
>>>> +                    "id_gpio");
>>>> +        if (ret)
>>>> +            return ret;
>>> Add blank line.
>>>
>>>> +        gpio_usbvid->id_irq = gpio_to_irq(gpio_usbvid->id_gpio);
>>>> +    } else {
>>>> +        dev_err(&pdev->dev, "failed to get id gpio\n");
>>>> +        return -ENODEV;
>>>> +    }
>>>> +
>>>> +    if (of_device_is_compatible(node, "ti,gpio-usb-vid")) {
>>>> +        gpio_usbvid->type = VBUS_ID_DETECT;
>>>> +        gpio = of_get_gpio(node, 1);
>>>> +        if (gpio_is_valid(gpio)) {
>>>> +            gpio_usbvid->vbus_gpio = gpio;
>>>> +            ret = devm_gpio_request(&pdev->dev,
>>>> +                        gpio_usbvid->vbus_gpio,
>>>> +                        "vbus_gpio");
>>>> +            if (ret)
>>>> +                return ret;
>>> Add blank line.
>>>
>>>> +            gpio_usbvid->vbus_irq =
>>>> +                    gpio_to_irq(gpio_usbvid->vbus_gpio);
>>>> +        } else {
>>>> +            dev_err(&pdev->dev, "failed to get vbus gpio\n");
>>>> +            return -ENODEV;
>>>> +        }
>>>> +    }
>>>> +
>>>> +    ret = extcon_dev_register(&gpio_usbvid->edev, gpio_usbvid->dev);
>>>> +    if (ret) {
>>>> +        dev_err(&pdev->dev, "failed to register extcon device\n");
>>>> +        return ret;
>>>> +    }
>>>> +
>>>> +    gpio_usbvid_set_initial_state(gpio_usbvid);
>>>> +    ret = gpio_usbvid_request_irq(gpio_usbvid);
> Third,
> gpio_usbvid_probe() get both gpio_usbvid->id_irq and gpio_usbvid->vbus_irq by using DT helper function.
> and gpio_usbvid_request_irq() register each interrupt handler(id_irq and vbus_irq) according to DT data.
> In result,
> id_irq_handler() would control both "USB-HOST" and "USB" cable state.
> vbus_irq_handler() would control only "USB" cable state.
>
> Two interrupt handler(id_irq_handler()/vbus_irq_handler()) would control "USB" cable state
> according to each gpio state at same time. Also, It include critical problem.
see above
>>> You should move gpio_usbvid_request_irq() call before extcon_dev_register().
>>>
>>>> +    if (ret)
>>>> +        goto err0;
>>> ? As following previous comment about v1 patchset:
>>>      I need correct meaning name as err_thread or etc ...
>> okay
>>>> +
>>>> +    return 0;
>>>> +
>>>> +err0:
>>> ditto.
>> okay
>>>> +    extcon_dev_unregister(&gpio_usbvid->edev);
>>>> +
>>>> +    return ret;
>>>> +}
>>>> +
>>>> +static int gpio_usbvid_remove(struct platform_device *pdev)
>>>> +{
>>>> +    struct gpio_usbvid *gpio_usbvid = platform_get_drvdata(pdev);
>>>> +
>>>> +    extcon_dev_unregister(&gpio_usbvid->edev);
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +static struct of_device_id of_gpio_usbvid_match_tbl[] = {
>>>> +    { .compatible = "ti,gpio-usb-vid", },
>>>> +    { .compatible = "ti,gpio-usb-id", },
>>>> +    { /* end */ }
>>>> +};
>>>> +
>>>> +static struct platform_driver gpio_usbvid_driver = {
>>>> +    .probe = gpio_usbvid_probe,
>>>> +    .remove = gpio_usbvid_remove,
>>>> +    .driver = {
>>>> +        .name = "gpio-usbvid",
>>>> +        .of_match_table = of_gpio_usbvid_match_tbl,
>>>> +        .owner = THIS_MODULE,
>>>> +    },
>>>> +};
>>>> +
>>>> +module_platform_driver(gpio_usbvid_driver);
>>>> +
>>>> +MODULE_ALIAS("platform:gpio-usbvid");
>>>> +MODULE_AUTHOR("George Cherian <george.cherian@ti.com>");
>>>> +MODULE_DESCRIPTION("GPIO based USB Connector driver");
>>>> +MODULE_LICENSE("GPL");
>>>> +MODULE_DEVICE_TABLE(of, of_gpio_usbvid_match_tbl);
>>>>
>>> Cheers,
>>> Chanwoo Choi
>>
> Thanks,
> Chanwoo Choi
>


-- 
-George

Re: [PATCH v3 1/3] extcon: extcon-gpio-usbvid: Generic USB VBUS/ID detection via GPIO

[Chanwoo Choi]

On 08/29/2013 04:30 PM, George Cherian wrote:
> Hi Chanwoo,
> 
> On 8/29/2013 11:53 AM, Chanwoo Choi wrote:
> [snip]
>> You should keep following naming stlye. extcon-gpio-usbvid.c is wrong naming style.
>> - extcon-[device name].c
>> - extcon-gpio-usbvid.c -> extcon-dra7xx.c or etc.
>>> Actually dra7xx is the SoC name and the USB VBUS/ID detection is not specific to SoC.
>>> It uses gpios to detect the VBUS/ID change. So i thought it would be better to have generic
>>> gpio based VBUS/ID detection rather than making dra7xx specific. Stephen Warren had this opinion
>>> with patch v1.
>> Would you guarantee that this driver support all of extcon devices using gpio pin?
> This driver would guarantee extcon devices using gpio pins for USB VBUS and ID detection.
> Following is the basic assumption made, correct me if I am wrong.
> ID pin ground means -> USB-HOST is true (this happens when a device is connected to USB port and we act as HOST )
> ID pin Float means -> USB-HOST is false (this happens when nothing is connected or when we act as a peripheral under a HOST)
> VBUS ON means -> USB is true (this happens when we are connected under a HOST as a peripheral)
> VBUS OFF means -> USB is false ( this happens when we are either disconnected from a HOST or when we are in HOST mode).
> 
> So normally USB is in peripheral mode is enabled when ID pin is float and VBUS is ON.
> and USB is in HOST mode when ID pin is ground and VBUS is OFF.
> 
> In all above cases VBUS is referred to the external VBUS supply from an external HOST.
> 
>> I can't agree. This driver has specific dependency on dra7x SoC.
>>
>> First,
>> vbus_irq_handler() determine USB cable state according to "gpio_usbvid->vbus_gpio" state.
>> If "gpio_usbvid->vbus_gpio" value is VBUS_OFF(0), this patch set USB cable state as 'false'
>> But, it include potential problems. Other extcon device using gpio would set USB cable state
>> as 'true' when gpio state is 1. This way has dependency on specific SoC.
> no this is not SoC specific. VBUS is referred to the external VBUS supply from an external HOST.
> so if VBUS is zero means its definitely not in connected state.

I tested various development board based on Samsung Exynos series SoC.
Although some gpio of Exynos series SoC set high state(non zero, 1) as default value,
this gpio state could mean off state, disconnected or un-powered state according to gpio.
Of course, above explanation about specific gpio don't include all gpios.
This is meaning that there is possibility.

>>
>> Second,
>> gpio_usbvid_set_initial_state() dertermine both "USB-HOST" and "USB" cable state at same time
>> in 'case ID_DETCT' according to 'gpio_usbvid->id_gpio'. But, I think that other extcon devices
>> would not control both "USB-HOST" and "USB" cable state at same time.
>>
>> Other extcon devices would support either "USB-HOST" or "USB" cable.
> The driver has 2 configurations.
> 1) supports implementations with both VBUS and ID pin are routed via gpio's for swicthing roles(compatible  gpio-usb-vid).

As you explained about case 1, it is only used on dra7x SoC.
Other SoC could not wish to control both "USB-HOST" and "USB" cable at same time.


> 2) supports implementations with only ID pin routed via gpio for swicthing roles(compatible gpio-usb-id).
> So if you take type as VBUS_ID_DETECT then it is as what you meant.

"2) case" don't support the detection of "USB-HOST" cable.
Only detect "USB" cable according to "vbus_gpio" value.

If user wish to detect only "USB-HOST" cable, should user enter "ID_DETECT" as "ti,gpio-usb-id" on DT file?
But, id_irq_handler() control both "USB-HOST" and "USB" cable at same time. It may not prefer this method.

>>
>> Third,
>> gpio_usbvid_probe() get both gpio_usbvid->id_irq and gpio_usbvid->vbus_irq by using DT helper function.
>> and gpio_usbvid_request_irq() register each interrupt handler(id_irq and vbus_irq) according to DT data.
>> In result,
>>     id_irq_handler() would control both "USB-HOST" and "USB" cable state.
> only if type is ID_DETECT id_irq_handler control both USB-HOST and USB cable states
> if type is VBUS_ID_DETECT then id_irq_handler only controls USB-HOST.

I have some confusion. I need additional your explanation.
Could this driver register only one interrupt handler either id_irq_handler() or vbus_irq_handler()?
or
Could this driver register two interrupt handler both id_irq_handler() or vbus_irq_handler()?

>>     vbus_irq_handler() would control only "USB" cable state.
>>
>> Two interrupt handler(id_irq_handler()/vbus_irq_handler()) would control "USB" cable state
>> according to each gpio state at same time. Also, It include critical problem.
> No it depends on the configuration as explained above.
> 
> [snip]
> 
>> +
>> +#define ID_GND        0
>> +#define ID_FLOAT    1
>> +#define VBUS_OFF    0
>> +#define VBUS_ON        1
>>>> I think you could only use two constant instead of four constant definition.
>>> you mean only ID_GND and VBUS_OFF?
>> you could only define two contant (0 and 1) to detect gpio state.
> okay
>>
>>>>> +
>>>>> +
>>>> This blank line isn't necessary.
>>>>
>>>>> +static irqreturn_t id_irq_handler(int irq, void *data)
>>>>> +{
>>>>> +    struct gpio_usbvid *gpio_usbvid = (struct gpio_usbvid *) data;
>>>> You should delete blank between ')' and 'data' as follwong:
>>>>      - (struct gpio_usbvid *)data;
>>> okay
>>>>> +    int id_current;
>>>>> +
>>>>> +    id_current = gpio_get_value_cansleep(gpio_usbvid->id_gpio);
>>>>> +    if (id_current == ID_GND) {
>>>>> +        if (gpio_usbvid->type == ID_DETECT)
>>>>> +            extcon_set_cable_state(&gpio_usbvid->edev,
>>>>> +                            "USB", false);
>>>>> +        extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", true);
>>>> As else statement, you should set "USB-HOST" cable state to improve readability.
>>>>
>>>>          extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", true);
>>>>          if (gpio_usbvid->type == ID_DETECT)
>>>>              extcon_set_cable_state(&gpio_usbvid->edev,
>>>>                              "USB", false);
>>> Actually, USB-HOST state should be set in the id_irq handler. But in cases were only ID pin is routed to gpio
>>> and VBUS is not used we set the USB state too. The gpio_usbvid->type differentiates whether its an ID only or
>>> VBUS and ID.
>> I don't understand. Wht does not you change the order of function call as following?
>>
>>     Before:
>>             if (gpio_usbvid->type == ID_DETECT)
>>             extcon_set_cable_state(&gpio_usbvid->edev,
>>                                     "USB", false);
>>         extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", true);
> Basically these notifiers would go and change the UTMI modes which is s/w controlled.
> so we want to gracefully exit Device mode first and then enter HOST mode.
> this is only with type=ID_DETECT.

Because id_irq_handler() control both "USB-HOST" and "USB" cable at same time,
you need this setting order between "USB-HOST" and "USB" cable.

Did you think that SoC should always connect either "USB-HOST" cable or "USB" cable?
I don't know this case except for dra7x SoC. So, I think it has dependency on specific SoC.
and can't agree as generic extcon gpio driver.

>>     After:
>>         extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", true);
>>         if (gpio_usbvid->type == ID_DETECT)
>>             extcon_set_cable_state(&gpio_usbvid->edev,
>>                     "USB", false);
>>
>> id_irq_handler() determine the state of "USB-HOST" cable according to 'id_current' value.
>> And this function dermine the state of "USB" cable accordign to "gpio_usbvid->type" value.
>>
>>>>> +    } else {
>>>>> +        extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", false);
>>>>> +        if (gpio_usbvid->type == ID_DETECT)
>>>>> +            extcon_set_cable_state(&gpio_usbvid->edev,
>>>>> +                            "USB", true);
>>>>> +    }
>>>> Add blank line.
>>> okay
>>>>> +    return IRQ_HANDLED;
>>>>> +}
>>>>> +
>>>>> +static irqreturn_t vbus_irq_handler(int irq, void *data)
>>>>> +{
>>>>> +    struct gpio_usbvid *gpio_usbvid = (struct gpio_usbvid *) data;
>>>> ditto.
>>> okay
>>>>> +    int vbus_current;
>>>>> +
>>>>> +    vbus_current = gpio_get_value_cansleep(gpio_usbvid->vbus_gpio);
>>>>> +    if (vbus_current == VBUS_OFF)
>>>>> +        extcon_set_cable_state(&gpio_usbvid->edev, "USB", false);
>>>>> +    else
>>>>> +        extcon_set_cable_state(&gpio_usbvid->edev, "USB", true);
>> First,
>> vbus_irq_handler() determine USB cable state according to "gpio_usbvid->vbus_gpio" state.
>> If "gpio_usbvid->vbus_gpio" value is VBUS_OFF(0), this patch set USB cable state as 'false'
>> But, it include potential problems. Other extcon device using gpio would set USB cable state
>> as 'true' when gpio state is 1. This way has dependency on specific SoC.
> see above
> [snip]
>>>> +
>>>> +    switch (gpio_usbvid->type) {
>>>> +    case ID_DETECT:
>>>> +        id_current = gpio_get_value_cansleep(gpio_usbvid->id_gpio);
>>>> +        if (!!id_current == ID_FLOAT) {
>>>> +            extcon_set_cable_state(&gpio_usbvid->edev,
>>>> +                            "USB-HOST", false);
>>>> +            extcon_set_cable_state(&gpio_usbvid->edev,
>>>> +                            "USB", true);
>>>> +        } else {
>>>> +            extcon_set_cable_state(&gpio_usbvid->edev,
>>>> +                            "USB", false);
>>>> +            extcon_set_cable_state(&gpio_usbvid->edev,
>>>> +                            "USB-HOST", true);
>>>> +        }
>> Second,
>> gpio_usbvid_set_initial_state() dertermine both "USB-HOST" and "USB" cable state at same time
>> in 'case ID_DETCT' according to 'gpio_usbvid->id_gpio'. But, I think that other extcon devices
>> would not control both "USB-HOST" and "USB" cable state at same time.
>>
>> Other extcon devices would support either "USB-HOST" or "USB" cable.
> see above
> [snip]
>> Why do you use the same interrupt name both gpio_usbvid->id_irq and gpio_usbvid->vbus_irq?
>> You should use characteristic interrupt name.
>>> if I use dev_name it gives me 2 same name interrupts associated with a single extcon device.
>>> Where as if i use characteristic (for eg: VBUS or ID) names then I will get multiple with same name
>>> since there will be 2 instances of extcon being used as of now.
>> I can't agree. Single extcon driver can have various interrupt.
>> If you use same interrupt name about different two interrupt,
>> can we know the kind of interrupt which is happened on /proc/interrupts?
>> We cannot count the number of each interrupt occurrences.
> ya so i will use something like "extcon_devname + VBUS/ID"
> otherwise if i have 3 instance of extcon device in type VBUS_ID_DETECT then i will have
> 2 entries named VBUS and 2 named ID.
> I will change it.
> [snip]
>> +
>> +    platform_set_drvdata(pdev, gpio_usbvid);
>> +
>> +    gpio_usbvid->edev.name = dev_name(&pdev->dev);
>>>> I add as follwong comment about your v1 patchset
>>>>
>>>>      "If edev name is equal with device name, this line is unnecessary.
>>>>      Because extcon_dev_register() use dev_name(&pdev->dev) as edev name
>>>>      in extcon-class.c"
>>>>
>>>> Why did not apply for my comment to v3 patchset?
>>>> Plesae pay attention for previous comment.
>>> I removed it but it gave me a  NULL pointer dereference  in extcon_get_extcon_dev (strcmp the sd->name was NULL).
>>> I am based on v3.11-rc3, did you have any fix for this in later rc's? probably I would rebase to your latest and check.
>> Always, you have to develop patch based on extcon-next or extcon-linux branch according to patch content.
>>
>> This feature related to your NULL pointer issue will include v3.12.
>> - http://git.kernel.org/cgit/linux/kernel/git/gregkh/char-misc.git/commit/?h=char-misc-next&id=6eee5b3b493824731ed34ade0299241f91f04096
> guilty as charged i will rebase.
>>>>> +    gpio_usbvid->edev.supported_cable = dra7xx_extcon_cable;
>>>>> +    gpio_usbvid->edev.mutually_exclusive = mutually_exclusive;
>>>>> +
>>>>> +    if (of_device_is_compatible(node, "ti,gpio-usb-id"))
>>>>> +        gpio_usbvid->type = ID_DETECT;
>>>>> +
>>>>> +    gpio = of_get_gpio(node, 0);
>>>>> +    if (gpio_is_valid(gpio)) {
>>>>> +        gpio_usbvid->id_gpio = gpio;
>>>>> +        ret = devm_gpio_request(&pdev->dev, gpio_usbvid->id_gpio,
>>>>> +                    "id_gpio");
>>>>> +        if (ret)
>>>>> +            return ret;
>>>> Add blank line.
>>>>
>>>>> +        gpio_usbvid->id_irq = gpio_to_irq(gpio_usbvid->id_gpio);
>>>>> +    } else {
>>>>> +        dev_err(&pdev->dev, "failed to get id gpio\n");
>>>>> +        return -ENODEV;
>>>>> +    }
>>>>> +
>>>>> +    if (of_device_is_compatible(node, "ti,gpio-usb-vid")) {
>>>>> +        gpio_usbvid->type = VBUS_ID_DETECT;
>>>>> +        gpio = of_get_gpio(node, 1);
>>>>> +        if (gpio_is_valid(gpio)) {
>>>>> +            gpio_usbvid->vbus_gpio = gpio;
>>>>> +            ret = devm_gpio_request(&pdev->dev,
>>>>> +                        gpio_usbvid->vbus_gpio,
>>>>> +                        "vbus_gpio");
>>>>> +            if (ret)
>>>>> +                return ret;
>>>> Add blank line.
>>>>
>>>>> +            gpio_usbvid->vbus_irq =
>>>>> +                    gpio_to_irq(gpio_usbvid->vbus_gpio);
>>>>> +        } else {
>>>>> +            dev_err(&pdev->dev, "failed to get vbus gpio\n");
>>>>> +            return -ENODEV;
>>>>> +        }
>>>>> +    }
>>>>> +
>>>>> +    ret = extcon_dev_register(&gpio_usbvid->edev, gpio_usbvid->dev);
>>>>> +    if (ret) {
>>>>> +        dev_err(&pdev->dev, "failed to register extcon device\n");
>>>>> +        return ret;
>>>>> +    }
>>>>> +
>>>>> +    gpio_usbvid_set_initial_state(gpio_usbvid);
>>>>> +    ret = gpio_usbvid_request_irq(gpio_usbvid);
>> Third,
>> gpio_usbvid_probe() get both gpio_usbvid->id_irq and gpio_usbvid->vbus_irq by using DT helper function.
>> and gpio_usbvid_request_irq() register each interrupt handler(id_irq and vbus_irq) according to DT data.
>> In result,
>> id_irq_handler() would control both "USB-HOST" and "USB" cable state.
>> vbus_irq_handler() would control only "USB" cable state.
>>
>> Two interrupt handler(id_irq_handler()/vbus_irq_handler()) would control "USB" cable state
>> according to each gpio state at same time. Also, It include critical problem.
> see above
>>>> You should move gpio_usbvid_request_irq() call before extcon_dev_register().
>>>>
>>>>> +    if (ret)
>>>>> +        goto err0;
>>>> ? As following previous comment about v1 patchset:
>>>>      I need correct meaning name as err_thread or etc ...
>>> okay
>>>>> +
>>>>> +    return 0;
>>>>> +
>>>>> +err0:
>>>> ditto.
>>> okay
>>>>> +    extcon_dev_unregister(&gpio_usbvid->edev);
>>>>> +
>>>>> +    return ret;
>>>>> +}
>>>>> +
>>>>> +static int gpio_usbvid_remove(struct platform_device *pdev)
>>>>> +{
>>>>> +    struct gpio_usbvid *gpio_usbvid = platform_get_drvdata(pdev);
>>>>> +
>>>>> +    extcon_dev_unregister(&gpio_usbvid->edev);
>>>>> +    return 0;
>>>>> +}
>>>>> +
>>>>> +static struct of_device_id of_gpio_usbvid_match_tbl[] = {
>>>>> +    { .compatible = "ti,gpio-usb-vid", },
>>>>> +    { .compatible = "ti,gpio-usb-id", },
>>>>> +    { /* end */ }
>>>>> +};
>>>>> +
>>>>> +static struct platform_driver gpio_usbvid_driver = {
>>>>> +    .probe = gpio_usbvid_probe,
>>>>> +    .remove = gpio_usbvid_remove,
>>>>> +    .driver = {
>>>>> +        .name = "gpio-usbvid",
>>>>> +        .of_match_table = of_gpio_usbvid_match_tbl,
>>>>> +        .owner = THIS_MODULE,
>>>>> +    },
>>>>> +};
>>>>> +
>>>>> +module_platform_driver(gpio_usbvid_driver);
>>>>> +
>>>>> +MODULE_ALIAS("platform:gpio-usbvid");
>>>>> +MODULE_AUTHOR("George Cherian <george.cherian@ti.com>");
>>>>> +MODULE_DESCRIPTION("GPIO based USB Connector driver");
>>>>> +MODULE_LICENSE("GPL");
>>>>> +MODULE_DEVICE_TABLE(of, of_gpio_usbvid_match_tbl);
>>>>>
>>>> Cheers,
>>>> Chanwoo Choi
>>>
>> Thanks,
>> Chanwoo Choi
>>
> 

Re: [PATCH v3 1/3] extcon: extcon-gpio-usbvid: Generic USB VBUS/ID detection via GPIO

[George Cherian]

On 8/29/2013 4:07 PM, Chanwoo Choi wrote:
> On 08/29/2013 04:30 PM, George Cherian wrote:
>> Hi Chanwoo,
>>
>> On 8/29/2013 11:53 AM, Chanwoo Choi wrote:
>> [snip]
>>> You should keep following naming stlye. extcon-gpio-usbvid.c is wrong naming style.
>>> - extcon-[device name].c
>>> - extcon-gpio-usbvid.c -> extcon-dra7xx.c or etc.
>>>> Actually dra7xx is the SoC name and the USB VBUS/ID detection is not specific to SoC.
>>>> It uses gpios to detect the VBUS/ID change. So i thought it would be better to have generic
>>>> gpio based VBUS/ID detection rather than making dra7xx specific. Stephen Warren had this opinion
>>>> with patch v1.
>>> Would you guarantee that this driver support all of extcon devices using gpio pin?
>> This driver would guarantee extcon devices using gpio pins for USB VBUS and ID detection.
>> Following is the basic assumption made, correct me if I am wrong.
>> ID pin ground means -> USB-HOST is true (this happens when a device is connected to USB port and we act as HOST )
>> ID pin Float means -> USB-HOST is false (this happens when nothing is connected or when we act as a peripheral under a HOST)
>> VBUS ON means -> USB is true (this happens when we are connected under a HOST as a peripheral)
>> VBUS OFF means -> USB is false ( this happens when we are either disconnected from a HOST or when we are in HOST mode).
>>
>> So normally USB is in peripheral mode is enabled when ID pin is float and VBUS is ON.
>> and USB is in HOST mode when ID pin is ground and VBUS is OFF.
>>
>> In all above cases VBUS is referred to the external VBUS supply from an external HOST.
>>
>>> I can't agree. This driver has specific dependency on dra7x SoC.
>>>
>>> First,
>>> vbus_irq_handler() determine USB cable state according to "gpio_usbvid->vbus_gpio" state.
>>> If "gpio_usbvid->vbus_gpio" value is VBUS_OFF(0), this patch set USB cable state as 'false'
>>> But, it include potential problems. Other extcon device using gpio would set USB cable state
>>> as 'true' when gpio state is 1. This way has dependency on specific SoC.
>> no this is not SoC specific. VBUS is referred to the external VBUS supply from an external HOST.
>> so if VBUS is zero means its definitely not in connected state.
> I tested various development board based on Samsung Exynos series SoC.
> Although some gpio of Exynos series SoC set high state(non zero, 1) as default value,
> this gpio state could mean off state, disconnected or un-powered state according to gpio.
> Of course, above explanation about specific gpio don't include all gpios.
> This is meaning that there is possibility.
okay then how about adding a flag for inverted logic too.  something 
like this

if(of_property_read_bool(np,"inverted_gpio))
     gpio_usbvid->gpio_inv = 1;
And always check on this before deciding?

>>> Second,
>>> gpio_usbvid_set_initial_state() dertermine both "USB-HOST" and "USB" cable state at same time
>>> in 'case ID_DETCT' according to 'gpio_usbvid->id_gpio'. But, I think that other extcon devices
>>> would not control both "USB-HOST" and "USB" cable state at same time.
>>>
>>> Other extcon devices would support either "USB-HOST" or "USB" cable.
>> The driver has 2 configurations.
>> 1) supports implementations with both VBUS and ID pin are routed via gpio's for swicthing roles(compatible  gpio-usb-vid).
> As you explained about case 1, it is only used on dra7x SoC.
No gpio-usb-id is used in dra7xx. dra7xx has got only ID pin routed via 
gpio.
> Other SoC could not wish to control both "USB-HOST" and "USB" cable at same time.
>
>
>> 2) supports implementations with only ID pin routed via gpio for swicthing roles(compatible gpio-usb-id).
>> So if you take type as VBUS_ID_DETECT then it is as what you meant.
I think i didnt explain it properly last time.
In perfect world you will have both VBUS and ID pin routed via gpios
for eg: VBUS via gpio2 and ID via gpio3. if this is the case then we 
have to use compatible gpio-usb-vid where in 2 gpios will be used
2 irq handlers will be installed and it will control both USB-HOST and 
USB cables. In this case its possible to have 3 states
USB-HOST (true), USB(true) and both false.

Now in case of dra7xx the board designers chose not to route the VBUS to 
gpio and only ID pin is routed.
But still we need to  differentiate USB-HOST and USB cables In such 
cases we use compatible gpio-usb-id where in 1 gpio will be used
1 irq handler is installed  and it will control both USB-HOST  and USB 
cables. In this case its possible to have only 2 states
USB-HOST (true) or USB(true).

Now there could be a 3rd scenario were in only VBUS is routed via gpio, 
that we would not support since we cant assume the value of ID pin
and configure the UTMI is not proper. So I have mentioned even in 
documentation that ID pin is mandatory. We can always assume role 
depending on ID pin.
> "2) case" don't support the detection of "USB-HOST" cable.
> Only detect "USB" cable according to "vbus_gpio" value.
>
> If user wish to detect only "USB-HOST" cable, should user enter "ID_DETECT" as "ti,gpio-usb-id" on DT file?
> But, id_irq_handler() control both "USB-HOST" and "USB" cable at same time. It may not prefer this method.
>
>>> Third,
>>> gpio_usbvid_probe() get both gpio_usbvid->id_irq and gpio_usbvid->vbus_irq by using DT helper function.
>>> and gpio_usbvid_request_irq() register each interrupt handler(id_irq and vbus_irq) according to DT data.
>>> In result,
>>>      id_irq_handler() would control both "USB-HOST" and "USB" cable state.
>> only if type is ID_DETECT id_irq_handler control both USB-HOST and USB cable states
>> if type is VBUS_ID_DETECT then id_irq_handler only controls USB-HOST.
> I have some confusion. I need additional your explanation.
> Could this driver register only one interrupt handler either id_irq_handler() or vbus_irq_handler()?
If compatible == ID_DETECT, only one handler --> id_irq_handler() and it 
will handle both USB and USB-HOST
> or
> Could this driver register two interrupt handler both id_irq_handler() or vbus_irq_handler()?
If compatible == VBUS_ID_DETECT, 2 handlers --> id_irq_handler() will 
handle USB-HOST and vbus_irq_handler will handle USB.
>>>      vbus_irq_handler() would control only "USB" cable state.
>>>
>>> Two interrupt handler(id_irq_handler()/vbus_irq_handler()) would control "USB" cable state
>>> according to each gpio state at same time. Also, It include critical problem.
>> No it depends on the configuration as explained above.
>>
>> [snip]
>>
>>> +
>>> +#define ID_GND        0
>>> +#define ID_FLOAT    1
>>> +#define VBUS_OFF    0
>>> +#define VBUS_ON        1
>>>>> I think you could only use two constant instead of four constant definition.
>>>> you mean only ID_GND and VBUS_OFF?
>>> you could only define two contant (0 and 1) to detect gpio state.
>> okay
>>>>>> +
>>>>>> +
>>>>> This blank line isn't necessary.
>>>>>
>>>>>> +static irqreturn_t id_irq_handler(int irq, void *data)
>>>>>> +{
>>>>>> +    struct gpio_usbvid *gpio_usbvid = (struct gpio_usbvid *) data;
>>>>> You should delete blank between ')' and 'data' as follwong:
>>>>>       - (struct gpio_usbvid *)data;
>>>> okay
>>>>>> +    int id_current;
>>>>>> +
>>>>>> +    id_current = gpio_get_value_cansleep(gpio_usbvid->id_gpio);
>>>>>> +    if (id_current == ID_GND) {
>>>>>> +        if (gpio_usbvid->type == ID_DETECT)
>>>>>> +            extcon_set_cable_state(&gpio_usbvid->edev,
>>>>>> +                            "USB", false);
>>>>>> +        extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", true);
>>>>> As else statement, you should set "USB-HOST" cable state to improve readability.
>>>>>
>>>>>           extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", true);
>>>>>           if (gpio_usbvid->type == ID_DETECT)
>>>>>               extcon_set_cable_state(&gpio_usbvid->edev,
>>>>>                               "USB", false);
>>>> Actually, USB-HOST state should be set in the id_irq handler. But in cases were only ID pin is routed to gpio
>>>> and VBUS is not used we set the USB state too. The gpio_usbvid->type differentiates whether its an ID only or
>>>> VBUS and ID.
>>> I don't understand. Wht does not you change the order of function call as following?
>>>
>>>      Before:
>>>              if (gpio_usbvid->type == ID_DETECT)
>>>              extcon_set_cable_state(&gpio_usbvid->edev,
>>>                                      "USB", false);
>>>          extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", true);
>> Basically these notifiers would go and change the UTMI modes which is s/w controlled.
>> so we want to gracefully exit Device mode first and then enter HOST mode.
>> this is only with type=ID_DETECT.
> Because id_irq_handler() control both "USB-HOST" and "USB" cable at same time,
> you need this setting order between "USB-HOST" and "USB" cable.
yes
> Did you think that SoC should always connect either "USB-HOST" cable or "USB" cable?
No,  even if a physical cable is not connected it should  default to 
either USB-HOST or USB

> I don't know this case except for dra7x SoC. So, I think it has dependency on specific SoC.
> and can't agree as generic extcon gpio driver.
>
>
[snip]

-- 
-George

Re: [PATCH v3 1/3] extcon: extcon-gpio-usbvid: Generic USB VBUS/ID detection via GPIO

[Chanwoo Choi]

On 08/29/2013 08:48 PM, George Cherian wrote:
> On 8/29/2013 4:07 PM, Chanwoo Choi wrote:
>> On 08/29/2013 04:30 PM, George Cherian wrote:
>>> Hi Chanwoo,
>>>
>>> On 8/29/2013 11:53 AM, Chanwoo Choi wrote:
>>> [snip]
>>>> You should keep following naming stlye. extcon-gpio-usbvid.c is wrong naming style.
>>>> - extcon-[device name].c
>>>> - extcon-gpio-usbvid.c -> extcon-dra7xx.c or etc.
>>>>> Actually dra7xx is the SoC name and the USB VBUS/ID detection is not specific to SoC.
>>>>> It uses gpios to detect the VBUS/ID change. So i thought it would be better to have generic
>>>>> gpio based VBUS/ID detection rather than making dra7xx specific. Stephen Warren had this opinion
>>>>> with patch v1.
>>>> Would you guarantee that this driver support all of extcon devices using gpio pin?
>>> This driver would guarantee extcon devices using gpio pins for USB VBUS and ID detection.
>>> Following is the basic assumption made, correct me if I am wrong.
>>> ID pin ground means -> USB-HOST is true (this happens when a device is connected to USB port and we act as HOST )
>>> ID pin Float means -> USB-HOST is false (this happens when nothing is connected or when we act as a peripheral under a HOST)
>>> VBUS ON means -> USB is true (this happens when we are connected under a HOST as a peripheral)
>>> VBUS OFF means -> USB is false ( this happens when we are either disconnected from a HOST or when we are in HOST mode).
>>>
>>> So normally USB is in peripheral mode is enabled when ID pin is float and VBUS is ON.
>>> and USB is in HOST mode when ID pin is ground and VBUS is OFF.
>>>
>>> In all above cases VBUS is referred to the external VBUS supply from an external HOST.
>>>
>>>> I can't agree. This driver has specific dependency on dra7x SoC.
>>>>
>>>> First,
>>>> vbus_irq_handler() determine USB cable state according to "gpio_usbvid->vbus_gpio" state.
>>>> If "gpio_usbvid->vbus_gpio" value is VBUS_OFF(0), this patch set USB cable state as 'false'
>>>> But, it include potential problems. Other extcon device using gpio would set USB cable state
>>>> as 'true' when gpio state is 1. This way has dependency on specific SoC.
>>> no this is not SoC specific. VBUS is referred to the external VBUS supply from an external HOST.
>>> so if VBUS is zero means its definitely not in connected state.
>> I tested various development board based on Samsung Exynos series SoC.
>> Although some gpio of Exynos series SoC set high state(non zero, 1) as default value,
>> this gpio state could mean off state, disconnected or un-powered state according to gpio.
>> Of course, above explanation about specific gpio don't include all gpios.
>> This is meaning that there is possibility.
> okay then how about adding a flag for inverted logic too.  something like this
> 
> if(of_property_read_bool(np,"inverted_gpio))
>     gpio_usbvid->gpio_inv = 1;
> And always check on this before deciding?
> 
>>>> Second,
>>>> gpio_usbvid_set_initial_state() dertermine both "USB-HOST" and "USB" cable state at same time
>>>> in 'case ID_DETCT' according to 'gpio_usbvid->id_gpio'. But, I think that other extcon devices
>>>> would not control both "USB-HOST" and "USB" cable state at same time.
>>>>
>>>> Other extcon devices would support either "USB-HOST" or "USB" cable.
>>> The driver has 2 configurations.
>>> 1) supports implementations with both VBUS and ID pin are routed via gpio's for swicthing roles(compatible  gpio-usb-vid).
>> As you explained about case 1, it is only used on dra7x SoC.
> No gpio-usb-id is used in dra7xx. dra7xx has got only ID pin routed via gpio.

>> Other SoC could not wish to control both "USB-HOST" and "USB" cable at same time.
I need your answer about above my opinion for other SoC.

>>
>>
>>> 2) supports implementations with only ID pin routed via gpio for swicthing roles(compatible gpio-usb-id).
>>> So if you take type as VBUS_ID_DETECT then it is as what you meant.
> I think i didnt explain it properly last time.
> In perfect world you will have both VBUS and ID pin routed via gpios
> for eg: VBUS via gpio2 and ID via gpio3. if this is the case then we have to use compatible gpio-usb-vid where in 2 gpios will be used
> 2 irq handlers will be installed and it will control both USB-HOST and USB cables. In this case its possible to have 3 states
> USB-HOST (true), USB(true) and both false.
> 
> Now in case of dra7xx the board designers chose not to route the VBUS to gpio and only ID pin is routed.
> But still we need to  differentiate USB-HOST and USB cables In such cases we use compatible gpio-usb-id where in 1 gpio will be used
> 1 irq handler is installed  and it will control both USB-HOST  and USB cables. In this case its possible to have only 2 states
> USB-HOST (true) or USB(true).
> 
> Now there could be a 3rd scenario were in only VBUS is routed via gpio, that we would not support since we cant assume the value of ID pin
> and configure the UTMI is not proper. So I have mentioned even in documentation that ID pin is mandatory. We can always assume role depending on ID pin.
>> "2) case" don't support the detection of "USB-HOST" cable.
>> Only detect "USB" cable according to "vbus_gpio" value.
>>
>> If user wish to detect only "USB-HOST" cable, should user enter "ID_DETECT" as "ti,gpio-usb-id" on DT file?
>> But, id_irq_handler() control both "USB-HOST" and "USB" cable at same time. It may not prefer this method.
>>
>>>> Third,
>>>> gpio_usbvid_probe() get both gpio_usbvid->id_irq and gpio_usbvid->vbus_irq by using DT helper function.
>>>> and gpio_usbvid_request_irq() register each interrupt handler(id_irq and vbus_irq) according to DT data.
>>>> In result,
>>>>      id_irq_handler() would control both "USB-HOST" and "USB" cable state.
>>> only if type is ID_DETECT id_irq_handler control both USB-HOST and USB cable states
>>> if type is VBUS_ID_DETECT then id_irq_handler only controls USB-HOST.
>> I have some confusion. I need additional your explanation.
>> Could this driver register only one interrupt handler either id_irq_handler() or vbus_irq_handler()?
> If compatible == ID_DETECT, only one handler --> id_irq_handler() and it will handle both USB and USB-HOST
>> or
>> Could this driver register two interrupt handler both id_irq_handler() or vbus_irq_handler()?
> If compatible == VBUS_ID_DETECT, 2 handlers --> id_irq_handler() will handle USB-HOST and vbus_irq_handler will handle USB.

As you mentioned, we cannot only control either USB or USB-HOST cable on this extcon driver.
This extcon driver is only suitable dra7x SoC.

>>>>      vbus_irq_handler() would control only "USB" cable state.
>>>>
>>>> Two interrupt handler(id_irq_handler()/vbus_irq_handler()) would control "USB" cable state
>>>> according to each gpio state at same time. Also, It include critical problem.
>>> No it depends on the configuration as explained above.
>>>
>>> [snip]
>>>
>>>> +
>>>> +#define ID_GND        0
>>>> +#define ID_FLOAT    1
>>>> +#define VBUS_OFF    0
>>>> +#define VBUS_ON        1
>>>>>> I think you could only use two constant instead of four constant definition.
>>>>> you mean only ID_GND and VBUS_OFF?
>>>> you could only define two contant (0 and 1) to detect gpio state.
>>> okay
>>>>>>> +
>>>>>>> +
>>>>>> This blank line isn't necessary.
>>>>>>
>>>>>>> +static irqreturn_t id_irq_handler(int irq, void *data)
>>>>>>> +{
>>>>>>> +    struct gpio_usbvid *gpio_usbvid = (struct gpio_usbvid *) data;
>>>>>> You should delete blank between ')' and 'data' as follwong:
>>>>>>       - (struct gpio_usbvid *)data;
>>>>> okay
>>>>>>> +    int id_current;
>>>>>>> +
>>>>>>> +    id_current = gpio_get_value_cansleep(gpio_usbvid->id_gpio);
>>>>>>> +    if (id_current == ID_GND) {
>>>>>>> +        if (gpio_usbvid->type == ID_DETECT)
>>>>>>> +            extcon_set_cable_state(&gpio_usbvid->edev,
>>>>>>> +                            "USB", false);
>>>>>>> +        extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", true);
>>>>>> As else statement, you should set "USB-HOST" cable state to improve readability.
>>>>>>
>>>>>>           extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", true);
>>>>>>           if (gpio_usbvid->type == ID_DETECT)
>>>>>>               extcon_set_cable_state(&gpio_usbvid->edev,
>>>>>>                               "USB", false);
>>>>> Actually, USB-HOST state should be set in the id_irq handler. But in cases were only ID pin is routed to gpio
>>>>> and VBUS is not used we set the USB state too. The gpio_usbvid->type differentiates whether its an ID only or
>>>>> VBUS and ID.
>>>> I don't understand. Wht does not you change the order of function call as following?
>>>>
>>>>      Before:
>>>>              if (gpio_usbvid->type == ID_DETECT)
>>>>              extcon_set_cable_state(&gpio_usbvid->edev,
>>>>                                      "USB", false);
>>>>          extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", true);
>>> Basically these notifiers would go and change the UTMI modes which is s/w controlled.
>>> so we want to gracefully exit Device mode first and then enter HOST mode.
>>> this is only with type=ID_DETECT.
>> Because id_irq_handler() control both "USB-HOST" and "USB" cable at same time,
>> you need this setting order between "USB-HOST" and "USB" cable.
> yes

I think that the setting order between cables isn't general. It is specific method for dra7x SoC.

>> Did you think that SoC should always connect either "USB-HOST" cable or "USB" cable?
> No,  even if a physical cable is not connected it should  default to either USB-HOST or USB
It isn't general.

If physical cable isn't connected to extcon device, the state both USB-HOST and USB cable
should certainly be zero. Because The extcon consumer driver could set proper operation
according to cable state.

> 
>> I don't know this case except for dra7x SoC. So, I think it has dependency on specific SoC.

I need your answer about above my opinion.

>> and can't agree as generic extcon gpio driver.

Re: [PATCH v3 1/3] extcon: extcon-gpio-usbvid: Generic USB VBUS/ID detection via GPIO

[George Cherian]

Hi Chanwoo,


On 8/29/2013 5:42 PM, Chanwoo Choi wrote:
[big snip ]
>>> I tested various development board based on Samsung Exynos series SoC.
>>> Although some gpio of Exynos series SoC set high state(non zero, 1) as default value,
>>> this gpio state could mean off state, disconnected or un-powered state according to gpio.
>>> Of course, above explanation about specific gpio don't include all gpios.
>>> This is meaning that there is possibility.
>> okay then how about adding a flag for inverted logic too.  something like this
>>
>> if(of_property_read_bool(np,"inverted_gpio))
>>      gpio_usbvid->gpio_inv = 1;
>> And always check on this before deciding?
Is this fine ?
>>
>>>>> Second,
>>>>> gpio_usbvid_set_initial_state() dertermine both "USB-HOST" and "USB" cable state at same time
>>>>> in 'case ID_DETCT' according to 'gpio_usbvid->id_gpio'. But, I think that other extcon devices
>>>>> would not control both "USB-HOST" and "USB" cable state at same time.
>>>>>
>>>>> Other extcon devices would support either "USB-HOST" or "USB" cable.
>>>> The driver has 2 configurations.
>>>> 1) supports implementations with both VBUS and ID pin are routed via gpio's for swicthing roles(compatible  gpio-usb-vid).
>>> As you explained about case 1, it is only used on dra7x SoC.
>> No gpio-usb-id is used in dra7xx. dra7xx has got only ID pin routed via gpio.
>>> Other SoC could not wish to control both "USB-HOST" and "USB" cable at same time.
I could'nt actually parse this. You meant since the id_irq_handler 
handles both USB and USB-HOST cable
its not proper?
> I need your answer about above my opinion for other SoC.
So how about this, I have removed the dra7x specific stuffs (gpio-usb-id)

static void gpio_usbvid_set_initial_state(struct gpio_usbvid *gpio_usbvid)
{
         int id_current, vbus_current;

     id_current = gpio_get_value_cansleep(gpio_usbvid->id_gpio);
     if (!!id_current == ID_FLOAT)
         extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", false);
     else
         extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", true);

     vbus_current = gpio_get_value_cansleep(gpio_usbvid->vbus_gpio);
      if (!!vbus_current == VBUS_ON)
         extcon_set_cable_state(&gpio_usbvid->edev, "USB", true);
     else
         extcon_set_cable_state(&gpio_usbvid->edev, "USB", false);
}

and the irq handlers like this

static irqreturn_t id_irq_handler(int irq, void *data)
{
         struct gpio_usbvid *gpio_usbvid = (struct gpio_usbvid *)data;
         int id_current;

         id_current = gpio_get_value_cansleep(gpio_usbvid->id_gpio);
         if (id_current == ID_GND)
                 extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", 
true);
         else
                 extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", 
false);
         return IRQ_HANDLED;
}

static irqreturn_t vbus_irq_handler(int irq, void *data)
{
         struct gpio_usbvid *gpio_usbvid = (struct gpio_usbvid *)data;
         int vbus_current;

         vbus_current = gpio_get_value_cansleep(gpio_usbvid->vbus_gpio);
         if (vbus_current == VBUS_OFF)
                 extcon_set_cable_state(&gpio_usbvid->edev, "USB", false);
         else
                 extcon_set_cable_state(&gpio_usbvid->edev, "USB", true);

         return IRQ_HANDLED;
}
[snip]
>>> I have some confusion. I need additional your explanation.
>>> Could this driver register only one interrupt handler either id_irq_handler() or vbus_irq_handler()?
>> If compatible == ID_DETECT, only one handler --> id_irq_handler() and it will handle both USB and USB-HOST
>>> or
>>> Could this driver register two interrupt handler both id_irq_handler() or vbus_irq_handler()?
>> If compatible == VBUS_ID_DETECT, 2 handlers --> id_irq_handler() will handle USB-HOST and vbus_irq_handler will handle USB.
> As you mentioned, we cannot only control either USB or USB-HOST cable on this extcon driver.
> This extcon driver is only suitable dra7x SoC.
Do you still feel its dra7x specific if i modify it as above?
> Because id_irq_handler() control both "USB-HOST" and "USB" cable at same time,
> you need this setting order between "USB-HOST" and "USB" cable.
>> yes
> I think that the setting order between cables isn't general. It is specific method for dra7x SoC.
So if i remove that part then?
>>> Did you think that SoC should always connect either "USB-HOST" cable or "USB" cable?
>> No,  even if a physical cable is not connected it should  default to either USB-HOST or USB
> It isn't general.
>
> If physical cable isn't connected to extcon device, the state both USB-HOST and USB cable
> should certainly be zero. Because The extcon consumer driver could set proper operation
> according to cable state.
okay
>
>>
>>> I don't know this case except for dra7x SoC. So, I think it has dependency on specific SoC.
> I need your answer about above my opinion.
Hope i could answer you :-)
>>> and can't agree as generic extcon gpio driver.
>

-- 
-George

Re: [PATCH v3 1/3] extcon: extcon-gpio-usbvid: Generic USB VBUS/ID detection via GPIO

[Stephen Warren]

On 08/29/2013 05:48 AM, George Cherian wrote:
> On 8/29/2013 4:07 PM, Chanwoo Choi wrote:
...
>> I tested various development board based on Samsung Exynos series SoC.
>> Although some gpio of Exynos series SoC set high state(non zero, 1) as
>> default value,
>> this gpio state could mean off state, disconnected or un-powered state
>> according to gpio.
>> Of course, above explanation about specific gpio don't include all gpios.
>> This is meaning that there is possibility.
>
> okay then how about adding a flag for inverted logic too.  something
> like this
> 
> if(of_property_read_bool(np,"inverted_gpio))
>     gpio_usbvid->gpio_inv = 1;
>
> And always check on this before deciding?

Don't do this. GPIO specifiers in DT typically include a "flags" cell
that describes certain GPIO properties. One of those properties is
signal inversion. You can use of_get_named_gpio_flags() to retrieve
those flags.

Re: [PATCH v3 1/3] extcon: extcon-gpio-usbvid: Generic USB VBUS/ID detection via GPIO

[Stephen Warren]

On 08/28/2013 11:33 AM, George Cherian wrote:
> Add a generic USB VBUS/ID detection EXTCON driver. This driver expects
> the ID/VBUS pin are connected via GPIOs. This driver is tested on
> DRA7x board were the ID pin is routed via GPIOs. The driver supports
> both VBUS and ID pin configuration and ID pin only configuration.

> diff --git a/Documentation/devicetree/bindings/extcon/extcon-gpio-usbvid.txt b/Documentation/devicetree/bindings/extcon/extcon-gpio-usbvid.txt

> +EXTCON FOR USB VIA GPIO

Please write a brief explanation of the HW that this binding represents.

"Extcon" is a Linux-specific term. Binding documentation should be
written in an OS-agnostic manner. Bindings should not reference
OS-specific terminology, and should be a pure description of the HW.

> +Required Properties:
> + - compatible : Should be "ti,gpio-usb-vid" for USB VBUS-ID detector
> +		using gpios or "ti,gpio-usb-id" for USB ID pin detector

Those souond like two rather different things. Should they be separate
bindings, or at the least, separate sections in the document?

If this binding is meant to represent some generic hardware, the vendor
prefix probably isn't required. So, remove "ti," from the compatible values.

> + - gpios : phandle and args ID pin gpio and VBUS gpio.

s/and args/and specifier/.

> +	   The first gpio used  for ID pin detection
> +	   and the second used for VBUS detection.
> +	   ID pin gpio is mandatory and VBUS is optional
> +	   depending on implementation.

It'd be better to use named GPIO properties here, rather than having
multiple different kinds of GPIO in the same property. How about
"id-gpios" and "vbus-gpios" as the property names?

The semantics of each property should be specified. Are these input
GPIOs that reflect the ID and VBUS state? Do they directly represent the
signal state on the connector, or are they processed somehow? Does the
VBUS GPIO control the board's VBUS output, or is it an input? If it
controls VBUS output, it probably should be represented as a regulator
rather than a GPIO.

I think the following might work:

Required properties:

id-gpios: GPIO specifier for the connector's ID pin input. This directly
represents the value present on the ID pin at the connector.

Optional properties:

vbus-gpios: GPIO specifier for the connector's VBUS signal. This
directly represents whether VBUS being driven by the USB cable itself.

(although perhaps that isn't an optional property, but rather a required
property of the second compatible value?)

> +Please refer to ../gpio/gpio.txt for details of the common GPIO bindings
> +
> +Example:
> +
> +	gpio_usbvid_extcon1 {
> +		compatible = "ti,gpio-usb-vid";
> +		gpios = <&gpio1 1 0>,
> +			<&gpio2 2 0>;
> +	};

Re: [PATCH v3 3/3] ARM: dts: dra7-evm: Add extcon nodes for USB ID pin detection

[Stephen Warren]

On 08/28/2013 11:33 AM, George Cherian wrote:
> Add
> 	-extcon nodes for USB ID pin detection.
> 	-i2c nodes.
> 	-pcf nodes to which USB ID pin is connected.

> diff --git a/arch/arm/boot/dts/dra7-evm.dts b/arch/arm/boot/dts/dra7-evm.dts

>  &dwc3_1 {
> -	dr_mode = "otg";
> +	dr_mode = "host";
>  };

I wonder why one cares about ID/VBUS detection if the port doesn't
operate in OTG mode?

>  &dwc3_2 {
>  	dr_mode = "host";
>  };
> +
> +&usb1 {
> +	extcon = <&extcon1>;
> +};
> +
> +&usb2 {
> +	extcon = <&extcon2>;
> +};

I assume the "extcon" property is already fully documented in the
binding for the USB controller? For some reason, "extcon" looks like an
odd property name; I would have expected something more HW-oriented that
Linux-subsystem-oriented, such as "connector", or "usb-connector".

Re: [PATCH v3 1/3] extcon: extcon-gpio-usbvid: Generic USB VBUS/ID detection via GPIO

[Chanwoo Choi]

Hi George,

On 08/29/2013 10:45 PM, George Cherian wrote:
> Hi Chanwoo,
> 
> 
> On 8/29/2013 5:42 PM, Chanwoo Choi wrote:
> [big snip ]
>>>> I tested various development board based on Samsung Exynos series SoC.
>>>> Although some gpio of Exynos series SoC set high state(non zero, 1) as default value,
>>>> this gpio state could mean off state, disconnected or un-powered state according to gpio.
>>>> Of course, above explanation about specific gpio don't include all gpios.
>>>> This is meaning that there is possibility.
>>> okay then how about adding a flag for inverted logic too.  something like this
>>>
>>> if(of_property_read_bool(np,"inverted_gpio))
>>>      gpio_usbvid->gpio_inv = 1;
>>> And always check on this before deciding?
> Is this fine ?

OK.
But, as Stephen commented on other mail, you should use proper DT helper function.

>>>
>>>>>> Second,
>>>>>> gpio_usbvid_set_initial_state() dertermine both "USB-HOST" and "USB" cable state at same time
>>>>>> in 'case ID_DETCT' according to 'gpio_usbvid->id_gpio'. But, I think that other extcon devices
>>>>>> would not control both "USB-HOST" and "USB" cable state at same time.
>>>>>>
>>>>>> Other extcon devices would support either "USB-HOST" or "USB" cable.
>>>>> The driver has 2 configurations.
>>>>> 1) supports implementations with both VBUS and ID pin are routed via gpio's for swicthing roles(compatible  gpio-usb-vid).
>>>> As you explained about case 1, it is only used on dra7x SoC.
>>> No gpio-usb-id is used in dra7xx. dra7xx has got only ID pin routed via gpio.
>>>> Other SoC could not wish to control both "USB-HOST" and "USB" cable at same time.
> I could'nt actually parse this. You meant since the id_irq_handler handles both USB and USB-HOST cable
> its not proper?

It's not proper in general case. The generic driver must guarantee all of extcon device using gpio.
As far as I know, the generic driver cannot direclty control gpio pin and get value from gpio pin.
Almost device driver including in kernel/drivers control gpio pin on specific device driver 
instead of generic driver.

>> I need your answer about above my opinion for other SoC.
> So how about this, I have removed the dra7x specific stuffs (gpio-usb-id)
> 
> static void gpio_usbvid_set_initial_state(struct gpio_usbvid *gpio_usbvid)
> {
>         int id_current, vbus_current;
> 
>     id_current = gpio_get_value_cansleep(gpio_usbvid->id_gpio);
>     if (!!id_current == ID_FLOAT)
>         extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", false);
>     else
>         extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", true);
> 
>     vbus_current = gpio_get_value_cansleep(gpio_usbvid->vbus_gpio);
>      if (!!vbus_current == VBUS_ON)
>         extcon_set_cable_state(&gpio_usbvid->edev, "USB", true);
>     else
>         extcon_set_cable_state(&gpio_usbvid->edev, "USB", false);
> }
> 
> and the irq handlers like this
> 
> static irqreturn_t id_irq_handler(int irq, void *data)
> {
>         struct gpio_usbvid *gpio_usbvid = (struct gpio_usbvid *)data;
>         int id_current;
> 
>         id_current = gpio_get_value_cansleep(gpio_usbvid->id_gpio);
>         if (id_current == ID_GND)
>                 extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", true);
>         else
>                 extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", false);
>         return IRQ_HANDLED;
> }
> 
> static irqreturn_t vbus_irq_handler(int irq, void *data)
> {
>         struct gpio_usbvid *gpio_usbvid = (struct gpio_usbvid *)data;
>         int vbus_current;
> 
>         vbus_current = gpio_get_value_cansleep(gpio_usbvid->vbus_gpio);
>         if (vbus_current == VBUS_OFF)
>                 extcon_set_cable_state(&gpio_usbvid->edev, "USB", false);
>         else
>                 extcon_set_cable_state(&gpio_usbvid->edev, "USB", true);
> 
>         return IRQ_HANDLED;
> }

I know your intention dividing interrupt handler for each cable.
But I don't think this driver must guarantee all of extcon device using gpio.

For example,
below three SoC wish to detect USB/USB-HOST cable with each different methods.

"SoC A" wish to detect USB/USB-HOST cable through only one gpio pin.
"SoC B" wish to detect USB/USB-HOST cable through ADC value instead of gpio pin.
"SoC C" wish to detect USB/USB-HOST cable through two gpio pin because USB connected on gpio an USB-HOST connected on another.

In addition,
if "SoC A/C" wish to write some data to own specific registers for proper opeating,
Could we write some data to register on generic driver?

Finally,
"SoC D" wish to detect USB/USB-HOST/JIG cable through two gpio pin
- one gpio may detect either USB or USB-HOST and another may detect JIG cable
or one gpio may detect either USb or JIG and another may detect USB-HOST cable.

That way, there are many cases we cannot guarantee all of extcon devices.

I think this driver could support dra7x series SoC but as I mentioned,
this driver may not guarantee all of cases.

> [snip]
>>>> I have some confusion. I need additional your explanation.
>>>> Could this driver register only one interrupt handler either id_irq_handler() or vbus_irq_handler()?
>>> If compatible == ID_DETECT, only one handler --> id_irq_handler() and it will handle both USB and USB-HOST
>>>> or
>>>> Could this driver register two interrupt handler both id_irq_handler() or vbus_irq_handler()?
>>> If compatible == VBUS_ID_DETECT, 2 handlers --> id_irq_handler() will handle USB-HOST and vbus_irq_handler will handle USB.
>> As you mentioned, we cannot only control either USB or USB-HOST cable on this extcon driver.
>> This extcon driver is only suitable dra7x SoC.
> Do you still feel its dra7x specific if i modify it as above?

I commented above about your modification.

>> Because id_irq_handler() control both "USB-HOST" and "USB" cable at same time,
>> you need this setting order between "USB-HOST" and "USB" cable.
>>> yes
>> I think that the setting order between cables isn't general. It is specific method for dra7x SoC.
> So if i remove that part then?

The setting order should be removed in generic driver.

>>>> Did you think that SoC should always connect either "USB-HOST" cable or "USB" cable?
>>> No,  even if a physical cable is not connected it should  default to either USB-HOST or USB
>> It isn't general.
>>
>> If physical cable isn't connected to extcon device, the state both USB-HOST and USB cable
>> should certainly be zero. Because The extcon consumer driver could set proper operation
>> according to cable state.
> okay
>>
>>>
>>>> I don't know this case except for dra7x SoC. So, I think it has dependency on specific SoC.
>> I need your answer about above my opinion.
> Hope i could answer you :-)
>>>> and can't agree as generic extcon gpio driver.
>>
> 

Thanks,
Chanwoo Choi

Re: [PATCH v3 1/3] extcon: extcon-gpio-usbvid: Generic USB VBUS/ID detection via GPIO

[George Cherian]

Hi Chanwoo,

On 8/30/2013 5:41 AM, Chanwoo Choi wrote:
> Hi George,
>
> On 08/29/2013 10:45 PM, George Cherian wrote:
>> Hi Chanwoo,
>>
>>
>> On 8/29/2013 5:42 PM, Chanwoo Choi wrote:
>> [big snip ]
>>>>> I tested various development board based on Samsung Exynos series SoC.
>>>>> Although some gpio of Exynos series SoC set high state(non zero, 1) as default value,
>>>>> this gpio state could mean off state, disconnected or un-powered state according to gpio.
>>>>> Of course, above explanation about specific gpio don't include all gpios.
>>>>> This is meaning that there is possibility.
>>>> okay then how about adding a flag for inverted logic too.  something like this
>>>>
>>>> if(of_property_read_bool(np,"inverted_gpio))
>>>>       gpio_usbvid->gpio_inv = 1;
>>>> And always check on this before deciding?
>> Is this fine ?
> OK.
> But, as Stephen commented on other mail, you should use proper DT helper function.
okay
>>>>>>> Second,
>>>>>>> gpio_usbvid_set_initial_state() dertermine both "USB-HOST" and "USB" cable state at same time
>>>>>>> in 'case ID_DETCT' according to 'gpio_usbvid->id_gpio'. But, I think that other extcon devices
>>>>>>> would not control both "USB-HOST" and "USB" cable state at same time.
>>>>>>>
>>>>>>> Other extcon devices would support either "USB-HOST" or "USB" cable.
>>>>>> The driver has 2 configurations.
>>>>>> 1) supports implementations with both VBUS and ID pin are routed via gpio's for swicthing roles(compatible  gpio-usb-vid).
>>>>> As you explained about case 1, it is only used on dra7x SoC.
>>>> No gpio-usb-id is used in dra7xx. dra7xx has got only ID pin routed via gpio.
>>>>> Other SoC could not wish to control both "USB-HOST" and "USB" cable at same time.
>> I could'nt actually parse this. You meant since the id_irq_handler handles both USB and USB-HOST cable
>> its not proper?
> It's not proper in general case. The generic driver must guarantee all of extcon device using gpio.
> As far as I know, the generic driver cannot direclty control gpio pin and get value from gpio pin.
> Almost device driver including in kernel/drivers control gpio pin on specific device driver
> instead of generic driver.
But without reading the gpio value how can we set the cable states? for 
this driver the assumption is the dedicated gpio
is always DIR_IN and in the driver we just read the value.
>>> I need your answer about above my opinion for other SoC.
>> So how about this, I have removed the dra7x specific stuffs (gpio-usb-id)
>>
>> static void gpio_usbvid_set_initial_state(struct gpio_usbvid *gpio_usbvid)
>> {
>>          int id_current, vbus_current;
>>
>>      id_current = gpio_get_value_cansleep(gpio_usbvid->id_gpio);
>>      if (!!id_current == ID_FLOAT)
>>          extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", false);
>>      else
>>          extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", true);
>>
>>      vbus_current = gpio_get_value_cansleep(gpio_usbvid->vbus_gpio);
>>       if (!!vbus_current == VBUS_ON)
>>          extcon_set_cable_state(&gpio_usbvid->edev, "USB", true);
>>      else
>>          extcon_set_cable_state(&gpio_usbvid->edev, "USB", false);
>> }
>>
>> and the irq handlers like this
>>
>> static irqreturn_t id_irq_handler(int irq, void *data)
>> {
>>          struct gpio_usbvid *gpio_usbvid = (struct gpio_usbvid *)data;
>>          int id_current;
>>
>>          id_current = gpio_get_value_cansleep(gpio_usbvid->id_gpio);
>>          if (id_current == ID_GND)
>>                  extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", true);
>>          else
>>                  extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", false);
>>          return IRQ_HANDLED;
>> }
>>
>> static irqreturn_t vbus_irq_handler(int irq, void *data)
>> {
>>          struct gpio_usbvid *gpio_usbvid = (struct gpio_usbvid *)data;
>>          int vbus_current;
>>
>>          vbus_current = gpio_get_value_cansleep(gpio_usbvid->vbus_gpio);
>>          if (vbus_current == VBUS_OFF)
>>                  extcon_set_cable_state(&gpio_usbvid->edev, "USB", false);
>>          else
>>                  extcon_set_cable_state(&gpio_usbvid->edev, "USB", true);
>>
>>          return IRQ_HANDLED;
>> }
> I know your intention dividing interrupt handler for each cable.
> But I don't think this driver must guarantee all of extcon device using gpio.
>
> For example,
> below three SoC wish to detect USB/USB-HOST cable with each different methods.
>
> "SoC A" wish to detect USB/USB-HOST cable through only one gpio pin.

You mean to say that both USB ID pin and VBUS are connected to same gpio?
If so that is a really bad h/w design and it will not fly.
Or, you meant that only USB ID pin is connected to single gpio and it 
controls the state of the USB/USB-HOST cables?
If so thats what exactly the v3 driver did with compatible gpio-usb-id.

> "SoC B" wish to detect USB/USB-HOST cable through ADC value instead of gpio pin.

Via ADC this driver should never be used , it should be 
extcon-adc-usbvid driver and not extcon-gpio-usbvid driver.
> "SoC C" wish to detect USB/USB-HOST cable through two gpio pin because USB connected on gpio an USB-HOST connected on another.

Whatever modifications above should meet this need  in combination with 
named gpios (id_gpio and vbus_gpio in dt)as stephen pointed.
But still i feel the above modification would even support Soc A 
provided the code registered for the notifier could handle it properly.
>
> In addition,
> if "SoC A/C" wish to write some data to own specific registers for proper opeating,
> Could we write some data to register on generic driver?

Yes definitely, those register configuration should not be part of this 
driver and it should be done in the notifier handler.
>
> Finally,
> "SoC D" wish to detect USB/USB-HOST/JIG cable through two gpio pin
Correct me If I am wrong, USB JIG is not a standard cable. so for 
supporting that anyways you need to have
a different driver.
> - one gpio may detect either USB or USB-HOST and another may detect JIG cable
I assume u meant the USB ID pin is connected to one gpio and based on it 
value USB/USB-HOST  is detected.
> or one gpio may detect either USb or JIG and another may detect USB-HOST cable.
  As I mentioned earlier these are gpios configured as input and if you 
try to drive with 2 sources (USB and JIG or USB and USB-HOST etc)
then its a potentially bad design . If at all we need to identify all 3 
then there should be 3 dedicated gpios.

> That way, there are many cases we cannot guarantee all of extcon devices.
>
> I think this driver could support dra7x series SoC but as I mentioned,
> this driver may not guarantee all of cases.
I am sorry, I feel it supports all standard cases except for something 
like a JIG cable which is not standard.
>
>> [snip]
>>>>> I have some confusion. I need additional your explanation.
>>>>> Could this driver register only one interrupt handler either id_irq_handler() or vbus_irq_handler()?
>>>> If compatible == ID_DETECT, only one handler --> id_irq_handler() and it will handle both USB and USB-HOST
>>>>> or
>>>>> Could this driver register two interrupt handler both id_irq_handler() or vbus_irq_handler()?
>>>> If compatible == VBUS_ID_DETECT, 2 handlers --> id_irq_handler() will handle USB-HOST and vbus_irq_handler will handle USB.
>>> As you mentioned, we cannot only control either USB or USB-HOST cable on this extcon driver.
>>> This extcon driver is only suitable dra7x SoC.
>> Do you still feel its dra7x specific if i modify it as above?
> I commented above about your modification.
>
>>> Because id_irq_handler() control both "USB-HOST" and "USB" cable at same time,
>>> you need this setting order between "USB-HOST" and "USB" cable.
>>>> yes
>>> I think that the setting order between cables isn't general. It is specific method for dra7x SoC.
>> So if i remove that part then?
> The setting order should be removed in generic driver.
Yes I agree and should be done by the subscriber to the notifier.
>
>>>>> Did you think that SoC should always connect either "USB-HOST" cable or "USB" cable?
>>>> No,  even if a physical cable is not connected it should  default to either USB-HOST or USB
>>> It isn't general.
>>>
>>> If physical cable isn't connected to extcon device, the state both USB-HOST and USB cable
>>> should certainly be zero. Because The extcon consumer driver could set proper operation
>>> according to cable state.
>> okay
>>>>> I don't know this case except for dra7x SoC. So, I think it has dependency on specific SoC.
>>> I need your answer about above my opinion.
>> Hope i could answer you :-)
>>>>> and can't agree as generic extcon gpio driver.
> Thanks,
> Chanwoo Choi


-- 
-George

Re: [PATCH v3 1/3] extcon: extcon-gpio-usbvid: Generic USB VBUS/ID detection via GPIO

[Chanwoo Choi]

Hi George,

On 08/30/2013 03:15 PM, George Cherian wrote:
> Hi Chanwoo,
> 
> On 8/30/2013 5:41 AM, Chanwoo Choi wrote:
>> Hi George,
>>
>> On 08/29/2013 10:45 PM, George Cherian wrote:
>>> Hi Chanwoo,
>>>
>>>
>>> On 8/29/2013 5:42 PM, Chanwoo Choi wrote:
>>> [big snip ]
>>>>>> I tested various development board based on Samsung Exynos series SoC.
>>>>>> Although some gpio of Exynos series SoC set high state(non zero, 1) as default value,
>>>>>> this gpio state could mean off state, disconnected or un-powered state according to gpio.
>>>>>> Of course, above explanation about specific gpio don't include all gpios.
>>>>>> This is meaning that there is possibility.
>>>>> okay then how about adding a flag for inverted logic too.  something like this
>>>>>
>>>>> if(of_property_read_bool(np,"inverted_gpio))
>>>>>       gpio_usbvid->gpio_inv = 1;
>>>>> And always check on this before deciding?
>>> Is this fine ?
>> OK.
>> But, as Stephen commented on other mail, you should use proper DT helper function.
> okay
>>>>>>>> Second,
>>>>>>>> gpio_usbvid_set_initial_state() dertermine both "USB-HOST" and "USB" cable state at same time
>>>>>>>> in 'case ID_DETCT' according to 'gpio_usbvid->id_gpio'. But, I think that other extcon devices
>>>>>>>> would not control both "USB-HOST" and "USB" cable state at same time.
>>>>>>>>
>>>>>>>> Other extcon devices would support either "USB-HOST" or "USB" cable.
>>>>>>> The driver has 2 configurations.
>>>>>>> 1) supports implementations with both VBUS and ID pin are routed via gpio's for swicthing roles(compatible  gpio-usb-vid).
>>>>>> As you explained about case 1, it is only used on dra7x SoC.
>>>>> No gpio-usb-id is used in dra7xx. dra7xx has got only ID pin routed via gpio.
>>>>>> Other SoC could not wish to control both "USB-HOST" and "USB" cable at same time.
>>> I could'nt actually parse this. You meant since the id_irq_handler handles both USB and USB-HOST cable
>>> its not proper?
>> It's not proper in general case. The generic driver must guarantee all of extcon device using gpio.
>> As far as I know, the generic driver cannot direclty control gpio pin and get value from gpio pin.
>> Almost device driver including in kernel/drivers control gpio pin on specific device driver
>> instead of generic driver.
> But without reading the gpio value how can we set the cable states? for this driver the assumption is the dedicated gpio
> is always DIR_IN and in the driver we just read the value.
>>>> I need your answer about above my opinion for other SoC.
>>> So how about this, I have removed the dra7x specific stuffs (gpio-usb-id)
>>>
>>> static void gpio_usbvid_set_initial_state(struct gpio_usbvid *gpio_usbvid)
>>> {
>>>          int id_current, vbus_current;
>>>
>>>      id_current = gpio_get_value_cansleep(gpio_usbvid->id_gpio);
>>>      if (!!id_current == ID_FLOAT)
>>>          extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", false);
>>>      else
>>>          extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", true);
>>>
>>>      vbus_current = gpio_get_value_cansleep(gpio_usbvid->vbus_gpio);
>>>       if (!!vbus_current == VBUS_ON)
>>>          extcon_set_cable_state(&gpio_usbvid->edev, "USB", true);
>>>      else
>>>          extcon_set_cable_state(&gpio_usbvid->edev, "USB", false);
>>> }
>>>
>>> and the irq handlers like this
>>>
>>> static irqreturn_t id_irq_handler(int irq, void *data)
>>> {
>>>          struct gpio_usbvid *gpio_usbvid = (struct gpio_usbvid *)data;
>>>          int id_current;
>>>
>>>          id_current = gpio_get_value_cansleep(gpio_usbvid->id_gpio);
>>>          if (id_current == ID_GND)
>>>                  extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", true);
>>>          else
>>>                  extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", false);
>>>          return IRQ_HANDLED;
>>> }
>>>
>>> static irqreturn_t vbus_irq_handler(int irq, void *data)
>>> {
>>>          struct gpio_usbvid *gpio_usbvid = (struct gpio_usbvid *)data;
>>>          int vbus_current;
>>>
>>>          vbus_current = gpio_get_value_cansleep(gpio_usbvid->vbus_gpio);
>>>          if (vbus_current == VBUS_OFF)
>>>                  extcon_set_cable_state(&gpio_usbvid->edev, "USB", false);
>>>          else
>>>                  extcon_set_cable_state(&gpio_usbvid->edev, "USB", true);
>>>
>>>          return IRQ_HANDLED;
>>> }
>> I know your intention dividing interrupt handler for each cable.
>> But I don't think this driver must guarantee all of extcon device using gpio.
>>
>> For example,
>> below three SoC wish to detect USB/USB-HOST cable with each different methods.
>>
>> "SoC A" wish to detect USB/USB-HOST cable through only one gpio pin.
> 
> You mean to say that both USB ID pin and VBUS are connected to same gpio?
> If so that is a really bad h/w design and it will not fly.
> Or, you meant that only USB ID pin is connected to single gpio and it controls the state of the USB/USB-HOST cables?
> If so thats what exactly the v3 driver did with compatible gpio-usb-id.

My original question intention,
I'd like you to answer that this driver can support all case of "SoC A"/"SoC B"/"SoC C" without othe implementation?

> 
>> "SoC B" wish to detect USB/USB-HOST cable through ADC value instead of gpio pin.
> 
> Via ADC this driver should never be used , it should be extcon-adc-usbvid driver and not extcon-gpio-usbvid driver.

No, we cannot implement many generic extcon device driver whenever this is the case.

>> "SoC C" wish to detect USB/USB-HOST cable through two gpio pin because USB connected on gpio an USB-HOST connected on another.
> 
> Whatever modifications above should meet this need  in combination with named gpios (id_gpio and vbus_gpio in dt)as stephen pointed.
> But still i feel the above modification would even support Soc A provided the code registered for the notifier could handle it properly.

I'd like you to answer that this driver can support all case of "SoC A"/"SoC B"/"SoC C"?
If we should implement extcon device driver according each case,
Could you tell me that this driver is generic? I think NO.

>>
>> In addition,
>> if "SoC A/C" wish to write some data to own specific registers for proper opeating,
>> Could we write some data to register on generic driver?
> 
> Yes definitely, those register configuration should not be part of this driver and it should be done in the notifier handler.

No. there are many extcon consumer driver. We cannot order the sequence of notification reception among all of extcon consumer driver.
Only, extcon consumer driver get the changed state of cable and can never set cable H/W setting in extcon consumer driver.
Certainly, we have to set cabe H/W setting in extcon producer driver.

>>
>> Finally,
>> "SoC D" wish to detect USB/USB-HOST/JIG cable through two gpio pin
> Correct me If I am wrong, USB JIG is not a standard cable. so for supporting that anyways you need to have
> a different driver.

No, we cannot guarantee that some cable would be connected to SoC.
If this dirver is generic, it have to certainly support above all case in this driver.

>> - one gpio may detect either USB or USB-HOST and another may detect JIG cable
> I assume u meant the USB ID pin is connected to one gpio and based on it value USB/USB-HOST  is detected.
>> or one gpio may detect either USb or JIG and another may detect USB-HOST cable.
>  As I mentioned earlier these are gpios configured as input and if you try to drive with 2 sources (USB and JIG or USB and USB-HOST etc)
> then its a potentially bad design . If at all we need to identify all 3 then there should be 3 dedicated gpios.
> 
>> That way, there are many cases we cannot guarantee all of extcon devices.
>>
>> I think this driver could support dra7x series SoC but as I mentioned,
>> this driver may not guarantee all of cases.
> I am sorry, I feel it supports all standard cases except for something like a JIG cable which is not standard.

I've never seen spec documentation that USB/USB-HOST cable have to be only connected to SoC.
As I mentioned above, there are many cases we cannot think about various extcon devices.

>>
>>> [snip]
>>>>>> I have some confusion. I need additional your explanation.
>>>>>> Could this driver register only one interrupt handler either id_irq_handler() or vbus_irq_handler()?
>>>>> If compatible == ID_DETECT, only one handler --> id_irq_handler() and it will handle both USB and USB-HOST
>>>>>> or
>>>>>> Could this driver register two interrupt handler both id_irq_handler() or vbus_irq_handler()?
>>>>> If compatible == VBUS_ID_DETECT, 2 handlers --> id_irq_handler() will handle USB-HOST and vbus_irq_handler will handle USB.
>>>> As you mentioned, we cannot only control either USB or USB-HOST cable on this extcon driver.
>>>> This extcon driver is only suitable dra7x SoC.
>>> Do you still feel its dra7x specific if i modify it as above?
>> I commented above about your modification.
>>
>>>> Because id_irq_handler() control both "USB-HOST" and "USB" cable at same time,
>>>> you need this setting order between "USB-HOST" and "USB" cable.
>>>>> yes
>>>> I think that the setting order between cables isn't general. It is specific method for dra7x SoC.
>>> So if i remove that part then?
>> The setting order should be removed in generic driver.
> Yes I agree and should be done by the subscriber to the notifier.

OK.

>>
>>>>>> Did you think that SoC should always connect either "USB-HOST" cable or "USB" cable?
>>>>> No,  even if a physical cable is not connected it should  default to either USB-HOST or USB
>>>> It isn't general.
>>>>
>>>> If physical cable isn't connected to extcon device, the state both USB-HOST and USB cable
>>>> should certainly be zero. Because The extcon consumer driver could set proper operation
>>>> according to cable state.
>>> okay
>>>>>> I don't know this case except for dra7x SoC. So, I think it has dependency on specific SoC.
>>>> I need your answer about above my opinion.
>>> Hope i could answer you :-)
>>>>>> and can't agree as generic extcon gpio driver.
>> Thanks,
>> Chanwoo Choi


Thanks,
Chanwoo Choi

Re: [PATCH v3 1/3] extcon: extcon-gpio-usbvid: Generic USB VBUS/ID detection via GPIO

[Chanwoo Choi]

Hi George,

In addition, I add answer about that device driver control gpio pin directly.

On 08/30/2013 03:15 PM, George Cherian wrote:
> Hi Chanwoo,
> 
> On 8/30/2013 5:41 AM, Chanwoo Choi wrote:
>> Hi George,
>>
>> On 08/29/2013 10:45 PM, George Cherian wrote:
>>> Hi Chanwoo,
>>>
>>>
>>> On 8/29/2013 5:42 PM, Chanwoo Choi wrote:
>>> [big snip ]
>>>>>> I tested various development board based on Samsung Exynos series SoC.
>>>>>> Although some gpio of Exynos series SoC set high state(non zero, 1) as default value,
>>>>>> this gpio state could mean off state, disconnected or un-powered state according to gpio.
>>>>>> Of course, above explanation about specific gpio don't include all gpios.
>>>>>> This is meaning that there is possibility.
>>>>> okay then how about adding a flag for inverted logic too.  something like this
>>>>>
>>>>> if(of_property_read_bool(np,"inverted_gpio))
>>>>>       gpio_usbvid->gpio_inv = 1;
>>>>> And always check on this before deciding?
>>> Is this fine ?
>> OK.
>> But, as Stephen commented on other mail, you should use proper DT helper function.
> okay
>>>>>>>> Second,
>>>>>>>> gpio_usbvid_set_initial_state() dertermine both "USB-HOST" and "USB" cable state at same time
>>>>>>>> in 'case ID_DETCT' according to 'gpio_usbvid->id_gpio'. But, I think that other extcon devices
>>>>>>>> would not control both "USB-HOST" and "USB" cable state at same time.
>>>>>>>>
>>>>>>>> Other extcon devices would support either "USB-HOST" or "USB" cable.
>>>>>>> The driver has 2 configurations.
>>>>>>> 1) supports implementations with both VBUS and ID pin are routed via gpio's for swicthing roles(compatible  gpio-usb-vid).
>>>>>> As you explained about case 1, it is only used on dra7x SoC.
>>>>> No gpio-usb-id is used in dra7xx. dra7xx has got only ID pin routed via gpio.
>>>>>> Other SoC could not wish to control both "USB-HOST" and "USB" cable at same time.
>>> I could'nt actually parse this. You meant since the id_irq_handler handles both USB and USB-HOST cable
>>> its not proper?
>> It's not proper in general case. The generic driver must guarantee all of extcon device using gpio.
>> As far as I know, the generic driver cannot direclty control gpio pin and get value from gpio pin.
>> Almost device driver including in kernel/drivers control gpio pin on specific device driver
>> instead of generic driver.
> But without reading the gpio value how can we set the cable states?

hmm. I mentioned above my answer as following:
	>> As far as I know, the generic driver cannot direclty control gpio pin and get value from gpio pin.
	>> Almost device driver including in kernel/drivers control gpio pin on specific device driver
Because your extcon driver directly control gpio pin so I think your extcon driver isn't generic.

for this driver the assumption is the dedicated gpio
> is always DIR_IN and in the driver we just read the value.

What is DIR_IN?

>>>> I need your answer about above my opinion for other SoC.
>>> So how about this, I have removed the dra7x specific stuffs (gpio-usb-id)
>>>
>>> static void gpio_usbvid_set_initial_state(struct gpio_usbvid *gpio_usbvid)
>>> {
>>>          int id_current, vbus_current;
>>>
>>>      id_current = gpio_get_value_cansleep(gpio_usbvid->id_gpio);
>>>      if (!!id_current == ID_FLOAT)
>>>          extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", false);
>>>      else
>>>          extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", true);
>>>
>>>      vbus_current = gpio_get_value_cansleep(gpio_usbvid->vbus_gpio);
>>>       if (!!vbus_current == VBUS_ON)
>>>          extcon_set_cable_state(&gpio_usbvid->edev, "USB", true);
>>>      else
>>>          extcon_set_cable_state(&gpio_usbvid->edev, "USB", false);
>>> }
>>>
>>> and the irq handlers like this
>>>
>>> static irqreturn_t id_irq_handler(int irq, void *data)
>>> {
>>>          struct gpio_usbvid *gpio_usbvid = (struct gpio_usbvid *)data;
>>>          int id_current;
>>>
>>>          id_current = gpio_get_value_cansleep(gpio_usbvid->id_gpio);
>>>          if (id_current == ID_GND)
>>>                  extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", true);
>>>          else
>>>                  extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", false);
>>>          return IRQ_HANDLED;
>>> }
>>>
>>> static irqreturn_t vbus_irq_handler(int irq, void *data)
>>> {
>>>          struct gpio_usbvid *gpio_usbvid = (struct gpio_usbvid *)data;
>>>          int vbus_current;
>>>
>>>          vbus_current = gpio_get_value_cansleep(gpio_usbvid->vbus_gpio);
>>>          if (vbus_current == VBUS_OFF)
>>>                  extcon_set_cable_state(&gpio_usbvid->edev, "USB", false);
>>>          else
>>>                  extcon_set_cable_state(&gpio_usbvid->edev, "USB", true);
>>>
>>>          return IRQ_HANDLED;
>>> }
>> I know your intention dividing interrupt handler for each cable.
>> But I don't think this driver must guarantee all of extcon device using gpio.
>>
>> For example,
>> below three SoC wish to detect USB/USB-HOST cable with each different methods.
>>
>> "SoC A" wish to detect USB/USB-HOST cable through only one gpio pin.
> 
> You mean to say that both USB ID pin and VBUS are connected to same gpio?
> If so that is a really bad h/w design and it will not fly.
> Or, you meant that only USB ID pin is connected to single gpio and it controls the state of the USB/USB-HOST cables?
> If so thats what exactly the v3 driver did with compatible gpio-usb-id.
> 
>> "SoC B" wish to detect USB/USB-HOST cable through ADC value instead of gpio pin.
> 
> Via ADC this driver should never be used , it should be extcon-adc-usbvid driver and not extcon-gpio-usbvid driver.
>> "SoC C" wish to detect USB/USB-HOST cable through two gpio pin because USB connected on gpio an USB-HOST connected on another.
> 
> Whatever modifications above should meet this need  in combination with named gpios (id_gpio and vbus_gpio in dt)as stephen pointed.
> But still i feel the above modification would even support Soc A provided the code registered for the notifier could handle it properly.
>>
>> In addition,
>> if "SoC A/C" wish to write some data to own specific registers for proper opeating,
>> Could we write some data to register on generic driver?
> 
> Yes definitely, those register configuration should not be part of this driver and it should be done in the notifier handler.
>>
>> Finally,
>> "SoC D" wish to detect USB/USB-HOST/JIG cable through two gpio pin
> Correct me If I am wrong, USB JIG is not a standard cable. so for supporting that anyways you need to have
> a different driver.
>> - one gpio may detect either USB or USB-HOST and another may detect JIG cable
> I assume u meant the USB ID pin is connected to one gpio and based on it value USB/USB-HOST  is detected.
>> or one gpio may detect either USb or JIG and another may detect USB-HOST cable.
>  As I mentioned earlier these are gpios configured as input and if you try to drive with 2 sources (USB and JIG or USB and USB-HOST etc)
> then its a potentially bad design . If at all we need to identify all 3 then there should be 3 dedicated gpios.
> 
>> That way, there are many cases we cannot guarantee all of extcon devices.
>>
>> I think this driver could support dra7x series SoC but as I mentioned,
>> this driver may not guarantee all of cases.
> I am sorry, I feel it supports all standard cases except for something like a JIG cable which is not standard.
>>
>>> [snip]
>>>>>> I have some confusion. I need additional your explanation.
>>>>>> Could this driver register only one interrupt handler either id_irq_handler() or vbus_irq_handler()?
>>>>> If compatible == ID_DETECT, only one handler --> id_irq_handler() and it will handle both USB and USB-HOST
>>>>>> or
>>>>>> Could this driver register two interrupt handler both id_irq_handler() or vbus_irq_handler()?
>>>>> If compatible == VBUS_ID_DETECT, 2 handlers --> id_irq_handler() will handle USB-HOST and vbus_irq_handler will handle USB.
>>>> As you mentioned, we cannot only control either USB or USB-HOST cable on this extcon driver.
>>>> This extcon driver is only suitable dra7x SoC.
>>> Do you still feel its dra7x specific if i modify it as above?
>> I commented above about your modification.
>>
>>>> Because id_irq_handler() control both "USB-HOST" and "USB" cable at same time,
>>>> you need this setting order between "USB-HOST" and "USB" cable.
>>>>> yes
>>>> I think that the setting order between cables isn't general. It is specific method for dra7x SoC.
>>> So if i remove that part then?
>> The setting order should be removed in generic driver.
> Yes I agree and should be done by the subscriber to the notifier.
>>
>>>>>> Did you think that SoC should always connect either "USB-HOST" cable or "USB" cable?
>>>>> No,  even if a physical cable is not connected it should  default to either USB-HOST or USB
>>>> It isn't general.
>>>>
>>>> If physical cable isn't connected to extcon device, the state both USB-HOST and USB cable
>>>> should certainly be zero. Because The extcon consumer driver could set proper operation
>>>> according to cable state.
>>> okay
>>>>>> I don't know this case except for dra7x SoC. So, I think it has dependency on specific SoC.
>>>> I need your answer about above my opinion.
>>> Hope i could answer you :-)
>>>>>> and can't agree as generic extcon gpio driver.
>> Thanks,
>> Chanwoo Choi
> 
> 

Thanks,
Chanwoo Choi

Re: [PATCH v3 1/3] extcon: extcon-gpio-usbvid: Generic USB VBUS/ID detection via GPIO

[George Cherian]

On 8/30/2013 12:23 PM, Chanwoo Choi wrote:
> Hi George,
>
> On 08/30/2013 03:15 PM, George Cherian wrote:
>> Hi Chanwoo,
>>
>> On 8/30/2013 5:41 AM, Chanwoo Choi wrote:
>>> Hi George,
>>>
>>> On 08/29/2013 10:45 PM, George Cherian wrote:
>>>> Hi Chanwoo,
>>>>
>>>>
>>>> On 8/29/2013 5:42 PM, Chanwoo Choi wrote:
>>>> [big snip ]
>>>>>>> I tested various development board based on Samsung Exynos series SoC.
>>>>>>> Although some gpio of Exynos series SoC set high state(non zero, 1) as default value,
>>>>>>> this gpio state could mean off state, disconnected or un-powered state according to gpio.
>>>>>>> Of course, above explanation about specific gpio don't include all gpios.
>>>>>>> This is meaning that there is possibility.
>>>>>> okay then how about adding a flag for inverted logic too.  something like this
>>>>>>
>>>>>> if(of_property_read_bool(np,"inverted_gpio))
>>>>>>        gpio_usbvid->gpio_inv = 1;
>>>>>> And always check on this before deciding?
>>>> Is this fine ?
>>> OK.
>>> But, as Stephen commented on other mail, you should use proper DT helper function.
>> okay
>>>>>>>>> Second,
>>>>>>>>> gpio_usbvid_set_initial_state() dertermine both "USB-HOST" and "USB" cable state at same time
>>>>>>>>> in 'case ID_DETCT' according to 'gpio_usbvid->id_gpio'. But, I think that other extcon devices
>>>>>>>>> would not control both "USB-HOST" and "USB" cable state at same time.
>>>>>>>>>
>>>>>>>>> Other extcon devices would support either "USB-HOST" or "USB" cable.
>>>>>>>> The driver has 2 configurations.
>>>>>>>> 1) supports implementations with both VBUS and ID pin are routed via gpio's for swicthing roles(compatible  gpio-usb-vid).
>>>>>>> As you explained about case 1, it is only used on dra7x SoC.
>>>>>> No gpio-usb-id is used in dra7xx. dra7xx has got only ID pin routed via gpio.
>>>>>>> Other SoC could not wish to control both "USB-HOST" and "USB" cable at same time.
>>>> I could'nt actually parse this. You meant since the id_irq_handler handles both USB and USB-HOST cable
>>>> its not proper?
>>> It's not proper in general case. The generic driver must guarantee all of extcon device using gpio.
>>> As far as I know, the generic driver cannot direclty control gpio pin and get value from gpio pin.
>>> Almost device driver including in kernel/drivers control gpio pin on specific device driver
>>> instead of generic driver.
>> But without reading the gpio value how can we set the cable states? for this driver the assumption is the dedicated gpio
>> is always DIR_IN and in the driver we just read the value.
>>>>> I need your answer about above my opinion for other SoC.
>>>> So how about this, I have removed the dra7x specific stuffs (gpio-usb-id)
>>>>
>>>> static void gpio_usbvid_set_initial_state(struct gpio_usbvid *gpio_usbvid)
>>>> {
>>>>           int id_current, vbus_current;
>>>>
>>>>       id_current = gpio_get_value_cansleep(gpio_usbvid->id_gpio);
>>>>       if (!!id_current == ID_FLOAT)
>>>>           extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", false);
>>>>       else
>>>>           extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", true);
>>>>
>>>>       vbus_current = gpio_get_value_cansleep(gpio_usbvid->vbus_gpio);
>>>>        if (!!vbus_current == VBUS_ON)
>>>>           extcon_set_cable_state(&gpio_usbvid->edev, "USB", true);
>>>>       else
>>>>           extcon_set_cable_state(&gpio_usbvid->edev, "USB", false);
>>>> }
>>>>
>>>> and the irq handlers like this
>>>>
>>>> static irqreturn_t id_irq_handler(int irq, void *data)
>>>> {
>>>>           struct gpio_usbvid *gpio_usbvid = (struct gpio_usbvid *)data;
>>>>           int id_current;
>>>>
>>>>           id_current = gpio_get_value_cansleep(gpio_usbvid->id_gpio);
>>>>           if (id_current == ID_GND)
>>>>                   extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", true);
>>>>           else
>>>>                   extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", false);
>>>>           return IRQ_HANDLED;
>>>> }
>>>>
>>>> static irqreturn_t vbus_irq_handler(int irq, void *data)
>>>> {
>>>>           struct gpio_usbvid *gpio_usbvid = (struct gpio_usbvid *)data;
>>>>           int vbus_current;
>>>>
>>>>           vbus_current = gpio_get_value_cansleep(gpio_usbvid->vbus_gpio);
>>>>           if (vbus_current == VBUS_OFF)
>>>>                   extcon_set_cable_state(&gpio_usbvid->edev, "USB", false);
>>>>           else
>>>>                   extcon_set_cable_state(&gpio_usbvid->edev, "USB", true);
>>>>
>>>>           return IRQ_HANDLED;
>>>> }
>>> I know your intention dividing interrupt handler for each cable.
>>> But I don't think this driver must guarantee all of extcon device using gpio.
>>>
>>> For example,
>>> below three SoC wish to detect USB/USB-HOST cable with each different methods.
>>>
>>> "SoC A" wish to detect USB/USB-HOST cable through only one gpio pin.
>> You mean to say that both USB ID pin and VBUS are connected to same gpio?
>> If so that is a really bad h/w design and it will not fly.
>> Or, you meant that only USB ID pin is connected to single gpio and it controls the state of the USB/USB-HOST cables?
>> If so thats what exactly the v3 driver did with compatible gpio-usb-id.
> My original question intention,
> I'd like you to answer that this driver can support all case of "SoC A"/"SoC B"/"SoC C" without othe implementation?
>
Definitely supports SoC A and SoC C. this is neither  a generic extcon 
driver nor a generic gpio driver.
This is a generic driver for USB VBUS-ID detection connected via gpios. 
so doesnot address ADC (SOC B)
>>> "SoC B" wish to detect USB/USB-HOST cable through ADC value instead of gpio pin.
>> Via ADC this driver should never be used , it should be extcon-adc-usbvid driver and not extcon-gpio-usbvid driver.
> No, we cannot implement many generic extcon device driver whenever this is the case.
then how can I make this one generic? Pointers?
>
>>> "SoC C" wish to detect USB/USB-HOST cable through two gpio pin because USB connected on gpio an USB-HOST connected on another.
>> Whatever modifications above should meet this need  in combination with named gpios (id_gpio and vbus_gpio in dt)as stephen pointed.
>> But still i feel the above modification would even support Soc A provided the code registered for the notifier could handle it properly.
> I'd like you to answer that this driver can support all case of "SoC A"/"SoC B"/"SoC C"?
Answered above.
> If we should implement extcon device driver according each case,
> Could you tell me that this driver is generic? I think NO.
If for each case separate extcon driver, then its  not generic. But How 
would I make one or how can I modify this one to be more generic?
>>> In addition,
>>> if "SoC A/C" wish to write some data to own specific registers for proper opeating,
>>> Could we write some data to register on generic driver?
>> Yes definitely, those register configuration should not be part of this driver and it should be done in the notifier handler.
> No. there are many extcon consumer driver. We cannot order the sequence of notification reception among all of extcon consumer driver.
> Only, extcon consumer driver get the changed state of cable and can never set cable H/W setting in extcon consumer driver.
> Certainly, we have to set cabe H/W setting in extcon producer driver.
I dont want the consumer driver to change the cable state. I wanted to 
mention that the cable state change might need a register write specifc 
to the H/W which could be done
in consumers call back.
  For eg:- When the USB cable is connected I need to set the UTMI config 
register to Peripheral mode and when USB-HOST is connected the same need 
to be set to HOST mode, these writes will be done
in the call back functions.
>>> Finally,
>>> "SoC D" wish to detect USB/USB-HOST/JIG cable through two gpio pin
>> Correct me If I am wrong, USB JIG is not a standard cable. so for supporting that anyways you need to have
>> a different driver.
> No, we cannot guarantee that some cable would be connected to SoC.
> If this dirver is generic, it have to certainly support above all case in this driver.

okay, but at the same time there are tons of devices running linux which 
doesnot support USB JIG cable.
Are those all attributed to linux and its drivers?
>
>>> - one gpio may detect either USB or USB-HOST and another may detect JIG cable
>> I assume u meant the USB ID pin is connected to one gpio and based on it value USB/USB-HOST  is detected.
>>> or one gpio may detect either USb or JIG and another may detect USB-HOST cable.
>>   As I mentioned earlier these are gpios configured as input and if you try to drive with 2 sources (USB and JIG or USB and USB-HOST etc)
>> then its a potentially bad design . If at all we need to identify all 3 then there should be 3 dedicated gpios.
>>
>>> That way, there are many cases we cannot guarantee all of extcon devices.
>>>
>>> I think this driver could support dra7x series SoC but as I mentioned,
>>> this driver may not guarantee all of cases.
>> I am sorry, I feel it supports all standard cases except for something like a JIG cable which is not standard.
> I've never seen spec documentation that USB/USB-HOST cable have to be only connected to SoC.
> As I mentioned above, there are many cases we cannot think about various extcon devices.

Okay but we have to start with some known working combinations and then 
increase the number of stuffs supported.
>>>> [snip]
>>>>>>> I have some confusion. I need additional your explanation.
>>>>>>> Could this driver register only one interrupt handler either id_irq_handler() or vbus_irq_handler()?
>>>>>> If compatible == ID_DETECT, only one handler --> id_irq_handler() and it will handle both USB and USB-HOST
>>>>>>> or
>>>>>>> Could this driver register two interrupt handler both id_irq_handler() or vbus_irq_handler()?
>>>>>> If compatible == VBUS_ID_DETECT, 2 handlers --> id_irq_handler() will handle USB-HOST and vbus_irq_handler will handle USB.
>>>>> As you mentioned, we cannot only control either USB or USB-HOST cable on this extcon driver.
>>>>> This extcon driver is only suitable dra7x SoC.
>>>> Do you still feel its dra7x specific if i modify it as above?
>>> I commented above about your modification.
>>>
>>>>> Because id_irq_handler() control both "USB-HOST" and "USB" cable at same time,
>>>>> you need this setting order between "USB-HOST" and "USB" cable.
>>>>>> yes
>>>>> I think that the setting order between cables isn't general. It is specific method for dra7x SoC.
>>>> So if i remove that part then?
>>> The setting order should be removed in generic driver.
>> Yes I agree and should be done by the subscriber to the notifier.
> OK.
>
>>>>>>> Did you think that SoC should always connect either "USB-HOST" cable or "USB" cable?
>>>>>> No,  even if a physical cable is not connected it should  default to either USB-HOST or USB
>>>>> It isn't general.
>>>>>
>>>>> If physical cable isn't connected to extcon device, the state both USB-HOST and USB cable
>>>>> should certainly be zero. Because The extcon consumer driver could set proper operation
>>>>> according to cable state.
>>>> okay
>>>>>>> I don't know this case except for dra7x SoC. So, I think it has dependency on specific SoC.
>>>>> I need your answer about above my opinion.
>>>> Hope i could answer you :-)
>>>>>>> and can't agree as generic extcon gpio driver.
>>> Thanks,
>>> Chanwoo Choi
>
> Thanks,
> Chanwoo Choi
>


-- 
-George

Re: [PATCH v3 1/3] extcon: extcon-gpio-usbvid: Generic USB VBUS/ID detection via GPIO

[George Cherian]

On 8/30/2013 12:44 PM, Chanwoo Choi wrote:
> Hi George,
>
> In addition, I add answer about that device driver control gpio pin directly.
>
> On 08/30/2013 03:15 PM, George Cherian wrote:
>> Hi Chanwoo,
>>
>> On 8/30/2013 5:41 AM, Chanwoo Choi wrote:
>>> Hi George,
>>>
>>> On 08/29/2013 10:45 PM, George Cherian wrote:
>>>> Hi Chanwoo,
>>>>
>>>>
>>>> On 8/29/2013 5:42 PM, Chanwoo Choi wrote:
>>>> [big snip ]
>>>>>>> I tested various development board based on Samsung Exynos series SoC.
>>>>>>> Although some gpio of Exynos series SoC set high state(non zero, 1) as default value,
>>>>>>> this gpio state could mean off state, disconnected or un-powered state according to gpio.
>>>>>>> Of course, above explanation about specific gpio don't include all gpios.
>>>>>>> This is meaning that there is possibility.
>>>>>> okay then how about adding a flag for inverted logic too.  something like this
>>>>>>
>>>>>> if(of_property_read_bool(np,"inverted_gpio))
>>>>>>        gpio_usbvid->gpio_inv = 1;
>>>>>> And always check on this before deciding?
>>>> Is this fine ?
>>> OK.
>>> But, as Stephen commented on other mail, you should use proper DT helper function.
>> okay
>>>>>>>>> Second,
>>>>>>>>> gpio_usbvid_set_initial_state() dertermine both "USB-HOST" and "USB" cable state at same time
>>>>>>>>> in 'case ID_DETCT' according to 'gpio_usbvid->id_gpio'. But, I think that other extcon devices
>>>>>>>>> would not control both "USB-HOST" and "USB" cable state at same time.
>>>>>>>>>
>>>>>>>>> Other extcon devices would support either "USB-HOST" or "USB" cable.
>>>>>>>> The driver has 2 configurations.
>>>>>>>> 1) supports implementations with both VBUS and ID pin are routed via gpio's for swicthing roles(compatible  gpio-usb-vid).
>>>>>>> As you explained about case 1, it is only used on dra7x SoC.
>>>>>> No gpio-usb-id is used in dra7xx. dra7xx has got only ID pin routed via gpio.
>>>>>>> Other SoC could not wish to control both "USB-HOST" and "USB" cable at same time.
>>>> I could'nt actually parse this. You meant since the id_irq_handler handles both USB and USB-HOST cable
>>>> its not proper?
>>> It's not proper in general case. The generic driver must guarantee all of extcon device using gpio.
>>> As far as I know, the generic driver cannot direclty control gpio pin and get value from gpio pin.
>>> Almost device driver including in kernel/drivers control gpio pin on specific device driver
>>> instead of generic driver.
>> But without reading the gpio value how can we set the cable states?
> hmm. I mentioned above my answer as following:
> 	>> As far as I know, the generic driver cannot direclty control gpio pin and get value from gpio pin.
> 	>> Almost device driver including in kernel/drivers control gpio pin on specific device driver
> Because your extcon driver directly control gpio pin so I think your extcon driver isn't generic.
>
> for this driver the assumption is the dedicated gpio
Obviously when I am writing a generic driver for USB VBUS-ID detetction 
which is via gpio then i assume I have a dedicated gpio.
what is wrong in that assumption? How else can you detect ID pin change 
and VBUS change via gpio if not you have them dedicated?
>> is always DIR_IN and in the driver we just read the value.
> What is DIR_IN?
Direction IN gpio.
>>>>> I need your answer about above my opinion for other SoC.
>>>> So how about this, I have removed the dra7x specific stuffs (gpio-usb-id)
>>>>
>>>> static void gpio_usbvid_set_initial_state(struct gpio_usbvid *gpio_usbvid)
>>>> {
>>>>           int id_current, vbus_current;
>>>>
>>>>       id_current = gpio_get_value_cansleep(gpio_usbvid->id_gpio);
>>>>       if (!!id_current == ID_FLOAT)
>>>>           extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", false);
>>>>       else
>>>>           extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", true);
>>>>
>>>>       vbus_current = gpio_get_value_cansleep(gpio_usbvid->vbus_gpio);
>>>>        if (!!vbus_current == VBUS_ON)
>>>>           extcon_set_cable_state(&gpio_usbvid->edev, "USB", true);
>>>>       else
>>>>           extcon_set_cable_state(&gpio_usbvid->edev, "USB", false);
>>>> }
>>>>
>>>> and the irq handlers like this
>>>>
>>>> static irqreturn_t id_irq_handler(int irq, void *data)
>>>> {
>>>>           struct gpio_usbvid *gpio_usbvid = (struct gpio_usbvid *)data;
>>>>           int id_current;
>>>>
>>>>           id_current = gpio_get_value_cansleep(gpio_usbvid->id_gpio);
>>>>           if (id_current == ID_GND)
>>>>                   extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", true);
>>>>           else
>>>>                   extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", false);
>>>>           return IRQ_HANDLED;
>>>> }
>>>>
>>>> static irqreturn_t vbus_irq_handler(int irq, void *data)
>>>> {
>>>>           struct gpio_usbvid *gpio_usbvid = (struct gpio_usbvid *)data;
>>>>           int vbus_current;
>>>>
>>>>           vbus_current = gpio_get_value_cansleep(gpio_usbvid->vbus_gpio);
>>>>           if (vbus_current == VBUS_OFF)
>>>>                   extcon_set_cable_state(&gpio_usbvid->edev, "USB", false);
>>>>           else
>>>>                   extcon_set_cable_state(&gpio_usbvid->edev, "USB", true);
>>>>
>>>>           return IRQ_HANDLED;
>>>> }
>>> I know your intention dividing interrupt handler for each cable.
>>> But I don't think this driver must guarantee all of extcon device using gpio.
>>>
>>> For example,
>>> below three SoC wish to detect USB/USB-HOST cable with each different methods.
>>>
>>> "SoC A" wish to detect USB/USB-HOST cable through only one gpio pin.
>> You mean to say that both USB ID pin and VBUS are connected to same gpio?
>> If so that is a really bad h/w design and it will not fly.
>> Or, you meant that only USB ID pin is connected to single gpio and it controls the state of the USB/USB-HOST cables?
>> If so thats what exactly the v3 driver did with compatible gpio-usb-id.
>>
>>> "SoC B" wish to detect USB/USB-HOST cable through ADC value instead of gpio pin.
>> Via ADC this driver should never be used , it should be extcon-adc-usbvid driver and not extcon-gpio-usbvid driver.
>>> "SoC C" wish to detect USB/USB-HOST cable through two gpio pin because USB connected on gpio an USB-HOST connected on another.
>> Whatever modifications above should meet this need  in combination with named gpios (id_gpio and vbus_gpio in dt)as stephen pointed.
>> But still i feel the above modification would even support Soc A provided the code registered for the notifier could handle it properly.
>>> In addition,
>>> if "SoC A/C" wish to write some data to own specific registers for proper opeating,
>>> Could we write some data to register on generic driver?
>> Yes definitely, those register configuration should not be part of this driver and it should be done in the notifier handler.
>>> Finally,
>>> "SoC D" wish to detect USB/USB-HOST/JIG cable through two gpio pin
>> Correct me If I am wrong, USB JIG is not a standard cable. so for supporting that anyways you need to have
>> a different driver.
>>> - one gpio may detect either USB or USB-HOST and another may detect JIG cable
>> I assume u meant the USB ID pin is connected to one gpio and based on it value USB/USB-HOST  is detected.
>>> or one gpio may detect either USb or JIG and another may detect USB-HOST cable.
>>   As I mentioned earlier these are gpios configured as input and if you try to drive with 2 sources (USB and JIG or USB and USB-HOST etc)
>> then its a potentially bad design . If at all we need to identify all 3 then there should be 3 dedicated gpios.
>>
>>> That way, there are many cases we cannot guarantee all of extcon devices.
>>>
>>> I think this driver could support dra7x series SoC but as I mentioned,
>>> this driver may not guarantee all of cases.
>> I am sorry, I feel it supports all standard cases except for something like a JIG cable which is not standard.
>>>> [snip]
>>>>>>> I have some confusion. I need additional your explanation.
>>>>>>> Could this driver register only one interrupt handler either id_irq_handler() or vbus_irq_handler()?
>>>>>> If compatible == ID_DETECT, only one handler --> id_irq_handler() and it will handle both USB and USB-HOST
>>>>>>> or
>>>>>>> Could this driver register two interrupt handler both id_irq_handler() or vbus_irq_handler()?
>>>>>> If compatible == VBUS_ID_DETECT, 2 handlers --> id_irq_handler() will handle USB-HOST and vbus_irq_handler will handle USB.
>>>>> As you mentioned, we cannot only control either USB or USB-HOST cable on this extcon driver.
>>>>> This extcon driver is only suitable dra7x SoC.
>>>> Do you still feel its dra7x specific if i modify it as above?
>>> I commented above about your modification.
>>>
>>>>> Because id_irq_handler() control both "USB-HOST" and "USB" cable at same time,
>>>>> you need this setting order between "USB-HOST" and "USB" cable.
>>>>>> yes
>>>>> I think that the setting order between cables isn't general. It is specific method for dra7x SoC.
>>>> So if i remove that part then?
>>> The setting order should be removed in generic driver.
>> Yes I agree and should be done by the subscriber to the notifier.
>>>>>>> Did you think that SoC should always connect either "USB-HOST" cable or "USB" cable?
>>>>>> No,  even if a physical cable is not connected it should  default to either USB-HOST or USB
>>>>> It isn't general.
>>>>>
>>>>> If physical cable isn't connected to extcon device, the state both USB-HOST and USB cable
>>>>> should certainly be zero. Because The extcon consumer driver could set proper operation
>>>>> according to cable state.
>>>> okay
>>>>>>> I don't know this case except for dra7x SoC. So, I think it has dependency on specific SoC.
>>>>> I need your answer about above my opinion.
>>>> Hope i could answer you :-)
>>>>>>> and can't agree as generic extcon gpio driver.
>>> Thanks,
>>> Chanwoo Choi
>>
> Thanks,
> Chanwoo Choi
>


-- 
-George