Re: [PATCH v3 04/14] drm/solomon: Add DRM driver for Solomon SSD16xx e-paper display controllers
From: Javier Martinez Canillas
Date: Tue Nov 10 2026 - 07:10:36 EST
Hello Devarsh,
Devarsh Thakkar <devarsht@xxxxxx> writes:
> Add a DRM driver for the Solomon Systech SSD16xx family of e-paper display
> controllers (SSD1680 [4], SSD1673 [3], SSD1681 [5], SSD1683 [1]) based
> e-paper display panels, starting with support for the Gooddisplay
> GDEY042T81 which is 4.2", 400x300 resolution black/white e-paper display
> using SSD1683 controller.
>
[...]
> new file mode 100644
> index 000000000000..ca6550b5dd5b
> --- /dev/null
> +++ b/drivers/gpu/drm/solomon/ssd16xx.c
> @@ -0,0 +1,1801 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * DRM driver for e-paper display panels using Solomon SSD16xx family controllers
> + *
> + * Copyright (C) 2026 Texas Instruments Incorporated - https://www.ti.com/
> + *
> + * Author: Devarsh Thakkar <devarsht@xxxxxx>
> + *
> + * References: https://github.com/Lesords/epaper
> + */
What is this reference about? I would just remove it.
[...]
> +#include <drm/drm_fb_helper.h>
This header does not exist anymore, check commit 6ccc5b5fc1d3 ("drm/fbdev:
Move fbdev helpers into client library"). But I think you can just drop it
no symbols from clients/drm_fbdev_helper.h are used as far as I can tell.
Also, please make sure to use the drm-misc/drm-misc-next branch as a base.
[...]
> +struct ssd16xx_device {
> + struct drm_device drm;
> +
> + struct drm_plane primary_plane;
> + struct drm_crtc crtc;
> + struct drm_encoder encoder;
> + struct drm_connector connector;
> +
> + struct spi_device *spi;
> + struct gpio_desc *reset;
> + struct gpio_desc *busy;
> + struct gpio_desc *dc;
> +
> + enum ssd16xx_model model;
> + enum ssd16xx_controller controller;
> + const struct ssd16xx_controller_config *controller_cfg;
> + const struct ssd16xx_device_config *device_cfg;
> + struct drm_display_mode *mode;
> + u32 width;
> + u32 height;
> +
> + bool initialized;
> + bool init_refresh_pending; /* First frame after refresh_mode_init enable */
> +
> + int orientation; /* Display orientation in degrees: 0/90/180/270 */
> + enum ssd16xx_refresh_mode refresh_mode; /* Active refresh mode */
> + enum ssd16xx_color_mode color_mode; /* Active color mode (BW or 3-color) */
> + bool fast_lut_pending; /* LUT pre-load needed before next fast refresh */
> +
> + /* Border waveform (as enum indices) */
> + int border_waveform_init_idx; /* Border waveform during clear/init */
> + int border_waveform_update_idx; /* Border waveform during display updates */
> + bool border_refresh_on_every_update; /* Re-send border cmd each display update */
> + bool border_waveform_pending; /* One-shot: send border cmd on next update */
> +
> + /* Display control */
> + int refresh_mode_init; /* -1=disabled, else use this mode for the first frame */
> +
> + u8 *tx_buf; /* 1bpp frame buffer (mono + white) */
> + u8 *tx_red_buf; /* 1bpp red-channel buffer (3-color panels only) */
> + u16 *tx_buf9; /* 9-bit SPI expansion buffer (3-wire mode only) */
> + u8 spi_buf[64] ____cacheline_aligned; /* DMA-safe buffer for SPI transfers */
Another option would be to dynamically allocate them using kzalloc(),
which already provides naturally cache-line-aligned buffers.
[...]
> +static void ssd16xx_send_cmd(struct ssd16xx_device *device, u8 cmd,
> + int *err)
> +{
> + struct spi_transfer xfer = {};
> + struct spi_message msg;
> +
> + if (*err)
> + return;
> +
> + spi_message_init(&msg);
> + spi_message_add_tail(&xfer, &msg);
> +
> + if (device->dc) {
> + /* 4-wire SPI: D/C# GPIO low selects command mode */
> + device->spi_buf[0] = cmd;
> + xfer.tx_buf = device->spi_buf;
> + xfer.len = 1;
> + gpiod_set_value_cansleep(device->dc, 0);
> + } else {
> + /*
> + * 3-wire SPI (9-bit): bit 8 is the D/C# bit.
> + * D/C# = 0 means the following 8 bits are a command.
> + */
> + u16 *word = (u16 *)device->spi_buf;
> +
> + *word = cmd; /* bit 8 = 0 for command */
> + xfer.tx_buf = device->spi_buf;
> + xfer.len = sizeof(u16);
> + xfer.bits_per_word = 9;
> + }
> +
> + ssd16xx_spi_sync(device->spi, &msg, err);
> +}
> +
> +static void ssd16xx_send_data(struct ssd16xx_device *device, u8 data,
> + int *err)
> +{
> + struct spi_transfer xfer = {};
> + struct spi_message msg;
> +
> + if (*err)
> + return;
> +
> + spi_message_init(&msg);
> + spi_message_add_tail(&xfer, &msg);
> +
> + if (device->dc) {
> + /* 4-wire SPI: D/C# GPIO high selects data mode */
> + device->spi_buf[0] = data;
> + xfer.tx_buf = device->spi_buf;
> + xfer.len = 1;
> + gpiod_set_value_cansleep(device->dc, 1);
> + } else {
> + /*
> + * 3-wire SPI (9-bit): bit 8 is the D/C# bit.
> + * D/C# = 1 means the following 8 bits are data.
> + */
> + u16 *word = (u16 *)device->spi_buf;
> +
> + *word = 0x100 | data;
> + xfer.tx_buf = device->spi_buf;
> + xfer.len = sizeof(u16);
> + xfer.bits_per_word = 9;
> + }
> +
> + ssd16xx_spi_sync(device->spi, &msg, err);
> +}
> +
As mentioned in a previous version, this logic is already partially
covered by the ssd130x_spi_write() function (for 4-wire SPI mode),
but that helper lacks the 3-wire SPI mode.
Once your driver lands, it would be great to have a single transfer
helper that could be shared by both drivers. And maybe even other
KMS drivers could benefit from it.
> +static void ssd16xx_send_x_param(struct ssd16xx_device *device, u16 x,
> + int *err)
> +{
> + if (*err)
> + return;
> +
> + if (device->controller_cfg->ram_x_address_bits == 8) {
> + ssd16xx_send_data(device, (u8)x, err);
> + } else {
> + ssd16xx_send_data(device, x & 0xFF, err);
> + ssd16xx_send_data(device, (x >> 8) & 0xFF, err);
> + }
> +}
> +
> +static void ssd16xx_send_y_param(struct ssd16xx_device *device, u16 y,
> + int *err)
> +{
> + if (*err)
> + return;
> +
> + if (device->controller_cfg->ram_y_address_bits == 8) {
> + ssd16xx_send_data(device, (u8)y, err);
> + } else {
> + ssd16xx_send_data(device, y & 0xFF, err);
> + ssd16xx_send_data(device, (y >> 8) & 0xFF, err);
> + }
> +}
> +
Maybe these two helpers can be consolidated in one? Callers can just
pass the ram_{x,y}_address_bits as another parameter.
> +static void ssd16xx_send_data_bulk(struct ssd16xx_device *device,
> + const u8 *data, size_t len,
> + int *err)
> +{
> + struct spi_transfer xfer = {};
> + struct spi_message msg;
> +
> + if (*err)
> + return;
> +
> + if (!data || !len)
> + return;
> +
> + spi_message_init(&msg);
> + spi_message_add_tail(&xfer, &msg);
> +
> + if (device->dc) {
> + /* 4-wire SPI: D/C# GPIO high selects data mode */
> + if (len <= sizeof(device->spi_buf)) {
> + memcpy(device->spi_buf, data, len);
> + xfer.tx_buf = device->spi_buf;
> + } else {
> + xfer.tx_buf = data;
> + }
> + xfer.len = len;
> + gpiod_set_value_cansleep(device->dc, 1);
> + ssd16xx_spi_sync(device->spi, &msg, err);
> + } else {
> + /* 3-wire (9-bit): expand u8 → u16 with D/C#=1 in bit 8. */
> + size_t i;
> + u16 *buf = device->tx_buf9;
> +
> + for (i = 0; i < len; i++)
> + buf[i] = 0x100 | data[i];
> +
> + xfer.tx_buf = buf;
> + xfer.len = len * sizeof(u16);
> + xfer.bits_per_word = 9;
> + ssd16xx_spi_sync(device->spi, &msg, err);
> + }
> +}
> +
Is the ssd16xx_send_data() helper needed or could just this be used with
len == 1 ? In general I feel there is a lot of code duplication on these
transfer helpers, and maybe you could simplify it a little bit.
> +static void ssd16xx_hw_reset(struct ssd16xx_device *device)
> +{
> + gpiod_set_value_cansleep(device->reset, 1);
> + usleep_range(10000, 11000);
> + gpiod_set_value_cansleep(device->reset, 0);
> + usleep_range(10000, 11000);
> +}
> +
This again is quite similar to ssd130x_reset(), not for now but these are
the kind of things that I wonder if could be moved to a solomon-common.c
library or something like that.
[...]
> +/*
> + * Convert framebuffer to 1-bit monochrome for e-paper display.
> + *
> + * Supports XRGB8888 (thresholded via ITU-R BT.601 luma at 127) and R1
> + * (native 1bpp, zero-copy fast path for aligned landscape frames).
> + *
> + * Output layout:
> + * 0°/180° landscape: row-major, left-to-right, top-to-bottom
> + * 90°/270° CW portrait: column-major, rightmost column first
> + */
> +static void ssd16xx_convert_fb_to_1bpp(u8 *dst, struct iosys_map *src,
> + struct drm_framebuffer *fb,
> + struct drm_rect *rect,
> + unsigned int orientation)
> +{
Maybe you can split this function in two, one that does the XRGB8888 -> R1
and another one that does the transposition. There is already a helper to
do drm_fb_xrgb8888_to_mono(), but that assumes LSB while you want MSB. So
an option is to add a bit-order parameter to that helper.
[...]
> +/* -----------------------------------------------------------------------------
> + * Plane Functions
> + */
> +
> +static void ssd16xx_plane_destroy(struct drm_plane *plane)
> +{
> + drm_plane_cleanup(plane);
> +}
> +
> +static const struct drm_plane_funcs ssd16xx_plane_funcs = {
> + .update_plane = drm_atomic_helper_update_plane,
> + .disable_plane = drm_atomic_helper_disable_plane,
> + .destroy = ssd16xx_plane_destroy,
why the level of indirection? You can just set to drm_plane_cleanup()
and delete ssd16xx_plane_destroy().
> +static void ssd16xx_plane_atomic_update(struct drm_plane *plane,
> + struct drm_atomic_commit *state)
> +{
> + struct drm_plane_state *old_state = drm_atomic_get_old_plane_state(state, plane);
> + struct drm_plane_state *new_state = drm_atomic_get_new_plane_state(state, plane);
> + struct drm_shadow_plane_state *shadow_state = to_drm_shadow_plane_state(new_state);
> + struct ssd16xx_device *device = plane_to_ssd16xx_device(plane);
> + enum ssd16xx_refresh_mode saved_mode;
> + u8 saved_border_waveform_idx;
> + struct drm_framebuffer *fb = new_state->fb;
> + struct drm_rect rect;
> + int ret, idx;
> +
> + if (!drm_dev_enter(&device->drm, &idx))
> + return;
> +
> + drm_dbg(&device->drm, "plane_atomic_update: fb=%p\n", fb);
> +
> + if (!fb || !device->initialized)
> + goto out_exit;
> +
> + if (!drm_atomic_helper_damage_merged(old_state, new_state, &rect))
> + goto out_exit;
> +
Do you think that creating a big merged damage rectangle is better than
iterating over the damaged areas and calling drm_rect_intersect() for each?
The optimal approach varies per device and maybe merging a big rectangle is
the correct thing to do for e-paper displays, but I thought that was worth
to mention that other drivers do it differently.
> + drm_dbg(&device->drm, "plane_atomic_update: calling fb_dirty rect=(%d,%d)-(%d,%d)\n",
> + rect.x1, rect.y1, rect.x2, rect.y2);
> + /*
> + * When refresh_mode_init was set, use the specified mode for this first
> + * frame only, then restore the user-configured refresh_mode so
> + * subsequent updates continue with the configured mode.
> + */
> + saved_mode = device->refresh_mode;
> + saved_border_waveform_idx = device->border_waveform_update_idx;
> + if (device->init_refresh_pending) {
> + device->refresh_mode = device->refresh_mode_init;
> + device->border_waveform_update_idx = device->border_waveform_init_idx;
> + }
> +
> + /*
> + * Fast refresh (0xC7) omits LOAD_LUT on every update cycle and relies
> + * on the LUT being pre-loaded upfront. The property setter arms
> + * fast_lut_pending whenever the user switches into fast mode. Consume
> + * the flag here (once) before the first fast-refresh frame so the
> + * controller's LUT is in the correct state.
> + */
> + if (device->fast_lut_pending) {
> + ret = ssd16xx_preload_fast_lut(device);
> + if (ret) {
> + drm_err(&device->drm,
> + "plane_atomic_update: fast LUT preload failed: %d\n", ret);
> + }
> +
> + device->fast_lut_pending = false;
> + }
> +
> + ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
I *think* that you need to do this earlier, before the damage checks.
[...]
> +static const struct drm_connector_funcs ssd16xx_connector_funcs = {
> + .reset = drm_atomic_helper_connector_reset,
This hook has been removed since commit ("27cae3ef0574 drm/connector:
Remove reset"), instead you need to do the following:
.atomic_create_state = drm_atomic_helper_connector_create_state,
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat