diff mbox series

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

Message ID 20201222105726.16906-8-arnaud.pouliquen@foss.st.com (mailing list archive)
State New, archived
Headers show
Series introduce generic IOCTL interface for RPMsg channels management | expand

Commit Message

Arnaud Pouliquen Dec. 22, 2020, 10:57 a.m. UTC
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.
This patch prepares the migration of the control device in rpmsg_ctrl.

Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
---
 drivers/rpmsg/rpmsg_char.c | 48 --------------------------------------
 1 file changed, 48 deletions(-)

Comments

Bjorn Andersson Jan. 5, 2021, 12:47 a.m. UTC | #1
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
>
Bjorn Andersson Jan. 5, 2021, 12:54 a.m. UTC | #2
On Mon 04 Jan 18:47 CST 2021, Bjorn Andersson wrote:

> 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.
> 

Now I see what this patch does, it removes the attributes from the
character device's struct device, because they are provided by the
struct rpmsg_device's bus!

I wish your commit message made this obvious.

Also, this implies that for a few patches here rpmsg_char is just
broken - which I don't like.

Regards,
Bjorn

> 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
> >
Arnaud Pouliquen Jan. 5, 2021, 5:03 p.m. UTC | #3
On 1/5/21 1:54 AM, Bjorn Andersson wrote:
> On Mon 04 Jan 18:47 CST 2021, Bjorn Andersson wrote:
> 
>> 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.
>>
> 
> Now I see what this patch does, it removes the attributes from the
> character device's struct device, because they are provided by the
> struct rpmsg_device's bus!
> 
> I wish your commit message made this obvious.
> 
> Also, this implies that for a few patches here rpmsg_char is just
> broken - which I don't like.

I will move this patch at the end of the series and clarify commit message

Thanks
Arnaud

> 
> Regards,
> Bjorn
> 
>> 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
>>>
diff mbox series

Patch

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);