ASoC: sgtl5000: Let the codec acquire its clock

PATCH Applied Fabio Estevam 3 participants 7 messages Jun 10, 2013 – Jul 15, 2013

Subsystems (6)

Device tree bindings Sound Arm i.MX and Layerscape ASoC +2 more subsystems

lore

Share & tools

lore

Fabio Estevam

author maintainer

2013-06-10 01:07 UTC #0 lore raw

PATCH 1/2 ASoC: sgtl5000: Let the codec acquire its clock

From: Fabio Estevam <[email protected]>

On a mx6qsabrelite board the following error happens on probe:

sgtl5000: probe of 0-000a failed with error -5 imx-sgtl5000 sound.13: ASoC: CODEC (null) not registered imx-sgtl5000 sound.13: snd_soc_register_card failed (-517) platform sound.13: Driver imx-sgtl5000 requests probe defer

Prior to reading the codec ID we need to turn the SYS_MCLK clock, so let's enable the codec clock inside sgtl5000_i2c_probe().

Also remove the codec clock enable/disable functions from the machine driver.

Signed-off-by Fabio Estevam <[email protected]>

---
Shawn,

I haven't had a chance to convert mx28 to pass the codec clock via device tree yet. It looks like it is a bit trickier there, as it uses the saif clock.

Given that mx28 audio playback is already broken in linux-next, this series does not make things worse on mx28 side.

 sound/soc/codecs/sgtl5000.c  | 34 ++++++++++++++++++++++++++++++----
 sound/soc/fsl/imx-sgtl5000.c | 30 +++++++-----------------------
 2 files changed, 37 insertions(+), 27 deletions(-)

sound/soc/codecs/sgtl5000.c +30 -4

index c8f2afb..2e0227b 100644
@@ -114,6 +114,7 @@ struct sgtl5000_priv {
 	struct regulator_bulk_data supplies[SGTL5000_SUPPLY_NUM];
 	struct ldo_regulator *ldo;
 	struct regmap *regmap;
+	struct clk *mclk;
 };
 
 /*

@@ -1522,16 +1523,28 @@ static int sgtl5000_i2c_probe(struct i2c_client *client,
 		return ret;
 	}
 
+	sgtl5000->mclk = devm_clk_get(&client->dev, NULL);
+	if (IS_ERR(sgtl5000->mclk)) {
+		ret = PTR_ERR(sgtl5000->mclk);
+		dev_err(&client->dev, "Failed to get mclock: %d\n", ret);
+		return ret;
+	}
+
+	ret = clk_prepare_enable(sgtl5000->mclk);
+	if (ret)
+		return ret;
+
 	/* read chip information */
 	ret = regmap_read(sgtl5000->regmap, SGTL5000_CHIP_ID, &reg);
 	if (ret)
-		return ret;
+		goto disable_clk;
 
 	if (((reg & SGTL5000_PARTID_MASK) >> SGTL5000_PARTID_SHIFT) !=
 	    SGTL5000_PARTID_PART_ID) {
 		dev_err(&client->dev,
 			"Device with ID register %x is not a sgtl5000\n", reg);
-		return -ENODEV;
+		ret = -ENODEV;
+		goto disable_clk;
 	}
 
 	rev = (reg & SGTL5000_REVID_MASK) >> SGTL5000_REVID_SHIFT;

@@ -1542,17 +1555,30 @@ static int sgtl5000_i2c_probe(struct i2c_client *client,
 	/* Ensure sgtl5000 will start with sane register values */
 	ret = sgtl5000_fill_defaults(sgtl5000);
 	if (ret)
-		return ret;
+		goto disable_clk;
 
 	ret = snd_soc_register_codec(&client->dev,
 			&sgtl5000_driver, &sgtl5000_dai, 1);
+	if (ret)
+		goto disable_clk;
+
+	return 0;
+
+disable_clk:
+	clk_disable_unprepare(sgtl5000->mclk);
 	return ret;
 }
 
 static int sgtl5000_i2c_remove(struct i2c_client *client)
 {
-	snd_soc_unregister_codec(&client->dev);
+	struct sgtl5000_priv *sgtl5000;
+	sgtl5000 = devm_kzalloc(&client->dev, sizeof(struct sgtl5000_priv),
+								GFP_KERNEL);
+	if (!sgtl5000)
+		return -ENOMEM;
 
+	snd_soc_unregister_codec(&client->dev);
+	clk_disable_unprepare(sgtl5000->mclk);
 	return 0;
 }

