Re: [PATCH v6 03/10] rpmsg: Move the rpmsg control device from rpmsg_char to rpmsg_ctrl

[Arnaud POULIQUEN]

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
>