Re: [PATCH v15 00/10] Add-Synopsys-DesignWare-HS-USB-OTG-driver

On Tue, Oct 25, 2011 at 2:54 PM, Tirumala Marri [off-list ref] wrote:

<
<Overall this driver seems to be based on the IP vendor driver? It
<looks like a completely flexible driver that implements all possible
<combinations of everything.
<
[Tirumala Marri] Some what true that it was based on skeletal driver
Provided from IP vendor.

Please use a real mail program instead of this [name] comment style.
It's hard to read.

<And as a result, it's huge, and it's got a lot of extra code in there
<that I'm
<willing to bet that you have never even executed on your platform.
<
<Please, pare it down to the portions that you have used, and know works
<and can support. If others need the extra functionality in the future,
<they can and will expand and bring in what is needed.
<
[Tirumala Marri] I can sure review and find if there are any dead
functions.
It may not be 100% free of dead code as some of the code paths may execut=

e

Asynchronously.

No, just start over from scratch. Just leave the crap driver behind,
use it for reference but write the new one.

It's obvious given that you are already at iteration v15 and it's
still looking this bad that this is not realistic to get reviewed and
accepted as-is. I don't think staging is a good target either -- what
the driver really needs is _functional_ cut-down to only cover the use
cases that your product uses, and staging cleanups are mostly around
style and refactoring, not changing, fixing or removing functionality.

<Compare this to the dwc3 driver, which is much much cleaner.
<
[Tirumala Marri] I will check.
<Overall other comments:
<
<* Register definitions are crazy long. It means you have to do lots of
<line
< =A0wraps to keep the 80-character limit, which makes it hard to read th=

e

<code.
[Tirumala Marri] This was suggestion from the review to use bit shifting.
I welcome any suggestions.

I'd be surprised if anyone asked to you that kind of crazy shifting.

<* The header files seem to have been autogenerated and have unnneeded
< =A0shift/mask operations.
<
<* It doesn't build on non-powerpc platforms since it uses out_{b,l}e
<accessors.
<
[Tirumala Marri] You have to select Little Endian mode for LE platform
From make menuconfig.

I don't think you understood what I meant. Try building an ARM config
with this driver enabled, for example, and you'll see that it breaks
the build.


-Olof