On 3/1/22 14:18, Lucas Stach wrote:
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.
The mxsfb currently has 1280 LoC as of patch 8/9 of this series. Of
that, there is some 400 LoC which are specific to old LCDIF and this
patch adds 380 LoC for the new LCDIF. So that's 800 LoC or ~60% of
shared boilerplate that would be duplicated .
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.
But then you also need to maintain two sets of boilerplate, they
diverge, and that is not good.
