On Fri, Sep 19, 2014 at 04:46:04PM +0530, Subhransu S. Prusty wrote:
+static int sst_slot_get(struct snd_kcontrol *kcontrol,
struct snd_ctl_elem_value *ucontrol)+{
struct sst_enum *e = (void *)kcontrol->private_value;struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);unsigned int ctl_no = e->reg;unsigned int is_tx = e->tx;unsigned int val, mux;u8 *map = is_tx ? sst_ssp_channel_map : sst_ssp_slot_map;
So the "channel" map is for transmit and the "slot" map is for receive? That naming doesn't seem at all obvious, I'd expect some confusion and resulting bugs there.
This is a really big block of code and there's lots of things like this in the code which may well *work* but don't seem robust - not flagging up easy to detect errors for example - and I've been spotting things like locking trouble as well. This is all setting off alarm bells which is worrying.
- mutex_lock(&drv->lock);
- /* first clear all registers of this bit */
- for (i = 0; i < e->max; i++)
map[i] &= ~val;- if (mux == 0) {/* kctl set to 'none' */
mutex_unlock(&drv->lock);return 0;- }
It's still not at all clear to me why we don't need to interact with the hardware if we're settinng this to zero. AFAICT from the previous discussion this comment at the top of the file:
- In the dpcm driver modelling when a particular FE/BE/Mixer/Pipe is active
- we forward the settings and parameters, rest we keep the values in
- driver and forward when DAPM enables them
is supposed to explain what's going on but since it's nowhere near the code it's unlikely that people will have seen it and it's not terribly easy to relate to the code here. As far as I can tell the theory is that something is going to trigger a power down of the DSP and it won't matter but if this is the case it isn't at all clear from the immediate code.
- if (type == SST_MODULE_GAIN) {
struct sst_gain_mixer_control *mc = (void *)kctl->private_value;mc->w = w;module->kctl = kctl;list_add_tail(&module->node, &ids->gain_list);- } else if (type == SST_MODULE_ALGO) {
struct sst_algo_control *bc = (void *)kctl->private_value;bc->w = w;module->kctl = kctl;list_add_tail(&module->node, &ids->algo_list);- }
This looks like it should be a switch statement with a default case to trap any errors.
if (idx == NULL)continue;index = strlen(kctl->id.name) - strlen(idx);
I keep on seeing lots of code with random double instead of single spaces.
if (strstr(kctl->id.name, "Volume") &&!strncmp(kctl->id.name, w->name, index))ret = sst_fill_module_list(kctl, w, SST_MODULE_GAIN);else if (strstr(kctl->id.name, "params") &&!strncmp(kctl->id.name, w->name, index))ret = sst_fill_module_list(kctl, w, SST_MODULE_ALGO);else if (strstr(kctl->id.name, "Switch") &&!strncmp(kctl->id.name, w->name, index)) {struct sst_gain_mixer_control *mc =(void *)kctl->private_value;mc->w = w;} else if (strstr(kctl->id.name, "interleaver") &&
Both or no branches of an if statement should use { }.
!strncmp(kctl->id.name, w->name, index)) {struct sst_enum *e = (void *)kctl->private_value;e->w = w;} else if (strstr(kctl->id.name, "deinterleaver") &&!strncmp(kctl->id.name, w->name, index)) {struct sst_enum *e = (void *)kctl->private_value;e->w = w;}
Again no fallthrough case.