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

fix(PeriphDrivers): Zephyr build issue #1234

Merged
merged 1 commit into from
Oct 24, 2024
Merged

Conversation

ozersa
Copy link
Contributor

@ozersa ozersa commented Oct 21, 2024

Description

Zephyr drivers requires pullup and pulldown definition. This commit fix build issue on zephyr side
image

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
Copy link
Contributor

Why are we re-including these unsupported definitions? The ME30 only supports high-impedance, weak pull-up, and weak pull-down. Because it's a new part, I removed MXC_GPIO_PAD_PULL_UP and MXC_GPIO_PAD_PULL_DOWN to prevent any legacy/deprecated code. The definitions currently exist on similar parts, like the ME12, because that code has been released publicly and added for backward compatibility.

@ozersa
Copy link
Contributor Author

ozersa commented Oct 21, 2024

Why are we re-including these unsupported definitions? The ME30 only supports high-impedance, weak pull-up, and weak pull-down. Because it's a new part, I removed MXC_GPIO_PAD_PULL_UP and MXC_GPIO_PAD_PULL_DOWN to prevent any legacy/deprecated code. The definitions currently exist on similar parts, like the ME12, because that code has been released publicly and added for backward compatibility.

Good question.
On zephyr side we have one gpio_max32.c driver that serve all MAX32 MCUs. And it is not allowed to add device specific #if else cases that cause file behave different part to part (@MaureenHelm correct me if I am wrong). https://github.com/zephyrproject-rtos/zephyr/blob/main/drivers/gpio/gpio_max32.c#L88C18-L88C38 So that in past we have move part specific #if else in Wrap layer for some drivers. like: https://github.com/zephyrproject-rtos/hal_adi/blob/main/MAX/Include/wrap_max32_tmr.h#L104 If we follow same mechanism than we need to add a wrap layer for gpio too which is not exist for now.

Comment on lines 154 to 157
MXC_GPIO_PAD_PULL_UP, ///< Set pad to weak pull-up
MXC_GPIO_PAD_PULL_DOWN, ///< Set pad to weak pull-down
MXC_GPIO_PAD_WEAK_PULL_UP = MXC_GPIO_PAD_PULL_UP, ///< Set pad to weak pull-up
MXC_GPIO_PAD_WEAK_PULL_DOWN = MXC_GPIO_PAD_PULL_DOWN, ///< Set pad to weak pull-down
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is a little more readable:

MXC_GPIO_PAD_WEAK_PULL_UP, ///< Set pad to weak pull-up
MXC_GPIO_PAD_WEAK_PULL_DOWN, ///< Set pad to weak pull-down
MXC_GPIO_PAD_PULL_UP = MXC_GPIO_PAD_WEAK_PULL_UP, ///< Set pad to default (weak) pull-up
MXC_GPIO_PAD_PULL_DOWN = MXC_GPIO_PAD_WEAK_PULL_DOWN, ///< Set pad to default (weak) pull-down

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct, make sense.

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.

My 2 cents:

  1. It only matters to make the distinction between weak/strong if the micro actually supports both. Otherwise, the only distinction to make is up or down. MXC_GPIO_PAD_PULL_UP and MXC_GPIO_PAD_PULL_DOWN should be used since the names are generic and the code is readable.

  2. Removing enums just for MAX32657 gpio.h will demand extra #ifdefs. I would favor consolidation instead of divergence and leave these defined.

With this in mind I support re-defining the enums.

With them re-added, MXC_GPIO_Config will return E_BAD_PARAM if the user requests MXC_GPIO_PAD_PULL_UP. I would suggest mapping the enums onto each other like I did for the AI87 here and avoid the confusing error condition.

Zephyr drivers requires pullup and pulldown definition.
This commit fix build issue on zephyr side

Signed-off-by: Sadik Ozer <[email protected]>
@sihyung-maxim sihyung-maxim merged commit d036f16 into main Oct 24, 2024
10 checks passed
@sihyung-maxim sihyung-maxim deleted the fix/zephyr_build branch October 24, 2024 19:09
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.

3 participants