Skip to content

cpu: mips_pic32_common: Refactor GPIO peripheral#12475

Merged
benpicco merged 3 commits intoRIOT-OS:masterfrom
francois-berder:wifire-gpio-refactor
Jan 31, 2020
Merged

cpu: mips_pic32_common: Refactor GPIO peripheral#12475
benpicco merged 3 commits intoRIOT-OS:masterfrom
francois-berder:wifire-gpio-refactor

Conversation

@francois-berder
Copy link
Copy Markdown
Contributor

@francois-berder francois-berder commented Oct 16, 2019

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

benpicco commented Oct 17, 2019

Murdock is not happy.

@francois-berder
Copy link
Copy Markdown
Contributor Author

francois-berder commented Oct 17, 2019

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

francois-berder commented Oct 24, 2019

This PR is now ready for review

@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
Copy Markdown
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
Copy Markdown
Contributor

@benpicco benpicco Jan 30, 2020

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

@benpicco benpicco Jan 30, 2020

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
Copy Markdown
Contributor Author

@francois-berder francois-berder Jan 31, 2020

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

@benpicco benpicco Jan 31, 2020

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