diff mbox series

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

Message ID 20221007125904.55371-7-y.oudjana@protonmail.com
State New
Headers show
Series MediaTek pinctrl DT binding cleanup and MT6735 pinctrl support | expand

Commit Message

Yassine Oudjana Oct. 7, 2022, 12:59 p.m. UTC
From: Yassine Oudjana <y.oudjana@protonmail.com>

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@protonmail.com>
---
 .../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

Comments

Krzysztof Kozlowski Oct. 10, 2022, 11:24 a.m. UTC | #1
On 07/10/2022 08:59, Yassine Oudjana wrote:
> From: Yassine Oudjana <y.oudjana@protonmail.com>
> 
> 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@protonmail.com>
> ---
>  .../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@mediatek.com>
> +  - AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
>    - Sean Wang <sean.wang@kernel.org>
>  
>  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?

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


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

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

The drive strengths are also not valid for the other 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
Yassine Oudjana Oct. 20, 2022, 11:36 a.m. UTC | #2
On Mon, Oct 10 2022 at 07:24:59 -04:00:00, Krzysztof Kozlowski 
<krzysztof.kozlowski@linaro.org> wrote:
> On 07/10/2022 08:59, Yassine Oudjana wrote:
>>  From: Yassine Oudjana <y.oudjana@protonmail.com>
>> 
>>  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@protonmail.com>
>>  ---
>>   .../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@mediatek.com>
>>  +  - AngeloGioacchino Del Regno 
>> <angelogioacchino.delregno@collabora.com>
>>     - Sean Wang <sean.wang@kernel.org>
>> 
>>   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
>
Krzysztof Kozlowski Oct. 20, 2022, 12:21 p.m. UTC | #3
On 20/10/2022 07:36, Yassine Oudjana wrote:
> 
> On Mon, Oct 10 2022 at 07:24:59 -04:00:00, Krzysztof Kozlowski 
> <krzysztof.kozlowski@linaro.org> wrote:
>> On 07/10/2022 08:59, Yassine Oudjana wrote:
>>>  From: Yassine Oudjana <y.oudjana@protonmail.com>
>>>
>>>  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@protonmail.com>
>>>  ---
>>>   .../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@mediatek.com>
>>>  +  - AngeloGioacchino Del Regno 
>>> <angelogioacchino.delregno@collabora.com>
>>>     - Sean Wang <sean.wang@kernel.org>
>>>
>>>   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.

Then the naming is confusing because "sysirq" sounds like "system
interrupt".

> 
>>
>>>
>>>     "#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?

Yes, and make changes in organized way, easier to read...

> 
>>
>>>
>>>           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?

If they vary, then yes.


Best regards,
Krzysztof
AngeloGioacchino Del Regno Oct. 21, 2022, 8:19 a.m. UTC | #4
Il 20/10/22 14:21, Krzysztof Kozlowski ha scritto:
> On 20/10/2022 07:36, Yassine Oudjana wrote:
>>
>> On Mon, Oct 10 2022 at 07:24:59 -04:00:00, Krzysztof Kozlowski
>> <krzysztof.kozlowski@linaro.org> wrote:
>>> On 07/10/2022 08:59, Yassine Oudjana wrote:
>>>>   From: Yassine Oudjana <y.oudjana@protonmail.com>
>>>>
>>>>   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@protonmail.com>
>>>>   ---
>>>>    .../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@mediatek.com>
>>>>   +  - AngeloGioacchino Del Regno
>>>> <angelogioacchino.delregno@collabora.com>
>>>>      - Sean Wang <sean.wang@kernel.org>
>>>>
>>>>    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.
> 
> Then the naming is confusing because "sysirq" sounds like "system
> interrupt".
> 

Yassine: "Interrupt outputs to the system interrupt controller (sysirq)"

That will surely clear up the confusion... :-)

Cheers,
Angelo
diff mbox series

Patch

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@mediatek.com>
+  - AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
   - Sean Wang <sean.wang@kernel.org>
 
 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.
 
   "#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
+
+        reg-names:
+          items:
+            - const: base
+            - const: eint
+
+        interrupts:
+          items:
+            - description: EINT interrupt
+            - description: EINT event_b interrupt
   - 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"
 
         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]
+                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]
+                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]
 
           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>;
                 };
             };
+
+            /* GPIO0 set as multifunction GPIO0 */
+            gpio-pins {
+                pins {
+                    pinmux = <PINMUX_GPIO0__FUNC_GPIO0>;
+                };
+            };
+
+            /* GPIO52 set as multifunction SDA0 */
+            i2c0-pins {
+                pins {
+                  pinmux = <PINMUX_GPIO52__FUNC_SDA0>;
+                };
+            };
         };
 
         mmc0 {
diff --git a/Documentation/devicetree/bindings/pinctrl/mediatek,pinctrl-mt6795.yaml b/Documentation/devicetree/bindings/pinctrl/mediatek,pinctrl-mt6795.yaml
deleted file mode 100644
index a3a3f7fb9605..000000000000
--- a/Documentation/devicetree/bindings/pinctrl/mediatek,pinctrl-mt6795.yaml
+++ /dev/null
@@ -1,227 +0,0 @@ 
-# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
-%YAML 1.2
----
-$id: http://devicetree.org/schemas/pinctrl/mediatek,pinctrl-mt6795.yaml#
-$schema: http://devicetree.org/meta-schemas/core.yaml#
-
-title: Mediatek MT6795 Pin Controller
-
-maintainers:
-  - AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
-  - Sean Wang <sean.wang@kernel.org>
-
-description: |
-  The Mediatek's Pin controller is used to control SoC pins.
-
-properties:
-  compatible:
-    const: mediatek,mt6795-pinctrl
-
-  gpio-controller: true
-
-  '#gpio-cells':
-    description: |
-      Number of cells in GPIO specifier. Since the generic GPIO binding is used,
-      the amount of cells must be specified as 2. See the below
-      mentioned gpio binding representation for description of particular cells.
-    const: 2
-
-  gpio-ranges:
-    description: GPIO valid number range.
-    maxItems: 1
-
-  reg:
-    description:
-      Physical address base for gpio base and eint registers.
-    minItems: 2
-
-  reg-names:
-    items:
-      - const: base
-      - const: eint
-
-  interrupt-controller: true
-
-  '#interrupt-cells':
-    const: 2
-
-  interrupts:
-    description: The interrupt outputs to sysirq.
-    minItems: 1
-    items:
-      - description: EINT interrupt
-      - description: EINT event_b interrupt
-
-# PIN CONFIGURATION NODES
-patternProperties:
-  '-pins$':
-    type: object
-    additionalProperties: false
-    patternProperties:
-      '^pins':
-        type: object
-        additionalProperties: false
-        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.
-          An example of using macro:
-          pincontroller {
-            /* GPIO0 set as multifunction GPIO0 */
-            gpio-pins {
-              pins {
-                pinmux = <PINMUX_GPIO0__FUNC_GPIO0>;
-              }
-            };
-            /* GPIO45 set as multifunction SDA0 */
-            i2c0-pins {
-              pins {
-                pinmux = <PINMUX_GPIO45__FUNC_SDA0>;
-              }
-            };
-          };
-        $ref: "pinmux-node.yaml"
-
-        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 dt-bindings/pinctrl/<soc>-pinfunc.h
-              directly.
-
-          drive-strength:
-            enum: [2, 4, 6, 8, 10, 12, 14, 16]
-
-          bias-pull-down:
-            oneOf:
-              - type: boolean
-              - enum: [100, 101, 102, 103]
-                description: mt6795 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.
-
-          bias-pull-up:
-            oneOf:
-              - type: boolean
-              - enum: [100, 101, 102, 103]
-                description: mt6795 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-disable: true
-
-          output-high: true
-
-          output-low: true
-
-          input-enable: true
-
-          input-disable: true
-
-          input-schmitt-enable: true
-
-          input-schmitt-disable: true
-
-          mediatek,pull-up-adv:
-            description: |
-              Pull up setings for 2 pull resistors, R0 and R1. User can
-              configure those special pins. Valid arguments are described as below:
-              0: (R1, R0) = (0, 0) which means R1 disabled and R0 disabled.
-              1: (R1, R0) = (0, 1) which means R1 disabled and R0 enabled.
-              2: (R1, R0) = (1, 0) which means R1 enabled and R0 disabled.
-              3: (R1, R0) = (1, 1) which means R1 enabled and R0 enabled.
-            $ref: /schemas/types.yaml#/definitions/uint32
-            enum: [0, 1, 2, 3]
-
-          mediatek,pull-down-adv:
-            description: |
-              Pull down settings for 2 pull resistors, R0 and R1. User can
-              configure those special pins. Valid arguments are described as below:
-              0: (R1, R0) = (0, 0) which means R1 disabled and R0 disabled.
-              1: (R1, R0) = (0, 1) which means R1 disabled and R0 enabled.
-              2: (R1, R0) = (1, 0) which means R1 enabled and R0 disabled.
-              3: (R1, R0) = (1, 1) which means R1 enabled and R0 enabled.
-            $ref: /schemas/types.yaml#/definitions/uint32
-            enum: [0, 1, 2, 3]
-
-        required:
-          - pinmux
-
-allOf:
-  - $ref: "pinctrl.yaml#"
-
-required:
-  - compatible
-  - reg
-  - reg-names
-  - interrupts
-  - interrupt-controller
-  - '#interrupt-cells'
-  - gpio-controller
-  - '#gpio-cells'
-  - gpio-ranges
-
-additionalProperties: false
-
-examples:
-  - |
-    #include <dt-bindings/interrupt-controller/arm-gic.h>
-    #include <dt-bindings/interrupt-controller/irq.h>
-    #include <dt-bindings/pinctrl/mt6795-pinfunc.h>
-
-    soc {
-        #address-cells = <2>;
-        #size-cells = <2>;
-
-        pio: pinctrl@10005000 {
-            compatible = "mediatek,mt6795-pinctrl";
-            reg = <0 0x10005000 0 0x1000>, <0 0x1000b000 0 0x1000>;
-            reg-names = "base", "eint";
-            gpio-controller;
-            #gpio-cells = <2>;
-            gpio-ranges = <&pio 0 0 196>;
-            interrupt-controller;
-            interrupts = <GIC_SPI 153 IRQ_TYPE_LEVEL_HIGH>;
-            #interrupt-cells = <2>;
-
-            i2c0-pins {
-                pins-sda-scl {
-                    pinmux = <PINMUX_GPIO45__FUNC_SDA0>,
-                             <PINMUX_GPIO46__FUNC_SCL0>;
-                };
-            };
-
-            mmc0-pins {
-                pins-cmd-dat {
-                    pinmux = <PINMUX_GPIO154__FUNC_MSDC0_DAT0>,
-                             <PINMUX_GPIO155__FUNC_MSDC0_DAT1>,
-                             <PINMUX_GPIO156__FUNC_MSDC0_DAT2>,
-                             <PINMUX_GPIO157__FUNC_MSDC0_DAT3>,
-                             <PINMUX_GPIO158__FUNC_MSDC0_DAT4>,
-                             <PINMUX_GPIO159__FUNC_MSDC0_DAT5>,
-                             <PINMUX_GPIO160__FUNC_MSDC0_DAT6>,
-                             <PINMUX_GPIO161__FUNC_MSDC0_DAT7>,
-                             <PINMUX_GPIO162__FUNC_MSDC0_CMD>;
-                    input-enable;
-                    bias-pull-up = <MTK_PUPD_SET_R1R0_01>;
-                };
-
-                pins-clk {
-                    pinmux = <PINMUX_GPIO163__FUNC_MSDC0_CLK>;
-                    bias-pull-down = <MTK_PUPD_SET_R1R0_10>;
-                };
-
-                pins-rst {
-                    pinmux = <PINMUX_GPIO165__FUNC_MSDC0_RSTB>;
-                    bias-pull-up = <MTK_PUPD_SET_R1R0_10>;
-                };
-            };
-        };
-    };