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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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:
> > On Tue, May 22, 2012 at 05:08:55PM +0800, Richard Zhao wrote:
> > > On Tue, May 22, 2012 at 11:58:05AM +0300, Felipe Balbi wrote:
> > > > On Tue, May 22, 2012 at 04:47:27PM +0800, Richard Zhao wrote:
> > > > > On Tue, May 22, 2012 at 11:37:43AM +0300, Felipe Balbi wrote:
> > > > > > Hi,
> > > > > > 
> > > > > > On Tue, May 22, 2012 at 04:34:36PM +0800, Richard Zhao wrote:
> > > > > > > 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 <richard.zhao@xxxxxxxxxxxxx>
> > > > > > > ---
> > > > > > >  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

Attachment: signature.asc
Description: Digital signature


[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux