Hello,
On Monday 24 November 2008 15:48:09 ext Mark Brown wrote:
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.
I have seen this approach in several codec code, so I have also implemented in a same way. I will change it. Do you have a pointer, where should I look for existing code?
- 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.
Hmmm, the situation is kind of both... The selection is done in a bitfield. Let's take the PREDL, as I used that in the helper function comment: bit 0 (0x1): voice_en bit 1 (0x2): Audio L1 enable (DACL1) bit 2 (0x4): Audio L2 enable (DACL2) bit 3 (0x8): Audio R2 enable (DACR2) The voice path can be enabled/disabled independently from the digital paths, but only _one_ of the DACL1, DACL2 or DACR2 can be selected at the time. On top of that the mute can be performed by setting all four bits to 0. I don't know actually what that means, but probably than the PreDriv path will be effectively turned off. On the side not: the voice path at the moment not in use -> the codec has to be in different mode to have the voice path enabled. But then lot's of things will behave differently. Initially I did not wanted to spread these controls - to make it simple for myself, but I might separate the voice and DAC mux selections (separated enable for voice and mux the DACL/Rs selection).
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.
OK, I'll try to move the controls to DAPM, as you suggested.
Thanks, Péter