[alsa-devel] [PATCH 0/3] Clean up RK3328 audio codec GPIO control
Hi all,
Investigating the RK3328 GPIO_MUTE pin in the context of boards that use it to control a regulator has highlighted that the audio codec driver currently has some hard-coded implicit control of that pin. Fortunately those boards don't currently enable the audio codec, because it would be pretty terrible if playing audio changed the SD card I/O voltage. This is a first crack at making things better.
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 | 20 +++++-------------- 4 files changed, 13 insertions(+), 16 deletions(-)
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 robin.murphy@arm.com --- .../devicetree/bindings/sound/rockchip,rk3328-codec.txt | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/sound/rockchip,rk3328-codec.txt b/Documentation/devicetree/bindings/sound/rockchip,rk3328-codec.txt index 2469588c7ccb..1ecd75d2032a 100644 --- a/Documentation/devicetree/bindings/sound/rockchip,rk3328-codec.txt +++ b/Documentation/devicetree/bindings/sound/rockchip,rk3328-codec.txt @@ -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"; };
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 robin.murphy@arm.com --- sound/soc/codecs/rk3328_codec.c | 20 +++++--------------- 1 file changed, 5 insertions(+), 15 deletions(-)
diff --git a/sound/soc/codecs/rk3328_codec.c b/sound/soc/codecs/rk3328_codec.c index 287c962ba00d..f0e9ef21c5f8 100644 --- a/sound/soc/codecs/rk3328_codec.c +++ b/sound/soc/codecs/rk3328_codec.c @@ -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,7 @@ 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);
rk3328->mclk = devm_clk_get(&pdev->dev, "mclk"); if (IS_ERR(rk3328->mclk))
On Thu, Feb 06, 2020 at 01:07:12AM +0000, Robin Murphy wrote:
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
This makes sense but it is an ABI break so is going to need quirking for existing boards that unfortunately rely on the existing behaviour.
On 2020-02-06 11:46 am, Mark Brown wrote:
On Thu, Feb 06, 2020 at 01:07:12AM +0000, Robin Murphy wrote:
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
This makes sense but it is an ABI break so is going to need quirking for existing boards that unfortunately rely on the existing behaviour.
Yeah, that's where it gets tricky - there doesn't seem to be a nice way to differentiate between "no GPIO because old DT" and "no GPIO because the enable is hard-wired/irrelevant and GPIO_MUTE doesn't do what you think it does", and it seems really improper to introduce a DT property for the sole purpose of telling a Linux driver not to assume something it shouldn't really have in the first place.
My opinion fell on the side of a minor ABI break being the lesser of two evils, given that the worst case once people start enabling this codec on Renegade/ROC-CC boards (which I was only anticipating, but have just discovered is happening already[1]) results in unexpectedly stuffing 3.3V into the SD card and SoC I/O domain while both are in 1.8V mode, and that the change would only really affect one other current board (Rock64), where most mainline users are likely to be upgrading their DTB in lock-step with the kernel anyway.
I guess the existing (mis)behaviour could be predicated on an of_machine_is_compatible() check for Rock64 boards - it's ugly, but should do the job if you feel it's more important to be 100% strict about not regressing supported systems for any possible kernel/DTB combination.
Thanks, Robin.
[1] https://github.com/armbian/build/commit/18b24717be9639b65b86db3dbcf2b42fe73c...
On Thu, Feb 06, 2020 at 12:36:04PM +0000, Robin Murphy wrote:
On 2020-02-06 11:46 am, Mark Brown wrote:
This makes sense but it is an ABI break so is going to need quirking for existing boards that unfortunately rely on the existing behaviour.
I guess the existing (mis)behaviour could be predicated on an of_machine_is_compatible() check for Rock64 boards - it's ugly, but should do the job if you feel it's more important to be 100% strict about not regressing supported systems for any possible kernel/DTB combination.
Yes, that's what I'm suggesting - we don't need to be exhaustive but having an obvious place for people to put the quirk in if they are affected is much better practice than just silently letting things break.
On Thu, Feb 6, 2020 at 8:57 AM Mark Brown broonie@kernel.org wrote:
On Thu, Feb 06, 2020 at 12:36:04PM +0000, Robin Murphy wrote:
On 2020-02-06 11:46 am, Mark Brown wrote:
This makes sense but it is an ABI break so is going to need quirking for existing boards that unfortunately rely on the existing behaviour.
I guess the existing (mis)behaviour could be predicated on an of_machine_is_compatible() check for Rock64 boards - it's ugly, but should do the job if you feel it's more important to be 100% strict about not regressing supported systems for any possible kernel/DTB combination.
Yes, that's what I'm suggesting - we don't need to be exhaustive but having an obvious place for people to put the quirk in if they are affected is much better practice than just silently letting things break.
Might want to put a warning in there too, so that if someone is paying attention they will see that they are using an out of date device tree.
On 2020-02-06 6:05 pm, Peter Geis wrote:
On Thu, Feb 6, 2020 at 8:57 AM Mark Brown broonie@kernel.org wrote:
On Thu, Feb 06, 2020 at 12:36:04PM +0000, Robin Murphy wrote:
On 2020-02-06 11:46 am, Mark Brown wrote:
This makes sense but it is an ABI break so is going to need quirking for existing boards that unfortunately rely on the existing behaviour.
I guess the existing (mis)behaviour could be predicated on an of_machine_is_compatible() check for Rock64 boards - it's ugly, but should do the job if you feel it's more important to be 100% strict about not regressing supported systems for any possible kernel/DTB combination.
Yes, that's what I'm suggesting - we don't need to be exhaustive but having an obvious place for people to put the quirk in if they are affected is much better practice than just silently letting things break.
Might want to put a warning in there too, so that if someone is paying attention they will see that they are using an out of date device tree.
Of course, that much is a given :)
Having thought it over some more, I reckon there's a reasonable compromise that isn't too hideous, preserves the logical cleanup but at least prevents audio going silent with old DTBs (plus handling GPIO probe deferral properly which I forgot about first time around). How does this look?
Robin.
----->8-----
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 robin.murphy@arm.com --- sound/soc/codecs/rk3328_codec.c | 31 ++++++++++++++++--------------- 1 file changed, 16 insertions(+), 15 deletions(-)
diff --git a/sound/soc/codecs/rk3328_codec.c b/sound/soc/codecs/rk3328_codec.c index 287c962ba00d..115706a55577 100644 --- a/sound/soc/codecs/rk3328_codec.c +++ b/sound/soc/codecs/rk3328_codec.c @@ -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))
On Thu, Feb 06, 2020 at 09:38:11PM +0000, Robin Murphy wrote:
On 2020-02-06 6:05 pm, Peter Geis wrote:
On Thu, Feb 6, 2020 at 8:57 AM Mark Brown broonie@kernel.org wrote:
compromise that isn't too hideous, preserves the logical cleanup but at least prevents audio going silent with old DTBs (plus handling GPIO probe deferral properly which I forgot about first time around). How does this look?
Robin.
----->8-----
Please don't send revisions of individual patches in reply to discussion of a series, it makes it really hard to follow what's going on with the series. Please just send patches normally.
Add explicit properties to describe existing boards' GPIO_MUTE usage for the analog codec.
Signed-off-by: Robin Murphy robin.murphy@arm.com --- arch/arm64/boot/dts/rockchip/rk3328-a1.dts | 1 + arch/arm64/boot/dts/rockchip/rk3328-rock64.dts | 1 + 2 files changed, 2 insertions(+)
diff --git a/arch/arm64/boot/dts/rockchip/rk3328-a1.dts b/arch/arm64/boot/dts/rockchip/rk3328-a1.dts index 5822cd0ee7db..6cd4543720fd 100644 --- a/arch/arm64/boot/dts/rockchip/rk3328-a1.dts +++ b/arch/arm64/boot/dts/rockchip/rk3328-a1.dts @@ -60,6 +60,7 @@ };
&codec { + mute-gpios = <&grf_gpio 0 GPIO_ACTIVE_LOW>; status = "okay"; };
diff --git a/arch/arm64/boot/dts/rockchip/rk3328-rock64.dts b/arch/arm64/boot/dts/rockchip/rk3328-rock64.dts index 62936b432f9a..bf3e546f5266 100644 --- a/arch/arm64/boot/dts/rockchip/rk3328-rock64.dts +++ b/arch/arm64/boot/dts/rockchip/rk3328-rock64.dts @@ -104,6 +104,7 @@ };
&codec { + mute-gpios = <&grf_gpio 0 GPIO_ACTIVE_LOW>; status = "okay";
port@0 {
participants (3)
-
Mark Brown -
Peter Geis -
Robin Murphy