On Fri, Oct 10, 2014 at 10:31:24AM -0400, Alan Stern wrote:
> On Fri, 10 Oct 2014, Peter Chen wrote:
>
> > @@ -1216,6 +1221,7 @@ static const struct hc_driver ehci_hc_driver = {
> > .bus_resume = ehci_bus_resume,
> > .relinquish_port = ehci_relinquish_port,
> > .port_handed_over = ehci_port_handed_over,
> > + .port_power = ehci_port_power,
> >
> > /*
> > * device support
> > @@ -1233,6 +1239,8 @@ void ehci_init_driver(struct hc_driver *drv,
> > drv->hcd_priv_size += over->extra_priv_size;
> > if (over->reset)
> > drv->reset = over->reset;
> > + if (over->port_power)
> > + drv->port_power = over->port_power;
>
> > +int ehci_port_power(struct usb_hcd *hcd, int portnum, bool enable)
> > +{
> > + struct ehci_hcd *ehci = hcd_to_ehci(hcd);
>
> You might as well pass ehci as the first argument instead of hcd.
>
> > + u32 __iomem *status_reg = &ehci->regs->port_status[portnum];
> > + u32 status = ehci_readl(ehci, status_reg);
> > +
> > + if (enable)
> > + ehci_writel(ehci, status | PORT_POWER, status_reg);
> > + else
> > + ehci_writel(ehci, status & ~PORT_POWER, status_reg);
>
> These writes are wrong. You have to turn off the RWC bits before
> writing anything to the status register; otherwise you will
> accidentally turn off some bit that should remain on.
>
Yes, I see the code at ehci-hub.c do like this.
Can I know the reason why these port changed bits written (cleared)
will cause other bits be turned off?
> > +
> > + return 0;
>
> You missed the point of what I said earlier. This routine should not
> be a callback, because it should run every time we adjust the port
> power. Then _this_ routine should invoke the callback.
>
Would you please explain the benefit for your suggestion?
If that, ehci_port_power would be an additional API to control
port power, and we also need hc_driver callback to define the
original port power control way like ehci_port_power in my patch.
Why we can't invoke the callback every time when we want to adjust
the port power?
--
Best Regards,
Peter Chen
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
