On 15 November 2016 at 15:05, Jeremy Linton Jeremy.Linton@arm.com wrote:
|-----Original Message----- |From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org] |Sent: Tuesday, November 15, 2016 8:58 AM |To: Jeremy Linton |Cc: edk2-devel-01; Steve Capper; Leif Lindholm; ryan.harkin@linaro.org; |linaro-uefi |Subject: Re: [edk2] [PATCH 0/8] ATAPI support on SiI SATA adapter | |On 15 November 2016 at 14:54, Jeremy Linton jeremy.linton@arm.com |wrote: |> On 11/15/2016 01:43 AM, Ard Biesheuvel wrote: |>> |>> Hi Jeremy, |>> |>> On 14 November 2016 at 21:09, Jeremy Linton jeremy.linton@arm.com |wrote: |>>> |>>> The SiI isn't an AHCI compatible adapter so it implements the EFI |>>> ATA pass-through protocol directly. This works for fixed hard |>>> drives, but not ATAPI attached devices (CDROM, DVDROM, TAPE, etc). |>>> |>>> This patch adds read only ATAPI support via the EFI SCSI |>>> pass-through protocol, allowing boot from attached CD/DVD. This |>>> patch also cleans up, and tweaks recovery paths/etc in the original driver. |>> |>> |>> Very nice! Thanks for getting to the bottom of this. |>> |>> However, looking at the patches, they are riddled with coding style |>> violations. I am usually less strict than Leif when it comes to |>> upholding those, but these patches really need to be cleaned up to be |>> considered for merging. |> |> |> |> Is there a tool which can correct or at least point out the formatting |> errors? |> | |Yes, BaseTools/Scripts/PatchCheck.py
I did use that before submission, the only thing it complained about was an error caused by the git submodule commit id not having a CR (which AFAIK is bogus)
[jlinton@mammon-v1 edk2]$ python BaseTools/Scripts/PatchCheck.py -8 |more Checking git commit: 4a9a9d7 The commit message format passed all checks. The code passed all checks.
Checking git commit: 8537b6c The commit message format passed all checks. The code passed all checks.
Checking git commit: 458452e The commit message format passed all checks. The code passed all checks.
Checking git commit: ca0606f The commit message format passed all checks. Code format is not valid:
- Line ending ('\n') is not CRLF File: OpenPlatformPkg Line: Subproject commit a072474a3b05ec7f12f2d4db271cc07a0cf7a369
Checking git commit: 0d9942f The commit message format passed all checks. The code passed all checks.
Checking git commit: b516f7a The commit message format passed all checks. The code passed all checks.
Checking git commit: 2259641 The commit message format passed all checks. The code passed all checks.
Checking git commit: 61be9c2 The commit message format passed all checks. The code passed all checks.
Hmm, ok, that is strange.
What I spotted when going over the patches was lots of initialized stack variables, which the EDK2 coding style does not allow. Local variables should be initialized using assignments not initializers (for some reason, which I think may have to do with generated code size on some toolchains)
Other things to check for is spaces around '=' and the use of STATIC for things that are only referenced locally.
In any case, I will go through the patches again to comment in some more detail.
Thanks, Ard.