Message-id: <585793A9.9070805@samsung.com>
Date:   Mon, 19 Dec 2016 17:00:41 +0900
From:   Chanwoo Choi <cw00.choi@...sung.com>
To:     Hans de Goede <hdegoede@...hat.com>,
        MyungJoo Ham <myungjoo.ham@...sung.com>
Cc:     linux-kernel@...r.kernel.org, David Cohen <david.a.cohen@...el.com>
Subject: Re: [PATCH] extcon: 3gpio: add driver for USB OTG port controlled by 3
 GPIOs

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@...el.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@...el.com>
> [hdgoede@...hat.com: Port to current kernel, submit upstream]
> Signed-off-by: Hans de Goede <hdegoede@...hat.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@...hat.com>

s/2016) -> 2016

> + *
> + * Based on android x86 kernel code which is:
> + *
> + * Copyright (c) 2014, Intel Corporation.
> + * Author: David Cohen <david.a.cohen@...ux.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@...hat.com>");
> +MODULE_DESCRIPTION("3 GPIO USB OTG extcon driver");
> +MODULE_LICENSE("GPL");
> 

-- 
Regards,
Chanwoo Choi

Powered by blists - more mailing lists