Re: [RFC PATCH] usb: makefile cleanup

matt mooney
2010-10-07T00:19:15Z

On 14:01 Wed 06 Oct , Michal Marek wrote:
> On 6.10.2010 09:51, matt mooney wrote:
> > For all modules, change <module>-objs to <module>-y; remove
> > if-statements and replace with lists using the kbuild idiom; move
> > flags to the top of the file; and fix alignment while trying to
> > maintain the original scheme in each file.
> >
> > None of the dependencies are modified.
> >
> > Signed-off-by: matt mooney <mfm@muteddisk.com>
> > ---
> >
> > So here is a sample cleanup patch; I am not posting it to greg-kh or
> > the rest of the necessary usb guys yet because I would like to know
> > what you guys think first.
>
> The elimination of conditionals in Makefiles is definitely worth it. Not
> sure about pure whitespace fixes, if the USB developers don't show
> interest, you can try pushing these through trivial@kernel.org. I have
> only one remark below:

I questioned this myself, but I concluded that whitespace fixes help with
readability (of course this is purely subjective).

> > diff --git a/drivers/usb/musb/Makefile b/drivers/usb/musb/Makefile
> > index 4fd29f8..b1f79ae 100644
> > --- a/drivers/usb/musb/Makefile
> > +++ b/drivers/usb/musb/Makefile
> > [...]
> > # the kconfig must guarantee that only one of the
> > # possible I/O schemes will be enabled at a time ...
> > @@ -54,24 +27,17 @@ endif
> > ifneq ($(CONFIG_MUSB_PIO_ONLY),y)
> >
> > ifeq ($(CONFIG_USB_INVENTRA_DMA),y)
> > - musb_hdrc-objs += musbhsdma.o
> > + musb_hdrc-y += musbhsdma.o
> >
> > else
> > ifeq ($(CONFIG_USB_TI_CPPI_DMA),y)
> > - musb_hdrc-objs += cppi_dma.o
> > + musb_hdrc-y += cppi_dma.o
> >
> > else
> > ifeq ($(CONFIG_USB_TUSB_OMAP_DMA),y)
> > - musb_hdrc-objs += tusb6010_omap.o
> > + musb_hdrc-y += tusb6010_omap.o
> >
> > endif
> > endif
> > endif
> > endif
>
> So this wasn't exactly elegant before and you are only changing *-objs
> to *-y. Looking at drivers/usb/musb/Kconfig, al the three USB_*_DMA
> depend on !MUSB_PIO_ONLY, so the outermost if statement can go away.
> Furthermore, the intent seems to be to only enable one of the four modes
> (MUSB_PIO_ONLY, USB_INVENTRA_DMA, USB_TI_CPPI_DMA or USB_TUSB_OMAP_DMA),
> so it is a perfect candidate for a "choice" group. Have a look e.g. at
> "PCI access mode" in arch/x86/Kconfig. Then, the Makefile part can be
> reduced to three lines without any ifs.

I thought this section was more of a Kconfig issue although I am not all that
familiar with Kconfig yet. After I look into it a little more and better
understand your advice, I will send a separate patch to fix this.

Thanks,
mfm