On Sun, Sep 10, 2017 at 08:32:59AM +0530, Naveen M wrote:

rt5663 needs mclk/sclk early to synchronize its internal clocks. Clocks are enabled in hw_params and disabled in platform_clk_control as the codec requires mclk/bclk need to turned ON before set_sysclk call for a successful clock synchronization.

This would be dependent on the series Subhransu posted right? If so please do mention that

+static int kabylake_enable_ssp_clks(struct snd_soc_card *card) +{

we don't need the empty line here

• struct kbl_codec_private *priv = snd_soc_card_get_drvdata(card);
• int ret;
• /* Enable MCLK */
• ret = clk_set_rate(priv->mclk, 24000000);
• if (ret < 0) {

• dev_err(card->dev, "Can't set rate for mclk, err: %d\n", ret);
  

• return ret;
  

• }
• ret = clk_prepare_enable(priv->mclk);
• if (ret < 0) {

• dev_err(card->dev, "Can't enable mclk, err: %d\n", ret);
  

• return ret;
  

• }
• /* Enable SCLK */
• ret = clk_set_rate(priv->sclk, 3072000);

why do we need both mclk and sclk for this codec? One should be enough for codec to sync

• if (ret < 0) {

• dev_err(card->dev, "Can't set rate for sclk, err: %d\n", ret);
  

• clk_disable_unprepare(priv->mclk);
  

• return ret;
  

• }
• ret = clk_prepare_enable(priv->sclk);
• if (ret < 0) {

• dev_err(card->dev, "Can't enable sclk, err: %d\n", ret);
  

• clk_disable_unprepare(priv->mclk);
  

• }
• return 0;

this looks wrong to me, shouldn't this be return ret;

@@ -647,6 +710,7 @@ static int kabylake_audio_probe(struct platform_device *pdev) { struct kbl_codec_private *ctx; struct skl_machine_pdata *pdata;

• int ret = 0;

  ctx = devm_kzalloc(&pdev->dev, sizeof(*ctx), GFP_ATOMIC); if (!ctx)

@@ -662,6 +726,34 @@ static int kabylake_audio_probe(struct platform_device *pdev) dmic_constraints = pdata->dmic_num == 2 ? &constraints_dmic_2ch : &constraints_dmic_channels;

• ctx->mclk = devm_clk_get(&pdev->dev, "ssp1_mclk");
• if (IS_ERR(ctx->mclk)) {

• ret = PTR_ERR(ctx->mclk);
  

• if (ret == -ENOENT) {
  

• 	dev_info(&pdev->dev,
  

• 		"Failed to get ssp1_mclk, defer probe\n");
  

can you check this, I think driver core emits warnings as well, so do we need it here?

• 	return -EPROBE_DEFER;
  

• }
  

• dev_err(&pdev->dev, "Failed to get ssp1_mclk with err:%d\n",
  

• 						ret);
  

• return ret;
  

• }
• ctx->sclk = devm_clk_get(&pdev->dev, "ssp1_sclk");
• if (IS_ERR(ctx->sclk)) {

• ret = PTR_ERR(ctx->sclk);
  

• if (ret == -ENOENT) {
  

• 	dev_info(&pdev->dev,
  

• 		"Failed to get ssp1_sclk, defer probe\n");
  

shouldn't the mclk ref be given back?