Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20200301174636.63446-1-paul@crapouillou.net> <20200301174636.63446-2-paul@crapouillou.net>
 <CAL_JsqKGzxdMj4_+i4ycKj6ZjiuGMY8F+yBzVPt_b2CLhrcdKg@mail.gmail.com>
 <1583173481.3.0@crapouillou.net> <CAL_JsqL7b8mwtg3XyNS2fdA4fxaFdUpsfqTPx521pW5xqSPneg@mail.gmail.com>
 <1583177720.3.6@crapouillou.net>
In-Reply-To: <1583177720.3.6@crapouillou.net>
From:   Rob Herring <robh+dt@kernel.org>
Date:   Mon, 2 Mar 2020 13:59:52 -0600
Message-ID: <CAL_Jsq+c+RHCunZt2m4m85VztVU4L=-JYMY7B_7ADEyjJLK90g@mail.gmail.com>
Subject: Re: [PATCH 1/1] dt-bindings: timer: Convert ingenic,tcu.txt to YAML
To:     Paul Cercueil <paul@crapouillou.net>
Cc:     Daniel Lezcano <daniel.lezcano@linaro.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        Mark Rutland <mark.rutland@arm.com>,
        =?UTF-8?B?5ZGo55Cw5p2w?= <zhouyanjie@wanyeetech.com>, od@zcrc.me,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        devicetree@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Mon, Mar 2, 2020 at 1:35 PM Paul Cercueil <paul@crapouillou.net> wrote:
>
>
>
> Le lun., mars 2, 2020 at 13:07, Rob Herring <robh+dt@kernel.org> a
> =C3=A9crit :
> > On Mon, Mar 2, 2020 at 12:25 PM Paul Cercueil <paul@crapouillou.net>
> > wrote:
> >>
> >>  Hi Rob,
> >>
> >>
> >>  Le lun., mars 2, 2020 at 11:06, Rob Herring <robh+dt@kernel.org> a
> >>  =C3=A9crit :
> >>  > On Sun, Mar 1, 2020 at 11:47 AM Paul Cercueil
> >> <paul@crapouillou.net>
> >>  > wrote:
> >>  >>
> >>  >
> >>  > Well, this flew into linux-next quickly and breaks 'make
> >>  > dt_binding_check'... Please drop, revert or fix quickly.
> >>
> >>  For my defense I said to merge "provided Rob acks it" ;)
> >>
> >>  >>  Convert the ingenic,tcu.txt file to YAML.
> >>  >>
> >>  >>  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> >>  >>  ---
> >>  >>   .../devicetree/bindings/timer/ingenic,tcu.txt | 138 ----------
> >>  >>   .../bindings/timer/ingenic,tcu.yaml           | 235
> >>  >> ++++++++++++++++++
> >>  >>   2 files changed, 235 insertions(+), 138 deletions(-)
> >>  >>   delete mode 100644
> >>  >> Documentation/devicetree/bindings/timer/ingenic,tcu.txt
> >>  >>   create mode 100644
> >>  >> Documentation/devicetree/bindings/timer/ingenic,tcu.yaml
> >>  >
> >>  >
> >>  >>  diff --git
> >>  >> a/Documentation/devicetree/bindings/timer/ingenic,tcu.yaml
> >>  >> b/Documentation/devicetree/bindings/timer/ingenic,tcu.yaml
> >>  >>  new file mode 100644
> >>  >>  index 000000000000..1ded3b4762bb
> >>  >>  --- /dev/null
> >>  >>  +++ b/Documentation/devicetree/bindings/timer/ingenic,tcu.yaml
> >>  >>  @@ -0,0 +1,235 @@
> >>  >>  +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> >>  >>  +%YAML 1.2
> >>  >>  +---
> >>  >>  +$id: http://devicetree.org/schemas/timer/ingenic,tcu.yaml#
> >>  >>  +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >>  >>  +
> >>  >>  +title: Ingenic SoCs Timer/Counter Unit (TCU) devicetree
> >> bindings
> >>  >>  +
> >>  >>  +description: |
> >>  >>  +  For a description of the TCU hardware and drivers, have a
> >> look at
> >>  >>  +  Documentation/mips/ingenic-tcu.rst.
> >>  >>  +
> >>  >>  +maintainers:
> >>  >>  +  - Paul Cercueil <paul@crapouillou.net>
> >>  >>  +
> >>  >>  +properties:
> >>  >>  +  $nodename:
> >>  >>  +    pattern: "^timer@.*"
> >>  >
> >>  > '.*' is redundant.
> >>  >
> >>  >>  +
> >>  >>  +  "#address-cells":
> >>  >>  +    const: 1
> >>  >>  +
> >>  >>  +  "#size-cells":
> >>  >>  +    const: 1
> >>  >>  +
> >>  >>  +  "#clock-cells":
> >>  >>  +    const: 1
> >>  >>  +
> >>  >>  +  "#interrupt-cells":
> >>  >>  +    const: 1
> >>  >>  +
> >>  >>  +  interrupt-controller: true
> >>  >>  +
> >>  >>  +  ranges: true
> >>  >>  +
> >>  >>  +  compatible:
> >>  >>  +    items:
> >>  >>  +      - enum:
> >>  >>  +        - ingenic,jz4740-tcu
> >>  >>  +        - ingenic,jz4725b-tcu
> >>  >>  +        - ingenic,jz4770-tcu
> >>  >>  +        - ingenic,x1000-tcu
> >>  >>  +      - const: simple-mfd
> >>  >
> >>  > This breaks several examples in dt_binding_check because this
> >> schema
> >>  > will be applied to every 'simple-mfd' node. You need a custom
> >> select
> >>  > entry that excludes 'simple-mfd'. There should be several
> >> examples in
> >>  > tree to copy.
> >>
> >>  Why would it be applied to all 'single-mfd' nodes?
> >
> > single-mfd?
>
> simple-mfd* of course, sorry.
>
> > The way the tool decides to apply a schema or not is my matching on
> > any of the compatible strings (or node name if no compatible
> > specified). You can override this with 'select'.
> >
> >>  Doesn't what I wrote
> >>  specify that it needs one of ingenic,*-tcu _and_ simple-mfd?
> >
> > Yes, but matching is on any of them. You need to add:
>
> Alright, will do. Is there a reason why it's done that way? It sounds a
> bit counter-intuitive.

I'm not sure how we could do it differently. We need some way to
express 'apply this schema to a node if ...'. If we just matched on
'compatible' schema as is, then we'd get silence if there's any error
in 'compatible'. That can still happen, but it's reduced in the cases
where there's more than one compatible string as only 1 has to be
right. It's also very common that valid combinations of compatible
strings are not documented clearly or followed correctly, so we can
catch these errors. I wasn't a fan of having to list out compatible
strings twice, so the tool does it for you in the common case.

Rob