Re: [PATCH v2 2/3] ASoC: codecs: rk3308: restore audio and headphone support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 11 potential issue(s) to consider:
- [Medium] ASoC: rk3308: rk3308_codec_adc_event() clobbers MIC Capture Switch state
- [Medium] ASoC: rk3308: broken mute and HPMIX_WORK sequencing in rk3308_codec_dac_event()
- [Medium] ASoC: rk3308: redundant PMU and invalid POP_SOUND in rk3308_codec_output_event()
- [Medium] ASoC: rk3308: zero-cross detection disabled in rk3308_codec_initialize()
- [Medium] ASoC: rk3308: rk3308_codec_dac_event() enables inactive DAPM outputs
- [Medium] ASoC: rk3308: HPOUT subseq inverts power order against machine HP widget
- [Medium] ASoC: rk3308: CH1_IN_SEL and CH2_IN_SEL not converted to SND_SOC_NOPM
- [Medium] ASoC: rk3308: leak of devm_kmemdup() widgets in rk3308_codec_add_widgets()
- [Medium] ASoC: rk3308: non-atomic route and dac_output update in rk3308_codec_set_output()
- [Medium] ASoC: rk3308: race on mixer registers in rk3308_codec_dac_event()
- [Medium] ASoC: rk3308: lock inversion on io_lock in rk3308_codec_mute_stream()
--

commit a099049aa9bf21b04f0558d74e37572e81c21ec8
Author: Xing Zheng <zhengxing@rock-chips.com>

ASoC: codecs: rk3308: restore audio and headphone support

This commit restores the vendor analog enable and disable sequencing,
settling delays, gain ramps, and ADC resynchronization across codec
revisions A, B, and C using DAPM supply events and virtual intermediate
widgets. It also adds per-channel microphone boost controls, ADC stereo
group routing, selectable DAC output routing, and headphone jack detection.

> diff --git a/sound/soc/codecs/rk3308_codec.c b/sound/soc/codecs/rk3308_codec.c
> index a3d41cd8150d0..1894e1a93fbf8 100644
> --- a/sound/soc/codecs/rk3308_codec.c
> +++ b/sound/soc/codecs/rk3308_codec.c

[ ... ]

