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
