[flashrom] [Patch] + RFC: Use shutdown callback mechanism to shutdown programmers

[Carl-Daniel Hailfinger]

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.


>
>   	/* Initialize PCI access for flash enables */
> @@ -264,11 +265,9 @@
>   #endif
>   }
>
> -int internal_shutdown(void)
> +void internal_shutdown(void *data)
>   {
>   	release_io_perms();
> -
> -	return 0;
>   }
>   #endif
>
> Index: it85spi.c
> ===================================================================
> --- it85spi.c	(revision 1288)
> +++ it85spi.c	(working copy)
> @@ -80,6 +80,8 @@
>   #define INDIRECT_WRITE(base, value) OUTB(value, (base) + 4)
>   #endif  /* LPC_IO */
>
> +void it85xx_shutdown(void *);
> +
>   #ifdef LPC_IO
>   unsigned int shm_io_base;
>   #endif
> @@ -308,6 +310,7 @@
>   	/* Set this as spi controller. */
>   	spi_controller = SPI_CONTROLLER_IT85XX;
>
> +	register_shutdown(it85xx_shutdown, NULL);
>   	return 0;
>   }
>
> @@ -349,11 +352,10 @@
>   	return ret;
>   }
>
> -int it85xx_shutdown(void)
> +void it85xx_shutdown(void *data)
>   {
>   	msg_pdbg("%s():%d\n", __func__, __LINE__);
>   	it85xx_exit_scratch_rom();
> -	return 0;
>   }
>
>   /* According to ITE 8502 document, the procedure to follow mode is following:
>    

It would be great if you could forward-port this patch to the current 
IT85* SPI code.


> 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.


> @@ -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.


>   	}
> -	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.


>   }
>
>   void *programmer_map_flash_region(const char *descr, unsigned long phys_addr,
> @@ -1939,6 +1940,6 @@
>   	free(oldcontents);
>   	free(newcontents);
>   out_nofree:
> -	programmer_shutdown();
> +	flashrom_shutdown();
>   	return ret;
>   }
> 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.

Overall, I like this patch. If you send a variant which converts all 
programmers and addresses the comments, I'll ack.

Regards,
Carl-Daniel

-- 
http://www.hailfinger.org/