Re: [PATCH v3 1/3] extcon: extcon-gpio-usbvid: Generic USB VBUS/ID detection via GPIO
On 8/30/2013 12:44 PM, Chanwoo Choi wrote:
Hi George,
In addition, I add answer about that device driver control gpio pin directly.
On 08/30/2013 03:15 PM, George Cherian wrote:
Hi Chanwoo,
On 8/30/2013 5:41 AM, Chanwoo Choi wrote:
Hi George,
On 08/29/2013 10:45 PM, George Cherian wrote:
Hi Chanwoo,
On 8/29/2013 5:42 PM, Chanwoo Choi wrote:
[big snip ]
I tested various development board based on Samsung Exynos series SoC.
Although some gpio of Exynos series SoC set high state(non zero, 1) as default
value,
this gpio state could mean off state, disconnected or un-powered state
according to gpio.
Of course, above explanation about specific gpio don't include all gpios.
This is meaning that there is possibility.
okay then how about adding a flag for inverted logic too. something like this
if(of_property_read_bool(np,"inverted_gpio))
gpio_usbvid->gpio_inv = 1;
And always check on this before deciding?
Is this fine ?
OK.
But, as Stephen commented on other mail, you should use proper DT helper
function.
okay
Second,
gpio_usbvid_set_initial_state() dertermine both "USB-HOST" and "USB" cable
state at same time
in 'case ID_DETCT' according to 'gpio_usbvid->id_gpio'. But, I think that other
extcon devices
would not control both "USB-HOST" and "USB" cable state at same time.
Other extcon devices would support either "USB-HOST" or "USB" cable.
The driver has 2 configurations.
1) supports implementations with both VBUS and ID pin are routed via gpio's for
swicthing roles(compatible gpio-usb-vid).
As you explained about case 1, it is only used on dra7x SoC.
No gpio-usb-id is used in dra7xx. dra7xx has got only ID pin routed via gpio.
Other SoC could not wish to control both "USB-HOST" and "USB" cable at same
time.
I could'nt actually parse this. You meant since the id_irq_handler handles both
USB and USB-HOST cable
its not proper?
It's not proper in general case. The generic driver must guarantee all of
extcon device using gpio.
As far as I know, the generic driver cannot direclty control gpio pin and get
value from gpio pin.
Almost device driver including in kernel/drivers control gpio pin on specific
device driver
instead of generic driver.
But without reading the gpio value how can we set the cable states?
hmm. I mentioned above my answer as following:
>> As far as I know, the generic driver cannot direclty control gpio
pin and get value from gpio pin.
>> Almost device driver including in kernel/drivers control gpio pin on
specific device driver
Because your extcon driver directly control gpio pin so I think your extcon
driver isn't generic.
for this driver the assumption is the dedicated gpio
Obviously when I am writing a generic driver for USB VBUS-ID detetction
which is via gpio then i assume I have a dedicated gpio.
what is wrong in that assumption? How else can you detect ID pin change
and VBUS change via gpio if not you have them dedicated?
is always DIR_IN and in the driver we just read the value.
What is DIR_IN?
Direction IN gpio.
I need your answer about above my opinion for other SoC.
So how about this, I have removed the dra7x specific stuffs (gpio-usb-id)
static void gpio_usbvid_set_initial_state(struct gpio_usbvid *gpio_usbvid)
{
int id_current, vbus_current;
id_current = gpio_get_value_cansleep(gpio_usbvid->id_gpio);
if (!!id_current == ID_FLOAT)
extcon_set_cable_state(_usbvid->edev, "USB-HOST", false);
else
extcon_set_cable_state(_usbvid->edev, "USB-HOST", true);
vbus_current = gpio_get_value_cansleep(gpio_usbvid->vbus_gpio);
if (!!vbus_current == VBUS_ON)
extcon_set_cable_state(_usbvid->edev, "USB", true);
else
extcon_set_cable_state(_usbvid->edev, "USB", false);
}
and the irq handlers like this
static irqreturn_t id_irq_handler(int irq, void *data)
{
struct gpio_usbvid *gpio_usbvid = (struct gpio_usbvid *)data;
int id_current;
id_current = gpio_get_value_cansleep(gpio_usbvid->id_gpio);
if (id_current == ID_GND)
extcon_set_cable_state(_usbvid->edev, "USB-HOST", true);
else
extcon_set_cable_state(_usbvid->edev, "USB-HOST", false);
return IRQ_HANDLED;
}
static irqreturn_t vbus_irq_handler(int irq, void *data)
{
struct gpio_usbvid *gpio_usbvid = (struct gpio_usbvid *)data;
int vbus_current;
vbus_current = gpio_get_value_cansleep(gpio_usbvid->vbus_gpio);
if (vbus_current == VBUS_OFF)
extcon_set_cable_state(_usbvid->edev, "USB", false);
else
extcon_set_cable_state(_usbvid->edev, "USB", true);
return IRQ_HANDLED;
}
I know your intention dividing interrupt handler for each cable.
But I don't think this driver must guarantee all of extcon device using gpio.
For example,
below three SoC wish to detect USB/USB-HOST cable with each different methods.
Re: [PATCH v3 1/3] extcon: extcon-gpio-usbvid: Generic USB VBUS/ID detection via GPIO
On 8/30/2013 12:23 PM, Chanwoo Choi wrote:
Hi George,
On 08/30/2013 03:15 PM, George Cherian wrote:
Hi Chanwoo,
On 8/30/2013 5:41 AM, Chanwoo Choi wrote:
Hi George,
On 08/29/2013 10:45 PM, George Cherian wrote:
Hi Chanwoo,
On 8/29/2013 5:42 PM, Chanwoo Choi wrote:
[big snip ]
I tested various development board based on Samsung Exynos series SoC.
Although some gpio of Exynos series SoC set high state(non zero, 1) as default
value,
this gpio state could mean off state, disconnected or un-powered state
according to gpio.
Of course, above explanation about specific gpio don't include all gpios.
This is meaning that there is possibility.
okay then how about adding a flag for inverted logic too. something like this
if(of_property_read_bool(np,"inverted_gpio))
gpio_usbvid->gpio_inv = 1;
And always check on this before deciding?
Is this fine ?
OK.
But, as Stephen commented on other mail, you should use proper DT helper
function.
okay
Second,
gpio_usbvid_set_initial_state() dertermine both "USB-HOST" and "USB" cable
state at same time
in 'case ID_DETCT' according to 'gpio_usbvid->id_gpio'. But, I think that other
extcon devices
would not control both "USB-HOST" and "USB" cable state at same time.
Other extcon devices would support either "USB-HOST" or "USB" cable.
The driver has 2 configurations.
1) supports implementations with both VBUS and ID pin are routed via gpio's for
swicthing roles(compatible gpio-usb-vid).
As you explained about case 1, it is only used on dra7x SoC.
No gpio-usb-id is used in dra7xx. dra7xx has got only ID pin routed via gpio.
Other SoC could not wish to control both "USB-HOST" and "USB" cable at same
time.
I could'nt actually parse this. You meant since the id_irq_handler handles both
USB and USB-HOST cable
its not proper?
It's not proper in general case. The generic driver must guarantee all of
extcon device using gpio.
As far as I know, the generic driver cannot direclty control gpio pin and get
value from gpio pin.
Almost device driver including in kernel/drivers control gpio pin on specific
device driver
instead of generic driver.
But without reading the gpio value how can we set the cable states? for this
driver the assumption is the dedicated gpio
is always DIR_IN and in the driver we just read the value.
I need your answer about above my opinion for other SoC.
So how about this, I have removed the dra7x specific stuffs (gpio-usb-id)
static void gpio_usbvid_set_initial_state(struct gpio_usbvid *gpio_usbvid)
{
int id_current, vbus_current;
id_current = gpio_get_value_cansleep(gpio_usbvid->id_gpio);
if (!!id_current == ID_FLOAT)
extcon_set_cable_state(_usbvid->edev, "USB-HOST", false);
else
extcon_set_cable_state(_usbvid->edev, "USB-HOST", true);
vbus_current = gpio_get_value_cansleep(gpio_usbvid->vbus_gpio);
if (!!vbus_current == VBUS_ON)
extcon_set_cable_state(_usbvid->edev, "USB", true);
else
extcon_set_cable_state(_usbvid->edev, "USB", false);
}
and the irq handlers like this
static irqreturn_t id_irq_handler(int irq, void *data)
{
struct gpio_usbvid *gpio_usbvid = (struct gpio_usbvid *)data;
int id_current;
id_current = gpio_get_value_cansleep(gpio_usbvid->id_gpio);
if (id_current == ID_GND)
extcon_set_cable_state(_usbvid->edev, "USB-HOST", true);
else
extcon_set_cable_state(_usbvid->edev, "USB-HOST", false);
return IRQ_HANDLED;
}
static irqreturn_t vbus_irq_handler(int irq, void *data)
{
struct gpio_usbvid *gpio_usbvid = (struct gpio_usbvid *)data;
int vbus_current;
vbus_current = gpio_get_value_cansleep(gpio_usbvid->vbus_gpio);
if (vbus_current == VBUS_OFF)
extcon_set_cable_state(_usbvid->edev, "USB", false);
else
extcon_set_cable_state(_usbvid->edev, "USB", true);
return IRQ_HANDLED;
}
I know your intention dividing interrupt handler for each cable.
But I don't think this driver must guarantee all of extcon device using gpio.
For example,
below three SoC wish to detect USB/USB-HOST cable with each different methods.
"SoC A" wish to detect USB/USB-HOST cable through only one gpio pin.
You mean to say that both USB ID pin and VBUS are connected to same gpio?
If so that is a really bad h/w design and it will not fly.
Or, you meant that only USB ID pin is connected to single gpio and it controls
the state of the USB/USB-HOST cables?
If so thats what exactly the v3 driver did with compatible gpio-usb-id.
My original question intention,
I'd like you to answer that this driver can support all case of "SoC A"/"SoC B"/"SoC
C" without othe implementation?
Definitely supports SoC A and SoC C. this is neither a generic extcon
driver nor a generic gpio driver.
This is a generic driver for USB VBUS-ID detection connected via gpios.
so doesnot
Re: [PATCH v3 1/3] extcon: extcon-gpio-usbvid: Generic USB VBUS/ID detection via GPIO
On 8/30/2013 12:23 PM, Chanwoo Choi wrote:
Hi George,
On 08/30/2013 03:15 PM, George Cherian wrote:
Hi Chanwoo,
On 8/30/2013 5:41 AM, Chanwoo Choi wrote:
Hi George,
On 08/29/2013 10:45 PM, George Cherian wrote:
Hi Chanwoo,
On 8/29/2013 5:42 PM, Chanwoo Choi wrote:
[big snip ]
I tested various development board based on Samsung Exynos series SoC.
Although some gpio of Exynos series SoC set high state(non zero, 1) as default
value,
this gpio state could mean off state, disconnected or un-powered state
according to gpio.
Of course, above explanation about specific gpio don't include all gpios.
This is meaning that there is possibility.
okay then how about adding a flag for inverted logic too. something like this
if(of_property_read_bool(np,inverted_gpio))
gpio_usbvid-gpio_inv = 1;
And always check on this before deciding?
Is this fine ?
OK.
But, as Stephen commented on other mail, you should use proper DT helper
function.
okay
Second,
gpio_usbvid_set_initial_state() dertermine both USB-HOST and USB cable
state at same time
in 'case ID_DETCT' according to 'gpio_usbvid-id_gpio'. But, I think that other
extcon devices
would not control both USB-HOST and USB cable state at same time.
Other extcon devices would support either USB-HOST or USB cable.
The driver has 2 configurations.
1) supports implementations with both VBUS and ID pin are routed via gpio's for
swicthing roles(compatible gpio-usb-vid).
As you explained about case 1, it is only used on dra7x SoC.
No gpio-usb-id is used in dra7xx. dra7xx has got only ID pin routed via gpio.
Other SoC could not wish to control both USB-HOST and USB cable at same
time.
I could'nt actually parse this. You meant since the id_irq_handler handles both
USB and USB-HOST cable
its not proper?
It's not proper in general case. The generic driver must guarantee all of
extcon device using gpio.
As far as I know, the generic driver cannot direclty control gpio pin and get
value from gpio pin.
Almost device driver including in kernel/drivers control gpio pin on specific
device driver
instead of generic driver.
But without reading the gpio value how can we set the cable states? for this
driver the assumption is the dedicated gpio
is always DIR_IN and in the driver we just read the value.
I need your answer about above my opinion for other SoC.
So how about this, I have removed the dra7x specific stuffs (gpio-usb-id)
static void gpio_usbvid_set_initial_state(struct gpio_usbvid *gpio_usbvid)
{
int id_current, vbus_current;
id_current = gpio_get_value_cansleep(gpio_usbvid-id_gpio);
if (!!id_current == ID_FLOAT)
extcon_set_cable_state(gpio_usbvid-edev, USB-HOST, false);
else
extcon_set_cable_state(gpio_usbvid-edev, USB-HOST, true);
vbus_current = gpio_get_value_cansleep(gpio_usbvid-vbus_gpio);
if (!!vbus_current == VBUS_ON)
extcon_set_cable_state(gpio_usbvid-edev, USB, true);
else
extcon_set_cable_state(gpio_usbvid-edev, USB, false);
}
and the irq handlers like this
static irqreturn_t id_irq_handler(int irq, void *data)
{
struct gpio_usbvid *gpio_usbvid = (struct gpio_usbvid *)data;
int id_current;
id_current = gpio_get_value_cansleep(gpio_usbvid-id_gpio);
if (id_current == ID_GND)
extcon_set_cable_state(gpio_usbvid-edev, USB-HOST, true);
else
extcon_set_cable_state(gpio_usbvid-edev, USB-HOST, false);
return IRQ_HANDLED;
}
static irqreturn_t vbus_irq_handler(int irq, void *data)
{
struct gpio_usbvid *gpio_usbvid = (struct gpio_usbvid *)data;
int vbus_current;
vbus_current = gpio_get_value_cansleep(gpio_usbvid-vbus_gpio);
if (vbus_current == VBUS_OFF)
extcon_set_cable_state(gpio_usbvid-edev, USB, false);
else
extcon_set_cable_state(gpio_usbvid-edev, USB, true);
return IRQ_HANDLED;
}
I know your intention dividing interrupt handler for each cable.
But I don't think this driver must guarantee all of extcon device using gpio.
For example,
below three SoC wish to detect USB/USB-HOST cable with each different methods.
SoC A wish to detect USB/USB-HOST cable through only one gpio pin.
You mean to say that both USB ID pin and VBUS are connected to same gpio?
If so that is a really bad h/w design and it will not fly.
Or, you meant that only USB ID pin is connected to single gpio and it controls
the state of the USB/USB-HOST cables?
If so thats what exactly the v3 driver did with compatible gpio-usb-id.
My original question intention,
I'd like you to answer that this driver can support all case of SoC A/SoC B/SoC
C without othe implementation?
Definitely supports SoC A and SoC C. this is neither a generic extcon
driver nor a generic gpio driver.
This is a generic driver for USB VBUS-ID detection connected via gpios.
so doesnot address ADC (SOC B)
SoC B
Re: [PATCH v3 1/3] extcon: extcon-gpio-usbvid: Generic USB VBUS/ID detection via GPIO
On 8/30/2013 12:44 PM, Chanwoo Choi wrote:
Hi George,
In addition, I add answer about that device driver control gpio pin directly.
On 08/30/2013 03:15 PM, George Cherian wrote:
Hi Chanwoo,
On 8/30/2013 5:41 AM, Chanwoo Choi wrote:
Hi George,
On 08/29/2013 10:45 PM, George Cherian wrote:
Hi Chanwoo,
On 8/29/2013 5:42 PM, Chanwoo Choi wrote:
[big snip ]
I tested various development board based on Samsung Exynos series SoC.
Although some gpio of Exynos series SoC set high state(non zero, 1) as default
value,
this gpio state could mean off state, disconnected or un-powered state
according to gpio.
Of course, above explanation about specific gpio don't include all gpios.
This is meaning that there is possibility.
okay then how about adding a flag for inverted logic too. something like this
if(of_property_read_bool(np,inverted_gpio))
gpio_usbvid-gpio_inv = 1;
And always check on this before deciding?
Is this fine ?
OK.
But, as Stephen commented on other mail, you should use proper DT helper
function.
okay
Second,
gpio_usbvid_set_initial_state() dertermine both USB-HOST and USB cable
state at same time
in 'case ID_DETCT' according to 'gpio_usbvid-id_gpio'. But, I think that other
extcon devices
would not control both USB-HOST and USB cable state at same time.
Other extcon devices would support either USB-HOST or USB cable.
The driver has 2 configurations.
1) supports implementations with both VBUS and ID pin are routed via gpio's for
swicthing roles(compatible gpio-usb-vid).
As you explained about case 1, it is only used on dra7x SoC.
No gpio-usb-id is used in dra7xx. dra7xx has got only ID pin routed via gpio.
Other SoC could not wish to control both USB-HOST and USB cable at same
time.
I could'nt actually parse this. You meant since the id_irq_handler handles both
USB and USB-HOST cable
its not proper?
It's not proper in general case. The generic driver must guarantee all of
extcon device using gpio.
As far as I know, the generic driver cannot direclty control gpio pin and get
value from gpio pin.
Almost device driver including in kernel/drivers control gpio pin on specific
device driver
instead of generic driver.
But without reading the gpio value how can we set the cable states?
hmm. I mentioned above my answer as following:
As far as I know, the generic driver cannot direclty control gpio
pin and get value from gpio pin.
Almost device driver including in kernel/drivers control gpio pin on
specific device driver
Because your extcon driver directly control gpio pin so I think your extcon
driver isn't generic.
for this driver the assumption is the dedicated gpio
Obviously when I am writing a generic driver for USB VBUS-ID detetction
which is via gpio then i assume I have a dedicated gpio.
what is wrong in that assumption? How else can you detect ID pin change
and VBUS change via gpio if not you have them dedicated?
is always DIR_IN and in the driver we just read the value.
What is DIR_IN?
Direction IN gpio.
I need your answer about above my opinion for other SoC.
So how about this, I have removed the dra7x specific stuffs (gpio-usb-id)
static void gpio_usbvid_set_initial_state(struct gpio_usbvid *gpio_usbvid)
{
int id_current, vbus_current;
id_current = gpio_get_value_cansleep(gpio_usbvid-id_gpio);
if (!!id_current == ID_FLOAT)
extcon_set_cable_state(gpio_usbvid-edev, USB-HOST, false);
else
extcon_set_cable_state(gpio_usbvid-edev, USB-HOST, true);
vbus_current = gpio_get_value_cansleep(gpio_usbvid-vbus_gpio);
if (!!vbus_current == VBUS_ON)
extcon_set_cable_state(gpio_usbvid-edev, USB, true);
else
extcon_set_cable_state(gpio_usbvid-edev, USB, false);
}
and the irq handlers like this
static irqreturn_t id_irq_handler(int irq, void *data)
{
struct gpio_usbvid *gpio_usbvid = (struct gpio_usbvid *)data;
int id_current;
id_current = gpio_get_value_cansleep(gpio_usbvid-id_gpio);
if (id_current == ID_GND)
extcon_set_cable_state(gpio_usbvid-edev, USB-HOST, true);
else
extcon_set_cable_state(gpio_usbvid-edev, USB-HOST, false);
return IRQ_HANDLED;
}
static irqreturn_t vbus_irq_handler(int irq, void *data)
{
struct gpio_usbvid *gpio_usbvid = (struct gpio_usbvid *)data;
int vbus_current;
vbus_current = gpio_get_value_cansleep(gpio_usbvid-vbus_gpio);
if (vbus_current == VBUS_OFF)
extcon_set_cable_state(gpio_usbvid-edev, USB, false);
else
extcon_set_cable_state(gpio_usbvid-edev, USB, true);
return IRQ_HANDLED;
}
I know your intention dividing interrupt handler for each cable.
But I don't think this driver must guarantee all of extcon device using gpio.
For example,
below three SoC wish to detect USB/USB-HOST cable with each different methods.
SoC A wish to detect
Re: [PATCH v3 1/3] extcon: extcon-gpio-usbvid: Generic USB VBUS/ID detection via GPIO
Hi George,
In addition, I add answer about that device driver control gpio pin directly.
On 08/30/2013 03:15 PM, George Cherian wrote:
> Hi Chanwoo,
>
> On 8/30/2013 5:41 AM, Chanwoo Choi wrote:
>> Hi George,
>>
>> On 08/29/2013 10:45 PM, George Cherian wrote:
>>> Hi Chanwoo,
>>>
>>>
>>> On 8/29/2013 5:42 PM, Chanwoo Choi wrote:
>>> [big snip ]
>> I tested various development board based on Samsung Exynos series SoC.
>> Although some gpio of Exynos series SoC set high state(non zero, 1) as
>> default value,
>> this gpio state could mean off state, disconnected or un-powered state
>> according to gpio.
>> Of course, above explanation about specific gpio don't include all gpios.
>> This is meaning that there is possibility.
> okay then how about adding a flag for inverted logic too. something like
> this
>
> if(of_property_read_bool(np,"inverted_gpio))
> gpio_usbvid->gpio_inv = 1;
> And always check on this before deciding?
>>> Is this fine ?
>> OK.
>> But, as Stephen commented on other mail, you should use proper DT helper
>> function.
> okay
Second,
gpio_usbvid_set_initial_state() dertermine both "USB-HOST" and "USB"
cable state at same time
in 'case ID_DETCT' according to 'gpio_usbvid->id_gpio'. But, I think
that other extcon devices
would not control both "USB-HOST" and "USB" cable state at same time.
Other extcon devices would support either "USB-HOST" or "USB" cable.
>>> The driver has 2 configurations.
>>> 1) supports implementations with both VBUS and ID pin are routed via
>>> gpio's for swicthing roles(compatible gpio-usb-vid).
>> As you explained about case 1, it is only used on dra7x SoC.
> No gpio-usb-id is used in dra7xx. dra7xx has got only ID pin routed via
> gpio.
>> Other SoC could not wish to control both "USB-HOST" and "USB" cable at
>> same time.
>>> I could'nt actually parse this. You meant since the id_irq_handler handles
>>> both USB and USB-HOST cable
>>> its not proper?
>> It's not proper in general case. The generic driver must guarantee all of
>> extcon device using gpio.
>> As far as I know, the generic driver cannot direclty control gpio pin and
>> get value from gpio pin.
>> Almost device driver including in kernel/drivers control gpio pin on
>> specific device driver
>> instead of generic driver.
> But without reading the gpio value how can we set the cable states?
hmm. I mentioned above my answer as following:
>> As far as I know, the generic driver cannot direclty control gpio
pin and get value from gpio pin.
>> Almost device driver including in kernel/drivers control gpio pin on
specific device driver
Because your extcon driver directly control gpio pin so I think your extcon
driver isn't generic.
for this driver the assumption is the dedicated gpio
> is always DIR_IN and in the driver we just read the value.
What is DIR_IN?
I need your answer about above my opinion for other SoC.
>>> So how about this, I have removed the dra7x specific stuffs (gpio-usb-id)
>>>
>>> static void gpio_usbvid_set_initial_state(struct gpio_usbvid *gpio_usbvid)
>>> {
>>> int id_current, vbus_current;
>>>
>>> id_current = gpio_get_value_cansleep(gpio_usbvid->id_gpio);
>>> if (!!id_current == ID_FLOAT)
>>> extcon_set_cable_state(_usbvid->edev, "USB-HOST", false);
>>> else
>>> extcon_set_cable_state(_usbvid->edev, "USB-HOST", true);
>>>
>>> vbus_current = gpio_get_value_cansleep(gpio_usbvid->vbus_gpio);
>>> if (!!vbus_current == VBUS_ON)
>>> extcon_set_cable_state(_usbvid->edev, "USB", true);
>>> else
>>> extcon_set_cable_state(_usbvid->edev, "USB", false);
>>> }
>>>
>>> and the irq handlers like this
>>>
>>> static irqreturn_t id_irq_handler(int irq, void *data)
>>> {
>>> struct gpio_usbvid *gpio_usbvid = (struct gpio_usbvid *)data;
>>> int id_current;
>>>
>>> id_current = gpio_get_value_cansleep(gpio_usbvid->id_gpio);
>>> if (id_current == ID_GND)
>>> extcon_set_cable_state(_usbvid->edev, "USB-HOST",
>>> true);
>>> else
>>> extcon_set_cable_state(_usbvid->edev, "USB-HOST",
>>> false);
>>> return IRQ_HANDLED;
>>> }
>>>
>>> static irqreturn_t vbus_irq_handler(int irq, void *data)
>>> {
>>> struct gpio_usbvid *gpio_usbvid = (struct gpio_usbvid *)data;
>>> int vbus_current;
>>>
>>> vbus_current = gpio_get_value_cansleep(gpio_usbvid->vbus_gpio);
>>> if (vbus_current == VBUS_OFF)
>>> extcon_set_cable_state(_usbvid->edev, "USB", false);
>>> else
>>> extcon_set_cable_state(_usbvid->edev, "USB", true);
>>>
>>> return IRQ_HANDLED;
>>> }
>> I know your intention dividing interrupt handler for each cable.
>> But I don't think this driver must guarantee all
Re: [PATCH v3 1/3] extcon: extcon-gpio-usbvid: Generic USB VBUS/ID detection via GPIO
Hi George,
On 08/30/2013 03:15 PM, George Cherian wrote:
> Hi Chanwoo,
>
> On 8/30/2013 5:41 AM, Chanwoo Choi wrote:
>> Hi George,
>>
>> On 08/29/2013 10:45 PM, George Cherian wrote:
>>> Hi Chanwoo,
>>>
>>>
>>> On 8/29/2013 5:42 PM, Chanwoo Choi wrote:
>>> [big snip ]
>> I tested various development board based on Samsung Exynos series SoC.
>> Although some gpio of Exynos series SoC set high state(non zero, 1) as
>> default value,
>> this gpio state could mean off state, disconnected or un-powered state
>> according to gpio.
>> Of course, above explanation about specific gpio don't include all gpios.
>> This is meaning that there is possibility.
> okay then how about adding a flag for inverted logic too. something like
> this
>
> if(of_property_read_bool(np,"inverted_gpio))
> gpio_usbvid->gpio_inv = 1;
> And always check on this before deciding?
>>> Is this fine ?
>> OK.
>> But, as Stephen commented on other mail, you should use proper DT helper
>> function.
> okay
Second,
gpio_usbvid_set_initial_state() dertermine both "USB-HOST" and "USB"
cable state at same time
in 'case ID_DETCT' according to 'gpio_usbvid->id_gpio'. But, I think
that other extcon devices
would not control both "USB-HOST" and "USB" cable state at same time.
Other extcon devices would support either "USB-HOST" or "USB" cable.
>>> The driver has 2 configurations.
>>> 1) supports implementations with both VBUS and ID pin are routed via
>>> gpio's for swicthing roles(compatible gpio-usb-vid).
>> As you explained about case 1, it is only used on dra7x SoC.
> No gpio-usb-id is used in dra7xx. dra7xx has got only ID pin routed via
> gpio.
>> Other SoC could not wish to control both "USB-HOST" and "USB" cable at
>> same time.
>>> I could'nt actually parse this. You meant since the id_irq_handler handles
>>> both USB and USB-HOST cable
>>> its not proper?
>> It's not proper in general case. The generic driver must guarantee all of
>> extcon device using gpio.
>> As far as I know, the generic driver cannot direclty control gpio pin and
>> get value from gpio pin.
>> Almost device driver including in kernel/drivers control gpio pin on
>> specific device driver
>> instead of generic driver.
> But without reading the gpio value how can we set the cable states? for this
> driver the assumption is the dedicated gpio
> is always DIR_IN and in the driver we just read the value.
I need your answer about above my opinion for other SoC.
>>> So how about this, I have removed the dra7x specific stuffs (gpio-usb-id)
>>>
>>> static void gpio_usbvid_set_initial_state(struct gpio_usbvid *gpio_usbvid)
>>> {
>>> int id_current, vbus_current;
>>>
>>> id_current = gpio_get_value_cansleep(gpio_usbvid->id_gpio);
>>> if (!!id_current == ID_FLOAT)
>>> extcon_set_cable_state(_usbvid->edev, "USB-HOST", false);
>>> else
>>> extcon_set_cable_state(_usbvid->edev, "USB-HOST", true);
>>>
>>> vbus_current = gpio_get_value_cansleep(gpio_usbvid->vbus_gpio);
>>> if (!!vbus_current == VBUS_ON)
>>> extcon_set_cable_state(_usbvid->edev, "USB", true);
>>> else
>>> extcon_set_cable_state(_usbvid->edev, "USB", false);
>>> }
>>>
>>> and the irq handlers like this
>>>
>>> static irqreturn_t id_irq_handler(int irq, void *data)
>>> {
>>> struct gpio_usbvid *gpio_usbvid = (struct gpio_usbvid *)data;
>>> int id_current;
>>>
>>> id_current = gpio_get_value_cansleep(gpio_usbvid->id_gpio);
>>> if (id_current == ID_GND)
>>> extcon_set_cable_state(_usbvid->edev, "USB-HOST",
>>> true);
>>> else
>>> extcon_set_cable_state(_usbvid->edev, "USB-HOST",
>>> false);
>>> return IRQ_HANDLED;
>>> }
>>>
>>> static irqreturn_t vbus_irq_handler(int irq, void *data)
>>> {
>>> struct gpio_usbvid *gpio_usbvid = (struct gpio_usbvid *)data;
>>> int vbus_current;
>>>
>>> vbus_current = gpio_get_value_cansleep(gpio_usbvid->vbus_gpio);
>>> if (vbus_current == VBUS_OFF)
>>> extcon_set_cable_state(_usbvid->edev, "USB", false);
>>> else
>>> extcon_set_cable_state(_usbvid->edev, "USB", true);
>>>
>>> return IRQ_HANDLED;
>>> }
>> I know your intention dividing interrupt handler for each cable.
>> But I don't think this driver must guarantee all of extcon device using gpio.
>>
>> For example,
>> below three SoC wish to detect USB/USB-HOST cable with each different
>> methods.
>>
>> "SoC A" wish to detect USB/USB-HOST cable through only one gpio pin.
>
> You mean to say that both USB ID pin and VBUS are connected to same gpio?
> If so that is a really bad h/w design and it will not fly.
> Or, you meant that only USB ID pin is connected to single gpio and it
> controls the state of the
Re: [PATCH v3 1/3] extcon: extcon-gpio-usbvid: Generic USB VBUS/ID detection via GPIO
Hi Chanwoo,
On 8/30/2013 5:41 AM, Chanwoo Choi wrote:
Hi George,
On 08/29/2013 10:45 PM, George Cherian wrote:
Hi Chanwoo,
On 8/29/2013 5:42 PM, Chanwoo Choi wrote:
[big snip ]
I tested various development board based on Samsung Exynos series SoC.
Although some gpio of Exynos series SoC set high state(non zero, 1) as default
value,
this gpio state could mean off state, disconnected or un-powered state
according to gpio.
Of course, above explanation about specific gpio don't include all gpios.
This is meaning that there is possibility.
okay then how about adding a flag for inverted logic too. something like this
if(of_property_read_bool(np,"inverted_gpio))
gpio_usbvid->gpio_inv = 1;
And always check on this before deciding?
Is this fine ?
OK.
But, as Stephen commented on other mail, you should use proper DT helper
function.
okay
Second,
gpio_usbvid_set_initial_state() dertermine both "USB-HOST" and "USB" cable
state at same time
in 'case ID_DETCT' according to 'gpio_usbvid->id_gpio'. But, I think that other
extcon devices
would not control both "USB-HOST" and "USB" cable state at same time.
Other extcon devices would support either "USB-HOST" or "USB" cable.
The driver has 2 configurations.
1) supports implementations with both VBUS and ID pin are routed via gpio's for
swicthing roles(compatible gpio-usb-vid).
As you explained about case 1, it is only used on dra7x SoC.
No gpio-usb-id is used in dra7xx. dra7xx has got only ID pin routed via gpio.
Other SoC could not wish to control both "USB-HOST" and "USB" cable at same
time.
I could'nt actually parse this. You meant since the id_irq_handler handles both
USB and USB-HOST cable
its not proper?
It's not proper in general case. The generic driver must guarantee all of
extcon device using gpio.
As far as I know, the generic driver cannot direclty control gpio pin and get
value from gpio pin.
Almost device driver including in kernel/drivers control gpio pin on specific
device driver
instead of generic driver.
But without reading the gpio value how can we set the cable states? for
this driver the assumption is the dedicated gpio
is always DIR_IN and in the driver we just read the value.
I need your answer about above my opinion for other SoC.
So how about this, I have removed the dra7x specific stuffs (gpio-usb-id)
static void gpio_usbvid_set_initial_state(struct gpio_usbvid *gpio_usbvid)
{
int id_current, vbus_current;
id_current = gpio_get_value_cansleep(gpio_usbvid->id_gpio);
if (!!id_current == ID_FLOAT)
extcon_set_cable_state(_usbvid->edev, "USB-HOST", false);
else
extcon_set_cable_state(_usbvid->edev, "USB-HOST", true);
vbus_current = gpio_get_value_cansleep(gpio_usbvid->vbus_gpio);
if (!!vbus_current == VBUS_ON)
extcon_set_cable_state(_usbvid->edev, "USB", true);
else
extcon_set_cable_state(_usbvid->edev, "USB", false);
}
and the irq handlers like this
static irqreturn_t id_irq_handler(int irq, void *data)
{
struct gpio_usbvid *gpio_usbvid = (struct gpio_usbvid *)data;
int id_current;
id_current = gpio_get_value_cansleep(gpio_usbvid->id_gpio);
if (id_current == ID_GND)
extcon_set_cable_state(_usbvid->edev, "USB-HOST", true);
else
extcon_set_cable_state(_usbvid->edev, "USB-HOST", false);
return IRQ_HANDLED;
}
static irqreturn_t vbus_irq_handler(int irq, void *data)
{
struct gpio_usbvid *gpio_usbvid = (struct gpio_usbvid *)data;
int vbus_current;
vbus_current = gpio_get_value_cansleep(gpio_usbvid->vbus_gpio);
if (vbus_current == VBUS_OFF)
extcon_set_cable_state(_usbvid->edev, "USB", false);
else
extcon_set_cable_state(_usbvid->edev, "USB", true);
return IRQ_HANDLED;
}
I know your intention dividing interrupt handler for each cable.
But I don't think this driver must guarantee all of extcon device using gpio.
For example,
below three SoC wish to detect USB/USB-HOST cable with each different methods.
"SoC A" wish to detect USB/USB-HOST cable through only one gpio pin.
You mean to say that both USB ID pin and VBUS are connected to same gpio?
If so that is a really bad h/w design and it will not fly.
Or, you meant that only USB ID pin is connected to single gpio and it
controls the state of the USB/USB-HOST cables?
If so thats what exactly the v3 driver did with compatible gpio-usb-id.
"SoC B" wish to detect USB/USB-HOST cable through ADC value instead of gpio pin.
Via ADC this driver should never be used , it should be
extcon-adc-usbvid driver and not extcon-gpio-usbvid driver.
"SoC C" wish to detect USB/USB-HOST cable through two gpio pin because USB
connected on gpio an USB-HOST connected on another.
Whatever modifications above should meet this need in combination with
named gpios (id_gpio and vbus_gpio in dt)as stephen pointed.
But still
Re: [PATCH v3 1/3] extcon: extcon-gpio-usbvid: Generic USB VBUS/ID detection via GPIO
Hi Chanwoo,
On 8/30/2013 5:41 AM, Chanwoo Choi wrote:
Hi George,
On 08/29/2013 10:45 PM, George Cherian wrote:
Hi Chanwoo,
On 8/29/2013 5:42 PM, Chanwoo Choi wrote:
[big snip ]
I tested various development board based on Samsung Exynos series SoC.
Although some gpio of Exynos series SoC set high state(non zero, 1) as default
value,
this gpio state could mean off state, disconnected or un-powered state
according to gpio.
Of course, above explanation about specific gpio don't include all gpios.
This is meaning that there is possibility.
okay then how about adding a flag for inverted logic too. something like this
if(of_property_read_bool(np,inverted_gpio))
gpio_usbvid-gpio_inv = 1;
And always check on this before deciding?
Is this fine ?
OK.
But, as Stephen commented on other mail, you should use proper DT helper
function.
okay
Second,
gpio_usbvid_set_initial_state() dertermine both USB-HOST and USB cable
state at same time
in 'case ID_DETCT' according to 'gpio_usbvid-id_gpio'. But, I think that other
extcon devices
would not control both USB-HOST and USB cable state at same time.
Other extcon devices would support either USB-HOST or USB cable.
The driver has 2 configurations.
1) supports implementations with both VBUS and ID pin are routed via gpio's for
swicthing roles(compatible gpio-usb-vid).
As you explained about case 1, it is only used on dra7x SoC.
No gpio-usb-id is used in dra7xx. dra7xx has got only ID pin routed via gpio.
Other SoC could not wish to control both USB-HOST and USB cable at same
time.
I could'nt actually parse this. You meant since the id_irq_handler handles both
USB and USB-HOST cable
its not proper?
It's not proper in general case. The generic driver must guarantee all of
extcon device using gpio.
As far as I know, the generic driver cannot direclty control gpio pin and get
value from gpio pin.
Almost device driver including in kernel/drivers control gpio pin on specific
device driver
instead of generic driver.
But without reading the gpio value how can we set the cable states? for
this driver the assumption is the dedicated gpio
is always DIR_IN and in the driver we just read the value.
I need your answer about above my opinion for other SoC.
So how about this, I have removed the dra7x specific stuffs (gpio-usb-id)
static void gpio_usbvid_set_initial_state(struct gpio_usbvid *gpio_usbvid)
{
int id_current, vbus_current;
id_current = gpio_get_value_cansleep(gpio_usbvid-id_gpio);
if (!!id_current == ID_FLOAT)
extcon_set_cable_state(gpio_usbvid-edev, USB-HOST, false);
else
extcon_set_cable_state(gpio_usbvid-edev, USB-HOST, true);
vbus_current = gpio_get_value_cansleep(gpio_usbvid-vbus_gpio);
if (!!vbus_current == VBUS_ON)
extcon_set_cable_state(gpio_usbvid-edev, USB, true);
else
extcon_set_cable_state(gpio_usbvid-edev, USB, false);
}
and the irq handlers like this
static irqreturn_t id_irq_handler(int irq, void *data)
{
struct gpio_usbvid *gpio_usbvid = (struct gpio_usbvid *)data;
int id_current;
id_current = gpio_get_value_cansleep(gpio_usbvid-id_gpio);
if (id_current == ID_GND)
extcon_set_cable_state(gpio_usbvid-edev, USB-HOST, true);
else
extcon_set_cable_state(gpio_usbvid-edev, USB-HOST, false);
return IRQ_HANDLED;
}
static irqreturn_t vbus_irq_handler(int irq, void *data)
{
struct gpio_usbvid *gpio_usbvid = (struct gpio_usbvid *)data;
int vbus_current;
vbus_current = gpio_get_value_cansleep(gpio_usbvid-vbus_gpio);
if (vbus_current == VBUS_OFF)
extcon_set_cable_state(gpio_usbvid-edev, USB, false);
else
extcon_set_cable_state(gpio_usbvid-edev, USB, true);
return IRQ_HANDLED;
}
I know your intention dividing interrupt handler for each cable.
But I don't think this driver must guarantee all of extcon device using gpio.
For example,
below three SoC wish to detect USB/USB-HOST cable with each different methods.
SoC A wish to detect USB/USB-HOST cable through only one gpio pin.
You mean to say that both USB ID pin and VBUS are connected to same gpio?
If so that is a really bad h/w design and it will not fly.
Or, you meant that only USB ID pin is connected to single gpio and it
controls the state of the USB/USB-HOST cables?
If so thats what exactly the v3 driver did with compatible gpio-usb-id.
SoC B wish to detect USB/USB-HOST cable through ADC value instead of gpio pin.
Via ADC this driver should never be used , it should be
extcon-adc-usbvid driver and not extcon-gpio-usbvid driver.
SoC C wish to detect USB/USB-HOST cable through two gpio pin because USB
connected on gpio an USB-HOST connected on another.
Whatever modifications above should meet this need in combination with
named gpios (id_gpio and vbus_gpio in dt)as stephen pointed.
But still i feel the above
Re: [PATCH v3 1/3] extcon: extcon-gpio-usbvid: Generic USB VBUS/ID detection via GPIO
Hi George,
On 08/30/2013 03:15 PM, George Cherian wrote:
Hi Chanwoo,
On 8/30/2013 5:41 AM, Chanwoo Choi wrote:
Hi George,
On 08/29/2013 10:45 PM, George Cherian wrote:
Hi Chanwoo,
On 8/29/2013 5:42 PM, Chanwoo Choi wrote:
[big snip ]
I tested various development board based on Samsung Exynos series SoC.
Although some gpio of Exynos series SoC set high state(non zero, 1) as
default value,
this gpio state could mean off state, disconnected or un-powered state
according to gpio.
Of course, above explanation about specific gpio don't include all gpios.
This is meaning that there is possibility.
okay then how about adding a flag for inverted logic too. something like
this
if(of_property_read_bool(np,inverted_gpio))
gpio_usbvid-gpio_inv = 1;
And always check on this before deciding?
Is this fine ?
OK.
But, as Stephen commented on other mail, you should use proper DT helper
function.
okay
Second,
gpio_usbvid_set_initial_state() dertermine both USB-HOST and USB
cable state at same time
in 'case ID_DETCT' according to 'gpio_usbvid-id_gpio'. But, I think
that other extcon devices
would not control both USB-HOST and USB cable state at same time.
Other extcon devices would support either USB-HOST or USB cable.
The driver has 2 configurations.
1) supports implementations with both VBUS and ID pin are routed via
gpio's for swicthing roles(compatible gpio-usb-vid).
As you explained about case 1, it is only used on dra7x SoC.
No gpio-usb-id is used in dra7xx. dra7xx has got only ID pin routed via
gpio.
Other SoC could not wish to control both USB-HOST and USB cable at
same time.
I could'nt actually parse this. You meant since the id_irq_handler handles
both USB and USB-HOST cable
its not proper?
It's not proper in general case. The generic driver must guarantee all of
extcon device using gpio.
As far as I know, the generic driver cannot direclty control gpio pin and
get value from gpio pin.
Almost device driver including in kernel/drivers control gpio pin on
specific device driver
instead of generic driver.
But without reading the gpio value how can we set the cable states? for this
driver the assumption is the dedicated gpio
is always DIR_IN and in the driver we just read the value.
I need your answer about above my opinion for other SoC.
So how about this, I have removed the dra7x specific stuffs (gpio-usb-id)
static void gpio_usbvid_set_initial_state(struct gpio_usbvid *gpio_usbvid)
{
int id_current, vbus_current;
id_current = gpio_get_value_cansleep(gpio_usbvid-id_gpio);
if (!!id_current == ID_FLOAT)
extcon_set_cable_state(gpio_usbvid-edev, USB-HOST, false);
else
extcon_set_cable_state(gpio_usbvid-edev, USB-HOST, true);
vbus_current = gpio_get_value_cansleep(gpio_usbvid-vbus_gpio);
if (!!vbus_current == VBUS_ON)
extcon_set_cable_state(gpio_usbvid-edev, USB, true);
else
extcon_set_cable_state(gpio_usbvid-edev, USB, false);
}
and the irq handlers like this
static irqreturn_t id_irq_handler(int irq, void *data)
{
struct gpio_usbvid *gpio_usbvid = (struct gpio_usbvid *)data;
int id_current;
id_current = gpio_get_value_cansleep(gpio_usbvid-id_gpio);
if (id_current == ID_GND)
extcon_set_cable_state(gpio_usbvid-edev, USB-HOST,
true);
else
extcon_set_cable_state(gpio_usbvid-edev, USB-HOST,
false);
return IRQ_HANDLED;
}
static irqreturn_t vbus_irq_handler(int irq, void *data)
{
struct gpio_usbvid *gpio_usbvid = (struct gpio_usbvid *)data;
int vbus_current;
vbus_current = gpio_get_value_cansleep(gpio_usbvid-vbus_gpio);
if (vbus_current == VBUS_OFF)
extcon_set_cable_state(gpio_usbvid-edev, USB, false);
else
extcon_set_cable_state(gpio_usbvid-edev, USB, true);
return IRQ_HANDLED;
}
I know your intention dividing interrupt handler for each cable.
But I don't think this driver must guarantee all of extcon device using gpio.
For example,
below three SoC wish to detect USB/USB-HOST cable with each different
methods.
SoC A wish to detect USB/USB-HOST cable through only one gpio pin.
You mean to say that both USB ID pin and VBUS are connected to same gpio?
If so that is a really bad h/w design and it will not fly.
Or, you meant that only USB ID pin is connected to single gpio and it
controls the state of the USB/USB-HOST cables?
If so thats what exactly the v3 driver did with compatible gpio-usb-id.
My original question intention,
I'd like you to answer that this driver can support all case of SoC A/SoC
B/SoC C without othe implementation?
SoC B wish to detect USB/USB-HOST cable through ADC value instead of gpio
pin.
Via ADC this driver should never be used , it should be extcon-adc-usbvid
driver and not extcon-gpio-usbvid
Re: [PATCH v3 1/3] extcon: extcon-gpio-usbvid: Generic USB VBUS/ID detection via GPIO
Hi George,
In addition, I add answer about that device driver control gpio pin directly.
On 08/30/2013 03:15 PM, George Cherian wrote:
Hi Chanwoo,
On 8/30/2013 5:41 AM, Chanwoo Choi wrote:
Hi George,
On 08/29/2013 10:45 PM, George Cherian wrote:
Hi Chanwoo,
On 8/29/2013 5:42 PM, Chanwoo Choi wrote:
[big snip ]
I tested various development board based on Samsung Exynos series SoC.
Although some gpio of Exynos series SoC set high state(non zero, 1) as
default value,
this gpio state could mean off state, disconnected or un-powered state
according to gpio.
Of course, above explanation about specific gpio don't include all gpios.
This is meaning that there is possibility.
okay then how about adding a flag for inverted logic too. something like
this
if(of_property_read_bool(np,inverted_gpio))
gpio_usbvid-gpio_inv = 1;
And always check on this before deciding?
Is this fine ?
OK.
But, as Stephen commented on other mail, you should use proper DT helper
function.
okay
Second,
gpio_usbvid_set_initial_state() dertermine both USB-HOST and USB
cable state at same time
in 'case ID_DETCT' according to 'gpio_usbvid-id_gpio'. But, I think
that other extcon devices
would not control both USB-HOST and USB cable state at same time.
Other extcon devices would support either USB-HOST or USB cable.
The driver has 2 configurations.
1) supports implementations with both VBUS and ID pin are routed via
gpio's for swicthing roles(compatible gpio-usb-vid).
As you explained about case 1, it is only used on dra7x SoC.
No gpio-usb-id is used in dra7xx. dra7xx has got only ID pin routed via
gpio.
Other SoC could not wish to control both USB-HOST and USB cable at
same time.
I could'nt actually parse this. You meant since the id_irq_handler handles
both USB and USB-HOST cable
its not proper?
It's not proper in general case. The generic driver must guarantee all of
extcon device using gpio.
As far as I know, the generic driver cannot direclty control gpio pin and
get value from gpio pin.
Almost device driver including in kernel/drivers control gpio pin on
specific device driver
instead of generic driver.
But without reading the gpio value how can we set the cable states?
hmm. I mentioned above my answer as following:
As far as I know, the generic driver cannot direclty control gpio
pin and get value from gpio pin.
Almost device driver including in kernel/drivers control gpio pin on
specific device driver
Because your extcon driver directly control gpio pin so I think your extcon
driver isn't generic.
for this driver the assumption is the dedicated gpio
is always DIR_IN and in the driver we just read the value.
What is DIR_IN?
I need your answer about above my opinion for other SoC.
So how about this, I have removed the dra7x specific stuffs (gpio-usb-id)
static void gpio_usbvid_set_initial_state(struct gpio_usbvid *gpio_usbvid)
{
int id_current, vbus_current;
id_current = gpio_get_value_cansleep(gpio_usbvid-id_gpio);
if (!!id_current == ID_FLOAT)
extcon_set_cable_state(gpio_usbvid-edev, USB-HOST, false);
else
extcon_set_cable_state(gpio_usbvid-edev, USB-HOST, true);
vbus_current = gpio_get_value_cansleep(gpio_usbvid-vbus_gpio);
if (!!vbus_current == VBUS_ON)
extcon_set_cable_state(gpio_usbvid-edev, USB, true);
else
extcon_set_cable_state(gpio_usbvid-edev, USB, false);
}
and the irq handlers like this
static irqreturn_t id_irq_handler(int irq, void *data)
{
struct gpio_usbvid *gpio_usbvid = (struct gpio_usbvid *)data;
int id_current;
id_current = gpio_get_value_cansleep(gpio_usbvid-id_gpio);
if (id_current == ID_GND)
extcon_set_cable_state(gpio_usbvid-edev, USB-HOST,
true);
else
extcon_set_cable_state(gpio_usbvid-edev, USB-HOST,
false);
return IRQ_HANDLED;
}
static irqreturn_t vbus_irq_handler(int irq, void *data)
{
struct gpio_usbvid *gpio_usbvid = (struct gpio_usbvid *)data;
int vbus_current;
vbus_current = gpio_get_value_cansleep(gpio_usbvid-vbus_gpio);
if (vbus_current == VBUS_OFF)
extcon_set_cable_state(gpio_usbvid-edev, USB, false);
else
extcon_set_cable_state(gpio_usbvid-edev, USB, true);
return IRQ_HANDLED;
}
I know your intention dividing interrupt handler for each cable.
But I don't think this driver must guarantee all of extcon device using gpio.
For example,
below three SoC wish to detect USB/USB-HOST cable with each different
methods.
SoC A wish to detect USB/USB-HOST cable through only one gpio pin.
You mean to say that both USB ID pin and VBUS are connected to same gpio?
If so that is a really bad h/w design and it will not fly.
Or, you meant that only USB ID pin is connected to single gpio and it
controls the
Re: [PATCH v3 1/3] extcon: extcon-gpio-usbvid: Generic USB VBUS/ID detection via GPIO
Hi George,
On 08/29/2013 10:45 PM, George Cherian wrote:
> Hi Chanwoo,
>
>
> On 8/29/2013 5:42 PM, Chanwoo Choi wrote:
> [big snip ]
I tested various development board based on Samsung Exynos series SoC.
Although some gpio of Exynos series SoC set high state(non zero, 1) as
default value,
this gpio state could mean off state, disconnected or un-powered state
according to gpio.
Of course, above explanation about specific gpio don't include all gpios.
This is meaning that there is possibility.
>>> okay then how about adding a flag for inverted logic too. something like
>>> this
>>>
>>> if(of_property_read_bool(np,"inverted_gpio))
>>> gpio_usbvid->gpio_inv = 1;
>>> And always check on this before deciding?
> Is this fine ?
OK.
But, as Stephen commented on other mail, you should use proper DT helper
function.
>>>
>> Second,
>> gpio_usbvid_set_initial_state() dertermine both "USB-HOST" and "USB"
>> cable state at same time
>> in 'case ID_DETCT' according to 'gpio_usbvid->id_gpio'. But, I think
>> that other extcon devices
>> would not control both "USB-HOST" and "USB" cable state at same time.
>>
>> Other extcon devices would support either "USB-HOST" or "USB" cable.
> The driver has 2 configurations.
> 1) supports implementations with both VBUS and ID pin are routed via
> gpio's for swicthing roles(compatible gpio-usb-vid).
As you explained about case 1, it is only used on dra7x SoC.
>>> No gpio-usb-id is used in dra7xx. dra7xx has got only ID pin routed via
>>> gpio.
Other SoC could not wish to control both "USB-HOST" and "USB" cable at
same time.
> I could'nt actually parse this. You meant since the id_irq_handler handles
> both USB and USB-HOST cable
> its not proper?
It's not proper in general case. The generic driver must guarantee all of
extcon device using gpio.
As far as I know, the generic driver cannot direclty control gpio pin and get
value from gpio pin.
Almost device driver including in kernel/drivers control gpio pin on specific
device driver
instead of generic driver.
>> I need your answer about above my opinion for other SoC.
> So how about this, I have removed the dra7x specific stuffs (gpio-usb-id)
>
> static void gpio_usbvid_set_initial_state(struct gpio_usbvid *gpio_usbvid)
> {
> int id_current, vbus_current;
>
> id_current = gpio_get_value_cansleep(gpio_usbvid->id_gpio);
> if (!!id_current == ID_FLOAT)
> extcon_set_cable_state(_usbvid->edev, "USB-HOST", false);
> else
> extcon_set_cable_state(_usbvid->edev, "USB-HOST", true);
>
> vbus_current = gpio_get_value_cansleep(gpio_usbvid->vbus_gpio);
> if (!!vbus_current == VBUS_ON)
> extcon_set_cable_state(_usbvid->edev, "USB", true);
> else
> extcon_set_cable_state(_usbvid->edev, "USB", false);
> }
>
> and the irq handlers like this
>
> static irqreturn_t id_irq_handler(int irq, void *data)
> {
> struct gpio_usbvid *gpio_usbvid = (struct gpio_usbvid *)data;
> int id_current;
>
> id_current = gpio_get_value_cansleep(gpio_usbvid->id_gpio);
> if (id_current == ID_GND)
> extcon_set_cable_state(_usbvid->edev, "USB-HOST", true);
> else
> extcon_set_cable_state(_usbvid->edev, "USB-HOST", false);
> return IRQ_HANDLED;
> }
>
> static irqreturn_t vbus_irq_handler(int irq, void *data)
> {
> struct gpio_usbvid *gpio_usbvid = (struct gpio_usbvid *)data;
> int vbus_current;
>
> vbus_current = gpio_get_value_cansleep(gpio_usbvid->vbus_gpio);
> if (vbus_current == VBUS_OFF)
> extcon_set_cable_state(_usbvid->edev, "USB", false);
> else
> extcon_set_cable_state(_usbvid->edev, "USB", true);
>
> return IRQ_HANDLED;
> }
I know your intention dividing interrupt handler for each cable.
But I don't think this driver must guarantee all of extcon device using gpio.
For example,
below three SoC wish to detect USB/USB-HOST cable with each different methods.
"SoC A" wish to detect USB/USB-HOST cable through only one gpio pin.
"SoC B" wish to detect USB/USB-HOST cable through ADC value instead of gpio pin.
"SoC C" wish to detect USB/USB-HOST cable through two gpio pin because USB
connected on gpio an USB-HOST connected on another.
In addition,
if "SoC A/C" wish to write some data to own specific registers for proper
opeating,
Could we write some data to register on generic driver?
Finally,
"SoC D" wish to detect USB/USB-HOST/JIG cable through two gpio pin
- one gpio may detect either USB or USB-HOST and another may detect JIG cable
or one gpio may detect either USb or JIG and another may detect USB-HOST cable.
That way, there are many cases we cannot guarantee all of extcon devices.
I think this driver could support dra7x series SoC but as I mentioned,
this driver may not guarantee all of cases.
> [snip]
I
Re: [PATCH v3 1/3] extcon: extcon-gpio-usbvid: Generic USB VBUS/ID detection via GPIO
On 08/28/2013 11:33 AM, George Cherian wrote:
> Add a generic USB VBUS/ID detection EXTCON driver. This driver expects
> the ID/VBUS pin are connected via GPIOs. This driver is tested on
> DRA7x board were the ID pin is routed via GPIOs. The driver supports
> both VBUS and ID pin configuration and ID pin only configuration.
> diff --git a/Documentation/devicetree/bindings/extcon/extcon-gpio-usbvid.txt
> b/Documentation/devicetree/bindings/extcon/extcon-gpio-usbvid.txt
> +EXTCON FOR USB VIA GPIO
Please write a brief explanation of the HW that this binding represents.
"Extcon" is a Linux-specific term. Binding documentation should be
written in an OS-agnostic manner. Bindings should not reference
OS-specific terminology, and should be a pure description of the HW.
> +Required Properties:
> + - compatible : Should be "ti,gpio-usb-vid" for USB VBUS-ID detector
> + using gpios or "ti,gpio-usb-id" for USB ID pin detector
Those souond like two rather different things. Should they be separate
bindings, or at the least, separate sections in the document?
If this binding is meant to represent some generic hardware, the vendor
prefix probably isn't required. So, remove "ti," from the compatible values.
> + - gpios : phandle and args ID pin gpio and VBUS gpio.
s/and args/and specifier/.
> +The first gpio used for ID pin detection
> +and the second used for VBUS detection.
> +ID pin gpio is mandatory and VBUS is optional
> +depending on implementation.
It'd be better to use named GPIO properties here, rather than having
multiple different kinds of GPIO in the same property. How about
"id-gpios" and "vbus-gpios" as the property names?
The semantics of each property should be specified. Are these input
GPIOs that reflect the ID and VBUS state? Do they directly represent the
signal state on the connector, or are they processed somehow? Does the
VBUS GPIO control the board's VBUS output, or is it an input? If it
controls VBUS output, it probably should be represented as a regulator
rather than a GPIO.
I think the following might work:
Required properties:
id-gpios: GPIO specifier for the connector's ID pin input. This directly
represents the value present on the ID pin at the connector.
Optional properties:
vbus-gpios: GPIO specifier for the connector's VBUS signal. This
directly represents whether VBUS being driven by the USB cable itself.
(although perhaps that isn't an optional property, but rather a required
property of the second compatible value?)
> +Please refer to ../gpio/gpio.txt for details of the common GPIO bindings
> +
> +Example:
> +
> + gpio_usbvid_extcon1 {
> + compatible = "ti,gpio-usb-vid";
> + gpios = < 1 0>,
> + < 2 0>;
> + };
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 1/3] extcon: extcon-gpio-usbvid: Generic USB VBUS/ID detection via GPIO
On 08/29/2013 05:48 AM, George Cherian wrote: > On 8/29/2013 4:07 PM, Chanwoo Choi wrote: ... >> I tested various development board based on Samsung Exynos series SoC. >> Although some gpio of Exynos series SoC set high state(non zero, 1) as >> default value, >> this gpio state could mean off state, disconnected or un-powered state >> according to gpio. >> Of course, above explanation about specific gpio don't include all gpios. >> This is meaning that there is possibility. > > okay then how about adding a flag for inverted logic too. something > like this > > if(of_property_read_bool(np,"inverted_gpio)) > gpio_usbvid->gpio_inv = 1; > > And always check on this before deciding? Don't do this. GPIO specifiers in DT typically include a "flags" cell that describes certain GPIO properties. One of those properties is signal inversion. You can use of_get_named_gpio_flags() to retrieve those flags. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 1/3] extcon: extcon-gpio-usbvid: Generic USB VBUS/ID detection via GPIO
Hi Chanwoo,
On 8/29/2013 5:42 PM, Chanwoo Choi wrote:
[big snip ]
I tested various development board based on Samsung Exynos series SoC.
Although some gpio of Exynos series SoC set high state(non zero, 1) as default
value,
this gpio state could mean off state, disconnected or un-powered state
according to gpio.
Of course, above explanation about specific gpio don't include all gpios.
This is meaning that there is possibility.
okay then how about adding a flag for inverted logic too. something like this
if(of_property_read_bool(np,"inverted_gpio))
gpio_usbvid->gpio_inv = 1;
And always check on this before deciding?
Is this fine ?
Second,
gpio_usbvid_set_initial_state() dertermine both "USB-HOST" and "USB" cable
state at same time
in 'case ID_DETCT' according to 'gpio_usbvid->id_gpio'. But, I think that other
extcon devices
would not control both "USB-HOST" and "USB" cable state at same time.
Other extcon devices would support either "USB-HOST" or "USB" cable.
The driver has 2 configurations.
1) supports implementations with both VBUS and ID pin are routed via gpio's for
swicthing roles(compatible gpio-usb-vid).
As you explained about case 1, it is only used on dra7x SoC.
No gpio-usb-id is used in dra7xx. dra7xx has got only ID pin routed via gpio.
Other SoC could not wish to control both "USB-HOST" and "USB" cable at same
time.
I could'nt actually parse this. You meant since the id_irq_handler
handles both USB and USB-HOST cable
its not proper?
I need your answer about above my opinion for other SoC.
So how about this, I have removed the dra7x specific stuffs (gpio-usb-id)
static void gpio_usbvid_set_initial_state(struct gpio_usbvid *gpio_usbvid)
{
int id_current, vbus_current;
id_current = gpio_get_value_cansleep(gpio_usbvid->id_gpio);
if (!!id_current == ID_FLOAT)
extcon_set_cable_state(_usbvid->edev, "USB-HOST", false);
else
extcon_set_cable_state(_usbvid->edev, "USB-HOST", true);
vbus_current = gpio_get_value_cansleep(gpio_usbvid->vbus_gpio);
if (!!vbus_current == VBUS_ON)
extcon_set_cable_state(_usbvid->edev, "USB", true);
else
extcon_set_cable_state(_usbvid->edev, "USB", false);
}
and the irq handlers like this
static irqreturn_t id_irq_handler(int irq, void *data)
{
struct gpio_usbvid *gpio_usbvid = (struct gpio_usbvid *)data;
int id_current;
id_current = gpio_get_value_cansleep(gpio_usbvid->id_gpio);
if (id_current == ID_GND)
extcon_set_cable_state(_usbvid->edev, "USB-HOST",
true);
else
extcon_set_cable_state(_usbvid->edev, "USB-HOST",
false);
return IRQ_HANDLED;
}
static irqreturn_t vbus_irq_handler(int irq, void *data)
{
struct gpio_usbvid *gpio_usbvid = (struct gpio_usbvid *)data;
int vbus_current;
vbus_current = gpio_get_value_cansleep(gpio_usbvid->vbus_gpio);
if (vbus_current == VBUS_OFF)
extcon_set_cable_state(_usbvid->edev, "USB", false);
else
extcon_set_cable_state(_usbvid->edev, "USB", true);
return IRQ_HANDLED;
}
[snip]
I have some confusion. I need additional your explanation.
Could this driver register only one interrupt handler either id_irq_handler()
or vbus_irq_handler()?
If compatible == ID_DETECT, only one handler --> id_irq_handler() and it will
handle both USB and USB-HOST
or
Could this driver register two interrupt handler both id_irq_handler() or
vbus_irq_handler()?
If compatible == VBUS_ID_DETECT, 2 handlers --> id_irq_handler() will handle
USB-HOST and vbus_irq_handler will handle USB.
As you mentioned, we cannot only control either USB or USB-HOST cable on this
extcon driver.
This extcon driver is only suitable dra7x SoC.
Do you still feel its dra7x specific if i modify it as above?
Because id_irq_handler() control both "USB-HOST" and "USB" cable at same time,
you need this setting order between "USB-HOST" and "USB" cable.
yes
I think that the setting order between cables isn't general. It is specific
method for dra7x SoC.
So if i remove that part then?
Did you think that SoC should always connect either "USB-HOST" cable or "USB"
cable?
No, even if a physical cable is not connected it should default to either
USB-HOST or USB
It isn't general.
If physical cable isn't connected to extcon device, the state both USB-HOST and
USB cable
should certainly be zero. Because The extcon consumer driver could set proper
operation
according to cable state.
okay
I don't know this case except for dra7x SoC. So, I think it has dependency on
specific SoC.
I need your answer about above my opinion.
Hope i could answer you :-)
and can't agree as generic extcon gpio driver.
--
-George
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read
Re: [PATCH v3 1/3] extcon: extcon-gpio-usbvid: Generic USB VBUS/ID detection via GPIO
On 08/29/2013 08:48 PM, George Cherian wrote: > On 8/29/2013 4:07 PM, Chanwoo Choi wrote: >> On 08/29/2013 04:30 PM, George Cherian wrote: >>> Hi Chanwoo, >>> >>> On 8/29/2013 11:53 AM, Chanwoo Choi wrote: >>> [snip] You should keep following naming stlye. extcon-gpio-usbvid.c is wrong naming style. - extcon-[device name].c - extcon-gpio-usbvid.c -> extcon-dra7xx.c or etc. > Actually dra7xx is the SoC name and the USB VBUS/ID detection is not > specific to SoC. > It uses gpios to detect the VBUS/ID change. So i thought it would be > better to have generic > gpio based VBUS/ID detection rather than making dra7xx specific. Stephen > Warren had this opinion > with patch v1. Would you guarantee that this driver support all of extcon devices using gpio pin? >>> This driver would guarantee extcon devices using gpio pins for USB VBUS and >>> ID detection. >>> Following is the basic assumption made, correct me if I am wrong. >>> ID pin ground means -> USB-HOST is true (this happens when a device is >>> connected to USB port and we act as HOST ) >>> ID pin Float means -> USB-HOST is false (this happens when nothing is >>> connected or when we act as a peripheral under a HOST) >>> VBUS ON means -> USB is true (this happens when we are connected under a >>> HOST as a peripheral) >>> VBUS OFF means -> USB is false ( this happens when we are either >>> disconnected from a HOST or when we are in HOST mode). >>> >>> So normally USB is in peripheral mode is enabled when ID pin is float and >>> VBUS is ON. >>> and USB is in HOST mode when ID pin is ground and VBUS is OFF. >>> >>> In all above cases VBUS is referred to the external VBUS supply from an >>> external HOST. >>> I can't agree. This driver has specific dependency on dra7x SoC. First, vbus_irq_handler() determine USB cable state according to "gpio_usbvid->vbus_gpio" state. If "gpio_usbvid->vbus_gpio" value is VBUS_OFF(0), this patch set USB cable state as 'false' But, it include potential problems. Other extcon device using gpio would set USB cable state as 'true' when gpio state is 1. This way has dependency on specific SoC. >>> no this is not SoC specific. VBUS is referred to the external VBUS supply >>> from an external HOST. >>> so if VBUS is zero means its definitely not in connected state. >> I tested various development board based on Samsung Exynos series SoC. >> Although some gpio of Exynos series SoC set high state(non zero, 1) as >> default value, >> this gpio state could mean off state, disconnected or un-powered state >> according to gpio. >> Of course, above explanation about specific gpio don't include all gpios. >> This is meaning that there is possibility. > okay then how about adding a flag for inverted logic too. something like this > > if(of_property_read_bool(np,"inverted_gpio)) > gpio_usbvid->gpio_inv = 1; > And always check on this before deciding? > Second, gpio_usbvid_set_initial_state() dertermine both "USB-HOST" and "USB" cable state at same time in 'case ID_DETCT' according to 'gpio_usbvid->id_gpio'. But, I think that other extcon devices would not control both "USB-HOST" and "USB" cable state at same time. Other extcon devices would support either "USB-HOST" or "USB" cable. >>> The driver has 2 configurations. >>> 1) supports implementations with both VBUS and ID pin are routed via gpio's >>> for swicthing roles(compatible gpio-usb-vid). >> As you explained about case 1, it is only used on dra7x SoC. > No gpio-usb-id is used in dra7xx. dra7xx has got only ID pin routed via gpio. >> Other SoC could not wish to control both "USB-HOST" and "USB" cable at same >> time. I need your answer about above my opinion for other SoC. >> >> >>> 2) supports implementations with only ID pin routed via gpio for swicthing >>> roles(compatible gpio-usb-id). >>> So if you take type as VBUS_ID_DETECT then it is as what you meant. > I think i didnt explain it properly last time. > In perfect world you will have both VBUS and ID pin routed via gpios > for eg: VBUS via gpio2 and ID via gpio3. if this is the case then we have to > use compatible gpio-usb-vid where in 2 gpios will be used > 2 irq handlers will be installed and it will control both USB-HOST and USB > cables. In this case its possible to have 3 states > USB-HOST (true), USB(true) and both false. > > Now in case of dra7xx the board designers chose not to route the VBUS to gpio > and only ID pin is routed. > But still we need to differentiate USB-HOST and USB cables In such cases we > use compatible gpio-usb-id where in 1 gpio will be used > 1 irq handler is installed and it will control both USB-HOST and USB > cables. In this case its possible to have only 2 states > USB-HOST (true) or USB(true). > > Now there could be a 3rd scenario were in only VBUS is routed via gpio, that > we would not support since we
Re: [PATCH v3 1/3] extcon: extcon-gpio-usbvid: Generic USB VBUS/ID detection via GPIO
On 8/29/2013 4:07 PM, Chanwoo Choi wrote: On 08/29/2013 04:30 PM, George Cherian wrote: Hi Chanwoo, On 8/29/2013 11:53 AM, Chanwoo Choi wrote: [snip] You should keep following naming stlye. extcon-gpio-usbvid.c is wrong naming style. - extcon-[device name].c - extcon-gpio-usbvid.c -> extcon-dra7xx.c or etc. Actually dra7xx is the SoC name and the USB VBUS/ID detection is not specific to SoC. It uses gpios to detect the VBUS/ID change. So i thought it would be better to have generic gpio based VBUS/ID detection rather than making dra7xx specific. Stephen Warren had this opinion with patch v1. Would you guarantee that this driver support all of extcon devices using gpio pin? This driver would guarantee extcon devices using gpio pins for USB VBUS and ID detection. Following is the basic assumption made, correct me if I am wrong. ID pin ground means -> USB-HOST is true (this happens when a device is connected to USB port and we act as HOST ) ID pin Float means -> USB-HOST is false (this happens when nothing is connected or when we act as a peripheral under a HOST) VBUS ON means -> USB is true (this happens when we are connected under a HOST as a peripheral) VBUS OFF means -> USB is false ( this happens when we are either disconnected from a HOST or when we are in HOST mode). So normally USB is in peripheral mode is enabled when ID pin is float and VBUS is ON. and USB is in HOST mode when ID pin is ground and VBUS is OFF. In all above cases VBUS is referred to the external VBUS supply from an external HOST. I can't agree. This driver has specific dependency on dra7x SoC. First, vbus_irq_handler() determine USB cable state according to "gpio_usbvid->vbus_gpio" state. If "gpio_usbvid->vbus_gpio" value is VBUS_OFF(0), this patch set USB cable state as 'false' But, it include potential problems. Other extcon device using gpio would set USB cable state as 'true' when gpio state is 1. This way has dependency on specific SoC. no this is not SoC specific. VBUS is referred to the external VBUS supply from an external HOST. so if VBUS is zero means its definitely not in connected state. I tested various development board based on Samsung Exynos series SoC. Although some gpio of Exynos series SoC set high state(non zero, 1) as default value, this gpio state could mean off state, disconnected or un-powered state according to gpio. Of course, above explanation about specific gpio don't include all gpios. This is meaning that there is possibility. okay then how about adding a flag for inverted logic too. something like this if(of_property_read_bool(np,"inverted_gpio)) gpio_usbvid->gpio_inv = 1; And always check on this before deciding? Second, gpio_usbvid_set_initial_state() dertermine both "USB-HOST" and "USB" cable state at same time in 'case ID_DETCT' according to 'gpio_usbvid->id_gpio'. But, I think that other extcon devices would not control both "USB-HOST" and "USB" cable state at same time. Other extcon devices would support either "USB-HOST" or "USB" cable. The driver has 2 configurations. 1) supports implementations with both VBUS and ID pin are routed via gpio's for swicthing roles(compatible gpio-usb-vid). As you explained about case 1, it is only used on dra7x SoC. No gpio-usb-id is used in dra7xx. dra7xx has got only ID pin routed via gpio. Other SoC could not wish to control both "USB-HOST" and "USB" cable at same time. 2) supports implementations with only ID pin routed via gpio for swicthing roles(compatible gpio-usb-id). So if you take type as VBUS_ID_DETECT then it is as what you meant. I think i didnt explain it properly last time. In perfect world you will have both VBUS and ID pin routed via gpios for eg: VBUS via gpio2 and ID via gpio3. if this is the case then we have to use compatible gpio-usb-vid where in 2 gpios will be used 2 irq handlers will be installed and it will control both USB-HOST and USB cables. In this case its possible to have 3 states USB-HOST (true), USB(true) and both false. Now in case of dra7xx the board designers chose not to route the VBUS to gpio and only ID pin is routed. But still we need to differentiate USB-HOST and USB cables In such cases we use compatible gpio-usb-id where in 1 gpio will be used 1 irq handler is installed and it will control both USB-HOST and USB cables. In this case its possible to have only 2 states USB-HOST (true) or USB(true). Now there could be a 3rd scenario were in only VBUS is routed via gpio, that we would not support since we cant assume the value of ID pin and configure the UTMI is not proper. So I have mentioned even in documentation that ID pin is mandatory. We can always assume role depending on ID pin. "2) case" don't support the detection of "USB-HOST" cable. Only detect "USB" cable according to "vbus_gpio" value. If user wish to detect only "USB-HOST" cable, should user enter "ID_DETECT" as "ti,gpio-usb-id" on DT file? But, id_irq_handler() control both
Re: [PATCH v3 1/3] extcon: extcon-gpio-usbvid: Generic USB VBUS/ID detection via GPIO
On 08/29/2013 04:30 PM, George Cherian wrote: > Hi Chanwoo, > > On 8/29/2013 11:53 AM, Chanwoo Choi wrote: > [snip] >> You should keep following naming stlye. extcon-gpio-usbvid.c is wrong naming >> style. >> - extcon-[device name].c >> - extcon-gpio-usbvid.c -> extcon-dra7xx.c or etc. >>> Actually dra7xx is the SoC name and the USB VBUS/ID detection is not >>> specific to SoC. >>> It uses gpios to detect the VBUS/ID change. So i thought it would be better >>> to have generic >>> gpio based VBUS/ID detection rather than making dra7xx specific. Stephen >>> Warren had this opinion >>> with patch v1. >> Would you guarantee that this driver support all of extcon devices using >> gpio pin? > This driver would guarantee extcon devices using gpio pins for USB VBUS and > ID detection. > Following is the basic assumption made, correct me if I am wrong. > ID pin ground means -> USB-HOST is true (this happens when a device is > connected to USB port and we act as HOST ) > ID pin Float means -> USB-HOST is false (this happens when nothing is > connected or when we act as a peripheral under a HOST) > VBUS ON means -> USB is true (this happens when we are connected under a HOST > as a peripheral) > VBUS OFF means -> USB is false ( this happens when we are either disconnected > from a HOST or when we are in HOST mode). > > So normally USB is in peripheral mode is enabled when ID pin is float and > VBUS is ON. > and USB is in HOST mode when ID pin is ground and VBUS is OFF. > > In all above cases VBUS is referred to the external VBUS supply from an > external HOST. > >> I can't agree. This driver has specific dependency on dra7x SoC. >> >> First, >> vbus_irq_handler() determine USB cable state according to >> "gpio_usbvid->vbus_gpio" state. >> If "gpio_usbvid->vbus_gpio" value is VBUS_OFF(0), this patch set USB cable >> state as 'false' >> But, it include potential problems. Other extcon device using gpio would set >> USB cable state >> as 'true' when gpio state is 1. This way has dependency on specific SoC. > no this is not SoC specific. VBUS is referred to the external VBUS supply > from an external HOST. > so if VBUS is zero means its definitely not in connected state. I tested various development board based on Samsung Exynos series SoC. Although some gpio of Exynos series SoC set high state(non zero, 1) as default value, this gpio state could mean off state, disconnected or un-powered state according to gpio. Of course, above explanation about specific gpio don't include all gpios. This is meaning that there is possibility. >> >> Second, >> gpio_usbvid_set_initial_state() dertermine both "USB-HOST" and "USB" cable >> state at same time >> in 'case ID_DETCT' according to 'gpio_usbvid->id_gpio'. But, I think that >> other extcon devices >> would not control both "USB-HOST" and "USB" cable state at same time. >> >> Other extcon devices would support either "USB-HOST" or "USB" cable. > The driver has 2 configurations. > 1) supports implementations with both VBUS and ID pin are routed via gpio's > for swicthing roles(compatible gpio-usb-vid). As you explained about case 1, it is only used on dra7x SoC. Other SoC could not wish to control both "USB-HOST" and "USB" cable at same time. > 2) supports implementations with only ID pin routed via gpio for swicthing > roles(compatible gpio-usb-id). > So if you take type as VBUS_ID_DETECT then it is as what you meant. "2) case" don't support the detection of "USB-HOST" cable. Only detect "USB" cable according to "vbus_gpio" value. If user wish to detect only "USB-HOST" cable, should user enter "ID_DETECT" as "ti,gpio-usb-id" on DT file? But, id_irq_handler() control both "USB-HOST" and "USB" cable at same time. It may not prefer this method. >> >> Third, >> gpio_usbvid_probe() get both gpio_usbvid->id_irq and gpio_usbvid->vbus_irq >> by using DT helper function. >> and gpio_usbvid_request_irq() register each interrupt handler(id_irq and >> vbus_irq) according to DT data. >> In result, >> id_irq_handler() would control both "USB-HOST" and "USB" cable state. > only if type is ID_DETECT id_irq_handler control both USB-HOST and USB cable > states > if type is VBUS_ID_DETECT then id_irq_handler only controls USB-HOST. I have some confusion. I need additional your explanation. Could this driver register only one interrupt handler either id_irq_handler() or vbus_irq_handler()? or Could this driver register two interrupt handler both id_irq_handler() or vbus_irq_handler()? >> vbus_irq_handler() would control only "USB" cable state. >> >> Two interrupt handler(id_irq_handler()/vbus_irq_handler()) would control >> "USB" cable state >> according to each gpio state at same time. Also, It include critical problem. > No it depends on the configuration as explained above. > > [snip] > >> + >> +#define ID_GND0 >> +#define ID_FLOAT1 >> +#define VBUS_OFF0 >> +#define VBUS_ON1 I think you could only use two
Re: [PATCH v3 1/3] extcon: extcon-gpio-usbvid: Generic USB VBUS/ID detection via GPIO
Hi Chanwoo,
On 8/29/2013 11:53 AM, Chanwoo Choi wrote:
[snip]
You should keep following naming stlye. extcon-gpio-usbvid.c is wrong naming
style.
- extcon-[device name].c
- extcon-gpio-usbvid.c -> extcon-dra7xx.c or etc.
Actually dra7xx is the SoC name and the USB VBUS/ID detection is not specific
to SoC.
It uses gpios to detect the VBUS/ID change. So i thought it would be better to
have generic
gpio based VBUS/ID detection rather than making dra7xx specific. Stephen Warren
had this opinion
with patch v1.
Would you guarantee that this driver support all of extcon devices using gpio
pin?
This driver would guarantee extcon devices using gpio pins for USB VBUS
and ID detection.
Following is the basic assumption made, correct me if I am wrong.
ID pin ground means -> USB-HOST is true (this happens when a device is
connected to USB port and we act as HOST )
ID pin Float means -> USB-HOST is false (this happens when nothing is
connected or when we act as a peripheral under a HOST)
VBUS ON means -> USB is true (this happens when we are connected under a
HOST as a peripheral)
VBUS OFF means -> USB is false ( this happens when we are either
disconnected from a HOST or when we are in HOST mode).
So normally USB is in peripheral mode is enabled when ID pin is float
and VBUS is ON.
and USB is in HOST mode when ID pin is ground and VBUS is OFF.
In all above cases VBUS is referred to the external VBUS supply from an
external HOST.
I can't agree. This driver has specific dependency on dra7x SoC.
First,
vbus_irq_handler() determine USB cable state according to
"gpio_usbvid->vbus_gpio" state.
If "gpio_usbvid->vbus_gpio" value is VBUS_OFF(0), this patch set USB cable
state as 'false'
But, it include potential problems. Other extcon device using gpio would set
USB cable state
as 'true' when gpio state is 1. This way has dependency on specific SoC.
no this is not SoC specific. VBUS is referred to the external VBUS
supply from an external HOST.
so if VBUS is zero means its definitely not in connected state.
Second,
gpio_usbvid_set_initial_state() dertermine both "USB-HOST" and "USB" cable
state at same time
in 'case ID_DETCT' according to 'gpio_usbvid->id_gpio'. But, I think that other
extcon devices
would not control both "USB-HOST" and "USB" cable state at same time.
Other extcon devices would support either "USB-HOST" or "USB" cable.
The driver has 2 configurations.
1) supports implementations with both VBUS and ID pin are routed via
gpio's for swicthing roles(compatible gpio-usb-vid).
2) supports implementations with only ID pin routed via gpio for
swicthing roles(compatible gpio-usb-id).
So if you take type as VBUS_ID_DETECT then it is as what you meant.
Third,
gpio_usbvid_probe() get both gpio_usbvid->id_irq and gpio_usbvid->vbus_irq by
using DT helper function.
and gpio_usbvid_request_irq() register each interrupt handler(id_irq and
vbus_irq) according to DT data.
In result,
id_irq_handler() would control both "USB-HOST" and "USB" cable state.
only if type is ID_DETECT id_irq_handler control both USB-HOST and USB
cable states
if type is VBUS_ID_DETECT then id_irq_handler only controls USB-HOST.
vbus_irq_handler() would control only "USB" cable state.
Two interrupt handler(id_irq_handler()/vbus_irq_handler()) would control "USB"
cable state
according to each gpio state at same time. Also, It include critical problem.
No it depends on the configuration as explained above.
[snip]
+
+#define ID_GND0
+#define ID_FLOAT1
+#define VBUS_OFF0
+#define VBUS_ON1
I think you could only use two constant instead of four constant definition.
you mean only ID_GND and VBUS_OFF?
you could only define two contant (0 and 1) to detect gpio state.
okay
+
+
This blank line isn't necessary.
+static irqreturn_t id_irq_handler(int irq, void *data)
+{
+struct gpio_usbvid *gpio_usbvid = (struct gpio_usbvid *) data;
You should delete blank between ')' and 'data' as follwong:
- (struct gpio_usbvid *)data;
okay
+int id_current;
+
+id_current = gpio_get_value_cansleep(gpio_usbvid->id_gpio);
+if (id_current == ID_GND) {
+if (gpio_usbvid->type == ID_DETECT)
+extcon_set_cable_state(_usbvid->edev,
+"USB", false);
+extcon_set_cable_state(_usbvid->edev, "USB-HOST", true);
As else statement, you should set "USB-HOST" cable state to improve readability.
extcon_set_cable_state(_usbvid->edev, "USB-HOST", true);
if (gpio_usbvid->type == ID_DETECT)
extcon_set_cable_state(_usbvid->edev,
"USB", false);
Actually, USB-HOST state should be set in the id_irq handler. But in cases were
only ID pin is routed to gpio
and VBUS is not used we set the USB state too. The gpio_usbvid->type
differentiates whether its an ID only or
VBUS and ID.
I don't understand. Wht does not you change the order of function call as
following?
Re: [PATCH v3 1/3] extcon: extcon-gpio-usbvid: Generic USB VBUS/ID detection via GPIO
Hi George, On 08/29/2013 11:21 AM, George Cherian wrote: > Hi Chanwoo, > > Thanks for the review and sorry for all the trivial mistakes. > > On 8/29/2013 7:05 AM, Chanwoo Choi wrote: >> Hi George, >> >> You didn't modify this patchset about my comment on v1 patchset. >> Please pay attention to comment. >> >> On 08/29/2013 02:33 AM, George Cherian wrote: >>> Add a generic USB VBUS/ID detection EXTCON driver. This driver expects >>> the ID/VBUS pin are connected via GPIOs. This driver is tested on >>> DRA7x board were the ID pin is routed via GPIOs. The driver supports >>> both VBUS and ID pin configuration and ID pin only configuration. >>> >>> Signed-off-by: George Cherian >>> --- >>> .../bindings/extcon/extcon-gpio-usbvid.txt | 20 ++ >>> drivers/extcon/Kconfig | 6 + >>> drivers/extcon/Makefile| 1 + >>> drivers/extcon/extcon-gpio-usbvid.c| 286 >>> + >> You should keep following naming stlye. extcon-gpio-usbvid.c is wrong naming >> style. >> - extcon-[device name].c >> - extcon-gpio-usbvid.c -> extcon-dra7xx.c or etc. > Actually dra7xx is the SoC name and the USB VBUS/ID detection is not specific > to SoC. > It uses gpios to detect the VBUS/ID change. So i thought it would be better > to have generic > gpio based VBUS/ID detection rather than making dra7xx specific. Stephen > Warren had this opinion > with patch v1. Would you guarantee that this driver support all of extcon devices using gpio pin? I can't agree. This driver has specific dependency on dra7x SoC. First, vbus_irq_handler() determine USB cable state according to "gpio_usbvid->vbus_gpio" state. If "gpio_usbvid->vbus_gpio" value is VBUS_OFF(0), this patch set USB cable state as 'false' But, it include potential problems. Other extcon device using gpio would set USB cable state as 'true' when gpio state is 1. This way has dependency on specific SoC. Second, gpio_usbvid_set_initial_state() dertermine both "USB-HOST" and "USB" cable state at same time in 'case ID_DETCT' according to 'gpio_usbvid->id_gpio'. But, I think that other extcon devices would not control both "USB-HOST" and "USB" cable state at same time. Other extcon devices would support either "USB-HOST" or "USB" cable. Third, gpio_usbvid_probe() get both gpio_usbvid->id_irq and gpio_usbvid->vbus_irq by using DT helper function. and gpio_usbvid_request_irq() register each interrupt handler(id_irq and vbus_irq) according to DT data. In result, id_irq_handler() would control both "USB-HOST" and "USB" cable state. vbus_irq_handler() would control only "USB" cable state. Two interrupt handler(id_irq_handler()/vbus_irq_handler()) would control "USB" cable state according to each gpio state at same time. Also, It include critical problem. >> >> Also, you should change the file name of extcon-gpio-usbvid.txt. >> >> Finally, You used 'gpio_usbvid' prefix in extcon-gpio-usbvid.c. >> It has caused the confusion that user would think extcon-gpio-usbvid.c driver >> support all of extcon driver using gpio irq pin. So I'd like you to use >> proper prefix including device name. > I meant to support all of extcon driver using gpio for USB VBUS/ID detection. >> >>> 4 files changed, 313 insertions(+) >>> create mode 100644 >>> Documentation/devicetree/bindings/extcon/extcon-gpio-usbvid.txt >>> create mode 100644 drivers/extcon/extcon-gpio-usbvid.c > [snip] >>> index f1d54a3..8097398 100644 >>> --- a/drivers/extcon/Kconfig >>> +++ b/drivers/extcon/Kconfig >>> @@ -64,4 +64,10 @@ config EXTCON_PALMAS >>> Say Y here to enable support for USB peripheral and USB host >>> detection by palmas usb. >>> +config EXTCON_GPIO_USBVID >>> +tristate "Generic USB VBUS/ID detection using GPIO EXTCON support" >>> +help >>> + Say Y here to enable support for USB VBUS/ID deetction by GPIO. >>> + >>> + >> Remove blank line. > okay >>> endif # MULTISTATE_SWITCH > [snip] >>> diff --git a/drivers/extcon/extcon-gpio-usbvid.c >>> b/drivers/extcon/extcon-gpio-usbvid.c >>> new file mode 100644 >>> index 000..e9bc2a97 >>> --- /dev/null >>> +++ b/drivers/extcon/extcon-gpio-usbvid.c >>> @@ -0,0 +1,286 @@ >>> +/* >>> + * Generic USB VBUS-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 >>> + * >>> + * Based on extcon-palmas.c >>> + * >>> + * Author: Kishon Vijay Abraham I >>> + * >>> + * 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
Re: [PATCH v3 1/3] extcon: extcon-gpio-usbvid: Generic USB VBUS/ID detection via GPIO
Hi George, On 08/29/2013 11:21 AM, George Cherian wrote: Hi Chanwoo, Thanks for the review and sorry for all the trivial mistakes. On 8/29/2013 7:05 AM, Chanwoo Choi wrote: Hi George, You didn't modify this patchset about my comment on v1 patchset. Please pay attention to comment. On 08/29/2013 02:33 AM, George Cherian wrote: Add a generic USB VBUS/ID detection EXTCON driver. This driver expects the ID/VBUS pin are connected via GPIOs. This driver is tested on DRA7x board were the ID pin is routed via GPIOs. The driver supports both VBUS and ID pin configuration and ID pin only configuration. Signed-off-by: George Cherian george.cher...@ti.com --- .../bindings/extcon/extcon-gpio-usbvid.txt | 20 ++ drivers/extcon/Kconfig | 6 + drivers/extcon/Makefile| 1 + drivers/extcon/extcon-gpio-usbvid.c| 286 + You should keep following naming stlye. extcon-gpio-usbvid.c is wrong naming style. - extcon-[device name].c - extcon-gpio-usbvid.c - extcon-dra7xx.c or etc. Actually dra7xx is the SoC name and the USB VBUS/ID detection is not specific to SoC. It uses gpios to detect the VBUS/ID change. So i thought it would be better to have generic gpio based VBUS/ID detection rather than making dra7xx specific. Stephen Warren had this opinion with patch v1. Would you guarantee that this driver support all of extcon devices using gpio pin? I can't agree. This driver has specific dependency on dra7x SoC. First, vbus_irq_handler() determine USB cable state according to gpio_usbvid-vbus_gpio state. If gpio_usbvid-vbus_gpio value is VBUS_OFF(0), this patch set USB cable state as 'false' But, it include potential problems. Other extcon device using gpio would set USB cable state as 'true' when gpio state is 1. This way has dependency on specific SoC. Second, gpio_usbvid_set_initial_state() dertermine both USB-HOST and USB cable state at same time in 'case ID_DETCT' according to 'gpio_usbvid-id_gpio'. But, I think that other extcon devices would not control both USB-HOST and USB cable state at same time. Other extcon devices would support either USB-HOST or USB cable. Third, gpio_usbvid_probe() get both gpio_usbvid-id_irq and gpio_usbvid-vbus_irq by using DT helper function. and gpio_usbvid_request_irq() register each interrupt handler(id_irq and vbus_irq) according to DT data. In result, id_irq_handler() would control both USB-HOST and USB cable state. vbus_irq_handler() would control only USB cable state. Two interrupt handler(id_irq_handler()/vbus_irq_handler()) would control USB cable state according to each gpio state at same time. Also, It include critical problem. Also, you should change the file name of extcon-gpio-usbvid.txt. Finally, You used 'gpio_usbvid' prefix in extcon-gpio-usbvid.c. It has caused the confusion that user would think extcon-gpio-usbvid.c driver support all of extcon driver using gpio irq pin. So I'd like you to use proper prefix including device name. I meant to support all of extcon driver using gpio for USB VBUS/ID detection. 4 files changed, 313 insertions(+) create mode 100644 Documentation/devicetree/bindings/extcon/extcon-gpio-usbvid.txt create mode 100644 drivers/extcon/extcon-gpio-usbvid.c [snip] index f1d54a3..8097398 100644 --- a/drivers/extcon/Kconfig +++ b/drivers/extcon/Kconfig @@ -64,4 +64,10 @@ config EXTCON_PALMAS Say Y here to enable support for USB peripheral and USB host detection by palmas usb. +config EXTCON_GPIO_USBVID +tristate Generic USB VBUS/ID detection using GPIO EXTCON support +help + Say Y here to enable support for USB VBUS/ID deetction by GPIO. + + Remove blank line. okay endif # MULTISTATE_SWITCH [snip] diff --git a/drivers/extcon/extcon-gpio-usbvid.c b/drivers/extcon/extcon-gpio-usbvid.c new file mode 100644 index 000..e9bc2a97 --- /dev/null +++ b/drivers/extcon/extcon-gpio-usbvid.c @@ -0,0 +1,286 @@ +/* + * Generic USB VBUS-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.cher...@ti.com + * + * Based on extcon-palmas.c + * + * Author: Kishon Vijay Abraham I kis...@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
Re: [PATCH v3 1/3] extcon: extcon-gpio-usbvid: Generic USB VBUS/ID detection via GPIO
Hi Chanwoo,
On 8/29/2013 11:53 AM, Chanwoo Choi wrote:
[snip]
You should keep following naming stlye. extcon-gpio-usbvid.c is wrong naming
style.
- extcon-[device name].c
- extcon-gpio-usbvid.c - extcon-dra7xx.c or etc.
Actually dra7xx is the SoC name and the USB VBUS/ID detection is not specific
to SoC.
It uses gpios to detect the VBUS/ID change. So i thought it would be better to
have generic
gpio based VBUS/ID detection rather than making dra7xx specific. Stephen Warren
had this opinion
with patch v1.
Would you guarantee that this driver support all of extcon devices using gpio
pin?
This driver would guarantee extcon devices using gpio pins for USB VBUS
and ID detection.
Following is the basic assumption made, correct me if I am wrong.
ID pin ground means - USB-HOST is true (this happens when a device is
connected to USB port and we act as HOST )
ID pin Float means - USB-HOST is false (this happens when nothing is
connected or when we act as a peripheral under a HOST)
VBUS ON means - USB is true (this happens when we are connected under a
HOST as a peripheral)
VBUS OFF means - USB is false ( this happens when we are either
disconnected from a HOST or when we are in HOST mode).
So normally USB is in peripheral mode is enabled when ID pin is float
and VBUS is ON.
and USB is in HOST mode when ID pin is ground and VBUS is OFF.
In all above cases VBUS is referred to the external VBUS supply from an
external HOST.
I can't agree. This driver has specific dependency on dra7x SoC.
First,
vbus_irq_handler() determine USB cable state according to
gpio_usbvid-vbus_gpio state.
If gpio_usbvid-vbus_gpio value is VBUS_OFF(0), this patch set USB cable
state as 'false'
But, it include potential problems. Other extcon device using gpio would set
USB cable state
as 'true' when gpio state is 1. This way has dependency on specific SoC.
no this is not SoC specific. VBUS is referred to the external VBUS
supply from an external HOST.
so if VBUS is zero means its definitely not in connected state.
Second,
gpio_usbvid_set_initial_state() dertermine both USB-HOST and USB cable
state at same time
in 'case ID_DETCT' according to 'gpio_usbvid-id_gpio'. But, I think that other
extcon devices
would not control both USB-HOST and USB cable state at same time.
Other extcon devices would support either USB-HOST or USB cable.
The driver has 2 configurations.
1) supports implementations with both VBUS and ID pin are routed via
gpio's for swicthing roles(compatible gpio-usb-vid).
2) supports implementations with only ID pin routed via gpio for
swicthing roles(compatible gpio-usb-id).
So if you take type as VBUS_ID_DETECT then it is as what you meant.
Third,
gpio_usbvid_probe() get both gpio_usbvid-id_irq and gpio_usbvid-vbus_irq by
using DT helper function.
and gpio_usbvid_request_irq() register each interrupt handler(id_irq and
vbus_irq) according to DT data.
In result,
id_irq_handler() would control both USB-HOST and USB cable state.
only if type is ID_DETECT id_irq_handler control both USB-HOST and USB
cable states
if type is VBUS_ID_DETECT then id_irq_handler only controls USB-HOST.
vbus_irq_handler() would control only USB cable state.
Two interrupt handler(id_irq_handler()/vbus_irq_handler()) would control USB
cable state
according to each gpio state at same time. Also, It include critical problem.
No it depends on the configuration as explained above.
[snip]
+
+#define ID_GND0
+#define ID_FLOAT1
+#define VBUS_OFF0
+#define VBUS_ON1
I think you could only use two constant instead of four constant definition.
you mean only ID_GND and VBUS_OFF?
you could only define two contant (0 and 1) to detect gpio state.
okay
+
+
This blank line isn't necessary.
+static irqreturn_t id_irq_handler(int irq, void *data)
+{
+struct gpio_usbvid *gpio_usbvid = (struct gpio_usbvid *) data;
You should delete blank between ')' and 'data' as follwong:
- (struct gpio_usbvid *)data;
okay
+int id_current;
+
+id_current = gpio_get_value_cansleep(gpio_usbvid-id_gpio);
+if (id_current == ID_GND) {
+if (gpio_usbvid-type == ID_DETECT)
+extcon_set_cable_state(gpio_usbvid-edev,
+USB, false);
+extcon_set_cable_state(gpio_usbvid-edev, USB-HOST, true);
As else statement, you should set USB-HOST cable state to improve readability.
extcon_set_cable_state(gpio_usbvid-edev, USB-HOST, true);
if (gpio_usbvid-type == ID_DETECT)
extcon_set_cable_state(gpio_usbvid-edev,
USB, false);
Actually, USB-HOST state should be set in the id_irq handler. But in cases were
only ID pin is routed to gpio
and VBUS is not used we set the USB state too. The gpio_usbvid-type
differentiates whether its an ID only or
VBUS and ID.
I don't understand. Wht does not you change the order of function call as
following?
Before:
if
Re: [PATCH v3 1/3] extcon: extcon-gpio-usbvid: Generic USB VBUS/ID detection via GPIO
On 08/29/2013 04:30 PM, George Cherian wrote: Hi Chanwoo, On 8/29/2013 11:53 AM, Chanwoo Choi wrote: [snip] You should keep following naming stlye. extcon-gpio-usbvid.c is wrong naming style. - extcon-[device name].c - extcon-gpio-usbvid.c - extcon-dra7xx.c or etc. Actually dra7xx is the SoC name and the USB VBUS/ID detection is not specific to SoC. It uses gpios to detect the VBUS/ID change. So i thought it would be better to have generic gpio based VBUS/ID detection rather than making dra7xx specific. Stephen Warren had this opinion with patch v1. Would you guarantee that this driver support all of extcon devices using gpio pin? This driver would guarantee extcon devices using gpio pins for USB VBUS and ID detection. Following is the basic assumption made, correct me if I am wrong. ID pin ground means - USB-HOST is true (this happens when a device is connected to USB port and we act as HOST ) ID pin Float means - USB-HOST is false (this happens when nothing is connected or when we act as a peripheral under a HOST) VBUS ON means - USB is true (this happens when we are connected under a HOST as a peripheral) VBUS OFF means - USB is false ( this happens when we are either disconnected from a HOST or when we are in HOST mode). So normally USB is in peripheral mode is enabled when ID pin is float and VBUS is ON. and USB is in HOST mode when ID pin is ground and VBUS is OFF. In all above cases VBUS is referred to the external VBUS supply from an external HOST. I can't agree. This driver has specific dependency on dra7x SoC. First, vbus_irq_handler() determine USB cable state according to gpio_usbvid-vbus_gpio state. If gpio_usbvid-vbus_gpio value is VBUS_OFF(0), this patch set USB cable state as 'false' But, it include potential problems. Other extcon device using gpio would set USB cable state as 'true' when gpio state is 1. This way has dependency on specific SoC. no this is not SoC specific. VBUS is referred to the external VBUS supply from an external HOST. so if VBUS is zero means its definitely not in connected state. I tested various development board based on Samsung Exynos series SoC. Although some gpio of Exynos series SoC set high state(non zero, 1) as default value, this gpio state could mean off state, disconnected or un-powered state according to gpio. Of course, above explanation about specific gpio don't include all gpios. This is meaning that there is possibility. Second, gpio_usbvid_set_initial_state() dertermine both USB-HOST and USB cable state at same time in 'case ID_DETCT' according to 'gpio_usbvid-id_gpio'. But, I think that other extcon devices would not control both USB-HOST and USB cable state at same time. Other extcon devices would support either USB-HOST or USB cable. The driver has 2 configurations. 1) supports implementations with both VBUS and ID pin are routed via gpio's for swicthing roles(compatible gpio-usb-vid). As you explained about case 1, it is only used on dra7x SoC. Other SoC could not wish to control both USB-HOST and USB cable at same time. 2) supports implementations with only ID pin routed via gpio for swicthing roles(compatible gpio-usb-id). So if you take type as VBUS_ID_DETECT then it is as what you meant. 2) case don't support the detection of USB-HOST cable. Only detect USB cable according to vbus_gpio value. If user wish to detect only USB-HOST cable, should user enter ID_DETECT as ti,gpio-usb-id on DT file? But, id_irq_handler() control both USB-HOST and USB cable at same time. It may not prefer this method. Third, gpio_usbvid_probe() get both gpio_usbvid-id_irq and gpio_usbvid-vbus_irq by using DT helper function. and gpio_usbvid_request_irq() register each interrupt handler(id_irq and vbus_irq) according to DT data. In result, id_irq_handler() would control both USB-HOST and USB cable state. only if type is ID_DETECT id_irq_handler control both USB-HOST and USB cable states if type is VBUS_ID_DETECT then id_irq_handler only controls USB-HOST. I have some confusion. I need additional your explanation. Could this driver register only one interrupt handler either id_irq_handler() or vbus_irq_handler()? or Could this driver register two interrupt handler both id_irq_handler() or vbus_irq_handler()? vbus_irq_handler() would control only USB cable state. Two interrupt handler(id_irq_handler()/vbus_irq_handler()) would control USB cable state according to each gpio state at same time. Also, It include critical problem. No it depends on the configuration as explained above. [snip] + +#define ID_GND0 +#define ID_FLOAT1 +#define VBUS_OFF0 +#define VBUS_ON1 I think you could only use two constant instead of four constant definition. you mean only ID_GND and VBUS_OFF? you could only define two contant (0 and 1) to detect gpio state. okay + + This blank line isn't necessary. +static
Re: [PATCH v3 1/3] extcon: extcon-gpio-usbvid: Generic USB VBUS/ID detection via GPIO
On 8/29/2013 4:07 PM, Chanwoo Choi wrote: On 08/29/2013 04:30 PM, George Cherian wrote: Hi Chanwoo, On 8/29/2013 11:53 AM, Chanwoo Choi wrote: [snip] You should keep following naming stlye. extcon-gpio-usbvid.c is wrong naming style. - extcon-[device name].c - extcon-gpio-usbvid.c - extcon-dra7xx.c or etc. Actually dra7xx is the SoC name and the USB VBUS/ID detection is not specific to SoC. It uses gpios to detect the VBUS/ID change. So i thought it would be better to have generic gpio based VBUS/ID detection rather than making dra7xx specific. Stephen Warren had this opinion with patch v1. Would you guarantee that this driver support all of extcon devices using gpio pin? This driver would guarantee extcon devices using gpio pins for USB VBUS and ID detection. Following is the basic assumption made, correct me if I am wrong. ID pin ground means - USB-HOST is true (this happens when a device is connected to USB port and we act as HOST ) ID pin Float means - USB-HOST is false (this happens when nothing is connected or when we act as a peripheral under a HOST) VBUS ON means - USB is true (this happens when we are connected under a HOST as a peripheral) VBUS OFF means - USB is false ( this happens when we are either disconnected from a HOST or when we are in HOST mode). So normally USB is in peripheral mode is enabled when ID pin is float and VBUS is ON. and USB is in HOST mode when ID pin is ground and VBUS is OFF. In all above cases VBUS is referred to the external VBUS supply from an external HOST. I can't agree. This driver has specific dependency on dra7x SoC. First, vbus_irq_handler() determine USB cable state according to gpio_usbvid-vbus_gpio state. If gpio_usbvid-vbus_gpio value is VBUS_OFF(0), this patch set USB cable state as 'false' But, it include potential problems. Other extcon device using gpio would set USB cable state as 'true' when gpio state is 1. This way has dependency on specific SoC. no this is not SoC specific. VBUS is referred to the external VBUS supply from an external HOST. so if VBUS is zero means its definitely not in connected state. I tested various development board based on Samsung Exynos series SoC. Although some gpio of Exynos series SoC set high state(non zero, 1) as default value, this gpio state could mean off state, disconnected or un-powered state according to gpio. Of course, above explanation about specific gpio don't include all gpios. This is meaning that there is possibility. okay then how about adding a flag for inverted logic too. something like this if(of_property_read_bool(np,inverted_gpio)) gpio_usbvid-gpio_inv = 1; And always check on this before deciding? Second, gpio_usbvid_set_initial_state() dertermine both USB-HOST and USB cable state at same time in 'case ID_DETCT' according to 'gpio_usbvid-id_gpio'. But, I think that other extcon devices would not control both USB-HOST and USB cable state at same time. Other extcon devices would support either USB-HOST or USB cable. The driver has 2 configurations. 1) supports implementations with both VBUS and ID pin are routed via gpio's for swicthing roles(compatible gpio-usb-vid). As you explained about case 1, it is only used on dra7x SoC. No gpio-usb-id is used in dra7xx. dra7xx has got only ID pin routed via gpio. Other SoC could not wish to control both USB-HOST and USB cable at same time. 2) supports implementations with only ID pin routed via gpio for swicthing roles(compatible gpio-usb-id). So if you take type as VBUS_ID_DETECT then it is as what you meant. I think i didnt explain it properly last time. In perfect world you will have both VBUS and ID pin routed via gpios for eg: VBUS via gpio2 and ID via gpio3. if this is the case then we have to use compatible gpio-usb-vid where in 2 gpios will be used 2 irq handlers will be installed and it will control both USB-HOST and USB cables. In this case its possible to have 3 states USB-HOST (true), USB(true) and both false. Now in case of dra7xx the board designers chose not to route the VBUS to gpio and only ID pin is routed. But still we need to differentiate USB-HOST and USB cables In such cases we use compatible gpio-usb-id where in 1 gpio will be used 1 irq handler is installed and it will control both USB-HOST and USB cables. In this case its possible to have only 2 states USB-HOST (true) or USB(true). Now there could be a 3rd scenario were in only VBUS is routed via gpio, that we would not support since we cant assume the value of ID pin and configure the UTMI is not proper. So I have mentioned even in documentation that ID pin is mandatory. We can always assume role depending on ID pin. 2) case don't support the detection of USB-HOST cable. Only detect USB cable according to vbus_gpio value. If user wish to detect only USB-HOST cable, should user enter ID_DETECT as ti,gpio-usb-id on DT file? But, id_irq_handler() control both USB-HOST and USB cable at same time. It may not
Re: [PATCH v3 1/3] extcon: extcon-gpio-usbvid: Generic USB VBUS/ID detection via GPIO
On 08/29/2013 08:48 PM, George Cherian wrote: On 8/29/2013 4:07 PM, Chanwoo Choi wrote: On 08/29/2013 04:30 PM, George Cherian wrote: Hi Chanwoo, On 8/29/2013 11:53 AM, Chanwoo Choi wrote: [snip] You should keep following naming stlye. extcon-gpio-usbvid.c is wrong naming style. - extcon-[device name].c - extcon-gpio-usbvid.c - extcon-dra7xx.c or etc. Actually dra7xx is the SoC name and the USB VBUS/ID detection is not specific to SoC. It uses gpios to detect the VBUS/ID change. So i thought it would be better to have generic gpio based VBUS/ID detection rather than making dra7xx specific. Stephen Warren had this opinion with patch v1. Would you guarantee that this driver support all of extcon devices using gpio pin? This driver would guarantee extcon devices using gpio pins for USB VBUS and ID detection. Following is the basic assumption made, correct me if I am wrong. ID pin ground means - USB-HOST is true (this happens when a device is connected to USB port and we act as HOST ) ID pin Float means - USB-HOST is false (this happens when nothing is connected or when we act as a peripheral under a HOST) VBUS ON means - USB is true (this happens when we are connected under a HOST as a peripheral) VBUS OFF means - USB is false ( this happens when we are either disconnected from a HOST or when we are in HOST mode). So normally USB is in peripheral mode is enabled when ID pin is float and VBUS is ON. and USB is in HOST mode when ID pin is ground and VBUS is OFF. In all above cases VBUS is referred to the external VBUS supply from an external HOST. I can't agree. This driver has specific dependency on dra7x SoC. First, vbus_irq_handler() determine USB cable state according to gpio_usbvid-vbus_gpio state. If gpio_usbvid-vbus_gpio value is VBUS_OFF(0), this patch set USB cable state as 'false' But, it include potential problems. Other extcon device using gpio would set USB cable state as 'true' when gpio state is 1. This way has dependency on specific SoC. no this is not SoC specific. VBUS is referred to the external VBUS supply from an external HOST. so if VBUS is zero means its definitely not in connected state. I tested various development board based on Samsung Exynos series SoC. Although some gpio of Exynos series SoC set high state(non zero, 1) as default value, this gpio state could mean off state, disconnected or un-powered state according to gpio. Of course, above explanation about specific gpio don't include all gpios. This is meaning that there is possibility. okay then how about adding a flag for inverted logic too. something like this if(of_property_read_bool(np,inverted_gpio)) gpio_usbvid-gpio_inv = 1; And always check on this before deciding? Second, gpio_usbvid_set_initial_state() dertermine both USB-HOST and USB cable state at same time in 'case ID_DETCT' according to 'gpio_usbvid-id_gpio'. But, I think that other extcon devices would not control both USB-HOST and USB cable state at same time. Other extcon devices would support either USB-HOST or USB cable. The driver has 2 configurations. 1) supports implementations with both VBUS and ID pin are routed via gpio's for swicthing roles(compatible gpio-usb-vid). As you explained about case 1, it is only used on dra7x SoC. No gpio-usb-id is used in dra7xx. dra7xx has got only ID pin routed via gpio. Other SoC could not wish to control both USB-HOST and USB cable at same time. I need your answer about above my opinion for other SoC. 2) supports implementations with only ID pin routed via gpio for swicthing roles(compatible gpio-usb-id). So if you take type as VBUS_ID_DETECT then it is as what you meant. I think i didnt explain it properly last time. In perfect world you will have both VBUS and ID pin routed via gpios for eg: VBUS via gpio2 and ID via gpio3. if this is the case then we have to use compatible gpio-usb-vid where in 2 gpios will be used 2 irq handlers will be installed and it will control both USB-HOST and USB cables. In this case its possible to have 3 states USB-HOST (true), USB(true) and both false. Now in case of dra7xx the board designers chose not to route the VBUS to gpio and only ID pin is routed. But still we need to differentiate USB-HOST and USB cables In such cases we use compatible gpio-usb-id where in 1 gpio will be used 1 irq handler is installed and it will control both USB-HOST and USB cables. In this case its possible to have only 2 states USB-HOST (true) or USB(true). Now there could be a 3rd scenario were in only VBUS is routed via gpio, that we would not support since we cant assume the value of ID pin and configure the UTMI is not proper. So I have mentioned even in documentation that ID pin is mandatory. We can always assume role depending on ID pin. 2) case don't support the detection of USB-HOST cable. Only detect USB cable according to vbus_gpio value. If user
Re: [PATCH v3 1/3] extcon: extcon-gpio-usbvid: Generic USB VBUS/ID detection via GPIO
Hi Chanwoo,
On 8/29/2013 5:42 PM, Chanwoo Choi wrote:
[big snip ]
I tested various development board based on Samsung Exynos series SoC.
Although some gpio of Exynos series SoC set high state(non zero, 1) as default
value,
this gpio state could mean off state, disconnected or un-powered state
according to gpio.
Of course, above explanation about specific gpio don't include all gpios.
This is meaning that there is possibility.
okay then how about adding a flag for inverted logic too. something like this
if(of_property_read_bool(np,inverted_gpio))
gpio_usbvid-gpio_inv = 1;
And always check on this before deciding?
Is this fine ?
Second,
gpio_usbvid_set_initial_state() dertermine both USB-HOST and USB cable
state at same time
in 'case ID_DETCT' according to 'gpio_usbvid-id_gpio'. But, I think that other
extcon devices
would not control both USB-HOST and USB cable state at same time.
Other extcon devices would support either USB-HOST or USB cable.
The driver has 2 configurations.
1) supports implementations with both VBUS and ID pin are routed via gpio's for
swicthing roles(compatible gpio-usb-vid).
As you explained about case 1, it is only used on dra7x SoC.
No gpio-usb-id is used in dra7xx. dra7xx has got only ID pin routed via gpio.
Other SoC could not wish to control both USB-HOST and USB cable at same
time.
I could'nt actually parse this. You meant since the id_irq_handler
handles both USB and USB-HOST cable
its not proper?
I need your answer about above my opinion for other SoC.
So how about this, I have removed the dra7x specific stuffs (gpio-usb-id)
static void gpio_usbvid_set_initial_state(struct gpio_usbvid *gpio_usbvid)
{
int id_current, vbus_current;
id_current = gpio_get_value_cansleep(gpio_usbvid-id_gpio);
if (!!id_current == ID_FLOAT)
extcon_set_cable_state(gpio_usbvid-edev, USB-HOST, false);
else
extcon_set_cable_state(gpio_usbvid-edev, USB-HOST, true);
vbus_current = gpio_get_value_cansleep(gpio_usbvid-vbus_gpio);
if (!!vbus_current == VBUS_ON)
extcon_set_cable_state(gpio_usbvid-edev, USB, true);
else
extcon_set_cable_state(gpio_usbvid-edev, USB, false);
}
and the irq handlers like this
static irqreturn_t id_irq_handler(int irq, void *data)
{
struct gpio_usbvid *gpio_usbvid = (struct gpio_usbvid *)data;
int id_current;
id_current = gpio_get_value_cansleep(gpio_usbvid-id_gpio);
if (id_current == ID_GND)
extcon_set_cable_state(gpio_usbvid-edev, USB-HOST,
true);
else
extcon_set_cable_state(gpio_usbvid-edev, USB-HOST,
false);
return IRQ_HANDLED;
}
static irqreturn_t vbus_irq_handler(int irq, void *data)
{
struct gpio_usbvid *gpio_usbvid = (struct gpio_usbvid *)data;
int vbus_current;
vbus_current = gpio_get_value_cansleep(gpio_usbvid-vbus_gpio);
if (vbus_current == VBUS_OFF)
extcon_set_cable_state(gpio_usbvid-edev, USB, false);
else
extcon_set_cable_state(gpio_usbvid-edev, USB, true);
return IRQ_HANDLED;
}
[snip]
I have some confusion. I need additional your explanation.
Could this driver register only one interrupt handler either id_irq_handler()
or vbus_irq_handler()?
If compatible == ID_DETECT, only one handler -- id_irq_handler() and it will
handle both USB and USB-HOST
or
Could this driver register two interrupt handler both id_irq_handler() or
vbus_irq_handler()?
If compatible == VBUS_ID_DETECT, 2 handlers -- id_irq_handler() will handle
USB-HOST and vbus_irq_handler will handle USB.
As you mentioned, we cannot only control either USB or USB-HOST cable on this
extcon driver.
This extcon driver is only suitable dra7x SoC.
Do you still feel its dra7x specific if i modify it as above?
Because id_irq_handler() control both USB-HOST and USB cable at same time,
you need this setting order between USB-HOST and USB cable.
yes
I think that the setting order between cables isn't general. It is specific
method for dra7x SoC.
So if i remove that part then?
Did you think that SoC should always connect either USB-HOST cable or USB
cable?
No, even if a physical cable is not connected it should default to either
USB-HOST or USB
It isn't general.
If physical cable isn't connected to extcon device, the state both USB-HOST and
USB cable
should certainly be zero. Because The extcon consumer driver could set proper
operation
according to cable state.
okay
I don't know this case except for dra7x SoC. So, I think it has dependency on
specific SoC.
I need your answer about above my opinion.
Hope i could answer you :-)
and can't agree as generic extcon gpio driver.
--
-George
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at
Re: [PATCH v3 1/3] extcon: extcon-gpio-usbvid: Generic USB VBUS/ID detection via GPIO
On 08/29/2013 05:48 AM, George Cherian wrote: On 8/29/2013 4:07 PM, Chanwoo Choi wrote: ... I tested various development board based on Samsung Exynos series SoC. Although some gpio of Exynos series SoC set high state(non zero, 1) as default value, this gpio state could mean off state, disconnected or un-powered state according to gpio. Of course, above explanation about specific gpio don't include all gpios. This is meaning that there is possibility. okay then how about adding a flag for inverted logic too. something like this if(of_property_read_bool(np,inverted_gpio)) gpio_usbvid-gpio_inv = 1; And always check on this before deciding? Don't do this. GPIO specifiers in DT typically include a flags cell that describes certain GPIO properties. One of those properties is signal inversion. You can use of_get_named_gpio_flags() to retrieve those flags. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 1/3] extcon: extcon-gpio-usbvid: Generic USB VBUS/ID detection via GPIO
On 08/28/2013 11:33 AM, George Cherian wrote:
Add a generic USB VBUS/ID detection EXTCON driver. This driver expects
the ID/VBUS pin are connected via GPIOs. This driver is tested on
DRA7x board were the ID pin is routed via GPIOs. The driver supports
both VBUS and ID pin configuration and ID pin only configuration.
diff --git a/Documentation/devicetree/bindings/extcon/extcon-gpio-usbvid.txt
b/Documentation/devicetree/bindings/extcon/extcon-gpio-usbvid.txt
+EXTCON FOR USB VIA GPIO
Please write a brief explanation of the HW that this binding represents.
Extcon is a Linux-specific term. Binding documentation should be
written in an OS-agnostic manner. Bindings should not reference
OS-specific terminology, and should be a pure description of the HW.
+Required Properties:
+ - compatible : Should be ti,gpio-usb-vid for USB VBUS-ID detector
+ using gpios or ti,gpio-usb-id for USB ID pin detector
Those souond like two rather different things. Should they be separate
bindings, or at the least, separate sections in the document?
If this binding is meant to represent some generic hardware, the vendor
prefix probably isn't required. So, remove ti, from the compatible values.
+ - gpios : phandle and args ID pin gpio and VBUS gpio.
s/and args/and specifier/.
+The first gpio used for ID pin detection
+and the second used for VBUS detection.
+ID pin gpio is mandatory and VBUS is optional
+depending on implementation.
It'd be better to use named GPIO properties here, rather than having
multiple different kinds of GPIO in the same property. How about
id-gpios and vbus-gpios as the property names?
The semantics of each property should be specified. Are these input
GPIOs that reflect the ID and VBUS state? Do they directly represent the
signal state on the connector, or are they processed somehow? Does the
VBUS GPIO control the board's VBUS output, or is it an input? If it
controls VBUS output, it probably should be represented as a regulator
rather than a GPIO.
I think the following might work:
Required properties:
id-gpios: GPIO specifier for the connector's ID pin input. This directly
represents the value present on the ID pin at the connector.
Optional properties:
vbus-gpios: GPIO specifier for the connector's VBUS signal. This
directly represents whether VBUS being driven by the USB cable itself.
(although perhaps that isn't an optional property, but rather a required
property of the second compatible value?)
+Please refer to ../gpio/gpio.txt for details of the common GPIO bindings
+
+Example:
+
+ gpio_usbvid_extcon1 {
+ compatible = ti,gpio-usb-vid;
+ gpios = gpio1 1 0,
+ gpio2 2 0;
+ };
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 1/3] extcon: extcon-gpio-usbvid: Generic USB VBUS/ID detection via GPIO
Hi George,
On 08/29/2013 10:45 PM, George Cherian wrote:
Hi Chanwoo,
On 8/29/2013 5:42 PM, Chanwoo Choi wrote:
[big snip ]
I tested various development board based on Samsung Exynos series SoC.
Although some gpio of Exynos series SoC set high state(non zero, 1) as
default value,
this gpio state could mean off state, disconnected or un-powered state
according to gpio.
Of course, above explanation about specific gpio don't include all gpios.
This is meaning that there is possibility.
okay then how about adding a flag for inverted logic too. something like
this
if(of_property_read_bool(np,inverted_gpio))
gpio_usbvid-gpio_inv = 1;
And always check on this before deciding?
Is this fine ?
OK.
But, as Stephen commented on other mail, you should use proper DT helper
function.
Second,
gpio_usbvid_set_initial_state() dertermine both USB-HOST and USB
cable state at same time
in 'case ID_DETCT' according to 'gpio_usbvid-id_gpio'. But, I think
that other extcon devices
would not control both USB-HOST and USB cable state at same time.
Other extcon devices would support either USB-HOST or USB cable.
The driver has 2 configurations.
1) supports implementations with both VBUS and ID pin are routed via
gpio's for swicthing roles(compatible gpio-usb-vid).
As you explained about case 1, it is only used on dra7x SoC.
No gpio-usb-id is used in dra7xx. dra7xx has got only ID pin routed via
gpio.
Other SoC could not wish to control both USB-HOST and USB cable at
same time.
I could'nt actually parse this. You meant since the id_irq_handler handles
both USB and USB-HOST cable
its not proper?
It's not proper in general case. The generic driver must guarantee all of
extcon device using gpio.
As far as I know, the generic driver cannot direclty control gpio pin and get
value from gpio pin.
Almost device driver including in kernel/drivers control gpio pin on specific
device driver
instead of generic driver.
I need your answer about above my opinion for other SoC.
So how about this, I have removed the dra7x specific stuffs (gpio-usb-id)
static void gpio_usbvid_set_initial_state(struct gpio_usbvid *gpio_usbvid)
{
int id_current, vbus_current;
id_current = gpio_get_value_cansleep(gpio_usbvid-id_gpio);
if (!!id_current == ID_FLOAT)
extcon_set_cable_state(gpio_usbvid-edev, USB-HOST, false);
else
extcon_set_cable_state(gpio_usbvid-edev, USB-HOST, true);
vbus_current = gpio_get_value_cansleep(gpio_usbvid-vbus_gpio);
if (!!vbus_current == VBUS_ON)
extcon_set_cable_state(gpio_usbvid-edev, USB, true);
else
extcon_set_cable_state(gpio_usbvid-edev, USB, false);
}
and the irq handlers like this
static irqreturn_t id_irq_handler(int irq, void *data)
{
struct gpio_usbvid *gpio_usbvid = (struct gpio_usbvid *)data;
int id_current;
id_current = gpio_get_value_cansleep(gpio_usbvid-id_gpio);
if (id_current == ID_GND)
extcon_set_cable_state(gpio_usbvid-edev, USB-HOST, true);
else
extcon_set_cable_state(gpio_usbvid-edev, USB-HOST, false);
return IRQ_HANDLED;
}
static irqreturn_t vbus_irq_handler(int irq, void *data)
{
struct gpio_usbvid *gpio_usbvid = (struct gpio_usbvid *)data;
int vbus_current;
vbus_current = gpio_get_value_cansleep(gpio_usbvid-vbus_gpio);
if (vbus_current == VBUS_OFF)
extcon_set_cable_state(gpio_usbvid-edev, USB, false);
else
extcon_set_cable_state(gpio_usbvid-edev, USB, true);
return IRQ_HANDLED;
}
I know your intention dividing interrupt handler for each cable.
But I don't think this driver must guarantee all of extcon device using gpio.
For example,
below three SoC wish to detect USB/USB-HOST cable with each different methods.
SoC A wish to detect USB/USB-HOST cable through only one gpio pin.
SoC B wish to detect USB/USB-HOST cable through ADC value instead of gpio pin.
SoC C wish to detect USB/USB-HOST cable through two gpio pin because USB
connected on gpio an USB-HOST connected on another.
In addition,
if SoC A/C wish to write some data to own specific registers for proper
opeating,
Could we write some data to register on generic driver?
Finally,
SoC D wish to detect USB/USB-HOST/JIG cable through two gpio pin
- one gpio may detect either USB or USB-HOST and another may detect JIG cable
or one gpio may detect either USb or JIG and another may detect USB-HOST cable.
That way, there are many cases we cannot guarantee all of extcon devices.
I think this driver could support dra7x series SoC but as I mentioned,
this driver may not guarantee all of cases.
[snip]
I have some confusion. I need additional your explanation.
Could this driver register only one interrupt handler either
id_irq_handler() or vbus_irq_handler()?
If compatible == ID_DETECT, only one handler --
Re: [PATCH v3 1/3] extcon: extcon-gpio-usbvid: Generic USB VBUS/ID detection via GPIO
Hi Chanwoo,
Thanks for the review and sorry for all the trivial mistakes.
On 8/29/2013 7:05 AM, Chanwoo Choi wrote:
Hi George,
You didn't modify this patchset about my comment on v1 patchset.
Please pay attention to comment.
On 08/29/2013 02:33 AM, George Cherian wrote:
Add a generic USB VBUS/ID detection EXTCON driver. This driver expects
the ID/VBUS pin are connected via GPIOs. This driver is tested on
DRA7x board were the ID pin is routed via GPIOs. The driver supports
both VBUS and ID pin configuration and ID pin only configuration.
Signed-off-by: George Cherian
---
.../bindings/extcon/extcon-gpio-usbvid.txt | 20 ++
drivers/extcon/Kconfig | 6 +
drivers/extcon/Makefile| 1 +
drivers/extcon/extcon-gpio-usbvid.c| 286 +
You should keep following naming stlye. extcon-gpio-usbvid.c is wrong naming
style.
- extcon-[device name].c
- extcon-gpio-usbvid.c -> extcon-dra7xx.c or etc.
Actually dra7xx is the SoC name and the USB VBUS/ID detection is not
specific to SoC.
It uses gpios to detect the VBUS/ID change. So i thought it would be
better to have generic
gpio based VBUS/ID detection rather than making dra7xx specific. Stephen
Warren had this opinion
with patch v1.
Also, you should change the file name of extcon-gpio-usbvid.txt.
Finally, You used 'gpio_usbvid' prefix in extcon-gpio-usbvid.c.
It has caused the confusion that user would think extcon-gpio-usbvid.c driver
support all of extcon driver using gpio irq pin. So I'd like you to use
proper prefix including device name.
I meant to support all of extcon driver using gpio for USB VBUS/ID
detection.
4 files changed, 313 insertions(+)
create mode 100644
Documentation/devicetree/bindings/extcon/extcon-gpio-usbvid.txt
create mode 100644 drivers/extcon/extcon-gpio-usbvid.c
[snip]
index f1d54a3..8097398 100644
--- a/drivers/extcon/Kconfig
+++ b/drivers/extcon/Kconfig
@@ -64,4 +64,10 @@ config EXTCON_PALMAS
Say Y here to enable support for USB peripheral and USB host
detection by palmas usb.
+config EXTCON_GPIO_USBVID
+ tristate "Generic USB VBUS/ID detection using GPIO EXTCON support"
+ help
+ Say Y here to enable support for USB VBUS/ID deetction by GPIO.
+
+
Remove blank line.
okay
endif # MULTISTATE_SWITCH
[snip]
diff --git a/drivers/extcon/extcon-gpio-usbvid.c
b/drivers/extcon/extcon-gpio-usbvid.c
new file mode 100644
index 000..e9bc2a97
--- /dev/null
+++ b/drivers/extcon/extcon-gpio-usbvid.c
@@ -0,0 +1,286 @@
+/*
+ * Generic USB VBUS-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
+ *
+ * Based on extcon-palmas.c
+ *
+ * Author: Kishon Vijay Abraham I
+ *
+ * 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
+#include
+#include
+#include
kthead.h, freezer.h headerfile is used in this file?
okay
+#include
+#include
+#include
+#include
+#include
+#include
+#include
Order headerfile alphabetically.
okay
+
+struct gpio_usbvid {
+ struct device *dev;
+
+ struct extcon_dev edev;
+
+ /*GPIO pin */
I commented previous your patch about this wrong coding style.
Why did not you fix this coding style?
okay
+ int id_gpio;
+ int vbus_gpio;
+
+ int id_irq;
+ int vbus_irq;
+ int type;
+};
+
+static const char *dra7xx_extcon_cable[] = {
+ [0] = "USB",
+ [1] = "USB-HOST",
+ NULL,
+};
+
+static const int mutually_exclusive[] = {0x3, 0x0};
+
+/* Two types of support are provided.
+ * Systems which has
+ * 1) VBUS and ID pin connected via GPIO
+ * 2) only ID pin connected via GPIO
Remove blank between '2)' and 'only'.
okay
+ * For Case 1 both the gpios should be provided via DT
+ * Always the first GPIO in dt is considered ID pin GPIO
+ */
+
+enum {
+ UNKNOWN = 0,
+ ID_DETECT,
+ VBUS_ID_DETECT,
+};
+
+#define ID_GND 0
+#define ID_FLOAT 1
+#define VBUS_OFF 0
+#define VBUS_ON1
I think you could only use two constant instead of four constant definition.
you mean only ID_GND and VBUS_OFF?
+
+
This blank line isn't necessary.
+static irqreturn_t id_irq_handler(int irq, void *data)
+{
+ struct gpio_usbvid *gpio_usbvid = (struct gpio_usbvid *) data;
You should delete blank between ')' and 'data' as follwong:
- (struct gpio_usbvid *)data;
okay
+ int id_current;
Re: [PATCH v3 1/3] extcon: extcon-gpio-usbvid: Generic USB VBUS/ID detection via GPIO
Hi George,
You didn't modify this patchset about my comment on v1 patchset.
Please pay attention to comment.
On 08/29/2013 02:33 AM, George Cherian wrote:
> Add a generic USB VBUS/ID detection EXTCON driver. This driver expects
> the ID/VBUS pin are connected via GPIOs. This driver is tested on
> DRA7x board were the ID pin is routed via GPIOs. The driver supports
> both VBUS and ID pin configuration and ID pin only configuration.
>
> Signed-off-by: George Cherian
> ---
> .../bindings/extcon/extcon-gpio-usbvid.txt | 20 ++
> drivers/extcon/Kconfig | 6 +
> drivers/extcon/Makefile| 1 +
> drivers/extcon/extcon-gpio-usbvid.c| 286
> +
You should keep following naming stlye. extcon-gpio-usbvid.c is wrong naming
style.
- extcon-[device name].c
- extcon-gpio-usbvid.c -> extcon-dra7xx.c or etc.
Also, you should change the file name of extcon-gpio-usbvid.txt.
Finally, You used 'gpio_usbvid' prefix in extcon-gpio-usbvid.c.
It has caused the confusion that user would think extcon-gpio-usbvid.c driver
support all of extcon driver using gpio irq pin. So I'd like you to use
proper prefix including device name.
> 4 files changed, 313 insertions(+)
> create mode 100644
> Documentation/devicetree/bindings/extcon/extcon-gpio-usbvid.txt
> create mode 100644 drivers/extcon/extcon-gpio-usbvid.c
>
> diff --git a/Documentation/devicetree/bindings/extcon/extcon-gpio-usbvid.txt
> b/Documentation/devicetree/bindings/extcon/extcon-gpio-usbvid.txt
> new file mode 100644
> index 000..eea0741
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/extcon/extcon-gpio-usbvid.txt
> @@ -0,0 +1,20 @@
> +EXTCON FOR USB VIA GPIO
> +
> +Required Properties:
> + - compatible : Should be "ti,gpio-usb-vid" for USB VBUS-ID detector
> + using gpios or "ti,gpio-usb-id" for USB ID pin detector
> + - gpios : phandle and args ID pin gpio and VBUS gpio.
> +The first gpio used for ID pin detection
> +and the second used for VBUS detection.
> +ID pin gpio is mandatory and VBUS is optional
> +depending on implementation.
> +
> +Please refer to ../gpio/gpio.txt for details of the common GPIO bindings
> +
> +Example:
> +
> + gpio_usbvid_extcon1 {
> + compatible = "ti,gpio-usb-vid";
> + gpios = < 1 0>,
> + < 2 0>;
> + };
> diff --git a/drivers/extcon/Kconfig b/drivers/extcon/Kconfig
> index f1d54a3..8097398 100644
> --- a/drivers/extcon/Kconfig
> +++ b/drivers/extcon/Kconfig
> @@ -64,4 +64,10 @@ config EXTCON_PALMAS
> Say Y here to enable support for USB peripheral and USB host
> detection by palmas usb.
>
> +config EXTCON_GPIO_USBVID
> + tristate "Generic USB VBUS/ID detection using GPIO EXTCON support"
> + help
> + Say Y here to enable support for USB VBUS/ID deetction by GPIO.
> +
> +
Remove blank line.
> endif # MULTISTATE_SWITCH
> diff --git a/drivers/extcon/Makefile b/drivers/extcon/Makefile
> index e4fa8ba..0451f698 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_GPIO_USBVID) += extcon-gpio-usbvid.o
> diff --git a/drivers/extcon/extcon-gpio-usbvid.c
> b/drivers/extcon/extcon-gpio-usbvid.c
> new file mode 100644
> index 000..e9bc2a97
> --- /dev/null
> +++ b/drivers/extcon/extcon-gpio-usbvid.c
> @@ -0,0 +1,286 @@
> +/*
> + * Generic USB VBUS-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
> + *
> + * Based on extcon-palmas.c
> + *
> + * Author: Kishon Vijay Abraham I
> + *
> + * 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
> +#include
> +#include
> +#include
kthead.h, freezer.h headerfile is used in this file?
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
Order headerfile alphabetically.
> +
> +struct gpio_usbvid {
> + struct device *dev;
> +
> + struct extcon_dev edev;
> +
> + /*GPIO pin */
I commented previous your patch about this wrong coding style.
Why did not you fix this coding style?
> + int id_gpio;
> + int vbus_gpio;
> +
> + int id_irq;
> + int
[PATCH v3 1/3] extcon: extcon-gpio-usbvid: Generic USB VBUS/ID detection via GPIO
Add a generic USB VBUS/ID detection EXTCON driver. This driver expects
the ID/VBUS pin are connected via GPIOs. This driver is tested on
DRA7x board were the ID pin is routed via GPIOs. The driver supports
both VBUS and ID pin configuration and ID pin only configuration.
Signed-off-by: George Cherian
---
.../bindings/extcon/extcon-gpio-usbvid.txt | 20 ++
drivers/extcon/Kconfig | 6 +
drivers/extcon/Makefile| 1 +
drivers/extcon/extcon-gpio-usbvid.c| 286 +
4 files changed, 313 insertions(+)
create mode 100644
Documentation/devicetree/bindings/extcon/extcon-gpio-usbvid.txt
create mode 100644 drivers/extcon/extcon-gpio-usbvid.c
diff --git a/Documentation/devicetree/bindings/extcon/extcon-gpio-usbvid.txt
b/Documentation/devicetree/bindings/extcon/extcon-gpio-usbvid.txt
new file mode 100644
index 000..eea0741
--- /dev/null
+++ b/Documentation/devicetree/bindings/extcon/extcon-gpio-usbvid.txt
@@ -0,0 +1,20 @@
+EXTCON FOR USB VIA GPIO
+
+Required Properties:
+ - compatible : Should be "ti,gpio-usb-vid" for USB VBUS-ID detector
+ using gpios or "ti,gpio-usb-id" for USB ID pin detector
+ - gpios : phandle and args ID pin gpio and VBUS gpio.
+ The first gpio used for ID pin detection
+ and the second used for VBUS detection.
+ ID pin gpio is mandatory and VBUS is optional
+ depending on implementation.
+
+Please refer to ../gpio/gpio.txt for details of the common GPIO bindings
+
+Example:
+
+ gpio_usbvid_extcon1 {
+ compatible = "ti,gpio-usb-vid";
+ gpios = < 1 0>,
+ < 2 0>;
+ };
diff --git a/drivers/extcon/Kconfig b/drivers/extcon/Kconfig
index f1d54a3..8097398 100644
--- a/drivers/extcon/Kconfig
+++ b/drivers/extcon/Kconfig
@@ -64,4 +64,10 @@ config EXTCON_PALMAS
Say Y here to enable support for USB peripheral and USB host
detection by palmas usb.
+config EXTCON_GPIO_USBVID
+ tristate "Generic USB VBUS/ID detection using GPIO EXTCON support"
+ help
+ Say Y here to enable support for USB VBUS/ID deetction by GPIO.
+
+
endif # MULTISTATE_SWITCH
diff --git a/drivers/extcon/Makefile b/drivers/extcon/Makefile
index e4fa8ba..0451f698 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_GPIO_USBVID) += extcon-gpio-usbvid.o
diff --git a/drivers/extcon/extcon-gpio-usbvid.c
b/drivers/extcon/extcon-gpio-usbvid.c
new file mode 100644
index 000..e9bc2a97
--- /dev/null
+++ b/drivers/extcon/extcon-gpio-usbvid.c
@@ -0,0 +1,286 @@
+/*
+ * Generic USB VBUS-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
+ *
+ * Based on extcon-palmas.c
+ *
+ * Author: Kishon Vijay Abraham I
+ *
+ * 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
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+struct gpio_usbvid {
+ struct device *dev;
+
+ struct extcon_dev edev;
+
+ /*GPIO pin */
+ int id_gpio;
+ int vbus_gpio;
+
+ int id_irq;
+ int vbus_irq;
+ int type;
+};
+
+static const char *dra7xx_extcon_cable[] = {
+ [0] = "USB",
+ [1] = "USB-HOST",
+ NULL,
+};
+
+static const int mutually_exclusive[] = {0x3, 0x0};
+
+/* Two types of support are provided.
+ * Systems which has
+ * 1) VBUS and ID pin connected via GPIO
+ * 2) only ID pin connected via GPIO
+ * For Case 1 both the gpios should be provided via DT
+ * Always the first GPIO in dt is considered ID pin GPIO
+ */
+
+enum {
+ UNKNOWN = 0,
+ ID_DETECT,
+ VBUS_ID_DETECT,
+};
+
+#define ID_GND 0
+#define ID_FLOAT 1
+#define VBUS_OFF 0
+#define VBUS_ON1
+
+
+static irqreturn_t id_irq_handler(int irq, void *data)
+{
+ struct gpio_usbvid *gpio_usbvid = (struct gpio_usbvid *) data;
+ int id_current;
+
+ id_current = gpio_get_value_cansleep(gpio_usbvid->id_gpio);
+ if (id_current == ID_GND) {
+ if (gpio_usbvid->type == ID_DETECT)
+ extcon_set_cable_state(_usbvid->edev,
+
[PATCH v3 1/3] extcon: extcon-gpio-usbvid: Generic USB VBUS/ID detection via GPIO
Add a generic USB VBUS/ID detection EXTCON driver. This driver expects
the ID/VBUS pin are connected via GPIOs. This driver is tested on
DRA7x board were the ID pin is routed via GPIOs. The driver supports
both VBUS and ID pin configuration and ID pin only configuration.
Signed-off-by: George Cherian george.cher...@ti.com
---
.../bindings/extcon/extcon-gpio-usbvid.txt | 20 ++
drivers/extcon/Kconfig | 6 +
drivers/extcon/Makefile| 1 +
drivers/extcon/extcon-gpio-usbvid.c| 286 +
4 files changed, 313 insertions(+)
create mode 100644
Documentation/devicetree/bindings/extcon/extcon-gpio-usbvid.txt
create mode 100644 drivers/extcon/extcon-gpio-usbvid.c
diff --git a/Documentation/devicetree/bindings/extcon/extcon-gpio-usbvid.txt
b/Documentation/devicetree/bindings/extcon/extcon-gpio-usbvid.txt
new file mode 100644
index 000..eea0741
--- /dev/null
+++ b/Documentation/devicetree/bindings/extcon/extcon-gpio-usbvid.txt
@@ -0,0 +1,20 @@
+EXTCON FOR USB VIA GPIO
+
+Required Properties:
+ - compatible : Should be ti,gpio-usb-vid for USB VBUS-ID detector
+ using gpios or ti,gpio-usb-id for USB ID pin detector
+ - gpios : phandle and args ID pin gpio and VBUS gpio.
+ The first gpio used for ID pin detection
+ and the second used for VBUS detection.
+ ID pin gpio is mandatory and VBUS is optional
+ depending on implementation.
+
+Please refer to ../gpio/gpio.txt for details of the common GPIO bindings
+
+Example:
+
+ gpio_usbvid_extcon1 {
+ compatible = ti,gpio-usb-vid;
+ gpios = gpio1 1 0,
+ gpio2 2 0;
+ };
diff --git a/drivers/extcon/Kconfig b/drivers/extcon/Kconfig
index f1d54a3..8097398 100644
--- a/drivers/extcon/Kconfig
+++ b/drivers/extcon/Kconfig
@@ -64,4 +64,10 @@ config EXTCON_PALMAS
Say Y here to enable support for USB peripheral and USB host
detection by palmas usb.
+config EXTCON_GPIO_USBVID
+ tristate Generic USB VBUS/ID detection using GPIO EXTCON support
+ help
+ Say Y here to enable support for USB VBUS/ID deetction by GPIO.
+
+
endif # MULTISTATE_SWITCH
diff --git a/drivers/extcon/Makefile b/drivers/extcon/Makefile
index e4fa8ba..0451f698 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_GPIO_USBVID) += extcon-gpio-usbvid.o
diff --git a/drivers/extcon/extcon-gpio-usbvid.c
b/drivers/extcon/extcon-gpio-usbvid.c
new file mode 100644
index 000..e9bc2a97
--- /dev/null
+++ b/drivers/extcon/extcon-gpio-usbvid.c
@@ -0,0 +1,286 @@
+/*
+ * Generic USB VBUS-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.cher...@ti.com
+ *
+ * Based on extcon-palmas.c
+ *
+ * Author: Kishon Vijay Abraham I kis...@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 gpio_usbvid {
+ struct device *dev;
+
+ struct extcon_dev edev;
+
+ /*GPIO pin */
+ int id_gpio;
+ int vbus_gpio;
+
+ int id_irq;
+ int vbus_irq;
+ int type;
+};
+
+static const char *dra7xx_extcon_cable[] = {
+ [0] = USB,
+ [1] = USB-HOST,
+ NULL,
+};
+
+static const int mutually_exclusive[] = {0x3, 0x0};
+
+/* Two types of support are provided.
+ * Systems which has
+ * 1) VBUS and ID pin connected via GPIO
+ * 2) only ID pin connected via GPIO
+ * For Case 1 both the gpios should be provided via DT
+ * Always the first GPIO in dt is considered ID pin GPIO
+ */
+
+enum {
+ UNKNOWN = 0,
+ ID_DETECT,
+ VBUS_ID_DETECT,
+};
+
+#define ID_GND 0
+#define ID_FLOAT 1
+#define VBUS_OFF 0
+#define VBUS_ON1
+
+
+static irqreturn_t id_irq_handler(int irq, void *data)
+{
+ struct gpio_usbvid *gpio_usbvid = (struct gpio_usbvid *) data;
+ int id_current;
+
+
Re: [PATCH v3 1/3] extcon: extcon-gpio-usbvid: Generic USB VBUS/ID detection via GPIO
Hi George,
You didn't modify this patchset about my comment on v1 patchset.
Please pay attention to comment.
On 08/29/2013 02:33 AM, George Cherian wrote:
Add a generic USB VBUS/ID detection EXTCON driver. This driver expects
the ID/VBUS pin are connected via GPIOs. This driver is tested on
DRA7x board were the ID pin is routed via GPIOs. The driver supports
both VBUS and ID pin configuration and ID pin only configuration.
Signed-off-by: George Cherian george.cher...@ti.com
---
.../bindings/extcon/extcon-gpio-usbvid.txt | 20 ++
drivers/extcon/Kconfig | 6 +
drivers/extcon/Makefile| 1 +
drivers/extcon/extcon-gpio-usbvid.c| 286
+
You should keep following naming stlye. extcon-gpio-usbvid.c is wrong naming
style.
- extcon-[device name].c
- extcon-gpio-usbvid.c - extcon-dra7xx.c or etc.
Also, you should change the file name of extcon-gpio-usbvid.txt.
Finally, You used 'gpio_usbvid' prefix in extcon-gpio-usbvid.c.
It has caused the confusion that user would think extcon-gpio-usbvid.c driver
support all of extcon driver using gpio irq pin. So I'd like you to use
proper prefix including device name.
4 files changed, 313 insertions(+)
create mode 100644
Documentation/devicetree/bindings/extcon/extcon-gpio-usbvid.txt
create mode 100644 drivers/extcon/extcon-gpio-usbvid.c
diff --git a/Documentation/devicetree/bindings/extcon/extcon-gpio-usbvid.txt
b/Documentation/devicetree/bindings/extcon/extcon-gpio-usbvid.txt
new file mode 100644
index 000..eea0741
--- /dev/null
+++ b/Documentation/devicetree/bindings/extcon/extcon-gpio-usbvid.txt
@@ -0,0 +1,20 @@
+EXTCON FOR USB VIA GPIO
+
+Required Properties:
+ - compatible : Should be ti,gpio-usb-vid for USB VBUS-ID detector
+ using gpios or ti,gpio-usb-id for USB ID pin detector
+ - gpios : phandle and args ID pin gpio and VBUS gpio.
+The first gpio used for ID pin detection
+and the second used for VBUS detection.
+ID pin gpio is mandatory and VBUS is optional
+depending on implementation.
+
+Please refer to ../gpio/gpio.txt for details of the common GPIO bindings
+
+Example:
+
+ gpio_usbvid_extcon1 {
+ compatible = ti,gpio-usb-vid;
+ gpios = gpio1 1 0,
+ gpio2 2 0;
+ };
diff --git a/drivers/extcon/Kconfig b/drivers/extcon/Kconfig
index f1d54a3..8097398 100644
--- a/drivers/extcon/Kconfig
+++ b/drivers/extcon/Kconfig
@@ -64,4 +64,10 @@ config EXTCON_PALMAS
Say Y here to enable support for USB peripheral and USB host
detection by palmas usb.
+config EXTCON_GPIO_USBVID
+ tristate Generic USB VBUS/ID detection using GPIO EXTCON support
+ help
+ Say Y here to enable support for USB VBUS/ID deetction by GPIO.
+
+
Remove blank line.
endif # MULTISTATE_SWITCH
diff --git a/drivers/extcon/Makefile b/drivers/extcon/Makefile
index e4fa8ba..0451f698 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_GPIO_USBVID) += extcon-gpio-usbvid.o
diff --git a/drivers/extcon/extcon-gpio-usbvid.c
b/drivers/extcon/extcon-gpio-usbvid.c
new file mode 100644
index 000..e9bc2a97
--- /dev/null
+++ b/drivers/extcon/extcon-gpio-usbvid.c
@@ -0,0 +1,286 @@
+/*
+ * Generic USB VBUS-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.cher...@ti.com
+ *
+ * Based on extcon-palmas.c
+ *
+ * Author: Kishon Vijay Abraham I kis...@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
kthead.h, freezer.h headerfile is used in this file?
+#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
Order headerfile alphabetically.
+
+struct gpio_usbvid {
+ struct device *dev;
+
+ struct extcon_dev edev;
+
+ /*GPIO pin */
I commented previous your patch about this wrong coding style.
Why did not you
Re: [PATCH v3 1/3] extcon: extcon-gpio-usbvid: Generic USB VBUS/ID detection via GPIO
Hi Chanwoo,
Thanks for the review and sorry for all the trivial mistakes.
On 8/29/2013 7:05 AM, Chanwoo Choi wrote:
Hi George,
You didn't modify this patchset about my comment on v1 patchset.
Please pay attention to comment.
On 08/29/2013 02:33 AM, George Cherian wrote:
Add a generic USB VBUS/ID detection EXTCON driver. This driver expects
the ID/VBUS pin are connected via GPIOs. This driver is tested on
DRA7x board were the ID pin is routed via GPIOs. The driver supports
both VBUS and ID pin configuration and ID pin only configuration.
Signed-off-by: George Cherian george.cher...@ti.com
---
.../bindings/extcon/extcon-gpio-usbvid.txt | 20 ++
drivers/extcon/Kconfig | 6 +
drivers/extcon/Makefile| 1 +
drivers/extcon/extcon-gpio-usbvid.c| 286 +
You should keep following naming stlye. extcon-gpio-usbvid.c is wrong naming
style.
- extcon-[device name].c
- extcon-gpio-usbvid.c - extcon-dra7xx.c or etc.
Actually dra7xx is the SoC name and the USB VBUS/ID detection is not
specific to SoC.
It uses gpios to detect the VBUS/ID change. So i thought it would be
better to have generic
gpio based VBUS/ID detection rather than making dra7xx specific. Stephen
Warren had this opinion
with patch v1.
Also, you should change the file name of extcon-gpio-usbvid.txt.
Finally, You used 'gpio_usbvid' prefix in extcon-gpio-usbvid.c.
It has caused the confusion that user would think extcon-gpio-usbvid.c driver
support all of extcon driver using gpio irq pin. So I'd like you to use
proper prefix including device name.
I meant to support all of extcon driver using gpio for USB VBUS/ID
detection.
4 files changed, 313 insertions(+)
create mode 100644
Documentation/devicetree/bindings/extcon/extcon-gpio-usbvid.txt
create mode 100644 drivers/extcon/extcon-gpio-usbvid.c
[snip]
index f1d54a3..8097398 100644
--- a/drivers/extcon/Kconfig
+++ b/drivers/extcon/Kconfig
@@ -64,4 +64,10 @@ config EXTCON_PALMAS
Say Y here to enable support for USB peripheral and USB host
detection by palmas usb.
+config EXTCON_GPIO_USBVID
+ tristate Generic USB VBUS/ID detection using GPIO EXTCON support
+ help
+ Say Y here to enable support for USB VBUS/ID deetction by GPIO.
+
+
Remove blank line.
okay
endif # MULTISTATE_SWITCH
[snip]
diff --git a/drivers/extcon/extcon-gpio-usbvid.c
b/drivers/extcon/extcon-gpio-usbvid.c
new file mode 100644
index 000..e9bc2a97
--- /dev/null
+++ b/drivers/extcon/extcon-gpio-usbvid.c
@@ -0,0 +1,286 @@
+/*
+ * Generic USB VBUS-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.cher...@ti.com
+ *
+ * Based on extcon-palmas.c
+ *
+ * Author: Kishon Vijay Abraham I kis...@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
kthead.h, freezer.h headerfile is used in this file?
okay
+#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
Order headerfile alphabetically.
okay
+
+struct gpio_usbvid {
+ struct device *dev;
+
+ struct extcon_dev edev;
+
+ /*GPIO pin */
I commented previous your patch about this wrong coding style.
Why did not you fix this coding style?
okay
+ int id_gpio;
+ int vbus_gpio;
+
+ int id_irq;
+ int vbus_irq;
+ int type;
+};
+
+static const char *dra7xx_extcon_cable[] = {
+ [0] = USB,
+ [1] = USB-HOST,
+ NULL,
+};
+
+static const int mutually_exclusive[] = {0x3, 0x0};
+
+/* Two types of support are provided.
+ * Systems which has
+ * 1) VBUS and ID pin connected via GPIO
+ * 2) only ID pin connected via GPIO
Remove blank between '2)' and 'only'.
okay
+ * For Case 1 both the gpios should be provided via DT
+ * Always the first GPIO in dt is considered ID pin GPIO
+ */
+
+enum {
+ UNKNOWN = 0,
+ ID_DETECT,
+ VBUS_ID_DETECT,
+};
+
+#define ID_GND 0
+#define ID_FLOAT 1
+#define VBUS_OFF 0
+#define VBUS_ON1
I think you could only use two constant instead of four constant definition.
you mean only ID_GND and VBUS_OFF?
+
+
This blank line isn't necessary.
+static irqreturn_t id_irq_handler(int irq, void
