[v3,4/7] dt-bindings: pinctrl: add bindings for Mediatek MT8365 SoC

  Add devicetree bindings for Mediatek MT8365 pinctrl driver.

Signed-off-by: Bernhard Rosenkränzer <bero@baylibre.com>
---
 .../pinctrl/mediatek,mt8365-pinctrl.yaml      | 202 ++++++++++++++++++
 1 file changed, 202 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pinctrl/mediatek,mt8365-pinctrl.yaml
  

On 17/11/2022 22:03, Bernhard Rosenkränzer wrote:
> Add devicetree bindings for Mediatek MT8365 pinctrl driver.
> 
> Signed-off-by: Bernhard Rosenkränzer <bero@baylibre.com>

Thank you for your patch. There is something to discuss/improve.

> +
> +  pins-are-numbered:
> +    $ref: /schemas/types.yaml#/definitions/flag
> +    description: |
> +      Specify the subnodes are using numbered pinmux to specify pins.

Why would you name pins differently per board? And why this different
naming of the same pins is a property of hardware?

This looks like something to drop.

> +
> +  gpio-controller: true
> +
> +  "#gpio-cells":
> +    const: 2
> +    description: |
> +      Number of cells in GPIO specifier. Since the generic GPIO
> +      binding is used, the amount of cells must be specified as 2. See the below
> +      mentioned gpio binding representation for description of particular cells.
> +
> +  interrupt-controller: true
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  "#interrupt-cells":
> +    const: 2
> +
> +required:
> +  - compatible
> +  - reg
> +  - gpio-controller
> +  - "#gpio-cells"
> +
> +allOf:
> +  - $ref: pinctrl.yaml#
> +
> +patternProperties:

patternProperties go before "required:" block.

> +  '-pins$':
> +    type: object
> +    additionalProperties: false
> +    patternProperties:
> +      'pins':

How about anchoring it to either prefix or suffix? ^pins or pins$.

> +        type: object
> +        additionalProperties: false
> +        description: |
> +          A pinctrl node should contain at least one subnode 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"

Drop quotes.

