Message-ID: <20200623195905.GB954398@dell>
Date:   Tue, 23 Jun 2020 20:59:05 +0100
From:   Lee Jones <lee.jones@...aro.org>
To:     Frank Rowand <frowand.list@...il.com>
Cc:     andy.shevchenko@...il.com, michael@...le.cc, robh+dt@...nel.org,
        broonie@...nel.org, devicetree@...r.kernel.org,
        linus.walleij@...aro.org, linux@...ck-us.net,
        andriy.shevchenko@...ux.intel.com, robin.murphy@....com,
        gregkh@...uxfoundation.org, linux-arm-kernel@...ts.infradead.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/3] mfd: core: Make a best effort attempt to match
 devices with the correct of_nodes

Suggestion #2

> >>>> 2) Modify patch 1/3.  The small part of the patch to modify is:
> >>>>
> >>>> +static int mfd_match_of_node_to_dev(struct platform_device *pdev,
> >>>> +				    struct device_node *np,
> >>>> +				    const struct mfd_cell *cell)
> >>>> +{
> >>>> +	struct mfd_of_node_entry *of_entry;
> >>>> +	const __be32 *reg;
> >>>> +	u64 of_node_addr;
> >>>> +
> >>>> +	/* Skip devices 'disabled' by Device Tree */
> >>>> +	if (!of_device_is_available(np))
> >>>> +		return -ENODEV;
> >>>> +
> >>>> +	/* Skip if OF node has previously been allocated to a device */
> >>>> +	list_for_each_entry(of_entry, &mfd_of_node_list, list)
> >>>>
> >>>> Change:
> >>>>
> >>>> +		if (of_entry->np == np)
> >>>> +			return -EAGAIN;
> >>>>
> >>>> To:
> >>>>
> >>>> +		if (of_entry->np == np) {
> >>>> +			if (!cell->use_of_reg)
> >>>> +				return -EINVAL;
> >>>> +			else
> >>>> +				return -EAGAIN;
> >>>>
> >>>> There may be a better choice than EINVAL, but I am just showing the method.
> >>>>
> >>>> You may also want to refactor this section of the patch slightly
> >>>> differently to achieve the same result.  It was just easiest to
> >>>> show the suggested change the way I did it.
> >>>>
> >>>> The test that returns EINVAL detects the issue that the FDT does
> >>>> not match the binding (there is more one child node with the
> >>>> "stericsson,ab8500-pwm" compatible.

My reply to suggestion #2

> >>> So here, instead of just failing a single device, we fail everything?
> >>> Sounds a lot like throwing the baby out with the bath water.  How is
> >>> that an improvement?
> > 
> > [0]
> 
> Is "[0]" the patch series, especially patch 1/3?

No, this is my reply to your suggestion #2.

The [0] is referenced further down.

[...]

> >>>  * False positives can occur and will fail as a result
> >>
> >> ((What is an example of a false positive?))  Never mind, now that
> >> I see that the previous issue is a fatal flaw, this becomes
> >> academic.
> > 
> > That's okay, I don't mind discussing.
> > 
> > Ironically, the 'ab8500-pwm' is a good example of a false positive,
> > since it's fine for the DT nodes to be identical.  So long as there
> > are nodes present for each instance, it doesn't matter which one is
> > allocated to which device .Forcing a 'reg' property onto them for no> good reason it not a valid solution here.
> 
> I thought that one of the points of this patch series was to add a
> "reg" property to any mfd child that was described by the
> OF_MFD_CELL_REG() macro.

The OF_MFD_CELL_REG() macro didn't exist until this patch-set.

There are currently no users.

> And that was meant to fix the problem where multiple indistinguishable
> children existed.  The only instance I found of that (using the
> weak search on OF_MFD_CELL()) was of compatible "stericsson,ab8500-pwm"
> in drivers/mfd/ab8500-core.c.  You agreed with my email that
> reported that.

No, I agreed with you that there is a current problem with
"stericsson,ab8500-pwm", as identified by Michael.  I didn't actually
know about this issue until *after* drafting this patch-set.  To be
clear the "stericsson,ab8500-pwm" scenario is not the reason for this
set's existence.

Also, please forget about the OF_MFD_* macros, they are totally
agnostic to this effort.  The only relevance they have here is the
addition of 1 extra macro which *could* be used to provide the 'reg'
property where appropriate.

> So I thought that drivers/mfd/ab8500-core.c would be modified to
> replace the multiple instances of compatible "stericsson,ab8500-pwm"
> in OF_MFD_CELL() with OF_MFD_CELL_REG().

That is not my vision.  There is no need for "stericsson,ab8500-pwm"
to have 'reg' properties as far as I see it.

> This is another problem with the patch series: there is no user
> of OF_MFD_CELL_REG().  Please add one to the series.

That's not a problem with this patch-set, it's a problem with your
understanding of this patch-set. :)

As far as I know, there aren't any current users who would benefit
from this work.  Instead, it is designed to provide future submitters
with another tool to help them link their child devices to the correct
OF nodes.  That's not to say that current users can't and won't
benefit from this.  Just that they are not the target audience.

> >>> The above actually makes the solution worse, not better.
> >>>
> >>
> >> Patch 1/3 silently fails to deal with a broken devicetree.
> >> It results on one of the three ab8500-pwm child nodes in
> >> the hypothetical devicetree source tree not being added.
> >>
> >> That is not a good result either.
> > 
> > No it doesn't.  In the case of 'ab8500-pwm' the OF node is not set for
> > 2 of the devices and warnings are presented in the kernel log.
> 
> OK, I was wrong about "silent".  There is a warning:
>    pr_warn("%s: Failed to locate of_node [id: %d]\n",
> 
> > The
> > device will continue to probe and function as usual.
> 
> If the device probes and functions as usual without the child of_node,
> then why does the node have any properties (for the cases of
> arch/arm/boot/dts/ste-ab8500.dtsi and arch/arm/boot/dts/ste-ab8505.dtsi
> the properties "clocks" and "clock-names").

Because DT is meant to describe the hardware, not the implementation.

DT does not know, or care that in our case most operations that happen
on the platform are passed back via an API to a central controlling
location.  Or that in reality, the OF node in this situation is
superfluous.

Can we please stop talking about the AB8500.  It doesn't have anything
to do with this series besides the fact that if it (this set) had
existed *before* 'ab8500-pwm' was OF enabled, it wouldn't now be
wonky.

> Digging through that leads to yet another related question, or actually
> sort of the same question.  Why do the child nodes with compatible
> "stericsson,ab8500-pwm" have the properties "clocks" and "clock-names"
> since the binding Documentation/devicetree/bindings/mfd/ab8500.txt
> does not list them?

If you want to talk about the AB8500, please start a new thread.

> >> OK, so my solution #3 is a no go.  How about my solution #2,
> >> which you did not comment on?
> > 
> > I did [0].  You must have missed it. :)
> 
> But yes or no to my solution #2 (with some slight changes to
> make it better (more gracious handling of the detected error) as
> discussed elsewhere in the email thread)?

Please see "[0]" above!

AFAICT your solution #2 involves bombing out *all* devices if there is
a duplicate compatible with no 'reg' property value.  This is a)
over-kill and b) not an error, as I mentioned:

> > It also suffers with false positives.

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Powered by blists - more mailing lists