Re: [PATCH v8 3/3] dt-bindings: mtd: marvell-nand: Convert to YAML DT scheme

[Miquel Raynal]

Hi Krzysztof,

krzysztof.kozlowski@xxxxxxxxxx wrote on Tue, 6 Jun 2023 12:40:45 +0200:

> On 06/06/2023 12:37, Krzysztof Kozlowski wrote:
> > On 06/06/2023 12:28, Miquel Raynal wrote:  
> >> Hi Krzysztof,
> >>
> >> krzysztof.kozlowski@xxxxxxxxxx wrote on Tue, 6 Jun 2023 10:44:34 +0200:
> >>  
> >>> On 06/06/2023 09:48, Miquel Raynal wrote:  
> >>>>>>>>>> +          it (otherwise it is harmless).
> >>>>>>>>>> +        $ref: /schemas/types.yaml#/definitions/flag
> >>>>>>>>>> +        deprecated: true
> >>>>>>>>>> +
> >>>>>>>>>> +    additionalProperties: false      
> >>>>>>>>> unevaluatedProperties: false      
> >>>>>>>> It was hiding by '"^nand@[0-3]$":'. Should I move it here?      
> >>>>>>> You cannot have both additionalProps and unevaluatedProps at the same
> >>>>>>> time, so we do not talk about same thing or this was never working?      
> >>>>>>
> >>>>>> Hmm, I'm a little confused then. At various times I've been told to 
> >>>>>> put 'additionalProperties: false' or 'unevaluatedProperties: false' 
> >>>>>> (although never at the same time). I'm not sure when to use one or the 
> >>>>>> other.
> >>>>>>
> >>>>>> From what I've been able to glean 'additionalProperties: true' 
> >>>>>> indicates that the node is expected to have child nodes defined in a 
> >>>>>> different schema so I would have thought 'additionalProperties: false' 
> >>>>>> would be appropriate for a schema covering a leaf node. 
> >>>>>> 'unevaluatedProperties: false' seems to enable stricter checking which 
> >>>>>> makes sense when all the properties are described in the schema.      
> >>>>>
> >>>>> So I think this might be the problem. If I look at qcom,nandc.yaml or 
> >>>>> ingenic,nand.yaml which both have a partitions property in their 
> >>>>> example. Neither have 'unevaluatedProperties: false' on the nand@... 
> >>>>> subnode. If I add it sure enough I start getting complaints about the 
> >>>>> 'partitions' node being unexpected.    
> >>>>
> >>>> Sorry if that was unclear, I think the whole logic around the yaml
> >>>> files is to progressively constrain the descriptions, schema after
> >>>> schema. IOW, in the marvell binding you should set
> >>>> unevaluatedProperties: false for the NAND controller. What is inside
> >>>> (NAND chips, partition container, partition parsers, "mtd" properties,
> >>>> etc) will be handled by other files. Of course you can constrain a bit
> >>>> what can/cannot be used inside these subnodes, but I think you don't
> >>>> need to set unevaluatedProperties in these subnodes (the NAND chip in
> >>>> this case, or even the partitions) because you already reference
> >>>> nand-controller.yaml which references nand-chip.yaml, mtd.yaml,
> >>>> partitions.yaml, etc. *they* will make the generic checks and hopefully
> >>>> apply stricter checks, when deemed relevant.    
> >>>
> >>> No, neither nand-controller.yaml nor nand-chip.yaml limit the properties
> >>> in this context, so each device schema must have unevaluatedProperties:
> >>> false, for which I asked few emails ago.  
> >>
> >> The controller description shall be guarded by unevaluatedProperties:
> >> false, we agree. Do you mean the nand chip description in each nand
> >> controller binding should also include it at its own level? Because
> >> that is not what we enforced so far IIRC. I am totally fine doing so
> >> starting from now on if this is a new requirement (which makes sense).
> >>
> >> If yes, then it means we would need to list *all* the nand
> >> chip properties in each schema, which clearly involves a lot of
> >> duplication as you would need to define all types of partitions,
> >> partition parsers, generic properties, etc in order for the examples to
> >> pass all the checks. Only the properties like pinctrl-* would not need
> >> to be listed I guess.  
> > 
> > Yes, this is what should be done. Each node should have either  
> 
> Eh, no, I responded in wrong part of message. My yes was for:
> 
> " Do you mean the nand chip description in each nand
> controller binding should also include it at its own level?"

Clear.

> 
> Now for actual paragraph:
> 
> "If yes, then it means we would need to list *all* the nand chip
> properties in each schema,"
> 
> No, why? I don't understand. Use the same pattern as all other bindings,
> this is not special. Absolutely all have the same behavior, e.g.
> mentioned leds. You finish with unevaluatedProps and you're done, which
> is what I wrote here long, long time ago.

Maybe because so far we did not bother referencing another schema in
the NAND chip nodes? For your hint to work I guess we should have, in
each controller binding, something along:

 patternProperties:
   "^nand@[a-f0-9]$":
     type: object
+    $ref: nand-chip.yaml#
     properties:

If yes, please ignore the series sent aside, I will work on it again
and send a v2.

Thanks,
Miquèl