On Fri, 27 May 2016 03:12:22 +0530 Hatim Kanchwala hatim@hatimak.me wrote:
So I incorporated the changes you suggested. Please have a look at the updated attached file (output is here http://paste.flashrom.org/view.php?id=2919). IMO, the flashchip struct abstracts the characteristics or behaviour of the flashchip and not its state (like status register bits). For that reason, I have removed the array of int. The struct not has pointers to struct status_register_layout, like you pointed out. This is indeed a much more elegant solution as many chips will have a lot in common. It was stupid of me to go with the previous model. The two function pointers, read_status and write_status, will also be modeled like the existing printlock/prettyprint functions. A good change in the implementation due to these changes was that I no longer required that INVALID bit.
It might make sense to have all the status register-related function pointers in the model structs as well. If the status registers are alike then the functions are probably as well.
One disadvantage of your current approach might be that the status register pretty prints will be less expressive (in case we use your print_status_register function exclusively and get rid of all the various customized functions). Some of them are pretty complicated to be as generic as possible and convey way more information than a simple "is (not) set" semantic, e.g. spi_prettyprint_status_register_at45db. However, combining your approach with the existing expressiveness may lead to something quite complicated... I'll let you try to come up with something useful :)