[1/3] dt-bindings: pinctrl: Add support for Amlogic A4 SoCs

From: Xianwei Zhao <xianwei.zhao@amlogic.com>

Add the new compatible name for Amlogic A4 pin controller, and add
a new dt-binding header file which document the detail pin names.

Signed-off-by: Xianwei Zhao <xianwei.zhao@amlogic.com>
---
 .../bindings/pinctrl/amlogic,meson-pinctrl-a1.yaml |  2 +
 .../dt-bindings/gpio/amlogic,a4-aobus-pinctrl.h    | 21 +++++
 .../dt-bindings/gpio/amlogic,a4-periphs-pinctrl.h  | 93 ++++++++++++++++++++++
 3 files changed, 116 insertions(+)

On Tue, Jun 11, 2024 at 01:10:57PM +0800, Xianwei Zhao wrote:
> Add the new compatible name for Amlogic A4 pin controller, and add
> a new dt-binding header file which document the detail pin names.
> 
> Signed-off-by: Xianwei Zhao <xianwei.zhao@amlogic.com>
> ---
>  .../bindings/pinctrl/amlogic,meson-pinctrl-a1.yaml |  2 +
>  .../dt-bindings/gpio/amlogic,a4-aobus-pinctrl.h    | 21 +++++
>  .../dt-bindings/gpio/amlogic,a4-periphs-pinctrl.h  | 93 ++++++++++++++++++++++
>  3 files changed, 116 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/pinctrl/amlogic,meson-pinctrl-a1.yaml b/Documentation/devicetree/bindings/pinctrl/amlogic,meson-pinctrl-a1.yaml
> index d9e0b2c48e84..f5eefa0fab5b 100644
> --- a/Documentation/devicetree/bindings/pinctrl/amlogic,meson-pinctrl-a1.yaml
> +++ b/Documentation/devicetree/bindings/pinctrl/amlogic,meson-pinctrl-a1.yaml
> @@ -15,6 +15,8 @@ allOf:
>  properties:
>    compatible:
>      enum:
> +      - amlogic,a4-aobus-pinctrl
> +      - amlogic,a4-periphs-pinctrl
>        - amlogic,c3-periphs-pinctrl
>        - amlogic,t7-periphs-pinctrl
>        - amlogic,meson-a1-periphs-pinctrl
> diff --git a/include/dt-bindings/gpio/amlogic,a4-aobus-pinctrl.h b/include/dt-bindings/gpio/amlogic,a4-aobus-pinctrl.h
> new file mode 100644
> index 000000000000..7c7e746baed5
> --- /dev/null
> +++ b/include/dt-bindings/gpio/amlogic,a4-aobus-pinctrl.h
> @@ -0,0 +1,21 @@
> +/* SPDX-License-Identifier: (GPL-2.0-only OR MIT) */
> +/*
> + * Copyright (c) 2024 Amlogic, Inc. All rights reserved.
> + * Author: Xianwei Zhao <xianwei.zhao@amlogic.com>
> + */
> +
> +#ifndef _DT_BINDINGS_AMLOGIC_A4_AOBUS_PINCTRL_H
> +#define _DT_BINDINGS_AMLOGIC_A4_AOBUS_PINCTRL_H
> +
> +/* GPIOAO */
> +#define GPIOAO_0			0
> +#define GPIOAO_1			1
> +#define GPIOAO_2			2
> +#define GPIOAO_3			3
> +#define GPIOAO_4			4
> +#define GPIOAO_5			5
> +#define GPIOAO_6			6

I find defines with the value of the define in the name pretty 
pointless.

> +
> +#define GPIO_TEST_N			7
> +
> +#endif
> diff --git a/include/dt-bindings/gpio/amlogic,a4-periphs-pinctrl.h b/include/dt-bindings/gpio/amlogic,a4-periphs-pinctrl.h
> new file mode 100644
> index 000000000000..dfabca4b4790
> --- /dev/null
> +++ b/include/dt-bindings/gpio/amlogic,a4-periphs-pinctrl.h
> @@ -0,0 +1,93 @@
> +/* SPDX-License-Identifier: (GPL-2.0-only OR MIT) */
> +/*
> + * Copyright (c) 2024 Amlogic, Inc. All rights reserved.
> + * Author: Xianwei Zhao <xianwei.zhao@amlogic.com>
> + */
> +
> +#ifndef _DT_BINDINGS_AMLOGIC_A4_PERIPHS_PINCTRL_H
> +#define _DT_BINDINGS_AMLOGIC_A4_PERIPHS_PINCTRL_H
> +
> +/* GPIOE */
> +#define GPIOE_0				0
> +#define GPIOE_1				1
> +
> +/* GPIOD */
> +#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

I'm not really much of a fan of using defines for GPIOs, but if you do, 
wouldn't be better to split banks and lines up rather than a global 
number space. See ASPEED_GPIO() or tegra header.

Rob

Hi Rob,
      Thanks for your review.

On 2024/6/14 01:08, Rob Herring wrote:
> [ EXTERNAL EMAIL ]
> 
> On Tue, Jun 11, 2024 at 01:10:57PM +0800, Xianwei Zhao wrote:
>> Add the new compatible name for Amlogic A4 pin controller, and add
>> a new dt-binding header file which document the detail pin names.
>>
>> Signed-off-by: Xianwei Zhao <xianwei.zhao@amlogic.com>
>> ---
>>   .../bindings/pinctrl/amlogic,meson-pinctrl-a1.yaml |  2 +
>>   .../dt-bindings/gpio/amlogic,a4-aobus-pinctrl.h    | 21 +++++
>>   .../dt-bindings/gpio/amlogic,a4-periphs-pinctrl.h  | 93 ++++++++++++++++++++++
>>   3 files changed, 116 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/pinctrl/amlogic,meson-pinctrl-a1.yaml b/Documentation/devicetree/bindings/pinctrl/amlogic,meson-pinctrl-a1.yaml
>> index d9e0b2c48e84..f5eefa0fab5b 100644
>> --- a/Documentation/devicetree/bindings/pinctrl/amlogic,meson-pinctrl-a1.yaml
>> +++ b/Documentation/devicetree/bindings/pinctrl/amlogic,meson-pinctrl-a1.yaml
>> @@ -15,6 +15,8 @@ allOf:
>>   properties:
>>     compatible:
>>       enum:
>> +      - amlogic,a4-aobus-pinctrl
>> +      - amlogic,a4-periphs-pinctrl
>>         - amlogic,c3-periphs-pinctrl
>>         - amlogic,t7-periphs-pinctrl
>>         - amlogic,meson-a1-periphs-pinctrl
>> diff --git a/include/dt-bindings/gpio/amlogic,a4-aobus-pinctrl.h b/include/dt-bindings/gpio/amlogic,a4-aobus-pinctrl.h
>> new file mode 100644
>> index 000000000000..7c7e746baed5
>> --- /dev/null
>> +++ b/include/dt-bindings/gpio/amlogic,a4-aobus-pinctrl.h
>> @@ -0,0 +1,21 @@
>> +/* SPDX-License-Identifier: (GPL-2.0-only OR MIT) */
>> +/*
>> + * Copyright (c) 2024 Amlogic, Inc. All rights reserved.
>> + * Author: Xianwei Zhao <xianwei.zhao@amlogic.com>
>> + */
>> +
>> +#ifndef _DT_BINDINGS_AMLOGIC_A4_AOBUS_PINCTRL_H
>> +#define _DT_BINDINGS_AMLOGIC_A4_AOBUS_PINCTRL_H
>> +
>> +/* GPIOAO */
>> +#define GPIOAO_0                     0
>> +#define GPIOAO_1                     1
>> +#define GPIOAO_2                     2
>> +#define GPIOAO_3                     3
>> +#define GPIOAO_4                     4
>> +#define GPIOAO_5                     5
>> +#define GPIOAO_6                     6
> 
> I find defines with the value of the define in the name pretty
> pointless.
> 
In the driver, this macro definition not only uses its value, but also 
uses this character, for example as following,

