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: spi_mcux_lpspi: Clean up code #78503

Closed

Conversation

decsny
Copy link
Member

@decsny decsny commented Sep 16, 2024

While working on fixing a bug, I found this driver to be a mess and difficult to read, so clean it up. Most of the changes have no functional effect, some however are going to cause minor optimizations in the image.

Trivial changes:

  • Organize #includes and #defines and put prototypes at the top.
  • Remove some very redundant comments.
  • Remove code that was commented out.
  • Reword a few comments.
  • Add comments to #endif showing the condition ending.
  • Remove as many preprocessor directives as possible where they are not necessary.
  • Reorder some functions to be located near other relevant functions (eg, put init and isr near the device init macro).
  • Add some delimiting comments for readability.
  • Remove many very unnecessary newlines.
  • Align slashes at the end of macros.
  • Align debugging statements.

Minor Changes:

  • Consolidate some duplicated code in the dma paths into helper functions (spi_mcux_dma_equal_block_length and spi_mcux_dma_common_load). This should make the image smaller but have no functional difference besides a struct potentially getting it's members filled in a different order.
  • Reduce redundancy and ifdef chaos in the transceive wrappers by making another wrapper function.
  • Some source code changed to reduce indentation levels and nested conditional controls which were hard to follow, but functional logic is kept the same.
  • Remove absolutely redundant and pointless wrapper of k_spin_lock.
  • Remove parent_dev from config struct and utilize irq_config_func in all cases, with a different definition for lp_flexcomm / legacy.

Fixes:

  • Fix some code added in 17032a0 that appears to have a bug where transceive_dma is called even if there is no dma device from spi_mcux_transceive_async.
  • Fix a code path in transceive_dma that appears to have a control flow bug (probably due to the super hard to follow nesting and #ifdefs), where under a certain scenario if an error happened, some code that seems to depend on the earlier code would execute, but the earlier code didn't execute because of the error, seems like what was meant to happen was to wrap up and exit the function in case of error like the other code path.
  • Fix the slave select input check allowing slave 4, because of using

    instead of >= in the check.

and after all this, run clang format

While working on fixing a bug, I found this driver
to be a mess and difficult to read, so clean it up.
Most of the changes have no functional effect, some
however are going to cause minor optimizations in the image.

Trivial changes:
- Organize #includes and #defines and put prototypes at the top.
- Remove some very redundant comments.
- Remove code that was commented out.
- Reword a few comments.
- Add comments to #endif showing the condition ending.
- Remove as many preprocessor directives as possible where
  they are not necessary.
- Reorder some functions to be located near other relevant
  functions (eg, put init and isr near the device init macro).
- Add some delimiting comments for readability.
- Remove many very unnecessary newlines.
- Align slashes at the end of macros.
- Align debugging statements.

Minor Changes:
- Consolidate some duplicated code in the dma paths into
  helper functions (spi_mcux_dma_equal_block_length and
  spi_mcux_dma_common_load). This should make the image
  smaller but have no functional difference besides a struct
  potentially getting it's members filled in a different order.
- Reduce redundancy and ifdef chaos in the transceive wrappers
  by making another wrapper function.
- Some source code changed to reduce indentation levels
  and nested conditional controls which were hard to follow,
  but functional logic is kept the same.
- Remove absolutely redundant and pointless wrapper of k_spin_lock.
- Remove parent_dev from config struct and utilize irq_config_func
  in all cases, with a different definition for lp_flexcomm / legacy.

Fixes:
- Fix some code added in 17032a0
  that appears to have a bug where transceive_dma is called even
  if there is no dma device from spi_mcux_transceive_async.
- Fix a code path in transceive_dma that appears to have a control
  flow bug (probably due to the super hard to follow nesting and
  #ifdefs), where under a certain scenario if an error happened,
  some code that seems to depend on the earlier code would execute,
  but the earlier code didn't execute because of the error,
  seems like what was meant to happen was to wrap up and exit the function
  in case of error like the other code path.
- Fix the slave select input check allowing slave 4, because of using
  > instead of >= in the check.

Signed-off-by: Declan Snyder <declan.snyder@nxp.com>
clang format the spi_mcux_lpspi driver.

Signed-off-by: Declan Snyder <declan.snyder@nxp.com>
@decsny
Copy link
Member Author

decsny commented Sep 20, 2024

this commit is too old, need to completely rework the PR

@decsny decsny closed this Sep 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant