Re: [PATCH v5 0/3] drm/panel: Support Rocktech jh057n00900 DSI panel

[Joe Perches <joe@perches.com>]

On Wed, 2019-04-03 at 10:11 -0700, Joe Perches wrote:
> On Wed, 2019-04-03 at 18:17 +0200, Thierry Reding wrote:
> > On Mon, Apr 01, 2019 at 12:35:32PM +0200, Guido Günther wrote:
> > > v4 fixes up the DT binding example and uses a wider cc list since I
> > > failed to extend that when touching more files.
> []
> > Applied, thanks.
> > 
> > checkpatch does complain about the dsi_generic_write_seq() macro
> > definition, because it uses flow control statements, but there are
> > already similar macros in other drivers, so I let this slide. We may
> > want to eventually come up with something better and then replace these
> > macros for the other drivers as well.
> 
> Dunno about the other drivers, but the mechanism isn't
> particularly nice as it separates the init identifier
> from the data being written.
> 
> It might be better to use something like a struct for
> each command and a for loop for each block of commands.
> 
> Also the 0xBF value used in one of the init sequence
> writes does not have an identifier #define in the
> 'Manufacturer specific Commands send via DSI' block
> which is odd.

Perhaps something like this below
(though adding a bunch of lines to avoid a macro goto isn't great)
It does seem to read a bit better to me though.
---
 drivers/gpu/drm/panel/panel-rocktech-jh057n00900.c | 210 +++++++++++++--------
 1 file changed, 136 insertions(+), 74 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-rocktech-jh057n00900.c b/drivers/gpu/drm/panel/panel-rocktech-jh057n00900.c
index 158a6d548068..7862863db5f7 100644
--- a/drivers/gpu/drm/panel/panel-rocktech-jh057n00900.c
+++ b/drivers/gpu/drm/panel/panel-rocktech-jh057n00900.c
@@ -20,27 +20,6 @@
 
 #define DRV_NAME "panel-rocktech-jh057n00900"
 
-/* Manufacturer specific Commands send via DSI */
-#define ST7703_CMD_ALL_PIXEL_OFF 0x22
-#define ST7703_CMD_ALL_PIXEL_ON	 0x23
-#define ST7703_CMD_SETDISP	 0xB2
-#define ST7703_CMD_SETRGBIF	 0xB3
-#define ST7703_CMD_SETCYC	 0xB4
-#define ST7703_CMD_SETBGP	 0xB5
-#define ST7703_CMD_SETVCOM	 0xB6
-#define ST7703_CMD_SETOTP	 0xB7
-#define ST7703_CMD_SETPOWER_EXT	 0xB8
-#define ST7703_CMD_SETEXTC	 0xB9
-#define ST7703_CMD_SETMIPI	 0xBA
-#define ST7703_CMD_SETVDC	 0xBC
-#define ST7703_CMD_SETSCR	 0xC0
-#define ST7703_CMD_SETPOWER	 0xC1
-#define ST7703_CMD_SETPANEL	 0xCC
-#define ST7703_CMD_SETGAMMA	 0xE0
-#define ST7703_CMD_SETEQ	 0xE3
-#define ST7703_CMD_SETGIP1	 0xE9
-#define ST7703_CMD_SETGIP2	 0xEA
-
 struct jh057n {
 	struct device *dev;
 	struct drm_panel panel;
@@ -51,75 +30,153 @@ struct jh057n {
 	struct dentry *debugfs;
 };
 
+struct st7703_cmd {
+	const size_t	size;
+	const u8	data[];
+};
+
+#define st7703_cmd_data(cmd, ...)					\
+	.size = 1 + (sizeof((u8[]){__VA_ARGS__})/sizeof(u8)),		\
+	.data = {cmd, __VA_ARGS__}
+
+/* Manufacturer specific Commands send via DSI */
+static const struct st7703_cmd ST7703_CMD_ALL_PIXEL_OFF = {
+	st7703_cmd_data(0x22)
+};
+static const struct st7703_cmd ST7703_CMD_ALL_PIXEL_ON = {
+	st7703_cmd_data(0x23)
+};
+static const struct st7703_cmd ST7703_CMD_SETDISP = {
+	st7703_cmd_data(0xB2,
+			0xF0, 0x12, 0x30)
+};
+static const struct st7703_cmd ST7703_CMD_SETRGBIF = {
+	st7703_cmd_data(0xB3,
+			0x10, 0x10, 0x05, 0x05, 0x03, 0xFF, 0x00, 0x00,
+			0x00, 0x00)
+};
+static const struct st7703_cmd ST7703_CMD_SETCYC = {
+	st7703_cmd_data(0xB4,
+			0x80)
+};
+static const struct st7703_cmd ST7703_CMD_SETBGP = {
+	st7703_cmd_data(0xB5,
+			0x08, 0x08)
+};
+static const struct st7703_cmd ST7703_CMD_SETVCOM = {
+	st7703_cmd_data(0xB6,
+			0x3F, 0x3F)
+};
+static const struct st7703_cmd ST7703_CMD_SETOTP = {
+	st7703_cmd_data(0xB7)
+};
+static const struct st7703_cmd ST7703_CMD_SETPOWER_EXT = {
+	st7703_cmd_data(0xB8)
+};
+static const struct st7703_cmd ST7703_CMD_SETEXTC = {
+	st7703_cmd_data(0xB9,
+			0xF1, 0x12, 0x83)
+};
+static const struct st7703_cmd ST7703_CMD_SETMIPI = {
+	st7703_cmd_data(0xBA)
+};
+static const struct st7703_cmd ST7703_CMD_SETVDC = {
+	st7703_cmd_data(0xBC,
+			0x4E)
+};
+static const struct st7703_cmd ST7703_CMD_SETSCR = {
+	st7703_cmd_data(0xC0,
+			0x73, 0x73, 0x50, 0x50, 0x00, 0x00, 0x08, 0x70,
+			0x00)
+};
+static const struct st7703_cmd ST7703_CMD_SETPOWER = {
+	st7703_cmd_data(0xC1)
+};
+static const struct st7703_cmd ST7703_CMD_SETPANEL = {
+	st7703_cmd_data(0xCC,
+			0x0B)
+};
+static const struct st7703_cmd ST7703_CMD_SETGAMMA = {
+	st7703_cmd_data(0xE0,
+			0x00, 0x09, 0x0E, 0x29, 0x2D, 0x3C, 0x41, 0x37,
+			0x07, 0x0B, 0x0D, 0x10, 0x11, 0x0F, 0x10, 0x11,
+			0x18, 0x00, 0x09, 0x0E, 0x29, 0x2D, 0x3C, 0x41,
+			0x37, 0x07, 0x0B, 0x0D, 0x10, 0x11, 0x0F, 0x10,
+			0x11, 0x18)
+};
+static const struct st7703_cmd ST7703_CMD_SETEQ = {
+	st7703_cmd_data(0xE3,
+			0x07, 0x07, 0x0B, 0x0B, 0x03, 0x0B, 0x00, 0x00,
+			0x00, 0x00, 0xFF, 0x00, 0xC0, 0x10)
+};
+static const struct st7703_cmd ST7703_CMD_SETGIP1 = {
+	st7703_cmd_data(0xE9,
+			0x82, 0x10, 0x06, 0x05, 0x9E, 0x0A, 0xA5, 0x12,
+			0x31, 0x23, 0x37, 0x83, 0x04, 0xBC, 0x27, 0x38,
+			0x0C, 0x00, 0x03, 0x00, 0x00, 0x00, 0x0C, 0x00,
+			0x03, 0x00, 0x00, 0x00, 0x75, 0x75, 0x31, 0x88,
+			0x88, 0x88, 0x88, 0x88, 0x88, 0x13, 0x88, 0x64,
+			0x64, 0x20, 0x88, 0x88, 0x88, 0x88, 0x88, 0x88,
+			0x02, 0x88, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+			0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00)
+};
+
+static const struct st7703_cmd ST7703_CMD_SETGIP2 = {
+	st7703_cmd_data(0xEA,
+			0x02, 0x21, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+			0x00, 0x00, 0x00, 0x00, 0x02, 0x46, 0x02, 0x88,
+			0x88, 0x88, 0x88, 0x88, 0x88, 0x64, 0x88, 0x13,
+			0x57, 0x13, 0x88, 0x88, 0x88, 0x88, 0x88, 0x88,
+			0x75, 0x88, 0x23, 0x14, 0x00, 0x00, 0x02, 0x00,
+			0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+			0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x30, 0x0A,
+			0xA5, 0x00, 0x00, 0x00, 0x00)
+};
+
 static inline struct jh057n *panel_to_jh057n(struct drm_panel *panel)
 {
 	return container_of(panel, struct jh057n, panel);
 }
 
-#define dsi_generic_write_seq(dsi, seq...) do {				\
-		static const u8 d[] = { seq };				\
-		int ret;						\
-		ret = mipi_dsi_generic_write(dsi, d, ARRAY_SIZE(d));	\
-		if (ret < 0)						\
-			return ret;					\
-	} while (0)
-
 static int jh057n_init_sequence(struct jh057n *ctx)
 {
 	struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev);
 	struct device *dev = ctx->dev;
+	int i;
 	int ret;
+	static const struct {
+		const struct st7703_cmd *cmd;
+		int msleep;
+	} st7703_init[] = {
+		{&ST7703_CMD_SETEXTC, 0},
+		{&ST7703_CMD_SETRGBIF, 0},
+		{&ST7703_CMD_SETSCR, 0},
+		{&ST7703_CMD_SETVDC, 0},
+		{&ST7703_CMD_SETPANEL, 0},
+		{&ST7703_CMD_SETCYC, 0},
+		{&ST7703_CMD_SETDISP, 0},
+		{&ST7703_CMD_SETEQ, 0},
+		{&ST7703_CMD_SETBGP, 20},
+		{&ST7703_CMD_SETVCOM, 0},
+		{&ST7703_CMD_SETGIP1, 0},
+		{&ST7703_CMD_SETGIP2, 0},
+		{&ST7703_CMD_SETGAMMA, 20},
+};
 
 	/*
 	 * Init sequence was supplied by the panel vendor. Most of the commands
 	 * resemble the ST7703 but the number of parameters often don't match
 	 * so it's likely a clone.
 	 */
-	dsi_generic_write_seq(dsi, ST7703_CMD_SETEXTC,
-			      0xF1, 0x12, 0x83);
-	dsi_generic_write_seq(dsi, ST7703_CMD_SETRGBIF,
-			      0x10, 0x10, 0x05, 0x05, 0x03, 0xFF, 0x00, 0x00,
-			      0x00, 0x00);
-	dsi_generic_write_seq(dsi, ST7703_CMD_SETSCR,
-			      0x73, 0x73, 0x50, 0x50, 0x00, 0x00, 0x08, 0x70,
-			      0x00);
-	dsi_generic_write_seq(dsi, ST7703_CMD_SETVDC, 0x4E);
-	dsi_generic_write_seq(dsi, ST7703_CMD_SETPANEL, 0x0B);
-	dsi_generic_write_seq(dsi, ST7703_CMD_SETCYC, 0x80);
-	dsi_generic_write_seq(dsi, ST7703_CMD_SETDISP, 0xF0, 0x12, 0x30);
-	dsi_generic_write_seq(dsi, ST7703_CMD_SETEQ,
-			      0x07, 0x07, 0x0B, 0x0B, 0x03, 0x0B, 0x00, 0x00,
-			      0x00, 0x00, 0xFF, 0x00, 0xC0, 0x10);
-	dsi_generic_write_seq(dsi, ST7703_CMD_SETBGP, 0x08, 0x08);
-	msleep(20);
 
-	dsi_generic_write_seq(dsi, ST7703_CMD_SETVCOM, 0x3F, 0x3F);
-	dsi_generic_write_seq(dsi, 0xBF, 0x02, 0x11, 0x00);
-	dsi_generic_write_seq(dsi, ST7703_CMD_SETGIP1,
-			      0x82, 0x10, 0x06, 0x05, 0x9E, 0x0A, 0xA5, 0x12,
-			      0x31, 0x23, 0x37, 0x83, 0x04, 0xBC, 0x27, 0x38,
-			      0x0C, 0x00, 0x03, 0x00, 0x00, 0x00, 0x0C, 0x00,
-			      0x03, 0x00, 0x00, 0x00, 0x75, 0x75, 0x31, 0x88,
-			      0x88, 0x88, 0x88, 0x88, 0x88, 0x13, 0x88, 0x64,
-			      0x64, 0x20, 0x88, 0x88, 0x88, 0x88, 0x88, 0x88,
-			      0x02, 0x88, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
-			      0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00);
-	dsi_generic_write_seq(dsi, ST7703_CMD_SETGIP2,
-			      0x02, 0x21, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
-			      0x00, 0x00, 0x00, 0x00, 0x02, 0x46, 0x02, 0x88,
-			      0x88, 0x88, 0x88, 0x88, 0x88, 0x64, 0x88, 0x13,
-			      0x57, 0x13, 0x88, 0x88, 0x88, 0x88, 0x88, 0x88,
-			      0x75, 0x88, 0x23, 0x14, 0x00, 0x00, 0x02, 0x00,
-			      0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
-			      0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x30, 0x0A,
-			      0xA5, 0x00, 0x00, 0x00, 0x00);
-	dsi_generic_write_seq(dsi, ST7703_CMD_SETGAMMA,
-			      0x00, 0x09, 0x0E, 0x29, 0x2D, 0x3C, 0x41, 0x37,
-			      0x07, 0x0B, 0x0D, 0x10, 0x11, 0x0F, 0x10, 0x11,
-			      0x18, 0x00, 0x09, 0x0E, 0x29, 0x2D, 0x3C, 0x41,
-			      0x37, 0x07, 0x0B, 0x0D, 0x10, 0x11, 0x0F, 0x10,
-			      0x11, 0x18);
-	msleep(20);
+	for (i = 0; i < ARRAY_SIZE(st7703_init); i++) {
+		ret = mipi_dsi_generic_write(dsi, st7703_init[i].cmd->data,
+					     st7703_init[i].cmd->size);
+		if (ret < 0)
+			return ret;
+		if (st7703_init[i].msleep)
+			msleep(st7703_init[i].msleep);
+	}
 
 	ret = mipi_dsi_dcs_exit_sleep_mode(dsi);
 	if (ret < 0) {
@@ -240,9 +297,14 @@ static int allpixelson_set(void *data, u64 val)
 {
 	struct jh057n *ctx = data;
 	struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev);
+	int ret;
 
 	DRM_DEV_DEBUG_DRIVER(ctx->dev, "Setting all pixels on");
-	dsi_generic_write_seq(dsi, ST7703_CMD_ALL_PIXEL_ON);
+	ret = mipi_dsi_generic_write(dsi, ST7703_CMD_ALL_PIXEL_ON.data,
+				     ST7703_CMD_ALL_PIXEL_ON.size);
+	if (ret < 0)							\
+		return ret;						\
+
 	msleep(val * 1000);
 	/* Reset the panel to get video back */
 	drm_panel_disable(&ctx->panel);