Clean up RK3328 audio codec GPIO control

PATCH In review Robin Murphy 4 participants 9 messages Feb 18, 2020 – Feb 29, 2020

Subsystems (4)

Device tree bindings Sound ASoC Arm Rockchip

lore

Share & tools

lore

Robin Murphy

author

2020-02-18 21:31 UTC #0 lore raw

v2 cover Clean up RK3328 audio codec GPIO control

Hi all,

Since nobody said anything resembling "that looks like a terrible idea!" to what I propsed on top of v1, resending as a full v2 per Mark's request.

Robin.

Robin Murphy (3):
ASoC: dt-bindings: Make RK3328 codec GPIO explicit
ASoC: rockchip: Make RK3328 GPIO_MUTE control explicit
arm64: dts: rockchip: Describe RK3328 GPIO_MUTE users

 .../bindings/sound/rockchip,rk3328-codec.txt  |  7 ++++-
 arch/arm64/boot/dts/rockchip/rk3328-a1.dts    |  1 +
 .../arm64/boot/dts/rockchip/rk3328-rock64.dts |  1 +
 sound/soc/codecs/rk3328_codec.c               | 31 ++++++++++---------
 4 files changed, 24 insertions(+), 16 deletions(-)

Signature · 1 line

2.17.1

Robin Murphy

author

2020-02-18 21:31 UTC #1 lore raw

PATCH v2 1/3 ASoC: dt-bindings: Make RK3328 codec GPIO explicit

Existing RK3328 codec drivers have overloaded the GRF phandle to assume implicit control of the limited-function GPIO_MUTE pin, which is usually used to enable an external audio line driver IC. Since this pin has a proper binding of its own (see gpio/rockchip,rk3328-grf-gpio.txt), make a GPIO explicit in the codec binding too. This will help avoid ambiguity on boards that use that pin for some other purpose.

(and while touching the example, enforce the "don't include status" rule)

Signed-off-by Robin Murphy <[email protected]>

---

v2: no change

 .../devicetree/bindings/sound/rockchip,rk3328-codec.txt    | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

Documentation/devicetree/bindings/sound/rockchip,rk3328-codec.txt +6 -1

index 2469588c7ccb..1ecd75d2032a 100644
@@ -10,6 +10,11 @@ Required properties:
 - clock-names: should be "pclk".
 - spk-depop-time-ms: speak depop time msec.
 
