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

[Krzysztof Kozlowski <krzk@xxxxxxxxxx>]

On 14/06/2024 10:51, Xianwei Zhao wrote:
> 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@xxxxxxxxxxx>
>>> ---
>>>   .../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@xxxxxxxxxxx>
>>> + */
>>> +
>>> +#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),                                   \
>                  },                                                     \
>          }

Still 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@xxxxxxxxxxx>
>>> + */
>>> +
>>> +#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.

Which part of Rob's feedback needs more explanation?


Best regards,
Krzysztof