Re: [PATCH 1/2] extcon: extcon-dra7xx: Add extcon driver for USB ID detection

Re: [PATCH 1/2] extcon: extcon-dra7xx: Add extcon driver for USB ID detection

[Stephen Warren]

On 08/16/2013 04:13 AM, George Cherian wrote:
> Adding extcon driver for USB ID detection to dynamically
> configure USB Host/Peripheral mode.

> diff --git a/Documentation/devicetree/bindings/extcon/extcon-dra7xx.txt b/Documentation/devicetree/bindings/extcon/extcon-dra7xx.txt

> +EXTCON FOR DRA7xx
> +
> +Required Properties:

Please at lest explain what a DRA7xxx is, and the purpose of the HW
module this binding describes.

> + - compatible : Should be "ti,dra7xx-usb"

If this is a USB VID detector rather than a full USB host controller,
then "usb" in the binding is a bit over-reaching. Perhaps "-usb-vid" or
"-usb-vid-detector" would be more accurate.

> + - gpios : phandle to ID pin and interrupt gpio.

This isn't just a phandle; it's phandle+args, or a GPIO specifier. Some
reference should be made to ../gpio/gpio.txt for the format.

Why does the interrupt line need to be included in a list of GPIOs?

If the DRA7xxx is a VID detector, why does it even need/have any GPIOs
associated with it; surely it has a dedicated VID input pin. Can you
provide more details re: how the HW is structured.

> +Optional Properties:
> +  - interrupt-parent : interrupt controller phandle
> +  - interrupts : interrupt number
> +
> +

It's typical insert the following between those two blank lines:

Example:

... or delete one of the blank lines.

> +dra7x_extcon1 {
> +		compatible = "ti,dra7xx-usb";
> +		gpios = <&pcf_usb 1 0>,
> +			<&gpio6 11 2>;
> +		interrupt-parent = <&gpio6>;
> +		interrupts = <11>;
> +	};
> +

No need for that trailing blank line.

Re: [PATCH 1/2] extcon: extcon-dra7xx: Add extcon driver for USB ID detection

[George Cherian]

Hi Stephen,

Thanks for your review.

On 8/20/2013 1:01 AM, Stephen Warren wrote:

> On 08/16/2013 04:13 AM, George Cherian wrote:
>> Adding extcon driver for USB ID detection to dynamically
>> configure USB Host/Peripheral mode.
>> diff --git a/Documentation/devicetree/bindings/extcon/extcon-dra7xx.txt b/Documentation/devicetree/bindings/extcon/extcon-dra7xx.txt
>> +EXTCON FOR DRA7xx
>> +
>> +Required Properties:
> Please at lest explain what a DRA7xxx is, and the purpose of the HW
> module this binding describes.

DRA7xx is the SoC name and the USB VID  detection is implemented via gpio's
Basically it does only ID detection via GPIO and there is no VBUS 
detection in h/w.
>
>> + - compatible : Should be "ti,dra7xx-usb"
> If this is a USB VID detector rather than a full USB host controller,
> then "usb" in the binding is a bit over-reaching. Perhaps "-usb-vid" or
> "-usb-vid-detector" would be more accurate.

This will be renamed to dra7xx-extcon.
>
>> + - gpios : phandle to ID pin and interrupt gpio.
> This isn't just a phandle; it's phandle+args, or a GPIO specifier. Some
> reference should be made to ../gpio/gpio.txt for the format.

okay
> Why does the interrupt line need to be included in a list of GPIOs?

ID pins are connected to pcf8575, and the pcf8575's interrupt line is 
inturn connected to
gpio bank6 pin 11, we use this gpio interrupt to detect the ID pin change.
> If the DRA7xxx is a VID detector, why does it even need/have any GPIOs
> associated with it; surely it has a dedicated VID input pin. Can you
> provide more details re: how the HW is structured.
>
>> +Optional Properties:
>> +  - interrupt-parent : interrupt controller phandle
>> +  - interrupts : interrupt number
>> +
>> +
> It's typical insert the following between those two blank lines:
>
> Example:
>
> ... or delete one of the blank lines.
>
>> +dra7x_extcon1 {
>> +		compatible = "ti,dra7xx-usb";
>> +		gpios = <&pcf_usb 1 0>,
>> +			<&gpio6 11 2>;
>> +		interrupt-parent = <&gpio6>;
>> +		interrupts = <11>;
>> +	};
>> +
> No need for that trailing blank line.

okay
>


-- 
-George

Re: [PATCH 1/2] extcon: extcon-dra7xx: Add extcon driver for USB ID detection

[Stephen Warren]

On 08/20/2013 12:55 AM, George Cherian wrote:
> Hi Stephen,
> 
> Thanks for your review.
> 
> On 8/20/2013 1:01 AM, Stephen Warren wrote:
> 
>> On 08/16/2013 04:13 AM, George Cherian wrote:
>>> Adding extcon driver for USB ID detection to dynamically
>>> configure USB Host/Peripheral mode.
>>> diff --git
>>> a/Documentation/devicetree/bindings/extcon/extcon-dra7xx.txt
>>> b/Documentation/devicetree/bindings/extcon/extcon-dra7xx.txt
>>> +EXTCON FOR DRA7xx
>>> +
>>> +Required Properties:
>> Please at lest explain what a DRA7xxx is, and the purpose of the HW
>> module this binding describes.
> 
> DRA7xx is the SoC name and the USB VID  detection is implemented via gpio's
> Basically it does only ID detection via GPIO and there is no VBUS
> detection in h/w.

If there's no SoC-specific HW, then the binding has nothing to do with
the SoC; it's entirely generic.

I think instead, you need to define a generic "gpio-usb-vid" binding
instead.

Otherwise, ever SoC that doesn't have explicit USB VID detection HW is
going to re-invent the exact same binding, with pointless differences,
rather than using a single generic binding.

>>> + - compatible : Should be "ti,dra7xx-usb"
>>
>> If this is a USB VID detector rather than a full USB host controller,
>> then "usb" in the binding is a bit over-reaching. Perhaps "-usb-vid" or
>> "-usb-vid-detector" would be more accurate.
> 
> This will be renamed to dra7xx-extcon.

So no, that rename doesn't sound correct.

>>> + - gpios : phandle to ID pin and interrupt gpio.
>>
>> Why does the interrupt line need to be included in a list of GPIOs?
> 
> ID pins are connected to pcf8575, and the pcf8575's interrupt line is
> inturn connected to
> gpio bank6 pin 11, we use this gpio interrupt to detect the ID pin change.

In that case, the PCF8575 node needs to be a GPIO controller and an IRQ
controller, as does the driver for the PCF8575. This binding should have
a single entry in the gpios property, and the driver can call
gpio_to_irq() on that so it knows which IRQ to request.

Re: [PATCH 1/2] extcon: extcon-dra7xx: Add extcon driver for USB ID detection

[George Cherian]

Hi Stephen,

On 8/20/2013 10:23 PM, Stephen Warren wrote:
>> >ID pins are connected to pcf8575, and the pcf8575's interrupt line is
>> >inturn connected to
>> >gpio bank6 pin 11, we use this gpio interrupt to detect the ID pin change.
> In that case, the PCF8575 node needs to be a GPIO controller and an IRQ
> controller, as does the driver for the PCF8575. This binding should have
> a single entry in the gpios property, and the driver can call
> gpio_to_irq() on that so it knows which IRQ to request.

You meant some thing like this?

        pcf_usb: pcf8575@21 {
                 compatible = "ti,pcf8575";
                 reg = <0x21>;
                 gpio-controller;
                 #gpio-cells = <2>;
                 interrupt-parent = <&gpio6>;
                 interrupts = <11 2>;
                 interrupt-controller;
                 #interrupt-cells = <2>;
         };

	 usb_vid_gpio {
                 compatible = "ti,dra7xx-usb";
                 gpios = <&pcf_usb 1 0>;
              
         };


-- 
-George

Re: [PATCH 1/2] extcon: extcon-dra7xx: Add extcon driver for USB ID detection

[Stephen Warren]

On 08/21/2013 07:06 AM, George Cherian wrote:
> Hi Stephen,
> 
> On 8/20/2013 10:23 PM, Stephen Warren wrote:
>>> >ID pins are connected to pcf8575, and the pcf8575's interrupt line is
>>> >inturn connected to
>>> >gpio bank6 pin 11, we use this gpio interrupt to detect the ID pin
>>> change.
>> In that case, the PCF8575 node needs to be a GPIO controller and an IRQ
>> controller, as does the driver for the PCF8575. This binding should have
>> a single entry in the gpios property, and the driver can call
>> gpio_to_irq() on that so it knows which IRQ to request.
> 
> You meant some thing like this?
> 
>        pcf_usb: pcf8575@21 {
>                 compatible = "ti,pcf8575";
>                 reg = <0x21>;
>                 gpio-controller;
>                 #gpio-cells = <2>;
>                 interrupt-parent = <&gpio6>;
>                 interrupts = <11 2>;
>                 interrupt-controller;
>                 #interrupt-cells = <2>;
>      };
> 
>      usb_vid_gpio {
>                 compatible = "ti,dra7xx-usb";
>                 gpios = <&pcf_usb 1 0>;
>      };

Yes.

Except that the compatible value for the usb_vid_gpio node still looks
wrong, since I think that node isn't anything to do with any particular SoC.

Re: [PATCH 1/2] extcon: extcon-dra7xx: Add extcon driver for USB ID detection

[George Cherian]

On 8/21/2013 11:05 PM, Stephen Warren wrote:
> On 08/21/2013 07:06 AM, George Cherian wrote:
>> Hi Stephen,
>>
>> On 8/20/2013 10:23 PM, Stephen Warren wrote:
>>>>> ID pins are connected to pcf8575, and the pcf8575's interrupt line is
>>>>> inturn connected to
>>>>> gpio bank6 pin 11, we use this gpio interrupt to detect the ID pin
>>>> change.
>>> In that case, the PCF8575 node needs to be a GPIO controller and an IRQ
>>> controller, as does the driver for the PCF8575. This binding should have
>>> a single entry in the gpios property, and the driver can call
>>> gpio_to_irq() on that so it knows which IRQ to request.
>> You meant some thing like this?
>>
>>         pcf_usb: pcf8575@21 {
>>                  compatible = "ti,pcf8575";
>>                  reg = <0x21>;
>>                  gpio-controller;
>>                  #gpio-cells = <2>;
>>                  interrupt-parent = <&gpio6>;
>>                  interrupts = <11 2>;
>>                  interrupt-controller;
>>                  #interrupt-cells = <2>;
>>       };
>>
>>       usb_vid_gpio {
>>                  compatible = "ti,dra7xx-usb";
>>                  gpios = <&pcf_usb 1 0>;
>>       };
> Yes.
>
> Except that the compatible value for the usb_vid_gpio node still looks
> wrong, since I think that node isn't anything to do with any particular SoC.

Yes will fix that too in v2.


-- 
-George

Re: [PATCH 1/2] extcon: extcon-dra7xx: Add extcon driver for USB ID detection

[Guenter Roeck]

On 08/20/2013 09:53 AM, Stephen Warren wrote:
> On 08/20/2013 12:55 AM, George Cherian wrote:
>> Hi Stephen,
>>
>> Thanks for your review.
>>
>> On 8/20/2013 1:01 AM, Stephen Warren wrote:
>>
>>> On 08/16/2013 04:13 AM, George Cherian wrote:
>>>> Adding extcon driver for USB ID detection to dynamically
>>>> configure USB Host/Peripheral mode.
>>>> diff --git
>>>> a/Documentation/devicetree/bindings/extcon/extcon-dra7xx.txt
>>>> b/Documentation/devicetree/bindings/extcon/extcon-dra7xx.txt
>>>> +EXTCON FOR DRA7xx
>>>> +
>>>> +Required Properties:
>>> Please at lest explain what a DRA7xxx is, and the purpose of the HW
>>> module this binding describes.
>>
>> DRA7xx is the SoC name and the USB VID  detection is implemented via gpio's
>> Basically it does only ID detection via GPIO and there is no VBUS
>> detection in h/w.
>
> If there's no SoC-specific HW, then the binding has nothing to do with
> the SoC; it's entirely generic.
>
I am currently working on adding device tree support to the extcon-gpio driver.

Wonder if it would make sense to just use that driver. As far as I can see
the only missing part is support for multiple cables and cable naming through
device tree properties.

Any thoughts ? The patches are not yet clean enough to submit upstream,
but I could send them as RFC if there is interest.

Guenter

Re: [PATCH 1/2] extcon: extcon-dra7xx: Add extcon driver for USB ID detection

[Chanwoo Choi]

Hi Guenter,

> I am currently working on adding device tree support to the extcon-gpio driver.
> 
> Wonder if it would make sense to just use that driver. As far as I can see
> the only missing part is support for multiple cables and cable naming through
> device tree properties.
> 
> Any thoughts ? The patches are not yet clean enough to submit upstream,
> but I could send them as RFC if there is interest.
> 

I'm considering ways to use and update extcon-gpio driver.
I have interest concerning your approach.
After completing your work, send them as RFC as you mentioned.

Thanks,
Chanwoo Choi

Re: [PATCH 1/2] extcon: extcon-dra7xx: Add extcon driver for USB ID detection

[Guenter Roeck]

On 08/29/2013 05:20 PM, Chanwoo Choi wrote:
> Hi Guenter,
>
>> I am currently working on adding device tree support to the extcon-gpio driver.
>>
>> Wonder if it would make sense to just use that driver. As far as I can see
>> the only missing part is support for multiple cables and cable naming through
>> device tree properties.
>>
>> Any thoughts ? The patches are not yet clean enough to submit upstream,
>> but I could send them as RFC if there is interest.
>>
>
> I'm considering ways to use and update extcon-gpio driver.
> I have interest concerning your approach.
> After completing your work, send them as RFC as you mentioned.
>

It does what we need it to do, so I sent it out a minute ago.
I added possible bindings for multi-cable support as additional RFC.

Guenter

Re: [PATCH 1/2] extcon: extcon-dra7xx: Add extcon driver for USB ID detection

[Chanwoo Choi]

Hi George,

On 08/16/2013 07:13 PM, George Cherian wrote:
> Adding extcon driver for USB ID detection to dynamically
> configure USB Host/Peripheral mode.
> 
> Signed-off-by: George Cherian <george.cherian@ti.com>
> ---
>  .../devicetree/bindings/extcon/extcon-dra7xx.txt   |  19 ++
>  drivers/extcon/Kconfig                             |   7 +
>  drivers/extcon/Makefile                            |   1 +
>  drivers/extcon/extcon-dra7xx.c                     | 234 +++++++++++++++++++++
>  4 files changed, 261 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/extcon/extcon-dra7xx.txt
>  create mode 100644 drivers/extcon/extcon-dra7xx.c
> 
> 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'? 

> +		compatible = "ti,dra7xx-usb";
> +		gpios = <&pcf_usb 1 0>,
> +			<&gpio6 11 2>;
> +		interrupt-parent = <&gpio6>;
> +		interrupts = <11>;
> +	};

You have to keep indentation rule.

> +
> diff --git a/drivers/extcon/Kconfig b/drivers/extcon/Kconfig
> index f1d54a3..b9cf0b2 100644
> --- a/drivers/extcon/Kconfig
> +++ b/drivers/extcon/Kconfig
> @@ -64,4 +64,11 @@ config EXTCON_PALMAS
>  	  Say Y here to enable support for USB peripheral and USB host
>  	  detection by palmas usb.
>  
> +config EXTCON_DRA7XX
> +	tristate "DRA7XX EXTCON support"
> +	help
> +	  Say Y here to enable support for USB peripheral and USB host
> +	  detection by pcf8575 using DRA7XX extcon.

You should explain detailed description about pcf8575 on patch description
and change description of EXTCON_DRA7xx as following:
"using DRA7XX extcon" -> "using DRA7XX device" or "using DRA7XX usb"

> +
> +

Remove unnecessary blank line.

>  endif # MULTISTATE_SWITCH
> diff --git a/drivers/extcon/Makefile b/drivers/extcon/Makefile
> index e4fa8ba..e4778f9 100644
> --- a/drivers/extcon/Makefile
> +++ b/drivers/extcon/Makefile
> @@ -10,3 +10,4 @@ obj-$(CONFIG_EXTCON_MAX77693)	+= extcon-max77693.o
>  obj-$(CONFIG_EXTCON_MAX8997)	+= extcon-max8997.o
>  obj-$(CONFIG_EXTCON_ARIZONA)	+= extcon-arizona.o
>  obj-$(CONFIG_EXTCON_PALMAS)	+= extcon-palmas.o
> +obj-$(CONFIG_EXTCON_DRA7XX)	+= extcon-dra7xx.o
> diff --git a/drivers/extcon/extcon-dra7xx.c b/drivers/extcon/extcon-dra7xx.c
> new file mode 100644
> index 0000000..268c25e
> --- /dev/null
> +++ b/drivers/extcon/extcon-dra7xx.c
> @@ -0,0 +1,234 @@
> +/*
> + * DRA7XX USB ID pin detection driver
> + *
> + * Copyright (C) 2013 Texas Instruments Incorporated - http://www.ti.com
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * Author: George Cherian <george.cherian@ti.com>
> + *
> + * Based on extcon-palmas.c
> + *
> + * Author: Kishon Vijay Abraham I <kishon@ti.com>
> + *
> + * 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/module.h>
> +#include <linux/interrupt.h>
> +#include <linux/kthread.h>
> +#include <linux/freezer.h>
> +#include <linux/platform_device.h>
> +#include <linux/extcon.h>
> +#include <linux/err.h>
> +#include <linux/of.h>
> +#include <linux/gpio.h>
> +#include <linux/of_gpio.h>
> +#include <linux/of_platform.h>
> +
> +struct dra7xx_usb {
> +	struct device *dev;
> +
> +	struct extcon_dev edev;
> +	struct task_struct *thread_task;
> +
> +	/*GPIO pin */

Add space between "/*" and "GPIO".

> +	int id_gpio;
> +	int irq_gpio;
> +
> +	int id_prev;
> +	int id_current;
> +
> +};
> +
> +static const char *dra7xx_extcon_cable[] = {
> +	[0] = "USB",
> +	[1] = "USB-HOST",
> +	NULL,
> +};
> +
> +static const int mutually_exclusive[] = {0x3, 0x0};
> +
> +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.

> +		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) {
> +		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);
> +	}

why did you do keep always connected state?

> +
> +	dra7xx_usb->id_prev = dra7xx_usb->id_current;

Add blank line.

> +	return IRQ_HANDLED;
> +

Remove unnecessary blank line.

> +}
> +
> +static int dra7xx_usb_probe(struct platform_device *pdev)
> +{
> +	struct device_node *node = pdev->dev.of_node;
> +	struct dra7xx_usb *dra7xx_usb;
> +	int status;

'status' local variable is used for return value.
So, I prefer 'ret' variable instead of 'status' name.

> +	int irq_num;
> +	int gpio;
> +
> +	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().

> +
> +

Remove unnecessary blank line.

> +	dra7xx_usb->dev	 = &pdev->dev;
> +
> +	platform_set_drvdata(pdev, dra7xx_usb);
> +
> +	dra7xx_usb->edev.name = dev_name(&pdev->dev);

If edev name is equal with device name, this line is unnecessary.
Because extcon_dev_register() use dev_name(&pdev->dev) as edev name
in extcon-class.c

> +	dra7xx_usb->edev.supported_cable = dra7xx_extcon_cable;
> +	dra7xx_usb->edev.mutually_exclusive = mutually_exclusive;
> +
> +	gpio = of_get_gpio(node, 0);
> +	if (gpio_is_valid(gpio)) {
> +		dra7xx_usb->id_gpio = gpio;
> +		status = gpio_request(dra7xx_usb->id_gpio, "id_gpio");
> +		if (status)
> +			return status;

You have to add error message with dev_err().

> +	} else {
> +		dev_err(&pdev->dev, "failed to get id gpio\n");
> +		return -ENODEV;
> +	}
> +
> +	gpio = of_get_gpio(node, 1);
> +	if (gpio_is_valid(gpio)) {
> +		dra7xx_usb->irq_gpio = gpio;
> +		irq_num = gpio_to_irq(dra7xx_usb->irq_gpio);
> +		if (irq_num < 0)
> +			dra7xx_usb->irq_gpio = 0;
> +	} else {
> +		dev_err(&pdev->dev, "failed to get irq gpio\n");
> +	}
> +
> +

Remove unnecessary blank line.

> +	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.

> +	}
> +
> +	dra7xx_usb->id_prev = gpio_get_value_cansleep(dra7xx_usb->id_gpio);
> +	if (dra7xx_usb->id_prev) {
> +		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?

> +
> +	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?

> +		else
> +			return 0;

If devm_request_threaded_irq() return success state, why did you directly call 'return'?
kthread_create operation isn't necessary?

> +	}
> +
> +	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.

