Add SPI Mode Enum to LinuxSpiDriver #2097
Conversation
SPI Modes specify the clock polarity for each transaction. The default Fprime LinuxSpiDriver assumed SPI Mode 0. This should be generalized to account for SPI devices with different clock polarity and phases.
| @@ -59,7 +77,8 @@ namespace Drv { | |||
| //! Open device | |||
| bool open(NATIVE_INT_TYPE device, | |||
| NATIVE_INT_TYPE select, | |||
| SpiFrequency clock); | |||
| SpiFrequency clock, | |||
| SpiMode spiMode); | |||
There was a problem hiding this comment.
Should we add a default parameter here such that old code still works?
spiMode = SPI_MODE_0
| @@ -70,7 +70,8 @@ namespace Drv { | |||
|
|
|||
| bool LinuxSpiDriverComponentImpl::open(NATIVE_INT_TYPE device, | |||
| NATIVE_INT_TYPE select, | |||
| SpiFrequency clock) { | |||
| SpiFrequency clock, | |||
| SpiMode spiMode) { | |||
|
|
|||
There was a problem hiding this comment.
We should assert that spiMode is in the correct range:
FW_ASSERT(spiMode >= SPI_MODE_0 || spiMode <= SPI_MODE_3);
|
@timcanham want to look at this too? |
|
@mohitsingh999 I think the RPI error will be resolved by adding a default to that argument. |
|
@LeStarch Glad to look at it after @mohitsingh999 makes the changes. |
|
@LeStarch @timcanham I just made the changes to 1) Have a default SPI Mode parameter value and 2) Assert that the SPI Device mode is in range. Let me know if anything else needs fixing! |
|
@timcanham I'll leave you to do a final approval and run. |
| mode = SPI_MODE_3; | ||
| default: | ||
| //Assert if the device SPI Mode is not in the correct range | ||
| FW_ASSERT(spiMode >= SpiMode::SPI_MODE_0 || spiMode <= SpiMode::SPI_MODE_3); |
There was a problem hiding this comment.
You should just assert zero in this case.
FW_ASSERT(0, spiMode);
This will force-fail the assert and print the invalid mode.
There was a problem hiding this comment.
@LeStarch Made the fix
| mode = SPI_MODE_1; | ||
| break; | ||
| case SpiMode::SPI_MODE_2: | ||
| mode = SPI_MODE_2; |
There was a problem hiding this comment.
Just noticed! Missing break statements.
There was a problem hiding this comment.
Thanks! Just added the missing statements
| * on the falling clock edge(CPHA = 0) or if data is shifted out on the | ||
| * falling clock edge and sampled on the rising clock edge(CPHA=1) | ||
| * | ||
| * SPI Mode 0: (CPOL = 0, CPHA = 0) |
There was a problem hiding this comment.
I would take these SPI comments out and annotate the SpiMode enum with a description of what each means, since the "SPI Mode X" values are now hidden.
| U8 mode = SPI_MODE_0; // Mode 0 (CPOL = 0, CPHA = 0) | ||
|
|
||
| U8 mode; // Mode Select (CPOL = 0/1, CPHA = 0/1) | ||
| switch(spiMode) { |
Check warning
Code scanning / CodeQL
Unchecked function argument
* Add SPI Mode Enum to LinuxSpiDriver SPI Modes specify the clock polarity for each transaction. The default Fprime LinuxSpiDriver assumed SPI Mode 0. This should be generalized to account for SPI devices with different clock polarity and phases. * Add default spiMode parameter value, assert check to ensure SPI Mode is in range * Use switch statement to set SPI Mode when opening SPI Device * Force-fail assert and print SPI Mode for invalid SPI modes * Redefined SpiMode enum variables, added missing break statements * Update SpiMode enum comments for each variable
SPI Modes specify the clock polarity for each transaction. The default Fprime LinuxSpiDriver assumed SPI Mode 0. This should be generalized to account for SPI devices with different clock polarity and phases.
Change Description
Add a SPI Mode Enumeration to the LinuxSpiDriver Component Implementation, and include the SpiMode as a formal parameter in the
LinuxSpiDriverComponentImpl::open()function .Rationale
This allows for SPI devices with a different clock phase and polarity to be properly integrated with F-Prime, without having to modify the component implementation. Previously, the SPI Mode for any SPI device was assumed to be SPI mode 0. This implies a clock phase=0 and a clock polarity=0, which is not always true.
Testing/Review Recommendations
The implementation has been tested with SPI Mode 0 and SPI Mode 1 devices and works correctly.
Future Work
In addition to SPI Modes, the Linux SPI implementations include several additional parameters, such as
SPI_CS_HIGH,SPI_LSB_FIRST, and the bits per word for each SPI Transaction. These parameters generally do not change for most SPI devices, but it might make sense to give the user ability to configure these parameters when opening a SPI device in FPrime.