Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20181124110948.209019-1-cychiang@chromium.org>
 <20181124110948.209019-3-cychiang@chromium.org> <4f8debc0-40f3-6195-8acb-de9ae3335671@linaro.org>
In-Reply-To: <4f8debc0-40f3-6195-8acb-de9ae3335671@linaro.org>
From:   Cheng-yi Chiang <cychiang@chromium.org>
Date:   Wed, 28 Nov 2018 18:56:22 +0800
Message-ID: <CAFv8NwLQGkJttDeLhmg+LPBO222khnprVxaX2VQyi6JMHx=svw@mail.gmail.com>
Subject: Re: [alsa-devel] [PATCH 3/4] ASoC: qcom: sdm845: Add codec related
 configuration for sdm845
To:     Srini Kandagatla <srinivas.kandagatla@linaro.org>
Cc:     linux-kernel@vger.kernel.org, Oder Chiou <oder_chiou@realtek.com>,
        alsa-devel@alsa-project.org, tzungbi@chromium.org,
        Mark Brown <broonie@kernel.org>, rohitkr@codeaurora.org,
        Dylan Reid <dgreid@chromium.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue, Nov 27, 2018 at 5:32 PM Srinivas Kandagatla
<srinivas.kandagatla@linaro.org> wrote:
>
> Thanks for the patch Jimmy,
>
Hi Srini,
Thanks for the review!
I have updated the patch series in v2.

