Re: [patch 2/4] Base support for the Cypress c67x00 multi-role USB controllers

At a quick read, this looks like the right kind of direction.

Chips this complex need their own driver subdirectory, they
can't go in either host or gadget (they'd need to be both!)
so that result is foredoomed.


On Thursday 26 April 2007, Peter Korsgaard wrote:
> 	Communication with the controller is done
> through the link control protocol (LCP) over either HPI, SPI or HSS;
> Sofar only HPI is supported.

Comments explaining HPI and HSS would be useful.  Does your
implementation strategy defer SPI and HSS till the whole thing
works?  It might be nice to flesh out at least one of the two
before things get too far along, to help avoid inadvertently
hard-wiring bad assumptions.

Your platform_data should for example say how the particular
platform works rather than assuming HPI; and there should be
some CONFIG_* variable saying whether to link in the HPI glue
or some other kind(s).  Default to HPI until some other glue
is working, of course.


> +#
> +# Cypress C67x00 USB controller
> +#
> +config USB_C67X00_DRV
> +       tristate "Cypress C67x00 support"
> +       default n
> +       help
> +         The Cypress C67x00 (EZ-Host/EZ-OTG) chips are dual-role
> +         host/peripheral USB controllers.
> +
> +         To compile this driver as a module, choose M here: the
> +         module will be called c67x00.
> +

Given that any dual-role controller can run in three modes,
I'm curious why you don't give config options here.  The
three mode being:  host-only, peripheral-only, and OTG.

That choice would naturally be part of this core patch;
the host patch would affect two roles, and the gadget
patch would also affect two roles.

Again, see how the musb_hdrc code handles this.  It's a bit
messy (Kconfig seems to glitch in some cases), but giving
customers the option of how to configure things makes a
difference with embedded configs that may not want to have
a few hundred KBytes of never-used kernel code...


> +       /* Fill in needed attributes */
> +       sie->pdev = platform_device_register_simple("c67x00_sie",
> +                                                   id++, NULL, 0);

This seems wrong.  Not only should drivers not create device
nodes themselves, but the only way this could be the right
way to fit into the driver model would be if each SIE were
its own host (or OTG) bus segment with independent driver.
In your case, one driver would handle each usb_hostN bus.

- Dave

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
[email protected]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel