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

[Xianwei Zhao <xianwei.zhao@amlogic.com>]

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