| Message ID | 20220405075959.2744803-16-s.hauer@pengutronix.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ASoC: fsl_micfil: Driver updates | expand |
On Tue, Apr 5, 2022 at 4:00 PM Sascha Hauer <s.hauer@pengutronix.de> wrote: > The reference manual has this for calculating the micfil internal clock > divider: > > MICFIL Clock rate > clkdiv = ----------------- > 8 * OSR * outrate > > (with OSR == Oversampling Rate, outrate == output sample rate) > > The driver first sets the MICFIL Clock rate to (outrate * 1024) and then > calculates back the clkdiv value from the above calculation. > > Simplify this by using a fixed clkdiv value of 8 and set the MICFIL > Clock rate to (outrate * clkdiv * OSR * 8). > > While at it drop disabling the clock before setting its rate. The MICFIL > module is disabled when the rate is changed and it is also resetted > before it is started again, so I doubt it's necessary to disable the > clock. > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > --- > sound/soc/fsl/fsl_micfil.c | 45 ++++---------------------------------- > 1 file changed, 4 insertions(+), 41 deletions(-) > > diff --git a/sound/soc/fsl/fsl_micfil.c b/sound/soc/fsl/fsl_micfil.c > index 8335646a84d17..fd3b168a38661 100644 > --- a/sound/soc/fsl/fsl_micfil.c > +++ b/sound/soc/fsl/fsl_micfil.c > @@ -111,19 +111,6 @@ static const struct snd_kcontrol_new > fsl_micfil_snd_controls[] = { > snd_soc_get_enum_double, snd_soc_put_enum_double), > }; > > -static inline int get_clk_div(struct fsl_micfil *micfil, > - unsigned int rate) > -{ > - long mclk_rate; > - int clk_div; > - > - mclk_rate = clk_get_rate(micfil->mclk); > - > - clk_div = mclk_rate / (rate * micfil->osr * 8); > - > - return clk_div; > -} > - > /* The SRES is a self-negated bit which provides the CPU with the > * capability to initialize the PDM Interface module through the > * slave-bus interface. This bit always reads as zero, and this > @@ -147,24 +134,6 @@ static int fsl_micfil_reset(struct device *dev) > return 0; > } > > -static int fsl_micfil_set_mclk_rate(struct fsl_micfil *micfil, > - unsigned int freq) > -{ > - struct device *dev = &micfil->pdev->dev; > - int ret; > - > - clk_disable_unprepare(micfil->mclk); > - > - ret = clk_set_rate(micfil->mclk, freq * 1024); > - if (ret) > - dev_warn(dev, "failed to set rate (%u): %d\n", > - freq * 1024, ret); > - > - clk_prepare_enable(micfil->mclk); > - > - return ret; > -} > - > static int fsl_micfil_startup(struct snd_pcm_substream *substream, > struct snd_soc_dai *dai) > { > @@ -238,13 +207,12 @@ static int fsl_micfil_trigger(struct > snd_pcm_substream *substream, int cmd, > static int fsl_set_clock_params(struct device *dev, unsigned int rate) > { > struct fsl_micfil *micfil = dev_get_drvdata(dev); > - int clk_div; > + int clk_div = 8; > int ret; > > - ret = fsl_micfil_set_mclk_rate(micfil, rate); > - if (ret < 0) > - dev_err(dev, "failed to set mclk[%lu] to rate %u\n", > - clk_get_rate(micfil->mclk), rate); > + ret = clk_set_rate(micfil->mclk, rate * clk_div * micfil->osr * 8); > Please make sure micfil->osr is assigned. > + if (ret) > + return ret; > > /* set CICOSR */ > ret = regmap_update_bits(micfil->regmap, REG_MICFIL_CTRL2, > @@ -253,11 +221,6 @@ static int fsl_set_clock_params(struct device *dev, > unsigned int rate) > if (ret) > return ret; > > - /* set CLK_DIV */ > - clk_div = get_clk_div(micfil, rate); > - if (clk_div < 0) > - ret = -EINVAL; > - > ret = regmap_update_bits(micfil->regmap, REG_MICFIL_CTRL2, > MICFIL_CTRL2_CLKDIV, > FIELD_PREP(MICFIL_CTRL2_CLKDIV, clk_div)); > -- > 2.30.2 > >
On Thu, Apr 07, 2022 at 01:09:37PM +0800, Shengjiu Wang wrote: > On Tue, Apr 5, 2022 at 4:00 PM Sascha Hauer <[1]s.hauer@pengutronix.de> > wrote: > > The reference manual has this for calculating the micfil internal clock > divider: > > MICFIL Clock rate > clkdiv = ----------------- > 8 * OSR * outrate > > (with OSR == Oversampling Rate, outrate == output sample rate) > > The driver first sets the MICFIL Clock rate to (outrate * 1024) and then > calculates back the clkdiv value from the above calculation. > > Simplify this by using a fixed clkdiv value of 8 and set the MICFIL > Clock rate to (outrate * clkdiv * OSR * 8). > > While at it drop disabling the clock before setting its rate. The MICFIL > module is disabled when the rate is changed and it is also resetted > before it is started again, so I doubt it's necessary to disable the > clock. > > Signed-off-by: Sascha Hauer <[2]s.hauer@pengutronix.de> > --- > sound/soc/fsl/fsl_micfil.c | 45 ++++---------------------------------- > 1 file changed, 4 insertions(+), 41 deletions(-) > > diff --git a/sound/soc/fsl/fsl_micfil.c b/sound/soc/fsl/fsl_micfil.c > index 8335646a84d17..fd3b168a38661 100644 > --- a/sound/soc/fsl/fsl_micfil.c > +++ b/sound/soc/fsl/fsl_micfil.c > @@ -111,19 +111,6 @@ static const struct snd_kcontrol_new > fsl_micfil_snd_controls[] = { > snd_soc_get_enum_double, snd_soc_put_enum_double), > }; > > -static inline int get_clk_div(struct fsl_micfil *micfil, > - unsigned int rate) > -{ > - long mclk_rate; > - int clk_div; > - > - mclk_rate = clk_get_rate(micfil->mclk); > - > - clk_div = mclk_rate / (rate * micfil->osr * 8); > - > - return clk_div; > -} > - > /* The SRES is a self-negated bit which provides the CPU with the > * capability to initialize the PDM Interface module through the > * slave-bus interface. This bit always reads as zero, and this > @@ -147,24 +134,6 @@ static int fsl_micfil_reset(struct device *dev) > return 0; > } > > -static int fsl_micfil_set_mclk_rate(struct fsl_micfil *micfil, > - unsigned int freq) > -{ > - struct device *dev = &micfil->pdev->dev; > - int ret; > - > - clk_disable_unprepare(micfil->mclk); > - > - ret = clk_set_rate(micfil->mclk, freq * 1024); > - if (ret) > - dev_warn(dev, "failed to set rate (%u): %d\n", > - freq * 1024, ret); > - > - clk_prepare_enable(micfil->mclk); > - > - return ret; > -} > - > static int fsl_micfil_startup(struct snd_pcm_substream *substream, > struct snd_soc_dai *dai) > { > @@ -238,13 +207,12 @@ static int fsl_micfil_trigger(struct > snd_pcm_substream *substream, int cmd, > static int fsl_set_clock_params(struct device *dev, unsigned int rate) > { > struct fsl_micfil *micfil = dev_get_drvdata(dev); > - int clk_div; > + int clk_div = 8; > int ret; > > - ret = fsl_micfil_set_mclk_rate(micfil, rate); > - if (ret < 0) > - dev_err(dev, "failed to set mclk[%lu] to rate %u\n", > - clk_get_rate(micfil->mclk), rate); > + ret = clk_set_rate(micfil->mclk, rate * clk_div * micfil->osr * > 8); > > Please make sure micfil->osr is assigned. Should also be MICFIL_OSR_DEFAULT instead. The micfil->osr field sneaked in during development and I decided against it finally. Sascha
diff --git a/sound/soc/fsl/fsl_micfil.c b/sound/soc/fsl/fsl_micfil.c index 8335646a84d17..fd3b168a38661 100644 --- a/sound/soc/fsl/fsl_micfil.c +++ b/sound/soc/fsl/fsl_micfil.c @@ -111,19 +111,6 @@ static const struct snd_kcontrol_new fsl_micfil_snd_controls[] = { snd_soc_get_enum_double, snd_soc_put_enum_double), }; -static inline int get_clk_div(struct fsl_micfil *micfil, - unsigned int rate) -{ - long mclk_rate; - int clk_div; - - mclk_rate = clk_get_rate(micfil->mclk); - - clk_div = mclk_rate / (rate * micfil->osr * 8); - - return clk_div; -} - /* The SRES is a self-negated bit which provides the CPU with the * capability to initialize the PDM Interface module through the * slave-bus interface. This bit always reads as zero, and this @@ -147,24 +134,6 @@ static int fsl_micfil_reset(struct device *dev) return 0; } -static int fsl_micfil_set_mclk_rate(struct fsl_micfil *micfil, - unsigned int freq) -{ - struct device *dev = &micfil->pdev->dev; - int ret; - - clk_disable_unprepare(micfil->mclk); - - ret = clk_set_rate(micfil->mclk, freq * 1024); - if (ret) - dev_warn(dev, "failed to set rate (%u): %d\n", - freq * 1024, ret); - - clk_prepare_enable(micfil->mclk); - - return ret; -} - static int fsl_micfil_startup(struct snd_pcm_substream *substream, struct snd_soc_dai *dai) { @@ -238,13 +207,12 @@ static int fsl_micfil_trigger(struct snd_pcm_substream *substream, int cmd, static int fsl_set_clock_params(struct device *dev, unsigned int rate) { struct fsl_micfil *micfil = dev_get_drvdata(dev); - int clk_div; + int clk_div = 8; int ret; - ret = fsl_micfil_set_mclk_rate(micfil, rate); - if (ret < 0) - dev_err(dev, "failed to set mclk[%lu] to rate %u\n", - clk_get_rate(micfil->mclk), rate); + ret = clk_set_rate(micfil->mclk, rate * clk_div * micfil->osr * 8); + if (ret) + return ret; /* set CICOSR */ ret = regmap_update_bits(micfil->regmap, REG_MICFIL_CTRL2, @@ -253,11 +221,6 @@ static int fsl_set_clock_params(struct device *dev, unsigned int rate) if (ret) return ret; - /* set CLK_DIV */ - clk_div = get_clk_div(micfil, rate); - if (clk_div < 0) - ret = -EINVAL; - ret = regmap_update_bits(micfil->regmap, REG_MICFIL_CTRL2, MICFIL_CTRL2_CLKDIV, FIELD_PREP(MICFIL_CTRL2_CLKDIV, clk_div));
The reference manual has this for calculating the micfil internal clock divider: MICFIL Clock rate clkdiv = ----------------- 8 * OSR * outrate (with OSR == Oversampling Rate, outrate == output sample rate) The driver first sets the MICFIL Clock rate to (outrate * 1024) and then calculates back the clkdiv value from the above calculation. Simplify this by using a fixed clkdiv value of 8 and set the MICFIL Clock rate to (outrate * clkdiv * OSR * 8). While at it drop disabling the clock before setting its rate. The MICFIL module is disabled when the rate is changed and it is also resetted before it is started again, so I doubt it's necessary to disable the clock. Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> --- sound/soc/fsl/fsl_micfil.c | 45 ++++---------------------------------- 1 file changed, 4 insertions(+), 41 deletions(-)