[1/2] ASoC: rockchip: i2s: separate capture and playback
diff mbox

Message ID 1449657147-26959-1-git-send-email-john@metanate.com
State New
Headers show

Commit Message

John Keeping Dec. 9, 2015, 10:32 a.m. UTC
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(-)

Comments

Enric Balletbo Serra April 29, 2016, 2:59 p.m. UTC | #1
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@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
John Keeping April 30, 2016, 7 a.m. UTC | #2
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
Enric Balletbo Serra May 2, 2016, 9:13 a.m. UTC | #3
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
Sugar Zhang May 3, 2016, 4:12 a.m. UTC | #4
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@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip
>
>
>
Enric Balletbo Serra May 3, 2016, 9:16 a.m. UTC | #5
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@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-rockchip
>>
>>
>>
>
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Patch
diff mbox

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;
 			}
 		}
 	}