All of lore.kernel.org
 help / color / mirror / Atom feed
From: Samuel Holland <samuel@sholland.org>
To: "Maxime Ripard" <maxime@cerno.tech>,
	"Clément Péron" <peron.clem@gmail.com>
Cc: Mark Brown <broonie@kernel.org>,
	Liam Girdwood <lgirdwood@gmail.com>,
	Rob Herring <robh+dt@kernel.org>, Chen-Yu Tsai <wens@csie.org>,
	Jaroslav Kysela <perex@perex.cz>, Takashi Iwai <tiwai@suse.com>,
	Linux-ALSA <alsa-devel@alsa-project.org>,
	devicetree <devicetree@vger.kernel.org>,
	linux-arm-kernel <linux-arm-kernel@lists.infradead.org>,
	linux-kernel <linux-kernel@vger.kernel.org>,
	Jernej Skrabec <jernej.skrabec@siol.net>,
	Marcus Cooper <codekipper@gmail.com>
Subject: Re: [PATCH v3 3/7] ASoC: sun4i-i2s: Add support for H6 I2S
Date: Thu, 3 Sep 2020 21:54:39 -0500	[thread overview]
Message-ID: <80b5a4e3-c8bc-9521-4ff1-12bb6424516f@sholland.org> (raw)
In-Reply-To: <20200903205851.gdnpthserywsxrbs@gilmour.lan>

Maxime,

On 9/3/20 3:58 PM, Maxime Ripard wrote:
> On Thu, Sep 03, 2020 at 10:02:31PM +0200, Clément Péron wrote:
>> Hi Maxime,
>>
>> On Wed, 29 Jul 2020 at 17:16, Mark Brown <broonie@kernel.org> wrote:
>>>
>>> On Wed, Jul 29, 2020 at 04:39:27PM +0200, Maxime Ripard wrote:
>>>
>>>> It really looks like the polarity of LRCK is fine though. The first word
>>>> is sent with LRCK low, and then high, so we have channel 0 and then
>>>> channel 1 which seems to be the proper ordering?

Which image file is this in reference to?

>>> Yes, that's normal.
>>
>> Thank you very much for this test.
>>
>> So I will revert the following commit:
>>
>> ASoC: sun4i-i2s: Fix the LRCK polarity
>>
>> https://github.com/clementperon/linux/commit/dd657eae8164f7e4bafe8b875031a7c6c50646a9
> 
> Like I said, the current code is working as expected with regard to the
> LRCK polarity. The issue is that the samples are delayed and start to be
> transmitted on the wrong phase of the signal.

Since an I2S LRCK frame is radially symmetric, "wrong phase" and "inverted
polarity" look the same. The only way to definitively distinguish them is by
looking at the sample data.

In "i2s-h6.png", the samples are all zeroes, so you're assuming that the first
sample transmitted (that is, when the bit clock starts transitioning) was a
"left" sample.

However, in "h6-i2s-start-data.png", there are pairs of samples we can look at.
I'm still assuming that similar samples are a left/right pair, but that's
probably a safe assumption. Here we see the first sample in each pair is
transmitted with LRCK *high*, and the second sample in the pair is transmitted
with LRCK *low*. This is the opposite of your claim above.

An ideal test would put left/right markers and frame numbers in the data
channel. The Python script below can generate such a file. Then you would know
how much startup delay there is, which channel the "first sample" came from, and
how each channel maps to the LRCK level.

It would also be helpful to test DSP_A mode, where the LRCK signal is asymmetric
and an inversion would be obvious.

> But the LRCK polarity is fine.
> 
> Maxime
> 

Samuel

----8<----
import wave
from struct import Struct

markers = (0x2, 0xe)
rate    = 8000
seconds = 10

struct  = Struct('<' + 'H' * len(markers))
nframes = seconds * rate
data    = bytearray(nframes * struct.size)

for i in range(nframes):
    frame  = [(m << 12) + (i % 2**12) for m in markers]
    offset = i * struct.size
    struct.pack_into(data, offset, *frame)

with wave.open('test.wav', 'wb') as wf:
    wf.setparams((len(markers), 2, rate, nframes, 'NONE', ''))
    wf.writeframes(data)

WARNING: multiple messages have this Message-ID (diff)
From: Samuel Holland <samuel@sholland.org>
To: "Maxime Ripard" <maxime@cerno.tech>,
	"Clément Péron" <peron.clem@gmail.com>
Cc: devicetree <devicetree@vger.kernel.org>,
	Linux-ALSA <alsa-devel@alsa-project.org>,
	linux-kernel <linux-kernel@vger.kernel.org>,
	Jernej Skrabec <jernej.skrabec@siol.net>,
	Takashi Iwai <tiwai@suse.com>, Rob Herring <robh+dt@kernel.org>,
	Liam Girdwood <lgirdwood@gmail.com>,
	Marcus Cooper <codekipper@gmail.com>,
	Chen-Yu Tsai <wens@csie.org>, Mark Brown <broonie@kernel.org>,
	linux-arm-kernel <linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH v3 3/7] ASoC: sun4i-i2s: Add support for H6 I2S
