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

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.

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(-)

--
2.33.0

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.

Rob

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