Re: [PATCH v5 2/2] drm/imx/lcdc: Implement DRM driver for imx25

All of lore.kernel.org
 help / color / mirror / Atom feed

* Re: [PATCH v5 2/2] drm/imx/lcdc: Implement DRM driver for imx25
@ 2023-02-12 13:23 kernel test robot
  0 siblings, 0 replies; 7+ messages in thread
From: kernel test robot @ 2023-02-12 13:23 UTC (permalink / raw)
  To: oe-kbuild; +Cc: lkp

:::::: 
:::::: Manual check reason: "low confidence bisect report"
:::::: 

BCC: lkp@intel.com
CC: oe-kbuild-all@lists.linux.dev
In-Reply-To: <20230210180014.173379-3-u.kleine-koenig@pengutronix.de>
References: <20230210180014.173379-3-u.kleine-koenig@pengutronix.de>
TO: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
TO: David Airlie <airlied@gmail.com>
TO: Daniel Vetter <daniel@ffwll.ch>
TO: Shawn Guo <shawnguo@kernel.org>
TO: Sascha Hauer <s.hauer@pengutronix.de>
TO: Philipp Zabel <p.zabel@pengutronix.de>
TO: Sam Ravnborg <sam@ravnborg.org>
TO: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
TO: Danilo Krummrich <dakr@redhat.com>
TO: Liu Ying <victor.liu@nxp.com>
CC: linux-arm-kernel@lists.infradead.org
CC: dri-devel@lists.freedesktop.org
CC: NXP Linux Team <linux-imx@nxp.com>
CC: Pengutronix Kernel Team <kernel@pengutronix.de>

Hi Uwe,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on 2591939e881cf728b6ac45971eeec2f58051c101]

url:    https://github.com/intel-lab-lkp/linux/commits/Uwe-Kleine-K-nig/dt-bindings-display-imx-Describe-drm-binding-for-fsl-imx-lcdc/20230211-020147
base:   2591939e881cf728b6ac45971eeec2f58051c101
patch link:    https://lore.kernel.org/r/20230210180014.173379-3-u.kleine-koenig%40pengutronix.de
patch subject: [PATCH v5 2/2] drm/imx/lcdc: Implement DRM driver for imx25
:::::: branch date: 2 days ago
:::::: commit date: 2 days ago
compiler: gcc-11 (Debian 11.3.0-8) 11.3.0

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/r/202302122146.3fEMhin0-lkp@intel.com/

includecheck warnings: (new ones prefixed by >>)
>> drivers/gpu/drm/imx/lcdc/imx-lcdc.c: drm/drm_fourcc.h is included more than once.

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH v5 0/2] drm/imx/lcdc: Implement DRM driver for imx25
@ 2023-02-10 18:00 Uwe Kleine-König
  2023-02-10 18:00   ` Uwe Kleine-König
  0 siblings, 1 reply; 7+ messages in thread
From: Uwe Kleine-König @ 2023-02-10 18:00 UTC (permalink / raw)
  To: Philipp Zabel, David Airlie, Daniel Vetter, Rob Herring,
	Krzysztof Kozlowski, Shawn Guo, Sascha Hauer, Sam Ravnborg,
	Laurent Pinchart, Danilo Krummrich, Liu Ying
  Cc: Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team, dri-devel,
	devicetree, linux-arm-kernel

Hello,

Changes since v4:

 - Explicitly handle the connector and pass
   DRM_BRIDGE_ATTACH_NO_CONNECTOR to drm_bridge_attach (Laurent)
 - Fix binding syntax hopfully makeing Rob's dt-checker bot happy
 - Resort #includes alphabetically

A big thanks to Philipp who (again!) was a great help to get v5 out.

Patch 1 depends on patch "dt-bindings: display: Convert fsl,imx-fb.txt
to dt-schema" which currently sits in Rob's tree as
93266da2409b1709474be00f1becbbdaddb2b706. Patch 2 bases on "drm/imx:
move IPUv3 driver into separate subdirectory" which currentlich sits in
drm-misc-next-2023-01-03 as 4b6cb2b67da883bc5095ee6d77f951f1cd7a1c24.

Unchanged since v3 is that the binding is using a different compatible. This is
a bit ugly, but a drm driver needs a considerably different binding anyhow and
this is the chance to pick a better name: The legacy binding uses "imx25-fb"
(and similar for other SoCs), but the hardware unit is called LCDC and so I
picked "imx25-lcdc" as new name. The idea is to deprecate imx25-fb (et al) and
convert the imx25.dtsi to imx25-lcdc. (So I don't plan to have both variants in
the dtsi file which Rob considered ugly.)

Marian Cichy (1):
  drm/imx/lcdc: Implement DRM driver for imx25

Uwe Kleine-König (1):
  dt-bindings: display: imx: Describe drm binding for fsl,imx-lcdc

 .../bindings/display/imx/fsl,imx-lcdc.yaml    |  46 +-
 drivers/gpu/drm/imx/Kconfig                   |   1 +
 drivers/gpu/drm/imx/Makefile                  |   1 +
 drivers/gpu/drm/imx/lcdc/Kconfig              |   7 +
 drivers/gpu/drm/imx/lcdc/Makefile             |   1 +
 drivers/gpu/drm/imx/lcdc/imx-lcdc.c           | 553 ++++++++++++++++++
 6 files changed, 608 insertions(+), 1 deletion(-)
 create mode 100644 drivers/gpu/drm/imx/lcdc/Kconfig
 create mode 100644 drivers/gpu/drm/imx/lcdc/Makefile
 create mode 100644 drivers/gpu/drm/imx/lcdc/imx-lcdc.c

base-commit: 2591939e881cf728b6ac45971eeec2f58051c101
prerequisite-patch-id: c3ef3de02516b5c159e76b40d2b4348a5ce0fe51
-- 
2.39.0

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH v5 2/2] drm/imx/lcdc: Implement DRM driver for imx25
  2023-02-10 18:00 [PATCH v5 0/2] " Uwe Kleine-König
@ 2023-02-10 18:00   ` Uwe Kleine-König
  0 siblings, 0 replies; 7+ messages in thread
From: Uwe Kleine-König @ 2023-02-10 18:00 UTC (permalink / raw)
  To: David Airlie, Daniel Vetter, Shawn Guo, Sascha Hauer,
	Philipp Zabel, Sam Ravnborg, Laurent Pinchart, Danilo Krummrich,
	Liu Ying
  Cc: Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team, dri-devel,
	linux-arm-kernel

From: Marian Cichy <m.cichy@pengutronix.de>

Add support for the LCD Controller found on i.MX21 and i.MX25.

It targets to be a drop in replacement for the imx-fb driver.

Signed-off-by: Marian Cichy <m.cichy@pengutronix.de>
[ukl: Rebase to a newer kernel version, various smaller fixes and
improvements]
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/gpu/drm/imx/Kconfig         |   1 +
 drivers/gpu/drm/imx/Makefile        |   1 +
 drivers/gpu/drm/imx/lcdc/Kconfig    |   7 +
 drivers/gpu/drm/imx/lcdc/Makefile   |   1 +
 drivers/gpu/drm/imx/lcdc/imx-lcdc.c | 553 ++++++++++++++++++++++++++++
 5 files changed, 563 insertions(+)
 create mode 100644 drivers/gpu/drm/imx/lcdc/Kconfig
 create mode 100644 drivers/gpu/drm/imx/lcdc/Makefile
 create mode 100644 drivers/gpu/drm/imx/lcdc/imx-lcdc.c

diff --git a/drivers/gpu/drm/imx/Kconfig b/drivers/gpu/drm/imx/Kconfig
index e5749927fd6c..03535a15dd8f 100644
--- a/drivers/gpu/drm/imx/Kconfig
+++ b/drivers/gpu/drm/imx/Kconfig
@@ -2,3 +2,4 @@
 
 source "drivers/gpu/drm/imx/dcss/Kconfig"
 source "drivers/gpu/drm/imx/ipuv3/Kconfig"
+source "drivers/gpu/drm/imx/lcdc/Kconfig"
diff --git a/drivers/gpu/drm/imx/Makefile b/drivers/gpu/drm/imx/Makefile
index 909622864716..86f38e7c7422 100644
--- a/drivers/gpu/drm/imx/Makefile
+++ b/drivers/gpu/drm/imx/Makefile
@@ -2,3 +2,4 @@
 
 obj-$(CONFIG_DRM_IMX_DCSS) += dcss/
 obj-$(CONFIG_DRM_IMX) += ipuv3/
+obj-$(CONFIG_DRM_IMX_LCDC) += lcdc/
diff --git a/drivers/gpu/drm/imx/lcdc/Kconfig b/drivers/gpu/drm/imx/lcdc/Kconfig
new file mode 100644
index 000000000000..7e57922bbd9d
--- /dev/null
+++ b/drivers/gpu/drm/imx/lcdc/Kconfig
@@ -0,0 +1,7 @@
+config DRM_IMX_LCDC
+      tristate "Freescale i.MX LCDC displays"
+      depends on DRM && (ARCH_MXC || COMPILE_TEST)
+      select DRM_GEM_DMA_HELPER
+      select DRM_KMS_HELPER
+      help
+        Found on i.MX1, i.MX21, i.MX25 and i.MX27.
diff --git a/drivers/gpu/drm/imx/lcdc/Makefile b/drivers/gpu/drm/imx/lcdc/Makefile
new file mode 100644
index 000000000000..e84daa432c2e
--- /dev/null
+++ b/drivers/gpu/drm/imx/lcdc/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_DRM_IMX_LCDC) += imx-lcdc.o
diff --git a/drivers/gpu/drm/imx/lcdc/imx-lcdc.c b/drivers/gpu/drm/imx/lcdc/imx-lcdc.c
new file mode 100644
index 000000000000..c2197fc50306
--- /dev/null
+++ b/drivers/gpu/drm/imx/lcdc/imx-lcdc.c
@@ -0,0 +1,553 @@
+// SPDX-License-Identifier: GPL-2.0-only
+// SPDX-FileCopyrightText: 2020 Marian Cichy <M.Cichy@pengutronix.de>
+
+#include <drm/drm_bridge_connector.h>
+#include <drm/drm_damage_helper.h>
+#include <drm/drm_drv.h>
+#include <drm/drm_fbdev_generic.h>
+#include <drm/drm_fb_dma_helper.h>
+#include <drm/drm_fourcc.h>
+#include <drm/drm_fourcc.h>
+#include <drm/drm_framebuffer.h>
+#include <drm/drm_gem_atomic_helper.h>
+#include <drm/drm_gem_dma_helper.h>
+#include <drm/drm_gem_framebuffer_helper.h>
+#include <drm/drm_of.h>
+#include <drm/drm_probe_helper.h>
+#include <drm/drm_simple_kms_helper.h>
+#include <drm/drm_vblank.h>
+#include <linux/bitfield.h>
+#include <linux/clk.h>
+#include <linux/dma-mapping.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+
+#define IMX21LCDC_LSSAR         0x0000 /* LCDC Screen Start Address Register */
+#define IMX21LCDC_LSR           0x0004 /* LCDC Size Register */
+#define IMX21LCDC_LVPWR         0x0008 /* LCDC Virtual Page Width Register */
+#define IMX21LCDC_LCPR          0x000C /* LCDC Cursor Position Register */
+#define IMX21LCDC_LCWHB         0x0010 /* LCDC Cursor Width Height and Blink Register*/
+#define IMX21LCDC_LCCMR         0x0014 /* LCDC Color Cursor Mapping Register */
+#define IMX21LCDC_LPCR          0x0018 /* LCDC Panel Configuration Register */
+#define IMX21LCDC_LHCR          0x001C /* LCDC Horizontal Configuration Register */
+#define IMX21LCDC_LVCR          0x0020 /* LCDC Vertical Configuration Register */
+#define IMX21LCDC_LPOR          0x0024 /* LCDC Panning Offset Register */
+#define IMX21LCDC_LSCR          0x0028 /* LCDC Sharp Configuration Register */
+#define IMX21LCDC_LPCCR         0x002C /* LCDC PWM Contrast Control Register */
+#define IMX21LCDC_LDCR          0x0030 /* LCDC DMA Control Register */
+#define IMX21LCDC_LRMCR         0x0034 /* LCDC Refresh Mode Control Register */
+#define IMX21LCDC_LICR          0x0038 /* LCDC Interrupt Configuration Register */
+#define IMX21LCDC_LIER          0x003C /* LCDC Interrupt Enable Register */
+#define IMX21LCDC_LISR          0x0040 /* LCDC Interrupt Status Register */
+#define IMX21LCDC_LGWSAR        0x0050 /* LCDC Graphic Window Start Address Register */
+#define IMX21LCDC_LGWSR         0x0054 /* LCDC Graph Window Size Register */
+#define IMX21LCDC_LGWVPWR       0x0058 /* LCDC Graphic Window Virtual Page Width Register */
+#define IMX21LCDC_LGWPOR        0x005C /* LCDC Graphic Window Panning Offset Register */
+#define IMX21LCDC_LGWPR         0x0060 /* LCDC Graphic Window Position Register */
+#define IMX21LCDC_LGWCR         0x0064 /* LCDC Graphic Window Control Register */
+#define IMX21LCDC_LGWDCR        0x0068 /* LCDC Graphic Window DMA Control Register */
+#define IMX21LCDC_LAUSCR        0x0080 /* LCDC AUS Mode Control Register */
+#define IMX21LCDC_LAUSCCR       0x0084 /* LCDC AUS Mode Cursor Control Register */
+#define IMX21LCDC_BGLUT         0x0800 /* Background Lookup Table */
+#define IMX21LCDC_GWLUT         0x0C00 /* Graphic Window Lookup Table */
+
+#define IMX21LCDC_LCPR_CC0 BIT(30) /* Cursor Control Bit 0 */
+#define IMX21LCDC_LCPR_CC1 BIT(31) /* Cursor Control Bit 1 */
+
+/* Values HSYNC, VSYNC and Framesize Register */
+#define IMX21LCDC_LHCR_HWIDTH		GENMASK(31, 26)
+#define IMX21LCDC_LHCR_HFPORCH		GENMASK(15, 8)		/* H_WAIT_1 in the i.MX25 Reference manual */
+#define IMX21LCDC_LHCR_HBPORCH		GENMASK(7, 0)		/* H_WAIT_2 in the i.MX25 Reference manual */
+
+#define IMX21LCDC_LVCR_VWIDTH		GENMASK(31, 26)
+#define IMX21LCDC_LVCR_VFPORCH		GENMASK(15, 8)		/* V_WAIT_1 in the i.MX25 Reference manual */
+#define IMX21LCDC_LVCR_VBPORCH		GENMASK(7, 0)		/* V_WAIT_2 in the i.MX25 Reference manual */
+
+#define IMX21LCDC_LSR_XMAX		GENMASK(25, 20)
+#define IMX21LCDC_LSR_YMAX		GENMASK(9, 0)
+
+/* Values for LPCR Register */
+#define IMX21LCDC_LPCR_PCD		GENMASK(5, 0)
+#define IMX21LCDC_LPCR_SHARP		BIT(6)
+#define IMX21LCDC_LPCR_SCLKSEL		BIT(7)
+#define IMX21LCDC_LPCR_ACD		GENMASK(14, 8)
+#define IMX21LCDC_LPCR_ACDSEL		BIT(15)
+#define IMX21LCDC_LPCR_REV_VS		BIT(16)
+#define IMX21LCDC_LPCR_SWAP_SEL		BIT(17)
+#define IMX21LCDC_LPCR_END_SEL		BIT(18)
+#define IMX21LCDC_LPCR_SCLKIDLE		BIT(19)
+#define IMX21LCDC_LPCR_OEPOL		BIT(20)
+#define IMX21LCDC_LPCR_CLKPOL		BIT(21)
+#define IMX21LCDC_LPCR_LPPOL		BIT(22)
+#define IMX21LCDC_LPCR_FLMPOL		BIT(23)
+#define IMX21LCDC_LPCR_PIXPOL		BIT(24)
+#define IMX21LCDC_LPCR_BPIX		GENMASK(27, 25)
+#define IMX21LCDC_LPCR_PBSIZ		GENMASK(29, 28)
+#define IMX21LCDC_LPCR_COLOR		BIT(30)
+#define IMX21LCDC_LPCR_TFT		BIT(31)
+
+#define INTR_EOF BIT(1) /* VBLANK Interrupt Bit */
+
+#define BPP_RGB565	0x05
+#define BPP_XRGB8888	0x07
+
+#define LCDC_MIN_XRES 64
+#define LCDC_MIN_YRES 64
+
+#define LCDC_MAX_XRES 1024
+#define LCDC_MAX_YRES 1024
+
+struct imx_lcdc {
+	struct drm_device drm;
+	struct drm_simple_display_pipe pipe;
+	const struct drm_display_mode *mode;
+	struct drm_bridge *bridge;
+	struct drm_connector *connector;
+	void __iomem *base;
+
+	struct clk *clk_ipg;
+	struct clk *clk_ahb;
+	struct clk *clk_per;
+};
+
+static const u32 imx_lcdc_formats[] = {
+	DRM_FORMAT_RGB565, DRM_FORMAT_XRGB8888,
+};
+
+static inline struct imx_lcdc *imx_lcdc_from_drmdev(struct drm_device *drm)
+{
+	return container_of(drm, struct imx_lcdc, drm);
+}
+
+static unsigned int imx_lcdc_get_format(unsigned int drm_format)
+{
+	unsigned int bpp;
+
+	switch (drm_format) {
+	default:
+		DRM_WARN("Format not supported - fallback to XRGB8888\n");
+		fallthrough;
+
+	case DRM_FORMAT_XRGB8888:
+		bpp = BPP_XRGB8888;
+		break;
+
+	case DRM_FORMAT_RGB565:
+		bpp = BPP_RGB565;
+		break;
+	}
+
+	return bpp;
+}
+
+static void imx_lcdc_update_hw_registers(struct drm_simple_display_pipe *pipe,
+					 struct drm_plane_state *old_state,
+					 bool mode_set)
+{
+	struct drm_crtc *crtc = &pipe->crtc;
+	struct drm_plane_state *new_state = pipe->plane.state;
+	struct drm_framebuffer *fb = new_state->fb;
+	struct imx_lcdc *lcdc = imx_lcdc_from_drmdev(pipe->crtc.dev);
+	u32 lpcr, lvcr, lhcr;
+	u32 framesize;
+	dma_addr_t addr;
+
+	addr = drm_fb_dma_get_gem_addr(fb, new_state, 0);
+	/* The LSSAR register specifies the LCD screen start address (SSA). */
+	writel(addr, lcdc->base + IMX21LCDC_LSSAR);
+
+	if (!mode_set)
+		return;
+
+	/* Disable PER clock to make register write possible */
+	if (old_state && old_state->crtc && old_state->crtc->enabled)
+		clk_disable_unprepare(lcdc->clk_per);
+
+	/* Framesize */
+	framesize = FIELD_PREP(IMX21LCDC_LSR_XMAX, crtc->mode.hdisplay >> 4) |
+		FIELD_PREP(IMX21LCDC_LSR_YMAX, crtc->mode.vdisplay);
+	writel(framesize, lcdc->base + IMX21LCDC_LSR);
+
+	/* HSYNC */
+	lhcr = FIELD_PREP(IMX21LCDC_LHCR_HFPORCH, crtc->mode.hsync_start - crtc->mode.hdisplay - 1) |
+		FIELD_PREP(IMX21LCDC_LHCR_HWIDTH, crtc->mode.hsync_end - crtc->mode.hsync_start - 1) |
+		FIELD_PREP(IMX21LCDC_LHCR_HBPORCH, crtc->mode.htotal - crtc->mode.hsync_end - 3);
+	writel(lhcr, lcdc->base + IMX21LCDC_LHCR);
+
+	/* VSYNC */
+	lvcr = FIELD_PREP(IMX21LCDC_LVCR_VFPORCH, crtc->mode.vsync_start - crtc->mode.vdisplay) |
+		FIELD_PREP(IMX21LCDC_LVCR_VWIDTH, crtc->mode.vsync_end - crtc->mode.vsync_start) |
+		FIELD_PREP(IMX21LCDC_LVCR_VBPORCH, crtc->mode.vtotal - crtc->mode.vsync_end);
+	writel(lvcr, lcdc->base + IMX21LCDC_LVCR);
+
+	lpcr = readl(lcdc->base + IMX21LCDC_LPCR);
+	lpcr &= ~IMX21LCDC_LPCR_BPIX;
+	lpcr |= FIELD_PREP(IMX21LCDC_LPCR_BPIX, imx_lcdc_get_format(fb->format->format));
+	writel(lpcr, lcdc->base + IMX21LCDC_LPCR);
+
+	/* Virtual Page Width */
+	writel(new_state->fb->pitches[0] / 4, lcdc->base + IMX21LCDC_LVPWR);
+
+	/* Enable PER clock */
+	if (new_state->crtc->enabled)
+		clk_prepare_enable(lcdc->clk_per);
+}
+
+static void imx_lcdc_pipe_enable(struct drm_simple_display_pipe *pipe,
+				 struct drm_crtc_state *crtc_state,
+				 struct drm_plane_state *plane_state)
+{
+	int ret;
+	int clk_div;
+	int bpp;
+	struct imx_lcdc *lcdc = imx_lcdc_from_drmdev(pipe->crtc.dev);
+	struct drm_display_mode *mode = &pipe->crtc.mode;
+	struct drm_display_info *disp_info = &lcdc->connector->display_info;
+	const int hsync_pol = (mode->flags & DRM_MODE_FLAG_PHSYNC) ? 0 : 1;
+	const int vsync_pol = (mode->flags & DRM_MODE_FLAG_PVSYNC) ? 0 : 1;
+	const int data_enable_pol =
+		(disp_info->bus_flags & DRM_BUS_FLAG_DE_HIGH) ? 0 : 1;
+	const int clk_pol =
+		(disp_info->bus_flags & DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE) ? 0 : 1;
+
+	clk_div = DIV_ROUND_CLOSEST_ULL(clk_get_rate(lcdc->clk_per),
+					mode->clock * 1000);
+	bpp = imx_lcdc_get_format(plane_state->fb->format->format);
+
+	writel(FIELD_PREP(IMX21LCDC_LPCR_PCD, clk_div - 1) |
+	       FIELD_PREP(IMX21LCDC_LPCR_LPPOL, hsync_pol) |
+	       FIELD_PREP(IMX21LCDC_LPCR_FLMPOL, vsync_pol) |
+	       FIELD_PREP(IMX21LCDC_LPCR_OEPOL, data_enable_pol) |
+	       FIELD_PREP(IMX21LCDC_LPCR_TFT, 1) |
+	       FIELD_PREP(IMX21LCDC_LPCR_COLOR, 1) |
+	       FIELD_PREP(IMX21LCDC_LPCR_PBSIZ, 3) |
+	       FIELD_PREP(IMX21LCDC_LPCR_BPIX, bpp) |
+	       FIELD_PREP(IMX21LCDC_LPCR_SCLKSEL, 1) |
+	       FIELD_PREP(IMX21LCDC_LPCR_PIXPOL, 0) |
+	       FIELD_PREP(IMX21LCDC_LPCR_CLKPOL, clk_pol),
+	       lcdc->base + IMX21LCDC_LPCR);
+
+	/* 0px panning offset */
+	writel(0x00000000, lcdc->base + IMX21LCDC_LPOR);
+
+	/* disable hardware cursor */
+	writel(readl(lcdc->base + IMX21LCDC_LCPR) & ~(IMX21LCDC_LCPR_CC0 | IMX21LCDC_LCPR_CC1),
+	       lcdc->base + IMX21LCDC_LCPR);
+
+	ret = clk_prepare_enable(lcdc->clk_ipg);
+	if (ret) {
+		dev_err(pipe->crtc.dev->dev, "Cannot enable ipg clock: %pe\n", ERR_PTR(ret));
+		return;
+	}
+	ret = clk_prepare_enable(lcdc->clk_ahb);
+	if (ret) {
+		dev_err(pipe->crtc.dev->dev, "Cannot enable ahb clock: %pe\n", ERR_PTR(ret));
+
+		clk_disable_unprepare(lcdc->clk_ipg);
+
+		return;
+	}
+
+	imx_lcdc_update_hw_registers(pipe, NULL, true);
+
+	/* Enable VBLANK Interrupt */
+	writel(INTR_EOF, lcdc->base + IMX21LCDC_LIER);
+}
+
+static void imx_lcdc_pipe_disable(struct drm_simple_display_pipe *pipe)
+{
+	struct imx_lcdc *lcdc = imx_lcdc_from_drmdev(pipe->crtc.dev);
+	struct drm_crtc *crtc = &lcdc->pipe.crtc;
+	struct drm_pending_vblank_event *event;
+
+	clk_disable_unprepare(lcdc->clk_ahb);
+	clk_disable_unprepare(lcdc->clk_ipg);
+
+	if (pipe->crtc.enabled)
+		clk_disable_unprepare(lcdc->clk_per);
+
+	spin_lock_irq(&lcdc->drm.event_lock);
+	event = crtc->state->event;
+	if (event) {
+		crtc->state->event = NULL;
+		drm_crtc_send_vblank_event(crtc, event);
+	}
+	spin_unlock_irq(&lcdc->drm.event_lock);
+
+	/* Disable VBLANK Interrupt */
+	writel(0, lcdc->base + IMX21LCDC_LIER);
+}
+
+static int imx_lcdc_pipe_check(struct drm_simple_display_pipe *pipe,
+			       struct drm_plane_state *plane_state,
+			       struct drm_crtc_state *crtc_state)
+{
+	const struct drm_display_mode *mode = &crtc_state->mode;
+	const struct drm_display_mode *old_mode = &pipe->crtc.state->mode;
+
+	if (mode->hdisplay < LCDC_MIN_XRES || mode->hdisplay > LCDC_MAX_XRES ||
+	    mode->vdisplay < LCDC_MIN_YRES || mode->vdisplay > LCDC_MAX_YRES ||
+	    mode->hdisplay % 0x10) { /* must be multiple of 16 */
+		drm_err(pipe->crtc.dev, "unsupported display mode (%u x %u)\n",
+			mode->hdisplay, mode->vdisplay);
+		return -EINVAL;
+	}
+
+	crtc_state->mode_changed =
+		old_mode->hdisplay != mode->hdisplay ||
+		old_mode->vdisplay != mode->vdisplay;
+
+	return 0;
+}
+
+static void imx_lcdc_pipe_update(struct drm_simple_display_pipe *pipe,
+				 struct drm_plane_state *old_state)
+{
+	struct drm_crtc *crtc = &pipe->crtc;
+	struct drm_pending_vblank_event *event = crtc->state->event;
+	struct drm_plane_state *new_state = pipe->plane.state;
+	struct drm_framebuffer *fb = new_state->fb;
+	struct drm_framebuffer *old_fb = old_state->fb;
+	struct drm_crtc *old_crtc = old_state->crtc;
+	bool mode_changed = false;
+
+	if (old_fb && old_fb->format != fb->format)
+		mode_changed = true;
+	else if (old_crtc != crtc)
+		mode_changed = true;
+
+	imx_lcdc_update_hw_registers(pipe, old_state, mode_changed);
+
+	if (event) {
+		crtc->state->event = NULL;
+
+		spin_lock_irq(&crtc->dev->event_lock);
+
+		if (crtc->state->active && drm_crtc_vblank_get(crtc) == 0)
+			drm_crtc_arm_vblank_event(crtc, event);
+		else
+			drm_crtc_send_vblank_event(crtc, event);
+
+		spin_unlock_irq(&crtc->dev->event_lock);
+	}
+}
+
+static const struct drm_simple_display_pipe_funcs imx_lcdc_pipe_funcs = {
+	.enable = imx_lcdc_pipe_enable,
+	.disable = imx_lcdc_pipe_disable,
+	.check = imx_lcdc_pipe_check,
+	.update = imx_lcdc_pipe_update,
+};
+
+static const struct drm_mode_config_funcs imx_lcdc_mode_config_funcs = {
+	.fb_create = drm_gem_fb_create_with_dirty,
+	.atomic_check = drm_atomic_helper_check,
+	.atomic_commit = drm_atomic_helper_commit,
+};
+
+static const struct drm_mode_config_helper_funcs imx_lcdc_mode_config_helpers = {
+	.atomic_commit_tail = drm_atomic_helper_commit_tail_rpm,
+};
+
+static void imx_lcdc_release(struct drm_device *drm)
+{
+	struct imx_lcdc *lcdc = imx_lcdc_from_drmdev(drm);
+
+	drm_kms_helper_poll_fini(drm);
+	kfree(lcdc);
+}
+
+DEFINE_DRM_GEM_DMA_FOPS(imx_lcdc_drm_fops);
+
+static struct drm_driver imx_lcdc_drm_driver = {
+	.driver_features = DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC,
+	.fops = &imx_lcdc_drm_fops,
+	DRM_GEM_DMA_DRIVER_OPS_VMAP,
+	.release = imx_lcdc_release,
+	.name = "imx-lcdc",
+	.desc = "i.MX LCDC driver",
+	.date = "20200716",
+};
+
+static const struct of_device_id imx_lcdc_of_dev_id[] = {
+	{
+		.compatible = "fsl,imx21-lcdc",
+	},
+	{
+		.compatible = "fsl,imx25-lcdc",
+	},
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, imx_lcdc_of_dev_id);
+
+static irqreturn_t imx_lcdc_irq_handler(int irq, void *arg)
+{
+	struct imx_lcdc *lcdc = arg;
+	struct drm_crtc *crtc = &lcdc->pipe.crtc;
+	unsigned int status;
+
+	status = readl(lcdc->base + IMX21LCDC_LISR);
+
+	if (status & INTR_EOF) {
+		drm_crtc_handle_vblank(crtc);
+		return IRQ_HANDLED;
+	}
+
+	return IRQ_NONE;
+}
+
+static int imx_lcdc_probe(struct platform_device *pdev)
+{
+	struct imx_lcdc *lcdc;
+	struct drm_device *drm;
+	int irq;
+	int ret;
+	struct device *dev = &pdev->dev;
+
+	lcdc = devm_drm_dev_alloc(dev, &imx_lcdc_drm_driver,
+				  struct imx_lcdc, drm);
+	if (!lcdc)
+		return -ENOMEM;
+
+	drm = &lcdc->drm;
+
+	lcdc->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(lcdc->base))
+		return dev_err_probe(dev, PTR_ERR(lcdc->base), "Cannot get IO memory\n");
+
+	lcdc->bridge = devm_drm_of_get_bridge(dev, dev->of_node, 0, 0);
+	if (IS_ERR(lcdc->bridge))
+		return dev_err_probe(dev, PTR_ERR(lcdc->bridge), "Failed to find bridge\n");
+
+	/* Get Clocks */
+	lcdc->clk_ipg = devm_clk_get(dev, "ipg");
+	if (IS_ERR(lcdc->clk_ipg))
+		return dev_err_probe(dev, PTR_ERR(lcdc->clk_ipg), "Failed to get %s clk\n", "ipg");
+
+	lcdc->clk_ahb = devm_clk_get(dev, "ahb");
+	if (IS_ERR(lcdc->clk_ahb))
+		return dev_err_probe(dev, PTR_ERR(lcdc->clk_ahb), "Failed to get %s clk\n", "ahb");
+
+	lcdc->clk_per = devm_clk_get(dev, "per");
+	if (IS_ERR(lcdc->clk_per))
+		return dev_err_probe(dev, PTR_ERR(lcdc->clk_per), "Failed to get %s clk\n", "per");
+
+	ret = dma_set_mask_and_coherent(drm->dev, DMA_BIT_MASK(32));
+	if (ret)
+		return dev_err_probe(dev, ret, "Cannot set DMA Mask\n");
+
+	/* Modeset init */
+	ret = drmm_mode_config_init(drm);
+	if (ret)
+		return dev_err_probe(dev, ret, "Cannot initialize mode configuration structure\n");
+
+	/* CRTC, Plane, Encoder */
+	ret = drm_simple_display_pipe_init(drm, &lcdc->pipe,
+					   &imx_lcdc_pipe_funcs,
+					   imx_lcdc_formats,
+					   ARRAY_SIZE(imx_lcdc_formats), NULL, NULL);
+	if (ret < 0)
+		return dev_err_probe(drm->dev, ret, "Cannot setup simple display pipe\n");
+
+	ret = drm_vblank_init(drm, drm->mode_config.num_crtc);
+	if (ret < 0)
+		return dev_err_probe(drm->dev, ret, "Failed to initialize vblank\n");
+
+	ret = drm_bridge_attach(&lcdc->pipe.encoder, lcdc->bridge, NULL, DRM_BRIDGE_ATTACH_NO_CONNECTOR);
+	if (ret)
+		return dev_err_probe(drm->dev, ret, "Cannot attach bridge\n");
+
+	lcdc->connector = drm_bridge_connector_init(drm, &lcdc->pipe.encoder);
+	if (IS_ERR(lcdc->connector))
+		return dev_err_probe(drm->dev, PTR_ERR(lcdc->connector), "Cannot init bridge connector\n");
+
+	drm_connector_attach_encoder(lcdc->connector, &lcdc->pipe.encoder);
+
+	/*
+	 * The LCDC controller does not have an enable bit. The
+	 * controller starts directly when the clocks are enabled.
+	 * If the clocks are enabled when the controller is not yet
+	 * programmed with proper register values (enabled at the
+	 * bootloader, for example) then it just goes into some undefined
+	 * state.
+	 * To avoid this issue, let's enable and disable LCDC IPG,
+	 * PER and AHB clock so that we force some kind of 'reset'
+	 * to the LCDC block.
+	 */
+
+	ret = clk_prepare_enable(lcdc->clk_ipg);
+	if (ret)
+		return dev_err_probe(dev, ret, "Cannot enable ipg clock\n");
+	clk_disable_unprepare(lcdc->clk_ipg);
+
+	ret = clk_prepare_enable(lcdc->clk_per);
+	if (ret)
+		return dev_err_probe(dev, ret, "Cannot enable per clock\n");
+	clk_disable_unprepare(lcdc->clk_per);
+
+	ret = clk_prepare_enable(lcdc->clk_ahb);
+	if (ret)
+		return dev_err_probe(dev, ret, "Cannot enable ahb clock\n");
+	clk_disable_unprepare(lcdc->clk_ahb);
+
+	drm->mode_config.min_width = LCDC_MIN_XRES;
+	drm->mode_config.max_width = LCDC_MAX_XRES;
+	drm->mode_config.min_height = LCDC_MIN_YRES;
+	drm->mode_config.max_height = LCDC_MAX_YRES;
+	drm->mode_config.preferred_depth = 16;
+	drm->mode_config.funcs = &imx_lcdc_mode_config_funcs;
+	drm->mode_config.helper_private = &imx_lcdc_mode_config_helpers;
+
+	drm_mode_config_reset(drm);
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0) {
+		ret = irq;
+		return ret;
+	}
+
+	ret = devm_request_irq(dev, irq, imx_lcdc_irq_handler, 0, "imx-lcdc", lcdc);
+	if (ret < 0)
+		return dev_err_probe(drm->dev, ret, "Failed to install IRQ handler\n");
+
+	platform_set_drvdata(pdev, drm);
+
+	ret = drm_dev_register(&lcdc->drm, 0);
+	if (ret)
+		return dev_err_probe(dev, ret, "Cannot register device\n");
+
+	drm_fbdev_generic_setup(drm, 0);
+
+	return 0;
+}
+
+static int imx_lcdc_remove(struct platform_device *pdev)
+{
+	struct drm_device *drm = platform_get_drvdata(pdev);
+
+	drm_dev_unregister(drm);
+	drm_atomic_helper_shutdown(drm);
+
+	return 0;
+}
+
+static void imx_lcdc_shutdown(struct platform_device *pdev)
+{
+	drm_atomic_helper_shutdown(platform_get_drvdata(pdev));
+}
+
+static struct platform_driver imx_lcdc_driver = {
+	.driver = {
+		.name = "imx-lcdc",
+		.of_match_table = imx_lcdc_of_dev_id,
+	},
+	.probe = imx_lcdc_probe,
+	.remove = imx_lcdc_remove,
+	.shutdown = imx_lcdc_shutdown,
+};
+module_platform_driver(imx_lcdc_driver);
+
+MODULE_AUTHOR("Marian Cichy <M.Cichy@pengutronix.de>");
+MODULE_DESCRIPTION("Freescale i.MX LCDC driver");
+MODULE_LICENSE("GPL");
-- 
2.39.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH v5 2/2] drm/imx/lcdc: Implement DRM driver for imx25
@ 2023-02-10 18:00   ` Uwe Kleine-König
  0 siblings, 0 replies; 7+ messages in thread
From: Uwe Kleine-König @ 2023-02-10 18:00 UTC (permalink / raw)
  To: David Airlie, Daniel Vetter, Shawn Guo, Sascha Hauer,
	Philipp Zabel, Sam Ravnborg, Laurent Pinchart, Danilo Krummrich,
	Liu Ying
  Cc: linux-arm-kernel, dri-devel, NXP Linux Team,
	Pengutronix Kernel Team

From: Marian Cichy <m.cichy@pengutronix.de>

Add support for the LCD Controller found on i.MX21 and i.MX25.

It targets to be a drop in replacement for the imx-fb driver.

Signed-off-by: Marian Cichy <m.cichy@pengutronix.de>
[ukl: Rebase to a newer kernel version, various smaller fixes and
improvements]
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/gpu/drm/imx/Kconfig         |   1 +
 drivers/gpu/drm/imx/Makefile        |   1 +
 drivers/gpu/drm/imx/lcdc/Kconfig    |   7 +
 drivers/gpu/drm/imx/lcdc/Makefile   |   1 +
 drivers/gpu/drm/imx/lcdc/imx-lcdc.c | 553 ++++++++++++++++++++++++++++
 5 files changed, 563 insertions(+)
 create mode 100644 drivers/gpu/drm/imx/lcdc/Kconfig
 create mode 100644 drivers/gpu/drm/imx/lcdc/Makefile
 create mode 100644 drivers/gpu/drm/imx/lcdc/imx-lcdc.c

diff --git a/drivers/gpu/drm/imx/Kconfig b/drivers/gpu/drm/imx/Kconfig
index e5749927fd6c..03535a15dd8f 100644
--- a/drivers/gpu/drm/imx/Kconfig
+++ b/drivers/gpu/drm/imx/Kconfig
@@ -2,3 +2,4 @@
 
 source "drivers/gpu/drm/imx/dcss/Kconfig"
 source "drivers/gpu/drm/imx/ipuv3/Kconfig"
+source "drivers/gpu/drm/imx/lcdc/Kconfig"
diff --git a/drivers/gpu/drm/imx/Makefile b/drivers/gpu/drm/imx/Makefile
index 909622864716..86f38e7c7422 100644
--- a/drivers/gpu/drm/imx/Makefile
+++ b/drivers/gpu/drm/imx/Makefile
@@ -2,3 +2,4 @@
 
 obj-$(CONFIG_DRM_IMX_DCSS) += dcss/
 obj-$(CONFIG_DRM_IMX) += ipuv3/
+obj-$(CONFIG_DRM_IMX_LCDC) += lcdc/
diff --git a/drivers/gpu/drm/imx/lcdc/Kconfig b/drivers/gpu/drm/imx/lcdc/Kconfig
new file mode 100644
index 000000000000..7e57922bbd9d
--- /dev/null
+++ b/drivers/gpu/drm/imx/lcdc/Kconfig
@@ -0,0 +1,7 @@
+config DRM_IMX_LCDC
+      tristate "Freescale i.MX LCDC displays"
+      depends on DRM && (ARCH_MXC || COMPILE_TEST)
+      select DRM_GEM_DMA_HELPER
+      select DRM_KMS_HELPER
+      help
+        Found on i.MX1, i.MX21, i.MX25 and i.MX27.
diff --git a/drivers/gpu/drm/imx/lcdc/Makefile b/drivers/gpu/drm/imx/lcdc/Makefile
new file mode 100644
index 000000000000..e84daa432c2e
--- /dev/null
+++ b/drivers/gpu/drm/imx/lcdc/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_DRM_IMX_LCDC) += imx-lcdc.o
diff --git a/drivers/gpu/drm/imx/lcdc/imx-lcdc.c b/drivers/gpu/drm/imx/lcdc/imx-lcdc.c
new file mode 100644
index 000000000000..c2197fc50306
--- /dev/null
+++ b/drivers/gpu/drm/imx/lcdc/imx-lcdc.c
@@ -0,0 +1,553 @@
+// SPDX-License-Identifier: GPL-2.0-only
+// SPDX-FileCopyrightText: 2020 Marian Cichy <M.Cichy@pengutronix.de>
+
+#include <drm/drm_bridge_connector.h>
+#include <drm/drm_damage_helper.h>
+#include <drm/drm_drv.h>
+#include <drm/drm_fbdev_generic.h>
+#include <drm/drm_fb_dma_helper.h>
+#include <drm/drm_fourcc.h>
+#include <drm/drm_fourcc.h>
+#include <drm/drm_framebuffer.h>
+#include <drm/drm_gem_atomic_helper.h>
+#include <drm/drm_gem_dma_helper.h>
+#include <drm/drm_gem_framebuffer_helper.h>
+#include <drm/drm_of.h>
+#include <drm/drm_probe_helper.h>
+#include <drm/drm_simple_kms_helper.h>
+#include <drm/drm_vblank.h>
+#include <linux/bitfield.h>
+#include <linux/clk.h>
+#include <linux/dma-mapping.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+
+#define IMX21LCDC_LSSAR         0x0000 /* LCDC Screen Start Address Register */
+#define IMX21LCDC_LSR           0x0004 /* LCDC Size Register */
+#define IMX21LCDC_LVPWR         0x0008 /* LCDC Virtual Page Width Register */
+#define IMX21LCDC_LCPR          0x000C /* LCDC Cursor Position Register */
+#define IMX21LCDC_LCWHB         0x0010 /* LCDC Cursor Width Height and Blink Register*/
+#define IMX21LCDC_LCCMR         0x0014 /* LCDC Color Cursor Mapping Register */
+#define IMX21LCDC_LPCR          0x0018 /* LCDC Panel Configuration Register */
+#define IMX21LCDC_LHCR          0x001C /* LCDC Horizontal Configuration Register */
+#define IMX21LCDC_LVCR          0x0020 /* LCDC Vertical Configuration Register */
+#define IMX21LCDC_LPOR          0x0024 /* LCDC Panning Offset Register */
+#define IMX21LCDC_LSCR          0x0028 /* LCDC Sharp Configuration Register */
+#define IMX21LCDC_LPCCR         0x002C /* LCDC PWM Contrast Control Register */
+#define IMX21LCDC_LDCR          0x0030 /* LCDC DMA Control Register */
+#define IMX21LCDC_LRMCR         0x0034 /* LCDC Refresh Mode Control Register */
+#define IMX21LCDC_LICR          0x0038 /* LCDC Interrupt Configuration Register */
+#define IMX21LCDC_LIER          0x003C /* LCDC Interrupt Enable Register */
+#define IMX21LCDC_LISR          0x0040 /* LCDC Interrupt Status Register */
+#define IMX21LCDC_LGWSAR        0x0050 /* LCDC Graphic Window Start Address Register */
+#define IMX21LCDC_LGWSR         0x0054 /* LCDC Graph Window Size Register */
+#define IMX21LCDC_LGWVPWR       0x0058 /* LCDC Graphic Window Virtual Page Width Register */
+#define IMX21LCDC_LGWPOR        0x005C /* LCDC Graphic Window Panning Offset Register */
+#define IMX21LCDC_LGWPR         0x0060 /* LCDC Graphic Window Position Register */
+#define IMX21LCDC_LGWCR         0x0064 /* LCDC Graphic Window Control Register */
+#define IMX21LCDC_LGWDCR        0x0068 /* LCDC Graphic Window DMA Control Register */
+#define IMX21LCDC_LAUSCR        0x0080 /* LCDC AUS Mode Control Register */
+#define IMX21LCDC_LAUSCCR       0x0084 /* LCDC AUS Mode Cursor Control Register */
+#define IMX21LCDC_BGLUT         0x0800 /* Background Lookup Table */
+#define IMX21LCDC_GWLUT         0x0C00 /* Graphic Window Lookup Table */
+
+#define IMX21LCDC_LCPR_CC0 BIT(30) /* Cursor Control Bit 0 */
+#define IMX21LCDC_LCPR_CC1 BIT(31) /* Cursor Control Bit 1 */
+
+/* Values HSYNC, VSYNC and Framesize Register */
+#define IMX21LCDC_LHCR_HWIDTH		GENMASK(31, 26)
+#define IMX21LCDC_LHCR_HFPORCH		GENMASK(15, 8)		/* H_WAIT_1 in the i.MX25 Reference manual */
+#define IMX21LCDC_LHCR_HBPORCH		GENMASK(7, 0)		/* H_WAIT_2 in the i.MX25 Reference manual */
+
+#define IMX21LCDC_LVCR_VWIDTH		GENMASK(31, 26)
+#define IMX21LCDC_LVCR_VFPORCH		GENMASK(15, 8)		/* V_WAIT_1 in the i.MX25 Reference manual */
+#define IMX21LCDC_LVCR_VBPORCH		GENMASK(7, 0)		/* V_WAIT_2 in the i.MX25 Reference manual */
+
+#define IMX21LCDC_LSR_XMAX		GENMASK(25, 20)
+#define IMX21LCDC_LSR_YMAX		GENMASK(9, 0)
+
+/* Values for LPCR Register */
+#define IMX21LCDC_LPCR_PCD		GENMASK(5, 0)
+#define IMX21LCDC_LPCR_SHARP		BIT(6)
+#define IMX21LCDC_LPCR_SCLKSEL		BIT(7)
+#define IMX21LCDC_LPCR_ACD		GENMASK(14, 8)
+#define IMX21LCDC_LPCR_ACDSEL		BIT(15)
+#define IMX21LCDC_LPCR_REV_VS		BIT(16)
+#define IMX21LCDC_LPCR_SWAP_SEL		BIT(17)
+#define IMX21LCDC_LPCR_END_SEL		BIT(18)
+#define IMX21LCDC_LPCR_SCLKIDLE		BIT(19)
+#define IMX21LCDC_LPCR_OEPOL		BIT(20)
+#define IMX21LCDC_LPCR_CLKPOL		BIT(21)
+#define IMX21LCDC_LPCR_LPPOL		BIT(22)
+#define IMX21LCDC_LPCR_FLMPOL		BIT(23)
+#define IMX21LCDC_LPCR_PIXPOL		BIT(24)
+#define IMX21LCDC_LPCR_BPIX		GENMASK(27, 25)
+#define IMX21LCDC_LPCR_PBSIZ		GENMASK(29, 28)
+#define IMX21LCDC_LPCR_COLOR		BIT(30)
+#define IMX21LCDC_LPCR_TFT		BIT(31)
+
+#define INTR_EOF BIT(1) /* VBLANK Interrupt Bit */
+
+#define BPP_RGB565	0x05
+#define BPP_XRGB8888	0x07
+
+#define LCDC_MIN_XRES 64
+#define LCDC_MIN_YRES 64
+
+#define LCDC_MAX_XRES 1024
+#define LCDC_MAX_YRES 1024
+
+struct imx_lcdc {
+	struct drm_device drm;
+	struct drm_simple_display_pipe pipe;
+	const struct drm_display_mode *mode;
+	struct drm_bridge *bridge;
+	struct drm_connector *connector;
+	void __iomem *base;
+
+	struct clk *clk_ipg;
+	struct clk *clk_ahb;
+	struct clk *clk_per;
+};
+
+static const u32 imx_lcdc_formats[] = {
+	DRM_FORMAT_RGB565, DRM_FORMAT_XRGB8888,
+};
+
+static inline struct imx_lcdc *imx_lcdc_from_drmdev(struct drm_device *drm)
+{
+	return container_of(drm, struct imx_lcdc, drm);
+}
+
+static unsigned int imx_lcdc_get_format(unsigned int drm_format)
+{
+	unsigned int bpp;
+
+	switch (drm_format) {
+	default:
+		DRM_WARN("Format not supported - fallback to XRGB8888\n");
+		fallthrough;
+
+	case DRM_FORMAT_XRGB8888:
+		bpp = BPP_XRGB8888;
+		break;
+
+	case DRM_FORMAT_RGB565:
+		bpp = BPP_RGB565;
+		break;
+	}
+
+	return bpp;
+}
+
+static void imx_lcdc_update_hw_registers(struct drm_simple_display_pipe *pipe,
+					 struct drm_plane_state *old_state,
+					 bool mode_set)
+{
+	struct drm_crtc *crtc = &pipe->crtc;
+	struct drm_plane_state *new_state = pipe->plane.state;
+	struct drm_framebuffer *fb = new_state->fb;
+	struct imx_lcdc *lcdc = imx_lcdc_from_drmdev(pipe->crtc.dev);
+	u32 lpcr, lvcr, lhcr;
+	u32 framesize;
+	dma_addr_t addr;
+
+	addr = drm_fb_dma_get_gem_addr(fb, new_state, 0);
+	/* The LSSAR register specifies the LCD screen start address (SSA). */
+	writel(addr, lcdc->base + IMX21LCDC_LSSAR);
+
+	if (!mode_set)
+		return;
+
+	/* Disable PER clock to make register write possible */
+	if (old_state && old_state->crtc && old_state->crtc->enabled)
+		clk_disable_unprepare(lcdc->clk_per);
+
+	/* Framesize */
+	framesize = FIELD_PREP(IMX21LCDC_LSR_XMAX, crtc->mode.hdisplay >> 4) |
+		FIELD_PREP(IMX21LCDC_LSR_YMAX, crtc->mode.vdisplay);
+	writel(framesize, lcdc->base + IMX21LCDC_LSR);
+
+	/* HSYNC */
+	lhcr = FIELD_PREP(IMX21LCDC_LHCR_HFPORCH, crtc->mode.hsync_start - crtc->mode.hdisplay - 1) |
+		FIELD_PREP(IMX21LCDC_LHCR_HWIDTH, crtc->mode.hsync_end - crtc->mode.hsync_start - 1) |
+		FIELD_PREP(IMX21LCDC_LHCR_HBPORCH, crtc->mode.htotal - crtc->mode.hsync_end - 3);
+	writel(lhcr, lcdc->base + IMX21LCDC_LHCR);
+
+	/* VSYNC */
+	lvcr = FIELD_PREP(IMX21LCDC_LVCR_VFPORCH, crtc->mode.vsync_start - crtc->mode.vdisplay) |
+		FIELD_PREP(IMX21LCDC_LVCR_VWIDTH, crtc->mode.vsync_end - crtc->mode.vsync_start) |
+		FIELD_PREP(IMX21LCDC_LVCR_VBPORCH, crtc->mode.vtotal - crtc->mode.vsync_end);
+	writel(lvcr, lcdc->base + IMX21LCDC_LVCR);
+
+	lpcr = readl(lcdc->base + IMX21LCDC_LPCR);
+	lpcr &= ~IMX21LCDC_LPCR_BPIX;
+	lpcr |= FIELD_PREP(IMX21LCDC_LPCR_BPIX, imx_lcdc_get_format(fb->format->format));
+	writel(lpcr, lcdc->base + IMX21LCDC_LPCR);
+
+	/* Virtual Page Width */
+	writel(new_state->fb->pitches[0] / 4, lcdc->base + IMX21LCDC_LVPWR);
+
+	/* Enable PER clock */
+	if (new_state->crtc->enabled)
+		clk_prepare_enable(lcdc->clk_per);
+}
+
+static void imx_lcdc_pipe_enable(struct drm_simple_display_pipe *pipe,
+				 struct drm_crtc_state *crtc_state,
+				 struct drm_plane_state *plane_state)
+{
+	int ret;
+	int clk_div;
+	int bpp;
+	struct imx_lcdc *lcdc = imx_lcdc_from_drmdev(pipe->crtc.dev);
+	struct drm_display_mode *mode = &pipe->crtc.mode;
+	struct drm_display_info *disp_info = &lcdc->connector->display_info;
+	const int hsync_pol = (mode->flags & DRM_MODE_FLAG_PHSYNC) ? 0 : 1;
+	const int vsync_pol = (mode->flags & DRM_MODE_FLAG_PVSYNC) ? 0 : 1;
+	const int data_enable_pol =
+		(disp_info->bus_flags & DRM_BUS_FLAG_DE_HIGH) ? 0 : 1;
+	const int clk_pol =
+		(disp_info->bus_flags & DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE) ? 0 : 1;
+
+	clk_div = DIV_ROUND_CLOSEST_ULL(clk_get_rate(lcdc->clk_per),
+					mode->clock * 1000);
+	bpp = imx_lcdc_get_format(plane_state->fb->format->format);
+
+	writel(FIELD_PREP(IMX21LCDC_LPCR_PCD, clk_div - 1) |
+	       FIELD_PREP(IMX21LCDC_LPCR_LPPOL, hsync_pol) |
+	       FIELD_PREP(IMX21LCDC_LPCR_FLMPOL, vsync_pol) |
+	       FIELD_PREP(IMX21LCDC_LPCR_OEPOL, data_enable_pol) |
+	       FIELD_PREP(IMX21LCDC_LPCR_TFT, 1) |
+	       FIELD_PREP(IMX21LCDC_LPCR_COLOR, 1) |
+	       FIELD_PREP(IMX21LCDC_LPCR_PBSIZ, 3) |
+	       FIELD_PREP(IMX21LCDC_LPCR_BPIX, bpp) |
+	       FIELD_PREP(IMX21LCDC_LPCR_SCLKSEL, 1) |
+	       FIELD_PREP(IMX21LCDC_LPCR_PIXPOL, 0) |
+	       FIELD_PREP(IMX21LCDC_LPCR_CLKPOL, clk_pol),
+	       lcdc->base + IMX21LCDC_LPCR);
+
+	/* 0px panning offset */
+	writel(0x00000000, lcdc->base + IMX21LCDC_LPOR);
+
+	/* disable hardware cursor */
+	writel(readl(lcdc->base + IMX21LCDC_LCPR) & ~(IMX21LCDC_LCPR_CC0 | IMX21LCDC_LCPR_CC1),
+	       lcdc->base + IMX21LCDC_LCPR);
+
+	ret = clk_prepare_enable(lcdc->clk_ipg);
+	if (ret) {
+		dev_err(pipe->crtc.dev->dev, "Cannot enable ipg clock: %pe\n", ERR_PTR(ret));
+		return;
+	}
+	ret = clk_prepare_enable(lcdc->clk_ahb);
+	if (ret) {
+		dev_err(pipe->crtc.dev->dev, "Cannot enable ahb clock: %pe\n", ERR_PTR(ret));
+
+		clk_disable_unprepare(lcdc->clk_ipg);
+
+		return;
+	}
+
+	imx_lcdc_update_hw_registers(pipe, NULL, true);
+
+	/* Enable VBLANK Interrupt */
+	writel(INTR_EOF, lcdc->base + IMX21LCDC_LIER);
+}
+
+static void imx_lcdc_pipe_disable(struct drm_simple_display_pipe *pipe)
+{
+	struct imx_lcdc *lcdc = imx_lcdc_from_drmdev(pipe->crtc.dev);
+	struct drm_crtc *crtc = &lcdc->pipe.crtc;
+	struct drm_pending_vblank_event *event;
+
+	clk_disable_unprepare(lcdc->clk_ahb);
+	clk_disable_unprepare(lcdc->clk_ipg);
+
+	if (pipe->crtc.enabled)
+		clk_disable_unprepare(lcdc->clk_per);
+
+	spin_lock_irq(&lcdc->drm.event_lock);
+	event = crtc->state->event;
+	if (event) {
+		crtc->state->event = NULL;
+		drm_crtc_send_vblank_event(crtc, event);
+	}
+	spin_unlock_irq(&lcdc->drm.event_lock);
+
+	/* Disable VBLANK Interrupt */
+	writel(0, lcdc->base + IMX21LCDC_LIER);
+}
+
+static int imx_lcdc_pipe_check(struct drm_simple_display_pipe *pipe,
+			       struct drm_plane_state *plane_state,
+			       struct drm_crtc_state *crtc_state)
+{
+	const struct drm_display_mode *mode = &crtc_state->mode;
+	const struct drm_display_mode *old_mode = &pipe->crtc.state->mode;
+
+	if (mode->hdisplay < LCDC_MIN_XRES || mode->hdisplay > LCDC_MAX_XRES ||
+	    mode->vdisplay < LCDC_MIN_YRES || mode->vdisplay > LCDC_MAX_YRES ||
+	    mode->hdisplay % 0x10) { /* must be multiple of 16 */
+		drm_err(pipe->crtc.dev, "unsupported display mode (%u x %u)\n",
+			mode->hdisplay, mode->vdisplay);
+		return -EINVAL;
+	}
+
+	crtc_state->mode_changed =
+		old_mode->hdisplay != mode->hdisplay ||
+		old_mode->vdisplay != mode->vdisplay;
+
+	return 0;
+}
+
+static void imx_lcdc_pipe_update(struct drm_simple_display_pipe *pipe,
+				 struct drm_plane_state *old_state)
+{
+	struct drm_crtc *crtc = &pipe->crtc;
+	struct drm_pending_vblank_event *event = crtc->state->event;
+	struct drm_plane_state *new_state = pipe->plane.state;
+	struct drm_framebuffer *fb = new_state->fb;
+	struct drm_framebuffer *old_fb = old_state->fb;
+	struct drm_crtc *old_crtc = old_state->crtc;
+	bool mode_changed = false;
+
+	if (old_fb && old_fb->format != fb->format)
+		mode_changed = true;
+	else if (old_crtc != crtc)
+		mode_changed = true;
+
+	imx_lcdc_update_hw_registers(pipe, old_state, mode_changed);
+
+	if (event) {
+		crtc->state->event = NULL;
+
+		spin_lock_irq(&crtc->dev->event_lock);
+
+		if (crtc->state->active && drm_crtc_vblank_get(crtc) == 0)
+			drm_crtc_arm_vblank_event(crtc, event);
+		else
+			drm_crtc_send_vblank_event(crtc, event);
+
+		spin_unlock_irq(&crtc->dev->event_lock);
+	}
+}
+
+static const struct drm_simple_display_pipe_funcs imx_lcdc_pipe_funcs = {
+	.enable = imx_lcdc_pipe_enable,
+	.disable = imx_lcdc_pipe_disable,
+	.check = imx_lcdc_pipe_check,
+	.update = imx_lcdc_pipe_update,
+};
+
+static const struct drm_mode_config_funcs imx_lcdc_mode_config_funcs = {
+	.fb_create = drm_gem_fb_create_with_dirty,
+	.atomic_check = drm_atomic_helper_check,
+	.atomic_commit = drm_atomic_helper_commit,
+};
+
+static const struct drm_mode_config_helper_funcs imx_lcdc_mode_config_helpers = {
+	.atomic_commit_tail = drm_atomic_helper_commit_tail_rpm,
+};
+
+static void imx_lcdc_release(struct drm_device *drm)
+{
+	struct imx_lcdc *lcdc = imx_lcdc_from_drmdev(drm);
+
+	drm_kms_helper_poll_fini(drm);
+	kfree(lcdc);
+}
+
+DEFINE_DRM_GEM_DMA_FOPS(imx_lcdc_drm_fops);
+
+static struct drm_driver imx_lcdc_drm_driver = {
+	.driver_features = DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC,
+	.fops = &imx_lcdc_drm_fops,
+	DRM_GEM_DMA_DRIVER_OPS_VMAP,
+	.release = imx_lcdc_release,
+	.name = "imx-lcdc",
+	.desc = "i.MX LCDC driver",
+	.date = "20200716",
+};
+
+static const struct of_device_id imx_lcdc_of_dev_id[] = {
+	{
+		.compatible = "fsl,imx21-lcdc",
+	},
+	{
+		.compatible = "fsl,imx25-lcdc",
+	},
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, imx_lcdc_of_dev_id);
+
+static irqreturn_t imx_lcdc_irq_handler(int irq, void *arg)
+{
+	struct imx_lcdc *lcdc = arg;
+	struct drm_crtc *crtc = &lcdc->pipe.crtc;
+	unsigned int status;
+
+	status = readl(lcdc->base + IMX21LCDC_LISR);
+
+	if (status & INTR_EOF) {
+		drm_crtc_handle_vblank(crtc);
+		return IRQ_HANDLED;
+	}
+
+	return IRQ_NONE;
+}
+
+static int imx_lcdc_probe(struct platform_device *pdev)
+{
+	struct imx_lcdc *lcdc;
+	struct drm_device *drm;
+	int irq;
+	int ret;
+	struct device *dev = &pdev->dev;
+
+	lcdc = devm_drm_dev_alloc(dev, &imx_lcdc_drm_driver,
+				  struct imx_lcdc, drm);
+	if (!lcdc)
+		return -ENOMEM;
+
+	drm = &lcdc->drm;
+
+	lcdc->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(lcdc->base))
+		return dev_err_probe(dev, PTR_ERR(lcdc->base), "Cannot get IO memory\n");
+
+	lcdc->bridge = devm_drm_of_get_bridge(dev, dev->of_node, 0, 0);
+	if (IS_ERR(lcdc->bridge))
+		return dev_err_probe(dev, PTR_ERR(lcdc->bridge), "Failed to find bridge\n");
+
+	/* Get Clocks */
+	lcdc->clk_ipg = devm_clk_get(dev, "ipg");
+	if (IS_ERR(lcdc->clk_ipg))
+		return dev_err_probe(dev, PTR_ERR(lcdc->clk_ipg), "Failed to get %s clk\n", "ipg");
+
+	lcdc->clk_ahb = devm_clk_get(dev, "ahb");
+	if (IS_ERR(lcdc->clk_ahb))
+		return dev_err_probe(dev, PTR_ERR(lcdc->clk_ahb), "Failed to get %s clk\n", "ahb");
+
+	lcdc->clk_per = devm_clk_get(dev, "per");
+	if (IS_ERR(lcdc->clk_per))
+		return dev_err_probe(dev, PTR_ERR(lcdc->clk_per), "Failed to get %s clk\n", "per");
+
+	ret = dma_set_mask_and_coherent(drm->dev, DMA_BIT_MASK(32));
+	if (ret)
+		return dev_err_probe(dev, ret, "Cannot set DMA Mask\n");
+
+	/* Modeset init */
+	ret = drmm_mode_config_init(drm);
+	if (ret)
+		return dev_err_probe(dev, ret, "Cannot initialize mode configuration structure\n");
+
+	/* CRTC, Plane, Encoder */
+	ret = drm_simple_display_pipe_init(drm, &lcdc->pipe,
+					   &imx_lcdc_pipe_funcs,
+					   imx_lcdc_formats,
+					   ARRAY_SIZE(imx_lcdc_formats), NULL, NULL);
+	if (ret < 0)
+		return dev_err_probe(drm->dev, ret, "Cannot setup simple display pipe\n");
+
+	ret = drm_vblank_init(drm, drm->mode_config.num_crtc);
+	if (ret < 0)
+		return dev_err_probe(drm->dev, ret, "Failed to initialize vblank\n");
+
+	ret = drm_bridge_attach(&lcdc->pipe.encoder, lcdc->bridge, NULL, DRM_BRIDGE_ATTACH_NO_CONNECTOR);
+	if (ret)
+		return dev_err_probe(drm->dev, ret, "Cannot attach bridge\n");
+
+	lcdc->connector = drm_bridge_connector_init(drm, &lcdc->pipe.encoder);
+	if (IS_ERR(lcdc->connector))
+		return dev_err_probe(drm->dev, PTR_ERR(lcdc->connector), "Cannot init bridge connector\n");
+
+	drm_connector_attach_encoder(lcdc->connector, &lcdc->pipe.encoder);
+
+	/*
+	 * The LCDC controller does not have an enable bit. The
+	 * controller starts directly when the clocks are enabled.
+	 * If the clocks are enabled when the controller is not yet
+	 * programmed with proper register values (enabled at the
+	 * bootloader, for example) then it just goes into some undefined
+	 * state.
+	 * To avoid this issue, let's enable and disable LCDC IPG,
+	 * PER and AHB clock so that we force some kind of 'reset'
+	 * to the LCDC block.
+	 */
+
+	ret = clk_prepare_enable(lcdc->clk_ipg);
+	if (ret)
+		return dev_err_probe(dev, ret, "Cannot enable ipg clock\n");
+	clk_disable_unprepare(lcdc->clk_ipg);
+
+	ret = clk_prepare_enable(lcdc->clk_per);
+	if (ret)
+		return dev_err_probe(dev, ret, "Cannot enable per clock\n");
+	clk_disable_unprepare(lcdc->clk_per);
+
+	ret = clk_prepare_enable(lcdc->clk_ahb);
+	if (ret)
+		return dev_err_probe(dev, ret, "Cannot enable ahb clock\n");
+	clk_disable_unprepare(lcdc->clk_ahb);
+
+	drm->mode_config.min_width = LCDC_MIN_XRES;
+	drm->mode_config.max_width = LCDC_MAX_XRES;
+	drm->mode_config.min_height = LCDC_MIN_YRES;
+	drm->mode_config.max_height = LCDC_MAX_YRES;
+	drm->mode_config.preferred_depth = 16;
+	drm->mode_config.funcs = &imx_lcdc_mode_config_funcs;
+	drm->mode_config.helper_private = &imx_lcdc_mode_config_helpers;
+
+	drm_mode_config_reset(drm);
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0) {
+		ret = irq;
+		return ret;
+	}
+
+	ret = devm_request_irq(dev, irq, imx_lcdc_irq_handler, 0, "imx-lcdc", lcdc);
+	if (ret < 0)
+		return dev_err_probe(drm->dev, ret, "Failed to install IRQ handler\n");
+
+	platform_set_drvdata(pdev, drm);
+
+	ret = drm_dev_register(&lcdc->drm, 0);
+	if (ret)
+		return dev_err_probe(dev, ret, "Cannot register device\n");
+
+	drm_fbdev_generic_setup(drm, 0);
+
+	return 0;
+}
+
+static int imx_lcdc_remove(struct platform_device *pdev)
+{
+	struct drm_device *drm = platform_get_drvdata(pdev);
+
+	drm_dev_unregister(drm);
+	drm_atomic_helper_shutdown(drm);
+
+	return 0;
+}
+
+static void imx_lcdc_shutdown(struct platform_device *pdev)
+{
+	drm_atomic_helper_shutdown(platform_get_drvdata(pdev));
+}
+
+static struct platform_driver imx_lcdc_driver = {
+	.driver = {
+		.name = "imx-lcdc",
+		.of_match_table = imx_lcdc_of_dev_id,
+	},
+	.probe = imx_lcdc_probe,
+	.remove = imx_lcdc_remove,
+	.shutdown = imx_lcdc_shutdown,
+};
+module_platform_driver(imx_lcdc_driver);
+
+MODULE_AUTHOR("Marian Cichy <M.Cichy@pengutronix.de>");
+MODULE_DESCRIPTION("Freescale i.MX LCDC driver");
+MODULE_LICENSE("GPL");
-- 
2.39.0


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH v5 2/2] drm/imx/lcdc: Implement DRM driver for imx25
  2023-02-10 18:00   ` Uwe Kleine-König
@ 2023-02-10 20:59     ` Uwe Kleine-König
  -1 siblings, 0 replies; 7+ messages in thread
From: Uwe Kleine-König @ 2023-02-10 20:59 UTC (permalink / raw)
  To: David Airlie, Daniel Vetter, Shawn Guo, Sascha Hauer,
	Philipp Zabel, Sam Ravnborg, Laurent Pinchart, Danilo Krummrich,
	Liu Ying
  Cc: linux-arm-kernel, dri-devel, Fabio Estevam, NXP Linux Team,
	Pengutronix Kernel Team


[-- Attachment #1.1: Type: text/plain, Size: 1014 bytes --]

Hello,

On Fri, Feb 10, 2023 at 07:00:14PM +0100, Uwe Kleine-König wrote:
> +	ret = drm_bridge_attach(&lcdc->pipe.encoder, lcdc->bridge, NULL, DRM_BRIDGE_ATTACH_NO_CONNECTOR);
> +	if (ret)
> +		return dev_err_probe(drm->dev, ret, "Cannot attach bridge\n");
> +
> +	lcdc->connector = drm_bridge_connector_init(drm, &lcdc->pipe.encoder);
> +	if (IS_ERR(lcdc->connector))
> +		return dev_err_probe(drm->dev, PTR_ERR(lcdc->connector), "Cannot init bridge connector\n");
> +
> +	drm_connector_attach_encoder(lcdc->connector, &lcdc->pipe.encoder);

At one point when I talked to Philipp, we wondered if it was sensible to
create a helper function for the above sequence of drm_bridge_attach +
drm_bridge_connector_init + drm_connector_attach_encoder.

Would that make sense? What would be a good name for such a function?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v5 2/2] drm/imx/lcdc: Implement DRM driver for imx25
@ 2023-02-10 20:59     ` Uwe Kleine-König
  0 siblings, 0 replies; 7+ messages in thread
