On Mon, 18 Oct 2021 at 18:24, Andy Shevchenko [off-list ref] wrote:

On Mon, Oct 18, 2021 at 6:56 PM Emil Renner Berthing [off-list ref] wrote:

quoted

On Mon, 18 Oct 2021 at 17:48, Andy Shevchenko [off-list ref] wrote:

quoted

On Mon, Oct 18, 2021 at 6:35 PM Emil Renner Berthing [off-list ref] wrote:

quoted

On Tue, 12 Oct 2021 at 19:03, Andy Shevchenko [off-list ref] wrote:

quoted

On Tue, Oct 12, 2021 at 4:43 PM Emil Renner Berthing [off-list ref] wrote:

...

quoted

quoted

quoted

quoted

quoted

+               case PIN_CONFIG_BIAS_DISABLE:

quoted

+                       mask |= PAD_BIAS_MASK;

Use it...

quoted

+                       value = (value & ~PAD_BIAS_MASK) | PAD_BIAS_DISABLE;

...here. Ditto for the similar cases in this function and elsewhere.

I don't follow. How do you want me to use mask? If I did value =
(value & ~mask) | PAD_BIAS_DISABLE; then I'd wipe the previous
configuration. Eg. suppose the first config is the drive strength and
second disables bias. Then on the 2nd loop mask =
PAD_DRIVE_STRENGTH_MASK | PAD_BIAS_MASK and the drive strength value
would be wiped.

Collect masks and new values in temporary variables and apply them
once after the loop is done, no?

But that's exactly what the code does. It merges all the config
options into a single mask and value so we only need to do rmw on the
register once.

Then masking the value makes no sense.
What you should have is simply as

  mask |= FOO;
  value |= BAR;

Yeah, but then we could get into weird states if the device tree
specifies both bias-disable and bias-pull-up by mistake. This code is
written so that only the last valid state is chosen.

...

quoted

quoted

quoted

quoted

quoted

+       ret = clk_prepare_enable(clk);
+       if (ret) {

quoted

+               reset_control_deassert(rst);

Use devm_add_action_or_reset().

I don't see how that is better.

Pity. The rule of thumb is to either try to use devm_*() everywhere in
the probe, or don't use it at all. Above is the more-or-less standard
pattern where devn_add_action_or_reset() is being used in the entire
kernel.

quoted

Then I'd first need to call that and
check for errors, but just on the line below enabling the clock the
reset line is deasserted anyway, so then the action isn't needed any
longer. So that 3 lines of code for devm_add_action_or_reset +
lingering unneeded action or code to remove it again vs. just the line
above.

Then don't use devm_*() at all. What's the point?

I'm confused. So you wan't an unneeded action to linger because the
probe function temporarily asserts reset for 3 lines of code?

I;m talking about clk_prepare_enable().

Ok, you wrote your comment under the reset_control_deassert call. How
would devm_add_action_or_reset for clk_prepare_enable work?

...

quoted

quoted

quoted

quoted

quoted

+       sfp->gc.of_node = dev->of_node;

Isn't GPIO library do this for you?

If it does I can't find it.

Heh... `man git grep`
Hint: `git grep -n 'of_node = .*of_node' -- drivers/gpio/gpiolib*`

That's exactly what I did.

Now look at the result and find the correct place where it's done.
Btw, all hits are in the very same function.

--
With Best Regards,
Andy Shevchenko