[PATCH] ASoC: kirkwood: Fix invalid SPDIF format

[PATCH] ASoC: kirkwood: Fix invalid SPDIF format

[Jean-Francois Moine]

This patch removes the 32 bits format which is not supported by S/PDIF
output.

Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
---
 sound/soc/kirkwood/kirkwood-i2s.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/sound/soc/kirkwood/kirkwood-i2s.c b/sound/soc/kirkwood/kirkwood-i2s.c
index b27f826..0b18f65 100644
--- a/sound/soc/kirkwood/kirkwood-i2s.c
+++ b/sound/soc/kirkwood/kirkwood-i2s.c
@@ -33,6 +33,10 @@
 	 SNDRV_PCM_FMTBIT_S24_LE | \
 	 SNDRV_PCM_FMTBIT_S32_LE)
 
+#define KIRKWOOD_SPDIF_FORMATS \
+	(SNDRV_PCM_FMTBIT_S16_LE | \
+	 SNDRV_PCM_FMTBIT_S24_LE)
+
 static int kirkwood_i2s_set_fmt(struct snd_soc_dai *cpu_dai,
 		unsigned int fmt)
 {
@@ -449,14 +453,14 @@ static struct snd_soc_dai_driver kirkwood_i2s_dai[2] = {
 		.channels_max = 2,
 		.rates = SNDRV_PCM_RATE_44100 | SNDRV_PCM_RATE_48000 |
 				SNDRV_PCM_RATE_96000,
-		.formats = KIRKWOOD_I2S_FORMATS,
+		.formats = KIRKWOOD_SPDIF_FORMATS,
 	},
 	.capture = {
 		.channels_min = 1,
 		.channels_max = 2,
 		.rates = SNDRV_PCM_RATE_44100 | SNDRV_PCM_RATE_48000 |
 				SNDRV_PCM_RATE_96000,
-		.formats = KIRKWOOD_I2S_FORMATS,
+		.formats = KIRKWOOD_SPDIF_FORMATS,
 	},
 	.ops = &kirkwood_i2s_dai_ops,
     },
@@ -493,7 +497,7 @@ static struct snd_soc_dai_driver kirkwood_i2s_dai_extclk[2] = {
 		.rates = SNDRV_PCM_RATE_8000_192000 |
 			 SNDRV_PCM_RATE_CONTINUOUS |
 			 SNDRV_PCM_RATE_KNOT,
-		.formats = KIRKWOOD_I2S_FORMATS,
+		.formats = KIRKWOOD_SPDIF_FORMATS,
 	},
 	.capture = {
 		.channels_min = 1,
@@ -501,7 +505,7 @@ static struct snd_soc_dai_driver kirkwood_i2s_dai_extclk[2] = {
 		.rates = SNDRV_PCM_RATE_8000_192000 |
 			 SNDRV_PCM_RATE_CONTINUOUS |
 			 SNDRV_PCM_RATE_KNOT,
-		.formats = KIRKWOOD_I2S_FORMATS,
+		.formats = KIRKWOOD_SPDIF_FORMATS,
 	},
 	.ops = &kirkwood_i2s_dai_ops,
     },

-- 
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] ASoC: kirkwood: Fix invalid SPDIF format

[Takashi Iwai]

At Tue, 26 Nov 2013 10:41:40 +0100,
Jean-Francois Moine wrote:
> 
> This patch removes the 32 bits format which is not supported by S/PDIF
> output.
> 
> Signed-off-by: Jean-Francois Moine <moinejf@free.fr>

Not against the patch itself, but just found looking through it:

