Re: [RESEND PATCH v3 1/2] dt-bindings: mtd: partitions: Document new partition-dynamic nodes

[Rob Herring]

On Wed, May 04, 2022 at 10:59:44PM +0200, Rafał Miłecki wrote:
> On 4.05.2022 22:52, Ansuel Smith wrote:
> > On Wed, May 04, 2022 at 10:39:14PM +0200, Rafał Miłecki wrote:
> > > On 29.04.2022 14:48, Ansuel Smith wrote:
> > > > Document new partition-dynamic nodes used to provide an OF node for
> > > > partition registred at runtime by parsers. This is required for nvmem
> > > > system to declare and detect nvmem-cells.
> > > > 
> > > > With these special partitions, the reg / offset is not required.
> > > > The node name must be in the form of "partition name"-dynamic.
> > > > If the partition can't be displayed using the node name, it's possible
> > > > to use the label binding that will be used instead of the node name.
> > > > The node name or the label binding is used to match the partition
> > > > allocated by the parser at runtime and the parser will provide reg
> > > > and offset of the mtd.
> > > > 
> > > > NVMEM will use the data from the parser and provide the NVMEM cells
> > > > declared in the DTS, "connecting" the dynamic partition with a
> > > > static declaration of cells in them.
> > > > 
> > > > Signed-off-by: Ansuel Smith <ansuelsmth@xxxxxxxxx>
> > > > ---
> > > >    .../mtd/partitions/partition-dynamic.yaml     | 56 +++++++++++++++++++
> > > >    .../mtd/partitions/qcom,smem-part.yaml        |  4 ++
> > > >    2 files changed, 60 insertions(+)
> > > >    create mode 100644 Documentation/devicetree/bindings/mtd/partitions/partition-dynamic.yaml
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/mtd/partitions/partition-dynamic.yaml b/Documentation/devicetree/bindings/mtd/partitions/partition-dynamic.yaml
> > > > new file mode 100644
> > > > index 000000000000..e0efa58e4fac
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/mtd/partitions/partition-dynamic.yaml
> > > > @@ -0,0 +1,56 @@
> > > > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> > > > +%YAML 1.2
> > > > +---
> > > > +$id: http://devicetree.org/schemas/mtd/partitions/partition-dynamic.yaml#
> > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > +
> > > > +title: Dynamic Partition
> > > 
> > > I'm not native but that "Dynamic Partition" sounds pretty natural and
> > > I'm wondering if you shouldn't make that binding dynamic-partition.yaml
> > > 
> > > Any natives to comment on this? :)
> > > 
> > > 
> > 
> > The naming for the file is used to keep the standard of
> > [parser]-partition.yaml. Agree that we should find a better naming for
> > all of this.
> 
> Someone has to break the pattern and start naming things sane ;)
> 
> 
> > > > +description: |
> > > > +  This binding describes a single flash partition that is dynamically allocated
> > > > +  by a dedicated parser that is not a fixed-partition parser.
> > > > +
> > > > +  A dynamic partition require the node ending with the "-dynamic" tag and if the
> > > > +  dynamic partition name can't be displayed using the node name, the label
> > > > +  properties can be used. The node name or the label have to match the dynamic
> > > > +  partition allocated by the parser.
> > > > +
> > > > +  These special partition definition can be used to give a dynamic partition
> > > > +  an OF node to declare NVMEM cells. An example is declaring the partition
> > > > +  label and all the NVMEM cells in it. The parser will detect the correct reg
> > > > +  and offset and the NVMEM will register the cells in it based on the data
> > > > +  extracted by the parser.
> > > > +
> > > > +maintainers:
> > > > +  - Ansuel Smith <ansuelsmth@xxxxxxxxx>
> > > > +
> > > > +properties:
> > > > +  label:
> > > > +    description: The label / name for the partition assigned by the parser at
> > > > +      runtime. This is needed for sybsystem like NVMEM to define cells and
> > > > +      register with this OF node.
> > > > +
> > > > +additionalProperties: true
> > > > +
> > > > +examples:
> > > > +  - |
> > > > +    flash {
> > > > +      partitions {
> > > > +        compatible = "qcom,smem-part";
> > > > +
> > > > +        art-dynamic {
> > > > +          compatible = "nvmem-cells";
> > > > +          #address-cells = <1>;
> > > > +          #size-cells = <1>;
> > > > +          label = "0:art";
> > > > +
> > > > +          macaddr_art_0: macaddr@0 {
> > > > +            reg = <0x0 0x6>;
> > > > +          };
> > > > +
> > > > +          macaddr_art_6: macaddr@6 {
> > > > +            reg = <0x6 0x6>;
> > > > +          };
> > > > +        };
> > > > +      };
> > > > +    };
> > > 
> > > I see that we need a property (like "label") for storing partition name
> > > as it may contain characters not allowed in $nodename.
> > > 
> > > Is there a reason to play with all that foo-dynamic $nodename then? With
> > > fallback from "label" to extracting foo from *-dynamic pattern?
> > > 
> > 
> > Honestly the "-dynamic" thing is to correctly handle this ""strange""
> > Documentation. At times using the pattern caused tons of problems with
> > pattern so I had this bright idea of using the suffix "-dynamic" to
> > cleary differentiate these special partition from fixed one.
> > 
> > > Could we just be lazy, keep things simple and require "label" property?
> > > 
> > 
> > This is problematic to correctly assign a patternProperties to any user
> > or this parser.
> > 
> > > Then we could e.g. require $nodename to be pattern ^partition-[0-9a-f]+$
> > > It's what leds-gpio.yaml does for reference.
> > > 
> > 
> > Mhhh ok I can totally make this change. My concern is that someone would
> > get confused thinking they are fixed partition declared on top of the
> > parser. But yhea this can also work... It's really a similar
> > implementation of what I already to with dynamic. If you want I can do
> > this change and send a v4.
> 
> We can wait for more reviews, in case other developers think my idea
> may be confusing. For me seeing partition without address is easy to
> interpret as dynamic partition but let's see what others think.

That sounds good to me.

Rob