[PATCH 1/4] ASoC: rt721-sdca: handle HID button read errors

[PATCH 1/4] ASoC: rt721-sdca: handle HID button read errors

[Avi Weiss]

rt_sdca_button_detect() returns zero when a regmap read fails,
so RT721 treats the error as a valid no-button result.

Return the original error and skip button reporting when a read
fails. Still report jack changes, and clear button state if the
headset is removed.

Fixes: bbca8e7050e0 ("ASoC: rt-sdw-common: Common functions for Realtek soundwire driver")
Signed-off-by: Avi Weiss <thnkslprpt@gmail.com>
---
 sound/soc/codecs/rt-sdw-common.c |  5 ++++-
 sound/soc/codecs/rt721-sdca.c    | 13 +++++++++++++
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/sound/soc/codecs/rt-sdw-common.c b/sound/soc/codecs/rt-sdw-common.c
index ad61943ce75ff..e4139e6393b1c 100644
--- a/sound/soc/codecs/rt-sdw-common.c
+++ b/sound/soc/codecs/rt-sdw-common.c
@@ -199,7 +199,7 @@ int rt_sdca_button_detect(struct regmap *map, unsigned int entity_id,
 	ret = regmap_read(map, SDW_SDCA_CTL(SDCA_NUM_HID, entity_id,
 			RT_SDCA_CTL_HIDTX_CURRENT_OWNER, 0), &owner);
 	if (ret < 0)
-		return 0;
+		return ret;
 
 	/* if owner is device then there is no button event from device */
 	if (owner == 1)
@@ -229,6 +229,9 @@ int rt_sdca_button_detect(struct regmap *map, unsigned int entity_id,
 			SDW_SDCA_CTL(SDCA_NUM_HID, entity_id,
 				RT_SDCA_CTL_HIDTX_CURRENT_OWNER, 0), 0x01);
 
+	if (ret < 0)
+		return ret;
+
 	return btn_type;
 }
 EXPORT_SYMBOL_GPL(rt_sdca_button_detect);
diff --git a/sound/soc/codecs/rt721-sdca.c b/sound/soc/codecs/rt721-sdca.c
index 9f567caecb70a..95c028efec01c 100644
--- a/sound/soc/codecs/rt721-sdca.c
+++ b/sound/soc/codecs/rt721-sdca.c
@@ -248,6 +248,19 @@ static void rt721_sdca_jack_detect_handler(struct work_struct *work)
 					RT721_SDCA_ENT_HID01, RT721_BUF_ADDR_HID1,
 					RT721_SDCA_HID_ID);
 
+	if (btn_type < 0) {
+		if (rt721->scp_sdca_stat1 & SDW_SCP_SDCA_INT_SDCA_0) {
+			unsigned int mask = SND_JACK_HEADSET;
+
+			if (rt721->jack_type != SND_JACK_HEADSET)
+				mask |= SND_JACK_BTN_0 | SND_JACK_BTN_1 |
+					SND_JACK_BTN_2 | SND_JACK_BTN_3;
+
+			snd_soc_jack_report(rt721->hs_jack, rt721->jack_type, mask);
+		}
+		return;
+	}
+
 	if (rt721->jack_type == 0)
 		btn_type = 0;
 

[PATCH 2/4] ASoC: rt711-sdca: handle HID button read errors

[Avi Weiss]

rt711_sdca_button_detect() returns zero when a regmap read fails,
so the jack handler cannot distinguish the error from a valid
no-button result.

Return the original error and make the button result signed.
Skip button reporting on read failure, but still report jack
changes and clear button state when the headset is removed.

Fixes: 7ad4d237e7c4 ("ASoC: rt711-sdca: Add RT711 SDCA vendor-specific driver")
Signed-off-by: Avi Weiss <thnkslprpt@gmail.com>
---
 sound/soc/codecs/rt711-sdca.c | 24 ++++++++++++++++++++----
 1 file changed, 20 insertions(+), 4 deletions(-)

