RE: [PATCH] pinctrl: rzn1: Fix check for used MDIO bus

[Phil Edworthy]

Hi Geert,

On 23 November 2018 10:16 Geert Uytterhoeven wrote:
> On Fri, Nov 23, 2018 at 11:07 AM Phil Edworthy wrote:
> > On 23 November 2018 09:41 Geert Uytterhoeven wrote:
> > > Subject: Re: [PATCH] pinctrl: rzn1: Fix check for used MDIO bus On
> > > Mon, Nov 19, 2018 at 5:18 PM Phil Edworthy wrote:
> > > > This fixes the check for unused mdio bus setting and the following
> > > >static checker warning:
> > > >  drivers/pinctrl/pinctrl-rzn1.c:198 rzn1_pinctrl_mdio_select()
> > > >  warn: always true condition '(ipctl->mdio_func[mdio] >= 0) =>
> > > >(0-u32max = 0)'
> > > >
> > > > It also fixes the return var when calling of_get_child_count()
> > >
> > > I think this should be a separate patch.
> > Ok, I'll split them.
> 
> Thanks!
> 
> > > BTW, I have a question about rzn1_pinctrl_mdio_select():
> > >
> > > static void rzn1_pinctrl_mdio_select(struct rzn1_pinctrl *ipctl, int mdio,
> > >                                      u32 func) {
> > >         if (ipctl->mdio_func[mdio] >= 0 && ipctl->mdio_func[mdio] != func)
> > >                 dev_warn(ipctl->dev, "conflicting setting for mdio%d!\n", mdio);
> > >         ipctl->mdio_func[mdio] = func;
> > >
> > >         dev_dbg(ipctl->dev, "setting mdio%d to %u\n", mdio, func);
> > >
> > >         writel(func, &ipctl->lev2->l2_mdio[mdio]); }
> > >
> > > The check warns the user if it overrides an already initialized MDIO
> > > function with a different value.
> > > However, there is no method to uninitialize (reset to -1)
> > > mdio_func[], to avoid getting the warning.
> > >
> > > For a use case, I was thinking about a DT overlay that would cause
> > > the MDIO function to be initialized on loading, and needs to
> > > uninitialize the MDIO function on removing.
> > >
> > > Perhaps that is very unlikely or even impossible, given the function
> > > of the pins controlled by the MDIO function?
> > I hadn't considered that DT overlay possibility...
> > Since this MDIO muxing selects one of several different IP blocks as
> > the MDIO master, I guess it could happen. However, this is pretty unlikely!
> >
> > I can't see any way via the pinctrl_ops or pinconf_ops to 'undo' a pin
> > setting, how would this work?
> 
> Actually the pinctrl core wouldn't undo the configuration on DT overlay
> unload, but would just do the new configuration when loading the new DT
> overlay.
> Hence that would print the warning, but work regardless.
> Only for GPIOs there would be an undo step (freeing the requested GPIO).
> 
> > If a DT overlay causes remove() then probe() to be called again, the
> > driver resets mdio_func[] in probe(), so it'll work.
> 
> Typically a DT overlay would only control I/O devices, not the actual pinctrl
> device, so the pin controller driver's .probe() wouldn't be called.
> 
> But I agree it's unlikely and rare, and would still work. And probably you do
> want to keep the warning.  DT overlays are still experimental, as there's no
> upstream support for loading a random DT overlay at runtime.

Ok, I'll leave it as it is then. If it ever becomes an issue, we can look again.

Thanks
Phil

> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-
> m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds