[5.2][PATCH 0/3] Ingenic JZ47xx KMS driver

[5.2][PATCH 0/3] Ingenic JZ47xx KMS driver

[Paul Cercueil]

Hi,

This is a first attempt at a KMS driver for the JZ47xx MIPS SoCs by
Ingenic. It is aimed to replace the aging jz4740-fb driver.

The driver will later be updated with new features (overlays, TV-out
etc.), that's why I didn't go with the simple/tiny DRM driver.

The driver has been tested on the Ben Nanonote (JZ4740) and the
RetroMini RS-90 (JZ4725B) handheld gaming console.

Greetings,
-Paul

[PATCH 1/3] dt-bindings: Add doc for the ingenic-drm driver

[Paul Cercueil]

Add documentation for the devicetree bindings of the DRM driver for the
JZ47xx family of SoCs from Ingenic.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
Tested-by: Artur Rojek <contact@artur-rojek.eu>
---
 .../devicetree/bindings/display/ingenic,drm.txt    | 30 ++++++++++++++++++++++
 1 file changed, 30 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/ingenic,drm.txt

diff --git a/Documentation/devicetree/bindings/display/ingenic,drm.txt b/Documentation/devicetree/bindings/display/ingenic,drm.txt
new file mode 100644
index 000000000000..52a368ec35cd
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/ingenic,drm.txt
@@ -0,0 +1,30 @@
+Ingenic JZ47xx DRM driver
+
+Required properties:
+- compatible: one of:
+  * ingenic,jz4740-drm
+  * ingenic,jz4725b-drm
+- reg: LCD registers location and length
+- clocks: LCD pixclock and device clock specifiers.
+	   The device clock is only required on the JZ4740.
+- clock-names: "lcd_pclk" and "lcd"
+- interrupts: Specifies the interrupt line the LCD controller is connected to.
+
+Optional properties:
+- ingenic,panel: phandle to a display panel, if one is present
+- ingenic,lcd-mode: LCD mode to use with the display panel.
+		    See <dt-bindings/display/ingenic,drm.h> for all the
+		    possible values.
+
+Example:
+
+lcd: lcd-controller@13050000 {
+	compatible = "ingenic,jz4725b-drm";
+	reg = <0x13050000 0x1000>;
+
+	interrupt-parent = <&intc>;
+	interrupts = <31>;
+
+	clocks = <&cgu JZ4725B_CLK_LCD>;
+	clock-names = "lcd";
+};
-- 
2.11.0

[PATCH 2/3] dt-bindings: Add header for the ingenic-drm driver bindings

[Paul Cercueil]

Add macros that can be used with the ingenic,lcd-mode property in the
devicetree node that corresponds to the ingenic-drm driver.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
Tested-by: Artur Rojek <contact@artur-rojek.eu>
---
 include/dt-bindings/display/ingenic,drm.h | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)
 create mode 100644 include/dt-bindings/display/ingenic,drm.h

diff --git a/include/dt-bindings/display/ingenic,drm.h b/include/dt-bindings/display/ingenic,drm.h
new file mode 100644
index 000000000000..20c5399366c1
--- /dev/null
+++ b/include/dt-bindings/display/ingenic,drm.h
@@ -0,0 +1,28 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ *  Ingenic JZ47xx KMS driver
+ *
+ *  Copyright (C) 2019, Paul Cercueil <paul@crapouillou.net>
+ */
+#ifndef __INCLUDE_DT_BINDINGS_DISPLAY_INGENIC_DRM_H__
+#define __INCLUDE_DT_BINDINGS_DISPLAY_INGENIC_DRM_H__
+
+#define JZ_DRM_LCD_GENERIC_16BIT		0
+#define JZ_DRM_LCD_GENERIC_18BIT		16
+
+#define JZ_DRM_LCD_SPECIAL_TFT_1		1
+#define JZ_DRM_LCD_SPECIAL_TFT_2		2
+#define JZ_DRM_LCD_SPECIAL_TFT_3		3
+
+#define JZ_DRM_LCD_NON_INTERLACED_TV_OUT	4
+#define JZ_DRM_LCD_INTERLACED_TV_OUT		6
+
+#define JZ_DRM_LCD_SINGLE_COLOR_STN		8
+#define JZ_DRM_LCD_SINGLE_MONOCHROME_STN	9
+#define JZ_DRM_LCD_DUAL_COLOR_STN		10
+#define JZ_DRM_LCD_DUAL_MONOCHROME_STN		11
+
+#define JZ_LCD_TYPE_8BIT_SERIAL			12
+#define JZ_LCD_TYPE_LCM				13
+
+#endif /* __INCLUDE_DT_BINDINGS_DISPLAY_INGENIC_DRM_H__ */
-- 
2.11.0

[PATCH 3/3] DRM: Add KMS driver for the Ingenic JZ47xx SoCs

[Paul Cercueil]

Add a KMS driver for the Ingenic JZ47xx family of SoCs.
This driver is meant to replace the aging jz4740-fb driver.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
Tested-by: Artur Rojek <contact@artur-rojek.eu>
---
 drivers/gpu/drm/Kconfig               |   2 +
 drivers/gpu/drm/Makefile              |   1 +
 drivers/gpu/drm/ingenic/Kconfig       |  16 +
 drivers/gpu/drm/ingenic/Makefile      |   1 +
 drivers/gpu/drm/ingenic/ingenic-drm.c | 896 ++++++++++++++++++++++++++++++++++
 5 files changed, 916 insertions(+)
 create mode 100644 drivers/gpu/drm/ingenic/Kconfig
 create mode 100644 drivers/gpu/drm/ingenic/Makefile
 create mode 100644 drivers/gpu/drm/ingenic/ingenic-drm.c

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index bd943a71756c..f1929112e559 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -303,6 +303,8 @@ source "drivers/gpu/drm/sti/Kconfig"
 
 source "drivers/gpu/drm/imx/Kconfig"
 
+source "drivers/gpu/drm/ingenic/Kconfig"
+
 source "drivers/gpu/drm/v3d/Kconfig"
 
 source "drivers/gpu/drm/vc4/Kconfig"
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 1ac55c65eac0..9666c0767f44 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -94,6 +94,7 @@ obj-$(CONFIG_DRM_TEGRA) += tegra/
 obj-$(CONFIG_DRM_STM) += stm/
 obj-$(CONFIG_DRM_STI) += sti/
 obj-$(CONFIG_DRM_IMX) += imx/
+obj-y			+= ingenic/
 obj-$(CONFIG_DRM_MEDIATEK) += mediatek/
 obj-$(CONFIG_DRM_MESON)	+= meson/
 obj-y			+= i2c/
