Hi, Jeff and Bart:

  Thanks for your review and sorry for the late reply.
The updated patch is attached for your review/comment.
(The patch is against libata-dev-2.6)

1.

why not jus pata_pdc_new? or pata_pdc_27x?

 changed to "pata_pdc2027x".

2.

 This table is redundant:
* official names (more detailed) can be get from lspci
* PDC_UDMA_[100,133] implies PDC_[100,133]_MHZ
* max UDMA rate is available from ata_port_info etc.

Fixed.

3.

quoted

+ /* For UDMA33, no 80c support is required */
+ if ((ap->udma_mask & ~ATA_UDMA_MASK_40C) == 0)
+  goto cbl40;

this can't happen

Removed.


4.

pds_set_pio() should be called only once
pdc_set_ultra() should be called only once

Fixed.

5.

quoted

+ /* Enable the ROM */
+ if (pdev->resource[PCI_ROM_RESOURCE].start) {
+  pci_write_config_dword(pdev, PCI_ROM_ADDRESS,
+   pdev->resource[PCI_ROM_RESOURCE].start | PCI_ROM_ADDRESS_ENABLE);
+  printk(KERN_INFO DRV_NAME ": ROM enabled at 0x%08lx\n",
+         pdev->resource[PCI_ROM_RESOURCE].start);
+ }

is this really needed?

Removed.

6.

quoted

+  * FIXME: If this driver happens to only be useful on Apple's K2, then
+  * we should check that here as it has a normal Serverworks ID
+  */

cut'n'paste mistake

 Fixed.

7.

pdc_hardware_init() seems to be the only reason
why generic ata_pci_init_one() can't be used here,
what about adding sth. like ->init_device() to
struct ata_port_info?

 Could this be fixed later?
(I am afraid to touch the core. Need more time to study.)

Thanks,

Albert Lee

----- Original Message ----- 
From: "Jeff Garzik" <redacted>
To: "Bartlomiej Zolnierkiewicz" <bzolnier@gmail.com>; "Albert Lee"
[off-list ref]
Cc: "IDE Linux" <redacted>
Sent: Thursday, October 14, 2004 3:58 AM
Subject: Re: [PATCH/RFC] Port of pdc202xx_new driver to libata

I'll use this message to respond to both Albert and you (Bart).

Bartlomiej Zolnierkiewicz wrote:

quoted

On Wed, 13 Oct 2004 18:06:19 +0800, Albert Lee [off-list ref]

wrote:

quoted

quoted

Hi, Jeff and Bart:

Hi!

quoted

 I am trying to port the Promise pdc202xx_new driver to libata and

adding

quoted

quoted

the
PLL tuning code to it. (The motivation is hotplug of the Promise host
adapter.)

I have libata version of pdc202xx_new for a long time now
but this one looks superior as it handles PLL. :-)

quoted

The ported driver has been tested with pdc20269 and pdc20275 on i386 and
x86_64 boxes, with 33 MHz PCI bus and 66 MHz PCI bus respectively.
It seems to be working fine with harddisk drives.

I got a report that sata_promise has problems in some >=66Mhz
configurations, but works fine with 33Mhz configuration.  I bet I could
learn a few things from this driver too, as the concepts probably apply
to newer hardware.

quoted

quoted

(However, CD-ROM drives not working yet, even when
ATA_ENABLE_PATA and ATA_ENABLE_ATAPI are enabled.)

Somehow this doesn't surprise me...

hehe :)

quoted

quoted

My question is, in the current kernel, the IDE subsystem drives PATA

chips

quoted

quoted

and libata drives SATA chips. Will PATA driver like this be accepted

into

quoted

quoted

libata?

My opinion is: yes but not now, final answer is of course left to Jeff.

With the progress currently being made, I think my preference would be
to start collecting PATA drivers such this in my libata-dev queue.

I would not send these PATA drivers upstream until the PATA to-do items
are complete (see other email), but it would be nice to have a central
location (both BK and patch on kernel.org) for developers and users to
mess with PATA support.

So, send PATA drivers to me and I will merge... but please understand
that I will not send upstream for quite some time.

quoted

quoted

Albert Lee
(The patch is against 2.6.8.1.)

It seems your mail client screwed it.

I will try to comment on the patch anyway...

I agree with Bart's comments on the patch.  It was also mangled here.

Albert, if you would resend a patch with Bart's fixes, I'll merge into
libata-dev.

My own comments:

quoted

quoted

obj-$(CONFIG_SCSI_ATA_PIIX) += libata.o ata_piix.o

quoted

+obj-$(CONFIG_SCSI_PATA_PDC_NEW) += libata.o pata_pdc202xx_new.o

why not jus pata_pdc_new? or pata_pdc_27x?

I would prefer to eliminate the "_new" suffix.  Other than that I don't
really care.  If I had written the driver, I would have named it
pata_pdc202xx I suppose.

quoted

+} pdc_pata_controller_tbl [] = {

quoted

+ { "Ultra100 TX2",        PDC_UDMA_100, PDC_100_MHZ }, /* pdc20268 */
+ { "Ultra133 TX2",        PDC_UDMA_133, PDC_133_MHZ }, /* pdc20269 */
+ { "FastTrak LP/TX2/TX4", PDC_UDMA_100, PDC_100_MHZ }, /* pdc20270 */
+ { "FastTrak TX2000",     PDC_UDMA_133, PDC_133_MHZ }, /* pdc20271 */
+ { "MBUltra133",          PDC_UDMA_133, PDC_133_MHZ }, /* pdc20275 */
+ { "MBFastTrak 133 Lite", PDC_UDMA_133, PDC_133_MHZ }, /* pdc20276 */
+ { "SBFastTrak 133 Lite", PDC_UDMA_133, PDC_133_MHZ }, /* pdc20277 */
+};

In general I prefer to eliminate _all_ "marketing strings" from all
drivers.  Model names appear on the market far faster than any
maintainer could hope to keep up.  (granted, this is old hardware and
not changing, in this case)  So as a general rule, it is preferred to
only use strings when absolutely necessary, eliminating them completely
if at all possible.

quoted

what exacty (besides PLL support) are the advantages
of libata driver?

IDE controller hotplug is a lot easier, and proper locking is already
there :)

Jeff


-
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html