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

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

Hi Rob,
    Thanks for your reply.

On 2024/12/17 22:20, Rob Herring wrote:
> [ EXTERNAL EMAIL ]
> 
> On Wed, Dec 11, 2024 at 02:47:49PM +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.yaml          | 150 +++++++++++++++++++++
>>   include/dt-bindings/pinctrl/amlogic,pinctrl.h      |  68 ++++++++++
>>   2 files changed, 218 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/pinctrl/amlogic,pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/amlogic,pinctrl.yaml
>> new file mode 100644
>> index 000000000000..b0c2ae585d7d
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/pinctrl/amlogic,pinctrl.yaml
>> @@ -0,0 +1,150 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/pinctrl/amlogic,pinctrl.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
> 
> Only one Amlogic pinctrl block? Pretty sure there's more than 1 SoC and
> pinctrl is unlikely identical for all. This must be SoC specific.
>

Will do, "amlogic,pinctrl" -> "amlogic,pinctrl-a4".

>> +
>> +  reg:
>> +    minItems: 2
>> +
>> +  reg-names:
>> +    items:
>> +      - const: mux
>> +      - const: gpio
>> +
>> +  "#address-cells":
>> +    const: 2
>> +
>> +  "#size-cells":
>> +    const: 2
> 
> Why do you need these? You don't have any child nodes with addresses.
> But maybe you should?
> 

I will modify "reg_mux_offset" to "reg", So
#address-cells = <1>
#size-cells = <0>

>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - reg-names
>> +  - "#address-cells"
>> +  - "#size-cells"
>> +
>> +patternProperties:
>> +  "^gpio[a-z]":
> 
> This allows "gpioanythingyouwant" for example. If there is no
> unit-address, then it should be '^gpio-[0-9]+$'.
> 

Will do.

>> +    type: object
>> +
>> +    properties:
>> +      gpio-controller: true
>> +
>> +      "#gpio-cells":
>> +        const: 2
>> +
>> +      bank-name:
>> +        $ref: /schemas/types.yaml#/definitions/string
> 
> You must define values, but why do you need this? No other GPIO
> controller needs something like this.
> 

We define the bank name(GPIOA,GPIOB) on the chip specification, so it is 
more readable to describe the source code with such a name in the 
pin(GPIOA-0,GPIOA-1).

Bank indexes are defined primarily for use in pinmux implicit queries.

Bank-name and bank-index, they both describe the characteristics of the 
Bank, they just use it differently. I can keep bank-index, I can query 
the bank-name through bank-index.

I will delete this property, keep bank-index property.

>> +
>> +      npins:
> 
> We have a standard property for this: ngpios
> 

Will do.

>> +        $ref: /schemas/types.yaml#/definitions/uint32
>> +        minimum: 1
>> +        maximum: 32
>> +
>> +      bank-index:
>> +        $ref: /schemas/types.yaml#/definitions/uint32
>> +        minimum: 0
>> +        maximum: 29
> 
> Consider if 'bank' should be part of the gpio cells rather than
> separate controller nodes. That works better if each bank is not its own
> separate h/w block. If banks share a register, then that's not separate
> blocks.
>

Each bank is its own separate h/w block(With TEST_N bank exception,it 
share mux reg with AO bank).

Change the name of this property to "bank-identity" make it more 
appropriate.

>> +
>> +      reg-mux-offset:
>> +        $ref: /schemas/types.yaml#/definitions/uint32
>> +
>> +      bit-mux-offset:
>> +        $ref: /schemas/types.yaml#/definitions/uint32
>> +
>> +      reg-gpio-offset:
>> +        $ref: /schemas/types.yaml#/definitions/uint32
> 
> Put these into 'reg'. We even have a way to deal with bit offsets. See
> leds/register-bit-led.yaml for example.
> 

Will do.

>  >> +
>> +    required:
>> +      - gpio-controller
>> +      - "#gpio-cells"
>> +      - npins
>> +      - bank-index
>> +      - reg-mux-offset
>> +      - reg-gpio-offset
>> +      - bank-name
>> +
>> +    additionalProperties: false
>> +    unevaluatedProperties: false
> 
> Don't need both. 'additionalProperties' is more restrictive, so drop
> unevaluatedProperties.
> 
Will drop  unevaluatedProperties.

>> +
>> +  "^func-[0-9a-z]":
> 
> "^func-[0-9a-z-]+$"
> 
Will do.

>> +    type: object
>> +    additionalProperties:
>> +      type: object
>> +      allOf:
>> +        - $ref: pincfg-node.yaml#
>> +        - $ref: pinmux-node.yaml#
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    apb {
>> +      #address-cells = <2>;
>> +      #size-cells = <2>;
>> +      periphs_pinctrl: pinctrl@8e700 {
>> +        compatible = "amlogic,pinctrl";
>> +        #address-cells = <2>;
>> +        #size-cells = <2>;
>> +        reg = <0x0 0x8e700 0x0 0x04>,
>> +              <0x0 0x8e704 0x0 0x60>;
>> +        reg-names = "mux", "gpio";
>> +
>> +        gpiob {
>> +          gpio-controller;
>> +          #gpio-cells = <2>;
>> +          npins = <10>;
>> +          bank-index = <1>;
>> +          reg-mux-offset = <0x14>;
>> +          bit-mux-offset = <0x14>;
>> +          reg-gpio-offset = <0x30>;
>> +          bank-name = "GPIOB";
>> +        };
>> +
>> +        gpioe {
>> +          gpio-controller;
>> +          #gpio-cells = <2>;
>> +          npins = <10>;
>> +          bank-index = <5>;
>> +          reg-mux-offset = <0x14>;
>> +          reg-gpio-offset = <0x30>;
>> +          bank-name = "GPIOE";
>> +        };
>> +
>> +        func-uart-b {
>> +          uart-b-default{
>> +            pinmux = <3>;
>> +            bias-pull-up;
>> +            drive-strength-microamp = <4000>;
>> +          };
>> +
>> +          uart-c-default{
>> +            pinmux = <4>;
>> +            bias-pull-up;
>> +            drive-strength-microamp = <4000>;
>> +          };
>> +        };
>> +
>> +        func-uart-c {
>> +          uart-c-default{
>> +            pinmux = <3>;
>> +            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
>> +
>> +#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