Re: [PATCH RFC 2/9] drm: panel: Add panel for Epson Moverio BT200

["Uwe Kleine-König" <u.kleine-koenig@baylibre.com>]

[-- Attachment #1: Type: text/plain, Size: 9108 bytes --]

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@kemnade.info>");
> +MODULE_DESCRIPTION("bt200 panel driver");
> +MODULE_LICENSE("GPL");
> 
> -- 
> 2.47.3
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]