* [PATCH v3 0/5] drm/ingenic: Add the X1600 display controller and the HiBy R1 panel
@ 2026-11-06 11:19 Olivier Louvignes
2026-11-06 11:19 ` [PATCH v3 1/5] dt-bindings: vendor-prefixes: Add HiBy Olivier Louvignes
` (4 more replies)
0 siblings, 5 replies; 9+ messages in thread
From: Olivier Louvignes @ 2026-11-06 11:19 UTC (permalink / raw)
To: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, Neil Armstrong
Cc: David Airlie, Simona Vetter, Jessica Zhang, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Paul Cercueil,
H . Nikolaus Schaller, dri-devel, devicetree, linux-mips,
linux-kernel, Olivier Louvignes
The display controller of the Ingenic X1600 is not the one ingenic-drm
drives: it fetches frames through descriptors and keeps its interface and
timings in register blocks of their own. It is the X2000's DPU without
the composer. This series adds a binding and a driver for it, then the
panel of the HiBy R1, an X1600 audio player: 480x800 parallel RGB, set up
over a 3-wire SPI bus.
The driver covers what the R1 uses and what could be tested: parallel RGB
panels on a 24-bit bus and one full-screen RGB565 plane. The composer,
smart panels and other pixel formats are left out.
Two things come from the vendor kernel rather than the manual. A flip
also writes SRD_CHAIN_START, which the manual doesn't describe for a
running controller; flips without it were not tried. The panel's set-up
sequence is the vendor's too: HiBy calls the panel LG35583, its maker and
controller are unknown, hence the hiby prefix.
Changes in v3:
- The compatible is ingenic,x1600-dpu, and the binding and the driver say
"display controller" rather than "LCD controller" (Nikolaus). The X2000
and X2600 manuals call this block the DPU, and the X1600 manual does too
in its register and clock names. There is no generic ingenic,dpu
fallback, as writing-bindings.rst asks for SoC compatibles: an X2000
binding can fall back to the X1600's. The driver keeps its file name;
splitting it for the X2000's composer and interfaces belongs with
X2000 support.
- 2/5: no $nodename and no clock-names; the driver takes its one clock
unnamed (Krzysztof). The example's node is display-controller@.
- 2/5: there is no IPU in front of this controller (Krzysztof asked on v2,
Paul answered): it fetches its own frames, so the output port is its
only link.
- 4/5: reset-gpios, backlight and port dropped, panel-common has them
(Krzysztof). The example's endpoint points at the controller's new
label.
- Krzysztof's Acked-by on 1/5 and Reviewed-by on 4/5.
- Rebased on drm-misc-next after -rc1.
Changes in v2, all in 3/5 and all found by the Sashiko review of v1:
- Commits wait for flip_done instead of a vblank before the planes are
cleaned up. A flip's event waits until the controller reports reading
the new descriptor, which can be a frame after the vblank that v1's
default commit_tail waited for, so the old framebuffer could be freed
while still scanned out.
- A CRTC being enabled sends its event from atomic_enable(), once the
controller runs; v1 sent it from atomic_flush(), which runs first.
- A plane that is not visible is rejected: the controller always scans
out a whole frame, so v1 would have read past the end of a small
framebuffer placed off-screen.
- atomic_disable() returns early when atomic_enable() failed to start
the pixel clock, instead of touching an unclocked controller.
Tested on a HiBy R1: the fbdev console and a player drawing through
fbdev, blanking and unblanking, 60 page flips in 0.97 s at about 61 Hz,
and the colour order (RGB order swaps red and blue on this panel). For
v2 also: 120 fbdev pans in 1.95 s (one flip per frame, none timing out),
three blank/unblank cycles, and a 64x64 framebuffer set on the primary
plane just past the right edge through SETPLANE, which v1 accepted and
v2 refuses with -EINVAL, while a full-screen one is still accepted. v3
was run on the R1 with its node changed to match: the driver binds to
ingenic,x1600-dpu with its clock unnamed, and the player draws.
The X1600 has no device tree upstream yet; its support is being posted
one subsystem at a time. The R1's backlight uses the X1600 PWM driver
posted separately, now at v3:
https://lore.kernel.org/r/20261102201325.29507-1-olivier@mgcrea.io
v2: https://lore.kernel.org/r/20261025191640.13592-1-olivier@mgcrea.io
v1: https://lore.kernel.org/r/20261025085816.60405-1-olivier@mgcrea.io
Olivier Louvignes (5):
dt-bindings: vendor-prefixes: Add HiBy
dt-bindings: display: Add the Ingenic X1600 display controller
drm/ingenic: Add a driver for the X1600 display controller
dt-bindings: display: panel: Add the HiBy LG35583
drm/panel: Add the HiBy LG35583
.../bindings/display/ingenic,x1600-dpu.yaml | 61 ++
.../bindings/display/panel/hiby,lg35583.yaml | 67 ++
.../devicetree/bindings/vendor-prefixes.yaml | 2 +
MAINTAINERS | 12 +
drivers/gpu/drm/Makefile | 1 +
drivers/gpu/drm/ingenic/Kconfig | 22 +
drivers/gpu/drm/ingenic/Makefile | 1 +
drivers/gpu/drm/ingenic/ingenic-x1600-drm.c | 744 ++++++++++++++++++
drivers/gpu/drm/panel/Kconfig | 10 +
drivers/gpu/drm/panel/Makefile | 1 +
drivers/gpu/drm/panel/panel-hiby-lg35583.c | 342 ++++++++
11 files changed, 1263 insertions(+)
create mode 100644 Documentation/devicetree/bindings/display/ingenic,x1600-dpu.yaml
create mode 100644 Documentation/devicetree/bindings/display/panel/hiby,lg35583.yaml
create mode 100644 drivers/gpu/drm/ingenic/ingenic-x1600-drm.c
create mode 100644 drivers/gpu/drm/panel/panel-hiby-lg35583.c
base-commit: 41ecf852c07391687beb129e12a231760b5d783f
--
2.39.5
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v3 1/5] dt-bindings: vendor-prefixes: Add HiBy
2026-11-06 11:19 [PATCH v3 0/5] drm/ingenic: Add the X1600 display controller and the HiBy R1 panel Olivier Louvignes
@ 2026-11-06 11:19 ` Olivier Louvignes
2026-11-06 11:19 ` [PATCH v3 2/5] dt-bindings: display: Add the Ingenic X1600 display controller Olivier Louvignes
` (3 subsequent siblings)
4 siblings, 0 replies; 9+ messages in thread
From: Olivier Louvignes @ 2026-11-06 11:19 UTC (permalink / raw)
To: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, Neil Armstrong
Cc: David Airlie, Simona Vetter, Jessica Zhang, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Paul Cercueil,
H . Nikolaus Schaller, dri-devel, devicetree, linux-mips,
linux-kernel, Olivier Louvignes, Krzysztof Kozlowski
HiBy makes digital audio players, among them the R1.
Signed-off-by: Olivier Louvignes <olivier@mgcrea.io>
Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>
---
Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++
1 file changed, 2 insertions(+)
diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
index 7602034374d7..0bbab57d6c25 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
+++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
@@ -732,6 +732,8 @@ patternProperties:
description: HEAD acoustics
"^hechuang,.*":
description: Shenzhen Hechuang Intelligent Co.
+ "^hiby,.*":
+ description: Guangzhou HiBy Technology Co., Ltd.
"^hideep,.*":
description: HiDeep Inc.
"^himax,.*":
--
2.39.5
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v3 2/5] dt-bindings: display: Add the Ingenic X1600 display controller
2026-11-06 11:19 [PATCH v3 0/5] drm/ingenic: Add the X1600 display controller and the HiBy R1 panel Olivier Louvignes
2026-11-06 11:19 ` [PATCH v3 1/5] dt-bindings: vendor-prefixes: Add HiBy Olivier Louvignes
@ 2026-11-06 11:19 ` Olivier Louvignes
2026-11-06 11:19 ` [PATCH v3 3/5] drm/ingenic: Add a driver for the " Olivier Louvignes
` (2 subsequent siblings)
4 siblings, 0 replies; 9+ messages in thread
From: Olivier Louvignes @ 2026-11-06 11:19 UTC (permalink / raw)
To: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, Neil Armstrong
Cc: David Airlie, Simona Vetter, Jessica Zhang, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Paul Cercueil,
H . Nikolaus Schaller, dri-devel, devicetree, linux-mips,
linux-kernel, Olivier Louvignes
The X1600's display controller fetches frames through descriptors and
keeps its interface and timings in register blocks of their own (X1600
programming manual, chapter 10), so it shares nothing with the
controllers ingenic,lcd.yaml describes. Its registers are those of the
X2000's display controller without the composer, which the X2000 and
X2600 manuals call the DPU, as the X1600 manual does in its register and
clock names: hence ingenic,x1600-dpu, which an X2000 binding can extend.
Signed-off-by: Olivier Louvignes <olivier@mgcrea.io>
---
v3:
- ingenic,x1600-dpu, titled "display controller (DPU)" (Nikolaus).
- No $nodename, no clock-names (Krzysztof); example node
display-controller@.
.../bindings/display/ingenic,x1600-dpu.yaml | 61 +++++++++++++++++++
MAINTAINERS | 5 ++
2 files changed, 66 insertions(+)
create mode 100644 Documentation/devicetree/bindings/display/ingenic,x1600-dpu.yaml
diff --git a/Documentation/devicetree/bindings/display/ingenic,x1600-dpu.yaml b/Documentation/devicetree/bindings/display/ingenic,x1600-dpu.yaml
new file mode 100644
index 000000000000..0e9f4442e4b9
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/ingenic,x1600-dpu.yaml
@@ -0,0 +1,61 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/ingenic,x1600-dpu.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Ingenic X1600 display controller (DPU)
+
+maintainers:
+ - Olivier Louvignes <olivier@mgcrea.io>
+
+description:
+ The display controller of the Ingenic X1600 SoC, the DPU of the X2000
+ without its composer. It fetches frames through descriptors in memory
+ and drives a parallel RGB panel or a smart panel; its registers have
+ nothing in common with the controllers described in ingenic,lcd.yaml.
+
+properties:
+ compatible:
+ const: ingenic,x1600-dpu
+
+ reg:
+ maxItems: 1
+
+ interrupts:
+ maxItems: 1
+
+ clocks:
+ items:
+ - description: The pixel clock, whose gate also gates the controller
+
+ port:
+ $ref: /schemas/graph.yaml#/properties/port
+ description: The output, to the panel or bridge.
+
+required:
+ - compatible
+ - reg
+ - interrupts
+ - clocks
+ - port
+
+additionalProperties: false
+
+examples:
+ - |
+ display-controller@13050000 {
+ compatible = "ingenic,x1600-dpu";
+ reg = <0x13050000 0x10000>;
+
+ interrupt-parent = <&intc>;
+ interrupts = <31>;
+
+ clocks = <&cgu 16>;
+
+ port {
+ dpu_out: endpoint {
+ remote-endpoint = <&panel_in>;
+ };
+ };
+ };
diff --git a/MAINTAINERS b/MAINTAINERS
index c10342aa32f3..a5160860ae26 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -13176,6 +13176,11 @@ F: include/linux/mfd/ingenic-tcu.h
F: sound/soc/codecs/jz47*
F: sound/soc/jz4740/
+INGENIC X1600 DRM DRIVER
+M: Olivier Louvignes <olivier@mgcrea.io>
+S: Maintained
+F: Documentation/devicetree/bindings/display/ingenic,x1600-dpu.yaml
+
INJOINIC IP5xxx POWER BANK IC DRIVER
M: Samuel Holland <samuel@sholland.org>
S: Maintained
--
2.39.5
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v3 3/5] drm/ingenic: Add a driver for the X1600 display controller
2026-11-06 11:19 [PATCH v3 0/5] drm/ingenic: Add the X1600 display controller and the HiBy R1 panel Olivier Louvignes
2026-11-06 11:19 ` [PATCH v3 1/5] dt-bindings: vendor-prefixes: Add HiBy Olivier Louvignes
2026-11-06 11:19 ` [PATCH v3 2/5] dt-bindings: display: Add the Ingenic X1600 display controller Olivier Louvignes
@ 2026-11-06 11:19 ` Olivier Louvignes
2026-11-06 13:34 ` Paul Cercueil
2026-11-06 11:19 ` [PATCH v3 4/5] dt-bindings: display: panel: Add the HiBy LG35583 Olivier Louvignes
2026-11-06 11:20 ` [PATCH v3 5/5] drm/panel: " Olivier Louvignes
4 siblings, 1 reply; 9+ messages in thread
From: Olivier Louvignes @ 2026-11-06 11:19 UTC (permalink / raw)
To: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, Neil Armstrong
Cc: David Airlie, Simona Vetter, Jessica Zhang, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Paul Cercueil,
H . Nikolaus Schaller, dri-devel, devicetree, linux-mips,
linux-kernel, Olivier Louvignes
The X1600's display controller has none of the registers ingenic-drm
programs: it fetches each frame through a descriptor whose address it
takes from SRD_CHAIN_ADDR, and keeps its interface, TFT timings and smart
panel set-up in separate register blocks (X1600 programming manual,
chapter 10; the X2000's controller without the composer). This driver
handles parallel RGB panels on a 24-bit bus, with one full-screen RGB565
plane, and takes the colour order (RGB or BGR) from the panel's bus
format.
Each descriptor points at itself, so a frame repeats until a flip hands
the controller the other one. The controller takes it "at appropriate
time", so a flip's event waits until the descriptor-site register
reports the new descriptor at the end of a frame, and a commit waits for
that event rather than for a vblank before the old framebuffer is
released: the controller can keep reading it for a frame after the flip.
Tested on a HiBy R1 with its 480x800 panel: the fbdev console and
r1player, blanking and unblanking, and 60 fbdev pans in 0.97 s, one flip
per frame at about 61 Hz, none timing out.
Signed-off-by: Olivier Louvignes <olivier@mgcrea.io>
---
v3:
- ingenic,x1600-dpu; the clock taken unnamed, as the binding no longer
has clock-names (Krzysztof).
- "display controller" in the Kconfig prompt, the header and the
descriptions.
v2:
- A commit_tail that waits for flip_done, not for a vblank, before the
planes are cleaned up.
- A CRTC being enabled sends its event once the controller runs, not
from atomic_flush() before atomic_enable().
- Reject a plane that is not visible.
- atomic_disable() leaves the controller alone when atomic_enable()
could not start the clock.
MAINTAINERS | 1 +
drivers/gpu/drm/Makefile | 1 +
drivers/gpu/drm/ingenic/Kconfig | 22 +
drivers/gpu/drm/ingenic/Makefile | 1 +
drivers/gpu/drm/ingenic/ingenic-x1600-drm.c | 744 ++++++++++++++++++++
5 files changed, 769 insertions(+)
create mode 100644 drivers/gpu/drm/ingenic/ingenic-x1600-drm.c
diff --git a/MAINTAINERS b/MAINTAINERS
index a5160860ae26..5d992133e0d5 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -13180,6 +13180,7 @@ INGENIC X1600 DRM DRIVER
M: Olivier Louvignes <olivier@mgcrea.io>
S: Maintained
F: Documentation/devicetree/bindings/display/ingenic,x1600-dpu.yaml
+F: drivers/gpu/drm/ingenic/ingenic-x1600-drm.c
INJOINIC IP5xxx POWER BANK IC DRIVER
M: Samuel Holland <samuel@sholland.org>
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 2c6bc093a923..37dc781761d6 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -206,6 +206,7 @@ obj-$(CONFIG_DRM_STM) += stm/
obj-$(CONFIG_DRM_STI) += sti/
obj-y += imx/
obj-$(CONFIG_DRM_INGENIC) += ingenic/
+obj-$(CONFIG_DRM_INGENIC_X1600) += ingenic/
obj-$(CONFIG_DRM_LOGICVC) += logicvc/
obj-$(CONFIG_DRM_MEDIATEK) += mediatek/
obj-$(CONFIG_DRM_MESON) += meson/
diff --git a/drivers/gpu/drm/ingenic/Kconfig b/drivers/gpu/drm/ingenic/Kconfig
index 987ed42f6f7a..85ac9ca62d7d 100644
--- a/drivers/gpu/drm/ingenic/Kconfig
+++ b/drivers/gpu/drm/ingenic/Kconfig
@@ -20,6 +20,28 @@ config DRM_INGENIC
If M is selected the module will be called ingenic-drm.
+config DRM_INGENIC_X1600
+ tristate "DRM support for the Ingenic X1600 display controller"
+ depends on MIPS || COMPILE_TEST
+ depends on DRM
+ depends on OF
+ depends on COMMON_CLK
+ select APERTURE_HELPERS
+ select DRM_BRIDGE
+ select DRM_CLIENT_SELECTION
+ select DRM_PANEL_BRIDGE
+ select DRM_KMS_HELPER
+ select DRM_DISPLAY_HELPER
+ select DRM_BRIDGE_CONNECTOR
+ select DRM_GEM_DMA_HELPER
+ help
+ Choose this option for DRM support for the display controller of
+ the Ingenic X1600 SoC. It drives parallel RGB panels on a 24-bit
+ bus from one full-screen RGB565 plane; the controller's smart panel
+ interface is not supported.
+
+ If M is selected the module will be called ingenic-x1600-drm.
+
if DRM_INGENIC
config DRM_INGENIC_IPU
diff --git a/drivers/gpu/drm/ingenic/Makefile b/drivers/gpu/drm/ingenic/Makefile
index f10cc1c5a5f2..90072ad13158 100644
--- a/drivers/gpu/drm/ingenic/Makefile
+++ b/drivers/gpu/drm/ingenic/Makefile
@@ -2,3 +2,4 @@ obj-$(CONFIG_DRM_INGENIC) += ingenic-drm.o
ingenic-drm-y = ingenic-drm-drv.o
ingenic-drm-$(CONFIG_DRM_INGENIC_IPU) += ingenic-ipu.o
obj-$(CONFIG_DRM_INGENIC_DW_HDMI) += ingenic-dw-hdmi.o
+obj-$(CONFIG_DRM_INGENIC_X1600) += ingenic-x1600-drm.o
diff --git a/drivers/gpu/drm/ingenic/ingenic-x1600-drm.c b/drivers/gpu/drm/ingenic/ingenic-x1600-drm.c
new file mode 100644
index 000000000000..e69c968ebcd9
--- /dev/null
+++ b/drivers/gpu/drm/ingenic/ingenic-x1600-drm.c
@@ -0,0 +1,744 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * DRM driver for the display controller (DPU) of the Ingenic X1600
+ *
+ * Copyright (c) 2026 Olivier Louvignes <olivier@mgcrea.io>
+ *
+ * The X1600's display controller (X1600 programming manual, chapter 10) is
+ * the simple read channel of the X2000's, without its composer. It shares no
+ * registers with the JZ47xx and X1000 controllers that ingenic-drm drives: it
+ * fetches each frame through a descriptor whose address is in SRD_CHAIN_ADDR,
+ * and its interface and timings sit in register blocks of their own. It
+ * drives parallel RGB (TFT) panels and smart panels; this driver handles
+ * parallel RGB panels on a 24-bit bus, from RGB565 framebuffers.
+ */
+
+#include <linux/aperture.h>
+#include <linux/bitfield.h>
+#include <linux/bits.h>
+#include <linux/clk.h>
+#include <linux/completion.h>
+#include <linux/dma-mapping.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/media-bus-format.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/spinlock.h>
+
+#include <drm/clients/drm_client_setup.h>
+#include <drm/drm_atomic.h>
+#include <drm/drm_atomic_helper.h>
+#include <drm/drm_bridge.h>
+#include <drm/drm_bridge_connector.h>
+#include <drm/drm_crtc.h>
+#include <drm/drm_drv.h>
+#include <drm/drm_encoder.h>
+#include <drm/drm_fb_dma_helper.h>
+#include <drm/drm_fbdev_dma.h>
+#include <drm/drm_fourcc.h>
+#include <drm/drm_framebuffer.h>
+#include <drm/drm_gem_dma_helper.h>
+#include <drm/drm_gem_framebuffer_helper.h>
+#include <drm/drm_of.h>
+#include <drm/drm_plane.h>
+#include <drm/drm_print.h>
+#include <drm/drm_probe_helper.h>
+#include <drm/drm_vblank.h>
+
+#define X1600_LCD_SRD_CHAIN_ADDR 0x1000
+#define X1600_LCD_SRD_CHAIN_START 0x1004
+#define X1600_LCD_CTRL 0x2000
+#define X1600_LCD_ST 0x2004
+#define X1600_LCD_CLR_ST 0x2008
+#define X1600_LCD_INTC 0x200c
+#define X1600_LCD_INT_FLAG 0x2010
+#define X1600_LCD_COM_CONFIG 0x2014
+#define X1600_LCD_CHAIN_SITE 0x2204
+#define X1600_LCD_DISP_COM 0x8000
+#define X1600_LCD_TFT_HSYNC 0x9000
+#define X1600_LCD_TFT_VSYNC 0x9004
+#define X1600_LCD_TFT_HDE 0x9008
+#define X1600_LCD_TFT_VDE 0x900c
+#define X1600_LCD_TFT_TRAN_CFG 0x9010
+
+/* CTRL: stop the simple read channel at the end of the frame. */
+#define X1600_LCD_CTRL_GEN_STP_SRD BIT(4)
+
+/* ST; CLR_ST (write 1 to clear), INTC and INT_FLAG have the same bits. */
+#define X1600_LCD_ST_WORKING BIT(0)
+#define X1600_LCD_ST_STOP_SRD_ACK BIT(7)
+#define X1600_LCD_ST_TFT_UNDR BIT(8)
+#define X1600_LCD_ST_DISP_END BIT(17) /* a frame was displayed */
+
+#define X1600_LCD_COM_CONFIG_BURST_LEN GENMASK(5, 4)
+#define X1600_LCD_BURST_LEN_32 3
+
+#define X1600_LCD_DISP_COM_IF GENMASK(1, 0)
+#define X1600_LCD_DISP_COM_IF_TFT 1
+#define X1600_LCD_DISP_COM_DITHER_EN BIT(4)
+#define X1600_LCD_DISP_COM_TFT_CLKGATE BIT(5)
+#define X1600_LCD_DISP_COM_SLCD_CLKGATE BIT(6)
+
+/* TFT_HSYNC, TFT_VSYNC: sync length and total; TFT_HDE, TFT_VDE: active area. */
+#define X1600_LCD_TIMING_START GENMASK(27, 16)
+#define X1600_LCD_TIMING_END GENMASK(11, 0)
+
+#define X1600_LCD_TFT_COLOR_EVEN GENMASK(21, 19)
+#define X1600_LCD_TFT_COLOR_ODD GENMASK(18, 16)
+#define X1600_LCD_TFT_PIX_CLK_INV BIT(10)
+#define X1600_LCD_TFT_DE_DL BIT(9)
+#define X1600_LCD_TFT_HSYNC_DL BIT(8)
+#define X1600_LCD_TFT_VSYNC_DL BIT(7)
+#define X1600_LCD_TFT_MODE GENMASK(2, 0)
+#define X1600_LCD_TFT_MODE_24BIT 0
+#define X1600_LCD_TFT_COLOR_RGB 0
+#define X1600_LCD_TFT_COLOR_BGR 2
+
+#define X1600_LCD_DESC_CFG_FMT GENMASK(22, 19)
+#define X1600_LCD_DESC_FMT_RGB565 2
+/* An interrupt at the end of the frame: DISP_END needs this and INTC set. */
+#define X1600_LCD_DESC_CTRL_EOD_MSK BIT(17)
+
+#define X1600_LCD_MAX_PIXELS (1280 * 720)
+/* A smaller frame breaks a continuous descriptor chain (manual, 10.7). */
+#define X1600_LCD_MIN_PIXELS 500
+#define X1600_LCD_STOP_TIMEOUT_MS 100
+
+/*
+ * The controller reads a descriptor at the start of each frame and then
+ * follows its next field. Each descriptor points at itself, so a frame
+ * repeats until SRD_CHAIN_ADDR names another one.
+ */
+struct x1600_lcd_desc {
+ u32 next;
+ u32 addr;
+ u32 stride; /* in pixels */
+ u32 cfg;
+ u32 ctrl;
+} __aligned(64);
+
+struct x1600_lcd {
+ struct drm_device drm;
+ struct drm_plane plane;
+ struct drm_crtc crtc;
+ struct drm_encoder encoder;
+
+ void __iomem *base;
+ struct clk *clk;
+
+ struct x1600_lcd_desc *desc;
+ dma_addr_t desc_phys;
+ unsigned int cur; /* the descriptor last handed over */
+ bool running;
+ struct completion stopped;
+ /* Serialises the read-modify-writes of INTC. */
+ spinlock_t irq_lock;
+
+ /* Under drm.event_lock: sent once the controller reads event_desc. */
+ struct drm_pending_vblank_event *event;
+ u32 event_desc;
+
+ u32 bus_flags;
+ u32 color_order;
+};
+
+static const u32 x1600_lcd_formats[] = {
+ DRM_FORMAT_RGB565,
+};
+
+static struct x1600_lcd *to_x1600_lcd(struct drm_device *drm)
+{
+ return container_of(drm, struct x1600_lcd, drm);
+}
+
+static dma_addr_t x1600_lcd_desc_addr(struct x1600_lcd *lcd, unsigned int i)
+{
+ return lcd->desc_phys + i * sizeof(*lcd->desc);
+}
+
+static void x1600_lcd_irq_enable(struct x1600_lcd *lcd, u32 mask, bool enable)
+{
+ unsigned long flags;
+ u32 val;
+
+ spin_lock_irqsave(&lcd->irq_lock, flags);
+ val = readl(lcd->base + X1600_LCD_INTC);
+ if (enable)
+ val |= mask;
+ else
+ val &= ~mask;
+ /* Seen on the R1: a bit still set when it is masked keeps the line up. */
+ writel(mask, lcd->base + X1600_LCD_CLR_ST);
+ writel(val, lcd->base + X1600_LCD_INTC);
+ spin_unlock_irqrestore(&lcd->irq_lock, flags);
+}
+
+/* Stops at the end of the frame; false if the controller did not report it. */
+static bool x1600_lcd_stop(struct x1600_lcd *lcd)
+{
+ unsigned long left;
+
+ reinit_completion(&lcd->stopped);
+ x1600_lcd_irq_enable(lcd, X1600_LCD_ST_STOP_SRD_ACK, true);
+ writel(X1600_LCD_CTRL_GEN_STP_SRD, lcd->base + X1600_LCD_CTRL);
+ left = wait_for_completion_timeout(&lcd->stopped,
+ msecs_to_jiffies(X1600_LCD_STOP_TIMEOUT_MS));
+ x1600_lcd_irq_enable(lcd, X1600_LCD_ST_STOP_SRD_ACK, false);
+ return left;
+}
+
+static u32 x1600_lcd_timing(unsigned int start, unsigned int end)
+{
+ return FIELD_PREP(X1600_LCD_TIMING_START, start) |
+ FIELD_PREP(X1600_LCD_TIMING_END, end);
+}
+
+/* Sends the commit's event once the controller has read the new descriptor. */
+static void x1600_lcd_queue_event(struct x1600_lcd *lcd, struct drm_crtc_state *crtc_state)
+{
+ struct drm_pending_vblank_event *event = crtc_state->event;
+ struct drm_crtc *crtc = &lcd->crtc;
+
+ if (!event)
+ return;
+
+ crtc_state->event = NULL;
+
+ /*
+ * The controller takes a new descriptor "at appropriate time" (manual,
+ * 10.6.4): wait until it reports reading the one just handed over.
+ */
+ spin_lock_irq(&crtc->dev->event_lock);
+ if (lcd->running && !WARN_ON(lcd->event) && drm_crtc_vblank_get(crtc) == 0) {
+ lcd->event = event;
+ lcd->event_desc = x1600_lcd_desc_addr(lcd, lcd->cur);
+ } else {
+ drm_crtc_send_vblank_event(crtc, event);
+ }
+ spin_unlock_irq(&crtc->dev->event_lock);
+}
+
+static void x1600_lcd_crtc_atomic_enable(struct drm_crtc *crtc,
+ struct drm_atomic_commit *state)
+{
+ struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
+ struct x1600_lcd *lcd = to_x1600_lcd(crtc->dev);
+ struct drm_display_mode *mode = &crtc->state->adjusted_mode;
+ unsigned int hde = mode->htotal - mode->hsync_start;
+ unsigned int vde = mode->vtotal - mode->vsync_start;
+ u32 val;
+ int ret;
+
+ ret = clk_set_rate(lcd->clk, mode->crtc_clock * 1000);
+ if (ret)
+ dev_warn(lcd->drm.dev, "Unable to set the pixel clock: %d\n", ret);
+ ret = clk_prepare_enable(lcd->clk);
+ if (ret) {
+ dev_err(lcd->drm.dev, "Unable to start the pixel clock: %d\n", ret);
+ x1600_lcd_queue_event(lcd, crtc_state);
+ return;
+ }
+
+ writel(readl(lcd->base + X1600_LCD_ST), lcd->base + X1600_LCD_CLR_ST);
+ val = readl(lcd->base + X1600_LCD_COM_CONFIG) & ~X1600_LCD_COM_CONFIG_BURST_LEN;
+ writel(val | FIELD_PREP(X1600_LCD_COM_CONFIG_BURST_LEN, X1600_LCD_BURST_LEN_32),
+ lcd->base + X1600_LCD_COM_CONFIG);
+
+ /* In the manual's order: timings, output format, then the interface. */
+ writel(x1600_lcd_timing(mode->hsync_end - mode->hsync_start, mode->htotal),
+ lcd->base + X1600_LCD_TFT_HSYNC);
+ writel(x1600_lcd_timing(mode->vsync_end - mode->vsync_start, mode->vtotal),
+ lcd->base + X1600_LCD_TFT_VSYNC);
+ writel(x1600_lcd_timing(hde, hde + mode->hdisplay),
+ lcd->base + X1600_LCD_TFT_HDE);
+ writel(x1600_lcd_timing(vde, vde + mode->vdisplay),
+ lcd->base + X1600_LCD_TFT_VDE);
+
+ val = FIELD_PREP(X1600_LCD_TFT_COLOR_EVEN, lcd->color_order) |
+ FIELD_PREP(X1600_LCD_TFT_COLOR_ODD, lcd->color_order) |
+ FIELD_PREP(X1600_LCD_TFT_MODE, X1600_LCD_TFT_MODE_24BIT);
+ if (lcd->bus_flags & DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE)
+ val |= X1600_LCD_TFT_PIX_CLK_INV;
+ if (lcd->bus_flags & DRM_BUS_FLAG_DE_LOW)
+ val |= X1600_LCD_TFT_DE_DL;
+ /* The *_DL bits set the level outside the pulse. */
+ if (mode->flags & DRM_MODE_FLAG_NHSYNC)
+ val |= X1600_LCD_TFT_HSYNC_DL;
+ if (mode->flags & DRM_MODE_FLAG_NVSYNC)
+ val |= X1600_LCD_TFT_VSYNC_DL;
+ writel(val, lcd->base + X1600_LCD_TFT_TRAN_CFG);
+
+ /* No dithering: it only applies to 888 sources. */
+ val = readl(lcd->base + X1600_LCD_DISP_COM);
+ val &= ~(X1600_LCD_DISP_COM_IF | X1600_LCD_DISP_COM_DITHER_EN);
+ val |= FIELD_PREP(X1600_LCD_DISP_COM_IF, X1600_LCD_DISP_COM_IF_TFT) |
+ X1600_LCD_DISP_COM_TFT_CLKGATE | X1600_LCD_DISP_COM_SLCD_CLKGATE;
+ writel(val, lcd->base + X1600_LCD_DISP_COM);
+
+ writel(x1600_lcd_desc_addr(lcd, lcd->cur), lcd->base + X1600_LCD_SRD_CHAIN_ADDR);
+ writel(1, lcd->base + X1600_LCD_SRD_CHAIN_START);
+ lcd->running = true;
+
+ drm_crtc_vblank_on(crtc);
+ x1600_lcd_queue_event(lcd, crtc_state);
+}
+
+static void x1600_lcd_crtc_atomic_disable(struct drm_crtc *crtc,
+ struct drm_atomic_commit *state)
+{
+ struct x1600_lcd *lcd = to_x1600_lcd(crtc->dev);
+
+ /* atomic_enable() could not start the clock: nothing runs. */
+ if (!lcd->running)
+ return;
+
+ if (!x1600_lcd_stop(lcd))
+ dev_warn(lcd->drm.dev, "The controller did not stop\n");
+ lcd->running = false;
+
+ spin_lock_irq(&crtc->dev->event_lock);
+ if (lcd->event) {
+ drm_crtc_send_vblank_event(crtc, lcd->event);
+ drm_crtc_vblank_put(crtc);
+ lcd->event = NULL;
+ }
+ spin_unlock_irq(&crtc->dev->event_lock);
+
+ drm_crtc_vblank_off(crtc);
+ clk_disable_unprepare(lcd->clk);
+}
+
+static int x1600_lcd_crtc_atomic_check(struct drm_crtc *crtc,
+ struct drm_atomic_commit *state)
+{
+ struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
+
+ /* Only when enabled: drm_atomic_helper_shutdown() disables both at once. */
+ if (!crtc_state->enable)
+ return 0;
+
+ return drm_atomic_helper_check_crtc_primary_plane(crtc_state);
+}
+
+static void x1600_lcd_crtc_atomic_flush(struct drm_crtc *crtc,
+ struct drm_atomic_commit *state)
+{
+ struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
+
+ /* The planes come first: a CRTC being enabled sends it once started. */
+ if (crtc_state->active && drm_atomic_crtc_needs_modeset(crtc_state))
+ return;
+
+ x1600_lcd_queue_event(to_x1600_lcd(crtc->dev), crtc_state);
+}
+
+static enum drm_mode_status
+x1600_lcd_crtc_mode_valid(struct drm_crtc *crtc, const struct drm_display_mode *mode)
+{
+ unsigned int pixels = mode->hdisplay * mode->vdisplay;
+
+ if (mode->htotal > FIELD_MAX(X1600_LCD_TIMING_END) ||
+ mode->vtotal > FIELD_MAX(X1600_LCD_TIMING_END))
+ return MODE_BAD;
+
+ /* The sync pulses' end points must not be 0. */
+ if (mode->hsync_end == mode->hsync_start)
+ return MODE_HSYNC_NARROW;
+ if (mode->vsync_end == mode->vsync_start)
+ return MODE_VSYNC_NARROW;
+
+ if (pixels > X1600_LCD_MAX_PIXELS)
+ return MODE_MEM;
+ if (pixels < X1600_LCD_MIN_PIXELS)
+ return MODE_BAD;
+
+ return MODE_OK;
+}
+
+static int x1600_lcd_enable_vblank(struct drm_crtc *crtc)
+{
+ struct x1600_lcd *lcd = to_x1600_lcd(crtc->dev);
+
+ x1600_lcd_irq_enable(lcd, X1600_LCD_ST_DISP_END, true);
+ return 0;
+}
+
+static void x1600_lcd_disable_vblank(struct drm_crtc *crtc)
+{
+ struct x1600_lcd *lcd = to_x1600_lcd(crtc->dev);
+
+ x1600_lcd_irq_enable(lcd, X1600_LCD_ST_DISP_END, false);
+}
+
+static irqreturn_t x1600_lcd_irq(int irq, void *data)
+{
+ struct x1600_lcd *lcd = data;
+ u32 st = readl(lcd->base + X1600_LCD_INT_FLAG);
+
+ if (!st)
+ return IRQ_NONE;
+
+ writel(st, lcd->base + X1600_LCD_CLR_ST);
+ if (st & X1600_LCD_ST_STOP_SRD_ACK)
+ complete(&lcd->stopped);
+
+ if (st & X1600_LCD_ST_DISP_END) {
+ drm_crtc_handle_vblank(&lcd->crtc);
+
+ spin_lock(&lcd->drm.event_lock);
+ if (lcd->event &&
+ readl(lcd->base + X1600_LCD_CHAIN_SITE) == lcd->event_desc) {
+ drm_crtc_send_vblank_event(&lcd->crtc, lcd->event);
+ drm_crtc_vblank_put(&lcd->crtc);
+ lcd->event = NULL;
+ }
+ spin_unlock(&lcd->drm.event_lock);
+ }
+
+ return IRQ_HANDLED;
+}
+
+static int x1600_lcd_plane_atomic_check(struct drm_plane *plane,
+ struct drm_atomic_commit *state)
+{
+ struct drm_plane_state *plane_state = drm_atomic_get_new_plane_state(state, plane);
+ struct drm_crtc_state *crtc_state;
+ int ret;
+
+ if (!plane_state->crtc)
+ return 0;
+
+ crtc_state = drm_atomic_get_new_crtc_state(state, plane_state->crtc);
+ if (WARN_ON(!crtc_state))
+ return -EINVAL;
+
+ /* The plane covers the whole screen: no scaling, no position. */
+ ret = drm_atomic_helper_check_plane_state(plane_state, crtc_state,
+ DRM_PLANE_NO_SCALING,
+ DRM_PLANE_NO_SCALING,
+ false, true);
+ if (ret)
+ return ret;
+
+ /* The controller scans out a whole frame from the framebuffer. */
+ if (crtc_state->enable && !plane_state->visible)
+ return -EINVAL;
+
+ return 0;
+}
+
+static void x1600_lcd_plane_atomic_update(struct drm_plane *plane,
+ struct drm_atomic_commit *state)
+{
+ struct x1600_lcd *lcd = to_x1600_lcd(plane->dev);
+ struct drm_plane_state *plane_state = drm_atomic_get_new_plane_state(state, plane);
+ struct drm_framebuffer *fb = plane_state->fb;
+ unsigned int next = lcd->running ? !lcd->cur : lcd->cur;
+ struct x1600_lcd_desc *desc = &lcd->desc[next];
+
+ if (!fb)
+ return;
+
+ desc->next = x1600_lcd_desc_addr(lcd, next);
+ desc->addr = drm_fb_dma_get_gem_addr(fb, plane_state, 0);
+ desc->stride = fb->pitches[0] / fb->format->cpp[0];
+ desc->cfg = FIELD_PREP(X1600_LCD_DESC_CFG_FMT, X1600_LCD_DESC_FMT_RGB565);
+ desc->ctrl = X1600_LCD_DESC_CTRL_EOD_MSK;
+
+ if (!lcd->running)
+ return;
+
+ /*
+ * The manual hands over a descriptor by writing SRD_CHAIN_ADDR; the
+ * vendor kernel also sets SRD_CHAIN_START, and so does this.
+ */
+ wmb();
+ writel(x1600_lcd_desc_addr(lcd, next), lcd->base + X1600_LCD_SRD_CHAIN_ADDR);
+ writel(1, lcd->base + X1600_LCD_SRD_CHAIN_START);
+ lcd->cur = next;
+}
+
+/*
+ * Panels with 18 data lines take the top six bits of each colour of the
+ * 888 mode: in the 666 mode, the HiBy R1's panel shows wrong colours.
+ */
+static int x1600_lcd_color_order(const struct drm_display_info *info)
+{
+ if (!info->num_bus_formats)
+ return X1600_LCD_TFT_COLOR_RGB;
+
+ switch (info->bus_formats[0]) {
+ case MEDIA_BUS_FMT_RGB666_1X18:
+ case MEDIA_BUS_FMT_RGB888_1X24:
+ return X1600_LCD_TFT_COLOR_RGB;
+ case MEDIA_BUS_FMT_BGR666_1X18:
+ case MEDIA_BUS_FMT_BGR888_1X24:
+ return X1600_LCD_TFT_COLOR_BGR;
+ default:
+ return -EINVAL;
+ }
+}
+
+static int x1600_lcd_encoder_atomic_check(struct drm_encoder *encoder,
+ struct drm_crtc_state *crtc_state,
+ struct drm_connector_state *conn_state)
+{
+ const struct drm_display_info *info = &conn_state->connector->display_info;
+
+ if (x1600_lcd_color_order(info) < 0) {
+ drm_dbg_kms(encoder->dev, "Unsupported bus format 0x%04x\n",
+ info->bus_formats[0]);
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static void x1600_lcd_encoder_atomic_mode_set(struct drm_encoder *encoder,
+ struct drm_crtc_state *crtc_state,
+ struct drm_connector_state *conn_state)
+{
+ struct x1600_lcd *lcd = to_x1600_lcd(encoder->dev);
+ const struct drm_display_info *info = &conn_state->connector->display_info;
+
+ lcd->bus_flags = info->bus_flags;
+ lcd->color_order = x1600_lcd_color_order(info);
+}
+
+static const struct drm_plane_funcs x1600_lcd_plane_funcs = {
+ .update_plane = drm_atomic_helper_update_plane,
+ .disable_plane = drm_atomic_helper_disable_plane,
+ .atomic_create_state = drm_atomic_helper_plane_create_state,
+ .destroy = drm_plane_cleanup,
+ .atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state,
+ .atomic_destroy_state = drm_atomic_helper_plane_destroy_state,
+};
+
+static const struct drm_plane_helper_funcs x1600_lcd_plane_helper_funcs = {
+ .atomic_check = x1600_lcd_plane_atomic_check,
+ .atomic_update = x1600_lcd_plane_atomic_update,
+};
+
+static const struct drm_crtc_funcs x1600_lcd_crtc_funcs = {
+ .set_config = drm_atomic_helper_set_config,
+ .page_flip = drm_atomic_helper_page_flip,
+ .atomic_create_state = drm_atomic_helper_crtc_create_state,
+ .destroy = drm_crtc_cleanup,
+ .atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state,
+ .atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
+ .enable_vblank = x1600_lcd_enable_vblank,
+ .disable_vblank = x1600_lcd_disable_vblank,
+};
+
+static const struct drm_crtc_helper_funcs x1600_lcd_crtc_helper_funcs = {
+ .atomic_check = x1600_lcd_crtc_atomic_check,
+ .atomic_enable = x1600_lcd_crtc_atomic_enable,
+ .atomic_disable = x1600_lcd_crtc_atomic_disable,
+ .atomic_flush = x1600_lcd_crtc_atomic_flush,
+ .mode_valid = x1600_lcd_crtc_mode_valid,
+};
+
+static const struct drm_encoder_helper_funcs x1600_lcd_encoder_helper_funcs = {
+ .atomic_check = x1600_lcd_encoder_atomic_check,
+ .atomic_mode_set = x1600_lcd_encoder_atomic_mode_set,
+};
+
+/*
+ * drm_atomic_helper_commit_tail(), waiting for the flip instead of a vblank:
+ * the old framebuffer stays in use until the controller reads the new
+ * descriptor, which can be a frame later.
+ */
+static void x1600_lcd_commit_tail(struct drm_atomic_commit *state)
+{
+ struct drm_device *drm = state->dev;
+
+ drm_atomic_helper_commit_modeset_disables(drm, state);
+ drm_atomic_helper_commit_planes(drm, state, 0);
+ drm_atomic_helper_commit_modeset_enables(drm, state);
+ drm_atomic_helper_fake_vblank(state);
+ drm_atomic_helper_commit_hw_done(state);
+ drm_atomic_helper_wait_for_flip_done(drm, state);
+ drm_atomic_helper_cleanup_planes(drm, state);
+}
+
+static const struct drm_mode_config_funcs x1600_lcd_mode_config_funcs = {
+ .fb_create = drm_gem_fb_create,
+ .atomic_check = drm_atomic_helper_check,
+ .atomic_commit = drm_atomic_helper_commit,
+};
+
+static const struct drm_mode_config_helper_funcs x1600_lcd_mode_config_helpers = {
+ .atomic_commit_tail = x1600_lcd_commit_tail,
+};
+
+DEFINE_DRM_GEM_DMA_FOPS(x1600_lcd_fops);
+
+static const struct drm_driver x1600_lcd_driver = {
+ .driver_features = DRIVER_MODESET | DRIVER_GEM | DRIVER_ATOMIC,
+ .name = "ingenic-x1600-drm",
+ .desc = "DRM driver for the Ingenic X1600 display controller",
+ .major = 1,
+ .minor = 0,
+ .fops = &x1600_lcd_fops,
+ DRM_GEM_DMA_DRIVER_OPS,
+ DRM_FBDEV_DMA_DRIVER_OPS,
+};
+
+static int x1600_lcd_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct drm_connector *connector;
+ struct drm_bridge *bridge;
+ struct drm_device *drm;
+ struct x1600_lcd *lcd;
+ int irq, ret;
+
+ lcd = devm_drm_dev_alloc(dev, &x1600_lcd_driver, struct x1600_lcd, drm);
+ if (IS_ERR(lcd))
+ return PTR_ERR(lcd);
+
+ drm = &lcd->drm;
+ platform_set_drvdata(pdev, lcd);
+ init_completion(&lcd->stopped);
+ spin_lock_init(&lcd->irq_lock);
+
+ lcd->base = devm_platform_ioremap_resource(pdev, 0);
+ if (IS_ERR(lcd->base))
+ return PTR_ERR(lcd->base);
+
+ lcd->clk = devm_clk_get(dev, NULL);
+ if (IS_ERR(lcd->clk))
+ return dev_err_probe(dev, PTR_ERR(lcd->clk), "Unable to get clock\n");
+
+ irq = platform_get_irq(pdev, 0);
+ if (irq < 0)
+ return irq;
+
+ bridge = devm_drm_of_get_bridge(dev, dev->of_node, 0, 0);
+ if (IS_ERR(bridge))
+ return dev_err_probe(dev, PTR_ERR(bridge), "Unable to get the panel\n");
+
+ lcd->desc = dmam_alloc_coherent(dev, 2 * sizeof(*lcd->desc),
+ &lcd->desc_phys, GFP_KERNEL);
+ if (!lcd->desc)
+ return -ENOMEM;
+
+ ret = drmm_mode_config_init(drm);
+ if (ret)
+ return ret;
+
+ /* Framebuffers, not modes: fbdev's double buffer is taller than 720p. */
+ drm->mode_config.max_width = FIELD_MAX(X1600_LCD_TIMING_END);
+ drm->mode_config.max_height = FIELD_MAX(X1600_LCD_TIMING_END);
+ drm->mode_config.funcs = &x1600_lcd_mode_config_funcs;
+ drm->mode_config.helper_private = &x1600_lcd_mode_config_helpers;
+
+ ret = drm_universal_plane_init(drm, &lcd->plane, 1, &x1600_lcd_plane_funcs,
+ x1600_lcd_formats, ARRAY_SIZE(x1600_lcd_formats),
+ NULL, DRM_PLANE_TYPE_PRIMARY, NULL);
+ if (ret)
+ return ret;
+ drm_plane_helper_add(&lcd->plane, &x1600_lcd_plane_helper_funcs);
+
+ ret = drm_crtc_init_with_planes(drm, &lcd->crtc, &lcd->plane, NULL,
+ &x1600_lcd_crtc_funcs, NULL);
+ if (ret)
+ return ret;
+ drm_crtc_helper_add(&lcd->crtc, &x1600_lcd_crtc_helper_funcs);
+
+ ret = drmm_encoder_init(drm, &lcd->encoder, NULL, DRM_MODE_ENCODER_DPI, NULL);
+ if (ret)
+ return ret;
+ lcd->encoder.possible_crtcs = drm_crtc_mask(&lcd->crtc);
+ drm_encoder_helper_add(&lcd->encoder, &x1600_lcd_encoder_helper_funcs);
+
+ ret = drm_bridge_attach(&lcd->encoder, bridge, NULL,
+ DRM_BRIDGE_ATTACH_NO_CONNECTOR);
+ if (ret)
+ return ret;
+
+ connector = drm_bridge_connector_init(drm, &lcd->encoder);
+ if (IS_ERR(connector))
+ return PTR_ERR(connector);
+ drm_connector_attach_encoder(connector, &lcd->encoder);
+
+ ret = drm_vblank_init(drm, 1);
+ if (ret)
+ return ret;
+
+ drm_mode_config_reset(drm);
+
+ /* Take over from a boot framebuffer, and stop what it left running. */
+ ret = aperture_remove_all_conflicting_devices(x1600_lcd_driver.name);
+ if (ret)
+ return ret;
+
+ ret = clk_prepare_enable(lcd->clk);
+ if (ret)
+ return ret;
+
+ /* No interrupt from what the boot loader left enabled or pending. */
+ writel(0, lcd->base + X1600_LCD_INTC);
+ writel(readl(lcd->base + X1600_LCD_ST), lcd->base + X1600_LCD_CLR_ST);
+
+ ret = devm_request_irq(dev, irq, x1600_lcd_irq, 0, dev_name(dev), lcd);
+ if (ret) {
+ clk_disable_unprepare(lcd->clk);
+ return dev_err_probe(dev, ret, "Unable to request the IRQ\n");
+ }
+
+ /*
+ * Seen on the R1: after an underrun (TFT_UNDR) the controller can stop
+ * answering GEN_STP_SRD, and only a reset brings it back.
+ */
+ if (readl(lcd->base + X1600_LCD_ST) & X1600_LCD_ST_WORKING && !x1600_lcd_stop(lcd))
+ dev_err(dev, "The controller did not stop%s\n",
+ readl(lcd->base + X1600_LCD_ST) & X1600_LCD_ST_TFT_UNDR ?
+ " after an underrun" : "");
+ clk_disable_unprepare(lcd->clk);
+
+ ret = drm_dev_register(drm, 0);
+ if (ret)
+ return ret;
+
+ drm_client_setup_with_fourcc(drm, DRM_FORMAT_RGB565);
+ return 0;
+}
+
+static void x1600_lcd_remove(struct platform_device *pdev)
+{
+ struct x1600_lcd *lcd = platform_get_drvdata(pdev);
+
+ drm_dev_unregister(&lcd->drm);
+ drm_atomic_helper_shutdown(&lcd->drm);
+}
+
+static void x1600_lcd_shutdown(struct platform_device *pdev)
+{
+ struct x1600_lcd *lcd = platform_get_drvdata(pdev);
+
+ drm_atomic_helper_shutdown(&lcd->drm);
+}
+
+static const struct of_device_id x1600_lcd_of_match[] = {
+ { .compatible = "ingenic,x1600-dpu" },
+ { }
+};
+MODULE_DEVICE_TABLE(of, x1600_lcd_of_match);
+
+static struct platform_driver x1600_lcd_platform_driver = {
+ .driver = {
+ .name = "ingenic-x1600-drm",
+ .of_match_table = x1600_lcd_of_match,
+ },
+ .probe = x1600_lcd_probe,
+ .remove = x1600_lcd_remove,
+ .shutdown = x1600_lcd_shutdown,
+};
+module_platform_driver(x1600_lcd_platform_driver);
+
+MODULE_AUTHOR("Olivier Louvignes <olivier@mgcrea.io>");
+MODULE_DESCRIPTION("DRM driver for the Ingenic X1600 display controller");
+MODULE_LICENSE("GPL");
--
2.39.5
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v3 4/5] dt-bindings: display: panel: Add the HiBy LG35583
2026-11-06 11:19 [PATCH v3 0/5] drm/ingenic: Add the X1600 display controller and the HiBy R1 panel Olivier Louvignes
` (2 preceding siblings ...)
2026-11-06 11:19 ` [PATCH v3 3/5] drm/ingenic: Add a driver for the " Olivier Louvignes
@ 2026-11-06 11:19 ` Olivier Louvignes
2026-11-06 11:20 ` [PATCH v3 5/5] drm/panel: " Olivier Louvignes
4 siblings, 0 replies; 9+ messages in thread
From: Olivier Louvignes @ 2026-11-06 11:19 UTC (permalink / raw)
To: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, Neil Armstrong
Cc: David Airlie, Simona Vetter, Jessica Zhang, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Paul Cercueil,
H . Nikolaus Schaller, dri-devel, devicetree, linux-mips,
linux-kernel, Olivier Louvignes, Krzysztof Kozlowski
The 480x800 parallel RGB panel of the HiBy R1, set up over a 3-wire SPI
bus with 16-bit words.
Signed-off-by: Olivier Louvignes <olivier@mgcrea.io>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>
---
v3:
- reset-gpios, backlight and port dropped: panel-common has them
(Krzysztof).
- The example's endpoint points at dpu_out.
.../bindings/display/panel/hiby,lg35583.yaml | 67 +++++++++++++++++++
MAINTAINERS | 5 ++
2 files changed, 72 insertions(+)
create mode 100644 Documentation/devicetree/bindings/display/panel/hiby,lg35583.yaml
diff --git a/Documentation/devicetree/bindings/display/panel/hiby,lg35583.yaml b/Documentation/devicetree/bindings/display/panel/hiby,lg35583.yaml
new file mode 100644
index 000000000000..efdaf6241e60
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/panel/hiby,lg35583.yaml
@@ -0,0 +1,67 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/panel/hiby,lg35583.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: HiBy LG35583 480x800 panel
+
+maintainers:
+ - Olivier Louvignes <olivier@mgcrea.io>
+
+description:
+ The 480x800 parallel RGB panel of the HiBy R1, which HiBy calls the
+ LG35583. It is set up over a 3-wire SPI bus with 16-bit words.
+
+allOf:
+ - $ref: panel-common.yaml#
+ - $ref: /schemas/spi/spi-peripheral-props.yaml#
+
+properties:
+ compatible:
+ const: hiby,lg35583
+
+ reg:
+ maxItems: 1
+
+ vcc-supply:
+ description: Panel power supply
+
+ vccio-supply:
+ description: I/O power supply
+
+required:
+ - compatible
+ - reg
+ - vcc-supply
+ - vccio-supply
+ - reset-gpios
+ - port
+
+unevaluatedProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/gpio/gpio.h>
+
+ spi {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ panel@0 {
+ compatible = "hiby,lg35583";
+ reg = <0>;
+ spi-max-frequency = <1000000>;
+
+ vcc-supply = <&vcc_lcd>;
+ vccio-supply = <&vccio_lcd>;
+ reset-gpios = <&gpa 31 GPIO_ACTIVE_LOW>;
+ backlight = <&backlight>;
+
+ port {
+ panel_in: endpoint {
+ remote-endpoint = <&dpu_out>;
+ };
+ };
+ };
+ };
diff --git a/MAINTAINERS b/MAINTAINERS
index 5d992133e0d5..474d8a597e1c 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8343,6 +8343,11 @@ S: Maintained
T: git https://gitlab.freedesktop.org/drm/misc/kernel.git
F: drivers/gpu/drm/tiny/gm12u320.c
+DRM DRIVER FOR HIBY LG35583 PANEL
+M: Olivier Louvignes <olivier@mgcrea.io>
+S: Maintained
+F: Documentation/devicetree/bindings/display/panel/hiby,lg35583.yaml
+
DRM DRIVER FOR HIMAX HX8394 MIPI-DSI LCD panels
M: Ondrej Jirman <megi@xff.cz>
M: Javier Martinez Canillas <javierm@redhat.com>
--
2.39.5
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v3 5/5] drm/panel: Add the HiBy LG35583
2026-11-06 11:19 [PATCH v3 0/5] drm/ingenic: Add the X1600 display controller and the HiBy R1 panel Olivier Louvignes
` (3 preceding siblings ...)
2026-11-06 11:19 ` [PATCH v3 4/5] dt-bindings: display: panel: Add the HiBy LG35583 Olivier Louvignes
@ 2026-11-06 11:20 ` Olivier Louvignes
4 siblings, 0 replies; 9+ messages in thread
From: Olivier Louvignes @ 2026-11-06 11:20 UTC (permalink / raw)
To: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, Neil Armstrong
Cc: David Airlie, Simona Vetter, Jessica Zhang, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Paul Cercueil,
H . Nikolaus Schaller, dri-devel, devicetree, linux-mips,
linux-kernel, Olivier Louvignes
The 480x800 parallel RGB panel of the HiBy R1. It is set up over a 3-wire
SPI bus with 16-bit words, each carrying a register address byte or a
data byte, which no existing panel driver speaks. The set-up sequence is
the vendor kernel's; prepare sends it up to sleep-out, and enable and
disable switch the display on and off. It asks for BGR666: in RGB order the
R1 shows red and blue swapped.
Signed-off-by: Olivier Louvignes <olivier@mgcrea.io>
---
MAINTAINERS | 1 +
drivers/gpu/drm/panel/Kconfig | 10 +
drivers/gpu/drm/panel/Makefile | 1 +
drivers/gpu/drm/panel/panel-hiby-lg35583.c | 342 +++++++++++++++++++++
4 files changed, 354 insertions(+)
create mode 100644 drivers/gpu/drm/panel/panel-hiby-lg35583.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 474d8a597e1c..f7543e55d7b9 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8347,6 +8347,7 @@ DRM DRIVER FOR HIBY LG35583 PANEL
M: Olivier Louvignes <olivier@mgcrea.io>
S: Maintained
F: Documentation/devicetree/bindings/display/panel/hiby,lg35583.yaml
+F: drivers/gpu/drm/panel/panel-hiby-lg35583.c
DRM DRIVER FOR HIMAX HX8394 MIPI-DSI LCD panels
M: Ondrej Jirman <megi@xff.cz>
diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
index 26ffa2378f55..79528ff0100a 100644
--- a/drivers/gpu/drm/panel/Kconfig
+++ b/drivers/gpu/drm/panel/Kconfig
@@ -209,6 +209,16 @@ config DRM_PANEL_LVDS
handling of power supplies or control signals. It implements automatic
backlight handling if the panel is attached to a backlight controller.
+config DRM_PANEL_HIBY_LG35583
+ tristate "HiBy LG35583 RGB/SPI panel"
+ depends on OF && SPI
+ help
+ Say Y here if you want to enable support for the LG35583, the
+ 480x800 parallel RGB panel of the HiBy R1 digital audio player,
+ which is set up over a 3-wire SPI bus.
+
+ To compile this driver as a module, choose M here.
+
config DRM_PANEL_HIMAX_HX8279
tristate "Himax HX8279-based panels"
depends on OF
diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
index 365f2e73f6b4..9f5cacfaba29 100644
--- a/drivers/gpu/drm/panel/Makefile
+++ b/drivers/gpu/drm/panel/Makefile
@@ -21,6 +21,7 @@ obj-$(CONFIG_DRM_PANEL_ELIDA_KD35T133) += panel-elida-kd35t133.o
obj-$(CONFIG_DRM_PANEL_FEIXIN_K101_IM2BA02) += panel-feixin-k101-im2ba02.o
obj-$(CONFIG_DRM_PANEL_FEIYANG_FY07024DI26A30D) += panel-feiyang-fy07024di26a30d.o
obj-$(CONFIG_DRM_PANEL_FOCALTECH_OTA7290B) += panel-focaltech-ota7290b.o
+obj-$(CONFIG_DRM_PANEL_HIBY_LG35583) += panel-hiby-lg35583.o
obj-$(CONFIG_DRM_PANEL_HIMAX_HX8279) += panel-himax-hx8279.o
obj-$(CONFIG_DRM_PANEL_HIMAX_HX83102) += panel-himax-hx83102.o
obj-$(CONFIG_DRM_PANEL_HIMAX_HX83112A) += panel-himax-hx83112a.o
diff --git a/drivers/gpu/drm/panel/panel-hiby-lg35583.c b/drivers/gpu/drm/panel/panel-hiby-lg35583.c
new file mode 100644
index 000000000000..47aba09e91a4
--- /dev/null
+++ b/drivers/gpu/drm/panel/panel-hiby-lg35583.c
@@ -0,0 +1,342 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * The LG35583 panel of the HiBy R1
+ *
+ * Copyright (c) 2026 Olivier Louvignes <olivier@mgcrea.io>
+ *
+ * A 480x800 parallel RGB panel, set up over a 3-wire SPI bus with 16-bit
+ * words: 0x20 and a register's high byte, 0x00 and its low byte, then 0x40
+ * and each data byte. HiBy calls it the LG35583; its controller is not
+ * identified. The set-up sequence is the one the vendor kernel sends.
+ */
+
+#include <linux/delay.h>
+#include <linux/gpio/consumer.h>
+#include <linux/media-bus-format.h>
+#include <linux/module.h>
+#include <linux/regulator/consumer.h>
+#include <linux/spi/spi.h>
+
+#include <drm/drm_modes.h>
+#include <drm/drm_panel.h>
+
+#define LG35583_ADDR_HI 0x2000
+#define LG35583_ADDR_LO 0x0000
+#define LG35583_DATA 0x4000
+
+#define LG35583_SLPIN 0x1000
+#define LG35583_SLPOUT 0x1100
+#define LG35583_DISPOFF 0x2800
+#define LG35583_DISPON 0x2900
+#define LG35583_RAMWR 0x2c00
+
+struct lg35583_write {
+ u16 reg; /* 0: wait val milliseconds */
+ s16 val; /* -1: no data */
+};
+
+#define REG(r, v) { (r), (v) }
+#define CMD(r) { (r), -1 }
+#define DELAY(ms) { 0, (ms) }
+
+static const struct lg35583_write lg35583_init[] = {
+ REG(0xb100, 0x06), REG(0xb101, 0x01), REG(0xb102, 0x54),
+ REG(0xb104, 0x07), REG(0xb105, 0x06), REG(0xb106, 0x06),
+ REG(0xb107, 0x4a), REG(0xb108, 0x21), REG(0xb109, 0x08),
+ REG(0xf101, 0xc0), REG(0xf103, 0x01),
+ DELAY(5),
+ REG(0xb400, 0x01), REG(0xb600, 0x10),
+ REG(0x3a00, 0x77), REG(0x3b00, 0x03), REG(0x5e04, 0x02),
+ REG(0x3600, 0x08),
+ DELAY(5),
+ /* Gamma */
+ REG(0xe000, 0x00), REG(0xe001, 0x2f), REG(0xe002, 0x3f),
+ REG(0xe003, 0x1f), REG(0xe004, 0x21), REG(0xe005, 0x2c),
+ REG(0xe006, 0x2c), REG(0xe007, 0x61), REG(0xe008, 0x1c),
+ REG(0xe009, 0x18), REG(0xe00a, 0xbc), REG(0xe00b, 0x27),
+ REG(0xe00c, 0x59), REG(0xe00d, 0x6a), REG(0xe00e, 0xbb),
+ REG(0xe00f, 0xbe), REG(0xe010, 0x3e), REG(0xe011, 0x48),
+ REG(0xe100, 0x00), REG(0xe101, 0x32), REG(0xe102, 0x45),
+ REG(0xe103, 0x26), REG(0xe104, 0x21), REG(0xe105, 0x2c),
+ REG(0xe106, 0x5c), REG(0xe107, 0x63), REG(0xe108, 0x1c),
+ REG(0xe109, 0x19), REG(0xe10a, 0xd3), REG(0xe10b, 0x28),
+ REG(0xe10c, 0x59), REG(0xe10d, 0x6b), REG(0xe10e, 0xdb),
+ REG(0xe10f, 0xde), REG(0xe110, 0x5f), REG(0xe111, 0x6b),
+ REG(0xe200, 0x7f), REG(0xe201, 0x7d), REG(0xe202, 0x86),
+ REG(0xe203, 0x89), REG(0xe204, 0x10), REG(0xe205, 0x1e),
+ REG(0xe206, 0x41), REG(0xe207, 0x77), REG(0xe208, 0x0d),
+ REG(0xe209, 0x1d), REG(0xe20a, 0xc8), REG(0xe20b, 0x30),
+ REG(0xe20c, 0x5f), REG(0xe20d, 0x71), REG(0xe20e, 0xc3),
+ REG(0xe20f, 0xbe), REG(0xe210, 0x3e), REG(0xe211, 0x48),
+ REG(0xe300, 0x7f), REG(0xe301, 0x7f), REG(0xe302, 0x86),
+ REG(0xe303, 0x93), REG(0xe304, 0x10), REG(0xe305, 0x1e),
+ REG(0xe306, 0x41), REG(0xe307, 0x89), REG(0xe308, 0x0d),
+ REG(0xe309, 0x1d), REG(0xe30a, 0xe0), REG(0xe30b, 0x30),
+ REG(0xe30c, 0x5f), REG(0xe30d, 0x70), REG(0xe30e, 0xe4),
+ REG(0xe30f, 0xdf), REG(0xe310, 0x5f), REG(0xe311, 0x6b),
+ REG(0xe400, 0x00), REG(0xe401, 0x43), REG(0xe402, 0x6f),
+ REG(0xe403, 0x60), REG(0xe404, 0x21), REG(0xe405, 0x2c),
+ REG(0xe406, 0x5a), REG(0xe407, 0x6d), REG(0xe408, 0x1c),
+ REG(0xe409, 0x18), REG(0xe40a, 0xc6), REG(0xe40b, 0x27),
+ REG(0xe40c, 0x59), REG(0xe40d, 0x6a), REG(0xe40e, 0xbb),
+ REG(0xe40f, 0xbe), REG(0xe410, 0x3e), REG(0xe411, 0x48),
+ REG(0xe500, 0x00), REG(0xe501, 0x46), REG(0xe502, 0x75),
+ REG(0xe503, 0x67), REG(0xe504, 0x21), REG(0xe505, 0x2c),
+ REG(0xe506, 0x5a), REG(0xe507, 0x7e), REG(0xe508, 0x1c),
+ REG(0xe509, 0x19), REG(0xe50a, 0xdd), REG(0xe50b, 0x28),
+ REG(0xe50c, 0x59), REG(0xe50d, 0x6b), REG(0xe50e, 0xdb),
+ REG(0xe50f, 0xde), REG(0xe510, 0x5f), REG(0xe511, 0x6b),
+ DELAY(5),
+ REG(0xc200, 0x70), REG(0xc202, 0x70), REG(0xc203, 0x44),
+ DELAY(20),
+ REG(0xc200, 0x51), REG(0xc201, 0x06), REG(0xc202, 0x51),
+ REG(0xc203, 0x66),
+ DELAY(20),
+ REG(0xc100, 0x45),
+ DELAY(5),
+ REG(0xc700, 0x46), REG(0xc000, 0xa3), REG(0xc002, 0x8a),
+ DELAY(5),
+ CMD(LG35583_SLPOUT),
+ DELAY(130),
+};
+
+struct lg35583 {
+ struct drm_panel panel;
+ struct spi_device *spi;
+ struct gpio_desc *reset;
+ struct regulator *vcc;
+ struct regulator *vccio;
+};
+
+static const struct drm_display_mode lg35583_mode = {
+ .clock = 28000,
+ .hdisplay = 480,
+ .hsync_start = 480 + 24,
+ .hsync_end = 480 + 24 + 24,
+ .htotal = 480 + 24 + 24 + 24,
+ .vdisplay = 800,
+ .vsync_start = 800 + 14,
+ .vsync_end = 800 + 14 + 5,
+ .vtotal = 800 + 14 + 5 + 8,
+ .flags = DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC,
+ .type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED,
+};
+
+static struct lg35583 *to_lg35583(struct drm_panel *panel)
+{
+ return container_of(panel, struct lg35583, panel);
+}
+
+static int lg35583_word(struct lg35583 *ctx, u16 word)
+{
+ /* Copied to a DMA-safe buffer. */
+ return spi_write_then_read(ctx->spi, &word, sizeof(word), NULL, 0);
+}
+
+static int lg35583_cmd(struct lg35583 *ctx, u16 reg)
+{
+ int ret;
+
+ ret = lg35583_word(ctx, LG35583_ADDR_HI | (reg >> 8));
+ if (ret)
+ return ret;
+ return lg35583_word(ctx, LG35583_ADDR_LO | (reg & 0xff));
+}
+
+static int lg35583_write(struct lg35583 *ctx, const struct lg35583_write *w)
+{
+ int ret;
+
+ if (!w->reg) {
+ msleep(w->val);
+ return 0;
+ }
+
+ ret = lg35583_cmd(ctx, w->reg);
+ if (ret || w->val < 0)
+ return ret;
+ return lg35583_word(ctx, LG35583_DATA | w->val);
+}
+
+static int lg35583_prepare(struct drm_panel *panel)
+{
+ struct lg35583 *ctx = to_lg35583(panel);
+ unsigned int i;
+ int ret;
+
+ ret = regulator_enable(ctx->vccio);
+ if (ret)
+ return ret;
+ usleep_range(5000, 6000);
+
+ ret = regulator_enable(ctx->vcc);
+ if (ret)
+ goto err_vccio;
+ usleep_range(5000, 6000);
+
+ gpiod_set_value_cansleep(ctx->reset, 0);
+ usleep_range(5000, 6000);
+ gpiod_set_value_cansleep(ctx->reset, 1);
+ msleep(50);
+ gpiod_set_value_cansleep(ctx->reset, 0);
+ msleep(30);
+
+ for (i = 0; i < ARRAY_SIZE(lg35583_init); i++) {
+ ret = lg35583_write(ctx, &lg35583_init[i]);
+ if (ret) {
+ dev_err(panel->dev, "Unable to set up the panel: %d\n", ret);
+ goto err_vcc;
+ }
+ }
+
+ return 0;
+
+err_vcc:
+ gpiod_set_value_cansleep(ctx->reset, 1);
+ regulator_disable(ctx->vcc);
+err_vccio:
+ regulator_disable(ctx->vccio);
+ return ret;
+}
+
+static int lg35583_enable(struct drm_panel *panel)
+{
+ struct lg35583 *ctx = to_lg35583(panel);
+ int ret;
+
+ ret = lg35583_cmd(ctx, LG35583_DISPON);
+ if (ret)
+ return ret;
+
+ return lg35583_cmd(ctx, LG35583_RAMWR);
+}
+
+static int lg35583_disable(struct drm_panel *panel)
+{
+ struct lg35583 *ctx = to_lg35583(panel);
+ int ret;
+
+ ret = lg35583_cmd(ctx, LG35583_DISPOFF);
+ usleep_range(5000, 6000);
+ return ret;
+}
+
+static int lg35583_unprepare(struct drm_panel *panel)
+{
+ struct lg35583 *ctx = to_lg35583(panel);
+
+ lg35583_cmd(ctx, LG35583_SLPIN);
+ usleep_range(5000, 6000);
+
+ gpiod_set_value_cansleep(ctx->reset, 1);
+ regulator_disable(ctx->vccio);
+ regulator_disable(ctx->vcc);
+ return 0;
+}
+
+static int lg35583_get_modes(struct drm_panel *panel, struct drm_connector *connector)
+{
+ /* Red and blue are swapped on the R1 without it. */
+ static const u32 bus_format = MEDIA_BUS_FMT_BGR666_1X18;
+ struct drm_display_mode *mode;
+
+ mode = drm_mode_duplicate(connector->dev, &lg35583_mode);
+ if (!mode)
+ return -ENOMEM;
+
+ drm_mode_set_name(mode);
+ drm_mode_probed_add(connector, mode);
+
+ connector->display_info.bpc = 6;
+ connector->display_info.bus_flags = DRM_BUS_FLAG_DE_HIGH |
+ DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE;
+ drm_display_info_set_bus_formats(&connector->display_info, &bus_format, 1);
+
+ return 1;
+}
+
+static const struct drm_panel_funcs lg35583_funcs = {
+ .prepare = lg35583_prepare,
+ .enable = lg35583_enable,
+ .disable = lg35583_disable,
+ .unprepare = lg35583_unprepare,
+ .get_modes = lg35583_get_modes,
+};
+
+static int lg35583_probe(struct spi_device *spi)
+{
+ struct device *dev = &spi->dev;
+ struct lg35583 *ctx;
+ int ret;
+
+ ctx = devm_drm_panel_alloc(dev, struct lg35583, panel, &lg35583_funcs,
+ DRM_MODE_CONNECTOR_DPI);
+ if (IS_ERR(ctx))
+ return PTR_ERR(ctx);
+
+ ctx->spi = spi;
+ spi_set_drvdata(spi, ctx);
+
+ spi->bits_per_word = 16;
+ ret = spi_setup(spi);
+ if (ret)
+ return dev_err_probe(dev, ret, "Unable to set up SPI\n");
+
+ ctx->vcc = devm_regulator_get(dev, "vcc");
+ if (IS_ERR(ctx->vcc))
+ return dev_err_probe(dev, PTR_ERR(ctx->vcc), "Unable to get vcc\n");
+
+ ctx->vccio = devm_regulator_get(dev, "vccio");
+ if (IS_ERR(ctx->vccio))
+ return dev_err_probe(dev, PTR_ERR(ctx->vccio), "Unable to get vccio\n");
+
+ /*
+ * Deasserted, which keeps a panel the boot loader left on as it is, but
+ * as an output: after a reset the pin is an input, and prepare's reset
+ * pulse would never reach the panel.
+ */
+ ctx->reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
+ if (IS_ERR(ctx->reset))
+ return dev_err_probe(dev, PTR_ERR(ctx->reset), "Unable to get reset\n");
+
+ ret = drm_panel_of_backlight(&ctx->panel);
+ if (ret)
+ return ret;
+
+ drm_panel_add(&ctx->panel);
+ return 0;
+}
+
+static void lg35583_remove(struct spi_device *spi)
+{
+ struct lg35583 *ctx = spi_get_drvdata(spi);
+
+ drm_panel_remove(&ctx->panel);
+}
+
+static const struct of_device_id lg35583_of_match[] = {
+ { .compatible = "hiby,lg35583" },
+ { }
+};
+MODULE_DEVICE_TABLE(of, lg35583_of_match);
+
+static const struct spi_device_id lg35583_ids[] = {
+ { "lg35583" },
+ { }
+};
+MODULE_DEVICE_TABLE(spi, lg35583_ids);
+
+static struct spi_driver lg35583_driver = {
+ .probe = lg35583_probe,
+ .remove = lg35583_remove,
+ .id_table = lg35583_ids,
+ .driver = {
+ .name = "panel-hiby-lg35583",
+ .of_match_table = lg35583_of_match,
+ },
+};
+module_spi_driver(lg35583_driver);
+
+MODULE_AUTHOR("Olivier Louvignes <olivier@mgcrea.io>");
+MODULE_DESCRIPTION("HiBy R1 LG35583 panel driver");
+MODULE_LICENSE("GPL");
--
2.39.5
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 3/5] drm/ingenic: Add a driver for the X1600 display controller
2026-11-06 11:19 ` [PATCH v3 3/5] drm/ingenic: Add a driver for the " Olivier Louvignes
@ 2026-11-06 13:34 ` Paul Cercueil
2026-11-06 14:38 ` Olivier Louvignes
0 siblings, 1 reply; 9+ messages in thread
From: Paul Cercueil @ 2026-11-06 13:34 UTC (permalink / raw)
To: Olivier Louvignes, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, Neil Armstrong
Cc: David Airlie, Simona Vetter, Jessica Zhang, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, H . Nikolaus Schaller,
dri-devel, devicetree, linux-mips, linux-kernel,
Thomas Petazzoni
Hi Olivier,
Le vendredi 06 novembre 2026 à 12:19 +0100, Olivier Louvignes a écrit :
> The X1600's display controller has none of the registers ingenic-drm
> programs: it fetches each frame through a descriptor whose address it
> takes from SRD_CHAIN_ADDR, and keeps its interface, TFT timings and smart
> panel set-up in separate register blocks (X1600 programming manual,
> chapter 10; the X2000's controller without the composer). This driver
> handles parallel RGB panels on a 24-bit bus, with one full-screen RGB565
> plane, and takes the colour order (RGB or BGR) from the panel's bus
> format.
>
> Each descriptor points at itself, so a frame repeats until a flip hands
> the controller the other one. The controller takes it "at appropriate
> time", so a flip's event waits until the descriptor-site register
> reports the new descriptor at the end of a frame, and a commit waits for
> that event rather than for a vblank before the old framebuffer is
> released: the controller can keep reading it for a frame after the flip.
>
> Tested on a HiBy R1 with its 480x800 panel: the fbdev console and
> r1player, blanking and unblanking, and 60 fbdev pans in 0.97 s, one flip
> per frame at about 61 Hz, none timing out.
>
> Signed-off-by: Olivier Louvignes <olivier@mgcrea.io>
> ---
> v3:
> - ingenic,x1600-dpu; the clock taken unnamed, as the binding no longer
> has clock-names (Krzysztof).
> - "display controller" in the Kconfig prompt, the header and the
> descriptions.
>
> v2:
> - A commit_tail that waits for flip_done, not for a vblank, before the
> planes are cleaned up.
> - A CRTC being enabled sends its event once the controller runs, not
> from atomic_flush() before atomic_enable().
> - Reject a plane that is not visible.
> - atomic_disable() leaves the controller alone when atomic_enable()
> could not start the clock.
>
> MAINTAINERS | 1 +
> drivers/gpu/drm/Makefile | 1 +
> drivers/gpu/drm/ingenic/Kconfig | 22 +
> drivers/gpu/drm/ingenic/Makefile | 1 +
> drivers/gpu/drm/ingenic/ingenic-x1600-drm.c | 744 ++++++++++++++++++++
> 5 files changed, 769 insertions(+)
> create mode 100644 drivers/gpu/drm/ingenic/ingenic-x1600-drm.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index a5160860ae26..5d992133e0d5 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -13180,6 +13180,7 @@ INGENIC X1600 DRM DRIVER
> M: Olivier Louvignes <olivier@mgcrea.io>
> S: Maintained
> F: Documentation/devicetree/bindings/display/ingenic,x1600-dpu.yaml
> +F: drivers/gpu/drm/ingenic/ingenic-x1600-drm.c
>
> INJOINIC IP5xxx POWER BANK IC DRIVER
> M: Samuel Holland <samuel@sholland.org>
> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> index 2c6bc093a923..37dc781761d6 100644
> --- a/drivers/gpu/drm/Makefile
> +++ b/drivers/gpu/drm/Makefile
> @@ -206,6 +206,7 @@ obj-$(CONFIG_DRM_STM) += stm/
> obj-$(CONFIG_DRM_STI) += sti/
> obj-y += imx/
> obj-$(CONFIG_DRM_INGENIC) += ingenic/
> +obj-$(CONFIG_DRM_INGENIC_X1600) += ingenic/
> obj-$(CONFIG_DRM_LOGICVC) += logicvc/
> obj-$(CONFIG_DRM_MEDIATEK) += mediatek/
> obj-$(CONFIG_DRM_MESON) += meson/
> diff --git a/drivers/gpu/drm/ingenic/Kconfig b/drivers/gpu/drm/ingenic/Kconfig
> index 987ed42f6f7a..85ac9ca62d7d 100644
> --- a/drivers/gpu/drm/ingenic/Kconfig
> +++ b/drivers/gpu/drm/ingenic/Kconfig
> @@ -20,6 +20,28 @@ config DRM_INGENIC
>
> If M is selected the module will be called ingenic-drm.
>
> +config DRM_INGENIC_X1600
> + tristate "DRM support for the Ingenic X1600 display controller"
> + depends on MIPS || COMPILE_TEST
> + depends on DRM
> + depends on OF
> + depends on COMMON_CLK
> + select APERTURE_HELPERS
> + select DRM_BRIDGE
> + select DRM_CLIENT_SELECTION
> + select DRM_PANEL_BRIDGE
> + select DRM_KMS_HELPER
> + select DRM_DISPLAY_HELPER
> + select DRM_BRIDGE_CONNECTOR
> + select DRM_GEM_DMA_HELPER
> + help
> + Choose this option for DRM support for the display controller of
> + the Ingenic X1600 SoC. It drives parallel RGB panels on a 24-bit
> + bus from one full-screen RGB565 plane; the controller's smart panel
> + interface is not supported.
> +
> + If M is selected the module will be called ingenic-x1600-drm.
> +
> if DRM_INGENIC
>
> config DRM_INGENIC_IPU
> diff --git a/drivers/gpu/drm/ingenic/Makefile
> b/drivers/gpu/drm/ingenic/Makefile
> index f10cc1c5a5f2..90072ad13158 100644
> --- a/drivers/gpu/drm/ingenic/Makefile
> +++ b/drivers/gpu/drm/ingenic/Makefile
> @@ -2,3 +2,4 @@ obj-$(CONFIG_DRM_INGENIC) += ingenic-drm.o
> ingenic-drm-y = ingenic-drm-drv.o
> ingenic-drm-$(CONFIG_DRM_INGENIC_IPU) += ingenic-ipu.o
> obj-$(CONFIG_DRM_INGENIC_DW_HDMI) += ingenic-dw-hdmi.o
> +obj-$(CONFIG_DRM_INGENIC_X1600) += ingenic-x1600-drm.o
> diff --git a/drivers/gpu/drm/ingenic/ingenic-x1600-drm.c
> b/drivers/gpu/drm/ingenic/ingenic-x1600-drm.c
> new file mode 100644
> index 000000000000..e69c968ebcd9
> --- /dev/null
> +++ b/drivers/gpu/drm/ingenic/ingenic-x1600-drm.c
> @@ -0,0 +1,744 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * DRM driver for the display controller (DPU) of the Ingenic X1600
> + *
> + * Copyright (c) 2026 Olivier Louvignes <olivier@mgcrea.io>
> + *
> + * The X1600's display controller (X1600 programming manual, chapter 10) is
> + * the simple read channel of the X2000's, without its composer. It shares no
> + * registers with the JZ47xx and X1000 controllers that ingenic-drm drives:
> it
> + * fetches each frame through a descriptor whose address is in
> SRD_CHAIN_ADDR,
> + * and its interface and timings sit in register blocks of their own. It
> + * drives parallel RGB (TFT) panels and smart panels; this driver handles
> + * parallel RGB panels on a 24-bit bus, from RGB565 framebuffers.
> + */
> +
> +#include <linux/aperture.h>
> +#include <linux/bitfield.h>
> +#include <linux/bits.h>
> +#include <linux/clk.h>
> +#include <linux/completion.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/media-bus-format.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/spinlock.h>
> +
> +#include <drm/clients/drm_client_setup.h>
> +#include <drm/drm_atomic.h>
> +#include <drm/drm_atomic_helper.h>
> +#include <drm/drm_bridge.h>
> +#include <drm/drm_bridge_connector.h>
> +#include <drm/drm_crtc.h>
> +#include <drm/drm_drv.h>
> +#include <drm/drm_encoder.h>
> +#include <drm/drm_fb_dma_helper.h>
> +#include <drm/drm_fbdev_dma.h>
> +#include <drm/drm_fourcc.h>
> +#include <drm/drm_framebuffer.h>
> +#include <drm/drm_gem_dma_helper.h>
> +#include <drm/drm_gem_framebuffer_helper.h>
> +#include <drm/drm_of.h>
> +#include <drm/drm_plane.h>
> +#include <drm/drm_print.h>
> +#include <drm/drm_probe_helper.h>
> +#include <drm/drm_vblank.h>
> +
> +#define X1600_LCD_SRD_CHAIN_ADDR 0x1000
> +#define X1600_LCD_SRD_CHAIN_START 0x1004
> +#define X1600_LCD_CTRL 0x2000
> +#define X1600_LCD_ST 0x2004
> +#define X1600_LCD_CLR_ST 0x2008
> +#define X1600_LCD_INTC 0x200c
> +#define X1600_LCD_INT_FLAG 0x2010
> +#define X1600_LCD_COM_CONFIG 0x2014
> +#define X1600_LCD_CHAIN_SITE 0x2204
> +#define X1600_LCD_DISP_COM 0x8000
> +#define X1600_LCD_TFT_HSYNC 0x9000
> +#define X1600_LCD_TFT_VSYNC 0x9004
> +#define X1600_LCD_TFT_HDE 0x9008
> +#define X1600_LCD_TFT_VDE 0x900c
> +#define X1600_LCD_TFT_TRAN_CFG 0x9010
> +
> +/* CTRL: stop the simple read channel at the end of the frame. */
> +#define X1600_LCD_CTRL_GEN_STP_SRD BIT(4)
> +
> +/* ST; CLR_ST (write 1 to clear), INTC and INT_FLAG have the same bits. */
> +#define X1600_LCD_ST_WORKING BIT(0)
> +#define X1600_LCD_ST_STOP_SRD_ACK BIT(7)
> +#define X1600_LCD_ST_TFT_UNDR BIT(8)
> +#define X1600_LCD_ST_DISP_END BIT(17) /* a frame was
> displayed */
> +
> +#define X1600_LCD_COM_CONFIG_BURST_LEN GENMASK(5, 4)
> +#define X1600_LCD_BURST_LEN_32 3
> +
> +#define X1600_LCD_DISP_COM_IF GENMASK(1, 0)
> +#define X1600_LCD_DISP_COM_IF_TFT 1
> +#define X1600_LCD_DISP_COM_DITHER_EN BIT(4)
> +#define X1600_LCD_DISP_COM_TFT_CLKGATE BIT(5)
> +#define X1600_LCD_DISP_COM_SLCD_CLKGATE BIT(6)
> +
> +/* TFT_HSYNC, TFT_VSYNC: sync length and total; TFT_HDE, TFT_VDE: active
> area. */
> +#define X1600_LCD_TIMING_START GENMASK(27, 16)
> +#define X1600_LCD_TIMING_END GENMASK(11, 0)
> +
> +#define X1600_LCD_TFT_COLOR_EVEN GENMASK(21, 19)
> +#define X1600_LCD_TFT_COLOR_ODD GENMASK(18, 16)
> +#define X1600_LCD_TFT_PIX_CLK_INV BIT(10)
> +#define X1600_LCD_TFT_DE_DL BIT(9)
> +#define X1600_LCD_TFT_HSYNC_DL BIT(8)
> +#define X1600_LCD_TFT_VSYNC_DL BIT(7)
> +#define X1600_LCD_TFT_MODE GENMASK(2, 0)
> +#define X1600_LCD_TFT_MODE_24BIT 0
> +#define X1600_LCD_TFT_COLOR_RGB 0
> +#define X1600_LCD_TFT_COLOR_BGR 2
> +
> +#define X1600_LCD_DESC_CFG_FMT GENMASK(22, 19)
> +#define X1600_LCD_DESC_FMT_RGB565 2
> +/* An interrupt at the end of the frame: DISP_END needs this and INTC set. */
> +#define X1600_LCD_DESC_CTRL_EOD_MSK BIT(17)
> +
> +#define X1600_LCD_MAX_PIXELS (1280 * 720)
> +/* A smaller frame breaks a continuous descriptor chain (manual, 10.7). */
> +#define X1600_LCD_MIN_PIXELS 500
> +#define X1600_LCD_STOP_TIMEOUT_MS 100
> +
> +/*
> + * The controller reads a descriptor at the start of each frame and then
> + * follows its next field. Each descriptor points at itself, so a frame
> + * repeats until SRD_CHAIN_ADDR names another one.
> + */
> +struct x1600_lcd_desc {
> + u32 next;
> + u32 addr;
> + u32 stride; /* in pixels */
> + u32 cfg;
> + u32 ctrl;
> +} __aligned(64);
> +
> +struct x1600_lcd {
> + struct drm_device drm;
> + struct drm_plane plane;
> + struct drm_crtc crtc;
> + struct drm_encoder encoder;
> +
> + void __iomem *base;
> + struct clk *clk;
> +
> + struct x1600_lcd_desc *desc;
> + dma_addr_t desc_phys;
> + unsigned int cur; /* the descriptor last handed over */
> + bool running;
> + struct completion stopped;
> + /* Serialises the read-modify-writes of INTC. */
> + spinlock_t irq_lock;
> +
> + /* Under drm.event_lock: sent once the controller reads event_desc.
> */
> + struct drm_pending_vblank_event *event;
> + u32 event_desc;
> +
> + u32 bus_flags;
> + u32 color_order;
> +};
> +
> +static const u32 x1600_lcd_formats[] = {
> + DRM_FORMAT_RGB565,
> +};
> +
> +static struct x1600_lcd *to_x1600_lcd(struct drm_device *drm)
> +{
> + return container_of(drm, struct x1600_lcd, drm);
> +}
> +
> +static dma_addr_t x1600_lcd_desc_addr(struct x1600_lcd *lcd, unsigned int i)
> +{
> + return lcd->desc_phys + i * sizeof(*lcd->desc);
> +}
> +
> +static void x1600_lcd_irq_enable(struct x1600_lcd *lcd, u32 mask, bool
> enable)
> +{
> + unsigned long flags;
> + u32 val;
> +
> + spin_lock_irqsave(&lcd->irq_lock, flags);
> + val = readl(lcd->base + X1600_LCD_INTC);
> + if (enable)
> + val |= mask;
> + else
> + val &= ~mask;
> + /* Seen on the R1: a bit still set when it is masked keeps the line
> up. */
> + writel(mask, lcd->base + X1600_LCD_CLR_ST);
> + writel(val, lcd->base + X1600_LCD_INTC);
> + spin_unlock_irqrestore(&lcd->irq_lock, flags);
> +}
> +
> +/* Stops at the end of the frame; false if the controller did not report it.
> */
> +static bool x1600_lcd_stop(struct x1600_lcd *lcd)
> +{
> + unsigned long left;
> +
> + reinit_completion(&lcd->stopped);
> + x1600_lcd_irq_enable(lcd, X1600_LCD_ST_STOP_SRD_ACK, true);
> + writel(X1600_LCD_CTRL_GEN_STP_SRD, lcd->base + X1600_LCD_CTRL);
> + left = wait_for_completion_timeout(&lcd->stopped,
> +
> msecs_to_jiffies(X1600_LCD_STOP_TIMEOUT_MS));
> + x1600_lcd_irq_enable(lcd, X1600_LCD_ST_STOP_SRD_ACK, false);
> + return left;
> +}
> +
> +static u32 x1600_lcd_timing(unsigned int start, unsigned int end)
> +{
> + return FIELD_PREP(X1600_LCD_TIMING_START, start) |
> + FIELD_PREP(X1600_LCD_TIMING_END, end);
> +}
> +
> +/* Sends the commit's event once the controller has read the new descriptor.
> */
> +static void x1600_lcd_queue_event(struct x1600_lcd *lcd, struct
> drm_crtc_state *crtc_state)
> +{
> + struct drm_pending_vblank_event *event = crtc_state->event;
> + struct drm_crtc *crtc = &lcd->crtc;
> +
> + if (!event)
> + return;
> +
> + crtc_state->event = NULL;
> +
> + /*
> + * The controller takes a new descriptor "at appropriate time"
> (manual,
> + * 10.6.4): wait until it reports reading the one just handed over.
> + */
> + spin_lock_irq(&crtc->dev->event_lock);
> + if (lcd->running && !WARN_ON(lcd->event) && drm_crtc_vblank_get(crtc)
> == 0) {
> + lcd->event = event;
> + lcd->event_desc = x1600_lcd_desc_addr(lcd, lcd->cur);
> + } else {
> + drm_crtc_send_vblank_event(crtc, event);
> + }
> + spin_unlock_irq(&crtc->dev->event_lock);
> +}
> +
> +static void x1600_lcd_crtc_atomic_enable(struct drm_crtc *crtc,
> + struct drm_atomic_commit *state)
> +{
> + struct drm_crtc_state *crtc_state =
> drm_atomic_get_new_crtc_state(state, crtc);
> + struct x1600_lcd *lcd = to_x1600_lcd(crtc->dev);
> + struct drm_display_mode *mode = &crtc->state->adjusted_mode;
This bothers me a bit, shouldn't it be using 'crtc_state'?
> + unsigned int hde = mode->htotal - mode->hsync_start;
> + unsigned int vde = mode->vtotal - mode->vsync_start;
> + u32 val;
> + int ret;
> +
> + ret = clk_set_rate(lcd->clk, mode->crtc_clock * 1000);
> + if (ret)
> + dev_warn(lcd->drm.dev, "Unable to set the pixel clock: %d\n",
> ret);
> + ret = clk_prepare_enable(lcd->clk);
> + if (ret) {
> + dev_err(lcd->drm.dev, "Unable to start the pixel clock:
> %d\n", ret);
> + x1600_lcd_queue_event(lcd, crtc_state);
> + return;
> + }
> +
> + writel(readl(lcd->base + X1600_LCD_ST), lcd->base +
> X1600_LCD_CLR_ST);
Not fond of the writel(readl()) nesting here.
> + val = readl(lcd->base + X1600_LCD_COM_CONFIG) &
> ~X1600_LCD_COM_CONFIG_BURST_LEN;
> + writel(val | FIELD_PREP(X1600_LCD_COM_CONFIG_BURST_LEN,
> X1600_LCD_BURST_LEN_32),
> + lcd->base + X1600_LCD_COM_CONFIG);
> +
> + /* In the manual's order: timings, output format, then the interface.
> */
> + writel(x1600_lcd_timing(mode->hsync_end - mode->hsync_start, mode-
> >htotal),
> + lcd->base + X1600_LCD_TFT_HSYNC);
> + writel(x1600_lcd_timing(mode->vsync_end - mode->vsync_start, mode-
> >vtotal),
> + lcd->base + X1600_LCD_TFT_VSYNC);
> + writel(x1600_lcd_timing(hde, hde + mode->hdisplay),
> + lcd->base + X1600_LCD_TFT_HDE);
> + writel(x1600_lcd_timing(vde, vde + mode->vdisplay),
> + lcd->base + X1600_LCD_TFT_VDE);
> +
> + val = FIELD_PREP(X1600_LCD_TFT_COLOR_EVEN, lcd->color_order) |
> + FIELD_PREP(X1600_LCD_TFT_COLOR_ODD, lcd->color_order) |
> + FIELD_PREP(X1600_LCD_TFT_MODE, X1600_LCD_TFT_MODE_24BIT);
> + if (lcd->bus_flags & DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE)
> + val |= X1600_LCD_TFT_PIX_CLK_INV;
> + if (lcd->bus_flags & DRM_BUS_FLAG_DE_LOW)
> + val |= X1600_LCD_TFT_DE_DL;
> + /* The *_DL bits set the level outside the pulse. */
> + if (mode->flags & DRM_MODE_FLAG_NHSYNC)
> + val |= X1600_LCD_TFT_HSYNC_DL;
> + if (mode->flags & DRM_MODE_FLAG_NVSYNC)
> + val |= X1600_LCD_TFT_VSYNC_DL;
> + writel(val, lcd->base + X1600_LCD_TFT_TRAN_CFG);
> +
> + /* No dithering: it only applies to 888 sources. */
> + val = readl(lcd->base + X1600_LCD_DISP_COM);
> + val &= ~(X1600_LCD_DISP_COM_IF | X1600_LCD_DISP_COM_DITHER_EN);
> + val |= FIELD_PREP(X1600_LCD_DISP_COM_IF, X1600_LCD_DISP_COM_IF_TFT) |
> + X1600_LCD_DISP_COM_TFT_CLKGATE |
> X1600_LCD_DISP_COM_SLCD_CLKGATE;
> + writel(val, lcd->base + X1600_LCD_DISP_COM);
This whole block above looks like out of place. Why not do it in
x1600_lcd_encoder_atomic_mode_set()?
Bonus point, you wouldn't need the 'bus_flags' and 'color_order' variables.
> +
> + writel(x1600_lcd_desc_addr(lcd, lcd->cur), lcd->base +
> X1600_LCD_SRD_CHAIN_ADDR);
> + writel(1, lcd->base + X1600_LCD_SRD_CHAIN_START);
> + lcd->running = true;
> +
> + drm_crtc_vblank_on(crtc);
> + x1600_lcd_queue_event(lcd, crtc_state);
> +}
> +
> +static void x1600_lcd_crtc_atomic_disable(struct drm_crtc *crtc,
> + struct drm_atomic_commit *state)
> +{
> + struct x1600_lcd *lcd = to_x1600_lcd(crtc->dev);
> +
> + /* atomic_enable() could not start the clock: nothing runs. */
> + if (!lcd->running)
> + return;
> +
> + if (!x1600_lcd_stop(lcd))
> + dev_warn(lcd->drm.dev, "The controller did not stop\n");
> + lcd->running = false;
> +
> + spin_lock_irq(&crtc->dev->event_lock);
> + if (lcd->event) {
> + drm_crtc_send_vblank_event(crtc, lcd->event);
> + drm_crtc_vblank_put(crtc);
> + lcd->event = NULL;
> + }
> + spin_unlock_irq(&crtc->dev->event_lock);
> +
> + drm_crtc_vblank_off(crtc);
> + clk_disable_unprepare(lcd->clk);
> +}
> +
> +static int x1600_lcd_crtc_atomic_check(struct drm_crtc *crtc,
> + struct drm_atomic_commit *state)
> +{
> + struct drm_crtc_state *crtc_state =
> drm_atomic_get_new_crtc_state(state, crtc);
> +
> + /* Only when enabled: drm_atomic_helper_shutdown() disables both at
> once. */
> + if (!crtc_state->enable)
> + return 0;
> +
> + return drm_atomic_helper_check_crtc_primary_plane(crtc_state);
> +}
> +
> +static void x1600_lcd_crtc_atomic_flush(struct drm_crtc *crtc,
> + struct drm_atomic_commit *state)
> +{
> + struct drm_crtc_state *crtc_state =
> drm_atomic_get_new_crtc_state(state, crtc);
> +
> + /* The planes come first: a CRTC being enabled sends it once started.
> */
> + if (crtc_state->active && drm_atomic_crtc_needs_modeset(crtc_state))
> + return;
> +
> + x1600_lcd_queue_event(to_x1600_lcd(crtc->dev), crtc_state);
> +}
> +
> +static enum drm_mode_status
> +x1600_lcd_crtc_mode_valid(struct drm_crtc *crtc, const struct
> drm_display_mode *mode)
> +{
> + unsigned int pixels = mode->hdisplay * mode->vdisplay;
> +
> + if (mode->htotal > FIELD_MAX(X1600_LCD_TIMING_END) ||
> + mode->vtotal > FIELD_MAX(X1600_LCD_TIMING_END))
> + return MODE_BAD;
> +
> + /* The sync pulses' end points must not be 0. */
> + if (mode->hsync_end == mode->hsync_start)
> + return MODE_HSYNC_NARROW;
> + if (mode->vsync_end == mode->vsync_start)
> + return MODE_VSYNC_NARROW;
> +
> + if (pixels > X1600_LCD_MAX_PIXELS)
> + return MODE_MEM;
The max resolution is 1280x720 so you should check mode->hdisplay / mode-
>vdisplay individually. Otherwise bogus resolutions like 1920x480 would be
accepted.
> + if (pixels < X1600_LCD_MIN_PIXELS)
> + return MODE_BAD;
> +
> + return MODE_OK;
> +}
> +
> +static int x1600_lcd_enable_vblank(struct drm_crtc *crtc)
> +{
> + struct x1600_lcd *lcd = to_x1600_lcd(crtc->dev);
> +
> + x1600_lcd_irq_enable(lcd, X1600_LCD_ST_DISP_END, true);
> + return 0;
> +}
> +
> +static void x1600_lcd_disable_vblank(struct drm_crtc *crtc)
> +{
> + struct x1600_lcd *lcd = to_x1600_lcd(crtc->dev);
> +
> + x1600_lcd_irq_enable(lcd, X1600_LCD_ST_DISP_END, false);
> +}
> +
> +static irqreturn_t x1600_lcd_irq(int irq, void *data)
> +{
> + struct x1600_lcd *lcd = data;
> + u32 st = readl(lcd->base + X1600_LCD_INT_FLAG);
> +
> + if (!st)
> + return IRQ_NONE;
> +
> + writel(st, lcd->base + X1600_LCD_CLR_ST);
> + if (st & X1600_LCD_ST_STOP_SRD_ACK)
> + complete(&lcd->stopped);
> +
> + if (st & X1600_LCD_ST_DISP_END) {
> + drm_crtc_handle_vblank(&lcd->crtc);
> +
> + spin_lock(&lcd->drm.event_lock);
spin_lock() can sleep with CONFIG_PREEMPT_RT so it looks wrong in an IRQ
handler. I *think* you don't need to lock it here anyway, as the spinlock is
locked in other places using spin_lock_irq().
> + if (lcd->event &&
> + readl(lcd->base + X1600_LCD_CHAIN_SITE) == lcd-
> >event_desc) {
> + drm_crtc_send_vblank_event(&lcd->crtc, lcd->event);
> + drm_crtc_vblank_put(&lcd->crtc);
> + lcd->event = NULL;
> + }
> + spin_unlock(&lcd->drm.event_lock);
> + }
> +
> + return IRQ_HANDLED;
> +}
> +
> +static int x1600_lcd_plane_atomic_check(struct drm_plane *plane,
> + struct drm_atomic_commit *state)
> +{
> + struct drm_plane_state *plane_state =
> drm_atomic_get_new_plane_state(state, plane);
> + struct drm_crtc_state *crtc_state;
> + int ret;
> +
> + if (!plane_state->crtc)
> + return 0;
> +
> + crtc_state = drm_atomic_get_new_crtc_state(state, plane_state->crtc);
> + if (WARN_ON(!crtc_state))
> + return -EINVAL;
> +
> + /* The plane covers the whole screen: no scaling, no position. */
> + ret = drm_atomic_helper_check_plane_state(plane_state, crtc_state,
> + DRM_PLANE_NO_SCALING,
> + DRM_PLANE_NO_SCALING,
> + false, true);
> + if (ret)
> + return ret;
> +
> + /* The controller scans out a whole frame from the framebuffer. */
> + if (crtc_state->enable && !plane_state->visible)
> + return -EINVAL;
> +
> + return 0;
> +}
> +
> +static void x1600_lcd_plane_atomic_update(struct drm_plane *plane,
> + struct drm_atomic_commit *state)
> +{
> + struct x1600_lcd *lcd = to_x1600_lcd(plane->dev);
> + struct drm_plane_state *plane_state =
> drm_atomic_get_new_plane_state(state, plane);
> + struct drm_framebuffer *fb = plane_state->fb;
> + unsigned int next = lcd->running ? !lcd->cur : lcd->cur;
> + struct x1600_lcd_desc *desc = &lcd->desc[next];
> +
> + if (!fb)
> + return;
> +
> + desc->next = x1600_lcd_desc_addr(lcd, next);
> + desc->addr = drm_fb_dma_get_gem_addr(fb, plane_state, 0);
> + desc->stride = fb->pitches[0] / fb->format->cpp[0];
> + desc->cfg = FIELD_PREP(X1600_LCD_DESC_CFG_FMT,
> X1600_LCD_DESC_FMT_RGB565);
> + desc->ctrl = X1600_LCD_DESC_CTRL_EOD_MSK;
> +
> + if (!lcd->running)
> + return;
> +
> + /*
> + * The manual hands over a descriptor by writing SRD_CHAIN_ADDR; the
> + * vendor kernel also sets SRD_CHAIN_START, and so does this.
> + */
> + wmb();
> + writel(x1600_lcd_desc_addr(lcd, next), lcd->base +
> X1600_LCD_SRD_CHAIN_ADDR);
> + writel(1, lcd->base + X1600_LCD_SRD_CHAIN_START);
> + lcd->cur = next;
> +}
> +
> +/*
> + * Panels with 18 data lines take the top six bits of each colour of the
> + * 888 mode: in the 666 mode, the HiBy R1's panel shows wrong colours.
> + */
> +static int x1600_lcd_color_order(const struct drm_display_info *info)
> +{
> + if (!info->num_bus_formats)
> + return X1600_LCD_TFT_COLOR_RGB;
> +
> + switch (info->bus_formats[0]) {
> + case MEDIA_BUS_FMT_RGB666_1X18:
> + case MEDIA_BUS_FMT_RGB888_1X24:
> + return X1600_LCD_TFT_COLOR_RGB;
> + case MEDIA_BUS_FMT_BGR666_1X18:
> + case MEDIA_BUS_FMT_BGR888_1X24:
> + return X1600_LCD_TFT_COLOR_BGR;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int x1600_lcd_encoder_atomic_check(struct drm_encoder *encoder,
> + struct drm_crtc_state *crtc_state,
> + struct drm_connector_state
> *conn_state)
> +{
> + const struct drm_display_info *info = &conn_state->connector-
> >display_info;
> +
> + if (x1600_lcd_color_order(info) < 0) {
> + drm_dbg_kms(encoder->dev, "Unsupported bus format 0x%04x\n",
> + info->bus_formats[0]);
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +static void x1600_lcd_encoder_atomic_mode_set(struct drm_encoder *encoder,
> + struct drm_crtc_state
> *crtc_state,
> + struct drm_connector_state
> *conn_state)
> +{
> + struct x1600_lcd *lcd = to_x1600_lcd(encoder->dev);
> + const struct drm_display_info *info = &conn_state->connector-
> >display_info;
> +
> + lcd->bus_flags = info->bus_flags;
> + lcd->color_order = x1600_lcd_color_order(info);
> +}
> +
> +static const struct drm_plane_funcs x1600_lcd_plane_funcs = {
> + .update_plane = drm_atomic_helper_update_plane,
> + .disable_plane = drm_atomic_helper_disable_plane,
> + .atomic_create_state = drm_atomic_helper_plane_create_state,
> + .destroy = drm_plane_cleanup,
> + .atomic_duplicate_state =
> drm_atomic_helper_plane_duplicate_state,
> + .atomic_destroy_state = drm_atomic_helper_plane_destroy_state,
> +};
> +
> +static const struct drm_plane_helper_funcs x1600_lcd_plane_helper_funcs = {
> + .atomic_check = x1600_lcd_plane_atomic_check,
> + .atomic_update = x1600_lcd_plane_atomic_update,
> +};
> +
> +static const struct drm_crtc_funcs x1600_lcd_crtc_funcs = {
> + .set_config = drm_atomic_helper_set_config,
> + .page_flip = drm_atomic_helper_page_flip,
> + .atomic_create_state = drm_atomic_helper_crtc_create_state,
> + .destroy = drm_crtc_cleanup,
> + .atomic_duplicate_state =
> drm_atomic_helper_crtc_duplicate_state,
> + .atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
> + .enable_vblank = x1600_lcd_enable_vblank,
> + .disable_vblank = x1600_lcd_disable_vblank,
> +};
> +
> +static const struct drm_crtc_helper_funcs x1600_lcd_crtc_helper_funcs = {
> + .atomic_check = x1600_lcd_crtc_atomic_check,
> + .atomic_enable = x1600_lcd_crtc_atomic_enable,
> + .atomic_disable = x1600_lcd_crtc_atomic_disable,
> + .atomic_flush = x1600_lcd_crtc_atomic_flush,
> + .mode_valid = x1600_lcd_crtc_mode_valid,
> +};
> +
> +static const struct drm_encoder_helper_funcs x1600_lcd_encoder_helper_funcs =
> {
> + .atomic_check = x1600_lcd_encoder_atomic_check,
> + .atomic_mode_set = x1600_lcd_encoder_atomic_mode_set,
> +};
> +
> +/*
> + * drm_atomic_helper_commit_tail(), waiting for the flip instead of a vblank:
> + * the old framebuffer stays in use until the controller reads the new
> + * descriptor, which can be a frame later.
> + */
> +static void x1600_lcd_commit_tail(struct drm_atomic_commit *state)
> +{
> + struct drm_device *drm = state->dev;
> +
> + drm_atomic_helper_commit_modeset_disables(drm, state);
> + drm_atomic_helper_commit_planes(drm, state, 0);
> + drm_atomic_helper_commit_modeset_enables(drm, state);
> + drm_atomic_helper_fake_vblank(state);
> + drm_atomic_helper_commit_hw_done(state);
> + drm_atomic_helper_wait_for_flip_done(drm, state);
> + drm_atomic_helper_cleanup_planes(drm, state);
> +}
> +
> +static const struct drm_mode_config_funcs x1600_lcd_mode_config_funcs = {
> + .fb_create = drm_gem_fb_create,
> + .atomic_check = drm_atomic_helper_check,
> + .atomic_commit = drm_atomic_helper_commit,
> +};
> +
> +static const struct drm_mode_config_helper_funcs
> x1600_lcd_mode_config_helpers = {
> + .atomic_commit_tail = x1600_lcd_commit_tail,
> +};
> +
> +DEFINE_DRM_GEM_DMA_FOPS(x1600_lcd_fops);
> +
> +static const struct drm_driver x1600_lcd_driver = {
> + .driver_features = DRIVER_MODESET | DRIVER_GEM |
> DRIVER_ATOMIC,
> + .name = "ingenic-x1600-drm",
> + .desc = "DRM driver for the Ingenic X1600 display
> controller",
> + .major = 1,
> + .minor = 0,
> + .fops = &x1600_lcd_fops,
> + DRM_GEM_DMA_DRIVER_OPS,
> + DRM_FBDEV_DMA_DRIVER_OPS,
> +};
> +
> +static int x1600_lcd_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct drm_connector *connector;
> + struct drm_bridge *bridge;
> + struct drm_device *drm;
> + struct x1600_lcd *lcd;
> + int irq, ret;
> +
> + lcd = devm_drm_dev_alloc(dev, &x1600_lcd_driver, struct x1600_lcd,
> drm);
> + if (IS_ERR(lcd))
> + return PTR_ERR(lcd);
> +
> + drm = &lcd->drm;
> + platform_set_drvdata(pdev, lcd);
> + init_completion(&lcd->stopped);
> + spin_lock_init(&lcd->irq_lock);
> +
> + lcd->base = devm_platform_ioremap_resource(pdev, 0);
> + if (IS_ERR(lcd->base))
> + return PTR_ERR(lcd->base);
> +
> + lcd->clk = devm_clk_get(dev, NULL);
> + if (IS_ERR(lcd->clk))
> + return dev_err_probe(dev, PTR_ERR(lcd->clk), "Unable to get
> clock\n");
You enable it later in the probe, so you could just use devm_clk_get_enabled()
here.
> +
> + irq = platform_get_irq(pdev, 0);
> + if (irq < 0)
> + return irq;
> +
> + bridge = devm_drm_of_get_bridge(dev, dev->of_node, 0, 0);
> + if (IS_ERR(bridge))
> + return dev_err_probe(dev, PTR_ERR(bridge), "Unable to get the
> panel\n");
> +
> + lcd->desc = dmam_alloc_coherent(dev, 2 * sizeof(*lcd->desc),
> + &lcd->desc_phys, GFP_KERNEL);
> + if (!lcd->desc)
> + return -ENOMEM;
> +
> + ret = drmm_mode_config_init(drm);
> + if (ret)
> + return ret;
> +
> + /* Framebuffers, not modes: fbdev's double buffer is taller than
> 720p. */
> + drm->mode_config.max_width = FIELD_MAX(X1600_LCD_TIMING_END);
> + drm->mode_config.max_height = FIELD_MAX(X1600_LCD_TIMING_END);
> + drm->mode_config.funcs = &x1600_lcd_mode_config_funcs;
> + drm->mode_config.helper_private = &x1600_lcd_mode_config_helpers;
> +
> + ret = drm_universal_plane_init(drm, &lcd->plane, 1,
> &x1600_lcd_plane_funcs,
> + x1600_lcd_formats,
> ARRAY_SIZE(x1600_lcd_formats),
> + NULL, DRM_PLANE_TYPE_PRIMARY, NULL);
> + if (ret)
> + return ret;
> + drm_plane_helper_add(&lcd->plane, &x1600_lcd_plane_helper_funcs);
> +
> + ret = drm_crtc_init_with_planes(drm, &lcd->crtc, &lcd->plane, NULL,
> + &x1600_lcd_crtc_funcs, NULL);
> + if (ret)
> + return ret;
> + drm_crtc_helper_add(&lcd->crtc, &x1600_lcd_crtc_helper_funcs);
> +
> + ret = drmm_encoder_init(drm, &lcd->encoder, NULL,
> DRM_MODE_ENCODER_DPI, NULL);
> + if (ret)
> + return ret;
> + lcd->encoder.possible_crtcs = drm_crtc_mask(&lcd->crtc);
> + drm_encoder_helper_add(&lcd->encoder,
> &x1600_lcd_encoder_helper_funcs);
> +
> + ret = drm_bridge_attach(&lcd->encoder, bridge, NULL,
> + DRM_BRIDGE_ATTACH_NO_CONNECTOR);
> + if (ret)
> + return ret;
> +
> + connector = drm_bridge_connector_init(drm, &lcd->encoder);
> + if (IS_ERR(connector))
> + return PTR_ERR(connector);
> + drm_connector_attach_encoder(connector, &lcd->encoder);
> +
> + ret = drm_vblank_init(drm, 1);
> + if (ret)
> + return ret;
> +
> + drm_mode_config_reset(drm);
I saw a patch from Maxime replacing these - you should be calling
drm_mode_config_create_initial_state(drm) here (and handle the error).
> +
> + /* Take over from a boot framebuffer, and stop what it left running.
> */
> + ret = aperture_remove_all_conflicting_devices(x1600_lcd_driver.name);
> + if (ret)
> + return ret;
> +
> + ret = clk_prepare_enable(lcd->clk);
> + if (ret)
> + return ret;
> +
> + /* No interrupt from what the boot loader left enabled or pending. */
> + writel(0, lcd->base + X1600_LCD_INTC);
> + writel(readl(lcd->base + X1600_LCD_ST), lcd->base +
> X1600_LCD_CLR_ST);
> +
> + ret = devm_request_irq(dev, irq, x1600_lcd_irq, 0, dev_name(dev),
> lcd);
> + if (ret) {
> + clk_disable_unprepare(lcd->clk);
> + return dev_err_probe(dev, ret, "Unable to request the
> IRQ\n");
> + }
> +
> + /*
> + * Seen on the R1: after an underrun (TFT_UNDR) the controller can
> stop
> + * answering GEN_STP_SRD, and only a reset brings it back.
> + */
> + if (readl(lcd->base + X1600_LCD_ST) & X1600_LCD_ST_WORKING &&
> !x1600_lcd_stop(lcd))
> + dev_err(dev, "The controller did not stop%s\n",
> + readl(lcd->base + X1600_LCD_ST) &
> X1600_LCD_ST_TFT_UNDR ?
> + " after an underrun" : "");
> + clk_disable_unprepare(lcd->clk);
> +
> + ret = drm_dev_register(drm, 0);
> + if (ret)
> + return ret;
> +
> + drm_client_setup_with_fourcc(drm, DRM_FORMAT_RGB565);
> + return 0;
> +}
> +
> +static void x1600_lcd_remove(struct platform_device *pdev)
> +{
> + struct x1600_lcd *lcd = platform_get_drvdata(pdev);
> +
> + drm_dev_unregister(&lcd->drm);
> + drm_atomic_helper_shutdown(&lcd->drm);
> +}
> +
> +static void x1600_lcd_shutdown(struct platform_device *pdev)
> +{
> + struct x1600_lcd *lcd = platform_get_drvdata(pdev);
> +
> + drm_atomic_helper_shutdown(&lcd->drm);
> +}
> +
> +static const struct of_device_id x1600_lcd_of_match[] = {
> + { .compatible = "ingenic,x1600-dpu" },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, x1600_lcd_of_match);
> +
> +static struct platform_driver x1600_lcd_platform_driver = {
> + .driver = {
> + .name = "ingenic-x1600-drm",
> + .of_match_table = x1600_lcd_of_match,
> + },
> + .probe = x1600_lcd_probe,
> + .remove = x1600_lcd_remove,
> + .shutdown = x1600_lcd_shutdown,
> +};
> +module_platform_driver(x1600_lcd_platform_driver);
> +
> +MODULE_AUTHOR("Olivier Louvignes <olivier@mgcrea.io>");
> +MODULE_DESCRIPTION("DRM driver for the Ingenic X1600 display controller");
> +MODULE_LICENSE("GPL");
Cheers,
-Paul
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 3/5] drm/ingenic: Add a driver for the X1600 display controller
2026-11-06 13:34 ` Paul Cercueil
@ 2026-11-06 14:38 ` Olivier Louvignes
2026-11-06 15:06 ` Paul Cercueil
0 siblings, 1 reply; 9+ messages in thread
From: Olivier Louvignes @ 2026-11-06 14:38 UTC (permalink / raw)
To: Paul Cercueil, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, Neil Armstrong
Cc: David Airlie, Simona Vetter, Jessica Zhang, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, H . Nikolaus Schaller,
Thomas Petazzoni, dri-devel, devicetree, linux-mips,
linux-kernel
Hi Paul,
On Fri, Nov 06, 2026 at 02:34:08PM +0100, Paul Cercueil wrote:
> > + struct drm_display_mode *mode = &crtc->state->adjusted_mode;
>
> This bothers me a bit, shouldn't it be using 'crtc_state'?
Yes. v4 takes the mode from crtc_state.
> > + writel(readl(lcd->base + X1600_LCD_ST), lcd->base +
> > X1600_LCD_CLR_ST);
>
> Not fond of the writel(readl()) nesting here.
OK, a local in v4, here and in probe.
> > + writel(val, lcd->base + X1600_LCD_DISP_COM);
>
> This whole block above looks like out of place. Why not do it in
> x1600_lcd_encoder_atomic_mode_set()?
>
> Bonus point, you wouldn't need the 'bus_flags' and 'color_order' variables.
The controller only takes register writes while its clock runs, and the
clock is off when atomic_mode_set() is called: on a full modeset the
CRTC's atomic_disable() has stopped it, and atomic_enable() starts it
again afterwards. I checked it on the R1: with the clock gated (CLKGR0
bit 23), every register reads 0x80, and a write to TFT_HSYNC is lost,
the register still holds the old value once the clock is back.
So moving the block means starting the clock in atomic_mode_set() too,
or leaving it running while the CRTC is off. I'd rather keep the writes
in atomic_enable(), next to clk_prepare_enable(), and look the bus
format up in the new connector state there, so the two fields go
anyway. Would that work for you?
> > + if (pixels > X1600_LCD_MAX_PIXELS)
> > + return MODE_MEM;
>
> The max resolution is 1280x720 so you should check mode->hdisplay / mode-
> >vdisplay individually. Otherwise bogus resolutions like 1920x480 would be
> accepted.
Right for 1920x480, but 720 per side would refuse the R1's own panel,
which is 480x800 in portrait. The manual only says "frame size up to
720p". v4 can bound each side by 1280 and keep the 1280x720 pixel count,
would that do?
> > + spin_lock(&lcd->drm.event_lock);
>
> spin_lock() can sleep with CONFIG_PREEMPT_RT so it looks wrong in an IRQ
> handler. I *think* you don't need to lock it here anyway, as the spinlock is
> locked in other places using spin_lock_irq().
On PREEMPT_RT the handler doesn't run in hard IRQ context:
force_irqthreads() is always true there, and the IRQ is requested
without IRQF_NO_THREAD, so the handler runs in a thread, where a
spinlock_t is fine (Documentation/locking/locktypes.rst).
And it needs the lock: drm_crtc_send_vblank_event() wants the caller to
hold event_lock, and lcd->event is shared with queue_event() and
atomic_disable(). Their spin_lock_irq() only keeps the handler off the
CPU they run on. armada, omapdrm and malidp take event_lock the same way
in their handlers. So I'd keep it.
> You enable it later in the probe, so you could just use devm_clk_get_enabled()
> here.
Probe only enables it to stop what the boot loader left running, and
disables it again before drm_dev_register(). From then on atomic_enable()
and atomic_disable() gate it. devm_clk_get_enabled() would keep it
running for as long as the driver is bound, display off included.
> I saw a patch from Maxime replacing these - you should be calling
> drm_mode_config_create_initial_state(drm) here (and handle the error).
Will do, it's in drm-misc-next already.
I'll leave v3 a few more days for the drm-misc side before sending v4.
Thanks for the review,
Olivier
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 3/5] drm/ingenic: Add a driver for the X1600 display controller
2026-11-06 14:38 ` Olivier Louvignes
@ 2026-11-06 15:06 ` Paul Cercueil
0 siblings, 0 replies; 9+ messages in thread
From: Paul Cercueil @ 2026-11-06 15:06 UTC (permalink / raw)
To: Olivier Louvignes, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, Neil Armstrong
Cc: David Airlie, Simona Vetter, Jessica Zhang, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, H . Nikolaus Schaller,
Thomas Petazzoni, dri-devel, devicetree, linux-mips,
linux-kernel
Le vendredi 06 novembre 2026 à 15:38 +0100, Olivier Louvignes a écrit :
> Hi Paul,
>
> On Fri, Nov 06, 2026 at 02:34:08PM +0100, Paul Cercueil wrote:
> > > + struct drm_display_mode *mode = &crtc->state->adjusted_mode;
> >
> > This bothers me a bit, shouldn't it be using 'crtc_state'?
>
> Yes. v4 takes the mode from crtc_state.
>
> > > + writel(readl(lcd->base + X1600_LCD_ST), lcd->base +
> > > X1600_LCD_CLR_ST);
> >
> > Not fond of the writel(readl()) nesting here.
>
> OK, a local in v4, here and in probe.
>
> > > + writel(val, lcd->base + X1600_LCD_DISP_COM);
> >
> > This whole block above looks like out of place. Why not do it in
> > x1600_lcd_encoder_atomic_mode_set()?
> >
> > Bonus point, you wouldn't need the 'bus_flags' and 'color_order' variables.
>
> The controller only takes register writes while its clock runs, and the
> clock is off when atomic_mode_set() is called: on a full modeset the
> CRTC's atomic_disable() has stopped it, and atomic_enable() starts it
> again afterwards. I checked it on the R1: with the clock gated (CLKGR0
> bit 23), every register reads 0x80, and a write to TFT_HSYNC is lost,
> the register still holds the old value once the clock is back.
>
> So moving the block means starting the clock in atomic_mode_set() too,
> or leaving it running while the CRTC is off. I'd rather keep the writes
> in atomic_enable(), next to clk_prepare_enable(), and look the bus
> format up in the new connector state there, so the two fields go
> anyway. Would that work for you?
Is there really a point in enabling the clock on-demand? Why not keep it enabled
always? It makes the driver more complex and I don't see a real benefit.
>
> > > + if (pixels > X1600_LCD_MAX_PIXELS)
> > > + return MODE_MEM;
> >
> > The max resolution is 1280x720 so you should check mode->hdisplay / mode-
> > > vdisplay individually. Otherwise bogus resolutions like 1920x480 would be
> > accepted.
>
> Right for 1920x480, but 720 per side would refuse the R1's own panel,
> which is 480x800 in portrait. The manual only says "frame size up to
> 720p". v4 can bound each side by 1280 and keep the 1280x720 pixel count,
> would that do?
Ok. Then I guess keep the current check.
> > > + spin_lock(&lcd->drm.event_lock);
> >
> > spin_lock() can sleep with CONFIG_PREEMPT_RT so it looks wrong in an IRQ
> > handler. I *think* you don't need to lock it here anyway, as the spinlock is
> > locked in other places using spin_lock_irq().
>
> On PREEMPT_RT the handler doesn't run in hard IRQ context:
> force_irqthreads() is always true there, and the IRQ is requested
> without IRQF_NO_THREAD, so the handler runs in a thread, where a
> spinlock_t is fine (Documentation/locking/locktypes.rst).
>
> And it needs the lock: drm_crtc_send_vblank_event() wants the caller to
> hold event_lock, and lcd->event is shared with queue_event() and
> atomic_disable(). Their spin_lock_irq() only keeps the handler off the
> CPU they run on. armada, omapdrm and malidp take event_lock the same way
> in their handlers. So I'd keep it.
Ok.
> > You enable it later in the probe, so you could just use
> > devm_clk_get_enabled()
> > here.
>
> Probe only enables it to stop what the boot loader left running, and
> disables it again before drm_dev_register(). From then on atomic_enable()
> and atomic_disable() gate it. devm_clk_get_enabled() would keep it
> running for as long as the driver is bound, display off included.
Yeah, I forgot that the devm action would cause a clk_disable_unprepare() to be
called on unprobe, which wouldn't work here. So ignore that.
As for the clock running for as long as the driver is bound, I don't think
that's much of a problem.
> > I saw a patch from Maxime replacing these - you should be calling
> > drm_mode_config_create_initial_state(drm) here (and handle the error).
>
> Will do, it's in drm-misc-next already.
>
> I'll leave v3 a few more days for the drm-misc side before sending v4.
Great.
Cheers,
-Paul
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2026-11-06 15:06 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-11-06 11:19 [PATCH v3 0/5] drm/ingenic: Add the X1600 display controller and the HiBy R1 panel Olivier Louvignes
2026-11-06 11:19 ` [PATCH v3 1/5] dt-bindings: vendor-prefixes: Add HiBy Olivier Louvignes
2026-11-06 11:19 ` [PATCH v3 2/5] dt-bindings: display: Add the Ingenic X1600 display controller Olivier Louvignes
2026-11-06 11:19 ` [PATCH v3 3/5] drm/ingenic: Add a driver for the " Olivier Louvignes
2026-11-06 13:34 ` Paul Cercueil
2026-11-06 14:38 ` Olivier Louvignes
2026-11-06 15:06 ` Paul Cercueil
2026-11-06 11:19 ` [PATCH v3 4/5] dt-bindings: display: panel: Add the HiBy LG35583 Olivier Louvignes
2026-11-06 11:20 ` [PATCH v3 5/5] drm/panel: " Olivier Louvignes
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®