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". Also, I recommend that you make the documentation for this driver. On 2016년 12월 19일 09:12, Hans de Goede wrote: > From: David Cohen <[email protected]> > > 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 <[email protected]> > [[email protected]: Port to current kernel, submit upstream] > Signed-off-by: Hans de Goede <[email protected]> > --- > 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 <[email protected]> s/2016) -> 2016 > + * > + * Based on android x86 kernel code which is: > + * > + * Copyright (c) 2014, Intel Corporation. > + * Author: David Cohen <[email protected]> > + * > + * 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. > +#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. > + > +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"); > + > + /* > + * 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. > + > + /* > + * 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. > + 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" > + } > + > + 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. > + > + 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. > + cancel_delayed_work_sync(&otg->work); > + > + return 0; > +} > + > +static struct acpi_device_id usb_otg_acpi_match[] = { > + { "INT3496" }, What is meaning of "INT3496"? > + { } > +}; > +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. > + .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 <[email protected]>"); > +MODULE_DESCRIPTION("3 GPIO USB OTG extcon driver"); > +MODULE_LICENSE("GPL"); > -- Regards, Chanwoo Choi
