[PATCH 0/3] ASoC: atmel_ssc_dai: add option to choose clock

[PATCH 0/3] ASoC: atmel_ssc_dai: add option to choose clock

[Bo Shen]

When SSC work in slave mode, the clock can come from TK pin and also
can come from RK pin, this is hardware design decided. So, make it
available to choose where the clock from.


Bo Shen (3):
  ASoC: atmel_ssc_dai: make option to choose clock
  ASoC: atmel_wm8904: make it available to choose clock
  Binding: atmel-wm8904: add option to choose clock

 Documentation/devicetree/bindings/sound/atmel-wm8904.txt |  2 ++
 sound/soc/atmel/atmel_ssc_dai.c                          | 16 ++++++++++++----
 sound/soc/atmel/atmel_ssc_dai.h                          |  1 +
 sound/soc/atmel/atmel_wm8904.c                           | 10 ++++++++++
 4 files changed, 25 insertions(+), 4 deletions(-)

-- 
1.8.5.2

[PATCH 1/3] ASoC: atmel_ssc_dai: make option to choose clock

[Bo Shen]

When SSC works in slave mode, according to the hardware design, the
clock can get from TK pin, also can get from RK pin. So, add one
parameter to choose where the clock from.

Signed-off-by: Bo Shen <voice.shen@atmel.com>
---

 sound/soc/atmel/atmel_ssc_dai.c | 16 ++++++++++++----
 sound/soc/atmel/atmel_ssc_dai.h |  1 +
 2 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/sound/soc/atmel/atmel_ssc_dai.c b/sound/soc/atmel/atmel_ssc_dai.c
index 8697ced..03eb0be 100644
--- a/sound/soc/atmel/atmel_ssc_dai.c
+++ b/sound/soc/atmel/atmel_ssc_dai.c
@@ -340,6 +340,7 @@ static int atmel_ssc_hw_params(struct snd_pcm_substream *substream,
 	struct snd_soc_dai *dai)
 {
 	int id = dai->id;
+	struct snd_soc_card *card = dai->card;
 	struct atmel_ssc_info *ssc_p = &ssc_info[id];
 	struct atmel_pcm_dma_params *dma_params;
 	int dir, channels, bits;
@@ -347,6 +348,9 @@ static int atmel_ssc_hw_params(struct snd_pcm_substream *substream,
 	int start_event;
 	int ret;
 
+	ssc_p->clk_from_rk_pin =
+		((struct atmel_ssc_info *)(card->drvdata))->clk_from_rk_pin;
+
 	/*
 	 * Currently, there is only one set of dma params for
 	 * each direction.  If more are added, this code will
@@ -466,7 +470,8 @@ static int atmel_ssc_hw_params(struct snd_pcm_substream *substream,
 			| SSC_BF(RCMR_START, start_event)
 			| SSC_BF(RCMR_CKI, SSC_CKI_RISING)
 			| SSC_BF(RCMR_CKO, SSC_CKO_NONE)
-			| SSC_BF(RCMR_CKS, SSC_CKS_CLOCK);
+			| SSC_BF(RCMR_CKS, ssc_p->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)
@@ -481,7 +486,8 @@ static int atmel_ssc_hw_params(struct snd_pcm_substream *substream,
 			| SSC_BF(TCMR_START, start_event)
 			| SSC_BF(TCMR_CKI, SSC_CKI_FALLING)
 			| SSC_BF(TCMR_CKO, SSC_CKO_NONE)
-			| SSC_BF(TCMR_CKS, SSC_CKS_PIN);
+			| SSC_BF(TCMR_CKS, ssc_p->clk_from_rk_pin ?
+					   SSC_CKS_CLOCK : SSC_CKS_PIN);
 
 		tfmr =	  SSC_BF(TFMR_FSEDGE, SSC_FSEDGE_POSITIVE)
 			| SSC_BF(TFMR_FSDEN, 0)
@@ -550,7 +556,8 @@ static int atmel_ssc_hw_params(struct snd_pcm_substream *substream,
 			| 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_PIN);
+			| SSC_BF(RCMR_CKS, ssc_p->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)
@@ -565,7 +572,8 @@ static int atmel_ssc_hw_params(struct snd_pcm_substream *substream,
 			| SSC_BF(TCMR_START, SSC_START_RISING_RF)
 			| SSC_BF(TCMR_CKI, SSC_CKI_FALLING)
 			| SSC_BF(TCMR_CKO, SSC_CKO_NONE)
-			| SSC_BF(TCMR_CKS, SSC_CKS_PIN);
+			| SSC_BF(RCMR_CKS, ssc_p->clk_from_rk_pin ?
+					   SSC_CKS_CLOCK : SSC_CKS_PIN);
 
 		tfmr =	  SSC_BF(TFMR_FSEDGE, SSC_FSEDGE_POSITIVE)
 			| SSC_BF(TFMR_FSDEN, 0)
diff --git a/sound/soc/atmel/atmel_ssc_dai.h b/sound/soc/atmel/atmel_ssc_dai.h
index b1f08d5..f147895 100644
--- a/sound/soc/atmel/atmel_ssc_dai.h
+++ b/sound/soc/atmel/atmel_ssc_dai.h
@@ -113,6 +113,7 @@ struct atmel_ssc_info {
 	unsigned short cmr_div;
 	unsigned short tcmr_period;
 	unsigned short rcmr_period;
+	bool clk_from_rk_pin;
 	struct atmel_pcm_dma_params *dma_params[2];
 	struct atmel_ssc_state ssc_state;
 };
-- 
1.8.5.2

[PATCH 2/3] ASoC: atmel_wm8904: make it available to choose clock

[Bo Shen]

Make it available to choose the clock from TK pin or RK pin. This
is hardware design decided.

Signed-off-by: Bo Shen <voice.shen@atmel.com>
---

 sound/soc/atmel/atmel_wm8904.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/sound/soc/atmel/atmel_wm8904.c b/sound/soc/atmel/atmel_wm8904.c
index b4e3690..b85088d 100644
--- a/sound/soc/atmel/atmel_wm8904.c
+++ b/sound/soc/atmel/atmel_wm8904.c
@@ -108,6 +108,7 @@ static int atmel_asoc_wm8904_dt_init(struct platform_device *pdev)
 	struct device_node *codec_np, *cpu_np;
 	struct snd_soc_card *card = &atmel_asoc_wm8904_card;
 	struct snd_soc_dai_link *dailink = &atmel_asoc_wm8904_dailink;
+	struct atmel_ssc_info *ssc_info;
 	int ret;
 
 	if (!np) {
@@ -115,6 +116,15 @@ static int atmel_asoc_wm8904_dt_init(struct platform_device *pdev)
 		return -EINVAL;
 	}
 
+	ssc_info = devm_kzalloc(&pdev->dev, sizeof(*ssc_info), GFP_KERNEL);
+	if (!ssc_info)
+		return -ENOMEM;
+
+	ssc_info->clk_from_rk_pin =
+		of_property_read_bool(np, "clk_from_rk_pin");
+
+	card->drvdata = (void *)ssc_info;
+
 	ret = snd_soc_of_parse_card_name(card, "atmel,model");
 	if (ret) {
 		dev_err(&pdev->dev, "failed to parse card name\n");
-- 
1.8.5.2

[PATCH 3/3] Binding: atmel-wm8904: add option to choose clock

[Bo Shen]

Add the option to choose clock output on which pin connect to SSC.
Default is on TK pin to SSC, add clk_from_rk_pin option, the clock
is on RK pin to SSC.

Signed-off-by: Bo Shen <voice.shen@atmel.com>
---

 Documentation/devicetree/bindings/sound/atmel-wm8904.txt | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/sound/atmel-wm8904.txt b/Documentation/devicetree/bindings/sound/atmel-wm8904.txt
index 8bbe50c..68a5c1a 100644
--- a/Documentation/devicetree/bindings/sound/atmel-wm8904.txt
+++ b/Documentation/devicetree/bindings/sound/atmel-wm8904.txt
@@ -33,6 +33,8 @@ Required properties:
 
 Optional properties:
   - pinctrl-names, pinctrl-0: Please refer to pinctrl-bindings.txt
+  - clk_from_rk_pin: according to hardware design, clk privide on rk pin
+    to ssc device
 
 Example:
 sound {
-- 
1.8.5.2

Re: [PATCH 2/3] ASoC: atmel_wm8904: make it available to choose clock

[Mark Brown]

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

On Tue, Jan 14, 2014 at 11:25:55AM +0800, Bo Shen wrote:
> Make it available to choose the clock from TK pin or RK pin. This
> is hardware design decided.

> --- a/sound/soc/atmel/atmel_wm8904.c
> +++ b/sound/soc/atmel/atmel_wm8904.c
> @@ -108,6 +108,7 @@ static int atmel_asoc_wm8904_dt_init(struct platform_device *pdev)
>  	struct device_node *codec_np, *cpu_np;
>  	struct snd_soc_card *card = &atmel_asoc_wm8904_card;
>  	struct snd_soc_dai_link *dailink = &atmel_asoc_wm8904_dailink;
> +	struct atmel_ssc_info *ssc_info;
>  	int ret;
>  
>  	if (!np) {
> @@ -115,6 +116,15 @@ static int atmel_asoc_wm8904_dt_init(struct platform_device *pdev)
>  		return -EINVAL;
>  	}
>  
> +	ssc_info = devm_kzalloc(&pdev->dev, sizeof(*ssc_info), GFP_KERNEL);
> +	if (!ssc_info)
> +		return -ENOMEM;
> +
> +	ssc_info->clk_from_rk_pin =
> +		of_property_read_bool(np, "clk_from_rk_pin");
> +
> +	card->drvdata = (void *)ssc_info;

Shouldn't this code be in the DAI driver?  Otherwise this series looks
fine to me, though the DT folks might have something to say I guess.

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

Re: [PATCH 3/3] Binding: atmel-wm8904: add option to choose clock

[Jean-Christophe PLAGNIOL-VILLARD]

On 11:25 Tue 14 Jan     , Bo Shen wrote:
> Add the option to choose clock output on which pin connect to SSC.
> Default is on TK pin to SSC, add clk_from_rk_pin option, the clock
> is on RK pin to SSC.
> 
> Signed-off-by: Bo Shen <voice.shen@atmel.com>
> ---
> 
>  Documentation/devicetree/bindings/sound/atmel-wm8904.txt | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/sound/atmel-wm8904.txt b/Documentation/devicetree/bindings/sound/atmel-wm8904.txt
> index 8bbe50c..68a5c1a 100644
> --- a/Documentation/devicetree/bindings/sound/atmel-wm8904.txt
> +++ b/Documentation/devicetree/bindings/sound/atmel-wm8904.txt
> @@ -33,6 +33,8 @@ Required properties:
>  
>  Optional properties:
>    - pinctrl-names, pinctrl-0: Please refer to pinctrl-bindings.txt
> +  - clk_from_rk_pin: according to hardware design, clk privide on rk pin
							  provide

	?? no example?
> +    to ssc device
>  
>  Example:
>  sound {
> -- 
> 1.8.5.2
> 

Re: [PATCH 2/3] ASoC: atmel_wm8904: make it available to choose clock

[Nicolas Ferre]

On 14/01/2014 04:25, Bo Shen :
> Make it available to choose the clock from TK pin or RK pin. This
> is hardware design decided.
> 
> Signed-off-by: Bo Shen <voice.shen@atmel.com>
> ---
> 
>  sound/soc/atmel/atmel_wm8904.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/sound/soc/atmel/atmel_wm8904.c b/sound/soc/atmel/atmel_wm8904.c
> index b4e3690..b85088d 100644
> --- a/sound/soc/atmel/atmel_wm8904.c
> +++ b/sound/soc/atmel/atmel_wm8904.c
> @@ -108,6 +108,7 @@ static int atmel_asoc_wm8904_dt_init(struct platform_device *pdev)
>  	struct device_node *codec_np, *cpu_np;
>  	struct snd_soc_card *card = &atmel_asoc_wm8904_card;
>  	struct snd_soc_dai_link *dailink = &atmel_asoc_wm8904_dailink;
> +	struct atmel_ssc_info *ssc_info;
>  	int ret;
>  
>  	if (!np) {
> @@ -115,6 +116,15 @@ static int atmel_asoc_wm8904_dt_init(struct platform_device *pdev)
>  		return -EINVAL;
>  	}
>  
> +	ssc_info = devm_kzalloc(&pdev->dev, sizeof(*ssc_info), GFP_KERNEL);

Isn't an atmel_ssc_info structure table already instantiated in
sound/soc/atmel/atmel_ssc_dai.c ...
I see, you copy the information contained in this field in the proper
ssc_info of the DAI in the previous patch... Well, isn't it a better way
to pass parameters to the DAI than this one?


> +	if (!ssc_info)
> +		return -ENOMEM;
> +
> +	ssc_info->clk_from_rk_pin =
> +		of_property_read_bool(np, "clk_from_rk_pin");
> +
> +	card->drvdata = (void *)ssc_info;
> +
>  	ret = snd_soc_of_parse_card_name(card, "atmel,model");
>  	if (ret) {
>  		dev_err(&pdev->dev, "failed to parse card name\n");
> 


-- 
Nicolas Ferre

Re: [PATCH 3/3] Binding: atmel-wm8904: add option to choose clock

[Nicolas Ferre]

On 14/01/2014 04:25, Bo Shen :
> Add the option to choose clock output on which pin connect to SSC.
> Default is on TK pin to SSC, add clk_from_rk_pin option, the clock

Please do not use "_" in DT properties. It is a common pattern to use
"-" instead.

> is on RK pin to SSC.
> 
> Signed-off-by: Bo Shen <voice.shen@atmel.com>
> ---
> 
>  Documentation/devicetree/bindings/sound/atmel-wm8904.txt | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/sound/atmel-wm8904.txt b/Documentation/devicetree/bindings/sound/atmel-wm8904.txt
> index 8bbe50c..68a5c1a 100644
> --- a/Documentation/devicetree/bindings/sound/atmel-wm8904.txt
> +++ b/Documentation/devicetree/bindings/sound/atmel-wm8904.txt
> @@ -33,6 +33,8 @@ Required properties:
>  
>  Optional properties:
>    - pinctrl-names, pinctrl-0: Please refer to pinctrl-bindings.txt
> +  - clk_from_rk_pin: according to hardware design, clk privide on rk pin

typo + more description is needed:
Please tell that it is a boolean property, that by default it is using
the TK pin (like in the commit message in fact).

Bye,

> +    to ssc device
>  
>  Example:
>  sound {
> 


-- 
Nicolas Ferre

Re: [PATCH 2/3] ASoC: atmel_wm8904: make it available to choose clock

[Bo Shen]

Hi Mark,

On 01/15/2014 04:36 AM, Mark Brown wrote:
> On Tue, Jan 14, 2014 at 11:25:55AM +0800, Bo Shen wrote:
>> Make it available to choose the clock from TK pin or RK pin. This
>> is hardware design decided.
>
>> --- a/sound/soc/atmel/atmel_wm8904.c
>> +++ b/sound/soc/atmel/atmel_wm8904.c
>> @@ -108,6 +108,7 @@ static int atmel_asoc_wm8904_dt_init(struct platform_device *pdev)
>>   	struct device_node *codec_np, *cpu_np;
>>   	struct snd_soc_card *card = &atmel_asoc_wm8904_card;
>>   	struct snd_soc_dai_link *dailink = &atmel_asoc_wm8904_dailink;
>> +	struct atmel_ssc_info *ssc_info;
>>   	int ret;
>>
>>   	if (!np) {
>> @@ -115,6 +116,15 @@ static int atmel_asoc_wm8904_dt_init(struct platform_device *pdev)
>>   		return -EINVAL;
>>   	}
>>
>> +	ssc_info = devm_kzalloc(&pdev->dev, sizeof(*ssc_info), GFP_KERNEL);
>> +	if (!ssc_info)
>> +		return -ENOMEM;
>> +
>> +	ssc_info->clk_from_rk_pin =
>> +		of_property_read_bool(np, "clk_from_rk_pin");
>> +
>> +	card->drvdata = (void *)ssc_info;
>
> Shouldn't this code be in the DAI driver?  Otherwise this series looks
> fine to me, though the DT folks might have something to say I guess.

   For audio on Atmel SoC, it depends on three device nodes, one is SSC 
node, one is the codec node and the sound node.
   The sound node will parse by machine driver, and machine driver is 
mainly for hardware connection. As the "clk_from_rk_pin" is decided by 
hardware, so, I put it here.
   If I move the code to dai driver, it will parse the sound node in dai 
driver, I think it will make the code a little bit not explicit. What do 
you think?

Best Regards,
Bo Shen

Re: [PATCH 3/3] Binding: atmel-wm8904: add option to choose clock

[Bo Shen]

Hi J,

On 01/15/2014 07:54 PM, Jean-Christophe PLAGNIOL-VILLARD wrote:
> On 11:25 Tue 14 Jan     , Bo Shen wrote:
>> Add the option to choose clock output on which pin connect to SSC.
>> Default is on TK pin to SSC, add clk_from_rk_pin option, the clock
>> is on RK pin to SSC.
>>
>> Signed-off-by: Bo Shen <voice.shen@atmel.com>
>> ---
>>
>>   Documentation/devicetree/bindings/sound/atmel-wm8904.txt | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/sound/atmel-wm8904.txt b/Documentation/devicetree/bindings/sound/atmel-wm8904.txt
>> index 8bbe50c..68a5c1a 100644
>> --- a/Documentation/devicetree/bindings/sound/atmel-wm8904.txt
>> +++ b/Documentation/devicetree/bindings/sound/atmel-wm8904.txt
>> @@ -33,6 +33,8 @@ Required properties:
>>
>>   Optional properties:
>>     - pinctrl-names, pinctrl-0: Please refer to pinctrl-bindings.txt
>> +  - clk_from_rk_pin: according to hardware design, clk privide on rk pin
> 							  provide
>
> 	?? no example?

If this patch set is acceptable. The sama5 audio will use it (I will 
send patch after this patch set). Do I still need to put the example in 
the binding document?

>> +    to ssc device
>>
>>   Example:
>>   sound {
>> --
>> 1.8.5.2
>>

Best Regards,
Bo Shen

Re: [PATCH 3/3] Binding: atmel-wm8904: add option to choose clock

[Bo Shen]

Hi Nicolas,

On 01/16/2014 01:17 AM, Nicolas Ferre wrote:
> On 14/01/2014 04:25, Bo Shen :
>> Add the option to choose clock output on which pin connect to SSC.
>> Default is on TK pin to SSC, add clk_from_rk_pin option, the clock
>
> Please do not use "_" in DT properties. It is a common pattern to use
> "-" instead.

OK, I will use "-" instead in next version.

>> is on RK pin to SSC.
>>
>> Signed-off-by: Bo Shen <voice.shen@atmel.com>
>> ---
>>
>>   Documentation/devicetree/bindings/sound/atmel-wm8904.txt | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/sound/atmel-wm8904.txt b/Documentation/devicetree/bindings/sound/atmel-wm8904.txt
>> index 8bbe50c..68a5c1a 100644
>> --- a/Documentation/devicetree/bindings/sound/atmel-wm8904.txt
>> +++ b/Documentation/devicetree/bindings/sound/atmel-wm8904.txt
>> @@ -33,6 +33,8 @@ Required properties:
>>
>>   Optional properties:
>>     - pinctrl-names, pinctrl-0: Please refer to pinctrl-bindings.txt
>> +  - clk_from_rk_pin: according to hardware design, clk privide on rk pin
>
> typo + more description is needed:
> Please tell that it is a boolean property, that by default it is using
> the TK pin (like in the commit message in fact).

OK, I will add more description for it.

Thanks

> Bye,
>
>> +    to ssc device
>>
>>   Example:
>>   sound {
>>
>
>

Best Regards,
Bo Shen

Re: [PATCH 3/3] Binding: atmel-wm8904: add option to choose clock

[Mark Brown]

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

On Thu, Jan 16, 2014 at 09:33:20AM +0800, Bo Shen wrote:
> On 01/15/2014 07:54 PM, Jean-Christophe PLAGNIOL-VILLARD wrote:

> >>  Optional properties:
> >>    - pinctrl-names, pinctrl-0: Please refer to pinctrl-bindings.txt
> >>+  - clk_from_rk_pin: according to hardware design, clk privide on rk pin
> >							  provide

> >	?? no example?

> If this patch set is acceptable. The sama5 audio will use it (I will
> send patch after this patch set). Do I still need to put the example
> in the binding document?

It doesn't seem terribly worthwhile to have the example include every
boolean property, it's not like people should find it hard to work out
how to use them.

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

Re: [PATCH 2/3] ASoC: atmel_wm8904: make it available to choose clock

[Mark Brown]

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

On Thu, Jan 16, 2014 at 09:31:23AM +0800, Bo Shen wrote:

> >Shouldn't this code be in the DAI driver?  Otherwise this series looks
> >fine to me, though the DT folks might have something to say I guess.

>   For audio on Atmel SoC, it depends on three device nodes, one is
> SSC node, one is the codec node and the sound node.
>   The sound node will parse by machine driver, and machine driver is
> mainly for hardware connection. As the "clk_from_rk_pin" is decided
> by hardware, so, I put it here.
>   If I move the code to dai driver, it will parse the sound node in
> dai driver, I think it will make the code a little bit not explicit.
> What do you think?

I think it should just be a property of the DAI device.  It's true that
the card defines the connections but if something is going to be an
option that's there for most if not all systems then putting it in the
device driver means less effort on each integration.

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