[alsa-devel] [PATCH v6] ASoC: dapm: add code to configure dai link parameters
Mark Brown
broonie at kernel.org
Tue Nov 4 20:40:49 CET 2014
On Fri, Sep 12, 2014 at 10:11:04AM +0100, Nikesh Oswal wrote:
> dai-link params for codec-codec links were fixed. The fixed
> link between codec and another chip which may be another codec,
> baseband, bluetooth codec etc may require run time configuaration
> changes. This change provides an optional alsa control to select
> one of the params from a list of params.
The shape of this looks OK, though there are some issues below. I
intend to test this before applying but couldn't do that since it
doesn't apply against current code.
> + /* Select the configuration set by alsa control */
> + config += w->params_select;
> +
Do an array lookup, code legibility is important.
> +static int snd_soc_dapm_dai_link_put(struct snd_kcontrol *kcontrol,
> + struct snd_ctl_elem_value *ucontrol)
> +{
> + struct snd_soc_dapm_widget *w = snd_kcontrol_chip(kcontrol);
> +
> + if (ucontrol->value.integer.value[0] == w->params_select)
> + return 0;
> +
> + if (ucontrol->value.integer.value[0] >= w->num_params)
> + return -EINVAL;
> +
> + w->params_select = ucontrol->value.integer.value[0];
> +
> + return 0;
> +}
This will just silently not immediately do anything if an attempt is
made to change the parameters while the link is active. There needs to
at least be a log message, and probably it's better to return -EBUSY
as well otherwise userspace might be going on reconfiguring things
assuming that this succeeded.
It'd be even better to actually reconfigure the link but that's a more
involved operation which might need us to power things down and can be
punted for now.
> len = strlen(source->name) + strlen(sink->name) + 2;
> link_name = devm_kzalloc(card->dev, len, GFP_KERNEL);
> - if (!link_name)
> - return -ENOMEM;
> + if (!link_name) {
> + ret = -ENOMEM;
> + goto outfree_w_param;
> + }
Random extra space here and in some other gotos.
> + for (count = 0 ; count < num_params; count++) {
> + if (!config->stream_name)
> + dev_warn(card->dapm.dev,
> + "ASoC: anonymous config %d for dai link %s\n",
> + count, link_name);
> + w_param_text[count] = kmemdup((void *)(config->stream_name),
> + strlen(config->stream_name) + 1, GFP_KERNEL);
Why are you casting to void * here? This looks like it's open coding
kstrdup() and we're mixing devm_ and non-devm_ allocations in this
function.
This is also dereferencing stream_name immediately after finding that
it's NULL which isn't good.
> + template.num_kcontrols = 1;
> + private_value =
> + (unsigned long) kmemdup((void *)(kcontrol_dai_link[0].private_value),
> + sizeof(struct soc_enum), GFP_KERNEL);
> + if (!private_value) {
> + dev_err(card->dev, "ASoC: Failed to create control for %s widget\n",
So, we need to kmemdup() this thing that we just allocated? If this is
needed it should be clear to the reader why we're doing this, especially
given all the funky casts.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <http://mailman.alsa-project.org/pipermail/alsa-devel/attachments/20141104/d008e681/attachment.sig>
More information about the Alsa-devel
mailing list