Re: [PATCH v2 2/3] arm64: dts: qcom: sdm845-xiaomi-beryllium-common: move common nodes to a common dtsi

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 01/08/2022 20:53, Marijn Suijten wrote:
> On 2022-08-01 16:55:11, Joel Selvaraj wrote:
>> Since there are two variants of Xiaomi Poco F1, move the common nodes from
>> Tianma variant into a new common dtsi. The EBBG variant will also inherit
>> the new common dtsi.
>>
>> Signed-off-by: Joel Selvaraj <joel.selvaraj@outlook.com>
>> ---
> 
> Any summary what changed since v1?

+1 (although I already reviewed it)

> 
>>  .../qcom/sdm845-xiaomi-beryllium-common.dtsi  | 595 ++++++++++++++++++
>>  .../qcom/sdm845-xiaomi-beryllium-tianma.dts   | 590 +----------------
>>  2 files changed, 598 insertions(+), 587 deletions(-)
>>  create mode 100644 arch/arm64/boot/dts/qcom/sdm845-xiaomi-beryllium-common.dtsi
>>
>> diff --git a/arch/arm64/boot/dts/qcom/sdm845-xiaomi-beryllium-common.dtsi b/arch/arm64/boot/dts/qcom/sdm845-xiaomi-beryllium-common.dtsi
>> new file mode 100644
>> index 000000000000..83edcb1171f5
>> --- /dev/null
>> +++ b/arch/arm64/boot/dts/qcom/sdm845-xiaomi-beryllium-common.dtsi
> 
> I haven't re-read what was discussed in v1, but doing it this way causes
> git to _not_ record this as a rename but instead state that everything
> has been removed from sdm845-xiaomi-beryllium-tianma.dts, and a new file
> sdm845-xiaomi-beryllium-common.dtsi was introduced with inconveniently
> almost identical contents (see the unnecessary size of the patch that
> follows).

The patch should be formatted a bit different. I agree that if combined
with first patch and proper settings (-M10% -C10%, optionally also
experiment with -B although here it looks not needed).

I reviewed the diff side-by-sie and there were differences (labels)
tricky to spot. If you generate the patch correctly, not much of review
is needed...

> 
> Instead, I'd keep the original patch with a rename from
> sdm845-xiaomi-beryllium.dts to sdm845-xiaomi-beryllium-common.dtsi, and
> _also_ update the existing:
> 
> 	dtb-$(CONFIG_ARCH_QCOM)	+= sdm845-xiaomi-beryllium.dtb
> 
> in Makefile to match this rename so that it keeps compiling, even if
> that means we treat a .dtsi as a .dts which may (likely) not be treated
> correctly by existing build rules.
> 
> If it doesn't - and this approach is probably frowned upon anyway - it
> is perhaps easiest to generalize sdm845-xiaomi-beryllium.dtb (as
> suggested above) _and_ introduce sdm845-xiaomi-beryllium-tianma.dtb
> _and_ update Makefile in a _single_ patch, such that everyting keeps
> compiling and stays consistent wrt how git treats renames.  Later
> patches update the compatible and add the ebbg variant.
> 
> - Marijn


Best regards,
Krzysztof