Re: [PATCH v8 1/4] dt-bindings: clock: Document external clocks for MSM8998 gcc

[Rob Herring <robh@kernel.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