* [PATCH 1/2] ASoC: rockchip: i2s: separate capture and playback
@ 2015-12-09 10:32 John Keeping
2015-12-09 10:32 ` [PATCH 2/2] ASoC: rockchip: i2s: remove unused variables John Keeping
2016-04-29 14:59 ` [PATCH 1/2] ASoC: rockchip: i2s: separate capture and playback Enric Balletbo Serra
0 siblings, 2 replies; 10+ messages in thread
From: John Keeping @ 2015-12-09 10:32 UTC (permalink / raw)
To: linux-arm-kernel
If we only clear the tx/rx state when both are disabled it is not
possible to start/stop one multiple times while the other is running.
Since the two are independently controlled, treat them as such and
remove the false dependency between capture and playback.
Signed-off-by: John Keeping <john@metanate.com>
---
sound/soc/rockchip/rockchip_i2s.c | 72 +++++++++++++++++----------------------
1 file changed, 32 insertions(+), 40 deletions(-)
diff --git a/sound/soc/rockchip/rockchip_i2s.c b/sound/soc/rockchip/rockchip_i2s.c
index 83b1b9c..acc6225 100644
--- a/sound/soc/rockchip/rockchip_i2s.c
+++ b/sound/soc/rockchip/rockchip_i2s.c
@@ -82,8 +82,8 @@ static void rockchip_snd_txctrl(struct rk_i2s_dev *i2s, int on)
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);
+ I2S_XFER_TXS_START,
+ I2S_XFER_TXS_START);
i2s->tx_start = true;
} else {
@@ -92,27 +92,23 @@ static void rockchip_snd_txctrl(struct rk_i2s_dev *i2s, int on)
regmap_update_bits(i2s->regmap, I2S_DMACR,
I2S_DMACR_TDE_ENABLE, I2S_DMACR_TDE_DISABLE);
- 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);
+ regmap_update_bits(i2s->regmap, I2S_XFER,
+ I2S_XFER_TXS_START,
+ I2S_XFER_TXS_STOP);
- regmap_update_bits(i2s->regmap, I2S_CLR,
- I2S_CLR_TXC | I2S_CLR_RXC,
- I2S_CLR_TXC | I2S_CLR_RXC);
+ regmap_update_bits(i2s->regmap, I2S_CLR,
+ I2S_CLR_TXC,
+ I2S_CLR_TXC);
- regmap_read(i2s->regmap, I2S_CLR, &val);
+ regmap_read(i2s->regmap, I2S_CLR, &val);
- /* Should wait for clear operation to finish */
- while (val) {
- regmap_read(i2s->regmap, I2S_CLR, &val);
- retry--;
- if (!retry) {
- dev_warn(i2s->dev, "fail to clear\n");
- break;
- }
+ /* Should wait for clear operation to finish */
+ while (val & I2S_CLR_TXC) {
+ regmap_read(i2s->regmap, I2S_CLR, &val);
+ retry--;
+ if (!retry) {
+ dev_warn(i2s->dev, "fail to clear\n");
+ break;
}
}
}
@@ -128,8 +124,8 @@ static void rockchip_snd_rxctrl(struct rk_i2s_dev *i2s, int on)
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);
+ I2S_XFER_RXS_START,
+ I2S_XFER_RXS_START);
i2s->rx_start = true;
} else {
@@ -138,27 +134,23 @@ static void rockchip_snd_rxctrl(struct rk_i2s_dev *i2s, int on)
regmap_update_bits(i2s->regmap, I2S_DMACR,
I2S_DMACR_RDE_ENABLE, I2S_DMACR_RDE_DISABLE);
- 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);
+ regmap_update_bits(i2s->regmap, I2S_XFER,
+ I2S_XFER_RXS_START,
+ I2S_XFER_RXS_STOP);
- regmap_update_bits(i2s->regmap, I2S_CLR,
- I2S_CLR_TXC | I2S_CLR_RXC,
- I2S_CLR_TXC | I2S_CLR_RXC);
+ regmap_update_bits(i2s->regmap, I2S_CLR,
+ I2S_CLR_RXC,
+ I2S_CLR_RXC);
- regmap_read(i2s->regmap, I2S_CLR, &val);
+ regmap_read(i2s->regmap, I2S_CLR, &val);
- /* Should wait for clear operation to finish */
- while (val) {
- regmap_read(i2s->regmap, I2S_CLR, &val);
- retry--;
- if (!retry) {
- dev_warn(i2s->dev, "fail to clear\n");
- break;
- }
+ /* Should wait for clear operation to finish */
+ while (val & I2S_CLR_RXC) {
+ regmap_read(i2s->regmap, I2S_CLR, &val);
+ retry--;
+ if (!retry) {
+ dev_warn(i2s->dev, "fail to clear\n");
+ break;
}
}
}
--
2.6.3.462.gbe2c914
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 2/2] ASoC: rockchip: i2s: remove unused variables
2015-12-09 10:32 [PATCH 1/2] ASoC: rockchip: i2s: separate capture and playback John Keeping
@ 2015-12-09 10:32 ` John Keeping
2016-04-29 14:59 ` [PATCH 1/2] ASoC: rockchip: i2s: separate capture and playback Enric Balletbo Serra
1 sibling, 0 replies; 10+ messages in thread
From: John Keeping @ 2015-12-09 10:32 UTC (permalink / raw)
To: linux-arm-kernel
The previous commit removed the only use of these variables.
Signed-off-by: John Keeping <john@metanate.com>
---
sound/soc/rockchip/rockchip_i2s.c | 15 ---------------
1 file changed, 15 deletions(-)
diff --git a/sound/soc/rockchip/rockchip_i2s.c b/sound/soc/rockchip/rockchip_i2s.c
index acc6225..8b0a588 100644
--- a/sound/soc/rockchip/rockchip_i2s.c
+++ b/sound/soc/rockchip/rockchip_i2s.c
@@ -34,13 +34,6 @@ struct rk_i2s_dev {
struct regmap *regmap;
-/*
- * Used to indicate the tx/rx status.
- * I2S controller hopes to start the tx and rx together,
- * also to stop them when they are both try to stop.
-*/
- bool tx_start;
- bool rx_start;
bool is_master_mode;
};
@@ -84,11 +77,7 @@ static void rockchip_snd_txctrl(struct rk_i2s_dev *i2s, int on)
regmap_update_bits(i2s->regmap, I2S_XFER,
I2S_XFER_TXS_START,
I2S_XFER_TXS_START);
-
- i2s->tx_start = true;
} else {
- i2s->tx_start = false;
-
regmap_update_bits(i2s->regmap, I2S_DMACR,
I2S_DMACR_TDE_ENABLE, I2S_DMACR_TDE_DISABLE);
@@ -126,11 +115,7 @@ static void rockchip_snd_rxctrl(struct rk_i2s_dev *i2s, int on)
regmap_update_bits(i2s->regmap, I2S_XFER,
I2S_XFER_RXS_START,
I2S_XFER_RXS_START);
-
- i2s->rx_start = true;
} else {
- i2s->rx_start = false;
-
regmap_update_bits(i2s->regmap, I2S_DMACR,
I2S_DMACR_RDE_ENABLE, I2S_DMACR_RDE_DISABLE);
--
2.6.3.462.gbe2c914
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 1/2] ASoC: rockchip: i2s: separate capture and playback
2015-12-09 10:32 [PATCH 1/2] ASoC: rockchip: i2s: separate capture and playback John Keeping
2015-12-09 10:32 ` [PATCH 2/2] ASoC: rockchip: i2s: remove unused variables John Keeping
@ 2016-04-29 14:59 ` Enric Balletbo Serra
2016-04-30 7:00 ` John Keeping
1 sibling, 1 reply; 10+ messages in thread
From: Enric Balletbo Serra @ 2016-04-29 14:59 UTC (permalink / raw)
To: linux-arm-kernel
Hi John,
2015-12-09 11:32 GMT+01:00 John Keeping <john@metanate.com>:
> If we only clear the tx/rx state when both are disabled it is not
> possible to start/stop one multiple times while the other is running.
> Since the two are independently controlled, treat them as such and
> remove the false dependency between capture and playback.
>
> Signed-off-by: John Keeping <john@metanate.com>
> ---
> sound/soc/rockchip/rockchip_i2s.c | 72 +++++++++++++++++----------------------
> 1 file changed, 32 insertions(+), 40 deletions(-)
>
> diff --git a/sound/soc/rockchip/rockchip_i2s.c b/sound/soc/rockchip/rockchip_i2s.c
> index 83b1b9c..acc6225 100644
> --- a/sound/soc/rockchip/rockchip_i2s.c
> +++ b/sound/soc/rockchip/rockchip_i2s.c
> @@ -82,8 +82,8 @@ static void rockchip_snd_txctrl(struct rk_i2s_dev *i2s, int on)
> 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);
> + I2S_XFER_TXS_START,
> + I2S_XFER_TXS_START);
>
> i2s->tx_start = true;
> } else {
> @@ -92,27 +92,23 @@ static void rockchip_snd_txctrl(struct rk_i2s_dev *i2s, int on)
> regmap_update_bits(i2s->regmap, I2S_DMACR,
> I2S_DMACR_TDE_ENABLE, I2S_DMACR_TDE_DISABLE);
>
> - 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);
> + regmap_update_bits(i2s->regmap, I2S_XFER,
> + I2S_XFER_TXS_START,
> + I2S_XFER_TXS_STOP);
>
> - regmap_update_bits(i2s->regmap, I2S_CLR,
> - I2S_CLR_TXC | I2S_CLR_RXC,
> - I2S_CLR_TXC | I2S_CLR_RXC);
> + regmap_update_bits(i2s->regmap, I2S_CLR,
> + I2S_CLR_TXC,
> + I2S_CLR_TXC);
>
> - regmap_read(i2s->regmap, I2S_CLR, &val);
> + regmap_read(i2s->regmap, I2S_CLR, &val);
>
> - /* Should wait for clear operation to finish */
> - while (val) {
> - regmap_read(i2s->regmap, I2S_CLR, &val);
> - retry--;
> - if (!retry) {
> - dev_warn(i2s->dev, "fail to clear\n");
> - break;
> - }
> + /* Should wait for clear operation to finish */
> + while (val & I2S_CLR_TXC) {
> + regmap_read(i2s->regmap, I2S_CLR, &val);
> + retry--;
> + if (!retry) {
> + dev_warn(i2s->dev, "fail to clear\n");
> + break;
> }
> }
> }
> @@ -128,8 +124,8 @@ static void rockchip_snd_rxctrl(struct rk_i2s_dev *i2s, int on)
> 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);
> + I2S_XFER_RXS_START,
> + I2S_XFER_RXS_START);
>
> i2s->rx_start = true;
> } else {
> @@ -138,27 +134,23 @@ static void rockchip_snd_rxctrl(struct rk_i2s_dev *i2s, int on)
> regmap_update_bits(i2s->regmap, I2S_DMACR,
> I2S_DMACR_RDE_ENABLE, I2S_DMACR_RDE_DISABLE);
>
> - 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);
> + regmap_update_bits(i2s->regmap, I2S_XFER,
> + I2S_XFER_RXS_START,
> + I2S_XFER_RXS_STOP);
>
> - regmap_update_bits(i2s->regmap, I2S_CLR,
> - I2S_CLR_TXC | I2S_CLR_RXC,
> - I2S_CLR_TXC | I2S_CLR_RXC);
> + regmap_update_bits(i2s->regmap, I2S_CLR,
> + I2S_CLR_RXC,
> + I2S_CLR_RXC);
>
> - regmap_read(i2s->regmap, I2S_CLR, &val);
> + regmap_read(i2s->regmap, I2S_CLR, &val);
>
> - /* Should wait for clear operation to finish */
> - while (val) {
> - regmap_read(i2s->regmap, I2S_CLR, &val);
> - retry--;
> - if (!retry) {
> - dev_warn(i2s->dev, "fail to clear\n");
> - break;
> - }
> + /* Should wait for clear operation to finish */
> + while (val & I2S_CLR_RXC) {
> + regmap_read(i2s->regmap, I2S_CLR, &val);
> + retry--;
> + if (!retry) {
> + dev_warn(i2s->dev, "fail to clear\n");
> + break;
> }
> }
> }
> --
> 2.6.3.462.gbe2c914
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
Using my Veyron Jerry Chromebook wiht latest kernel I found that the
speakers doesn't work. Bisect points to this patch as the offending
commit. However your changes looks reasonable the fact is that
reverting the patch makes the audio work again on my device. I need to
dig a bit more into the issue, but meanwhile, any idea on what is
happening ? Can I ask which device did you test?
PS: Sorry for the noise if you received the email twice.
Best regards,
Enric
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/2] ASoC: rockchip: i2s: separate capture and playback
2016-04-29 14:59 ` [PATCH 1/2] ASoC: rockchip: i2s: separate capture and playback Enric Balletbo Serra
@ 2016-04-30 7:00 ` John Keeping
2016-05-02 9:13 ` Enric Balletbo Serra
2016-05-03 4:12 ` sugar
0 siblings, 2 replies; 10+ messages in thread
From: John Keeping @ 2016-04-30 7:00 UTC (permalink / raw)
To: linux-arm-kernel
Hi Enric,
On Fri, Apr 29, 2016 at 04:59:27PM +0200, Enric Balletbo Serra wrote:
> 2015-12-09 11:32 GMT+01:00 John Keeping <john@metanate.com>:
> > If we only clear the tx/rx state when both are disabled it is not
> > possible to start/stop one multiple times while the other is running.
> > Since the two are independently controlled, treat them as such and
> > remove the false dependency between capture and playback.
> >
> > Signed-off-by: John Keeping <john@metanate.com>
> > ---
> > sound/soc/rockchip/rockchip_i2s.c | 72 +++++++++++++++++----------------------
> > 1 file changed, 32 insertions(+), 40 deletions(-)
> >
> > diff --git a/sound/soc/rockchip/rockchip_i2s.c b/sound/soc/rockchip/rockchip_i2s.c
> > index 83b1b9c..acc6225 100644
> > --- a/sound/soc/rockchip/rockchip_i2s.c
> > +++ b/sound/soc/rockchip/rockchip_i2s.c
> > @@ -82,8 +82,8 @@ static void rockchip_snd_txctrl(struct rk_i2s_dev *i2s, int on)
> > 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);
> > + I2S_XFER_TXS_START,
> > + I2S_XFER_TXS_START);
> >
> > i2s->tx_start = true;
> > } else {
> > @@ -92,27 +92,23 @@ static void rockchip_snd_txctrl(struct rk_i2s_dev *i2s, int on)
> > regmap_update_bits(i2s->regmap, I2S_DMACR,
> > I2S_DMACR_TDE_ENABLE, I2S_DMACR_TDE_DISABLE);
> >
> > - 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);
> > + regmap_update_bits(i2s->regmap, I2S_XFER,
> > + I2S_XFER_TXS_START,
> > + I2S_XFER_TXS_STOP);
> >
> > - regmap_update_bits(i2s->regmap, I2S_CLR,
> > - I2S_CLR_TXC | I2S_CLR_RXC,
> > - I2S_CLR_TXC | I2S_CLR_RXC);
> > + regmap_update_bits(i2s->regmap, I2S_CLR,
> > + I2S_CLR_TXC,
> > + I2S_CLR_TXC);
> >
> > - regmap_read(i2s->regmap, I2S_CLR, &val);
> > + regmap_read(i2s->regmap, I2S_CLR, &val);
> >
> > - /* Should wait for clear operation to finish */
> > - while (val) {
> > - regmap_read(i2s->regmap, I2S_CLR, &val);
> > - retry--;
> > - if (!retry) {
> > - dev_warn(i2s->dev, "fail to clear\n");
> > - break;
> > - }
> > + /* Should wait for clear operation to finish */
> > + while (val & I2S_CLR_TXC) {
> > + regmap_read(i2s->regmap, I2S_CLR, &val);
> > + retry--;
> > + if (!retry) {
> > + dev_warn(i2s->dev, "fail to clear\n");
> > + break;
> > }
> > }
> > }
> > @@ -128,8 +124,8 @@ static void rockchip_snd_rxctrl(struct rk_i2s_dev *i2s, int on)
> > 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);
> > + I2S_XFER_RXS_START,
> > + I2S_XFER_RXS_START);
> >
> > i2s->rx_start = true;
> > } else {
> > @@ -138,27 +134,23 @@ static void rockchip_snd_rxctrl(struct rk_i2s_dev *i2s, int on)
> > regmap_update_bits(i2s->regmap, I2S_DMACR,
> > I2S_DMACR_RDE_ENABLE, I2S_DMACR_RDE_DISABLE);
> >
> > - 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);
> > + regmap_update_bits(i2s->regmap, I2S_XFER,
> > + I2S_XFER_RXS_START,
> > + I2S_XFER_RXS_STOP);
> >
> > - regmap_update_bits(i2s->regmap, I2S_CLR,
> > - I2S_CLR_TXC | I2S_CLR_RXC,
> > - I2S_CLR_TXC | I2S_CLR_RXC);
> > + regmap_update_bits(i2s->regmap, I2S_CLR,
> > + I2S_CLR_RXC,
> > + I2S_CLR_RXC);
> >
> > - regmap_read(i2s->regmap, I2S_CLR, &val);
> > + regmap_read(i2s->regmap, I2S_CLR, &val);
> >
> > - /* Should wait for clear operation to finish */
> > - while (val) {
> > - regmap_read(i2s->regmap, I2S_CLR, &val);
> > - retry--;
> > - if (!retry) {
> > - dev_warn(i2s->dev, "fail to clear\n");
> > - break;
> > - }
> > + /* Should wait for clear operation to finish */
> > + while (val & I2S_CLR_RXC) {
> > + regmap_read(i2s->regmap, I2S_CLR, &val);
> > + retry--;
> > + if (!retry) {
> > + dev_warn(i2s->dev, "fail to clear\n");
> > + break;
> > }
> > }
> > }
> > --
> > 2.6.3.462.gbe2c914
>
> Using my Veyron Jerry Chromebook wiht latest kernel I found that the
> speakers doesn't work. Bisect points to this patch as the offending
> commit. However your changes looks reasonable the fact is that
> reverting the patch makes the audio work again on my device. I need to
> dig a bit more into the issue, but meanwhile, any idea on what is
> happening ? Can I ask which device did you test?
I tested this on a Radxa Rock2 square. This change fixed an issue I
found when running separate processes for playback and recording (I was
testing with arecord and speaker-test IIRC) but I don't think that
should be relevant. I just read through the patch again and I can't see
anything obviously wrong so I'm afraid I don't have any idea what's
causing the problem you're seeing.
John
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/2] ASoC: rockchip: i2s: separate capture and playback
2016-04-30 7:00 ` John Keeping
@ 2016-05-02 9:13 ` Enric Balletbo Serra
2016-05-03 4:12 ` sugar
1 sibling, 0 replies; 10+ messages in thread
From: Enric Balletbo Serra @ 2016-05-02 9:13 UTC (permalink / raw)
To: linux-arm-kernel
Hi John,
Thanks for answer.
2016-04-30 9:00 GMT+02:00 John Keeping <john@keeping.me.uk>:
> Hi Enric,
>
> On Fri, Apr 29, 2016 at 04:59:27PM +0200, Enric Balletbo Serra wrote:
>> 2015-12-09 11:32 GMT+01:00 John Keeping <john@metanate.com>:
>> > If we only clear the tx/rx state when both are disabled it is not
>> > possible to start/stop one multiple times while the other is running.
>> > Since the two are independently controlled, treat them as such and
>> > remove the false dependency between capture and playback.
>> >
>> > Signed-off-by: John Keeping <john@metanate.com>
>> > ---
>> > sound/soc/rockchip/rockchip_i2s.c | 72 +++++++++++++++++----------------------
>> > 1 file changed, 32 insertions(+), 40 deletions(-)
>> >
>> > diff --git a/sound/soc/rockchip/rockchip_i2s.c b/sound/soc/rockchip/rockchip_i2s.c
>> > index 83b1b9c..acc6225 100644
>> > --- a/sound/soc/rockchip/rockchip_i2s.c
>> > +++ b/sound/soc/rockchip/rockchip_i2s.c
>> > @@ -82,8 +82,8 @@ static void rockchip_snd_txctrl(struct rk_i2s_dev *i2s, int on)
>> > 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);
>> > + I2S_XFER_TXS_START,
>> > + I2S_XFER_TXS_START);
>> >
>> > i2s->tx_start = true;
>> > } else {
>> > @@ -92,27 +92,23 @@ static void rockchip_snd_txctrl(struct rk_i2s_dev *i2s, int on)
>> > regmap_update_bits(i2s->regmap, I2S_DMACR,
>> > I2S_DMACR_TDE_ENABLE, I2S_DMACR_TDE_DISABLE);
>> >
>> > - 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);
>> > + regmap_update_bits(i2s->regmap, I2S_XFER,
>> > + I2S_XFER_TXS_START,
>> > + I2S_XFER_TXS_STOP);
>> >
>> > - regmap_update_bits(i2s->regmap, I2S_CLR,
>> > - I2S_CLR_TXC | I2S_CLR_RXC,
>> > - I2S_CLR_TXC | I2S_CLR_RXC);
>> > + regmap_update_bits(i2s->regmap, I2S_CLR,
>> > + I2S_CLR_TXC,
>> > + I2S_CLR_TXC);
>> >
>> > - regmap_read(i2s->regmap, I2S_CLR, &val);
>> > + regmap_read(i2s->regmap, I2S_CLR, &val);
>> >
>> > - /* Should wait for clear operation to finish */
>> > - while (val) {
>> > - regmap_read(i2s->regmap, I2S_CLR, &val);
>> > - retry--;
>> > - if (!retry) {
>> > - dev_warn(i2s->dev, "fail to clear\n");
>> > - break;
>> > - }
>> > + /* Should wait for clear operation to finish */
>> > + while (val & I2S_CLR_TXC) {
>> > + regmap_read(i2s->regmap, I2S_CLR, &val);
>> > + retry--;
>> > + if (!retry) {
>> > + dev_warn(i2s->dev, "fail to clear\n");
>> > + break;
>> > }
>> > }
>> > }
>> > @@ -128,8 +124,8 @@ static void rockchip_snd_rxctrl(struct rk_i2s_dev *i2s, int on)
>> > 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);
>> > + I2S_XFER_RXS_START,
>> > + I2S_XFER_RXS_START);
>> >
>> > i2s->rx_start = true;
>> > } else {
>> > @@ -138,27 +134,23 @@ static void rockchip_snd_rxctrl(struct rk_i2s_dev *i2s, int on)
>> > regmap_update_bits(i2s->regmap, I2S_DMACR,
>> > I2S_DMACR_RDE_ENABLE, I2S_DMACR_RDE_DISABLE);
>> >
>> > - 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);
>> > + regmap_update_bits(i2s->regmap, I2S_XFER,
>> > + I2S_XFER_RXS_START,
>> > + I2S_XFER_RXS_STOP);
>> >
>> > - regmap_update_bits(i2s->regmap, I2S_CLR,
>> > - I2S_CLR_TXC | I2S_CLR_RXC,
>> > - I2S_CLR_TXC | I2S_CLR_RXC);
>> > + regmap_update_bits(i2s->regmap, I2S_CLR,
>> > + I2S_CLR_RXC,
>> > + I2S_CLR_RXC);
>> >
>> > - regmap_read(i2s->regmap, I2S_CLR, &val);
>> > + regmap_read(i2s->regmap, I2S_CLR, &val);
>> >
>> > - /* Should wait for clear operation to finish */
>> > - while (val) {
>> > - regmap_read(i2s->regmap, I2S_CLR, &val);
>> > - retry--;
>> > - if (!retry) {
>> > - dev_warn(i2s->dev, "fail to clear\n");
>> > - break;
>> > - }
>> > + /* Should wait for clear operation to finish */
>> > + while (val & I2S_CLR_RXC) {
>> > + regmap_read(i2s->regmap, I2S_CLR, &val);
>> > + retry--;
>> > + if (!retry) {
>> > + dev_warn(i2s->dev, "fail to clear\n");
>> > + break;
>> > }
>> > }
>> > }
>> > --
>> > 2.6.3.462.gbe2c914
>>
>> Using my Veyron Jerry Chromebook wiht latest kernel I found that the
>> speakers doesn't work. Bisect points to this patch as the offending
>> commit. However your changes looks reasonable the fact is that
>> reverting the patch makes the audio work again on my device. I need to
>> dig a bit more into the issue, but meanwhile, any idea on what is
>> happening ? Can I ask which device did you test?
>
> I tested this on a Radxa Rock2 square. This change fixed an issue I
> found when running separate processes for playback and recording (I was
> testing with arecord and speaker-test IIRC) but I don't think that
> should be relevant. I just read through the patch again and I can't see
> anything obviously wrong so I'm afraid I don't have any idea what's
> causing the problem you're seeing.
>
By chance, do you have a git repo containing the changes required to
test the sound on Radxa Rock2? Guess this is not in mainline.
Thanks,
Enric
>
> John
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/2] ASoC: rockchip: i2s: separate capture and playback
2016-04-30 7:00 ` John Keeping
2016-05-02 9:13 ` Enric Balletbo Serra
@ 2016-05-03 4:12 ` sugar
2016-05-03 9:16 ` [alsa-devel] " Enric Balletbo Serra
1 sibling, 1 reply; 10+ messages in thread
From: sugar @ 2016-05-03 4:12 UTC (permalink / raw)
To: linux-arm-kernel
Hi John,
On 4/30/2016 15:00, John Keeping Wrote:
> Hi Enric,
>
> On Fri, Apr 29, 2016 at 04:59:27PM +0200, Enric Balletbo Serra wrote:
>> 2015-12-09 11:32 GMT+01:00 John Keeping <john@metanate.com>:
>>> If we only clear the tx/rx state when both are disabled it is not
>>> possible to start/stop one multiple times while the other is running.
>>> Since the two are independently controlled, treat them as such and
>>> remove the false dependency between capture and playback.
>>>
>>> Signed-off-by: John Keeping <john@metanate.com>
>>> ---
>>> sound/soc/rockchip/rockchip_i2s.c | 72 +++++++++++++++++----------------------
>>> 1 file changed, 32 insertions(+), 40 deletions(-)
>>>
>>> diff --git a/sound/soc/rockchip/rockchip_i2s.c b/sound/soc/rockchip/rockchip_i2s.c
>>> index 83b1b9c..acc6225 100644
>>> --- a/sound/soc/rockchip/rockchip_i2s.c
>>> +++ b/sound/soc/rockchip/rockchip_i2s.c
>>> @@ -82,8 +82,8 @@ static void rockchip_snd_txctrl(struct rk_i2s_dev *i2s, int on)
>>> 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);
>>> + I2S_XFER_TXS_START,
>>> + I2S_XFER_TXS_START);
>>>
>>> i2s->tx_start = true;
>>> } else {
>>> @@ -92,27 +92,23 @@ static void rockchip_snd_txctrl(struct rk_i2s_dev *i2s, int on)
>>> regmap_update_bits(i2s->regmap, I2S_DMACR,
>>> I2S_DMACR_TDE_ENABLE, I2S_DMACR_TDE_DISABLE);
>>>
>>> - 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);
>>> + regmap_update_bits(i2s->regmap, I2S_XFER,
>>> + I2S_XFER_TXS_START,
>>> + I2S_XFER_TXS_STOP);
>>>
>>> - regmap_update_bits(i2s->regmap, I2S_CLR,
>>> - I2S_CLR_TXC | I2S_CLR_RXC,
>>> - I2S_CLR_TXC | I2S_CLR_RXC);
>>> + regmap_update_bits(i2s->regmap, I2S_CLR,
>>> + I2S_CLR_TXC,
>>> + I2S_CLR_TXC);
>>>
>>> - regmap_read(i2s->regmap, I2S_CLR, &val);
>>> + regmap_read(i2s->regmap, I2S_CLR, &val);
>>>
>>> - /* Should wait for clear operation to finish */
>>> - while (val) {
>>> - regmap_read(i2s->regmap, I2S_CLR, &val);
>>> - retry--;
>>> - if (!retry) {
>>> - dev_warn(i2s->dev, "fail to clear\n");
>>> - break;
>>> - }
>>> + /* Should wait for clear operation to finish */
>>> + while (val & I2S_CLR_TXC) {
>>> + regmap_read(i2s->regmap, I2S_CLR, &val);
>>> + retry--;
>>> + if (!retry) {
>>> + dev_warn(i2s->dev, "fail to clear\n");
>>> + break;
>>> }
>>> }
>>> }
>>> @@ -128,8 +124,8 @@ static void rockchip_snd_rxctrl(struct rk_i2s_dev *i2s, int on)
>>> 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);
>>> + I2S_XFER_RXS_START,
>>> + I2S_XFER_RXS_START);
>>>
>>> i2s->rx_start = true;
>>> } else {
>>> @@ -138,27 +134,23 @@ static void rockchip_snd_rxctrl(struct rk_i2s_dev *i2s, int on)
>>> regmap_update_bits(i2s->regmap, I2S_DMACR,
>>> I2S_DMACR_RDE_ENABLE, I2S_DMACR_RDE_DISABLE);
>>>
>>> - 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);
>>> + regmap_update_bits(i2s->regmap, I2S_XFER,
>>> + I2S_XFER_RXS_START,
>>> + I2S_XFER_RXS_STOP);
>>>
>>> - regmap_update_bits(i2s->regmap, I2S_CLR,
>>> - I2S_CLR_TXC | I2S_CLR_RXC,
>>> - I2S_CLR_TXC | I2S_CLR_RXC);
>>> + regmap_update_bits(i2s->regmap, I2S_CLR,
>>> + I2S_CLR_RXC,
>>> + I2S_CLR_RXC);
>>>
>>> - regmap_read(i2s->regmap, I2S_CLR, &val);
>>> + regmap_read(i2s->regmap, I2S_CLR, &val);
>>>
>>> - /* Should wait for clear operation to finish */
>>> - while (val) {
>>> - regmap_read(i2s->regmap, I2S_CLR, &val);
>>> - retry--;
>>> - if (!retry) {
>>> - dev_warn(i2s->dev, "fail to clear\n");
>>> - break;
>>> - }
>>> + /* Should wait for clear operation to finish */
>>> + while (val & I2S_CLR_RXC) {
>>> + regmap_read(i2s->regmap, I2S_CLR, &val);
>>> + retry--;
>>> + if (!retry) {
>>> + dev_warn(i2s->dev, "fail to clear\n");
>>> + break;
>>> }
>>> }
>>> }
>>> --
>>> 2.6.3.462.gbe2c914
>>
>> Using my Veyron Jerry Chromebook wiht latest kernel I found that the
>> speakers doesn't work. Bisect points to this patch as the offending
>> commit. However your changes looks reasonable the fact is that
>> reverting the patch makes the audio work again on my device. I need to
>> dig a bit more into the issue, but meanwhile, any idea on what is
>> happening ? Can I ask which device did you test?
>
> I tested this on a Radxa Rock2 square. This change fixed an issue I
> found when running separate processes for playback and recording (I was
> testing with arecord and speaker-test IIRC) but I don't think that
> should be relevant. I just read through the patch again and I can't see
> anything obviously wrong so I'm afraid I don't have any idea what's
> causing the problem you're seeing.
>
Do you have test playback/capture at the same time? I think you may
reproduce this issue if you do as follows:
step 1: tinyplay test.wav -D 0 -d 0 -p 1024 -n 4 &
step 2: tinycap r.wav -D 0 -d 0 -p 1024 -n 4 &
you will find that there is no music now.
because of this problem, we enable tx/rx at the same time to WA this issue.
May I know what's the issue that you met before?
>
> John
>
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip
>
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* [alsa-devel] [PATCH 1/2] ASoC: rockchip: i2s: separate capture and playback
2016-05-03 4:12 ` sugar
@ 2016-05-03 9:16 ` Enric Balletbo Serra
2016-05-03 9:43 ` John Keeping
0 siblings, 1 reply; 10+ messages in thread
From: Enric Balletbo Serra @ 2016-05-03 9:16 UTC (permalink / raw)
To: linux-arm-kernel
Hi all,
2016-05-03 6:12 GMT+02:00 sugar <sugar.zhang@rock-chips.com>:
> Hi John,
>
>
> On 4/30/2016 15:00, John Keeping Wrote:
>>
>> Hi Enric,
>>
>> On Fri, Apr 29, 2016 at 04:59:27PM +0200, Enric Balletbo Serra wrote:
>>>
>>> 2015-12-09 11:32 GMT+01:00 John Keeping <john@metanate.com>:
>>>>
>>>> If we only clear the tx/rx state when both are disabled it is not
>>>> possible to start/stop one multiple times while the other is running.
>>>> Since the two are independently controlled, treat them as such and
>>>> remove the false dependency between capture and playback.
>>>>
>>>> Signed-off-by: John Keeping <john@metanate.com>
>>>> ---
>>>> sound/soc/rockchip/rockchip_i2s.c | 72
>>>> +++++++++++++++++----------------------
>>>> 1 file changed, 32 insertions(+), 40 deletions(-)
>>>>
>>>> diff --git a/sound/soc/rockchip/rockchip_i2s.c
>>>> b/sound/soc/rockchip/rockchip_i2s.c
>>>> index 83b1b9c..acc6225 100644
>>>> --- a/sound/soc/rockchip/rockchip_i2s.c
>>>> +++ b/sound/soc/rockchip/rockchip_i2s.c
>>>> @@ -82,8 +82,8 @@ static void rockchip_snd_txctrl(struct rk_i2s_dev
>>>> *i2s, int on)
>>>> 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);
>>>> + I2S_XFER_TXS_START,
>>>> + I2S_XFER_TXS_START);
>>>>
>>>> i2s->tx_start = true;
>>>> } else {
>>>> @@ -92,27 +92,23 @@ static void rockchip_snd_txctrl(struct rk_i2s_dev
>>>> *i2s, int on)
>>>> regmap_update_bits(i2s->regmap, I2S_DMACR,
>>>> I2S_DMACR_TDE_ENABLE,
>>>> I2S_DMACR_TDE_DISABLE);
>>>>
>>>> - 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);
>>>> + regmap_update_bits(i2s->regmap, I2S_XFER,
>>>> + I2S_XFER_TXS_START,
>>>> + I2S_XFER_TXS_STOP);
>>>>
>>>> - regmap_update_bits(i2s->regmap, I2S_CLR,
>>>> - I2S_CLR_TXC | I2S_CLR_RXC,
>>>> - I2S_CLR_TXC | I2S_CLR_RXC);
>>>> + regmap_update_bits(i2s->regmap, I2S_CLR,
>>>> + I2S_CLR_TXC,
>>>> + I2S_CLR_TXC);
>>>>
>>>> - regmap_read(i2s->regmap, I2S_CLR, &val);
>>>> + regmap_read(i2s->regmap, I2S_CLR, &val);
>>>>
>>>> - /* Should wait for clear operation to finish */
>>>> - while (val) {
>>>> - regmap_read(i2s->regmap, I2S_CLR, &val);
>>>> - retry--;
>>>> - if (!retry) {
>>>> - dev_warn(i2s->dev, "fail to
>>>> clear\n");
>>>> - break;
>>>> - }
>>>> + /* Should wait for clear operation to finish */
>>>> + while (val & I2S_CLR_TXC) {
>>>> + regmap_read(i2s->regmap, I2S_CLR, &val);
>>>> + retry--;
>>>> + if (!retry) {
>>>> + dev_warn(i2s->dev, "fail to clear\n");
>>>> + break;
>>>> }
>>>> }
>>>> }
>>>> @@ -128,8 +124,8 @@ static void rockchip_snd_rxctrl(struct rk_i2s_dev
>>>> *i2s, int on)
>>>> 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);
>>>> + I2S_XFER_RXS_START,
>>>> + I2S_XFER_RXS_START);
>>>>
>>>> i2s->rx_start = true;
>>>> } else {
>>>> @@ -138,27 +134,23 @@ static void rockchip_snd_rxctrl(struct rk_i2s_dev
>>>> *i2s, int on)
>>>> regmap_update_bits(i2s->regmap, I2S_DMACR,
>>>> I2S_DMACR_RDE_ENABLE,
>>>> I2S_DMACR_RDE_DISABLE);
>>>>
>>>> - 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);
>>>> + regmap_update_bits(i2s->regmap, I2S_XFER,
>>>> + I2S_XFER_RXS_START,
>>>> + I2S_XFER_RXS_STOP);
>>>>
>>>> - regmap_update_bits(i2s->regmap, I2S_CLR,
>>>> - I2S_CLR_TXC | I2S_CLR_RXC,
>>>> - I2S_CLR_TXC | I2S_CLR_RXC);
>>>> + regmap_update_bits(i2s->regmap, I2S_CLR,
>>>> + I2S_CLR_RXC,
>>>> + I2S_CLR_RXC);
>>>>
>>>> - regmap_read(i2s->regmap, I2S_CLR, &val);
>>>> + regmap_read(i2s->regmap, I2S_CLR, &val);
>>>>
>>>> - /* Should wait for clear operation to finish */
>>>> - while (val) {
>>>> - regmap_read(i2s->regmap, I2S_CLR, &val);
>>>> - retry--;
>>>> - if (!retry) {
>>>> - dev_warn(i2s->dev, "fail to
>>>> clear\n");
>>>> - break;
>>>> - }
>>>> + /* Should wait for clear operation to finish */
>>>> + while (val & I2S_CLR_RXC) {
>>>> + regmap_read(i2s->regmap, I2S_CLR, &val);
>>>> + retry--;
>>>> + if (!retry) {
>>>> + dev_warn(i2s->dev, "fail to clear\n");
>>>> + break;
>>>> }
>>>> }
>>>> }
>>>> --
>>>> 2.6.3.462.gbe2c914
>>>
>>>
>>> Using my Veyron Jerry Chromebook wiht latest kernel I found that the
>>> speakers doesn't work. Bisect points to this patch as the offending
>>> commit. However your changes looks reasonable the fact is that
>>> reverting the patch makes the audio work again on my device. I need to
>>> dig a bit more into the issue, but meanwhile, any idea on what is
>>> happening ? Can I ask which device did you test?
>>
>>
>> I tested this on a Radxa Rock2 square. This change fixed an issue I
>> found when running separate processes for playback and recording (I was
>> testing with arecord and speaker-test IIRC) but I don't think that
>> should be relevant. I just read through the patch again and I can't see
>> anything obviously wrong so I'm afraid I don't have any idea what's
>> causing the problem you're seeing.
>>
> Do you have test playback/capture at the same time? I think you may
> reproduce this issue if you do as follows:
>
> step 1: tinyplay test.wav -D 0 -d 0 -p 1024 -n 4 &
> step 2: tinycap r.wav -D 0 -d 0 -p 1024 -n 4 &
>
> you will find that there is no music now.
>
> because of this problem, we enable tx/rx at the same time to WA this issue.
>
I can confirm that reverting John's patch audio playback/capture at
the same time works on my Veyron Jerry Chromebook.
> May I know what's the issue that you met before?
>>
>>
>> John
>>
>> _______________________________________________
>> Linux-rockchip mailing list
>> Linux-rockchip at lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-rockchip
>>
>>
>>
>
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel at alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* [alsa-devel] [PATCH 1/2] ASoC: rockchip: i2s: separate capture and playback
2016-05-03 9:16 ` [alsa-devel] " Enric Balletbo Serra
@ 2016-05-03 9:43 ` John Keeping
2016-05-03 10:21 ` Enric Balletbo Serra
0 siblings, 1 reply; 10+ messages in thread
From: John Keeping @ 2016-05-03 9:43 UTC (permalink / raw)
To: linux-arm-kernel
Hi Enric,
On Tue, 3 May 2016 11:16:58 +0200, Enric Balletbo Serra wrote:
> 2016-05-03 6:12 GMT+02:00 sugar <sugar.zhang@rock-chips.com>:
> > On 4/30/2016 15:00, John Keeping Wrote:
> >> On Fri, Apr 29, 2016 at 04:59:27PM +0200, Enric Balletbo Serra wrote:
> >>>
> >>> 2015-12-09 11:32 GMT+01:00 John Keeping <john@metanate.com>:
> >>>>
> >>>> If we only clear the tx/rx state when both are disabled it is not
> >>>> possible to start/stop one multiple times while the other is running.
> >>>> Since the two are independently controlled, treat them as such and
> >>>> remove the false dependency between capture and playback.
> >>>>
> >>>> Signed-off-by: John Keeping <john@metanate.com>
> >>>> ---
> >>>> sound/soc/rockchip/rockchip_i2s.c | 72
> >>>> +++++++++++++++++----------------------
> >>>> 1 file changed, 32 insertions(+), 40 deletions(-)
> >>>>
> >>>> diff --git a/sound/soc/rockchip/rockchip_i2s.c
> >>>> b/sound/soc/rockchip/rockchip_i2s.c
> >>>> index 83b1b9c..acc6225 100644
> >>>> --- a/sound/soc/rockchip/rockchip_i2s.c
> >>>> +++ b/sound/soc/rockchip/rockchip_i2s.c
> >>>> @@ -82,8 +82,8 @@ static void rockchip_snd_txctrl(struct rk_i2s_dev
> >>>> *i2s, int on)
> >>>> 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);
> >>>> + I2S_XFER_TXS_START,
> >>>> + I2S_XFER_TXS_START);
> >>>>
> >>>> i2s->tx_start = true;
> >>>> } else {
> >>>> @@ -92,27 +92,23 @@ static void rockchip_snd_txctrl(struct rk_i2s_dev
> >>>> *i2s, int on)
> >>>> regmap_update_bits(i2s->regmap, I2S_DMACR,
> >>>> I2S_DMACR_TDE_ENABLE,
> >>>> I2S_DMACR_TDE_DISABLE);
> >>>>
> >>>> - 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);
> >>>> + regmap_update_bits(i2s->regmap, I2S_XFER,
> >>>> + I2S_XFER_TXS_START,
> >>>> + I2S_XFER_TXS_STOP);
> >>>>
> >>>> - regmap_update_bits(i2s->regmap, I2S_CLR,
> >>>> - I2S_CLR_TXC | I2S_CLR_RXC,
> >>>> - I2S_CLR_TXC | I2S_CLR_RXC);
> >>>> + regmap_update_bits(i2s->regmap, I2S_CLR,
> >>>> + I2S_CLR_TXC,
> >>>> + I2S_CLR_TXC);
> >>>>
> >>>> - regmap_read(i2s->regmap, I2S_CLR, &val);
> >>>> + regmap_read(i2s->regmap, I2S_CLR, &val);
> >>>>
> >>>> - /* Should wait for clear operation to finish */
> >>>> - while (val) {
> >>>> - regmap_read(i2s->regmap, I2S_CLR, &val);
> >>>> - retry--;
> >>>> - if (!retry) {
> >>>> - dev_warn(i2s->dev, "fail to
> >>>> clear\n");
> >>>> - break;
> >>>> - }
> >>>> + /* Should wait for clear operation to finish */
> >>>> + while (val & I2S_CLR_TXC) {
> >>>> + regmap_read(i2s->regmap, I2S_CLR, &val);
> >>>> + retry--;
> >>>> + if (!retry) {
> >>>> + dev_warn(i2s->dev, "fail to clear\n");
> >>>> + break;
> >>>> }
> >>>> }
> >>>> }
> >>>> @@ -128,8 +124,8 @@ static void rockchip_snd_rxctrl(struct rk_i2s_dev
> >>>> *i2s, int on)
> >>>> 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);
> >>>> + I2S_XFER_RXS_START,
> >>>> + I2S_XFER_RXS_START);
> >>>>
> >>>> i2s->rx_start = true;
> >>>> } else {
> >>>> @@ -138,27 +134,23 @@ static void rockchip_snd_rxctrl(struct rk_i2s_dev
> >>>> *i2s, int on)
> >>>> regmap_update_bits(i2s->regmap, I2S_DMACR,
> >>>> I2S_DMACR_RDE_ENABLE,
> >>>> I2S_DMACR_RDE_DISABLE);
> >>>>
> >>>> - 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);
> >>>> + regmap_update_bits(i2s->regmap, I2S_XFER,
> >>>> + I2S_XFER_RXS_START,
> >>>> + I2S_XFER_RXS_STOP);
> >>>>
> >>>> - regmap_update_bits(i2s->regmap, I2S_CLR,
> >>>> - I2S_CLR_TXC | I2S_CLR_RXC,
> >>>> - I2S_CLR_TXC | I2S_CLR_RXC);
> >>>> + regmap_update_bits(i2s->regmap, I2S_CLR,
> >>>> + I2S_CLR_RXC,
> >>>> + I2S_CLR_RXC);
> >>>>
> >>>> - regmap_read(i2s->regmap, I2S_CLR, &val);
> >>>> + regmap_read(i2s->regmap, I2S_CLR, &val);
> >>>>
> >>>> - /* Should wait for clear operation to finish */
> >>>> - while (val) {
> >>>> - regmap_read(i2s->regmap, I2S_CLR, &val);
> >>>> - retry--;
> >>>> - if (!retry) {
> >>>> - dev_warn(i2s->dev, "fail to
> >>>> clear\n");
> >>>> - break;
> >>>> - }
> >>>> + /* Should wait for clear operation to finish */
> >>>> + while (val & I2S_CLR_RXC) {
> >>>> + regmap_read(i2s->regmap, I2S_CLR, &val);
> >>>> + retry--;
> >>>> + if (!retry) {
> >>>> + dev_warn(i2s->dev, "fail to clear\n");
> >>>> + break;
> >>>> }
> >>>> }
> >>>> }
> >>>> --
> >>>> 2.6.3.462.gbe2c914
> >>>
> >>>
> >>> Using my Veyron Jerry Chromebook wiht latest kernel I found that the
> >>> speakers doesn't work. Bisect points to this patch as the offending
> >>> commit. However your changes looks reasonable the fact is that
> >>> reverting the patch makes the audio work again on my device. I need to
> >>> dig a bit more into the issue, but meanwhile, any idea on what is
> >>> happening ? Can I ask which device did you test?
> >>
> >>
> >> I tested this on a Radxa Rock2 square. This change fixed an issue I
> >> found when running separate processes for playback and recording (I was
> >> testing with arecord and speaker-test IIRC) but I don't think that
> >> should be relevant. I just read through the patch again and I can't see
> >> anything obviously wrong so I'm afraid I don't have any idea what's
> >> causing the problem you're seeing.
> >>
> > Do you have test playback/capture at the same time? I think you may
> > reproduce this issue if you do as follows:
> >
> > step 1: tinyplay test.wav -D 0 -d 0 -p 1024 -n 4 &
> > step 2: tinycap r.wav -D 0 -d 0 -p 1024 -n 4 &
> >
> > you will find that there is no music now.
> >
> > because of this problem, we enable tx/rx at the same time to WA this issue.
> >
>
> I can confirm that reverting John's patch audio playback/capture at
> the same time works on my Veyron Jerry Chromebook.
Have you tried stopping one of the processes and then starting it again
(without touching the other process)? That's the case that was broken
for me without this patch.
I have had a quick look through the code I'm using and I noticed that I
have another change that hasn't been sent upstream, which marks the
I2S_FIFOLR register as volatile in rockchip_i2s_volatile_reg():
-- >8 --
Subject: [PATCH] ASoC: rockchip: i2s: mark FIFOLR register volatile
The FIFO levels will be updated by the device so they should not be
cached.
Signed-off-by: John Keeping <john@metanate.com>
---
sound/soc/rockchip/rockchip_i2s.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/sound/soc/rockchip/rockchip_i2s.c b/sound/soc/rockchip/rockchip_i2s.c
index d78f4c925b02..b517d8abcc40 100644
--- a/sound/soc/rockchip/rockchip_i2s.c
+++ b/sound/soc/rockchip/rockchip_i2s.c
@@ -434,6 +434,7 @@ static bool rockchip_i2s_volatile_reg(struct device *dev, unsigned int reg)
switch (reg) {
case I2S_INTSR:
case I2S_CLR:
+ case I2S_FIFOLR:
return true;
default:
return false;
-- 8< --
Regards,
John
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [alsa-devel] [PATCH 1/2] ASoC: rockchip: i2s: separate capture and playback
2016-05-03 9:43 ` John Keeping
@ 2016-05-03 10:21 ` Enric Balletbo Serra
2016-05-03 10:32 ` John Keeping
0 siblings, 1 reply; 10+ messages in thread
From: Enric Balletbo Serra @ 2016-05-03 10:21 UTC (permalink / raw)
To: linux-arm-kernel
Hi John,
2016-05-03 11:43 GMT+02:00 John Keeping <john@metanate.com>:
> Hi Enric,
>
> On Tue, 3 May 2016 11:16:58 +0200, Enric Balletbo Serra wrote:
>> 2016-05-03 6:12 GMT+02:00 sugar <sugar.zhang@rock-chips.com>:
>> > On 4/30/2016 15:00, John Keeping Wrote:
>> >> On Fri, Apr 29, 2016 at 04:59:27PM +0200, Enric Balletbo Serra wrote:
>> >>>
>> >>> 2015-12-09 11:32 GMT+01:00 John Keeping <john@metanate.com>:
>> >>>>
>> >>>> If we only clear the tx/rx state when both are disabled it is not
>> >>>> possible to start/stop one multiple times while the other is running.
>> >>>> Since the two are independently controlled, treat them as such and
>> >>>> remove the false dependency between capture and playback.
>> >>>>
>> >>>> Signed-off-by: John Keeping <john@metanate.com>
>> >>>> ---
>> >>>> sound/soc/rockchip/rockchip_i2s.c | 72
>> >>>> +++++++++++++++++----------------------
>> >>>> 1 file changed, 32 insertions(+), 40 deletions(-)
>> >>>>
>> >>>> diff --git a/sound/soc/rockchip/rockchip_i2s.c
>> >>>> b/sound/soc/rockchip/rockchip_i2s.c
>> >>>> index 83b1b9c..acc6225 100644
>> >>>> --- a/sound/soc/rockchip/rockchip_i2s.c
>> >>>> +++ b/sound/soc/rockchip/rockchip_i2s.c
>> >>>> @@ -82,8 +82,8 @@ static void rockchip_snd_txctrl(struct rk_i2s_dev
>> >>>> *i2s, int on)
>> >>>> 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);
>> >>>> + I2S_XFER_TXS_START,
>> >>>> + I2S_XFER_TXS_START);
>> >>>>
>> >>>> i2s->tx_start = true;
>> >>>> } else {
>> >>>> @@ -92,27 +92,23 @@ static void rockchip_snd_txctrl(struct rk_i2s_dev
>> >>>> *i2s, int on)
>> >>>> regmap_update_bits(i2s->regmap, I2S_DMACR,
>> >>>> I2S_DMACR_TDE_ENABLE,
>> >>>> I2S_DMACR_TDE_DISABLE);
>> >>>>
>> >>>> - 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);
>> >>>> + regmap_update_bits(i2s->regmap, I2S_XFER,
>> >>>> + I2S_XFER_TXS_START,
>> >>>> + I2S_XFER_TXS_STOP);
>> >>>>
>> >>>> - regmap_update_bits(i2s->regmap, I2S_CLR,
>> >>>> - I2S_CLR_TXC | I2S_CLR_RXC,
>> >>>> - I2S_CLR_TXC | I2S_CLR_RXC);
>> >>>> + regmap_update_bits(i2s->regmap, I2S_CLR,
>> >>>> + I2S_CLR_TXC,
>> >>>> + I2S_CLR_TXC);
>> >>>>
>> >>>> - regmap_read(i2s->regmap, I2S_CLR, &val);
>> >>>> + regmap_read(i2s->regmap, I2S_CLR, &val);
>> >>>>
>> >>>> - /* Should wait for clear operation to finish */
>> >>>> - while (val) {
>> >>>> - regmap_read(i2s->regmap, I2S_CLR, &val);
>> >>>> - retry--;
>> >>>> - if (!retry) {
>> >>>> - dev_warn(i2s->dev, "fail to
>> >>>> clear\n");
>> >>>> - break;
>> >>>> - }
>> >>>> + /* Should wait for clear operation to finish */
>> >>>> + while (val & I2S_CLR_TXC) {
>> >>>> + regmap_read(i2s->regmap, I2S_CLR, &val);
>> >>>> + retry--;
>> >>>> + if (!retry) {
>> >>>> + dev_warn(i2s->dev, "fail to clear\n");
>> >>>> + break;
>> >>>> }
>> >>>> }
>> >>>> }
>> >>>> @@ -128,8 +124,8 @@ static void rockchip_snd_rxctrl(struct rk_i2s_dev
>> >>>> *i2s, int on)
>> >>>> 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);
>> >>>> + I2S_XFER_RXS_START,
>> >>>> + I2S_XFER_RXS_START);
>> >>>>
>> >>>> i2s->rx_start = true;
>> >>>> } else {
>> >>>> @@ -138,27 +134,23 @@ static void rockchip_snd_rxctrl(struct rk_i2s_dev
>> >>>> *i2s, int on)
>> >>>> regmap_update_bits(i2s->regmap, I2S_DMACR,
>> >>>> I2S_DMACR_RDE_ENABLE,
>> >>>> I2S_DMACR_RDE_DISABLE);
>> >>>>
>> >>>> - 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);
>> >>>> + regmap_update_bits(i2s->regmap, I2S_XFER,
>> >>>> + I2S_XFER_RXS_START,
>> >>>> + I2S_XFER_RXS_STOP);
>> >>>>
>> >>>> - regmap_update_bits(i2s->regmap, I2S_CLR,
>> >>>> - I2S_CLR_TXC | I2S_CLR_RXC,
>> >>>> - I2S_CLR_TXC | I2S_CLR_RXC);
>> >>>> + regmap_update_bits(i2s->regmap, I2S_CLR,
>> >>>> + I2S_CLR_RXC,
>> >>>> + I2S_CLR_RXC);
>> >>>>
>> >>>> - regmap_read(i2s->regmap, I2S_CLR, &val);
>> >>>> + regmap_read(i2s->regmap, I2S_CLR, &val);
>> >>>>
>> >>>> - /* Should wait for clear operation to finish */
>> >>>> - while (val) {
>> >>>> - regmap_read(i2s->regmap, I2S_CLR, &val);
>> >>>> - retry--;
>> >>>> - if (!retry) {
>> >>>> - dev_warn(i2s->dev, "fail to
>> >>>> clear\n");
>> >>>> - break;
>> >>>> - }
>> >>>> + /* Should wait for clear operation to finish */
>> >>>> + while (val & I2S_CLR_RXC) {
>> >>>> + regmap_read(i2s->regmap, I2S_CLR, &val);
>> >>>> + retry--;
>> >>>> + if (!retry) {
>> >>>> + dev_warn(i2s->dev, "fail to clear\n");
>> >>>> + break;
>> >>>> }
>> >>>> }
>> >>>> }
>> >>>> --
>> >>>> 2.6.3.462.gbe2c914
>> >>>
>> >>>
>> >>> Using my Veyron Jerry Chromebook wiht latest kernel I found that the
>> >>> speakers doesn't work. Bisect points to this patch as the offending
>> >>> commit. However your changes looks reasonable the fact is that
>> >>> reverting the patch makes the audio work again on my device. I need to
>> >>> dig a bit more into the issue, but meanwhile, any idea on what is
>> >>> happening ? Can I ask which device did you test?
>> >>
>> >>
>> >> I tested this on a Radxa Rock2 square. This change fixed an issue I
>> >> found when running separate processes for playback and recording (I was
>> >> testing with arecord and speaker-test IIRC) but I don't think that
>> >> should be relevant. I just read through the patch again and I can't see
>> >> anything obviously wrong so I'm afraid I don't have any idea what's
>> >> causing the problem you're seeing.
>> >>
>> > Do you have test playback/capture at the same time? I think you may
>> > reproduce this issue if you do as follows:
>> >
>> > step 1: tinyplay test.wav -D 0 -d 0 -p 1024 -n 4 &
>> > step 2: tinycap r.wav -D 0 -d 0 -p 1024 -n 4 &
>> >
>> > you will find that there is no music now.
>> >
>> > because of this problem, we enable tx/rx at the same time to WA this issue.
>> >
>>
>> I can confirm that reverting John's patch audio playback/capture at
>> the same time works on my Veyron Jerry Chromebook.
>
> Have you tried stopping one of the processes and then starting it again
> (without touching the other process)? That's the case that was broken
> for me without this patch.
>
I just tried that and it works on my platform. Meanwhile I reproduced
a song I recorded two wavs.
> I have had a quick look through the code I'm using and I noticed that I
> have another change that hasn't been sent upstream, which marks the
> I2S_FIFOLR register as volatile in rockchip_i2s_volatile_reg():
>
Unfortunately that didn't help, I hear nothing from the speakers :(
I have a Radxa Rock2 Square board, by chance, do you have a git tree
containing the patches to enable the i2s sound for radxa so I can
reproduce both here? At least the codec is not upstreamed, I guess.
> -- >8 --
> Subject: [PATCH] ASoC: rockchip: i2s: mark FIFOLR register volatile
>
> The FIFO levels will be updated by the device so they should not be
> cached.
>
> Signed-off-by: John Keeping <john@metanate.com>
> ---
> sound/soc/rockchip/rockchip_i2s.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/sound/soc/rockchip/rockchip_i2s.c b/sound/soc/rockchip/rockchip_i2s.c
> index d78f4c925b02..b517d8abcc40 100644
> --- a/sound/soc/rockchip/rockchip_i2s.c
> +++ b/sound/soc/rockchip/rockchip_i2s.c
> @@ -434,6 +434,7 @@ static bool rockchip_i2s_volatile_reg(struct device *dev, unsigned int reg)
> switch (reg) {
> case I2S_INTSR:
> case I2S_CLR:
> + case I2S_FIFOLR:
> return true;
> default:
> return false;
> -- 8< --
>
>
> Regards,
> John
^ permalink raw reply [flat|nested] 10+ messages in thread
* [alsa-devel] [PATCH 1/2] ASoC: rockchip: i2s: separate capture and playback
2016-05-03 10:21 ` Enric Balletbo Serra
@ 2016-05-03 10:32 ` John Keeping
0 siblings, 0 replies; 10+ messages in thread
From: John Keeping @ 2016-05-03 10:32 UTC (permalink / raw)
To: linux-arm-kernel
Hi Enric,
On Tue, 3 May 2016 12:21:25 +0200, Enric Balletbo Serra wrote:
> 2016-05-03 11:43 GMT+02:00 John Keeping <john@metanate.com>:
> > On Tue, 3 May 2016 11:16:58 +0200, Enric Balletbo Serra wrote:
> >> 2016-05-03 6:12 GMT+02:00 sugar <sugar.zhang@rock-chips.com>:
> >> > On 4/30/2016 15:00, John Keeping Wrote:
> >> >> On Fri, Apr 29, 2016 at 04:59:27PM +0200, Enric Balletbo Serra wrote:
> >> >>>
> >> >>> 2015-12-09 11:32 GMT+01:00 John Keeping <john@metanate.com>:
> >> >>>>
> >> >>>> If we only clear the tx/rx state when both are disabled it is not
> >> >>>> possible to start/stop one multiple times while the other is running.
> >> >>>> Since the two are independently controlled, treat them as such and
> >> >>>> remove the false dependency between capture and playback.
> >> >>>>
> >> >>>> Signed-off-by: John Keeping <john@metanate.com>
> >> >>>> ---
> >> >>>> sound/soc/rockchip/rockchip_i2s.c | 72
> >> >>>> +++++++++++++++++----------------------
> >> >>>> 1 file changed, 32 insertions(+), 40 deletions(-)
> >> >>>>
> >> >>>> diff --git a/sound/soc/rockchip/rockchip_i2s.c
> >> >>>> b/sound/soc/rockchip/rockchip_i2s.c
> >> >>>> index 83b1b9c..acc6225 100644
> >> >>>> --- a/sound/soc/rockchip/rockchip_i2s.c
> >> >>>> +++ b/sound/soc/rockchip/rockchip_i2s.c
> >> >>>> @@ -82,8 +82,8 @@ static void rockchip_snd_txctrl(struct rk_i2s_dev
> >> >>>> *i2s, int on)
> >> >>>> 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);
> >> >>>> + I2S_XFER_TXS_START,
> >> >>>> + I2S_XFER_TXS_START);
> >> >>>>
> >> >>>> i2s->tx_start = true;
> >> >>>> } else {
> >> >>>> @@ -92,27 +92,23 @@ static void rockchip_snd_txctrl(struct rk_i2s_dev
> >> >>>> *i2s, int on)
> >> >>>> regmap_update_bits(i2s->regmap, I2S_DMACR,
> >> >>>> I2S_DMACR_TDE_ENABLE,
> >> >>>> I2S_DMACR_TDE_DISABLE);
> >> >>>>
> >> >>>> - 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);
> >> >>>> + regmap_update_bits(i2s->regmap, I2S_XFER,
> >> >>>> + I2S_XFER_TXS_START,
> >> >>>> + I2S_XFER_TXS_STOP);
> >> >>>>
> >> >>>> - regmap_update_bits(i2s->regmap, I2S_CLR,
> >> >>>> - I2S_CLR_TXC | I2S_CLR_RXC,
> >> >>>> - I2S_CLR_TXC | I2S_CLR_RXC);
> >> >>>> + regmap_update_bits(i2s->regmap, I2S_CLR,
> >> >>>> + I2S_CLR_TXC,
> >> >>>> + I2S_CLR_TXC);
> >> >>>>
> >> >>>> - regmap_read(i2s->regmap, I2S_CLR, &val);
> >> >>>> + regmap_read(i2s->regmap, I2S_CLR, &val);
> >> >>>>
> >> >>>> - /* Should wait for clear operation to finish */
> >> >>>> - while (val) {
> >> >>>> - regmap_read(i2s->regmap, I2S_CLR, &val);
> >> >>>> - retry--;
> >> >>>> - if (!retry) {
> >> >>>> - dev_warn(i2s->dev, "fail to
> >> >>>> clear\n");
> >> >>>> - break;
> >> >>>> - }
> >> >>>> + /* Should wait for clear operation to finish */
> >> >>>> + while (val & I2S_CLR_TXC) {
> >> >>>> + regmap_read(i2s->regmap, I2S_CLR, &val);
> >> >>>> + retry--;
> >> >>>> + if (!retry) {
> >> >>>> + dev_warn(i2s->dev, "fail to clear\n");
> >> >>>> + break;
> >> >>>> }
> >> >>>> }
> >> >>>> }
> >> >>>> @@ -128,8 +124,8 @@ static void rockchip_snd_rxctrl(struct rk_i2s_dev
> >> >>>> *i2s, int on)
> >> >>>> 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);
> >> >>>> + I2S_XFER_RXS_START,
> >> >>>> + I2S_XFER_RXS_START);
> >> >>>>
> >> >>>> i2s->rx_start = true;
> >> >>>> } else {
> >> >>>> @@ -138,27 +134,23 @@ static void rockchip_snd_rxctrl(struct rk_i2s_dev
> >> >>>> *i2s, int on)
> >> >>>> regmap_update_bits(i2s->regmap, I2S_DMACR,
> >> >>>> I2S_DMACR_RDE_ENABLE,
> >> >>>> I2S_DMACR_RDE_DISABLE);
> >> >>>>
> >> >>>> - 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);
> >> >>>> + regmap_update_bits(i2s->regmap, I2S_XFER,
> >> >>>> + I2S_XFER_RXS_START,
> >> >>>> + I2S_XFER_RXS_STOP);
> >> >>>>
> >> >>>> - regmap_update_bits(i2s->regmap, I2S_CLR,
> >> >>>> - I2S_CLR_TXC | I2S_CLR_RXC,
> >> >>>> - I2S_CLR_TXC | I2S_CLR_RXC);
> >> >>>> + regmap_update_bits(i2s->regmap, I2S_CLR,
> >> >>>> + I2S_CLR_RXC,
> >> >>>> + I2S_CLR_RXC);
> >> >>>>
> >> >>>> - regmap_read(i2s->regmap, I2S_CLR, &val);
> >> >>>> + regmap_read(i2s->regmap, I2S_CLR, &val);
> >> >>>>
> >> >>>> - /* Should wait for clear operation to finish */
> >> >>>> - while (val) {
> >> >>>> - regmap_read(i2s->regmap, I2S_CLR, &val);
> >> >>>> - retry--;
> >> >>>> - if (!retry) {
> >> >>>> - dev_warn(i2s->dev, "fail to
> >> >>>> clear\n");
> >> >>>> - break;
> >> >>>> - }
> >> >>>> + /* Should wait for clear operation to finish */
> >> >>>> + while (val & I2S_CLR_RXC) {
> >> >>>> + regmap_read(i2s->regmap, I2S_CLR, &val);
> >> >>>> + retry--;
> >> >>>> + if (!retry) {
> >> >>>> + dev_warn(i2s->dev, "fail to clear\n");
> >> >>>> + break;
> >> >>>> }
> >> >>>> }
> >> >>>> }
> >> >>>> --
> >> >>>> 2.6.3.462.gbe2c914
> >> >>>
> >> >>>
> >> >>> Using my Veyron Jerry Chromebook wiht latest kernel I found that the
> >> >>> speakers doesn't work. Bisect points to this patch as the offending
> >> >>> commit. However your changes looks reasonable the fact is that
> >> >>> reverting the patch makes the audio work again on my device. I need to
> >> >>> dig a bit more into the issue, but meanwhile, any idea on what is
> >> >>> happening ? Can I ask which device did you test?
> >> >>
> >> >>
> >> >> I tested this on a Radxa Rock2 square. This change fixed an issue I
> >> >> found when running separate processes for playback and recording (I was
> >> >> testing with arecord and speaker-test IIRC) but I don't think that
> >> >> should be relevant. I just read through the patch again and I can't see
> >> >> anything obviously wrong so I'm afraid I don't have any idea what's
> >> >> causing the problem you're seeing.
> >> >>
> >> > Do you have test playback/capture at the same time? I think you may
> >> > reproduce this issue if you do as follows:
> >> >
> >> > step 1: tinyplay test.wav -D 0 -d 0 -p 1024 -n 4 &
> >> > step 2: tinycap r.wav -D 0 -d 0 -p 1024 -n 4 &
> >> >
> >> > you will find that there is no music now.
> >> >
> >> > because of this problem, we enable tx/rx at the same time to WA this issue.
> >> >
> >>
> >> I can confirm that reverting John's patch audio playback/capture at
> >> the same time works on my Veyron Jerry Chromebook.
> >
> > Have you tried stopping one of the processes and then starting it again
> > (without touching the other process)? That's the case that was broken
> > for me without this patch.
> >
>
> I just tried that and it works on my platform. Meanwhile I reproduced
> a song I recorded two wavs.
>
> > I have had a quick look through the code I'm using and I noticed that I
> > have another change that hasn't been sent upstream, which marks the
> > I2S_FIFOLR register as volatile in rockchip_i2s_volatile_reg():
> >
>
> Unfortunately that didn't help, I hear nothing from the speakers :(
Yeah, I looked a bit more after sending the previous mail and that
register is never referenced by the driver!
> I have a Radxa Rock2 Square board, by chance, do you have a git tree
> containing the patches to enable the i2s sound for radxa so I can
> reproduce both here? At least the codec is not upstreamed, I guess.
I've just pushed a branch to Github:
https://github.com/johnkeeping/linux/tree/topic/audio
Regards,
John
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2016-05-03 10:32 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-12-09 10:32 [PATCH 1/2] ASoC: rockchip: i2s: separate capture and playback John Keeping
2015-12-09 10:32 ` [PATCH 2/2] ASoC: rockchip: i2s: remove unused variables John Keeping
2016-04-29 14:59 ` [PATCH 1/2] ASoC: rockchip: i2s: separate capture and playback Enric Balletbo Serra
2016-04-30 7:00 ` John Keeping
2016-05-02 9:13 ` Enric Balletbo Serra
2016-05-03 4:12 ` sugar
2016-05-03 9:16 ` [alsa-devel] " Enric Balletbo Serra
2016-05-03 9:43 ` John Keeping
2016-05-03 10:21 ` Enric Balletbo Serra
2016-05-03 10:32 ` John Keeping
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).