diff mbox series

[4/5] ASoC: max98088: Add master clock handling

Message ID 20180925142352.24106-5-m.felsch@pengutronix.de (mailing list archive)
State New, archived
Headers show
Series MAX98088/9 of features | expand

Commit Message

Marco Felsch Sept. 25, 2018, 2:23 p.m. UTC 
From: Andreas Färber <afaerber@suse.de>

If master clock is provided through device tree, then update
the master clock frequency during set_sysclk.

Cc: Tushar Behera <tushar.behera@linaro.org>
Cc: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
Signed-off-by: Andreas Färber <afaerber@suse.de>
Acked-by: Tushar Behera <trblinux@gmail.com>
Reviewed-by: Javier Martinez Canillas <javier.martinez@collabora.co.uk>
[m.felsch@pengutronix.de: move mclk request to i2c_probe]
[m.felsch@pengutronix.de: make use of snd_soc_component_get_bias_level()]
Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 sound/soc/codecs/max98088.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

Comments

Mark Brown Sept. 25, 2018, 5:17 p.m. UTC | #1 
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.
Marco Felsch Sept. 26, 2018, 10 a.m. UTC | #2 
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
Mark Brown Sept. 26, 2018, 12:22 p.m. UTC | #3 
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.
Marco Felsch Sept. 26, 2018, 12:30 p.m. UTC | #4 
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 mbox series

Patch

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);