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 <><
-
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
