Re: [PATCH v6 17/18] dt-bindings: arm: msm: Convert kpss-gcc driver Documentation to yaml

[Krzysztof Kozlowski]

On 22/03/2022 14:26, Ansuel Smith wrote:
> 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@xxxxxxxxx>
>>> ---
>>>  .../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@xxxxxxxxx>
>>> +
>>> +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. 

It does not necessarily mean that bindings are bad and such changes
should be documented.

> (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)

Rob also pointed to this - sending two versions of this huge patchset
the same day is way too much.

> 
> 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.

Does not have to be 100% but deviations should be either expected or
explained. Bindings are used also outside of Linux kernel.

> 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)

If these are assumptions, then they need to be checked. If these were
new bindings, we would discuss/check the need of syscon. Now we do not
question existing properties, because they were accepted. But syscon
compatible was not accepted, so putting it here requires our acknowledgment.

The bindings are probably pure junk, so this is not merely a conversion
how you wrote in commit msg. This is rework of the bindings. Don't hide
rework under "conversion". Conversion is TXT->YAML without any changes...

I asked about this before and the only part you added to commit msg was
"clock-cells". And now I see syscon - so isn't it a bit surprising?

> 
> 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.

Why you cannot test your changes and fix them all before sending sixth
version? Why the bot has to test your code, not you?

> 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.
Best regards,
Krzysztof