[RFC/PATCH] extcon: otg_gpio: add driver for USB OTG port controlled by GPIO(s)

[RFC/PATCH] extcon: otg_gpio: add driver for USB OTG port controlled by GPIO(s)

[David Cohen]

Some platforms have an USB OTG port fully (or partially) controlled by
GPIOs:

(1) USB ID is connected directly to GPIO

Optionally:
(2) VBUS is enabled by a GPIO (when ID is grounded)
(3) Platform has 2 USB controllers connected to same port: one for
    device and one for host role. D+/- are switched between phys
    by GPIO.

As per initial version, this driver has the duty to control whether
USB-Host cable is plugged in or not:
 - If yes, OTG port is configured for host role
 - If no, by standard, the OTG port is configured for device role

Signed-off-by: David Cohen <david.a.cohen@linux.intel.com>
---

Hi,

Some Intel Bay Trail boards have an unusual way to handle the USB OTG port:
 - The USB ID pin is connected directly to GPIO on SoC
 - When in host role, VBUS is provided by enabling a GPIO
 - Device and host roles are supported by 2 independent controllers with D+/-
   pins from port switched between different phys according a GPIO level.

The ACPI table describes this USB port as a (virtual) device with all the
necessary GPIOs. This driver implements support to this virtual device as an
extcon class driver. All drivers that depend on the USB OTG port state (USB phy,
PMIC, charge detection) will listen to extcon events.

Comments are welcome.

Br, David
---

 drivers/extcon/Kconfig           |   8 ++
 drivers/extcon/Makefile          |   1 +
 drivers/extcon/extcon-otg_gpio.c | 200 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 209 insertions(+)
 create mode 100644 drivers/extcon/extcon-otg_gpio.c

diff --git a/drivers/extcon/Kconfig b/drivers/extcon/Kconfig
index 6a1f7de6fa54..e8010cda4642 100644
--- a/drivers/extcon/Kconfig
+++ b/drivers/extcon/Kconfig
@@ -93,4 +93,12 @@ config EXTCON_SM5502
 	  Silicon Mitus SM5502. The SM5502 is a USB port accessory
 	  detector and switch.
 
+config EXTCON_OTG_GPIO
+	tristate "VIRTUAL USB OTG PORT support"
+	depends on GPIOLIB
+	help
+	  Say Y here to enable support for virtual USB OTG port device
+	  controlled by GPIOs. This driver can be used when at least USB ID pin
+	  is connected directly to GPIO.
+
 endif # MULTISTATE_SWITCH
diff --git a/drivers/extcon/Makefile b/drivers/extcon/Makefile
index 0370b42e5a27..9e81088c6584 100644
--- a/drivers/extcon/Makefile
+++ b/drivers/extcon/Makefile
@@ -12,3 +12,4 @@ obj-$(CONFIG_EXTCON_MAX8997)	+= extcon-max8997.o
 obj-$(CONFIG_EXTCON_PALMAS)	+= extcon-palmas.o
 obj-$(CONFIG_EXTCON_RT8973A)	+= extcon-rt8973a.o
 obj-$(CONFIG_EXTCON_SM5502)	+= extcon-sm5502.o
