Message-ID: <f4c33946-125b-459c-aa95-f34e7fb11185@linaro.org>
Date: Mon, 18 Nov 2024 09:41:36 +0100
From: neil.armstrong@...aro.org
To: Rob Herring <robh@...nel.org>
Cc: Xianwei Zhao <xianwei.zhao@...ogic.com>,
 Linus Walleij <linus.walleij@...aro.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Kevin Hilman <khilman@...libre.com>,
 Jerome Brunet <jbrunet@...libre.com>,
 Martin Blumenstingl <martin.blumenstingl@...glemail.com>,
 Bartosz Golaszewski <brgl@...ev.pl>, linux-gpio@...r.kernel.org,
 devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
 linux-amlogic@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v6 4/5] pinctrl: meson: Add driver support for Amlogic A4
 SoCs

Hi Rob,

On 15/11/2024 18:14, Rob Herring wrote:
> On Thu, Nov 14, 2024 at 3:21 AM Neil Armstrong
> <neil.armstrong@...aro.org> 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@...ogic.com>
>>>> ---
>>>>    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@...ogic.com>
>>>> + *         Huqiang Qin <huqiang.qin@...ogic.com>
>>>> + */
>>>> +
>>>> +#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

Powered by blists - more mailing lists