Received-SPF: pass (google.com: domain of linux-kernel+bounces-214582-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <42752f1c-fa79-450c-bbf8-f55464db14ae@amlogic.com>
Date: Fri, 14 Jun 2024 16:51:01 +0800
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 1/3] dt-bindings: pinctrl: Add support for Amlogic A4 SoCs
Content-Language: en-US
To: Rob Herring <robh@kernel.org>
Cc: Linus Walleij <linus.walleij@linaro.org>,
 Krzysztof Kozlowski <krzk+dt@kernel.org>, Conor Dooley
 <conor+dt@kernel.org>, Neil Armstrong <neil.armstrong@linaro.org>,
 Kevin Hilman <khilman@baylibre.com>, Jerome Brunet <jbrunet@baylibre.com>,
 Martin Blumenstingl <martin.blumenstingl@googlemail.com>,
 Bartosz Golaszewski <brgl@bgdev.pl>, linux-gpio@vger.kernel.org,
 devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
 linux-amlogic@lists.infradead.org, linux-kernel@vger.kernel.org
References: <20240611-a4_pinctrl-v1-0-dc487b1977b3@amlogic.com>
 <20240611-a4_pinctrl-v1-1-dc487b1977b3@amlogic.com>
 <20240613170816.GA2020944-robh@kernel.org>