+Optional properties:
+
+- mute-gpios: GPIO specifier for external line driver control (typically the
+              dedicated GPIO_MUTE pin)
+
 Example for rk3328 internal codec:
 
 codec: codec@ff410000 {

@@ -18,6 +23,6 @@ codec: codec@ff410000 {
 	rockchip,grf = <&grf>;
 	clocks = <&cru PCLK_ACODEC>;
 	clock-names = "pclk";
+	mute-gpios = <&grf_gpio 0 GPIO_ACTIVE_LOW>;
 	spk-depop-time-ms = 100;
-	status = "disabled";
 };

Signature · 1 line

2.17.1

Rob Herring

maintainer Acked-by on v2 1/3

2020-02-19 21:38 UTC #2 lore raw

Re: [PATCH v2 1/3] ASoC: dt-bindings: Make RK3328 codec GPIO explicit

Quoted 9 lines · Robin Murphy

Existing RK3328 codec drivers have overloaded the GRF phandle to assume implicit control of the limited-function GPIO_MUTE pin, which is usually used to enable an external audio line driver IC. Since this pin has a proper binding of its own (see gpio/rockchip,rk3328-grf-gpio.txt), make a GPIO explicit in the codec binding too. This will help avoid ambiguity on boards that use that pin for some other purpose.
(and while touching the example, enforce the "don't include status" rule)

Quoted 8 lines

Signed-off-by Robin Murphy <[email protected]>
---
v2: no change
 .../devicetree/bindings/sound/rockchip,rk3328-codec.txt    | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

Acked-by Rob Herring <[email protected]>

Robin Murphy

author

2020-02-18 21:31 UTC #3 lore raw

PATCH v2 2/3 ASoC: rockchip: Make RK3328 GPIO_MUTE control explicit

In reply to Robin Murphy

The RK3328 reference design uses an external line driver IC as a buffer on the analog codec output, enabled by the GPIO_MUTE pin, and such a configuration is currently assumed in the codec driver's direct poking of GRF_SOC_CON10 to control the GPIO_MUTE output value. However, some boards wire up analog audio yet use that pin for some other purpose, so that assumption doesn't always hold. Update this functionality to rely on an explicit GPIO descriptor, such that it can be managed at the board level.

Signed-off-by Robin Murphy <[email protected]>

---

v2:
- add fallback case to avoid possible Rock64 regressions
- propagate GPIO errors; "optional" doesn't really mean "ignore brokenness if present"

 sound/soc/codecs/rk3328_codec.c | 31 ++++++++++++++++---------------
 1 file changed, 16 insertions(+), 15 deletions(-)

sound/soc/codecs/rk3328_codec.c +16 ~~-15~~

index 287c962ba00d..115706a55577 100644
@@ -7,6 +7,7 @@
 #include <linux/clk.h>
 #include <linux/delay.h>
 #include <linux/device.h>
+#include <linux/gpio/consumer.h>
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>

@@ -31,7 +32,7 @@
 
 struct rk3328_codec_priv {
 	struct regmap *regmap;
-	struct regmap *grf;
+	struct gpio_desc *mute;
 	struct clk *mclk;
 	struct clk *pclk;
 	unsigned int sclk;

@@ -106,16 +107,6 @@ static int rk3328_set_dai_fmt(struct snd_soc_dai *dai, unsigned int fmt)
 	return 0;
 }
 
-static void rk3328_analog_output(struct rk3328_codec_priv *rk3328, int mute)
-{
-	unsigned int val = BIT(17);
-
-	if (mute)
-		val |= BIT(1);
-
-	regmap_write(rk3328->grf, RK3328_GRF_SOC_CON10, val);
-}
-
 static int rk3328_digital_mute(struct snd_soc_dai *dai, int mute)
 {
 	struct rk3328_codec_priv *rk3328 =

@@ -205,7 +196,7 @@ static int rk3328_codec_open_playback(struct rk3328_codec_priv *rk3328)
 	}
 
 	msleep(rk3328->spk_depop_time);
-	rk3328_analog_output(rk3328, 1);
+	gpiod_set_value(rk3328->mute, 0);
 
 	regmap_update_bits(rk3328->regmap, HPOUTL_GAIN_CTRL,
 			   HPOUTL_GAIN_MASK, OUT_VOLUME);

@@ -246,7 +237,7 @@ static int rk3328_codec_close_playback(struct rk3328_codec_priv *rk3328)
 {
 	size_t i;
 
-	rk3328_analog_output(rk3328, 0);
+	gpiod_set_value(rk3328->mute, 1);
 
 	regmap_update_bits(rk3328->regmap, HPOUTL_GAIN_CTRL,
 			   HPOUTL_GAIN_MASK, 0);

@@ -446,7 +437,6 @@ static int rk3328_platform_probe(struct platform_device *pdev)
 		dev_err(&pdev->dev, "missing 'rockchip,grf'\n");
 		return PTR_ERR(grf);
 	}
-	rk3328->grf = grf;
 	/* enable i2s_acodec_en */
 	regmap_write(grf, RK3328_GRF_SOC_CON2,
 		     (BIT(14) << 16 | BIT(14)));

@@ -458,7 +448,18 @@ static int rk3328_platform_probe(struct platform_device *pdev)
 		rk3328->spk_depop_time = 200;
 	}
 
-	rk3328_analog_output(rk3328, 0);
+	rk3328->mute = gpiod_get_optional(&pdev->dev, "mute", GPIOD_OUT_HIGH);
+	if (IS_ERR(rk3328->mute))
+		return PTR_ERR(rk3328->mute);
+	/*
+	 * Rock64 is the only supported platform to have widely relied on
+	 * this; if we do happen to come across an old DTB, just leave the
+	 * external mute forced off.
+	 */
+	if (!rk3328->mute && of_machine_is_compatible("pine64,rock64")) {
+		dev_warn(&pdev->dev, "assuming implicit control of GPIO_MUTE; update devicetree if possible\n");
+		regmap_write(grf, RK3328_GRF_SOC_CON10, BIT(17) | BIT(1));
+	}
 
 	rk3328->mclk = devm_clk_get(&pdev->dev, "mclk");
 	if (IS_ERR(rk3328->mclk))

Signature · 1 line

2.17.1

