Re: EP93xx PIO IDE driver proposal

[Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>]

On Monday 18 May 2009 15:49:15 João Ramos wrote:
> Hi,
> 
> Here is the revised patch according to Bart's suggestions in previous 
> emails.
> The patch is against linux-2.6.30-rc6.

Thanks.  I see that we can still cleanup the code a bit by folding current
ep93xx_ide_{read,write}() into __ep93xx_ide_{read,write}() and then adding
wrappers on top of it.

I think that the patch would the best to show the idea in this case (it is
completely untested, it may not build).

[ While at it I also changed data transfer functions to use drive's timings
  and fixed ->remove method to unregister IDE host before doing host specific
  cleanup.  These fixes should be applied regardless of the cleanup change. ]

Please review/integrate/test and then resubmit the final patch.

---
 drivers/ide/ep93xx-ide.c |  111 ++++++++++++++++++++++-------------------------
 1 file changed, 54 insertions(+), 57 deletions(-)

Index: b/drivers/ide/ep93xx-ide.c
===================================================================
--- a/drivers/ide/ep93xx-ide.c
+++ b/drivers/ide/ep93xx-ide.c
@@ -126,50 +126,67 @@ static void ep93xx_ide_clean_regs(void _
 	writel(0, base + IDEUDMADEBUG);
 }
 
-static u16 ep93xx_ide_read(void __iomem *base, unsigned long addr,
-		struct ide_timing *timing)
+static u16 __ep93xx_ide_read(void __iomem *base, unsigned long addr,
+			     struct ide_timing *t)
 {
 	u32 reg;
 
 	reg = addr | IDECTRL_DIORN | IDECTRL_DIOWN;
 	writel(reg, base + IDECTRL);
-	ndelay(timing->setup);
+	ndelay(t->setup);
 
 	reg &= ~IDECTRL_DIORN;
 	writel(reg, base + IDECTRL);
-	ndelay(timing->active);
+	ndelay(t->active);
 
 	while (!ep93xx_ide_check_iordy(base))
 		cpu_relax();
 
 	reg |= IDECTRL_DIORN;
 	writel(reg, base + IDECTRL);
-	ndelay(timing->recover);
+	ndelay(t->recover);
 
 	return readl(base + IDEDATAIN);
 }
 
