On Mon, Mar 27, 2023 at 08:15:52PM +0200, Javier Martinez Canillas wrote: > Ondřej Jirman <megi@xxxxxx> 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 >
