Re: update for TRIZEPS4 modules

["eric miao" <eric.y.miao@xxxxxxxxx>]

On Thu, Jul 24, 2008 at 5:29 AM, Jürgen Schindele <linux@xxxxxxxxxxxxxx> wrote:> Am Mittwoch, 23. Juli 2008 23:12:12 schrieb Russell King - ARM Linux:>> On Wed, Jul 23, 2008 at 10:57:02PM +0200, Jürgen Schindele wrote:>> > hi folks,>> > i would like to submit an update for trizeps4 modules>> > The following work is included in this patch:>> >  - make use of MFP-ABI>> >  - support TRIZEPS4-WL module variant>> >  - adapt to changes till 2.6.24>>>> I gave up reading this patch when it became obvious that the entire>> file has had tabs converted to four spaces.>>>> Indentation uses tabs, not four spaces please.> I'am sorry about that i think my "VIM" config> is buggy. I'll rework that.
The default sample vim configuration file under/usr/share/vim/vimxx/vimrc_exampleworks for me most of the time, except that I don't like keep backupfiles, which makes my directory a mess. This is FYI.
And with a rough glimpse of the patch itself, I have thefollowing comments:
1. don't make characters number on a line exceeding 80, esp. thosecomments line
2. direct use of MFP_CFG_OUT() is not encouraged, please useGPIO46_GPIO,GPIO47_GPIO,
for the STUART LEDs instead, you then invoke gpio_direction_output()later to set this up.
3. what about those two: GPIO80_nCS_4, /* ??? */ and GPIO100_DREQ_2,where the former is unknown of its purpose, and the latter is mostlyused with external DMA transfer, and is probably incorrect here.
4. leave a space around operators
5. use IORESOURCE_IRQ_{HIGH,LOW}EDGE please, dm9000 drivershould be smart enough to convert that to IRQF_*
6. We now have dedicated PXA serial drivers, are those 8250 serialcode necessary and still valid, I doubt that.
7. Someone may want to configure GPIO_LEDS as a module, so youbetter use #if defined(CONFIG_GPIO_LEDS) || defined(CONFIG_GPIO_LEDS_MODULE), though it reads awkward
8. placing parentheses around constants is not necessary, this istrue for board_pcmcia_power()
9. I'd prefer to leave a comment about its status of unimplementedboard_mci_power() above the pxamci_platform_data declaration
10. Look other boards for the recent introduction ofpxa2xx_irda_transceiver_mode() for pin function switching betweenSTUART and IrDA, besides leaving a comment of FIXME: write valuesto the register does not sounds look a good idea to me, either writethe real register or put a comment explaining why that doesn't work.
11. I doubt the I2C_BOARD_INFO() + .type is a right way to go,I2C_BOARD_INFO() is used in simple cases where itself is enough,not intended to use as an structure initializer.
12. Remove trizeps4_mci_get_ro() when write protection detectionis not implemented.
13. remove trizeps4_ohci_exit() if its not doing anything useful
14. The ConXS should really be made more modular from codestructure, and the trizeps4_io_desc[] looks unnecessary to meunless those registers are needed early in the boot process,otherwise, ioremap() can just do the job
15. braces around single C statement is not necessary
16. machine type should really be decided at boot time beforethe kernel start, such dynamic assignment of machine type isjust ... I don't know, if only the board code needs to distinguishthis, maybe one internal static variable should do the job,instead of play tricks with __machine_arch_type, which I guessRussell will be unhappy about.
Get these fixed and be prepared for another round of review :-)
Thanks for the job.
>>>> Probably the same applies to your other patches.> Yes, shame on me :-(>> --> Jürgen Schindele> Linux for Trizeps Modules> www.schindele.name>> -------------------------------------------------------------------> List admin: http://lists.arm.linux.org.uk/mailman/listinfo/linux-arm-kernel> FAQ:        http://www.arm.linux.org.uk/mailinglists/faq.php> Etiquette:  http://www.arm.linux.org.uk/mailinglists/etiquette.php>


-- Cheers- eric-------------------------------------------------------------------List admin: http://lists.arm.linux.org.uk/mailman/listinfo/linux-arm-kernelFAQ:        http://www.arm.linux.org.uk/mailinglists/faq.phpEtiquette:  http://www.arm.linux.org.uk/mailinglists/etiquette.php