[PATCH v2 0/5] ASoC: clock provider clean-up

[PATCH v2 0/5] ASoC: clock provider clean-up

[Jerome Brunet]

The purpose of this patchset it remove the use the clk member of
'struct clk_hw' in ASoC. 'struct clk' is a per-user reference to an actual
clock. In the future, the clk member in 'struct clk_hw' may go away.

The usage of this member by a clock provider usually falls into either of
following categories:
* Mis-usage of the clock consumer API by a clock provider.
* Clock provider also being a user of its own clocks. In this case the
provider should request a 'struct clk' through the appropriate API
instead of poking in 'struct clk_hw' internals.

v1 [0] -> v2:
 * finalize lpass provider move to devm

[0]: https://lore.kernel.org/r/20210410111356.467340-1-jbrunet@baylibre.com

Jerome Brunet (5):
  ASoC: stm32: properly get clk from the provider
  ASoC: wcd934x: use the clock provider API
  ASoC: rt5682: clock driver must use the clock provider API
  ASoC: lpass: use the clock provider API
  ASoC: da7219: properly get clk from the provider

 sound/soc/codecs/da7219.c          |  5 ++++-
 sound/soc/codecs/lpass-va-macro.c  |  7 ++-----
 sound/soc/codecs/lpass-wsa-macro.c | 11 +++--------
 sound/soc/codecs/rt5682.c          |  6 +++---
 sound/soc/codecs/wcd934x.c         |  6 ++++--
 sound/soc/stm/stm32_sai_sub.c      |  5 ++++-
 6 files changed, 20 insertions(+), 20 deletions(-)

-- 
2.31.1

[PATCH v2 0/5] ASoC: clock provider clean-up

[Jerome Brunet]

The purpose of this patchset it remove the use the clk member of
'struct clk_hw' in ASoC. 'struct clk' is a per-user reference to an actual
clock. In the future, the clk member in 'struct clk_hw' may go away.

The usage of this member by a clock provider usually falls into either of
following categories:
* Mis-usage of the clock consumer API by a clock provider.
* Clock provider also being a user of its own clocks. In this case the
provider should request a 'struct clk' through the appropriate API
instead of poking in 'struct clk_hw' internals.

v1 [0] -> v2:
 * finalize lpass provider move to devm

[0]: https://lore.kernel.org/r/20210410111356.467340-1-jbrunet@baylibre.com

Jerome Brunet (5):
  ASoC: stm32: properly get clk from the provider
  ASoC: wcd934x: use the clock provider API
  ASoC: rt5682: clock driver must use the clock provider API
  ASoC: lpass: use the clock provider API
  ASoC: da7219: properly get clk from the provider

 sound/soc/codecs/da7219.c          |  5 ++++-
 sound/soc/codecs/lpass-va-macro.c  |  7 ++-----
 sound/soc/codecs/lpass-wsa-macro.c | 11 +++--------
 sound/soc/codecs/rt5682.c          |  6 +++---
 sound/soc/codecs/wcd934x.c         |  6 ++++--
 sound/soc/stm/stm32_sai_sub.c      |  5 ++++-
 6 files changed, 20 insertions(+), 20 deletions(-)

-- 
2.31.1

[PATCH v2 1/5] ASoC: stm32: properly get clk from the provider

[Jerome Brunet]

Instead of using the clk embedded in the clk_hw (which is meant to go
away), a clock provider which need to interact with its own clock should
request clk reference through the clock provider API.

Reviewed-by: Stephen Boyd <sboyd@kernel.org>
Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 sound/soc/stm/stm32_sai_sub.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/sound/soc/stm/stm32_sai_sub.c b/sound/soc/stm/stm32_sai_sub.c
index 3aa1cf262402..c1561237ee24 100644
--- a/sound/soc/stm/stm32_sai_sub.c
+++ b/sound/soc/stm/stm32_sai_sub.c
@@ -484,7 +484,10 @@ static int stm32_sai_add_mclk_provider(struct stm32_sai_sub_data *sai)
 		dev_err(dev, "mclk register returned %d\n", ret);
 		return ret;
 	}
-	sai->sai_mclk = hw->clk;
+
+	sai->sai_mclk = devm_clk_hw_get_clk(dev, hw, NULL);
+	if (IS_ERR(sai->sai_mclk))
+		return PTR_ERR(sai->sai_mclk);
 
 	/* register mclk provider */
 	return devm_of_clk_add_hw_provider(dev, of_clk_hw_simple_get, hw);
