PATCH: trident , pci_enable_device moved

PATCH: trident , pci_enable_device moved

[Marcus Meissner]

Hi Alan, linux-kernel,

This moves pci_enable_device() in trident.c before any PCI resource access.
Everything else appears to be ok in regards to 2.4 PCI API and return values.

Ciao, Marcus

Index: trident.c
===================================================================
RCS file: /build/mm/work/repository/linux-mm/drivers/sound/trident.c,v
retrieving revision 1.12
diff -u -r1.12 trident.c
--- trident.c	2001/04/24 09:47:13	1.12
+++ trident.c	2001/04/24 10:19:36
@@ -3309,6 +3309,9 @@
 	struct trident_card *card;
 	u8 revision;
 
+	if (pci_enable_device(pci_dev))
+	    return -ENODEV;
+
 	if (pci_set_dma_mask(pci_dev, TRIDENT_DMA_MASK)) {
 		printk(KERN_ERR "trident: architecture does not support"
 		       " 30bit PCI busmaster DMA\n");
@@ -3322,9 +3325,6 @@
 		       iobase);
 		return -ENODEV;
 	}
-
-	if (pci_enable_device(pci_dev))
-	    return -ENODEV;
 
 	if ((card = kmalloc(sizeof(struct trident_card), GFP_KERNEL)) == NULL) {
 		printk(KERN_ERR "trident: out of memory\n");

Re: PATCH: trident , pci_enable_device moved

[Marcus Meissner]

On Wed, Apr 25, 2001 at 09:04:38AM +0200, Marcus Meissner wrote:
> Hi Alan, linux-kernel,
> 
> This moves pci_enable_device() in trident.c before any PCI resource access.
> Everything else appears to be ok in regards to 2.4 PCI API and return values.
> 
> Ciao, Marcus

Argh, actually the return value of pci_enable_device*() should be returned.

Ciao, Marcus

Index: drivers/sound/trident.c
===================================================================
RCS file: /build/mm/work/repository/linux-mm/drivers/sound/trident.c,v
retrieving revision 1.12
diff -u -r1.12 trident.c
--- drivers/sound/trident.c	2001/04/24 09:47:13	1.12
+++ drivers/sound/trident.c	2001/04/25 07:31:11
@@ -3308,7 +3308,11 @@
 	unsigned long iobase;
 	struct trident_card *card;
 	u8 revision;
+	int ret;
 
+	if ((ret=pci_enable_device(pci_dev)))
+	    return ret;
+
 	if (pci_set_dma_mask(pci_dev, TRIDENT_DMA_MASK)) {
 		printk(KERN_ERR "trident: architecture does not support"
 		       " 30bit PCI busmaster DMA\n");
@@ -3322,9 +3326,6 @@
 		       iobase);
 		return -ENODEV;
 	}
-
-	if (pci_enable_device(pci_dev))
-	    return -ENODEV;
 
 	if ((card = kmalloc(sizeof(struct trident_card), GFP_KERNEL)) == NULL) {
 		printk(KERN_ERR "trident: out of memory\n");

Re: PATCH: trident , pci_enable_device moved

[Andres Salomon]

Just a warning; I was informed by Alan that doing this for video
drivers was unnecessary, since video devices were already enabled
during bootup.


On Wed, Apr 25, 2001 at 01:06:24PM +0200, Marcus Meissner wrote:
> 
> On Wed, Apr 25, 2001 at 09:04:38AM +0200, Marcus Meissner wrote:
> > Hi Alan, linux-kernel,
> > 
> > This moves pci_enable_device() in trident.c before any PCI resource access.
> > Everything else appears to be ok in regards to 2.4 PCI API and return values.
> > 
> > Ciao, Marcus
> 
> Argh, actually the return value of pci_enable_device*() should be returned.
> 
> Ciao, Marcus
> 

-- 
"... being a Linux user is sort of like living in a house inhabited
by a large family of carpenters and architects. Every morning when
you wake up, the house is a little different. Maybe there is a new
turret, or some walls have moved. Or perhaps someone has temporarily
removed the floor under your bed." - Unix for Dummies, 2nd Edition
        -- found in the .sig of Rob Riggs, rriggs@tesser.com

Re: PATCH: trident , pci_enable_device moved

[Jeff Garzik]

Andres Salomon wrote:
> Just a warning; I was informed by Alan that doing this for video
> drivers was unnecessary, since video devices were already enabled
> during bootup.

To clarify:  the primary display device is enabled and initialized, and
its video BIOS executed, when during BIOS startup and before the Linux
kernel gets control.  All other display devices are not only not
initialized, but they are disabled as well.

Marcus is doing sound ATM so I doubt this matters to him...

-- 
Jeff Garzik      | The difference between America and England is that
Building 1024    | the English think 100 miles is a long distance and
MandrakeSoft     | the Americans think 100 years is a long time.
                 |      (random fortune)

Re: PATCH: trident , pci_enable_device moved

[Andres Salomon]

Oops, I saw "trident" and thought video.  Sorry, marcus.  :)

