Re: [PATCH] usb: host: Faraday fotg210-hcd driver

[Sarah Sharp <sarah.a.sharp@linux.intel.com>]

On Tue, Jun 18, 2013 at 10:42:09AM +0800, Yuan-Hsin Chen wrote:
> Hi,
> 
> On Tue, Jun 18, 2013 at 4:39 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
> > On Wed, Jun 05, 2013 at 05:15:43PM +0000, Yuan-Hsin Chen wrote:
> >> FOTG210 is an OTG controller which can be configured as an
> >> USB2.0 host. FOTG210 host is an ehci-like controller with
> >> some differences. First, register layout of FOTG210 is
> >> incompatible with EHCI. Furthermore, FOTG210 is lack of
> >> siTDs which means iTDs are used for both HS and FS ISO
> >> transfer.
> >>
> >> Signed-off-by: Yuan-Hsin Chen <yhchen@faraday-tech.com>
> >> ---
> >>  drivers/usb/Makefile           |    1 +
> >>  drivers/usb/host/Kconfig       |   12 +
> >>  drivers/usb/host/Makefile      |    1 +
> >>  drivers/usb/host/fotg210-hcd.c | 5967 ++++++++++++++++++++++++++++++++++++++++
> >>  drivers/usb/host/fotg210.h     |  746 +++++
> >>  5 files changed, 6727 insertions(+), 0 deletions(-)
> >>  create mode 100644 drivers/usb/host/fotg210-hcd.c
> >>  create mode 100644 drivers/usb/host/fotg210.h
> >
> > You obviously didn't even run this through checkpatch.pl, did you?
> >
> > $ ./scripts/checkpatch.pl --terse ../usb.mbox | tail -n 1
> > total: 138 errors, 618 warnings, 6742 lines checked
> >
> > Please fix all of these if you wish us to at least start reviewing the
> > patch.  Your internal Q/A should have caught this first, please be more
> > careful in the future.
> >
> 
> Actually I did run checkpatch.pl and found that almost all errors and
> warnings are from ehci core (ehci-hcd.c, ehci-hub.c and so on) where
> my driver borrowed most of code.
> 
> $ ./scripts/checkpatch.pl --terse -f drivers/usb/host/ehci-hub.c | tail -n 1
> total: 18 errors, 69 warnings, 1138 lines checked
> 
> $ ./scripts/checkpatch.pl --terse -f drivers/usb/host/ehci-hcd.c | tail -n 1
> total: 17 errors, 78 warnings, 1403 lines checked
> 
> So you're saying that I should fix them, is that right?

In that case, no, you should be figuring out how to refactor and reuse
the EHCI code instead of copying it straight into your driver.

Sarah Sharp