Re: [PATCH v3 2/3] ASoC: codecs: add new pm4125 audio codec driver

["Alexey Klimov" <alexey.klimov@linaro.org>]

Hi Srini,

On Fri Aug 15, 2025 at 4:36 PM BST, Srinivas Kandagatla wrote:
>

[..]

> I manged to test this one, found 2 issues.
>
> 1. incorrect mic bias handling, results in recording stop working.
> 2. memory corruption leading to kernel crash.
>
> More details below..
>
>> Co-developed-by: Srinivas Kandagatla <srinivas.kandagatla@oss.qualcomm.com>
>> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@oss.qualcomm.com>
>> Signed-off-by: Alexey Klimov <alexey.klimov@linaro.org>
>> ---
>>  sound/soc/codecs/Kconfig      |   18 +
>>  sound/soc/codecs/Makefile     |    8 +
>>  sound/soc/codecs/pm4125-sdw.c |  547 +++++++++++++
>>  sound/soc/codecs/pm4125.c     | 1793 +++++++++++++++++++++++++++++++++++++++++
>>  sound/soc/codecs/pm4125.h     |  313 +++++++
>>  5 files changed, 2679 insertions(+)

>> --- /dev/null
>> +++ b/sound/soc/codecs/pm4125-sdw.c
>> @@ -0,0 +1,547 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +// Copyright (c) 2023-2024 Qualcomm Innovation Center, Inc. All rights reserved.
>> +// Copyright, 2025 Linaro Ltd
>
>
>> +
>> +static struct pm4125_sdw_ch_info pm4125_sdw_rx_ch_info[] = {
>> +	WCD_SDW_CH(PM4125_HPH_L, PM4125_HPH_PORT, BIT(0)),
>> +	WCD_SDW_CH(PM4125_HPH_R, PM4125_HPH_PORT, BIT(1)),
>> +	WCD_SDW_CH(PM4125_COMP_L, PM4125_COMP_PORT, BIT(0)),
>> +	WCD_SDW_CH(PM4125_COMP_R, PM4125_COMP_PORT, BIT(1)),
> Issue 1: we are adding only 4 channels here however the mixer Switches
> that lookup this table is more than 4.
>> +};
>> +
>> +static struct pm4125_sdw_ch_info pm4125_sdw_tx_ch_info[] = {
>> +	WCD_SDW_CH(PM4125_ADC1, PM4125_ADC_1_2_DMIC1L_BCS_PORT, BIT(0)),
>> +	WCD_SDW_CH(PM4125_ADC2, PM4125_ADC_1_2_DMIC1L_BCS_PORT, BIT(1)),
>
> Same issue here,
>> +};
>> +

Okay. Let's fix it as you suggested.


