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 <[email protected]>
> ---
> 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.
thanks,
greg k-h
Hi,
On Tue, Jun 18, 2013 at 4:39 AM, Greg KH <[email protected]> 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 <[email protected]>
>> ---
>> 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?
thanks,
Yuan-Hsin
> thanks,
>
> greg k-h
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 <[email protected]> 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 <[email protected]>
> >> ---
> >> 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
Hi,
On Tue, Jun 18, 2013 at 11:07 AM, Sarah Sharp
<[email protected]> wrote:
> 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 <[email protected]> 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 <[email protected]>
>> >> ---
>> >> 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.
I was trying to use ehci-platform.c, anonymous union/struct, and quirk
flags to avoid copying EHCI code.
But there are too big incompatibilities between fotg210/fusbh200
controller and EHCI.
That's why Alan agreed that I could create a stand-alone driver for
fusbh200 host controller.
Since fotg210 and fusbh200 have the same issue, fotg210 hcd is
supposed to be stand-alone.
More details please refer to mail sequence
http://www.spinics.net/lists/linux-usb/msg83812.html
Thanks,
Yuan-Hsin
>
> Sarah Sharp
On Tue, 18 Jun 2013, Yuan-Hsin Chen wrote:
> > 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.
>
> I was trying to use ehci-platform.c, anonymous union/struct, and quirk
> flags to avoid copying EHCI code.
> But there are too big incompatibilities between fotg210/fusbh200
> controller and EHCI.
> That's why Alan agreed that I could create a stand-alone driver for
> fusbh200 host controller.
> Since fotg210 and fusbh200 have the same issue, fotg210 hcd is
> supposed to be stand-alone.
> More details please refer to mail sequence
> http://www.spinics.net/lists/linux-usb/msg83812.html
That's right. The patch's description mentions some of the
incompatibilities. In short, the Faraday controllers are a _very_
noncompliant EHCI variant. The changes needed to make ehci-hcd work
with them were too invasive IMO.
It's a shame, because so much of the code is the same. It makes you
want to go back and ask those Faraday engineers what they were thinking
of at the time.
Alan Stern
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 <[email protected]> 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 <[email protected]>
> >> ---
> >> 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?
At the very least, yes, you should. And those tiny numbers of errors do
not line up with the much larger number of errors in your patch
submission, so it isn't just a "copy and paste" issue here.
thanks,
greg k-h
On Tue, Jun 18, 2013 at 10:43:56AM -0400, Alan Stern wrote:
> On Tue, 18 Jun 2013, Yuan-Hsin Chen wrote:
>
> > > 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.
> >
> > I was trying to use ehci-platform.c, anonymous union/struct, and quirk
> > flags to avoid copying EHCI code.
> > But there are too big incompatibilities between fotg210/fusbh200
> > controller and EHCI.
> > That's why Alan agreed that I could create a stand-alone driver for
> > fusbh200 host controller.
> > Since fotg210 and fusbh200 have the same issue, fotg210 hcd is
> > supposed to be stand-alone.
> > More details please refer to mail sequence
> > http://www.spinics.net/lists/linux-usb/msg83812.html
>
> That's right. The patch's description mentions some of the
> incompatibilities. In short, the Faraday controllers are a _very_
> noncompliant EHCI variant. The changes needed to make ehci-hcd work
> with them were too invasive IMO.
>
> It's a shame, because so much of the code is the same. It makes you
> want to go back and ask those Faraday engineers what they were thinking
> of at the time.
Ah, ok, that's too bad. I understand why the code can't be refactored.
Sarah Sharp