Received-SPF: pass (google.com: domain of linux-kernel+bounces-8325-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
From: Emil Renner Berthing <emil.renner.berthing@canonical.com>
In-Reply-To: <CACRpkdZf09uKr+ka0_rsw5kHMjjQbaGypn2fx2-QobLFBKYrtQ@mail.gmail.com>
References: <20231215143906.3651122-1-emil.renner.berthing@canonical.com>
 <20231215143906.3651122-2-emil.renner.berthing@canonical.com> <CACRpkdZf09uKr+ka0_rsw5kHMjjQbaGypn2fx2-QobLFBKYrtQ@mail.gmail.com>
Precedence: bulk
Mime-Version: 1.0
Date: Thu, 21 Dec 2023 04:21:34 -0800
Message-ID: <CAJM55Z8hT_fUAjzNR=7aoWJPU8MMumjdFy74MDD85Yic2Gjg-A@mail.gmail.com>
Subject: Re: [PATCH v1 1/8] dt-bindings: pinctrl: Add thead,th1520-pinctrl bindings
To: Linus Walleij <linus.walleij@linaro.org>, Jisheng Zhang <jszhang@kernel.org>, 
	Guo Ren <guoren@kernel.org>, Drew Fustini <dfustini@baylibre.com>, 
	Emil Renner Berthing <emil.renner.berthing@canonical.com>
Cc: linux-gpio@vger.kernel.org, devicetree@vger.kernel.org, 
	linux-kernel@vger.kernel.org, linux-riscv@lists.infradead.org, 
	Hoan Tran <hoan@os.amperecomputing.com>, Serge Semin <fancer.lancer@gmail.com>, 
	Bartosz Golaszewski <brgl@bgdev.pl>, Andy Shevchenko <andy@kernel.org>, Rob Herring <robh+dt@kernel.org>, 
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>, Conor Dooley <conor+dt@kernel.org>, 
	Fu Wei <wefu@redhat.com>, Paul Walmsley <paul.walmsley@sifive.com>, 
	Palmer Dabbelt <palmer@dabbelt.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Linus Walleij wrote:
> Hi Emil,
>
> thanks for your patch!
>
> On Fri, Dec 15, 2023 at 3:39=E2=80=AFPM Emil Renner Berthing
> <emil.renner.berthing@canonical.com> wrote:
>
> > +  The TH1520 has 3 groups of pads each controlled from different memor=
y 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 actua=
l
> chip, does a pattern emerge?

Yes, the documentation is where I got this from:
https://git.beagleboard.org/beaglev-ahead/beaglev-ahead/-/blob/main/docs/TH=
1520%20System%20User%20Manual.pdf

The pinmux chapter starting on page 31 only talks about the 3 pad groups, b=
ut
if you match the base addresses, table 3-8 page 46, with the memory map, ta=
ble
1-2 page 1, they same base addresses have the PADCTRL names above.

> 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.

I don't know if or where such documentation is available.
Jisheng, Guo or Drew, do you know?

> > +          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 f=
or
> 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=
.

It *is* the number of mA. I can change it uA if that's better.

> At which point using drive-strength-microamp and a translation table in t=
he
> driver may be a better idea.

That's what it does, just with mA.

> The only reason to use opaque numbers is if 1, 2 (etc) mean something lik=
e
> "number of driver stages" with a current output that varies with technolo=
gy.
>
> > +          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 =3D <2100000>;
>
> No argument would be the default setting.
>
> No need for custom bindings.
>
> > +        uart0_pins: uart0-0 {
> > +            tx-pins {
> > +                pins =3D "UART0_TXD";
>
> Pins have reasonable names, but...
>
> > +                function =3D "0";
>
> What about function =3D "uart_0" hmmm?
>
> Yours,
> Linus Walleij
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv