[...]
> > It seems to me that the main point to step forward is to clarify the global
> > design and features of the rpmsg-ctrl.
> > Depending on the decision taken, this series could be trashed and rewritten
> > from
> > a blank page...To not lost to much time on the series don't hesitate to
> > limit
> > the review to the minimum.
> >
>
> I doubt you will ever get clear guidelines on the whole solution. I will get
> back to you once I am done with the SMD driver, which should be in the
> latter part of next week.
>
After looking at the rpmsg_chrdev driver, its current customers (i.e the Qcom
drivers), the rpmsg name service and considering the long term goals of this
patchset I have the following guidelines:
1) I thought long and hard about how to split the current rpmsg_chrdev driver
between the control plane and the raw device plane and the end solution looks
much slimpler than I expected. Exporting function rpmsg_eptdev_create() after
moving it to another file (along with other dependencies) should be all we need.
Calling rpmsg_eptdev_create() from rpmsg_ctrldev_ioctl() will automatically load
the new driver, the same way calling rpmsg_ns_register_device() from
rpmsg_probe() took care of loading the rpmsg_ns driver.
2) While keeping the control plane functionality related to
RPMSG_CREATE_EPT_IOCTL intact, introduce a new RPMSG_CREATE_DEV_IOCTL that will
allow for the instantiation of rpmsg_devices, exactly the same way a name
service
announcement from a remote processor does. I envision that code path to
eventually call rpmsg_create_channel().
3) Leave the rpmsg_channel_info structure intact and use the
rpmsg_channel_info::name to bind to a rpmsg_driver, exactly how it is currently
done for name service driver selection. That will allow us to re-use the
current rpmsg_bus intrastructure, i.e rpmsg_bus::match(), without having to deal
with yet another bus type. Proceeding this way gives us the opportunity to keep
the current channel name convention for other rpmch_chrdev users untouched.
4) In a prior conversation you indicated the intention of instantiating the
rpmsg_chrdev from the name service interface. I agree with doing so but
conjugating that with the RPMSG_CHAR kenrel define may be tricky. I will wait
to see what you come up with.
I hope this helps.
Thanks,
Mathieu
> > Thanks,
> > Arnaud
> >
> > >
> > > Thanks,
> > > Mathieu
> > >
> > >> + return NULL;
> > >> +}
> > >> +
> > >> static long rpmsg_ctrl_dev_ioctl(struct file *fp, unsigned int cmd,
> > >> unsigned long arg)
> > >> {
> > >> struct rpmsg_ctrl_dev *ctrldev = fp->private_data;
> > >> -
> > >> - dev_info(&ctrldev->dev, "Control not yet implemented\n");
> > >> + void __user *argp = (void __user *)arg;
> > >> + struct rpmsg_channel_info chinfo;
> > >> + struct rpmsg_endpoint_info eptinfo;
> > >> + struct rpmsg_device *newch;
> > >> +
> > >> + if (cmd != RPMSG_CREATE_EPT_IOCTL)
> > >> + return -EINVAL;
> > >> +
> > >> + if (copy_from_user(&eptinfo, argp, sizeof(eptinfo)))
> > >> + return -EFAULT;
> > >> +
> > >> + /*
> > >> + * In a frst step only the rpmsg_raw service is supported.
> > >> + * The override is foorced to RPMSG_RAW_SERVICE
> > >> + */
> > >> + chinfo.driver_override =
> > >> rpmsg_ctrl_get_drv_name(RPMSG_RAW_SERVICE);
> > >> + if (!chinfo.driver_override)
> > >> + return -ENODEV;
> > >> +
> > >> + memcpy(chinfo.name, eptinfo.name, RPMSG_NAME_SIZE);
> > >> + chinfo.name[RPMSG_NAME_SIZE - 1] = '\0';
> > >> + chinfo.src = eptinfo.src;
> > >> + chinfo.dst = eptinfo.dst;
> > >> +
> > >> + newch = rpmsg_create_channel(ctrldev->rpdev, &chinfo);
> > >> + if (!newch) {
> > >> + dev_err(&ctrldev->dev, "rpmsg_create_channel failed\n");
> > >> + return -ENXIO;
> > >> + }
> > >>
> > >> return 0;
> > >> };
> > >> --
> > >> 2.17.1
> > >>
