Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1544252 > unrolled thread

[PATCH] extcon: 3gpio: add driver for USB OTG port controlled by 3 GPIOs

Started byHans de Goede <hdegoede@redhat.com>
First post2016-12-19 01:20 +0100
Last post2016-12-19 10:50 +0100
Articles 6 — 3 participants

Back to article view | Back to linux.kernel

Contents

  [PATCH] extcon: 3gpio: add driver for USB OTG port controlled by 3 GPIOs Hans de Goede <hdegoede@redhat.com> - 2016-12-19 01:20 +0100
    [PATCH] extcon: 3gpio: fix platform_no_drv_owner.cocci warnings kbuild test robot <lkp@intel.com> - 2016-12-19 03:10 +0100
    Re: [PATCH] extcon: 3gpio: add driver for USB OTG port controlled by  3 GPIOs kbuild test robot <lkp@intel.com> - 2016-12-19 03:10 +0100
    Re: [PATCH] extcon: 3gpio: add driver for USB OTG port controlled by 3  GPIOs Chanwoo Choi <cw00.choi@samsung.com> - 2016-12-19 09:10 +0100
      Re: [PATCH] extcon: 3gpio: add driver for USB OTG port controlled by  3 GPIOs Hans de Goede <hdegoede@redhat.com> - 2016-12-19 10:00 +0100
        Re: [PATCH] extcon: 3gpio: add driver for USB OTG port controlled by 3  GPIOs Chanwoo Choi <cw00.choi@samsung.com> - 2016-12-19 10:50 +0100

#1544252 — [PATCH] extcon: 3gpio: add driver for USB OTG port controlled by 3 GPIOs

FromHans de Goede <hdegoede@redhat.com>
Date2016-12-19 01:20 +0100
Subject[PATCH] extcon: 3gpio: add driver for USB OTG port controlled by 3 GPIOs
Message-ID<sPTQl-tj-19@gated-at.bofh.it>
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

[toc] | [next] | [standalone]

#1544275 — [PATCH] extcon: 3gpio: fix platform_no_drv_owner.cocci warnings

Fromkbuild test robot <lkp@intel.com>
Date2016-12-19 03:10 +0100
Subject[PATCH] extcon: 3gpio: fix platform_no_drv_owner.cocci warnings
Message-ID<sPVyN-1zj-7@gated-at.bofh.it>
In reply to#1544252
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,

[toc] | [prev] | [next] | [standalone]

#1544277 — Re: [PATCH] extcon: 3gpio: add driver for USB OTG port controlled by 3 GPIOs

Fromkbuild test robot <lkp@intel.com>
Date2016-12-19 03:10 +0100
SubjectRe: [PATCH] extcon: 3gpio: add driver for USB OTG port controlled by 3 GPIOs
Message-ID<sPVyN-1zj-9@gated-at.bofh.it>
In reply to#1544252
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

[toc] | [prev] | [next] | [standalone]

#1544354 — Re: [PATCH] extcon: 3gpio: add driver for USB OTG port controlled by 3 GPIOs

FromChanwoo Choi <cw00.choi@samsung.com>
Date2016-12-19 09:10 +0100
SubjectRe: [PATCH] extcon: 3gpio: add driver for USB OTG port controlled by 3 GPIOs
Message-ID<sQ1bc-5bq-7@gated-at.bofh.it>
In reply to#1544252
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

[toc] | [prev] | [next] | [standalone]

#1544380 — Re: [PATCH] extcon: 3gpio: add driver for USB OTG port controlled by 3 GPIOs

FromHans de Goede <hdegoede@redhat.com>
Date2016-12-19 10:00 +0100
SubjectRe: [PATCH] extcon: 3gpio: add driver for USB OTG port controlled by 3 GPIOs
Message-ID<sQ1Xz-5sb-7@gated-at.bofh.it>
In reply to#1544354
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

[toc] | [prev] | [next] | [standalone]

#1544404 — Re: [PATCH] extcon: 3gpio: add driver for USB OTG port controlled by 3 GPIOs

FromChanwoo Choi <cw00.choi@samsung.com>
Date2016-12-19 10:50 +0100
SubjectRe: [PATCH] extcon: 3gpio: add driver for USB OTG port controlled by 3 GPIOs
Message-ID<sQ2JY-60r-15@gated-at.bofh.it>
In reply to#1544380
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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web