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

Remove extra system header include files #291

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

glneo
Copy link
Contributor

@glneo glneo commented Mar 20, 2024

No description provided.

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

@edmooring edmooring left a 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.

@xiaoxiang781216
Copy link
Collaborator

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>

Copy link
Contributor

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?

Copy link
Collaborator

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.

Copy link
Contributor

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?

Copy link
Collaborator

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

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

Copy link

github-actions bot commented Jul 2, 2024

This pull request has been marked as a stale pull request because it has been open (more than) 45 days with no activity.

@github-actions github-actions bot added the Stale label Jul 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants