lkml.org 
[lkml]   [2014]   [Nov]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v6] ASoC: dapm: add code to configure dai link parameters
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.
[unhandled content-type:application/pgp-signature]
\
 
 \ /
  Last update: 2014-11-04 21:21    [W:0.548 / U:1.260 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site