NXP LPSPI: Fix native CS on non-DMA driver #82877
Conversation
decsny
force-pushed
the
feature/lpspi_enhancements_3
branch
6 times, most recently
from
6776d08 to
ca3b8ac
Compare
zephyrbot
added
area: Devicetree
platform: NXP Drivers
zephyrbot
requested review from
dbaluta,
dleach02,
galak,
manuargue,
mmahadevan108,
nashif,
rruuaanng,
tbursztyka,
teburd and
tejlmand
decsny
removed request for
dbaluta,
teburd,
nashif,
manuargue,
tbursztyka and
tejlmand
decsny
force-pushed
the
feature/lpspi_enhancements_3
branch
from
e606d19 to
5643ad4
Compare
|
rebased to fix conflicts |
|
@wearyzen, can you help look at the IRQ additions. |
|
@tbursztyka @teburd , can you help look at the changes to the SPI code. |
|
|
||
| if (!spi_context_tx_on(ctx) && lpspi_is_rx_done(dev)) { | ||
| spi_context_complete(ctx, dev, 0); | ||
| arch_irq_clear_pending(config->irqn); |
There was a problem hiding this comment.
For consistency I think you should use irq_* macros as we have here and use that here instead of calling arch_irq_*
There was a problem hiding this comment.
those macros don't exist for clearing pending, I added this arch_irq_clear_pending one in this PR, and didn't want to fiddle with adding generic APIs supported by all architectures just for one driver's use case
There was a problem hiding this comment.
Sorry for not making that clear, what I meant if we are adding an arch_irq_* () apis then we should also add a new irq_clear_pending() macro in generic layer to be consistent with how arch_* apis are used.
Also, I see some other code in drivers using NVIC_ClearPendingIRQ and NVIC_SetPendingIRQ so this is not the only use case.
You can chose to use the NVIC apis directly as others have used but if we add arch_irq_* apis then just defining these 3 seems incomplete. Ideally you would also need to define the arch_irq_* () to z_soc_irq_* () here so things don't break when there is a custom interrupt controller defined with CONFIG_ARM_CUSTOM_INTERRUPT_CONTROLLER.
There was a problem hiding this comment.
okay, in this case, I will just directly use those CMSIS functions without any zephyr wrapper, because I don't want to address the arch irq code really in this PR, thanks
decsny
force-pushed
the
feature/lpspi_enhancements_3
branch
from
5643ad4 to
cd5b959
Compare
|
updated to remove the commit touching the cortex m irq file, and use cmsis function directly instead, and rebased |
|
Looks like this PR also fixes #63280. |
Add properties for describing RX and TX fifo sizes. Also reformat some descriptions and fix the description of the transfer-delay property which was incorrect. Since zephyr spi bufs are not continuous, every possible Zephyr LPSPI driver must use continuous transfer mode, for which the meaning of this delay has nothing to do with the chip select. Signed-off-by: Declan Snyder <[email protected]>
Add functions to get remaining length in the transfer. Refactor a bit to avoid duplicate code for the for loop that is the same as in the total length function. The difference between the total length and left length function is that the current buffer total length is counted in the former and the current buffer remaining length is counted in the latter. Signed-off-by: Declan Snyder <[email protected]>
To fix the native hardware chip select, we need to rewrite this driver to not use the MCUX SDK handle abstraction, which does not fit the zephyr use case. Signed-off-by: Declan Snyder <[email protected]>
decsny
force-pushed
the
feature/lpspi_enhancements_3
branch
from
cd5b959 to
b633892
Compare
|
rebased to fix conflict with recently-enabled lpspi on mcxa156, and added a fix to an incorrect description of the transfer-delay property in the binding |
There was a problem hiding this comment.
Looks fine to me. I wouldn't bother implementing the transcieve_async API though as its not actually async (can block at any time, not usable from any context). To support rtio natively with this driver doesn't seem that far off, it requires using a different context with different functions to get the next read/write/transcieve and inform when the task is complete slightly different.
Have to rework this driver completely to fix the CS because the SDK handle is based on the idea of a transfer being a single buffer, so we can't use the SDK transfer function
depends on:
Fixes #16544
Fixes #63280