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

Hi Morimoto-san,

On Thu, May 27, 2021 at 12:06 AM Kuninori Morimoto kuninori.morimoto.gx@renesas.com wrote:

...

Oh right, I missed the static clk_hw pointer. What if you unload the snd-soc-rcar.ko module?

Hmm.. indeed. It needs something.. Thank you for poining it.

...

#define for_each_rsnd_clk(pos, adg, i)          \
        for (i = 0; (pos) = adg->clk[i], i < CLKMAX; i++)            \
                if (pos) {                      \
                        continue;               \
                } else

Wow!! I didn't know this technique. Indeed it can use NULL pointer.

But, I want to avoid "if (pos) else" code as much as possible, and keep simple code. It can handle all clk case without thinking it if it has null_clk.

Why you don't want null_clk ??

It adds a dummy object, which needs to be cleaned up. Basically you are trading a simple NULL pointer check for a zero clock rate check deeper inside the driver, with the additional burden of needing to take care of the dummy clock's life cycle.

Note that most clk_*() calls happily operate on a NULL pointer, and just return success. This includes clk_get_rate(), which returns a zero rate.

Mark might have a different view, though, due to his experience with dummy regulators?

Gr{oetje,eeting}s,

Geert