sound/soc/fsl/imx-sgtl5000.c +7 ~~-23~~

index a60aaa0..823151b 100644
@@ -129,20 +129,10 @@ static int imx_sgtl5000_probe(struct platform_device *pdev)
 	}
 
 	data->codec_clk = clk_get(&codec_dev->dev, NULL);
-	if (IS_ERR(data->codec_clk)) {
-		/* assuming clock enabled by default */
-		data->codec_clk = NULL;
-		ret = of_property_read_u32(codec_np, "clock-frequency",
-					&data->clk_frequency);
-		if (ret) {
-			dev_err(&codec_dev->dev,
-				"clock-frequency missing or invalid\n");
-			goto fail;
-		}
-	} else {
-		data->clk_frequency = clk_get_rate(data->codec_clk);
-		clk_prepare_enable(data->codec_clk);
-	}
+	if (IS_ERR(data->codec_clk))
+		goto fail;
+
+	data->clk_frequency = clk_get_rate(data->codec_clk);
 
 	data->dai.name = "HiFi";
 	data->dai.stream_name = "HiFi";

@@ -157,10 +147,10 @@ static int imx_sgtl5000_probe(struct platform_device *pdev)
 	data->card.dev = &pdev->dev;
 	ret = snd_soc_of_parse_card_name(&data->card, "model");
 	if (ret)
-		goto clk_fail;
+		goto fail;
 	ret = snd_soc_of_parse_audio_routing(&data->card, "audio-routing");
 	if (ret)
-		goto clk_fail;
+		goto fail;
 	data->card.num_links = 1;
 	data->card.owner = THIS_MODULE;
 	data->card.dai_link = &data->dai;

@@ -170,7 +160,7 @@ static int imx_sgtl5000_probe(struct platform_device *pdev)
 	ret = snd_soc_register_card(&data->card);
 	if (ret) {
 		dev_err(&pdev->dev, "snd_soc_register_card failed (%d)\n", ret);
-		goto clk_fail;
+		goto fail;
 	}
 
 	platform_set_drvdata(pdev, data);

@@ -179,8 +169,6 @@ static int imx_sgtl5000_probe(struct platform_device *pdev)
 
 	return 0;
 
-clk_fail:
-	clk_put(data->codec_clk);
 fail:
 	if (ssi_np)
 		of_node_put(ssi_np);

@@ -194,10 +182,6 @@ static int imx_sgtl5000_remove(struct platform_device *pdev)
 {
 	struct imx_sgtl5000_data *data = platform_get_drvdata(pdev);
 
-	if (data->codec_clk) {
-		clk_disable_unprepare(data->codec_clk);
-		clk_put(data->codec_clk);
-	}
 	snd_soc_unregister_card(&data->card);
 
 	return 0;

Signature · 1 line

1.8.1.2

Fabio Estevam

author maintainer

2013-06-10 01:07 UTC #1 lore raw

PATCH 2/2 ARM: dts: imx51-babbage: Pass a real clock to the codec

From: Fabio Estevam <[email protected]>

On imx51_babbage the codec clock is activated via GPIO4_26.

Provide a real clock to the sgtl5000 codec via device tree.

Signed-off-by Fabio Estevam <[email protected]>

---
 arch/arm/boot/dts/imx51-babbage.dts | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

arch/arm/boot/dts/imx51-babbage.dts +12 -1

index 6dd9486..27b4061 100644
@@ -61,6 +61,16 @@
 		mux-int-port = <2>;
 		mux-ext-port = <3>;
 	};
+
+	clocks {
+		clk_26M: codec_clock {
+			compatible = "fixed-clock";
+			reg=<0>;
+			#clock-cells = <0>;
+			clock-frequency = <26000000>;
+			gpios = <&gpio4 26 1>;
+		};
+	};
 };
 
 &esdhc1 {

@@ -229,6 +239,7 @@
 				MX51_PAD_EIM_A27__GPIO2_21   0x5
 				MX51_PAD_CSPI1_SS0__GPIO4_24 0x85
 				MX51_PAD_CSPI1_SS1__GPIO4_25 0x85
+				MX51_PAD_CSPI1_RDY__GPIO4_26 0x80000000
 			>;
 		};
 	};

@@ -255,7 +266,7 @@
 	sgtl5000: codec at 0a {
 		compatible = "fsl,sgtl5000";
 		reg = <0x0a>;
-		clock-frequency = <26000000>;
+		clocks = <&clk_26M>;
 		VDDA-supply = <&vdig_reg>;
 		VDDIO-supply = <&vvideo_reg>;
 	};

Signature · 1 line

1.8.1.2

Shawn Guo

Applied

2013-07-15 14:30 UTC #2 lore raw

Re: [PATCH 2/2] ARM: dts: imx51-babbage: Pass a real clock to the codec

Quoted 7 lines · Fabio Estevam

From: Fabio Estevam <[email protected]>
On imx51_babbage the codec clock is activated via GPIO4_26.
Provide a real clock to the sgtl5000 codec via device tree.
Signed-off-by Fabio Estevam <[email protected]>

Applied, thanks.

Shawn Guo

2013-06-10 08:49 UTC #3 lore raw

In reply to Fabio Estevam

Quoted 15 lines · Fabio Estevam

From: Fabio Estevam <[email protected]>
On a mx6qsabrelite board the following error happens on probe:
sgtl5000: probe of 0-000a failed with error -5 imx-sgtl5000 sound.13: ASoC: CODEC (null) not registered imx-sgtl5000 sound.13: snd_soc_register_card failed (-517) platform sound.13: Driver imx-sgtl5000 requests probe defer
Prior to reading the codec ID we need to turn the SYS_MCLK clock, so let's enable the codec clock inside sgtl5000_i2c_probe().
Also remove the codec clock enable/disable functions from the machine driver.
Signed-off-by Fabio Estevam <[email protected]>

Quoted 8 lines

---
Shawn,
I haven't had a chance to convert mx28 to pass the codec clock via device tree yet. It looks like it is a bit trickier there, as it uses the saif clock.
Given that mx28 audio playback is already broken in linux-next, this series does not make things worse on mx28 side.

Okay. But I assume you will get it fixed soon.

Quoted 63 lines

 sound/soc/codecs/sgtl5000.c  | 34 ++++++++++++++++++++++++++++++----
 sound/soc/fsl/imx-sgtl5000.c | 30 +++++++-----------------------
 2 files changed, 37 insertions(+), 27 deletions(-)

sound/soc/codecs/sgtl5000.c +22 -3

index c8f2afb..2e0227b 100644
@@ -114,6 +114,7 @@ struct sgtl5000_priv {
 	struct regulator_bulk_data supplies[SGTL5000_SUPPLY_NUM];
 	struct ldo_regulator *ldo;
 	struct regmap *regmap;
+	struct clk *mclk;
 };
 
 /*

@@ -1522,16 +1523,28 @@ static int sgtl5000_i2c_probe(struct i2c_client *client,
 		return ret;
 	}
 
+	sgtl5000->mclk = devm_clk_get(&client->dev, NULL);
+	if (IS_ERR(sgtl5000->mclk)) {
+		ret = PTR_ERR(sgtl5000->mclk);
+		dev_err(&client->dev, "Failed to get mclock: %d\n", ret);
+		return ret;
+	}
+
+	ret = clk_prepare_enable(sgtl5000->mclk);
+	if (ret)
+		return ret;
+
 	/* read chip information */
 	ret = regmap_read(sgtl5000->regmap, SGTL5000_CHIP_ID, &reg);
 	if (ret)
-		return ret;
+		goto disable_clk;
 
 	if (((reg & SGTL5000_PARTID_MASK) >> SGTL5000_PARTID_SHIFT) !=
 	    SGTL5000_PARTID_PART_ID) {
 		dev_err(&client->dev,
 			"Device with ID register %x is not a sgtl5000\n", reg);
-		return -ENODEV;
+		ret = -ENODEV;
+		goto disable_clk;
 	}
 
 	rev = (reg & SGTL5000_REVID_MASK) >> SGTL5000_REVID_SHIFT;

@@ -1542,17 +1555,30 @@ static int sgtl5000_i2c_probe(struct i2c_client *client,
 	/* Ensure sgtl5000 will start with sane register values */
 	ret = sgtl5000_fill_defaults(sgtl5000);
 	if (ret)
-		return ret;
+		goto disable_clk;
 
 	ret = snd_soc_register_codec(&client->dev,
 			&sgtl5000_driver, &sgtl5000_dai, 1);
+	if (ret)
+		goto disable_clk;
+
+	return 0;
+
+disable_clk:

sound/soc/codecs/sgtl5000.c static int sgtl5000_i2c_probe(struct i2c_client *client, · line 1576

Quoted 10 lines

sound/soc/codecs/sgtl5000.c +4 -1

+	clk_disable_unprepare(sgtl5000->mclk);
 	return ret;
 }
 
 static int sgtl5000_i2c_remove(struct i2c_client *client)
 {
-	snd_soc_unregister_codec(&client->dev);
+	struct sgtl5000_priv *sgtl5000;
+	sgtl5000 = devm_kzalloc(&client->dev, sizeof(struct sgtl5000_priv),
+								GFP_KERNEL);

Are you kidding? You should call i2c_get_clientdata() rather than devm_kzalloc() to get the pointer.

Quoted 24 lines

sound/soc/codecs/sgtl5000.c +4 -0

+	if (!sgtl5000)
+		return -ENOMEM;
 
+	snd_soc_unregister_codec(&client->dev);
+	clk_disable_unprepare(sgtl5000->mclk);
 	return 0;
 }

sound/soc/fsl/imx-sgtl5000.c +0 -8

index a60aaa0..823151b 100644
@@ -129,20 +129,10 @@ static int imx_sgtl5000_probe(struct platform_device *pdev)
 	}
 
 	data->codec_clk = clk_get(&codec_dev->dev, NULL);
-	if (IS_ERR(data->codec_clk)) {
-		/* assuming clock enabled by default */
-		data->codec_clk = NULL;
-		ret = of_property_read_u32(codec_np, "clock-frequency",
-					&data->clk_frequency);
-		if (ret) {
-			dev_err(&codec_dev->dev,
-				"clock-frequency missing or invalid\n");

sound/soc/fsl/imx-sgtl5000.c static int imx_sgtl5000_probe(struct platform_device *pdev) · line 135

Quoted 10 lines

sound/soc/fsl/imx-sgtl5000.c +4 -6

-			goto fail;
-		}
-	} else {
-		data->clk_frequency = clk_get_rate(data->codec_clk);
-		clk_prepare_enable(data->codec_clk);
-	}
+	if (IS_ERR(data->codec_clk))
+		goto fail;
+
+	data->clk_frequency = clk_get_rate(data->codec_clk);

With codec driver setting up the clock now, the only use of codec_clk n this machine driver is getting the rate and setting up sysclk with snd_soc_dai_set_sysclk(). I'm wondering if it makes more sense to move all these over into codec driver, so that machine driver does not need to touch the clock at all.

Shawn

Quoted 48 lines

sound/soc/fsl/imx-sgtl5000.c +3 -9

 	data->dai.name = "HiFi";
 	data->dai.stream_name = "HiFi";

@@ -157,10 +147,10 @@ static int imx_sgtl5000_probe(struct platform_device *pdev)
 	data->card.dev = &pdev->dev;
 	ret = snd_soc_of_parse_card_name(&data->card, "model");
 	if (ret)
-		goto clk_fail;
+		goto fail;
 	ret = snd_soc_of_parse_audio_routing(&data->card, "audio-routing");
 	if (ret)
-		goto clk_fail;
+		goto fail;
 	data->card.num_links = 1;
 	data->card.owner = THIS_MODULE;
 	data->card.dai_link = &data->dai;

@@ -170,7 +160,7 @@ static int imx_sgtl5000_probe(struct platform_device *pdev)
 	ret = snd_soc_register_card(&data->card);
 	if (ret) {
 		dev_err(&pdev->dev, "snd_soc_register_card failed (%d)\n", ret);
-		goto clk_fail;
+		goto fail;
 	}
 
 	platform_set_drvdata(pdev, data);

@@ -179,8 +169,6 @@ static int imx_sgtl5000_probe(struct platform_device *pdev)
 
 	return 0;
 
-clk_fail:
-	clk_put(data->codec_clk);
 fail:
 	if (ssi_np)
 		of_node_put(ssi_np);

