| Message ID | 87tumsoe2p.wl-kuninori.morimoto.gx@renesas.com |
|---|---|
| State | Accepted |
| Commit | d6956a7dde6fbf843da117f8b69cc512101fdea2 |
| Headers | show |
| Series | ASoC: rsnd: add D3 support | expand |
Hi Geert > 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 ?? Thank you for your help !! Best regards --- Kuninori Morimoto
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
On Thu, May 27, 2021 at 09:30:38AM +0200, Geert Uytterhoeven wrote: > 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? Not particularly TBH. The regulator API doesn't accept NULL pointers due to constant issues with people just ignoring errors especially around trying to decide that devices don't need power, it'd just make all that worse.
diff --git a/sound/soc/sh/rcar/adg.c b/sound/soc/sh/rcar/adg.c index a0b5bd5a7464..134549b16e0a 100644 --- a/sound/soc/sh/rcar/adg.c +++ b/sound/soc/sh/rcar/adg.c @@ -3,8 +3,8 @@ // Helper routines for R-Car sound ADG. // // Copyright (C) 2013 Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> - #include <linux/clk-provider.h> +#include <linux/clkdev.h> #include "rsnd.h" #define CLKA 0 @@ -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); + 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); + if (IS_ERR(clk)) + dev_err(dev, "no adg clock (%s)\n", clk_name[i]); + + adg->clk[i] = clk; } }