diff mbox

ASoC: Extend FLL function

Message ID 1451548510-4989-1-git-send-email-KCHSU0@nuvoton.com (mailing list archive)
State New, archived
Headers show

Commit Message

AS50 KCHsu0 Dec. 31, 2015, 7:55 a.m. UTC
Extend FFL clock source from MCLK/BCLK/FS;
And modify some FLL parameter setting.

Signed-off-by: John Hsu <KCHSU0@nuvoton.com>
---
 sound/soc/codecs/nau8825.c | 100 +++++++++++++++++++++++++++++++++------------
 sound/soc/codecs/nau8825.h |  14 ++++++-
 2 files changed, 86 insertions(+), 28 deletions(-)

Comments

Anatol Pomozov Dec. 31, 2015, 9:14 a.m. UTC | #1
Hi

On Wed, Dec 30, 2015 at 11:55 PM, John Hsu <KCHSU0@nuvoton.com> wrote:
> Extend FFL clock source from MCLK/BCLK/FS;
> And modify some FLL parameter setting.
>
> Signed-off-by: John Hsu <KCHSU0@nuvoton.com>
> ---
>  sound/soc/codecs/nau8825.c | 100 +++++++++++++++++++++++++++++++++------------
>  sound/soc/codecs/nau8825.h |  14 ++++++-
>  2 files changed, 86 insertions(+), 28 deletions(-)
>
> diff --git a/sound/soc/codecs/nau8825.c b/sound/soc/codecs/nau8825.c
> index 7fc7b4e..16da8ef 100644
> --- a/sound/soc/codecs/nau8825.c
> +++ b/sound/soc/codecs/nau8825.c
> @@ -926,7 +926,8 @@ static void nau8825_fll_apply(struct nau8825 *nau8825,
>                 struct nau8825_fll *fll_param)
>  {
>         regmap_update_bits(nau8825->regmap, NAU8825_REG_CLK_DIVIDER,
> -               NAU8825_CLK_MCLK_SRC_MASK, fll_param->mclk_src);
> +               NAU8825_CLK_SRC_MASK | NAU8825_CLK_MCLK_SRC_MASK,
> +               NAU8825_CLK_SRC_MCLK | fll_param->mclk_src);
>         regmap_update_bits(nau8825->regmap, NAU8825_REG_FLL1,
>                         NAU8825_FLL_RATIO_MASK, fll_param->ratio);
>         /* FLL 16-bit fractional input */
> @@ -939,10 +940,20 @@ static void nau8825_fll_apply(struct nau8825 *nau8825,
>                         NAU8825_FLL_REF_DIV_MASK, fll_param->clk_ref_div);
>         /* select divided VCO input */
>         regmap_update_bits(nau8825->regmap, NAU8825_REG_FLL5,
> -                       NAU8825_FLL_FILTER_SW_MASK, 0x0000);
> -       /* FLL sigma delta modulator enable */
> +               NAU8825_FLL_PDB_DAC_EN | NAU8825_FLL_LOOP_FTR_EN |
> +               NAU8825_FLL_FILTER_SW_MASK,
> +               NAU8825_FLL_PDB_DAC_EN | NAU8825_FLL_LOOP_FTR_EN |
> +               NAU8825_FLL_FILTER_SW_REF);
> +       /* Disable free-running mode */
>         regmap_update_bits(nau8825->regmap, NAU8825_REG_FLL6,
> -                       NAU8825_SDM_EN_MASK, NAU8825_SDM_EN);
> +                               NAU8825_DCO_EN, 0);
> +       /* FLL sigma delta modulator enable */
> +       if (fll_param->fll_frac)
> +               regmap_update_bits(nau8825->regmap, NAU8825_REG_FLL6,
> +                               NAU8825_SDM_EN_MASK, NAU8825_SDM_EN);
> +       else
> +               regmap_update_bits(nau8825->regmap, NAU8825_REG_FLL6,
> +                               NAU8825_SDM_EN_MASK, 0);
>  }
>
>  /* freq_out must be 256*Fs in order to achieve the best performance */
> @@ -970,6 +981,37 @@ static int nau8825_set_pll(struct snd_soc_codec *codec, int pll_id, int source,
>         return 0;
>  }
>
> +static int nau8825_mclk_prepare(struct nau8825 *nau8825, unsigned int freq)
> +{
> +       int ret = 0;
> +
> +       /* We selected MCLK source but the clock itself managed externally */
> +       if (!nau8825->mclk)
> +               goto done;

A nitpick.

This "goto" style is used for recovering resources or something that
should be restored in case of error. But in your case "done:" does not
recover anything. It is cleaner remove "goto" and replace with "return
0" or "return err".

> +
> +       if (!nau8825->mclk_freq) {
> +               ret = clk_prepare_enable(nau8825->mclk);
> +               if (ret) {
> +                       dev_err(nau8825->dev, "Unable to prepare codec mclk\n");
> +                       goto done;
> +               }
> +       }
> +
> +       if (nau8825->mclk_freq != freq) {
> +               nau8825->mclk_freq = freq;
> +
> +               freq = clk_round_rate(nau8825->mclk, freq);
> +               ret = clk_set_rate(nau8825->mclk, freq);
> +               if (ret) {
> +                       dev_err(nau8825->dev, "Unable to set mclk rate\n");
> +                       goto done;
> +               }
> +       }
> +
> +done:
> +       return ret;
> +}
> +
>  static int nau8825_configure_sysclk(struct nau8825 *nau8825, int clk_id,
>         unsigned int freq)
>  {
> @@ -981,29 +1023,9 @@ static int nau8825_configure_sysclk(struct nau8825 *nau8825, int clk_id,
>                 regmap_update_bits(regmap, NAU8825_REG_CLK_DIVIDER,
>                         NAU8825_CLK_SRC_MASK, NAU8825_CLK_SRC_MCLK);
>                 regmap_update_bits(regmap, NAU8825_REG_FLL6, NAU8825_DCO_EN, 0);
> -
> -               /* We selected MCLK source but the clock itself managed externally */
> -               if (!nau8825->mclk)
> -                       break;
> -
> -               if (!nau8825->mclk_freq) {
> -                       ret = clk_prepare_enable(nau8825->mclk);
> -                       if (ret) {
> -                               dev_err(nau8825->dev, "Unable to prepare codec mclk\n");
> -                               return ret;
> -                       }
> -               }
> -
> -               if (nau8825->mclk_freq != freq) {
> -                       nau8825->mclk_freq = freq;
> -
> -                       freq = clk_round_rate(nau8825->mclk, freq);
> -                       ret = clk_set_rate(nau8825->mclk, freq);
> -                       if (ret) {
> -                               dev_err(nau8825->dev, "Unable to set mclk rate\n");
> -                               return ret;
> -                       }
> -               }
> +               ret = nau8825_mclk_prepare(nau8825, freq);
> +               if (ret)
> +                       return ret;
>
>                 break;
>         case NAU8825_CLK_INTERNAL:
> @@ -1018,6 +1040,30 @@ static int nau8825_configure_sysclk(struct nau8825 *nau8825, int clk_id,
>                 }
>
>                 break;
> +       case NAU8825_CLK_FLL_MCLK:
> +               regmap_update_bits(regmap, NAU8825_REG_FLL3,
> +                       NAU8825_FLL_CLK_SRC_MASK, NAU8825_FLL_CLK_SRC_MCLK);
> +               ret = nau8825_mclk_prepare(nau8825, freq);
> +               if (ret)
> +                       return ret;
> +
> +               break;
> +       case NAU8825_CLK_FLL_BLK:
> +               regmap_update_bits(regmap, NAU8825_REG_FLL3,
> +                       NAU8825_FLL_CLK_SRC_MASK, NAU8825_FLL_CLK_SRC_BLK);
> +               ret = nau8825_mclk_prepare(nau8825, freq);


Based on names for these constants I believe CODEC is going to use
BCLK/FS signal of I2S bus as a clock signal reference.

Do you still need MCLK signal in this case? If no then you should
disable MCLK to save some power.


> +               if (ret)
> +                       return ret;
> +
> +               break;
> +       case NAU8825_CLK_FLL_FS:
> +               regmap_update_bits(regmap, NAU8825_REG_FLL3,
> +                       NAU8825_FLL_CLK_SRC_MASK, NAU8825_FLL_CLK_SRC_FS);
> +               ret = nau8825_mclk_prepare(nau8825, freq);
> +               if (ret)
> +                       return ret;
> +
> +               break;
>         default:
>                 dev_err(nau8825->dev, "Invalid clock id (%d)\n", clk_id);
>                 return -EINVAL;
> diff --git a/sound/soc/codecs/nau8825.h b/sound/soc/codecs/nau8825.h
> index dff8edb..e8f7ca5 100644
> --- a/sound/soc/codecs/nau8825.h
> +++ b/sound/soc/codecs/nau8825.h
> @@ -112,12 +112,21 @@
>
>  /* FLL3 (0x06) */
>  #define NAU8825_FLL_INTEGER_MASK               (0x3ff << 0)
> +#define NAU8825_FLL_CLK_SRC_SFT                10
> +#define NAU8825_FLL_CLK_SRC_MASK               (0x3 << NAU8825_FLL_CLK_SRC_SFT)
> +#define NAU8825_FLL_CLK_SRC_MCLK               (0 << NAU8825_FLL_CLK_SRC_SFT)
> +#define NAU8825_FLL_CLK_SRC_BLK                (0x2 << NAU8825_FLL_CLK_SRC_SFT)
> +#define NAU8825_FLL_CLK_SRC_FS                 (0x3 << NAU8825_FLL_CLK_SRC_SFT)
>
>  /* FLL4 (0x07) */
>  #define NAU8825_FLL_REF_DIV_MASK               (0x3 << 10)
>
>  /* FLL5 (0x08) */
> -#define NAU8825_FLL_FILTER_SW_MASK             (0x1 << 14)
> +#define NAU8825_FLL_PDB_DAC_EN         (0x1 << 15)
> +#define NAU8825_FLL_LOOP_FTR_EN                (0x1 << 14)
> +#define NAU8825_FLL_FILTER_SW_MASK             (0x1 << 13)
> +#define NAU8825_FLL_FILTER_SW_N2               (0x1 << 13)
> +#define NAU8825_FLL_FILTER_SW_REF              (0x0 << 13)
>
>  /* FLL6 (0x9) */
>  #define NAU8825_DCO_EN_MASK                    (0x1 << 15)
> @@ -306,6 +315,9 @@
>  enum {
>         NAU8825_CLK_MCLK = 0,
>         NAU8825_CLK_INTERNAL,
> +       NAU8825_CLK_FLL_MCLK,
> +       NAU8825_CLK_FLL_BLK,
> +       NAU8825_CLK_FLL_FS,
>  };
>
>  struct nau8825 {
> --
> 1.9.1
>
AS50 KCHsu0 Jan. 4, 2016, 10:04 a.m. UTC | #2
Dear all,
Please check up in the following comment, and I cut off original mail.

On 12/31/2015 5:14 PM, Anatol Pomozov wrote:
>>  /* freq_out must be 256*Fs in order to achieve the best performance */
>> @@ -970,6 +981,37 @@ static int nau8825_set_pll(struct snd_soc_codec *codec, int pll_id, int source,
>>         return 0;
>>  }
>>
>> +static int nau8825_mclk_prepare(struct nau8825 *nau8825, unsigned int freq)
>> +{
>> +       int ret = 0;
>> +
>> +       /* We selected MCLK source but the clock itself managed externally */
>> +       if (!nau8825->mclk)
>> +               goto done;
>>     
>
> A nitpick.
>
> This "goto" style is used for recovering resources or something that
> should be restored in case of error. But in your case "done:" does not
> recover anything. It is cleaner remove "goto" and replace with "return
> 0" or "return err".
>
>   
OK, I'll change it.
>> +
>> +       if (!nau8825->mclk_freq) {
>> +               ret = clk_prepare_enable(nau8825->mclk);
>> +               if (ret) {
>> +                       dev_err(nau8825->dev, "Unable to prepare codec mclk\n");
>> +                       goto done;
>> +               }
>> +       }
>> +
>> +       if (nau8825->mclk_freq != freq) {
>> +               nau8825->mclk_freq = freq;
>> +
>> +               freq = clk_round_rate(nau8825->mclk, freq);
>> +               ret = clk_set_rate(nau8825->mclk, freq);
>> +               if (ret) {
>> +                       dev_err(nau8825->dev, "Unable to set mclk rate\n");
>> +                       goto done;
>> +               }
>> +       }
>> +
>> +done:
>> +       return ret;
>> +}
>> +
>>
>>                 break;
>>         case NAU8825_CLK_INTERNAL:
>> @@ -1018,6 +1040,30 @@ static int nau8825_configure_sysclk(struct nau8825 *nau8825, int clk_id,
>>                 }
>>
>>                 break;
>> +       case NAU8825_CLK_FLL_MCLK:
>> +               regmap_update_bits(regmap, NAU8825_REG_FLL3,
>> +                       NAU8825_FLL_CLK_SRC_MASK, NAU8825_FLL_CLK_SRC_MCLK);
>> +               ret = nau8825_mclk_prepare(nau8825, freq);
>> +               if (ret)
>> +                       return ret;
>> +
>> +               break;
>> +       case NAU8825_CLK_FLL_BLK:
>> +               regmap_update_bits(regmap, NAU8825_REG_FLL3,
>> +                       NAU8825_FLL_CLK_SRC_MASK, NAU8825_FLL_CLK_SRC_BLK);
>> +               ret = nau8825_mclk_prepare(nau8825, freq);
>>     
>
>
> Based on names for these constants I believe CODEC is going to use
> BCLK/FS signal of I2S bus as a clock signal reference.
>
> Do you still need MCLK signal in this case? If no then you should
> disable MCLK to save some power.
>
>   
Yes, the MCLK should disable when FLL reference to BCLK/FS for power saving.
I'll disable it like internal clock case.


BR.
John
AS50 KCHsu0 Feb. 24, 2016, 10:51 p.m. UTC | #3
Hi Mark,
Could you help to review and merge this patch?
Some patches later about nau8825 driver will patch base on it. Thank you 
very much.

BR.
John

On 12/31/2015 5:14 PM, Anatol Pomozov wrote:
> Hi
>
> On Wed, Dec 30, 2015 at 11:55 PM, John Hsu <KCHSU0@nuvoton.com> wrote:
>   
>> Extend FFL clock source from MCLK/BCLK/FS;
>> And modify some FLL parameter setting.
>>
>> Signed-off-by: John Hsu <KCHSU0@nuvoton.com>
>> ---
>>  sound/soc/codecs/nau8825.c | 100 +++++++++++++++++++++++++++++++++------------
>>  sound/soc/codecs/nau8825.h |  14 ++++++-
>>  2 files changed, 86 insertions(+), 28 deletions(-)
>>
>> diff --git a/sound/soc/codecs/nau8825.c b/sound/soc/codecs/nau8825.c
>> index 7fc7b4e..16da8ef 100644
>> --- a/sound/soc/codecs/nau8825.c
>> +++ b/sound/soc/codecs/nau8825.c
>> @@ -926,7 +926,8 @@ static void nau8825_fll_apply(struct nau8825 *nau8825,
>>                 struct nau8825_fll *fll_param)
>>  {
>>         regmap_update_bits(nau8825->regmap, NAU8825_REG_CLK_DIVIDER,
>> -               NAU8825_CLK_MCLK_SRC_MASK, fll_param->mclk_src);
>> +               NAU8825_CLK_SRC_MASK | NAU8825_CLK_MCLK_SRC_MASK,
>> +               NAU8825_CLK_SRC_MCLK | fll_param->mclk_src);
>>         regmap_update_bits(nau8825->regmap, NAU8825_REG_FLL1,
>>                         NAU8825_FLL_RATIO_MASK, fll_param->ratio);
>>         /* FLL 16-bit fractional input */
>> @@ -939,10 +940,20 @@ static void nau8825_fll_apply(struct nau8825 *nau8825,
>>                         NAU8825_FLL_REF_DIV_MASK, fll_param->clk_ref_div);
>>         /* select divided VCO input */
>>         regmap_update_bits(nau8825->regmap, NAU8825_REG_FLL5,
>> -                       NAU8825_FLL_FILTER_SW_MASK, 0x0000);
>> -       /* FLL sigma delta modulator enable */
>> +               NAU8825_FLL_PDB_DAC_EN | NAU8825_FLL_LOOP_FTR_EN |
>> +               NAU8825_FLL_FILTER_SW_MASK,
>> +               NAU8825_FLL_PDB_DAC_EN | NAU8825_FLL_LOOP_FTR_EN |
>> +               NAU8825_FLL_FILTER_SW_REF);
>> +       /* Disable free-running mode */
>>         regmap_update_bits(nau8825->regmap, NAU8825_REG_FLL6,
>> -                       NAU8825_SDM_EN_MASK, NAU8825_SDM_EN);
>> +                               NAU8825_DCO_EN, 0);
>> +       /* FLL sigma delta modulator enable */
>> +       if (fll_param->fll_frac)
>> +               regmap_update_bits(nau8825->regmap, NAU8825_REG_FLL6,
>> +                               NAU8825_SDM_EN_MASK, NAU8825_SDM_EN);
>> +       else
>> +               regmap_update_bits(nau8825->regmap, NAU8825_REG_FLL6,
>> +                               NAU8825_SDM_EN_MASK, 0);
>>  }
>>
>>  /* freq_out must be 256*Fs in order to achieve the best performance */
>> @@ -970,6 +981,37 @@ static int nau8825_set_pll(struct snd_soc_codec *codec, int pll_id, int source,
>>         return 0;
>>  }
>>
>> +static int nau8825_mclk_prepare(struct nau8825 *nau8825, unsigned int freq)
>> +{
>> +       int ret = 0;
>> +
>> +       /* We selected MCLK source but the clock itself managed externally */
>> +       if (!nau8825->mclk)
>> +               goto done;
>>     
>
> A nitpick.
>
> This "goto" style is used for recovering resources or something that
> should be restored in case of error. But in your case "done:" does not
> recover anything. It is cleaner remove "goto" and replace with "return
> 0" or "return err".
>
>   
>> +
>> +       if (!nau8825->mclk_freq) {
>> +               ret = clk_prepare_enable(nau8825->mclk);
>> +               if (ret) {
>> +                       dev_err(nau8825->dev, "Unable to prepare codec mclk\n");
>> +                       goto done;
>> +               }
>> +       }
>> +
>> +       if (nau8825->mclk_freq != freq) {
>> +               nau8825->mclk_freq = freq;
>> +
>> +               freq = clk_round_rate(nau8825->mclk, freq);
>> +               ret = clk_set_rate(nau8825->mclk, freq);
>> +               if (ret) {
>> +                       dev_err(nau8825->dev, "Unable to set mclk rate\n");
>> +                       goto done;
>> +               }
>> +       }
>> +
>> +done:
>> +       return ret;
>> +}
>> +
>>  static int nau8825_configure_sysclk(struct nau8825 *nau8825, int clk_id,
>>         unsigned int freq)
>>  {
>> @@ -981,29 +1023,9 @@ static int nau8825_configure_sysclk(struct nau8825 *nau8825, int clk_id,
>>                 regmap_update_bits(regmap, NAU8825_REG_CLK_DIVIDER,
>>                         NAU8825_CLK_SRC_MASK, NAU8825_CLK_SRC_MCLK);
>>                 regmap_update_bits(regmap, NAU8825_REG_FLL6, NAU8825_DCO_EN, 0);
>> -
>> -               /* We selected MCLK source but the clock itself managed externally */
>> -               if (!nau8825->mclk)
>> -                       break;
>> -
>> -               if (!nau8825->mclk_freq) {
>> -                       ret = clk_prepare_enable(nau8825->mclk);
>> -                       if (ret) {
>> -                               dev_err(nau8825->dev, "Unable to prepare codec mclk\n");
>> -                               return ret;
>> -                       }
>> -               }
>> -
>> -               if (nau8825->mclk_freq != freq) {
>> -                       nau8825->mclk_freq = freq;
>> -
>> -                       freq = clk_round_rate(nau8825->mclk, freq);
>> -                       ret = clk_set_rate(nau8825->mclk, freq);
>> -                       if (ret) {
>> -                               dev_err(nau8825->dev, "Unable to set mclk rate\n");
>> -                               return ret;
>> -                       }
>> -               }
>> +               ret = nau8825_mclk_prepare(nau8825, freq);
>> +               if (ret)
>> +                       return ret;
>>
>>                 break;
>>         case NAU8825_CLK_INTERNAL:
>> @@ -1018,6 +1040,30 @@ static int nau8825_configure_sysclk(struct nau8825 *nau8825, int clk_id,
>>                 }
>>
>>                 break;
>> +       case NAU8825_CLK_FLL_MCLK:
>> +               regmap_update_bits(regmap, NAU8825_REG_FLL3,
>> +                       NAU8825_FLL_CLK_SRC_MASK, NAU8825_FLL_CLK_SRC_MCLK);
>> +               ret = nau8825_mclk_prepare(nau8825, freq);
>> +               if (ret)
>> +                       return ret;
>> +
>> +               break;
>> +       case NAU8825_CLK_FLL_BLK:
>> +               regmap_update_bits(regmap, NAU8825_REG_FLL3,
>> +                       NAU8825_FLL_CLK_SRC_MASK, NAU8825_FLL_CLK_SRC_BLK);
>> +               ret = nau8825_mclk_prepare(nau8825, freq);
>>     
>
>
> Based on names for these constants I believe CODEC is going to use
> BCLK/FS signal of I2S bus as a clock signal reference.
>
> Do you still need MCLK signal in this case? If no then you should
> disable MCLK to save some power.
>
>
>   
>> +               if (ret)
>> +                       return ret;
>> +
>> +               break;
>> +       case NAU8825_CLK_FLL_FS:
>> +               regmap_update_bits(regmap, NAU8825_REG_FLL3,
>> +                       NAU8825_FLL_CLK_SRC_MASK, NAU8825_FLL_CLK_SRC_FS);
>> +               ret = nau8825_mclk_prepare(nau8825, freq);
>> +               if (ret)
>> +                       return ret;
>> +
>> +               break;
>>         default:
>>                 dev_err(nau8825->dev, "Invalid clock id (%d)\n", clk_id);
>>                 return -EINVAL;
>> diff --git a/sound/soc/codecs/nau8825.h b/sound/soc/codecs/nau8825.h
>> index dff8edb..e8f7ca5 100644
>> --- a/sound/soc/codecs/nau8825.h
>> +++ b/sound/soc/codecs/nau8825.h
>> @@ -112,12 +112,21 @@
>>
>>  /* FLL3 (0x06) */
>>  #define NAU8825_FLL_INTEGER_MASK               (0x3ff << 0)
>> +#define NAU8825_FLL_CLK_SRC_SFT                10
>> +#define NAU8825_FLL_CLK_SRC_MASK               (0x3 << NAU8825_FLL_CLK_SRC_SFT)
>> +#define NAU8825_FLL_CLK_SRC_MCLK               (0 << NAU8825_FLL_CLK_SRC_SFT)
>> +#define NAU8825_FLL_CLK_SRC_BLK                (0x2 << NAU8825_FLL_CLK_SRC_SFT)
>> +#define NAU8825_FLL_CLK_SRC_FS                 (0x3 << NAU8825_FLL_CLK_SRC_SFT)
>>
>>  /* FLL4 (0x07) */
>>  #define NAU8825_FLL_REF_DIV_MASK               (0x3 << 10)
>>
>>  /* FLL5 (0x08) */
>> -#define NAU8825_FLL_FILTER_SW_MASK             (0x1 << 14)
>> +#define NAU8825_FLL_PDB_DAC_EN         (0x1 << 15)
>> +#define NAU8825_FLL_LOOP_FTR_EN                (0x1 << 14)
>> +#define NAU8825_FLL_FILTER_SW_MASK             (0x1 << 13)
>> +#define NAU8825_FLL_FILTER_SW_N2               (0x1 << 13)
>> +#define NAU8825_FLL_FILTER_SW_REF              (0x0 << 13)
>>
>>  /* FLL6 (0x9) */
>>  #define NAU8825_DCO_EN_MASK                    (0x1 << 15)
>> @@ -306,6 +315,9 @@
>>  enum {
>>         NAU8825_CLK_MCLK = 0,
>>         NAU8825_CLK_INTERNAL,
>> +       NAU8825_CLK_FLL_MCLK,
>> +       NAU8825_CLK_FLL_BLK,
>> +       NAU8825_CLK_FLL_FS,
>>  };
>>
>>  struct nau8825 {
>> --
>> 1.9.1
>>
>>     
> .
>
>
Mark Brown Feb. 25, 2016, 1:55 a.m. UTC | #4
On Thu, Feb 25, 2016 at 06:51:00AM +0800, John Hsu wrote:
> Hi Mark,
> Could you help to review and merge this patch?
> Some patches later about nau8825 driver will patch base on it. Thank you
> very much.

Please don't top post, reply in line with needed context.  This allows
readers to readily follow the flow of conversation and understand what
you are talking about and also helps ensure that everything in the
discussion is being addressed.

I don't have whatever this patch is (I suspect the fact that there were
concerns raised meant I expected a new version).  Please do try to use
subject lines reflecting the style to the subsystem.
AS50 KCHsu0 Feb. 25, 2016, 5:34 p.m. UTC | #5
Hi

On 2/25/2016 9:55 AM, Mark Brown wrote:
> On Thu, Feb 25, 2016 at 06:51:00AM +0800, John Hsu wrote:
>   
>> Hi Mark,
>> Could you help to review and merge this patch?
>> Some patches later about nau8825 driver will patch base on it. Thank you
>> very much.
>>     
>
> Please don't top post, reply in line with needed context.  This allows
> readers to readily follow the flow of conversation and understand what
> you are talking about and also helps ensure that everything in the
> discussion is being addressed.
>
>   
I see and follow it in the future patch and review. Thanks for your 
information.

> I don't have whatever this patch is (I suspect the fact that there were
> concerns raised meant I expected a new version).  Please do try to use
> subject lines reflecting the style to the subsystem.
>   

Do you mean I need to resubmit this patch and in-reply-to Message-ID of 
this mail?
I don't really catch the meaning of "use subject lines reflecting the 
style to the subsystem".
Could you tell me more detail about it? Thank you very much.
Mark Brown Feb. 26, 2016, 1:34 a.m. UTC | #6
On Fri, Feb 26, 2016 at 01:34:19AM +0800, John Hsu wrote:
> On 2/25/2016 9:55 AM, Mark Brown wrote:

> >I don't have whatever this patch is (I suspect the fact that there were
> >concerns raised meant I expected a new version).  Please do try to use
> >subject lines reflecting the style to the subsystem.

