Date:	Fri, 30 Aug 2013 09:11:09 +0900
From:	Chanwoo Choi <cw00.choi@...sung.com>
To:	George Cherian <george.cherian@...com>
Cc:	balbi@...com, myungjoo.ham@...sung.com, linux-doc@...r.kernel.org,
	linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
	grant.likely@...aro.org, rob@...dley.net, ian.campbell@...rix.com,
	swarren@...dotorg.org, mark.rutland@....com, pawel.moll@....com,
	rob.herring@...xeda.com, linux-omap@...r.kernel.org,
	linux-usb@...r.kernel.org, bcousson@...libre.com,
	davidb@...eaurora.org, arnd@...db.de, swarren@...dia.com,
	popcornmix@...il.com
Subject: 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 --> 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?

I commented above about your modification.

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

The setting order should be removed in generic driver.

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

Thanks,
Chanwoo Choi
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists