Re: [PATCH 3/4] ide: add struct ide_dma_ops

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

 



Hello again. :-)

Bartlomiej Zolnierkiewicz wrote:

Add struct ide_dma_ops and convert core code + drivers to use it.

While at it:

* Drop "ide_" prefix from ->ide_dma_end and ->ide_dma_test_irq methods.

I have expect you to drop the prefix from the method implementations which didn't happen...

* siimage.c:
  - add siimage_ide_dma_test_irq() helper

   ... at least you shouldn't have added the new ones. :-)

  - print SATA warning in siimage_init_one()

* Remove no longer needed ->init_hwif implementations.

Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@xxxxxxxxx>

I'd expect some init code weight loss at the expense of one more indirection when calling the methods... :-)
   Unfortunately, the patch is not entirely correct...

Index: b/drivers/ide/ide-cd.c
===================================================================
--- a/drivers/ide/ide-cd.c
+++ b/drivers/ide/ide-cd.c
[...]
@@ -929,7 +929,7 @@ static ide_startstop_t cdrom_newpc_intr(
 	dma = info->dma;
 	if (dma) {
 		info->dma = 0;
-		dma_error = HWIF(drive)->ide_dma_end(drive);
+		dma_error = hwif->dma_ops->dma_end(drive);

   Erm, I don't see 'hwif' declared in that function -- this won't compile.
Though wait, some patch adds it... OK.

Index: b/drivers/ide/ide-floppy.c
===================================================================
--- a/drivers/ide/ide-floppy.c
+++ b/drivers/ide/ide-floppy.c
@@ -411,7 +411,7 @@ static ide_startstop_t idefloppy_pc_intr
 	debug_log("Reached %s interrupt handler\n", __func__);
if (pc->flags & PC_FLAG_DMA_IN_PROGRESS) {
-		dma_error = hwif->ide_dma_end(drive);
+		dma_error = drive->hwif->dma_ops->dma_end(drive);

   Here we, contrarily, already have 'hwif', so no need for extra indirection...

Index: b/drivers/ide/mips/au1xxx-ide.c
===================================================================
--- a/drivers/ide/mips/au1xxx-ide.c
+++ b/drivers/ide/mips/au1xxx-ide.c
@@ -371,21 +371,31 @@ static void auide_init_dbdma_dev(dbdev_t
 	dev->dev_devwidth    = devwidth;
 	dev->dev_flags       = flags;
 }
- -#if defined(CONFIG_BLK_DEV_IDE_AU1XXX_MDMA2_DBDMA) +#ifdef CONFIG_BLK_DEV_IDE_AU1XXX_MDMA2_DBDMA
 static void auide_dma_timeout(ide_drive_t *drive)
 {
 	ide_hwif_t *hwif = HWIF(drive);
printk(KERN_ERR "%s: DMA timeout occurred: ", drive->name); - if (hwif->ide_dma_test_irq(drive))
+	if (auide_dma_test_irq(drive))
 		return;
- hwif->ide_dma_end(drive);
+	auide_dma_end(drive);
 }

   Probably worth noting under "while at it"?

+static struct ide_dma_ops auide_dma_ops = {
+	.dma_host_set		= auide_dma_host_set,
+	.dma_setup		= auide_dma_setup,
+	.dma_exec_cmd		= auide_dma_exec_cmd,
+	.dma_start		= auide_dma_start,
+	.dma_end		= auide_dma_end,
+	.dma_test_irq		= auide_dma_test_irq,
+	.dma_lost_irq		= auide_dma_lost_irq,
+	.dma_timeout		= auide_dma_timeout,
+};
+
 static int auide_ddma_init(ide_hwif_t *hwif, const struct ide_port_info *d)
 {
 	_auide_hwif *auide = (_auide_hwif *)hwif->hwif_data;
@@ -516,6 +526,9 @@ static const struct ide_port_ops au1xxx_
 static const struct ide_port_info au1xxx_port_info = {
 	.init_dma		= auide_ddma_init,
 	.port_ops		= &au1xxx_port_ops,
+#ifdef CONFIG_BLK_DEV_IDE_AU1XXX_MDMA2_DBDMA
+	.dma_ops		= &au1xxx_dma_ops,

You need to decide between au1xxx_ and auide_ prefixes, or the file won't compile... ;-)

Index: b/drivers/ide/pci/cmd64x.c
===================================================================
--- a/drivers/ide/pci/cmd64x.c
+++ b/drivers/ide/pci/cmd64x.c
@@ -385,62 +385,33 @@ static u8 __devinit cmd64x_cable_detect(
[...]

+static struct ide_dma_ops cmd643_dma_ops = {

   Perhaps cmd64x_ prefix would match better here...

+	.dma_end		= cmd64x_ide_dma_end,
+	.dma_test_irq		= cmd64x_ide_dma_test_irq,
+};
+
+static struct ide_dma_ops cmd646_rev1_dma_ops = {
+	.dma_end		= cmd646_1_ide_dma_end,
+};
+
+static struct ide_dma_ops cmd64x_dma_ops = {
+	.dma_end		= cmd648_ide_dma_end,
+	.dma_test_irq		= cmd648_ide_dma_test_irq,
+};

   ... and cmd648_ prefix here.
   And why not drop the remaning ide_ "infixes" from functions while at it?

Index: b/drivers/ide/pci/hpt366.c
===================================================================
--- a/drivers/ide/pci/hpt366.c
+++ b/drivers/ide/pci/hpt366.c

   Alas, this part looks incorrect...

@@ -1312,19 +1312,6 @@ static void __devinit init_hwif_hpt366(i
if (new_mcr != old_mcr)
 		pci_write_config_byte(dev, hwif->select_data + 1, new_mcr);
-
-	if (hwif->dma_base == 0)
-		return;
-
-	if (chip_type >= HPT374) {
-		hwif->ide_dma_test_irq	= &hpt374_ide_dma_test_irq;
-		hwif->ide_dma_end	= &hpt374_ide_dma_end;
-	} else if (chip_type >= HPT370) {
-		hwif->dma_start 	= &hpt370_ide_dma_start;
-		hwif->ide_dma_end	= &hpt370_ide_dma_end;
-		hwif->dma_timeout	= &hpt370_dma_timeout;
-	} else
-		hwif->dma_lost_irq	= &hpt366_dma_lost_irq;
 }
static int __devinit init_dma_hpt366(ide_hwif_t *hwif,
[...]
@@ -1428,6 +1415,21 @@ static const struct ide_port_ops hpt3xx_
 	.cable_detect		= hpt3xx_cable_detect,
 };
+static struct ide_dma_ops hpt3xxx_dma_ops = {

   HPT3xxx is a RAID chip I heard. :-) Keep the prefix hpt37x_ please

+	.dma_end		= hpt374_ide_dma_end,
+	.dma_test_irq		= hpt374_ide_dma_test_irq,
+};
+
+static struct ide_dma_ops hpt370x_dma_ops = {

   ... and this one just hpt370_.

+	.dma_start		= hpt370_ide_dma_start,
+	.dma_end		= hpt370_ide_dma_end,
+	.dma_timeout		= hpt370_dma_timeout,
+};
+
+static struct ide_dma_ops hpt36x_dma_ops = {
+	.dma_lost_irq		= hpt366_dma_lost_irq,
+};
+
 static const struct ide_port_info hpt366_chipsets[] __devinitdata = {
 	{	/* 0 */
 		.name		= "HPT36x",
[...]
@@ -1452,6 +1455,7 @@ static const struct ide_port_info hpt366
 		.init_dma	= init_dma_hpt366,
 		.enablebits	= {{0x50,0x04,0x04}, {0x54,0x04,0x04}},
 		.port_ops	= &hpt3xx_port_ops,
+		.dma_ops	= &hpt3xxx_dma_ops,

   For HPT372A/N it should be hpt37x_. Correct.

 		.host_flags	= IDE_HFLAGS_HPT3XX,
 		.pio_mask	= ATA_PIO4,
 		.mwdma_mask	= ATA_MWDMA2,
@@ -1472,6 +1476,7 @@ static const struct ide_port_info hpt366
 		.init_dma	= init_dma_hpt366,
 		.enablebits	= {{0x50,0x04,0x04}, {0x54,0x04,0x04}},
 		.port_ops	= &hpt3xx_port_ops,
+		.dma_ops	= &hpt370x_dma_ops,

   For HPT302/N it should be hpt37x_. Incorrect.

 		.host_flags	= IDE_HFLAGS_HPT3XX,
 		.pio_mask	= ATA_PIO4,
 		.mwdma_mask	= ATA_MWDMA2,
@@ -1483,6 +1488,7 @@ static const struct ide_port_info hpt366
 		.enablebits	= {{0x50,0x04,0x04}, {0x54,0x04,0x04}},
 		.udma_mask	= ATA_UDMA5,
 		.port_ops	= &hpt3xx_port_ops,
+		.dma_ops	= &hpt370x_dma_ops,

   For HPT371/N it should be hpt37x_. Incorrect.

 		.host_flags	= IDE_HFLAGS_HPT3XX,
 		.pio_mask	= ATA_PIO4,
 		.mwdma_mask	= ATA_MWDMA2,
@@ -1493,6 +1499,7 @@ static const struct ide_port_info hpt366
 		.init_dma	= init_dma_hpt366,
 		.enablebits	= {{0x50,0x04,0x04}, {0x54,0x04,0x04}},
 		.port_ops	= &hpt3xx_port_ops,
+		.dma_ops	= &hpt3xxx_dma_ops,

   For HPT34 it should be hpt37x_. Correct.

 		.host_flags	= IDE_HFLAGS_HPT3XX,
 		.pio_mask	= ATA_PIO4,
 		.mwdma_mask	= ATA_MWDMA2,

Now where is the code which selects the correct dma_ops for the HPT36x/370/372/372N chip with device ID 4 I'm asking you? :-)

Index: b/drivers/ide/pci/it821x.c
===================================================================
--- a/drivers/ide/pci/it821x.c
+++ b/drivers/ide/pci/it821x.c
@@ -511,6 +511,11 @@ static void __devinit it821x_quirkproc(i
} +static struct ide_dma_ops it821x_smart_dma_ops = {

   I've got an impression that the mode is contrarywise -- non-SMART...

+	.dma_start		= it821x_dma_start,
+	.dma_end		= it821x_dma_end,
+};
+
 /**
  *	init_hwif_it821x	-	set up hwif structs
  *	@hwif: interface to set up
@@ -562,8 +567,7 @@ static void __devinit init_hwif_it821x(i
if (idev->smart == 0) {
 		/* MWDMA/PIO clock switching for pass through mode */
-		hwif->dma_start = &it821x_dma_start;
-		hwif->ide_dma_end = &it821x_dma_end;
+		hwif->dma_ops = &it821x_smart_dma_ops;
 	} else
 		hwif->host_flags |= IDE_HFLAG_NO_SET_MODE;

   See for yourself...

Index: b/drivers/ide/pci/ns87415.c
===================================================================
--- a/drivers/ide/pci/ns87415.c
+++ b/drivers/ide/pci/ns87415.c
@@ -252,14 +252,17 @@ static void __devinit init_hwif_ns87415 return; outb(0x60, hwif->dma_status);
-	hwif->dma_setup = &ns87415_ide_dma_setup;
-	hwif->ide_dma_end = &ns87415_ide_dma_end;
 }
static const struct ide_port_ops ns87415_port_ops = {
 	.selectproc		= ns87415_selectproc,
 };
+static struct ide_dma_ops ns87415_dma_ops = {
+	.dma_setup		= ns87415_ide_dma_setup,
+	.dma_end		= ns87415_ide_dma_end,
+};
+

   Time to drop ide_ "infixes" here too?

Index: b/drivers/ide/pci/pdc202xx_old.c
===================================================================
--- a/drivers/ide/pci/pdc202xx_old.c
+++ b/drivers/ide/pci/pdc202xx_old.c
@@ -263,23 +263,6 @@ static void pdc202xx_dma_timeout(ide_dri
 	ide_dma_timeout(drive);
 }
-static void __devinit init_hwif_pdc202xx(ide_hwif_t *hwif)
-{
-	struct pci_dev *dev = to_pci_dev(hwif->dev);
-
-	if (hwif->dma_base == 0)
-		return;
-
-	hwif->dma_lost_irq = &pdc202xx_dma_lost_irq;
-	hwif->dma_timeout = &pdc202xx_dma_timeout;
-
-	if (dev->device != PCI_DEVICE_ID_PROMISE_20246) {
-		hwif->dma_start = &pdc202xx_old_ide_dma_start;
-		hwif->ide_dma_end = &pdc202xx_old_ide_dma_end;
- } - hwif->ide_dma_test_irq = &pdc202xx_old_ide_dma_test_irq;
-}
-
 static unsigned int __devinit init_chipset_pdc202xx(struct pci_dev *dev,
 						    const char *name)
 {
@@ -346,12 +329,26 @@ static const struct ide_port_ops pdc2026
 	.cable_detect		= pdc2026x_cable_detect,
 };
+static struct ide_dma_ops pdc20246_dma_ops = {
+	.dma_test_irq		= pdc202xx_old_ide_dma_test_irq,

   Infixes galore! I'd killed both old_ and ide_ (but not Old IDE :-).
Much shorter and uniform name.

+	.dma_lost_irq		= pdc202xx_dma_lost_irq,
+	.dma_timeout		= pdc202xx_dma_timeout,
+};
+
+static struct ide_dma_ops pdc2026x_dma_ops = {
+	.dma_start		= pdc202xx_old_ide_dma_start,
+	.dma_end		= pdc202xx_old_ide_dma_end,
+	.dma_test_irq		= pdc202xx_old_ide_dma_test_irq,

   Drop 'em both! :-)

Index: b/drivers/ide/pci/sc1200.c
===================================================================
--- a/drivers/ide/pci/sc1200.c
+++ b/drivers/ide/pci/sc1200.c
@@ -214,7 +214,7 @@ static void sc1200_set_pio_mode(ide_driv
 		printk("SC1200: %s: changing (U)DMA mode\n", drive->name);
 		ide_dma_off_quietly(drive);
 		if (ide_set_dma_mode(drive, mode) == 0 && drive->using_dma)
-			hwif->dma_host_set(drive, 1);
+			hwif->dma_ops->dma_host_set(drive, 1);
 		return;
 	}
@@ -286,28 +286,20 @@ static int sc1200_resume (struct pci_dev
 }
 #endif
-/*
- * This gets invoked by the IDE driver once for each channel,
- * and performs channel-specific pre-initialization before drive probing.
- */
-static void __devinit init_hwif_sc1200 (ide_hwif_t *hwif)
-{
-	if (hwif->dma_base == 0)
-		return;
-
-	hwif->ide_dma_end   = &sc1200_ide_dma_end;
-}
-
 static const struct ide_port_ops sc1200_port_ops = {
 	.set_pio_mode		= sc1200_set_pio_mode,
 	.set_dma_mode		= sc1200_set_dma_mode,
 	.udma_filter		= sc1200_udma_filter,
 };
+static struct ide_dma_ops sc1200_dma_ops = {
+	.dma_end		= sc1200_ide_dma_end,

   Again, dropping "infix" is possible...

Index: b/drivers/ide/pci/scc_pata.c
===================================================================
--- a/drivers/ide/pci/scc_pata.c
+++ b/drivers/ide/pci/scc_pata.c
@@ -659,10 +659,6 @@ static void __devinit init_hwif_scc(ide_
 	/* PTERADD */
 	out_be32((void __iomem *)(hwif->dma_base + 0x018), hwif->dmatable_dma);
- hwif->dma_setup = scc_dma_setup;
-	hwif->ide_dma_end = scc_ide_dma_end;
-	hwif->ide_dma_test_irq = scc_dma_test_irq;
-
 	if (in_be32((void __iomem *)(hwif->config_data + 0xff0)) & CCKCTRL_ATACLKOEN)
 		hwif->ultra_mask = ATA_UDMA6; /* 133MHz */
 	else
@@ -676,12 +672,19 @@ static const struct ide_port_ops scc_por
 	.cable_detect		= scc_cable_detect,
 };
+static struct ide_dma_ops scc_dma_ops = {
+	.dma_setup		= scc_dma_setup,
+	.dma_end		= scc_ide_dma_end,

   And here too.

Index: b/drivers/ide/pci/sgiioc4.c
===================================================================
--- a/drivers/ide/pci/sgiioc4.c
+++ b/drivers/ide/pci/sgiioc4.c
[...]
@@ -575,10 +560,21 @@ static const struct ide_port_ops sgiioc4
 	.maskproc		= sgiioc4_maskproc,
 };
+static struct ide_dma_ops sgiioc4_dma_ops = {
+	.dma_host_set		= sgiioc4_dma_host_set,
+	.dma_setup		= sgiioc4_ide_dma_setup,
+	.dma_start		= sgiioc4_ide_dma_start,
+	.dma_end		= sgiioc4_ide_dma_end,
+	.dma_test_irq		= sgiioc4_ide_dma_test_irq,
+	.dma_lost_irq		= sgiioc4_dma_lost_irq,
+	.dma_timeout		= ide_dma_timeout,
+};
+

   Wow, a lot of survived infixes! :-)

Index: b/drivers/ide/pci/siimage.c
===================================================================
--- a/drivers/ide/pci/siimage.c
+++ b/drivers/ide/pci/siimage.c
@@ -369,6 +369,14 @@ static int siimage_mmio_ide_dma_test_irq
 	return 0;
 }
+static int siimage_ide_dma_test_irq(ide_drive_t *drive)
+{
+	if (drive->hwif->mmio)
+		return siimage_mmio_ide_dma_test_irq(drive);
+	else
+		return siimage_io_ide_dma_test_irq(drive);
+}
+

I suggest removing infixes from the above functions instead of adding another one...

Index: b/include/linux/ide.h
===================================================================
--- a/include/linux/ide.h
+++ b/include/linux/ide.h
[...]
@@ -459,15 +471,7 @@ typedef struct hwif_s {
 	void (*atapi_input_bytes)(ide_drive_t *, void *, u32);
 	void (*atapi_output_bytes)(ide_drive_t *, void *, u32);
- void (*dma_host_set)(ide_drive_t *, int);
-	int (*dma_setup)(ide_drive_t *);
-	void (*dma_exec_cmd)(ide_drive_t *, u8);
-	void (*dma_start)(ide_drive_t *);
-	int (*ide_dma_end)(ide_drive_t *drive);
-	int (*ide_dma_test_irq)(ide_drive_t *drive);
 	void (*ide_dma_clear_irq)(ide_drive_t *drive);

   Now what about this lonely method? It belongs to dma_ops I think...

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

[Index of Archives]     [Linux Filesystems]     [Linux SCSI]     [Linux RAID]     [Git]     [Kernel Newbies]     [Linux Newbie]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Samba]     [Device Mapper]

  Powered by Linux