> Do you mean I need to resubmit this patch and in-reply-to Message-ID of this
> mail?

Yes, you need to resubmit the patch.  No, you do not need to do it in
reply to this e-mail.

> I don't really catch the meaning of "use subject lines reflecting the style
> to the subsystem".
> Could you tell me more detail about it? Thank you very much.

If you look at all the other driver commits you'll see that their
subject lines start "ASoC: driver: " .
AS50 KCHsu0 Feb. 26, 2016, 4:16 p.m. UTC | #7
Hi

On 2/26/2016 9:34 AM, Mark Brown wrote:
> On Fri, Feb 26, 2016 at 01:34:19AM +0800, John Hsu wrote:
>   
>> On 2/25/2016 9:55 AM, Mark Brown wrote:
>>     
>
>   
>>> I don't have whatever this patch is (I suspect the fact that there were
>>> concerns raised meant I expected a new version).  Please do try to use
>>> subject lines reflecting the style to the subsystem.
>>>       
>
>   
>> Do you mean I need to resubmit this patch and in-reply-to Message-ID of this
>> mail?
>>     
>
> Yes, you need to resubmit the patch.  No, you do not need to do it in
> reply to this e-mail.
>
>   
I'll submit it again with proper subject.
>> I don't really catch the meaning of "use subject lines reflecting the style
>> to the subsystem".
>> Could you tell me more detail about it? Thank you very much.
>>     
>
> If you look at all the other driver commits you'll see that their
> subject lines start "ASoC: driver: " .
>   

