Message ID | 20210506162514.5913-2-denis281089@gmail.com |
---|---|
State | Superseded |
Delegated to: | Koen Vandeputte |
Headers | show |
Series | RFC: ath79: add support for Mikrotik RB91xG | expand |
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?
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
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