spi: spi_ll_stm32: Fix spi_slave case in transceive() #7687
Conversation
galak
added
bug
|
I realized that the git commit message was not clear at all. I re-formulated it and re-pushed the commit. |
drivers/spi/spi_ll_stm32.c
Outdated
| return ret; /* number of frames received */ | ||
| } | ||
| #endif | ||
|
|
There was a problem hiding this comment.
Actually, I would have rewritten how ret is handled here in transceive: removing the below test for logging error, and removing the ret ? -EIO : 0; to me, in case of error, ret should already have the right error code.
There was a problem hiding this comment.
Not sure I got your proposal.
For example ret == 0x40, may be generated by an OVR error or by a correct reception of 64 frames in slave mode. How can we distinguish between the two case w/o re-reading the SR in transceive()?
There was a problem hiding this comment.
you'll need to make the function returning errors giving directly the right error code with relevant changes depending on whether you are slave or master.
There was a problem hiding this comment.
I don't understand why this should be Slave specific, from API point of view Slave or Master transfers should be the same, with the same error return scheme.
ret != 0 means an error, ret = 0 mean you no error
why do you want the number of frames when an error occurs ?
There was a problem hiding this comment.
This is part of the SPI changes that happened recently. In slave mode: if an error occurs it will need to return a negative error code, but in case of success it will have to return the number of received frames.
Master mode is as before: 0 on success, an negative error code otherwise.
There was a problem hiding this comment.
As I commented few minutes ago after re-pushing the commit, the situation is now clean and aligned with other SPI controllers (like dw or intel), except when CONFIG_SPI_STM32_INTERRUPT is disabled. In that case I had to add a check that in all other cases is already done in spi_context_wait_for_completion () routine.
Question: do we really need the stm32 case with interrupt disabled?
There was a problem hiding this comment.
My bad, I missed the point, sorry
Codecov Report
@@ Coverage Diff @@
## master #7687 +/- ##
=======================================
Coverage 55.02% 55.02%
=======================================
Files 483 483
Lines 53969 53969
Branches 10501 10501
=======================================
Hits 29695 29695
Misses 19986 19986
Partials 4288 4288Continue to review full report at Codecov.
|
avisconti
force-pushed
the
spi-slave-fix
branch
2 times, most recently
from
2e34536 to
461a787
Compare
|
@tbursztyka |
drivers/spi/spi_ll_stm32.c
Outdated
| if (sr & SPI_STM32_ERR_MSK) { | ||
| SYS_LOG_ERR("%s: err=%d", __func__, | ||
| sr & SPI_STM32_ERR_MSK); | ||
| return(-EIO); |
There was a problem hiding this comment.
Add a space before (-EIO)
I'm changing it right now |
In SPI slave case the transceive should return either the negative
errno code in case of error or the number of frames received.
So, now:
1. the spi_stm32_get_err() routine already checks whether
the SPI cell got an error and returns -EIO in that case.
2. the transceive() routine always returns whatever the
spi_context_wait_for_completion() has returned, which
is either:
a. -EIO in case of error
b. 0 in spi_master ok case
c. the number of frames received in spi_slave ok case
Signed-off-by: Armando Visconti <[email protected]>
If we are in SPI slave context we need to check, before
exiting the transceive() routine, whether the ret value
really represents a I/O error or instead is the number
of frames received.
Signed-off-by: Armando Visconti [email protected]