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

drivers: i2s_mcux_sai: Cleanup #75797

Merged
merged 7 commits into from
Jan 9, 2025

Conversation

decsny
Copy link
Member

@decsny decsny commented Jul 11, 2024

While debugging the i2s sai driver, we discovered that the driver is very hard to follow and there is a lot of redundant code. Clean up the code to be more readable and understandable. All the logic should be the same.

Copy link

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.

Copy link

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.

@github-actions github-actions bot added the Stale label Nov 10, 2024
@decsny decsny removed the Stale label Nov 11, 2024
@decsny decsny force-pushed the fix/clean_i2s_driver branch from 641cf45 to f90bd10 Compare November 21, 2024 00:34
@decsny
Copy link
Member Author

decsny commented Nov 21, 2024

rebased, updated the PR, removed some of the changes. The changes in the current state of the PR reduce the size of the driver .text section size by almost 3%.

@decsny decsny added the block: HW Test Testing on hardware required before merging label Nov 21, 2024
@decsny decsny marked this pull request as ready for review November 21, 2024 00:38
@zephyrbot zephyrbot added area: I2S platform: NXP Drivers NXP Semiconductors, drivers labels Nov 21, 2024
danieldegrasse
danieldegrasse previously approved these changes Nov 26, 2024
Copy link
Collaborator

@danieldegrasse danieldegrasse left a comment

Choose a reason for hiding this comment

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

Approving, but hardware testing should be done before this merges

@decsny decsny added DNM This PR should not be merged (Do Not Merge) and removed DNM This PR should not be merged (Do Not Merge) labels Dec 18, 2024
@kartben
Copy link
Collaborator

kartben commented Dec 19, 2024

@decsny this needs a rebase

decsny added 7 commits January 9, 2025 09:56
Put DT_DRV_COMPAT at top of file by custom

Consolidate logging macros

Clean up checks and use build assert

Remove header file which was not necessary

Remove unnecessary forward declarations

Fix the type of the stream state to be more precise, it is used as the
enum i2s_state type. This requires handling all the enum cases in switch
statements

Signed-off-by: Declan Snyder <[email protected]>
There's no need to put an if before i2s_purge_stream_buffers,
because this is handled already by the boolean values entered to the
function

Also remove the inline of the function, which allowed the compiler to
make optimizations and save some bytes.

Signed-off-by: Declan Snyder <[email protected]>
Simplify some code by using the sai_sync_mode_t type directly instead of
converting from boolean

Signed-off-by: Declan Snyder <[email protected]>
move copy pasted code into common spot for when there is an invalid
configuration passed to the config function

Signed-off-by: Declan Snyder <[email protected]>
Change this code about the frame and bit clock to be more readable

Signed-off-by: Declan Snyder <[email protected]>
Creating this local variable saves some text space in ROM

Signed-off-by: Declan Snyder <[email protected]>
The dma callback functions were written in a very confusing way, clean
them up, and this change also saves some bytes of ROM.

Signed-off-by: Declan Snyder <[email protected]>
@decsny decsny dismissed stale reviews from bjarki-andreasen and danieldegrasse via 956dad7 January 9, 2025 16:49
@decsny decsny force-pushed the fix/clean_i2s_driver branch from f90bd10 to 956dad7 Compare January 9, 2025 16:49
@decsny decsny removed the block: HW Test Testing on hardware required before merging label Jan 9, 2025
@decsny
Copy link
Member Author

decsny commented Jan 9, 2025

removed the commit refactoring the token names, it was not necessary and too many conflicts, I didn't care

also tested on RT1060 EVKB and passed i2s_speed test

Copy link
Collaborator

@bjarki-andreasen bjarki-andreasen left a comment

Choose a reason for hiding this comment

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

Really nice! found one oddity :)

drivers/i2s/i2s_mcux_sai.c Show resolved Hide resolved
@kartben kartben merged commit 3c1dcf3 into zephyrproject-rtos:main Jan 9, 2025
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: I2S platform: NXP Drivers NXP Semiconductors, drivers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants