Re: [PATCH 3/3] ASoC: rsnd: add null CLOCKIN support

[Geert Uytterhoeven <geert@linux-m68k.org>]

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

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds