Skip to contentSkip to conversation

ASoC: jz47xx: Convert to devm_clk_get_enabled()

PATCH In review Jihed Chaibi 2 participants 5 messages Mar 23

Subsystems (4)
Sound MIPS ASoC Ingenic JZ47xx SoCs
lore
Share & tools
lore

Jihed Chaibi

author
#0 lore raw

cover ASoC: jz47xx: Convert to devm_clk_get_enabled()

The jz4725b, jz4760 and jz4770 Ingenic codec drivers all share the same clock management pattern: the clock is obtained with devm_clk_get() in the platform probe, then manually enabled in the component probe and disabled in the component remove. The clk_prepare_enable() call in the component probe is unchecked, meaning clock enable failures are silently ignored and can lead to register access on unpowered hardware.

This series converts all three drivers to devm_clk_get_enabled(), which combines the get, prepare and enable steps and ties the clock lifetime to the device via devres. The now-redundant component remove callbacks and the struct clk pointers in the private structs are removed.

Jihed Chaibi (3):
ASoC: jz4725b: Convert to devm_clk_get_enabled()
ASoC: jz4760: Convert to devm_clk_get_enabled()
ASoC: jz4770: Convert to devm_clk_get_enabled()

 sound/soc/codecs/jz4725b.c | 18 ++++--------------
 sound/soc/codecs/jz4760.c  | 20 ++++----------------
 sound/soc/codecs/jz4770.c  | 20 ++++----------------
 3 files changed, 12 insertions(+), 46 deletions(-)
Signature · 1 line
2.47.3
author
#1 lore raw

PATCH 1/3 ASoC: jz4725b: Convert to devm_clk_get_enabled()

The clock is obtained with devm_clk_get() in the platform probe, then manually enabled in the component probe and disabled in the component remove without checking the return value of clk_prepare_enable().

Use devm_clk_get_enabled() instead, which combines the get, prepare and enable operations into one call whose lifetime is tied to the device. This removes the need for explicit enable/disable in the component probe/remove callbacks, and ensures that clock enable failures are propagated as errors rather than silently ignored.

Remove the now-unused struct clk pointer from struct jz_icdc and drop the empty component remove callback.

Signed-off-by Jihed Chaibi <[email protected]>
---
 sound/soc/codecs/jz4725b.c | 18 ++++--------------
 1 file changed, 4 insertions(+), 14 deletions(-)
sound/soc/codecs/jz4725b.c +4 -14
index 39cebaa167be..8a7d26a08c03 100644
@@ -160,7 +160,6 @@ enum {
 struct jz_icdc {
 	struct regmap *regmap;
 	void __iomem *base;
-	struct clk *clk;
 };
 
 static const SNDRV_CTL_TLVD_DECLARE_DB_SCALE(jz4725b_adc_tlv,     0, 150, 0);

@@ -405,8 +404,6 @@ static int jz4725b_codec_dev_probe(struct snd_soc_component *component)
 	struct jz_icdc *icdc = snd_soc_component_get_drvdata(component);
 	struct regmap *map = icdc->regmap;
 
-	clk_prepare_enable(icdc->clk);
-
 	/* Write CONFIGn (n=1 to 8) bits.
 	 * The value 0x0f is specified in the datasheet as a requirement.
 	 */

@@ -418,16 +415,8 @@ static int jz4725b_codec_dev_probe(struct snd_soc_component *component)
 	return 0;
 }
 