-- 
2.31.1

[PATCH v2 1/5] ASoC: stm32: properly get clk from the provider

[Jerome Brunet]

Instead of using the clk embedded in the clk_hw (which is meant to go
away), a clock provider which need to interact with its own clock should
request clk reference through the clock provider API.

Reviewed-by: Stephen Boyd <sboyd@kernel.org>
Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 sound/soc/stm/stm32_sai_sub.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/sound/soc/stm/stm32_sai_sub.c b/sound/soc/stm/stm32_sai_sub.c
index 3aa1cf262402..c1561237ee24 100644
--- a/sound/soc/stm/stm32_sai_sub.c
+++ b/sound/soc/stm/stm32_sai_sub.c
@@ -484,7 +484,10 @@ static int stm32_sai_add_mclk_provider(struct stm32_sai_sub_data *sai)
 		dev_err(dev, "mclk register returned %d\n", ret);
 		return ret;
 	}
-	sai->sai_mclk = hw->clk;
+
+	sai->sai_mclk = devm_clk_hw_get_clk(dev, hw, NULL);
+	if (IS_ERR(sai->sai_mclk))
+		return PTR_ERR(sai->sai_mclk);
 
 	/* register mclk provider */
 	return devm_of_clk_add_hw_provider(dev, of_clk_hw_simple_get, hw);
-- 
2.31.1

[PATCH v2 2/5] ASoC: wcd934x: use the clock provider API

[Jerome Brunet]

Clock providers should use the clk_hw API

Reviewed-by: Stephen Boyd <sboyd@kernel.org>
Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 sound/soc/codecs/wcd934x.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/sound/soc/codecs/wcd934x.c b/sound/soc/codecs/wcd934x.c
index cddc49bbb7f6..046874ef490e 100644
--- a/sound/soc/codecs/wcd934x.c
+++ b/sound/soc/codecs/wcd934x.c
@@ -2116,11 +2116,13 @@ static struct clk *wcd934x_register_mclk_output(struct wcd934x_codec *wcd)
 	wcd->hw.init = &init;
 
 	hw = &wcd->hw;
-	ret = clk_hw_register(wcd->dev->parent, hw);
+	ret = devm_clk_hw_register(wcd->dev->parent, hw);
 	if (ret)
 		return ERR_PTR(ret);
 
-	of_clk_add_provider(np, of_clk_src_simple_get, hw->clk);
+	ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_simple_get, hw);
+	if (ret)
+		return ERR_PTR(ret);
 
 	return NULL;
 }
-- 
2.31.1

[PATCH v2 2/5] ASoC: wcd934x: use the clock provider API

[Jerome Brunet]

Clock providers should use the clk_hw API

Reviewed-by: Stephen Boyd <sboyd@kernel.org>
Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 sound/soc/codecs/wcd934x.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/sound/soc/codecs/wcd934x.c b/sound/soc/codecs/wcd934x.c
index cddc49bbb7f6..046874ef490e 100644
--- a/sound/soc/codecs/wcd934x.c
+++ b/sound/soc/codecs/wcd934x.c
@@ -2116,11 +2116,13 @@ static struct clk *wcd934x_register_mclk_output(struct wcd934x_codec *wcd)
 	wcd->hw.init = &init;
 
 	hw = &wcd->hw;
-	ret = clk_hw_register(wcd->dev->parent, hw);
+	ret = devm_clk_hw_register(wcd->dev->parent, hw);
 	if (ret)
 		return ERR_PTR(ret);
 
-	of_clk_add_provider(np, of_clk_src_simple_get, hw->clk);
+	ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_simple_get, hw);
+	if (ret)
+		return ERR_PTR(ret);
 
 	return NULL;
 }
-- 
2.31.1

[PATCH v2 3/5] ASoC: rt5682: clock driver must use the clock provider API

[Jerome Brunet]

Clock drivers ops should not call the clk API but the clock provider
(clk_hw) instead.

Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 sound/soc/codecs/rt5682.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/sound/soc/codecs/rt5682.c b/sound/soc/codecs/rt5682.c
index a5aacfe01a0d..e4c91571abae 100644
--- a/sound/soc/codecs/rt5682.c
+++ b/sound/soc/codecs/rt5682.c
@@ -2634,7 +2634,7 @@ static int rt5682_wclk_set_rate(struct clk_hw *hw, unsigned long rate,
 		container_of(hw, struct rt5682_priv,
 			     dai_clks_hw[RT5682_DAI_WCLK_IDX]);
 	struct snd_soc_component *component = rt5682->component;
-	struct clk *parent_clk;
+	struct clk_hw *parent_hw;
 	const char * const clk_name = clk_hw_get_name(hw);
 	int pre_div;
 	unsigned int clk_pll2_out;
@@ -2649,8 +2649,8 @@ static int rt5682_wclk_set_rate(struct clk_hw *hw, unsigned long rate,
 	 *
 	 * It will set the codec anyway by assuming mclk is 48MHz.
 	 */
-	parent_clk = clk_get_parent(hw->clk);
-	if (!parent_clk)
+	parent_hw = clk_hw_get_parent(hw);
+	if (!parent_hw)
 		dev_warn(component->dev,
 			"Parent mclk of wclk not acquired in driver. Please ensure mclk was provided as %d Hz.\n",
 			CLK_PLL2_FIN);
-- 
2.31.1

[PATCH v2 3/5] ASoC: rt5682: clock driver must use the clock provider API

[Jerome Brunet]

Clock drivers ops should not call the clk API but the clock provider
(clk_hw) instead.

Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 sound/soc/codecs/rt5682.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/sound/soc/codecs/rt5682.c b/sound/soc/codecs/rt5682.c
index a5aacfe01a0d..e4c91571abae 100644
--- a/sound/soc/codecs/rt5682.c
+++ b/sound/soc/codecs/rt5682.c
@@ -2634,7 +2634,7 @@ static int rt5682_wclk_set_rate(struct clk_hw *hw, unsigned long rate,
 		container_of(hw, struct rt5682_priv,
 			     dai_clks_hw[RT5682_DAI_WCLK_IDX]);
 	struct snd_soc_component *component = rt5682->component;
-	struct clk *parent_clk;
+	struct clk_hw *parent_hw;
 	const char * const clk_name = clk_hw_get_name(hw);
 	int pre_div;
 	unsigned int clk_pll2_out;
@@ -2649,8 +2649,8 @@ static int rt5682_wclk_set_rate(struct clk_hw *hw, unsigned long rate,
 	 *
 	 * It will set the codec anyway by assuming mclk is 48MHz.
 	 */
-	parent_clk = clk_get_parent(hw->clk);
-	if (!parent_clk)
+	parent_hw = clk_hw_get_parent(hw);
+	if (!parent_hw)
 		dev_warn(component->dev,
 			"Parent mclk of wclk not acquired in driver. Please ensure mclk was provided as %d Hz.\n",
 			CLK_PLL2_FIN);
-- 
2.31.1

[PATCH v2 4/5] ASoC: lpass: use the clock provider API

[Jerome Brunet]

Clock providers should be registered using the clk_hw API.

Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 sound/soc/codecs/lpass-va-macro.c  |  7 ++-----
 sound/soc/codecs/lpass-wsa-macro.c | 11 +++--------
 2 files changed, 5 insertions(+), 13 deletions(-)

diff --git a/sound/soc/codecs/lpass-va-macro.c b/sound/soc/codecs/lpass-va-macro.c
index 5294c57b2cd4..56c93f4465c9 100644
--- a/sound/soc/codecs/lpass-va-macro.c
+++ b/sound/soc/codecs/lpass-va-macro.c
@@ -1343,7 +1343,7 @@ static int va_macro_register_fsgen_output(struct va_macro *va)
 	if (ret)
 		return ret;
 
-	return of_clk_add_provider(np, of_clk_src_simple_get, va->hw.clk);
+	return devm_of_clk_add_hw_provider(dev, of_clk_hw_simple_get, &va->hw);
 }
 
 static int va_macro_validate_dmic_sample_rate(u32 dmic_sample_rate,
@@ -1452,12 +1452,10 @@ static int va_macro_probe(struct platform_device *pdev)
 					      va_macro_dais,
 					      ARRAY_SIZE(va_macro_dais));
 	if (ret)
-		goto soc_err;
+		goto err;
 
 	return ret;
 
-soc_err:
-	of_clk_del_provider(pdev->dev.of_node);
 err:
 	clk_bulk_disable_unprepare(VA_NUM_CLKS_MAX, va->clks);
 
@@ -1468,7 +1466,6 @@ static int va_macro_remove(struct platform_device *pdev)
 {
 	struct va_macro *va = dev_get_drvdata(&pdev->dev);
 
-	of_clk_del_provider(pdev->dev.of_node);
 	clk_bulk_disable_unprepare(VA_NUM_CLKS_MAX, va->clks);
 
 	return 0;
diff --git a/sound/soc/codecs/lpass-wsa-macro.c b/sound/soc/codecs/lpass-wsa-macro.c
index e79a70386b4b..1a7fa5492f28 100644
--- a/sound/soc/codecs/lpass-wsa-macro.c
+++ b/sound/soc/codecs/lpass-wsa-macro.c
@@ -2337,10 +2337,9 @@ static const struct clk_ops swclk_gate_ops = {
 	.recalc_rate = swclk_recalc_rate,
 };
 
-static struct clk *wsa_macro_register_mclk_output(struct wsa_macro *wsa)
+static int wsa_macro_register_mclk_output(struct wsa_macro *wsa)
 {
 	struct device *dev = wsa->dev;
-	struct device_node *np = dev->of_node;
 	const char *parent_clk_name;
 	const char *clk_name = "mclk";
 	struct clk_hw *hw;
@@ -2358,11 +2357,9 @@ static struct clk *wsa_macro_register_mclk_output(struct wsa_macro *wsa)
 	hw = &wsa->hw;
 	ret = clk_hw_register(wsa->dev, hw);
 	if (ret)
-		return ERR_PTR(ret);
-
-	of_clk_add_provider(np, of_clk_src_simple_get, hw->clk);
+		return ret;
 
-	return NULL;
+	return devm_of_clk_add_hw_provider(dev, of_clk_hw_simple_get, hw);
 }
 
 static const struct snd_soc_component_driver wsa_macro_component_drv = {
@@ -2438,8 +2435,6 @@ static int wsa_macro_remove(struct platform_device *pdev)
 {
 	struct wsa_macro *wsa = dev_get_drvdata(&pdev->dev);
 
-	of_clk_del_provider(pdev->dev.of_node);
-
 	clk_bulk_disable_unprepare(WSA_NUM_CLKS_MAX, wsa->clks);
 
 	return 0;
-- 
2.31.1

[PATCH v2 4/5] ASoC: lpass: use the clock provider API

[Jerome Brunet]

Clock providers should be registered using the clk_hw API.

Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 sound/soc/codecs/lpass-va-macro.c  |  7 ++-----
 sound/soc/codecs/lpass-wsa-macro.c | 11 +++--------
 2 files changed, 5 insertions(+), 13 deletions(-)

diff --git a/sound/soc/codecs/lpass-va-macro.c b/sound/soc/codecs/lpass-va-macro.c
index 5294c57b2cd4..56c93f4465c9 100644
--- a/sound/soc/codecs/lpass-va-macro.c
+++ b/sound/soc/codecs/lpass-va-macro.c
@@ -1343,7 +1343,7 @@ static int va_macro_register_fsgen_output(struct va_macro *va)
 	if (ret)
 		return ret;
 
-	return of_clk_add_provider(np, of_clk_src_simple_get, va->hw.clk);
+	return devm_of_clk_add_hw_provider(dev, of_clk_hw_simple_get, &va->hw);
 }
 
 static int va_macro_validate_dmic_sample_rate(u32 dmic_sample_rate,
@@ -1452,12 +1452,10 @@ static int va_macro_probe(struct platform_device *pdev)
 					      va_macro_dais,
 					      ARRAY_SIZE(va_macro_dais));
 	if (ret)
-		goto soc_err;
+		goto err;
 
 	return ret;
 
-soc_err:
-	of_clk_del_provider(pdev->dev.of_node);
 err:
 	clk_bulk_disable_unprepare(VA_NUM_CLKS_MAX, va->clks);
 
@@ -1468,7 +1466,6 @@ static int va_macro_remove(struct platform_device *pdev)
 {
 	struct va_macro *va = dev_get_drvdata(&pdev->dev);
 
-	of_clk_del_provider(pdev->dev.of_node);
 	clk_bulk_disable_unprepare(VA_NUM_CLKS_MAX, va->clks);
 
 	return 0;
diff --git a/sound/soc/codecs/lpass-wsa-macro.c b/sound/soc/codecs/lpass-wsa-macro.c
index e79a70386b4b..1a7fa5492f28 100644
--- a/sound/soc/codecs/lpass-wsa-macro.c
+++ b/sound/soc/codecs/lpass-wsa-macro.c
@@ -2337,10 +2337,9 @@ static const struct clk_ops swclk_gate_ops = {
 	.recalc_rate = swclk_recalc_rate,
 };
 
-static struct clk *wsa_macro_register_mclk_output(struct wsa_macro *wsa)
+static int wsa_macro_register_mclk_output(struct wsa_macro *wsa)
 {
 	struct device *dev = wsa->dev;
-	struct device_node *np = dev->of_node;
 	const char *parent_clk_name;
 	const char *clk_name = "mclk";
 	struct clk_hw *hw;
@@ -2358,11 +2357,9 @@ static struct clk *wsa_macro_register_mclk_output(struct wsa_macro *wsa)
 	hw = &wsa->hw;
 	ret = clk_hw_register(wsa->dev, hw);
 	if (ret)
-		return ERR_PTR(ret);
-
-	of_clk_add_provider(np, of_clk_src_simple_get, hw->clk);
+		return ret;
 
-	return NULL;
+	return devm_of_clk_add_hw_provider(dev, of_clk_hw_simple_get, hw);
 }
 
 static const struct snd_soc_component_driver wsa_macro_component_drv = {
@@ -2438,8 +2435,6 @@ static int wsa_macro_remove(struct platform_device *pdev)
 {
 	struct wsa_macro *wsa = dev_get_drvdata(&pdev->dev);
 
-	of_clk_del_provider(pdev->dev.of_node);
-
 	clk_bulk_disable_unprepare(WSA_NUM_CLKS_MAX, wsa->clks);
 
 	return 0;
-- 
2.31.1

[PATCH v2 5/5] ASoC: da7219: properly get clk from the provider

[Jerome Brunet]

Instead of using the clk embedded in the clk_hw (which is meant to go
away), a clock provider which need to interact with its own clock should
request clk reference through the clock provider API.

Reviewed-by: Stephen Boyd <sboyd@kernel.org>
Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 sound/soc/codecs/da7219.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/sound/soc/codecs/da7219.c b/sound/soc/codecs/da7219.c
index 13009d08b09a..bd3c523a8617 100644
--- a/sound/soc/codecs/da7219.c
+++ b/sound/soc/codecs/da7219.c
@@ -2181,7 +2181,10 @@ static int da7219_register_dai_clks(struct snd_soc_component *component)
 				 ret);
 			goto err;
 		}
-		da7219->dai_clks[i] = dai_clk_hw->clk;
+
+		da7219->dai_clks[i] = devm_clk_hw_get_clk(dev, dai_clk_hw, NULL);
+		if (IS_ERR(da7219->dai_clks[i]))
+			return PTR_ERR(da7219->dai_clks[i]);
 
 		/* For DT setup onecell data, otherwise create lookup */
 		if (np) {
-- 
2.31.1

[PATCH v2 5/5] ASoC: da7219: properly get clk from the provider

[Jerome Brunet]

Instead of using the clk embedded in the clk_hw (which is meant to go
away), a clock provider which need to interact with its own clock should
request clk reference through the clock provider API.

Reviewed-by: Stephen Boyd <sboyd@kernel.org>
Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 sound/soc/codecs/da7219.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/sound/soc/codecs/da7219.c b/sound/soc/codecs/da7219.c
index 13009d08b09a..bd3c523a8617 100644
--- a/sound/soc/codecs/da7219.c
+++ b/sound/soc/codecs/da7219.c
@@ -2181,7 +2181,10 @@ static int da7219_register_dai_clks(struct snd_soc_component *component)
 				 ret);
 			goto err;
 		}
-		da7219->dai_clks[i] = dai_clk_hw->clk;
+
+		da7219->dai_clks[i] = devm_clk_hw_get_clk(dev, dai_clk_hw, NULL);
+		if (IS_ERR(da7219->dai_clks[i]))
+			return PTR_ERR(da7219->dai_clks[i]);
 
 		/* For DT setup onecell data, otherwise create lookup */
 		if (np) {
-- 
2.31.1

Re: [PATCH v2 5/5] ASoC: da7219: properly get clk from the provider

[Pierre-Louis Bossart]

On 4/21/21 7:05 AM, Jerome Brunet wrote:
> Instead of using the clk embedded in the clk_hw (which is meant to go
> away), a clock provider which need to interact with its own clock should
> request clk reference through the clock provider API.
> 
> Reviewed-by: Stephen Boyd <sboyd@kernel.org>
> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>

This patch seems to introduce a regression in our modprobe/rmmod tests

https://github.com/thesofproject/linux/pull/2870

RMMOD	snd_soc_da7219
rmmod: ERROR: Module snd_soc_da7219 is in use

Reverting this patch restores the ability to remove the module.

Wondering if devm_ increases a module/device refcount somehow?

> ---
>   sound/soc/codecs/da7219.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/sound/soc/codecs/da7219.c b/sound/soc/codecs/da7219.c
> index 13009d08b09a..bd3c523a8617 100644
> --- a/sound/soc/codecs/da7219.c
> +++ b/sound/soc/codecs/da7219.c
> @@ -2181,7 +2181,10 @@ static int da7219_register_dai_clks(struct snd_soc_component *component)
>   				 ret);
>   			goto err;
>   		}
> -		da7219->dai_clks[i] = dai_clk_hw->clk;
> +
> +		da7219->dai_clks[i] = devm_clk_hw_get_clk(dev, dai_clk_hw, NULL);
> +		if (IS_ERR(da7219->dai_clks[i]))
> +			return PTR_ERR(da7219->dai_clks[i]);
>   
>   		/* For DT setup onecell data, otherwise create lookup */
>   		if (np) {
> 

Re: [PATCH v2 5/5] ASoC: da7219: properly get clk from the provider

[Pierre-Louis Bossart]

> On 4/21/21 7:05 AM, Jerome Brunet wrote:
>> Instead of using the clk embedded in the clk_hw (which is meant to go
>> away), a clock provider which need to interact with its own clock should
>> request clk reference through the clock provider API.
>>
>> Reviewed-by: Stephen Boyd <sboyd@kernel.org>
>> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
> 
> This patch seems to introduce a regression in our modprobe/rmmod tests
> 
> https://github.com/thesofproject/linux/pull/2870
> 
> RMMOD    snd_soc_da7219
> rmmod: ERROR: Module snd_soc_da7219 is in use
> 
> Reverting this patch restores the ability to remove the module.
> 
> Wondering if devm_ increases a module/device refcount somehow?

the following diff fixes the issue for me

There is an explicit try_module_get() in clk_hw_create_clk, so you 
end-up increasing the refcount of your own module.

devm_ doesn't seem like a good idea here, I think we have to release the 
clk and its implicit module reference when the component is freed, no?

I can send a proper fix if there is consensus.


diff --git a/sound/soc/codecs/da7219.c b/sound/soc/codecs/da7219.c
index bd3c523a8617..8696ac749af3 100644
--- a/sound/soc/codecs/da7219.c
+++ b/sound/soc/codecs/da7219.c
@@ -2182,7 +2182,7 @@ static int da7219_register_dai_clks(struct 
snd_soc_component *component)
                         goto err;
                 }

-               da7219->dai_clks[i] = devm_clk_hw_get_clk(dev, 
dai_clk_hw, NULL);
+               da7219->dai_clks[i] = clk_hw_get_clk(dai_clk_hw, NULL);
                 if (IS_ERR(da7219->dai_clks[i]))
                         return PTR_ERR(da7219->dai_clks[i]);

@@ -2218,6 +2218,8 @@ static int da7219_register_dai_clks(struct 
snd_soc_component *component)
                 if (da7219->dai_clks_lookup[i])
                         clkdev_drop(da7219->dai_clks_lookup[i]);

+               clk_put(da7219->dai_clks[i]);
+
                 clk_hw_unregister(&da7219->dai_clks_hw[i]);
         } while (i-- > 0);

@@ -2240,6 +2242,8 @@ static void da7219_free_dai_clks(struct 
snd_soc_component *component)
                 if (da7219->dai_clks_lookup[i])
                         clkdev_drop(da7219->dai_clks_lookup[i]);

+               clk_put(da7219->dai_clks[i]);
+
                 clk_hw_unregister(&da7219->dai_clks_hw[i]);
         }

Re: [PATCH v2 5/5] ASoC: da7219: properly get clk from the provider

[Jerome Brunet]

On Mon 26 Apr 2021 at 20:10, Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> wrote:

> On 4/21/21 7:05 AM, Jerome Brunet wrote:
>> Instead of using the clk embedded in the clk_hw (which is meant to go
>> away), a clock provider which need to interact with its own clock should
>> request clk reference through the clock provider API.
>> Reviewed-by: Stephen Boyd <sboyd@kernel.org>
>> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
>
> This patch seems to introduce a regression in our modprobe/rmmod tests

Really sorry about that :/

>
> https://github.com/thesofproject/linux/pull/2870
>
> RMMOD	snd_soc_da7219
> rmmod: ERROR: Module snd_soc_da7219 is in use
>
> Reverting this patch restores the ability to remove the module.
>
> Wondering if devm_ increases a module/device refcount somehow?

The driver is the provider and consumer, so it is consuming itself.
This was the intent, I think the patch should be correct like
this. Maybe I overlooked something on the clock side. I'll check.

I'm not sure the problem is devm_ variant, it might be 
clk_hw_get_clk() simpler variant which also plays with module ref counts.

I don't have this particular HW to check but I'll try to replicate the
test with a dummy module and report ASAP.

Of course, I suppose the same problem applies to PATCH 1 of the series

>
>> ---
>>   sound/soc/codecs/da7219.c | 5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>> diff --git a/sound/soc/codecs/da7219.c b/sound/soc/codecs/da7219.c
>> index 13009d08b09a..bd3c523a8617 100644
>> --- a/sound/soc/codecs/da7219.c
>> +++ b/sound/soc/codecs/da7219.c
>> @@ -2181,7 +2181,10 @@ static int da7219_register_dai_clks(struct snd_soc_component *component)
>>   				 ret);
>>   			goto err;
>>   		}
>> -		da7219->dai_clks[i] = dai_clk_hw->clk;
>> +
>> +		da7219->dai_clks[i] = devm_clk_hw_get_clk(dev, dai_clk_hw, NULL);
>> +		if (IS_ERR(da7219->dai_clks[i]))
>> +			return PTR_ERR(da7219->dai_clks[i]);
>>     		/* For DT setup onecell data, otherwise create lookup */
>>   		if (np) {
>> 

Re: [PATCH v2 5/5] ASoC: da7219: properly get clk from the provider

[Jerome Brunet]

On Mon 26 Apr 2021 at 21:35, Jerome Brunet <jbrunet@baylibre.com> wrote:

> On Mon 26 Apr 2021 at 20:10, Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> wrote:
>
>> On 4/21/21 7:05 AM, Jerome Brunet wrote:
>>> Instead of using the clk embedded in the clk_hw (which is meant to go
>>> away), a clock provider which need to interact with its own clock should
>>> request clk reference through the clock provider API.
>>> Reviewed-by: Stephen Boyd <sboyd@kernel.org>
>>> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
>>
>> This patch seems to introduce a regression in our modprobe/rmmod tests
>
> Really sorry about that :/
>
>>
>> https://github.com/thesofproject/linux/pull/2870
>>
>> RMMOD	snd_soc_da7219
>> rmmod: ERROR: Module snd_soc_da7219 is in use
>>
>> Reverting this patch restores the ability to remove the module.
>>
>> Wondering if devm_ increases a module/device refcount somehow?
>
> The driver is the provider and consumer, so it is consuming itself.
> This was the intent, I think the patch should be correct like
> this. Maybe I overlooked something on the clock side. I'll check.
>
> I'm not sure the problem is devm_ variant, it might be 
> clk_hw_get_clk() simpler variant which also plays with module ref counts.
>
> I don't have this particular HW to check but I'll try to replicate the
> test with a dummy module and report ASAP.
>
> Of course, I suppose the same problem applies to PATCH 1 of the series

So I can confirm that the problem is in CCF and the function
clk_hw_get_clk() which pins the clocks provider to itself.

Not that many clock providers are modules and even less are getting
removed. This is probably why this fundamental issue has not been
detected before. Thanks a lot for reporting it.

Mark, at this point I think it would be best to revert patches 1 and 5
while we work this out in CCF. The other patches are not affected.
Sorry for the mess.

Re: [PATCH v2 5/5] ASoC: da7219: properly get clk from the provider

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 306 bytes --]

