Re: [PATCH 2/3] ALSA: hda/cs35l56: Use scoped_guard() for irq_lock in cs35l56_hda_fw_load()

[Richard Fitzgerald <rf@opensource.cirrus.com>]

On 01/07/2026 7:51 am, wangdich9700@163.com wrote:
> From: wangdicheng <wangdicheng@kylinos.cn>
> 
> Replace the manual mutex_lock/mutex_unlock of irq_lock with
> scoped_guard(). This lets the compiler guarantee the lock is
> released on all exit paths, eliminating the err label that
> previously existed only to reach the unlock call.
> 
> No functional changes.
> 
> Signed-off-by: wangdicheng <wangdicheng@kylinos.cn>
> ---

Is this fixing any bugs?

This is a lot of code churn to fix nothing.

>   sound/hda/codecs/side-codecs/cs35l56_hda.c | 104 ++++++++++-----------
>   1 file changed, 51 insertions(+), 53 deletions(-)
> 
> diff --git a/sound/hda/codecs/side-codecs/cs35l56_hda.c b/sound/hda/codecs/side-codecs/cs35l56_hda.c
> index a0ea08eb96a9..2d9d75053293 100644
> --- a/sound/hda/codecs/side-codecs/cs35l56_hda.c
> +++ b/sound/hda/codecs/side-codecs/cs35l56_hda.c
> @@ -625,74 +625,72 @@ static void cs35l56_hda_fw_load(struct cs35l56_hda *cs35l56)
>   		goto err_fw_release;
>   	}
>   
> -	mutex_lock(&cs35l56->base.irq_lock);
> +	scoped_guard(mutex, &cs35l56->base.irq_lock) {
> +		/*
> +		 * If the firmware hasn't been patched it must be shutdown before
> +		 * doing a full patch and reset afterwards. If it is already
> +		 * running a patched version the firmware files only contain
> +		 * tunings and we can use the lower cost reinit sequence instead.
> +		 */
> +		if (firmware_missing && (wmfw_firmware || coeff_firmware)) {
> +			ret = cs35l56_firmware_shutdown(&cs35l56->base);
> +			if (ret)
> +				goto err_fw_release;
> +		}
>   
> -	/*
> -	 * If the firmware hasn't been patched it must be shutdown before
> -	 * doing a full patch and reset afterwards. If it is already
> -	 * running a patched version the firmware files only contain
> -	 * tunings and we can use the lower cost reinit sequence instead.
> -	 */
> -	if (firmware_missing && (wmfw_firmware || coeff_firmware)) {
> -		ret = cs35l56_firmware_shutdown(&cs35l56->base);
> -		if (ret)
> -			goto err;
> -	}
> +		ret = cs_dsp_power_up(&cs35l56->cs_dsp, wmfw_firmware, wmfw_filename,
> +				      coeff_firmware, coeff_filename, "misc");
> +		if (ret) {
> +			dev_dbg(cs35l56->base.dev, "%s: cs_dsp_power_up ret %d\n", __func__, ret);
> +			goto err_fw_release;
> +		}
>   
> -	ret = cs_dsp_power_up(&cs35l56->cs_dsp, wmfw_firmware, wmfw_filename,
> -			      coeff_firmware, coeff_filename, "misc");
> -	if (ret) {
> -		dev_dbg(cs35l56->base.dev, "%s: cs_dsp_power_up ret %d\n", __func__, ret);
> -		goto err;
> -	}
> +		if (wmfw_filename)
> +			dev_dbg(cs35l56->base.dev, "Loaded WMFW Firmware: %s\n", wmfw_filename);
> +
> +		if (coeff_filename)
> +			dev_dbg(cs35l56->base.dev, "Loaded Coefficients: %s\n", coeff_filename);
>   
> -	if (wmfw_filename)
> -		dev_dbg(cs35l56->base.dev, "Loaded WMFW Firmware: %s\n", wmfw_filename);
> +		/* If we downloaded firmware, reset the device and wait for it to boot */
> +		if (firmware_missing && (wmfw_firmware || coeff_firmware)) {
> +			cs35l56_system_reset(&cs35l56->base, false);
> +			regcache_mark_dirty(cs35l56->base.regmap);
> +			ret = cs35l56_wait_for_firmware_boot(&cs35l56->base);
> +			if (ret)
> +				goto err_powered_up;
>   
> -	if (coeff_filename)
> -		dev_dbg(cs35l56->base.dev, "Loaded Coefficients: %s\n", coeff_filename);
> +			regcache_cache_only(cs35l56->base.regmap, false);
> +		}
>   
> -	/* If we downloaded firmware, reset the device and wait for it to boot */
> -	if (firmware_missing && (wmfw_firmware || coeff_firmware)) {
> -		cs35l56_system_reset(&cs35l56->base, false);
> -		regcache_mark_dirty(cs35l56->base.regmap);
> -		ret = cs35l56_wait_for_firmware_boot(&cs35l56->base);
> +		/* Disable auto-hibernate so that runtime_pm has control */
> +		ret = cs35l56_mbox_send(&cs35l56->base, CS35L56_MBOX_CMD_PREVENT_AUTO_HIBERNATE);
>   		if (ret)
>   			goto err_powered_up;
>   
> -		regcache_cache_only(cs35l56->base.regmap, false);
> -	}
> -
> -	/* Disable auto-hibernate so that runtime_pm has control */
> -	ret = cs35l56_mbox_send(&cs35l56->base, CS35L56_MBOX_CMD_PREVENT_AUTO_HIBERNATE);
> -	if (ret)
> -		goto err_powered_up;
> -
> -	regcache_sync(cs35l56->base.regmap);
> +		regcache_sync(cs35l56->base.regmap);
>   
> -	regmap_clear_bits(cs35l56->base.regmap,
> -			  cs35l56->base.fw_reg->prot_sts,
> -			  CS35L56_FIRMWARE_MISSING);
> -	cs35l56->base.fw_patched = true;
> +		regmap_clear_bits(cs35l56->base.regmap,
> +				  cs35l56->base.fw_reg->prot_sts,
> +				  CS35L56_FIRMWARE_MISSING);
> +		cs35l56->base.fw_patched = true;
>   
> -	ret = cs_dsp_run(&cs35l56->cs_dsp);
> -	if (ret)
> -		dev_dbg(cs35l56->base.dev, "%s: cs_dsp_run ret %d\n", __func__, ret);
> +		ret = cs_dsp_run(&cs35l56->cs_dsp);
> +		if (ret)
> +			dev_dbg(cs35l56->base.dev, "%s: cs_dsp_run ret %d\n", __func__, ret);
>   
> -	/* Don't need to check return code, it's not fatal if this fails */
> -	cs35l56_hda_apply_calibration(cs35l56);
> +		/* Don't need to check return code, it's not fatal if this fails */
> +		cs35l56_hda_apply_calibration(cs35l56);
>   
> -	ret = cs35l56_mbox_send(&cs35l56->base, CS35L56_MBOX_CMD_AUDIO_REINIT);
> -	if (ret)
> -		cs_dsp_stop(&cs35l56->cs_dsp);
> +		ret = cs35l56_mbox_send(&cs35l56->base, CS35L56_MBOX_CMD_AUDIO_REINIT);
> +		if (ret)
> +			cs_dsp_stop(&cs35l56->cs_dsp);
>   
> -	cs35l56_log_tuning(&cs35l56->base, &cs35l56->cs_dsp);
> +		cs35l56_log_tuning(&cs35l56->base, &cs35l56->cs_dsp);
>   
>   err_powered_up:
> -	if (!cs35l56->base.fw_patched)
> -		cs_dsp_power_down(&cs35l56->cs_dsp);
> -err:
> -	mutex_unlock(&cs35l56->base.irq_lock);
> +		if (!cs35l56->base.fw_patched)
> +			cs_dsp_power_down(&cs35l56->cs_dsp);
> +	}
>   err_fw_release:
>   	cs35l56_hda_release_firmware_files(wmfw_firmware, wmfw_filename,
>   					   coeff_firmware, coeff_filename);