Hi,

On 10/12/18 14:39, Pawel Laszczak wrote:

quoted

This patch introduce new Cadence USBSS DRD driver
to linux kernel.

The Cadence USBSS DRD Driver is a highly
configurable IP Core which can be
instantiated as Dual-Role Device (DRD),
Peripheral Only and Host Only (XHCI)
configurations.

The current driver has been validated with
FPGA burned. We have support for PCIe
bus, which is used on FPGA prototyping.

The host side of USBSS-DRD controller is compliance
with XHCI specification, so it works with
standard XHCI linux driver.

Signed-off-by: Pawel Laszczak <pawell@cadence.com>
---
 drivers/usb/Kconfig                |    2 +
 drivers/usb/Makefile               |    2 +
 drivers/usb/cdns3/Kconfig          |   44 +
 drivers/usb/cdns3/Makefile         |   16 +
 drivers/usb/cdns3/cdns3-pci-wrap.c |  157 +++
 drivers/usb/cdns3/core.c           |  451 +++++++
 drivers/usb/cdns3/core.h           |  108 ++
 drivers/usb/cdns3/debug.h          |  346 ++++++
 drivers/usb/cdns3/debugfs.c        |  168 +++
 drivers/usb/cdns3/drd.c            |  315 +++++
 drivers/usb/cdns3/drd.h            |  129 ++
 drivers/usb/cdns3/ep0.c            |  864 +++++++++++++
 drivers/usb/cdns3/gadget-export.h  |   28 +
 drivers/usb/cdns3/gadget.c         | 1802 ++++++++++++++++++++++++++++
 drivers/usb/cdns3/gadget.h         | 1177 ++++++++++++++++++
 drivers/usb/cdns3/host-export.h    |   28 +
 drivers/usb/cdns3/host.c           |   74 ++
 drivers/usb/cdns3/trace.c          |   11 +
 drivers/usb/cdns3/trace.h          |  343 ++++++

You went to the other extreme of combining everything (host/gadget/drd) together
which again makes this very hard to review.

I think what Felipe meant was to only combine the gadget driver code into one patch.

The series could be split into 6 patches like so.
-dt binding
-pci glue
-core driver
-host driver
-gadget driver
-drd driver

Felipe wrote:
" 
Frankly, I don't understand why this is a series. It's a single driver
and splitting it into a series just makes it more difficult to review,
actually.

Sure, a single patch will be large, but there's no way to have a
functional driver until all patches are applied, anyway.
"

Felipe should I split this driver as suggested by Roger ?. 
Now it's very big patch but it's still a single driver.

quoted

 19 files changed, 6065 insertions(+)
 create mode 100644 drivers/usb/cdns3/Kconfig
 create mode 100644 drivers/usb/cdns3/Makefile
 create mode 100644 drivers/usb/cdns3/cdns3-pci-wrap.c
 create mode 100644 drivers/usb/cdns3/core.c
 create mode 100644 drivers/usb/cdns3/core.h
 create mode 100644 drivers/usb/cdns3/debug.h
 create mode 100644 drivers/usb/cdns3/debugfs.c
 create mode 100644 drivers/usb/cdns3/drd.c
 create mode 100644 drivers/usb/cdns3/drd.h
 create mode 100644 drivers/usb/cdns3/ep0.c
 create mode 100644 drivers/usb/cdns3/gadget-export.h
 create mode 100644 drivers/usb/cdns3/gadget.c
 create mode 100644 drivers/usb/cdns3/gadget.h
 create mode 100644 drivers/usb/cdns3/host-export.h
 create mode 100644 drivers/usb/cdns3/host.c
 create mode 100644 drivers/usb/cdns3/trace.c
 create mode 100644 drivers/usb/cdns3/trace.h

<snip>

Cheers 
Pawel