Re: [alsa-devel] [PATCH v3 6/6] ASoC: sgtl5000: Improve VAG power and mute control

On 2019-07-12 16:56, Oleksandr Suvorov wrote:

+enum {

• HP_POWER_EVENT,
• DAC_POWER_EVENT,
• ADC_POWER_EVENT

+};

+struct sgtl5000_mute_state {

• u16 hp_event;
• u16 dac_event;
• u16 adc_event;

+};

• /* sgtl5000 private structure in codec */ struct sgtl5000_priv { int sysclk; /* sysclk rate */

@@ -137,8 +156,109 @@ struct sgtl5000_priv { u8 micbias_voltage; u8 lrclk_strength; u8 sclk_strength;

• struct sgtl5000_mute_state mute_state;

Why not array instead? u16 mute_state[ADC_POWER_EVENT+1]; -or- u16 mute_state[LAST_POWER_EVENT+1]; (if you choose to add explicit LAST enum constant).

Enables simplification, see below.

@@ -170,40 +290,79 @@ 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 void vag_and_mute_control(struct snd_soc_component *component,

• 		 int event, int event_source,
  

• 		 u16 mute_mask, u16 *mute_reg)
  

  {

• struct snd_soc_component *component = snd_soc_dapm_to_component(w->dapm);
• const u32 mask = SGTL5000_DAC_POWERUP | SGTL5000_ADC_POWERUP;
• 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:

• *mute_reg = mute_output(component, mute_mask);
  

• break;
  

• case SND_SOC_DAPM_POST_PMU:

• vag_power_on(component, event_source);
  

• restore_output(component, mute_mask, *mute_reg);
  

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

• }
  

• *mute_reg = mute_output(component, mute_mask);
  

• vag_power_off(component, event_source);
  

• break;
  

• case SND_SOC_DAPM_POST_PMD:

• restore_output(component, mute_mask, *mute_reg);
  

  break; default: break; }

+}

+/*

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

• struct sgtl5000_priv *sgtl5000 =

• snd_soc_component_get_drvdata(component);
  

• vag_and_mute_control(component, event, HP_POWER_EVENT,

• 	     SGTL5000_HP_MUTE,
  

• 	     &sgtl5000->mute_state.hp_event);
  

• return 0;

+}

+/* 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);
  

• struct sgtl5000_priv *sgtl5000 =

• snd_soc_component_get_drvdata(component);
  

• vag_and_mute_control(component, event, ADC_POWER_EVENT,

• 	     SGTL5000_OUTPUTS_MUTE,
  

• 	     &sgtl5000->mute_state.adc_event);
  

• return 0;

+}

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

• struct sgtl5000_priv *sgtl5000 =

• snd_soc_component_get_drvdata(component);
  

• vag_and_mute_control(component, event, DAC_POWER_EVENT,

• 	     SGTL5000_OUTPUTS_MUTE,
  

• 	     &sgtl5000->mute_state.dac_event);
  
  

  return 0; }

With array approach you can simplify these 3 callbacks: - remove local declaration of sgtl5000 - remove the need for "u16 *mute_reg" param for vag_and_mute_control (you always provide the xxx_event field of mute_state corresponding to given XXX_POWER_EVENT anyway)

The sgtl5000 local ptr would be declared within common handler, which vag_and_mute_control clearly is. Said handler declaration could be updated to again require widget rather than component.

Cherry on top: relocation of "return 0;" directly to vag_and_mute_control. Leaving it void (as it is), however, might also be appropriate (explicit).

Czarek