* Re: [PATCH] extcon: 3gpio: add driver for USB OTG port controlled by 3 GPIOs
2016-12-19 0:12 ` Hans de Goede
@ 2016-12-19 2:01 kbuild test robot
2016-12-19 0:12 ` Hans de Goede
3 siblings, 1 reply; 8+ messages in thread
From: kbuild test robot @ 2016-12-19 2:01 UTC (permalink / raw)
To: Hans de Goede
Cc: kbuild-all, MyungJoo Ham, Chanwoo Choi, linux-kernel,
David Cohen, Hans de Goede
Hi David,
[auto build test WARNING on chanwoo-extcon/extcon-next]
[also build test WARNING on v4.9 next-20161216]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Hans-de-Goede/extcon-3gpio-add-driver-for-USB-OTG-port-controlled-by-3-GPIOs/20161219-082834
base: https://git.kernel.org/pub/scm/linux/kernel/git/chanwoo/extcon.git extcon-next
coccinelle warnings: (new ones prefixed by >>)
>> drivers/extcon/extcon-3gpio_otg.c:190:3-8: No need to set .owner here. The core will do it.
Please review and possibly fold the followup patch.
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
^ permalink raw reply [flat|nested] 8+ messages in thread* [PATCH] extcon: 3gpio: add driver for USB OTG port controlled by 3 GPIOs @ 2016-12-19 0:12 ` Hans de Goede 2016-12-19 1:57 ` kbuild test robot ` (3 more replies) 0 siblings, 4 replies; 8+ messages in thread From: Hans de Goede @ 2016-12-19 0:12 UTC (permalink / raw) To: MyungJoo Ham, Chanwoo Choi; +Cc: linux-kernel, David Cohen, Hans de Goede From: David Cohen <david.a.cohen@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.cohen@intel.com> [hdgoede@redhat.com: Port to current kernel, submit upstream] Signed-off-by: Hans de Goede <hdegoede@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 <hdegoede@redhat.com> + * + * Based on android x86 kernel code which is: + * + * Copyright (c) 2014, Intel Corporation. + * Author: David Cohen <david.a.cohen@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" +#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) + +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"); + + /* + * id == 1: PERIPHERAL + * id == 0: HOST + */ + usb_otg_set_port(otg, id); + + /* + * 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 */ + 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); + } + + 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); + 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"); + + 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"); + + /* 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"); + return -ENOMEM; + } + + ret = devm_extcon_dev_register(dev, otg->edev); + if (ret < 0) { + dev_err(dev, "failed to register extcon device: %d\n", + ret); + 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); + 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); + cancel_delayed_work_sync(&otg->work); + + return 0; +} + +static struct acpi_device_id usb_otg_acpi_match[] = { + { "INT3496" }, + { } +}; +MODULE_DEVICE_TABLE(acpi, usb_otg_acpi_match); + +static struct platform_driver usb_otg_driver = { + .driver = { + .name = DRV_NAME, + .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@redhat.com>"); +MODULE_DESCRIPTION("3 GPIO USB OTG extcon driver"); +MODULE_LICENSE("GPL"); -- 2.9.3 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] extcon: 3gpio: add driver for USB OTG port controlled by 3 GPIOs 2016-12-19 0:12 ` Hans de Goede @ 2016-12-19 1:57 ` kbuild test robot 2016-12-19 2:01 ` [PATCH] extcon: 3gpio: fix platform_no_drv_owner.cocci warnings kbuild test robot ` (2 subsequent siblings) 3 siblings, 0 replies; 8+ messages in thread From: kbuild test robot @ 2016-12-19 1:57 UTC (permalink / raw) To: Hans de Goede Cc: kbuild-all, MyungJoo Ham, Chanwoo Choi, linux-kernel, David Cohen, Hans de Goede [-- Attachment #1: Type: text/plain, Size: 9809 bytes --] Hi David, [auto build test WARNING on chanwoo-extcon/extcon-next] [also build test WARNING on v4.9 next-20161216] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Hans-de-Goede/extcon-3gpio-add-driver-for-USB-OTG-port-controlled-by-3-GPIOs/20161219-082834 base: https://git.kernel.org/pub/scm/linux/kernel/git/chanwoo/extcon.git extcon-next config: alpha-allyesconfig (attached as .config) compiler: alpha-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705 reproduce: wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree make.cross ARCH=alpha All warnings (new ones prefixed by >>): drivers/extcon/extcon-3gpio_otg.c:185:1: warning: data definition has no type or storage class MODULE_DEVICE_TABLE(acpi, usb_otg_acpi_match); ^~~~~~~~~~~~~~~~~~~ drivers/extcon/extcon-3gpio_otg.c:185:1: error: type defaults to 'int' in declaration of 'MODULE_DEVICE_TABLE' [-Werror=implicit-int] drivers/extcon/extcon-3gpio_otg.c:185:1: warning: parameter names (without types) in function declaration In file included from include/linux/acpi.h:27:0, from drivers/extcon/extcon-3gpio_otg.c:21: include/linux/device.h:1353:1: warning: data definition has no type or storage class module_init(__driver##_init); \ ^ include/linux/platform_device.h:228:2: note: in expansion of macro 'module_driver' module_driver(__platform_driver, platform_driver_register, \ ^~~~~~~~~~~~~ drivers/extcon/extcon-3gpio_otg.c:197:1: note: in expansion of macro 'module_platform_driver' module_platform_driver(usb_otg_driver); ^~~~~~~~~~~~~~~~~~~~~~ include/linux/device.h:1353:1: error: type defaults to 'int' in declaration of 'module_init' [-Werror=implicit-int] module_init(__driver##_init); \ ^ include/linux/platform_device.h:228:2: note: in expansion of macro 'module_driver' module_driver(__platform_driver, platform_driver_register, \ ^~~~~~~~~~~~~ drivers/extcon/extcon-3gpio_otg.c:197:1: note: in expansion of macro 'module_platform_driver' module_platform_driver(usb_otg_driver); ^~~~~~~~~~~~~~~~~~~~~~ In file included from include/linux/linkage.h:6:0, from include/linux/kernel.h:6, from include/linux/list.h:8, from include/linux/resource_ext.h:17, from include/linux/acpi.h:26, from drivers/extcon/extcon-3gpio_otg.c:21: >> include/linux/export.h:37:30: warning: parameter names (without types) in function declaration #define THIS_MODULE ((struct module *)0) ^ >> include/linux/platform_device.h:198:34: note: in expansion of macro 'THIS_MODULE' __platform_driver_register(drv, THIS_MODULE) ^~~~~~~~~~~ >> include/linux/device.h:1351:9: note: in expansion of macro 'platform_driver_register' return __register(&(__driver) , ##__VA_ARGS__); \ ^~~~~~~~~~ include/linux/platform_device.h:228:2: note: in expansion of macro 'module_driver' module_driver(__platform_driver, platform_driver_register, \ ^~~~~~~~~~~~~ drivers/extcon/extcon-3gpio_otg.c:197:1: note: in expansion of macro 'module_platform_driver' module_platform_driver(usb_otg_driver); ^~~~~~~~~~~~~~~~~~~~~~ In file included from include/linux/acpi.h:27:0, from drivers/extcon/extcon-3gpio_otg.c:21: include/linux/device.h:1358:1: warning: data definition has no type or storage class module_exit(__driver##_exit); ^ include/linux/platform_device.h:228:2: note: in expansion of macro 'module_driver' module_driver(__platform_driver, platform_driver_register, \ ^~~~~~~~~~~~~ drivers/extcon/extcon-3gpio_otg.c:197:1: note: in expansion of macro 'module_platform_driver' module_platform_driver(usb_otg_driver); ^~~~~~~~~~~~~~~~~~~~~~ include/linux/device.h:1358:1: error: type defaults to 'int' in declaration of 'module_exit' [-Werror=implicit-int] module_exit(__driver##_exit); ^ include/linux/platform_device.h:228:2: note: in expansion of macro 'module_driver' module_driver(__platform_driver, platform_driver_register, \ ^~~~~~~~~~~~~ drivers/extcon/extcon-3gpio_otg.c:197:1: note: in expansion of macro 'module_platform_driver' module_platform_driver(usb_otg_driver); ^~~~~~~~~~~~~~~~~~~~~~ In file included from include/linux/linkage.h:6:0, from include/linux/kernel.h:6, from include/linux/list.h:8, from include/linux/resource_ext.h:17, from include/linux/acpi.h:26, from drivers/extcon/extcon-3gpio_otg.c:21: >> include/linux/export.h:37:30: warning: parameter names (without types) in function declaration #define THIS_MODULE ((struct module *)0) ^ >> include/linux/platform_device.h:198:34: note: in expansion of macro 'THIS_MODULE' __platform_driver_register(drv, THIS_MODULE) ^~~~~~~~~~~ >> include/linux/device.h:1351:9: note: in expansion of macro 'platform_driver_register' return __register(&(__driver) , ##__VA_ARGS__); \ ^~~~~~~~~~ include/linux/platform_device.h:228:2: note: in expansion of macro 'module_driver' module_driver(__platform_driver, platform_driver_register, \ ^~~~~~~~~~~~~ drivers/extcon/extcon-3gpio_otg.c:197:1: note: in expansion of macro 'module_platform_driver' module_platform_driver(usb_otg_driver); ^~~~~~~~~~~~~~~~~~~~~~ drivers/extcon/extcon-3gpio_otg.c:199:15: error: expected declaration specifiers or '...' before string constant MODULE_AUTHOR("Hans de Goede <hdegoede@redhat.com>"); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ drivers/extcon/extcon-3gpio_otg.c:200:20: error: expected declaration specifiers or '...' before string constant MODULE_DESCRIPTION("3 GPIO USB OTG extcon driver"); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ drivers/extcon/extcon-3gpio_otg.c:201:16: error: expected declaration specifiers or '...' before string constant MODULE_LICENSE("GPL"); ^~~~~ In file included from include/linux/acpi.h:27:0, from drivers/extcon/extcon-3gpio_otg.c:21: drivers/extcon/extcon-3gpio_otg.c:197:24: warning: 'usb_otg_driver_init' defined but not used [-Wunused-function] module_platform_driver(usb_otg_driver); ^ include/linux/device.h:1349:19: note: in definition of macro 'module_driver' static int __init __driver##_init(void) \ ^~~~~~~~ drivers/extcon/extcon-3gpio_otg.c:197:1: note: in expansion of macro 'module_platform_driver' module_platform_driver(usb_otg_driver); ^~~~~~~~~~~~~~~~~~~~~~ drivers/extcon/extcon-3gpio_otg.c:181:30: warning: 'usb_otg_acpi_match' defined but not used [-Wunused-variable] static struct acpi_device_id usb_otg_acpi_match[] = { ^~~~~~~~~~~~~~~~~~ cc1: some warnings being treated as errors vim +/THIS_MODULE +198 include/linux/platform_device.h 00d3dcdd Russell King 2005-11-09 182 int (*remove)(struct platform_device *); 00d3dcdd Russell King 2005-11-09 183 void (*shutdown)(struct platform_device *); 00d3dcdd Russell King 2005-11-09 184 int (*suspend)(struct platform_device *, pm_message_t state); 00d3dcdd Russell King 2005-11-09 185 int (*resume)(struct platform_device *); 00d3dcdd Russell King 2005-11-09 186 struct device_driver driver; 831fad2f Uwe Kleine-König 2010-01-26 187 const struct platform_device_id *id_table; 3f9120b0 Johan Hovold 2013-09-23 188 bool prevent_deferred_probe; 00d3dcdd Russell King 2005-11-09 189 }; 00d3dcdd Russell King 2005-11-09 190 10dbc5e3 Rob Herring 2013-04-21 191 #define to_platform_driver(drv) (container_of((drv), struct platform_driver, \ 10dbc5e3 Rob Herring 2013-04-21 192 driver)) 10dbc5e3 Rob Herring 2013-04-21 193 9447057e Libo Chen 2013-05-25 194 /* 9447057e Libo Chen 2013-05-25 195 * use a macro to avoid include chaining to get THIS_MODULE 9447057e Libo Chen 2013-05-25 196 */ 9447057e Libo Chen 2013-05-25 197 #define platform_driver_register(drv) \ 9447057e Libo Chen 2013-05-25 @198 __platform_driver_register(drv, THIS_MODULE) 9447057e Libo Chen 2013-05-25 199 extern int __platform_driver_register(struct platform_driver *, 9447057e Libo Chen 2013-05-25 200 struct module *); 00d3dcdd Russell King 2005-11-09 201 extern void platform_driver_unregister(struct platform_driver *); 00d3dcdd Russell King 2005-11-09 202 c67334fb David Brownell 2006-11-16 203 /* non-hotpluggable platform devices may use this so that probe() and c67334fb David Brownell 2006-11-16 204 * its support may live in __init sections, conserving runtime memory. c67334fb David Brownell 2006-11-16 205 */ c3b50dc2 Wolfram Sang 2014-10-28 206 #define platform_driver_probe(drv, probe) \ :::::: The code at line 198 was first introduced by commit :::::: 9447057eaff871dd7c63c808de761b8732407169 platform_device: use a macro instead of platform_driver_register :::::: TO: Libo Chen <clbchenlibo.chen@huawei.com> :::::: CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org> --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation [-- Attachment #2: .config.gz --] [-- Type: application/gzip, Size: 47891 bytes --] ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] extcon: 3gpio: fix platform_no_drv_owner.cocci warnings 2016-12-19 0:12 ` Hans de Goede 2016-12-19 1:57 ` kbuild test robot @ 2016-12-19 2:01 ` kbuild test robot 2016-12-19 2:34 ` [PATCH] extcon: 3gpio: add driver for USB OTG port controlled by 3 GPIOs kbuild test robot 2016-12-19 8:00 ` Chanwoo Choi 3 siblings, 0 replies; 8+ messages in thread From: kbuild test robot @ 2016-12-19 2:01 UTC (permalink / raw) To: Hans de Goede Cc: kbuild-all, MyungJoo Ham, Chanwoo Choi, linux-kernel, David Cohen, Hans de Goede drivers/extcon/extcon-3gpio_otg.c:190:3-8: No need to set .owner here. The core will do it. Remove .owner field if calls are used which set it automatically Generated by: scripts/coccinelle/api/platform_no_drv_owner.cocci CC: David Cohen <david.a.cohen@intel.com> Signed-off-by: Fengguang Wu <fengguang.wu@intel.com> --- extcon-3gpio_otg.c | 1 - 1 file changed, 1 deletion(-) --- a/drivers/extcon/extcon-3gpio_otg.c +++ b/drivers/extcon/extcon-3gpio_otg.c @@ -187,7 +187,6 @@ MODULE_DEVICE_TABLE(acpi, usb_otg_acpi_m static struct platform_driver usb_otg_driver = { .driver = { .name = DRV_NAME, - .owner = THIS_MODULE, .acpi_match_table = ACPI_PTR(usb_otg_acpi_match), }, .probe = usb_otg_probe, ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] extcon: 3gpio: add driver for USB OTG port controlled by 3 GPIOs 2016-12-19 0:12 ` Hans de Goede 2016-12-19 1:57 ` kbuild test robot 2016-12-19 2:01 ` [PATCH] extcon: 3gpio: fix platform_no_drv_owner.cocci warnings kbuild test robot @ 2016-12-19 2:34 ` kbuild test robot 2016-12-19 8:00 ` Chanwoo Choi 3 siblings, 0 replies; 8+ messages in thread From: kbuild test robot @ 2016-12-19 2:34 UTC (permalink / raw) To: Hans de Goede Cc: kbuild-all, MyungJoo Ham, Chanwoo Choi, linux-kernel, David Cohen, Hans de Goede [-- Attachment #1: Type: text/plain, Size: 6560 bytes --] Hi David, [auto build test ERROR on chanwoo-extcon/extcon-next] [also build test ERROR on v4.9 next-20161216] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Hans-de-Goede/extcon-3gpio-add-driver-for-USB-OTG-port-controlled-by-3-GPIOs/20161219-082834 base: https://git.kernel.org/pub/scm/linux/kernel/git/chanwoo/extcon.git extcon-next config: blackfin-allmodconfig (attached as .config) compiler: bfin-uclinux-gcc (GCC) 6.2.0 reproduce: wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree make.cross ARCH=blackfin All error/warnings (new ones prefixed by >>): >> drivers/extcon/extcon-3gpio_otg.c:185:1: warning: data definition has no type or storage class MODULE_DEVICE_TABLE(acpi, usb_otg_acpi_match); ^~~~~~~~~~~~~~~~~~~ >> drivers/extcon/extcon-3gpio_otg.c:185:1: error: type defaults to 'int' in declaration of 'MODULE_DEVICE_TABLE' [-Werror=implicit-int] >> drivers/extcon/extcon-3gpio_otg.c:185:1: warning: parameter names (without types) in function declaration In file included from include/linux/acpi.h:27:0, from drivers/extcon/extcon-3gpio_otg.c:21: include/linux/device.h:1353:1: warning: data definition has no type or storage class module_init(__driver##_init); \ ^ >> include/linux/platform_device.h:228:2: note: in expansion of macro 'module_driver' module_driver(__platform_driver, platform_driver_register, \ ^~~~~~~~~~~~~ >> drivers/extcon/extcon-3gpio_otg.c:197:1: note: in expansion of macro 'module_platform_driver' module_platform_driver(usb_otg_driver); ^~~~~~~~~~~~~~~~~~~~~~ >> include/linux/device.h:1353:1: error: type defaults to 'int' in declaration of 'module_init' [-Werror=implicit-int] module_init(__driver##_init); \ ^ >> include/linux/platform_device.h:228:2: note: in expansion of macro 'module_driver' module_driver(__platform_driver, platform_driver_register, \ ^~~~~~~~~~~~~ >> drivers/extcon/extcon-3gpio_otg.c:197:1: note: in expansion of macro 'module_platform_driver' module_platform_driver(usb_otg_driver); ^~~~~~~~~~~~~~~~~~~~~~ drivers/extcon/extcon-3gpio_otg.c:197:1: warning: parameter names (without types) in function declaration In file included from include/linux/acpi.h:27:0, from drivers/extcon/extcon-3gpio_otg.c:21: include/linux/device.h:1358:1: warning: data definition has no type or storage class module_exit(__driver##_exit); ^ >> include/linux/platform_device.h:228:2: note: in expansion of macro 'module_driver' module_driver(__platform_driver, platform_driver_register, \ ^~~~~~~~~~~~~ >> drivers/extcon/extcon-3gpio_otg.c:197:1: note: in expansion of macro 'module_platform_driver' module_platform_driver(usb_otg_driver); ^~~~~~~~~~~~~~~~~~~~~~ >> include/linux/device.h:1358:1: error: type defaults to 'int' in declaration of 'module_exit' [-Werror=implicit-int] module_exit(__driver##_exit); ^ >> include/linux/platform_device.h:228:2: note: in expansion of macro 'module_driver' module_driver(__platform_driver, platform_driver_register, \ ^~~~~~~~~~~~~ >> drivers/extcon/extcon-3gpio_otg.c:197:1: note: in expansion of macro 'module_platform_driver' module_platform_driver(usb_otg_driver); ^~~~~~~~~~~~~~~~~~~~~~ drivers/extcon/extcon-3gpio_otg.c:197:1: warning: parameter names (without types) in function declaration >> drivers/extcon/extcon-3gpio_otg.c:199:15: error: expected declaration specifiers or '...' before string constant MODULE_AUTHOR("Hans de Goede <hdegoede@redhat.com>"); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ drivers/extcon/extcon-3gpio_otg.c:200:20: error: expected declaration specifiers or '...' before string constant MODULE_DESCRIPTION("3 GPIO USB OTG extcon driver"); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ drivers/extcon/extcon-3gpio_otg.c:201:16: error: expected declaration specifiers or '...' before string constant MODULE_LICENSE("GPL"); ^~~~~ In file included from include/linux/acpi.h:27:0, from drivers/extcon/extcon-3gpio_otg.c:21: drivers/extcon/extcon-3gpio_otg.c:197:24: warning: 'usb_otg_driver_exit' defined but not used [-Wunused-function] module_platform_driver(usb_otg_driver); ^ include/linux/device.h:1354:20: note: in definition of macro 'module_driver' static void __exit __driver##_exit(void) \ ^~~~~~~~ >> drivers/extcon/extcon-3gpio_otg.c:197:1: note: in expansion of macro 'module_platform_driver' module_platform_driver(usb_otg_driver); ^~~~~~~~~~~~~~~~~~~~~~ drivers/extcon/extcon-3gpio_otg.c:197:24: warning: 'usb_otg_driver_init' defined but not used [-Wunused-function] module_platform_driver(usb_otg_driver); ^ include/linux/device.h:1349:19: note: in definition of macro 'module_driver' static int __init __driver##_init(void) \ ^~~~~~~~ >> drivers/extcon/extcon-3gpio_otg.c:197:1: note: in expansion of macro 'module_platform_driver' module_platform_driver(usb_otg_driver); ^~~~~~~~~~~~~~~~~~~~~~ drivers/extcon/extcon-3gpio_otg.c:181:30: warning: 'usb_otg_acpi_match' defined but not used [-Wunused-variable] static struct acpi_device_id usb_otg_acpi_match[] = { ^~~~~~~~~~~~~~~~~~ cc1: some warnings being treated as errors vim +185 drivers/extcon/extcon-3gpio_otg.c 179 } 180 181 static struct acpi_device_id usb_otg_acpi_match[] = { 182 { "INT3496" }, 183 { } 184 }; > 185 MODULE_DEVICE_TABLE(acpi, usb_otg_acpi_match); 186 187 static struct platform_driver usb_otg_driver = { 188 .driver = { 189 .name = DRV_NAME, 190 .owner = THIS_MODULE, 191 .acpi_match_table = ACPI_PTR(usb_otg_acpi_match), 192 }, 193 .probe = usb_otg_probe, 194 .remove = usb_otg_remove, 195 }; 196 > 197 module_platform_driver(usb_otg_driver); 198 > 199 MODULE_AUTHOR("Hans de Goede <hdegoede@redhat.com>"); 200 MODULE_DESCRIPTION("3 GPIO USB OTG extcon driver"); 201 MODULE_LICENSE("GPL"); --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation [-- Attachment #2: .config.gz --] [-- Type: application/gzip, Size: 41983 bytes --] ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] extcon: 3gpio: add driver for USB OTG port controlled by 3 GPIOs 2016-12-19 0:12 ` Hans de Goede ` (2 preceding siblings ...) 2016-12-19 2:34 ` [PATCH] extcon: 3gpio: add driver for USB OTG port controlled by 3 GPIOs kbuild test robot @ 2016-12-19 8:00 ` Chanwoo Choi 2016-12-19 8:57 ` Hans de Goede 3 siblings, 1 reply; 8+ messages in thread From: Chanwoo Choi @ 2016-12-19 8:00 UTC (permalink / raw) To: Hans de Goede, MyungJoo Ham; +Cc: linux-kernel, David Cohen 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 <david.a.cohen@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.cohen@intel.com> > [hdgoede@redhat.com: Port to current kernel, submit upstream] > Signed-off-by: Hans de Goede <hdegoede@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 <hdegoede@redhat.com> s/2016) -> 2016 > + * > + * Based on android x86 kernel code which is: > + * > + * Copyright (c) 2014, Intel Corporation. > + * Author: David Cohen <david.a.cohen@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. > +#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 <hdegoede@redhat.com>"); > +MODULE_DESCRIPTION("3 GPIO USB OTG extcon driver"); > +MODULE_LICENSE("GPL"); > -- Regards, Chanwoo Choi ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] extcon: 3gpio: add driver for USB OTG port controlled by 3 GPIOs 2016-12-19 8:00 ` Chanwoo Choi @ 2016-12-19 8:57 ` Hans de Goede 2016-12-19 9:41 ` Chanwoo Choi 0 siblings, 1 reply; 8+ messages in thread From: Hans de Goede @ 2016-12-19 8:57 UTC (permalink / raw) To: Chanwoo Choi, MyungJoo Ham; +Cc: linux-kernel, David Cohen 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@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.cohen@intel.com> >> [hdgoede@redhat.com: Port to current kernel, submit upstream] >> Signed-off-by: Hans de Goede <hdegoede@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 <hdegoede@redhat.com> > > s/2016) -> 2016 Ok. > >> + * >> + * Based on android x86 kernel code which is: >> + * >> + * Copyright (c) 2014, Intel Corporation. >> + * Author: David Cohen <david.a.cohen@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. >> + >> +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@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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] extcon: 3gpio: add driver for USB OTG port controlled by 3 GPIOs 2016-12-19 8:57 ` Hans de Goede @ 2016-12-19 9:41 ` Chanwoo Choi 0 siblings, 0 replies; 8+ messages in thread From: Chanwoo Choi @ 2016-12-19 9:41 UTC (permalink / raw) To: Hans de Goede, MyungJoo Ham; +Cc: linux-kernel, David Cohen 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.cohen@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.cohen@intel.com> >>> [hdgoede@redhat.com: Port to current kernel, submit upstream] >>> Signed-off-by: Hans de Goede <hdegoede@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 <hdegoede@redhat.com> >> >> s/2016) -> 2016 > > Ok. > >> >>> + * >>> + * Based on android x86 kernel code which is: >>> + * >>> + * Copyright (c) 2014, Intel Corporation. >>> + * Author: David Cohen <david.a.cohen@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 <hdegoede@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 ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2016-12-19 9:41 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-12-19 2:01 [PATCH] extcon: 3gpio: add driver for USB OTG port controlled by 3 GPIOs kbuild test robot 2016-12-19 0:12 ` Hans de Goede 2016-12-19 1:57 ` kbuild test robot 2016-12-19 2:01 ` [PATCH] extcon: 3gpio: fix platform_no_drv_owner.cocci warnings kbuild test robot 2016-12-19 2:34 ` [PATCH] extcon: 3gpio: add driver for USB OTG port controlled by 3 GPIOs kbuild test robot 2016-12-19 8:00 ` Chanwoo Choi 2016-12-19 8:57 ` Hans de Goede 2016-12-19 9:41 ` Chanwoo Choi
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).