-
Notifications
You must be signed in to change notification settings - Fork 170
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
Remove extra system header include files #291
base: main
Are you sure you want to change the base?
Conversation
Many of the system specific irq.h files are empty, and those that are not are only used by the internal IRQ handling functions. Remove the empty ones and do not expose the contents of the internal ones outside of the compilation unit that uses them. Signed-off-by: Andrew Davis <[email protected]>
Most of the system specific log.h files are empty, and those that are not are only used by the internal log handling functions. Remove the empty ones and do not expose the contents of the internal ones outside of the compilation unit that uses them. Signed-off-by: Andrew Davis <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Builds cleanly for FreeRTOS and bare metal on Xilinx R5. Builds cleanly for Linux on ARM64. Looks good to go.
LGTM. |
@@ -93,8 +93,6 @@ void metal_irq_enable(unsigned int vector); | |||
*/ | |||
void metal_irq_disable(unsigned int vector); | |||
|
|||
#include <metal/system/@PROJECT_SYSTEM@/irq.h> | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the only use case we need it is to include /lib/system/linux/irq.h
@tnmysh , @edmooring does AMD need the metal_linux_irq_register_dev()
as external API?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure, but I prefer to keep headers for now.
Can take a look after this release.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure, but I prefer to keep headers for now. Can take a look after this release.
Probably late to address for this release without some risks...?
@tnmysh could you add it in your review list that we merge it ( or not) just after the release?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, this can be reviewed and merged after the release.
@@ -111,6 +111,4 @@ extern void metal_default_log_handler(enum metal_log_level level, | |||
} | |||
#endif | |||
|
|||
#include <metal/system/@PROJECT_SYSTEM@/log.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
metal_zephyr_log_handler
not used in zephyr so LGTM
This pull request has been marked as a stale pull request because it has been open (more than) 45 days with no activity. |
No description provided.