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

[Takashi Iwai <tiwai@suse.de>]

On Wed, 01 Jul 2026 11:46:18 +0200,
Richard Fitzgerald wrote:
> 
> 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.

IMO, as a cleanup, such a change *may* be acceptable.  Dropping the
manual locking calls can reduce the mistakes and makes the code better
maintained.

Most of changes are due to the indentation, and you can ignore them
via -w option of git-diff or git-show.  (It's difficult to do it in
the review of manual patches, though.)

But, one thing still needs to be clarified: such changes must be
verified with clang W=1 build, at least, too; the combination of
guard() and goto aren't always good.


thanks,

Takashi

> 
> >   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);
>