Re: [patch 1/1] ata: Add iMX pata support

[Sergei Shtylyov <sshtylyov@mvista.com>]

Hello.

On 17-07-2011 1:04, Arnaud Patard (Rtp) wrote:

> 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.

> Signed-off-by: Arnaud Patard<arnaud.patard@rtp-net.org>
[...]
> Index: linux-2.6-submit/drivers/ata/pata_imx.c
> ===================================================================
> --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> +++ linux-2.6-submit/drivers/ata/pata_imx.c	2011-07-16 22:35:41.000000000 +0200
> @@ -0,0 +1,254 @@
[...]
> + * TODO:
> + * - dmaengine support
> + * - check if timing stuff needed

    Sure, it's needed, especially if you want to support DMA.

> +#define PATA_IMX_ATA_CONTROL		0x24
> +#define PATA_IMX_ATA_CTRL_FIFO_RST_B	(1<<7)
> +#define PATA_IMX_ATA_CTRL_ATA_RST_B	(1<<6)
> +#define PATA_IMX_ATA_CTRL_IORDY_EN	(1<<0)

    So, it seems to support advanced PIO modes...

> +static int __devinit pata_imx_probe(struct platform_device *pdev)
> +{
> +	struct ata_host *host;
> +	struct ata_port *ap;
> +	struct pata_imx_priv *priv;
> +	int irq = 0;
> +	struct resource *io_res;
> +	struct resource *irq_res;
> +
> +	io_res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (io_res == NULL)
> +		return -EINVAL;
> +
> +	irq_res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);

    Why not use platform_get_irq()?

> +	if (irq_res == NULL)
> +		return -EINVAL;
> +	irq = irq_res->start;
> +
> +	priv = kzalloc(sizeof(struct pata_imx_priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->clk = clk_get(&pdev->dev, "imx-pata");

    Does iMX support clkdev?

[...]
> +	ap->ops = &pata_imx_port_ops;
> +	/* pio 0-4 */
> +	ap->pio_mask = ATA_PIO4;

    But you don't support PIO1-4!

> +	ap->flags |= ATA_FLAG_SLAVE_POSS;
> +
> +	priv->host_regs = devm_ioremap(&pdev->dev, io_res->start, 0x40);
> +	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);

    Why not ioremap all registers as one block?

> +	ata_port_desc(ap, "cmd 0x%llx ctl 0x%llx",
> +		(unsigned long long)io_res->start + PATA_IMX_DRIVE_DATA,
> +		(unsigned long long)io_res->start + PATA_IMX_DRIVE_CONTROL);
> +
> +	/* deassert resets */
> +	__raw_writel(PATA_IMX_ATA_CTRL_FIFO_RST_B |
> +			PATA_IMX_ATA_CTRL_ATA_RST_B |
> +			PATA_IMX_ATA_CTRL_IORDY_EN,

    You can't enable IORDY by default, only when the drive supports it!

[...]
> +#ifdef CONFIG_PM
> +static int pata_imx_suspend(struct device *dev)
> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct ata_host *host = platform_get_drvdata(pdev);

    You can just use dev_get_drvdata(), without calling to_platfrom_device().

> +	struct pata_imx_priv *priv = host->private_data;
> +	int ret;
> +
> +	ret = ata_host_suspend(host, PMSG_SUSPEND);
> +

    Empty line not needed here...

> +	if (!ret)
> +		__raw_writel(0, priv->host_regs + PATA_IMX_ATA_INT_EN);
[...]
> +static int pata_imx_resume(struct device *dev)
> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct ata_host *host = platform_get_drvdata(pdev);

    Just use dev_get_drvdata().

> +	struct pata_imx_priv *priv = host->private_data;
[...]

WBR, Sergei