[PATCH] ARM: kirkwood: enable S/PDIF

[PATCH] ARM: kirkwood: enable S/PDIF

[Jean-Francois Moine]

This patch enables S/PDIF.

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

diff --git a/sound/soc/kirkwood/kirkwood-i2s.c b/sound/soc/kirkwood/kirkwood-i2s.c
index 4c9dad3..ad94c50 100644
--- a/sound/soc/kirkwood/kirkwood-i2s.c
+++ b/sound/soc/kirkwood/kirkwood-i2s.c
@@ -159,6 +159,7 @@ static int kirkwood_i2s_hw_params(struct snd_pcm_substream *substream,
 	case SNDRV_PCM_FORMAT_S16_LE:
 		i2s_value |= KIRKWOOD_I2S_CTL_SIZE_16;
 		ctl_play = KIRKWOOD_PLAYCTL_SIZE_16_C |
+			   KIRKWOOD_PLAYCTL_SPDIF_EN |
 			   KIRKWOOD_PLAYCTL_I2S_EN;
 		ctl_rec = KIRKWOOD_RECCTL_SIZE_16_C |
 			  KIRKWOOD_RECCTL_I2S_EN;
@@ -169,6 +170,7 @@ static int kirkwood_i2s_hw_params(struct snd_pcm_substream *substream,
 	case SNDRV_PCM_FORMAT_S20_3LE:
 		i2s_value |= KIRKWOOD_I2S_CTL_SIZE_20;
 		ctl_play = KIRKWOOD_PLAYCTL_SIZE_20 |
+			   KIRKWOOD_PLAYCTL_SPDIF_EN |
 			   KIRKWOOD_PLAYCTL_I2S_EN;
 		ctl_rec = KIRKWOOD_RECCTL_SIZE_20 |
 			  KIRKWOOD_RECCTL_I2S_EN;
@@ -177,6 +179,7 @@ static int kirkwood_i2s_hw_params(struct snd_pcm_substream *substream,
 	case SNDRV_PCM_FORMAT_S24_LE:
 		i2s_value |= KIRKWOOD_I2S_CTL_SIZE_24;
 		ctl_play = KIRKWOOD_PLAYCTL_SIZE_24 |
+			   KIRKWOOD_PLAYCTL_SPDIF_EN |
 			   KIRKWOOD_PLAYCTL_I2S_EN;
 		ctl_rec = KIRKWOOD_RECCTL_SIZE_24 |
 			  KIRKWOOD_RECCTL_I2S_EN;

-- 
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

Re: [PATCH] ARM: kirkwood: enable S/PDIF

[Russell King - ARM Linux]

On Tue, Jul 23, 2013 at 10:23:20AM +0200, Jean-Francois Moine wrote:
> This patch enables S/PDIF.
> 
> Signed-off-by: Jean-Francois Moine <moinejf@free.fr>

I'm not submitting my patch to do this because:

(a) we don't know what effect this has on other hardware.
(b) Mark suggested that we use the slave PCM stuff to deal with this.  As
    yet, that's something which I haven't been able to get to grips with
    because ASoC is soo damned complicated and undocumented.

Re: [PATCH] ARM: kirkwood: enable S/PDIF

[Russell King - ARM Linux]

On Tue, Jul 23, 2013 at 10:23:20AM +0200, Jean-Francois Moine wrote:
> This patch enables S/PDIF.
> 
> Signed-off-by: Jean-Francois Moine <moinejf@free.fr>

I'm not submitting my patch to do this because:

(a) we don't know what effect this has on other hardware.
(b) Mark suggested that we use the slave PCM stuff to deal with this.  As
    yet, that's something which I haven't been able to get to grips with
    because ASoC is soo damned complicated and undocumented.

Re: [PATCH] ARM: kirkwood: enable S/PDIF

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 569 bytes --]

On Tue, Jul 23, 2013 at 09:43:47AM +0100, Russell King - ARM Linux wrote:
> On Tue, Jul 23, 2013 at 10:23:20AM +0200, Jean-Francois Moine wrote:
> > This patch enables S/PDIF.

> > Signed-off-by: Jean-Francois Moine <moinejf@free.fr>

> I'm not submitting my patch to do this because:

> (a) we don't know what effect this has on other hardware.

This patch will do absolutely nothing unless it's used in a machine
driver which connects a S/PDIF CODEC to it.  I see no reason not to
apply it, someone with hardware with more complex needs can always build
on it later.

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

Re: [PATCH] ARM: kirkwood: enable S/PDIF

[Mark Brown]

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

On Tue, Jul 23, 2013 at 09:43:47AM +0100, Russell King - ARM Linux wrote:
> On Tue, Jul 23, 2013 at 10:23:20AM +0200, Jean-Francois Moine wrote:
> > This patch enables S/PDIF.

> > Signed-off-by: Jean-Francois Moine <moinejf@free.fr>

> I'm not submitting my patch to do this because:

> (a) we don't know what effect this has on other hardware.

This patch will do absolutely nothing unless it's used in a machine
driver which connects a S/PDIF CODEC to it.  I see no reason not to
apply it, someone with hardware with more complex needs can always build
on it later.

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

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

Re: [alsa-devel] [PATCH] ARM: kirkwood: enable S/PDIF

[Sebastian Hesselbarth]

On 07/23/13 15:06, Mark Brown wrote:
> On Tue, Jul 23, 2013 at 09:43:47AM +0100, Russell King - ARM Linux wrote:
>> On Tue, Jul 23, 2013 at 10:23:20AM +0200, Jean-Francois Moine wrote:
>>> This patch enables S/PDIF.
>
>>> Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
>
>> I'm not submitting my patch to do this because:
>
>> (a) we don't know what effect this has on other hardware.
>
> This patch will do absolutely nothing unless it's used in a machine
> driver which connects a S/PDIF CODEC to it.  I see no reason not to
> apply it, someone with hardware with more complex needs can always build
> on it later.
>

Mark,

the mask that is changed in the patch is what will be written
into i2s controller's registers. So, if there is no S/PDIF in that 
specific controller that bit can possibly have a different meaning.
Also, enabling both I2S playback and SPDIF playback can cause the
controller to behave differently.

I share Russell's concern about it and would rather like to use
multiple codecs per DAI (DPCM) for that. I see Daniel Mack picked
that up again, maybe he submits something soon.

Sebastian

Re: [PATCH] ARM: kirkwood: enable S/PDIF

[Sebastian Hesselbarth]

On 07/23/13 15:06, Mark Brown wrote:
> On Tue, Jul 23, 2013 at 09:43:47AM +0100, Russell King - ARM Linux wrote:
>> On Tue, Jul 23, 2013 at 10:23:20AM +0200, Jean-Francois Moine wrote:
>>> This patch enables S/PDIF.
>
>>> Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
>
>> I'm not submitting my patch to do this because:
>
>> (a) we don't know what effect this has on other hardware.
>
> This patch will do absolutely nothing unless it's used in a machine
> driver which connects a S/PDIF CODEC to it.  I see no reason not to
> apply it, someone with hardware with more complex needs can always build
> on it later.
>

Mark,

the mask that is changed in the patch is what will be written
into i2s controller's registers. So, if there is no S/PDIF in that 
specific controller that bit can possibly have a different meaning.
Also, enabling both I2S playback and SPDIF playback can cause the
controller to behave differently.

I share Russell's concern about it and would rather like to use
multiple codecs per DAI (DPCM) for that. I see Daniel Mack picked
that up again, maybe he submits something soon.

Sebastian

Re: [PATCH] ARM: kirkwood: enable S/PDIF

[Russell King - ARM Linux]

On Tue, Jul 23, 2013 at 02:06:22PM +0100, Mark Brown wrote:
> On Tue, Jul 23, 2013 at 09:43:47AM +0100, Russell King - ARM Linux wrote:
> > On Tue, Jul 23, 2013 at 10:23:20AM +0200, Jean-Francois Moine wrote:
> > > This patch enables S/PDIF.
> 
> > > Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
> 
> > I'm not submitting my patch to do this because:
> 
> > (a) we don't know what effect this has on other hardware.
> 
> This patch will do absolutely nothing unless it's used in a machine
> driver which connects a S/PDIF CODEC to it.  I see no reason not to
> apply it, someone with hardware with more complex needs can always build
> on it later.

So... what if setting this bit causes the SoC to start wiggling a pin
with the SPDIF signal which has been used for a different purpose?

Re: [PATCH] ARM: kirkwood: enable S/PDIF

[Russell King - ARM Linux]

On Tue, Jul 23, 2013 at 02:06:22PM +0100, Mark Brown wrote:
> On Tue, Jul 23, 2013 at 09:43:47AM +0100, Russell King - ARM Linux wrote:
> > On Tue, Jul 23, 2013 at 10:23:20AM +0200, Jean-Francois Moine wrote:
> > > This patch enables S/PDIF.
> 
> > > Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
> 
> > I'm not submitting my patch to do this because:
> 
> > (a) we don't know what effect this has on other hardware.
> 
> This patch will do absolutely nothing unless it's used in a machine
> driver which connects a S/PDIF CODEC to it.  I see no reason not to
> apply it, someone with hardware with more complex needs can always build
> on it later.

So... what if setting this bit causes the SoC to start wiggling a pin
with the SPDIF signal which has been used for a different purpose?

Re: [alsa-devel] [PATCH] ARM: kirkwood: enable S/PDIF

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 873 bytes --]

On Tue, Jul 23, 2013 at 03:12:56PM +0200, Sebastian Hesselbarth wrote:

> the mask that is changed in the patch is what will be written
> into i2s controller's registers. So, if there is no S/PDIF in that
> specific controller that bit can possibly have a different meaning.
> Also, enabling both I2S playback and SPDIF playback can cause the
> controller to behave differently.

Oh, so it will - I glanced through it and misread, sorry.  If it just
makes the enabling of S/PDIF mode conditional on DAI format that'd cover
it.

> I share Russell's concern about it and would rather like to use
> multiple codecs per DAI (DPCM) for that. I see Daniel Mack picked
> that up again, maybe he submits something soon.

Well, that'd be ideal and is going to be needed for any hardware which
has both wired up in parallel but a simpler either/or thing doesn't seem
like a problem.

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

Re: [PATCH] ARM: kirkwood: enable S/PDIF

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 627 bytes --]

