[coreboot-gerrit] Change in coreboot[master]: drivers/spi: Add flash probe callbacks to spi_ctrlr structure

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/19708 )

Change subject: drivers/spi: Add flash probe callbacks to spi_ctrlr structure
......................................................................


Patch Set 4:

(1 comment)

https://review.coreboot.org/#/c/19708/4/src/include/spi-generic.h
File src/include/spi-generic.h:

Line 132: 	bool (*try_generic_probe_first)(void);
> This is specific to spi flash. It should probably be named accordingly.
That is exactly what I wanted to do. Basically have a spi flash controller structure that would be pointed to by both spi_ctrlr and spi_flash. But I ran into following problems:

1. spi_ctrlr:

I thought it was getting really messy while reading the code with:
if (spi->ctrlr->flash_ctrlr && spi->ctrlr->flash_ctrlr->probe && !spi->ctrlr->flash_ctrlr->try_generic_probing && spi->ctrlr->flash_ctrlr->probe(..) != 0)

But I like the way you have put it. Basically, assume that if the pointer to spi_flash_controller is provided, then probe is present. That way it would be easier to read as well. And the good thing is I don't need to put the flash controller structure in spi-generic.h. It can live in spi_flash.h

2. spi_flash:
I had almost made this entire change last night, but then I realized there is just one particular vendor driver (sst) that decides which write function to use based on the idcode. And it has a separate write function only for one particular idcode. And so, the spi_flash_controller cannot be made const.

But as I am writing this, I realize, that we can have two spi_flash_controller structures in the sst driver and let it assign the correct one in spi_flash_probe based on idcode. That way we can throw all the spi flash controller ops in one separate structure.

Thanks for the ideas. I think I can improve this better than what is done right now :).


-- 
To view, visit https://review.coreboot.org/19708
To unsubscribe, visit https://review.coreboot.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I35f3bd8ddc5e71515df3ef0c1c4b1a68ee56bf4b
Gerrit-PatchSet: 4
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Owner: Furquan Shaikh <furquan at google.com>
Gerrit-Reviewer: Aaron Durbin <adurbin at chromium.org>
Gerrit-Reviewer: Duncan Laurie <dlaurie at chromium.org>
Gerrit-Reviewer: Furquan Shaikh <furquan at google.com>
Gerrit-Reviewer: Paul Menzel <paulepanter at users.sourceforge.net>
Gerrit-Reviewer: build bot (Jenkins) <no-reply at coreboot.org>
Gerrit-HasComments: Yes