diff mbox series

[02/10] rpmsg: core: Add channel creation internal API

Message ID 20200922001000.899956-3-mathieu.poirier@linaro.org
State Superseded
Headers show
Series rpmsg: Make RPMSG name service modular | expand

Commit Message

Mathieu Poirier Sept. 22, 2020, 12:09 a.m. UTC
From: Arnaud Pouliquen <arnaud.pouliquen@st.com>

Add the channel creation API as a first step to be able to define the
name service announcement as a rpmsg driver independent from the RPMsg
virtio bus.

Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
---
 drivers/rpmsg/rpmsg_core.c     | 45 ++++++++++++++++++++++++++++++++++
 drivers/rpmsg/rpmsg_internal.h | 12 +++++++++
 2 files changed, 57 insertions(+)

Comments

Guennadi Liakhovetski Sept. 30, 2020, 6:35 a.m. UTC | #1
On Mon, Sep 21, 2020 at 06:09:52PM -0600, Mathieu Poirier wrote:
> From: Arnaud Pouliquen <arnaud.pouliquen@st.com>
> 
> Add the channel creation API as a first step to be able to define the
> name service announcement as a rpmsg driver independent from the RPMsg
> virtio bus.
> 
> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
> ---
>  drivers/rpmsg/rpmsg_core.c     | 45 ++++++++++++++++++++++++++++++++++
>  drivers/rpmsg/rpmsg_internal.h | 12 +++++++++
>  2 files changed, 57 insertions(+)
> 
> diff --git a/drivers/rpmsg/rpmsg_core.c b/drivers/rpmsg/rpmsg_core.c
> index 91de940896e3..50a835eaf1ba 100644
> --- a/drivers/rpmsg/rpmsg_core.c
> +++ b/drivers/rpmsg/rpmsg_core.c
> @@ -20,6 +20,51 @@
>  
>  #include "rpmsg_internal.h"
>  
> +/**
> + * rpmsg_create_channel() - create a new rpmsg channel
> + * using its name and address info.
> + * @rpdev: rpmsg driver

device

> + * @chinfo: channel_info to bind
> + *
> + * Returns a pointer to the new rpmsg device on success, or NULL on error.
> + */
> +struct rpmsg_device *
> +	rpmsg_create_channel(struct rpmsg_device *rpdev,

You might call this nitpicking, but we already have two indentation styles for 
functions:

return_type function(type1 arg1,
			...)

(my personal preference, it also has sub-variants - depending on the aligning 
of the second line and any following lines, and one more moving "type1 arg1" 
to the second line)

return_type
function(...

and now you're also introducing the third style - with "function" indented... 
Maybe we don't need more of those, particularly since now with 100 chars per 
line in most cases the very first style can be used.

> +			     struct rpmsg_channel_info *chinfo)
> +{
> +	if (WARN_ON(!rpdev))
> +		return NULL;
> +	if (!rpdev->ops || !rpdev->ops->create_channel) {
> +		dev_err(&rpdev->dev, "no create_channel ops found\n");
> +		return NULL;
> +	}
> +
> +	return rpdev->ops->create_channel(rpdev, chinfo);
> +}
> +EXPORT_SYMBOL(rpmsg_create_channel);
> +
> +/**
> + * rpmsg_release_channel() - release a rpmsg channel
> + * using its name and address info.
> + * @rpdev: rpmsg driver

device

> + * @chinfo: channel_info to bind
> + *
> + * Returns 0 on success or an appropriate error value.
> + */
> +int rpmsg_release_channel(struct rpmsg_device *rpdev,
> +			  struct rpmsg_channel_info *chinfo)
> +{
> +	if (WARN_ON(!rpdev))
> +		return -EINVAL;
> +	if (!rpdev->ops || !rpdev->ops->release_channel) {
> +		dev_err(&rpdev->dev, "no release_channel ops found\n");
> +		return -EPERM;

ENOSYS or EOPNOTSUPP? I'm never sure which one is appropriate for 
this kind of errors.

> +	}
> +
> +	return rpdev->ops->release_channel(rpdev, chinfo);
> +}
> +EXPORT_SYMBOL(rpmsg_release_channel);
> +
>  /**
>   * rpmsg_create_ept() - create a new rpmsg_endpoint
>   * @rpdev: rpmsg channel device
> diff --git a/drivers/rpmsg/rpmsg_internal.h b/drivers/rpmsg/rpmsg_internal.h
> index 3fc83cd50e98..587f723757d4 100644
> --- a/drivers/rpmsg/rpmsg_internal.h
> +++ b/drivers/rpmsg/rpmsg_internal.h
> @@ -20,6 +20,8 @@
>  
>  /**
>   * struct rpmsg_device_ops - indirection table for the rpmsg_device operations
> + * @create_channel:	create backend-specific channel, optional
> + * @release_channel:	release backend-specific channel, optional

Are they really optional? You return errors if they aren't available.

>   * @create_ept:		create backend-specific endpoint, required
>   * @announce_create:	announce presence of new channel, optional
>   * @announce_destroy:	announce destruction of channel, optional
> @@ -29,6 +31,11 @@
>   * advertise new channels implicitly by creating the endpoints.
>   */
>  struct rpmsg_device_ops {
> +	struct rpmsg_device *(*create_channel)(struct rpmsg_device *rpdev,
> +					     struct rpmsg_channel_info *chinfo);
> +	int (*release_channel)(struct rpmsg_device *rpdev,
> +			       struct rpmsg_channel_info *chinfo);
> +
>  	struct rpmsg_endpoint *(*create_ept)(struct rpmsg_device *rpdev,
>  					    rpmsg_rx_cb_t cb, void *priv,
>  					    struct rpmsg_channel_info chinfo);
> @@ -75,6 +82,11 @@ int rpmsg_unregister_device(struct device *parent,
>  struct device *rpmsg_find_device(struct device *parent,
>  				 struct rpmsg_channel_info *chinfo);
>  
> +struct rpmsg_device *
> +rpmsg_create_channel(struct rpmsg_device *rpdev,
> +		     struct rpmsg_channel_info *chinfo);
> +int rpmsg_release_channel(struct rpmsg_device *rpdev,
> +			  struct rpmsg_channel_info *chinfo);
>  /**
>   * rpmsg_chrdev_register_device() - register chrdev device based on rpdev
>   * @rpdev:	prepared rpdev to be used for creating endpoints
> -- 
> 2.25.1
>
Arnaud POULIQUEN Oct. 1, 2020, 2:46 p.m. UTC | #2
On 9/30/20 8:35 AM, Guennadi Liakhovetski wrote:
> On Mon, Sep 21, 2020 at 06:09:52PM -0600, Mathieu Poirier wrote:
>> From: Arnaud Pouliquen <arnaud.pouliquen@st.com>
>>
>> Add the channel creation API as a first step to be able to define the
>> name service announcement as a rpmsg driver independent from the RPMsg
>> virtio bus.
>>
>> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
>> ---
>>  drivers/rpmsg/rpmsg_core.c     | 45 ++++++++++++++++++++++++++++++++++
>>  drivers/rpmsg/rpmsg_internal.h | 12 +++++++++
>>  2 files changed, 57 insertions(+)
>>
>> diff --git a/drivers/rpmsg/rpmsg_core.c b/drivers/rpmsg/rpmsg_core.c
>> index 91de940896e3..50a835eaf1ba 100644
>> --- a/drivers/rpmsg/rpmsg_core.c
>> +++ b/drivers/rpmsg/rpmsg_core.c
>> @@ -20,6 +20,51 @@
>>  
>>  #include "rpmsg_internal.h"
>>  
>> +/**
>> + * rpmsg_create_channel() - create a new rpmsg channel
>> + * using its name and address info.
>> + * @rpdev: rpmsg driver
> 
> device
> 
>> + * @chinfo: channel_info to bind
>> + *
>> + * Returns a pointer to the new rpmsg device on success, or NULL on error.
>> + */
>> +struct rpmsg_device *
>> +	rpmsg_create_channel(struct rpmsg_device *rpdev,
> 
> You might call this nitpicking, but we already have two indentation styles for 
> functions:
> 
> return_type function(type1 arg1,
> 			...)
> 
> (my personal preference, it also has sub-variants - depending on the aligning 
> of the second line and any following lines, and one more moving "type1 arg1" 
> to the second line)
> 
> return_type
> function(...
> 
> and now you're also introducing the third style - with "function" indented... 
> Maybe we don't need more of those, particularly since now with 100 chars per 
> line in most cases the very first style can be used.

Right, bad coding style.

> 
>> +			     struct rpmsg_channel_info *chinfo)
>> +{
>> +	if (WARN_ON(!rpdev))
>> +		return NULL;
>> +	if (!rpdev->ops || !rpdev->ops->create_channel) {
>> +		dev_err(&rpdev->dev, "no create_channel ops found\n");
>> +		return NULL;
>> +	}
>> +
>> +	return rpdev->ops->create_channel(rpdev, chinfo);
>> +}
>> +EXPORT_SYMBOL(rpmsg_create_channel);
>> +
>> +/**
>> + * rpmsg_release_channel() - release a rpmsg channel
>> + * using its name and address info.
>> + * @rpdev: rpmsg driver
> 
> device
> 
>> + * @chinfo: channel_info to bind
>> + *
>> + * Returns 0 on success or an appropriate error value.
>> + */
>> +int rpmsg_release_channel(struct rpmsg_device *rpdev,
>> +			  struct rpmsg_channel_info *chinfo)
>> +{
>> +	if (WARN_ON(!rpdev))
>> +		return -EINVAL;
>> +	if (!rpdev->ops || !rpdev->ops->release_channel) {
>> +		dev_err(&rpdev->dev, "no release_channel ops found\n");
>> +		return -EPERM;
> 
> ENOSYS or EOPNOTSUPP? I'm never sure which one is appropriate for 
> this kind of errors.

For coherency with the other RPMsg API functions, could be ENXIO...

> 
>> +	}
>> +
>> +	return rpdev->ops->release_channel(rpdev, chinfo);
>> +}
>> +EXPORT_SYMBOL(rpmsg_release_channel);
>> +
>>  /**
>>   * rpmsg_create_ept() - create a new rpmsg_endpoint
>>   * @rpdev: rpmsg channel device
>> diff --git a/drivers/rpmsg/rpmsg_internal.h b/drivers/rpmsg/rpmsg_internal.h
>> index 3fc83cd50e98..587f723757d4 100644
>> --- a/drivers/rpmsg/rpmsg_internal.h
>> +++ b/drivers/rpmsg/rpmsg_internal.h
>> @@ -20,6 +20,8 @@
>>  
>>  /**
>>   * struct rpmsg_device_ops - indirection table for the rpmsg_device operations
>> + * @create_channel:	create backend-specific channel, optional
>> + * @release_channel:	release backend-specific channel, optional
> 
> Are they really optional? You return errors if they aren't available.

I think they are optional, the back-end might not support the operation. 
In this case, activating an RPMsg client that uses the interface should result in an
error because the service is not compatible with the back-end implementation.

Regards,
Arnaud

> 
>>   * @create_ept:		create backend-specific endpoint, required
>>   * @announce_create:	announce presence of new channel, optional
>>   * @announce_destroy:	announce destruction of channel, optional
>> @@ -29,6 +31,11 @@
>>   * advertise new channels implicitly by creating the endpoints.
>>   */
>>  struct rpmsg_device_ops {
>> +	struct rpmsg_device *(*create_channel)(struct rpmsg_device *rpdev,
>> +					     struct rpmsg_channel_info *chinfo);
>> +	int (*release_channel)(struct rpmsg_device *rpdev,
>> +			       struct rpmsg_channel_info *chinfo);
>> +
>>  	struct rpmsg_endpoint *(*create_ept)(struct rpmsg_device *rpdev,
>>  					    rpmsg_rx_cb_t cb, void *priv,
>>  					    struct rpmsg_channel_info chinfo);
>> @@ -75,6 +82,11 @@ int rpmsg_unregister_device(struct device *parent,
>>  struct device *rpmsg_find_device(struct device *parent,
>>  				 struct rpmsg_channel_info *chinfo);
>>  
>> +struct rpmsg_device *
>> +rpmsg_create_channel(struct rpmsg_device *rpdev,
>> +		     struct rpmsg_channel_info *chinfo);
>> +int rpmsg_release_channel(struct rpmsg_device *rpdev,
>> +			  struct rpmsg_channel_info *chinfo);
>>  /**
>>   * rpmsg_chrdev_register_device() - register chrdev device based on rpdev
>>   * @rpdev:	prepared rpdev to be used for creating endpoints
>> -- 
>> 2.25.1
>>
diff mbox series

Patch

diff --git a/drivers/rpmsg/rpmsg_core.c b/drivers/rpmsg/rpmsg_core.c
index 91de940896e3..50a835eaf1ba 100644
--- a/drivers/rpmsg/rpmsg_core.c
+++ b/drivers/rpmsg/rpmsg_core.c
@@ -20,6 +20,51 @@ 
 
 #include "rpmsg_internal.h"
 
+/**
+ * rpmsg_create_channel() - create a new rpmsg channel
+ * using its name and address info.
+ * @rpdev: rpmsg driver
+ * @chinfo: channel_info to bind
+ *
+ * Returns a pointer to the new rpmsg device on success, or NULL on error.
+ */
+struct rpmsg_device *
+	rpmsg_create_channel(struct rpmsg_device *rpdev,
+			     struct rpmsg_channel_info *chinfo)
+{
+	if (WARN_ON(!rpdev))
+		return NULL;
+	if (!rpdev->ops || !rpdev->ops->create_channel) {
+		dev_err(&rpdev->dev, "no create_channel ops found\n");
+		return NULL;
+	}
+
+	return rpdev->ops->create_channel(rpdev, chinfo);
+}
+EXPORT_SYMBOL(rpmsg_create_channel);
+
+/**
+ * rpmsg_release_channel() - release a rpmsg channel
+ * using its name and address info.
+ * @rpdev: rpmsg driver
+ * @chinfo: channel_info to bind
+ *
+ * Returns 0 on success or an appropriate error value.
+ */
+int rpmsg_release_channel(struct rpmsg_device *rpdev,
+			  struct rpmsg_channel_info *chinfo)
+{
+	if (WARN_ON(!rpdev))
+		return -EINVAL;
+	if (!rpdev->ops || !rpdev->ops->release_channel) {
+		dev_err(&rpdev->dev, "no release_channel ops found\n");
+		return -EPERM;
+	}
+
+	return rpdev->ops->release_channel(rpdev, chinfo);
+}
+EXPORT_SYMBOL(rpmsg_release_channel);
+
 /**
  * rpmsg_create_ept() - create a new rpmsg_endpoint
  * @rpdev: rpmsg channel device
diff --git a/drivers/rpmsg/rpmsg_internal.h b/drivers/rpmsg/rpmsg_internal.h
index 3fc83cd50e98..587f723757d4 100644
--- a/drivers/rpmsg/rpmsg_internal.h
+++ b/drivers/rpmsg/rpmsg_internal.h
@@ -20,6 +20,8 @@ 
 
 /**
  * struct rpmsg_device_ops - indirection table for the rpmsg_device operations
+ * @create_channel:	create backend-specific channel, optional
+ * @release_channel:	release backend-specific channel, optional
  * @create_ept:		create backend-specific endpoint, required
  * @announce_create:	announce presence of new channel, optional
  * @announce_destroy:	announce destruction of channel, optional
@@ -29,6 +31,11 @@ 
  * advertise new channels implicitly by creating the endpoints.
  */
 struct rpmsg_device_ops {
+	struct rpmsg_device *(*create_channel)(struct rpmsg_device *rpdev,
+					     struct rpmsg_channel_info *chinfo);
+	int (*release_channel)(struct rpmsg_device *rpdev,
+			       struct rpmsg_channel_info *chinfo);
+
 	struct rpmsg_endpoint *(*create_ept)(struct rpmsg_device *rpdev,
 					    rpmsg_rx_cb_t cb, void *priv,
 					    struct rpmsg_channel_info chinfo);
@@ -75,6 +82,11 @@  int rpmsg_unregister_device(struct device *parent,
 struct device *rpmsg_find_device(struct device *parent,
 				 struct rpmsg_channel_info *chinfo);
 
+struct rpmsg_device *
+rpmsg_create_channel(struct rpmsg_device *rpdev,
+		     struct rpmsg_channel_info *chinfo);
+int rpmsg_release_channel(struct rpmsg_device *rpdev,
+			  struct rpmsg_channel_info *chinfo);
 /**
  * rpmsg_chrdev_register_device() - register chrdev device based on rpdev
  * @rpdev:	prepared rpdev to be used for creating endpoints