On Fri, 31 Aug 2012 02:18:33 +0200 Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
I might have missed something, but right now I hope the review is complete.
you missed nothing, but the point ;)
we need to decide what we want to achieve with this patch. atm it is neither fish nor fowl. we need to decide if we want to add erase (and write) support in general «A» (for example by adding rather safe, common chip-wide erasers), and by letting the user define them «B». If we want to require any brick-like parameter before attempting any action (or just erases/writes) «C».
i think «A» would be a good idea and justified if we include a force parameter and appropriate warning. i do understand your concerns regarding «B» (that if at all it would make more sense to add a way to completely specify a chip without recompilation, and that this could have rather unpleasant consequences (e.g wrong definitions floating around, or less contribution towards upstream)) and agree that we should not add such a thing to probe_spi_automagic_res1. i can not remember if you ever answered «C» in our chats yet. imho it makes sense for erase/write, but spi reads shouldn't be harmful... OTOH there are the AT45DB chips that interpret a block eraser opcode as read status register... maybe there are some that interpret 0x03 as chip erase... who knows :)
apart from this major decisions there are also some other points, which you have partially addressed in the review:
- .tested status field: it is probably a good idea to add our own warning like we do in the SFDP code, i.e. we should set it to TESTED_OK_PREW and think about the message content (which relies on the outcome of the other discussion points).
- order of flashchips.c entries: while it is true that the RES2 and REMS probing is of high quality regarding unique identification, the question is what is more beneficial to the user? i argue that the user has little benefit from knowing the exact IDs in comparison to be able to at least read it. hence i would rather move the automagic chip on top of the three (RES1, RES2, REMS). why not before all RDID chips? if RDID works, but the chip is not there yet, it should be. this is not true with the other methods with our current infrastructure.
- testing for RES2, REMS and RDID inside the automagic probe function: this practice is in general awful and stems from the inferior probing mechanism that is in place atm. due to this it makes some* sense in the current implementation of probe_spi_res1. it does not in the automagic method: either we want it to match even if the other generic methods (of higher quality but less use) match as stated above, or it does not get called at all because there was a match before anyway.
* actually i think it does only make sense if there are no non-generic chips in flashchips.c that have multiple entries to use RDID (or REMS or RES2) and RES1. currently there are 3 such chips of the ST M25P* family. btw did we ever try to contact ST/Numonyx/Micro/whoever to make sure the M25P05 and M25P10 really have the same RES ID?