From: Uwe Kleine-König @ 2023-02-10 20:59 UTC (permalink / raw)
  To: David Airlie, Daniel Vetter, Shawn Guo, Sascha Hauer,
	Philipp Zabel, Sam Ravnborg, Laurent Pinchart, Danilo Krummrich,
	Liu Ying
  Cc: Pengutronix Kernel Team, dri-devel, linux-arm-kernel,
	NXP Linux Team

[-- Attachment #1: Type: text/plain, Size: 1014 bytes --]

Hello,

On Fri, Feb 10, 2023 at 07:00:14PM +0100, Uwe Kleine-König wrote:
> +	ret = drm_bridge_attach(&lcdc->pipe.encoder, lcdc->bridge, NULL, DRM_BRIDGE_ATTACH_NO_CONNECTOR);
> +	if (ret)
> +		return dev_err_probe(drm->dev, ret, "Cannot attach bridge\n");
> +
> +	lcdc->connector = drm_bridge_connector_init(drm, &lcdc->pipe.encoder);
> +	if (IS_ERR(lcdc->connector))
> +		return dev_err_probe(drm->dev, PTR_ERR(lcdc->connector), "Cannot init bridge connector\n");
> +
> +	drm_connector_attach_encoder(lcdc->connector, &lcdc->pipe.encoder);

At one point when I talked to Philipp, we wondered if it was sensible to
create a helper function for the above sequence of drm_bridge_attach +
drm_bridge_connector_init + drm_connector_attach_encoder.

Would that make sense? What would be a good name for such a function?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v5 2/2] drm/imx/lcdc: Implement DRM driver for imx25
  2023-02-10 18:00   ` Uwe Kleine-König
@ 2023-03-06 11:25     ` Philipp Zabel
  -1 siblings, 0 replies; 7+ messages in thread
