[PATCH v1 0/3] ASoC: atmel: Fixes for AT91SAM9G20-EK audio driver

[PATCH v1 0/3] ASoC: atmel: Fixes for AT91SAM9G20-EK audio driver

[Mark Brown]

At some point the machine driver for the audio subsystem on the
AT91SAM9G20-EK appears to have bitrotted, no longer probing due to
effects of the transition to the common clock framework. The first patch
in this series fixes the initial probe problem, with the rest of the
series being random other fixes and cleanups I noticed while looking at
the driver.

Mark Brown (3):
  ASoC: atmel: Remove system clock tree configuration for at91sam9g20ek
  ASoC: atmel: Fix error handling in at91samg20ek probe()
  ASoC: atmel: Don't squash error codes from atmel_ssc_set_audio()

 sound/soc/atmel/sam9g20_wm8731.c | 74 ++++----------------------------
 1 file changed, 8 insertions(+), 66 deletions(-)


base-commit: f590797fa3c1bccdd19e55441592a23b46aef449
-- 
2.30.2

[PATCH v1 1/3] ASoC: atmel: Remove system clock tree configuration for at91sam9g20ek

[Mark Brown]

The MCLK of the WM8731 on the AT91SAM9G20-EK board is connected to the
PCK0 output of the SoC, intended in the reference software to be supplied
using PLLB and programmed to 12MHz. As originally written for use with a
board file the audio driver was responsible for configuring the entire tree
but in the conversion to the common clock framework the registration of
the named pck0 and pllb clocks was removed so the driver has failed to
instantiate ever since.

Since the WM8731 driver has had support for managing a MCLK provided via
the common clock framework for some time we can simply drop all the clock
management code from the machine driver other than configuration of the
sysclk rate, the CODEC driver still respects that configuration from the
machine driver.

Fixes: ff78a189b0ae55f ("ARM: at91: remove old at91-specific clock driver")
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 sound/soc/atmel/sam9g20_wm8731.c | 61 --------------------------------
 1 file changed, 61 deletions(-)

diff --git a/sound/soc/atmel/sam9g20_wm8731.c b/sound/soc/atmel/sam9g20_wm8731.c
index 33e43013ff77..0d639a33ad96 100644
--- a/sound/soc/atmel/sam9g20_wm8731.c
+++ b/sound/soc/atmel/sam9g20_wm8731.c
@@ -46,35 +46,6 @@
  */
 #undef ENABLE_MIC_INPUT
 
