Re: [PATCH] dt-bindings: mtd: convert "fixed-partitions" to the json-schema

[Rob Herring <robh@kernel.org>]

On Thu, Dec 10, 2020 at 7:56 AM Rafał Miłecki <zajec5@gmail.com> wrote:
>
> On 10.12.2020 03:48, Rob Herring wrote:
> > On Wed, Dec 09, 2020 at 02:02:35PM +0100, Rafał Miłecki wrote:
> >> From: Rafał Miłecki <rafal@milecki.pl>
> >>
> >> This standardizes its documentation, allows validating with Makefile
> >> checks and helps writing DTS files.
> >>
> >> Noticeable changes:
> >> 1. Dropped "Partitions can be represented by sub-nodes of a flash
> >>     device." as we also support subpartitions (don't have to be part of
> >>     flash device node)
> >> 2. Dropped "to Linux" as bindings are meant to be os agnostic.
> >>
> >> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> >> ---
> >>   .../devicetree/bindings/mtd/partition.txt     | 131 +---------------
> >>   .../mtd/partitions/fixed-partitions.yaml      | 146 ++++++++++++++++++
> >>   2 files changed, 148 insertions(+), 129 deletions(-)
> >>   create mode 100644 Documentation/devicetree/bindings/mtd/partitions/fixed-partitions.yaml

> >> diff --git a/Documentation/devicetree/bindings/mtd/partitions/fixed-partitions.yaml b/Documentation/devicetree/bindings/mtd/partitions/fixed-partitions.yaml
> >> new file mode 100644
> >> index 000000000000..c5e509e08f31
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/mtd/partitions/fixed-partitions.yaml
> >> @@ -0,0 +1,146 @@
> >> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> >> +%YAML 1.2
> >> +---
> >> +$id: http://devicetree.org/schemas/mtd/partitions/fixed-partitions.yaml#
> >> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >> +
> >> +title: Fixed partitions
> >> +
> >> +description: |
> >> +  This binding can be used on platforms which have strong conventions about
> >> +  which portions of a flash are used for what purposes, but which don't use an
> >> +  on-flash partition table such as RedBoot.
> >> +
> >> +  The partition table should be a node named "partitions". Partitions are then
> >> +  defined as subnodes.
> >> +
> >> +maintainers:
> >> +  - Rafał Miłecki <rafal@milecki.pl>
> >> +
> >> +properties:
> >> +  compatible:
> >> +    const: fixed-partitions
> >> +
> >> +patternProperties:
> >> +  "^.*@[0-9a-f]+$":
> >
> > You can drop '^.*'.
> >
> > This needs to recurse to nested nodes.
> >
> > I think here you can do just:
> >
> > $ref: #/
> >
> > And drop 'compatible' as required. It's redundant anyways because the
> > schema will only be applied if compatible matches.
>
> I managed to implement recursive schema but then I run dt_binding_check and
> realized it may be not what we really want.
>
> The error I got was:
> Documentation/devicetree/bindings/mtd/partitions/fixed-partitions.example.dt.yaml: partitions: firmware@100000:compatible:0: 'fixed-partitions' was expected
>          From schema: Documentation/devicetree/bindings/mtd/partitions/fixed-partitions.yaml
>
> and was caused by the following example:
> partitions {
>         compatible = "fixed-partitions";
>         #address-cells = <1>;
>         #size-cells = <1>;
>
>         (...)
>
>         firmware@100000 {
>                 label = "firmware";
>                 reg = <0x100000 0xe00000>;
>                 compatible = "brcm,trx";
>         };
>
>         (...)
> }
>
> As you can see a single partition can use any available parser, so we can't
> require only "fixed-partitions" to be nested in the "fixed-partitions".
>
> In this situation I think this commit may be OK after all if I just fix regex
> and required (drop "compatible").
>
> What do you think?

You're still going to have the above error with the schema below.

I completely missed that the nested example has "fixed-partitions"
compatible in it. So what you had is actually fine in that regard. Let
me start over reviewing.

> FWIW:
>
> properties:
>    compatible:
>      const: fixed-partitions
>
> patternProperties:
>    "@[0-9a-f]+$":
>      allOf:
>        - $ref: "#"
>        - properties:
>            reg:
>              maxItems: 1
>              description: partition's offset and size within the flash
>
>            label:
>              description: The label / name for this partition. If omitted, the label
>                is taken from the node name (excluding the unit address).
>
>            read-only:
>              description: This parameter, if present, is a hint that this partition
>                should only be mounted read-only. This is usually used for flash
>                partitions containing early-boot firmware images or data which should
>                not be clobbered.
>              type: boolean
>
>            lock:
>              description: Do not unlock the partition at initialization time (not
>                supported on all devices)
>              type: boolean
>
>            slc-mode:
>              description: This parameter, if present, allows one to emulate SLC mode
>                on a partition attached to an MLC NAND thus making this partition
>                immune to paired-pages corruptions
>              type: boolean
>
>          required:
>            - reg