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