Messages in this thread | | | Date | Sat, 21 Feb 2015 23:03:05 +0800 | From | zhangfei <> | Subject | Re: [PATCH v4 4/4] phy: add phy-hi6220-usb |
| |
Hi, Balbi
On 02/21/2015 12:06 AM, Felipe Balbi wrote: > Hi, > > On Fri, Feb 20, 2015 at 11:44:37PM +0800, zhangfei wrote: >> Hi, Balbi >> >> On 02/20/2015 10:41 PM, Felipe Balbi wrote: >> >>>> +static void hi6220_start_peripheral(struct hi6220_priv *priv, bool on) >>>> +{ >>>> + struct usb_otg *otg = priv->phy.otg; >>>> + >>>> + if (!otg->gadget) >>>> + return; >>>> + >>>> + if (on) >>>> + usb_gadget_connect(otg->gadget); >>>> + else >>>> + usb_gadget_disconnect(otg->gadget); >>> >>> why is the PHY fiddling with pullups ? >> >> We use this to enable/disable otg gadget mode. > > I got that, but the pullups don't belong to the PHY, they belong to the > gadget. > >> The gpio_id & gpio_vbus are used to distinguish otg gadget mode or >> host mode. >> When micro usb or otg device attached to otg, gpio_vbus falling down. >> And gpio_id = 1 is micro usb, gpio_id = 0 is otg device. > > all of that I understood clearly :-) > >> So when micro usb attached, we enable gadget mode; while micro usb >> detached, we disable gadget mode, and dwc2 will automatically set to >> host mode. > > that's all fine, I'm concerned about letting the PHY fiddle with > something it doesn't own. If I am to change pullups rules in udc-core, > this is likely to break down miserably and I don't want to have to go > through that.
Thanks for the clarifying.
How about using usb_gadget_vbus_connect/disconnect, which are used in many files under drivers/usb/phy. There is no vbus_session in dwc2/gadget.c, I thought it would be same as pullup.
However, usb_gadget_vbus_connect still need para gadget, where should we put this file, drivers/usb/phy or drivers/phy
> >>>> +static void hi6220_detect_work(struct work_struct *work) >>>> +{ >>>> + struct hi6220_priv *priv = >>>> + container_of(work, struct hi6220_priv, work.work); >>>> + int gpio_id, gpio_vbus; >>>> + enum usb_otg_state state; >>>> + >>>> + if (!gpio_is_valid(priv->gpio_id) || !gpio_is_valid(priv->gpio_vbus)) >>>> + return; >>>> + >>>> + gpio_id = gpio_get_value_cansleep(priv->gpio_id); >>>> + gpio_vbus = gpio_get_value_cansleep(priv->gpio_vbus); >>> >>> looks like this should be using extcon >> Not used extcon before. >> However, we need gpio_vbus interrupt. >> Checked phy-tahvo.c and phy-omap-otg.c, not find extcon related with >> interrupt. >> Will investigate tomorrow. > > drivers/extcon/extcon-gpio.c I think there is no need to use extcon, gpio is clear enough. extcon-gpio.c even do not support dt.
> >>>> + if (gpio_vbus == 0) { >>>> + if (gpio_id == 1) >>>> + state = OTG_STATE_B_PERIPHERAL; >>>> + else >>>> + state = OTG_STATE_A_HOST; >>>> + } else { >>>> + state = OTG_STATE_A_HOST; >>>> + } >>>> + >>>> + if (priv->state != state) { >>>> + hi6220_start_peripheral(priv, state == OTG_STATE_B_PERIPHERAL); >>>> + priv->state = state; >>>> + } >>>> +} >>>> + >>>> +static irqreturn_t hiusb_gpio_intr(int irq, void *data) >>>> +{ >>>> + struct hi6220_priv *priv = (struct hi6220_priv *)data; >>>> + >>>> + /* add debounce time */ >>>> + schedule_delayed_work(&priv->work, msecs_to_jiffies(100)); >>> >>> this is really bad. We have threaded interrupt support, right ? >> >> Since we use two gpio to distinguish gadget mode or host mode. >> Debounce time can introduce more accuracy. > > gpio_set_debounce() ? Not all gpio.c support set_debounce, including gpio-pl061.c.
> >> I think threaded interrupt can not be used for adding debounce time. >> Here add debounce is just for safety. > > add the debounce to the gpio itself.
Here the debounce added only for safety. gpio_id may mis-report when unplug usb, but it is correct for plug usb & otg device. So debounce can be omitted. If you think using delayed work for debounce is ugly, it is fine switch to threaded_irq.
Thanks
| |