MESON_PIN(GPIOE_0),
#define MESON_PIN(x) PINCTRL_PIN(x, #x)

GPIO_GROUP(GPIOE_0),
#define GPIO_GROUP(gpio)                                               \
         {                                                              \
                 .name = #gpio,                                         \
                 .pins = (const unsigned int[]){ gpio },                \
                 .num_pins = 1,                                         \
                 .data = (const struct meson_pmx_axg_data[]){           \
                         PMX_DATA(0),                                   \
                 },                                                     \
         }

>> +
>> +#define GPIO_TEST_N                  7
>> +
>> +#endif
>> diff --git a/include/dt-bindings/gpio/amlogic,a4-periphs-pinctrl.h b/include/dt-bindings/gpio/amlogic,a4-periphs-pinctrl.h
>> new file mode 100644
>> index 000000000000..dfabca4b4790
>> --- /dev/null
>> +++ b/include/dt-bindings/gpio/amlogic,a4-periphs-pinctrl.h
>> @@ -0,0 +1,93 @@
>> +/* SPDX-License-Identifier: (GPL-2.0-only OR MIT) */
>> +/*
>> + * Copyright (c) 2024 Amlogic, Inc. All rights reserved.
>> + * Author: Xianwei Zhao <xianwei.zhao@amlogic.com>
>> + */
>> +
>> +#ifndef _DT_BINDINGS_AMLOGIC_A4_PERIPHS_PINCTRL_H
>> +#define _DT_BINDINGS_AMLOGIC_A4_PERIPHS_PINCTRL_H
>> +
>> +/* GPIOE */
>> +#define GPIOE_0                              0
>> +#define GPIOE_1                              1
>> +
>> +/* GPIOD */
>> +#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
> 
> I'm not really much of a fan of using defines for GPIOs, but if you do,
> wouldn't be better to split banks and lines up rather than a global
> number space. See ASPEED_GPIO() or tegra header.
> For the same reasons described above.
I want to keep the same style as the previous drive.

> Rob

Hi Rob,

Thanks for your reply!

On 2024/6/14 1:08, Rob Herring wrote:
...
>> diff --git a/include/dt-bindings/gpio/amlogic,a4-periphs-pinctrl.h b/include/dt-bindings/gpio/amlogic,a4-periphs-pinctrl.h
>> new file mode 100644
>> index 000000000000..dfabca4b4790
>> --- /dev/null
>> +++ b/include/dt-bindings/gpio/amlogic,a4-periphs-pinctrl.h
>> @@ -0,0 +1,93 @@
>> +/* SPDX-License-Identifier: (GPL-2.0-only OR MIT) */
>> +/*
>> + * Copyright (c) 2024 Amlogic, Inc. All rights reserved.
>> + * Author: Xianwei Zhao <xianwei.zhao@amlogic.com>
>> + */
>> +
>> +#ifndef _DT_BINDINGS_AMLOGIC_A4_PERIPHS_PINCTRL_H
>> +#define _DT_BINDINGS_AMLOGIC_A4_PERIPHS_PINCTRL_H
>> +
>> +/* GPIOE */
>> +#define GPIOE_0				0
>> +#define GPIOE_1				1
>> +
>> +/* GPIOD */
>> +#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
> 
> I'm not really much of a fan of using defines for GPIOs, but if you do, 
> wouldn't be better to split banks and lines up rather than a global 
> number space. See ASPEED_GPIO() or tegra header.

We have always wanted to make the GPIO code look beautiful and concise.

However, since we put all the banks together at the beginning, this also
means that the automatic numbering of gpiolib will become continuous,
while ASPEED_GPIO() will make the numbering discontinuous.

In pinctrl-meson.c, we did not assign a value to pc->chip.of_xlate,
so this will use the default translation function of_gpio_simple_xlate()

