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

From: Mark Brown <broonie@kernel.org>
To: "Subhransu S. Prusty" <subhransu.s.prusty@intel.com>
Cc: vinod.koul@intel.com, alsa-devel@alsa-project.org,
	Lars-Peter Clausen <lars@metafoo.de>,
	lgirdwood@gmail.com
Subject: Re: [PATCH v7 1/7] ASoC: Intel: mrfld: add the gain controls
Date: Thu, 25 Sep 2014 15:20:26 +0100	[thread overview]
Message-ID: <20140925142026.GS27755@sirena.org.uk> (raw)
In-Reply-To: <1411125368-5836-2-git-send-email-subhransu.s.prusty@intel.com>

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

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.

> +#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.

>  	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.

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

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