Re: [PATCH 3/3] arm64: dts: qcom: msm8939-wingtech-wt82918: Add Lenovo Vibe K5 devices

[Nikita Travkin]

Nikita Travkin писал(а) 13.07.2024 15:37:
> Krzysztof Kozlowski писал(а) 13.07.2024 15:02:
>> On 12/07/2024 18:04, Nikita Travkin wrote:
>>> From: Adam Słaboń <asaillen@xxxxxxxxxxxxxx>
>>>
>>> This commit introduces multiple hardware variants of Lenovo Vibe K5.
>>>
>>> - A6020a40 (msm8929-wingtech-wt82918hd)
>>> - A6020a46/A6020l36 (msm8939-wingtech-wt82918)
>>> - A6020a40 S616 H39 (msm8939-wingtech-wt82918hd)
>>>
>>> These devices are added with support for many features, notably:
>>>
>>> - Basic features like USB, mmc/sd storage, wifi, buttons, leds;
>>> - Accelerometer;
>>> - Touchscreen;
>>> - Sound and modem.
>>>
>>> Note that "HD" variant of K5 is based on msm8929 which is a lower bin
>>> of msm8939 SoC. A simple dtsi is added for this soc along with the new
>>> devices.
>>>
>>> Unfortunately, despite the heavy similarities, the combination of minor
>>> differences between variants make them incompatible between each other.
>>>
>>> Signed-off-by: Adam Słaboń <asaillen@xxxxxxxxxxxxxx>
>>> [Nikita: Minor cleanup, commit message]
>>> Signed-off-by: Nikita Travkin <nikita@xxxxxxx>
>>> ---
>>>  arch/arm64/boot/dts/qcom/Makefile                  |   3 +
>>>  .../boot/dts/qcom/msm8929-wingtech-wt82918hd.dts   |  17 ++
>>>  arch/arm64/boot/dts/qcom/msm8929.dtsi              |   5 +
>>>  .../boot/dts/qcom/msm8939-wingtech-wt82918.dts     |  16 ++
>>>  .../boot/dts/qcom/msm8939-wingtech-wt82918.dtsi    | 254 +++++++++++++++++++++
>>>  .../boot/dts/qcom/msm8939-wingtech-wt82918hd.dts   |  16 ++
>>>  6 files changed, 311 insertions(+)
>>>
>>> diff --git a/arch/arm64/boot/dts/qcom/Makefile b/arch/arm64/boot/dts/qcom/Makefile
>>> index fd4c7c41ddc4..48ec781fa1d8 100644
>>> --- a/arch/arm64/boot/dts/qcom/Makefile
>>> +++ b/arch/arm64/boot/dts/qcom/Makefile
>>> @@ -58,10 +58,13 @@ dtb-$(CONFIG_ARCH_QCOM)	+= msm8916-wingtech-wt86518.dtb
>>>  dtb-$(CONFIG_ARCH_QCOM)	+= msm8916-wingtech-wt86528.dtb
>>>  dtb-$(CONFIG_ARCH_QCOM)	+= msm8916-wingtech-wt88047.dtb
>>>  dtb-$(CONFIG_ARCH_QCOM)	+= msm8916-yiming-uz801v3.dtb
>>> +dtb-$(CONFIG_ARCH_QCOM)	+= msm8929-wingtech-wt82918hd.dtb
>>>  dtb-$(CONFIG_ARCH_QCOM)	+= msm8939-huawei-kiwi.dtb
>>>  dtb-$(CONFIG_ARCH_QCOM)	+= msm8939-longcheer-l9100.dtb
>>>  dtb-$(CONFIG_ARCH_QCOM)	+= msm8939-samsung-a7.dtb
>>>  dtb-$(CONFIG_ARCH_QCOM)	+= msm8939-sony-xperia-kanuti-tulip.dtb
>>> +dtb-$(CONFIG_ARCH_QCOM)	+= msm8939-wingtech-wt82918.dtb
>>> +dtb-$(CONFIG_ARCH_QCOM)	+= msm8939-wingtech-wt82918hd.dtb
>>>  dtb-$(CONFIG_ARCH_QCOM)	+= msm8953-motorola-potter.dtb
>>>  dtb-$(CONFIG_ARCH_QCOM)	+= msm8953-xiaomi-daisy.dtb
>>>  dtb-$(CONFIG_ARCH_QCOM)	+= msm8953-xiaomi-mido.dtb
>>> diff --git a/arch/arm64/boot/dts/qcom/msm8929-wingtech-wt82918hd.dts b/arch/arm64/boot/dts/qcom/msm8929-wingtech-wt82918hd.dts
>>> new file mode 100644
>>> index 000000000000..f9a358e852f8
>>> --- /dev/null
>>> +++ b/arch/arm64/boot/dts/qcom/msm8929-wingtech-wt82918hd.dts
>>> @@ -0,0 +1,17 @@
>>> +// SPDX-License-Identifier: GPL-2.0-only
>>> +
>>> +/dts-v1/;
>>> +
>>> +#include "msm8939-wingtech-wt82918.dtsi"
>>> +#include "msm8929.dtsi"
>>> +
>>> +/ {
>>> +	model = "Lenovo Vibe K5 (HD) (Wingtech WT82918)";
>>> +	compatible = "wingtech,wt82918hd", "qcom,msm8929";
>>> +	chassis-type = "handset";
>>> +};
>>> +
>>> +&touchscreen {
>>> +	touchscreen-size-x = <720>;
>>> +	touchscreen-size-y = <1280>;
>>> +};
>>> diff --git a/arch/arm64/boot/dts/qcom/msm8929.dtsi b/arch/arm64/boot/dts/qcom/msm8929.dtsi
>>> new file mode 100644
>>> index 000000000000..c3d1d1ace2f6
>>> --- /dev/null
>>> +++ b/arch/arm64/boot/dts/qcom/msm8929.dtsi
>>> @@ -0,0 +1,5 @@
>>> +// SPDX-License-Identifier: GPL-2.0-only
>>> +
>>> +&opp_table {
>>> +	/delete-node/ opp-550000000;
>>> +};
>>
>> That's a very odd SoC DTSI.
>>
>> SoCs DTSIs are not meant to be included as complementary, but rather as
>> full DTSI.
>>
>> IOW, this is very confusing code and will confuse everyone reading it.
>>
> 
> I think Adam wanted to keep the common device dtsi based on msm8939.dtsi to
> simplify things a bit. I was also a bit unsure if I should change how it's
> done but decided to keep it as it was. I will rework the v2 so:
> 
> - msm8929.dtsi includes msm8939.dtsi
> - devices .dts include needed soc.dtsi, then include the common.dtsi
> - common.dtsi doesn't include any soc.dtsi
> 

(...) except gah this makes things quite a bit more complicated since the
device makes use of the "generic design" msm8939-pm8916.dtsi and duplicating
that would be quite silly IMO...

I wonder if we can clarify things without making everything too complicated 
by calling that dtsi "msm8929-opp.dtsi" and keeping it as extension for now,
then if we find that msm8929 has more differences - we can unfold and refactor
everything.

What do you think?

Nikita

> Thanks for the review!
> Nikita
> 
>>
>> Best regards,
>> Krzysztof