[PATCH 1/4] ath79: add MFD driver (NAND and GPIO) for Mikrotik RB91xG

Sergey Ryazanov ryazanov.s.a at gmail.com
Fri May 14 04:03:03 BST 2021


On Thu, May 6, 2021 at 7:31 PM Denis Kalashnikov <denis281089 at gmail.com> wrote:
> rb91x-ngl (nand-gpio-latch)

I would like to suggest rename it to 'rb91x-latch'. This driver has no
NAND or GPIO functionality. Looks like it just multiplexes access to
subordinate HW, what is clearly indicated by the device tree, where
the latch is a parent for NAND and GPIO nodes. Besides this, the
'latch' name already looks puzzling, while using NGL abbreviation adds
another layer of puzzling. See referenced 'rb41x-cpld' driver. IMHO it
utilizes a perfect naming scheme.

> requests and controls SoC GPIO
> lines that are used for NAND control and data lines multiplexed
> with a latch. Lines of the latch that are not used for NAND
> control lines, are used for power LED and user LED and a Shift
> Register nCS.

As a common note, you are using the legacy GPIO API. Please use modern
API from linux/gpio/consumer.h

> Like rb4xx-cpld driver rb91x-ngl provides API for separate
> NAND driver and latch-GPIO driver.
>
> This driver is used in place of the ar71xx gpio-latch driver.
>
> Signed-off-by: Denis Kalashnikov <denis281089 at gmail.com>
> ---
>  .../linux/ath79/files/drivers/mfd/rb91x-ngl.c | 331 ++++++++++++++++++
>  .../linux/ath79/files/include/mfd/rb91x-ngl.h |  59 ++++
>  target/linux/ath79/mikrotik/config-default    |   1 +
>  .../patches-5.4/939-mikrotik-rb91x.patch      |  21 ++
>  4 files changed, 412 insertions(+)
>  create mode 100644 target/linux/ath79/files/drivers/mfd/rb91x-ngl.c
>  create mode 100644 target/linux/ath79/files/include/mfd/rb91x-ngl.h
>  create mode 100644 target/linux/ath79/patches-5.4/939-mikrotik-rb91x.patch
>
> diff --git a/target/linux/ath79/files/drivers/mfd/rb91x-ngl.c b/target/linux/ath79/files/drivers/mfd/rb91x-ngl.c
> new file mode 100644
> index 0000000000..c6ab4631f5
> --- /dev/null
> +++ b/target/linux/ath79/files/drivers/mfd/rb91x-ngl.c
> @@ -0,0 +1,331 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * MFD driver for the MikroTik RouterBoard NAND controlled through GPIO
> + * multiplexed with latch. Why MFD, not pure NAND driver? Since the latch
> + * lines, that are not used for NAND control lines, are used for GPIO
> + * output function -- for leds and other.
> + *
> + * Copyright (C) 2021 Denis Kalashnikov <denis281089 at gmail.com>
> + *
> + */
> +#include <linux/mutex.h>
> +#include <linux/mfd/core.h>
> +#include <linux/module.h>
> +#include <linux/of_platform.h>
> +#include <linux/of_gpio.h>
> +
> +#include <mfd/rb91x-ngl.h>
> +
> +#define DRIVER_NAME "rb91x-nand-gpio-latch"
> +
> +#define NAND_DATAS 8
> +#define LATCH_GPIOS 3
> +
> +static int nand_datas_count(struct rb91x_ngl *ngl)
> +{
> +       return NAND_DATAS;
> +}
> +
> +static int latch_gpios_count(struct rb91x_ngl *ngl)
> +{
> +       return LATCH_GPIOS;
> +}

Should this information be provided in the runtime? You could move
NAND_DATAS and LATCH_GPIOS definitions to the common header file and
use them directly in the subordinate device drivers.

> +static void latch_lock(struct rb91x_ngl *ngl)

Please use a driver name as a common prefix for each driver function,
e.g. rb91x_latch_lock(). When someone sees a call for this function,
the prefix helps to distinguish whether it is a generic kernel
function call or some driver specific routine invocation. Also such
prefixing helps to avoid naming conflicts with generic kernel symbols.

