[PATCH 0/5] ASoC: rsnd: tidyup adg and header
Hi Mark
I noticed that adg can be more clean code. And rsnd.h header comment was not so good because patch has been randomly added.
This patch tidyup these.
Kuninori Morimoto (5): ASoC: rsnd: adg: supply __printf(x, y) formatting for dbg_msg() ASoC: rsnd: adg: tidyup rsnd_adg_get_clkin/out() parameter ASoC: rsnd: adg: use more simple method for null_clk ASoC: rsnd: adg: check return value for rsnd_adg_get_clkin/out() ASoC: rsnd: tidyup __rsnd_mod_xxx macro comments
sound/soc/sh/rcar/adg.c | 139 +++++++++++++++++++++++++-------------- sound/soc/sh/rcar/rsnd.h | 21 ++---- 2 files changed, 97 insertions(+), 63 deletions(-)
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
Fixes the following W=1 kernel build warning(s):
sound/soc/sh/rcar/adg.c: In function 'dbg_msg': sound/soc/sh/rcar/adg.c:594:2: warning: function 'dbg_msg' might \ be a candidate for 'gnu_printf' format attribute\ [-Wsuggest-attribute=format]
Fixes: 1f9c82b5ab83 ("ASoC: rsnd: add debugfs support") Reported-by: kernel test robot lkp@intel.com Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com --- sound/soc/sh/rcar/adg.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/sound/soc/sh/rcar/adg.c b/sound/soc/sh/rcar/adg.c index 78916332c22f..390d5e22fbb8 100644 --- a/sound/soc/sh/rcar/adg.c +++ b/sound/soc/sh/rcar/adg.c @@ -584,6 +584,7 @@ static void rsnd_adg_get_clkout(struct rsnd_priv *priv, }
#if defined(DEBUG) || defined(CONFIG_DEBUG_FS) +__printf(3, 4) static void dbg_msg(struct device *dev, struct seq_file *m, const char *fmt, ...) {
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
set priv->adg before rsnd_adg_get_clkin/out() to be more simple code. Nothing is changed, but is preparation for next "ASoC: rsnd: adg: use more simple method for null_clk" patch
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com --- sound/soc/sh/rcar/adg.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/sound/soc/sh/rcar/adg.c b/sound/soc/sh/rcar/adg.c index 390d5e22fbb8..af6132479593 100644 --- a/sound/soc/sh/rcar/adg.c +++ b/sound/soc/sh/rcar/adg.c @@ -412,9 +412,9 @@ static struct clk *rsnd_adg_null_clk_get(struct rsnd_priv *priv) return clk_hw_get_clk(priv->null_hw, NULL_CLK); }
-static void rsnd_adg_get_clkin(struct rsnd_priv *priv, - struct rsnd_adg *adg) +static void rsnd_adg_get_clkin(struct rsnd_priv *priv) { + struct rsnd_adg *adg = priv->adg; struct device *dev = rsnd_priv_to_dev(priv); int i;
@@ -430,9 +430,9 @@ static void rsnd_adg_get_clkin(struct rsnd_priv *priv, } }
-static void rsnd_adg_get_clkout(struct rsnd_priv *priv, - struct rsnd_adg *adg) +static void rsnd_adg_get_clkout(struct rsnd_priv *priv) { + struct rsnd_adg *adg = priv->adg; struct clk *clk; struct device *dev = rsnd_priv_to_dev(priv); struct device_node *np = dev->of_node; @@ -644,11 +644,11 @@ int rsnd_adg_probe(struct rsnd_priv *priv) if (ret) return ret;
- rsnd_adg_get_clkin(priv, adg); - rsnd_adg_get_clkout(priv, adg); - priv->adg = adg;
+ rsnd_adg_get_clkin(priv); + rsnd_adg_get_clkout(priv); + rsnd_adg_clk_enable(priv); rsnd_adg_clk_dbg_info(priv, NULL);
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
commit 965386c97616c ("ASoC: rsnd: call unregister for null_hw when removed") tried unregister null_clk, but it has some issues.
1st issue is kernel will indicate below message when unregistering, because of its timing. unregistering should be happen after clk_disable().
clk_unregister: unregistering prepared clock: rsnd_adg_null
2nd issue is, it is using priv->null_clk, but it should be adg->null_clk.
3rd issue is it is using very complex clk registering method. more simple clk_register/unregister_fixed_rate() should be OK.
This patch fixes these.
Fixes: 965386c97616c ("ASoC: rsnd: call unregister for null_hw when removed") Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com --- sound/soc/sh/rcar/adg.c | 52 +++++++++++++++++++++------------------- sound/soc/sh/rcar/rsnd.h | 1 - 2 files changed, 27 insertions(+), 26 deletions(-)
diff --git a/sound/soc/sh/rcar/adg.c b/sound/soc/sh/rcar/adg.c index af6132479593..3dfd07c8a7e3 100644 --- a/sound/soc/sh/rcar/adg.c +++ b/sound/soc/sh/rcar/adg.c @@ -28,6 +28,7 @@ static struct rsnd_mod_ops adg_ops = { struct rsnd_adg { struct clk *clk[CLKMAX]; struct clk *clkout[CLKOUTMAX]; + struct clk *null_clk; struct clk_onecell_data onecell; struct rsnd_mod mod; int clk_rate[CLKMAX]; @@ -363,53 +364,52 @@ int rsnd_adg_ssi_clk_try_start(struct rsnd_mod *ssi_mod, unsigned int rate) void rsnd_adg_clk_control(struct rsnd_priv *priv, int enable) { struct rsnd_adg *adg = rsnd_priv_to_adg(priv); - struct device *dev = rsnd_priv_to_dev(priv); struct clk *clk; int i;
for_each_rsnd_clk(clk, adg, i) { if (enable) { - int ret = clk_prepare_enable(clk); + clk_prepare_enable(clk);
/* * We shouldn't use clk_get_rate() under * atomic context. Let's keep it when * rsnd_adg_clk_enable() was called */ - adg->clk_rate[i] = 0; - if (ret < 0) - dev_warn(dev, "can't use clk %d\n", i); - else - adg->clk_rate[i] = clk_get_rate(clk); + adg->clk_rate[i] = clk_get_rate(clk); } else { - if (adg->clk_rate[i]) - clk_disable_unprepare(clk); - adg->clk_rate[i] = 0; + clk_disable_unprepare(clk); } } }
-#define NULL_CLK "rsnd_adg_null" -static struct clk *rsnd_adg_null_clk_get(struct rsnd_priv *priv) +static struct clk *rsnd_adg_create_null_clk(struct rsnd_priv *priv, + const char * const name, + const char *parent) { struct device *dev = rsnd_priv_to_dev(priv); + struct clk *clk; + + clk = clk_register_fixed_rate(dev, name, parent, 0, 0); + if (IS_ERR(clk)) { + dev_err(dev, "create null clk error\n"); + return NULL; + }
- if (!priv->null_hw) { - struct clk_hw *_hw; - int ret; + return clk; +}
- _hw = clk_hw_register_fixed_rate_with_accuracy(dev, NULL_CLK, NULL, 0, 0, 0); - if (IS_ERR(_hw)) - return NULL; +static struct clk *rsnd_adg_null_clk_get(struct rsnd_priv *priv) +{ + struct rsnd_adg *adg = priv->adg;
- ret = of_clk_add_hw_provider(dev->of_node, of_clk_hw_simple_get, _hw); - if (ret < 0) - clk_hw_unregister_fixed_rate(_hw); + if (!adg->null_clk) { + static const char * const name = "rsnd_adg_null";
- priv->null_hw = _hw; + adg->null_clk = rsnd_adg_create_null_clk(priv, name, NULL); }
- return clk_hw_get_clk(priv->null_hw, NULL_CLK); + return adg->null_clk; }
static void rsnd_adg_get_clkin(struct rsnd_priv *priv) @@ -666,10 +666,12 @@ void rsnd_adg_remove(struct rsnd_priv *priv) for_each_rsnd_clkout(clk, adg, i) if (adg->clkout[i]) clk_unregister_fixed_rate(adg->clkout[i]); - if (priv->null_hw) - clk_hw_unregister_fixed_rate(priv->null_hw);
of_clk_del_provider(np);
rsnd_adg_clk_disable(priv); + + /* It should be called after rsnd_adg_clk_disable() */ + if (adg->null_clk) + clk_unregister_fixed_rate(adg->null_clk); } diff --git a/sound/soc/sh/rcar/rsnd.h b/sound/soc/sh/rcar/rsnd.h index b2fbe3bbaabd..0182ea5b31d2 100644 --- a/sound/soc/sh/rcar/rsnd.h +++ b/sound/soc/sh/rcar/rsnd.h @@ -646,7 +646,6 @@ struct rsnd_priv { * below value will be filled on rsnd_adg_probe() */ void *adg; - struct clk_hw *null_hw;
/* * below value will be filled on rsnd_dma_probe()
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
Current rsnd_adg_get_clkin/out() are void function, thus adg->clk/clkout[i] might be NULL.
But, for_each_rsnd_clk/clkout() macros are assuming all clks are non NULL.
Because of this mismatch, code can be complex and/or buggy. These functions return error by this patch, and make sure all clks are non NULL.
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com --- sound/soc/sh/rcar/adg.c | 84 ++++++++++++++++++++++++++++++----------- 1 file changed, 61 insertions(+), 23 deletions(-)
diff --git a/sound/soc/sh/rcar/adg.c b/sound/soc/sh/rcar/adg.c index 3dfd07c8a7e3..0ebee1ed06a9 100644 --- a/sound/soc/sh/rcar/adg.c +++ b/sound/soc/sh/rcar/adg.c @@ -412,25 +412,53 @@ static struct clk *rsnd_adg_null_clk_get(struct rsnd_priv *priv) return adg->null_clk; }
-static void rsnd_adg_get_clkin(struct rsnd_priv *priv) +static void rsnd_adg_null_clk_clean(struct rsnd_priv *priv) +{ + struct rsnd_adg *adg = priv->adg; + + if (adg->null_clk) + clk_unregister_fixed_rate(adg->null_clk); +} + +static int rsnd_adg_get_clkin(struct rsnd_priv *priv) { struct rsnd_adg *adg = priv->adg; struct device *dev = rsnd_priv_to_dev(priv); + struct clk *clk; int i;
for (i = 0; i < CLKMAX; i++) { - struct clk *clk = devm_clk_get(dev, clk_name[i]); + clk = devm_clk_get(dev, clk_name[i]);
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]); + goto err;
adg->clk[i] = clk; } + + return 0; + +err: + dev_err(dev, "adg clock IN get failed\n"); + + rsnd_adg_null_clk_clean(priv); + + return -EIO; +} + +static void rsnd_adg_unregister_clkout(struct rsnd_priv *priv) +{ + struct rsnd_adg *adg = priv->adg; + struct clk *clk; + int i; + + for_each_rsnd_clkout(clk, adg, i) + clk_unregister_fixed_rate(clk); }
-static void rsnd_adg_get_clkout(struct rsnd_priv *priv) +static int rsnd_adg_get_clkout(struct rsnd_priv *priv) { struct rsnd_adg *adg = priv->adg; struct clk *clk; @@ -472,9 +500,8 @@ static void rsnd_adg_get_clkout(struct rsnd_priv *priv)
req_size = prop->length / sizeof(u32); if (req_size > REQ_SIZE) { - dev_err(dev, - "too many clock-frequency, use top %d\n", REQ_SIZE); - req_size = REQ_SIZE; + dev_err(dev, "too many clock-frequency\n"); + return -EINVAL; }
of_property_read_u32_array(np, "clock-frequency", req_rate, req_size); @@ -555,10 +582,11 @@ static void rsnd_adg_get_clkout(struct rsnd_priv *priv) if (!count) { clk = clk_register_fixed_rate(dev, clkout_name[CLKOUT], parent_clk_name, 0, req_rate[0]); - if (!IS_ERR(clk)) { - adg->clkout[CLKOUT] = clk; - of_clk_add_provider(np, of_clk_src_simple_get, clk); - } + if (IS_ERR(clk)) + goto err; + + adg->clkout[CLKOUT] = clk; + of_clk_add_provider(np, of_clk_src_simple_get, clk); } /* * for clkout0/1/2/3 @@ -568,8 +596,10 @@ static void rsnd_adg_get_clkout(struct rsnd_priv *priv) clk = clk_register_fixed_rate(dev, clkout_name[i], parent_clk_name, 0, req_rate[0]); - if (!IS_ERR(clk)) - adg->clkout[i] = clk; + if (IS_ERR(clk)) + goto err; + + adg->clkout[i] = clk; } adg->onecell.clks = adg->clkout; adg->onecell.clk_num = CLKOUTMAX; @@ -581,6 +611,15 @@ static void rsnd_adg_get_clkout(struct rsnd_priv *priv) adg->ckr = ckr; adg->rbga = rbga; adg->rbgb = rbgb; + + return 0; + +err: + dev_err(dev, "adg clock OUT get failed\n"); + + rsnd_adg_unregister_clkout(priv); + + return -EIO; }
#if defined(DEBUG) || defined(CONFIG_DEBUG_FS) @@ -646,8 +685,13 @@ int rsnd_adg_probe(struct rsnd_priv *priv)
priv->adg = adg;
- rsnd_adg_get_clkin(priv); - rsnd_adg_get_clkout(priv); + ret = rsnd_adg_get_clkin(priv); + if (ret) + return ret; + + ret = rsnd_adg_get_clkout(priv); + if (ret) + return ret;
rsnd_adg_clk_enable(priv); rsnd_adg_clk_dbg_info(priv, NULL); @@ -659,19 +703,13 @@ void rsnd_adg_remove(struct rsnd_priv *priv) { struct device *dev = rsnd_priv_to_dev(priv); struct device_node *np = dev->of_node; - struct rsnd_adg *adg = priv->adg; - struct clk *clk; - int i;
- for_each_rsnd_clkout(clk, adg, i) - if (adg->clkout[i]) - clk_unregister_fixed_rate(adg->clkout[i]); + rsnd_adg_unregister_clkout(priv);
of_clk_del_provider(np);
rsnd_adg_clk_disable(priv);
/* It should be called after rsnd_adg_clk_disable() */ - if (adg->null_clk) - clk_unregister_fixed_rate(adg->null_clk); + rsnd_adg_null_clk_clean(priv); }
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
status and __rsnd_mod_xxx were updated, but some related comments were not. And it has verbose comments. This patch cleanup/tidyup these.
1) adds missing "D" to status sample 2) remove verbose list for "H" 3) add "needs protect" to __rsnd_mod_call_xxx
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com --- sound/soc/sh/rcar/rsnd.h | 20 +++++++------------- 1 file changed, 7 insertions(+), 13 deletions(-)
diff --git a/sound/soc/sh/rcar/rsnd.h b/sound/soc/sh/rcar/rsnd.h index 0182ea5b31d2..6580bab0e229 100644 --- a/sound/soc/sh/rcar/rsnd.h +++ b/sound/soc/sh/rcar/rsnd.h @@ -364,19 +364,13 @@ struct rsnd_mod { /* * status * - * 0xH0000CB0 + * 0xH000DCB0 * * B 0: init 1: quit * C 0: start 1: stop * D 0: hw_params 1: hw_free * * H is always called (see __rsnd_mod_call) - * H 0: probe 1: remove - * H 0: pcm_new - * H 0: fallback - * H 0: pointer - * H 0: prepare - * H 0: cleanup */ #define __rsnd_mod_shift_init 4 #define __rsnd_mod_shift_quit 4 @@ -412,16 +406,16 @@ struct rsnd_mod { #define __rsnd_mod_call_remove 0 #define __rsnd_mod_call_prepare 0 #define __rsnd_mod_call_cleanup 0 -#define __rsnd_mod_call_init 0 -#define __rsnd_mod_call_quit 1 -#define __rsnd_mod_call_start 0 -#define __rsnd_mod_call_stop 1 +#define __rsnd_mod_call_init 0 /* needs protect */ +#define __rsnd_mod_call_quit 1 /* needs protect */ +#define __rsnd_mod_call_start 0 /* needs protect */ +#define __rsnd_mod_call_stop 1 /* needs protect */ +#define __rsnd_mod_call_hw_params 0 /* needs protect */ +#define __rsnd_mod_call_hw_free 1 /* needs protect */ #define __rsnd_mod_call_irq 0 #define __rsnd_mod_call_pcm_new 0 #define __rsnd_mod_call_fallback 0 -#define __rsnd_mod_call_hw_params 0 #define __rsnd_mod_call_pointer 0 -#define __rsnd_mod_call_hw_free 1
#define rsnd_mod_to_priv(mod) ((mod)->priv) #define rsnd_mod_power_on(mod) clk_enable((mod)->clk)
On 02 Jun 2021 08:43:01 +0900, Kuninori Morimoto wrote:
I noticed that adg can be more clean code. And rsnd.h header comment was not so good because patch has been randomly added.
This patch tidyup these.
Kuninori Morimoto (5): ASoC: rsnd: adg: supply __printf(x, y) formatting for dbg_msg() ASoC: rsnd: adg: tidyup rsnd_adg_get_clkin/out() parameter ASoC: rsnd: adg: use more simple method for null_clk ASoC: rsnd: adg: check return value for rsnd_adg_get_clkin/out() ASoC: rsnd: tidyup __rsnd_mod_xxx macro comments
[...]
Applied to
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next
Thanks!
[1/5] ASoC: rsnd: adg: supply __printf(x, y) formatting for dbg_msg() commit: 2cdfe6520c939aff60bf78be2fc682e7635d0618 [2/5] ASoC: rsnd: adg: tidyup rsnd_adg_get_clkin/out() parameter commit: b48e4aa48931030382d26c624cf4ae1c68d15666 [3/5] ASoC: rsnd: adg: use more simple method for null_clk commit: cb2f97d89f383dafa822bce66f0c3514dfb135b8 [4/5] ASoC: rsnd: adg: check return value for rsnd_adg_get_clkin/out() commit: d668a5e2409b2ff9291493b70c961ecbe883bfb2 [5/5] ASoC: rsnd: tidyup __rsnd_mod_xxx macro comments commit: 3f4593fb4a9ddb53edefcbf7d4c5fd1f04717422
All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying to this mail.
Thanks, Mark
participants (2)
-
Kuninori Morimoto -
Mark Brown