lkml.org 
[lkml]   [2013]   [Aug]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [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


    \
     
     \ /
      Last update: 2013-08-30 02:21    [W:2.917 / U:0.292 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site