> +{
> +       mutex_lock(&ngl->mutex);
> +
> +       gpio_set_value_cansleep(ngl->gpio[RB91X_NGL_NLE], 0);
> +}
> +
> +static void latch_unlock(struct rb91x_ngl *ngl)

ditto

> +{
> +       gpio_set_value_cansleep(ngl->gpio[RB91X_NGL_NLE], 1);
> +
> +       mutex_unlock(&ngl->mutex);
> +}

You could add a 'bool lock' argument and combine these two functions
into one to reduce the API a bit.

> +
> +#define OFFSET_INVAL(offset) ((offset) < 0 || (offset) >= RB91X_NGL_GPIOS)

You can define the 'offset' argument as an unsigned integer to avoid
checking for negative value. As for upper range bound, it is better to
check not against a constant that was used to define an array size,
but against the actual array size if array is static:

if (offset >= ARRAY_SIZE(ngl->gpio))

This is more flexible, clearly indicates the purpose of a check (i.e.
self descriptive) and is less error prone.

> +static void rb91x_ngl_gpio_set_value(struct rb91x_ngl *ngl, int offset, int val)
> +{
> +       if (OFFSET_INVAL(offset))
> +               return;

If you simplify the offset validation as I suggested above, then maybe
it is better to avoid macro usage and do the check explicitly to keep
code clear? E.g.

if (offset >= ARRAY_SIZE(ngl->gpio))
    return;

> +       gpio_set_value_cansleep(ngl->gpio[offset], val);
> +}
> +
> +static void latch_gpio_set_value(struct rb91x_ngl *ngl, int offset, int val)
> +{
> +       if (offset >= LATCH_GPIOS)
> +               return;
> +
> +       mutex_lock(&ngl->mutex);
> +
> +       gpio_set_value_cansleep(ngl->gpio[RB91X_NGL_LATCH_GPIO0 + offset], val);
> +
> +       mutex_unlock(&ngl->mutex);
> +}
> +
> +static int rb91x_ngl_gpio_get_value(struct rb91x_ngl *ngl, int offset)
> +{
> +       if (OFFSET_INVAL(offset))
> +               return -EINVAL;
> +
> +       return gpio_get_value(ngl->gpio[offset]);
> +}
> +
> +static void rb91x_ngl_gpio_direction_output(struct rb91x_ngl *ngl, int offset,
> +                                      int val)
> +{
> +       if (OFFSET_INVAL(offset))
> +               return;
> +
> +       gpio_direction_output(ngl->gpio[offset], val);
> +}
> +
> +static void rb91x_ngl_gpio_direction_input(struct rb91x_ngl *ngl, int offset)
> +{
> +       if (OFFSET_INVAL(offset))
> +               return;
> +
> +       gpio_direction_input(ngl->gpio[offset]);
> +}
> +
> +static const struct mfd_cell mfd_cells[] = {
> +       {
> +               .name = "mikrotik,rb91x-nand",

There is no need to use a vendor name prefix in the 'name' property.
It should be .name = "rb91x-nand"

> +               .of_compatible = "mikrotik,rb91x-nand",
> +       }, {
> +               .name = "mikrotik,rb91x-gpio-latch",
> +               .of_compatible = "mikrotik,rb91x-gpio-latch",
> +       },
> +};
> +
> +static int get_gpios(struct device *dev, const char *prop_name)
> +{
> +       int n;
> +
> +       n = of_get_named_gpio(dev->of_node, prop_name, 0);
> +       if (n < 0)
> +               dev_err(dev, "Could not read required '%s' property: %d\n", prop_name, n);
> +       //pr_info(DRIVER_NAME ": %s = %d\n", prop_name, n);
> +
> +       return n;
> +}
> +
> +/*
> + * NOTE: all gpios are labeled with driver name, not with @name.
> + * @name is used only in error message. Since we failed to choose
> + * a good names for multiplexed gpios.
> + */
> +static int req_gpio(struct device *dev, int gpio, const char *name)
> +{
> +       int ret;
> +
> +       ret = devm_gpio_request(dev, gpio, DRIVER_NAME);
> +       if (ret) {
> +               pr_err(DRIVER_NAME ": failed to request gpio %d ('%s'): %d\n",
> +                       gpio, name, ret);

dev_err(dev, ...)

> +               return ret;
> +       }
> +
> +       //pr_info(DRIVER_NAME ": request gpio %d ('%s')\n", gpio, name);
> +
> +       return ret;
> +}
> +
> +static int probe(struct platform_device *pdev)

