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

[Xianwei Zhao <xianwei.zhao@amlogic.com>]

Hi Rob,
    Thanks for your reply.

On 2025/1/3 05:24, Rob Herring wrote:
> [ EXTERNAL EMAIL ]
> 
> On Thu, Dec 26, 2024 at 03:57:41PM +0800, Xianwei Zhao wrote:
>> Add the dt-bindings for Amlogic pin controller, and add a new
>> dt-binding header file which document the GPIO bank names and
>> alternative func value of all Amlogic subsequent SoCs.
>>
>> Signed-off-by: Xianwei Zhao <xianwei.zhao@amlogic.com>
>> ---
>>   .../bindings/pinctrl/amlogic,pinctrl-a4.yaml       | 155 +++++++++++++++++++++
>>   include/dt-bindings/pinctrl/amlogic,pinctrl.h      |  68 +++++++++
>>   2 files changed, 223 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/pinctrl/amlogic,pinctrl-a4.yaml b/Documentation/devicetree/bindings/pinctrl/amlogic,pinctrl-a4.yaml
>> new file mode 100644
>> index 000000000000..75863d179933
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/pinctrl/amlogic,pinctrl-a4.yaml
>> @@ -0,0 +1,155 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/pinctrl/amlogic,pinctrl-a4.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Amlogic pinmux controller
>> +
>> +maintainers:
>> +  - Xianwei Zhao <xianwei.zhao@amlogic.com>
>> +
>> +allOf:
>> +  - $ref: pinctrl.yaml#
>> +
>> +properties:
>> +  compatible:
>> +    const: amlogic,pinctrl-a4
>> +
>> +  reg:
>> +    minItems: 2
>> +
>> +  reg-names:
>> +    items:
>> +      - const: mux
>> +      - const: gpio
>> +
>> +  "#address-cells":
>> +    const: 1
>> +
>> +  "#size-cells":
>> +    const: 0
> 
> The addresses in 'reg' below are MMIO, right? If so, #size-cells can't
> be 0 and you also need 'ranges' here.
> 

Will add ranges here, reg and reg-names move into gpio object.

>> +
>> +required:
> 
> Move after 'patternProperties'.
> 
Will do.
>> +  - compatible
>> +  - reg
>> +  - reg-names
>> +  - "#address-cells"
>> +  - "#size-cells"
>> +
>> +patternProperties:
>> +  "^gpio@[0-9a-f]+$":
>> +    type: object
>> +
>> +    properties:
>> +      reg:
>> +        minItems: 2
> 
> You need to describe what each entry is:
> 
> items:
>    - description: foo
>    - description: bar
> 
Will add description in next version
>> +
>> +      mask:
>> +        $ref: /schemas/types.yaml#/definitions/uint32
>> +
>> +      gpio-controller: true
>> +
>> +      "#gpio-cells":
>> +        const: 2
>> +
>> +      ngpios:
>> +        $ref: /schemas/types.yaml#/definitions/uint32
> 
> Don't need a type here.
> 

Will drop ngpios, use gpio_rangs instead.

>> +        minimum: 1
>> +        maximum: 32
>> +
>> +      identity:
> 
> Needs a better name. This is the bank number?
> 

named 'bank-number'.

> But shouldn't you be able to use gpio-ranges here:
> 
> gpio-ranges = <&periphs_pinctrl 0 (bank# << 8) (value of npgios)>;
> 
> And do you really need ngpios if you have the value here?
> 
>> +        description: |
>> +          identifier are provided by the pin controller header file at:
>> +          <include/dt-bindings/pinctrl/amlogic,pinctrl.h>
>> +
>> +        $ref: /schemas/types.yaml#/definitions/uint32
>> +
>> +    required:
>> +      - reg
>> +      - gpio-controller
>> +      - "#gpio-cells"
>> +      - ngpios
>> +      - identity
>> +
>> +    additionalProperties: false
>> +
>> +  "^func-[0-9a-z-]+$":
>> +    type: object
>> +    additionalProperties:
> 
> Define a pattern for the node names instead. We only allow anything for
> existing bindings.
>

Will use patternProperies

>> +      type: object
>> +      allOf:
>> +        - $ref: pincfg-node.yaml#
>> +        - $ref: pinmux-node.yaml#
>> +
>> +      description:
>> +        A pin multiplexing sub-node describes how to configure a set of (or a
>> +        single) pin in some desired alternate function mode.
>> +        A single sub-node may define several pin configurations.
>> +
>> +      properties:
>> +        pinmux:
>> +          description: |
>> +            Integer array representing pin number and pin multiplexing
>> +            configuration.
>> +            When a pin has to be configured in alternate function mode, use
>> +            this property to identify the pin by its global index, and provide
>> +            its alternate function configuration number along with it.
>> +            bank identifier are provided by the pin controller header file at:
>> +            <include/dt-bindings/pinctrl/amlogic,pinctrl.h>
>> +            Integers values in "pinmux" argument list are assembled as:
>> +            (((BANK << 8) + PIN) << 8)  | MUX_FUNC))
>> +
>> +      required:
>> +        - pinmux
>> +
>> +      additionalProperties: true
> 
> No, not allowed to be true except on common schemas.
> 

This object  I will only save
      allOf:
        - $ref: pincfg-node.yaml#
        - $ref: pinmux-node.yaml#
others(description, properties,required,  additionalProperties) will be 
dropped.

>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/pinctrl/amlogic,pinctrl.h>
>> +    apb {
>> +      #address-cells = <2>;
>> +      #size-cells = <2>;
>> +      periphs_pinctrl: pinctrl@8e700 {
>> +        compatible = "amlogic,pinctrl-a4";
>> +        #address-cells = <1>;
>> +        #size-cells = <0>;
>> +        reg = <0x0 0x8e700 0x0 0x04>,
>> +              <0x0 0x8e704 0x0 0x60>;
>> +        reg-names = "mux", "gpio";
>> +
>> +        gpio@14 {
>> +          reg = <0x14>,<0x30>;
>> +          gpio-controller;
>> +          #gpio-cells = <2>;
>> +          ngpios = <10>;
>> +          identity = <AMLOGIC_GPIO_B>;
>> +        };
>> +
>> +        func-uart-b {
>> +          uart-b-default {
>> +            pinmux = <AML_PINMUX(AMLOGIC_GPIO_B, 1, AF4)>;
>> +            bias-pull-up;
>> +            drive-strength-microamp = <4000>;
>> +          };
>> +
>> +          uart-b-pins1 {
>> +            pinmux = <AML_PINMUX(AMLOGIC_GPIO_B, 5, AF2)>;
>> +            bias-pull-up;
>> +            drive-strength-microamp = <4000>;
>> +          };
>> +        };
>> +
>> +        func-uart-c {
>> +          uart-c-default {
>> +            pinmux = <AML_PINMUX(AMLOGIC_GPIO_B, 3, AF1)>,
>> +                     <AML_PINMUX(AMLOGIC_GPIO_B, 2, AF1)>;
>> +            bias-pull-up;
>> +            drive-strength-microamp = <4000>;
>> +          };
>> +        };
>> +      };
>> +    };
>> diff --git a/include/dt-bindings/pinctrl/amlogic,pinctrl.h b/include/dt-bindings/pinctrl/amlogic,pinctrl.h
>> new file mode 100644
>> index 000000000000..03db0a730e8b
>> --- /dev/null
>> +++ b/include/dt-bindings/pinctrl/amlogic,pinctrl.h
>> @@ -0,0 +1,68 @@
>> +/* 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_PINCTRL_H
>> +#define _DT_BINDINGS_AMLOGIC_PINCTRL_H
>> +
>> +/* define PIN modes */
>> +#define AF0  0x0
>> +#define AF1  0x1
>> +#define AF2  0x2
>> +#define AF3  0x3
>> +#define AF4  0x4
>> +#define AF5  0x5
>> +#define AF6  0x6
>> +#define AF7  0x7
>> +#define AF8  0x8
>> +#define AF9  0x9
>> +#define AF10 0xa
>> +#define AF11 0xb
>> +#define AF12 0xc
>> +#define AF13 0xd
>> +#define AF14 0xe
>> +#define AF15 0xf
> 
> There's no need for defines in the form "FOOn n".
> 

Will do.

>> +
>> +#define AML_PIN_ALT_FUNC_MASK        0xf
>> +
>> +/* Normal PIN bank */
>> +#define AMLOGIC_GPIO_A               0
>> +#define AMLOGIC_GPIO_B               1
>> +#define AMLOGIC_GPIO_C               2
>> +#define AMLOGIC_GPIO_D               3
>> +#define AMLOGIC_GPIO_E               4
>> +#define AMLOGIC_GPIO_F               5
>> +#define AMLOGIC_GPIO_G               6
>> +#define AMLOGIC_GPIO_H               7
>> +#define AMLOGIC_GPIO_I               8
>> +#define AMLOGIC_GPIO_J               9
>> +#define AMLOGIC_GPIO_K               10
>> +#define AMLOGIC_GPIO_L               11
>> +#define AMLOGIC_GPIO_M               12
>> +#define AMLOGIC_GPIO_N               13
>> +#define AMLOGIC_GPIO_O               14
>> +#define AMLOGIC_GPIO_P               15
>> +#define AMLOGIC_GPIO_Q               16
>> +#define AMLOGIC_GPIO_R               17
>> +#define AMLOGIC_GPIO_S               18
>> +#define AMLOGIC_GPIO_T               19
>> +#define AMLOGIC_GPIO_U               20
>> +#define AMLOGIC_GPIO_V               21
>> +#define AMLOGIC_GPIO_W               22
>> +#define AMLOGIC_GPIO_X               23
>> +#define AMLOGIC_GPIO_Y               24
>> +#define AMLOGIC_GPIO_Z               25
>> +
>> +/* Special PIN bank */
>> +#define AMLOGIC_GPIO_DV              26
>> +#define AMLOGIC_GPIO_AO              27
>> +#define AMLOGIC_GPIO_CC              28
>> +#define AMLOGIC_GPIO_TEST_N  29
>> +
>> +#define AML_PINMUX(bank, offset, mode)       (((((bank) << 8) + (offset)) << 8) | (mode))
>> +#define AML_PINMUX_TO_BANK(pinmux)   (((pinmux) >> 16) & 0xff)
>> +#define AML_PINMUX_TO_OFFSET(pinmux) (((pinmux) >> 8) & 0xff)
>> +
>> +#endif /* _DT_BINDINGS_AMLOGIC_PINCTRL_H */
>>
>> --
>> 2.37.1
>>

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic