From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932224AbcLSI5h (ORCPT ); Mon, 19 Dec 2016 03:57:37 -0500 Received: from mx1.redhat.com ([209.132.183.28]:33924 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754886AbcLSI5g (ORCPT ); Mon, 19 Dec 2016 03:57:36 -0500 Subject: Re: [PATCH] extcon: 3gpio: add driver for USB OTG port controlled by 3 GPIOs To: Chanwoo Choi , MyungJoo Ham References: <20161219001220.13321-1-hdegoede@redhat.com> <585793A9.9070805@samsung.com> Cc: linux-kernel@vger.kernel.org, David Cohen From: Hans de Goede Message-ID: <5a268c86-7531-bc89-b472-b11fc0adf81f@redhat.com> Date: Mon, 19 Dec 2016 09:57:33 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.5.1 MIME-Version: 1.0 In-Reply-To: <585793A9.9070805@samsung.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.38]); Mon, 19 Dec 2016 08:57:35 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 >> >> 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 >> [hdgoede@redhat.com: Port to current kernel, submit upstream] >> Signed-off-by: Hans de Goede >> --- >> 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 > > s/2016) -> 2016 Ok. > >> + * >> + * Based on android x86 kernel code which is: >> + * >> + * Copyright (c) 2014, Intel Corporation. >> + * Author: David Cohen >> + * >> + * 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 >> +#include >> +#include >> +#include >> +#include >> + >> +#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 "); >> +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