Re: [PATCH 1/3] dt-bindings: mtd: Relax BCM4908 partition schema

[Sam Edwards]

On Wed, Oct 9, 2024 at 7:38 PM Rob Herring <robh@xxxxxxxxxx> wrote:
>
> On Wed, Oct 09, 2024 at 02:50:44PM -0700, Sam Edwards wrote:
> > The BCM4908 partition "parser" is really just a fixed partitions table,
> > with a special partition compatible (`brcm,bcm4908-firmware`) that
> > automatically labels the partition as "firmware" or "backup" depending
> > on what CFE is communicating as the selected active partition.
> >
> > The bcm4908-partitions schema is currently too restrictive, requiring
> > that all child nodes use this special compatible or none at all. This
> > not only contracits what is allowed by the "parser" but also causes
> > warnings for an existing file ("bcm4908-asus-gt-ac5300.dts").
> >
> > Modify the schema to be strict only for child partitions that use the
> > -firmware compatible. Also update the child name regex to agree with
> > fixed-partitions, so that these differences apply consistently.
> >
> > Signed-off-by: Sam Edwards <CFSworks@xxxxxxxxx>
> > ---
> >  .../mtd/partitions/brcm,bcm4908-partitions.yaml | 17 +++++++++++------
> >  1 file changed, 11 insertions(+), 6 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/mtd/partitions/brcm,bcm4908-partitions.yaml b/Documentation/devicetree/bindings/mtd/partitions/brcm,bcm4908-partitions.yaml
> > index 94f0742b375c..aed37922a5fc 100644
> > --- a/Documentation/devicetree/bindings/mtd/partitions/brcm,bcm4908-partitions.yaml
> > +++ b/Documentation/devicetree/bindings/mtd/partitions/brcm,bcm4908-partitions.yaml
> > @@ -30,12 +30,17 @@ properties:
> >      enum: [ 1, 2 ]
> >
> >  patternProperties:
> > -  "^partition@[0-9a-f]+$":
> > -    $ref: partition.yaml#
> > -    properties:
> > -      compatible:
> > -        const: brcm,bcm4908-firmware
> > -    unevaluatedProperties: false
> > +  "^partition(-.+|@[0-9a-f]+)$":
> > +    type: object
> > +    if:
> > +      properties:
> > +        compatible:
> > +          const: brcm,bcm4908-firmware

Hi Rob,

Thanks for your attention on this one! This is an odd case where I'm
not sure what else could be done, and I'm a newbie to modifying DT
schemata, so I'm glad to be receiving your feedback. :)

> What schema applies to the node if this is not true? That needs to be
> addressed. You should be able to use oneOf here rather than if/then
> schema.

The schema that should apply "in general" is partition.yaml, though
moving the '$ref:' outside of 'then:' made the 'unevaluatedProperties:
false' disallow everything. The if block here is just trying to
memorialize bcm4908-firmware as a valid compatible at this level, and
(ideally) disallow unevaluated properties if so.

Could that be done with a oneOf? I would think if one of the arms of
the oneOf was "unmodified $partition.yaml, unevaluated allowed" it
would always match, rendering the "unevaluated disallowed when
compatible=bcm4908-firmware" arm unused.

Best,
Sam

>
> > +    then:
> > +      $ref: partition.yaml#
> > +      properties:
> > +        compatible: true
> > +      unevaluatedProperties: false
> >
> >  required:
> >    - "#address-cells"
> > --
> > 2.44.2
> >