This is what I was told (it was only needed for secondary video
devices).  From that, I would expect that all video devices would
need it, just in case they happened to be the second card.  Am I
missing some subtlety in some of the video driers/chipsets that
wouldn't allow them to be used as a second video device (therefore
not requiring pci_enable_device)?


On Wed, Apr 25, 2001 at 11:04:55AM -0400, Jeff Garzik wrote:
> 
> Andres Salomon wrote:
> > Just a warning; I was informed by Alan that doing this for video
> > drivers was unnecessary, since video devices were already enabled
> > during bootup.
> 
> To clarify:  the primary display device is enabled and initialized, and
> its video BIOS executed, when during BIOS startup and before the Linux
> kernel gets control.  All other display devices are not only not
> initialized, but they are disabled as well.
> 
> Marcus is doing sound ATM so I doubt this matters to him...
> 
> -- 
> Jeff Garzik      | The difference between America and England is that
> Building 1024    | the English think 100 miles is a long distance and
> MandrakeSoft     | the Americans think 100 years is a long time.
>                  |      (random fortune)
> 

-- 
"... being a Linux user is sort of like living in a house inhabited
by a large family of carpenters and architects. Every morning when
you wake up, the house is a little different. Maybe there is a new
turret, or some walls have moved. Or perhaps someone has temporarily
removed the floor under your bed." - Unix for Dummies, 2nd Edition
        -- found in the .sig of Rob Riggs, rriggs@tesser.com

Re: PATCH: trident , pci_enable_device moved

[Jeff Garzik]

Andres Salomon wrote:
> This is what I was told (it was only needed for secondary video
> devices).  From that, I would expect that all video devices would
> need it, just in case they happened to be the second card.  Am I
> missing some subtlety in some of the video driers/chipsets that
> wouldn't allow them to be used as a second video device (therefore
> not requiring pci_enable_device)?

They do need pci_enable_device, both primary and secondary displays. 
For the primary display its safe to call pci_enable_device.  For
secondary displays, you have to first disable I/O decoding for all VGA
devices before you can enable a secondary display.  You don't want more
than one device decoding the legacy VGA region at any one time.

Some cards have the capability to relocate the VGA region, which is
nice.  The bigger problem is initializing secondary displays; every
video card has a proprietary video BIOS initialization sequence that is
run by main BIOS on startup.  You can either duplicate this sequence
with C code, which is sometimes difficult due to lack of docs or variety
of boards, or you can execute the video BIOS with an x86 emulator.

-- 
Jeff Garzik      | The difference between America and England is that
Building 1024    | the English think 100 miles is a long distance and
MandrakeSoft     | the Americans think 100 years is a long time.
                 |      (random fortune)

Re: PATCH: trident , pci_enable_device moved

[Eric W. Biederman]

Jeff Garzik <jgarzik@mandrakesoft.com> writes:

> Andres Salomon wrote:
> > This is what I was told (it was only needed for secondary video
> > devices).  From that, I would expect that all video devices would
> > need it, just in case they happened to be the second card.  Am I
> > missing some subtlety in some of the video driers/chipsets that
> > wouldn't allow them to be used as a second video device (therefore
> > not requiring pci_enable_device)?
> 
> They do need pci_enable_device, both primary and secondary displays. 
> For the primary display its safe to call pci_enable_device.  For
> secondary displays, you have to first disable I/O decoding for all VGA
> devices before you can enable a secondary display.  You don't want more
> than one device decoding the legacy VGA region at any one time.
> 
> Some cards have the capability to relocate the VGA region, which is
> nice.  The bigger problem is initializing secondary displays; every
> video card has a proprietary video BIOS initialization sequence that is
> run by main BIOS on startup.  You can either duplicate this sequence
> with C code, which is sometimes difficult due to lack of docs or variety
> of boards, or you can execute the video BIOS with an x86 emulator.

Note:  With linuxBIOS (and some other embedded linux setups) even a
primary display doesn't get initialized until you start linux so if
you can properly initialize your display please do it.

Eric