> On 24/11/18 11:09, Cheng-Yi Chiang wrote:
> > Set TDM time slots and DAI format for speaker codec.
> > Set DAI format and clock for headset. >
> > Signed-off-by: Rohit kumar <rohitkr@codeaurora.org>
> > Signed-off-by: Cheng-Yi Chiang <cychiang@chromium.org>
>
> Overall the patch looks good for me, but this needs to be split into two
> patches + few more minor nits!
Separated for speaker and headset in v2.
> > ---
> >   sound/soc/qcom/sdm845.c | 82 ++++++++++++++++++++++++++++++++++++++++-
> >   1 file changed, 81 insertions(+), 1 deletion(-)
> >
> > diff --git a/sound/soc/qcom/sdm845.c b/sound/soc/qcom/sdm845.c
> > index 43c03f8e8cdc2..d815040e98dc9 100644
> > --- a/sound/soc/qcom/sdm845.c
> > +++ b/sound/soc/qcom/sdm845.c
> > @@ -6,12 +6,15 @@
> >   #include <linux/module.h>
> >   #include <linux/platform_device.h>
> >   #include <linux/of_device.h>
> > +#include <sound/core.h>
> >   #include <sound/pcm.h>
> >   #include <sound/pcm_params.h>
> >   #include <sound/jack.h>
> > +#include <sound/soc.h>
> >   #include <uapi/linux/input-event-codes.h>
> >   #include "common.h"
> >   #include "qdsp6/q6afe.h"
> > +#include "../codecs/rt5663.h"
> >
> >   #define DEFAULT_SAMPLE_RATE_48K             48000
> >   #define DEFAULT_MCLK_RATE           24576000
> > @@ -34,7 +37,7 @@ static int sdm845_tdm_snd_hw_params(struct snd_pcm_substream *substream,
> >   {
> >       struct snd_soc_pcm_runtime *rtd = substream->private_data;
> >       struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
> > -     int ret = 0;
> > +     int ret = 0, j;
> >       int channels, slot_width;
> >
> >       switch (params_format(params)) {
> > @@ -81,6 +84,31 @@ static int sdm845_tdm_snd_hw_params(struct snd_pcm_substream *substream,
> >                       goto end;
> >               }
> >       }
> > +
> > +     for (j = 0; j < rtd->num_codecs; j++) {
> > +             struct snd_soc_dai *codec_dai = rtd->codec_dais[j];
> > +
> > +             if (!strcmp(codec_dai->component->name_prefix, "Left")) {
> > +                     ret = snd_soc_dai_set_tdm_slot(
> > +                                     codec_dai, 0x30, 0x3, 8, slot_width);
>
> These constants needs some kind of defines to make the code more readable!
Fixed in v2.
>
> > +                     if (ret < 0) {
> > +                             dev_err(rtd->dev,
> > +                                     "DEV0 TDM slot err:%d\n", ret);
> > +                             return ret;
> > +                     }
> > +             }
> > +
> > +             if (!strcmp(codec_dai->component->name_prefix, "Right")) {
> > +                     ret = snd_soc_dai_set_tdm_slot(
> > +                                     codec_dai, 0xC0, 0x3, 8, slot_width);
> > +                     if (ret < 0) {
> > +                             dev_err(rtd->dev,
> > +                                     "DEV1 TDM slot err:%d\n", ret);
> > +                             return ret;
> > +                     }
> > +             }
> > +     }
> > +
> >   end:
> >       return ret;
> >   }
> > @@ -90,9 +118,26 @@ static int sdm845_snd_hw_params(struct snd_pcm_substream *substream,
> >   {
> >       struct snd_soc_pcm_runtime *rtd = substream->private_data;
> >       struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
> > +     struct snd_soc_dai *codec_dai = rtd->codec_dai;
> >       int ret = 0;
> >
> >       switch (cpu_dai->id) {
> > +     case PRIMARY_MI2S_RX:
> > +     case PRIMARY_MI2S_TX:
> > +             /*
> > +              * Use ASRC for internal clocks, as PLL rate isn't multiple
> > +              * of BCLK.
> > +              */
> > +             rt5663_sel_asrc_clk_src(
> > +                     codec_dai->component,
> > +                     RT5663_DA_STEREO_FILTER | RT5663_AD_STEREO_FILTER,
> > +                     RT5663_CLK_SEL_I2S1_ASRC);
> > +             ret = snd_soc_dai_set_sysclk(codec_dai,
> > +                             RT5663_SCLK_S_MCLK, 24576000, SND_SOC_CLOCK_IN);
>
> Use DEFAULT_MCLK_RATE here.
Fixed in v2.
>
>
> > +             if (ret < 0)
> > +                     dev_err(rtd->dev,
> > +                             "snd_soc_dai_set_sysclk err = %d\n", ret);
> > +             break;
> >       case QUATERNARY_TDM_RX_0:
> >       case QUATERNARY_TDM_TX_0:
> >               ret = sdm845_tdm_snd_hw_params(substream, params);
> > @@ -155,14 +200,20 @@ static int sdm845_dai_init(struct snd_soc_pcm_runtime *rtd)
> >   static int sdm845_snd_startup(struct snd_pcm_substream *substream)
> >   {
> >       unsigned int fmt = SND_SOC_DAIFMT_CBS_CFS;
> > +     unsigned int codec_dai_fmt = SND_SOC_DAIFMT_CBS_CFS;
> >       struct snd_soc_pcm_runtime *rtd = substream->private_data;
> >       struct snd_soc_card *card = rtd->card;
> >       struct sdm845_snd_data *data = snd_soc_card_get_drvdata(card);
> >       struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
> > +     struct snd_soc_dai *codec_dai = rtd->codec_dai;
> > +
> Unnecessary New line here.
Fixed in v2.
>
> > +     int j;
> > +     int ret;
> >
> >       switch (cpu_dai->id) {
> >       case PRIMARY_MI2S_RX:
> >       case PRIMARY_MI2S_TX:
> > +             codec_dai_fmt |= SND_SOC_DAIFMT_NB_NF;
> >               if (++(data->pri_mi2s_clk_count) == 1) {
> >                       snd_soc_dai_set_sysclk(cpu_dai,
> >                               Q6AFE_LPASS_CLK_ID_MCLK_1,
> > @@ -172,6 +223,7 @@ static int sdm845_snd_startup(struct snd_pcm_substream *substream)
> >                               MI2S_BCLK_RATE, SNDRV_PCM_STREAM_PLAYBACK);
> >               }
> >               snd_soc_dai_set_fmt(cpu_dai, fmt);
> > +             snd_soc_dai_set_fmt(codec_dai, codec_dai_fmt);
> >               break;
> >
> >       case SECONDARY_MI2S_TX:
> > @@ -190,6 +242,34 @@ static int sdm845_snd_startup(struct snd_pcm_substream *substream)
> >                               Q6AFE_LPASS_CLK_ID_QUAD_TDM_IBIT,
> >                               TDM_BCLK_RATE, SNDRV_PCM_STREAM_PLAYBACK);
> >               }
> > +
> > +             codec_dai_fmt |= SND_SOC_DAIFMT_IB_NF | SND_SOC_DAIFMT_DSP_B;
> > +
> > +             for (j = 0; j < rtd->num_codecs; j++) {
> > +                     codec_dai = rtd->codec_dais[j];
> > +
> > +                     if (!strcmp(codec_dai->component->name_prefix,
> > +                                 "Left")) {
> > +                             ret = snd_soc_dai_set_fmt(
> > +                                             codec_dai, codec_dai_fmt);
> > +                             if (ret < 0) {
> > +                                     dev_err(rtd->dev,
> > +                                             "Left TDM fmt err:%d\n", ret);
> > +                                     return ret;
> > +                             }
> > +                     }
> > +
> > +                     if (!strcmp(codec_dai->component->name_prefix,
> > +                                 "Right")) {
> > +                             ret = snd_soc_dai_set_fmt(
> > +                                             codec_dai, codec_dai_fmt);
> > +                             if (ret < 0) {
> > +                                     dev_err(rtd->dev,
> > +                                             "Right TDM slot err:%d\n", ret);
> > +                                     return ret;
> > +                             }
> > +                     }
> > +             }
> >               break;
> >
> >       default:
> >