Re: [PATCH v2 07/16] rpmsg: char: clean up rpmsg class

From: Bjorn Andersson <bjorn.andersson@linaro.org>
To: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
Cc: Ohad Ben-Cohen <ohad@wizery.com>,
	Mathieu Poirier <mathieu.poirier@linaro.org>,
	Andy Gross <agross@kernel.org>,
	linux-remoteproc@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-stm32@st-md-mailman.stormreply.com,
	linux-arm-msm@vger.kernel.org
Subject: Re: [PATCH v2 07/16] rpmsg: char: clean up rpmsg class
Date: Mon, 4 Jan 2021 18:47:38 -0600	[thread overview]
Message-ID: <X/O3KkJrmVfGvFEF@builder.lan> (raw)
In-Reply-To: <20201222105726.16906-8-arnaud.pouliquen@foss.st.com>

On Tue 22 Dec 04:57 CST 2020, Arnaud Pouliquen wrote:

This patch doesn't "clean up" the class, as described in $subject. It
just removes it.

> Suppress the management of the rpmsg class as attribute. It is already
> handled in /sys/bus rpmsg as specified in
> documentation/ABI/testing/sysfs-bus-rpmsg.

Afaict it doesn't relate to /sys/bus/rpmsg, but rather what attributes
should be associated with the rpmsg_device (and thereby present in its
sysfs directory). But if these attributes are also added by the bus,
then why do we have this code? If that's the case this seems like a nice
cleanup that we should do outside/before merging the other patches.

> This patch prepares the migration of the control device in rpmsg_ctrl.
> 

It would be useful if the commit message described how it prepares for
the migration and why.

Regards,
Bjorn

> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
> ---
>  drivers/rpmsg/rpmsg_char.c | 48 --------------------------------------
>  1 file changed, 48 deletions(-)
> 
> diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
> index 4bbbacdbf3bb..732f5caf068a 100644
> --- a/drivers/rpmsg/rpmsg_char.c
> +++ b/drivers/rpmsg/rpmsg_char.c
> @@ -27,7 +27,6 @@
>  #define RPMSG_DEV_MAX	(MINORMASK + 1)
>  
>  static dev_t rpmsg_major;
> -static struct class *rpmsg_class;
>  
>  static DEFINE_IDA(rpmsg_ctrl_ida);
>  static DEFINE_IDA(rpmsg_ept_ida);
> @@ -291,41 +290,6 @@ static const struct file_operations rpmsg_eptdev_fops = {
>  	.compat_ioctl = compat_ptr_ioctl,
>  };
>  
> -static ssize_t name_show(struct device *dev, struct device_attribute *attr,
> -			 char *buf)
> -{
> -	struct rpmsg_eptdev *eptdev = dev_get_drvdata(dev);
> -
> -	return sprintf(buf, "%s\n", eptdev->chinfo.name);
> -}
> -static DEVICE_ATTR_RO(name);
> -
> -static ssize_t src_show(struct device *dev, struct device_attribute *attr,
> -			 char *buf)
> -{
> -	struct rpmsg_eptdev *eptdev = dev_get_drvdata(dev);
> -
> -	return sprintf(buf, "%d\n", eptdev->chinfo.src);
> -}
> -static DEVICE_ATTR_RO(src);
> -
> -static ssize_t dst_show(struct device *dev, struct device_attribute *attr,
> -			 char *buf)
> -{
> -	struct rpmsg_eptdev *eptdev = dev_get_drvdata(dev);
> -
> -	return sprintf(buf, "%d\n", eptdev->chinfo.dst);
> -}
> -static DEVICE_ATTR_RO(dst);
> -
> -static struct attribute *rpmsg_eptdev_attrs[] = {
> -	&dev_attr_name.attr,
> -	&dev_attr_src.attr,
> -	&dev_attr_dst.attr,
> -	NULL
> -};
> -ATTRIBUTE_GROUPS(rpmsg_eptdev);
> -
>  static void rpmsg_eptdev_release_device(struct device *dev)
>  {
>  	struct rpmsg_eptdev *eptdev = dev_to_eptdev(dev);
> @@ -358,9 +322,7 @@ static int rpmsg_eptdev_create(struct rpmsg_ctrldev *ctrldev,
>  	init_waitqueue_head(&eptdev->readq);
>  
>  	device_initialize(dev);
> -	dev->class = rpmsg_class;
>  	dev->parent = &ctrldev->dev;
> -	dev->groups = rpmsg_eptdev_groups;
>  	dev_set_drvdata(dev, eptdev);
>  
>  	cdev_init(&eptdev->cdev, &rpmsg_eptdev_fops);
> @@ -477,7 +439,6 @@ static int rpmsg_chrdev_probe(struct rpmsg_device *rpdev)
>  	dev = &ctrldev->dev;
>  	device_initialize(dev);
>  	dev->parent = &rpdev->dev;
> -	dev->class = rpmsg_class;
>  
>  	cdev_init(&ctrldev->cdev, &rpmsg_ctrldev_fops);
>  	ctrldev->cdev.owner = THIS_MODULE;
> @@ -553,17 +514,9 @@ static int rpmsg_char_init(void)
>  		return ret;
>  	}
>  
> -	rpmsg_class = class_create(THIS_MODULE, "rpmsg");
> -	if (IS_ERR(rpmsg_class)) {
> -		pr_err("failed to create rpmsg class\n");
> -		unregister_chrdev_region(rpmsg_major, RPMSG_DEV_MAX);
> -		return PTR_ERR(rpmsg_class);
> -	}
> -
>  	ret = register_rpmsg_driver(&rpmsg_chrdev_driver);
>  	if (ret < 0) {
>  		pr_err("rpmsgchr: failed to register rpmsg driver\n");
> -		class_destroy(rpmsg_class);
>  		unregister_chrdev_region(rpmsg_major, RPMSG_DEV_MAX);
>  	}
>  
> @@ -574,7 +527,6 @@ postcore_initcall(rpmsg_char_init);
>  static void rpmsg_chrdev_exit(void)
>  {
>  	unregister_rpmsg_driver(&rpmsg_chrdev_driver);
> -	class_destroy(rpmsg_class);
>  	unregister_chrdev_region(rpmsg_major, RPMSG_DEV_MAX);
>  }
>  module_exit(rpmsg_chrdev_exit);
> -- 
> 2.17.1
> 