[4/4] ARM: dts: qcom: msm8974: Add device tree for Samsung Galaxy S5 China

  This device has little difference compared to Samsung Galaxy S5 (klte),
so the device tree is based on qcom-msm8974pro-samsung-klte.dts. The
only difference is the gpio pins of i2c_led_gpio. With pins corrected,
the LEDs and WiFi are able to work properly.

Signed-off-by: Rong Zhang <i@rong.moe>
---
 arch/arm/boot/dts/qcom/Makefile                  |  1 +
 .../dts/qcom/qcom-msm8974pro-samsung-kltechn.dts | 16 ++++++++++++++++
 2 files changed, 17 insertions(+)
 create mode 100644 arch/arm/boot/dts/qcom/qcom-msm8974pro-samsung-kltechn.dts
  

On 21/01/2024 16:39, Rong Zhang wrote:
> This device has little difference compared to Samsung Galaxy S5 (klte),
> so the device tree is based on qcom-msm8974pro-samsung-klte.dts. The
> only difference is the gpio pins of i2c_led_gpio. With pins corrected,
> the LEDs and WiFi are able to work properly.
> 
> Signed-off-by: Rong Zhang <i@rong.moe>


> diff --git a/arch/arm/boot/dts/qcom/qcom-msm8974pro-samsung-kltechn.dts b/arch/arm/boot/dts/qcom/qcom-msm8974pro-samsung-kltechn.dts
> new file mode 100644
> index 000000000000..5a8d59ea4439
> --- /dev/null
> +++ b/arch/arm/boot/dts/qcom/qcom-msm8974pro-samsung-kltechn.dts
> @@ -0,0 +1,16 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include "qcom-msm8974pro-samsung-klte.dts"
> +
> +/ {
> +	model = "Samsung Galaxy S5 China";
> +	compatible = "samsung,kltechn", "samsung,klte", "qcom,msm8974pro", "qcom,msm8974";

That's not what you said in the binding.

It does not look like you tested the DTS against bindings. Please run
`make dtbs_check W=1` (see
Documentation/devicetree/bindings/writing-schema.rst or
https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/
for instructions).

Best regards,
Krzysztof
  

On 21.01.2024 16:39, Rong Zhang wrote:
> This device has little difference compared to Samsung Galaxy S5 (klte),
> so the device tree is based on qcom-msm8974pro-samsung-klte.dts. The
> only difference is the gpio pins of i2c_led_gpio. With pins corrected,
> the LEDs and WiFi are able to work properly.
> 
> Signed-off-by: Rong Zhang <i@rong.moe>
> ---

Looks like you didn't change the brcm,board-type though?

[...]

> +++ b/arch/arm/boot/dts/qcom/qcom-msm8974pro-samsung-kltechn.dts
> @@ -0,0 +1,16 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include "qcom-msm8974pro-samsung-klte.dts"

It's customary not to include .dts files, instead split the common parts
into e.g. qcom-msm8974pro-samsung-klte-common.dtsi and include this in
both the existing and the new one.

Konrad
  

在 2024-01-22星期一的 11:50 +0100，Konrad Dybcio写道：
> On 21.01.2024 16:39, Rong Zhang wrote:
> > This device has little difference compared to Samsung Galaxy S5
> > (klte),
> > so the device tree is based on qcom-msm8974pro-samsung-klte.dts.
> > The
> > only difference is the gpio pins of i2c_led_gpio. With pins
> > corrected,
> > the LEDs and WiFi are able to work properly.
> > 
> > Signed-off-by: Rong Zhang <i@rong.moe>
> > ---
> 
> Looks like you didn't change the brcm,board-type though?

This should be intentional to allow kltechn and klte to share Wi-Fi
NVRAM file.

> 
> [...]
> 
> > +++ b/arch/arm/boot/dts/qcom/qcom-msm8974pro-samsung-kltechn.dts
> > @@ -0,0 +1,16 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +#include "qcom-msm8974pro-samsung-klte.dts"
> 
> It's customary not to include .dts files, instead split the common
> parts
> into e.g. qcom-msm8974pro-samsung-klte-common.dtsi and include this
> in
> both the existing and the new one.
> 
> Konrad
  

On Mon, 2024-01-22 at 10:48 +0100, Krzysztof Kozlowski wrote:
> On 21/01/2024 16:39, Rong Zhang wrote:
> > This device has little difference compared to Samsung Galaxy S5 (klte),
> > so the device tree is based on qcom-msm8974pro-samsung-klte.dts. The
> > only difference is the gpio pins of i2c_led_gpio. With pins corrected,
> > the LEDs and WiFi are able to work properly.
> > 
> > Signed-off-by: Rong Zhang <i@rong.moe>
> 
> 
> > diff --git a/arch/arm/boot/dts/qcom/qcom-msm8974pro-samsung-kltechn.dts b/arch/arm/boot/dts/qcom/qcom-msm8974pro-samsung-kltechn.dts
> > new file mode 100644
> > index 000000000000..5a8d59ea4439
> > --- /dev/null
> > +++ b/arch/arm/boot/dts/qcom/qcom-msm8974pro-samsung-kltechn.dts
> > @@ -0,0 +1,16 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +#include "qcom-msm8974pro-samsung-klte.dts"
> > +
> > +/ {
> > +	model = "Samsung Galaxy S5 China";
> > +	compatible = "samsung,kltechn", "samsung,klte", "qcom,msm8974pro", "qcom,msm8974";
> 
> That's not what you said in the binding.
> 
> It does not look like you tested the DTS against bindings. Please run
> `make dtbs_check W=1` (see
> Documentation/devicetree/bindings/writing-schema.rst or
> https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/
> for instructions).

Oops, I've forgot to run dtbs_check again after my final decision of
adding "samsung,klte". Thanks for pointing it out.

I added it because I thought the difference between klte and kltechn is
so tiny and I've seen some other dts doing that.

I've glanced similar dts. To solve this, I think we could either:
1. keep the dt-binding in [PATCH 3/4], and delete "samsung,klte" here
2. rewrite dt-binding like crystalfontz,cfa100{36,37,49,55,56,57,58}:

diff --git a/Documentation/devicetree/bindings/arm/qcom.yaml b/Documentation/devicetree/bindings/arm/qcom.yaml
index 2bd29a2399ad..4979ccae2b64 100644
--- a/Documentation/devicetree/bindings/arm/qcom.yaml
+++ b/Documentation/devicetree/bindings/arm/qcom.yaml
@@ -223,11 +223,17 @@ properties:
               - fairphone,fp2
               - oneplus,bacon
               - samsung,klte
-              - samsung,kltechn
               - sony,xperia-castor
           - const: qcom,msm8974pro
           - const: qcom,msm8974
 
+      - items:
+          - enum:
+              - samsung,kltechn
+          - const: samsung,klte
+          - const: qcom,msm8974pro
+          - const: qcom,msm8974
+
       - items:
           - const: qcom,msm8916-mtp
           - const: qcom,msm8916-mtp/1

My preference is (2.) since other variants of klte may be added in the
future. I would like to hear your preferences.

Thanks,
Rong


> Best regards,
> Krzysztof
>
  

On 22/01/2024 15:51, Rong Zhang wrote:
> I've glanced similar dts. To solve this, I think we could either:
> 1. keep the dt-binding in [PATCH 3/4], and delete "samsung,klte" here
> 2. rewrite dt-binding like crystalfontz,cfa100{36,37,49,55,56,57,58}:
> 
> diff --git a/Documentation/devicetree/bindings/arm/qcom.yaml b/Documentation/devicetree/bindings/arm/qcom.yaml
> index 2bd29a2399ad..4979ccae2b64 100644
> --- a/Documentation/devicetree/bindings/arm/qcom.yaml
> +++ b/Documentation/devicetree/bindings/arm/qcom.yaml
> @@ -223,11 +223,17 @@ properties:
>                - fairphone,fp2
>                - oneplus,bacon
>                - samsung,klte
> -              - samsung,kltechn
>                - sony,xperia-castor
>            - const: qcom,msm8974pro
>            - const: qcom,msm8974
>  
> +      - items:
> +          - enum:
> +              - samsung,kltechn
> +          - const: samsung,klte
> +          - const: qcom,msm8974pro
> +          - const: qcom,msm8974
> +
>        - items:
>            - const: qcom,msm8916-mtp
>            - const: qcom,msm8916-mtp/1
> 
> My preference is (2.) since other variants of klte may be added in the
> future. I would like to hear your preferences.

It depends whether the devices are compatible. IOW, entire klte DTB
should work fine on kltechn, just without few new devices.

Best regards,
Krzysztof
  

On Mon, 2024-01-22 at 16:17 +0100, Krzysztof Kozlowski wrote:
> On 22/01/2024 15:51, Rong Zhang wrote:
> > I've glanced similar dts. To solve this, I think we could either:
> > 1. keep the dt-binding in [PATCH 3/4], and delete "samsung,klte" here
> > 2. rewrite dt-binding like crystalfontz,cfa100{36,37,49,55,56,57,58}:
[...]
> > My preference is (2.) since other variants of klte may be added in the
> > future. I would like to hear your preferences.
> 
> It depends whether the devices are compatible. IOW, entire klte DTB
> should work fine on kltechn, just without few new devices.

Yes, they are compatible. I'd used the klte DTB on kltechn for a long
time before I made this patchset. It worked totally fine expect for
LEDs and WiFi, as I've said in the cover letter.

Thanks,
Rong


> Best regards,
> Krzysztof
>
  

On Mon, 2024-01-22 at 11:50 +0100, Konrad Dybcio wrote:
> Looks like you didn't change the brcm,board-type though?

In "[PATCH 2/4] ARM: dts: qcom: msm8974-samsung-klte: Pin brcm,board-
type in wifi":

   /*
    * This aims to allow other klte* variants to load the same firmware,
    * as klte variants have little differences in the wifi part.
    */

So it is intentional, in order to let them share the same FW, in
particular, the NVRAM file.

Without [PATCH 2/4] and with this [PATCH 4/4]:
- klte DT probes "brcmfmac*.samsung,klte.txt"
- kltechn DT probes "brcmfmac*.samsung,kltechn.txt", but never probes
"brcmfmac*.samsung,klte.txt"

With both [PATCH 2/4] and this [PATCH 4/4]:
- klte DT probes "brcmfmac*.samsung,klte.txt"
- kltechn DT probes "brcmfmac*.samsung,klte.txt"

I pinned "brcm,board-type" in the klte DT instead of the kltechn one
because other klte* variants are known to have little difference in the
WiFi part. By pinning it in the klte DT, future ports could be easier.

If you'd prefer not doing this, I am OK to drop [PATCH 2/4] in the v2
patchset.

FYI:

LineageOS considers all klte variants use "common" WiFi FW:
https://github.com/search?q=repo%3ALineageOS%2Fandroid_kernel_samsung_klte+CONFIG_BCMDHD_NVRAM_PATH&type=code
https://github.com/LineageOS/android_device_samsung_klte-common/blob/8f71a63415397def5ba886f4030b0d91e2253262/common-proprietary-files.txt#L251

I've tested the klte port of PostmarketOS (with klte FW and this
patchset) on kltechn, and it worked fine. For the FW used by
PostmarketOS, see also:
https://gitlab.com/postmarketOS/pmaports/-/blob/439227770ffcd32eb7e26436598c804dc35637ad/device/testing/firmware-samsung-klte/APKBUILD#L17


> > +++ b/arch/arm/boot/dts/qcom/qcom-msm8974pro-samsung-kltechn.dts
> > @@ -0,0 +1,16 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +#include "qcom-msm8974pro-samsung-klte.dts"
> 
> It's customary not to include .dts files, instead split the common parts
> into e.g. qcom-msm8974pro-samsung-klte-common.dtsi and include this in
> both the existing and the new one.

At the very beginning, I thought including .dts could make the patchset
tiny (considering that the difference among klte* is also tiny).

I agree that splitting the common parts into a .dtsi will make things
more elegant and make klte* DTs consistent with the style of qcom DTs.

Will do in v2. Thanks for your advice.

Thanks,
Rong