On Thu, Apr 22, 2021 at 09:56:41AM +0200, Arnaud POULIQUEN wrote:
>
>
> On 4/21/21 8:04 PM, Mathieu Poirier wrote:
> > On Tue, Apr 13, 2021 at 03:44:53PM +0200, 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>
> >> ---
> >> update from v1:
> >> - keep "rpmsg_chrdev" driver name in rpmsg_ctrl, driver will be renamed
> >> in next path that renames the rpmsg_chrdev_create_eptdev.
> >> - rename the chardev regions
> >> - move RPMSG_DEV_MAX to rpmsg_char.h
> >> ---
> >> drivers/rpmsg/Kconfig | 9 ++
> >> drivers/rpmsg/Makefile | 1 +
> >> drivers/rpmsg/rpmsg_char.c | 181 +----------------------------
> >> drivers/rpmsg/rpmsg_char.h | 2 +
> >> drivers/rpmsg/rpmsg_ctrl.c | 231 +++++++++++++++++++++++++++++++++++++
> >> 5 files changed, 245 insertions(+), 179 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
>
> snip[...]
>
> >> +static int rpmsg_ctrldev_init(void)
> >> +{
> >> + int ret;
> >> +
> >> + ret = alloc_chrdev_region(&rpmsg_major, 0, RPMSG_DEV_MAX, "rpmsg_ctrl");
> >> + if (ret < 0) {
> >> + pr_err("rpmsg: failed to allocate char dev region\n");
> >> + return ret;
> >> + }
> >> +
> >> + rpmsg_class = class_create(THIS_MODULE, "rpmsg");
> >
> > This class thing really bothers me. Keeping this here means that rpmsg_eptdevs
> > created from user space will be associated to this rpmsg_class but those created
> > from the name service won't. As such I propose to move this to rpmsg_char and
> > simply not associate the control device to the class.
> >
> > Otherwise we'd have to introduce some mechanic only to deal with the creation of
> > the class and I don't think it is worth. We can revise that approach if someone
> > complains we broke their user space.
>
> I agree with that as it was my first proposed approach here [1]
Yeah, sorry about that. This patch review process is not an exact science...
>
> [1] https://www.spinics.net/lists/linux-arm-msm/msg81194.html
>
> Thanks,
> Arnaud
>
> >
> >
> >> + if (IS_ERR(rpmsg_class)) {
> >> + pr_err("failed to create rpmsg class\n");
> >> + ret = PTR_ERR(rpmsg_class);
> >> + goto free_region;
> >> + }
> >> +
> >> + ret = register_rpmsg_driver(&rpmsg_ctrldev_driver);
> >> + if (ret < 0) {
> >> + pr_err("rpmsg ctrl: failed to register rpmsg driver\n");
> >> + goto free_class;
> >> + }
> >> +
> >> + return 0;
> >> +
> >> +free_class:
> >> + class_destroy(rpmsg_class);
> >> +free_region:
> >> + unregister_chrdev_region(rpmsg_major, RPMSG_DEV_MAX);
> >> +
> >> + return ret;
> >> +}
> >> +postcore_initcall(rpmsg_ctrldev_init);
> >> +
> >> +static void rpmsg_ctrldev_exit(void)
> >> +{
> >> + unregister_rpmsg_driver(&rpmsg_ctrldev_driver);
> >> + class_destroy(rpmsg_class);
> >> + unregister_chrdev_region(rpmsg_major, RPMSG_DEV_MAX);
> >> +}
> >> +module_exit(rpmsg_ctrldev_exit);
> >> +
> >> +MODULE_DESCRIPTION("rpmsg control interface");
> >> +MODULE_ALIAS("rpmsg:" KBUILD_MODNAME);
> >> +MODULE_LICENSE("GPL v2");
> >> --
> >> 2.17.1
> >>
