Re: [PATCH v7 1/7] ASoC: Intel: mrfld: add the gain controls

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

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

On Thu, Sep 25, 2014 at 03:20:26PM +0100, Mark Brown wrote:
> On Fri, Sep 19, 2014 at 04:46:02PM +0530, Subhransu S. Prusty wrote:
> 
> > +/* Gain helper with min/max set */
> > +#define SST_GAIN(name, path_id, task_id, instance, gain_var)				\
> > +	SST_GAIN_KCONTROLS(name, "Gain", SST_GAIN_MIN_VALUE, SST_GAIN_MAX_VALUE,	\
> > +		SST_GAIN_TC_MIN, SST_GAIN_TC_MAX,					\
> 
> As far as I can tell from following the macros through this looks like
> it will create controls called "name Gain Volume" which doesn't seem
> excellent.
The convenstion we used was <pipeline identifer> and then module. So if gain
block is in media 0 input we refer to it as "media 0 in Gain Volume"
This helps to identify the module location and help in understanding control
by looking at the name
> 
> > +#define SST_NUM_GAINS 36
> > +static struct sst_gain_value sst_gains[SST_NUM_GAINS];
> 
> > +static const struct snd_kcontrol_new sst_gain_controls[] = {
> > +	SST_GAIN("media0_in", SST_PATH_INDEX_MEDIA0_IN, SST_TASK_MMX, 0, &sst_gains[0]),
> 
> This table is full of magic number references into the array...
> 
> > +	SST_GAIN("codec_out1", SST_PATH_INDEX_CODEC_OUT1, SST_TASK_SBA, 0, &sst_gains[23]),
> > +	SST_GAIN("media_loop1_out", SST_PATH_INDEX_MEDIA_LOOP1_OUT, SST_TASK_SBA, 0, &sst_gains[30]),
> 
> ...with holes in the array.  This seems really error prone.
I will fix the holes here..

> >  	char params[0];
> >  } __packed;
> > +/**** widget defines *****/
> > +
> > +#include <sound/soc.h>
> > +#include <sound/tlv.h>
> > +struct sst_ids {
> 
> This is adding includes in the middle of the file (which isn't the usual
> kernel coding style) and is missing a blank lines.  This sort of stuff
> is getting quite repetitive and hence annoying, especially the blank
> lines thing which comes up again and again and just makes everything
> even harder to read.
Sorry about this one, i will make sure this is fixed properly and not
repeated

-- 
~Vinod

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

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