-static struct clk *mclk;
-
-static int at91sam9g20ek_set_bias_level(struct snd_soc_card *card,
-					struct snd_soc_dapm_context *dapm,
-					enum snd_soc_bias_level level)
-{
-	static int mclk_on;
-	int ret = 0;
-
-	switch (level) {
-	case SND_SOC_BIAS_ON:
-	case SND_SOC_BIAS_PREPARE:
-		if (!mclk_on)
-			ret = clk_enable(mclk);
-		if (ret == 0)
-			mclk_on = 1;
-		break;
-
-	case SND_SOC_BIAS_OFF:
-	case SND_SOC_BIAS_STANDBY:
-		if (mclk_on)
-			clk_disable(mclk);
-		mclk_on = 0;
-		break;
-	}
-
-	return ret;
-}
-
 static const struct snd_soc_dapm_widget at91sam9g20ek_dapm_widgets[] = {
 	SND_SOC_DAPM_MIC("Int Mic", NULL),
 	SND_SOC_DAPM_SPK("Ext Spk", NULL),
@@ -135,7 +106,6 @@ static struct snd_soc_card snd_soc_at91sam9g20ek = {
 	.owner = THIS_MODULE,
 	.dai_link = &at91sam9g20ek_dai,
 	.num_links = 1,
-	.set_bias_level = at91sam9g20ek_set_bias_level,
 
 	.dapm_widgets = at91sam9g20ek_dapm_widgets,
 	.num_dapm_widgets = ARRAY_SIZE(at91sam9g20ek_dapm_widgets),
@@ -148,7 +118,6 @@ static int at91sam9g20ek_audio_probe(struct platform_device *pdev)
 {
 	struct device_node *np = pdev->dev.of_node;
 	struct device_node *codec_np, *cpu_np;
-	struct clk *pllb;
 	struct snd_soc_card *card = &snd_soc_at91sam9g20ek;
 	int ret;
 
@@ -162,31 +131,6 @@ static int at91sam9g20ek_audio_probe(struct platform_device *pdev)
 		return -EINVAL;
 	}
 
-	/*
-	 * Codec MCLK is supplied by PCK0 - set it up.
-	 */
-	mclk = clk_get(NULL, "pck0");
-	if (IS_ERR(mclk)) {
-		dev_err(&pdev->dev, "Failed to get MCLK\n");
-		ret = PTR_ERR(mclk);
-		goto err;
-	}
-
-	pllb = clk_get(NULL, "pllb");
-	if (IS_ERR(pllb)) {
-		dev_err(&pdev->dev, "Failed to get PLLB\n");
-		ret = PTR_ERR(pllb);
-		goto err_mclk;
-	}
-	ret = clk_set_parent(mclk, pllb);
-	clk_put(pllb);
-	if (ret != 0) {
-		dev_err(&pdev->dev, "Failed to set MCLK parent\n");
-		goto err_mclk;
-	}
-
-	clk_set_rate(mclk, MCLK_RATE);
-
 	card->dev = &pdev->dev;
 
 	/* Parse device node info */
@@ -230,9 +174,6 @@ static int at91sam9g20ek_audio_probe(struct platform_device *pdev)
 
 	return ret;
 
-err_mclk:
-	clk_put(mclk);
-	mclk = NULL;
 err:
 	atmel_ssc_put_audio(0);
 	return ret;
@@ -242,8 +183,6 @@ static int at91sam9g20ek_audio_remove(struct platform_device *pdev)
 {
 	struct snd_soc_card *card = platform_get_drvdata(pdev);
 
-	clk_disable(mclk);
-	mclk = NULL;
 	snd_soc_unregister_card(card);
 	atmel_ssc_put_audio(0);
 
-- 
2.30.2

[PATCH v1 2/3] ASoC: atmel: Fix error handling in at91samg20ek probe()

[Mark Brown]

The error handling in the AT91SAM9G20-EK machine driver probe did not
consistently free the SSC in error paths, sometimes immediately returning
an error rather than doing cleanup. Fix this.

Signed-off-by: Mark Brown <broonie@kernel.org>
---
 sound/soc/atmel/sam9g20_wm8731.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/sound/soc/atmel/sam9g20_wm8731.c b/sound/soc/atmel/sam9g20_wm8731.c
index 0d639a33ad96..d771843011ea 100644
--- a/sound/soc/atmel/sam9g20_wm8731.c
+++ b/sound/soc/atmel/sam9g20_wm8731.c
@@ -148,7 +148,8 @@ static int at91sam9g20ek_audio_probe(struct platform_device *pdev)
 	codec_np = of_parse_phandle(np, "atmel,audio-codec", 0);
 	if (!codec_np) {
 		dev_err(&pdev->dev, "codec info missing\n");
-		return -EINVAL;
+		ret = -EINVAL;
+		goto err;
 	}
 	at91sam9g20ek_dai.codecs->of_node = codec_np;
 
@@ -159,7 +160,8 @@ static int at91sam9g20ek_audio_probe(struct platform_device *pdev)
 	if (!cpu_np) {
 		dev_err(&pdev->dev, "dai and pcm info missing\n");
 		of_node_put(codec_np);
-		return -EINVAL;
+		ret = -EINVAL;
+		goto err;
 	}
 	at91sam9g20ek_dai.cpus->of_node = cpu_np;
 	at91sam9g20ek_dai.platforms->of_node = cpu_np;
@@ -170,9 +172,10 @@ static int at91sam9g20ek_audio_probe(struct platform_device *pdev)
 	ret = snd_soc_register_card(card);
 	if (ret) {
 		dev_err(&pdev->dev, "snd_soc_register_card() failed\n");
+		goto err;
 	}
 
-	return ret;
+	return 0;
 
 err:
 	atmel_ssc_put_audio(0);
-- 
2.30.2

[PATCH v1 3/3] ASoC: atmel: Don't squash error codes from atmel_ssc_set_audio()

[Mark Brown]

The AT91SAM9G20-EK audio driver is replacing any error code returned by
atmel_ssc_set_audio() with -EINVAL which could be unhelpful for debugging.
Pass through the error code, and include it in the log message we print for
good measure.

Signed-off-by: Mark Brown <broonie@kernel.org>
---
 sound/soc/atmel/sam9g20_wm8731.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/sound/soc/atmel/sam9g20_wm8731.c b/sound/soc/atmel/sam9g20_wm8731.c
index d771843011ea..0365b583ba70 100644
--- a/sound/soc/atmel/sam9g20_wm8731.c
+++ b/sound/soc/atmel/sam9g20_wm8731.c
@@ -127,8 +127,8 @@ static int at91sam9g20ek_audio_probe(struct platform_device *pdev)
 
 	ret = atmel_ssc_set_audio(0);
 	if (ret) {
-		dev_err(&pdev->dev, "ssc channel is not valid\n");
-		return -EINVAL;
+		dev_err(&pdev->dev, "ssc channel is not valid: %d\n", ret);
+		return ret;
 	}
 
 	card->dev = &pdev->dev;
-- 
2.30.2

Re: [PATCH v1 1/3] ASoC: atmel: Remove system clock tree configuration for at91sam9g20ek

[Codrin.Ciubotariu]

On 25.03.2022 17:42, Mark Brown wrote:
> The MCLK of the WM8731 on the AT91SAM9G20-EK board is connected to the
> PCK0 output of the SoC, intended in the reference software to be supplied
> using PLLB and programmed to 12MHz. As originally written for use with a
> board file the audio driver was responsible for configuring the entire tree
> but in the conversion to the common clock framework the registration of
> the named pck0 and pllb clocks was removed so the driver has failed to
> instantiate ever since.
> 
> Since the WM8731 driver has had support for managing a MCLK provided via
> the common clock framework for some time we can simply drop all the clock
> management code from the machine driver other than configuration of the
> sysclk rate, the CODEC driver still respects that configuration from the
> machine driver.
> 
> Fixes: ff78a189b0ae55f ("ARM: at91: remove old at91-specific clock driver")
> Signed-off-by: Mark Brown <broonie@kernel.org>

Reviewed-by: Codrin Ciubotariu <codrin.ciubotariu@microchip.com>

Thank you for addressing this!

Best regards,
Codrin

Re: [PATCH v1 2/3] ASoC: atmel: Fix error handling in at91samg20ek probe()

[Codrin.Ciubotariu]

On 25.03.2022 17:42, Mark Brown wrote:
> The error handling in the AT91SAM9G20-EK machine driver probe did not
> consistently free the SSC in error paths, sometimes immediately returning
> an error rather than doing cleanup. Fix this.
> 
> Signed-off-by: Mark Brown <broonie@kernel.org>

Should we have a 'Fixes' tag here?
Other than that:

Reviewed-by: Codrin Ciubotariu <codrin.ciubotariu@microchip.com>

Thanks!

Re: [PATCH v1 3/3] ASoC: atmel: Don't squash error codes from atmel_ssc_set_audio()

[Codrin.Ciubotariu]

On 25.03.2022 17:42, Mark Brown wrote:
> The AT91SAM9G20-EK audio driver is replacing any error code returned by
> atmel_ssc_set_audio() with -EINVAL which could be unhelpful for debugging.
> Pass through the error code, and include it in the log message we print for
> good measure.
> 
> Signed-off-by: Mark Brown <broonie@kernel.org>

Reviewed-by: Codrin Ciubotariu <codrin.ciubotariu@microchip.com>

Thanks!

Re: [PATCH v1 2/3] ASoC: atmel: Fix error handling in at91samg20ek probe()

[Mark Brown]

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

On Mon, Apr 04, 2022 at 12:47:27PM +0000, Codrin.Ciubotariu@microchip.com wrote:
> On 25.03.2022 17:42, Mark Brown wrote:
> > The error handling in the AT91SAM9G20-EK machine driver probe did not
> > consistently free the SSC in error paths, sometimes immediately returning
> > an error rather than doing cleanup. Fix this.
> > 
> > Signed-off-by: Mark Brown <broonie@kernel.org>
> 
> Should we have a 'Fixes' tag here?

I'm not sure those code paths are ever executing, I really don't think
it's worth the effort or noise checking.

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

Re: [PATCH v1 0/3] ASoC: atmel: Fixes for AT91SAM9G20-EK audio driver

[Mark Brown]

On Fri, 25 Mar 2022 15:42:38 +0000, Mark Brown wrote:
> At some point the machine driver for the audio subsystem on the
> AT91SAM9G20-EK appears to have bitrotted, no longer probing due to
> effects of the transition to the common clock framework. The first patch
> in this series fixes the initial probe problem, with the rest of the
> series being random other fixes and cleanups I noticed while looking at
> the driver.
> 
> [...]

Applied to

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

Thanks!

[1/3] ASoC: atmel: Remove system clock tree configuration for at91sam9g20ek
      commit: c775cbf62ed4911e4f0f23880f01815753123690
[2/3] ASoC: atmel: Fix error handling in at91samg20ek probe()
      commit: 28103509248b94392e04a8ffcbc47da5e3e31dfc
[3/3] ASoC: atmel: Don't squash error codes from atmel_ssc_set_audio()
      commit: 01251dd004d8e106295c3aa8e3ba890f0dd55e02

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