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
