[alsa-devel] [EXTERNAL] Re: [PATCH] ASoC: wm_adsp: Expose mixer control API

[Rhodes, David]

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 at kernel.org> wrote:

    On Thu, Oct 31, 2019 at 02:36:43PM -0500, David Rhodes wrote:
    
    > From: Li Xu <li.xu at 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.