Messages in this thread | | | From | Martin Blumenstingl <> | Date | Fri, 4 Nov 2022 22:02:02 +0100 | Subject | Re: [PATCH] net: stmmac: dwmac-meson8b: fix meson8b_devm_clk_prepare_enable() |
| |
Hello Rasmus,
Thanks for your patch! It would be great if you could Cc linux-amlogic@lists.infradead.org in v2 of this patch, so other Amlogic SoC maintainers can also provide their feedback.
On Fri, Nov 4, 2022 at 9:30 AM Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote: > > There are two problems with meson8b_devm_clk_prepare_enable(), > introduced in a54dc4a49045 (net: stmmac: dwmac-meson8b: Make the clock > enabling code re-usable): > > - It doesn't pass the clk argument, but instead always the > rgmii_tx_clk of the device. Indeed, this is a problem - thanks for fixing this!
> - It silently ignores the return value of devm_add_action_or_reset(). This second part is not so easy. The thought process back when this code was implemented was: let's continue loading of the driver even if devm_add_action_or_reset() fails as this just means during shutdown/rmmod we don't disable all clocks. If we want to propagate the error code returned by devm_add_action_or_reset() then we also need to do the clean up within meson8b_devm_clk_prepare_enable(), meaning we need to call clk_disable_unprepare() in case devm_add_action_or_reset() failed. Your change just propagates the error code without disabling and unpreparing the clock.
[...] > The latter means the callers could > end up with the clock not actually prepared/enabled. In my opinion this statement is not correct: even if devm_add_action_or_reset() fails the clock will be prepared and enabled (by clk_prepare_enable() right before).
Personally I would just change the clk argument and keep the return 0. What do you think?
Best regards, Martin
| |