| Message ID | 20240818172937.121928-1-krzysztof.kozlowski@linaro.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] media: dt-bindings: renesas,fcp: add top-level constraints | expand |
Hi Krzysztof, Thank you for the patch. On Sun, Aug 18, 2024 at 07:29:36PM +0200, Krzysztof Kozlowski wrote: > Properties with variable number of items per each device are expected to > have widest constraints in top-level "properties:" block and further > customized (narrowed) in "if:then:". Add missing top-level constraints > for clocks and clock-names. In this specific case I think it's fine, but generally speaking, how do you handle that rule when different variants have completely different clocks, not just lack some of the clocks ? > Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > --- > .../devicetree/bindings/media/renesas,fcp.yaml | 15 ++++++++------- > 1 file changed, 8 insertions(+), 7 deletions(-) > > diff --git a/Documentation/devicetree/bindings/media/renesas,fcp.yaml b/Documentation/devicetree/bindings/media/renesas,fcp.yaml > index c6abe719881b..68c088821b22 100644 > --- a/Documentation/devicetree/bindings/media/renesas,fcp.yaml > +++ b/Documentation/devicetree/bindings/media/renesas,fcp.yaml > @@ -34,9 +34,15 @@ properties: > reg: > maxItems: 1 > > - clocks: true > + clocks: > + minItems: 1 > + maxItems: 3 > > - clock-names: true > + clock-names: > + items: > + - const: aclk > + - const: pclk > + - const: vclk > > iommus: > maxItems: 1 > @@ -71,11 +77,6 @@ allOf: > - description: Main clock > - description: Register access clock > - description: Video clock > - clock-names: > - items: > - - const: aclk > - - const: pclk > - - const: vclk Any specific reason to move the clock names but not the descriptions ? The assymetry bothers me. > required: > - clock-names > else:
On 18/08/2024 19:37, Laurent Pinchart wrote: > Hi Krzysztof, > > Thank you for the patch. > > On Sun, Aug 18, 2024 at 07:29:36PM +0200, Krzysztof Kozlowski wrote: >> Properties with variable number of items per each device are expected to >> have widest constraints in top-level "properties:" block and further >> customized (narrowed) in "if:then:". Add missing top-level constraints >> for clocks and clock-names. > > In this specific case I think it's fine, but generally speaking, how do > you handle that rule when different variants have completely different > clocks, not just lack some of the clocks ? I don't understand the problem. We handle it as usual, as in all bindings. Here there is no such case, thus names go to the top. >> >> - clock-names: true >> + clock-names: >> + items: >> + - const: aclk >> + - const: pclk >> + - const: vclk >> >> iommus: >> maxItems: 1 >> @@ -71,11 +77,6 @@ allOf: >> - description: Main clock >> - description: Register access clock >> - description: Video clock >> - clock-names: >> - items: >> - - const: aclk >> - - const: pclk >> - - const: vclk > > Any specific reason to move the clock names but not the descriptions ? > The assymetry bothers me. The other variant does not have description of the first clock, so moving it would be incorrect. Moving names is correct, because other variant does not have clock-names at all. Best regards, Krzysztof
On Sun, Aug 18, 2024 at 07:45:55PM +0200, Krzysztof Kozlowski wrote: > On 18/08/2024 19:37, Laurent Pinchart wrote: > > On Sun, Aug 18, 2024 at 07:29:36PM +0200, Krzysztof Kozlowski wrote: > >> Properties with variable number of items per each device are expected to > >> have widest constraints in top-level "properties:" block and further > >> customized (narrowed) in "if:then:". Add missing top-level constraints > >> for clocks and clock-names. > > > > In this specific case I think it's fine, but generally speaking, how do > > you handle that rule when different variants have completely different > > clocks, not just lack some of the clocks ? > > I don't understand the problem. We handle it as usual, as in all > bindings. Here there is no such case, thus names go to the top. That answers the question, the clock names would still be variant-specific in that case. While the change here won't cause validation failures, I think it's confusing to define the clock names at the top level, knowing they don't apply to some of the variants, if we don't also define the description there. I'd move either both or neither. > >> > >> - clock-names: true > >> + clock-names: > >> + items: > >> + - const: aclk > >> + - const: pclk > >> + - const: vclk > >> > >> iommus: > >> maxItems: 1 > >> @@ -71,11 +77,6 @@ allOf: > >> - description: Main clock > >> - description: Register access clock > >> - description: Video clock > >> - clock-names: > >> - items: > >> - - const: aclk > >> - - const: pclk > >> - - const: vclk > > > > Any specific reason to move the clock names but not the descriptions ? > > The assymetry bothers me. > > The other variant does not have description of the first clock, so > moving it would be incorrect. Moving names is correct, because other > variant does not have clock-names at all. I don't think it's incorrect, when the FCP has a single clock, it's the main clock.
On 18/08/2024 19:50, Laurent Pinchart wrote: > On Sun, Aug 18, 2024 at 07:45:55PM +0200, Krzysztof Kozlowski wrote: >> On 18/08/2024 19:37, Laurent Pinchart wrote: >>> On Sun, Aug 18, 2024 at 07:29:36PM +0200, Krzysztof Kozlowski wrote: >>>> Properties with variable number of items per each device are expected to >>>> have widest constraints in top-level "properties:" block and further >>>> customized (narrowed) in "if:then:". Add missing top-level constraints >>>> for clocks and clock-names. >>> >>> In this specific case I think it's fine, but generally speaking, how do >>> you handle that rule when different variants have completely different >>> clocks, not just lack some of the clocks ? >> >> I don't understand the problem. We handle it as usual, as in all >> bindings. Here there is no such case, thus names go to the top. > > That answers the question, the clock names would still be > variant-specific in that case. > > While the change here won't cause validation failures, I think it's > confusing to define the clock names at the top level, knowing they don't > apply to some of the variants, if we don't also define the description > there. I'd move either both or neither. First, they apply to ALL variants using clock-names. Second, we want such lists, like clocks/resets/interrupts, to share as much as possible between variants, e.g. keep the same order. Having clock-names listed at top-level encourages this and prevents people from adding new binding with: "vclk", "aclk", "pclk", "new_clock_but_i_want_to_mess_order_of_everything_because_i_can" > >>>> >>>> - clock-names: true >>>> + clock-names: >>>> + items: >>>> + - const: aclk >>>> + - const: pclk >>>> + - const: vclk >>>> >>>> iommus: >>>> maxItems: 1 >>>> @@ -71,11 +77,6 @@ allOf: >>>> - description: Main clock >>>> - description: Register access clock >>>> - description: Video clock >>>> - clock-names: >>>> - items: >>>> - - const: aclk >>>> - - const: pclk >>>> - - const: vclk >>> >>> Any specific reason to move the clock names but not the descriptions ? >>> The assymetry bothers me. >> >> The other variant does not have description of the first clock, so >> moving it would be incorrect. Moving names is correct, because other >> variant does not have clock-names at all. > > I don't think it's incorrect, when the FCP has a single clock, it's the > main clock. Could be main clock, could be something else for me - I did not investigate enough. If it is main clock, I will move the description as well. Best regards, Krzysztof
On Sun, Aug 18, 2024 at 07:29:36PM +0200, Krzysztof Kozlowski wrote: > Properties with variable number of items per each device are expected to > have widest constraints in top-level "properties:" block and further > customized (narrowed) in "if:then:". Add missing top-level constraints > for clocks and clock-names. > > Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Acked-by: Conor Dooley <conor.dooley@microchip.com>
diff --git a/Documentation/devicetree/bindings/media/renesas,fcp.yaml b/Documentation/devicetree/bindings/media/renesas,fcp.yaml index c6abe719881b..68c088821b22 100644 --- a/Documentation/devicetree/bindings/media/renesas,fcp.yaml +++ b/Documentation/devicetree/bindings/media/renesas,fcp.yaml @@ -34,9 +34,15 @@ properties: reg: maxItems: 1 - clocks: true + clocks: + minItems: 1 + maxItems: 3 - clock-names: true + clock-names: + items: + - const: aclk + - const: pclk + - const: vclk iommus: maxItems: 1 @@ -71,11 +77,6 @@ allOf: - description: Main clock - description: Register access clock - description: Video clock - clock-names: - items: - - const: aclk - - const: pclk - - const: vclk required: - clock-names else:
Properties with variable number of items per each device are expected to have widest constraints in top-level "properties:" block and further customized (narrowed) in "if:then:". Add missing top-level constraints for clocks and clock-names. Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> --- .../devicetree/bindings/media/renesas,fcp.yaml | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-)