Hi Morimoto-san,
On Wed, May 26, 2021 at 12:48 AM Kuninori Morimoto kuninori.morimoto.gx@renesas.com wrote:
I'm not such a big fan of creating dummy clocks. And what if a future SoC lacks two CLOCKIN pins? Then you'll try to register a second dummy clock with the same name, which will fail, presumably?
I think current code will reuse same null_clk for these.
Oh right, I missed the static clk_hw pointer. What if you unload the snd-soc-rcar.ko module?
This should only be done when the clock does not exist, not in case of other errors (e.g. -EPROBE_DEFER, which isn't handled yet)?
As devm_clk_get_optional() already checks for existence, you could use:
struct clk *clk = devm_clk_get_optional(dev, clk_name[i]); if (!clk) clk = rsnd_adg_null_clk_get(priv);Ah, indeed. Thanks. I will fix it.
But in light of the above (avoiding dummy clocks), it might be more robust to make sure all code can handle adg->clk[i] = NULL?
The reason why I don't use adg->clk[i] = NULL is it is using this macro
#define for_each_rsnd_clk(pos, adg, i) \ for (i = 0; \ (i < CLKMAX) && \ ((pos) = adg->clk[i]); \ i++)The loop will stop at (A) if it was
adg->clk[0] = audio_clk_a; adg->clk[1] = audio_clk_b;(A) adg->clk[2] = NULL adg->clk[3] = audio_clk_i;
If you use this macro everywhere, that is easily handled by the following variant:
#define for_each_rsnd_clk(pos, adg, i) \ for (i = 0; (pos) = adg->clk[i], i < CLKMAX; i++) \ if (pos) { \ continue; \ } else
There are several existing examples of such a construct.
Gr{oetje,eeting}s,
Geert