+obj-$(CONFIG_EXTCON_OTG_GPIO) += extcon-otg_gpio.o
diff --git a/drivers/extcon/extcon-otg_gpio.c b/drivers/extcon/extcon-otg_gpio.c
new file mode 100644
index 000000000000..c5ee765a5f4f
--- /dev/null
+++ b/drivers/extcon/extcon-otg_gpio.c
@@ -0,0 +1,200 @@
+/*
+ * Virtual USB OTG Port driver controlled by gpios
+ *
+ * Copyright (c) 2014, Intel Corporation.
+ * Author: David Cohen <david.a.cohen@linux.intel.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * 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/acpi.h>
+#include <linux/extcon.h>
+#include <linux/gpio.h>
+#include <linux/interrupt.h>
+#include <linux/platform_device.h>
+
+#define DRV_NAME	"usb_otg_port"
+
+struct vuport {
+	struct device *dev;
+	struct gpio_desc *gpio_vbus_en;
+	struct gpio_desc *gpio_usb_id;
+	struct gpio_desc *gpio_usb_mux;
+
+	struct extcon_dev edev;
+};
+
+static const char *const vuport_extcon_cable[] = {
+	[0] = "USB-Host",
+	NULL,
+};
+
+/*
+ * If id == 1, USB port should be set to peripheral
+ * if id == 0, USB port should be set to host
+ *
+ * Peripheral: set USB mux to peripheral and disable VBUS
+ * Host: set USB mux to host and enable VBUS
+ */
+static void vuport_set_port(struct vuport *vup, int id)
+{
+	int mux_val = id;
+	int vbus_val = !id;
+
+	if (!IS_ERR(vup->gpio_usb_mux))
+		gpiod_direction_output(vup->gpio_usb_mux, mux_val);
+
+	if (!IS_ERR(vup->gpio_vbus_en))
+		gpiod_direction_output(vup->gpio_vbus_en, vbus_val);
+}
+
+static void vuport_do_usb_id(struct vuport *vup)
+{
+	int id = gpiod_get_value(vup->gpio_usb_id);
+
+	dev_info(vup->dev, "USB PORT ID: %s\n", id ? "PERIPHERAL" : "HOST");
+
+	/*
+	 * id == 1: PERIPHERAL
+	 * id == 0: HOST
+	 */
+	vuport_set_port(vup, id);
+
+	/*
+	 * id == 0: HOST connected
+	 * id == 1: Host disconnected
+	 */
+	extcon_set_cable_state(&vup->edev, "USB-Host", !id);
+}
+
+static irqreturn_t vuport_thread_isr(int irq, void *priv)
+{
+	struct vuport *vup = priv;
+
+	vuport_do_usb_id(vup);
+	return IRQ_HANDLED;
+}
+
+static irqreturn_t vuport_isr(int irq, void *priv)
+{
+	return IRQ_WAKE_THREAD;
+}
+
+#define VUPORT_GPIO_USB_ID	0
+#define VUPORT_GPIO_VBUS_EN	1
+#define VUPORT_GPIO_USB_MUX	2
+static int vuport_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct vuport *vup;
+	int ret;
+
+	vup = devm_kzalloc(dev, sizeof(*vup), GFP_KERNEL);
+	if (!vup) {
+		dev_err(dev, "cannot allocate private data\n");
+		return -ENOMEM;
+	}
+	vup->dev = dev;
+
+	vup->gpio_usb_id = devm_gpiod_get_index(dev, "id", VUPORT_GPIO_USB_ID);
+	if (IS_ERR(vup->gpio_usb_id)) {
+		dev_err(dev, "cannot request USB ID GPIO: ret = %ld\n",
+			PTR_ERR(vup->gpio_usb_id));
+		return PTR_ERR(vup->gpio_usb_id);
+	}
+
+	ret = gpiod_direction_input(vup->gpio_usb_id);
+	if (ret < 0) {
+		dev_err(dev, "cannot set input direction to USB ID GPIO: ret = %d\n",
+			ret);
+		return ret;
+	}
+
+	vup->gpio_vbus_en = devm_gpiod_get_index(dev, "vbus en",
+						 VUPORT_GPIO_VBUS_EN);
+	if (IS_ERR(vup->gpio_vbus_en))
+		dev_info(dev, "cannot request VBUS EN GPIO, skipping it.\n");
+
+	vup->gpio_usb_mux = devm_gpiod_get_index(dev, "usb mux",
+						 VUPORT_GPIO_USB_MUX);
+	if (IS_ERR(vup->gpio_usb_mux))
+		dev_info(dev, "cannot request USB USB MUX, skipping it.\n");
+
+	/* register extcon device */
+	vup->edev.dev.parent = dev;
+	vup->edev.supported_cable = vuport_extcon_cable;
+	ret = extcon_dev_register(&vup->edev);
+	if (ret < 0) {
+		dev_err(dev, "failed to register extcon device: ret = %d\n",
+			ret);
+		return ret;
+	}
+
+	ret = devm_request_threaded_irq(dev, gpiod_to_irq(vup->gpio_usb_id),
+					vuport_isr, vuport_thread_isr,
+					IRQF_SHARED | IRQF_TRIGGER_RISING |
+					IRQF_TRIGGER_FALLING,
+					dev_name(dev), vup);
+	if (ret < 0) {
+		dev_err(dev, "cannot request IRQ for USB ID GPIO: ret = %d\n",
+			ret);
+		goto irq_err;
+	}
+	vuport_do_usb_id(vup);
+
+	platform_set_drvdata(pdev, vup);
+
+	dev_info(dev, "driver successfully probed\n");
+
+	return 0;
+
+irq_err:
+	extcon_dev_unregister(&vup->edev);
+
+	return ret;
+}
+
+static int vuport_remove(struct platform_device *pdev)
+{
+	struct vuport *vup = platform_get_drvdata(pdev);
+
+	extcon_dev_unregister(&vup->edev);
+	return 0;
+}
+
+static struct acpi_device_id vuport_acpi_match[] = {
+	{ "INT3496" },
+	{ }
+};
+MODULE_DEVICE_TABLE(acpi, vuport_acpi_match);
+
+static struct platform_driver vuport_driver = {
+	.driver = {
+		.name = DRV_NAME,
+		.acpi_match_table = ACPI_PTR(vuport_acpi_match),
+	},
+	.probe = vuport_probe,
+	.remove = vuport_remove,
+};
+
+static int __init vuport_init(void)
+{
+	return platform_driver_register(&vuport_driver);
+}
+subsys_initcall(vuport_init);
+
+static void __exit vuport_exit(void)
+{
+	platform_driver_unregister(&vuport_driver);
+}
+module_exit(vuport_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("David Cohen <david.a.cohen@linux.intel.com>");
-- 
2.1.1

Re: [RFC/PATCH] extcon: otg_gpio: add driver for USB OTG port controlled by GPIO(s)

[Peter Chen]

On Mon, Dec 22, 2014 at 02:43:37PM -0800, David Cohen wrote:
> Some platforms have an USB OTG port fully (or partially) controlled by
> GPIOs:
> 
> (1) USB ID is connected directly to GPIO
> 
> Optionally:
> (2) VBUS is enabled by a GPIO (when ID is grounded)
> (3) Platform has 2 USB controllers connected to same port: one for
>     device and one for host role. D+/- are switched between phys
>     by GPIO.

Would you explain how it works? Choosing controller runtime?

> 
> As per initial version, this driver has the duty to control whether
> USB-Host cable is plugged in or not:

You mean Micro-AB cable, right?

>  - If yes, OTG port is configured for host role
>  - If no, by standard, the OTG port is configured for device role
> 
> Signed-off-by: David Cohen <david.a.cohen@linux.intel.com>
> ---
> 
> Hi,
> 
> Some Intel Bay Trail boards have an unusual way to handle the USB OTG port:
>  - The USB ID pin is connected directly to GPIO on SoC
>  - When in host role, VBUS is provided by enabling a GPIO
>  - Device and host roles are supported by 2 independent controllers with D+/-
>    pins from port switched between different phys according a GPIO level.
> 
> The ACPI table describes this USB port as a (virtual) device with all the
> necessary GPIOs. This driver implements support to this virtual device as an
> extcon class driver. All drivers that depend on the USB OTG port state (USB phy,
> PMIC, charge detection) will listen to extcon events.
> 
> Comments are welcome.
> 
> Br, David
> ---
> 
>  drivers/extcon/Kconfig           |   8 ++
>  drivers/extcon/Makefile          |   1 +
>  drivers/extcon/extcon-otg_gpio.c | 200 +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 209 insertions(+)
>  create mode 100644 drivers/extcon/extcon-otg_gpio.c
> 
> diff --git a/drivers/extcon/Kconfig b/drivers/extcon/Kconfig
> index 6a1f7de6fa54..e8010cda4642 100644
> --- a/drivers/extcon/Kconfig
> +++ b/drivers/extcon/Kconfig
> @@ -93,4 +93,12 @@ config EXTCON_SM5502
>  	  Silicon Mitus SM5502. The SM5502 is a USB port accessory
>  	  detector and switch.
>  
> +config EXTCON_OTG_GPIO
> +	tristate "VIRTUAL USB OTG PORT support"
> +	depends on GPIOLIB
> +	help
> +	  Say Y here to enable support for virtual USB OTG port device
> +	  controlled by GPIOs. This driver can be used when at least USB ID pin
> +	  is connected directly to GPIO.
> +
>  endif # MULTISTATE_SWITCH
> diff --git a/drivers/extcon/Makefile b/drivers/extcon/Makefile
> index 0370b42e5a27..9e81088c6584 100644
> --- a/drivers/extcon/Makefile
> +++ b/drivers/extcon/Makefile
> @@ -12,3 +12,4 @@ obj-$(CONFIG_EXTCON_MAX8997)	+= extcon-max8997.o
>  obj-$(CONFIG_EXTCON_PALMAS)	+= extcon-palmas.o
>  obj-$(CONFIG_EXTCON_RT8973A)	+= extcon-rt8973a.o
>  obj-$(CONFIG_EXTCON_SM5502)	+= extcon-sm5502.o
> +obj-$(CONFIG_EXTCON_OTG_GPIO) += extcon-otg_gpio.o
> diff --git a/drivers/extcon/extcon-otg_gpio.c b/drivers/extcon/extcon-otg_gpio.c
> new file mode 100644
> index 000000000000..c5ee765a5f4f
> --- /dev/null
> +++ b/drivers/extcon/extcon-otg_gpio.c
> @@ -0,0 +1,200 @@
> +/*
> + * Virtual USB OTG Port driver controlled by gpios
> + *
> + * Copyright (c) 2014, Intel Corporation.
> + * Author: David Cohen <david.a.cohen@linux.intel.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * 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/acpi.h>
> +#include <linux/extcon.h>
> +#include <linux/gpio.h>
> +#include <linux/interrupt.h>
> +#include <linux/platform_device.h>
> +
> +#define DRV_NAME	"usb_otg_port"
> +
> +struct vuport {
> +	struct device *dev;
> +	struct gpio_desc *gpio_vbus_en;
> +	struct gpio_desc *gpio_usb_id;
> +	struct gpio_desc *gpio_usb_mux;
> +
> +	struct extcon_dev edev;
> +};
> +
> +static const char *const vuport_extcon_cable[] = {
> +	[0] = "USB-Host",
> +	NULL,
> +};
> +
> +/*
> + * If id == 1, USB port should be set to peripheral
> + * if id == 0, USB port should be set to host
> + *
> + * Peripheral: set USB mux to peripheral and disable VBUS
> + * Host: set USB mux to host and enable VBUS
> + */
> +static void vuport_set_port(struct vuport *vup, int id)
> +{
> +	int mux_val = id;
> +	int vbus_val = !id;
> +
> +	if (!IS_ERR(vup->gpio_usb_mux))
> +		gpiod_direction_output(vup->gpio_usb_mux, mux_val);
> +
> +	if (!IS_ERR(vup->gpio_vbus_en))
> +		gpiod_direction_output(vup->gpio_vbus_en, vbus_val);
> +}
> +
> +static void vuport_do_usb_id(struct vuport *vup)
> +{
> +	int id = gpiod_get_value(vup->gpio_usb_id);
> +
> +	dev_info(vup->dev, "USB PORT ID: %s\n", id ? "PERIPHERAL" : "HOST");

dev_dbg

> +
> +	/*
> +	 * id == 1: PERIPHERAL
> +	 * id == 0: HOST
> +	 */
> +	vuport_set_port(vup, id);
> +
> +	/*
> +	 * id == 0: HOST connected
> +	 * id == 1: Host disconnected
> +	 */
> +	extcon_set_cable_state(&vup->edev, "USB-Host", !id);
> +}
> +
> +static irqreturn_t vuport_thread_isr(int irq, void *priv)
> +{
> +	struct vuport *vup = priv;
> +
> +	vuport_do_usb_id(vup);
> +	return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t vuport_isr(int irq, void *priv)
> +{
> +	return IRQ_WAKE_THREAD;
> +}
> +
> +#define VUPORT_GPIO_USB_ID	0
> +#define VUPORT_GPIO_VBUS_EN	1
> +#define VUPORT_GPIO_USB_MUX	2
> +static int vuport_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct vuport *vup;
> +	int ret;
> +
> +	vup = devm_kzalloc(dev, sizeof(*vup), GFP_KERNEL);
> +	if (!vup) {
> +		dev_err(dev, "cannot allocate private data\n");
> +		return -ENOMEM;
> +	}
> +	vup->dev = dev;
> +
> +	vup->gpio_usb_id = devm_gpiod_get_index(dev, "id", VUPORT_GPIO_USB_ID);
> +	if (IS_ERR(vup->gpio_usb_id)) {
> +		dev_err(dev, "cannot request USB ID GPIO: ret = %ld\n",
> +			PTR_ERR(vup->gpio_usb_id));
> +		return PTR_ERR(vup->gpio_usb_id);
> +	}
> +
> +	ret = gpiod_direction_input(vup->gpio_usb_id);
> +	if (ret < 0) {
> +		dev_err(dev, "cannot set input direction to USB ID GPIO: ret = %d\n",
> +			ret);
> +		return ret;
> +	}
> +
> +	vup->gpio_vbus_en = devm_gpiod_get_index(dev, "vbus en",
> +						 VUPORT_GPIO_VBUS_EN);
> +	if (IS_ERR(vup->gpio_vbus_en))
> +		dev_info(dev, "cannot request VBUS EN GPIO, skipping it.\n");
> +
> +	vup->gpio_usb_mux = devm_gpiod_get_index(dev, "usb mux",
> +						 VUPORT_GPIO_USB_MUX);
> +	if (IS_ERR(vup->gpio_usb_mux))
> +		dev_info(dev, "cannot request USB USB MUX, skipping it.\n");

Using dev_err

> +
> +	/* register extcon device */
> +	vup->edev.dev.parent = dev;
> +	vup->edev.supported_cable = vuport_extcon_cable;
> +	ret = extcon_dev_register(&vup->edev);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to register extcon device: ret = %d\n",
> +			ret);
> +		return ret;
> +	}
> +
> +	ret = devm_request_threaded_irq(dev, gpiod_to_irq(vup->gpio_usb_id),
> +					vuport_isr, vuport_thread_isr,
> +					IRQF_SHARED | IRQF_TRIGGER_RISING |
> +					IRQF_TRIGGER_FALLING,
> +					dev_name(dev), vup);
> +	if (ret < 0) {
> +		dev_err(dev, "cannot request IRQ for USB ID GPIO: ret = %d\n",
> +			ret);
> +		goto irq_err;
> +	}
> +	vuport_do_usb_id(vup);
> +
> +	platform_set_drvdata(pdev, vup);
> +
> +	dev_info(dev, "driver successfully probed\n");
> +
> +	return 0;
> +
> +irq_err:
> +	extcon_dev_unregister(&vup->edev);
> +
> +	return ret;
> +}
> +
> +static int vuport_remove(struct platform_device *pdev)
> +{
> +	struct vuport *vup = platform_get_drvdata(pdev);
> +
> +	extcon_dev_unregister(&vup->edev);
> +	return 0;
> +}
> +
> +static struct acpi_device_id vuport_acpi_match[] = {
> +	{ "INT3496" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(acpi, vuport_acpi_match);
> +
> +static struct platform_driver vuport_driver = {
> +	.driver = {
> +		.name = DRV_NAME,
> +		.acpi_match_table = ACPI_PTR(vuport_acpi_match),
> +	},
> +	.probe = vuport_probe,
> +	.remove = vuport_remove,
> +};
> +
> +static int __init vuport_init(void)
> +{
> +	return platform_driver_register(&vuport_driver);
> +}
> +subsys_initcall(vuport_init);
> +
> +static void __exit vuport_exit(void)
> +{
> +	platform_driver_unregister(&vuport_driver);
> +}
> +module_exit(vuport_exit);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("David Cohen <david.a.cohen@linux.intel.com>");
> -- 
> 2.1.1
> 

-- 

Best Regards,
Peter Chen

Re: [RFC/PATCH] extcon: otg_gpio: add driver for USB OTG port controlled by GPIO(s)

[Sergei Shtylyov]

Hello.

On 12/23/2014 1:43 AM, David Cohen wrote:

> Some platforms have an USB OTG port fully (or partially) controlled by
> GPIOs:

> (1) USB ID is connected directly to GPIO

> Optionally:
> (2) VBUS is enabled by a GPIO (when ID is grounded)

    Can't the host driver still control Vbus?

> (3) Platform has 2 USB controllers connected to same port: one for
>      device and one for host role. D+/- are switched between phys
>      by GPIO.

> As per initial version, this driver has the duty to control whether
> USB-Host cable is plugged in or not:
>   - If yes, OTG port is configured for host role
>   - If no, by standard, the OTG port is configured for device role

> Signed-off-by: David Cohen <david.a.cohen@linux.intel.com>
> ---

> Hi,

> Some Intel Bay Trail boards have an unusual way to handle the USB OTG port:
>   - The USB ID pin is connected directly to GPIO on SoC
>   - When in host role, VBUS is provided by enabling a GPIO
>   - Device and host roles are supported by 2 independent controllers with D+/-
>     pins from port switched between different phys according a GPIO level.

> The ACPI table describes this USB port as a (virtual) device with all the
> necessary GPIOs. This driver implements support to this virtual device as an
> extcon class driver. All drivers that depend on the USB OTG port state (USB phy,
> PMIC, charge detection) will listen to extcon events.

    It's very close to my setup on R-Car R8A7791 based boards. :-)
I have already submitted Maxim MAX3355 OTG chip GPIO-based driver.

> Comments are welcome.

> Br, David
> ---
>
>   drivers/extcon/Kconfig           |   8 ++
>   drivers/extcon/Makefile          |   1 +
>   drivers/extcon/extcon-otg_gpio.c | 200 +++++++++++++++++++++++++++++++++++++++
>   3 files changed, 209 insertions(+)
>   create mode 100644 drivers/extcon/extcon-otg_gpio.c
>
> diff --git a/drivers/extcon/Kconfig b/drivers/extcon/Kconfig
> index 6a1f7de6fa54..e8010cda4642 100644
> --- a/drivers/extcon/Kconfig
> +++ b/drivers/extcon/Kconfig
> @@ -93,4 +93,12 @@ config EXTCON_SM5502
>   	  Silicon Mitus SM5502. The SM5502 is a USB port accessory
>   	  detector and switch.
>
> +config EXTCON_OTG_GPIO
> +	tristate "VIRTUAL USB OTG PORT support"
> +	depends on GPIOLIB
> +	help
> +	  Say Y here to enable support for virtual USB OTG port device
> +	  controlled by GPIOs. This driver can be used when at least USB ID pin
> +	  is connected directly to GPIO.
> +

    The entries in this file seem alphabetically sorted.

>   endif # MULTISTATE_SWITCH
> diff --git a/drivers/extcon/Makefile b/drivers/extcon/Makefile
> index 0370b42e5a27..9e81088c6584 100644
> --- a/drivers/extcon/Makefile
> +++ b/drivers/extcon/Makefile
> @@ -12,3 +12,4 @@ obj-$(CONFIG_EXTCON_MAX8997)	+= extcon-max8997.o
>   obj-$(CONFIG_EXTCON_PALMAS)	+= extcon-palmas.o
>   obj-$(CONFIG_EXTCON_RT8973A)	+= extcon-rt8973a.o
>   obj-$(CONFIG_EXTCON_SM5502)	+= extcon-sm5502.o
> +obj-$(CONFIG_EXTCON_OTG_GPIO) += extcon-otg_gpio.o

    The lines here are sorted too.

> diff --git a/drivers/extcon/extcon-otg_gpio.c b/drivers/extcon/extcon-otg_gpio.c
> new file mode 100644
> index 000000000000..c5ee765a5f4f
> --- /dev/null
> +++ b/drivers/extcon/extcon-otg_gpio.c
> @@ -0,0 +1,200 @@
[...]
> +static irqreturn_t vuport_isr(int irq, void *priv)
> +{
> +	return IRQ_WAKE_THREAD;
> +}

    It's the same as the default IRQ handler...

> +	ret = devm_request_threaded_irq(dev, gpiod_to_irq(vup->gpio_usb_id),
> +					vuport_isr, vuport_thread_isr,

    ... so you probably can just pass NULL instead of vuport_isr, no?

[...]

> +static int __init vuport_init(void)
> +{
> +	return platform_driver_register(&vuport_driver);
> +}
> +subsys_initcall(vuport_init);

    Hm, why?

WBR, Sergei

Re: [RFC/PATCH] extcon: otg_gpio: add driver for USB OTG port controlled by GPIO(s)

[David Cohen]

Hi Peter,

Thanks for the review.

On Tue, Dec 23, 2014 at 09:25:18AM +0800, Peter Chen wrote:
> On Mon, Dec 22, 2014 at 02:43:37PM -0800, David Cohen wrote:
> > Some platforms have an USB OTG port fully (or partially) controlled by
> > GPIOs:
> > 
> > (1) USB ID is connected directly to GPIO
> > 
> > Optionally:
> > (2) VBUS is enabled by a GPIO (when ID is grounded)
> > (3) Platform has 2 USB controllers connected to same port: one for
> >     device and one for host role. D+/- are switched between phys
> >     by GPIO.
> 
> Would you explain how it works? Choosing controller runtime?

Both controllers are (indirectly) connected to the same micro B port.
The D+/- goes from the port to a switch operated by a GPIO. From the
switch, D+/- may go to Host controller's phy or Device controller's phy.
Depends on the GPIO level.

> 
> > 
> > As per initial version, this driver has the duty to control whether
> > USB-Host cable is plugged in or not:
> 
> You mean Micro-AB cable, right?

In my case I'd say micro B. But USB-Host would be any cable or
combination of cables where USB ID is grounded.

> 
> >  - If yes, OTG port is configured for host role
> >  - If no, by standard, the OTG port is configured for device role
> > 
> > Signed-off-by: David Cohen <david.a.cohen@linux.intel.com>
> > ---
> > 
> > Hi,
> > 
> > Some Intel Bay Trail boards have an unusual way to handle the USB OTG port:
> >  - The USB ID pin is connected directly to GPIO on SoC
> >  - When in host role, VBUS is provided by enabling a GPIO
> >  - Device and host roles are supported by 2 independent controllers with D+/-
> >    pins from port switched between different phys according a GPIO level.
> > 
> > The ACPI table describes this USB port as a (virtual) device with all the
> > necessary GPIOs. This driver implements support to this virtual device as an
> > extcon class driver. All drivers that depend on the USB OTG port state (USB phy,
> > PMIC, charge detection) will listen to extcon events.
> > 
> > Comments are welcome.
> > 
> > Br, David
> > ---
> > 
> >  drivers/extcon/Kconfig           |   8 ++
> >  drivers/extcon/Makefile          |   1 +
> >  drivers/extcon/extcon-otg_gpio.c | 200 +++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 209 insertions(+)
> >  create mode 100644 drivers/extcon/extcon-otg_gpio.c
> > 
> > diff --git a/drivers/extcon/Kconfig b/drivers/extcon/Kconfig
> > index 6a1f7de6fa54..e8010cda4642 100644
> > --- a/drivers/extcon/Kconfig
> > +++ b/drivers/extcon/Kconfig
> > @@ -93,4 +93,12 @@ config EXTCON_SM5502
> >  	  Silicon Mitus SM5502. The SM5502 is a USB port accessory
> >  	  detector and switch.
> >  
> > +config EXTCON_OTG_GPIO
> > +	tristate "VIRTUAL USB OTG PORT support"
> > +	depends on GPIOLIB
> > +	help
> > +	  Say Y here to enable support for virtual USB OTG port device
> > +	  controlled by GPIOs. This driver can be used when at least USB ID pin
> > +	  is connected directly to GPIO.
> > +
> >  endif # MULTISTATE_SWITCH
> > diff --git a/drivers/extcon/Makefile b/drivers/extcon/Makefile
> > index 0370b42e5a27..9e81088c6584 100644
> > --- a/drivers/extcon/Makefile
> > +++ b/drivers/extcon/Makefile
> > @@ -12,3 +12,4 @@ obj-$(CONFIG_EXTCON_MAX8997)	+= extcon-max8997.o
> >  obj-$(CONFIG_EXTCON_PALMAS)	+= extcon-palmas.o
> >  obj-$(CONFIG_EXTCON_RT8973A)	+= extcon-rt8973a.o
> >  obj-$(CONFIG_EXTCON_SM5502)	+= extcon-sm5502.o
> > +obj-$(CONFIG_EXTCON_OTG_GPIO) += extcon-otg_gpio.o
> > diff --git a/drivers/extcon/extcon-otg_gpio.c b/drivers/extcon/extcon-otg_gpio.c
> > new file mode 100644
> > index 000000000000..c5ee765a5f4f
> > --- /dev/null
> > +++ b/drivers/extcon/extcon-otg_gpio.c
> > @@ -0,0 +1,200 @@
> > +/*
> > + * Virtual USB OTG Port driver controlled by gpios
> > + *
> > + * Copyright (c) 2014, Intel Corporation.
> > + * Author: David Cohen <david.a.cohen@linux.intel.com>
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + *
> > + * 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/acpi.h>
> > +#include <linux/extcon.h>
> > +#include <linux/gpio.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/platform_device.h>
> > +
> > +#define DRV_NAME	"usb_otg_port"
> > +
> > +struct vuport {
> > +	struct device *dev;
> > +	struct gpio_desc *gpio_vbus_en;
> > +	struct gpio_desc *gpio_usb_id;
> > +	struct gpio_desc *gpio_usb_mux;
> > +
> > +	struct extcon_dev edev;
> > +};
> > +
> > +static const char *const vuport_extcon_cable[] = {
> > +	[0] = "USB-Host",
> > +	NULL,
> > +};
> > +
> > +/*
> > + * If id == 1, USB port should be set to peripheral
> > + * if id == 0, USB port should be set to host
> > + *
> > + * Peripheral: set USB mux to peripheral and disable VBUS
> > + * Host: set USB mux to host and enable VBUS
> > + */
> > +static void vuport_set_port(struct vuport *vup, int id)
> > +{
> > +	int mux_val = id;
> > +	int vbus_val = !id;
> > +
> > +	if (!IS_ERR(vup->gpio_usb_mux))
> > +		gpiod_direction_output(vup->gpio_usb_mux, mux_val);
> > +
> > +	if (!IS_ERR(vup->gpio_vbus_en))
> > +		gpiod_direction_output(vup->gpio_vbus_en, vbus_val);
> > +}
> > +
> > +static void vuport_do_usb_id(struct vuport *vup)
> > +{
> > +	int id = gpiod_get_value(vup->gpio_usb_id);
> > +
> > +	dev_info(vup->dev, "USB PORT ID: %s\n", id ? "PERIPHERAL" : "HOST");
> 
> dev_dbg

Agreed.

> 
> > +
> > +	/*
> > +	 * id == 1: PERIPHERAL
> > +	 * id == 0: HOST
> > +	 */
> > +	vuport_set_port(vup, id);
> > +
> > +	/*
> > +	 * id == 0: HOST connected
> > +	 * id == 1: Host disconnected
> > +	 */
> > +	extcon_set_cable_state(&vup->edev, "USB-Host", !id);
> > +}
> > +
> > +static irqreturn_t vuport_thread_isr(int irq, void *priv)
> > +{
> > +	struct vuport *vup = priv;
> > +
> > +	vuport_do_usb_id(vup);
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +static irqreturn_t vuport_isr(int irq, void *priv)
> > +{
> > +	return IRQ_WAKE_THREAD;
> > +}
> > +
> > +#define VUPORT_GPIO_USB_ID	0
> > +#define VUPORT_GPIO_VBUS_EN	1
> > +#define VUPORT_GPIO_USB_MUX	2
> > +static int vuport_probe(struct platform_device *pdev)
> > +{
> > +	struct device *dev = &pdev->dev;
> > +	struct vuport *vup;
> > +	int ret;
> > +
> > +	vup = devm_kzalloc(dev, sizeof(*vup), GFP_KERNEL);
> > +	if (!vup) {
> > +		dev_err(dev, "cannot allocate private data\n");
> > +		return -ENOMEM;
> > +	}
> > +	vup->dev = dev;
> > +
> > +	vup->gpio_usb_id = devm_gpiod_get_index(dev, "id", VUPORT_GPIO_USB_ID);
> > +	if (IS_ERR(vup->gpio_usb_id)) {
> > +		dev_err(dev, "cannot request USB ID GPIO: ret = %ld\n",
> > +			PTR_ERR(vup->gpio_usb_id));
> > +		return PTR_ERR(vup->gpio_usb_id);
> > +	}
> > +
> > +	ret = gpiod_direction_input(vup->gpio_usb_id);
> > +	if (ret < 0) {
> > +		dev_err(dev, "cannot set input direction to USB ID GPIO: ret = %d\n",
> > +			ret);
> > +		return ret;
> > +	}
> > +
> > +	vup->gpio_vbus_en = devm_gpiod_get_index(dev, "vbus en",
> > +						 VUPORT_GPIO_VBUS_EN);
> > +	if (IS_ERR(vup->gpio_vbus_en))
> > +		dev_info(dev, "cannot request VBUS EN GPIO, skipping it.\n");
> > +
> > +	vup->gpio_usb_mux = devm_gpiod_get_index(dev, "usb mux",
> > +						 VUPORT_GPIO_USB_MUX);
> > +	if (IS_ERR(vup->gpio_usb_mux))
> > +		dev_info(dev, "cannot request USB USB MUX, skipping it.\n");
> 
> Using dev_err

That's not really an error, although the IS_ERR() suggests otherwise.
The driver works well if a board doesn't need this mux (I'll add a
comment to state that clear). IMHO either keep dev_info or use dev_dgb
instead?

Br, David

> 
> > +
> > +	/* register extcon device */
> > +	vup->edev.dev.parent = dev;
> > +	vup->edev.supported_cable = vuport_extcon_cable;
> > +	ret = extcon_dev_register(&vup->edev);
> > +	if (ret < 0) {
> > +		dev_err(dev, "failed to register extcon device: ret = %d\n",
> > +			ret);
> > +		return ret;
> > +	}
> > +
> > +	ret = devm_request_threaded_irq(dev, gpiod_to_irq(vup->gpio_usb_id),
> > +					vuport_isr, vuport_thread_isr,
> > +					IRQF_SHARED | IRQF_TRIGGER_RISING |
> > +					IRQF_TRIGGER_FALLING,
> > +					dev_name(dev), vup);
> > +	if (ret < 0) {
> > +		dev_err(dev, "cannot request IRQ for USB ID GPIO: ret = %d\n",
> > +			ret);
> > +		goto irq_err;
> > +	}
> > +	vuport_do_usb_id(vup);
> > +
> > +	platform_set_drvdata(pdev, vup);
> > +
> > +	dev_info(dev, "driver successfully probed\n");
> > +
> > +	return 0;
> > +
> > +irq_err:
> > +	extcon_dev_unregister(&vup->edev);
> > +
> > +	return ret;
> > +}
> > +
> > +static int vuport_remove(struct platform_device *pdev)
> > +{
> > +	struct vuport *vup = platform_get_drvdata(pdev);
> > +
> > +	extcon_dev_unregister(&vup->edev);
> > +	return 0;
> > +}
> > +
> > +static struct acpi_device_id vuport_acpi_match[] = {
> > +	{ "INT3496" },
> > +	{ }
> > +};
> > +MODULE_DEVICE_TABLE(acpi, vuport_acpi_match);
> > +
> > +static struct platform_driver vuport_driver = {
> > +	.driver = {
> > +		.name = DRV_NAME,
> > +		.acpi_match_table = ACPI_PTR(vuport_acpi_match),
> > +	},
> > +	.probe = vuport_probe,
> > +	.remove = vuport_remove,
> > +};
> > +
> > +static int __init vuport_init(void)
> > +{
> > +	return platform_driver_register(&vuport_driver);
> > +}
> > +subsys_initcall(vuport_init);
> > +
> > +static void __exit vuport_exit(void)
> > +{
> > +	platform_driver_unregister(&vuport_driver);
> > +}
> > +module_exit(vuport_exit);
> > +
> > +MODULE_LICENSE("GPL");
> > +MODULE_AUTHOR("David Cohen <david.a.cohen@linux.intel.com>");
> > -- 
> > 2.1.1
> > 
> 
> -- 
> 
> Best Regards,
> Peter Chen

Re: [RFC/PATCH] extcon: otg_gpio: add driver for USB OTG port controlled by GPIO(s)

[David Cohen]

Hi Sergei,

On Tue, Dec 23, 2014 at 04:10:44PM +0300, Sergei Shtylyov wrote:
> Hello.
> 
> On 12/23/2014 1:43 AM, David Cohen wrote:
> 
> >Some platforms have an USB OTG port fully (or partially) controlled by
> >GPIOs:
> 
> >(1) USB ID is connected directly to GPIO
> 
> >Optionally:
> >(2) VBUS is enabled by a GPIO (when ID is grounded)
> 
>    Can't the host driver still control Vbus?

I can't a clean way for host driver to control VBUS considering it
depends on USB ID.

> 
> >(3) Platform has 2 USB controllers connected to same port: one for
> >     device and one for host role. D+/- are switched between phys
> >     by GPIO.
> 
> >As per initial version, this driver has the duty to control whether
> >USB-Host cable is plugged in or not:
> >  - If yes, OTG port is configured for host role
> >  - If no, by standard, the OTG port is configured for device role
> 
> >Signed-off-by: David Cohen <david.a.cohen@linux.intel.com>
> >---
> 
> >Hi,
> 
> >Some Intel Bay Trail boards have an unusual way to handle the USB OTG port:
> >  - The USB ID pin is connected directly to GPIO on SoC
> >  - When in host role, VBUS is provided by enabling a GPIO
> >  - Device and host roles are supported by 2 independent controllers with D+/-
> >    pins from port switched between different phys according a GPIO level.
> 
> >The ACPI table describes this USB port as a (virtual) device with all the
> >necessary GPIOs. This driver implements support to this virtual device as an
> >extcon class driver. All drivers that depend on the USB OTG port state (USB phy,
> >PMIC, charge detection) will listen to extcon events.
> 
>    It's very close to my setup on R-Car R8A7791 based boards. :-)
> I have already submitted Maxim MAX3355 OTG chip GPIO-based driver.

Hm. I'll look for it. Thanks for pointing.

> 
> >Comments are welcome.
> 
> >Br, David
> >---
> >
> >  drivers/extcon/Kconfig           |   8 ++
> >  drivers/extcon/Makefile          |   1 +
> >  drivers/extcon/extcon-otg_gpio.c | 200 +++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 209 insertions(+)
> >  create mode 100644 drivers/extcon/extcon-otg_gpio.c
> >
> >diff --git a/drivers/extcon/Kconfig b/drivers/extcon/Kconfig
> >index 6a1f7de6fa54..e8010cda4642 100644
> >--- a/drivers/extcon/Kconfig
> >+++ b/drivers/extcon/Kconfig
> >@@ -93,4 +93,12 @@ config EXTCON_SM5502
> >  	  Silicon Mitus SM5502. The SM5502 is a USB port accessory
> >  	  detector and switch.
> >
> >+config EXTCON_OTG_GPIO
> >+	tristate "VIRTUAL USB OTG PORT support"
> >+	depends on GPIOLIB
> >+	help
> >+	  Say Y here to enable support for virtual USB OTG port device
> >+	  controlled by GPIOs. This driver can be used when at least USB ID pin
> >+	  is connected directly to GPIO.
> >+
> 
>    The entries in this file seem alphabetically sorted.

I'll fix it.

> 
> >  endif # MULTISTATE_SWITCH
> >diff --git a/drivers/extcon/Makefile b/drivers/extcon/Makefile
> >index 0370b42e5a27..9e81088c6584 100644
> >--- a/drivers/extcon/Makefile
> >+++ b/drivers/extcon/Makefile
> >@@ -12,3 +12,4 @@ obj-$(CONFIG_EXTCON_MAX8997)	+= extcon-max8997.o
> >  obj-$(CONFIG_EXTCON_PALMAS)	+= extcon-palmas.o
> >  obj-$(CONFIG_EXTCON_RT8973A)	+= extcon-rt8973a.o
> >  obj-$(CONFIG_EXTCON_SM5502)	+= extcon-sm5502.o
> >+obj-$(CONFIG_EXTCON_OTG_GPIO) += extcon-otg_gpio.o
> 
>    The lines here are sorted too.

Ditto.

> 
> >diff --git a/drivers/extcon/extcon-otg_gpio.c b/drivers/extcon/extcon-otg_gpio.c
> >new file mode 100644
> >index 000000000000..c5ee765a5f4f
> >--- /dev/null
> >+++ b/drivers/extcon/extcon-otg_gpio.c
> >@@ -0,0 +1,200 @@
> [...]
> >+static irqreturn_t vuport_isr(int irq, void *priv)
> >+{
> >+	return IRQ_WAKE_THREAD;
> >+}
> 
>    It's the same as the default IRQ handler...
> 
> >+	ret = devm_request_threaded_irq(dev, gpiod_to_irq(vup->gpio_usb_id),
> >+					vuport_isr, vuport_thread_isr,
> 
>    ... so you probably can just pass NULL instead of vuport_isr, no?

I'll review that.

> 
> [...]
> 
> >+static int __init vuport_init(void)
> >+{
> >+	return platform_driver_register(&vuport_driver);
> >+}
> >+subsys_initcall(vuport_init);
> 
>    Hm, why?

We have drivers that depend on this one during their probe.

Br, David

> 
> WBR, Sergei
> 

Re: [RFC/PATCH] extcon: otg_gpio: add driver for USB OTG port controlled by GPIO(s)

[Sergei Shtylyov]

Hello.

On 12/23/2014 10:57 PM, David Cohen wrote:

>>> Some platforms have an USB OTG port fully (or partially) controlled by
>>> GPIOs:

>>> (1) USB ID is connected directly to GPIO

>>> Optionally:
>>> (2) VBUS is enabled by a GPIO (when ID is grounded)

>>     Can't the host driver still control Vbus?

> I can't a clean way for host driver to control VBUS considering it
> depends on USB ID.

    You're using the cable state notifiers, why not control Vbus from there?
You need some way of passing the GPIO to host driver though... I assume you're 
not using the device tree, and your host controllers live on PCI, so the 
platform data is out of question. You may be right then...

>>> (3) Platform has 2 USB controllers connected to same port: one for
>>>      device and one for host role. D+/- are switched between phys
>>>      by GPIO.

>>> As per initial version, this driver has the duty to control whether
>>> USB-Host cable is plugged in or not:
>>>   - If yes, OTG port is configured for host role
>>>   - If no, by standard, the OTG port is configured for device role

>>> Signed-off-by: David Cohen <david.a.cohen@linux.intel.com>
>>> ---

>>> Hi,

>>> Some Intel Bay Trail boards have an unusual way to handle the USB OTG port:
>>>   - The USB ID pin is connected directly to GPIO on SoC
>>>   - When in host role, VBUS is provided by enabling a GPIO
>>>   - Device and host roles are supported by 2 independent controllers with D+/-
>>>     pins from port switched between different phys according a GPIO level.

>>> The ACPI table describes this USB port as a (virtual) device with all the
>>> necessary GPIOs. This driver implements support to this virtual device as an
>>> extcon class driver. All drivers that depend on the USB OTG port state (USB phy,
>>> PMIC, charge detection) will listen to extcon events.

>>     It's very close to my setup on R-Car R8A7791 based boards. :-)
>> I have already submitted Maxim MAX3355 OTG chip GPIO-based driver.

> Hm. I'll look for it. Thanks for pointing.

    http://marc.info/?l=linux-usb&m=141825413802370
    In my case, Vbus is not controlled via GPIO though. I would have probably 
used the generic GPIO extcon driver if I didn't have to drive MAX3355's SHDN# 
pin high...
    There were also some other patches for this issue, the one probably 
interesting to you is there:

    http://marc.info/?l=linux-usb&m=141877180912359

>>> Comments are welcome.

>>> Br, David

[...]

>>> +static int __init vuport_init(void)
>>> +{
>>> +	return platform_driver_register(&vuport_driver);
>>> +}
>>> +subsys_initcall(vuport_init);

>>     Hm, why?

> We have drivers that depend on this one during their probe.

    What about deferred probing? With EPROBE_DEFER we don't need to play the 
initcall games any more AFAIU.

> Br, David

WBR, Sergei

Re: [RFC/PATCH] extcon: otg_gpio: add driver for USB OTG port controlled by GPIO(s)

[David Cohen]

On Tue, Dec 23, 2014 at 11:09:44PM +0300, Sergei Shtylyov wrote:
> Hello.
> 
> On 12/23/2014 10:57 PM, David Cohen wrote:
> 
> >>>Some platforms have an USB OTG port fully (or partially) controlled by
> >>>GPIOs:
> 
> >>>(1) USB ID is connected directly to GPIO
> 
> >>>Optionally:
> >>>(2) VBUS is enabled by a GPIO (when ID is grounded)
> 
> >>    Can't the host driver still control Vbus?
> 
> >I can't a clean way for host driver to control VBUS considering it
> >depends on USB ID.
> 
>    You're using the cable state notifiers, why not control Vbus from there?
> You need some way of passing the GPIO to host driver though... I assume
> you're not using the device tree, and your host controllers live on PCI, so
> the platform data is out of question. You may be right then...

Yes to all questions :)

> 
> >>>(3) Platform has 2 USB controllers connected to same port: one for
> >>>     device and one for host role. D+/- are switched between phys
> >>>     by GPIO.
> 
> >>>As per initial version, this driver has the duty to control whether
> >>>USB-Host cable is plugged in or not:
> >>>  - If yes, OTG port is configured for host role
> >>>  - If no, by standard, the OTG port is configured for device role
> 
> >>>Signed-off-by: David Cohen <david.a.cohen@linux.intel.com>
> >>>---
> 
> >>>Hi,
> 
> >>>Some Intel Bay Trail boards have an unusual way to handle the USB OTG port:
> >>>  - The USB ID pin is connected directly to GPIO on SoC
> >>>  - When in host role, VBUS is provided by enabling a GPIO
> >>>  - Device and host roles are supported by 2 independent controllers with D+/-
> >>>    pins from port switched between different phys according a GPIO level.
> 
> >>>The ACPI table describes this USB port as a (virtual) device with all the
> >>>necessary GPIOs. This driver implements support to this virtual device as an
> >>>extcon class driver. All drivers that depend on the USB OTG port state (USB phy,
> >>>PMIC, charge detection) will listen to extcon events.
> 
> >>    It's very close to my setup on R-Car R8A7791 based boards. :-)
> >>I have already submitted Maxim MAX3355 OTG chip GPIO-based driver.
> 
> >Hm. I'll look for it. Thanks for pointing.
> 
>    http://marc.info/?l=linux-usb&m=141825413802370
>    In my case, Vbus is not controlled via GPIO though. I would have probably
> used the generic GPIO extcon driver if I didn't have to drive MAX3355's
> SHDN# pin high...

Besides the USB ID, I need to control VBUS and the D+/- switch. We have
a new use case coming soon that may need to add a second switch control.

>    There were also some other patches for this issue, the one probably
> interesting to you is there:
> 
>    http://marc.info/?l=linux-usb&m=141877180912359

This one is interesting, but I'm restricted to ACPI and to the DSDTs already
released.
E.g. http://www.androidauthority.com/trekstor-xintron-lollipop-564364/

Br, David

> 
> >>>Comments are welcome.
> 
> >>>Br, David
> 
> [...]
> 
> >>>+static int __init vuport_init(void)
> >>>+{
> >>>+	return platform_driver_register(&vuport_driver);
> >>>+}
> >>>+subsys_initcall(vuport_init);
> 
> >>    Hm, why?
> 
> >We have drivers that depend on this one during their probe.
> 
>    What about deferred probing? With EPROBE_DEFER we don't need to play the
> initcall games any more AFAIU.
> 
> >Br, David
> 
> WBR, Sergei
> 

Re: [RFC/PATCH] extcon: otg_gpio: add driver for USB OTG port controlled by GPIO(s)

[Felipe Balbi]

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

Hi,

On Mon, Dec 22, 2014 at 02:43:37PM -0800, David Cohen wrote:
> Some platforms have an USB OTG port fully (or partially) controlled by
> GPIOs:
> 
> (1) USB ID is connected directly to GPIO
> 
> Optionally:
> (2) VBUS is enabled by a GPIO (when ID is grounded)

ok, so a fixed regulator with a GPIO enable pin.

> (3) Platform has 2 USB controllers connected to same port: one for
>     device and one for host role. D+/- are switched between phys
>     by GPIO.

so you have discrete mux with a GPIO select signal, like below ?


 .-------.----------------.  .--------.
 |       |                |  |        |      D+
 |       |                |  |        |<-------------.
 |       |                |  |        |              |
 |       |    USB Host    -->|    P   |              |
 |       |                |  |    H   |              |
 |       |                |  |    Y   |    D-        |
 |       '----------------'  |    0   |<--------.    |
 |                       |   |        |         |    |
 |                       |   '--------'      .--------.  D+
 |                       |                   |        |------>
 |       SOC        GPIO | ----------------->|        |
 |                       |                   |   MUX  |
 |                       |                   |        |------>
 |                       |   .--------.      '--------'  D-
 |       .----------------.  |        |   D-  |      |
 |       |                |  |    P   |<------'      |
 |       |                |  |    H   |              |
 |       |                |  |    Y   |              |
 |       |   USB Device   -->|    1   |              |
 |       |                |  |        |      D+      |
 |       |                |  |        |<-------------'
 |       |                |  |        |
 '-------'----------------'  '--------'

I have been on and off about writing a pinctrl-gpio.c driver which would
allow us to hide this detail from users. It shouldn't really matter
which modes are available behind the mux, but we should be able to tell
the mux to go into mode 0 or mode 1 by toggling its select signal. And
it shouldn't really matter that we have a GPIO pin. The problem is: I
don't really know if pinctrl would be able to handle discrete muxes.

Adding Linus W to ask. Linus, what do you think ? Should we have a
pinctrl-gpio.c for such cases ? In TI we too have quite a few boards
which different modes hidden behind discrete muxes.

> As per initial version, this driver has the duty to control whether
> USB-Host cable is plugged in or not:
>  - If yes, OTG port is configured for host role
>  - If no, by standard, the OTG port is configured for device role

correct, this default-B is mandated by OTG spec anyway.

> Signed-off-by: David Cohen <david.a.cohen@linux.intel.com>
> ---
> 
> Hi,
> 
> Some Intel Bay Trail boards have an unusual way to handle the USB OTG port:
>  - The USB ID pin is connected directly to GPIO on SoC
>  - When in host role, VBUS is provided by enabling a GPIO
>  - Device and host roles are supported by 2 independent controllers with D+/-
>    pins from port switched between different phys according a GPIO level.
> 
> The ACPI table describes this USB port as a (virtual) device with all the
> necessary GPIOs. This driver implements support to this virtual device as an
> extcon class driver. All drivers that depend on the USB OTG port state (USB phy,
> PMIC, charge detection) will listen to extcon events.

Right I think you're almost there, but I still think that this needs to
be a bit broken down. First, we need some generic DRD library under
drivers/usb/common, and that should be the one listening to extcon cable
events. But your extcon driver should be doing only that: checking which
cable was attached, it shouldn't be doing the switch by itself. That
should be part of the DRD library.

That DRD library would also be the one enabling the (optional) VBUS
regulator.

George Cherian tried to implement a generic DRD library but I think
there are still too many changes happening on usbcore and udc-core. Most
of the pieces are already there (usb_del_hcd() and usb_del_gadget_udc()
know how to properly unload an hcd/udc), but there are details missing,
no doubt.

If we can find a way to broadcast (probably not the best term, but
whatever) a "Hey ID pin was just grounded" message, we can get things
working.

That message, btw, shouldn't really depend on extcon-gpio alone because
other platforms might use non-gpio methods to verify ID pin level. In
any case, we need to have generic ID_PIN_LOW and ID_PIN_HIGH messages
that a generic DRD library can listen to and load/unload the correct
drivers by means of usb_{add,del}_{hcd,gadget_udc}().

With that in mind, I think you can use extcon-gpio.c for your purposes
if the other pieces can be fullfilled by regulator and pinctrl.

> diff --git a/drivers/extcon/Makefile b/drivers/extcon/Makefile
> index 0370b42e5a27..9e81088c6584 100644
> --- a/drivers/extcon/Makefile
> +++ b/drivers/extcon/Makefile
> @@ -12,3 +12,4 @@ obj-$(CONFIG_EXTCON_MAX8997)	+= extcon-max8997.o
>  obj-$(CONFIG_EXTCON_PALMAS)	+= extcon-palmas.o
>  obj-$(CONFIG_EXTCON_RT8973A)	+= extcon-rt8973a.o
>  obj-$(CONFIG_EXTCON_SM5502)	+= extcon-sm5502.o
> +obj-$(CONFIG_EXTCON_OTG_GPIO) += extcon-otg_gpio.o
> diff --git a/drivers/extcon/extcon-otg_gpio.c b/drivers/extcon/extcon-otg_gpio.c
> new file mode 100644
> index 000000000000..c5ee765a5f4f
> --- /dev/null
> +++ b/drivers/extcon/extcon-otg_gpio.c
> @@ -0,0 +1,200 @@
> +/*
> + * Virtual USB OTG Port driver controlled by gpios
> + *
> + * Copyright (c) 2014, Intel Corporation.
> + * Author: David Cohen <david.a.cohen@linux.intel.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * 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/acpi.h>
> +#include <linux/extcon.h>
> +#include <linux/gpio.h>
> +#include <linux/interrupt.h>
> +#include <linux/platform_device.h>
> +
> +#define DRV_NAME	"usb_otg_port"
> +
> +struct vuport {
> +	struct device *dev;
> +	struct gpio_desc *gpio_vbus_en;
> +	struct gpio_desc *gpio_usb_id;
> +	struct gpio_desc *gpio_usb_mux;
> +
> +	struct extcon_dev edev;
> +};
> +
> +static const char *const vuport_extcon_cable[] = {
> +	[0] = "USB-Host",
> +	NULL,
> +};
> +
> +/*
> + * If id == 1, USB port should be set to peripheral
> + * if id == 0, USB port should be set to host
> + *
> + * Peripheral: set USB mux to peripheral and disable VBUS
> + * Host: set USB mux to host and enable VBUS
> + */
> +static void vuport_set_port(struct vuport *vup, int id)
> +{
> +	int mux_val = id;
> +	int vbus_val = !id;
> +
> +	if (!IS_ERR(vup->gpio_usb_mux))
> +		gpiod_direction_output(vup->gpio_usb_mux, mux_val);
> +
> +	if (!IS_ERR(vup->gpio_vbus_en))
> +		gpiod_direction_output(vup->gpio_vbus_en, vbus_val);

not all SoCs will allow for direction to be set all the time. This can
be glitchy in some cases. What you want here is to set direction during
probe and just set value here.

> +static void vuport_do_usb_id(struct vuport *vup)
> +{
> +	int id = gpiod_get_value(vup->gpio_usb_id);
> +
> +	dev_info(vup->dev, "USB PORT ID: %s\n", id ? "PERIPHERAL" : "HOST");

info ? sounds like debug to me.

> +
> +	/*
> +	 * id == 1: PERIPHERAL
> +	 * id == 0: HOST
> +	 */
> +	vuport_set_port(vup, id);
> +
> +	/*
> +	 * id == 0: HOST connected
> +	 * id == 1: Host disconnected
> +	 */
> +	extcon_set_cable_state(&vup->edev, "USB-Host", !id);
> +}
> +
> +static irqreturn_t vuport_thread_isr(int irq, void *priv)
> +{

this is unrelated to $subject, but I always consider if we should have a
generic way to figure out if the interrupt was for rising or falling
edge, if we knew that, we could avoid reading the GPIO value altogether
;-)

> +	struct vuport *vup = priv;
> +
> +	vuport_do_usb_id(vup);
> +	return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t vuport_isr(int irq, void *priv)
> +{
> +	return IRQ_WAKE_THREAD;
> +}

you don't need this. Set the top half handler to NULL and pass
IRQF_ONESHOT (which you shoudl already have set anyway).

> +#define VUPORT_GPIO_USB_ID	0
> +#define VUPORT_GPIO_VBUS_EN	1
> +#define VUPORT_GPIO_USB_MUX	2
> +static int vuport_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct vuport *vup;
> +	int ret;
> +
> +	vup = devm_kzalloc(dev, sizeof(*vup), GFP_KERNEL);
> +	if (!vup) {
> +		dev_err(dev, "cannot allocate private data\n");
> +		return -ENOMEM;
> +	}
> +	vup->dev = dev;
> +
> +	vup->gpio_usb_id = devm_gpiod_get_index(dev, "id", VUPORT_GPIO_USB_ID);
> +	if (IS_ERR(vup->gpio_usb_id)) {
> +		dev_err(dev, "cannot request USB ID GPIO: ret = %ld\n",
> +			PTR_ERR(vup->gpio_usb_id));
> +		return PTR_ERR(vup->gpio_usb_id);
> +	}
> +
> +	ret = gpiod_direction_input(vup->gpio_usb_id);
> +	if (ret < 0) {
> +		dev_err(dev, "cannot set input direction to USB ID GPIO: ret = %d\n",
> +			ret);
> +		return ret;
> +	}
> +
> +	vup->gpio_vbus_en = devm_gpiod_get_index(dev, "vbus en",
> +						 VUPORT_GPIO_VBUS_EN);
> +	if (IS_ERR(vup->gpio_vbus_en))
> +		dev_info(dev, "cannot request VBUS EN GPIO, skipping it.\n");
> +
> +	vup->gpio_usb_mux = devm_gpiod_get_index(dev, "usb mux",
> +						 VUPORT_GPIO_USB_MUX);
> +	if (IS_ERR(vup->gpio_usb_mux))
> +		dev_info(dev, "cannot request USB USB MUX, skipping it.\n");
> +
> +	/* register extcon device */
> +	vup->edev.dev.parent = dev;
> +	vup->edev.supported_cable = vuport_extcon_cable;

IIRC, edev should be allocated from now on. Have a look at
devm_extcon_dev_allocate() and devm_extcon_dev_register().

> +	ret = extcon_dev_register(&vup->edev);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to register extcon device: ret = %d\n",
> +			ret);
> +		return ret;
> +	}
> +
> +	ret = devm_request_threaded_irq(dev, gpiod_to_irq(vup->gpio_usb_id),
> +					vuport_isr, vuport_thread_isr,
> +					IRQF_SHARED | IRQF_TRIGGER_RISING |
> +					IRQF_TRIGGER_FALLING,
> +					dev_name(dev), vup);
> +	if (ret < 0) {
> +		dev_err(dev, "cannot request IRQ for USB ID GPIO: ret = %d\n",
> +			ret);
> +		goto irq_err;
> +	}
> +	vuport_do_usb_id(vup);
> +
> +	platform_set_drvdata(pdev, vup);
> +
> +	dev_info(dev, "driver successfully probed\n");

this will just make boot noisier, make it dev_dbg() ? Or even
dev_vdbg() ?

> +MODULE_LICENSE("GPL");

you header on the top of this C file states gpl 2 only, but this says
GPL 2+.

cheers

-- 
balbi

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

Re: [RFC/PATCH] extcon: otg_gpio: add driver for USB OTG port controlled by GPIO(s)

[Peter Chen]

On Tue, Dec 23, 2014 at 11:40:23AM -0800, David Cohen wrote:
> Hi Peter,
> 
> Thanks for the review.
> 
> On Tue, Dec 23, 2014 at 09:25:18AM +0800, Peter Chen wrote:
> > On Mon, Dec 22, 2014 at 02:43:37PM -0800, David Cohen wrote:
> > > Some platforms have an USB OTG port fully (or partially) controlled by
> > > GPIOs:
> > > 
> > > (1) USB ID is connected directly to GPIO
> > > 
> > > Optionally:
> > > (2) VBUS is enabled by a GPIO (when ID is grounded)
> > > (3) Platform has 2 USB controllers connected to same port: one for
> > >     device and one for host role. D+/- are switched between phys
> > >     by GPIO.
> > 
> > Would you explain how it works? Choosing controller runtime?
> 
> Both controllers are (indirectly) connected to the same micro B port.
> The D+/- goes from the port to a switch operated by a GPIO. From the
> switch, D+/- may go to Host controller's phy or Device controller's phy.
> Depends on the GPIO level.
> 

Get it, why the design like that? If your controller supports both
roles, the software can do role switch by ID pin (through gpio in your
case).

> > 
> > > 
> > > As per initial version, this driver has the duty to control whether
> > > USB-Host cable is plugged in or not:
> > 
> > You mean Micro-AB cable, right?
> 
> > > +
> > > +	vup->gpio_usb_mux = devm_gpiod_get_index(dev, "usb mux",
> > > +						 VUPORT_GPIO_USB_MUX);
> > > +	if (IS_ERR(vup->gpio_usb_mux))
> > > +		dev_info(dev, "cannot request USB USB MUX, skipping it.\n");
> > 
> > Using dev_err
> 
> That's not really an error, although the IS_ERR() suggests otherwise.
> The driver works well if a board doesn't need this mux (I'll add a
> comment to state that clear). IMHO either keep dev_info or use dev_dgb
> instead?
> 

If that, dev_dbg may be suitable.


-- 

Best Regards,
Peter Chen

Re: [RFC/PATCH] extcon: otg_gpio: add driver for USB OTG port controlled by GPIO(s)

[David Cohen]

Hi Felipe,

Thanks replying.

On Tue, Dec 23, 2014 at 06:29:04PM -0600, Felipe Balbi wrote:
> Hi,
> 
> On Mon, Dec 22, 2014 at 02:43:37PM -0800, David Cohen wrote:
> > Some platforms have an USB OTG port fully (or partially) controlled by
> > GPIOs:
> > 
> > (1) USB ID is connected directly to GPIO
> > 
> > Optionally:
> > (2) VBUS is enabled by a GPIO (when ID is grounded)
> 
> ok, so a fixed regulator with a GPIO enable pin.

Pretty much yes.

> 
> > (3) Platform has 2 USB controllers connected to same port: one for
> >     device and one for host role. D+/- are switched between phys
> >     by GPIO.
> 
> so you have discrete mux with a GPIO select signal, like below ?
> 
> 
>  .-------.----------------.  .--------.
>  |       |                |  |        |      D+
>  |       |                |  |        |<-------------.
>  |       |                |  |        |              |
>  |       |    USB Host    -->|    P   |              |
>  |       |                |  |    H   |              |
>  |       |                |  |    Y   |    D-        |
>  |       '----------------'  |    0   |<--------.    |
>  |                       |   |        |         |    |
>  |                       |   '--------'      .--------.  D+
>  |                       |                   |        |------>
>  |       SOC        GPIO | ----------------->|        |
>  |                       |                   |   MUX  |
>  |                       |                   |        |------>
>  |                       |   .--------.      '--------'  D-
>  |       .----------------.  |        |   D-  |      |
>  |       |                |  |    P   |<------'      |
>  |       |                |  |    H   |              |
>  |       |                |  |    Y   |              |
>  |       |   USB Device   -->|    1   |              |
>  |       |                |  |        |      D+      |
>  |       |                |  |        |<-------------'
>  |       |                |  |        |
>  '-------'----------------'  '--------'

Nice ASCII pic :)
Yes, that's the case.

