by Krzysztof Kozlowski

[permalink] [raw]

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

On 20/10/2022 07:36, Yassine Oudjana wrote:
>
> On Mon, Oct 10 2022 at 07:24:59 -04:00:00, Krzysztof Kozlowski
> <[email protected]> wrote:
>> On 07/10/2022 08:59, Yassine Oudjana wrote:
>>> From: Yassine Oudjana <[email protected]>
>>>
>>> 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 <[email protected]>
>>> ---
>>> .../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 <[email protected]>
>>> + - AngeloGioacchino Del Regno
>>> <[email protected]>
>>> - Sean Wang <[email protected]>
>>>
>>> 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

2022-10-21 08:53:40

by AngeloGioacchino Del Regno

[permalink] [raw]

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

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
>> <[email protected]> wrote:
>>> On 07/10/2022 08:59, Yassine Oudjana wrote:
>>>> From: Yassine Oudjana <[email protected]>
>>>>
>>>> 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 <[email protected]>
>>>> ---
>>>> .../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 <[email protected]>
>>>> + - AngeloGioacchino Del Regno
>>>> <[email protected]>
>>>> - Sean Wang <[email protected]>
>>>>
>>>> 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