[alsa-devel] HDA: Enable chipset gcap usage

[Takashi Iwai]

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;

So, the above would be a bit more readable like below:

	chip->capture_streams = (gcap >> 8) & 0x0f;
	chip->playback_streams = (gcap >> 12) & 0x0f;
	chip->capture_index_offset = 0;
	chip->playback_index_offset = chip->capture_streams;

>     > +
>     >               break;
>     >       }
>     >       chip->num_streams = chip->playback_streams + chip->capture_streams;
>     
>     The patch has some coding style issues, for example, no space around
>     operators, etc.  Try to run $LINUX/scripts/checkpatch.pl, fix the
>     errors and the issue above, and please repost the patch again.
>     
> This must be a new script in the kernel.  My development system (2.6.17)
> doesn't have it, but it is on my laptop (2.6.22).  I ran it, ant it reported
> one error " if(!gcap) " which I fixed.  I also added some spacing around the
> bit manipulation code for streams and offsets. 

You should try checkpatch.pl included in the very latest linux kernel.
The script itself has been improved (and became less annoying).


thanks,

Takashi