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

[Neil Armstrong <neil.armstrong@linaro.org>]

On 05/02/2023 17:54, Heiner Kallweit wrote:
> On 05.02.2023 08:57, Neil Armstrong wrote:
>>
>>> Le 5 févr. 2023 à 01:05, Heiner Kallweit <hkallweit1@gmail.com> a écrit :
>>>
>>> On 04.02.2023 23:56, Martin Blumenstingl wrote:
>>>> Hi Heiner,
>>>>
>>>> On Wed, Feb 1, 2023 at 11:13 PM Heiner Kallweit <hkallweit1@gmail.com> wrote:
>>>> [...]
>>>>>>> +      - 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

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic