[alsa-devel] [PATCH 5/9] ASoC: TWL4030: Add PreDriv outupt mux and volume controls

[Mark Brown]

On Mon, Nov 24, 2008 at 01:49:39PM +0200, Peter Ujfalusi wrote:

> +static const char *twl4030_predrivl_mix[] = {"Off", "Voice", "DACL1",
> +					    "DACL2", "DACR2"};
> +static const char *twl4030_predrivr_mix[] = {"Off", "Voice", "DACR1",
> +					    "DACR2", "DACL2"};

> +static const struct soc_enum twl4030_enum[] = {

I know quite a few existing drivers use an array of enums but please
don't do this for new code - it doesn't help legibility to have to find
the enums in the table.

> +	SOC_ENUM_SINGLE(TWL4030_REG_PREDL_CTL, 0, 5, twl4030_predrivl_mix),
> +	SOC_ENUM_SINGLE(TWL4030_REG_PREDR_CTL, 0, 5, twl4030_predrivr_mix),
> +};

Is this really an enum?  The fact that it's described as a mixer and
it's got a series of individual bits for selecting the inputs make it
sound like it should be possible to enable multiple inputs at once in
which case this should be a series of switches - see SND_SOC_DAPM_MIXERs
in other drivers for examples.  If it's really an enum then calling it a
mux is probably better.

Another issue is that it might be better to add these as DAPM controls
now rather than have to move them to be DAPM controls later.  If you use
SND_SOC_NOPM for the power bit then DAPM won't actually do any power
control, it'll just expose the control.

These comments apply to the other patches in the series.