On Tue, Nov 2, 2021 at 9:08 PM Andy Shevchenko
[off-list ref] wrote:
(...)

quoted

On Tue, Nov 2, 2021 at 6:50 PM Emil Renner Berthing [off-list ref] wrote:

...

quoted

quoted

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

Yours,
Linus Walleij