Hi Stefan,
thanks for the review.
Am 07.05.2011 19:38 schrieb Stefan Tauner:
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?
I want to separate probing and lock printing. I have a followup patch which kills that switch statement completely.
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?
That way I can call them when I want, not necessarily directly on probe. We also have to think of storing lock regions in a generic format somewhere (I have a patch for that, and Google has a patch for that as well), and starting from a lock printing function is the way I plan to handle this.
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.
Indeed, I should have mentioned the design goal in the changelog.
not touched by your patch, but the comment above spi_prettyprint_status_register_bp3210 (and/or spi_prettyprint_status_register_welwip) should be changed.
I think my followup patch does that because it takes care of the remaining code. Please remind me if said followup patch fails to handle this.
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.
I tried, but I could not find a clean way to handle it except a separate spi_read_status_register_and_print(). Not sure if that would be better.
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).
Hm yes. I was not sure whether moving a single line to a separate function is a good idea or not.
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?
Naming is odd, indeed. This has consistency reasons. In the past we usually picked family names where possible (e.g. functionname_a25, but for stuff like the various a25 sub-families picking a good name is hard, so I usually picked the smallest chip name which had the desired characteristics.
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.
This is a bigger question: Should we use the chip vendor naming for all bits, or only for bits which are neither BUSY or WRITE_DISABLE? Or should we use generic names everywhere? People will not care at all for lock bits once we can print lock regions instead of bit names.
- 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).
Will fix.
at25.c: the bp bits of the atmels should be refactored, but thats for later (iirc i have started this in my patch).
OK, I'll leave that unchanged.
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.
Same here. Usually chip commands are consistent in a subfamily.
i did not check the content of the disable_blockprotect functions because they are just moved.
OK.
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 Taunerstefan.tauner@student.tuwien.ac.at
Thanks, I'll check in my next iteration directly.
Regards, Carl-Daniel