rpmsg API

[Mattemagikern]

Hello,
I'm looking at: lib/rpmsg/rpmsg.c and the rpmsg_create_ept function.
I'm wondering if it is possible to change the API or if it is set in stone?

My current thought is that the rpmsg_create_ept function is a bit superfluous.

e.g.
Most of the parameters are members of the struct rpmsg_endpoint *ept parameter.
What I'd like to change is to make the initialization of the struct rpmsg_endpoint *ept up to the user so the priv member can be set which would allow for better handling in the callback function(s) as is intended, I would assume.

From:

int rpmsg_create_ept(struct rpmsg_endpoint *ept, struct rpmsg_device *rdev,
		     const char *name, uint32_t src, uint32_t dest,
		     rpmsg_ept_cb cb, rpmsg_ns_unbind_cb unbind_cb)

to:

int rpmsg_(add|setup|register)_ept(struct rpmsg_endpoint *ept, struct rpmsg_device *rdev)

or even

int rpmsg_(add|setup|register)_ept(struct rpmsg_endpoint *ept)

If I were to make these API changes would they be considered?

Best regards,

[arnopo]

Hello @Mattemagikern,

The API is not set in the stone, But must has to be as stable as possible. So we need a strong reason to update it.

that said If your issue is the management of the priv field:
You can propose a new API to define it. Something like rpmsg_ept_set_priv_data API

But first i would suggest you to have alook to following alternative that not need the use of the priv field.
If the endpoint structure is a member of your application structure that stores your context. You can simply retrieve your context using metal_container_of function.

Two examples:
https://github.com/OpenAMP/open-amp/blob/master/lib/proxy/rpmsg_retarget.c#L40
https://github.com/STMicroelectronics/STM32CubeMP1/blob/master/Middlewares/Third_Party/OpenAMP/virtual_driver/virt_uart.c#L57

[Mattemagikern]

But first i would suggest you to have alook to following alternative that not need the use of the priv field.
If the endpoint structure is a member of your application structure that stores your context. You can simply retrieve your context using metal_container_of function.

yes the container_of macro has crossed my mind. I think it is a neat solution when it comes to multiple instances of rpmsg and reusing of the callback functions. But that makes me think that the priv field is redundant or depricated?

You can propose a new API to define it. Something like rpmsg_ept_set_priv_data API.

If the priv is neither redundant or depricated it should be initialized with the other members for concistency:

int rpmsg_create_ept(struct rpmsg_endpoint *ept, struct rpmsg_device *rdev,
		     const char *name, uint32_t src, uint32_t dest,
		     rpmsg_ept_cb cb, rpmsg_ns_unbind_cb unbind_cb, void *priv)

[arnopo]

But first i would suggest you to have alook to following alternative that not need the use of the priv field.
If the endpoint structure is a member of your application structure that stores your context. You can simply retrieve your context using metal_container_of function.

yes the container_of macro has crossed my mind. I think it is a neat solution when it comes to multiple instances of rpmsg and reusing of the callback functions. But that makes me think that the priv field is redundant or depricated?

You can propose a new API to define it. Something like rpmsg_ept_set_priv_data API.

If the priv is neither redundant or depricated it should be initialized with the other members for concistency:

int rpmsg_create_ept(struct rpmsg_endpoint *ept, struct rpmsg_device *rdev,
		     const char *name, uint32_t src, uint32_t dest,
		     rpmsg_ept_cb cb, rpmsg_ns_unbind_cb unbind_cb, void *priv)

Right the priv field seems a relicat of the legacy. Seems to me that this field could be put in deprecated.
Today, applications that still use this field probably set it after calling rpmsg_create_ept.

I would prefer this solution insteaf of updating the API.

@edmooring : any opinion on this?

[edmooring]

I agree with @arnopo . I am not in favor of changing the rpmsg_create_ept() API in this manner. It would break too many people's code across at least 4 different RTOSs.

I did a little archeology, and the priv struct member appears to be a copy/paste of the Linux remoteproc implementation back in 2014, including the comments. I can't find any indication that it has ever been used in the OpenAMP project.
I would not be opposed to marking it as deprecated.

[Mattemagikern]

Then I believe we all agree that the priv field should be deprecated.

Thank you for taking the time to discuss this with me!

Best regards,