Message-ID: <20230327194518.qkm5qgap6vkivpeg@core>
Date:   Mon, 27 Mar 2023 21:45:18 +0200
From:   Ondřej Jirman <megi@....cz>
To:     Javier Martinez Canillas <javierm@...hat.com>
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

On Mon, Mar 27, 2023 at 08:15:52PM +0200, Javier Martinez Canillas wrote:
> Ondřej Jirman <megi@....cz> writes:
> 
> > On Mon, Mar 27, 2023 at 06:15:55PM +0200, Javier Martinez Canillas wrote:
> 
> [...]
> 
> >> 
> >> It is broken though? This is what is in Ondrej downstream tree and I see
> >> no issues on my Pinephone Pro. He mentioned some flicker when looking at
> >> the signals with a scope and hooking a photoresistor.
> >
> > LED regulator is driven out of spec by a frequency that's 20x lower than
> > recommended, if you want short version of what's broken about the DT patch.
> >
> >> But that's fair. I'll let Ondrej then post a v3 if he wants to address the
> >> issues he pointed out, since is his patch after all.
> >
> > It's not my patch. Original author of the DT is Martijn or Kamil. I just carry
> > their DT work in split-up patches in my tree, and I sometimes try to find solutions
> > to bugs I find when using PPP. That's the story of these DT changes you're posting.
> >
> > Since you posted this DT patch for upstreaming, I wanted to help you by reviewed
> > it more completely, so I opened the schematic and datasheets for the components
> > that are described in this patch, and discovered these new issues I commented
> > about. And I also tested it on top of linus/master.
> >
> > 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.

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.

> > 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.

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.

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.

regards,
	o.

> So I thought that we could do it in steps without creating that much work
> for the people trying to post the downstream patches and having to re-spin
> too many times.
> 
> -- 
> Best regards,
> 
> Javier Martinez Canillas
> Core Platforms
> Red Hat
> 

Powered by blists - more mailing lists