On Tue, Jul 23, 2013 at 02:21:04PM +0100, Russell King - ARM Linux wrote:
> On Tue, Jul 23, 2013 at 02:06:22PM +0100, Mark Brown wrote:

> > This patch will do absolutely nothing unless it's used in a machine
> > driver which connects a S/PDIF CODEC to it.  I see no reason not to
> > apply it, someone with hardware with more complex needs can always build
> > on it later.

> So... what if setting this bit causes the SoC to start wiggling a pin
> with the SPDIF signal which has been used for a different purpose?

Right, yup - didn't read it fully.  Like I say doing it conditional on
the DAI format should be fine though.

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

Re: [alsa-devel] [PATCH] ARM: kirkwood: enable S/PDIF

[Russell King - ARM Linux]

On Tue, Jul 23, 2013 at 03:12:56PM +0200, Sebastian Hesselbarth wrote:
> On 07/23/13 15:06, Mark Brown wrote:
>> On Tue, Jul 23, 2013 at 09:43:47AM +0100, Russell King - ARM Linux wrote:
>>> On Tue, Jul 23, 2013 at 10:23:20AM +0200, Jean-Francois Moine wrote:
>>>> This patch enables S/PDIF.
>>
>>>> Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
>>
>>> I'm not submitting my patch to do this because:
>>
>>> (a) we don't know what effect this has on other hardware.
>>
>> This patch will do absolutely nothing unless it's used in a machine
>> driver which connects a S/PDIF CODEC to it.  I see no reason not to
>> apply it, someone with hardware with more complex needs can always build
>> on it later.
>>
>
> Mark,
>
> the mask that is changed in the patch is what will be written
> into i2s controller's registers. So, if there is no S/PDIF in that  
> specific controller that bit can possibly have a different meaning.
> Also, enabling both I2S playback and SPDIF playback can cause the
> controller to behave differently.

Right, so the SPDIF output on Dove isn't multiplexed, so I was wrong on
that _for_ _dove_, but that may not necessarily be the case for the
Kirkwood SoCs - the pin may be multiplexed there.

However, Dove does have two I2S/SPDIF controllers which are otherwise
identical, apart from one having SPDIF support and the other not.
Setting the SPDIF enable bit on the one without is unspecified what
the behaviour would be, so it should not be set.

Re: [PATCH] ARM: kirkwood: enable S/PDIF

[Russell King - ARM Linux]

On Tue, Jul 23, 2013 at 03:12:56PM +0200, Sebastian Hesselbarth wrote:
> On 07/23/13 15:06, Mark Brown wrote:
>> On Tue, Jul 23, 2013 at 09:43:47AM +0100, Russell King - ARM Linux wrote:
>>> On Tue, Jul 23, 2013 at 10:23:20AM +0200, Jean-Francois Moine wrote:
>>>> This patch enables S/PDIF.
>>
>>>> Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
>>
>>> I'm not submitting my patch to do this because:
>>
>>> (a) we don't know what effect this has on other hardware.
>>
>> This patch will do absolutely nothing unless it's used in a machine
>> driver which connects a S/PDIF CODEC to it.  I see no reason not to
>> apply it, someone with hardware with more complex needs can always build
>> on it later.
>>
>
> Mark,
>
> the mask that is changed in the patch is what will be written
> into i2s controller's registers. So, if there is no S/PDIF in that  
> specific controller that bit can possibly have a different meaning.
> Also, enabling both I2S playback and SPDIF playback can cause the
> controller to behave differently.

Right, so the SPDIF output on Dove isn't multiplexed, so I was wrong on
that _for_ _dove_, but that may not necessarily be the case for the
Kirkwood SoCs - the pin may be multiplexed there.

However, Dove does have two I2S/SPDIF controllers which are otherwise
identical, apart from one having SPDIF support and the other not.
Setting the SPDIF enable bit on the one without is unspecified what
the behaviour would be, so it should not be set.