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

From: Mark Brown <broonie@kernel.org>
To: Vinod Koul <vinod.koul@intel.com>
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: Sat, 27 Sep 2014 12:13:33 +0100	[thread overview]
Message-ID: <20140927111333.GH27755@sirena.org.uk> (raw)
In-Reply-To: <20140925160853.GB1638@intel.com>

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

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.

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

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