[PATCH 0/4] ASoC: codecs: pm4125: Enable MBHC jack detection

[PATCH 0/4] ASoC: codecs: pm4125: Enable MBHC jack detection

[Mohammad Rafi Shaik]

The pm4125 codec registers with the WCD MBHC framework, but it only
provides the electrical bias and button threshold callbacks and leaves the
rest of struct wcd_mbhc_cb empty. The framework can fall back to driving
some of the shared register fields itself, but it has no fallback for the
ones that control hardware it cannot describe generically, so ADC based
plug type detection runs without the MBHC RCO clock, without a micbias
ramp and without the headset ground detection comparator, and never gets a
usable ADC reading. On top of that, the one threshold path that does exist
programs the register wrongly, and a reference counting bug in the micbias
code turns a jack insertion during playback into silence.

This series fixes those issues and fills in the missing callbacks so a
3.5mm jack can be detected, together with the binding needed to describe
one.

Signed-off-by: Mohammad Rafi Shaik <mohammad.rafi.shaik@oss.qualcomm.com>

---
Mohammad Rafi Shaik (4):
      ASoC: dt-bindings: qcom,pm4125-codec: Add the MBHC jack properties
      ASoC: codecs: pm4125: Fix MBHC button threshold programming
      ASoC: codecs: pm4125: Keep the micbias reference count balanced
      ASoC: codecs: pm4125: Add the MBHC callbacks needed for jack detection

 .../bindings/sound/qcom,pm4125-codec.yaml          |  18 ++
 sound/soc/codecs/pm4125.c                          | 206 ++++++++++++++++++++-
 sound/soc/codecs/pm4125.h                          |  13 ++
 3 files changed, 233 insertions(+), 4 deletions(-)
---
base-commit: 686b3eff603871bd500434e00ff15394a0c782bc
change-id: 20261015-pm4125_mbhc_support-42fbbccd2b51

Best regards,
--  
Mohammad Rafi Shaik <mohammad.rafi.shaik@oss.qualcomm.com>

[PATCH v1 1/4] ASoC: dt-bindings: qcom,pm4125-codec: Add the MBHC jack properties

[Mohammad Rafi Shaik]

The binding already describes qcom,mbhc-buttons-vthreshold-microvolt,
but not the rest of the data the MBHC block needs to detect a 3.5mm
jack, even though wcd_dt_parse_mbhc_data() reads all of it from the
codec node and the schema has unevaluatedProperties set to false. A
board that describes its jack is therefore rejected by the schema.

Document the remaining four properties, matching the descriptions used
for the same properties in qcom,wcd93xx-common.yaml:

 - qcom,hphl-jack-type-normally-closed and
   qcom,ground-jack-type-normally-closed describe the board's jack
   switches. The MBHC logic has to know the idle state of the HPHL and
   ground switches to tell an insertion from a removal.
 - qcom,mbhc-headset-vthreshold-microvolt and
   qcom,mbhc-headphone-vthreshold-microvolt are the ADC thresholds that
   separate a headset from a headphone and a headphone from a floating
   jack. They are board specific because they depend on the jack
   wiring and on the microphone bias voltage they are measured against.

All four stay optional, as they are for the wcd93xx codecs: the driver
derives the thresholds from the microphone bias voltage when they are
absent and assumes normally open switches, so existing device trees
keep working unchanged.

Assisted-by: Claude Opus 5
Signed-off-by: Mohammad Rafi Shaik <mohammad.rafi.shaik@oss.qualcomm.com>
---
 .../devicetree/bindings/sound/qcom,pm4125-codec.yaml   | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/Documentation/devicetree/bindings/sound/qcom,pm4125-codec.yaml b/Documentation/devicetree/bindings/sound/qcom,pm4125-codec.yaml
index 6e2f103be..54e5b7607 100644
--- a/Documentation/devicetree/bindings/sound/qcom,pm4125-codec.yaml
+++ b/Documentation/devicetree/bindings/sound/qcom,pm4125-codec.yaml
@@ -64,6 +64,24 @@ properties:
     minimum: 1800000
     maximum: 2850000
 
+  qcom,hphl-jack-type-normally-closed:
+    description: Indicates that HPHL jack switch type is normally closed
+    type: boolean
+
+  qcom,ground-jack-type-normally-closed:
+    description: Indicates that Headset Ground switch type is normally closed
+    type: boolean
+
+  qcom,mbhc-headset-vthreshold-microvolt:
+    description: Voltage threshold value for headset detection
+    minimum: 0
+    maximum: 2850000
+
+  qcom,mbhc-headphone-vthreshold-microvolt:
+    description: Voltage threshold value for headphone detection
+    minimum: 0
+    maximum: 2850000
+
   qcom,mbhc-buttons-vthreshold-microvolt:
     description:
       Array of 8 Voltage threshold values corresponding to headset

-- 
2.34.1

Re: [PATCH v1 1/4] ASoC: dt-bindings: qcom,pm4125-codec: Add the MBHC jack properties

[Ajay Kumar Nandam]

On 10/15/2026 7:43 PM, Mohammad Rafi Shaik wrote:
> The binding already describes qcom,mbhc-buttons-vthreshold-microvolt,
> but not the rest of the data the MBHC block needs to detect a 3.5mm
> jack, even though wcd_dt_parse_mbhc_data() reads all of it from the
> codec node and the schema has unevaluatedProperties set to false. A
> board that describes its jack is therefore rejected by the schema.
> 
> Document the remaining four properties, matching the descriptions used
> for the same properties in qcom,wcd93xx-common.yaml:
> 
>   - qcom,hphl-jack-type-normally-closed and
>     qcom,ground-jack-type-normally-closed describe the board's jack
>     switches. The MBHC logic has to know the idle state of the HPHL and
>     ground switches to tell an insertion from a removal.
>   - qcom,mbhc-headset-vthreshold-microvolt and
>     qcom,mbhc-headphone-vthreshold-microvolt are the ADC thresholds that
>     separate a headset from a headphone and a headphone from a floating
>     jack. They are board specific because they depend on the jack
>     wiring and on the microphone bias voltage they are measured against.
> 
> All four stay optional, as they are for the wcd93xx codecs: the driver
> derives the thresholds from the microphone bias voltage when they are
> absent and assumes normally open switches, so existing device trees
> keep working unchanged.
> 
> Assisted-by: Claude Opus 5
> Signed-off-by: Mohammad Rafi Shaik <mohammad.rafi.shaik@oss.qualcomm.com>
> ---
>   .../devicetree/bindings/sound/qcom,pm4125-codec.yaml   | 18 ++++++++++++++++++
>   1 file changed, 18 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/sound/qcom,pm4125-codec.yaml b/Documentation/devicetree/bindings/sound/qcom,pm4125-codec.yaml
> index 6e2f103be..54e5b7607 100644
> --- a/Documentation/devicetree/bindings/sound/qcom,pm4125-codec.yaml
> +++ b/Documentation/devicetree/bindings/sound/qcom,pm4125-codec.yaml
> @@ -64,6 +64,24 @@ properties:
>       minimum: 1800000
>       maximum: 2850000
>   
> +  qcom,hphl-jack-type-normally-closed:
> +    description: Indicates that HPHL jack switch type is normally closed
> +    type: boolean
> +
> +  qcom,ground-jack-type-normally-closed:
> +    description: Indicates that Headset Ground switch type is normally closed
> +    type: boolean
> +
> +  qcom,mbhc-headset-vthreshold-microvolt:
> +    description: Voltage threshold value for headset detection
> +    minimum: 0
> +    maximum: 2850000
> +
> +  qcom,mbhc-headphone-vthreshold-microvolt:
> +    description: Voltage threshold value for headphone detection
> +    minimum: 0
> +    maximum: 2850000
> +
>     qcom,mbhc-buttons-vthreshold-microvolt:
>       description:
>         Array of 8 Voltage threshold values corresponding to headset

The binding documents 8 button thresholds, but looking at the PM4125
register definitions I only see threshold registers for BTN0 through 
BTN4. The following registers are MICB2_RAMP, MBHC_CTL_1 and MBHC_CTL_2.

Am I missing how thresholds for buttons 5-7 are programmed here, or 
should PM4125 only expose 5 button thresholds?

Thanks & Regards
Ajay kumar Nandam

> 

[PATCH v1 2/4] ASoC: codecs: pm4125: Fix MBHC button threshold programming

[Mohammad Rafi Shaik]

pm4125_mbhc_program_btn_thr() pre-shifts the computed threshold by two
bits before handing it to snd_soc_component_write_field(). That helper
already shifts the value by the number of trailing zeroes of the mask,
so with PM4125_ANA_MBHC_BTN0_THRESHOLD_MASK being GENMASK(7, 2) the
value gets shifted twice.

The two most significant bits of the threshold are shifted out past the
mask and the remaining bits land four times too high, so every button
voltage is programmed wrong and button presses on a headset are either
missed or reported as the wrong button.

Pass the unshifted value and let the helper place it in the field.

Fixes: 8ad529484937 ("ASoC: codecs: add new pm4125 audio codec driver")
Assisted-by: Claude Opus 5
Signed-off-by: Mohammad Rafi Shaik <mohammad.rafi.shaik@oss.qualcomm.com>
---
 sound/soc/codecs/pm4125.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/soc/codecs/pm4125.c b/sound/soc/codecs/pm4125.c
index 674a2fbc4..923318c78 100644
--- a/sound/soc/codecs/pm4125.c
+++ b/sound/soc/codecs/pm4125.c
@@ -981,7 +981,7 @@ static void pm4125_mbhc_program_btn_thr(struct snd_soc_component *component,
 	for (i = 0; i < num_btn; i++) {
 		vth = ((btn_high[i] * 2) / 25) & 0x3F;
 		snd_soc_component_write_field(component, PM4125_ANA_MBHC_BTN0_ZDET_VREF1 + i,
-					      PM4125_ANA_MBHC_BTN0_THRESHOLD_MASK, vth << 2);
+					      PM4125_ANA_MBHC_BTN0_THRESHOLD_MASK, vth);
 	}
 }
 

-- 
2.34.1

Re: [PATCH v1 2/4] ASoC: codecs: pm4125: Fix MBHC button threshold programming

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] pm4125: out-of-bounds register write in pm4125_mbhc_program_btn_thr()
--

commit 2cce30fa45a4f0c5ff81fb48ecba9f5d3b9f5681
Author: Mohammad Rafi Shaik <mohammad.rafi.shaik@oss.qualcomm.com>

ASoC: codecs: pm4125: Fix MBHC button threshold programming

This commit fixes MBHC button threshold programming by removing an
incorrect double shift of the threshold value before passing it to
snd_soc_component_write_field().

> diff --git a/sound/soc/codecs/pm4125.c b/sound/soc/codecs/pm4125.c
> index 674a2fbc4edae..923318c78d261 100644
> --- a/sound/soc/codecs/pm4125.c
> +++ b/sound/soc/codecs/pm4125.c
> @@ -981,7 +981,7 @@ static void pm4125_mbhc_program_btn_thr(struct snd_soc_component *component,
>  	for (i = 0; i < num_btn; i++) {
>  		vth = ((btn_high[i] * 2) / 25) & 0x3F;
>  		snd_soc_component_write_field(component, PM4125_ANA_MBHC_BTN0_ZDET_VREF1 + i,
> -					      PM4125_ANA_MBHC_BTN0_THRESHOLD_MASK, vth << 2);
> +					      PM4125_ANA_MBHC_BTN0_THRESHOLD_MASK, vth);

[Severity: High]
Does this code write past the end of the button threshold registers?

The pm4125_probe() function sets cfg->num_btn = PM4125_MBHC_MAX_BUTTONS
(which is defined as 8). However, the PM4125 codec only has 5 button
threshold registers:

sound/soc/codecs/pm4125.h:
#define PM4125_ANA_MBHC_BTN0_ZDET_VREF1         (PM4125_ANA_BASE_ADDR+0x060)
...
#define PM4125_ANA_MBHC_BTN4_ZDET_DBG_1400      (PM4125_ANA_BASE_ADDR+0x064)
#define PM4125_ANA_MBHC_MICB2_RAMP              (PM4125_ANA_BASE_ADDR+0x065)
#define PM4125_ANA_MBHC_CTL_1                   (PM4125_ANA_BASE_ADDR+0x066)

Because the loop iterates 8 times, it writes threshold voltages into offsets
0x065, 0x066, and 0x067. Can this silently corrupt the adjacent
PM4125_ANA_MBHC_MICB2_RAMP, PM4125_ANA_MBHC_CTL_1, and
PM4125_ANA_MBHC_CTL_2 registers?

>  	}
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261015-pm4125_mbhc_support-v1-0-8bc1212db6f7@oss.qualcomm.com?part=2

[PATCH v1 3/4] ASoC: codecs: pm4125: Keep the micbias reference count balanced

[Mohammad Rafi Shaik]

The analog master bias is shared between the microphone bias supplies
and the playback and capture paths, so pm4125_global_mbias_enable() and
pm4125_global_mbias_disable() reference count it and only touch
PM4125_ANA_MBIAS_EN for the first user and the last one.

MICB_DISABLE clamps the per-supply reference count at zero but then
evaluates the enable and pull-up counts regardless, so a release that
has no matching request still reaches pm4125_global_mbias_disable() and
drops a reference it never took. The MBHC framework issues exactly such
a release: it takes MIC_BIAS_2 once when it starts detecting a plug
type and releases it both at the end of the correction work and again
from wcd_micbias_disable(). Inserting a jack while a stream is running
therefore switches the analog master bias off underneath it, leaving
the path up but silent, and it stays silent until another stream takes
the first reference again.

Return early when there is nothing to release, so the hardware is only
touched for a request that was actually made.

While at it, use atomic_dec_if_positive() for the global count.
atomic_dec_and_test() lets it go negative, and once it is negative the
next enable increments it to zero instead of one and never re-arms the
bias at all.

Assisted-by: Claude Opus 5
Signed-off-by: Mohammad Rafi Shaik <mohammad.rafi.shaik@oss.qualcomm.com>
---
 sound/soc/codecs/pm4125.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/sound/soc/codecs/pm4125.c b/sound/soc/codecs/pm4125.c
