Re: [PATCH 1/4] dt-bindings: pinctrl: mobileye,eyeq5-pinctrl: add bindings

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 18/12/2023 18:19, Théo Lebrun wrote:
> Add dt-schema type bindings for the Mobileye EyeQ5 pin controller.
> 
> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
> ---
>  .../bindings/pinctrl/mobileye,eyeq5-pinctrl.yaml   | 125 +++++++++++++++++++++
>  MAINTAINERS                                        |   1 +
>  2 files changed, 126 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/pinctrl/mobileye,eyeq5-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/mobileye,eyeq5-pinctrl.yaml
> new file mode 100644
> index 000000000000..5faddebe2413
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pinctrl/mobileye,eyeq5-pinctrl.yaml
> @@ -0,0 +1,125 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pinctrl/mobileye,eyeq5-pinctrl.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Mobileye EyeQ5 pinctrl (pinmux & pinconf) controller

pinctrl means pin controller, so you basically wrote:
pin controller pinmux and pin configuration controller

Just "pin controller"


> +
> +description:
> +  The EyeQ5 pin controller handles a pin bank. It is custom to this platform,

Can part of SoC be not custom to given platform? I mean... describe the
hardware, not write essay.

> +  its registers live in a shared region called OLB.
> +  There are two pin banks on the platform, each having a specific compatible.

Instead of repeating something obvious - visible from the binding -
explain why. Say something different than the binding is saying.


> +  Pins and groups are bijective.
> +
> +maintainers:
> +  - Grégory Clement <gregory.clement@bootlin.com>
> +  - Théo Lebrun <theo.lebrun@bootlin.com>
> +  - Vladimir Kondratiev <vladimir.kondratiev@mobileye.com>
> +
> +properties:
> +  $nodename:
> +    pattern: "^pinctrl([0-9]+)?$"
> +    description:
> +      We have no unique address, we rely on OLB; we therefore can't keep the
> +      standard pattern and cannot inherit from pinctrl.yaml.

No, instead fix pinctrl.yaml

> +
> +  compatible:
> +    enum:
> +      - mobileye,eyeq5-a-pinctrl
> +      - mobileye,eyeq5-b-pinctrl

Why two compatibles? Description provided no rationale for this.

> +
> +  "#pinctrl-cells":
> +    const: 1
> +
> +  mobileye,olb:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description:
> +      A phandle to the OLB syscon. This is a fallback to using the parent as
> +      syscon node.

So here is the explanation for missing unit address. If all registers,
as you claim in description, belong to OLB, then this should be part of
OLB. Drop the phandle.

> +
> +required:
> +  - compatible
> +  - "#pinctrl-cells"

So now please test your code without olb phandle...

> +
> +patternProperties:

patternProperties go after properties

> +  "-pins?$":
> +    type: object
> +    description: Pin muxing configuration.
> +    $ref: pinmux-node.yaml#
> +    additionalProperties: false

Why not unevaluatedProperties?

> +    properties:
> +      pins: true
> +      function: true
> +      bias-disable: true
> +      bias-pull-down: true
> +      bias-pull-up: true
> +      drive-strength: true
> +    required:
> +      - pins
> +      - function
> +
> +allOf:
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: mobileye,eyeq5-a-pinctrl
> +    then:
> +      patternProperties:
> +        "-pins?$":
> +          properties:
> +            function:
> +              enum: [gpio, timer0, timer1, timer2, timer5, uart0, uart1, can0,
> +                     can1, spi0, spi1, refclk0]
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: mobileye,eyeq5-b-pinctrl
> +    then:
> +      patternProperties:
> +        "-pins?$":
> +          properties:
> +            function:
> +              enum: [gpio, timer3, timer4, timer6, uart2, can2, spi2, spi3,
> +                     mclk0]
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    olb@e00000 {
> +      compatible = "mobileye,eyeq5-olb", "syscon", "simple-mfd";

Drop, not erlated.

> +      reg = <0xe00000 0x400>;
> +      reg-io-width = <4>;
> +
> +      pinctrl0 {

Suffixes are always after -

> +        compatible = "mobileye,eyeq5-a-pinctrl";
> +        #pinctrl-cells = <1>;

Where is the phandle?

> +      };
> +
> +      pinctrl1 {
> +        compatible = "mobileye,eyeq5-b-pinctrl";
> +        #pinctrl-cells = <1>;
> +      };
> +    };
> +  - |
> +    olb: olb@e00000 {
> +      compatible = "mobileye,eyeq5-olb", "syscon", "simple-mfd";
> +      reg = <0xe00000 0x400>;
> +      reg-io-width = <4>;
> +    };
> +
> +    pinctrl0 {
> +      compatible = "mobileye,eyeq5-a-pinctrl";
> +      #pinctrl-cells = <1>;
> +      mobileye,olb = <&olb>;

Really, why? This is just confusing. There is no explanation for
supporting both. Hardware is either this or that, not both!


Best regards,
Krzysztof