diff --git a/drivers/gpu/drm/ingenic/Kconfig b/drivers/gpu/drm/ingenic/Kconfig
new file mode 100644
index 000000000000..d82c3d37ec9c
--- /dev/null
+++ b/drivers/gpu/drm/ingenic/Kconfig
@@ -0,0 +1,16 @@
+config DRM_INGENIC
+	tristate "DRM Support for Ingenic SoCs"
+	depends on MIPS || COMPILE_TEST
+	depends on DRM
+	depends on CMA
+	depends on OF
+	select DRM_BRIDGE
+	select DRM_PANEL_BRIDGE
+	select DRM_KMS_HELPER
+	select DRM_KMS_CMA_HELPER
+	select DRM_GEM_CMA_HELPER
+	select VT_HW_CONSOLE_BINDING if FRAMEBUFFER_CONSOLE
+	help
+	  Choose this option for DRM support for the Ingenic SoCs.
+
+	  If M is selected the module will be called ingenic-drm.
diff --git a/drivers/gpu/drm/ingenic/Makefile b/drivers/gpu/drm/ingenic/Makefile
new file mode 100644
index 000000000000..11cac42ce0bb
--- /dev/null
+++ b/drivers/gpu/drm/ingenic/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_DRM_INGENIC) += ingenic-drm.o
diff --git a/drivers/gpu/drm/ingenic/ingenic-drm.c b/drivers/gpu/drm/ingenic/ingenic-drm.c
new file mode 100644
index 000000000000..18120deea96f
--- /dev/null
+++ b/drivers/gpu/drm/ingenic/ingenic-drm.c
@@ -0,0 +1,896 @@
+// SPDX-License-Identifier: GPL-2.0
+//
+// Ingenic JZ47xx KMS driver
+//
+// Copyright (C) 2019, Paul Cercueil <paul@crapouillou.net>
+
+#include <linux/clk.h>
+#include <linux/dma-mapping.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+
+#include <drm/drm_atomic.h>
+#include <drm/drm_atomic_helper.h>
+#include <drm/drm_crtc.h>
+#include <drm/drm_crtc_helper.h>
+#include <drm/drm_drv.h>
+#include <drm/drm_gem_cma_helper.h>
+#include <drm/drm_fb_cma_helper.h>
+#include <drm/drm_fb_helper.h>
+#include <drm/drm_fourcc.h>
+#include <drm/drm_gem_framebuffer_helper.h>
+#include <drm/drm_irq.h>
+#include <drm/drm_panel.h>
+#include <drm/drm_plane.h>
+#include <drm/drm_plane_helper.h>
+#include <drm/drm_probe_helper.h>
+#include <drm/drm_vblank.h>
+
+#include <dt-bindings/display/ingenic,drm.h>
+
+#include "../drm_internal.h"
+
+#define JZ_REG_LCD_CFG		0x00
+#define JZ_REG_LCD_VSYNC	0x04
+#define JZ_REG_LCD_HSYNC	0x08
+#define JZ_REG_LCD_VAT		0x0C
+#define JZ_REG_LCD_DAH		0x10
+#define JZ_REG_LCD_DAV		0x14
+#define JZ_REG_LCD_PS		0x18
+#define JZ_REG_LCD_CLS		0x1C
+#define JZ_REG_LCD_SPL		0x20
+#define JZ_REG_LCD_REV		0x24
+#define JZ_REG_LCD_CTRL		0x30
+#define JZ_REG_LCD_STATE	0x34
+#define JZ_REG_LCD_IID		0x38
+#define JZ_REG_LCD_DA0		0x40
+#define JZ_REG_LCD_SA0		0x44
+#define JZ_REG_LCD_FID0		0x48
+#define JZ_REG_LCD_CMD0		0x4C
+#define JZ_REG_LCD_DA1		0x50
+#define JZ_REG_LCD_SA1		0x54
+#define JZ_REG_LCD_FID1		0x58
+#define JZ_REG_LCD_CMD1		0x5C
+
+#define JZ_LCD_CFG_SLCD			BIT(31)
+#define JZ_LCD_CFG_PS_DISABLE		BIT(23)
+#define JZ_LCD_CFG_CLS_DISABLE		BIT(22)
+#define JZ_LCD_CFG_SPL_DISABLE		BIT(21)
+#define JZ_LCD_CFG_REV_DISABLE		BIT(20)
+#define JZ_LCD_CFG_HSYNCM		BIT(19)
+#define JZ_LCD_CFG_PCLKM		BIT(18)
+#define JZ_LCD_CFG_INV			BIT(17)
+#define JZ_LCD_CFG_SYNC_DIR		BIT(16)
+#define JZ_LCD_CFG_PS_POLARITY		BIT(15)
+#define JZ_LCD_CFG_CLS_POLARITY		BIT(14)
+#define JZ_LCD_CFG_SPL_POLARITY		BIT(13)
+#define JZ_LCD_CFG_REV_POLARITY		BIT(12)
+#define JZ_LCD_CFG_HSYNC_ACTIVE_LOW	BIT(11)
+#define JZ_LCD_CFG_PCLK_FALLING_EDGE	BIT(10)
+#define JZ_LCD_CFG_DE_ACTIVE_LOW	BIT(9)
+#define JZ_LCD_CFG_VSYNC_ACTIVE_LOW	BIT(8)
+#define JZ_LCD_CFG_18_BIT		BIT(7)
+#define JZ_LCD_CFG_PDW			(BIT(5) | BIT(4))
+#define JZ_LCD_CFG_MODE_MASK		0xf
+
+#define JZ_LCD_VSYNC_VPS_OFFSET		16
+#define JZ_LCD_VSYNC_VPE_OFFSET		0
+
+#define JZ_LCD_HSYNC_HPS_OFFSET		16
+#define JZ_LCD_HSYNC_HPE_OFFSET		0
+
+#define JZ_LCD_VAT_HT_OFFSET		16
+#define JZ_LCD_VAT_VT_OFFSET		0
+
+#define JZ_LCD_DAH_HDS_OFFSET		16
+#define JZ_LCD_DAH_HDE_OFFSET		0
+
+#define JZ_LCD_DAV_VDS_OFFSET		16
+#define JZ_LCD_DAV_VDE_OFFSET		0
+
+#define JZ_LCD_CTRL_BURST_4		(0x0 << 28)
+#define JZ_LCD_CTRL_BURST_8		(0x1 << 28)
+#define JZ_LCD_CTRL_BURST_16		(0x2 << 28)
+#define JZ_LCD_CTRL_RGB555		BIT(27)
+#define JZ_LCD_CTRL_OFUP		BIT(26)
+#define JZ_LCD_CTRL_FRC_GRAYSCALE_16	(0x0 << 24)
+#define JZ_LCD_CTRL_FRC_GRAYSCALE_4	(0x1 << 24)
+#define JZ_LCD_CTRL_FRC_GRAYSCALE_2	(0x2 << 24)
+#define JZ_LCD_CTRL_PDD_MASK		(0xff << 16)
+#define JZ_LCD_CTRL_EOF_IRQ		BIT(13)
+#define JZ_LCD_CTRL_SOF_IRQ		BIT(12)
+#define JZ_LCD_CTRL_OFU_IRQ		BIT(11)
+#define JZ_LCD_CTRL_IFU0_IRQ		BIT(10)
+#define JZ_LCD_CTRL_IFU1_IRQ		BIT(9)
+#define JZ_LCD_CTRL_DD_IRQ		BIT(8)
+#define JZ_LCD_CTRL_QDD_IRQ		BIT(7)
+#define JZ_LCD_CTRL_REVERSE_ENDIAN	BIT(6)
+#define JZ_LCD_CTRL_LSB_FISRT		BIT(5)
+#define JZ_LCD_CTRL_DISABLE		BIT(4)
+#define JZ_LCD_CTRL_ENABLE		BIT(3)
+#define JZ_LCD_CTRL_BPP_1		0x0
+#define JZ_LCD_CTRL_BPP_2		0x1
+#define JZ_LCD_CTRL_BPP_4		0x2
+#define JZ_LCD_CTRL_BPP_8		0x3
+#define JZ_LCD_CTRL_BPP_15_16		0x4
+#define JZ_LCD_CTRL_BPP_18_24		0x5
+#define JZ_LCD_CTRL_BPP_MASK		(JZ_LCD_CTRL_RGB555 | (0x7 << 0))
+
+#define JZ_LCD_CMD_SOF_IRQ		BIT(31)
+#define JZ_LCD_CMD_EOF_IRQ		BIT(30)
+#define JZ_LCD_CMD_ENABLE_PAL		BIT(28)
+
+#define JZ_LCD_SYNC_MASK		0x3ff
+
+#define JZ_LCD_STATE_EOF_IRQ		BIT(5)
+#define JZ_LCD_STATE_SOF_IRQ		BIT(4)
+#define JZ_LCD_STATE_DISABLED		BIT(0)
+
+struct ingenic_framedesc {
+	uint32_t next;
+	uint32_t addr;
+	uint32_t id;
+	uint32_t cmd;
+} __packed;
+
+struct jz_soc_info {
+	bool needs_dev_clk;
+};
+
+struct ingenic_drm {
+	struct device *dev;
+	void __iomem *base;
+	struct regmap *map;
+	struct clk *lcd_clk, *pix_clk;
+
+	u32 lcd_mode;
+
+	struct ingenic_framedesc *framedesc;
+	dma_addr_t framedesc_phys;
+
+	struct drm_device *drm;
+	struct drm_plane primary;
+	struct drm_crtc crtc;
+	struct drm_connector connector;
+	struct drm_encoder encoder;
+	struct drm_panel *panel;
+
+	struct device_node *panel_node;
+
+	unsigned short ps_start, ps_end, cls_start,
+		       cls_end, spl_start, spl_end, rev_start;
+};
+
+static const uint32_t ingenic_drm_primary_formats[] = {
+	DRM_FORMAT_XRGB1555,
+	DRM_FORMAT_RGB565,
+	DRM_FORMAT_XRGB8888,
+};
+
+static bool ingenic_drm_writeable_reg(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	case JZ_REG_LCD_IID:
+	case JZ_REG_LCD_SA0:
+	case JZ_REG_LCD_FID0:
+	case JZ_REG_LCD_CMD0:
+	case JZ_REG_LCD_SA1:
+	case JZ_REG_LCD_FID1:
+	case JZ_REG_LCD_CMD1:
+		return false;
+	default:
+		return true;
+	}
+}
+
+static const struct regmap_config ingenic_drm_regmap_config = {
+	.reg_bits = 32,
+	.val_bits = 32,
+	.reg_stride = 4,
+
+	.max_register = JZ_REG_LCD_CMD1,
+	.writeable_reg = ingenic_drm_writeable_reg,
+};
+
+static inline struct ingenic_drm *drm_crtc_get_priv(struct drm_crtc *crtc)
+{
+	return container_of(crtc, struct ingenic_drm, crtc);
+}
+
+static inline struct ingenic_drm *drm_plane_get_priv(struct drm_plane *plane)
+{
+	return container_of(plane, struct ingenic_drm, primary);
+}
+
+static inline struct ingenic_drm *drm_conn_get_priv(struct drm_connector *conn)
+{
+	return container_of(conn, struct ingenic_drm, connector);
+}
+
+static void ingenic_drm_crtc_atomic_enable(struct drm_crtc *crtc,
+					  struct drm_crtc_state *state)
+{
+	struct ingenic_drm *priv = drm_crtc_get_priv(crtc);
+
+	regmap_write(priv->map, JZ_REG_LCD_STATE, 0);
+
+	regmap_update_bits(priv->map, JZ_REG_LCD_CTRL,
+			   JZ_LCD_CTRL_ENABLE | JZ_LCD_CTRL_DISABLE,
+			   JZ_LCD_CTRL_ENABLE);
+
+	drm_panel_enable(priv->panel);
+
+	drm_crtc_vblank_on(crtc);
+}
+
+static void ingenic_drm_crtc_atomic_disable(struct drm_crtc *crtc,
+					   struct drm_crtc_state *state)
+{
+	struct ingenic_drm *priv = drm_crtc_get_priv(crtc);
+	unsigned int var;
+
+	drm_crtc_vblank_off(crtc);
+
+	drm_panel_disable(priv->panel);
+
+	regmap_update_bits(priv->map, JZ_REG_LCD_CTRL,
+			   JZ_LCD_CTRL_DISABLE, JZ_LCD_CTRL_DISABLE);
+
+	regmap_read_poll_timeout(priv->map, JZ_REG_LCD_STATE, var,
+				 var & JZ_LCD_STATE_DISABLED,
+				 1000, 0);
+}
+
+static inline bool ingenic_drm_lcd_is_special_mode(u32 mode)
+{
+	switch (mode) {
+	case JZ_DRM_LCD_SPECIAL_TFT_1:
+	case JZ_DRM_LCD_SPECIAL_TFT_2:
+	case JZ_DRM_LCD_SPECIAL_TFT_3:
+		return true;
+	default:
+		return false;
+	}
+}
+
+static inline bool ingenic_drm_lcd_is_stn_mode(u32 mode)
+{
+	switch (mode) {
+	case JZ_DRM_LCD_SINGLE_COLOR_STN:
+	case JZ_DRM_LCD_SINGLE_MONOCHROME_STN:
+	case JZ_DRM_LCD_DUAL_COLOR_STN:
+	case JZ_DRM_LCD_DUAL_MONOCHROME_STN:
+		return true;
+	default:
+		return false;
+	}
+}
+
+static void ingenic_drm_crtc_update_timings(struct ingenic_drm *priv,
+					    struct drm_display_mode *mode)
+{
+	unsigned int vpe, vds, vde, vt, hpe, hds, hde, ht;
+
+	vpe = mode->vsync_end - mode->vsync_start;
+	vds = mode->vtotal - mode->vsync_start;
+	vde = vds + mode->vdisplay;
+	vt = vde + mode->vsync_start - mode->vdisplay;
+
+	hpe = mode->hsync_end - mode->hsync_start;
+	hds = mode->htotal - mode->hsync_start;
+	hde = hds + mode->hdisplay;
+	ht = hde + mode->hsync_start - mode->hdisplay;
+
+	regmap_write(priv->map, JZ_REG_LCD_VSYNC,
+		     0 << JZ_LCD_VSYNC_VPS_OFFSET |
+		     vpe << JZ_LCD_VSYNC_VPE_OFFSET);
+
+	regmap_write(priv->map, JZ_REG_LCD_HSYNC,
+		     0 << JZ_LCD_HSYNC_HPS_OFFSET |
+		     hpe << JZ_LCD_HSYNC_HPE_OFFSET);
+
+	regmap_write(priv->map, JZ_REG_LCD_VAT,
+		     ht << JZ_LCD_VAT_HT_OFFSET |
+		     vt << JZ_LCD_VAT_VT_OFFSET);
+
+	regmap_write(priv->map, JZ_REG_LCD_DAH,
+		     hds << JZ_LCD_DAH_HDS_OFFSET |
+		     hde << JZ_LCD_DAH_HDE_OFFSET);
+	regmap_write(priv->map, JZ_REG_LCD_DAV,
+		     vds << JZ_LCD_DAV_VDS_OFFSET |
+		     vde << JZ_LCD_DAV_VDE_OFFSET);
+
+	if (ingenic_drm_lcd_is_special_mode(priv->lcd_mode)) {
+		regmap_write(priv->map, JZ_REG_LCD_PS, hde << 16 | (hde + 1));
+		regmap_write(priv->map, JZ_REG_LCD_CLS, hde << 16 | (hde + 1));
+		regmap_write(priv->map, JZ_REG_LCD_SPL, hpe << 16 | (hpe + 1));
+		regmap_write(priv->map, JZ_REG_LCD_REV, mode->htotal << 16);
+	}
+}
+
+static void ingenic_drm_crtc_update_cfg(struct ingenic_drm *priv,
+					struct drm_display_mode *mode)
+
+{
+	unsigned int cfg = priv->lcd_mode;
+	u32 bus_flags = priv->connector.display_info.bus_flags;
+
+	if (mode->flags & DRM_MODE_FLAG_NHSYNC)
+		cfg |= JZ_LCD_CFG_HSYNC_ACTIVE_LOW;
+	if (mode->flags & DRM_MODE_FLAG_NVSYNC)
+		cfg |= JZ_LCD_CFG_VSYNC_ACTIVE_LOW;
+	if (bus_flags & DRM_BUS_FLAG_DE_LOW)
+		cfg |= JZ_LCD_CFG_DE_ACTIVE_LOW;
+	if (bus_flags & DRM_BUS_FLAG_PIXDATA_NEGEDGE)
+		cfg |= JZ_LCD_CFG_PCLK_FALLING_EDGE;
+
+	if (ingenic_drm_lcd_is_special_mode(priv->lcd_mode)) {
+		// TODO: Is that valid for all special modes?
+		cfg |= JZ_LCD_CFG_REV_POLARITY;
+	} else {
+		cfg |= JZ_LCD_CFG_PS_DISABLE
+		    | JZ_LCD_CFG_CLS_DISABLE
+		    | JZ_LCD_CFG_SPL_DISABLE
+		    | JZ_LCD_CFG_REV_DISABLE;
+	}
+
+	regmap_write(priv->map, JZ_REG_LCD_CFG, cfg);
+}
+
+static void ingenic_drm_crtc_update_ctrl(struct ingenic_drm *priv,
+					 unsigned int bpp)
+{
+	unsigned int ctrl = JZ_LCD_CTRL_OFUP | JZ_LCD_CTRL_BURST_16;
+
+	switch (bpp) {
+	case 1:
+		ctrl |= JZ_LCD_CTRL_BPP_1;
+		break;
+	case 2:
+		ctrl |= JZ_LCD_CTRL_BPP_2;
+		break;
+	case 4:
+		ctrl |= JZ_LCD_CTRL_BPP_4;
+		break;
+	case 8:
+		ctrl |= JZ_LCD_CTRL_BPP_8;
+	break;
+	case 15:
+		ctrl |= JZ_LCD_CTRL_RGB555; /* Falltrough */
+	case 16:
+		ctrl |= JZ_LCD_CTRL_BPP_15_16;
+		break;
+	case 18:
+	case 24:
+	case 32:
+		ctrl |= JZ_LCD_CTRL_BPP_18_24;
+		break;
+	default:
+		break;
+	}
+
+	regmap_update_bits(priv->map, JZ_REG_LCD_CTRL,
+			   JZ_LCD_CTRL_OFUP | JZ_LCD_CTRL_BURST_16 |
+			   JZ_LCD_CTRL_BPP_MASK, ctrl);
+}
+
+static int ingenic_drm_crtc_atomic_check(struct drm_crtc *crtc,
+					 struct drm_crtc_state *state)
+{
+	struct ingenic_drm *priv = drm_crtc_get_priv(crtc);
+	long rate;
+
+	if (!drm_atomic_crtc_needs_modeset(state))
+		return 0;
+
+	rate = clk_round_rate(priv->pix_clk,
+			      state->adjusted_mode.clock * 1000);
+	if (rate < 0)
+		return rate;
+
+	return 0;
+}
+
+static void ingenic_drm_crtc_atomic_flush(struct drm_crtc *crtc,
+					  struct drm_crtc_state *oldstate)
+{
+	struct ingenic_drm *priv = drm_crtc_get_priv(crtc);
+	struct drm_crtc_state *state = crtc->state;
+	struct drm_pending_vblank_event *event = state->event;
+	struct drm_framebuffer *drm_fb = crtc->primary->state->fb;
+	const struct drm_format_info *finfo;
+	unsigned int width, height;
+
+	if (drm_atomic_crtc_needs_modeset(state)) {
+		finfo = drm_format_info(drm_fb->format->format);
+		width = state->adjusted_mode.hdisplay;
+		height = state->adjusted_mode.vdisplay;
+
+		ingenic_drm_crtc_update_timings(priv, &state->mode);
+
+		ingenic_drm_crtc_update_ctrl(priv, finfo->depth);
+		ingenic_drm_crtc_update_cfg(priv, &state->adjusted_mode);
+
+		clk_set_rate(priv->pix_clk, state->adjusted_mode.clock * 1000);
+
+		regmap_write(priv->map, JZ_REG_LCD_DA0, priv->framedesc->next);
+	}
+
+	if (event) {
+		state->event = NULL;
+
+		spin_lock_irq(&crtc->dev->event_lock);
+		if (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 void ingenic_drm_plane_atomic_update(struct drm_plane *plane,
+					    struct drm_plane_state *oldstate)
+{
+	struct ingenic_drm *priv = drm_plane_get_priv(plane);
+	struct drm_plane_state *state = plane->state;
+	const struct drm_format_info *finfo;
+	unsigned int width, height;
+
+	finfo = drm_format_info(state->fb->format->format);
+	width = state->crtc->state->adjusted_mode.hdisplay;
+	height = state->crtc->state->adjusted_mode.vdisplay;
+
+	priv->framedesc->addr = drm_fb_cma_get_gem_addr(state->fb, state, 0);
+
+	priv->framedesc->cmd = width * height * ((finfo->depth + 7) / 8) / 4;
+	priv->framedesc->cmd |= JZ_LCD_CMD_EOF_IRQ;
+}
+
+static enum drm_connector_status
+ingenic_drm_conn_detect(struct drm_connector *conn, bool force)
+{
+	struct ingenic_drm *priv = drm_conn_get_priv(conn);
+
+	if (priv->panel) {
+		drm_panel_attach(priv->panel, conn);
+
+		return connector_status_connected;
+	}
+
+	return connector_status_disconnected;
+}
+
+static void ingenic_drm_conn_destroy(struct drm_connector *conn)
+{
+	struct ingenic_drm *priv = drm_conn_get_priv(conn);
+
+	if (priv->panel)
+		drm_panel_detach(priv->panel);
+
+	drm_connector_cleanup(conn);
+}
+
+static int ingenic_drm_conn_get_modes(struct drm_connector *conn)
+{
+	struct ingenic_drm *priv = drm_conn_get_priv(conn);
+
+	if (priv->panel)
+		return priv->panel->funcs->get_modes(priv->panel);
+
+	return 0;
+}
+
+static irqreturn_t ingenic_drm_irq_handler(int irq, void *arg)
+{
+	struct drm_device *drm = arg;
+	struct ingenic_drm *priv = drm->dev_private;
+	unsigned int state;
+
+	regmap_read(priv->map, JZ_REG_LCD_STATE, &state);
+
+	regmap_update_bits(priv->map, JZ_REG_LCD_STATE,
+			   JZ_LCD_STATE_EOF_IRQ, 0);
+
+	if (state & JZ_LCD_STATE_EOF_IRQ)
+		drm_crtc_handle_vblank(&priv->crtc);
+
+	return IRQ_HANDLED;
+}
+
+static int ingenic_drm_enable_vblank(struct drm_crtc *crtc)
+{
+	struct ingenic_drm *priv = drm_crtc_get_priv(crtc);
+
+	regmap_update_bits(priv->map, JZ_REG_LCD_CTRL,
+			   JZ_LCD_CTRL_EOF_IRQ, JZ_LCD_CTRL_EOF_IRQ);
+
+	return 0;
+}
+
+static void ingenic_drm_disable_vblank(struct drm_crtc *crtc)
+{
+	struct ingenic_drm *priv = drm_crtc_get_priv(crtc);
+
+	regmap_update_bits(priv->map, JZ_REG_LCD_CTRL, JZ_LCD_CTRL_EOF_IRQ, 0);
+}
+
+DEFINE_DRM_GEM_CMA_FOPS(ingenic_drm_fops);
+
+static struct drm_driver ingenic_drm_driver_data = {
+	.driver_features	= DRIVER_MODESET | DRIVER_GEM | DRIVER_PRIME
+				| DRIVER_ATOMIC | DRIVER_HAVE_IRQ,
+	.name			= "ingenic-drm",
+	.desc			= "DRM module for Ingenic SoCs",
+	.date			= "20190228",
+	.major			= 1,
+	.minor			= 0,
+	.patchlevel		= 0,
+
+	.fops			= &ingenic_drm_fops,
+
+	.dumb_create		= drm_gem_cma_dumb_create,
+	.gem_free_object_unlocked = drm_gem_cma_free_object,
+	.gem_vm_ops		= &drm_gem_cma_vm_ops,
+
+	.prime_handle_to_fd	= drm_gem_prime_handle_to_fd,
+	.prime_fd_to_handle	= drm_gem_prime_fd_to_handle,
+	.gem_prime_import	= drm_gem_prime_import,
+	.gem_prime_export	= drm_gem_prime_export,
+	.gem_prime_get_sg_table	= drm_gem_cma_prime_get_sg_table,
+	.gem_prime_import_sg_table = drm_gem_cma_prime_import_sg_table,
+	.gem_prime_vmap		= drm_gem_cma_prime_vmap,
+	.gem_prime_vunmap	= drm_gem_cma_prime_vunmap,
+	.gem_prime_mmap		= drm_gem_cma_prime_mmap,
+
+	.irq_handler		= ingenic_drm_irq_handler,
+};
+
+static const struct drm_plane_funcs ingenic_drm_primary_plane_funcs = {
+	.update_plane		= drm_atomic_helper_update_plane,
+	.disable_plane		= drm_atomic_helper_disable_plane,
+	.reset			= drm_atomic_helper_plane_reset,
+
+	.atomic_duplicate_state	= drm_atomic_helper_plane_duplicate_state,
+	.atomic_destroy_state	= drm_atomic_helper_plane_destroy_state,
+};
+
+static const struct drm_crtc_funcs ingenic_drm_crtc_funcs = {
+	.reset			= drm_atomic_helper_crtc_reset,
+	.set_config		= drm_atomic_helper_set_config,
+	.page_flip		= drm_atomic_helper_page_flip,
+
+	.atomic_duplicate_state	= drm_atomic_helper_crtc_duplicate_state,
+	.atomic_destroy_state	= drm_atomic_helper_crtc_destroy_state,
+
+	.enable_vblank		= ingenic_drm_enable_vblank,
+	.disable_vblank		= ingenic_drm_disable_vblank,
+
+	.gamma_set		= drm_atomic_helper_legacy_gamma_set,
+};
+
+static const struct drm_connector_funcs ingenic_drm_conn_funcs = {
+	.fill_modes		= drm_helper_probe_single_connector_modes,
+	.detect			= ingenic_drm_conn_detect,
+	.destroy		= ingenic_drm_conn_destroy,
+	.reset			= drm_atomic_helper_connector_reset,
+	.atomic_duplicate_state	= drm_atomic_helper_connector_duplicate_state,
+	.atomic_destroy_state	= drm_atomic_helper_connector_destroy_state,
+};
+
+static const struct drm_plane_helper_funcs ingenic_drm_plane_helper_funcs = {
+	.atomic_update		= ingenic_drm_plane_atomic_update,
+	.prepare_fb		= drm_gem_fb_prepare_fb,
+};
+
+static const struct drm_crtc_helper_funcs ingenic_drm_crtc_helper_funcs = {
+	.atomic_enable		= ingenic_drm_crtc_atomic_enable,
+	.atomic_disable		= ingenic_drm_crtc_atomic_disable,
+	.atomic_flush		= ingenic_drm_crtc_atomic_flush,
+	.atomic_check		= ingenic_drm_crtc_atomic_check,
+};
+
+static const struct drm_connector_helper_funcs ingenic_drm_conn_helper_funcs = {
+	.get_modes		= ingenic_drm_conn_get_modes,
+};
+
+static const struct drm_mode_config_funcs ingenic_drm_mode_config_funcs = {
+	.fb_create		= drm_gem_fb_create,
+	.output_poll_changed	= drm_fb_helper_output_poll_changed,
+	.atomic_check		= drm_atomic_helper_check,
+	.atomic_commit		= drm_atomic_helper_commit,
+};
+
+static const struct drm_encoder_funcs ingenic_drm_encoder_funcs = {
+	.destroy		= drm_encoder_cleanup,
+};
+
+static int ingenic_drm_probe(struct platform_device *pdev)
+{
+	const struct jz_soc_info *soc_info;
+	struct device *dev = &pdev->dev;
+	struct ingenic_drm *priv;
+	struct clk *parent_clk;
+	struct drm_device *drm;
+	struct resource *mem;
+	void __iomem *base;
+	long parent_rate;
+	int ret, irq;
+
+	soc_info = device_get_match_data(dev);
+	if (!soc_info)
+		return -EINVAL;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->dev = dev;
+
+	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	priv->base = base = devm_ioremap_resource(dev, mem);
+	if (IS_ERR(base))
+		return PTR_ERR(base);
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0) {
+		dev_err(dev, "Failed to get platform irq\n");
+		return -ENOENT;
+	}
+
+	priv->map = devm_regmap_init_mmio(dev, base,
+					  &ingenic_drm_regmap_config);
+	if (IS_ERR(priv->map)) {
+		dev_err(dev, "Failed to create regmap\n");
+		return PTR_ERR(priv->map);
+	}
+
+	if (soc_info->needs_dev_clk) {
+		priv->lcd_clk = devm_clk_get(dev, "lcd");
+		if (IS_ERR(priv->lcd_clk)) {
+			dev_err(dev, "Failed to get lcd clock\n");
+			return PTR_ERR(priv->lcd_clk);
+		}
+	}
+
+	priv->pix_clk = devm_clk_get(dev, "lcd_pclk");
+	if (IS_ERR(priv->pix_clk)) {
+		dev_err(dev, "Failed to get pixel clock\n");
+		return PTR_ERR(priv->pix_clk);
+	}
+
+	priv->panel_node = of_parse_phandle(dev->of_node, "ingenic,panel", 0);
+	if (!priv->panel_node) {
+		DRM_INFO("No panel found");
+	} else {
+		priv->panel = of_drm_find_panel(priv->panel_node);
+		of_node_put(priv->panel_node);
+
+		if (IS_ERR(priv->panel)) {
+			if (PTR_ERR(priv->panel) == -EPROBE_DEFER)
+				return -EPROBE_DEFER;
+
+			priv->panel = NULL;
+		} else {
+			ret = drm_panel_prepare(priv->panel);
+			if (ret && ret != -ENOSYS)
+				return ret;
+		}
+	}
+
+	if (priv->panel) {
+		ret = device_property_read_u32(dev, "ingenic,lcd-mode",
+					       &priv->lcd_mode);
+		if (ret) {
+			dev_err(dev, "Unable to read ingenic,lcd-mode property\n");
+			return ret;
+		}
+	}
+
+	priv->framedesc = dma_alloc_coherent(dev, sizeof(*priv->framedesc),
+					     &priv->framedesc_phys, GFP_KERNEL);
+	if (!priv->framedesc)
+		return -ENOMEM;
+
+	priv->framedesc->next = priv->framedesc_phys;
+	priv->framedesc->id = 0xdeafbead;
+
+	drm = drm_dev_alloc(&ingenic_drm_driver_data, dev);
+	if (IS_ERR(drm)) {
+		ret = PTR_ERR(drm);
+		goto err_free_dma;
+	}
+
+	priv->drm = drm;
+
+	drm_mode_config_init(drm);
+	drm->mode_config.min_width = 0;
+	drm->mode_config.min_height = 0;
+	drm->mode_config.max_width = 800;
+	drm->mode_config.max_height = 600;
+	drm->mode_config.funcs = &ingenic_drm_mode_config_funcs;
+
+	drm_plane_helper_add(&priv->primary, &ingenic_drm_plane_helper_funcs);
+
+	ret = drm_universal_plane_init(drm, &priv->primary,
+				       0, &ingenic_drm_primary_plane_funcs,
+				       ingenic_drm_primary_formats,
+				       ARRAY_SIZE(ingenic_drm_primary_formats),
+				       NULL, DRM_PLANE_TYPE_PRIMARY, NULL);
+	if (ret) {
+		dev_err(dev, "Failed to register primary plane: %i\n", ret);
+		goto err_unref_drm;
+	}
+
+	drm_crtc_helper_add(&priv->crtc, &ingenic_drm_crtc_helper_funcs);
+
+	ret = drm_crtc_init_with_planes(drm, &priv->crtc, &priv->primary,
+					NULL, &ingenic_drm_crtc_funcs, NULL);
+	if (ret) {
+		dev_err(dev, "Failed to init CRTC: %i\n", ret);
+		goto err_cleanup_plane;
+	}
+
+	drm_connector_helper_add(&priv->connector,
+				 &ingenic_drm_conn_helper_funcs);
+	ret = drm_connector_init(drm, &priv->connector,
+				 &ingenic_drm_conn_funcs,
+				 DRM_MODE_CONNECTOR_Unknown);
+	if (ret) {
+		dev_err(dev, "Failed to init connector: %i\n", ret);
+		goto err_cleanup_crtc;
+	}
+
+	priv->encoder.possible_crtcs = 1;
+
+	ret = drm_encoder_init(drm, &priv->encoder, &ingenic_drm_encoder_funcs,
+			       DRM_MODE_ENCODER_NONE, NULL);
+	if (ret) {
+		dev_err(dev, "Failed to init encoder: %i\n", ret);
+		goto err_cleanup_connector;
+	}
+
+	drm_connector_attach_encoder(&priv->connector, &priv->encoder);
+
+	platform_set_drvdata(pdev, drm);
+	priv->drm = drm;
+	drm->dev_private = priv;
+
+	ret = drm_irq_install(drm, irq);
+	if (ret) {
+		dev_err(dev, "Unable to install IRQ handler\n");
+		goto err_cleanup_encoder;
+	}
+
+	ret = drm_vblank_init(drm, 1);
+	if (ret) {
+		dev_err(dev, "Failed calling drm_vblank_init()\n");
+		goto err_uninstall_irq;
+	}
+
+	drm_mode_config_reset(drm);
+
+	ret = clk_prepare_enable(priv->pix_clk);
+	if (ret) {
+		dev_err(dev, "Unable to start pixel clock\n");
+		goto err_cleanup_vblank;
+	}
+
+	if (priv->lcd_clk) {
+		parent_clk = clk_get_parent(priv->lcd_clk);
+		parent_rate = clk_get_rate(parent_clk);
+
+		/* LCD Device clock must be 3x the pixel clock for STN panels,
+		 * or 1.5x the pixel clock for TFT panels. To avoid having to
+		 * check for the LCD device clock everytime we do a mode change,
+		 * we set the LCD device clock to the highest rate possible.
+		 */
+		ret = clk_set_rate(priv->lcd_clk, parent_rate);
+		if (ret) {
+			dev_err(dev, "Unable to set LCD clock rate\n");
+			goto err_pixclk_disable;
+		}
+
+		ret = clk_prepare_enable(priv->lcd_clk);
+		if (ret) {
+			dev_err(dev, "Unable to start lcd clock\n");
+			goto err_pixclk_disable;
+		}
+	}
+
+	ret = drm_fbdev_generic_setup(drm, 16);
+	if (ret) {
+		dev_err(dev, "Failed to init fbdev\n");
+		goto err_devclk_disable;
+	}
+
+	ret = drm_dev_register(drm, 0);
+	if (ret) {
+		dev_err(dev, "Failed to register DRM driver\n");
+		goto err_cleanup_fbdev;
+	}
+
+	return 0;
+
+err_cleanup_fbdev:
+	drm_mode_config_cleanup(drm);
+err_devclk_disable:
+	if (priv->lcd_clk)
+		clk_disable_unprepare(priv->lcd_clk);
+err_pixclk_disable:
+	clk_disable_unprepare(priv->pix_clk);
+err_cleanup_vblank:
+	drm_vblank_cleanup(drm);
+err_uninstall_irq:
+	drm_irq_uninstall(drm);
+err_cleanup_encoder:
+	drm_encoder_cleanup(&priv->encoder);
+err_cleanup_connector:
+	drm_connector_cleanup(&priv->connector);
+err_cleanup_crtc:
+	drm_crtc_cleanup(&priv->crtc);
+err_cleanup_plane:
+	drm_plane_cleanup(&priv->primary);
+err_unref_drm:
+	drm_dev_put(drm);
+err_free_dma:
+	dma_free_coherent(dev, sizeof(*priv->framedesc),
+			  priv->framedesc, priv->framedesc_phys);
+	return ret;
+}
+
+static int ingenic_drm_remove(struct platform_device *pdev)
+{
+	struct drm_device *drm = platform_get_drvdata(pdev);
+	struct ingenic_drm *priv = drm->dev_private;
+
+	drm_dev_unregister(drm);
+	drm_mode_config_cleanup(drm);
+
+	if (priv->lcd_clk)
+		clk_disable_unprepare(priv->lcd_clk);
+	clk_disable_unprepare(priv->pix_clk);
+
+	drm_vblank_cleanup(drm);
+	drm_irq_uninstall(drm);
+
+	drm_encoder_cleanup(&priv->encoder);
+	drm_connector_cleanup(&priv->connector);
+	drm_crtc_cleanup(&priv->crtc);
+	drm_plane_cleanup(&priv->primary);
+
+	drm_dev_put(drm);
+
+	dma_free_coherent(&pdev->dev, sizeof(*priv->framedesc),
+			  priv->framedesc, priv->framedesc_phys);
+
+	return 0;
+}
+
+static const struct jz_soc_info jz4740_soc_info = {
+	.needs_dev_clk = true,
+};
+
+static const struct jz_soc_info jz4725b_soc_info = {
+	.needs_dev_clk = false,
+};
+
+static const struct of_device_id ingenic_drm_of_match[] = {
+	{ .compatible = "ingenic,jz4740-drm", .data = &jz4740_soc_info },
+	{ .compatible = "ingenic,jz4725b-drm", .data = &jz4725b_soc_info },
+	{},
+};
+
+static struct platform_driver ingenic_drm_driver = {
+	.driver = {
+		.name = "ingenic-drm",
+		.of_match_table = of_match_ptr(ingenic_drm_of_match),
+	},
+	.probe = ingenic_drm_probe,
+	.remove = ingenic_drm_remove,
+};
+module_platform_driver(ingenic_drm_driver);
+
+MODULE_AUTHOR("Paul Cercueil <paul@crapouillou.net>");
+MODULE_DESCRIPTION("DRM driver for the Ingenic SoCs\n");
+MODULE_LICENSE("GPL v2");
-- 
2.11.0

Re: [PATCH 3/3] DRM: Add KMS driver for the Ingenic JZ47xx SoCs

[Daniel Vetter]

On Thu, Feb 28, 2019 at 07:07:56PM -0300, Paul Cercueil wrote:
> Add a KMS driver for the Ingenic JZ47xx family of SoCs.
> This driver is meant to replace the aging jz4740-fb driver.
> 
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> Tested-by: Artur Rojek <contact@artur-rojek.eu>

Yay, more fbdev drivers moving to atmic!

Going on vacations for a week now, so just a few quick high-level comments
below. Those should make the driver even smaller and cleaner.

> ---
>  drivers/gpu/drm/Kconfig               |   2 +
>  drivers/gpu/drm/Makefile              |   1 +
>  drivers/gpu/drm/ingenic/Kconfig       |  16 +
>  drivers/gpu/drm/ingenic/Makefile      |   1 +
>  drivers/gpu/drm/ingenic/ingenic-drm.c | 896 ++++++++++++++++++++++++++++++++++
>  5 files changed, 916 insertions(+)
>  create mode 100644 drivers/gpu/drm/ingenic/Kconfig
>  create mode 100644 drivers/gpu/drm/ingenic/Makefile
>  create mode 100644 drivers/gpu/drm/ingenic/ingenic-drm.c
> 
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index bd943a71756c..f1929112e559 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -303,6 +303,8 @@ source "drivers/gpu/drm/sti/Kconfig"
>  
>  source "drivers/gpu/drm/imx/Kconfig"
>  
> +source "drivers/gpu/drm/ingenic/Kconfig"
> +
>  source "drivers/gpu/drm/v3d/Kconfig"
>  
>  source "drivers/gpu/drm/vc4/Kconfig"
> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> index 1ac55c65eac0..9666c0767f44 100644
> --- a/drivers/gpu/drm/Makefile
> +++ b/drivers/gpu/drm/Makefile
> @@ -94,6 +94,7 @@ obj-$(CONFIG_DRM_TEGRA) += tegra/
>  obj-$(CONFIG_DRM_STM) += stm/
>  obj-$(CONFIG_DRM_STI) += sti/
>  obj-$(CONFIG_DRM_IMX) += imx/
> +obj-y			+= ingenic/
>  obj-$(CONFIG_DRM_MEDIATEK) += mediatek/
>  obj-$(CONFIG_DRM_MESON)	+= meson/
>  obj-y			+= i2c/
> diff --git a/drivers/gpu/drm/ingenic/Kconfig b/drivers/gpu/drm/ingenic/Kconfig
> new file mode 100644
> index 000000000000..d82c3d37ec9c
> --- /dev/null
> +++ b/drivers/gpu/drm/ingenic/Kconfig
> @@ -0,0 +1,16 @@
> +config DRM_INGENIC
> +	tristate "DRM Support for Ingenic SoCs"
> +	depends on MIPS || COMPILE_TEST
> +	depends on DRM
> +	depends on CMA
> +	depends on OF
> +	select DRM_BRIDGE
> +	select DRM_PANEL_BRIDGE
> +	select DRM_KMS_HELPER
> +	select DRM_KMS_CMA_HELPER
> +	select DRM_GEM_CMA_HELPER
> +	select VT_HW_CONSOLE_BINDING if FRAMEBUFFER_CONSOLE
> +	help
> +	  Choose this option for DRM support for the Ingenic SoCs.
> +
> +	  If M is selected the module will be called ingenic-drm.
> diff --git a/drivers/gpu/drm/ingenic/Makefile b/drivers/gpu/drm/ingenic/Makefile
> new file mode 100644
> index 000000000000..11cac42ce0bb
> --- /dev/null
> +++ b/drivers/gpu/drm/ingenic/Makefile
> @@ -0,0 +1 @@
> +obj-$(CONFIG_DRM_INGENIC) += ingenic-drm.o
> diff --git a/drivers/gpu/drm/ingenic/ingenic-drm.c b/drivers/gpu/drm/ingenic/ingenic-drm.c
> new file mode 100644
> index 000000000000..18120deea96f
> --- /dev/null
> +++ b/drivers/gpu/drm/ingenic/ingenic-drm.c
> @@ -0,0 +1,896 @@
> +// SPDX-License-Identifier: GPL-2.0
> +//
> +// Ingenic JZ47xx KMS driver
> +//
> +// Copyright (C) 2019, Paul Cercueil <paul@crapouillou.net>
> +
> +#include <linux/clk.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +
> +#include <drm/drm_atomic.h>
> +#include <drm/drm_atomic_helper.h>
> +#include <drm/drm_crtc.h>
> +#include <drm/drm_crtc_helper.h>
> +#include <drm/drm_drv.h>
> +#include <drm/drm_gem_cma_helper.h>
> +#include <drm/drm_fb_cma_helper.h>
> +#include <drm/drm_fb_helper.h>
> +#include <drm/drm_fourcc.h>
> +#include <drm/drm_gem_framebuffer_helper.h>
> +#include <drm/drm_irq.h>
> +#include <drm/drm_panel.h>
> +#include <drm/drm_plane.h>
> +#include <drm/drm_plane_helper.h>
> +#include <drm/drm_probe_helper.h>
> +#include <drm/drm_vblank.h>
> +
> +#include <dt-bindings/display/ingenic,drm.h>
> +
> +#include "../drm_internal.h"
> +
> +#define JZ_REG_LCD_CFG		0x00
> +#define JZ_REG_LCD_VSYNC	0x04
> +#define JZ_REG_LCD_HSYNC	0x08
> +#define JZ_REG_LCD_VAT		0x0C
> +#define JZ_REG_LCD_DAH		0x10
> +#define JZ_REG_LCD_DAV		0x14
> +#define JZ_REG_LCD_PS		0x18
> +#define JZ_REG_LCD_CLS		0x1C
> +#define JZ_REG_LCD_SPL		0x20
> +#define JZ_REG_LCD_REV		0x24
> +#define JZ_REG_LCD_CTRL		0x30
> +#define JZ_REG_LCD_STATE	0x34
> +#define JZ_REG_LCD_IID		0x38
> +#define JZ_REG_LCD_DA0		0x40
> +#define JZ_REG_LCD_SA0		0x44
> +#define JZ_REG_LCD_FID0		0x48
> +#define JZ_REG_LCD_CMD0		0x4C
> +#define JZ_REG_LCD_DA1		0x50
> +#define JZ_REG_LCD_SA1		0x54
> +#define JZ_REG_LCD_FID1		0x58
> +#define JZ_REG_LCD_CMD1		0x5C
> +
> +#define JZ_LCD_CFG_SLCD			BIT(31)
> +#define JZ_LCD_CFG_PS_DISABLE		BIT(23)
> +#define JZ_LCD_CFG_CLS_DISABLE		BIT(22)
> +#define JZ_LCD_CFG_SPL_DISABLE		BIT(21)
> +#define JZ_LCD_CFG_REV_DISABLE		BIT(20)
> +#define JZ_LCD_CFG_HSYNCM		BIT(19)
> +#define JZ_LCD_CFG_PCLKM		BIT(18)
> +#define JZ_LCD_CFG_INV			BIT(17)
> +#define JZ_LCD_CFG_SYNC_DIR		BIT(16)
> +#define JZ_LCD_CFG_PS_POLARITY		BIT(15)
> +#define JZ_LCD_CFG_CLS_POLARITY		BIT(14)
> +#define JZ_LCD_CFG_SPL_POLARITY		BIT(13)
> +#define JZ_LCD_CFG_REV_POLARITY		BIT(12)
> +#define JZ_LCD_CFG_HSYNC_ACTIVE_LOW	BIT(11)
> +#define JZ_LCD_CFG_PCLK_FALLING_EDGE	BIT(10)
> +#define JZ_LCD_CFG_DE_ACTIVE_LOW	BIT(9)
> +#define JZ_LCD_CFG_VSYNC_ACTIVE_LOW	BIT(8)
> +#define JZ_LCD_CFG_18_BIT		BIT(7)
> +#define JZ_LCD_CFG_PDW			(BIT(5) | BIT(4))
> +#define JZ_LCD_CFG_MODE_MASK		0xf
> +
> +#define JZ_LCD_VSYNC_VPS_OFFSET		16
> +#define JZ_LCD_VSYNC_VPE_OFFSET		0
> +
> +#define JZ_LCD_HSYNC_HPS_OFFSET		16
> +#define JZ_LCD_HSYNC_HPE_OFFSET		0
> +
> +#define JZ_LCD_VAT_HT_OFFSET		16
> +#define JZ_LCD_VAT_VT_OFFSET		0
> +
> +#define JZ_LCD_DAH_HDS_OFFSET		16
> +#define JZ_LCD_DAH_HDE_OFFSET		0
> +
> +#define JZ_LCD_DAV_VDS_OFFSET		16
> +#define JZ_LCD_DAV_VDE_OFFSET		0
> +
> +#define JZ_LCD_CTRL_BURST_4		(0x0 << 28)
> +#define JZ_LCD_CTRL_BURST_8		(0x1 << 28)
> +#define JZ_LCD_CTRL_BURST_16		(0x2 << 28)
> +#define JZ_LCD_CTRL_RGB555		BIT(27)
> +#define JZ_LCD_CTRL_OFUP		BIT(26)
> +#define JZ_LCD_CTRL_FRC_GRAYSCALE_16	(0x0 << 24)
> +#define JZ_LCD_CTRL_FRC_GRAYSCALE_4	(0x1 << 24)
> +#define JZ_LCD_CTRL_FRC_GRAYSCALE_2	(0x2 << 24)
> +#define JZ_LCD_CTRL_PDD_MASK		(0xff << 16)
> +#define JZ_LCD_CTRL_EOF_IRQ		BIT(13)
> +#define JZ_LCD_CTRL_SOF_IRQ		BIT(12)
> +#define JZ_LCD_CTRL_OFU_IRQ		BIT(11)
> +#define JZ_LCD_CTRL_IFU0_IRQ		BIT(10)
> +#define JZ_LCD_CTRL_IFU1_IRQ		BIT(9)
> +#define JZ_LCD_CTRL_DD_IRQ		BIT(8)
> +#define JZ_LCD_CTRL_QDD_IRQ		BIT(7)
> +#define JZ_LCD_CTRL_REVERSE_ENDIAN	BIT(6)
> +#define JZ_LCD_CTRL_LSB_FISRT		BIT(5)
> +#define JZ_LCD_CTRL_DISABLE		BIT(4)
> +#define JZ_LCD_CTRL_ENABLE		BIT(3)
> +#define JZ_LCD_CTRL_BPP_1		0x0
> +#define JZ_LCD_CTRL_BPP_2		0x1
> +#define JZ_LCD_CTRL_BPP_4		0x2
> +#define JZ_LCD_CTRL_BPP_8		0x3
> +#define JZ_LCD_CTRL_BPP_15_16		0x4
> +#define JZ_LCD_CTRL_BPP_18_24		0x5
> +#define JZ_LCD_CTRL_BPP_MASK		(JZ_LCD_CTRL_RGB555 | (0x7 << 0))
> +
> +#define JZ_LCD_CMD_SOF_IRQ		BIT(31)
> +#define JZ_LCD_CMD_EOF_IRQ		BIT(30)
> +#define JZ_LCD_CMD_ENABLE_PAL		BIT(28)
> +
> +#define JZ_LCD_SYNC_MASK		0x3ff
> +
> +#define JZ_LCD_STATE_EOF_IRQ		BIT(5)
> +#define JZ_LCD_STATE_SOF_IRQ		BIT(4)
> +#define JZ_LCD_STATE_DISABLED		BIT(0)
> +
> +struct ingenic_framedesc {
> +	uint32_t next;
> +	uint32_t addr;
> +	uint32_t id;
> +	uint32_t cmd;
> +} __packed;
> +
> +struct jz_soc_info {
> +	bool needs_dev_clk;
> +};
> +
> +struct ingenic_drm {
> +	struct device *dev;

Embedding is kinda the new thing, since it also prevents people from using
devm_ for this, which is (strictly speaking, not that it matters much) the
wrong lifetime for drm allocations. There's some patches from Noralf that
should land soon to make this a bit more convenient.

> +	void __iomem *base;
> +	struct regmap *map;
> +	struct clk *lcd_clk, *pix_clk;
> +
> +	u32 lcd_mode;
> +
> +	struct ingenic_framedesc *framedesc;
> +	dma_addr_t framedesc_phys;
> +
> +	struct drm_device *drm;
> +	struct drm_plane primary;
> +	struct drm_crtc crtc;
> +	struct drm_connector connector;
> +	struct drm_encoder encoder;

Any reason you didn't opt for the drm_simple_display_pipe helpers? Since
you have just 1:1:1 between crtc/plane/encoder that will allow you to
ditch lots of boilerplate.

https://dri.freedesktop.org/docs/drm/gpu/drm-kms-helpers.html#simple-kms-helper-reference

> +	struct drm_panel *panel;
> +
> +	struct device_node *panel_node;

There's panel bridge helpers which takes a panel and wraps it into a
drm_bridge (which you can glue into the simple pipe helpers), giving you
the connector for free.

https://dri.freedesktop.org/docs/drm/gpu/drm-kms-helpers.html#panel-bridge-helper-reference

This should cover all the bigger items, details look already really tidy.

Cheers, Daniel

> +
> +	unsigned short ps_start, ps_end, cls_start,
> +		       cls_end, spl_start, spl_end, rev_start;
> +};
> +
> +static const uint32_t ingenic_drm_primary_formats[] = {
> +	DRM_FORMAT_XRGB1555,
> +	DRM_FORMAT_RGB565,
> +	DRM_FORMAT_XRGB8888,
> +};
> +
> +static bool ingenic_drm_writeable_reg(struct device *dev, unsigned int reg)
> +{
> +	switch (reg) {
> +	case JZ_REG_LCD_IID:
> +	case JZ_REG_LCD_SA0:
> +	case JZ_REG_LCD_FID0:
> +	case JZ_REG_LCD_CMD0:
> +	case JZ_REG_LCD_SA1:
> +	case JZ_REG_LCD_FID1:
> +	case JZ_REG_LCD_CMD1:
> +		return false;
> +	default:
> +		return true;
> +	}
> +}
> +
> +static const struct regmap_config ingenic_drm_regmap_config = {
> +	.reg_bits = 32,
> +	.val_bits = 32,
> +	.reg_stride = 4,
> +
> +	.max_register = JZ_REG_LCD_CMD1,
> +	.writeable_reg = ingenic_drm_writeable_reg,
> +};
> +
> +static inline struct ingenic_drm *drm_crtc_get_priv(struct drm_crtc *crtc)
> +{
> +	return container_of(crtc, struct ingenic_drm, crtc);
> +}
> +
> +static inline struct ingenic_drm *drm_plane_get_priv(struct drm_plane *plane)
> +{
> +	return container_of(plane, struct ingenic_drm, primary);
> +}
> +
> +static inline struct ingenic_drm *drm_conn_get_priv(struct drm_connector *conn)
> +{
> +	return container_of(conn, struct ingenic_drm, connector);
> +}
> +
> +static void ingenic_drm_crtc_atomic_enable(struct drm_crtc *crtc,
> +					  struct drm_crtc_state *state)
> +{
> +	struct ingenic_drm *priv = drm_crtc_get_priv(crtc);
> +
> +	regmap_write(priv->map, JZ_REG_LCD_STATE, 0);
> +
> +	regmap_update_bits(priv->map, JZ_REG_LCD_CTRL,
> +			   JZ_LCD_CTRL_ENABLE | JZ_LCD_CTRL_DISABLE,
> +			   JZ_LCD_CTRL_ENABLE);
> +
> +	drm_panel_enable(priv->panel);
> +
> +	drm_crtc_vblank_on(crtc);
> +}
> +
> +static void ingenic_drm_crtc_atomic_disable(struct drm_crtc *crtc,
> +					   struct drm_crtc_state *state)
> +{
> +	struct ingenic_drm *priv = drm_crtc_get_priv(crtc);
> +	unsigned int var;
> +
> +	drm_crtc_vblank_off(crtc);
> +
> +	drm_panel_disable(priv->panel);
> +
> +	regmap_update_bits(priv->map, JZ_REG_LCD_CTRL,
> +			   JZ_LCD_CTRL_DISABLE, JZ_LCD_CTRL_DISABLE);
> +
> +	regmap_read_poll_timeout(priv->map, JZ_REG_LCD_STATE, var,
> +				 var & JZ_LCD_STATE_DISABLED,
> +				 1000, 0);
> +}
> +
> +static inline bool ingenic_drm_lcd_is_special_mode(u32 mode)
> +{
> +	switch (mode) {
> +	case JZ_DRM_LCD_SPECIAL_TFT_1:
> +	case JZ_DRM_LCD_SPECIAL_TFT_2:
> +	case JZ_DRM_LCD_SPECIAL_TFT_3:
> +		return true;
> +	default:
> +		return false;
> +	}
> +}
> +
> +static inline bool ingenic_drm_lcd_is_stn_mode(u32 mode)
> +{
> +	switch (mode) {
> +	case JZ_DRM_LCD_SINGLE_COLOR_STN:
> +	case JZ_DRM_LCD_SINGLE_MONOCHROME_STN:
> +	case JZ_DRM_LCD_DUAL_COLOR_STN:
> +	case JZ_DRM_LCD_DUAL_MONOCHROME_STN:
> +		return true;
> +	default:
> +		return false;
> +	}
> +}
> +
> +static void ingenic_drm_crtc_update_timings(struct ingenic_drm *priv,
> +					    struct drm_display_mode *mode)
> +{
> +	unsigned int vpe, vds, vde, vt, hpe, hds, hde, ht;
> +
> +	vpe = mode->vsync_end - mode->vsync_start;
> +	vds = mode->vtotal - mode->vsync_start;
> +	vde = vds + mode->vdisplay;
> +	vt = vde + mode->vsync_start - mode->vdisplay;
> +
> +	hpe = mode->hsync_end - mode->hsync_start;
> +	hds = mode->htotal - mode->hsync_start;
> +	hde = hds + mode->hdisplay;
> +	ht = hde + mode->hsync_start - mode->hdisplay;
> +
> +	regmap_write(priv->map, JZ_REG_LCD_VSYNC,
> +		     0 << JZ_LCD_VSYNC_VPS_OFFSET |
> +		     vpe << JZ_LCD_VSYNC_VPE_OFFSET);
> +
> +	regmap_write(priv->map, JZ_REG_LCD_HSYNC,
> +		     0 << JZ_LCD_HSYNC_HPS_OFFSET |
> +		     hpe << JZ_LCD_HSYNC_HPE_OFFSET);
> +
> +	regmap_write(priv->map, JZ_REG_LCD_VAT,
> +		     ht << JZ_LCD_VAT_HT_OFFSET |
> +		     vt << JZ_LCD_VAT_VT_OFFSET);
> +
> +	regmap_write(priv->map, JZ_REG_LCD_DAH,
> +		     hds << JZ_LCD_DAH_HDS_OFFSET |
> +		     hde << JZ_LCD_DAH_HDE_OFFSET);
> +	regmap_write(priv->map, JZ_REG_LCD_DAV,
> +		     vds << JZ_LCD_DAV_VDS_OFFSET |
> +		     vde << JZ_LCD_DAV_VDE_OFFSET);
> +
> +	if (ingenic_drm_lcd_is_special_mode(priv->lcd_mode)) {
> +		regmap_write(priv->map, JZ_REG_LCD_PS, hde << 16 | (hde + 1));
> +		regmap_write(priv->map, JZ_REG_LCD_CLS, hde << 16 | (hde + 1));
> +		regmap_write(priv->map, JZ_REG_LCD_SPL, hpe << 16 | (hpe + 1));
> +		regmap_write(priv->map, JZ_REG_LCD_REV, mode->htotal << 16);
> +	}
> +}
> +
> +static void ingenic_drm_crtc_update_cfg(struct ingenic_drm *priv,
> +					struct drm_display_mode *mode)
> +
> +{
> +	unsigned int cfg = priv->lcd_mode;
> +	u32 bus_flags = priv->connector.display_info.bus_flags;
> +
> +	if (mode->flags & DRM_MODE_FLAG_NHSYNC)
> +		cfg |= JZ_LCD_CFG_HSYNC_ACTIVE_LOW;
> +	if (mode->flags & DRM_MODE_FLAG_NVSYNC)
> +		cfg |= JZ_LCD_CFG_VSYNC_ACTIVE_LOW;
> +	if (bus_flags & DRM_BUS_FLAG_DE_LOW)
> +		cfg |= JZ_LCD_CFG_DE_ACTIVE_LOW;
> +	if (bus_flags & DRM_BUS_FLAG_PIXDATA_NEGEDGE)
> +		cfg |= JZ_LCD_CFG_PCLK_FALLING_EDGE;
> +
> +	if (ingenic_drm_lcd_is_special_mode(priv->lcd_mode)) {
> +		// TODO: Is that valid for all special modes?
> +		cfg |= JZ_LCD_CFG_REV_POLARITY;
> +	} else {
> +		cfg |= JZ_LCD_CFG_PS_DISABLE
> +		    | JZ_LCD_CFG_CLS_DISABLE
> +		    | JZ_LCD_CFG_SPL_DISABLE
> +		    | JZ_LCD_CFG_REV_DISABLE;
> +	}
> +
> +	regmap_write(priv->map, JZ_REG_LCD_CFG, cfg);
> +}
> +
> +static void ingenic_drm_crtc_update_ctrl(struct ingenic_drm *priv,
> +					 unsigned int bpp)
> +{
> +	unsigned int ctrl = JZ_LCD_CTRL_OFUP | JZ_LCD_CTRL_BURST_16;
> +
> +	switch (bpp) {
> +	case 1:
> +		ctrl |= JZ_LCD_CTRL_BPP_1;
> +		break;
> +	case 2:
> +		ctrl |= JZ_LCD_CTRL_BPP_2;
> +		break;
> +	case 4:
> +		ctrl |= JZ_LCD_CTRL_BPP_4;
> +		break;
> +	case 8:
> +		ctrl |= JZ_LCD_CTRL_BPP_8;
> +	break;
> +	case 15:
> +		ctrl |= JZ_LCD_CTRL_RGB555; /* Falltrough */
> +	case 16:
> +		ctrl |= JZ_LCD_CTRL_BPP_15_16;
> +		break;
> +	case 18:
> +	case 24:
> +	case 32:
> +		ctrl |= JZ_LCD_CTRL_BPP_18_24;
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	regmap_update_bits(priv->map, JZ_REG_LCD_CTRL,
> +			   JZ_LCD_CTRL_OFUP | JZ_LCD_CTRL_BURST_16 |
> +			   JZ_LCD_CTRL_BPP_MASK, ctrl);
> +}
> +
> +static int ingenic_drm_crtc_atomic_check(struct drm_crtc *crtc,
> +					 struct drm_crtc_state *state)
> +{
> +	struct ingenic_drm *priv = drm_crtc_get_priv(crtc);
> +	long rate;
> +
> +	if (!drm_atomic_crtc_needs_modeset(state))
> +		return 0;
> +
> +	rate = clk_round_rate(priv->pix_clk,
> +			      state->adjusted_mode.clock * 1000);
> +	if (rate < 0)
> +		return rate;
> +
> +	return 0;
> +}
> +
> +static void ingenic_drm_crtc_atomic_flush(struct drm_crtc *crtc,
> +					  struct drm_crtc_state *oldstate)
> +{
> +	struct ingenic_drm *priv = drm_crtc_get_priv(crtc);
> +	struct drm_crtc_state *state = crtc->state;
> +	struct drm_pending_vblank_event *event = state->event;
> +	struct drm_framebuffer *drm_fb = crtc->primary->state->fb;
> +	const struct drm_format_info *finfo;
> +	unsigned int width, height;
> +
> +	if (drm_atomic_crtc_needs_modeset(state)) {
> +		finfo = drm_format_info(drm_fb->format->format);
> +		width = state->adjusted_mode.hdisplay;
> +		height = state->adjusted_mode.vdisplay;
> +
> +		ingenic_drm_crtc_update_timings(priv, &state->mode);
> +
> +		ingenic_drm_crtc_update_ctrl(priv, finfo->depth);
> +		ingenic_drm_crtc_update_cfg(priv, &state->adjusted_mode);
> +
> +		clk_set_rate(priv->pix_clk, state->adjusted_mode.clock * 1000);
> +
> +		regmap_write(priv->map, JZ_REG_LCD_DA0, priv->framedesc->next);
> +	}
> +
> +	if (event) {
> +		state->event = NULL;
> +
> +		spin_lock_irq(&crtc->dev->event_lock);
> +		if (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 void ingenic_drm_plane_atomic_update(struct drm_plane *plane,
> +					    struct drm_plane_state *oldstate)
> +{
> +	struct ingenic_drm *priv = drm_plane_get_priv(plane);
> +	struct drm_plane_state *state = plane->state;
> +	const struct drm_format_info *finfo;
> +	unsigned int width, height;
> +
> +	finfo = drm_format_info(state->fb->format->format);
> +	width = state->crtc->state->adjusted_mode.hdisplay;
> +	height = state->crtc->state->adjusted_mode.vdisplay;
> +
> +	priv->framedesc->addr = drm_fb_cma_get_gem_addr(state->fb, state, 0);
> +
> +	priv->framedesc->cmd = width * height * ((finfo->depth + 7) / 8) / 4;
> +	priv->framedesc->cmd |= JZ_LCD_CMD_EOF_IRQ;
> +}
> +
> +static enum drm_connector_status
> +ingenic_drm_conn_detect(struct drm_connector *conn, bool force)
> +{
> +	struct ingenic_drm *priv = drm_conn_get_priv(conn);
> +
> +	if (priv->panel) {
> +		drm_panel_attach(priv->panel, conn);
> +
> +		return connector_status_connected;
> +	}
> +
> +	return connector_status_disconnected;
> +}
> +
> +static void ingenic_drm_conn_destroy(struct drm_connector *conn)
> +{
> +	struct ingenic_drm *priv = drm_conn_get_priv(conn);
> +
> +	if (priv->panel)
> +		drm_panel_detach(priv->panel);
> +
> +	drm_connector_cleanup(conn);
> +}
> +
> +static int ingenic_drm_conn_get_modes(struct drm_connector *conn)
> +{
> +	struct ingenic_drm *priv = drm_conn_get_priv(conn);
> +
> +	if (priv->panel)
> +		return priv->panel->funcs->get_modes(priv->panel);
> +
> +	return 0;
> +}
> +
> +static irqreturn_t ingenic_drm_irq_handler(int irq, void *arg)
> +{
> +	struct drm_device *drm = arg;
> +	struct ingenic_drm *priv = drm->dev_private;
> +	unsigned int state;
> +
> +	regmap_read(priv->map, JZ_REG_LCD_STATE, &state);
> +
> +	regmap_update_bits(priv->map, JZ_REG_LCD_STATE,
> +			   JZ_LCD_STATE_EOF_IRQ, 0);
> +
> +	if (state & JZ_LCD_STATE_EOF_IRQ)
> +		drm_crtc_handle_vblank(&priv->crtc);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int ingenic_drm_enable_vblank(struct drm_crtc *crtc)
> +{
> +	struct ingenic_drm *priv = drm_crtc_get_priv(crtc);
> +
> +	regmap_update_bits(priv->map, JZ_REG_LCD_CTRL,
> +			   JZ_LCD_CTRL_EOF_IRQ, JZ_LCD_CTRL_EOF_IRQ);
> +
> +	return 0;
> +}
> +
> +static void ingenic_drm_disable_vblank(struct drm_crtc *crtc)
> +{
> +	struct ingenic_drm *priv = drm_crtc_get_priv(crtc);
> +
> +	regmap_update_bits(priv->map, JZ_REG_LCD_CTRL, JZ_LCD_CTRL_EOF_IRQ, 0);
> +}
> +
> +DEFINE_DRM_GEM_CMA_FOPS(ingenic_drm_fops);
> +
> +static struct drm_driver ingenic_drm_driver_data = {
> +	.driver_features	= DRIVER_MODESET | DRIVER_GEM | DRIVER_PRIME
> +				| DRIVER_ATOMIC | DRIVER_HAVE_IRQ,
> +	.name			= "ingenic-drm",
> +	.desc			= "DRM module for Ingenic SoCs",
> +	.date			= "20190228",
> +	.major			= 1,
> +	.minor			= 0,
> +	.patchlevel		= 0,
> +
> +	.fops			= &ingenic_drm_fops,
> +
> +	.dumb_create		= drm_gem_cma_dumb_create,
> +	.gem_free_object_unlocked = drm_gem_cma_free_object,
> +	.gem_vm_ops		= &drm_gem_cma_vm_ops,
> +
> +	.prime_handle_to_fd	= drm_gem_prime_handle_to_fd,
> +	.prime_fd_to_handle	= drm_gem_prime_fd_to_handle,
> +	.gem_prime_import	= drm_gem_prime_import,
> +	.gem_prime_export	= drm_gem_prime_export,
> +	.gem_prime_get_sg_table	= drm_gem_cma_prime_get_sg_table,
> +	.gem_prime_import_sg_table = drm_gem_cma_prime_import_sg_table,
> +	.gem_prime_vmap		= drm_gem_cma_prime_vmap,
> +	.gem_prime_vunmap	= drm_gem_cma_prime_vunmap,
> +	.gem_prime_mmap		= drm_gem_cma_prime_mmap,
> +
> +	.irq_handler		= ingenic_drm_irq_handler,
> +};
> +
> +static const struct drm_plane_funcs ingenic_drm_primary_plane_funcs = {
> +	.update_plane		= drm_atomic_helper_update_plane,
> +	.disable_plane		= drm_atomic_helper_disable_plane,
> +	.reset			= drm_atomic_helper_plane_reset,
> +
> +	.atomic_duplicate_state	= drm_atomic_helper_plane_duplicate_state,
> +	.atomic_destroy_state	= drm_atomic_helper_plane_destroy_state,
> +};
> +
> +static const struct drm_crtc_funcs ingenic_drm_crtc_funcs = {
> +	.reset			= drm_atomic_helper_crtc_reset,
> +	.set_config		= drm_atomic_helper_set_config,
> +	.page_flip		= drm_atomic_helper_page_flip,
> +
> +	.atomic_duplicate_state	= drm_atomic_helper_crtc_duplicate_state,
> +	.atomic_destroy_state	= drm_atomic_helper_crtc_destroy_state,
> +
> +	.enable_vblank		= ingenic_drm_enable_vblank,
> +	.disable_vblank		= ingenic_drm_disable_vblank,
> +
> +	.gamma_set		= drm_atomic_helper_legacy_gamma_set,
> +};
> +
> +static const struct drm_connector_funcs ingenic_drm_conn_funcs = {
> +	.fill_modes		= drm_helper_probe_single_connector_modes,
> +	.detect			= ingenic_drm_conn_detect,
> +	.destroy		= ingenic_drm_conn_destroy,
> +	.reset			= drm_atomic_helper_connector_reset,
> +	.atomic_duplicate_state	= drm_atomic_helper_connector_duplicate_state,
> +	.atomic_destroy_state	= drm_atomic_helper_connector_destroy_state,
> +};
> +
> +static const struct drm_plane_helper_funcs ingenic_drm_plane_helper_funcs = {
> +	.atomic_update		= ingenic_drm_plane_atomic_update,
> +	.prepare_fb		= drm_gem_fb_prepare_fb,
> +};
> +
> +static const struct drm_crtc_helper_funcs ingenic_drm_crtc_helper_funcs = {
> +	.atomic_enable		= ingenic_drm_crtc_atomic_enable,
> +	.atomic_disable		= ingenic_drm_crtc_atomic_disable,
> +	.atomic_flush		= ingenic_drm_crtc_atomic_flush,
> +	.atomic_check		= ingenic_drm_crtc_atomic_check,
> +};
> +
> +static const struct drm_connector_helper_funcs ingenic_drm_conn_helper_funcs = {
> +	.get_modes		= ingenic_drm_conn_get_modes,
> +};
> +
> +static const struct drm_mode_config_funcs ingenic_drm_mode_config_funcs = {
> +	.fb_create		= drm_gem_fb_create,
> +	.output_poll_changed	= drm_fb_helper_output_poll_changed,
> +	.atomic_check		= drm_atomic_helper_check,
> +	.atomic_commit		= drm_atomic_helper_commit,
> +};
> +
> +static const struct drm_encoder_funcs ingenic_drm_encoder_funcs = {
> +	.destroy		= drm_encoder_cleanup,
> +};
> +
> +static int ingenic_drm_probe(struct platform_device *pdev)
> +{
> +	const struct jz_soc_info *soc_info;
> +	struct device *dev = &pdev->dev;
> +	struct ingenic_drm *priv;
> +	struct clk *parent_clk;
> +	struct drm_device *drm;
> +	struct resource *mem;
> +	void __iomem *base;
> +	long parent_rate;
> +	int ret, irq;
> +
> +	soc_info = device_get_match_data(dev);
> +	if (!soc_info)
> +		return -EINVAL;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->dev = dev;
> +
> +	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	priv->base = base = devm_ioremap_resource(dev, mem);
> +	if (IS_ERR(base))
> +		return PTR_ERR(base);
> +
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq < 0) {
> +		dev_err(dev, "Failed to get platform irq\n");
> +		return -ENOENT;
> +	}
> +
> +	priv->map = devm_regmap_init_mmio(dev, base,
> +					  &ingenic_drm_regmap_config);
> +	if (IS_ERR(priv->map)) {
> +		dev_err(dev, "Failed to create regmap\n");
> +		return PTR_ERR(priv->map);
> +	}
> +
> +	if (soc_info->needs_dev_clk) {
> +		priv->lcd_clk = devm_clk_get(dev, "lcd");
> +		if (IS_ERR(priv->lcd_clk)) {
> +			dev_err(dev, "Failed to get lcd clock\n");
> +			return PTR_ERR(priv->lcd_clk);
> +		}
> +	}
> +
> +	priv->pix_clk = devm_clk_get(dev, "lcd_pclk");
> +	if (IS_ERR(priv->pix_clk)) {
> +		dev_err(dev, "Failed to get pixel clock\n");
> +		return PTR_ERR(priv->pix_clk);
> +	}
> +
> +	priv->panel_node = of_parse_phandle(dev->of_node, "ingenic,panel", 0);
> +	if (!priv->panel_node) {
> +		DRM_INFO("No panel found");
> +	} else {
> +		priv->panel = of_drm_find_panel(priv->panel_node);
> +		of_node_put(priv->panel_node);
> +
> +		if (IS_ERR(priv->panel)) {
> +			if (PTR_ERR(priv->panel) == -EPROBE_DEFER)
> +				return -EPROBE_DEFER;
> +
> +			priv->panel = NULL;
> +		} else {
> +			ret = drm_panel_prepare(priv->panel);
> +			if (ret && ret != -ENOSYS)
> +				return ret;
> +		}
> +	}
> +
> +	if (priv->panel) {
> +		ret = device_property_read_u32(dev, "ingenic,lcd-mode",
> +					       &priv->lcd_mode);
> +		if (ret) {
> +			dev_err(dev, "Unable to read ingenic,lcd-mode property\n");
> +			return ret;
> +		}
> +	}
> +
> +	priv->framedesc = dma_alloc_coherent(dev, sizeof(*priv->framedesc),
> +					     &priv->framedesc_phys, GFP_KERNEL);
> +	if (!priv->framedesc)
> +		return -ENOMEM;
> +
> +	priv->framedesc->next = priv->framedesc_phys;
> +	priv->framedesc->id = 0xdeafbead;
> +
> +	drm = drm_dev_alloc(&ingenic_drm_driver_data, dev);
> +	if (IS_ERR(drm)) {
> +		ret = PTR_ERR(drm);
> +		goto err_free_dma;
> +	}
> +
> +	priv->drm = drm;
> +
> +	drm_mode_config_init(drm);
> +	drm->mode_config.min_width = 0;
> +	drm->mode_config.min_height = 0;
> +	drm->mode_config.max_width = 800;
> +	drm->mode_config.max_height = 600;
> +	drm->mode_config.funcs = &ingenic_drm_mode_config_funcs;
> +
> +	drm_plane_helper_add(&priv->primary, &ingenic_drm_plane_helper_funcs);
> +
> +	ret = drm_universal_plane_init(drm, &priv->primary,
> +				       0, &ingenic_drm_primary_plane_funcs,
> +				       ingenic_drm_primary_formats,
> +				       ARRAY_SIZE(ingenic_drm_primary_formats),
> +				       NULL, DRM_PLANE_TYPE_PRIMARY, NULL);
> +	if (ret) {
> +		dev_err(dev, "Failed to register primary plane: %i\n", ret);
> +		goto err_unref_drm;
> +	}
> +
> +	drm_crtc_helper_add(&priv->crtc, &ingenic_drm_crtc_helper_funcs);
> +
> +	ret = drm_crtc_init_with_planes(drm, &priv->crtc, &priv->primary,
> +					NULL, &ingenic_drm_crtc_funcs, NULL);
> +	if (ret) {
> +		dev_err(dev, "Failed to init CRTC: %i\n", ret);
> +		goto err_cleanup_plane;
> +	}
> +
> +	drm_connector_helper_add(&priv->connector,
> +				 &ingenic_drm_conn_helper_funcs);
> +	ret = drm_connector_init(drm, &priv->connector,
> +				 &ingenic_drm_conn_funcs,
> +				 DRM_MODE_CONNECTOR_Unknown);
> +	if (ret) {
> +		dev_err(dev, "Failed to init connector: %i\n", ret);
> +		goto err_cleanup_crtc;
> +	}
> +
> +	priv->encoder.possible_crtcs = 1;
> +
> +	ret = drm_encoder_init(drm, &priv->encoder, &ingenic_drm_encoder_funcs,
> +			       DRM_MODE_ENCODER_NONE, NULL);
> +	if (ret) {
> +		dev_err(dev, "Failed to init encoder: %i\n", ret);
> +		goto err_cleanup_connector;
> +	}
> +
> +	drm_connector_attach_encoder(&priv->connector, &priv->encoder);
> +
> +	platform_set_drvdata(pdev, drm);
> +	priv->drm = drm;
> +	drm->dev_private = priv;
> +
> +	ret = drm_irq_install(drm, irq);
> +	if (ret) {
> +		dev_err(dev, "Unable to install IRQ handler\n");
> +		goto err_cleanup_encoder;
> +	}
> +
> +	ret = drm_vblank_init(drm, 1);
> +	if (ret) {
> +		dev_err(dev, "Failed calling drm_vblank_init()\n");
> +		goto err_uninstall_irq;
> +	}
> +
> +	drm_mode_config_reset(drm);
> +
> +	ret = clk_prepare_enable(priv->pix_clk);
> +	if (ret) {
> +		dev_err(dev, "Unable to start pixel clock\n");
> +		goto err_cleanup_vblank;
> +	}
> +
> +	if (priv->lcd_clk) {
> +		parent_clk = clk_get_parent(priv->lcd_clk);
> +		parent_rate = clk_get_rate(parent_clk);
> +
> +		/* LCD Device clock must be 3x the pixel clock for STN panels,
> +		 * or 1.5x the pixel clock for TFT panels. To avoid having to
> +		 * check for the LCD device clock everytime we do a mode change,
> +		 * we set the LCD device clock to the highest rate possible.
> +		 */
> +		ret = clk_set_rate(priv->lcd_clk, parent_rate);
> +		if (ret) {
> +			dev_err(dev, "Unable to set LCD clock rate\n");
> +			goto err_pixclk_disable;
> +		}
> +
> +		ret = clk_prepare_enable(priv->lcd_clk);
> +		if (ret) {
> +			dev_err(dev, "Unable to start lcd clock\n");
> +			goto err_pixclk_disable;
> +		}
> +	}
> +
> +	ret = drm_fbdev_generic_setup(drm, 16);
> +	if (ret) {
> +		dev_err(dev, "Failed to init fbdev\n");
> +		goto err_devclk_disable;
> +	}
> +
> +	ret = drm_dev_register(drm, 0);
> +	if (ret) {
> +		dev_err(dev, "Failed to register DRM driver\n");
> +		goto err_cleanup_fbdev;
> +	}
> +
> +	return 0;
> +
> +err_cleanup_fbdev:
> +	drm_mode_config_cleanup(drm);
> +err_devclk_disable:
> +	if (priv->lcd_clk)
> +		clk_disable_unprepare(priv->lcd_clk);
> +err_pixclk_disable:
> +	clk_disable_unprepare(priv->pix_clk);
> +err_cleanup_vblank:
> +	drm_vblank_cleanup(drm);
> +err_uninstall_irq:
> +	drm_irq_uninstall(drm);
> +err_cleanup_encoder:
> +	drm_encoder_cleanup(&priv->encoder);
> +err_cleanup_connector:
> +	drm_connector_cleanup(&priv->connector);
> +err_cleanup_crtc:
> +	drm_crtc_cleanup(&priv->crtc);
> +err_cleanup_plane:
> +	drm_plane_cleanup(&priv->primary);
> +err_unref_drm:
> +	drm_dev_put(drm);
> +err_free_dma:
> +	dma_free_coherent(dev, sizeof(*priv->framedesc),
> +			  priv->framedesc, priv->framedesc_phys);
> +	return ret;
> +}
> +
> +static int ingenic_drm_remove(struct platform_device *pdev)
> +{
> +	struct drm_device *drm = platform_get_drvdata(pdev);
> +	struct ingenic_drm *priv = drm->dev_private;
> +
> +	drm_dev_unregister(drm);
> +	drm_mode_config_cleanup(drm);
> +
> +	if (priv->lcd_clk)
> +		clk_disable_unprepare(priv->lcd_clk);
> +	clk_disable_unprepare(priv->pix_clk);
> +
> +	drm_vblank_cleanup(drm);
> +	drm_irq_uninstall(drm);
> +
> +	drm_encoder_cleanup(&priv->encoder);
> +	drm_connector_cleanup(&priv->connector);
> +	drm_crtc_cleanup(&priv->crtc);
> +	drm_plane_cleanup(&priv->primary);
> +
> +	drm_dev_put(drm);
> +
> +	dma_free_coherent(&pdev->dev, sizeof(*priv->framedesc),
> +			  priv->framedesc, priv->framedesc_phys);
> +
> +	return 0;
> +}
> +
> +static const struct jz_soc_info jz4740_soc_info = {
> +	.needs_dev_clk = true,
> +};
> +
> +static const struct jz_soc_info jz4725b_soc_info = {
> +	.needs_dev_clk = false,
> +};
> +
> +static const struct of_device_id ingenic_drm_of_match[] = {
> +	{ .compatible = "ingenic,jz4740-drm", .data = &jz4740_soc_info },
> +	{ .compatible = "ingenic,jz4725b-drm", .data = &jz4725b_soc_info },
> +	{},
> +};
> +
> +static struct platform_driver ingenic_drm_driver = {
> +	.driver = {
> +		.name = "ingenic-drm",
> +		.of_match_table = of_match_ptr(ingenic_drm_of_match),
> +	},
> +	.probe = ingenic_drm_probe,
> +	.remove = ingenic_drm_remove,
> +};
> +module_platform_driver(ingenic_drm_driver);
> +
> +MODULE_AUTHOR("Paul Cercueil <paul@crapouillou.net>");
> +MODULE_DESCRIPTION("DRM driver for the Ingenic SoCs\n");
> +MODULE_LICENSE("GPL v2");
> -- 
> 2.11.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 3/3] DRM: Add KMS driver for the Ingenic JZ47xx SoCs

[Paul Cercueil]

Hi Daniel,

Le ven. 1 mars 2019 à 5:26, Daniel Vetter <daniel@ffwll.ch> a écrit :
> On Thu, Feb 28, 2019 at 07:07:56PM -0300, Paul Cercueil wrote:
>>  Add a KMS driver for the Ingenic JZ47xx family of SoCs.
>>  This driver is meant to replace the aging jz4740-fb driver.
>> 
>>  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>>  Tested-by: Artur Rojek <contact@artur-rojek.eu>
> 
> Yay, more fbdev drivers moving to atmic!
> 
> Going on vacations for a week now, so just a few quick high-level 
> comments
> below. Those should make the driver even smaller and cleaner.

Alright, no problem, there's no rush.

>>  ---
>>   drivers/gpu/drm/Kconfig               |   2 +
>>   drivers/gpu/drm/Makefile              |   1 +
>>   drivers/gpu/drm/ingenic/Kconfig       |  16 +
>>   drivers/gpu/drm/ingenic/Makefile      |   1 +
>>   drivers/gpu/drm/ingenic/ingenic-drm.c | 896 
>> ++++++++++++++++++++++++++++++++++
>>   5 files changed, 916 insertions(+)
>>   create mode 100644 drivers/gpu/drm/ingenic/Kconfig
>>   create mode 100644 drivers/gpu/drm/ingenic/Makefile
>>   create mode 100644 drivers/gpu/drm/ingenic/ingenic-drm.c
>> 
>>  diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
>>  index bd943a71756c..f1929112e559 100644
>>  --- a/drivers/gpu/drm/Kconfig
>>  +++ b/drivers/gpu/drm/Kconfig
>>  @@ -303,6 +303,8 @@ source "drivers/gpu/drm/sti/Kconfig"
>> 
>>   source "drivers/gpu/drm/imx/Kconfig"
>> 
>>  +source "drivers/gpu/drm/ingenic/Kconfig"
>>  +
>>   source "drivers/gpu/drm/v3d/Kconfig"
>> 
>>   source "drivers/gpu/drm/vc4/Kconfig"
>>  diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
>>  index 1ac55c65eac0..9666c0767f44 100644
>>  --- a/drivers/gpu/drm/Makefile
>>  +++ b/drivers/gpu/drm/Makefile
>>  @@ -94,6 +94,7 @@ obj-$(CONFIG_DRM_TEGRA) += tegra/
>>   obj-$(CONFIG_DRM_STM) += stm/
>>   obj-$(CONFIG_DRM_STI) += sti/
>>   obj-$(CONFIG_DRM_IMX) += imx/
>>  +obj-y			+= ingenic/
>>   obj-$(CONFIG_DRM_MEDIATEK) += mediatek/
>>   obj-$(CONFIG_DRM_MESON)	+= meson/
>>   obj-y			+= i2c/
>>  diff --git a/drivers/gpu/drm/ingenic/Kconfig 
>> b/drivers/gpu/drm/ingenic/Kconfig
>>  new file mode 100644
>>  index 000000000000..d82c3d37ec9c
>>  --- /dev/null
>>  +++ b/drivers/gpu/drm/ingenic/Kconfig
>>  @@ -0,0 +1,16 @@
>>  +config DRM_INGENIC
>>  +	tristate "DRM Support for Ingenic SoCs"
>>  +	depends on MIPS || COMPILE_TEST
>>  +	depends on DRM
>>  +	depends on CMA
>>  +	depends on OF
>>  +	select DRM_BRIDGE
>>  +	select DRM_PANEL_BRIDGE
>>  +	select DRM_KMS_HELPER
>>  +	select DRM_KMS_CMA_HELPER
>>  +	select DRM_GEM_CMA_HELPER
>>  +	select VT_HW_CONSOLE_BINDING if FRAMEBUFFER_CONSOLE
>>  +	help
>>  +	  Choose this option for DRM support for the Ingenic SoCs.
>>  +
>>  +	  If M is selected the module will be called ingenic-drm.
>>  diff --git a/drivers/gpu/drm/ingenic/Makefile 
>> b/drivers/gpu/drm/ingenic/Makefile
>>  new file mode 100644
>>  index 000000000000..11cac42ce0bb
>>  --- /dev/null
>>  +++ b/drivers/gpu/drm/ingenic/Makefile
>>  @@ -0,0 +1 @@
>>  +obj-$(CONFIG_DRM_INGENIC) += ingenic-drm.o
>>  diff --git a/drivers/gpu/drm/ingenic/ingenic-drm.c 
>> b/drivers/gpu/drm/ingenic/ingenic-drm.c
>>  new file mode 100644
>>  index 000000000000..18120deea96f
>>  --- /dev/null
>>  +++ b/drivers/gpu/drm/ingenic/ingenic-drm.c
>>  @@ -0,0 +1,896 @@
>>  +// SPDX-License-Identifier: GPL-2.0
>>  +//
>>  +// Ingenic JZ47xx KMS driver
>>  +//
>>  +// Copyright (C) 2019, Paul Cercueil <paul@crapouillou.net>
>>  +
>>  +#include <linux/clk.h>
>>  +#include <linux/dma-mapping.h>
>>  +#include <linux/module.h>
>>  +#include <linux/platform_device.h>
>>  +#include <linux/regmap.h>
>>  +
>>  +#include <drm/drm_atomic.h>
>>  +#include <drm/drm_atomic_helper.h>
>>  +#include <drm/drm_crtc.h>
>>  +#include <drm/drm_crtc_helper.h>
>>  +#include <drm/drm_drv.h>
>>  +#include <drm/drm_gem_cma_helper.h>
>>  +#include <drm/drm_fb_cma_helper.h>
>>  +#include <drm/drm_fb_helper.h>
>>  +#include <drm/drm_fourcc.h>
>>  +#include <drm/drm_gem_framebuffer_helper.h>
>>  +#include <drm/drm_irq.h>
>>  +#include <drm/drm_panel.h>
>>  +#include <drm/drm_plane.h>
>>  +#include <drm/drm_plane_helper.h>
>>  +#include <drm/drm_probe_helper.h>
>>  +#include <drm/drm_vblank.h>
>>  +
>>  +#include <dt-bindings/display/ingenic,drm.h>
>>  +
>>  +#include "../drm_internal.h"
>>  +
>>  +#define JZ_REG_LCD_CFG		0x00
>>  +#define JZ_REG_LCD_VSYNC	0x04
>>  +#define JZ_REG_LCD_HSYNC	0x08
>>  +#define JZ_REG_LCD_VAT		0x0C
>>  +#define JZ_REG_LCD_DAH		0x10
>>  +#define JZ_REG_LCD_DAV		0x14
>>  +#define JZ_REG_LCD_PS		0x18
>>  +#define JZ_REG_LCD_CLS		0x1C
>>  +#define JZ_REG_LCD_SPL		0x20
>>  +#define JZ_REG_LCD_REV		0x24
>>  +#define JZ_REG_LCD_CTRL		0x30
>>  +#define JZ_REG_LCD_STATE	0x34
>>  +#define JZ_REG_LCD_IID		0x38
>>  +#define JZ_REG_LCD_DA0		0x40
>>  +#define JZ_REG_LCD_SA0		0x44
>>  +#define JZ_REG_LCD_FID0		0x48
>>  +#define JZ_REG_LCD_CMD0		0x4C
>>  +#define JZ_REG_LCD_DA1		0x50
>>  +#define JZ_REG_LCD_SA1		0x54
>>  +#define JZ_REG_LCD_FID1		0x58
>>  +#define JZ_REG_LCD_CMD1		0x5C
>>  +
>>  +#define JZ_LCD_CFG_SLCD			BIT(31)
>>  +#define JZ_LCD_CFG_PS_DISABLE		BIT(23)
>>  +#define JZ_LCD_CFG_CLS_DISABLE		BIT(22)
>>  +#define JZ_LCD_CFG_SPL_DISABLE		BIT(21)
>>  +#define JZ_LCD_CFG_REV_DISABLE		BIT(20)
>>  +#define JZ_LCD_CFG_HSYNCM		BIT(19)
>>  +#define JZ_LCD_CFG_PCLKM		BIT(18)
>>  +#define JZ_LCD_CFG_INV			BIT(17)
>>  +#define JZ_LCD_CFG_SYNC_DIR		BIT(16)
>>  +#define JZ_LCD_CFG_PS_POLARITY		BIT(15)
>>  +#define JZ_LCD_CFG_CLS_POLARITY		BIT(14)
>>  +#define JZ_LCD_CFG_SPL_POLARITY		BIT(13)
>>  +#define JZ_LCD_CFG_REV_POLARITY		BIT(12)
>>  +#define JZ_LCD_CFG_HSYNC_ACTIVE_LOW	BIT(11)
>>  +#define JZ_LCD_CFG_PCLK_FALLING_EDGE	BIT(10)
>>  +#define JZ_LCD_CFG_DE_ACTIVE_LOW	BIT(9)
>>  +#define JZ_LCD_CFG_VSYNC_ACTIVE_LOW	BIT(8)
>>  +#define JZ_LCD_CFG_18_BIT		BIT(7)
>>  +#define JZ_LCD_CFG_PDW			(BIT(5) | BIT(4))
>>  +#define JZ_LCD_CFG_MODE_MASK		0xf
>>  +
>>  +#define JZ_LCD_VSYNC_VPS_OFFSET		16
>>  +#define JZ_LCD_VSYNC_VPE_OFFSET		0
>>  +
>>  +#define JZ_LCD_HSYNC_HPS_OFFSET		16
>>  +#define JZ_LCD_HSYNC_HPE_OFFSET		0
>>  +
>>  +#define JZ_LCD_VAT_HT_OFFSET		16
>>  +#define JZ_LCD_VAT_VT_OFFSET		0
>>  +
>>  +#define JZ_LCD_DAH_HDS_OFFSET		16
>>  +#define JZ_LCD_DAH_HDE_OFFSET		0
>>  +
>>  +#define JZ_LCD_DAV_VDS_OFFSET		16
>>  +#define JZ_LCD_DAV_VDE_OFFSET		0
>>  +
>>  +#define JZ_LCD_CTRL_BURST_4		(0x0 << 28)
>>  +#define JZ_LCD_CTRL_BURST_8		(0x1 << 28)
>>  +#define JZ_LCD_CTRL_BURST_16		(0x2 << 28)
>>  +#define JZ_LCD_CTRL_RGB555		BIT(27)
>>  +#define JZ_LCD_CTRL_OFUP		BIT(26)
>>  +#define JZ_LCD_CTRL_FRC_GRAYSCALE_16	(0x0 << 24)
>>  +#define JZ_LCD_CTRL_FRC_GRAYSCALE_4	(0x1 << 24)
>>  +#define JZ_LCD_CTRL_FRC_GRAYSCALE_2	(0x2 << 24)
>>  +#define JZ_LCD_CTRL_PDD_MASK		(0xff << 16)
>>  +#define JZ_LCD_CTRL_EOF_IRQ		BIT(13)
>>  +#define JZ_LCD_CTRL_SOF_IRQ		BIT(12)
>>  +#define JZ_LCD_CTRL_OFU_IRQ		BIT(11)
>>  +#define JZ_LCD_CTRL_IFU0_IRQ		BIT(10)
>>  +#define JZ_LCD_CTRL_IFU1_IRQ		BIT(9)
>>  +#define JZ_LCD_CTRL_DD_IRQ		BIT(8)
>>  +#define JZ_LCD_CTRL_QDD_IRQ		BIT(7)
>>  +#define JZ_LCD_CTRL_REVERSE_ENDIAN	BIT(6)
>>  +#define JZ_LCD_CTRL_LSB_FISRT		BIT(5)
>>  +#define JZ_LCD_CTRL_DISABLE		BIT(4)
>>  +#define JZ_LCD_CTRL_ENABLE		BIT(3)
>>  +#define JZ_LCD_CTRL_BPP_1		0x0
>>  +#define JZ_LCD_CTRL_BPP_2		0x1
>>  +#define JZ_LCD_CTRL_BPP_4		0x2
>>  +#define JZ_LCD_CTRL_BPP_8		0x3
>>  +#define JZ_LCD_CTRL_BPP_15_16		0x4
>>  +#define JZ_LCD_CTRL_BPP_18_24		0x5
>>  +#define JZ_LCD_CTRL_BPP_MASK		(JZ_LCD_CTRL_RGB555 | (0x7 << 0))
>>  +
>>  +#define JZ_LCD_CMD_SOF_IRQ		BIT(31)
>>  +#define JZ_LCD_CMD_EOF_IRQ		BIT(30)
>>  +#define JZ_LCD_CMD_ENABLE_PAL		BIT(28)
>>  +
>>  +#define JZ_LCD_SYNC_MASK		0x3ff
>>  +
>>  +#define JZ_LCD_STATE_EOF_IRQ		BIT(5)
>>  +#define JZ_LCD_STATE_SOF_IRQ		BIT(4)
>>  +#define JZ_LCD_STATE_DISABLED		BIT(0)
>>  +
>>  +struct ingenic_framedesc {
>>  +	uint32_t next;
>>  +	uint32_t addr;
>>  +	uint32_t id;
>>  +	uint32_t cmd;
>>  +} __packed;
>>  +
>>  +struct jz_soc_info {
>>  +	bool needs_dev_clk;
>>  +};
>>  +
>>  +struct ingenic_drm {
>>  +	struct device *dev;
> 
> Embedding is kinda the new thing, since it also prevents people from 
> using
> devm_ for this, which is (strictly speaking, not that it matters 
> much) the
> wrong lifetime for drm allocations. There's some patches from Noralf 
> that
> should land soon to make this a bit more convenient.

Sorry I didn't quite understand what you wrote here. What do you mean by
"embedding"?

>>  +	void __iomem *base;
>>  +	struct regmap *map;
>>  +	struct clk *lcd_clk, *pix_clk;
>>  +
>>  +	u32 lcd_mode;
>>  +
>>  +	struct ingenic_framedesc *framedesc;
>>  +	dma_addr_t framedesc_phys;
>>  +
>>  +	struct drm_device *drm;
>>  +	struct drm_plane primary;
>>  +	struct drm_crtc crtc;
>>  +	struct drm_connector connector;
>>  +	struct drm_encoder encoder;
> 
> Any reason you didn't opt for the drm_simple_display_pipe helpers? 
> Since
> you have just 1:1:1 between crtc/plane/encoder that will allow you to
> ditch lots of boilerplate.
> 
> https://dri.freedesktop.org/docs/drm/gpu/drm-kms-helpers.html#simple-kms-helper-reference

Yes, the reason is that I will then add support for newer SoCs that can
deal with overlay planes, have an integrated IPU for scaling and 
colorspace
conversion, and have different possible outputs (LCD panel, TV-out, 
HDMI).

>>  +	struct drm_panel *panel;
>>  +
>>  +	struct device_node *panel_node;
> 
> There's panel bridge helpers which takes a panel and wraps it into a
> drm_bridge (which you can glue into the simple pipe helpers), giving 
> you
> the connector for free.
> 
> https://dri.freedesktop.org/docs/drm/gpu/drm-kms-helpers.html#panel-bridge-helper-reference

I will look into it, thanks.

> This should cover all the bigger items, details look already really 
> tidy.
> 
> Cheers, Daniel
> 
>>  +
>>  +	unsigned short ps_start, ps_end, cls_start,
>>  +		       cls_end, spl_start, spl_end, rev_start;
>>  +};
>>  +
>>  +static const uint32_t ingenic_drm_primary_formats[] = {
>>  +	DRM_FORMAT_XRGB1555,
>>  +	DRM_FORMAT_RGB565,
>>  +	DRM_FORMAT_XRGB8888,
>>  +};
>>  +
>>  +static bool ingenic_drm_writeable_reg(struct device *dev, unsigned 
>> int reg)
>>  +{
>>  +	switch (reg) {
>>  +	case JZ_REG_LCD_IID:
>>  +	case JZ_REG_LCD_SA0:
>>  +	case JZ_REG_LCD_FID0:
>>  +	case JZ_REG_LCD_CMD0:
>>  +	case JZ_REG_LCD_SA1:
>>  +	case JZ_REG_LCD_FID1:
>>  +	case JZ_REG_LCD_CMD1:
>>  +		return false;
>>  +	default:
>>  +		return true;
>>  +	}
>>  +}
>>  +
>>  +static const struct regmap_config ingenic_drm_regmap_config = {
>>  +	.reg_bits = 32,
>>  +	.val_bits = 32,
>>  +	.reg_stride = 4,
>>  +
>>  +	.max_register = JZ_REG_LCD_CMD1,
>>  +	.writeable_reg = ingenic_drm_writeable_reg,
>>  +};
>>  +
>>  +static inline struct ingenic_drm *drm_crtc_get_priv(struct 
>> drm_crtc *crtc)
>>  +{
>>  +	return container_of(crtc, struct ingenic_drm, crtc);
>>  +}
>>  +
>>  +static inline struct ingenic_drm *drm_plane_get_priv(struct 
>> drm_plane *plane)
>>  +{
>>  +	return container_of(plane, struct ingenic_drm, primary);
>>  +}
>>  +
>>  +static inline struct ingenic_drm *drm_conn_get_priv(struct 
>> drm_connector *conn)
>>  +{
>>  +	return container_of(conn, struct ingenic_drm, connector);
>>  +}
>>  +
>>  +static void ingenic_drm_crtc_atomic_enable(struct drm_crtc *crtc,
>>  +					  struct drm_crtc_state *state)
>>  +{
>>  +	struct ingenic_drm *priv = drm_crtc_get_priv(crtc);
>>  +
>>  +	regmap_write(priv->map, JZ_REG_LCD_STATE, 0);
>>  +
>>  +	regmap_update_bits(priv->map, JZ_REG_LCD_CTRL,
>>  +			   JZ_LCD_CTRL_ENABLE | JZ_LCD_CTRL_DISABLE,
>>  +			   JZ_LCD_CTRL_ENABLE);
>>  +
>>  +	drm_panel_enable(priv->panel);
>>  +
>>  +	drm_crtc_vblank_on(crtc);
>>  +}
>>  +
>>  +static void ingenic_drm_crtc_atomic_disable(struct drm_crtc *crtc,
>>  +					   struct drm_crtc_state *state)
>>  +{
>>  +	struct ingenic_drm *priv = drm_crtc_get_priv(crtc);
>>  +	unsigned int var;
>>  +
>>  +	drm_crtc_vblank_off(crtc);
>>  +
>>  +	drm_panel_disable(priv->panel);
>>  +
>>  +	regmap_update_bits(priv->map, JZ_REG_LCD_CTRL,
>>  +			   JZ_LCD_CTRL_DISABLE, JZ_LCD_CTRL_DISABLE);
>>  +
>>  +	regmap_read_poll_timeout(priv->map, JZ_REG_LCD_STATE, var,
>>  +				 var & JZ_LCD_STATE_DISABLED,
>>  +				 1000, 0);
>>  +}
>>  +
>>  +static inline bool ingenic_drm_lcd_is_special_mode(u32 mode)
>>  +{
>>  +	switch (mode) {
>>  +	case JZ_DRM_LCD_SPECIAL_TFT_1:
>>  +	case JZ_DRM_LCD_SPECIAL_TFT_2:
>>  +	case JZ_DRM_LCD_SPECIAL_TFT_3:
>>  +		return true;
>>  +	default:
>>  +		return false;
>>  +	}
>>  +}
>>  +
>>  +static inline bool ingenic_drm_lcd_is_stn_mode(u32 mode)
>>  +{
>>  +	switch (mode) {
>>  +	case JZ_DRM_LCD_SINGLE_COLOR_STN:
>>  +	case JZ_DRM_LCD_SINGLE_MONOCHROME_STN:
>>  +	case JZ_DRM_LCD_DUAL_COLOR_STN:
>>  +	case JZ_DRM_LCD_DUAL_MONOCHROME_STN:
>>  +		return true;
>>  +	default:
>>  +		return false;
>>  +	}
>>  +}
>>  +
>>  +static void ingenic_drm_crtc_update_timings(struct ingenic_drm 
>> *priv,
>>  +					    struct drm_display_mode *mode)
>>  +{
>>  +	unsigned int vpe, vds, vde, vt, hpe, hds, hde, ht;
>>  +
>>  +	vpe = mode->vsync_end - mode->vsync_start;
>>  +	vds = mode->vtotal - mode->vsync_start;
>>  +	vde = vds + mode->vdisplay;
>>  +	vt = vde + mode->vsync_start - mode->vdisplay;
>>  +
>>  +	hpe = mode->hsync_end - mode->hsync_start;
>>  +	hds = mode->htotal - mode->hsync_start;
>>  +	hde = hds + mode->hdisplay;
>>  +	ht = hde + mode->hsync_start - mode->hdisplay;
>>  +
>>  +	regmap_write(priv->map, JZ_REG_LCD_VSYNC,
>>  +		     0 << JZ_LCD_VSYNC_VPS_OFFSET |
>>  +		     vpe << JZ_LCD_VSYNC_VPE_OFFSET);
>>  +
>>  +	regmap_write(priv->map, JZ_REG_LCD_HSYNC,
>>  +		     0 << JZ_LCD_HSYNC_HPS_OFFSET |
>>  +		     hpe << JZ_LCD_HSYNC_HPE_OFFSET);
>>  +
>>  +	regmap_write(priv->map, JZ_REG_LCD_VAT,
>>  +		     ht << JZ_LCD_VAT_HT_OFFSET |
>>  +		     vt << JZ_LCD_VAT_VT_OFFSET);
>>  +
>>  +	regmap_write(priv->map, JZ_REG_LCD_DAH,
>>  +		     hds << JZ_LCD_DAH_HDS_OFFSET |
>>  +		     hde << JZ_LCD_DAH_HDE_OFFSET);
>>  +	regmap_write(priv->map, JZ_REG_LCD_DAV,
>>  +		     vds << JZ_LCD_DAV_VDS_OFFSET |
>>  +		     vde << JZ_LCD_DAV_VDE_OFFSET);
>>  +
>>  +	if (ingenic_drm_lcd_is_special_mode(priv->lcd_mode)) {
>>  +		regmap_write(priv->map, JZ_REG_LCD_PS, hde << 16 | (hde + 1));
>>  +		regmap_write(priv->map, JZ_REG_LCD_CLS, hde << 16 | (hde + 1));
>>  +		regmap_write(priv->map, JZ_REG_LCD_SPL, hpe << 16 | (hpe + 1));
>>  +		regmap_write(priv->map, JZ_REG_LCD_REV, mode->htotal << 16);
>>  +	}
>>  +}
>>  +
>>  +static void ingenic_drm_crtc_update_cfg(struct ingenic_drm *priv,
>>  +					struct drm_display_mode *mode)
>>  +
>>  +{
>>  +	unsigned int cfg = priv->lcd_mode;
>>  +	u32 bus_flags = priv->connector.display_info.bus_flags;
>>  +
>>  +	if (mode->flags & DRM_MODE_FLAG_NHSYNC)
>>  +		cfg |= JZ_LCD_CFG_HSYNC_ACTIVE_LOW;
>>  +	if (mode->flags & DRM_MODE_FLAG_NVSYNC)
>>  +		cfg |= JZ_LCD_CFG_VSYNC_ACTIVE_LOW;
>>  +	if (bus_flags & DRM_BUS_FLAG_DE_LOW)
>>  +		cfg |= JZ_LCD_CFG_DE_ACTIVE_LOW;
>>  +	if (bus_flags & DRM_BUS_FLAG_PIXDATA_NEGEDGE)
>>  +		cfg |= JZ_LCD_CFG_PCLK_FALLING_EDGE;
>>  +
>>  +	if (ingenic_drm_lcd_is_special_mode(priv->lcd_mode)) {
>>  +		// TODO: Is that valid for all special modes?
>>  +		cfg |= JZ_LCD_CFG_REV_POLARITY;
>>  +	} else {
>>  +		cfg |= JZ_LCD_CFG_PS_DISABLE
>>  +		    | JZ_LCD_CFG_CLS_DISABLE
>>  +		    | JZ_LCD_CFG_SPL_DISABLE
>>  +		    | JZ_LCD_CFG_REV_DISABLE;
>>  +	}
>>  +
>>  +	regmap_write(priv->map, JZ_REG_LCD_CFG, cfg);
>>  +}
>>  +
>>  +static void ingenic_drm_crtc_update_ctrl(struct ingenic_drm *priv,
>>  +					 unsigned int bpp)
>>  +{
>>  +	unsigned int ctrl = JZ_LCD_CTRL_OFUP | JZ_LCD_CTRL_BURST_16;
>>  +
>>  +	switch (bpp) {
>>  +	case 1:
>>  +		ctrl |= JZ_LCD_CTRL_BPP_1;
>>  +		break;
>>  +	case 2:
>>  +		ctrl |= JZ_LCD_CTRL_BPP_2;
>>  +		break;
>>  +	case 4:
>>  +		ctrl |= JZ_LCD_CTRL_BPP_4;
>>  +		break;
>>  +	case 8:
>>  +		ctrl |= JZ_LCD_CTRL_BPP_8;
>>  +	break;
>>  +	case 15:
>>  +		ctrl |= JZ_LCD_CTRL_RGB555; /* Falltrough */
>>  +	case 16:
>>  +		ctrl |= JZ_LCD_CTRL_BPP_15_16;
>>  +		break;
>>  +	case 18:
>>  +	case 24:
>>  +	case 32:
>>  +		ctrl |= JZ_LCD_CTRL_BPP_18_24;
>>  +		break;
>>  +	default:
>>  +		break;
>>  +	}
>>  +
>>  +	regmap_update_bits(priv->map, JZ_REG_LCD_CTRL,
>>  +			   JZ_LCD_CTRL_OFUP | JZ_LCD_CTRL_BURST_16 |
>>  +			   JZ_LCD_CTRL_BPP_MASK, ctrl);
>>  +}
>>  +
>>  +static int ingenic_drm_crtc_atomic_check(struct drm_crtc *crtc,
>>  +					 struct drm_crtc_state *state)
>>  +{
>>  +	struct ingenic_drm *priv = drm_crtc_get_priv(crtc);
>>  +	long rate;
>>  +
>>  +	if (!drm_atomic_crtc_needs_modeset(state))
>>  +		return 0;
>>  +
>>  +	rate = clk_round_rate(priv->pix_clk,
>>  +			      state->adjusted_mode.clock * 1000);
>>  +	if (rate < 0)
>>  +		return rate;
>>  +
>>  +	return 0;
>>  +}
>>  +
>>  +static void ingenic_drm_crtc_atomic_flush(struct drm_crtc *crtc,
>>  +					  struct drm_crtc_state *oldstate)
>>  +{
>>  +	struct ingenic_drm *priv = drm_crtc_get_priv(crtc);
>>  +	struct drm_crtc_state *state = crtc->state;
>>  +	struct drm_pending_vblank_event *event = state->event;
>>  +	struct drm_framebuffer *drm_fb = crtc->primary->state->fb;
>>  +	const struct drm_format_info *finfo;
>>  +	unsigned int width, height;
>>  +
>>  +	if (drm_atomic_crtc_needs_modeset(state)) {
>>  +		finfo = drm_format_info(drm_fb->format->format);
>>  +		width = state->adjusted_mode.hdisplay;
>>  +		height = state->adjusted_mode.vdisplay;
>>  +
>>  +		ingenic_drm_crtc_update_timings(priv, &state->mode);
>>  +
>>  +		ingenic_drm_crtc_update_ctrl(priv, finfo->depth);
>>  +		ingenic_drm_crtc_update_cfg(priv, &state->adjusted_mode);
>>  +
>>  +		clk_set_rate(priv->pix_clk, state->adjusted_mode.clock * 1000);
>>  +
>>  +		regmap_write(priv->map, JZ_REG_LCD_DA0, priv->framedesc->next);
>>  +	}
>>  +
>>  +	if (event) {
>>  +		state->event = NULL;
>>  +
>>  +		spin_lock_irq(&crtc->dev->event_lock);
>>  +		if (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 void ingenic_drm_plane_atomic_update(struct drm_plane 
>> *plane,
>>  +					    struct drm_plane_state *oldstate)
>>  +{
>>  +	struct ingenic_drm *priv = drm_plane_get_priv(plane);
>>  +	struct drm_plane_state *state = plane->state;
>>  +	const struct drm_format_info *finfo;
>>  +	unsigned int width, height;
>>  +
>>  +	finfo = drm_format_info(state->fb->format->format);
>>  +	width = state->crtc->state->adjusted_mode.hdisplay;
>>  +	height = state->crtc->state->adjusted_mode.vdisplay;
>>  +
>>  +	priv->framedesc->addr = drm_fb_cma_get_gem_addr(state->fb, state, 
>> 0);
>>  +
>>  +	priv->framedesc->cmd = width * height * ((finfo->depth + 7) / 8) 
>> / 4;
>>  +	priv->framedesc->cmd |= JZ_LCD_CMD_EOF_IRQ;
>>  +}
>>  +
>>  +static enum drm_connector_status
>>  +ingenic_drm_conn_detect(struct drm_connector *conn, bool force)
>>  +{
>>  +	struct ingenic_drm *priv = drm_conn_get_priv(conn);
>>  +
>>  +	if (priv->panel) {
>>  +		drm_panel_attach(priv->panel, conn);
>>  +
>>  +		return connector_status_connected;
>>  +	}
>>  +
>>  +	return connector_status_disconnected;
>>  +}
>>  +
>>  +static void ingenic_drm_conn_destroy(struct drm_connector *conn)
>>  +{
>>  +	struct ingenic_drm *priv = drm_conn_get_priv(conn);
>>  +
>>  +	if (priv->panel)
>>  +		drm_panel_detach(priv->panel);
>>  +
>>  +	drm_connector_cleanup(conn);
>>  +}
>>  +
>>  +static int ingenic_drm_conn_get_modes(struct drm_connector *conn)
>>  +{
>>  +	struct ingenic_drm *priv = drm_conn_get_priv(conn);
>>  +
>>  +	if (priv->panel)
>>  +		return priv->panel->funcs->get_modes(priv->panel);
>>  +
>>  +	return 0;
>>  +}
>>  +
>>  +static irqreturn_t ingenic_drm_irq_handler(int irq, void *arg)
>>  +{
>>  +	struct drm_device *drm = arg;
>>  +	struct ingenic_drm *priv = drm->dev_private;
>>  +	unsigned int state;
>>  +
>>  +	regmap_read(priv->map, JZ_REG_LCD_STATE, &state);
>>  +
>>  +	regmap_update_bits(priv->map, JZ_REG_LCD_STATE,
>>  +			   JZ_LCD_STATE_EOF_IRQ, 0);
>>  +
>>  +	if (state & JZ_LCD_STATE_EOF_IRQ)
>>  +		drm_crtc_handle_vblank(&priv->crtc);
>>  +
>>  +	return IRQ_HANDLED;
>>  +}
>>  +
>>  +static int ingenic_drm_enable_vblank(struct drm_crtc *crtc)
>>  +{
>>  +	struct ingenic_drm *priv = drm_crtc_get_priv(crtc);
>>  +
>>  +	regmap_update_bits(priv->map, JZ_REG_LCD_CTRL,
>>  +			   JZ_LCD_CTRL_EOF_IRQ, JZ_LCD_CTRL_EOF_IRQ);
>>  +
>>  +	return 0;
>>  +}
>>  +
>>  +static void ingenic_drm_disable_vblank(struct drm_crtc *crtc)
>>  +{
>>  +	struct ingenic_drm *priv = drm_crtc_get_priv(crtc);
>>  +
>>  +	regmap_update_bits(priv->map, JZ_REG_LCD_CTRL, 
>> JZ_LCD_CTRL_EOF_IRQ, 0);
>>  +}
>>  +
>>  +DEFINE_DRM_GEM_CMA_FOPS(ingenic_drm_fops);
>>  +
>>  +static struct drm_driver ingenic_drm_driver_data = {
>>  +	.driver_features	= DRIVER_MODESET | DRIVER_GEM | DRIVER_PRIME
>>  +				| DRIVER_ATOMIC | DRIVER_HAVE_IRQ,
>>  +	.name			= "ingenic-drm",
>>  +	.desc			= "DRM module for Ingenic SoCs",
>>  +	.date			= "20190228",
>>  +	.major			= 1,
>>  +	.minor			= 0,
>>  +	.patchlevel		= 0,
>>  +
>>  +	.fops			= &ingenic_drm_fops,
>>  +
>>  +	.dumb_create		= drm_gem_cma_dumb_create,
>>  +	.gem_free_object_unlocked = drm_gem_cma_free_object,
>>  +	.gem_vm_ops		= &drm_gem_cma_vm_ops,
>>  +
>>  +	.prime_handle_to_fd	= drm_gem_prime_handle_to_fd,
>>  +	.prime_fd_to_handle	= drm_gem_prime_fd_to_handle,
>>  +	.gem_prime_import	= drm_gem_prime_import,
>>  +	.gem_prime_export	= drm_gem_prime_export,
>>  +	.gem_prime_get_sg_table	= drm_gem_cma_prime_get_sg_table,
>>  +	.gem_prime_import_sg_table = drm_gem_cma_prime_import_sg_table,
>>  +	.gem_prime_vmap		= drm_gem_cma_prime_vmap,
>>  +	.gem_prime_vunmap	= drm_gem_cma_prime_vunmap,
>>  +	.gem_prime_mmap		= drm_gem_cma_prime_mmap,
>>  +
>>  +	.irq_handler		= ingenic_drm_irq_handler,
>>  +};
>>  +
>>  +static const struct drm_plane_funcs 
>> ingenic_drm_primary_plane_funcs = {
>>  +	.update_plane		= drm_atomic_helper_update_plane,
>>  +	.disable_plane		= drm_atomic_helper_disable_plane,
>>  +	.reset			= drm_atomic_helper_plane_reset,
>>  +
>>  +	.atomic_duplicate_state	= drm_atomic_helper_plane_duplicate_state,
>>  +	.atomic_destroy_state	= drm_atomic_helper_plane_destroy_state,
>>  +};
>>  +
>>  +static const struct drm_crtc_funcs ingenic_drm_crtc_funcs = {
>>  +	.reset			= drm_atomic_helper_crtc_reset,
>>  +	.set_config		= drm_atomic_helper_set_config,
>>  +	.page_flip		= drm_atomic_helper_page_flip,
>>  +
>>  +	.atomic_duplicate_state	= drm_atomic_helper_crtc_duplicate_state,
>>  +	.atomic_destroy_state	= drm_atomic_helper_crtc_destroy_state,
>>  +
>>  +	.enable_vblank		= ingenic_drm_enable_vblank,
>>  +	.disable_vblank		= ingenic_drm_disable_vblank,
>>  +
>>  +	.gamma_set		= drm_atomic_helper_legacy_gamma_set,
>>  +};
>>  +
>>  +static const struct drm_connector_funcs ingenic_drm_conn_funcs = {
>>  +	.fill_modes		= drm_helper_probe_single_connector_modes,
>>  +	.detect			= ingenic_drm_conn_detect,
>>  +	.destroy		= ingenic_drm_conn_destroy,
>>  +	.reset			= drm_atomic_helper_connector_reset,
>>  +	.atomic_duplicate_state	= 
>> drm_atomic_helper_connector_duplicate_state,
>>  +	.atomic_destroy_state	= drm_atomic_helper_connector_destroy_state,
>>  +};
>>  +
>>  +static const struct drm_plane_helper_funcs 
>> ingenic_drm_plane_helper_funcs = {
>>  +	.atomic_update		= ingenic_drm_plane_atomic_update,
>>  +	.prepare_fb		= drm_gem_fb_prepare_fb,
>>  +};
>>  +
>>  +static const struct drm_crtc_helper_funcs 
>> ingenic_drm_crtc_helper_funcs = {
>>  +	.atomic_enable		= ingenic_drm_crtc_atomic_enable,
>>  +	.atomic_disable		= ingenic_drm_crtc_atomic_disable,
>>  +	.atomic_flush		= ingenic_drm_crtc_atomic_flush,
>>  +	.atomic_check		= ingenic_drm_crtc_atomic_check,
>>  +};
>>  +
>>  +static const struct drm_connector_helper_funcs 
>> ingenic_drm_conn_helper_funcs = {
>>  +	.get_modes		= ingenic_drm_conn_get_modes,
>>  +};
>>  +
>>  +static const struct drm_mode_config_funcs 
>> ingenic_drm_mode_config_funcs = {
>>  +	.fb_create		= drm_gem_fb_create,
>>  +	.output_poll_changed	= drm_fb_helper_output_poll_changed,
>>  +	.atomic_check		= drm_atomic_helper_check,
>>  +	.atomic_commit		= drm_atomic_helper_commit,
>>  +};
>>  +
>>  +static const struct drm_encoder_funcs ingenic_drm_encoder_funcs = {
>>  +	.destroy		= drm_encoder_cleanup,
>>  +};
>>  +
>>  +static int ingenic_drm_probe(struct platform_device *pdev)
>>  +{
>>  +	const struct jz_soc_info *soc_info;
>>  +	struct device *dev = &pdev->dev;
>>  +	struct ingenic_drm *priv;
>>  +	struct clk *parent_clk;
>>  +	struct drm_device *drm;
>>  +	struct resource *mem;
>>  +	void __iomem *base;
>>  +	long parent_rate;
>>  +	int ret, irq;
>>  +
>>  +	soc_info = device_get_match_data(dev);
>>  +	if (!soc_info)
>>  +		return -EINVAL;
>>  +
>>  +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>>  +	if (!priv)
>>  +		return -ENOMEM;
>>  +
>>  +	priv->dev = dev;
>>  +
>>  +	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>  +	priv->base = base = devm_ioremap_resource(dev, mem);
>>  +	if (IS_ERR(base))
>>  +		return PTR_ERR(base);
>>  +
>>  +	irq = platform_get_irq(pdev, 0);
>>  +	if (irq < 0) {
>>  +		dev_err(dev, "Failed to get platform irq\n");
>>  +		return -ENOENT;
>>  +	}
>>  +
>>  +	priv->map = devm_regmap_init_mmio(dev, base,
>>  +					  &ingenic_drm_regmap_config);
>>  +	if (IS_ERR(priv->map)) {
>>  +		dev_err(dev, "Failed to create regmap\n");
>>  +		return PTR_ERR(priv->map);
>>  +	}
>>  +
>>  +	if (soc_info->needs_dev_clk) {
>>  +		priv->lcd_clk = devm_clk_get(dev, "lcd");
>>  +		if (IS_ERR(priv->lcd_clk)) {
>>  +			dev_err(dev, "Failed to get lcd clock\n");
>>  +			return PTR_ERR(priv->lcd_clk);
>>  +		}
>>  +	}
>>  +
>>  +	priv->pix_clk = devm_clk_get(dev, "lcd_pclk");
>>  +	if (IS_ERR(priv->pix_clk)) {
>>  +		dev_err(dev, "Failed to get pixel clock\n");
>>  +		return PTR_ERR(priv->pix_clk);
>>  +	}
>>  +
>>  +	priv->panel_node = of_parse_phandle(dev->of_node, 
>> "ingenic,panel", 0);
>>  +	if (!priv->panel_node) {
>>  +		DRM_INFO("No panel found");
>>  +	} else {
>>  +		priv->panel = of_drm_find_panel(priv->panel_node);
>>  +		of_node_put(priv->panel_node);
>>  +
>>  +		if (IS_ERR(priv->panel)) {
>>  +			if (PTR_ERR(priv->panel) == -EPROBE_DEFER)
>>  +				return -EPROBE_DEFER;
>>  +
>>  +			priv->panel = NULL;
>>  +		} else {
>>  +			ret = drm_panel_prepare(priv->panel);
>>  +			if (ret && ret != -ENOSYS)
>>  +				return ret;
>>  +		}
>>  +	}
>>  +
>>  +	if (priv->panel) {
>>  +		ret = device_property_read_u32(dev, "ingenic,lcd-mode",
>>  +					       &priv->lcd_mode);
>>  +		if (ret) {
>>  +			dev_err(dev, "Unable to read ingenic,lcd-mode property\n");
>>  +			return ret;
>>  +		}
>>  +	}
>>  +
>>  +	priv->framedesc = dma_alloc_coherent(dev, 
>> sizeof(*priv->framedesc),
>>  +					     &priv->framedesc_phys, GFP_KERNEL);
>>  +	if (!priv->framedesc)
>>  +		return -ENOMEM;
>>  +
>>  +	priv->framedesc->next = priv->framedesc_phys;
>>  +	priv->framedesc->id = 0xdeafbead;
>>  +
>>  +	drm = drm_dev_alloc(&ingenic_drm_driver_data, dev);
>>  +	if (IS_ERR(drm)) {
>>  +		ret = PTR_ERR(drm);
>>  +		goto err_free_dma;
>>  +	}
>>  +
>>  +	priv->drm = drm;
>>  +
>>  +	drm_mode_config_init(drm);
>>  +	drm->mode_config.min_width = 0;
>>  +	drm->mode_config.min_height = 0;
>>  +	drm->mode_config.max_width = 800;
>>  +	drm->mode_config.max_height = 600;
>>  +	drm->mode_config.funcs = &ingenic_drm_mode_config_funcs;
>>  +
>>  +	drm_plane_helper_add(&priv->primary, 
>> &ingenic_drm_plane_helper_funcs);
>>  +
>>  +	ret = drm_universal_plane_init(drm, &priv->primary,
>>  +				       0, &ingenic_drm_primary_plane_funcs,
>>  +				       ingenic_drm_primary_formats,
>>  +				       ARRAY_SIZE(ingenic_drm_primary_formats),
>>  +				       NULL, DRM_PLANE_TYPE_PRIMARY, NULL);
>>  +	if (ret) {
>>  +		dev_err(dev, "Failed to register primary plane: %i\n", ret);
>>  +		goto err_unref_drm;
>>  +	}
>>  +
>>  +	drm_crtc_helper_add(&priv->crtc, &ingenic_drm_crtc_helper_funcs);
>>  +
>>  +	ret = drm_crtc_init_with_planes(drm, &priv->crtc, &priv->primary,
>>  +					NULL, &ingenic_drm_crtc_funcs, NULL);
>>  +	if (ret) {
>>  +		dev_err(dev, "Failed to init CRTC: %i\n", ret);
>>  +		goto err_cleanup_plane;
>>  +	}
>>  +
>>  +	drm_connector_helper_add(&priv->connector,
>>  +				 &ingenic_drm_conn_helper_funcs);
>>  +	ret = drm_connector_init(drm, &priv->connector,
>>  +				 &ingenic_drm_conn_funcs,
>>  +				 DRM_MODE_CONNECTOR_Unknown);
>>  +	if (ret) {
>>  +		dev_err(dev, "Failed to init connector: %i\n", ret);
>>  +		goto err_cleanup_crtc;
>>  +	}
>>  +
>>  +	priv->encoder.possible_crtcs = 1;
>>  +
>>  +	ret = drm_encoder_init(drm, &priv->encoder, 
>> &ingenic_drm_encoder_funcs,
>>  +			       DRM_MODE_ENCODER_NONE, NULL);
>>  +	if (ret) {
>>  +		dev_err(dev, "Failed to init encoder: %i\n", ret);
>>  +		goto err_cleanup_connector;
>>  +	}
>>  +
>>  +	drm_connector_attach_encoder(&priv->connector, &priv->encoder);
>>  +
>>  +	platform_set_drvdata(pdev, drm);
>>  +	priv->drm = drm;
>>  +	drm->dev_private = priv;
>>  +
>>  +	ret = drm_irq_install(drm, irq);
>>  +	if (ret) {
>>  +		dev_err(dev, "Unable to install IRQ handler\n");
>>  +		goto err_cleanup_encoder;
>>  +	}
>>  +
>>  +	ret = drm_vblank_init(drm, 1);
>>  +	if (ret) {
>>  +		dev_err(dev, "Failed calling drm_vblank_init()\n");
>>  +		goto err_uninstall_irq;
>>  +	}
>>  +
>>  +	drm_mode_config_reset(drm);
>>  +
>>  +	ret = clk_prepare_enable(priv->pix_clk);
>>  +	if (ret) {
>>  +		dev_err(dev, "Unable to start pixel clock\n");
>>  +		goto err_cleanup_vblank;
>>  +	}
>>  +
>>  +	if (priv->lcd_clk) {
>>  +		parent_clk = clk_get_parent(priv->lcd_clk);
>>  +		parent_rate = clk_get_rate(parent_clk);
>>  +
>>  +		/* LCD Device clock must be 3x the pixel clock for STN panels,
>>  +		 * or 1.5x the pixel clock for TFT panels. To avoid having to
>>  +		 * check for the LCD device clock everytime we do a mode change,
>>  +		 * we set the LCD device clock to the highest rate possible.
>>  +		 */
>>  +		ret = clk_set_rate(priv->lcd_clk, parent_rate);
>>  +		if (ret) {
>>  +			dev_err(dev, "Unable to set LCD clock rate\n");
>>  +			goto err_pixclk_disable;
>>  +		}
>>  +
>>  +		ret = clk_prepare_enable(priv->lcd_clk);
>>  +		if (ret) {
>>  +			dev_err(dev, "Unable to start lcd clock\n");
>>  +			goto err_pixclk_disable;
>>  +		}
>>  +	}
>>  +
>>  +	ret = drm_fbdev_generic_setup(drm, 16);
>>  +	if (ret) {
>>  +		dev_err(dev, "Failed to init fbdev\n");
>>  +		goto err_devclk_disable;
>>  +	}
>>  +
>>  +	ret = drm_dev_register(drm, 0);
>>  +	if (ret) {
>>  +		dev_err(dev, "Failed to register DRM driver\n");
>>  +		goto err_cleanup_fbdev;
>>  +	}
>>  +
>>  +	return 0;
>>  +
>>  +err_cleanup_fbdev:
>>  +	drm_mode_config_cleanup(drm);
>>  +err_devclk_disable:
>>  +	if (priv->lcd_clk)
>>  +		clk_disable_unprepare(priv->lcd_clk);
>>  +err_pixclk_disable:
>>  +	clk_disable_unprepare(priv->pix_clk);
>>  +err_cleanup_vblank:
>>  +	drm_vblank_cleanup(drm);
>>  +err_uninstall_irq:
>>  +	drm_irq_uninstall(drm);
>>  +err_cleanup_encoder:
>>  +	drm_encoder_cleanup(&priv->encoder);
>>  +err_cleanup_connector:
>>  +	drm_connector_cleanup(&priv->connector);
>>  +err_cleanup_crtc:
>>  +	drm_crtc_cleanup(&priv->crtc);
>>  +err_cleanup_plane:
>>  +	drm_plane_cleanup(&priv->primary);
>>  +err_unref_drm:
>>  +	drm_dev_put(drm);
>>  +err_free_dma:
>>  +	dma_free_coherent(dev, sizeof(*priv->framedesc),
>>  +			  priv->framedesc, priv->framedesc_phys);
>>  +	return ret;
>>  +}
>>  +
>>  +static int ingenic_drm_remove(struct platform_device *pdev)
>>  +{
>>  +	struct drm_device *drm = platform_get_drvdata(pdev);
>>  +	struct ingenic_drm *priv = drm->dev_private;
>>  +
>>  +	drm_dev_unregister(drm);
>>  +	drm_mode_config_cleanup(drm);
>>  +
>>  +	if (priv->lcd_clk)
>>  +		clk_disable_unprepare(priv->lcd_clk);
>>  +	clk_disable_unprepare(priv->pix_clk);
>>  +
>>  +	drm_vblank_cleanup(drm);
>>  +	drm_irq_uninstall(drm);
>>  +
>>  +	drm_encoder_cleanup(&priv->encoder);
>>  +	drm_connector_cleanup(&priv->connector);
>>  +	drm_crtc_cleanup(&priv->crtc);
>>  +	drm_plane_cleanup(&priv->primary);
>>  +
>>  +	drm_dev_put(drm);
>>  +
>>  +	dma_free_coherent(&pdev->dev, sizeof(*priv->framedesc),
>>  +			  priv->framedesc, priv->framedesc_phys);
>>  +
>>  +	return 0;
>>  +}
>>  +
>>  +static const struct jz_soc_info jz4740_soc_info = {
>>  +	.needs_dev_clk = true,
>>  +};
>>  +
>>  +static const struct jz_soc_info jz4725b_soc_info = {
>>  +	.needs_dev_clk = false,
>>  +};
>>  +
>>  +static const struct of_device_id ingenic_drm_of_match[] = {
>>  +	{ .compatible = "ingenic,jz4740-drm", .data = &jz4740_soc_info },
>>  +	{ .compatible = "ingenic,jz4725b-drm", .data = &jz4725b_soc_info 
>> },
>>  +	{},
>>  +};
>>  +
>>  +static struct platform_driver ingenic_drm_driver = {
>>  +	.driver = {
>>  +		.name = "ingenic-drm",
>>  +		.of_match_table = of_match_ptr(ingenic_drm_of_match),
>>  +	},
>>  +	.probe = ingenic_drm_probe,
>>  +	.remove = ingenic_drm_remove,
>>  +};
>>  +module_platform_driver(ingenic_drm_driver);
>>  +
>>  +MODULE_AUTHOR("Paul Cercueil <paul@crapouillou.net>");
>>  +MODULE_DESCRIPTION("DRM driver for the Ingenic SoCs\n");
>>  +MODULE_LICENSE("GPL v2");
>>  --
>>  2.11.0
>> 
> 
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

Re: [PATCH 1/3] dt-bindings: Add doc for the ingenic-drm driver

[Sam Ravnborg]

Hi Paul.

Good to see work migrating fbdev => drm.

Following comments is based on experience while working on migrating
another driver from fbdev => drm.

On Thu, Feb 28, 2019 at 07:07:54PM -0300, Paul Cercueil wrote:
> Add documentation for the devicetree bindings of the DRM driver for the
> JZ47xx family of SoCs from Ingenic.
> 
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> Tested-by: Artur Rojek <contact@artur-rojek.eu>
> ---
>  .../devicetree/bindings/display/ingenic,drm.txt    | 30 ++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/display/ingenic,drm.txt
> 
> diff --git a/Documentation/devicetree/bindings/display/ingenic,drm.txt b/Documentation/devicetree/bindings/display/ingenic,drm.txt
> new file mode 100644
> index 000000000000..52a368ec35cd
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/ingenic,drm.txt
> @@ -0,0 +1,30 @@
> +Ingenic JZ47xx DRM driver
> +
> +Required properties:
> +- compatible: one of:
> +  * ingenic,jz4740-drm
> +  * ingenic,jz4725b-drm
> +- reg: LCD registers location and length
The fbdev way of doing this was to hardcode register setttings.
In the drm world we read the configuration of the panel and let
this decide the actual configuration.

Note that no drm drivers supports stn displays and that there
is really no use for this these days. So you can skip the STN parts.

Also the modern way to specify a panel in the DT is using OF graph bindings.
You can find many examples how to do this.

I was there with the atmel LCDC driver (not in the kernel yet),
and had some challenges with this.

	Sam

Re: [PATCH 3/3] DRM: Add KMS driver for the Ingenic JZ47xx SoCs

[Sam Ravnborg]

Hi Paul.

Driver looks good and is a very nice piece of work.
In the following a number of minor issues.

One area that jumped at me was framedesc and the use of dma_alloc_coherent()
I hope someone that knows the memory handling better can give some advice here.
To me it looks like something drm should be able to help you with.

	Sam

> --- a/drivers/gpu/drm/Makefile
> +++ b/drivers/gpu/drm/Makefile
> @@ -94,6 +94,7 @@ obj-$(CONFIG_DRM_TEGRA) += tegra/
>  obj-$(CONFIG_DRM_STM) += stm/
>  obj-$(CONFIG_DRM_STI) += sti/
>  obj-$(CONFIG_DRM_IMX) += imx/
> +obj-y			+= ingenic/

To avoid visiting ingenic/ dir for every build use:
obj-$(CONFIG_DRM_INGENIC) += ingennic/

And accept that you need to do this also in ingenic/Makefile

>  obj-$(CONFIG_DRM_MEDIATEK) += mediatek/
>  obj-$(CONFIG_DRM_MESON)	+= meson/
>  obj-y			+= i2c/
> +#include <drm/drm_gem_cma_helper.h>
> +#include <drm/drm_fb_cma_helper.h>
> +#include <drm/drm_fb_helper.h>
> +#include <drm/drm_fourcc.h>
> +#include <drm/drm_gem_framebuffer_helper.h>
> +#include <drm/drm_irq.h>
> +#include <drm/drm_panel.h>
> +#include <drm/drm_plane.h>
> +#include <drm/drm_plane_helper.h>
> +#include <drm/drm_probe_helper.h>
> +#include <drm/drm_vblank.h>
> +
> +#include <dt-bindings/display/ingenic,drm.h>
> +
> +#include "../drm_internal.h"
No other drivers needs drm_internal.h - what makes this driver special?
Or do we have something in drm_internal that should be moved to
include/drm/ ?

> +struct ingenic_framedesc {
> +	uint32_t next;
> +	uint32_t addr;
> +	uint32_t id;
> +	uint32_t cmd;
> +} __packed;
For internel types u32 is the typical use.
uint32_t is usually used in uapi.

Consider to use dma_addr_t for addresses - we see this in other drivers.
If there are alignemnt constraints then add these.

> +
> +struct jz_soc_info {
> +	bool needs_dev_clk;
> +};
> +
> +struct ingenic_drm {
> +	struct device *dev;
> +	void __iomem *base;
> +	struct regmap *map;
> +	struct clk *lcd_clk, *pix_clk;
> +
> +	u32 lcd_mode;
> +
> +	struct ingenic_framedesc *framedesc;
> +	dma_addr_t framedesc_phys;
> +
> +	struct drm_device *drm;
> +	struct drm_plane primary;
> +	struct drm_crtc crtc;
> +	struct drm_connector connector;
> +	struct drm_encoder encoder;
> +	struct drm_panel *panel;
> +
> +	struct device_node *panel_node;
panel_node is not used outside ingenic_drm_probe() so no need
to have it here.

> +
> +	unsigned short ps_start, ps_end, cls_start,
> +		       cls_end, spl_start, spl_end, rev_start;
I do not see these used, they can all be dropped.

> +};
> +
> +
> +static const struct regmap_config ingenic_drm_regmap_config = {
> +	.reg_bits = 32,
> +	.val_bits = 32,
> +	.reg_stride = 4,
> +
> +	.max_register = JZ_REG_LCD_CMD1,
> +	.writeable_reg = ingenic_drm_writeable_reg,
> +};

+1 for using regmap.

> +
> +static inline bool ingenic_drm_lcd_is_special_mode(u32 mode)
> +{
> +	switch (mode) {
> +	case JZ_DRM_LCD_SPECIAL_TFT_1:
> +	case JZ_DRM_LCD_SPECIAL_TFT_2:
> +	case JZ_DRM_LCD_SPECIAL_TFT_3:
> +		return true;
> +	default:
> +		return false;
> +	}
> +}
Does it make sense to support these modes today?
If it is not used in practice then no need to carry it in the driver.


> +
> +static inline bool ingenic_drm_lcd_is_stn_mode(u32 mode)
> +{
> +	switch (mode) {
> +	case JZ_DRM_LCD_SINGLE_COLOR_STN:
> +	case JZ_DRM_LCD_SINGLE_MONOCHROME_STN:
> +	case JZ_DRM_LCD_DUAL_COLOR_STN:
> +	case JZ_DRM_LCD_DUAL_MONOCHROME_STN:
> +		return true;
> +	default:
> +		return false;
> +	}
> +}
This function is not used and can be deleted.
stn display are not really worth it these days anyway.


> +static void ingenic_drm_crtc_atomic_flush(struct drm_crtc *crtc,
> +					  struct drm_crtc_state *oldstate)
> +{
> +	struct ingenic_drm *priv = drm_crtc_get_priv(crtc);
> +	struct drm_crtc_state *state = crtc->state;
> +	struct drm_pending_vblank_event *event = state->event;
> +	struct drm_framebuffer *drm_fb = crtc->primary->state->fb;
> +	const struct drm_format_info *finfo;
> +	unsigned int width, height;
> +
> +	if (drm_atomic_crtc_needs_modeset(state)) {
> +		finfo = drm_format_info(drm_fb->format->format);
> +		width = state->adjusted_mode.hdisplay;
> +		height = state->adjusted_mode.vdisplay;

width and height are unused and can be dropped.

> +
> +		ingenic_drm_crtc_update_timings(priv, &state->mode);
> +
> +		ingenic_drm_crtc_update_ctrl(priv, finfo->depth);
> +		ingenic_drm_crtc_update_cfg(priv, &state->adjusted_mode);
> +
> +		clk_set_rate(priv->pix_clk, state->adjusted_mode.clock * 1000);
> +
> +		regmap_write(priv->map, JZ_REG_LCD_DA0, priv->framedesc->next);
> +	}
> +
> +	if (event) {
> +		state->event = NULL;
> +
> +		spin_lock_irq(&crtc->dev->event_lock);
> +		if (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 void ingenic_drm_plane_atomic_update(struct drm_plane *plane,
> +					    struct drm_plane_state *oldstate)
> +{
> +	struct ingenic_drm *priv = drm_plane_get_priv(plane);
> +	struct drm_plane_state *state = plane->state;
> +	const struct drm_format_info *finfo;
> +	unsigned int width, height;
> +
> +	finfo = drm_format_info(state->fb->format->format);
> +	width = state->crtc->state->adjusted_mode.hdisplay;
> +	height = state->crtc->state->adjusted_mode.vdisplay;
adjusted_mode->{h,v}display are both ints. So there is a hidden conversion to unsigned int here.
and framedesc->cmd is uint32_t then this is maybe fine.
Noticed so added a comment about it.

> +
> +	priv->framedesc->addr = drm_fb_cma_get_gem_addr(state->fb, state, 0);
> +
> +	priv->framedesc->cmd = width * height * ((finfo->depth + 7) / 8) / 4;
> +	priv->framedesc->cmd |= JZ_LCD_CMD_EOF_IRQ;
> +}
> +
> +static struct drm_driver ingenic_drm_driver_data = {
> +	.driver_features	= DRIVER_MODESET | DRIVER_GEM | DRIVER_PRIME
> +				| DRIVER_ATOMIC | DRIVER_HAVE_IRQ,

DRIVER_HAVE_IRQ are only for legacy drivers these days.
You can drop it.

> +	.name			= "ingenic-drm",
> +	.desc			= "DRM module for Ingenic SoCs",
> +	.date			= "20190228",
> +	.major			= 1,
> +	.minor			= 0,
> +	.patchlevel		= 0,
> +
> +	.fops			= &ingenic_drm_fops,
> +
> +	.dumb_create		= drm_gem_cma_dumb_create,
> +	.gem_free_object_unlocked = drm_gem_cma_free_object,
> +	.gem_vm_ops		= &drm_gem_cma_vm_ops,
> +
> +	.prime_handle_to_fd	= drm_gem_prime_handle_to_fd,
> +	.prime_fd_to_handle	= drm_gem_prime_fd_to_handle,

> +	.gem_prime_import	= drm_gem_prime_import,
> +	.gem_prime_export	= drm_gem_prime_export,
The two assignments above are not needed. This is the default behaviour. See drm_drv.h
(from drm-misc-next)

> +static int ingenic_drm_probe(struct platform_device *pdev)
> +{
> +	const struct jz_soc_info *soc_info;
> +	struct device *dev = &pdev->dev;
> +	struct ingenic_drm *priv;
> +	struct clk *parent_clk;
> +	struct drm_device *drm;
> +	struct resource *mem;
> +	void __iomem *base;
> +	long parent_rate;
> +	int ret, irq;
> +
> +	soc_info = device_get_match_data(dev);
> +	if (!soc_info)
> +		return -EINVAL;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->dev = dev;
> +
> +	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	priv->base = base = devm_ioremap_resource(dev, mem);
> +	if (IS_ERR(base))
> +		return PTR_ERR(base);
> +
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq < 0) {
> +		dev_err(dev, "Failed to get platform irq\n");
> +		return -ENOENT;
> +	}
> +
> +	priv->map = devm_regmap_init_mmio(dev, base,
> +					  &ingenic_drm_regmap_config);
> +	if (IS_ERR(priv->map)) {
> +		dev_err(dev, "Failed to create regmap\n");
> +		return PTR_ERR(priv->map);
> +	}
> +
> +	if (soc_info->needs_dev_clk) {
> +		priv->lcd_clk = devm_clk_get(dev, "lcd");
> +		if (IS_ERR(priv->lcd_clk)) {
> +			dev_err(dev, "Failed to get lcd clock\n");
> +			return PTR_ERR(priv->lcd_clk);
> +		}
> +	}
> +
> +	priv->pix_clk = devm_clk_get(dev, "lcd_pclk");
> +	if (IS_ERR(priv->pix_clk)) {
> +		dev_err(dev, "Failed to get pixel clock\n");
> +		return PTR_ERR(priv->pix_clk);
> +	}
> +
> +	priv->panel_node = of_parse_phandle(dev->of_node, "ingenic,panel", 0);
> +	if (!priv->panel_node) {
> +		DRM_INFO("No panel found");
> +	} else {
> +		priv->panel = of_drm_find_panel(priv->panel_node);
> +		of_node_put(priv->panel_node);
> +
> +		if (IS_ERR(priv->panel)) {
> +			if (PTR_ERR(priv->panel) == -EPROBE_DEFER)
> +				return -EPROBE_DEFER;
> +
> +			priv->panel = NULL;
> +		} else {
> +			ret = drm_panel_prepare(priv->panel);
> +			if (ret && ret != -ENOSYS)
> +				return ret;
> +		}
> +	}
> +
> +	if (priv->panel) {
> +		ret = device_property_read_u32(dev, "ingenic,lcd-mode",
> +					       &priv->lcd_mode);
> +		if (ret) {
> +			dev_err(dev, "Unable to read ingenic,lcd-mode property\n");
> +			return ret;
> +		}
> +	}
> +
> +	priv->framedesc = dma_alloc_coherent(dev, sizeof(*priv->framedesc),
> +					     &priv->framedesc_phys, GFP_KERNEL);
> +	if (!priv->framedesc)
> +		return -ENOMEM;
> +
> +	priv->framedesc->next = priv->framedesc_phys;
> +	priv->framedesc->id = 0xdeafbead;
I did not really follow this code.
But you have framedesc that include a next pointer which is uint32_t,
and you assign to it framedesc_phys which is dmaaddr_t.
This looks fishy to me, but maybe it is fine.

When browsing other drivers I see uses of for example {dma,dmam}_pool_create()

When lookign at the origianl fbdev code it is obvious where this comes from.
Notice that in the fbdev code JZ_REG_LCD_DA0 register is updated in the enable function.
This is done different in this driver.
Just an observation, what is right I dunno.

> +static int ingenic_drm_remove(struct platform_device *pdev)
> +{
> +	struct drm_device *drm = platform_get_drvdata(pdev);
> +	struct ingenic_drm *priv = drm->dev_private;
> +
> +	drm_dev_unregister(drm);
> +	drm_mode_config_cleanup(drm);
> +
> +	if (priv->lcd_clk)
> +		clk_disable_unprepare(priv->lcd_clk);
> +	clk_disable_unprepare(priv->pix_clk);
> +
> +	drm_vblank_cleanup(drm);
> +	drm_irq_uninstall(drm);
> +
> +	drm_encoder_cleanup(&priv->encoder);
> +	drm_connector_cleanup(&priv->connector);
> +	drm_crtc_cleanup(&priv->crtc);
> +	drm_plane_cleanup(&priv->primary);
> +
> +	drm_dev_put(drm);
> +
> +	dma_free_coherent(&pdev->dev, sizeof(*priv->framedesc),
> +			  priv->framedesc, priv->framedesc_phys);
> +
> +	return 0;
> +}
> +
> +static const struct jz_soc_info jz4740_soc_info = {
> +	.needs_dev_clk = true,
> +};
> +
> +static const struct jz_soc_info jz4725b_soc_info = {
> +	.needs_dev_clk = false,
> +};
> +
> +static const struct of_device_id ingenic_drm_of_match[] = {
> +	{ .compatible = "ingenic,jz4740-drm", .data = &jz4740_soc_info },
> +	{ .compatible = "ingenic,jz4725b-drm", .data = &jz4725b_soc_info },
> +	{},
Use /* sentinel */ like in most drivers.

Re: [PATCH 1/3] dt-bindings: Add doc for the ingenic-drm driver

[Paul Cercueil]

Hi Sam, thanks for the feedback.

Le ven. 1 mars 2019 à 17:16, Sam Ravnborg <sam@ravnborg.org> a écrit :
> Hi Paul.
> 
> Good to see work migrating fbdev => drm.
> 
> Following comments is based on experience while working on migrating
> another driver from fbdev => drm.
> 
> On Thu, Feb 28, 2019 at 07:07:54PM -0300, Paul Cercueil wrote:
>>  Add documentation for the devicetree bindings of the DRM driver for 
>> the
>>  JZ47xx family of SoCs from Ingenic.
>> 
>>  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>>  Tested-by: Artur Rojek <contact@artur-rojek.eu>
>>  ---
>>   .../devicetree/bindings/display/ingenic,drm.txt    | 30 
>> ++++++++++++++++++++++
>>   1 file changed, 30 insertions(+)
>>   create mode 100644 
>> Documentation/devicetree/bindings/display/ingenic,drm.txt
>> 
>>  diff --git 
>> a/Documentation/devicetree/bindings/display/ingenic,drm.txt 
>> b/Documentation/devicetree/bindings/display/ingenic,drm.txt
>>  new file mode 100644
>>  index 000000000000..52a368ec35cd
>>  --- /dev/null
>>  +++ b/Documentation/devicetree/bindings/display/ingenic,drm.txt
>>  @@ -0,0 +1,30 @@
>>  +Ingenic JZ47xx DRM driver
>>  +
>>  +Required properties:
>>  +- compatible: one of:
>>  +  * ingenic,jz4740-drm
>>  +  * ingenic,jz4725b-drm
>>  +- reg: LCD registers location and length
> The fbdev way of doing this was to hardcode register setttings.
> In the drm world we read the configuration of the panel and let
> this decide the actual configuration.
> 
> Note that no drm drivers supports stn displays and that there
> is really no use for this these days. So you can skip the STN parts.

STN panels aren't really my problem; but I have a board with a Sharp
LS020B1DD01D panel, which requires the LCD IP to be configured for a
"special 2" panel. I also have a board with a "smart" panel (ili9331)
that needs a special configuration in the IP. There is nothing in
the drm_panel interface that tells me what mode I should use for what
panel...

> Also the modern way to specify a panel in the DT is using OF graph 
> bindings.
> You can find many examples how to do this.

Ok, I was looking at the Tegra driver, which uses a "nvidia,panel" DT
property. I will change it to use the graph thing.

> I was there with the atmel LCDC driver (not in the kernel yet),
> and had some challenges with this.
> 
> 	Sam

Thanks,
-Paul

Re: [PATCH 3/3] DRM: Add KMS driver for the Ingenic JZ47xx SoCs

[Paul Cercueil]

Hi,

Le ven. 1 mars 2019 à 18:00, Sam Ravnborg <sam@ravnborg.org> a écrit :
> Hi Paul.
> 
> Driver looks good and is a very nice piece of work.
> In the following a number of minor issues.
> 
> One area that jumped at me was framedesc and the use of 
> dma_alloc_coherent()
> I hope someone that knows the memory handling better can give some 
> advice here.
> To me it looks like something drm should be able to help you with.
> 
> 	Sam
> 
>>  --- a/drivers/gpu/drm/Makefile
>>  +++ b/drivers/gpu/drm/Makefile
>>  @@ -94,6 +94,7 @@ obj-$(CONFIG_DRM_TEGRA) += tegra/
>>   obj-$(CONFIG_DRM_STM) += stm/
>>   obj-$(CONFIG_DRM_STI) += sti/
>>   obj-$(CONFIG_DRM_IMX) += imx/
>>  +obj-y			+= ingenic/
> 
> To avoid visiting ingenic/ dir for every build use:
> obj-$(CONFIG_DRM_INGENIC) += ingennic/
> 
> And accept that you need to do this also in ingenic/Makefile

OK.

>>   obj-$(CONFIG_DRM_MEDIATEK) += mediatek/
>>   obj-$(CONFIG_DRM_MESON)	+= meson/
>>   obj-y			+= i2c/
>>  +#include <drm/drm_gem_cma_helper.h>
>>  +#include <drm/drm_fb_cma_helper.h>
>>  +#include <drm/drm_fb_helper.h>
>>  +#include <drm/drm_fourcc.h>
>>  +#include <drm/drm_gem_framebuffer_helper.h>
>>  +#include <drm/drm_irq.h>
>>  +#include <drm/drm_panel.h>
>>  +#include <drm/drm_plane.h>
>>  +#include <drm/drm_plane_helper.h>
>>  +#include <drm/drm_probe_helper.h>
>>  +#include <drm/drm_vblank.h>
>>  +
>>  +#include <dt-bindings/display/ingenic,drm.h>
>>  +
>>  +#include "../drm_internal.h"
> No other drivers needs drm_internal.h - what makes this driver 
> special?
> Or do we have something in drm_internal that should be moved to
> include/drm/ ?

I needed to include it for drm_vblank_cleanup().
But I guess I don't need to call that function? It's not obvious to me
how the error handling and driver removal should be done.

>>  +struct ingenic_framedesc {
>>  +	uint32_t next;
>>  +	uint32_t addr;
>>  +	uint32_t id;
>>  +	uint32_t cmd;
>>  +} __packed;
> For internel types u32 is the typical use.
> uint32_t is usually used in uapi.
> 
> Consider to use dma_addr_t for addresses - we see this in other 
> drivers.
> If there are alignemnt constraints then add these.

So 'ingenic_framedesc' represents a DMA descriptor, in the format that
the hardware expects it. The fields must be 32-bit. Is a dma_addr_t
the best option in that case?

>>  +
>>  +struct jz_soc_info {
>>  +	bool needs_dev_clk;
>>  +};
>>  +
>>  +struct ingenic_drm {
>>  +	struct device *dev;
>>  +	void __iomem *base;
>>  +	struct regmap *map;
>>  +	struct clk *lcd_clk, *pix_clk;
>>  +
>>  +	u32 lcd_mode;
>>  +
>>  +	struct ingenic_framedesc *framedesc;
>>  +	dma_addr_t framedesc_phys;
>>  +
>>  +	struct drm_device *drm;
>>  +	struct drm_plane primary;
>>  +	struct drm_crtc crtc;
>>  +	struct drm_connector connector;
>>  +	struct drm_encoder encoder;
>>  +	struct drm_panel *panel;
>>  +
>>  +	struct device_node *panel_node;
> panel_node is not used outside ingenic_drm_probe() so no need
> to have it here.

Noted.

> 
>>  +
>>  +	unsigned short ps_start, ps_end, cls_start,
>>  +		       cls_end, spl_start, spl_end, rev_start;
> I do not see these used, they can all be dropped.
> 
>>  +};
>>  +
>>  +
>>  +static const struct regmap_config ingenic_drm_regmap_config = {
>>  +	.reg_bits = 32,
>>  +	.val_bits = 32,
>>  +	.reg_stride = 4,
>>  +
>>  +	.max_register = JZ_REG_LCD_CMD1,
>>  +	.writeable_reg = ingenic_drm_writeable_reg,
>>  +};
> 
> +1 for using regmap.
> 
>>  +
>>  +static inline bool ingenic_drm_lcd_is_special_mode(u32 mode)
>>  +{
>>  +	switch (mode) {
>>  +	case JZ_DRM_LCD_SPECIAL_TFT_1:
>>  +	case JZ_DRM_LCD_SPECIAL_TFT_2:
>>  +	case JZ_DRM_LCD_SPECIAL_TFT_3:
>>  +		return true;
>>  +	default:
>>  +		return false;
>>  +	}
>>  +}
> Does it make sense to support these modes today?
> If it is not used in practice then no need to carry it in the driver.

Yes, I have a board that uses such a panel.

>>  +
>>  +static inline bool ingenic_drm_lcd_is_stn_mode(u32 mode)
>>  +{
>>  +	switch (mode) {
>>  +	case JZ_DRM_LCD_SINGLE_COLOR_STN:
>>  +	case JZ_DRM_LCD_SINGLE_MONOCHROME_STN:
>>  +	case JZ_DRM_LCD_DUAL_COLOR_STN:
>>  +	case JZ_DRM_LCD_DUAL_MONOCHROME_STN:
>>  +		return true;
>>  +	default:
>>  +		return false;
>>  +	}
>>  +}
> This function is not used and can be deleted.
> stn display are not really worth it these days anyway.
> 
> 
>>  +static void ingenic_drm_crtc_atomic_flush(struct drm_crtc *crtc,
>>  +					  struct drm_crtc_state *oldstate)
>>  +{
>>  +	struct ingenic_drm *priv = drm_crtc_get_priv(crtc);
>>  +	struct drm_crtc_state *state = crtc->state;
>>  +	struct drm_pending_vblank_event *event = state->event;
>>  +	struct drm_framebuffer *drm_fb = crtc->primary->state->fb;
>>  +	const struct drm_format_info *finfo;
>>  +	unsigned int width, height;
>>  +
>>  +	if (drm_atomic_crtc_needs_modeset(state)) {
>>  +		finfo = drm_format_info(drm_fb->format->format);
>>  +		width = state->adjusted_mode.hdisplay;
>>  +		height = state->adjusted_mode.vdisplay;
> 
> width and height are unused and can be dropped.
> 
>>  +
>>  +		ingenic_drm_crtc_update_timings(priv, &state->mode);
>>  +
>>  +		ingenic_drm_crtc_update_ctrl(priv, finfo->depth);
>>  +		ingenic_drm_crtc_update_cfg(priv, &state->adjusted_mode);
>>  +
>>  +		clk_set_rate(priv->pix_clk, state->adjusted_mode.clock * 1000);
>>  +
>>  +		regmap_write(priv->map, JZ_REG_LCD_DA0, priv->framedesc->next);
>>  +	}
>>  +
>>  +	if (event) {
>>  +		state->event = NULL;
>>  +
>>  +		spin_lock_irq(&crtc->dev->event_lock);
>>  +		if (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 void ingenic_drm_plane_atomic_update(struct drm_plane 
>> *plane,
>>  +					    struct drm_plane_state *oldstate)
>>  +{
>>  +	struct ingenic_drm *priv = drm_plane_get_priv(plane);
>>  +	struct drm_plane_state *state = plane->state;
>>  +	const struct drm_format_info *finfo;
>>  +	unsigned int width, height;
>>  +
>>  +	finfo = drm_format_info(state->fb->format->format);
>>  +	width = state->crtc->state->adjusted_mode.hdisplay;
>>  +	height = state->crtc->state->adjusted_mode.vdisplay;
> adjusted_mode->{h,v}display are both ints. So there is a hidden 
> conversion to unsigned int here.
> and framedesc->cmd is uint32_t then this is maybe fine.
> Noticed so added a comment about it.
> 
>>  +
>>  +	priv->framedesc->addr = drm_fb_cma_get_gem_addr(state->fb, state, 
>> 0);
>>  +
>>  +	priv->framedesc->cmd = width * height * ((finfo->depth + 7) / 8) 
>> / 4;
>>  +	priv->framedesc->cmd |= JZ_LCD_CMD_EOF_IRQ;
>>  +}
>>  +
>>  +static struct drm_driver ingenic_drm_driver_data = {
>>  +	.driver_features	= DRIVER_MODESET | DRIVER_GEM | DRIVER_PRIME
>>  +				| DRIVER_ATOMIC | DRIVER_HAVE_IRQ,
> 
> DRIVER_HAVE_IRQ are only for legacy drivers these days.
> You can drop it.
> 
>>  +	.name			= "ingenic-drm",
>>  +	.desc			= "DRM module for Ingenic SoCs",
>>  +	.date			= "20190228",
>>  +	.major			= 1,
>>  +	.minor			= 0,
>>  +	.patchlevel		= 0,
>>  +
>>  +	.fops			= &ingenic_drm_fops,
>>  +
>>  +	.dumb_create		= drm_gem_cma_dumb_create,
>>  +	.gem_free_object_unlocked = drm_gem_cma_free_object,
>>  +	.gem_vm_ops		= &drm_gem_cma_vm_ops,
>>  +
>>  +	.prime_handle_to_fd	= drm_gem_prime_handle_to_fd,
>>  +	.prime_fd_to_handle	= drm_gem_prime_fd_to_handle,
> 
>>  +	.gem_prime_import	= drm_gem_prime_import,
>>  +	.gem_prime_export	= drm_gem_prime_export,
> The two assignments above are not needed. This is the default 
> behaviour. See drm_drv.h
> (from drm-misc-next)
> 
>>  +static int ingenic_drm_probe(struct platform_device *pdev)
>>  +{
>>  +	const struct jz_soc_info *soc_info;
>>  +	struct device *dev = &pdev->dev;
>>  +	struct ingenic_drm *priv;
>>  +	struct clk *parent_clk;
>>  +	struct drm_device *drm;
>>  +	struct resource *mem;
>>  +	void __iomem *base;
>>  +	long parent_rate;
>>  +	int ret, irq;
>>  +
>>  +	soc_info = device_get_match_data(dev);
>>  +	if (!soc_info)
>>  +		return -EINVAL;
>>  +
>>  +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>>  +	if (!priv)
>>  +		return -ENOMEM;
>>  +
>>  +	priv->dev = dev;
>>  +
>>  +	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>  +	priv->base = base = devm_ioremap_resource(dev, mem);
>>  +	if (IS_ERR(base))
>>  +		return PTR_ERR(base);
>>  +
>>  +	irq = platform_get_irq(pdev, 0);
>>  +	if (irq < 0) {
>>  +		dev_err(dev, "Failed to get platform irq\n");
>>  +		return -ENOENT;
>>  +	}
>>  +
>>  +	priv->map = devm_regmap_init_mmio(dev, base,
>>  +					  &ingenic_drm_regmap_config);
>>  +	if (IS_ERR(priv->map)) {
>>  +		dev_err(dev, "Failed to create regmap\n");
>>  +		return PTR_ERR(priv->map);
>>  +	}
>>  +
>>  +	if (soc_info->needs_dev_clk) {
>>  +		priv->lcd_clk = devm_clk_get(dev, "lcd");
>>  +		if (IS_ERR(priv->lcd_clk)) {
>>  +			dev_err(dev, "Failed to get lcd clock\n");
>>  +			return PTR_ERR(priv->lcd_clk);
>>  +		}
>>  +	}
>>  +
>>  +	priv->pix_clk = devm_clk_get(dev, "lcd_pclk");
>>  +	if (IS_ERR(priv->pix_clk)) {
>>  +		dev_err(dev, "Failed to get pixel clock\n");
>>  +		return PTR_ERR(priv->pix_clk);
>>  +	}
>>  +
>>  +	priv->panel_node = of_parse_phandle(dev->of_node, 
>> "ingenic,panel", 0);
>>  +	if (!priv->panel_node) {
>>  +		DRM_INFO("No panel found");
>>  +	} else {
>>  +		priv->panel = of_drm_find_panel(priv->panel_node);
>>  +		of_node_put(priv->panel_node);
>>  +
>>  +		if (IS_ERR(priv->panel)) {
>>  +			if (PTR_ERR(priv->panel) == -EPROBE_DEFER)
>>  +				return -EPROBE_DEFER;
>>  +
>>  +			priv->panel = NULL;
>>  +		} else {
>>  +			ret = drm_panel_prepare(priv->panel);
>>  +			if (ret && ret != -ENOSYS)
>>  +				return ret;
>>  +		}
>>  +	}
>>  +
>>  +	if (priv->panel) {
>>  +		ret = device_property_read_u32(dev, "ingenic,lcd-mode",
>>  +					       &priv->lcd_mode);
>>  +		if (ret) {
>>  +			dev_err(dev, "Unable to read ingenic,lcd-mode property\n");
>>  +			return ret;
>>  +		}
>>  +	}
>>  +
>>  +	priv->framedesc = dma_alloc_coherent(dev, 
>> sizeof(*priv->framedesc),
>>  +					     &priv->framedesc_phys, GFP_KERNEL);
>>  +	if (!priv->framedesc)
>>  +		return -ENOMEM;
>>  +
>>  +	priv->framedesc->next = priv->framedesc_phys;
>>  +	priv->framedesc->id = 0xdeafbead;
> I did not really follow this code.
> But you have framedesc that include a next pointer which is uint32_t,
> and you assign to it framedesc_phys which is dmaaddr_t.
> This looks fishy to me, but maybe it is fine.

As explained above, the framedesc is the hardware DMA descriptor, its 
fields
must be 32-bit, that's why it's done this way.

> When browsing other drivers I see uses of for example 
> {dma,dmam}_pool_create()
> 
> When lookign at the origianl fbdev code it is obvious where this 
> comes from.
> Notice that in the fbdev code JZ_REG_LCD_DA0 register is updated in 
> the enable function.
> This is done different in this driver.
> Just an observation, what is right I dunno.

JZ_REG_LCD_DA0 should contain the address of the DMA descriptor. In 
this driver,
I create a single DMA descriptor that commands the transfer of a full 
frame.
It only has to be set once, so it could even be written in the probe 
function,
provided the clocks are enabled.

>>  +static int ingenic_drm_remove(struct platform_device *pdev)
>>  +{
>>  +	struct drm_device *drm = platform_get_drvdata(pdev);
>>  +	struct ingenic_drm *priv = drm->dev_private;
>>  +
>>  +	drm_dev_unregister(drm);
>>  +	drm_mode_config_cleanup(drm);
>>  +
>>  +	if (priv->lcd_clk)
>>  +		clk_disable_unprepare(priv->lcd_clk);
>>  +	clk_disable_unprepare(priv->pix_clk);
>>  +
>>  +	drm_vblank_cleanup(drm);
>>  +	drm_irq_uninstall(drm);
>>  +
>>  +	drm_encoder_cleanup(&priv->encoder);
>>  +	drm_connector_cleanup(&priv->connector);
>>  +	drm_crtc_cleanup(&priv->crtc);
>>  +	drm_plane_cleanup(&priv->primary);
>>  +
>>  +	drm_dev_put(drm);
>>  +
>>  +	dma_free_coherent(&pdev->dev, sizeof(*priv->framedesc),
>>  +			  priv->framedesc, priv->framedesc_phys);
>>  +
>>  +	return 0;
>>  +}
>>  +
>>  +static const struct jz_soc_info jz4740_soc_info = {
>>  +	.needs_dev_clk = true,
>>  +};
>>  +
>>  +static const struct jz_soc_info jz4725b_soc_info = {
>>  +	.needs_dev_clk = false,
>>  +};
>>  +
>>  +static const struct of_device_id ingenic_drm_of_match[] = {
>>  +	{ .compatible = "ingenic,jz4740-drm", .data = &jz4740_soc_info },
>>  +	{ .compatible = "ingenic,jz4725b-drm", .data = &jz4725b_soc_info 
>> },
>>  +	{},
> Use /* sentinel */ like in most drivers.

For this and other embedded comments I didn't directly reply: duly 
noted.

Thanks for the review.

Greetings,
-Paul

Re: [PATCH 1/3] dt-bindings: Add doc for the ingenic-drm driver

[Linus Walleij]

On Fri, Mar 1, 2019 at 11:41 PM Paul Cercueil <paul@crapouillou.net> wrote:

> I also have a board with a "smart" panel (ili9331)
> that needs a special configuration in the IP. There is nothing in
> the drm_panel interface that tells me what mode I should use for what
> panel...

Is the ILI9331 anything similar to ILI9322 that I have a driver
for in drivers/gpu/drm/panel/panel-ilitek-ili9322.c?
I was hoping they would be similar and we ẃould be able
to just reuse that driver for more of them but I might be
mistaken.

Yours,
Linus Walleij

Re: [PATCH 1/3] dt-bindings: Add doc for the ingenic-drm driver

[Rob Herring]

On Thu, Feb 28, 2019 at 4:08 PM Paul Cercueil <paul@crapouillou.net> wrote:
>
> Add documentation for the devicetree bindings of the DRM driver for the
> JZ47xx family of SoCs from Ingenic.
>
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> Tested-by: Artur Rojek <contact@artur-rojek.eu>
> ---
>  .../devicetree/bindings/display/ingenic,drm.txt    | 30 ++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/display/ingenic,drm.txt
>
> diff --git a/Documentation/devicetree/bindings/display/ingenic,drm.txt b/Documentation/devicetree/bindings/display/ingenic,drm.txt
> new file mode 100644
> index 000000000000..52a368ec35cd
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/ingenic,drm.txt
> @@ -0,0 +1,30 @@
> +Ingenic JZ47xx DRM driver
> +
> +Required properties:
> +- compatible: one of:
> +  * ingenic,jz4740-drm
> +  * ingenic,jz4725b-drm

DRM is a kernel thing. What's the h/w called?

> +- reg: LCD registers location and length
> +- clocks: LCD pixclock and device clock specifiers.
> +          The device clock is only required on the JZ4740.
> +- clock-names: "lcd_pclk" and "lcd"
> +- interrupts: Specifies the interrupt line the LCD controller is connected to.
> +
> +Optional properties:
> +- ingenic,panel: phandle to a display panel, if one is present

Use the graph binding or a child node. See other bindings.

> +- ingenic,lcd-mode: LCD mode to use with the display panel.
> +                   See <dt-bindings/display/ingenic,drm.h> for all the
> +                   possible values.
> +
> +Example:
> +
> +lcd: lcd-controller@13050000 {
> +       compatible = "ingenic,jz4725b-drm";
> +       reg = <0x13050000 0x1000>;
> +
> +       interrupt-parent = <&intc>;
> +       interrupts = <31>;
> +
> +       clocks = <&cgu JZ4725B_CLK_LCD>;
> +       clock-names = "lcd";
> +};
> --
> 2.11.0
>

Re: [PATCH 1/3] dt-bindings: Add doc for the ingenic-drm driver

[Paul Cercueil]

Hi Linus,

On Fri, Mar 8, 2019 at 11:23 PM, Linus Walleij 
<linus.walleij@linaro.org> wrote:
> On Fri, Mar 1, 2019 at 11:41 PM Paul Cercueil <paul@crapouillou.net 
> <mailto:paul@crapouillou.net>> wrote:
> 
>>  I also have a board with a "smart" panel (ili9331)
>>  that needs a special configuration in the IP. There is nothing in
>>  the drm_panel interface that tells me what mode I should use for 
>> what
>>  panel...
> 
> Is the ILI9331 anything similar to ILI9322 that I have a driver
> for in drivers/gpu/drm/panel/panel-ilitek-ili9322.c?
> I was hoping they would be similar and we ẃould be able
> to just reuse that driver for more of them but I might be
> mistaken.

That's a bit hard to tell right now, but from a quick look at the
datasheet, it looks quite different :(

-Paul

Re: [5.2][PATCH 0/3] Ingenic JZ47xx KMS driver

[Ezequiel Garcia]

On Thu, 2019-02-28 at 19:07 -0300, Paul Cercueil wrote:
> Hi,
> 
> This is a first attempt at a KMS driver for the JZ47xx MIPS SoCs by
> Ingenic. It is aimed to replace the aging jz4740-fb driver.
> 
> The driver will later be updated with new features (overlays, TV-out
> etc.), that's why I didn't go with the simple/tiny DRM driver.
> 
> The driver has been tested on the Ben Nanonote (JZ4740) and the
> RetroMini RS-90 (JZ4725B) handheld gaming console.
> 

Does this support JZ4780? Or otherwise,
is there any similarity with JZ4780 display controller?

Thanks,
Eze

Re: [5.2][PATCH 0/3] Ingenic JZ47xx KMS driver

[Paul Cercueil]

Hi Ezequiel,

On Mon, Mar 11, 2019 at 1:02 AM, Ezequiel Garcia 
<ezequiel@collabora.com> wrote:
> On Thu, 2019-02-28 at 19:07 -0300, Paul Cercueil wrote:
>>  Hi,
>> 
>>  This is a first attempt at a KMS driver for the JZ47xx MIPS SoCs by
>>  Ingenic. It is aimed to replace the aging jz4740-fb driver.
>> 
>>  The driver will later be updated with new features (overlays, TV-out
>>  etc.), that's why I didn't go with the simple/tiny DRM driver.
>> 
>>  The driver has been tested on the Ben Nanonote (JZ4740) and the
>>  RetroMini RS-90 (JZ4725B) handheld gaming console.
>> 
> 
> Does this support JZ4780? Or otherwise,
> is there any similarity with JZ4780 display controller?
> 
> Thanks,
> Eze

The JZ4780 LCD controller would in theory work with this driver, but
to test on the CI20 you'd need to add support for HDMI first.

-Paul

Re: [PATCH 3/3] DRM: Add KMS driver for the Ingenic JZ47xx SoCs

[Paul Cercueil]

Hi Daniel,

Le ven. 1 mars 2019 à 9:26, Daniel Vetter <daniel@ffwll.ch> a écrit :
> On Thu, Feb 28, 2019 at 07:07:56PM -0300, Paul Cercueil wrote:
>>  Add a KMS driver for the Ingenic JZ47xx family of SoCs.
>>  This driver is meant to replace the aging jz4740-fb driver.
>> 
>>  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>>  Tested-by: Artur Rojek <contact@artur-rojek.eu>
> 
> Yay, more fbdev drivers moving to atmic!
> 
> Going on vacations for a week now, so just a few quick high-level 
> comments
> below. Those should make the driver even smaller and cleaner.
> 
>>  ---
>>   drivers/gpu/drm/Kconfig               |   2 +
>>   drivers/gpu/drm/Makefile              |   1 +
>>   drivers/gpu/drm/ingenic/Kconfig       |  16 +
>>   drivers/gpu/drm/ingenic/Makefile      |   1 +
>>   drivers/gpu/drm/ingenic/ingenic-drm.c | 896 
>> ++++++++++++++++++++++++++++++++++
>>   5 files changed, 916 insertions(+)
>>   create mode 100644 drivers/gpu/drm/ingenic/Kconfig
>>   create mode 100644 drivers/gpu/drm/ingenic/Makefile
>>   create mode 100644 drivers/gpu/drm/ingenic/ingenic-drm.c
>> 
>>  diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
>>  index bd943a71756c..f1929112e559 100644
>>  --- a/drivers/gpu/drm/Kconfig
>>  +++ b/drivers/gpu/drm/Kconfig
>>  @@ -303,6 +303,8 @@ source "drivers/gpu/drm/sti/Kconfig"
>> 
>>   source "drivers/gpu/drm/imx/Kconfig"
>> 
>>  +source "drivers/gpu/drm/ingenic/Kconfig"
>>  +
>>   source "drivers/gpu/drm/v3d/Kconfig"
>> 
>>   source "drivers/gpu/drm/vc4/Kconfig"
>>  diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
>>  index 1ac55c65eac0..9666c0767f44 100644
>>  --- a/drivers/gpu/drm/Makefile
>>  +++ b/drivers/gpu/drm/Makefile
>>  @@ -94,6 +94,7 @@ obj-$(CONFIG_DRM_TEGRA) += tegra/
>>   obj-$(CONFIG_DRM_STM) += stm/
>>   obj-$(CONFIG_DRM_STI) += sti/
>>   obj-$(CONFIG_DRM_IMX) += imx/
>>  +obj-y			+= ingenic/
>>   obj-$(CONFIG_DRM_MEDIATEK) += mediatek/
>>   obj-$(CONFIG_DRM_MESON)	+= meson/
>>   obj-y			+= i2c/
>>  diff --git a/drivers/gpu/drm/ingenic/Kconfig 
>> b/drivers/gpu/drm/ingenic/Kconfig
>>  new file mode 100644
>>  index 000000000000..d82c3d37ec9c
>>  --- /dev/null
>>  +++ b/drivers/gpu/drm/ingenic/Kconfig
>>  @@ -0,0 +1,16 @@
>>  +config DRM_INGENIC
>>  +	tristate "DRM Support for Ingenic SoCs"
>>  +	depends on MIPS || COMPILE_TEST
>>  +	depends on DRM
>>  +	depends on CMA
>>  +	depends on OF
>>  +	select DRM_BRIDGE
>>  +	select DRM_PANEL_BRIDGE
>>  +	select DRM_KMS_HELPER
>>  +	select DRM_KMS_CMA_HELPER
>>  +	select DRM_GEM_CMA_HELPER
>>  +	select VT_HW_CONSOLE_BINDING if FRAMEBUFFER_CONSOLE
>>  +	help
>>  +	  Choose this option for DRM support for the Ingenic SoCs.
>>  +
>>  +	  If M is selected the module will be called ingenic-drm.
>>  diff --git a/drivers/gpu/drm/ingenic/Makefile 
>> b/drivers/gpu/drm/ingenic/Makefile
>>  new file mode 100644
>>  index 000000000000..11cac42ce0bb
>>  --- /dev/null
>>  +++ b/drivers/gpu/drm/ingenic/Makefile
>>  @@ -0,0 +1 @@
>>  +obj-$(CONFIG_DRM_INGENIC) += ingenic-drm.o
>>  diff --git a/drivers/gpu/drm/ingenic/ingenic-drm.c 
>> b/drivers/gpu/drm/ingenic/ingenic-drm.c
>>  new file mode 100644
>>  index 000000000000..18120deea96f
>>  --- /dev/null
>>  +++ b/drivers/gpu/drm/ingenic/ingenic-drm.c
>>  @@ -0,0 +1,896 @@
>>  +// SPDX-License-Identifier: GPL-2.0
>>  +//
>>  +// Ingenic JZ47xx KMS driver
>>  +//
>>  +// Copyright (C) 2019, Paul Cercueil <paul@crapouillou.net>
>>  +
>>  +#include <linux/clk.h>
>>  +#include <linux/dma-mapping.h>
>>  +#include <linux/module.h>
>>  +#include <linux/platform_device.h>
>>  +#include <linux/regmap.h>
>>  +
>>  +#include <drm/drm_atomic.h>
>>  +#include <drm/drm_atomic_helper.h>
>>  +#include <drm/drm_crtc.h>
>>  +#include <drm/drm_crtc_helper.h>
>>  +#include <drm/drm_drv.h>
>>  +#include <drm/drm_gem_cma_helper.h>
>>  +#include <drm/drm_fb_cma_helper.h>
>>  +#include <drm/drm_fb_helper.h>
>>  +#include <drm/drm_fourcc.h>
>>  +#include <drm/drm_gem_framebuffer_helper.h>
>>  +#include <drm/drm_irq.h>
>>  +#include <drm/drm_panel.h>
>>  +#include <drm/drm_plane.h>
>>  +#include <drm/drm_plane_helper.h>
>>  +#include <drm/drm_probe_helper.h>
>>  +#include <drm/drm_vblank.h>
>>  +
>>  +#include <dt-bindings/display/ingenic,drm.h>
>>  +
>>  +#include "../drm_internal.h"
>>  +
>>  +#define JZ_REG_LCD_CFG		0x00
>>  +#define JZ_REG_LCD_VSYNC	0x04
>>  +#define JZ_REG_LCD_HSYNC	0x08
>>  +#define JZ_REG_LCD_VAT		0x0C
>>  +#define JZ_REG_LCD_DAH		0x10
>>  +#define JZ_REG_LCD_DAV		0x14
>>  +#define JZ_REG_LCD_PS		0x18
>>  +#define JZ_REG_LCD_CLS		0x1C
>>  +#define JZ_REG_LCD_SPL		0x20
>>  +#define JZ_REG_LCD_REV		0x24
>>  +#define JZ_REG_LCD_CTRL		0x30
>>  +#define JZ_REG_LCD_STATE	0x34
>>  +#define JZ_REG_LCD_IID		0x38
>>  +#define JZ_REG_LCD_DA0		0x40
>>  +#define JZ_REG_LCD_SA0		0x44
>>  +#define JZ_REG_LCD_FID0		0x48
>>  +#define JZ_REG_LCD_CMD0		0x4C
>>  +#define JZ_REG_LCD_DA1		0x50
>>  +#define JZ_REG_LCD_SA1		0x54
>>  +#define JZ_REG_LCD_FID1		0x58
>>  +#define JZ_REG_LCD_CMD1		0x5C
>>  +
>>  +#define JZ_LCD_CFG_SLCD			BIT(31)
>>  +#define JZ_LCD_CFG_PS_DISABLE		BIT(23)
>>  +#define JZ_LCD_CFG_CLS_DISABLE		BIT(22)
>>  +#define JZ_LCD_CFG_SPL_DISABLE		BIT(21)
>>  +#define JZ_LCD_CFG_REV_DISABLE		BIT(20)
>>  +#define JZ_LCD_CFG_HSYNCM		BIT(19)
>>  +#define JZ_LCD_CFG_PCLKM		BIT(18)
>>  +#define JZ_LCD_CFG_INV			BIT(17)
>>  +#define JZ_LCD_CFG_SYNC_DIR		BIT(16)
>>  +#define JZ_LCD_CFG_PS_POLARITY		BIT(15)
>>  +#define JZ_LCD_CFG_CLS_POLARITY		BIT(14)
>>  +#define JZ_LCD_CFG_SPL_POLARITY		BIT(13)
>>  +#define JZ_LCD_CFG_REV_POLARITY		BIT(12)
>>  +#define JZ_LCD_CFG_HSYNC_ACTIVE_LOW	BIT(11)
>>  +#define JZ_LCD_CFG_PCLK_FALLING_EDGE	BIT(10)
>>  +#define JZ_LCD_CFG_DE_ACTIVE_LOW	BIT(9)
>>  +#define JZ_LCD_CFG_VSYNC_ACTIVE_LOW	BIT(8)
>>  +#define JZ_LCD_CFG_18_BIT		BIT(7)
>>  +#define JZ_LCD_CFG_PDW			(BIT(5) | BIT(4))
>>  +#define JZ_LCD_CFG_MODE_MASK		0xf
>>  +
>>  +#define JZ_LCD_VSYNC_VPS_OFFSET		16
>>  +#define JZ_LCD_VSYNC_VPE_OFFSET		0
>>  +
>>  +#define JZ_LCD_HSYNC_HPS_OFFSET		16
>>  +#define JZ_LCD_HSYNC_HPE_OFFSET		0
>>  +
>>  +#define JZ_LCD_VAT_HT_OFFSET		16
>>  +#define JZ_LCD_VAT_VT_OFFSET		0
>>  +
>>  +#define JZ_LCD_DAH_HDS_OFFSET		16
>>  +#define JZ_LCD_DAH_HDE_OFFSET		0
>>  +
>>  +#define JZ_LCD_DAV_VDS_OFFSET		16
>>  +#define JZ_LCD_DAV_VDE_OFFSET		0
>>  +
>>  +#define JZ_LCD_CTRL_BURST_4		(0x0 << 28)
>>  +#define JZ_LCD_CTRL_BURST_8		(0x1 << 28)
>>  +#define JZ_LCD_CTRL_BURST_16		(0x2 << 28)
>>  +#define JZ_LCD_CTRL_RGB555		BIT(27)
>>  +#define JZ_LCD_CTRL_OFUP		BIT(26)
>>  +#define JZ_LCD_CTRL_FRC_GRAYSCALE_16	(0x0 << 24)
>>  +#define JZ_LCD_CTRL_FRC_GRAYSCALE_4	(0x1 << 24)
>>  +#define JZ_LCD_CTRL_FRC_GRAYSCALE_2	(0x2 << 24)
>>  +#define JZ_LCD_CTRL_PDD_MASK		(0xff << 16)
>>  +#define JZ_LCD_CTRL_EOF_IRQ		BIT(13)
>>  +#define JZ_LCD_CTRL_SOF_IRQ		BIT(12)
>>  +#define JZ_LCD_CTRL_OFU_IRQ		BIT(11)
>>  +#define JZ_LCD_CTRL_IFU0_IRQ		BIT(10)
>>  +#define JZ_LCD_CTRL_IFU1_IRQ		BIT(9)
>>  +#define JZ_LCD_CTRL_DD_IRQ		BIT(8)
>>  +#define JZ_LCD_CTRL_QDD_IRQ		BIT(7)
>>  +#define JZ_LCD_CTRL_REVERSE_ENDIAN	BIT(6)
>>  +#define JZ_LCD_CTRL_LSB_FISRT		BIT(5)
>>  +#define JZ_LCD_CTRL_DISABLE		BIT(4)
>>  +#define JZ_LCD_CTRL_ENABLE		BIT(3)
>>  +#define JZ_LCD_CTRL_BPP_1		0x0
>>  +#define JZ_LCD_CTRL_BPP_2		0x1
>>  +#define JZ_LCD_CTRL_BPP_4		0x2
>>  +#define JZ_LCD_CTRL_BPP_8		0x3
>>  +#define JZ_LCD_CTRL_BPP_15_16		0x4
>>  +#define JZ_LCD_CTRL_BPP_18_24		0x5
>>  +#define JZ_LCD_CTRL_BPP_MASK		(JZ_LCD_CTRL_RGB555 | (0x7 << 0))
>>  +
>>  +#define JZ_LCD_CMD_SOF_IRQ		BIT(31)
>>  +#define JZ_LCD_CMD_EOF_IRQ		BIT(30)
>>  +#define JZ_LCD_CMD_ENABLE_PAL		BIT(28)
>>  +
>>  +#define JZ_LCD_SYNC_MASK		0x3ff
>>  +
>>  +#define JZ_LCD_STATE_EOF_IRQ		BIT(5)
>>  +#define JZ_LCD_STATE_SOF_IRQ		BIT(4)
>>  +#define JZ_LCD_STATE_DISABLED		BIT(0)
>>  +
>>  +struct ingenic_framedesc {
>>  +	uint32_t next;
>>  +	uint32_t addr;
>>  +	uint32_t id;
>>  +	uint32_t cmd;
>>  +} __packed;
>>  +
>>  +struct jz_soc_info {
>>  +	bool needs_dev_clk;
>>  +};
>>  +
>>  +struct ingenic_drm {
>>  +	struct device *dev;
> 
> Embedding is kinda the new thing, since it also prevents people from 
> using
> devm_ for this, which is (strictly speaking, not that it matters 
> much) the
> wrong lifetime for drm allocations. There's some patches from Noralf 
> that
> should land soon to make this a bit more convenient.
> 
>>  +	void __iomem *base;
>>  +	struct regmap *map;
>>  +	struct clk *lcd_clk, *pix_clk;
>>  +
>>  +	u32 lcd_mode;
>>  +
>>  +	struct ingenic_framedesc *framedesc;
>>  +	dma_addr_t framedesc_phys;
>>  +
>>  +	struct drm_device *drm;
>>  +	struct drm_plane primary;
>>  +	struct drm_crtc crtc;
>>  +	struct drm_connector connector;
>>  +	struct drm_encoder encoder;
> 
> Any reason you didn't opt for the drm_simple_display_pipe helpers? 
> Since
> you have just 1:1:1 between crtc/plane/encoder that will allow you to
> ditch lots of boilerplate.
> 
> https://dri.freedesktop.org/docs/drm/gpu/drm-kms-helpers.html#simple-kms-helper-reference
> 
>>  +	struct drm_panel *panel;
>>  +
>>  +	struct device_node *panel_node;
> 
> There's panel bridge helpers which takes a panel and wraps it into a
> drm_bridge (which you can glue into the simple pipe helpers), giving 
> you
> the connector for free.
> 
> https://dri.freedesktop.org/docs/drm/gpu/drm-kms-helpers.html#panel-bridge-helper-reference

I'm having some trouble using this; if I use a drm_bridge and attach it
directly to the encoder, how can I get a pointer to the drm_connector?
I need it to retrieve the connector->display_info.bus_flags.

Thanks,
-Paul

> This should cover all the bigger items, details look already really 
> tidy.
> 
> Cheers, Daniel
> 
>>  +
>>  +	unsigned short ps_start, ps_end, cls_start,
>>  +		       cls_end, spl_start, spl_end, rev_start;
>>  +};
>>  +
>>  +static const uint32_t ingenic_drm_primary_formats[] = {
>>  +	DRM_FORMAT_XRGB1555,
>>  +	DRM_FORMAT_RGB565,
>>  +	DRM_FORMAT_XRGB8888,
>>  +};
>>  +
>>  +static bool ingenic_drm_writeable_reg(struct device *dev, unsigned 
>> int reg)
>>  +{
>>  +	switch (reg) {
>>  +	case JZ_REG_LCD_IID:
>>  +	case JZ_REG_LCD_SA0:
>>  +	case JZ_REG_LCD_FID0:
>>  +	case JZ_REG_LCD_CMD0:
>>  +	case JZ_REG_LCD_SA1:
>>  +	case JZ_REG_LCD_FID1:
>>  +	case JZ_REG_LCD_CMD1:
>>  +		return false;
>>  +	default:
>>  +		return true;
>>  +	}
>>  +}
>>  +
>>  +static const struct regmap_config ingenic_drm_regmap_config = {
>>  +	.reg_bits = 32,
>>  +	.val_bits = 32,
>>  +	.reg_stride = 4,
>>  +
>>  +	.max_register = JZ_REG_LCD_CMD1,
>>  +	.writeable_reg = ingenic_drm_writeable_reg,
>>  +};
>>  +
>>  +static inline struct ingenic_drm *drm_crtc_get_priv(struct 
>> drm_crtc *crtc)
>>  +{
>>  +	return container_of(crtc, struct ingenic_drm, crtc);
>>  +}
>>  +
>>  +static inline struct ingenic_drm *drm_plane_get_priv(struct 
>> drm_plane *plane)
>>  +{
>>  +	return container_of(plane, struct ingenic_drm, primary);
>>  +}
>>  +
>>  +static inline struct ingenic_drm *drm_conn_get_priv(struct 
>> drm_connector *conn)
>>  +{
>>  +	return container_of(conn, struct ingenic_drm, connector);
>>  +}
>>  +
>>  +static void ingenic_drm_crtc_atomic_enable(struct drm_crtc *crtc,
>>  +					  struct drm_crtc_state *state)
>>  +{
>>  +	struct ingenic_drm *priv = drm_crtc_get_priv(crtc);
>>  +
>>  +	regmap_write(priv->map, JZ_REG_LCD_STATE, 0);
>>  +
>>  +	regmap_update_bits(priv->map, JZ_REG_LCD_CTRL,
>>  +			   JZ_LCD_CTRL_ENABLE | JZ_LCD_CTRL_DISABLE,
>>  +			   JZ_LCD_CTRL_ENABLE);
>>  +
>>  +	drm_panel_enable(priv->panel);
>>  +
>>  +	drm_crtc_vblank_on(crtc);
>>  +}
>>  +
>>  +static void ingenic_drm_crtc_atomic_disable(struct drm_crtc *crtc,
>>  +					   struct drm_crtc_state *state)
>>  +{
>>  +	struct ingenic_drm *priv = drm_crtc_get_priv(crtc);
>>  +	unsigned int var;
>>  +
>>  +	drm_crtc_vblank_off(crtc);
>>  +
>>  +	drm_panel_disable(priv->panel);
>>  +
>>  +	regmap_update_bits(priv->map, JZ_REG_LCD_CTRL,
>>  +			   JZ_LCD_CTRL_DISABLE, JZ_LCD_CTRL_DISABLE);
>>  +
>>  +	regmap_read_poll_timeout(priv->map, JZ_REG_LCD_STATE, var,
>>  +				 var & JZ_LCD_STATE_DISABLED,
>>  +				 1000, 0);
>>  +}
>>  +
>>  +static inline bool ingenic_drm_lcd_is_special_mode(u32 mode)
>>  +{
>>  +	switch (mode) {
>>  +	case JZ_DRM_LCD_SPECIAL_TFT_1:
>>  +	case JZ_DRM_LCD_SPECIAL_TFT_2:
>>  +	case JZ_DRM_LCD_SPECIAL_TFT_3:
>>  +		return true;
>>  +	default:
>>  +		return false;
>>  +	}
>>  +}
>>  +
>>  +static inline bool ingenic_drm_lcd_is_stn_mode(u32 mode)
>>  +{
>>  +	switch (mode) {
>>  +	case JZ_DRM_LCD_SINGLE_COLOR_STN:
>>  +	case JZ_DRM_LCD_SINGLE_MONOCHROME_STN:
>>  +	case JZ_DRM_LCD_DUAL_COLOR_STN:
>>  +	case JZ_DRM_LCD_DUAL_MONOCHROME_STN:
>>  +		return true;
>>  +	default:
>>  +		return false;
>>  +	}
>>  +}
>>  +
>>  +static void ingenic_drm_crtc_update_timings(struct ingenic_drm 
>> *priv,
>>  +					    struct drm_display_mode *mode)
>>  +{
>>  +	unsigned int vpe, vds, vde, vt, hpe, hds, hde, ht;
>>  +
>>  +	vpe = mode->vsync_end - mode->vsync_start;
>>  +	vds = mode->vtotal - mode->vsync_start;
>>  +	vde = vds + mode->vdisplay;
>>  +	vt = vde + mode->vsync_start - mode->vdisplay;
>>  +
>>  +	hpe = mode->hsync_end - mode->hsync_start;
>>  +	hds = mode->htotal - mode->hsync_start;
>>  +	hde = hds + mode->hdisplay;
>>  +	ht = hde + mode->hsync_start - mode->hdisplay;
>>  +
>>  +	regmap_write(priv->map, JZ_REG_LCD_VSYNC,
>>  +		     0 << JZ_LCD_VSYNC_VPS_OFFSET |
>>  +		     vpe << JZ_LCD_VSYNC_VPE_OFFSET);
>>  +
>>  +	regmap_write(priv->map, JZ_REG_LCD_HSYNC,
>>  +		     0 << JZ_LCD_HSYNC_HPS_OFFSET |
>>  +		     hpe << JZ_LCD_HSYNC_HPE_OFFSET);
>>  +
>>  +	regmap_write(priv->map, JZ_REG_LCD_VAT,
>>  +		     ht << JZ_LCD_VAT_HT_OFFSET |
>>  +		     vt << JZ_LCD_VAT_VT_OFFSET);
>>  +
>>  +	regmap_write(priv->map, JZ_REG_LCD_DAH,
>>  +		     hds << JZ_LCD_DAH_HDS_OFFSET |
>>  +		     hde << JZ_LCD_DAH_HDE_OFFSET);
>>  +	regmap_write(priv->map, JZ_REG_LCD_DAV,
>>  +		     vds << JZ_LCD_DAV_VDS_OFFSET |
>>  +		     vde << JZ_LCD_DAV_VDE_OFFSET);
>>  +
>>  +	if (ingenic_drm_lcd_is_special_mode(priv->lcd_mode)) {
>>  +		regmap_write(priv->map, JZ_REG_LCD_PS, hde << 16 | (hde + 1));
>>  +		regmap_write(priv->map, JZ_REG_LCD_CLS, hde << 16 | (hde + 1));
>>  +		regmap_write(priv->map, JZ_REG_LCD_SPL, hpe << 16 | (hpe + 1));
>>  +		regmap_write(priv->map, JZ_REG_LCD_REV, mode->htotal << 16);
>>  +	}
>>  +}
>>  +
>>  +static void ingenic_drm_crtc_update_cfg(struct ingenic_drm *priv,
>>  +					struct drm_display_mode *mode)
>>  +
>>  +{
>>  +	unsigned int cfg = priv->lcd_mode;
>>  +	u32 bus_flags = priv->connector.display_info.bus_flags;
>>  +
>>  +	if (mode->flags & DRM_MODE_FLAG_NHSYNC)
>>  +		cfg |= JZ_LCD_CFG_HSYNC_ACTIVE_LOW;
>>  +	if (mode->flags & DRM_MODE_FLAG_NVSYNC)
>>  +		cfg |= JZ_LCD_CFG_VSYNC_ACTIVE_LOW;
>>  +	if (bus_flags & DRM_BUS_FLAG_DE_LOW)
>>  +		cfg |= JZ_LCD_CFG_DE_ACTIVE_LOW;
>>  +	if (bus_flags & DRM_BUS_FLAG_PIXDATA_NEGEDGE)
>>  +		cfg |= JZ_LCD_CFG_PCLK_FALLING_EDGE;
>>  +
>>  +	if (ingenic_drm_lcd_is_special_mode(priv->lcd_mode)) {
>>  +		// TODO: Is that valid for all special modes?
>>  +		cfg |= JZ_LCD_CFG_REV_POLARITY;
>>  +	} else {
>>  +		cfg |= JZ_LCD_CFG_PS_DISABLE
>>  +		    | JZ_LCD_CFG_CLS_DISABLE
>>  +		    | JZ_LCD_CFG_SPL_DISABLE
>>  +		    | JZ_LCD_CFG_REV_DISABLE;
>>  +	}
>>  +
>>  +	regmap_write(priv->map, JZ_REG_LCD_CFG, cfg);
>>  +}
>>  +
>>  +static void ingenic_drm_crtc_update_ctrl(struct ingenic_drm *priv,
>>  +					 unsigned int bpp)
>>  +{
>>  +	unsigned int ctrl = JZ_LCD_CTRL_OFUP | JZ_LCD_CTRL_BURST_16;
>>  +
>>  +	switch (bpp) {
>>  +	case 1:
>>  +		ctrl |= JZ_LCD_CTRL_BPP_1;
>>  +		break;
>>  +	case 2:
>>  +		ctrl |= JZ_LCD_CTRL_BPP_2;
>>  +		break;
>>  +	case 4:
>>  +		ctrl |= JZ_LCD_CTRL_BPP_4;
>>  +		break;
>>  +	case 8:
>>  +		ctrl |= JZ_LCD_CTRL_BPP_8;
>>  +	break;
>>  +	case 15:
>>  +		ctrl |= JZ_LCD_CTRL_RGB555; /* Falltrough */
>>  +	case 16:
>>  +		ctrl |= JZ_LCD_CTRL_BPP_15_16;
>>  +		break;
>>  +	case 18:
>>  +	case 24:
>>  +	case 32:
>>  +		ctrl |= JZ_LCD_CTRL_BPP_18_24;
>>  +		break;
>>  +	default:
>>  +		break;
>>  +	}
>>  +
>>  +	regmap_update_bits(priv->map, JZ_REG_LCD_CTRL,
>>  +			   JZ_LCD_CTRL_OFUP | JZ_LCD_CTRL_BURST_16 |
>>  +			   JZ_LCD_CTRL_BPP_MASK, ctrl);
>>  +}
>>  +
>>  +static int ingenic_drm_crtc_atomic_check(struct drm_crtc *crtc,
>>  +					 struct drm_crtc_state *state)
>>  +{
>>  +	struct ingenic_drm *priv = drm_crtc_get_priv(crtc);
>>  +	long rate;
>>  +
>>  +	if (!drm_atomic_crtc_needs_modeset(state))
>>  +		return 0;
>>  +
>>  +	rate = clk_round_rate(priv->pix_clk,
>>  +			      state->adjusted_mode.clock * 1000);
>>  +	if (rate < 0)
>>  +		return rate;
>>  +
>>  +	return 0;
>>  +}
>>  +
>>  +static void ingenic_drm_crtc_atomic_flush(struct drm_crtc *crtc,
>>  +					  struct drm_crtc_state *oldstate)
>>  +{
>>  +	struct ingenic_drm *priv = drm_crtc_get_priv(crtc);
>>  +	struct drm_crtc_state *state = crtc->state;
>>  +	struct drm_pending_vblank_event *event = state->event;
>>  +	struct drm_framebuffer *drm_fb = crtc->primary->state->fb;
>>  +	const struct drm_format_info *finfo;
>>  +	unsigned int width, height;
>>  +
>>  +	if (drm_atomic_crtc_needs_modeset(state)) {
>>  +		finfo = drm_format_info(drm_fb->format->format);
>>  +		width = state->adjusted_mode.hdisplay;
>>  +		height = state->adjusted_mode.vdisplay;
>>  +
>>  +		ingenic_drm_crtc_update_timings(priv, &state->mode);
>>  +
>>  +		ingenic_drm_crtc_update_ctrl(priv, finfo->depth);
>>  +		ingenic_drm_crtc_update_cfg(priv, &state->adjusted_mode);
>>  +
>>  +		clk_set_rate(priv->pix_clk, state->adjusted_mode.clock * 1000);
>>  +
>>  +		regmap_write(priv->map, JZ_REG_LCD_DA0, priv->framedesc->next);
>>  +	}
>>  +
>>  +	if (event) {
>>  +		state->event = NULL;
>>  +
>>  +		spin_lock_irq(&crtc->dev->event_lock);
>>  +		if (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 void ingenic_drm_plane_atomic_update(struct drm_plane 
>> *plane,
>>  +					    struct drm_plane_state *oldstate)
>>  +{
>>  +	struct ingenic_drm *priv = drm_plane_get_priv(plane);
>>  +	struct drm_plane_state *state = plane->state;
>>  +	const struct drm_format_info *finfo;
>>  +	unsigned int width, height;
>>  +
>>  +	finfo = drm_format_info(state->fb->format->format);
>>  +	width = state->crtc->state->adjusted_mode.hdisplay;
>>  +	height = state->crtc->state->adjusted_mode.vdisplay;
>>  +
>>  +	priv->framedesc->addr = drm_fb_cma_get_gem_addr(state->fb, state, 
>> 0);
>>  +
>>  +	priv->framedesc->cmd = width * height * ((finfo->depth + 7) / 8) 
>> / 4;
>>  +	priv->framedesc->cmd |= JZ_LCD_CMD_EOF_IRQ;
>>  +}
>>  +
>>  +static enum drm_connector_status
>>  +ingenic_drm_conn_detect(struct drm_connector *conn, bool force)
>>  +{
>>  +	struct ingenic_drm *priv = drm_conn_get_priv(conn);
>>  +
>>  +	if (priv->panel) {
>>  +		drm_panel_attach(priv->panel, conn);
>>  +
>>  +		return connector_status_connected;
>>  +	}
>>  +
>>  +	return connector_status_disconnected;
>>  +}
>>  +
>>  +static void ingenic_drm_conn_destroy(struct drm_connector *conn)
>>  +{
>>  +	struct ingenic_drm *priv = drm_conn_get_priv(conn);
>>  +
>>  +	if (priv->panel)
>>  +		drm_panel_detach(priv->panel);
>>  +
>>  +	drm_connector_cleanup(conn);
>>  +}
>>  +
>>  +static int ingenic_drm_conn_get_modes(struct drm_connector *conn)
>>  +{
>>  +	struct ingenic_drm *priv = drm_conn_get_priv(conn);
>>  +
>>  +	if (priv->panel)
>>  +		return priv->panel->funcs->get_modes(priv->panel);
>>  +
>>  +	return 0;
>>  +}
>>  +
>>  +static irqreturn_t ingenic_drm_irq_handler(int irq, void *arg)
>>  +{
>>  +	struct drm_device *drm = arg;
>>  +	struct ingenic_drm *priv = drm->dev_private;
>>  +	unsigned int state;
>>  +
>>  +	regmap_read(priv->map, JZ_REG_LCD_STATE, &state);
>>  +
>>  +	regmap_update_bits(priv->map, JZ_REG_LCD_STATE,
>>  +			   JZ_LCD_STATE_EOF_IRQ, 0);
>>  +
>>  +	if (state & JZ_LCD_STATE_EOF_IRQ)
>>  +		drm_crtc_handle_vblank(&priv->crtc);
>>  +
>>  +	return IRQ_HANDLED;
>>  +}
>>  +
>>  +static int ingenic_drm_enable_vblank(struct drm_crtc *crtc)
>>  +{
>>  +	struct ingenic_drm *priv = drm_crtc_get_priv(crtc);
>>  +
>>  +	regmap_update_bits(priv->map, JZ_REG_LCD_CTRL,
>>  +			   JZ_LCD_CTRL_EOF_IRQ, JZ_LCD_CTRL_EOF_IRQ);
>>  +
>>  +	return 0;
>>  +}
>>  +
>>  +static void ingenic_drm_disable_vblank(struct drm_crtc *crtc)
>>  +{
>>  +	struct ingenic_drm *priv = drm_crtc_get_priv(crtc);
>>  +
>>  +	regmap_update_bits(priv->map, JZ_REG_LCD_CTRL, 
>> JZ_LCD_CTRL_EOF_IRQ, 0);
>>  +}
>>  +
>>  +DEFINE_DRM_GEM_CMA_FOPS(ingenic_drm_fops);
>>  +
>>  +static struct drm_driver ingenic_drm_driver_data = {
>>  +	.driver_features	= DRIVER_MODESET | DRIVER_GEM | DRIVER_PRIME
>>  +				| DRIVER_ATOMIC | DRIVER_HAVE_IRQ,
>>  +	.name			= "ingenic-drm",
>>  +	.desc			= "DRM module for Ingenic SoCs",
>>  +	.date			= "20190228",
>>  +	.major			= 1,
>>  +	.minor			= 0,
>>  +	.patchlevel		= 0,
>>  +
>>  +	.fops			= &ingenic_drm_fops,
>>  +
>>  +	.dumb_create		= drm_gem_cma_dumb_create,
>>  +	.gem_free_object_unlocked = drm_gem_cma_free_object,
>>  +	.gem_vm_ops		= &drm_gem_cma_vm_ops,
>>  +
>>  +	.prime_handle_to_fd	= drm_gem_prime_handle_to_fd,
>>  +	.prime_fd_to_handle	= drm_gem_prime_fd_to_handle,
>>  +	.gem_prime_import	= drm_gem_prime_import,
>>  +	.gem_prime_export	= drm_gem_prime_export,
>>  +	.gem_prime_get_sg_table	= drm_gem_cma_prime_get_sg_table,
>>  +	.gem_prime_import_sg_table = drm_gem_cma_prime_import_sg_table,
>>  +	.gem_prime_vmap		= drm_gem_cma_prime_vmap,
>>  +	.gem_prime_vunmap	= drm_gem_cma_prime_vunmap,
>>  +	.gem_prime_mmap		= drm_gem_cma_prime_mmap,
>>  +
>>  +	.irq_handler		= ingenic_drm_irq_handler,
>>  +};
>>  +
>>  +static const struct drm_plane_funcs 
>> ingenic_drm_primary_plane_funcs = {
>>  +	.update_plane		= drm_atomic_helper_update_plane,
>>  +	.disable_plane		= drm_atomic_helper_disable_plane,
>>  +	.reset			= drm_atomic_helper_plane_reset,
>>  +
>>  +	.atomic_duplicate_state	= drm_atomic_helper_plane_duplicate_state,
>>  +	.atomic_destroy_state	= drm_atomic_helper_plane_destroy_state,
>>  +};
>>  +
>>  +static const struct drm_crtc_funcs ingenic_drm_crtc_funcs = {
>>  +	.reset			= drm_atomic_helper_crtc_reset,
>>  +	.set_config		= drm_atomic_helper_set_config,
>>  +	.page_flip		= drm_atomic_helper_page_flip,
>>  +
>>  +	.atomic_duplicate_state	= drm_atomic_helper_crtc_duplicate_state,
>>  +	.atomic_destroy_state	= drm_atomic_helper_crtc_destroy_state,
>>  +
>>  +	.enable_vblank		= ingenic_drm_enable_vblank,
>>  +	.disable_vblank		= ingenic_drm_disable_vblank,
>>  +
>>  +	.gamma_set		= drm_atomic_helper_legacy_gamma_set,
>>  +};
>>  +
>>  +static const struct drm_connector_funcs ingenic_drm_conn_funcs = {
>>  +	.fill_modes		= drm_helper_probe_single_connector_modes,
>>  +	.detect			= ingenic_drm_conn_detect,
>>  +	.destroy		= ingenic_drm_conn_destroy,
>>  +	.reset			= drm_atomic_helper_connector_reset,
>>  +	.atomic_duplicate_state	= 
>> drm_atomic_helper_connector_duplicate_state,
>>  +	.atomic_destroy_state	= drm_atomic_helper_connector_destroy_state,
>>  +};
>>  +
>>  +static const struct drm_plane_helper_funcs 
>> ingenic_drm_plane_helper_funcs = {
>>  +	.atomic_update		= ingenic_drm_plane_atomic_update,
>>  +	.prepare_fb		= drm_gem_fb_prepare_fb,
>>  +};
>>  +
>>  +static const struct drm_crtc_helper_funcs 
>> ingenic_drm_crtc_helper_funcs = {
>>  +	.atomic_enable		= ingenic_drm_crtc_atomic_enable,
>>  +	.atomic_disable		= ingenic_drm_crtc_atomic_disable,
>>  +	.atomic_flush		= ingenic_drm_crtc_atomic_flush,
>>  +	.atomic_check		= ingenic_drm_crtc_atomic_check,
>>  +};
>>  +
>>  +static const struct drm_connector_helper_funcs 
>> ingenic_drm_conn_helper_funcs = {
>>  +	.get_modes		= ingenic_drm_conn_get_modes,
>>  +};
>>  +
>>  +static const struct drm_mode_config_funcs 
>> ingenic_drm_mode_config_funcs = {
>>  +	.fb_create		= drm_gem_fb_create,
>>  +	.output_poll_changed	= drm_fb_helper_output_poll_changed,
>>  +	.atomic_check		= drm_atomic_helper_check,
>>  +	.atomic_commit		= drm_atomic_helper_commit,
>>  +};
>>  +
>>  +static const struct drm_encoder_funcs ingenic_drm_encoder_funcs = {
>>  +	.destroy		= drm_encoder_cleanup,
>>  +};
>>  +
>>  +static int ingenic_drm_probe(struct platform_device *pdev)
>>  +{
>>  +	const struct jz_soc_info *soc_info;
>>  +	struct device *dev = &pdev->dev;
>>  +	struct ingenic_drm *priv;
>>  +	struct clk *parent_clk;
>>  +	struct drm_device *drm;
>>  +	struct resource *mem;
>>  +	void __iomem *base;
>>  +	long parent_rate;
>>  +	int ret, irq;
>>  +
>>  +	soc_info = device_get_match_data(dev);
>>  +	if (!soc_info)
>>  +		return -EINVAL;
>>  +
>>  +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>>  +	if (!priv)
>>  +		return -ENOMEM;
>>  +
>>  +	priv->dev = dev;
>>  +
>>  +	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>  +	priv->base = base = devm_ioremap_resource(dev, mem);
>>  +	if (IS_ERR(base))
>>  +		return PTR_ERR(base);
>>  +
>>  +	irq = platform_get_irq(pdev, 0);
>>  +	if (irq < 0) {
>>  +		dev_err(dev, "Failed to get platform irq\n");
>>  +		return -ENOENT;
>>  +	}
>>  +
>>  +	priv->map = devm_regmap_init_mmio(dev, base,
>>  +					  &ingenic_drm_regmap_config);
>>  +	if (IS_ERR(priv->map)) {
>>  +		dev_err(dev, "Failed to create regmap\n");
>>  +		return PTR_ERR(priv->map);
>>  +	}
>>  +
>>  +	if (soc_info->needs_dev_clk) {
>>  +		priv->lcd_clk = devm_clk_get(dev, "lcd");
>>  +		if (IS_ERR(priv->lcd_clk)) {
>>  +			dev_err(dev, "Failed to get lcd clock\n");
>>  +			return PTR_ERR(priv->lcd_clk);
>>  +		}
>>  +	}
>>  +
>>  +	priv->pix_clk = devm_clk_get(dev, "lcd_pclk");
>>  +	if (IS_ERR(priv->pix_clk)) {
>>  +		dev_err(dev, "Failed to get pixel clock\n");
>>  +		return PTR_ERR(priv->pix_clk);
>>  +	}
>>  +
>>  +	priv->panel_node = of_parse_phandle(dev->of_node, 
>> "ingenic,panel", 0);
>>  +	if (!priv->panel_node) {
>>  +		DRM_INFO("No panel found");
>>  +	} else {
>>  +		priv->panel = of_drm_find_panel(priv->panel_node);
>>  +		of_node_put(priv->panel_node);
>>  +
>>  +		if (IS_ERR(priv->panel)) {
>>  +			if (PTR_ERR(priv->panel) == -EPROBE_DEFER)
>>  +				return -EPROBE_DEFER;
>>  +
>>  +			priv->panel = NULL;
>>  +		} else {
>>  +			ret = drm_panel_prepare(priv->panel);
>>  +			if (ret && ret != -ENOSYS)
>>  +				return ret;
>>  +		}
>>  +	}
>>  +
>>  +	if (priv->panel) {
>>  +		ret = device_property_read_u32(dev, "ingenic,lcd-mode",
>>  +					       &priv->lcd_mode);
>>  +		if (ret) {
>>  +			dev_err(dev, "Unable to read ingenic,lcd-mode property\n");
>>  +			return ret;
>>  +		}
>>  +	}
>>  +
>>  +	priv->framedesc = dma_alloc_coherent(dev, 
>> sizeof(*priv->framedesc),
>>  +					     &priv->framedesc_phys, GFP_KERNEL);
>>  +	if (!priv->framedesc)
>>  +		return -ENOMEM;
>>  +
>>  +	priv->framedesc->next = priv->framedesc_phys;
>>  +	priv->framedesc->id = 0xdeafbead;
>>  +
>>  +	drm = drm_dev_alloc(&ingenic_drm_driver_data, dev);
>>  +	if (IS_ERR(drm)) {
>>  +		ret = PTR_ERR(drm);
>>  +		goto err_free_dma;
>>  +	}
>>  +
>>  +	priv->drm = drm;
>>  +
>>  +	drm_mode_config_init(drm);
>>  +	drm->mode_config.min_width = 0;
>>  +	drm->mode_config.min_height = 0;
>>  +	drm->mode_config.max_width = 800;
>>  +	drm->mode_config.max_height = 600;
>>  +	drm->mode_config.funcs = &ingenic_drm_mode_config_funcs;
>>  +
>>  +	drm_plane_helper_add(&priv->primary, 
>> &ingenic_drm_plane_helper_funcs);
>>  +
>>  +	ret = drm_universal_plane_init(drm, &priv->primary,
>>  +				       0, &ingenic_drm_primary_plane_funcs,
>>  +				       ingenic_drm_primary_formats,
>>  +				       ARRAY_SIZE(ingenic_drm_primary_formats),
>>  +				       NULL, DRM_PLANE_TYPE_PRIMARY, NULL);
>>  +	if (ret) {
>>  +		dev_err(dev, "Failed to register primary plane: %i\n", ret);
>>  +		goto err_unref_drm;
>>  +	}
>>  +
>>  +	drm_crtc_helper_add(&priv->crtc, &ingenic_drm_crtc_helper_funcs);
>>  +
>>  +	ret = drm_crtc_init_with_planes(drm, &priv->crtc, &priv->primary,
>>  +					NULL, &ingenic_drm_crtc_funcs, NULL);
>>  +	if (ret) {
>>  +		dev_err(dev, "Failed to init CRTC: %i\n", ret);
>>  +		goto err_cleanup_plane;
>>  +	}
>>  +
>>  +	drm_connector_helper_add(&priv->connector,
>>  +				 &ingenic_drm_conn_helper_funcs);
>>  +	ret = drm_connector_init(drm, &priv->connector,
>>  +				 &ingenic_drm_conn_funcs,
>>  +				 DRM_MODE_CONNECTOR_Unknown);
>>  +	if (ret) {
>>  +		dev_err(dev, "Failed to init connector: %i\n", ret);
>>  +		goto err_cleanup_crtc;
>>  +	}
>>  +
>>  +	priv->encoder.possible_crtcs = 1;
>>  +
>>  +	ret = drm_encoder_init(drm, &priv->encoder, 
>> &ingenic_drm_encoder_funcs,
>>  +			       DRM_MODE_ENCODER_NONE, NULL);
>>  +	if (ret) {
>>  +		dev_err(dev, "Failed to init encoder: %i\n", ret);
>>  +		goto err_cleanup_connector;
>>  +	}
>>  +
>>  +	drm_connector_attach_encoder(&priv->connector, &priv->encoder);
>>  +
>>  +	platform_set_drvdata(pdev, drm);
>>  +	priv->drm = drm;
>>  +	drm->dev_private = priv;
>>  +
>>  +	ret = drm_irq_install(drm, irq);
>>  +	if (ret) {
>>  +		dev_err(dev, "Unable to install IRQ handler\n");
>>  +		goto err_cleanup_encoder;
>>  +	}
>>  +
>>  +	ret = drm_vblank_init(drm, 1);
>>  +	if (ret) {
>>  +		dev_err(dev, "Failed calling drm_vblank_init()\n");
>>  +		goto err_uninstall_irq;
>>  +	}
>>  +
>>  +	drm_mode_config_reset(drm);
>>  +
>>  +	ret = clk_prepare_enable(priv->pix_clk);
>>  +	if (ret) {
>>  +		dev_err(dev, "Unable to start pixel clock\n");
>>  +		goto err_cleanup_vblank;
>>  +	}
>>  +
>>  +	if (priv->lcd_clk) {
>>  +		parent_clk = clk_get_parent(priv->lcd_clk);
>>  +		parent_rate = clk_get_rate(parent_clk);
>>  +
>>  +		/* LCD Device clock must be 3x the pixel clock for STN panels,
>>  +		 * or 1.5x the pixel clock for TFT panels. To avoid having to
>>  +		 * check for the LCD device clock everytime we do a mode change,
>>  +		 * we set the LCD device clock to the highest rate possible.
>>  +		 */
>>  +		ret = clk_set_rate(priv->lcd_clk, parent_rate);
>>  +		if (ret) {
>>  +			dev_err(dev, "Unable to set LCD clock rate\n");
>>  +			goto err_pixclk_disable;
>>  +		}
>>  +
>>  +		ret = clk_prepare_enable(priv->lcd_clk);
>>  +		if (ret) {
>>  +			dev_err(dev, "Unable to start lcd clock\n");
>>  +			goto err_pixclk_disable;
>>  +		}
>>  +	}
>>  +
>>  +	ret = drm_fbdev_generic_setup(drm, 16);
>>  +	if (ret) {
>>  +		dev_err(dev, "Failed to init fbdev\n");
>>  +		goto err_devclk_disable;
>>  +	}
>>  +
>>  +	ret = drm_dev_register(drm, 0);
>>  +	if (ret) {
>>  +		dev_err(dev, "Failed to register DRM driver\n");
>>  +		goto err_cleanup_fbdev;
>>  +	}
>>  +
>>  +	return 0;
>>  +
>>  +err_cleanup_fbdev:
>>  +	drm_mode_config_cleanup(drm);
>>  +err_devclk_disable:
>>  +	if (priv->lcd_clk)
>>  +		clk_disable_unprepare(priv->lcd_clk);
>>  +err_pixclk_disable:
>>  +	clk_disable_unprepare(priv->pix_clk);
>>  +err_cleanup_vblank:
>>  +	drm_vblank_cleanup(drm);
>>  +err_uninstall_irq:
>>  +	drm_irq_uninstall(drm);
>>  +err_cleanup_encoder:
>>  +	drm_encoder_cleanup(&priv->encoder);
>>  +err_cleanup_connector:
>>  +	drm_connector_cleanup(&priv->connector);
>>  +err_cleanup_crtc:
>>  +	drm_crtc_cleanup(&priv->crtc);
>>  +err_cleanup_plane:
>>  +	drm_plane_cleanup(&priv->primary);
>>  +err_unref_drm:
>>  +	drm_dev_put(drm);
>>  +err_free_dma:
>>  +	dma_free_coherent(dev, sizeof(*priv->framedesc),
>>  +			  priv->framedesc, priv->framedesc_phys);
>>  +	return ret;
>>  +}
>>  +
>>  +static int ingenic_drm_remove(struct platform_device *pdev)
>>  +{
>>  +	struct drm_device *drm = platform_get_drvdata(pdev);
>>  +	struct ingenic_drm *priv = drm->dev_private;
>>  +
>>  +	drm_dev_unregister(drm);
>>  +	drm_mode_config_cleanup(drm);
>>  +
>>  +	if (priv->lcd_clk)
>>  +		clk_disable_unprepare(priv->lcd_clk);
>>  +	clk_disable_unprepare(priv->pix_clk);
>>  +
>>  +	drm_vblank_cleanup(drm);
>>  +	drm_irq_uninstall(drm);
>>  +
>>  +	drm_encoder_cleanup(&priv->encoder);
>>  +	drm_connector_cleanup(&priv->connector);
>>  +	drm_crtc_cleanup(&priv->crtc);
>>  +	drm_plane_cleanup(&priv->primary);
>>  +
>>  +	drm_dev_put(drm);
>>  +
>>  +	dma_free_coherent(&pdev->dev, sizeof(*priv->framedesc),
>>  +			  priv->framedesc, priv->framedesc_phys);
>>  +
>>  +	return 0;
>>  +}
>>  +
>>  +static const struct jz_soc_info jz4740_soc_info = {
>>  +	.needs_dev_clk = true,
>>  +};
>>  +
>>  +static const struct jz_soc_info jz4725b_soc_info = {
>>  +	.needs_dev_clk = false,
>>  +};
>>  +
>>  +static const struct of_device_id ingenic_drm_of_match[] = {
>>  +	{ .compatible = "ingenic,jz4740-drm", .data = &jz4740_soc_info },
>>  +	{ .compatible = "ingenic,jz4725b-drm", .data = &jz4725b_soc_info 
>> },
>>  +	{},
>>  +};
>>  +
>>  +static struct platform_driver ingenic_drm_driver = {
>>  +	.driver = {
>>  +		.name = "ingenic-drm",
>>  +		.of_match_table = of_match_ptr(ingenic_drm_of_match),
>>  +	},
>>  +	.probe = ingenic_drm_probe,
>>  +	.remove = ingenic_drm_remove,
>>  +};
>>  +module_platform_driver(ingenic_drm_driver);
>>  +
>>  +MODULE_AUTHOR("Paul Cercueil <paul@crapouillou.net>");
>>  +MODULE_DESCRIPTION("DRM driver for the Ingenic SoCs\n");
>>  +MODULE_LICENSE("GPL v2");
>>  --
>>  2.11.0
>> 
> 
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

Re: [PATCH 3/3] DRM: Add KMS driver for the Ingenic JZ47xx SoCs

[Daniel Vetter]

On Thu, Mar 14, 2019 at 01:36:59PM +0100, Paul Cercueil wrote:
> Hi Daniel,
> 
> Le ven. 1 mars 2019 à 9:26, Daniel Vetter <daniel@ffwll.ch> a écrit :
> > On Thu, Feb 28, 2019 at 07:07:56PM -0300, Paul Cercueil wrote:
> > >  Add a KMS driver for the Ingenic JZ47xx family of SoCs.
> > >  This driver is meant to replace the aging jz4740-fb driver.
> > > 
> > >  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> > >  Tested-by: Artur Rojek <contact@artur-rojek.eu>
> > 
> > Yay, more fbdev drivers moving to atmic!
> > 
> > Going on vacations for a week now, so just a few quick high-level
> > comments
> > below. Those should make the driver even smaller and cleaner.
> > 
> > >  ---
> > >   drivers/gpu/drm/Kconfig               |   2 +
> > >   drivers/gpu/drm/Makefile              |   1 +
> > >   drivers/gpu/drm/ingenic/Kconfig       |  16 +
> > >   drivers/gpu/drm/ingenic/Makefile      |   1 +
> > >   drivers/gpu/drm/ingenic/ingenic-drm.c | 896
> > > ++++++++++++++++++++++++++++++++++
> > >   5 files changed, 916 insertions(+)
> > >   create mode 100644 drivers/gpu/drm/ingenic/Kconfig
> > >   create mode 100644 drivers/gpu/drm/ingenic/Makefile
> > >   create mode 100644 drivers/gpu/drm/ingenic/ingenic-drm.c
> > > 
> > >  diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> > >  index bd943a71756c..f1929112e559 100644
> > >  --- a/drivers/gpu/drm/Kconfig
> > >  +++ b/drivers/gpu/drm/Kconfig
> > >  @@ -303,6 +303,8 @@ source "drivers/gpu/drm/sti/Kconfig"
> > > 
> > >   source "drivers/gpu/drm/imx/Kconfig"
> > > 
> > >  +source "drivers/gpu/drm/ingenic/Kconfig"
> > >  +
> > >   source "drivers/gpu/drm/v3d/Kconfig"
> > > 
> > >   source "drivers/gpu/drm/vc4/Kconfig"
> > >  diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> > >  index 1ac55c65eac0..9666c0767f44 100644
> > >  --- a/drivers/gpu/drm/Makefile
> > >  +++ b/drivers/gpu/drm/Makefile
> > >  @@ -94,6 +94,7 @@ obj-$(CONFIG_DRM_TEGRA) += tegra/
> > >   obj-$(CONFIG_DRM_STM) += stm/
> > >   obj-$(CONFIG_DRM_STI) += sti/
> > >   obj-$(CONFIG_DRM_IMX) += imx/
> > >  +obj-y			+= ingenic/
> > >   obj-$(CONFIG_DRM_MEDIATEK) += mediatek/
> > >   obj-$(CONFIG_DRM_MESON)	+= meson/
> > >   obj-y			+= i2c/
> > >  diff --git a/drivers/gpu/drm/ingenic/Kconfig
> > > b/drivers/gpu/drm/ingenic/Kconfig
> > >  new file mode 100644
> > >  index 000000000000..d82c3d37ec9c
> > >  --- /dev/null
> > >  +++ b/drivers/gpu/drm/ingenic/Kconfig
> > >  @@ -0,0 +1,16 @@
> > >  +config DRM_INGENIC
> > >  +	tristate "DRM Support for Ingenic SoCs"
> > >  +	depends on MIPS || COMPILE_TEST
> > >  +	depends on DRM
> > >  +	depends on CMA
> > >  +	depends on OF
> > >  +	select DRM_BRIDGE
> > >  +	select DRM_PANEL_BRIDGE
> > >  +	select DRM_KMS_HELPER
> > >  +	select DRM_KMS_CMA_HELPER
> > >  +	select DRM_GEM_CMA_HELPER
> > >  +	select VT_HW_CONSOLE_BINDING if FRAMEBUFFER_CONSOLE
> > >  +	help
> > >  +	  Choose this option for DRM support for the Ingenic SoCs.
> > >  +
> > >  +	  If M is selected the module will be called ingenic-drm.
> > >  diff --git a/drivers/gpu/drm/ingenic/Makefile
> > > b/drivers/gpu/drm/ingenic/Makefile
> > >  new file mode 100644
> > >  index 000000000000..11cac42ce0bb
> > >  --- /dev/null
> > >  +++ b/drivers/gpu/drm/ingenic/Makefile
> > >  @@ -0,0 +1 @@
> > >  +obj-$(CONFIG_DRM_INGENIC) += ingenic-drm.o
> > >  diff --git a/drivers/gpu/drm/ingenic/ingenic-drm.c
> > > b/drivers/gpu/drm/ingenic/ingenic-drm.c
> > >  new file mode 100644
> > >  index 000000000000..18120deea96f
> > >  --- /dev/null
> > >  +++ b/drivers/gpu/drm/ingenic/ingenic-drm.c
> > >  @@ -0,0 +1,896 @@
> > >  +// SPDX-License-Identifier: GPL-2.0
> > >  +//
> > >  +// Ingenic JZ47xx KMS driver
> > >  +//
> > >  +// Copyright (C) 2019, Paul Cercueil <paul@crapouillou.net>
> > >  +
> > >  +#include <linux/clk.h>
> > >  +#include <linux/dma-mapping.h>
> > >  +#include <linux/module.h>
> > >  +#include <linux/platform_device.h>
> > >  +#include <linux/regmap.h>
> > >  +
> > >  +#include <drm/drm_atomic.h>
> > >  +#include <drm/drm_atomic_helper.h>
> > >  +#include <drm/drm_crtc.h>
> > >  +#include <drm/drm_crtc_helper.h>
> > >  +#include <drm/drm_drv.h>
> > >  +#include <drm/drm_gem_cma_helper.h>
> > >  +#include <drm/drm_fb_cma_helper.h>
> > >  +#include <drm/drm_fb_helper.h>
> > >  +#include <drm/drm_fourcc.h>
> > >  +#include <drm/drm_gem_framebuffer_helper.h>
> > >  +#include <drm/drm_irq.h>
> > >  +#include <drm/drm_panel.h>
> > >  +#include <drm/drm_plane.h>
> > >  +#include <drm/drm_plane_helper.h>
> > >  +#include <drm/drm_probe_helper.h>
> > >  +#include <drm/drm_vblank.h>
> > >  +
> > >  +#include <dt-bindings/display/ingenic,drm.h>
> > >  +
> > >  +#include "../drm_internal.h"
> > >  +
> > >  +#define JZ_REG_LCD_CFG		0x00
> > >  +#define JZ_REG_LCD_VSYNC	0x04
> > >  +#define JZ_REG_LCD_HSYNC	0x08
> > >  +#define JZ_REG_LCD_VAT		0x0C
> > >  +#define JZ_REG_LCD_DAH		0x10
> > >  +#define JZ_REG_LCD_DAV		0x14
> > >  +#define JZ_REG_LCD_PS		0x18
> > >  +#define JZ_REG_LCD_CLS		0x1C
> > >  +#define JZ_REG_LCD_SPL		0x20
> > >  +#define JZ_REG_LCD_REV		0x24
> > >  +#define JZ_REG_LCD_CTRL		0x30
> > >  +#define JZ_REG_LCD_STATE	0x34
> > >  +#define JZ_REG_LCD_IID		0x38
> > >  +#define JZ_REG_LCD_DA0		0x40
> > >  +#define JZ_REG_LCD_SA0		0x44
> > >  +#define JZ_REG_LCD_FID0		0x48
> > >  +#define JZ_REG_LCD_CMD0		0x4C
> > >  +#define JZ_REG_LCD_DA1		0x50
> > >  +#define JZ_REG_LCD_SA1		0x54
> > >  +#define JZ_REG_LCD_FID1		0x58
> > >  +#define JZ_REG_LCD_CMD1		0x5C
> > >  +
> > >  +#define JZ_LCD_CFG_SLCD			BIT(31)
> > >  +#define JZ_LCD_CFG_PS_DISABLE		BIT(23)
> > >  +#define JZ_LCD_CFG_CLS_DISABLE		BIT(22)
> > >  +#define JZ_LCD_CFG_SPL_DISABLE		BIT(21)
> > >  +#define JZ_LCD_CFG_REV_DISABLE		BIT(20)
> > >  +#define JZ_LCD_CFG_HSYNCM		BIT(19)
> > >  +#define JZ_LCD_CFG_PCLKM		BIT(18)
> > >  +#define JZ_LCD_CFG_INV			BIT(17)
> > >  +#define JZ_LCD_CFG_SYNC_DIR		BIT(16)
> > >  +#define JZ_LCD_CFG_PS_POLARITY		BIT(15)
> > >  +#define JZ_LCD_CFG_CLS_POLARITY		BIT(14)
> > >  +#define JZ_LCD_CFG_SPL_POLARITY		BIT(13)
> > >  +#define JZ_LCD_CFG_REV_POLARITY		BIT(12)
> > >  +#define JZ_LCD_CFG_HSYNC_ACTIVE_LOW	BIT(11)
> > >  +#define JZ_LCD_CFG_PCLK_FALLING_EDGE	BIT(10)
> > >  +#define JZ_LCD_CFG_DE_ACTIVE_LOW	BIT(9)
> > >  +#define JZ_LCD_CFG_VSYNC_ACTIVE_LOW	BIT(8)
> > >  +#define JZ_LCD_CFG_18_BIT		BIT(7)
> > >  +#define JZ_LCD_CFG_PDW			(BIT(5) | BIT(4))
> > >  +#define JZ_LCD_CFG_MODE_MASK		0xf
> > >  +
> > >  +#define JZ_LCD_VSYNC_VPS_OFFSET		16
> > >  +#define JZ_LCD_VSYNC_VPE_OFFSET		0
> > >  +
> > >  +#define JZ_LCD_HSYNC_HPS_OFFSET		16
> > >  +#define JZ_LCD_HSYNC_HPE_OFFSET		0
> > >  +
> > >  +#define JZ_LCD_VAT_HT_OFFSET		16
> > >  +#define JZ_LCD_VAT_VT_OFFSET		0
> > >  +
> > >  +#define JZ_LCD_DAH_HDS_OFFSET		16
> > >  +#define JZ_LCD_DAH_HDE_OFFSET		0
> > >  +
> > >  +#define JZ_LCD_DAV_VDS_OFFSET		16
> > >  +#define JZ_LCD_DAV_VDE_OFFSET		0
> > >  +
> > >  +#define JZ_LCD_CTRL_BURST_4		(0x0 << 28)
> > >  +#define JZ_LCD_CTRL_BURST_8		(0x1 << 28)
> > >  +#define JZ_LCD_CTRL_BURST_16		(0x2 << 28)
> > >  +#define JZ_LCD_CTRL_RGB555		BIT(27)
> > >  +#define JZ_LCD_CTRL_OFUP		BIT(26)
> > >  +#define JZ_LCD_CTRL_FRC_GRAYSCALE_16	(0x0 << 24)
> > >  +#define JZ_LCD_CTRL_FRC_GRAYSCALE_4	(0x1 << 24)
> > >  +#define JZ_LCD_CTRL_FRC_GRAYSCALE_2	(0x2 << 24)
> > >  +#define JZ_LCD_CTRL_PDD_MASK		(0xff << 16)
> > >  +#define JZ_LCD_CTRL_EOF_IRQ		BIT(13)
> > >  +#define JZ_LCD_CTRL_SOF_IRQ		BIT(12)
> > >  +#define JZ_LCD_CTRL_OFU_IRQ		BIT(11)
> > >  +#define JZ_LCD_CTRL_IFU0_IRQ		BIT(10)
> > >  +#define JZ_LCD_CTRL_IFU1_IRQ		BIT(9)
> > >  +#define JZ_LCD_CTRL_DD_IRQ		BIT(8)
> > >  +#define JZ_LCD_CTRL_QDD_IRQ		BIT(7)
> > >  +#define JZ_LCD_CTRL_REVERSE_ENDIAN	BIT(6)
> > >  +#define JZ_LCD_CTRL_LSB_FISRT		BIT(5)
> > >  +#define JZ_LCD_CTRL_DISABLE		BIT(4)
> > >  +#define JZ_LCD_CTRL_ENABLE		BIT(3)
> > >  +#define JZ_LCD_CTRL_BPP_1		0x0
> > >  +#define JZ_LCD_CTRL_BPP_2		0x1
> > >  +#define JZ_LCD_CTRL_BPP_4		0x2
> > >  +#define JZ_LCD_CTRL_BPP_8		0x3
> > >  +#define JZ_LCD_CTRL_BPP_15_16		0x4
> > >  +#define JZ_LCD_CTRL_BPP_18_24		0x5
> > >  +#define JZ_LCD_CTRL_BPP_MASK		(JZ_LCD_CTRL_RGB555 | (0x7 << 0))
> > >  +
> > >  +#define JZ_LCD_CMD_SOF_IRQ		BIT(31)
> > >  +#define JZ_LCD_CMD_EOF_IRQ		BIT(30)
> > >  +#define JZ_LCD_CMD_ENABLE_PAL		BIT(28)
> > >  +
> > >  +#define JZ_LCD_SYNC_MASK		0x3ff
> > >  +
> > >  +#define JZ_LCD_STATE_EOF_IRQ		BIT(5)
> > >  +#define JZ_LCD_STATE_SOF_IRQ		BIT(4)
> > >  +#define JZ_LCD_STATE_DISABLED		BIT(0)
> > >  +
> > >  +struct ingenic_framedesc {
> > >  +	uint32_t next;
> > >  +	uint32_t addr;
> > >  +	uint32_t id;
> > >  +	uint32_t cmd;
> > >  +} __packed;
> > >  +
> > >  +struct jz_soc_info {
> > >  +	bool needs_dev_clk;
> > >  +};
> > >  +
> > >  +struct ingenic_drm {
> > >  +	struct device *dev;
> > 
> > Embedding is kinda the new thing, since it also prevents people from
> > using
> > devm_ for this, which is (strictly speaking, not that it matters much)
> > the
> > wrong lifetime for drm allocations. There's some patches from Noralf
> > that
> > should land soon to make this a bit more convenient.
> > 
> > >  +	void __iomem *base;
> > >  +	struct regmap *map;
> > >  +	struct clk *lcd_clk, *pix_clk;
> > >  +
> > >  +	u32 lcd_mode;
> > >  +
> > >  +	struct ingenic_framedesc *framedesc;
> > >  +	dma_addr_t framedesc_phys;
> > >  +
> > >  +	struct drm_device *drm;
> > >  +	struct drm_plane primary;
> > >  +	struct drm_crtc crtc;
> > >  +	struct drm_connector connector;
> > >  +	struct drm_encoder encoder;
> > 
> > Any reason you didn't opt for the drm_simple_display_pipe helpers? Since
> > you have just 1:1:1 between crtc/plane/encoder that will allow you to
> > ditch lots of boilerplate.
> > 
> > https://dri.freedesktop.org/docs/drm/gpu/drm-kms-helpers.html#simple-kms-helper-reference
> > 
> > >  +	struct drm_panel *panel;
> > >  +
> > >  +	struct device_node *panel_node;
> > 
> > There's panel bridge helpers which takes a panel and wraps it into a
> > drm_bridge (which you can glue into the simple pipe helpers), giving you
> > the connector for free.
> > 
> > https://dri.freedesktop.org/docs/drm/gpu/drm-kms-helpers.html#panel-bridge-helper-reference
> 
> I'm having some trouble using this; if I use a drm_bridge and attach it
> directly to the encoder, how can I get a pointer to the drm_connector?
> I need it to retrieve the connector->display_info.bus_flags.

Not sure why you need these? bridges also have callbacks to adjust state,
crtc code shouldn't really dig around in connectors.
-Daniel

> 
> Thanks,
> -Paul
> 
> > This should cover all the bigger items, details look already really
> > tidy.
> > 
> > Cheers, Daniel
> > 
> > >  +
> > >  +	unsigned short ps_start, ps_end, cls_start,
> > >  +		       cls_end, spl_start, spl_end, rev_start;
> > >  +};
> > >  +
> > >  +static const uint32_t ingenic_drm_primary_formats[] = {
> > >  +	DRM_FORMAT_XRGB1555,
> > >  +	DRM_FORMAT_RGB565,
> > >  +	DRM_FORMAT_XRGB8888,
> > >  +};
> > >  +
> > >  +static bool ingenic_drm_writeable_reg(struct device *dev, unsigned
> > > int reg)
> > >  +{
> > >  +	switch (reg) {
> > >  +	case JZ_REG_LCD_IID:
> > >  +	case JZ_REG_LCD_SA0:
> > >  +	case JZ_REG_LCD_FID0:
> > >  +	case JZ_REG_LCD_CMD0:
> > >  +	case JZ_REG_LCD_SA1:
> > >  +	case JZ_REG_LCD_FID1:
> > >  +	case JZ_REG_LCD_CMD1:
> > >  +		return false;
> > >  +	default:
> > >  +		return true;
> > >  +	}
> > >  +}
> > >  +
> > >  +static const struct regmap_config ingenic_drm_regmap_config = {
> > >  +	.reg_bits = 32,
> > >  +	.val_bits = 32,
> > >  +	.reg_stride = 4,
> > >  +
> > >  +	.max_register = JZ_REG_LCD_CMD1,
> > >  +	.writeable_reg = ingenic_drm_writeable_reg,
> > >  +};
> > >  +
> > >  +static inline struct ingenic_drm *drm_crtc_get_priv(struct
> > > drm_crtc *crtc)
> > >  +{
> > >  +	return container_of(crtc, struct ingenic_drm, crtc);
> > >  +}
> > >  +
> > >  +static inline struct ingenic_drm *drm_plane_get_priv(struct
> > > drm_plane *plane)
> > >  +{
> > >  +	return container_of(plane, struct ingenic_drm, primary);
> > >  +}
> > >  +
> > >  +static inline struct ingenic_drm *drm_conn_get_priv(struct
> > > drm_connector *conn)
> > >  +{
> > >  +	return container_of(conn, struct ingenic_drm, connector);
> > >  +}
> > >  +
> > >  +static void ingenic_drm_crtc_atomic_enable(struct drm_crtc *crtc,
> > >  +					  struct drm_crtc_state *state)
> > >  +{
> > >  +	struct ingenic_drm *priv = drm_crtc_get_priv(crtc);
> > >  +
> > >  +	regmap_write(priv->map, JZ_REG_LCD_STATE, 0);
> > >  +
> > >  +	regmap_update_bits(priv->map, JZ_REG_LCD_CTRL,
> > >  +			   JZ_LCD_CTRL_ENABLE | JZ_LCD_CTRL_DISABLE,
> > >  +			   JZ_LCD_CTRL_ENABLE);
> > >  +
> > >  +	drm_panel_enable(priv->panel);
> > >  +
> > >  +	drm_crtc_vblank_on(crtc);
> > >  +}
> > >  +
> > >  +static void ingenic_drm_crtc_atomic_disable(struct drm_crtc *crtc,
> > >  +					   struct drm_crtc_state *state)
> > >  +{
> > >  +	struct ingenic_drm *priv = drm_crtc_get_priv(crtc);
> > >  +	unsigned int var;
> > >  +
> > >  +	drm_crtc_vblank_off(crtc);
> > >  +
> > >  +	drm_panel_disable(priv->panel);
> > >  +
> > >  +	regmap_update_bits(priv->map, JZ_REG_LCD_CTRL,
> > >  +			   JZ_LCD_CTRL_DISABLE, JZ_LCD_CTRL_DISABLE);
> > >  +
> > >  +	regmap_read_poll_timeout(priv->map, JZ_REG_LCD_STATE, var,
> > >  +				 var & JZ_LCD_STATE_DISABLED,
> > >  +				 1000, 0);
> > >  +}
> > >  +
> > >  +static inline bool ingenic_drm_lcd_is_special_mode(u32 mode)
> > >  +{
> > >  +	switch (mode) {
> > >  +	case JZ_DRM_LCD_SPECIAL_TFT_1:
> > >  +	case JZ_DRM_LCD_SPECIAL_TFT_2:
> > >  +	case JZ_DRM_LCD_SPECIAL_TFT_3:
> > >  +		return true;
> > >  +	default:
> > >  +		return false;
> > >  +	}
> > >  +}
> > >  +
> > >  +static inline bool ingenic_drm_lcd_is_stn_mode(u32 mode)
> > >  +{
> > >  +	switch (mode) {
> > >  +	case JZ_DRM_LCD_SINGLE_COLOR_STN:
> > >  +	case JZ_DRM_LCD_SINGLE_MONOCHROME_STN:
> > >  +	case JZ_DRM_LCD_DUAL_COLOR_STN:
> > >  +	case JZ_DRM_LCD_DUAL_MONOCHROME_STN:
> > >  +		return true;
> > >  +	default:
> > >  +		return false;
> > >  +	}
> > >  +}
> > >  +
> > >  +static void ingenic_drm_crtc_update_timings(struct ingenic_drm
> > > *priv,
> > >  +					    struct drm_display_mode *mode)
> > >  +{
> > >  +	unsigned int vpe, vds, vde, vt, hpe, hds, hde, ht;
> > >  +
> > >  +	vpe = mode->vsync_end - mode->vsync_start;
> > >  +	vds = mode->vtotal - mode->vsync_start;
> > >  +	vde = vds + mode->vdisplay;
> > >  +	vt = vde + mode->vsync_start - mode->vdisplay;
> > >  +
> > >  +	hpe = mode->hsync_end - mode->hsync_start;
> > >  +	hds = mode->htotal - mode->hsync_start;
> > >  +	hde = hds + mode->hdisplay;
> > >  +	ht = hde + mode->hsync_start - mode->hdisplay;
> > >  +
> > >  +	regmap_write(priv->map, JZ_REG_LCD_VSYNC,
> > >  +		     0 << JZ_LCD_VSYNC_VPS_OFFSET |
> > >  +		     vpe << JZ_LCD_VSYNC_VPE_OFFSET);
> > >  +
> > >  +	regmap_write(priv->map, JZ_REG_LCD_HSYNC,
> > >  +		     0 << JZ_LCD_HSYNC_HPS_OFFSET |
> > >  +		     hpe << JZ_LCD_HSYNC_HPE_OFFSET);
> > >  +
> > >  +	regmap_write(priv->map, JZ_REG_LCD_VAT,
> > >  +		     ht << JZ_LCD_VAT_HT_OFFSET |
> > >  +		     vt << JZ_LCD_VAT_VT_OFFSET);
> > >  +
> > >  +	regmap_write(priv->map, JZ_REG_LCD_DAH,
> > >  +		     hds << JZ_LCD_DAH_HDS_OFFSET |
> > >  +		     hde << JZ_LCD_DAH_HDE_OFFSET);
> > >  +	regmap_write(priv->map, JZ_REG_LCD_DAV,
> > >  +		     vds << JZ_LCD_DAV_VDS_OFFSET |
> > >  +		     vde << JZ_LCD_DAV_VDE_OFFSET);
> > >  +
> > >  +	if (ingenic_drm_lcd_is_special_mode(priv->lcd_mode)) {
> > >  +		regmap_write(priv->map, JZ_REG_LCD_PS, hde << 16 | (hde + 1));
> > >  +		regmap_write(priv->map, JZ_REG_LCD_CLS, hde << 16 | (hde + 1));
> > >  +		regmap_write(priv->map, JZ_REG_LCD_SPL, hpe << 16 | (hpe + 1));
> > >  +		regmap_write(priv->map, JZ_REG_LCD_REV, mode->htotal << 16);
> > >  +	}
> > >  +}
> > >  +
> > >  +static void ingenic_drm_crtc_update_cfg(struct ingenic_drm *priv,
> > >  +					struct drm_display_mode *mode)
> > >  +
> > >  +{
> > >  +	unsigned int cfg = priv->lcd_mode;
> > >  +	u32 bus_flags = priv->connector.display_info.bus_flags;
> > >  +
> > >  +	if (mode->flags & DRM_MODE_FLAG_NHSYNC)
> > >  +		cfg |= JZ_LCD_CFG_HSYNC_ACTIVE_LOW;
> > >  +	if (mode->flags & DRM_MODE_FLAG_NVSYNC)
> > >  +		cfg |= JZ_LCD_CFG_VSYNC_ACTIVE_LOW;
> > >  +	if (bus_flags & DRM_BUS_FLAG_DE_LOW)
> > >  +		cfg |= JZ_LCD_CFG_DE_ACTIVE_LOW;
> > >  +	if (bus_flags & DRM_BUS_FLAG_PIXDATA_NEGEDGE)
> > >  +		cfg |= JZ_LCD_CFG_PCLK_FALLING_EDGE;
> > >  +
> > >  +	if (ingenic_drm_lcd_is_special_mode(priv->lcd_mode)) {
> > >  +		// TODO: Is that valid for all special modes?
> > >  +		cfg |= JZ_LCD_CFG_REV_POLARITY;
> > >  +	} else {
> > >  +		cfg |= JZ_LCD_CFG_PS_DISABLE
> > >  +		    | JZ_LCD_CFG_CLS_DISABLE
> > >  +		    | JZ_LCD_CFG_SPL_DISABLE
> > >  +		    | JZ_LCD_CFG_REV_DISABLE;
> > >  +	}
> > >  +
> > >  +	regmap_write(priv->map, JZ_REG_LCD_CFG, cfg);
> > >  +}
> > >  +
> > >  +static void ingenic_drm_crtc_update_ctrl(struct ingenic_drm *priv,
> > >  +					 unsigned int bpp)
> > >  +{
> > >  +	unsigned int ctrl = JZ_LCD_CTRL_OFUP | JZ_LCD_CTRL_BURST_16;
> > >  +
> > >  +	switch (bpp) {
> > >  +	case 1:
> > >  +		ctrl |= JZ_LCD_CTRL_BPP_1;
> > >  +		break;
> > >  +	case 2:
> > >  +		ctrl |= JZ_LCD_CTRL_BPP_2;
> > >  +		break;
> > >  +	case 4:
> > >  +		ctrl |= JZ_LCD_CTRL_BPP_4;
> > >  +		break;
> > >  +	case 8:
> > >  +		ctrl |= JZ_LCD_CTRL_BPP_8;
> > >  +	break;
> > >  +	case 15:
> > >  +		ctrl |= JZ_LCD_CTRL_RGB555; /* Falltrough */
> > >  +	case 16:
> > >  +		ctrl |= JZ_LCD_CTRL_BPP_15_16;
> > >  +		break;
> > >  +	case 18:
> > >  +	case 24:
> > >  +	case 32:
> > >  +		ctrl |= JZ_LCD_CTRL_BPP_18_24;
> > >  +		break;
> > >  +	default:
> > >  +		break;
> > >  +	}
> > >  +
> > >  +	regmap_update_bits(priv->map, JZ_REG_LCD_CTRL,
> > >  +			   JZ_LCD_CTRL_OFUP | JZ_LCD_CTRL_BURST_16 |
> > >  +			   JZ_LCD_CTRL_BPP_MASK, ctrl);
> > >  +}
> > >  +
> > >  +static int ingenic_drm_crtc_atomic_check(struct drm_crtc *crtc,
> > >  +					 struct drm_crtc_state *state)
> > >  +{
> > >  +	struct ingenic_drm *priv = drm_crtc_get_priv(crtc);
> > >  +	long rate;
> > >  +
> > >  +	if (!drm_atomic_crtc_needs_modeset(state))
> > >  +		return 0;
> > >  +
> > >  +	rate = clk_round_rate(priv->pix_clk,
> > >  +			      state->adjusted_mode.clock * 1000);
> > >  +	if (rate < 0)
> > >  +		return rate;
> > >  +
> > >  +	return 0;
> > >  +}
> > >  +
> > >  +static void ingenic_drm_crtc_atomic_flush(struct drm_crtc *crtc,
> > >  +					  struct drm_crtc_state *oldstate)
> > >  +{
> > >  +	struct ingenic_drm *priv = drm_crtc_get_priv(crtc);
> > >  +	struct drm_crtc_state *state = crtc->state;
> > >  +	struct drm_pending_vblank_event *event = state->event;
> > >  +	struct drm_framebuffer *drm_fb = crtc->primary->state->fb;
> > >  +	const struct drm_format_info *finfo;
> > >  +	unsigned int width, height;
> > >  +
> > >  +	if (drm_atomic_crtc_needs_modeset(state)) {
> > >  +		finfo = drm_format_info(drm_fb->format->format);
> > >  +		width = state->adjusted_mode.hdisplay;
> > >  +		height = state->adjusted_mode.vdisplay;
> > >  +
> > >  +		ingenic_drm_crtc_update_timings(priv, &state->mode);
> > >  +
> > >  +		ingenic_drm_crtc_update_ctrl(priv, finfo->depth);
> > >  +		ingenic_drm_crtc_update_cfg(priv, &state->adjusted_mode);
> > >  +
> > >  +		clk_set_rate(priv->pix_clk, state->adjusted_mode.clock * 1000);
> > >  +
> > >  +		regmap_write(priv->map, JZ_REG_LCD_DA0, priv->framedesc->next);
> > >  +	}
> > >  +
> > >  +	if (event) {
> > >  +		state->event = NULL;
> > >  +
> > >  +		spin_lock_irq(&crtc->dev->event_lock);
> > >  +		if (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 void ingenic_drm_plane_atomic_update(struct drm_plane
> > > *plane,
> > >  +					    struct drm_plane_state *oldstate)
> > >  +{
> > >  +	struct ingenic_drm *priv = drm_plane_get_priv(plane);
> > >  +	struct drm_plane_state *state = plane->state;
> > >  +	const struct drm_format_info *finfo;
> > >  +	unsigned int width, height;
> > >  +
> > >  +	finfo = drm_format_info(state->fb->format->format);
> > >  +	width = state->crtc->state->adjusted_mode.hdisplay;
> > >  +	height = state->crtc->state->adjusted_mode.vdisplay;
> > >  +
> > >  +	priv->framedesc->addr = drm_fb_cma_get_gem_addr(state->fb, state,
> > > 0);
> > >  +
> > >  +	priv->framedesc->cmd = width * height * ((finfo->depth + 7) / 8)
> > > / 4;
> > >  +	priv->framedesc->cmd |= JZ_LCD_CMD_EOF_IRQ;
> > >  +}
> > >  +
> > >  +static enum drm_connector_status
> > >  +ingenic_drm_conn_detect(struct drm_connector *conn, bool force)
> > >  +{
> > >  +	struct ingenic_drm *priv = drm_conn_get_priv(conn);
> > >  +
> > >  +	if (priv->panel) {
> > >  +		drm_panel_attach(priv->panel, conn);
> > >  +
> > >  +		return connector_status_connected;
> > >  +	}
> > >  +
> > >  +	return connector_status_disconnected;
> > >  +}
> > >  +
> > >  +static void ingenic_drm_conn_destroy(struct drm_connector *conn)
> > >  +{
> > >  +	struct ingenic_drm *priv = drm_conn_get_priv(conn);
> > >  +
> > >  +	if (priv->panel)
> > >  +		drm_panel_detach(priv->panel);
> > >  +
> > >  +	drm_connector_cleanup(conn);
> > >  +}
> > >  +
> > >  +static int ingenic_drm_conn_get_modes(struct drm_connector *conn)
> > >  +{
> > >  +	struct ingenic_drm *priv = drm_conn_get_priv(conn);
> > >  +
> > >  +	if (priv->panel)
> > >  +		return priv->panel->funcs->get_modes(priv->panel);
> > >  +
> > >  +	return 0;
> > >  +}
> > >  +
> > >  +static irqreturn_t ingenic_drm_irq_handler(int irq, void *arg)
> > >  +{
> > >  +	struct drm_device *drm = arg;
> > >  +	struct ingenic_drm *priv = drm->dev_private;
> > >  +	unsigned int state;
> > >  +
> > >  +	regmap_read(priv->map, JZ_REG_LCD_STATE, &state);
> > >  +
> > >  +	regmap_update_bits(priv->map, JZ_REG_LCD_STATE,
> > >  +			   JZ_LCD_STATE_EOF_IRQ, 0);
> > >  +
> > >  +	if (state & JZ_LCD_STATE_EOF_IRQ)
> > >  +		drm_crtc_handle_vblank(&priv->crtc);
> > >  +
> > >  +	return IRQ_HANDLED;
> > >  +}
> > >  +
> > >  +static int ingenic_drm_enable_vblank(struct drm_crtc *crtc)
> > >  +{
> > >  +	struct ingenic_drm *priv = drm_crtc_get_priv(crtc);
> > >  +
> > >  +	regmap_update_bits(priv->map, JZ_REG_LCD_CTRL,
> > >  +			   JZ_LCD_CTRL_EOF_IRQ, JZ_LCD_CTRL_EOF_IRQ);
> > >  +
> > >  +	return 0;
> > >  +}
> > >  +
> > >  +static void ingenic_drm_disable_vblank(struct drm_crtc *crtc)
> > >  +{
> > >  +	struct ingenic_drm *priv = drm_crtc_get_priv(crtc);
> > >  +
> > >  +	regmap_update_bits(priv->map, JZ_REG_LCD_CTRL,
> > > JZ_LCD_CTRL_EOF_IRQ, 0);
> > >  +}
> > >  +
> > >  +DEFINE_DRM_GEM_CMA_FOPS(ingenic_drm_fops);
> > >  +
> > >  +static struct drm_driver ingenic_drm_driver_data = {
> > >  +	.driver_features	= DRIVER_MODESET | DRIVER_GEM | DRIVER_PRIME
> > >  +				| DRIVER_ATOMIC | DRIVER_HAVE_IRQ,
> > >  +	.name			= "ingenic-drm",
> > >  +	.desc			= "DRM module for Ingenic SoCs",
> > >  +	.date			= "20190228",
> > >  +	.major			= 1,
> > >  +	.minor			= 0,
> > >  +	.patchlevel		= 0,
> > >  +
> > >  +	.fops			= &ingenic_drm_fops,
> > >  +
> > >  +	.dumb_create		= drm_gem_cma_dumb_create,
> > >  +	.gem_free_object_unlocked = drm_gem_cma_free_object,
> > >  +	.gem_vm_ops		= &drm_gem_cma_vm_ops,
> > >  +
> > >  +	.prime_handle_to_fd	= drm_gem_prime_handle_to_fd,
> > >  +	.prime_fd_to_handle	= drm_gem_prime_fd_to_handle,
> > >  +	.gem_prime_import	= drm_gem_prime_import,
> > >  +	.gem_prime_export	= drm_gem_prime_export,
> > >  +	.gem_prime_get_sg_table	= drm_gem_cma_prime_get_sg_table,
> > >  +	.gem_prime_import_sg_table = drm_gem_cma_prime_import_sg_table,
> > >  +	.gem_prime_vmap		= drm_gem_cma_prime_vmap,
> > >  +	.gem_prime_vunmap	= drm_gem_cma_prime_vunmap,
> > >  +	.gem_prime_mmap		= drm_gem_cma_prime_mmap,
> > >  +
> > >  +	.irq_handler		= ingenic_drm_irq_handler,
> > >  +};
> > >  +
> > >  +static const struct drm_plane_funcs
> > > ingenic_drm_primary_plane_funcs = {
> > >  +	.update_plane		= drm_atomic_helper_update_plane,
> > >  +	.disable_plane		= drm_atomic_helper_disable_plane,
> > >  +	.reset			= drm_atomic_helper_plane_reset,
> > >  +
> > >  +	.atomic_duplicate_state	= drm_atomic_helper_plane_duplicate_state,
> > >  +	.atomic_destroy_state	= drm_atomic_helper_plane_destroy_state,
> > >  +};
> > >  +
> > >  +static const struct drm_crtc_funcs ingenic_drm_crtc_funcs = {
> > >  +	.reset			= drm_atomic_helper_crtc_reset,
> > >  +	.set_config		= drm_atomic_helper_set_config,
> > >  +	.page_flip		= drm_atomic_helper_page_flip,
> > >  +
> > >  +	.atomic_duplicate_state	= drm_atomic_helper_crtc_duplicate_state,
> > >  +	.atomic_destroy_state	= drm_atomic_helper_crtc_destroy_state,
> > >  +
> > >  +	.enable_vblank		= ingenic_drm_enable_vblank,
> > >  +	.disable_vblank		= ingenic_drm_disable_vblank,
> > >  +
> > >  +	.gamma_set		= drm_atomic_helper_legacy_gamma_set,
> > >  +};
> > >  +
> > >  +static const struct drm_connector_funcs ingenic_drm_conn_funcs = {
> > >  +	.fill_modes		= drm_helper_probe_single_connector_modes,
> > >  +	.detect			= ingenic_drm_conn_detect,
> > >  +	.destroy		= ingenic_drm_conn_destroy,
> > >  +	.reset			= drm_atomic_helper_connector_reset,
> > >  +	.atomic_duplicate_state	=
> > > drm_atomic_helper_connector_duplicate_state,
> > >  +	.atomic_destroy_state	= drm_atomic_helper_connector_destroy_state,
> > >  +};
> > >  +
> > >  +static const struct drm_plane_helper_funcs
> > > ingenic_drm_plane_helper_funcs = {
> > >  +	.atomic_update		= ingenic_drm_plane_atomic_update,
> > >  +	.prepare_fb		= drm_gem_fb_prepare_fb,
> > >  +};
> > >  +
> > >  +static const struct drm_crtc_helper_funcs
> > > ingenic_drm_crtc_helper_funcs = {
> > >  +	.atomic_enable		= ingenic_drm_crtc_atomic_enable,
> > >  +	.atomic_disable		= ingenic_drm_crtc_atomic_disable,
> > >  +	.atomic_flush		= ingenic_drm_crtc_atomic_flush,
> > >  +	.atomic_check		= ingenic_drm_crtc_atomic_check,
> > >  +};
> > >  +
> > >  +static const struct drm_connector_helper_funcs
> > > ingenic_drm_conn_helper_funcs = {
> > >  +	.get_modes		= ingenic_drm_conn_get_modes,
> > >  +};
> > >  +
> > >  +static const struct drm_mode_config_funcs
> > > ingenic_drm_mode_config_funcs = {
> > >  +	.fb_create		= drm_gem_fb_create,
> > >  +	.output_poll_changed	= drm_fb_helper_output_poll_changed,
> > >  +	.atomic_check		= drm_atomic_helper_check,
> > >  +	.atomic_commit		= drm_atomic_helper_commit,
> > >  +};
> > >  +
> > >  +static const struct drm_encoder_funcs ingenic_drm_encoder_funcs = {
> > >  +	.destroy		= drm_encoder_cleanup,
> > >  +};
> > >  +
> > >  +static int ingenic_drm_probe(struct platform_device *pdev)
> > >  +{
> > >  +	const struct jz_soc_info *soc_info;
> > >  +	struct device *dev = &pdev->dev;
> > >  +	struct ingenic_drm *priv;
> > >  +	struct clk *parent_clk;
> > >  +	struct drm_device *drm;
> > >  +	struct resource *mem;
> > >  +	void __iomem *base;
> > >  +	long parent_rate;
> > >  +	int ret, irq;
> > >  +
> > >  +	soc_info = device_get_match_data(dev);
> > >  +	if (!soc_info)
> > >  +		return -EINVAL;
> > >  +
> > >  +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > >  +	if (!priv)
> > >  +		return -ENOMEM;
> > >  +
> > >  +	priv->dev = dev;
> > >  +
> > >  +	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > >  +	priv->base = base = devm_ioremap_resource(dev, mem);
> > >  +	if (IS_ERR(base))
> > >  +		return PTR_ERR(base);
> > >  +
> > >  +	irq = platform_get_irq(pdev, 0);
> > >  +	if (irq < 0) {
> > >  +		dev_err(dev, "Failed to get platform irq\n");
> > >  +		return -ENOENT;
> > >  +	}
> > >  +
> > >  +	priv->map = devm_regmap_init_mmio(dev, base,
> > >  +					  &ingenic_drm_regmap_config);
> > >  +	if (IS_ERR(priv->map)) {
> > >  +		dev_err(dev, "Failed to create regmap\n");
> > >  +		return PTR_ERR(priv->map);
> > >  +	}
> > >  +
> > >  +	if (soc_info->needs_dev_clk) {
> > >  +		priv->lcd_clk = devm_clk_get(dev, "lcd");
> > >  +		if (IS_ERR(priv->lcd_clk)) {
> > >  +			dev_err(dev, "Failed to get lcd clock\n");
> > >  +			return PTR_ERR(priv->lcd_clk);
> > >  +		}
> > >  +	}
> > >  +
> > >  +	priv->pix_clk = devm_clk_get(dev, "lcd_pclk");
> > >  +	if (IS_ERR(priv->pix_clk)) {
> > >  +		dev_err(dev, "Failed to get pixel clock\n");
> > >  +		return PTR_ERR(priv->pix_clk);
> > >  +	}
> > >  +
> > >  +	priv->panel_node = of_parse_phandle(dev->of_node,
> > > "ingenic,panel", 0);
> > >  +	if (!priv->panel_node) {
> > >  +		DRM_INFO("No panel found");
> > >  +	} else {
> > >  +		priv->panel = of_drm_find_panel(priv->panel_node);
> > >  +		of_node_put(priv->panel_node);
> > >  +
> > >  +		if (IS_ERR(priv->panel)) {
> > >  +			if (PTR_ERR(priv->panel) == -EPROBE_DEFER)
> > >  +				return -EPROBE_DEFER;
> > >  +
> > >  +			priv->panel = NULL;
> > >  +		} else {
> > >  +			ret = drm_panel_prepare(priv->panel);
> > >  +			if (ret && ret != -ENOSYS)
> > >  +				return ret;
> > >  +		}
> > >  +	}
> > >  +
> > >  +	if (priv->panel) {
> > >  +		ret = device_property_read_u32(dev, "ingenic,lcd-mode",
> > >  +					       &priv->lcd_mode);
> > >  +		if (ret) {
> > >  +			dev_err(dev, "Unable to read ingenic,lcd-mode property\n");
> > >  +			return ret;
> > >  +		}
> > >  +	}
> > >  +
> > >  +	priv->framedesc = dma_alloc_coherent(dev,
> > > sizeof(*priv->framedesc),
> > >  +					     &priv->framedesc_phys, GFP_KERNEL);
> > >  +	if (!priv->framedesc)
> > >  +		return -ENOMEM;
> > >  +
> > >  +	priv->framedesc->next = priv->framedesc_phys;
> > >  +	priv->framedesc->id = 0xdeafbead;
> > >  +
> > >  +	drm = drm_dev_alloc(&ingenic_drm_driver_data, dev);
> > >  +	if (IS_ERR(drm)) {
> > >  +		ret = PTR_ERR(drm);
> > >  +		goto err_free_dma;
> > >  +	}
> > >  +
> > >  +	priv->drm = drm;
> > >  +
> > >  +	drm_mode_config_init(drm);
> > >  +	drm->mode_config.min_width = 0;
> > >  +	drm->mode_config.min_height = 0;
> > >  +	drm->mode_config.max_width = 800;
> > >  +	drm->mode_config.max_height = 600;
> > >  +	drm->mode_config.funcs = &ingenic_drm_mode_config_funcs;
> > >  +
> > >  +	drm_plane_helper_add(&priv->primary,
> > > &ingenic_drm_plane_helper_funcs);
> > >  +
> > >  +	ret = drm_universal_plane_init(drm, &priv->primary,
> > >  +				       0, &ingenic_drm_primary_plane_funcs,
> > >  +				       ingenic_drm_primary_formats,
> > >  +				       ARRAY_SIZE(ingenic_drm_primary_formats),
> > >  +				       NULL, DRM_PLANE_TYPE_PRIMARY, NULL);
> > >  +	if (ret) {
> > >  +		dev_err(dev, "Failed to register primary plane: %i\n", ret);
> > >  +		goto err_unref_drm;
> > >  +	}
> > >  +
> > >  +	drm_crtc_helper_add(&priv->crtc, &ingenic_drm_crtc_helper_funcs);
> > >  +
> > >  +	ret = drm_crtc_init_with_planes(drm, &priv->crtc, &priv->primary,
> > >  +					NULL, &ingenic_drm_crtc_funcs, NULL);
> > >  +	if (ret) {
> > >  +		dev_err(dev, "Failed to init CRTC: %i\n", ret);
> > >  +		goto err_cleanup_plane;
> > >  +	}
> > >  +
> > >  +	drm_connector_helper_add(&priv->connector,
> > >  +				 &ingenic_drm_conn_helper_funcs);
> > >  +	ret = drm_connector_init(drm, &priv->connector,
> > >  +				 &ingenic_drm_conn_funcs,
> > >  +				 DRM_MODE_CONNECTOR_Unknown);
> > >  +	if (ret) {
> > >  +		dev_err(dev, "Failed to init connector: %i\n", ret);
> > >  +		goto err_cleanup_crtc;
> > >  +	}
> > >  +
> > >  +	priv->encoder.possible_crtcs = 1;
> > >  +
> > >  +	ret = drm_encoder_init(drm, &priv->encoder,
> > > &ingenic_drm_encoder_funcs,
> > >  +			       DRM_MODE_ENCODER_NONE, NULL);
> > >  +	if (ret) {
> > >  +		dev_err(dev, "Failed to init encoder: %i\n", ret);
> > >  +		goto err_cleanup_connector;
> > >  +	}
> > >  +
> > >  +	drm_connector_attach_encoder(&priv->connector, &priv->encoder);
> > >  +
> > >  +	platform_set_drvdata(pdev, drm);
> > >  +	priv->drm = drm;
> > >  +	drm->dev_private = priv;
> > >  +
> > >  +	ret = drm_irq_install(drm, irq);
> > >  +	if (ret) {
> > >  +		dev_err(dev, "Unable to install IRQ handler\n");
> > >  +		goto err_cleanup_encoder;
> > >  +	}
> > >  +
> > >  +	ret = drm_vblank_init(drm, 1);
> > >  +	if (ret) {
> > >  +		dev_err(dev, "Failed calling drm_vblank_init()\n");
> > >  +		goto err_uninstall_irq;
> > >  +	}
> > >  +
> > >  +	drm_mode_config_reset(drm);
> > >  +
> > >  +	ret = clk_prepare_enable(priv->pix_clk);
> > >  +	if (ret) {
> > >  +		dev_err(dev, "Unable to start pixel clock\n");
> > >  +		goto err_cleanup_vblank;
> > >  +	}
> > >  +
> > >  +	if (priv->lcd_clk) {
> > >  +		parent_clk = clk_get_parent(priv->lcd_clk);
> > >  +		parent_rate = clk_get_rate(parent_clk);
> > >  +
> > >  +		/* LCD Device clock must be 3x the pixel clock for STN panels,
> > >  +		 * or 1.5x the pixel clock for TFT panels. To avoid having to
> > >  +		 * check for the LCD device clock everytime we do a mode change,
> > >  +		 * we set the LCD device clock to the highest rate possible.
> > >  +		 */
> > >  +		ret = clk_set_rate(priv->lcd_clk, parent_rate);
> > >  +		if (ret) {
> > >  +			dev_err(dev, "Unable to set LCD clock rate\n");
> > >  +			goto err_pixclk_disable;
> > >  +		}
> > >  +
> > >  +		ret = clk_prepare_enable(priv->lcd_clk);
> > >  +		if (ret) {
> > >  +			dev_err(dev, "Unable to start lcd clock\n");
> > >  +			goto err_pixclk_disable;
> > >  +		}
> > >  +	}
> > >  +
> > >  +	ret = drm_fbdev_generic_setup(drm, 16);
> > >  +	if (ret) {
> > >  +		dev_err(dev, "Failed to init fbdev\n");
> > >  +		goto err_devclk_disable;
> > >  +	}
> > >  +
> > >  +	ret = drm_dev_register(drm, 0);
> > >  +	if (ret) {
> > >  +		dev_err(dev, "Failed to register DRM driver\n");
> > >  +		goto err_cleanup_fbdev;
> > >  +	}
> > >  +
> > >  +	return 0;
> > >  +
> > >  +err_cleanup_fbdev:
> > >  +	drm_mode_config_cleanup(drm);
> > >  +err_devclk_disable:
> > >  +	if (priv->lcd_clk)
> > >  +		clk_disable_unprepare(priv->lcd_clk);
> > >  +err_pixclk_disable:
> > >  +	clk_disable_unprepare(priv->pix_clk);
> > >  +err_cleanup_vblank:
> > >  +	drm_vblank_cleanup(drm);
> > >  +err_uninstall_irq:
> > >  +	drm_irq_uninstall(drm);
> > >  +err_cleanup_encoder:
> > >  +	drm_encoder_cleanup(&priv->encoder);
> > >  +err_cleanup_connector:
> > >  +	drm_connector_cleanup(&priv->connector);
> > >  +err_cleanup_crtc:
> > >  +	drm_crtc_cleanup(&priv->crtc);
> > >  +err_cleanup_plane:
> > >  +	drm_plane_cleanup(&priv->primary);
> > >  +err_unref_drm:
> > >  +	drm_dev_put(drm);
> > >  +err_free_dma:
> > >  +	dma_free_coherent(dev, sizeof(*priv->framedesc),
> > >  +			  priv->framedesc, priv->framedesc_phys);
> > >  +	return ret;
> > >  +}
> > >  +
> > >  +static int ingenic_drm_remove(struct platform_device *pdev)
> > >  +{
> > >  +	struct drm_device *drm = platform_get_drvdata(pdev);
> > >  +	struct ingenic_drm *priv = drm->dev_private;
> > >  +
> > >  +	drm_dev_unregister(drm);
> > >  +	drm_mode_config_cleanup(drm);
> > >  +
> > >  +	if (priv->lcd_clk)
> > >  +		clk_disable_unprepare(priv->lcd_clk);
> > >  +	clk_disable_unprepare(priv->pix_clk);
> > >  +
> > >  +	drm_vblank_cleanup(drm);
> > >  +	drm_irq_uninstall(drm);
> > >  +
> > >  +	drm_encoder_cleanup(&priv->encoder);
> > >  +	drm_connector_cleanup(&priv->connector);
> > >  +	drm_crtc_cleanup(&priv->crtc);
> > >  +	drm_plane_cleanup(&priv->primary);
> > >  +
> > >  +	drm_dev_put(drm);
> > >  +
> > >  +	dma_free_coherent(&pdev->dev, sizeof(*priv->framedesc),
> > >  +			  priv->framedesc, priv->framedesc_phys);
> > >  +
> > >  +	return 0;
> > >  +}
> > >  +
> > >  +static const struct jz_soc_info jz4740_soc_info = {
> > >  +	.needs_dev_clk = true,
> > >  +};
> > >  +
> > >  +static const struct jz_soc_info jz4725b_soc_info = {
> > >  +	.needs_dev_clk = false,
> > >  +};
> > >  +
> > >  +static const struct of_device_id ingenic_drm_of_match[] = {
> > >  +	{ .compatible = "ingenic,jz4740-drm", .data = &jz4740_soc_info },
> > >  +	{ .compatible = "ingenic,jz4725b-drm", .data = &jz4725b_soc_info
> > > },
> > >  +	{},
> > >  +};
> > >  +
> > >  +static struct platform_driver ingenic_drm_driver = {
> > >  +	.driver = {
> > >  +		.name = "ingenic-drm",
> > >  +		.of_match_table = of_match_ptr(ingenic_drm_of_match),
> > >  +	},
> > >  +	.probe = ingenic_drm_probe,
> > >  +	.remove = ingenic_drm_remove,
> > >  +};
> > >  +module_platform_driver(ingenic_drm_driver);
> > >  +
> > >  +MODULE_AUTHOR("Paul Cercueil <paul@crapouillou.net>");
> > >  +MODULE_DESCRIPTION("DRM driver for the Ingenic SoCs\n");
> > >  +MODULE_LICENSE("GPL v2");
> > >  --
> > >  2.11.0
> > > 
> > 
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [5.2][PATCH 0/3] Ingenic JZ47xx KMS driver

[Ezequiel Garcia]

On Mon, 2019-03-11 at 12:45 +0100, Paul Cercueil wrote:
> Hi Ezequiel,
> 
> On Mon, Mar 11, 2019 at 1:02 AM, Ezequiel Garcia 
> <ezequiel@collabora.com> wrote:
> > On Thu, 2019-02-28 at 19:07 -0300, Paul Cercueil wrote:
> > >  Hi,
> > > 
> > >  This is a first attempt at a KMS driver for the JZ47xx MIPS SoCs by
> > >  Ingenic. It is aimed to replace the aging jz4740-fb driver.
> > > 
> > >  The driver will later be updated with new features (overlays, TV-out
> > >  etc.), that's why I didn't go with the simple/tiny DRM driver.
> > > 
> > >  The driver has been tested on the Ben Nanonote (JZ4740) and the
> > >  RetroMini RS-90 (JZ4725B) handheld gaming console.
> > > 
> > 
> > Does this support JZ4780? Or otherwise,
> > is there any similarity with JZ4780 display controller?
> > 
> > Thanks,
> > Eze
> 
> The JZ4780 LCD controller would in theory work with this driver, but
> to test on the CI20 you'd need to add support for HDMI first.
> 

After reviewing a bit this driver and the JZ480 one on the CI20 repo,
there seems to be some differences.

To begin with, the JZ480 apparently has a new 8-word descriptor.

The HDMI looks pretty much a Synopsys DW HDMI, mostly compatible
with the dw-hdmi encoder.

I'll try to put together some patches to enable JZ4780 support.
Won't happen anytime soon, so if anyone wants to give it
a shot, be my guest :-)

Regards,
Eze