All of lore.kernel.org
 help / color / mirror / Atom feed
From: Hans de Goede <hdegoede@redhat.com>
To: Mark Brown <broonie@kernel.org>, Bard Liao <bardliao@realtek.com>
Cc: Hans de Goede <hdegoede@redhat.com>,
	alsa-devel@alsa-project.org, Carlo Caione <carlo@endlessm.com>,
	Takashi Iwai <tiwai@suse.com>
Subject: [PATCH 02/11] ASoC: rt5651: Remove is_sys_clk_from_pll, it has ordering issues
Date: Tue, 20 Feb 2018 23:15:02 +0100	[thread overview]
Message-ID: <20180220221511.22861-2-hdegoede@redhat.com> (raw)
In-Reply-To: <20180220221511.22861-1-hdegoede@redhat.com>

dapm_power_widgets() first builds a list of which widgets to power-up
before actually powering any of them up. For dapm-supply widgets their
connected method, in our case is_sys_clk_from_pll() get called at this
point.

Before this commit is_sys_clk_from_pll() was looking at the actually
selected clock in the RT5651_GBL_CLK register. But the sysclk itself is
selected by another dapm-supply (the "Platform Clock" supply in the
machine driver) and any changes to that supply part of the same power-
transition get executed after building the list, causing
is_sys_clk_from_pll() to return the wrong value.

This sometimes leads to the PWR_PLL bit not getting set even though we
have configured an active audio chain and RT5651_GBL_CLK does point to
PLL1 as the sysclk-source.

Note that even if we do not have an active audio chain we should still keep
the PWR_PLL bit set if PLL1 is our sysclk-source, because we must always
have a working sysclk-source, otherwise e.g. jack-detection will not work.

If we don't have an active audio-chain then the machine-driver should
switch the sysclk-source to the RCCLK and if does not then that is a
machine-driver (or UCM config) problem and not something we should try
to work around by disabling our sysclk-source and breaking jack-detect.

TL;DR: The PLL1-supply must always be powered up when PLL1 is the sysclk
source and we can simply set the RT5651_PWR_PLL bit when selecting PLL1.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Note that many of the other rt56?? drivers have the same
is_sys_clk_from_pll() stuff and may need similar fixes.
---
 sound/soc/codecs/rt5651.c | 25 +++++--------------------
 1 file changed, 5 insertions(+), 20 deletions(-)

diff --git a/sound/soc/codecs/rt5651.c b/sound/soc/codecs/rt5651.c
index c5d19a80506b..a48278f6205d 100644
--- a/sound/soc/codecs/rt5651.c
+++ b/sound/soc/codecs/rt5651.c
@@ -393,20 +393,6 @@ static int set_dmic_clk(struct snd_soc_dapm_widget *w,
 	return idx;
 }
 
-static int is_sysclk_from_pll(struct snd_soc_dapm_widget *source,
-			 struct snd_soc_dapm_widget *sink)
-{
-	struct snd_soc_codec *codec = snd_soc_dapm_to_codec(source->dapm);
-	unsigned int val;
-
-	val = snd_soc_read(codec, RT5651_GLB_CLK);
-	val &= RT5651_SCLK_SRC_MASK;
-	if (val == RT5651_SCLK_SRC_PLL1)
-		return 1;
-	else
-		return 0;
-}
-
 /* Digital Mixer */
 static const struct snd_kcontrol_new rt5651_sto1_adc_l_mix[] = {
 	SOC_DAPM_SINGLE("ADC1 Switch", RT5651_STO1_ADC_MIXER,
@@ -878,8 +864,6 @@ static const struct snd_soc_dapm_widget rt5651_dapm_widgets[] = {
 	SND_SOC_DAPM_SUPPLY_S("ADC ASRC", 1, RT5651_PLL_MODE_2,
 			      11, 0, NULL, 0),
 
-	SND_SOC_DAPM_SUPPLY("PLL1", RT5651_PWR_ANLG2,
-			RT5651_PWR_PLL_BIT, 0, NULL, 0),
 	/* Input Side */
 	SND_SOC_DAPM_SUPPLY("JD Power", RT5651_PWR_ANLG2,
 		RT5651_PWM_JD_M_BIT, 0, NULL, 0),
@@ -1162,7 +1146,6 @@ static const struct snd_soc_dapm_route rt5651_dapm_routes[] = {
 	{"Stereo1 ADC MIXL", "ADC1 Switch", "Stereo1 ADC L1 Mux"},
 	{"Stereo1 ADC MIXL", "ADC2 Switch", "Stereo1 ADC L2 Mux"},
 	{"Stereo1 ADC MIXL", NULL, "Stereo1 Filter"},
-	{"Stereo1 Filter", NULL, "PLL1", is_sysclk_from_pll},
 	{"Stereo1 Filter", NULL, "ADC ASRC"},
 
 	{"Stereo1 ADC MIXR", "ADC1 Switch", "Stereo1 ADC R1 Mux"},
@@ -1172,7 +1155,6 @@ static const struct snd_soc_dapm_route rt5651_dapm_routes[] = {
 	{"Stereo2 ADC MIXL", "ADC1 Switch", "Stereo2 ADC L1 Mux"},
 	{"Stereo2 ADC MIXL", "ADC2 Switch", "Stereo2 ADC L2 Mux"},
 	{"Stereo2 ADC MIXL", NULL, "Stereo2 Filter"},
-	{"Stereo2 Filter", NULL, "PLL1", is_sysclk_from_pll},
 	{"Stereo2 Filter", NULL, "ADC ASRC"},
 
 	{"Stereo2 ADC MIXR", "ADC1 Switch", "Stereo2 ADC R1 Mux"},
@@ -1239,10 +1221,8 @@ static const struct snd_soc_dapm_route rt5651_dapm_routes[] = {
 	{"PDM R Mux", "DD MIX", "DAC MIXR"},
 
 	{"DAC L1", NULL, "Stereo DAC MIXL"},
-	{"DAC L1", NULL, "PLL1", is_sysclk_from_pll},
 	{"DAC L1", NULL, "DAC L1 Power"},
 	{"DAC R1", NULL, "Stereo DAC MIXR"},
-	{"DAC R1", NULL, "PLL1", is_sysclk_from_pll},
 	{"DAC R1", NULL, "DAC R1 Power"},
 
 	{"DD MIXL", "DAC L1 Switch", "DAC MIXL"},
@@ -1438,6 +1418,7 @@ static int rt5651_set_dai_sysclk(struct snd_soc_dai *dai,
 	struct snd_soc_codec *codec = dai->codec;
 	struct rt5651_priv *rt5651 = snd_soc_codec_get_drvdata(codec);
 	unsigned int reg_val = 0;
+	unsigned int pll_bit = 0;
 
 	if (freq == rt5651->sysclk && clk_id == rt5651->sysclk_src)
 		return 0;
@@ -1448,6 +1429,7 @@ static int rt5651_set_dai_sysclk(struct snd_soc_dai *dai,
 		break;
 	case RT5651_SCLK_S_PLL1:
 		reg_val |= RT5651_SCLK_SRC_PLL1;
+		pll_bit |= RT5651_PWR_PLL;
 		break;
 	case RT5651_SCLK_S_RCCLK:
 		reg_val |= RT5651_SCLK_SRC_RCCLK;
@@ -1456,6 +1438,9 @@ static int rt5651_set_dai_sysclk(struct snd_soc_dai *dai,
 		dev_err(codec->dev, "Invalid clock id (%d)\n", clk_id);
 		return -EINVAL;
 	}
+
+	snd_soc_update_bits(codec, RT5651_PWR_ANLG2,
+		RT5651_PWR_PLL, pll_bit);
 	snd_soc_update_bits(codec, RT5651_GLB_CLK,
 		RT5651_SCLK_SRC_MASK, reg_val);
 	rt5651->sysclk = freq;
-- 
2.14.3

  reply	other threads:[~2018-02-20 22:15 UTC|newest]

Thread overview: 35+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-02-20 22:15 [PATCH 01/11] ASoC: rt5651: Avoid duplicating DMI quirks between codec and machine driver Hans de Goede
2018-02-20 22:15 ` Hans de Goede [this message]
2018-02-21 11:18   ` [PATCH 02/11] ASoC: rt5651: Remove is_sys_clk_from_pll, it has ordering issues Mark Brown
2018-02-21 19:38     ` Hans de Goede
2018-02-22 10:48       ` Mark Brown
2018-02-22 11:00         ` Hans de Goede
2018-02-22 11:13           ` Mark Brown
2018-02-20 22:15 ` [PATCH 03/11] ASoC: rt5651: Fix bias_level confusion / overcurrent detection deps Hans de Goede
2018-02-21 11:40   ` Mark Brown
2018-02-21 19:43     ` Hans de Goede
2018-02-22 10:52       ` Mark Brown
2018-02-22 11:04         ` Hans de Goede
2018-02-20 22:15 ` [PATCH 04/11] ASoC: rt5651: Simplify set_bias_level() Hans de Goede
2018-02-21 11:43   ` Mark Brown
2018-02-21 20:11     ` Hans de Goede
2018-02-22 11:12       ` Mark Brown
2018-02-20 22:15 ` [PATCH 05/11] ASoC: rt5651: Allow specifying micbias over-current thresholds through pdata Hans de Goede
2018-02-20 22:15 ` [PATCH 06/11] ASoC: rt5651: Improve headphone vs headset detection Hans de Goede
2018-02-21 12:05   ` Mark Brown
2018-02-21 20:22     ` Hans de Goede
2018-02-20 22:15 ` [PATCH 07/11] ASoC: Intel: bytcr_rt5651: Configure PLL1 before using it Hans de Goede
2018-02-20 22:15 ` [PATCH 08/11] ASoC: Intel: bytcr_rt5651: Rename IN3_MAP to IN1_HS_IN3_MAP Hans de Goede
2018-02-20 22:15 ` [PATCH 09/11] ASoC: Intel: bytcr_rt5651: Add new IN2_HS_IN3 input map and a quirk using it Hans de Goede
2018-02-20 22:15 ` [PATCH 10/11] ASoC: Intel: bytcr_rt5651: Add support for Bay Trail CR / SSP0 using boards Hans de Goede
2018-02-20 22:44   ` Pierre-Louis Bossart
2018-02-24 11:52     ` Hans de Goede
2018-02-20 22:15 ` [PATCH 11/11] ASoC: Intel: bytcr_rt5651: Add quirk for the VIOS LTH17 laptop Hans de Goede
2018-02-21 11:54 ` [PATCH 01/11] ASoC: rt5651: Avoid duplicating DMI quirks between codec and machine driver Mark Brown
2018-02-21 19:23   ` Hans de Goede
2018-02-22 11:08     ` Hans de Goede
2018-02-22 11:35       ` Mark Brown
2018-02-22 20:38         ` Hans de Goede
2018-02-22 11:32     ` Mark Brown
2018-02-21 17:36 ` Carlo Caione
2018-02-21 20:55   ` Hans de Goede

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180220221511.22861-2-hdegoede@redhat.com \
    --to=hdegoede@redhat.com \
    --cc=alsa-devel@alsa-project.org \
    --cc=bardliao@realtek.com \
    --cc=broonie@kernel.org \
    --cc=carlo@endlessm.com \
    --cc=tiwai@suse.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.