[PATCH 0/5] ALSA: cirrus: Tidy up of firmware control read/write
This set of patches factors out some repeated code to clean up firmware control read/write functions, and removes some redundant control notification code.
Simon Trimmer (5): firmware: cs_dsp: Add locked wrappers for coeff read and write ASoC: wm_adsp: Use cs_dsp_coeff_lock_and_[read|write]_ctrl() ALSA: hda: hda_cs_dsp_ctl: Use cs_dsp_coeff_lock_and_[read|write]_ctrl() ASoC: wm_adsp: Remove notification of driver write ALSA: hda: hda_cs_dsp_ctl: Remove notification of driver write
drivers/firmware/cirrus/cs_dsp.c | 54 ++++++++++++++++++++++++++ include/linux/firmware/cirrus/cs_dsp.h | 4 ++ sound/pci/hda/hda_cs_dsp_ctl.c | 22 +---------- sound/soc/codecs/wm_adsp.c | 32 ++++----------- 4 files changed, 67 insertions(+), 45 deletions(-)
From: Simon Trimmer simont@opensource.cirrus.com
It is a common pattern for functions to take and release the DSP pwr_lock over the cs_dsp calls to read and write firmware controls. Add wrapper functions to do this sequence so that the calling code can be simplified to a single function call..
Signed-off-by: Simon Trimmer simont@opensource.cirrus.com Signed-off-by: Richard Fitzgerald rf@opensource.cirrus.com --- drivers/firmware/cirrus/cs_dsp.c | 54 ++++++++++++++++++++++++++ include/linux/firmware/cirrus/cs_dsp.h | 4 ++ 2 files changed, 58 insertions(+)
diff --git a/drivers/firmware/cirrus/cs_dsp.c b/drivers/firmware/cirrus/cs_dsp.c index 9f3d665cfdcf..0d139e4de37c 100644 --- a/drivers/firmware/cirrus/cs_dsp.c +++ b/drivers/firmware/cirrus/cs_dsp.c @@ -819,6 +819,33 @@ int cs_dsp_coeff_write_ctrl(struct cs_dsp_coeff_ctl *ctl, } EXPORT_SYMBOL_NS_GPL(cs_dsp_coeff_write_ctrl, FW_CS_DSP);
+/** + * cs_dsp_coeff_lock_and_write_ctrl() - Writes the given buffer to the given coefficient control + * @ctl: pointer to coefficient control + * @off: word offset at which data should be written + * @buf: the buffer to write to the given control + * @len: the length of the buffer in bytes + * + * Same as cs_dsp_coeff_write_ctrl() but takes pwr_lock. + * + * Return: A negative number on error, 1 when the control value changed and 0 when it has not. + */ +int cs_dsp_coeff_lock_and_write_ctrl(struct cs_dsp_coeff_ctl *ctl, + unsigned int off, const void *buf, size_t len) +{ + struct cs_dsp *dsp = ctl->dsp; + int ret; + + lockdep_assert_not_held(&dsp->pwr_lock); + + mutex_lock(&dsp->pwr_lock); + ret = cs_dsp_coeff_write_ctrl(ctl, off, buf, len); + mutex_unlock(&dsp->pwr_lock); + + return ret; +} +EXPORT_SYMBOL_GPL(cs_dsp_coeff_lock_and_write_ctrl); + static int cs_dsp_coeff_read_ctrl_raw(struct cs_dsp_coeff_ctl *ctl, unsigned int off, void *buf, size_t len) { @@ -891,6 +918,33 @@ int cs_dsp_coeff_read_ctrl(struct cs_dsp_coeff_ctl *ctl, } EXPORT_SYMBOL_NS_GPL(cs_dsp_coeff_read_ctrl, FW_CS_DSP);
+/** + * cs_dsp_coeff_lock_and_read_ctrl() - Reads the given coefficient control into the given buffer + * @ctl: pointer to coefficient control + * @off: word offset at which data should be read + * @buf: the buffer to store to the given control + * @len: the length of the buffer in bytes + * + * Same as cs_dsp_coeff_read_ctrl() but takes pwr_lock. + * + * Return: Zero for success, a negative number on error. + */ +int cs_dsp_coeff_lock_and_read_ctrl(struct cs_dsp_coeff_ctl *ctl, + unsigned int off, void *buf, size_t len) +{ + struct cs_dsp *dsp = ctl->dsp; + int ret; + + lockdep_assert_not_held(&dsp->pwr_lock); + + mutex_lock(&dsp->pwr_lock); + ret = cs_dsp_coeff_read_ctrl(ctl, off, buf, len); + mutex_unlock(&dsp->pwr_lock); + + return ret; +} +EXPORT_SYMBOL_GPL(cs_dsp_coeff_lock_and_read_ctrl); + static int cs_dsp_coeff_init_control_caches(struct cs_dsp *dsp) { struct cs_dsp_coeff_ctl *ctl; diff --git a/include/linux/firmware/cirrus/cs_dsp.h b/include/linux/firmware/cirrus/cs_dsp.h index 23384a54d575..82687e07a7c2 100644 --- a/include/linux/firmware/cirrus/cs_dsp.h +++ b/include/linux/firmware/cirrus/cs_dsp.h @@ -238,8 +238,12 @@ void cs_dsp_cleanup_debugfs(struct cs_dsp *dsp); int cs_dsp_coeff_write_acked_control(struct cs_dsp_coeff_ctl *ctl, unsigned int event_id); int cs_dsp_coeff_write_ctrl(struct cs_dsp_coeff_ctl *ctl, unsigned int off, const void *buf, size_t len); +int cs_dsp_coeff_lock_and_write_ctrl(struct cs_dsp_coeff_ctl *ctl, unsigned int off, + const void *buf, size_t len); int cs_dsp_coeff_read_ctrl(struct cs_dsp_coeff_ctl *ctl, unsigned int off, void *buf, size_t len); +int cs_dsp_coeff_lock_and_read_ctrl(struct cs_dsp_coeff_ctl *ctl, unsigned int off, + void *buf, size_t len); struct cs_dsp_coeff_ctl *cs_dsp_get_ctl(struct cs_dsp *dsp, const char *name, int type, unsigned int alg);
From: Simon Trimmer simont@opensource.cirrus.com
Using the cs_dsp_coeff_lock_and_[read|write]_ctrl() wrappers tidies the calling functions as it does not need to manage the DSP pwr_lock.
Signed-off-by: Simon Trimmer simont@opensource.cirrus.com Signed-off-by: Richard Fitzgerald rf@opensource.cirrus.com --- sound/soc/codecs/wm_adsp.c | 24 ++++++------------------ 1 file changed, 6 insertions(+), 18 deletions(-)
diff --git a/sound/soc/codecs/wm_adsp.c b/sound/soc/codecs/wm_adsp.c index 7d5c096e06cd..8e366902ee4e 100644 --- a/sound/soc/codecs/wm_adsp.c +++ b/sound/soc/codecs/wm_adsp.c @@ -403,13 +403,8 @@ static int wm_coeff_put(struct snd_kcontrol *kctl, struct wm_coeff_ctl *ctl = bytes_ext_to_ctl(bytes_ext); struct cs_dsp_coeff_ctl *cs_ctl = ctl->cs_ctl; char *p = ucontrol->value.bytes.data; - int ret = 0; - - mutex_lock(&cs_ctl->dsp->pwr_lock); - ret = cs_dsp_coeff_write_ctrl(cs_ctl, 0, p, cs_ctl->len); - mutex_unlock(&cs_ctl->dsp->pwr_lock);
- return ret; + return cs_dsp_coeff_lock_and_write_ctrl(cs_ctl, 0, p, cs_ctl->len); }
static int wm_coeff_tlv_put(struct snd_kcontrol *kctl, @@ -426,13 +421,11 @@ static int wm_coeff_tlv_put(struct snd_kcontrol *kctl, if (!scratch) return -ENOMEM;
- if (copy_from_user(scratch, bytes, size)) { + if (copy_from_user(scratch, bytes, size)) ret = -EFAULT; - } else { - mutex_lock(&cs_ctl->dsp->pwr_lock); - ret = cs_dsp_coeff_write_ctrl(cs_ctl, 0, scratch, size); - mutex_unlock(&cs_ctl->dsp->pwr_lock); - } + else + ret = cs_dsp_coeff_lock_and_write_ctrl(cs_ctl, 0, scratch, size); + vfree(scratch);
return ret; @@ -474,13 +467,8 @@ static int wm_coeff_get(struct snd_kcontrol *kctl, struct wm_coeff_ctl *ctl = bytes_ext_to_ctl(bytes_ext); struct cs_dsp_coeff_ctl *cs_ctl = ctl->cs_ctl; char *p = ucontrol->value.bytes.data; - int ret;
- mutex_lock(&cs_ctl->dsp->pwr_lock); - ret = cs_dsp_coeff_read_ctrl(cs_ctl, 0, p, cs_ctl->len); - mutex_unlock(&cs_ctl->dsp->pwr_lock); - - return ret; + return cs_dsp_coeff_lock_and_read_ctrl(cs_ctl, 0, p, cs_ctl->len); }
static int wm_coeff_tlv_get(struct snd_kcontrol *kctl,
From: Simon Trimmer simont@opensource.cirrus.com
Using the cs_dsp_coeff_lock_and_[read|write]_ctrl() wrappers tidies the calling functions as it does not need to manage the DSP pwr_lock.
Signed-off-by: Simon Trimmer simont@opensource.cirrus.com Signed-off-by: Richard Fitzgerald rf@opensource.cirrus.com --- sound/pci/hda/hda_cs_dsp_ctl.c | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-)
diff --git a/sound/pci/hda/hda_cs_dsp_ctl.c b/sound/pci/hda/hda_cs_dsp_ctl.c index 463ca06036bf..725544d81941 100644 --- a/sound/pci/hda/hda_cs_dsp_ctl.c +++ b/sound/pci/hda/hda_cs_dsp_ctl.c @@ -51,13 +51,8 @@ static int hda_cs_dsp_coeff_put(struct snd_kcontrol *kctl, struct snd_ctl_elem_v struct hda_cs_dsp_coeff_ctl *ctl = (struct hda_cs_dsp_coeff_ctl *)snd_kcontrol_chip(kctl); struct cs_dsp_coeff_ctl *cs_ctl = ctl->cs_ctl; char *p = ucontrol->value.bytes.data; - int ret = 0; - - mutex_lock(&cs_ctl->dsp->pwr_lock); - ret = cs_dsp_coeff_write_ctrl(cs_ctl, 0, p, cs_ctl->len); - mutex_unlock(&cs_ctl->dsp->pwr_lock);
- return ret; + return cs_dsp_coeff_lock_and_write_ctrl(cs_ctl, 0, p, cs_ctl->len); }
static int hda_cs_dsp_coeff_get(struct snd_kcontrol *kctl, struct snd_ctl_elem_value *ucontrol) @@ -65,13 +60,8 @@ static int hda_cs_dsp_coeff_get(struct snd_kcontrol *kctl, struct snd_ctl_elem_v struct hda_cs_dsp_coeff_ctl *ctl = (struct hda_cs_dsp_coeff_ctl *)snd_kcontrol_chip(kctl); struct cs_dsp_coeff_ctl *cs_ctl = ctl->cs_ctl; char *p = ucontrol->value.bytes.data; - int ret; - - mutex_lock(&cs_ctl->dsp->pwr_lock); - ret = cs_dsp_coeff_read_ctrl(cs_ctl, 0, p, cs_ctl->len); - mutex_unlock(&cs_ctl->dsp->pwr_lock);
- return ret; + return cs_dsp_coeff_lock_and_read_ctrl(cs_ctl, 0, p, cs_ctl->len); }
static unsigned int wmfw_convert_flags(unsigned int in)
From: Simon Trimmer simont@opensource.cirrus.com
Any control that the driver is updating should be marked as SYSTEM and therefore will not have an ALSA control to notify.
Signed-off-by: Simon Trimmer simont@opensource.cirrus.com Signed-off-by: Richard Fitzgerald rf@opensource.cirrus.com --- sound/soc/codecs/wm_adsp.c | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-)
diff --git a/sound/soc/codecs/wm_adsp.c b/sound/soc/codecs/wm_adsp.c index 8e366902ee4e..6e348d49a89c 100644 --- a/sound/soc/codecs/wm_adsp.c +++ b/sound/soc/codecs/wm_adsp.c @@ -672,7 +672,6 @@ int wm_adsp_write_ctl(struct wm_adsp *dsp, const char *name, int type, unsigned int alg, void *buf, size_t len) { struct cs_dsp_coeff_ctl *cs_ctl; - struct wm_coeff_ctl *ctl; int ret;
mutex_lock(&dsp->cs_dsp.pwr_lock); @@ -683,12 +682,7 @@ int wm_adsp_write_ctl(struct wm_adsp *dsp, const char *name, int type, if (ret < 0) return ret;
- if (ret == 0 || (cs_ctl->flags & WMFW_CTL_FLAG_SYS)) - return 0; - - ctl = cs_ctl->priv; - - return snd_soc_component_notify_control(dsp->component, ctl->name); + return 0; } EXPORT_SYMBOL_GPL(wm_adsp_write_ctl);
From: Simon Trimmer simont@opensource.cirrus.com
Any control that the driver is updating should be marked as SYSTEM and therefore will not have an ALSA control to notify.
Signed-off-by: Simon Trimmer simont@opensource.cirrus.com Signed-off-by: Richard Fitzgerald rf@opensource.cirrus.com --- sound/pci/hda/hda_cs_dsp_ctl.c | 8 -------- 1 file changed, 8 deletions(-)
diff --git a/sound/pci/hda/hda_cs_dsp_ctl.c b/sound/pci/hda/hda_cs_dsp_ctl.c index 725544d81941..7f2d35cf245b 100644 --- a/sound/pci/hda/hda_cs_dsp_ctl.c +++ b/sound/pci/hda/hda_cs_dsp_ctl.c @@ -201,7 +201,6 @@ int hda_cs_dsp_write_ctl(struct cs_dsp *dsp, const char *name, int type, unsigned int alg, const void *buf, size_t len) { struct cs_dsp_coeff_ctl *cs_ctl; - struct hda_cs_dsp_coeff_ctl *ctl; int ret;
mutex_lock(&dsp->pwr_lock); @@ -211,13 +210,6 @@ int hda_cs_dsp_write_ctl(struct cs_dsp *dsp, const char *name, int type, if (ret < 0) return ret;
- if (ret == 0 || (cs_ctl->flags & WMFW_CTL_FLAG_SYS)) - return 0; - - ctl = cs_ctl->priv; - - snd_ctl_notify(ctl->card, SNDRV_CTL_EVENT_MASK_VALUE, &ctl->kctl->id); - return 0; } EXPORT_SYMBOL_NS_GPL(hda_cs_dsp_write_ctl, SND_HDA_CS_DSP_CONTROLS);
On Mon, 25 Mar 2024 12:31:22 +0100, Richard Fitzgerald wrote:
This set of patches factors out some repeated code to clean up firmware control read/write functions, and removes some redundant control notification code.
Simon Trimmer (5): firmware: cs_dsp: Add locked wrappers for coeff read and write ASoC: wm_adsp: Use cs_dsp_coeff_lock_and_[read|write]_ctrl() ALSA: hda: hda_cs_dsp_ctl: Use cs_dsp_coeff_lock_and_[read|write]_ctrl() ASoC: wm_adsp: Remove notification of driver write ALSA: hda: hda_cs_dsp_ctl: Remove notification of driver write
The patch 4 doesn't look cleanly applicable to my tree. Should it be applied via Mark's tree?
thanks,
Takashi
On 30/03/2024 08:40, Takashi Iwai wrote:
On Mon, 25 Mar 2024 12:31:22 +0100, Richard Fitzgerald wrote:
This set of patches factors out some repeated code to clean up firmware control read/write functions, and removes some redundant control notification code.
Simon Trimmer (5): firmware: cs_dsp: Add locked wrappers for coeff read and write ASoC: wm_adsp: Use cs_dsp_coeff_lock_and_[read|write]_ctrl() ALSA: hda: hda_cs_dsp_ctl: Use cs_dsp_coeff_lock_and_[read|write]_ctrl() ASoC: wm_adsp: Remove notification of driver write ALSA: hda: hda_cs_dsp_ctl: Remove notification of driver write
The patch 4 doesn't look cleanly applicable to my tree. Should it be applied via Mark's tree?
Yes, it will need to go through Mark's tree. Mark's for-next has one extra patch to wm_adsp.c that changes the same function:
f193957b0fbb ("ASoC: wm_adsp: Fix missing mutex_lock in wm_adsp_write_ctl()")
On Mon, 01 Apr 2024 11:32:49 +0200, Richard Fitzgerald wrote:
On 30/03/2024 08:40, Takashi Iwai wrote:
On Mon, 25 Mar 2024 12:31:22 +0100, Richard Fitzgerald wrote:
This set of patches factors out some repeated code to clean up firmware control read/write functions, and removes some redundant control notification code.
Simon Trimmer (5): firmware: cs_dsp: Add locked wrappers for coeff read and write ASoC: wm_adsp: Use cs_dsp_coeff_lock_and_[read|write]_ctrl() ALSA: hda: hda_cs_dsp_ctl: Use cs_dsp_coeff_lock_and_[read|write]_ctrl() ASoC: wm_adsp: Remove notification of driver write ALSA: hda: hda_cs_dsp_ctl: Remove notification of driver write
The patch 4 doesn't look cleanly applicable to my tree. Should it be applied via Mark's tree?
Yes, it will need to go through Mark's tree. Mark's for-next has one extra patch to wm_adsp.c that changes the same function:
f193957b0fbb ("ASoC: wm_adsp: Fix missing mutex_lock in wm_adsp_write_ctl()")
OK, then it should go via Mark's tree. Feel free to take my ack:
Reviewed-by: Takashi Iwai tiwai@suse.de
thanks,
Takashi
On Mon, 25 Mar 2024 11:31:22 +0000, Richard Fitzgerald wrote:
This set of patches factors out some repeated code to clean up firmware control read/write functions, and removes some redundant control notification code.
Simon Trimmer (5): firmware: cs_dsp: Add locked wrappers for coeff read and write ASoC: wm_adsp: Use cs_dsp_coeff_lock_and_[read|write]_ctrl() ALSA: hda: hda_cs_dsp_ctl: Use cs_dsp_coeff_lock_and_[read|write]_ctrl() ASoC: wm_adsp: Remove notification of driver write ALSA: hda: hda_cs_dsp_ctl: Remove notification of driver write
[...]
Applied to
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next
Thanks!
[1/5] firmware: cs_dsp: Add locked wrappers for coeff read and write commit: 4d0333798ebbfa1683cc3bc056d1b25b8c24344c [2/5] ASoC: wm_adsp: Use cs_dsp_coeff_lock_and_[read|write]_ctrl() commit: 3802a9969bd365749f5e34928082cff96ed7769b [3/5] ALSA: hda: hda_cs_dsp_ctl: Use cs_dsp_coeff_lock_and_[read|write]_ctrl() commit: 62daf3df8a6b1920f7613e478935443a8f449708 [4/5] ASoC: wm_adsp: Remove notification of driver write commit: e81f5c9f7d06a69dc505fa6ad351df6cc86a6c2d [5/5] ALSA: hda: hda_cs_dsp_ctl: Remove notification of driver write commit: d641def12ec929af6c4f9b1b28efcd3e5dff21b4
All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying to this mail.
Thanks, Mark
participants (3)
-
Mark Brown -
Richard Fitzgerald -
Takashi Iwai