On Thu, Apr 29, 2021 at 12:03 PM Ezequiel Garcia <ezequiel@xxxxxxxxxxxxx> wrote:
>
> On Thu, 2021-04-29 at 11:25 -0500, Rob Herring wrote:
> > On Sat, Apr 24, 2021 at 04:19:46PM -0300, Ezequiel Garcia wrote:
> > > Convert Rockchip Timer dt-bindings to YAML.
> > >
> > > Signed-off-by: Ezequiel Garcia <ezequiel@xxxxxxxxxxxxx>
> > > ---
> > > .../bindings/timer/rockchip,rk-timer.txt | 27 --------
> > > .../bindings/timer/rockchip,rk-timer.yaml | 67 +++++++++++++++++++
> > > 2 files changed, 67 insertions(+), 27 deletions(-)
> > > delete mode 100644 Documentation/devicetree/bindings/timer/rockchip,rk-timer.txt
> > > create mode 100644 Documentation/devicetree/bindings/timer/rockchip,rk-timer.yaml
> > >
> > > diff --git a/Documentation/devicetree/bindings/timer/rockchip,rk-timer.txt b/Documentation/devicetree/bindings/timer/rockchip,rk-timer.txt
> > > deleted file mode 100644
> > > index d65fdce7c7f0..000000000000
> > > --- a/Documentation/devicetree/bindings/timer/rockchip,rk-timer.txt
> > > +++ /dev/null
> > > @@ -1,27 +0,0 @@
> > > -Rockchip rk timer
> > > -
> > > -Required properties:
> > > -- compatible: should be:
> > > - "rockchip,rv1108-timer", "rockchip,rk3288-timer": for Rockchip RV1108
> > > - "rockchip,rk3036-timer", "rockchip,rk3288-timer": for Rockchip RK3036
> > > - "rockchip,rk3066-timer", "rockchip,rk3288-timer": for Rockchip RK3066
> > > - "rockchip,rk3188-timer", "rockchip,rk3288-timer": for Rockchip RK3188
> > > - "rockchip,rk3228-timer", "rockchip,rk3288-timer": for Rockchip RK3228
> > > - "rockchip,rk3229-timer", "rockchip,rk3288-timer": for Rockchip RK3229
> > > - "rockchip,rk3288-timer": for Rockchip RK3288
> > > - "rockchip,rk3368-timer", "rockchip,rk3288-timer": for Rockchip RK3368
> > > - "rockchip,rk3399-timer": for Rockchip RK3399
> > > -- reg: base address of the timer register starting with TIMERS CONTROL register
> > > -- interrupts: should contain the interrupts for Timer0
> > > -- clocks : must contain an entry for each entry in clock-names
> > > -- clock-names : must include the following entries:
> > > - "timer", "pclk"
> > > -
> > > -Example:
> > > - timer: timer@ff810000 {
> > > - compatible = "rockchip,rk3288-timer";
> > > - reg = <0xff810000 0x20>;
> > > - interrupts = <GIC_SPI 72 IRQ_TYPE_LEVEL_HIGH>;
> > > - clocks = <&xin24m>, <&cru PCLK_TIMER>;
> > > - clock-names = "timer", "pclk";
> > > - };
> > > diff --git a/Documentation/devicetree/bindings/timer/rockchip,rk-timer.yaml b/Documentation/devicetree/bindings/timer/rockchip,rk-timer.yaml
> > > new file mode 100644
> > > index 000000000000..f1bc3ac7abc8
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/timer/rockchip,rk-timer.yaml
> > > @@ -0,0 +1,67 @@
> > > +# SPDX-License-Identifier: GPL-2.0
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/timer/rockchip,rk-timer.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Rockchip Timer Device Tree Bindings
> > > +
> > > +maintainers:
> > > + - Daniel Lezcano <daniel.lezcano@xxxxxxxxxx>
> >
> > This should be someone that knows the h/w and cares about Rockchip.
> >
>
> Daniel wrote the driver, so I figured he'd care :)
Ah, then that's fine I guess. Given he is also the subsystem
maintainer I was confused.
> If not, perhaps Heiko (if he agrees)?
>
> > > +
> > > +properties:
> > > + compatible:
> > > + oneOf:
> > > + - const: rockchip,rk3288-timer
> > > + - const: rockchip,rk3399-timer
> > > + - items:
> > > + - enum:
> > > + - rockchip,rv1108-timer
> > > + - rockchip,rk3036-timer
> > > + - rockchip,rk3066-timer
> > > + - rockchip,rk3188-timer
> > > + - rockchip,rk3228-timer
> > > + - rockchip,rk3229-timer
> > > + - rockchip,rk3288-timer
> > > + - rockchip,rk3368-timer
> > > + - rockchip,px30-timer
> > > + - const: rockchip,rk3288-timer
> > > + reg:
> > > + maxItems: 1
> > > +
> > > + interrupts:
> > > + maxItems: 1
> > > +
> > > + clocks:
> > > + minItems: 2
> > > + maxItems: 2
> > > +
> > > + clock-names:
> > > + items:
> > > + enum:
> > > + - timer
> > > + - pclk
> >
> > We can't define the order here? We should fix dts files if they are
> > inconsistent.
> >
>
> The driver requests the clocks by name, and unfortunately DTS
> rely on that.
That's good, then the OS doesn't care if you change it. Of course, I
didn't have to look to tell that. If the order varied, the OS would
have to use the names.
> We can change all the DTSI, but wouldn't that
> be too much trouble for something that is currently working fine?
It's not *all*. At least half are correct. Pick the order that's the majority.
> Why is the order important?
Because that's the DT convention. Why is random order important?
I don't really care so much on an individual binding, but overall if
defining the order doesn't cost anything then we should. The more
order is not defined, the more people are going to copy those
examples. Yes, there's a cost on fixing dts files, but if we're
writing schema to pass on existing dts files, what's the point of
schema?
Rob
