diff mbox series

ASoC: wm_adsp: Expose mixer control API

Message ID 1572550603-21560-1-git-send-email-david.rhodes@cirrus.com (mailing list archive)
State New, archived
Headers show
Series ASoC: wm_adsp: Expose mixer control API | expand

Commit Message

David Rhodes Oct. 31, 2019, 7:36 p.m. UTC 
From: Li Xu <li.xu@cirrus.com>

Expose mixer control API for reading and writing.
The exposed API can be used by codec driver for
interacting with mixer control in kernel space.
This allows codec driver to implement more involved
interactions with DSP firmware, such as Fast Use
Case Switching.

Signed-off-by: Li Xu <li.xu@cirrus.com>
Signed-off-by: David Rhodes <david.rhodes@cirrus.com>
---
 sound/soc/codecs/wm_adsp.c | 70 +++++++++++++++++++++++++++++++++++++++++++++-
 sound/soc/codecs/wm_adsp.h |  4 +++
 2 files changed, 73 insertions(+), 1 deletion(-)

Comments

Mark Brown Nov. 4, 2019, 12:58 p.m. UTC | #1 
On Thu, Oct 31, 2019 at 02:36:43PM -0500, David Rhodes wrote:

> From: Li Xu <li.xu@cirrus.com>

> Expose mixer control API for reading and writing.
> The exposed API can be used by codec driver for
> interacting with mixer control in kernel space.
> This allows codec driver to implement more involved
> interactions with DSP firmware, such as Fast Use
> Case Switching.

It would be helpful if somewhere in the changelog you more explicitly
said that this was an in-kernel API, it isn't very clear what the API
you're adding is supposed to be.

The formatting here is also a bit weird, the lines are very short.

> +/*
> + * Find wm_coeff_ctl with input name as its subname
> + * If not found, return NULL
> + */
> +static struct wm_coeff_ctl *wm_adsp_get_ctl(struct wm_adsp *dsp,
> +                                            const char *name)

It is not clear why we only look things up by subname.  What's wrong
with the rest of the name?

> +int wm_adsp_write_ctl(struct wm_adsp *dsp, const char *name, const void *buf,
> +		      size_t len)
> +{
> +	struct wm_coeff_ctl *ctl;
> +
> +	ctl = wm_adsp_get_ctl(dsp, name);
> +	if (!ctl)
> +		return -EINVAL;
> +
> +	if (len > ctl->len)
> +		return -EINVAL;
> +
> +	return wm_coeff_write_control(ctl, buf, len);
> +}
> +EXPORT_SYMBOL_GPL(wm_adsp_write_ctl);

There should be a snd_ctl_notify() somewhere in the write path to tell
userspace that the value changed.
David Rhodes Nov. 6, 2019, 11:04 p.m. UTC | #2 
Thanks for the review.

> It is not clear why we only look things up by subname.  What's wrong
> with the rest of the name?

The reason we use the subname for lookup is because the subname portion of the name can be truncated when the length of the subname + prefixes exceeds the maximum.
However, you raise a good point that the same subname by itself may not be a unique identifier, and the same subname could be associated with more than one algorithm.

I will send an updated version after internal review.

Thanks,
David

On 11/4/19, 6:58 AM, "Mark Brown" <broonie@kernel.org> wrote:

    On Thu, Oct 31, 2019 at 02:36:43PM -0500, David Rhodes wrote:
    
    > From: Li Xu <li.xu@cirrus.com>
    
    > Expose mixer control API for reading and writing.
    > The exposed API can be used by codec driver for
    > interacting with mixer control in kernel space.
    > This allows codec driver to implement more involved
    > interactions with DSP firmware, such as Fast Use
    > Case Switching.
    
    It would be helpful if somewhere in the changelog you more explicitly
    said that this was an in-kernel API, it isn't very clear what the API
    you're adding is supposed to be.
    
    The formatting here is also a bit weird, the lines are very short.
    
    > +/*
    > + * Find wm_coeff_ctl with input name as its subname
    > + * If not found, return NULL
    > + */
    > +static struct wm_coeff_ctl *wm_adsp_get_ctl(struct wm_adsp *dsp,
    > +                                            const char *name)
    
    It is not clear why we only look things up by subname.  What's wrong
    with the rest of the name?
    
    > +int wm_adsp_write_ctl(struct wm_adsp *dsp, const char *name, const void *buf,
    > +		      size_t len)
    > +{
    > +	struct wm_coeff_ctl *ctl;
    > +
    > +	ctl = wm_adsp_get_ctl(dsp, name);
    > +	if (!ctl)
    > +		return -EINVAL;
    > +
    > +	if (len > ctl->len)
    > +		return -EINVAL;
    > +
    > +	return wm_coeff_write_control(ctl, buf, len);
    > +}
    > +EXPORT_SYMBOL_GPL(wm_adsp_write_ctl);
    
    There should be a snd_ctl_notify() somewhere in the write path to tell
    userspace that the value changed.
diff mbox series

Patch

