Re: [PATCH v3 1/3] extcon: extcon-gpio-usbvid: Generic USB VBUS/IDdetection via GPIO

[George Cherian]

On 8/30/2013 12:44 PM, Chanwoo Choi wrote:
> Hi George,
>
> In addition, I add answer about that device driver control gpio pin directly.
>
> On 08/30/2013 03:15 PM, George Cherian wrote:
> > Hi Chanwoo,
> >
> > On 8/30/2013 5:41 AM, Chanwoo Choi wrote:
> > > Hi George,
> > >
> > > On 08/29/2013 10:45 PM, George Cherian wrote:
> > > > Hi Chanwoo,
> > > >
> > > >
> > > > On 8/29/2013 5:42 PM, Chanwoo Choi wrote:
> > > > [big snip ]
> > > > > > > I tested various development board based on Samsung Exynos series SoC.
> > > > > > > Although some gpio of Exynos series SoC set high state(non zero, 1) as default value,
> > > > > > > this gpio state could mean off state, disconnected or un-powered state according to gpio.
> > > > > > > Of course, above explanation about specific gpio don't include all gpios.
> > > > > > > This is meaning that there is possibility.
> > > > > > okay then how about adding a flag for inverted logic too.  something like this
> > > > > >
> > > > > > if(of_property_read_bool(np,"inverted_gpio))
> > > > > >        gpio_usbvid->gpio_inv = 1;
> > > > > > And always check on this before deciding?
> > > > Is this fine ?
> > > OK.
> > > But, as Stephen commented on other mail, you should use proper DT helper function.
> > okay
> > > > > > > > > Second,
> > > > > > > > > gpio_usbvid_set_initial_state() dertermine both "USB-HOST" and "USB" cable state at same time
> > > > > > > > > in 'case ID_DETCT' according to 'gpio_usbvid->id_gpio'. But, I think that other extcon devices
> > > > > > > > > would not control both "USB-HOST" and "USB" cable state at same time.
> > > > > > > > >
> > > > > > > > > Other extcon devices would support either "USB-HOST" or "USB" cable.
> > > > > > > > The driver has 2 configurations.
> > > > > > > > 1) supports implementations with both VBUS and ID pin are routed via gpio's for swicthing roles(compatible  gpio-usb-vid).
> > > > > > > As you explained about case 1, it is only used on dra7x SoC.
> > > > > > No gpio-usb-id is used in dra7xx. dra7xx has got only ID pin routed via gpio.
> > > > > > > Other SoC could not wish to control both "USB-HOST" and "USB" cable at same time.
> > > > I could'nt actually parse this. You meant since the id_irq_handler handles both USB and USB-HOST cable
> > > > its not proper?
> > > It's not proper in general case. The generic driver must guarantee all of extcon device using gpio.
> > > As far as I know, the generic driver cannot direclty control gpio pin and get value from gpio pin.
> > > Almost device driver including in kernel/drivers control gpio pin on specific device driver
> > > instead of generic driver.
> > But without reading the gpio value how can we set the cable states?
> hmm. I mentioned above my answer as following:
> 	>> As far as I know, the generic driver cannot direclty control gpio pin and get value from gpio pin.
> 	>> Almost device driver including in kernel/drivers control gpio pin on specific device driver
> Because your extcon driver directly control gpio pin so I think your extcon driver isn't generic.
>
> for this driver the assumption is the dedicated gpio
Obviously when I am writing a generic driver for USB VBUS-ID detetction 
which is via gpio then i assume I have a dedicated gpio.
what is wrong in that assumption? How else can you detect ID pin change 
and VBUS change via gpio if not you have them dedicated?
> > is always DIR_IN and in the driver we just read the value.
> What is DIR_IN?
Direction IN gpio.
> > > > > I need your answer about above my opinion for other SoC.
> > > > So how about this, I have removed the dra7x specific stuffs (gpio-usb-id)
> > > >
> > > > static void gpio_usbvid_set_initial_state(struct gpio_usbvid *gpio_usbvid)
> > > > {
> > > >           int id_current, vbus_current;
> > > >
> > > >       id_current = gpio_get_value_cansleep(gpio_usbvid->id_gpio);
> > > >       if (!!id_current == ID_FLOAT)
> > > >           extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", false);
> > > >       else
> > > >           extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", true);
> > > >
> > > >       vbus_current = gpio_get_value_cansleep(gpio_usbvid->vbus_gpio);
> > > >        if (!!vbus_current == VBUS_ON)
> > > >           extcon_set_cable_state(&gpio_usbvid->edev, "USB", true);
> > > >       else
> > > >           extcon_set_cable_state(&gpio_usbvid->edev, "USB", false);
> > > > }
> > > >
> > > > and the irq handlers like this
> > > >
> > > > static irqreturn_t id_irq_handler(int irq, void *data)
> > > > {
> > > >           struct gpio_usbvid *gpio_usbvid = (struct gpio_usbvid *)data;
> > > >           int id_current;
> > > >
> > > >           id_current = gpio_get_value_cansleep(gpio_usbvid->id_gpio);
> > > >           if (id_current == ID_GND)
> > > >                   extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", true);
> > > >           else
> > > >                   extcon_set_cable_state(&gpio_usbvid->edev, "USB-HOST", false);
> > > >           return IRQ_HANDLED;
> > > > }
> > > >
> > > > static irqreturn_t vbus_irq_handler(int irq, void *data)
> > > > {
> > > >           struct gpio_usbvid *gpio_usbvid = (struct gpio_usbvid *)data;
> > > >           int vbus_current;
> > > >
> > > >           vbus_current = gpio_get_value_cansleep(gpio_usbvid->vbus_gpio);
> > > >           if (vbus_current == VBUS_OFF)
> > > >                   extcon_set_cable_state(&gpio_usbvid->edev, "USB", false);
> > > >           else
> > > >                   extcon_set_cable_state(&gpio_usbvid->edev, "USB", true);
> > > >
> > > >           return IRQ_HANDLED;
> > > > }
> > > I know your intention dividing interrupt handler for each cable.
> > > But I don't think this driver must guarantee all of extcon device using gpio.
> > >
> > > For example,
> > > below three SoC wish to detect USB/USB-HOST cable with each different methods.
> > >
> > > "SoC A" wish to detect USB/USB-HOST cable through only one gpio pin.
> > You mean to say that both USB ID pin and VBUS are connected to same gpio?
> > If so that is a really bad h/w design and it will not fly.
> > Or, you meant that only USB ID pin is connected to single gpio and it controls the state of the USB/USB-HOST cables?
> > If so thats what exactly the v3 driver did with compatible gpio-usb-id.
> >
> > > "SoC B" wish to detect USB/USB-HOST cable through ADC value instead of gpio pin.
> > Via ADC this driver should never be used , it should be extcon-adc-usbvid driver and not extcon-gpio-usbvid driver.
> > > "SoC C" wish to detect USB/USB-HOST cable through two gpio pin because USB connected on gpio an USB-HOST connected on another.
> > Whatever modifications above should meet this need  in combination with named gpios (id_gpio and vbus_gpio in dt)as stephen pointed.
> > But still i feel the above modification would even support Soc A provided the code registered for the notifier could handle it properly.
> > > 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?
> > Yes definitely, those register configuration should not be part of this driver and it should be done in the notifier handler.
> > > Finally,
> > > "SoC D" wish to detect USB/USB-HOST/JIG cable through two gpio pin
> > Correct me If I am wrong, USB JIG is not a standard cable. so for supporting that anyways you need to have
> > a different driver.
> > > - one gpio may detect either USB or USB-HOST and another may detect JIG cable
> > I assume u meant the USB ID pin is connected to one gpio and based on it value USB/USB-HOST  is detected.
> > > or one gpio may detect either USb or JIG and another may detect USB-HOST cable.
> >   As I mentioned earlier these are gpios configured as input and if you try to drive with 2 sources (USB and JIG or USB and USB-HOST etc)
> > then its a potentially bad design . If at all we need to identify all 3 then there should be 3 dedicated gpios.
> >
> > > 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.
> > I am sorry, I feel it supports all standard cases except for something like a JIG cable which is not standard.
> > > > [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.
> > Yes I agree and should be done by the subscriber to the notifier.
> > > > > > > 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
> Thanks,
> Chanwoo Choi


--
-George

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