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

[Lee Jones <lee.jones@linaro.org>]

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>;                                  
> interrupt-controller;                                                 
> gpio-controller;                                                      
> #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",
        },
  };

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'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?

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