Extend panel-mipi-dbi driver with variants or tweaks to support more panels?

[johado]

This is more a discussions about the panel-mipi-dbi.c driver you made, hope it's ok to start the discussion here before posting to kernel mailing list.
I have come across a couple of small display with various driver IC:s that doesn't have any Linux driver and they are not exactly register compatible with the MIPI_DCS_SET_COLUMN_ADDRESS, MIPI_DCS_SET_PAGE_ADDRESS and MIPI_DCS_WRITE_MEMORY_START and MIPI_DCS_SOFT_RESET, which seems to be all that needs to be handled by the panel driver tweaks.
So the cmd registers are different and some uses only 1 byte per xs, xe ys, ye, some tiny LCD displays actually combine the xs into the cmd byte, and requires new cmd for each line.
In some cases the framebuffer data needs modification from rgb565 as well, some are 4 bit grayscale with 2 pixels/byte, some are monochrome with weird 3 pixels/byte as the st7586.c driver in the kernel. Others needs data sent in 8 rows/byte.

Would it be a good idea to extend the panel-mipi-dbi to handle these variants? Either by a new "variant" or similar attribute in the devicetree or a 3rd optional parameter in the "compatible" that select a built in variant.
Or perhaps additional fields in devicetree, e.g "set_column_cmd", "set_page_cmd" "byte_per_column" etc. as well as "frame-buffer-format" to make it truly generic.
The alternative would be to make another custom driver, but I really like to reuse the firmware support for init sequence as in panel-mipi-dbi. Should /could that code be moved to drm_mipi_dbi.c so more drivers could use it?

Example of code snippet to handle an ST75156 based display, the framebuffer conversion should be moved to a separate function I guess.
Do you think I should clean up and send a proper patch for panel-mipi-dbi or go for a new driver?

`
static int mipi_dbi_spi_command_st75156(struct mipi_dbi *dbi, u8 *cmd,
u8 parameters, size_t num)
{
/ Each byte of data is 8 vertical pixels, not horisontal. Max 256x160 */
struct mipi_dbi_dev *dbidev = container_of(dbi, struct mipi_dbi_dev, dbi);
struct panel_mipi_dbi_commands *priv = dbidev->driver_private;
int ret = 0;

//MIPI_DBI_DEBUG_COMMAND(*cmd, parameters, num);
switch (*cmd) {
case MIPI_DCS_SET_COLUMN_ADDRESS: {
	*cmd = 0x15;
	parameters[0] = priv->xs & 0xFF;
	parameters[1] = priv->xe & 0xFF;
	priv->command(dbi, cmd, parameters, 2);
}
break;
case MIPI_DCS_SET_PAGE_ADDRESS: {
	/* 0 <= ys <=0x13, ys <= ye <= 0x13 (19), each page is 8 rows */
	*cmd = 0x75;
	parameters[0] = (priv->ys/8) & 0x1F; /* 8 rows/page */
	parameters[1] = (priv->ye/8) & 0x1F; /* 8 rows/page */
	priv->command(dbi, cmd, parameters, 2);
}
break;
case MIPI_DCS_WRITE_MEMORY_START: {
	/* One byte of data to display gives a 8 pixel column
	 * MSB is highest row
	 * a123456  ->  hgfedcbaHGFEDCB1......
	 * bBcdef
	 * cCCDEF
	 * dD
	 * eE
	 * fF
	 * gG
	 * hH
	 */
	u8 fb[256] = {0};
	u16 offs = 0;
	u8 *src = parameters;
	//u8 *endbuf = parameters + num;
	u16 linelen = priv->xe - priv->xs + 1;
	u16 ys = 0;
	u16 x;
	u16 y;

	for (ys = priv->ys; ys <= priv->ye; ys += 8) {
		*cmd = 0x5C; /* Write data */
		ret = priv->command(dbi, cmd, NULL, 0);
		offs = 0;
		for (x = 0; x < linelen; x++) {
			u8 px0 = 0;
			u8 mask = 0x01;

			for (y = ys; y < ys+8; y++) {
				/* Assume 16 bit pixels with LSB first */
				u8 *buf = &src[x*2+y*2*linelen];
				u16 rgb565 = buf[0] | (buf[1] << 8);
				u8 px = rgb565_to_4bit_greyscale(rgb565);

				if (px >= 0x01)
					px0 |= mask;
				mask <<= 1;
			}
			fb[offs] = px0;
			offs++;
		}
		ret = mipi_dbi_spi_raw_data(dbi, fb, offs);
	}
}
break;
case MIPI_DCS_SOFT_RESET: {
	*cmd = 0xAE; /* No RESET cmd, use Display OFF? */
	ret = priv->command(dbi, cmd, NULL, 0);
}
break;
default:
	break;
}
return ret;

}

static int mipi_dbi_spi_command(struct mipi_dbi *dbi, u8 *cmd,
u8 *parameters, size_t num)
{
//unsigned int bpw = (*cmd == MIPI_DCS_WRITE_MEMORY_START) ? 16 : 8;
int ret = 0;
struct mipi_dbi_dev *dbidev = container_of(dbi, struct mipi_dbi_dev, dbi);
struct panel_mipi_dbi_commands *priv = dbidev->driver_private;

switch (*cmd) {
case MIPI_DCS_SET_COLUMN_ADDRESS:
	priv->xs = (parameters[0] << 8) | parameters[1];
	priv->xe = (parameters[2] << 8) | parameters[3];
	break;
case MIPI_DCS_SET_PAGE_ADDRESS:
	priv->ys = (parameters[0] << 8) | parameters[1];
	priv->ye = (parameters[2] << 8) | parameters[3];
	break;
default:
	break;
}

switch (priv->devid) {
case DEVID_DBI:
	return priv->command(dbi, cmd, parameters, num);
case DEVID_SH1122:
case DEVID_SH1122mono:
	return mipi_dbi_spi_command_sh1122(dbi, cmd, parameters, num, priv->devid);
case DEVID_ST7567:
	return mipi_dbi_spi_command_st7567(dbi, cmd, parameters, num);
case DEVID_ST75156:
	return mipi_dbi_spi_command_st75156(dbi, cmd, parameters, num);
case DEVID_ST7586S_4bit:
case DEVID_ST7586S_mono:
		return mipi_dbi_spi_command_st7586s(dbi, cmd, parameters, num);
case DEVID_LAST:
	break;
/* No default to catch new enums */
}

return ret;

}

@@ -203,8 +250,11 @@ static void panel_mipi_dbi_enable(struct drm_simple_display_pipe *pipe,
ret = mipi_dbi_poweron_conditional_reset(dbidev);
if (ret < 0)
goto out_exit;
- if (!ret)
+ if (!ret) {
+ dbi->command = priv->command;
panel_mipi_dbi_commands_execute(dbi, dbidev->driver_private);
+ dbi->command = mipi_dbi_spi_command;
+ }

@ -318,6 +862,10 @@ static int panel_mipi_dbi_spi_probe(struct spi_device *spi)
dbidev->driver_private = panel_mipi_dbi_commands_from_fw(dev);
if (IS_ERR(dbidev->driver_private))
return PTR_ERR(dbidev->driver_private);
+ commands = dbidev->driver_private;
+ commands->command = dbi->command;
+ dbi->command = mipi_dbi_spi_command;

`

[notro]

panel-mipi-dbi is only for panels that support the MIPI DBI standard, it's not meant to go beyond that. This probably means a new driver. I'm not doing any kernel development for the time being, not sure if will return to it apart from the occasional patch, so it's best you ask on the mailinglist. But I don't think you should base your driver on drm_mipi_dbi, I think it's better to write a new one akin to drivers/gpu/drm/solomon. The drivers in tiny/ that use drm_mipi_dbi but are not MIPI DBI compatible, would most likely not have been accepted in that form today.

[johado]

Thank you for your quick response!
I'll investigate the driver you suggested and reach out to the kernel mailinglist.

[johado]

I investigated the solomon driver a bit, but since that uses regmap abstraction it doesn't seem fit to handle controllers that want 1 byte commands followed by 1 or 2 data bytes.
(Also we need it on Linux 5.15 at the moment and a lot of things has changed so the backport was not trivial)

[notro]

I had forgotten about it, but there's tiny/repaper as well. It's for an e-ink display, but it's a full SPI driver that doesn't use the mipi-dbi library. There is one problem with it and that is its use of drm_simple_display_pipe, I think that abstraction is deprecated now.