diff --git a/sound/soc/codecs/rt711-sdca.c b/sound/soc/codecs/rt711-sdca.c
index 9781f289f8adc..11ffe1b861972 100644
--- a/sound/soc/codecs/rt711-sdca.c
+++ b/sound/soc/codecs/rt711-sdca.c
@@ -184,10 +184,10 @@ static int rt711_sdca_calibration(struct rt711_sdca_priv *rt711)
 	return ret;
 }
 
-static unsigned int rt711_sdca_button_detect(struct rt711_sdca_priv *rt711)
+static int rt711_sdca_button_detect(struct rt711_sdca_priv *rt711)
 {
-	unsigned int btn_type = 0, offset, idx, val, owner;
-	int ret;
+	unsigned int offset, idx, val, owner;
+	int btn_type = 0, ret;
 	unsigned char buf[3];
 
 	/* get current UMP message owner */
@@ -195,7 +195,7 @@ static unsigned int rt711_sdca_button_detect(struct rt711_sdca_priv *rt711)
 		SDW_SDCA_CTL(FUNC_NUM_HID, RT711_SDCA_ENT_HID01, RT711_SDCA_CTL_HIDTX_CURRENT_OWNER, 0),
 		&owner);
 	if (ret < 0)
-		return 0;
+		return ret;
 
 	/* if owner is device then there is no button event from device */
 	if (owner == 1)
@@ -259,6 +259,9 @@ static unsigned int rt711_sdca_button_detect(struct rt711_sdca_priv *rt711)
 			SDW_SDCA_CTL(FUNC_NUM_HID, RT711_SDCA_ENT_HID01,
 				RT711_SDCA_CTL_HIDTX_SET_OWNER_TO_DEVICE, 0), 0x01);
 
+	if (ret < 0)
+		return ret;
+
 	return btn_type;
 }
 
@@ -330,6 +333,19 @@ static void rt711_sdca_jack_detect_handler(struct work_struct *work)
 	if (rt711->scp_sdca_stat2 & SDW_SCP_SDCA_INT_SDCA_8)
 		btn_type = rt711_sdca_button_detect(rt711);
 
+	if (btn_type < 0) {
+		if (rt711->scp_sdca_stat1 & SDW_SCP_SDCA_INT_SDCA_0) {
+			unsigned int mask = SND_JACK_HEADSET;
+
+			if (rt711->jack_type != SND_JACK_HEADSET)
+				mask |= SND_JACK_BTN_0 | SND_JACK_BTN_1 |
+					SND_JACK_BTN_2 | SND_JACK_BTN_3;
+
+			snd_soc_jack_report(rt711->hs_jack, rt711->jack_type, mask);
+		}
+		return;
+	}
+
 	if (rt711->jack_type == 0)
 		btn_type = 0;
 

[PATCH 3/4] ASoC: rt712-sdca: handle HID button read errors

[Avi Weiss]

rt712_sdca_button_detect() returns zero when a regmap read fails,
so the jack handler cannot distinguish the error from a valid
no-button result.

Return the original error and make the button result signed.
Skip button reporting on read failure, but still report jack
changes and clear button state when the headset is removed.

Fixes: 6c39710da75c ("ASoC: rt712-sdca: Add RT712 SDCA driver for Jack and Amp topology")
Signed-off-by: Avi Weiss <thnkslprpt@gmail.com>
---
 sound/soc/codecs/rt712-sdca.c | 24 ++++++++++++++++++++----
 1 file changed, 20 insertions(+), 4 deletions(-)

diff --git a/sound/soc/codecs/rt712-sdca.c b/sound/soc/codecs/rt712-sdca.c
index 20961e5760c65..d4c0a3ac5d61e 100644
--- a/sound/soc/codecs/rt712-sdca.c
+++ b/sound/soc/codecs/rt712-sdca.c
@@ -335,10 +335,10 @@ static int rt712_sdca_calibration(struct rt712_sdca_priv *rt712)
 	return ret;
 }
 
