On Fri, Nov 08, 2024 at 11:53:57AM +0000, Andre Przywara wrote: > Hi John, Hi Andre! > Can you say *why* this patch is needed? Is there something broken that > needs fixing? Where does this show and why wasn't this a problem before? Oops, that's a good point. There is currently a bug where the LCD output will be tinted. I have full context here which I should have probably linked in the patch description: https://lore.kernel.org/linux-sunxi/Zn8GVkpwXwhaUFno@titan/T/#u > To be honest, given the isolation on this patch, I'd rather wait for this > full fledged solution, especially if there is no pressing need (see above). I'd be interested to hear if that's still the wanted solution given the link above. This currently blocks many people from having working LCD output. Doing it the proper way might be overkill for now unless someone deliberately tries to run two DEs to the same output. I haven't seen this use case. Allwinner kernel fork initially sets them up to values like these then makes sure both DEs can't be set to the same TCON. > > - writel(0, regs + TCON_TOP_PORT_SEL_REG); > > + writel(0x20, regs + TCON_TOP_PORT_SEL_REG); > > Sorry, but that looks weird: > First, please explain the 0x20. Is it bit 5? If yes, what does that bit > mean? The commit message suggests you know that? > > And second: the comment above clearly states that those two writes just > *clear* some registers, to have some sane base line. So please adjust this > comment, and copy in some of the rationale from the commit message. > Explaining things in the commit message is good (so thanks for that!), but > having at least some terse technical explanations near the code, in a > comment, is better. Bit 5 is value 3 of TCON_TOP_PORT_DE1_MSK. The R40 datasheet explains the values of both masks as follows: 00: TCON_LCD0 01: TCON_LCD1 10: TCON_TV0 11: TCON_TV1 So this sets DE1's input to DE0. > > Cheers, > Andre Thanks, John Watts
