linux-kernel - Re: [PATCH v1 1/8] dt-bindings: pinctrl: Add thead,th1520-pinctrl bindings

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CACRpkdZf09uKr+ka0_rsw5kHMjjQbaGypn2fx2-QobLFBKYrtQ@mail.gmail.com>
Date: Wed, 20 Dec 2023 20:20:51 +0100
From: Linus Walleij <linus.walleij@...aro.org>
To: Emil Renner Berthing <emil.renner.berthing@...onical.com>
Cc: linux-gpio@...r.kernel.org, devicetree@...r.kernel.org, 
	linux-kernel@...r.kernel.org, linux-riscv@...ts.infradead.org, 
	Hoan Tran <hoan@...amperecomputing.com>, Serge Semin <fancer.lancer@...il.com>, 
	Bartosz Golaszewski <brgl@...ev.pl>, Andy Shevchenko <andy@...nel.org>, Rob Herring <robh+dt@...nel.org>, 
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>, Conor Dooley <conor+dt@...nel.org>, 
	Jisheng Zhang <jszhang@...nel.org>, Guo Ren <guoren@...nel.org>, Fu Wei <wefu@...hat.com>, 
	Paul Walmsley <paul.walmsley@...ive.com>, Palmer Dabbelt <palmer@...belt.com>
Subject: Re: [PATCH v1 1/8] dt-bindings: pinctrl: Add thead,th1520-pinctrl bindings

Hi Emil,

thanks for your patch!

On Fri, Dec 15, 2023 at 3:39 PM Emil Renner Berthing
<emil.renner.berthing@...onical.com> wrote:

> +  The TH1520 has 3 groups of pads each controlled from different memory ranges.
> +  Confusingly the memory ranges are named
> +    PADCTRL_AOSYS  -> PAD Group 1
> +    PADCTRL1_APSYS -> PAD Group 2
> +    PADCTRL0_APSYS -> PAD Group 3

Really, even in the documentation? If you look at the layout on the actual
chip, does a pattern emerge?

I think some use the north/south/east/west as group names with the BGA
chip facing up with the package text correctly readable then it is a bit
like a map.

> +          function:
> +            $ref: /schemas/types.yaml#/definitions/string
> +            enum: [ "0", "1", "2", "3", "4", "5" ]
> +            description: The mux function to select for the given pins.

So why is the opaque names "0", "1" etc used, and they will be the same for
all pins I bet. Most drivers use a string identifying the actual function here.
Such as "i2c", "gpio", etc.

Names that are just figures are *impossible* to understand without access
to a datasheet.

The point of device trees sources are to be human readable, strings of
magic numbers are not human readable at all.

> +          bias-disable: true
> +
> +          bias-pull-up:
> +            type: boolean
> +
> +          bias-pull-down:
> +            type: boolean
> +
> +          drive-strength:
> +            enum: [ 1, 2, 3, 5, 7, 8, 10, 12, 13, 15, 16, 18, 20, 21, 23, 25 ]

milliamperes? Then use drive-strength-microamp.

If not, explain what each setting means, i.e. the number of max microamps.

At which point using drive-strength-microamp and a translation table in the
driver may be a better idea.

The only reason to use opaque numbers is if 1, 2 (etc) mean something like
"number of driver stages" with a current output that varies with technology.

> +          thead,strong-pull-up:
> +            oneOf:
> +              - type: boolean
> +              - $ref: /schemas/types.yaml#/definitions/uint32
> +                enum: [ 0, 2100 ]
> +            description: Enable or disable strong 2.1kOhm pull-up.

Just use bias-pull-up with an argument.

bias-pull-up = <2100000>;

No argument would be the default setting.

No need for custom bindings.

> +        uart0_pins: uart0-0 {
> +            tx-pins {
> +                pins = "UART0_TXD";

Pins have reasonable names, but...

> +                function = "0";

What about function = "uart_0" hmmm?

Yours,
Linus Walleij