Re: [PATCH 3/6] dt-bindings: pinctrl: Add fsl,ls1012a-pinctrl yaml file

[David Leonard]

On Tue, 27 Aug 2024, Krzysztof Kozlowski wrote:

> On Tue, Aug 27, 2024 at 12:10:44PM +1000, David Leonard wrote:
> > Add a binding schema and examples for the LS1012A's pinctrl function.
> >
> > Signed-off-by: David Leonard <David.Leonard@xxxxxxxx>
> > ---
>
> It does not look like you tested the bindings, at least after quick
> look. Please run  (see
> Documentation/devicetree/bindings/writing-schema.rst for instructions).
> Maybe you need to update your dtschema and yamllint.

I've since updated tools (dt-valdate 2024.5 and yamllint 1.33.0) and
rebased onto v6.11-rc1.

> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/pinctrl/fsl,ls1012a-pinctrl.yaml
> > @@ -0,0 +1,83 @@

> > +description: >
>
> Drop >
>
> > +  Bindings for LS1012A pinmux control.
>
> Drop "Bindings for" and explain the hardware.

Changed to

 description:
   The LS1012A pinmux controller can override the RCW and
   alter some pin groups' functions in a limited way.

> > +
> > +properties:
> > +  compatible:
> > +    const: fsl,ls1012a-pinctrl
> > +
> > +  reg:
> > +    description: Specifies the base address of the PMUXCR0 register.
> > +    maxItems: 2
>
> Instead list and describe the items.

Changed to

   reg:
     items:
       - description: Physical base address of the PMUXCR0 register.
       - description: Size of the PMUXCR0 register (4).

Is this what you meant?

> > +
> > +  big-endian:
> > +    description: If present, the PMUXCR0 register is implemented in big-endian.
>
> Why is this here? Either it is or it is not?

You're right. Changed to

   big-endian: true

(This also lead to some code simplification)

> > +    type: boolean
> > +
> > +  dcfg-regmap:
>
> Missing vendor prefix.

Will use "fsl,"

> > +    $ref: /schemas/types.yaml#/definitions/phandle
> > +    description:
> > +      The phandle of the syscon node for the DCFG registers.
>
> Instead explain what it is needed it for, how is it used.

Changed to

   fsl,dcfg-regmap:
     $ref: /schemas/types.yaml#/definitions/phandle
     description:
       The phandle of the syscon node for the DCFG registers
       providing the RCW's pin configuration that is in effect when
       the pinmux controller is disabled.

> > +
> > +patternProperties:
> > +  '^pinctrl-':
>
> Rather -pins$ or ^pins-

Changed to ^pins-  See example at the end.

> > +allOf:
> > +  - $ref: pinctrl.yaml#
>
> Thies goes after required.
>
> > +
> > +required:
> > +  - compatible
> > +  - reg

Relocated

> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    pinctrl: pinctrl@1570430 {
> > +        compatible = "fsl,ls1012a-pinctrl";
> > +        reg = <0x0 0x1570430 0x0 0x4>;
> > +        big-endian;
> > +        dcfg-regmap = <&dcfg>;
> > +        pinctrl_qspi_1: pinctrl-qspi-1 {
> > +            groups = "qspi_1_grp";
> > +            function = "spi";
> > +        };
> > +        pinctrl_qspi_2: pinctrl-qspi-2 {
> > +            groups = "qspi_1_grp", "qspi_2_grp";
> > +            function = "spi";
> > +        };
> > +        pinctrl_qspi_4: pinctrl-qspi-4 {
> > +            groups = "qspi_1_grp", "qspi_2_grp", "qspi_3_grp";
> > +            function = "spi";
> > +        };
> > +    };
> > +  - |
> > +    qspi: quadspi@1550000 {
>
> Drop, useless and not related.

Dropped the qspi example. Examples are now

  examples:
    - |
      pinctrl: pinctrl@1570430 {
	  compatible = "fsl,ls1012a-pinctrl";
	  reg = <0x0 0x1570430 0x0 0x4>;
	  big-endian;
	  fsl,dcfg-regmap = <&dcfg>;
	  pinctrl_qspi_1: pins-qspi-1 { /* buswidth 1 */
	      groups = "qspi_1_grp";
	      function = "spi";
	  };
	  pinctrl_qspi_2: pins-qspi-2 { /* buswidth 2 */
	      groups = "qspi_1_grp", "qspi_2_grp";
	      function = "spi";
	  };
	  pinctrl_qspi_4: pins-qspi-4 { /* buswidth 4 */
	      groups = "qspi_1_grp", "qspi_2_grp", "qspi_3_grp";
	      function = "spi";
	  };
      };


Thanks,

David