-static unsigned int rt712_sdca_button_detect(struct rt712_sdca_priv *rt712)
+static int rt712_sdca_button_detect(struct rt712_sdca_priv *rt712)
 {
-	unsigned int btn_type = 0, offset, idx, val, owner;
-	int ret;
+	unsigned int offset, idx, val, owner;
+	int btn_type = 0, ret;
 	unsigned char buf[3];
 
 	/* get current UMP message owner */
@@ -346,7 +346,7 @@ static unsigned int rt712_sdca_button_detect(struct rt712_sdca_priv *rt712)
 		SDW_SDCA_CTL(FUNC_NUM_HID, RT712_SDCA_ENT_HID01, RT712_SDCA_CTL_HIDTX_CURRENT_OWNER, 0),
 		&owner);
 	if (ret < 0)
-		return 0;
+		return ret;
 
 	/* if owner is device then there is no button event from device */
 	if (owner == 1)
@@ -416,6 +416,9 @@ static unsigned int rt712_sdca_button_detect(struct rt712_sdca_priv *rt712)
 					RT712_SDCA_CTL_HIDTX_CURRENT_OWNER, 0), 0x01);
 	}
 
+	if (ret < 0)
+		return ret;
+
 	return btn_type;
 }
 
@@ -485,6 +488,19 @@ static void rt712_sdca_jack_detect_handler(struct work_struct *work)
 	if (rt712->scp_sdca_stat2 & SDW_SCP_SDCA_INT_SDCA_8)
 		btn_type = rt712_sdca_button_detect(rt712);
 
+	if (btn_type < 0) {
+		if (rt712->scp_sdca_stat1 & SDW_SCP_SDCA_INT_SDCA_0) {
+			unsigned int mask = SND_JACK_HEADSET;
+
+			if (rt712->jack_type != SND_JACK_HEADSET)
+				mask |= SND_JACK_BTN_0 | SND_JACK_BTN_1 |
+					SND_JACK_BTN_2 | SND_JACK_BTN_3;
+
+			snd_soc_jack_report(rt712->hs_jack, rt712->jack_type, mask);
+		}
+		return;
+	}
+
 	if (rt712->jack_type == 0)
 		btn_type = 0;
 

[PATCH 4/4] ASoC: rt722-sdca: handle HID button read errors

[Avi Weiss]

rt722_sdca_button_detect() returns zero when a regmap read fails,
so the jack handler cannot distinguish the error from a valid
no-button result.

Return the original error and make the button result signed.
Skip button reporting on read failure, but still report jack
changes and clear button state when the headset is removed.

Fixes: 7f5d6036ca00 ("ASoC: rt722-sdca: Add RT722 SDCA driver")
Signed-off-by: Avi Weiss <thnkslprpt@gmail.com>
---
 sound/soc/codecs/rt722-sdca.c | 24 ++++++++++++++++++++----
 1 file changed, 20 insertions(+), 4 deletions(-)

diff --git a/sound/soc/codecs/rt722-sdca.c b/sound/soc/codecs/rt722-sdca.c
index 1dad8012d904e..01f56058cdcc0 100644
--- a/sound/soc/codecs/rt722-sdca.c
+++ b/sound/soc/codecs/rt722-sdca.c
@@ -95,10 +95,10 @@ static int rt722_sdca_btn_type(unsigned char *buffer)
 	return 0;
 }
 
-static unsigned int rt722_sdca_button_detect(struct rt722_sdca_priv *rt722)
+static int rt722_sdca_button_detect(struct rt722_sdca_priv *rt722)
 {
-	unsigned int btn_type = 0, offset, idx, val, owner;
-	int ret;
+	unsigned int offset, idx, val, owner;
+	int btn_type = 0, ret;
 	unsigned char buf[3];
 
 	/* get current UMP message owner */
@@ -106,7 +106,7 @@ static unsigned int rt722_sdca_button_detect(struct rt722_sdca_priv *rt722)
 		SDW_SDCA_CTL(FUNC_NUM_HID, RT722_SDCA_ENT_HID01,
 			RT722_SDCA_CTL_HIDTX_CURRENT_OWNER, 0), &owner);
 	if (ret < 0)
-		return 0;
+		return ret;
 
 	/* if owner is device then there is no button event from device */
 	if (owner == 1)
