| Message ID | 20231213195125.212923-2-knaerzche@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Add HDMI support for RK3128 | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | fail | build log |
On 13/12/2023 20:51, Alex Bee wrote: > Document the compatible for RK3128's HDMI controller block. > The integration for this SoC is somewhat different here: It needs the PHY's Please wrap commit message according to Linux coding style / submission process (neither too early nor over the limit): https://elixir.bootlin.com/linux/v6.4-rc1/source/Documentation/process/submitting-patches.rst#L597 > reference clock rate to calculate the ddc bus frequency correctly. This > clock is part of a power-domain (PD_VIO), so this gets added as an optional > property too. If clock is part of power domain, then the power domain must be in the clock controller, not here. So either you put power domain in wrong place or you used incorrect reason for a change. > > Signed-off-by: Alex Bee <knaerzche@gmail.com> > --- > .../display/rockchip/rockchip,inno-hdmi.yaml | 30 +++++++++++++++++-- > 1 file changed, 28 insertions(+), 2 deletions(-) > > diff --git a/Documentation/devicetree/bindings/display/rockchip/rockchip,inno-hdmi.yaml b/Documentation/devicetree/bindings/display/rockchip/rockchip,inno-hdmi.yaml > index 96889c86849a..9f00abcbfb38 100644 > --- a/Documentation/devicetree/bindings/display/rockchip/rockchip,inno-hdmi.yaml > +++ b/Documentation/devicetree/bindings/display/rockchip/rockchip,inno-hdmi.yaml > @@ -14,6 +14,7 @@ properties: > compatible: > enum: > - rockchip,rk3036-inno-hdmi > + - rockchip,rk3128-inno-hdmi > > reg: > maxItems: 1 > @@ -22,10 +23,21 @@ properties: > maxItems: 1 > > clocks: > - maxItems: 1 > + minItems: 1 > + items: > + - description: The HDMI controller main clock > + - description: The HDMI PHY reference clock > > clock-names: > - const: pclk > + minItems: 1 > + items: > + - const: pclk > + - enum: > + - pclk > + - ref That's way overcomplicated. Just items listing the names and minItems: 1. See other bindings how this is done. > + > + power-domains: > + maxItems: 1 Is it relevant to existing device? > > ports: > $ref: /schemas/graph.yaml#/properties/ports > @@ -55,6 +67,20 @@ required: > - pinctrl-names > - ports Best regards, Krzysztof
Am 14.12.23 um 08:53 schrieb Krzysztof Kozlowski: > On 13/12/2023 20:51, Alex Bee wrote: >> Document the compatible for RK3128's HDMI controller block. >> The integration for this SoC is somewhat different here: It needs the PHY's > Please wrap commit message according to Linux coding style / submission > process (neither too early nor over the limit): > https://elixir.bootlin.com/linux/v6.4-rc1/source/Documentation/process/submitting-patches.rst#L597 OK. Not sure why checkpatch --strict didn't tell me that I'm over the limit here. > >> reference clock rate to calculate the ddc bus frequency correctly. This >> clock is part of a power-domain (PD_VIO), so this gets added as an optional >> property too. > If clock is part of power domain, then the power domain must be in the > clock controller, not here. So either you put power domain in wrong > place or you used incorrect reason for a change. Rockchip defines it's powerdomains per clock and I was little to much in that world when writing this. Actually the controller itself is part of the powerdomain. Will rephrase. >> Signed-off-by: Alex Bee <knaerzche@gmail.com> >> --- >> .../display/rockchip/rockchip,inno-hdmi.yaml | 30 +++++++++++++++++-- >> 1 file changed, 28 insertions(+), 2 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/display/rockchip/rockchip,inno-hdmi.yaml b/Documentation/devicetree/bindings/display/rockchip/rockchip,inno-hdmi.yaml >> index 96889c86849a..9f00abcbfb38 100644 >> --- a/Documentation/devicetree/bindings/display/rockchip/rockchip,inno-hdmi.yaml >> +++ b/Documentation/devicetree/bindings/display/rockchip/rockchip,inno-hdmi.yaml >> @@ -14,6 +14,7 @@ properties: >> compatible: >> enum: >> - rockchip,rk3036-inno-hdmi >> + - rockchip,rk3128-inno-hdmi >> >> reg: >> maxItems: 1 >> @@ -22,10 +23,21 @@ properties: >> maxItems: 1 >> >> clocks: >> - maxItems: 1 >> + minItems: 1 >> + items: >> + - description: The HDMI controller main clock >> + - description: The HDMI PHY reference clock >> >> clock-names: >> - const: pclk >> + minItems: 1 >> + items: >> + - const: pclk >> + - enum: >> + - pclk >> + - ref > That's way overcomplicated. Just items listing the names and minItems: > 1. See other bindings how this is done. OK. >> + >> + power-domains: >> + maxItems: 1 > Is it relevant to existing device? Will move to new variant only. Alex >> >> ports: >> $ref: /schemas/graph.yaml#/properties/ports >> @@ -55,6 +67,20 @@ required: >> - pinctrl-names >> - ports > > Best regards, > Krzysztof >
On 14/12/2023 16:22, Alex Bee wrote: > > Am 14.12.23 um 08:53 schrieb Krzysztof Kozlowski: >> On 13/12/2023 20:51, Alex Bee wrote: >>> Document the compatible for RK3128's HDMI controller block. >>> The integration for this SoC is somewhat different here: It needs the PHY's >> Please wrap commit message according to Linux coding style / submission >> process (neither too early nor over the limit): >> https://elixir.bootlin.com/linux/v6.4-rc1/source/Documentation/process/submitting-patches.rst#L597 > OK. Not sure why checkpatch --strict didn't tell me that I'm over the > limit here. >> >>> reference clock rate to calculate the ddc bus frequency correctly. This >>> clock is part of a power-domain (PD_VIO), so this gets added as an optional >>> property too. >> If clock is part of power domain, then the power domain must be in the >> clock controller, not here. So either you put power domain in wrong >> place or you used incorrect reason for a change. > Rockchip defines it's powerdomains per clock and I was little to much > in that world when writing this. Actually the controller itself is part > of the powerdomain. Will rephrase. Does it mean you have like 200 different power domains in one SoC? Then how are they different than clock if there is one-to-one mapping? >>> Signed-off-by: Alex Bee <knaerzche@gmail.com> >>> --- >>> .../display/rockchip/rockchip,inno-hdmi.yaml | 30 +++++++++++++++++-- >>> 1 file changed, 28 insertions(+), 2 deletions(-) >>> >>> diff --git a/Documentation/devicetree/bindings/display/rockchip/rockchip,inno-hdmi.yaml b/Documentation/devicetree/bindings/display/rockchip/rockchip,inno-hdmi.yaml >>> index 96889c86849a..9f00abcbfb38 100644 >>> --- a/Documentation/devicetree/bindings/display/rockchip/rockchip,inno-hdmi.yaml >>> +++ b/Documentation/devicetree/bindings/display/rockchip/rockchip,inno-hdmi.yaml >>> @@ -14,6 +14,7 @@ properties: >>> compatible: >>> enum: >>> - rockchip,rk3036-inno-hdmi >>> + - rockchip,rk3128-inno-hdmi >>> >>> reg: >>> maxItems: 1 >>> @@ -22,10 +23,21 @@ properties: >>> maxItems: 1 >>> >>> clocks: >>> - maxItems: 1 >>> + minItems: 1 >>> + items: >>> + - description: The HDMI controller main clock >>> + - description: The HDMI PHY reference clock >>> >>> clock-names: >>> - const: pclk >>> + minItems: 1 >>> + items: >>> + - const: pclk >>> + - enum: >>> + - pclk >>> + - ref >> That's way overcomplicated. Just items listing the names and minItems: >> 1. See other bindings how this is done. > OK. >>> + >>> + power-domains: >>> + maxItems: 1 >> Is it relevant to existing device? > > Will move to new variant only. > Definition should be here, but in if:then: it should be disallowed (:false) for other variants. Best regards, Krzysztof
Am Donnerstag, 14. Dezember 2023, 17:07:27 CET schrieb Krzysztof Kozlowski: > On 14/12/2023 16:22, Alex Bee wrote: > > > > Am 14.12.23 um 08:53 schrieb Krzysztof Kozlowski: > >> On 13/12/2023 20:51, Alex Bee wrote: > >>> Document the compatible for RK3128's HDMI controller block. > >>> The integration for this SoC is somewhat different here: It needs the PHY's > >> Please wrap commit message according to Linux coding style / submission > >> process (neither too early nor over the limit): > >> https://elixir.bootlin.com/linux/v6.4-rc1/source/Documentation/process/submitting-patches.rst#L597 > > OK. Not sure why checkpatch --strict didn't tell me that I'm over the > > limit here. > >> > >>> reference clock rate to calculate the ddc bus frequency correctly. This > >>> clock is part of a power-domain (PD_VIO), so this gets added as an optional > >>> property too. > >> If clock is part of power domain, then the power domain must be in the > >> clock controller, not here. So either you put power domain in wrong > >> place or you used incorrect reason for a change. > > Rockchip defines it's powerdomains per clock and I was little to much > > in that world when writing this. Actually the controller itself is part > > of the powerdomain. Will rephrase. > > Does it mean you have like 200 different power domains in one SoC? Then > how are they different than clock if there is one-to-one mapping? It's more like the other way around. Controllers and their clocks belong to specific power-domains. So there are of course more clocks than domains.
On 14/12/2023 17:20, Heiko Stübner wrote: > Am Donnerstag, 14. Dezember 2023, 17:07:27 CET schrieb Krzysztof Kozlowski: >> On 14/12/2023 16:22, Alex Bee wrote: >>> >>> Am 14.12.23 um 08:53 schrieb Krzysztof Kozlowski: >>>> On 13/12/2023 20:51, Alex Bee wrote: >>>>> Document the compatible for RK3128's HDMI controller block. >>>>> The integration for this SoC is somewhat different here: It needs the PHY's >>>> Please wrap commit message according to Linux coding style / submission >>>> process (neither too early nor over the limit): >>>> https://elixir.bootlin.com/linux/v6.4-rc1/source/Documentation/process/submitting-patches.rst#L597 >>> OK. Not sure why checkpatch --strict didn't tell me that I'm over the >>> limit here. >>>> >>>>> reference clock rate to calculate the ddc bus frequency correctly. This >>>>> clock is part of a power-domain (PD_VIO), so this gets added as an optional >>>>> property too. >>>> If clock is part of power domain, then the power domain must be in the >>>> clock controller, not here. So either you put power domain in wrong >>>> place or you used incorrect reason for a change. >>> Rockchip defines it's powerdomains per clock and I was little to much >>> in that world when writing this. Actually the controller itself is part >>> of the powerdomain. Will rephrase. >> >> Does it mean you have like 200 different power domains in one SoC? Then >> how are they different than clock if there is one-to-one mapping? > > It's more like the other way around. Controllers and their clocks belong > to specific power-domains. So there are of course more clocks than domains. That's fine and expected. Here the comment was suggested that you need to add power-domain because clock is in power-domain. That would be clearly wrong and instead the clock controller should model the power domain relationship. > > > > Best regards, Krzysztof
diff --git a/Documentation/devicetree/bindings/display/rockchip/rockchip,inno-hdmi.yaml b/Documentation/devicetree/bindings/display/rockchip/rockchip,inno-hdmi.yaml index 96889c86849a..9f00abcbfb38 100644 --- a/Documentation/devicetree/bindings/display/rockchip/rockchip,inno-hdmi.yaml +++ b/Documentation/devicetree/bindings/display/rockchip/rockchip,inno-hdmi.yaml @@ -14,6 +14,7 @@ properties: compatible: enum: - rockchip,rk3036-inno-hdmi + - rockchip,rk3128-inno-hdmi reg: maxItems: 1 @@ -22,10 +23,21 @@ properties: maxItems: 1 clocks: - maxItems: 1 + minItems: 1 + items: + - description: The HDMI controller main clock + - description: The HDMI PHY reference clock clock-names: - const: pclk + minItems: 1 + items: + - const: pclk + - enum: + - pclk + - ref + + power-domains: + maxItems: 1 ports: $ref: /schemas/graph.yaml#/properties/ports @@ -55,6 +67,20 @@ required: - pinctrl-names - ports +allOf: + - if: + properties: + compatible: + contains: + const: rockchip,rk3128-inno-hdmi + + then: + properties: + clocks: + minItems: 2 + clock-names: + minItems: 2 + additionalProperties: false examples:
Document the compatible for RK3128's HDMI controller block. The integration for this SoC is somewhat different here: It needs the PHY's reference clock rate to calculate the ddc bus frequency correctly. This clock is part of a power-domain (PD_VIO), so this gets added as an optional property too. Signed-off-by: Alex Bee <knaerzche@gmail.com> --- .../display/rockchip/rockchip,inno-hdmi.yaml | 30 +++++++++++++++++-- 1 file changed, 28 insertions(+), 2 deletions(-)