Re: [PATCH v6] drm/panel: Add a driver for the TPO TPG110

[Sam Ravnborg <sam@ravnborg.org>]

Hi Linus.

On Wed, Jan 09, 2019 at 02:53:31PM +0100, Linus Walleij wrote:
> The TPO (Toppoly) TPG110 is a pretty generic display driver
> similar in vein to the Ilitek 93xx devices. It is not a panel
> per se but a driver used with several low-cost noname panels.
> 
> This is used on the Nomadik NHK15 combined with a OSD
> OSD057VA01CT display for WVGA 800x480.
> 
> The driver is pretty minimalistic right now but can be
> extended to handle non-default polarities, gamma correction
> etc.
> 
> The driver is based on the baked-in code in
> drivers/video/fbdev/amba-clcd-nomadik.c which will be
> decomissioned once this us upstream.
> 
> Acked-by: Sam Ravnborg <sam@ravnborg.org>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

While browsing this code again a few things jumped at me.
- we try to kill use of drmP.h
- DRM_DEV_XXX rather than dev_xxx (not all like these, but they are used in other panels)
- drop videomode - not used anymore.

I went ahead and made a patch. Build tested on top of drm-misc-next
(had to define SPI_3WIRE_HIZ, because we miss a backmerge of -rc1)

Apply what you think is relevant, and sorry for the late feedback.

	Sam

diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
index e3821180b6cd..a71f44191273 100644
--- a/drivers/gpu/drm/panel/Kconfig
+++ b/drivers/gpu/drm/panel/Kconfig
@@ -208,7 +208,6 @@ config DRM_PANEL_TPO_TPG110
 	tristate "TPO TPG 800x400 panel"
 	depends on OF && SPI && GPIOLIB
 	depends on BACKLIGHT_CLASS_DEVICE
-	select VIDEOMODE_HELPERS
 	help
 	  Say Y here if you want to enable support for TPO TPG110
 	  400CH LTPS TFT LCD Single Chip Digital Driver for up to
diff --git a/drivers/gpu/drm/panel/panel-tpo-tpg110.c b/drivers/gpu/drm/panel/panel-tpo-tpg110.c
index d7c3541fdf28..67c7be8eb166 100644
--- a/drivers/gpu/drm/panel/panel-tpo-tpg110.c
+++ b/drivers/gpu/drm/panel/panel-tpo-tpg110.c
@@ -10,11 +10,14 @@
  * Author:
  * Linus Walleij <linus.walleij@linaro.org>
  */
-#include <drm/drmP.h>
+
+#include <drm/drm_modes.h>
 #include <drm/drm_panel.h>
+#include <drm/drm_print.h>
 
 #include <linux/backlight.h>
 #include <linux/bitops.h>
+#include <linux/delay.h>
 #include <linux/gpio/consumer.h>
 #include <linux/init.h>
 #include <linux/kernel.h>
@@ -22,9 +25,6 @@
 #include <linux/platform_device.h>
 #include <linux/spi/spi.h>
 
-#include <video/of_videomode.h>
-#include <video/videomode.h>
-
 #define TPG110_TEST			0x00
 #define TPG110_CHIPID			0x01
 #define TPG110_CTRL1			0x02
@@ -248,7 +248,7 @@ static u8 tpg110_readwrite_reg(struct tpg110 *tpg, bool write,
 	spi_message_add_tail(&t[1], &m);
 	ret = spi_sync(tpg->spi, &m);
 	if (ret) {
-		dev_err(tpg->dev, "SPI message error %d\n", ret);
+		DRM_DEV_ERROR(tpg->dev, "SPI message error %d\n", ret);
 		return ret;
 	}
 	if (write)
@@ -275,46 +275,46 @@ static int tpg110_startup(struct tpg110 *tpg)
 	/* De-assert the reset signal */
 	gpiod_set_value_cansleep(tpg->grestb, 0);
 	mdelay(1);
-	dev_info(tpg->dev, "de-asserted GRESTB\n");
+	DRM_DEV_INFO(tpg->dev, "de-asserted GRESTB\n");
 
 	/* Test display communication */
 	tpg110_write_reg(tpg, TPG110_TEST, 0x55);
 	val = tpg110_read_reg(tpg, TPG110_TEST);
 	if (val != 0x55) {
-		dev_err(tpg->dev, "failed communication test\n");
+		DRM_DEV_ERROR(tpg->dev, "failed communication test\n");
 		return -ENODEV;
 	}
 
 	val = tpg110_read_reg(tpg, TPG110_CHIPID);
-	dev_info(tpg->dev, "TPG110 chip ID: %d version: %d\n",
-		 val >> 4, val & 0x0f);
+	DRM_DEV_INFO(tpg->dev, "TPG110 chip ID: %d version: %d\n",
+		     val >> 4, val & 0x0f);
 
 	/* Show display resolution */
 	val = tpg110_read_reg(tpg, TPG110_CTRL1);
 	val &= TPG110_RES_MASK;
 	switch (val) {
 	case TPG110_RES_400X240_D:
-		dev_info(tpg->dev,
-			 "IN 400x240 RGB -> OUT 800x480 RGB (dual scan)");
+		DRM_DEV_INFO(tpg->dev,
+			     "IN 400x240 RGB -> OUT 800x480 RGB (dual scan)");
 		break;
 	case TPG110_RES_480X272_D:
-		dev_info(tpg->dev,
-			 "IN 480x272 RGB -> OUT 800x480 RGB (dual scan)");
+		DRM_DEV_INFO(tpg->dev,
+			     "IN 480x272 RGB -> OUT 800x480 RGB (dual scan)");
 		break;
 	case TPG110_RES_480X640:
-		dev_info(tpg->dev, "480x640 RGB");
+		DRM_DEV_INFO(tpg->dev, "480x640 RGB");
 		break;
 	case TPG110_RES_480X272:
-		dev_info(tpg->dev, "480x272 RGB");
+		DRM_DEV_INFO(tpg->dev, "480x272 RGB");
 		break;
 	case TPG110_RES_640X480:
-		dev_info(tpg->dev, "640x480 RGB");
+		DRM_DEV_INFO(tpg->dev, "640x480 RGB");
 		break;
 	case TPG110_RES_800X480:
-		dev_info(tpg->dev, "800x480 RGB");
+		DRM_DEV_INFO(tpg->dev, "800x480 RGB");
 		break;
 	default:
-		dev_info(tpg->dev, "ILLEGAL RESOLUTION");
+		DRM_DEV_INFO(tpg->dev, "ILLEGAL RESOLUTION");
 		break;
 	}
 
@@ -332,13 +332,13 @@ static int tpg110_startup(struct tpg110 *tpg)
 		}
 	}
 	if (i == ARRAY_SIZE(tpg110_modes)) {
-		dev_err(tpg->dev, "unsupported mode (%02x) detected\n",
+		DRM_DEV_ERROR(tpg->dev, "unsupported mode (%02x) detected\n",
 			val);
 		return -ENODEV;
 	}
 
 	val = tpg110_read_reg(tpg, TPG110_CTRL2);
-	dev_info(tpg->dev, "resolution and standby is controlled by %s\n",
+	DRM_DEV_INFO(tpg->dev, "resolution and standby is controlled by %s\n",
 		 (val & TPG110_CTRL2_RES_PM_CTRL) ? "software" : "hardware");
 	/* Take control over resolution and standby */
 	val |= TPG110_CTRL2_RES_PM_CTRL;
@@ -439,10 +439,10 @@ static int tpg110_probe(struct spi_device *spi)
 	/* We get the physical display dimensions from the DT */
 	ret = of_property_read_u32(np, "width-mm", &tpg->width);
 	if (ret)
-		dev_err(dev, "no panel width specified\n");
+		DRM_DEV_ERROR(dev, "no panel width specified\n");
 	ret = of_property_read_u32(np, "height-mm", &tpg->height);
 	if (ret)
-		dev_err(dev, "no panel height specified\n");
+		DRM_DEV_ERROR(dev, "no panel height specified\n");
 
 	/* Look for some optional backlight */
 	backlight = of_parse_phandle(np, "backlight", 0);
@@ -457,7 +457,7 @@ static int tpg110_probe(struct spi_device *spi)
 	/* This asserts the GRESTB signal, putting the display into reset */
 	tpg->grestb = devm_gpiod_get(dev, "grestb", GPIOD_OUT_HIGH);
 	if (IS_ERR(tpg->grestb)) {
-		dev_err(dev, "no GRESTB GPIO\n");
+		DRM_DEV_ERROR(dev, "no GRESTB GPIO\n");
 		return -ENODEV;
 	}
 
@@ -465,7 +465,7 @@ static int tpg110_probe(struct spi_device *spi)
 	spi->mode |= SPI_3WIRE_HIZ;
 	ret = spi_setup(spi);
 	if (ret < 0) {
-		dev_err(dev, "spi setup failed.\n");
+		DRM_DEV_ERROR(dev, "spi setup failed.\n");
 		return ret;
 	}
 	tpg->spi = spi;
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel