[PULL] dove: update to Marvell Dove LSP 5.1.1

[Brad Figg]

On 06/09/2010 07:52 PM, Eric Miao wrote:
>> 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).

Do the minimum amount of changes, (just fix the patch that causes the
compile issue). We don't want to create more aggravation for ourselves
down the road incorporating their subsequent patches.

As we continue to work with them, we will continue to educate them on
how to play nicely with others. I know they've got new people coming
up to speed and there is a learning curve.

As soon as you make your next pull request, I'll review right away and
we'll get the changes in.

Brad
-- 
Brad Figg brad.figg at canonical.com http://www.canonical.com