linux-mips
[Top] [All Lists]

Re: [PATCH] eXcite nand flash driver

To: Thomas Koeller <thomas.koeller@baslerweb.com>
Subject: Re: [PATCH] eXcite nand flash driver
From: Thomas Gleixner <tglx@linutronix.de>
Date: Thu, 08 Feb 2007 16:48:57 +0100
Cc: linux-mtd@lists.infradead.org, linux-mips@linux-mips.org
In-reply-to: <200702080157.25432.thomas.koeller@baslerweb.com>
Original-recipient: rfc822;linux-mips@linux-mips.org
References: <200702080157.25432.thomas.koeller@baslerweb.com>
Sender: linux-mips-bounce@linux-mips.org
On Thu, 2007-02-08 at 01:57 +0100, Thomas Koeller wrote:

> +static inline const struct resource *excite_nand_get_resource
> +    (struct platform_device *d, unsigned long flags, const char *basename) {

Move curly bracket to new line.

> +     const char fmt[] = "%s_%u";

Please move the format into the snprintf

> +     char buf[80];
> +
> +     if (unlikely
> +         (snprintf(buf, sizeof buf, fmt, basename, d->id) >= sizeof buf))
> +             return NULL;

Useless unlikely

> +     return platform_get_resource_byname(d, flags, buf);
> +}
> +
> +static inline io_reg_t
> +excite_nand_map_regs(struct platform_device *d, const char *basename)
> +{
> +     void *result = NULL;
> +     const struct resource *const r =
> +         excite_nand_get_resource(d, IORESOURCE_MEM, basename);

Blank line between variable declaration and code.

> +     if (likely(r))
> +             result = ioremap_nocache(r->start, r->end + 1 - r->start);

Useless likely

> +     return result;
> +}


> +/* command and control functions */
> +static void excite_nand_control(struct mtd_info *mtd, int cmd,
> +                                    unsigned int ctrl)
> +{
> +     io_reg_t regs =
> +         container_of(mtd, struct excite_nand_drvdata, board_mtd)->regs;
> +     static void __iomem *tgt = NULL;
> +
> +     switch (ctrl) {
> +     case NAND_CTRL_CHANGE | NAND_CTRL_CLE:
> +             tgt = regs + EXCITE_NANDFLASH_CMD_BYTE;
> +             break;
> +     case NAND_CTRL_CHANGE | NAND_CTRL_ALE:
> +             tgt = regs + EXCITE_NANDFLASH_ADDR_BYTE;
> +             break;
> +     case NAND_CTRL_CHANGE | NAND_NCE:
> +             tgt = regs + EXCITE_NANDFLASH_DATA_BYTE;
> +             break;
> +     }

Err, did this ever work ? I doubt it. From nand_base.c:

                chip->cmd_ctrl(mtd, page_addr, ctrl);                           
                                                                             
                ctrl &= ~NAND_CTRL_CHANGE;                                      
                                                                             
                chip->cmd_ctrl(mtd, page_addr >> 8, ctrl);  

So I expect an OOPS happens on a regular base.

> +static int excite_nand_devready(struct mtd_info *mtd)
> +{
> +     struct excite_nand_drvdata * const drvdata =
> +         container_of(mtd, struct excite_nand_drvdata, board_mtd);

Blank line missing

> +     return io_readb(drvdata->regs + EXCITE_NANDFLASH_STATUS_BYTE);
> +}
> +
> +/* excite_nand_remove
> + *
> + * called by device layer to remove the driver
> + * the binding to the mtd and all allocated
> + * resources are released
> + */
> +static int __exit excite_nand_remove(struct device *dev)
> +{
> +     struct excite_nand_drvdata * const this = dev_get_drvdata(dev);
> +
> +     dev_set_drvdata(dev, NULL);
> +
> +     if (unlikely(!this)) {
> +             printk(KERN_ERR "%s: called %s without private data!!",
> +                    module_id, __func__);
> +             return -EINVAL;
> +     }
> +
> +     /* first thing we need to do is release our mtd
> +      * then go through freeing the resource used
> +      */
> +     nand_release(&this->board_mtd);
> +
> +     /* free the common resources */
> +     if (likely(this->regs)) {

Why should you ever come here with no mapping ?

        tglx



<Prev in Thread] Current Thread [Next in Thread>