Alsa-Devel Archive on lore.kernel.org
 help / color / Atom feed
* [alsa-devel] [PATCH] ASoC: wm_adsp: Expose mixer control API
@ 2019-10-31 19:36 David Rhodes
  2019-11-04  9:47 ` Charles Keepax
  2019-11-04 12:58 ` Mark Brown
  0 siblings, 2 replies; 4+ messages in thread
From: David Rhodes @ 2019-10-31 19:36 UTC (permalink / raw)
  To: ckeepax, david.rhodes, broonie, lgirdwood, patches, alsa-devel; +Cc: Li Xu

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
-- 
1.9.1

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [alsa-devel] [PATCH] ASoC: wm_adsp: Expose mixer control API
  2019-10-31 19:36 [alsa-devel] [PATCH] ASoC: wm_adsp: Expose mixer control API David Rhodes
@ 2019-11-04  9:47 ` Charles Keepax
  2019-11-04 12:58 ` Mark Brown
  1 sibling, 0 replies; 4+ messages in thread
From: Charles Keepax @ 2019-11-04  9:47 UTC (permalink / raw)
  To: David Rhodes; +Cc: patches, alsa-devel, broonie, lgirdwood, Li Xu

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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [alsa-devel] [PATCH] ASoC: wm_adsp: Expose mixer control API
  2019-10-31 19:36 [alsa-devel] [PATCH] ASoC: wm_adsp: Expose mixer control API David Rhodes
  2019-11-04  9:47 ` Charles Keepax
@ 2019-11-04 12:58 ` Mark Brown
  2019-11-06 23:04   ` [alsa-devel] [EXTERNAL] " Rhodes, David
  1 sibling, 1 reply; 4+ messages in thread
From: Mark Brown @ 2019-11-04 12:58 UTC (permalink / raw)
  To: David Rhodes; +Cc: patches, alsa-devel, ckeepax, lgirdwood, Li Xu

[-- Attachment #1.1: Type: text/plain, Size: 1417 bytes --]

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.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 161 bytes --]

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [alsa-devel] [EXTERNAL] Re: [PATCH] ASoC: wm_adsp: Expose mixer control API
  2019-11-04 12:58 ` Mark Brown
@ 2019-11-06 23:04   ` " Rhodes, David
  0 siblings, 0 replies; 4+ messages in thread
From: Rhodes, David @ 2019-11-06 23:04 UTC (permalink / raw)
  To: Mark Brown; +Cc: patches, alsa-devel, ckeepax, lgirdwood, Xu, Li

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.
    

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, back to index

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-31 19:36 [alsa-devel] [PATCH] ASoC: wm_adsp: Expose mixer control API David Rhodes
2019-11-04  9:47 ` Charles Keepax
2019-11-04 12:58 ` Mark Brown
2019-11-06 23:04   ` [alsa-devel] [EXTERNAL] " Rhodes, David

Alsa-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/alsa-devel/0 alsa-devel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 alsa-devel alsa-devel/ https://lore.kernel.org/alsa-devel \
		alsa-devel@alsa-project.org
	public-inbox-index alsa-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.alsa-project.alsa-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git