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

From: Vinod Koul <vinod.koul@intel.com>
To: Mark Brown <broonie@kernel.org>
Cc: alsa-devel@alsa-project.org, Lars-Peter Clausen <lars@metafoo.de>,
	"Subhransu S. Prusty" <subhransu.s.prusty@intel.com>,
	lgirdwood@gmail.com
Subject: Re: [PATCH v7 3/7] ASoC: Intel: mrfld: add DSP core controls
Date: Mon, 29 Sep 2014 11:30:09 +0530	[thread overview]
Message-ID: <20140929060008.GM1638@intel.com> (raw)
In-Reply-To: <20140927111333.GH27755@sirena.org.uk>


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

On Sat, Sep 27, 2014 at 12:13:33PM +0100, Mark Brown wrote:
> 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.
Sure will add tx and rx to the control names to make it explict. Thanks for
the suggestion

> > 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?
Okay thanks for pointing out, We did check on this bit. So initially DSP
wasn't supporting the dynamic updates of the  slots so thsi was fine but
later we added support. On re-examining again as you pointed yes 'none'
value needs to reset the bitmap and send to DSP so we just need to forward
the IPC here as well. I will fix it up.

> > > > +			 !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.
Yup, will add these

-- 
~Vinod

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

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