Re: [PATCH] mfd: mfd-core: Change "Failed to locate of_node" warning to debug

[Lee Jones]

On Thu, 01 Jul 2021, Yunus Bas wrote:

> Am Mittwoch, dem 30.06.2021 um 13:33 +0100 schrieb Lee Jones:
> > On Wed, 30 Jun 2021, Daniel Thompson wrote:
> > 
> > > On Wed, Jun 30, 2021 at 07:27:32AM +0000, Yunus Bas wrote:
> > > > Am Dienstag, dem 29.06.2021 um 14:39 +0100 schrieb Lee Jones:
> > > > > On Tue, 29 Jun 2021, Yunus Bas wrote:
> > > > > > Interestingly, all subdevices defined in the driver are
> > > > > > registered
> > > > > > as platform devices from the MFD framework, regardless of a
> > > > > > devicetree entry or not. The preceding code checks the
> > > > > > subdevice
> > > > > > cells with an additional compatible. In case a device has no
> > > > > > devicetree entry, an irritating failed-message is printed on
> > > > > > the
> > > > > > display. I'm not sure if this was the intention but the
> > > > > > framework
> > > > > > somehow forces the users to describe all subdevices of an
> > > > > > MFD. I
> > > > > > think the info print is not needed. It makes more sense to
> > > > > > set it
> > > > > > as a debug print.
> > > > > 
> > > > > Actually, this has served to highlight that your DTS is not
> > > > > correct.
> > > > > 
> > > > > Why are some devices represented in DT and some aren't?
> > > > > 
> > > > > If anything, I'm tempted to upgrade the info() print to warn().
> > > > 
> > > > Imagine only required parts of the MFD is connected to the
> > > > designed
> > > > system and unrequired parts are not. In that case, fully
> > > > describing the
> > > > MFD in the devicetree wouldn't represent the system at all.
> > > 
> > > To describe hardware that is present but unused we would normally
> > > use
> > > status = "disabled".
> > > 
> > > So if, for example, your board cannot use the RTC for some reason
> > > (perhaps the board has no 32KHz oscillator?) then the DA9062 still
> > > contains the hardware but it is useless. Such hardware could be
> > > described as:
> > > 
> > > da9062_rtc: rtc {
> > >     compatible = "dlg,da9062-rtc";
> > >     status = "disabled";
> > > }
> > > 
> > > Is this sufficient to suppress the warnings when the hardware is
> > > not
> > > fully described?
> > > 
> > > There is almost certainly a problem here since there is a mismatch
> > > between mfd-core and the DA9062 DT bindings. mfd-core warns when
> > > the
> > > hardware description is incomplete and the DA9062 (and generic mfd)
> > > DT
> > > bindings are ambiguous about whether sub-nodes are mandatory and
> > > include
> > > an example that contains missing compatibles rather than disabled
> > > nodes
> > > like the above.
> > > 
> > > However it is not entirely clear to me at this point whether this
> > > should
> > > be fixed in mfd-core or by improving the bindings documentation.
> > 
> > Right.  This is a potential solution.
> 
> @Daniel, you hit the nail on the head :). Thank you for that.
> 
> This solution would indeed surpress the warnings, but what is the
> benefit of this? We would define never used device nodes just to
> satisfy the driver.
> 
> Also, this could lead potential users of the DTS to falsly assume that
> the defined subfunction is actually supported and only needs a change
> of status for that.
> 
> Actually I accept the fact, that changing the info() to debug() is not
> a good idea, since the driver should naturally warn in case of a
> compatible mismatch. But this should only apply for devices defined in
> the devicetree.
> 
> But regardless, if defining all the MFD subnodes in the devicetree is
> the way to go on with, it needs at least to be documented in the MFD-
> bindings.
> > 
> > However before I provide any further assistance, I really want to get
> > an idea of the H/W you're working with.  Is this a reduced function
> > DA9062?  Or is the functionality actually present, you just don't
> > want
> > to make use of it?
> > 
> No it's not a reduced version. Some functions have been deliberately
> omitted. Internally, MFD's normally have a common control register-
> sets, but the functions have separate controllers and therefor separate
> connections to the controllers. When a MFD has e.g. seven sub-functions
> and five of them are needed, then the two unneeded functions can or
> will be left out on purpose. This is common usage and can also be seen
> on other devicetrees using MFD's.

I think what you describe here is a reduced function device, no?

> > NB: The suggestion above is usually the default for devices (at least
> > this was the case back when I was neck deep in DT).  You usually have
> > the a device specified in a DTSI file with the generic properties
> > defined from within a top-level node which is usually disabled.  Then
> > you link back to that node (usually with a &) from within your DTS
> > file where you provide platform specific properties and override the
> > status to 'okay' or what have you.
> 
> Yes, that's the common way which we also use. Normally, there is a
> generic DTSI file representing a set of possible settings which is then
> specified in the DTS-file it is included.

Right, so no problem then.

Describe the full device in the DTSI file and disable all of the
functions by default.  Then, in the DTS file for your particular
platform, only provide nodes for the functionality that is present.

Explanation:

Essentially, by populating the mfd_cell, you have told MFD what to
expect from a fully functional device.  If any nodes are omitted
without explanation, it (rightfully) complains that something is
missing.

In order to describe the device properly in Device Tree, you need to
firstly describe the entire device, then only enable what is actually
present or what you wish to actually use on your H/W.

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