Am Dienstag, dem 01.03.2022 um 07:03 -0600 schrieb Adam Ford: > On Tue, Mar 1, 2022 at 5:05 AM Lucas Stach <l.stach@xxxxxxxxxxxxxx> wrote: > > > > Am Dienstag, dem 01.03.2022 um 11:19 +0100 schrieb Marek Vasut: > > > On 3/1/22 11:04, Lucas Stach wrote: > > > > > > Hi, > > > > > > [...] > > > > > > > > Given the two totally different IPs, I don't see bugs of IP control > > > > > logics should be fixed for both drivers. Naturally, the two would > > > > > diverge due to different HWs. Looking at Patch 9/9, it basically > > > > > squashes code to control LCDIFv3 into the mxsfb drm driver with > > > > > 'if/else' checks(barely no common control code), which is hard to > > > > > maintain and not able to achieve good scalability for both 'LCDIFv3' > > > > > and 'LCDIF'. > > > > > > > > I tend to agree with Liu here. Writing a DRM driver isn't that much > > > > boilerplate anymore with all the helpers we have available in the > > > > framework today. > > > > > > I did write a separate driver for this IP before I spent time merging > > > them into single driver, that's when I realized a single driver is much > > > better and discarded the separate driver idea. > > > > > > > The IP is so different from the currently supported LCDIF controllers > > > > that I think trying to support this one in the existing driver actually > > > > increases the chances to break something when modifying the driver in > > > > the future. Not everyone is able to test all LCDIF versions. My vote is > > > > on having a separate driver for the i.MX8MP LCDIF. > > > > > > If you look at both controllers, it is clear it is still the LCDIF > > > behind, even the CSC that is bolted on would suggest that. > > > > Yes, but from a driver PoV what you care about is not really the > > hardware blocks used to implement something, but the programming model, > > i.e. the register interface exposed to software. > > > > > > > > I am also not happy when I look at the amount of duplication a separate > > > driver would create, it will be some 50% of the code that would be just > > > duplicated. > > > > > Yea, the duplicated code is still significant, as the HW itself is so > > simple. However, if you find yourself in the situation where basically > > every actual register access in the driver ends up being in a "if (some > > HW rev) ... " clause, i still think it would be better to have a > > separate driver, as the programming interface is just different. > > I tend to agree with Marek on this one. We have an instance where the > blk-ctrl and the GPC driver between 8m, mini, nano, plus are close, > but different enough where each SoC has it's own set of tables and > some checks. Lucas created the framework, and others adapted it for > various SoC's. If there really is nearly 50% common code for the > LCDIF, why not either leave the driver as one or split the common code > into its own driver like lcdif-common and then have smaller drivers > that handle their specific variations. I don't know exactly how the standalone driver looks like, but I guess the overlap is not really in any real HW specific parts, but the common DRM boilerplate, so there isn't much point in creating a common lcdif driver. As you brought up the blk-ctrl as an example: I'm all for supporting slightly different hardware in the same driver, as long as the HW interface is close enough. But then I also opted for a separate 8MP blk-ctrl driver for those blk-ctrls that differ significantly from the others, as I think it would make the common driver unmaintainable trying to support all the different variants in one driver. Regards, Lucas