> 
> I have been on and off about writing a pinctrl-gpio.c driver which would
> allow us to hide this detail from users. It shouldn't really matter
> which modes are available behind the mux, but we should be able to tell
> the mux to go into mode 0 or mode 1 by toggling its select signal. And
> it shouldn't really matter that we have a GPIO pin. The problem is: I
> don't really know if pinctrl would be able to handle discrete muxes.
> 
> Adding Linus W to ask. Linus, what do you think ? Should we have a
> pinctrl-gpio.c for such cases ? In TI we too have quite a few boards
> which different modes hidden behind discrete muxes.

An input from Linus would fine in this case.

> 
> > As per initial version, this driver has the duty to control whether
> > USB-Host cable is plugged in or not:
> >  - If yes, OTG port is configured for host role
> >  - If no, by standard, the OTG port is configured for device role
> 
> correct, this default-B is mandated by OTG spec anyway.
> 
> > Signed-off-by: David Cohen <david.a.cohen@linux.intel.com>
> > ---
> > 
> > Hi,
> > 
> > Some Intel Bay Trail boards have an unusual way to handle the USB OTG port:
> >  - The USB ID pin is connected directly to GPIO on SoC
> >  - When in host role, VBUS is provided by enabling a GPIO
> >  - Device and host roles are supported by 2 independent controllers with D+/-
> >    pins from port switched between different phys according a GPIO level.
> > 
> > The ACPI table describes this USB port as a (virtual) device with all the
> > necessary GPIOs. This driver implements support to this virtual device as an
> > extcon class driver. All drivers that depend on the USB OTG port state (USB phy,
> > PMIC, charge detection) will listen to extcon events.
> 
> Right I think you're almost there, but I still think that this needs to
> be a bit broken down. First, we need some generic DRD library under
> drivers/usb/common, and that should be the one listening to extcon cable
> events. But your extcon driver should be doing only that: checking which
> cable was attached, it shouldn't be doing the switch by itself. That
> should be part of the DRD library.
> 
> That DRD library would also be the one enabling the (optional) VBUS
> regulator.
> 
> George Cherian tried to implement a generic DRD library but I think
> there are still too many changes happening on usbcore and udc-core. Most
> of the pieces are already there (usb_del_hcd() and usb_del_gadget_udc()
> know how to properly unload an hcd/udc), but there are details missing,
> no doubt.
> 
> If we can find a way to broadcast (probably not the best term, but
> whatever) a "Hey ID pin was just grounded" message, we can get things
> working.
> 
> That message, btw, shouldn't really depend on extcon-gpio alone because
> other platforms might use non-gpio methods to verify ID pin level. In
> any case, we need to have generic ID_PIN_LOW and ID_PIN_HIGH messages
> that a generic DRD library can listen to and load/unload the correct
> drivers by means of usb_{add,del}_{hcd,gadget_udc}().

IMHO extcon is the correct way to broadcast it, as long as we define a
standard for the cable names. E.g. DRD library could listen to
"USB-HOST" cable state. Then it doesn't matter how ID pin is grounded,
it just matters that whoever is controlling it broadcast via this cable.

> 
> With that in mind, I think you can use extcon-gpio.c for your purposes
> if the other pieces can be fullfilled by regulator and pinctrl.

In my case we have all gpios listed in a single ACPI device. In order to
be backwards compatible with products already on market, we'd need
something like a single mfd to register platform devices for this
smaller pieces (extcon gpio, possible pintrl gpio, maybe vbus as regulator??).

