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

[Jeff Garzik <jgarzik@mandrakesoft.com>]

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