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-omap" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html