Hi Morimoto-san,
On Mon, May 24, 2021 at 8:12 AM Kuninori Morimoto
<kuninori.morimoto.gx@xxxxxxxxxxx> wrote:
> From: Kuninori Morimoto <kuninori.morimoto.gx@xxxxxxxxxxx>
>
> Some Renesas SoC doesn't have full CLOCKIN.
> This patch add null_clk, and accepts it.
>
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@xxxxxxxxxxx>
Thanks for your patch, which is now commit d6956a7dde6fbf84 ("ASoC:
rsnd: add null CLOCKIN support") in asoc/for-next.
]> --- a/sound/soc/sh/rcar/adg.c
> +++ b/sound/soc/sh/rcar/adg.c
> @@ -389,6 +389,30 @@ void rsnd_adg_clk_control(struct rsnd_priv *priv, int enable)
> }
> }
>
> +#define NULL_CLK "rsnd_adg_null"
> +static struct clk *rsnd_adg_null_clk_get(struct rsnd_priv *priv)
> +{
> + static struct clk_hw *hw;
> + struct device *dev = rsnd_priv_to_dev(priv);
> +
> + if (!hw) {
> + struct clk_hw *_hw;
> + int ret;
> +
> + _hw = clk_hw_register_fixed_rate_with_accuracy(dev, NULL_CLK, NULL, 0, 0, 0);
> + if (IS_ERR(_hw))
> + return NULL;
> +
> + ret = of_clk_add_hw_provider(dev->of_node, of_clk_hw_simple_get, _hw);
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?
> + if (ret < 0)
> + clk_hw_unregister_fixed_rate(_hw);
> +
> + hw = _hw;
> + }
> +
> + return clk_hw_get_clk(hw, NULL_CLK);
> +}
> +
> static void rsnd_adg_get_clkin(struct rsnd_priv *priv,
> struct rsnd_adg *adg)
> {
> @@ -398,7 +422,12 @@ static void rsnd_adg_get_clkin(struct rsnd_priv *priv,
> for (i = 0; i < CLKMAX; i++) {
> struct clk *clk = devm_clk_get(dev, clk_name[i]);
>
> - adg->clk[i] = IS_ERR(clk) ? NULL : clk;
> + if (IS_ERR(clk))
> + clk = rsnd_adg_null_clk_get(priv);
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);
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?
> + if (IS_ERR(clk))
> + dev_err(dev, "no adg clock (%s)\n", clk_name[i]);
> +
> + adg->clk[i] = clk;
> }
> }
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx
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