> @@ -78,7 +184,288 @@ static SOC_ENUM_SINGLE_DECL(rk3308_codec_hpf_cutoff_enum56, RK3308_ADC_DIG_CON04
>  static SOC_ENUM_SINGLE_DECL(rk3308_codec_hpf_cutoff_enum78, RK3308_ADC_DIG_CON04(3), 0,
>  			    rk3308_codec_hpf_cutoff_text);
>  
> +static int rk3308_codec_output_routes(struct rk3308_codec_priv *rk3308)
> +{
> +	static const struct snd_soc_dapm_route routes[][2] = {
> +		{
> +			{ "L_LINEOUT_EN", NULL, "DAC_L_HPMIX_SEL" },
> +			{ "R_LINEOUT_EN", NULL, "DAC_R_HPMIX_SEL" },
> +		}, {
> +			{ "L_HPOUT_EN", NULL, "DAC_L_HPMIX_SEL" },
> +			{ "R_HPOUT_EN", NULL, "DAC_R_HPMIX_SEL" },
> +		},
> +	};
> +	struct snd_soc_dapm_context *dapm = snd_soc_component_to_dapm(rk3308->component);
> +	u32 mask = rk3308->dac_output == 11 ? 3 :
> +		rk3308->dac_output == 0 ? 1 : 2;
> +	int i, ret;
> +
> +	for (i = 0; i < ARRAY_SIZE(routes); i++) {
> +		if (!((mask ^ rk3308->output_routes_mask) & BIT(i)))
> +			continue;
> +		if (mask & BIT(i))
> +			ret = snd_soc_dapm_add_routes(dapm, routes[i], ARRAY_SIZE(routes[i]));
> +		else
> +			ret = snd_soc_dapm_del_routes(dapm, routes[i], ARRAY_SIZE(routes[i]));
> +		if (ret)
> +			return ret;
> +		rk3308->output_routes_mask ^= BIT(i);
> +	}
> +
> +	return 0;
> +}
> +
> +static int rk3308_codec_set_output(struct rk3308_codec_priv *rk3308, unsigned int output)
> +{
> +	unsigned int saved = rk3308->dac_output;
> +	int ret;
> +
> +	rk3308->dac_output = output;
> +	ret = rk3308_codec_output_routes(rk3308);
> +	if (ret) {
> +		rk3308->dac_output = saved;
> +		rk3308_codec_output_routes(rk3308);
> +	}
> +	if (!ret && rk3308->hpdet_jack)
> +		snd_soc_jack_report(rk3308->hpdet_jack,
> +				    output ? SND_JACK_HEADPHONE : 0, SND_JACK_HEADPHONE);
> +	if (!ret)
> +		ret = snd_soc_dapm_sync(snd_soc_component_to_dapm(rk3308->component));
> +	return ret;
> +}

[Severity: Medium]
Can updating rk3308->dac_output and the DAPM routes in
rk3308_codec_set_output() race with concurrent DAPM power sequencing?

In rk3308_codec_set_output(), rk3308->dac_output is modified under
rk3308->io_lock before card->dapm_mutex is acquired in
rk3308_codec_output_routes() and snd_soc_dapm_sync(). Meanwhile,
rk3308_codec_dac_event() reads rk3308->dac_output while holding
card->dapm_mutex (without holding rk3308->io_lock):

    bool lineout = rk3308->dac_output != 1;
    bool headphone = rk3308->dac_output != 0;

Because dapm_power_widgets() holds card->dapm_mutex across
rk3308_codec_set_bias_level() and the delays in rk3308_codec_dac_event(), a
concurrent call to rk3308_codec_set_output() via rk3308_codec_output_put()
or rk3308_codec_hpdet_work() can overwrite rk3308->dac_output and block on
card->dapm_mutex in rk3308_codec_output_routes(), causing
rk3308_codec_dac_event() to execute with the new rk3308->dac_output value
while DAPM is still powering up the widget list computed from the old route
graph.

In addition, each call to snd_soc_dapm_del_routes() and
snd_soc_dapm_add_routes() in rk3308_codec_output_routes() acquires and
releases card->dapm_mutex separately. When switching from Line Out
(dac_output == 0, mask == 1) to Headphone (dac_output == 1, mask == 2),
routes[0] is deleted in the first iteration and card->dapm_mutex is dropped
before routes[1] is added in the second iteration. If another thread runs a
DAPM sync in that window, DAPM sees no connected output routes from
DAC_L_HPMIX_SEL and DAC_R_HPMIX_SEL and powers down the active DAC.

[ ... ]

> @@ -175,107 +562,350 @@ static const struct snd_kcontrol_new rk3308_codec_controls[] = {

[ ... ]

> +/* DAPM calls this before the machine driver enables either output amplifier. */
> +static int rk3308_codec_dac_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 rk3308_codec_priv *rk3308 = snd_soc_component_get_drvdata(component);
> +	unsigned int line, hp, aif1, aif2, digital_gain = 0, gain_l, gain_r;
> +	int gain;
> +	unsigned int line_unmute = BIT(RK3308_DAC_L_LINEOUT_MUTE_SFT) |
> +				   BIT(RK3308_DAC_R_LINEOUT_MUTE_SFT);
> +	bool lineout = rk3308->dac_output != 1;
> +	bool headphone = rk3308->dac_output != 0;

[Severity: Medium]
Does deriving lineout and headphone from rk3308->dac_output instead of the
DAPM widget power states cause rk3308_codec_dac_event() to enable inactive
outputs?

rk3308_codec_probe() initializes rk3308->dac_output to 11 (Both). On boards
that only route LINEOUT or HPOUT in Device Tree audio-routing (or when
headphone detection is not used, such as on revision A), the unconnected
output widgets have new_power == 0, but rk3308_codec_dac_event() still sees
both lineout and headphone as true and enables both RK3308_DAC_L/R_LINEOUT_EN
and RK3308_DAC_L/R_HPOUT_EN while ramping the headphone gain.

> +
> +	rk3308->dac_analog = w;
> +	regmap_read(rk3308->regmap, RK3308_DAC_ANA_CON04, &line);
> +	regmap_read(rk3308->regmap, RK3308_DAC_ANA_CON03, &hp);
> +	regmap_read(rk3308->regmap, RK3308_DAC_ANA_CON05, &gain_l);
> +	regmap_read(rk3308->regmap, RK3308_DAC_ANA_CON06, &gain_r);
> +	gain_l &= RK3308_DAC_x_HPOUT_GAIN_MSK;
> +	gain_r &= RK3308_DAC_x_HPOUT_GAIN_MSK;
> +	if (event == SND_SOC_DAPM_PRE_PMU) {
> +		if (headphone) {
> +			regmap_update_bits(rk3308->regmap, RK3308_DAC_ANA_CON05,
> +					   RK3308_DAC_x_HPOUT_GAIN_MSK, 0);
> +			regmap_update_bits(rk3308->regmap, RK3308_DAC_ANA_CON06,
> +					   RK3308_DAC_x_HPOUT_GAIN_MSK, 0);
> +		}

[Severity: Medium]
What happens if userspace reads or writes the Headphone Playback Volume,
Headphone Playback Switch, or Line Out Playback Switch controls while
rk3308_codec_dac_event() is running?

rk3308_codec_dac_event() runs under card->dapm_mutex and snapshots
RK3308_DAC_ANA_CON03..06 into hp, line, gain_l, and gain_r, then zeroes or
ramps RK3308_DAC_ANA_CON05/06 and clears the unmute bits in
RK3308_DAC_ANA_CON03/04 across multiple sleeps (~10-14 ms) before writing the
snapshots back. Because snd_soc_get_volsw() and snd_soc_put_volsw() run
under card->controls_rwsem without acquiring card->dapm_mutex, concurrent
mixer reads can observe transient zeroed/muted register values and
concurrent mixer writes to RK3308_DAC_ANA_CON03..06 will be overwritten when
rk3308_codec_dac_event() restores its initial snapshots.

[ ... ]

> +		/* Steps 15-21: select I2S and restore the user mute/gain settings. */
> +		rk3308_codec_analog_update(rk3308, RK3308_DAC_ANA_CON12,
> +					   RK3308_DAC_L_HPMIX_SEL_MSK | RK3308_DAC_R_HPMIX_SEL_MSK,
> +			RK3308_DAC_L_HPMIX_I2S | RK3308_DAC_R_HPMIX_I2S);
> +		rk3308_codec_analog_update(rk3308, RK3308_DAC_ANA_CON13,
> +					   (RK3308_DAC_L_HPMIX_UNMUTE | RK3308_DAC_R_HPMIX_UNMUTE),
> +			(RK3308_DAC_L_HPMIX_UNMUTE | RK3308_DAC_R_HPMIX_UNMUTE));
> +		if (headphone)
> +			for (gain = 0; gain <= max(gain_l, gain_r); gain++) {
> +				regmap_update_bits(rk3308->regmap, RK3308_DAC_ANA_CON05,
> +						   RK3308_DAC_x_HPOUT_GAIN_MSK,
> +					min_t(unsigned int, gain, gain_l));
> +				regmap_update_bits(rk3308->regmap, RK3308_DAC_ANA_CON06,
> +						   RK3308_DAC_x_HPOUT_GAIN_MSK,
> +					min_t(unsigned int, gain, gain_r));
> +			}
> +		rk3308_codec_resync_adcs(rk3308);
> +	} else {
> +		/* Vendor DAC disable steps 3-19, after the external amps are off. */
> +		if (hp & (RK3308_DAC_L_HPOUT_EN | RK3308_DAC_R_HPOUT_EN))
> +			for (gain = max(gain_l, gain_r); gain >= 0; gain--) {
> +				regmap_update_bits(rk3308->regmap, RK3308_DAC_ANA_CON05,
> +						   RK3308_DAC_x_HPOUT_GAIN_MSK,
> +					min_t(unsigned int, gain, gain_l));
> +				regmap_update_bits(rk3308->regmap, RK3308_DAC_ANA_CON06,
> +						   RK3308_DAC_x_HPOUT_GAIN_MSK,
> +					min_t(unsigned int, gain, gain_r));
> +				usleep_range(200, 300);
> +			}
> +		rk3308_codec_analog_update(rk3308, RK3308_DAC_ANA_CON12,
> +					   RK3308_DAC_L_HPMIX_SEL_MSK | RK3308_DAC_R_HPMIX_SEL_MSK,
> +			RK3308_DAC_L_HPMIX_NONE | RK3308_DAC_R_HPMIX_NONE);
> +		rk3308_codec_analog_update(rk3308, RK3308_DAC_ANA_CON03,
> +					   RK3308_DAC_L_HPOUT_UNMUTE |
> +					   RK3308_DAC_R_HPOUT_UNMUTE, 0);
> +		rk3308_codec_analog_update(rk3308, RK3308_DAC_ANA_CON02,
> +					   (RK3308_DAC_L_DAC_WORK | RK3308_DAC_R_DAC_WORK), 0);
> +		rk3308_codec_analog_update(rk3308, RK3308_DAC_ANA_CON03,
> +					   (RK3308_DAC_L_HPOUT_EN | RK3308_DAC_R_HPOUT_EN), 0);
> +		rk3308_codec_analog_update(rk3308, RK3308_DAC_ANA_CON04, line_unmute, 0);
> +		rk3308_codec_analog_update(rk3308, RK3308_DAC_ANA_CON04,
> +					   (RK3308_DAC_L_LINEOUT_EN | RK3308_DAC_R_LINEOUT_EN), 0);
> +		rk3308_codec_analog_update(rk3308, RK3308_DAC_ANA_CON13,
> +					   (RK3308_DAC_L_HPMIX_EN | RK3308_DAC_R_HPMIX_EN), 0);
> +		rk3308_codec_analog_update(rk3308, RK3308_DAC_ANA_CON02,
> +					   (RK3308_DAC_L_DAC_EN | RK3308_DAC_R_DAC_EN), 0);
> +		rk3308_codec_analog_update(rk3308, RK3308_DAC_ANA_CON02,
> +					   (RK3308_DAC_L_CLK_EN | RK3308_DAC_R_CLK_EN), 0);
> +		rk3308_codec_analog_update(rk3308, RK3308_DAC_ANA_CON02,
> +					   (RK3308_DAC_L_REF_EN | RK3308_DAC_R_REF_EN), 0);
> +		rk3308_codec_analog_update(rk3308, RK3308_DAC_ANA_CON01,
> +					   RK3308_DAC_HPOUT_POP_SOUND_L_MSK |
> +					   RK3308_DAC_HPOUT_POP_SOUND_R_MSK,
> +			RK3308_DAC_HPOUT_POP_SOUND_L_INIT | RK3308_DAC_HPOUT_POP_SOUND_R_INIT);
> +		if (rk3308->codec_ver >= ACODEC_VERSION_B)
> +			rk3308_codec_analog_update(rk3308, RK3308_DAC_ANA_CON15,
> +						   RK3308_DAC_LINEOUT_POP_SOUND_L_MSK |
> +						   RK3308_DAC_LINEOUT_POP_SOUND_R_MSK,
> +			RK3308_DAC_L_SEL_DC_FROM_VCM | RK3308_DAC_R_SEL_DC_FROM_VCM);
> +		rk3308_codec_analog_update(rk3308, RK3308_DAC_ANA_CON01,
> +					   (RK3308_DAC_BUF_REF_L_EN | RK3308_DAC_BUF_REF_R_EN), 0);
> +		rk3308_codec_analog_update(rk3308, RK3308_DAC_ANA_CON00,
> +					   RK3308_DAC_CURRENT_EN, 0);
> +		rk3308_codec_analog_update(rk3308, RK3308_DAC_ANA_CON03,
> +					   (RK3308_DAC_L_HPOUT_WORK | RK3308_DAC_R_HPOUT_WORK), 0);
> +		rk3308_codec_analog_update(rk3308, RK3308_DAC_ANA_CON13,
> +					   (RK3308_DAC_L_HPMIX_WORK | RK3308_DAC_R_HPMIX_WORK),
> +			(RK3308_DAC_L_HPMIX_WORK | RK3308_DAC_R_HPMIX_WORK));

[Severity: Medium]
Should the SND_SOC_DAPM_PRE_PMU and SND_SOC_DAPM_POST_PMD branches in
rk3308_codec_dac_event() be muting the outputs during power-up and clearing
RK3308_DAC_L/R_HPMIX_WORK and RK3308_DAC_L/R_HPMIX_UNMUTE on power-down?

In rk3308_codec_initialize() and at the end of SND_SOC_DAPM_POST_PMD, the
driver sets the unmute bits in RK3308_DAC_ANA_CON03 and RK3308_DAC_ANA_CON04
so the ALSA switches remain readable while the DAC is off. At the start of
rk3308_codec_dac_event(), line and hp are read from RK3308_DAC_ANA_CON04 and
RK3308_DAC_ANA_CON03, and the comment at steps 15-21 says "select I2S and
restore the user mute/gain settings". However, SND_SOC_DAPM_PRE_PMU never
clears the unmute bits in RK3308_DAC_ANA_CON03 or RK3308_DAC_ANA_CON04
before enabling the outputs, and never uses line or hp to restore them at
steps 15-21. As a result, LINEOUT_EN and HPOUT_EN/HPOUT_WORK are powered on
while already unmuted before DAC_REF_EN, DAC_CLK_EN, DAC_EN, DAC_WORK, and
HPMIX_SEL are configured.

Additionally, in the SND_SOC_DAPM_POST_PMD branch,
RK3308_DAC_L_HPMIX_UNMUTE | RK3308_DAC_R_HPMIX_UNMUTE in
RK3308_DAC_ANA_CON13 is never cleared, and the update to
RK3308_DAC_L_HPMIX_WORK | RK3308_DAC_R_HPMIX_WORK passes
(RK3308_DAC_L_HPMIX_WORK | RK3308_DAC_R_HPMIX_WORK) instead of 0 as the
value argument, leaving HPMIX unmuted and in WORK state across power-down.

> +		regmap_update_bits(rk3308->regmap, RK3308_DAC_ANA_CON05,
> +				   RK3308_DAC_x_HPOUT_GAIN_MSK, gain_l);
> +		regmap_update_bits(rk3308->regmap, RK3308_DAC_ANA_CON06,
> +				   RK3308_DAC_x_HPOUT_GAIN_MSK, gain_r);
> +		rk3308_codec_resync_adcs(rk3308);
> +		/* Preserve ALSA switches while the analog output stages are off. */
> +		regmap_update_bits(rk3308->regmap, RK3308_DAC_ANA_CON03,
> +				   RK3308_DAC_L_HPOUT_UNMUTE | RK3308_DAC_R_HPOUT_UNMUTE,
> +			hp & (RK3308_DAC_L_HPOUT_UNMUTE | RK3308_DAC_R_HPOUT_UNMUTE));
> +		regmap_update_bits(rk3308->regmap, RK3308_DAC_ANA_CON04,
> +				   line_unmute, line & line_unmute);
> +	}
>  
>  	return 0;
>  }
>  
> +static int rk3308_codec_adc_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 rk3308_codec_priv *rk3308 = snd_soc_component_get_drvdata(component);
> +	unsigned int grp = w->shift;
> +	unsigned int zerocross;
> +
> +	if (event == SND_SOC_DAPM_PRE_PMU) {
> +		regmap_update_bits(rk3308->regmap, RK3308_ADC_ANA_CON07(grp),
> +				   RK3308_ADC_CH1_IN_SEL_MSK | RK3308_ADC_CH2_IN_SEL_MSK,
> +				   RK3308_ADC_CH1_IN_MIC | RK3308_ADC_CH2_IN_MIC);
> +		regmap_set_bits(rk3308->regmap, RK3308_ADC_ANA_CON00(grp),
> +				RK3308_ADC_CH1_MIC_UNMUTE | RK3308_ADC_CH2_MIC_UNMUTE);

[Severity: Medium]
Doesn't setting RK3308_ADC_CH1_MIC_UNMUTE | RK3308_ADC_CH2_MIC_UNMUTE in
rk3308_codec_adc_event() on SND_SOC_DAPM_PRE_PMU overwrite the user's
MIC1..MIC8 Capture Switch settings?

In rk3308_codec_controls[], MIC1 Capture Switch through MIC8 Capture Switch
are backed by bits 3 (RK3308_ADC_CH1_MIC_UNMUTE) and 7
(RK3308_ADC_CH2_MIC_UNMUTE) of RK3308_ADC_ANA_CON00(0..3), and
rk3308_codec_initialize() already sets BIT(3) | BIT(7) once after reset so
those controls remain adjustable. Unconditionally setting those bits on every
SND_SOC_DAPM_PRE_PMU in rk3308_codec_adc_event() forces any muted microphone
channels back to unmuted whenever capture starts.

[ ... ]

> +/* Output-only route changes must not restart the shared DAC analog supply. */
> +static int rk3308_codec_output_event(struct snd_soc_dapm_widget *w,
> +				     bool hp, int event)
> +{
> +	struct snd_soc_component *component = snd_soc_dapm_to_component(w->dapm);
> +	struct rk3308_codec_priv *rk3308 = snd_soc_component_get_drvdata(component);
> +	unsigned int shift = hp ? w->shift - 1 : w->shift;
> +	unsigned int reg = hp ? RK3308_DAC_ANA_CON03 : RK3308_DAC_ANA_CON04;
> +	unsigned int mask = (hp ? (RK3308_DAC_L_HPOUT_EN | RK3308_DAC_L_HPOUT_WORK) :
> +			     RK3308_DAC_L_LINEOUT_EN) << shift;
>  
> -	if (rk3308->codec_ver != ACODEC_VERSION_B)
> +	if (!rk3308->dac_analog)
>  		return 0;
>  
> -	switch (event) {
> -	case SND_SOC_DAPM_POST_PMU:
> -		val = RK3308_DAC_L_SEL_LINEOUT_FROM_INTERNAL << w->shift;
> -		break;
> -	case SND_SOC_DAPM_POST_PMD:
> -		val = RK3308_DAC_L_SEL_DC_FROM_INTERNAL << w->shift;
> -		break;
> -	default:
> +	/*
> +	 * ASoC computes widget new_power before running DAPM events.
> +	 * If the shared supply is also going down, its shutdown event runs
> +	 * the complete vendor disable sequence. Recheck this ordering when
> +	 * updating the ASoC core.
> +	 */
> +	if (event == SND_SOC_DAPM_PRE_PMD && !rk3308->dac_analog->new_power)
>  		return 0;
> +	if (event == SND_SOC_DAPM_PRE_PMU) {
> +		if (hp) {
> +			rk3308_codec_analog_update(rk3308, RK3308_DAC_ANA_CON01,
> +						   RK3308_DAC_HPOUT_POP_SOUND_x_MSK << shift,
> +						   RK3308_DAC_HPOUT_POP_SOUND_x_WORK << shift);
> +			rk3308_codec_analog_update(rk3308, reg,
> +						   RK3308_DAC_L_HPOUT_EN << shift,
> +				RK3308_DAC_L_HPOUT_EN << shift);
> +		}
> +		rk3308_codec_analog_update(rk3308, reg,
> +					   hp ? RK3308_DAC_L_HPOUT_WORK << shift : mask,
> +					   hp ? RK3308_DAC_L_HPOUT_WORK << shift : mask);
> +	} else {
> +		if (hp)
> +			rk3308_codec_analog_update(rk3308, reg, RK3308_DAC_L_HPOUT_EN << shift, 0);
> +		rk3308_codec_analog_update(rk3308, reg,
> +					   hp ? RK3308_DAC_L_HPOUT_WORK << shift : mask, 0);
> +		if (hp)
> +			rk3308_codec_analog_update(rk3308, RK3308_DAC_ANA_CON01,
> +						   RK3308_DAC_HPOUT_POP_SOUND_x_MSK << shift, 0);
>  	}

[Severity: Medium]
In rk3308_codec_output_event(), why is 0 written to
RK3308_DAC_HPOUT_POP_SOUND_x_MSK << shift on SND_SOC_DAPM_PRE_PMD instead of
RK3308_DAC_HPOUT_POP_SOUND_x_INIT << shift (0x1 << shift) as in
rk3308_codec_dac_event() and rk3308_codec_set_bias_level()?

Also, on playback power-up, DAC_CURRENT_EN (snd_soc_dapm_supply) runs
rk3308_codec_dac_event(SND_SOC_DAPM_PRE_PMU) and sets rk3308->dac_analog = w
before the L/R_HPOUT_EN and L/R_LINEOUT_EN (snd_soc_dapm_out_drv) widgets
run rk3308_codec_output_event(SND_SOC_DAPM_PRE_PMU). Because there is no
SND_SOC_DAPM_PRE_PMU check corresponding to the SND_SOC_DAPM_PRE_PMD check,
rk3308_codec_output_event() re-executes these output register writes and
delays after rk3308_codec_dac_event() has already completed the full enable
sequence and headphone gain ramp.

[ ... ]

> @@ -400,33 +1027,116 @@ static const struct snd_soc_dapm_widget rk3308_codec_dapm_widgets[] = {
>  
>  	SND_SOC_DAPM_SUPPLY("DAC_BUF_REF_L", RK3308_DAC_ANA_CON01, 2, 0, NULL, 0),
>  	SND_SOC_DAPM_SUPPLY("DAC_BUF_REF_R", RK3308_DAC_ANA_CON01, 6, 0, NULL, 0),
> -	SND_SOC_DAPM_OUT_DRV_E("HPOUT_POP_SOUND_L", SND_SOC_NOPM, 0, 0, NULL, 0,
> -			       rk3308_codec_pop_sound_set,
> -			       SND_SOC_DAPM_POST_PMU | SND_SOC_DAPM_PRE_PMD),
> -	SND_SOC_DAPM_OUT_DRV_E("HPOUT_POP_SOUND_R", SND_SOC_NOPM, 4, 0, NULL, 0,
> -			       rk3308_codec_pop_sound_set,
> -			       SND_SOC_DAPM_POST_PMU | SND_SOC_DAPM_PRE_PMD),
> -	SND_SOC_DAPM_SUPPLY("DAC_OUTPUT_VCM", SND_SOC_NOPM, 0, 0,
> -			    rk3308_codec_dac_output_vcm_event,
> -			    SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_POST_PMU |
> -			    SND_SOC_DAPM_POST_PMD),
> -	SND_SOC_DAPM_OUT_DRV("L_HPOUT_EN", RK3308_DAC_ANA_CON03, 1, 0, NULL, 0),
> -	SND_SOC_DAPM_OUT_DRV("R_HPOUT_EN", RK3308_DAC_ANA_CON03, 5, 0, NULL, 0),
> -	SND_SOC_DAPM_OUT_DRV("L_HPOUT_WORK", RK3308_DAC_ANA_CON03, 2, 0, NULL, 0),
> -	SND_SOC_DAPM_OUT_DRV("R_HPOUT_WORK", RK3308_DAC_ANA_CON03, 6, 0, NULL, 0),
> +	{
> +		.id = snd_soc_dapm_out_drv, .name = "L_HPOUT_EN",
> +		SND_SOC_DAPM_INIT_REG_VAL(SND_SOC_NOPM, 1, 0),
> +		.subseq = 1, /* Enable HPOUT after the HPMIX output drivers. */
> +		.event = rk3308_codec_hpout_event,
> +		.event_flags = SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_PRE_PMD,
> +	},
> +	{
> +		.id = snd_soc_dapm_out_drv, .name = "R_HPOUT_EN",
> +		SND_SOC_DAPM_INIT_REG_VAL(SND_SOC_NOPM, 5, 0),
> +		.subseq = 1, /* Enable HPOUT after the HPMIX output drivers. */
> +		.event = rk3308_codec_hpout_event,
> +		.event_flags = SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_PRE_PMD,
> +	},

[Severity: Medium]
Does setting .subseq = 1 on L_HPOUT_EN and R_HPOUT_EN invert the power
ordering relative to machine SND_SOC_DAPM_HP widgets?

In sound/soc/soc-dapm.c, snd_soc_dapm_out_drv and snd_soc_dapm_hp have the
same sequence priority in both dapm_up_seq and dapm_down_seq, so
dapm_seq_compare() orders them by subseq (ascending on power-up, descending
on power-down). Meanwhile, rk3308_codec_add_widgets() converts the HPMIX
output drivers into SND_SOC_NOPM widgets with no events.

With .subseq = 1 on L_HPOUT_EN and R_HPOUT_EN, a machine SND_SOC_DAPM_HP
widget (subseq 0) powers up before rk3308_codec_hpout_event() enables the
codec HPOUT stage, and powers down after rk3308_codec_hpout_event() disables
the codec HPOUT stage when switching output routes.

[ ... ]

> +static int rk3308_codec_add_widgets(struct snd_soc_component *component)
> +{
> +	struct snd_soc_dapm_widget *widgets;
> +	int i, grp, ret;
> +
> +	widgets = devm_kmemdup(component->dev, rk3308_codec_dapm_widgets,
> +			       sizeof(rk3308_codec_dapm_widgets), GFP_KERNEL);
> +	if (!widgets)
> +		return -ENOMEM;

[Severity: Medium]
Does rk3308_codec_add_widgets() leak the widgets array allocated with
devm_kmemdup()?

snd_soc_dapm_new_controls() duplicates each widget via dapm_cnew_widget() and
does not keep the caller's template array. Because rk3308_codec_add_widgets()
is called from rk3308_codec_probe() (the ASoC component probe callback) and
allocates widgets on component->dev with devm_kmemdup(), the temporary array
is kept for the lifetime of the platform device and an additional copy is
allocated on every card unbind/rebind or probe retry cycle.

> +	for (i = 0; i < ARRAY_SIZE(rk3308_codec_dapm_widgets); i++) {
> +		struct snd_soc_dapm_widget *w = &widgets[i];
> +		unsigned int reg = w->reg;
> +
> +		if (reg >= RK3308_DAC_ANA_CON00 && reg <= RK3308_DAC_ANA_CON13) {
> +			w->reg = SND_SOC_NOPM;
> +			if (reg == RK3308_DAC_ANA_CON04)
> +				w->event_flags = SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_PRE_PMD;
> +			if (reg == RK3308_DAC_ANA_CON00) {
> +				w->subseq = 1;
> +				w->event = rk3308_codec_dac_event;
> +				w->event_flags = SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_POST_PMD;
> +			}
> +		}
> +		for (grp = 0; grp < ADC_LR_GROUP_MAX; grp++) {
> +			if (reg == RK3308_ADC_ANA_CON00(grp) ||
> +			    reg == RK3308_ADC_ANA_CON02(grp) ||
> +			    reg == RK3308_ADC_ANA_CON05(grp))
> +				w->reg = SND_SOC_NOPM;

[Severity: Medium]
Should RK3308_ADC_ANA_CON07(0) (used by CH1_IN_SEL and CH2_IN_SEL) also be
converted to SND_SOC_NOPM here?

CH1_IN_SEL and CH2_IN_SEL are defined in rk3308_codec_dapm_widgets[] as
SND_SOC_DAPM_PGA widgets backed by RK3308_ADC_ANA_CON07(0) shifts 4 and 6,
while rk3308_codec_initialize() and rk3308_codec_adc_event() also program
RK3308_ADC_CH1_IN_MIC | RK3308_ADC_CH2_IN_MIC in RK3308_ADC_ANA_CON07(grp).

Because CH1_IN_SEL and CH2_IN_SEL keep w->reg = RK3308_ADC_ANA_CON07(0):
1. During card instantiation, snd_soc_dapm_new_widgets() sees bits 4 and 6
   set by rk3308_codec_initialize() on the unpowered PGA widgets and clears
   them back to 0.
2. On capture stop, CH1_IN_SEL and CH2_IN_SEL (snd_soc_dapm_pga,
   dapm_down_seq = 6) clear bits 4 and 6 in RK3308_ADC_ANA_CON07(0) while the
   ADC, ALC, and MIC stages are still enabled and in WORK mode, before
   ADC_CURRENT_EN12 (snd_soc_dapm_supply, dapm_down_seq = 14) runs
   rk3308_codec_adc_event(SND_SOC_DAPM_POST_PMD).

[ ... ]

> @@ -775,6 +1480,24 @@ static int rk3308_codec_hw_params(struct snd_pcm_substream *substream,
>  		rk3308_codec_adc_dig_config(rk3308, params);
>  }
>  
> +static int rk3308_codec_mute_stream(struct snd_soc_dai *dai, int mute, int stream)
> +{
> +	struct rk3308_codec_priv *rk3308 = snd_soc_component_get_drvdata(dai->component);
> +	int gain;
> +
> +	if (rk3308->codec_ver > ACODEC_VERSION_B || stream != SNDRV_PCM_STREAM_PLAYBACK)
> +		return 0;
> +	mutex_lock(&rk3308->io_lock);
> +	for (gain = mute ? 2 : 7; mute ? gain <= 7 : gain >= 2; gain += mute ? 1 : -1) {
> +		regmap_update_bits(rk3308->regmap, RK3308_DAC_DIG_CON04,
> +				   RK3308_DAC_CIC_IF_GAIN_MSK, gain);
> +		usleep_range(200, 300);
> +	}
> +	mutex_unlock(&rk3308->io_lock);
> +
> +	return 0;
> +}

[Severity: Medium]
Can acquiring rk3308->io_lock in rk3308_codec_mute_stream() cause a lock
inversion against card->dapm_mutex?

rk3308_codec_output_put(), rk3308_codec_hpdet_work(), and
rk3308_codec_set_jack() acquire rk3308->io_lock and then acquire
card->dapm_mutex via snd_soc_dapm_add_routes(), snd_soc_dapm_del_routes(),
snd_soc_jack_report(), snd_soc_dapm_sync(), and snd_soc_dapm_mutex_lock().
However, ASoC can invoke .mute_stream() via dapm_dai_link_event() ->
snd_soc_dai_digital_mute() while card->dapm_mutex is already held, inverting
the lock order to dapm_mutex -> io_lock.

[ ... ]

> @@ -853,89 +1577,143 @@ static int rk3308_codec_initialize(struct rk3308_codec_priv *rk3308)

[ ... ]

>  	/* recover DAC digital gain to 0 dB (reset value is 0xff, undocumented) */
>  	if (rk3308->codec_ver == ACODEC_VERSION_C)
>  		regmap_write(rk3308->regmap, RK3308_DAC_DIG_CON04,
>  			     RK3308BS_DAC_DIG_GAIN_0DB);
>  
> -	/*
> -	 * Unconditionally enable zero-cross detection (needed for AGC,
> -	 * harmless without AGC)
> -	 */
> -	for (grp = 0; grp < ADC_LR_GROUP_MAX; grp++)
> -		regmap_set_bits(rk3308->regmap, RK3308_ADC_ANA_CON02(grp),
> -				RK3308_ADC_CH1_ZEROCROSS_DET_EN |
> -				RK3308_ADC_CH2_ZEROCROSS_DET_EN);
> -
>  	return 0;
>  }

[Severity: Medium]
Was removing the RK3308_ADC_CH1_ZEROCROSS_DET_EN |
RK3308_ADC_CH2_ZEROCROSS_DET_EN initialization from rk3308_codec_initialize()
intentional?

Nothing else in the driver sets RK3308_ADC_CH1_ZEROCROSS_DET_EN or
RK3308_ADC_CH2_ZEROCROSS_DET_EN after rk3308_codec_reset() zeroes
RK3308_ADC_ANA_CON02(grp). Consequently, zero-cross detection remains
disabled on all ADC channels, and the code in rk3308_codec_adc_event() that
saves, clears, and restores those bits during SND_SOC_DAPM_POST_PMD always
reads and restores 0.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261101044149.2018744-1-zhengxing@rock-chips.com?part=2