Re: [PATCH v2 1/4] dt-bindings: clock: separate SDM845 GCC clock bindings

From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
To: Stephen Boyd <sboyd@kernel.org>
Cc: Andy Gross <agross@kernel.org>,
	Bjorn Andersson <bjorn.andersson@linaro.org>,
	Michael Turquette <mturquette@baylibre.com>,
	Rob Herring <robh+dt@kernel.org>,
	"open list:DRM DRIVER FOR MSM ADRENO GPU" 
	<linux-arm-msm@vger.kernel.org>,
	"open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
	<devicetree@vger.kernel.org>,
	"open list:COMMON CLK FRAMEWORK" <linux-clk@vger.kernel.org>
Subject: Re: [PATCH v2 1/4] dt-bindings: clock: separate SDM845 GCC clock bindings
Date: Thu, 8 Apr 2021 03:38:06 +0300	[thread overview]
Message-ID: <CAA8EJpqVJgj0eBm5m91MNqiBZg0y1v=iMAuLpW8H-i3ut8-q0A@mail.gmail.com> (raw)
In-Reply-To: <161784119850.3790633.17698180700358661431@swboyd.mtv.corp.google.com>

Hello,

On Thu, 8 Apr 2021 at 03:20, Stephen Boyd <sboyd@kernel.org> wrote:
>
> Quoting Dmitry Baryshkov (2021-04-06 16:19:06)
> > Separate qcom,gcc-sdm845 clock bindings, adding required clocks and
> > clock-names properties.
>
> Yes, but why?

Why separate or why add required clocks? Consider the rest of
bindings, where qcom,gcc.yaml defines older bindings, which do not use
clocks/clock-names and for newer bindings we have one file per binding
(qcom,gcc-apq8064.yaml, qcom,gcc-qcs404.yaml, qcom,gcc-sdx55.yaml,
qcom,gcc-sm8150.yaml, etc).

Do you suggest merging all of them back into a single yaml file?

>
> >
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > ---
> >  .../bindings/clock/qcom,gcc-sdm845.yaml       | 84 +++++++++++++++++++
> >  .../devicetree/bindings/clock/qcom,gcc.yaml   |  2 -
> >  2 files changed, 84 insertions(+), 2 deletions(-)
> >  create mode 100644 Documentation/devicetree/bindings/clock/qcom,gcc-sdm845.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/clock/qcom,gcc-sdm845.yaml b/Documentation/devicetree/bindings/clock/qcom,gcc-sdm845.yaml
> > new file mode 100644
> > index 000000000000..4808fa7a6b8c
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/clock/qcom,gcc-sdm845.yaml
> > @@ -0,0 +1,84 @@
> > +# SPDX-License-Identifier: GPL-2.0-only
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/clock/qcom,gcc-sdm845.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Qualcomm Global Clock & Reset Controller Binding
> > +
> > +maintainers:
> > +  - Stephen Boyd <sboyd@kernel.org>
> > +  - Taniya Das <tdas@codeaurora.org>
> > +
> > +description: |
> > +  Qualcomm global clock control module which supports the clocks, resets and
> > +  power domains on SDM845
> > +
> > +  See also:
> > +  - dt-bindings/clock/qcom,gcc-sdm845.h
> > +
> > +properties:
> > +  compatible:
> > +    const: qcom,gcc-sdm845
> > +
> > +  clocks:
> > +    items:
> > +      - description: Board XO source
> > +      - description: Board active XO source
> > +      - description: Sleep clock source
> > +      - description: PCIE 0 Pipe clock source (Optional clock)
> > +      - description: PCIE 1 Pipe clock source (Optional clock)
>
> Can we make them not optional?

Sure,

>
> > +    minItems: 3
> > +    maxItems: 5
>
> And remove this? Because in reality the clks are always going there so
> we can put a <0> element in the clocks array if we don't care to
> actually specify it.

Fine with me.

>
> > +
> > +  clock-names:
> > +    items:
> > +      - const: bi_tcxo
> > +      - const: bi_tcxo_ao
> > +      - const: sleep_clk
> > +      - const: pcie_0_pipe_clk # Optional clock
> > +      - const: pcie_1_pipe_clk # Optional clock
> > +    minItems: 3
> > +    maxItems: 5
> > +

-- 
With best wishes
Dmitry