Message-ID: <9b5d0003cce92cad57e7712d1e46c78c10f1a0ab.camel@phytec.de>
Date:   Thu, 1 Jul 2021 15:34:43 +0000
From:   Yunus Bas <Y.Bas@...tec.de>
To:     "daniel.thompson@...aro.org" <daniel.thompson@...aro.org>,
        "lee.jones@...aro.org" <lee.jones@...aro.org>
CC:     "stwiss.opensource@...semi.com" <stwiss.opensource@...semi.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] mfd: mfd-core: Change "Failed to locate of_node" warning
 to debug

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.

> 
> 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.

> 
> 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.

-- 
Mit freundlichen Grüßen
Yunus Bas

-Software Engineer-
PHYTEC Messtechnik GmbH
Robert-Koch-Str. 39
55129 Mainz
Germany
Tel.: +49 (0)6131 9221- 466
Web: www.phytec.de

Sie finden uns auch auf: Facebook, LinkedIn, Xing, YouTube

PHYTEC Messtechnik GmbH | Robert-Koch-Str. 39 | 55129 Mainz, Germany
Geschäftsführer: Dipl.-Ing. Michael Mitezki, Dipl.-Ing. Bodo Huber |
Handelsregister Mainz HRB 4656 | Finanzamt Mainz | St.Nr. 266500608, DE
149059855
This E-Mail may contain confidential or privileged information. If you
are not the intended recipient (or have received this E-Mail in error)
please notify the sender immediately and destroy this E-Mail. Any
unauthorized copying, disclosure or distribution of the material in
this E-Mail is strictly forbidden.

Powered by blists - more mailing lists