@@ -138,6 +138,9 @@ static unsigned int rt722_sdca_button_detect(struct rt722_sdca_priv *rt722)
 			SDW_SDCA_CTL(FUNC_NUM_HID, RT722_SDCA_ENT_HID01,
 				RT722_SDCA_CTL_HIDTX_CURRENT_OWNER, 0), 0x01);
 
+	if (ret < 0)
+		return ret;
+
 	return btn_type;
 }
 
@@ -207,6 +210,19 @@ static void rt722_sdca_jack_detect_handler(struct work_struct *work)
 	if (rt722->scp_sdca_stat2 & SDW_SCP_SDCA_INT_SDCA_8)
 		btn_type = rt722_sdca_button_detect(rt722);
 
+	if (btn_type < 0) {
+		if (rt722->scp_sdca_stat1 & SDW_SCP_SDCA_INT_SDCA_0) {
+			unsigned int mask = SND_JACK_HEADSET;
+
+			if (rt722->jack_type != SND_JACK_HEADSET)
+				mask |= SND_JACK_BTN_0 | SND_JACK_BTN_1 |
+					SND_JACK_BTN_2 | SND_JACK_BTN_3;
+
+			snd_soc_jack_report(rt722->hs_jack, rt722->jack_type, mask);
+		}
+		return;
+	}
+
 	if (rt722->jack_type == 0)
 		btn_type = 0;
 

Re: [PATCH 1/4] ASoC: rt721-sdca: handle HID button read errors

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] ASoC: rt721-sdca: missed jack unplug report in rt721_sdca_jack_detect_handler()
- [Low] ASoC: rt-sdw-common: outdated kernel-doc for rt_sdca_button_detect() errors
--

commit cf6272e44cca067c36577fc836fb60a9747c068e
Author: Avi Weiss <thnkslprpt@gmail.com>

ASoC: rt721-sdca: handle HID button read errors

This commit updates rt_sdca_button_detect() to return negative error codes
on regmap_read() failures instead of zero, and updates
rt721_sdca_jack_detect_handler() to skip reporting button state on read
errors while still reporting jack state changes.

> diff --git a/sound/soc/codecs/rt-sdw-common.c b/sound/soc/codecs/rt-sdw-common.c
> index ad61943ce75ff..e4139e6393b1c 100644
> --- a/sound/soc/codecs/rt-sdw-common.c
> +++ b/sound/soc/codecs/rt-sdw-common.c
> @@ -199,7 +199,7 @@ int rt_sdca_button_detect(struct regmap *map, unsigned int entity_id,
>  	ret = regmap_read(map, SDW_SDCA_CTL(SDCA_NUM_HID, entity_id,
>  			RT_SDCA_CTL_HIDTX_CURRENT_OWNER, 0), &owner);
>  	if (ret < 0)
> -		return 0;
> +		return ret;

[Severity: Low]
This isn't a bug, but since rt_sdca_button_detect() in
sound/soc/codecs/rt-sdw-common.c now returns negative error codes when
regmap_read() fails, should its kernel-doc comment above the function:

	 * A button type will be returned regarding to buffer,
	 * it returns zero if buffer cannot be recognized.

also be updated to note that a negative errno is returned on error,
similar to rt_sdca_headset_detect()?

>  	/* if owner is device then there is no button event from device */
>  	if (owner == 1)
> @@ -229,6 +229,9 @@ int rt_sdca_button_detect(struct regmap *map, unsigned int entity_id,
>  			SDW_SDCA_CTL(SDCA_NUM_HID, entity_id,
>  				RT_SDCA_CTL_HIDTX_CURRENT_OWNER, 0), 0x01);
>  
> +	if (ret < 0)
> +		return ret;
> +
>  	return btn_type;
>  }
>  EXPORT_SYMBOL_GPL(rt_sdca_button_detect);
> diff --git a/sound/soc/codecs/rt721-sdca.c b/sound/soc/codecs/rt721-sdca.c
> index 9f567caecb70a..95c028efec01c 100644
> --- a/sound/soc/codecs/rt721-sdca.c
> +++ b/sound/soc/codecs/rt721-sdca.c
> @@ -248,6 +248,19 @@ static void rt721_sdca_jack_detect_handler(struct work_struct *work)
>  					RT721_SDCA_ENT_HID01, RT721_BUF_ADDR_HID1,
>  					RT721_SDCA_HID_ID);
>  
> +	if (btn_type < 0) {
> +		if (rt721->scp_sdca_stat1 & SDW_SCP_SDCA_INT_SDCA_0) {
> +			unsigned int mask = SND_JACK_HEADSET;
> +
> +			if (rt721->jack_type != SND_JACK_HEADSET)
> +				mask |= SND_JACK_BTN_0 | SND_JACK_BTN_1 |
> +					SND_JACK_BTN_2 | SND_JACK_BTN_3;
> +
> +			snd_soc_jack_report(rt721->hs_jack, rt721->jack_type, mask);
> +		}
> +		return;
> +	}

