On Thu, Sep 25, 2014 at 09:38:53PM +0530, Vinod Koul wrote:
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:
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.
I can tell what they're supposed to do given the above code, what I'm saying is that when I see the above code I'm expecting to find bugs elsewhere since it's not going to be clear if you see either variable in isolation. Better naming please, for example use the direction.
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
But why not - why is the effect of clearing all the bits to do nothing?
!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.
Yes, it's better. This is part of the whole thing about the code setting off alarm bells - things look fragile and the missing error checking does nothing to dispel that.