Skip to content
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.

Sign up for GitHub

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

Jump to bottom

NXP LPSPI: Minor refactor and tweaks #82650

Merged
merged 2 commits into from
Dec 14, 2024
Merged

NXP LPSPI: Minor refactor and tweaks #82650

merged 2 commits into from
Dec 14, 2024

Conversation

decsny
Copy link
Member

@decsny decsny commented Dec 6, 2024 •
edited
Loading

Continuing the cleanup of LPSPI driver, these are first few commits I have in my branch to prepare to make the real functional changes. Since the last PR which had no functional change took 4 months to merge, I am posting these commits ahead of any others to pipeline the review process

  • Fix the check of the word size to be more useful
    • check min frame size instead of max
    • check for min word size requirement
    • add a clarifying comment about what the word size represents in
      hardware since the nomenclature from zephyr does not match the nxp
      references
  • Add a clarifying comment about half duplex being supported by hardware
  • Add LPSPI_ namespace to defines
  • Change chip select error message to be more clear about the problem
  • Move the check of the clock device being ready to the lpspi init,
    instead of checking it every time on configure. It probably also makes
    more sense to not ready the lpspi device if the clock is not ready.
  • Move the bare-metal configuration of bit fields (like debug mode) AFTER the SDK Init call.
  • Return the proper error code if clock control call errors.
  • To facilitate changing this driver, decouple rtio from functions not specific to RTIO.
    This also requires moving the sdk driver handle creation outide of the configure call.
    An effect of this is we can stop initializing an unused sdk driver handle for the dma path.

@zephyrbot zephyrbot added area: SPI SPI bus platform: NXP Drivers NXP Semiconductors, drivers labels Dec 6, 2024
@decsny decsny force-pushed the feature/lpspi_enhancements_1 branch 2 times, most recently from e161fdc to f868ca3 Compare December 6, 2024 14:42
@decsny decsny requested a review from DerekSnell December 6, 2024 17:39
@decsny decsny force-pushed the feature/lpspi_enhancements_1 branch 2 times, most recently from 996c410 to ed2dcc8 Compare December 9, 2024 15:37
@decsny decsny marked this pull request as draft December 9, 2024 16:31
@decsny decsny force-pushed the feature/lpspi_enhancements_1 branch from ed2dcc8 to 1f2c371 Compare December 9, 2024 17:38
@decsny decsny marked this pull request as ready for review December 9, 2024 17:40
@decsny decsny mentioned this pull request Dec 9, 2024
Merged
yvanderv
yvanderv reviewed Dec 10, 2024
View reviewed changes
Copy link
Contributor

@yvanderv yvanderv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good commit messages.

drivers/spi/spi_mcux_lpspi.c
@@ -531,6 +525,22 @@ static int transceive_dma(const struct device *dev, const struct spi_config *spi
#endif /* CONFIG_SPI_MCUX_LPSPI_DMA */

#ifdef CONFIG_SPI_RTIO
static void spi_mcux_iodev_complete(const struct device *dev, int status);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is the prototype defined here? It does not seem necessary as there are no calls to this function until it's defined.

Copy link
Member Author

@decsny decsny Dec 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does not seem necessary as there are no calls to this function until it's defined

this is not true, it's at the end of iodev_start

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yvanderv just do a search and you will see that it is called in multiple places before the definition.

dleach02
dleach02 reviewed Dec 10, 2024
View reviewed changes
drivers/spi/spi_mcux_lpspi.c
@@ -226,7 +215,6 @@ static int spi_mcux_configure(const struct device *dev, const struct spi_config
master_config.pinCfg = config->data_pin_config;

LPSPI_MasterInit(base, &master_config, clock_freq);
LPSPI_MasterTransferCreateHandle(base, &data->handle, spi_mcux_master_callback, data);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor nit on commit message:

"creation outide of the configure call" -- outside instead of outide

@decsny decsny mentioned this pull request Dec 11, 2024
Merged
@decsny
spi_mcux_lpspi: More precise configuration checks
86f7e56 
These changes:
* Fix the check of the word size to be more useful
  - check min frame size instead of max
  - check for min word size requirement
  - add a clarifying comment about what the word size represents in
    hardware since the nomenclature from zephyr does not match the nxp
    references
* Add a clarifying comment about half duplex being supported by hardware
* Add LPSPI_ namespace to defines
* Change chip select error message to be more clear about the problem
* Move the check of the clock device being ready to the lpspi init,
  instead of checking it every time on configure. It probably also makes
  more sense to not ready the lpspi device if the clock is not ready.
* Move the bare-metal configuration of bit fields AFTER the SDK Init
  call.
* Return the proper error code if clock control call errors.

Signed-off-by: Declan Snyder <[email protected]>
@decsny
spi_mcux_lpspi: Move RTIO code to rtio functions
e13cc8c 
To facilitate changing this driver, decouple rtio from functions not
specific to RTIO. This also requires moving the sdk driver handle
creation outside of the configure call. An effect of this is we can
stop initializing an unused sdk driver handle for the dma path.

Signed-off-by: Declan Snyder <[email protected]>
@decsny decsny force-pushed the feature/lpspi_enhancements_1 branch from 1f2c371 to e13cc8c Compare December 13, 2024 17:57
@decsny
Copy link
Member Author

decsny commented Dec 13, 2024

rebased and fixed commit message

@mmahadevan108 @tbursztyka can one of the assignee help to review this

@kartben kartben merged commit 42511c8 into zephyrproject-rtos:main Dec 14, 2024
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yvanderv yvanderv yvanderv left review comments

@tbursztyka tbursztyka tbursztyka approved these changes

@dleach02 dleach02 dleach02 approved these changes

@EmilioCBen EmilioCBen EmilioCBen approved these changes

@danieldegrasse danieldegrasse Awaiting requested review from danieldegrasse

@dbaluta dbaluta Awaiting requested review from dbaluta

@manuargue manuargue Awaiting requested review from manuargue

@MarkWangChinese MarkWangChinese Awaiting requested review from MarkWangChinese

@mmahadevan108 mmahadevan108 Awaiting requested review from mmahadevan108

@teburd teburd Awaiting requested review from teburd

@DerekSnell DerekSnell Awaiting requested review from DerekSnell

Assignees

@tbursztyka tbursztyka

@mmahadevan108 mmahadevan108

Labels
area: SPI SPI bus platform: NXP Drivers NXP Semiconductors, drivers
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@decsny @tbursztyka @yvanderv @dleach02 @EmilioCBen @kartben @mmahadevan108 @zephyrbot