[alsa-devel] [PATCH] ASoC: wm_adsp: Expose mixer control API
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(-)
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
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.
Signed-off-by: Li Xu li.xu@cirrus.com Signed-off-by: David Rhodes david.rhodes@cirrus.com
Overall I think we have seen enough valid use-cases for this, but we do need to be a little careful to police its usage.
Acked-by: Charles Keepax ckeepax@opensource.cirrus.com
Thanks, Charles
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.
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.
participants (4)
-
Charles Keepax -
David Rhodes -
Mark Brown -
Rhodes, David