LinuxLists.cc - [PATCH v2 0/3] ASoC: rockchip: i2s: switch BCLK to GPIO

2022-06-16 16:38:58

Subject: [PATCH v2 0/3] ASoC: rockchip: i2s: switch BCLK to GPIO

The patches series is to fix the unexpected large
DC output voltage of Max98357a to burn the speakers
on rockchip platform when BCLK and SD_MODE are ON but LRCLK is OFF.

Changes Since V2:
-- Add documents of i2s pinctrl-names.
-- Fix dtsi syntax error.
-- Include the dtsi change and the driver change in the same series.
-- Ensure that driver gets both bclk_on and bclk_off states before using them.

Judy Hsiao (3):
ASoC: rockchip: i2s: switch BCLK to GPIO
arm64: dts: rk3399: i2s: switch BCLK to GPIO
ASoC: dt-bindings: rockchip: Document pinctrl-names for i2s

.../bindings/sound/rockchip-i2s.yaml | 7 +
.../boot/dts/rockchip/rk3399-gru-scarlet.dtsi | 10 ++
arch/arm64/boot/dts/rockchip/rk3399.dtsi | 25 +++-
sound/soc/rockchip/rockchip_i2s.c | 134 ++++++++++++++----
4 files changed, 149 insertions(+), 27 deletions(-)

--
2.36.1.476.g0c4daa206d-goog

2022-06-17 04:44:45

by Judy Hsiao

[permalink] [raw]

Subject: [PATCH v3 0/3] ASoC: rockchip: i2s: switch BCLK to GPIO

The patches series is to fix the unexpected large
DC output voltage of Max98357a to burn the speakers
on rockchip platform when BCLK and SD_MODE are ON but LRCLK is OFF.

Changes Since V3:
-- Fix indentation in the documentation.
-- Put pinctrl-1 right after pinctrl-0 in dtsi.
-- Fix indentation in the driver.
-- Remove unnecessary dev_dbg() in the driver.
Changes Since V2:
-- Add documents of i2s pinctrl-names.
-- Fix dtsi syntax error.
-- Include the dtsi change and the driver change in the same series.
-- Ensure that driver gets both bclk_on and bclk_off states before using them.

Judy Hsiao (3):
ASoC: rockchip: i2s: switch BCLK to GPIO
arm64: dts: rk3399: i2s: switch BCLK to GPIO
ASoC: dt-bindings: rockchip: Document pinctrl-names for i2s

.../bindings/sound/rockchip-i2s.yaml | 7 +
.../boot/dts/rockchip/rk3399-gru-scarlet.dtsi | 10 ++
arch/arm64/boot/dts/rockchip/rk3399.dtsi | 25 ++-
sound/soc/rockchip/rockchip_i2s.c | 167 +++++++++++++-----
4 files changed, 162 insertions(+), 47 deletions(-)

--
2.36.1.476.g0c4daa206d-goog

2022-06-17 04:45:02

by Judy Hsiao

[permalink] [raw]

Subject: [PATCH v3 3/3] ASoC: dt-bindings: rockchip: Document pinctrl-names for i2s

This patch documents pinctrl-names for i2s.

Signed-off-by: Judy Hsiao <[email protected]>
---
Documentation/devicetree/bindings/sound/rockchip-i2s.yaml | 7 +++++++
1 file changed, 7 insertions(+)

diff --git a/Documentation/devicetree/bindings/sound/rockchip-i2s.yaml b/Documentation/devicetree/bindings/sound/rockchip-i2s.yaml
index 5ea16b8ef93f..7e36e389e976 100644
--- a/Documentation/devicetree/bindings/sound/rockchip-i2s.yaml
+++ b/Documentation/devicetree/bindings/sound/rockchip-i2s.yaml
@@ -61,6 +61,13 @@ properties:
- const: tx
- const: rx

+ pinctrl-names:
+ oneOf:
+ - const: default
+ - items:
+ - const: bclk_on
+ - const: bclk_off
+
power-domains:
maxItems: 1

--
2.36.1.476.g0c4daa206d-goog

2022-06-17 05:10:31

by Judy Hsiao

[permalink] [raw]

Subject: [PATCH v3 1/3] ASoC: rockchip: i2s: switch BCLK to GPIO

We discovered that the state of BCLK on, LRCLK off and SD_MODE on
may cause the speaker melting issue. Removing LRCLK while BCLK
is present can cause unexpected output behavior including a large
DC output voltage as described in the Max98357a datasheet.

In order to:
1. prevent BCLK from turning on by other component.
2. keep BCLK and LRCLK being present at the same time

This patch switches BCLK to GPIO func before LRCLK output, and
configures BCLK func back during LRCLK is output.

Without this fix, BCLK is turned on 11 ms earlier than LRCK by the
da7219.
With this fix, BCLK is turned on only 0.4 ms earlier than LRCK by
the rockchip codec.

Signed-off-by: Judy Hsiao <[email protected]>
---
sound/soc/rockchip/rockchip_i2s.c | 167 ++++++++++++++++++++++--------
1 file changed, 121 insertions(+), 46 deletions(-)

diff --git a/sound/soc/rockchip/rockchip_i2s.c b/sound/soc/rockchip/rockchip_i2s.c
index 47a3971a9ce1..5e3cb06377a1 100644
--- a/sound/soc/rockchip/rockchip_i2s.c
+++ b/sound/soc/rockchip/rockchip_i2s.c
@@ -54,8 +54,38 @@ struct rk_i2s_dev {
const struct rk_i2s_pins *pins;
unsigned int bclk_ratio;
spinlock_t lock; /* tx/rx lock */
+ struct pinctrl *pinctrl;
+ struct pinctrl_state *bclk_on;
+ struct pinctrl_state *bclk_off;
};

+static int i2s_pinctrl_select_bclk_on(struct rk_i2s_dev *i2s)
+{
+ int ret = 0;
+
+ if (!IS_ERR(i2s->pinctrl) && !IS_ERR_OR_NULL(i2s->bclk_on))
+ ret = pinctrl_select_state(i2s->pinctrl, i2s->bclk_on);
+
+ if (ret)
+ dev_err(i2s->dev, "bclk enable failed %d\n", ret);
+
+ return ret;
+}
+
+static int i2s_pinctrl_select_bclk_off(struct rk_i2s_dev *i2s)
+{
+
+ int ret = 0;
+
+ if (!IS_ERR(i2s->pinctrl) && !IS_ERR_OR_NULL(i2s->bclk_off))
+ ret = pinctrl_select_state(i2s->pinctrl, i2s->bclk_off);
+
+ if (ret)
+ dev_err(i2s->dev, "bclk disable failed %d\n", ret);
+
+ return ret;
+}
+
static int i2s_runtime_suspend(struct device *dev)
{
struct rk_i2s_dev *i2s = dev_get_drvdata(dev);
@@ -92,39 +122,46 @@ static inline struct rk_i2s_dev *to_info(struct snd_soc_dai *dai)
return snd_soc_dai_get_drvdata(dai);
}

-static void rockchip_snd_txctrl(struct rk_i2s_dev *i2s, int on)
+static int rockchip_snd_txctrl(struct rk_i2s_dev *i2s, int on)
{
unsigned int val = 0;
int retry = 10;
-
+ int ret = 0;
+
spin_lock(&i2s->lock);
if (on) {
- regmap_update_bits(i2s->regmap, I2S_DMACR,
- I2S_DMACR_TDE_ENABLE, I2S_DMACR_TDE_ENABLE);
-
- regmap_update_bits(i2s->regmap, I2S_XFER,
- I2S_XFER_TXS_START | I2S_XFER_RXS_START,
- I2S_XFER_TXS_START | I2S_XFER_RXS_START);
-
+ ret = regmap_update_bits(i2s->regmap, I2S_DMACR,
+ I2S_DMACR_TDE_ENABLE, I2S_DMACR_TDE_ENABLE);
+ if (ret < 0)
+ goto end;
+ ret = regmap_update_bits(i2s->regmap, I2S_XFER,
+ I2S_XFER_TXS_START | I2S_XFER_RXS_START,
+ I2S_XFER_TXS_START | I2S_XFER_RXS_START);
+ if (ret < 0)
+ goto end;
i2s->tx_start = true;
} else {
i2s->tx_start = false;

- regmap_update_bits(i2s->regmap, I2S_DMACR,
- I2S_DMACR_TDE_ENABLE, I2S_DMACR_TDE_DISABLE);
+ ret = regmap_update_bits(i2s->regmap, I2S_DMACR,
+ I2S_DMACR_TDE_ENABLE, I2S_DMACR_TDE_DISABLE);
+ if (ret < 0)
+ goto end;

if (!i2s->rx_start) {
- regmap_update_bits(i2s->regmap, I2S_XFER,
- I2S_XFER_TXS_START |
- I2S_XFER_RXS_START,
- I2S_XFER_TXS_STOP |
- I2S_XFER_RXS_STOP);
-
+ ret = regmap_update_bits(i2s->regmap, I2S_XFER,
+ I2S_XFER_TXS_START |
+ I2S_XFER_RXS_START,
+ I2S_XFER_TXS_STOP |
+ I2S_XFER_RXS_STOP);
+ if (ret < 0)
+ goto end;
udelay(150);
- regmap_update_bits(i2s->regmap, I2S_CLR,
- I2S_CLR_TXC | I2S_CLR_RXC,
- I2S_CLR_TXC | I2S_CLR_RXC);
-
+ ret = regmap_update_bits(i2s->regmap, I2S_CLR,
+ I2S_CLR_TXC | I2S_CLR_RXC,
+ I2S_CLR_TXC | I2S_CLR_RXC);
+ if (ret < 0)
+ goto end;
regmap_read(i2s->regmap, I2S_CLR, &val);

/* Should wait for clear operation to finish */
@@ -138,42 +175,53 @@ static void rockchip_snd_txctrl(struct rk_i2s_dev *i2s, int on)
}
}
}
+end:
spin_unlock(&i2s->lock);
+ if (ret < 0)
+ dev_err(i2s->dev, "lrclk update failed\n");
+
+ return ret;
}

static void rockchip_snd_rxctrl(struct rk_i2s_dev *i2s, int on)
{
unsigned int val = 0;
int retry = 10;
+ int ret = 0;

spin_lock(&i2s->lock);
if (on) {
- regmap_update_bits(i2s->regmap, I2S_DMACR,
- I2S_DMACR_RDE_ENABLE, I2S_DMACR_RDE_ENABLE);
-
- regmap_update_bits(i2s->regmap, I2S_XFER,
- I2S_XFER_TXS_START | I2S_XFER_RXS_START,
- I2S_XFER_TXS_START | I2S_XFER_RXS_START);
-
+ ret = regmap_update_bits(i2s->regmap, I2S_DMACR,
+ I2S_DMACR_RDE_ENABLE, I2S_DMACR_RDE_ENABLE);
+ if (ret < 0)
+ goto end;
+
+ ret = regmap_update_bits(i2s->regmap, I2S_XFER,
+ I2S_XFER_TXS_START | I2S_XFER_RXS_START,
+ I2S_XFER_TXS_START | I2S_XFER_RXS_START);
+ if (ret < 0)
+ goto end;
i2s->rx_start = true;
} else {
i2s->rx_start = false;

- regmap_update_bits(i2s->regmap, I2S_DMACR,
- I2S_DMACR_RDE_ENABLE, I2S_DMACR_RDE_DISABLE);
+ ret = regmap_update_bits(i2s->regmap, I2S_DMACR,
+ I2S_DMACR_RDE_ENABLE, I2S_DMACR_RDE_DISABLE);
+ if (ret < 0)
+ goto end;

if (!i2s->tx_start) {
- regmap_update_bits(i2s->regmap, I2S_XFER,
- I2S_XFER_TXS_START |
- I2S_XFER_RXS_START,
- I2S_XFER_TXS_STOP |
- I2S_XFER_RXS_STOP);
-
+ ret = regmap_update_bits(i2s->regmap, I2S_XFER,
+ I2S_XFER_TXS_START | I2S_XFER_RXS_START,
+ I2S_XFER_TXS_STOP | I2S_XFER_RXS_STOP);
+ if (ret < 0)
+ goto end;
udelay(150);
- regmap_update_bits(i2s->regmap, I2S_CLR,
- I2S_CLR_TXC | I2S_CLR_RXC,
- I2S_CLR_TXC | I2S_CLR_RXC);
-
+ ret = regmap_update_bits(i2s->regmap, I2S_CLR,
+ I2S_CLR_TXC | I2S_CLR_RXC,
+ I2S_CLR_TXC | I2S_CLR_RXC);
+ if (ret < 0)
+ goto end;
regmap_read(i2s->regmap, I2S_CLR, &val);

/* Should wait for clear operation to finish */
@@ -187,7 +235,12 @@ static void rockchip_snd_rxctrl(struct rk_i2s_dev *i2s, int on)
}
}
}
+end:
spin_unlock(&i2s->lock);
+ if (ret < 0)
+ dev_err(i2s->dev, "lrclk update failed\n");
+
+ return ret;
}

static int rockchip_i2s_set_fmt(struct snd_soc_dai *cpu_dai,
@@ -425,17 +478,25 @@ static int rockchip_i2s_trigger(struct snd_pcm_substream *substream,
case SNDRV_PCM_TRIGGER_RESUME:
case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
if (substream->stream == SNDRV_PCM_STREAM_CAPTURE)
- rockchip_snd_rxctrl(i2s, 1);
+ ret = rockchip_snd_rxctrl(i2s, 1);
else
- rockchip_snd_txctrl(i2s, 1);
+ ret = rockchip_snd_txctrl(i2s, 1);
+ if (ret < 0)
+ return ret;
+ i2s_pinctrl_select_bclk_on(i2s);
break;
case SNDRV_PCM_TRIGGER_SUSPEND:
case SNDRV_PCM_TRIGGER_STOP:
case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
- if (substream->stream == SNDRV_PCM_STREAM_CAPTURE)
- rockchip_snd_rxctrl(i2s, 0);
- else
- rockchip_snd_txctrl(i2s, 0);
+ if (substream->stream == SNDRV_PCM_STREAM_CAPTURE) {
+ if (!i2s->tx_start)
+ i2s_pinctrl_select_bclk_off(i2s);
+ ret = rockchip_snd_rxctrl(i2s, 0);
+ } else {
+ if (!i2s->rx_start)
+ i2s_pinctrl_select_bclk_off(i2s);
+ ret = rockchip_snd_txctrl(i2s, 0);
+ }
break;
default:
ret = -EINVAL;
@@ -736,6 +797,20 @@ static int rockchip_i2s_probe(struct platform_device *pdev)
}

i2s->bclk_ratio = 64;
+ i2s->pinctrl = devm_pinctrl_get(&pdev->dev);
+ if (IS_ERR(i2s->pinctrl))
+ dev_err(&pdev->dev, "failed to find i2s pinctrl\n");
+
+ i2s->bclk_on = pinctrl_lookup_state(i2s->pinctrl, "bclk_on");
+ if (!IS_ERR_OR_NULL(i2s->bclk_on)) {
+ i2s->bclk_off = pinctrl_lookup_state(i2s->pinctrl, "bclk_off");
+ if (IS_ERR_OR_NULL(i2s->bclk_off)) {
+ dev_err(&pdev->dev, "failed to find i2s bclk_off\n");
+ goto err_clk;
+ }
+ }
+
+ i2s_pinctrl_select_bclk_off(i2s);

dev_set_drvdata(&pdev->dev, i2s);

--
2.36.1.476.g0c4daa206d-goog

2022-06-17 05:36:01

by Judy Hsiao

[permalink] [raw]

Subject: [PATCH v3 2/3] arm64: dts: rk3399: i2s: switch BCLK to GPIO

2022-06-17 09:14:01

by Mark Brown

[permalink] [raw]

Subject: Re: [PATCH v3 0/3] ASoC: rockchip: i2s: switch BCLK to GPIO

On Fri, Jun 17, 2022 at 04:42:48AM +0000, Judy Hsiao wrote:
> The patches series is to fix the unexpected large
> DC output voltage of Max98357a to burn the speakers
> on rockchip platform when BCLK and SD_MODE are ON but LRCLK is OFF.

Please don't send new patches in reply to old patches or serieses, this
makes it harder for both people and tools to understand what is going
on - it can bury things in mailboxes and make it difficult to keep track
of what current patches are, both for the new patches and the old ones.

Attachments:

(No filename) (534.00 B)
signature.asc (499.00 B)
Download all attachments

2022-06-17 21:54:02

by Brian Norris

[permalink] [raw]

Subject: Re: [PATCH v3 1/3] ASoC: rockchip: i2s: switch BCLK to GPIO

Hi,

On Fri, Jun 17, 2022 at 04:42:49AM +0000, Judy Hsiao wrote:
> We discovered that the state of BCLK on, LRCLK off and SD_MODE on
> may cause the speaker melting issue. Removing LRCLK while BCLK
> is present can cause unexpected output behavior including a large
> DC output voltage as described in the Max98357a datasheet.
>
> In order to:
> 1. prevent BCLK from turning on by other component.
> 2. keep BCLK and LRCLK being present at the same time
>
> This patch switches BCLK to GPIO func before LRCLK output, and
> configures BCLK func back during LRCLK is output.
>
> Without this fix, BCLK is turned on 11 ms earlier than LRCK by the
> da7219.
> With this fix, BCLK is turned on only 0.4 ms earlier than LRCK by
> the rockchip codec.
>
> Signed-off-by: Judy Hsiao <[email protected]>
> ---
> sound/soc/rockchip/rockchip_i2s.c | 167 ++++++++++++++++++++++--------
> 1 file changed, 121 insertions(+), 46 deletions(-)
>
> diff --git a/sound/soc/rockchip/rockchip_i2s.c b/sound/soc/rockchip/rockchip_i2s.c
> index 47a3971a9ce1..5e3cb06377a1 100644
> --- a/sound/soc/rockchip/rockchip_i2s.c
> +++ b/sound/soc/rockchip/rockchip_i2s.c

> @@ -92,39 +122,46 @@ static inline struct rk_i2s_dev *to_info(struct snd_soc_dai *dai)
> return snd_soc_dai_get_drvdata(dai);
> }
>
> -static void rockchip_snd_txctrl(struct rk_i2s_dev *i2s, int on)
> +static int rockchip_snd_txctrl(struct rk_i2s_dev *i2s, int on)
> {
> unsigned int val = 0;
> int retry = 10;
> -
> + int ret = 0;
> +
> spin_lock(&i2s->lock);
> if (on) {
> - regmap_update_bits(i2s->regmap, I2S_DMACR,
> - I2S_DMACR_TDE_ENABLE, I2S_DMACR_TDE_ENABLE);
> -
> - regmap_update_bits(i2s->regmap, I2S_XFER,
> - I2S_XFER_TXS_START | I2S_XFER_RXS_START,
> - I2S_XFER_TXS_START | I2S_XFER_RXS_START);
> -
> + ret = regmap_update_bits(i2s->regmap, I2S_DMACR,
> + I2S_DMACR_TDE_ENABLE, I2S_DMACR_TDE_ENABLE);
> + if (ret < 0)
> + goto end;
> + ret = regmap_update_bits(i2s->regmap, I2S_XFER,
> + I2S_XFER_TXS_START | I2S_XFER_RXS_START,
> + I2S_XFER_TXS_START | I2S_XFER_RXS_START);

This still isn't how I'd expect the indentation to look, but I think I
will stop complaining about whitespace. Maybe you can either use
'clang-format' (sparingly), or else see what Mark thinks.

> + if (ret < 0)
> + goto end;
> i2s->tx_start = true;
> } else {
> i2s->tx_start = false;
>
...

> static void rockchip_snd_rxctrl(struct rk_i2s_dev *i2s, int on)

I didn't think I needed to call this out, because you already got build
bots complaining about this, but...
...did you even compile-test this? You should be making this function
return 'int', not 'void', because you're adding return values below.

> {
> unsigned int val = 0;
> int retry = 10;
> + int ret = 0;
>
...
> +end:
> spin_unlock(&i2s->lock);
> + if (ret < 0)
> + dev_err(i2s->dev, "lrclk update failed\n");
> +
> + return ret;

^^ here.

> }
>
> static int rockchip_i2s_set_fmt(struct snd_soc_dai *cpu_dai,

Aside from those, I think this looks OK to the best of my
(not-very-familiar with sounds/soc/) ability:

Reviewed-by: Brian Norris <[email protected]>