> 
> > diff --git a/drivers/extcon/Makefile b/drivers/extcon/Makefile
> > index 0370b42e5a27..9e81088c6584 100644
> > --- a/drivers/extcon/Makefile
> > +++ b/drivers/extcon/Makefile
> > @@ -12,3 +12,4 @@ obj-$(CONFIG_EXTCON_MAX8997)	+= extcon-max8997.o
> >  obj-$(CONFIG_EXTCON_PALMAS)	+= extcon-palmas.o
> >  obj-$(CONFIG_EXTCON_RT8973A)	+= extcon-rt8973a.o
> >  obj-$(CONFIG_EXTCON_SM5502)	+= extcon-sm5502.o
> > +obj-$(CONFIG_EXTCON_OTG_GPIO) += extcon-otg_gpio.o
> > diff --git a/drivers/extcon/extcon-otg_gpio.c b/drivers/extcon/extcon-otg_gpio.c
> > new file mode 100644
> > index 000000000000..c5ee765a5f4f
> > --- /dev/null
> > +++ b/drivers/extcon/extcon-otg_gpio.c
> > @@ -0,0 +1,200 @@
> > +/*
> > + * Virtual USB OTG Port driver controlled by gpios
> > + *
> > + * Copyright (c) 2014, Intel Corporation.
> > + * Author: David Cohen <david.a.cohen@linux.intel.com>
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + *
> > + * 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/acpi.h>
> > +#include <linux/extcon.h>
> > +#include <linux/gpio.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/platform_device.h>
> > +
> > +#define DRV_NAME	"usb_otg_port"
> > +
> > +struct vuport {
> > +	struct device *dev;
> > +	struct gpio_desc *gpio_vbus_en;
> > +	struct gpio_desc *gpio_usb_id;
> > +	struct gpio_desc *gpio_usb_mux;
> > +
> > +	struct extcon_dev edev;
> > +};
> > +
> > +static const char *const vuport_extcon_cable[] = {
> > +	[0] = "USB-Host",
> > +	NULL,
> > +};
> > +
> > +/*
> > + * If id == 1, USB port should be set to peripheral
> > + * if id == 0, USB port should be set to host
> > + *
> > + * Peripheral: set USB mux to peripheral and disable VBUS
> > + * Host: set USB mux to host and enable VBUS
> > + */
> > +static void vuport_set_port(struct vuport *vup, int id)
> > +{
> > +	int mux_val = id;
> > +	int vbus_val = !id;
> > +
> > +	if (!IS_ERR(vup->gpio_usb_mux))
> > +		gpiod_direction_output(vup->gpio_usb_mux, mux_val);
> > +
> > +	if (!IS_ERR(vup->gpio_vbus_en))
> > +		gpiod_direction_output(vup->gpio_vbus_en, vbus_val);
> 
> not all SoCs will allow for direction to be set all the time. This can
> be glitchy in some cases. What you want here is to set direction during
> probe and just set value here.

It makes sense, I'll change it.

> 
> > +static void vuport_do_usb_id(struct vuport *vup)
> > +{
> > +	int id = gpiod_get_value(vup->gpio_usb_id);
> > +
> > +	dev_info(vup->dev, "USB PORT ID: %s\n", id ? "PERIPHERAL" : "HOST");
> 
> info ? sounds like debug to me.

Ack.

> 
> > +
> > +	/*
> > +	 * id == 1: PERIPHERAL
> > +	 * id == 0: HOST
> > +	 */
> > +	vuport_set_port(vup, id);
> > +
> > +	/*
> > +	 * id == 0: HOST connected
> > +	 * id == 1: Host disconnected
> > +	 */
> > +	extcon_set_cable_state(&vup->edev, "USB-Host", !id);
> > +}
> > +
> > +static irqreturn_t vuport_thread_isr(int irq, void *priv)
> > +{
> 
> this is unrelated to $subject, but I always consider if we should have a
> generic way to figure out if the interrupt was for rising or falling
> edge, if we knew that, we could avoid reading the GPIO value altogether
> ;-)

Yeah, that would be nice.

> 
> > +	struct vuport *vup = priv;
> > +
> > +	vuport_do_usb_id(vup);
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +static irqreturn_t vuport_isr(int irq, void *priv)
> > +{
> > +	return IRQ_WAKE_THREAD;
> > +}
> 
> you don't need this. Set the top half handler to NULL and pass
> IRQF_ONESHOT (which you shoudl already have set anyway).

Ack.

> 
> > +#define VUPORT_GPIO_USB_ID	0
> > +#define VUPORT_GPIO_VBUS_EN	1
> > +#define VUPORT_GPIO_USB_MUX	2
> > +static int vuport_probe(struct platform_device *pdev)
> > +{
> > +	struct device *dev = &pdev->dev;
> > +	struct vuport *vup;
> > +	int ret;
> > +
> > +	vup = devm_kzalloc(dev, sizeof(*vup), GFP_KERNEL);
> > +	if (!vup) {
> > +		dev_err(dev, "cannot allocate private data\n");
> > +		return -ENOMEM;
> > +	}
> > +	vup->dev = dev;
> > +
> > +	vup->gpio_usb_id = devm_gpiod_get_index(dev, "id", VUPORT_GPIO_USB_ID);
> > +	if (IS_ERR(vup->gpio_usb_id)) {
> > +		dev_err(dev, "cannot request USB ID GPIO: ret = %ld\n",
> > +			PTR_ERR(vup->gpio_usb_id));
> > +		return PTR_ERR(vup->gpio_usb_id);
> > +	}
> > +
> > +	ret = gpiod_direction_input(vup->gpio_usb_id);
> > +	if (ret < 0) {
> > +		dev_err(dev, "cannot set input direction to USB ID GPIO: ret = %d\n",
> > +			ret);
> > +		return ret;
> > +	}
> > +
> > +	vup->gpio_vbus_en = devm_gpiod_get_index(dev, "vbus en",
> > +						 VUPORT_GPIO_VBUS_EN);
> > +	if (IS_ERR(vup->gpio_vbus_en))
> > +		dev_info(dev, "cannot request VBUS EN GPIO, skipping it.\n");
> > +
> > +	vup->gpio_usb_mux = devm_gpiod_get_index(dev, "usb mux",
> > +						 VUPORT_GPIO_USB_MUX);
> > +	if (IS_ERR(vup->gpio_usb_mux))
> > +		dev_info(dev, "cannot request USB USB MUX, skipping it.\n");
> > +
> > +	/* register extcon device */
> > +	vup->edev.dev.parent = dev;
> > +	vup->edev.supported_cable = vuport_extcon_cable;
> 
> IIRC, edev should be allocated from now on. Have a look at
> devm_extcon_dev_allocate() and devm_extcon_dev_register().

Thanks, I'll check.

> 
> > +	ret = extcon_dev_register(&vup->edev);
> > +	if (ret < 0) {
> > +		dev_err(dev, "failed to register extcon device: ret = %d\n",
> > +			ret);
> > +		return ret;
> > +	}
> > +
> > +	ret = devm_request_threaded_irq(dev, gpiod_to_irq(vup->gpio_usb_id),
> > +					vuport_isr, vuport_thread_isr,
> > +					IRQF_SHARED | IRQF_TRIGGER_RISING |
> > +					IRQF_TRIGGER_FALLING,
> > +					dev_name(dev), vup);
> > +	if (ret < 0) {
> > +		dev_err(dev, "cannot request IRQ for USB ID GPIO: ret = %d\n",
> > +			ret);
> > +		goto irq_err;
> > +	}
> > +	vuport_do_usb_id(vup);
> > +
> > +	platform_set_drvdata(pdev, vup);
> > +
> > +	dev_info(dev, "driver successfully probed\n");
> 
> this will just make boot noisier, make it dev_dbg() ? Or even
> dev_vdbg() ?

dev_dgb() perhaps.

> 
> > +MODULE_LICENSE("GPL");
> 
> you header on the top of this C file states gpl 2 only, but this says
> GPL 2+.

I'll fix it.

Thanks,

David

> 
> cheers
> 
> -- 
> balbi

Re: [RFC/PATCH] extcon: otg_gpio: add driver for USB OTG port controlled by GPIO(s)

[David Cohen]

On Wed, Dec 24, 2014 at 11:08:52AM +0800, Peter Chen wrote:
> On Tue, Dec 23, 2014 at 11:40:23AM -0800, David Cohen wrote:
> > Hi Peter,
> > 
> > Thanks for the review.
> > 
> > On Tue, Dec 23, 2014 at 09:25:18AM +0800, Peter Chen wrote:
> > > On Mon, Dec 22, 2014 at 02:43:37PM -0800, David Cohen wrote:
> > > > Some platforms have an USB OTG port fully (or partially) controlled by
> > > > GPIOs:
> > > > 
> > > > (1) USB ID is connected directly to GPIO
> > > > 
> > > > Optionally:
> > > > (2) VBUS is enabled by a GPIO (when ID is grounded)
> > > > (3) Platform has 2 USB controllers connected to same port: one for
> > > >     device and one for host role. D+/- are switched between phys
> > > >     by GPIO.
> > > 
> > > Would you explain how it works? Choosing controller runtime?
> > 
> > Both controllers are (indirectly) connected to the same micro B port.
> > The D+/- goes from the port to a switch operated by a GPIO. From the
> > switch, D+/- may go to Host controller's phy or Device controller's phy.
> > Depends on the GPIO level.
> > 
> 
> Get it, why the design like that? If your controller supports both
> roles, the software can do role switch by ID pin (through gpio in your
> case).

It's really 2 controllers: 1 for each role. And the ID pin from port is
connected to GPIO only.

> 
> > > 
> > > > 
> > > > As per initial version, this driver has the duty to control whether
> > > > USB-Host cable is plugged in or not:
> > > 
> > > You mean Micro-AB cable, right?
> > 
> > > > +
> > > > +	vup->gpio_usb_mux = devm_gpiod_get_index(dev, "usb mux",
> > > > +						 VUPORT_GPIO_USB_MUX);
> > > > +	if (IS_ERR(vup->gpio_usb_mux))
> > > > +		dev_info(dev, "cannot request USB USB MUX, skipping it.\n");
> > > 
> > > Using dev_err
> > 
> > That's not really an error, although the IS_ERR() suggests otherwise.
> > The driver works well if a board doesn't need this mux (I'll add a
> > comment to state that clear). IMHO either keep dev_info or use dev_dgb
> > instead?
> > 
> 
> If that, dev_dbg may be suitable.

Ack.

Br, David

> 
> 
> -- 
> 
> Best Regards,
> Peter Chen

Re: [RFC/PATCH] extcon: otg_gpio: add driver for USB OTG port controlled by GPIO(s)

[Felipe Balbi]

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

Hi,

On Wed, Dec 24, 2014 at 02:43:27PM -0800, David Cohen wrote:
> Hi Felipe,
> 
> Thanks replying.
> 
> On Tue, Dec 23, 2014 at 06:29:04PM -0600, Felipe Balbi wrote:
> > Hi,
> > 
> > On Mon, Dec 22, 2014 at 02:43:37PM -0800, David Cohen wrote:
> > > Some platforms have an USB OTG port fully (or partially) controlled by
> > > GPIOs:
> > > 
> > > (1) USB ID is connected directly to GPIO
> > > 
> > > Optionally:
> > > (2) VBUS is enabled by a GPIO (when ID is grounded)
> > 
> > ok, so a fixed regulator with a GPIO enable pin.
> 
> Pretty much yes.

ok

> > > (3) Platform has 2 USB controllers connected to same port: one for
> > >     device and one for host role. D+/- are switched between phys
> > >     by GPIO.
> > 
> > so you have discrete mux with a GPIO select signal, like below ?
> > 
> > 
> >  .-------.----------------.  .--------.
> >  |       |                |  |        |      D+
> >  |       |                |  |        |<-------------.
> >  |       |                |  |        |              |
> >  |       |    USB Host    -->|    P   |              |
> >  |       |                |  |    H   |              |
> >  |       |                |  |    Y   |    D-        |
> >  |       '----------------'  |    0   |<--------.    |
> >  |                       |   |        |         |    |
> >  |                       |   '--------'      .--------.  D+
> >  |                       |                   |        |------>
> >  |       SOC        GPIO | ----------------->|        |
> >  |                       |                   |   MUX  |
> >  |                       |                   |        |------>
> >  |                       |   .--------.      '--------'  D-
> >  |       .----------------.  |        |   D-  |      |
> >  |       |                |  |    P   |<------'      |
> >  |       |                |  |    H   |              |
> >  |       |                |  |    Y   |              |
> >  |       |   USB Device   -->|    1   |              |
> >  |       |                |  |        |      D+      |
> >  |       |                |  |        |<-------------'
> >  |       |                |  |        |
> >  '-------'----------------'  '--------'
> 
> Nice ASCII pic :)

asciio ftw \o/

> Yes, that's the case.

alright

> > I have been on and off about writing a pinctrl-gpio.c driver which would
> > allow us to hide this detail from users. It shouldn't really matter
> > which modes are available behind the mux, but we should be able to tell
> > the mux to go into mode 0 or mode 1 by toggling its select signal. And
> > it shouldn't really matter that we have a GPIO pin. The problem is: I
> > don't really know if pinctrl would be able to handle discrete muxes.
> > 
> > Adding Linus W to ask. Linus, what do you think ? Should we have a
> > pinctrl-gpio.c for such cases ? In TI we too have quite a few boards
> > which different modes hidden behind discrete muxes.
> 
> An input from Linus would fine in this case.
> 
> > 
> > > As per initial version, this driver has the duty to control whether
> > > USB-Host cable is plugged in or not:
> > >  - If yes, OTG port is configured for host role
> > >  - If no, by standard, the OTG port is configured for device role
> > 
> > correct, this default-B is mandated by OTG spec anyway.
> > 
> > > Signed-off-by: David Cohen <david.a.cohen@linux.intel.com>
> > > ---
> > > 
> > > Hi,
> > > 
> > > Some Intel Bay Trail boards have an unusual way to handle the USB OTG port:
> > >  - The USB ID pin is connected directly to GPIO on SoC
> > >  - When in host role, VBUS is provided by enabling a GPIO
> > >  - Device and host roles are supported by 2 independent controllers with D+/-
> > >    pins from port switched between different phys according a GPIO level.
> > > 
> > > The ACPI table describes this USB port as a (virtual) device with all the
> > > necessary GPIOs. This driver implements support to this virtual device as an
> > > extcon class driver. All drivers that depend on the USB OTG port state (USB phy,
> > > PMIC, charge detection) will listen to extcon events.
> > 
> > Right I think you're almost there, but I still think that this needs to
> > be a bit broken down. First, we need some generic DRD library under
> > drivers/usb/common, and that should be the one listening to extcon cable
> > events. But your extcon driver should be doing only that: checking which
> > cable was attached, it shouldn't be doing the switch by itself. That
> > should be part of the DRD library.
> > 
> > That DRD library would also be the one enabling the (optional) VBUS
> > regulator.
> > 
> > George Cherian tried to implement a generic DRD library but I think
> > there are still too many changes happening on usbcore and udc-core. Most
> > of the pieces are already there (usb_del_hcd() and usb_del_gadget_udc()
> > know how to properly unload an hcd/udc), but there are details missing,
> > no doubt.
> > 
> > If we can find a way to broadcast (probably not the best term, but
> > whatever) a "Hey ID pin was just grounded" message, we can get things
> > working.
> > 
> > That message, btw, shouldn't really depend on extcon-gpio alone because
> > other platforms might use non-gpio methods to verify ID pin level. In
> > any case, we need to have generic ID_PIN_LOW and ID_PIN_HIGH messages
> > that a generic DRD library can listen to and load/unload the correct
> > drivers by means of usb_{add,del}_{hcd,gadget_udc}().
> 
> IMHO extcon is the correct way to broadcast it, as long as we define a
> standard for the cable names. E.g. DRD library could listen to
> "USB-HOST" cable state. Then it doesn't matter how ID pin is grounded,
> it just matters that whoever is controlling it broadcast via this cable.

right, the likelyhood that someone would not use a GPIO is also quite
minimal and for such cases, the controller would likely switch roles
automatically (like with MUSB).

> > With that in mind, I think you can use extcon-gpio.c for your purposes
> > if the other pieces can be fullfilled by regulator and pinctrl.
> 
> In my case we have all gpios listed in a single ACPI device. In order to
> be backwards compatible with products already on market, we'd need
> something like a single mfd to register platform devices for this
> smaller pieces (extcon gpio, possible pintrl gpio, maybe vbus as regulator??).

correct.

> > > +	ret = devm_request_threaded_irq(dev, gpiod_to_irq(vup->gpio_usb_id),
> > > +					vuport_isr, vuport_thread_isr,
> > > +					IRQF_SHARED | IRQF_TRIGGER_RISING |
> > > +					IRQF_TRIGGER_FALLING,
> > > +					dev_name(dev), vup);
> > > +	if (ret < 0) {
> > > +		dev_err(dev, "cannot request IRQ for USB ID GPIO: ret = %d\n",
> > > +			ret);
> > > +		goto irq_err;
> > > +	}
> > > +	vuport_do_usb_id(vup);
> > > +
> > > +	platform_set_drvdata(pdev, vup);
> > > +
> > > +	dev_info(dev, "driver successfully probed\n");
> > 
> > this will just make boot noisier, make it dev_dbg() ? Or even
> > dev_vdbg() ?
> 
> dev_dgb() perhaps.

sure, why not :-)

-- 
balbi

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

Re: [RFC/PATCH] extcon: otg_gpio: add driver for USB OTG port controlled by GPIO(s)

[Linus Walleij]

On Mon, Dec 22, 2014 at 11:43 PM, David Cohen
<david.a.cohen@linux.intel.com> wrote:

> Some platforms have an USB OTG port fully (or partially) controlled by
> GPIOs:
>
> (1) USB ID is connected directly to GPIO
>
> Optionally:
> (2) VBUS is enabled by a GPIO (when ID is grounded)
> (3) Platform has 2 USB controllers connected to same port: one for
>     device and one for host role. D+/- are switched between phys
>     by GPIO.
>
> As per initial version, this driver has the duty to control whether
> USB-Host cable is plugged in or not:
>  - If yes, OTG port is configured for host role
>  - If no, by standard, the OTG port is configured for device role
>
> Signed-off-by: David Cohen <david.a.cohen@linux.intel.com>

Pretty interesting! I don't understand the USB stuff so commenting
from a GPIO side of things only.

> +config EXTCON_OTG_GPIO
> +       tristate "VIRTUAL USB OTG PORT support"
> +       depends on GPIOLIB

Isn't it dependent on ACPI? This was mentioned in the commit message.

