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. Signed-off-by: David Hendricks dhendrix@google.com
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
Thanks, looks good. I found one compile issue: 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 Sat, Jun 11, 2011 at 5:09 AM, Carl-Daniel Hailfinger < c-d.hailfinger.devel.2006@gmx.net> wrote:
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.
On Sat, Jun 11, 2011 at 5:09 AM, Carl-Daniel Hailfinger < c-d.hailfinger.devel.2006@gmx.net> wrote:
Other comments: drkaiser and satasii are missing physunmap in shutdown.
Fixed.
On Sat, Jun 11, 2011 at 5:09 AM, Carl-Daniel Hailfinger < c-d.hailfinger.devel.2006@gmx.net> wrote:
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.