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