-static void ep93xx_ide_write(void __iomem *base, u16 value,
-		unsigned long addr, struct ide_timing *timing)
+static inline u16 ep93xx_ide_read(ide_hwif_t *hwif, unsigned long addr)
+{
+	void __iomem *base = hwif->host->host_priv;
+	struct ide_timing *t = (struct ide_timing *)ide_get_hwifdata(hwif);
+
+	return __ep93xx_ide_read(base, addr, t);
+}
+
+static void __ep93xx_ide_write(void __iomem *base, u16 value,
+			       unsigned long addr, struct ide_timing *t)
 {
 	u32 reg;
 
 	reg = addr | IDECTRL_DIORN | IDECTRL_DIOWN;
 	writel(reg, base + IDECTRL);
-	ndelay(timing->setup);
+	ndelay(t->setup);
 
 	writel(value, base + IDEDATAOUT);
 
 	reg &= ~IDECTRL_DIOWN;
 	writel(reg, base + IDECTRL);
-	ndelay(timing->active);
+	ndelay(t->active);
 
 	while (!ep93xx_ide_check_iordy(base))
 		cpu_relax();
 
 	reg |= IDECTRL_DIOWN;
 	writel(reg, base + IDECTRL);
-	ndelay(timing->recover);
+	ndelay(t->recover);
+}
+
+static inline void ep93xx_ide_write(ide_hwif_t *hwif, u16 value,
+				    unsigned long addr)
+{
+	void __iomem *base = hwif->host->host_priv;
+	struct ide_timing *t = (struct ide_timing *)ide_get_hwifdata(hwif);
+
+	__ep93xx_ide_write(base, value, addr, t);
 }
 
 /*
@@ -178,88 +195,70 @@ static void ep93xx_ide_write(void __iome
 
 static void ep93xx_ide_exec_command(struct hwif_s *hwif, u8 cmd)
 {
-	void __iomem *base = hwif->host->host_priv;
-	struct ide_timing *t = (struct ide_timing *)ide_get_hwifdata(hwif);
-
-	ep93xx_ide_write(base, cmd, hwif->io_ports.command_addr, t);
+	ep93xx_ide_write(hwif, cmd, hwif->io_ports.command_addr);
 }
 
 static u8 ep93xx_ide_read_status(ide_hwif_t *hwif)
 {
-	void __iomem *base = hwif->host->host_priv;
-	struct ide_timing *t = (struct ide_timing *)ide_get_hwifdata(hwif);
-
-	return ep93xx_ide_read(base, hwif->io_ports.status_addr, t);
+	return ep93xx_ide_read(whif, hwif->io_ports.status_addr);
 }
 
 static u8 ep93xx_ide_read_altstatus(ide_hwif_t *hwif)
 {
-	void __iomem *base = hwif->host->host_priv;
-	struct ide_timing *t = (struct ide_timing *)ide_get_hwifdata(hwif);
-
-	return ep93xx_ide_read(base, hwif->io_ports.ctl_addr, t);
+	return ep93xx_ide_read(hwif, hwif->io_ports.ctl_addr);
 }
 
 static void ep93xx_ide_write_devctl(ide_hwif_t *hwif, u8 ctl)
 {
-	void __iomem *base = hwif->host->host_priv;
-	struct ide_timing *t = (struct ide_timing *)ide_get_hwifdata(hwif);
-
-	ep93xx_ide_write(base, ctl, hwif->io_ports.ctl_addr, t);
+	ep93xx_ide_write(hwif, ctl, hwif->io_ports.ctl_addr);
 }
 
 static void ep93xx_ide_dev_select(ide_drive_t *drive)
 {
 	ide_hwif_t *hwif = drive->hwif;
-	void __iomem *base = hwif->host->host_priv;
 	u8 select = drive->select | ATA_DEVICE_OBS;
-	struct ide_timing *t = (struct ide_timing *)ide_get_hwifdata(hwif);
 
-	ep93xx_ide_write(base, select, hwif->io_ports.device_addr, t);
+	ep93xx_ide_write(hwif, select, hwif->io_ports.device_addr);
 }
 
 static void ep93xx_ide_tf_load(ide_drive_t *drive, struct ide_taskfile *tf,
 		u8 valid)
 {
 	ide_hwif_t *hwif = drive->hwif;
-	void __iomem *base = hwif->host->host_priv;
 	struct ide_io_ports *io_ports = &hwif->io_ports;
-	struct ide_timing *t = (struct ide_timing *)ide_get_hwifdata(hwif);
 
 	if (valid & IDE_VALID_FEATURE)
-		ep93xx_ide_write(base, tf->feature, io_ports->feature_addr, t);
+		ep93xx_ide_write(hwif, tf->feature, io_ports->feature_addr);
 	if (valid & IDE_VALID_NSECT)
-		ep93xx_ide_write(base, tf->nsect, io_ports->nsect_addr, t);
+		ep93xx_ide_write(hwif, tf->nsect, io_ports->nsect_addr);
 	if (valid & IDE_VALID_LBAL)
-		ep93xx_ide_write(base, tf->lbal, io_ports->lbal_addr, t);
+		ep93xx_ide_write(hwif, tf->lbal, io_ports->lbal_addr);
 	if (valid & IDE_VALID_LBAM)
-		ep93xx_ide_write(base, tf->lbam, io_ports->lbam_addr, t);
+		ep93xx_ide_write(hwif, tf->lbam, io_ports->lbam_addr);
 	if (valid & IDE_VALID_LBAH)
-		ep93xx_ide_write(base, tf->lbah, io_ports->lbah_addr, t);
+		ep93xx_ide_write(hwif, tf->lbah, io_ports->lbah_addr);
 	if (valid & IDE_VALID_DEVICE)
-		ep93xx_ide_write(base, tf->device, io_ports->device_addr, t);
+		ep93xx_ide_write(hwif, tf->device, io_ports->device_addr);
 }
 
 static void ep93xx_ide_tf_read(ide_drive_t *drive, struct ide_taskfile *tf,
 		u8 valid)
 {
 	ide_hwif_t *hwif = drive->hwif;
-	void __iomem *base = hwif->host->host_priv;
 	struct ide_io_ports *io_ports = &hwif->io_ports;
-	struct ide_timing *t = (struct ide_timing *)ide_get_hwifdata(hwif);
 
 	if (valid & IDE_VALID_ERROR)
-		tf->error = ep93xx_ide_read(base, io_ports->feature_addr, t);
+		tf->error = ep93xx_ide_read(hwif, io_ports->feature_addr);
 	if (valid & IDE_VALID_NSECT)
-		tf->nsect = ep93xx_ide_read(base, io_ports->nsect_addr, t);
+		tf->nsect = ep93xx_ide_read(hwif, io_ports->nsect_addr);
 	if (valid & IDE_VALID_LBAL)
-		tf->lbal = ep93xx_ide_read(base, io_ports->lbal_addr, t);
+		tf->lbal = ep93xx_ide_read(hwif, io_ports->lbal_addr);
 	if (valid & IDE_VALID_LBAM)
-		tf->lbam = ep93xx_ide_read(base, io_ports->lbam_addr, t);
+		tf->lbam = ep93xx_ide_read(hwif, io_ports->lbam_addr);
 	if (valid & IDE_VALID_LBAH)
-		tf->lbah = ep93xx_ide_read(base, io_ports->lbah_addr, t);
+		tf->lbah = ep93xx_ide_read(hwif, io_ports->lbah_addr);
 	if (valid & IDE_VALID_DEVICE)
-		tf->device = ep93xx_ide_read(base, io_ports->device_addr, t);
+		tf->device = ep93xx_ide_read(hwif, io_ports->device_addr);
 }
 
 static void ep93xx_ide_input_data(ide_drive_t *drive, struct ide_cmd *cmd,
@@ -268,13 +267,12 @@ static void ep93xx_ide_input_data(ide_dr
 	u16 *data = (u16 *)buf;
 	ide_hwif_t *hwif = drive->hwif;
 	void __iomem *base = hwif->host->host_priv;
-	struct ide_io_ports *io_ports = &hwif->io_ports;
+	unsigned long data_addr = hwif->io_ports.data_addr;
+	struct ide_timing *t = (struct ide_timing *)ide_get_drivedata(drive);
 	unsigned int words = (len + 1) >> 1;
-	struct ide_timing *t = (struct ide_timing *)ide_get_hwifdata(hwif);
 
 	while (words--)
-		*data++ = cpu_to_le16(ep93xx_ide_read(base,
-					io_ports->data_addr, t));
+		*data++ = cpu_to_le16(__ep93xx_ide_read(base, data_addr, t);
 }
 
 static void ep93xx_ide_output_data(ide_drive_t *drive, struct ide_cmd *cmd,
@@ -283,13 +281,12 @@ static void ep93xx_ide_output_data(ide_d
 	u16 *data = (u16 *)buf;
 	ide_hwif_t *hwif = drive->hwif;
 	void __iomem *base = hwif->host->host_priv;
-	struct ide_io_ports *io_ports = &hwif->io_ports;
+	unsigned long data_addr = hwif->io_ports.data_addr;
+	struct ide_timing *t = (struct ide_timing *)ide_get_drivedata(drive);
 	unsigned int words = (len + 1) >> 1;
-	struct ide_timing *t = (struct ide_timing *)ide_get_hwifdata(hwif);
 
 	while (words--)
-		ep93xx_ide_write(base, le16_to_cpu(*data++),
-			io_ports->data_addr, t);
+		__ep93xx_ide_write(base, le16_to_cpu(*data++), data_addr, t);
 }
 
 static struct ide_tp_ops ep93xx_ide_tp_ops = {
@@ -477,18 +474,18 @@ fail_release_mem:
 
 static int __devexit ep93xx_ide_remove(struct platform_device *pdev)
 {
-	struct resource *mem_res;
 	struct ide_host *host = pdev->dev.driver_data;
 	void __iomem *base = host->host_priv;
+	struct resource *mem_res;
+
+	ide_host_remove(host);
 
 	/* reset state */
-	ep93xx_ide_clean_regs(host->host_priv);
+	ep93xx_ide_clean_regs(base);
 
 	/* restore back GPIO port E, G and H for GPIO use */
 	ep93xx_ide_release_gpios();
 
-	ide_host_remove(host);
-
 	iounmap(base);
 
 	mem_res = platform_get_resource(pdev, IORESOURCE_MEM, 0);