[flashrom] [PATCH] Lock printing for AMIC A25* and Atmel AT25*

[Stefan Tauner]

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 at student.tuwien.ac.at>
-- 
Kind regards/Mit freundlichen Grüßen, Stefan Tauner