On 6/8/22 12:10, Caleb Connolly wrote:
I was surprised to see this series, and this patch especially.
An almost ready to submit version of this patch with considerably more
functionality has been sat around for a while but unfortunately never
sent [1].
Firstly, thank you for your review!
I'm not sure why that other patch series has never been submitted. It
was prepared 3 months ago (unbeknownst to me, at the time of v1), but
since then has not been submitted.
I would feel uncomfortable submitting that patch series, since I am not
familiar with parts of the full DT. In time I intend to be, but for now
I think we'd benefit from having a base DT mainlined, on top of which we
can iterate and parallelise.
According to the link below (and my own knowledge of PPP development)
Kamil is the original author of this patch, both Kamil and Martijn
created the initial version of the devicetree. Given that you're using
their work as a base, Kamil's authorship should be respected in the
patch you submit.
I agree authorship is important, and thus Kamil, Martijn and Megi are
listed as Co-developed-by in this patch.
Their original patch [2] contained SoBs from them and Martijn, those are
both missing below. Both of their signed-off-by tags should be added
before this patch hits the mailing list, and the same for Ondrej. The
order also seems wrong (Ondrej should be last before you).
Yes, this patch's acceptance is blocked until all Co-developed-by
authors (Kamil, Martjin, Megi) provide their Signed-off-by to this patch.
Support for the volume keys, accelerometer, magnetometer, GPIO LEDs,
touchscreen, modem codec and audio support are all missing here, but
they're included in the patches you referenced. In their current state
(see Martijn's commit [1] or my 5.19 rebase [3]) the DT for these
components has been worked on by several people, tested by the larger
user community, and are already supported in mainline. It seems strange
not to include them and just leads to a bunch of extra busywork to add
them back later. It's easy enough to drop any of these nodes during
review if they become an issue.
To keep this patch series light and easy-to-review, I'd be keen to keep
it as small as possible for now. This DT is >18 months old out-of-tree
(across multiple repos), so I think this minimal DT being mainlined is
an improvement over the status quo.
The existing work that the community has done will still be useful,
albeit in future patch series. This DT just allows that future work to
be done iteratively, and in parallel.
With that being said, I've left some feedback below, with it addressed
and the authorship/SoB situation sorted out:
Reviewed-by: Caleb Connolly <kc@xxxxxxxxxxxxxxxx>
Thank you for your comments, I appreciate your review! I will ensure v3
addresses those.
