Hi ChenYu, Thank you for the detailed review and the pointers toward the documentation. I will ensure the headers are sorted alphabetically and the driver name conflict is resolved in the next iteration.
> The reset logic in mipi_dbi is inverted when compared to panel-st7789v. > mipi_dbi needs to be taught the "proper" reset polarity. Noted. I will look into the mipi_dbi core to see how to handle the reset polarity correctly. > Instead this functionality could be merged into the existing panel-st7789v > driver. You mentioned above that that driver only supports the 9-bit SPI > transfer mode. However porting that driver over to mipi_dbi would fix this, > and remove some redundant code. And tinydrm support could be added on top > of that. > > I actually mentioned I was going to work on this on IRC. But I only ported > the driver over to use mipi_dbi, and haven't gotten around to adding > tinydrm support. I can send out the conversion patches if that helps > you. That would be fantastic and would save a lot of redundant effort. If you send out the patches to convert the existing panel-st7789v driver to mipi_dbi, I would be happy to build the 'tiny' (simple display pipe) support on top of your series. Please CC me when you post them, and I will rebase my work accordingly. Best regards, Archit On Sat, Feb 14, 2026 at 6:35 PM Chen-Yu Tsai <[email protected]> wrote: > > On Sat, Feb 14, 2026 at 5:21 PM Archit Anant <[email protected]> wrote: > > > > Add a DRM driver for Sitronix ST7789V display controllers using the > > mipi_dbi interface. > > > > Currently, support for this controller is split between a legacy fbdev > > driver in staging (fb_st7789v.c) and a DRM panel driver that requires > > 9-bit SPI words (panel-sitronix-st7789v.c). This new driver uses the > > mipi_dbi helper to support standard 8-bit SPI with a D/C GPIO, which > > is the configuration used by the vast majority of hobbyist and > > embedded hardware. > > Notes about this below. > > > > > The initialization sequence is ported from the staging driver and > > supports several panels: > > - Generic 240x320 profile > > - HannStar HSD20 IPS > > - Inanbo T28CP45TN89-V17 > > - EDT ET028013DMA > > - Jasonic JT240MHQS-HWT-EK-E3 > > First of all, please run scripts/checkpatch.pl on patches before you send > them. The indentation is all wrong. The script would have caught it. > > And check out Documentation/process/submitting-patches.rst for any other > steps you should do before submitting patches. > > > Signed-off-by: Archit Anant <[email protected]> > > --- > > drivers/gpu/drm/sitronix/Kconfig | 17 ++ > > drivers/gpu/drm/sitronix/Makefile | 2 + > > drivers/gpu/drm/sitronix/st7789v.c | 307 +++++++++++++++++++++++++++++ > > 3 files changed, 326 insertions(+) > > create mode 100644 drivers/gpu/drm/sitronix/st7789v.c > > > > diff --git a/drivers/gpu/drm/sitronix/Kconfig > > b/drivers/gpu/drm/sitronix/Kconfig > > index 6de7d92d9b74..7a2c66677003 100644 > > --- a/drivers/gpu/drm/sitronix/Kconfig > > +++ b/drivers/gpu/drm/sitronix/Kconfig > > @@ -40,3 +40,20 @@ config DRM_ST7735R > > > > If M is selected the module will be called st7735r. > > > > +config DRM_ST7789V > > + tristate "DRM support for Sitronix ST7789V display panels" > > + depends on DRM && SPI > > + select DRM_CLIENT_SELECTION > > + select DRM_GEM_DMA_HELPER > > + select DRM_KMS_HELPER > > + select DRM_MIPI_DBI > > + select BACKLIGHT_CLASS_DEVICE > > + help > > + DRM driver for Sitronix ST7789V panels connected via SPI. > > + This driver supports several panels including: > > + * HannStar HSD20 IPS > > + * Inanbo T28CP45TN89-V17 > > + * EDT ET028013DMA > > + * Jasonic JT240MHQS-HWT-EK-E3 > > + > > + If M is selected the module will be called st7789v. > > diff --git a/drivers/gpu/drm/sitronix/Makefile > > b/drivers/gpu/drm/sitronix/Makefile > > index bd139e5a6995..96b2a4d85368 100644 > > --- a/drivers/gpu/drm/sitronix/Makefile > > +++ b/drivers/gpu/drm/sitronix/Makefile > > @@ -1,3 +1,5 @@ > > obj-$(CONFIG_DRM_ST7571_I2C) += st7571-i2c.o > > obj-$(CONFIG_DRM_ST7586) += st7586.o > > obj-$(CONFIG_DRM_ST7735R) += st7735r.o > > +obj-$(CONFIG_DRM_ST7789V) += st7789v.o > > + > > Empty line not needed. > > > diff --git a/drivers/gpu/drm/sitronix/st7789v.c > > b/drivers/gpu/drm/sitronix/st7789v.c > > new file mode 100644 > > index 000000000000..4ce4b46d8df2 > > --- /dev/null > > +++ b/drivers/gpu/drm/sitronix/st7789v.c > > @@ -0,0 +1,307 @@ > > +// SPDX-License-Identifier: GPL-2.0-or-later > > +/* > > + * DRM driver for Sitronix ST7789V LCD panels > > + * > > + * Copyright (C) 2026 Archit Anant <[email protected]> > > + */ > > + > > +#include <linux/bits.h> > > +#include <linux/module.h> > > +#include <linux/kernel.h> > > +#include <linux/spi/spi.h> > > +#include <linux/gpio/consumer.h> > > +#include <linux/delay.h> > > +#include <linux/backlight.h> > > Sort this group alphabetically. > > > + > > +#include <drm/drm_device.h> > > +#include <drm/drm_drv.h> > > +#include <drm/drm_managed.h> > > +#include <drm/drm_mipi_dbi.h> > > +#include <drm/drm_modes.h> > > +#include <drm/drm_gem_dma_helper.h> > > +#include <drm/drm_atomic_helper.h> > > +#include <drm/clients/drm_client_setup.h> > > +#include <drm/drm_fbdev_dma.h> > > Sort this group alphabetically. > > And add an empty line here for group separation. > > > +#include <video/mipi_display.h> > > + > > +#define ST7789V_PORCTRL 0xb2 > > +#define ST7789V_GCTRL 0xb7 > > +#define ST7789V_VCOMS 0xbb > > +#define ST7789V_LCMCTRL 0xc0 > > +#define ST7789V_VDVVRHEN 0xc2 > > +#define ST7789V_VRHS 0xc3 > > +#define ST7789V_VDVS 0xc4 > > +#define ST7789V_VCMOFSET 0xc5 > > +#define ST7789V_FRCTRL2 0xc6 > > +#define ST7789V_PWCTRL1 0xd0 > > +#define ST7789V_PVGAMCTRL 0xe0 > > +#define ST7789V_NVGAMCTRL 0xe1 > > + > > +#define ST7789V_MADCTL_MY BIT(7) > > +#define ST7789V_MADCTL_MX BIT(6) > > +#define ST7789V_MADCTL_MV BIT(5) > > +#define ST7789V_MADCTL_BGR BIT(3) > > + > > + > > +struct st7789v_cfg { > > + const struct drm_display_mode mode; > > + unsigned int left_offset; > > + unsigned int top_offset; > > + bool is_ips; /* Controls PORCTRL and GCTRL timings */ > > + bool invert; /* Controls Color Inversion (positive/negative) */ > > +}; > > +struct st7789v_priv { > > + struct mipi_dbi_dev dbidev; /* Must be first for .release() */ > > + const struct st7789v_cfg *cfg; > > +}; > > + > > + > > +/* 1. Generic Fallback (Matches default behavior of fb_st7789v.c) */ > > +static const struct st7789v_cfg generic_cfg = { > > + .mode = { DRM_SIMPLE_MODE(240, 320, 0, 0) }, > > + .is_ips = false, > > + .invert = true, > > +}; > > + > > +/* 2. HannStar 2.0" IPS (The specific panel handled in staging) */ > > +static const struct st7789v_cfg hsd20_ips_cfg = { > > + .mode = { DRM_SIMPLE_MODE(240, 320, 31, 41) }, > > + .is_ips = true, > > + .invert = true, > > +}; > > + > > +/* 3. Inanbo 2.8" (From the 9-bit driver: No Inversion) */ > > +static const struct st7789v_cfg inanbo_panel_cfg = { > > + .mode = { DRM_SIMPLE_MODE(240, 320, 43, 57) }, > > + .is_ips = false, > > + .invert = false, > > +}; > > + > > +/* 4. EDT 2.8" (From the 9-bit driver: Normal Inversion) */ > > +static const struct st7789v_cfg edt_panel_cfg = { > > + .mode = { DRM_SIMPLE_MODE(240, 320, 43, 58) }, > > + .is_ips = false, > > + .invert = true, > > +}; > > + > > +/* 5. Jasonic 2.4" (From the 9-bit driver: Custom Height + Offset) */ > > +static const struct st7789v_cfg jasonic_panel_cfg = { > > + .mode = { DRM_SIMPLE_MODE(240, 280, 37, 43) }, > > + .is_ips = true, > > + .invert = true, > > + .top_offset = 38, > > +}; > > + > > +DEFINE_DRM_GEM_DMA_FOPS(st7789v_fops); > > + > > +static const struct drm_driver st7789v_driver = { > > + .driver_features = DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC, > > + .fops = &st7789v_fops, > > + DRM_GEM_DMA_DRIVER_OPS_VMAP, > > + DRM_FBDEV_DMA_DRIVER_OPS, > > + .debugfs_init = mipi_dbi_debugfs_init, > > + .name = "st7789v", > > + .desc = "Sitronix ST7789V", > > + .major = 1, > > + .minor = 0, > > +}; > > + > > +static void st7789v_pipe_enable(struct drm_simple_display_pipe *pipe, > > + struct drm_crtc_state *crtc_state, > > + struct drm_plane_state *plane_state) > > +{ > > + struct mipi_dbi_dev *dbidev = drm_to_mipi_dbi_dev(pipe->crtc.dev); > > + struct st7789v_priv *priv = container_of(dbidev, struct st7789v_priv, > > dbidev); > > + struct mipi_dbi *dbi = &dbidev->dbi; > > + int ret,idx; > > + > > + if (!drm_dev_enter(pipe->crtc.dev, &idx)) > > + return; > > + > > + ret = mipi_dbi_poweron_reset(dbidev); > > + if (ret) > > + goto out_exit; > > + > > + mipi_dbi_command(dbi, MIPI_DCS_SOFT_RESET); > > + msleep(150); > > + > > + mipi_dbi_command(dbi, MIPI_DCS_EXIT_SLEEP_MODE); > > + msleep(500); > > + > > + mipi_dbi_command(dbi, MIPI_DCS_SET_PIXEL_FORMAT, > > MIPI_DCS_PIXEL_FMT_16BIT); > > + > > + if (priv->cfg->is_ips) { > > + mipi_dbi_command(dbi, ST7789V_PORCTRL, 0x05, 0x05, 0x00, 0x33, > > 0x33); > > + mipi_dbi_command(dbi, ST7789V_GCTRL, 0x75); > > + } else { > > + mipi_dbi_command(dbi, ST7789V_PORCTRL, 0x0c, 0x0c, 0x00, 0x33, > > 0x33); > > + mipi_dbi_command(dbi, ST7789V_GCTRL, 0x35); > > + } > > + > > + mipi_dbi_command(dbi, ST7789V_VCOMS, 0x20); > > + mipi_dbi_command(dbi, ST7789V_LCMCTRL, 0x2c); > > + mipi_dbi_command(dbi, ST7789V_VDVVRHEN, 0x01); > > + mipi_dbi_command(dbi, ST7789V_VRHS, 0x12); > > + mipi_dbi_command(dbi, ST7789V_VDVS, 0x20); > > + mipi_dbi_command(dbi, ST7789V_FRCTRL2, 0x0f); > > + mipi_dbi_command(dbi, ST7789V_PWCTRL1, 0xa4, 0xa1); > > + > > + mipi_dbi_command(dbi, ST7789V_PVGAMCTRL, > > + 0xd0, 0x04, 0x0d, 0x11, 0x13, 0x2b, 0x3f, 0x54, > > + 0x4c, 0x18, 0x0d, 0x0b, 0x1f, 0x23); > > + mipi_dbi_command(dbi, ST7789V_NVGAMCTRL, > > + 0xd0, 0x04, 0x0c, 0x11, 0x13, 0x2c, 0x3f, 0x44, > > + 0x51, 0x2f, 0x1f, 0x1f, 0x20, 0x23); > > + > > + if (priv->cfg->invert) > > + mipi_dbi_command(dbi, MIPI_DCS_ENTER_INVERT_MODE); > > + else > > + mipi_dbi_command(dbi, MIPI_DCS_EXIT_INVERT_MODE); > > + mipi_dbi_command(dbi, MIPI_DCS_SET_DISPLAY_ON); > > + msleep(100); > > + > > + u8 addr_mode = 0; > > + > > + switch (dbidev->rotation) { > > + case 90: > > + > > + addr_mode = ST7789V_MADCTL_MV | ST7789V_MADCTL_MY; > > + break; > > + case 180: > > + addr_mode = ST7789V_MADCTL_MX | ST7789V_MADCTL_MY; > > + break; > > + case 270: > > + addr_mode = ST7789V_MADCTL_MV | ST7789V_MADCTL_MX; > > + break; > > + default: > > + addr_mode = 0; > > + break; > > + } > > + > > + addr_mode |= ST7789V_MADCTL_BGR; > > + > > + mipi_dbi_command(dbi, MIPI_DCS_SET_ADDRESS_MODE, addr_mode); > > + > > + mipi_dbi_enable_flush(dbidev, crtc_state, plane_state); > > + > > +out_exit: > > + drm_dev_exit(idx); > > +} > > + > > +static const struct drm_simple_display_pipe_funcs st7789v_pipe_funcs = > > +{ > > + DRM_MIPI_DBI_SIMPLE_DISPLAY_PIPE_FUNCS(st7789v_pipe_enable), > > +}; > > + > > +static int st7789v_probe(struct spi_device *spi) > > +{ > > + struct device *dev = &spi->dev; > > + const struct st7789v_cfg *cfg; > > + struct mipi_dbi_dev *dbidev; > > + struct st7789v_priv *priv; > > + struct drm_device *drm; > > + struct mipi_dbi *dbi; > > + struct gpio_desc *dc; > > + u32 rotation = 0; > > + int ret; > > + > > + cfg = device_get_match_data(&spi->dev); > > + > > + if (!cfg) > > + cfg = (void *)spi_get_device_id(spi)->driver_data; > > + > > + priv = devm_drm_dev_alloc(dev, &st7789v_driver, > > + struct st7789v_priv, dbidev.drm); > > + > > + if (IS_ERR(priv)) > > + return PTR_ERR(priv); > > + > > + dbidev = &priv->dbidev; > > + priv->cfg = cfg; > > + > > + dbi = &dbidev->dbi; > > + drm = &dbidev->drm; > > + > > + spi_set_drvdata(spi, drm); > > Maybe also add power supply regulator support to make it complete? > > > + dbi->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH); > > + if (IS_ERR(dbi->reset)) > > + return dev_err_probe(dev, PTR_ERR(dbi->reset), "Failed to get GPIO > > 'reset'\n"); > > The reset logic in mipi_dbi is inverted when compared to panel-st7789v. > mipi_dbi needs to be taught the "proper" reset polarity. > > > + > > + dc = devm_gpiod_get(dev, "dc", GPIOD_OUT_LOW); > > + if (IS_ERR(dc)) > > + return dev_err_probe(dev, PTR_ERR(dc), "Failed to get GPIO > > 'dc'\n"); > > This should be optional, since mipi_dbi can handle the "no dc" case. > > > + > > + dbidev->backlight = devm_of_find_backlight(dev); > > + if (IS_ERR(dbidev->backlight)) > > + return PTR_ERR(dbidev->backlight); > > + > > + dbidev->left_offset = priv->cfg->left_offset; > > + dbidev->top_offset = priv->cfg->top_offset; > > + > > + device_property_read_u32(dev, "rotation", &rotation); > > + > > + ret = mipi_dbi_spi_init(spi, dbi, dc); > > + if (ret) > > + return ret; > > + > > + ret = mipi_dbi_dev_init(dbidev, &st7789v_pipe_funcs, &cfg->mode, > > rotation); > > + if (ret) > > + return ret; > > + > > + drm_mode_config_reset(drm); > > + > > + ret = drm_dev_register(drm, 0); > > + if (ret) > > + return ret; > > + > > + drm_client_setup(drm, NULL); > > + > > + return 0; > > +} > > + > > +static void st7789v_remove(struct spi_device *spi) > > +{ > > + struct drm_device *drm = spi_get_drvdata(spi); > > + drm_dev_unplug(drm); > > + drm_atomic_helper_shutdown(drm); > > +} > > + > > +static void st7789v_shutdown(struct spi_device *spi) > > +{ > > + drm_atomic_helper_shutdown(spi_get_drvdata(spi)); > > +} > > + > > + > > +static const struct of_device_id st7789v_of_match[] = { > > + { .compatible = "sitronix,st7789v", .data = &generic_cfg }, > > + { .compatible = "hannstar,hsd20-ips", .data = &hsd20_ips_cfg }, > > + { .compatible = "inanbo,t28cp45tn89-v17", .data = &inanbo_panel_cfg > > }, > > + { .compatible = "edt,et028013dma", .data = &edt_panel_cfg }, > > + { .compatible = "jasonic,jt240mhqs-hwt-ek-e3", .data = > > &jasonic_panel_cfg }, > > + { } > > +}; > > +MODULE_DEVICE_TABLE(of, st7789v_of_match); > > We typically don't want two drivers matching on the same devices. The > driver match and probe order becomes hard to predict. > > Instead this functionality could be merged into the existing panel-st7789v > driver. You mentioned above that that driver only supports the 9-bit SPI > transfer mode. However porting that driver over to mipi_dbi would fix this, > and remove some redundant code. And tinydrm support could be added on top > of that. > > I actually mentioned I was going to work on this on IRC. But I only ported > the driver over to use mipi_dbi, and haven't gotten around to adding > tinydrm support. I can send out the conversion patches if that helps > you. > > > + > > +static const struct spi_device_id st7789v_id[] = { > > + { "st7789v", 0 }, > > You should add all the other ones as well. OF-based SPI driver module > loading doesn't seem to work. I think that's a known issue? > > > + { }, > > +}; > > +MODULE_DEVICE_TABLE(spi, st7789v_id); > > + > > +static struct spi_driver st7789v_spi_driver = { > > + .driver = { > > + .name = "st7789v", > > And you definitely can't have two drivers with the same name. > > > + .of_match_table = st7789v_of_match, > > + }, > > + .probe = st7789v_probe, > > + .remove = st7789v_remove, > > + .shutdown = st7789v_shutdown, > > + .id_table = st7789v_id, > > +}; > > + > > Empty line not needed here. > > > +module_spi_driver(st7789v_spi_driver); > > + > > +MODULE_DESCRIPTION("Sitronix ST7789V DRM driver"); > > +MODULE_AUTHOR("Archit Anant <[email protected]>"); > > +MODULE_LICENSE("GPL"); > > \ No newline at end of file > > ^ > > > ChenYu -- Sincerely, Archit Anant
