Linux-ARM-MSM Archive on lore.kernel.org
 help / color / Atom feed
From: Rob Herring <robh@kernel.org>
To: Jeffrey Hugo <jhugo@codeaurora.org>
Cc: Michael Turquette <mturquette@baylibre.com>,
	Stephen Boyd <sboyd@kernel.org>,
	Mark Rutland <mark.rutland@arm.com>,
	Andy Gross <agross@kernel.org>,
	Bjorn Andersson <bjorn.andersson@linaro.org>,
	Marc Gonzalez <marc.w.gonzalez@free.fr>,
	linux-arm-msm <linux-arm-msm@vger.kernel.org>,
	linux-clk <linux-clk@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	devicetree@vger.kernel.org
Subject: Re: [PATCH v8 1/4] dt-bindings: clock: Document external clocks for MSM8998 gcc
Date: Tue, 12 Nov 2019 12:37:04 -0600
Message-ID: <CAL_JsqJ3R0Y-KPKaknVT=+RTAskGhqmarb=i9ZDyX5-LzoFOjg@mail.gmail.com> (raw)
In-Reply-To: <3e4b1342-7965-2d80-e28d-0cb728037abd@codeaurora.org>

On Tue, Nov 12, 2019 at 10:25 AM Jeffrey Hugo <jhugo@codeaurora.org> wrote:
>
> On 11/11/2019 5:44 PM, Rob Herring wrote:
> > On Fri, Nov 08, 2019 at 04:17:16PM -0700, Jeffrey Hugo wrote:
> >> The global clock controller on MSM8998 can consume a number of external
> >> clocks.  Document them.
> >>
> >> Signed-off-by: Jeffrey Hugo <jhugo@codeaurora.org>
> >> ---
> >>   .../devicetree/bindings/clock/qcom,gcc.yaml        | 47 +++++++++++++++-------
> >>   1 file changed, 33 insertions(+), 14 deletions(-)
> >>
> >> diff --git a/Documentation/devicetree/bindings/clock/qcom,gcc.yaml b/Documentation/devicetree/bindings/clock/qcom,gcc.yaml
> >> index e73a56f..2f3512b 100644
> >> --- a/Documentation/devicetree/bindings/clock/qcom,gcc.yaml
> >> +++ b/Documentation/devicetree/bindings/clock/qcom,gcc.yaml
> >> @@ -40,20 +40,38 @@ properties:
> >>          - qcom,gcc-sm8150
> >>
> >>     clocks:
> >> -    minItems: 1
> >
> > 1 or 2 clocks are no longer allowed?
>
> Correct.
>
> The primary reason is that Stephen indicated in previous discussions
> that if the hardware exists, it should be indicated in DT, regardless if
> the driver uses it.  In the 7180 and 8150 case, the hardware exists, so
> these should not be optional.

Agreed. The commit message should mention this though.

>
> The secondary reason is I found that the schema was broken anyways.  In
> the way it was written, if you implemented sleep, you could not skip
> xo_ao, however there is a dts that did exactly that.

If a dts can be updated in a compatible way, we should do that rather
than carry inconsistencies into the schema.

> The third reason was that I couldn't find a way to write valid yaml to
> preserve the original meaning.  when you have an "items" as a subnode of
> "oneOf", you no longer have control over the minItems/maxItems, so all 3
> became required anyways.

That would be a bug. You're saying something like this doesn't work?:

oneOf:
  - minItems: 1
    maxItems: 3
    items:
      - const: a
      - const: b
      - const: c

>  I find it disappointing that the "version" of
> Yaml used for DT bindings is not documented,

Not sure which part you mean? json-schema is the vocabulary which has
a spec. The meta-schema then constrains what the json-schema structure
should look like. That's still evolving a bit as I try to improve it
based on mistakes people make. Then there's the intermediate .dt.yaml
format used internally. That's supposed to stay internal and may go
away when/if we integrate the validation into dtc.

> so after several hours of
> trial and error, I just gave up since I found this to work (failed cases
> just gave me an error with no indication of what was wrong, not even a
> line number).

Schema failures or dts failures? It is possible to get line numbers
for either, but that makes validation much slower. In the latter case,
the line numbers aren't too useful either given they are for the
.dt.yaml file and not the .dts source file (dtc integration would
solve that). Adding '-n' to dt-doc-validate or dt-validate will turn
them on though.

Yes, error messages need work. I have some idea how to improve them,
but haven't had time to implement. Too many binding reviews... You can
get more detail with '-v' option. It's *way* more verbose, but not
necessarily more useful.

Rob

  reply index

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-11-08 23:16 [PATCH v8 0/4] MSM8998 Multimedia Clock Controller Jeffrey Hugo
2019-11-08 23:17 ` [PATCH v8 1/4] dt-bindings: clock: Document external clocks for MSM8998 gcc Jeffrey Hugo
2019-11-12  0:44   ` Rob Herring
2019-11-12 16:25     ` Jeffrey Hugo
2019-11-12 18:37       ` Rob Herring [this message]
2019-11-12 19:38         ` Jeffrey Hugo
2019-11-12 21:18           ` Rob Herring
2019-11-12 22:03             ` Jeffrey Hugo
2019-11-08 23:17 ` [PATCH v8 2/4] dt-bindings: clock: Convert qcom,mmcc to DT schema Jeffrey Hugo
2019-11-12  0:58   ` Rob Herring
2019-11-12 16:11     ` Jeffrey Hugo
2019-11-08 23:17 ` [PATCH v8 3/4] dt-bindings: clock: Add support for the MSM8998 mmcc Jeffrey Hugo
2019-11-12  0:55   ` Rob Herring
2019-11-12 16:07     ` Jeffrey Hugo
2019-11-12  0:59   ` Rob Herring
2019-11-08 23:18 ` [PATCH v8 4/4] clk: qcom: Add MSM8998 Multimedia Clock Controller (MMCC) driver Jeffrey Hugo

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAL_JsqJ3R0Y-KPKaknVT=+RTAskGhqmarb=i9ZDyX5-LzoFOjg@mail.gmail.com' \
    --to=robh@kernel.org \
    --cc=agross@kernel.org \
    --cc=bjorn.andersson@linaro.org \
    --cc=devicetree@vger.kernel.org \
    --cc=jhugo@codeaurora.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-clk@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=marc.w.gonzalez@free.fr \
    --cc=mark.rutland@arm.com \
    --cc=mturquette@baylibre.com \
    --cc=sboyd@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-ARM-MSM Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-arm-msm/0 linux-arm-msm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-arm-msm linux-arm-msm/ https://lore.kernel.org/linux-arm-msm \
		linux-arm-msm@vger.kernel.org
	public-inbox-index linux-arm-msm

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-arm-msm


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git