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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
