On Sat, 11 Oct 2014, Peter Chen wrote: > > > + 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? I meant that writing the port-power bit this way will cause the port-changed bits to be turned off. > > 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? What happens if somebody writes a callback routine and forgets to call ehci_port_power? It's safer to arrange the code so that people don't need to remember that kind of detail. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
