[alsa-devel] HDA: Enable chipset gcap usage

[Takashi Iwai]

At Mon, 07 Jan 2008 23:23:02 -0800,
Tobin Davis wrote:
> 
> On Tue, 2008-01-08 at 08:11 +0100, Takashi Iwai wrote:
> 
>     At Mon, 07 Jan 2008 10:00:20 -0800,
>     Tobin Davis wrote:
>     > 
>     > On Mon, 2008-01-07 at 11:59 +0100, Takashi Iwai wrote:
>     > 
>     >     At Thu, 03 Jan 2008 15:34:05 -0800,
>     >     Tobin Davis wrote:
>     >     > 
>     >     > Summary:  hda_intel.c use gcap register to determine number of streams
>     >     > supported by southbridge.
>     >     > 
>     >     > This patch removes hardcoded values for the number of streams supported
>     >     > by the southbridge in most chipsets, and reads these values from the
>     >     > chipset directly.  Most systems are hardwired for 4 streams in each
>     >     > direction, but newer chipsets change that capability.
>     >     > 
>     >     > 
>     >     > Signed off by Tobin Davis <tdavis at dsl-only.net>
>     >     > [2 hda-gcap.patch <text/x-patch; UTF-8 (7bit)>]
>     >     > diff -r 1227a1c12325 pci/hda/hda_intel.c
>     >     > --- a/pci/hda/hda_intel.c     Mon Dec 24 14:40:56 2007 +0100
>     >     > +++ b/pci/hda/hda_intel.c     Thu Jan 03 15:27:10 2008 -0800
>     >     > @@ -1709,12 +1709,13 @@ static int __devinit azx_create(struct s
>     >     >  {
>     >     >       struct azx *chip;
>     >     >       int err;
>     >     > +     unsigned short gcap;
>     >     >       static struct snd_device_ops ops = {
>     >     >               .dev_free = azx_dev_free,
>     >     >       };
>     >     >  
>     >     >       *rchip = NULL;
>     >     > -     
>     >     > +
>     >     >       err = pci_enable_device(pci);
>     >     >       if (err < 0)
>     >     >               return err;
>     >     > @@ -1790,10 +1791,19 @@ static int __devinit azx_create(struct s
>     >     >               chip->capture_index_offset = ATIHDMI_CAPTURE_INDEX;
>     >     >               break;
>     >     >       default:
>     >     > -             chip->playback_streams = ICH6_NUM_PLAYBACK;
>     >     > -             chip->capture_streams = ICH6_NUM_CAPTURE;
>     >     > -             chip->playback_index_offset = ICH6_PLAYBACK_INDEX;
>     >     > -             chip->capture_index_offset = ICH6_CAPTURE_INDEX;
>     >     > +             /* read number of streams from GCAP register instead of using
>     >     > +              * hardcoded value
>     >     > +              */
>     >     > +             gcap = azx_readw(chip, GCAP);
>     >     > +             if(!gcap) {
>     >     > +                     snd_printk(KERN_ERR "Device has no streams \n");
>     >     > +                     goto errout;
>     >     > +             };
>     >     
>     >     I think it's safer to assign old ICH6_* values in this case (just to
>     >     be sure) after a warning message.
>     > 
>     > Actually, this should never have been hardcoded in the first place.  The HDA
>     > PRM (April 2005) states that the value returned is hardwired to 4 streams in
>     > the ICH6/7 southbridge of the Intel chipset.  By hardcoding the values in the
>     > driver, we don't allow changes to future (hint - real soon) chipsets.  This
>     > just cleans up this bit of code that has been around since this code was
>     > created.  If I had the other systems in the case statement to test, I'm sure
>     > we could do away with the case statement entirely, just by reading the values
>     > instead of second guessing them.  If the chipsets follow the spec, this method
>     > should work all around.
>     
>     Well, I wasn't clear about the comment above.  I meant the constant
>     values can be used as fallbacks in case these values can't be read,
>     instead of returning error and quit.  This would avoid regressions, at
>     least.
> 
>     >     > +             chip->playback_streams = (gcap&(0xF<<12))>>12;
>     >     > +             chip->capture_streams = (gcap&(0xF<<8))>>8;
>     >     > +             chip->playback_index_offset = (gcap&(0xF<<12))>>12;
>     >     > +             chip->capture_index_offset = 0;
>     >     
>     > I'm not entirely convinced of this method for the index_offsets, but it works
>     > on tested systems here.  I'll review the PRM for more guidance.
>     
>     It should be fine as is.  A stream can be assigned freely  regardless
>     the I/O direction on ICH (and others too).  So, basically it's always:
>     
>             capture_index_offset = 0;
>             playback_index_offset = capture_streams;
> 
> This isn't entirely true, at least based on the ATIHDMI code in the previous
> case statement.

ATIHDMI has no capture.  Thus the above is OK.

>  It would be interesting to see what GCAP returns for the two
> cases (ATIHDMI, and ULI).  If they could be used by this, we could do away
> with the switch entirely (except maybe as a fallback).

Yes, but it's not so important at this stage unless they release new
(incompatible) hardwares.


Takashi