Re: [PATCH 8/13] ATA ACPI: PATA methods

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Pavel Machek wrote:
Hi!


From: Randy Dunlap <[email protected]>

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

[Index of Archives]     [Kernel Newbies]     [Netfilter]     [Bugtraq]     [Photo]     [Stuff]     [Gimp]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Video 4 Linux]     [Linux for the blind]     [Linux Resources]
  Powered by Linux