On Tue, Apr 27, 2021 at 11:16:25AM +0200, Jerome Brunet wrote:

> Mark, at this point I think it would be best to revert patches 1 and 5
> while we work this out in CCF. The other patches are not affected.
> Sorry for the mess.

Sure, can someone send a patch with a changelog explaining the issue
please?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 5/5] ASoC: da7219: properly get clk from the provider

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 306 bytes --]

On Tue, Apr 27, 2021 at 11:16:25AM +0200, Jerome Brunet wrote:

> Mark, at this point I think it would be best to revert patches 1 and 5
> while we work this out in CCF. The other patches are not affected.
> Sorry for the mess.

Sure, can someone send a patch with a changelog explaining the issue
please?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 5/5] ASoC: da7219: properly get clk from the provider

[Jerome Brunet]

On Tue 27 Apr 2021 at 12:27, Mark Brown <broonie@kernel.org> wrote:

> On Tue, Apr 27, 2021 at 11:16:25AM +0200, Jerome Brunet wrote:
>
>> Mark, at this point I think it would be best to revert patches 1 and 5
>> while we work this out in CCF. The other patches are not affected.
>> Sorry for the mess.
>
> Sure, can someone send a patch with a changelog explaining the issue
> please?

Will do

Re: [PATCH v2 5/5] ASoC: da7219: properly get clk from the provider

