lkml.org 
[lkml]   [2021]   [Apr]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v2 5/7] rpmsg: char: Introduce a rpmsg driver for the rpmsg char device
    On Thu, Apr 22, 2021 at 09:58:27AM +0200, Arnaud POULIQUEN wrote:
    > On 4/21/21 7:40 PM, Mathieu Poirier wrote:
    > > Good day Arnaud,
    > >
    > > On Tue, Apr 13, 2021 at 03:44:56PM +0200, Arnaud Pouliquen wrote:
    > >> A rpmsg char device allows to probe the endpoint device on a remote name
    > >> service announcement.
    > >>
    > >> With this patch the /dev/rpmsgX interface is created either by a user
    > >> application or by the remote firmware.
    > >>
    > >> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
    > >>
    > >> ---
    > >> update from V1:
    > >> - add missing unregister_rpmsg_driver call on module exit.
    > >>
    > >> ---
    > >> drivers/rpmsg/rpmsg_char.c | 59 +++++++++++++++++++++++++++++++++++++-
    > >> 1 file changed, 58 insertions(+), 1 deletion(-)
    > >>
    > >> diff --git a/drivers/rpmsg/rpmsg_char.c b/drivers/rpmsg/rpmsg_char.c
    > >> index a64249d83172..4606787b7011 100644
    > >> --- a/drivers/rpmsg/rpmsg_char.c
    > >> +++ b/drivers/rpmsg/rpmsg_char.c
    > >> @@ -26,6 +26,8 @@
    > >> #include "rpmsg_char.h"
    > >> #include "rpmsg_internal.h"
    > >>
    > >> +#define RPMSG_CHAR_DEVNAME "rpmsg-raw"
    > >> +
    > >
    > > Why not simply call it rpmsg-char?
    >
    > I would avoid to link the rpmsg name service to the Linux Kernel device.

    To me that's exactly what we want to do... Am I missing something?

    >
    > >
    > >> static dev_t rpmsg_major;
    > >>
    > >> static DEFINE_IDA(rpmsg_ept_ida);
    > >> @@ -403,13 +405,67 @@ int rpmsg_chrdev_create_eptdev(struct rpmsg_device *rpdev, struct device *parent
    > >> }
    > >> EXPORT_SYMBOL(rpmsg_chrdev_create_eptdev);
    > >>
    > >> +static int rpmsg_chrdev_probe(struct rpmsg_device *rpdev)
    > >> +{
    > >> + struct rpmsg_channel_info chinfo;
    > >> + struct rpmsg_eptdev *eptdev;
    > >> +
    > >> + if (!rpdev->ept)
    > >> + return -EINVAL;
    > >> +
    > >> + memcpy(chinfo.name, RPMSG_CHAR_DEVNAME, sizeof(RPMSG_CHAR_DEVNAME));
    > >> + chinfo.src = rpdev->src;
    > >> + chinfo.dst = rpdev->dst;
    > >> +
    > >> + eptdev = __rpmsg_chrdev_create_eptdev(rpdev, &rpdev->dev, chinfo, NULL);
    > >> + if (IS_ERR(eptdev))
    > >> + return PTR_ERR(eptdev);
    > >> +
    > >> + /* Set the private field of the default endpoint to retrieve context on callback. */
    > >> + rpdev->ept->priv = eptdev;
    > >
    > > This is already done in rpmsg_create_ept() when rpmsg_eptdev_open() is called.
    > >
    > >> +
    > >> + return 0;
    > >> +}
    > >> +
    > >> +static void rpmsg_chrdev_remove(struct rpmsg_device *rpdev)
    > >> +{
    > >> + int ret;
    > >> +
    > >> + ret = device_for_each_child(&rpdev->dev, NULL, rpmsg_chrdev_destroy_eptdev);
    > >> + if (ret)
    > >> + dev_warn(&rpdev->dev, "failed to destroy endpoints: %d\n", ret);
    > >> +}
    > >> +
    > >> +static struct rpmsg_device_id rpmsg_chrdev_id_table[] = {
    > >> + { .name = RPMSG_CHAR_DEVNAME },
    > >> + { },
    > >> +};
    > >> +
    > >> +static struct rpmsg_driver rpmsg_chrdev_driver = {
    > >> + .probe = rpmsg_chrdev_probe,
    > >> + .remove = rpmsg_chrdev_remove,
    > >> + .id_table = rpmsg_chrdev_id_table,
    > >> + .callback = rpmsg_ept_cb,
    > >
    > > Not sure why we need a callback associated to this driver when
    > > rpmsg_eptdev_open() already creates and rpmsg_endpoint. To me the only thing
    > > having a callback provides is the association between the rpmsg_device and the
    > > rpmsg_endpoint[1] that happens in rpmsg_dev_probe(). The QC folks already do
    > > this association in their platform code[2]. Since this is not done in
    > > __rpmsg_create_ept() a check for rpdev->ept == NULL could be done in
    > > rpmsg_eptdev_open() and do the assignment there.
    > >
    > > [1]. https://elixir.bootlin.com/linux/v5.12-rc6/source/drivers/rpmsg/rpmsg_core.c#L513
    > > [2]. https://elixir.bootlin.com/linux/v5.12-rc6/source/drivers/rpmsg/qcom_glink_native.c#L1623
    > >
    >
    > That's a good point! When I started the redesign, I faced some issues with the
    > approach you propose. But as I can not remember the reason and because the code
    > has evolved, i need to re-think about this.
    >

    Glad to see we're on the same page. I stared at this code for a very long time,
    thinking there was some kind of bigger picture I wasn't getting.


    > Thanks,
    > Arnaud
    >
    >
    > >> + .drv = {
    > >> + .name = "rpmsg_chrdev",
    > >> + },
    > >> +};
    > >> +
    > >> static int rpmsg_chrdev_init(void)
    > >> {
    > >> int ret;
    > >>
    > >> ret = alloc_chrdev_region(&rpmsg_major, 0, RPMSG_DEV_MAX, "rpmsg_char");
    > >> - if (ret < 0)
    > >> + if (ret < 0) {
    > >> pr_err("rpmsg: failed to allocate char dev region\n");
    > >> + return ret;
    > >> + }
    > >> +
    > >> + ret = register_rpmsg_driver(&rpmsg_chrdev_driver);
    > >> + if (ret < 0) {
    > >> + pr_err("rpmsg: failed to register rpmsg raw driver\n");
    > >> + unregister_chrdev_region(rpmsg_major, RPMSG_DEV_MAX);
    > >> + }
    > >>
    > >> return ret;
    > >> }
    > >> @@ -417,6 +473,7 @@ postcore_initcall(rpmsg_chrdev_init);
    > >>
    > >> static void rpmsg_chrdev_exit(void)
    > >> {
    > >> + unregister_rpmsg_driver(&rpmsg_chrdev_driver);
    > >> unregister_chrdev_region(rpmsg_major, RPMSG_DEV_MAX);
    > >> }
    > >> module_exit(rpmsg_chrdev_exit);
    > >> --
    > >> 2.17.1
    > >>

    \
     
     \ /
      Last update: 2021-04-22 18:37    [W:2.196 / U:0.108 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site