lkml.org 
[lkml]   [2021]   [Jun]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH] mfd: mfd-core: Change "Failed to locate of_node" warning to debug
Date
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@phytec.de>
> > > > ---
> > > >  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.

Regards,
Yunus

--
Mit freundlichen Grüßen
Yunus Bas

-Software Entwicklung-
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.
\
 
 \ /
  Last update: 2021-06-29 09:26    [W:0.086 / U:0.716 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site