> +/*
> + * Virtual USB OTG Port driver controlled by gpios
> + *
> + * Copyright (c) 2014, Intel Corporation.
> + * Author: David Cohen <david.a.cohen@linux.intel.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * 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/acpi.h>
> +#include <linux/extcon.h>
> +#include <linux/gpio.h>

You should include <linux/gpio/consumer.h>

And nothing else. (I think it'll just work.)

> +static int __init vuport_init(void)
> +{
> +       return platform_driver_register(&vuport_driver);
> +}
> +subsys_initcall(vuport_init);

Usually we try to avoid this kind of early initcalls.
Doesn't deferred probe work as intended?

Yours,
Linus Walleij

Re: [RFC/PATCH] extcon: otg_gpio: add driver for USB OTG port controlled by GPIO(s)

[David Cohen]

Hi Felipe and Linus,

On Thu, Dec 25, 2014 at 10:49:29PM -0600, Felipe Balbi wrote:
> Hi,
> 
> On Wed, Dec 24, 2014 at 02:43:27PM -0800, David Cohen wrote:
> > Hi Felipe,
> > 
> > Thanks replying.
> > 
> > On Tue, Dec 23, 2014 at 06:29:04PM -0600, Felipe Balbi wrote:
> > > Hi,
> > > 
> > > On Mon, Dec 22, 2014 at 02:43:37PM -0800, David Cohen wrote:
> > > > Some platforms have an USB OTG port fully (or partially) controlled by
> > > > GPIOs:
> > > > 
> > > > (1) USB ID is connected directly to GPIO
> > > > 
> > > > Optionally:
> > > > (2) VBUS is enabled by a GPIO (when ID is grounded)
> > > 
> > > ok, so a fixed regulator with a GPIO enable pin.
> > 
> > Pretty much yes.
> 
> ok
> 
> > > > (3) Platform has 2 USB controllers connected to same port: one for
> > > >     device and one for host role. D+/- are switched between phys
> > > >     by GPIO.
> > > 
> > > so you have discrete mux with a GPIO select signal, like below ?
> > > 
> > > 
> > >  .-------.----------------.  .--------.
> > >  |       |                |  |        |      D+
> > >  |       |                |  |        |<-------------.
> > >  |       |                |  |        |              |
> > >  |       |    USB Host    -->|    P   |              |
> > >  |       |                |  |    H   |              |
> > >  |       |                |  |    Y   |    D-        |
> > >  |       '----------------'  |    0   |<--------.    |
> > >  |                       |   |        |         |    |
> > >  |                       |   '--------'      .--------.  D+
> > >  |                       |                   |        |------>
> > >  |       SOC        GPIO | ----------------->|        |
> > >  |                       |                   |   MUX  |
> > >  |                       |                   |        |------>
> > >  |                       |   .--------.      '--------'  D-
> > >  |       .----------------.  |        |   D-  |      |
> > >  |       |                |  |    P   |<------'      |
> > >  |       |                |  |    H   |              |
> > >  |       |                |  |    Y   |              |
> > >  |       |   USB Device   -->|    1   |              |
> > >  |       |                |  |        |      D+      |
> > >  |       |                |  |        |<-------------'
> > >  |       |                |  |        |
> > >  '-------'----------------'  '--------'
> > 
> > Nice ASCII pic :)
> 
> asciio ftw \o/
> 
> > Yes, that's the case.
> 
> alright
> 
> > > I have been on and off about writing a pinctrl-gpio.c driver which would
> > > allow us to hide this detail from users. It shouldn't really matter
> > > which modes are available behind the mux, but we should be able to tell
> > > the mux to go into mode 0 or mode 1 by toggling its select signal. And
> > > it shouldn't really matter that we have a GPIO pin. The problem is: I
> > > don't really know if pinctrl would be able to handle discrete muxes.
> > > 
> > > Adding Linus W to ask. Linus, what do you think ? Should we have a
> > > pinctrl-gpio.c for such cases ? In TI we too have quite a few boards
> > > which different modes hidden behind discrete muxes.
> > 
> > An input from Linus would fine in this case.
> > 
> > > 
> > > > As per initial version, this driver has the duty to control whether
> > > > USB-Host cable is plugged in or not:
> > > >  - If yes, OTG port is configured for host role
> > > >  - If no, by standard, the OTG port is configured for device role
> > > 
> > > correct, this default-B is mandated by OTG spec anyway.
> > > 
> > > > Signed-off-by: David Cohen <david.a.cohen@linux.intel.com>
> > > > ---
> > > > 
> > > > Hi,
> > > > 
> > > > Some Intel Bay Trail boards have an unusual way to handle the USB OTG port:
> > > >  - The USB ID pin is connected directly to GPIO on SoC
> > > >  - When in host role, VBUS is provided by enabling a GPIO
> > > >  - Device and host roles are supported by 2 independent controllers with D+/-
> > > >    pins from port switched between different phys according a GPIO level.
> > > > 
> > > > The ACPI table describes this USB port as a (virtual) device with all the
> > > > necessary GPIOs. This driver implements support to this virtual device as an
> > > > extcon class driver. All drivers that depend on the USB OTG port state (USB phy,
> > > > PMIC, charge detection) will listen to extcon events.
> > > 
> > > Right I think you're almost there, but I still think that this needs to
> > > be a bit broken down. First, we need some generic DRD library under
> > > drivers/usb/common, and that should be the one listening to extcon cable
> > > events. But your extcon driver should be doing only that: checking which
> > > cable was attached, it shouldn't be doing the switch by itself. That
> > > should be part of the DRD library.
> > > 
> > > That DRD library would also be the one enabling the (optional) VBUS
> > > regulator.
> > > 
> > > George Cherian tried to implement a generic DRD library but I think
> > > there are still too many changes happening on usbcore and udc-core. Most
> > > of the pieces are already there (usb_del_hcd() and usb_del_gadget_udc()
> > > know how to properly unload an hcd/udc), but there are details missing,
> > > no doubt.
> > > 
> > > If we can find a way to broadcast (probably not the best term, but
> > > whatever) a "Hey ID pin was just grounded" message, we can get things
> > > working.
> > > 
> > > That message, btw, shouldn't really depend on extcon-gpio alone because
> > > other platforms might use non-gpio methods to verify ID pin level. In
> > > any case, we need to have generic ID_PIN_LOW and ID_PIN_HIGH messages
> > > that a generic DRD library can listen to and load/unload the correct
> > > drivers by means of usb_{add,del}_{hcd,gadget_udc}().
> > 
> > IMHO extcon is the correct way to broadcast it, as long as we define a
> > standard for the cable names. E.g. DRD library could listen to
> > "USB-HOST" cable state. Then it doesn't matter how ID pin is grounded,
> > it just matters that whoever is controlling it broadcast via this cable.
> 
> right, the likelyhood that someone would not use a GPIO is also quite
> minimal and for such cases, the controller would likely switch roles
> automatically (like with MUSB).
> 
> > > With that in mind, I think you can use extcon-gpio.c for your purposes
> > > if the other pieces can be fullfilled by regulator and pinctrl.
> > 
> > In my case we have all gpios listed in a single ACPI device. In order to
> > be backwards compatible with products already on market, we'd need
> > something like a single mfd to register platform devices for this
> > smaller pieces (extcon gpio, possible pintrl gpio, maybe vbus as regulator??).
> 
> correct.

Getting back to this case :)
Guess I need to get back my words.

extcon-gpio.c cannot work out-of-the-box with my case. There is no clean
way to get the GPIO given to this device via ACPI and refer it to another
device (i.e. extcon-gpio).

Here's my scenario:
This platform has only one ACPI device representing the USB port with 3
gpios controlling it. As GPIO consumer, there is no clean interface
where I could get a GPIO descriptor via ACPI without requesting it.
After request it, I cannot give it to extcon-gpio.c. Same would happen
for a possible pinctrl gpio and regulator controller by gpio.

So my choices:
1) request GPIO locally, give it to other drivers and somehow inform
them they should not request, but just to handle it (ugly)

2) implement a way to pass this GPIO resource to another device without
requesting locally

3) stick with this driver fully handling the GPIOs which control this
virtual "USB OTG port" device

Any thoughts?

Br, David

Re: [RFC/PATCH] extcon: otg_gpio: add driver for USB OTG port controlled by GPIO(s)

[David Cohen]

Hi Linus,

Thanks for reviewing.

On Thu, Jan 08, 2015 at 08:23:03PM +0100, Linus Walleij wrote:
> On Mon, Dec 22, 2014 at 11:43 PM, David Cohen
> <david.a.cohen@linux.intel.com> wrote:
> 
> > Some platforms have an USB OTG port fully (or partially) controlled by
> > GPIOs:
> >
> > (1) USB ID is connected directly to GPIO
> >
> > Optionally:
> > (2) VBUS is enabled by a GPIO (when ID is grounded)
> > (3) Platform has 2 USB controllers connected to same port: one for
> >     device and one for host role. D+/- are switched between phys
> >     by GPIO.
> >
> > As per initial version, this driver has the duty to control whether
> > USB-Host cable is plugged in or not:
> >  - If yes, OTG port is configured for host role
> >  - If no, by standard, the OTG port is configured for device role
> >
> > Signed-off-by: David Cohen <david.a.cohen@linux.intel.com>
> 
> Pretty interesting! I don't understand the USB stuff so commenting
> from a GPIO side of things only.
> 
> > +config EXTCON_OTG_GPIO
> > +       tristate "VIRTUAL USB OTG PORT support"
> > +       depends on GPIOLIB
> 
> Isn't it dependent on ACPI? This was mentioned in the commit message.

Yep, I'll add it :)

> 
> > +/*
> > + * Virtual USB OTG Port driver controlled by gpios
> > + *
> > + * Copyright (c) 2014, Intel Corporation.
> > + * Author: David Cohen <david.a.cohen@linux.intel.com>
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + *
> > + * 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/acpi.h>
> > +#include <linux/extcon.h>
> > +#include <linux/gpio.h>
> 
> You should include <linux/gpio/consumer.h>
> 
> And nothing else. (I think it'll just work.)

It should work. I'll fix it.

> 
> > +static int __init vuport_init(void)
> > +{
> > +       return platform_driver_register(&vuport_driver);
> > +}
> > +subsys_initcall(vuport_init);
> 
> Usually we try to avoid this kind of early initcalls.
> Doesn't deferred probe work as intended?

Yeah, deferred probe is a better thing to try here.

Br, David

> 
> Yours,
> Linus Walleij

Re: [RFC/PATCH] extcon: otg_gpio: add driver for USB OTG port controlled by GPIO(s)

[Felipe Balbi]

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

Hi,

On Tue, Feb 17, 2015 at 11:18:44AM -0800, David Cohen wrote:
> > > > > (3) Platform has 2 USB controllers connected to same port: one for
> > > > >     device and one for host role. D+/- are switched between phys
> > > > >     by GPIO.
> > > > 
> > > > so you have discrete mux with a GPIO select signal, like below ?
> > > > 
> > > > 
> > > >  .-------.----------------.  .--------.
> > > >  |       |                |  |        |      D+
> > > >  |       |                |  |        |<-------------.
> > > >  |       |                |  |        |              |
> > > >  |       |    USB Host    -->|    P   |              |
> > > >  |       |                |  |    H   |              |
> > > >  |       |                |  |    Y   |    D-        |
> > > >  |       '----------------'  |    0   |<--------.    |
> > > >  |                       |   |        |         |    |
> > > >  |                       |   '--------'      .--------.  D+
> > > >  |                       |                   |        |------>
> > > >  |       SOC        GPIO | ----------------->|        |
> > > >  |                       |                   |   MUX  |
> > > >  |                       |                   |        |------>
> > > >  |                       |   .--------.      '--------'  D-
> > > >  |       .----------------.  |        |   D-  |      |
> > > >  |       |                |  |    P   |<------'      |
> > > >  |       |                |  |    H   |              |
> > > >  |       |                |  |    Y   |              |
> > > >  |       |   USB Device   -->|    1   |              |
> > > >  |       |                |  |        |      D+      |
> > > >  |       |                |  |        |<-------------'
> > > >  |       |                |  |        |
> > > >  '-------'----------------'  '--------'
> > > 
> > > Nice ASCII pic :)
> > 
> > asciio ftw \o/
> > 
> > > Yes, that's the case.
> > 
> > alright
> > 
> > > > I have been on and off about writing a pinctrl-gpio.c driver which would
> > > > allow us to hide this detail from users. It shouldn't really matter
> > > > which modes are available behind the mux, but we should be able to tell
> > > > the mux to go into mode 0 or mode 1 by toggling its select signal. And
> > > > it shouldn't really matter that we have a GPIO pin. The problem is: I
> > > > don't really know if pinctrl would be able to handle discrete muxes.
> > > > 
> > > > Adding Linus W to ask. Linus, what do you think ? Should we have a
> > > > pinctrl-gpio.c for such cases ? In TI we too have quite a few boards
> > > > which different modes hidden behind discrete muxes.
> > > 
> > > An input from Linus would fine in this case.
> > > 
> > > > 
> > > > > As per initial version, this driver has the duty to control whether
> > > > > USB-Host cable is plugged in or not:
> > > > >  - If yes, OTG port is configured for host role
> > > > >  - If no, by standard, the OTG port is configured for device role
> > > > 
> > > > correct, this default-B is mandated by OTG spec anyway.
> > > > 
> > > > > Signed-off-by: David Cohen <david.a.cohen@linux.intel.com>
> > > > > ---
> > > > > 
> > > > > Hi,
> > > > > 
> > > > > Some Intel Bay Trail boards have an unusual way to handle the USB OTG port:
> > > > >  - The USB ID pin is connected directly to GPIO on SoC
> > > > >  - When in host role, VBUS is provided by enabling a GPIO
> > > > >  - Device and host roles are supported by 2 independent controllers with D+/-
> > > > >    pins from port switched between different phys according a GPIO level.
> > > > > 
> > > > > The ACPI table describes this USB port as a (virtual) device with all the
> > > > > necessary GPIOs. This driver implements support to this virtual device as an
> > > > > extcon class driver. All drivers that depend on the USB OTG port state (USB phy,
> > > > > PMIC, charge detection) will listen to extcon events.
> > > > 
> > > > Right I think you're almost there, but I still think that this needs to
> > > > be a bit broken down. First, we need some generic DRD library under
> > > > drivers/usb/common, and that should be the one listening to extcon cable
> > > > events. But your extcon driver should be doing only that: checking which
> > > > cable was attached, it shouldn't be doing the switch by itself. That
> > > > should be part of the DRD library.
> > > > 
> > > > That DRD library would also be the one enabling the (optional) VBUS
> > > > regulator.
> > > > 
> > > > George Cherian tried to implement a generic DRD library but I think
> > > > there are still too many changes happening on usbcore and udc-core. Most
> > > > of the pieces are already there (usb_del_hcd() and usb_del_gadget_udc()
> > > > know how to properly unload an hcd/udc), but there are details missing,
> > > > no doubt.
> > > > 
> > > > If we can find a way to broadcast (probably not the best term, but
> > > > whatever) a "Hey ID pin was just grounded" message, we can get things
> > > > working.
> > > > 
> > > > That message, btw, shouldn't really depend on extcon-gpio alone because
> > > > other platforms might use non-gpio methods to verify ID pin level. In
> > > > any case, we need to have generic ID_PIN_LOW and ID_PIN_HIGH messages
> > > > that a generic DRD library can listen to and load/unload the correct
> > > > drivers by means of usb_{add,del}_{hcd,gadget_udc}().
> > > 
> > > IMHO extcon is the correct way to broadcast it, as long as we define a
> > > standard for the cable names. E.g. DRD library could listen to
> > > "USB-HOST" cable state. Then it doesn't matter how ID pin is grounded,
> > > it just matters that whoever is controlling it broadcast via this cable.
> > 
> > right, the likelyhood that someone would not use a GPIO is also quite
> > minimal and for such cases, the controller would likely switch roles
> > automatically (like with MUSB).
> > 
> > > > With that in mind, I think you can use extcon-gpio.c for your purposes
> > > > if the other pieces can be fullfilled by regulator and pinctrl.
> > > 
> > > In my case we have all gpios listed in a single ACPI device. In order to
> > > be backwards compatible with products already on market, we'd need
> > > something like a single mfd to register platform devices for this
> > > smaller pieces (extcon gpio, possible pintrl gpio, maybe vbus as regulator??).
> > 
> > correct.
> 
> Getting back to this case :)
> Guess I need to get back my words.
> 
> extcon-gpio.c cannot work out-of-the-box with my case. There is no clean
> way to get the GPIO given to this device via ACPI and refer it to another
> device (i.e. extcon-gpio).

add what's missing ?

> Here's my scenario:
> This platform has only one ACPI device representing the USB port with 3
> gpios controlling it. As GPIO consumer, there is no clean interface
> where I could get a GPIO descriptor via ACPI without requesting it.
> After request it, I cannot give it to extcon-gpio.c. Same would happen
> for a possible pinctrl gpio and regulator controller by gpio.
> 
> So my choices:
> 1) request GPIO locally, give it to other drivers and somehow inform
> them they should not request, but just to handle it (ugly)
> 
> 2) implement a way to pass this GPIO resource to another device without
> requesting locally
> 
> 3) stick with this driver fully handling the GPIOs which control this
> virtual "USB OTG port" device

4) grab gpio via ACPI, gpio_free() it, pass to this driver. Would that
work ?

-- 
balbi

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

Re: [RFC/PATCH] extcon: otg_gpio: add driver for USB OTG port controlled by GPIO(s)

[David Cohen]

Hi,

Adding Mika.

On Tue, Feb 17, 2015 at 01:25:00PM -0600, Felipe Balbi wrote:
> Hi,
> 
> On Tue, Feb 17, 2015 at 11:18:44AM -0800, David Cohen wrote:
> > > > > > (3) Platform has 2 USB controllers connected to same port: one for
> > > > > >     device and one for host role. D+/- are switched between phys
> > > > > >     by GPIO.
> > > > > 
> > > > > so you have discrete mux with a GPIO select signal, like below ?
> > > > > 
> > > > > 
> > > > >  .-------.----------------.  .--------.
> > > > >  |       |                |  |        |      D+
> > > > >  |       |                |  |        |<-------------.
> > > > >  |       |                |  |        |              |
> > > > >  |       |    USB Host    -->|    P   |              |
> > > > >  |       |                |  |    H   |              |
> > > > >  |       |                |  |    Y   |    D-        |
> > > > >  |       '----------------'  |    0   |<--------.    |
> > > > >  |                       |   |        |         |    |
> > > > >  |                       |   '--------'      .--------.  D+
> > > > >  |                       |                   |        |------>
> > > > >  |       SOC        GPIO | ----------------->|        |
> > > > >  |                       |                   |   MUX  |
> > > > >  |                       |                   |        |------>
> > > > >  |                       |   .--------.      '--------'  D-
> > > > >  |       .----------------.  |        |   D-  |      |
> > > > >  |       |                |  |    P   |<------'      |
> > > > >  |       |                |  |    H   |              |
> > > > >  |       |                |  |    Y   |              |
> > > > >  |       |   USB Device   -->|    1   |              |
> > > > >  |       |                |  |        |      D+      |
> > > > >  |       |                |  |        |<-------------'
> > > > >  |       |                |  |        |
> > > > >  '-------'----------------'  '--------'
> > > > 
> > > > Nice ASCII pic :)
> > > 
> > > asciio ftw \o/
> > > 
> > > > Yes, that's the case.
> > > 
> > > alright
> > > 
> > > > > I have been on and off about writing a pinctrl-gpio.c driver which would
> > > > > allow us to hide this detail from users. It shouldn't really matter
> > > > > which modes are available behind the mux, but we should be able to tell
> > > > > the mux to go into mode 0 or mode 1 by toggling its select signal. And
> > > > > it shouldn't really matter that we have a GPIO pin. The problem is: I
> > > > > don't really know if pinctrl would be able to handle discrete muxes.
> > > > > 
> > > > > Adding Linus W to ask. Linus, what do you think ? Should we have a
> > > > > pinctrl-gpio.c for such cases ? In TI we too have quite a few boards
> > > > > which different modes hidden behind discrete muxes.
> > > > 
> > > > An input from Linus would fine in this case.
> > > > 
> > > > > 
> > > > > > As per initial version, this driver has the duty to control whether
> > > > > > USB-Host cable is plugged in or not:
> > > > > >  - If yes, OTG port is configured for host role
> > > > > >  - If no, by standard, the OTG port is configured for device role
> > > > > 
> > > > > correct, this default-B is mandated by OTG spec anyway.
> > > > > 
> > > > > > Signed-off-by: David Cohen <david.a.cohen@linux.intel.com>
> > > > > > ---
> > > > > > 
> > > > > > Hi,
> > > > > > 
> > > > > > Some Intel Bay Trail boards have an unusual way to handle the USB OTG port:
> > > > > >  - The USB ID pin is connected directly to GPIO on SoC
> > > > > >  - When in host role, VBUS is provided by enabling a GPIO
> > > > > >  - Device and host roles are supported by 2 independent controllers with D+/-
> > > > > >    pins from port switched between different phys according a GPIO level.
> > > > > > 
> > > > > > The ACPI table describes this USB port as a (virtual) device with all the
> > > > > > necessary GPIOs. This driver implements support to this virtual device as an
> > > > > > extcon class driver. All drivers that depend on the USB OTG port state (USB phy,
> > > > > > PMIC, charge detection) will listen to extcon events.
> > > > > 
> > > > > Right I think you're almost there, but I still think that this needs to
> > > > > be a bit broken down. First, we need some generic DRD library under
> > > > > drivers/usb/common, and that should be the one listening to extcon cable
> > > > > events. But your extcon driver should be doing only that: checking which
> > > > > cable was attached, it shouldn't be doing the switch by itself. That
> > > > > should be part of the DRD library.
> > > > > 
> > > > > That DRD library would also be the one enabling the (optional) VBUS
> > > > > regulator.
> > > > > 
> > > > > George Cherian tried to implement a generic DRD library but I think
> > > > > there are still too many changes happening on usbcore and udc-core. Most
> > > > > of the pieces are already there (usb_del_hcd() and usb_del_gadget_udc()
> > > > > know how to properly unload an hcd/udc), but there are details missing,
> > > > > no doubt.
> > > > > 
> > > > > If we can find a way to broadcast (probably not the best term, but
> > > > > whatever) a "Hey ID pin was just grounded" message, we can get things
> > > > > working.
> > > > > 
> > > > > That message, btw, shouldn't really depend on extcon-gpio alone because
> > > > > other platforms might use non-gpio methods to verify ID pin level. In
> > > > > any case, we need to have generic ID_PIN_LOW and ID_PIN_HIGH messages
> > > > > that a generic DRD library can listen to and load/unload the correct
> > > > > drivers by means of usb_{add,del}_{hcd,gadget_udc}().
> > > > 
> > > > IMHO extcon is the correct way to broadcast it, as long as we define a
> > > > standard for the cable names. E.g. DRD library could listen to
> > > > "USB-HOST" cable state. Then it doesn't matter how ID pin is grounded,
> > > > it just matters that whoever is controlling it broadcast via this cable.
> > > 
> > > right, the likelyhood that someone would not use a GPIO is also quite
> > > minimal and for such cases, the controller would likely switch roles
> > > automatically (like with MUSB).
> > > 
> > > > > With that in mind, I think you can use extcon-gpio.c for your purposes
> > > > > if the other pieces can be fullfilled by regulator and pinctrl.
> > > > 
> > > > In my case we have all gpios listed in a single ACPI device. In order to
> > > > be backwards compatible with products already on market, we'd need
> > > > something like a single mfd to register platform devices for this
> > > > smaller pieces (extcon gpio, possible pintrl gpio, maybe vbus as regulator??).
> > > 
> > > correct.
> > 
> > Getting back to this case :)
> > Guess I need to get back my words.
> > 
> > extcon-gpio.c cannot work out-of-the-box with my case. There is no clean
> > way to get the GPIO given to this device via ACPI and refer it to another
> > device (i.e. extcon-gpio).
> 
> add what's missing ?
> 
> > Here's my scenario:
> > This platform has only one ACPI device representing the USB port with 3
> > gpios controlling it. As GPIO consumer, there is no clean interface
> > where I could get a GPIO descriptor via ACPI without requesting it.
> > After request it, I cannot give it to extcon-gpio.c. Same would happen
> > for a possible pinctrl gpio and regulator controller by gpio.
> > 
> > So my choices:
> > 1) request GPIO locally, give it to other drivers and somehow inform
> > them they should not request, but just to handle it (ugly)
> > 
> > 2) implement a way to pass this GPIO resource to another device without
> > requesting locally
> > 
> > 3) stick with this driver fully handling the GPIOs which control this
> > virtual "USB OTG port" device
> 
> 4) grab gpio via ACPI, gpio_free() it, pass to this driver. Would that
> work ?

That works. But I feel it'd be same as 2. In case we don't want to
implement 2, the same reasons would apply to not implement 4.

Linus, Mika, would you have any thoughts about case 4?

Br, David

Re: [RFC/PATCH] extcon: otg_gpio: add driver for USB OTG port controlled by GPIO(s)

[Mika Westerberg]

On Tue, Feb 17, 2015 at 11:35:23AM -0800, David Cohen wrote:
> Hi,
> 
> Adding Mika.
> 
> On Tue, Feb 17, 2015 at 01:25:00PM -0600, Felipe Balbi wrote:
> > Hi,
> > 
> > On Tue, Feb 17, 2015 at 11:18:44AM -0800, David Cohen wrote:
> > > > > > > (3) Platform has 2 USB controllers connected to same port: one for
> > > > > > >     device and one for host role. D+/- are switched between phys
> > > > > > >     by GPIO.
> > > > > > 
> > > > > > so you have discrete mux with a GPIO select signal, like below ?
> > > > > > 
> > > > > > 
> > > > > >  .-------.----------------.  .--------.
> > > > > >  |       |                |  |        |      D+
> > > > > >  |       |                |  |        |<-------------.
> > > > > >  |       |                |  |        |              |
> > > > > >  |       |    USB Host    -->|    P   |              |
> > > > > >  |       |                |  |    H   |              |
> > > > > >  |       |                |  |    Y   |    D-        |
> > > > > >  |       '----------------'  |    0   |<--------.    |
> > > > > >  |                       |   |        |         |    |
> > > > > >  |                       |   '--------'      .--------.  D+
> > > > > >  |                       |                   |        |------>
> > > > > >  |       SOC        GPIO | ----------------->|        |
> > > > > >  |                       |                   |   MUX  |
> > > > > >  |                       |                   |        |------>
> > > > > >  |                       |   .--------.      '--------'  D-
> > > > > >  |       .----------------.  |        |   D-  |      |
> > > > > >  |       |                |  |    P   |<------'      |
> > > > > >  |       |                |  |    H   |              |
> > > > > >  |       |                |  |    Y   |              |
> > > > > >  |       |   USB Device   -->|    1   |              |
> > > > > >  |       |                |  |        |      D+      |
> > > > > >  |       |                |  |        |<-------------'
> > > > > >  |       |                |  |        |
> > > > > >  '-------'----------------'  '--------'
> > > > > 
> > > > > Nice ASCII pic :)
> > > > 
> > > > asciio ftw \o/
> > > > 
> > > > > Yes, that's the case.
> > > > 
> > > > alright
> > > > 
> > > > > > I have been on and off about writing a pinctrl-gpio.c driver which would
> > > > > > allow us to hide this detail from users. It shouldn't really matter
> > > > > > which modes are available behind the mux, but we should be able to tell
> > > > > > the mux to go into mode 0 or mode 1 by toggling its select signal. And
> > > > > > it shouldn't really matter that we have a GPIO pin. The problem is: I
> > > > > > don't really know if pinctrl would be able to handle discrete muxes.
> > > > > > 
> > > > > > Adding Linus W to ask. Linus, what do you think ? Should we have a
> > > > > > pinctrl-gpio.c for such cases ? In TI we too have quite a few boards
> > > > > > which different modes hidden behind discrete muxes.
> > > > > 
> > > > > An input from Linus would fine in this case.
> > > > > 
> > > > > > 
> > > > > > > As per initial version, this driver has the duty to control whether
> > > > > > > USB-Host cable is plugged in or not:
> > > > > > >  - If yes, OTG port is configured for host role
> > > > > > >  - If no, by standard, the OTG port is configured for device role
> > > > > > 
> > > > > > correct, this default-B is mandated by OTG spec anyway.
> > > > > > 
> > > > > > > Signed-off-by: David Cohen <david.a.cohen@linux.intel.com>
> > > > > > > ---
> > > > > > > 
> > > > > > > Hi,
> > > > > > > 
> > > > > > > Some Intel Bay Trail boards have an unusual way to handle the USB OTG port:
> > > > > > >  - The USB ID pin is connected directly to GPIO on SoC
> > > > > > >  - When in host role, VBUS is provided by enabling a GPIO
> > > > > > >  - Device and host roles are supported by 2 independent controllers with D+/-
> > > > > > >    pins from port switched between different phys according a GPIO level.
> > > > > > > 
> > > > > > > The ACPI table describes this USB port as a (virtual) device with all the
> > > > > > > necessary GPIOs. This driver implements support to this virtual device as an
> > > > > > > extcon class driver. All drivers that depend on the USB OTG port state (USB phy,
> > > > > > > PMIC, charge detection) will listen to extcon events.
> > > > > > 
> > > > > > Right I think you're almost there, but I still think that this needs to
> > > > > > be a bit broken down. First, we need some generic DRD library under
> > > > > > drivers/usb/common, and that should be the one listening to extcon cable
> > > > > > events. But your extcon driver should be doing only that: checking which
> > > > > > cable was attached, it shouldn't be doing the switch by itself. That
> > > > > > should be part of the DRD library.
> > > > > > 
> > > > > > That DRD library would also be the one enabling the (optional) VBUS
> > > > > > regulator.
> > > > > > 
> > > > > > George Cherian tried to implement a generic DRD library but I think
> > > > > > there are still too many changes happening on usbcore and udc-core. Most
> > > > > > of the pieces are already there (usb_del_hcd() and usb_del_gadget_udc()
> > > > > > know how to properly unload an hcd/udc), but there are details missing,
> > > > > > no doubt.
> > > > > > 
> > > > > > If we can find a way to broadcast (probably not the best term, but
> > > > > > whatever) a "Hey ID pin was just grounded" message, we can get things
> > > > > > working.
> > > > > > 
> > > > > > That message, btw, shouldn't really depend on extcon-gpio alone because
> > > > > > other platforms might use non-gpio methods to verify ID pin level. In
> > > > > > any case, we need to have generic ID_PIN_LOW and ID_PIN_HIGH messages
> > > > > > that a generic DRD library can listen to and load/unload the correct
> > > > > > drivers by means of usb_{add,del}_{hcd,gadget_udc}().
> > > > > 
> > > > > IMHO extcon is the correct way to broadcast it, as long as we define a
> > > > > standard for the cable names. E.g. DRD library could listen to
> > > > > "USB-HOST" cable state. Then it doesn't matter how ID pin is grounded,
> > > > > it just matters that whoever is controlling it broadcast via this cable.
> > > > 
> > > > right, the likelyhood that someone would not use a GPIO is also quite
> > > > minimal and for such cases, the controller would likely switch roles
> > > > automatically (like with MUSB).
> > > > 
> > > > > > With that in mind, I think you can use extcon-gpio.c for your purposes
> > > > > > if the other pieces can be fullfilled by regulator and pinctrl.
> > > > > 
> > > > > In my case we have all gpios listed in a single ACPI device. In order to
> > > > > be backwards compatible with products already on market, we'd need
> > > > > something like a single mfd to register platform devices for this
> > > > > smaller pieces (extcon gpio, possible pintrl gpio, maybe vbus as regulator??).
> > > > 
> > > > correct.
> > > 
> > > Getting back to this case :)
> > > Guess I need to get back my words.
> > > 
> > > extcon-gpio.c cannot work out-of-the-box with my case. There is no clean
> > > way to get the GPIO given to this device via ACPI and refer it to another
> > > device (i.e. extcon-gpio).
> > 
> > add what's missing ?
> > 
> > > Here's my scenario:
> > > This platform has only one ACPI device representing the USB port with 3
> > > gpios controlling it. As GPIO consumer, there is no clean interface
> > > where I could get a GPIO descriptor via ACPI without requesting it.
> > > After request it, I cannot give it to extcon-gpio.c. Same would happen
> > > for a possible pinctrl gpio and regulator controller by gpio.
> > > 
> > > So my choices:
> > > 1) request GPIO locally, give it to other drivers and somehow inform
> > > them they should not request, but just to handle it (ugly)
> > > 
> > > 2) implement a way to pass this GPIO resource to another device without
> > > requesting locally
> > > 
> > > 3) stick with this driver fully handling the GPIOs which control this
> > > virtual "USB OTG port" device
> > 
> > 4) grab gpio via ACPI, gpio_free() it, pass to this driver. Would that
> > work ?
> 
> That works. But I feel it'd be same as 2. In case we don't want to
> implement 2, the same reasons would apply to not implement 4.
> 
> Linus, Mika, would you have any thoughts about case 4?

Isn't this exactly what Heikki suggested in his GPIO forwarding series
here:

https://lkml.org/lkml/2014/12/18/82

Re: [RFC/PATCH] extcon: otg_gpio: add driver for USB OTG port controlled by GPIO(s)

[David Cohen]

Hi,

On Wed, Feb 18, 2015 at 12:17:06PM +0200, Mika Westerberg wrote:
> On Tue, Feb 17, 2015 at 11:35:23AM -0800, David Cohen wrote:
> > Hi,
> > 
> > Adding Mika.
> > 
> > On Tue, Feb 17, 2015 at 01:25:00PM -0600, Felipe Balbi wrote:
> > > Hi,
> > > 
> > > On Tue, Feb 17, 2015 at 11:18:44AM -0800, David Cohen wrote:
> > > > > > > > (3) Platform has 2 USB controllers connected to same port: one for
> > > > > > > >     device and one for host role. D+/- are switched between phys
> > > > > > > >     by GPIO.
> > > > > > > 
> > > > > > > so you have discrete mux with a GPIO select signal, like below ?
> > > > > > > 
> > > > > > > 
> > > > > > >  .-------.----------------.  .--------.
> > > > > > >  |       |                |  |        |      D+
> > > > > > >  |       |                |  |        |<-------------.
> > > > > > >  |       |                |  |        |              |
> > > > > > >  |       |    USB Host    -->|    P   |              |
> > > > > > >  |       |                |  |    H   |              |
> > > > > > >  |       |                |  |    Y   |    D-        |
> > > > > > >  |       '----------------'  |    0   |<--------.    |
> > > > > > >  |                       |   |        |         |    |
> > > > > > >  |                       |   '--------'      .--------.  D+
> > > > > > >  |                       |                   |        |------>
> > > > > > >  |       SOC        GPIO | ----------------->|        |
> > > > > > >  |                       |                   |   MUX  |
> > > > > > >  |                       |                   |        |------>
> > > > > > >  |                       |   .--------.      '--------'  D-
> > > > > > >  |       .----------------.  |        |   D-  |      |
> > > > > > >  |       |                |  |    P   |<------'      |
> > > > > > >  |       |                |  |    H   |              |
> > > > > > >  |       |                |  |    Y   |              |
> > > > > > >  |       |   USB Device   -->|    1   |              |
> > > > > > >  |       |                |  |        |      D+      |
> > > > > > >  |       |                |  |        |<-------------'
> > > > > > >  |       |                |  |        |
> > > > > > >  '-------'----------------'  '--------'
> > > > > > 
> > > > > > Nice ASCII pic :)
> > > > > 
> > > > > asciio ftw \o/
> > > > > 
> > > > > > Yes, that's the case.
> > > > > 
> > > > > alright
> > > > > 
> > > > > > > I have been on and off about writing a pinctrl-gpio.c driver which would
> > > > > > > allow us to hide this detail from users. It shouldn't really matter
> > > > > > > which modes are available behind the mux, but we should be able to tell
> > > > > > > the mux to go into mode 0 or mode 1 by toggling its select signal. And
> > > > > > > it shouldn't really matter that we have a GPIO pin. The problem is: I
> > > > > > > don't really know if pinctrl would be able to handle discrete muxes.
> > > > > > > 
> > > > > > > Adding Linus W to ask. Linus, what do you think ? Should we have a
> > > > > > > pinctrl-gpio.c for such cases ? In TI we too have quite a few boards
> > > > > > > which different modes hidden behind discrete muxes.
> > > > > > 
> > > > > > An input from Linus would fine in this case.
> > > > > > 
> > > > > > > 
> > > > > > > > As per initial version, this driver has the duty to control whether
> > > > > > > > USB-Host cable is plugged in or not:
> > > > > > > >  - If yes, OTG port is configured for host role
> > > > > > > >  - If no, by standard, the OTG port is configured for device role
> > > > > > > 
> > > > > > > correct, this default-B is mandated by OTG spec anyway.
> > > > > > > 
> > > > > > > > Signed-off-by: David Cohen <david.a.cohen@linux.intel.com>
> > > > > > > > ---
> > > > > > > > 
> > > > > > > > Hi,
> > > > > > > > 
> > > > > > > > Some Intel Bay Trail boards have an unusual way to handle the USB OTG port:
> > > > > > > >  - The USB ID pin is connected directly to GPIO on SoC
> > > > > > > >  - When in host role, VBUS is provided by enabling a GPIO
> > > > > > > >  - Device and host roles are supported by 2 independent controllers with D+/-
> > > > > > > >    pins from port switched between different phys according a GPIO level.
> > > > > > > > 
> > > > > > > > The ACPI table describes this USB port as a (virtual) device with all the
> > > > > > > > necessary GPIOs. This driver implements support to this virtual device as an
> > > > > > > > extcon class driver. All drivers that depend on the USB OTG port state (USB phy,
> > > > > > > > PMIC, charge detection) will listen to extcon events.
> > > > > > > 
> > > > > > > Right I think you're almost there, but I still think that this needs to
> > > > > > > be a bit broken down. First, we need some generic DRD library under
> > > > > > > drivers/usb/common, and that should be the one listening to extcon cable
> > > > > > > events. But your extcon driver should be doing only that: checking which
> > > > > > > cable was attached, it shouldn't be doing the switch by itself. That
> > > > > > > should be part of the DRD library.
> > > > > > > 
> > > > > > > That DRD library would also be the one enabling the (optional) VBUS
> > > > > > > regulator.
> > > > > > > 
> > > > > > > George Cherian tried to implement a generic DRD library but I think
> > > > > > > there are still too many changes happening on usbcore and udc-core. Most
> > > > > > > of the pieces are already there (usb_del_hcd() and usb_del_gadget_udc()
> > > > > > > know how to properly unload an hcd/udc), but there are details missing,
> > > > > > > no doubt.
> > > > > > > 
> > > > > > > If we can find a way to broadcast (probably not the best term, but
> > > > > > > whatever) a "Hey ID pin was just grounded" message, we can get things
> > > > > > > working.
> > > > > > > 
> > > > > > > That message, btw, shouldn't really depend on extcon-gpio alone because
> > > > > > > other platforms might use non-gpio methods to verify ID pin level. In
> > > > > > > any case, we need to have generic ID_PIN_LOW and ID_PIN_HIGH messages
> > > > > > > that a generic DRD library can listen to and load/unload the correct
> > > > > > > drivers by means of usb_{add,del}_{hcd,gadget_udc}().
> > > > > > 
> > > > > > IMHO extcon is the correct way to broadcast it, as long as we define a
> > > > > > standard for the cable names. E.g. DRD library could listen to
> > > > > > "USB-HOST" cable state. Then it doesn't matter how ID pin is grounded,
> > > > > > it just matters that whoever is controlling it broadcast via this cable.
> > > > > 
> > > > > right, the likelyhood that someone would not use a GPIO is also quite
> > > > > minimal and for such cases, the controller would likely switch roles
> > > > > automatically (like with MUSB).
> > > > > 
> > > > > > > With that in mind, I think you can use extcon-gpio.c for your purposes
> > > > > > > if the other pieces can be fullfilled by regulator and pinctrl.
> > > > > > 
> > > > > > In my case we have all gpios listed in a single ACPI device. In order to
> > > > > > be backwards compatible with products already on market, we'd need
> > > > > > something like a single mfd to register platform devices for this
> > > > > > smaller pieces (extcon gpio, possible pintrl gpio, maybe vbus as regulator??).
> > > > > 
> > > > > correct.
> > > > 
> > > > Getting back to this case :)
> > > > Guess I need to get back my words.
> > > > 
> > > > extcon-gpio.c cannot work out-of-the-box with my case. There is no clean
> > > > way to get the GPIO given to this device via ACPI and refer it to another
> > > > device (i.e. extcon-gpio).
> > > 
> > > add what's missing ?
> > > 
> > > > Here's my scenario:
> > > > This platform has only one ACPI device representing the USB port with 3
> > > > gpios controlling it. As GPIO consumer, there is no clean interface
> > > > where I could get a GPIO descriptor via ACPI without requesting it.
> > > > After request it, I cannot give it to extcon-gpio.c. Same would happen
> > > > for a possible pinctrl gpio and regulator controller by gpio.
> > > > 
> > > > So my choices:
> > > > 1) request GPIO locally, give it to other drivers and somehow inform
> > > > them they should not request, but just to handle it (ugly)
> > > > 
> > > > 2) implement a way to pass this GPIO resource to another device without
> > > > requesting locally
> > > > 
> > > > 3) stick with this driver fully handling the GPIOs which control this
> > > > virtual "USB OTG port" device
> > > 
> > > 4) grab gpio via ACPI, gpio_free() it, pass to this driver. Would that
> > > work ?
> > 
> > That works. But I feel it'd be same as 2. In case we don't want to
> > implement 2, the same reasons would apply to not implement 4.
> > 
> > Linus, Mika, would you have any thoughts about case 4?
> 
> Isn't this exactly what Heikki suggested in his GPIO forwarding series
> here:
> 
> https://lkml.org/lkml/2014/12/18/82

Yes, that's pretty much the case. It's a dead end for case 2 (and 4).
If we split this driver into smaller pieces, we'll have to force kernel
to hack GPIO ownership.

I get back to the original proposal in this case. I can send a second
version addressing all comments I received.

Comments? :)

BR, David

[PATCH v2] extcon: otg_gpio: add driver for USB OTG port controlled by GPIO(s)

[David Cohen]

Some Intel platforms have an USB OTG port fully (or partially)
controlled by GPIOs:

(1) USB ID is connected directly to a pulled up GPIO.

Optionally:
(2) VBUS is enabled/disabled by a GPIO
(3) Platform has 2 USB controllers connected to same port: one for
    device and one for host role. D+/- are switched between phys.
    according to this GPIO level.

This driver configures USB OTG port for device or host role according to
USB ID value.
 - If USB ID's GPIO level is low, OTG port is configured for host role
   by sourcing VBUS and switching D+/- to host phy.
 - If USB ID's GPIO level is high, by standard, the OTG port is
   configured for device role by not sourcing VBUS and switching D+/- to
   device controller.

Signed-off-by: David Cohen <david.a.cohen@linux.intel.com>
---

Hi,

Since splitting this driver into smaller pieces would result in ugly fixes WRT
ACPI, I'm resending the same approach.
This time I addressed all comments I got from RFC version.

As always, comments are welcome.

Br, David
---

 drivers/extcon/Kconfig           |   8 ++
 drivers/extcon/Makefile          |   1 +
 drivers/extcon/extcon-otg_gpio.c | 186 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 195 insertions(+)
 create mode 100644 drivers/extcon/extcon-otg_gpio.c

diff --git a/drivers/extcon/Kconfig b/drivers/extcon/Kconfig
index 6a1f7de6fa54..986ca7da9c1b 100644
--- a/drivers/extcon/Kconfig
+++ b/drivers/extcon/Kconfig
@@ -63,6 +63,14 @@ config EXTCON_MAX8997
 	  Maxim MAX8997 PMIC. The MAX8997 MUIC is a USB port accessory
 	  detector and switch.
 
+config EXTCON_OTG_GPIO
+	tristate "VIRTUAL USB OTG PORT support"
+	depends on GPIOLIB && ACPI
+	help
+	  Say Y here to enable support for virtual USB OTG port device
+	  controlled by GPIOs. This driver can be used when at least USB ID pin
+	  is connected directly to GPIO.
+
 config EXTCON_PALMAS
 	tristate "Palmas USB EXTCON support"
 	depends on MFD_PALMAS
diff --git a/drivers/extcon/Makefile b/drivers/extcon/Makefile
index 0370b42e5a27..0d5b152b51ea 100644
--- a/drivers/extcon/Makefile
+++ b/drivers/extcon/Makefile
@@ -9,6 +9,7 @@ obj-$(CONFIG_EXTCON_GPIO)	+= extcon-gpio.o
 obj-$(CONFIG_EXTCON_MAX14577)	+= extcon-max14577.o
 obj-$(CONFIG_EXTCON_MAX77693)	+= extcon-max77693.o
 obj-$(CONFIG_EXTCON_MAX8997)	+= extcon-max8997.o
+obj-$(CONFIG_EXTCON_OTG_GPIO)	+= extcon-otg_gpio.o
 obj-$(CONFIG_EXTCON_PALMAS)	+= extcon-palmas.o
 obj-$(CONFIG_EXTCON_RT8973A)	+= extcon-rt8973a.o
 obj-$(CONFIG_EXTCON_SM5502)	+= extcon-sm5502.o
diff --git a/drivers/extcon/extcon-otg_gpio.c b/drivers/extcon/extcon-otg_gpio.c
new file mode 100644
index 000000000000..7dfa2c15b562
--- /dev/null
+++ b/drivers/extcon/extcon-otg_gpio.c
@@ -0,0 +1,186 @@
+/*
+ * Virtual USB OTG Port driver controlled by gpios
+ *
+ * Copyright (c) 2014, Intel Corporation.
+ * Author: David Cohen <david.a.cohen@linux.intel.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * 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/acpi.h>
+#include <linux/extcon.h>
+#include <linux/gpio/consumer.h>
+#include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+
+#define DRV_NAME	"usb_otg_port"
+
+struct vuport {
+	struct device *dev;
+	struct gpio_desc *gpio_vbus_en;
+	struct gpio_desc *gpio_usb_id;
+	struct gpio_desc *gpio_usb_mux;
+
+	struct extcon_dev edev;
+};
+
+static const char *vuport_extcon_cable[] = {
+	[0] = "USB-Host",
+	NULL,
+};
+
+/*
+ * If id == 1, USB port should be set to peripheral
+ * if id == 0, USB port should be set to host
+ *
+ * Peripheral: set USB mux to peripheral and disable VBUS
+ * Host: set USB mux to host and enable VBUS
+ */
+static void vuport_set_port(struct vuport *vup, int id)
+{
+	int mux_val = id;
+	int vbus_val = !id;
+
+	if (!IS_ERR(vup->gpio_usb_mux))
+		gpiod_direction_output(vup->gpio_usb_mux, mux_val);
+
+	if (!IS_ERR(vup->gpio_vbus_en))
+		gpiod_direction_output(vup->gpio_vbus_en, vbus_val);
+}
+
+static void vuport_do_usb_id(struct vuport *vup)
+{
+	int id = gpiod_get_value(vup->gpio_usb_id);
+
+	dev_info(vup->dev, "USB PORT ID: %s\n", id ? "PERIPHERAL" : "HOST");
+
+	/*
+	 * id == 1: PERIPHERAL
+	 * id == 0: HOST
+	 */
+	vuport_set_port(vup, id);
+
+	/*
+	 * id == 0: HOST connected
+	 * id == 1: Host disconnected
+	 */
+	extcon_set_cable_state(&vup->edev, "USB-Host", !id);
+}
+
+static irqreturn_t vuport_thread_isr(int irq, void *priv)
+{
+	struct vuport *vup = priv;
+
+	vuport_do_usb_id(vup);
+	return IRQ_HANDLED;
+}
+
+#define VUPORT_GPIO_USB_ID	0
+#define VUPORT_GPIO_VBUS_EN	1
+#define VUPORT_GPIO_USB_MUX	2
+static int vuport_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct vuport *vup;
+	int ret;
+
+	vup = devm_kzalloc(dev, sizeof(*vup), GFP_KERNEL);
+	if (!vup) {
+		dev_err(dev, "cannot allocate private data\n");
+		return -ENOMEM;
+	}
+	vup->dev = dev;
+
+	vup->gpio_usb_id = devm_gpiod_get_index(dev, "id", VUPORT_GPIO_USB_ID);
+	if (IS_ERR(vup->gpio_usb_id)) {
+		dev_err(dev, "cannot request USB ID GPIO: ret = %ld\n",
+			PTR_ERR(vup->gpio_usb_id));
+		return PTR_ERR(vup->gpio_usb_id);
+	}
+
+	ret = gpiod_direction_input(vup->gpio_usb_id);
+	if (ret < 0) {
+		dev_err(dev, "cannot set input direction to USB ID GPIO: ret = %d\n",
+			ret);
+		return ret;
+	}
+
+	vup->gpio_vbus_en = devm_gpiod_get_index(dev, "vbus en",
+						 VUPORT_GPIO_VBUS_EN);
+	if (IS_ERR(vup->gpio_vbus_en))
+		dev_dbg(dev, "cannot request VBUS EN GPIO, skipping it.\n");
+
+	vup->gpio_usb_mux = devm_gpiod_get_index(dev, "usb mux",
+						 VUPORT_GPIO_USB_MUX);
+	if (IS_ERR(vup->gpio_usb_mux))
+		dev_dbg(dev, "cannot request USB USB MUX, skipping it.\n");
+
+	/* register extcon device */
+	vup->edev.dev.parent = dev;
+	vup->edev.supported_cable = vuport_extcon_cable;
+	ret = extcon_dev_register(&vup->edev);
+	if (ret < 0) {
+		dev_err(dev, "failed to register extcon device: ret = %d\n",
+			ret);
+		return ret;
+	}
+
+	ret = devm_request_threaded_irq(dev, gpiod_to_irq(vup->gpio_usb_id),
+					NULL, vuport_thread_isr,
+					IRQF_SHARED | IRQF_TRIGGER_RISING |
+					IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
+					dev_name(dev), vup);
+	if (ret < 0) {
+		dev_err(dev, "cannot request IRQ for USB ID GPIO: ret = %d\n",
+			ret);
+		goto irq_err;
+	}
+	vuport_do_usb_id(vup);
+
+	platform_set_drvdata(pdev, vup);
+
+	dev_dbg(dev, "driver successfully probed\n");
+
+	return 0;
+
+irq_err:
+	extcon_dev_unregister(&vup->edev);
+
+	return ret;
+}
+
+static int vuport_remove(struct platform_device *pdev)
+{
+	struct vuport *vup = platform_get_drvdata(pdev);
+
+	extcon_dev_unregister(&vup->edev);
+	return 0;
+}
+
+static struct acpi_device_id vuport_acpi_match[] = {
+	{ "INT3496" },
+	{ }
+};
+MODULE_DEVICE_TABLE(acpi, vuport_acpi_match);
+
+static struct platform_driver vuport_driver = {
+	.driver = {
+		.name = DRV_NAME,
+		.acpi_match_table = ACPI_PTR(vuport_acpi_match),
+	},
+	.probe = vuport_probe,
+	.remove = vuport_remove,
+};
+
+module_platform_driver(vuport_driver);
+
+MODULE_LICENSE("GPLv2");
+MODULE_AUTHOR("David Cohen <david.a.cohen@linux.intel.com>");
-- 
2.1.1

