hi
before getting to the details i have to admit that i might not have grasped the whole concept. :/
there is spi_prettyprint_status_register called by various spi25.c probing functions which prints the generic return value of spi_read_status_register() and uses a switch to call different chip(familiy)-specific pretty prints. your patch removes the case for the amic chips (instead of adding one for the atmels). why?
then there is the printlock field of the flashchips which is called by probe_flash in flashrom.c after a chip was detected and was used so far for non-spi chips to print the locking status only. you add the new pretty print functions as printlock functions pointers to the flash chips. why?
to answer the two "why"s above: this patch seems to start migration from the spi25-specific spi_prettyprint_status_register to a more generic lockprint implementation (with a slightly changed semantic)? if so this may also be indicated in the changelog. it was not obvious at all to me.
not touched by your patch, but the comment above spi_prettyprint_status_register_bp3210 (and/or spi_prettyprint_status_register_welwip) should be changed.
uint8_t status;
status = spi_read_status_register(); msg_cdbg("Chip status register is %02x\n", status);
is copied all over the place. dunno if it is really worth it, but it could be refactored.
the same applies to:
msg_cdbg("Chip status register: Status Register Write Disable " "(SRWD) is %sset\n", (status & (1 << 7)) ? "" : "not ");
in a25.c (only).
it might by worth to add a comment to spi_prettyprint_status_register_amic_a25l05p that it is used for 10 and 20 too. redundant because that information can be gathered from flashchips.c though. the same is true for the a25l40p function and the 80, 16, 010, 020, 040, 080, 016 chips. maybe rename the function to something more generic? *amic_with_x_bp etc?
in spi_prettyprint_status_register_amic_a25l032: - the 7th bit is not called "SRWD" but "SRP0". if the srwd printing is not factored out this could/should(?) be corrected. - there should(?) be a fixme regarding the second status register. - it could be combined with a25lq032, because the 1. status register is equivalent, but since there is an additional bit in the 2. status register in the q version, it's better to keep them separate.
the word "randomly" in the fix me comment at the end of a25.c is misleading. the real reason why those chips may not be unlocked should be noted (i.e. the TB bit).
at25.c: the bp bits of the atmels should be refactored, but thats for later (iirc i have started this in my patch).
i am not sure about spi_prettyprint_status_register_atmel_at25_swp. the details of the "Read Sector Protection Registers" command may differ between the chips using this function. i did not find any proof of this, so this is ok for now.
i did not check the content of the disable_blockprotect functions because they are just moved.
personally i would like to see other function names about almost everywhere. having functions named after one single chip model that is used for a lot of other - even quite differently named - chips is bad imho. all in all it looks ack-able to me, differences of opinions and missing refactorings can be dealt with later: Acked-by: Stefan Tauner stefan.tauner@student.tuwien.ac.at