Just a few cosmetic nits for this patch...
On Thu, Sep 09, 2010 at 06:20:30PM +0800, Roy Zang wrote:
[...]
> @@ -94,22 +73,26 @@ int fsl_upm_find(phys_addr_t addr_base, struct fsl_upm
> *upm)
> {
> int bank;
> __be32 br;
> + struct fsl_lbc_regs __iomem *lbc;
>
> bank = fsl_lbc_find(addr_base);
> if (bank < 0)
> return bank;
>
> - br = in_be32(&fsl_lbc_regs->bank[bank].br);
> + if (!fsl_lbc_ctrl_dev || !fsl_lbc_ctrl_dev->regs)
> + return -ENODEV;
I'd add an empty line here.
> + lbc = fsl_lbc_ctrl_dev->regs;
> + br = in_be32(&lbc->bank[bank].br);
>
> switch (br & BR_MSEL) {
> case BR_MS_UPMA:
> - upm->mxmr = &fsl_lbc_regs->mamr;
> + upm->mxmr = &lbc->mamr;
> break;
> case BR_MS_UPMB:
> - upm->mxmr = &fsl_lbc_regs->mbmr;
> + upm->mxmr = &lbc->mbmr;
> break;
> case BR_MS_UPMC:
> - upm->mxmr = &fsl_lbc_regs->mcmr;
> + upm->mxmr = &lbc->mcmr;
> break;
> default:
> return -EINVAL;
> @@ -143,14 +126,18 @@ EXPORT_SYMBOL(fsl_upm_find);
> * thus UPM pattern actually executed. Note that mar usage depends on the
> * pre-programmed AMX bits in the UPM RAM.
> */
> +
> int fsl_upm_run_pattern(struct fsl_upm *upm, void __iomem *io_base, u32 mar)
> {
> int ret = 0;
> unsigned long flags;
>
> + if (!fsl_lbc_ctrl_dev || !fsl_lbc_ctrl_dev->regs)
> + return -ENODEV;
> +
> spin_lock_irqsave(&fsl_lbc_lock, flags);
>
> - out_be32(&fsl_lbc_regs->mar, mar);
> + out_be32(&fsl_lbc_ctrl_dev->regs->mar, mar);
>
> switch (upm->width) {
> case 8:
> @@ -172,3 +159,188 @@ int fsl_upm_run_pattern(struct fsl_upm *upm, void
> __iomem *io_base, u32 mar)
> return ret;
> }
> EXPORT_SYMBOL(fsl_upm_run_pattern);
> +
> +static int __devinit fsl_lbc_ctrl_init(struct fsl_lbc_ctrl *ctrl)
> +{
> + struct fsl_lbc_regs __iomem *lbc = ctrl->regs;
> +
> + /* clear event registers */
> + setbits32(&lbc->ltesr, LTESR_CLEAR);
> + out_be32(&lbc->lteatr, 0);
> + out_be32(&lbc->ltear, 0);
> + out_be32(&lbc->lteccr, LTECCR_CLEAR);
> + out_be32(&lbc->ltedr, LTEDR_ENABLE);
> +
> + /* Enable interrupts for any detected events */
> + out_be32(&lbc->lteir, LTEIR_ENABLE);
> +
> + return 0;
> +}
> +
> +static int __devexit fsl_lbc_ctrl_remove(struct platform_device *ofdev)
> +{
> + struct fsl_lbc_ctrl *ctrl = dev_get_drvdata(&ofdev->dev);
> +
> + if (ctrl->irq)
> + free_irq(ctrl->irq, ctrl);
> +
> + if (ctrl->regs)
> + iounmap(ctrl->regs);
> +
> + dev_set_drvdata(&ofdev->dev, NULL);
> +
> + kfree(ctrl);
> +
> + return 0;
> +}
> +
> +/*
> + * NOTE: This interrupt is used to report localbus events of various kinds,
> + * such as transaction errors on the chipselects.
> + */
> +
> +static irqreturn_t fsl_lbc_ctrl_irq(int irqno, void *data)
> +{
> + struct fsl_lbc_ctrl *ctrl = data;
> + struct fsl_lbc_regs __iomem *lbc = ctrl->regs;
> + u32 status;
> +
> + status = in_be32(&lbc->ltesr);
> +
> + if (status) {
You could save indentation and improve readability by writing
status = in_be32(&lbc->ltesr);
if (!status)
return IRQ_NONE;
...the rest of the function...
I understand that you just move the code around though.
> + out_be32(&lbc->ltesr, LTESR_CLEAR);
> + out_be32(&lbc->lteatr, 0);
> + out_be32(&lbc->ltear, 0);
> + ctrl->irq_status = status;
> +
> + if (status & LTESR_BM)
> + dev_err(ctrl->dev, "Local bus monitor time-out: "
> + "LTESR 0x%08X\n", status);
> + if (status & LTESR_WP)
> + dev_err(ctrl->dev, "Write protect error: "
> + "LTESR 0x%08X\n", status);
> + if (status & LTESR_ATMW)
> + dev_err(ctrl->dev, "Atomic write error: "
> + "LTESR 0x%08X\n", status);
> + if (status & LTESR_ATMR)
> + dev_err(ctrl->dev, "Atomic read error: "
> + "LTESR 0x%08X\n", status);
> + if (status & LTESR_CS)
> + dev_err(ctrl->dev, "Chip select error: "
> + "LTESR 0x%08X\n", status);
> + if (status & LTESR_UPM)
> + ;
> + if (status & LTESR_FCT) {
> + dev_err(ctrl->dev, "FCM command time-out: "
> + "LTESR 0x%08X\n", status);
> + smp_wmb();
> + wake_up(&ctrl->irq_wait);
> + }
> + if (status & LTESR_PAR) {
> + dev_err(ctrl->dev, "Parity or Uncorrectable ECC error: "
> + "LTESR 0x%08X\n", status);
> + smp_wmb();
> + wake_up(&ctrl->irq_wait);
> + }
> + if (status & LTESR_CC) {
> + smp_wmb();
> + wake_up(&ctrl->irq_wait);
> + }
> + if (status & ~LTESR_MASK)
> + dev_err(ctrl->dev, "Unknown error: "
> + "LTESR 0x%08X\n", status);
> +
> + return IRQ_HANDLED;
> + }
> +
> + return IRQ_NONE;
> +}
> +
> +/*
> + * fsl_lbc_ctrl_probe
> + *
> + * called by device layer when it finds a device matching
> + * one our driver can handled. This code allocates all of
> + * the resources needed for the controller only. The
> + * resources for the NAND banks themselves are allocated
> + * in the chip probe function.
> +*/
> +
> +static int __devinit fsl_lbc_ctrl_probe(struct platform_device *ofdev,
> + const struct of_device_id *match)
> +{
> + int ret;
> +
> + if (!ofdev->dev.of_node) {
> + dev_err(&ofdev->dev, "Device OF-Node is NULL");
> + return -EFAULT;
> + }
> + fsl_lbc_ctrl_dev = kzalloc(sizeof(*fsl_lbc_ctrl_dev), GFP_KERNEL);
> + if (!fsl_lbc_ctrl_dev)
> + return -ENOMEM;
> +
> + dev_set_drvdata(&ofdev->dev, fsl_lbc_ctrl_dev);
> +
> + spin_lock_init(&fsl_lbc_ctrl_dev->lock);
> + init_waitqueue_head(&fsl_lbc_ctrl_dev->irq_wait);
> +
> + fsl_lbc_ctrl_dev->regs = of_iomap(ofdev->dev.of_node, 0);
> + if (!fsl_lbc_ctrl_dev->regs) {
> + dev_err(&ofdev->dev, "failed to get memory region\n");
> + ret = -ENODEV;
> + goto err;
> + }
> +
> + fsl_lbc_ctrl_dev->irq = irq_of_parse_and_map(ofdev->dev.of_node, 0);
> + if (fsl_lbc_ctrl_dev->irq == NO_IRQ) {
> + dev_err(&ofdev->dev, "failed to get irq resource\n");
> + ret = -ENODEV;
> + goto err;
> + }
> +
> + fsl_lbc_ctrl_dev->dev = &ofdev->dev;
> +
> + ret = fsl_lbc_ctrl_init(fsl_lbc_ctrl_dev);
> + if (ret < 0)
> + goto err;
> +
> + ret = request_irq(fsl_lbc_ctrl_dev->irq, fsl_lbc_ctrl_irq, 0,
> + "fsl-lbc", fsl_lbc_ctrl_dev);
> + if (ret != 0) {
> + dev_err(&ofdev->dev, "failed to install irq (%d)\n",
> + fsl_lbc_ctrl_dev->irq);
> + ret = fsl_lbc_ctrl_dev->irq;
> + goto err;
> + }
> +
> + return 0;
> +
> +err:
> + iounmap(fsl_lbc_ctrl_dev->regs);
> + kfree(fsl_lbc_ctrl_dev);
> + return ret;
> +}
> +
> +static const struct of_device_id fsl_lbc_match[] = {
> + { .compatible = "fsl,elbc", },
> + { .compatible = "fsl,pq3-localbus", },
> + { .compatible = "fsl,pq2-localbus", },
> + { .compatible = "fsl,pq2pro-localbus", },
> + {},
> +};
You need linux/mod_devicetable.h for this.
> +
> +static struct of_platform_driver fsl_lbc_ctrl_driver = {
Need linux/of_platform.h for this.
But you actually don't need of_platform_driver, as for the
new code you can use platform_driver (and thus
linux/platform_device.h).
> + .driver = {
> + .name = "fsl-lbc",
> + .of_match_table = fsl_lbc_match,
> + },
> + .probe = fsl_lbc_ctrl_probe,
> +};
> +
> +static int __init fsl_lbc_init(void)
> +{
> + return of_register_platform_driver(&fsl_lbc_ctrl_driver);
> +}
> +
No need for this empty line.
> +module_init(fsl_lbc_init);
Thanks,
--
Anton Vorontsov
email: cbouatmai...@gmail.com
irc://irc.freenode.net/bd2
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
