Re: [PATCH] mfd: mfd-core: Change "Failed to locate of_node" warning to debug
From: Lee Jones
Date: Tue Jun 29 2021 - 05:07:07 EST
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",
> },
> };
>
> >
> > 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.
>
> I think you're right here. But this is design specific and has not much
> to do with my request.
> >
> > 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.
My current understanding is that; it's far more likely that the
driver's use of the API is incorrect than the print itself. I need to
spend more time on this (time is no my friend at this present moment)
in order to fully understand what's happening though. Please bear
with me.
--
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog