[PATCH v2] drivers/ata: add support to Freescale 3.0Gbps SATA Controller

[Kalra Ashish-B00888]

Hello Arnd,

Thanks for your comments and feedback.

Actually, for PowerPC platforms iowrite32/ioread32 internally call
writel/readl, which are again mapped to out_le32/in_le32,
therefore we will modify our code to use of_iomap() to combine
functionalities of both of_address_to_resource() & ioremap().

We will also incorporate your other suggestions and then resubmit the
patch.

Ashish

-----Original Message-----
From: Arnd Bergmann [mailto:arnd at arndb.de] 
Sent: Friday, October 12, 2007 7:52 PM
To: linuxppc-dev at ozlabs.org
Cc: Li Yang-r58472; jgarzik at pobox.com; linux-ide at vger.kernel.org; Kalra
Ashish-B00888
Subject: Re: [PATCH v2] drivers/ata: add support to Freescale 3.0Gbps
SATA Controller

On Friday 12 October 2007, Li Yang wrote:
> This patch adds support for Freescale 3.0Gbps SATA Controller 
> supporting Native Command Queueing(NCQ), device hotplug, and ATAPI.  
> This controller can be found on MPC8315 and MPC8378.

Most of the driver looks really good, but here are a few things that
stick out:

> +static int sata_fsl_probe(struct of_device *ofdev,
> +			const struct of_device_id *match)
> +{
> +	int retval = 0;
> +	void __iomem *hcr_base = NULL;
> +	void __iomem *ssr_base = NULL;
> +	void __iomem *csr_base = NULL;
> +	struct sata_fsl_host_priv *host_priv = NULL;
> +	struct resource *r;
> +	int irq;
> +	struct ata_host *host;
> +
> +	struct ata_port_info pi = sata_fsl_port_info[0];
> +	const struct ata_port_info *ppi[] = { &pi, NULL };
> +
> +	dev_printk(KERN_INFO, &ofdev->dev,
> +		   "Sata FSL Platform/CSB Driver init\n");
> +
> +	r = kmalloc(sizeof(struct resource), GFP_KERNEL);
> +	retval = of_address_to_resource(ofdev->node, 0, r);
> +	if (retval)
> +		return -EINVAL;
> +
> +	DPRINTK("start i/o @0x%x\n", r->start);
> +
> +	hcr_base = ioremap(r->start, r->end - r->start + 1);
> +	if (!hcr_base)
> +		goto error_exit_with_cleanup;

Hmm, I think we should redefine of_iomap to do the right thing for you.
currently, it is the combination of of_address_to_resource and ioremap,
which you do as well, so your code can be simplified to do that.
However, ioremap is meant to be used with readl/writel or
in_le32/out_le32 and similar functions, not with ioread32/iowrite32
which you are using.

I had planned to do a patch to get that right for some time so you can
use of_iomap with ioread in all cases, but I guess you should start
using of_iomap even now.

> +
> +error_exit_with_cleanup:
> +
> +	if (hcr_base)
> +		iounmap(hcr_base);
> +	if (host_priv)
> +		kfree(host_priv);
> +
> +	return retval;
> +}

Once of_iomap start using devres, we would no longer need to iounmap
here.

> +static int sata_fsl_remove(struct of_device *ofdev) {
> +	struct ata_host *host = dev_get_drvdata(&ofdev->dev);
> +	struct sata_fsl_host_priv *host_priv = host->private_data;
> +
> +	dev_set_drvdata(&ofdev->dev, NULL);
> +
> +	iounmap(host_priv->hcr_base);
> +	kfree(host_priv);
> +
> +	return 0;
> +}

Should you also free the irq mapping here?

> --- /dev/null
> +++ b/drivers/ata/sata_fsl.h
> @@ -0,0 +1,92 @@
> +/*
> + * drivers/ata/sata_fsl.h
> + *
> + * Freescale 3.0Gbps SATA device driver

The header file is entirely pointless, since all definitions in here are
only used in a single file. Please merge the header into the
implementation.

> +static int sata_fsl_probe(struct of_device *ofdev,
> +			const struct of_device_id *match); static int 
> +sata_fsl_remove(struct of_device *ofdev); static int 
> +sata_fsl_scr_read(struct ata_port *, unsigned int, u32 *); static int

> +sata_fsl_scr_write(struct ata_port *, unsigned int, u32); static 
> +unsigned int sata_fsl_qc_issue(struct ata_queued_cmd *); static 
> +irqreturn_t sata_fsl_interrupt(int, void *); static void 
> +sata_fsl_irq_clear(struct ata_port *); static int 
> +sata_fsl_port_start(struct ata_port *); static void 
> +sata_fsl_port_stop(struct ata_port *); static void 
> +sata_fsl_tf_read(struct ata_port *, struct ata_taskfile *); static 
> +void sata_fsl_qc_prep(struct ata_queued_cmd *); static u8 
> +sata_fsl_check_status(struct ata_port *); static void 
> +sata_fsl_freeze(struct ata_port *); static void sata_fsl_thaw(struct 
> +ata_port *); static void sata_fsl_error_handler(struct ata_port *); 
> +static void sata_fsl_post_internal_cmd(struct ata_queued_cmd *);
> +
> +static inline unsigned int sata_fsl_tag(unsigned int, void __iomem 
> +*);

In particular, get rid of the forward declarations for static functions.
All functions in a simple driver should be ordered in a way that you
always reference only code that was previously defined. This helps avoid
accidental recursions and makes it easier to review.

	Arnd <><