lkml.org 
[lkml]   [2013]   [Aug]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 1/2] extcon: extcon-dra7xx: Add extcon driver for USB ID detection
On 8/20/2013 3:59 PM, Chanwoo Choi wrote:
> Hi George,
>
>>>> diff --git a/Documentation/devicetree/bindings/extcon/extcon-dra7xx.txt b/Documentation/devicetree/bindings/extcon/extcon-dra7xx.txt
>>>> new file mode 100644
>>>> index 0000000..37e4c22
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/extcon/extcon-dra7xx.txt
>>>> @@ -0,0 +1,19 @@
>>>> +EXTCON FOR DRA7xx
>>>> +
>>>> +Required Properties:
>>>> + - compatible : Should be "ti,dra7xx-usb"
>>>> + - gpios : phandle to ID pin and interrupt gpio.
>>>> +
>>>> +Optional Properties:
>>>> + - interrupt-parent : interrupt controller phandle
>>>> + - interrupts : interrupt number
>>>> +
>>>> +
>>>> +dra7x_extcon1 {
>>> You used 'dra7xx-usb' device name. Why did you use 'dra7x_extcon1' name?
>>> What is meaning 'dra7x_extcon1'?
>> I will rename it to dra7xx_extcon.
> extcon naming means various external connector device. e.g., usb, jack, etc...
> So, I prefer 'dra7xx_usb' instead of 'dra7xx_extcon'. I have plan to divide
> extcon device driver according to the kind of device.

okay will do it in v2
>
>
>>>> +static int id_poll_func(void *data)
>>>> +{
>>>> + struct dra7xx_usb *dra7xx_usb = (struct dra7xx_usb *) data;
>>>> +
>>>> + allow_signal(SIGINT);
>>>> + allow_signal(SIGTERM);
>>>> + allow_signal(SIGKILL);
>>>> + allow_signal(SIGUSR1);
>>>> +
>>>> + set_freezable();
>>>> +
>>>> + while (!kthread_should_stop()) {
>>>> + dra7xx_usb->id_current = gpio_get_value_cansleep
>>>> + (dra7xx_usb->id_gpio);
>>>> + if (dra7xx_usb->id_current == dra7xx_usb->id_prev) {
>>>> + schedule_timeout_interruptible
>>>> + (msecs_to_jiffies(2*1000));
>>>> + continue;
>>>> + } else if (dra7xx_usb->id_current == 0) {
>>>> + extcon_set_cable_state(&dra7xx_usb->edev, "USB", false);
>>>> + extcon_set_cable_state(&dra7xx_usb->edev,
>>>> + "USB-HOST", true);
>>>> + } else {
>>>> + extcon_set_cable_state(&dra7xx_usb->edev,
>>>> + "USB-HOST", false);
>>>> + extcon_set_cable_state(&dra7xx_usb->edev, "USB", true);
>>>> + }
>>> Should dra7xx_usb keep always connected state with either USB or USB-HOST cable?
>>> I don't understand. So please explain detailed operation method of dra7xx_usb device.
>> In dra7xx only ID pin is connected to the SoC gpio. There is no way, currently to detect the VBUS on/off.
>> So I always default to either HOST/DEVICE mode solely depending on the ID pin value.
> OK.
>
> But I don't want to use kthread with polling method.
> I recommend that you use interrupt method for cable detection.
> All of extcon device driver have only used interrupt method without polling.

okay will remove the polling thread.
>
>>>> + dra7xx_usb->id_prev = dra7xx_usb->id_current;
>>>> + }
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static irqreturn_t id_irq_handler(int irq, void *data)
>>>> +{
>>>> + struct dra7xx_usb *dra7xx_usb = (struct dra7xx_usb *) data;
>>>> +
>>>> + dra7xx_usb->id_current = gpio_get_value_cansleep(dra7xx_usb->id_gpio);
>>>> +
>>>> + if (dra7xx_usb->id_current == dra7xx_usb->id_prev) {
>>>> + return IRQ_NONE;
>>>> + } else if (dra7xx_usb->id_current == 0) {
> You should define some constant variable to clarify '0' meaning instead of using '0' directly.

okay
>
>>>> + dra7xx_usb = devm_kzalloc(&pdev->dev, sizeof(*dra7xx_usb), GFP_KERNEL);
>>>> + if (!dra7xx_usb)
>>>> + return -ENOMEM;
>>> You have to add error message with dev_err().
>> devm_kzalloc itself should give some message.
> ok.
>
>
>>>> + status = extcon_dev_register(&dra7xx_usb->edev, dra7xx_usb->dev);
>>>> + if (status) {
>>>> + dev_err(&pdev->dev, "failed to register extcon device\n");
>>>> + return status;
>>> You should restore previous operation about dra7xx_usb->irq_gpio.
>> okay
>>>> + }
>>>> +
>>>> + dra7xx_usb->id_prev = gpio_get_value_cansleep(dra7xx_usb->id_gpio);
>>>> + if (dra7xx_usb->id_prev) {
> ditto.
> You should define some constant variable to clarify 'dra7xx_usb->id_prev' meaning.

okay
>
>>>> + extcon_set_cable_state(&dra7xx_usb->edev, "USB-HOST", false);
>>>> + extcon_set_cable_state(&dra7xx_usb->edev, "USB", true);
>>>> + } else {
>>>> + extcon_set_cable_state(&dra7xx_usb->edev, "USB", false);
>>>> + extcon_set_cable_state(&dra7xx_usb->edev, "USB-HOST", true);
>>>> + }
>>> why did you do keep always connected state?
>> There is no way, currently to detect the VBUS on/off.
>> So I always default to either HOST/DEVICE mode solely depending on the ID pin value.
>>
>>>> +
>>>> + if (dra7xx_usb->irq_gpio) {
>>>> + status = devm_request_threaded_irq(dra7xx_usb->dev, irq_num,
>>>> + NULL, id_irq_handler, IRQF_SHARED |
>>>> + IRQF_ONESHOT | IRQF_TRIGGER_FALLING,
>>>> + dev_name(&pdev->dev), (void *) dra7xx_usb);
>>>> + if (status)
>>>> + dev_err(dra7xx_usb->dev, "failed to request irq #%d\n",
>>>> + irq_num);
>>> If devm_request_threaded_irq() return fail state, why did not you do add error exception?
>> If interrupt fails I fallback to polling thread.
>>>> + else
>>>> + return 0;
>>> If devm_request_threaded_irq() return success state, why did you directly call 'return'?
>>> kthread_create operation isn't necessary?
>> Yes kthread is optional. Some boards doenot have the ID pin hooked onto the GPIO.
>> In such cases we will run the kthread and poll on the ID pin values.
>>>> + }
>>>> +
>>>> + dra7xx_usb->thread_task = kthread_create(id_poll_func, dra7xx_usb,
>>>> + dev_name(&pdev->dev));
>>> Should you use polling method with kthread? I think it isn't proper method.
>>> You did get the irq number by using DT helper function and register irq handler
>>> with devm_request_threaded_irq(). I prefer interrupt method for detection of cable state.
>> I also prefer interrupt method. If any implementation does not have ID pin connected to GPIOs then still
>> it could use the driver in polling mode.
> As I mentioned above, I don't prefer interrupt method for cable detection.

I hope you meant, you prefer interrupt method for cable detection over
polling .
> Polling method for detection isn't appropriate for extcon device driver.
>
> Instead, I will consider whether to support polling method or not on extcon core.
>
> Thanks,
> Chanwoo Choi
>
>

--
-George



\
 
 \ /
  Last update: 2013-08-20 16:01    [W:0.102 / U:0.536 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site