| Message ID | 87sffa8g99.wl-kuninori.morimoto.gx@renesas.com |
|---|---|
| State | Accepted |
| Commit | 7f8b5b24bbb463614dd6b797e6b2ee92b3e2a6a1 |
| Headers | show |
| Series | ASoC: dt-bindings: renesas,rsnd.yaml: add R-Car Gen4 support | expand |
On 13/02/2023 03:13, Kuninori Morimoto wrote: > From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> > > There are no compatible for "reg/reg-names" and "clock-name" > between previous R-Car series and R-Car Gen4. > > "reg/reg-names" needs 3 categorize (for Gen1, for Gen2/Gen3, for Gen4), > therefore, use 3 if-then to avoid nested if-then-else. > > Move "clock-name" detail properties to under allOf to use if-then-else > for Gen4 or others. > > Link: https://lore.kernel.org/all/87zg9vk0ex.wl-kuninori.morimoto.gx@renesas.com/#r > Link: https://lore.kernel.org/all/87r0v2uvm7.wl-kuninori.morimoto.gx@renesas.com/#r > Link: https://lore.kernel.org/all/87r0v1t02h.wl-kuninori.morimoto.gx@renesas.com/#r > Link: https://lore.kernel.org/all/87y1p7bpma.wl-kuninori.morimoto.gx@renesas.com/#r > Reported-by: Geert Uytterhoeven <geert+renesas@glider.be> > Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> > --- > .../bindings/sound/renesas,rsnd.yaml | 76 ++++++++++++++++--- > 1 file changed, 64 insertions(+), 12 deletions(-) > > diff --git a/Documentation/devicetree/bindings/sound/renesas,rsnd.yaml b/Documentation/devicetree/bindings/sound/renesas,rsnd.yaml > index 12ccf29338d9..55e5213b90a1 100644 > --- a/Documentation/devicetree/bindings/sound/renesas,rsnd.yaml > +++ b/Documentation/devicetree/bindings/sound/renesas,rsnd.yaml > @@ -101,17 +101,7 @@ properties: > > clock-names: > description: List of necessary clock names. > - minItems: 1 > - maxItems: 31 Not much of an improvement here. We asked to keep the constraints here. I gave you the reference how it should look like. Why did you decide to do it differently than what I linked? Best regards, Krzysztof
On Mon, Feb 13, 2023 at 09:58:15AM +0100, Krzysztof Kozlowski wrote: > On 13/02/2023 03:13, Kuninori Morimoto wrote: > > clock-names: > > description: List of necessary clock names. > > - minItems: 1 > > - maxItems: 31 > Not much of an improvement here. We asked to keep the constraints here. > I gave you the reference how it should look like. Why did you decide to > do it differently than what I linked? Krzysztof, please take more time to explain what issues you're seeing, what you want people to do and why. I'm having a hard time identifying what the issue is here - AFAICT when you talk about the example you linked you're referring to: https://elixir.bootlin.com/linux/v5.19-rc6/source/Documentation/devicetree/bindings/clock/samsung,exynos7-clock.yaml#L57 which as far as I can tell looks like what Morimoto-san is trying to accomplish here. I can't tell what the issue you're raising is, let alone if it's something important or just a stylistic nit.
On 14/02/2023 18:52, Mark Brown wrote: > On Mon, Feb 13, 2023 at 09:58:15AM +0100, Krzysztof Kozlowski wrote: >> On 13/02/2023 03:13, Kuninori Morimoto wrote: > >>> clock-names: >>> description: List of necessary clock names. >>> - minItems: 1 >>> - maxItems: 31 > >> Not much of an improvement here. We asked to keep the constraints here. >> I gave you the reference how it should look like. Why did you decide to >> do it differently than what I linked? > > Krzysztof, please take more time to explain what issues you're > seeing, what you want people to do and why. I'm having a hard > time identifying what the issue is here - AFAICT when you talk > about the example you linked you're referring to: > > https://elixir.bootlin.com/linux/v5.19-rc6/source/Documentation/devicetree/bindings/clock/samsung,exynos7-clock.yaml#L57 > > which as far as I can tell looks like what Morimoto-san is trying > to accomplish here. I can't tell what the issue you're raising > is, let alone if it's something important or just a stylistic > nit. 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: ----- 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. BTW, if there is reason not to use it - sure, bring the reason, we talk and maybe skip it, I don't mind. But there was no reason - just part was skipped/missing. Best regards, Krzysztof
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. Thus people confused. That is the reason why above strange style was created. And it is already using "else", your concern never happen ? Thank you for your help !! Best regards --- Kuninori Morimoto
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
Hi Krzysztof > 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:). Do you mean you want to tell was keeping minItems/maxItems on top ?? I think I could understand what you want to tell if your indicated link was pointing to "clock-name" line, and/or if you indicated like "please keep minItems/maxItems on top for constraints" or something. But pointed link was to "allOf:" line with very unclear comment, and no response to my question mail. And your words of "constraints". I have been thoughting that you are indicating was "if-then-else" need to catch all "compatible" (but don't use "else if"). It is using "if-then-else", and "else" has minItems/maxItems, I think it is there is no difference. But if my above assumption was correct and Krzysztof agreed to it, I will post v4 patch which keeps min/maxItems on top. Otherwise I will do nothing to avoid endless pointless ping-pong, becuase it already got Reviewed-by. To avoid pointless ping-pong, I think v4 will be like this ---------- clock-names: description: List of necessary clock names. minItems: 1 maxItems: 31 ... - if: properties: compatible: contains: const: renesas,rcar_sound-gen4 then: properties: clock-names: maxItems: 3 ... else properties: clock-names: ... ------------- Thank you for your help !! Best regards --- Kuninori Morimoto
On 13.02.2023 03:13, Kuninori Morimoto wrote: > From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> > > There are no compatible for "reg/reg-names" and "clock-name" > between previous R-Car series and R-Car Gen4. > > "reg/reg-names" needs 3 categorize (for Gen1, for Gen2/Gen3, for Gen4), > therefore, use 3 if-then to avoid nested if-then-else. > > Move "clock-name" detail properties to under allOf to use if-then-else > for Gen4 or others. Hi, this patch seems to add errors for me. Are my tools outdated or is it a real issue? See below. > Link: https://lore.kernel.org/all/87zg9vk0ex.wl-kuninori.morimoto.gx@renesas.com/#r > Link: https://lore.kernel.org/all/87r0v2uvm7.wl-kuninori.morimoto.gx@renesas.com/#r > Link: https://lore.kernel.org/all/87r0v1t02h.wl-kuninori.morimoto.gx@renesas.com/#r > Link: https://lore.kernel.org/all/87y1p7bpma.wl-kuninori.morimoto.gx@renesas.com/#r > Reported-by: Geert Uytterhoeven <geert+renesas@glider.be> > Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> > --- > .../bindings/sound/renesas,rsnd.yaml | 76 ++++++++++++++++--- > 1 file changed, 64 insertions(+), 12 deletions(-) > > diff --git a/Documentation/devicetree/bindings/sound/renesas,rsnd.yaml b/Documentation/devicetree/bindings/sound/renesas,rsnd.yaml > index 12ccf29338d9..55e5213b90a1 100644 > --- a/Documentation/devicetree/bindings/sound/renesas,rsnd.yaml > +++ b/Documentation/devicetree/bindings/sound/renesas,rsnd.yaml > @@ -101,17 +101,7 @@ properties: > > clock-names: > description: List of necessary clock names. > - minItems: 1 > - maxItems: 31 > - items: > - oneOf: > - - const: ssi-all > - - pattern: '^ssi\.[0-9]$' > - - pattern: '^src\.[0-9]$' > - - pattern: '^mix\.[0-1]$' > - - pattern: '^ctu\.[0-1]$' > - - pattern: '^dvc\.[0-1]$' > - - pattern: '^clk_(a|b|c|i)$' > + # details are defined below > > ports: > $ref: audio-graph-port.yaml#/definitions/port-base > @@ -288,6 +278,11 @@ required: > > allOf: > - $ref: dai-common.yaml# > + > + #-------------------- > + # reg/reg-names > + #-------------------- > + # for Gen1 This seems to cause: ./Documentation/devicetree/bindings/sound/renesas,rsnd.yaml:282:4: [error] missing starting space in comment (comments) ./Documentation/devicetree/bindings/sound/renesas,rsnd.yaml:284:4: [error] missing starting space in comment (comments) ./Documentation/devicetree/bindings/sound/renesas,rsnd.yaml:339:4: [error] missing starting space in comment (comments) ./Documentation/devicetree/bindings/sound/renesas,rsnd.yaml:341:4: [error] missing starting space in comment (comments) > - if: > properties: > compatible: > @@ -303,7 +298,15 @@ allOf: > - scu > - ssi > - adg > - else: > + # for Gen2/Gen3 > + - if: > + properties: > + compatible: > + contains: > + enum: > + - renesas,rcar_sound-gen2 > + - renesas,rcar_sound-gen3 > + then: > properties: > reg: > minItems: 5 > @@ -315,6 +318,55 @@ allOf: > - ssiu > - ssi > - audmapp > + # for Gen4 > + - if: > + properties: > + compatible: > + contains: > + const: renesas,rcar_sound-gen4 > + then: > + properties: > + reg: > + maxItems: 4 > + reg-names: > + items: > + enum: > + - adg > + - ssiu > + - ssi > + - sdmc > + > + #-------------------- > + # clock-names > + #-------------------- > + - if: > + properties: > + compatible: > + contains: > + const: renesas,rcar_sound-gen4 > + then: > + properties: > + clock-names: > + maxItems: 3 > + items: > + enum: > + - ssi.0 > + - ssiu.0 > + - clkin > + else: > + properties: > + clock-names: > + minItems: 1 > + maxItems: 31 > + items: > + oneOf: > + - const: ssi-all > + - pattern: '^ssi\.[0-9]$' > + - pattern: '^src\.[0-9]$' > + - pattern: '^mix\.[0-1]$' > + - pattern: '^ctu\.[0-1]$' > + - pattern: '^dvc\.[0-1]$' > + - pattern: '^clk_(a|b|c|i)$' > > unevaluatedProperties: false >
Hi Rafał > Hi, this patch seems to add errors for me. Are my tools outdated or is > it a real issue? See below. (snip) > > + #-------------------- > > + # reg/reg-names > > + #-------------------- > > + # for Gen1 > > This seems to cause: > > ./Documentation/devicetree/bindings/sound/renesas,rsnd.yaml:282:4: [error] missing starting space in comment (comments) > ./Documentation/devicetree/bindings/sound/renesas,rsnd.yaml:284:4: [error] missing starting space in comment (comments) > ./Documentation/devicetree/bindings/sound/renesas,rsnd.yaml:339:4: [error] missing starting space in comment (comments) > ./Documentation/devicetree/bindings/sound/renesas,rsnd.yaml:341:4: [error] missing starting space in comment (comments) Hmm... I couldn't reproduce this Thank you for your help !! Best regards --- Kuninori Morimoto
diff --git a/Documentation/devicetree/bindings/sound/renesas,rsnd.yaml b/Documentation/devicetree/bindings/sound/renesas,rsnd.yaml index 12ccf29338d9..55e5213b90a1 100644 --- a/Documentation/devicetree/bindings/sound/renesas,rsnd.yaml +++ b/Documentation/devicetree/bindings/sound/renesas,rsnd.yaml @@ -101,17 +101,7 @@ properties: clock-names: description: List of necessary clock names. - minItems: 1 - maxItems: 31 - items: - oneOf: - - const: ssi-all - - pattern: '^ssi\.[0-9]$' - - pattern: '^src\.[0-9]$' - - pattern: '^mix\.[0-1]$' - - pattern: '^ctu\.[0-1]$' - - pattern: '^dvc\.[0-1]$' - - pattern: '^clk_(a|b|c|i)$' + # details are defined below ports: $ref: audio-graph-port.yaml#/definitions/port-base @@ -288,6 +278,11 @@ required: allOf: - $ref: dai-common.yaml# + + #-------------------- + # reg/reg-names + #-------------------- + # for Gen1 - if: properties: compatible: @@ -303,7 +298,15 @@ allOf: - scu - ssi - adg - else: + # for Gen2/Gen3 + - if: + properties: + compatible: + contains: + enum: + - renesas,rcar_sound-gen2 + - renesas,rcar_sound-gen3 + then: properties: reg: minItems: 5 @@ -315,6 +318,55 @@ allOf: - ssiu - ssi - audmapp + # for Gen4 + - if: + properties: + compatible: + contains: + const: renesas,rcar_sound-gen4 + then: + properties: + reg: + maxItems: 4 + reg-names: + items: + enum: + - adg + - ssiu + - ssi + - sdmc + + #-------------------- + # clock-names + #-------------------- + - if: + properties: + compatible: + contains: + const: renesas,rcar_sound-gen4 + then: + properties: + clock-names: + maxItems: 3 + items: + enum: + - ssi.0 + - ssiu.0 + - clkin + else: + properties: + clock-names: + minItems: 1 + maxItems: 31 + items: + oneOf: + - const: ssi-all + - pattern: '^ssi\.[0-9]$' + - pattern: '^src\.[0-9]$' + - pattern: '^mix\.[0-1]$' + - pattern: '^ctu\.[0-1]$' + - pattern: '^dvc\.[0-1]$' + - pattern: '^clk_(a|b|c|i)$' unevaluatedProperties: false