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

[Emil Renner Berthing]

On Tue, 9 Nov 2021 at 02:01, Linus Walleij <linus.walleij@xxxxxxxxxx> wrote:
>
> On Tue, Nov 2, 2021 at 9:08 PM Andy Shevchenko
> <andy.shevchenko@xxxxxxxxx> wrote:
> (...)
> > > On Tue, Nov 2, 2021 at 6:50 PM Emil Renner Berthing <kernel@xxxxxxxx> wrote:
> >
> > ...
> >
> > > > +static int starfive_pinconf_group_set(struct pinctrl_dev *pctldev,
> > > > +                                     unsigned int gsel,
> > > > +                                     unsigned long *configs,
> > > > +                                     unsigned int num_configs)
> > > > +{
> > > > +       struct starfive_pinctrl *sfp = pinctrl_dev_get_drvdata(pctldev);
> > > > +       const struct group_desc *group;
> > > > +       u16 mask, value;
> > > > +       int i;
> > > > +
> > > > +       group = pinctrl_generic_get_group(pctldev, gsel);
> > > > +       if (!group)
> > > > +               return -EINVAL;
> > > > +
> > > > +       mask = 0;
> > > > +       value = 0;
> > > > +       for (i = 0; i < num_configs; i++) {
> > > > +               int param = pinconf_to_config_param(configs[i]);
> > > > +               u32 arg = pinconf_to_config_argument(configs[i]);
> > > > +
> > > > +               switch (param) {
> > > > +               case PIN_CONFIG_BIAS_DISABLE:
> > > > +                       mask |= PAD_BIAS_MASK;
> > > > +                       value = (value & ~PAD_BIAS_MASK) | PAD_BIAS_DISABLE;
> > > > +                       break;
> > > > +               case PIN_CONFIG_BIAS_PULL_DOWN:
> > > > +                       if (arg == 0)
> > > > +                               return -ENOTSUPP;
> > > > +                       mask |= PAD_BIAS_MASK;
> > > > +                       value = (value & ~PAD_BIAS_MASK) | PAD_BIAS_PULL_DOWN;
> > > > +                       break;
> > > > +               case PIN_CONFIG_BIAS_PULL_UP:
> > > > +                       if (arg == 0)
> > > > +                               return -ENOTSUPP;
> > > > +                       mask |= PAD_BIAS_MASK;
> > > > +                       value = value & ~PAD_BIAS_MASK;
> > > > +                       break;
> > > > +               case PIN_CONFIG_DRIVE_STRENGTH:
> > > > +                       mask |= PAD_DRIVE_STRENGTH_MASK;
> > > > +                       value = (value & ~PAD_DRIVE_STRENGTH_MASK) |
> > > > +                               starfive_drive_strength_from_max_mA(arg);
> > > > +                       break;
> > > > +               case PIN_CONFIG_INPUT_ENABLE:
> > > > +                       mask |= PAD_INPUT_ENABLE;
> > > > +                       if (arg)
> > > > +                               value |= PAD_INPUT_ENABLE;
> > > > +                       else
> > > > +                               value &= ~PAD_INPUT_ENABLE;
> > > > +                       break;
> > > > +               case PIN_CONFIG_INPUT_SCHMITT_ENABLE:
> > > > +                       mask |= PAD_INPUT_SCHMITT_ENABLE;
> > > > +                       if (arg)
> > > > +                               value |= PAD_INPUT_SCHMITT_ENABLE;
> > > > +                       else
> > > > +                               value &= ~PAD_INPUT_SCHMITT_ENABLE;
> > > > +                       break;
> > > > +               case PIN_CONFIG_SLEW_RATE:
> > > > +                       mask |= PAD_SLEW_RATE_MASK;
> > > > +                       value = (value & ~PAD_SLEW_RATE_MASK) |
> > > > +                               ((arg << PAD_SLEW_RATE_POS) & PAD_SLEW_RATE_MASK);
> > > > +                       break;
> > > > +               case PIN_CONFIG_STARFIVE_STRONG_PULL_UP:
> > > > +                       if (arg) {
> > > > +                               mask |= PAD_BIAS_MASK;
> > > > +                               value = (value & ~PAD_BIAS_MASK) |
> > > > +                                       PAD_BIAS_STRONG_PULL_UP;
> > > > +                       } else {
> > > > +                               mask |= PAD_BIAS_STRONG_PULL_UP;
> > > > +                               value = value & ~PAD_BIAS_STRONG_PULL_UP;
> > > > +                       }
> > > > +                       break;
> > > > +               default:
> > > > +                       return -ENOTSUPP;
> > > > +               }
> > > > +       }
> > > > +
> > > > +       for (i = 0; i < group->num_pins; i++)
> > > > +               starfive_padctl_rmw(sfp, group->pins[i], mask, value);
> > > > +
> > > > +       return 0;
> > > > +}
> >
> > Linus any comments on this code (sorry if I missed your reply)? The
> > idea behind above is to skip all settings from the same category and
> > apply only the last one, e.g. if we have "bias set to X", ..., "bias
> > disable", ..., "bias set to Y", the hardware will see only the last
> > operation, i.e. "bias set to Y". I think it may not be the best
> > approach (theoretically?) since the hardware definitely may behave
> > differently on the other side in case of such series of the
> > configurations (yes, I have seen some interesting implementations of
> > the touchpad / touchscreen GPIOs that may be affected).
>
> That sounds weird. I think we need to look at how other drivers
> deal with this.
>
> To me it seems more natural that
> starfive_padctl_rmw(sfp, group->pins[i], mask, value);
> would get called at the end of each iteration of the
> for (i = 0; i < num_configs; i++) loop.

That would work, but when the loop is done the end result would be
exactly the same. The only difference is that the above would rapidly
"blink" the different states during the loop until it arrives at the
result. This would certainly be different, but it can never be the
intended behaviour and only a side-effect on how the pinctrl framework
works. The order the different states are blinked depends entirely on
how the pinctrl framework parses the device tree. I still think it
would be more natural to cleanly go to the end result without this
blinking.

/Emil