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

[Emil Renner Berthing <kernel@xxxxxxxx>]

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:
> > > On Fri, 22 Oct 2021 at 15:32, Andy Shevchenko <andy.shevchenko@xxxxxxxxx> wrote:
> > > > On Thu, Oct 21, 2021 at 8:44 PM Emil Renner Berthing <kernel@xxxxxxxx> wrote:
> > > So I asked you if you thought it was better to leave these unused
> > > allocations when parsing the device tree node fails but you never
> > > answered that. I didn't want put words in your mouth so I could only
> > > assume you didn't. I'd really like a straight answer to that so I have
> > > something to refer to when people ask why this driver doesn't do the
> > > same as fx. the pinctrl-single. So just to be clear: do you think it's
> > > better to leave this unused garbage allocated if parsing the device
> > > tree node fails?
> >
> > If it's only one time use, I don't think it's good to have it hanging
> > around, BUT at the same time devm_*() is not suitable for such
> > allocations.
>
> 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.

> > > > > +               if (reg_din)
> > > > > +                       writel_relaxed(gpio + 2, reg_din);
> > > >
> > > > Why 0 can't be written?
> > >
> > > Because signal 0 is a special "always 0" signal and signal 1 is a
> > > special "always 1" signal, and after that signal n is the input value
> > > of GPIO n - 2. We don't want to overwrite the PoR defaults.
> >
> > Okay, this, perhaps, needs a comment (if I have not missed the existing one).
> >
> > And what about checking for reg_din? Do you have some blocks output-only?
>
> I don't know know what you mean by the first question, but yes fx. the
> uart tx pins would be an example of pins that have their output signal
> set to the uart peripheral, the output enable set to the special
> "always enabled" signal, and no input signal is set to any of the tx
> pins.
>
> > > > > +               case PIN_CONFIG_BIAS_DISABLE:
> > > > > +                       mask |= PAD_BIAS_MASK;
> > > > > +                       value = (value & ~PAD_BIAS_MASK) | PAD_BIAS_DISABLE;
> > > >
> > > > Okay, I have got why you are masking on each iteration, but here is
> > > > the question, shouldn't you apply the cnages belonged to each of the
> > > > group of options as it's requested by the user? Here you basically
> > > > ignore all previous changes to bias.
> > > >
> > > > I would expect that you have something like
> > > >
> > > > for () {
> > > >   switch (type) {
> > > >   case BIAS*:
> > > >     return apply_bias();
> > > >   ...other types...
> > > >   default:
> > > >     return err;
> > > >   }
> > > > }
> > >
> > > 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.