[PATCH 1/4] firmware: Add request/release_firmware() calls

[PATCH 1/4] firmware: Add request/release_firmware() calls

[Philipp Zabel]

Add request_firmware() and release_firmware() calls that allow drivers
to load a firmware file. Also move the struct firmware definition from
remoteproc.h into firmware.h.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 common/firmware.c          | 33 +++++++++++++++++++++++++++++++++
 include/firmware.h         | 16 ++++++++++++++++
 include/linux/remoteproc.h |  5 -----
 3 files changed, 49 insertions(+), 5 deletions(-)

diff --git a/common/firmware.c b/common/firmware.c
index e4ad6ac867b0..c6668fd528b6 100644
--- a/common/firmware.c
+++ b/common/firmware.c
@@ -17,6 +17,7 @@
 #include <linux/stat.h>
 #include <linux/err.h>
 #include <uncompress.h>
+#include <unistd.h>
 #include <filetype.h>
 
 #define BUFSIZ 4096
@@ -304,6 +305,38 @@ out:
 	return ret;
 }
 
+/*
+ * request_firmware - load a firmware to a device
+ */
+int request_firmware(const struct firmware **out, const char *fw_name, struct device *dev)
+{
+	char fw_path[PATH_MAX + 1];
+	struct firmware *fw;
+	int ret;
+
+	fw = kzalloc(sizeof(struct firmware), GFP_KERNEL);
+	if (!fw)
+		return -ENOMEM;
+
+	snprintf(fw_path, sizeof(fw_path), "%s/%s", firmware_path, fw_name);
+
+	ret = read_file_2(fw_path, &fw->size, (void *)&fw->data, FILESIZE_MAX);
+	if (ret) {
+		kfree(fw);
+		return ret;
+	}
+
+	*out = fw;
+
+	return 0;
+}
+
+void release_firmware(const struct firmware *fw)
+{
+	kfree_const(fw->data);
+	kfree_const(fw);
+}
+
 static int firmware_init(void)
 {
 	firmware_path = strdup("/env/firmware");
diff --git a/include/firmware.h b/include/firmware.h
index 05433f2f7858..b4fa40a3ab44 100644
--- a/include/firmware.h
+++ b/include/firmware.h
@@ -13,6 +13,11 @@
 #include <debug_ll.h>
 #include <linux/kernel.h>
 
+struct firmware {
+	size_t size;
+	const u8 *data;
+};
+
 struct firmware_handler {
 	char *id; /* unique identifier for this firmware device */
 	char *model; /* description for this device */
@@ -37,6 +42,8 @@ struct firmware_mgr *firmwaremgr_find_by_node(struct device_node *np);
 int firmwaremgr_load_file(struct firmware_mgr *, const char *path);
 char *firmware_get_searchpath(void);
 void firmware_set_searchpath(const char *path);
+int request_firmware(const struct firmware **fw, const char *fw_name, struct device *dev);
+void release_firmware(const struct firmware *fw);
 #else
 static inline struct firmware_mgr *firmwaremgr_find_by_node(struct device_node *np)
 {
@@ -57,6 +64,15 @@ static inline void firmware_set_searchpath(const char *path)
 {
 }
 
+static inline int request_firmware(const struct firmware **fw, const char *fw_name,
+				   struct device *dev)
+{
+	return -EOPNOTSUPP;
+}
+
+static inline void release_firmware(const struct firmware *fw)
+{
+}
 #endif
 
 void firmwaremgr_list_handlers(void);
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index 170fff7987fb..33fe2f81b748 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -18,11 +18,6 @@ struct resource_table {
 	u32 offset[0];
 } __packed;
 
-struct firmware {
-	size_t size;
-	const u8 *data;
-};
-
 struct rproc;
 
 struct rproc_ops {
-- 
2.39.2

[PATCH 2/4] video: Add of_get_display_timing

[Philipp Zabel]

Add a port of the kernel of_get_display_timing() that writes a
struct fb_videomode instead of struct display_timing, which we
don't have.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 drivers/video/of_display_timing.c | 22 ++++++++++++++++++++++
 include/fb.h                      |  2 ++
 2 files changed, 24 insertions(+)

diff --git a/drivers/video/of_display_timing.c b/drivers/video/of_display_timing.c
index 6fe1e1b08b6a..6082d454932d 100644
--- a/drivers/video/of_display_timing.c
+++ b/drivers/video/of_display_timing.c
@@ -98,6 +98,28 @@ static int of_parse_display_timing(const struct device_node *np,
 	return 0;
 }
 
+/**
+ * of_get_display_timing - parse a display_timing entry
+ * @np: device_node with the timing subnode
+ * @name: name of the timing node
+ * @mode: fb_videomode struct to fill
+ **/
+int of_get_display_timing(const struct device_node *np, const char *name,
+			  struct fb_videomode *mode)
+{
+	struct device_node *timing_np;
+
+	if (!np)
+		return -EINVAL;
+
+	timing_np = of_get_child_by_name(np, name);
+	if (!timing_np)
+		return -ENOENT;
+
+	return of_parse_display_timing(timing_np, mode);
+}
+EXPORT_SYMBOL_GPL(of_get_display_timing);
+
 /**
  * of_get_display_timings - parse all display_timing entries from a device_node
  * @np: device_node with the subnodes
diff --git a/include/fb.h b/include/fb.h
index bf5f688342fd..15bb74b99576 100644
--- a/include/fb.h
+++ b/include/fb.h
@@ -147,6 +147,8 @@ struct fb_info {
 	int shadowfb;
 };
 
+int of_get_display_timing(const struct device_node *np, const char *name,
+			  struct fb_videomode *mode);
 struct display_timings *of_get_display_timings(struct device_node *np);
 void display_timings_release(struct display_timings *);
 
-- 
2.39.2

[PATCH 3/4] video: add MIPI DBI framebuffer helpers

[Philipp Zabel]

Port helper functions for the panel-mipi-dbi driver from the Linux
kernel.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 drivers/video/mipi_dbi.c | 242 +++++++++++++++++++++++++++++++++++++++
 include/video/mipi_dbi.h |  63 ++++++++++
 2 files changed, 305 insertions(+)

diff --git a/drivers/video/mipi_dbi.c b/drivers/video/mipi_dbi.c
index 50d2fc4b29b9..9aa16abb1c9b 100644
--- a/drivers/video/mipi_dbi.c
+++ b/drivers/video/mipi_dbi.c
@@ -13,6 +13,7 @@
 #include <gpiod.h>
 #include <regulator.h>
 #include <spi/spi.h>
+#include <video/backlight.h>
 #include <video/mipi_dbi.h>
 
 #include <video/vpl.h>
@@ -196,6 +197,178 @@ int mipi_dbi_command_stackbuf(struct mipi_dbi *dbi, u8 cmd, const u8 *data,
 }
 EXPORT_SYMBOL(mipi_dbi_command_stackbuf);
 
+/**
+ * mipi_dbi_buf_copy - Copy a framebuffer, transforming it if necessary
+ * @dst: The destination buffer
+ * @info: The source framebuffer info
+ * @swap: When true, swap MSB/LSB of 16-bit values
+ *
+ * Returns:
+ * Zero on success, negative error code on failure.
+ */
+static void mipi_dbi_buf_copy(void *dst, struct fb_info *info, bool swap)
+{
+	u16 *src = (u16 *)info->screen_base;
+	u16 *dst16 = dst;
+	size_t len = info->xres * info->yres;
+	int i;
+
+	if (swap) {
+		for (i = 0; i < len; i++) {
+			*dst16++ = *src << 8 | *src >> 8;
+			src++;
+		}
+	} else {
+		memcpy(dst, src, len * 2);
+	}
+}
+
+static void mipi_dbi_set_window_address(struct mipi_dbi_dev *dbidev,
+					unsigned int xs, unsigned int xe,
+					unsigned int ys, unsigned int ye)
+{
+	struct mipi_dbi *dbi = &dbidev->dbi;
+
+	xs += dbidev->mode.left_margin;
+	xe += dbidev->mode.left_margin;
+	ys += dbidev->mode.upper_margin;
+	ye += dbidev->mode.upper_margin;
+
+	mipi_dbi_command(dbi, MIPI_DCS_SET_COLUMN_ADDRESS, (xs >> 8) & 0xff,
+			 xs & 0xff, (xe >> 8) & 0xff, xe & 0xff);
+	mipi_dbi_command(dbi, MIPI_DCS_SET_PAGE_ADDRESS, (ys >> 8) & 0xff,
+			 ys & 0xff, (ye >> 8) & 0xff, ye & 0xff);
+}
+
+static void mipi_dbi_fb_dirty(struct mipi_dbi_dev *dbidev, struct fb_info *info)
+{
+	struct mipi_dbi *dbi = &dbidev->dbi;
+	u16 width = info->xres;
+	u16 height = info->yres;
+	size_t len = width * height * 2;
+	int ret;
+
+	mipi_dbi_buf_copy(dbidev->tx_buf, info, dbi->swap_bytes);
+
+	mipi_dbi_set_window_address(dbidev, 0, width - 1, 0, height - 1);
+
+	ret = mipi_dbi_command_buf(dbi, MIPI_DCS_WRITE_MEMORY_START, dbidev->tx_buf, len);
+	if (ret)
+		pr_err_once("Failed to update display %d\n", ret);
+}
+
+/**
+ * mipi_dbi_enable_flush - MIPI DBI enable helper
+ * @dbidev: MIPI DBI device structure
+ * @crtc_state: CRTC state
+ * @plane_state: Plane state
+ *
+ * Flushes the whole framebuffer and enables the backlight. Drivers can use this
+ * in their &drm_simple_display_pipe_funcs->enable callback.
+ *
+ * Note: Drivers which don't use mipi_dbi_pipe_update() because they have custom
+ * framebuffer flushing, can't use this function since they both use the same
+ * flushing code.
+ */
+void mipi_dbi_enable_flush(struct mipi_dbi_dev *dbidev,
+			   struct fb_info *info)
+{
+	mipi_dbi_fb_dirty(dbidev, info);
+
+	if (dbidev->backlight)
+		backlight_set_brightness_default(dbidev->backlight);
+}
+EXPORT_SYMBOL(mipi_dbi_enable_flush);
+
+
+static void mipi_dbi_blank(struct mipi_dbi_dev *dbidev)
+{
+	u16 height = dbidev->mode.xres;
+	u16 width = dbidev->mode.yres;
+	struct mipi_dbi *dbi = &dbidev->dbi;
+	size_t len = width * height * 2;
+
+	memset(dbidev->tx_buf, 0, len);
+
+	mipi_dbi_set_window_address(dbidev, 0, width - 1, 0, height - 1);
+	mipi_dbi_command_buf(dbi, MIPI_DCS_WRITE_MEMORY_START, dbidev->tx_buf, len);
+}
+
+/**
+ * mipi_dbi_fb_disable - MIPI DBI framebuffer disable helper
+ * @info: Framebuffer info
+ *
+ * This function disables backlight if present, if not the display memory is
+ * blanked. The regulator is disabled if in use. Drivers can use this as their
+ * &fb_ops->fb_disable callback.
+ */
+void mipi_dbi_fb_disable(struct fb_info *info)
+{
+	struct mipi_dbi_dev *dbidev = container_of(info, struct mipi_dbi_dev, info);
+
+	if (dbidev->backlight)
+		backlight_set_brightness(dbidev->backlight, 0);
+	else
+		mipi_dbi_blank(dbidev);
+
+	if (dbidev->regulator)
+		regulator_disable(dbidev->regulator);
+	if (dbidev->io_regulator)
+		regulator_disable(dbidev->io_regulator);
+}
+EXPORT_SYMBOL(mipi_dbi_fb_disable);
+
+void mipi_dbi_fb_flush(struct fb_info *info)
+{
+	struct mipi_dbi_dev *dbidev = container_of(info, struct mipi_dbi_dev, info);
+
+	mipi_dbi_fb_dirty(dbidev, info);
+}
+EXPORT_SYMBOL(mipi_dbi_fb_flush);
+
+/**
+ * mipi_dbi_dev_init - MIPI DBI device initialization
+ * @dbidev: MIPI DBI device structure to initialize
+ * @ops: Framebuffer operations
+ * @mode: Display mode
+ *
+ * This function sets up a &fb_info with one fixed &fb_videomode.
+ * Additionally &mipi_dbi.tx_buf is allocated.
+ *
+ * Supported format: RGB565.
+ *
+ * Returns:
+ * Zero on success, negative error code on failure.
+ */
+int mipi_dbi_dev_init(struct mipi_dbi_dev *dbidev, struct fb_ops *ops,
+		      struct fb_videomode *mode)
+{
+	struct fb_info *info = &dbidev->info;
+
+	info->mode = mode;
+	info->fbops = ops;
+	info->dev.parent = dbidev->dev;
+
+	info->xres = mode->xres;
+	info->yres = mode->yres;
+	info->bits_per_pixel = 16;
+	info->line_length = info->xres * (info->bits_per_pixel >> 3);
+
+	info->screen_size = info->line_length * info->yres;
+	info->screen_base = kzalloc(info->screen_size, GFP_KERNEL);
+
+	info->red.length = 5;
+	info->red.offset = 11;
+	info->green.length = 6;
+	info->green.offset = 5;
+	info->blue.length = 5;
+	info->blue.offset = 0;
+
+	dbidev->tx_buf = kzalloc(mode->xres * mode->yres * 2, GFP_KERNEL);
+
+	return 0;
+}
+
 /**
  * mipi_dbi_hw_reset - Hardware reset of controller
  * @dbi: MIPI DBI structure
@@ -246,6 +419,75 @@ bool mipi_dbi_display_is_on(struct mipi_dbi *dbi)
 }
 EXPORT_SYMBOL(mipi_dbi_display_is_on);
 
+static int mipi_dbi_poweron_reset_conditional(struct mipi_dbi_dev *dbidev, bool cond)
+{
+	struct device *dev = dbidev->dev;
+	struct mipi_dbi *dbi = &dbidev->dbi;
+	int ret;
+
+	if (dbidev->regulator) {
+		ret = regulator_enable(dbidev->regulator);
+		if (ret) {
+			dev_err(dev, "Failed to enable regulator (%d)\n", ret);
+			return ret;
+		}
+	}
+
+	if (dbidev->io_regulator) {
+		ret = regulator_enable(dbidev->io_regulator);
+		if (ret) {
+			dev_err(dev, "Failed to enable I/O regulator (%d)\n", ret);
+			if (dbidev->regulator)
+				regulator_disable(dbidev->regulator);
+			return ret;
+		}
+	}
+
+	if (cond && mipi_dbi_display_is_on(dbi))
+		return 1;
+
+	mipi_dbi_hw_reset(dbi);
+	ret = mipi_dbi_command(dbi, MIPI_DCS_SOFT_RESET);
+	if (ret) {
+		dev_err(dev, "Failed to send reset command (%d)\n", ret);
+		if (dbidev->regulator)
+			regulator_disable(dbidev->regulator);
+		if (dbidev->io_regulator)
+			regulator_disable(dbidev->io_regulator);
+		return ret;
+	}
+
+	/*
+	 * If we did a hw reset, we know the controller is in Sleep mode and
+	 * per MIPI DSC spec should wait 5ms after soft reset. If we didn't,
+	 * we assume worst case and wait 120ms.
+	 */
+	if (dbi->reset)
+		mdelay(5);
+	else
+		mdelay(120);
+
+	return 0;
+}
+
+/**
+ * mipi_dbi_poweron_conditional_reset - MIPI DBI poweron and conditional reset
+ * @dbidev: MIPI DBI device structure
+ *
+ * This function enables the regulator if used and if the display is off, it
+ * does a hardware and software reset. If mipi_dbi_display_is_on() determines
+ * that the display is on, no reset is performed.
+ *
+ * Returns:
+ * Zero if the controller was reset, 1 if the display was already on, or a
+ * negative error code.
+ */
+int mipi_dbi_poweron_conditional_reset(struct mipi_dbi_dev *dbidev)
+{
+	return mipi_dbi_poweron_reset_conditional(dbidev, true);
+}
+EXPORT_SYMBOL(mipi_dbi_poweron_conditional_reset);
+
 #if IS_ENABLED(CONFIG_SPI)
 
 /**
diff --git a/include/video/mipi_dbi.h b/include/video/mipi_dbi.h
index 54526006935f..917f7ddd597f 100644
--- a/include/video/mipi_dbi.h
+++ b/include/video/mipi_dbi.h
@@ -11,6 +11,7 @@
 #include <linux/types.h>
 #include <spi/spi.h>
 #include <driver.h>
+#include <fb.h>
 
 struct regulator;
 struct fb_videomode;
@@ -55,6 +56,61 @@ struct mipi_dbi {
 	struct list_head list;
 };
 
+/**
+ * struct mipi_dbi_dev - MIPI DBI device
+ */
+struct mipi_dbi_dev {
+	/**
+	 * @dev: Device
+	 */
+	struct device *dev;
+
+	/**
+	 * @info: Framebuffer info
+	 */
+	struct fb_info info;
+
+	/**
+	 * @mode: Fixed display mode
+	 */
+	struct fb_videomode mode;
+
+	/**
+	 * @tx_buf: Buffer used for transfer (copy clip rect area)
+	 */
+	u8 *tx_buf;
+
+	/**
+	 * @backlight_node: backlight device node (optional)
+	 */
+	struct device_node *backlight_node;
+
+	/**
+	 * @backlight: backlight device (optional)
+	 */
+	struct backlight_device *backlight;
+
+	/**
+	 * @regulator: power regulator (Vdd) (optional)
+	 */
+	struct regulator *regulator;
+
+	/**
+	 * @io_regulator: I/O power regulator (Vddi) (optional)
+	 */
+	struct regulator *io_regulator;
+
+	/**
+	 * @dbi: MIPI DBI interface
+	 */
+	struct mipi_dbi dbi;
+
+	/**
+	 * @driver_private: Driver private data.
+	 */
+	void *driver_private;
+};
+
 static inline const char *mipi_dbi_name(struct mipi_dbi *dbi)
 {
 	return dev_name(&dbi->spi->dev);
@@ -62,8 +118,15 @@ static inline const char *mipi_dbi_name(struct mipi_dbi *dbi)
 
 int mipi_dbi_spi_init(struct spi_device *spi, struct mipi_dbi *dbi,
 		      int dc);
+int mipi_dbi_dev_init(struct mipi_dbi_dev *dbidev,
+		      struct fb_ops *ops, struct fb_videomode *mode);
+void mipi_dbi_fb_flush(struct fb_info *info);
+void mipi_dbi_enable_flush(struct mipi_dbi_dev *dbidev,
+			   struct fb_info *info);
+void mipi_dbi_fb_disable(struct fb_info *info);
 void mipi_dbi_hw_reset(struct mipi_dbi *dbi);
 bool mipi_dbi_display_is_on(struct mipi_dbi *dbi);
+int mipi_dbi_poweron_conditional_reset(struct mipi_dbi_dev *dbidev);
 
 u32 mipi_dbi_spi_cmd_max_speed(struct spi_device *spi, size_t len);
 int mipi_dbi_spi_transfer(struct spi_device *spi, u32 speed_hz,
-- 
2.39.2

[PATCH 4/4] video: Add MIPI DBI compatible SPI driver

[Philipp Zabel]

Port the panel-mipi-dbi driver from the Linux kernel. It works with most
MIPI DBI compatible SPI panels.
This avoids adding a driver for every new MIPI DBI compatible controller
that is to be used by Barebox. The 'compatible' Device Tree property
with a '.bin' suffix will be used to load a firmware file that contains
the controller configuration.

Example (driver will load sainsmart18.bin):

display@0 {
        compatible = "sainsmart18", "panel-mipi-dbi-spi";
...
};

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 drivers/video/Kconfig          |  12 ++
 drivers/video/Makefile         |   1 +
 drivers/video/panel-mipi-dbi.c | 330 +++++++++++++++++++++++++++++++++
 3 files changed, 343 insertions(+)
 create mode 100644 drivers/video/panel-mipi-dbi.c

diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
index 01bdaf47bfca..0e710bb25f79 100644
--- a/drivers/video/Kconfig
+++ b/drivers/video/Kconfig
@@ -198,4 +198,16 @@ config DRIVER_VIDEO_PANEL_ILITEK_ILI9341
 	  QVGA (240x320) RGB panels. support serial & parallel rgb
 	  interface.
 
+config DRIVER_VIDEO_PANEL_MIPI_DBI
+	tristate "DRM support for MIPI DBI compatible panels"
+	depends on OFTREE && SPI
+	select DRIVER_VIDEO_MIPI_DBI
+	select FIRMWARE
+	select VIDEO_VPL
+	help
+          Say Y here if you want to enable support for MIPI DBI compatible
+          panels. The controller command setup can be provided using a
+          firmware file. For more information see
+          https://github.com/notro/panel-mipi-dbi/wiki.
+
 endif
diff --git a/drivers/video/Makefile b/drivers/video/Makefile
index d50d2d3ba562..7718b63f448a 100644
--- a/drivers/video/Makefile
+++ b/drivers/video/Makefile
@@ -11,6 +11,7 @@ obj-$(CONFIG_DRIVER_VIDEO_TC358767) += tc358767.o
 obj-$(CONFIG_DRIVER_VIDEO_SIMPLE_PANEL) += simple-panel.o
 obj-$(CONFIG_DRIVER_VIDEO_MIPI_DBI) += mipi_dbi.o
 obj-$(CONFIG_DRIVER_VIDEO_PANEL_ILITEK_ILI9341) += panel-ilitek-ili9341.o
+obj-$(CONFIG_DRIVER_VIDEO_PANEL_MIPI_DBI) += panel-mipi-dbi.o
 
 obj-$(CONFIG_DRIVER_VIDEO_ATMEL) += atmel_lcdfb.o atmel_lcdfb_core.o
 obj-$(CONFIG_DRIVER_VIDEO_ATMEL_HLCD) += atmel_hlcdfb.o atmel_lcdfb_core.o
diff --git a/drivers/video/panel-mipi-dbi.c b/drivers/video/panel-mipi-dbi.c
new file mode 100644
index 000000000000..ac6f585be32c
--- /dev/null
+++ b/drivers/video/panel-mipi-dbi.c
@@ -0,0 +1,330 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * DRM driver for MIPI DBI compatible display panels
+ *
+ * Copyright 2022 Noralf Trønnes
+ */
+
+#include <clock.h>
+#include <common.h>
+#include <fb.h>
+#include <firmware.h>
+#include <gpiod.h>
+#include <linux/printk.h>
+#include <of.h>
+#include <regulator.h>
+#include <spi/spi.h>
+
+#include <video/backlight.h>
+#include <video/mipi_dbi.h>
+#include <video/mipi_display.h>
+
+static const u8 panel_mipi_dbi_magic[15] = { 'M', 'I', 'P', 'I', ' ', 'D', 'B', 'I',
+					     0, 0, 0, 0, 0, 0, 0 };
+
+/*
+ * The display controller configuration is stored in a firmware file.
+ * The Device Tree 'compatible' property value with a '.bin' suffix is passed
+ * to request_firmware() to fetch this file.
+ */
+struct panel_mipi_dbi_config {
+	/* Magic string: panel_mipi_dbi_magic */
+	u8 magic[15];
+
+	/* Config file format version */
+	u8 file_format_version;
+
+	/*
+	 * MIPI commands to execute when the display pipeline is enabled.
+	 * This is used to configure the display controller.
+	 *
+	 * The commands are stored in a byte array with the format:
+	 *     command, num_parameters, [ parameter, ...], command, ...
+	 *
+	 * Some commands require a pause before the next command can be received.
+	 * Inserting a delay in the command sequence is done by using the NOP command with one
+	 * parameter: delay in miliseconds (the No Operation command is part of the MIPI Display
+	 * Command Set where it has no parameters).
+	 *
+	 * Example:
+	 *     command 0x11
+	 *     sleep 120ms
+	 *     command 0xb1 parameters 0x01, 0x2c, 0x2d
+	 *     command 0x29
+	 *
+	 * Byte sequence:
+	 *     0x11 0x00
+	 *     0x00 0x01 0x78
+	 *     0xb1 0x03 0x01 0x2c 0x2d
+	 *     0x29 0x00
+	 */
+	u8 commands[];
+};
+
+struct panel_mipi_dbi_commands {
+	const u8 *buf;
+	size_t len;
+};
+
+static struct panel_mipi_dbi_commands *
+panel_mipi_dbi_check_commands(struct device *dev, const struct firmware *fw)
+{
+	const struct panel_mipi_dbi_config *config = (struct panel_mipi_dbi_config *)fw->data;
+	struct panel_mipi_dbi_commands *commands;
+	size_t size = fw->size, commands_len;
+	unsigned int i = 0;
+
+	if (size < sizeof(*config) + 2) { /* At least 1 command */
+		dev_err(dev, "config: file size=%zu is too small\n", size);
+		return ERR_PTR(-EINVAL);
+	}
+
+	if (memcmp(config->magic, panel_mipi_dbi_magic, sizeof(config->magic))) {
+		dev_err(dev, "config: Bad magic: %15ph\n", config->magic);
+		return ERR_PTR(-EINVAL);
+	}
+
+	if (config->file_format_version != 1) {
+		dev_err(dev, "config: version=%u is not supported\n", config->file_format_version);
+		return ERR_PTR(-EINVAL);
+	}
+
+	dev_dbg(dev, "size=%zu version=%u\n", size, config->file_format_version);
+
+	commands_len = size - sizeof(*config);
+
+	while ((i + 1) < commands_len) {
+		u8 command = config->commands[i++];
+		u8 num_parameters = config->commands[i++];
+		const u8 *parameters = &config->commands[i];
+
+		i += num_parameters;
+		if (i > commands_len) {
+			dev_err(dev, "config: command=0x%02x num_parameters=%u overflows\n",
+				command, num_parameters);
+			return ERR_PTR(-EINVAL);
+		}
+
+		if (command == 0x00 && num_parameters == 1)
+			dev_dbg(dev, "sleep %ums\n", parameters[0]);
+		else
+			dev_dbg(dev, "command %02x %*ph\n",
+				command, num_parameters, parameters);
+	}
+
+	if (i != commands_len) {
+		dev_err(dev, "config: malformed command array\n");
+		return ERR_PTR(-EINVAL);
+	}
+
+	commands = kzalloc(sizeof(*commands), GFP_KERNEL);
+	if (!commands)
+		return ERR_PTR(-ENOMEM);
+
+	commands->len = commands_len;
+	commands->buf = kmemdup(config->commands, commands->len, GFP_KERNEL);
+	if (!commands->buf)
+		return ERR_PTR(-ENOMEM);
+
+	return commands;
+}
+
+static struct panel_mipi_dbi_commands *panel_mipi_dbi_commands_from_fw(struct device *dev)
+{
+	struct panel_mipi_dbi_commands *commands;
+	const struct firmware *fw;
+	const char *compatible;
+	char fw_name[40];
+	int ret;
+
+	ret = of_property_read_string_index(dev->of_node, "compatible", 0, &compatible);
+	if (ret)
+		return ERR_PTR(ret);
+
+	snprintf(fw_name, sizeof(fw_name), "%s.bin", compatible);
+	ret = request_firmware(&fw, fw_name, dev);
+	if (ret) {
+		dev_err(dev, "No config file found for compatible '%s' (error=%d)\n",
+			compatible, ret);
+
+		return ERR_PTR(ret);
+	}
+
+	commands = panel_mipi_dbi_check_commands(dev, fw);
+	release_firmware(fw);
+
+	return commands;
+}
+
+static void panel_mipi_dbi_commands_execute(struct mipi_dbi *dbi,
+					    struct panel_mipi_dbi_commands *commands)
+{
+	unsigned int i = 0;
+
+	if (!commands)
+		return;
+
+	while (i < commands->len) {
+		u8 command = commands->buf[i++];
+		u8 num_parameters = commands->buf[i++];
+		const u8 *parameters = &commands->buf[i];
+
+		if (command == 0x00 && num_parameters == 1)
+			mdelay(parameters[0]);
+		else if (num_parameters)
+			mipi_dbi_command_stackbuf(dbi, command, parameters, num_parameters);
+		else
+			mipi_dbi_command(dbi, command);
+
+		i += num_parameters;
+	}
+}
+
+static void panel_mipi_dbi_enable(struct fb_info *info)
+{
+	struct mipi_dbi_dev *dbidev = container_of(info, struct mipi_dbi_dev, info);
+	struct mipi_dbi *dbi = &dbidev->dbi;
+	int ret;
+
+	if (!info->mode) {
+		dev_err(dbidev->dev, "No valid mode found\n");
+		return;
+	}
+
+	if (dbidev->backlight_node && !dbidev->backlight) {
+		dbidev->backlight = of_backlight_find(dbidev->backlight_node);
+		if (!dbidev->backlight)
+			dev_err(dbidev->dev, "No backlight found\n");
+	}
+
+	if (!dbidev->driver_private) {
+		dbidev->driver_private = panel_mipi_dbi_commands_from_fw(dbidev->dev);
+		if (IS_ERR(dbidev->driver_private)) {
+			dbidev->driver_private = NULL;
+			return;
+		}
+	}
+
+	ret = mipi_dbi_poweron_conditional_reset(dbidev);
+	if (ret < 0)
+		return;
+	if (!ret)
+		panel_mipi_dbi_commands_execute(dbi, dbidev->driver_private);
+
+	mipi_dbi_enable_flush(dbidev, info);
+}
+
+
+static struct fb_ops panel_mipi_dbi_ops = {
+	.fb_enable = panel_mipi_dbi_enable,
+	.fb_disable = mipi_dbi_fb_disable,
+	.fb_flush = mipi_dbi_fb_flush,
+};
+
+
+static int panel_mipi_dbi_get_mode(struct mipi_dbi_dev *dbidev, struct fb_videomode *mode)
+{
+	struct device *dev = dbidev->dev;
+	int ret;
+
+	ret = of_get_display_timing(dev->of_node, "panel-timing", mode);
+	if (ret) {
+		dev_err(dev, "%pOF: failed to get panel-timing (error=%d)\n", dev->of_node, ret);
+		return ret;
+	}
+
+	/*
+	 * Make sure width and height are set and that only back porch and
+	 * pixelclock are set in the other timing values. Also check that
+	 * width and height don't exceed the 16-bit value specified by MIPI DCS.
+	 */
+	if (!mode->xres || !mode->yres || mode->display_flags ||
+	    mode->right_margin || mode->hsync_len || (mode->left_margin + mode->xres) > 0xffff ||
+	    mode->lower_margin || mode->vsync_len || (mode->upper_margin + mode->yres) > 0xffff) {
+		dev_err(dev, "%pOF: panel-timing out of bounds\n", dev->of_node);
+		return -EINVAL;
+	}
+
+	/* The driver doesn't use the pixel clock but it is mandatory so fake one if not set */
+	if (!mode->pixclock) {
+		mode->pixclock =
+			(mode->left_margin + mode->xres + mode->right_margin + mode->hsync_len) *
+			(mode->upper_margin + mode->yres + mode->lower_margin + mode->vsync_len) *
+			60 / 1000;
+	}
+
+	return 0;
+}
+
+static int panel_mipi_dbi_spi_probe(struct device *dev)
+{
+	struct mipi_dbi_dev *dbidev;
+	struct spi_device *spi = to_spi_device(dev);
+	struct mipi_dbi *dbi;
+	struct fb_info *info;
+	int dc;
+	int ret;
+
+	dbidev = kzalloc(sizeof(*dbidev), GFP_KERNEL);
+	if (!dbidev)
+		return -ENOMEM;
+
+	dbidev->dev = dev;
+	dbi = &dbidev->dbi;
+	info = &dbidev->info;
+
+	ret = panel_mipi_dbi_get_mode(dbidev, &dbidev->mode);
+	if (ret)
+		return ret;
+
+	dbidev->regulator = regulator_get(dev, "power");
+	if (IS_ERR(dbidev->regulator))
+		return dev_err_probe(dev, PTR_ERR(dbidev->regulator),
+				     "Failed to get regulator 'power'\n");
+
+	dbidev->io_regulator = regulator_get(dev, "io");
+	if (IS_ERR(dbidev->io_regulator))
+		return dev_err_probe(dev, PTR_ERR(dbidev->io_regulator),
+				     "Failed to get regulator 'io'\n");
+
+	dbidev->backlight_node = of_parse_phandle(dev->of_node, "backlight", 0);
+
+	dbi->reset = gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
+	if (dbi->reset < 0 && dbi->reset != -ENOENT)
+		return dev_err_probe(dev, dbi->reset, "Failed to get GPIO 'reset'\n");
+
+	dc = gpiod_get(dev, "dc", GPIOD_OUT_LOW);
+	if (dc < 0 && dc != -ENOENT)
+		return dev_err_probe(dev, dc, "Failed to get GPIO 'dc'\n");
+
+	ret = mipi_dbi_spi_init(spi, dbi, dc);
+	if (ret)
+		return ret;
+
+	ret = mipi_dbi_dev_init(dbidev, &panel_mipi_dbi_ops, &dbidev->mode);
+	if (ret)
+		return ret;
+
+	ret = register_framebuffer(info);
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "Failed to register framebuffer\n");
+
+	return 0;
+}
+
+static const struct of_device_id panel_mipi_dbi_spi_of_match[] = {
+	{ .compatible = "panel-mipi-dbi-spi" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, panel_mipi_dbi_spi_of_match);
+
+static struct driver panel_mipi_dbi_spi_driver = {
+	.name = "panel-mipi-dbi-spi",
+	.probe = panel_mipi_dbi_spi_probe,
+	.of_compatible = DRV_OF_COMPAT(panel_mipi_dbi_spi_of_match),
+};
+device_spi_driver(panel_mipi_dbi_spi_driver);
+
+MODULE_DESCRIPTION("MIPI DBI compatible display panel driver");
+MODULE_AUTHOR("Noralf Trønnes");
+MODULE_LICENSE("GPL");
-- 
2.39.2

Re: [PATCH 1/4] firmware: Add request/release_firmware() calls

[Ahmad Fatoum]

On 29.03.23 12:56, Philipp Zabel wrote:
> Add request_firmware() and release_firmware() calls that allow drivers
> to load a firmware file. Also move the struct firmware definition from
> remoteproc.h into firmware.h.
> 
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
>  common/firmware.c          | 33 +++++++++++++++++++++++++++++++++
>  include/firmware.h         | 16 ++++++++++++++++
>  include/linux/remoteproc.h |  5 -----
>  3 files changed, 49 insertions(+), 5 deletions(-)
> 
> diff --git a/common/firmware.c b/common/firmware.c
> index e4ad6ac867b0..c6668fd528b6 100644
> --- a/common/firmware.c
> +++ b/common/firmware.c
> @@ -17,6 +17,7 @@
>  #include <linux/stat.h>
>  #include <linux/err.h>
>  #include <uncompress.h>
> +#include <unistd.h>
>  #include <filetype.h>
>  
>  #define BUFSIZ 4096
> @@ -304,6 +305,38 @@ out:
>  	return ret;
>  }
>  
> +/*
> + * request_firmware - load a firmware to a device
> + */
> +int request_firmware(const struct firmware **out, const char *fw_name, struct device *dev)
> +{
> +	char fw_path[PATH_MAX + 1];
> +	struct firmware *fw;
> +	int ret;
> +
> +	fw = kzalloc(sizeof(struct firmware), GFP_KERNEL);
> +	if (!fw)
> +		return -ENOMEM;
> +
> +	snprintf(fw_path, sizeof(fw_path), "%s/%s", firmware_path, fw_name);
> +
> +	ret = read_file_2(fw_path, &fw->size, (void *)&fw->data, FILESIZE_MAX);
> +	if (ret) {
> +		kfree(fw);
> +		return ret;
> +	}
> +
> +	*out = fw;
> +
> +	return 0;
> +}
> +
> +void release_firmware(const struct firmware *fw)
> +{
> +	kfree_const(fw->data);
> +	kfree_const(fw);
> +}
> +
>  static int firmware_init(void)
>  {
>  	firmware_path = strdup("/env/firmware");
> diff --git a/include/firmware.h b/include/firmware.h
> index 05433f2f7858..b4fa40a3ab44 100644
> --- a/include/firmware.h
> +++ b/include/firmware.h
> @@ -13,6 +13,11 @@
>  #include <debug_ll.h>
>  #include <linux/kernel.h>
>  
> +struct firmware {
> +	size_t size;
> +	const u8 *data;
> +};
> +
>  struct firmware_handler {
>  	char *id; /* unique identifier for this firmware device */
>  	char *model; /* description for this device */
> @@ -37,6 +42,8 @@ struct firmware_mgr *firmwaremgr_find_by_node(struct device_node *np);
>  int firmwaremgr_load_file(struct firmware_mgr *, const char *path);
>  char *firmware_get_searchpath(void);
>  void firmware_set_searchpath(const char *path);
> +int request_firmware(const struct firmware **fw, const char *fw_name, struct device *dev);
> +void release_firmware(const struct firmware *fw);
>  #else
>  static inline struct firmware_mgr *firmwaremgr_find_by_node(struct device_node *np)
>  {
> @@ -57,6 +64,15 @@ static inline void firmware_set_searchpath(const char *path)
>  {
>  }
>  
> +static inline int request_firmware(const struct firmware **fw, const char *fw_name,
> +				   struct device *dev)
> +{
> +	return -EOPNOTSUPP;

Kernel returns -EINVAL here. We should do the same.

With this addressed,

Reviewed-by: Ahmad Fatoum <a.fatoum@pengutronix.de>

> +}
> +
> +static inline void release_firmware(const struct firmware *fw)
> +{
> +}
>  #endif
>  
>  void firmwaremgr_list_handlers(void);
> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> index 170fff7987fb..33fe2f81b748 100644
> --- a/include/linux/remoteproc.h
> +++ b/include/linux/remoteproc.h
> @@ -18,11 +18,6 @@ struct resource_table {
>  	u32 offset[0];
>  } __packed;
>  
> -struct firmware {
> -	size_t size;
> -	const u8 *data;
> -};
> -
>  struct rproc;
>  
>  struct rproc_ops {

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 2/4] video: Add of_get_display_timing

[Ahmad Fatoum]

On 29.03.23 12:56, Philipp Zabel wrote:
> Add a port of the kernel of_get_display_timing() that writes a
> struct fb_videomode instead of struct display_timing, which we
> don't have.
> 
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>

Reviewed-by: Ahmad Fatoum <a.fatoum@pengutronix.de>

> ---
>  drivers/video/of_display_timing.c | 22 ++++++++++++++++++++++
>  include/fb.h                      |  2 ++
>  2 files changed, 24 insertions(+)
> 
> diff --git a/drivers/video/of_display_timing.c b/drivers/video/of_display_timing.c
> index 6fe1e1b08b6a..6082d454932d 100644
> --- a/drivers/video/of_display_timing.c
> +++ b/drivers/video/of_display_timing.c
> @@ -98,6 +98,28 @@ static int of_parse_display_timing(const struct device_node *np,
>  	return 0;
>  }
>  
> +/**
> + * of_get_display_timing - parse a display_timing entry
> + * @np: device_node with the timing subnode
> + * @name: name of the timing node
> + * @mode: fb_videomode struct to fill
> + **/
> +int of_get_display_timing(const struct device_node *np, const char *name,
> +			  struct fb_videomode *mode)
> +{
> +	struct device_node *timing_np;
> +
> +	if (!np)
> +		return -EINVAL;
> +
> +	timing_np = of_get_child_by_name(np, name);
> +	if (!timing_np)
> +		return -ENOENT;
> +
> +	return of_parse_display_timing(timing_np, mode);
> +}
> +EXPORT_SYMBOL_GPL(of_get_display_timing);
> +
>  /**
>   * of_get_display_timings - parse all display_timing entries from a device_node
>   * @np: device_node with the subnodes
> diff --git a/include/fb.h b/include/fb.h
> index bf5f688342fd..15bb74b99576 100644
> --- a/include/fb.h
> +++ b/include/fb.h
> @@ -147,6 +147,8 @@ struct fb_info {
>  	int shadowfb;
>  };
>  
> +int of_get_display_timing(const struct device_node *np, const char *name,
> +			  struct fb_videomode *mode);
>  struct display_timings *of_get_display_timings(struct device_node *np);
>  void display_timings_release(struct display_timings *);
>  

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 3/4] video: add MIPI DBI framebuffer helpers

[Ahmad Fatoum]

On 29.03.23 12:56, Philipp Zabel wrote:
> Port helper functions for the panel-mipi-dbi driver from the Linux
> kernel.
> 
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
>  drivers/video/mipi_dbi.c | 242 +++++++++++++++++++++++++++++++++++++++
>  include/video/mipi_dbi.h |  63 ++++++++++
>  2 files changed, 305 insertions(+)
> 
> diff --git a/drivers/video/mipi_dbi.c b/drivers/video/mipi_dbi.c
> index 50d2fc4b29b9..9aa16abb1c9b 100644
> --- a/drivers/video/mipi_dbi.c
> +++ b/drivers/video/mipi_dbi.c
> @@ -13,6 +13,7 @@
>  #include <gpiod.h>
>  #include <regulator.h>
>  #include <spi/spi.h>
> +#include <video/backlight.h>
>  #include <video/mipi_dbi.h>
>  
>  #include <video/vpl.h>
> @@ -196,6 +197,178 @@ int mipi_dbi_command_stackbuf(struct mipi_dbi *dbi, u8 cmd, const u8 *data,
>  }
>  EXPORT_SYMBOL(mipi_dbi_command_stackbuf);
>  
> +/**
> + * mipi_dbi_buf_copy - Copy a framebuffer, transforming it if necessary
> + * @dst: The destination buffer
> + * @info: The source framebuffer info
> + * @swap: When true, swap MSB/LSB of 16-bit values
> + *
> + * Returns:
> + * Zero on success, negative error code on failure.
> + */
> +static void mipi_dbi_buf_copy(void *dst, struct fb_info *info, bool swap)
> +{
> +	u16 *src = (u16 *)info->screen_base;
> +	u16 *dst16 = dst;
> +	size_t len = info->xres * info->yres;
> +	int i;
> +
> +	if (swap) {
> +		for (i = 0; i < len; i++) {
> +			*dst16++ = *src << 8 | *src >> 8;
> +			src++;
> +		}
> +	} else {
> +		memcpy(dst, src, len * 2);

Do we need this? Why can't we send out framebuffer directly if there is no
swapping to be done?

> +	}
> +}
> +
> +static void mipi_dbi_set_window_address(struct mipi_dbi_dev *dbidev,
> +					unsigned int xs, unsigned int xe,
> +					unsigned int ys, unsigned int ye)
> +{
> +	struct mipi_dbi *dbi = &dbidev->dbi;
> +
> +	xs += dbidev->mode.left_margin;
> +	xe += dbidev->mode.left_margin;
> +	ys += dbidev->mode.upper_margin;
> +	ye += dbidev->mode.upper_margin;
> +
> +	mipi_dbi_command(dbi, MIPI_DCS_SET_COLUMN_ADDRESS, (xs >> 8) & 0xff,
> +			 xs & 0xff, (xe >> 8) & 0xff, xe & 0xff);
> +	mipi_dbi_command(dbi, MIPI_DCS_SET_PAGE_ADDRESS, (ys >> 8) & 0xff,
> +			 ys & 0xff, (ye >> 8) & 0xff, ye & 0xff);
> +}
> +
> +static void mipi_dbi_fb_dirty(struct mipi_dbi_dev *dbidev, struct fb_info *info)
> +{
> +	struct mipi_dbi *dbi = &dbidev->dbi;
> +	u16 width = info->xres;
> +	u16 height = info->yres;
> +	size_t len = width * height * 2;
> +	int ret;
> +
> +	mipi_dbi_buf_copy(dbidev->tx_buf, info, dbi->swap_bytes);
> +
> +	mipi_dbi_set_window_address(dbidev, 0, width - 1, 0, height - 1);
> +
> +	ret = mipi_dbi_command_buf(dbi, MIPI_DCS_WRITE_MEMORY_START, dbidev->tx_buf, len);
> +	if (ret)
> +		pr_err_once("Failed to update display %d\n", ret);

FYI, there's %pe support for printing error codes as string if they
were compiled in.

> +}
> +
> +/**
> + * mipi_dbi_enable_flush - MIPI DBI enable helper
> + * @dbidev: MIPI DBI device structure
> + * @crtc_state: CRTC state
> + * @plane_state: Plane state
> + *
> + * Flushes the whole framebuffer and enables the backlight. Drivers can use this
> + * in their &drm_simple_display_pipe_funcs->enable callback.

fb_ops->fb_enable ?

> + *
> + * Note: Drivers which don't use mipi_dbi_pipe_update() because they have custom
> + * framebuffer flushing, can't use this function since they both use the same
> + * flushing code.
> + */
> +void mipi_dbi_enable_flush(struct mipi_dbi_dev *dbidev,
> +			   struct fb_info *info)
> +{
> +	mipi_dbi_fb_dirty(dbidev, info);
> +
> +	if (dbidev->backlight)
> +		backlight_set_brightness_default(dbidev->backlight);
> +}
> +EXPORT_SYMBOL(mipi_dbi_enable_flush);
> +
> +
> +static void mipi_dbi_blank(struct mipi_dbi_dev *dbidev)
> +{
> +	u16 height = dbidev->mode.xres;
> +	u16 width = dbidev->mode.yres;
> +	struct mipi_dbi *dbi = &dbidev->dbi;
> +	size_t len = width * height * 2;
> +
> +	memset(dbidev->tx_buf, 0, len);
> +
> +	mipi_dbi_set_window_address(dbidev, 0, width - 1, 0, height - 1);
> +	mipi_dbi_command_buf(dbi, MIPI_DCS_WRITE_MEMORY_START, dbidev->tx_buf, len);
> +}
> +
> +/**
> + * mipi_dbi_fb_disable - MIPI DBI framebuffer disable helper
> + * @info: Framebuffer info
> + *
> + * This function disables backlight if present, if not the display memory is
> + * blanked. The regulator is disabled if in use. Drivers can use this as their
> + * &fb_ops->fb_disable callback.
> + */
> +void mipi_dbi_fb_disable(struct fb_info *info)
> +{
> +	struct mipi_dbi_dev *dbidev = container_of(info, struct mipi_dbi_dev, info);
> +
> +	if (dbidev->backlight)
> +		backlight_set_brightness(dbidev->backlight, 0);
> +	else
> +		mipi_dbi_blank(dbidev);
> +
> +	if (dbidev->regulator)
> +		regulator_disable(dbidev->regulator);
> +	if (dbidev->io_regulator)
> +		regulator_disable(dbidev->io_regulator);

Calling regulator_disable on NULL pointer is a no-op.

> +}
> +EXPORT_SYMBOL(mipi_dbi_fb_disable);
> +
> +void mipi_dbi_fb_flush(struct fb_info *info)
> +{
> +	struct mipi_dbi_dev *dbidev = container_of(info, struct mipi_dbi_dev, info);
> +
> +	mipi_dbi_fb_dirty(dbidev, info);
> +}
> +EXPORT_SYMBOL(mipi_dbi_fb_flush);
> +
> +/**
> + * mipi_dbi_dev_init - MIPI DBI device initialization
> + * @dbidev: MIPI DBI device structure to initialize
> + * @ops: Framebuffer operations
> + * @mode: Display mode
> + *
> + * This function sets up a &fb_info with one fixed &fb_videomode.
> + * Additionally &mipi_dbi.tx_buf is allocated.
> + *
> + * Supported format: RGB565.
> + *
> + * Returns:
> + * Zero on success, negative error code on failure.
> + */
> +int mipi_dbi_dev_init(struct mipi_dbi_dev *dbidev, struct fb_ops *ops,
> +		      struct fb_videomode *mode)
> +{
> +	struct fb_info *info = &dbidev->info;
> +
> +	info->mode = mode;
> +	info->fbops = ops;
> +	info->dev.parent = dbidev->dev;
> +
> +	info->xres = mode->xres;
> +	info->yres = mode->yres;
> +	info->bits_per_pixel = 16;
> +	info->line_length = info->xres * (info->bits_per_pixel >> 3);
> +
> +	info->screen_size = info->line_length * info->yres;
> +	info->screen_base = kzalloc(info->screen_size, GFP_KERNEL);

dma_alloc? In case some SPI driver gets DMA support.

> +
> +	info->red.length = 5;
> +	info->red.offset = 11;
> +	info->green.length = 6;
> +	info->green.offset = 5;
> +	info->blue.length = 5;
> +	info->blue.offset = 0;
> +
> +	dbidev->tx_buf = kzalloc(mode->xres * mode->yres * 2, GFP_KERNEL);

Use info->screen_size here as well?

> +
> +	return 0;
> +}
> +
>  /**
>   * mipi_dbi_hw_reset - Hardware reset of controller
>   * @dbi: MIPI DBI structure
> @@ -246,6 +419,75 @@ bool mipi_dbi_display_is_on(struct mipi_dbi *dbi)
>  }
>  EXPORT_SYMBOL(mipi_dbi_display_is_on);
>  
> +static int mipi_dbi_poweron_reset_conditional(struct mipi_dbi_dev *dbidev, bool cond)
> +{
> +	struct device *dev = dbidev->dev;
> +	struct mipi_dbi *dbi = &dbidev->dbi;
> +	int ret;
> +
> +	if (dbidev->regulator) {
> +		ret = regulator_enable(dbidev->regulator);

returns 0 when called on NULL pointer, so no need for if above.

> +		if (ret) {
> +			dev_err(dev, "Failed to enable regulator (%d)\n", ret);
> +			return ret;
> +		}
> +	}
> +
> +	if (dbidev->io_regulator) {
> +		ret = regulator_enable(dbidev->io_regulator);

Ditto

> +		if (ret) {
> +			dev_err(dev, "Failed to enable I/O regulator (%d)\n", ret);
> +			if (dbidev->regulator)
> +				regulator_disable(dbidev->regulator);
> +			return ret;
> +		}
> +	}
> +
> +	if (cond && mipi_dbi_display_is_on(dbi))
> +		return 1;
> +
> +	mipi_dbi_hw_reset(dbi);
> +	ret = mipi_dbi_command(dbi, MIPI_DCS_SOFT_RESET);
> +	if (ret) {
> +		dev_err(dev, "Failed to send reset command (%d)\n", ret);
> +		if (dbidev->regulator)
> +			regulator_disable(dbidev->regulator);
> +		if (dbidev->io_regulator)
> +			regulator_disable(dbidev->io_regulator);

As above.

> +		return ret;
> +	}
> +
> +	/*
> +	 * If we did a hw reset, we know the controller is in Sleep mode and
> +	 * per MIPI DSC spec should wait 5ms after soft reset. If we didn't,
> +	 * we assume worst case and wait 120ms.
> +	 */
> +	if (dbi->reset)
> +		mdelay(5);
> +	else
> +		mdelay(120);
> +
> +	return 0;
> +}
> +
> +/**
> + * mipi_dbi_poweron_conditional_reset - MIPI DBI poweron and conditional reset
> + * @dbidev: MIPI DBI device structure
> + *
> + * This function enables the regulator if used and if the display is off, it
> + * does a hardware and software reset. If mipi_dbi_display_is_on() determines
> + * that the display is on, no reset is performed.
> + *
> + * Returns:
> + * Zero if the controller was reset, 1 if the display was already on, or a
> + * negative error code.
> + */
> +int mipi_dbi_poweron_conditional_reset(struct mipi_dbi_dev *dbidev)
> +{
> +	return mipi_dbi_poweron_reset_conditional(dbidev, true);
> +}
> +EXPORT_SYMBOL(mipi_dbi_poweron_conditional_reset);
> +
>  #if IS_ENABLED(CONFIG_SPI)
>  
>  /**
> diff --git a/include/video/mipi_dbi.h b/include/video/mipi_dbi.h
> index 54526006935f..917f7ddd597f 100644
> --- a/include/video/mipi_dbi.h
> +++ b/include/video/mipi_dbi.h
> @@ -11,6 +11,7 @@
>  #include <linux/types.h>
>  #include <spi/spi.h>
>  #include <driver.h>
> +#include <fb.h>
>  
>  struct regulator;
>  struct fb_videomode;
> @@ -55,6 +56,61 @@ struct mipi_dbi {
>  	struct list_head list;
>  };
>  
> +/**
> + * struct mipi_dbi_dev - MIPI DBI device
> + */
> +struct mipi_dbi_dev {
> +	/**
> +	 * @dev: Device
> +	 */
> +	struct device *dev;
> +
> +	/**
> +	 * @info: Framebuffer info
> +	 */
> +	struct fb_info info;
> +
> +	/**
> +	 * @mode: Fixed display mode
> +	 */
> +	struct fb_videomode mode;
> +
> +	/**
> +	 * @tx_buf: Buffer used for transfer (copy clip rect area)
> +	 */
> +	u8 *tx_buf;
> +
> +	/**
> +	 * @backlight_node: backlight device node (optional)
> +	 */
> +	struct device_node *backlight_node;
> +
> +	/**
> +	 * @backlight: backlight device (optional)
> +	 */
> +	struct backlight_device *backlight;
> +
> +	/**
> +	 * @regulator: power regulator (Vdd) (optional)
> +	 */
> +	struct regulator *regulator;
> +
> +	/**
> +	 * @io_regulator: I/O power regulator (Vddi) (optional)
> +	 */
> +	struct regulator *io_regulator;
> +
> +	/**
> +	 * @dbi: MIPI DBI interface
> +	 */
> +	struct mipi_dbi dbi;
> +
> +	/**
> +	 * @driver_private: Driver private data.
> +	 */
> +	void *driver_private;
> +};
> +
>  static inline const char *mipi_dbi_name(struct mipi_dbi *dbi)
>  {
>  	return dev_name(&dbi->spi->dev);
> @@ -62,8 +118,15 @@ static inline const char *mipi_dbi_name(struct mipi_dbi *dbi)
>  
>  int mipi_dbi_spi_init(struct spi_device *spi, struct mipi_dbi *dbi,
>  		      int dc);
> +int mipi_dbi_dev_init(struct mipi_dbi_dev *dbidev,
> +		      struct fb_ops *ops, struct fb_videomode *mode);
> +void mipi_dbi_fb_flush(struct fb_info *info);
> +void mipi_dbi_enable_flush(struct mipi_dbi_dev *dbidev,
> +			   struct fb_info *info);
> +void mipi_dbi_fb_disable(struct fb_info *info);
>  void mipi_dbi_hw_reset(struct mipi_dbi *dbi);
>  bool mipi_dbi_display_is_on(struct mipi_dbi *dbi);
> +int mipi_dbi_poweron_conditional_reset(struct mipi_dbi_dev *dbidev);
>  
>  u32 mipi_dbi_spi_cmd_max_speed(struct spi_device *spi, size_t len);
>  int mipi_dbi_spi_transfer(struct spi_device *spi, u32 speed_hz,

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 4/4] video: Add MIPI DBI compatible SPI driver

[Ahmad Fatoum]

On 29.03.23 12:56, Philipp Zabel wrote:
> Port the panel-mipi-dbi driver from the Linux kernel. It works with most
> MIPI DBI compatible SPI panels.
> This avoids adding a driver for every new MIPI DBI compatible controller
> that is to be used by Barebox. The 'compatible' Device Tree property
> with a '.bin' suffix will be used to load a firmware file that contains
> the controller configuration.
> 
> Example (driver will load sainsmart18.bin):
> 
> display@0 {
>         compatible = "sainsmart18", "panel-mipi-dbi-spi";
> ...
> };

This is a strange binding, but if Linux does it this way, I guess
we'll want to stay compatible...

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

Reviewed-by: Ahmad Fatoum <a.fatoum@pengutronix.de>

> ---
>  drivers/video/Kconfig          |  12 ++
>  drivers/video/Makefile         |   1 +
>  drivers/video/panel-mipi-dbi.c | 330 +++++++++++++++++++++++++++++++++
>  3 files changed, 343 insertions(+)
>  create mode 100644 drivers/video/panel-mipi-dbi.c
> 
> diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
> index 01bdaf47bfca..0e710bb25f79 100644
> --- a/drivers/video/Kconfig
> +++ b/drivers/video/Kconfig
> @@ -198,4 +198,16 @@ config DRIVER_VIDEO_PANEL_ILITEK_ILI9341
>  	  QVGA (240x320) RGB panels. support serial & parallel rgb
>  	  interface.
>  
> +config DRIVER_VIDEO_PANEL_MIPI_DBI
> +	tristate "DRM support for MIPI DBI compatible panels"
> +	depends on OFTREE && SPI
> +	select DRIVER_VIDEO_MIPI_DBI
> +	select FIRMWARE
> +	select VIDEO_VPL
> +	help
> +          Say Y here if you want to enable support for MIPI DBI compatible
> +          panels. The controller command setup can be provided using a
> +          firmware file. For more information see
> +          https://github.com/notro/panel-mipi-dbi/wiki.
> +
>  endif
> diff --git a/drivers/video/Makefile b/drivers/video/Makefile
> index d50d2d3ba562..7718b63f448a 100644
> --- a/drivers/video/Makefile
> +++ b/drivers/video/Makefile
> @@ -11,6 +11,7 @@ obj-$(CONFIG_DRIVER_VIDEO_TC358767) += tc358767.o
>  obj-$(CONFIG_DRIVER_VIDEO_SIMPLE_PANEL) += simple-panel.o
>  obj-$(CONFIG_DRIVER_VIDEO_MIPI_DBI) += mipi_dbi.o
>  obj-$(CONFIG_DRIVER_VIDEO_PANEL_ILITEK_ILI9341) += panel-ilitek-ili9341.o
> +obj-$(CONFIG_DRIVER_VIDEO_PANEL_MIPI_DBI) += panel-mipi-dbi.o
>  
>  obj-$(CONFIG_DRIVER_VIDEO_ATMEL) += atmel_lcdfb.o atmel_lcdfb_core.o
>  obj-$(CONFIG_DRIVER_VIDEO_ATMEL_HLCD) += atmel_hlcdfb.o atmel_lcdfb_core.o
> diff --git a/drivers/video/panel-mipi-dbi.c b/drivers/video/panel-mipi-dbi.c
> new file mode 100644
> index 000000000000..ac6f585be32c
> --- /dev/null
> +++ b/drivers/video/panel-mipi-dbi.c
> @@ -0,0 +1,330 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * DRM driver for MIPI DBI compatible display panels
> + *
> + * Copyright 2022 Noralf Trønnes
> + */
> +
> +#include <clock.h>
> +#include <common.h>
> +#include <fb.h>
> +#include <firmware.h>
> +#include <gpiod.h>
> +#include <linux/printk.h>
> +#include <of.h>
> +#include <regulator.h>
> +#include <spi/spi.h>
> +
> +#include <video/backlight.h>
> +#include <video/mipi_dbi.h>
> +#include <video/mipi_display.h>
> +
> +static const u8 panel_mipi_dbi_magic[15] = { 'M', 'I', 'P', 'I', ' ', 'D', 'B', 'I',
> +					     0, 0, 0, 0, 0, 0, 0 };
> +
> +/*
> + * The display controller configuration is stored in a firmware file.
> + * The Device Tree 'compatible' property value with a '.bin' suffix is passed
> + * to request_firmware() to fetch this file.
> + */
> +struct panel_mipi_dbi_config {
> +	/* Magic string: panel_mipi_dbi_magic */
> +	u8 magic[15];
> +
> +	/* Config file format version */
> +	u8 file_format_version;
> +
> +	/*
> +	 * MIPI commands to execute when the display pipeline is enabled.
> +	 * This is used to configure the display controller.
> +	 *
> +	 * The commands are stored in a byte array with the format:
> +	 *     command, num_parameters, [ parameter, ...], command, ...
> +	 *
> +	 * Some commands require a pause before the next command can be received.
> +	 * Inserting a delay in the command sequence is done by using the NOP command with one
> +	 * parameter: delay in miliseconds (the No Operation command is part of the MIPI Display
> +	 * Command Set where it has no parameters).
> +	 *
> +	 * Example:
> +	 *     command 0x11
> +	 *     sleep 120ms
> +	 *     command 0xb1 parameters 0x01, 0x2c, 0x2d
> +	 *     command 0x29
> +	 *
> +	 * Byte sequence:
> +	 *     0x11 0x00
> +	 *     0x00 0x01 0x78
> +	 *     0xb1 0x03 0x01 0x2c 0x2d
> +	 *     0x29 0x00
> +	 */
> +	u8 commands[];
> +};
> +
> +struct panel_mipi_dbi_commands {
> +	const u8 *buf;
> +	size_t len;
> +};
> +
> +static struct panel_mipi_dbi_commands *
> +panel_mipi_dbi_check_commands(struct device *dev, const struct firmware *fw)
> +{
> +	const struct panel_mipi_dbi_config *config = (struct panel_mipi_dbi_config *)fw->data;
> +	struct panel_mipi_dbi_commands *commands;
> +	size_t size = fw->size, commands_len;
> +	unsigned int i = 0;
> +
> +	if (size < sizeof(*config) + 2) { /* At least 1 command */
> +		dev_err(dev, "config: file size=%zu is too small\n", size);
> +		return ERR_PTR(-EINVAL);
> +	}
> +
> +	if (memcmp(config->magic, panel_mipi_dbi_magic, sizeof(config->magic))) {
> +		dev_err(dev, "config: Bad magic: %15ph\n", config->magic);
> +		return ERR_PTR(-EINVAL);
> +	}
> +
> +	if (config->file_format_version != 1) {
> +		dev_err(dev, "config: version=%u is not supported\n", config->file_format_version);
> +		return ERR_PTR(-EINVAL);
> +	}
> +
> +	dev_dbg(dev, "size=%zu version=%u\n", size, config->file_format_version);
> +
> +	commands_len = size - sizeof(*config);
> +
> +	while ((i + 1) < commands_len) {
> +		u8 command = config->commands[i++];
> +		u8 num_parameters = config->commands[i++];
> +		const u8 *parameters = &config->commands[i];
> +
> +		i += num_parameters;
> +		if (i > commands_len) {
> +			dev_err(dev, "config: command=0x%02x num_parameters=%u overflows\n",
> +				command, num_parameters);
> +			return ERR_PTR(-EINVAL);
> +		}
> +
> +		if (command == 0x00 && num_parameters == 1)
> +			dev_dbg(dev, "sleep %ums\n", parameters[0]);
> +		else
> +			dev_dbg(dev, "command %02x %*ph\n",
> +				command, num_parameters, parameters);
> +	}
> +
> +	if (i != commands_len) {
> +		dev_err(dev, "config: malformed command array\n");
> +		return ERR_PTR(-EINVAL);
> +	}
> +
> +	commands = kzalloc(sizeof(*commands), GFP_KERNEL);
> +	if (!commands)
> +		return ERR_PTR(-ENOMEM);
> +
> +	commands->len = commands_len;
> +	commands->buf = kmemdup(config->commands, commands->len, GFP_KERNEL);
> +	if (!commands->buf)
> +		return ERR_PTR(-ENOMEM);
> +
> +	return commands;
> +}
> +
> +static struct panel_mipi_dbi_commands *panel_mipi_dbi_commands_from_fw(struct device *dev)
> +{
> +	struct panel_mipi_dbi_commands *commands;
> +	const struct firmware *fw;
> +	const char *compatible;
> +	char fw_name[40];
> +	int ret;
> +
> +	ret = of_property_read_string_index(dev->of_node, "compatible", 0, &compatible);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	snprintf(fw_name, sizeof(fw_name), "%s.bin", compatible);
> +	ret = request_firmware(&fw, fw_name, dev);
> +	if (ret) {
> +		dev_err(dev, "No config file found for compatible '%s' (error=%d)\n",
> +			compatible, ret);
> +
> +		return ERR_PTR(ret);
> +	}
> +
> +	commands = panel_mipi_dbi_check_commands(dev, fw);
> +	release_firmware(fw);
> +
> +	return commands;
> +}
> +
> +static void panel_mipi_dbi_commands_execute(struct mipi_dbi *dbi,
> +					    struct panel_mipi_dbi_commands *commands)
> +{
> +	unsigned int i = 0;
> +
> +	if (!commands)
> +		return;
> +
> +	while (i < commands->len) {
> +		u8 command = commands->buf[i++];
> +		u8 num_parameters = commands->buf[i++];
> +		const u8 *parameters = &commands->buf[i];
> +
> +		if (command == 0x00 && num_parameters == 1)
> +			mdelay(parameters[0]);
> +		else if (num_parameters)
> +			mipi_dbi_command_stackbuf(dbi, command, parameters, num_parameters);
> +		else
> +			mipi_dbi_command(dbi, command);
> +
> +		i += num_parameters;
> +	}
> +}
> +
> +static void panel_mipi_dbi_enable(struct fb_info *info)
> +{
> +	struct mipi_dbi_dev *dbidev = container_of(info, struct mipi_dbi_dev, info);
> +	struct mipi_dbi *dbi = &dbidev->dbi;
> +	int ret;
> +
> +	if (!info->mode) {
> +		dev_err(dbidev->dev, "No valid mode found\n");
> +		return;
> +	}
> +
> +	if (dbidev->backlight_node && !dbidev->backlight) {
> +		dbidev->backlight = of_backlight_find(dbidev->backlight_node);
> +		if (!dbidev->backlight)
> +			dev_err(dbidev->dev, "No backlight found\n");
> +	}
> +
> +	if (!dbidev->driver_private) {
> +		dbidev->driver_private = panel_mipi_dbi_commands_from_fw(dbidev->dev);
> +		if (IS_ERR(dbidev->driver_private)) {
> +			dbidev->driver_private = NULL;
> +			return;
> +		}
> +	}
> +
> +	ret = mipi_dbi_poweron_conditional_reset(dbidev);
> +	if (ret < 0)
> +		return;
> +	if (!ret)
> +		panel_mipi_dbi_commands_execute(dbi, dbidev->driver_private);
> +
> +	mipi_dbi_enable_flush(dbidev, info);
> +}
> +
> +
> +static struct fb_ops panel_mipi_dbi_ops = {
> +	.fb_enable = panel_mipi_dbi_enable,
> +	.fb_disable = mipi_dbi_fb_disable,
> +	.fb_flush = mipi_dbi_fb_flush,
> +};
> +
> +
> +static int panel_mipi_dbi_get_mode(struct mipi_dbi_dev *dbidev, struct fb_videomode *mode)
> +{
> +	struct device *dev = dbidev->dev;
> +	int ret;
> +
> +	ret = of_get_display_timing(dev->of_node, "panel-timing", mode);
> +	if (ret) {
> +		dev_err(dev, "%pOF: failed to get panel-timing (error=%d)\n", dev->of_node, ret);
> +		return ret;
> +	}
> +
> +	/*
> +	 * Make sure width and height are set and that only back porch and
> +	 * pixelclock are set in the other timing values. Also check that
> +	 * width and height don't exceed the 16-bit value specified by MIPI DCS.
> +	 */
> +	if (!mode->xres || !mode->yres || mode->display_flags ||
> +	    mode->right_margin || mode->hsync_len || (mode->left_margin + mode->xres) > 0xffff ||
> +	    mode->lower_margin || mode->vsync_len || (mode->upper_margin + mode->yres) > 0xffff) {
> +		dev_err(dev, "%pOF: panel-timing out of bounds\n", dev->of_node);
> +		return -EINVAL;
> +	}
> +
> +	/* The driver doesn't use the pixel clock but it is mandatory so fake one if not set */
> +	if (!mode->pixclock) {
> +		mode->pixclock =
> +			(mode->left_margin + mode->xres + mode->right_margin + mode->hsync_len) *
> +			(mode->upper_margin + mode->yres + mode->lower_margin + mode->vsync_len) *
> +			60 / 1000;
> +	}
> +
> +	return 0;
> +}
> +
> +static int panel_mipi_dbi_spi_probe(struct device *dev)
> +{
> +	struct mipi_dbi_dev *dbidev;
> +	struct spi_device *spi = to_spi_device(dev);
> +	struct mipi_dbi *dbi;
> +	struct fb_info *info;
> +	int dc;
> +	int ret;
> +
> +	dbidev = kzalloc(sizeof(*dbidev), GFP_KERNEL);
> +	if (!dbidev)
> +		return -ENOMEM;
> +
> +	dbidev->dev = dev;
> +	dbi = &dbidev->dbi;
> +	info = &dbidev->info;
> +
> +	ret = panel_mipi_dbi_get_mode(dbidev, &dbidev->mode);
> +	if (ret)
> +		return ret;
> +
> +	dbidev->regulator = regulator_get(dev, "power");
> +	if (IS_ERR(dbidev->regulator))
> +		return dev_err_probe(dev, PTR_ERR(dbidev->regulator),
> +				     "Failed to get regulator 'power'\n");
> +
> +	dbidev->io_regulator = regulator_get(dev, "io");
> +	if (IS_ERR(dbidev->io_regulator))
> +		return dev_err_probe(dev, PTR_ERR(dbidev->io_regulator),
> +				     "Failed to get regulator 'io'\n");
> +
> +	dbidev->backlight_node = of_parse_phandle(dev->of_node, "backlight", 0);
> +
> +	dbi->reset = gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
> +	if (dbi->reset < 0 && dbi->reset != -ENOENT)
> +		return dev_err_probe(dev, dbi->reset, "Failed to get GPIO 'reset'\n");
> +
> +	dc = gpiod_get(dev, "dc", GPIOD_OUT_LOW);
> +	if (dc < 0 && dc != -ENOENT)
> +		return dev_err_probe(dev, dc, "Failed to get GPIO 'dc'\n");
> +
> +	ret = mipi_dbi_spi_init(spi, dbi, dc);
> +	if (ret)
> +		return ret;
> +
> +	ret = mipi_dbi_dev_init(dbidev, &panel_mipi_dbi_ops, &dbidev->mode);
> +	if (ret)
> +		return ret;
> +
> +	ret = register_framebuffer(info);
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret, "Failed to register framebuffer\n");
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id panel_mipi_dbi_spi_of_match[] = {
> +	{ .compatible = "panel-mipi-dbi-spi" },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, panel_mipi_dbi_spi_of_match);
> +
> +static struct driver panel_mipi_dbi_spi_driver = {
> +	.name = "panel-mipi-dbi-spi",
> +	.probe = panel_mipi_dbi_spi_probe,
> +	.of_compatible = DRV_OF_COMPAT(panel_mipi_dbi_spi_of_match),
> +};
> +device_spi_driver(panel_mipi_dbi_spi_driver);
> +
> +MODULE_DESCRIPTION("MIPI DBI compatible display panel driver");
> +MODULE_AUTHOR("Noralf Trønnes");
> +MODULE_LICENSE("GPL");

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 3/4] video: add MIPI DBI framebuffer helpers

[Philipp Zabel]

Hi Ahmad,

thank you for the review.

On Thu, Mar 30, 2023 at 10:43:54AM +0200, Ahmad Fatoum wrote:
> On 29.03.23 12:56, Philipp Zabel wrote:
[...]
> > +static void mipi_dbi_buf_copy(void *dst, struct fb_info *info, bool swap)
> > +{
> > +	u16 *src = (u16 *)info->screen_base;
> > +	u16 *dst16 = dst;
> > +	size_t len = info->xres * info->yres;
> > +	int i;
> > +
> > +	if (swap) {
> > +		for (i = 0; i < len; i++) {
> > +			*dst16++ = *src << 8 | *src >> 8;
> > +			src++;
> > +		}
> > +	} else {
> > +		memcpy(dst, src, len * 2);
> 
> Do we need this? Why can't we send out framebuffer directly if there is no
> swapping to be done?

We do not. If byte swapping is not necessary, we can transfer directly
from the framebuffer, as long as this driver only supports 16-bit
RGB565. I'll improve this in v2.

[...]
> > +/**
> > + * mipi_dbi_enable_flush - MIPI DBI enable helper
> > + * @dbidev: MIPI DBI device structure
> > + * @crtc_state: CRTC state
> > + * @plane_state: Plane state
> > + *
> > + * Flushes the whole framebuffer and enables the backlight. Drivers can use this
> > + * in their &drm_simple_display_pipe_funcs->enable callback.
> 
> fb_ops->fb_enable ?

Yes. The whole comment slipped through unchanged from the Linux driver.
Will fix.

[...]
> > +void mipi_dbi_fb_disable(struct fb_info *info)
> > +{
> > +	struct mipi_dbi_dev *dbidev = container_of(info, struct mipi_dbi_dev, info);
> > +
> > +	if (dbidev->backlight)
> > +		backlight_set_brightness(dbidev->backlight, 0);
> > +	else
> > +		mipi_dbi_blank(dbidev);
> > +
> > +	if (dbidev->regulator)
> > +		regulator_disable(dbidev->regulator);
> > +	if (dbidev->io_regulator)
> > +		regulator_disable(dbidev->io_regulator);
> 
> Calling regulator_disable on NULL pointer is a no-op.

Ah right, this is different from Linux.
I'll remove the unnecessary checks.

> > +int mipi_dbi_dev_init(struct mipi_dbi_dev *dbidev, struct fb_ops *ops,
> > +		      struct fb_videomode *mode)
> > +{
> > +	struct fb_info *info = &dbidev->info;
> > +
> > +	info->mode = mode;
> > +	info->fbops = ops;
> > +	info->dev.parent = dbidev->dev;
> > +
> > +	info->xres = mode->xres;
> > +	info->yres = mode->yres;
> > +	info->bits_per_pixel = 16;
> > +	info->line_length = info->xres * (info->bits_per_pixel >> 3);
> > +
> > +	info->screen_size = info->line_length * info->yres;
> > +	info->screen_base = kzalloc(info->screen_size, GFP_KERNEL);
> 
> dma_alloc? In case some SPI driver gets DMA support.

Ok.

> > +
> > +	info->red.length = 5;
> > +	info->red.offset = 11;
> > +	info->green.length = 6;
> > +	info->green.offset = 5;
> > +	info->blue.length = 5;
> > +	info->blue.offset = 0;
> > +
> > +	dbidev->tx_buf = kzalloc(mode->xres * mode->yres * 2, GFP_KERNEL);
> 
> Use info->screen_size here as well?

Yes. This driver doesn't support 32-bit framebuffers with conversion
to 16-bit transfers, so this can use info->screen_size as well.

regards
Philipp