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

From: Krzysztof Kozlowski <krzk@kernel.org>
To: Ansuel Smith <ansuelsmth@gmail.com>
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
Date: Wed, 23 Mar 2022 15:19:54 +0100	[thread overview]
Message-ID: <1d3e9846-f092-09c6-e4ef-7a52d61613f1@kernel.org> (raw)
In-Reply-To: <Yjr+nHBFqNqMV+v0@Ansuel-xps.localdomain>

On 23/03/2022 12:03, Ansuel Smith wrote:
>>>
>>> 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.
>>
> 
> About this I have a question. If the dts already have some binding and
> the Documentation doesn't have them. Should the dts have priority or the
> Documentation?

Depends, usually yes, if the DTS is being actually used. There might be
some exceptions, though. The priority is for the ones which are correct.

Judging by current DTS, the syscon is indeed used in DTS and documented
in bindings. It might be deprecated, because one binding is saying that
mailboxes can be used instead.

Here your call to add syscon looks correct. Just please document it in
commit msg, why it has to be added (there are real users of it: Qualcomm
RPM).

> In the case where we can't prove that syscon is needed (for example), can
> we remove it from dts (and accept to have inconsistency while the dts
> changes are merged) or we should add the extra binding to the
> Documentation putting some comments about it and discussing the
> inclusion? 
> 
>> 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...
>>
> 
> Ok, thanks for the clarification. I still think should be handled with
> conversion + additional commit to add the missing part so I have to fix
> my wrong commits.
> 
>> 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?
>>
> 
> You are right... I will just do the 1:1 conversion and put all these
> addition to a separate commit to make them clear.
> 
>>>
>>> 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?
>>
> 
> I'm aksed Rob if there is a quicker way to test single Documenation and
> dts but it's my fault anyway.

make -j8 dt_binding_check DT_SCHEMA_FILES="qcom-kpss"

make defconfig (or allyesconfig etc)
make -j8 dtbs_check DT_SCHEMA_FILES="qcom-kpss"

Best regards,
Krzysztof