Thanks again for the additional comments. I made the changes (comments below) and re-compiled with the extra programmers enabled.
New version of the patch attached.
On Sat, Jun 11, 2011 at 5:09 AM, Carl-Daniel Hailfinger
<c-d.hailfinger.devel.2006@gmx.net> wrote:
Am 10.06.2011 23:55 schrieb David Hendricks:
> As always, thanks for the thorough review! Comments in-line, and a revised
> patch is attached.
>
> New patch is:
> Signed-off-by: David Hendricks <
dhendrix@google.com>
>
nicnatsemi.c:61:24: error: incompatible pointer types passing 'int
(void)' to parameter of type 'int (*)(void *)'
if (register_shutdown(nicnatsemi_shutdown, NULL))
^~~~~~~~~~~~~~~~~~~
In file included from nicnatsemi.c:24:
./flash.h:43:29: note: passing argument to parameter 'function' here
int register_shutdown(int (*function) (void *data), void *data);
^
1 error generated.
make: *** [nicnatsemi.o] Fehler 1
The compilation test was run like this:
make CONFIG_ATAHPT=yes CONFIG_NICNATSEMI=yes CONFIG_DEDIPROG=yes
CONFIG_PRINT_WIKI=yes
eek--i forgot there are targets being changed that are not included in the compilation by default. Basically I just forgot to update the nicnatsemi_shutdown() function to take a void *. This was a change from the second iteration, IIRC.
There was also a typo in dediprog.c that caused a compilation failure on my machine (maybe only a warning on others).
> On Thu, Jun 9, 2011 at 5:33 PM, Carl-Daniel Hailfinger wrote:
>
>> To be honest, I don't know serprog well enough to say something useful
>> about it. And the massive code move in serprog makes review a bit hard.
>>
> Yeah, that one was quite a bit more painful than the others. Maybe I re-do
> serprog using a forward declaration for serprog_shutdown() and leave the
> rest as is? We can remove the forward declaration and move the code in a
> follow-up patch to reduce diffs in this one.
>
Good idea.
Done. We now see that the changes to serprog.c are almost trivial. The insertion of register_shutdown() is done after some command parsing. The shutdown function is pretty simple; I don't think there's much to think about in terms of ordering dependency.
Other comments:
drkaiser and satasii are missing physunmap in shutdown.
Fixed.
nicintel_spi has the register_shutdown too early.
nicintel will not unmap the nicintel_bar if mapping nicintel_control_bar
fails (error case handling needs a second label which unmaps nicintel_bar).
Good catch -- I moved the register_shutdown call toward the bottom of the function, but before the bitbang_spi_init call so that failure in bitbang_spi_init will not prevent nicintel_spi_shutdown from being called.