Thanks for the comments! I tend to agree with them all. FWIW, we've been testing the basic ideas here in the Chromium OS version and it seems to work pretty well. So at least I think we're on the right path.
A few details to address before making broader changes (and potentially making a huge mess of things)....
On Wed, Apr 27, 2011 at 7:56 AM, Carl-Daniel Hailfinger < c-d.hailfinger.devel.2006@gmx.net> wrote:
Am 22.04.2011 06:31 schrieb David Hendricks:
Index: internal.c
--- internal.c (revision 1288) +++ internal.c (working copy) @@ -164,6 +164,7 @@ } free(arg);
register_shutdown(internal_shutdown, NULL); get_io_perms();Wrong order. We want to register the shutdown function only after we got IO permissions. That said, should get_io_perms automatically register release_io_perms? This would kill a few useless shutdown functions.
Fixed the ordering, and I agree that get_io_perms should register release_io_perms.
On Wed, Apr 27, 2011 at 7:56 AM, Carl-Daniel Hailfinger < c-d.hailfinger.devel.2006@gmx.net> wrote:
It would be great if you could forward-port this patch to the current IT85*
SPI code.
Done. Also fixed the prototype thing Stefan pointed out.
On Wed, Apr 27, 2011 at 7:56 AM, Carl-Daniel Hailfinger < c-d.hailfinger.devel.2006@gmx.net> wrote:
Index: flashrom.c
=================================================================== --- flashrom.c (revision 1288) +++ flashrom.c (working copy) @@ -126,7 +126,8 @@ { .name = "internal", .init = internal_init,
.shutdown = internal_shutdown,
/* called implicitly using shutdown callback */+// .shutdown = internal_shutdown, .map_flash_region = physmap, .unmap_flash_region = physunmap, .chip_readb = internal_chip_readb,
The same for all other programmers, please. And kill the .shutdown struct member everywhere (even in the header file) while you're at it.
After giving this some thought, I think getting rid of .shutdown struct members might be premature. Perhaps they can be useful for something like register_shutdown(programmer.shutdown, data). Also, the structure looks a bit awkward without a shutdown member, even if it is superfluous.
If you still feel we should get rid of the .shutdown member, that's fine too. Otherwise we can just change the structure member so that it matches the function signature that register_shutdown() expects.
On Wed, Apr 27, 2011 at 7:56 AM, Carl-Daniel Hailfinger < c-d.hailfinger.devel.2006@gmx.net> wrote:
@@ -543,7 +544,7 @@
return ret;}
-int programmer_shutdown(void) +int flashrom_shutdown(void) { /* Registering shutdown functions is no longer allowed. */ may_register_shutdown = 0; @@ -551,7 +552,7 @@ int i = --shutdown_fn_count; shutdown_fn[i].func(shutdown_fn[i].data);
We should care about the result of the shutdown function, even if it is only for printing diagnostic messages.
In that case, the shutdown callbacks should return an int. Right now they return void. Should we go ahead and update the return type for shutdown functions?
On Wed, Apr 27, 2011 at 7:56 AM, Carl-Daniel Hailfinger < c-d.hailfinger.devel.2006@gmx.net> wrote:
}
return programmer_table[programmer].shutdown();
return 0;And return the error code of the failing shutdown function (if one failed), zero (if none failed) or some arbitrary value if more than one shutdown function failed.
That would be much more useful, but requires the above change.
On Wed, Apr 27, 2011 at 7:56 AM, Carl-Daniel Hailfinger < c-d.hailfinger.devel.2006@gmx.net> wrote:
Index: programmer.h
=================================================================== --- programmer.h (revision 1288) +++ programmer.h (working copy) @@ -111,7 +111,7 @@ extern const struct programmer_entry programmer_table[];
int programmer_init(char *param); -int programmer_shutdown(void); +int flashrom_shutdown(void);
Given that this is still a programmer shutdown (as opposed to a flash chip shutdown which may be necessary in the future), I'd like to keep the name.
Agreed.