>-----Original Message----- >From: Lucas Stach <l.stach@xxxxxxxxxxxxxx> >Sent: 2022年3月1日 21:19 >To: Adam Ford <aford173@xxxxxxxxx> >Cc: Marek Vasut <marex@xxxxxxx>; Ying Liu (OSS) <victor.liu@xxxxxxxxxxx>; >dri-devel <dri-devel@xxxxxxxxxxxxxxxxxxxxx>; devicetree ><devicetree@xxxxxxxxxxxxxxx>; Peng Fan <peng.fan@xxxxxxx>; Alexander Stein ><alexander.stein@xxxxxxxxxxxxxxx>; Rob Herring <robh+dt@xxxxxxxxxx>; >Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>; Sam Ravnborg ><sam@xxxxxxxxxxxx>; Robby Cai <robby.cai@xxxxxxx> >Subject: [EXT] Re: [PATCH 1/9] dt-bindings: mxsfb: Add compatible for >i.MX8MP > >Caution: EXT Email > >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 LCDIF on i.MX8MP is a different IP which is borrowed from non-iMX series, although it's also called 'LCDIF'. We prefer not mix these two series of IPs in one driver for ease of maintenance and extension. Regards, Robby
