All of lore.kernel.org
 help / color / mirror / Atom feed
From: Lucas Stach <l.stach@pengutronix.de>
To: Marek Vasut <marex@denx.de>, Liu Ying <victor.liu@oss.nxp.com>,
	dri-devel@lists.freedesktop.org
Cc: 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, 01 Mar 2022 12:05:27 +0100	[thread overview]
Message-ID: <8bf0b5a1c9ab9faee28077436cdfd49c0cd08792.camel@pengutronix.de> (raw)
In-Reply-To: <7aeed693-dfb7-950f-fdf0-3c90de285392@denx.de>

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.

Regards,
Lucas



  reply	other threads:[~2022-03-01 11:05 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 [this message]
2022-03-01 13:03                 ` Adam Ford
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=8bf0b5a1c9ab9faee28077436cdfd49c0cd08792.camel@pengutronix.de \
    --to=l.stach@pengutronix.de \
    --cc=alexander.stein@ew.tq-group.com \
    --cc=devicetree@vger.kernel.org \
    --cc=dri-devel@lists.freedesktop.org \
    --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: link
Be 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.