Date: Thu, 3 Sep 2020 21:54:39 -0500	[thread overview]
Message-ID: <80b5a4e3-c8bc-9521-4ff1-12bb6424516f@sholland.org> (raw)
In-Reply-To: <20200903205851.gdnpthserywsxrbs@gilmour.lan>

Maxime,

On 9/3/20 3:58 PM, Maxime Ripard wrote:
> On Thu, Sep 03, 2020 at 10:02:31PM +0200, Clément Péron wrote:
>> Hi Maxime,
>>
>> On Wed, 29 Jul 2020 at 17:16, Mark Brown <broonie@kernel.org> wrote:
>>>
>>> On Wed, Jul 29, 2020 at 04:39:27PM +0200, Maxime Ripard wrote:
>>>
>>>> It really looks like the polarity of LRCK is fine though. The first word
>>>> is sent with LRCK low, and then high, so we have channel 0 and then
>>>> channel 1 which seems to be the proper ordering?

Which image file is this in reference to?

>>> Yes, that's normal.
>>
>> Thank you very much for this test.
>>
>> So I will revert the following commit:
>>
>> ASoC: sun4i-i2s: Fix the LRCK polarity
>>
>> https://github.com/clementperon/linux/commit/dd657eae8164f7e4bafe8b875031a7c6c50646a9
> 
> Like I said, the current code is working as expected with regard to the
> LRCK polarity. The issue is that the samples are delayed and start to be
> transmitted on the wrong phase of the signal.

Since an I2S LRCK frame is radially symmetric, "wrong phase" and "inverted
polarity" look the same. The only way to definitively distinguish them is by
looking at the sample data.

In "i2s-h6.png", the samples are all zeroes, so you're assuming that the first
sample transmitted (that is, when the bit clock starts transitioning) was a
"left" sample.

However, in "h6-i2s-start-data.png", there are pairs of samples we can look at.
I'm still assuming that similar samples are a left/right pair, but that's
probably a safe assumption. Here we see the first sample in each pair is
transmitted with LRCK *high*, and the second sample in the pair is transmitted
with LRCK *low*. This is the opposite of your claim above.

An ideal test would put left/right markers and frame numbers in the data
channel. The Python script below can generate such a file. Then you would know
how much startup delay there is, which channel the "first sample" came from, and
how each channel maps to the LRCK level.

It would also be helpful to test DSP_A mode, where the LRCK signal is asymmetric
and an inversion would be obvious.

> But the LRCK polarity is fine.
> 
> Maxime
> 

Samuel

----8<----
import wave
from struct import Struct

markers = (0x2, 0xe)
rate    = 8000
seconds = 10

struct  = Struct('<' + 'H' * len(markers))
nframes = seconds * rate
data    = bytearray(nframes * struct.size)

for i in range(nframes):
    frame  = [(m << 12) + (i % 2**12) for m in markers]
    offset = i * struct.size
    struct.pack_into(data, offset, *frame)

with wave.open('test.wav', 'wb') as wf:
    wf.setparams((len(markers), 2, rate, nframes, 'NONE', ''))
    wf.writeframes(data)

WARNING: multiple messages have this Message-ID (diff)
From: Samuel Holland <samuel@sholland.org>
To: "Maxime Ripard" <maxime@cerno.tech>,
	"Clément Péron" <peron.clem@gmail.com>
Cc: devicetree <devicetree@vger.kernel.org>,
	Linux-ALSA <alsa-devel@alsa-project.org>,
	linux-kernel <linux-kernel@vger.kernel.org>,
	Jernej Skrabec <jernej.skrabec@siol.net>,
	Takashi Iwai <tiwai@suse.com>, Rob Herring <robh+dt@kernel.org>,
	Liam Girdwood <lgirdwood@gmail.com>,
	Marcus Cooper <codekipper@gmail.com>,
	Chen-Yu Tsai <wens@csie.org>, Mark Brown <broonie@kernel.org>,
	Jaroslav Kysela <perex@perex.cz>,
	linux-arm-kernel <linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH v3 3/7] ASoC: sun4i-i2s: Add support for H6 I2S
Date: Thu, 3 Sep 2020 21:54:39 -0500	[thread overview]
Message-ID: <80b5a4e3-c8bc-9521-4ff1-12bb6424516f@sholland.org> (raw)
In-Reply-To: <20200903205851.gdnpthserywsxrbs@gilmour.lan>

Maxime,

