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.

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

NXP LPSPI: Move RTIO/DMA code to separate file #82750

Merged

Conversation

decsny
Copy link
Member

@decsny decsny commented Dec 9, 2024

depends on:

This file is basically 3 drivers in one sharing just the device structs and the configure function, and in my opinion it is not manageable, move them to seperate files

@decsny decsny force-pushed the feature/lpspi_enhancements_2 branch 4 times, most recently from 07e3813 to 0239d0c Compare December 10, 2024 20:13
Move this driver to its own subfolder to organize it
since there will be new files added for this hardware.

Signed-off-by: Declan Snyder <[email protected]>
There is (almost) a whole separate driver for RTIO,
move this code to its own file and move shared code
and definitions to a common file.

Signed-off-by: Declan Snyder <[email protected]>
The DMA-based path of the lpspi driver is basically
almost a driver of it's own, move it to it's own file.

Signed-off-by: Declan Snyder <[email protected]>
@decsny decsny force-pushed the feature/lpspi_enhancements_2 branch from 0239d0c to fd73aaa Compare December 17, 2024 17:20
@decsny decsny marked this pull request as ready for review December 17, 2024 17:20
@zephyrbot zephyrbot added area: SPI SPI bus platform: NXP Drivers NXP Semiconductors, drivers labels Dec 17, 2024
@@ -0,0 +1,131 @@
/*
Copy link
Contributor

Choose a reason for hiding this comment

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

What made you consider making this a data a separate header file?
Nit But what do you think about moving this into the common.c?

Copy link
Member Author

Choose a reason for hiding this comment

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

it's not clear to me what you are commenting about because your comment is just at the top of the file

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean moving the entire file into common.c

Copy link
Member Author

@decsny decsny Dec 18, 2024

Choose a reason for hiding this comment

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

I'm not sure what you mean, the header files has definitions that the other 3 files need in order to compile

@decsny
Copy link
Member Author

decsny commented Dec 18, 2024

@tbursztyka can you help to review, note this would be the first spi driver with its own subfolder

@decsny
Copy link
Member Author

decsny commented Dec 20, 2024

Another effect of this due to new kconfig structure is that RTIO will actually be tested by twister because now it takes precedence if enabled. Before, I realized the RTIO testcase was most likely causing the DMA (non-rtio) path of the driver to be tested instead of RTIO because of the convolution of having so many different orthogonal "paths"/drivers in one file, and the testcase.yaml didn't account for the convolution properly by disabling the DMA (which we dont need to do anymore since they are no longer coupled)

@teburd
Copy link
Collaborator

teburd commented Dec 20, 2024

Another effect of this due to new kconfig structure is that RTIO will actually be tested by twister because now it takes precedence if enabled. Before, I realized the RTIO testcase was most likely causing the DMA (non-rtio) path of the driver to be tested instead of RTIO because of the convolution of having so many different orthogonal "paths"/drivers in one file, and the testcase.yaml didn't account for the convolution properly by disabling the DMA (which we dont need to do anymore since they are no longer coupled)

Good! RTIO could be viewed as a feature superset, maybe one day we can simply say this is the right way to do things, but some caveats likely would need to be sorted first.

@decsny decsny requested a review from hakehuang January 6, 2025 18:04
@dleach02
Copy link
Member

dleach02 commented Jan 7, 2025

@tbursztyka can we get your attention on this PR so we can move it along?

@hakehuang
Copy link
Collaborator

board testing pass

@dleach02
Copy link
Member

@tbursztyka I've added myself to the assignee to help move this along.

Copy link
Collaborator

@tbursztyka tbursztyka left a comment

Choose a reason for hiding this comment

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

@dleach02 sure don't hesitate to do so, I am assigned by default because of maintainership. But when it comes to manufacturer specific changes, the default is not the most relevant anyway. (And I still get issues with being notified but GitHub...)

@kartben kartben merged commit 6cbe52b into zephyrproject-rtos:main Jan 14, 2025
29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: SPI SPI bus platform: NXP Drivers NXP Semiconductors, drivers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants