lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Wed, 25 Feb 2015 21:28:36 +0800 From: zhangfei <zhangfei.gao@...aro.org> To: balbi@...com CC: Kishon Vijay Abraham I <kishon@...com>, mark.rutland@....com, Peter Chen <peter.chen@...escale.com>, Sergei Shtylyov <sergei.shtylyov@...entembedded.com>, "dan . zhao" <dan.zhao@...ilicon.com>, Wangbinghui <wangbinghui@...ilicon.com>, linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org, devicetree@...r.kernel.org, linux-usb@...r.kernel.org, Roger Quadros <rogerq@...com> Subject: Re: [PATCH v4 4/4] phy: add phy-hi6220-usb On 02/23/2015 11:36 PM, Felipe Balbi wrote: > Hi, > > On Sun, Feb 22, 2015 at 11:10:36AM +0800, zhangfei 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. >>> >>> no problem. >>> >>>> 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 >>> >>> drivers/phy, if the framework misses anything you need, it's a great >>> opportunity to give back to the community by extending the framework. >> >> Sorry, I am a little confused. >> I need some concrete suggestion for the next step of this patch, which is >> required for the community board, hikey board. >> >> Do you mean in the future we need use hsotg->phy instead of hsotg->uphy. >> struct phy *phy; >> struct usb_phy *uphy; > > yes, we need to move everybody to use struct phy, instead of struct > usb_phy. > >> usb_phy has many members that struct phy does not have, including otg. >> struct usb_otg *otg; >> Is that mean we need port such member from usb_phy to phy. > > This means we have a little ground work to do before we can add your phy > driver to that framework, right ? As I said, if the framework is missing > anything, work with Kishon (generic phy maintainer) to add those missing > pieces generically. OK, got it. > >> Besides, are you ok with using usb_gadget_vbus_connect/disconnect. >> >>> >>> Scratching one's own itch kinda thing... >>> >>>>>>>> +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. >>> >>> well, add DT. The whole idea of free software is that we improve on >>> things we already have. EXTCON is *the* API to handle such things. >> >> I think I am still not understanding extcon-gpio, not sure why need >> use this API here. > > because extcon is the API to use for external connectors. The same way > you use regulator framework to control that single GPIO tied to an > enable signal of a fixed regulator, you use extcon when you need to read > that gpio signal tied to id pin of the USB connector. > >> Here two gpio requires, one gpio as interrupt, in the interrupt >> handler, we detect the gpio status judging the otg status. >> extcon-gpio.c use the interrupt, then can we also use the gpio >> interrupt. Using extcon-gpio is used for saving gpio_request? > > extcon is used to hide gpio_request from dwc2. dwc2 only knows about > extcon, not gpios. extcon will request the gpio and use it as interrupt > source. When an IRQ fires, it will read the gpio state and decide if it > should broadcast a message to tell dwc2 to become host or peripheral. Thanks for the kind education, understand now. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@...r.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists