From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755738AbcLSJlf (ORCPT ); Mon, 19 Dec 2016 04:41:35 -0500 Received: from mailout2.samsung.com ([203.254.224.25]:58693 "EHLO mailout2.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753138AbcLSJlc (ORCPT ); Mon, 19 Dec 2016 04:41:32 -0500 MIME-version: 1.0 Content-type: text/plain; charset=utf-8 X-AuditID: cbfee61a-f79bd6d000000fc6-84-5857ab4a2873 Content-transfer-encoding: 8BIT Message-id: <5857AB49.80602@samsung.com> Date: Mon, 19 Dec 2016 18:41:29 +0900 From: Chanwoo Choi Organization: Samsung Electronics User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.6.0 To: Hans de Goede , MyungJoo Ham Cc: linux-kernel@vger.kernel.org, David Cohen Subject: Re: [PATCH] extcon: 3gpio: add driver for USB OTG port controlled by 3 GPIOs References: <20161219001220.13321-1-hdegoede@redhat.com> <585793A9.9070805@samsung.com> <5a268c86-7531-bc89-b472-b11fc0adf81f@redhat.com> In-reply-to: <5a268c86-7531-bc89-b472-b11fc0adf81f@redhat.com> X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFlrKIsWRmVeSWpSXmKPExsVy+t9jQV2v1eERBm8vMFuse+Fs8eb4dCaL y7vmsFncblzB5sDisXjPSyaP9/uusnn0bVnF6PF5k1wAS5SbTUZqYkpqkUJqXnJ+SmZeuq1S aIibroWSQl5ibqqtUoSub0iQkkJZYk4pkGdkgAYcnAPcg5X07RLcMr7ccClYl1yx58xepgbG Jd5djJwcEgImEi//vmCDsMUkLtxbD2YLCcxilPj+ww3E5hUQlPgx+R5LFyMHB7OAvMSRS9kQ prrElCm5XYxcQNUPGCV+3DvFBFGuIfFu3Wd2kBoWAVWJCfNDQcJsAloS+1/cAJvOL6AocfXH Y0aQElGBCInuE5UgYRGBAImfp/rZQWxmAXeJS8+XgZULC4RLbPl8ghli1UNGidWfDjOCJDgF 7CT2vNzMNIFRcBaSQ2chHDoL4dAFjMyrGCVSC5ILipPScw3zUsv1ihNzi0vz0vWS83M3MYKj 6JnUDsaDu9wPMQpwMCrx8Ba8D4sQYk0sK67MPcQowcGsJMJ7f1V4hBBvSmJlVWpRfnxRaU5q 8SFGU6BPJzJLiSbnAyM8ryTe0MTcxNzYwMLc0tLESEmct3H2s3AhgfTEktTs1NSC1CKYPiYO TqkGxnpe5USPfncRDeGJsq+2l5+eeFFVJiji76alnUvWGwtLmk98cNP+oG/m1OgLN7e8VpLm eb5X1n7r1ftO6tY71pzN2fvCs7xuQ5+l10+djpMn27p3/fe+Z3bnW8j+qC9vNwqcPBRRt8Yz nHv5GuYpplPd5z0sEdnVpZc7db/5IfZ783Mv1Rzc2KPEUpyRaKjFXFScCAC7i+eauAIAAA== X-MTR: 20000000000000000@CPGS Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 >>> >>> 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. 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 "); >>> +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