Re: EP93xx PIO IDE driver proposal

["João Ramos" <joao.ramos@inov.pt>]

Ok, I will apply the changes, test them and then resubmit the final patch.
By the way, I will submit the final patch through 'arm-linux' mailing 
list, if that is OK to you, since the full patch has platform-specific 
dependencies.
I mean, as long as we iterate with the IDE part of it untill it is good 
for submission from you, I can submit it in 'arm-linux' with your 
acknowledge, right?

Thank you for your revisions.

Best regards,
João Ramos

Bartlomiej Zolnierkiewicz escreveu:
> 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);
>
>
>   


-- 
************************************************************************

    João Ramos        <joao.ramos@inov.pt>

    INOV INESC Inovação - ESTG Leiria
    Escola Superior de Tecnologia e Gestão de Leiria
    Edíficio C1, Campus 2
    Morro do Lena, Alto do Vieiro
    Leiria
    2411-901 Leiria
    Portugal

    Tel: +351244843424
    Fax: +351244843424

************************************************************************