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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20130814094528.GC4046@lee--X1>
Date:	Wed, 14 Aug 2013 10:45:28 +0100
From:	Lee Jones <lee.jones@...aro.org>
To:	Benedikt Spranger <b.spranger@...utronix.de>
Cc:	netdev@...r.kernel.org,
	Alexander Frank <Alexander.Frank@...rspaecher.com>,
	Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
	Samuel Ortiz <sameo@...ux.intel.com>,
	Holger Dengler <dengler@...utronix.de>
Subject: Re: [PATCH 4/7] mfd: add MFD based flexcard driver

> > 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
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html