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
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
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
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
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_gp
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 guara
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-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.
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 c
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"
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
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 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 >>> --- >>> .../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 PARTICULAR
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 = <&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 > + * > + * 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; >