[Severity: Medium]
Can this leave rt721->hs_jack out of sync with rt721->jack_type on RT721_S
when a headset is unplugged without SDW_SCP_SDCA_INT_SDCA_0 set in
rt721->scp_sdca_stat1?

In sound/soc/codecs/rt721-sdca-sdw.c:rt721_sdca_interrupt_callback(), if a
pending jack_detect_work scheduled by an unplug interrupt
(SDW_SCP_SDCA_INT_SDCA_0) is cancelled by a subsequent button interrupt
(SDW_SCP_SDCA_INT_SDCA_8) from contact bounce, only scp_sdca_stat2 is
preserved while scp_sdca_stat1 is overwritten with a fresh read of
SDW_SCP_SDCA_INT1 (which was already cleared):

	if (cancel_delayed_work_sync(&rt721->jack_detect_work)) {
		dev_warn(&slave->dev, "%s the pending delayed_work was cancelled", __func__);
		/* avoid the HID owner doesn't change to device */
		if (rt721->scp_sdca_stat2)
			scp_sdca_stat2 = rt721->scp_sdca_stat2;
	}
	...
	ret = sdw_read_no_pm(rt721->slave, SDW_SCP_SDCA_INT1);
	if (ret < 0)
		goto io_error;

	rt721->scp_sdca_stat1 = ret;

Then, when rt721_sdca_jack_detect_handler() runs on RT721_S, it checks
RT721_JD_1PIN_GAT_STA1 and unconditionally resets the driver's jack state
(setting rt721->jack_type = 0) regardless of SDW_SCP_SDCA_INT_SDCA_0:

	if (rt721->wf_id == RT721_S) {
		...
		rt_sdca_index_read(rt721->mbq_regmap, RT721_JD_CTRL,
			RT721_JD_1PIN_GAT_STA1, &real_JD);
		real_JD &= 0x1;
		if (real_JD == 0x0)
			rt721_sdca_jack_reset_state(rt721);

If rt_sdca_button_detect() then fails and returns a negative error while
SDW_SCP_SDCA_INT_SDCA_0 is clear in rt721->scp_sdca_stat1, this new error
path returns early without calling snd_soc_jack_report(), leaving
rt721->hs_jack stuck reporting the headset as connected and any previously
active buttons as pressed even though rt721->jack_type was reset to 0.

Should the error path also report the jack state when rt721->jack_type == 0
(or preserve scp_sdca_stat1 across cancelled work)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261030085735.2146047-1-thnkslprpt@gmail.com?part=1

Re: [PATCH 1/4] ASoC: rt721-sdca: handle HID button read errors

[Mark Brown]

> rt_sdca_button_detect() returns zero when a regmap read fails,
> so RT721 treats the error as a valid no-button result.
> 
> Return the original error and skip button reporting when a read
> fails. Still report jack changes, and clear button state if the
> headset is removed.

The Sashiko issue with missing updates when we exit early seem valid at
first glance:

  https://sashiko.dev/#/patchset/20261030085735.2146047-1-thnkslprpt%40gmail.com

-- 
Mark Brown <broonie@kernel.org>