[flashrom] [PATCH 1/8] whitespace, documentation and other small stuff

[Carl-Daniel Hailfinger]

Am 15.03.2011 16:29 schrieb Stefan Tauner:
> Signed-off-by: Stefan Tauner<stefan.tauner at student.tuwien.ac.at>
>    

Thanks for your patch!


> --- a/flash.h
> +++ b/flash.h
> @@ -107,7 +107,9 @@ struct flashchip {
>   	uint32_t manufacture_id;
>   	uint32_t model_id;
>
> +	/* Total chip size in kilobytes */
>    

You can't know that, but we have a pending patch which changes this to 
bytes. OTOH, until that patch is applied, your patch makes sense.


>   	int total_size;
> +	/* Page chip size in bytes */
>    

page_size will be renamed to max_write_size or something like that. 
Until then, a comment like yours makes sense. Please change it to "Chip 
page size...".


>   	int page_size;
>   	int feature_bits;
>
> @@ -125,7 +127,10 @@ struct flashchip {
>   	/*
>   	 * Erase blocks and associated erase function. Any chip erase function
>   	 * is stored as chip-sized virtual block together with said function.
> -	 */
> +	 * The first one that fits will be chosen, there is currently no way to
> +	 * influence that behaviour. For testing just comment out the others elements or
>    

s/others/other/
80 column limit please.


> +	 * set the function pointer to NULL.
> +	*/
>    

Missing blank before *.


>   	struct block_eraser {
>   		struct eraseblock{
>   			unsigned int size; /* Eraseblock size */
> --- a/spi25.c
> +++ b/spi25.c
> @@ -314,13 +314,14 @@ uint8_t spi_read_status_register(void)
>   /* Prettyprint the status register. Common definitions. */
>   static void spi_prettyprint_status_register_welwip(uint8_t status)
>   {
> -	msg_cdbg("Chip status register: Write Enable Latch (WEL) is "
> +	msg_cdbg("Chip status register: Write Enable Latch (WEL/WEN) is "
>   		     "%sset\n", (status&  (1<<  1)) ? "" : "not ");
> -	msg_cdbg("Chip status register: Write In Progress (WIP/BUSY) is "
> +	msg_cdbg("Chip status register: Write In Progress (WIP/BUSY/nRDY) is "
>   		     "%sset\n", (status&  (1<<  0)) ? "" : "not ");
>   }
>
> -/* Prettyprint the status register. Common definitions. */
> +/* Prettyprint the status register. Common definitions.
> + TODO: parameterize number of protected blocks. */
>    

Some chips do not use _common at all, and it may be a good idea to 
create a separate function for parameter printing. Hmmm... can you drop 
this change for now?

That said, I should repost my detailed locking patch which allows 
printing of locking areas as well.


>   static void spi_prettyprint_status_register_common(uint8_t status)
>   {
>   	msg_cdbg("Chip status register: Bit 5 / Block Protect 3 (BP3) is "
>    

Rest looks OK and will be committed immediately once I have a fixed up 
version.

Regards,
Carl-Daniel

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