Re: [PATCH v2 2/3] arm64: dts: qcom: msm8916-samsung-fortuna: Add initial device trees

[Bjorn Andersson]

On Tue, Aug 01, 2023 at 11:22:47AM +0000, Lin, Meng-Bo wrote:
> From: Walter Broemeling <wallebroem@xxxxxxxxx>
> 
> Samsung Galaxy Core Prime, Grand Prime and Ace 4 are phones based on
> MSM8916. They are similar to the other Samsung devices based on MSM8916
> with only a few minor differences.
> 
> This initial commit adds support for:
>  - fortuna3g (SM-G530H)
>  - fortunaltezt (SM-G530Y)
>  - gprimeltecan (SM-G530W)
>  - grandprimelte (SM-G530FZ)
>  - heatqlte (SM-G357FZ)
>  - rossa (SM-G360G)
> 
> The device trees contain initial support with:
>  - GPIO keys
>  - Regulator haptic
>  - SDHCI (internal and external storage)
>  - USB Device Mode
>  - UART (on USB connector via the SM5502/SM5504 MUIC)
>  - WCNSS (WiFi/BT)
>  - Regulators
> 
> There are different variants of Grand Prime, with some differences
> in accelerometer, NFC and panel.
> Core Prime and Grand Prime are similar, with some differences in MUIC,
> panel and touchscreen.
> Ace 4 and Core Prime are similar, with some differences in panel and
> touchscreen.
> 
> The common parts are shared in
> msm8916-samsung-fortuna-common.dtsi and msm8916-samsung-rossa-common.dtsi
> to reduce duplication.
> 
> Unfortunately, SM-G357FZ and SM-G530Y were released with outdated 32-bit
> only firmware and never received any update from Samsung. Since the 32-bit
> TrustZone firmware is signed there seems to be no way currently to
> actually boot this device tree on arm64 Linux at the moment.
> 
> However, it is possible to use this device tree by compiling an ARM32
> kernel instead. The device tree can be easily built on ARM32 with
> an #include and it works really well there. To avoid confusion for others
> it is still better to add this device tree on arm64. Otherwise it's easy
> to forget to update this one when making some changes that affect all
> MSM8916 devices.
> 
> Maybe someone finds a way to boot ARM64 Linux on this device at some
> point. In this case I expect that this device tree can be simply used
> as-is.
> 

Can you please help me understand the development flow of this patch?

> Signed-off-by: Walter Broemeling <wallebroem@xxxxxxxxx>
> Co-developed-by: Stephan Gerhold <stephan@xxxxxxxxxxx>
> Signed-off-by: Stephan Gerhold <stephan@xxxxxxxxxxx>

Walter and Stephan wrote the initial patch, right?

> [Add fortuna-common.dtsi, buttons, and WiFi]
> Co-developed-by: Joe Mason <buddyjojo06@xxxxxxxxxxx>
> Signed-off-by: Joe Mason <buddyjojo06@xxxxxxxxxxx>

Then Joe added fortuna-common, buttons and Wifi.

If so, then Joe shouldn't be "Co-developed-by", the [note] and
Signed-off-by is sufficient here.

But it is customary to prefix the "changes note" with ones first name,
such as:

[joe: Add fortuna-common.dtsi, buttons, and WiFi]

> [Add fortuna3g]
> Co-developed-by: Siddharth Manthan <siddharth.manthan@xxxxxxxxx>
> Signed-off-by: Siddharth Manthan <siddharth.manthan@xxxxxxxxx>

Then Siddharth picked it up, ad added fortuna3g. Again, it looks like he
did this step alone, and as such no Co-developed-by, please.

> [Add heatqlte]
> Co-developed-by: Gareth Peoples <mail@xxxxxxxxx>
> Signed-off-by: Gareth Peoples <mail@xxxxxxxxx>

Again, no Co-developed-by, and please prefix the change note with
"Gareth:", or "gareth:".

> [Add grandprimelte and fortunaltezt]
> [Use msm8916-samsung-rossa-common.dtsi and reword the commit]

Why two different notes? Is this one note split over two separate
entries? Please just comma-separate them, possible line wrap within the
[].

> Co-developed-by: Lin, Meng-Bo <linmengbo0689@xxxxxxxxxxxxxx>
> Signed-off-by: Lin, Meng-Bo <linmengbo0689@xxxxxxxxxxxxxx>

You should be alone here.


Alternatively, if y'all all contributed to this one patch through the
entire flow, please drop the change notes and just list out each
contributor with the Co-developed-by and Signed-off-by.


PS. Could you please drop the ',' from your name. When I tried to apply
this everything after the ',' disappeared.

Regards,
Bjorn