of_gpio_simple_xlate() performs a linear conversion on the parameters
passed in by DT. In this way, in pinctrl-meson.c, we can easily use
meson_calc_reg_and_bit() to get the register corresponding to the pin.

However, when we refer to the ASPEED_GPIO() approach, we have to implement
our own of_xlate to convert the macro-defined numbers (discontinuous) into
continuous numbers of gpiolib.

And we need to define each range on DT, similar to:
	periphs_pinctrl: pinctrl@4000 {
		compatible = "amlogic,a4-periphs-pinctrl";
		...
		gpio: bank@4000 {
			...
			/*  gpio_offset  pin_offset  npins  */
			gpio-ranges = <&periphs_pinctrl 0  AMLOGIC_GPIO(GPIOE, 0) 2 >,	/*  0 ~ 1 */
				      <&periphs_pinctrl 2  AMLOGIC_GPIO(GPIOD, 0) 16>,	/*  2 ~ 17 */
				      <&periphs_pinctrl 18 AMLOGIC_GPIO(GPIOB, 0) 14>,	/* 18 ~ 31 */
				      <&periphs_pinctrl 32 AMLOGIC_GPIO(GPIOX, 0) 18>,	/* 32 ~ 49 */
				      <&periphs_pinctrl 50 AMLOGIC_GPIO(GPIOT, 0) 23>;	/* 50 ~ 72 */
		};
	};
Moreover, we also need to convert the following groups of functions:
	pc->chip.get_direction = meson_gpio_get_direction;
	pc->chip.direction_input = meson_gpio_direction_input;
	pc->chip.direction_output = meson_gpio_direction_output;
	pc->chip.get = meson_gpio_get;
	pc->chip.set = meson_gpio_set;



Of course there is another way to do it. DT can be written like this:
	periphs_pinctrl: pinctrl@4000 {
		compatible = "amlogic,a4-periphs-pinctrl";
		...
		gpioe: bank@0 {
			...
		};
		gpiod: bank@1 {
			...
		};
		gpiob: bank@2 {
			...
		};
		gpiox: bank@3 {
			...
		};
		gpiot: bank@4 {
			...
		};
	};

But this still requires a lot of modifications to pinctrl-meson.c,
because pinctrl-meson.c was originally designed for one gpiochip:
	static int meson_pinctrl_parse_dt(struct meson_pinctrl *pc)
	{
		...
		chips = gpiochip_node_count(pc->dev);
		if (!chips) {
				dev_err(pc->dev, "no gpio node found\n");
				return -EINVAL;
		}
		if (chips > 1) {
				dev_err(pc->dev, "multiple gpio nodes\n");
				return -EINVAL;
		}
		...
	}


The above are the issues I know and have considered. Because it involves
a lot of modifications, and we also need to ensure compatibility with
other platforms, we do not want to do this.

There are many historical reasons why Amlogic's Pinctrl/GPIO driver is so long.
I have been considering whether to refactor Amlogic's Pinctrl driver for
a long time, but I don't know whether the community will accept my doing so.
If Amlogic's driver can be refactored, the driver will become universal.
To support new platforms, we only need to add the pinctrl node to the DT
of the new platform, without having to add a c file and h file and compatible
every time.

If I can refactor Amlogic's Pinctrl, I will provide a new version of
Pinctrl driver within this year at the latest. If not, we can't think
of a better way to make this patch better. We can only do the next best thing
and remove amlogic,a4-aobus-pinctrl.h and amlogic,a4-periphs-pinctrl.h
from the patch. Although this will not affect the function of Pinctrl,
it will not be as easy to use as before.

For example:
	When there is a macro definition:   gpios = <&gpio GPIOX_15 GPIO_ACTIVE_LOW>;
	When there is no macro definition:  gpios = <&gpio 47 GPIO_ACTIVE_LOW>;


If there is any better way to do it, please guide me.

Thank you so much！

:)

Best regards,
Huqiang