by Rob Herring

[permalink] [raw]

Subject: Re: [PATCH v5 1/7] dt-bindings: pinctrl: mediatek,mt6779-pinctrl: Pull pinctrl node changes from MT6795 document

On Fri, Nov 18, 2022 at 02:30:22PM +0300, Yassine Oudjana wrote:
> From: Yassine Oudjana <[email protected]>
>
> 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 <[email protected]>
> ---
> .../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 a2141eb0854e..d6231d11e949 100644
> --- a/Documentation/devicetree/bindings/pinctrl/mediatek,mt6779-pinctrl.yaml
> +++ b/Documentation/devicetree/bindings/pinctrl/mediatek,mt6779-pinctrl.yaml
> @@ -111,19 +111,21 @@ 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"
> + allOf:
> + - $ref: pinmux-node.yaml
> + - $ref: pincfg-node.yaml
>
> properties:
> pinmux:
> @@ -134,9 +136,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]

'bias-pull-down' is defined to be in Ohms. This doesn't look like it's
Ohms.

> + 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
>
> @@ -218,8 +236,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>,
> @@ -232,11 +250,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>;
> };
> --
> 2.38.1
>
>

2022-12-01 12:45:09

by Yassine Oudjana

[permalink] [raw]

Subject: Re: [PATCH v5 1/7] dt-bindings: pinctrl: mediatek,mt6779-pinctrl: Pull pinctrl node changes from MT6795 document

On Wednesday, November 30th, 2022 at 6:20 PM, Rob Herring <[email protected]> wrote:

> On Fri, Nov 18, 2022 at 02:30:22PM +0300, Yassine Oudjana wrote:
>
> > From: Yassine Oudjana [email protected]
> >
> > 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 [email protected]
> > ---
> > .../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 a2141eb0854e..d6231d11e949 100644
> > --- a/Documentation/devicetree/bindings/pinctrl/mediatek,mt6779-pinctrl.yaml
> > +++ b/Documentation/devicetree/bindings/pinctrl/mediatek,mt6779-pinctrl.yaml
> > @@ -111,19 +111,21 @@ 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"
> > + allOf:
> > + - $ref: pinmux-node.yaml
> > + - $ref: pincfg-node.yaml
> >
> > properties:
> > pinmux:
> > @@ -134,9 +136,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]
>
>
> 'bias-pull-down' is defined to be in Ohms. This doesn't look like it's
> Ohms.

That's right, these numbers appear to correspond to MTK_PUPD_SET_R1R0_*
values defined in include/dt-bindings/pinctrl/mt65xx.h, and work similar
to mediatek,pull-down-adv as defined in mediatek,mt8183-pinctrl.yaml.

Now I think the easiest thing to do in order to sort this out would be
be to stop supporting bias-pull-[up/down] properties and replace them
with mediatek,pull-[up/down]-adv, but I guess that would break old DT.
Changing the supported values to represent ohms and modifying drivers
to accommodate for that would be quite tedious since every pin group
on every SoC has different supported pull resistances, and it would
still break compatibility with old DT anyway.

Does anyone have a better idea on this? Or perhaps we could leave fixing
this for another time since issues with these bindings seem to never end
and this patch series just keeps growing.