| Message ID | 20180925142352.24106-5-m.felsch@pengutronix.de |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/5] ASoC: dt-bindings: add max98088 audio codec | expand |
On Tue, Sep 25, 2018 at 04:23:51PM +0200, Marco Felsch wrote: > From: Andreas Färber <afaerber@suse.de> > > If master clock is provided through device tree, then update > the master clock frequency during set_sysclk. This changelog suggests that the clock is optional... > + if (!IS_ERR(max98088->mclk)) { > + freq = clk_round_rate(max98088->mclk, freq); > + clk_set_rate(max98088->mclk, freq); > + } ...as does much of the code (note BTw that this ignores errors setting the clock)... > + max98088->mclk = devm_clk_get(&i2c->dev, "mclk"); > + if (IS_ERR(max98088->mclk)) { > + if (PTR_ERR(max98088->mclk) == -EPROBE_DEFER) > + return -EPROBE_DEFER; > + dev_err(&i2c->dev, "Invalid MCLK\n"); > + return -EINVAL; > + } > + ...but the probe function makes it mandatory (and also throws away the error code from clk_get() if it's not -EPROBE_DEFER). Is this the best choice? It seems inconsistent anyway.
Hi Mark, thanks for review. On 18-09-25 10:17, Mark Brown wrote: > On Tue, Sep 25, 2018 at 04:23:51PM +0200, Marco Felsch wrote: > > From: Andreas Färber <afaerber@suse.de> > > > > If master clock is provided through device tree, then update > > the master clock frequency during set_sysclk. > > This changelog suggests that the clock is optional... Your are right, it was my fail to make it not optional. > > > + if (!IS_ERR(max98088->mclk)) { > > + freq = clk_round_rate(max98088->mclk, freq); > > + clk_set_rate(max98088->mclk, freq); > > + } > > ...as does much of the code (note BTw that this ignores errors setting > the clock)... > > > + max98088->mclk = devm_clk_get(&i2c->dev, "mclk"); > > + if (IS_ERR(max98088->mclk)) { > > + if (PTR_ERR(max98088->mclk) == -EPROBE_DEFER) > > + return -EPROBE_DEFER; > > + dev_err(&i2c->dev, "Invalid MCLK\n"); > > + return -EINVAL; > > + } > > + > > ...but the probe function makes it mandatory (and also throws away the > error code from clk_get() if it's not -EPROBE_DEFER). Is this the best > choice? It seems inconsistent anyway. One question, should it optional or not? If not I will fix it to return the clk_get error else I will drop it. It shouldn't be optional In my point of view, since it is needed by the chip. Regards, Marco
On Wed, Sep 26, 2018 at 12:00:30PM +0200, Marco Felsch wrote: > One question, should it optional or not? If not I will fix it to return > the clk_get error else I will drop it. It shouldn't be optional In my > point of view, since it is needed by the chip. Optional seems safer, requiring the clock isn't going to work so well on ACPI systems.
On 18-09-26 13:22, Mark Brown wrote: > On Wed, Sep 26, 2018 at 12:00:30PM +0200, Marco Felsch wrote: > > > One question, should it optional or not? If not I will fix it to return > > the clk_get error else I will drop it. It shouldn't be optional In my > > point of view, since it is needed by the chip. > > Optional seems safer, requiring the clock isn't going to work so well on > ACPI systems. Okay, I will integrate it in my v2.
diff --git a/sound/soc/codecs/max98088.c b/sound/soc/codecs/max98088.c index 9450d5d9c492..01f1ea0af9c1 100644 --- a/sound/soc/codecs/max98088.c +++ b/sound/soc/codecs/max98088.c @@ -16,6 +16,7 @@ #include <linux/pm.h> #include <linux/i2c.h> #include <linux/regmap.h> +#include <linux/clk.h> #include <sound/core.h> #include <sound/pcm.h> #include <sound/pcm_params.h> @@ -42,6 +43,7 @@ struct max98088_priv { struct regmap *regmap; enum max98088_type devtype; struct max98088_pdata *pdata; + struct clk *mclk; unsigned int sysclk; struct max98088_cdata dai[2]; int eq_textcnt; @@ -1103,6 +1105,11 @@ static int max98088_dai_set_sysclk(struct snd_soc_dai *dai, if (freq == max98088->sysclk) return 0; + if (!IS_ERR(max98088->mclk)) { + freq = clk_round_rate(max98088->mclk, freq); + clk_set_rate(max98088->mclk, freq); + } + /* Setup clocks for slave mode, and using the PLL * PSCLK = 0x01 (when master clk is 10MHz to 20MHz) * 0x02 (when master clk is 20MHz to 30MHz).. @@ -1310,6 +1317,20 @@ static int max98088_set_bias_level(struct snd_soc_component *component, break; case SND_SOC_BIAS_PREPARE: + /* + * SND_SOC_BIAS_PREPARE is called while preparing for a + * transition to ON or away from ON. If current bias_level + * is SND_SOC_BIAS_ON, then it is preparing for a transition + * away from ON. Disable the clock in that case, otherwise + * enable it. + */ + if (!IS_ERR(max98088->mclk)) { + if (snd_soc_component_get_bias_level(component) == + SND_SOC_BIAS_ON) + clk_disable_unprepare(max98088->mclk); + else + clk_prepare_enable(max98088->mclk); + } break; case SND_SOC_BIAS_STANDBY: @@ -1725,6 +1746,14 @@ static int max98088_i2c_probe(struct i2c_client *i2c, if (IS_ERR(max98088->regmap)) return PTR_ERR(max98088->regmap); + max98088->mclk = devm_clk_get(&i2c->dev, "mclk"); + if (IS_ERR(max98088->mclk)) { + if (PTR_ERR(max98088->mclk) == -EPROBE_DEFER) + return -EPROBE_DEFER; + dev_err(&i2c->dev, "Invalid MCLK\n"); + return -EINVAL; + } + max98088->devtype = id->driver_data; i2c_set_clientdata(i2c, max98088);