Hi, On 2016년 12월 19일 17:57, Hans de Goede wrote: > 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 ?
I prefer "2) extcon-intel-INT3496.c". But, should you use the captical letter for "INT..."? Usually, the device name uses the small letter. > > >> >> 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.co...@intel.com> >>> >>> 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.co...@intel.com> >>> [hdgo...@redhat.com: Port to current kernel, submit upstream] >>> Signed-off-by: Hans de Goede <hdego...@redhat.com> >>> --- >>> 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 <hdego...@redhat.com> >> >> s/2016) -> 2016 > > Ok. > >> >>> + * >>> + * Based on android x86 kernel code which is: >>> + * >>> + * Copyright (c) 2014, Intel Corporation. >>> + * Author: David Cohen <david.a.co...@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" >> >> 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. OK. > >>> + >>> +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. OK. If so, could you use the more formal sentence instead of "id changed"? > >>> + 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. OK. > >>> + 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. Thanks for explanation. > >> >>> + { } >>> +}; >>> +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 <hdego...@redhat.com>"); >>> +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 > > > -- Regards, Chanwoo Choi