On 6/30/22 10:30, Liu Ying wrote:
Hi Marek,
Hi,
drivers/gpu/drm/Makefile | 2 +-
drivers/gpu/drm/mxsfb/Kconfig | 16 +
drivers/gpu/drm/mxsfb/Makefile | 2 +
drivers/gpu/drm/mxsfb/lcdif_drv.c | 341 ++++++++++++++++++++
drivers/gpu/drm/mxsfb/lcdif_drv.h | 44 +++
drivers/gpu/drm/mxsfb/lcdif_kms.c | 483 +++++++++++++++++++++++++++++
drivers/gpu/drm/mxsfb/lcdif_regs.h | 257 +++++++++++++++
The mxsfb-drm driver is in the mxsfb directory as a successor of the
legacy mxsfb fbdev driver. The fbdev driver is for i.MX23/28 lcdif.
That mxsfb-drm is also used on iMX8M{,M,N}
So, in order to avoid confusion, maybe don't put the new lcdifv3 driver
here. I would choose to create a new sub-directory in
drivers/gpu/drm/imx and put it there. The full path can be
drivers/gpu/drm/imx/lcdifv3, which matches the IP name(as called by
design team).
It also matches the NXP downstream vendor kernel paths.
If people don't like it because i.MX23 lcdif version
register reads major version3, drivers/gpu/drm/imx/imx8mp-lcdif
It cannot, because this IP is also used in iMXRT.
can be
an alternative, though longer directory name. I tend to use lcdifv3
since separate directory(imx vs mxsfb) hints totally different display
controllers.
We've had this discussion about naming/placement before.
Placing the lcdif driver into mxsfb would allow code deduplication
between the two drivers, that's why it is in mxsfb directory.
7 files changed, 1144 insertions(+), 1 deletion(-)
create mode 100644 drivers/gpu/drm/mxsfb/lcdif_drv.c
create mode 100644 drivers/gpu/drm/mxsfb/lcdif_drv.h
create mode 100644 drivers/gpu/drm/mxsfb/lcdif_kms.c
create mode 100644 drivers/gpu/drm/mxsfb/lcdif_regs.h
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 13ef240b3d2b2..75b5ac7c2663c 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -130,7 +130,7 @@ obj-y += bridge/
obj-$(CONFIG_DRM_FSL_DCU) += fsl-dcu/
obj-$(CONFIG_DRM_ETNAVIV) += etnaviv/
obj-y += hisilicon/
-obj-$(CONFIG_DRM_MXSFB) += mxsfb/
+obj-y += mxsfb/
obj-y += tiny/
obj-$(CONFIG_DRM_PL111) += pl111/
obj-$(CONFIG_DRM_TVE200) += tve200/
diff --git a/drivers/gpu/drm/mxsfb/Kconfig b/drivers/gpu/drm/mxsfb/Kconfig
index 987170e16ebd6..873551b4552f5 100644
--- a/drivers/gpu/drm/mxsfb/Kconfig
+++ b/drivers/gpu/drm/mxsfb/Kconfig
@@ -19,3 +19,19 @@ config DRM_MXSFB
i.MX28, i.MX6SX, i.MX7 and i.MX8M).
If M is selected the module will be called mxsfb.
+
+config DRM_IMX_LCDIF
Perhaps, choose a less generic name here in case yet another new IP
with similar name in future... Use DRM_IMX_LCDIFV3?
+ tristate "i.MX LCDIFv3 LCD controller"
+ depends on DRM && OF
+ depends on COMMON_CLK
+ select DRM_MXS
+ select DRM_KMS_HELPER
+ select DRM_GEM_CMA_HELPER
+ select DRM_PANEL
+ select DRM_PANEL_BRIDGE
+ help
+ Choose this option if you have an LCDIFv3 LCD controller.
+ Those devices are found in various i.MX SoC (i.MX8MP,
+ i.MXRT).
+
+ If M is selected the module will be called imx-lcdif.
Same here. Use imx-lcdifv3?
The mxsfb LCDIF also supports multiple versions of the LCDIF IP (v3, v4,
v6 at least). So calling a driver LCDIF v3 is about as confusing, since
you cannot tell whether it is the iMX23 LCDIF v3 or the iMXRT/iMX8MP
LCDIFv3 .
Similar comment applies to the entire driver - less generic name.
[...]
+struct lcdif_drm_private {
+ void __iomem *base; /* registers */
+ struct clk *clk;
+ struct clk *clk_axi;
+ struct clk *clk_disp_axi;
i.MX8mp RM mentions that clocks are pix_clk, apb_clk and b_clk.
Why not use them?
That's only because the DT bindings are aligned between old and new LCDIF.
[...]
+ switch (format) {
+ case DRM_FORMAT_RGB565:
+ writel(CTRLDESCL0_5_BPP_16_RGB565,
+ lcdif->base + LCDC_V8_CTRLDESCL0_5);
+ break;
+ case DRM_FORMAT_RGB888:
+ writel(CTRLDESCL0_5_BPP_24_RGB888,
+ lcdif->base + LCDC_V8_CTRLDESCL0_5);
+ break;
+ case DRM_FORMAT_XRGB1555:
DRM_FORMAT_ARGB1555?
Why would there be formats with Alpha channel here ?
Because of the (unsupported) overlay planes ?
+ writel(CTRLDESCL0_5_BPP_32_ARGB8888,
+ lcdif->base + LCDC_V8_CTRLDESCL0_5);
+ break;
+ default:
+ dev_err(drm->dev, "Unknown pixel format 0x%x\n", format);
+ break;
+ }
lcdif_set_formats() is called in lcdif_crtc_mode_set_nofb(), so no fb,
which means the framebuffer pixel format should be set in plane
callback.
Do you have iMXRT and are/or you able to test overlay planes with this
IP (the overlay planes which are currently not supported) ? Or is there
any other iMX which has overlay plane available ?
[...]
