[PULL] dove: update to Marvell Dove LSP 5.1.1

Thu Jun 10 02:52:41 UTC 2010

> NAK
>
> Eric,
>
> I'm including my review of these changes along with the ones for LSP 5.2.1. It
> looks to me that one file won't even compile after a particular patch is made.
> It could be that it is a change to a driver which we don't care about but that
> doesn't say much for the overall quality of the patches. Also there is way too
> much just commenting out or #ifdefing out blocks of code.
>
> Note, I'm not NAKing for anything other than the fix that appears to break the
> code from compiling (Fix suspend/resume issues with GC600). If we were more
> strict with the arm branches I'd be NAKing for other reasons as well. When
> reviewing the changes you take from Marvel I think you should be pushing back
> on them harder.
>

Thanks Brad, for the intensive review of these patches, I know it's sort of
painful. Some of my comments below:

> Brad
>
> commit f28114ee3c97fd196b5cbd480d50b3f394b08681
> Author: Stefan Bader <stefan.bader at canonical.com>
>
>     UBUNTU: SAUCE: Convert file to propper newline
>
> # Type: NEW
>

This is the patch Stefan added to address the MSDOS CR/LF issue when applying
those patches. I guess the problem Stefan was having is that git-am tries to
remove the CR/LF automatically when applying patches. The latest git source,
provides an option of '--keep-cr' in git-am to avoid doing that, not sure if
it's helpful.

> commit 4bbc32e23946d1635c4d4f73e42262ef49957b64
> Author: Nicolas Pitre <nico at marvell.com>
>
>     ARM: fix highmem with VIPT cache and DMA
>
> # Type: Bug fix
> . Non-trivial patch, quite a few lines changed. Looks good.
>
> commit a08a859d4121d3294773b1e883b83c9c9fa558c6
> Author: Saeed Bishara <saeed at marvell.com>
>
>     dove: disable the 2G user/kernel space split and enable HIGHMEM as the highmem issue fixed
>
> # Type: Bug fix
> . Note: There are numerous configuration changes in this patch, not all dealing
>         with just the HIGHMEM issue.
> . I don't like this patch just due to all the different config changes happening.
>   However, since we don't use the default config files, maybe we don't care. But,
>   someone should look at the changes and see if any of them are ones that should
>   also be applied to our config.
>

Yeah, I kept this patch for completeness. And just as you said, so I introduced
another config sync patch and only those relevant changes were picked.

> commit 73d134b58880c507356c595376e0a9e1d371cc17
> Author: Saeed Bishara <saeed at marvell.com>
>
>     ubifs: fix hang bug when doing suspend to disk and rootfs is over ubifs
>
> # Type: Bug fix
> . Small patch. Looks good.
>
> commit d46b1c36836615e704978edaa48468585d520338
> Author: Brian Hsu <bhsu at marvell.com>
>
>     Fix issue:suspend/resume,TSC cannot work
>
> # Type: Bug fix with bug id.
> . This is a very poor comment for this patch.

Yeah, it's always a headache. I had been telling them to avoid having the
subject and comments in a single line when committing, now I seem to need
to educate them to have a proper comment :-/

> . There is a single call related to rt5610 in addition to all the calls related
>   to rt5611. Assume that is what is desired.
> . Moderately complex patch. Looks good as far as I can tell.
>
> commit 5b718b3d507eda384d992d8c5b20d6efd156b19f
> Author: hchu <hchu at marvell.com>
>
>     (1) Fix fail to display after resume. (2) Prepare works for KG2 Application mode
>
> # Type: Bug fix
> . Another poor comment for this patch.
> . Commented out line of code (hate that)
> . They need to run checkpatch on their patches. Wrong indentation.
> . Looks like two different things are fixed with this patch, it should have been
>   split up.

And I guess that's why the subject looks like that.

> . Moderately complex patch. Looks good.
>
> commit e69583f8cb5e2142b0880e5cdc46fba365289665
> Author: Green Wan <gwan at marvell.com>
>
>     Fix bug: Not clear color key enable bit in configure process
>
> # Type: Bug fix
> . Small patch. Looks good.
> . checkpatch (extra whitespace)
>
> commit 1e57a0f017d3bc39b6bbbccc6f5850cc33c59f7b
> Author: Green Wan <gwan at marvell.com>
>
>     Fix bug: read wrong register. This might cause problem in A0 chip. No effect for Y0/Y1/X0.
>
> # Type: Bug fix
> . Small patch. Looks good.
>
> commit eb02fa1b4cc01d3f913fb178b3d5214fcf8c6fa9
> Author: Peter Liao <pliao at marvell.com>
>
>     Fix X0 L2 way 7-4 been turned off and X0 WCO setting Wrong
>
> # Type: Bug fix
> . ARM asm code.
> . Could use a more detailed commit description of what is being done.
> . Small patch. Looks good.
>
> commit c68c2096e6e273666e244416ad81ac271f36fb58
> Author: Peter Liao <pliao at marvell.com>
>
>     Revise for X0 WCO setting
>
> # Type: Bug fix
> . Could use a more detailed commit description of what is being done.
> . Small patch. Looks good.
>
> commit d88b0cb97e4540a7762d24acdd8d36ae24594d68
> Author: Chao Deng <cdeng at marvell.com>
>
>     Fix suspend/resume issues with GC600
>
> # Type: Bug fix
> . This patch is "#if 0" several sections of code. This isn't very
>   acceptable. Another block of code is "#if 1 #else" which is just
>   silly.
> . Given the amount of code that has _really_ changed as a result
>   of adding some #if this is a non-trivial change and deserves
>   much more of a comment and closer review.
> . I don't think this code will even compile. Look at:
>   arch/arm/mach-dove/gc600_driver_dove/galcore_ko_src/hal/os/linux/kernel/PreComp.h
>   there is a missing '#' from the 'if' at the begining of line 48
> . Looks BAD!
>

Fixed in a second push.

> commit 4a5e5c59a45ba16b1b97bfdd30dbba4ad6308c46
> Author: Joseph Lo <jlo at marvell.com>
>
>     Fix: gpio driver can't use mpp0 as interrupt pin
>
> # Type: Bug fix
> . Small patch. Looks good.
>
> commit e8fa4802db90449f5d29fbfacc4679a164843713
> Author: stephen kou <dhkou at marvell.com>
>
>     Fix hardware cursor issue with suspend/resume
>
> # Type: Bug fix
> . Small patch. Looks good.
>
> commit 7e061c9f2c1adef6c1d3b2e5a2649d9fefcc3d46
> Author: Brian Hsu <bhsu at marvell.com>
>
>     Fixed audio recording failed after suspend/resume
>
> # Type: Bug fix
> . Contains more commented out code. (dead code)
> . Non-trivial fix. Looks good.
>
> commit db051d9bd5804006229fe79e673a2c327a6911e0
> Author: Chao Deng <cdeng at marvell.com>
>
>     Fix Power Button event reverse error
>
> # Type: Bug fix
> . Small fix. Looks good.
>
> commit 3d2d769e6092f873ba03d3ffc30dc65832772d9d
> Author: Chao Deng <cdeng at marvell.com>
>
>     Fix GC600 driver crash when entering hibernation
>
> # Type: Bug fix
> . Code that was "#if 0" now "#if 1"
> . Non-trivial fix. Looks ok as far as I can tell.
>
> commit 191d3ad543c66bbe69c61877637decdf3653bfa2
> Author: Eric Miao <eric.miao at canonical.com>
>
>     UBUNTU: [Config]: sync with Dove LSP 5.1.0
>
> # Type: Bug fix
> . Configuration change.
> . Small fix that matches up with the earlier HIGHMEM fix. Looks good.
>
> commit 81ba28a71646e07704559f1f0e6ab0da4fc37f43
> Author: Saeed Bishara <saeed at marvell.com>
>
>     dove pm: restore cp15 Marvell Auxiliary Function and PMC register
>
> # Type: Bug fix
> . ARM asm code.
> . Small fix. Looks good.
>
> commit 6e634cce202546b373d3bfbae0f41fc6c8c43e3c
> Author: Saeed Bishara <saeed at marvell.com>
>
>     dove: fix build issues when MV_HAL_DRIVERS_SUPPORT disabled
>
> # Type: Bug fix
> . Small fix. Looks good.
>
> commit e65d7ba401bdc253011f428a12267697237e8d73
> Author: Saeed Bishara <saeed at marvell.com>
>
>     dove: register the usb as host mode when HAL drivers disabled
>
> # Type: Bug fix
> . Small fix. Looks good.
>
> commit d72f4b9161920761e552e9c0bfde0192d8e44bf6
> Author: Chao Deng <cdeng at marvell.com>
>
>     Fix wrong stride issue for RGB565 on overlay
>
> # Type: Bug fix
> . Small fix. Looks good.
>
> commit 8a5db89a5ebd5bbc84e647dcd4ebc39c7312d29f
> Author: Green Wan <gwan at marvell.com>
>
>     Refine alogrithm of filling yuv pitch length and do correction when input pitch value <= 0.
>
> # Type: Unknown
> . Non-trivial patch. Assuming it is correct.
>
> commit 6174661e2f7e5bc3b612281787774d23e093cf08
> Author: Tawfik Bayouk <tawfik at marvell.com>
>
>     Nand: fix PIO support for 8bit devices.
>
> # Type: Bug fix
> . Smallish patch. Looks good.
>
> commit 774b0311e2f74c8201e60c13b99a0c3156c6f959
> Author: Joseph Lo <jlo at marvell.com>
>
>     MRVL BMM module update
>
> # Type: Unknown
> . The comment gives no indication what this patch is about.
> . Non-trivial patch. Marvel only driver. Assume its correct.
>
> commit 1e5cc92d8cacc61ca8d9ee84864d217f5194e783
> Author: Joseph Lo <jlo at marvell.com>
>
>     BMM module: remove ioremap_xxx for kernel space VA mapping
>
> # Type: Bug fix
> . More code commented out.
> . Small patch. Looks ok.
>
> commit a770c097ab5d9aaec9ad71299c3fe4a4bd93498e
> Author: Ethan Ku <eku at marvell.com>
>
>     fix spi flash type to mx2513205d for AVD1 rev2
>
> # Type: Bug fix
> . Small patch. Looks good.
>
> commit dfbf7d6df9ac9e8381eb5fa804c64f99771c9228
> Author: stephenkou <stephenkou at stephenkou-desktop.(none)>
>
>     Fix colorkey issue with PM
>
> # Type: Bug fix
> . Small patch. Looks good.
>
>
>

So Brad, I'd like to know what is your opinion what we can do now to
this (coding style issues, MSDOS CR/LF, poor commenting, ...)?

1. accept it as is? (so subsequent patches can still apply)
2. or get them right
3. some other way around

Saeed,

I guess you need to educate them more on how to make a proper patch.
(and first rule, if there is some one out there using Source Insight
please tell him not to, guess that's where the cr*p all came from).