Re: [PATCH v4 4/4] phy: add phy-hi6220-usb

[zhangfei]

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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/