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