Hi Sergei,
thanks for finding time to review these patches.
On Sat, Sep 12, 2009 at 05:19:29PM +0400, Sergei Shtylyov wrote:
> Daniel Mack wrote:
>
> >+/* ULPI hardcoded IDs, used for probing */
> >+static unsigned int ulpi_ids[] = {
> >+ ULPI_ID(0x04cc, 0x1504), /* NXP ISP1504 */
> >+};
>
> I don't understand why you need this at all... what does depend in
> your code on the ULPI chip type?
It's just a list to detect whether there's a chip connected with a sane
ID. It does not currently alter the code pathes in any way, that's true.
But I've seen conditions where chips return bogus data, and it's
probably better to let the probing fail in that case so there won't be
any more conversation to that chip.
Wd could still drop that and just display the read ID. I don't mind
much, just considered that as one kind of sanity check.
> >+static int ulpi_set_vbus(struct otg_transceiver *otg, bool on)
> >+{
> >+ unsigned int flags = otg_io_read(otg, ULPI_OTGCTL);
> >+
> >+ flags &= ~(DRV_VBUS | DRV_VBUS_EXT);
> >+
> >+ if (on) {
> >+ if (otg->flags & USB_OTG_DRV_VBUS)
>
> Why this flag does even exist? If the driver is commanded to set
> VBUS it should just do so...
Because the hardware design could require to not let the chip use its
own voltage generators. The EHCI driver calls ulpi_set_vbus() upon
initialisation, and if the board support code did not set the flag to
make the chip use its own booters, this function becomes a noop.
I though about passing this option as flag to the EHCI driver, but I
liked the current approach more because it sets up the flags for the
transceiver at a point where the transceiver driver instance is created.
And IMO, this is just where it belongs to.
The problem is the variety of different applications where such chips
are found, and so the interface should be versatile enough to cover
such cases.
> >+ flags |= DRV_VBUS;
> >+
> >+ if (otg->flags & USB_OTG_DRV_VBUS_EXT)
> >+ flags |= DRV_VBUS_EXT;
>
> Why not do this selection in ulpi_set_flags()?
I see no reason to factor that out, especially because the flags are not
likely to be changed during operation. They just describe how the chip
is used within a certain hardware design.
Does that make more sense now?
Daniel
--
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
