diff mbox series

[08/22] dt-bindings: display: rockchip: dw-hdmi: use "ref" as clock name

Message ID 20211220110630.3521121-9-s.hauer@pengutronix.de (mailing list archive)
State New
Headers show
Series drm/rockchip: RK356x VOP2 support | expand

Commit Message

Sascha Hauer Dec. 20, 2021, 11:06 a.m. UTC
"vpll" is a misnomer. A clock input to a device should be named after
the usage in the device, not after the clock that drives it. On the
rk3568 the same clock is driven by the HPLL.
To fix that, this patch renames the vpll clock to ref clock.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 .../bindings/display/rockchip/rockchip,dw-hdmi.yaml        | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

Comments

Rob Herring Dec. 20, 2021, 2:27 p.m. UTC | #1
On Mon, 20 Dec 2021 12:06:16 +0100, Sascha Hauer wrote:
> "vpll" is a misnomer. A clock input to a device should be named after
> the usage in the device, not after the clock that drives it. On the
> rk3568 the same clock is driven by the HPLL.
> To fix that, this patch renames the vpll clock to ref clock.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  .../bindings/display/rockchip/rockchip,dw-hdmi.yaml        | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml: properties:clock-names:items: 'oneOf' conditional failed, one must be fixed:
	[{}, {}, {'enum': ['cec', 'grf', 'ref']}, {'enum': ['grf', 'ref']}, {'const': ['ref']}] is not of type 'object'
	['ref'] is not of type 'string'
	from schema $id: http://devicetree.org/meta-schemas/string-array.yaml#
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml: ignoring, error in schema: properties: clock-names: items
warning: no schema found in file: ./Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml
Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.example.dt.yaml:0:0: /example-0/hdmi@ff980000: failed to match any schema with compatible: ['rockchip,rk3288-dw-hdmi']

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/patch/1570972

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.
Rob Herring Dec. 21, 2021, 2:31 p.m. UTC | #2
On Mon, Dec 20, 2021 at 12:06:16PM +0100, Sascha Hauer wrote:
> "vpll" is a misnomer. A clock input to a device should be named after
> the usage in the device, not after the clock that drives it. On the
> rk3568 the same clock is driven by the HPLL.
> To fix that, this patch renames the vpll clock to ref clock.

The problem with this series is it breaks an old kernel with new dt. You 
can partially mitigate that with stable kernel backport, but IMO keeping 
the old name is not a burden to maintain.

And given RK3399 is widely used including by me, we should not be 
breaking compatibility.

So allow for ref in addition to vpll if you like, but only use 'ref' for 
new users. And add a comment in the schema to that effect.

Rob

> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  .../bindings/display/rockchip/rockchip,dw-hdmi.yaml        | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml b/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml
> index 6e09dd2ee05ac..3b40219e3ea60 100644
> --- a/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml
> +++ b/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml
> @@ -47,11 +47,12 @@ properties:
>        - enum:
>            - cec
>            - grf
> -          - vpll
> +          - ref
>        - enum:
>            - grf
> -          - vpll
> -      - const: vpll
> +          - ref
> +      - const:
> +          - ref
>  
>    ddc-i2c-bus:
>      $ref: /schemas/types.yaml#/definitions/phandle
> -- 
> 2.30.2
> 
>
Sascha Hauer Dec. 22, 2021, 10:47 a.m. UTC | #3
On Tue, Dec 21, 2021 at 10:31:23AM -0400, Rob Herring wrote:
> On Mon, Dec 20, 2021 at 12:06:16PM +0100, Sascha Hauer wrote:
> > "vpll" is a misnomer. A clock input to a device should be named after
> > the usage in the device, not after the clock that drives it. On the
> > rk3568 the same clock is driven by the HPLL.
> > To fix that, this patch renames the vpll clock to ref clock.
> 
> The problem with this series is it breaks an old kernel with new dt. You 
> can partially mitigate that with stable kernel backport, but IMO keeping 
> the old name is not a burden to maintain.

As suggested I only removed vpll from the binding document, but not from
the code. The code still handles the old binding as well.

Sascha
Rob Herring Dec. 22, 2021, 1:52 p.m. UTC | #4
On Wed, Dec 22, 2021 at 6:47 AM Sascha Hauer <s.hauer@pengutronix.de> wrote:
>
> On Tue, Dec 21, 2021 at 10:31:23AM -0400, Rob Herring wrote:
> > On Mon, Dec 20, 2021 at 12:06:16PM +0100, Sascha Hauer wrote:
> > > "vpll" is a misnomer. A clock input to a device should be named after
> > > the usage in the device, not after the clock that drives it. On the
> > > rk3568 the same clock is driven by the HPLL.
> > > To fix that, this patch renames the vpll clock to ref clock.
> >
> > The problem with this series is it breaks an old kernel with new dt. You
> > can partially mitigate that with stable kernel backport, but IMO keeping
> > the old name is not a burden to maintain.
>
> As suggested I only removed vpll from the binding document, but not from
> the code. The code still handles the old binding as well.

The problem is updating rk3399.dtsi. That change won't work with old
kernels because they won't look for 'ref'. Since you shouldn't change
it, the binding needs to cover both the old and new cases.

