On 13.08.2013 09:19, Takashi Iwai wrote:
Well, looking at rme96.c, the rme96 driver code can be cleaned up and lots of redundant checks can be reduced, then the sync-support patch will be the half size. For example, it's almost useless to check RME96_ISPLAYING() in trigger, or doing in close or prepare callback. (It's still valid to do it in snd_rme96_create(), though, since the register values are unknown at the first initialization state, though.)
But it's no serious problem, and we can live with the current code. So the only problem is the coding-style issues in your patch.
Ok, I admit that I never used scripts/checkpatch.pl ;-) Alsa, feeding the current rme96.c code to scripts/checkpatch.pl gives 235 errors and 205 warnings:
99 * ERROR: trailing whitespace 88 * ERROR: code indent should use tabs where possible 76 * WARNING: please, no spaces at the start of a line 72 * WARNING: line over 80 characters 39 * WARNING: braces {} are not necessary for single statement blocks 21 * ERROR: do not use assignment in if condition 18 * ERROR: that open brace { should be on the previous line 13 * WARNING: braces {} are not necessary for any arm of this statement 8 * ERROR: space required after that ',' (ctx:VxV) 3 * WARNING: quoted string split across lines 1 * ERROR: space prohibited after that '!' (ctx:BxW) 1 * WARNING: Use #include <linux/io.h> instead of <asm/io.h> 1 * WARNING: Avoid CamelCase: <snd_BUG>
Comments to the list of errors and warnings:
The CamelCase snd_BUG() isn´t a problem of rme96.c.
I don´t agree that code like "if ((err = pci_request_regions(pci, "RME96")) < 0)" is a problem.
I don´t agree with the script that lines over 80 characters are _always_ a problem.
Nevertheless, I really don´t like code alternating between different styles, that´s the reason for some unnecessary braces {} I used in the submitted patches.
So I suggest that I will (re)submit three patches: - first a patch cleaning the current rme96.c source - the PM and SYNC patches based on the first patch.
After that additional patches removing useless checks, etc could follow. But this should be patch 4(+) as it´s not always obvious for me if a check is really useless.
Do you agree?
cu, Knut