On Mon, Oct 25, 2021 at 1:24 PM Emil Renner Berthing <kernel@xxxxxxxx> wrote: > 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: ... > > > > > > 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. That's why I said that is up to you and maintainers and people who know hardware better than me. -- With Best Regards, Andy Shevchenko
