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. -- With Best Regards, Andy Shevchenko