>> diff --git a/sound/soc/codecs/pm4125.c b/sound/soc/codecs/pm4125.c
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..fc320152b9254e4412cbf593cdc456ee159d071f
>> --- /dev/null
>> +++ b/sound/soc/codecs/pm4125.c
>> @@ -0,0 +1,1793 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +// Copyright (c) 2023-2024 Qualcomm Innovation Center, Inc. All rights reserved.
>> +// Copyright (c) 2025, Linaro Ltd
>> +
>
>> +static int pm4125_rx_clk_enable(struct snd_soc_component *component)
>> +{
>> +	struct pm4125_priv *pm4125 = snd_soc_component_get_drvdata(component);
>> +
>> +	if (atomic_read(&pm4125->rx_clk_cnt))
>> +		return 0;
>> +
>> +	snd_soc_component_write_field(component, PM4125_DIG_SWR_CDC_RX_CLK_CTL,
>> +				      PM4125_DIG_SWR_ANA_RX_CLK_EN_MASK,
>> +				      PM4125_DIG_SWR_RX_CLK_ENABLE);
>> +	snd_soc_component_write_field(component, PM4125_DIG_SWR_CDC_RX_CLK_CTL,
>> +				      PM4125_DIG_SWR_ANA_RX_DIV2_CLK_EN_MASK,
>> +				      PM4125_DIG_SWR_RX_CLK_ENABLE);
>> +	usleep_range(5000, 5100);
>> +
>> +	pm4125_global_mbias_enable(component);
>
> Please remove handing of mbias calls directly this is racing, Please
> handle it via dapm widgets directly. If not we will endup with switching
> off micbias off while recording is in progress or recording will
> continue assuming that micbias is on, but some path can switch it off.

Please see below, there are problems with playback without pm4125_global_mbias_enable().

>> +
>> +	snd_soc_component_write_field(component, PM4125_ANA_HPHPA_FSM_CLK,
>> +				      PM4125_ANA_HPHPA_FSM_DIV_RATIO_MASK,
>> +				      PM4125_ANA_HPHPA_FSM_DIV_RATIO_68);
>> +	snd_soc_component_write_field(component, PM4125_ANA_HPHPA_FSM_CLK,
>> +				      PM4125_ANA_HPHPA_FSM_CLK_DIV_EN_MASK,
>> +				      PM4125_ANA_HPHPA_FSM_CLK_DIV_ENABLE);
>> +	snd_soc_component_update_bits(component, PM4125_ANA_NCP_VCTRL, 0x07, 0x06);
>> +	snd_soc_component_write_field(component, PM4125_ANA_NCP_EN,
>> +				      PM4125_ANA_NCP_ENABLE_MASK,
>> +				      PM4125_ANA_NCP_ENABLE);
>> +	usleep_range(500, 510);
>> +
>> +	atomic_inc(&pm4125->rx_clk_cnt);
>> +
>> +	return 0;
>> +}
>> +
>> +static int pm4125_rx_clk_disable(struct snd_soc_component *component)
>> +{
>> +	struct pm4125_priv *pm4125 = snd_soc_component_get_drvdata(component);
>> +
>> +	if (!atomic_read(&pm4125->rx_clk_cnt)) {
>> +		dev_err(component->dev, "clk already disabled\n");
>> +		return 0;
>> +	}
>> +
>> +	atomic_dec(&pm4125->rx_clk_cnt);
>> +
>> +	snd_soc_component_write_field(component, PM4125_ANA_HPHPA_FSM_CLK,
>> +				      PM4125_ANA_HPHPA_FSM_CLK_DIV_EN_MASK,
>> +				      PM4125_ANA_HPHPA_FSM_CLK_DIV_DISABLE);
>> +	snd_soc_component_write_field(component, PM4125_ANA_HPHPA_FSM_CLK,
>> +				      PM4125_ANA_HPHPA_FSM_DIV_RATIO_MASK,
>> +				      0x00);
>> +	snd_soc_component_write_field(component, PM4125_ANA_NCP_EN,
>> +				      PM4125_ANA_NCP_ENABLE_MASK,
>> +				      PM4125_ANA_NCP_DISABLE);
>> +	snd_soc_component_write_field(component, PM4125_DIG_SWR_CDC_RX_CLK_CTL,
>> +				      PM4125_DIG_SWR_ANA_RX_DIV2_CLK_EN_MASK,
>> +				      PM4125_DIG_SWR_RX_CLK_DISABLE);
>> +	snd_soc_component_write_field(component, PM4125_DIG_SWR_CDC_RX_CLK_CTL,
>> +				      PM4125_DIG_SWR_ANA_RX_CLK_EN_MASK,
>> +				      PM4125_DIG_SWR_RX_CLK_DISABLE);
>> +	pm4125_global_mbias_disable(component);
>> +
>> +	return 0;
>> +}
>> +
>> +
>
> %s/^\n\n/\r/

s/^/Ok\n/

