Re: [PATCHv5 7/7] usb: dwc2: Update Kconfig to support dual-role

[Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>]

Hi,

On Monday, October 20, 2014 01:52:06 PM dinguyen@opensource.altera.com wrote:
> From: Dinh Nguyen <dinguyen@opensource.altera.com>
> 
> Update DWC2 kconfig and makefile to support dual-role mode. The platform
> file will always get compiled for the case where the controller is directly
> connected to the CPU. So for loadable modules, dwc2.ko is built for host,
> peripheral, and dual-role mode. The PCI bus interface will be called
> dwc2_pci.ko and the platform interface module will be called dwc2_platform.ko.
> 
> Signed-off-by: Dinh Nguyen <dinguyen@opensource.altera.com>
> Acked-by: Paul Zimmerman <paulz@synopsys.com>
> ---
> v5: dwc2.ko for all modes along with dwc2_platform.ko and dwc2_pci.ko will
>     get built for kernel modules.
> v3: Add USB_GADGET=y and USB_GADGET=USB_DWC2 for peripheral and dual-role
>     config options.
> v2: Remove reference to dwc2_gadget
> ---
>  drivers/usb/dwc2/Kconfig  | 61 ++++++++++++++++++++++++++++-------------------
>  drivers/usb/dwc2/Makefile | 32 ++++++++++++-------------
>  2 files changed, 53 insertions(+), 40 deletions(-)
> 
> diff --git a/drivers/usb/dwc2/Kconfig b/drivers/usb/dwc2/Kconfig
> index f93807b..1ea702e 100644
> --- a/drivers/usb/dwc2/Kconfig
> +++ b/drivers/usb/dwc2/Kconfig
> @@ -1,5 +1,5 @@
>  config USB_DWC2
> -	bool "DesignWare USB2 DRD Core Support"
> +	tristate "DesignWare USB2 DRD Core Support"
>  	depends on USB
>  	help
>  	  Say Y here if your system has a Dual Role Hi-Speed USB
> @@ -10,31 +10,53 @@ config USB_DWC2
>  	  bus interface module (if you have a PCI bus system) will be
>  	  called dwc2_pci.ko, and the platform interface module (for
>  	  controllers directly connected to the CPU) will be called
> -	  dwc2_platform.ko. For gadget mode, there will be a single
> -	  module called dwc2_gadget.ko.
> -
> -	  NOTE: The s3c-hsotg driver is now renamed to dwc2_gadget. The
> -	  host and gadget drivers are still currently separate drivers.
> -	  There are plans to merge the dwc2_gadget driver with the dwc2
> -	  host driver in the near future to create a dual-role driver.
> +	  dwc2_platform.ko. For all modes(host, gadget and dual-role), there
> +	  will be a single module called dwc2.ko.
>  
>  if USB_DWC2
>  
> +choice
> +	bool "DWC2 Mode Selection"
> +	default USB_DWC2_DUAL_ROLE if (USB && USB_GADGET)
> +	default USB_DWC2_HOST if (USB && !USB_GADGET)
> +	default USB_DWC2_PERIPHERAL if (!USB && USB_GADGET)
> +
>  config USB_DWC2_HOST
> -	tristate "Host only mode"
> +	bool "Host only mode"
>  	depends on USB
>  	help
>  	  The Designware USB2.0 high-speed host controller
> -	  integrated into many SoCs.
> +	  integrated into many SoCs. Select this option if you want the
> +	  driver to operate in Host-only mode.
> +
> +comment "Gadget/Dual-role mode requires USB Gadget support to be enabled"
> +
> +config USB_DWC2_PERIPHERAL
> +	bool "Gadget only mode"
> +	depends on USB_GADGET=y || USB_GADGET=USB_DWC2
> +	help
> +	  The Designware USB2.0 high-speed gadget controller
> +	  integrated into many SoCs. Select this option if you want the
> +	  driver to operate in Peripheral-only mode. This option requires
> +	  USB_GADGET=y.
> +
> +config USB_DWC2_DUAL_ROLE
> +	bool "Dual Role mode"
> +	depends on (USB=y || USB=USB_DWC2) && (USB_GADGET=y || USB_GADGET=USB_DWC2)
> +	help
> +	  Select this option if you want the driver to work in a dual-role
> +	  mode. In this mode both host and gadget features are enabled, and
> +	  the role will be determined by the cable that gets plugged-in. This
> +	  option requires USB_GADGET=y.
> +endchoice
>  
>  config USB_DWC2_PLATFORM
>  	bool "DWC2 Platform"
> -	depends on USB_DWC2_HOST
>  	default USB_DWC2_HOST

It should be "default USB_DWC2_HOST || USB_DWC2_PERIPHERAL" because
USB_DWC2_PLATFORM replaces current USB_DWC2_PERIPHERAL functionality.

Additionaly USB_DWC2_PLATFORM should be changed to tristate
(USB_DWC2_PERIPHERAL was a tristeate before your changes).

BTW It is a bit late but it would be great if you could split your
patchset on two.  First one merging gadget functionality into
core/platform code and the second one adding USB_DWC2_DUAL_ROLE
functionality.

> -	help
> -	  The Designware USB2.0 platform interface module for
> -	  controllers directly connected to the CPU. This is only
> -	  used for host mode.
> +        default y
> +        help
> +          The Designware USB2.0 platform interface module for
> +          controllers directly connected to the CPU.
>  
>  config USB_DWC2_PCI
>  	bool "DWC2 PCI"
> @@ -44,15 +66,6 @@ config USB_DWC2_PCI
>  	  The Designware USB2.0 PCI interface module for controllers
>  	  connected to a PCI bus. This is only used for host mode.
>  
> -comment "Gadget mode requires USB Gadget support to be enabled"
> -
> -config USB_DWC2_PERIPHERAL
> -	tristate "Gadget only mode"
> -	depends on USB_GADGET
> -	help
> -	  The Designware USB2.0 high-speed gadget controller
> -	  integrated into many SoCs.
> -
>  config USB_DWC2_DEBUG
>  	bool "Enable Debugging Messages"
>  	help
> diff --git a/drivers/usb/dwc2/Makefile b/drivers/usb/dwc2/Makefile
> index b73d2a5..2175d93 100644
> --- a/drivers/usb/dwc2/Makefile
> +++ b/drivers/usb/dwc2/Makefile
> @@ -1,28 +1,28 @@
>  ccflags-$(CONFIG_USB_DWC2_DEBUG)	+= -DDEBUG
>  ccflags-$(CONFIG_USB_DWC2_VERBOSE)	+= -DVERBOSE_DEBUG
>  
> -obj-$(CONFIG_USB_DWC2_HOST)		+= dwc2.o
> +obj-$(CONFIG_USB_DWC2)			+= dwc2.o
>  dwc2-y					:= core.o core_intr.o
> -dwc2-y					+= hcd.o hcd_intr.o
> -dwc2-y					+= hcd_queue.o hcd_ddma.o
> +
> +ifneq ($(filter y,$(CONFIG_USB_DWC2_HOST) $(CONFIG_USB_DWC2_DUAL_ROLE)),)
> +	dwc2-y				+= hcd.o hcd_intr.o
> +	dwc2-y				+= hcd_queue.o hcd_ddma.o
> +endif
> +
> +ifneq ($(filter y,$(CONFIG_USB_DWC2_PERIPHERAL) $(CONFIG_USB_DWC2_DUAL_ROLE)),)
> +	dwc2-y       			+= gadget.o
> +endif
>  
>  # NOTE: The previous s3c-hsotg peripheral mode only driver has been moved to
>  # this location and renamed gadget.c. When building for dynamically linked
> -# modules, dwc2_gadget.ko will get built for peripheral mode. For host mode,
> -# the core module will be dwc2.ko, the PCI bus interface module will called
> -# dwc2_pci.ko and the platform interface module will be called dwc2_platform.ko.
> -# At present the host and gadget driver will be separate drivers, but there
> -# are plans in the near future to create a dual-role driver.
> +# modules, dwc2.ko will get built for host mode, peripheral mode, and dual-role
> +# mode. The PCI bus interface module will called dwc2_pci.ko and the platform
> +# interface module will be called dwc2_platform.ko.
>  
>  ifneq ($(CONFIG_USB_DWC2_PCI),)
> -	obj-$(CONFIG_USB_DWC2_HOST)	+= dwc2_pci.o
> +	obj-$(CONFIG_USB_DWC2)		+= dwc2_pci.o
>  	dwc2_pci-y			:= pci.o
>  endif
>  
> -ifneq ($(CONFIG_USB_DWC2_PLATFORM),)
> -	obj-$(CONFIG_USB_DWC2_HOST)	+= dwc2_platform.o
> -	dwc2_platform-y			:= platform.o
> -endif
> -
> -obj-$(CONFIG_USB_DWC2_PERIPHERAL)	+= dwc2_gadget.o
> -dwc2_gadget-y				:= gadget.o
> +obj-$(CONFIG_USB_DWC2)			+= dwc2_platform.o

Shouldn't it use CONFIG_USB_DWC2_PLATFORM instead of CONFIG_USB_DWC2?

> +dwc2_platform-y				:= platform.o
 
Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics