Hi Rob,
On 15/11/2024 18:14, Rob Herring wrote:
On Thu, Nov 14, 2024 at 3:21 AM Neil Armstrong
<neil.armstrong@xxxxxxxxxx> wrote:
On 13/11/2024 19:04, Rob Herring wrote:
On Wed, Nov 13, 2024 at 03:29:42PM +0800, Xianwei Zhao wrote:
Add a new pinctrl driver for Amlogic A4 SoCs which share
the same register layout as the previous Amlogic S4.
Signed-off-by: Xianwei Zhao <xianwei.zhao@xxxxxxxxxxx>
---
drivers/pinctrl/meson/Kconfig | 6 +
drivers/pinctrl/meson/Makefile | 1 +
drivers/pinctrl/meson/pinctrl-amlogic-a4.c | 1324 ++++++++++++++++++++++++++++
3 files changed, 1331 insertions(+)
diff --git a/drivers/pinctrl/meson/Kconfig b/drivers/pinctrl/meson/Kconfig
index cc397896762c..3e90bb5ec442 100644
--- a/drivers/pinctrl/meson/Kconfig
+++ b/drivers/pinctrl/meson/Kconfig
@@ -67,6 +67,12 @@ config PINCTRL_MESON_S4
select PINCTRL_MESON_AXG_PMX
default y
+config PINCTRL_AMLOGIC_A4
+ tristate "Amlogic A4 SoC pinctrl driver"
+ depends on ARM64
+ select PINCTRL_MESON_AXG_PMX
+ default y
+
config PINCTRL_AMLOGIC_C3
tristate "Amlogic C3 SoC pinctrl driver"
depends on ARM64
diff --git a/drivers/pinctrl/meson/Makefile b/drivers/pinctrl/meson/Makefile
index 9e538b9ffb9b..c92a65a83344 100644
--- a/drivers/pinctrl/meson/Makefile
+++ b/drivers/pinctrl/meson/Makefile
@@ -10,5 +10,6 @@ obj-$(CONFIG_PINCTRL_MESON_AXG) += pinctrl-meson-axg.o
obj-$(CONFIG_PINCTRL_MESON_G12A) += pinctrl-meson-g12a.o
obj-$(CONFIG_PINCTRL_MESON_A1) += pinctrl-meson-a1.o
obj-$(CONFIG_PINCTRL_MESON_S4) += pinctrl-meson-s4.o
+obj-$(CONFIG_PINCTRL_AMLOGIC_A4) += pinctrl-amlogic-a4.o
obj-$(CONFIG_PINCTRL_AMLOGIC_C3) += pinctrl-amlogic-c3.o
obj-$(CONFIG_PINCTRL_AMLOGIC_T7) += pinctrl-amlogic-t7.o
diff --git a/drivers/pinctrl/meson/pinctrl-amlogic-a4.c b/drivers/pinctrl/meson/pinctrl-amlogic-a4.c
new file mode 100644
index 000000000000..edc5f2ba2c8a
--- /dev/null
+++ b/drivers/pinctrl/meson/pinctrl-amlogic-a4.c
@@ -0,0 +1,1324 @@
+// SPDX-License-Identifier: (GPL-2.0-only OR MIT)
+/*
+ * Pin controller and GPIO driver for Amlogic A4 SoC.
+ *
+ * Copyright (c) 2024 Amlogic, Inc. All rights reserved.
+ * Author: Xianwei Zhao <xianwei.zhao@xxxxxxxxxxx>
+ * Huqiang Qin <huqiang.qin@xxxxxxxxxxx>
+ */
+
+#include "pinctrl-meson.h"
+#include "pinctrl-meson-axg-pmx.h"
+#include <dt-bindings/gpio/amlogic-gpio.h>
+
+/* Standard port */
+
+#define GPIOE_0 0
+#define GPIOE_1 1
+
+#define GPIOD_0 2
+#define GPIOD_1 3
+#define GPIOD_2 4
+#define GPIOD_3 5
+#define GPIOD_4 6
+#define GPIOD_5 7
+#define GPIOD_6 8
+#define GPIOD_7 9
+#define GPIOD_8 10
+#define GPIOD_9 11
+#define GPIOD_10 12
+#define GPIOD_11 13
+#define GPIOD_12 14
+#define GPIOD_13 15
+#define GPIOD_14 16
+#define GPIOD_15 17
The conversion from bank+index to a single index space seems less than
ideal, and looks like a work-around to fit into the existing driver from
a brief look at it.
Not really, it simply adds a custom xlate per SoC, nothing particulary hacky.
I was relunctant at first, but since Xianwei added the plumbing for a per-SoC
xlate, then it was easy to add 3-cells support.
If there's not really banks of GPIOs here, then DT shouldn't have them
either. The question is does anything need to know the bank number
and/or index? If it's only for human readability (and matching to
datasheet), then just something like this can be done:
#define GPIOD(n) (2 + (n))
There's no linear mapping possible, each set of gpios is grouped into logical
"banks" per group of functions, and this grouping is also in the gpio controller
register space.
v1 had just that. So it is possible.
Exact, it had just that like all the other amlogic pinctrl drivers since 2017,
So I waited for the bindings to be reviewed as usual, but then the bindings
were rejected.
So here we are now, trying to not "using defines for GPIOs" as requested,
and Xianwei tried his best and we agreed on this 3-cells solution which
is much better designed on the bindings side and won't need anymore
header file for future SoCs bindings which is a huge win.
The small downside is a custom xlate function, but as I pointed, there's
no linear mapping possible between banks, the current bindings with a
set of defines for each gpio still requires a calculation to get the
register/offset value, so this solution doesn't change much here.
Requesting a gpio is a slow path, adding 2 comparisons and an addition
is an acceptable cost to not add more header files in the future.
Neil
> No one reviewed the driver then,> so I don't know if that was less than ideal on the driver side. But it
looks like that linear map was just moved from DT into the driver. Why
are there a bunch of defines creating a linear mapping? The xlate()
function just converts bank+index into a linear number. At some point
you need arrays of settings I suppose, but why do you need a #define
for every index? Why not an array per bank? I suppose you could use
the above define within the driver to at least not have a bunch of
defines.
Rob
