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

[Carl-Daniel Hailfinger]

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 Tauner<stefan.tauner at student.tuwien.ac.at>
>    

Thanks, I'll check in my next iteration directly.

Regards,
Carl-Daniel

-- 
http://www.hailfinger.org/