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

rb91x-ngl (nand-gpio-latch) 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.

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@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

Hi,

> -----Original Message-----
> From: openwrt-devel [mailto:openwrt-devel-bounces@lists.openwrt.org]
> On Behalf Of Denis Kalashnikov
> Sent: Donnerstag, 6. Mai 2021 18:25
> To: openwrt-devel@lists.openwrt.org
> Cc: Gabor Juhos <juhosg@openwrt.org>
> Subject: [PATCH 1/4] ath79: add MFD driver (NAND and GPIO) for Mikrotik
> RB91xG
> 
> rb91x-ngl (nand-gpio-latch) 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.
> 
> 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@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

Please also take care of 5.10.

Best

Adrian

> 
> 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@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;
> +}
> +
> +static void latch_lock(struct rb91x_ngl *ngl) {
> +	mutex_lock(&ngl->mutex);
> +
> +	gpio_set_value_cansleep(ngl->gpio[RB91X_NGL_NLE], 0); }
> +
> +static void latch_unlock(struct rb91x_ngl *ngl) {
> +	gpio_set_value_cansleep(ngl->gpio[RB91X_NGL_NLE], 1);
> +
> +	mutex_unlock(&ngl->mutex);
> +}
> +
> +#define OFFSET_INVAL(offset) ((offset) < 0 || (offset) >=
> +RB91X_NGL_GPIOS)
> +
> +static void rb91x_ngl_gpio_set_value(struct rb91x_ngl *ngl, int offset,
> +int val) {
> +	if (OFFSET_INVAL(offset))
> +		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",
> +		.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);
> +		return ret;
> +	}
> +
> +	//pr_info(DRIVER_NAME ": request gpio %d ('%s')\n", gpio, name);
> +
> +	return ret;
> +}
> +
> +static int probe(struct platform_device *pdev) {
> +	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");
> +
> +	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");
> +
> +	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
> +		  ": 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;
> +
> +	/* 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;
> +
> +	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[] = {
> +	{ .compatible = "mikrotik,nand-gpio-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@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
> @@ -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@gmail.com>  */
> +
> +#include <linux/mutex.h>
> +
> +enum rb91x_ngl_gpios {
> +	/* 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 */
> +
> +	/* 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
> +*/ };
> +
> +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);
> +	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);
> +
> +
> +	/* Private */
> +
> +	/*
> +	 * To synchronize access of NAND driver and GPIO driver
> +	 * to shared gpio lines.
> +	 */
> +	struct mutex mutex;
> +
> +	int gpio[RB91X_NGL_GPIOS];
> +};
> diff --git a/target/linux/ath79/mikrotik/config-default
> b/target/linux/ath79/mikrotik/config-default
> index 1e637bdfd3..67c980a491 100644
> --- a/target/linux/ath79/mikrotik/config-default
> +++ b/target/linux/ath79/mikrotik/config-default
> @@ -8,6 +8,7 @@ CONFIG_LEDS_RESET=y
>  CONFIG_LZO_DECOMPRESS=y
>  CONFIG_MDIO_GPIO=y
>  CONFIG_MFD_RB4XX_CPLD=y
> +CONFIG_MFD_RB91X_NGL=y
>  CONFIG_MIKROTIK=y
>  CONFIG_MIKROTIK_RB_SYSFS=y
>  CONFIG_MTD_NAND=y
> diff --git a/target/linux/ath79/patches-5.4/939-mikrotik-rb91x.patch
> b/target/linux/ath79/patches-5.4/939-mikrotik-rb91x.patch
> new file mode 100644
> index 0000000000..a85db0892c
> --- /dev/null
> +++ b/target/linux/ath79/patches-5.4/939-mikrotik-rb91x.patch
> @@ -0,0 +1,21 @@
> +--- a/drivers/mfd/Kconfig
> ++++ b/drivers/mfd/Kconfig
> +@@ -2020,5 +2020,10 @@ config MFD_RB4XX_CPLD
> + 	  Enables support for the CPLD chip (NAND & GPIO) on Mikrotik
> + 	  Routerboard RB4xx series.
> +
> ++config MFD_RB91X_NGL
> ++	tristate "Mikrotik RB91x NAND and GPIO driver
> ++	select MFD_CORE
> ++	depends on ATH79 || COMPILE_TEST
> ++
> + endmenu
> + endif
> +--- a/drivers/mfd/Makefile
> ++++ b/drivers/mfd/Makefile
> +@@ -257,3 +257,5 @@ obj-$(CONFIG_MFD_ROHM_BD718XX)	+=
> rohm-b
> + obj-$(CONFIG_MFD_STMFX) 	+= stmfx.o
> +
> + obj-$(CONFIG_MFD_RB4XX_CPLD)	+= rb4xx-cpld.o
> ++
> ++obj-$(CONFIG_MFD_RB91X_NGL)	+= rb91x-ngl.o
> --
> 2.26.3
> 
> 
> _______________________________________________
> openwrt-devel mailing list
> openwrt-devel@lists.openwrt.org
> https://lists.openwrt.org/mailman/listinfo/openwrt-devel

