[PATCH 2/2] drm/panel: s6e63m0: Switch to DBI abstraction for SPI

[PATCH 2/2] drm/panel: s6e63m0: Switch to DBI abstraction for SPI

[Linus Walleij]

The SPI access to s6e63m0 is using the DBI protocol, so switch
to using the elaborate DBI protocol implementation in the DRM
DBI helper library.

Cc: Douglas Anderson <dianders@chromium.org>
Cc: Noralf Trønnes <noralf@tronnes.org>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/gpu/drm/panel/Kconfig                 |  1 +
 .../gpu/drm/panel/panel-samsung-s6e63m0-dsi.c | 10 ++-
 .../gpu/drm/panel/panel-samsung-s6e63m0-spi.c | 85 ++++++++-----------
 drivers/gpu/drm/panel/panel-samsung-s6e63m0.c | 41 ++-------
 drivers/gpu/drm/panel/panel-samsung-s6e63m0.h | 33 ++++++-
 5 files changed, 81 insertions(+), 89 deletions(-)

diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
index 4894913936e9..324b932ac213 100644
--- a/drivers/gpu/drm/panel/Kconfig
+++ b/drivers/gpu/drm/panel/Kconfig
@@ -376,6 +376,7 @@ config DRM_PANEL_SAMSUNG_S6E63M0_SPI
 	depends on SPI
 	depends on DRM_PANEL_SAMSUNG_S6E63M0
 	default DRM_PANEL_SAMSUNG_S6E63M0
+	select DRM_MIPI_DBI
 	help
 	  Say Y here if you want to be able to access the Samsung
 	  S6E63M0 panel using SPI.
diff --git a/drivers/gpu/drm/panel/panel-samsung-s6e63m0-dsi.c b/drivers/gpu/drm/panel/panel-samsung-s6e63m0-dsi.c
index 07a48f621289..e0b1a7e354f3 100644
--- a/drivers/gpu/drm/panel/panel-samsung-s6e63m0-dsi.c
+++ b/drivers/gpu/drm/panel/panel-samsung-s6e63m0-dsi.c
@@ -16,7 +16,8 @@
 #define MCS_GLOBAL_PARAM	0xb0
 #define S6E63M0_DSI_MAX_CHUNK	15 /* CMD + 15 bytes max */
 
-static int s6e63m0_dsi_dcs_read(struct device *dev, const u8 cmd, u8 *data)
+static int s6e63m0_dsi_dcs_read(struct device *dev, void *trsp,
+				const u8 cmd, u8 *data)
 {
 	struct mipi_dsi_device *dsi = to_mipi_dsi_device(dev);
 	int ret;
@@ -32,7 +33,8 @@ static int s6e63m0_dsi_dcs_read(struct device *dev, const u8 cmd, u8 *data)
 	return 0;
 }
 
-static int s6e63m0_dsi_dcs_write(struct device *dev, const u8 *data, size_t len)
+static int s6e63m0_dsi_dcs_write(struct device *dev, void *trsp,
+				 const u8 *data, size_t len)
 {
 	struct mipi_dsi_device *dsi = to_mipi_dsi_device(dev);
 	const u8 *seqp = data;
@@ -99,8 +101,8 @@ static int s6e63m0_dsi_probe(struct mipi_dsi_device *dsi)
 	dsi->mode_flags = MIPI_DSI_MODE_VIDEO |
 		MIPI_DSI_MODE_VIDEO_BURST;
 
-	ret = s6e63m0_probe(dev, s6e63m0_dsi_dcs_read, s6e63m0_dsi_dcs_write,
-			    true);
+	ret = s6e63m0_probe(dev, NULL, s6e63m0_dsi_dcs_read,
+			    s6e63m0_dsi_dcs_write, true);
 	if (ret)
 		return ret;
 
diff --git a/drivers/gpu/drm/panel/panel-samsung-s6e63m0-spi.c b/drivers/gpu/drm/panel/panel-samsung-s6e63m0-spi.c
index 326deb3177b6..293c18ee448a 100644
--- a/drivers/gpu/drm/panel/panel-samsung-s6e63m0-spi.c
+++ b/drivers/gpu/drm/panel/panel-samsung-s6e63m0-spi.c
@@ -5,62 +5,42 @@
 #include <linux/spi/spi.h>
 #include <linux/delay.h>
 
+#include <drm/drm_mipi_dbi.h>
 #include <drm/drm_print.h>
 
 #include "panel-samsung-s6e63m0.h"
 
-#define DATA_MASK	0x100
+static const u8 s6e63m0_dbi_read_commands[] = {
+	MCS_READ_ID1,
+	MCS_READ_ID2,
+	MCS_READ_ID3,
+	0, /* sentinel */
+};
 
-static int s6e63m0_spi_dcs_read(struct device *dev, const u8 cmd, u8 *data)
+static int s6e63m0_spi_dcs_read(struct device *dev, void *trsp,
+				const u8 cmd, u8 *data)
 {
-	struct spi_device *spi = to_spi_device(dev);
-	u16 buf[1];
-	u16 rbuf[1];
+	struct mipi_dbi *dbi = trsp;
 	int ret;
 
-	/* SPI buffers are always in CPU order */
-	buf[0] = (u16)cmd;
-	ret = spi_write_then_read(spi, buf, 2, rbuf, 2);
-	dev_dbg(dev, "READ CMD: %04x RET: %04x\n", buf[0], rbuf[0]);
-	if (!ret)
-		/* These high 8 bits of the 9 contains the readout */
-		*data = (rbuf[0] & 0x1ff) >> 1;
+	ret = mipi_dbi_command_read(dbi, cmd, data);
+	if (ret)
+		dev_err(dev, "error on DBI read command %02x\n", cmd);
+	else
+		dev_dbg(dev, "read DBI %02x\n", *data);
 
 	return ret;
 }
 
-static int s6e63m0_spi_write_word(struct device *dev, u16 data)
-{
-	struct spi_device *spi = to_spi_device(dev);
-
-	/* SPI buffers are always in CPU order */
-	return spi_write(spi, &data, 2);
-}
-
-static int s6e63m0_spi_dcs_write(struct device *dev, const u8 *data, size_t len)
+static int s6e63m0_spi_dcs_write(struct device *dev, void *trsp,
+				 const u8 *data, size_t len)
 {
-	int ret = 0;
+	struct mipi_dbi *dbi = trsp;
+	int ret;
 
 	dev_dbg(dev, "SPI writing dcs seq: %*ph\n", (int)len, data);
 
-	/*
-	 * This sends 9 bits with the first bit (bit 8) set to 0
-	 * This indicates that this is a command. Anything after the
-	 * command is data.
-	 */
-	ret = s6e63m0_spi_write_word(dev, *data);
-
-	while (!ret && --len) {
-		++data;
-		/* This sends 9 bits with the first bit (bit 8) set to 1 */
-		ret = s6e63m0_spi_write_word(dev, *data | DATA_MASK);
-	}
-
-	if (ret) {
-		dev_err(dev, "SPI error %d writing dcs seq: %*ph\n", ret,
-			(int)len, data);
-	}
-
+	ret = mipi_dbi_command_stackbuf(dbi, data[0], (data + 1), (len - 1));
 	usleep_range(300, 310);
 
 	return ret;
@@ -69,18 +49,23 @@ static int s6e63m0_spi_dcs_write(struct device *dev, const u8 *data, size_t len)
 static int s6e63m0_spi_probe(struct spi_device *spi)
 {
 	struct device *dev = &spi->dev;
+	struct mipi_dbi *dbi;
 	int ret;
 
-	spi->bits_per_word = 9;
-	/* Preserve e.g. SPI_3WIRE setting */
-	spi->mode |= SPI_MODE_3;
-	ret = spi_setup(spi);
-	if (ret < 0) {
-		dev_err(dev, "spi setup failed.\n");
-		return ret;
-	}
-	return s6e63m0_probe(dev, s6e63m0_spi_dcs_read, s6e63m0_spi_dcs_write,
-			     false);
+	dbi = devm_kzalloc(dev, sizeof(*dbi), GFP_KERNEL);
+	if (!dbi)
+		return -ENOMEM;
+
+	ret = mipi_dbi_spi_init(spi, dbi, NULL);
+	if (ret)
+		return dev_err_probe(dev, ret, "MIPI DBI init failed\n");
+	/* Register our custom MCS read commands */
+	dbi->read_commands = s6e63m0_dbi_read_commands;
+
+	spi_set_drvdata(spi, dbi);
+
+	return s6e63m0_probe(dev, dbi, s6e63m0_spi_dcs_read,
+			     s6e63m0_spi_dcs_write, false);
 }
 
 static int s6e63m0_spi_remove(struct spi_device *spi)
diff --git a/drivers/gpu/drm/panel/panel-samsung-s6e63m0.c b/drivers/gpu/drm/panel/panel-samsung-s6e63m0.c
index 5e4d2e8aa7a7..3a8a772245c0 100644
--- a/drivers/gpu/drm/panel/panel-samsung-s6e63m0.c
+++ b/drivers/gpu/drm/panel/panel-samsung-s6e63m0.c
@@ -24,31 +24,6 @@
 
 #include "panel-samsung-s6e63m0.h"
 
-/* Manufacturer Command Set */
-#define MCS_ELVSS_ON		0xb1
-#define MCS_TEMP_SWIRE		0xb2
-#define MCS_PENTILE_1		0xb3
-#define MCS_PENTILE_2		0xb4
-#define MCS_GAMMA_DELTA_Y_RED	0xb5
-#define MCS_GAMMA_DELTA_X_RED	0xb6
-#define MCS_GAMMA_DELTA_Y_GREEN	0xb7
-#define MCS_GAMMA_DELTA_X_GREEN	0xb8
-#define MCS_GAMMA_DELTA_Y_BLUE	0xb9
-#define MCS_GAMMA_DELTA_X_BLUE	0xba
-#define MCS_MIECTL1		0xc0
-#define MCS_BCMODE		0xc1
-#define MCS_ERROR_CHECK		0xd5
-#define MCS_READ_ID1		0xda
-#define MCS_READ_ID2		0xdb
-#define MCS_READ_ID3		0xdc
-#define MCS_LEVEL_2_KEY		0xf0
-#define MCS_MTP_KEY		0xf1
-#define MCS_DISCTL		0xf2
-#define MCS_SRCCTL		0xf6
-#define MCS_IFCTL		0xf7
-#define MCS_PANELCTL		0xf8
-#define MCS_PGAMMACTL		0xfa
-
 #define S6E63M0_LCD_ID_VALUE_M2		0xA4
 #define S6E63M0_LCD_ID_VALUE_SM2	0xB4
 #define S6E63M0_LCD_ID_VALUE_SM2_1	0xB6
@@ -285,8 +260,9 @@ static u8 const s6e63m0_elvss_per_gamma[NUM_GAMMA_LEVELS] = {
 
 struct s6e63m0 {
 	struct device *dev;
-	int (*dcs_read)(struct device *dev, const u8 cmd, u8 *val);
-	int (*dcs_write)(struct device *dev, const u8 *data, size_t len);
+	void *transport_data;
+	int (*dcs_read)(struct device *dev, void *trsp, const u8 cmd, u8 *val);
+	int (*dcs_write)(struct device *dev, void *trsp, const u8 *data, size_t len);
 	struct drm_panel panel;
 	struct backlight_device *bl_dev;
 	u8 lcd_type;
@@ -342,7 +318,7 @@ static void s6e63m0_dcs_read(struct s6e63m0 *ctx, const u8 cmd, u8 *data)
 	if (ctx->error < 0)
 		return;
 
-	ctx->error = ctx->dcs_read(ctx->dev, cmd, data);
+	ctx->error = ctx->dcs_read(ctx->dev, ctx->transport_data, cmd, data);
 }
 
 static void s6e63m0_dcs_write(struct s6e63m0 *ctx, const u8 *data, size_t len)
@@ -350,7 +326,7 @@ static void s6e63m0_dcs_write(struct s6e63m0 *ctx, const u8 *data, size_t len)
 	if (ctx->error < 0 || len == 0)
 		return;
 
-	ctx->error = ctx->dcs_write(ctx->dev, data, len);
+	ctx->error = ctx->dcs_write(ctx->dev, ctx->transport_data, data, len);
 }
 
 #define s6e63m0_dcs_write_seq_static(ctx, seq ...) \
@@ -727,9 +703,9 @@ static irqreturn_t s6e63m0_esd_irq(int irq, void *data)
 	return IRQ_HANDLED;
 }
 
-int s6e63m0_probe(struct device *dev,
-		  int (*dcs_read)(struct device *dev, const u8 cmd, u8 *val),
-		  int (*dcs_write)(struct device *dev, const u8 *data, size_t len),
+int s6e63m0_probe(struct device *dev, void *trsp,
+		  int (*dcs_read)(struct device *dev, void *trsp, const u8 cmd, u8 *val),
+		  int (*dcs_write)(struct device *dev, void *trsp, const u8 *data, size_t len),
 		  bool dsi_mode)
 {
 	struct s6e63m0 *ctx;
@@ -741,6 +717,7 @@ int s6e63m0_probe(struct device *dev,
 	if (!ctx)
 		return -ENOMEM;
 
+	ctx->transport_data = trsp;
 	ctx->dsi_mode = dsi_mode;
 	ctx->dcs_read = dcs_read;
 	ctx->dcs_write = dcs_write;
diff --git a/drivers/gpu/drm/panel/panel-samsung-s6e63m0.h b/drivers/gpu/drm/panel/panel-samsung-s6e63m0.h
index c669fec91763..306605ed1117 100644
--- a/drivers/gpu/drm/panel/panel-samsung-s6e63m0.h
+++ b/drivers/gpu/drm/panel/panel-samsung-s6e63m0.h
@@ -3,9 +3,36 @@
 #ifndef _PANEL_SAMSUNG_S6E63M0_H
 #define _PANEL_SAMSUNG_S6E63M0_H
 
-int s6e63m0_probe(struct device *dev,
-		  int (*dcs_read)(struct device *dev, const u8 cmd, u8 *val),
-		  int (*dcs_write)(struct device *dev, const u8 *data,
+/* Manufacturer Command Set */
+#define MCS_ELVSS_ON		0xb1
+#define MCS_TEMP_SWIRE		0xb2
+#define MCS_PENTILE_1		0xb3
+#define MCS_PENTILE_2		0xb4
+#define MCS_GAMMA_DELTA_Y_RED	0xb5
+#define MCS_GAMMA_DELTA_X_RED	0xb6
+#define MCS_GAMMA_DELTA_Y_GREEN	0xb7
+#define MCS_GAMMA_DELTA_X_GREEN	0xb8
+#define MCS_GAMMA_DELTA_Y_BLUE	0xb9
+#define MCS_GAMMA_DELTA_X_BLUE	0xba
+#define MCS_MIECTL1		0xc0
+#define MCS_BCMODE		0xc1
+#define MCS_ERROR_CHECK		0xd5
+#define MCS_READ_ID1		0xda
+#define MCS_READ_ID2		0xdb
+#define MCS_READ_ID3		0xdc
+#define MCS_LEVEL_2_KEY		0xf0
+#define MCS_MTP_KEY		0xf1
+#define MCS_DISCTL		0xf2
+#define MCS_SRCCTL		0xf6
+#define MCS_IFCTL		0xf7
+#define MCS_PANELCTL		0xf8
+#define MCS_PGAMMACTL		0xfa
+
+int s6e63m0_probe(struct device *dev, void *trsp,
+		  int (*dcs_read)(struct device *dev, void *trsp,
+				  const u8 cmd, u8 *val),
+		  int (*dcs_write)(struct device *dev, void *trsp,
+				   const u8 *data,
 				   size_t len),
 		  bool dsi_mode);
 int s6e63m0_remove(struct device *dev);
-- 
2.31.1

Re: [PATCH 2/2] drm/panel: s6e63m0: Switch to DBI abstraction for SPI

[Noralf Trønnes]

Den 11.06.2021 23.42, skrev Linus Walleij:
> The SPI access to s6e63m0 is using the DBI protocol, so switch
> to using the elaborate DBI protocol implementation in the DRM
> DBI helper library.
> 
> Cc: Douglas Anderson <dianders@chromium.org>
> Cc: Noralf Trønnes <noralf@tronnes.org>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---

> diff --git a/drivers/gpu/drm/panel/panel-samsung-s6e63m0-spi.c b/drivers/gpu/drm/panel/panel-samsung-s6e63m0-spi.c
> index 326deb3177b6..293c18ee448a 100644
> --- a/drivers/gpu/drm/panel/panel-samsung-s6e63m0-spi.c
> +++ b/drivers/gpu/drm/panel/panel-samsung-s6e63m0-spi.c
> @@ -5,62 +5,42 @@
>  #include <linux/spi/spi.h>
>  #include <linux/delay.h>
>  
> +#include <drm/drm_mipi_dbi.h>
>  #include <drm/drm_print.h>
>  
>  #include "panel-samsung-s6e63m0.h"
>  
> -#define DATA_MASK	0x100
> +static const u8 s6e63m0_dbi_read_commands[] = {
> +	MCS_READ_ID1,
> +	MCS_READ_ID2,
> +	MCS_READ_ID3,
> +	0, /* sentinel */
> +};
>  
> -static int s6e63m0_spi_dcs_read(struct device *dev, const u8 cmd, u8 *data)
> +static int s6e63m0_spi_dcs_read(struct device *dev, void *trsp,
> +				const u8 cmd, u8 *data)
>  {
> -	struct spi_device *spi = to_spi_device(dev);
> -	u16 buf[1];
> -	u16 rbuf[1];
> +	struct mipi_dbi *dbi = trsp;
>  	int ret;
>  
> -	/* SPI buffers are always in CPU order */
> -	buf[0] = (u16)cmd;
> -	ret = spi_write_then_read(spi, buf, 2, rbuf, 2);
> -	dev_dbg(dev, "READ CMD: %04x RET: %04x\n", buf[0], rbuf[0]);
> -	if (!ret)
> -		/* These high 8 bits of the 9 contains the readout */
> -		*data = (rbuf[0] & 0x1ff) >> 1;
> +	ret = mipi_dbi_command_read(dbi, cmd, data);
> +	if (ret)
> +		dev_err(dev, "error on DBI read command %02x\n", cmd);
> +	else
> +		dev_dbg(dev, "read DBI %02x\n", *data);

You could drop this dev_dbg() and set drm.debug=2 instead to get the
same info. See MIPI_DBI_DEBUG_COMMAND().

>  
>  	return ret;
>  }
>  
> -static int s6e63m0_spi_write_word(struct device *dev, u16 data)
> -{
> -	struct spi_device *spi = to_spi_device(dev);
> -
> -	/* SPI buffers are always in CPU order */
> -	return spi_write(spi, &data, 2);
> -}
> -
> -static int s6e63m0_spi_dcs_write(struct device *dev, const u8 *data, size_t len)
> +static int s6e63m0_spi_dcs_write(struct device *dev, void *trsp,
> +				 const u8 *data, size_t len)
>  {
> -	int ret = 0;
> +	struct mipi_dbi *dbi = trsp;
> +	int ret;
>  
>  	dev_dbg(dev, "SPI writing dcs seq: %*ph\n", (int)len, data);

Same here MIPI_DBI_DEBUG_COMMAND() can print this.

>  
> -	/*
> -	 * This sends 9 bits with the first bit (bit 8) set to 0
> -	 * This indicates that this is a command. Anything after the
> -	 * command is data.
> -	 */
> -	ret = s6e63m0_spi_write_word(dev, *data);
> -
> -	while (!ret && --len) {
> -		++data;
> -		/* This sends 9 bits with the first bit (bit 8) set to 1 */
> -		ret = s6e63m0_spi_write_word(dev, *data | DATA_MASK);
> -	}
> -
> -	if (ret) {
> -		dev_err(dev, "SPI error %d writing dcs seq: %*ph\n", ret,
> -			(int)len, data);
> -	}
> -
> +	ret = mipi_dbi_command_stackbuf(dbi, data[0], (data + 1), (len - 1));

_stackbuf was not meant to be used by drivers, but I see that the driver
layering gives you no choice (except open coding ofc). So:

Acked-by: Noralf Trønnes <noralf@tronnes.org>

Noralf.

>  	usleep_range(300, 310);
>  
>  	return ret;

Re: [PATCH 2/2] drm/panel: s6e63m0: Switch to DBI abstraction for SPI

[Doug Anderson]

Hi,

On Fri, Jun 11, 2021 at 2:44 PM Linus Walleij <linus.walleij@linaro.org> wrote:
>
>  static int s6e63m0_spi_probe(struct spi_device *spi)
>  {
>         struct device *dev = &spi->dev;
> +       struct mipi_dbi *dbi;
>         int ret;
>
> -       spi->bits_per_word = 9;
> -       /* Preserve e.g. SPI_3WIRE setting */
> -       spi->mode |= SPI_MODE_3;
> -       ret = spi_setup(spi);
> -       if (ret < 0) {
> -               dev_err(dev, "spi setup failed.\n");
> -               return ret;
> -       }
> -       return s6e63m0_probe(dev, s6e63m0_spi_dcs_read, s6e63m0_spi_dcs_write,
> -                            false);
> +       dbi = devm_kzalloc(dev, sizeof(*dbi), GFP_KERNEL);
> +       if (!dbi)
> +               return -ENOMEM;
> +
> +       ret = mipi_dbi_spi_init(spi, dbi, NULL);
> +       if (ret)
> +               return dev_err_probe(dev, ret, "MIPI DBI init failed\n");
> +       /* Register our custom MCS read commands */
> +       dbi->read_commands = s6e63m0_dbi_read_commands;
> +
> +       spi_set_drvdata(spi, dbi);

I _think_ you want to remove the spi_set_drvdata(). It just gets
clobbered by the dev_set_drvdata() in s6e63m0_probe(), right? That's
why you had to add the "void *" data pointer?

Other than that this looks right to my non-expert eyes. ;-)

Reviewed-by: Douglas Anderson <dianders@chromium.org>