lkml.org 
[lkml]   [2016]   [Dec]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] extcon: 3gpio: add driver for USB OTG port controlled by 3 GPIOs
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

\
 
 \ /
  Last update: 2016-12-19 10:42    [W:0.115 / U:0.068 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site