drivers: Clean up NXP MCUX LPSPI driver #78789
Conversation
zephyrbot
added
area: SPI
zephyrbot
requested review from
MarkWangChinese,
danieldegrasse,
dbaluta,
manuargue,
tbursztyka and
teburd
decsny
force-pushed
the
fix/lpspi_cleanup2
branch
from
581588c to
771de85
Compare
decsny
force-pushed
the
fix/lpspi_cleanup2
branch
from
771de85 to
89e2b49
Compare
decsny
force-pushed
the
fix/lpspi_cleanup2
branch
from
89e2b49 to
9e45af4
Compare
|
dropped the dma path cleanup commit for now because something after recent merged patch makes the dma transfers wrong by one byte, not sure why yet |
EmilioCBen
left a comment
EmilioCBen
left a comment
There was a problem hiding this comment.
Thank you for your effort on this PR,
the only concern I have is of those checks
I mentioned otherwise LGTM!
drivers/spi/spi_mcux_lpspi.c
Outdated
There was a problem hiding this comment.
Is the '!!' here required?
There was a problem hiding this comment.
probably not, I can remove that if I need to push again
There was a problem hiding this comment.
(non blocking) ping on this- looks like it could be removed
There was a problem hiding this comment.
it's how the spi_context code does these types of checks with returns, is why I did it
danieldegrasse
left a comment
danieldegrasse
left a comment
There was a problem hiding this comment.
Few non-blocking nits
drivers/spi/spi_mcux_lpspi.c
Outdated
There was a problem hiding this comment.
(non blocking) ping on this- looks like it could be removed
decsny
dismissed stale reviews from dleach02, danieldegrasse, and EmilioCBen
via
5cb58a6
|
found the bug in the DMA cleanup commit, re--added |
decsny
force-pushed
the
fix/lpspi_cleanup2
branch
from
5cb58a6 to
aec6af1
Compare
|
rebased and added change to fix RT platforms from upstream |
yvanderv
left a comment
yvanderv
left a comment
There was a problem hiding this comment.
Consider comments on the git commit messages of this PR
Organize #includes and #defines for less redundancy and more readability. Shorten some macros to avoid multi line statements. Add some comments. Signed-off-by: Declan Snyder <[email protected]>
Remove parent_dev from config struct, and simplify code for the definition and use of the irq config function. Signed-off-by: Declan Snyder <[email protected]>
Move init function to bottom of file by custom. Signed-off-by: Declan Snyder <[email protected]>
Do initializations in a more logical order, remove unnecessarily duplicated code, reorder stack variables to be in reverse christmas tree order. Signed-off-by: Declan Snyder <[email protected]>
Create helper functions for duplicated code related to checking for DMA devices, with readable names. Also remove unneeded function prototype. Signed-off-by: Declan Snyder <[email protected]>
Add closing comment for #ifdef and remove unnecessary indentation of an else block. Also move the transcieve_rtio function to be near the other rtio functions. And move function prototypes to top of file. Signed-off-by: Declan Snyder <[email protected]>
Small change to consolidate the amount of lines of code in the spi_mcux_isr by making a macro for the argument and removing a line of code that was commented out. Also move the ISR to be the first function in the file which is common in many other drivers, instead of randomly in the middle of the file. And move the isr callback to be next to the isr. Signed-off-by: Declan Snyder <[email protected]>
Group the actual API functions to be next to each other. Signed-off-by: Declan Snyder <[email protected]>
Move transceive funtions next to each other and clean up the wrapping of functions to eliminate the ifdefs. Signed-off-by: Declan Snyder <[email protected]>
This line doesn't need to be in multiple places. Signed-off-by: Declan Snyder <[email protected]>
Remove lines of code that are commented out. Signed-off-by: Declan Snyder <[email protected]>
Next packet function is way more complicated than it needs to be. Clean it up by simplifying the code and making readable helper functions. Signed-off-by: Declan Snyder <[email protected]>
All the master transfer seemed to use the same config flags, reuse with a macro. Signed-off-by: Declan Snyder <[email protected]>
Log the device name and status if the transfer could not start, same way as in non-RTIO path. Signed-off-by: Declan Snyder <[email protected]>
Remove unneeded prepare_start function since all uses of it are paired with the start function. Also, probably should not mess with chip select before deciding to do the transfer. And just return on some error like the rest of the driver does instead of assert. Signed-off-by: Declan Snyder <[email protected]>
Do all input validation etc BEFORE setting up configuration, instead of mixing these things everywhere. Also condense the formatting of the code. Signed-off-by: Declan Snyder <[email protected]>
Optimize DMA callback by cleaning up the code and storing less debug logging strings. Signed-off-by: Declan Snyder <[email protected]>
Clean up DMA path of code. Signed-off-by: Declan Snyder <[email protected]>
decsny
force-pushed
the
fix/lpspi_cleanup2
branch
from
aec6af1 to
bcb563b
Compare
|
updated commit titles and removed the |
drivers/spi/spi_mcux_lpspi.c
Outdated
| } | ||
| #endif | ||
| #else | ||
| #define lpspi_inst_has_dma(arg) arg != arg |
There was a problem hiding this comment.
Is the #else required? This function is only needed when CONFIG_SPI_MCUX_LPSPI_DMA is defined.
There was a problem hiding this comment.
yes otherwise there will be a build error
|
@decsny, how was this PR tested? |
i ran all combinations of rtio, async, dma configs on the spi loopback test on mcxn to see if it had the same behavior as before , but that was a while ago, and there was some rebases since then |
|
@tbursztyka , can you help review |
10 participants
Driver was totally unreadable and tangled up in all sorts of conditional compilations and duplicated code, this PR addresses it, and makes the paths way more obvious to facilitate making changes much easier
No major functional changes intended by this PR.
There are a lot of commits so that one merge conflict doesnt cause a giant problem when rebasing.