index 923318c78..2100f4e36 100644
--- a/sound/soc/codecs/pm4125.c
+++ b/sound/soc/codecs/pm4125.c
@@ -229,7 +229,7 @@ static int pm4125_global_mbias_disable(struct snd_soc_component *component)
 {
 	struct pm4125_priv *pm4125 = snd_soc_component_get_drvdata(component);
 
-	if (atomic_dec_and_test(&pm4125->gloal_mbias_cnt)) {
+	if (atomic_dec_if_positive(&pm4125->gloal_mbias_cnt) == 0) {
 
 		snd_soc_component_write_field(component, PM4125_ANA_MBIAS_EN,
 					      PM4125_ANA_MBIAS_EN_V2I_MASK,
@@ -772,8 +772,18 @@ static int pm4125_micbias_control(struct snd_soc_component *component, int micb_
 		}
 		break;
 	case MICB_DISABLE:
-		if (pm4125->micb_ref[micb_index] > 0)
-			pm4125->micb_ref[micb_index]--;
+		/*
+		 * Ignore a release that has no matching request. The MBHC
+		 * framework releases MIC_BIAS_2 once when the plug type
+		 * correction work ends and again when it turns the bias off,
+		 * and honouring the second one would drop a reference on the
+		 * shared global mbias that was never taken, switching the
+		 * analog master bias off under an active stream.
+		 */
+		if (!pm4125->micb_ref[micb_index])
+			break;
+
+		pm4125->micb_ref[micb_index]--;
 		if ((pm4125->micb_ref[micb_index] == 0) &&
 		    (pm4125->pullup_ref[micb_index] > 0)) {
 			snd_soc_component_update_bits(component, micb_reg,

-- 
2.34.1

Re: [PATCH v1 3/4] ASoC: codecs: pm4125: Keep the micbias reference count balanced

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] ASoC: pm4125: data races in micbias control from MBHC workqueues
--

commit 8af5811d67f1f26cf2cd472e51e4bbe2a10e4f6f
Author: Mohammad Rafi Shaik <mohammad.rafi.shaik@oss.qualcomm.com>

ASoC: codecs: pm4125: Keep the micbias reference count balanced

This patch updates the pm4125 microphone bias handling to prevent
unbalanced reference drops from the MBHC framework. It changes the global
bias disable to use atomic_dec_if_positive() and ignores micbias releases
that do not have matching requests.

> diff --git a/sound/soc/codecs/pm4125.c b/sound/soc/codecs/pm4125.c
> --- a/sound/soc/codecs/pm4125.c
> +++ b/sound/soc/codecs/pm4125.c
> @@ -229,7 +229,7 @@ static int pm4125_global_mbias_disable(struct snd_soc_component *component)
>  {
>  	struct pm4125_priv *pm4125 = snd_soc_component_get_drvdata(component);
>  
> -	if (atomic_dec_and_test(&pm4125->gloal_mbias_cnt)) {
> +	if (atomic_dec_if_positive(&pm4125->gloal_mbias_cnt) == 0) {
>  
>  		snd_soc_component_write_field(component, PM4125_ANA_MBIAS_EN,
>  					      PM4125_ANA_MBIAS_EN_V2I_MASK,

[Severity: Medium]
Is there a race condition here in pm4125_global_mbias_disable() between the
atomic decrement and the hardware register updates?

If a racing enable thread increments gloal_mbias_cnt to 1 and enables the
hardware between this thread's atomic_dec_if_positive() and the subsequent
snd_soc_component_write_field() calls, could the hardware bias be left
disabled while the count is 1? 

This sequence might silently break the active audio path by tearing down
the mic bias underneath it.

[ ... ]

> @@ -772,8 +772,18 @@ static int pm4125_micbias_control(struct snd_soc_component *component, int micb_
>  		}
>  		break;
>  	case MICB_DISABLE:
> -		if (pm4125->micb_ref[micb_index] > 0)
> -			pm4125->micb_ref[micb_index]--;
> +		/*
> +		 * Ignore a release that has no matching request. The MBHC
> +		 * framework releases MIC_BIAS_2 once when the plug type
> +		 * correction work ends and again when it turns the bias off,
> +		 * and honouring the second one would drop a reference on the
> +		 * shared global mbias that was never taken, switching the
> +		 * analog master bias off under an active stream.
> +		 */
> +		if (!pm4125->micb_ref[micb_index])
> +			break;
> +
> +		pm4125->micb_ref[micb_index]--;

[Severity: Medium]
Could this lead to data corruption of the reference tracking arrays?

The micb_ref array is evaluated and decremented non-atomically without a
local lock in pm4125_micbias_control(). Because the MBHC framework executes
from asynchronous workqueues (like mbhc_plug_detect_fn), and DAPM widget
events can run concurrently, inserting a jack during stream playback might
cause these threads to race on the micb_ref array updates.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261015-pm4125_mbhc_support-v1-0-8bc1212db6f7@oss.qualcomm.com?part=3

Re: [PATCH v1 3/4] ASoC: codecs: pm4125: Keep the micbias reference count balanced

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1353 bytes --]

On Thu, Oct 15, 2026 at 07:43:11PM +0530, Mohammad Rafi Shaik wrote:

> The analog master bias is shared between the microphone bias supplies
> and the playback and capture paths, so pm4125_global_mbias_enable() and
> pm4125_global_mbias_disable() reference count it and only touch
> PM4125_ANA_MBIAS_EN for the first user and the last one.

> MICB_DISABLE clamps the per-supply reference count at zero but then
> evaluates the enable and pull-up counts regardless, so a release that
> has no matching request still reaches pm4125_global_mbias_disable() and
> drops a reference it never took. The MBHC framework issues exactly such
> a release: it takes MIC_BIAS_2 once when it starts detecting a plug
> type and releases it both at the end of the correction work and again
> from wcd_micbias_disable(). Inserting a jack while a stream is running
> therefore switches the analog master bias off underneath it, leaving
> the path up but silent, and it stays silent until another stream takes
> the first reference again.
> 
> Return early when there is nothing to release, so the hardware is only
> touched for a request that was actually made.

This feels like a bodge for poor reference counting elsewhere - in
particular the MBHC seems to be fairly clearly double freeing.  The fix
here just silently eats double frees AFAICT?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH v1 4/4] ASoC: codecs: pm4125: Add the MBHC callbacks needed for jack detection

[Mohammad Rafi Shaik]

The codec only hands the MBHC framework the electrical bias and button
threshold callbacks, and leaves the rest of struct wcd_mbhc_cb empty.
The framework falls back to driving the shared register fields itself
for some of them, but it has no fallback at all for the ones that
control hardware it cannot describe generically, so plug type detection
runs without the RCO clock, without a micbias ramp and without the
headset ground detection comparator, and never gets a usable ADC
reading.

Add the callbacks the ADC based detection logic needs:

 - clk_setup() enables the MBHC RCO, which clocks the detection FSM.
 - micbias_enable_status() lets the framework tell whether MIC_BIAS_2
   is already up for recording, so it does not switch the button
   current source on underneath a capture stream.
 - hph_pull_up_control() selects the HPHL pull up current used while
   sensing an insertion, defaulting to 3uA as the hardware expects.
 - mbhc_micbias_control() routes the framework's MIC_BIAS_2 requests
   through pm4125_micbias_control() so they share the reference count
   with the DAPM micbias widgets and with the analog master bias.
 - mbhc_micb_ramp_control() ramps MIC_BIAS_2 instead of stepping it,
   which is required before the cross connection check reads the
   Schmitt trigger outputs.
 - mbhc_micb_ctrl_thr_mic() raises MIC_BIAS_2 to the threshold needed
   to tell a special headset from an extension cable, and puts it back
   afterwards.
 - get_micbias_val() reports the programmed MIC_BIAS_2 voltage. The
   generic path assumes a 1V base, while this codec starts at 1.6V in
   50mV steps, so the framework would scale every threshold wrongly.
 - mbhc_gnd_det_ctrl() enables the headset ground pull up comparator
   together with ground detection, in that order on the way up and the
   reverse on the way down.
 - hph_pull_down_ctrl() parks the headphone outputs in a pull down
   while no jack is being driven.

pm4125_mbhc_micb_adjust_voltage() changes MIC_BIAS_2 while it is
enabled, which has to be done with the output briefly pulled up, so
serialise it against concurrent requests with a mutex.

Assisted-by: Claude Opus 5
Signed-off-by: Mohammad Rafi Shaik <mohammad.rafi.shaik@oss.qualcomm.com>
---
 sound/soc/codecs/pm4125.c | 188 ++++++++++++++++++++++++++++++++++++++++++++++
 sound/soc/codecs/pm4125.h |  13 ++++
 2 files changed, 201 insertions(+)

diff --git a/sound/soc/codecs/pm4125.c b/sound/soc/codecs/pm4125.c
index 2100f4e36..6dd5ce0f4 100644
--- a/sound/soc/codecs/pm4125.c
+++ b/sound/soc/codecs/pm4125.c
@@ -8,6 +8,7 @@
 #include <linux/irqdomain.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
+#include <linux/mutex.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
@@ -76,6 +77,8 @@ struct pm4125_priv {
 	struct regmap_irq_chip_data *irq_chip;
 	struct snd_soc_jack *jack;
 	unsigned long status_mask;
+	/* Protects micbias voltage adjustment against concurrent requests */
+	struct mutex micb_lock;
 	s32 micb_ref[PM4125_MAX_MICBIAS];
 	s32 pullup_ref[PM4125_MAX_MICBIAS];
 
@@ -968,6 +971,12 @@ static int pm4125_set_swr_port(struct snd_kcontrol *kcontrol, struct snd_ctl_ele
 	return 1;
 }
 
+static void pm4125_mbhc_clk_setup(struct snd_soc_component *component, bool enable)
+{
+	snd_soc_component_write_field(component, PM4125_ANA_MBHC_CTL_1,
+				      PM4125_ANA_MBHC_CTL_RCO_EN_MASK, enable);
+}
+
 static void pm4125_mbhc_bias_control(struct snd_soc_component *component, bool enable)
 {
 	snd_soc_component_write_field(component, PM4125_ANA_MBHC_ELECT,
@@ -995,9 +1004,187 @@ static void pm4125_mbhc_program_btn_thr(struct snd_soc_component *component,
 	}
 }
 
+static bool pm4125_mbhc_micb_en_status(struct snd_soc_component *component, int micb_num)
+{
+	if (micb_num != MIC_BIAS_2)
+		return false;
+
+	return snd_soc_component_read_field(component, PM4125_ANA_MICBIAS_MICB_1_2_EN,
+					    PM4125_ANA_MICBIAS_MICB2_EN_MASK) == 1;
+}
+
+static void pm4125_mbhc_hph_l_pull_up_control(struct snd_soc_component *component,
+					      enum mbhc_hs_pullup_iref pull_up_cur)
+{
+	/* Default pull up current to 3uA */
+	if (pull_up_cur < I_OFF || pull_up_cur > I_3P0_UA || pull_up_cur == I_DEFAULT)
+		pull_up_cur = I_3P0_UA;
+
+	snd_soc_component_write_field(component, PM4125_ANA_MBHC_PLUG_DETECT_CTL,
+				      PM4125_ANA_MBHC_HSDET_PULLUP_C_MASK, pull_up_cur);
+}
+
+static int pm4125_mbhc_request_micbias(struct snd_soc_component *component,
+				       int micb_num, int req)
+{
+	return pm4125_micbias_control(component, micb_num, req, false);
+}
+
+static void pm4125_mbhc_micb_ramp_control(struct snd_soc_component *component, bool enable)
+{
+	if (enable) {
+		snd_soc_component_write_field(component, PM4125_ANA_MBHC_MICB2_RAMP,
+					      PM4125_ANA_MBHC_MICB2_RAMP_SHIFT_MASK,
+					      PM4125_ANA_MBHC_MICB2_RAMP_SHIFT_CTRL);
+		snd_soc_component_write_field(component, PM4125_ANA_MBHC_MICB2_RAMP,
+					      PM4125_ANA_MBHC_MICB2_RAMP_EN_MASK, 1);
+	} else {
+		snd_soc_component_write_field(component, PM4125_ANA_MBHC_MICB2_RAMP,
+					      PM4125_ANA_MBHC_MICB2_RAMP_EN_MASK, 0);
+		snd_soc_component_write_field(component, PM4125_ANA_MBHC_MICB2_RAMP,
+					      PM4125_ANA_MBHC_MICB2_RAMP_SHIFT_MASK, 0);
+	}
+}
+
+static int pm4125_mbhc_micb_adjust_voltage(struct snd_soc_component *component,
+					   int req_volt, int micb_num)
+{
+	struct pm4125_priv *pm4125 = snd_soc_component_get_drvdata(component);
+	int cur_vout_ctl, req_vout_ctl, micb_en, ret = 0;
+	unsigned int pullup_mask, en_mask;
+
+	switch (micb_num) {
+	case MIC_BIAS_1:
+		pullup_mask = PM4125_ANA_MICBIAS_MICB1_PULL_UP_MASK;
+		en_mask = PM4125_ANA_MICBIAS_MICB1_EN_MASK;
+		break;
+	case MIC_BIAS_2:
+		pullup_mask = PM4125_ANA_MICBIAS_MICB2_PULL_UP_MASK;
+		en_mask = PM4125_ANA_MICBIAS_MICB2_EN_MASK;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	mutex_lock(&pm4125->micb_lock);
+
+	/*
+	 * If requested micbias voltage is same as current micbias voltage,
+	 * then just return. Otherwise, adjust voltage as per requested value.
+	 * If micbias is already enabled, then to avoid slow micbias ramp-up
+	 * or down enable pull-up momentarily, change the micbias value and
+	 * then re-enable micbias.
+	 */
+	micb_en = snd_soc_component_read_field(component, PM4125_ANA_MICBIAS_MICB_1_2_EN,
+					       en_mask);
+	cur_vout_ctl = snd_soc_component_read_field(component, PM4125_ANA_MICBIAS_LDO_1_SETTING,
+						    PM4125_ANA_MICBIAS_MICB_OUT_VAL_MASK);
+
+	req_vout_ctl = pm4125_get_micb_vout_ctl_val(component->dev, req_volt);
+	if (req_vout_ctl < 0) {
+		ret = -EINVAL;
+		goto exit;
+	}
+
+	if (cur_vout_ctl == req_vout_ctl)
+		goto exit;
+
+	if (micb_en == 1)
+		snd_soc_component_write_field(component, PM4125_ANA_MICBIAS_MICB_1_2_EN,
+					      pullup_mask,
+					      PM4125_ANA_MICBIAS_MICB_PULL_ENABLE);
+
+	snd_soc_component_write_field(component, PM4125_ANA_MICBIAS_LDO_1_SETTING,
+				      PM4125_ANA_MICBIAS_MICB_OUT_VAL_MASK, req_vout_ctl);
+
+	if (micb_en == 1) {
+		snd_soc_component_write_field(component, PM4125_ANA_MICBIAS_MICB_1_2_EN,
+					      pullup_mask,
+					      PM4125_ANA_MICBIAS_MICB_PULL_DISABLE);
+		/* Add 2ms delay as per HW requirement after enabling micbias */
+		usleep_range(2000, 2100);
+	}
+exit:
+	mutex_unlock(&pm4125->micb_lock);
+
+	return ret;
+}
+
+static int pm4125_mbhc_micb_ctrl_threshold_mic(struct snd_soc_component *component,
+					       int micb_num, bool req_en)
+{
+	struct pm4125_priv *pm4125 = snd_soc_component_get_drvdata(component);
+	int micb_mv;
+
+	if (micb_num != MIC_BIAS_2)
+		return -EINVAL;
+
+	/*
+	 * If device tree micbias level is already above the minimum voltage
+	 * needed to detect threshold microphone, then do not change the
+	 * micbias, just return.
+	 */
+	if (pm4125->micb2_mv >= WCD_MBHC_THR_HS_MICB_MV)
+		return 0;
+
+	micb_mv = req_en ? WCD_MBHC_THR_HS_MICB_MV : pm4125->micb2_mv;
+
+	return pm4125_mbhc_micb_adjust_voltage(component, micb_mv, MIC_BIAS_2);
+}
+
+static void pm4125_mbhc_get_micbias_val(struct snd_soc_component *component, int *mb)
+{
+	u8 vout_ctl;
+
+	/* Read MBHC Micbias (Mic Bias2) voltage */
+	vout_ctl = snd_soc_component_read_field(component, PM4125_ANA_MICBIAS_LDO_1_SETTING,
+						PM4125_ANA_MICBIAS_MICB_OUT_VAL_MASK);
+
+	/*
+	 * Formula for getting micbias from vout:
+	 *	micbias = 1.6V + VOUT_CTL * 50mV
+	 */
+	*mb = 1600 + (vout_ctl * 50);
+}
+
+static void pm4125_mbhc_gnd_det_ctrl(struct snd_soc_component *component, bool enable)
+{
+	if (enable) {
+		snd_soc_component_write_field(component, PM4125_ANA_MBHC_MECH,
+					      PM4125_ANA_MBHC_HSG_PULLUP_COMP_EN_MASK, 1);
+		snd_soc_component_write_field(component, PM4125_ANA_MBHC_MECH,
+					      PM4125_ANA_MBHC_GND_DET_EN_MASK, 1);
+	} else {
+		snd_soc_component_write_field(component, PM4125_ANA_MBHC_MECH,
+					      PM4125_ANA_MBHC_GND_DET_EN_MASK, 0);
+		snd_soc_component_write_field(component, PM4125_ANA_MBHC_MECH,
+					      PM4125_ANA_MBHC_HSG_PULLUP_COMP_EN_MASK, 0);
+	}
+}
+
+static void pm4125_mbhc_hph_pull_down_ctrl(struct snd_soc_component *component, bool enable)
+{
+	u8 val = enable ? PM4125_ANA_HPHPA_CNP_GND_PULL_DOWN :
+			  PM4125_ANA_HPHPA_CNP_GND_PULL_NONE;
+
+	snd_soc_component_write_field(component, PM4125_ANA_HPHPA_CNP_CTL_2,
+				      PM4125_ANA_HPHPA_CNP_GND_L_MASK, val);
+	snd_soc_component_write_field(component, PM4125_ANA_HPHPA_CNP_CTL_2,
+				      PM4125_ANA_HPHPA_CNP_GND_R_MASK, val);
+}
+
 static const struct wcd_mbhc_cb mbhc_cb = {
+	.clk_setup = pm4125_mbhc_clk_setup,
 	.mbhc_bias = pm4125_mbhc_bias_control,
 	.set_btn_thr = pm4125_mbhc_program_btn_thr,
+	.micbias_enable_status = pm4125_mbhc_micb_en_status,
+	.hph_pull_up_control = pm4125_mbhc_hph_l_pull_up_control,
+	.mbhc_micbias_control = pm4125_mbhc_request_micbias,
+	.mbhc_micb_ramp_control = pm4125_mbhc_micb_ramp_control,
+	.mbhc_micb_ctrl_thr_mic = pm4125_mbhc_micb_ctrl_threshold_mic,
+	.get_micbias_val = pm4125_mbhc_get_micbias_val,
+	.mbhc_gnd_det_ctrl = pm4125_mbhc_gnd_det_ctrl,
+	.hph_pull_down_ctrl = pm4125_mbhc_hph_pull_down_ctrl,
 };
 
 static int pm4125_mbhc_init(struct snd_soc_component *component)
@@ -1692,6 +1879,7 @@ static int pm4125_probe(struct platform_device *pdev)
 		return dev_err_probe(dev, ret, "Failed to get micbias\n");
 
 	atomic_set(&pm4125->gloal_mbias_cnt, 0);
+	mutex_init(&pm4125->micb_lock);
 
 	cfg = &pm4125->mbhc_cfg;
 	cfg->mbhc_micbias = MIC_BIAS_2;
diff --git a/sound/soc/codecs/pm4125.h b/sound/soc/codecs/pm4125.h
index 25fd3106f..3ee6f9c25 100644
--- a/sound/soc/codecs/pm4125.h
+++ b/sound/soc/codecs/pm4125.h
@@ -13,7 +13,9 @@
 #define PM4125_DIG_BASE_ADDR			0x3400
 
 #define PM4125_ANA_MICBIAS_MICB_1_2_EN		(PM4125_ANA_BASE_ADDR+0x040)
+#define  PM4125_ANA_MICBIAS_MICB1_EN_MASK	BIT(6)
 #define  PM4125_ANA_MICBIAS_MICB1_PULL_UP_MASK	BIT(5)
+#define  PM4125_ANA_MICBIAS_MICB2_EN_MASK	BIT(2)
 #define  PM4125_ANA_MICBIAS_MICB2_PULL_UP_MASK	BIT(1)
 #define  PM4125_ANA_MICBIAS_MICB2_PULL_DN_MASK	BIT(0)
 #define  PM4125_ANA_MICBIAS_MICB_PULL_ENABLE	1
@@ -25,6 +27,8 @@
 #define PM4125_ANA_TX_AMIC1			(PM4125_ANA_BASE_ADDR+0x047)
 #define PM4125_ANA_TX_AMIC2			(PM4125_ANA_BASE_ADDR+0x048)
 #define PM4125_ANA_MBHC_MECH			(PM4125_ANA_BASE_ADDR+0x05A)
+#define  PM4125_ANA_MBHC_GND_DET_EN_MASK	BIT(6)
+#define  PM4125_ANA_MBHC_HSG_PULLUP_COMP_EN_MASK BIT(1)
 #define PM4125_ANA_MBHC_ELECT			(PM4125_ANA_BASE_ADDR+0x05B)
 #define  PM4125_ANA_MBHC_ELECT_BIAS_EN_MASK	BIT(0)
 #define  PM4125_ANA_MBHC_ELECT_BIAS_ENABLE	1
@@ -40,9 +44,14 @@
 #define PM4125_ANA_MBHC_BTN3_ZDET_DBG_400	(PM4125_ANA_BASE_ADDR+0x063)
 #define PM4125_ANA_MBHC_BTN4_ZDET_DBG_1400	(PM4125_ANA_BASE_ADDR+0x064)
 #define PM4125_ANA_MBHC_MICB2_RAMP		(PM4125_ANA_BASE_ADDR+0x065)
+#define  PM4125_ANA_MBHC_MICB2_RAMP_EN_MASK	BIT(7)
+#define  PM4125_ANA_MBHC_MICB2_RAMP_SHIFT_MASK	GENMASK(4, 2)
+#define  PM4125_ANA_MBHC_MICB2_RAMP_SHIFT_CTRL	3
 #define PM4125_ANA_MBHC_CTL_1			(PM4125_ANA_BASE_ADDR+0x066)
+#define  PM4125_ANA_MBHC_CTL_RCO_EN_MASK	BIT(7)
 #define PM4125_ANA_MBHC_CTL_2			(PM4125_ANA_BASE_ADDR+0x067)
 #define PM4125_ANA_MBHC_PLUG_DETECT_CTL		(PM4125_ANA_BASE_ADDR+0x068)
+#define  PM4125_ANA_MBHC_HSDET_PULLUP_C_MASK	GENMASK(7, 6)
 #define PM4125_ANA_MBHC_ZDET_ANA_CTL		(PM4125_ANA_BASE_ADDR+0x069)
 #define PM4125_ANA_MBHC_ZDET_RAMP_CTL		(PM4125_ANA_BASE_ADDR+0x06A)
 #define PM4125_ANA_MBHC_FSM_STATUS		(PM4125_ANA_BASE_ADDR+0x06B)
@@ -58,6 +67,10 @@
 #define  PM4125_ANA_HPHPA_CNP_CTL_1_EN_MASK	BIT(1)
 #define  PM4125_ANA_HPHPA_CNP_CTL_1_EN		1
 #define PM4125_ANA_HPHPA_CNP_CTL_2		(PM4125_ANA_BASE_ADDR+0x084)
+#define  PM4125_ANA_HPHPA_CNP_GND_L_MASK	GENMASK(5, 4)
+#define  PM4125_ANA_HPHPA_CNP_GND_R_MASK	GENMASK(3, 2)
+#define  PM4125_ANA_HPHPA_CNP_GND_PULL_DOWN	2
+#define  PM4125_ANA_HPHPA_CNP_GND_PULL_NONE	0
 #define  PM4125_ANA_HPHPA_CNP_OCP_EN_L_MASK	BIT(1)
 #define  PM4125_ANA_HPHPA_CNP_OCP_EN_R_MASK	BIT(0)
 #define  PM4125_ANA_HPHPA_CNP_OCP_ENABLE	1

-- 
2.34.1

Re: [PATCH v1 4/4] ASoC: codecs: pm4125: Add the MBHC callbacks needed for jack detection

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] sound: pm4125: Build failures due to undefined function and missing struct member
- [High] ASoC: codecs: pm4125: pm4125_mbhc_program_btn_thr() OOB register write
- [High] ASoC: pm4125: missing lock for concurrent micbias and mbias initialization
- [Medium] sound: pm4125: Data race and hardware state corruption in micbias control
--

commit a2d3fc28945c83703320008a102f99781fb6c008
Author: Mohammad Rafi Shaik <mohammad.rafi.shaik@oss.qualcomm.com>

ASoC: codecs: pm4125: Add the MBHC callbacks needed for jack detection

This commit populates the remaining wcd_mbhc_cb callbacks required by the
MBHC framework for jack detection.

> diff --git a/sound/soc/codecs/pm4125.c b/sound/soc/codecs/pm4125.c
> index 2100f4e36bbbd..6dd5ce0f4bab3 100644
> --- a/sound/soc/codecs/pm4125.c
> +++ b/sound/soc/codecs/pm4125.c

[ ... ]

> @@ -995,9 +1004,187 @@ static void pm4125_mbhc_program_btn_thr(struct snd_soc_component *component,
>  	}
>  }

[Severity: High]
Does pm4125_mbhc_program_btn_thr() write past the button threshold registers?

The function writes to contiguous registers starting at 0x060 in a loop up to
num_btn. The device only has 5 button threshold registers (0x060 to 0x064),
but num_btn is configured to 8 (PM4125_MBHC_MAX_BUTTONS). If this loops to 8,
won't it write to 0x065, 0x066, and 0x067, silently clobbering
PM4125_ANA_MBHC_MICB2_RAMP, PM4125_ANA_MBHC_CTL_1, and PM4125_ANA_MBHC_CTL_2,
breaking the functionality added in this patch?

> +static bool pm4125_mbhc_micb_en_status(struct snd_soc_component *component, int micb_num)
> +{
> +	if (micb_num != MIC_BIAS_2)
> +		return false;

[ ... ]

> +static int pm4125_mbhc_request_micbias(struct snd_soc_component *component,
> +				       int micb_num, int req)
> +{
> +	return pm4125_micbias_control(component, micb_num, req, false);
> +}

[Severity: High]
Are there missing locks for concurrent micbias and mbias initialization here?

By hooking up the mbhc_micbias_control callback, pm4125_micbias_control() and
pm4125_global_mbias_enable() are exposed to concurrent execution by the jack
detection workqueue and the ALSA DAPM power-up thread.

In pm4125_global_mbias_enable(), secondary callers that hit
atomic_inc_return(&pm4125->gloal_mbias_cnt) > 1 will bypass the 1ms wait and
return immediately, using uninitialized hardware.

Additionally, in pm4125_micbias_control(), the micb_ref array is modified
non-atomically, leading to reference count corruption.

[Severity: Medium]
Can this also cause a data race and hardware state corruption?

pm4125_micbias_control() modifies PM4125_ANA_MICBIAS_MICB_1_2_EN and the
software reference counters without taking the newly introduced micb_lock.
Since this can run concurrently with pm4125_mbhc_micb_adjust_voltage(),
won't this corrupt the reference counts and clobber the register state?

[ ... ]

> +static int pm4125_mbhc_micb_adjust_voltage(struct snd_soc_component *component,
> +					   int req_volt, int micb_num)
> +{
> +	struct pm4125_priv *pm4125 = snd_soc_component_get_drvdata(component);
> +	int cur_vout_ctl, req_vout_ctl, micb_en, ret = 0;
> +	unsigned int pullup_mask, en_mask;

[ ... ]

> +	cur_vout_ctl = snd_soc_component_read_field(component, PM4125_ANA_MICBIAS_LDO_1_SETTING,
> +						    PM4125_ANA_MICBIAS_MICB_OUT_VAL_MASK);
> +
> +	req_vout_ctl = pm4125_get_micb_vout_ctl_val(component->dev, req_volt);

[Severity: High]
Will this cause a build failure?

The function pm4125_get_micb_vout_ctl_val() is not declared or defined
anywhere. Should this be using the shared WCD common helper
wcd_get_micb_vout_ctl_val() instead?

> +	if (req_vout_ctl < 0) {
> +		ret = -EINVAL;
> +		goto exit;
> +	}

[ ... ]

> +static int pm4125_mbhc_micb_ctrl_threshold_mic(struct snd_soc_component *component,
> +					       int micb_num, bool req_en)
> +{
> +	struct pm4125_priv *pm4125 = snd_soc_component_get_drvdata(component);
> +	int micb_mv;
> +
> +	if (micb_num != MIC_BIAS_2)
> +		return -EINVAL;
> +
> +	/*
> +	 * If device tree micbias level is already above the minimum voltage
> +	 * needed to detect threshold microphone, then do not change the
> +	 * micbias, just return.
> +	 */
> +	if (pm4125->micb2_mv >= WCD_MBHC_THR_HS_MICB_MV)
> +		return 0;

[Severity: High]
Will this access to pm4125->micb2_mv cause a compile error?

struct pm4125_priv does not have a micb2_mv member. Based on pm4125_probe(),
should this be pm4125->common.micb_mv[1] or pm4125->mbhc_cfg.micb_mv?

> +
> +	micb_mv = req_en ? WCD_MBHC_THR_HS_MICB_MV : pm4125->micb2_mv;
> +
> +	return pm4125_mbhc_micb_adjust_voltage(component, micb_mv, MIC_BIAS_2);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261015-pm4125_mbhc_support-v1-0-8bc1212db6f7@oss.qualcomm.com?part=4