Re: [PATCH v3 2/2] ASoC: dt-bindings: renesas,rsnd.yaml: add R-Car Gen4 support

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

On 16/02/2023 02:09, Kuninori Morimoto wrote:
> 
> Hi Krzysztof
> 
>> If you leave the top-level definition without any constraints and you
>> forget to include all your compatibles in allOf:if:then, then you end up
>> without constraints. Consider:
> (snip)
>> -----
>> properties:
>>   compatible:
>>     enum:
>>       - foo
>>       - bar
>>
>> clock-names:
>>   description: anything
>>
>> if:
>>   prop:
>>     compat:
>>       enum:
>>         - foo
>> then:
>>   prop:
>>     clock-names:
>>       - ahb
>>       - sclk
>> -----
>>
>> What clocks are valid for bar?
>>
>> For simple cases this might be obvious, for more complex this is easy to
>> miss. So the recommended syntax is with constraints at the top.
> 
> I can understand we want to avoid the future miss.
> But I did it on v2 patch and you NACKed it.

No, you did not do it in v2. The top-level property is a must, we talk
now about constraints.

> Thus people confused. That is the reason why above strange style was created.
> And it is already using "else", your concern never happen ?

Yes, with else my concern will never happen. However you have there
multiple ifs, thus finding the one related to clocks is not obvious now
and won't be anyhow easier later. What's more, clocks have constraints
in top-level, thus seeing clock-names without the constraints also
raises reviewers question. Someone might bring a new compatible with
another set of clocks (quite likely), thus else won't be valid anymore
and you will have to define constraints per *each* variant (each
if:then:). When this happens, please move the widest constraints to
top-level, just like I was asking since some time. Will you remember to
do this? I might not because I assume we follow same pattern everywhere.

With a promise of above:

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>


Best regards,
Krzysztof