[Jerome Brunet]

On Tue 27 Apr 2021 at 12:27, Mark Brown <broonie@kernel.org> wrote:

> On Tue, Apr 27, 2021 at 11:16:25AM +0200, Jerome Brunet wrote:
>
>> Mark, at this point I think it would be best to revert patches 1 and 5
>> while we work this out in CCF. The other patches are not affected.
>> Sorry for the mess.
>
> Sure, can someone send a patch with a changelog explaining the issue
> please?

Will do

Re: [PATCH v2 0/5] ASoC: clock provider clean-up

[Mark Brown]

On Wed, 21 Apr 2021 14:05:07 +0200, Jerome Brunet wrote:
> The purpose of this patchset it remove the use the clk member of
> 'struct clk_hw' in ASoC. 'struct clk' is a per-user reference to an actual
> clock. In the future, the clk member in 'struct clk_hw' may go away.
> 
> The usage of this member by a clock provider usually falls into either of
> following categories:
> * Mis-usage of the clock consumer API by a clock provider.
> * Clock provider also being a user of its own clocks. In this case the
> provider should request a 'struct clk' through the appropriate API
> instead of poking in 'struct clk_hw' internals.
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next

Thanks!

[1/5] ASoC: stm32: properly get clk from the provider
      commit: 65d1cce726d4912793d0a84c55ecdb0ef5832130
