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