Yes, I get it. I forgot to do that. Thanks for your remind.
diff mbox

Patch

diff --git a/sound/soc/codecs/nau8825.c b/sound/soc/codecs/nau8825.c
index 7fc7b4e..16da8ef 100644
--- a/sound/soc/codecs/nau8825.c
+++ b/sound/soc/codecs/nau8825.c
@@ -926,7 +926,8 @@  static void nau8825_fll_apply(struct nau8825 *nau8825,
 		struct nau8825_fll *fll_param)
 {
 	regmap_update_bits(nau8825->regmap, NAU8825_REG_CLK_DIVIDER,
-		NAU8825_CLK_MCLK_SRC_MASK, fll_param->mclk_src);
+		NAU8825_CLK_SRC_MASK | NAU8825_CLK_MCLK_SRC_MASK,
+		NAU8825_CLK_SRC_MCLK | fll_param->mclk_src);
 	regmap_update_bits(nau8825->regmap, NAU8825_REG_FLL1,
 			NAU8825_FLL_RATIO_MASK, fll_param->ratio);
 	/* FLL 16-bit fractional input */
@@ -939,10 +940,20 @@  static void nau8825_fll_apply(struct nau8825 *nau8825,
 			NAU8825_FLL_REF_DIV_MASK, fll_param->clk_ref_div);
 	/* select divided VCO input */
 	regmap_update_bits(nau8825->regmap, NAU8825_REG_FLL5,
-			NAU8825_FLL_FILTER_SW_MASK, 0x0000);
-	/* FLL sigma delta modulator enable */
+		NAU8825_FLL_PDB_DAC_EN | NAU8825_FLL_LOOP_FTR_EN |
+		NAU8825_FLL_FILTER_SW_MASK,
+		NAU8825_FLL_PDB_DAC_EN | NAU8825_FLL_LOOP_FTR_EN |
+		NAU8825_FLL_FILTER_SW_REF);
+	/* Disable free-running mode */
 	regmap_update_bits(nau8825->regmap, NAU8825_REG_FLL6,
-			NAU8825_SDM_EN_MASK, NAU8825_SDM_EN);
+				NAU8825_DCO_EN, 0);
+	/* FLL sigma delta modulator enable */
+	if (fll_param->fll_frac)
+		regmap_update_bits(nau8825->regmap, NAU8825_REG_FLL6,
+				NAU8825_SDM_EN_MASK, NAU8825_SDM_EN);
+	else
+		regmap_update_bits(nau8825->regmap, NAU8825_REG_FLL6,
+				NAU8825_SDM_EN_MASK, 0);
 }
 
 /* freq_out must be 256*Fs in order to achieve the best performance */
