Re: [PATCH] extcon: 3gpio: add driver for USB OTG port controlled by 3 GPIOs
From: Hans de Goede
Date: Mon Dec 19 2016 - 03:57:41 EST
Hi,
On 19-12-16 09:00, Chanwoo Choi wrote:
Hi Hans,
I'm glad to post new extcon driver. But, we need to discuss the device name of "3gpio".
I think that "3 GPIO" is ambiguous. You need to find more proper name. For example, extcon-qcom-spmi-misc.c uses one interrupt line to detect "USB_HOST". This driver name means the "Qualcomm USB extcon support".
Ok, so this driver is for the INT3496 Intel ACPI device,
so I think we should put intel in the name, I see a 2 options:
1) extcon-intel-3gpio-otg.c
2) extcon-intel-INT3496.c
Which one do you like best ?
Also, I recommend that you make the documentation for this driver.
On 2016ë 12ì 19ì 09:12, Hans de Goede wrote:
From: David Cohen <david.a.cohen@xxxxxxxxx>
Add an exntcon driver to for USB OTG ports controlled by 3 GPIOs used on
Intel Baytrail and Cherrytrail tablets.
Signed-off-by: David Cohen <david.a.cohen@xxxxxxxxx>
[hdgoede@xxxxxxxxxx: Port to current kernel, submit upstream]
Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
---
drivers/extcon/Kconfig | 7 ++
drivers/extcon/Makefile | 1 +
drivers/extcon/extcon-3gpio_otg.c | 201 ++++++++++++++++++++++++++++++++++++++
3 files changed, 209 insertions(+)
create mode 100644 drivers/extcon/extcon-3gpio_otg.c
diff --git a/drivers/extcon/Kconfig b/drivers/extcon/Kconfig
index 04788d9..1aaa64f 100644
--- a/drivers/extcon/Kconfig
+++ b/drivers/extcon/Kconfig
@@ -14,6 +14,13 @@ if EXTCON
comment "Extcon Device Drivers"
+config EXTCON_3GPIO_OTG
+ tristate "3 GPIO USB OTG extcon driver"
+ depends on GPIOLIB || COMPILE_TEST
+ help
+ Say Y here to enable extcon support for USB OTG ports controlled
+ by 3 GPIOs used on Intel Baytrail and Cherrytrail tablets.
+
config EXTCON_ADC_JACK
tristate "ADC Jack extcon support"
depends on IIO
diff --git a/drivers/extcon/Makefile b/drivers/extcon/Makefile
index 31a0a99..aa646fd 100644
--- a/drivers/extcon/Makefile
+++ b/drivers/extcon/Makefile
@@ -4,6 +4,7 @@
obj-$(CONFIG_EXTCON) += extcon-core.o
extcon-core-objs += extcon.o devres.o
+obj-$(CONFIG_EXTCON_3GPIO_OTG) += extcon-3gpio_otg.o
obj-$(CONFIG_EXTCON_ADC_JACK) += extcon-adc-jack.o
obj-$(CONFIG_EXTCON_ARIZONA) += extcon-arizona.o
obj-$(CONFIG_EXTCON_AXP288) += extcon-axp288.o
diff --git a/drivers/extcon/extcon-3gpio_otg.c b/drivers/extcon/extcon-3gpio_otg.c
new file mode 100644
index 0000000..fc8b14d1
--- /dev/null
+++ b/drivers/extcon/extcon-3gpio_otg.c
@@ -0,0 +1,201 @@
+/*
+ * 3 GPIO USB OTG extcon driver
+ *
+ * Copyright (c) 2016) Hans de Goede <hdegoede@xxxxxxxxxx>
s/2016) -> 2016
Ok.
+ *
+ * Based on android x86 kernel code which is:
+ *
+ * Copyright (c) 2014, Intel Corporation.
+ * Author: David Cohen <david.a.cohen@xxxxxxxxxxxxxxx>
+ *
+ * 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"
This definition is not necessary. It is used only one time in this driver. Also, this name should reflect specific hardware.
Ok.
+#define USB_OTG_GPIO_USB_ID 0
+#define USB_OTG_GPIO_VBUS_EN 1
+#define USB_OTG_GPIO_USB_MUX 2
+#define DEBOUNCE_TIME msecs_to_jiffies(50)
I recommend that you use the gpiod_set_debounce() to get the debounce time. If there is any property for debounce, you use the default debounce time of DEBOUNCE_TIME.
This driver is for Intel SoCs only, and Intel SoC's gpios
do not support gpiod_set_debounce(), so calling it only
to find out it returns -ENOTSUPP and then still doing
debounce ourselves does not seem useful.
+
+struct usb_otg {
+ struct device *dev;
+ struct extcon_dev *edev;
+ struct delayed_work work;
+ struct gpio_desc *gpio_usb_id;
+ struct gpio_desc *gpio_vbus_en;
+ struct gpio_desc *gpio_usb_mux;
+ int usb_id_irq;
+};
+
+static const unsigned int usb_otg_cable[] = {
+ EXTCON_USB_HOST,
+ EXTCON_NONE,
+};
+
+/*
+ * 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 usb_otg_set_port(struct usb_otg *otg, int id)
+{
+ int mux_val = id;
+ int vbus_val = !id;
+
+ if (!IS_ERR(otg->gpio_usb_mux))
+ gpiod_direction_output(otg->gpio_usb_mux, mux_val);
+
+ if (!IS_ERR(otg->gpio_vbus_en))
+ gpiod_direction_output(otg->gpio_vbus_en, vbus_val);
+}
+
+static void usb_otg_do_usb_id(struct work_struct *work)
+{
+ struct usb_otg *otg =
+ container_of(work, struct usb_otg, work.work);
+ int id = gpiod_get_value_cansleep(otg->gpio_usb_id);
+
+ dev_info(otg->dev, "USB PORT ID: %s\n", id ? "PERIPHERAL" : "HOST");
I don't prefer to use the dev_info() on the fly. And I recommend that you modify the debug message which include more correct information.
It is just example. Not forced.
dev_dbg(otg->dev, "Connected device is %s\n", id ? "PERIPHERAL" : "HOST");
Ok.
+
+ /*
+ * id == 1: PERIPHERAL
+ * id == 0: HOST
+ */
+ usb_otg_set_port(otg, id);
This function is used only one time in this driver and it is not complex. You don't need to make the separate function.
Ok.
+
+ /*
+ * id == 0: HOST connected
+ * id == 1: Host disconnected
+ */
+ extcon_set_state_sync(otg->edev, EXTCON_USB_HOST, !id);
+}
+
+static irqreturn_t usb_otg_thread_isr(int irq, void *priv)
+{
+ struct usb_otg *otg = priv;
+
+ /* id changed, let the pin settle and then process it */
I think "id changed" comment is unneeded.
The comment is there because of the "let the pin settle" bit,
it explains why DEBOUNCE_TIME is used.
+ mod_delayed_work(system_wq, &otg->work, DEBOUNCE_TIME);
+
+ return IRQ_HANDLED;
+}
+
+static int usb_otg_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct usb_otg *otg;
+ int ret;
+
+ otg = devm_kzalloc(dev, sizeof(*otg), GFP_KERNEL);
+ if (!otg)
+ return -ENOMEM;
+
+ otg->dev = dev;
+ INIT_DELAYED_WORK(&otg->work, usb_otg_do_usb_id);
+
+ otg->gpio_usb_id = devm_gpiod_get_index(dev, "id",
+ USB_OTG_GPIO_USB_ID,
+ GPIOD_IN);
+ if (IS_ERR(otg->gpio_usb_id)) {
+ dev_err(dev, "can't request USB ID GPIO: ret = %ld\n",
+ PTR_ERR(otg->gpio_usb_id));
+ return PTR_ERR(otg->gpio_usb_id);
I prefer to use the consistent error message. This patch uses two style sentence when error happen as following: I recommend that you use the one style.
- "can't ..."
- "failed to ..."
how about changing the print style of error message as following? Because other error message print just error value in this driver.
- ": ret = %ld" -> ": %d"
Ok, will fix.
+ }
+
+ otg->usb_id_irq = gpiod_to_irq(otg->gpio_usb_id);
+ if (otg->usb_id_irq <= 0) {
+ dev_err(dev, "invalid USB ID IRQ: %d\n", otg->usb_id_irq);
ditto.
+ return -EINVAL;
+ }
+
+ otg->gpio_vbus_en = devm_gpiod_get_index(dev, "vbus en",
+ USB_OTG_GPIO_VBUS_EN,
+ GPIOD_ASIS);
+ if (IS_ERR(otg->gpio_vbus_en))
+ dev_info(dev, "can't request VBUS EN GPIO, skipping it.\n");
I think that "skipping it." is unneeded.
Ok.
+
+ otg->gpio_usb_mux = devm_gpiod_get_index(dev, "usb mux",
+ USB_OTG_GPIO_USB_MUX,
+ GPIOD_ASIS);
+ if (IS_ERR(otg->gpio_usb_mux))
+ dev_info(dev, "can't request USB MUX, skipping it.\n");
I think that "skipping it." is unneeded.
+
+ /* register extcon device */
+ otg->edev = devm_extcon_dev_allocate(dev, usb_otg_cable);
+ if (IS_ERR(otg->edev)) {
+ dev_err(dev, "failed to allocate extcon device\n");
ditto.
+ return -ENOMEM;
+ }
+
+ ret = devm_extcon_dev_register(dev, otg->edev);
+ if (ret < 0) {
+ dev_err(dev, "failed to register extcon device: %d\n",
+ ret);
ditto.
+ return ret;
+ }
+
+ ret = devm_request_threaded_irq(dev, otg->usb_id_irq,
+ NULL, usb_otg_thread_isr,
+ IRQF_SHARED | IRQF_ONESHOT |
+ IRQF_TRIGGER_RISING |
+ IRQF_TRIGGER_FALLING,
+ dev_name(dev), otg);
+ if (ret < 0) {
+ dev_err(dev, "can't request IRQ for USB ID GPIO: %d\n", ret);
ditto.
+ return ret;
+ }
+
+ /* queue initial processing of id-pin */
+ queue_delayed_work(system_wq, &otg->work, 0);
+
+ platform_set_drvdata(pdev, otg);
+
+ return 0;
+}
+
+static int usb_otg_remove(struct platform_device *pdev)
+{
+ struct usb_otg *otg = platform_get_drvdata(pdev);
+
+ devm_free_irq(&pdev->dev, otg->usb_id_irq, otg);
As I knew, you don't need to free irq when using devm_request_threaded_irq() function.
Correct, but we need to free it before cancelling the work, otherwise
the irq may trigger between us cancelling the work and the devm code
disabling the irq, re-queueing the work while the struct work
now sits in free-ed memory and bad things happen.
+ cancel_delayed_work_sync(&otg->work);
+
+ return 0;
+}
+
+static struct acpi_device_id usb_otg_acpi_match[] = {
+ { "INT3496" },
What is meaning of "INT3496"?
It is the ACPI hardware-id for the ACPI device representing
the presence of an otg connector which should be handled by
this driver. The INT stands for Intel, the rest is just like
say a pci device-id, so a vendor defined number.
+ { }
+};
+MODULE_DEVICE_TABLE(acpi, usb_otg_acpi_match);
+
+static struct platform_driver usb_otg_driver = {
+ .driver = {
+ .name = DRV_NAME,
ditto. You can set the driver name directly without separate definition.
Ok.
+ .owner = THIS_MODULE,
+ .acpi_match_table = ACPI_PTR(usb_otg_acpi_match),
+ },
+ .probe = usb_otg_probe,
+ .remove = usb_otg_remove,
+};
+
+module_platform_driver(usb_otg_driver);
+
+MODULE_AUTHOR("Hans de Goede <hdegoede@xxxxxxxxxx>");
+MODULE_DESCRIPTION("3 GPIO USB OTG extcon driver");
+MODULE_LICENSE("GPL");
Thank you for the review, I will send a v2 when we've decided on a better
name for the driver.
Regards,
Hans