On 11/1/21 6:07 PM, Bjorn Andersson wrote:
> 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.
That's true, I will fix this
>
>> +
>> #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?
>
Mainly because i created a new file here, but right it is a copy-past, so
I will remove it.
Thanks,
Arnaud
>> + * 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
>