Also, you set delay time as 2000 millisecond for kthread. kthread don't guarantee rapid response.
> +			schedule_timeout_interruptible
> +						(msecs_to_jiffies(2*1000));



> +	if (!dra7xx_usb->thread_task) {
> +		dev_err(dra7xx_usb->dev, "failed to create thread for %s\n"
> +					, dev_name(&pdev->dev));
> +		goto err0;

I need correct meaning name as err_thread or etc ...

> +	}
> +
> +	wake_up_process(dra7xx_usb->thread_task);
> +
> +	return 0;
> +
> +err0:

ditto.

> +	gpio_free(dra7xx_usb->id_gpio);
> +	extcon_dev_unregister(&dra7xx_usb->edev);
> +
> +	return status;
> +}
> +
> +static int dra7xx_usb_remove(struct platform_device *pdev)
> +{
> +	struct dra7xx_usb *dra7xx_usb = platform_get_drvdata(pdev);
> +
> +	if (!dra7xx_usb->irq_gpio)
> +		kthread_stop(dra7xx_usb->thread_task);
> +
> +	gpio_free(dra7xx_usb->id_gpio);
> +	extcon_dev_unregister(&dra7xx_usb->edev);
> +
> +	return 0;
> +}
> +
> +static struct of_device_id of_dra7xx_match_tbl[] = {
> +	{ .compatible = "ti,dra7xx-usb", },
> +	{ /* end */ }
> +};
> +
> +static struct platform_driver dra7xx_usb_driver = {
> +	.probe = dra7xx_usb_probe,
> +	.remove = dra7xx_usb_remove,
> +	.driver = {
> +		.name = "dra7xx-usb",
> +		.of_match_table = of_dra7xx_match_tbl,
> +		.owner = THIS_MODULE,
> +	},
> +};
> +
> +module_platform_driver(dra7xx_usb_driver);
> +
> +MODULE_ALIAS("platform:dra7xx-usb");
> +MODULE_AUTHOR("George Cherian <george.cherian@ti.com>");
> +MODULE_DESCRIPTION("DRA7x USB Connector driver");
> +MODULE_LICENSE("GPL");
> +MODULE_DEVICE_TABLE(of, of_dra7xx_match_tbl);
> 

Thanks,
Chanwoo Choi

Re: [PATCH 1/2] extcon: extcon-dra7xx: Add extcon driver for USB ID detection

[George Cherian]

Hi Chanwoo,

Thanks for your review.

On 8/20/2013 5:54 AM, Chanwoo Choi wrote:
> Hi George,
>
> On 08/16/2013 07:13 PM, George Cherian wrote:
>> Adding extcon driver for USB ID detection to dynamically
>> configure USB Host/Peripheral mode.
>>
>> Signed-off-by: George Cherian <george.cherian@ti.com>
>> ---
>>   .../devicetree/bindings/extcon/extcon-dra7xx.txt   |  19 ++
>>   drivers/extcon/Kconfig                             |   7 +
>>   drivers/extcon/Makefile                            |   1 +
>>   drivers/extcon/extcon-dra7xx.c                     | 234 +++++++++++++++++++++
>>   4 files changed, 261 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/extcon/extcon-dra7xx.txt
>>   create mode 100644 drivers/extcon/extcon-dra7xx.c
>>
>> 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.
>> +		compatible = "ti,dra7xx-usb";
>> +		gpios = <&pcf_usb 1 0>,
>> +			<&gpio6 11 2>;
>> +		interrupt-parent = <&gpio6>;
>> +		interrupts = <11>;
>> +	};
> You have to keep indentation rule.

okay
>
>> +
>> diff --git a/drivers/extcon/Kconfig b/drivers/extcon/Kconfig
>> index f1d54a3..b9cf0b2 100644
>> --- a/drivers/extcon/Kconfig
>> +++ b/drivers/extcon/Kconfig
>> @@ -64,4 +64,11 @@ config EXTCON_PALMAS
>>   	  Say Y here to enable support for USB peripheral and USB host
>>   	  detection by palmas usb.
>>   
>> +config EXTCON_DRA7XX
>> +	tristate "DRA7XX EXTCON support"
>> +	help
>> +	  Say Y here to enable support for USB peripheral and USB host
>> +	  detection by pcf8575 using DRA7XX extcon.
> You should explain detailed description about pcf8575 on patch description
> and change description of EXTCON_DRA7xx as following:
> "using DRA7XX extcon" -> "using DRA7XX device" or "using DRA7XX usb"

okay
>
>> +
>> +
> Remove unnecessary blank line.

okay
>
>>   endif # MULTISTATE_SWITCH
>> diff --git a/drivers/extcon/Makefile b/drivers/extcon/Makefile
>> index e4fa8ba..e4778f9 100644
>> --- a/drivers/extcon/Makefile
>> +++ b/drivers/extcon/Makefile
>> @@ -10,3 +10,4 @@ obj-$(CONFIG_EXTCON_MAX77693)	+= extcon-max77693.o
>>   obj-$(CONFIG_EXTCON_MAX8997)	+= extcon-max8997.o
>>   obj-$(CONFIG_EXTCON_ARIZONA)	+= extcon-arizona.o
>>   obj-$(CONFIG_EXTCON_PALMAS)	+= extcon-palmas.o
>> +obj-$(CONFIG_EXTCON_DRA7XX)	+= extcon-dra7xx.o
>> diff --git a/drivers/extcon/extcon-dra7xx.c b/drivers/extcon/extcon-dra7xx.c
>> new file mode 100644
>> index 0000000..268c25e
>> --- /dev/null
>> +++ b/drivers/extcon/extcon-dra7xx.c
>> @@ -0,0 +1,234 @@
>> +/*
>> + * DRA7XX USB ID pin detection driver
>> + *
>> + * Copyright (C) 2013 Texas Instruments Incorporated - http://www.ti.com
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + *
>> + * Author: George Cherian <george.cherian@ti.com>
>> + *
>> + * Based on extcon-palmas.c
>> + *
>> + * Author: Kishon Vijay Abraham I <kishon@ti.com>
>> + *
>> + * 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/module.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/kthread.h>
>> +#include <linux/freezer.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/extcon.h>
>> +#include <linux/err.h>
>> +#include <linux/of.h>
>> +#include <linux/gpio.h>
>> +#include <linux/of_gpio.h>
>> +#include <linux/of_platform.h>
>> +
>> +struct dra7xx_usb {
>> +	struct device *dev;
>> +
>> +	struct extcon_dev edev;
>> +	struct task_struct *thread_task;
>> +
>> +	/*GPIO pin */
> Add space between "/*" and "GPIO".

okay
>
>> +	int id_gpio;
>> +	int irq_gpio;
>> +
>> +	int id_prev;
>> +	int id_current;
>> +
>> +};
>> +
>> +static const char *dra7xx_extcon_cable[] = {
>> +	[0] = "USB",
>> +	[1] = "USB-HOST",
>> +	NULL,
>> +};
>> +
>> +static const int mutually_exclusive[] = {0x3, 0x0};
>> +
>> +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.

>
>> +		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) {
>> +		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);
>> +	}
> why did you do keep always connected state?

Same as above
>
>> +
>> +	dra7xx_usb->id_prev = dra7xx_usb->id_current;
> Add blank line.

okay
>> +	return IRQ_HANDLED;
>> +
> Remove unnecessary blank line.

okay
>> +}
>> +
>> +static int dra7xx_usb_probe(struct platform_device *pdev)
>> +{
>> +	struct device_node *node = pdev->dev.of_node;
>> +	struct dra7xx_usb *dra7xx_usb;
>> +	int status;
> 'status' local variable is used for return value.
> So, I prefer 'ret' variable instead of 'status' name.

okay
>> +	int irq_num;
>> +	int gpio;
>> +
>> +	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.
>
>> +
>> +
> Remove unnecessary blank line.

okay
>
>> +	dra7xx_usb->dev	 = &pdev->dev;
>> +
>> +	platform_set_drvdata(pdev, dra7xx_usb);
>> +
>> +	dra7xx_usb->edev.name = dev_name(&pdev->dev);
> If edev name is equal with device name, this line is unnecessary.
> Because extcon_dev_register() use dev_name(&pdev->dev) as edev name
> in extcon-class.c

okay
>> +	dra7xx_usb->edev.supported_cable = dra7xx_extcon_cable;
>> +	dra7xx_usb->edev.mutually_exclusive = mutually_exclusive;
>> +
>> +	gpio = of_get_gpio(node, 0);
>> +	if (gpio_is_valid(gpio)) {
>> +		dra7xx_usb->id_gpio = gpio;
>> +		status = gpio_request(dra7xx_usb->id_gpio, "id_gpio");
>> +		if (status)
>> +			return status;
> You have to add error message with dev_err().

okay
>> +	} else {
>> +		dev_err(&pdev->dev, "failed to get id gpio\n");
>> +		return -ENODEV;
>> +	}
>> +
>> +	gpio = of_get_gpio(node, 1);
>> +	if (gpio_is_valid(gpio)) {
>> +		dra7xx_usb->irq_gpio = gpio;
>> +		irq_num = gpio_to_irq(dra7xx_usb->irq_gpio);
>> +		if (irq_num < 0)
>> +			dra7xx_usb->irq_gpio = 0;
>> +	} else {
>> +		dev_err(&pdev->dev, "failed to get irq gpio\n");
>> +	}
>> +
>> +
> Remove unnecessary blank line.

okay
>> +	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) {
>> +		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.


>
> Also, you set delay time as 2000 millisecond for kthread. kthread don't guarantee rapid response.
>> +			schedule_timeout_interruptible
>> +						(msecs_to_jiffies(2*1000));
>
>
>> +	if (!dra7xx_usb->thread_task) {
>> +		dev_err(dra7xx_usb->dev, "failed to create thread for %s\n"
>> +					, dev_name(&pdev->dev));
>> +		goto err0;
> I need correct meaning name as err_thread or etc ...

okay
>
>> +	}
>> +
>> +	wake_up_process(dra7xx_usb->thread_task);
>> +
>> +	return 0;
>> +
>> +err0:
> ditto.

okay
>
>> +	gpio_free(dra7xx_usb->id_gpio);
>> +	extcon_dev_unregister(&dra7xx_usb->edev);
>> +
>> +	return status;
>> +}
>> +
>> +static int dra7xx_usb_remove(struct platform_device *pdev)
>> +{
>> +	struct dra7xx_usb *dra7xx_usb = platform_get_drvdata(pdev);
>> +
>> +	if (!dra7xx_usb->irq_gpio)
>> +		kthread_stop(dra7xx_usb->thread_task);
>> +
>> +	gpio_free(dra7xx_usb->id_gpio);
>> +	extcon_dev_unregister(&dra7xx_usb->edev);
>> +
>> +	return 0;
>> +}
>> +
>> +static struct of_device_id of_dra7xx_match_tbl[] = {
>> +	{ .compatible = "ti,dra7xx-usb", },
>> +	{ /* end */ }
>> +};
>> +
>> +static struct platform_driver dra7xx_usb_driver = {
>> +	.probe = dra7xx_usb_probe,
>> +	.remove = dra7xx_usb_remove,
>> +	.driver = {
>> +		.name = "dra7xx-usb",
>> +		.of_match_table = of_dra7xx_match_tbl,
>> +		.owner = THIS_MODULE,
>> +	},
>> +};
>> +
>> +module_platform_driver(dra7xx_usb_driver);
>> +
>> +MODULE_ALIAS("platform:dra7xx-usb");
>> +MODULE_AUTHOR("George Cherian <george.cherian@ti.com>");
>> +MODULE_DESCRIPTION("DRA7x USB Connector driver");
>> +MODULE_LICENSE("GPL");
>> +MODULE_DEVICE_TABLE(of, of_dra7xx_match_tbl);
>>
> Thanks,
> Chanwoo Choi


-- 
-George

Re: [PATCH 1/2] extcon: extcon-dra7xx: Add extcon driver for USB ID detection

[Chanwoo Choi]

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.


>>> +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.

> 
>>
>>> +        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.


>>> +    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.

>>> +        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.
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

Re: [PATCH 1/2] extcon: extcon-dra7xx: Add extcon driver for USB ID detection

[George Cherian]

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

Re: [PATCH 1/2] extcon: extcon-dra7xx: Add extcon driver for USB ID detection

[Chanwoo Choi]

>>>>> +
>>>>> +    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 .

Right, my mistake. I prefer interrupt method.

Thanks,
Chanwoo CHoi