lkml.org 
[lkml]   [2023]   [Jun]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH v1 24/43] mtd: nand: add support for ts72xx
    Thu, Jun 01, 2023 at 08:45:29AM +0300, Nikita Shubin kirjoitti:
    > Technologic Systems has it's own nand controller implementation in CPLD.
    >
    > This patch adds support for TS-72XX boards family.

    Use imperative mode, this is documented in the Submitting Patches,

    ...

    > +/*
    > + * Technologic Systems TS72xx NAND controller driver
    > + *
    > + * Copyright (C) 2023 Nikita Shubin <nikita.shubin@maquefel.me>
    > + *
    > + * derived: plat_nand.c

    Derived from:

    > + * Author: Vitaly Wool <vitalywool@gmail.com>
    > + */

    ...

    > +#include <linux/err.h>
    > +#include <linux/io.h>
    > +#include <linux/module.h>
    > +#include <linux/platform_device.h>
    > +#include <linux/slab.h>

    + Blank line?

    > +#include <linux/mtd/mtd.h>
    > +#include <linux/mtd/platnand.h>

    ...

    > + bits = readb(addr) & ~0x07;

    GENMASK()?

    ...

    > + addr += (1 << TS72XX_NAND_BUSY_ADDR_LINE);

    BIT() ?

    ...

    > + return !!(readb(addr) & 0x20);

    BIT() ?

    ...

    > + struct ts72xx_nand_data *data;
    > + struct mtd_info *mtd;
    > + int err = 0;

    Redundant assignment.

    > + /* Allocate memory for the device structure (and zero it) */
    > + data = devm_kzalloc(&pdev->dev, sizeof(struct ts72xx_nand_data),

    sizeof(*data) and make it a single line.

    > + GFP_KERNEL);
    > + if (!data)
    > + return -ENOMEM;

    ...

    > + nand_set_flash_node(&data->chip, pdev->dev.of_node);

    Hmm... wondering why this uses OF node instead of fwnode... But okay, this is
    question to the subsystem maintaners.


    > + err = mtd_device_parse_register(mtd, NULL, NULL,
    > + NULL, 0);

    There is plenty of space on the previous line.

    > +

    Redundant blank line.

    > + if (!err)
    > + return err;
    > +
    > + nand_cleanup(&data->chip);
    > +
    > + return 0;

    This seems at least weird and rather broken.
    To me it looks like

    if (err) {
    nand_cleanup(&data->chip);
    return err;
    }

    return 0;

    has to be here.

    > +}

    ...

    > + ret = mtd_device_unregister(nand_to_mtd(chip));
    > + WARN_ON(ret);

    WARN_ON()?! Why?

    > + nand_cleanup(chip);
    > +}

    --
    With Best Regards,
    Andy Shevchenko


    \
     
     \ /
      Last update: 2023-06-03 22:21    [W:4.215 / U:0.036 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site