Re: [PATCH v2] extcon: otg_gpio: add driver for USB OTG port controlled by GPIO(s)

[Paul Bolle]

On Thu, 2015-02-19 at 11:59 -0800, David Cohen wrote:
> As always, comments are welcome.

Are nits welcome too?

> +MODULE_LICENSE("GPLv2");

You probably meant
    MODULE_LICENSE("GPL v2")

Didn't that trigger a warning or error?


Paul Bolle

Re: [PATCH v2] extcon: otg_gpio: add driver for USB OTG port controlled by GPIO(s)

[Robert Baldyga]

Hi David,

On 02/19/2015 08:59 PM, David Cohen wrote:
> Some Intel platforms have an USB OTG port fully (or partially)
> controlled by GPIOs:
> 
> (1) USB ID is connected directly to a pulled up GPIO.
> 
> Optionally:
> (2) VBUS is enabled/disabled by a GPIO
> (3) Platform has 2 USB controllers connected to same port: one for
>     device and one for host role. D+/- are switched between phys.
>     according to this GPIO level.
> 
> This driver configures USB OTG port for device or host role according to
> USB ID value.
>  - If USB ID's GPIO level is low, OTG port is configured for host role
>    by sourcing VBUS and switching D+/- to host phy.
>  - If USB ID's GPIO level is high, by standard, the OTG port is
>    configured for device role by not sourcing VBUS and switching D+/- to
>    device controller.

IMO it's not very elegant to handle VBUS power on/off in extcon driver.
Creating fixed regulator would allow to make VBUS handling more generic.
Then extcon client could handle VBUS regulator. Also D+/- routing would
be separated but I don't see better place for it.

Odroid platforms also uses GPIO for USB cable detection so your driver
would be useful in that case. I created some simple driver for it but I
see I could use your driver after adding VBUS detection.

BTW don't you have VBUS detection on your board? It would allow to
distinguish between USB, USB-Host and disconnected states.

--
Robert

> 
> Signed-off-by: David Cohen <david.a.cohen@linux.intel.com>
> ---
> 
> Hi,
> 
> Since splitting this driver into smaller pieces would result in ugly fixes WRT
> ACPI, I'm resending the same approach.
> This time I addressed all comments I got from RFC version.
> 
> As always, comments are welcome.
> 
> Br, David
> ---
> 
>  drivers/extcon/Kconfig           |   8 ++
>  drivers/extcon/Makefile          |   1 +
>  drivers/extcon/extcon-otg_gpio.c | 186 +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 195 insertions(+)
>  create mode 100644 drivers/extcon/extcon-otg_gpio.c
> 
> diff --git a/drivers/extcon/Kconfig b/drivers/extcon/Kconfig
> index 6a1f7de6fa54..986ca7da9c1b 100644
> --- a/drivers/extcon/Kconfig
> +++ b/drivers/extcon/Kconfig
> @@ -63,6 +63,14 @@ config EXTCON_MAX8997
>  	  Maxim MAX8997 PMIC. The MAX8997 MUIC is a USB port accessory
>  	  detector and switch.
>  
> +config EXTCON_OTG_GPIO
> +	tristate "VIRTUAL USB OTG PORT support"
> +	depends on GPIOLIB && ACPI
> +	help
> +	  Say Y here to enable support for virtual USB OTG port device
> +	  controlled by GPIOs. This driver can be used when at least USB ID pin
> +	  is connected directly to GPIO.
> +
>  config EXTCON_PALMAS
>  	tristate "Palmas USB EXTCON support"
>  	depends on MFD_PALMAS
> diff --git a/drivers/extcon/Makefile b/drivers/extcon/Makefile
> index 0370b42e5a27..0d5b152b51ea 100644
> --- a/drivers/extcon/Makefile
> +++ b/drivers/extcon/Makefile
> @@ -9,6 +9,7 @@ obj-$(CONFIG_EXTCON_GPIO)	+= extcon-gpio.o
>  obj-$(CONFIG_EXTCON_MAX14577)	+= extcon-max14577.o
>  obj-$(CONFIG_EXTCON_MAX77693)	+= extcon-max77693.o
>  obj-$(CONFIG_EXTCON_MAX8997)	+= extcon-max8997.o
> +obj-$(CONFIG_EXTCON_OTG_GPIO)	+= extcon-otg_gpio.o
>  obj-$(CONFIG_EXTCON_PALMAS)	+= extcon-palmas.o
>  obj-$(CONFIG_EXTCON_RT8973A)	+= extcon-rt8973a.o
>  obj-$(CONFIG_EXTCON_SM5502)	+= extcon-sm5502.o
> diff --git a/drivers/extcon/extcon-otg_gpio.c b/drivers/extcon/extcon-otg_gpio.c
> new file mode 100644
> index 000000000000..7dfa2c15b562
> --- /dev/null
> +++ b/drivers/extcon/extcon-otg_gpio.c
> @@ -0,0 +1,186 @@
> +/*
> + * Virtual USB OTG Port driver controlled by gpios
> + *
> + * Copyright (c) 2014, Intel Corporation.
> + * Author: David Cohen <david.a.cohen@linux.intel.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * 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/acpi.h>
> +#include <linux/extcon.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/interrupt.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +
> +#define DRV_NAME	"usb_otg_port"
> +
> +struct vuport {
> +	struct device *dev;
> +	struct gpio_desc *gpio_vbus_en;
> +	struct gpio_desc *gpio_usb_id;
> +	struct gpio_desc *gpio_usb_mux;
> +
> +	struct extcon_dev edev;
> +};
> +
> +static const char *vuport_extcon_cable[] = {
> +	[0] = "USB-Host",
> +	NULL,
> +};
> +
> +/*
> + * If id == 1, USB port should be set to peripheral
> + * if id == 0, USB port should be set to host
> + *
> + * Peripheral: set USB mux to peripheral and disable VBUS
> + * Host: set USB mux to host and enable VBUS
> + */
> +static void vuport_set_port(struct vuport *vup, int id)
> +{
> +	int mux_val = id;
> +	int vbus_val = !id;
> +
> +	if (!IS_ERR(vup->gpio_usb_mux))
> +		gpiod_direction_output(vup->gpio_usb_mux, mux_val);
> +
> +	if (!IS_ERR(vup->gpio_vbus_en))
> +		gpiod_direction_output(vup->gpio_vbus_en, vbus_val);
> +}
> +
> +static void vuport_do_usb_id(struct vuport *vup)
> +{
> +	int id = gpiod_get_value(vup->gpio_usb_id);
> +
> +	dev_info(vup->dev, "USB PORT ID: %s\n", id ? "PERIPHERAL" : "HOST");
> +
> +	/*
> +	 * id == 1: PERIPHERAL
> +	 * id == 0: HOST
> +	 */
> +	vuport_set_port(vup, id);
> +
> +	/*
> +	 * id == 0: HOST connected
> +	 * id == 1: Host disconnected
> +	 */
> +	extcon_set_cable_state(&vup->edev, "USB-Host", !id);
> +}
> +
> +static irqreturn_t vuport_thread_isr(int irq, void *priv)
> +{
> +	struct vuport *vup = priv;
> +
> +	vuport_do_usb_id(vup);
> +	return IRQ_HANDLED;
> +}
> +
> +#define VUPORT_GPIO_USB_ID	0
> +#define VUPORT_GPIO_VBUS_EN	1
> +#define VUPORT_GPIO_USB_MUX	2
> +static int vuport_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct vuport *vup;
> +	int ret;
> +
> +	vup = devm_kzalloc(dev, sizeof(*vup), GFP_KERNEL);
> +	if (!vup) {
> +		dev_err(dev, "cannot allocate private data\n");
> +		return -ENOMEM;
> +	}
> +	vup->dev = dev;
> +
> +	vup->gpio_usb_id = devm_gpiod_get_index(dev, "id", VUPORT_GPIO_USB_ID);
> +	if (IS_ERR(vup->gpio_usb_id)) {
> +		dev_err(dev, "cannot request USB ID GPIO: ret = %ld\n",
> +			PTR_ERR(vup->gpio_usb_id));
> +		return PTR_ERR(vup->gpio_usb_id);
> +	}
> +
> +	ret = gpiod_direction_input(vup->gpio_usb_id);
> +	if (ret < 0) {
> +		dev_err(dev, "cannot set input direction to USB ID GPIO: ret = %d\n",
> +			ret);
> +		return ret;
> +	}
> +
> +	vup->gpio_vbus_en = devm_gpiod_get_index(dev, "vbus en",
> +						 VUPORT_GPIO_VBUS_EN);
> +	if (IS_ERR(vup->gpio_vbus_en))
> +		dev_dbg(dev, "cannot request VBUS EN GPIO, skipping it.\n");
> +
> +	vup->gpio_usb_mux = devm_gpiod_get_index(dev, "usb mux",
> +						 VUPORT_GPIO_USB_MUX);
> +	if (IS_ERR(vup->gpio_usb_mux))
> +		dev_dbg(dev, "cannot request USB USB MUX, skipping it.\n");
> +
> +	/* register extcon device */
> +	vup->edev.dev.parent = dev;
> +	vup->edev.supported_cable = vuport_extcon_cable;
> +	ret = extcon_dev_register(&vup->edev);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to register extcon device: ret = %d\n",
> +			ret);
> +		return ret;
> +	}
> +
> +	ret = devm_request_threaded_irq(dev, gpiod_to_irq(vup->gpio_usb_id),
> +					NULL, vuport_thread_isr,
> +					IRQF_SHARED | IRQF_TRIGGER_RISING |
> +					IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> +					dev_name(dev), vup);
> +	if (ret < 0) {
> +		dev_err(dev, "cannot request IRQ for USB ID GPIO: ret = %d\n",
> +			ret);
> +		goto irq_err;
> +	}
> +	vuport_do_usb_id(vup);
> +
> +	platform_set_drvdata(pdev, vup);
> +
> +	dev_dbg(dev, "driver successfully probed\n");
> +
> +	return 0;
> +
> +irq_err:
> +	extcon_dev_unregister(&vup->edev);
> +
> +	return ret;
> +}
> +
> +static int vuport_remove(struct platform_device *pdev)
> +{
> +	struct vuport *vup = platform_get_drvdata(pdev);
> +
> +	extcon_dev_unregister(&vup->edev);
> +	return 0;
> +}
> +
> +static struct acpi_device_id vuport_acpi_match[] = {
> +	{ "INT3496" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(acpi, vuport_acpi_match);
> +
> +static struct platform_driver vuport_driver = {
> +	.driver = {
> +		.name = DRV_NAME,
> +		.acpi_match_table = ACPI_PTR(vuport_acpi_match),
> +	},
> +	.probe = vuport_probe,
> +	.remove = vuport_remove,
> +};
> +
> +module_platform_driver(vuport_driver);
> +
> +MODULE_LICENSE("GPLv2");
> +MODULE_AUTHOR("David Cohen <david.a.cohen@linux.intel.com>");
> 

Re: [PATCH v2] extcon: otg_gpio: add driver for USB OTG port controlled by GPIO(s)

[Linus Walleij]

On Fri, Feb 20, 2015 at 7:41 AM, Robert Baldyga <r.baldyga@samsung.com> wrote:
> Hi David,
>
> On 02/19/2015 08:59 PM, David Cohen wrote:
>> Some Intel platforms have an USB OTG port fully (or partially)
>> controlled by GPIOs:
>>
>> (1) USB ID is connected directly to a pulled up GPIO.
>>
>> Optionally:
>> (2) VBUS is enabled/disabled by a GPIO
>> (3) Platform has 2 USB controllers connected to same port: one for
>>     device and one for host role. D+/- are switched between phys.
>>     according to this GPIO level.
>>
>> This driver configures USB OTG port for device or host role according to
>> USB ID value.
>>  - If USB ID's GPIO level is low, OTG port is configured for host role
>>    by sourcing VBUS and switching D+/- to host phy.
>>  - If USB ID's GPIO level is high, by standard, the OTG port is
>>    configured for device role by not sourcing VBUS and switching D+/- to
>>    device controller.
>
> IMO it's not very elegant to handle VBUS power on/off in extcon driver.
> Creating fixed regulator would allow to make VBUS handling more generic.

IMHO it's just layers of abstraction piled on top of each other here.

I would put this adjacent to the phy driver somewhere in drivers/usb/*
and make the actual USB-driver thing handle its GPIOs directly.
But I guess David and Felipe have already discussed that as we're
seeing this patch?

Yours,
Linus Walleij

Re: [PATCH v2] extcon: otg_gpio: add driver for USB OTG port controlled by GPIO(s)

[David Cohen]

Hi,

On Thu, Feb 19, 2015 at 11:39:06PM +0100, Paul Bolle wrote:
> On Thu, 2015-02-19 at 11:59 -0800, David Cohen wrote:
> > As always, comments are welcome.
> 
> Are nits welcome too?
> 
> > +MODULE_LICENSE("GPLv2");
> 
> You probably meant
>     MODULE_LICENSE("GPL v2")
> 
> Didn't that trigger a warning or error?

checkpatch showed no warning about that, not even with --strict option.
I believe both ways are fine. But I can add the space.

Br, David

> 
> 
> Paul Bolle
> 

Re: [PATCH v2] extcon: otg_gpio: add driver for USB OTG port controlled by GPIO(s)

[Felipe Balbi]

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

On Fri, Feb 20, 2015 at 11:02:26AM -0800, David Cohen wrote:
> Hi,
> 
> On Thu, Feb 19, 2015 at 11:39:06PM +0100, Paul Bolle wrote:
> > On Thu, 2015-02-19 at 11:59 -0800, David Cohen wrote:
> > > As always, comments are welcome.
> > 
> > Are nits welcome too?
> > 
> > > +MODULE_LICENSE("GPLv2");
> > 
> > You probably meant
> >     MODULE_LICENSE("GPL v2")
> > 
> > Didn't that trigger a warning or error?
> 
> checkpatch showed no warning about that, not even with --strict option.
> I believe both ways are fine. But I can add the space.

Documentation says it should be with space:

/*
 * The following license idents are currently accepted as indicating free
 * software modules
 *
 *	"GPL"				[GNU Public License v2 or later]
 *	"GPL v2"			[GNU Public License v2]
 *	"GPL and additional rights"	[GNU Public License v2 rights and more]
 *	"Dual BSD/GPL"			[GNU Public License v2
 *					 or BSD license choice]
 *	"Dual MIT/GPL"			[GNU Public License v2
 *					 or MIT license choice]
 *	"Dual MPL/GPL"			[GNU Public License v2
 *					 or Mozilla license choice]
 *
 * The following other idents are available
 *
 *	"Proprietary"			[Non free products]
 *
 * There are dual licensed components, but when running with Linux it is the
 * GPL that is relevant so this is a non issue. Similarly LGPL linked with GPL
 * is a GPL combined work.
 *
 * This exists for several reasons
 * 1.	So modinfo can show license info for users wanting to vet their setup
 *	is free
 * 2.	So the community can ignore bug reports including proprietary modules
 * 3.	So vendors can do likewise based on their own policies
 */
#define MODULE_LICENSE(_license) MODULE_INFO(license, _license)

-- 
balbi

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

Re: [PATCH v2] extcon: otg_gpio: add driver for USB OTG port controlled by GPIO(s)

[Paul Bolle]

On Fri, 2015-02-20 at 11:02 -0800, David Cohen wrote:
> On Thu, Feb 19, 2015 at 11:39:06PM +0100, Paul Bolle wrote:
> > On Thu, 2015-02-19 at 11:59 -0800, David Cohen wrote:
> > > As always, comments are welcome.
> > 
> > Are nits welcome too?
> > 
> > > +MODULE_LICENSE("GPLv2");
> > 
> > You probably meant
> >     MODULE_LICENSE("GPL v2")
> > 
> > Didn't that trigger a warning or error?
> 
> checkpatch showed no warning about that, not even with --strict option.
> I believe both ways are fine. But I can add the space.

"GPLv2" is not mentioned in include/linux/license.h so, as I remember,
it would taint the kernel, and a warning will be printed when you load
that module.

Thanks,


Paul Bolle

Re: [PATCH v2] extcon: otg_gpio: add driver for USB OTG port controlled by GPIO(s)

[David Cohen]

Hi Linus and Robert,

CC'ing Heikki as it involves a RFC from him.

On Fri, Feb 20, 2015 at 10:53:44AM +0100, Linus Walleij wrote:
> On Fri, Feb 20, 2015 at 7:41 AM, Robert Baldyga <r.baldyga@samsung.com> wrote:
> > Hi David,
> >
> > On 02/19/2015 08:59 PM, David Cohen wrote:
> >> Some Intel platforms have an USB OTG port fully (or partially)
> >> controlled by GPIOs:
> >>
> >> (1) USB ID is connected directly to a pulled up GPIO.
> >>
> >> Optionally:
> >> (2) VBUS is enabled/disabled by a GPIO
> >> (3) Platform has 2 USB controllers connected to same port: one for
> >>     device and one for host role. D+/- are switched between phys.
> >>     according to this GPIO level.
> >>
> >> This driver configures USB OTG port for device or host role according to
> >> USB ID value.
> >>  - If USB ID's GPIO level is low, OTG port is configured for host role
> >>    by sourcing VBUS and switching D+/- to host phy.
> >>  - If USB ID's GPIO level is high, by standard, the OTG port is
> >>    configured for device role by not sourcing VBUS and switching D+/- to
> >>    device controller.
> >
> > IMO it's not very elegant to handle VBUS power on/off in extcon driver.
> > Creating fixed regulator would allow to make VBUS handling more generic.

I agree. But please, see below.

> 
> IMHO it's just layers of abstraction piled on top of each other here.
> 
> I would put this adjacent to the phy driver somewhere in drivers/usb/*
> and make the actual USB-driver thing handle its GPIOs directly.
> But I guess David and Felipe have already discussed that as we're
> seeing this patch?

Felipe suggested to "divide to conquer" instead of having a single
extcon driver to handle all these functions:

- The mux functions would be controlled by a possible new pinctrl-gpio
driver (Linus, your input here would be nice :)
- The VBUS would be a fixed regulator
- The USB ID would make usage of existent extcon-gpio

But the on fw side, this is a single ACPI device representing a virtual
device for USB OTG port, which is nothing but a bunch of independent
GPIOs.

I could make a mfd driver to register devices for those simpler and more
generic drivers, but according to [1] community recognized it as a hack
with ACPI since I'd need to give them the GPIO without requesting on
mfd.

I'm open for suggestions :)

Br, David

[1] https://lkml.org/lkml/2014/12/18/82

> 
> Yours,
> Linus Walleij

Re: [PATCH v2] extcon: otg_gpio: add driver for USB OTG port controlled by GPIO(s)

[David Cohen]

On Fri, Feb 20, 2015 at 08:10:34PM +0100, Paul Bolle wrote:
> On Fri, 2015-02-20 at 11:02 -0800, David Cohen wrote:
> > On Thu, Feb 19, 2015 at 11:39:06PM +0100, Paul Bolle wrote:
> > > On Thu, 2015-02-19 at 11:59 -0800, David Cohen wrote:
> > > > As always, comments are welcome.
> > > 
> > > Are nits welcome too?
> > > 
> > > > +MODULE_LICENSE("GPLv2");
> > > 
> > > You probably meant
> > >     MODULE_LICENSE("GPL v2")
> > > 
> > > Didn't that trigger a warning or error?
> > 
> > checkpatch showed no warning about that, not even with --strict option.
> > I believe both ways are fine. But I can add the space.
> 
> "GPLv2" is not mentioned in include/linux/license.h so, as I remember,
> it would taint the kernel, and a warning will be printed when you load
> that module.

Ack :)
I'll add a space on next version.

Br, David

> 
> Thanks,
> 
> 
> Paul Bolle
> 

Re: [PATCH v2] extcon: otg_gpio: add driver for USB OTG port controlled by GPIO(s)

[David Cohen]

On Fri, Feb 20, 2015 at 01:09:00PM -0600, Felipe Balbi wrote:
> On Fri, Feb 20, 2015 at 11:02:26AM -0800, David Cohen wrote:
> > Hi,
> > 
> > On Thu, Feb 19, 2015 at 11:39:06PM +0100, Paul Bolle wrote:
> > > On Thu, 2015-02-19 at 11:59 -0800, David Cohen wrote:
> > > > As always, comments are welcome.
> > > 
> > > Are nits welcome too?
> > > 
> > > > +MODULE_LICENSE("GPLv2");
> > > 
> > > You probably meant
> > >     MODULE_LICENSE("GPL v2")
> > > 
> > > Didn't that trigger a warning or error?
> > 
> > checkpatch showed no warning about that, not even with --strict option.
> > I believe both ways are fine. But I can add the space.
> 
> Documentation says it should be with space:
> 
> /*
>  * The following license idents are currently accepted as indicating free
>  * software modules
>  *
>  *	"GPL"				[GNU Public License v2 or later]
>  *	"GPL v2"			[GNU Public License v2]

Thanks :)
I'll add it.

Br, David

Re: [PATCH v2] extcon: otg_gpio: add driver for USB OTG port controlled by GPIO(s)

[Felipe Balbi]

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

On Fri, Feb 20, 2015 at 11:17:00AM -0800, David Cohen wrote:
> Hi Linus and Robert,
> 
> CC'ing Heikki as it involves a RFC from him.
> 
> On Fri, Feb 20, 2015 at 10:53:44AM +0100, Linus Walleij wrote:
> > On Fri, Feb 20, 2015 at 7:41 AM, Robert Baldyga <r.baldyga@samsung.com> wrote:
> > > Hi David,
> > >
> > > On 02/19/2015 08:59 PM, David Cohen wrote:
> > >> Some Intel platforms have an USB OTG port fully (or partially)
> > >> controlled by GPIOs:
> > >>
> > >> (1) USB ID is connected directly to a pulled up GPIO.
> > >>
> > >> Optionally:
> > >> (2) VBUS is enabled/disabled by a GPIO
> > >> (3) Platform has 2 USB controllers connected to same port: one for
> > >>     device and one for host role. D+/- are switched between phys.
> > >>     according to this GPIO level.
> > >>
> > >> This driver configures USB OTG port for device or host role according to
> > >> USB ID value.
> > >>  - If USB ID's GPIO level is low, OTG port is configured for host role
> > >>    by sourcing VBUS and switching D+/- to host phy.
> > >>  - If USB ID's GPIO level is high, by standard, the OTG port is
> > >>    configured for device role by not sourcing VBUS and switching D+/- to
> > >>    device controller.
> > >
> > > IMO it's not very elegant to handle VBUS power on/off in extcon driver.
> > > Creating fixed regulator would allow to make VBUS handling more generic.
> 
> I agree. But please, see below.
> 
> > 
> > IMHO it's just layers of abstraction piled on top of each other here.
> > 
> > I would put this adjacent to the phy driver somewhere in drivers/usb/*
> > and make the actual USB-driver thing handle its GPIOs directly.
> > But I guess David and Felipe have already discussed that as we're
> > seeing this patch?
> 
> Felipe suggested to "divide to conquer" instead of having a single
> extcon driver to handle all these functions:
> 
> - The mux functions would be controlled by a possible new pinctrl-gpio
> driver (Linus, your input here would be nice :)
> - The VBUS would be a fixed regulator
> - The USB ID would make usage of existent extcon-gpio
> 
> But the on fw side, this is a single ACPI device representing a virtual
> device for USB OTG port, which is nothing but a bunch of independent
> GPIOs.
> 
> I could make a mfd driver to register devices for those simpler and more
> generic drivers, but according to [1] community recognized it as a hack
> with ACPI since I'd need to give them the GPIO without requesting on
> mfd.
> 
> I'm open for suggestions :)

use MFD to create children devices and pass the required data to each
one ?

-- 
balbi

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

Re: [PATCH v2] extcon: otg_gpio: add driver for USB OTG port controlled by GPIO(s)

[David Cohen]

On Fri, Feb 20, 2015 at 01:36:06PM -0600, Felipe Balbi wrote:
> On Fri, Feb 20, 2015 at 11:17:00AM -0800, David Cohen wrote:
> > Hi Linus and Robert,
> > 
> > CC'ing Heikki as it involves a RFC from him.
> > 
> > On Fri, Feb 20, 2015 at 10:53:44AM +0100, Linus Walleij wrote:
> > > On Fri, Feb 20, 2015 at 7:41 AM, Robert Baldyga <r.baldyga@samsung.com> wrote:
> > > > Hi David,
> > > >
> > > > On 02/19/2015 08:59 PM, David Cohen wrote:
> > > >> Some Intel platforms have an USB OTG port fully (or partially)
> > > >> controlled by GPIOs:
> > > >>
> > > >> (1) USB ID is connected directly to a pulled up GPIO.
> > > >>
> > > >> Optionally:
> > > >> (2) VBUS is enabled/disabled by a GPIO
> > > >> (3) Platform has 2 USB controllers connected to same port: one for
> > > >>     device and one for host role. D+/- are switched between phys.
> > > >>     according to this GPIO level.
> > > >>
> > > >> This driver configures USB OTG port for device or host role according to
> > > >> USB ID value.
> > > >>  - If USB ID's GPIO level is low, OTG port is configured for host role
> > > >>    by sourcing VBUS and switching D+/- to host phy.
> > > >>  - If USB ID's GPIO level is high, by standard, the OTG port is
> > > >>    configured for device role by not sourcing VBUS and switching D+/- to
> > > >>    device controller.
> > > >
> > > > IMO it's not very elegant to handle VBUS power on/off in extcon driver.
> > > > Creating fixed regulator would allow to make VBUS handling more generic.
> > 
> > I agree. But please, see below.
> > 
> > > 
> > > IMHO it's just layers of abstraction piled on top of each other here.
> > > 
> > > I would put this adjacent to the phy driver somewhere in drivers/usb/*
> > > and make the actual USB-driver thing handle its GPIOs directly.
> > > But I guess David and Felipe have already discussed that as we're
> > > seeing this patch?
> > 
> > Felipe suggested to "divide to conquer" instead of having a single
> > extcon driver to handle all these functions:
> > 
> > - The mux functions would be controlled by a possible new pinctrl-gpio
> > driver (Linus, your input here would be nice :)
> > - The VBUS would be a fixed regulator
> > - The USB ID would make usage of existent extcon-gpio
> > 
> > But the on fw side, this is a single ACPI device representing a virtual
> > device for USB OTG port, which is nothing but a bunch of independent
> > GPIOs.
> > 
> > I could make a mfd driver to register devices for those simpler and more
> > generic drivers, but according to [1] community recognized it as a hack
> > with ACPI since I'd need to give them the GPIO without requesting on
> > mfd.
> > 
> > I'm open for suggestions :)
> 
> use MFD to create children devices and pass the required data to each
> one ?

I'd need to lookup GPIOs via ACPI without requesting them on mfd driver
and then give them to children devices.
Heikki proposed a way to do that on [1], but it got nack'ed by community.

Br, David

> 
> -- 
> balbi

Re: [PATCH v2] extcon: otg_gpio: add driver for USB OTG port controlled by GPIO(s)

[Felipe Balbi]

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

On Fri, Feb 20, 2015 at 11:59:27AM -0800, David Cohen wrote:
> On Fri, Feb 20, 2015 at 01:36:06PM -0600, Felipe Balbi wrote:
> > On Fri, Feb 20, 2015 at 11:17:00AM -0800, David Cohen wrote:
> > > Hi Linus and Robert,
> > > 
> > > CC'ing Heikki as it involves a RFC from him.
> > > 
> > > On Fri, Feb 20, 2015 at 10:53:44AM +0100, Linus Walleij wrote:
> > > > On Fri, Feb 20, 2015 at 7:41 AM, Robert Baldyga <r.baldyga@samsung.com> wrote:
> > > > > Hi David,
> > > > >
> > > > > On 02/19/2015 08:59 PM, David Cohen wrote:
> > > > >> Some Intel platforms have an USB OTG port fully (or partially)
> > > > >> controlled by GPIOs:
> > > > >>
> > > > >> (1) USB ID is connected directly to a pulled up GPIO.
> > > > >>
> > > > >> Optionally:
> > > > >> (2) VBUS is enabled/disabled by a GPIO
> > > > >> (3) Platform has 2 USB controllers connected to same port: one for
> > > > >>     device and one for host role. D+/- are switched between phys.
> > > > >>     according to this GPIO level.
> > > > >>
> > > > >> This driver configures USB OTG port for device or host role according to
> > > > >> USB ID value.
> > > > >>  - If USB ID's GPIO level is low, OTG port is configured for host role
> > > > >>    by sourcing VBUS and switching D+/- to host phy.
> > > > >>  - If USB ID's GPIO level is high, by standard, the OTG port is
> > > > >>    configured for device role by not sourcing VBUS and switching D+/- to
> > > > >>    device controller.
> > > > >
> > > > > IMO it's not very elegant to handle VBUS power on/off in extcon driver.
> > > > > Creating fixed regulator would allow to make VBUS handling more generic.
> > > 
> > > I agree. But please, see below.
> > > 
> > > > 
> > > > IMHO it's just layers of abstraction piled on top of each other here.
> > > > 
> > > > I would put this adjacent to the phy driver somewhere in drivers/usb/*
> > > > and make the actual USB-driver thing handle its GPIOs directly.
> > > > But I guess David and Felipe have already discussed that as we're
> > > > seeing this patch?
> > > 
> > > Felipe suggested to "divide to conquer" instead of having a single
> > > extcon driver to handle all these functions:
> > > 
> > > - The mux functions would be controlled by a possible new pinctrl-gpio
> > > driver (Linus, your input here would be nice :)
> > > - The VBUS would be a fixed regulator
> > > - The USB ID would make usage of existent extcon-gpio
> > > 
> > > But the on fw side, this is a single ACPI device representing a virtual
> > > device for USB OTG port, which is nothing but a bunch of independent
> > > GPIOs.
> > > 
> > > I could make a mfd driver to register devices for those simpler and more
> > > generic drivers, but according to [1] community recognized it as a hack
> > > with ACPI since I'd need to give them the GPIO without requesting on
> > > mfd.
> > > 
> > > I'm open for suggestions :)
> > 
> > use MFD to create children devices and pass the required data to each
> > one ?
> 
> I'd need to lookup GPIOs via ACPI without requesting them on mfd driver
> and then give them to children devices.
> Heikki proposed a way to do that on [1], but it got nack'ed by community.

you missed [1] :-)

-- 
balbi

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

Re: [PATCH v2] extcon: otg_gpio: add driver for USB OTG port controlled by GPIO(s)

[David Cohen]

On Fri, Feb 20, 2015 at 02:00:26PM -0600, Felipe Balbi wrote:
> On Fri, Feb 20, 2015 at 11:59:27AM -0800, David Cohen wrote:
> > On Fri, Feb 20, 2015 at 01:36:06PM -0600, Felipe Balbi wrote:
> > > On Fri, Feb 20, 2015 at 11:17:00AM -0800, David Cohen wrote:
> > > > Hi Linus and Robert,
> > > > 
> > > > CC'ing Heikki as it involves a RFC from him.
> > > > 
> > > > On Fri, Feb 20, 2015 at 10:53:44AM +0100, Linus Walleij wrote:
> > > > > On Fri, Feb 20, 2015 at 7:41 AM, Robert Baldyga <r.baldyga@samsung.com> wrote:
> > > > > > Hi David,
> > > > > >
> > > > > > On 02/19/2015 08:59 PM, David Cohen wrote:
> > > > > >> Some Intel platforms have an USB OTG port fully (or partially)
> > > > > >> controlled by GPIOs:
> > > > > >>
> > > > > >> (1) USB ID is connected directly to a pulled up GPIO.
> > > > > >>
> > > > > >> Optionally:
> > > > > >> (2) VBUS is enabled/disabled by a GPIO
> > > > > >> (3) Platform has 2 USB controllers connected to same port: one for
> > > > > >>     device and one for host role. D+/- are switched between phys.
> > > > > >>     according to this GPIO level.
> > > > > >>
> > > > > >> This driver configures USB OTG port for device or host role according to
> > > > > >> USB ID value.
> > > > > >>  - If USB ID's GPIO level is low, OTG port is configured for host role
> > > > > >>    by sourcing VBUS and switching D+/- to host phy.
> > > > > >>  - If USB ID's GPIO level is high, by standard, the OTG port is
> > > > > >>    configured for device role by not sourcing VBUS and switching D+/- to
> > > > > >>    device controller.
> > > > > >
> > > > > > IMO it's not very elegant to handle VBUS power on/off in extcon driver.
> > > > > > Creating fixed regulator would allow to make VBUS handling more generic.
> > > > 
> > > > I agree. But please, see below.
> > > > 
> > > > > 
> > > > > IMHO it's just layers of abstraction piled on top of each other here.
> > > > > 
> > > > > I would put this adjacent to the phy driver somewhere in drivers/usb/*
> > > > > and make the actual USB-driver thing handle its GPIOs directly.
> > > > > But I guess David and Felipe have already discussed that as we're
> > > > > seeing this patch?
> > > > 
> > > > Felipe suggested to "divide to conquer" instead of having a single
> > > > extcon driver to handle all these functions:
> > > > 
> > > > - The mux functions would be controlled by a possible new pinctrl-gpio
> > > > driver (Linus, your input here would be nice :)
> > > > - The VBUS would be a fixed regulator
> > > > - The USB ID would make usage of existent extcon-gpio
> > > > 
> > > > But the on fw side, this is a single ACPI device representing a virtual
> > > > device for USB OTG port, which is nothing but a bunch of independent
> > > > GPIOs.
> > > > 
> > > > I could make a mfd driver to register devices for those simpler and more
> > > > generic drivers, but according to [1] community recognized it as a hack
> > > > with ACPI since I'd need to give them the GPIO without requesting on
> > > > mfd.
> > > > 
> > > > I'm open for suggestions :)
> > > 
> > > use MFD to create children devices and pass the required data to each
> > > one ?
> > 
> > I'd need to lookup GPIOs via ACPI without requesting them on mfd driver
> > and then give them to children devices.
> > Heikki proposed a way to do that on [1], but it got nack'ed by community.
> 
> you missed [1] :-)

Oops. Looks like it went away during your past reply :)

[1] https://lkml.org/lkml/2014/12/18/82

Br, David

> 
> -- 
> balbi

Re: [PATCH v2] extcon: otg_gpio: add driver for USB OTG port controlled by GPIO(s)

[David Cohen]

Hi,

[snip]

> Felipe suggested to "divide to conquer" instead of having a single
> extcon driver to handle all these functions:
> 
> - The mux functions would be controlled by a possible new pinctrl-gpio
> driver (Linus, your input here would be nice :)
> - The VBUS would be a fixed regulator
> - The USB ID would make usage of existent extcon-gpio
> 
> But the on fw side, this is a single ACPI device representing a virtual
> device for USB OTG port, which is nothing but a bunch of independent
> GPIOs.
> 
> I could make a mfd driver to register devices for those simpler and more
> generic drivers, but according to [1] community recognized it as a hack
> with ACPI since I'd need to give them the GPIO without requesting on
> mfd.

I believe this case could be resumed in:

Would be [1] acceptable for mfd drivers?
- If yes, I can split this driver into more generic ones
- If no, I see no other option but having this driver fully controlling
  the USB OTG port.

BR, David

[1] https://lkml.org/lkml/2014/12/18/82

Re: [PATCH v2] extcon: otg_gpio: add driver for USB OTG port controlled by GPIO(s)

[Linus Walleij]

On Fri, Feb 20, 2015 at 9:40 PM, David Cohen
<david.a.cohen@linux.intel.com> wrote:
> On Fri, Feb 20, 2015 at 02:00:26PM -0600, Felipe Balbi wrote:
>> On Fri, Feb 20, 2015 at 11:59:27AM -0800, David Cohen wrote:

>> > I'd need to lookup GPIOs via ACPI without requesting them on mfd driver
>> > and then give them to children devices.
>> > Heikki proposed a way to do that on [1], but it got nack'ed by community.
>>
>> you missed [1] :-)
>
> Oops. Looks like it went away during your past reply :)
>
> [1] https://lkml.org/lkml/2014/12/18/82

We don't want to do that in generic code since it's an ACPI
problem, not a generic GPIO problem.

So the solution must be confined to the GPIO ACPI portions
of the GPIO code, not altering central mechanisms.

Yours,
Linus Walleij

Re: [PATCH v2] extcon: otg_gpio: add driver for USB OTG port controlled by GPIO(s)

[Linus Walleij]

On Fri, Feb 20, 2015 at 8:17 PM, David Cohen
<david.a.cohen@linux.intel.com> wrote:
> On Fri, Feb 20, 2015 at 10:53:44AM +0100, Linus Walleij wrote:

>> I would put this adjacent to the phy driver somewhere in drivers/usb/*
>> and make the actual USB-driver thing handle its GPIOs directly.
>> But I guess David and Felipe have already discussed that as we're
>> seeing this patch?
>
> - The mux functions would be controlled by a possible new pinctrl-gpio
> driver (Linus, your input here would be nice :)

I don't understand what this means, does it mean a pin control function
somewhere else controlled by a GPIO pin?

Or do you mean a new combined pin control and GPIO driver (we have
plenty of these).

If you elaborate on what you need to do in that driver I might
understand it better.

Yours,
Linus Walleij

Re: [PATCH v2] extcon: otg_gpio: add driver for USB OTG port controlled by GPIO(s)

[Felipe Balbi]

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

On Sat, Mar 07, 2015 at 09:06:22PM +0100, Linus Walleij wrote:
> On Fri, Feb 20, 2015 at 8:17 PM, David Cohen
> <david.a.cohen@linux.intel.com> wrote:
> > On Fri, Feb 20, 2015 at 10:53:44AM +0100, Linus Walleij wrote:
> 
> >> I would put this adjacent to the phy driver somewhere in drivers/usb/*
> >> and make the actual USB-driver thing handle its GPIOs directly.
> >> But I guess David and Felipe have already discussed that as we're
> >> seeing this patch?
> >
> > - The mux functions would be controlled by a possible new pinctrl-gpio
> > driver (Linus, your input here would be nice :)
> 
> I don't understand what this means, does it mean a pin control function
> somewhere else controlled by a GPIO pin?
> 
> Or do you mean a new combined pin control and GPIO driver (we have
> plenty of these).
> 
> If you elaborate on what you need to do in that driver I might
> understand it better.

there's a discrete mux (not something integrated in the SoC) whose
select signal is tied to a GPIO (in some cases, more than one, but
usually people use 2-state muxes).

-- 
balbi

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

Re: [PATCH v2] extcon: otg_gpio: add driver for USB OTG port controlled by GPIO(s)

[David Cohen]

Hi Linus,

On Mon, Mar 09, 2015 at 11:16:08AM -0500, Felipe Balbi wrote:
> On Sat, Mar 07, 2015 at 09:06:22PM +0100, Linus Walleij wrote:
> > On Fri, Feb 20, 2015 at 8:17 PM, David Cohen
> > <david.a.cohen@linux.intel.com> wrote:
> > > On Fri, Feb 20, 2015 at 10:53:44AM +0100, Linus Walleij wrote:
> > 
> > >> I would put this adjacent to the phy driver somewhere in drivers/usb/*
> > >> and make the actual USB-driver thing handle its GPIOs directly.
> > >> But I guess David and Felipe have already discussed that as we're
> > >> seeing this patch?
> > >
> > > - The mux functions would be controlled by a possible new pinctrl-gpio
> > > driver (Linus, your input here would be nice :)
> > 
> > I don't understand what this means, does it mean a pin control function
> > somewhere else controlled by a GPIO pin?
> > 
> > Or do you mean a new combined pin control and GPIO driver (we have
> > plenty of these).
> > 
> > If you elaborate on what you need to do in that driver I might
> > understand it better.

This is a "virtual" ACPI device. Long story short we've 3 GPIOs
controlling the state of an USB OTG port. Neither of the hw components
controlled by these GPIOs are connected to a bus.
The ACPI table was implemented in such way that it considers this USB
port as a single "device" giving all 3 GPIOs to it.

When upstreaming this driver, the feedback I got is to split this driver
into simpler and more generic ones. FWIW ACPI tables are constructed
considering a valid Linux API during its implementation and are quite
difficult to change after product is released. The solution would be to
use MFD subsystem: this driver would create simpler children platform
devices.

But at least on ACPI case, GPIO API blocks the ability to create
children platform devices that require GPIO as platform data, despite
we've many drivers on upstream that expect this behavior: e.g. extcon-gpio.c

Are we considering those driver deprecated from the GPIO point of view?
If yes, we need to provide an update for them (we can't let upstreamed
drivers broken). If no, we need to provide a way to move forward the
GPIO to children devices.

BR, David

> 
> there's a discrete mux (not something integrated in the SoC) whose
> select signal is tied to a GPIO (in some cases, more than one, but
> usually people use 2-state muxes).
> 
> -- 
> balbi

Re: [PATCH v2] extcon: otg_gpio: add driver for USB OTG port controlled by GPIO(s)

[David Cohen]

Adding Mika to CC list.

Br, David

On Mon, Mar 09, 2015 at 12:10:51PM -0700, David Cohen wrote:
> Hi Linus,
> 
> On Mon, Mar 09, 2015 at 11:16:08AM -0500, Felipe Balbi wrote:
> > On Sat, Mar 07, 2015 at 09:06:22PM +0100, Linus Walleij wrote:
> > > On Fri, Feb 20, 2015 at 8:17 PM, David Cohen
> > > <david.a.cohen@linux.intel.com> wrote:
> > > > On Fri, Feb 20, 2015 at 10:53:44AM +0100, Linus Walleij wrote:
> > > 
> > > >> I would put this adjacent to the phy driver somewhere in drivers/usb/*
> > > >> and make the actual USB-driver thing handle its GPIOs directly.
> > > >> But I guess David and Felipe have already discussed that as we're
> > > >> seeing this patch?
> > > >
> > > > - The mux functions would be controlled by a possible new pinctrl-gpio
> > > > driver (Linus, your input here would be nice :)
> > > 
> > > I don't understand what this means, does it mean a pin control function
> > > somewhere else controlled by a GPIO pin?
> > > 
> > > Or do you mean a new combined pin control and GPIO driver (we have
> > > plenty of these).
> > > 
> > > If you elaborate on what you need to do in that driver I might
> > > understand it better.
> 
> This is a "virtual" ACPI device. Long story short we've 3 GPIOs
> controlling the state of an USB OTG port. Neither of the hw components
> controlled by these GPIOs are connected to a bus.
> The ACPI table was implemented in such way that it considers this USB
> port as a single "device" giving all 3 GPIOs to it.
> 
> When upstreaming this driver, the feedback I got is to split this driver
> into simpler and more generic ones. FWIW ACPI tables are constructed
> considering a valid Linux API during its implementation and are quite
> difficult to change after product is released. The solution would be to
> use MFD subsystem: this driver would create simpler children platform
> devices.
> 
> But at least on ACPI case, GPIO API blocks the ability to create
> children platform devices that require GPIO as platform data, despite
> we've many drivers on upstream that expect this behavior: e.g. extcon-gpio.c
> 
> Are we considering those driver deprecated from the GPIO point of view?
> If yes, we need to provide an update for them (we can't let upstreamed
> drivers broken). If no, we need to provide a way to move forward the
> GPIO to children devices.
> 
> BR, David
> 
> > 
> > there's a discrete mux (not something integrated in the SoC) whose
> > select signal is tied to a GPIO (in some cases, more than one, but
> > usually people use 2-state muxes).
> > 
> > -- 
> > balbi
> 
> 

Re: [PATCH v2] extcon: otg_gpio: add driver for USB OTG port controlled by GPIO(s)

[David Cohen]

On Mon, Mar 16, 2015 at 09:46:00AM -0700, David Cohen wrote:
> Adding Mika to CC list.

Grrr :(
Adding for real now.

> 
> Br, David
> 
> On Mon, Mar 09, 2015 at 12:10:51PM -0700, David Cohen wrote:
> > Hi Linus,
> > 
> > On Mon, Mar 09, 2015 at 11:16:08AM -0500, Felipe Balbi wrote:
> > > On Sat, Mar 07, 2015 at 09:06:22PM +0100, Linus Walleij wrote:
> > > > On Fri, Feb 20, 2015 at 8:17 PM, David Cohen
> > > > <david.a.cohen@linux.intel.com> wrote:
> > > > > On Fri, Feb 20, 2015 at 10:53:44AM +0100, Linus Walleij wrote:
> > > > 
> > > > >> I would put this adjacent to the phy driver somewhere in drivers/usb/*
> > > > >> and make the actual USB-driver thing handle its GPIOs directly.
> > > > >> But I guess David and Felipe have already discussed that as we're
> > > > >> seeing this patch?
> > > > >
> > > > > - The mux functions would be controlled by a possible new pinctrl-gpio
> > > > > driver (Linus, your input here would be nice :)
> > > > 
> > > > I don't understand what this means, does it mean a pin control function
> > > > somewhere else controlled by a GPIO pin?
> > > > 
> > > > Or do you mean a new combined pin control and GPIO driver (we have
> > > > plenty of these).
> > > > 
> > > > If you elaborate on what you need to do in that driver I might
> > > > understand it better.
> > 
> > This is a "virtual" ACPI device. Long story short we've 3 GPIOs
> > controlling the state of an USB OTG port. Neither of the hw components
> > controlled by these GPIOs are connected to a bus.
> > The ACPI table was implemented in such way that it considers this USB
> > port as a single "device" giving all 3 GPIOs to it.
> > 
> > When upstreaming this driver, the feedback I got is to split this driver
> > into simpler and more generic ones. FWIW ACPI tables are constructed
> > considering a valid Linux API during its implementation and are quite
> > difficult to change after product is released. The solution would be to
> > use MFD subsystem: this driver would create simpler children platform
> > devices.
> > 
> > But at least on ACPI case, GPIO API blocks the ability to create
> > children platform devices that require GPIO as platform data, despite
> > we've many drivers on upstream that expect this behavior: e.g. extcon-gpio.c
> > 
> > Are we considering those driver deprecated from the GPIO point of view?
> > If yes, we need to provide an update for them (we can't let upstreamed
> > drivers broken). If no, we need to provide a way to move forward the
> > GPIO to children devices.
> > 
> > BR, David
> > 
> > > 
> > > there's a discrete mux (not something integrated in the SoC) whose
> > > select signal is tied to a GPIO (in some cases, more than one, but
> > > usually people use 2-state muxes).
> > > 
> > > -- 
> > > balbi
> > 
> > 

Re: [PATCH v2] extcon: otg_gpio: add driver for USB OTG port controlled by GPIO(s)

[Linus Walleij]

On Mon, Mar 9, 2015 at 8:10 PM, David Cohen
<david.a.cohen@linux.intel.com> wrote:

> But at least on ACPI case, GPIO API blocks the ability to create
> children platform devices that require GPIO as platform data, despite
> we've many drivers on upstream that expect this behavior: e.g. extcon-gpio.c
>
> Are we considering those driver deprecated from the GPIO point of view?
> If yes, we need to provide an update for them (we can't let upstreamed
> drivers broken). If no, we need to provide a way to move forward the
> GPIO to children devices.

Aha OK... Let me paste this code from
arch/arm/mach-integrator/impd1.c - this is not an MFD (it would
be if we implemented it today) but you can see the idea I deploy:

    for (i = 0; i < ARRAY_SIZE(impd1_devs); i++) {
        struct impd1_device *idev = impd1_devs + i;
        struct amba_device *d;
        unsigned long pc_base;
        char devname[32];
        int irq1 = idev->irq[0];
        int irq2 = idev->irq[1];

        /* Translate IRQs to IM-PD1 local numberspace */
        if (irq1)
            irq1 += irq_base;
        if (irq2)
            irq2 += irq_base;

        pc_base = dev->resource.start + idev->offset;
        snprintf(devname, 32, "lm%x:%5.5lx", dev->id, idev->offset >> 12);

        /* Add GPIO descriptor lookup table for the PL061 block */
        if (idev->offset == 0x00400000) {
            struct gpiod_lookup_table *lookup;
            char *chipname;
            char *mmciname;

            lookup = devm_kzalloc(&dev->dev,
                          sizeof(*lookup) + 3 * sizeof(struct gpiod_lookup),
                          GFP_KERNEL);
            chipname = devm_kstrdup(&dev->dev, devname, GFP_KERNEL);
            mmciname = kasprintf(GFP_KERNEL, "lm%x:00700", dev->id);
            lookup->dev_id = mmciname;
            /*
             * Offsets on GPIO block 1:
             * 3 = MMC WP (write protect)
             * 4 = MMC CD (card detect)
             *
             * Offsets on GPIO block 2:
             * 0 = Up key
             * 1 = Down key
             * 2 = Left key
             * 3 = Right key
             * 4 = Key lower left
             * 5 = Key lower right
             */
            /* We need the two MMCI GPIO entries */
            lookup->table[0].chip_label = chipname;
            lookup->table[0].chip_hwnum = 3;
            lookup->table[0].con_id = "wp";
            lookup->table[1].chip_label = chipname;
            lookup->table[1].chip_hwnum = 4;
            lookup->table[1].con_id = "cd";
            lookup->table[1].flags = GPIO_ACTIVE_LOW;
            gpiod_add_lookup_table(lookup);
        }

        d = amba_ahb_device_add_res(&dev->dev, devname, pc_base, SZ_4K,
                        irq1, irq2,
                        idev->platform_data, idev->id,
                        &dev->resource);
        if (IS_ERR(d)) {
            dev_err(&dev->dev, "unable to register device: %ld\n", PTR_ERR(d));
            continue;
        }
    }

Notice how I build a dynamic lookup table.

I don't see any problem for an MFD nexus doing this too, just need
some quirks I guess.

But I guess it's not that simple, i.e. the GPIOs are not just
"appearing" in the system like here (the devices we add has a GPIO
block going with it) but it's external GPIOs.

So I presume we are talking about the case of GPIO forwarding that
was mentioned earlier, so that GPIOs are defined for the nexus
(MFD) device and need to be forwarded to the clients so they can
issue gpiod_get() on their own struct device*?


Yours,
Linus Walleij