[PATCH 0/5] ASoC: rsnd: tidyup adg and header

[PATCH 0/5] ASoC: rsnd: tidyup adg and header

[Kuninori Morimoto]

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(-)

-- 
2.25.1

[PATCH 1/5] ASoC: rsnd: adg: supply __printf(x, y) formatting for dbg_msg()

[Kuninori Morimoto]

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, ...)
 {
-- 
2.25.1

[PATCH 2/5] ASoC: rsnd: adg: tidyup rsnd_adg_get_clkin/out() parameter

[Kuninori Morimoto]

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);
 
-- 
2.25.1

[PATCH 3/5] ASoC: rsnd: adg: use more simple method for null_clk

[Kuninori Morimoto]

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()
-- 
2.25.1

[PATCH 4/5] ASoC: rsnd: adg: check return value for rsnd_adg_get_clkin/out()

[Kuninori Morimoto]

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);
 }
-- 
2.25.1

[PATCH 5/5] ASoC: rsnd: tidyup __rsnd_mod_xxx macro comments

[Kuninori Morimoto]

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)
-- 
2.25.1

Re: [PATCH 0/5] ASoC: rsnd: tidyup adg and header

[Mark Brown]

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