Re: [PATCH v3 06/10] dt-bindings: pinctrl: mediatek,mt6779-pinctrl: Add MT6795

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

 




On Mon, Oct 10 2022 at 07:24:59 -04:00:00, Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx> wrote:
On 07/10/2022 08:59, Yassine Oudjana wrote:
 From: Yassine Oudjana <y.oudjana@xxxxxxxxxxxxxx>

 Combine MT6795 pin controller document into MT6779 one. In the
 process, replace the current interrupts property description with
 the one from the MT6795 document since it makes more sense. Also
 amend property descriptions and examples with more detailed
 information that was available in the MT6795 document, and replace
 the current pinmux node name patterns with ones from it since they
 are more common across mediatek pin controller bindings.

 Signed-off-by: Yassine Oudjana <y.oudjana@xxxxxxxxxxxxxx>
 ---
  .../pinctrl/mediatek,mt6779-pinctrl.yaml      |  94 ++++++--
.../pinctrl/mediatek,pinctrl-mt6795.yaml | 227 ------------------
  2 files changed, 77 insertions(+), 244 deletions(-)
delete mode 100644 Documentation/devicetree/bindings/pinctrl/mediatek,pinctrl-mt6795.yaml

diff --git a/Documentation/devicetree/bindings/pinctrl/mediatek,mt6779-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/mediatek,mt6779-pinctrl.yaml
 index a2141eb0854e..cada3530dd0a 100644
--- a/Documentation/devicetree/bindings/pinctrl/mediatek,mt6779-pinctrl.yaml +++ b/Documentation/devicetree/bindings/pinctrl/mediatek,mt6779-pinctrl.yaml
 @@ -8,6 +8,7 @@ title: Mediatek MT6779 Pin Controller

  maintainers:
    - Andy Teng <andy.teng@xxxxxxxxxxxx>
+ - AngeloGioacchino Del Regno <angelogioacchino.delregno@xxxxxxxxxxxxx>
    - Sean Wang <sean.wang@xxxxxxxxxx>

  description:
 @@ -18,6 +19,7 @@ properties:
    compatible:
      enum:
        - mediatek,mt6779-pinctrl
 +      - mediatek,mt6795-pinctrl
        - mediatek,mt6797-pinctrl

    reg:
 @@ -43,9 +45,10 @@ properties:
    interrupt-controller: true

    interrupts:
 -    maxItems: 1
 +    minItems: 1
 +    maxItems: 2
      description: |
 -      Specifies the summary IRQ.
 +      The interrupt outputs to sysirq.

I am not sure if description is relevant now for all variants... what is
the sysirq? You have two interrupts so both go to one sysirq?

It's the system interrupt controller and it has several inputs. Both interrupts go to it.



    "#interrupt-cells":
      const: 2
 @@ -81,6 +84,30 @@ allOf:
              - const: iocfg_lt
              - const: iocfg_tl
              - const: eint
 +
 +        interrupts:
 +          items:
 +            - description: EINT interrupt
 +
 +  - if:
 +      properties:
 +        compatible:
 +          contains:
 +            const: mediatek,mt6795-pinctrl
 +    then:
 +      properties:
 +        reg:
 +          minItems: 2

What's the maxItems? You declared reg and reg-names in top-level
properties as accepting anything, therefore you cannot have loose
constraints here.

That was an oversight. I'll fix it.

 +
 +        reg-names:
 +          items:
 +            - const: base
 +            - const: eint
 +
 +        interrupts:
 +          items:
 +            - description: EINT interrupt
 +            - description: EINT event_b interrupt

Blank line

    - if:
        properties:
          compatible:
 @@ -111,32 +138,50 @@ allOf:
          - "#interrupt-cells"

  patternProperties:
 -  '-[0-9]*$':
 +  '-pins$':
      type: object
      additionalProperties: false

      patternProperties:
 -      '-pins*$':
 +      '^pins':
          type: object
          description: |
A pinctrl node should contain at least one subnodes representing the pinctrl groups available on the machine. Each subnode will list the pins it needs, and how they should be configured, with regard to muxer - configuration, pullups, drive strength, input enable/disable and input schmitt.
 -        $ref: "/schemas/pinctrl/pincfg-node.yaml"
+ configuration, pullups, drive strength, input enable/disable and
 +          input schmitt.
 +        $ref: "pinmux-node.yaml"

Drop quotes

Why this one is not pincfg-node anymore? All your properties are not
valid then? You mix here so many changes it is a bit difficult to
understand the concept.

Seems like I didn't pay enough attention to that. This node actually takes both pinmux-node (pinmux specifically) and pincfg-node properties, so would it make sense to add ref for both?



          properties:
            pinmux:
              description:
- integer array, represents gpio pin number and mux setting. - Supported pin number and mux varies for different SoCs, and are defined
 -              as macros in boot/dts/<soc>-pinfunc.h directly.
+ Integer array, represents gpio pin number and mux setting. + Supported pin number and mux varies for different SoCs, and are + defined as macros in dt-bindings/pinctrl/<soc>-pinfunc.h
 +              directly.

            bias-disable: true

 -          bias-pull-up: true
 -
 -          bias-pull-down: true
 +          bias-pull-up:
 +            oneOf:
 +              - type: boolean
 +              - enum: [100, 101, 102, 103]

Missing ref

 +                description: Pull up PUPD/R0/R1 type define value.
 +            description: |
+ For normal pull up type, it is not necessary to specify R1R0 + values; When pull up type is PUPD/R0/R1, adding R1R0 defines
 +               will set different resistance values.
 +
 +          bias-pull-down:
 +            oneOf:
 +              - type: boolean
 +              - enum: [100, 101, 102, 103]

Missing ref

+ description: Pull down PUPD/R0/R1 type define value.
 +            description: |
+ For normal pull down type, it is not necessary to specify R1R0 + values; When pull down type is PUPD/R0/R1, adding R1R0 defines
 +               will set different resistance values.

            input-enable: true

 @@ -151,7 +196,7 @@ patternProperties:
            input-schmitt-disable: true

            drive-strength:
 -            enum: [2, 4, 8, 12, 16]
 +            enum: [2, 4, 6, 8, 10, 12, 14, 16]

Now you are missing ref - you do not have a type now, because you
removed pincfg-node. Split the merging of different pinctrl bindings and
reorganization.

Will do.


The drive strengths are also not valid for the other variant...

Actually the supported drive strengths vary between pins of a single variant, so technically they have never been described completely accurately. The old drive strenghs are a superset of strengths supported by pins on the MT6779 pin controller, and this change expands the superset with values supported by some pins in MT6795. Would it be better to move this to the conditionals to have it defined per variant?


            slew-rate:
              enum: [0, 1]
 @@ -218,8 +263,9 @@ examples:
              #interrupt-cells = <2>;
              interrupts = <GIC_SPI 204 IRQ_TYPE_LEVEL_HIGH>;

 -            mmc0_pins_default: mmc0-0 {
 -                cmd-dat-pins {
 +            /* GPIOs 167-174, 176-178 set as multifunction MSDC0 */
 +            mmc0_pins_default: mmc0-pins {
 +                pins-cmd-dat {
                      pinmux = <PINMUX_GPIO168__FUNC_MSDC0_DAT0>,
                          <PINMUX_GPIO172__FUNC_MSDC0_DAT1>,
                          <PINMUX_GPIO169__FUNC_MSDC0_DAT2>,
 @@ -232,15 +278,29 @@ examples:
                      input-enable;
                      mediatek,pull-up-adv = <1>;
                  };
 -                clk-pins {
 +                pins-clk {
                      pinmux = <PINMUX_GPIO176__FUNC_MSDC0_CLK>;
                      mediatek,pull-down-adv = <2>;
                  };
 -                rst-pins {
 +                pins-rst {
                      pinmux = <PINMUX_GPIO178__FUNC_MSDC0_RSTB>;
                      mediatek,pull-up-adv = <0>;
                  };

Best regards,
Krzysztof






[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