Prefix function name with the driver name, please.

> +{
> +       struct device_node *of_node = pdev->dev.of_node;
> +       struct device *dev = &pdev->dev;
> +       struct rb91x_ngl *ngl;
> +       int i, n, ret;
> +
> +       pr_info("rb91x-nand-gpio-latch driver probe\n");

Remove this custom tracing output on the final submission. Such output
produces no meaningful information, only noise.

> +       ngl = devm_kzalloc(dev, sizeof(*ngl), GFP_KERNEL);
> +       if (!ngl)
> +               return -ENOMEM;
> +
> +       /* TODO: read gpios flags (active high/low) */
> +
> +       for (i = 0; i < RB91X_NGL_GPIOS; i++) {
> +               ngl->gpio[i] = -ENOENT;
> +       }
> +
> +       /* Read NAND control gpios */
> +       ngl->gpio[RB91X_NGL_NAND_NCE] = get_gpios(dev, "nand-nce-gpios");
> +       ngl->gpio[RB91X_NGL_NAND_CLE] = get_gpios(dev, "nand-cle-gpios");
> +       ngl->gpio[RB91X_NGL_NAND_ALE] = get_gpios(dev, "nand-ale-gpios");
> +       ngl->gpio[RB91X_NGL_NAND_NRW] = get_gpios(dev, "nand-nrw-gpios");
> +       ngl->gpio[RB91X_NGL_NAND_RDY] = get_gpios(dev, "nand-rdy-gpios");
> +       ngl->gpio[RB91X_NGL_NAND_READ] = get_gpios(dev, "nand-read-gpios");

You could collect names, indexes, offsets and other information that
are describing the I/O line usage in an array and then simplify a lot
of code places by referencing that array and performing operations in
a loop. E.g.

static const struct rb91x_latch_line_info {
    const char *propname;
    unsigned int offset;
    ...
} rb91x_latch_lines_info[] = {
    {"nand-nce-gpios", RB91X_NGL_NAND_NCE, ... },
    {"nand-cle-gpios", RB91X_NGL_NAND_CLE, ... },
    ...
};

...

static int rb91x_latch_probe(...)
{
    ....
   for (info = rb91x_latch_lines_info; info->name != NULL; ++info)
        ngl->gpio[info->offset] = get_gpios(dev, info->propname);

This will make the driver data-driven and notably reduce its code
length. See more comments in the DTS patch.

> +       ngl->gpio[RB91X_NGL_NLE] = get_gpios(dev, "nle-gpios");
> +
> +       /* Read NAND data gpios */
> +
> +       n = of_gpio_named_count(of_node, "nand-data-gpios");
> +       if (n != NAND_DATAS) {
> +               dev_err(dev, DRIVER_NAME

There is no need to use DRIVER_NAME with dev_err(), since dev_err()
will prefix a message with a device name.

> +                 ": required 'nand-data-gpios' property must have %d gpios\n",
> +                 NAND_DATAS);
> +               return -EINVAL;
> +       }
> +
> +       //dev_info(dev, DRIVER_NAME ": nand-data-gpios count = %d\n", n);
> +
> +       for (i = 0; i < n; i++) {
> +               ret = of_get_named_gpio(of_node, "nand-data-gpios", i);
> +               if (ret < 0) {
> +                       dev_err(dev, DRIVER_NAME
> +                         ": Couldn't read required 'nand-data-gpios': %d\n",
> +                         ret);
> +                       return -EINVAL;
> +               }
> +
> +               //dev_info(dev, DRIVER_NAME ": nand-data-gpios = %d\n", ret);
> +
> +               ngl->gpio[RB91X_NGL_NAND_DATA0 + i] = ret;
> +       }
> +
> +       /* Read latch gpios */
> +
> +       n = of_gpio_named_count(of_node, "latch-gpios");
> +       if (n != LATCH_GPIOS) {
> +               dev_err(dev, DRIVER_NAME
> +                 ": required 'latch-gpios' property must have %d gpios\n",
> +                 LATCH_GPIOS);
> +               return -EINVAL;
> +       }
> +
> +       //dev_info(dev, DRIVER_NAME ": latch-gpios count = %d\n", n);
> +
> +       for (i = 0; i < n; i++) {
> +               ret = of_get_named_gpio(of_node, "latch-gpios", i);
> +               if (ret < 0) {
> +                       dev_err(dev, DRIVER_NAME
> +                         ": Couldn't read required 'latch-gpios': %d\n",
> +                         ret);
> +                       return -EINVAL;
> +               }
> +
> +               //dev_info(dev, DRIVER_NAME ": latch-gpios = %d\n", ret);
> +
> +               ngl->gpio[RB91X_NGL_LATCH_GPIO0 + i] = ret;
> +       }
> +
> +       if (ngl->gpio[RB91X_NGL_NAND_NCE] < 0
> +        || ngl->gpio[RB91X_NGL_NAND_CLE] < 0
> +        || ngl->gpio[RB91X_NGL_NAND_ALE] < 0
> +        || ngl->gpio[RB91X_NGL_NAND_NRW] < 0
> +        || ngl->gpio[RB91X_NGL_NAND_RDY] < 0
> +        || ngl->gpio[RB91X_NGL_NAND_READ] < 0
> +        || ngl->gpio[RB91X_NGL_NLE] < 0)
> +           return -EINVAL;

Convert to a loop?

> +
> +       /* Request gpios */
> +
> +       if (req_gpio(dev, ngl->gpio[RB91X_NGL_NLE], "nLE"))
> +               return -EINVAL;
> +
> +       if (req_gpio(dev, ngl->gpio[RB91X_NGL_NAND_NCE], "NAND-nCE"))
> +               return -EINVAL;
> +
> +       if (req_gpio(dev, ngl->gpio[RB91X_NGL_NAND_CLE], "NAND-CLE"))
> +               return -EINVAL;
> +
> +       if (req_gpio(dev, ngl->gpio[RB91X_NGL_NAND_ALE], "NAND-ALE"))
> +               return -EINVAL;
> +
> +       if (req_gpio(dev, ngl->gpio[RB91X_NGL_NAND_NRW], "NAND-nRW"))
> +               return -EINVAL;
> +
> +       if (req_gpio(dev, ngl->gpio[RB91X_NGL_NAND_RDY], "NAND-RDY"))
> +               return -EINVAL;
> +
> +       if (req_gpio(dev, ngl->gpio[RB91X_NGL_NAND_READ], "NAND-READ"))
> +               return -EINVAL;

Maybe this set of similar calls should be converted to a loop over the
array of lines info?

> +       for (i = 0; i < NAND_DATAS; i++) {
> +               /*
> +                * Some data gpios are equal to control gpios.
> +                * Check this.
> +                */
> +               n = ngl->gpio[RB91X_NGL_NAND_DATA0 + i];
> +               if (n == ngl->gpio[RB91X_NGL_NAND_NCE]
> +                || n == ngl->gpio[RB91X_NGL_NAND_CLE]
> +                || n == ngl->gpio[RB91X_NGL_NAND_ALE]
> +                || n == ngl->gpio[RB91X_NGL_NAND_NRW]
> +                || n == ngl->gpio[RB91X_NGL_NAND_RDY]
> +                || n == ngl->gpio[RB91X_NGL_NAND_READ])
> +                       continue;
> +               if (req_gpio(dev, n, "NAND-DATAx"))
> +                       return -EINVAL;
> +       }
> +
> +       /*
> +        * NOTE: We suppose that latch gpios are equal to some
> +        * control gpios, so they have been already requested.
> +        */
> +
> +       ngl->nand_datas_count = nand_datas_count;
> +       ngl->latch_lock = latch_lock;
> +       ngl->latch_unlock = latch_unlock;
> +       ngl->gpio_set_value = rb91x_ngl_gpio_set_value;
> +       ngl->gpio_get_value = rb91x_ngl_gpio_get_value;
> +       ngl->gpio_direction_input = rb91x_ngl_gpio_direction_input;
> +       ngl->gpio_direction_output = rb91x_ngl_gpio_direction_output;
> +       ngl->latch_gpio_set_value = latch_gpio_set_value;
> +       ngl->latch_gpios_count = latch_gpios_count;
> +
> +       mutex_init(&ngl->mutex);
> +
> +       dev_set_drvdata(dev, ngl);
> +
> +       /*
> +        * All gpios and the latch are controlled by NAND driver,
> +        * but we need to init gpio lines for the latch gpio in case
> +        * of NAND driver is missing.
> +        */
> +
> +       /* Unlock the latch */
> +       gpio_direction_output(ngl->gpio[RB91X_NGL_NLE], 1);
> +
> +       /* TODO: are latch gpio lines active high or low? */
> +       for (i = 0; i < LATCH_GPIOS; i++) {
> +               gpio_direction_output(ngl->gpio[RB91X_NGL_LATCH_GPIO0 + i], 0);
> +       }
> +
> +       return devm_mfd_add_devices(dev, PLATFORM_DEVID_NONE,
> +                                   mfd_cells, ARRAY_SIZE(mfd_cells),
> +                                   NULL, 0, NULL);
> +}
> +
> +static int remove(struct platform_device *pdev)
> +{
> +       return 0;
> +}
> +
> +static const struct of_device_id match[] = {

Please prefix global variables with the driver name too.

> +       { .compatible = "mikrotik,nand-gpio-latch", },

Should this be a compatible = "mikrotik,rb91x-ngl" or compatible =
"mikrotik,rb91x-latch"?

> +       { },
> +};
> +MODULE_DEVICE_TABLE(of, match);
> +
> +static struct platform_driver rb91x_ngl_driver = {
> +       .probe = probe,
> +       .remove = remove,
> +       .driver = {
> +               .name = DRIVER_NAME,
> +               .owner = THIS_MODULE,
> +               .of_match_table = of_match_ptr(match),
> +       },
> +};
> +
> +module_platform_driver(rb91x_ngl_driver);
> +
> +MODULE_DESCRIPTION("Driver for Mikrotik RouterBoard 91x NAND controlled through GPIOs multiplexed with latch");
> +MODULE_AUTHOR("Denis Kalashnikov <denis281089 at gmail.com>");
> +MODULE_LICENSE("GPL v2");
> diff --git a/target/linux/ath79/files/include/mfd/rb91x-ngl.h b/target/linux/ath79/files/include/mfd/rb91x-ngl.h
> new file mode 100644
> index 0000000000..5360aa7548
> --- /dev/null
> +++ b/target/linux/ath79/files/include/mfd/rb91x-ngl.h

This header should be placed in the include/*linux*/mfd/ directory.

> @@ -0,0 +1,59 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * MFD driver for the MikroTik RouterBoard 91xG NAND controlled
> + * through GPIOs multiplexed with latch (rb91x-nand-gpio-latch).
> + *
> + * Copyright (C) 2021 Denis Kalashnikov <denis281089 at gmail.com>
> + */
> +
> +#include <linux/mutex.h>
> +
> +enum rb91x_ngl_gpios {

What is the purpose of this enum? Is it used to define all possible
constants or to enumerate lines that are available for subordinate
devices? It causes a lot of questions (see below), but maybe I just
wrongly understand its purpose. Please clarify.

> +       /* NAND control gpios */
> +       RB91X_NGL_NAND_NCE, /* nCE -- Chip Enable, Active Low */
> +       RB91X_NGL_NAND_CLE, /* CLE -- Command Latch Enable */
> +       RB91X_NGL_NAND_ALE, /* ALE -- Address Latch Enable */
> +       RB91X_NGL_NAND_NRW, /* nRW -- Read/Write Enable, Active Low */
> +       RB91X_NGL_NAND_RDY, /* RDY -- NAND Ready */
> +       RB91X_NGL_NAND_READ, /* READ */
> +
> +       RB91X_NGL_NLE, /* nLE -- Latch Enable, Active Low */

The latch enable line has a special purpose and even a dedicated API
to control it. Why did you describe it here as a regular output line?

> +       /* NAND data gpios */
> +       RB91X_NGL_NAND_DATA0,
> +
> +       /* Latch gpios */
> +       RB91X_NGL_LATCH_GPIO0 = RB91X_NGL_NAND_DATA0 + 32,
> +
> +       RB91X_NGL_GPIOS = RB91X_NGL_LATCH_GPIO0 + 32, /* Total number of gpios */

LVC573A has only 8 lines, so why is the number of GPIOs 32?

> +};
> +
> +struct rb91x_ngl {
> +       /* Public */
> +
> +       /* API for RB91x NAND controller driver */
> +       void (*gpio_set_value)(struct rb91x_ngl *ngl, int offset, int val);
> +       void (*gpio_direction_input)(struct rb91x_ngl *ngl, int offset);
> +       void (*gpio_direction_output)(struct rb91x_ngl *ngl, int offset,
> +         int val);

Just an idea. There is only one line that is used as input (NAND RDY).
NAND data line directions are in fact configured by the rb91x-nand
driver directly via the SoC GPIO controller registers (bypassing the
latch driver). So you could configure line directions in the latch
driver probe function and even do not export the line direction
configuration wrapper functions.

> +       int (*gpio_get_value)(struct rb91x_ngl *ngl, int offset);
> +       void (*latch_lock)(struct rb91x_ngl *ngl);
> +       void (*latch_unlock)(struct rb91x_ngl *ngl);
> +       int (*nand_datas_count)(struct rb91x_ngl *ngl);
> +
> +       /* API for RB91x gpio latch controller driver */
> +       void (*latch_gpio_set_value)(struct rb91x_ngl *ngl, int offset,
> +         int val);
> +       int (*latch_gpios_count)(struct rb91x_ngl *ngl);
> +

Is it possible to export all these functions not as function pointers
within the hardly accessible structure, but as regular functions with
the driver name prefix? E.g.

void rb91x_latch_lock(struct deivce *dev, bool lock);
void rb91x_latch_gpio_set(struct device *dev, enum rb91x_ngl_gpios
gpio, int val);
int rb91x_latch_gpio_get(struct device *dev, enum rb91x_ngl_gpios gpio);

This will make parent device access less hackish, less error prone and
more clear. If MFD exports functions in a such way, then subordinate
drivers no longer depend on the parent private data. For example see
TPS6586x MFD driver and its interface defined in linux/mfd/tps6586x.h

> +
> +       /* Private */
> +
> +       /*
> +        * To synchronize access of NAND driver and GPIO driver
> +        * to shared gpio lines.
> +        */
> +       struct mutex mutex;
> +
> +       int gpio[RB91X_NGL_GPIOS];

Private data should be kept private to the driver. If you prefer to
use this hackish way to export MFD driver functions, you could rename
public structure to struct rb91x_latch_ops, and define the private
driver data structure inside the driver source code file in the
following way:

struct rb91x_latch_priv {
    struct rb91x_latch_ops ops;    /* Should be first */
    struct mutex mutex;
    int gpio[...];
};

This will make driver internal data really private and do not break
subordinate drivers assumption about that the beginning of the parent
device private data contains pointers to the MFD device operations.

But I suggest you avoid such hackish access methods and use simple
functions exports (see above).

--
Sergey



More information about the openwrt-devel mailing list