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

feat(PeriphDrivers)!: Add support for configuring GPIO Drive Strength #673

Merged
merged 23 commits into from
Sep 18, 2023

Conversation

sihyung-maxim
Copy link
Contributor

@sihyung-maxim sihyung-maxim commented Jul 17, 2023

Description

This PR adds a new configuration option to select the drive strength level for GPIO pins.

Support for GPIO drive strength was not added initially due to the varying drive strength name types, but the latest user guides have been updated with consistent drive strength names across all parts which help for code name stability.

Breaking change due to the new parameter with the mxc_gpio_cfg_t struct and the MXC_GPIO_Config(...) function now sets the drive strength. The new drvstr parameter was added to the struct because this was how the mxc_gpio_cfg_t was originally intended to be when it was first created.

Checklist Before Requesting Review

  • PR Title follows correct guidelines.
  • Description of changes and all other relevant information.
  • (Optional) Link any related GitHub issues using a keyword
  • (Optional) Provide info on any relevant functional testing/validation. For API changes or significant features, this is not optional.

@sihyung-maxim sihyung-maxim added the WIP work in progress label Jul 17, 2023
@github-actions github-actions bot added MAX32520 Related to the MAX32520 (ES17) MAX32570 Related to the MAX32570 (ME13) MAX32650 Related to the MAX32650 (ME10) MAX32655 Related to the MAX32655 (ME17) MAX32660 Related to the MAX32660 (ME11) MAX32665 Related to the MAX32665 (ME14) MAX32670 Related to the MAX32670 (ME15) MAX32672 Related to the MAX32672 (ME21) MAX32675 Related to the MAX32675 (ME16) MAX32680 Related to the MAX32680 (ME20) MAX32690 Related to the MAX32690 (ME18) MAX78000 Related to the MAX78000 (AI85) MAX78002 Related to the MAX78002 (AI87) MAX32662 Related to the MAX32662 (ME12) labels Aug 22, 2023
@sihyung-maxim sihyung-maxim added the API Change This issue or pull request involves a change to the current MSDK API label Aug 22, 2023
@sihyung-maxim sihyung-maxim removed the WIP work in progress label Aug 22, 2023
@sihyung-maxim sihyung-maxim changed the title feat(PeriphDrivers): Add support for configuring GPIO Drive Strength feat(PeriphDrivers)!: Add support for configuring GPIO Drive Strength Aug 22, 2023
@sihyung-maxim
Copy link
Contributor Author

/clang-format-run

@sihyung-maxim
Copy link
Contributor Author

/clang-format-run

Copy link
Contributor

@Jake-Carter Jake-Carter left a comment

Choose a reason for hiding this comment

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

The addition of SetDriveStrength is good.

I'm a little concerned about adding the drive strength to the GPIO config struct. Existing code will have the struct member uninitialized. I just tested this and got "16", for example:

image

The switch statement for MXC_GPIO_RevA_SetDriveStrength is well written, so in this case we initialize with drive strength 0 in the default case handler. But there is a non-zero chance that we hit 0, 1, 2, or 3, which will unexpectedly change the drive strength.

Drive strength is also only relevant in output mode.

I don't want to make you jump out of a window considering the number of files and pin config structs changed here... but I'm in favor of just adding the helper function and leaving the config struct unmodified.

@sihyung-maxim
Copy link
Contributor Author

The addition of SetDriveStrength is good.

I'm a little concerned about adding the drive strength to the GPIO config struct. Existing code will have the struct member uninitialized. I just tested this and got "16", for example:

image

The switch statement for MXC_GPIO_RevA_SetDriveStrength is well written, so in this case we initialize with drive strength 0 in the default case handler. But there is a non-zero chance that we hit 0, 1, 2, or 3, which will unexpectedly change the drive strength.

Drive strength is also only relevant in output mode.

I don't want to make you jump out of a window considering the number of files and pin config structs changed here... but I'm in favor of just adding the helper function and leaving the config struct unmodified.

No worries. Adding the new parameter in the mxc_gpio_cfg_t struct was a concern when first making these changes, but I discussed this with Lorne before committing to making all the changes with the cfg structs. This change is not as dangerous compared to adding the .vssel (VDDIO/VDDIOH) parameter to the config struct. The main potential downside is that the current consumption could slightly increase when the drive strength increases from what it originally was if the setting was uninitialized to a non-zero value. However, this should not physically break anything, unlike if we were to add the VDDIO/VDDIOH setting to the config struct.

Note: support for GPIO drive strength was not added initially due to the varying drive strength name types, but the latest user guides have been updated with consistent drive strength names across all parts which helps for code name stability. The new drvstr parameter was added to the struct because this was how the mxc_gpio_cfg_t was originally intended to be when it was first created. It was decided the benefits of having this requested feature as its intended use outweigh the potential downsides.

Good call on the drive strength only being relevant in output mode. I've added a check where this setting is ignored if the pin function is set to input mode. There are alternate functions where this setting is still used though such as the SPI pins for high-speed transactions.

@Jake-Carter Jake-Carter merged commit b64514d into main Sep 18, 2023
@Jake-Carter Jake-Carter deleted the feat/gpio_drvstr branch September 18, 2023 22:05
EricB-ADI pushed a commit that referenced this pull request Nov 21, 2023
…#673)

Co-authored-by: sihyung-maxim <sihyung-maxim@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Change This issue or pull request involves a change to the current MSDK API MAX32520 Related to the MAX32520 (ES17) MAX32570 Related to the MAX32570 (ME13) MAX32650 Related to the MAX32650 (ME10) MAX32655 Related to the MAX32655 (ME17) MAX32660 Related to the MAX32660 (ME11) MAX32662 Related to the MAX32662 (ME12) MAX32665 Related to the MAX32665 (ME14) MAX32670 Related to the MAX32670 (ME15) MAX32672 Related to the MAX32672 (ME21) MAX32675 Related to the MAX32675 (ME16) MAX32680 Related to the MAX32680 (ME20) MAX32690 Related to the MAX32690 (ME18) MAX78000 Related to the MAX78000 (AI85) MAX78002 Related to the MAX78002 (AI87)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants