* [PATCH v5 0/6] VAG power control improvement for sgtl5000 codec
@ 2019-07-18 9:02 Oleksandr Suvorov
2019-07-18 9:02 ` [PATCH v5 1/6] ASoC: Define a set of DAPM pre/post-up events Oleksandr Suvorov
` (5 more replies)
0 siblings, 6 replies; 13+ messages in thread
From: Oleksandr Suvorov @ 2019-07-18 9:02 UTC (permalink / raw
To: Fabio Estevam
Cc: linux-kernel@vger.kernel.org, Igor Opaniuk, Marcel Ziswiler,
alsa-devel@alsa-project.org, Oleksandr Suvorov, Jaroslav Kysela,
Sasha Levin, Mark Brown, stable@vger.kernel.org, Takashi Iwai,
Liam Girdwood
VAG power control is improved to fit the manual [1]. This patchset fixes as
minimum one bug: if customer muxes Headphone to Line-In right after boot,
the VAG power remains off that leads to poor sound quality from line-in.
I.e. after boot:
- Connect sound source to Line-In jack;
- Connect headphone to HP jack;
- Run following commands:
$ amixer set 'Headphone' 80%
$ amixer set 'Headphone Mux' LINE_IN
Also this series includes fixes of non-important bugs in sgtl5000 codec
driver.
[1] https://www.nxp.com/docs/en/data-sheet/SGTL5000.pdf
Changes in v5:
- Add explicit stable tag
- Improve commit message
- Add explicit stable tag
Changes in v4:
- CC the patch to kernel-stable
- Code optimization, simplify function signature
(thanks to Cezary Rojewski <cezary.rojewski@intel.com> for an idea)
- CC the patch to kernel-stable
- Add a Fixes tag
Changes in v3:
- Add the reference to NXP SGTL5000 data sheet to commit message
- Add the reference to NXP SGTL5000 data sheet to commit message
- Fix multi-line comment format
Changes in v2:
- Fix patch formatting
- Fix patch formatting
- Fix patch formatting
- Fix patch formatting
- Fix patch formatting
- Fix patch formatting
Oleksandr Suvorov (6):
ASoC: Define a set of DAPM pre/post-up events
ASoC: sgtl5000: Improve VAG power and mute control
ASoC: sgtl5000: Fix definition of VAG Ramp Control
ASoC: sgtl5000: add ADC mute control
ASoC: sgtl5000: Fix of unmute outputs on probe
ASoC: sgtl5000: Fix charge pump source assignment
include/sound/soc-dapm.h | 2 +
sound/soc/codecs/sgtl5000.c | 240 ++++++++++++++++++++++++++++++------
sound/soc/codecs/sgtl5000.h | 2 +-
3 files changed, 203 insertions(+), 41 deletions(-)
--
2.20.1
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v5 1/6] ASoC: Define a set of DAPM pre/post-up events
2019-07-18 9:02 [PATCH v5 0/6] VAG power control improvement for sgtl5000 codec Oleksandr Suvorov
@ 2019-07-18 9:02 ` Oleksandr Suvorov
2019-07-18 9:02 ` [PATCH v5 2/6] ASoC: sgtl5000: Improve VAG power and mute control Oleksandr Suvorov
` (4 subsequent siblings)
5 siblings, 0 replies; 13+ messages in thread
From: Oleksandr Suvorov @ 2019-07-18 9:02 UTC (permalink / raw
To: Fabio Estevam
Cc: linux-kernel@vger.kernel.org, Igor Opaniuk, Marcel Ziswiler,
alsa-devel@alsa-project.org, Oleksandr Suvorov,
stable@vger.kernel.org, Jaroslav Kysela, Sasha Levin, Mark Brown,
Takashi Iwai, Liam Girdwood
Prepare to use SND_SOC_DAPM_PRE_POST_PMU definition to
reduce coming code size and make it more readable.
Cc: stable@vger.kernel.org
Signed-off-by: Oleksandr Suvorov <oleksandr.suvorov@toradex.com>
Reviewed-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>
Reviewed-by: Igor Opaniuk <igor.opaniuk@toradex.com>
Reviewed-by: Fabio Estevam <festevam@gmail.com>
---
Changes in v5:
- Add explicit stable tag
Changes in v4:
- CC the patch to kernel-stable
Changes in v3: None
Changes in v2:
- Fix patch formatting
include/sound/soc-dapm.h | 2 ++
1 file changed, 2 insertions(+)
diff --git a/include/sound/soc-dapm.h b/include/sound/soc-dapm.h
index c00a0b8ade086..6c66941601307 100644
--- a/include/sound/soc-dapm.h
+++ b/include/sound/soc-dapm.h
@@ -353,6 +353,8 @@ struct device;
#define SND_SOC_DAPM_WILL_PMD 0x80 /* called at start of sequence */
#define SND_SOC_DAPM_PRE_POST_PMD \
(SND_SOC_DAPM_PRE_PMD | SND_SOC_DAPM_POST_PMD)
+#define SND_SOC_DAPM_PRE_POST_PMU \
+ (SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_POST_PMU)
/* convenience event type detection */
#define SND_SOC_DAPM_EVENT_ON(e) \
--
2.20.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v5 2/6] ASoC: sgtl5000: Improve VAG power and mute control
2019-07-18 9:02 [PATCH v5 0/6] VAG power control improvement for sgtl5000 codec Oleksandr Suvorov
2019-07-18 9:02 ` [PATCH v5 1/6] ASoC: Define a set of DAPM pre/post-up events Oleksandr Suvorov
@ 2019-07-18 9:02 ` Oleksandr Suvorov
2019-07-18 16:25 ` Igor Opaniuk
2019-07-18 18:42 ` Cezary Rojewski
2019-07-18 9:02 ` [PATCH v5 3/6] ASoC: sgtl5000: Fix definition of VAG Ramp Control Oleksandr Suvorov
` (3 subsequent siblings)
5 siblings, 2 replies; 13+ messages in thread
From: Oleksandr Suvorov @ 2019-07-18 9:02 UTC (permalink / raw
To: Fabio Estevam
Cc: linux-kernel@vger.kernel.org, Igor Opaniuk, Marcel Ziswiler,
alsa-devel@alsa-project.org, Oleksandr Suvorov,
stable@vger.kernel.org, Jaroslav Kysela, Sasha Levin, Mark Brown,
Takashi Iwai, Liam Girdwood
VAG power control is improved to fit the manual [1]. This patch fixes as
minimum one bug: if customer muxes Headphone to Line-In right after boot,
the VAG power remains off that leads to poor sound quality from line-in.
I.e. after boot:
- Connect sound source to Line-In jack;
- Connect headphone to HP jack;
- Run following commands:
$ amixer set 'Headphone' 80%
$ amixer set 'Headphone Mux' LINE_IN
Change VAG power on/off control according to the following algorithm:
- turn VAG power ON on the 1st incoming event.
- keep it ON if there is any active VAG consumer (ADC/DAC/HP/Line-In).
- turn VAG power OFF when there is the latest consumer's pre-down event
come.
- always delay after VAG power OFF to avoid pop.
- delay after VAG power ON if the initiative consumer is Line-In, this
prevents pop during line-in muxing.
According to the data sheet [1], to avoid any pops/clicks,
the outputs should be muted during input/output
routing changes.
[1] https://www.nxp.com/docs/en/data-sheet/SGTL5000.pdf
Cc: stable@vger.kernel.org
Fixes: 9b34e6cc3bc2 ("ASoC: Add Freescale SGTL5000 codec support")
Signed-off-by: Oleksandr Suvorov <oleksandr.suvorov@toradex.com>
Reviewed-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>
Reviewed-by: Fabio Estevam <festevam@gmail.com>
---
Changes in v5:
- Improve commit message
- Add explicit stable tag
Changes in v4:
- Code optimization, simplify function signature
(thanks to Cezary Rojewski <cezary.rojewski@intel.com> for an idea)
- CC the patch to kernel-stable
- Add a Fixes tag
Changes in v3:
- Add the reference to NXP SGTL5000 data sheet to commit message
Changes in v2:
- Fix patch formatting
sound/soc/codecs/sgtl5000.c | 216 ++++++++++++++++++++++++++++++------
1 file changed, 185 insertions(+), 31 deletions(-)
diff --git a/sound/soc/codecs/sgtl5000.c b/sound/soc/codecs/sgtl5000.c
index a6a4748c97f9d..a2aeb5fb9b537 100644
--- a/sound/soc/codecs/sgtl5000.c
+++ b/sound/soc/codecs/sgtl5000.c
@@ -31,6 +31,13 @@
#define SGTL5000_DAP_REG_OFFSET 0x0100
#define SGTL5000_MAX_REG_OFFSET 0x013A
+/* Delay for the VAG ramp up */
+#define SGTL5000_VAG_POWERUP_DELAY 500 /* ms */
+/* Delay for the VAG ramp down */
+#define SGTL5000_VAG_POWERDOWN_DELAY 500 /* ms */
+
+#define SGTL5000_OUTPUTS_MUTE (SGTL5000_HP_MUTE | SGTL5000_LINE_OUT_MUTE)
+
/* default value of sgtl5000 registers */
static const struct reg_default sgtl5000_reg_defaults[] = {
{ SGTL5000_CHIP_DIG_POWER, 0x0000 },
@@ -123,6 +130,19 @@ enum {
I2S_SCLK_STRENGTH_HIGH,
};
+enum {
+ HP_POWER_EVENT,
+ DAC_POWER_EVENT,
+ ADC_POWER_EVENT,
+ LAST_POWER_EVENT
+};
+
+static u16 mute_mask[] = {
+ SGTL5000_HP_MUTE,
+ SGTL5000_OUTPUTS_MUTE,
+ SGTL5000_OUTPUTS_MUTE
+};
+
/* sgtl5000 private structure in codec */
struct sgtl5000_priv {
int sysclk; /* sysclk rate */
@@ -137,8 +157,109 @@ struct sgtl5000_priv {
u8 micbias_voltage;
u8 lrclk_strength;
u8 sclk_strength;
+ u16 mute_state[LAST_POWER_EVENT];
};
+static inline int hp_sel_input(struct snd_soc_component *component)
+{
+ return (snd_soc_component_read32(component, SGTL5000_CHIP_ANA_CTRL) &
+ SGTL5000_HP_SEL_MASK) >> SGTL5000_HP_SEL_SHIFT;
+}
+
+static inline u16 mute_output(struct snd_soc_component *component,
+ u16 mute_mask)
+{
+ u16 mute_reg = snd_soc_component_read32(component,
+ SGTL5000_CHIP_ANA_CTRL);
+
+ snd_soc_component_update_bits(component, SGTL5000_CHIP_ANA_CTRL,
+ mute_mask, mute_mask);
+ return mute_reg;
+}
+
+static inline void restore_output(struct snd_soc_component *component,
+ u16 mute_mask, u16 mute_reg)
+{
+ snd_soc_component_update_bits(component, SGTL5000_CHIP_ANA_CTRL,
+ mute_mask, mute_reg);
+}
+
+static void vag_power_on(struct snd_soc_component *component, u32 source)
+{
+ if (snd_soc_component_read32(component, SGTL5000_CHIP_ANA_POWER) &
+ SGTL5000_VAG_POWERUP)
+ return;
+
+ snd_soc_component_update_bits(component, SGTL5000_CHIP_ANA_POWER,
+ SGTL5000_VAG_POWERUP, SGTL5000_VAG_POWERUP);
+
+ /* When VAG powering on to get local loop from Line-In, the sleep
+ * is required to avoid loud pop.
+ */
+ if (hp_sel_input(component) == SGTL5000_HP_SEL_LINE_IN &&
+ source == HP_POWER_EVENT)
+ msleep(SGTL5000_VAG_POWERUP_DELAY);
+}
+
+static int vag_power_consumers(struct snd_soc_component *component,
+ u16 ana_pwr_reg, u32 source)
+{
+ int consumers = 0;
+
+ /* count dac/adc consumers unconditional */
+ if (ana_pwr_reg & SGTL5000_DAC_POWERUP)
+ consumers++;
+ if (ana_pwr_reg & SGTL5000_ADC_POWERUP)
+ consumers++;
+
+ /*
+ * If the event comes from HP and Line-In is selected,
+ * current action is 'DAC to be powered down'.
+ * As HP_POWERUP is not set when HP muxed to line-in,
+ * we need to keep VAG power ON.
+ */
+ if (source == HP_POWER_EVENT) {
+ if (hp_sel_input(component) == SGTL5000_HP_SEL_LINE_IN)
+ consumers++;
+ } else {
+ if (ana_pwr_reg & SGTL5000_HP_POWERUP)
+ consumers++;
+ }
+
+ return consumers;
+}
+
+static void vag_power_off(struct snd_soc_component *component, u32 source)
+{
+ u16 ana_pwr = snd_soc_component_read32(component,
+ SGTL5000_CHIP_ANA_POWER);
+
+ if (!(ana_pwr & SGTL5000_VAG_POWERUP))
+ return;
+
+ /*
+ * This function calls when any of VAG power consumers is disappearing.
+ * Thus, if there is more than one consumer at the moment, as minimum
+ * one consumer will definitely stay after the end of the current
+ * event.
+ * Don't clear VAG_POWERUP if 2 or more consumers of VAG present:
+ * - LINE_IN (for HP events) / HP (for DAC/ADC events)
+ * - DAC
+ * - ADC
+ * (the current consumer is disappearing right now)
+ */
+ if (vag_power_consumers(component, ana_pwr, source) >= 2)
+ return;
+
+ snd_soc_component_update_bits(component, SGTL5000_CHIP_ANA_POWER,
+ SGTL5000_VAG_POWERUP, 0);
+ /* In power down case, we need wait 400-1000 ms
+ * when VAG fully ramped down.
+ * As longer we wait, as smaller pop we've got.
+ */
+ msleep(SGTL5000_VAG_POWERDOWN_DELAY);
+}
+
/*
* mic_bias power on/off share the same register bits with
* output impedance of mic bias, when power on mic bias, we
@@ -170,36 +291,30 @@ static int mic_bias_event(struct snd_soc_dapm_widget *w,
return 0;
}
-/*
- * As manual described, ADC/DAC only works when VAG powerup,
- * So enabled VAG before ADC/DAC up.
- * In power down case, we need wait 400ms when vag fully ramped down.
- */
-static int power_vag_event(struct snd_soc_dapm_widget *w,
- struct snd_kcontrol *kcontrol, int event)
+static int vag_and_mute_control(struct snd_soc_component *component,
+ int event, int event_source)
{
- struct snd_soc_component *component = snd_soc_dapm_to_component(w->dapm);
- const u32 mask = SGTL5000_DAC_POWERUP | SGTL5000_ADC_POWERUP;
+ struct sgtl5000_priv *sgtl5000 =
+ snd_soc_component_get_drvdata(component);
switch (event) {
- case SND_SOC_DAPM_POST_PMU:
- snd_soc_component_update_bits(component, SGTL5000_CHIP_ANA_POWER,
- SGTL5000_VAG_POWERUP, SGTL5000_VAG_POWERUP);
- msleep(400);
+ case SND_SOC_DAPM_PRE_PMU:
+ sgtl5000->mute_state[event_source] =
+ mute_output(component, mute_mask[event_source]);
+ break;
+ case SND_SOC_DAPM_POST_PMU:
+ vag_power_on(component, event_source);
+ restore_output(component, mute_mask[event_source],
+ sgtl5000->mute_state[event_source]);
break;
-
case SND_SOC_DAPM_PRE_PMD:
- /*
- * Don't clear VAG_POWERUP, when both DAC and ADC are
- * operational to prevent inadvertently starving the
- * other one of them.
- */
- if ((snd_soc_component_read32(component, SGTL5000_CHIP_ANA_POWER) &
- mask) != mask) {
- snd_soc_component_update_bits(component, SGTL5000_CHIP_ANA_POWER,
- SGTL5000_VAG_POWERUP, 0);
- msleep(400);
- }
+ sgtl5000->mute_state[event_source] =
+ mute_output(component, mute_mask[event_source]);
+ vag_power_off(component, event_source);
+ break;
+ case SND_SOC_DAPM_POST_PMD:
+ restore_output(component, mute_mask[event_source],
+ sgtl5000->mute_state[event_source]);
break;
default:
break;
@@ -208,6 +323,41 @@ static int power_vag_event(struct snd_soc_dapm_widget *w,
return 0;
}
+/*
+ * Mute Headphone when power it up/down.
+ * Control VAG power on HP power path.
+ */
+static int headphone_pga_event(struct snd_soc_dapm_widget *w,
+ struct snd_kcontrol *kcontrol, int event)
+{
+ struct snd_soc_component *component =
+ snd_soc_dapm_to_component(w->dapm);
+
+ return vag_and_mute_control(component, event, HP_POWER_EVENT);
+}
+
+/* As manual describes, ADC/DAC powering up/down requires
+ * to mute outputs to avoid pops.
+ * Control VAG power on ADC/DAC power path.
+ */
+static int adc_updown_depop(struct snd_soc_dapm_widget *w,
+ struct snd_kcontrol *kcontrol, int event)
+{
+ struct snd_soc_component *component =
+ snd_soc_dapm_to_component(w->dapm);
+
+ return vag_and_mute_control(component, event, ADC_POWER_EVENT);
+}
+
+static int dac_updown_depop(struct snd_soc_dapm_widget *w,
+ struct snd_kcontrol *kcontrol, int event)
+{
+ struct snd_soc_component *component =
+ snd_soc_dapm_to_component(w->dapm);
+
+ return vag_and_mute_control(component, event, DAC_POWER_EVENT);
+}
+
/* input sources for ADC */
static const char *adc_mux_text[] = {
"MIC_IN", "LINE_IN"
@@ -280,7 +430,10 @@ static const struct snd_soc_dapm_widget sgtl5000_dapm_widgets[] = {
mic_bias_event,
SND_SOC_DAPM_POST_PMU | SND_SOC_DAPM_PRE_PMD),
- SND_SOC_DAPM_PGA("HP", SGTL5000_CHIP_ANA_POWER, 4, 0, NULL, 0),
+ SND_SOC_DAPM_PGA_E("HP", SGTL5000_CHIP_ANA_POWER, 4, 0, NULL, 0,
+ headphone_pga_event,
+ SND_SOC_DAPM_PRE_POST_PMU |
+ SND_SOC_DAPM_PRE_POST_PMD),
SND_SOC_DAPM_PGA("LO", SGTL5000_CHIP_ANA_POWER, 0, 0, NULL, 0),
SND_SOC_DAPM_MUX("Capture Mux", SND_SOC_NOPM, 0, 0, &adc_mux),
@@ -301,11 +454,12 @@ static const struct snd_soc_dapm_widget sgtl5000_dapm_widgets[] = {
0, SGTL5000_CHIP_DIG_POWER,
1, 0),
- SND_SOC_DAPM_ADC("ADC", "Capture", SGTL5000_CHIP_ANA_POWER, 1, 0),
- SND_SOC_DAPM_DAC("DAC", "Playback", SGTL5000_CHIP_ANA_POWER, 3, 0),
-
- SND_SOC_DAPM_PRE("VAG_POWER_PRE", power_vag_event),
- SND_SOC_DAPM_POST("VAG_POWER_POST", power_vag_event),
+ SND_SOC_DAPM_ADC_E("ADC", "Capture", SGTL5000_CHIP_ANA_POWER, 1, 0,
+ adc_updown_depop, SND_SOC_DAPM_PRE_POST_PMU |
+ SND_SOC_DAPM_PRE_POST_PMD),
+ SND_SOC_DAPM_DAC_E("DAC", "Playback", SGTL5000_CHIP_ANA_POWER, 3, 0,
+ dac_updown_depop, SND_SOC_DAPM_PRE_POST_PMU |
+ SND_SOC_DAPM_PRE_POST_PMD),
};
/* routes for sgtl5000 */
--
2.20.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v5 3/6] ASoC: sgtl5000: Fix definition of VAG Ramp Control
2019-07-18 9:02 [PATCH v5 0/6] VAG power control improvement for sgtl5000 codec Oleksandr Suvorov
2019-07-18 9:02 ` [PATCH v5 1/6] ASoC: Define a set of DAPM pre/post-up events Oleksandr Suvorov
2019-07-18 9:02 ` [PATCH v5 2/6] ASoC: sgtl5000: Improve VAG power and mute control Oleksandr Suvorov
@ 2019-07-18 9:02 ` Oleksandr Suvorov
2019-07-18 9:02 ` [PATCH v5 4/6] ASoC: sgtl5000: add ADC mute control Oleksandr Suvorov
` (2 subsequent siblings)
5 siblings, 0 replies; 13+ messages in thread
From: Oleksandr Suvorov @ 2019-07-18 9:02 UTC (permalink / raw
To: Fabio Estevam
Cc: linux-kernel@vger.kernel.org, Igor Opaniuk, Marcel Ziswiler,
alsa-devel@alsa-project.org, Oleksandr Suvorov, Jaroslav Kysela,
Mark Brown, Takashi Iwai, Liam Girdwood
SGTL5000_SMALL_POP is a bit mask, not a value. Usage of
correct definition makes device probing code more clear.
Signed-off-by: Oleksandr Suvorov <oleksandr.suvorov@toradex.com>
Reviewed-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>
Reviewed-by: Igor Opaniuk <igor.opaniuk@toradex.com>
Reviewed-by: Fabio Estevam <festevam@gmail.com>
---
Changes in v5: None
Changes in v4: None
Changes in v3: None
Changes in v2:
- Fix patch formatting
sound/soc/codecs/sgtl5000.c | 2 +-
sound/soc/codecs/sgtl5000.h | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/sound/soc/codecs/sgtl5000.c b/sound/soc/codecs/sgtl5000.c
index a2aeb5fb9b537..a8d55c21a5f2d 100644
--- a/sound/soc/codecs/sgtl5000.c
+++ b/sound/soc/codecs/sgtl5000.c
@@ -1450,7 +1450,7 @@ static int sgtl5000_probe(struct snd_soc_component *component)
/* enable small pop, introduce 400ms delay in turning off */
snd_soc_component_update_bits(component, SGTL5000_CHIP_REF_CTRL,
- SGTL5000_SMALL_POP, 1);
+ SGTL5000_SMALL_POP, SGTL5000_SMALL_POP);
/* disable short cut detector */
snd_soc_component_write(component, SGTL5000_CHIP_SHORT_CTRL, 0);
diff --git a/sound/soc/codecs/sgtl5000.h b/sound/soc/codecs/sgtl5000.h
index 18cae08bbd3a6..a4bf4bca95bf7 100644
--- a/sound/soc/codecs/sgtl5000.h
+++ b/sound/soc/codecs/sgtl5000.h
@@ -273,7 +273,7 @@
#define SGTL5000_BIAS_CTRL_MASK 0x000e
#define SGTL5000_BIAS_CTRL_SHIFT 1
#define SGTL5000_BIAS_CTRL_WIDTH 3
-#define SGTL5000_SMALL_POP 1
+#define SGTL5000_SMALL_POP 0x0001
/*
* SGTL5000_CHIP_MIC_CTRL
--
2.20.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v5 4/6] ASoC: sgtl5000: add ADC mute control
2019-07-18 9:02 [PATCH v5 0/6] VAG power control improvement for sgtl5000 codec Oleksandr Suvorov
` (2 preceding siblings ...)
2019-07-18 9:02 ` [PATCH v5 3/6] ASoC: sgtl5000: Fix definition of VAG Ramp Control Oleksandr Suvorov
@ 2019-07-18 9:02 ` Oleksandr Suvorov
2019-07-18 9:02 ` [PATCH v5 5/6] ASoC: sgtl5000: Fix of unmute outputs on probe Oleksandr Suvorov
2019-07-18 9:02 ` [PATCH v5 6/6] ASoC: sgtl5000: Fix charge pump source assignment Oleksandr Suvorov
5 siblings, 0 replies; 13+ messages in thread
From: Oleksandr Suvorov @ 2019-07-18 9:02 UTC (permalink / raw
To: Fabio Estevam
Cc: linux-kernel@vger.kernel.org, Igor Opaniuk, Marcel Ziswiler,
alsa-devel@alsa-project.org, Oleksandr Suvorov, Jaroslav Kysela,
Mark Brown, Takashi Iwai, Liam Girdwood
This control mute/unmute the ADC input of SGTL5000
using its CHIP_ANA_CTRL register.
Signed-off-by: Oleksandr Suvorov <oleksandr.suvorov@toradex.com>
Reviewed-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>
Reviewed-by: Igor Opaniuk <igor.opaniuk@toradex.com>
Reviewed-by: Fabio Estevam <festevam@gmail.com>
---
Changes in v5: None
Changes in v4: None
Changes in v3: None
Changes in v2:
- Fix patch formatting
sound/soc/codecs/sgtl5000.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/sound/soc/codecs/sgtl5000.c b/sound/soc/codecs/sgtl5000.c
index a8d55c21a5f2d..0123d9cfcb473 100644
--- a/sound/soc/codecs/sgtl5000.c
+++ b/sound/soc/codecs/sgtl5000.c
@@ -710,6 +710,7 @@ static const struct snd_kcontrol_new sgtl5000_snd_controls[] = {
SGTL5000_CHIP_ANA_ADC_CTRL,
8, 1, 0, capture_6db_attenuate),
SOC_SINGLE("Capture ZC Switch", SGTL5000_CHIP_ANA_CTRL, 1, 1, 0),
+ SOC_SINGLE("Capture Switch", SGTL5000_CHIP_ANA_CTRL, 0, 1, 1),
SOC_DOUBLE_TLV("Headphone Playback Volume",
SGTL5000_CHIP_ANA_HP_CTRL,
--
2.20.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v5 5/6] ASoC: sgtl5000: Fix of unmute outputs on probe
2019-07-18 9:02 [PATCH v5 0/6] VAG power control improvement for sgtl5000 codec Oleksandr Suvorov
` (3 preceding siblings ...)
2019-07-18 9:02 ` [PATCH v5 4/6] ASoC: sgtl5000: add ADC mute control Oleksandr Suvorov
@ 2019-07-18 9:02 ` Oleksandr Suvorov
2019-07-18 9:02 ` [PATCH v5 6/6] ASoC: sgtl5000: Fix charge pump source assignment Oleksandr Suvorov
5 siblings, 0 replies; 13+ messages in thread
From: Oleksandr Suvorov @ 2019-07-18 9:02 UTC (permalink / raw
To: Fabio Estevam
Cc: linux-kernel@vger.kernel.org, Igor Opaniuk, Marcel Ziswiler,
alsa-devel@alsa-project.org, Oleksandr Suvorov, Jaroslav Kysela,
Mark Brown, Takashi Iwai, Liam Girdwood
To enable "zero cross detect" for ADC/HP, change
HP_ZCD_EN/ADC_ZCD_EN bits only instead of writing the whole
CHIP_ANA_CTRL register.
Signed-off-by: Oleksandr Suvorov <oleksandr.suvorov@toradex.com>
Reviewed-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>
Reviewed-by: Igor Opaniuk <igor.opaniuk@toradex.com>
Reviewed-by: Fabio Estevam <festevam@gmail.com>
---
Changes in v5: None
Changes in v4: None
Changes in v3: None
Changes in v2:
- Fix patch formatting
sound/soc/codecs/sgtl5000.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/sound/soc/codecs/sgtl5000.c b/sound/soc/codecs/sgtl5000.c
index 0123d9cfcb473..31d546abde717 100644
--- a/sound/soc/codecs/sgtl5000.c
+++ b/sound/soc/codecs/sgtl5000.c
@@ -1443,6 +1443,7 @@ static int sgtl5000_probe(struct snd_soc_component *component)
int ret;
u16 reg;
struct sgtl5000_priv *sgtl5000 = snd_soc_component_get_drvdata(component);
+ unsigned int zcd_mask = SGTL5000_HP_ZCD_EN | SGTL5000_ADC_ZCD_EN;
/* power up sgtl5000 */
ret = sgtl5000_set_power_regs(component);
@@ -1470,9 +1471,8 @@ static int sgtl5000_probe(struct snd_soc_component *component)
0x1f);
snd_soc_component_write(component, SGTL5000_CHIP_PAD_STRENGTH, reg);
- snd_soc_component_write(component, SGTL5000_CHIP_ANA_CTRL,
- SGTL5000_HP_ZCD_EN |
- SGTL5000_ADC_ZCD_EN);
+ snd_soc_component_update_bits(component, SGTL5000_CHIP_ANA_CTRL,
+ zcd_mask, zcd_mask);
snd_soc_component_update_bits(component, SGTL5000_CHIP_MIC_CTRL,
SGTL5000_BIAS_R_MASK,
--
2.20.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v5 6/6] ASoC: sgtl5000: Fix charge pump source assignment
2019-07-18 9:02 [PATCH v5 0/6] VAG power control improvement for sgtl5000 codec Oleksandr Suvorov
` (4 preceding siblings ...)
2019-07-18 9:02 ` [PATCH v5 5/6] ASoC: sgtl5000: Fix of unmute outputs on probe Oleksandr Suvorov
@ 2019-07-18 9:02 ` Oleksandr Suvorov
5 siblings, 0 replies; 13+ messages in thread
From: Oleksandr Suvorov @ 2019-07-18 9:02 UTC (permalink / raw
To: Fabio Estevam
Cc: linux-kernel@vger.kernel.org, Igor Opaniuk, Marcel Ziswiler,
alsa-devel@alsa-project.org, Oleksandr Suvorov, Jaroslav Kysela,
Mark Brown, Takashi Iwai, Liam Girdwood
If VDDA != VDDIO and any of them is greater than 3.1V, charge pump
source can be assigned automatically [1].
[1] https://www.nxp.com/docs/en/data-sheet/SGTL5000.pdf
Signed-off-by: Oleksandr Suvorov <oleksandr.suvorov@toradex.com>
Reviewed-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>
Reviewed-by: Igor Opaniuk <igor.opaniuk@toradex.com>
Reviewed-by: Fabio Estevam <festevam@gmail.com>
---
Changes in v5: None
Changes in v4: None
Changes in v3:
- Add the reference to NXP SGTL5000 data sheet to commit message
- Fix multi-line comment format
Changes in v2:
- Fix patch formatting
sound/soc/codecs/sgtl5000.c | 15 ++++++++++-----
1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/sound/soc/codecs/sgtl5000.c b/sound/soc/codecs/sgtl5000.c
index 31d546abde717..a04cba66615de 100644
--- a/sound/soc/codecs/sgtl5000.c
+++ b/sound/soc/codecs/sgtl5000.c
@@ -1328,12 +1328,17 @@ static int sgtl5000_set_power_regs(struct snd_soc_component *component)
SGTL5000_INT_OSC_EN);
/* Enable VDDC charge pump */
ana_pwr |= SGTL5000_VDDC_CHRGPMP_POWERUP;
- } else if (vddio >= 3100 && vdda >= 3100) {
+ } else {
ana_pwr &= ~SGTL5000_VDDC_CHRGPMP_POWERUP;
- /* VDDC use VDDIO rail */
- lreg_ctrl |= SGTL5000_VDDC_ASSN_OVRD;
- lreg_ctrl |= SGTL5000_VDDC_MAN_ASSN_VDDIO <<
- SGTL5000_VDDC_MAN_ASSN_SHIFT;
+ /*
+ * if vddio == vdda the source of charge pump should be
+ * assigned manually to VDDIO
+ */
+ if (vddio == vdda) {
+ lreg_ctrl |= SGTL5000_VDDC_ASSN_OVRD;
+ lreg_ctrl |= SGTL5000_VDDC_MAN_ASSN_VDDIO <<
+ SGTL5000_VDDC_MAN_ASSN_SHIFT;
+ }
}
snd_soc_component_write(component, SGTL5000_CHIP_LINREG_CTRL, lreg_ctrl);
--
2.20.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v5 2/6] ASoC: sgtl5000: Improve VAG power and mute control
2019-07-18 9:02 ` [PATCH v5 2/6] ASoC: sgtl5000: Improve VAG power and mute control Oleksandr Suvorov
@ 2019-07-18 16:25 ` Igor Opaniuk
2019-07-18 18:42 ` Cezary Rojewski
1 sibling, 0 replies; 13+ messages in thread
From: Igor Opaniuk @ 2019-07-18 16:18 UTC (permalink / raw
To: Oleksandr Suvorov
Cc: Sasha Levin, Igor Opaniuk, alsa-devel@alsa-project.org,
Liam Girdwood, Marcel Ziswiler, Takashi Iwai,
linux-kernel@vger.kernel.org, stable@vger.kernel.org, Mark Brown,
Fabio Estevam
On Thu, Jul 18, 2019 at 12:03 PM Oleksandr Suvorov
<oleksandr.suvorov@toradex.com> wrote:
>
> VAG power control is improved to fit the manual [1]. This patch fixes as
> minimum one bug: if customer muxes Headphone to Line-In right after boot,
> the VAG power remains off that leads to poor sound quality from line-in.
>
> I.e. after boot:
> - Connect sound source to Line-In jack;
> - Connect headphone to HP jack;
> - Run following commands:
> $ amixer set 'Headphone' 80%
> $ amixer set 'Headphone Mux' LINE_IN
>
> Change VAG power on/off control according to the following algorithm:
> - turn VAG power ON on the 1st incoming event.
> - keep it ON if there is any active VAG consumer (ADC/DAC/HP/Line-In).
> - turn VAG power OFF when there is the latest consumer's pre-down event
> come.
> - always delay after VAG power OFF to avoid pop.
> - delay after VAG power ON if the initiative consumer is Line-In, this
> prevents pop during line-in muxing.
>
> According to the data sheet [1], to avoid any pops/clicks,
> the outputs should be muted during input/output
> routing changes.
>
> [1] https://www.nxp.com/docs/en/data-sheet/SGTL5000.pdf
>
> Cc: stable@vger.kernel.org
> Fixes: 9b34e6cc3bc2 ("ASoC: Add Freescale SGTL5000 codec support")
> Signed-off-by: Oleksandr Suvorov <oleksandr.suvorov@toradex.com>
> Reviewed-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>
> Reviewed-by: Fabio Estevam <festevam@gmail.com>
>
> ---
>
> Changes in v5:
> - Improve commit message
> - Add explicit stable tag
>
> Changes in v4:
> - Code optimization, simplify function signature
> (thanks to Cezary Rojewski <cezary.rojewski@intel.com> for an idea)
> - CC the patch to kernel-stable
> - Add a Fixes tag
>
> Changes in v3:
> - Add the reference to NXP SGTL5000 data sheet to commit message
>
> Changes in v2:
> - Fix patch formatting
>
> sound/soc/codecs/sgtl5000.c | 216 ++++++++++++++++++++++++++++++------
> 1 file changed, 185 insertions(+), 31 deletions(-)
>
> diff --git a/sound/soc/codecs/sgtl5000.c b/sound/soc/codecs/sgtl5000.c
> index a6a4748c97f9d..a2aeb5fb9b537 100644
> --- a/sound/soc/codecs/sgtl5000.c
> +++ b/sound/soc/codecs/sgtl5000.c
> @@ -31,6 +31,13 @@
> #define SGTL5000_DAP_REG_OFFSET 0x0100
> #define SGTL5000_MAX_REG_OFFSET 0x013A
>
> +/* Delay for the VAG ramp up */
> +#define SGTL5000_VAG_POWERUP_DELAY 500 /* ms */
> +/* Delay for the VAG ramp down */
> +#define SGTL5000_VAG_POWERDOWN_DELAY 500 /* ms */
> +
> +#define SGTL5000_OUTPUTS_MUTE (SGTL5000_HP_MUTE | SGTL5000_LINE_OUT_MUTE)
> +
> /* default value of sgtl5000 registers */
> static const struct reg_default sgtl5000_reg_defaults[] = {
> { SGTL5000_CHIP_DIG_POWER, 0x0000 },
> @@ -123,6 +130,19 @@ enum {
> I2S_SCLK_STRENGTH_HIGH,
> };
>
> +enum {
> + HP_POWER_EVENT,
> + DAC_POWER_EVENT,
> + ADC_POWER_EVENT,
> + LAST_POWER_EVENT
> +};
> +
> +static u16 mute_mask[] = {
> + SGTL5000_HP_MUTE,
> + SGTL5000_OUTPUTS_MUTE,
> + SGTL5000_OUTPUTS_MUTE
> +};
> +
> /* sgtl5000 private structure in codec */
> struct sgtl5000_priv {
> int sysclk; /* sysclk rate */
> @@ -137,8 +157,109 @@ struct sgtl5000_priv {
> u8 micbias_voltage;
> u8 lrclk_strength;
> u8 sclk_strength;
> + u16 mute_state[LAST_POWER_EVENT];
> };
>
> +static inline int hp_sel_input(struct snd_soc_component *component)
> +{
> + return (snd_soc_component_read32(component, SGTL5000_CHIP_ANA_CTRL) &
> + SGTL5000_HP_SEL_MASK) >> SGTL5000_HP_SEL_SHIFT;
> +}
> +
> +static inline u16 mute_output(struct snd_soc_component *component,
> + u16 mute_mask)
> +{
> + u16 mute_reg = snd_soc_component_read32(component,
> + SGTL5000_CHIP_ANA_CTRL);
> +
> + snd_soc_component_update_bits(component, SGTL5000_CHIP_ANA_CTRL,
> + mute_mask, mute_mask);
> + return mute_reg;
> +}
> +
> +static inline void restore_output(struct snd_soc_component *component,
> + u16 mute_mask, u16 mute_reg)
> +{
> + snd_soc_component_update_bits(component, SGTL5000_CHIP_ANA_CTRL,
> + mute_mask, mute_reg);
> +}
> +
> +static void vag_power_on(struct snd_soc_component *component, u32 source)
> +{
> + if (snd_soc_component_read32(component, SGTL5000_CHIP_ANA_POWER) &
> + SGTL5000_VAG_POWERUP)
> + return;
> +
> + snd_soc_component_update_bits(component, SGTL5000_CHIP_ANA_POWER,
> + SGTL5000_VAG_POWERUP, SGTL5000_VAG_POWERUP);
> +
> + /* When VAG powering on to get local loop from Line-In, the sleep
> + * is required to avoid loud pop.
> + */
> + if (hp_sel_input(component) == SGTL5000_HP_SEL_LINE_IN &&
> + source == HP_POWER_EVENT)
> + msleep(SGTL5000_VAG_POWERUP_DELAY);
> +}
> +
> +static int vag_power_consumers(struct snd_soc_component *component,
> + u16 ana_pwr_reg, u32 source)
> +{
> + int consumers = 0;
> +
> + /* count dac/adc consumers unconditional */
> + if (ana_pwr_reg & SGTL5000_DAC_POWERUP)
> + consumers++;
> + if (ana_pwr_reg & SGTL5000_ADC_POWERUP)
> + consumers++;
> +
> + /*
> + * If the event comes from HP and Line-In is selected,
> + * current action is 'DAC to be powered down'.
> + * As HP_POWERUP is not set when HP muxed to line-in,
> + * we need to keep VAG power ON.
> + */
> + if (source == HP_POWER_EVENT) {
> + if (hp_sel_input(component) == SGTL5000_HP_SEL_LINE_IN)
> + consumers++;
> + } else {
> + if (ana_pwr_reg & SGTL5000_HP_POWERUP)
> + consumers++;
> + }
> +
> + return consumers;
> +}
> +
> +static void vag_power_off(struct snd_soc_component *component, u32 source)
> +{
> + u16 ana_pwr = snd_soc_component_read32(component,
> + SGTL5000_CHIP_ANA_POWER);
> +
> + if (!(ana_pwr & SGTL5000_VAG_POWERUP))
> + return;
> +
> + /*
> + * This function calls when any of VAG power consumers is disappearing.
> + * Thus, if there is more than one consumer at the moment, as minimum
> + * one consumer will definitely stay after the end of the current
> + * event.
> + * Don't clear VAG_POWERUP if 2 or more consumers of VAG present:
> + * - LINE_IN (for HP events) / HP (for DAC/ADC events)
> + * - DAC
> + * - ADC
> + * (the current consumer is disappearing right now)
> + */
> + if (vag_power_consumers(component, ana_pwr, source) >= 2)
> + return;
> +
> + snd_soc_component_update_bits(component, SGTL5000_CHIP_ANA_POWER,
> + SGTL5000_VAG_POWERUP, 0);
> + /* In power down case, we need wait 400-1000 ms
> + * when VAG fully ramped down.
> + * As longer we wait, as smaller pop we've got.
> + */
> + msleep(SGTL5000_VAG_POWERDOWN_DELAY);
> +}
> +
> /*
> * mic_bias power on/off share the same register bits with
> * output impedance of mic bias, when power on mic bias, we
> @@ -170,36 +291,30 @@ static int mic_bias_event(struct snd_soc_dapm_widget *w,
> return 0;
> }
>
> -/*
> - * As manual described, ADC/DAC only works when VAG powerup,
> - * So enabled VAG before ADC/DAC up.
> - * In power down case, we need wait 400ms when vag fully ramped down.
> - */
> -static int power_vag_event(struct snd_soc_dapm_widget *w,
> - struct snd_kcontrol *kcontrol, int event)
> +static int vag_and_mute_control(struct snd_soc_component *component,
> + int event, int event_source)
> {
> - struct snd_soc_component *component = snd_soc_dapm_to_component(w->dapm);
> - const u32 mask = SGTL5000_DAC_POWERUP | SGTL5000_ADC_POWERUP;
> + struct sgtl5000_priv *sgtl5000 =
> + snd_soc_component_get_drvdata(component);
>
> switch (event) {
> - case SND_SOC_DAPM_POST_PMU:
> - snd_soc_component_update_bits(component, SGTL5000_CHIP_ANA_POWER,
> - SGTL5000_VAG_POWERUP, SGTL5000_VAG_POWERUP);
> - msleep(400);
> + case SND_SOC_DAPM_PRE_PMU:
> + sgtl5000->mute_state[event_source] =
> + mute_output(component, mute_mask[event_source]);
> + break;
> + case SND_SOC_DAPM_POST_PMU:
> + vag_power_on(component, event_source);
> + restore_output(component, mute_mask[event_source],
> + sgtl5000->mute_state[event_source]);
> break;
> -
> case SND_SOC_DAPM_PRE_PMD:
> - /*
> - * Don't clear VAG_POWERUP, when both DAC and ADC are
> - * operational to prevent inadvertently starving the
> - * other one of them.
> - */
> - if ((snd_soc_component_read32(component, SGTL5000_CHIP_ANA_POWER) &
> - mask) != mask) {
> - snd_soc_component_update_bits(component, SGTL5000_CHIP_ANA_POWER,
> - SGTL5000_VAG_POWERUP, 0);
> - msleep(400);
> - }
> + sgtl5000->mute_state[event_source] =
> + mute_output(component, mute_mask[event_source]);
> + vag_power_off(component, event_source);
> + break;
> + case SND_SOC_DAPM_POST_PMD:
> + restore_output(component, mute_mask[event_source],
> + sgtl5000->mute_state[event_source]);
> break;
> default:
> break;
> @@ -208,6 +323,41 @@ static int power_vag_event(struct snd_soc_dapm_widget *w,
> return 0;
> }
>
> +/*
> + * Mute Headphone when power it up/down.
> + * Control VAG power on HP power path.
> + */
> +static int headphone_pga_event(struct snd_soc_dapm_widget *w,
> + struct snd_kcontrol *kcontrol, int event)
> +{
> + struct snd_soc_component *component =
> + snd_soc_dapm_to_component(w->dapm);
> +
> + return vag_and_mute_control(component, event, HP_POWER_EVENT);
> +}
> +
> +/* As manual describes, ADC/DAC powering up/down requires
> + * to mute outputs to avoid pops.
> + * Control VAG power on ADC/DAC power path.
> + */
> +static int adc_updown_depop(struct snd_soc_dapm_widget *w,
> + struct snd_kcontrol *kcontrol, int event)
> +{
> + struct snd_soc_component *component =
> + snd_soc_dapm_to_component(w->dapm);
> +
> + return vag_and_mute_control(component, event, ADC_POWER_EVENT);
> +}
> +
> +static int dac_updown_depop(struct snd_soc_dapm_widget *w,
> + struct snd_kcontrol *kcontrol, int event)
> +{
> + struct snd_soc_component *component =
> + snd_soc_dapm_to_component(w->dapm);
> +
> + return vag_and_mute_control(component, event, DAC_POWER_EVENT);
> +}
> +
> /* input sources for ADC */
> static const char *adc_mux_text[] = {
> "MIC_IN", "LINE_IN"
> @@ -280,7 +430,10 @@ static const struct snd_soc_dapm_widget sgtl5000_dapm_widgets[] = {
> mic_bias_event,
> SND_SOC_DAPM_POST_PMU | SND_SOC_DAPM_PRE_PMD),
>
> - SND_SOC_DAPM_PGA("HP", SGTL5000_CHIP_ANA_POWER, 4, 0, NULL, 0),
> + SND_SOC_DAPM_PGA_E("HP", SGTL5000_CHIP_ANA_POWER, 4, 0, NULL, 0,
> + headphone_pga_event,
> + SND_SOC_DAPM_PRE_POST_PMU |
> + SND_SOC_DAPM_PRE_POST_PMD),
> SND_SOC_DAPM_PGA("LO", SGTL5000_CHIP_ANA_POWER, 0, 0, NULL, 0),
>
> SND_SOC_DAPM_MUX("Capture Mux", SND_SOC_NOPM, 0, 0, &adc_mux),
> @@ -301,11 +454,12 @@ static const struct snd_soc_dapm_widget sgtl5000_dapm_widgets[] = {
> 0, SGTL5000_CHIP_DIG_POWER,
> 1, 0),
>
> - SND_SOC_DAPM_ADC("ADC", "Capture", SGTL5000_CHIP_ANA_POWER, 1, 0),
> - SND_SOC_DAPM_DAC("DAC", "Playback", SGTL5000_CHIP_ANA_POWER, 3, 0),
> -
> - SND_SOC_DAPM_PRE("VAG_POWER_PRE", power_vag_event),
> - SND_SOC_DAPM_POST("VAG_POWER_POST", power_vag_event),
> + SND_SOC_DAPM_ADC_E("ADC", "Capture", SGTL5000_CHIP_ANA_POWER, 1, 0,
> + adc_updown_depop, SND_SOC_DAPM_PRE_POST_PMU |
> + SND_SOC_DAPM_PRE_POST_PMD),
> + SND_SOC_DAPM_DAC_E("DAC", "Playback", SGTL5000_CHIP_ANA_POWER, 3, 0,
> + dac_updown_depop, SND_SOC_DAPM_PRE_POST_PMU |
> + SND_SOC_DAPM_PRE_POST_PMD),
> };
>
> /* routes for sgtl5000 */
> --
> 2.20.1
>
Reviewed-by: Igor Opaniuk <igor.opaniuk@toradex.com>
--
Best regards - Freundliche Grüsse - Meilleures salutations
Senior Development Engineer,
Igor Opaniuk
Toradex AG
Altsagenstrasse 5 | 6048 Horw/Luzern | Switzerland | T: +41 41 500 48
00 (main line)
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v5 2/6] ASoC: sgtl5000: Improve VAG power and mute control
@ 2019-07-18 16:25 ` Igor Opaniuk
0 siblings, 0 replies; 13+ messages in thread
From: Igor Opaniuk @ 2019-07-18 16:25 UTC (permalink / raw
To: Oleksandr Suvorov
Cc: Fabio Estevam, linux-kernel@vger.kernel.org, Igor Opaniuk,
Marcel Ziswiler, alsa-devel@alsa-project.org,
stable@vger.kernel.org, Jaroslav Kysela, Sasha Levin, Mark Brown,
Takashi Iwai, Liam Girdwood
On Thu, Jul 18, 2019 at 12:03 PM Oleksandr Suvorov
<oleksandr.suvorov@toradex.com> wrote:
>
> VAG power control is improved to fit the manual [1]. This patch fixes as
> minimum one bug: if customer muxes Headphone to Line-In right after boot,
> the VAG power remains off that leads to poor sound quality from line-in.
>
> I.e. after boot:
> - Connect sound source to Line-In jack;
> - Connect headphone to HP jack;
> - Run following commands:
> $ amixer set 'Headphone' 80%
> $ amixer set 'Headphone Mux' LINE_IN
>
> Change VAG power on/off control according to the following algorithm:
> - turn VAG power ON on the 1st incoming event.
> - keep it ON if there is any active VAG consumer (ADC/DAC/HP/Line-In).
> - turn VAG power OFF when there is the latest consumer's pre-down event
> come.
> - always delay after VAG power OFF to avoid pop.
> - delay after VAG power ON if the initiative consumer is Line-In, this
> prevents pop during line-in muxing.
>
> According to the data sheet [1], to avoid any pops/clicks,
> the outputs should be muted during input/output
> routing changes.
>
> [1] https://www.nxp.com/docs/en/data-sheet/SGTL5000.pdf
>
> Cc: stable@vger.kernel.org
> Fixes: 9b34e6cc3bc2 ("ASoC: Add Freescale SGTL5000 codec support")
> Signed-off-by: Oleksandr Suvorov <oleksandr.suvorov@toradex.com>
> Reviewed-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>
> Reviewed-by: Fabio Estevam <festevam@gmail.com>
>
> ---
>
> Changes in v5:
> - Improve commit message
> - Add explicit stable tag
>
> Changes in v4:
> - Code optimization, simplify function signature
> (thanks to Cezary Rojewski <cezary.rojewski@intel.com> for an idea)
> - CC the patch to kernel-stable
> - Add a Fixes tag
>
> Changes in v3:
> - Add the reference to NXP SGTL5000 data sheet to commit message
>
> Changes in v2:
> - Fix patch formatting
>
> sound/soc/codecs/sgtl5000.c | 216 ++++++++++++++++++++++++++++++------
> 1 file changed, 185 insertions(+), 31 deletions(-)
>
> diff --git a/sound/soc/codecs/sgtl5000.c b/sound/soc/codecs/sgtl5000.c
> index a6a4748c97f9d..a2aeb5fb9b537 100644
> --- a/sound/soc/codecs/sgtl5000.c
> +++ b/sound/soc/codecs/sgtl5000.c
> @@ -31,6 +31,13 @@
> #define SGTL5000_DAP_REG_OFFSET 0x0100
> #define SGTL5000_MAX_REG_OFFSET 0x013A
>
> +/* Delay for the VAG ramp up */
> +#define SGTL5000_VAG_POWERUP_DELAY 500 /* ms */
> +/* Delay for the VAG ramp down */
> +#define SGTL5000_VAG_POWERDOWN_DELAY 500 /* ms */
> +
> +#define SGTL5000_OUTPUTS_MUTE (SGTL5000_HP_MUTE | SGTL5000_LINE_OUT_MUTE)
> +
> /* default value of sgtl5000 registers */
> static const struct reg_default sgtl5000_reg_defaults[] = {
> { SGTL5000_CHIP_DIG_POWER, 0x0000 },
> @@ -123,6 +130,19 @@ enum {
> I2S_SCLK_STRENGTH_HIGH,
> };
>
> +enum {
> + HP_POWER_EVENT,
> + DAC_POWER_EVENT,
> + ADC_POWER_EVENT,
> + LAST_POWER_EVENT
> +};
> +
> +static u16 mute_mask[] = {
> + SGTL5000_HP_MUTE,
> + SGTL5000_OUTPUTS_MUTE,
> + SGTL5000_OUTPUTS_MUTE
> +};
> +
> /* sgtl5000 private structure in codec */
> struct sgtl5000_priv {
> int sysclk; /* sysclk rate */
> @@ -137,8 +157,109 @@ struct sgtl5000_priv {
> u8 micbias_voltage;
> u8 lrclk_strength;
> u8 sclk_strength;
> + u16 mute_state[LAST_POWER_EVENT];
> };
>
> +static inline int hp_sel_input(struct snd_soc_component *component)
> +{
> + return (snd_soc_component_read32(component, SGTL5000_CHIP_ANA_CTRL) &
> + SGTL5000_HP_SEL_MASK) >> SGTL5000_HP_SEL_SHIFT;
> +}
> +
> +static inline u16 mute_output(struct snd_soc_component *component,
> + u16 mute_mask)
> +{
> + u16 mute_reg = snd_soc_component_read32(component,
> + SGTL5000_CHIP_ANA_CTRL);
> +
> + snd_soc_component_update_bits(component, SGTL5000_CHIP_ANA_CTRL,
> + mute_mask, mute_mask);
> + return mute_reg;
> +}
> +
> +static inline void restore_output(struct snd_soc_component *component,
> + u16 mute_mask, u16 mute_reg)
> +{
> + snd_soc_component_update_bits(component, SGTL5000_CHIP_ANA_CTRL,
> + mute_mask, mute_reg);
> +}
> +
> +static void vag_power_on(struct snd_soc_component *component, u32 source)
> +{
> + if (snd_soc_component_read32(component, SGTL5000_CHIP_ANA_POWER) &
> + SGTL5000_VAG_POWERUP)
> + return;
> +
> + snd_soc_component_update_bits(component, SGTL5000_CHIP_ANA_POWER,
> + SGTL5000_VAG_POWERUP, SGTL5000_VAG_POWERUP);
> +
> + /* When VAG powering on to get local loop from Line-In, the sleep
> + * is required to avoid loud pop.
> + */
> + if (hp_sel_input(component) == SGTL5000_HP_SEL_LINE_IN &&
> + source == HP_POWER_EVENT)
> + msleep(SGTL5000_VAG_POWERUP_DELAY);
> +}
> +
> +static int vag_power_consumers(struct snd_soc_component *component,
> + u16 ana_pwr_reg, u32 source)
> +{
> + int consumers = 0;
> +
> + /* count dac/adc consumers unconditional */
> + if (ana_pwr_reg & SGTL5000_DAC_POWERUP)
> + consumers++;
> + if (ana_pwr_reg & SGTL5000_ADC_POWERUP)
> + consumers++;
> +
> + /*
> + * If the event comes from HP and Line-In is selected,
> + * current action is 'DAC to be powered down'.
> + * As HP_POWERUP is not set when HP muxed to line-in,
> + * we need to keep VAG power ON.
> + */
> + if (source == HP_POWER_EVENT) {
> + if (hp_sel_input(component) == SGTL5000_HP_SEL_LINE_IN)
> + consumers++;
> + } else {
> + if (ana_pwr_reg & SGTL5000_HP_POWERUP)
> + consumers++;
> + }
> +
> + return consumers;
> +}
> +
> +static void vag_power_off(struct snd_soc_component *component, u32 source)
> +{
> + u16 ana_pwr = snd_soc_component_read32(component,
> + SGTL5000_CHIP_ANA_POWER);
> +
> + if (!(ana_pwr & SGTL5000_VAG_POWERUP))
> + return;
> +
> + /*
> + * This function calls when any of VAG power consumers is disappearing.
> + * Thus, if there is more than one consumer at the moment, as minimum
> + * one consumer will definitely stay after the end of the current
> + * event.
> + * Don't clear VAG_POWERUP if 2 or more consumers of VAG present:
> + * - LINE_IN (for HP events) / HP (for DAC/ADC events)
> + * - DAC
> + * - ADC
> + * (the current consumer is disappearing right now)
> + */
> + if (vag_power_consumers(component, ana_pwr, source) >= 2)
> + return;
> +
> + snd_soc_component_update_bits(component, SGTL5000_CHIP_ANA_POWER,
> + SGTL5000_VAG_POWERUP, 0);
> + /* In power down case, we need wait 400-1000 ms
> + * when VAG fully ramped down.
> + * As longer we wait, as smaller pop we've got.
> + */
> + msleep(SGTL5000_VAG_POWERDOWN_DELAY);
> +}
> +
> /*
> * mic_bias power on/off share the same register bits with
> * output impedance of mic bias, when power on mic bias, we
> @@ -170,36 +291,30 @@ static int mic_bias_event(struct snd_soc_dapm_widget *w,
> return 0;
> }
>
> -/*
> - * As manual described, ADC/DAC only works when VAG powerup,
> - * So enabled VAG before ADC/DAC up.
> - * In power down case, we need wait 400ms when vag fully ramped down.
> - */
> -static int power_vag_event(struct snd_soc_dapm_widget *w,
> - struct snd_kcontrol *kcontrol, int event)
> +static int vag_and_mute_control(struct snd_soc_component *component,
> + int event, int event_source)
> {
> - struct snd_soc_component *component = snd_soc_dapm_to_component(w->dapm);
> - const u32 mask = SGTL5000_DAC_POWERUP | SGTL5000_ADC_POWERUP;
> + struct sgtl5000_priv *sgtl5000 =
> + snd_soc_component_get_drvdata(component);
>
> switch (event) {
> - case SND_SOC_DAPM_POST_PMU:
> - snd_soc_component_update_bits(component, SGTL5000_CHIP_ANA_POWER,
> - SGTL5000_VAG_POWERUP, SGTL5000_VAG_POWERUP);
> - msleep(400);
> + case SND_SOC_DAPM_PRE_PMU:
> + sgtl5000->mute_state[event_source] =
> + mute_output(component, mute_mask[event_source]);
> + break;
> + case SND_SOC_DAPM_POST_PMU:
> + vag_power_on(component, event_source);
> + restore_output(component, mute_mask[event_source],
> + sgtl5000->mute_state[event_source]);
> break;
> -
> case SND_SOC_DAPM_PRE_PMD:
> - /*
> - * Don't clear VAG_POWERUP, when both DAC and ADC are
> - * operational to prevent inadvertently starving the
> - * other one of them.
> - */
> - if ((snd_soc_component_read32(component, SGTL5000_CHIP_ANA_POWER) &
> - mask) != mask) {
> - snd_soc_component_update_bits(component, SGTL5000_CHIP_ANA_POWER,
> - SGTL5000_VAG_POWERUP, 0);
> - msleep(400);
> - }
> + sgtl5000->mute_state[event_source] =
> + mute_output(component, mute_mask[event_source]);
> + vag_power_off(component, event_source);
> + break;
> + case SND_SOC_DAPM_POST_PMD:
> + restore_output(component, mute_mask[event_source],
> + sgtl5000->mute_state[event_source]);
> break;
> default:
> break;
> @@ -208,6 +323,41 @@ static int power_vag_event(struct snd_soc_dapm_widget *w,
> return 0;
> }
>
> +/*
> + * Mute Headphone when power it up/down.
> + * Control VAG power on HP power path.
> + */
> +static int headphone_pga_event(struct snd_soc_dapm_widget *w,
> + struct snd_kcontrol *kcontrol, int event)
> +{
> + struct snd_soc_component *component =
> + snd_soc_dapm_to_component(w->dapm);
> +
> + return vag_and_mute_control(component, event, HP_POWER_EVENT);
> +}
> +
> +/* As manual describes, ADC/DAC powering up/down requires
> + * to mute outputs to avoid pops.
> + * Control VAG power on ADC/DAC power path.
> + */
> +static int adc_updown_depop(struct snd_soc_dapm_widget *w,
> + struct snd_kcontrol *kcontrol, int event)
> +{
> + struct snd_soc_component *component =
> + snd_soc_dapm_to_component(w->dapm);
> +
> + return vag_and_mute_control(component, event, ADC_POWER_EVENT);
> +}
> +
> +static int dac_updown_depop(struct snd_soc_dapm_widget *w,
> + struct snd_kcontrol *kcontrol, int event)
> +{
> + struct snd_soc_component *component =
> + snd_soc_dapm_to_component(w->dapm);
> +
> + return vag_and_mute_control(component, event, DAC_POWER_EVENT);
> +}
> +
> /* input sources for ADC */
> static const char *adc_mux_text[] = {
> "MIC_IN", "LINE_IN"
> @@ -280,7 +430,10 @@ static const struct snd_soc_dapm_widget sgtl5000_dapm_widgets[] = {
> mic_bias_event,
> SND_SOC_DAPM_POST_PMU | SND_SOC_DAPM_PRE_PMD),
>
> - SND_SOC_DAPM_PGA("HP", SGTL5000_CHIP_ANA_POWER, 4, 0, NULL, 0),
> + SND_SOC_DAPM_PGA_E("HP", SGTL5000_CHIP_ANA_POWER, 4, 0, NULL, 0,
> + headphone_pga_event,
> + SND_SOC_DAPM_PRE_POST_PMU |
> + SND_SOC_DAPM_PRE_POST_PMD),
> SND_SOC_DAPM_PGA("LO", SGTL5000_CHIP_ANA_POWER, 0, 0, NULL, 0),
>
> SND_SOC_DAPM_MUX("Capture Mux", SND_SOC_NOPM, 0, 0, &adc_mux),
> @@ -301,11 +454,12 @@ static const struct snd_soc_dapm_widget sgtl5000_dapm_widgets[] = {
> 0, SGTL5000_CHIP_DIG_POWER,
> 1, 0),
>
> - SND_SOC_DAPM_ADC("ADC", "Capture", SGTL5000_CHIP_ANA_POWER, 1, 0),
> - SND_SOC_DAPM_DAC("DAC", "Playback", SGTL5000_CHIP_ANA_POWER, 3, 0),
> -
> - SND_SOC_DAPM_PRE("VAG_POWER_PRE", power_vag_event),
> - SND_SOC_DAPM_POST("VAG_POWER_POST", power_vag_event),
> + SND_SOC_DAPM_ADC_E("ADC", "Capture", SGTL5000_CHIP_ANA_POWER, 1, 0,
> + adc_updown_depop, SND_SOC_DAPM_PRE_POST_PMU |
> + SND_SOC_DAPM_PRE_POST_PMD),
> + SND_SOC_DAPM_DAC_E("DAC", "Playback", SGTL5000_CHIP_ANA_POWER, 3, 0,
> + dac_updown_depop, SND_SOC_DAPM_PRE_POST_PMU |
> + SND_SOC_DAPM_PRE_POST_PMD),
> };
>
> /* routes for sgtl5000 */
> --
> 2.20.1
>
Reviewed-by: Igor Opaniuk <igor.opaniuk@toradex.com>
--
Best regards - Freundliche Grüsse - Meilleures salutations
Senior Development Engineer,
Igor Opaniuk
Toradex AG
Altsagenstrasse 5 | 6048 Horw/Luzern | Switzerland | T: +41 41 500 48
00 (main line)
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v5 2/6] ASoC: sgtl5000: Improve VAG power and mute control
2019-07-18 9:02 ` [PATCH v5 2/6] ASoC: sgtl5000: Improve VAG power and mute control Oleksandr Suvorov
2019-07-18 16:25 ` Igor Opaniuk
@ 2019-07-18 18:42 ` Cezary Rojewski
2019-07-18 18:48 ` Cezary Rojewski
1 sibling, 1 reply; 13+ messages in thread
From: Cezary Rojewski @ 2019-07-18 18:42 UTC (permalink / raw
To: Oleksandr Suvorov
Cc: Fabio Estevam, linux-kernel@vger.kernel.org, Igor Opaniuk,
Marcel Ziswiler, alsa-devel@alsa-project.org,
stable@vger.kernel.org, Jaroslav Kysela, Sasha Levin, Mark Brown,
Takashi Iwai, Liam Girdwood
On 2019-07-18 11:02, Oleksandr Suvorov wrote:
>
> +enum {
> + HP_POWER_EVENT,
> + DAC_POWER_EVENT,
> + ADC_POWER_EVENT,
> + LAST_POWER_EVENT
> +};
> +
> +static u16 mute_mask[] = {
> + SGTL5000_HP_MUTE,
> + SGTL5000_OUTPUTS_MUTE,
> + SGTL5000_OUTPUTS_MUTE
> +};
If mute_mask[] is only used within common handler, you may consider
declaring const array within said handler instead (did not check that
myself).
Otherwise, simple comment for the second _OUTPUTS_MUTE should suffice -
its not self explanatory why you doubled that mask.
> +
> /* sgtl5000 private structure in codec */
> struct sgtl5000_priv {
> int sysclk; /* sysclk rate */
> @@ -137,8 +157,109 @@ struct sgtl5000_priv {
> u8 micbias_voltage;
> u8 lrclk_strength;
> u8 sclk_strength;
> + u16 mute_state[LAST_POWER_EVENT];
> };
>
When I spoke of LAST enum constant, I did not really had this specific
usage in mind.
From design perspective, _LAST_ does not exist and should never be
referred to as "the next option" i.e.: new enum constant.
That is way preferred usage is:
u16 mute_state[ADC_POWER_EVENT+1;
-or-
u16 mute_state[LAST_POWER_EVENT+1];
Maybe I'm just being radical here :)
Czarek
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v5 2/6] ASoC: sgtl5000: Improve VAG power and mute control
2019-07-18 18:42 ` Cezary Rojewski
@ 2019-07-18 18:48 ` Cezary Rojewski
2019-07-19 7:09 ` Oleksandr Suvorov
0 siblings, 1 reply; 13+ messages in thread
From: Cezary Rojewski @ 2019-07-18 18:48 UTC (permalink / raw
To: Oleksandr Suvorov
Cc: Fabio Estevam, linux-kernel@vger.kernel.org, Igor Opaniuk,
Marcel Ziswiler, alsa-devel@alsa-project.org,
stable@vger.kernel.org, Jaroslav Kysela, Sasha Levin, Mark Brown,
Takashi Iwai, Liam Girdwood
On 2019-07-18 20:42, Cezary Rojewski wrote:
> On 2019-07-18 11:02, Oleksandr Suvorov wrote:
>> +enum {
>> + HP_POWER_EVENT,
>> + DAC_POWER_EVENT,
>> + ADC_POWER_EVENT,
>> + LAST_POWER_EVENT
>> +};
>> +
>> +static u16 mute_mask[] = {
>> + SGTL5000_HP_MUTE,
>> + SGTL5000_OUTPUTS_MUTE,
>> + SGTL5000_OUTPUTS_MUTE
>> +};
>
> If mute_mask[] is only used within common handler, you may consider
> declaring const array within said handler instead (did not check that
> myself).
> Otherwise, simple comment for the second _OUTPUTS_MUTE should suffice -
> its not self explanatory why you doubled that mask.
>
>> +
>> /* sgtl5000 private structure in codec */
>> struct sgtl5000_priv {
>> int sysclk; /* sysclk rate */
>> @@ -137,8 +157,109 @@ struct sgtl5000_priv {
>> u8 micbias_voltage;
>> u8 lrclk_strength;
>> u8 sclk_strength;
>> + u16 mute_state[LAST_POWER_EVENT];
>> };
>
> When I spoke of LAST enum constant, I did not really had this specific
> usage in mind.
>
> From design perspective, _LAST_ does not exist and should never be
> referred to as "the next option" i.e.: new enum constant.
> That is way preferred usage is:
> u16 mute_state[ADC_POWER_EVENT+1;
> -or-
> u16 mute_state[LAST_POWER_EVENT+1];
>
> Maybe I'm just being radical here :)
>
> Czarek
Forgive me for double posting. Comment above is targeted towards:
>> +enum {
>> + HP_POWER_EVENT,
>> + DAC_POWER_EVENT,
>> + ADC_POWER_EVENT,
>> + LAST_POWER_EVENT
>> +};
as LAST_POWER_EVENT is not assigned explicitly to ADC_POWER_EVENT and
thus generates implicit "new option" of value 3.
Czarek
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v5 2/6] ASoC: sgtl5000: Improve VAG power and mute control
2019-07-18 18:48 ` Cezary Rojewski
@ 2019-07-19 7:09 ` Oleksandr Suvorov
2019-07-19 7:19 ` Cezary Rojewski
0 siblings, 1 reply; 13+ messages in thread
From: Oleksandr Suvorov @ 2019-07-19 7:09 UTC (permalink / raw
To: Cezary Rojewski
Cc: Oleksandr Suvorov, Fabio Estevam, linux-kernel@vger.kernel.org,
Igor Opaniuk, Marcel Ziswiler, alsa-devel@alsa-project.org,
stable@vger.kernel.org, Jaroslav Kysela, Sasha Levin, Mark Brown,
Takashi Iwai, Liam Girdwood
On Thu, 18 Jul 2019 at 21:49, Cezary Rojewski <cezary.rojewski@intel.com> wrote:
>
> On 2019-07-18 20:42, Cezary Rojewski wrote:
> > On 2019-07-18 11:02, Oleksandr Suvorov wrote:
> >> +enum {
> >> + HP_POWER_EVENT,
> >> + DAC_POWER_EVENT,
> >> + ADC_POWER_EVENT,
> >> + LAST_POWER_EVENT
> >> +};
> >> +
> >> +static u16 mute_mask[] = {
> >> + SGTL5000_HP_MUTE,
> >> + SGTL5000_OUTPUTS_MUTE,
> >> + SGTL5000_OUTPUTS_MUTE
> >> +};
> >
> > If mute_mask[] is only used within common handler, you may consider
> > declaring const array within said handler instead (did not check that
> > myself).
> > Otherwise, simple comment for the second _OUTPUTS_MUTE should suffice -
> > its not self explanatory why you doubled that mask.
Ok, I'll add a comment to explain doubled mask.
> >
> >> +
> >> /* sgtl5000 private structure in codec */
> >> struct sgtl5000_priv {
> >> int sysclk; /* sysclk rate */
> >> @@ -137,8 +157,109 @@ struct sgtl5000_priv {
> >> u8 micbias_voltage;
> >> u8 lrclk_strength;
> >> u8 sclk_strength;
> >> + u16 mute_state[LAST_POWER_EVENT];
> >> };
> >
> > When I spoke of LAST enum constant, I did not really had this specific
> > usage in mind.
> >
> > From design perspective, _LAST_ does not exist and should never be
> > referred to as "the next option" i.e.: new enum constant.
By its nature, LAST_POWER_EVENT is actually a size of the array, but I
couldn't come up with a better name.
> > That is way preferred usage is:
> > u16 mute_state[ADC_POWER_EVENT+1;
> > -or-
> > u16 mute_state[LAST_POWER_EVENT+1];
> >
> > Maybe I'm just being radical here :)
Maybe :) I don't like first variant (ADC_POWER_EVENT+1): somewhen in
future, someone can add a new event to this enum and we've got a
possible situation with "out of array indexing".
> >
> > Czarek
>
> Forgive me for double posting. Comment above is targeted towards:
>
> >> +enum {
> >> + HP_POWER_EVENT,
> >> + DAC_POWER_EVENT,
> >> + ADC_POWER_EVENT,
> >> + LAST_POWER_EVENT
> >> +};
>
> as LAST_POWER_EVENT is not assigned explicitly to ADC_POWER_EVENT and
> thus generates implicit "new option" of value 3.
So will you be happy with the following variant?
...
ADC_POWER_EVENT,
LAST_POWER_EVENT = ADC_POWER_EVENT,
...
u16 mute_state[LAST_POWER_EVENT+1];
...
--
Best regards
Oleksandr Suvorov
Toradex AG
Altsagenstrasse 5 | 6048 Horw/Luzern | Switzerland | T: +41 41 500
4800 (main line)
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v5 2/6] ASoC: sgtl5000: Improve VAG power and mute control
2019-07-19 7:09 ` Oleksandr Suvorov
@ 2019-07-19 7:19 ` Cezary Rojewski
0 siblings, 0 replies; 13+ messages in thread
From: Cezary Rojewski @ 2019-07-19 7:19 UTC (permalink / raw
To: Oleksandr Suvorov
Cc: Fabio Estevam, linux-kernel@vger.kernel.org, Igor Opaniuk,
Marcel Ziswiler, alsa-devel@alsa-project.org,
stable@vger.kernel.org, Jaroslav Kysela, Sasha Levin, Mark Brown,
Takashi Iwai, Liam Girdwood
On 2019-07-19 09:09, Oleksandr Suvorov wrote:
> On Thu, 18 Jul 2019 at 21:49, Cezary Rojewski <cezary.rojewski@intel.com> wrote:
>>
>> On 2019-07-18 20:42, Cezary Rojewski wrote:
>>> On 2019-07-18 11:02, Oleksandr Suvorov wrote:
>>>> +enum {
>>>> + HP_POWER_EVENT,
>>>> + DAC_POWER_EVENT,
>>>> + ADC_POWER_EVENT,
>>>> + LAST_POWER_EVENT
>>>> +};
>>>> +
>>>> +static u16 mute_mask[] = {
>>>> + SGTL5000_HP_MUTE,
>>>> + SGTL5000_OUTPUTS_MUTE,
>>>> + SGTL5000_OUTPUTS_MUTE
>>>> +};
>>>
>>> If mute_mask[] is only used within common handler, you may consider
>>> declaring const array within said handler instead (did not check that
>>> myself).
>>> Otherwise, simple comment for the second _OUTPUTS_MUTE should suffice -
>>> its not self explanatory why you doubled that mask.
>
> Ok, I'll add a comment to explain doubled mask.
>
>>>
>>>> +
>>>> /* sgtl5000 private structure in codec */
>>>> struct sgtl5000_priv {
>>>> int sysclk; /* sysclk rate */
>>>> @@ -137,8 +157,109 @@ struct sgtl5000_priv {
>>>> u8 micbias_voltage;
>>>> u8 lrclk_strength;
>>>> u8 sclk_strength;
>>>> + u16 mute_state[LAST_POWER_EVENT];
>>>> };
>>>
>>> When I spoke of LAST enum constant, I did not really had this specific
>>> usage in mind.
>>>
>>> From design perspective, _LAST_ does not exist and should never be
>>> referred to as "the next option" i.e.: new enum constant.
>
> By its nature, LAST_POWER_EVENT is actually a size of the array, but I
> couldn't come up with a better name.
>
>>> That is way preferred usage is:
>>> u16 mute_state[ADC_POWER_EVENT+1;
>>> -or-
>>> u16 mute_state[LAST_POWER_EVENT+1];
>>>
>>> Maybe I'm just being radical here :)
>
> Maybe :) I don't like first variant (ADC_POWER_EVENT+1): somewhen in
> future, someone can add a new event to this enum and we've got a
> possible situation with "out of array indexing".
>
>>>
>>> Czarek
>>
>> Forgive me for double posting. Comment above is targeted towards:
>>
>> >> +enum {
>> >> + HP_POWER_EVENT,
>> >> + DAC_POWER_EVENT,
>> >> + ADC_POWER_EVENT,
>> >> + LAST_POWER_EVENT
>> >> +};
>>
>> as LAST_POWER_EVENT is not assigned explicitly to ADC_POWER_EVENT and
>> thus generates implicit "new option" of value 3.
>
> So will you be happy with the following variant?
> ...
> ADC_POWER_EVENT,
> LAST_POWER_EVENT = ADC_POWER_EVENT,
> ...
> u16 mute_state[LAST_POWER_EVENT+1];
> ...
>
It's not about being happy - I'm a happy man in general ;p
As stated already, declaring _LAST_ as the "new option" is misleading
and not advised.
And yeah, [_LAST_ + 1] is usually the one you should go with.
Czarek
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2019-07-19 7:19 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-07-18 9:02 [PATCH v5 0/6] VAG power control improvement for sgtl5000 codec Oleksandr Suvorov
2019-07-18 9:02 ` [PATCH v5 1/6] ASoC: Define a set of DAPM pre/post-up events Oleksandr Suvorov
2019-07-18 9:02 ` [PATCH v5 2/6] ASoC: sgtl5000: Improve VAG power and mute control Oleksandr Suvorov
2019-07-18 16:18 ` Igor Opaniuk
2019-07-18 16:25 ` Igor Opaniuk
2019-07-18 18:42 ` Cezary Rojewski
2019-07-18 18:48 ` Cezary Rojewski
2019-07-19 7:09 ` Oleksandr Suvorov
2019-07-19 7:19 ` Cezary Rojewski
2019-07-18 9:02 ` [PATCH v5 3/6] ASoC: sgtl5000: Fix definition of VAG Ramp Control Oleksandr Suvorov
2019-07-18 9:02 ` [PATCH v5 4/6] ASoC: sgtl5000: add ADC mute control Oleksandr Suvorov
2019-07-18 9:02 ` [PATCH v5 5/6] ASoC: sgtl5000: Fix of unmute outputs on probe Oleksandr Suvorov
2019-07-18 9:02 ` [PATCH v5 6/6] ASoC: sgtl5000: Fix charge pump source assignment Oleksandr Suvorov
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.