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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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),                                   \
                },                                                     \
        }

+
+#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.

Rob




[Index of Archives]     [Linux SPI]     [Linux Kernel]     [Linux ARM (vger)]     [Linux ARM MSM]     [Linux Omap]     [Linux Arm]     [Linux Tegra]     [Fedora ARM]     [Linux for Samsung SOC]     [eCos]     [Linux Fastboot]     [Gcc Help]     [Git]     [DCCP]     [IETF Announce]     [Security]     [Linux MIPS]     [Yosemite Campsites]

  Powered by Linux