Rob
Heiko Stübner Dec. 22, 2021, 7:39 p.m. UTC | #5
Am Mittwoch, 22. Dezember 2021, 14:52:51 CET schrieb Rob Herring:
> On Wed, Dec 22, 2021 at 6:47 AM Sascha Hauer <s.hauer@pengutronix.de> wrote:
> >
> > On Tue, Dec 21, 2021 at 10:31:23AM -0400, Rob Herring wrote:
> > > On Mon, Dec 20, 2021 at 12:06:16PM +0100, Sascha Hauer wrote:
> > > > "vpll" is a misnomer. A clock input to a device should be named after
> > > > the usage in the device, not after the clock that drives it. On the
> > > > rk3568 the same clock is driven by the HPLL.
> > > > To fix that, this patch renames the vpll clock to ref clock.
> > >
> > > The problem with this series is it breaks an old kernel with new dt. You
> > > can partially mitigate that with stable kernel backport, but IMO keeping
> > > the old name is not a burden to maintain.
> >
> > As suggested I only removed vpll from the binding document, but not from
> > the code. The code still handles the old binding as well.
> 
> The problem is updating rk3399.dtsi. That change won't work with old
> kernels because they won't look for 'ref'. Since you shouldn't change
> it, the binding needs to cover both the old and new cases.

is "newer dt with old kernel" really a case these days?

I do understand the new kernel old dt case - for example with the
dtb being provided by firmware.

But which user would get the idea of updating only the devicetree
while staying with an older kernel?
Nicolas Frattaroli Dec. 22, 2021, 7:44 p.m. UTC | #6
On Mittwoch, 22. Dezember 2021 20:39:58 CET Heiko Stübner wrote:
> Am Mittwoch, 22. Dezember 2021, 14:52:51 CET schrieb Rob Herring:
> > On Wed, Dec 22, 2021 at 6:47 AM Sascha Hauer <s.hauer@pengutronix.de> wrote:
> > >
> > > On Tue, Dec 21, 2021 at 10:31:23AM -0400, Rob Herring wrote:
> > > > On Mon, Dec 20, 2021 at 12:06:16PM +0100, Sascha Hauer wrote:
> > > > > "vpll" is a misnomer. A clock input to a device should be named after
> > > > > the usage in the device, not after the clock that drives it. On the
> > > > > rk3568 the same clock is driven by the HPLL.
> > > > > To fix that, this patch renames the vpll clock to ref clock.
> > > >
> > > > The problem with this series is it breaks an old kernel with new dt. You
> > > > can partially mitigate that with stable kernel backport, but IMO keeping
> > > > the old name is not a burden to maintain.
> > >
> > > As suggested I only removed vpll from the binding document, but not from
> > > the code. The code still handles the old binding as well.
> > 
> > The problem is updating rk3399.dtsi. That change won't work with old
> > kernels because they won't look for 'ref'. Since you shouldn't change
> > it, the binding needs to cover both the old and new cases.
> 
> is "newer dt with old kernel" really a case these days?
> 
> I do understand the new kernel old dt case - for example with the
> dtb being provided by firmware.
> 
> But which user would get the idea of updating only the devicetree
> while staying with an older kernel?
> 

Side-by-side installations of LTS kernels with new kernels. LTS kernel
uses same DT as new kernel because distribution set it up this way.

Other scenario: user wants to modify their device tree. They download
the latest kernel sources from kernel.org because they can't use over-
lays and they don't want to fiddle with decompiled device trees.
Rob Herring Dec. 22, 2021, 7:56 p.m. UTC | #7
On Wed, Dec 22, 2021 at 3:40 PM Heiko Stübner <heiko@sntech.de> wrote:
>
> Am Mittwoch, 22. Dezember 2021, 14:52:51 CET schrieb Rob Herring:
> > On Wed, Dec 22, 2021 at 6:47 AM Sascha Hauer <s.hauer@pengutronix.de> wrote:
> > >
> > > On Tue, Dec 21, 2021 at 10:31:23AM -0400, Rob Herring wrote:
> > > > On Mon, Dec 20, 2021 at 12:06:16PM +0100, Sascha Hauer wrote:
> > > > > "vpll" is a misnomer. A clock input to a device should be named after
> > > > > the usage in the device, not after the clock that drives it. On the
> > > > > rk3568 the same clock is driven by the HPLL.
> > > > > To fix that, this patch renames the vpll clock to ref clock.
> > > >
> > > > The problem with this series is it breaks an old kernel with new dt. You
> > > > can partially mitigate that with stable kernel backport, but IMO keeping
> > > > the old name is not a burden to maintain.
> > >
> > > As suggested I only removed vpll from the binding document, but not from
> > > the code. The code still handles the old binding as well.
> >
> > The problem is updating rk3399.dtsi. That change won't work with old
> > kernels because they won't look for 'ref'. Since you shouldn't change
> > it, the binding needs to cover both the old and new cases.
>
> is "newer dt with old kernel" really a case these days?

I've had complaints about it. In particular from SUSE folks that were
shipping new dtbs with old (stable) kernels.

> I do understand the new kernel old dt case - for example with the
> dtb being provided by firmware.

Yes, so update your firmware that contains a newer dtb and then you
stop booting or a device stops working.

> But which user would get the idea of updating only the devicetree
> while staying with an older kernel?

Any synchronization between firmware and OS updates is a problem.

Rob
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml b/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml
index 6e09dd2ee05ac..3b40219e3ea60 100644
--- a/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml
+++ b/Documentation/devicetree/bindings/display/rockchip/rockchip,dw-hdmi.yaml
@@ -47,11 +47,12 @@  properties:
       - enum:
           - cec
           - grf
-          - vpll
+          - ref
       - enum:
           - grf
-          - vpll
-      - const: vpll
+          - ref
+      - const:
+          - ref
 
   ddc-i2c-bus:
     $ref: /schemas/types.yaml#/definitions/phandle