Re: [PATCH RFC 2/9] drm: panel: Add panel for Epson Moverio BT200
From: Uwe Kleine-König
Date: Tue Sep 22 2026 - 05:18:45 EST
Hallo Andreas,
On Tue, Sep 22, 2026 at 08:16:59AM +0200, Andreas Kemnade wrote:
> diff --git a/drivers/gpu/drm/panel/panel-epson-bt200.c b/drivers/gpu/drm/panel/panel-epson-bt200.c
> new file mode 100644
> index 000000000000..c26a4f8ae15e
> --- /dev/null
> +++ b/drivers/gpu/drm/panel/panel-epson-bt200.c
> @@ -0,0 +1,251 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +#include <linux/backlight.h>
> +#include <linux/errno.h>
> +#include <linux/delay.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
Please don't include <linux/mod_devicetable.h>. You can rely on
<linux/spi/spi.h> to provide of_device_id and spi_device_id; or if you
prefer full iwyu include <linux/device-id/of.h> and <.../spi.h>.
> +#include <linux/spi/spi.h>
> +
> +#include <drm/drm_connector.h>
> +#include <drm/drm_modes.h>
> +#include <drm/drm_panel.h>
> +#include <drm/drm_print.h>
> +#include <drm/drm_probe_helper.h>
> +
> +static const struct drm_display_mode default_mode = {
> + .clock = 41600, /* kHz */
> +#if 0
huh, upstream code isn't supposed to have #if 0 blocks.
> + /* values matching struct omap_video_timings in vendor kernel */
> + .hdisplay = 960,
> + .hsync_start = 960 + 176,
> + .hsync_end = 960 + 176 + 20,
> + .htotal = 960 + 176 + 20 + 86,
> +#else
> + /* values from what is writted to LCDCTRL on vendor kernel */
> + .hdisplay = 960,
> + .hsync_start = 960 + 33,
> + .hsync_end = 960 + 33 + 20,
> + .htotal = 960 + 33 + 20 + 86,
> +#endif
> + .vdisplay = 540,
> + .vsync_start = 540 + 9,
> + .vsync_end = 540 + 9 + 3,
> + .vtotal = 540 + 9 + 3 + 12,
> + .flags = 0,
> +};
> +
> +struct bt200_panel {
> + struct drm_panel panel;
> + struct spi_device *spi;
> +};
> +
> +static int bt200_panel_write(struct bt200_panel *ctx, u8 addr, u8 data)
> +{
> + u8 val[2];
> +
> + val[0] = addr;
> + val[1] = data;
> +
> + return spi_write(ctx->spi, val, sizeof(val));
> +}
> +
> +static inline struct bt200_panel *panel_to_bt200(struct drm_panel *panel)
> +{
> + return container_of(panel, struct bt200_panel, panel);
> +}
> +
> +static int bt200_disable(struct drm_panel *panel)
> +{
> + struct bt200_panel *ctx = panel_to_bt200(panel);
> +
> + bt200_panel_write(ctx, 0x0A, 0);
> + return 0;
> +}
> +
> +static int bt200_unprepare(struct drm_panel *panel)
> +{
> + return 0;
> +}
> +
> +/* found this table in vendor kernel */
> +static struct {
> + u8 addr;
> + u8 data;
> +} lcddr_init[] = {
> + {0x01, 0x00}, {0x02, 0x00}, {0x03, 0x00}, {0x05, 0x01},
> + {0x07, 0x00}, {0x0A, 0x00}, {0x10, 0x03}, {0x11, 0x44},
> + {0x12, 0x44}, {0x13, 0x55}, {0x14, 0x03}, {0x15, 0x00},
> + {0x16, 0x2A}, {0x17, 0x20}, {0x18, 0x00}, {0x19, 0x10},
> + {0x1A, 0x12}, {0x1B, 0x0E}, {0x1C, 0x0F}, {0x1D, 0x10},
> + {0x1E, 0x0F}, {0x1F, 0x1B}, {0x20, 0x0F}, {0x21, 0x00},
> + {0x22, 0x00}, {0x23, 0x00}, {0x24, 0x00}, {0x28, 0x14},
> + {0x29, 0x19}, {0x2A, 0x17}, {0x2B, 0x2B}, {0x2C, 0x99},
> + {0x2D, 0x13}, {0x2E, 0x2A}, {0x30, 0x0B}, {0x31, 0x00},
> + {0x32, 0x00}, {0x33, 0x01}, {0x34, 0x00}, {0x35, 0x0B},
> + {0x36, 0x04}, {0x37, 0x21}, {0x38, 0x00}, {0x39, 0x46},
> + {0x3A, 0x01}, {0x3B, 0x06}, {0x3C, 0x03}, {0x3D, 0x00},
> + {0x3E, 0x06}, {0x3F, 0x04}, {0x40, 0x00}, {0x41, 0x0D},
> + {0x42, 0x00}, {0x43, 0x2E}, {0x45, 0x08}, {0x46, 0x00},
> + {0x47, 0x01}, {0x48, 0x00}, {0x49, 0x00}, {0x4A, 0x0B},
> + {0x4B, 0x38}, {0x4C, 0x03}, {0x4D, 0x04}, {0x50, 0x0F},
> + {0x51, 0x04}, {0x52, 0x01}, {0x53, 0x0E}, {0x54, 0x11},
> + {0x55, 0x9F}, {0x56, 0x36}, {0x57, 0x00}, {0x58, 0x68},
> + {0x59, 0x01}, {0x5A, 0xE0}, {0x5B, 0x00}, {0x5C, 0x00},
> + {0x5D, 0x10}, {0x5E, 0x36}, {0x5F, 0x36}, {0x60, 0x00},
> + {0x61, 0x04}, {0x62, 0x12}, {0x63, 0x00}, {0x64, 0x80},
> + {0x65, 0x00}, {0x66, 0x80}, {0x67, 0x1C}, {0x68, 0x00},
> + {0x69, 0x00}, {0x6A, 0x00}, {0x6B, 0x00}, {0x6C, 0x00},
> + {0x70, 0x00}, {0x71, 0x55}, {0x72, 0x44}, {0x73, 0x33},
> + {0x74, 0x22}, {0x75, 0x33}, {0x76, 0x22}, {0x77, 0x22},
> + {0x78, 0x33}, {0x79, 0x22}, {0x7A, 0x44}, {0x7B, 0x55},
> + {0x7C, 0x00}, {0x80, 0x00}, {0x81, 0x66}, {0x82, 0x22},
> + {0x83, 0x33}, {0x84, 0x22}, {0x85, 0x33}, {0x86, 0x22},
> + {0x87, 0x22}, {0x88, 0x33}, {0x89, 0x44}, {0x8A, 0x44},
> + {0x8B, 0x44}, {0x8C, 0x00}, {0x90, 0x00}, {0x91, 0x77},
> + {0x92, 0x66}, {0x93, 0x33}, {0x94, 0x11}, {0x95, 0x33},
> + {0x96, 0x22}, {0x97, 0x22}, {0x98, 0x44}, {0x99, 0x44},
> + {0x9A, 0x33}, {0x9B, 0x44}, {0x9C, 0x00}, {0xA0, 0x88},
> + {0xA1, 0x88}, {0xA2, 0x88}, {0xA3, 0x88}, {0xA4, 0x88},
> + {0xA5, 0x88}, {0xA6, 0x88}, {0xA7, 0x88}, {0xA8, 0x88},
> + {0xA9, 0x88}, {0xAA, 0x88}, {0xAB, 0x88}, {0xAC, 0x88},
> + {0xAD, 0x88}, {0xAE, 0x88}, {0xAF, 0x88}, {0xB0, 0x88},
> + {0xB1, 0x88}, {0xB2, 0x88}, {0xB3, 0x88}, {0xB4, 0x88},
> + {0xB5, 0x88}, {0xB6, 0x88}, {0xB7, 0x88}, {0xB8, 0x88},
> + {0xB9, 0x88}, {0xBA, 0x88}, {0xBB, 0xA6}, {0xBC, 0x88},
> + {0xBD, 0x88}, {0xBE, 0x88}, {0xBF, 0x88}, {0xC0, 0x88},
> + {0xC1, 0x88}, {0xC2, 0x88}, {0xC3, 0x88}, {0xC4, 0x88},
> + {0xC5, 0x88}, {0xC6, 0x88}, {0xC7, 0x88}, {0xC8, 0x88},
> + {0xC9, 0x88}, {0xD0, 0x36}, {0xD1, 0x26}, {0xD2, 0x21},
> + {0xD3, 0x1F}, {0xD4, 0x17}, {0xD5, 0x15}, {0xD6, 0x13},
> + {0xD7, 0x10}, {0xD8, 0x08}, {0xD9, 0x08}, {0xDA, 0x18},
> + {0xDB, 0x1D}, {0xDC, 0x1F}, {0xDD, 0x27}, {0xDE, 0x29},
> + {0xDF, 0x2B}, {0xE0, 0x2E}, {0xE1, 0x36}, {0xE6, 0x00},
> + {0xF1, 0x00}, {0xF2, 0x00}, {0xF3, 0x00}
Smells non-GPLly. Maybe ask the provider of the vendor kernel for "the
preferred form of the work for making modifications to it".
I guess {0x0A, 0x00} disables the display, so taking over an enabled
display setup by the bootloader doesn't work as is.
> +};
> +
> +static int init_lcd(struct bt200_panel *ctx)
> +{
> + int i;
> + int r;
> +
> + r = spi_setup(ctx->spi);
> + if (r < 0)
> + return r;
> +
> + for (i = 0; i < ARRAY_SIZE(lcddr_init); ++i) {
> + r = bt200_panel_write(ctx, (u16)lcddr_init[i].addr,
> + lcddr_init[i].data);
> + if (r) {
> + dev_err(ctx->panel.dev, "failed to write initial config (write) %d\n", i);
I claim that `i` isn't very interesting here, but maybe mention
lcddr_init[i].addr and/or `r` (using %pe).
> + return r;
> + }
> + }
> +
> + return 0;
> +}
> +
> +static int bt200_prepare(struct drm_panel *panel)
> +{
> + int r;
> + struct bt200_panel *ctx = panel_to_bt200(panel);
> +
> + r = init_lcd(ctx);
> + if (r)
> + return r;
> +
> + msleep(50);
> + return 0;
> +}
> +
> +static int bt200_enable(struct drm_panel *panel)
> +{
> + struct bt200_panel *ctx = panel_to_bt200(panel);
> +
> + return bt200_panel_write(ctx, 0x0A, 1);
> +}
> +
> +static int bt200_get_modes(struct drm_panel *panel, struct drm_connector *connector)
> +{
> + struct drm_display_mode *mode;
> +
> + mode = drm_mode_duplicate(connector->dev, &default_mode);
> + if (!mode)
> + return -ENOMEM;
> +
> + drm_mode_set_name(mode);
> + mode->type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED;
> + drm_mode_probed_add(connector, mode);
> +
> + return 1;
> +}
> +
> +static const struct drm_panel_funcs bt200_panel_funcs = {
> + .disable = bt200_disable,
> + .unprepare = bt200_unprepare,
> + .prepare = bt200_prepare,
> + .enable = bt200_enable,
> + .get_modes = bt200_get_modes,
> +};
> +
> +static int bt200_probe(struct spi_device *spi)
> +{
> + struct bt200_panel *ctx;
> + int ret;
> +
> + ctx = devm_drm_panel_alloc(&spi->dev, struct bt200_panel, panel,
> + &bt200_panel_funcs, DRM_MODE_CONNECTOR_DPI);
> + if (IS_ERR(ctx))
> + return PTR_ERR(ctx);
> +
> + spi_set_drvdata(spi, ctx);
> + ctx->spi = spi;
> + ctx->panel.prepare_prev_first = true;
> +
> + spi->bits_per_word = 8;
> +
> + ret = drm_panel_of_backlight(&ctx->panel);
> + if (ret)
Error message here? (Use dev_err_probe().)
> + return ret;
> +
> + drm_panel_add(&ctx->panel);
> + return 0;
> +}
> +
> +static void bt200_remove(struct spi_device *dsi)
> +{
> + struct bt200_panel *ctx = spi_get_drvdata(dsi);
> +
> + drm_panel_remove(&ctx->panel);
> +}
> +
> +static const struct of_device_id bt200_of_match[] = {
> + { .compatible = "epson,panel-bt200", },
> + {},
Please use { } here (space between curlys and no trailing comma).
> +};
> +
> +MODULE_DEVICE_TABLE(of, bt200_of_match);
> +
> +static const struct spi_device_id bt200_ids[] = {
> + { "panel-bt200", 0 },
Drop the unused 0.
> + { /* sentinel */ }
> +};
> +
> +MODULE_DEVICE_TABLE(spi, bt200_ids);
> +
> +static struct spi_driver bt200_driver = {
> + .probe = bt200_probe,
> + .remove = bt200_remove,
> + .id_table = bt200_ids,
> + .driver = {
> + .name = "panel-bt200",
> + .of_match_table = bt200_of_match,
> + .suppress_bind_attrs = true,
Why suppress bind attrs?
> + },
> +};
> +
> +module_spi_driver(bt200_driver);
> +
> +MODULE_AUTHOR("Andreas Kemnade <andreas@xxxxxxxxxxxx>");
> +MODULE_DESCRIPTION("bt200 panel driver");
> +MODULE_LICENSE("GPL");
>
> --
> 2.47.3
>
Attachment:
signature.asc
Description: PGP signature