Re: [PATCH v3] dt-bindings: pinctrl: Convert Amlogic Meson pinctrl binding

On 05/02/2023 17:54, Heiner Kallweit wrote:

On 05.02.2023 08:57, Neil Armstrong wrote:

quoted

quoted

Le 5 févr. 2023 à 01:05, Heiner Kallweit [off-list ref] a écrit :

On 04.02.2023 23:56, Martin Blumenstingl wrote:

quoted

Hi Heiner,

On Wed, Feb 1, 2023 at 11:13 PM Heiner Kallweit [off-list ref] wrote:
[...]

quoted

quoted

quoted

+      - items:
+          - const: amlogic,meson8m2-aobus-pinctrl
+          - const: amlogic,meson8-aobus-pinctrl
+      - items:
+          - const: amlogic,meson8m2-cbus-pinctrl
+          - const: amlogic,meson8-cbus-pinctrl

Again, can't have both with and without the fallback allowed.

Hi Martin,

meson8m2 is the only chip version having a fallback for the
pinctrl compatible. Is this fallback really needed?
Looking at the driver it seems that both compatibles
are handled identically.

Back in the day we decided to duplicate the Meson8 driver code just to
add four new pin functions that are added by the Meson8m2 SoC
generation:
"eth_rxd2", "eth_rxd3", "eth_txd2", "eth_txd3"

The compatible string was defined with a similar approach: since
Meson8m2 just adds a few bits to the Meson8 pin controller it's
backwards compatible.

If the fallback has to be removed then I'm okay with that but I would
like to understand it first.
So far I thought that Rob basically asked to remove the following two
compatible strings from the enum (as they're listed separately with
their fallbacks):
- amlogic,meson8m2-cbus-pinctrl
- amlogic,meson8m2-aobus-pinctrl

Right, this should be sufficient. There's no place where the 8m2 pinctrl
compatibles are used w/o fallback.

Then the hopefully final version of the binding is almost ready.
I'm just still checking whether there's any way in yaml to specify
a reg-names list with mandatory and optional names. Doesn't seem so.

It’s possible with minItems/maxItems and allOf:if/else to specify different min/max for each compatible

I have a problem here because reg-names is in the gpio-controller child node and compatible is in the
pinctrl parent node. An expression under patternProperties doesn't see the compatible property of the
parent node. It would have been better to place regs/reg-names in the pinctrl node.
Not sure how to deal with this. The easiest would be to just have minItems:2 and maxItems:5 as criteria
for regs and reg-names.

In this case, either you add 2 yaml bindings with perhaps a common yaml using allOf:ref or
you add some complex matching but I think you should split it in 3 files like:


================================================================
amlogic,meson-pinctrl-common.yaml:
...

properties:
   ranges: true

   "#address-cells":
     enum: [1, 2]

   "#size-cells":
     enum: [1, 2]

patternProperties:
   "-state$":
     allOf:
       - $ref: pincfg-node.yaml#
       - $ref: pinmux-node.yaml#

allOf:
   - $ref: pinctrl.yaml#

required:
   - compatible
   - ranges
   - "#address-cells"
   - "#size-cells"

additionalProperties: true

$defs:
   meson-gpio:
     type: object
     properties:
       gpio-controller: true

       "#gpio-cells":
         const: 2

       gpio-ranges:
         $ref: /schemas/types.yaml#/definitions/phandle

       required:
         - gpio-controller
         - "#gpio-cells"
         - gpio-ranges
...

================================================================
amlogic,meson8-cbus-pinctrl.yaml:

...
$ref: amlogic,meson-pinctrl-common.yaml#

properties:
   compatible:
     oneOf:
       - items:
           - enum:
               - amlogic,meson8-cbus-pinctrl
               - amlogic,meson8b-cbus-pinctrl
               - amlogic,meson8-aobus-pinctrl
               - amlogic,meson8b-aobus-pinctrl
       - items:
           - const: amlogic,meson8m2-aobus-pinctrl
           - const: amlogic,meson8-aobus-pinctrl
       - items:
           - const: amlogic,meson8m2-cbus-pinctrl
           - const: amlogic,meson8-cbus-pinctrl

required:
   - compatible

patternProperties:
   "^bank@[0-9]$":
     $ref: "#/$defs/meson-gpio"

     properties:
       reg:
         maxItems: 2

       reg-names:
         items:
           - const: gpio
           - const: pull

     required:
       - reg
       - reg-names
...

================================================================
amlogic,meson-gxbb-periphs-pinctrl.yaml:

...
$ref: amlogic,meson-pinctrl-common.yaml#

properties:
   compatible:
     items:
       - enum:
          - amlogic,meson-gxbb-periphs-pinctrl
          - amlogic,meson-gxbb-aobus-pinctrl
          - amlogic,meson-gxl-periphs-pinctrl
          - amlogic,meson-gxl-aobus-pinctrl
          - amlogic,meson-axg-periphs-pinctrl
          - amlogic,meson-axg-aobus-pinctrl
          - amlogic,meson-g12a-periphs-pinctrl
          - amlogic,meson-g12a-aobus-pinctrl
          - amlogic,meson-a1-periphs-pinctrl
          - amlogic,meson-s4-periphs-pinctrl

required:
   - compatible

patternProperties:
   "^bank@[0-9]$":
     $ref: "#/$defs/meson-gpio"

     properties:
       reg:
         minItems: 5

       reg-names:
         items:
           - const: gpio
           - const: pull
           - const: pull-enable
           - const: mux
           - const: ds

     required:
       - reg
       - reg-names
...
================================================================

You may even add a 4th file for G12/A1 is reg-names need a different count.

Neil