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

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 06/06/2023 13:07, Miquel Raynal wrote:
> Hi Krzysztof,
> 
> miquel.raynal@bootlin.com wrote on Tue, 6 Jun 2023 12:57:24 +0200:
> 
>> Hi Krzysztof,
>>
>> krzysztof.kozlowski@linaro.org 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@linaro.org 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.
> 
> Actually I already see a problem, let's the ingenic,nand.yaml example.
> The goal, IIUC, is to do:
> 
>  patternProperties:
>    "^nand@[a-f0-9]$":
>      type: object
> +    $ref: nand-chip.yaml
>      properties:
> 
>        ...
> 
> +    unevaluatedProperties: false
> 
> The example in this file uses a property, nand-on-flash-bbt, which is
> described inside nand-controller.yaml instead of nand-chip.yaml.
> Indeed, the former actually describes many properties which are a bit
> more controller related than chip related. With the above description,
> the example fails because nand-on-flash-bbt is not allowed (it is not
> listed in nand-chip.yaml).
> 
> How would you proceed in this case?
> 
> Maybe I could move all the NAND chip properties which are somehow
> related to NAND controllers (and defined in nand-controller.yaml) in a
> dedicated file and reference it from nand-chip.yaml? Any other idea is
> welcome.

Yes, this would work and seems reasonable. Other way could be to add
unevaluatedProperties:false on this level (so after ref:nand-chip.yaml)
in nand-controller.yaml. This however would not allow any new properties
to be defined in device bindings.

Best regards,
Krzysztof