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

[sashiko-bot@kernel.org]

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