Re: [PATCH v2 30/34] staging: vchiq_arm: Give vchiq children DT nodes

[Phil Elwell <phil@raspberrypi.com>]

Hi Nicolas,

On Tue, 5 May 2020 at 11:37, Nicolas Saenz Julienne
<nsaenzjulienne@suse.de> wrote:
>
> On Mon, 2020-05-04 at 20:42 +0100, Phil Elwell wrote:
> > Hi Nicolas,
> >
> > On 04/05/2020 18:12, Nicolas Saenz Julienne wrote:
> > > Hi Phil, Laurent,
> > >
> > > On Mon, 2020-05-04 at 12:26 +0300, Laurent Pinchart wrote:
> > > > From: Phil Elwell <phil@raspberrypi.com>
> > > >
> > > > vchiq kernel clients are now instantiated as platform drivers rather
> > > > than using DT, but the children of the vchiq interface may still
> > > > benefit from access to DT properties. Give them the option of a
> > > > a sub-node of the vchiq parent for configuration and to allow
> > > > them to be disabled.
> > > >
> > > > Signed-off-by: Phil Elwell <phil@raspberrypi.com>
> > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > > > ---
> > > >   .../staging/vc04_services/interface/vchiq_arm/vchiq_arm.c | 8 ++++++++
> > > >   1 file changed, 8 insertions(+)
> > > >
> > > > diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> > > > b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> > > > index dd3c8f829daa..2325ab825941 100644
> > > > --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> > > > +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> > > > @@ -2734,12 +2734,20 @@ vchiq_register_child(struct platform_device *pdev,
> > > > const char *name)
> > > >           pdevinfo.id = PLATFORM_DEVID_NONE;
> > > >           pdevinfo.dma_mask = DMA_BIT_MASK(32);
> > > >
> > > > + np = of_get_child_by_name(pdev->dev.of_node, name);
> > > > +
> > > > + /* Skip the child if it is explicitly disabled */
> > > > + if (np && !of_device_is_available(np))
> > > > +         return NULL;
> > >
> > > I think this is alright, although I'd reshufle the code a little so it looks
> > > nicer:
> > >
> > > +   /* Skip the child if it is explicitly disabled */
> > > +   np = of_get_child_by_name(pdev->dev.of_node, name);
> > > +   if (np && !of_device_is_available(np))
> > > +           return NULL;
> >
> > I prefer the original.
>
> Fair enough
>
> > > >           child = platform_device_register_full(&pdevinfo);
> > > >           if (IS_ERR(child)) {
> > > >                   dev_warn(&pdev->dev, "%s not registered\n", name);
> > > >                   child = NULL;
> > > >           }
> > > >
> > > > + child->dev.of_node = np;
> > >
> > > Is this really needed? I'd rather have the parent's np (as commented in
> > > patch
> > > 26) as long as this is not a real device-tree defined platform device.
> >
> > Unless the of_node pointer refers to the sub-node for the child, all children
> > would have to share a common set of properties, rather defeating the point of
> > the
> > change.
>
> Sorry I wasn't clear, my main point is that, since manually editing device
> internals is bad a practice, specially after they have been registered (there
> are potential races with dma_configure()/probe()). I want to make sure we need
> it in the first place (i.e. I don't see any further usage of that device node).
>
> If we can get rid of this line, we're better-off.

Thanks - that is much clearer.

> If we actually need the device node further down I propose two things:
> - Use dev.of_node_reused, and do an children lookup anytime you need to get a
>   property. It's a one-liner in the end.
> - Move device registration to DT. There has been some push-back of this in the
>   past, but IMO things like arm's SCMI already set a standard on what firmware
>   devices can do trough DT and it fits this situation.

I much prefer registration via DT - enumerating the children in code rather than
data always felt like a baffling step backwards.

Phil