Mark Brown

maintainer

2020-02-19 12:06 UTC #4 lore raw

Re: [PATCH v2 2/3] ASoC: rockchip: Make RK3328 GPIO_MUTE control explicit

Quoted 1 line · Robin Murphy

- add fallback case to avoid possible Rock64 regressions

This should really have been called out in the changelog, were I reading this cold I might've stopped at the changelog and said you needed to keep the ABI stable for old boards.

Mark Brown

maintainer

2020-02-19 20:57 UTC #5 lore raw

Applied "ASoC: rockchip: Make RK3328 GPIO_MUTE control explicit" to the asoc tree

In reply to Robin Murphy

The patch

ASoC: rockchip: Make RK3328 GPIO_MUTE control explicit

has been applied to the asoc tree at

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

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

From 87d12d5545fa72d67d99d797cdb464c0c7efb9c9 Mon Sep 17 00:00:00 2001
From: Robin Murphy <[email protected]>
Date: Tue, 18 Feb 2020 21:31:59 +0000
Subject: [PATCH] ASoC: rockchip: Make RK3328 GPIO_MUTE control explicit

Signed-off-by Robin Murphy <[email protected]>

Link https://lore.kernel.org/r/5bc383ed1832f0f5d1dcb3c97ad92fd68e5217e3.1581376744.git.robin.murphy-5wv7dgnIgG8@public.gmane.org open here

Signed-off-by Mark Brown <[email protected]>

---
 sound/soc/codecs/rk3328_codec.c | 31 ++++++++++++++++---------------
 1 file changed, 16 insertions(+), 15 deletions(-)

sound/soc/codecs/rk3328_codec.c +16 ~~-15~~

index 287c962ba00d..115706a55577 100644
@@ -7,6 +7,7 @@
 #include <linux/clk.h>
 #include <linux/delay.h>
 #include <linux/device.h>
+#include <linux/gpio/consumer.h>
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>

@@ -31,7 +32,7 @@
 
 struct rk3328_codec_priv {
 	struct regmap *regmap;
-	struct regmap *grf;
+	struct gpio_desc *mute;
 	struct clk *mclk;
 	struct clk *pclk;
 	unsigned int sclk;

@@ -106,16 +107,6 @@ static int rk3328_set_dai_fmt(struct snd_soc_dai *dai, unsigned int fmt)
 	return 0;
 }
 
-static void rk3328_analog_output(struct rk3328_codec_priv *rk3328, int mute)
-{
-	unsigned int val = BIT(17);
-
-	if (mute)
-		val |= BIT(1);
-
-	regmap_write(rk3328->grf, RK3328_GRF_SOC_CON10, val);
-}
-
 static int rk3328_digital_mute(struct snd_soc_dai *dai, int mute)
 {
 	struct rk3328_codec_priv *rk3328 =

@@ -205,7 +196,7 @@ static int rk3328_codec_open_playback(struct rk3328_codec_priv *rk3328)
 	}
 
 	msleep(rk3328->spk_depop_time);
-	rk3328_analog_output(rk3328, 1);
+	gpiod_set_value(rk3328->mute, 0);
 
 	regmap_update_bits(rk3328->regmap, HPOUTL_GAIN_CTRL,
 			   HPOUTL_GAIN_MASK, OUT_VOLUME);

@@ -246,7 +237,7 @@ static int rk3328_codec_close_playback(struct rk3328_codec_priv *rk3328)
 {
 	size_t i;
 
-	rk3328_analog_output(rk3328, 0);
+	gpiod_set_value(rk3328->mute, 1);
 
 	regmap_update_bits(rk3328->regmap, HPOUTL_GAIN_CTRL,
 			   HPOUTL_GAIN_MASK, 0);

@@ -446,7 +437,6 @@ static int rk3328_platform_probe(struct platform_device *pdev)
 		dev_err(&pdev->dev, "missing 'rockchip,grf'\n");
 		return PTR_ERR(grf);
 	}
-	rk3328->grf = grf;
 	/* enable i2s_acodec_en */
 	regmap_write(grf, RK3328_GRF_SOC_CON2,
 		     (BIT(14) << 16 | BIT(14)));

@@ -458,7 +448,18 @@ static int rk3328_platform_probe(struct platform_device *pdev)
 		rk3328->spk_depop_time = 200;
 	}
 
