Message-ID: <87a5zxshcq.fsf@minerva.mail-host-address-is-not-set>
Date:   Tue, 28 Mar 2023 05:08:21 +0200
From:   Javier Martinez Canillas <javierm@...hat.com>
To:     Ondřej Jirman <megi@....cz>
Cc:     Heiko Stübner <heiko@...ech.de>,
        linux-kernel@...r.kernel.org,
        Robert Mader <robert.mader@...labora.com>,
        Laurent Pinchart <laurent.pinchart@...asonboard.com>,
        Peter Robinson <pbrobinson@...il.com>,
        Jacopo Mondi <jacopo.mondi@...asonboard.com>,
        Martijn Braam <martijn@...xit.nl>,
        Kamil Trzciński <ayufan@...fan.eu>,
        Caleb Connolly <kc@...tmarketos.org>,
        Jarrah Gosbell <kernel@...ef.tools>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Rob Herring <robh+dt@...nel.org>,
        Tom Fitzhenry <tom@...-fitzhenry.me.uk>,
        devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-rockchip@...ts.infradead.org
Subject: Re: [PATCH v2] arm64: dts: rk3399-pinephone-pro: Add internal
 display support

Ondřej Jirman <megi@....cz> writes:

> On Mon, Mar 27, 2023 at 08:15:52PM +0200, Javier Martinez Canillas wrote:
>> Ondřej Jirman <megi@....cz> writes:

[...]

>> > Just because something is in my tree doesn't mean it's mine, or that I reviewed
>> > it in detail and prepared it for upstreaming, or that I'm interested in
>> 
>> Thanks for the clarification. Because the patch had your authorship I
>> wrongly assumed that came from you. Sorry about the confusion.
>
> Ever since base DT support for Pinephone Pro was merged, none of the DT patches
> in my tree are in the original form as prepared by the authors + fixes I've
> added. That's simply impossible anymore.
>
> To look up who did what, you'd have to look at older branches, pre-merge.
>
> Patches after the merge just came from squashing everything into one patch,
> cleaning it up, and re-splitting along some vague functionality boundaries,
> while trying to keep best-effort original SoBs as faithfully as possible, so
> that I can keep maintaining the PPP support in a sane manner.
>

Go it.

> Anyway, SoB's are added in chronological order. So:
>
> https://github.com/megous/linux/commit/471c5f33ba74c3d498ccc1eb69c098623b193926
>
> Means the author of the changes is Martijn + Kamil (roughly) and I may have
> a line of code in there too, since my SoB is last. For some reason, the order is
> inverted in the patch you posted, making it seem I developed these changes
> originally.
>

Since the patch had your authorship I changed the order so that your S-o-B
was first but I'll then change the author in v3 and make it match the
first S-o-B entry in your tree (Martijn) then.

>> > upstreaming it. I'm just trying to help you with your upstreaming effort by
>> > testing and review since I got to know the hardware quite well over the last
>> > years and can check the schematics and datasheets quickly, and I like to think
>> > upstream code is held to higher standard. That's all.
>> >
>> 
>> Appreciate your help and I agree that upstream code should be held to a
>> high standard. But since the DTS in mainline is pretty basic anyways (you
>> can only boot to serial right now), is not really usable for other thing
>> than development and keep adding the missing support.
>
> Having wrong frequency used is not a missing support for something. Sorry it's
> too bothersome to get the review piecemeal, but sometimes people have more time to
> look at patches in-depth, and at other times they don't and you just get surface
> level or no review.
>

Ok.

> One point of posting patches to the mailing list is to get review. And it's not
> that great to do in-depth review for you, up to going through schematics and
> datasheets, testing, and even proposing and testing solutions for found issues,
> just to be dismissed without technical reason.
>
> The thing is this review will most likely happen just once, and noone will go
> back, read through the entire huge DT along with a schematic, to look at whether
> this or that pullup is really necessary, whether some parameter is out of spec
> from the datasheet for each part, or things like that. That's just not
> pragmatic.
>

That's fair.

> Instead, people will happily attribute non-obvious issues caused by these
> omissions of manual review to shoddy or slow or power-inefficient HW. "1kHz
> + harmonics interference in codec because high power backlight DC-DC regulator
> basically spews off 1kHz of 1-2W load + harmonics because it's driven
> incorrectly? Ah, the phone just has a shitty audio codec!"
>
> So, don't take it as some perfectionism. Upstreaming just seems like the best
> time to look at things that were overlooked in the past in more detail and get
> these little things right, because the DT additions are done piecemal and
> slowly/gradually, so it's more manageable to review and fix right away. This
> will just not happen later on for these obscure devices like Pinephone Pro,
> where the whole effort that goes into it is like one half of a fulltime
> developer time split over 4 mildly interested real persons, slowly tapering off
> over time as the device ages.
>

Makes sense. I'll address your comments in v3 then and also include a
separate patch (again with Martijn as author and the S-o-B as ordered in
your tree), that includes the touchscreen DT nodes as well. Since Jarrah
pointed out that there's already the correct compatible string in the
driver's OF device ID table.

> regards,
> 	o.
>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Powered by blists - more mailing lists