Re: [PATCH v1 09/14] dt-bindings: nvmem: add YAML schema for the sl28 vpd layout

[Michael Walle]

Am 2022-08-31 11:24, schrieb Krzysztof Kozlowski:
> On 31/08/2022 11:17, Michael Walle wrote:

> > First thing, this binding isn't like the usual ones, so it might be
> > totally wrong.
> >
> > What I'd like to achieve here is the following:
> >
> > We have the nvmem-consumer dt binding where you can reference a
> > nvmem cell in a consumer node. Example:
> >    nvmem-cells = <&base_mac_address 5>;
> >    nvmem-cell-names = "mac-address";
> >
> > On the other end of the link we have the nvmem-provider. The dt
> > bindings works well if that one has individual cell nodes, like
> > it is described in the nvmem.yaml binding. I.e. you can give the
> > cell a label and make a reference to it in the consumer just like
> > in the example above.
>
> You can also achieve it with phandle argument to the nvmwm controller,
> right? Just like most of providers are doing (clocks, resets). Having
> fake (empty) nodes just for that seems like overkill.

You mean like
 nvmem-cells = <&nvmem_device SERIAL_NUMBER>;

I'm not sure about the implications for now, because one is
referencing the device and not individal cells. Putting that
aside for now, there seems to be a problem with the index for
the base mac address: You will have different number of arguments
for the phandle. That doesn't work, right?

nvmem-cells = <&nvmem_device SERIAL_NUMBER>;
nvmem-cells = <&nvmem_device BASE_MAC_ADDRESS 1>;

> > Now comes the catch: what if there is no actual description of the
> > cell in the device tree, but is is generated during runtime. How
> > can I get a label to it.
>
> Same as clocks, resets, power-domains and everyone else.

See
https://git.kernel.org/torvalds/c/084973e944bec21804f8afb0515b25434438699a

And I guess this discussion is relevant here:
https://lore.kernel.org/linux-devicetree/20220124160300.25131-1-zajec5@xxxxxxxxx/

> > Therefore, in this case, there is just
> > an empty node and the driver will associate it with the cell
> > created during runtime (see patch 10). It is not expected, that
> > is has any properties.
>
> It cannot be even referenced as it does not have #cells property...

You mean "#nvmem-cell-cells"? See patch #2. None of the nvmem
cells had such a property for now.

> > > > +
> > > > +  base-mac-address:
> > >
> > > Fields should be rather described here, not in top-level description.
> > >
> > > > +    type: object
> > >
> > > On this level:
> > >     additionalProperties: false
> > >
> > > > +
> > > > +    properties:
> > > > +      "#nvmem-cell-cells":
> > > > +        const: 1
> > > > +
> > >
> > > I also wonder why you do not have unit addresses. What if you want to
> > > have two base MAC addresses?
> >
> > That would describe an offset within the nvmem device. But the offset
> > might not be constant, depending on the content. My understanding
> > so far was that in that case, you use the "-N" suffix.
> >
> > base-mac-address-1
> > base-mac-address-2
> >
> > (or maybe completely different names).
>
> You do not allow "base-mac-address-1". Your binding explicitly accepts
> only "base-mac-address".

Because the binding matches the driver, which matches the driver
which matches the VPD data and there is only one base mac address.
Thus, no need for different ones.

-michael