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.