Messages in this thread | | | Date | Tue, 4 Nov 2014 19:40:49 +0000 | From | Mark Brown <> | Subject | Re: [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] | |