Hi Emil,
thanks for your patch!
On Fri, Dec 15, 2023 at 3:39 PM Emil Renner Berthing
<emil.renner.berthing@xxxxxxxxxxxxx> 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
