MIME-Version: 1.0
References: <20230123073305.149940-1-lchen@ambarella.com> <20230123073305.149940-14-lchen@ambarella.com>
In-Reply-To: <20230123073305.149940-14-lchen@ambarella.com>
From:   Linus Walleij <linus.walleij@linaro.org>
Date:   Mon, 23 Jan 2023 13:32:28 +0100
Message-ID: <CACRpkdYVGv=YM6cvtXVxNW42uM+dW_Zv-BkrDyMX+DB73w=2nA@mail.gmail.com>
Subject: Re: [PATCH 13/15] dt-bindings: pinctrl: add support for Ambarella
To:     Li Chen <lchen@ambarella.com>
Cc:     Li Chen <me@linux.beauty>, Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        "moderated list:ARM/Ambarella SoC support" 
        <linux-arm-kernel@lists.infradead.org>,
        "open list:PIN CONTROL SUBSYSTEM" <linux-gpio@vger.kernel.org>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@vger.kernel.org>,
        open list <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Li,

thanks for your patch!

It's nice to see Ambarella working with the kernel community.

On Mon, Jan 23, 2023 at 8:41 AM Li Chen <lchen@ambarella.com> wrote:

> +properties:
> +  compatible:
> +    items:
> +      - const: ambarella,pinctrl

I bet there will be more instances of pin controllers from Ambarella, so I would
use this only as a fallback, so the for should likely be:

compatible = "ambarella,<soc-name>-pinctrl", "ambarella,pinctrl";

we need to establish this already otherwise "ambarella,pinctrl" just becomes
the "weird name of the first ambarella SoC supported by standard DT bindings".

> +  amb,pull-regmap:
> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> +    items:
> +      maxItems: 1
> +
> +  amb,ds-regmap:
> +    items:
> +      maxItems: 1

Interesting that these registers are elsewhere, but I bet there is an
engineering
explanation for this :)

> +    properties:
> +      amb,pinmux-ids:
> +        description: |
> +          an integer array. Each integer in the array specifies a pin
> +          with given mux function, with pin id and mux packed as:
> +          mux << 12 | pin id
> +          Here mux means function of this pin, and pin id is identical to gpio id. For
> +          the SoC supporting IOMUX, like S2L, the maximal value of mux is 5. However,
> +          for the SoC not supporting IOMUX, like A5S, S2, the third or fourth function
> +          is selected by other "virtual pins" setting. Here the "virtual pins" means
> +          there is no real hardware pins mapping to the corresponding register address.
> +          So the registers for the "virtual pins" can be used for the selection of 3rd
> +          or 4th function for other real pins.

I think you can use the standard bindings for this if you insist on
using the "magic
numbers" scheme.

(I prefer function names and group names as strings, but I gave up on trying
to convince the world to use this because people have so strong opions about
it.)

From Documentation/devicetree/bindings/pinctrl/pinmux-node.yaml:

  pinmux:
    description:
      The list of numeric pin ids and their mux settings that properties in the
      node apply to (either this, "pins" or "groups" have to be specified)
    $ref: /schemas/types.yaml#/definitions/uint32-array

> +      amb,pinconf-ids:
> +        description: |
> +          an integer array. Each integer in the array specifies a pin
> +          with given configuration, with pin id and config packed as:
> +            config << 16 | pin id
> +          Here config is used to configure pull up/down and drive strength of the pin,
> +          and it's orgnization is:
> +          bit1~0: 00: pull down, 01: pull up, 1x: clear pull up/down
> +          bit2:   reserved
> +          bit3:   0: leave pull up/down as default value, 1: config pull up/down
> +          bit5~4: drive strength value, 0: 2mA, 1: 4mA, 2: 8mA, 3: 12mA
> +          bit6:   reserved
> +          bit7:   0: leave drive strength as default value, 1: config drive strength

I would be very happy if I could convince you to use the standard (string)
bindings for this.
And from Documentation/devicetree/bindings/pinctrl/pincfg-node.yaml

For each config node this means using strings such as
bias-high-impedance; etc in the device tree pin config node.

Following that scheme just makes life so much easier for maintainers and
reviewers: very few people reviewing or debugging the system will think
it is easy to read a magic number and then (in their head) mask out the
bits to see that "OK this is drive strength 8mA" and then have energy and
memory enough in their head to remember that "wait a minute, that is supposed
to be 12mA in this design", leading to long review and development
cycles.

By using:

drive-push-pull;
drive-strength = <8>;

you make the cognitive load on the people reading the device tree much
lower and easier to write, maintain and debug for humans.

The tendency to encode this info in terse bitfields appear to be done for either
of these reasons:

- Footprint / memory usage
- Adopt the users to the way the machine thinks instead of the other way around
- "We always did it this way"

Neither is a very good argument on a new 64bit platform.

Yours,
Linus Walleij