Re: [PATCH v9 4/4] remoteproc: virtio: Create platform device for the remoteproc_virtio

[Rob Herring]

On Tue, Oct 4, 2022 at 10:18 AM Arnaud POULIQUEN
<arnaud.pouliquen@xxxxxxxxxxx> wrote:
>
> Hello Rob,
>
> On 10/4/22 16:39, Rob Herring wrote:
> > On Wed, Sep 21, 2022 at 03:50:44PM +0200, Arnaud Pouliquen wrote:
> >> Define a platform driver to manage the remoteproc virtio device as
> >> a platform devices.
> >>
> >> The platform device allows to pass rproc_vdev_data platform data to
> >> specify properties that are stored in the rproc_vdev structure.
> >>
> >> Such approach will allow to preserve legacy remoteproc virtio device
> >> creation but also to probe the device using device tree mechanism.
> >>
> >> remoteproc_virtio.c update:
> >>   - Add rproc_virtio_driver platform driver. The probe ops replaces
> >>     the rproc_rvdev_add_device function.
> >>   - All reference to the rvdev->dev has been updated to rvdev-pdev->dev.
> >>   - rproc_rvdev_release is removed as associated to the rvdev device.
> >>   - The use of rvdev->kref counter is replaced by get/put_device on the
> >>     remoteproc virtio platform device.
> >>   - The vdev device no longer increments rproc device counter.
> >>     increment/decrement is done in rproc_virtio_probe/rproc_virtio_remove
> >>     function in charge of the vrings allocation/free.
> >>
> >> remoteproc_core.c update:
> >>   Migrate from the rvdev device to the rvdev platform device.
> >>   From this patch, when a vdev resource is found in the resource table
> >>   the remoteproc core register a platform device.
> >>
> >> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@xxxxxxxxxxx>
> >> Reviewed-by: Mathieu Poirier <mathieu.poirier@xxxxxxxxxx>
> >> ---
> >>  drivers/remoteproc/remoteproc_core.c     |  12 +-
> >>  drivers/remoteproc/remoteproc_internal.h |   2 -
> >>  drivers/remoteproc/remoteproc_virtio.c   | 143 ++++++++++++-----------
> >>  include/linux/remoteproc.h               |   6 +-
> >>  4 files changed, 82 insertions(+), 81 deletions(-)
> >
> > [...]
> >
> >> +/* Platform driver */
> >> +static const struct of_device_id rproc_virtio_match[] = {
> >> +    { .compatible = "virtio,rproc" },
> >
> > This is not documented. Add a binding schema if you need DT support.
>
>
> Mathieu also pointed this out to me in V8, you can see the discussion here [1]
>
> Here is an extract:
> "Yes I saw the warning, but for this first series it is not possible to declare
> the associated "rproc-virtio" device in device tree.
> So at this step it seems not make senses to create the devicetree bindings file.
> More than that I don't know how I could justify the properties in bindings if
> there is not driver code associated.
>
> So i would be in favor of not adding the bindings in this series but to define
> bindings in the first patch of my "step 2" series; as done on my github:
> https://github.com/arnopo/linux/commit/9616d89a4f478cf78865a244efcde108d900f69f
> "

Okay, since I only just started checking this (in a more reliable way
than checkpatch does).

But why do you need the DT match entry if it is not usable yet? You
could just add that in later when the binding is defined. Review of
the binding could say that 'virtio,rproc' should be something else and
you'd have to change it.

Rob