>> +static int pm4125_codec_enable_rxclk(struct snd_soc_dapm_widget *w, struct snd_kcontrol *kcontrol,
>> +				     int event)
>> +{
>> +	struct snd_soc_component *component = snd_soc_dapm_to_component(w->dapm);
>> +
>> +	switch (event) {
>> +	case SND_SOC_DAPM_PRE_PMU:
>> 
>> +static const struct snd_kcontrol_new pm4125_snd_controls[] = {
>> +	SOC_SINGLE_EXT("HPHL_COMP Switch", SND_SOC_NOPM, 0, 1, 0,
>
>     SOC_SINGLE_EXT("HPHL_COMP Switch", PM4125_COMP_L, 0, 1, 0, ?
>
>> +		       pm4125_get_compander, pm4125_set_compander),
>> +	SOC_SINGLE_EXT("HPHR_COMP Switch", SND_SOC_NOPM, 1, 1, 0,
>
>        SOC_SINGLE_EXT("HPHR_COMP Switch", PM4125_COMP_R, 1, 1, 0,?
>
>> +		       pm4125_get_compander, pm4125_set_compander),
>
> This is same issue in one of the WCD codec, which am going to send fixes
> along with my original wcd fixes series.

So this was in other codecs for years, right?

>> +
>> +	SOC_SINGLE_TLV("HPHL Volume", PM4125_ANA_HPHPA_L_GAIN, 0, 20, 1,
>> +		       line_gain),
>> +	SOC_SINGLE_TLV("HPHR Volume", PM4125_ANA_HPHPA_R_GAIN, 0, 20, 1,
>> +		       line_gain),
>> +	SOC_SINGLE_TLV("ADC1 Volume", PM4125_ANA_TX_AMIC1, 0, 8, 0,
>> +		       analog_gain),
>> +	SOC_SINGLE_TLV("ADC2 Volume", PM4125_ANA_TX_AMIC2, 0, 8, 0,
>> +		       analog_gain),
>> +
>> +	SOC_SINGLE_EXT("HPHL Switch", PM4125_HPH_L, 0, 1, 0,
>> +		       pm4125_get_swr_port, pm4125_set_swr_port),
>> +	SOC_SINGLE_EXT("HPHR Switch", PM4125_HPH_R, 0, 1, 0,
>> +		       pm4125_get_swr_port, pm4125_set_swr_port),
>
> <---
>> +	SOC_SINGLE_EXT("LO Switch", PM4125_LO, 0, 1, 0,
>> +		       pm4125_get_swr_port, pm4125_set_swr_port),--->
>
>> +
>> +	SOC_SINGLE_EXT("ADC1 Switch", PM4125_ADC1, 1, 1, 0,
>> +		       pm4125_get_swr_port, pm4125_set_swr_port),
>> +	SOC_SINGLE_EXT("ADC2 Switch", PM4125_ADC2, 1, 1, 0,
>> +		       pm4125_get_swr_port, pm4125_set_swr_port),
> <-----------------
>> +	SOC_SINGLE_EXT("DMIC0 Switch", PM4125_DMIC0, 1, 1, 0,
>> +		       pm4125_get_swr_port, pm4125_set_swr_port),
>> +	SOC_SINGLE_EXT("DMIC1 Switch", PM4125_DMIC1, 1, 1, 0,
>> +		       pm4125_get_swr_port, pm4125_set_swr_port),
>> +	SOC_SINGLE_EXT("MBHC Switch", PM4125_MBHC, 1, 1, 0,
>> +		       pm4125_get_swr_port, pm4125_set_swr_port),
>> +	SOC_SINGLE_EXT("DMIC2 Switch", PM4125_DMIC2, 1, 1, 0,
>> +		       pm4125_get_swr_port, pm4125_set_swr_port),
> -------------->
> Please delete these entires as there are no entires for any of this
> channels in pm4125_sdw_rx_ch_info or pm4125_sdw_tx_ch_info.
>
> Side effect of this out of boundary array access is memory corruption as
> we will set port_config values based on port index which could be
> negative in this cases resulting in writing to othe members of
> pm4125_sdw_priv struct.


Ack. I applied your changes that you suggested.


>> +};
>> +
>> +static const struct snd_kcontrol_new adc1_switch[] = {
>> +	SOC_DAPM_SINGLE("Switch", SND_SOC_NOPM, 0, 1, 0)
>> +};
>> +
>> +static const struct snd_kcontrol_new adc2_switch[] = {
>> +	SOC_DAPM_SINGLE("Switch", SND_SOC_NOPM, 0, 1, 0)
>> +};
>> +
>> +static const struct snd_kcontrol_new dmic1_switch[] = {
>> +	SOC_DAPM_SINGLE("Switch", SND_SOC_NOPM, 0, 1, 0)
>> +};
>> +
>> +static const struct snd_kcontrol_new dmic2_switch[] = {
>> +	SOC_DAPM_SINGLE("Switch", SND_SOC_NOPM, 0, 1, 0)
>> +};
>> +
>> +static const struct snd_kcontrol_new ear_rdac_switch[] = {
>> +	SOC_DAPM_SINGLE("Switch", SND_SOC_NOPM, 0, 1, 0)
>> +};
>> +
>
> during my test i had to do below code changes to get things working.
> Please feel free to wrap it into your next version.
>
> ----------------------->cut<----------------------------------
> diff --git a/sound/soc/codecs/pm4125.c b/sound/soc/codecs/pm4125.c
> index fc320152b925..12d4be1f7149 100644
> --- a/sound/soc/codecs/pm4125.c
> +++ b/sound/soc/codecs/pm4125.c
> @@ -264,8 +264,6 @@ static int pm4125_rx_clk_enable(struct
> snd_soc_component *component)
>                                       PM4125_DIG_SWR_RX_CLK_ENABLE);
>         usleep_range(5000, 5100);
>
> -       pm4125_global_mbias_enable(component);
> -
>         snd_soc_component_write_field(component, PM4125_ANA_HPHPA_FSM_CLK,
>                                       PM4125_ANA_HPHPA_FSM_DIV_RATIO_MASK,
>                                       PM4125_ANA_HPHPA_FSM_DIV_RATIO_68);
> @@ -309,8 +307,6 @@ static int pm4125_rx_clk_disable(struct
> snd_soc_component *component)
>         snd_soc_component_write_field(component,
> PM4125_DIG_SWR_CDC_RX_CLK_CTL,
>                                       PM4125_DIG_SWR_ANA_RX_CLK_EN_MASK,
>                                       PM4125_DIG_SWR_RX_CLK_DISABLE);
> -       pm4125_global_mbias_disable(component);
> -
>         return 0;
>  }

This doesn't work. Playback has two issues: 1) volume is very low and probably
not adjustable and 2) sound during playback dies after a couple of seconds.

Returning these global_mbias() calls restores the good behaviour.

Maybe let's make a widget out of it? In such case I am not sure about
routing meaning that I not sure which paths do require mbias enable.

Best regards,
Alexey