[PATCH 1/9] dt-bindings: mxsfb: Add compatible for i.MX8MP

[Lucas Stach]

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 at pengutronix.de> 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