Re: [PATCH 1/4] ath79: add MFD driver (NAND and GPIO) for Mikrotik RB91xG
On Thu, May 6, 2021 at 7:31 PM Denis Kalashnikov 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 = <_gpio 0 ...>,
<_gpio 1 ...>,
...
<_gpio 15 ...>;
le-gpios = <_gpio 11 ...>;
};
nand {
compatible = "mikrotik,rb91x-nand";
data-gpios = <_gpio 0 ...>,
<_gpio 1 ...>,
...
<_gpio 7 ...>;
ctrl-gpios = <_gpio 12 ...>, /* NAND RDY */
<_gpio 13 ...>, /* NAND nCE */
<_gpio 15 ...>, /* NAND ALE */
<_gpio 14 ...>, /* NAND CLE */
<_gpio 12 ...>, /* NAND nRW */
<_gpio 11 ...>; /* NAND Read */
ctrl-latch-gpios = <_gpio 16 ...>;
...
};
{
...
cs-gpios = <0>, <_gpio 8 ...>;
...
};
...
led_power {
gpios = <_gpio 9 ...>;
};
led_user {
gpios = <_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?
--
Sergey
___
openwrt-devel mailing list
openwrt-devel@lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel
Re: [PATCH 1/4] ath79: add MFD driver (NAND and GPIO) for Mikrotik RB91xG
On Thu, May 6, 2021 at 7:31 PM Denis Kalashnikov 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
> ---
> .../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 00..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
> + *
> + */
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include
> +
> +#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(>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(>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(>mutex);
> +
> + gpio_set_value_cansleep(ngl->gpio[RB91X_NGL_LATCH_GPIO0 + offset],
> val);
> +
> + mutex_unlock(>mutex);
> +}
> +
> +static int rb91x_ngl_gpio_get_value(struct rb91x_ngl *ngl, int offset)
> +{
> + if (OFFSET_INVAL(offset))
> + return -EINVAL;
>
RE: [PATCH 1/4] ath79: add MFD driver (NAND and GPIO) for Mikrotik RB91xG
Hi,
> -Original Message-
> From: openwrt-devel [mailto:openwrt-devel-boun...@lists.openwrt.org]
> On Behalf Of Denis Kalashnikov
> Sent: Donnerstag, 6. Mai 2021 18:25
> To: openwrt-devel@lists.openwrt.org
> Cc: Gabor Juhos
> 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
> ---
> .../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 00..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
> + *
> + */
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include
> +
> +#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(>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(>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(>mutex);
> +
> + gpio_set_value_cansleep(ngl->gpio[RB91X_NGL_LATCH_GPIO0 +
> offset],
> +val);
> +
> + mutex_unlock(>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) {
