Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 22 Mar 2022 14:26:13 +0100
From:   Ansuel Smith <ansuelsmth@gmail.com>
To:     Krzysztof Kozlowski <krzk@kernel.org>
Cc:     Rob Herring <robh+dt@kernel.org>,
        Bjorn Andersson <bjorn.andersson@linaro.org>,
        Andy Gross <agross@kernel.org>,
        Michael Turquette <mturquette@baylibre.com>,
        Stephen Boyd <sboyd@kernel.org>, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-arm-msm@vger.kernel.org,
        linux-clk@vger.kernel.org
Subject: Re: [PATCH v6 17/18] dt-bindings: arm: msm: Convert kpss-gcc driver
 Documentation to yaml
Message-ID: <YjnOdYMS+P85pqvF@Ansuel-xps.localdomain>
References: <20220321231548.14276-1-ansuelsmth@gmail.com>
 <20220321231548.14276-18-ansuelsmth@gmail.com>
 <e832516d-277d-6a0b-4588-b32a085185c8@kernel.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <e832516d-277d-6a0b-4588-b32a085185c8@kernel.org>
Precedence: bulk

On Tue, Mar 22, 2022 at 11:07:26AM +0100, Krzysztof Kozlowski wrote:
> On 22/03/2022 00:15, Ansuel Smith wrote:
> > Convert kpss-gcc driver Documentation to yaml. Since kpss-gcc expose a
> > clock add the required '#clock-cells' binding while converting it.
> > 
> > Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> > ---
> >  .../bindings/arm/msm/qcom,kpss-gcc.txt        | 44 ------------
> >  .../bindings/arm/msm/qcom,kpss-gcc.yaml       | 69 +++++++++++++++++++
> >  2 files changed, 69 insertions(+), 44 deletions(-)
> >  delete mode 100644 Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.txt
> >  create mode 100644 Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.txt b/Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.txt
> > deleted file mode 100644
> > index e628758950e1..000000000000
> > --- a/Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.txt
> > +++ /dev/null
> > @@ -1,44 +0,0 @@
> > -Krait Processor Sub-system (KPSS) Global Clock Controller (GCC)
> > -
> > -PROPERTIES
> > -
> > -- compatible:
> > -	Usage: required
> > -	Value type: <string>
> > -	Definition: should be one of the following. The generic compatible
> > -			"qcom,kpss-gcc" should also be included.
> > -			"qcom,kpss-gcc-ipq8064", "qcom,kpss-gcc"
> > -			"qcom,kpss-gcc-apq8064", "qcom,kpss-gcc"
> > -			"qcom,kpss-gcc-msm8974", "qcom,kpss-gcc"
> > -			"qcom,kpss-gcc-msm8960", "qcom,kpss-gcc"
> > -
> > -- reg:
> > -	Usage: required
> > -	Value type: <prop-encoded-array>
> > -	Definition: base address and size of the register region
> > -
> > -- clocks:
> > -	Usage: required
> > -	Value type: <prop-encoded-array>
> > -	Definition: reference to the pll parents.
> > -
> > -- clock-names:
> > -	Usage: required
> > -	Value type: <stringlist>
> > -	Definition: must be "pll8_vote", "pxo".
> > -
> > -- clock-output-names:
> > -	Usage: required
> > -	Value type: <string>
> > -	Definition: Name of the output clock. Typically acpu_l2_aux indicating
> > -		    an L2 cache auxiliary clock.
> > -
> > -Example:
> > -
> > -	l2cc: clock-controller@2011000 {
> > -		compatible = "qcom,kpss-gcc-ipq8064", "qcom,kpss-gcc";
> > -		reg = <0x2011000 0x1000>;
> > -		clocks = <&gcc PLL8_VOTE>, <&gcc PXO_SRC>;
> > -		clock-names = "pll8_vote", "pxo";
> > -		clock-output-names = "acpu_l2_aux";
> > -	};
> > diff --git a/Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.yaml b/Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.yaml
> > new file mode 100644
> > index 000000000000..7eb852be02c1
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/arm/msm/qcom,kpss-gcc.yaml
> > @@ -0,0 +1,69 @@
> > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/arm/msm/qcom,kpss-gcc.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Krait Processor Sub-system (KPSS) Global Clock Controller (GCC)
> > +
> > +maintainers:
> > +  - Ansuel Smith <ansuelsmth@gmail.com>
> > +
> > +description: |
> > +  Krait Processor Sub-system (KPSS) Global Clock Controller (GCC). Used
> > +  to control L2 mux (in the current implementation).
> > +
> > +properties:
> > +  compatible:
> > +    items:
> > +      - enum:
> > +          - qcom,kpss-gcc-ipq8064
> > +          - qcom,kpss-gcc-apq8064
> > +          - qcom,kpss-gcc-msm8974
> > +          - qcom,kpss-gcc-msm8960
> > +      - const: qcom,kpss-gcc
> > +      - const: syscon
> 
> There was no syscon here before. This is not explained in commit msg or
> patch history, while I asked to document explicitly any deviation from
> the conversion.
> 
> This is not how the process works. You keep making silent/hidden changes
> to the bindings and to the patch submission process. It's difficult to
> review and it is even more difficult to trust you that you implement
> what we ask for. You keep resending versions of the patchset the same
> day (two versions yesterday, shortly after another one) which does not
> give time to react and review. Plus then you hide some more changes to
> regular conversion without explaining them.
> 
> NAK. It's really bad process. :(
> 
> 
> Best regards,
> Krzysztof

The thing is that i'm trying to fix all the mess of years of keeping bad
Documentation and having dts that never followed Documentation. It's
really nothing silent/hidden. You add review tag to a patch? That won't
change. The bot alert me of some bugs? I push another revision with the
bug fixed. (I understand I should not send that much revision in the
same day but still considering the slow process of reviewing the c code,
I prefer to keep the Documentation part correct and ready)

If you notice the changes across the different patch, it's very minimal
and 99% of it has not changed. Nothing silent just me addressing warning
from the bot. About the trust issue...
Is it really a syscon addition that bad? Again the original
Documentation was just bad so why should we care to have a 100% 1:1
conversion if it should have been not accepted in the first place.
The addition of this new syscon is because in the current dtsi it's
there and I assume it's there as this is a global accessor and probably
other driver would access the same regs (so it's also a syscon)

I understand the complain about putting too much revision... But NAK
this cause I'm trying to fix all this mess just because more and more
problems are coming up and I'm trying to fix them. It's a bit sad.
Hope you can understand that it's not my interest to push silent changes
or other nasty stuff. It's just me fixing the mess and trying to at
least have the Documentation part ready while I wait for c code review.

-- 
	Ansuel