Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752692AbbBWPhS (ORCPT ); Mon, 23 Feb 2015 10:37:18 -0500 Received: from comal.ext.ti.com ([198.47.26.152]:49967 "EHLO comal.ext.ti.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751995AbbBWPhQ (ORCPT ); Mon, 23 Feb 2015 10:37:16 -0500 Date: Mon, 23 Feb 2015 09:36:12 -0600 From: Felipe Balbi To: zhangfei CC: , Kishon Vijay Abraham I , , Peter Chen , Sergei Shtylyov , "dan . zhao" , Wangbinghui , , , , , Roger Quadros Subject: Re: [PATCH v4 4/4] phy: add phy-hi6220-usb Message-ID: <20150223153612.GF32701@saruman.tx.rr.com> Reply-To: References: <1423726646-30336-1-git-send-email-zhangfei.gao@linaro.org> <1423726646-30336-5-git-send-email-zhangfei.gao@linaro.org> <20150220144119.GB1707@saruman.tx.rr.com> <54E75665.6050205@linaro.org> <20150220160610.GB6430@saruman.tx.rr.com> <54E89E29.4010807@linaro.org> <20150221162102.GA1784@saruman.tx.rr.com> <54E948AC.2010609@linaro.org> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="27ZtN5FSuKKSZcBU" Content-Disposition: inline In-Reply-To: <54E948AC.2010609@linaro.org> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 8885 Lines: 238 --27ZtN5FSuKKSZcBU Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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 =3D 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 =3D 1 is micro usb, gpio_id =3D 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 m= any > >>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 w= e 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. >=20 > 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. >=20 > 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. > Besides, are you ok with using usb_gadget_vbus_connect/disconnect. >=20 > > > >Scratching one's own itch kinda thing... > > > >>>>>>+static void hi6220_detect_work(struct work_struct *work) > >>>>>>+{ > >>>>>>+ struct hi6220_priv *priv =3D > >>>>>>+ 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_vb= us)) > >>>>>>+ return; > >>>>>>+ > >>>>>>+ gpio_id =3D gpio_get_value_cansleep(priv->gpio_id); > >>>>>>+ gpio_vbus =3D 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. >=20 > 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. > >Quite frankly, though, Roger Quadros (now in Cc) has been working to get > >DT support on extcon-gpio, so perhaps wait for that and base your > >patches on top of his. > > > >Now your statement that GPIO is clear enough is completely bogus to me. > > > >Why do we have fixed regulators with GPIO enable signals, right ? Might > >as well just fiddle with the GPIO directly, right ? Wrong, the idea of > >using these frameworks is to encapsulate implementation details and make > >sure that if things change from one board to another, we can still use > >our SW without major modifications. > > > >>>>>>+ if (gpio_vbus =3D=3D 0) { > >>>>>>+ if (gpio_id =3D=3D 1) > >>>>>>+ state =3D OTG_STATE_B_PERIPHERAL; > >>>>>>+ else > >>>>>>+ state =3D OTG_STATE_A_HOST; > >>>>>>+ } else { > >>>>>>+ state =3D OTG_STATE_A_HOST; > >>>>>>+ } > >>>>>>+ > >>>>>>+ if (priv->state !=3D state) { > >>>>>>+ hi6220_start_peripheral(priv, state =3D=3D OTG_STATE_B_PERIPHERA= L); > >>>>>>+ priv->state =3D state; > >>>>>>+ } > >>>>>>+} > >>>>>>+ > >>>>>>+static irqreturn_t hiusb_gpio_intr(int irq, void *data) > >>>>>>+{ > >>>>>>+ struct hi6220_priv *priv =3D (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. > > > >then the framework should implement it in SW. That was the whole idea of > >adding set_debounce() to gpiolib. If the controller can't handle it, > >gpiolib handles it in SW. Again, encapsulating details. > > > >>>>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. > > > >debounce might be very well needed. We *do* want to filter out the > >transient period. I'm just telling you there are better ways of doing > >so; and your response to that is "let's just remove it" and I'm not > >really comfortable with that attitude. > > > >That's the attitude of a lazy person which, I hope, you are not ;) >=20 > Understand. > What I mean here is gpio_id is not used when unplug, it is only used after > plug. sure, but does it make a difference if you let gpiolib handle debouncing for you ? It makes no difference at all, right ? --=20 balbi --27ZtN5FSuKKSZcBU Content-Type: application/pgp-signature; name="signature.asc" Content-Description: Digital signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJU60jsAAoJEIaOsuA1yqRE7xMP/RqFj7mlOUhwtd75bs1Gf270 ELHKedQX5CrmtiY5jMEWxVbTECl8/0UzOdQtdjwLRJAULGXxdn341xQ41fye5d9Q ctNs3MRSALS50USKmsRQHSBcFhE7bY1Fm+jtX2hY1V17C2oNGuCBuXFoouRsfOX0 F33s/b7StgEUhZIzeT/odSV40GNXb+jc6PJliSNmWPwTtYXn24HB2ve8MOFujTsD ozi1ain4379w1woWOgf1wcs7MnOz+3bvWsOD3T/IlQ7Xb+mXqBGJj/UcJzXTzDxx +hoi+YjDoOUjE22dTTRHkIk1ClQ1+AOXWR1cd1D2jVHUYqQ1F+9ceVS6AebNnT51 UkEQAoQ33exmcts+n5XZPyrRUl6mbufR9pOuomJ+Exd9JFTuz639+T8hzpOd0eRs GVfLveWYcJVcZr3HFezRBpkQGkx6eCvKd8iKuuJYLmn2p9uefDtWTK0SPqIsfEq8 iFfxqlK3/t/lUhhczFk/x88bu5vnuQ4CaV5SmoSVF3zQq8pTmeUpndwHFSGcg4VM ij7At5S3WL9gIyEk9Pmg0PbL0lc7LwVvVRuEMNlqn5z2Q06xDMrZswtjCEFs2Qdb Mq8uG7Y0owo+iti3dIi/rTOLhI+Ro7j0ENkySUDljdLFW37uLc6iRXyWsaVFQ3BQ qzLGR6Xv/u7v+G97Gece =H+Js -----END PGP SIGNATURE----- --27ZtN5FSuKKSZcBU-- -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/