Marcus Meissner wrote:
>
> Hi,
>
> This updates the nm256_audio driver to the 2.4 PCI API.
>
> Patch is against 2.4.3-ac9, verified on Sony VAIO Laptop.
"verified" is the really important part with this driver, since its
really finicky. I have a patch I would love to bounce to you in
private, that I have been searching for a tester for -months- because I
had no test hardware of my own.
Your patch looks ok to me and I would say apply it. But I also think it
is incomplete.
* there is no need for a linked list of cards, since
pci_{get,set}_drvdata is used. This eliminates the list walk in
nm256_remove
* the new PCI API has suspend/resume hooks, and those should be used in
preference to register_pm_...
* there is rarely a need to compare PCI ids manually in a driver. Do
this implicitly by using the struct pci_device_id::driver_data field.
In the following code, you could simply pass these two strings as your
driver_data:
> + if (pcidev->device == PCI_DEVICE_ID_NEOMAGIC_NM256AV_AUDIO)
> + return nm256_install(pcidev, REV_NM256AV, "256AV");
> + if (pcidev->device == PCI_DEVICE_ID_NEOMAGIC_NM256ZX_AUDIO)
> + return nm256_install(pcidev, REV_NM256ZX, "256ZX");
* typically you don't want to -always- printout the module version when
built into the kernel. that can get ugly, when you have a bunch of
drivers in the kernel. you should surround the existing version printk
code with ifdef MODULE, and then add some additional code in your
pci_driver::probe routine which looks something like
#ifndef MODULE
static int printed_version;
if (!printed_version++)
printk(version);
#endif
and further, declare your version string like the following code, so it
can be dropped from the kernel image...
static char version[] __devinitdata =
KERN_INFO "... version ...\n";
Looks good, thanks for the work! If you spot any other drivers you can
convert, feel free.
Jeff
--
Jeff Garzik | "The universe is like a safe to which there is a
Building 1024 | combination -- but the combination is locked up
MandrakeSoft | in the safe." -- Peter DeVries
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