From: Xianwei Zhao <xianwei.zhao@amlogic.com>
In-Reply-To: <20240613170816.GA2020944-robh@kernel.org>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?utf-8?B?cHRvZ2ZxYzNuK2k2bHhBbWMrREdsdkl5bnpwZFdpVWQ5VnJrTkZFU0hFRlhD?=
 =?utf-8?B?L2JqVkdOWWE0R2NuL2hEQnA3U1ExS3FVUmRrVEVTSE1ydHhlS0tsVFNSTkxI?=
 =?utf-8?B?S0lRbzNsRnEzclJGNU9zZ0xIZlBRcmlSbkpNSXVHVVhOWTFQL2M4LzMxNGVB?=
 =?utf-8?B?M0xvckx0dHVaek51TkJVWmFaSUdpTTJjZGV2UUw5a0luWWNYQkRJUGNJNnl6?=
 =?utf-8?B?WmgvVXU0d0lNYjdqTWtxN3cvTmxYYURTTmRhZm15YlhkN0t5TkVSSDVzTGZO?=
 =?utf-8?B?SHNSSE44TVpEeHhBSVp5T3BYa3owcFNaMlUrS0lhU0w4SS9rL2dDUXJ3czJE?=
 =?utf-8?B?UFJoU2Z2L1l4LzltUmI3Z2xDamhkNVVJeXJnWlR3WHNBcW85akZuaWpXODhj?=
 =?utf-8?B?R09CMFFaVUJGY0hmTnlETC9sNjBHZWN5Qy9FeENzUjJYd3pPZmJqQlBrQUMz?=
 =?utf-8?B?WDdzMVhCUzdZU1Jpak9iYWlDdlc4TDUvSWVkVGhoNmhoVG1qUkttaXhHREIw?=
 =?utf-8?B?NmdjczFlSExDU0NLcXJ3RURmWmNWNUV3M2swUDF4WTB0RzJEbmplZkM1Z3ov?=
 =?utf-8?B?aVdXMUJqdk5Eb2RranZMbXpGWkZ5dkhYdlNidThYNWMwZXpMRTZLQ2hvWFJx?=
 =?utf-8?B?cmFtcHdhaWpQYTJKQ1BmalBNTC9lQWxEbndkR0RmczlZL3hvckVrSkNuTjVP?=
 =?utf-8?B?MStibmFrNkJSdllkUDlDSDU3Zi9GemtkZG9JOGY4NmJvWjFOWEhDZFF0WTc3?=
 =?utf-8?B?RDVIeHpQem9SeFdyeEZ0NklrN0VsNE1qOTlWeXg0Q0pwUEJNYjFvYWhzdHQ0?=
 =?utf-8?B?eDJnTkNrVmk1MktWTVNVY3NuSUVVS0grSHN3WUo4aC9jd1V3RjNIOXZBODYz?=
 =?utf-8?B?Y1J4RFNybnNNaTAxNTZIVW5qcUEvMGtCT1NWQ2FidTlqcG81QjBHSCtMbmNy?=
 =?utf-8?B?UDF5OEhpR1dZdStyU3UvS2hKbEw2R3Q0a0pPbjlTa3YzU3NoN0JmQ2tMVHdn?=
 =?utf-8?B?QkY0OVErc3NCMkFldEFGdGFtYWtQQVZNZHo2K0pwRU5HMXRhV3VYMEgrSW1Z?=
 =?utf-8?B?YVhsaGplSVdrRmptK0p2TXJ6bHJrRGJNaHltNTJaRWZQQld3RU1KWmZhVjcz?=
 =?utf-8?B?MzViLzNXMVpVbG9NOHVLZVpxaU83YlF2dE5adHBQM08ycXVYSmhFUEtzbzBq?=
 =?utf-8?B?enZ0ZnJrdXU5QVhhOGlJUzVLV2JVcFBQMWI0ZDV1aTd1bndadUNRTVRPbTZs?=
 =?utf-8?B?TFRPNXIrZzZ5bmdYb0ZUWis2d0svL1RTVEdJMnZ6d0loTmo1QU1WVVZYSWFm?=
 =?utf-8?B?aEFvQ3hnYUt2U09MRkhHNGVGTW1qaWxPLzRTS1V0bDNyYTh5M1pFMklqeUtw?=
 =?utf-8?B?NjUyaXhpRTJoZDYyRVJaN01PRHErSTdKa2pLN21qUmp5Y1JQWVdiaWk0c05z?=
 =?utf-8?B?YXU5VmZMOVVKYk83OVFZYVV4cTRSSUJXTmNzK1o5YTRyZHFmY0M0d0F3cHJE?=
 =?utf-8?B?MDB1cG0xbnJjZFV3dFZubkJWVGNDVzJ1dFlISlFDVXlTTlhZejByWjVOMXR6?=
 =?utf-8?B?SGpWMnlVUXNqNk1iZzhWa1pCd1hLOHJoN1BFeFJLck1vd0pvU1dLcHBDaG1T?=
 =?utf-8?B?dk4weVVYd1QxMUw4a0Jld3J3clBicHAvRUxvenV3NnY4V0tMVHNIM1RyT2hL?=
 =?utf-8?B?dUluR000akN1azBmL0hYYkNsamRTL0g3WU9MT2xsaWVnUTNjTTBRU2ZMREJw?=
 =?utf-8?B?V28xT0lEdllCQU4wVWtzbHZUZUZsb0swcW16cTB5Z0p1SWp5dG4wLzVIaDM1?=
 =?utf-8?B?aUVyUFk3Tm5PMDVmVnJNejc3SzZZdUJYa01lTFpCdUNoU0h3SCtnU1hpd0tW?=
 =?utf-8?B?Q1FIVjZVVTJXWlZmV0dma3pZcCttWlVtcUgydjk5Q0Eza0JzUEo5eE10dlJ1?=
 =?utf-8?B?Z3hDL1NQTHMyTnhML21pVjd4bnNFMnBueW5xM052WnNlKzRwS2VKVWUzV0lW?=
 =?utf-8?B?TE5sU3hVdnVuNlhZRngvQkZFcXRabDdnQ2IxVXFycG4vNURBR2NLN2t5VkY3?=
 =?utf-8?B?L0Y0Rm9IZ1dUZHFmMHViNEdPQm40NTRHNXdWNGRHM1F6aUE2bC9YQjIreUJv?=
 =?utf-8?B?YStIanB5VjNBWTFCMUZiNmhuRmlvOEorQ25OVDNmc2pLRWI2UGRQTnowQ3Zo?=
 =?utf-8?B?alE9PQ==?=
X-OriginatorOrg: amlogic.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 57bda40a-c10c-4954-2cf9-08dc8c4f237a
X-MS-Exchange-CrossTenant-AuthSource: TYZPR03MB6896.apcprd03.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 14 Jun 2024 08:51:10.7312
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 0df2add9-25ca-4b3a-acb4-c99ddf0b1114
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: k3l4CYE+S+/OucuqLNmMRs/q7Js8lYB/PZwE9M4nPd0Kcm1rK9OA8+443bTCGqQWq52CTJP/0z7e5h80xvgr8fW4i4g9/JBLfRXpJhLJNKk=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SEZPR03MB7193

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