Re: [PATCH v1] dt-bindings: mtd: spi-nor: use unevaluatedProperties: false

[Pratyush Yadav <p.yadav@ti.com>]

On 04/10/21 12:09PM, Rob Herring wrote:
> On Fri, Sep 24, 2021 at 11:37:05PM +0530, Pratyush Yadav wrote:
> > Many SPI controllers need to add properties to slave devices. This could
> > be the delay in clock or data lines, etc. These properties are
> > controller specific but need to be defined in the slave node because
> > they are per-slave and there can be multiple slaves attached to a
> > controller.
> > 
> > If these properties are not added to the slave binding, then the dtbs
> > check emits a warning. But these properties do not make much sense in
> > the slave binding because they are controller-specific and they will
> > just pollute every slave binding.
> > 
> > One option is to add a separate schema that collects all such properties
> > from all such controllers. Slave bindings can simply refer to this
> > binding and they should be rid of the warnings.
> > 
> > There are some limitations with this approach:
> > 
> > 1. There is no way to specify required properties. The schema would
> > contain properties for all controllers and there would be no way to know
> > which controller is being used.
> > 
> > 2. There would be no way to restrict additional properties. Since this
> > schema will be used with an allOf operator, additionalProperties would
> > need to be true. In addition, the slave schema will have to set
> > unevaluatedProperties: false.
> 
> I don't see what is the problem. If there's a $ref, then 
> unevaluatedProperties is what should be used.
> 
> > 
> > A much simpler option would be to make controller schemas specify those
> > properties in patternProperties and set unevaluatedProperties to false
> > on slave schemas, which is done in the previous approach anyway. This
> > approach would have the same limitations as the 2nd limitation in the
> > previous approach. But it does not have the 1st limitation since the
> > properties of all controllers are not collected in a single schema, but
> > instead reside in the same schema as the controller. It also has the
> > added benefit of being much simpler.
> > 
> > The SPI NOR binding is taken as the first one to follow this. Other
> > bindings like SPI NAND can follow in later patches.
> > 
> > Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
> > 
> > ---
> > I sent the first approach mentioned in the commit message some time ago
> > [0]. When re-rolling this series I realized that if we are going to use
> > unevaluatedProperties: false, then it would be much simpler to just keep
> > everything else as-is. This has clear positives with no negatives
> > relative to [0], as explained in the commit message.
> > 
> > [0] https://lore.kernel.org/all/20210609111707.9555-1-p.yadav@ti.com/T/#u
> > 
> >  Documentation/devicetree/bindings/mtd/jedec,spi-nor.yaml | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/mtd/jedec,spi-nor.yaml b/Documentation/devicetree/bindings/mtd/jedec,spi-nor.yaml
> > index ed590d7c6e37..81be0620b264 100644
> > --- a/Documentation/devicetree/bindings/mtd/jedec,spi-nor.yaml
> > +++ b/Documentation/devicetree/bindings/mtd/jedec,spi-nor.yaml
> > @@ -88,7 +88,7 @@ patternProperties:
> >    "^otp(-[0-9]+)?$":
> >      type: object
> > 
> > -additionalProperties: false
> > +unevaluatedProperties: false
> 
> This only works until unevaluatedProperties support is actually 
> implemented. Then it's back to the same warnings. In the mean time, we'd 
> be allowing any extra random properties to be added for everyone.

Ok, I didn't know that. I don't understand the validation frameworks all 
that well. I will go back to the method you suggested. Thanks.

> 
> Rob

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.