Re: [PATCH v6 4/5] pinctrl: meson: Add driver support for Amlogic A4 SoCs

[Rob Herring <robh@xxxxxxxxxx>]

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