[PATCH 0/5] ASoC: atmel: extend SSC support

[PATCH 0/5] ASoC: atmel: extend SSC support

[Michał Mirosław]

This series improves support for various configurations using SSC module
as implemented in Atmel SAMA5Dx SoCs. Patches are:

1. enable SSC in Kconfig for audio-graph-card support
2-3. implement left-justified data mode
4-5. fix shared FSYNC source for slave mode

Patches against tiwai/sound/for-next tree. You can also pull from
   https://rere.qmqm.pl/git/linux
branch:
   atmel-ssc

Michał Mirosław (5):
  ASoC: atmel: enable SSC_PCM_DMA in Kconfig
  ASoC: atmel_ssc_dai: rework DAI format configuration
  ASoC: atmel_ssc_dai: implement left-justified data mode
  ASoC: atmel_ssc_dai: split TX/RX FS constants
  ASoC: atmel_ssc_dai: Enable shared FSYNC source in frame-slave mode

 .../devicetree/bindings/misc/atmel-ssc.txt    |   4 +
 drivers/misc/atmel-ssc.c                      |   2 +
 include/linux/atmel-ssc.h                     |   1 +
 sound/soc/atmel/Kconfig                       |   2 +-
 sound/soc/atmel/atmel_ssc_dai.c               | 306 ++++++------------
 sound/soc/atmel/atmel_ssc_dai.h               |   9 +-
 6 files changed, 120 insertions(+), 204 deletions(-)

-- 
2.20.1


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

[PATCH 1/5] ASoC: atmel: enable SSC_PCM_DMA in Kconfig

[Michał Mirosław]

Allow SSC to be used on platforms described using audio-graph-card
in Device Tree.

Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
 sound/soc/atmel/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/soc/atmel/Kconfig b/sound/soc/atmel/Kconfig
index 06c1d5ce642c..9ef9d25bb517 100644
--- a/sound/soc/atmel/Kconfig
+++ b/sound/soc/atmel/Kconfig
@@ -25,7 +25,7 @@ config SND_ATMEL_SOC_DMA
 	default y if SND_ATMEL_SOC_SSC_DMA=y || (SND_ATMEL_SOC_SSC_DMA=m && SND_ATMEL_SOC_SSC=y)
 
 config SND_ATMEL_SOC_SSC_DMA
-	tristate
+	tristate "SoC PCM DAI support for AT91 SSC controller using DMA"
 
 config SND_ATMEL_SOC_SSC
 	tristate
-- 
2.20.1


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

Re: [PATCH 1/5] ASoC: atmel: enable SSC_PCM_DMA in Kconfig

[Codrin.Ciubotariu]

On 22.07.2019 21:27, Michał Mirosław wrote:
> Allow SSC to be used on platforms described using audio-graph-card
> in Device Tree.
> 
> Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> ---
>   sound/soc/atmel/Kconfig | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/sound/soc/atmel/Kconfig b/sound/soc/atmel/Kconfig
> index 06c1d5ce642c..9ef9d25bb517 100644
> --- a/sound/soc/atmel/Kconfig
> +++ b/sound/soc/atmel/Kconfig
> @@ -25,7 +25,7 @@ config SND_ATMEL_SOC_DMA
>   	default y if SND_ATMEL_SOC_SSC_DMA=y || (SND_ATMEL_SOC_SSC_DMA=m && SND_ATMEL_SOC_SSC=y)
>   
>   config SND_ATMEL_SOC_SSC_DMA
> -	tristate
> +	tristate "SoC PCM DAI support for AT91 SSC controller using DMA"

Could you please make something similar for SND_ATMEL_SOC_SSC_PDC? Also, 
I think that it should select ATMEL_SSC, to be able to use 
simple/graph-card with SSC.

Thanks and best regards,
Codrin

>   
>   config SND_ATMEL_SOC_SSC
>   	tristate
> 

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

Re: [PATCH 1/5] ASoC: atmel: enable SSC_PCM_DMA in Kconfig

[Codrin.Ciubotariu]

On 23.07.2019 16:36, Codrin.Ciubotariu@microchip.com wrote:
> On 22.07.2019 21:27, Michał Mirosław wrote:
>> Allow SSC to be used on platforms described using audio-graph-card
>> in Device Tree.
>>
>> Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
>> ---
>>    sound/soc/atmel/Kconfig | 2 +-
>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/sound/soc/atmel/Kconfig b/sound/soc/atmel/Kconfig
>> index 06c1d5ce642c..9ef9d25bb517 100644
>> --- a/sound/soc/atmel/Kconfig
>> +++ b/sound/soc/atmel/Kconfig
>> @@ -25,7 +25,7 @@ config SND_ATMEL_SOC_DMA
>>    	default y if SND_ATMEL_SOC_SSC_DMA=y || (SND_ATMEL_SOC_SSC_DMA=m && SND_ATMEL_SOC_SSC=y)
>>    
>>    config SND_ATMEL_SOC_SSC_DMA
>> -	tristate
>> +	tristate "SoC PCM DAI support for AT91 SSC controller using DMA"
> 
> Could you please make something similar for SND_ATMEL_SOC_SSC_PDC? Also,
> I think that it should select ATMEL_SSC, to be able to use
> simple/graph-card with SSC.

It looks like 'select' creates a recursive dependency with audio machine 
drivers, so try 'depends on' instead.

> 
> Thanks and best regards,
> Codrin
> 
>>    
>>    config SND_ATMEL_SOC_SSC
>>    	tristate
>>
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

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

Re: [PATCH 1/5] ASoC: atmel: enable SSC_PCM_DMA in Kconfig

[mirq-linux]

On Tue, Jul 23, 2019 at 01:36:37PM +0000, Codrin.Ciubotariu@microchip.com wrote:
> On 22.07.2019 21:27, Michał Mirosław wrote:
> > Allow SSC to be used on platforms described using audio-graph-card
> > in Device Tree.
> > 
> > Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> > ---
> >   sound/soc/atmel/Kconfig | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/sound/soc/atmel/Kconfig b/sound/soc/atmel/Kconfig
> > index 06c1d5ce642c..9ef9d25bb517 100644
> > --- a/sound/soc/atmel/Kconfig
> > +++ b/sound/soc/atmel/Kconfig
> > @@ -25,7 +25,7 @@ config SND_ATMEL_SOC_DMA
> >   	default y if SND_ATMEL_SOC_SSC_DMA=y || (SND_ATMEL_SOC_SSC_DMA=m && SND_ATMEL_SOC_SSC=y)
> >   
> >   config SND_ATMEL_SOC_SSC_DMA
> > -	tristate
> > +	tristate "SoC PCM DAI support for AT91 SSC controller using DMA"
> 
> Could you please make something similar for SND_ATMEL_SOC_SSC_PDC? Also, 
> I think that it should select ATMEL_SSC, to be able to use 
> simple/graph-card with SSC.

Hmm. The Kconfig dependencies seems overly complicated, do you mind if I
get rid of most of the entries in the process?

Best Regards,
Michał Mirosław

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

Re: [PATCH 1/5] ASoC: atmel: enable SSC_PCM_DMA in Kconfig

[Codrin.Ciubotariu]

On 23.07.2019 19:43, mirq-linux@rere.qmqm.pl wrote:
> External E-Mail
> 
> 
> On Tue, Jul 23, 2019 at 01:36:37PM +0000, Codrin.Ciubotariu@microchip.com wrote:
>> On 22.07.2019 21:27, Michał Mirosław wrote:
>>> Allow SSC to be used on platforms described using audio-graph-card
>>> in Device Tree.
>>>
>>> Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
>>> ---
>>>    sound/soc/atmel/Kconfig | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/sound/soc/atmel/Kconfig b/sound/soc/atmel/Kconfig
>>> index 06c1d5ce642c..9ef9d25bb517 100644
>>> --- a/sound/soc/atmel/Kconfig
>>> +++ b/sound/soc/atmel/Kconfig
>>> @@ -25,7 +25,7 @@ config SND_ATMEL_SOC_DMA
>>>    	default y if SND_ATMEL_SOC_SSC_DMA=y || (SND_ATMEL_SOC_SSC_DMA=m && SND_ATMEL_SOC_SSC=y)
>>>    
>>>    config SND_ATMEL_SOC_SSC_DMA
>>> -	tristate
>>> +	tristate "SoC PCM DAI support for AT91 SSC controller using DMA"
>>
>> Could you please make something similar for SND_ATMEL_SOC_SSC_PDC? Also,
>> I think that it should select ATMEL_SSC, to be able to use
>> simple/graph-card with SSC.
> 
> Hmm. The Kconfig dependencies seems overly complicated, do you mind if I
> get rid of most of the entries in the process?
> 
> Best Regards,
> Michał Mirosław
> 

'select' creates recursive dependencies with our machine
drivers, but 'depends on' should work. The complication comes from the 
fact that PDC and DMA support for SSC can be both enabled at the same 
time, so I think we need all of them...

I am reviewing the rest of your patches, so bear with me please.

Best regards,
Codrin

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

Re: [PATCH 1/5] ASoC: atmel: enable SSC_PCM_DMA in Kconfig

[Alexandre Belloni]

On 23/07/2019 18:43:12+0200, mirq-linux@rere.qmqm.pl wrote:
> On Tue, Jul 23, 2019 at 01:36:37PM +0000, Codrin.Ciubotariu@microchip.com wrote:
> > On 22.07.2019 21:27, Michał Mirosław wrote:
> > > Allow SSC to be used on platforms described using audio-graph-card
> > > in Device Tree.
> > > 
> > > Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> > > ---
> > >   sound/soc/atmel/Kconfig | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/sound/soc/atmel/Kconfig b/sound/soc/atmel/Kconfig
> > > index 06c1d5ce642c..9ef9d25bb517 100644
> > > --- a/sound/soc/atmel/Kconfig
> > > +++ b/sound/soc/atmel/Kconfig
> > > @@ -25,7 +25,7 @@ config SND_ATMEL_SOC_DMA
> > >   	default y if SND_ATMEL_SOC_SSC_DMA=y || (SND_ATMEL_SOC_SSC_DMA=m && SND_ATMEL_SOC_SSC=y)
> > >   
> > >   config SND_ATMEL_SOC_SSC_DMA
> > > -	tristate
> > > +	tristate "SoC PCM DAI support for AT91 SSC controller using DMA"
> > 
> > Could you please make something similar for SND_ATMEL_SOC_SSC_PDC? Also, 
> > I think that it should select ATMEL_SSC, to be able to use 
> > simple/graph-card with SSC.
> 
> Hmm. The Kconfig dependencies seems overly complicated, do you mind if I
> get rid of most of the entries in the process?
> 

Unfortunately, it is just complicated enough. This is done to support
all the possible configurations. Removing them will lead to linking
errors.

After having that discussion back in March, I had a very quick look and
didn't send a patch because I still had linking issues. It is not
impossible but it required more time than I had.

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

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

Re: [PATCH 1/5] ASoC: atmel: enable SSC_PCM_DMA in Kconfig

[mirq-linux]

On Tue, Jul 23, 2019 at 08:39:15PM +0200, Alexandre Belloni wrote:
> On 23/07/2019 18:43:12+0200, mirq-linux@rere.qmqm.pl wrote:
> > On Tue, Jul 23, 2019 at 01:36:37PM +0000, Codrin.Ciubotariu@microchip.com wrote:
> > > On 22.07.2019 21:27, Michał Mirosław wrote:
> > > > Allow SSC to be used on platforms described using audio-graph-card
> > > > in Device Tree.
> > > > 
> > > > Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> > > > ---
> > > >   sound/soc/atmel/Kconfig | 2 +-
> > > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/sound/soc/atmel/Kconfig b/sound/soc/atmel/Kconfig
> > > > index 06c1d5ce642c..9ef9d25bb517 100644
> > > > --- a/sound/soc/atmel/Kconfig
> > > > +++ b/sound/soc/atmel/Kconfig
> > > > @@ -25,7 +25,7 @@ config SND_ATMEL_SOC_DMA
> > > >   	default y if SND_ATMEL_SOC_SSC_DMA=y || (SND_ATMEL_SOC_SSC_DMA=m && SND_ATMEL_SOC_SSC=y)
> > > >   
> > > >   config SND_ATMEL_SOC_SSC_DMA
> > > > -	tristate
> > > > +	tristate "SoC PCM DAI support for AT91 SSC controller using DMA"
> > > 
> > > Could you please make something similar for SND_ATMEL_SOC_SSC_PDC? Also, 
> > > I think that it should select ATMEL_SSC, to be able to use 
> > > simple/graph-card with SSC.
> > 
> > Hmm. The Kconfig dependencies seems overly complicated, do you mind if I
> > get rid of most of the entries in the process?
> > 
> 
> Unfortunately, it is just complicated enough. This is done to support
> all the possible configurations. Removing them will lead to linking
> errors.
> 
> After having that discussion back in March, I had a very quick look and
> didn't send a patch because I still had linking issues. It is not
> impossible but it required more time than I had.

Can you try patch below if it covers the configurations you mention?
This uses Kconfig's m/y resolution instead of open-coded defaults.

Best Regards,
Michał Mirosław


diff --git a/sound/soc/atmel/Kconfig b/sound/soc/atmel/Kconfig
index 06c1d5ce642c..f118c229ed82 100644
--- a/sound/soc/atmel/Kconfig
+++ b/sound/soc/atmel/Kconfig
@@ -12,25 +12,31 @@ if SND_ATMEL_SOC
 config SND_ATMEL_SOC_PDC
 	tristate
 	depends on HAS_DMA
-	default m if SND_ATMEL_SOC_SSC_PDC=m && SND_ATMEL_SOC_SSC=m
-	default y if SND_ATMEL_SOC_SSC_PDC=y || (SND_ATMEL_SOC_SSC_PDC=m && SND_ATMEL_SOC_SSC=y)
-
-config SND_ATMEL_SOC_SSC_PDC
-	tristate
 
 config SND_ATMEL_SOC_DMA
 	tristate
 	select SND_SOC_GENERIC_DMAENGINE_PCM
-	default m if SND_ATMEL_SOC_SSC_DMA=m && SND_ATMEL_SOC_SSC=m
-	default y if SND_ATMEL_SOC_SSC_DMA=y || (SND_ATMEL_SOC_SSC_DMA=m && SND_ATMEL_SOC_SSC=y)
-
-config SND_ATMEL_SOC_SSC_DMA
-	tristate
 
 config SND_ATMEL_SOC_SSC
 	tristate
-	default y if SND_ATMEL_SOC_SSC_DMA=y || SND_ATMEL_SOC_SSC_PDC=y
-	default m if SND_ATMEL_SOC_SSC_DMA=m || SND_ATMEL_SOC_SSC_PDC=m
+
+config SND_ATMEL_SOC_SSC_PDC
+	tristate "SoC PCM DAI support for AT91 SSC controller using PDC"
+	depends on ATMEL_SSC
+	select SND_ATMEL_SOC_PDC
+	select SND_ATMEL_SOC_SSC
+	help
+	  Say Y or M if you want to add support for Atmel SSC interface
+	  in PDC mode configured using audio-graph-card in device-tree.
+
+config SND_ATMEL_SOC_SSC_DMA
+	tristate "SoC PCM DAI support for AT91 SSC controller using DMA"
+	depends on ATMEL_SSC
+	select SND_ATMEL_SOC_DMA
+	select SND_ATMEL_SOC_SSC
+	help
+	  Say Y or M if you want to add support for Atmel SSC interface
+	  in DMA mode configured using audio-graph-card in device-tree.
 
 config SND_AT91_SOC_SAM9G20_WM8731
 	tristate "SoC Audio support for WM8731-based At91sam9g20 evaluation board"
-- 
2.20.1


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

Re: [PATCH 1/5] ASoC: atmel: enable SSC_PCM_DMA in Kconfig

[Codrin.Ciubotariu]

On 24.07.2019 02:25, mirq-linux@rere.qmqm.pl wrote:
> External E-Mail
> 
> 
> On Tue, Jul 23, 2019 at 08:39:15PM +0200, Alexandre Belloni wrote:
>> On 23/07/2019 18:43:12+0200, mirq-linux@rere.qmqm.pl wrote:
>>> On Tue, Jul 23, 2019 at 01:36:37PM +0000, Codrin.Ciubotariu@microchip.com wrote:
>>>> On 22.07.2019 21:27, Michał Mirosław wrote:
>>>>> Allow SSC to be used on platforms described using audio-graph-card
>>>>> in Device Tree.
>>>>>
>>>>> Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
>>>>> ---
>>>>>    sound/soc/atmel/Kconfig | 2 +-
>>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/sound/soc/atmel/Kconfig b/sound/soc/atmel/Kconfig
>>>>> index 06c1d5ce642c..9ef9d25bb517 100644
>>>>> --- a/sound/soc/atmel/Kconfig
>>>>> +++ b/sound/soc/atmel/Kconfig
>>>>> @@ -25,7 +25,7 @@ config SND_ATMEL_SOC_DMA
>>>>>    	default y if SND_ATMEL_SOC_SSC_DMA=y || (SND_ATMEL_SOC_SSC_DMA=m && SND_ATMEL_SOC_SSC=y)
>>>>>    
>>>>>    config SND_ATMEL_SOC_SSC_DMA
>>>>> -	tristate
>>>>> +	tristate "SoC PCM DAI support for AT91 SSC controller using DMA"
>>>>
>>>> Could you please make something similar for SND_ATMEL_SOC_SSC_PDC? Also,
>>>> I think that it should select ATMEL_SSC, to be able to use
>>>> simple/graph-card with SSC.
>>>
>>> Hmm. The Kconfig dependencies seems overly complicated, do you mind if I
>>> get rid of most of the entries in the process?
>>>
>>
>> Unfortunately, it is just complicated enough. This is done to support
>> all the possible configurations. Removing them will lead to linking
>> errors.
>>
>> After having that discussion back in March, I had a very quick look and
>> didn't send a patch because I still had linking issues. It is not
>> impossible but it required more time than I had.
> 
> Can you try patch below if it covers the configurations you mention?
> This uses Kconfig's m/y resolution instead of open-coded defaults.
> 
> Best Regards,
> Michał Mirosław
> 
> 
> diff --git a/sound/soc/atmel/Kconfig b/sound/soc/atmel/Kconfig
> index 06c1d5ce642c..f118c229ed82 100644
> --- a/sound/soc/atmel/Kconfig
> +++ b/sound/soc/atmel/Kconfig
> @@ -12,25 +12,31 @@ if SND_ATMEL_SOC
>   config SND_ATMEL_SOC_PDC
>   	tristate
>   	depends on HAS_DMA
> -	default m if SND_ATMEL_SOC_SSC_PDC=m && SND_ATMEL_SOC_SSC=m
> -	default y if SND_ATMEL_SOC_SSC_PDC=y || (SND_ATMEL_SOC_SSC_PDC=m && SND_ATMEL_SOC_SSC=y)
> -
> -config SND_ATMEL_SOC_SSC_PDC
> -	tristate
>   
>   config SND_ATMEL_SOC_DMA
>   	tristate
>   	select SND_SOC_GENERIC_DMAENGINE_PCM
> -	default m if SND_ATMEL_SOC_SSC_DMA=m && SND_ATMEL_SOC_SSC=m
> -	default y if SND_ATMEL_SOC_SSC_DMA=y || (SND_ATMEL_SOC_SSC_DMA=m && SND_ATMEL_SOC_SSC=y)
> -
> -config SND_ATMEL_SOC_SSC_DMA
> -	tristate
>   
>   config SND_ATMEL_SOC_SSC
>   	tristate
> -	default y if SND_ATMEL_SOC_SSC_DMA=y || SND_ATMEL_SOC_SSC_PDC=y
> -	default m if SND_ATMEL_SOC_SSC_DMA=m || SND_ATMEL_SOC_SSC_PDC=m
> +
> +config SND_ATMEL_SOC_SSC_PDC
> +	tristate "SoC PCM DAI support for AT91 SSC controller using PDC"
> +	depends on ATMEL_SSC
> +	select SND_ATMEL_SOC_PDC
> +	select SND_ATMEL_SOC_SSC
> +	help
> +	  Say Y or M if you want to add support for Atmel SSC interface
> +	  in PDC mode configured using audio-graph-card in device-tree.
> +
> +config SND_ATMEL_SOC_SSC_DMA
> +	tristate "SoC PCM DAI support for AT91 SSC controller using DMA"
> +	depends on ATMEL_SSC
> +	select SND_ATMEL_SOC_DMA
> +	select SND_ATMEL_SOC_SSC
> +	help
> +	  Say Y or M if you want to add support for Atmel SSC interface
> +	  in DMA mode configured using audio-graph-card in device-tree.
>   
>   config SND_AT91_SOC_SAM9G20_WM8731
>   	tristate "SoC Audio support for WM8731-based At91sam9g20 evaluation board"
>

Looks fine to me.

Best regards,
Codrin


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

Re: [PATCH 1/5] ASoC: atmel: enable SSC_PCM_DMA in Kconfig

[Alexandre Belloni]

On 24/07/2019 01:25:05+0200, mirq-linux@rere.qmqm.pl wrote:
> On Tue, Jul 23, 2019 at 08:39:15PM +0200, Alexandre Belloni wrote:
> > On 23/07/2019 18:43:12+0200, mirq-linux@rere.qmqm.pl wrote:
> > > On Tue, Jul 23, 2019 at 01:36:37PM +0000, Codrin.Ciubotariu@microchip.com wrote:
> > > > On 22.07.2019 21:27, Michał Mirosław wrote:
> > > > > Allow SSC to be used on platforms described using audio-graph-card
> > > > > in Device Tree.
> > > > > 
> > > > > Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> > > > > ---
> > > > >   sound/soc/atmel/Kconfig | 2 +-
> > > > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/sound/soc/atmel/Kconfig b/sound/soc/atmel/Kconfig
> > > > > index 06c1d5ce642c..9ef9d25bb517 100644
> > > > > --- a/sound/soc/atmel/Kconfig
> > > > > +++ b/sound/soc/atmel/Kconfig
> > > > > @@ -25,7 +25,7 @@ config SND_ATMEL_SOC_DMA
> > > > >   	default y if SND_ATMEL_SOC_SSC_DMA=y || (SND_ATMEL_SOC_SSC_DMA=m && SND_ATMEL_SOC_SSC=y)
> > > > >   
> > > > >   config SND_ATMEL_SOC_SSC_DMA
> > > > > -	tristate
> > > > > +	tristate "SoC PCM DAI support for AT91 SSC controller using DMA"
> > > > 
> > > > Could you please make something similar for SND_ATMEL_SOC_SSC_PDC? Also, 
> > > > I think that it should select ATMEL_SSC, to be able to use 
> > > > simple/graph-card with SSC.
> > > 
> > > Hmm. The Kconfig dependencies seems overly complicated, do you mind if I
> > > get rid of most of the entries in the process?
> > > 
> > 
> > Unfortunately, it is just complicated enough. This is done to support
> > all the possible configurations. Removing them will lead to linking
> > errors.
> > 
> > After having that discussion back in March, I had a very quick look and
> > didn't send a patch because I still had linking issues. It is not
> > impossible but it required more time than I had.
> 
> Can you try patch below if it covers the configurations you mention?
> This uses Kconfig's m/y resolution instead of open-coded defaults.
> 

Seems good to me, thanks.


-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

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

Re: [PATCH 1/5] ASoC: atmel: enable SSC_PCM_DMA in Kconfig

[Mark Brown]

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

On Mon, Jul 22, 2019 at 08:27:20PM +0200, Michał Mirosław wrote:

>  config SND_ATMEL_SOC_SSC_DMA
> -	tristate
> +	tristate "SoC PCM DAI support for AT91 SSC controller using DMA"

This breaks the build for me:

ld: sound/soc/atmel/atmel_ssc_dai.o: in function `atmel_ssc_set_audio':
atmel_ssc_dai.c:(.text+0xbd9): undefined reference to `ssc_request'
ld: sound/soc/atmel/atmel_ssc_dai.o: in function `atmel_ssc_put_audio':
atmel_ssc_dai.c:(.text+0xc74): undefined reference to `ssc_free'

It's not selecting the SSC comon code.  It also looks like it'd be
sensible to add a dependency on the platform (or at least architecture),
with a || COMPILE_TEST to help with build coverage.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

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

[PATCH 2/5] ASoC: atmel_ssc_dai: rework DAI format configuration

[Michał Mirosław]

Rework DAI format calculation in preparation for adding more formats
later.

Note: this changes FSEDGE to POSITIVE for I2S CBM_CFS mode as the TXSYN
interrupt is not used anyway.

Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
 sound/soc/atmel/atmel_ssc_dai.c | 283 +++++++++-----------------------
 1 file changed, 79 insertions(+), 204 deletions(-)

diff --git a/sound/soc/atmel/atmel_ssc_dai.c b/sound/soc/atmel/atmel_ssc_dai.c
index 6f89483ac88c..b2992496e52f 100644
--- a/sound/soc/atmel/atmel_ssc_dai.c
+++ b/sound/soc/atmel/atmel_ssc_dai.c
@@ -471,7 +471,7 @@ static int atmel_ssc_hw_params(struct snd_pcm_substream *substream,
 	int dir, channels, bits;
 	u32 tfmr, rfmr, tcmr, rcmr;
 	int ret;
-	int fslen, fslen_ext;
+	int fslen, fslen_ext, fs_osync;
 	u32 cmr_div;
 	u32 tcmr_period;
 	u32 rcmr_period;
@@ -558,226 +558,40 @@ static int atmel_ssc_hw_params(struct snd_pcm_substream *substream,
 	/*
 	 * Compute SSC register settings.
 	 */
-	switch (ssc_p->daifmt
-		& (SND_SOC_DAIFMT_FORMAT_MASK | SND_SOC_DAIFMT_MASTER_MASK)) {
 
-	case SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_CBS_CFS:
-		/*
-		 * I2S format, SSC provides BCLK and LRC clocks.
-		 *
-		 * The SSC transmit and receive clocks are generated
-		 * from the MCK divider, and the BCLK signal
-		 * is output on the SSC TK line.
-		 */
-
-		if (bits > 16 && !ssc->pdata->has_fslen_ext) {
-			dev_err(dai->dev,
-				"sample size %d is too large for SSC device\n",
-				bits);
-			return -EINVAL;
-		}
-
-		fslen_ext = (bits - 1) / 16;
-		fslen = (bits - 1) % 16;
-
-		rcmr =	  SSC_BF(RCMR_PERIOD, rcmr_period)
-			| SSC_BF(RCMR_STTDLY, START_DELAY)
-			| SSC_BF(RCMR_START, SSC_START_FALLING_RF)
-			| SSC_BF(RCMR_CKI, SSC_CKI_RISING)
-			| SSC_BF(RCMR_CKO, SSC_CKO_NONE)
-			| SSC_BF(RCMR_CKS, SSC_CKS_DIV);
-
-		rfmr =    SSC_BF(RFMR_FSLEN_EXT, fslen_ext)
-			| SSC_BF(RFMR_FSEDGE, SSC_FSEDGE_POSITIVE)
-			| SSC_BF(RFMR_FSOS, SSC_FSOS_NEGATIVE)
-			| SSC_BF(RFMR_FSLEN, fslen)
-			| SSC_BF(RFMR_DATNB, (channels - 1))
-			| SSC_BIT(RFMR_MSBF)
-			| SSC_BF(RFMR_LOOP, 0)
-			| SSC_BF(RFMR_DATLEN, (bits - 1));
-
-		tcmr =	  SSC_BF(TCMR_PERIOD, tcmr_period)
-			| SSC_BF(TCMR_STTDLY, START_DELAY)
-			| SSC_BF(TCMR_START, SSC_START_FALLING_RF)
-			| SSC_BF(TCMR_CKI, SSC_CKI_FALLING)
-			| SSC_BF(TCMR_CKO, SSC_CKO_CONTINUOUS)
-			| SSC_BF(TCMR_CKS, SSC_CKS_DIV);
-
-		tfmr =    SSC_BF(TFMR_FSLEN_EXT, fslen_ext)
-			| SSC_BF(TFMR_FSEDGE, SSC_FSEDGE_POSITIVE)
-			| SSC_BF(TFMR_FSDEN, 0)
-			| SSC_BF(TFMR_FSOS, SSC_FSOS_NEGATIVE)
-			| SSC_BF(TFMR_FSLEN, fslen)
-			| SSC_BF(TFMR_DATNB, (channels - 1))
-			| SSC_BIT(TFMR_MSBF)
-			| SSC_BF(TFMR_DATDEF, 0)
-			| SSC_BF(TFMR_DATLEN, (bits - 1));
-		break;
-
-	case SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_CBM_CFM:
-		/* I2S format, CODEC supplies BCLK and LRC clocks. */
-		rcmr =	  SSC_BF(RCMR_PERIOD, 0)
-			| SSC_BF(RCMR_STTDLY, START_DELAY)
-			| SSC_BF(RCMR_START, SSC_START_FALLING_RF)
-			| SSC_BF(RCMR_CKI, SSC_CKI_RISING)
-			| SSC_BF(RCMR_CKO, SSC_CKO_NONE)
-			| SSC_BF(RCMR_CKS, ssc->clk_from_rk_pin ?
-					   SSC_CKS_PIN : SSC_CKS_CLOCK);
-
-		rfmr =	  SSC_BF(RFMR_FSEDGE, SSC_FSEDGE_POSITIVE)
-			| SSC_BF(RFMR_FSOS, SSC_FSOS_NONE)
-			| SSC_BF(RFMR_FSLEN, 0)
-			| SSC_BF(RFMR_DATNB, (channels - 1))
-			| SSC_BIT(RFMR_MSBF)
-			| SSC_BF(RFMR_LOOP, 0)
-			| SSC_BF(RFMR_DATLEN, (bits - 1));
+	fslen_ext = (bits - 1) / 16;
+	fslen = (bits - 1) % 16;
 
-		tcmr =	  SSC_BF(TCMR_PERIOD, 0)
-			| SSC_BF(TCMR_STTDLY, START_DELAY)
-			| SSC_BF(TCMR_START, SSC_START_FALLING_RF)
-			| SSC_BF(TCMR_CKI, SSC_CKI_FALLING)
-			| SSC_BF(TCMR_CKO, SSC_CKO_NONE)
-			| SSC_BF(TCMR_CKS, ssc->clk_from_rk_pin ?
-					   SSC_CKS_CLOCK : SSC_CKS_PIN);
+	switch (ssc_p->daifmt & SND_SOC_DAIFMT_FORMAT_MASK) {
 
-		tfmr =	  SSC_BF(TFMR_FSEDGE, SSC_FSEDGE_POSITIVE)
-			| SSC_BF(TFMR_FSDEN, 0)
-			| SSC_BF(TFMR_FSOS, SSC_FSOS_NONE)
-			| SSC_BF(TFMR_FSLEN, 0)
-			| SSC_BF(TFMR_DATNB, (channels - 1))
-			| SSC_BIT(TFMR_MSBF)
-			| SSC_BF(TFMR_DATDEF, 0)
-			| SSC_BF(TFMR_DATLEN, (bits - 1));
-		break;
-
-	case SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_CBM_CFS:
-		/* I2S format, CODEC supplies BCLK, SSC supplies LRCLK. */
-		if (bits > 16 && !ssc->pdata->has_fslen_ext) {
-			dev_err(dai->dev,
-				"sample size %d is too large for SSC device\n",
-				bits);
-			return -EINVAL;
-		}
-
-		fslen_ext = (bits - 1) / 16;
-		fslen = (bits - 1) % 16;
-
-		rcmr =	  SSC_BF(RCMR_PERIOD, rcmr_period)
-			| SSC_BF(RCMR_STTDLY, START_DELAY)
-			| SSC_BF(RCMR_START, SSC_START_FALLING_RF)
-			| SSC_BF(RCMR_CKI, SSC_CKI_RISING)
-			| SSC_BF(RCMR_CKO, SSC_CKO_NONE)
-			| SSC_BF(RCMR_CKS, ssc->clk_from_rk_pin ?
-					   SSC_CKS_PIN : SSC_CKS_CLOCK);
-
-		rfmr =    SSC_BF(RFMR_FSLEN_EXT, fslen_ext)
-			| SSC_BF(RFMR_FSEDGE, SSC_FSEDGE_POSITIVE)
-			| SSC_BF(RFMR_FSOS, SSC_FSOS_NEGATIVE)
-			| SSC_BF(RFMR_FSLEN, fslen)
-			| SSC_BF(RFMR_DATNB, (channels - 1))
-			| SSC_BIT(RFMR_MSBF)
-			| SSC_BF(RFMR_LOOP, 0)
-			| SSC_BF(RFMR_DATLEN, (bits - 1));
-
-		tcmr =	  SSC_BF(TCMR_PERIOD, tcmr_period)
-			| SSC_BF(TCMR_STTDLY, START_DELAY)
-			| SSC_BF(TCMR_START, SSC_START_FALLING_RF)
-			| SSC_BF(TCMR_CKI, SSC_CKI_FALLING)
-			| SSC_BF(TCMR_CKO, SSC_CKO_NONE)
-			| SSC_BF(TCMR_CKS, ssc->clk_from_rk_pin ?
-					   SSC_CKS_CLOCK : SSC_CKS_PIN);
-
-		tfmr =    SSC_BF(TFMR_FSLEN_EXT, fslen_ext)
-			| SSC_BF(TFMR_FSEDGE, SSC_FSEDGE_NEGATIVE)
-			| SSC_BF(TFMR_FSDEN, 0)
-			| SSC_BF(TFMR_FSOS, SSC_FSOS_NEGATIVE)
-			| SSC_BF(TFMR_FSLEN, fslen)
-			| SSC_BF(TFMR_DATNB, (channels - 1))
-			| SSC_BIT(TFMR_MSBF)
-			| SSC_BF(TFMR_DATDEF, 0)
-			| SSC_BF(TFMR_DATLEN, (bits - 1));
-		break;
-
-	case SND_SOC_DAIFMT_DSP_A | SND_SOC_DAIFMT_CBS_CFS:
-		/*
-		 * DSP/PCM Mode A format, SSC provides BCLK and LRC clocks.
-		 *
-		 * The SSC transmit and receive clocks are generated from the
-		 * MCK divider, and the BCLK signal is output
-		 * on the SSC TK line.
-		 */
-		rcmr =	  SSC_BF(RCMR_PERIOD, rcmr_period)
-			| SSC_BF(RCMR_STTDLY, 1)
-			| SSC_BF(RCMR_START, SSC_START_RISING_RF)
-			| SSC_BF(RCMR_CKI, SSC_CKI_RISING)
-			| SSC_BF(RCMR_CKO, SSC_CKO_NONE)
-			| SSC_BF(RCMR_CKS, SSC_CKS_DIV);
+	case SND_SOC_DAIFMT_I2S:
+		fs_osync = SSC_FSOS_NEGATIVE;
 
-		rfmr =	  SSC_BF(RFMR_FSEDGE, SSC_FSEDGE_POSITIVE)
-			| SSC_BF(RFMR_FSOS, SSC_FSOS_POSITIVE)
-			| SSC_BF(RFMR_FSLEN, 0)
-			| SSC_BF(RFMR_DATNB, (channels - 1))
-			| SSC_BIT(RFMR_MSBF)
-			| SSC_BF(RFMR_LOOP, 0)
-			| SSC_BF(RFMR_DATLEN, (bits - 1));
+		rcmr =	  SSC_BF(RCMR_STTDLY, 1)
+			| SSC_BF(RCMR_START, SSC_START_FALLING_RF);
 
-		tcmr =	  SSC_BF(TCMR_PERIOD, tcmr_period)
-			| SSC_BF(TCMR_STTDLY, 1)
-			| SSC_BF(TCMR_START, SSC_START_RISING_RF)
-			| SSC_BF(TCMR_CKI, SSC_CKI_FALLING)
-			| SSC_BF(TCMR_CKO, SSC_CKO_CONTINUOUS)
-			| SSC_BF(TCMR_CKS, SSC_CKS_DIV);
+		tcmr =	  SSC_BF(TCMR_STTDLY, 1)
+			| SSC_BF(TCMR_START, SSC_START_FALLING_RF);
 
-		tfmr =	  SSC_BF(TFMR_FSEDGE, SSC_FSEDGE_POSITIVE)
-			| SSC_BF(TFMR_FSDEN, 0)
-			| SSC_BF(TFMR_FSOS, SSC_FSOS_POSITIVE)
-			| SSC_BF(TFMR_FSLEN, 0)
-			| SSC_BF(TFMR_DATNB, (channels - 1))
-			| SSC_BIT(TFMR_MSBF)
-			| SSC_BF(TFMR_DATDEF, 0)
-			| SSC_BF(TFMR_DATLEN, (bits - 1));
 		break;
 
-	case SND_SOC_DAIFMT_DSP_A | SND_SOC_DAIFMT_CBM_CFM:
+	case SND_SOC_DAIFMT_DSP_A:
 		/*
-		 * DSP/PCM Mode A format, CODEC supplies BCLK and LRC clocks.
+		 * DSP/PCM Mode A format
 		 *
 		 * Data is transferred on first BCLK after LRC pulse rising
 		 * edge.If stereo, the right channel data is contiguous with
 		 * the left channel data.
 		 */
-		rcmr =	  SSC_BF(RCMR_PERIOD, 0)
-			| SSC_BF(RCMR_STTDLY, START_DELAY)
-			| SSC_BF(RCMR_START, SSC_START_RISING_RF)
-			| SSC_BF(RCMR_CKI, SSC_CKI_RISING)
-			| SSC_BF(RCMR_CKO, SSC_CKO_NONE)
-			| SSC_BF(RCMR_CKS, ssc->clk_from_rk_pin ?
-					   SSC_CKS_PIN : SSC_CKS_CLOCK);
+		fs_osync = SSC_FSOS_POSITIVE;
+		fslen = fslen_ext = 0;
 
-		rfmr =	  SSC_BF(RFMR_FSEDGE, SSC_FSEDGE_POSITIVE)
-			| SSC_BF(RFMR_FSOS, SSC_FSOS_NONE)
-			| SSC_BF(RFMR_FSLEN, 0)
-			| SSC_BF(RFMR_DATNB, (channels - 1))
-			| SSC_BIT(RFMR_MSBF)
-			| SSC_BF(RFMR_LOOP, 0)
-			| SSC_BF(RFMR_DATLEN, (bits - 1));
+		rcmr =	  SSC_BF(RCMR_STTDLY, 1)
+			| SSC_BF(RCMR_START, SSC_START_RISING_RF);
 
-		tcmr =	  SSC_BF(TCMR_PERIOD, 0)
-			| SSC_BF(TCMR_STTDLY, START_DELAY)
-			| SSC_BF(TCMR_START, SSC_START_RISING_RF)
-			| SSC_BF(TCMR_CKI, SSC_CKI_FALLING)
-			| SSC_BF(TCMR_CKO, SSC_CKO_NONE)
-			| SSC_BF(RCMR_CKS, ssc->clk_from_rk_pin ?
-					   SSC_CKS_CLOCK : SSC_CKS_PIN);
+		tcmr =	  SSC_BF(TCMR_STTDLY, 1)
+			| SSC_BF(TCMR_START, SSC_START_RISING_RF);
 
-		tfmr =	  SSC_BF(TFMR_FSEDGE, SSC_FSEDGE_POSITIVE)
-			| SSC_BF(TFMR_FSDEN, 0)
-			| SSC_BF(TFMR_FSOS, SSC_FSOS_NONE)
-			| SSC_BF(TFMR_FSLEN, 0)
-			| SSC_BF(TFMR_DATNB, (channels - 1))
-			| SSC_BIT(TFMR_MSBF)
-			| SSC_BF(TFMR_DATDEF, 0)
-			| SSC_BF(TFMR_DATLEN, (bits - 1));
 		break;
 
 	default:
@@ -785,6 +599,67 @@ static int atmel_ssc_hw_params(struct snd_pcm_substream *substream,
 			ssc_p->daifmt);
 		return -EINVAL;
 	}
+
+	if (!atmel_ssc_cfs(ssc_p)) {
+		fslen = fslen_ext = 0;
+		rcmr_period = tcmr_period = 0;
+		fs_osync = SSC_FSOS_NONE;
+	}
+
+	if (atmel_ssc_cbs(ssc_p)) {
+		/*
+		 * SSC provides BCLK
+		 *
+		 * The SSC transmit and receive clocks are generated from the
+		 * MCK divider, and the BCLK signal is output
+		 * on the SSC TK line.
+		 */
+		rcmr |=	  SSC_BF(RCMR_CKS, SSC_CKS_DIV)
+			| SSC_BF(RCMR_CKO, SSC_CKO_NONE);
+
+		tcmr |=	  SSC_BF(TCMR_CKS, SSC_CKS_DIV)
+			| SSC_BF(TCMR_CKO, SSC_CKO_CONTINUOUS);
+	} else {
+		rcmr |=	  SSC_BF(RCMR_CKS, ssc->clk_from_rk_pin ?
+					SSC_CKS_PIN : SSC_CKS_CLOCK)
+			| SSC_BF(RCMR_CKO, SSC_CKO_NONE);
+
+		tcmr |=	  SSC_BF(TCMR_CKS, ssc->clk_from_rk_pin ?
+					SSC_CKS_CLOCK : SSC_CKS_PIN)
+			| SSC_BF(TCMR_CKO, SSC_CKO_NONE);
+	}
+
+	rcmr |=	  SSC_BF(RCMR_PERIOD, rcmr_period)
+		| SSC_BF(RCMR_CKI, SSC_CKI_RISING);
+
+	tcmr |=   SSC_BF(TCMR_PERIOD, tcmr_period)
+		| SSC_BF(TCMR_CKI, SSC_CKI_FALLING);
+
+	rfmr =    SSC_BF(RFMR_FSLEN_EXT, fslen_ext)
+		| SSC_BF(RFMR_FSEDGE, SSC_FSEDGE_POSITIVE)
+		| SSC_BF(RFMR_FSOS, fs_osync)
+		| SSC_BF(RFMR_FSLEN, fslen)
+		| SSC_BF(RFMR_DATNB, (channels - 1))
+		| SSC_BIT(RFMR_MSBF)
+		| SSC_BF(RFMR_LOOP, 0)
+		| SSC_BF(RFMR_DATLEN, (bits - 1));
+
+	tfmr =    SSC_BF(TFMR_FSLEN_EXT, fslen_ext)
+		| SSC_BF(TFMR_FSEDGE, SSC_FSEDGE_POSITIVE)
+		| SSC_BF(TFMR_FSDEN, 0)
+		| SSC_BF(TFMR_FSOS, fs_osync)
+		| SSC_BF(TFMR_FSLEN, fslen)
+		| SSC_BF(TFMR_DATNB, (channels - 1))
+		| SSC_BIT(TFMR_MSBF)
+		| SSC_BF(TFMR_DATDEF, 0)
+		| SSC_BF(TFMR_DATLEN, (bits - 1));
+
+	if (fslen_ext && !ssc->pdata->has_fslen_ext) {
+		dev_err(dai->dev, "sample size %d is too large for SSC device\n",
+			bits);
+		return -EINVAL;
+	}
+
 	pr_debug("atmel_ssc_hw_params: "
 			"RCMR=%08x RFMR=%08x TCMR=%08x TFMR=%08x\n",
 			rcmr, rfmr, tcmr, tfmr);
-- 
2.20.1


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

Re: [PATCH 2/5] ASoC: atmel_ssc_dai: rework DAI format configuration

[Codrin.Ciubotariu]

On 22.07.2019 21:27, Michał Mirosław wrote:
> Rework DAI format calculation in preparation for adding more formats
> later.
> 
> Note: this changes FSEDGE to POSITIVE for I2S CBM_CFS mode as the TXSYN
> interrupt is not used anyway.
> 
> Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> ---
>   sound/soc/atmel/atmel_ssc_dai.c | 283 +++++++++-----------------------
>   1 file changed, 79 insertions(+), 204 deletions(-)
> 
> diff --git a/sound/soc/atmel/atmel_ssc_dai.c b/sound/soc/atmel/atmel_ssc_dai.c
> index 6f89483ac88c..b2992496e52f 100644
> --- a/sound/soc/atmel/atmel_ssc_dai.c
> +++ b/sound/soc/atmel/atmel_ssc_dai.c
> @@ -471,7 +471,7 @@ static int atmel_ssc_hw_params(struct snd_pcm_substream *substream,
>   	int dir, channels, bits;
>   	u32 tfmr, rfmr, tcmr, rcmr;
>   	int ret;
> -	int fslen, fslen_ext;
> +	int fslen, fslen_ext, fs_osync;
>   	u32 cmr_div;
>   	u32 tcmr_period;
>   	u32 rcmr_period;
> @@ -558,226 +558,40 @@ static int atmel_ssc_hw_params(struct snd_pcm_substream *substream,
>   	/*
>   	 * Compute SSC register settings.
>   	 */
> -	switch (ssc_p->daifmt
> -		& (SND_SOC_DAIFMT_FORMAT_MASK | SND_SOC_DAIFMT_MASTER_MASK)) {
>   
> -	case SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_CBS_CFS:
> -		/*
> -		 * I2S format, SSC provides BCLK and LRC clocks.
> -		 *
> -		 * The SSC transmit and receive clocks are generated
> -		 * from the MCK divider, and the BCLK signal
> -		 * is output on the SSC TK line.
> -		 */
> -
> -		if (bits > 16 && !ssc->pdata->has_fslen_ext) {
> -			dev_err(dai->dev,
> -				"sample size %d is too large for SSC device\n",
> -				bits);
> -			return -EINVAL;
> -		}
> -
> -		fslen_ext = (bits - 1) / 16;
> -		fslen = (bits - 1) % 16;
> -
> -		rcmr =	  SSC_BF(RCMR_PERIOD, rcmr_period)
> -			| SSC_BF(RCMR_STTDLY, START_DELAY)
> -			| SSC_BF(RCMR_START, SSC_START_FALLING_RF)
> -			| SSC_BF(RCMR_CKI, SSC_CKI_RISING)
> -			| SSC_BF(RCMR_CKO, SSC_CKO_NONE)
> -			| SSC_BF(RCMR_CKS, SSC_CKS_DIV);
> -
> -		rfmr =    SSC_BF(RFMR_FSLEN_EXT, fslen_ext)
> -			| SSC_BF(RFMR_FSEDGE, SSC_FSEDGE_POSITIVE)
> -			| SSC_BF(RFMR_FSOS, SSC_FSOS_NEGATIVE)
> -			| SSC_BF(RFMR_FSLEN, fslen)
> -			| SSC_BF(RFMR_DATNB, (channels - 1))
> -			| SSC_BIT(RFMR_MSBF)
> -			| SSC_BF(RFMR_LOOP, 0)
> -			| SSC_BF(RFMR_DATLEN, (bits - 1));
> -
> -		tcmr =	  SSC_BF(TCMR_PERIOD, tcmr_period)
> -			| SSC_BF(TCMR_STTDLY, START_DELAY)
> -			| SSC_BF(TCMR_START, SSC_START_FALLING_RF)
> -			| SSC_BF(TCMR_CKI, SSC_CKI_FALLING)
> -			| SSC_BF(TCMR_CKO, SSC_CKO_CONTINUOUS)
> -			| SSC_BF(TCMR_CKS, SSC_CKS_DIV);
> -
> -		tfmr =    SSC_BF(TFMR_FSLEN_EXT, fslen_ext)
> -			| SSC_BF(TFMR_FSEDGE, SSC_FSEDGE_POSITIVE)
> -			| SSC_BF(TFMR_FSDEN, 0)
> -			| SSC_BF(TFMR_FSOS, SSC_FSOS_NEGATIVE)
> -			| SSC_BF(TFMR_FSLEN, fslen)
> -			| SSC_BF(TFMR_DATNB, (channels - 1))
> -			| SSC_BIT(TFMR_MSBF)
> -			| SSC_BF(TFMR_DATDEF, 0)
> -			| SSC_BF(TFMR_DATLEN, (bits - 1));
> -		break;
> -
> -	case SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_CBM_CFM:
> -		/* I2S format, CODEC supplies BCLK and LRC clocks. */
> -		rcmr =	  SSC_BF(RCMR_PERIOD, 0)
> -			| SSC_BF(RCMR_STTDLY, START_DELAY)
> -			| SSC_BF(RCMR_START, SSC_START_FALLING_RF)
> -			| SSC_BF(RCMR_CKI, SSC_CKI_RISING)
> -			| SSC_BF(RCMR_CKO, SSC_CKO_NONE)
> -			| SSC_BF(RCMR_CKS, ssc->clk_from_rk_pin ?
> -					   SSC_CKS_PIN : SSC_CKS_CLOCK);
> -
> -		rfmr =	  SSC_BF(RFMR_FSEDGE, SSC_FSEDGE_POSITIVE)
> -			| SSC_BF(RFMR_FSOS, SSC_FSOS_NONE)
> -			| SSC_BF(RFMR_FSLEN, 0)
> -			| SSC_BF(RFMR_DATNB, (channels - 1))
> -			| SSC_BIT(RFMR_MSBF)
> -			| SSC_BF(RFMR_LOOP, 0)
> -			| SSC_BF(RFMR_DATLEN, (bits - 1));
> +	fslen_ext = (bits - 1) / 16;
> +	fslen = (bits - 1) % 16;
>   
> -		tcmr =	  SSC_BF(TCMR_PERIOD, 0)
> -			| SSC_BF(TCMR_STTDLY, START_DELAY)
> -			| SSC_BF(TCMR_START, SSC_START_FALLING_RF)
> -			| SSC_BF(TCMR_CKI, SSC_CKI_FALLING)
> -			| SSC_BF(TCMR_CKO, SSC_CKO_NONE)
> -			| SSC_BF(TCMR_CKS, ssc->clk_from_rk_pin ?
> -					   SSC_CKS_CLOCK : SSC_CKS_PIN);
> +	switch (ssc_p->daifmt & SND_SOC_DAIFMT_FORMAT_MASK) {
>   
> -		tfmr =	  SSC_BF(TFMR_FSEDGE, SSC_FSEDGE_POSITIVE)
> -			| SSC_BF(TFMR_FSDEN, 0)
> -			| SSC_BF(TFMR_FSOS, SSC_FSOS_NONE)
> -			| SSC_BF(TFMR_FSLEN, 0)
> -			| SSC_BF(TFMR_DATNB, (channels - 1))
> -			| SSC_BIT(TFMR_MSBF)
> -			| SSC_BF(TFMR_DATDEF, 0)
> -			| SSC_BF(TFMR_DATLEN, (bits - 1));
> -		break;
> -
> -	case SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_CBM_CFS:
> -		/* I2S format, CODEC supplies BCLK, SSC supplies LRCLK. */
> -		if (bits > 16 && !ssc->pdata->has_fslen_ext) {
> -			dev_err(dai->dev,
> -				"sample size %d is too large for SSC device\n",
> -				bits);
> -			return -EINVAL;
> -		}
> -
> -		fslen_ext = (bits - 1) / 16;
> -		fslen = (bits - 1) % 16;
> -
> -		rcmr =	  SSC_BF(RCMR_PERIOD, rcmr_period)
> -			| SSC_BF(RCMR_STTDLY, START_DELAY)
> -			| SSC_BF(RCMR_START, SSC_START_FALLING_RF)
> -			| SSC_BF(RCMR_CKI, SSC_CKI_RISING)
> -			| SSC_BF(RCMR_CKO, SSC_CKO_NONE)
> -			| SSC_BF(RCMR_CKS, ssc->clk_from_rk_pin ?
> -					   SSC_CKS_PIN : SSC_CKS_CLOCK);
> -
> -		rfmr =    SSC_BF(RFMR_FSLEN_EXT, fslen_ext)
> -			| SSC_BF(RFMR_FSEDGE, SSC_FSEDGE_POSITIVE)
> -			| SSC_BF(RFMR_FSOS, SSC_FSOS_NEGATIVE)
> -			| SSC_BF(RFMR_FSLEN, fslen)
> -			| SSC_BF(RFMR_DATNB, (channels - 1))
> -			| SSC_BIT(RFMR_MSBF)
> -			| SSC_BF(RFMR_LOOP, 0)
> -			| SSC_BF(RFMR_DATLEN, (bits - 1));
> -
> -		tcmr =	  SSC_BF(TCMR_PERIOD, tcmr_period)
> -			| SSC_BF(TCMR_STTDLY, START_DELAY)
> -			| SSC_BF(TCMR_START, SSC_START_FALLING_RF)
> -			| SSC_BF(TCMR_CKI, SSC_CKI_FALLING)
> -			| SSC_BF(TCMR_CKO, SSC_CKO_NONE)
> -			| SSC_BF(TCMR_CKS, ssc->clk_from_rk_pin ?
> -					   SSC_CKS_CLOCK : SSC_CKS_PIN);
> -
> -		tfmr =    SSC_BF(TFMR_FSLEN_EXT, fslen_ext)
> -			| SSC_BF(TFMR_FSEDGE, SSC_FSEDGE_NEGATIVE)
> -			| SSC_BF(TFMR_FSDEN, 0)
> -			| SSC_BF(TFMR_FSOS, SSC_FSOS_NEGATIVE)
> -			| SSC_BF(TFMR_FSLEN, fslen)
> -			| SSC_BF(TFMR_DATNB, (channels - 1))
> -			| SSC_BIT(TFMR_MSBF)
> -			| SSC_BF(TFMR_DATDEF, 0)
> -			| SSC_BF(TFMR_DATLEN, (bits - 1));
> -		break;
> -
> -	case SND_SOC_DAIFMT_DSP_A | SND_SOC_DAIFMT_CBS_CFS:
> -		/*
> -		 * DSP/PCM Mode A format, SSC provides BCLK and LRC clocks.
> -		 *
> -		 * The SSC transmit and receive clocks are generated from the
> -		 * MCK divider, and the BCLK signal is output
> -		 * on the SSC TK line.
> -		 */
> -		rcmr =	  SSC_BF(RCMR_PERIOD, rcmr_period)
> -			| SSC_BF(RCMR_STTDLY, 1)
> -			| SSC_BF(RCMR_START, SSC_START_RISING_RF)
> -			| SSC_BF(RCMR_CKI, SSC_CKI_RISING)
> -			| SSC_BF(RCMR_CKO, SSC_CKO_NONE)
> -			| SSC_BF(RCMR_CKS, SSC_CKS_DIV);
> +	case SND_SOC_DAIFMT_I2S:
> +		fs_osync = SSC_FSOS_NEGATIVE;
>   
> -		rfmr =	  SSC_BF(RFMR_FSEDGE, SSC_FSEDGE_POSITIVE)
> -			| SSC_BF(RFMR_FSOS, SSC_FSOS_POSITIVE)
> -			| SSC_BF(RFMR_FSLEN, 0)
> -			| SSC_BF(RFMR_DATNB, (channels - 1))
> -			| SSC_BIT(RFMR_MSBF)
> -			| SSC_BF(RFMR_LOOP, 0)
> -			| SSC_BF(RFMR_DATLEN, (bits - 1));
> +		rcmr =	  SSC_BF(RCMR_STTDLY, 1)
> +			| SSC_BF(RCMR_START, SSC_START_FALLING_RF);
>   
> -		tcmr =	  SSC_BF(TCMR_PERIOD, tcmr_period)
> -			| SSC_BF(TCMR_STTDLY, 1)
> -			| SSC_BF(TCMR_START, SSC_START_RISING_RF)
> -			| SSC_BF(TCMR_CKI, SSC_CKI_FALLING)
> -			| SSC_BF(TCMR_CKO, SSC_CKO_CONTINUOUS)
> -			| SSC_BF(TCMR_CKS, SSC_CKS_DIV);
> +		tcmr =	  SSC_BF(TCMR_STTDLY, 1)
> +			| SSC_BF(TCMR_START, SSC_START_FALLING_RF);
>   
> -		tfmr =	  SSC_BF(TFMR_FSEDGE, SSC_FSEDGE_POSITIVE)
> -			| SSC_BF(TFMR_FSDEN, 0)
> -			| SSC_BF(TFMR_FSOS, SSC_FSOS_POSITIVE)
> -			| SSC_BF(TFMR_FSLEN, 0)
> -			| SSC_BF(TFMR_DATNB, (channels - 1))
> -			| SSC_BIT(TFMR_MSBF)
> -			| SSC_BF(TFMR_DATDEF, 0)
> -			| SSC_BF(TFMR_DATLEN, (bits - 1));
>   		break;
>   
> -	case SND_SOC_DAIFMT_DSP_A | SND_SOC_DAIFMT_CBM_CFM:
> +	case SND_SOC_DAIFMT_DSP_A:
>   		/*
> -		 * DSP/PCM Mode A format, CODEC supplies BCLK and LRC clocks.
> +		 * DSP/PCM Mode A format
>   		 *
>   		 * Data is transferred on first BCLK after LRC pulse rising
>   		 * edge.If stereo, the right channel data is contiguous with
>   		 * the left channel data.
>   		 */
> -		rcmr =	  SSC_BF(RCMR_PERIOD, 0)
> -			| SSC_BF(RCMR_STTDLY, START_DELAY)
> -			| SSC_BF(RCMR_START, SSC_START_RISING_RF)
> -			| SSC_BF(RCMR_CKI, SSC_CKI_RISING)
> -			| SSC_BF(RCMR_CKO, SSC_CKO_NONE)
> -			| SSC_BF(RCMR_CKS, ssc->clk_from_rk_pin ?
> -					   SSC_CKS_PIN : SSC_CKS_CLOCK);
> +		fs_osync = SSC_FSOS_POSITIVE;
> +		fslen = fslen_ext = 0;
>   
> -		rfmr =	  SSC_BF(RFMR_FSEDGE, SSC_FSEDGE_POSITIVE)
> -			| SSC_BF(RFMR_FSOS, SSC_FSOS_NONE)
> -			| SSC_BF(RFMR_FSLEN, 0)
> -			| SSC_BF(RFMR_DATNB, (channels - 1))
> -			| SSC_BIT(RFMR_MSBF)
> -			| SSC_BF(RFMR_LOOP, 0)
> -			| SSC_BF(RFMR_DATLEN, (bits - 1));
> +		rcmr =	  SSC_BF(RCMR_STTDLY, 1)
> +			| SSC_BF(RCMR_START, SSC_START_RISING_RF);
>   
> -		tcmr =	  SSC_BF(TCMR_PERIOD, 0)
> -			| SSC_BF(TCMR_STTDLY, START_DELAY)
> -			| SSC_BF(TCMR_START, SSC_START_RISING_RF)
> -			| SSC_BF(TCMR_CKI, SSC_CKI_FALLING)
> -			| SSC_BF(TCMR_CKO, SSC_CKO_NONE)
> -			| SSC_BF(RCMR_CKS, ssc->clk_from_rk_pin ?
> -					   SSC_CKS_CLOCK : SSC_CKS_PIN);
> +		tcmr =	  SSC_BF(TCMR_STTDLY, 1)
> +			| SSC_BF(TCMR_START, SSC_START_RISING_RF);
>   
> -		tfmr =	  SSC_BF(TFMR_FSEDGE, SSC_FSEDGE_POSITIVE)
> -			| SSC_BF(TFMR_FSDEN, 0)
> -			| SSC_BF(TFMR_FSOS, SSC_FSOS_NONE)
> -			| SSC_BF(TFMR_FSLEN, 0)
> -			| SSC_BF(TFMR_DATNB, (channels - 1))
> -			| SSC_BIT(TFMR_MSBF)
> -			| SSC_BF(TFMR_DATDEF, 0)
> -			| SSC_BF(TFMR_DATLEN, (bits - 1));
>   		break;
>   
>   	default:
> @@ -785,6 +599,67 @@ static int atmel_ssc_hw_params(struct snd_pcm_substream *substream,
>   			ssc_p->daifmt);
>   		return -EINVAL;
>   	}
> +
> +	if (!atmel_ssc_cfs(ssc_p)) {
> +		fslen = fslen_ext = 0;
> +		rcmr_period = tcmr_period = 0;
> +		fs_osync = SSC_FSOS_NONE;
> +	}
> +
> +	if (atmel_ssc_cbs(ssc_p)) {
> +		/*
> +		 * SSC provides BCLK
> +		 *
> +		 * The SSC transmit and receive clocks are generated from the
> +		 * MCK divider, and the BCLK signal is output
> +		 * on the SSC TK line.
> +		 */
> +		rcmr |=	  SSC_BF(RCMR_CKS, SSC_CKS_DIV)
> +			| SSC_BF(RCMR_CKO, SSC_CKO_NONE);
> +
> +		tcmr |=	  SSC_BF(TCMR_CKS, SSC_CKS_DIV)
> +			| SSC_BF(TCMR_CKO, SSC_CKO_CONTINUOUS);
> +	} else {
> +		rcmr |=	  SSC_BF(RCMR_CKS, ssc->clk_from_rk_pin ?
> +					SSC_CKS_PIN : SSC_CKS_CLOCK)
> +			| SSC_BF(RCMR_CKO, SSC_CKO_NONE);
> +
> +		tcmr |=	  SSC_BF(TCMR_CKS, ssc->clk_from_rk_pin ?
> +					SSC_CKS_CLOCK : SSC_CKS_PIN)
> +			| SSC_BF(TCMR_CKO, SSC_CKO_NONE);
> +	}
> +
> +	rcmr |=	  SSC_BF(RCMR_PERIOD, rcmr_period)
> +		| SSC_BF(RCMR_CKI, SSC_CKI_RISING);

You can also add here SSC_BF(RCMR_CKO, SSC_CKO_NONE) and remove it from 
the if-else above;

> +
> +	tcmr |=   SSC_BF(TCMR_PERIOD, tcmr_period)
> +		| SSC_BF(TCMR_CKI, SSC_CKI_FALLING);
> +
> +	rfmr =    SSC_BF(RFMR_FSLEN_EXT, fslen_ext)
> +		| SSC_BF(RFMR_FSEDGE, SSC_FSEDGE_POSITIVE)
> +		| SSC_BF(RFMR_FSOS, fs_osync)
> +		| SSC_BF(RFMR_FSLEN, fslen)
> +		| SSC_BF(RFMR_DATNB, (channels - 1))
> +		| SSC_BIT(RFMR_MSBF)
> +		| SSC_BF(RFMR_LOOP, 0)
> +		| SSC_BF(RFMR_DATLEN, (bits - 1));
> +
> +	tfmr =    SSC_BF(TFMR_FSLEN_EXT, fslen_ext)
> +		| SSC_BF(TFMR_FSEDGE, SSC_FSEDGE_POSITIVE)
> +		| SSC_BF(TFMR_FSDEN, 0)
> +		| SSC_BF(TFMR_FSOS, fs_osync)
> +		| SSC_BF(TFMR_FSLEN, fslen)
> +		| SSC_BF(TFMR_DATNB, (channels - 1))
> +		| SSC_BIT(TFMR_MSBF)
> +		| SSC_BF(TFMR_DATDEF, 0)
> +		| SSC_BF(TFMR_DATLEN, (bits - 1));
> +
> +	if (fslen_ext && !ssc->pdata->has_fslen_ext) {
> +		dev_err(dai->dev, "sample size %d is too large for SSC device\n",
> +			bits);
> +		return -EINVAL;
> +	}
> +
>   	pr_debug("atmel_ssc_hw_params: "
>   			"RCMR=%08x RFMR=%08x TCMR=%08x TFMR=%08x\n",
>   			rcmr, rfmr, tcmr, tfmr);
> 

You are adding support for SND_SOC_DAIFMT_DSP_A | 
SND_SOC_DAIFMT_CBM_CFS. If this is intended, please make a separate 
patch. If not, then:

printk(KERN_WARNING "atmel_ssc_dai: unsupported DAI format 0x%x\n",
			ssc_p->daifmt);
return -EINVAL;

The rest of the patch looks great.

Thanks and best regards,
Codrin
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/5] ASoC: atmel_ssc_dai: rework DAI format configuration

[mirq-linux]

On Wed, Jul 24, 2019 at 10:35:29AM +0000, Codrin.Ciubotariu@microchip.com wrote:
> On 22.07.2019 21:27, Michał Mirosław wrote:
> > Rework DAI format calculation in preparation for adding more formats
> > later.
> > 
> > Note: this changes FSEDGE to POSITIVE for I2S CBM_CFS mode as the TXSYN
> > interrupt is not used anyway.
> > 
> > Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> > ---
> >   sound/soc/atmel/atmel_ssc_dai.c | 283 +++++++++-----------------------
> >   1 file changed, 79 insertions(+), 204 deletions(-)
> > 
> > diff --git a/sound/soc/atmel/atmel_ssc_dai.c b/sound/soc/atmel/atmel_ssc_dai.c
> > index 6f89483ac88c..b2992496e52f 100644
> > --- a/sound/soc/atmel/atmel_ssc_dai.c
> > +++ b/sound/soc/atmel/atmel_ssc_dai.c
[...]
> > +	if (atmel_ssc_cbs(ssc_p)) {
> > +		/*
> > +		 * SSC provides BCLK
> > +		 *
> > +		 * The SSC transmit and receive clocks are generated from the
> > +		 * MCK divider, and the BCLK signal is output
> > +		 * on the SSC TK line.
> > +		 */
> > +		rcmr |=	  SSC_BF(RCMR_CKS, SSC_CKS_DIV)
> > +			| SSC_BF(RCMR_CKO, SSC_CKO_NONE);
> > +
> > +		tcmr |=	  SSC_BF(TCMR_CKS, SSC_CKS_DIV)
> > +			| SSC_BF(TCMR_CKO, SSC_CKO_CONTINUOUS);
> > +	} else {
> > +		rcmr |=	  SSC_BF(RCMR_CKS, ssc->clk_from_rk_pin ?
> > +					SSC_CKS_PIN : SSC_CKS_CLOCK)
> > +			| SSC_BF(RCMR_CKO, SSC_CKO_NONE);
> > +
> > +		tcmr |=	  SSC_BF(TCMR_CKS, ssc->clk_from_rk_pin ?
> > +					SSC_CKS_CLOCK : SSC_CKS_PIN)
> > +			| SSC_BF(TCMR_CKO, SSC_CKO_NONE);
> > +	}
> > +
> > +	rcmr |=	  SSC_BF(RCMR_PERIOD, rcmr_period)
> > +		| SSC_BF(RCMR_CKI, SSC_CKI_RISING);
> 
> You can also add here SSC_BF(RCMR_CKO, SSC_CKO_NONE) and remove it from 
> the if-else above;

I left it to keep symmetry between TX and RX code. I can pull it here if
you prefer that way.

> > +
> > +	tcmr |=   SSC_BF(TCMR_PERIOD, tcmr_period)
> > +		| SSC_BF(TCMR_CKI, SSC_CKI_FALLING);
> > +
> > +	rfmr =    SSC_BF(RFMR_FSLEN_EXT, fslen_ext)
> > +		| SSC_BF(RFMR_FSEDGE, SSC_FSEDGE_POSITIVE)
> > +		| SSC_BF(RFMR_FSOS, fs_osync)
> > +		| SSC_BF(RFMR_FSLEN, fslen)
> > +		| SSC_BF(RFMR_DATNB, (channels - 1))
> > +		| SSC_BIT(RFMR_MSBF)
> > +		| SSC_BF(RFMR_LOOP, 0)
> > +		| SSC_BF(RFMR_DATLEN, (bits - 1));
> > +
> > +	tfmr =    SSC_BF(TFMR_FSLEN_EXT, fslen_ext)
> > +		| SSC_BF(TFMR_FSEDGE, SSC_FSEDGE_POSITIVE)
> > +		| SSC_BF(TFMR_FSDEN, 0)
> > +		| SSC_BF(TFMR_FSOS, fs_osync)
> > +		| SSC_BF(TFMR_FSLEN, fslen)
> > +		| SSC_BF(TFMR_DATNB, (channels - 1))
> > +		| SSC_BIT(TFMR_MSBF)
> > +		| SSC_BF(TFMR_DATDEF, 0)
> > +		| SSC_BF(TFMR_DATLEN, (bits - 1));
> > +
> > +	if (fslen_ext && !ssc->pdata->has_fslen_ext) {
> > +		dev_err(dai->dev, "sample size %d is too large for SSC device\n",
> > +			bits);
> > +		return -EINVAL;
> > +	}
> > +
> >   	pr_debug("atmel_ssc_hw_params: "
> >   			"RCMR=%08x RFMR=%08x TCMR=%08x TFMR=%08x\n",
> >   			rcmr, rfmr, tcmr, tfmr);
> > 
> 
> You are adding support for SND_SOC_DAIFMT_DSP_A | 
> SND_SOC_DAIFMT_CBM_CFS. If this is intended, please make a separate 
> patch. If not, then:
> 
> printk(KERN_WARNING "atmel_ssc_dai: unsupported DAI format 0x%x\n",
> 			ssc_p->daifmt);
> return -EINVAL;

Hmm. I guess this is actually a good side effect. I can't see a way to
contain this change that doesn't involve adding code that's immediately
removed in next patch. So would you agree to just mentioning this in
commit message?

Best Regards,
Michał Mirosław

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

Re: [PATCH 2/5] ASoC: atmel_ssc_dai: rework DAI format configuration

[Codrin.Ciubotariu]

On 24.07.2019 14:15, mirq-linux@rere.qmqm.pl wrote:
> External E-Mail
> 
> 
> On Wed, Jul 24, 2019 at 10:35:29AM +0000, Codrin.Ciubotariu@microchip.com wrote:
>> On 22.07.2019 21:27, Michał Mirosław wrote:
>>> Rework DAI format calculation in preparation for adding more formats
>>> later.
>>>
>>> Note: this changes FSEDGE to POSITIVE for I2S CBM_CFS mode as the TXSYN
>>> interrupt is not used anyway.
>>>
>>> Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
>>> ---
>>>    sound/soc/atmel/atmel_ssc_dai.c | 283 +++++++++-----------------------
>>>    1 file changed, 79 insertions(+), 204 deletions(-)
>>>
>>> diff --git a/sound/soc/atmel/atmel_ssc_dai.c b/sound/soc/atmel/atmel_ssc_dai.c
>>> index 6f89483ac88c..b2992496e52f 100644
>>> --- a/sound/soc/atmel/atmel_ssc_dai.c
>>> +++ b/sound/soc/atmel/atmel_ssc_dai.c
> [...]
>>> +	if (atmel_ssc_cbs(ssc_p)) {
>>> +		/*
>>> +		 * SSC provides BCLK
>>> +		 *
>>> +		 * The SSC transmit and receive clocks are generated from the
>>> +		 * MCK divider, and the BCLK signal is output
>>> +		 * on the SSC TK line.
>>> +		 */
>>> +		rcmr |=	  SSC_BF(RCMR_CKS, SSC_CKS_DIV)
>>> +			| SSC_BF(RCMR_CKO, SSC_CKO_NONE);
>>> +
>>> +		tcmr |=	  SSC_BF(TCMR_CKS, SSC_CKS_DIV)
>>> +			| SSC_BF(TCMR_CKO, SSC_CKO_CONTINUOUS);
>>> +	} else {
>>> +		rcmr |=	  SSC_BF(RCMR_CKS, ssc->clk_from_rk_pin ?
>>> +					SSC_CKS_PIN : SSC_CKS_CLOCK)
>>> +			| SSC_BF(RCMR_CKO, SSC_CKO_NONE);
>>> +
>>> +		tcmr |=	  SSC_BF(TCMR_CKS, ssc->clk_from_rk_pin ?
>>> +					SSC_CKS_CLOCK : SSC_CKS_PIN)
>>> +			| SSC_BF(TCMR_CKO, SSC_CKO_NONE);
>>> +	}
>>> +
>>> +	rcmr |=	  SSC_BF(RCMR_PERIOD, rcmr_period)
>>> +		| SSC_BF(RCMR_CKI, SSC_CKI_RISING);
>>
>> You can also add here SSC_BF(RCMR_CKO, SSC_CKO_NONE) and remove it from
>> the if-else above;
> 
> I left it to keep symmetry between TX and RX code. I can pull it here if
> you prefer that way.

Right, you can leave it then.

> 
>>> +
>>> +	tcmr |=   SSC_BF(TCMR_PERIOD, tcmr_period)
>>> +		| SSC_BF(TCMR_CKI, SSC_CKI_FALLING);
>>> +
>>> +	rfmr =    SSC_BF(RFMR_FSLEN_EXT, fslen_ext)
>>> +		| SSC_BF(RFMR_FSEDGE, SSC_FSEDGE_POSITIVE)
>>> +		| SSC_BF(RFMR_FSOS, fs_osync)
>>> +		| SSC_BF(RFMR_FSLEN, fslen)
>>> +		| SSC_BF(RFMR_DATNB, (channels - 1))
>>> +		| SSC_BIT(RFMR_MSBF)
>>> +		| SSC_BF(RFMR_LOOP, 0)
>>> +		| SSC_BF(RFMR_DATLEN, (bits - 1));
>>> +
>>> +	tfmr =    SSC_BF(TFMR_FSLEN_EXT, fslen_ext)
>>> +		| SSC_BF(TFMR_FSEDGE, SSC_FSEDGE_POSITIVE)
>>> +		| SSC_BF(TFMR_FSDEN, 0)
>>> +		| SSC_BF(TFMR_FSOS, fs_osync)
>>> +		| SSC_BF(TFMR_FSLEN, fslen)
>>> +		| SSC_BF(TFMR_DATNB, (channels - 1))
>>> +		| SSC_BIT(TFMR_MSBF)
>>> +		| SSC_BF(TFMR_DATDEF, 0)
>>> +		| SSC_BF(TFMR_DATLEN, (bits - 1));
>>> +
>>> +	if (fslen_ext && !ssc->pdata->has_fslen_ext) {
>>> +		dev_err(dai->dev, "sample size %d is too large for SSC device\n",
>>> +			bits);
>>> +		return -EINVAL;
>>> +	}
>>> +
>>>    	pr_debug("atmel_ssc_hw_params: "
>>>    			"RCMR=%08x RFMR=%08x TCMR=%08x TFMR=%08x\n",
>>>    			rcmr, rfmr, tcmr, tfmr);
>>>
>>
>> You are adding support for SND_SOC_DAIFMT_DSP_A |
>> SND_SOC_DAIFMT_CBM_CFS. If this is intended, please make a separate
>> patch. If not, then:
>>
>> printk(KERN_WARNING "atmel_ssc_dai: unsupported DAI format 0x%x\n",
>> 			ssc_p->daifmt);
>> return -EINVAL;
> 
> Hmm. I guess this is actually a good side effect. I can't see a way to
> contain this change that doesn't involve adding code that's immediately
> removed in next patch. So would you agree to just mentioning this in
> commit message?

I prefer a separate patch, for clarity mostly, but I don't have a strong 
opinion on this. Later, it might prove trickier to investigate a bug 
this case and review this patch. Also, we should test and see that this 
format works indeed...

Best regards,
Codrin

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

[PATCH 5/5] ASoC: atmel_ssc_dai: Enable shared FSYNC source in frame-slave mode

[Michał Mirosław]

SSC driver allows only synchronous TX and RX. In slave mode for BCLK
it uses only one of TK or RK pin, but for LRCLK it configured separate
inputs from TF and RF pins. Allow configuration with common FS signal.

Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
 .../devicetree/bindings/misc/atmel-ssc.txt    |  4 ++
 drivers/misc/atmel-ssc.c                      |  2 +
 include/linux/atmel-ssc.h                     |  1 +
 sound/soc/atmel/atmel_ssc_dai.c               | 48 ++++++++++++-------
 4 files changed, 38 insertions(+), 17 deletions(-)

diff --git a/Documentation/devicetree/bindings/misc/atmel-ssc.txt b/Documentation/devicetree/bindings/misc/atmel-ssc.txt
index f9fb412642fe..89133c5b82cb 100644
--- a/Documentation/devicetree/bindings/misc/atmel-ssc.txt
+++ b/Documentation/devicetree/bindings/misc/atmel-ssc.txt
@@ -24,6 +24,10 @@ Optional properties:
        this parameter to choose where the clock from.
      - By default the clock is from TK pin, if the clock from RK pin, this
        property is needed.
+  - atmel,shared-fs-pin: bool property.
+     - When SSC works in slave mode, by default it gets separate LRCLK signals
+       from RF and TF. This property makes SSC use a single pin for both
+       RX and TX. TF is used unless atmel,clk-from-rk-pin is also present.
   - #sound-dai-cells: Should contain <0>.
      - This property makes the SSC into an automatically registered DAI.
 
diff --git a/drivers/misc/atmel-ssc.c b/drivers/misc/atmel-ssc.c
index ab4144ea1f11..da63eee1cdf5 100644
--- a/drivers/misc/atmel-ssc.c
+++ b/drivers/misc/atmel-ssc.c
@@ -210,6 +210,8 @@ static int ssc_probe(struct platform_device *pdev)
 		struct device_node *np = pdev->dev.of_node;
 		ssc->clk_from_rk_pin =
 			of_property_read_bool(np, "atmel,clk-from-rk-pin");
+		ssc->shared_fs_pin =
+			of_property_read_bool(np, "atmel,shared-fs-pin");
 	}
 
 	regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
diff --git a/include/linux/atmel-ssc.h b/include/linux/atmel-ssc.h
index 6091d2abc1eb..46fdff2dfbb0 100644
--- a/include/linux/atmel-ssc.h
+++ b/include/linux/atmel-ssc.h
@@ -21,6 +21,7 @@ struct ssc_device {
 	int			user;
 	int			irq;
 	bool			clk_from_rk_pin;
+	bool			shared_fs_pin;
 	bool			sound_dai;
 };
 
diff --git a/sound/soc/atmel/atmel_ssc_dai.c b/sound/soc/atmel/atmel_ssc_dai.c
index cf2cfc345676..6f595a748618 100644
--- a/sound/soc/atmel/atmel_ssc_dai.c
+++ b/sound/soc/atmel/atmel_ssc_dai.c
@@ -471,7 +471,7 @@ static int atmel_ssc_hw_params(struct snd_pcm_substream *substream,
 	int dir, channels, bits;
 	u32 tfmr, rfmr, tcmr, rcmr;
 	int ret;
-	int fslen, fslen_ext, fs_osync;
+	int fslen, fslen_ext, fs_osync, fs_edge;
 	u32 cmr_div;
 	u32 tcmr_period;
 	u32 rcmr_period;
@@ -567,24 +567,20 @@ static int atmel_ssc_hw_params(struct snd_pcm_substream *substream,
 	case SND_SOC_DAIFMT_LEFT_J:
 		/* left-justified format */
 		fs_osync = SSC_FSOS_POSITIVE;
+		fs_edge = SSC_START_RISING_TF;
 
-		rcmr =	  SSC_BF(RCMR_STTDLY, 0)
-			| SSC_BF(RCMR_START, SSC_START_RISING_RF);
-
-		tcmr =	  SSC_BF(TCMR_STTDLY, 0)
-			| SSC_BF(TCMR_START, SSC_START_RISING_TF);
+		rcmr =	  SSC_BF(RCMR_STTDLY, 0);
+		tcmr =	  SSC_BF(TCMR_STTDLY, 0);
 
 		break;
 
 	case SND_SOC_DAIFMT_I2S:
 		/* I2S format = left-justified with start bit and inverted LRCLK */
 		fs_osync = SSC_FSOS_NEGATIVE;
+		fs_edge = SSC_START_FALLING_TF;
 
-		rcmr =	  SSC_BF(RCMR_STTDLY, 1)
-			| SSC_BF(RCMR_START, SSC_START_FALLING_RF);
-
-		tcmr =	  SSC_BF(TCMR_STTDLY, 1)
-			| SSC_BF(TCMR_START, SSC_START_FALLING_TF);
+		rcmr =	  SSC_BF(RCMR_STTDLY, 1);
+		tcmr =	  SSC_BF(TCMR_STTDLY, 1);
 
 		break;
 
@@ -597,13 +593,11 @@ static int atmel_ssc_hw_params(struct snd_pcm_substream *substream,
 		 * the left channel data.
 		 */
 		fs_osync = SSC_FSOS_POSITIVE;
+		fs_edge = SSC_START_RISING_TF;
 		fslen = fslen_ext = 0;
 
-		rcmr =	  SSC_BF(RCMR_STTDLY, 1)
-			| SSC_BF(RCMR_START, SSC_START_RISING_RF);
-
-		tcmr =	  SSC_BF(TCMR_STTDLY, 1)
-			| SSC_BF(TCMR_START, SSC_START_RISING_TF);
+		rcmr =	  SSC_BF(RCMR_STTDLY, 1);
+		tcmr =	  SSC_BF(TCMR_STTDLY, 1);
 
 		break;
 
@@ -613,10 +607,30 @@ static int atmel_ssc_hw_params(struct snd_pcm_substream *substream,
 		return -EINVAL;
 	}
 
-	if (!atmel_ssc_cfs(ssc_p)) {
+	if (atmel_ssc_cfs(ssc_p)) {
+		/*
+		 * SSC provides LRCLK
+		 *
+		 * Both TF and RF are generated, so use them directly.
+		 */
+		rcmr |=	  SSC_BF(RCMR_START, fs_edge);
+		tcmr |=	  SSC_BF(TCMR_START, fs_edge);
+	} else {
 		fslen = fslen_ext = 0;
 		rcmr_period = tcmr_period = 0;
 		fs_osync = SSC_FSOS_NONE;
+		if (!ssc->shared_fs_pin) {
+			rcmr |=	  SSC_BF(RCMR_START, fs_edge);
+			tcmr |=	  SSC_BF(TCMR_START, fs_edge);
+		} else if (ssc->clk_from_rk_pin) {
+			/* assume RF is to be used when RK is used as BCLK input */
+			/* Note: won't work correctly on SAMA5D2 due to errata */
+			rcmr |=	  SSC_BF(RCMR_START, fs_edge);
+			tcmr |=	  SSC_BF(TCMR_START, SSC_START_RECEIVE);
+		} else {
+			rcmr |=	  SSC_BF(RCMR_START, SSC_START_TRANSMIT);
+			tcmr |=	  SSC_BF(TCMR_START, fs_edge);
+		}
 	}
 
 	if (atmel_ssc_cbs(ssc_p)) {
-- 
2.20.1


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

Re: [PATCH 5/5] ASoC: atmel_ssc_dai: Enable shared FSYNC source in frame-slave mode

[Codrin.Ciubotariu]

On 22.07.2019 21:27, Michał Mirosław wrote:
> SSC driver allows only synchronous TX and RX. In slave mode for BCLK
> it uses only one of TK or RK pin, but for LRCLK it configured separate
> inputs from TF and RF pins. Allow configuration with common FS signal.
> 
> Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> ---
>   .../devicetree/bindings/misc/atmel-ssc.txt    |  4 ++
>   drivers/misc/atmel-ssc.c                      |  2 +
>   include/linux/atmel-ssc.h                     |  1 +
>   sound/soc/atmel/atmel_ssc_dai.c               | 48 ++++++++++++-------
>   4 files changed, 38 insertions(+), 17 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/misc/atmel-ssc.txt b/Documentation/devicetree/bindings/misc/atmel-ssc.txt
> index f9fb412642fe..89133c5b82cb 100644
> --- a/Documentation/devicetree/bindings/misc/atmel-ssc.txt
> +++ b/Documentation/devicetree/bindings/misc/atmel-ssc.txt
> @@ -24,6 +24,10 @@ Optional properties:
>          this parameter to choose where the clock from.
>        - By default the clock is from TK pin, if the clock from RK pin, this
>          property is needed.
> +  - atmel,shared-fs-pin: bool property.
> +     - When SSC works in slave mode, by default it gets separate LRCLK signals
> +       from RF and TF. This property makes SSC use a single pin for both
> +       RX and TX. TF is used unless atmel,clk-from-rk-pin is also present.
>     - #sound-dai-cells: Should contain <0>.
>        - This property makes the SSC into an automatically registered DAI.

the binding changes should be sent in a separate patch, please see 
Documentation/devicetree/bindings/submitting-patches.txt for details.

>   
> diff --git a/drivers/misc/atmel-ssc.c b/drivers/misc/atmel-ssc.c
> index ab4144ea1f11..da63eee1cdf5 100644
> --- a/drivers/misc/atmel-ssc.c
> +++ b/drivers/misc/atmel-ssc.c
> @@ -210,6 +210,8 @@ static int ssc_probe(struct platform_device *pdev)
>   		struct device_node *np = pdev->dev.of_node;
>   		ssc->clk_from_rk_pin =
>   			of_property_read_bool(np, "atmel,clk-from-rk-pin");
> +		ssc->shared_fs_pin =
> +			of_property_read_bool(np, "atmel,shared-fs-pin");
>   	}
>   
>   	regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> diff --git a/include/linux/atmel-ssc.h b/include/linux/atmel-ssc.h
> index 6091d2abc1eb..46fdff2dfbb0 100644
> --- a/include/linux/atmel-ssc.h
> +++ b/include/linux/atmel-ssc.h
> @@ -21,6 +21,7 @@ struct ssc_device {
>   	int			user;
>   	int			irq;
>   	bool			clk_from_rk_pin;
> +	bool			shared_fs_pin;
>   	bool			sound_dai;
>   };

These changes should also be in a separate patch, since drivers/misc/* 
files are in a different subsystem, with different maintainers.

>   
> diff --git a/sound/soc/atmel/atmel_ssc_dai.c b/sound/soc/atmel/atmel_ssc_dai.c
> index cf2cfc345676..6f595a748618 100644
> --- a/sound/soc/atmel/atmel_ssc_dai.c
> +++ b/sound/soc/atmel/atmel_ssc_dai.c
> @@ -471,7 +471,7 @@ static int atmel_ssc_hw_params(struct snd_pcm_substream *substream,
>   	int dir, channels, bits;
>   	u32 tfmr, rfmr, tcmr, rcmr;
>   	int ret;
> -	int fslen, fslen_ext, fs_osync;
> +	int fslen, fslen_ext, fs_osync, fs_edge;
>   	u32 cmr_div;
>   	u32 tcmr_period;
>   	u32 rcmr_period;
> @@ -567,24 +567,20 @@ static int atmel_ssc_hw_params(struct snd_pcm_substream *substream,
>   	case SND_SOC_DAIFMT_LEFT_J:
>   		/* left-justified format */
>   		fs_osync = SSC_FSOS_POSITIVE;
> +		fs_edge = SSC_START_RISING_TF;
>   
> -		rcmr =	  SSC_BF(RCMR_STTDLY, 0)
> -			| SSC_BF(RCMR_START, SSC_START_RISING_RF);
> -
> -		tcmr =	  SSC_BF(TCMR_STTDLY, 0)
> -			| SSC_BF(TCMR_START, SSC_START_RISING_TF);
> +		rcmr =	  SSC_BF(RCMR_STTDLY, 0);
> +		tcmr =	  SSC_BF(TCMR_STTDLY, 0);
>   
>   		break;
>   
>   	case SND_SOC_DAIFMT_I2S:
>   		/* I2S format = left-justified with start bit and inverted LRCLK */
>   		fs_osync = SSC_FSOS_NEGATIVE;
> +		fs_edge = SSC_START_FALLING_TF;
>   
> -		rcmr =	  SSC_BF(RCMR_STTDLY, 1)
> -			| SSC_BF(RCMR_START, SSC_START_FALLING_RF);
> -
> -		tcmr =	  SSC_BF(TCMR_STTDLY, 1)
> -			| SSC_BF(TCMR_START, SSC_START_FALLING_TF);
> +		rcmr =	  SSC_BF(RCMR_STTDLY, 1);
> +		tcmr =	  SSC_BF(TCMR_STTDLY, 1);
>   
>   		break;
>   
> @@ -597,13 +593,11 @@ static int atmel_ssc_hw_params(struct snd_pcm_substream *substream,
>   		 * the left channel data.
>   		 */
>   		fs_osync = SSC_FSOS_POSITIVE;
> +		fs_edge = SSC_START_RISING_TF;
>   		fslen = fslen_ext = 0;
>   
> -		rcmr =	  SSC_BF(RCMR_STTDLY, 1)
> -			| SSC_BF(RCMR_START, SSC_START_RISING_RF);
> -
> -		tcmr =	  SSC_BF(TCMR_STTDLY, 1)
> -			| SSC_BF(TCMR_START, SSC_START_RISING_TF);
> +		rcmr =	  SSC_BF(RCMR_STTDLY, 1);
> +		tcmr =	  SSC_BF(TCMR_STTDLY, 1);
>   
>   		break;
>   
> @@ -613,10 +607,30 @@ static int atmel_ssc_hw_params(struct snd_pcm_substream *substream,
>   		return -EINVAL;
>   	}
>   
> -	if (!atmel_ssc_cfs(ssc_p)) {
> +	if (atmel_ssc_cfs(ssc_p)) {
> +		/*
> +		 * SSC provides LRCLK
> +		 *
> +		 * Both TF and RF are generated, so use them directly.
> +		 */
> +		rcmr |=	  SSC_BF(RCMR_START, fs_edge);
> +		tcmr |=	  SSC_BF(TCMR_START, fs_edge);
> +	} else {
>   		fslen = fslen_ext = 0;
>   		rcmr_period = tcmr_period = 0;
>   		fs_osync = SSC_FSOS_NONE;
> +		if (!ssc->shared_fs_pin) {
> +			rcmr |=	  SSC_BF(RCMR_START, fs_edge);
> +			tcmr |=	  SSC_BF(TCMR_START, fs_edge);
> +		} else if (ssc->clk_from_rk_pin) {
> +			/* assume RF is to be used when RK is used as BCLK input */
> +			/* Note: won't work correctly on SAMA5D2 due to errata */
> +			rcmr |=	  SSC_BF(RCMR_START, fs_edge);
> +			tcmr |=	  SSC_BF(TCMR_START, SSC_START_RECEIVE);

Did you find a platform in which this mode works?

> +		} else {
> +			rcmr |=	  SSC_BF(RCMR_START, SSC_START_TRANSMIT);
> +			tcmr |=	  SSC_BF(TCMR_START, fs_edge);
> +		}
>   	}
>   
>   	if (atmel_ssc_cbs(ssc_p)) {
> 

I like this feature, just separate the changes in different patches.

Thanks and best regards,
Codrin
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 5/5] ASoC: atmel_ssc_dai: Enable shared FSYNC source in frame-slave mode

[mirq-linux]

On Thu, Jul 25, 2019 at 03:02:34PM +0000, Codrin.Ciubotariu@microchip.com wrote:
> On 22.07.2019 21:27, Michał Mirosław wrote:
> > SSC driver allows only synchronous TX and RX. In slave mode for BCLK
> > it uses only one of TK or RK pin, but for LRCLK it configured separate
> > inputs from TF and RF pins. Allow configuration with common FS signal.
[...]
> > @@ -613,10 +607,30 @@ static int atmel_ssc_hw_params(struct snd_pcm_substream *substream,
> >   		return -EINVAL;
> >   	}
> >   
> > -	if (!atmel_ssc_cfs(ssc_p)) {
> > +	if (atmel_ssc_cfs(ssc_p)) {
> > +		/*
> > +		 * SSC provides LRCLK
> > +		 *
> > +		 * Both TF and RF are generated, so use them directly.
> > +		 */
> > +		rcmr |=	  SSC_BF(RCMR_START, fs_edge);
> > +		tcmr |=	  SSC_BF(TCMR_START, fs_edge);
> > +	} else {
> >   		fslen = fslen_ext = 0;
> >   		rcmr_period = tcmr_period = 0;
> >   		fs_osync = SSC_FSOS_NONE;
> > +		if (!ssc->shared_fs_pin) {
> > +			rcmr |=	  SSC_BF(RCMR_START, fs_edge);
> > +			tcmr |=	  SSC_BF(TCMR_START, fs_edge);
> > +		} else if (ssc->clk_from_rk_pin) {
> > +			/* assume RF is to be used when RK is used as BCLK input */
> > +			/* Note: won't work correctly on SAMA5D2 due to errata */
> > +			rcmr |=	  SSC_BF(RCMR_START, fs_edge);
> > +			tcmr |=	  SSC_BF(TCMR_START, SSC_START_RECEIVE);
> 
> Did you find a platform in which this mode works?

To be exact: according to the errata, TX is delayed improperly. So if you
use only RX (SSC side receives) direction, you're fine.

Best Regards,
Michał Mirosław

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

Re: [PATCH 5/5] ASoC: atmel_ssc_dai: Enable shared FSYNC source in frame-slave mode

[Codrin.Ciubotariu]

On 25.07.2019 21:24, mirq-linux@rere.qmqm.pl wrote:
> External E-Mail
> 
> 
> On Thu, Jul 25, 2019 at 03:02:34PM +0000, Codrin.Ciubotariu@microchip.com wrote:
>> On 22.07.2019 21:27, Michał Mirosław wrote:
>>> SSC driver allows only synchronous TX and RX. In slave mode for BCLK
>>> it uses only one of TK or RK pin, but for LRCLK it configured separate
>>> inputs from TF and RF pins. Allow configuration with common FS signal.
> [...]
>>> @@ -613,10 +607,30 @@ static int atmel_ssc_hw_params(struct snd_pcm_substream *substream,
>>>    		return -EINVAL;
>>>    	}
>>>    
>>> -	if (!atmel_ssc_cfs(ssc_p)) {
>>> +	if (atmel_ssc_cfs(ssc_p)) {
>>> +		/*
>>> +		 * SSC provides LRCLK
>>> +		 *
>>> +		 * Both TF and RF are generated, so use them directly.
>>> +		 */
>>> +		rcmr |=	  SSC_BF(RCMR_START, fs_edge);
>>> +		tcmr |=	  SSC_BF(TCMR_START, fs_edge);
>>> +	} else {
>>>    		fslen = fslen_ext = 0;
>>>    		rcmr_period = tcmr_period = 0;
>>>    		fs_osync = SSC_FSOS_NONE;
>>> +		if (!ssc->shared_fs_pin) {
>>> +			rcmr |=	  SSC_BF(RCMR_START, fs_edge);
>>> +			tcmr |=	  SSC_BF(TCMR_START, fs_edge);
>>> +		} else if (ssc->clk_from_rk_pin) {
>>> +			/* assume RF is to be used when RK is used as BCLK input */
>>> +			/* Note: won't work correctly on SAMA5D2 due to errata */
>>> +			rcmr |=	  SSC_BF(RCMR_START, fs_edge);
>>> +			tcmr |=	  SSC_BF(TCMR_START, SSC_START_RECEIVE);
>>
>> Did you find a platform in which this mode works?
> 
> To be exact: according to the errata, TX is delayed improperly. So if you
> use only RX (SSC side receives) direction, you're fine.

I know, but there are other platforms with SSC, which don't have this 
errata, like sam9x35 or sama5d3. Have you tested this mode, RK input, RF 
input, RD starts on edge detect, TF input, TD starts synchronously with 
receiver?

Best regards,
Codrin

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

Re: [PATCH 5/5] ASoC: atmel_ssc_dai: Enable shared FSYNC source in frame-slave mode

[mirq-linux]

On Fri, Jul 26, 2019 at 10:33:29AM +0000, Codrin.Ciubotariu@microchip.com wrote:
> On 25.07.2019 21:24, mirq-linux@rere.qmqm.pl wrote:
> > On Thu, Jul 25, 2019 at 03:02:34PM +0000, Codrin.Ciubotariu@microchip.com wrote:
> >> On 22.07.2019 21:27, Michał Mirosław wrote:
> >>> SSC driver allows only synchronous TX and RX. In slave mode for BCLK
> >>> it uses only one of TK or RK pin, but for LRCLK it configured separate
> >>> inputs from TF and RF pins. Allow configuration with common FS signal.
> > [...]
> >>> @@ -613,10 +607,30 @@ static int atmel_ssc_hw_params(struct snd_pcm_substream *substream,
> >>>    		return -EINVAL;
> >>>    	}
> >>>    
> >>> -	if (!atmel_ssc_cfs(ssc_p)) {
> >>> +	if (atmel_ssc_cfs(ssc_p)) {
> >>> +		/*
> >>> +		 * SSC provides LRCLK
> >>> +		 *
> >>> +		 * Both TF and RF are generated, so use them directly.
> >>> +		 */
> >>> +		rcmr |=	  SSC_BF(RCMR_START, fs_edge);
> >>> +		tcmr |=	  SSC_BF(TCMR_START, fs_edge);
> >>> +	} else {
> >>>    		fslen = fslen_ext = 0;
> >>>    		rcmr_period = tcmr_period = 0;
> >>>    		fs_osync = SSC_FSOS_NONE;
> >>> +		if (!ssc->shared_fs_pin) {
> >>> +			rcmr |=	  SSC_BF(RCMR_START, fs_edge);
> >>> +			tcmr |=	  SSC_BF(TCMR_START, fs_edge);
> >>> +		} else if (ssc->clk_from_rk_pin) {
> >>> +			/* assume RF is to be used when RK is used as BCLK input */
> >>> +			/* Note: won't work correctly on SAMA5D2 due to errata */
> >>> +			rcmr |=	  SSC_BF(RCMR_START, fs_edge);
> >>> +			tcmr |=	  SSC_BF(TCMR_START, SSC_START_RECEIVE);
> >>
> >> Did you find a platform in which this mode works?
> > 
> > To be exact: according to the errata, TX is delayed improperly. So if you
> > use only RX (SSC side receives) direction, you're fine.
> 
> I know, but there are other platforms with SSC, which don't have this 
> errata, like sam9x35 or sama5d3. Have you tested this mode, RK input, RF 
> input, RD starts on edge detect, TF input, TD starts synchronously with 
> receiver?

No, I have only SAMA5D2 available to test.

Best Regards,
Michał Mirosław

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

[PATCH 4/5] ASoC: atmel_ssc_dai: split TX/RX FS constants

[Michał Mirosław]

The constants are the same, but the names are misleading when used for
TCMR configuration. Use names from SAMA5D2 datasheet.

Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
 sound/soc/atmel/atmel_ssc_dai.c | 6 +++---
 sound/soc/atmel/atmel_ssc_dai.h | 9 ++++++++-
 2 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/sound/soc/atmel/atmel_ssc_dai.c b/sound/soc/atmel/atmel_ssc_dai.c
index 04541d7c33fe..cf2cfc345676 100644
--- a/sound/soc/atmel/atmel_ssc_dai.c
+++ b/sound/soc/atmel/atmel_ssc_dai.c
@@ -572,7 +572,7 @@ static int atmel_ssc_hw_params(struct snd_pcm_substream *substream,
 			| SSC_BF(RCMR_START, SSC_START_RISING_RF);
 
 		tcmr =	  SSC_BF(TCMR_STTDLY, 0)
-			| SSC_BF(TCMR_START, SSC_START_RISING_RF);
+			| SSC_BF(TCMR_START, SSC_START_RISING_TF);
 
 		break;
 
@@ -584,7 +584,7 @@ static int atmel_ssc_hw_params(struct snd_pcm_substream *substream,
 			| SSC_BF(RCMR_START, SSC_START_FALLING_RF);
 
 		tcmr =	  SSC_BF(TCMR_STTDLY, 1)
-			| SSC_BF(TCMR_START, SSC_START_FALLING_RF);
+			| SSC_BF(TCMR_START, SSC_START_FALLING_TF);
 
 		break;
 
@@ -603,7 +603,7 @@ static int atmel_ssc_hw_params(struct snd_pcm_substream *substream,
 			| SSC_BF(RCMR_START, SSC_START_RISING_RF);
 
 		tcmr =	  SSC_BF(TCMR_STTDLY, 1)
-			| SSC_BF(TCMR_START, SSC_START_RISING_RF);
+			| SSC_BF(TCMR_START, SSC_START_RISING_TF);
 
 		break;
 
diff --git a/sound/soc/atmel/atmel_ssc_dai.h b/sound/soc/atmel/atmel_ssc_dai.h
index ae764cb541c7..efb458b6d187 100644
--- a/sound/soc/atmel/atmel_ssc_dai.h
+++ b/sound/soc/atmel/atmel_ssc_dai.h
@@ -42,13 +42,20 @@
  */
 /* START bit field values */
 #define SSC_START_CONTINUOUS	0
-#define SSC_START_TX_RX		1
+#define SSC_START_TRANSMIT	1
+#define SSC_START_RECEIVE	1
 #define SSC_START_LOW_RF	2
+#define SSC_START_LOW_TF	2
 #define SSC_START_HIGH_RF	3
+#define SSC_START_HIGH_TF	3
 #define SSC_START_FALLING_RF	4
+#define SSC_START_FALLING_TF	4
 #define SSC_START_RISING_RF	5
+#define SSC_START_RISING_TF	5
 #define SSC_START_LEVEL_RF	6
+#define SSC_START_LEVEL_TF	6
 #define SSC_START_EDGE_RF	7
+#define SSC_START_EDGE_TF	7
 #define SSS_START_COMPARE_0	8
 
 /* CKI bit field values */
-- 
2.20.1


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

Re: [PATCH 4/5] ASoC: atmel_ssc_dai: split TX/RX FS constants

[Codrin.Ciubotariu]

On 22.07.2019 21:27, Michał Mirosław wrote:
> External E-Mail
> 
> 
> The constants are the same, but the names are misleading when used for
> TCMR configuration. Use names from SAMA5D2 datasheet.
> 
> Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> ---
>   sound/soc/atmel/atmel_ssc_dai.c | 6 +++---
>   sound/soc/atmel/atmel_ssc_dai.h | 9 ++++++++-
>   2 files changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/sound/soc/atmel/atmel_ssc_dai.c b/sound/soc/atmel/atmel_ssc_dai.c
> index 04541d7c33fe..cf2cfc345676 100644
> --- a/sound/soc/atmel/atmel_ssc_dai.c
> +++ b/sound/soc/atmel/atmel_ssc_dai.c
> @@ -572,7 +572,7 @@ static int atmel_ssc_hw_params(struct snd_pcm_substream *substream,
>   			| SSC_BF(RCMR_START, SSC_START_RISING_RF);
>   
>   		tcmr =	  SSC_BF(TCMR_STTDLY, 0)
> -			| SSC_BF(TCMR_START, SSC_START_RISING_RF);
> +			| SSC_BF(TCMR_START, SSC_START_RISING_TF);
>   
>   		break;
>   
> @@ -584,7 +584,7 @@ static int atmel_ssc_hw_params(struct snd_pcm_substream *substream,
>   			| SSC_BF(RCMR_START, SSC_START_FALLING_RF);
>   
>   		tcmr =	  SSC_BF(TCMR_STTDLY, 1)
> -			| SSC_BF(TCMR_START, SSC_START_FALLING_RF);
> +			| SSC_BF(TCMR_START, SSC_START_FALLING_TF);
>   
>   		break;
>   
> @@ -603,7 +603,7 @@ static int atmel_ssc_hw_params(struct snd_pcm_substream *substream,
>   			| SSC_BF(RCMR_START, SSC_START_RISING_RF);
>   
>   		tcmr =	  SSC_BF(TCMR_STTDLY, 1)
> -			| SSC_BF(TCMR_START, SSC_START_RISING_RF);
> +			| SSC_BF(TCMR_START, SSC_START_RISING_TF);
>   
>   		break;
>   
> diff --git a/sound/soc/atmel/atmel_ssc_dai.h b/sound/soc/atmel/atmel_ssc_dai.h
> index ae764cb541c7..efb458b6d187 100644
> --- a/sound/soc/atmel/atmel_ssc_dai.h
> +++ b/sound/soc/atmel/atmel_ssc_dai.h
> @@ -42,13 +42,20 @@
>    */
>   /* START bit field values */
>   #define SSC_START_CONTINUOUS	0
> -#define SSC_START_TX_RX		1
> +#define SSC_START_TRANSMIT	1
> +#define SSC_START_RECEIVE	1
>   #define SSC_START_LOW_RF	2
> +#define SSC_START_LOW_TF	2
>   #define SSC_START_HIGH_RF	3
> +#define SSC_START_HIGH_TF	3
>   #define SSC_START_FALLING_RF	4
> +#define SSC_START_FALLING_TF	4
>   #define SSC_START_RISING_RF	5
> +#define SSC_START_RISING_TF	5
>   #define SSC_START_LEVEL_RF	6
> +#define SSC_START_LEVEL_TF	6
>   #define SSC_START_EDGE_RF	7
> +#define SSC_START_EDGE_TF	7
>   #define SSS_START_COMPARE_0	8
>   
>   /* CKI bit field values */
> 

Wouldn't it be easier to just use SSC_START, SSC_START_LOW, 
SSC_START_HIGH, etc.? If not...

Reviewed-by: Codrin Ciubotariu <codrin.ciubotariu@microchip.com>

Thanks and best regards,
Codrin
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 3/5] ASoC: atmel_ssc_dai: implement left-justified data mode

[Michał Mirosław]

Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
 sound/soc/atmel/atmel_ssc_dai.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/sound/soc/atmel/atmel_ssc_dai.c b/sound/soc/atmel/atmel_ssc_dai.c
index b2992496e52f..04541d7c33fe 100644
--- a/sound/soc/atmel/atmel_ssc_dai.c
+++ b/sound/soc/atmel/atmel_ssc_dai.c
@@ -564,7 +564,20 @@ static int atmel_ssc_hw_params(struct snd_pcm_substream *substream,
 
 	switch (ssc_p->daifmt & SND_SOC_DAIFMT_FORMAT_MASK) {
 
+	case SND_SOC_DAIFMT_LEFT_J:
+		/* left-justified format */
+		fs_osync = SSC_FSOS_POSITIVE;
+
+		rcmr =	  SSC_BF(RCMR_STTDLY, 0)
+			| SSC_BF(RCMR_START, SSC_START_RISING_RF);
+
+		tcmr =	  SSC_BF(TCMR_STTDLY, 0)
+			| SSC_BF(TCMR_START, SSC_START_RISING_RF);
+
+		break;
+
 	case SND_SOC_DAIFMT_I2S:
+		/* I2S format = left-justified with start bit and inverted LRCLK */
 		fs_osync = SSC_FSOS_NEGATIVE;
 
 		rcmr =	  SSC_BF(RCMR_STTDLY, 1)
-- 
2.20.1


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

Re: [PATCH 3/5] ASoC: atmel_ssc_dai: implement left-justified data mode

[Codrin.Ciubotariu]

On 22.07.2019 21:27, Michał Mirosław wrote:
> External E-Mail
> 
> 
> Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> ---
>   sound/soc/atmel/atmel_ssc_dai.c | 13 +++++++++++++
>   1 file changed, 13 insertions(+)
> 
> diff --git a/sound/soc/atmel/atmel_ssc_dai.c b/sound/soc/atmel/atmel_ssc_dai.c
> index b2992496e52f..04541d7c33fe 100644
> --- a/sound/soc/atmel/atmel_ssc_dai.c
> +++ b/sound/soc/atmel/atmel_ssc_dai.c
> @@ -564,7 +564,20 @@ static int atmel_ssc_hw_params(struct snd_pcm_substream *substream,
>   
>   	switch (ssc_p->daifmt & SND_SOC_DAIFMT_FORMAT_MASK) {
>   
> +	case SND_SOC_DAIFMT_LEFT_J:
> +		/* left-justified format */
> +		fs_osync = SSC_FSOS_POSITIVE;
> +
> +		rcmr =	  SSC_BF(RCMR_STTDLY, 0)
> +			| SSC_BF(RCMR_START, SSC_START_RISING_RF);
> +
> +		tcmr =	  SSC_BF(TCMR_STTDLY, 0)
> +			| SSC_BF(TCMR_START, SSC_START_RISING_RF);
> +
> +		break;
> +
>   	case SND_SOC_DAIFMT_I2S:
> +		/* I2S format = left-justified with start bit and inverted LRCLK */
>   		fs_osync = SSC_FSOS_NEGATIVE;
>   
>   		rcmr =	  SSC_BF(RCMR_STTDLY, 1)
> 

Reviewed-by: Codrin Ciubotariu <codrin.ciubotariu@microchip.com>

Thanks and best regards,
Codrin
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel