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@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

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@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