@@ -970,6 +981,37 @@  static int nau8825_set_pll(struct snd_soc_codec *codec, int pll_id, int source,
 	return 0;
 }
 
+static int nau8825_mclk_prepare(struct nau8825 *nau8825, unsigned int freq)
+{
+	int ret = 0;
+
+	/* We selected MCLK source but the clock itself managed externally */
+	if (!nau8825->mclk)
+		goto done;
+
+	if (!nau8825->mclk_freq) {
+		ret = clk_prepare_enable(nau8825->mclk);
+		if (ret) {
+			dev_err(nau8825->dev, "Unable to prepare codec mclk\n");
+			goto done;
+		}
+	}
+
+	if (nau8825->mclk_freq != freq) {
+		nau8825->mclk_freq = freq;
+
+		freq = clk_round_rate(nau8825->mclk, freq);
+		ret = clk_set_rate(nau8825->mclk, freq);
+		if (ret) {
+			dev_err(nau8825->dev, "Unable to set mclk rate\n");
+			goto done;
+		}
+	}
+
+done:
+	return ret;
+}
+
 static int nau8825_configure_sysclk(struct nau8825 *nau8825, int clk_id,
 	unsigned int freq)
 {
@@ -981,29 +1023,9 @@  static int nau8825_configure_sysclk(struct nau8825 *nau8825, int clk_id,
 		regmap_update_bits(regmap, NAU8825_REG_CLK_DIVIDER,
 			NAU8825_CLK_SRC_MASK, NAU8825_CLK_SRC_MCLK);
 		regmap_update_bits(regmap, NAU8825_REG_FLL6, NAU8825_DCO_EN, 0);
-
-		/* We selected MCLK source but the clock itself managed externally */
-		if (!nau8825->mclk)
-			break;
-
-		if (!nau8825->mclk_freq) {
-			ret = clk_prepare_enable(nau8825->mclk);
-			if (ret) {
-				dev_err(nau8825->dev, "Unable to prepare codec mclk\n");
-				return ret;
-			}
-		}
-
-		if (nau8825->mclk_freq != freq) {
-			nau8825->mclk_freq = freq;
-
-			freq = clk_round_rate(nau8825->mclk, freq);
-			ret = clk_set_rate(nau8825->mclk, freq);
-			if (ret) {
-				dev_err(nau8825->dev, "Unable to set mclk rate\n");
-				return ret;
-			}
-		}
+		ret = nau8825_mclk_prepare(nau8825, freq);
+		if (ret)
+			return ret;
 
 		break;
 	case NAU8825_CLK_INTERNAL:
@@ -1018,6 +1040,30 @@  static int nau8825_configure_sysclk(struct nau8825 *nau8825, int clk_id,
 		}
 
 		break;
+	case NAU8825_CLK_FLL_MCLK:
+		regmap_update_bits(regmap, NAU8825_REG_FLL3,
+			NAU8825_FLL_CLK_SRC_MASK, NAU8825_FLL_CLK_SRC_MCLK);
+		ret = nau8825_mclk_prepare(nau8825, freq);
+		if (ret)
+			return ret;
+
+		break;
+	case NAU8825_CLK_FLL_BLK:
+		regmap_update_bits(regmap, NAU8825_REG_FLL3,
+			NAU8825_FLL_CLK_SRC_MASK, NAU8825_FLL_CLK_SRC_BLK);
+		ret = nau8825_mclk_prepare(nau8825, freq);
+		if (ret)
+			return ret;
+
+		break;
+	case NAU8825_CLK_FLL_FS:
+		regmap_update_bits(regmap, NAU8825_REG_FLL3,
+			NAU8825_FLL_CLK_SRC_MASK, NAU8825_FLL_CLK_SRC_FS);
+		ret = nau8825_mclk_prepare(nau8825, freq);
+		if (ret)
+			return ret;
+
+		break;
 	default:
 		dev_err(nau8825->dev, "Invalid clock id (%d)\n", clk_id);
 		return -EINVAL;
diff --git a/sound/soc/codecs/nau8825.h b/sound/soc/codecs/nau8825.h
index dff8edb..e8f7ca5 100644
--- a/sound/soc/codecs/nau8825.h
+++ b/sound/soc/codecs/nau8825.h
@@ -112,12 +112,21 @@ 
 
 /* FLL3 (0x06) */
 #define NAU8825_FLL_INTEGER_MASK		(0x3ff << 0)
+#define NAU8825_FLL_CLK_SRC_SFT		10
+#define NAU8825_FLL_CLK_SRC_MASK		(0x3 << NAU8825_FLL_CLK_SRC_SFT)
+#define NAU8825_FLL_CLK_SRC_MCLK		(0 << NAU8825_FLL_CLK_SRC_SFT)
+#define NAU8825_FLL_CLK_SRC_BLK		(0x2 << NAU8825_FLL_CLK_SRC_SFT)
+#define NAU8825_FLL_CLK_SRC_FS			(0x3 << NAU8825_FLL_CLK_SRC_SFT)
 
 /* FLL4 (0x07) */
 #define NAU8825_FLL_REF_DIV_MASK		(0x3 << 10)
 
 /* FLL5 (0x08) */
-#define NAU8825_FLL_FILTER_SW_MASK		(0x1 << 14)
+#define NAU8825_FLL_PDB_DAC_EN		(0x1 << 15)
+#define NAU8825_FLL_LOOP_FTR_EN		(0x1 << 14)
+#define NAU8825_FLL_FILTER_SW_MASK		(0x1 << 13)
+#define NAU8825_FLL_FILTER_SW_N2		(0x1 << 13)
+#define NAU8825_FLL_FILTER_SW_REF		(0x0 << 13)
 
 /* FLL6 (0x9) */
 #define NAU8825_DCO_EN_MASK			(0x1 << 15)
@@ -306,6 +315,9 @@ 
 enum {
 	NAU8825_CLK_MCLK = 0,
 	NAU8825_CLK_INTERNAL,
+	NAU8825_CLK_FLL_MCLK,
+	NAU8825_CLK_FLL_BLK,
+	NAU8825_CLK_FLL_FS,
 };
 
 struct nau8825 {