[PATCH 2/3] dt-bindings: firmware: xilinx: Add conditional pinctrl schema

[Jain, Ronak]

[AMD Official Use Only - AMD Internal Distribution Only]

Hi Rob,

> -----Original Message-----
> From: Jain, Ronak
> Sent: Thursday, February 13, 2025 4:46 PM
> To: Rob Herring <robh at kernel.org>
> Cc: krzk+dt at kernel.org; conor+dt at kernel.org; Simek, Michal
> <michal.simek at amd.com>; Manne, Nava kishore
> <nava.kishore.manne at amd.com>; devicetree at vger.kernel.org; linux-arm-
> kernel at lists.infradead.org; linux-kernel at vger.kernel.org
> Subject: RE: [PATCH 2/3] dt-bindings: firmware: xilinx: Add conditional pinctrl
> schema
>
> Hi Rob,
>
> > -----Original Message-----
> > From: Rob Herring <robh at kernel.org>
> > Sent: Wednesday, February 12, 2025 3:24 AM
> > To: Jain, Ronak <ronak.jain at amd.com>
> > Cc: krzk+dt at kernel.org; conor+dt at kernel.org; Simek, Michal
> > <michal.simek at amd.com>; Manne, Nava kishore
> > <nava.kishore.manne at amd.com>; devicetree at vger.kernel.org; linux-arm-
> > kernel at lists.infradead.org; linux-kernel at vger.kernel.org
> > Subject: Re: [PATCH 2/3] dt-bindings: firmware: xilinx: Add conditional pinctrl
> > schema
> >
> > On Thu, Feb 06, 2025 at 06:22:43AM -0800, Ronak Jain wrote:
> > > Updates the Device Tree bindings for Xilinx firmware by introducing
> > > conditional schema references for the pinctrl node.
> > >
> > > Previously, the pinctrl node directly referenced
> > > xlnx,zynqmp-pinctrl.yaml. However, this patch modifies the schema to
> > > conditionally apply the correct pinctrl schema based on the compatible
> > > property. Specifically:
> > > - If compatible contains "xlnx,zynqmp-pinctrl", reference
> > >   xlnx,zynqmp-pinctrl.yaml.
> > > - If compatible contains "xlnx,versal-pinctrl", reference
> > >   xlnx,versal-pinctrl.yaml.
> > >
> > > Additionally, an example entry for "xlnx,versal-pinctrl" has been
> > > added under the examples section.
> > >
> > > Signed-off-by: Ronak Jain <ronak.jain at amd.com>
> > > ---
> > >  .../firmware/xilinx/xlnx,zynqmp-firmware.yaml | 20 ++++++++++++++++++-
> > >  1 file changed, 19 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/Documentation/devicetree/bindings/firmware/xilinx/xlnx,zynqmp-
> > firmware.yaml b/Documentation/devicetree/bindings/firmware/xilinx/xlnx,zynqmp-
> > firmware.yaml
> > > index 2b72fb9d3c22..d50438b0fca8 100644
> > > --- a/Documentation/devicetree/bindings/firmware/xilinx/xlnx,zynqmp-
> > firmware.yaml
> > > +++ b/Documentation/devicetree/bindings/firmware/xilinx/xlnx,zynqmp-
> > firmware.yaml
> > > @@ -76,7 +76,6 @@ properties:
> > >      type: object
> > >
> > >    pinctrl:
> > > -    $ref: /schemas/pinctrl/xlnx,zynqmp-pinctrl.yaml#
> > >      description: The pinctrl node provides access to pinconfig and pincontrol
> > >        functionality available in firmware.
> > >      type: object
> > > @@ -106,6 +105,21 @@ properties:
> > >      type: object
> > >      deprecated: true
> > >
> > > +allOf:
> > > +  - if:
> > > +      properties:
> > > +        compatible:
> > > +          contains:
> > > +            const: xlnx,zynqmp-firmware
> > > +    then:
> > > +      properties:
> > > +        pinctrl:
> > > +          $ref: /schemas/pinctrl/xlnx,zynqmp-pinctrl.yaml#
> > > +    else:
> > > +      properties:
> > > +        pinctrl:
> > > +          $ref: /schemas/pinctrl/xlnx,versal-pinctrl.yaml#
> >
> > The somewhat preferred way to do this would be to do this in the top
> > level:
> >
> > pinctrl:
> >   type: object
> >   additionalProperties: true
> >   properties:
> >     compatible:
> >       contains:
> >         enum:
> >           - xlnx,zynqmp-pinctrl
> >           - xlnx,versal-pinctrl
> >   required:
> >     - compatible
> >
> > Otherwise, the pinctrl schema ends up being applied twice.
>
> Thanks for the patch review and inputs. I'll update and send the next version.
>

In your suggested code, the schema allows either xlnx,zynqmp-pinctrl or xlnx,versal-pinctrl on any platform, which is incorrect. This means that if a user mistakenly assigns xlnx,versal-pinctrl to a ZynqMP platform or xlnx,zynqmp-pinctrl to a Versal platform, the wrong reference will be used, but no error is reported. The dt-binding check still passes instead of flagging this as an issue.

By using a conditional schema, we can enforce platform-specific compatibility, ensuring that the correct compatible string is used for the corresponding platform. This would also generate an error if an incorrect compatible string is provided, preventing misconfigurations.

Please let me know your thoughts.

Thanks,
Ronak

> Thanks,
> Ronak
> >
> > > +
> > >  required:
> > >    - compatible
> > >
> > > @@ -164,6 +178,10 @@ examples:
> > >          compatible = "xlnx,versal-fpga";
> > >        };
> > >
> > > +      pinctrl {
> > > +        compatible = "xlnx,versal-pinctrl";
> > > +      };
> > > +
> > >        xlnx_aes: zynqmp-aes {
> > >          compatible = "xlnx,zynqmp-aes";
> > >        };
> > > --
> > > 2.34.1
> > >