Re: [PATCH 2/2] mfd: mfd-core: Honour Device Tree's request to disable a child-device

[Robin Murphy]

On 19/10/2019 08:28, Lee Jones wrote:
> Good morning Robin,
>
> It's been a while.  I hope that you are well.
>
> Thanks for taking an interest.
>
> On Fri, 18 Oct 2019, Robin Murphy wrote:
> > On 18/10/2019 13:26, Lee Jones wrote:
> > > Until now, MFD has assumed all child devices passed to it (via
> > > mfd_cells) are to be registered.  It does not take into account
> > > requests from Device Tree and the like to disable child devices
> > > on a per-platform basis.
> > >
> > > Well now it does.
> > >
> > > Reported-by: Barry Song <Baohua.Song@xxxxxxx>
> > > Reported-by: Stephan Gerhold <stephan@xxxxxxxxxxx>
> > > Signed-off-by: Lee Jones <lee.jones@xxxxxxxxxx>
> > > ---
> > >    drivers/mfd/mfd-core.c | 5 +++++
> > >    1 file changed, 5 insertions(+)
> > >
> > > diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c
> > > index eafdadd58e8b..24c139633524 100644
> > > --- a/drivers/mfd/mfd-core.c
> > > +++ b/drivers/mfd/mfd-core.c
> > > @@ -182,6 +182,11 @@ static int mfd_add_device(struct device *parent, int id,
> > >    	if (parent->of_node && cell->of_compatible) {
> > >    		for_each_child_of_node(parent->of_node, np) {
> > >    			if (of_device_is_compatible(np, cell->of_compatible)) {
> > > +				if (!of_device_is_available(np)) {
> > > +					/* Ignore disabled devices error free */
> > > +					ret = 0;
> > > +					goto fail_alias;
> > > +				}
> >
> > Is it possible for a device to have multiple children of the same type? If
> > so, it seems like this might not work as desired if, say, the first child
> > was disabled but subsequent ones weren't.
> >
> > It might make sense to use for_each_available_child_of_node() for the outer
> > loop, then check afterwards if anything was found.
>
> The subsystem in its current guise doesn't reliably support the
> situation you describe. We have no way to track which child nodes have
> been through this process previously, thus mfd-core will always choose
> the first child node with a matching compatible string.

Ah, OK, if that situation has never been expected to work properly then 
the simple patch is probably fine.

> If you have any suggests in terms of adding support for multiple
> children with matching compatible strings, I'd be very receptive.

I know very little about the MFD layer and its users, so I wasn't sure 
whether this 'multiple child instances' thing would ever actually be a 
real concern (other than for "simple-mfd"s which apparently don't use 
this mechanism anyway) - I was just considering the code from a pure DT 
perspective.

Cheers,
Robin.

> > >    				pdev->dev.of_node = np;
> > >    				pdev->dev.fwnode = &np->fwnode;
> > >    				break;