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: mips_pic32_common: Refactor GPIO peripheral #12475

Merged
merged 3 commits into from
Jan 31, 2020
Merged

cpu: mips_pic32_common: Refactor GPIO peripheral #12475

merged 3 commits into from
Jan 31, 2020

Conversation

francois-berder
Copy link
Contributor

Contribution description

This PR simplifies the code of the GPIO peripheral.

 # (master, commit: 0146c7b497df)
 111496	   3017	   5056	 119569	  1d311	/home/francois/projects/RIOT/examples/hello-world/bin/pic32-wifire/hello-world.elf

 # This PR
 111308	   3017	   5056	 119381	  1d255	/home/francois/projects/RIOT/examples/hello-world/bin/pic32-wifire/hello-world.elf

Testing procedure

I modified hello-world example to toggle, switch on/off LEDs. UART should keep working.

Issues/PRs references

None

@benpicco benpicco added Area: drivers Area: Device drivers CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: MIPS Platform: This PR/issue effects MIPS-based platforms labels Oct 17, 2019
@benpicco
Copy link
Contributor

Murdock is not happy.

@francois-berder
Copy link
Contributor Author

It seems that PIC32MX470F512H does not have PORTA. Sigh...
Anyway, I think it would be better if PR #12477 is merged before this one.

@francois-berder
cpu: mips_pic32_common: Refactor GPIO peripheral
d210a16 
Signed-off-by: Francois Berder <18538310+francois-berder@users.noreply.github.com>
@francois-berder
boards: pic32-clicker: Do not include vendor header in board header
2d68b16 
Signed-off-by: Francois Berder <18538310+francois-berder@users.noreply.github.com>
@francois-berder
Copy link
Contributor Author

This PR is now ready for review

@francois-berder
boards: pic32-wifire: Do not include vendor header in board header
237f736 
Signed-off-by: Francois Berder <18538310+francois-berder@users.noreply.github.com>
@francois-berder francois-berder mentioned this pull request Jan 11, 2020
Merged
benpicco
benpicco approved these changes Jan 30, 2020
View reviewed changes
Copy link
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

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

Code looks good, I trust in your testing.

cpu/mips_pic32_common/include/periph_cpu_common.h
/**
* @brief Override GPIO pin selection macro
*/
#define GPIO_PIN(x,y) ((x << 4) | (y & 0xf))
#define GPIO_PIN(x, y) (((_PORTB_BASE_ADDRESS & 0xFFFFF000) + (x << 8)) | y)
Copy link
Contributor

Choose a reason for hiding this comment

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

I know it doesn't make a difference, but it would feel nicer if you used _PORTA_BASE_ADDRESS here.
Never mind, PIC32MX470F512H does not have PORTA.

cpu/mips_pic32_common/periph/gpio.c
#define ODCxSET(P) ((P)[0x48/0x4])
#define ANSELxCLR(P) ((P)[0x04/0x4])
#define TRISxCLR(P) ((P)[0x14/0x4])
#define TRISxSET(P) ((P)[0x18/0x4])
Copy link
Contributor

Choose a reason for hiding this comment

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

Using a struct for this would be a more orthodox solution, but with all those gaps I see how this is easier to handle.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought about that but it went against one of the mains reasons for refactoring GPIO peripheral: avoiding storing base addresses to reduce the size of the firmware.

Copy link
Contributor

Choose a reason for hiding this comment

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

If it's a compile-time constant the compiler will also optimize it away if you are using a struct.

@benpicco benpicco merged commit cdb427b into RIOT-OS:master Jan 31, 2020
@francois-berder francois-berder deleted the wifire-gpio-refactor branch January 31, 2020 18:50
@leandrolanzieri leandrolanzieri added the Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation label Feb 20, 2020
@leandrolanzieri leandrolanzieri added this to the Release 2020.04 milestone Feb 20, 2020
@leandrolanzieri leandrolanzieri added the Area: cpu Area: CPU/MCU ports label Feb 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benpicco benpicco benpicco approved these changes

Assignees
No one assigned
Labels
Area: cpu Area: CPU/MCU ports Area: drivers Area: Device drivers CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: MIPS Platform: This PR/issue effects MIPS-based platforms Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation
Projects
None yet
Milestone
Release 2020.04
Development

Successfully merging this pull request may close these issues.
3 participants
@francois-berder @benpicco @leandrolanzieri