Re: [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

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