From: Adam Ford <aford173@gmail.com> To: Lucas Stach <l.stach@pengutronix.de> Cc: Marek Vasut <marex@denx.de>, devicetree <devicetree@vger.kernel.org>, Peng Fan <peng.fan@nxp.com>, Alexander Stein <alexander.stein@ew.tq-group.com>, dri-devel <dri-devel@lists.freedesktop.org>, Liu Ying <victor.liu@oss.nxp.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: Re: [PATCH 1/9] dt-bindings: mxsfb: Add compatible for i.MX8MP Date: Tue, 1 Mar 2022 07:03:26 -0600 [thread overview] Message-ID: <CAHCN7xJ6ypDxZouZV1b1F1EgQFwdTvmY6EEekj+_z-UWbQMD5Q@mail.gmail.com> (raw) In-Reply-To: <8bf0b5a1c9ab9faee28077436cdfd49c0cd08792.camel@pengutronix.de> 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. adam > > Regards, > Lucas > >
WARNING: multiple messages have this Message-ID (diff)
From: Adam Ford <aford173@gmail.com> To: Lucas Stach <l.stach@pengutronix.de> Cc: Marek Vasut <marex@denx.de>, Liu Ying <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: Re: [PATCH 1/9] dt-bindings: mxsfb: Add compatible for i.MX8MP Date: Tue, 1 Mar 2022 07:03:26 -0600 [thread overview] Message-ID: <CAHCN7xJ6ypDxZouZV1b1F1EgQFwdTvmY6EEekj+_z-UWbQMD5Q@mail.gmail.com> (raw) In-Reply-To: <8bf0b5a1c9ab9faee28077436cdfd49c0cd08792.camel@pengutronix.de> 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. adam > > Regards, > Lucas > >
next prev parent reply other threads:[~2022-03-01 13:03 UTC|newest] Thread overview: 61+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-02-28 0:45 [PATCH 1/9] dt-bindings: mxsfb: Add compatible for i.MX8MP Marek Vasut 2022-02-28 0:45 ` Marek Vasut 2022-02-28 0:45 ` [PATCH 2/9] drm: mxsfb: Simplify LCDIF clock handling Marek Vasut 2022-02-28 0:45 ` [PATCH 3/9] drm: mxsfb: Simplify LCDIF IRQ handling Marek Vasut 2022-02-28 0:46 ` [PATCH 4/9] drm: mxsfb: Wrap FIFO reset and comments into mxsfb_reset_block() Marek Vasut 2022-02-28 0:46 ` [PATCH 5/9] drm: mxsfb: Move mxsfb_get_fb_paddr() away from register IO functions Marek Vasut 2022-02-28 0:46 ` [PATCH 6/9] drm: mxsfb: Factor out mxsfb_set_mode() Marek Vasut 2022-02-28 0:46 ` [PATCH 7/9] drm: mxsfb: Reorder mxsfb_crtc_mode_set_nofb() Marek Vasut 2022-02-28 0:46 ` [PATCH 8/9] drm: mxsfb: Factor out mxsfb_update_buffer() Marek Vasut 2022-02-28 0:46 ` [PATCH 9/9] drm: mxsfb: Add support for i.MX8MP LCDIF variant Marek Vasut 2022-02-28 3:48 ` kernel test robot 2022-02-28 3:48 ` kernel test robot 2022-02-28 6:37 ` [PATCH 1/9] dt-bindings: mxsfb: Add compatible for i.MX8MP Liu Ying 2022-02-28 6:37 ` Liu Ying 2022-02-28 6:57 ` Marek Vasut 2022-02-28 6:57 ` Marek Vasut 2022-02-28 8:18 ` Liu Ying 2022-02-28 8:18 ` Liu Ying 2022-02-28 15:34 ` Marek Vasut 2022-02-28 15:34 ` Marek Vasut 2022-03-01 2:44 ` Liu Ying 2022-03-01 2:44 ` Liu Ying 2022-03-01 10:04 ` Lucas Stach 2022-03-01 10:19 ` Marek Vasut 2022-03-01 11:05 ` Lucas Stach 2022-03-01 13:03 ` Adam Ford [this message] 2022-03-01 13:03 ` Adam Ford 2022-03-01 13:18 ` Lucas Stach 2022-03-01 13:18 ` Lucas Stach 2022-03-01 13:37 ` [EXT] " Robby Cai 2022-03-01 13:37 ` Robby Cai 2022-03-02 2:49 ` Marek Vasut 2022-03-02 2:49 ` Marek Vasut 2022-03-02 13:14 ` Robby Cai 2022-03-02 13:14 ` Robby Cai 2022-03-03 3:16 ` Marek Vasut 2022-03-03 3:16 ` Marek Vasut 2022-03-02 2:54 ` Marek Vasut 2022-03-02 2:54 ` Marek Vasut 2022-03-02 9:23 ` Lucas Stach 2022-03-02 9:23 ` Lucas Stach 2022-03-02 9:41 ` Liu Ying 2022-03-02 9:41 ` Liu Ying 2022-03-02 11:57 ` Lucas Stach 2022-03-02 11:57 ` Lucas Stach 2022-03-03 2:54 ` Liu Ying 2022-03-03 2:54 ` Liu Ying 2022-03-03 4:14 ` Marek Vasut 2022-03-03 4:14 ` Marek Vasut 2022-03-03 8:19 ` Lucas Stach 2022-03-03 8:19 ` Lucas Stach 2022-03-03 9:14 ` Liu Ying 2022-03-03 9:14 ` Liu Ying 2022-03-03 3:14 ` Marek Vasut 2022-03-03 3:14 ` Marek Vasut 2022-03-03 8:21 ` Lucas Stach 2022-03-03 8:21 ` Lucas Stach 2022-03-04 4:50 ` Marek Vasut 2022-03-04 4:50 ` Marek Vasut 2022-02-28 6:40 ` Laurent Pinchart 2022-02-28 6:40 ` Laurent Pinchart
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=CAHCN7xJ6ypDxZouZV1b1F1EgQFwdTvmY6EEekj+_z-UWbQMD5Q@mail.gmail.com \ --to=aford173@gmail.com \ --cc=alexander.stein@ew.tq-group.com \ --cc=devicetree@vger.kernel.org \ --cc=dri-devel@lists.freedesktop.org \ --cc=l.stach@pengutronix.de \ --cc=laurent.pinchart@ideasonboard.com \ --cc=marex@denx.de \ --cc=peng.fan@nxp.com \ --cc=robby.cai@nxp.com \ --cc=robh+dt@kernel.org \ --cc=sam@ravnborg.org \ --cc=victor.liu@oss.nxp.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.