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

From: Robby Cai <robby.cai@nxp.com>
To: Lucas Stach <l.stach@pengutronix.de>, Adam Ford <aford173@gmail.com>
Cc: Marek Vasut <marex@denx.de>,
	"Ying Liu (OSS)" <victor.liu@oss.nxp.com>,
	dri-devel <dri-devel@lists.freedesktop.org>,
	devicetree <devicetree@vger.kernel.org>,
	Peng Fan <peng.fan@nxp.com>,
	Alexander Stein <alexander.stein@ew.tq-group.com>,
	Rob Herring <robh+dt@kernel.org>,
	Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
	Sam Ravnborg <sam@ravnborg.org>
Subject: RE: [EXT] Re: [PATCH 1/9] dt-bindings: mxsfb: Add compatible for i.MX8MP
Date: Tue, 1 Mar 2022 13:37:45 +0000	[thread overview]
Message-ID: <VI1PR04MB699190CC6CB1D5C37C4BB64CF2029@VI1PR04MB6991.eurprd04.prod.outlook.com> (raw)
In-Reply-To: <4253aa4b5dc4a3568e45755678849961468bfd38.camel@pengutronix.de>

>-----Original Message-----
>From: Lucas Stach <l.stach@pengutronix.de>
>Sent: 2022年3月1日 21:19
>To: Adam Ford <aford173@gmail.com>
>Cc: Marek Vasut <marex@denx.de>; Ying Liu (OSS) <victor.liu@oss.nxp.com>;
>dri-devel <dri-devel@lists.freedesktop.org>; devicetree
><devicetree@vger.kernel.org>; Peng Fan <peng.fan@nxp.com>; Alexander Stein
><alexander.stein@ew.tq-group.com>; Rob Herring <robh+dt@kernel.org>;
>Laurent Pinchart <laurent.pinchart@ideasonboard.com>; Sam Ravnborg
><sam@ravnborg.org>; Robby Cai <robby.cai@nxp.com>
>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@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

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   