On Thu, May 6, 2021 at 7:31 PM Denis Kalashnikov <denis281089@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@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@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@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@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

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

When I thought about porting RB91x code to the ath79 target, I also
had assumed as well as you that the parent MFD device is a good choice
to model the latch functionality. But after reviewing the
implementation of this idea, I would like to discuss another design
option. Maybe we should follow the original Gabor's approach and
implement the latch driver as a GPIO controller, but in a bit more
clean way?

For this board, the MFD type latch driver substitutes the common GPIO
API with a custom GPIO API and still knows too much about NAND lines.

If we model the latch as a GPIO controller, then it will consume 9
GPIO lines and produce 17 new GPIO lines. 8 consumed lines are for the
latch data input and another one line for the latch enabled state
control. 17 produced GPIO lines will be used for: 8 GPIOs for NAND
data + 8 additional GPIOs (4 GPIOs for NAND control + 4 GPIOs for LEDs
and for SSR nCS) + 1 more line to control the latch enabled state from
the rb91x-nand driver. The latch enabling line should be passed
through the latch driver to make the driver aware about the latch chip
state to block access to the latched lines as earlier.

So DTS will looks like this:

--------------------------8<---------------------------

latch_gpio: gpio_latch {
    compatible = "gpio-latch";
    gpio-controller;
    data-gpios = <&soc_gpio 0 ...>,
                 <&soc_gpio 1 ...>,
                 ...
                 <&soc_gpio 15 ...>;
    le-gpios = <&soc_gpio 11 ...>;
};

nand {
    compatible = "mikrotik,rb91x-nand";
    data-gpios = <&latch_gpio 0 ...>,
                 <&latch_gpio 1 ...>,
                 ...
                 <&latch_gpio 7 ...>;
    ctrl-gpios = <&latch_gpio 12 ...>, /* NAND RDY */
                 <&latch_gpio 13 ...>, /* NAND nCE */
                 <&latch_gpio 15 ...>, /* NAND ALE */
                 <&latch_gpio 14 ...>, /* NAND CLE */
                 <&soc_gpio 12 ...>,   /* NAND nRW */
                 <&latch_gpio 11 ...>; /* NAND Read */
    ctrl-latch-gpios = <&latch_gpio 16 ...>;
    ...
};

&spi {
    ...
    cs-gpios = <0>, <&latch_gpio 8 ...>;
    ...
};

...

    led_power {
        gpios = <&latch_gpio 9 ...>;
    };
    led_user {
        gpios = <&latch_gpio 10 ...>;
    };

--------------------------8<---------------------------

Using this approach we need one less driver (no need for MFD). But we
lose the clear indication of the latch enabled state change operation
and return to the hook in the output value set handler of the latch
GPIO driver. Also the NAND controller node became a sibling of the
latch node in the DTS, so we partially loose hierarchy information.

Any thoughts?