> +
> +        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 <soc>-pinfunc.h directly.
> +
> +          bias-disable: true
> +
> +          bias-pull-up:
> +            description: |
> +              Besides generic pinconfig options, it can be used as the pull up
> +              settings for 2 pull resistors, R0 and R1. User can configure those
> +              special pins.
> +
> +          bias-pull-down: true
> +
> +          input-enable: true
> +
> +          input-disable: true
> +
> +          output-low: true
> +
> +          output-high: true
> +
> +          input-schmitt-enable: true
> +
> +          input-schmitt-disable: true
> +
> +          mediatek,drive-strength-adv:
> +            description: |
> +              Describe the specific driving setup property.
> +              For I2C pins, the existing generic driving setup can only support
> +              2/4/6/8/10/12/14/16mA driving. But in specific driving setup, they
> +              can support 0.125/0.25/0.5/1mA adjustment. If we enable specific
> +              driving setup, the existing generic setup will be disabled.
> +              The specific driving setup is controlled by E1E0EN.
> +              When E1=0/E0=0, the strength is 0.125mA.
> +              When E1=0/E0=1, the strength is 0.25mA.
> +              When E1=1/E0=0, the strength is 0.5mA.
> +              When E1=1/E0=1, the strength is 1mA.
> +              EN is used to enable or disable the specific driving setup.
> +              Valid arguments are described as below:
> +              0: (E1, E0, EN) = (0, 0, 0)
> +              1: (E1, E0, EN) = (0, 0, 1)
> +              2: (E1, E0, EN) = (0, 1, 0)
> +              3: (E1, E0, EN) = (0, 1, 1)
> +              4: (E1, E0, EN) = (1, 0, 0)
> +              5: (E1, E0, EN) = (1, 0, 1)
> +              6: (E1, E0, EN) = (1, 1, 0)
> +              7: (E1, E0, EN) = (1, 1, 1)
> +              So the valid arguments are from 0 to 7.
> +            $ref: /schemas/types.yaml#/definitions/uint32
> +            enum: [0, 1, 2, 3, 4, 5, 6, 7]
> +
> +          mediatek,pull-up-adv:
> +            description: |
> +              Pull up setings for 2 pull resistors, R0 and R1. User can
> +              configure those special pins. Valid arguments are described as below:
> +              0: (R1, R0) = (0, 0) which means R1 disabled and R0 disabled.
> +              1: (R1, R0) = (0, 1) which means R1 disabled and R0 enabled.
> +              2: (R1, R0) = (1, 0) which means R1 enabled and R0 disabled.
> +              3: (R1, R0) = (1, 1) which means R1 enabled and R0 enabled.
> +            $ref: /schemas/types.yaml#/definitions/uint32
> +            enum: [0, 1, 2, 3]
> +
> +          mediatek,pull-down-adv:
> +            description: |
> +              Pull down settings for 2 pull resistors, R0 and R1. User can
> +              configure those special pins. Valid arguments are described as below:
> +              0: (R1, R0) = (0, 0) which means R1 disabled and R0 disabled.
> +              1: (R1, R0) = (0, 1) which means R1 disabled and R0 enabled.
> +              2: (R1, R0) = (1, 0) which means R1 enabled and R0 disabled.
> +              3: (R1, R0) = (1, 1) which means R1 enabled and R0 enabled.
> +            $ref: /schemas/types.yaml#/definitions/uint32
> +            enum: [0, 1, 2, 3]
> +
> +          mediatek,tdsel:
> +            description: |
> +              An integer describing the steps for output level shifter duty
> +              cycle when asserted (high pulse width adjustment). Valid arguments
> +              are from 0 to 15.
> +            $ref: /schemas/types.yaml#/definitions/uint32
> +
> +          mediatek,rdsel:
> +            description: |
> +              An integer describing the steps for input level shifter duty cycle
> +              when asserted (high pulse width adjustment). Valid arguments are
> +              from 0 to 63.
> +            $ref: /schemas/types.yaml#/definitions/uint32
> +
> +        required:
> +          - pinmux
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    #include <dt-bindings/pinctrl/mt8365-pinfunc.h>
> +    soc {
> +        #address-cells = <2>;
> +        #size-cells = <2>;
> +
> +        pio: pinctrl@1000b000 {
> +          compatible = "mediatek,mt8365-pinctrl";
> +          reg = <0 0x1000b000 0 0x1000>;
> +          mediatek,pctl-regmap = <&syscfg_pctl>;
> +          pins-are-numbered;
> +          gpio-controller;
> +          #gpio-cells = <2>;
> +          interrupt-controller;
> +          #interrupt-cells = <2>;
> +          interrupts = <GIC_SPI 115 IRQ_TYPE_LEVEL_HIGH>;

Blank line

> +          pio-pins {
> +            pins {
> +              pinmux = <MT8365_PIN_59_SDA1__FUNC_SDA1_0>, <MT8365_PIN_60_SCL1__FUNC_SCL1_0>;
> +              mediatek,pull-up-adv = <3>;
> +              mediatek,drive-strength-adv = <00>;
> +              bias-pull-up;
> +            };
> +          };
> +        };
> +    };

Best regards,
Krzysztof
  

Hi Krzysztof,

Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> writes:

> On 17/11/2022 22:03, Bernhard Rosenkränzer wrote:
>> Add devicetree bindings for Mediatek MT8365 pinctrl driver.
>> 
>> Signed-off-by: Bernhard Rosenkränzer <bero@baylibre.com>
>
> Thank you for your patch. There is something to discuss/improve.
>
>> +
>> +  pins-are-numbered:
>> +    $ref: /schemas/types.yaml#/definitions/flag
>> +    description: |
>> +      Specify the subnodes are using numbered pinmux to specify pins.
>
> Why would you name pins differently per board? And why this different
> naming of the same pins is a property of hardware?
>
> This looks like something to drop.

Yeah, having this as a flag kind of implies that this could be present
for some boards but not others.  But in practice, the driver requires it
to be present or just fails[1].  What's the right way to describe that?
We're just trying to add a binding that reflects the existing driver.

We also noticed that there's another documented binding with this
same flag[2] where similiarily, the driver simply requires it to be
present[2].

So is the way this flag is documented in the stm32 binding OK for the
mediatek one also?  If not, what would you suggest?

Thanks for the review,

Kevin

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pinctrl/mediatek/pinctrl-mtk-common.c#n1053
[2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/pinctrl/st,stm32-pinctrl.yaml#n37
[3] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pinctrl/stm32/pinctrl-stm32.c#n1499
  

On 18/11/2022 20:52, Kevin Hilman wrote:
> Hi Krzysztof,
> 
> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> writes:
> 
>> On 17/11/2022 22:03, Bernhard Rosenkränzer wrote:
>>> Add devicetree bindings for Mediatek MT8365 pinctrl driver.
>>>
>>> Signed-off-by: Bernhard Rosenkränzer <bero@baylibre.com>
>>
>> Thank you for your patch. There is something to discuss/improve.
>>
>>> +
>>> +  pins-are-numbered:
>>> +    $ref: /schemas/types.yaml#/definitions/flag
>>> +    description: |
>>> +      Specify the subnodes are using numbered pinmux to specify pins.
>>
>> Why would you name pins differently per board? And why this different
>> naming of the same pins is a property of hardware?
>>
>> This looks like something to drop.
> 
> Yeah, having this as a flag kind of implies that this could be present
> for some boards but not others.  But in practice, the driver requires it
> to be present or just fails[1].  What's the right way to describe that?
> We're just trying to add a binding that reflects the existing driver.

Uh, what an interesting property. What's the point of it then? Why
failing to probe on a missing property which does nothing else?

The solution is also to drop that property from the driver.

> We also noticed that there's another documented binding with this
> same flag[2] where similiarily, the driver simply requires it to be
> present[2].
> 
> So is the way this flag is documented in the stm32 binding OK for the
> mediatek one also?  If not, what would you suggest?

I would like to understand why do we need this property and what is
described by it. Because if it's purpose is only to fail or not fail
driver probe, then we should just drop it everywhere.


> 

Best regards,
Krzysztof
  

Hi Krzysztof,

On Sun, Nov 20, 2022 at 11:40 AM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
> >>> +  pins-are-numbered:
> >
> > Yeah, having this as a flag kind of implies that this could be present
> > for some boards but not others.  But in practice, the driver requires it
> > to be present or just fails[1].  What's the right way to describe that?
> > We're just trying to add a binding that reflects the existing driver.
>
> Uh, what an interesting property. What's the point of it then? Why
> failing to probe on a missing property which does nothing else?

I couldn't find any other use of it in the kernel, and also checked
u-boot (where the property also appears in some devicetree files, but
isn't used anywhere).
Both the MTK and STM drivers use it just to refuse it if it isn't there.

Unfortunately "git blame" only shows pins-are-numbered being added as
part of a larger commit ("add the driver"), so there's no "add check
for pins-are-numbered because xyz" commit message.

I can think of 3 possible explanations, but none of them are good:
1. It's something that had a purpose at some point, but doesn't
anymore (but that would likely leave some trace in "git blame"...)
2. It's something that was added in preparation for another patch (but
I can't find any queued/suggested patches that make use of it)
3. It's for the sake of userland -- check if pins-are-numbered is set
(which will be true for MTK and STM because these drivers enforce it,
false for anything else because the schemas don't mention it) and then
do different things. But this seems unlikely as well, the usual
suspects (libgpiod and friends) don't do any such lookup, and there
are ways to look up pins without that property - and I'd expect pins
are numbered on many controllers outside of MTK and STM, so looking up
that property would give a false response there.

> I would like to understand why do we need this property and what is
> described by it.

Same here...

> Because if it's purpose is only to fail or not fail
> driver probe, then we should just drop it everywhere.

Agreed (and I think more likely than not, that is the only purpose),
but probably a "add support for another board" patchset isn't the
right context for that.

I can prepare a "remove pins-are-numbered" patchset, but given it will
likely take time to track down someone who knows why this was added in
the first place, I don't think it should block the MT8365 patchset.

Best regards
bero
  

On Sun, Nov 20, 2022 at 3:38 PM Bernhard Rosenkränzer <bero@baylibre.com> wrote:
> I can prepare a "remove pins-are-numbered" patchset

... and I did.
https://lore.kernel.org/linux-devicetree/20221121015451.2471196-1-bero@baylibre.com/

Best regards
bero
  

On 21/11/2022 02:58, Bernhard Rosenkränzer wrote:
> On Sun, Nov 20, 2022 at 3:38 PM Bernhard Rosenkränzer <bero@baylibre.com> wrote:
>> I can prepare a "remove pins-are-numbered" patchset
> 
> ... and I did.
> https://lore.kernel.org/linux-devicetree/20221121015451.2471196-1-bero@baylibre.com/

Awesome, thank you!

Best regards,
Krzysztof