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

[Nikita Travkin]

Dmitry Baryshkov писал(а) 13.07.2024 21:12:
> On Sat, Jul 13, 2024 at 04:07:13PM GMT, Nikita Travkin wrote:
>> 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.
>> >>>
> 
>> >>> 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?
> 
> What about adding msm8929-pm8916.dtsi, which includes just the right
> things? This might result in duplication with the existing files, but in
> the end msm8939-pm8916 and msm8919-pm8916 are also very similar.

Right, I guess the reason my thought was to avoid it is that msm8929 is
(seemingly) just a bin of msm8939, compared to i.e. msm8916 which is a
different soc.

But I suppose it's fine to create a new dtsi for it too, will create it
and change the includes as suggested (soc+pmic dtsi is included by
device, then common dtsi that itself doesn't include soc dtsi)

Thanks!
Nikita