From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> To: Yassine Oudjana <yassine.oudjana@gmail.com> Cc: Linus Walleij <linus.walleij@linaro.org>, Rob Herring <robh+dt@kernel.org>, Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>, Matthias Brugger <matthias.bgg@gmail.com>, Sean Wang <sean.wang@kernel.org>, Andy Teng <andy.teng@mediatek.com>, AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>, Yassine Oudjana <y.oudjana@protonmail.com>, linux-mediatek@lists.infradead.org, linux-gpio@vger.kernel.org, devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v3 06/10] dt-bindings: pinctrl: mediatek,mt6779-pinctrl: Add MT6795 Date: Thu, 20 Oct 2022 08:21:43 -0400 [thread overview] Message-ID: <2cfed688-f401-e69e-1ff4-f775c6d90f64@linaro.org> (raw) In-Reply-To: <8WU1KR.065JU8WYUX9C3@gmail.com> 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
WARNING: multiple messages have this Message-ID (diff)
From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> To: Yassine Oudjana <yassine.oudjana@gmail.com> Cc: Linus Walleij <linus.walleij@linaro.org>, Rob Herring <robh+dt@kernel.org>, Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>, Matthias Brugger <matthias.bgg@gmail.com>, Sean Wang <sean.wang@kernel.org>, Andy Teng <andy.teng@mediatek.com>, AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>, Yassine Oudjana <y.oudjana@protonmail.com>, linux-mediatek@lists.infradead.org, linux-gpio@vger.kernel.org, devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v3 06/10] dt-bindings: pinctrl: mediatek,mt6779-pinctrl: Add MT6795 Date: Thu, 20 Oct 2022 08:21:43 -0400 [thread overview] Message-ID: <2cfed688-f401-e69e-1ff4-f775c6d90f64@linaro.org> (raw) In-Reply-To: <8WU1KR.065JU8WYUX9C3@gmail.com> 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2022-10-20 12:21 UTC|newest] Thread overview: 53+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-10-07 12:58 [PATCH v3 00/10] MediaTek pinctrl DT binding cleanup and MT6735 pinctrl support Yassine Oudjana 2022-10-07 12:58 ` Yassine Oudjana 2022-10-07 12:58 ` [PATCH v3 01/10] arm64: dts: mediatek: mt6779: Remove syscon compatible from pin controller Yassine Oudjana 2022-10-07 12:58 ` Yassine Oudjana 2022-10-07 12:58 ` [PATCH v3 02/10] dt-bindings: pinctrl: mediatek,mt6779-pinctrl: Improve description Yassine Oudjana 2022-10-07 12:58 ` Yassine Oudjana 2022-10-10 8:16 ` AngeloGioacchino Del Regno 2022-10-10 8:16 ` AngeloGioacchino Del Regno 2022-10-07 12:58 ` [PATCH v3 03/10] dt-bindings: pinctrl: mediatek,mt6779-pinctrl: Make gpio-ranges optional Yassine Oudjana 2022-10-07 12:58 ` Yassine Oudjana 2022-10-10 8:16 ` AngeloGioacchino Del Regno 2022-10-10 8:16 ` AngeloGioacchino Del Regno 2022-10-07 12:58 ` [PATCH v3 04/10] dt-bindings: pinctrl: mediatek,mt6779-pinctrl: Add MT6797 Yassine Oudjana 2022-10-07 12:58 ` Yassine Oudjana 2022-10-10 8:16 ` AngeloGioacchino Del Regno 2022-10-10 8:16 ` AngeloGioacchino Del Regno 2022-10-07 12:58 ` [PATCH v3 05/10] dt-bindings: pinctrl: mediatek,pinctrl-mt6795: Fix interrupt count Yassine Oudjana 2022-10-07 12:58 ` Yassine Oudjana 2022-10-10 8:16 ` AngeloGioacchino Del Regno 2022-10-10 8:16 ` AngeloGioacchino Del Regno 2022-10-10 11:13 ` Krzysztof Kozlowski 2022-10-10 11:13 ` Krzysztof Kozlowski 2022-10-10 11:47 ` AngeloGioacchino Del Regno 2022-10-10 11:47 ` AngeloGioacchino Del Regno 2022-10-10 12:57 ` Rob Herring 2022-10-10 12:57 ` Rob Herring 2022-10-11 10:03 ` AngeloGioacchino Del Regno 2022-10-11 10:03 ` AngeloGioacchino Del Regno 2022-10-07 12:59 ` [PATCH v3 06/10] dt-bindings: pinctrl: mediatek,mt6779-pinctrl: Add MT6795 Yassine Oudjana 2022-10-07 12:59 ` Yassine Oudjana 2022-10-10 11:24 ` Krzysztof Kozlowski 2022-10-10 11:24 ` Krzysztof Kozlowski 2022-10-20 11:36 ` Yassine Oudjana 2022-10-20 11:36 ` Yassine Oudjana 2022-10-20 12:21 ` Krzysztof Kozlowski [this message] 2022-10-20 12:21 ` Krzysztof Kozlowski 2022-10-21 8:19 ` AngeloGioacchino Del Regno 2022-10-21 8:19 ` AngeloGioacchino Del Regno 2022-10-07 12:59 ` [PATCH v3 07/10] arm64: dts: mediatek: mt6797: Make pin configuration nodes follow DT bindings Yassine Oudjana 2022-10-07 12:59 ` Yassine Oudjana 2022-10-07 12:59 ` [PATCH v3 08/10] dt-bindings: pinctrl: mediatek,mt6779-pinctrl: Document MT6765 pin controller Yassine Oudjana 2022-10-07 12:59 ` Yassine Oudjana 2022-10-10 11:25 ` Krzysztof Kozlowski 2022-10-10 11:25 ` Krzysztof Kozlowski 2022-10-07 12:59 ` [PATCH v3 09/10] dt-bindings: pinctrl: mediatek,mt6779-pinctrl: Document MT6735 " Yassine Oudjana 2022-10-07 12:59 ` Yassine Oudjana 2022-10-10 11:25 ` Krzysztof Kozlowski 2022-10-10 11:25 ` Krzysztof Kozlowski 2022-10-07 12:59 ` [PATCH v3 10/10] pinctrl: mediatek: Add MT6735 pinctrl driver Yassine Oudjana 2022-10-10 8:16 ` AngeloGioacchino Del Regno 2022-10-10 8:16 ` AngeloGioacchino Del Regno 2022-10-17 9:13 ` [PATCH v3 00/10] MediaTek pinctrl DT binding cleanup and MT6735 pinctrl support Linus Walleij 2022-10-17 9:13 ` Linus Walleij
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=2cfed688-f401-e69e-1ff4-f775c6d90f64@linaro.org \ --to=krzysztof.kozlowski@linaro.org \ --cc=andy.teng@mediatek.com \ --cc=angelogioacchino.delregno@collabora.com \ --cc=devicetree@vger.kernel.org \ --cc=krzysztof.kozlowski+dt@linaro.org \ --cc=linus.walleij@linaro.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-gpio@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-mediatek@lists.infradead.org \ --cc=matthias.bgg@gmail.com \ --cc=robh+dt@kernel.org \ --cc=sean.wang@kernel.org \ --cc=y.oudjana@protonmail.com \ --cc=yassine.oudjana@gmail.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.