Re: [PATCH v2 12/16] pinctrl: starfive: Add pinctrl driver for StarFive SoCs

[Emil Renner Berthing <kernel@xxxxxxxx>]

On Mon, 25 Oct 2021 at 12:16, Andy Shevchenko <andy.shevchenko@xxxxxxxxx> wrote:
> On Sun, Oct 24, 2021 at 12:29 PM Emil Renner Berthing <kernel@xxxxxxxx> wrote:
> > On Sat, 23 Oct 2021 at 23:02, Emil Renner Berthing <kernel@xxxxxxxx> wrote:
> > > On Sat, 23 Oct 2021 at 22:29, Andy Shevchenko <andy.shevchenko@xxxxxxxxx> wrote:
> > > > On Sat, Oct 23, 2021 at 9:46 PM Emil Renner Berthing <kernel@xxxxxxxx> wrote:
>
> ...
>
> > > So is that a yes or a no to my question? It's not clear to me.
> >
> > I see now that you've probably misunderstood what the code does. It's
> > not one time use. The function parses the device tree and dynamically
> > registers groups and functions with the pinctrl framework. Each group
> > needs a string name, an int array of pins and optionally the pinmux
> > data. Once the group is registered those pieces of data needs to live
> > with the group until the drive is unloaded. But if the device tree
> > parsing fails before the group is registered then those allocations
> > would never be referenced and just hang around as garbage until the
> > driver is unloaded. In such cases fx. pinctrl-single uses devm_free to
> > free them again.
>
> Thank you for elaboration. Please, drop devm_*(). In this case it's
> inappropriate to use it. pinctrl-single should be amended accordingly,
> but it's out of scope here.
>
> ...
>
> > > > > I such cases where you get conflicting PIN_CONFIG_BIAS_* settings I
> > > > > don't see why it's better to do the rmw on the padctl register for the
> > > > > first bias setting only to then change the bits again a few
> > > > > microseconds later when the loop encounters the second bias setting.
> > > > > After the loop is done the end result would still be just the last
> > > > > bias setting.
> > > >
> > > > It could be bias X followed by something else followed by bias Y. You
> > > > will write something else with bias Y. I admit I don't know this
> > > > hardware and you and maintainers are supposed to decide what's better,
> > > > but my guts are telling me that current algo is buggy.
> > >
> > > So there is only one padctl register pr. pin. I don't see why first
> > > setting the bias bits to X, then setting some other bits, and then
> > > setting the bias bits to Y would be different from just setting all
> > > the bits in one go. Except for during that little microsecond window
> > > during the loop that I actually think it's better to avoid.
> >
> > Maybe an example is in order. Suppose we get strong pull-up, drive
> > strength 3 and pull-down config flags (the strong pull-up and pull
> > down flags conflict) and the padctl value is 0x0c0 (pull-up, input and
> > schmitt trigger enabled). With your solution of just altering the
> > padctl bits immediately we'd call starfive_padctl_rmw 3 times in rapid
> > succession like this:
> >
> > starfive_padctl_rmw(pin, 0x130, 0x100);
> > starfive_padctl_rmw(pin, 0x007, 0x003);
> > starfive_padctl_rmw(pin, 0x130, 0x010);
> >
> > ..and the end result would be 0x0d3, although the strong pull-up would
> > be enabled for the microseconds between the 1st and 3nd call.
> > As the code is now it'd just directly do
> >
> > starfive_padctl_rmw(pin, 0x137, 0x013)
> >
> > ..which again results in 0x0d3, only without the microsecond blink of
> > the strong pull-up.
>
> You missed the point. Hardware on the other end may behave well
> differently in these two cases.

Right, but that can never be an intended behaviour. Which of the
conflicting bias settings comes first and is blipped before the 2nd
remains entirely depends on how the pinctrl framework parses the
devicetree. I'd much rather have it cleanly go to just one of the
states, which might be the wrong one, but the conflicting bias
settings are wrong to begin with.