On Tue, 14 Jan 2025 15:01:31 +0800
Chen-Yu Tsai [off-list ref] wrote:

Hi Chen-Yu,

before I get to your specific question below: what do you think in
general of the idea of getting rid of that table based approach we use so
far? Is that something worthwhile? I definitely think yes, but wanted to
hear the maintainers' opinion about this. Happy to present some arguments
if need be.

...

On Wed, Nov 20, 2024 at 6:12 PM Andre Przywara [off-list ref] wrote:

quoted

On Wed, 13 Nov 2024 16:50:19 +0800
Chen-Yu Tsai [off-list ref] wrote:

Hi Chen-Yu,

sorry for the late reply, I was away for a week.
 

quoted

On Mon, Nov 11, 2024 at 8:58 AM Andre Przywara [off-list ref] wrote:  

quoted

The A523 contains a pin controller similar to previous SoCs, although
using 10 GPIO banks (PortB-PortK), all of them being IRQ capable.
This introduces a new style of binding, where the pinmux values for each
pin group is stored in the new "allwinner,pinmux" property in the DT
node, instead of requiring every driver to store a mapping between the
function names and the required pinmux.

Add the new name to the list of compatible strings, and required it to
have 10 interrupts described. Also add the new pinmux property.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 .../pinctrl/allwinner,sun4i-a10-pinctrl.yaml  | 23 +++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/pinctrl/allwinner,sun4i-a10-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/allwinner,sun4i-a10-pinctrl.yaml
index 4502405703145..6fc18e92e1e94 100644
--- a/Documentation/devicetree/bindings/pinctrl/allwinner,sun4i-a10-pinctrl.yaml
+++ b/Documentation/devicetree/bindings/pinctrl/allwinner,sun4i-a10-pinctrl.yaml

@@ -56,6 +56,8 @@ properties:
       - allwinner,sun50i-h6-r-pinctrl
       - allwinner,sun50i-h616-pinctrl
       - allwinner,sun50i-h616-r-pinctrl
+      - allwinner,sun55i-a523-pinctrl
+      - allwinner,sun55i-a523-r-pinctrl
       - allwinner,suniv-f1c100s-pinctrl
       - nextthing,gr8-pinctrl

@@ -64,7 +66,7 @@ properties:

   interrupts:
     minItems: 1
-    maxItems: 8
+    maxItems: 10
     description:
       One interrupt per external interrupt bank supported on the
       controller, sorted by bank number ascending order.

@@ -119,13 +121,17 @@ patternProperties:
         $ref: /schemas/types.yaml#/definitions/uint32
         enum: [10, 20, 30, 40]

+      allwinner,pinmux:
+        $ref: /schemas/types.yaml#/definitions/uint32-array
+        description: pinmux selector for each pin
+  

Why not just the standard "pinmux" property, as given in
Documentation/devicetree/bindings/pinctrl/pinmux-node.yaml  

I had it like this in my last post two years ago, but learned from
LinusW [1] that the generic pinmux property has a slightly different
meaning, and abusing it for just the pinmux index values would not match
the generic definition.
We *could* use the generic definition, but then this would include what's
in the "pins" property, like I sketched out in the cover letter, as an
alternative to this approach:
        pinmux = <SUNXI_PIN(PB, 9, 2)>, <SUNXI_PIN(PB, 10, 2)>;
Where the SUNXI_PIN macro would combine the pin number and the pinmux into
one 32-bit cell. See the Apple GPIO DT nodes for an example.
This looks indeed nicer, but requires quite some rewrite of the existing
pinctrl driver, AFAICS.  

Sorry for taking so long to get back to this.

Could we maybe add a generic replacement of the existing "function"
property, which takes a string? Like "function-id" or "function-selector"
that takes u32 (or u8). Then it could be one or the other. Not sure
if the binding maintainers would accept this or not.

Do you mean specifically a *generic* property, as opposed to something
prefixed with a vendor string, and coded up in just the sunxi driver?

Because otherwise "allwinner,pinmux" is that numeric equivalent to
"function". I kept "function", as a string, because the GPIO framework
still needs a string at places, for instance for sysfs. We could create
those strings, based on the node name, by sprintf'ing something, but I
figured we might as well keep "function".
In my U-Boot patches [1] I actually ignore the new pinmux property, and
still use the function name, as it was easier to integrate into the
existing code. U-Boot uses a very limited subset of our current table,
so each new SoC doesn't add much to the code.

[1] https://github.com/apritzel/u-boot/commit/ab4f7ed0879022357646

Code-wise I think we would still need our own driver code, so whether we
use "function-id" or "allwinner,pinmux" in there doesn't make much of a
difference, I think. 

I understand that we probably don't want the mux value combined with
the pin number.

You mean like the Apple GPIO does? I wonder why not, actually? I find this
actually quite clever and compact. Again the "pins" property atm is quite
string-heavy, so the code has to translate this back into a bank and pin
number, then lookup the function string in our table to get the pinmux
value. We could fit all of this information easily into this new
generic "pinmux" property, and the code just needs to mask and shift that
number. Each pin occupies a cell, I don't think we can get much better
than that?

Cheers,
Andre

ChenYu

quoted

[1] Previous reply from LinusW:
https://lore.kernel.org/linux-sunxi/CACRpkdbMc-Q6wjgsiddu6-tWC1dt2uFk+4LyerMdgFk2KRGK4w@mail.gmail.com/ (local)
 

quoted

quoted

     required:
       - pins
       - function  

This section should be made to apply only to the existing
compatibles? Maybe we could just split the files and have
a clean slate for sun55i?  

Yeah, I couldn't find a good example how to make it *required* for one
compatible and *not allowed* for all the others. But creating a whole new
file is actually a good idea, as this also avoids adding another case to
the already quite indented if-else cascade.

Cheers,
Andre
 

quoted

ChenYu
 

quoted

     additionalProperties: false

-  "^vcc-p[a-ilm]-supply$":
+  "^vcc-p[a-klm]-supply$":
     description:
       Power supplies for pin banks.

@@ -156,6 +162,17 @@ allOf:
         - interrupts
         - interrupt-controller

+  - if:
+      properties:
+        compatible:
+          enum:
+            - allwinner,sun55i-a523-pinctrl
+
+    then:
+      properties:
+        interrupts:
+          minItems: 10
+
   - if:
       properties:
         compatible:

@@ -166,6 +183,7 @@ allOf:
       properties:
         interrupts:
           minItems: 8
+          maxItems: 8

   - if:
       properties:

@@ -244,6 +262,7 @@ allOf:
             - allwinner,sun8i-v3s-pinctrl
             - allwinner,sun9i-a80-r-pinctrl
             - allwinner,sun50i-h6-r-pinctrl
+            - allwinner,sun55i-a523-r-pinctrl

     then:
       properties:
--

2.46.2