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

[Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx>]

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

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