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

Hi Morimoto-san,

On Mon, May 24, 2021 at 8:12 AM Kuninori Morimoto kuninori.morimoto.gx@renesas.com wrote:

From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com

Some Renesas SoC doesn't have full CLOCKIN. This patch add null_clk, and accepts it.

Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com

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