From: Enric Balletbo Serra <eballetbo@gmail.com>
To: John Keeping <john@keeping.me.uk>
Cc: linux-rockchip@lists.infradead.org,
Liam Girdwood <lgirdwood@gmail.com>,
Mark Brown <broonie@kernel.org>, Jaroslav Kysela <perex@perex.cz>,
Takashi Iwai <tiwai@suse.com>, Heiko Stuebner <heiko@sntech.de>,
alsa-devel@alsa-project.org,
"linux-arm-kernel@lists.infradead.org"
<linux-arm-kernel@lists.infradead.org>,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/2] ASoC: rockchip: i2s: separate capture and playback
Date: Mon, 2 May 2016 11:13:31 +0200 [thread overview]
Message-ID: <CAFqH_51sSqaoUtadYux+A38wQ1y9T-GKU3_PEcgT9rp_19_b8g@mail.gmail.com> (raw)
In-Reply-To: <20160430070049.GF14612@serenity.lan>
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
WARNING: multiple messages have this Message-ID (diff)
From: eballetbo@gmail.com (Enric Balletbo Serra)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH 1/2] ASoC: rockchip: i2s: separate capture and playback
Date: Mon, 2 May 2016 11:13:31 +0200 [thread overview]
Message-ID: <CAFqH_51sSqaoUtadYux+A38wQ1y9T-GKU3_PEcgT9rp_19_b8g@mail.gmail.com> (raw)
In-Reply-To: <20160430070049.GF14612@serenity.lan>
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
next prev parent reply other threads:[~2016-05-02 9:13 UTC|newest]
Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-12-09 10:32 [PATCH 1/2] ASoC: rockchip: i2s: separate capture and playback John Keeping
2015-12-09 10:32 ` John Keeping
2015-12-09 10:32 ` [PATCH 2/2] ASoC: rockchip: i2s: remove unused variables John Keeping
2015-12-09 10:32 ` John Keeping
2015-12-09 10:32 ` John Keeping
2015-12-09 20:44 ` Applied "ASoC: rockchip: i2s: remove unused variables" to the asoc tree Mark Brown
2015-12-09 20:44 ` Applied "ASoC: rockchip: i2s: separate capture and playback" " Mark Brown
2016-04-29 14:55 ` [PATCH 1/2] ASoC: rockchip: i2s: separate capture and playback Enric Balletbo Serra
2016-04-29 14:59 ` Enric Balletbo Serra
2016-04-29 14:59 ` Enric Balletbo Serra
2016-04-29 14:59 ` Enric Balletbo Serra
2016-04-30 7:00 ` John Keeping
2016-04-30 7:00 ` John Keeping
2016-04-30 7:00 ` John Keeping
2016-05-02 9:13 ` Enric Balletbo Serra [this message]
2016-05-02 9:13 ` Enric Balletbo Serra
2016-05-02 9:13 ` Enric Balletbo Serra
2016-05-03 4:12 ` sugar
2016-05-03 4:12 ` sugar
2016-05-03 4:12 ` sugar
2016-05-03 9:16 ` [alsa-devel] " Enric Balletbo Serra
2016-05-03 9:16 ` Enric Balletbo Serra
2016-05-03 9:16 ` Enric Balletbo Serra
2016-05-03 9:43 ` [alsa-devel] " John Keeping
2016-05-03 9:43 ` John Keeping
2016-05-03 9:43 ` John Keeping
2016-05-03 10:21 ` Enric Balletbo Serra
2016-05-03 10:21 ` Enric Balletbo Serra
2016-05-03 10:21 ` Enric Balletbo Serra
2016-05-03 10:32 ` John Keeping
2016-05-03 10:32 ` John Keeping
2016-05-03 10:32 ` John Keeping
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAFqH_51sSqaoUtadYux+A38wQ1y9T-GKU3_PEcgT9rp_19_b8g@mail.gmail.com \
--to=eballetbo@gmail.com \
--cc=alsa-devel@alsa-project.org \
--cc=broonie@kernel.org \
--cc=heiko@sntech.de \
--cc=john@keeping.me.uk \
--cc=lgirdwood@gmail.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-rockchip@lists.infradead.org \
--cc=perex@perex.cz \
--cc=tiwai@suse.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.