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.

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.

Regards,
Nicolas