> @@ -493,7 +497,7 @@ static struct snd_soc_dai_driver kirkwood_i2s_dai_extclk[2] = {
>  		.rates = SNDRV_PCM_RATE_8000_192000 |
>  			 SNDRV_PCM_RATE_CONTINUOUS |
>  			 SNDRV_PCM_RATE_KNOT,

Setting both CONTINUOUS and KNOT doesn't make sense.


Takashi

Re: KNOT / CONTINUOUS (was: [PATCH] ASoC: kirkwood: Fix invalid SPDIF format)

[Jean-Francois Moine]

On Tue, 26 Nov 2013 11:20:48 +0100
Takashi Iwai <tiwai@suse.de> wrote:

> Not against the patch itself, but just found looking through it:
> 
> > @@ -493,7 +497,7 @@ static struct snd_soc_dai_driver kirkwood_i2s_dai_extclk[2] = {
> >  		.rates = SNDRV_PCM_RATE_8000_192000 |
> >  			 SNDRV_PCM_RATE_CONTINUOUS |
> >  			 SNDRV_PCM_RATE_KNOT,
> 
> Setting both CONTINUOUS and KNOT doesn't make sense.

Hi Takashi,

I understand 'continuous', but I could not find any clear definition of
'knot'. May you explain what is its purpose?

BTW, if you may help me, while looking for SNDRV_PCM_RATE_KNOT,
I found this sequence in soc-pcm.c:

	if (codec_stream->rates
		& (SNDRV_PCM_RATE_KNOT | SNDRV_PCM_RATE_CONTINUOUS))
		hw->rates |= cpu_stream->rates;
	if (cpu_stream->rates
		& (SNDRV_PCM_RATE_KNOT | SNDRV_PCM_RATE_CONTINUOUS))
		hw->rates |= codec_stream->rates;

and it seems related to the problem I have:

- the cpu dai (kirkwood i2s) may generate any continuous rate
  8000..192000 as shown in the patch.

- the HDMI transmitter (tda998x) accepts either i2s or s/pdif audio
  input at any continuous rate, but when getting audio from s/pdif, the
  lowest rate is 22.06kHz.

In the associated codec, if I define:

	.rates = SNDRV_PCM_RATE_22050 |
		SNDRV_PCM_RATE_32000 |
		...
		SNDRV_PCM_RATE_192000 |
		SNDRV_PCM_RATE_CONTINUOUS,

audio works fine with streams at 33.075kHz (the kirkwood clock is
exactly 33.075kHz). But when I want a stream at 7850Hz, the kirkwood
i2s driver gets a clock at 8000Hz and the tda998x cannot do audio
output.

Otherwise, removing SNDRV_PCM_RATE_CONTINUOUS in the codec, I can hear
the 7850Hz stream which is converted to 22.05 kHz by vlc, but when I
want a stream at 33.075kHz, vlc converts it to 32kHz.

So, with the commit d9ad6296ec3b4a55b "ASoC: PCM_RATE: Check for KNOT
and CONTINUOUS flags", I cannot get the exact clock I want.

Do you know the reason of this patch, and, if it must stay, is there
any possible bypass?

Thanks.

-- 
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: KNOT / CONTINUOUS (was: [PATCH] ASoC: kirkwood: Fix invalid SPDIF format)

[Russell King - ARM Linux]

On Tue, Nov 26, 2013 at 08:09:16PM +0100, Jean-Francois Moine wrote:
> Otherwise, removing SNDRV_PCM_RATE_CONTINUOUS in the codec, I can hear
> the 7850Hz stream which is converted to 22.05 kHz by vlc, but when I
> want a stream at 33.075kHz, vlc converts it to 32kHz.

What is your fascination with doing stuff outside the specifications?
The HDMI spec is quite clear:

"An HDMI Source is permitted to transmit L-PCM audio data at sample rates
of 32kHz, 44.1kHz, 48kHz, 88.2kHz, 96kHz, 176.4kHz or 192kHz."

It goes on to say:

"An HDMI Sink may accept L-PCM audio at sample rates of 32kHz, 44.1kHz,
48kHz, 88.2kHz, 96kHz, 176.4kHz or 192kHz, and should indicate these
capabilities in the E-EDID data structure."

That doesn't say that 8kHz, 22.05kHz etc are valid sample rates for
HDMI.  It may work for some devices but it's non-standard.

Re: KNOT / CONTINUOUS

[Lars-Peter Clausen]

On 11/26/2013 08:09 PM, Jean-Francois Moine wrote:
> On Tue, 26 Nov 2013 11:20:48 +0100
> Takashi Iwai <tiwai@suse.de> wrote:
> 
>> Not against the patch itself, but just found looking through it:
>>
>>> @@ -493,7 +497,7 @@ static struct snd_soc_dai_driver kirkwood_i2s_dai_extclk[2] = {
>>>  		.rates = SNDRV_PCM_RATE_8000_192000 |
>>>  			 SNDRV_PCM_RATE_CONTINUOUS |
>>>  			 SNDRV_PCM_RATE_KNOT,
>>
>> Setting both CONTINUOUS and KNOT doesn't make sense.
> 
> Hi Takashi,
> 
> I understand 'continuous', but I could not find any clear definition of
> 'knot'. May you explain what is its purpose?

CONTINUOUS means that any rate between the specified min and max is fine, if
no min or max is specified any rate is fine. KNOT means there are rates
supported other than the standard rates defines by ALSA, but the other rates
are enumerable. You'd typically specify them by explicitly listing them all
and use a list constraint or you'd use one of the ratio constraints.

> 
> BTW, if you may help me, while looking for SNDRV_PCM_RATE_KNOT,
> I found this sequence in soc-pcm.c:
> 
> 	if (codec_stream->rates
> 		& (SNDRV_PCM_RATE_KNOT | SNDRV_PCM_RATE_CONTINUOUS))
> 		hw->rates |= cpu_stream->rates;
> 	if (cpu_stream->rates
> 		& (SNDRV_PCM_RATE_KNOT | SNDRV_PCM_RATE_CONTINUOUS))
> 		hw->rates |= codec_stream->rates;

I think this code is wrong, but I don't think it is related to your problem.

> 
> and it seems related to the problem I have:
> 
> - the cpu dai (kirkwood i2s) may generate any continuous rate
>   8000..192000 as shown in the patch.
> 

If you set SNDRV_PCM_RATE_CONTINUOUS, the other rates flags will be ignored
(IIRC). You need to explicitly specify the minimum and maximum rates if they
exist.

> - the HDMI transmitter (tda998x) accepts either i2s or s/pdif audio
>   input at any continuous rate, but when getting audio from s/pdif, the
>   lowest rate is 22.06kHz.
> 
> In the associated codec, if I define:
> 
> 	.rates = SNDRV_PCM_RATE_22050 |
> 		SNDRV_PCM_RATE_32000 |
> 		...
> 		SNDRV_PCM_RATE_192000 |
> 		SNDRV_PCM_RATE_CONTINUOUS,
> 
> audio works fine with streams at 33.075kHz (the kirkwood clock is
> exactly 33.075kHz). But when I want a stream at 7850Hz, the kirkwood
> i2s driver gets a clock at 8000Hz and the tda998x cannot do audio
> output.
> 
> Otherwise, removing SNDRV_PCM_RATE_CONTINUOUS in the codec, I can hear
> the 7850Hz stream which is converted to 22.05 kHz by vlc, but when I
> want a stream at 33.075kHz, vlc converts it to 32kHz.
> 
> So, with the commit d9ad6296ec3b4a55b "ASoC: PCM_RATE: Check for KNOT
> and CONTINUOUS flags", I cannot get the exact clock I want.
> 
> Do you know the reason of this patch, and, if it must stay, is there
> any possible bypass?

In your codec driver you should specify the minimum and maximum supported
rate by calling snd_pcm_hw_constraint_minmax() in the DAI startup callback.

- Lars

Re: KNOT / CONTINUOUS

[Lars-Peter Clausen]

[...]
> In your codec driver you should specify the minimum and maximum supported
> rate by calling snd_pcm_hw_constraint_minmax() in the DAI startup callback.

It is also possible to set the min and max rate directly in the dai_driver
struct.

Re: KNOT / CONTINUOUS

[Takashi Iwai]

At Tue, 26 Nov 2013 20:37:00 +0100,
Lars-Peter Clausen wrote:
> 
> On 11/26/2013 08:09 PM, Jean-Francois Moine wrote:
> > On Tue, 26 Nov 2013 11:20:48 +0100
> > Takashi Iwai <tiwai@suse.de> wrote:
> > 
> >> Not against the patch itself, but just found looking through it:
> >>
> >>> @@ -493,7 +497,7 @@ static struct snd_soc_dai_driver kirkwood_i2s_dai_extclk[2] = {
> >>>  		.rates = SNDRV_PCM_RATE_8000_192000 |
> >>>  			 SNDRV_PCM_RATE_CONTINUOUS |
> >>>  			 SNDRV_PCM_RATE_KNOT,
> >>
> >> Setting both CONTINUOUS and KNOT doesn't make sense.
> > 
> > Hi Takashi,
> > 
> > I understand 'continuous', but I could not find any clear definition of
> > 'knot'. May you explain what is its purpose?
> 
> CONTINUOUS means that any rate between the specified min and max is fine, if
> no min or max is specified any rate is fine. KNOT means there are rates
> supported other than the standard rates defines by ALSA, but the other rates
> are enumerable. You'd typically specify them by explicitly listing them all
> and use a list constraint or you'd use one of the ratio constraints.
> 
> > 
> > BTW, if you may help me, while looking for SNDRV_PCM_RATE_KNOT,
> > I found this sequence in soc-pcm.c:
> > 
> > 	if (codec_stream->rates
> > 		& (SNDRV_PCM_RATE_KNOT | SNDRV_PCM_RATE_CONTINUOUS))
> > 		hw->rates |= cpu_stream->rates;
> > 	if (cpu_stream->rates
> > 		& (SNDRV_PCM_RATE_KNOT | SNDRV_PCM_RATE_CONTINUOUS))
> > 		hw->rates |= codec_stream->rates;
> 
> I think this code is wrong, but I don't think it is related to your problem.

It's not completely wrong, but imperfect, indeed.

Essentially, the rates bits are set by a line above the code snippet:
	hw->rates = codec_stream->rates & cpu_stream->rates;

And, the code above handles exceptional rules for CONTINUOUS and
KNOT.  It would work if either codec or cpu DAI has KNOT or CONTINUOUS
flag and another doesn't but only explicit rate bits, because
CONTINUOUS does allow any rates and KNOW may allow any rates.
(So, basically you need no other SNDRV_PCM_RATE_*bits if you have
CONTINUOUS or KNOT bit.)  And the actual rates are limited by rate_min
and rate_max values.

It won't work properly, however, if one side has KNOT and another has
CONTINUOUS.  Then it'd end up with CONTINUOUS|KNOT, which is wrong.

Instead, the following should work (if I counted all possible cases
properly):

	hw->rates = codec_stream->rates & cpu_stream->rates;
	if (codec_stream->rates
		& (SNDRV_PCM_RATE_KNOT | SNDRV_PCM_RATE_CONTINUOUS))
		hw->rates |= cpu_stream->rates & ~SNDRV_PCM_RATE_CONTINUOUS;
	if (cpu_stream->rates
		& (SNDRV_PCM_RATE_KNOT | SNDRV_PCM_RATE_CONTINUOUS))
		hw->rates |= codec_stream->rates & ~SNDRV_PCM_RATE_CONTINUOUS;

The code looks tricky, so a proper comment should be added there.


Takashi

Re: [PATCH] ASoC: kirkwood: Fix invalid SPDIF format

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 170 bytes --]

On Tue, Nov 26, 2013 at 10:41:40AM +0100, Jean-Francois Moine wrote:
> This patch removes the 32 bits format which is not supported by S/PDIF
> output.

Applied, thanks.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]