On Fri, 8 Apr 2005, Quentin Garnier wrote:
> On Fri, Apr 08, 2005 at 12:33:38PM -0300, Jared D. McNeill wrote:
>> Currently our AC'97 code assumes that we're only talking to the first
>> codec. The spec allows for multiple codecs, and they don't have to be in
>> any particular order. So far we've been safe with assuming that audio
>> codecs are the first, but I don't want to build the assumption into the
>> low-level hardware drivers that modem codecs are always the second.
>
> That's not correct.  AC'97 itself (from the driver POV) doesn't deal
> with multiple codecs.  Our AC'97 code is currently fine.  auich(4) and
> friends are the one making dubious assumptions.
>
> What can happen is that a controller may be connected to several AC'97
> codecs (which is indeed covered in the spec).  This is a controller-
> dependent situation.  There can be 3 audio codecs, or two with a modem
> codec, or even a single audio codec.

I still think that ac97_read/ac97_write should be responsible for doing 
the register offsetting (ie reg + (codecnum * 0x80)). These functions 
should be exposed to the low-level hw driver and they should be used in 
favour of the hw-specific read/write calls (to take advantage of 
shadowing, etc as well as getting the offsetting for free).

> In the ICH case, which we probably know better, the controller doesn't
> tell which codec(s) should be used by a given controller (either the
> audio one or the modem one, which appear as two distinct PCI functions).
> Therefore we need a way to probe the codecs instead of hard-wiring a
> (albeit very common) configuration.

I've been through the ICH4 spec dozens of times.. this is a completely 
idiotic design.

>> What I'm proposing is adding the ability to pass a hint to ac97_attach,
>> AC97_CODEC_AUDIO or AC97_CODEC_MODEM, that on attach will search for a
>> codec of a specific type to use. We can speed up the search for
>
> No, ac97_attach will not search.  The controller driver will, and the
> flag tells ac97_attach to return an error if the codec is not of the
> specified type.

Seems fair enough. Although I'd like to pass the codec number into 
ac97_attach so the hw driver doesn't have to worry about keeping track of 
the offsetting later. This "second codec is at 0x80, third codec is at 
0x100" stuff is part of the AC'97 spec, is it not?

> I think the safest is to do the probe in that order:
>  o read ext_id.  If non-zero, we have a 2.x audio codec.  Done.
>  o If ext_id is zero, read ext_mid.  If bit 0 is set, we have a 2.x
>    modem codec.  Done.
>  o we have a 1.x codec.  Bit 1 in the reset register tells if it is
>    a modem codec (bit set) or an audio codec.
>
> Of course, I agree that we can safely tag all the already known codecs
> as audio codecs, and in case we stumble upon a, say, modem codec that
> doesn't like ext_id to be read, or return garbage in it, we add it to
> the list as a modem codec.

Agreed.

> Once we have that, auich_attach will do something like the following in
> the modem case (the modem codec is either 0 or 1, per ICH4
> documentation):
>
>  sc->codecnum = 0;
>  if ((error = ac97_attach(..., AC97_CODEC_TYPE_MODEM)) != 0 &&
>      error == ENXIO) {
>          sc->codecnum = 1;
>          error = ac97_attach(..., AC97_CODEC_TYPE_MODEM);
>  }
>  if (error) {
>          /* printf a complaint */
>          return;
>  }

Sure, this sounds reasonable (as long as we can pass the codecnum into 
ac97_attach as well).

> Well, I guess it could start with codecnum = 1 to directly fall into
> the most common situation :)

Good point.

> One last thing, it should be defined what an audio controller driver
> should do with multiple audio codecs.  I must say I don't know how
> such a system is supposed to work.

I would imagine that there is a 1:1 mapping of PCI functions to audio 
codecs in that situation. I don't know what to do with ICH, but as long as 
the controller has a register telling us which codec to use, we should be 
fine:
   codecnum = auimaginary_read(sc, AUIMAGINARY_REG_CODECNUM);
   if (ac97_attach(sc->host_if, codecnum, AC97_CODEC_TYPE_AUDIO) != 0)
     return;

Cheers,
Jared