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.
Am 15.03.2011 16:29 schrieb Stefan Tauner:
Signed-off-by: Stefan Taunerstefan.tauner@student.tuwien.ac.at
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,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;
On Thu, 31 Mar 2011 08:35:38 +0200 Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
Am 15.03.2011 16:29 schrieb Stefan Tauner:
Signed-off-by: Stefan Taunerstefan.tauner@student.tuwien.ac.at
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 reallythe right way to do
* this, but the side effects of said functionwork 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.
yes the purpose is to remove the wrapper and comment it in a conspicuous place. commenting it _this way_ in the non-wrapped function (spi_disable_blockprotect_at25df) of course does not make sense. adding a comment to the called, shared function mentioning all calling functions _additionally_ to the comment above would be the best think imho. redundancy is not always bad.
.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.
all those pending patches are a pain. i know you know that. *shrug*
-
Carl-Daniel Hailfinger -
Stefan Tauner