Messages in this thread Patch in this message | | | Date | Tue, 28 Feb 2006 07:02:56 -0500 | From | Jeff Garzik <> | Subject | Re: [PATCH 8/13] ATA ACPI: PATA methods |
| |
Pavel Machek wrote: > Hi! > > >>From: Randy Dunlap <randy_d_dunlap@linux.intel.com> >> >>Add PATA support to the previous SATA support. > > > Does it make your CONFIG_SATA_ACPI option missnamed?
Yes.
>>--- linux-2616-rc4-ata.orig/drivers/scsi/libata-acpi.c >>+++ linux-2616-rc4-ata/drivers/scsi/libata-acpi.c >>@@ -35,6 +35,23 @@ struct taskfile_array { >> u8 tfa[REGS_PER_GTF]; /* regs. 0x1f1 - 0x1f7 */ >> }; >> >>+struct GTM_buffer { >>+ __u32 PIO_speed0; >>+ __u32 DMA_speed0; >>+ __u32 PIO_speed1; >>+ __u32 DMA_speed1; >>+ __u32 GTM_flags; >>+}; > > > No reason to use __u32 here, u32 should be okay.
Agreed.
>>+static int pata_get_dev_handle(struct device *dev, acpi_handle *handle, >>+ acpi_integer *pcidevfn) >>+{ >>+ unsigned int domain, bus, devnum, func; >>+ acpi_integer addr; >>+ acpi_handle dev_handle, parent_handle; >>+ int scanned; >>+ struct acpi_buffer buffer = {.length = ACPI_ALLOCATE_BUFFER, >>+ .pointer = NULL}; >>+ acpi_status status; >>+ struct acpi_device_info *dinfo = NULL; >>+ int ret = -ENODEV; >>+ >>+ printk(KERN_DEBUG "%s: ENTER: dev->bus_id='%s'\n", >>+ __FUNCTION__, dev->bus_id); > > > Have you catched some nasty disease from acpi people or what? Having > "printk(enter)" at beggining of each function may be nice for your > development, but should be removed prior to merge.
Mostly agreed: the above should be changed to
if (ata_msg_xxx()) ...
prior to merging.
>>+ if ((scanned = sscanf(dev->bus_id, "%x:%x:%x.%x", >>+ &domain, &bus, &devnum, &func)) != 4) { >>+ printk(KERN_DEBUG "%s: sscanf ret. %d\n", >>+ __FUNCTION__, scanned); >>+ goto err; >>+ } > > > Is there no other way than to parse back name?
Agreed. Should obtain the struct pci_dev pointer, and access the elements, rather than parsing a string.
>>+ if (ata_msg_probe(ap)) >>+ printk(KERN_DEBUG >>+ "%s: ENTER: ap->id: %d, port#: %d, hard_port#: %d\n", >>+ __FUNCTION__, ap->id, >>+ ap->port_no, ap->hard_port_no); >>+ > > > Again, please clean your printks.
NAK, the above is better form.
>>@@ -557,11 +701,11 @@ EXPORT_SYMBOL_GPL(do_drive_set_taskfiles >> */ >> int ata_acpi_exec_tfs(struct ata_port *ap) >> { >>- int ix; >>- int ret; >>- unsigned int gtf_length; >>- unsigned long gtf_address; >>- unsigned long obj_loc; >>+ int ix; >>+ int ret; >>+ unsigned int gtf_length; >>+ unsigned long gtf_address; >>+ unsigned long obj_loc; >> >> if (ata_msg_probe(ap)) >> printk(KERN_DEBUG "%s: ENTER:\n", __FUNCTION__); > > > Again!
Ditto.
>>+void ata_acpi_get_timing(struct ata_port *ap) >>+{ >>+ struct device *dev = ap->dev; >>+ int err; >>+ acpi_handle dev_handle; >>+ acpi_integer pcidevfn; >>+ acpi_handle chan_handle; >>+ acpi_status status; >>+ struct acpi_buffer output; >>+ union acpi_object *out_obj; >>+ struct GTM_buffer *gtm; >>+ >>+ if (noacpi) >>+ goto out; >>+ >>+ if (ata_msg_probe(ap)) >>+ printk(KERN_DEBUG "%s: ENTER:\n", __FUNCTION__); > > > Again!
Ditto.
>>+void ata_acpi_push_timing(struct ata_port *ap) >>+{ >>+ struct device *dev = ap->dev; >>+ int err; >>+ acpi_handle dev_handle; >>+ acpi_integer pcidevfn; >>+ acpi_handle chan_handle; >>+ acpi_status status; >>+ struct acpi_object_list input; >>+ union acpi_object in_params[1]; >>+ >>+ if (noacpi) >>+ goto out; >>+ >>+ if (ata_msg_probe(ap)) >>+ printk(KERN_DEBUG "%s: ENTER:\n", __FUNCTION__); > > > And again!
Ditto. All ata_msg_xxx are OK.
Jeff
- 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/
| |