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.