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@xxxxxxxxxxxxxxx>
> > >
> > > 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@xxxxxxxxxxxxxxx>
> > > Signed-off-by: Jacopo Mondi <jacopo@xxxxxxxxxx>
> > > ---
> > > .../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
Attachment:
signature.asc
Description: This is a digitally signed message part
