-
Notifications
You must be signed in to change notification settings - Fork 6.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
drivers: spi: STM32 half-duplex master SPI support #46803
Conversation
drivers/spi/spi_ll_stm32.c
Outdated
*/ | ||
static int spi_stm32_shift_frames(SPI_TypeDef *spi, struct spi_stm32_data *data) | ||
{ | ||
uint16_t operation = data->ctx.config->operation; | ||
|
||
if (SPI_OP_MODE_GET(operation) == SPI_OP_MODE_MASTER) { | ||
spi_stm32_shift_m(spi, data); | ||
if (operation & SPI_HALF_DUPLEX) { | ||
spi_stm32_shift_m_half(spi, data); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about a bool half_duplex parameter in the spi_stm32_shift_m function ?
spi_stm32_shift_m(SPI_TypeDef *spi, struct spi_stm32_data *data, bool half_duplex)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wrote it the way I did to avoid making spi_stm32_shift_m
have a bunch of cases that do different things, but if that style is better then I'm not opposed to changing it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reusing spi_stm32_shift_m would indeed be more memory efficient.
Can you take this request into account ?
drivers/spi/spi_ll_stm32.c
Outdated
/* Shift a SPI frame as half-duplex master. */ | ||
static void spi_stm32_shift_m_half(SPI_TypeDef *spi, struct spi_stm32_data* data) { | ||
#if defined(CONFIG_SOC_SERIES_STM32MP1X) || defined(CONFIG_SOC_SERIES_STM32H7X) || \ | ||
defined(CONFIG_SOC_SERIES_STM32U5X) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
with line break:
#if defined(CONFIG_SOC_SERIES_STM32MP1X) || \
defined(CONFIG_SOC_SERIES_STM32H7X) || \
defined(CONFIG_SOC_SERIES_STM32U5X)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use "st,stm32h7-spi", cf #46852 which was merged recently
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question about the formatting - is there a particular clang-format
version that's needed, because the zephyr .clang-format
file does not seem to agree with a lot of the formatting in this file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't run clang-format on all files.
And for now we're still discussing about the rules we want to apply. See #cleanup channel on discord.
ee21289
to
2d1c649
Compare
if (LL_SPI_GetMode(spi) == LL_SPI_MODE_MASTER) { | ||
LL_SPI_StartMasterTransfer(spi); | ||
while (!LL_SPI_IsActiveMasterTransfer(spi)) { | ||
/* NOP */ | ||
if (LL_SPI_GetMode(spi) == LL_SPI_MODE_MASTER) { | ||
LL_SPI_StartMasterTransfer(spi); | ||
while (!LL_SPI_IsActiveMasterTransfer(spi)) { | ||
/* NOP */ | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you factorize half_duplex and full_duplex case a bit more ?
This exact piece of code seems to be exact same on full_duplex
@jefflongo Once you're done with code changes, can you have a look to these slides on how to format the PR and commit message: https://static.sched.com/hosted_files/zephyr2022/a3/HOWTO_Get_Your_Zephyr_Patches_Merged_ZDS_2022.pdf ? |
2d1c649
to
7c22e86
Compare
/* NOP */ | ||
} | ||
} | ||
spi_context_update_tx(&data->ctx, 1, 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think spi_context_update_tx need put to previous line.
because when spi transmit sucessed then update spi context.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And I think there is no need split half_duplex code and funll_fuplex code.
only add if ( half_duplex ) return;
after spi_context_update_tx.
/* NOP */ | ||
} | ||
} | ||
spi_context_update_tx(&data->ctx, 2, 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as 8bit process
uint8_t rx_frame = LL_SPI_ReceiveData8(spi); | ||
UNALIGNED_PUT(rx_frame, (uint8_t *)data->ctx.rx_buf); | ||
} | ||
spi_context_update_rx(&data->ctx, 1, 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as 8bit tx process
uint16_t rx_frame = LL_SPI_ReceiveData16(spi); | ||
UNALIGNED_PUT(rx_frame, (uint16_t *)data->ctx.rx_buf); | ||
} | ||
spi_context_update_rx(&data->ctx, 2, 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as 8bit tx process
@jefflongo Any plans to continue upstream process for this driver ? |
if (spi_context_tx_buf_on(&data->ctx)) { | ||
uint8_t tx_frame = UNALIGNED_GET((uint8_t *)(data->ctx.tx_buf)); | ||
LL_SPI_TransmitData8(spi, tx_frame); | ||
while (!ll_func_tx_is_empty(spi)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In spi interrupt mode, invoke spi_stm32_shift_m too.
We ended up switching to 4-wire SPI in the project I needed this for. I'm pretty busy at the moment so I probably won't get to updating this for awhile. If I catch a break I will work on updating this PR. |
This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time. |
Adds support for the
SPI_HALF_DUPLEX
flag to the STM32 SPI driver for an STM32 SPI master to use unidirectional or bidirection 3-wire SPI. Tested using an HMC7043.