Re: [PATCH 1/1] USB: Chipidea: Create a menu in Kconfig to choose driver bindings

On Tue, May 22, 2012 at 05:40:34PM +0800, Richard Zhao wrote:

On Tue, May 22, 2012 at 12:21:31PM +0300, Felipe Balbi wrote:

quoted

On Tue, May 22, 2012 at 05:08:55PM +0800, Richard Zhao wrote:

quoted

On Tue, May 22, 2012 at 11:58:05AM +0300, Felipe Balbi wrote:

quoted

On Tue, May 22, 2012 at 04:47:27PM +0800, Richard Zhao wrote:

quoted

On Tue, May 22, 2012 at 11:37:43AM +0300, Felipe Balbi wrote:

quoted

Hi,

On Tue, May 22, 2012 at 04:34:36PM +0800, Richard Zhao wrote:

quoted

It's more flexible to enable each driver binding, especially
when one driver binding is used by more than one platforms.

Signed-off-by: Richard Zhao <redacted>
---
 drivers/usb/chipidea/Kconfig  |   18 ++++++++++++++++++
 drivers/usb/chipidea/Makefile |    9 ++-------
 2 files changed, 20 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/chipidea/Kconfig b/drivers/usb/chipidea/Kconfig
index fd36dc8..91cf82b 100644
--- a/drivers/usb/chipidea/Kconfig
+++ b/drivers/usb/chipidea/Kconfig

@@ -29,4 +29,22 @@ config USB_CHIPIDEA_DEBUG
 	help
 	  Say Y here to enable debugging output of the ChipIdea driver.
 
+menu "ChipIdea driver bindings"
+
+config USB_CHIPIDEA_PCI
+	tristate "PCI ChipIdea USB controller"
+	default y if PCI
+	depends on PCI
+	help
+	  Say Y here to enable ChipIdea USB controller with PCI interface.
+
+config USB_CHIPIDEA_MSM
+	tristate "MSM ChipIdea USB controller"
+	default y if ARCH_MSM
+	depends on ARCH_MSM
+	help
+	  Say Y here to enable ChipIdea USB controller for Qualcomm MSM SoCs.
+
+endmenu
+
 endif

diff --git a/drivers/usb/chipidea/Makefile b/drivers/usb/chipidea/Makefile
index cc34937..74d8c60 100644
--- a/drivers/usb/chipidea/Makefile
+++ b/drivers/usb/chipidea/Makefile

@@ -5,10 +5,5 @@ ci_hdrc-$(CONFIG_USB_CHIPIDEA_UDC)	+= udc.o
 ci_hdrc-$(CONFIG_USB_CHIPIDEA_HOST)	+= host.o
 ci_hdrc-$(CONFIG_USB_CHIPIDEA_DEBUG)	+= debug.o
 
-ifneq ($(CONFIG_PCI),)
-	obj-$(CONFIG_USB_CHIPIDEA)	+= ci13xxx_pci.o
-endif
-
-ifneq ($(CONFIG_ARCH_MSM),)
-	obj-$(CONFIG_USB_CHIPIDEA)	+= ci13xxx_msm.o
-endif
+obj-$(CONFIG_USB_CHIPIDEA_PCI)		+= ci13xxx_pci.o
+obj-$(CONFIG_USB_CHIPIDEA_MSM)		+= ci13xxx_msm.o

no, this is not what I wanted. I want this to be always buildable, with
no need for an extra Kconfig option.

ifneq ($(CONFIG_ARCH_MXC),)
       obj-$(CONFIG_USB_CHIPIDEA)      += ci13xxx_imx.o
else
  ifneq ($(CONFIG_ARCH_MXS),)
         obj-$(CONFIG_USB_CHIPIDEA)    += ci13xxx_imx.o
  endif
endif

If you accept the above, that's ok.

Just remove the ARCH tests.

hmm.. I'm not sure it can pass build for x86 (probably PCI). Maybe
we can at least check CONFIG_ARM ?

PCI fails to compile unless CONFIG_PCI is enabled, as it doesn't provide
stubs. But that's the only thing that needs checking. I just tested this
myself:

diff --git a/drivers/usb/chipidea/Makefile b/drivers/usb/chipidea/Makefile
index cc34937..b69900a 100644
--- a/drivers/usb/chipidea/Makefile
+++ b/drivers/usb/chipidea/Makefile

@@ -5,10 +5,12 @@ ci_hdrc-$(CONFIG_USB_CHIPIDEA_UDC)	+= udc.o
 ci_hdrc-$(CONFIG_USB_CHIPIDEA_HOST)	+= host.o
 ci_hdrc-$(CONFIG_USB_CHIPIDEA_DEBUG)	+= debug.o
 
+# Glue/Bridge layers go here
+
+obj-$(CONFIG_USB_CHIPIDEA)	+= ci13xxx_msm.o
+
+# PCI doesn't provide stubs, need to check
 ifneq ($(CONFIG_PCI),)
 	obj-$(CONFIG_USB_CHIPIDEA)	+= ci13xxx_pci.o
 endif
 
-ifneq ($(CONFIG_ARCH_MSM),)
-	obj-$(CONFIG_USB_CHIPIDEA)	+= ci13xxx_msm.o
-endif

$ make -j8 drivers/usb/chipidea/
make[1]: Nothing to be done for `all'.
  CHK     include/linux/version.h
  CHK     include/generated/utsrelease.h
  CALL    scripts/checksyscalls.sh
  LD      drivers/usb/chipidea/built-in.o
  CC [M]  drivers/usb/chipidea/core.o
  CC [M]  drivers/usb/chipidea/udc.o
  CC [M]  drivers/usb/chipidea/host.o
  CC [M]  drivers/usb/chipidea/debug.o
  CC [M]  drivers/usb/chipidea/ci13xxx_msm.o
  CC [M]  drivers/usb/chipidea/ci13xxx_pci.o
  LD [M]  drivers/usb/chipidea/ci_hdrc.o

But it don't work for all driver. On x86:

  CC      drivers/usb/chipidea/ci13xxx_pci.o
  CC      drivers/usb/chipidea/ci13xxx_imx.o
  CC      drivers/tty/vt/vt_ioctl.o
In file included from drivers/usb/chipidea/ci13xxx_imx.c:15:0:
include/linux/of_platform.h:106:13: warning: ‘struct of_device_id’ declared inside parameter list [enabled by default]
include/linux/of_platform.h:106:13: warning: its scope is only this definition or declaration, which is probably not what you want [enabled by default]
include/linux/of_platform.h:106:13: warning: ‘struct device_node’ declared inside parameter list [enabled by default]
drivers/usb/chipidea/ci13xxx_imx.c: In function ‘ci13xxx_imx_probe’:
drivers/usb/chipidea/ci13xxx_imx.c:89:3: error: implicit declaration of function ‘of_find_device_by_node’ [-Werror=implicit-function-declaration]
drivers/usb/chipidea/ci13xxx_imx.c:89:12: warning: assignment makes pointer from integer without a cast [enabled by default]
cc1: some warnings being treated as errors

then, IMX depends on CONFIG_OF... so you check for CONFIG_OF, but don't
check for IMX. In fact, this is bigger bug on the whole OF
implementation because it defines a function outside of any ifdef
CONFIG_OF_DEVICE, but the prototype on the header is only available
ifdef CONFIG_OF_DEVICE.  Which is quite bogus... that needs to be fixed
ASAP.

-- 
balbi