Re: [PATCH] mfd: mfd-core: Change "Failed to locate of_node" warning to debug
From: Yunus Bas
Date: Wed Jun 30 2021 - 03:27:37 EST
Am Dienstag, dem 29.06.2021 um 14:39 +0100 schrieb Lee Jones:
> On Tue, 29 Jun 2021, Yunus Bas wrote:
>
> > Am Donnerstag, dem 17.06.2021 um 09:27 +0100 schrieb Lee Jones:
> > > On Thu, 17 Jun 2021, Yunus Bas wrote:
> > >
> > > > Hi Lee,
> > > >
> > > > Am Mittwoch, dem 16.06.2021 um 10:03 +0100 schrieb Lee Jones:
> > > > > On Wed, 16 Jun 2021, Yunus Bas wrote:
> > > > >
> > > > > > The MFD-core iterates through all subdevices of the
> > > > > > corresponding
> > > > > > MFD-device and checks, if the devicetree subnode has a
> > > > > > fitting
> > > > > > compatible.
> > > > > > When nothing is found, a warning is thrown. This can be the
> > > > > > case,
> > > > > > when it
> > > > > > is the intention to not use the MFD-device to it's full
> > > > > > content.
> > > > > > Therefore, change the warning to a debug print instead, to
> > > > > > also
> > > > > > avoid
> > > > > > irritations.
> > > > > >
> > > > > > Signed-off-by: Yunus Bas <y.bas@xxxxxxxxx>
> > > > > > ---
> > > > > > drivers/mfd/mfd-core.c | 2 +-
> > > > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-
> > > > > > core.c
> > > > > > index 6f02b8022c6d..e34c97088943 100644
> > > > > > --- a/drivers/mfd/mfd-core.c
> > > > > > +++ b/drivers/mfd/mfd-core.c
> > > > > > @@ -213,7 +213,7 @@ static int mfd_add_device(struct device
> > > > > > *parent,
> > > > > > int id,
> > > > > > }
> > > > > >
> > > > > > if (!pdev->dev.of_node)
> > > > > > - pr_warn("%s: Failed to locate
> > > > > > of_node
> > > > > > [id:
> > > > > > %d]\n",
> > > > > > + pr_debug("%s: Failed to locate
> > > > > > of_node
> > > > > > [id:
> > > > > > %d]\n",
> > > > > > cell->name, platform_id);
> > > > > > }
> > > > >
> > > > > Can you provide an example of a device tree where this is a
> > > > > problem?
> > > >
> > > > Of course, sorry for the poor description.
> > > >
> > > > Here is an example of the imx6qdl-phytec-phycore-som.dtsi which
> > > > uses
> > > > the DA9062 multi-functional device. The DA9062 has five mfd-
> > > > cell
> > > > devices with compatibles defined as subfunctions. The
> > > > devicetree
> > > > needs
> > > > and uses just three of them:
> > > >
> > > > ...
> > > > pmic: pmic@58
> > > > {
> > > > compatible =
> > > > "dlg,da9062";
> > > > pinctrl-names =
> > > > "default";
> > > > pinctrl-0 =
> > > > <&pinctrl_pmic>;
> > > > reg =
> > > > <0x58>;
> > > > interrupt-parent =
> > > > <&gpio1>;
> > > > interrupts = <2
> > > > IRQ_TYPE_LEVEL_LOW>;
> > > > #gpio-cells =
> > > > <2>;
> > > > da9062_rtc: rtc
> > > > {
> > > > compatible = "dlg,da9062-
> > > > rtc";
> > > >
> > > > da9062_onkey: onkey
> > > > {
> > > > compatible = "dlg,da9062-
> > > > onkey";
> > > > watchdog
> > > > {
> > > > compatible = "dlg,da9062-
> > > > watchdog";
> > > > dlg,use-sw-
> > > > pm;
> > > > }
> > > > ...
> > >
> > > So, looking at the mfd_cells table, I see:
> > >
> > > static const struct mfd_cell da9061_devs[] = {
> > > {
> > > .name = "da9061-core",
> > > .num_resources =
> > > ARRAY_SIZE(da9061_core_resources),
> > > .resources = da9061_core_resources,
> > > },
> > > {
> > > .name = "da9062-regulators",
> > > .num_resources =
> > > ARRAY_SIZE(da9061_regulators_resources),
> > > .resources = da9061_regulators_resources,
> > > },
> > > {
> > > .name = "da9061-watchdog",
> > > .num_resources =
> > > ARRAY_SIZE(da9061_wdt_resources),
> > > .resources = da9061_wdt_resources,
> > > .of_compatible = "dlg,da9061-watchdog",
> > > },
> > > {
> > > .name = "da9061-thermal",
> > > .num_resources =
> > > ARRAY_SIZE(da9061_thermal_resources),
> > > .resources = da9061_thermal_resources,
> > > .of_compatible = "dlg,da9061-thermal",
> > > },
> > > {
> > > .name = "da9061-onkey",
> > > .num_resources =
> > > ARRAY_SIZE(da9061_onkey_resources),
> > > .resources = da9061_onkey_resources,
> > > .of_compatible = "dlg,da9061-onkey",
> > > },
> > > };
> >
> > First of all, this is the wrong device. Further down is listed a
> > second
> > machine, the da9062, with more subdevices:
> >
> > static const struct mfd_cell da9062_devs[] = {
> > {
> > .name = "da9062-core",
> > .num_resources = ARRAY_SIZE(da9062_core_resources),
> > .resources = da9062_core_resources,
> > },
> > {
> > .name = "da9062-regulators",
> > .num_resources = ARRAY_SIZE(da9062_regulators_resources),
> > .resources = da9062_regulators_resources,
> > },
> > {
> > .name = "da9062-watchdog",
> > .num_resources = ARRAY_SIZE(da9062_wdt_resources),
> > .resources = da9062_wdt_resources,
> > .of_compatible = "dlg,da9062-watchdog",
> > },
> > {
> > .name = "da9062-thermal",
> > .num_resources = ARRAY_SIZE(da9062_thermal_resources),
> > .resources = da9062_thermal_resources,
> > .of_compatible = "dlg,da9062-thermal",
> > },
> > {
> > .name = "da9062-rtc",
> > .num_resources = ARRAY_SIZE(da9062_rtc_resources),
> > .resources = da9062_rtc_resources,
> > .of_compatible = "dlg,da9062-rtc",
> > },
> > {
> > .name = "da9062-onkey",
> > .num_resources = ARRAY_SIZE(da9062_onkey_resources),
> > .resources = da9062_onkey_resources,
> > .of_compatible = "dlg,da9062-onkey",
> > },
> > {
> > .name = "da9062-gpio",
> > .num_resources = ARRAY_SIZE(da9062_gpio_resources),
> > .resources = da9062_gpio_resources,
> > .of_compatible = "dlg,da9062-gpio",
> > },
> > };
>
> So again the issues here are -core and -regulators, right?
>
> Is that where you're seeing the warnings?
No, it's not.
>
> > > Not sure why "da9061-core" is even in there. It looks like this
> > > would
> > > be referencing itself (if this driver's name contained the "-
> > > core"
> > > element). So what from I can tell, I think this entry should
> > > actually
> > > just be removed.
> > >
> > > With regards to "da9062-regulators", this looks like an oversight
> > > at
> > > best or a Linux hack at worst. Device Tree is designed to be OS
> > > agnostic. It should describe the H/W as-is, which would include
> > > the
> > > Regulator functionality. Choosing to opt-out and instead use
> > > Linux
> > > specific systems (i.e. MFD) for device registration is a hack.
>
> So all of this is still correct.
>
> > I think you're right here. But this is design specific and has not
> > much
> > to do with my request.
>
> On the contrary.
>
> Your request is to consciously ignore the hack I describe here.
No. This is not about DA9061 or it's mfd_cell-entries at all. My
concern is about the general behaviour of the MFD-framework and how
mfd_cell entries with compatibles are initialized.
>
> > > I've always said we should not mix DT and MFD in this way.
> > >
> > > > Since the driver iterates through the mfd_cells-struct tries
> > > > matching
> > > > compatibles in the devicetree MFD node, it throws a warning
> > > > when
> > > > there
> > > > is no counterpart in the devicetree.
> > > >
> > > > In fact, I could also evalutate oder devicetree's using MFD-
> > > > devices
> > > > not
> > > > to it's full content.
> > > >
> > > > >
> > > > > Probably worth popping that in the commit message too.
> > > >
> > > > Yes, I will send a v2 ASAP. Thank you for the advice.
> > >
> > > I think the current code is fine as it is.
> > >
> > > It's the implementation that needs to change.
> > >
> > > Maybe Steve would like to comment?
> > >
> >
> > The problem I want to address lies in the mfd_add_device function
> > in
> > the mfd-core:
> >
> > if (IS_ENABLED(CONFIG_OF) && parent->of_node && cell-
> > > of_compatible) {
> > for_each_child_of_node(parent->of_node, np)
> > {
> > if (of_device_is_compatible(np, cell->of_compatible))
> > {
> > /* Ignore 'disabled' devices error free
> > */
> > if (!of_device_is_available(np))
> > {
> > ret =
> > 0;
> > goto
> > fail_alias;
> >
> > }
> >
> >
> > ret = mfd_match_of_node_to_dev(pdev, np,
> > cell);
> > if (ret == -
> > EAGAIN)
> >
> > continue;
> > if
> > (ret)
> > goto
> > fail_alias;
> >
> >
> >
> > break;
> >
> > }
> >
> > }
> >
> >
> > if (!pdev-
> > >dev.of_node)
> > pr_info("%s: Failed to locate of_node [id:
> > %d]\n",
> > cell->name,
> > platform_id);
> > }
> >
> > 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.
Actually it would make more sense to check if mfd_cell-entries with
compatibles are represented in the devicetree and add them after
matching. This way the warning would serve it's purpose. What do you
think of it?
--
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.