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

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:

When answering, cut down your message to the point, please! It's a bit
annoying to remove overquoting...

...

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.

...

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?

...

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.

--
With Best Regards,
Andy Shevchenko