[PATCH 0/5] ALSA: cirrus: Tidy up of firmware control read/write

[PATCH 0/5] ALSA: cirrus: Tidy up of firmware control read/write

[Richard Fitzgerald]

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

-- 
2.39.2

[PATCH 1/5] firmware: cs_dsp: Add locked wrappers for coeff read and write

[Richard Fitzgerald]

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

[PATCH 2/5] ASoC: wm_adsp: Use cs_dsp_coeff_lock_and_[read|write]_ctrl()

[Richard Fitzgerald]

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,
-- 
2.39.2

[PATCH 3/5] ALSA: hda: hda_cs_dsp_ctl: Use cs_dsp_coeff_lock_and_[read|write]_ctrl()

[Richard Fitzgerald]

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

[PATCH 4/5] ASoC: wm_adsp: Remove notification of driver write

[Richard Fitzgerald]

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

[PATCH 5/5] ALSA: hda: hda_cs_dsp_ctl: Remove notification of driver write

[Richard Fitzgerald]

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

Re: [PATCH 0/5] ALSA: cirrus: Tidy up of firmware control read/write

[Takashi Iwai]

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

Re: [PATCH 0/5] ALSA: cirrus: Tidy up of firmware control read/write

[Richard Fitzgerald]

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()")

Re: [PATCH 0/5] ALSA: cirrus: Tidy up of firmware control read/write

[Takashi Iwai]

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

Re: [PATCH 0/5] ALSA: cirrus: Tidy up of firmware control read/write

[Mark Brown]

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