On 9/3/20 3:58 PM, Maxime Ripard wrote:
> On Thu, Sep 03, 2020 at 10:02:31PM +0200, Clément Péron wrote:
>> Hi Maxime,
>>
>> On Wed, 29 Jul 2020 at 17:16, Mark Brown <broonie@kernel.org> wrote:
>>>
>>> On Wed, Jul 29, 2020 at 04:39:27PM +0200, Maxime Ripard wrote:
>>>
>>>> It really looks like the polarity of LRCK is fine though. The first word
>>>> is sent with LRCK low, and then high, so we have channel 0 and then
>>>> channel 1 which seems to be the proper ordering?

Which image file is this in reference to?

>>> Yes, that's normal.
>>
>> Thank you very much for this test.
>>
>> So I will revert the following commit:
>>
>> ASoC: sun4i-i2s: Fix the LRCK polarity
>>
>> https://github.com/clementperon/linux/commit/dd657eae8164f7e4bafe8b875031a7c6c50646a9
> 
> Like I said, the current code is working as expected with regard to the
> LRCK polarity. The issue is that the samples are delayed and start to be
> transmitted on the wrong phase of the signal.

Since an I2S LRCK frame is radially symmetric, "wrong phase" and "inverted
polarity" look the same. The only way to definitively distinguish them is by
looking at the sample data.

In "i2s-h6.png", the samples are all zeroes, so you're assuming that the first
sample transmitted (that is, when the bit clock starts transitioning) was a
"left" sample.

However, in "h6-i2s-start-data.png", there are pairs of samples we can look at.
I'm still assuming that similar samples are a left/right pair, but that's
probably a safe assumption. Here we see the first sample in each pair is
transmitted with LRCK *high*, and the second sample in the pair is transmitted
with LRCK *low*. This is the opposite of your claim above.

An ideal test would put left/right markers and frame numbers in the data
channel. The Python script below can generate such a file. Then you would know
how much startup delay there is, which channel the "first sample" came from, and
how each channel maps to the LRCK level.

It would also be helpful to test DSP_A mode, where the LRCK signal is asymmetric
and an inversion would be obvious.

> But the LRCK polarity is fine.
> 
> Maxime
> 

Samuel

----8<----
import wave
from struct import Struct

markers = (0x2, 0xe)
rate    = 8000
seconds = 10

struct  = Struct('<' + 'H' * len(markers))
nframes = seconds * rate
data    = bytearray(nframes * struct.size)

for i in range(nframes):
    frame  = [(m << 12) + (i % 2**12) for m in markers]
    offset = i * struct.size
    struct.pack_into(data, offset, *frame)

with wave.open('test.wav', 'wb') as wf:
    wf.setparams((len(markers), 2, rate, nframes, 'NONE', ''))
    wf.writeframes(data)

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

  reply	other threads:[~2020-09-04  2:54 UTC|newest]

