Messages in this thread | | | Date | Tue, 27 Jul 2004 13:07:07 -0400 | From | Jeff Garzik <> | Subject | Re: [PATCH 2.6.8-rc2] sata_nv.c |
| |
Andrew Chew wrote: > This patch updates the NVIDIA libata-based SATA driver to support the > CK804 SATA controller.
Please fix and resubmit:
1) [style] no need to explicitly init first enum to zero:
+typedef enum +{ + NFORCE2 = 0, + NFORCE3, + CK804 +} nv_host_type_t;
2) in Linux (and POSIX), 'struct foo' is preferred over 'foo_t':
+{ + nv_host_type_t host_type; + unsigned long host_flags; + void (*enable_hotplug)(struct ata_probe_ent *probe_en t); + void (*disable_hotplug)(struct ata_host_set *host_set ); + void (*check_hotplug)(struct ata_host_set *host_set); + +} nv_host_desc_t;
3) [style] single statements should not be enclosed by braces
+ if (host->host_desc->check_hotplug) { + host->host_desc->check_hotplug(host_set); } ... + if (host->host_desc->host_flags & NV_HOST_FLAGS_SCR_MMIO) { + return readl(ap->ioaddr.scr_addr + (sc_reg * 4)); + } else { + return inl(ap->ioaddr.scr_addr + (sc_reg * 4)); + } ... + // Enable hotplug event interrupts. + if (host->host_desc->enable_hotplug) { + host->host_desc->enable_hotplug(probe_ent); + }
etc.
4) [leak] driver appears to be missing a ->host_free hook, to free your nv_host_t structure.
+ host = kmalloc(sizeof(nv_host_t), GFP_KERNEL);
5) [bug] check ioremap return value for failure (==NULL)
+ mmio_base = ioremap(pci_resource_start(pdev, 5), + pci_resource_len(pdev, 5));
6) [leak] probe_ent->mmio_base is not set for the MMIO case, therefore, iounmap() is never called for you by ata_pci_remove_one() [libata-core.c].
7) don't use host->scr_addr as a sly way to obtain your base address. Use host_set->mmio_base directly (casting from unsigned long if it's PIO rather than MMIO).
8) "avoid magic numbers". Avoid decimal or hexidecimal constants in actual code. Prefer to use macros/enums instead.
+ pci_read_config_byte(probe_ent->pdev, 0x50, ®val); + regval |= 0x04; + pci_write_config_byte(probe_ent->pdev, 0x50, regval);
- 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/
| |