[2/5] ASoC: wcd934x: use the clock provider API
      commit: 104c3a9ed07411288efcd34f08a577df318aafc0
[3/5] ASoC: rt5682: clock driver must use the clock provider API
      commit: 8691743c511d6f92d7647d78ea1e5f5ef69937b1
[4/5] ASoC: lpass: use the clock provider API
      commit: 27dc72b44e85997dfd5f3b120e5ec847c43c272a
[5/5] ASoC: da7219: properly get clk from the provider
      commit: 12f8127fe9e6154dd4197df97e44f3fd67583071

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

Re: [PATCH v2 0/5] ASoC: clock provider clean-up

[Mark Brown]

On Wed, 21 Apr 2021 14:05:07 +0200, Jerome Brunet wrote:
> The purpose of this patchset it remove the use the clk member of
> 'struct clk_hw' in ASoC. 'struct clk' is a per-user reference to an actual
> clock. In the future, the clk member in 'struct clk_hw' may go away.
> 
> The usage of this member by a clock provider usually falls into either of
> following categories:
> * Mis-usage of the clock consumer API by a clock provider.
> * Clock provider also being a user of its own clocks. In this case the
> provider should request a 'struct clk' through the appropriate API
> instead of poking in 'struct clk_hw' internals.
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next

Thanks!

[1/5] ASoC: stm32: properly get clk from the provider
      commit: 65d1cce726d4912793d0a84c55ecdb0ef5832130
[2/5] ASoC: wcd934x: use the clock provider API
      commit: 104c3a9ed07411288efcd34f08a577df318aafc0
[3/5] ASoC: rt5682: clock driver must use the clock provider API
      commit: 8691743c511d6f92d7647d78ea1e5f5ef69937b1
[4/5] ASoC: lpass: use the clock provider API
      commit: 27dc72b44e85997dfd5f3b120e5ec847c43c272a
[5/5] ASoC: da7219: properly get clk from the provider
      commit: 12f8127fe9e6154dd4197df97e44f3fd67583071

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