Thread overview: 102+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-04-26 10:41 [PATCH v3 0/7] Add H6 I2S support Clément Péron
2020-04-26 10:41 ` Clément Péron
2020-04-26 10:41 ` Clément Péron
2020-04-26 10:41 ` [PATCH v3 1/7] ASoC: sun4i-i2s: Adjust LRCLK width Clément Péron
2020-04-26 10:41   ` Clément Péron
2020-04-26 10:41   ` Clément Péron
2020-04-28  8:02   ` Maxime Ripard
2020-04-28  8:02     ` Maxime Ripard
2020-04-28  8:02     ` Maxime Ripard
2020-04-26 10:41 ` [PATCH v3 2/7] dt-bindings: ASoC: sun4i-i2s: Add H6 compatible Clément Péron
2020-04-26 10:41   ` Clément Péron
2020-04-26 10:41   ` Clément Péron
2020-04-28  8:08   ` Maxime Ripard
2020-04-28  8:08     ` Maxime Ripard
2020-04-28  8:08     ` Maxime Ripard
2020-05-11 22:26   ` Rob Herring
2020-05-11 22:26     ` Rob Herring
2020-05-11 22:26     ` Rob Herring
2020-04-26 10:41 ` [PATCH v3 3/7] ASoC: sun4i-i2s: Add support for H6 I2S Clément Péron
2020-04-26 10:41   ` Clément Péron
2020-04-26 10:41   ` Clément Péron
2020-04-28  8:13   ` Maxime Ripard
2020-04-28  8:13     ` Maxime Ripard
2020-04-28  8:13     ` Maxime Ripard
2020-04-28  8:55     ` Clément Péron
2020-04-28  8:55       ` Clément Péron
2020-04-28  8:55       ` Clément Péron
2020-04-29 12:35       ` Maxime Ripard
2020-04-29 12:35         ` Maxime Ripard
2020-04-29 12:35         ` Maxime Ripard
2020-04-29 16:33         ` Clément Péron
2020-04-29 16:33           ` Clément Péron
2020-04-29 16:33           ` Clément Péron
2020-04-30  8:46           ` Maxime Ripard
2020-04-30  8:46             ` Maxime Ripard
2020-04-30  8:46             ` Maxime Ripard
2020-04-30 14:00             ` Clément Péron
2020-04-30 14:00               ` Clément Péron
2020-04-30 14:00               ` Clément Péron
2020-05-04 12:09               ` Maxime Ripard
2020-05-04 12:09                 ` Maxime Ripard
2020-05-04 12:09                 ` Maxime Ripard
2020-05-04 19:43                 ` Clément Péron
2020-05-04 19:43                   ` Clément Péron
2020-05-04 19:43                   ` Clément Péron
2020-07-29 14:39                   ` Maxime Ripard
2020-07-29 14:39                     ` Maxime Ripard
2020-07-29 15:15                     ` Mark Brown
2020-07-29 15:15                       ` Mark Brown
2020-07-29 15:15                       ` Mark Brown
2020-09-03 20:02                       ` Clément Péron
2020-09-03 20:02                         ` Clément Péron
2020-09-03 20:02                         ` Clément Péron
2020-09-03 20:58                         ` Maxime Ripard
2020-09-03 20:58                           ` Maxime Ripard
2020-09-03 20:58                           ` Maxime Ripard
2020-09-04  2:54                           ` Samuel Holland [this message]
2020-09-04  2:54                             ` Samuel Holland
2020-09-04  2:54                             ` Samuel Holland
2020-09-10 14:33                             ` Maxime Ripard
2020-09-12 20:29                               ` Samuel Holland
2020-09-12 20:29                                 ` Samuel Holland
2020-09-12 20:29                                 ` Samuel Holland
2020-09-17 13:21                                 ` Maxime Ripard
2020-09-17 13:21                                   ` Maxime Ripard
2020-09-17 13:21                                   ` Maxime Ripard
2020-09-17 13:55                                   ` Clément Péron
2020-09-17 13:55                                     ` Clément Péron
2020-09-17 13:55                                     ` Clément Péron
2020-09-17 14:06                                     ` Maxime Ripard
2020-09-17 14:06                                       ` Maxime Ripard
2020-09-17 14:06                                       ` Maxime Ripard
2020-09-20 12:38                                       ` Clément Péron
2020-09-20 12:38                                         ` Clément Péron
2020-09-20 12:38                                         ` Clément Péron
2020-04-26 10:41 ` [PATCH v3 4/7] ASoC: sun4i-i2s: Set sign extend sample Clément Péron
2020-04-26 10:41   ` Clément Péron
2020-04-26 10:41   ` Clément Péron
2020-04-26 10:41 ` [PATCH v3 5/7] ASoc: sun4i-i2s: Add 20 and 24 bit support Clément Péron
2020-04-26 10:41   ` Clément Péron
2020-04-26 10:41   ` Clément Péron
2020-04-26 10:41 ` [PATCH v3 6/7] ASoC: sun4i-i2s: Adjust regmap settings Clément Péron
2020-04-26 10:41   ` Clément Péron
2020-04-26 10:41   ` Clément Péron
2020-04-27 11:03   ` Chen-Yu Tsai
2020-04-27 11:03     ` Chen-Yu Tsai
2020-04-27 11:03     ` Chen-Yu Tsai
2020-05-03 11:42     ` Clément Péron
2020-05-03 11:42       ` Clément Péron
2020-05-03 11:42       ` Clément Péron
2020-04-26 10:41 ` [PATCH v3 7/7] arm64: dts: sun50i-h6: Add HDMI audio to H6 DTSI Clément Péron
2020-04-26 10:41   ` Clément Péron
2020-04-26 10:41   ` Clément Péron
2020-04-26 11:51   ` Clément Péron
2020-04-26 11:51     ` Clément Péron
2020-04-26 11:51     ` Clément Péron
2020-04-28  8:14   ` Maxime Ripard
2020-04-28  8:14     ` Maxime Ripard
2020-04-28  8:14     ` Maxime Ripard
2020-04-28 14:36     ` Clément Péron
2020-04-28 14:36       ` Clément Péron
2020-04-28 14:36       ` Clément Péron

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=80b5a4e3-c8bc-9521-4ff1-12bb6424516f@sholland.org \
    --to=samuel@sholland.org \
    --cc=alsa-devel@alsa-project.org \
    --cc=broonie@kernel.org \
    --cc=codekipper@gmail.com \
    --cc=devicetree@vger.kernel.org \
    --cc=jernej.skrabec@siol.net \
    --cc=lgirdwood@gmail.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=maxime@cerno.tech \
    --cc=perex@perex.cz \
    --cc=peron.clem@gmail.com \
    --cc=robh+dt@kernel.org \
    --cc=tiwai@suse.com \
    --cc=wens@csie.org \
    /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.