On Fri 22 Oct 07:54 CDT 2021, Arnaud Pouliquen wrote:
> Create the rpmsg_ctrl.c module and move the code related to the
> rpmsg_ctrldev device in this new module.
>
> Add the dependency between rpmsg_char and rpmsg_ctrl in the
> kconfig file.
>
> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@xxxxxxxxxxx>
> Reviewed-by: Mathieu Poirier <mathieu.poirier@xxxxxxxxxx>
>
> ---
> update vs previous version:
> - set the ctrl device class with new rpmsg_get_class API for legacy support
> ---
> drivers/rpmsg/Kconfig | 9 ++
> drivers/rpmsg/Makefile | 1 +
> drivers/rpmsg/rpmsg_char.c | 169 +----------------------------
> drivers/rpmsg/rpmsg_char.h | 2 +
> drivers/rpmsg/rpmsg_ctrl.c | 216 +++++++++++++++++++++++++++++++++++++
> 5 files changed, 230 insertions(+), 167 deletions(-)
> create mode 100644 drivers/rpmsg/rpmsg_ctrl.c
>
> diff --git a/drivers/rpmsg/Kconfig b/drivers/rpmsg/Kconfig
> index 0b4407abdf13..d822ec9ec692 100644
> --- a/drivers/rpmsg/Kconfig
> +++ b/drivers/rpmsg/Kconfig
> @@ -10,11 +10,20 @@ config RPMSG_CHAR
> tristate "RPMSG device interface"
> depends on RPMSG
> depends on NET
> + select RPMSG_CTRL
We don't want select of user selectable config options.
> help
> Say Y here to export rpmsg endpoints as device files, usually found
> in /dev. They make it possible for user-space programs to send and
> receive rpmsg packets.
>
> +config RPMSG_CTRL
I still don't like the introduction of more Kconfig options - search the
list for the number of patches that has corrected Kconfig dependency
issues.
That said, if you get it right...
> + tristate "RPMSG control interface"
> + depends on RPMSG
> + help
> + Say Y here to enable the support of the /dev/rpmsg_ctrlX API. This API
> + allows user-space programs to create endpoints with specific service name,
> + source and destination addresses.
> +
> config RPMSG_NS
> tristate "RPMSG name service announcement"
> depends on RPMSG
[..]
> diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
[..]
> diff --git a/drivers/rpmsg/rpmsg_char.h b/drivers/rpmsg/rpmsg_char.h
> index 109c2c43005f..ff1acc42628a 100644
> --- a/drivers/rpmsg/rpmsg_char.h
> +++ b/drivers/rpmsg/rpmsg_char.h
> @@ -12,6 +12,8 @@
> * In such case a kernel warning is printed to help develloper to fix the issue.
> */
>
> +#define RPMSG_DEV_MAX (MINORMASK + 1)
This was used to define the minors of the rpmsg chdev, now you're
splitting that range in one for the ctrl and one for the char.
Moving this define to a common place gives an impression that there's a
relationship between the two, but I don't see any. So I think you should
duplicate this in the two files - just like the other stuff.
> +
> #if IS_REACHABLE(CONFIG_RPMSG_CHAR)
> /**
> * rpmsg_chrdev_eptdev_create() - register char device based on an endpoint
> diff --git a/drivers/rpmsg/rpmsg_ctrl.c b/drivers/rpmsg/rpmsg_ctrl.c
> new file mode 100644
> index 000000000000..1d3c12e5cdcf
> --- /dev/null
> +++ b/drivers/rpmsg/rpmsg_ctrl.c
> @@ -0,0 +1,216 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2021, STMicroelectronics
Did you actually change anything that warrant the explicit copyright
claim?
> + * Copyright (c) 2016, Linaro Ltd.
> + * Copyright (c) 2012, Michal Simek <monstr@xxxxxxxxx>
> + * Copyright (c) 2012, PetaLogix
> + * Copyright (c) 2011, Texas Instruments, Inc.
> + * Copyright (c) 2011, Google, Inc.
> + *
> + * Based on rpmsg performance statistics driver by Michal Simek, which in turn
> + * was based on TI & Google OMX rpmsg driver.
> + */
[..]
> +static int rpmsg_ctrldev_probe(struct rpmsg_device *rpdev)
> +{
> + struct rpmsg_ctrldev *ctrldev;
> + struct device *dev;
> + int ret;
> +
> + ctrldev = kzalloc(sizeof(*ctrldev), GFP_KERNEL);
> + if (!ctrldev)
> + return -ENOMEM;
> +
> + ctrldev->rpdev = rpdev;
> +
> + dev = &ctrldev->dev;
> + device_initialize(dev);
> + dev->parent = &rpdev->dev;
> + dev->class = rpmsg_get_class();
Thank you.
Regards,
Bjorn
