Re: [PATCH 4/7] mfd: add MFD based flexcard driver

[Lee Jones <lee.jones@linaro.org>]

> > Wow, this is a huge patch which is going to be an insanely long,
> > laborious and complex task to review it all.
> This is a card with a "insane" amount of features:
> 
> 1) a Interrupt controller
> 2) a DMA engine
> 3) up to 8 CAN devices
> 4) up to 4 FlexRay devices
> 5) a dozen different trigger sources
> 6) timer
> 7) timestamp counter

Right. Even more reason for the the patch to be broken up into many.

> > Can you split this up into many patches?
> Not sure how to split this up into a smaller and workable series of patches.
> Maybe rip out the header and the DMA part into a separate patch.

I was thinking more along the lines of:

    Patch 1: Basic compilation support
     - Core driver Kconfig entry
     - Introduction of the core driver
       - Basic probe(): memory allocation, register as MFD device
       - Basic remove(): unregister as MFD device
    
    Patch 2: Basic PCI support
     - pci_enable_device()
     - pci_request_regions()
     - Memory mapping
    
    Patch 3:
     - fc_init_dma()
    
    Patch 4:
     - fc_init_uio()
    
    Patch 5:
     - fc_init_event()
    
    Patch 6:
     - fc_init_clk()
    
    Patch 7:
     - fc_init_bus()
    
    Patch 8 - 8+n: Start adding sysfs entries
    
    Patch 8+n+1: Basic IRQ support
    
    Etc etc etc

Remember, the smaller the patches, the more chance that they'll be
accepted. I already see quite a few issues with this patch. Breaking
it up will allow for more thorough review and finer grain feedback.

Issues I see after first glance:

  - Lack of Device Tree support
  - Lack of documentation for new sysfs entries
  - Different IRQs with same .name property
  - Memory allocations and mapping should be managed (devm_*)

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog