Messages in this thread | ![/](/images/icornerl.gif) | | From | Linus Walleij <> | Date | Wed, 20 Dec 2023 20:20:51 +0100 | 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@canonical.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
| ![\](/images/icornerr.gif) |