lkml.org 
[lkml]   [2006]   [Feb]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [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/
\
 
 \ /
  Last update: 2006-02-28 13:05    [W:0.036 / U:0.032 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site