Re: [PATCH] drivers/sound/nm256_audio.c

Jeff Garzik (jgarzik@mandrakesoft.com)
Thu, 19 Apr 2001 11:56:01 -0400


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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/