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

openamp: add const for remoteproc_ops, image_store_ops and loader_ops #336

Merged
merged 3 commits into from
Apr 28, 2022

Conversation

xiaoxiang781216
Copy link
Collaborator

No description provided.

@arnopo arnopo changed the title openamp: add const for remoteproc_ops, image_store_ops and laoder_ops openamp: add const for remoteproc_ops, image_store_ops and loader_ops Jan 25, 2022
@arnopo
Copy link
Collaborator

arnopo commented Jan 25, 2022

Hello @xiaoxiang781216
Please could you split in 3 commits : lib, doc, app
Thanks,
Arnaud

@xiaoxiang781216
Copy link
Collaborator Author

xiaoxiang781216 commented Jan 25, 2022

Hello @xiaoxiang781216 Please could you split in 3 commits : lib, doc, app Thanks, Arnaud

doc can be split without error, but app can't pass the build independently. Do you still want to has some commit point can't pass the compile?

@arnopo
Copy link
Collaborator

arnopo commented Jan 25, 2022

Hello @xiaoxiang781216 Please could you split in 3 commits : lib, doc, app Thanks, Arnaud

doc can be split without error, but app can't pass the build independently. Do you still want to has some commit point can't pass the compile?

The point here is that the API is updated. So, even if it builds with success for OpenAMP examples this will not be the case for user project.
That said I did a test on my github and only warning seems generated due to the forced cast of a pointer to code to a pointer data

/github/workspace/apps/system/linux/machine/generic/platform_info.c:357:10: warning: initialization of ‘struct remoteproc * (*)(struct remoteproc *, const struct remoteproc_ops *, void *)’ from incompatible pointer type ‘struct remoteproc * (*)(struct remoteproc *, struct remoteproc_ops *, void *)’ [-Wincompatible-pointer-types]
1253
  357 |  .init = linux_proc_init,

But I suppose that depending on compilation options the warning could become an error.

So I still suggest making separate commits with an explicit commit message explaining the API update and its effect on project build.

Then concerning the API update itself, the disrupt is minor, and I would be OK to integrate if enough documented in commit message and release note.

@edmooring: what is your feeling on this?

@xiaoxiang781216
Copy link
Collaborator Author

Done.

Copy link
Collaborator

@arnopo arnopo left a comment

Choose a reason for hiding this comment

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

Look good to me.
The impact on API seems minor, but I would prefer to wait non regression test for release 2022.04 to evaluate impact on different platforms, before integrating it.

@edmooring
Copy link
Contributor

Hello @xiaoxiang781216 Please could you split in 3 commits : lib, doc, app Thanks, Arnaud

doc can be split without error, but app can't pass the build independently. Do you still want to has some commit point can't pass the compile?

The point here is that the API is updated. So, even if it builds with success for OpenAMP examples this will not be the case for user project. That said I did a test on my github and only warning seems generated due to the forced cast of a pointer to code to a pointer data

/github/workspace/apps/system/linux/machine/generic/platform_info.c:357:10: warning: initialization of ‘struct remoteproc * (*)(struct remoteproc *, const struct remoteproc_ops *, void *)’ from incompatible pointer type ‘struct remoteproc * (*)(struct remoteproc *, struct remoteproc_ops *, void *)’ [-Wincompatible-pointer-types]
1253
  357 |  .init = linux_proc_init,

But I suppose that depending on compilation options the warning could become an error.

So I still suggest making separate commits with an explicit commit message explaining the API update and its effect on project build.

Then concerning the API update itself, the disrupt is minor, and I would be OK to integrate if enough documented in commit message and release note.

@edmooring: what is your feeling on this?

Agreed, including your later comment about testing. I will approve these changes so we can move forward.

On �a side note, we should probably bring this up at the steering committee, since these are technically API changes, but they are very minor. Does Zephyr have a policy for these kind of changes.

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.

Looks good to me.

@arnopo arnopo added this to the Release V2022.04 milestone Feb 14, 2022
to avoid load the constant data to ram in XIP mode

Signed-off-by: Xiang Xiao <[email protected]>
follow up the change in lib

Signed-off-by: Xiang Xiao <[email protected]>
follow up the change in lib

Signed-off-by: Xiang Xiao <[email protected]>
@arnopo arnopo merged commit 923641d into OpenAMP:main Apr 28, 2022
@xiaoxiang781216 xiaoxiang781216 deleted the const branch April 28, 2022 07:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants