ASoC: fsl-asoc-card: add wm8960 support
diff mbox

Message ID dc4e5a52a90982cea95445e27a30f2ad6cb73513.1439349623.git.zidan.wang@freescale.com
State New
Headers show

Commit Message

Zidan Wang Aug. 12, 2015, 3:22 a.m. UTC
add wm8960 support for fsl-asoc-card

Signed-off-by: Zidan Wang <zidan.wang@freescale.com>
---
 .../devicetree/bindings/sound/fsl-asoc-card.txt    |  2 ++
 sound/soc/fsl/fsl-asoc-card.c                      | 37 ++++++++++++++++++++--
 2 files changed, 36 insertions(+), 3 deletions(-)

Comments

Nicolin Chen Aug. 12, 2015, 3:42 a.m. UTC | #1
On Wed, Aug 12, 2015 at 11:22:30AM +0800, Zidan Wang wrote:
> add wm8960 support for fsl-asoc-card
> 
> Signed-off-by: Zidan Wang <zidan.wang@freescale.com>

> +	gpr_np = of_parse_phandle(np, "gpr", 0);
> +	if (gpr_np) {
> +		priv->gpr = syscon_node_to_regmap(gpr_np);
> +		if (IS_ERR(priv->gpr)) {
> +			ret = PTR_ERR(priv->gpr);
> +			dev_err(&pdev->dev, "failed to get gpr regmap\n");
> +			goto fail;
> +		}
> +		/*
> +		 * for imx6ul we should set SAI2_MCLK_DIR to enable
> +		 * codec MCLK
> +		 */
> +		if (of_device_is_compatible(np, "fsl,imx6ul-evk-wm8960"))
> +			regmap_update_bits(priv->gpr, 4, 1 << 20, 1 << 20);

What's this SAI2_MCLK_DIR? Can you provide the description of it?

And why not put it inside:
 +	} else if (of_device_is_compatible(np, "fsl,imx-audio-wm8960")) {
 +		priv->card.set_bias_level = fsl_asoc_card_set_bias_level;
 +		priv->codec_priv.fll_id = WM8960_SYSCLK_AUTO;
 +		priv->codec_priv.pll_id = WM8960_SYSCLK_AUTO;
 +		priv->dai_fmt |= SND_SOC_DAIFMT_CBM_CFM;

> @@ -521,7 +545,10 @@ static int fsl_asoc_card_probe(struct platform_device *pdev)
>  	/* Normal DAI Link */
>  	priv->dai_link[0].cpu_of_node = cpu_np;
>  	priv->dai_link[0].codec_of_node = codec_np;
> -	priv->dai_link[0].codec_dai_name = codec_dev->name;
> +	if (strstr(codec_dev->name, "wm8960"))
> +		priv->dai_link[0].codec_dai_name = "wm8960-hifi";
> +	else
> +		priv->dai_link[0].codec_dai_name = codec_dev->name;

What happens to the codec_dev->name over here so that you need to
specify the name?

Nicolin
Zidan Wang Aug. 12, 2015, 4:45 a.m. UTC | #2
On Tue, Aug 11, 2015 at 08:42:59PM -0700, Nicolin Chen wrote:
> On Wed, Aug 12, 2015 at 11:22:30AM +0800, Zidan Wang wrote:
> > add wm8960 support for fsl-asoc-card
> > 
> > Signed-off-by: Zidan Wang <zidan.wang@freescale.com>
> 
> > +	gpr_np = of_parse_phandle(np, "gpr", 0);
> > +	if (gpr_np) {
> > +		priv->gpr = syscon_node_to_regmap(gpr_np);
> > +		if (IS_ERR(priv->gpr)) {
> > +			ret = PTR_ERR(priv->gpr);
> > +			dev_err(&pdev->dev, "failed to get gpr regmap\n");
> > +			goto fail;
> > +		}
> > +		/*
> > +		 * for imx6ul we should set SAI2_MCLK_DIR to enable
> > +		 * codec MCLK
> > +		 */
> > +		if (of_device_is_compatible(np, "fsl,imx6ul-evk-wm8960"))
> > +			regmap_update_bits(priv->gpr, 4, 1 << 20, 1 << 20);
> 
> What's this SAI2_MCLK_DIR? Can you provide the description of it?
SAI2_MCLK_DIR: SD1_CLK data direction control when sai2.MCLK is selected (ALT2)
0 SD1_CLK output driver is disabled when configured for ALT2
1 SD1_CLK output driver is enabled when configured for ALT2

We are using SAI2 now, so we should enable SAI2_MCLK_DIR. If we want to use SAI1 and SAI3, we should also enable SAI1_MCLK_DIR and SAI3_MCLK_DIR. I don't consider SAI1 and SAI3 case, should i add it like below:

gpr = <&gpr reg shift val>

> 
> And why not put it inside:
>  +	} else if (of_device_is_compatible(np, "fsl,imx-audio-wm8960")) {
>  +		priv->card.set_bias_level = fsl_asoc_card_set_bias_level;
>  +		priv->codec_priv.fll_id = WM8960_SYSCLK_AUTO;
>  +		priv->codec_priv.pll_id = WM8960_SYSCLK_AUTO;
>  +		priv->dai_fmt |= SND_SOC_DAIFMT_CBM_CFM;
> 

Just imx6ul should enable such bit. Put inside is also ok.


> > @@ -521,7 +545,10 @@ static int fsl_asoc_card_probe(struct platform_device *pdev)
> >  	/* Normal DAI Link */
> >  	priv->dai_link[0].cpu_of_node = cpu_np;
> >  	priv->dai_link[0].codec_of_node = codec_np;
> > -	priv->dai_link[0].codec_dai_name = codec_dev->name;
> > +	if (strstr(codec_dev->name, "wm8960"))
> > +		priv->dai_link[0].codec_dai_name = "wm8960-hifi";
> > +	else
> > +		priv->dai_link[0].codec_dai_name = codec_dev->name;
> 
> What happens to the codec_dev->name over here so that you need to
> specify the name?
In wm8960 codec driver, codec dai name is "wm8960-hifi". Using codec_dev->name will cauce register card failed.

> 
> Nicolin
Nicolin Chen Aug. 12, 2015, 6:40 a.m. UTC | #3
On Wed, Aug 12, 2015 at 12:45:10PM +0800, Zidan Wang wrote:

> > > +		/*
> > > +		 * for imx6ul we should set SAI2_MCLK_DIR to enable
> > > +		 * codec MCLK
> > > +		 */
> > > +		if (of_device_is_compatible(np, "fsl,imx6ul-evk-wm8960"))
> > > +			regmap_update_bits(priv->gpr, 4, 1 << 20, 1 << 20);

> > What's this SAI2_MCLK_DIR? Can you provide the description of it?
> SAI2_MCLK_DIR: SD1_CLK data direction control when sai2.MCLK is selected (ALT2)
> 0 SD1_CLK output driver is disabled when configured for ALT2
> 1 SD1_CLK output driver is enabled when configured for ALT2

> We are using SAI2 now, so we should enable SAI2_MCLK_DIR. If we want to
> use SAI1 and SAI3, we should also enable SAI1_MCLK_DIR and SAI3_MCLK_DIR.
> I don't consider SAI1 and SAI3 case, should i add it like below:

Yes. Just think about a general interface/configuration that can
satisfy all the MCLK cases you can imagine.

Since it's a clock dealing with SAI, I think it would be better to
put the configuration inside:

494         } else if (strstr(cpu_np->name, "sai")) {
495                 priv->cpu_priv.sysclk_id[1] = FSL_SAI_CLK_MAST1;
496                 priv->cpu_priv.sysclk_id[0] = FSL_SAI_CLK_MAST1;
497         }

And I still don't understand the meaning of this MCLK at all.
Does it indicate SAI's MCLK or an MCLK for external CODEC chips?
Will it be possible that a user needs this MCLK bit to be unset?

Can you share the document to me privately so I can give you a
suggestion in detail?

> > > -	priv->dai_link[0].codec_dai_name = codec_dev->name;
> > > +	if (strstr(codec_dev->name, "wm8960"))
> > > +		priv->dai_link[0].codec_dai_name = "wm8960-hifi";
> > > +	else
> > > +		priv->dai_link[0].codec_dai_name = codec_dev->name;

> > What happens to the codec_dev->name over here so that you need to
> > specify the name?
> In wm8960 codec driver, codec dai name is "wm8960-hifi".
> Using codec_dev->name will cauce register card failed.

When we meet a problem, the first response should be to find out
the route cause instead of fixing it with a work around directly.
If the codec_dev->name caused a card registration failure, you
need to find out why. There must be something wrong with its codec
driver or this sound card driver.

Nicolin
Zidan Wang Aug. 12, 2015, 7:39 a.m. UTC | #4
On Tue, Aug 11, 2015 at 11:40:54PM -0700, Nicolin Chen wrote:
> On Wed, Aug 12, 2015 at 12:45:10PM +0800, Zidan Wang wrote:
> 
> > > > +		/*
> > > > +		 * for imx6ul we should set SAI2_MCLK_DIR to enable
> > > > +		 * codec MCLK
> > > > +		 */
> > > > +		if (of_device_is_compatible(np, "fsl,imx6ul-evk-wm8960"))
> > > > +			regmap_update_bits(priv->gpr, 4, 1 << 20, 1 << 20);
> 
> > > What's this SAI2_MCLK_DIR? Can you provide the description of it?
> > SAI2_MCLK_DIR: SD1_CLK data direction control when sai2.MCLK is selected (ALT2)
> > 0 SD1_CLK output driver is disabled when configured for ALT2
> > 1 SD1_CLK output driver is enabled when configured for ALT2
> 
> > We are using SAI2 now, so we should enable SAI2_MCLK_DIR. If we want to
> > use SAI1 and SAI3, we should also enable SAI1_MCLK_DIR and SAI3_MCLK_DIR.
> > I don't consider SAI1 and SAI3 case, should i add it like below:
> 
> Yes. Just think about a general interface/configuration that can
> satisfy all the MCLK cases you can imagine.
> 
> Since it's a clock dealing with SAI, I think it would be better to
> put the configuration inside:
> 
> 494         } else if (strstr(cpu_np->name, "sai")) {
> 495                 priv->cpu_priv.sysclk_id[1] = FSL_SAI_CLK_MAST1;
> 496                 priv->cpu_priv.sysclk_id[0] = FSL_SAI_CLK_MAST1;
> 497         }
> 
> And I still don't understand the meaning of this MCLK at all.
> Does it indicate SAI's MCLK or an MCLK for external CODEC chips?
> Will it be possible that a user needs this MCLK bit to be unset?
For imx6ul-evk, MCLK for wm8960 CODEC chips and SAI's MCLK is the same pin.
So it should always be set for SAI.
> 
> Can you share the document to me privately so I can give you a
> suggestion in detail?
> 
> > > > -	priv->dai_link[0].codec_dai_name = codec_dev->name;
> > > > +	if (strstr(codec_dev->name, "wm8960"))
> > > > +		priv->dai_link[0].codec_dai_name = "wm8960-hifi";
> > > > +	else
> > > > +		priv->dai_link[0].codec_dai_name = codec_dev->name;
> 
> > > What happens to the codec_dev->name over here so that you need to
> > > specify the name?
> > In wm8960 codec driver, codec dai name is "wm8960-hifi".
> > Using codec_dev->name will cauce register card failed.
> 
> When we meet a problem, the first response should be to find out
> the route cause instead of fixing it with a work around directly.
> If the codec_dev->name caused a card registration failure, you
> need to find out why. There must be something wrong with its codec
> driver or this sound card driver.
> 
The root cause is that the codec dai name is not always be codec_dev->name. It will cause the sound card can't find such codec dai. You can refer to function snd_soc_find_dai().
For wm8962 the codec dai name and codec_dev->name are "wm8962". But some other codec like wm8960, it may have different codec dai name.

> Nicolin
Nicolin Chen Aug. 12, 2015, 9:03 a.m. UTC | #5
On Wed, Aug 12, 2015 at 03:39:00PM +0800, Zidan Wang wrote:

> > > > > -	priv->dai_link[0].codec_dai_name = codec_dev->name;
> > > > > +	if (strstr(codec_dev->name, "wm8960"))
> > > > > +		priv->dai_link[0].codec_dai_name = "wm8960-hifi";
> > > > > +	else
> > > > > +		priv->dai_link[0].codec_dai_name = codec_dev->name;
> > 
> > > > What happens to the codec_dev->name over here so that you need to
> > > > specify the name?
> > > In wm8960 codec driver, codec dai name is "wm8960-hifi".
> > > Using codec_dev->name will cauce register card failed.
> > 
> > When we meet a problem, the first response should be to find out
> > the route cause instead of fixing it with a work around directly.
> > If the codec_dev->name caused a card registration failure, you
> > need to find out why. There must be something wrong with its codec
> > driver or this sound card driver.
> > 
> The root cause is that the codec dai name is not always be codec_dev->name.
> It will cause the sound card can't find such codec dai. You can refer to
> function snd_soc_find_dai().
> For wm8962 the codec dai name and codec_dev->name are "wm8962". But some
> other codec like wm8960, it may have different codec dai name.

Then we should fix this by finding out a better way to get the dai
name. I will look into it tomorrow.
Fabio Estevam Aug. 12, 2015, 12:05 p.m. UTC | #6
On Wed, Aug 12, 2015 at 12:22 AM, Zidan Wang <zidan.wang@freescale.com> wrote:

> --- a/Documentation/devicetree/bindings/sound/fsl-asoc-card.txt
> +++ b/Documentation/devicetree/bindings/sound/fsl-asoc-card.txt
> @@ -59,6 +59,8 @@ Optional properties:
>    - audio-asrc         : The phandle of ASRC. It can be absent if there's no
>                           need to add ASRC support via DPCM.
>
> +  - gpr                        : The gpr handle

This deserves a better explanation.
Nicolin Chen Aug. 12, 2015, 10:27 p.m. UTC | #7
On Wed, Aug 12, 2015 at 03:39:00PM +0800, Zidan Wang wrote:
> On Tue, Aug 11, 2015 at 11:40:54PM -0700, Nicolin Chen wrote:
> > On Wed, Aug 12, 2015 at 12:45:10PM +0800, Zidan Wang wrote:
> > 
> > > > > +		/*
> > > > > +		 * for imx6ul we should set SAI2_MCLK_DIR to enable
> > > > > +		 * codec MCLK
> > > > > +		 */
> > > > > +		if (of_device_is_compatible(np, "fsl,imx6ul-evk-wm8960"))
> > > > > +			regmap_update_bits(priv->gpr, 4, 1 << 20, 1 << 20);
> > 
> > > > What's this SAI2_MCLK_DIR? Can you provide the description of it?
> > > SAI2_MCLK_DIR: SD1_CLK data direction control when sai2.MCLK is selected (ALT2)
> > > 0 SD1_CLK output driver is disabled when configured for ALT2
> > > 1 SD1_CLK output driver is enabled when configured for ALT2
> > 
> > > We are using SAI2 now, so we should enable SAI2_MCLK_DIR. If we want to
> > > use SAI1 and SAI3, we should also enable SAI1_MCLK_DIR and SAI3_MCLK_DIR.
> > > I don't consider SAI1 and SAI3 case, should i add it like below:
> > 
> > Yes. Just think about a general interface/configuration that can
> > satisfy all the MCLK cases you can imagine.

> > And I still don't understand the meaning of this MCLK at all.
> > Does it indicate SAI's MCLK or an MCLK for external CODEC chips?
> > Will it be possible that a user needs this MCLK bit to be unset?

> For imx6ul-evk, MCLK for wm8960 CODEC chips and SAI's MCLK is the same pin.
> So it should always be set for SAI.

Since the MCLK is driven to the external CODEC chip, it should
be similar to the CLKO for wm8962 on the imx6qdl-sabresd board.
So this configuration might be included in the clock tree in my
opinion.

And I Just checked the Reference Manual, it seems the direction
could be totally set to input by leaving this bit unset: MCLK
outputs via the SD1_CLK pad of IOMUX when setting this bit in
the GPR; and MCLK inputs from an external clock source on the
board level when clearing this bit. The SAI chapter mentions
something similar in its MCLK section as well.

But I can't find any clock MUX for the MCLK of SAI by selecting
between the CCM and the external PAD: there is only a MUX inside
the CCM. So I think you need to confirm with the IC team about
this bit and the clock source of SAI MCLK.

And there is a crucial problem I can imagine:
  Is it okay to set this bit while setting SD1_CLK PAD as one
  of other functions rather than SAI MCLK? -- Customers might
  keep your code as they also use SAI2 while using SD1_CLK PAD
  as a GPIO input.

Nicolin
Zidan Wang Aug. 14, 2015, 2:21 a.m. UTC | #8
On Wed, Aug 12, 2015 at 03:27:36PM -0700, Nicolin Chen wrote:
> On Wed, Aug 12, 2015 at 03:39:00PM +0800, Zidan Wang wrote:
> > On Tue, Aug 11, 2015 at 11:40:54PM -0700, Nicolin Chen wrote:
> > > On Wed, Aug 12, 2015 at 12:45:10PM +0800, Zidan Wang wrote:
> > > 
> > > > > > +		/*
> > > > > > +		 * for imx6ul we should set SAI2_MCLK_DIR to enable
> > > > > > +		 * codec MCLK
> > > > > > +		 */
> > > > > > +		if (of_device_is_compatible(np, "fsl,imx6ul-evk-wm8960"))
> > > > > > +			regmap_update_bits(priv->gpr, 4, 1 << 20, 1 << 20);
> > > 
> > > > > What's this SAI2_MCLK_DIR? Can you provide the description of it?
> > > > SAI2_MCLK_DIR: SD1_CLK data direction control when sai2.MCLK is selected (ALT2)
> > > > 0 SD1_CLK output driver is disabled when configured for ALT2
> > > > 1 SD1_CLK output driver is enabled when configured for ALT2
> > > 
> > > > We are using SAI2 now, so we should enable SAI2_MCLK_DIR. If we want to
> > > > use SAI1 and SAI3, we should also enable SAI1_MCLK_DIR and SAI3_MCLK_DIR.
> > > > I don't consider SAI1 and SAI3 case, should i add it like below:
> > > 
> > > Yes. Just think about a general interface/configuration that can
> > > satisfy all the MCLK cases you can imagine.
> 
> > > And I still don't understand the meaning of this MCLK at all.
> > > Does it indicate SAI's MCLK or an MCLK for external CODEC chips?
> > > Will it be possible that a user needs this MCLK bit to be unset?
> 
> > For imx6ul-evk, MCLK for wm8960 CODEC chips and SAI's MCLK is the same pin.
> > So it should always be set for SAI.
> 
> Since the MCLK is driven to the external CODEC chip, it should
> be similar to the CLKO for wm8962 on the imx6qdl-sabresd board.
> So this configuration might be included in the clock tree in my
> opinion.
> 
> And I Just checked the Reference Manual, it seems the direction
> could be totally set to input by leaving this bit unset: MCLK
> outputs via the SD1_CLK pad of IOMUX when setting this bit in
> the GPR; and MCLK inputs from an external clock source on the
> board level when clearing this bit. The SAI chapter mentions
> something similar in its MCLK section as well.
> 
> But I can't find any clock MUX for the MCLK of SAI by selecting
> between the CCM and the external PAD: there is only a MUX inside
> the CCM. So I think you need to confirm with the IC team about
> this bit and the clock source of SAI MCLK.

Don't have such clock MUX. So i think it can't be add to clock tree. 

> 
> And there is a crucial problem I can imagine:
>   Is it okay to set this bit while setting SD1_CLK PAD as one
>   of other functions rather than SAI MCLK? -- Customers might
>   keep your code as they also use SAI2 while using SD1_CLK PAD
>   as a GPIO input.
> 
I just add a gpr node in device tree, if customer want to use SD1_CLK PAD as a GPIO input, remove the gpr node.

Best Regards,
Zidan Wang

> Nicolin
Nicolin Chen Aug. 14, 2015, 3:58 a.m. UTC | #9
On Fri, Aug 14, 2015 at 10:21:18AM +0800, Zidan Wang wrote:

> > And there is a crucial problem I can imagine:
> >   Is it okay to set this bit while setting SD1_CLK PAD as one
> >   of other functions rather than SAI MCLK? -- Customers might
> >   keep your code as they also use SAI2 while using SD1_CLK PAD
> >   as a GPIO input.

> I just add a gpr node in device tree, if customer want to use SD1_CLK
> PAD as a GPIO input, remove the gpr node.

My question was whether setting this bit would cause a hardware
damage -- Customers might not notice your gpr node in the Device
Tree at all, not to mention the possibility of an accidental wrong
configurations.

And another reason I asked that is to find out if we can set these
bits anyway in the SoC level driver of imx6ul when this bit would
not cause anything dangerous and when there's no extra clock MUX
for the SAI MCLK between internal CCM and external clock source.

Nicolin
Zidan Wang Aug. 14, 2015, 7:38 a.m. UTC | #10
On Thu, Aug 13, 2015 at 08:58:06PM -0700, Nicolin Chen wrote:
> On Fri, Aug 14, 2015 at 10:21:18AM +0800, Zidan Wang wrote:
> 
> > > And there is a crucial problem I can imagine:
> > >   Is it okay to set this bit while setting SD1_CLK PAD as one
> > >   of other functions rather than SAI MCLK? -- Customers might
> > >   keep your code as they also use SAI2 while using SD1_CLK PAD
> > >   as a GPIO input.
> 
> > I just add a gpr node in device tree, if customer want to use SD1_CLK
> > PAD as a GPIO input, remove the gpr node.
> 
> My question was whether setting this bit would cause a hardware
> damage -- Customers might not notice your gpr node in the Device
> Tree at all, not to mention the possibility of an accidental wrong
> configurations.
> 
> And another reason I asked that is to find out if we can set these
> bits anyway in the SoC level driver of imx6ul when this bit would
> not cause anything dangerous and when there's no extra clock MUX
> for the SAI MCLK between internal CCM and external clock source.
> 
This bit will not cause any hardware damage. 

ccm ----->1------------------>2<------------>3
	  |                   |
    	  |                   |		
	  4 SAIx MCLK1        5 SAIx MCLK2

If this bit is set, the clock route is ccm->1->2->3, 2 to 5 is disconnect.
If this bit is clear, the clock route is 3->2->5, 1 to 2 is disconnect.

Best Regards,
Zidan Wang

> Nicolin
Nicolin Chen Aug. 14, 2015, 9:22 a.m. UTC | #11
I have to add Shawn for help :)

On Fri, Aug 14, 2015 at 03:38:16PM +0800, Zidan Wang wrote:

> ccm ----->1------------------>2<------------>3
> 	  |                   |
>     	  |                   |		
> 	  4 SAIx MCLK1        5 SAIx MCLK2
> 
> If this bit is set, the clock route is ccm->1->2->3, 2 to 5 is disconnect.
> If this bit is clear, the clock route is 3->2->5, 1 to 2 is disconnect.

@Shawn

Suppose 1 and 2 are clock MUXs that we can't specifically touch
via registers. And 3 is an IO PAD that can either output a clock
from CCM or input a clock from an external source outside SoC.
Both 4 and 5 are two clock sources for one single SAI, SAI2 for
example.

There is a bit in the GPR register to control above routes. And
my question is where should be the best place to put this bit.

Apparently it's outside the CCM and SAI, not to mention it is
hard to relate this bit to this ASoC dai-link driver which's
supposed to solve problems between the SAI and CODECs.

Thanks
Nicolin
Shawn Guo Sept. 6, 2015, 11:33 a.m. UTC | #12
On Fri, Aug 14, 2015 at 02:22:53AM -0700, Nicolin Chen wrote:
> I have to add Shawn for help :)
> 
> On Fri, Aug 14, 2015 at 03:38:16PM +0800, Zidan Wang wrote:
> 
> > ccm ----->1------------------>2<------------>3
> > 	  |                   |
> >     	  |                   |		
> > 	  4 SAIx MCLK1        5 SAIx MCLK2
> > 
> > If this bit is set, the clock route is ccm->1->2->3, 2 to 5 is disconnect.
> > If this bit is clear, the clock route is 3->2->5, 1 to 2 is disconnect.
> 
> @Shawn
> 
> Suppose 1 and 2 are clock MUXs that we can't specifically touch
> via registers. And 3 is an IO PAD that can either output a clock
> from CCM or input a clock from an external source outside SoC.
> Both 4 and 5 are two clock sources for one single SAI, SAI2 for
> example.
> 
> There is a bit in the GPR register to control above routes. And
> my question is where should be the best place to put this bit.

If this route selection is a decision of board design, it should fit
device tree quite well.  As the platform maintainer, I surely hope we
can handle this in audio driver.  But if there is really no place better
than platform code to handle it, I can accept it.

Shawn

> 
> Apparently it's outside the CCM and SAI, not to mention it is
> hard to relate this bit to this ASoC dai-link driver which's
> supposed to solve problems between the SAI and CODECs.
> 
> Thanks
> Nicolin
Nicolin Chen Sept. 17, 2015, 8:03 a.m. UTC | #13
Hi Shawn,

   Sorry for my late reply.

On Sun, Sep 06, 2015 at 07:33:44PM +0800, Shawn Guo wrote:
> On Fri, Aug 14, 2015 at 02:22:53AM -0700, Nicolin Chen wrote:
> > I have to add Shawn for help :)
> > 
> > On Fri, Aug 14, 2015 at 03:38:16PM +0800, Zidan Wang wrote:
> > 
> > > ccm ----->1------------------>2<------------>3
> > > 	  |                   |
> > >     	  |                   |		
> > > 	  4 SAIx MCLK1        5 SAIx MCLK2
> > > 
> > > If this bit is set, the clock route is ccm->1->2->3, 2 to 5 is disconnect.
> > > If this bit is clear, the clock route is 3->2->5, 1 to 2 is disconnect.
> > 
> > @Shawn
> > 
> > Suppose 1 and 2 are clock MUXs that we can't specifically touch
> > via registers. And 3 is an IO PAD that can either output a clock
> > from CCM or input a clock from an external source outside SoC.
> > Both 4 and 5 are two clock sources for one single SAI, SAI2 for
> > example.
> > 
> > There is a bit in the GPR register to control above routes. And
> > my question is where should be the best place to put this bit.
> 
> If this route selection is a decision of board design, it should fit
> device tree quite well.  As the platform maintainer, I surely hope we
> can handle this in audio driver.  But if there is really no place better
> than platform code to handle it, I can accept it.

I understand your concern. This route decides the source of mclk2,

955                         sai1: sai@021d4000 {
956                                 compatible = "fsl,imx6sx-sai";
957                                 reg = <0x021d4000 0x4000>;
958                                 interrupts = <GIC_SPI 97 IRQ_TYPE_LEVEL_HIGH>;
959                                 clocks = <&clks IMX6SX_CLK_SAI1_IPG>,
960                                          <&clks IMX6SX_CLK_SAI1>,
961                                          <&clks 0>, <&clks 0>;
962                                 clock-names = "bus", "mclk1", "mclk2", "mclk3";

So it doesn't make sense to me if putting it in the audio driver as
it then would be out of the clock tree.

I'm not familiar with the current platform code. Is that possible we
put it in the clock tree? Would you please give me and Zidan another
hint for the implementation?

Thanks a lot
Nicolin
Shawn Guo Sept. 18, 2015, 1:19 p.m. UTC | #14
On Thu, Sep 17, 2015 at 01:03:40AM -0700, Nicolin Chen wrote:
> Hi Shawn,
> 
>    Sorry for my late reply.
> 
> On Sun, Sep 06, 2015 at 07:33:44PM +0800, Shawn Guo wrote:
> > On Fri, Aug 14, 2015 at 02:22:53AM -0700, Nicolin Chen wrote:
> > > I have to add Shawn for help :)
> > > 
> > > On Fri, Aug 14, 2015 at 03:38:16PM +0800, Zidan Wang wrote:
> > > 
> > > > ccm ----->1------------------>2<------------>3
> > > > 	  |                   |
> > > >     	  |                   |		
> > > > 	  4 SAIx MCLK1        5 SAIx MCLK2
> > > > 
> > > > If this bit is set, the clock route is ccm->1->2->3, 2 to 5 is disconnect.
> > > > If this bit is clear, the clock route is 3->2->5, 1 to 2 is disconnect.
> > > 
> > > @Shawn
> > > 
> > > Suppose 1 and 2 are clock MUXs that we can't specifically touch
> > > via registers. And 3 is an IO PAD that can either output a clock
> > > from CCM or input a clock from an external source outside SoC.
> > > Both 4 and 5 are two clock sources for one single SAI, SAI2 for
> > > example.
> > > 
> > > There is a bit in the GPR register to control above routes. And
> > > my question is where should be the best place to put this bit.
> > 
> > If this route selection is a decision of board design, it should fit
> > device tree quite well.  As the platform maintainer, I surely hope we
> > can handle this in audio driver.  But if there is really no place better
> > than platform code to handle it, I can accept it.
> 
> I understand your concern. This route decides the source of mclk2,
> 
> 955                         sai1: sai@021d4000 {
> 956                                 compatible = "fsl,imx6sx-sai";
> 957                                 reg = <0x021d4000 0x4000>;
> 958                                 interrupts = <GIC_SPI 97 IRQ_TYPE_LEVEL_HIGH>;
> 959                                 clocks = <&clks IMX6SX_CLK_SAI1_IPG>,
> 960                                          <&clks IMX6SX_CLK_SAI1>,
> 961                                          <&clks 0>, <&clks 0>;
> 962                                 clock-names = "bus", "mclk1", "mclk2", "mclk3";
> 
> So it doesn't make sense to me if putting it in the audio driver as
> it then would be out of the clock tree.
> 
> I'm not familiar with the current platform code. Is that possible we
> put it in the clock tree? Would you please give me and Zidan another
> hint for the implementation?

The platform code we generally mean is the code in arch/arm/mach-imx,
mostly these mach-imx*.c files.  I guess you would need to define a
device tree property to reflect the board design on this mclk2 route,
and parse the property in platform code and set up the GPR register
accordingly.

Shawn
Nicolin Chen Sept. 19, 2015, 5:50 p.m. UTC | #15
On Fri, Sep 18, 2015 at 09:19:18PM +0800, Shawn Guo wrote:
> > > > On Fri, Aug 14, 2015 at 03:38:16PM +0800, Zidan Wang wrote:
> > > > 
> > > > > ccm ----->1------------------>2<------------>3
> > > > > 	  |                   |
> > > > >     	  |                   |		
> > > > > 	  4 SAIx MCLK1        5 SAIx MCLK2
> > > > > 
> > > > > If this bit is set, the clock route is ccm->1->2->3, 2 to 5 is disconnect.
> > > > > If this bit is clear, the clock route is 3->2->5, 1 to 2 is disconnect.
> > > > Suppose 1 and 2 are clock MUXs that we can't specifically touch
> > > > via registers. And 3 is an IO PAD that can either output a clock
> > > > from CCM or input a clock from an external source outside SoC.
> > > > Both 4 and 5 are two clock sources for one single SAI, SAI2 for
> > > > example.
> > > > 
> > > > There is a bit in the GPR register to control above routes. And
> > > > my question is where should be the best place to put this bit.
> > > 
> > > If this route selection is a decision of board design, it should fit
> > > device tree quite well.  As the platform maintainer, I surely hope we
> > > can handle this in audio driver.  But if there is really no place better
> > > than platform code to handle it, I can accept it.
> > 
> > I understand your concern. This route decides the source of mclk2,
> > 
> > 955                         sai1: sai@021d4000 {
> > 956                                 compatible = "fsl,imx6sx-sai";
> > 957                                 reg = <0x021d4000 0x4000>;
> > 958                                 interrupts = <GIC_SPI 97 IRQ_TYPE_LEVEL_HIGH>;
> > 959                                 clocks = <&clks IMX6SX_CLK_SAI1_IPG>,
> > 960                                          <&clks IMX6SX_CLK_SAI1>,
> > 961                                          <&clks 0>, <&clks 0>;
> > 962                                 clock-names = "bus", "mclk1", "mclk2", "mclk3";
> > 
> > So it doesn't make sense to me if putting it in the audio driver as
> > it then would be out of the clock tree.
> > 
> > I'm not familiar with the current platform code. Is that possible we
> > put it in the clock tree? Would you please give me and Zidan another
> > hint for the implementation?
> 
> The platform code we generally mean is the code in arch/arm/mach-imx,
> mostly these mach-imx*.c files.  I guess you would need to define a
> device tree property to reflect the board design on this mclk2 route,
> and parse the property in platform code and set up the GPR register
> accordingly.

I see. I noticed that the clock related files have been moved out of
that directory. But I think we may still be able to register clocks
in the clock files and call a set of clk_set_parent() in the platform
code depending on the DT property right? -- As I'd love to see this
clock route being included in the clock tree so that the mclk2 would
have a corresponding rate in order to normally take part in the bclk
and fsclk calculations.

Patch
diff mbox

diff --git a/Documentation/devicetree/bindings/sound/fsl-asoc-card.txt b/Documentation/devicetree/bindings/sound/fsl-asoc-card.txt
index a96774c..fa0a10d 100644
--- a/Documentation/devicetree/bindings/sound/fsl-asoc-card.txt
+++ b/Documentation/devicetree/bindings/sound/fsl-asoc-card.txt
@@ -59,6 +59,8 @@  Optional properties:
   - audio-asrc		: The phandle of ASRC. It can be absent if there's no
 			  need to add ASRC support via DPCM.
 
+  - gpr			: The gpr handle
+
 Example:
 sound-cs42888 {
 	compatible = "fsl,imx-audio-cs42888";
diff --git a/sound/soc/fsl/fsl-asoc-card.c b/sound/soc/fsl/fsl-asoc-card.c
index de43887..c058e68 100644
--- a/sound/soc/fsl/fsl-asoc-card.c
+++ b/sound/soc/fsl/fsl-asoc-card.c
@@ -16,6 +16,7 @@ 
 #include <linux/of_platform.h>
 #include <sound/pcm_params.h>
 #include <sound/soc.h>
+#include <linux/mfd/syscon.h>
 
 #include "fsl_esai.h"
 #include "fsl_sai.h"
@@ -23,6 +24,7 @@ 
 
 #include "../codecs/sgtl5000.h"
 #include "../codecs/wm8962.h"
+#include "../codecs/wm8960.h"
 
 #define RX 0
 #define TX 1
@@ -90,6 +92,7 @@  struct fsl_asoc_card_priv {
 	u32 asrc_format;
 	u32 dai_fmt;
 	char name[32];
+	struct regmap *gpr;
 };
 
 /**
@@ -400,7 +403,7 @@  static int fsl_asoc_card_late_probe(struct snd_soc_card *card)
 
 static int fsl_asoc_card_probe(struct platform_device *pdev)
 {
-	struct device_node *cpu_np, *codec_np, *asrc_np;
+	struct device_node *cpu_np, *codec_np, *asrc_np, *gpr_np;
 	struct device_node *np = pdev->dev.of_node;
 	struct platform_device *asrc_pdev = NULL;
 	struct platform_device *cpu_pdev;
@@ -443,6 +446,22 @@  static int fsl_asoc_card_probe(struct platform_device *pdev)
 	if (asrc_np)
 		asrc_pdev = of_find_device_by_node(asrc_np);
 
+	gpr_np = of_parse_phandle(np, "gpr", 0);
+	if (gpr_np) {
+		priv->gpr = syscon_node_to_regmap(gpr_np);
+		if (IS_ERR(priv->gpr)) {
+			ret = PTR_ERR(priv->gpr);
+			dev_err(&pdev->dev, "failed to get gpr regmap\n");
+			goto fail;
+		}
+		/*
+		 * for imx6ul we should set SAI2_MCLK_DIR to enable
+		 * codec MCLK
+		 */
+		if (of_device_is_compatible(np, "fsl,imx6ul-evk-wm8960"))
+			regmap_update_bits(priv->gpr, 4, 1 << 20, 1 << 20);
+	}
+
 	/* Get the MCLK rate only, and leave it controlled by CODEC drivers */
 	codec_clk = clk_get(&codec_dev->dev, NULL);
 	if (!IS_ERR(codec_clk)) {
@@ -475,6 +494,11 @@  static int fsl_asoc_card_probe(struct platform_device *pdev)
 		priv->codec_priv.fll_id = WM8962_SYSCLK_FLL;
 		priv->codec_priv.pll_id = WM8962_FLL;
 		priv->dai_fmt |= SND_SOC_DAIFMT_CBM_CFM;
+	} else if (of_device_is_compatible(np, "fsl,imx-audio-wm8960")) {
+		priv->card.set_bias_level = fsl_asoc_card_set_bias_level;
+		priv->codec_priv.fll_id = WM8960_SYSCLK_AUTO;
+		priv->codec_priv.pll_id = WM8960_SYSCLK_AUTO;
+		priv->dai_fmt |= SND_SOC_DAIFMT_CBM_CFM;
 	} else {
 		dev_err(&pdev->dev, "unknown Device Tree compatible\n");
 		return -EINVAL;
@@ -521,7 +545,10 @@  static int fsl_asoc_card_probe(struct platform_device *pdev)
 	/* Normal DAI Link */
 	priv->dai_link[0].cpu_of_node = cpu_np;
 	priv->dai_link[0].codec_of_node = codec_np;
-	priv->dai_link[0].codec_dai_name = codec_dev->name;
+	if (strstr(codec_dev->name, "wm8960"))
+		priv->dai_link[0].codec_dai_name = "wm8960-hifi";
+	else
+		priv->dai_link[0].codec_dai_name = codec_dev->name;
 	priv->dai_link[0].platform_of_node = cpu_np;
 	priv->dai_link[0].dai_fmt = priv->dai_fmt;
 	priv->card.num_links = 1;
@@ -530,7 +557,10 @@  static int fsl_asoc_card_probe(struct platform_device *pdev)
 		/* DPCM DAI Links only if ASRC exsits */
 		priv->dai_link[1].cpu_of_node = asrc_np;
 		priv->dai_link[1].platform_of_node = asrc_np;
-		priv->dai_link[2].codec_dai_name = codec_dev->name;
+		if (strstr(codec_dev->name, "wm8960"))
+			priv->dai_link[2].codec_dai_name = "wm8960-hifi";
+		else
+			priv->dai_link[2].codec_dai_name = codec_dev->name;
 		priv->dai_link[2].codec_of_node = codec_np;
 		priv->dai_link[2].cpu_of_node = cpu_np;
 		priv->dai_link[2].dai_fmt = priv->dai_fmt;
@@ -578,6 +608,7 @@  static const struct of_device_id fsl_asoc_card_dt_ids[] = {
 	{ .compatible = "fsl,imx-audio-cs42888", },
 	{ .compatible = "fsl,imx-audio-sgtl5000", },
 	{ .compatible = "fsl,imx-audio-wm8962", },
+	{ .compatible = "fsl,imx-audio-wm8960", },
 	{}
 };