Re: [flashrom] [PATCH 2/8] The AT25F512B is quite different from the other (yet unsupported) chips in the AT25F* familiy, so rename 512B-specific stuff.

[Carl-Daniel Hailfinger]

Am 15.03.2011 16:29 schrieb Stefan Tauner:
> Signed-off-by: Stefan Tauner
>
> chipdrivers.h |    3 +--
>   flashchips.c  |    6 ++++--
>   spi25.c       |   10 +---------
>   3 files changed, 6 insertions(+), 13 deletions(-)
>
> diff --git a/chipdrivers.h b/chipdrivers.h
> index c01ab7a..dc46fe1 100644
> --- a/chipdrivers.h
> +++ b/chipdrivers.h
> @@ -45,13 +45,12 @@ int spi_chip_read(struct flashchip *flash, uint8_t *buf, int start, int len);
>   uint8_t spi_read_status_register(void);
>   int spi_prettyprint_status_register_at25df(struct flashchip *flash);
>   int spi_prettyprint_status_register_at25df_sec(struct flashchip *flash);
> -int spi_prettyprint_status_register_at25f(struct flashchip *flash);
> +int spi_prettyprint_status_register_at25f512b(struct flashchip *flash);
>   int spi_prettyprint_status_register_at25fs010(struct flashchip *flash);
>   int spi_prettyprint_status_register_at25fs040(struct flashchip *flash);
>   int spi_disable_blockprotect(struct flashchip *flash);
>   int spi_disable_blockprotect_at25df(struct flashchip *flash);
>   int spi_disable_blockprotect_at25df_sec(struct flashchip *flash);
> -int spi_disable_blockprotect_at25f(struct flashchip *flash);
>   int spi_disable_blockprotect_at25fs010(struct flashchip *flash);
>   int spi_disable_blockprotect_at25fs040(struct flashchip *flash);
>   int spi_byte_program(int addr, uint8_t databyte);
> diff --git a/flashchips.c b/flashchips.c
> index 753a094..29a4da0 100644
> --- a/flashchips.c
> +++ b/flashchips.c
> @@ -1612,8 +1612,10 @@ struct flashchip flashchips[] = {
>    			.block_erase = spi_block_erase_c7,
>    		}
>    	},
> .printlock	= spi_prettyprint_status_register_at25f,
> .unlock		= spi_disable_blockprotect_at25f,
> .printlock	= spi_prettyprint_status_register_at25f512b,
> /* spi_disable_blockprotect_at25df is not really the right way to do
>  * this, but the side effects of said function work here as well. */

For disabling block protection of SPI chips we have quite a few 
functions where the side effects work just fine, but the comments inside 
the function are not correct. Not sure if we have to list this in a 
comment here or rather at the top of this unlock function because it is 
used for multiple chips.
I see you just wanted to avoid the existing wrapper function and that 
sort of makes sense... I'm undecided here.
> .unlock		= spi_disable_blockprotect_at25df,
> .write		= spi_chip_write_256,
> .read		= spi_chip_read,
> },
>
> diff --git a/spi25.c b/spi25.c
> index c774032..5d73411 100644
> --- a/spi25.c
> +++ b/spi25.c
> @@ -394,7 +394,7 @@ int spi_prettyprint_status_register_at25df_sec(struct flashchip *flash)
>    return spi_prettyprint_status_register_at25df(flash);
>   }
>
> -int spi_prettyprint_status_register_at25f(struct flashchip *flash)
> +int spi_prettyprint_status_register_at25f512b(struct flashchip *flash)

_at25f was originally intended as generic version usable by more AT25* 
chips. I have a conflicting patch for this region, will repost it so we 
can discuss how to merge them.
> {
>    uint8_t status;
>
> @@ -1123,14 +1123,6 @@ int spi_disable_blockprotect_at25df_sec(struct flashchip *flash)
>    return spi_disable_blockprotect_at25df(flash);
>   }
>
> -int spi_disable_blockprotect_at25f(struct flashchip *flash)
> -{
> /* spi_disable_blockprotect_at25df is not really the right way to do
> * this, but the side effects of said function work here as well.
> */
> return spi_disable_blockprotect_at25df(flash);
>
> -}
> int spi_disable_blockprotect_at25fs010(struct flashchip *flash)
> {
>  uint8_t status;