Hello.
Daniel Mack wrote:
Hi Sergei,
thanks for finding time to review these patches.
Oh, I actually have plenty of time now, just need more
concentration... :-)
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.
Hm...
+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.
You do have another bit for external VBUS generation, no?
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.
You mean that there's some circuitry external to an ULPI transceiver
that is used to control VBUS softwarily in this case?
+ 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?
Actually no. I still don't understand why a selection of
internal/external VBUS generation shoiudl belong here and not in
ulpi_set_flags as all ther fixed settings (some of which should not
really be such)...
Daniel
WBR, Sergei
--
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
