| Message ID | 20241011120520.140318-2-y.oudjana@protonmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | MediaTek pinctrl DT binding cleanup and MT6735 pinctrl support | expand |
On 11/10/2024 7:56 pm, Rob Herring wrote: > On Fri, Oct 11, 2024 at 03:03:46PM +0300, Yassine Oudjana wrote: >> From: Yassine Oudjana <y.oudjana@protonmail.com> >> >> mediatek,pinctrl-mt6795.yaml has different node name patterns which match >> bindings of other MediaTek pin controllers, ref for pinmux-node.yaml which >> has a description of the pinmux property, as well as some additional >> descriptions for some pin configuration properties. Pull those changes >> into mediatek,mt6779-pinctrl.yaml and adjust the example DTS to match in >> preparation to combine the MT6795 document into it. >> >> Signed-off-by: Yassine Oudjana <y.oudjana@protonmail.com> >> --- >> .../pinctrl/mediatek,mt6779-pinctrl.yaml | 38 ++++++++++++++----- >> 1 file changed, 28 insertions(+), 10 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/pinctrl/mediatek,mt6779-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/mediatek,mt6779-pinctrl.yaml >> index 3bbc00df5548d..352a88d7b135e 100644 >> --- a/Documentation/devicetree/bindings/pinctrl/mediatek,mt6779-pinctrl.yaml >> +++ b/Documentation/devicetree/bindings/pinctrl/mediatek,mt6779-pinctrl.yaml >> @@ -111,12 +111,12 @@ allOf: >> - "#interrupt-cells" >> >> patternProperties: >> - '-[0-9]*$': >> + '-pins$': > > Worst case, this could be an ABI break. Best case, it's churn for > mt6779. Is it worth unifying? It's better than keeping different patterns, isn't it? We wouldn't have ended up here if they were made as one in the beginning as it was ought to be considering how similar the hardware is. It's easier to change now since nothing is using it yet.
Il 11/10/24 14:03, Yassine Oudjana ha scritto: > From: Yassine Oudjana <y.oudjana@protonmail.com> > > mediatek,pinctrl-mt6795.yaml has different node name patterns which match > bindings of other MediaTek pin controllers, ref for pinmux-node.yaml which > has a description of the pinmux property, as well as some additional > descriptions for some pin configuration properties. Pull those changes > into mediatek,mt6779-pinctrl.yaml and adjust the example DTS to match in > preparation to combine the MT6795 document into it. > > Signed-off-by: Yassine Oudjana <y.oudjana@protonmail.com> For the sake of consistency and reducing duplication, I agree. Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Il 11/10/24 18:56, Rob Herring ha scritto: > On Fri, Oct 11, 2024 at 03:03:46PM +0300, Yassine Oudjana wrote: >> From: Yassine Oudjana <y.oudjana@protonmail.com> >> >> mediatek,pinctrl-mt6795.yaml has different node name patterns which match >> bindings of other MediaTek pin controllers, ref for pinmux-node.yaml which >> has a description of the pinmux property, as well as some additional >> descriptions for some pin configuration properties. Pull those changes >> into mediatek,mt6779-pinctrl.yaml and adjust the example DTS to match in >> preparation to combine the MT6795 document into it. >> >> Signed-off-by: Yassine Oudjana <y.oudjana@protonmail.com> >> --- >> .../pinctrl/mediatek,mt6779-pinctrl.yaml | 38 ++++++++++++++----- >> 1 file changed, 28 insertions(+), 10 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/pinctrl/mediatek,mt6779-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/mediatek,mt6779-pinctrl.yaml >> index 3bbc00df5548d..352a88d7b135e 100644 >> --- a/Documentation/devicetree/bindings/pinctrl/mediatek,mt6779-pinctrl.yaml >> +++ b/Documentation/devicetree/bindings/pinctrl/mediatek,mt6779-pinctrl.yaml >> @@ -111,12 +111,12 @@ allOf: >> - "#interrupt-cells" >> >> patternProperties: >> - '-[0-9]*$': >> + '-pins$': > > Worst case, this could be an ABI break. Best case, it's churn for > mt6779. Is it worth unifying? > All those MediaTek pinctrl bindings are mostly the same, where only the pin definitions in the binding header does actually change. I think that it's worth unifying them, not only to get rid of the duplication but mostly for consistency between all of those subnode names which are wildly differing for no real reason... and consistency is a long time issue with MediaTek bindings/dts in general (which is way way way better now, but still)... Besides - just for context and nothing else: the driver doesn't care about the names of the subnodes, anyway... so while this is technically an ABI break it's not really creating any functionality issue, and then, actually, Yassine is also modifying the devicetrees to comply with his consistency changes, so, in my own perspective, it's still acceptable. If you think otherwise, though, I'm still fine with your POV. Cheers, Angelo
On Sat, Oct 12, 2024 at 3:09 AM Yassine Oudjana <yassine.oudjana@gmail.com> wrote: > > > On 11/10/2024 7:56 pm, Rob Herring wrote: > > On Fri, Oct 11, 2024 at 03:03:46PM +0300, Yassine Oudjana wrote: > >> From: Yassine Oudjana <y.oudjana@protonmail.com> > >> > >> mediatek,pinctrl-mt6795.yaml has different node name patterns which match > >> bindings of other MediaTek pin controllers, ref for pinmux-node.yaml which > >> has a description of the pinmux property, as well as some additional > >> descriptions for some pin configuration properties. Pull those changes > >> into mediatek,mt6779-pinctrl.yaml and adjust the example DTS to match in > >> preparation to combine the MT6795 document into it. > >> > >> Signed-off-by: Yassine Oudjana <y.oudjana@protonmail.com> > >> --- > >> .../pinctrl/mediatek,mt6779-pinctrl.yaml | 38 ++++++++++++++----- > >> 1 file changed, 28 insertions(+), 10 deletions(-) > >> > >> diff --git a/Documentation/devicetree/bindings/pinctrl/mediatek,mt6779-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/mediatek,mt6779-pinctrl.yaml > >> index 3bbc00df5548d..352a88d7b135e 100644 > >> --- a/Documentation/devicetree/bindings/pinctrl/mediatek,mt6779-pinctrl.yaml > >> +++ b/Documentation/devicetree/bindings/pinctrl/mediatek,mt6779-pinctrl.yaml > >> @@ -111,12 +111,12 @@ allOf: > >> - "#interrupt-cells" > >> > >> patternProperties: > >> - '-[0-9]*$': > >> + '-pins$': > > > > Worst case, this could be an ABI break. Best case, it's churn for > > mt6779. Is it worth unifying? > > It's better than keeping different patterns, isn't it? We wouldn't have > ended up here if they were made as one in the beginning as it was ought > to be considering how similar the hardware is. It's easier to change now > since nothing is using it yet. I can only assume there are users unless you tell me otherwise (in the commit msg). Rob
On Mon, Oct 14, 2024 at 3:27 AM AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> wrote: > > Il 11/10/24 18:56, Rob Herring ha scritto: > > On Fri, Oct 11, 2024 at 03:03:46PM +0300, Yassine Oudjana wrote: > >> From: Yassine Oudjana <y.oudjana@protonmail.com> > >> > >> mediatek,pinctrl-mt6795.yaml has different node name patterns which match > >> bindings of other MediaTek pin controllers, ref for pinmux-node.yaml which > >> has a description of the pinmux property, as well as some additional > >> descriptions for some pin configuration properties. Pull those changes > >> into mediatek,mt6779-pinctrl.yaml and adjust the example DTS to match in > >> preparation to combine the MT6795 document into it. > >> > >> Signed-off-by: Yassine Oudjana <y.oudjana@protonmail.com> > >> --- > >> .../pinctrl/mediatek,mt6779-pinctrl.yaml | 38 ++++++++++++++----- > >> 1 file changed, 28 insertions(+), 10 deletions(-) > >> > >> diff --git a/Documentation/devicetree/bindings/pinctrl/mediatek,mt6779-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/mediatek,mt6779-pinctrl.yaml > >> index 3bbc00df5548d..352a88d7b135e 100644 > >> --- a/Documentation/devicetree/bindings/pinctrl/mediatek,mt6779-pinctrl.yaml > >> +++ b/Documentation/devicetree/bindings/pinctrl/mediatek,mt6779-pinctrl.yaml > >> @@ -111,12 +111,12 @@ allOf: > >> - "#interrupt-cells" > >> > >> patternProperties: > >> - '-[0-9]*$': > >> + '-pins$': > > > > Worst case, this could be an ABI break. Best case, it's churn for > > mt6779. Is it worth unifying? > > > All those MediaTek pinctrl bindings are mostly the same, where only the pin > definitions in the binding header does actually change. > > I think that it's worth unifying them, not only to get rid of the duplication > but mostly for consistency between all of those subnode names which are wildly > differing for no real reason... and consistency is a long time issue with > MediaTek bindings/dts in general (which is way way way better now, but still)... > > Besides - just for context and nothing else: the driver doesn't care about > the names of the subnodes, anyway... so while this is technically an ABI break > it's not really creating any functionality issue, and then, actually, Yassine > is also modifying the devicetrees to comply with his consistency changes, so, > in my own perspective, it's still acceptable. Wait, I thought there were no users? We generally only consider node names ABI when/if something or someone cares. Most of the time it doesn't matter. For the pinctrl nodes, it's really just a question of churn renaming a lot of nodes. Ultimately, it's up to you. I only care that the implications of the changes are clear in the commit msg. Rob
On Mon, Oct 14, 2024 at 2:02 PM Rob Herring <robh@kernel.org> wrote: > > On Mon, Oct 14, 2024 at 3:27 AM AngeloGioacchino Del Regno > <angelogioacchino.delregno@collabora.com> wrote: > > > > Il 11/10/24 18:56, Rob Herring ha scritto: > > > On Fri, Oct 11, 2024 at 03:03:46PM +0300, Yassine Oudjana wrote: > > >> From: Yassine Oudjana <y.oudjana@protonmail.com> > > >> > > >> mediatek,pinctrl-mt6795.yaml has different node name patterns which match > > >> bindings of other MediaTek pin controllers, ref for pinmux-node.yaml which > > >> has a description of the pinmux property, as well as some additional > > >> descriptions for some pin configuration properties. Pull those changes > > >> into mediatek,mt6779-pinctrl.yaml and adjust the example DTS to match in > > >> preparation to combine the MT6795 document into it. > > >> > > >> Signed-off-by: Yassine Oudjana <y.oudjana@protonmail.com> > > >> --- > > >> .../pinctrl/mediatek,mt6779-pinctrl.yaml | 38 ++++++++++++++----- > > >> 1 file changed, 28 insertions(+), 10 deletions(-) > > >> > > >> diff --git a/Documentation/devicetree/bindings/pinctrl/mediatek,mt6779-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/mediatek,mt6779-pinctrl.yaml > > >> index 3bbc00df5548d..352a88d7b135e 100644 > > >> --- a/Documentation/devicetree/bindings/pinctrl/mediatek,mt6779-pinctrl.yaml > > >> +++ b/Documentation/devicetree/bindings/pinctrl/mediatek,mt6779-pinctrl.yaml > > >> @@ -111,12 +111,12 @@ allOf: > > >> - "#interrupt-cells" > > >> > > >> patternProperties: > > >> - '-[0-9]*$': > > >> + '-pins$': > > > > > > Worst case, this could be an ABI break. Best case, it's churn for > > > mt6779. Is it worth unifying? > > > > > All those MediaTek pinctrl bindings are mostly the same, where only the pin > > definitions in the binding header does actually change. > > > > I think that it's worth unifying them, not only to get rid of the duplication > > but mostly for consistency between all of those subnode names which are wildly > > differing for no real reason... and consistency is a long time issue with > > MediaTek bindings/dts in general (which is way way way better now, but still)... > > > > Besides - just for context and nothing else: the driver doesn't care about > > the names of the subnodes, anyway... so while this is technically an ABI break > > it's not really creating any functionality issue, and then, actually, Yassine > > is also modifying the devicetrees to comply with his consistency changes, so, > > in my own perspective, it's still acceptable. > > Wait, I thought there were no users? > > We generally only consider node names ABI when/if something or someone > cares. Most of the time it doesn't matter. For the pinctrl nodes, it's > really just a question of churn renaming a lot of nodes. > > Ultimately, it's up to you. I only care that the implications of the > changes are clear in the commit msg. FWIW, these are the top occuring warnings on arm64 mediatek pinctrl nodes: 27 mediatek: pinctrl@10005000: panel-pins-default: 'panel-reset' does not match any of the regexes: '^pins', 'pinctrl-[0-9]+' 27 mediatek: pinctrl@10005000: mmc1-pins-uhs:pins-cmd-dat:mediatek,pull-up-adv: 10 is not one of [0, 1, 2, 3] 27 mediatek: pinctrl@10005000: mmc1-pins-uhs:pins-clk:mediatek,pull-down-adv: 10 is not one of [0, 1, 2, 3] 27 mediatek: pinctrl@10005000: mmc1-pins-default:pins-cmd-dat:mediatek,pull-up-adv: 10 is not one of [0, 1, 2, 3] 27 mediatek: pinctrl@10005000: mmc1-pins-default:pins-clk:mediatek,pull-down-adv: 10 is not one of [0, 1, 2, 3] 27 mediatek: pinctrl@10005000: mmc0-pins-uhs:pins-ds:mediatek,pull-down-adv: 10 is not one of [0, 1, 2, 3] 27 mediatek: pinctrl@10005000: mmc0-pins-uhs:pins-clk:mediatek,pull-down-adv: 10 is not one of [0, 1, 2, 3] 27 mediatek: pinctrl@10005000: mmc0-pins-default:pins-clk:mediatek,pull-down-adv: 10 is not one of [0, 1, 2, 3] 17 mediatek: pinctrl@10005000: volume-button-pins: 'voldn-btn-odl', 'volup-btn-odl' do not match any of the regexes: '^pins', 'pinctrl-[0-9]+' 17 mediatek: pinctrl@10005000: trackpad-pins: 'trackpad-int' does not match any of the regexes: '^pins', 'pinctrl-[0-9]+' 17 mediatek: pinctrl@10005000: touchscreen-pins: 'touch-int-odl', 'touch-rst-l' do not match any of the regexes: '^pins', 'pinctrl-[0-9]+' 17 mediatek: pinctrl@10005000: pp3300-panel-pins: 'panel-3v3-enable' does not match any of the regexes: '^pins', 'pinctrl-[0-9]+' Not sure what SoC because I strip the dtb names to deduplicate the warnings which get amplified by number of boards for an SoC. The first number is the number of times the warning occurs. Rob
diff --git a/Documentation/devicetree/bindings/pinctrl/mediatek,mt6779-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/mediatek,mt6779-pinctrl.yaml index 3bbc00df5548d..352a88d7b135e 100644 --- a/Documentation/devicetree/bindings/pinctrl/mediatek,mt6779-pinctrl.yaml +++ b/Documentation/devicetree/bindings/pinctrl/mediatek,mt6779-pinctrl.yaml @@ -111,12 +111,12 @@ 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 @@ -124,7 +124,9 @@ patternProperties: 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 + allOf: + - $ref: pinmux-node.yaml + - $ref: pincfg-node.yaml properties: pinmux: @@ -135,9 +137,25 @@ patternProperties: 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 @@ -221,8 +239,8 @@ examples: #interrupt-cells = <2>; interrupts = <GIC_SPI 204 IRQ_TYPE_LEVEL_HIGH>; - mmc0_pins_default: mmc0-0 { - cmd-dat-pins { + mmc0_pins_default: mmc0-pins { + pins-cmd-dat { pinmux = <PINMUX_GPIO168__FUNC_MSDC0_DAT0>, <PINMUX_GPIO172__FUNC_MSDC0_DAT1>, <PINMUX_GPIO169__FUNC_MSDC0_DAT2>, @@ -235,11 +253,11 @@ 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>; };