Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <1592289761-29118-1-git-send-email-shengjiu.wang@nxp.com>
 <1592289761-29118-2-git-send-email-shengjiu.wang@nxp.com> <20200616232810.GA19896@Asurada-Nvidia>
In-Reply-To: <20200616232810.GA19896@Asurada-Nvidia>
From:   Shengjiu Wang <shengjiu.wang@gmail.com>
Date:   Wed, 17 Jun 2020 11:43:11 +0800
Message-ID: <CAA+D8AMv0EW+zUbT8bA7H+hWB8t_DQdWR=cWuYhFZA2z3F-twg@mail.gmail.com>
Subject: Re: [PATCH 2/2] ASoC: fsl_spdif: Add support for imx6sx platform
To:     Nicolin Chen <nicoleotsuka@gmail.com>
Cc:     Shengjiu Wang <shengjiu.wang@nxp.com>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@vger.kernel.org>,
        Linux-ALSA <alsa-devel@alsa-project.org>,
        Timur Tabi <timur@kernel.org>, Xiubo Li <Xiubo.Lee@gmail.com>,
        Liam Girdwood <lgirdwood@gmail.com>,
        linuxppc-dev@lists.ozlabs.org, Takashi Iwai <tiwai@suse.com>,
        Rob Herring <robh+dt@kernel.org>,
        Mark Brown <broonie@kernel.org>,
        Fabio Estevam <festevam@gmail.com>,
        linux-kernel <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Wed, Jun 17, 2020 at 7:30 AM Nicolin Chen <nicoleotsuka@gmail.com> wrote:
>
> On Tue, Jun 16, 2020 at 02:42:41PM +0800, Shengjiu Wang wrote:
> > The one difference on imx6sx platform is that the root clock
> > is shared with ASRC module, so we add a new flags "ind_root_clk"
> > which means the root clock is independent, then we will not
> > do the clk_set_rate and clk_round_rate to avoid impact ASRC
> > module usage.
> >
> > As add a new flags, we include the soc specific data struct.
> >
> > Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> > ---
> >  sound/soc/fsl/fsl_spdif.c | 43 +++++++++++++++++++++++++++++++++++----
> >  1 file changed, 39 insertions(+), 4 deletions(-)
> >
> > diff --git a/sound/soc/fsl/fsl_spdif.c b/sound/soc/fsl/fsl_spdif.c
> > index 1b2e516f9162..00e06803d32f 100644
> > --- a/sound/soc/fsl/fsl_spdif.c
> > +++ b/sound/soc/fsl/fsl_spdif.c
> > @@ -42,6 +42,17 @@ static u8 srpc_dpll_locked[] = { 0x0, 0x1, 0x2, 0x3, 0x4, 0xa, 0xb };
> >
> >  #define DEFAULT_RXCLK_SRC    1
> >
> > +/**
> > + * struct fsl_spdif_soc_data: soc specific data
> > + *
> > + * @imx: for imx platform
> > + * @ind_root_clk: flag for round clk rate
> > + */
> > +struct fsl_spdif_soc_data {
> > +     bool imx;
> > +     bool ind_root_clk;
>
> "ind" doesn't look very straightforward; maybe "shared_root_clock"?
>
> And for its comments:
>         * @shared_root_clock: flag of sharing a clock source with others;
>         *                     so the driver shouldn't set root clock rate
>
> > +};
> > +
> >  /*
> >   * SPDIF control structure
> >   * Defines channel status, subcode and Q sub
> > @@ -93,6 +104,7 @@ struct fsl_spdif_priv {
> >       struct snd_soc_dai_driver cpu_dai_drv;
> >       struct platform_device *pdev;
> >       struct regmap *regmap;
> > +     const struct fsl_spdif_soc_data *soc;
>
> Looks better if we move it to the top of the list :)
>
> > @@ -421,7 +448,7 @@ static int spdif_set_sample_rate(struct snd_pcm_substream *substream,
> >       sysclk_df = spdif_priv->sysclk_df[rate];
> >
> >       /* Don't mess up the clocks from other modules */
> > -     if (clk != STC_TXCLK_SPDIF_ROOT)
> > +     if (clk != STC_TXCLK_SPDIF_ROOT || !spdif_priv->soc->ind_root_clk)
> >               goto clk_set_bypass;
> >
> >       /* The S/PDIF block needs a clock of 64 * fs * txclk_df */
> > @@ -1186,7 +1213,8 @@ static int fsl_spdif_probe_txclk(struct fsl_spdif_priv *spdif_priv,
> >                       continue;
> >
> >               ret = fsl_spdif_txclk_caldiv(spdif_priv, clk, savesub, index,
> > -                                          i == STC_TXCLK_SPDIF_ROOT);
> > +                                          i == STC_TXCLK_SPDIF_ROOT &&
> > +                                          spdif_priv->soc->ind_root_clk);
>
> Having more than one place that checks the condition, we can add:
>
> /* Check if clk is a root clock that does not share clock source with others */
> static inline bool fsl_spdif_can_set_clk_rate(struct fsl_spdif_priv *spdif, int clk)
> {
>         return (clk == STC_TXCLK_SPDIF_ROOT) && !spdif->soc->shared_root_clock;
> }

will update them in v2

best regards
wang shengjiu