-static void jz4725b_codec_dev_remove(struct snd_soc_component *component)
-{
-	struct jz_icdc *icdc = snd_soc_component_get_drvdata(component);
-
-	clk_disable_unprepare(icdc->clk);
-}
-
 static const struct snd_soc_component_driver jz4725b_codec = {
 	.probe			= jz4725b_codec_dev_probe,
-	.remove			= jz4725b_codec_dev_remove,
 	.set_bias_level		= jz4725b_codec_set_bias_level,
 	.controls		= jz4725b_codec_controls,
 	.num_controls		= ARRAY_SIZE(jz4725b_codec_controls),

@@ -618,6 +607,7 @@ static int jz4725b_codec_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct jz_icdc *icdc;
+	struct clk *clk;
 	int ret;
 
 	icdc = devm_kzalloc(dev, sizeof(*icdc), GFP_KERNEL);

@@ -633,9 +623,9 @@ static int jz4725b_codec_probe(struct platform_device *pdev)
 	if (IS_ERR(icdc->regmap))
 		return PTR_ERR(icdc->regmap);
 
-	icdc->clk = devm_clk_get(&pdev->dev, "aic");
-	if (IS_ERR(icdc->clk))
-		return PTR_ERR(icdc->clk);
+	clk = devm_clk_get_enabled(dev, "aic");
+	if (IS_ERR(clk))
+		return PTR_ERR(clk);
 
 	platform_set_drvdata(pdev, icdc);
Signature · 1 line
2.47.3
maintainer
#2 lore raw

Re: [PATCH 1/3] ASoC: jz4725b: Convert to devm_clk_get_enabled()

Quoted 5 lines · Jihed Chaibi

Use devm_clk_get_enabled() instead, which combines the get, prepare and enable operations into one call whose lifetime is tied to the device. This removes the need for explicit enable/disable in the component probe/remove callbacks, and ensures that clock enable failures are propagated as errors rather than silently ignored.

This will increase power consumption a little bit in the case where we don't actually instantiate a sound card, the clocks will be turned on at device probe time. How often these systems don't have a sound card, and how noticable the additional power consumption from running the clocks is, are separate questions - it might not be worth worrying about.

author
#3 lore raw

PATCH 2/3 ASoC: jz4760: Convert to devm_clk_get_enabled()

In reply to Jihed Chaibi

The clock is obtained with devm_clk_get() in the platform probe, then manually enabled in the component probe and disabled in the component remove without checking the return value of clk_prepare_enable().

Use devm_clk_get_enabled() instead, which combines the get, prepare and enable operations into one call whose lifetime is tied to the device. This removes the need for explicit enable/disable in the component probe/remove callbacks, and ensures that clock enable failures are propagated as errors rather than silently ignored.

Remove the now-unused struct clk pointer from struct jz_codec and drop the empty component remove callback.

Signed-off-by Jihed Chaibi <[email protected]>
---
 sound/soc/codecs/jz4760.c | 20 ++++----------------
 1 file changed, 4 insertions(+), 16 deletions(-)
sound/soc/codecs/jz4760.c +4 -16
index 344c251be397..6846ace06415 100644
@@ -163,7 +163,6 @@ struct jz_codec {
 	struct device *dev;
 	struct regmap *regmap;
 	void __iomem *base;
-	struct clk *clk;
 };
 
 static int jz4760_codec_set_bias_level(struct snd_soc_component *codec,

@@ -602,25 +601,13 @@ static void jz4760_codec_codec_init_regs(struct snd_soc_component *codec)
 
 static int jz4760_codec_codec_probe(struct snd_soc_component *codec)
 {
-	struct jz_codec *jz_codec = snd_soc_component_get_drvdata(codec);
-
-	clk_prepare_enable(jz_codec->clk);
-
 	jz4760_codec_codec_init_regs(codec);
 
 	return 0;
 }
 
-static void jz4760_codec_codec_remove(struct snd_soc_component *codec)
-{
-	struct jz_codec *jz_codec = snd_soc_component_get_drvdata(codec);
-
-	clk_disable_unprepare(jz_codec->clk);
-}
-
 static const struct snd_soc_component_driver jz4760_codec_soc_codec_dev = {
 	.probe			= jz4760_codec_codec_probe,
-	.remove			= jz4760_codec_codec_remove,
 	.set_bias_level		= jz4760_codec_set_bias_level,
 	.controls		= jz4760_codec_snd_controls,
 	.num_controls		= ARRAY_SIZE(jz4760_codec_snd_controls),

@@ -818,6 +805,7 @@ static int jz4760_codec_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct jz_codec *codec;
+	struct clk *clk;
 	int ret;
 
 	codec = devm_kzalloc(dev, sizeof(*codec), GFP_KERNEL);

@@ -835,9 +823,9 @@ static int jz4760_codec_probe(struct platform_device *pdev)
 	if (IS_ERR(codec->regmap))
 		return PTR_ERR(codec->regmap);
 
-	codec->clk = devm_clk_get(dev, "aic");
-	if (IS_ERR(codec->clk))
-		return PTR_ERR(codec->clk);
+	clk = devm_clk_get_enabled(dev, "aic");
+	if (IS_ERR(clk))
+		return PTR_ERR(clk);
 
 	platform_set_drvdata(pdev, codec);
Signature · 1 line
2.47.3
author
#4 lore raw

PATCH 3/3 ASoC: jz4770: Convert to devm_clk_get_enabled()

In reply to Jihed Chaibi

The clock is obtained with devm_clk_get() in the platform probe, then manually enabled in the component probe and disabled in the component remove without checking the return value of clk_prepare_enable().

Use devm_clk_get_enabled() instead, which combines the get, prepare and enable operations into one call whose lifetime is tied to the device. This removes the need for explicit enable/disable in the component probe/remove callbacks, and ensures that clock enable failures are propagated as errors rather than silently ignored.

Remove the now-unused struct clk pointer from struct jz_codec and drop the empty component remove callback.

Signed-off-by Jihed Chaibi <[email protected]>
---
 sound/soc/codecs/jz4770.c | 20 ++++----------------
 1 file changed, 4 insertions(+), 16 deletions(-)
sound/soc/codecs/jz4770.c +4 -16
index 6b86d47028d7..be1ecdcc737b 100644
@@ -179,7 +179,6 @@ struct jz_codec {
 	struct device *dev;
 	struct regmap *regmap;
 	void __iomem *base;
-	struct clk *clk;
 };
 
 static int jz4770_codec_set_bias_level(struct snd_soc_component *codec,

@@ -634,25 +633,13 @@ static void jz4770_codec_codec_init_regs(struct snd_soc_component *codec)
 
 static int jz4770_codec_codec_probe(struct snd_soc_component *codec)
 {
-	struct jz_codec *jz_codec = snd_soc_component_get_drvdata(codec);
-
-	clk_prepare_enable(jz_codec->clk);
-
 	jz4770_codec_codec_init_regs(codec);
 
 	return 0;
 }
 
-static void jz4770_codec_codec_remove(struct snd_soc_component *codec)
-{
-	struct jz_codec *jz_codec = snd_soc_component_get_drvdata(codec);
-
-	clk_disable_unprepare(jz_codec->clk);
-}
-
 static const struct snd_soc_component_driver jz4770_codec_soc_codec_dev = {
 	.probe			= jz4770_codec_codec_probe,
-	.remove			= jz4770_codec_codec_remove,
 	.set_bias_level		= jz4770_codec_set_bias_level,
 	.controls		= jz4770_codec_snd_controls,
 	.num_controls		= ARRAY_SIZE(jz4770_codec_snd_controls),

@@ -865,6 +852,7 @@ static int jz4770_codec_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct jz_codec *codec;
+	struct clk *clk;
 	int ret;
 
 	codec = devm_kzalloc(dev, sizeof(*codec), GFP_KERNEL);

@@ -882,9 +870,9 @@ static int jz4770_codec_probe(struct platform_device *pdev)
 	if (IS_ERR(codec->regmap))
 		return PTR_ERR(codec->regmap);
 
-	codec->clk = devm_clk_get(dev, "aic");
-	if (IS_ERR(codec->clk))
-		return PTR_ERR(codec->clk);
+	clk = devm_clk_get_enabled(dev, "aic");
+	if (IS_ERR(clk))
+		return PTR_ERR(clk);
 
 	platform_set_drvdata(pdev, codec);
Signature · 1 line
2.47.3