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

[Yunus Bas <Y.Bas@phytec.de>]

Am Dienstag, dem 29.06.2021 um 10:07 +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@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.
> 
> 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.

Oh sorry, it wasn't my intention to push you :). Please take your time.


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