Re: [PATCH v2 1/7] extcon: usb-gpio: Introduce gpio usb extcon driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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@xxxxxx> 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@xxxxxx>
>>>>>> ---
>>>>>>  .../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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]
  Powered by Linux