-	rk3328_analog_output(rk3328, 0);
+	rk3328->mute = gpiod_get_optional(&pdev->dev, "mute", GPIOD_OUT_HIGH);
+	if (IS_ERR(rk3328->mute))
+		return PTR_ERR(rk3328->mute);
+	/*
+	 * Rock64 is the only supported platform to have widely relied on
+	 * this; if we do happen to come across an old DTB, just leave the
+	 * external mute forced off.
+	 */
+	if (!rk3328->mute && of_machine_is_compatible("pine64,rock64")) {
+		dev_warn(&pdev->dev, "assuming implicit control of GPIO_MUTE; update devicetree if possible\n");
+		regmap_write(grf, RK3328_GRF_SOC_CON10, BIT(17) | BIT(1));
+	}
 
 	rk3328->mclk = devm_clk_get(&pdev->dev, "mclk");
 	if (IS_ERR(rk3328->mclk))

Signature · 1 line

2.20.1

Robin Murphy

author

2020-02-18 21:32 UTC #6 lore raw

PATCH v2 3/3 arm64: dts: rockchip: Describe RK3328 GPIO_MUTE users

In reply to Robin Murphy

Add explicit properties to describe existing boards' GPIO_MUTE usage for the analog codec.

Signed-off-by Robin Murphy <[email protected]>

---

v2: no change

 arch/arm64/boot/dts/rockchip/rk3328-a1.dts     | 1 +
 arch/arm64/boot/dts/rockchip/rk3328-rock64.dts | 1 +
 2 files changed, 2 insertions(+)

arch/arm64/boot/dts/rockchip/rk3328-a1.dts +1 -0

index 16f1656d5203..797e90a3ac92 100644
@@ -60,6 +60,7 @@
 };
 
 &codec {
+	mute-gpios = <&grf_gpio 0 GPIO_ACTIVE_LOW>;
 	status = "okay";
 };

arch/arm64/boot/dts/rockchip/rk3328-rock64.dts +1 -0

index 62936b432f9a..bf3e546f5266 100644
@@ -104,6 +104,7 @@
 };
 
 &codec {
+	mute-gpios = <&grf_gpio 0 GPIO_ACTIVE_LOW>;
 	status = "okay";
 
 	port@0 {

Signature · 1 line

2.17.1

Robin Murphy

author

2020-02-28 12:48 UTC #7 lore raw

Re: [PATCH v2 3/3] arm64: dts: rockchip: Describe RK3328 GPIO_MUTE users

Hi Heiko,

Quoted 2 lines · Robin Murphy

Add explicit properties to describe existing boards' GPIO_MUTE usage for the analog codec.

Are you happy to pick this up now that the driver patches are queued?

Thanks,
Robin.

Quoted 34 lines

Signed-off-by Robin Murphy <[email protected]>

---

v2: no change

  arch/arm64/boot/dts/rockchip/rk3328-a1.dts     | 1 +
  arch/arm64/boot/dts/rockchip/rk3328-rock64.dts | 1 +
  2 files changed, 2 insertions(+)

arch/arm64/boot/dts/rockchip/rk3328-a1.dts +1 -0

index 16f1656d5203..797e90a3ac92 100644
@@ -60,6 +60,7 @@
  };
 
  &codec {
+	mute-gpios = <&grf_gpio 0 GPIO_ACTIVE_LOW>;
  	status = "okay";
  };

arch/arm64/boot/dts/rockchip/rk3328-rock64.dts +1 -0

index 62936b432f9a..bf3e546f5266 100644
@@ -104,6 +104,7 @@
  };
 
  &codec {
+	mute-gpios = <&grf_gpio 0 GPIO_ACTIVE_LOW>;
  	status = "okay";
 
  	port@0 {

Heiko Stuebner

maintainer

2020-02-29 23:42 UTC #8 lore raw

Re: [PATCH v2 3/3] arm64: dts: rockchip: Describe RK3328 GPIO_MUTE users

Am Freitag, 28. Februar 2020, 13:48:50 CET schrieb Robin Murphy:

Quoted 7 lines

Hi Heiko,
Quoted 2 lines · Robin Murphy
Add explicit properties to describe existing boards' GPIO_MUTE usage for the analog codec.
Are you happy to pick this up now that the driver patches are queued?

very happy :-)
And applied for 5.7

Thanks
Heiko