On Thu, Sep 25, 2014 at 04:08:13PM +0100, Mark Brown wrote:
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.
yes as multiple channels get interleaved to slot while transmitting and on recive side various lots get de-interleaved to channels :) These controls allow you to route stuff.
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.
Ok will recheck this part and fix wherever required.
- 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.
None is just a SW state thats why :)
The DSP has the bitmap for interlever as explained in sst_send_slot_map() /* * channel map value is a bitfield where each bit represents a slot * * 76543210 # 0 = slot 0, 1 = slot 1 * * e.g. codec1_0 tx map = 00000101b -> data from codec_out1_0 goes into slot * 0, 2 */ so based on the channel selected here we will set the map and send to DSP. For none case we don't need to do anything. Yes this bit need this explanation so will add up
- 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.
this shouldn't be, will fix
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 { }.
will fix
!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.
would that be valid. We have only volume, params, mute, interlevaer and deinterleaver controls only :) Can add fall thru for therotical if you feel that is right thing to do.
Thanks