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

[Jeff Garzik <jgarzik@pobox.com>]

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/