@@ -194,10 +182,6 @@ static int imx_sgtl5000_remove(struct platform_device *pdev)
 {
 	struct imx_sgtl5000_data *data = platform_get_drvdata(pdev);
 
-	if (data->codec_clk) {
-		clk_disable_unprepare(data->codec_clk);
-		clk_put(data->codec_clk);
-	}
 	snd_soc_unregister_card(&data->card);
 
 	return 0;

Signature · 1 line

1.8.1.2

Mark Brown

maintainer

2013-06-10 09:27 UTC #4 lore raw

In reply to Fabio Estevam

sound/soc/fsl/imx-sgtl5000.c static int imx_sgtl5000_probe(struct platform_device *pdev) · line 141

Quoted 10 lines · Fabio Estevam

sound/soc/fsl/imx-sgtl5000.c +0 ~~-10~~

-	if (IS_ERR(data->codec_clk)) {
-		/* assuming clock enabled by default */
-		data->codec_clk = NULL;
-		ret = of_property_read_u32(codec_np, "clock-frequency",
-					&data->clk_frequency);
-		if (ret) {
-			dev_err(&codec_dev->dev,
-				"clock-frequency missing or invalid\n");
-			goto fail;
-		}

Applied but this means that the binding document needs to be updated to
remove the clock-frequency property.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130610/5a2535da/attachment.sig>

Shawn Guo

2013-06-10 15:41 UTC #5 lore raw

In reply to Fabio Estevam

Quoted 52 lines · Fabio Estevam

sound/soc/fsl/imx-sgtl5000.c +7 ~~-17~~

index a60aaa0..823151b 100644
@@ -129,20 +129,10 @@ static int imx_sgtl5000_probe(struct platform_device *pdev)
 	}
 
 	data->codec_clk = clk_get(&codec_dev->dev, NULL);
-	if (IS_ERR(data->codec_clk)) {
-		/* assuming clock enabled by default */
-		data->codec_clk = NULL;
-		ret = of_property_read_u32(codec_np, "clock-frequency",
-					&data->clk_frequency);
-		if (ret) {
-			dev_err(&codec_dev->dev,
-				"clock-frequency missing or invalid\n");
-			goto fail;
-		}
-	} else {
-		data->clk_frequency = clk_get_rate(data->codec_clk);
-		clk_prepare_enable(data->codec_clk);
-	}
+	if (IS_ERR(data->codec_clk))
+		goto fail;
+
+	data->clk_frequency = clk_get_rate(data->codec_clk);
 
 	data->dai.name = "HiFi";
 	data->dai.stream_name = "HiFi";

@@ -157,10 +147,10 @@ static int imx_sgtl5000_probe(struct platform_device *pdev)
 	data->card.dev = &pdev->dev;
 	ret = snd_soc_of_parse_card_name(&data->card, "model");
 	if (ret)
-		goto clk_fail;
+		goto fail;
 	ret = snd_soc_of_parse_audio_routing(&data->card, "audio-routing");
 	if (ret)
-		goto clk_fail;
+		goto fail;
 	data->card.num_links = 1;
 	data->card.owner = THIS_MODULE;
 	data->card.dai_link = &data->dai;

@@ -170,7 +160,7 @@ static int imx_sgtl5000_probe(struct platform_device *pdev)
 	ret = snd_soc_register_card(&data->card);
 	if (ret) {
 		dev_err(&pdev->dev, "snd_soc_register_card failed (%d)\n", ret);
-		goto clk_fail;
+		goto fail;
 	}
 
 	platform_set_drvdata(pdev, data);

sound/soc/fsl/imx-sgtl5000.c static int imx_sgtl5000_probe(struct platform_device *pdev) · line 183

Quoted 5 lines

sound/soc/fsl/imx-sgtl5000.c +0 -2

@@ -179,8 +169,6 @@ static int imx_sgtl5000_probe(struct platform_device *pdev)
 
 	return 0;
 
-clk_fail:
-	clk_put(data->codec_clk);

Why can this clk_put() be removed now?

sound/soc/fsl/imx-sgtl5000.c static int imx_sgtl5000_remove(struct platform_device *pdev) · line 199

Quoted 10 lines

sound/soc/fsl/imx-sgtl5000.c +0 -3

 fail:
 	if (ssi_np)
 		of_node_put(ssi_np);

@@ -194,10 +182,6 @@ static int imx_sgtl5000_remove(struct platform_device *pdev)
 {
 	struct imx_sgtl5000_data *data = platform_get_drvdata(pdev);
 
-	if (data->codec_clk) {
-		clk_disable_unprepare(data->codec_clk);
-		clk_put(data->codec_clk);

Ditto

Shawn

Quoted 7 lines

sound/soc/fsl/imx-sgtl5000.c +0 -1
-	}
 	snd_soc_unregister_card(&data->card);
 
 	return 0;
Signature · 1 line
1.8.1.2

Fabio Estevam

author maintainer

2013-06-10 16:07 UTC #6 lore raw

Quoted 4 lines · Shawn Guo

sound/soc/fsl/imx-sgtl5000.c static int imx_sgtl5000_probe(struct platform_device *pdev) · line 183
Quoted 2 lines
sound/soc/fsl/imx-sgtl5000.c +0 -2
-clk_fail:
-     clk_put(data->codec_clk);
Why can this clk_put() be removed now?

Good point. Will send a patch to convert it to devm_clk_get, so that we do not need to call these clk_put.