diff --git a/sound/soc/codecs/wm_adsp.c b/sound/soc/codecs/wm_adsp.c
index aa74fd1..77b8aea 100644
--- a/sound/soc/codecs/wm_adsp.c
+++ b/sound/soc/codecs/wm_adsp.c
@@ -635,6 +635,9 @@  struct wm_coeff_ctl_ops {
 struct wm_coeff_ctl {
 	const char *name;
 	const char *fw_name;
+	/* Subname is needed to match with firmware */
+	const char *subname;
+	unsigned int subname_len;
 	struct wm_adsp_alg_region alg_region;
 	struct wm_coeff_ctl_ops ops;
 	struct wm_adsp *dsp;
@@ -1520,6 +1523,7 @@  static void wm_adsp_free_ctl_blk(struct wm_coeff_ctl *ctl)
 {
 	kfree(ctl->cache);
 	kfree(ctl->name);
+	kfree(ctl->subname);
 	kfree(ctl);
 }
 
@@ -1597,6 +1601,15 @@  static int wm_adsp_create_control(struct wm_adsp *dsp,
 		ret = -ENOMEM;
 		goto err_ctl;
 	}
+	if (subname) {
+		ctl->subname_len = subname_len;
+		ctl->subname = kmemdup(subname,
+				       strlen(subname) + 1, GFP_KERNEL);
+		if (!ctl->subname) {
+			ret = -ENOMEM;
+			goto err_ctl_name;
+		}
+	}
 	ctl->enabled = 1;
 	ctl->set = 0;
 	ctl->ops.xget = wm_coeff_get;
@@ -1610,7 +1623,7 @@  static int wm_adsp_create_control(struct wm_adsp *dsp,
 	ctl->cache = kzalloc(ctl->len, GFP_KERNEL);
 	if (!ctl->cache) {
 		ret = -ENOMEM;
-		goto err_ctl_name;
+		goto err_ctl_subname;
 	}
 
 	list_add(&ctl->list, &dsp->ctl_list);
@@ -1633,6 +1646,8 @@  static int wm_adsp_create_control(struct wm_adsp *dsp,
 
 err_ctl_cache:
 	kfree(ctl->cache);
+err_ctl_subname:
+	kfree(ctl->subname);
 err_ctl_name:
 	kfree(ctl->name);
 err_ctl:
@@ -2194,6 +2209,59 @@  static int wm_adsp_load(struct wm_adsp *dsp)
 	return ret;
 }
 
+/*
+ * Find wm_coeff_ctl with input name as its subname
+ * If not found, return NULL
+ */
+static struct wm_coeff_ctl *wm_adsp_get_ctl(struct wm_adsp *dsp,
+					     const char *name)
+{
+	struct wm_coeff_ctl *pos, *rslt = NULL;
+
+	list_for_each_entry(pos, &dsp->ctl_list, list) {
+		if (!pos->subname)
+			continue;
+		if (strncmp(pos->subname, name, pos->subname_len) == 0) {
+			rslt = pos;
+			break;
+		}
+	}
+
+	return rslt;
+}
+
+int wm_adsp_write_ctl(struct wm_adsp *dsp, const char *name, const void *buf,
+		      size_t len)
+{
+	struct wm_coeff_ctl *ctl;
+
+	ctl = wm_adsp_get_ctl(dsp, name);
+	if (!ctl)
+		return -EINVAL;
+
+	if (len > ctl->len)
+		return -EINVAL;
+
+	return wm_coeff_write_control(ctl, buf, len);
+}
+EXPORT_SYMBOL_GPL(wm_adsp_write_ctl);
+
+int wm_adsp_read_ctl(struct wm_adsp *dsp, const char *name, void *buf,
+		     size_t len)
+{
+	struct wm_coeff_ctl *ctl;
+
+	ctl = wm_adsp_get_ctl(dsp, name);
+	if (!ctl)
+		return -EINVAL;
+
+	if (len > ctl->len)
+		return -EINVAL;
+
+	return wm_coeff_read_control(ctl, buf, len);
+}
+EXPORT_SYMBOL_GPL(wm_adsp_read_ctl);
+
 static void wm_adsp_ctl_fixup_base(struct wm_adsp *dsp,
 				  const struct wm_adsp_alg_region *alg_region)
 {
diff --git a/sound/soc/codecs/wm_adsp.h b/sound/soc/codecs/wm_adsp.h
index 5275a49..2a8cd09 100644
--- a/sound/soc/codecs/wm_adsp.h
+++ b/sound/soc/codecs/wm_adsp.h
@@ -223,5 +223,9 @@  int wm_adsp_compr_pointer(struct snd_compr_stream *stream,
 			  struct snd_compr_tstamp *tstamp);
 int wm_adsp_compr_copy(struct snd_compr_stream *stream,
 		       char __user *buf, size_t count);
+int wm_adsp_write_ctl(struct wm_adsp *dsp, const char *name, const void *buf,
+		      size_t len);
+int wm_adsp_read_ctl(struct wm_adsp *dsp, const char *name, void *buf,
+		     size_t len);
 
 #endif