Re: [PATCH v7 3/7] ASoC: Intel: mrfld: add DSP core controls

[Vinod Koul <vinod.koul@intel.com>]

[-- Attachment #1.1: Type: text/plain, Size: 4999 bytes --]

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
-- 
~Vinod

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]