From: Philipp Zabel @ 2023-03-06 11:25 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: David Airlie, Daniel Vetter, Shawn Guo, Sascha Hauer,
	Sam Ravnborg, Laurent Pinchart, Danilo Krummrich, Liu Ying,
	Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team, dri-devel,
	linux-arm-kernel

Hi Uwe,

just a few nitpicks, see below.

On Fri, Feb 10, 2023 at 07:00:14PM +0100, Uwe Kleine-König wrote:
> From: Marian Cichy <m.cichy@pengutronix.de>
> 
> Add support for the LCD Controller found on i.MX21 and i.MX25.
> 
> It targets to be a drop in replacement for the imx-fb driver.
> 
> Signed-off-by: Marian Cichy <m.cichy@pengutronix.de>
> [ukl: Rebase to a newer kernel version, various smaller fixes and
> improvements]
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/gpu/drm/imx/Kconfig         |   1 +
>  drivers/gpu/drm/imx/Makefile        |   1 +
>  drivers/gpu/drm/imx/lcdc/Kconfig    |   7 +
>  drivers/gpu/drm/imx/lcdc/Makefile   |   1 +
>  drivers/gpu/drm/imx/lcdc/imx-lcdc.c | 553 ++++++++++++++++++++++++++++
>  5 files changed, 563 insertions(+)
>  create mode 100644 drivers/gpu/drm/imx/lcdc/Kconfig
>  create mode 100644 drivers/gpu/drm/imx/lcdc/Makefile
>  create mode 100644 drivers/gpu/drm/imx/lcdc/imx-lcdc.c
> 
[...]
> diff --git a/drivers/gpu/drm/imx/lcdc/imx-lcdc.c b/drivers/gpu/drm/imx/lcdc/imx-lcdc.c
> new file mode 100644
> index 000000000000..c2197fc50306
> --- /dev/null
> +++ b/drivers/gpu/drm/imx/lcdc/imx-lcdc.c
> @@ -0,0 +1,553 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +// SPDX-FileCopyrightText: 2020 Marian Cichy <M.Cichy@pengutronix.de>
> +
> +#include <drm/drm_bridge_connector.h>
> +#include <drm/drm_damage_helper.h>
> +#include <drm/drm_drv.h>
> +#include <drm/drm_fbdev_generic.h>
> +#include <drm/drm_fb_dma_helper.h>
> +#include <drm/drm_fourcc.h>
> +#include <drm/drm_fourcc.h>

Choose one, remove the other.

[...]
> +struct imx_lcdc {
> +	struct drm_device drm;
> +	struct drm_simple_display_pipe pipe;
> +	const struct drm_display_mode *mode;

The mode pointer appears to be unused.

> +	struct drm_bridge *bridge;

The bridge could be a local variable in _probe().

> +	struct drm_connector *connector;
> +	void __iomem *base;
> +
> +	struct clk *clk_ipg;
> +	struct clk *clk_ahb;
> +	struct clk *clk_per;
> +};
> +
> +static const u32 imx_lcdc_formats[] = {
> +	DRM_FORMAT_RGB565, DRM_FORMAT_XRGB8888,
> +};
> +
> +static inline struct imx_lcdc *imx_lcdc_from_drmdev(struct drm_device *drm)
> +{
> +	return container_of(drm, struct imx_lcdc, drm);
> +}
> +
> +static unsigned int imx_lcdc_get_format(unsigned int drm_format)
> +{
> +	unsigned int bpp;
> +
> +	switch (drm_format) {
> +	default:
> +		DRM_WARN("Format not supported - fallback to XRGB8888\n");
> +		fallthrough;
> +
> +	case DRM_FORMAT_XRGB8888:
> +		bpp = BPP_XRGB8888;
> +		break;

This could just return directly, no need for the local bpp variable.

> +
> +	case DRM_FORMAT_RGB565:
> +		bpp = BPP_RGB565;
> +		break;
> +	}
> +
> +	return bpp;
> +}
> +
[...]
> +
> +static int imx_lcdc_probe(struct platform_device *pdev)
> +{
> +	struct imx_lcdc *lcdc;
> +	struct drm_device *drm;
> +	int irq;
> +	int ret;
> +	struct device *dev = &pdev->dev;
> +
> +	lcdc = devm_drm_dev_alloc(dev, &imx_lcdc_drm_driver,
> +				  struct imx_lcdc, drm);
> +	if (!lcdc)
> +		return -ENOMEM;
> +
> +	drm = &lcdc->drm;
> +
> +	lcdc->base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(lcdc->base))
> +		return dev_err_probe(dev, PTR_ERR(lcdc->base), "Cannot get IO memory\n");
> +
> +	lcdc->bridge = devm_drm_of_get_bridge(dev, dev->of_node, 0, 0);
> +	if (IS_ERR(lcdc->bridge))
> +		return dev_err_probe(dev, PTR_ERR(lcdc->bridge), "Failed to find bridge\n");
[...]
> +	ret = drm_bridge_attach(&lcdc->pipe.encoder, lcdc->bridge, NULL, DRM_BRIDGE_ATTACH_NO_CONNECTOR);
> +	if (ret)
> +		return dev_err_probe(drm->dev, ret, "Cannot attach bridge\n");

The bridge could be a local variable.

With those addressed,

Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>

regards
Philipp

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v5 2/2] drm/imx/lcdc: Implement DRM driver for imx25
@ 2023-03-06 11:25     ` Philipp Zabel
  0 siblings, 0 replies; 7+ messages in thread
From: Philipp Zabel @ 2023-03-06 11:25 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: Pengutronix Kernel Team, Liu Ying, Shawn Guo, Sascha Hauer,
	dri-devel, Danilo Krummrich, Laurent Pinchart, Sam Ravnborg,
	linux-arm-kernel, NXP Linux Team

Hi Uwe,

just a few nitpicks, see below.

On Fri, Feb 10, 2023 at 07:00:14PM +0100, Uwe Kleine-König wrote:
> From: Marian Cichy <m.cichy@pengutronix.de>
> 
> Add support for the LCD Controller found on i.MX21 and i.MX25.
> 
> It targets to be a drop in replacement for the imx-fb driver.
> 
> Signed-off-by: Marian Cichy <m.cichy@pengutronix.de>
> [ukl: Rebase to a newer kernel version, various smaller fixes and
> improvements]
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/gpu/drm/imx/Kconfig         |   1 +
>  drivers/gpu/drm/imx/Makefile        |   1 +
>  drivers/gpu/drm/imx/lcdc/Kconfig    |   7 +
>  drivers/gpu/drm/imx/lcdc/Makefile   |   1 +
>  drivers/gpu/drm/imx/lcdc/imx-lcdc.c | 553 ++++++++++++++++++++++++++++
>  5 files changed, 563 insertions(+)
>  create mode 100644 drivers/gpu/drm/imx/lcdc/Kconfig
>  create mode 100644 drivers/gpu/drm/imx/lcdc/Makefile
>  create mode 100644 drivers/gpu/drm/imx/lcdc/imx-lcdc.c
> 
[...]
> diff --git a/drivers/gpu/drm/imx/lcdc/imx-lcdc.c b/drivers/gpu/drm/imx/lcdc/imx-lcdc.c
> new file mode 100644
> index 000000000000..c2197fc50306
> --- /dev/null
> +++ b/drivers/gpu/drm/imx/lcdc/imx-lcdc.c
> @@ -0,0 +1,553 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +// SPDX-FileCopyrightText: 2020 Marian Cichy <M.Cichy@pengutronix.de>
> +
> +#include <drm/drm_bridge_connector.h>
> +#include <drm/drm_damage_helper.h>
> +#include <drm/drm_drv.h>
> +#include <drm/drm_fbdev_generic.h>
> +#include <drm/drm_fb_dma_helper.h>
> +#include <drm/drm_fourcc.h>
> +#include <drm/drm_fourcc.h>

Choose one, remove the other.

[...]
> +struct imx_lcdc {
> +	struct drm_device drm;
> +	struct drm_simple_display_pipe pipe;
> +	const struct drm_display_mode *mode;

The mode pointer appears to be unused.

> +	struct drm_bridge *bridge;

The bridge could be a local variable in _probe().

> +	struct drm_connector *connector;
> +	void __iomem *base;
> +
> +	struct clk *clk_ipg;
> +	struct clk *clk_ahb;
> +	struct clk *clk_per;
> +};
> +
> +static const u32 imx_lcdc_formats[] = {
> +	DRM_FORMAT_RGB565, DRM_FORMAT_XRGB8888,
> +};
> +
> +static inline struct imx_lcdc *imx_lcdc_from_drmdev(struct drm_device *drm)
> +{
> +	return container_of(drm, struct imx_lcdc, drm);
> +}
> +
> +static unsigned int imx_lcdc_get_format(unsigned int drm_format)
> +{
> +	unsigned int bpp;
> +
> +	switch (drm_format) {
> +	default:
> +		DRM_WARN("Format not supported - fallback to XRGB8888\n");
> +		fallthrough;
> +
> +	case DRM_FORMAT_XRGB8888:
> +		bpp = BPP_XRGB8888;
> +		break;

This could just return directly, no need for the local bpp variable.

> +
> +	case DRM_FORMAT_RGB565:
> +		bpp = BPP_RGB565;
> +		break;
> +	}
> +
> +	return bpp;
> +}
> +
[...]
> +
> +static int imx_lcdc_probe(struct platform_device *pdev)
> +{
> +	struct imx_lcdc *lcdc;
> +	struct drm_device *drm;
> +	int irq;
> +	int ret;
> +	struct device *dev = &pdev->dev;
> +
> +	lcdc = devm_drm_dev_alloc(dev, &imx_lcdc_drm_driver,
> +				  struct imx_lcdc, drm);
> +	if (!lcdc)
> +		return -ENOMEM;
> +
> +	drm = &lcdc->drm;
> +
> +	lcdc->base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(lcdc->base))
> +		return dev_err_probe(dev, PTR_ERR(lcdc->base), "Cannot get IO memory\n");
> +
> +	lcdc->bridge = devm_drm_of_get_bridge(dev, dev->of_node, 0, 0);
> +	if (IS_ERR(lcdc->bridge))
> +		return dev_err_probe(dev, PTR_ERR(lcdc->bridge), "Failed to find bridge\n");
[...]
> +	ret = drm_bridge_attach(&lcdc->pipe.encoder, lcdc->bridge, NULL, DRM_BRIDGE_ATTACH_NO_CONNECTOR);
> +	if (ret)
> +		return dev_err_probe(drm->dev, ret, "Cannot attach bridge\n");

The bridge could be a local variable.

With those addressed,

Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>

regards
Philipp

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2023-03-06 11:27 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-02-12 13:23 [PATCH v5 2/2] drm/imx/lcdc: Implement DRM driver for imx25 kernel test robot
  -- strict thread matches above, loose matches on Subject: below --
2023-02-10 18:00 [PATCH v5 0/2] " Uwe Kleine-König
2023-02-10 18:00 ` [PATCH v5 2/2] " Uwe Kleine-König
2023-02-10 18:00   ` Uwe Kleine-König
2023-02-10 20:59   ` Uwe Kleine-König
2023-02-10 20:59     ` Uwe Kleine-König
2023-03-06 11:25   ` Philipp Zabel
2023-03-06 11:25     ` Philipp Zabel

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.