Re: [PATCH v2 2/4] ASoC: mediatek: mt8183: Fix APLL enable error handling

Hi Cezary,

Thank you for your review.

I have mixed feelings about appl*_mux_setting().  Take a look at its
disable-path: if clk_set_parent() fails the follow up
clk_disable_unprepare() is skipped possibly leaving one of the clks
hanging.  I'd expect error paths of callers (of said mux_setting()
function) to ensure all the clks are disabled and unprepared
unconditionally.

Perhaps the primary purpose of the appl*_mux_setting() functions is to
configure the mux, as their names suggest, which may explain why the disable
path is currently structured this way. However, I think your point is valid.

As far as I understand, calling clk_disable_unprepare() before
clk_set_parent() should not cause any issues. Therefore, we could move both
clk_disable_unprepare() calls before changing the parent in the disable path:

------------------------------------
clk_disable_unprepare(afe_priv->clk[CLK_TOP_MUX_AUD_ENG2]);
clk_disable_unprepare(afe_priv->clk[CLK_TOP_MUX_AUD_2]);
ret = clk_set_parent(afe_priv->clk[CLK_TOP_MUX_AUD_ENG2],
                                 afe_priv->clk[CLK_CLK26M]);
if (ret) {
...
goto EXIT;
}
ret = clk_set_parent(afe_priv->clk[CLK_TOP_MUX_AUD_2],
                                afe_priv->clk[CLK_CLK26M]);
if (ret) {
...
goto EXIT;
}
-------------------------------------

To be honest, after looking at the code for other MediaTek SoCs, I found quite
a few logic issues and inconsistent clock handling. This, along with
the git log history,
makes me wonder whether these code paths were ever properly tested on
real hardware.
Therefore, I have stopped at mt8186 for now.

quoted

      /* setting for APLL */
-     apll1_mux_setting(afe, true);
+     ret = apll1_mux_setting(afe, true);
+     if (ret)
+             goto ERR_APLL1_MUX_SETTING;

Why goto? The check is valid but the label in my opinion is unnecessary.

The function could return directly here, but I used goto to keep it
consistent with
the existing error handling in this function.

quoted

+     apll1_mux_setting(afe, false);
+ERR_APLL1_MUX_SETTING:

While UPPER case for goto-labels is not part of the coding style I see
why you did it - to be cohesive with the rest of the file.

Yes, that's right. However, I noticed that the mt8186 code uses
lowercase names for goto labels,
so the style is not consistent across the file.

https://elixir.bootlin.com/linux/v7.2/source/sound/soc/mediatek/mt8186/mt8186-afe-clk.c#L303

Best regards,
Phuc