On Mer, 2004-12-29 at 17:29, Bartlomiej Zolnierkiewicz wrote:

Let me complain once again :-), libata based driver would be better...

Eventually probably but libata's PATA support is pretty pathetic right
now, it doesn't know anything about PATA drive errata. I did look at it
and when it grows up into a real IDE layer for PATA I'm all for moving
*every* IDE driver to it because some of the IDE error path corner cases
are almost rewrite level fixes (eg DMA changedown, timer/interrupt CD
race oops)

quoted

+       ide_hwif_t *hwif        = HWIF(drive);

Please do not use HWIF() macro in a new code, it does
needless casting which hides errors and increases code size.

Ok - I agree entirely about that one.

quoted

+static byte it8212_ratemask (ide_drive_t *drive)

please don't use 'byte' type in a new code

Ok

quoted

+       u8 unit = drive->select.b.unit;
+       ide_hwif_t *hwif = HWIF(drive);
+       ide_drive_t *pair = &hwif->drives[1-unit];
+       u8 speed = 0, set_pio   = ide_get_best_pio_mode(drive, 4, 5, NULL);

ide_get_best_pio_mode(drive, 4, 5, NULL) always returns 4
[ quick fix == hardcode 4 for now and add FIXME ]

Will look at that.

quoted

+       u8 speed                = ide_rate_filter(it8212_ratemask(drive), xferspeed);

itdev->smart check should be here

Can move that

quoted

+static int config_chipset_for_dma (ide_drive_t *drive)
+{
+       u8 speed        = ide_dma_speed(drive, it8212_ratemask(drive));
+
+       config_chipset_for_pio(drive, !speed);

set autotune in ->init_hwif instead, only difference will be that PIO command
will be send out - if this is a problem cache set_speed parameter in ide_drive_t

That will be fine.

quoted

+       if (ide_set_xfer_rate(drive, speed))
+               return 0;

just use it8212_tune_chipset()

ok

quoted

+                *      If we are in pass through mode then not much
+                *      needs to be done, but we do bother to clear the
+                *      IRQ mask in case the drives are PIO (eg rev 0x10)
+                *      for now.
+                */

comment or code is wrong, unmask is turned on unconditionally

Disagree. It says "e.g."

quoted

+               if(strstr(id->model, "Integrated Technology Express")) {
+                       /* In raid mode the ident block is slightly buggy */
+                       id->capability |= 3;            /* LBA28, DMA */
+                       id->command_set_2 |= 0x0400;    /* LBA48 valid */
+                       id->cfs_enable_2 |= 0x0400;     /* LBA48 on */

comment why this is need would be helpful

Will add

quoted

+                       hwif->ide_dma_off_quietly(drive);
+#ifdef CONFIG_IDEDMA_ONLYDISK
+                       if (drive->media == ide_disk)
+#endif
+                               hwif->ide_dma_check(drive);

hack, it looks like fixup code in ide-probe.c need to be moved to probe_hwif()

I'm not sure of the best way to do that cleanly. What do you have in
mind ?

quoted

+                       id->csfo = 0;
+                       id->cfa_power = 0;

aargghh, this is a gross hack to support smart mode in IDE driver

The alternative is to stick a command filter into the IDE core in
various places. I'd rather hide the uglies in the driver having played
with ->taskfile hooks in 2.6.9-ac. That keeps the uglies where they
belong but isn't nice to retrofit to the IDE core (unlike libata)

quoted

+       if (it8212_noraid) {
+               printk(KERN_INFO "it8212: forcing bypass mode.\n");
+
+               /* Reset local CPU, and set BIOS not ready */
+               pci_write_config_byte(hwif->pci_dev, 0x5E, 0x01);
+
+               /* Set to bypass mode, and reset PCI bus */
+               pci_write_config_byte(hwif->pci_dev, 0x50, 0x00);
+
+               pci_write_config_word(hwif->pci_dev, PCI_COMMAND,
+                                     PCI_COMMAND_PARITY | PCI_COMMAND_IO |
+                                     PCI_COMMAND_MEMORY | PCI_COMMAND_MASTER);
+
+               pci_write_config_word(hwif->pci_dev, 0x40, 0xA0F3);
+
+               pci_write_config_dword(hwif->pci_dev,0x4C, 0x02040204);
+               pci_write_config_byte(hwif->pci_dev, 0x42, 0x36);
+               pci_write_config_byte(hwif->pci_dev, PCI_LATENCY_TIMER, 0);
+       }

this code is in the wrong hook, it should be in ->init_chipset
(it should be also moved to separate function for better readability)

Agreed

quoted

+       if(hwif->channel == 0)
+               printk(KERN_INFO "it8212: controller in %s mode.\n",
+                       mode[idev->smart]);

should be in ->init_chipset

Ok

quoted

+       pci_read_config_byte(hwif->pci_dev, 0x50, &conf);

conf is already available

Ok

quoted

+       hwif->drives[0].autotune = 1;
+       hwif->drives[1].autotune = 1;

autotune should be always set

Thanks. I'll go and polish these up.