Re: [PATCH 1/4] rpmsg: Introduce rpmsg_create_default_ept function

Mathieu Poirier
2021-06-21T22:42:41Z

On Mon, Jun 07, 2021 at 07:30:29PM +0200, Arnaud Pouliquen wrote:
> The rpmsg devices can be probed without default endpoint. This function
> provides the capability for rpmsg drivers to create a default endpoint
> on runtime.
>
> For example, a driver might want the rpmsg core dispatcher to drop its
> messages until it is ready to process them. In this case, the driver will
> create the default endpoint when the conditions are met to process the
> messages.
>
> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
> ---
> drivers/rpmsg/rpmsg_core.c | 51 ++++++++++++++++++++++++++++++++++++++
> include/linux/rpmsg.h | 14 +++++++++++
> 2 files changed, 65 insertions(+)
>
> diff --git a/drivers/rpmsg/rpmsg_core.c b/drivers/rpmsg/rpmsg_core.c
> index e5daee4f9373..07b680bda61f 100644
> --- a/drivers/rpmsg/rpmsg_core.c
> +++ b/drivers/rpmsg/rpmsg_core.c
> @@ -115,6 +115,57 @@ struct rpmsg_endpoint *rpmsg_create_ept(struct rpmsg_device *rpdev,
> }
> EXPORT_SYMBOL(rpmsg_create_ept);
>
> +/**
> + * rpmsg_create_default_ept() - create a default rpmsg_endpoint for a rpmsg device
> + * @rpdev: rpmsg channel device
> + * @cb: rx callback handler
> + * @priv: private data for the driver's use
> + * @chinfo: channel_info with the local rpmsg address to bind with @cb
> + *
> + * On register_rpmsg_driver if no callback is provided in the rpmsg_driver structure,
> + * no endpoint is created when the device is probed by the rpmsg bus.
> + *
> + * This function returns a pointer to the default endpoint if already created or creates
> + * a endpoint and assign it as the default endpoint of the rpmsg device.
> + *
> + * Drivers should provide their @rpdev channel (so the new endpoint would belong
> + * to the same remote processor their channel belongs to), an rx callback
> + * function, an optional private data (which is provided back when the
> + * rx callback is invoked), and an address they want to bind with the
> + * callback. If @addr is RPMSG_ADDR_ANY, then rpmsg_create_ept will
> + * dynamically assign them an available rpmsg address (drivers should have
> + * a very good reason why not to always use RPMSG_ADDR_ANY here).
> + *
> + * Returns a pointer to the endpoint on success, or NULL on error.
> + */
> +struct rpmsg_endpoint *rpmsg_create_default_ept(struct rpmsg_device *rpdev,
> + rpmsg_rx_cb_t cb, void *priv,
> + struct rpmsg_channel_info chinfo)
> +{
> + struct rpmsg_endpoint *ept;
> +
> + if (WARN_ON(!rpdev))
> + return NULL;
> +
> + /* It does not make sense to create a default endpoint without a callback. */
> + if (!cb)
> + return NULL;
> +
> + if (rpdev->ept)
> + return rpdev->ept;
> +
> + ept = rpdev->ops->create_ept(rpdev, cb, priv, chinfo);
> + if (!ept)
> + return NULL;
> +
> + /* Assign the new endpoint as default endpoint */
> + rpdev->ept = ept;
> + rpdev->src = ept->addr;
> +
> + return ept;
> +}
> +EXPORT_SYMBOL(rpmsg_create_default_ept);
> +
> /**
> * rpmsg_destroy_ept() - destroy an existing rpmsg endpoint
> * @ept: endpoing to destroy
> diff --git a/include/linux/rpmsg.h b/include/linux/rpmsg.h
> index d97dcd049f18..ab034061722c 100644
> --- a/include/linux/rpmsg.h
> +++ b/include/linux/rpmsg.h
> @@ -172,6 +172,9 @@ void rpmsg_destroy_ept(struct rpmsg_endpoint *);
> struct rpmsg_endpoint *rpmsg_create_ept(struct rpmsg_device *,
> rpmsg_rx_cb_t cb, void *priv,
> struct rpmsg_channel_info chinfo);
> +struct rpmsg_endpoint *rpmsg_create_default_ept(struct rpmsg_device *rpdev,
> + rpmsg_rx_cb_t cb, void *priv,
> + struct rpmsg_channel_info chinfo);
>
> int rpmsg_send(struct rpmsg_endpoint *ept, void *data, int len);
> int rpmsg_sendto(struct rpmsg_endpoint *ept, void *data, int len, u32 dst);
> @@ -234,6 +237,17 @@ static inline struct rpmsg_endpoint *rpmsg_create_ept(struct rpmsg_device *rpdev
> return ERR_PTR(-ENXIO);
> }
>
> +static inline struct rpmsg_endpoint *rpmsg_create_default_ept(struct rpmsg_device *rpdev,
> + rpmsg_rx_cb_t cb,
> + void *priv,

Please move this to the previous line to match the definition in the other arm
of the #if.

> + struct rpmsg_channel_info chinfo)
> +{
> + /* This shouldn't be possible */
> + WARN_ON(1);
> +
> + return NULL;
> +}
> +
> static inline int rpmsg_send(struct rpmsg_endpoint *ept, void *data, int len)
> {
> /* This shouldn't be possible */
> --
> 2.17.1
>