On Tue, Jul 26, 2011 at 12:04:27PM +0200, Arnaud Patard wrote:

Sascha Hauer [off-list ref] writes:

quoted

Hi Arnaud,

Hi,

quoted

On Sun, Jul 24, 2011 at 08:38:07PM +0200, Arnaud Patard wrote:

quoted

Add basic support for pata on iMX. It has been tested only on imx51.
SDMA support will probably be added later so this version supports only
PIO.

v2:
  - enable only when needed IORDY
  - use dev_get_drvdata
v3:
  - add missing clk_put() calls
  - use platform_get_irq()
  - fix resume code to avoid disabling IORDY on resume

Signed-off-by: Arnaud Patard <redacted>
Index: linux-2.6-submit/drivers/ata/Kconfig
===================================================================

--- linux-2.6-submit.orig/drivers/ata/Kconfig	2011-07-22 23:29:06.000000000 +0200
+++ linux-2.6-submit/drivers/ata/Kconfig	2011-07-22 23:30:58.000000000 +0200

@@ -467,6 +467,15 @@ config PATA_ICSIDE
 	  interface card.  This is not required for ICS partition support.
 	  If you are unsure, say N to this.
 
+config PATA_IMX
+	tristate "PATA support for Freescale iMX (Experimental)"
+	depends on ARCH_MX5 && EXPERIMENTAL

Should this really depend on EXPERIMENTAL? This driver looks pretty
straightforward.

I've been wondering about it but given that it has been tested only on
efika platforms, I prefered to keep it. If you think it's better to
remove it, fine. I'll do it.

Only the efika platform registers the device, so there's no risk to
harm other boards.

quoted

IS_ERR(priv->clk)

(also for the checks below)

Does it really make sense to continue when the clock failed?

if you have uboot version like the one I have, the clock will be already
enabled before booting linux so the driver will work. Maybe too fragile ?
I've changed the checks on my local version and about to test it. Let me
know if you think it should really be a failure, in order to avoid
having to send a new version only to make it a failure.

Yes, it should be a failure.

quoted

quoted

+	ap->ioaddr.cmd_addr = devm_ioremap(&pdev->dev,
+					io_res->start + PATA_IMX_DRIVE_DATA,
+					0x20);
+	ap->ioaddr.ctl_addr = devm_ioremap(&pdev->dev,
+					io_res->start + PATA_IMX_DRIVE_CONTROL,
+					0x04);

It looks strange to ioremap the register offsets again. Why not just a

	ap->ioaddr.cmd_addr = priv->host_regs + PATA_IMX_DRIVE_DATA
	ap->ioaddr.ctl_addr = priv->host_regs + PATA_IMX_DRIVE_CONTROL

Hm, probably copied from the pxa driver, but this uses different
resources which makes different ioremaps necessary.

There are holes in the register map in the manual, that's why I used
several ioremap. I prefer staying near to what the manual says and avoid
remapping unknown registers.

Having holes in the register space is a very common case, no need to do
several ioremaps. Besides, ioremap will give you a whole page anyway.

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |