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.

Sign up for GitHub

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

Jump to bottom

cpu/stm32/periph/gpio: simplify condition code #20479

Merged
merged 1 commit into from
Mar 21, 2024
Merged

cpu/stm32/periph/gpio: simplify condition code #20479

merged 1 commit into from
Mar 21, 2024

Conversation

Enoch247
Copy link
Contributor

@Enoch247 Enoch247 commented Mar 18, 2024 •
edited
Loading

Contribution description

I am testing the water with this patch. If it is well received, there will be more coming. As I am adding support for the STM32H7 family of chips, I noticed I needed to add the chip family's name to several lists of chips to select the right code for the build. The aim here is to let the build system do the work for us to reduce the overhead of adding new chips.

Testing procedure

I tested by doing:

make -C examples/blinky BOARD=nucleo-f303ze

The above also works for my work-in-progress port to the Nucleo-STM32H723ZG. Other testers welcome.

Issues/PRs references

None known

@github-actions github-actions bot added Platform: ARM Platform: This PR/issue effects ARM-based platforms Area: cpu Area: CPU/MCU ports labels Mar 18, 2024
@kfessel kfessel added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Mar 18, 2024
@riot-ci
Copy link

riot-ci commented Mar 18, 2024 •
edited
Loading

Murdock results

✔️ PASSED

8555a66 cpu/stm32/periph/gpio: simplify conditional code

Success Failures Total Runtime
143091 0 143091 01h:45m:22s

Artifacts

kfessel
kfessel reviewed Mar 18, 2024
View reviewed changes
cpu/stm32/periph/gpio_all.c Outdated
Comment on lines 111 to 112
#elif defined(RCC_AHB4ENR_GPIOAEN)
periph_clk_en(AHB4, (RCC_MC_AHB4ENSETR_GPIOAEN << _port_num(pin)));
Copy link
Contributor

Choose a reason for hiding this comment

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

missmatch ??

Suggested change
#elif defined(RCC_AHB4ENR_GPIOAEN)
periph_clk_en(AHB4, (RCC_MC_AHB4ENSETR_GPIOAEN << _port_num(pin)));
#elif defined(RCC_MC_AHB4ENSETR_GPIOAEN)
periph_clk_en(AHB4, (RCC_MC_AHB4ENSETR_GPIOAEN << _port_num(pin)));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. Fixed. I am wondering why the build server didn't catch this?

Copy link
Contributor Author

@Enoch247 Enoch247 Mar 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Interesting. Building locally (make -C examples/blinky BOARD=stm32mp157c-dk2), the mismatch is detected. Does CI/CD build for all boards for PRs?

Copy link
Contributor

Choose a reason for hiding this comment

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

it does not build for all board, but more like one per kind (eg one stm32, one sam)

kfessel
kfessel reviewed Mar 18, 2024
View reviewed changes
cpu/stm32/periph/gpio_all.c Outdated
periph_clk_en(AHB2, (RCC_AHB2ENR1_GPIOAEN << _port_num(pin)));
#elif defined(CPU_FAM_STM32MP1)
#elif defined(RCC_AHB4ENR_GPIOAEN)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
#elif defined(RCC_AHB4ENR_GPIOAEN)
#elif defined(RCC_MC_AHB4ENSETR_GPIOAEN)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

kfessel
kfessel reviewed Mar 18, 2024
View reviewed changes
Copy link
Contributor

@kfessel kfessel left a comment

Choose a reason for hiding this comment

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

valid change, since the vendor macro contains the register name, makes this easier to read and adapt.

@kfessel kfessel added the CI: full build disable CI build filter label Mar 19, 2024
@maribu
Copy link
Member

maribu commented Mar 19, 2024

If it is well received, there will be more coming.

This is a good cleanup.

Note though, that this is pretty similar to what we do in the STM32 GPIO LL implementation. Given that the plan is to drop all periph/gpio implementations and replace them with one implementation that sits on top of GPIO LL (and eventually also on top of GPIO extenders), time spent on cleaning up periph/gpio might be spent better elsewhere...

@Enoch247
Copy link
Contributor Author

If it is well received, there will be more coming.

This is a good cleanup.

Note though, that this is pretty similar to what we do in the STM32 GPIO LL implementation. Given that the plan is to drop all periph/gpio implementations and replace them with one implementation that sits on top of GPIO LL (and eventually also on top of GPIO extenders), time spent on cleaning up periph/gpio might be spent better elsewhere...

Thanks for the heads-up. I had some other improvements in gpio I wanted to make, but won't, given this info. However, my intent is to use this technique in several of the STM32 periph drivers as well as the STM32's periph bus code. So this small test has served its purpose to me.

@Enoch247
cpu/stm32/periph/gpio: simplify conditional code
8555a66 
This patch similifies some of the handling of differences between STM32
chips. The intent is to improve scaling of the code as more chips are
added.
@Enoch247
Copy link
Contributor Author

fixup commits squashed

@Enoch247
Copy link
Contributor Author

I believe this is ready to merge. Is there something more I should be doing to gain the needed approval?

@maribu maribu enabled auto-merge March 21, 2024 12:33
@maribu maribu added this pull request to the merge queue Mar 21, 2024
Merged via the queue into RIOT-OS:master with commit ad4b8f2 Mar 21, 2024
26 checks passed
@MrKevinWeiss MrKevinWeiss added this to the Release 2024.04 milestone Apr 30, 2024
@Enoch247 Enoch247 deleted the simplify-stm32-gpio-code branch October 21, 2024 14:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kfessel kfessel kfessel left review comments

@maribu maribu maribu approved these changes

@aabadie aabadie Awaiting requested review from aabadie aabadie is a code owner

@vincent-d vincent-d Awaiting requested review from vincent-d vincent-d is a code owner

Assignees
No one assigned
Labels
Area: cpu Area: CPU/MCU ports CI: full build disable CI build filter CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: ARM Platform: This PR/issue effects ARM-based platforms
Projects
None yet
Milestone
Release 2024.04
Development

Successfully merging this pull request may close these issues.
5 participants
@Enoch247 @riot-ci @maribu @kfessel @MrKevinWeiss