[coreboot-gerrit] Change in coreboot[master]: drivers/spi/acpi: Do not perform spi_setup in acpi path

[Furquan Shaikh (Code Review)]

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

Change subject: drivers/spi/acpi: Do not perform spi_setup in acpi path
......................................................................


Patch Set 1:

> > > You need to assign a bus speed in devicetree for eve.
> >
> > It's not just bus speed. There are more spi bus parameters -- clk
> > polarity, clk phase, cs polarity. Right now we are just using the
> > default, but SoC can override those.

 > What parameters were they using before? It would have just been these:

 > https://review.coreboot.org/#/c/19099/14/src/soc/intel/skylake/spi.c

 > One could always put in a callback to the mainboard for per peripheral configuration, but I don't see why you'd need to do anything since the values are the same.


> Additionally, you could make a default spi bus speed 500kHz if you didn't want to add devicetree.cb entries.


That is true. Instead of calling get_config in here, we can just put in the default values and only take in speed as input from devicetree. 

BTW, there is one more thing that can be done. Split the work done in spi_setup_slave into two functions:
1. spi_setup_slave: Only identifies the spi_ctrlr structure for the bus and setups spi_slave structure

2. spi_init_bus: Actually performs the initialization of SPI controller for a bus.

That way, callers who intend to actually use the bus can call spi_setup_slave -> spi_init_bus. And, callers like acpi who just need to read the config can call spi_setup_slave --> spi_get_config. Thoughts? Or is this making it unnecessarily complicated?

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ied38e2670784ee3317bb12e542666c224bd9e819
Gerrit-PatchSet: 1
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: Subrata Banik <subrata.banik at intel.com>
Gerrit-Reviewer: build bot (Jenkins)
Gerrit-HasComments: No