Re: [PATCH 2/2] drm/panel: Add driver for sitronix ST7789V panel

On Fri, Feb 03, 2017 at 10:59:06AM +0100, Maxime Ripard wrote:

"Sitronix"

+	depends on OF && SPI
+

Maybe be more verbose here about what kind of panel it is, and what
resolution it provides.

I prefer linux/* headers to go first.

+
+#define ST7789V_SLPIN_CMD		0x10
+
+#define ST7789V_SLPOUT_CMD		0x11
+
+#define ST7789V_INVON_CMD		0x21
+
+#define ST7789V_DISPOFF_CMD		0x28
+
+#define ST7789V_DISPON_CMD		0x29
+
+#define ST7789V_MADCTL_CMD		0x36
+
+#define ST7789V_COLMOD_CMD		0x3a
+#define ST7789V_COLMOD_RGB_FMT_18BITS		(6 << 4)
+#define ST7789V_COLMOD_CTRL_FMT_18BITS		(6 << 0)

MIPI_DCS_PIXEL_FMT_18BIT?

[...]

+struct st7789v {
+	struct drm_panel panel;
+	struct spi_device *spi;
+	struct gpio_desc *reset;
+	struct backlight_device *backlight;
+};
+
+enum st7789v_prefix {
+	ST7789V_COMMAND = 0,
+	ST7789V_DATA = 1,
+};
+
+static inline struct st7789v *panel_to_st7789v(struct drm_panel *panel)
+{
+	return container_of(panel, struct st7789v, panel);
+}
+
+static int st7789v_spi_write(struct st7789v *ctx, u8 prefix, u8 data)

Why aren't you using the proper type here for prefix?

+{
+	struct spi_transfer xfer = { };
+	struct spi_message msg;
+	u16 txbuf = ((prefix & 1) << 8) | data;
+
+	spi_message_init(&msg);
+
+	xfer.tx_buf = &txbuf;
+	xfer.bits_per_word = 9;
+	xfer.len = sizeof(txbuf);
+
+	spi_message_add_tail(&xfer, &msg);
+	return spi_sync(ctx->spi, &msg);
+}
+
+static int st7789v_write_command(struct st7789v *ctx, u8 cmd)
+{
+	return st7789v_spi_write(ctx, ST7789V_COMMAND, cmd);
+}
+
+static int st7789v_write_data(struct st7789v *ctx, u8 cmd)
+{
+	return st7789v_spi_write(ctx, ST7789V_DATA, cmd);
+}
+
+static int st7789v_write_command_data(struct st7789v *ctx, u8 cmd,
+				      unsigned long n_data, ...)
+{
+	va_list ap;
+	int i;
+
+	st7789v_write_command(ctx, cmd);
+
+	va_start(ap, n_data);
+	for (i = 0; i < n_data; i++)
+		st7789v_write_data(ctx, va_arg(ap, int));
+	va_end(ap);
+
+	return 0;
+}

Please propagate error codes from st7789v_write_{command,data}(). And
you should abort early on errors.

+#define NUMARGS(...)  (sizeof((int[]){__VA_ARGS__}) / sizeof(int))
+#define st7789v_send(ctx, cmd, ...)					\
+	st7789v_write_command_data(ctx, cmd, NUMARGS(__VA_ARGS__),	\
+				   ##__VA_ARGS__)

How is this going to work if any of the arguments happens to not be an
int? What if you have something like this:

	u8 value = 0x2;

	st7789v_write_command_data(ctx, cmd, 0x1, value, 0x3);

? Wouldn't that invalidly read "value" as int and wrongly increment the
ap by three bytes too many?

+static int st7789v_prepare(struct drm_panel *panel)
+{
+	struct st7789v *ctx = panel_to_st7789v(panel);
+
+	gpiod_set_value(ctx->reset, 1);
+	msleep(30);
+	gpiod_set_value(ctx->reset, 0);
+	msleep(120);
+
+	st7789v_send(ctx, ST7789V_SLPOUT_CMD);
+
+	/* We need to wait 120ms after a sleep out command */
+	msleep(120);
+
+	st7789v_send(ctx, ST7789V_MADCTL_CMD, 0);
+
+	st7789v_send(ctx, ST7789V_COLMOD_CMD,
+		     ST7789V_COLMOD_CTRL_FMT_18BITS |
+		     ST7789V_COLMOD_RGB_FMT_18BITS);
+
+	st7789v_send(ctx, ST7789V_PORCTRL_CMD,
+		     0xc,		/* Backporch, normal mode*/
+		     0xc,		/* Frontporch, normal mode */
+		     0,			/* Disable separate porch control */
+		     ST7789V_PORCTRL_IDLE_BP(3) |
+		     ST7789V_PORCTRL_IDLE_FP(3),
+		     ST7789V_PORCTRL_PARTIAL_BP(3) |
+		     ST7789V_PORCTRL_PARTIAL_FP(3));
+
+	st7789v_send(ctx, ST7789V_GCTRL_CMD,
+		     ST7789V_GCTRL_VGLS(5) | ST7789V_GCTRL_VGHS(3));
+
+	st7789v_send(ctx, ST7789V_VCOMS_CMD, 0x2b);
+
+	st7789v_send(ctx, ST7789V_LCMCTRL_CMD, ST7789V_LCMCTRL_XMH |
+		     ST7789V_LCMCTRL_XMX | ST7789V_LCMCTRL_XBGR);
+
+	st7789v_send(ctx, ST7789V_VDVVRHEN_CMD, ST7789V_VDVVRHEN_CMDEN);
+
+	st7789v_send(ctx, ST7789V_VRHS_CMD, 0xf);
+
+	st7789v_send(ctx, ST7789V_VDVS_CMD, 0x20);
+
+	st7789v_send(ctx, ST7789V_FRCTRL2_CMD, 0xf);
+
+	st7789v_send(ctx, ST7789V_PWCTRL1_CMD, ST7789V_PWCTRL1_MAGIC,
+		     ST7789V_PWCTRL1_AVDD(2) | ST7789V_PWCTRL1_AVCL(2) |
+		     ST7789V_PWCTRL1_VDS(1));
+
+	st7789v_send(ctx, ST7789V_PVGAMCTRL_CMD,
+		     ST7789V_PVGAMCTRL_VP63(0xd),
+		     ST7789V_PVGAMCTRL_VP1(0xca),
+		     ST7789V_PVGAMCTRL_VP2(0xe),
+		     ST7789V_PVGAMCTRL_VP4(8),
+		     ST7789V_PVGAMCTRL_VP6(9),
+		     ST7789V_PVGAMCTRL_VP13(7),
+		     ST7789V_PVGAMCTRL_VP20(0x2d),
+		     ST7789V_PVGAMCTRL_VP27(0xb) | ST7789V_PVGAMCTRL_VP36(3),
+		     ST7789V_PVGAMCTRL_VP43(0x3d),
+		     ST7789V_PVGAMCTRL_JP1(3) | ST7789V_PVGAMCTRL_VP50(4),
+		     ST7789V_PVGAMCTRL_VP57(0xa),
+		     ST7789V_PVGAMCTRL_VP59(0xa),
+		     ST7789V_PVGAMCTRL_VP61(0x1b),
+		     ST7789V_PVGAMCTRL_VP62(0x28));
+
+	st7789v_send(ctx, ST7789V_NVGAMCTRL_CMD,
+		     ST7789V_NVGAMCTRL_VN63(0xd),
+		     ST7789V_NVGAMCTRL_VN1(0xca),
+		     ST7789V_NVGAMCTRL_VN2(0xf),
+		     ST7789V_NVGAMCTRL_VN4(8),
+		     ST7789V_NVGAMCTRL_VN6(8),
+		     ST7789V_NVGAMCTRL_VN13(7),
+		     ST7789V_NVGAMCTRL_VN20(0x2e),
+		     ST7789V_NVGAMCTRL_VN27(0xc) | ST7789V_NVGAMCTRL_VN36(5),
+		     ST7789V_NVGAMCTRL_VN43(0x40),
+		     ST7789V_NVGAMCTRL_JN1(3) | ST7789V_NVGAMCTRL_VN50(4),
+		     ST7789V_NVGAMCTRL_VN57(9),
+		     ST7789V_NVGAMCTRL_VN59(0xb),
+		     ST7789V_NVGAMCTRL_VN61(0x1b),
+		     ST7789V_NVGAMCTRL_VN62(0x28));
+
+	st7789v_send(ctx, ST7789V_INVON_CMD);
+
+	st7789v_send(ctx, ST7789V_RAMCTRL_CMD,
+		     ST7789V_RAMCTRL_DM_RGB | ST7789V_RAMCTRL_RM_RGB,
+		     ST7789V_RAMCTRL_EPF(3) | ST7789V_RAMCTRL_MAGIC);
+
+	st7789v_send(ctx, ST7789V_RGBCTRL_CMD,
+		     ST7789V_RGBCTRL_WO | ST7789V_RGBCTRL_RCM(2) |
+		     ST7789V_RGBCTRL_VSYNC_HIGH | ST7789V_RGBCTRL_HSYNC_HIGH |
+		     ST7789V_RGBCTRL_PCLK_HIGH,
+		     ST7789V_RGBCTRL_VBP(8), ST7789V_RGBCTRL_HBP(20));
+
+	return 0;
+}

Please check for errors in all of the above.

+static int st7789v_probe(struct spi_device *spi)
+{
+	struct device_node *backlight;
+	struct st7789v *ctx;
+	int ret;
+
+	ctx = devm_kzalloc(&spi->dev, sizeof(*ctx), GFP_KERNEL);
+	if (!ctx)
+		return -ENOMEM;
+
+	spi_set_drvdata(spi, ctx);
+	ctx->spi = spi;
+
+	ctx->panel.dev = &spi->dev;
+	ctx->panel.funcs = &st7789v_drm_funcs;
+
+	ctx->reset = devm_gpiod_get(&spi->dev, "reset", GPIOD_OUT_LOW);
+	if (IS_ERR(ctx->reset)) {
+		dev_err(&spi->dev, "Couldn't get our reset line\n");
+		return PTR_ERR(ctx->reset);
+	}
+
+	backlight = of_parse_phandle(spi->dev.of_node, "backlight", 0);
+	if (backlight) {
+		ctx->backlight = of_find_backlight_by_node(backlight);
+		of_node_put(backlight);
+
+		if (!ctx->backlight)
+			return -EPROBE_DEFER;
+	}

This could make use of one of the helpers that Noralf had sent earlier
for TinyDRM. That is, if it weren't specific to TinyDRM.

Thierry