| Message ID | 1422274532-9488-2-git-send-email-rogerq@ti.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
Hi Roger, This patch looks good to me. But I add some comment. If you modify some comment, I'll apply this patch on 3.21 queue. On Mon, Jan 26, 2015 at 9:15 PM, Roger Quadros <rogerq@ti.com> wrote: > This driver observes the USB ID pin connected over a GPIO and > updates the USB cable extcon states accordingly. > > The existing GPIO extcon driver is not suitable for this purpose > as it needs to be taught to understand USB cable states and it > can't handle more than one cable per instance. > > For the USB case we need to handle 2 cable states. > 1) USB (attach/detach) > 2) USB-Host (attach/detach) > > This driver can be easily updated in the future to handle VBUS > events in case it happens to be available on GPIO for any platform. > > Signed-off-by: Roger Quadros <rogerq@ti.com> > --- > .../devicetree/bindings/extcon/extcon-usb-gpio.txt | 20 ++ > drivers/extcon/Kconfig | 7 + > drivers/extcon/Makefile | 1 + > drivers/extcon/extcon-usb-gpio.c | 214 +++++++++++++++++++++ > 4 files changed, 242 insertions(+) > create mode 100644 Documentation/devicetree/bindings/extcon/extcon-usb-gpio.txt > create mode 100644 drivers/extcon/extcon-usb-gpio.c > > diff --git a/Documentation/devicetree/bindings/extcon/extcon-usb-gpio.txt b/Documentation/devicetree/bindings/extcon/extcon-usb-gpio.txt > new file mode 100644 > index 0000000..ab52a85 > --- /dev/null > +++ b/Documentation/devicetree/bindings/extcon/extcon-usb-gpio.txt > @@ -0,0 +1,20 @@ > +USB GPIO Extcon device > + > +This is a virtual device used to generate USB cable states from the USB ID pin > +connected to a GPIO pin. > + > +Required properties: > +- compatible: Should be "linux,extcon-usb-gpio" > +- id-gpio: gpio for USB ID pin. See gpio binding. > + > +Example: > + extcon_usb1 { > + compatible = "linux,extcon-usb-gpio"; > + id-gpio = <&gpio6 1 GPIO_ACTIVE_HIGH>; > + } > + > + usb@1 { > + ... > + extcon = <&extcon_usb1>; > + ... I don' want to use '...' for example. How about using omap usecase in your patch4[1] as following? [1] ARM: dts: dra72-evm: Add extcon nodes for USB &omap_dwc3_1 { extcon = <&extcon_usb1>; }; > + }; > diff --git a/drivers/extcon/Kconfig b/drivers/extcon/Kconfig > index 6a1f7de..e4c01ab 100644 > --- a/drivers/extcon/Kconfig > +++ b/drivers/extcon/Kconfig > @@ -93,4 +93,11 @@ config EXTCON_SM5502 > Silicon Mitus SM5502. The SM5502 is a USB port accessory > detector and switch. > > +config EXTCON_USB_GPIO > + tristate "USB GPIO extcon support" > + depends on GPIOLIB How about use 'select' keyword instead of 'depends on'? - depends on GPIOLIB -> select GPIOLIB > + help > + Say Y here to enable GPIO based USB cable detection extcon support. > + Used typically if GPIO is used for USB ID pin detection. > + > endif # MULTISTATE_SWITCH > diff --git a/drivers/extcon/Makefile b/drivers/extcon/Makefile > index 0370b42..6a08a98 100644 > --- a/drivers/extcon/Makefile > +++ b/drivers/extcon/Makefile > @@ -12,3 +12,4 @@ obj-$(CONFIG_EXTCON_MAX8997) += extcon-max8997.o > obj-$(CONFIG_EXTCON_PALMAS) += extcon-palmas.o > obj-$(CONFIG_EXTCON_RT8973A) += extcon-rt8973a.o > obj-$(CONFIG_EXTCON_SM5502) += extcon-sm5502.o > +obj-$(CONFIG_EXTCON_USB_GPIO) += extcon-usb-gpio.o > diff --git a/drivers/extcon/extcon-usb-gpio.c b/drivers/extcon/extcon-usb-gpio.c > new file mode 100644 > index 0000000..a20aa39 > --- /dev/null > +++ b/drivers/extcon/extcon-usb-gpio.c > @@ -0,0 +1,214 @@ > +/** > + * drivers/extcon/extcon_usb_gpio.c - USB GPIO extcon driver You should use the '-' instead of '_'. - s/extcon_usb_gpio.c/extcon-usb-gpio.c > + * > + * Copyright (C) 2015 Texas Instruments Incorporated - http://www.ti.com > + * Author: Roger Quadros <rogerq@ti.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/extcon.h> > +#include <linux/init.h> > +#include <linux/interrupt.h> > +#include <linux/irq.h> > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/of_gpio.h> > +#include <linux/platform_device.h> > +#include <linux/slab.h> > +#include <linux/workqueue.h> > + > +#define USB_GPIO_DEBOUNCE_MS 20 /* ms */ > + > +struct usb_extcon_info { > + struct device *dev; > + struct extcon_dev *edev; > + > + struct gpio_desc *id_gpiod; > + int id_irq; > + > + unsigned long debounce_jiffies; > + struct delayed_work wq_detcable; > +}; > + > +/* List of detectable cables */ > +enum { > + EXTCON_CABLE_USB = 0, > + EXTCON_CABLE_USB_HOST, > + > + EXTCON_CABLE_END, > +}; > + > +static const char *usb_extcon_cable[] = { > + [EXTCON_CABLE_USB] = "USB", > + [EXTCON_CABLE_USB_HOST] = "USB-Host", > + NULL, > +}; > + > +static void usb_extcon_detect_cable(struct work_struct *work) > +{ > + int id; > + struct usb_extcon_info *info; > + > + info = container_of(to_delayed_work(work), struct usb_extcon_info, > + wq_detcable); Simplify container_of statement as following: struct usb_extcon_info *info = container_of(to_delayed_work(work), struct usb_extcon_info, wq_detcable); > + > + /* check ID and update cable state */ > + id = gpiod_get_value_cansleep(info->id_gpiod); > + if (id) { > + /* > + * ID = 1 means USB HOST cable detached. > + * As we don't have event for USB peripheral cable attached, > + * we simulate USB peripheral attach here. > + */ > + extcon_set_cable_state(info->edev, > + usb_extcon_cable[EXTCON_CABLE_USB_HOST], > + false); > + extcon_set_cable_state(info->edev, > + usb_extcon_cable[EXTCON_CABLE_USB], > + true); > + } else { > + /* > + * ID = 0 means USB HOST cable attached. > + * As we don't have event for USB peripheral cable detached, > + * we simulate USB peripheral detach here. > + */ > + extcon_set_cable_state(info->edev, > + usb_extcon_cable[EXTCON_CABLE_USB], > + false); > + extcon_set_cable_state(info->edev, > + usb_extcon_cable[EXTCON_CABLE_USB_HOST], > + true); > + } > +} > + > +static irqreturn_t usb_irq_handler(int irq, void *dev_id) > +{ > + struct usb_extcon_info *info = dev_id; > + > + queue_delayed_work(system_power_efficient_wq, &info->wq_detcable, > + info->debounce_jiffies); Need to add blank line. > + return IRQ_HANDLED; > +} > + > +static int usb_extcon_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct device_node *np = dev->of_node; > + struct usb_extcon_info *info; > + int ret; > + > + if (!np) > + return -EINVAL; > + > + info = devm_kzalloc(&pdev->dev, sizeof(*info), GFP_KERNEL); > + if (!info) > + return -ENOMEM; > + > + info->dev = dev; > + info->id_gpiod = devm_gpiod_get(&pdev->dev, "id"); > + if (IS_ERR(info->id_gpiod)) { > + dev_err(dev, "failed to get ID GPIO\n"); > + return PTR_ERR(info->id_gpiod); > + } > + > + ret = gpiod_set_debounce(info->id_gpiod, > + USB_GPIO_DEBOUNCE_MS * 1000); > + if (ret < 0) > + info->debounce_jiffies = msecs_to_jiffies(USB_GPIO_DEBOUNCE_MS); > + > + INIT_DELAYED_WORK(&info->wq_detcable, usb_extcon_detect_cable); > + > + info->id_irq = gpiod_to_irq(info->id_gpiod); > + if (info->id_irq < 0) { > + dev_err(dev, "failed to get ID IRQ\n"); > + return info->id_irq; > + } > + > + ret = devm_request_threaded_irq(dev, info->id_irq, NULL, > + usb_irq_handler, > + IRQF_SHARED | IRQF_ONESHOT | > + IRQF_NO_SUSPEND, > + pdev->name, info); > + if (ret < 0) { > + dev_err(dev, "failed to request handler for ID IRQ\n"); > + return ret; > + } > + > + info->edev = devm_extcon_dev_allocate(dev, usb_extcon_cable); > + if (IS_ERR(info->edev)) { > + dev_err(dev, "failed to allocate extcon device\n"); > + return -ENOMEM; > + } > + > + ret = devm_extcon_dev_register(dev, info->edev); > + if (ret < 0) { > + dev_err(dev, "failed to register extcon device\n"); > + return ret; > + } > + > + platform_set_drvdata(pdev, info); I prefer to execute the device_init_wakeup() function as following for suspend/resume function: device_init_wakeup(&pdev->dev, 1); > + > + /* Perform initial detection */ > + usb_extcon_detect_cable(&info->wq_detcable.work); > + > + return 0; > +} > + > +static int usb_extcon_remove(struct platform_device *pdev) > +{ > + struct usb_extcon_info *info = platform_get_drvdata(pdev); > + > + cancel_delayed_work_sync(&info->wq_detcable); Need to add blank line. > + return 0; > +} > + > +#ifdef CONFIG_PM_SLEEP > +static int usb_extcon_suspend(struct device *dev) > +{ > + struct usb_extcon_info *info = dev_get_drvdata(dev); > + > + enable_irq_wake(info->id_irq); I prefer to use device_may_wakeup() function for whether executing enable_irq_wake() or not. Also, The disable_irq() in the suspend function would prevent us from discarding interrupt before wakeup from suspend completely. if (device_may_wakeup(dev)) enable_irq_wake(info->id_irq); disable_irq(info->id_irq); > + return 0; > +} > + > +static int usb_extcon_resume(struct device *dev) > +{ > + struct usb_extcon_info *info = dev_get_drvdata(dev); > + > + disable_irq_wake(info->id_irq); ditto as following: if (device_may_wakeup(dev)) disable_irq_wake(info->id_irq); enable_irq(info->id_irq); > + return 0; > +} > +#endif > + > +static SIMPLE_DEV_PM_OPS(usb_extcon_pm_ops, > + usb_extcon_suspend, usb_extcon_resume); > + > +static struct of_device_id usb_extcon_dt_match[] = { > + { .compatible = "linux,extcon-usb-gpio", }, > + { /* end */ } I prefer to use 'sentinel' word instead of 'end' > +}; > +MODULE_DEVICE_TABLE(of, usb_extcon_dt_match); > + > +static struct platform_driver usb_extcon_driver = { > + .probe = usb_extcon_probe, > + .remove = usb_extcon_remove, > + .driver = { > + .name = "extcon-usb-gpio", > + .pm = &usb_extcon_pm_ops, > + .of_match_table = usb_extcon_dt_match, > + }, > +}; > + > +module_platform_driver(usb_extcon_driver); > + > +MODULE_AUTHOR("Roger Quadros <rogerq@ti.com>"); > +MODULE_DESCRIPTION("USB GPIO extcon driver"); > +MODULE_LICENSE("GPL v2"); Thanks, Chanwoo Choi -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Chanwoo, All your comments are valid. Need some clarification on one comment. On 26/01/15 15:56, Chanwoo Choi wrote: > Hi Roger, > > This patch looks good to me. But I add some comment. > If you modify some comment, I'll apply this patch on 3.21 queue. > > On Mon, Jan 26, 2015 at 9:15 PM, Roger Quadros <rogerq@ti.com> wrote: >> This driver observes the USB ID pin connected over a GPIO and >> updates the USB cable extcon states accordingly. >> >> The existing GPIO extcon driver is not suitable for this purpose >> as it needs to be taught to understand USB cable states and it >> can't handle more than one cable per instance. >> >> For the USB case we need to handle 2 cable states. >> 1) USB (attach/detach) >> 2) USB-Host (attach/detach) >> >> This driver can be easily updated in the future to handle VBUS >> events in case it happens to be available on GPIO for any platform. >> >> Signed-off-by: Roger Quadros <rogerq@ti.com> >> --- >> .../devicetree/bindings/extcon/extcon-usb-gpio.txt | 20 ++ >> drivers/extcon/Kconfig | 7 + >> drivers/extcon/Makefile | 1 + >> drivers/extcon/extcon-usb-gpio.c | 214 +++++++++++++++++++++ >> 4 files changed, 242 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/extcon/extcon-usb-gpio.txt >> create mode 100644 drivers/extcon/extcon-usb-gpio.c >> <snip> >> + >> +static int usb_extcon_probe(struct platform_device *pdev) >> +{ >> + struct device *dev = &pdev->dev; >> + struct device_node *np = dev->of_node; >> + struct usb_extcon_info *info; >> + int ret; >> + >> + if (!np) >> + return -EINVAL; >> + >> + info = devm_kzalloc(&pdev->dev, sizeof(*info), GFP_KERNEL); >> + if (!info) >> + return -ENOMEM; >> + >> + info->dev = dev; >> + info->id_gpiod = devm_gpiod_get(&pdev->dev, "id"); >> + if (IS_ERR(info->id_gpiod)) { >> + dev_err(dev, "failed to get ID GPIO\n"); >> + return PTR_ERR(info->id_gpiod); >> + } >> + >> + ret = gpiod_set_debounce(info->id_gpiod, >> + USB_GPIO_DEBOUNCE_MS * 1000); >> + if (ret < 0) >> + info->debounce_jiffies = msecs_to_jiffies(USB_GPIO_DEBOUNCE_MS); >> + >> + INIT_DELAYED_WORK(&info->wq_detcable, usb_extcon_detect_cable); >> + >> + info->id_irq = gpiod_to_irq(info->id_gpiod); >> + if (info->id_irq < 0) { >> + dev_err(dev, "failed to get ID IRQ\n"); >> + return info->id_irq; >> + } >> + >> + ret = devm_request_threaded_irq(dev, info->id_irq, NULL, >> + usb_irq_handler, >> + IRQF_SHARED | IRQF_ONESHOT | >> + IRQF_NO_SUSPEND, >> + pdev->name, info); use of IRQF_NO_SUSPEND is not recommended to be used together with IRQF_SHARED so I'll remove IRQF_SHARED from here if we decide to stick with IRQF_NO_SUSPEND. More on this below. >> + if (ret < 0) { >> + dev_err(dev, "failed to request handler for ID IRQ\n"); >> + return ret; >> + } >> + >> + info->edev = devm_extcon_dev_allocate(dev, usb_extcon_cable); >> + if (IS_ERR(info->edev)) { >> + dev_err(dev, "failed to allocate extcon device\n"); >> + return -ENOMEM; >> + } >> + >> + ret = devm_extcon_dev_register(dev, info->edev); >> + if (ret < 0) { >> + dev_err(dev, "failed to register extcon device\n"); >> + return ret; >> + } >> + >> + platform_set_drvdata(pdev, info); > > I prefer to execute the device_init_wakeup() function as following > for suspend/resume function: > device_init_wakeup(&pdev->dev, 1); > >> + >> + /* Perform initial detection */ >> + usb_extcon_detect_cable(&info->wq_detcable.work); >> + >> + return 0; >> +} >> + >> +static int usb_extcon_remove(struct platform_device *pdev) >> +{ >> + struct usb_extcon_info *info = platform_get_drvdata(pdev); >> + >> + cancel_delayed_work_sync(&info->wq_detcable); > > Need to add blank line. > >> + return 0; >> +} >> + >> +#ifdef CONFIG_PM_SLEEP >> +static int usb_extcon_suspend(struct device *dev) >> +{ >> + struct usb_extcon_info *info = dev_get_drvdata(dev); >> + >> + enable_irq_wake(info->id_irq); > > I prefer to use device_may_wakeup() function for whether > executing enable_irq_wake() or not. Also, The disable_irq() > in the suspend function would prevent us from discarding interrupt > before wakeup from suspend completely. > I need more clarification here. If we are going to use enable_irq_wake() here then what is the point of IRQF_NO_SUSPEND? >From Documentation/power/suspend-and-interrupts.txt I see that interrupts marked as IRQF_NO_SUSPEND should not be configured for system wakeup using enable_irq_wake(). what is your preference? Is it good enough to not use IRQF_NO_SUSPEND but use enable_irq_wake() instead to enable system wakeup for that IRQ. > if (device_may_wakeup(dev)) > enable_irq_wake(info->id_irq); > disable_irq(info->id_irq); why do we need to disable irq here? How will the system wakeup if IRQ is disabled? > > >> + return 0; >> +} >> + <snip> cheers, -roger -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Roger, On 01/27/2015 01:27 AM, Roger Quadros wrote: > Hi Chanwoo, > > All your comments are valid. Need some clarification on one comment. > > On 26/01/15 15:56, Chanwoo Choi wrote: >> Hi Roger, >> >> This patch looks good to me. But I add some comment. >> If you modify some comment, I'll apply this patch on 3.21 queue. >> >> On Mon, Jan 26, 2015 at 9:15 PM, Roger Quadros <rogerq@ti.com> wrote: >>> This driver observes the USB ID pin connected over a GPIO and >>> updates the USB cable extcon states accordingly. >>> >>> The existing GPIO extcon driver is not suitable for this purpose >>> as it needs to be taught to understand USB cable states and it >>> can't handle more than one cable per instance. >>> >>> For the USB case we need to handle 2 cable states. >>> 1) USB (attach/detach) >>> 2) USB-Host (attach/detach) >>> >>> This driver can be easily updated in the future to handle VBUS >>> events in case it happens to be available on GPIO for any platform. >>> >>> Signed-off-by: Roger Quadros <rogerq@ti.com> >>> --- >>> .../devicetree/bindings/extcon/extcon-usb-gpio.txt | 20 ++ >>> drivers/extcon/Kconfig | 7 + >>> drivers/extcon/Makefile | 1 + >>> drivers/extcon/extcon-usb-gpio.c | 214 +++++++++++++++++++++ >>> 4 files changed, 242 insertions(+) >>> create mode 100644 Documentation/devicetree/bindings/extcon/extcon-usb-gpio.txt >>> create mode 100644 drivers/extcon/extcon-usb-gpio.c >>> > > <snip> > >>> + >>> +static int usb_extcon_probe(struct platform_device *pdev) >>> +{ >>> + struct device *dev = &pdev->dev; >>> + struct device_node *np = dev->of_node; >>> + struct usb_extcon_info *info; >>> + int ret; >>> + >>> + if (!np) >>> + return -EINVAL; >>> + >>> + info = devm_kzalloc(&pdev->dev, sizeof(*info), GFP_KERNEL); >>> + if (!info) >>> + return -ENOMEM; >>> + >>> + info->dev = dev; >>> + info->id_gpiod = devm_gpiod_get(&pdev->dev, "id"); >>> + if (IS_ERR(info->id_gpiod)) { >>> + dev_err(dev, "failed to get ID GPIO\n"); >>> + return PTR_ERR(info->id_gpiod); >>> + } >>> + >>> + ret = gpiod_set_debounce(info->id_gpiod, >>> + USB_GPIO_DEBOUNCE_MS * 1000); >>> + if (ret < 0) >>> + info->debounce_jiffies = msecs_to_jiffies(USB_GPIO_DEBOUNCE_MS); >>> + >>> + INIT_DELAYED_WORK(&info->wq_detcable, usb_extcon_detect_cable); >>> + >>> + info->id_irq = gpiod_to_irq(info->id_gpiod); >>> + if (info->id_irq < 0) { >>> + dev_err(dev, "failed to get ID IRQ\n"); >>> + return info->id_irq; >>> + } >>> + >>> + ret = devm_request_threaded_irq(dev, info->id_irq, NULL, >>> + usb_irq_handler, >>> + IRQF_SHARED | IRQF_ONESHOT | >>> + IRQF_NO_SUSPEND, >>> + pdev->name, info); > > use of IRQF_NO_SUSPEND is not recommended to be used together with IRQF_SHARED so > I'll remove IRQF_SHARED from here if we decide to stick with IRQF_NO_SUSPEND. > More on this below. > >>> + if (ret < 0) { >>> + dev_err(dev, "failed to request handler for ID IRQ\n"); >>> + return ret; >>> + } >>> + >>> + info->edev = devm_extcon_dev_allocate(dev, usb_extcon_cable); >>> + if (IS_ERR(info->edev)) { >>> + dev_err(dev, "failed to allocate extcon device\n"); >>> + return -ENOMEM; >>> + } >>> + >>> + ret = devm_extcon_dev_register(dev, info->edev); >>> + if (ret < 0) { >>> + dev_err(dev, "failed to register extcon device\n"); >>> + return ret; >>> + } >>> + >>> + platform_set_drvdata(pdev, info); >> >> I prefer to execute the device_init_wakeup() function as following >> for suspend/resume function: >> device_init_wakeup(&pdev->dev, 1); >> >>> + >>> + /* Perform initial detection */ >>> + usb_extcon_detect_cable(&info->wq_detcable.work); >>> + >>> + return 0; >>> +} >>> + >>> +static int usb_extcon_remove(struct platform_device *pdev) >>> +{ >>> + struct usb_extcon_info *info = platform_get_drvdata(pdev); >>> + >>> + cancel_delayed_work_sync(&info->wq_detcable); >> >> Need to add blank line. >> >>> + return 0; >>> +} >>> + >>> +#ifdef CONFIG_PM_SLEEP >>> +static int usb_extcon_suspend(struct device *dev) >>> +{ >>> + struct usb_extcon_info *info = dev_get_drvdata(dev); >>> + >>> + enable_irq_wake(info->id_irq); >> >> I prefer to use device_may_wakeup() function for whether >> executing enable_irq_wake() or not. Also, The disable_irq() >> in the suspend function would prevent us from discarding interrupt >> before wakeup from suspend completely. >> > > I need more clarification here. > > If we are going to use enable_irq_wake() here then what is the point of IRQF_NO_SUSPEND? > >>From Documentation/power/suspend-and-interrupts.txt I see that interrupts marked > as IRQF_NO_SUSPEND should not be configured for system wakeup using enable_irq_wake(). > > what is your preference? > > Is it good enough to not use IRQF_NO_SUSPEND but use enable_irq_wake() instead to > enable system wakeup for that IRQ. I'm sorry for confusion about usage both IRQF_NO_SUSPEND and enable_irq_wake(). If suspend() function in device driver executes the enable_irq_wake(), IRQF_NO_SUSPEND flag is not necessary. I think that we better use enable_irq_wake() instead of adding IRQF_NO_SUSPEND flag. I'll expect to remove IRQF_NO_SUSPEND flag when requesting gpio interrupt. > >> if (device_may_wakeup(dev)) >> enable_irq_wake(info->id_irq); >> disable_irq(info->id_irq); > > why do we need to disable irq here? How will the system wakeup if IRQ is disabled? The disable_irq() may make the interrupt as masking state. Although interrput is masking state(disable), interrup can happen. but, the interrupt may remain the pending state without discarding it. And then, When resume() function in extcon-usb-gpio.c executes enable_irq(info->id_irq), pending interrupt will happen and executes the interrupt handler(usb_irq_handler). If we don't execute disable_irq() in suspend function, info->id->irq interrupt might happen before completing the resume sequence of extcon-gpio-usb driver. Thanks, Chanwoo Choi -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Chanwoo, On 27/01/15 03:54, Chanwoo Choi wrote: > Hi Roger, > > On 01/27/2015 01:27 AM, Roger Quadros wrote: >> Hi Chanwoo, >> >> All your comments are valid. Need some clarification on one comment. >> >> On 26/01/15 15:56, Chanwoo Choi wrote: >>> Hi Roger, >>> >>> This patch looks good to me. But I add some comment. >>> If you modify some comment, I'll apply this patch on 3.21 queue. >>> >>> On Mon, Jan 26, 2015 at 9:15 PM, Roger Quadros <rogerq@ti.com> wrote: >>>> This driver observes the USB ID pin connected over a GPIO and >>>> updates the USB cable extcon states accordingly. >>>> >>>> The existing GPIO extcon driver is not suitable for this purpose >>>> as it needs to be taught to understand USB cable states and it >>>> can't handle more than one cable per instance. >>>> >>>> For the USB case we need to handle 2 cable states. >>>> 1) USB (attach/detach) >>>> 2) USB-Host (attach/detach) >>>> >>>> This driver can be easily updated in the future to handle VBUS >>>> events in case it happens to be available on GPIO for any platform. >>>> >>>> Signed-off-by: Roger Quadros <rogerq@ti.com> >>>> --- >>>> .../devicetree/bindings/extcon/extcon-usb-gpio.txt | 20 ++ >>>> drivers/extcon/Kconfig | 7 + >>>> drivers/extcon/Makefile | 1 + >>>> drivers/extcon/extcon-usb-gpio.c | 214 +++++++++++++++++++++ >>>> 4 files changed, 242 insertions(+) >>>> create mode 100644 Documentation/devicetree/bindings/extcon/extcon-usb-gpio.txt >>>> create mode 100644 drivers/extcon/extcon-usb-gpio.c >>>> >> >> <snip> >> >>>> + >>>> +static int usb_extcon_probe(struct platform_device *pdev) >>>> +{ >>>> + struct device *dev = &pdev->dev; >>>> + struct device_node *np = dev->of_node; >>>> + struct usb_extcon_info *info; >>>> + int ret; >>>> + >>>> + if (!np) >>>> + return -EINVAL; >>>> + >>>> + info = devm_kzalloc(&pdev->dev, sizeof(*info), GFP_KERNEL); >>>> + if (!info) >>>> + return -ENOMEM; >>>> + >>>> + info->dev = dev; >>>> + info->id_gpiod = devm_gpiod_get(&pdev->dev, "id"); >>>> + if (IS_ERR(info->id_gpiod)) { >>>> + dev_err(dev, "failed to get ID GPIO\n"); >>>> + return PTR_ERR(info->id_gpiod); >>>> + } >>>> + >>>> + ret = gpiod_set_debounce(info->id_gpiod, >>>> + USB_GPIO_DEBOUNCE_MS * 1000); >>>> + if (ret < 0) >>>> + info->debounce_jiffies = msecs_to_jiffies(USB_GPIO_DEBOUNCE_MS); >>>> + >>>> + INIT_DELAYED_WORK(&info->wq_detcable, usb_extcon_detect_cable); >>>> + >>>> + info->id_irq = gpiod_to_irq(info->id_gpiod); >>>> + if (info->id_irq < 0) { >>>> + dev_err(dev, "failed to get ID IRQ\n"); >>>> + return info->id_irq; >>>> + } >>>> + >>>> + ret = devm_request_threaded_irq(dev, info->id_irq, NULL, >>>> + usb_irq_handler, >>>> + IRQF_SHARED | IRQF_ONESHOT | >>>> + IRQF_NO_SUSPEND, >>>> + pdev->name, info); >> >> use of IRQF_NO_SUSPEND is not recommended to be used together with IRQF_SHARED so >> I'll remove IRQF_SHARED from here if we decide to stick with IRQF_NO_SUSPEND. >> More on this below. >> >>>> + if (ret < 0) { >>>> + dev_err(dev, "failed to request handler for ID IRQ\n"); >>>> + return ret; >>>> + } >>>> + >>>> + info->edev = devm_extcon_dev_allocate(dev, usb_extcon_cable); >>>> + if (IS_ERR(info->edev)) { >>>> + dev_err(dev, "failed to allocate extcon device\n"); >>>> + return -ENOMEM; >>>> + } >>>> + >>>> + ret = devm_extcon_dev_register(dev, info->edev); >>>> + if (ret < 0) { >>>> + dev_err(dev, "failed to register extcon device\n"); >>>> + return ret; >>>> + } >>>> + >>>> + platform_set_drvdata(pdev, info); >>> >>> I prefer to execute the device_init_wakeup() function as following >>> for suspend/resume function: >>> device_init_wakeup(&pdev->dev, 1); >>> >>>> + >>>> + /* Perform initial detection */ >>>> + usb_extcon_detect_cable(&info->wq_detcable.work); >>>> + >>>> + return 0; >>>> +} >>>> + >>>> +static int usb_extcon_remove(struct platform_device *pdev) >>>> +{ >>>> + struct usb_extcon_info *info = platform_get_drvdata(pdev); >>>> + >>>> + cancel_delayed_work_sync(&info->wq_detcable); >>> >>> Need to add blank line. >>> >>>> + return 0; >>>> +} >>>> + >>>> +#ifdef CONFIG_PM_SLEEP >>>> +static int usb_extcon_suspend(struct device *dev) >>>> +{ >>>> + struct usb_extcon_info *info = dev_get_drvdata(dev); >>>> + >>>> + enable_irq_wake(info->id_irq); >>> >>> I prefer to use device_may_wakeup() function for whether >>> executing enable_irq_wake() or not. Also, The disable_irq() >>> in the suspend function would prevent us from discarding interrupt >>> before wakeup from suspend completely. >>> >> >> I need more clarification here. >> >> If we are going to use enable_irq_wake() here then what is the point of IRQF_NO_SUSPEND? >> >> >From Documentation/power/suspend-and-interrupts.txt I see that interrupts marked >> as IRQF_NO_SUSPEND should not be configured for system wakeup using enable_irq_wake(). >> >> what is your preference? >> >> Is it good enough to not use IRQF_NO_SUSPEND but use enable_irq_wake() instead to >> enable system wakeup for that IRQ. > > I'm sorry for confusion about usage both IRQF_NO_SUSPEND and enable_irq_wake(). > If suspend() function in device driver executes the enable_irq_wake(), > IRQF_NO_SUSPEND flag is not necessary. > > I think that we better use enable_irq_wake() instead of adding IRQF_NO_SUSPEND flag. > I'll expect to remove IRQF_NO_SUSPEND flag when requesting gpio interrupt. > OK. >> >>> if (device_may_wakeup(dev)) >>> enable_irq_wake(info->id_irq); >>> disable_irq(info->id_irq); >> >> why do we need to disable irq here? How will the system wakeup if IRQ is disabled? > > The disable_irq() may make the interrupt as masking state. > Although interrput is masking state(disable), interrup can happen. > but, the interrupt may remain the pending state without discarding it. > > And then, > When resume() function in extcon-usb-gpio.c executes enable_irq(info->id_irq), > pending interrupt will happen and executes the interrupt handler(usb_irq_handler). > > If we don't execute disable_irq() in suspend function, > info->id->irq interrupt might happen before completing the resume sequence > of extcon-gpio-usb driver. How will that cause a problem? If an interrupt happens _before_ the system enters SUSPEND state then kernel should abort the suspend. This should be taken care by kernel PM core and not the device driver. I still fail to understand that we need to call disable_irq() in .suspend() and enable_irq() in .resume() can you point me to any other drivers doing so? cheers, -roger -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Roger, On 01/28/2015 12:38 AM, Roger Quadros wrote: > Chanwoo, > > On 27/01/15 03:54, Chanwoo Choi wrote: >> Hi Roger, >> >> On 01/27/2015 01:27 AM, Roger Quadros wrote: >>> Hi Chanwoo, >>> >>> All your comments are valid. Need some clarification on one comment. >>> >>> On 26/01/15 15:56, Chanwoo Choi wrote: >>>> Hi Roger, >>>> >>>> This patch looks good to me. But I add some comment. >>>> If you modify some comment, I'll apply this patch on 3.21 queue. >>>> >>>> On Mon, Jan 26, 2015 at 9:15 PM, Roger Quadros <rogerq@ti.com> wrote: >>>>> This driver observes the USB ID pin connected over a GPIO and >>>>> updates the USB cable extcon states accordingly. >>>>> >>>>> The existing GPIO extcon driver is not suitable for this purpose >>>>> as it needs to be taught to understand USB cable states and it >>>>> can't handle more than one cable per instance. >>>>> >>>>> For the USB case we need to handle 2 cable states. >>>>> 1) USB (attach/detach) >>>>> 2) USB-Host (attach/detach) >>>>> >>>>> This driver can be easily updated in the future to handle VBUS >>>>> events in case it happens to be available on GPIO for any platform. >>>>> >>>>> Signed-off-by: Roger Quadros <rogerq@ti.com> >>>>> --- >>>>> .../devicetree/bindings/extcon/extcon-usb-gpio.txt | 20 ++ >>>>> drivers/extcon/Kconfig | 7 + >>>>> drivers/extcon/Makefile | 1 + >>>>> drivers/extcon/extcon-usb-gpio.c | 214 +++++++++++++++++++++ >>>>> 4 files changed, 242 insertions(+) >>>>> create mode 100644 Documentation/devicetree/bindings/extcon/extcon-usb-gpio.txt >>>>> create mode 100644 drivers/extcon/extcon-usb-gpio.c >>>>> >>> >>> <snip> >>> >>>>> + >>>>> +static int usb_extcon_probe(struct platform_device *pdev) >>>>> +{ >>>>> + struct device *dev = &pdev->dev; >>>>> + struct device_node *np = dev->of_node; >>>>> + struct usb_extcon_info *info; >>>>> + int ret; >>>>> + >>>>> + if (!np) >>>>> + return -EINVAL; >>>>> + >>>>> + info = devm_kzalloc(&pdev->dev, sizeof(*info), GFP_KERNEL); >>>>> + if (!info) >>>>> + return -ENOMEM; >>>>> + >>>>> + info->dev = dev; >>>>> + info->id_gpiod = devm_gpiod_get(&pdev->dev, "id"); >>>>> + if (IS_ERR(info->id_gpiod)) { >>>>> + dev_err(dev, "failed to get ID GPIO\n"); >>>>> + return PTR_ERR(info->id_gpiod); >>>>> + } >>>>> + >>>>> + ret = gpiod_set_debounce(info->id_gpiod, >>>>> + USB_GPIO_DEBOUNCE_MS * 1000); >>>>> + if (ret < 0) >>>>> + info->debounce_jiffies = msecs_to_jiffies(USB_GPIO_DEBOUNCE_MS); >>>>> + >>>>> + INIT_DELAYED_WORK(&info->wq_detcable, usb_extcon_detect_cable); >>>>> + >>>>> + info->id_irq = gpiod_to_irq(info->id_gpiod); >>>>> + if (info->id_irq < 0) { >>>>> + dev_err(dev, "failed to get ID IRQ\n"); >>>>> + return info->id_irq; >>>>> + } >>>>> + >>>>> + ret = devm_request_threaded_irq(dev, info->id_irq, NULL, >>>>> + usb_irq_handler, >>>>> + IRQF_SHARED | IRQF_ONESHOT | >>>>> + IRQF_NO_SUSPEND, >>>>> + pdev->name, info); >>> >>> use of IRQF_NO_SUSPEND is not recommended to be used together with IRQF_SHARED so >>> I'll remove IRQF_SHARED from here if we decide to stick with IRQF_NO_SUSPEND. >>> More on this below. >>> >>>>> + if (ret < 0) { >>>>> + dev_err(dev, "failed to request handler for ID IRQ\n"); >>>>> + return ret; >>>>> + } >>>>> + >>>>> + info->edev = devm_extcon_dev_allocate(dev, usb_extcon_cable); >>>>> + if (IS_ERR(info->edev)) { >>>>> + dev_err(dev, "failed to allocate extcon device\n"); >>>>> + return -ENOMEM; >>>>> + } >>>>> + >>>>> + ret = devm_extcon_dev_register(dev, info->edev); >>>>> + if (ret < 0) { >>>>> + dev_err(dev, "failed to register extcon device\n"); >>>>> + return ret; >>>>> + } >>>>> + >>>>> + platform_set_drvdata(pdev, info); >>>> >>>> I prefer to execute the device_init_wakeup() function as following >>>> for suspend/resume function: >>>> device_init_wakeup(&pdev->dev, 1); >>>> >>>>> + >>>>> + /* Perform initial detection */ >>>>> + usb_extcon_detect_cable(&info->wq_detcable.work); >>>>> + >>>>> + return 0; >>>>> +} >>>>> + >>>>> +static int usb_extcon_remove(struct platform_device *pdev) >>>>> +{ >>>>> + struct usb_extcon_info *info = platform_get_drvdata(pdev); >>>>> + >>>>> + cancel_delayed_work_sync(&info->wq_detcable); >>>> >>>> Need to add blank line. >>>> >>>>> + return 0; >>>>> +} >>>>> + >>>>> +#ifdef CONFIG_PM_SLEEP >>>>> +static int usb_extcon_suspend(struct device *dev) >>>>> +{ >>>>> + struct usb_extcon_info *info = dev_get_drvdata(dev); >>>>> + >>>>> + enable_irq_wake(info->id_irq); >>>> >>>> I prefer to use device_may_wakeup() function for whether >>>> executing enable_irq_wake() or not. Also, The disable_irq() >>>> in the suspend function would prevent us from discarding interrupt >>>> before wakeup from suspend completely. >>>> >>> >>> I need more clarification here. >>> >>> If we are going to use enable_irq_wake() here then what is the point of IRQF_NO_SUSPEND? >>> >>> >From Documentation/power/suspend-and-interrupts.txt I see that interrupts marked >>> as IRQF_NO_SUSPEND should not be configured for system wakeup using enable_irq_wake(). >>> >>> what is your preference? >>> >>> Is it good enough to not use IRQF_NO_SUSPEND but use enable_irq_wake() instead to >>> enable system wakeup for that IRQ. >> >> I'm sorry for confusion about usage both IRQF_NO_SUSPEND and enable_irq_wake(). >> If suspend() function in device driver executes the enable_irq_wake(), >> IRQF_NO_SUSPEND flag is not necessary. >> >> I think that we better use enable_irq_wake() instead of adding IRQF_NO_SUSPEND flag. >> I'll expect to remove IRQF_NO_SUSPEND flag when requesting gpio interrupt. >> > OK. > >>> >>>> if (device_may_wakeup(dev)) >>>> enable_irq_wake(info->id_irq); >>>> disable_irq(info->id_irq); >>> >>> why do we need to disable irq here? How will the system wakeup if IRQ is disabled? >> >> The disable_irq() may make the interrupt as masking state. >> Although interrput is masking state(disable), interrup can happen. >> but, the interrupt may remain the pending state without discarding it. >> >> And then, >> When resume() function in extcon-usb-gpio.c executes enable_irq(info->id_irq), >> pending interrupt will happen and executes the interrupt handler(usb_irq_handler). >> >> If we don't execute disable_irq() in suspend function, >> info->id->irq interrupt might happen before completing the resume sequence >> of extcon-gpio-usb driver. > > How will that cause a problem? If an interrupt happens _before_ the system enters > SUSPEND state then kernel should abort the suspend. This should be taken care by > kernel PM core and not the device driver. > > I still fail to understand that we need to call disable_irq() in .suspend() and > enable_irq() in .resume() > > can you point me to any other drivers doing so? You can refer the suspend function in drivers/mfd/max14577.c or drivers/mfd/max77693.c. The max14577_suspend() includes the detailed comment for why using disable_irq() in suspend function. In max14577 case, max14577_suspend() use disable_irq() function because of i2c dependency. If max14577 device is wake-up from suspend state before completing the resume sequence of i2c, max14577 may fail to read/write i2c communication. Thanks, Chanwoo Choi -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Chanwoo, On 28/01/15 04:19, Chanwoo Choi wrote: > Hi Roger, > > On 01/28/2015 12:38 AM, Roger Quadros wrote: >> Chanwoo, >> >> On 27/01/15 03:54, Chanwoo Choi wrote: >>> Hi Roger, >>> >>> On 01/27/2015 01:27 AM, Roger Quadros wrote: >>>> Hi Chanwoo, >>>> >>>> All your comments are valid. Need some clarification on one comment. >>>> >>>> On 26/01/15 15:56, Chanwoo Choi wrote: >>>>> Hi Roger, >>>>> >>>>> This patch looks good to me. But I add some comment. >>>>> If you modify some comment, I'll apply this patch on 3.21 queue. >>>>> >>>>> On Mon, Jan 26, 2015 at 9:15 PM, Roger Quadros <rogerq@ti.com> wrote: >>>>>> This driver observes the USB ID pin connected over a GPIO and >>>>>> updates the USB cable extcon states accordingly. >>>>>> >>>>>> The existing GPIO extcon driver is not suitable for this purpose >>>>>> as it needs to be taught to understand USB cable states and it >>>>>> can't handle more than one cable per instance. >>>>>> >>>>>> For the USB case we need to handle 2 cable states. >>>>>> 1) USB (attach/detach) >>>>>> 2) USB-Host (attach/detach) >>>>>> >>>>>> This driver can be easily updated in the future to handle VBUS >>>>>> events in case it happens to be available on GPIO for any platform. >>>>>> >>>>>> Signed-off-by: Roger Quadros <rogerq@ti.com> >>>>>> --- >>>>>> .../devicetree/bindings/extcon/extcon-usb-gpio.txt | 20 ++ >>>>>> drivers/extcon/Kconfig | 7 + >>>>>> drivers/extcon/Makefile | 1 + >>>>>> drivers/extcon/extcon-usb-gpio.c | 214 +++++++++++++++++++++ >>>>>> 4 files changed, 242 insertions(+) >>>>>> create mode 100644 Documentation/devicetree/bindings/extcon/extcon-usb-gpio.txt >>>>>> create mode 100644 drivers/extcon/extcon-usb-gpio.c >>>>>> >>>> >>>> <snip> >>>> >>>>>> + >>>>>> +static int usb_extcon_probe(struct platform_device *pdev) >>>>>> +{ >>>>>> + struct device *dev = &pdev->dev; >>>>>> + struct device_node *np = dev->of_node; >>>>>> + struct usb_extcon_info *info; >>>>>> + int ret; >>>>>> + >>>>>> + if (!np) >>>>>> + return -EINVAL; >>>>>> + >>>>>> + info = devm_kzalloc(&pdev->dev, sizeof(*info), GFP_KERNEL); >>>>>> + if (!info) >>>>>> + return -ENOMEM; >>>>>> + >>>>>> + info->dev = dev; >>>>>> + info->id_gpiod = devm_gpiod_get(&pdev->dev, "id"); >>>>>> + if (IS_ERR(info->id_gpiod)) { >>>>>> + dev_err(dev, "failed to get ID GPIO\n"); >>>>>> + return PTR_ERR(info->id_gpiod); >>>>>> + } >>>>>> + >>>>>> + ret = gpiod_set_debounce(info->id_gpiod, >>>>>> + USB_GPIO_DEBOUNCE_MS * 1000); >>>>>> + if (ret < 0) >>>>>> + info->debounce_jiffies = msecs_to_jiffies(USB_GPIO_DEBOUNCE_MS); >>>>>> + >>>>>> + INIT_DELAYED_WORK(&info->wq_detcable, usb_extcon_detect_cable); >>>>>> + >>>>>> + info->id_irq = gpiod_to_irq(info->id_gpiod); >>>>>> + if (info->id_irq < 0) { >>>>>> + dev_err(dev, "failed to get ID IRQ\n"); >>>>>> + return info->id_irq; >>>>>> + } >>>>>> + >>>>>> + ret = devm_request_threaded_irq(dev, info->id_irq, NULL, >>>>>> + usb_irq_handler, >>>>>> + IRQF_SHARED | IRQF_ONESHOT | >>>>>> + IRQF_NO_SUSPEND, >>>>>> + pdev->name, info); >>>> >>>> use of IRQF_NO_SUSPEND is not recommended to be used together with IRQF_SHARED so >>>> I'll remove IRQF_SHARED from here if we decide to stick with IRQF_NO_SUSPEND. >>>> More on this below. >>>> >>>>>> + if (ret < 0) { >>>>>> + dev_err(dev, "failed to request handler for ID IRQ\n"); >>>>>> + return ret; >>>>>> + } >>>>>> + >>>>>> + info->edev = devm_extcon_dev_allocate(dev, usb_extcon_cable); >>>>>> + if (IS_ERR(info->edev)) { >>>>>> + dev_err(dev, "failed to allocate extcon device\n"); >>>>>> + return -ENOMEM; >>>>>> + } >>>>>> + >>>>>> + ret = devm_extcon_dev_register(dev, info->edev); >>>>>> + if (ret < 0) { >>>>>> + dev_err(dev, "failed to register extcon device\n"); >>>>>> + return ret; >>>>>> + } >>>>>> + >>>>>> + platform_set_drvdata(pdev, info); >>>>> >>>>> I prefer to execute the device_init_wakeup() function as following >>>>> for suspend/resume function: >>>>> device_init_wakeup(&pdev->dev, 1); >>>>> >>>>>> + >>>>>> + /* Perform initial detection */ >>>>>> + usb_extcon_detect_cable(&info->wq_detcable.work); >>>>>> + >>>>>> + return 0; >>>>>> +} >>>>>> + >>>>>> +static int usb_extcon_remove(struct platform_device *pdev) >>>>>> +{ >>>>>> + struct usb_extcon_info *info = platform_get_drvdata(pdev); >>>>>> + >>>>>> + cancel_delayed_work_sync(&info->wq_detcable); >>>>> >>>>> Need to add blank line. >>>>> >>>>>> + return 0; >>>>>> +} >>>>>> + >>>>>> +#ifdef CONFIG_PM_SLEEP >>>>>> +static int usb_extcon_suspend(struct device *dev) >>>>>> +{ >>>>>> + struct usb_extcon_info *info = dev_get_drvdata(dev); >>>>>> + >>>>>> + enable_irq_wake(info->id_irq); >>>>> >>>>> I prefer to use device_may_wakeup() function for whether >>>>> executing enable_irq_wake() or not. Also, The disable_irq() >>>>> in the suspend function would prevent us from discarding interrupt >>>>> before wakeup from suspend completely. >>>>> >>>> >>>> I need more clarification here. >>>> >>>> If we are going to use enable_irq_wake() here then what is the point of IRQF_NO_SUSPEND? >>>> >>>> >From Documentation/power/suspend-and-interrupts.txt I see that interrupts marked >>>> as IRQF_NO_SUSPEND should not be configured for system wakeup using enable_irq_wake(). >>>> >>>> what is your preference? >>>> >>>> Is it good enough to not use IRQF_NO_SUSPEND but use enable_irq_wake() instead to >>>> enable system wakeup for that IRQ. >>> >>> I'm sorry for confusion about usage both IRQF_NO_SUSPEND and enable_irq_wake(). >>> If suspend() function in device driver executes the enable_irq_wake(), >>> IRQF_NO_SUSPEND flag is not necessary. >>> >>> I think that we better use enable_irq_wake() instead of adding IRQF_NO_SUSPEND flag. >>> I'll expect to remove IRQF_NO_SUSPEND flag when requesting gpio interrupt. >>> >> OK. >> >>>> >>>>> if (device_may_wakeup(dev)) >>>>> enable_irq_wake(info->id_irq); >>>>> disable_irq(info->id_irq); >>>> >>>> why do we need to disable irq here? How will the system wakeup if IRQ is disabled? >>> >>> The disable_irq() may make the interrupt as masking state. >>> Although interrput is masking state(disable), interrup can happen. >>> but, the interrupt may remain the pending state without discarding it. >>> >>> And then, >>> When resume() function in extcon-usb-gpio.c executes enable_irq(info->id_irq), >>> pending interrupt will happen and executes the interrupt handler(usb_irq_handler). >>> >>> If we don't execute disable_irq() in suspend function, >>> info->id->irq interrupt might happen before completing the resume sequence >>> of extcon-gpio-usb driver. >> >> How will that cause a problem? If an interrupt happens _before_ the system enters >> SUSPEND state then kernel should abort the suspend. This should be taken care by >> kernel PM core and not the device driver. >> >> I still fail to understand that we need to call disable_irq() in .suspend() and >> enable_irq() in .resume() >> >> can you point me to any other drivers doing so? > > You can refer the suspend function in drivers/mfd/max14577.c or drivers/mfd/max77693.c. > The max14577_suspend() includes the detailed comment for why using disable_irq() in suspend function. > > In max14577 case, max14577_suspend() use disable_irq() function because of i2c dependency. > If max14577 device is wake-up from suspend state before completing the resume sequence > of i2c, max14577 may fail to read/write i2c communication. Thanks for this information. I will add disable/enable_irq() in suspend/resume(). cheers, -roger -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* Roger Quadros <rogerq@ti.com> [150128 04:15]: > On 28/01/15 04:19, Chanwoo Choi wrote: > >> > >> I still fail to understand that we need to call disable_irq() in .suspend() and > >> enable_irq() in .resume() > >> > >> can you point me to any other drivers doing so? > > > > You can refer the suspend function in drivers/mfd/max14577.c or drivers/mfd/max77693.c. > > The max14577_suspend() includes the detailed comment for why using disable_irq() in suspend function. > > > > In max14577 case, max14577_suspend() use disable_irq() function because of i2c dependency. > > If max14577 device is wake-up from suspend state before completing the resume sequence > > of i2c, max14577 may fail to read/write i2c communication. > > Thanks for this information. I will add disable/enable_irq() in suspend/resume(). Are the .dts changes safe for me to apply already? Regards, Tony -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 28/01/15 19:09, Tony Lindgren wrote: > * Roger Quadros <rogerq@ti.com> [150128 04:15]: >> On 28/01/15 04:19, Chanwoo Choi wrote: >>>> >>>> I still fail to understand that we need to call disable_irq() in .suspend() and >>>> enable_irq() in .resume() >>>> >>>> can you point me to any other drivers doing so? >>> >>> You can refer the suspend function in drivers/mfd/max14577.c or drivers/mfd/max77693.c. >>> The max14577_suspend() includes the detailed comment for why using disable_irq() in suspend function. >>> >>> In max14577 case, max14577_suspend() use disable_irq() function because of i2c dependency. >>> If max14577 device is wake-up from suspend state before completing the resume sequence >>> of i2c, max14577 may fail to read/write i2c communication. >> >> Thanks for this information. I will add disable/enable_irq() in suspend/resume(). > > Are the .dts changes safe for me to apply already? > Yes Tony, you can pick them. Thanks. cheers, -roger -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* Roger Quadros <rogerq@ti.com> [150129 03:34]: > On 28/01/15 19:09, Tony Lindgren wrote: > > * Roger Quadros <rogerq@ti.com> [150128 04:15]: > >> On 28/01/15 04:19, Chanwoo Choi wrote: > >>>> > >>>> I still fail to understand that we need to call disable_irq() in .suspend() and > >>>> enable_irq() in .resume() > >>>> > >>>> can you point me to any other drivers doing so? > >>> > >>> You can refer the suspend function in drivers/mfd/max14577.c or drivers/mfd/max77693.c. > >>> The max14577_suspend() includes the detailed comment for why using disable_irq() in suspend function. > >>> > >>> In max14577 case, max14577_suspend() use disable_irq() function because of i2c dependency. > >>> If max14577 device is wake-up from suspend state before completing the resume sequence > >>> of i2c, max14577 may fail to read/write i2c communication. > >> > >> Thanks for this information. I will add disable/enable_irq() in suspend/resume(). > > > > Are the .dts changes safe for me to apply already? > > > > Yes Tony, you can pick them. Thanks. OK will apply the dts changes into omap-for-v3.20/dt thanks. I have also the defconfig changes tagged, will apply those a bit later probably as a fix after the driver is merged. Regards, Tony -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 29/01/15 18:56, Tony Lindgren wrote: > * Roger Quadros <rogerq@ti.com> [150129 03:34]: >> On 28/01/15 19:09, Tony Lindgren wrote: >>> * Roger Quadros <rogerq@ti.com> [150128 04:15]: >>>> On 28/01/15 04:19, Chanwoo Choi wrote: >>>>>> >>>>>> I still fail to understand that we need to call disable_irq() in .suspend() and >>>>>> enable_irq() in .resume() >>>>>> >>>>>> can you point me to any other drivers doing so? >>>>> >>>>> You can refer the suspend function in drivers/mfd/max14577.c or drivers/mfd/max77693.c. >>>>> The max14577_suspend() includes the detailed comment for why using disable_irq() in suspend function. >>>>> >>>>> In max14577 case, max14577_suspend() use disable_irq() function because of i2c dependency. >>>>> If max14577 device is wake-up from suspend state before completing the resume sequence >>>>> of i2c, max14577 may fail to read/write i2c communication. >>>> >>>> Thanks for this information. I will add disable/enable_irq() in suspend/resume(). >>> >>> Are the .dts changes safe for me to apply already? >>> >> >> Yes Tony, you can pick them. Thanks. > > OK will apply the dts changes into omap-for-v3.20/dt thanks. > I have also the defconfig changes tagged, will apply those > a bit later probably as a fix after the driver is merged. Sounds good to me. Thanks Tony. cheers, -roger -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/extcon/extcon-usb-gpio.txt b/Documentation/devicetree/bindings/extcon/extcon-usb-gpio.txt new file mode 100644 index 0000000..ab52a85 --- /dev/null +++ b/Documentation/devicetree/bindings/extcon/extcon-usb-gpio.txt @@ -0,0 +1,20 @@ +USB GPIO Extcon device + +This is a virtual device used to generate USB cable states from the USB ID pin +connected to a GPIO pin. + +Required properties: +- compatible: Should be "linux,extcon-usb-gpio" +- id-gpio: gpio for USB ID pin. See gpio binding. + +Example: + extcon_usb1 { + compatible = "linux,extcon-usb-gpio"; + id-gpio = <&gpio6 1 GPIO_ACTIVE_HIGH>; + } + + usb@1 { + ... + extcon = <&extcon_usb1>; + ... + }; diff --git a/drivers/extcon/Kconfig b/drivers/extcon/Kconfig index 6a1f7de..e4c01ab 100644 --- a/drivers/extcon/Kconfig +++ b/drivers/extcon/Kconfig @@ -93,4 +93,11 @@ config EXTCON_SM5502 Silicon Mitus SM5502. The SM5502 is a USB port accessory detector and switch. +config EXTCON_USB_GPIO + tristate "USB GPIO extcon support" + depends on GPIOLIB + help + Say Y here to enable GPIO based USB cable detection extcon support. + Used typically if GPIO is used for USB ID pin detection. + endif # MULTISTATE_SWITCH diff --git a/drivers/extcon/Makefile b/drivers/extcon/Makefile index 0370b42..6a08a98 100644 --- a/drivers/extcon/Makefile +++ b/drivers/extcon/Makefile @@ -12,3 +12,4 @@ obj-$(CONFIG_EXTCON_MAX8997) += extcon-max8997.o obj-$(CONFIG_EXTCON_PALMAS) += extcon-palmas.o obj-$(CONFIG_EXTCON_RT8973A) += extcon-rt8973a.o obj-$(CONFIG_EXTCON_SM5502) += extcon-sm5502.o +obj-$(CONFIG_EXTCON_USB_GPIO) += extcon-usb-gpio.o diff --git a/drivers/extcon/extcon-usb-gpio.c b/drivers/extcon/extcon-usb-gpio.c new file mode 100644 index 0000000..a20aa39 --- /dev/null +++ b/drivers/extcon/extcon-usb-gpio.c @@ -0,0 +1,214 @@ +/** + * drivers/extcon/extcon_usb_gpio.c - USB GPIO extcon driver + * + * Copyright (C) 2015 Texas Instruments Incorporated - http://www.ti.com + * Author: Roger Quadros <rogerq@ti.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/extcon.h> +#include <linux/init.h> +#include <linux/interrupt.h> +#include <linux/irq.h> +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/of_gpio.h> +#include <linux/platform_device.h> +#include <linux/slab.h> +#include <linux/workqueue.h> + +#define USB_GPIO_DEBOUNCE_MS 20 /* ms */ + +struct usb_extcon_info { + struct device *dev; + struct extcon_dev *edev; + + struct gpio_desc *id_gpiod; + int id_irq; + + unsigned long debounce_jiffies; + struct delayed_work wq_detcable; +}; + +/* List of detectable cables */ +enum { + EXTCON_CABLE_USB = 0, + EXTCON_CABLE_USB_HOST, + + EXTCON_CABLE_END, +}; + +static const char *usb_extcon_cable[] = { + [EXTCON_CABLE_USB] = "USB", + [EXTCON_CABLE_USB_HOST] = "USB-Host", + NULL, +}; + +static void usb_extcon_detect_cable(struct work_struct *work) +{ + int id; + struct usb_extcon_info *info; + + info = container_of(to_delayed_work(work), struct usb_extcon_info, + wq_detcable); + + /* check ID and update cable state */ + id = gpiod_get_value_cansleep(info->id_gpiod); + if (id) { + /* + * ID = 1 means USB HOST cable detached. + * As we don't have event for USB peripheral cable attached, + * we simulate USB peripheral attach here. + */ + extcon_set_cable_state(info->edev, + usb_extcon_cable[EXTCON_CABLE_USB_HOST], + false); + extcon_set_cable_state(info->edev, + usb_extcon_cable[EXTCON_CABLE_USB], + true); + } else { + /* + * ID = 0 means USB HOST cable attached. + * As we don't have event for USB peripheral cable detached, + * we simulate USB peripheral detach here. + */ + extcon_set_cable_state(info->edev, + usb_extcon_cable[EXTCON_CABLE_USB], + false); + extcon_set_cable_state(info->edev, + usb_extcon_cable[EXTCON_CABLE_USB_HOST], + true); + } +} + +static irqreturn_t usb_irq_handler(int irq, void *dev_id) +{ + struct usb_extcon_info *info = dev_id; + + queue_delayed_work(system_power_efficient_wq, &info->wq_detcable, + info->debounce_jiffies); + return IRQ_HANDLED; +} + +static int usb_extcon_probe(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + struct device_node *np = dev->of_node; + struct usb_extcon_info *info; + int ret; + + if (!np) + return -EINVAL; + + info = devm_kzalloc(&pdev->dev, sizeof(*info), GFP_KERNEL); + if (!info) + return -ENOMEM; + + info->dev = dev; + info->id_gpiod = devm_gpiod_get(&pdev->dev, "id"); + if (IS_ERR(info->id_gpiod)) { + dev_err(dev, "failed to get ID GPIO\n"); + return PTR_ERR(info->id_gpiod); + } + + ret = gpiod_set_debounce(info->id_gpiod, + USB_GPIO_DEBOUNCE_MS * 1000); + if (ret < 0) + info->debounce_jiffies = msecs_to_jiffies(USB_GPIO_DEBOUNCE_MS); + + INIT_DELAYED_WORK(&info->wq_detcable, usb_extcon_detect_cable); + + info->id_irq = gpiod_to_irq(info->id_gpiod); + if (info->id_irq < 0) { + dev_err(dev, "failed to get ID IRQ\n"); + return info->id_irq; + } + + ret = devm_request_threaded_irq(dev, info->id_irq, NULL, + usb_irq_handler, + IRQF_SHARED | IRQF_ONESHOT | + IRQF_NO_SUSPEND, + pdev->name, info); + if (ret < 0) { + dev_err(dev, "failed to request handler for ID IRQ\n"); + return ret; + } + + info->edev = devm_extcon_dev_allocate(dev, usb_extcon_cable); + if (IS_ERR(info->edev)) { + dev_err(dev, "failed to allocate extcon device\n"); + return -ENOMEM; + } + + ret = devm_extcon_dev_register(dev, info->edev); + if (ret < 0) { + dev_err(dev, "failed to register extcon device\n"); + return ret; + } + + platform_set_drvdata(pdev, info); + + /* Perform initial detection */ + usb_extcon_detect_cable(&info->wq_detcable.work); + + return 0; +} + +static int usb_extcon_remove(struct platform_device *pdev) +{ + struct usb_extcon_info *info = platform_get_drvdata(pdev); + + cancel_delayed_work_sync(&info->wq_detcable); + return 0; +} + +#ifdef CONFIG_PM_SLEEP +static int usb_extcon_suspend(struct device *dev) +{ + struct usb_extcon_info *info = dev_get_drvdata(dev); + + enable_irq_wake(info->id_irq); + return 0; +} + +static int usb_extcon_resume(struct device *dev) +{ + struct usb_extcon_info *info = dev_get_drvdata(dev); + + disable_irq_wake(info->id_irq); + return 0; +} +#endif + +static SIMPLE_DEV_PM_OPS(usb_extcon_pm_ops, + usb_extcon_suspend, usb_extcon_resume); + +static struct of_device_id usb_extcon_dt_match[] = { + { .compatible = "linux,extcon-usb-gpio", }, + { /* end */ } +}; +MODULE_DEVICE_TABLE(of, usb_extcon_dt_match); + +static struct platform_driver usb_extcon_driver = { + .probe = usb_extcon_probe, + .remove = usb_extcon_remove, + .driver = { + .name = "extcon-usb-gpio", + .pm = &usb_extcon_pm_ops, + .of_match_table = usb_extcon_dt_match, + }, +}; + +module_platform_driver(usb_extcon_driver); + +MODULE_AUTHOR("Roger Quadros <rogerq@ti.com>"); +MODULE_DESCRIPTION("USB GPIO extcon driver"); +MODULE_LICENSE("GPL v2");
This driver observes the USB ID pin connected over a GPIO and updates the USB cable extcon states accordingly. The existing GPIO extcon driver is not suitable for this purpose as it needs to be taught to understand USB cable states and it can't handle more than one cable per instance. For the USB case we need to handle 2 cable states. 1) USB (attach/detach) 2) USB-Host (attach/detach) This driver can be easily updated in the future to handle VBUS events in case it happens to be available on GPIO for any platform. Signed-off-by: Roger Quadros <rogerq@ti.com> --- .../devicetree/bindings/extcon/extcon-usb-gpio.txt | 20 ++ drivers/extcon/Kconfig | 7 + drivers/extcon/Makefile | 1 + drivers/extcon/extcon-usb-gpio.c | 214 +++++++++++++++++++++ 4 files changed, 242 insertions(+) create mode 100644 Documentation/devicetree/bindings/extcon/extcon-usb-gpio.txt create mode 100644 drivers/extcon/extcon-usb-gpio.c