<div class="gmail_quote">Thanks again for the additional comments. I made the changes (comments below) and re-compiled with the extra programmers enabled.</div><div class="gmail_quote"><br></div><div class="gmail_quote">New version of the patch attached.</div>
<div class="gmail_quote">Signed-off-by: David Hendricks <<a href="mailto:dhendrix@google.com">dhendrix@google.com</a>></div><div class="gmail_quote"><br></div><div class="gmail_quote">On Sat, Jun 11, 2011 at 5:09 AM, Carl-Daniel Hailfinger <span dir="ltr"><<a href="mailto:c-d.hailfinger.devel.2006@gmx.net">c-d.hailfinger.devel.2006@gmx.net</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">Am 10.06.2011 23:55 schrieb David Hendricks:<br>
<div class="im">> As always, thanks for the thorough review! Comments in-line, and a revised<br>
> patch is attached.<br>
><br>
> New patch is:<br>
> Signed-off-by: David Hendricks <<a href="mailto:dhendrix@google.com">dhendrix@google.com</a>><br>
><br>
<br>
</div>Thanks, looks good. I found one compile issue:<br>
nicnatsemi.c:61:24: error: incompatible pointer types passing 'int<br>
(void)' to parameter of type 'int (*)(void *)'<br>
if (register_shutdown(nicnatsemi_shutdown, NULL))<br>
^~~~~~~~~~~~~~~~~~~<br>
In file included from nicnatsemi.c:24:<br>
./flash.h:43:29: note: passing argument to parameter 'function' here<br>
int register_shutdown(int (*function) (void *data), void *data);<br>
^<br>
1 error generated.<br>
make: *** [nicnatsemi.o] Fehler 1<br>
<br>
<br>
The compilation test was run like this:<br>
make CONFIG_ATAHPT=yes CONFIG_NICNATSEMI=yes CONFIG_DEDIPROG=yes<br>
CONFIG_PRINT_WIKI=yes<br></blockquote><div><br></div><div>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.</div>
<div><br></div><div>There was also a typo in dediprog.c that caused a compilation failure on my machine (maybe only a warning on others).</div><meta http-equiv="content-type" content="text/html; charset=utf-8"><div><br></div>
<div><meta http-equiv="content-type" content="text/html; charset=utf-8">On Sat, Jun 11, 2011 at 5:09 AM, Carl-Daniel Hailfinger <span dir="ltr"><<a href="mailto:c-d.hailfinger.devel.2006@gmx.net">c-d.hailfinger.devel.2006@gmx.net</a>></span> wrote:</div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;"><div class="im">> On Thu, Jun 9, 2011 at 5:33 PM, Carl-Daniel Hailfinger wrote:</div><div class="im">
><br>
>> To be honest, I don't know serprog well enough to say something useful<br>
>> about it. And the massive code move in serprog makes review a bit hard.<br>
>><br>
> Yeah, that one was quite a bit more painful than the others. Maybe I re-do<br>
> serprog using a forward declaration for serprog_shutdown() and leave the<br>
> rest as is? We can remove the forward declaration and move the code in a<br>
> follow-up patch to reduce diffs in this one.<br>
><br>
<br>
</div>Good idea.<br></blockquote><div><br></div><div>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.</div>
<div><br></div><div><meta http-equiv="content-type" content="text/html; charset=utf-8">On Sat, Jun 11, 2011 at 5:09 AM, Carl-Daniel Hailfinger <span dir="ltr"><<a href="mailto:c-d.hailfinger.devel.2006@gmx.net">c-d.hailfinger.devel.2006@gmx.net</a>></span> wrote:</div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;"><div class="im">Other comments:</div>
drkaiser and satasii are missing physunmap in shutdown.<br></blockquote><div><br></div><div>Fixed.</div><div><br></div><div><meta http-equiv="content-type" content="text/html; charset=utf-8">On Sat, Jun 11, 2011 at 5:09 AM, Carl-Daniel Hailfinger <span dir="ltr"><<a href="mailto:c-d.hailfinger.devel.2006@gmx.net">c-d.hailfinger.devel.2006@gmx.net</a>></span> wrote:</div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
nicintel_spi has the register_shutdown too early.<br>
nicintel will not unmap the nicintel_bar if mapping nicintel_control_bar<br>
fails (error case handling needs a second label which unmaps nicintel_bar).<br></blockquote><div><br></div><div>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.</div>
</div><br>-- <br>David Hendricks (dhendrix)<br>Systems Software Engineer, Google Inc.<br>