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

[Marek Vasut]

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 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.

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.