| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Sun, 16 Jun 2024 09:31:02 +0200
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Xianwei Zhao <xianwei.zhao@...ogic.com>, Rob Herring <robh@...nel.org>
Cc: Linus Walleij <linus.walleij@...aro.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
<conor+dt@...nel.org>, Neil Armstrong <neil.armstrong@...aro.org>,
Kevin Hilman <khilman@...libre.com>, Jerome Brunet <jbrunet@...libre.com>,
Martin Blumenstingl <martin.blumenstingl@...glemail.com>,
Bartosz Golaszewski <brgl@...ev.pl>, linux-gpio@...r.kernel.org,
devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
linux-amlogic@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/3] dt-bindings: pinctrl: Add support for Amlogic A4 SoCs
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@...ogic.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@...ogic.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), \
> }, \
> }
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@...ogic.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.
Which part of Rob's feedback needs more explanation?
Best regards,
Krzysztof
Powered by blists - more mailing lists