At Thu, 09 Aug 2007 21:44:46 +0200,
Joachim Förster wrote:
>
> Hi Takashi,
>
> before posting a corrected version, I would like to ask some unclear
> things (I think I understood the rest):
>
> On Thu, 2007-08-09 at 19:13 +0200, Takashi Iwai wrote:
> > (snip)
> > > +static int index[SNDRV_CARDS] = SNDRV_DEFAULT_IDX;
> > > +static char *id[SNDRV_CARDS] = SNDRV_DEFAULT_STR;
> > > +static int enable[SNDRV_CARDS] = SNDRV_DEFAULT_ENABLE;
> >
> > Can the driver really support multiple instances?
> > If not, better to avoid these arrays.
>
> Well, while I wrote the driver I considered that there might be more
> than one instance - though I didn't test it and I won't be able to test
> it (no such hardware available - with more than one LM4550 chip).
> So, shall I remove it?
It's up to you. If it makes the code easier and more maintenable,
it's worth to try.
> > > +struct lm4550_reg lm4550_regfile[64] = {
> > > + {.reg = 0x00,
> > > + .flag = LM4550_REG_NOSAVE | LM4550_REG_FAKEREAD,
> > > + .def = 0x0D50},
> >
> > Better to use C99 style initialization here, e.g.
> >
> > [0x00] = { .... },
> > [0x02] = { .... },
> > [0x7e] = { .... },
> >
> > so you can avoid writing empty items.
> > The index value could be also AC97_XXX, such as [AC97_MASTER] =
> > {...}.
> >
> > What is the purpose of reg field at all, BTW? I guess it's
> > superfluous.
>
> No, it's there to provide a shadow copy of the codec's (LM4550)
> registers.
That I understood too. My question was the reg _field_. It looks
like an index but it can be even easily calculated from the pointer
address...
> It contains default values and (while driver is running)
> current values, which were written to the hardware. I had to introduce
> this, because Xilinx's AC97 Controller Reference has a very ugly bug: It
> provides a "register access finish" bit, so the driver is able to tell
> when a register read or write access is finished. Unfortunately this
> particular bit only works in the range of 0 to 100 times since board
> reset. After that many register access it just remains saying: I'm _not_
> ready. But in fact, in most cases (98%) the correct value already is the
> read register (assume we have just said to the control that we want to
> read a register).
> I thought, ok we have such RegAccessFinished bit, so use it, if we have
> to, until it doesn't work anymore.
> So, through a shadow copy of most registers (some cannot be shadow or it
> makes no sence) I can provide the values without having to actually read
> from the controller/codec. The regfile also contains info which register
> might be shadowed, if values get saved at all (if written) ...
> Furthermore ALSA's AC97 layer does heavy initialization access series on
> the codec, which I tried to "mask out" completely
> (LM4550_REG_FAKEPROBE).
>
> > > +#define LM4550_RF_FLAG(reg) lm4550_regfile[reg / 2].flag
> <snip>
> > > +static void lm4550_regfile_init(void)
> > > +{
> > > + int i;
> > > + for (i = 0; i < 128; i = i + 2)
> > > + if (LM4550_RF_FLAG(i) & LM4550_REG_FAKEPROBE)
> > > + LM4550_RF_VAL(i) = LM4550_RF_DEF(i);
> >
> > "MACRO(x) = XXX" looks a bit strange to me.
>
> Hmmm, ok. I thought about that, too. I think, I'll spell them out?
>
> > RATE_CONTINUOUS and RATE_KNOW are usually exclusive.
>
> Ok, so what I want is RATE_CONTINUOUS, right? (because the LM4550
> supports 4000 to 48000kHz in 1Hz steps)
> BTW: What is RATE_KNOT good for?
It indicates unusual non-continuous sample rates that don't match with
SNDRV_PCM_RATE_XXX bits are supported. The rates in 1Hz step is
continuous enough :)
Takashi
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
