[PATCH 0/4] drm/mipi-dbi: Support panel drivers

[PATCH 0/4] drm/mipi-dbi: Support panel drivers

[Noralf Trønnes]

This series adds drm_panel support to drm_mipi_dbi. This avoids having 2
drivers for the same controller, one per interface DPI/DBI. Some display
controllers like the ILI9341 support both interfaces and some
displays/panels make both these interfaces available.

Some info about why MIPI DBI is special:
MIPI DSI (Display Serial Interface) and MIPI DPI (Display Pixel
Interface) needs specialiced hardware and thus they have interface
drivers for this. MIPI DBI (Display Bus Interface) on the other hand
reuses existing busses like SPI/8080/6800 and in the case of SPI just
reuses the exising SPI subsystem. All the panels I have seen that have
onboard RAM and can receive pixels over DBI needs to initialize the
controller as well. This means that both the panel driver and the
interface driver needs access to the same bus device.

Changes since RFC[1]:
- Instead of starting with Josef's driver[2], I've started with moving
  the existing ili9341 driver into drm/panel and then move over mi0283qt
  as well since it has a ili9341 controller. Josef can then add support
  for his panel to this driver. His panel can support both interface
  modes.
- Emil mentioned that the DRM driver name is ABI so I've honoured that.
- The DRM driver structure is now defined in the driver using a macro
  instead of doing it in drm_mipi_dbi. This is necessary for file open to
  take a ref on the driver module.

Another thing:
Looking at drm_panel.c I couldn't find anything that pins the panel
driver module while the panel is attached to the connector. Shouldn't
struct drm_panel_funcs have a module owner field?

Noralf.

[1] https://patchwork.freedesktop.org/series/64398/
[2] https://patchwork.freedesktop.org/patch/316528/

Noralf Trønnes (4):
  drm/mipi-dbi: Support command mode panel drivers
  drm/tiny/ili9341: Move driver to drm/panel
  drm/tiny/mi0283qt: Move driver to panel-ilitek-ili9341
  drm/panel/ili9341: Support DPI panels

 MAINTAINERS                                  |  16 +-
 drivers/gpu/drm/drm_mipi_dbi.c               |  92 ++++
 drivers/gpu/drm/panel/Kconfig                |  13 +
 drivers/gpu/drm/panel/Makefile               |   1 +
 drivers/gpu/drm/panel/panel-ilitek-ili9341.c | 469 +++++++++++++++++++
 drivers/gpu/drm/tiny/Kconfig                 |  24 -
 drivers/gpu/drm/tiny/Makefile                |   2 -
 drivers/gpu/drm/tiny/ili9341.c               | 268 -----------
 drivers/gpu/drm/tiny/mi0283qt.c              | 294 ------------
 include/drm/drm_mipi_dbi.h                   |  34 ++
 10 files changed, 618 insertions(+), 595 deletions(-)
 create mode 100644 drivers/gpu/drm/panel/panel-ilitek-ili9341.c
 delete mode 100644 drivers/gpu/drm/tiny/ili9341.c
 delete mode 100644 drivers/gpu/drm/tiny/mi0283qt.c

-- 
2.20.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/4] drm/mipi-dbi: Support command mode panel drivers

[Noralf Trønnes]

This adds a function that registers a DRM driver for use with MIPI DBI
panels in command mode. That is, framebuffer upload over DBI.

Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
 drivers/gpu/drm/drm_mipi_dbi.c | 92 ++++++++++++++++++++++++++++++++++
 include/drm/drm_mipi_dbi.h     | 34 +++++++++++++
 2 files changed, 126 insertions(+)

diff --git a/drivers/gpu/drm/drm_mipi_dbi.c b/drivers/gpu/drm/drm_mipi_dbi.c
index 1961f713aaab..797a20e3a017 100644
--- a/drivers/gpu/drm/drm_mipi_dbi.c
+++ b/drivers/gpu/drm/drm_mipi_dbi.c
@@ -17,11 +17,13 @@
 #include <drm/drm_damage_helper.h>
 #include <drm/drm_drv.h>
 #include <drm/drm_gem_cma_helper.h>
+#include <drm/drm_fb_helper.h>
 #include <drm/drm_format_helper.h>
 #include <drm/drm_fourcc.h>
 #include <drm/drm_gem_framebuffer_helper.h>
 #include <drm/drm_mipi_dbi.h>
 #include <drm/drm_modes.h>
+#include <drm/drm_panel.h>
 #include <drm/drm_probe_helper.h>
 #include <drm/drm_rect.h>
 #include <drm/drm_vblank.h>
@@ -597,6 +599,96 @@ void mipi_dbi_release(struct drm_device *drm)
 }
 EXPORT_SYMBOL(mipi_dbi_release);
 
+static void drm_mipi_dbi_panel_pipe_enable(struct drm_simple_display_pipe *pipe,
+					   struct drm_crtc_state *crtc_state,
+					   struct drm_plane_state *plane_state)
+{
+	struct mipi_dbi_dev *dbidev = drm_to_mipi_dbi_dev(pipe->crtc.dev);
+	struct drm_panel *panel = dbidev->panel;
+	int ret, idx;
+
+	if (!drm_dev_enter(pipe->crtc.dev, &idx))
+		return;
+
+	DRM_DEBUG_KMS("\n");
+
+	ret = drm_panel_prepare(panel);
+	if (ret)
+		goto out_exit;
+
+	mipi_dbi_enable_flush(dbidev, crtc_state, plane_state);
+
+	drm_panel_enable(panel);
+out_exit:
+	drm_dev_exit(idx);
+}
+
+static void drm_mipi_dbi_panel_pipe_disable(struct drm_simple_display_pipe *pipe)
+{
+	struct mipi_dbi_dev *dbidev = drm_to_mipi_dbi_dev(pipe->crtc.dev);
+	struct drm_panel *panel = dbidev->panel;
+
+	if (!dbidev->enabled)
+		return;
+
+	DRM_DEBUG_KMS("\n");
+
+	dbidev->enabled = false;
+	drm_panel_disable(panel);
+	drm_panel_unprepare(panel);
+}
+
+static const struct drm_simple_display_pipe_funcs drm_mipi_dbi_pipe_funcs = {
+	.enable = drm_mipi_dbi_panel_pipe_enable,
+	.disable = drm_mipi_dbi_panel_pipe_disable,
+	.update = mipi_dbi_pipe_update,
+	.prepare_fb = drm_gem_fb_simple_display_pipe_prepare_fb,
+};
+
+/**
+ * drm_mipi_dbi_panel_register - Register a MIPI DBI DRM driver
+ * @panel: DRM Panel
+ * @dbidev: MIPI DBI device structure to initialize
+ * @mode: Display mode
+ *
+ * This function registeres a DRM driver with @panel attached.
+ *
+ * Returns:
+ * Zero on success, negative error code on failure.
+ */
+int drm_mipi_dbi_panel_register(struct drm_panel *panel, struct mipi_dbi_dev *dbidev,
+				struct drm_driver *driver, const struct drm_display_mode *mode,
+				u32 rotation)
+{
+	struct device *dev = panel->dev;
+	struct drm_device *drm;
+	int ret;
+
+	dbidev->panel = panel;
+
+	drm = &dbidev->drm;
+	ret = devm_drm_dev_init(dev, drm, driver);
+	if (ret) {
+		kfree(dbidev);
+		return ret;
+	}
+
+	drm_mode_config_init(drm);
+
+	ret = mipi_dbi_dev_init(dbidev, &drm_mipi_dbi_pipe_funcs, mode, rotation);
+
+	drm_mode_config_reset(drm);
+
+	ret = drm_dev_register(drm, 0);
+	if (ret)
+		return ret;
+
+	drm_fbdev_generic_setup(drm, 16);
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_mipi_dbi_panel_register);
+
 /**
  * mipi_dbi_hw_reset - Hardware reset of controller
  * @dbi: MIPI DBI structure
diff --git a/include/drm/drm_mipi_dbi.h b/include/drm/drm_mipi_dbi.h
index 67c66f5ee591..f41ee0d31871 100644
--- a/include/drm/drm_mipi_dbi.h
+++ b/include/drm/drm_mipi_dbi.h
@@ -12,6 +12,7 @@
 #include <drm/drm_device.h>
 #include <drm/drm_simple_kms_helper.h>
 
+struct drm_panel;
 struct drm_rect;
 struct spi_device;
 struct gpio_desc;
@@ -123,6 +124,11 @@ struct mipi_dbi_dev {
 	 * @dbi: MIPI DBI interface
 	 */
 	struct mipi_dbi dbi;
+
+	/**
+	 * @panel: Attached DRM panel. See drm_mipi_dbi_panel_register().
+	 */
+	struct drm_panel *panel;
 };
 
 static inline struct mipi_dbi_dev *drm_to_mipi_dbi_dev(struct drm_device *drm)
@@ -140,6 +146,34 @@ int mipi_dbi_dev_init_with_formats(struct mipi_dbi_dev *dbidev,
 int mipi_dbi_dev_init(struct mipi_dbi_dev *dbidev,
 		      const struct drm_simple_display_pipe_funcs *funcs,
 		      const struct drm_display_mode *mode, unsigned int rotation);
+
+/**
+ * DEFINE_DRM_MIPI_DBI_PANEL_DRIVER - Define a DRM driver structure
+ * @_name: Name
+ * @_desc: Description
+ * @_date: Date
+ *
+ * This macro defines a &drm_driver for MIPI DBI panel drivers.
+ */
+#define DEFINE_DRM_MIPI_DBI_PANEL_DRIVER(_name, _desc, _date) \
+	DEFINE_DRM_GEM_CMA_FOPS(_name ## _fops); \
+	static struct drm_driver _name ## _drm_driver = { \
+		.driver_features	= DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC, \
+		.fops			= & _name ## _fops, \
+		.release		= mipi_dbi_release, \
+		DRM_GEM_CMA_VMAP_DRIVER_OPS, \
+		.debugfs_init		= mipi_dbi_debugfs_init, \
+		.name			= #_name, \
+		.desc			= _desc, \
+		.date			= _date, \
+		.major			= 1, \
+		.minor			= 0, \
+	}
+
+int drm_mipi_dbi_panel_register(struct drm_panel *panel, struct mipi_dbi_dev *dbidev,
+				struct drm_driver *driver, const struct drm_display_mode *mode,
+				u32 rotation);
+
 void mipi_dbi_release(struct drm_device *drm);
 void mipi_dbi_pipe_update(struct drm_simple_display_pipe *pipe,
 			  struct drm_plane_state *old_state);
-- 
2.20.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/4] drm/tiny/ili9341: Move driver to drm/panel

[Noralf Trønnes]

Move the driver to drm/panel and take advantage of the new panel support
in drm_mipi_dbi. Change the file name to match the naming standard in
drm/panel. The DRM driver name is kept since it is ABI.

Add missing MAINTAINERS entry.

Cc: David Lechner <david@lechnology.com>
Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
 MAINTAINERS                                   |   7 +
 drivers/gpu/drm/panel/Kconfig                 |  12 ++
 drivers/gpu/drm/panel/Makefile                |   1 +
 .../panel-ilitek-ili9341.c}                   | 174 ++++++++++--------
 drivers/gpu/drm/tiny/Kconfig                  |  13 --
 drivers/gpu/drm/tiny/Makefile                 |   1 -
 6 files changed, 113 insertions(+), 95 deletions(-)
 rename drivers/gpu/drm/{tiny/ili9341.c => panel/panel-ilitek-ili9341.c} (66%)

diff --git a/MAINTAINERS b/MAINTAINERS
index 6fe3462a1f7a..66b3893a100f 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5108,6 +5108,13 @@ S:	Maintained
 F:	drivers/gpu/drm/tiny/ili9225.c
 F:	Documentation/devicetree/bindings/display/ilitek,ili9225.txt
 
+DRM DRIVER FOR ILITEK ILI9341 PANELS
+M:	David Lechner <david@lechnology.com>
+T:	git git://anongit.freedesktop.org/drm/drm-misc
+S:	Maintained
+F:	drivers/gpu/drm/panel/panel-ilitek-ili9341.c
+F:	Documentation/devicetree/bindings/display/ilitek,ili9341.txt
+
 DRM DRIVER FOR HX8357D PANELS
 M:	Eric Anholt <eric@anholt.net>
 T:	git git://anongit.freedesktop.org/drm/drm-misc
diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
index eaecd40cc32e..a24953ec2d40 100644
--- a/drivers/gpu/drm/panel/Kconfig
+++ b/drivers/gpu/drm/panel/Kconfig
@@ -56,6 +56,18 @@ config DRM_PANEL_ILITEK_IL9322
 	  Say Y here if you want to enable support for Ilitek IL9322
 	  QVGA (320x240) RGB, YUV and ITU-T BT.656 panels.
 
+config DRM_PANEL_ILITEK_ILI9341
+	tristate "Ilitek ILI9341 display panels"
+	depends on SPI
+	depends on BACKLIGHT_CLASS_DEVICE
+	select DRM_KMS_CMA_HELPER
+	select DRM_MIPI_DBI
+	help
+	  DRM driver for the following Ilitek ILI9341 panels:
+	  * YX240QV29-T 2.4" 240x320 TFT (Adafruit 2.4")
+
+	  If M is selected the module will be called panel-ilitek-ili9341.
+
 config DRM_PANEL_ILITEK_ILI9881C
 	tristate "Ilitek ILI9881C-based panels"
 	depends on OF
diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
index 62dae45f8f74..ba4a303c1a66 100644
--- a/drivers/gpu/drm/panel/Makefile
+++ b/drivers/gpu/drm/panel/Makefile
@@ -4,6 +4,7 @@ obj-$(CONFIG_DRM_PANEL_LVDS) += panel-lvds.o
 obj-$(CONFIG_DRM_PANEL_SIMPLE) += panel-simple.o
 obj-$(CONFIG_DRM_PANEL_FEIYANG_FY07024DI26A30D) += panel-feiyang-fy07024di26a30d.o
 obj-$(CONFIG_DRM_PANEL_ILITEK_IL9322) += panel-ilitek-ili9322.o
+obj-$(CONFIG_DRM_PANEL_ILITEK_ILI9341) += panel-ilitek-ili9341.o
 obj-$(CONFIG_DRM_PANEL_ILITEK_ILI9881C) += panel-ilitek-ili9881c.o
 obj-$(CONFIG_DRM_PANEL_INNOLUX_P079ZCA) += panel-innolux-p079zca.o
 obj-$(CONFIG_DRM_PANEL_JDI_LT070ME05000) += panel-jdi-lt070me05000.o
diff --git a/drivers/gpu/drm/tiny/ili9341.c b/drivers/gpu/drm/panel/panel-ilitek-ili9341.c
similarity index 66%
rename from drivers/gpu/drm/tiny/ili9341.c
rename to drivers/gpu/drm/panel/panel-ilitek-ili9341.c
index 33b51dc7faa8..f8df737018d3 100644
--- a/drivers/gpu/drm/tiny/ili9341.c
+++ b/drivers/gpu/drm/panel/panel-ilitek-ili9341.c
@@ -12,17 +12,17 @@
 #include <linux/delay.h>
 #include <linux/gpio/consumer.h>
 #include <linux/module.h>
+#include <linux/of_device.h>
 #include <linux/property.h>
 #include <linux/spi/spi.h>
 
+#include <video/mipi_display.h>
+
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_drv.h>
-#include <drm/drm_fb_helper.h>
 #include <drm/drm_gem_cma_helper.h>
-#include <drm/drm_gem_framebuffer_helper.h>
 #include <drm/drm_mipi_dbi.h>
-#include <drm/drm_modeset_helper.h>
-#include <video/mipi_display.h>
+#include <drm/drm_panel.h>
 
 #define ILI9341_FRMCTR1		0xb1
 #define ILI9341_DISCTRL		0xb6
@@ -49,23 +49,48 @@
 #define ILI9341_MADCTL_MX	BIT(6)
 #define ILI9341_MADCTL_MY	BIT(7)
 
-static void yx240qv29_enable(struct drm_simple_display_pipe *pipe,
-			     struct drm_crtc_state *crtc_state,
-			     struct drm_plane_state *plane_state)
+struct ili9341_config {
+	const struct drm_panel_funcs *funcs;
+	const struct drm_display_mode *mode;
+};
+
+struct ili9341 {
+	struct mipi_dbi_dev dbidev; /* This must be the first entry */
+	struct drm_panel panel;
+	struct backlight_device *backlight;
+	const struct ili9341_config *conf;
+};
+
+static inline struct ili9341 *panel_to_ili9341(struct drm_panel *panel)
 {
-	struct mipi_dbi_dev *dbidev = drm_to_mipi_dbi_dev(pipe->crtc.dev);
+	return container_of(panel, struct ili9341, panel);
+}
+
+static int ili9341_enable(struct drm_panel *panel)
+{
+	struct ili9341 *ili = panel_to_ili9341(panel);
+
+	return backlight_enable(ili->backlight);
+}
+
+static int ili9341_disable(struct drm_panel *panel)
+{
+	struct ili9341 *ili = panel_to_ili9341(panel);
+
+	return backlight_disable(ili->backlight);
+}
+
+static int yx240qv29_prepare(struct drm_panel *panel)
+{
+	struct ili9341 *ili = panel_to_ili9341(panel);
+	struct mipi_dbi_dev *dbidev = &ili->dbidev;
 	struct mipi_dbi *dbi = &dbidev->dbi;
 	u8 addr_mode;
-	int ret, idx;
-
-	if (!drm_dev_enter(pipe->crtc.dev, &idx))
-		return;
-
-	DRM_DEBUG_KMS("\n");
+	int ret;
 
 	ret = mipi_dbi_poweron_conditional_reset(dbidev);
 	if (ret < 0)
-		goto out_exit;
+		return ret;
 	if (ret == 1)
 		goto out_enable;
 
@@ -130,72 +155,54 @@ static void yx240qv29_enable(struct drm_simple_display_pipe *pipe,
 	}
 	addr_mode |= ILI9341_MADCTL_BGR;
 	mipi_dbi_command(dbi, MIPI_DCS_SET_ADDRESS_MODE, addr_mode);
-	mipi_dbi_enable_flush(dbidev, crtc_state, plane_state);
-out_exit:
-	drm_dev_exit(idx);
+
+	return 0;
 }
 
-static const struct drm_simple_display_pipe_funcs ili9341_pipe_funcs = {
-	.enable = yx240qv29_enable,
-	.disable = mipi_dbi_pipe_disable,
-	.update = mipi_dbi_pipe_update,
-	.prepare_fb = drm_gem_fb_simple_display_pipe_prepare_fb,
+static const struct drm_panel_funcs yx240qv29_funcs = {
+	.disable = ili9341_disable,
+	.prepare = yx240qv29_prepare,
+	.enable = ili9341_enable,
 };
 
 static const struct drm_display_mode yx240qv29_mode = {
 	DRM_SIMPLE_MODE(240, 320, 37, 49),
 };
 
-DEFINE_DRM_GEM_CMA_FOPS(ili9341_fops);
-
-static struct drm_driver ili9341_driver = {
-	.driver_features	= DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC,
-	.fops			= &ili9341_fops,
-	.release		= mipi_dbi_release,
-	DRM_GEM_CMA_VMAP_DRIVER_OPS,
-	.debugfs_init		= mipi_dbi_debugfs_init,
-	.name			= "ili9341",
-	.desc			= "Ilitek ILI9341",
-	.date			= "20180514",
-	.major			= 1,
-	.minor			= 0,
+static const struct ili9341_config yx240qv29_data = {
+	.funcs = &yx240qv29_funcs,
+	.mode = &yx240qv29_mode,
 };
 
-static const struct of_device_id ili9341_of_match[] = {
-	{ .compatible = "adafruit,yx240qv29" },
-	{ }
-};
-MODULE_DEVICE_TABLE(of, ili9341_of_match);
-
-static const struct spi_device_id ili9341_id[] = {
-	{ "yx240qv29", 0 },
-	{ }
-};
-MODULE_DEVICE_TABLE(spi, ili9341_id);
+DEFINE_DRM_MIPI_DBI_PANEL_DRIVER(ili9341, "Ilitek ILI9341", "20180514");
 
 static int ili9341_probe(struct spi_device *spi)
 {
+	const struct spi_device_id *spi_id;
 	struct device *dev = &spi->dev;
-	struct mipi_dbi_dev *dbidev;
-	struct drm_device *drm;
 	struct mipi_dbi *dbi;
 	struct gpio_desc *dc;
+	struct ili9341 *ili;
 	u32 rotation = 0;
 	int ret;
 
-	dbidev = kzalloc(sizeof(*dbidev), GFP_KERNEL);
-	if (!dbidev)
+	ili = kzalloc(sizeof(*ili), GFP_KERNEL);
+	if (!ili)
 		return -ENOMEM;
 
-	dbi = &dbidev->dbi;
-	drm = &dbidev->drm;
-	ret = devm_drm_dev_init(dev, drm, &ili9341_driver);
-	if (ret) {
-		kfree(dbidev);
-		return ret;
+	ili->conf = of_device_get_match_data(dev);
+	if (!ili->conf) {
+		spi_id = spi_get_device_id(spi);
+		if (spi_id)
+			ili->conf = (struct ili9341_config *)spi_id->driver_data;
 	}
 
-	drm_mode_config_init(drm);
+	if (!ili->conf)
+		return -ENODEV;
+
+	spi_set_drvdata(spi, ili);
+
+	dbi = &ili->dbidev.dbi;
 
 	dbi->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
 	if (IS_ERR(dbi->reset)) {
@@ -209,9 +216,9 @@ static int ili9341_probe(struct spi_device *spi)
 		return PTR_ERR(dc);
 	}
 
-	dbidev->backlight = devm_of_find_backlight(dev);
-	if (IS_ERR(dbidev->backlight))
-		return PTR_ERR(dbidev->backlight);
+	ili->backlight = devm_of_find_backlight(dev);
+	if (IS_ERR(ili->backlight))
+		return PTR_ERR(ili->backlight);
 
 	device_property_read_u32(dev, "rotation", &rotation);
 
@@ -219,41 +226,46 @@ static int ili9341_probe(struct spi_device *spi)
 	if (ret)
 		return ret;
 
-	ret = mipi_dbi_dev_init(dbidev, &ili9341_pipe_funcs, &yx240qv29_mode, rotation);
-	if (ret)
-		return ret;
+	drm_panel_init(&ili->panel);
+	ili->panel.dev = dev;
+	ili->panel.funcs = ili->conf->funcs;
 
-	drm_mode_config_reset(drm);
-
-	ret = drm_dev_register(drm, 0);
-	if (ret)
-		return ret;
-
-	spi_set_drvdata(spi, drm);
-
-	drm_fbdev_generic_setup(drm, 0);
-
-	return 0;
+	return drm_mipi_dbi_panel_register(&ili->panel, &ili->dbidev, &ili9341_drm_driver,
+					   ili->conf->mode, rotation);
 }
 
 static int ili9341_remove(struct spi_device *spi)
 {
-	struct drm_device *drm = spi_get_drvdata(spi);
+	struct ili9341 *ili = spi_get_drvdata(spi);
 
-	drm_dev_unplug(drm);
-	drm_atomic_helper_shutdown(drm);
+	drm_dev_unplug(&ili->dbidev.drm);
+	drm_atomic_helper_shutdown(&ili->dbidev.drm);
 
 	return 0;
 }
 
 static void ili9341_shutdown(struct spi_device *spi)
 {
-	drm_atomic_helper_shutdown(spi_get_drvdata(spi));
+	struct ili9341 *ili = spi_get_drvdata(spi);
+
+	drm_atomic_helper_shutdown(&ili->dbidev.drm);
 }
 
+static const struct of_device_id ili9341_of_match[] = {
+	{ .compatible = "adafruit,yx240qv29", .data = &yx240qv29_data },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, ili9341_of_match);
+
+static const struct spi_device_id ili9341_id[] = {
+	{ "yx240qv29", (unsigned long)&yx240qv29_data },
+	{ }
+};
+MODULE_DEVICE_TABLE(spi, ili9341_id);
+
 static struct spi_driver ili9341_spi_driver = {
 	.driver = {
-		.name = "ili9341",
+		.name = "panel-ilitek-ili9341",
 		.of_match_table = ili9341_of_match,
 	},
 	.id_table = ili9341_id,
@@ -263,6 +275,6 @@ static struct spi_driver ili9341_spi_driver = {
 };
 module_spi_driver(ili9341_spi_driver);
 
-MODULE_DESCRIPTION("Ilitek ILI9341 DRM driver");
+MODULE_DESCRIPTION("Ilitek ILI9341 panel driver");
 MODULE_AUTHOR("David Lechner <david@lechnology.com>");
 MODULE_LICENSE("GPL");
diff --git a/drivers/gpu/drm/tiny/Kconfig b/drivers/gpu/drm/tiny/Kconfig
index 504763423d46..3e63e93fbeac 100644
--- a/drivers/gpu/drm/tiny/Kconfig
+++ b/drivers/gpu/drm/tiny/Kconfig
@@ -34,19 +34,6 @@ config TINYDRM_ILI9225
 
 	  If M is selected the module will be called ili9225.
 
-config TINYDRM_ILI9341
-	tristate "DRM support for ILI9341 display panels"
-	depends on DRM && SPI
-	select DRM_KMS_HELPER
-	select DRM_KMS_CMA_HELPER
-	select DRM_MIPI_DBI
-	select BACKLIGHT_CLASS_DEVICE
-	help
-	  DRM driver for the following Ilitek ILI9341 panels:
-	  * YX240QV29-T 2.4" 240x320 TFT (Adafruit 2.4")
-
-	  If M is selected the module will be called ili9341.
-
 config TINYDRM_MI0283QT
 	tristate "DRM support for MI0283QT"
 	depends on DRM && SPI
diff --git a/drivers/gpu/drm/tiny/Makefile b/drivers/gpu/drm/tiny/Makefile
index 896cf31132d3..c140962f5c7e 100644
--- a/drivers/gpu/drm/tiny/Makefile
+++ b/drivers/gpu/drm/tiny/Makefile
@@ -3,7 +3,6 @@
 obj-$(CONFIG_DRM_GM12U320)		+= gm12u320.o
 obj-$(CONFIG_TINYDRM_HX8357D)		+= hx8357d.o
 obj-$(CONFIG_TINYDRM_ILI9225)		+= ili9225.o
-obj-$(CONFIG_TINYDRM_ILI9341)		+= ili9341.o
 obj-$(CONFIG_TINYDRM_MI0283QT)		+= mi0283qt.o
 obj-$(CONFIG_TINYDRM_REPAPER)		+= repaper.o
 obj-$(CONFIG_TINYDRM_ST7586)		+= st7586.o
-- 
2.20.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 3/4] drm/tiny/mi0283qt: Move driver to panel-ilitek-ili9341

[Noralf Trønnes]

The MI0283QT panels use a ILI9341 controller so it makes sense to merge
it with the other ili9341 code.

The DRM driver name is ABI, so that is retained.

Cc: David Lechner <david@lechnology.com>
Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
 MAINTAINERS                                  |   9 +-
 drivers/gpu/drm/panel/Kconfig                |   1 +
 drivers/gpu/drm/panel/panel-ilitek-ili9341.c | 161 +++++++++-
 drivers/gpu/drm/tiny/Kconfig                 |  11 -
 drivers/gpu/drm/tiny/Makefile                |   1 -
 drivers/gpu/drm/tiny/mi0283qt.c              | 294 -------------------
 6 files changed, 161 insertions(+), 316 deletions(-)
 delete mode 100644 drivers/gpu/drm/tiny/mi0283qt.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 66b3893a100f..cd7b8bdb3780 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5110,10 +5110,12 @@ F:	Documentation/devicetree/bindings/display/ilitek,ili9225.txt
 
 DRM DRIVER FOR ILITEK ILI9341 PANELS
 M:	David Lechner <david@lechnology.com>
+M:	Noralf Trønnes <noralf@tronnes.org>
 T:	git git://anongit.freedesktop.org/drm/drm-misc
 S:	Maintained
 F:	drivers/gpu/drm/panel/panel-ilitek-ili9341.c
 F:	Documentation/devicetree/bindings/display/ilitek,ili9341.txt
+F:	Documentation/devicetree/bindings/display/multi-inno,mi0283qt.txt
 
 DRM DRIVER FOR HX8357D PANELS
 M:	Eric Anholt <eric@anholt.net>
@@ -5137,13 +5139,6 @@ M:	Dave Airlie <airlied@redhat.com>
 S:	Odd Fixes
 F:	drivers/gpu/drm/mgag200/
 
-DRM DRIVER FOR MI0283QT
-M:	Noralf Trønnes <noralf@tronnes.org>
-T:	git git://anongit.freedesktop.org/drm/drm-misc
-S:	Maintained
-F:	drivers/gpu/drm/tiny/mi0283qt.c
-F:	Documentation/devicetree/bindings/display/multi-inno,mi0283qt.txt
-
 DRM DRIVER FOR MSM ADRENO GPU
 M:	Rob Clark <robdclark@gmail.com>
 M:	Sean Paul <sean@poorly.run>
diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
index a24953ec2d40..fbb5a723eeb5 100644
--- a/drivers/gpu/drm/panel/Kconfig
+++ b/drivers/gpu/drm/panel/Kconfig
@@ -65,6 +65,7 @@ config DRM_PANEL_ILITEK_ILI9341
 	help
 	  DRM driver for the following Ilitek ILI9341 panels:
 	  * YX240QV29-T 2.4" 240x320 TFT (Adafruit 2.4")
+	  * Multi-Inno MI0283QT
 
 	  If M is selected the module will be called panel-ilitek-ili9341.
 
diff --git a/drivers/gpu/drm/panel/panel-ilitek-ili9341.c b/drivers/gpu/drm/panel/panel-ilitek-ili9341.c
index f8df737018d3..f6082fa2a389 100644
--- a/drivers/gpu/drm/panel/panel-ilitek-ili9341.c
+++ b/drivers/gpu/drm/panel/panel-ilitek-ili9341.c
@@ -3,8 +3,6 @@
  * DRM driver for Ilitek ILI9341 panels
  *
  * Copyright 2018 David Lechner <david@lechnology.com>
- *
- * Based on mi0283qt.c:
  * Copyright 2016 Noralf Trønnes
  */
 
@@ -13,7 +11,9 @@
 #include <linux/gpio/consumer.h>
 #include <linux/module.h>
 #include <linux/of_device.h>
+#include <linux/pm.h>
 #include <linux/property.h>
+#include <linux/regulator/consumer.h>
 #include <linux/spi/spi.h>
 
 #include <video/mipi_display.h>
@@ -22,6 +22,7 @@
 #include <drm/drm_drv.h>
 #include <drm/drm_gem_cma_helper.h>
 #include <drm/drm_mipi_dbi.h>
+#include <drm/drm_modeset_helper.h>
 #include <drm/drm_panel.h>
 
 #define ILI9341_FRMCTR1		0xb1
@@ -57,6 +58,7 @@ struct ili9341_config {
 struct ili9341 {
 	struct mipi_dbi_dev dbidev; /* This must be the first entry */
 	struct drm_panel panel;
+	struct regulator *regulator;
 	struct backlight_device *backlight;
 	const struct ili9341_config *conf;
 };
@@ -174,12 +176,133 @@ static const struct ili9341_config yx240qv29_data = {
 	.mode = &yx240qv29_mode,
 };
 
+static int mi0283qt_prepare(struct drm_panel *panel)
+{
+	struct ili9341 *ili = panel_to_ili9341(panel);
+	struct mipi_dbi_dev *dbidev = &ili->dbidev;
+	struct mipi_dbi *dbi = &dbidev->dbi;
+	u8 addr_mode;
+	int ret;
+
+	if (ili->regulator)
+		regulator_enable(ili->regulator);
+
+	ret = mipi_dbi_poweron_conditional_reset(dbidev);
+	if (ret < 0) {
+		if (ili->regulator)
+			regulator_disable(ili->regulator);
+		return ret;
+	}
+	if (ret == 1)
+		goto out_enable;
+
+	mipi_dbi_command(dbi, MIPI_DCS_SET_DISPLAY_OFF);
+
+	mipi_dbi_command(dbi, ILI9341_PWCTRLB, 0x00, 0x83, 0x30);
+	mipi_dbi_command(dbi, ILI9341_PWRSEQ, 0x64, 0x03, 0x12, 0x81);
+	mipi_dbi_command(dbi, ILI9341_DTCTRLA, 0x85, 0x01, 0x79);
+	mipi_dbi_command(dbi, ILI9341_PWCTRLA, 0x39, 0x2c, 0x00, 0x34, 0x02);
+	mipi_dbi_command(dbi, ILI9341_PUMPCTRL, 0x20);
+	mipi_dbi_command(dbi, ILI9341_DTCTRLB, 0x00, 0x00);
+
+	/* Power Control */
+	mipi_dbi_command(dbi, ILI9341_PWCTRL1, 0x26);
+	mipi_dbi_command(dbi, ILI9341_PWCTRL2, 0x11);
+	/* VCOM */
+	mipi_dbi_command(dbi, ILI9341_VMCTRL1, 0x35, 0x3e);
+	mipi_dbi_command(dbi, ILI9341_VMCTRL2, 0xbe);
+
+	/* Memory Access Control */
+	mipi_dbi_command(dbi, MIPI_DCS_SET_PIXEL_FORMAT, MIPI_DCS_PIXEL_FMT_16BIT);
+
+	/* Frame Rate */
+	mipi_dbi_command(dbi, ILI9341_FRMCTR1, 0x00, 0x1b);
+
+	/* Gamma */
+	mipi_dbi_command(dbi, ILI9341_EN3GAM, 0x08);
+	mipi_dbi_command(dbi, MIPI_DCS_SET_GAMMA_CURVE, 0x01);
+	mipi_dbi_command(dbi, ILI9341_PGAMCTRL,
+			 0x1f, 0x1a, 0x18, 0x0a, 0x0f, 0x06, 0x45, 0x87,
+			 0x32, 0x0a, 0x07, 0x02, 0x07, 0x05, 0x00);
+	mipi_dbi_command(dbi, ILI9341_NGAMCTRL,
+			 0x00, 0x25, 0x27, 0x05, 0x10, 0x09, 0x3a, 0x78,
+			 0x4d, 0x05, 0x18, 0x0d, 0x38, 0x3a, 0x1f);
+
+	/* DDRAM */
+	mipi_dbi_command(dbi, ILI9341_ETMOD, 0x07);
+
+	/* Display */
+	mipi_dbi_command(dbi, ILI9341_DISCTRL, 0x0a, 0x82, 0x27, 0x00);
+	mipi_dbi_command(dbi, MIPI_DCS_EXIT_SLEEP_MODE);
+	msleep(100);
+
+	mipi_dbi_command(dbi, MIPI_DCS_SET_DISPLAY_ON);
+	msleep(100);
+
+out_enable:
+	/* The PiTFT (ili9340) has a hardware reset circuit that
+	 * resets only on power-on and not on each reboot through
+	 * a gpio like the rpi-display does.
+	 * As a result, we need to always apply the rotation value
+	 * regardless of the display "on/off" state.
+	 */
+	switch (dbidev->rotation) {
+	default:
+		addr_mode = ILI9341_MADCTL_MV | ILI9341_MADCTL_MY |
+			    ILI9341_MADCTL_MX;
+		break;
+	case 90:
+		addr_mode = ILI9341_MADCTL_MY;
+		break;
+	case 180:
+		addr_mode = ILI9341_MADCTL_MV;
+		break;
+	case 270:
+		addr_mode = ILI9341_MADCTL_MX;
+		break;
+	}
+	addr_mode |= ILI9341_MADCTL_BGR;
+	mipi_dbi_command(dbi, MIPI_DCS_SET_ADDRESS_MODE, addr_mode);
+
+	return 0;
+}
+
+static int mi0283qt_unprepare(struct drm_panel *panel)
+{
+	struct ili9341 *ili = panel_to_ili9341(panel);
+
+	if (ili->regulator)
+		regulator_disable(ili->regulator);
+
+	return 0;
+}
+
+static const struct drm_panel_funcs mi0283qt_drm_funcs = {
+	.prepare = mi0283qt_prepare,
+	.enable = ili9341_enable,
+	.disable = ili9341_disable,
+	.unprepare = mi0283qt_unprepare,
+};
+
+static const struct drm_display_mode mi0283qt_mode = {
+	DRM_SIMPLE_MODE(320, 240, 58, 43),
+};
+
+static const struct ili9341_config mi0283qt_data = {
+	.funcs = &mi0283qt_drm_funcs,
+	.mode = &mi0283qt_mode,
+};
+
+/* Legacy, DRM driver name is ABI */
+DEFINE_DRM_MIPI_DBI_PANEL_DRIVER(mi0283qt, "Multi-Inno MI0283QT", "20160614");
+
 DEFINE_DRM_MIPI_DBI_PANEL_DRIVER(ili9341, "Ilitek ILI9341", "20180514");
 
 static int ili9341_probe(struct spi_device *spi)
 {
 	const struct spi_device_id *spi_id;
 	struct device *dev = &spi->dev;
+	struct drm_driver *driver;
 	struct mipi_dbi *dbi;
 	struct gpio_desc *dc;
 	struct ili9341 *ili;
@@ -216,6 +339,10 @@ static int ili9341_probe(struct spi_device *spi)
 		return PTR_ERR(dc);
 	}
 
+	ili->regulator = devm_regulator_get(dev, "power");
+	if (IS_ERR(ili->regulator))
+		return PTR_ERR(ili->regulator);
+
 	ili->backlight = devm_of_find_backlight(dev);
 	if (IS_ERR(ili->backlight))
 		return PTR_ERR(ili->backlight);
@@ -230,7 +357,12 @@ static int ili9341_probe(struct spi_device *spi)
 	ili->panel.dev = dev;
 	ili->panel.funcs = ili->conf->funcs;
 
-	return drm_mipi_dbi_panel_register(&ili->panel, &ili->dbidev, &ili9341_drm_driver,
+	if (ili->conf == &mi0283qt_data)
+		driver = &mi0283qt_drm_driver;
+	else
+		driver = &ili9341_drm_driver;
+
+	return drm_mipi_dbi_panel_register(&ili->panel, &ili->dbidev, driver,
 					   ili->conf->mode, rotation);
 }
 
@@ -251,14 +383,36 @@ static void ili9341_shutdown(struct spi_device *spi)
 	drm_atomic_helper_shutdown(&ili->dbidev.drm);
 }
 
+static int __maybe_unused ili9341_pm_suspend(struct device *dev)
+{
+	struct ili9341 *ili = dev_get_drvdata(dev);
+
+	return drm_mode_config_helper_suspend(&ili->dbidev.drm);
+}
+
+static int __maybe_unused ili9341_pm_resume(struct device *dev)
+{
+	struct ili9341 *ili = dev_get_drvdata(dev);
+
+	drm_mode_config_helper_resume(&ili->dbidev.drm);
+
+	return 0;
+}
+
+static const struct dev_pm_ops ili9341_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(ili9341_pm_suspend, ili9341_pm_resume)
+};
+
 static const struct of_device_id ili9341_of_match[] = {
 	{ .compatible = "adafruit,yx240qv29", .data = &yx240qv29_data },
+	{ .compatible = "multi-inno,mi0283qt", .data = &mi0283qt_data },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, ili9341_of_match);
 
 static const struct spi_device_id ili9341_id[] = {
 	{ "yx240qv29", (unsigned long)&yx240qv29_data },
+	{ "mi0283qt", (unsigned long)&mi0283qt_data },
 	{ }
 };
 MODULE_DEVICE_TABLE(spi, ili9341_id);
@@ -267,6 +421,7 @@ static struct spi_driver ili9341_spi_driver = {
 	.driver = {
 		.name = "panel-ilitek-ili9341",
 		.of_match_table = ili9341_of_match,
+		.pm = &ili9341_pm_ops,
 	},
 	.id_table = ili9341_id,
 	.probe = ili9341_probe,
diff --git a/drivers/gpu/drm/tiny/Kconfig b/drivers/gpu/drm/tiny/Kconfig
index 3e63e93fbeac..4fae3e50147d 100644
--- a/drivers/gpu/drm/tiny/Kconfig
+++ b/drivers/gpu/drm/tiny/Kconfig
@@ -34,17 +34,6 @@ config TINYDRM_ILI9225
 
 	  If M is selected the module will be called ili9225.
 
-config TINYDRM_MI0283QT
-	tristate "DRM support for MI0283QT"
-	depends on DRM && SPI
-	select DRM_KMS_HELPER
-	select DRM_KMS_CMA_HELPER
-	select DRM_MIPI_DBI
-	select BACKLIGHT_CLASS_DEVICE
-	help
-	  DRM driver for the Multi-Inno MI0283QT display panel
-	  If M is selected the module will be called mi0283qt.
-
 config TINYDRM_REPAPER
 	tristate "DRM support for Pervasive Displays RePaper panels (V231)"
 	depends on DRM && SPI
diff --git a/drivers/gpu/drm/tiny/Makefile b/drivers/gpu/drm/tiny/Makefile
index c140962f5c7e..05029c4b1916 100644
--- a/drivers/gpu/drm/tiny/Makefile
+++ b/drivers/gpu/drm/tiny/Makefile
@@ -3,7 +3,6 @@
 obj-$(CONFIG_DRM_GM12U320)		+= gm12u320.o
 obj-$(CONFIG_TINYDRM_HX8357D)		+= hx8357d.o
 obj-$(CONFIG_TINYDRM_ILI9225)		+= ili9225.o
-obj-$(CONFIG_TINYDRM_MI0283QT)		+= mi0283qt.o
 obj-$(CONFIG_TINYDRM_REPAPER)		+= repaper.o
 obj-$(CONFIG_TINYDRM_ST7586)		+= st7586.o
 obj-$(CONFIG_TINYDRM_ST7735R)		+= st7735r.o
diff --git a/drivers/gpu/drm/tiny/mi0283qt.c b/drivers/gpu/drm/tiny/mi0283qt.c
deleted file mode 100644
index e2cfd9a17143..000000000000
--- a/drivers/gpu/drm/tiny/mi0283qt.c
+++ /dev/null
@@ -1,294 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0-or-later
-/*
- * DRM driver for Multi-Inno MI0283QT panels
- *
- * Copyright 2016 Noralf Trønnes
- */
-
-#include <linux/backlight.h>
-#include <linux/delay.h>
-#include <linux/gpio/consumer.h>
-#include <linux/module.h>
-#include <linux/property.h>
-#include <linux/regulator/consumer.h>
-#include <linux/spi/spi.h>
-
-#include <drm/drm_atomic_helper.h>
-#include <drm/drm_drv.h>
-#include <drm/drm_fb_helper.h>
-#include <drm/drm_gem_cma_helper.h>
-#include <drm/drm_gem_framebuffer_helper.h>
-#include <drm/drm_mipi_dbi.h>
-#include <drm/drm_modeset_helper.h>
-#include <video/mipi_display.h>
-
-#define ILI9341_FRMCTR1		0xb1
-#define ILI9341_DISCTRL		0xb6
-#define ILI9341_ETMOD		0xb7
-
-#define ILI9341_PWCTRL1		0xc0
-#define ILI9341_PWCTRL2		0xc1
-#define ILI9341_VMCTRL1		0xc5
-#define ILI9341_VMCTRL2		0xc7
-#define ILI9341_PWCTRLA		0xcb
-#define ILI9341_PWCTRLB		0xcf
-
-#define ILI9341_PGAMCTRL	0xe0
-#define ILI9341_NGAMCTRL	0xe1
-#define ILI9341_DTCTRLA		0xe8
-#define ILI9341_DTCTRLB		0xea
-#define ILI9341_PWRSEQ		0xed
-
-#define ILI9341_EN3GAM		0xf2
-#define ILI9341_PUMPCTRL	0xf7
-
-#define ILI9341_MADCTL_BGR	BIT(3)
-#define ILI9341_MADCTL_MV	BIT(5)
-#define ILI9341_MADCTL_MX	BIT(6)
-#define ILI9341_MADCTL_MY	BIT(7)
-
-static void mi0283qt_enable(struct drm_simple_display_pipe *pipe,
-			    struct drm_crtc_state *crtc_state,
-			    struct drm_plane_state *plane_state)
-{
-	struct mipi_dbi_dev *dbidev = drm_to_mipi_dbi_dev(pipe->crtc.dev);
-	struct mipi_dbi *dbi = &dbidev->dbi;
-	u8 addr_mode;
-	int ret, idx;
-
-	if (!drm_dev_enter(pipe->crtc.dev, &idx))
-		return;
-
-	DRM_DEBUG_KMS("\n");
-
-	ret = mipi_dbi_poweron_conditional_reset(dbidev);
-	if (ret < 0)
-		goto out_exit;
-	if (ret == 1)
-		goto out_enable;
-
-	mipi_dbi_command(dbi, MIPI_DCS_SET_DISPLAY_OFF);
-
-	mipi_dbi_command(dbi, ILI9341_PWCTRLB, 0x00, 0x83, 0x30);
-	mipi_dbi_command(dbi, ILI9341_PWRSEQ, 0x64, 0x03, 0x12, 0x81);
-	mipi_dbi_command(dbi, ILI9341_DTCTRLA, 0x85, 0x01, 0x79);
-	mipi_dbi_command(dbi, ILI9341_PWCTRLA, 0x39, 0x2c, 0x00, 0x34, 0x02);
-	mipi_dbi_command(dbi, ILI9341_PUMPCTRL, 0x20);
-	mipi_dbi_command(dbi, ILI9341_DTCTRLB, 0x00, 0x00);
-
-	/* Power Control */
-	mipi_dbi_command(dbi, ILI9341_PWCTRL1, 0x26);
-	mipi_dbi_command(dbi, ILI9341_PWCTRL2, 0x11);
-	/* VCOM */
-	mipi_dbi_command(dbi, ILI9341_VMCTRL1, 0x35, 0x3e);
-	mipi_dbi_command(dbi, ILI9341_VMCTRL2, 0xbe);
-
-	/* Memory Access Control */
-	mipi_dbi_command(dbi, MIPI_DCS_SET_PIXEL_FORMAT, MIPI_DCS_PIXEL_FMT_16BIT);
-
-	/* Frame Rate */
-	mipi_dbi_command(dbi, ILI9341_FRMCTR1, 0x00, 0x1b);
-
-	/* Gamma */
-	mipi_dbi_command(dbi, ILI9341_EN3GAM, 0x08);
-	mipi_dbi_command(dbi, MIPI_DCS_SET_GAMMA_CURVE, 0x01);
-	mipi_dbi_command(dbi, ILI9341_PGAMCTRL,
-		       0x1f, 0x1a, 0x18, 0x0a, 0x0f, 0x06, 0x45, 0x87,
-		       0x32, 0x0a, 0x07, 0x02, 0x07, 0x05, 0x00);
-	mipi_dbi_command(dbi, ILI9341_NGAMCTRL,
-		       0x00, 0x25, 0x27, 0x05, 0x10, 0x09, 0x3a, 0x78,
-		       0x4d, 0x05, 0x18, 0x0d, 0x38, 0x3a, 0x1f);
-
-	/* DDRAM */
-	mipi_dbi_command(dbi, ILI9341_ETMOD, 0x07);
-
-	/* Display */
-	mipi_dbi_command(dbi, ILI9341_DISCTRL, 0x0a, 0x82, 0x27, 0x00);
-	mipi_dbi_command(dbi, MIPI_DCS_EXIT_SLEEP_MODE);
-	msleep(100);
-
-	mipi_dbi_command(dbi, MIPI_DCS_SET_DISPLAY_ON);
-	msleep(100);
-
-out_enable:
-	/* The PiTFT (ili9340) has a hardware reset circuit that
-	 * resets only on power-on and not on each reboot through
-	 * a gpio like the rpi-display does.
-	 * As a result, we need to always apply the rotation value
-	 * regardless of the display "on/off" state.
-	 */
-	switch (dbidev->rotation) {
-	default:
-		addr_mode = ILI9341_MADCTL_MV | ILI9341_MADCTL_MY |
-			    ILI9341_MADCTL_MX;
-		break;
-	case 90:
-		addr_mode = ILI9341_MADCTL_MY;
-		break;
-	case 180:
-		addr_mode = ILI9341_MADCTL_MV;
-		break;
-	case 270:
-		addr_mode = ILI9341_MADCTL_MX;
-		break;
-	}
-	addr_mode |= ILI9341_MADCTL_BGR;
-	mipi_dbi_command(dbi, MIPI_DCS_SET_ADDRESS_MODE, addr_mode);
-	mipi_dbi_enable_flush(dbidev, crtc_state, plane_state);
-out_exit:
-	drm_dev_exit(idx);
-}
-
-static const struct drm_simple_display_pipe_funcs mi0283qt_pipe_funcs = {
-	.enable = mi0283qt_enable,
-	.disable = mipi_dbi_pipe_disable,
-	.update = mipi_dbi_pipe_update,
-	.prepare_fb = drm_gem_fb_simple_display_pipe_prepare_fb,
-};
-
-static const struct drm_display_mode mi0283qt_mode = {
-	DRM_SIMPLE_MODE(320, 240, 58, 43),
-};
-
-DEFINE_DRM_GEM_CMA_FOPS(mi0283qt_fops);
-
-static struct drm_driver mi0283qt_driver = {
-	.driver_features	= DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC,
-	.fops			= &mi0283qt_fops,
-	.release		= mipi_dbi_release,
-	DRM_GEM_CMA_VMAP_DRIVER_OPS,
-	.debugfs_init		= mipi_dbi_debugfs_init,
-	.name			= "mi0283qt",
-	.desc			= "Multi-Inno MI0283QT",
-	.date			= "20160614",
-	.major			= 1,
-	.minor			= 0,
-};
-
-static const struct of_device_id mi0283qt_of_match[] = {
-	{ .compatible = "multi-inno,mi0283qt" },
-	{},
-};
-MODULE_DEVICE_TABLE(of, mi0283qt_of_match);
-
-static const struct spi_device_id mi0283qt_id[] = {
-	{ "mi0283qt", 0 },
-	{ },
-};
-MODULE_DEVICE_TABLE(spi, mi0283qt_id);
-
-static int mi0283qt_probe(struct spi_device *spi)
-{
-	struct device *dev = &spi->dev;
-	struct mipi_dbi_dev *dbidev;
-	struct drm_device *drm;
-	struct mipi_dbi *dbi;
-	struct gpio_desc *dc;
-	u32 rotation = 0;
-	int ret;
-
-	dbidev = kzalloc(sizeof(*dbidev), GFP_KERNEL);
-	if (!dbidev)
-		return -ENOMEM;
-
-	dbi = &dbidev->dbi;
-	drm = &dbidev->drm;
-	ret = devm_drm_dev_init(dev, drm, &mi0283qt_driver);
-	if (ret) {
-		kfree(dbidev);
-		return ret;
-	}
-
-	drm_mode_config_init(drm);
-
-	dbi->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
-	if (IS_ERR(dbi->reset)) {
-		DRM_DEV_ERROR(dev, "Failed to get gpio 'reset'\n");
-		return PTR_ERR(dbi->reset);
-	}
-
-	dc = devm_gpiod_get_optional(dev, "dc", GPIOD_OUT_LOW);
-	if (IS_ERR(dc)) {
-		DRM_DEV_ERROR(dev, "Failed to get gpio 'dc'\n");
-		return PTR_ERR(dc);
-	}
-
-	dbidev->regulator = devm_regulator_get(dev, "power");
-	if (IS_ERR(dbidev->regulator))
-		return PTR_ERR(dbidev->regulator);
-
-	dbidev->backlight = devm_of_find_backlight(dev);
-	if (IS_ERR(dbidev->backlight))
-		return PTR_ERR(dbidev->backlight);
-
-	device_property_read_u32(dev, "rotation", &rotation);
-
-	ret = mipi_dbi_spi_init(spi, dbi, dc);
-	if (ret)
-		return ret;
-
-	ret = mipi_dbi_dev_init(dbidev, &mi0283qt_pipe_funcs, &mi0283qt_mode, rotation);
-	if (ret)
-		return ret;
-
-	drm_mode_config_reset(drm);
-
-	ret = drm_dev_register(drm, 0);
-	if (ret)
-		return ret;
-
-	spi_set_drvdata(spi, drm);
-
-	drm_fbdev_generic_setup(drm, 0);
-
-	return 0;
-}
-
-static int mi0283qt_remove(struct spi_device *spi)
-{
-	struct drm_device *drm = spi_get_drvdata(spi);
-
-	drm_dev_unplug(drm);
-	drm_atomic_helper_shutdown(drm);
-
-	return 0;
-}
-
-static void mi0283qt_shutdown(struct spi_device *spi)
-{
-	drm_atomic_helper_shutdown(spi_get_drvdata(spi));
-}
-
-static int __maybe_unused mi0283qt_pm_suspend(struct device *dev)
-{
-	return drm_mode_config_helper_suspend(dev_get_drvdata(dev));
-}
-
-static int __maybe_unused mi0283qt_pm_resume(struct device *dev)
-{
-	drm_mode_config_helper_resume(dev_get_drvdata(dev));
-
-	return 0;
-}
-
-static const struct dev_pm_ops mi0283qt_pm_ops = {
-	SET_SYSTEM_SLEEP_PM_OPS(mi0283qt_pm_suspend, mi0283qt_pm_resume)
-};
-
-static struct spi_driver mi0283qt_spi_driver = {
-	.driver = {
-		.name = "mi0283qt",
-		.owner = THIS_MODULE,
-		.of_match_table = mi0283qt_of_match,
-		.pm = &mi0283qt_pm_ops,
-	},
-	.id_table = mi0283qt_id,
-	.probe = mi0283qt_probe,
-	.remove = mi0283qt_remove,
-	.shutdown = mi0283qt_shutdown,
-};
-module_spi_driver(mi0283qt_spi_driver);
-
-MODULE_DESCRIPTION("Multi-Inno MI0283QT DRM driver");
-MODULE_AUTHOR("Noralf Trønnes");
-MODULE_LICENSE("GPL");
-- 
2.20.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 4/4] drm/panel/ili9341: Support DPI panels

[Noralf Trønnes]

Add support for panels that use the DPI interface.
ILI9341 has onboard RAM so the assumption made here is that all such
panels support pixel upload over DBI.

The presence/absense of the Device Tree 'port' node decides which
interface is used for pixel transfer.

Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
 drivers/gpu/drm/panel/panel-ilitek-ili9341.c | 56 ++++++++++++++++----
 1 file changed, 45 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-ilitek-ili9341.c b/drivers/gpu/drm/panel/panel-ilitek-ili9341.c
index f6082fa2a389..7cbfd739c7fd 100644
--- a/drivers/gpu/drm/panel/panel-ilitek-ili9341.c
+++ b/drivers/gpu/drm/panel/panel-ilitek-ili9341.c
@@ -11,6 +11,7 @@
 #include <linux/gpio/consumer.h>
 #include <linux/module.h>
 #include <linux/of_device.h>
+#include <linux/of_graph.h>
 #include <linux/pm.h>
 #include <linux/property.h>
 #include <linux/regulator/consumer.h>
@@ -53,11 +54,13 @@
 struct ili9341_config {
 	const struct drm_panel_funcs *funcs;
 	const struct drm_display_mode *mode;
+	bool no_dpi;
 };
 
 struct ili9341 {
 	struct mipi_dbi_dev dbidev; /* This must be the first entry */
 	struct drm_panel panel;
+	bool use_dpi;
 	struct regulator *regulator;
 	struct backlight_device *backlight;
 	const struct ili9341_config *conf;
@@ -174,6 +177,7 @@ static const struct drm_display_mode yx240qv29_mode = {
 static const struct ili9341_config yx240qv29_data = {
 	.funcs = &yx240qv29_funcs,
 	.mode = &yx240qv29_mode,
+	.no_dpi = true,
 };
 
 static int mi0283qt_prepare(struct drm_panel *panel)
@@ -291,6 +295,7 @@ static const struct drm_display_mode mi0283qt_mode = {
 static const struct ili9341_config mi0283qt_data = {
 	.funcs = &mi0283qt_drm_funcs,
 	.mode = &mi0283qt_mode,
+	.no_dpi = true,
 };
 
 /* Legacy, DRM driver name is ABI */
@@ -303,6 +308,7 @@ static int ili9341_probe(struct spi_device *spi)
 	const struct spi_device_id *spi_id;
 	struct device *dev = &spi->dev;
 	struct drm_driver *driver;
+	struct device_node *port;
 	struct mipi_dbi *dbi;
 	struct gpio_desc *dc;
 	struct ili9341 *ili;
@@ -357,21 +363,44 @@ static int ili9341_probe(struct spi_device *spi)
 	ili->panel.dev = dev;
 	ili->panel.funcs = ili->conf->funcs;
 
-	if (ili->conf == &mi0283qt_data)
-		driver = &mi0283qt_drm_driver;
-	else
-		driver = &ili9341_drm_driver;
 
-	return drm_mipi_dbi_panel_register(&ili->panel, &ili->dbidev, driver,
-					   ili->conf->mode, rotation);
+	port = of_get_child_by_name(dev->of_node, "port");
+	if (port) {
+		of_node_put(port);
+		ili->use_dpi = true;
+	}
+
+	if (ili->conf->no_dpi)
+		ili->use_dpi = false;
+
+	if (ili->use_dpi) {
+		ret = drm_panel_add(&ili->panel);
+	} else {
+		if (ili->conf == &mi0283qt_data)
+			driver = &mi0283qt_drm_driver;
+		else
+			driver = &ili9341_drm_driver;
+
+		ret = drm_mipi_dbi_panel_register(&ili->panel, &ili->dbidev, driver,
+						  ili->conf->mode, rotation);
+	}
+
+	return ret;
 }
 
 static int ili9341_remove(struct spi_device *spi)
 {
 	struct ili9341 *ili = spi_get_drvdata(spi);
 
-	drm_dev_unplug(&ili->dbidev.drm);
-	drm_atomic_helper_shutdown(&ili->dbidev.drm);
+	if (ili->use_dpi) {
+		drm_panel_remove(&ili->panel);
+		drm_panel_disable(&ili->panel);
+		drm_panel_unprepare(&ili->panel);
+		kfree(ili);
+	} else {
+		drm_dev_unplug(&ili->dbidev.drm);
+		drm_atomic_helper_shutdown(&ili->dbidev.drm);
+	}
 
 	return 0;
 }
@@ -380,21 +409,26 @@ static void ili9341_shutdown(struct spi_device *spi)
 {
 	struct ili9341 *ili = spi_get_drvdata(spi);
 
-	drm_atomic_helper_shutdown(&ili->dbidev.drm);
+	if (!ili->use_dpi)
+		drm_atomic_helper_shutdown(&ili->dbidev.drm);
 }
 
 static int __maybe_unused ili9341_pm_suspend(struct device *dev)
 {
 	struct ili9341 *ili = dev_get_drvdata(dev);
 
-	return drm_mode_config_helper_suspend(&ili->dbidev.drm);
+	if (!ili->use_dpi)
+		return drm_mode_config_helper_suspend(&ili->dbidev.drm);
+
+	return 0;
 }
 
 static int __maybe_unused ili9341_pm_resume(struct device *dev)
 {
 	struct ili9341 *ili = dev_get_drvdata(dev);
 
-	drm_mode_config_helper_resume(&ili->dbidev.drm);
+	if (!ili->use_dpi)
+		drm_mode_config_helper_resume(&ili->dbidev.drm);
 
 	return 0;
 }
-- 
2.20.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [4/4] drm/panel/ili9341: Support DPI panels

[David Lechner]

On 8/1/19 8:52 AM, Noralf Trønnes wrote:
> Add support for panels that use the DPI interface.
> ILI9341 has onboard RAM so the assumption made here is that all such
> panels support pixel upload over DBI.
> 
> The presence/absense of the Device Tree 'port' node decides which
> interface is used for pixel transfer.
> 
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> ---
>   drivers/gpu/drm/panel/panel-ilitek-ili9341.c | 56 ++++++++++++++++----
>   1 file changed, 45 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panel/panel-ilitek-ili9341.c b/drivers/gpu/drm/panel/panel-ilitek-ili9341.c
> index f6082fa2a389..7cbfd739c7fd 100644
> --- a/drivers/gpu/drm/panel/panel-ilitek-ili9341.c
> +++ b/drivers/gpu/drm/panel/panel-ilitek-ili9341.c
> @@ -11,6 +11,7 @@
>   #include <linux/gpio/consumer.h>
>   #include <linux/module.h>
>   #include <linux/of_device.h>
> +#include <linux/of_graph.h>
>   #include <linux/pm.h>
>   #include <linux/property.h>
>   #include <linux/regulator/consumer.h>
> @@ -53,11 +54,13 @@
>   struct ili9341_config {
>   	const struct drm_panel_funcs *funcs;
>   	const struct drm_display_mode *mode;
> +	bool no_dpi;

Can we invert this to use positive logic? i.e. use_dpi instead of
no_dpi.

>   };
>   
>   struct ili9341 {
>   	struct mipi_dbi_dev dbidev; /* This must be the first entry */
>   	struct drm_panel panel;
> +	bool use_dpi;
>   	struct regulator *regulator;
>   	struct backlight_device *backlight;
>   	const struct ili9341_config *conf;
> @@ -174,6 +177,7 @@ static const struct drm_display_mode yx240qv29_mode = {
>   static const struct ili9341_config yx240qv29_data = {
>   	.funcs = &yx240qv29_funcs,
>   	.mode = &yx240qv29_mode,
> +	.no_dpi = true,
>   };
>   
>   static int mi0283qt_prepare(struct drm_panel *panel)
> @@ -291,6 +295,7 @@ static const struct drm_display_mode mi0283qt_mode = {
>   static const struct ili9341_config mi0283qt_data = {
>   	.funcs = &mi0283qt_drm_funcs,
>   	.mode = &mi0283qt_mode,
> +	.no_dpi = true,
>   };
>   
>   /* Legacy, DRM driver name is ABI */
> @@ -303,6 +308,7 @@ static int ili9341_probe(struct spi_device *spi)
>   	const struct spi_device_id *spi_id;
>   	struct device *dev = &spi->dev;
>   	struct drm_driver *driver;
> +	struct device_node *port;
>   	struct mipi_dbi *dbi;
>   	struct gpio_desc *dc;
>   	struct ili9341 *ili;
> @@ -357,21 +363,44 @@ static int ili9341_probe(struct spi_device *spi)
>   	ili->panel.dev = dev;
>   	ili->panel.funcs = ili->conf->funcs;
>   
> -	if (ili->conf == &mi0283qt_data)
> -		driver = &mi0283qt_drm_driver;
> -	else
> -		driver = &ili9341_drm_driver;
>   
> -	return drm_mipi_dbi_panel_register(&ili->panel, &ili->dbidev, driver,
> -					   ili->conf->mode, rotation);
> +	port = of_get_child_by_name(dev->of_node, "port");
> +	if (port) {
> +		of_node_put(port);
> +		ili->use_dpi = true;
> +	}
> +
> +	if (ili->conf->no_dpi)
> +		ili->use_dpi = false;

then this can just be ili->use_dpi = ili->conf->use_dpi.

> +
> +	if (ili->use_dpi) {
> +		ret = drm_panel_add(&ili->panel);
> +	} else {
> +		if (ili->conf == &mi0283qt_data)
> +			driver = &mi0283qt_drm_driver;
> +		else
> +			driver = &ili9341_drm_driver;
> +
> +		ret = drm_mipi_dbi_panel_register(&ili->panel, &ili->dbidev, driver,
> +						  ili->conf->mode, rotation);
> +	}
> +
> +	return ret;
>   }
>   
>   static int ili9341_remove(struct spi_device *spi)
>   {
>   	struct ili9341 *ili = spi_get_drvdata(spi);
>   
> -	drm_dev_unplug(&ili->dbidev.drm);
> -	drm_atomic_helper_shutdown(&ili->dbidev.drm);
> +	if (ili->use_dpi) {
> +		drm_panel_remove(&ili->panel);
> +		drm_panel_disable(&ili->panel);
> +		drm_panel_unprepare(&ili->panel);
> +		kfree(ili);
> +	} else {
> +		drm_dev_unplug(&ili->dbidev.drm);
> +		drm_atomic_helper_shutdown(&ili->dbidev.drm);
> +	}
>   
>   	return 0;
>   }
> @@ -380,21 +409,26 @@ static void ili9341_shutdown(struct spi_device *spi)
>   {
>   	struct ili9341 *ili = spi_get_drvdata(spi);
>   
> -	drm_atomic_helper_shutdown(&ili->dbidev.drm);
> +	if (!ili->use_dpi)
> +		drm_atomic_helper_shutdown(&ili->dbidev.drm);
>   }
>   
>   static int __maybe_unused ili9341_pm_suspend(struct device *dev)
>   {
>   	struct ili9341 *ili = dev_get_drvdata(dev);
>   
> -	return drm_mode_config_helper_suspend(&ili->dbidev.drm);
> +	if (!ili->use_dpi)
> +		return drm_mode_config_helper_suspend(&ili->dbidev.drm);
> +
> +	return 0;
>   }
>   
>   static int __maybe_unused ili9341_pm_resume(struct device *dev)
>   {
>   	struct ili9341 *ili = dev_get_drvdata(dev);
>   
> -	drm_mode_config_helper_resume(&ili->dbidev.drm);
> +	if (!ili->use_dpi)
> +		drm_mode_config_helper_resume(&ili->dbidev.drm);
>   
>   	return 0;
>   }
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/4] drm/tiny/mi0283qt: Move driver to panel-ilitek-ili9341

[David Lechner]

On 8/1/19 8:52 AM, Noralf Trønnes wrote:
> The MI0283QT panels use a ILI9341 controller so it makes sense to merge
> it with the other ili9341 code.
> 
> The DRM driver name is ABI, so that is retained.
> 
> Cc: David Lechner <david@lechnology.com>
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> ---

...

> @@ -216,6 +339,10 @@ static int ili9341_probe(struct spi_device *spi)
>   		return PTR_ERR(dc);
>   	}
>   
> +	ili->regulator = devm_regulator_get(dev, "power");
> +	if (IS_ERR(ili->regulator))
> +		return PTR_ERR(ili->regulator);
> +
>   	ili->backlight = devm_of_find_backlight(dev);
>   	if (IS_ERR(ili->backlight))
>   		return PTR_ERR(ili->backlight);
> @@ -230,7 +357,12 @@ static int ili9341_probe(struct spi_device *spi)
>   	ili->panel.dev = dev;
>   	ili->panel.funcs = ili->conf->funcs;
>   

Should probably add a comment here that this is for backwards
compatibility of the driver name so that no one is tempted to
to add more driver structs when adding new panels.

> -	return drm_mipi_dbi_panel_register(&ili->panel, &ili->dbidev, &ili9341_drm_driver,
> +	if (ili->conf == &mi0283qt_data)
> +		driver = &mi0283qt_drm_driver;
> +	else
> +		driver = &ili9341_drm_driver;
> +
> +	return drm_mipi_dbi_panel_register(&ili->panel, &ili->dbidev, driver,
>   					   ili->conf->mode, rotation);
>   }
>   
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/4] drm/tiny/ili9341: Move driver to drm/panel

[David Lechner]

On 8/1/19 8:52 AM, Noralf Trønnes wrote:
> Move the driver to drm/panel and take advantage of the new panel support
> in drm_mipi_dbi. Change the file name to match the naming standard in
> drm/panel. The DRM driver name is kept since it is ABI.
> 
> Add missing MAINTAINERS entry.
> 
> Cc: David Lechner <david@lechnology.com>
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> ---

Reviewed-by: David Lechner <david@lechnology.com>

Although, I will say that the way the diff came out, it makes it a bit
hard to follow the patch, so more more details in the commit message about
the specific changes could be helpful.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [4/4] drm/panel/ili9341: Support DPI panels

[Noralf Trønnes]

Den 01.08.2019 21.10, skrev David Lechner:
> On 8/1/19 8:52 AM, Noralf Trønnes wrote:
>> Add support for panels that use the DPI interface.
>> ILI9341 has onboard RAM so the assumption made here is that all such
>> panels support pixel upload over DBI.
>>
>> The presence/absense of the Device Tree 'port' node decides which
>> interface is used for pixel transfer.
>>
>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
>> ---
>>   drivers/gpu/drm/panel/panel-ilitek-ili9341.c | 56 ++++++++++++++++----
>>   1 file changed, 45 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/panel/panel-ilitek-ili9341.c
>> b/drivers/gpu/drm/panel/panel-ilitek-ili9341.c
>> index f6082fa2a389..7cbfd739c7fd 100644
>> --- a/drivers/gpu/drm/panel/panel-ilitek-ili9341.c
>> +++ b/drivers/gpu/drm/panel/panel-ilitek-ili9341.c
>> @@ -11,6 +11,7 @@
>>   #include <linux/gpio/consumer.h>
>>   #include <linux/module.h>
>>   #include <linux/of_device.h>
>> +#include <linux/of_graph.h>
>>   #include <linux/pm.h>
>>   #include <linux/property.h>
>>   #include <linux/regulator/consumer.h>
>> @@ -53,11 +54,13 @@
>>   struct ili9341_config {
>>       const struct drm_panel_funcs *funcs;
>>       const struct drm_display_mode *mode;
>> +    bool no_dpi;
> 
> Can we invert this to use positive logic? i.e. use_dpi instead of
> no_dpi.
> 

This is a capability flag. I could call it has_dpi and add a comment
about what it does.

Noralf.

>>   };
>>     struct ili9341 {
>>       struct mipi_dbi_dev dbidev; /* This must be the first entry */
>>       struct drm_panel panel;
>> +    bool use_dpi;
>>       struct regulator *regulator;
>>       struct backlight_device *backlight;
>>       const struct ili9341_config *conf;
>> @@ -174,6 +177,7 @@ static const struct drm_display_mode
>> yx240qv29_mode = {
>>   static const struct ili9341_config yx240qv29_data = {
>>       .funcs = &yx240qv29_funcs,
>>       .mode = &yx240qv29_mode,
>> +    .no_dpi = true,
>>   };
>>     static int mi0283qt_prepare(struct drm_panel *panel)
>> @@ -291,6 +295,7 @@ static const struct drm_display_mode mi0283qt_mode
>> = {
>>   static const struct ili9341_config mi0283qt_data = {
>>       .funcs = &mi0283qt_drm_funcs,
>>       .mode = &mi0283qt_mode,
>> +    .no_dpi = true,
>>   };
>>     /* Legacy, DRM driver name is ABI */
>> @@ -303,6 +308,7 @@ static int ili9341_probe(struct spi_device *spi)
>>       const struct spi_device_id *spi_id;
>>       struct device *dev = &spi->dev;
>>       struct drm_driver *driver;
>> +    struct device_node *port;
>>       struct mipi_dbi *dbi;
>>       struct gpio_desc *dc;
>>       struct ili9341 *ili;
>> @@ -357,21 +363,44 @@ static int ili9341_probe(struct spi_device *spi)
>>       ili->panel.dev = dev;
>>       ili->panel.funcs = ili->conf->funcs;
>>   -    if (ili->conf == &mi0283qt_data)
>> -        driver = &mi0283qt_drm_driver;
>> -    else
>> -        driver = &ili9341_drm_driver;
>>   -    return drm_mipi_dbi_panel_register(&ili->panel, &ili->dbidev,
>> driver,
>> -                       ili->conf->mode, rotation);
>> +    port = of_get_child_by_name(dev->of_node, "port");
>> +    if (port) {
>> +        of_node_put(port);
>> +        ili->use_dpi = true;
>> +    }
>> +
>> +    if (ili->conf->no_dpi)
>> +        ili->use_dpi = false;
> 
> then this can just be ili->use_dpi = ili->conf->use_dpi.
> 
>> +
>> +    if (ili->use_dpi) {
>> +        ret = drm_panel_add(&ili->panel);
>> +    } else {
>> +        if (ili->conf == &mi0283qt_data)
>> +            driver = &mi0283qt_drm_driver;
>> +        else
>> +            driver = &ili9341_drm_driver;
>> +
>> +        ret = drm_mipi_dbi_panel_register(&ili->panel, &ili->dbidev,
>> driver,
>> +                          ili->conf->mode, rotation);
>> +    }
>> +
>> +    return ret;
>>   }
>>     static int ili9341_remove(struct spi_device *spi)
>>   {
>>       struct ili9341 *ili = spi_get_drvdata(spi);
>>   -    drm_dev_unplug(&ili->dbidev.drm);
>> -    drm_atomic_helper_shutdown(&ili->dbidev.drm);
>> +    if (ili->use_dpi) {
>> +        drm_panel_remove(&ili->panel);
>> +        drm_panel_disable(&ili->panel);
>> +        drm_panel_unprepare(&ili->panel);
>> +        kfree(ili);
>> +    } else {
>> +        drm_dev_unplug(&ili->dbidev.drm);
>> +        drm_atomic_helper_shutdown(&ili->dbidev.drm);
>> +    }
>>         return 0;
>>   }
>> @@ -380,21 +409,26 @@ static void ili9341_shutdown(struct spi_device
>> *spi)
>>   {
>>       struct ili9341 *ili = spi_get_drvdata(spi);
>>   -    drm_atomic_helper_shutdown(&ili->dbidev.drm);
>> +    if (!ili->use_dpi)
>> +        drm_atomic_helper_shutdown(&ili->dbidev.drm);
>>   }
>>     static int __maybe_unused ili9341_pm_suspend(struct device *dev)
>>   {
>>       struct ili9341 *ili = dev_get_drvdata(dev);
>>   -    return drm_mode_config_helper_suspend(&ili->dbidev.drm);
>> +    if (!ili->use_dpi)
>> +        return drm_mode_config_helper_suspend(&ili->dbidev.drm);
>> +
>> +    return 0;
>>   }
>>     static int __maybe_unused ili9341_pm_resume(struct device *dev)
>>   {
>>       struct ili9341 *ili = dev_get_drvdata(dev);
>>   -    drm_mode_config_helper_resume(&ili->dbidev.drm);
>> +    if (!ili->use_dpi)
>> +        drm_mode_config_helper_resume(&ili->dbidev.drm);
>>         return 0;
>>   }
>>
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/4] drm/tiny/ili9341: Move driver to drm/panel

[Noralf Trønnes]

Den 01.08.2019 21.43, skrev David Lechner:
> On 8/1/19 8:52 AM, Noralf Trønnes wrote:
>> Move the driver to drm/panel and take advantage of the new panel support
>> in drm_mipi_dbi. Change the file name to match the naming standard in
>> drm/panel. The DRM driver name is kept since it is ABI.
>>
>> Add missing MAINTAINERS entry.
>>
>> Cc: David Lechner <david@lechnology.com>
>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
>> ---
> 
> Reviewed-by: David Lechner <david@lechnology.com>
> 
> Although, I will say that the way the diff came out, it makes it a bit
> hard to follow the patch, so more more details in the commit message about
> the specific changes could be helpful.

Oh, I actually liked how the diff came out, since I found it easy to see
how the drivers differed. But then again, I have have the code fresh in
my brain cache so that might make a difference in how it looks.

I can expand the commit message.

I also see that I have moved the device tables, maybe I should move them
back where they where originally so they are closer to the configs they
refer to.

Noralf.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/4] drm/mipi-dbi: Support command mode panel drivers

[Sam Ravnborg]

Hi Noralf.

I really like how this allows us to have a single file for all
the uses of a driver IC.
And this patch-set is much easier to grasp than the first RFC.

General things:

- The current design have a tight connection between the display
  controller and the panel. Will this hurt in the long run?
  In other words, should we try to add a panel_bridge in-between?
  For now I think this would just make something simple more
  complicated.
  So this note was to say - no I think we should not use panel_bridge.

- drm_panel has proper support for modes.
  This is today duplicated in mipi_dbi.
  Could we make it so that when a panel is used then the panel
  has the mode info - as we then use the panel more in the way we do
  in other cases?
  As it is now the mode is specified in mipi_dbi_dev_init()
  The drm_connector would then, if a panel is used, ask the panel for
  the mode.
  I did not really think to the end of this, but it seems wrong that
  we introduce drm_panel and then keep modes in mipi_dbi.

- For backlight support please move this to drm_panel.
  I have patches that makes it simple to use backlight with drm_panel,
  and this will then benefit from it.
  The drm_panel backlight supports requires that a backlight
  phandle is specified in the DT node of the panel.

Some more specific comments in the following.

	Sam

On Thu, Aug 01, 2019 at 03:52:46PM +0200, Noralf Trønnes wrote:
> This adds a function that registers a DRM driver for use with MIPI DBI
> panels in command mode. That is, framebuffer upload over DBI.
> 
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> ---
>  drivers/gpu/drm/drm_mipi_dbi.c | 92 ++++++++++++++++++++++++++++++++++
>  include/drm/drm_mipi_dbi.h     | 34 +++++++++++++
>  2 files changed, 126 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_mipi_dbi.c b/drivers/gpu/drm/drm_mipi_dbi.c
> index 1961f713aaab..797a20e3a017 100644
> --- a/drivers/gpu/drm/drm_mipi_dbi.c
> +++ b/drivers/gpu/drm/drm_mipi_dbi.c
> @@ -17,11 +17,13 @@
>  #include <drm/drm_damage_helper.h>
>  #include <drm/drm_drv.h>
>  #include <drm/drm_gem_cma_helper.h>
> +#include <drm/drm_fb_helper.h>
>  #include <drm/drm_format_helper.h>
>  #include <drm/drm_fourcc.h>
>  #include <drm/drm_gem_framebuffer_helper.h>
>  #include <drm/drm_mipi_dbi.h>
>  #include <drm/drm_modes.h>
> +#include <drm/drm_panel.h>
>  #include <drm/drm_probe_helper.h>
>  #include <drm/drm_rect.h>
>  #include <drm/drm_vblank.h>
> @@ -597,6 +599,96 @@ void mipi_dbi_release(struct drm_device *drm)
>  }
>  EXPORT_SYMBOL(mipi_dbi_release);
>  
> +static void drm_mipi_dbi_panel_pipe_enable(struct drm_simple_display_pipe *pipe,
> +					   struct drm_crtc_state *crtc_state,
> +					   struct drm_plane_state *plane_state)
> +{
> +	struct mipi_dbi_dev *dbidev = drm_to_mipi_dbi_dev(pipe->crtc.dev);
> +	struct drm_panel *panel = dbidev->panel;
> +	int ret, idx;
> +
> +	if (!drm_dev_enter(pipe->crtc.dev, &idx))
> +		return;
> +
> +	DRM_DEBUG_KMS("\n");
Still usefull?

> +
> +	ret = drm_panel_prepare(panel);
> +	if (ret)
> +		goto out_exit;
> +
> +	mipi_dbi_enable_flush(dbidev, crtc_state, plane_state);
> +
> +	drm_panel_enable(panel);
> +out_exit:
nit - blank line above label?

> +	drm_dev_exit(idx);
> +}
> +
> +static void drm_mipi_dbi_panel_pipe_disable(struct drm_simple_display_pipe *pipe)
> +{
> +	struct mipi_dbi_dev *dbidev = drm_to_mipi_dbi_dev(pipe->crtc.dev);
> +	struct drm_panel *panel = dbidev->panel;
> +
> +	if (!dbidev->enabled)
> +		return;
> +
> +	DRM_DEBUG_KMS("\n");
Still usefull?
> +
> +	dbidev->enabled = false;
> +	drm_panel_disable(panel);
> +	drm_panel_unprepare(panel);
> +}
> +
> +static const struct drm_simple_display_pipe_funcs drm_mipi_dbi_pipe_funcs = {
> +	.enable = drm_mipi_dbi_panel_pipe_enable,
> +	.disable = drm_mipi_dbi_panel_pipe_disable,
> +	.update = mipi_dbi_pipe_update,
> +	.prepare_fb = drm_gem_fb_simple_display_pipe_prepare_fb,
> +};
> +
> +/**
> + * drm_mipi_dbi_panel_register - Register a MIPI DBI DRM driver
> + * @panel: DRM Panel
> + * @dbidev: MIPI DBI device structure to initialize
> + * @mode: Display mode
> + *
> + * This function registeres a DRM driver with @panel attached.
> + *
> + * Returns:
> + * Zero on success, negative error code on failure.
> + */
> +int drm_mipi_dbi_panel_register(struct drm_panel *panel, struct mipi_dbi_dev *dbidev,
> +				struct drm_driver *driver, const struct drm_display_mode *mode,
> +				u32 rotation)
Can we make this use enum drm_panel_orientation - so we can use
of_drm_get_panel_orientation() in the callers?
of_drm_get_panel_orientation() is not merged yet, but we could do so if
this patch-set needs it.

I know that this would require mipi_dbi_dev_init() and all users to be
updated. But it is a simpler interface so worth it.

> +{
> +	struct device *dev = panel->dev;
> +	struct drm_device *drm;
> +	int ret;
> +
> +	dbidev->panel = panel;
> +
> +	drm = &dbidev->drm;
> +	ret = devm_drm_dev_init(dev, drm, driver);
> +	if (ret) {
> +		kfree(dbidev);
> +		return ret;
> +	}
> +
> +	drm_mode_config_init(drm);
> +
> +	ret = mipi_dbi_dev_init(dbidev, &drm_mipi_dbi_pipe_funcs, mode, rotation);
> +
> +	drm_mode_config_reset(drm);
> +
> +	ret = drm_dev_register(drm, 0);
> +	if (ret)
> +		return ret;
> +
> +	drm_fbdev_generic_setup(drm, 16);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_mipi_dbi_panel_register);
> +
>  /**
>   * mipi_dbi_hw_reset - Hardware reset of controller
>   * @dbi: MIPI DBI structure
> diff --git a/include/drm/drm_mipi_dbi.h b/include/drm/drm_mipi_dbi.h
> index 67c66f5ee591..f41ee0d31871 100644
> --- a/include/drm/drm_mipi_dbi.h
> +++ b/include/drm/drm_mipi_dbi.h
> @@ -12,6 +12,7 @@
>  #include <drm/drm_device.h>
>  #include <drm/drm_simple_kms_helper.h>
>  
> +struct drm_panel;
>  struct drm_rect;
>  struct spi_device;
>  struct gpio_desc;
> @@ -123,6 +124,11 @@ struct mipi_dbi_dev {
>  	 * @dbi: MIPI DBI interface
>  	 */
>  	struct mipi_dbi dbi;
> +
> +	/**
> +	 * @panel: Attached DRM panel. See drm_mipi_dbi_panel_register().
> +	 */
> +	struct drm_panel *panel;
>  };
>  
>  static inline struct mipi_dbi_dev *drm_to_mipi_dbi_dev(struct drm_device *drm)
> @@ -140,6 +146,34 @@ int mipi_dbi_dev_init_with_formats(struct mipi_dbi_dev *dbidev,
>  int mipi_dbi_dev_init(struct mipi_dbi_dev *dbidev,
>  		      const struct drm_simple_display_pipe_funcs *funcs,
>  		      const struct drm_display_mode *mode, unsigned int rotation);
> +
> +/**
> + * DEFINE_DRM_MIPI_DBI_PANEL_DRIVER - Define a DRM driver structure
> + * @_name: Name
> + * @_desc: Description
> + * @_date: Date
> + *
> + * This macro defines a &drm_driver for MIPI DBI panel drivers.
> + */
> +#define DEFINE_DRM_MIPI_DBI_PANEL_DRIVER(_name, _desc, _date) \
> +	DEFINE_DRM_GEM_CMA_FOPS(_name ## _fops); \
> +	static struct drm_driver _name ## _drm_driver = { \
> +		.driver_features	= DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC, \
> +		.fops			= & _name ## _fops, \
> +		.release		= mipi_dbi_release, \
> +		DRM_GEM_CMA_VMAP_DRIVER_OPS, \
> +		.debugfs_init		= mipi_dbi_debugfs_init, \
> +		.name			= #_name, \
> +		.desc			= _desc, \
> +		.date			= _date, \
> +		.major			= 1, \
> +		.minor			= 0, \
> +	}
> +
> +int drm_mipi_dbi_panel_register(struct drm_panel *panel, struct mipi_dbi_dev *dbidev,
> +				struct drm_driver *driver, const struct drm_display_mode *mode,
> +				u32 rotation);
> +
>  void mipi_dbi_release(struct drm_device *drm);
>  void mipi_dbi_pipe_update(struct drm_simple_display_pipe *pipe,
>  			  struct drm_plane_state *old_state);
> -- 
> 2.20.1
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/4] drm/tiny/ili9341: Move driver to drm/panel

[Sam Ravnborg]

Hi Noralf.

Most feedback on this driver was covered in comment to 1/4.
Only a few things caught my eye.

On Thu, Aug 01, 2019 at 03:52:47PM +0200, Noralf Trønnes wrote:
> Move the driver to drm/panel and take advantage of the new panel support
> in drm_mipi_dbi. Change the file name to match the naming standard in
> drm/panel. The DRM driver name is kept since it is ABI.
> 
> Add missing MAINTAINERS entry.
> 
> Cc: David Lechner <david@lechnology.com>
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> ---
>  MAINTAINERS                                   |   7 +
>  drivers/gpu/drm/panel/Kconfig                 |  12 ++
>  drivers/gpu/drm/panel/Makefile                |   1 +
>  .../panel-ilitek-ili9341.c}                   | 174 ++++++++++--------
>  drivers/gpu/drm/tiny/Kconfig                  |  13 --
>  drivers/gpu/drm/tiny/Makefile                 |   1 -
>  6 files changed, 113 insertions(+), 95 deletions(-)
>  rename drivers/gpu/drm/{tiny/ili9341.c => panel/panel-ilitek-ili9341.c} (66%)
> 
> +
> +struct ili9341 {
> +	struct mipi_dbi_dev dbidev; /* This must be the first entry */

Can we avoid the need for this to be the first entry?


> -static struct drm_driver ili9341_driver = {
> -	.driver_features	= DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC,
> -	.fops			= &ili9341_fops,
> -	.release		= mipi_dbi_release,
> -	DRM_GEM_CMA_VMAP_DRIVER_OPS,
> -	.debugfs_init		= mipi_dbi_debugfs_init,
> -	.name			= "ili9341",
> -	.desc			= "Ilitek ILI9341",
> -	.date			= "20180514",
> -	.major			= 1,
> -	.minor			= 0,

> +DEFINE_DRM_MIPI_DBI_PANEL_DRIVER(ili9341, "Ilitek ILI9341", "20180514");
Update the date. This is a major change so let it be refelcted in the
date.

	Sam
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/4] drm/panel/ili9341: Support DPI panels

[Sam Ravnborg]

Hi Noralf.

On Thu, Aug 01, 2019 at 03:52:49PM +0200, Noralf Trønnes wrote:
> Add support for panels that use the DPI interface.
> ILI9341 has onboard RAM so the assumption made here is that all such
> panels support pixel upload over DBI.
> 
> The presence/absense of the Device Tree 'port' node decides which
> interface is used for pixel transfer.
Have you consiered if the compatible could be used to determine the use
of a panel?
Then it is more explicit how the HW is described in DT.

	Sam

> 
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> ---
>  drivers/gpu/drm/panel/panel-ilitek-ili9341.c | 56 ++++++++++++++++----
>  1 file changed, 45 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panel/panel-ilitek-ili9341.c b/drivers/gpu/drm/panel/panel-ilitek-ili9341.c
> index f6082fa2a389..7cbfd739c7fd 100644
> --- a/drivers/gpu/drm/panel/panel-ilitek-ili9341.c
> +++ b/drivers/gpu/drm/panel/panel-ilitek-ili9341.c
> @@ -11,6 +11,7 @@
>  #include <linux/gpio/consumer.h>
>  #include <linux/module.h>
>  #include <linux/of_device.h>
> +#include <linux/of_graph.h>
>  #include <linux/pm.h>
>  #include <linux/property.h>
>  #include <linux/regulator/consumer.h>
> @@ -53,11 +54,13 @@
>  struct ili9341_config {
>  	const struct drm_panel_funcs *funcs;
>  	const struct drm_display_mode *mode;
> +	bool no_dpi;
>  };
>  
>  struct ili9341 {
>  	struct mipi_dbi_dev dbidev; /* This must be the first entry */
>  	struct drm_panel panel;
> +	bool use_dpi;
>  	struct regulator *regulator;
>  	struct backlight_device *backlight;
>  	const struct ili9341_config *conf;
> @@ -174,6 +177,7 @@ static const struct drm_display_mode yx240qv29_mode = {
>  static const struct ili9341_config yx240qv29_data = {
>  	.funcs = &yx240qv29_funcs,
>  	.mode = &yx240qv29_mode,
> +	.no_dpi = true,
>  };
>  
>  static int mi0283qt_prepare(struct drm_panel *panel)
> @@ -291,6 +295,7 @@ static const struct drm_display_mode mi0283qt_mode = {
>  static const struct ili9341_config mi0283qt_data = {
>  	.funcs = &mi0283qt_drm_funcs,
>  	.mode = &mi0283qt_mode,
> +	.no_dpi = true,
>  };
>  
>  /* Legacy, DRM driver name is ABI */
> @@ -303,6 +308,7 @@ static int ili9341_probe(struct spi_device *spi)
>  	const struct spi_device_id *spi_id;
>  	struct device *dev = &spi->dev;
>  	struct drm_driver *driver;
> +	struct device_node *port;
>  	struct mipi_dbi *dbi;
>  	struct gpio_desc *dc;
>  	struct ili9341 *ili;
> @@ -357,21 +363,44 @@ static int ili9341_probe(struct spi_device *spi)
>  	ili->panel.dev = dev;
>  	ili->panel.funcs = ili->conf->funcs;
>  
> -	if (ili->conf == &mi0283qt_data)
> -		driver = &mi0283qt_drm_driver;
> -	else
> -		driver = &ili9341_drm_driver;
>  
> -	return drm_mipi_dbi_panel_register(&ili->panel, &ili->dbidev, driver,
> -					   ili->conf->mode, rotation);
> +	port = of_get_child_by_name(dev->of_node, "port");
> +	if (port) {
> +		of_node_put(port);
> +		ili->use_dpi = true;
> +	}
> +
> +	if (ili->conf->no_dpi)
> +		ili->use_dpi = false;
> +
> +	if (ili->use_dpi) {
> +		ret = drm_panel_add(&ili->panel);
> +	} else {
> +		if (ili->conf == &mi0283qt_data)
> +			driver = &mi0283qt_drm_driver;
> +		else
> +			driver = &ili9341_drm_driver;
> +
> +		ret = drm_mipi_dbi_panel_register(&ili->panel, &ili->dbidev, driver,
> +						  ili->conf->mode, rotation);
> +	}
> +
> +	return ret;
>  }
>  
>  static int ili9341_remove(struct spi_device *spi)
>  {
>  	struct ili9341 *ili = spi_get_drvdata(spi);
>  
> -	drm_dev_unplug(&ili->dbidev.drm);
> -	drm_atomic_helper_shutdown(&ili->dbidev.drm);
> +	if (ili->use_dpi) {
> +		drm_panel_remove(&ili->panel);
> +		drm_panel_disable(&ili->panel);
> +		drm_panel_unprepare(&ili->panel);
> +		kfree(ili);
> +	} else {
> +		drm_dev_unplug(&ili->dbidev.drm);
> +		drm_atomic_helper_shutdown(&ili->dbidev.drm);
> +	}
>  
>  	return 0;
>  }
> @@ -380,21 +409,26 @@ static void ili9341_shutdown(struct spi_device *spi)
>  {
>  	struct ili9341 *ili = spi_get_drvdata(spi);
>  
> -	drm_atomic_helper_shutdown(&ili->dbidev.drm);
> +	if (!ili->use_dpi)
> +		drm_atomic_helper_shutdown(&ili->dbidev.drm);
>  }
>  
>  static int __maybe_unused ili9341_pm_suspend(struct device *dev)
>  {
>  	struct ili9341 *ili = dev_get_drvdata(dev);
>  
> -	return drm_mode_config_helper_suspend(&ili->dbidev.drm);
> +	if (!ili->use_dpi)
> +		return drm_mode_config_helper_suspend(&ili->dbidev.drm);
> +
> +	return 0;
>  }
>  
>  static int __maybe_unused ili9341_pm_resume(struct device *dev)
>  {
>  	struct ili9341 *ili = dev_get_drvdata(dev);
>  
> -	drm_mode_config_helper_resume(&ili->dbidev.drm);
> +	if (!ili->use_dpi)
> +		drm_mode_config_helper_resume(&ili->dbidev.drm);
>  
>  	return 0;
>  }
> -- 
> 2.20.1
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/4] drm/panel/ili9341: Support DPI panels

[Sam Ravnborg]

Hi Noralf.

On Thu, Aug 01, 2019 at 03:52:49PM +0200, Noralf Trønnes wrote:
> Add support for panels that use the DPI interface.
> ILI9341 has onboard RAM so the assumption made here is that all such
> panels support pixel upload over DBI.
> 
> The presence/absense of the Device Tree 'port' node decides which
> interface is used for pixel transfer.
> 
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> ---
>  drivers/gpu/drm/panel/panel-ilitek-ili9341.c | 56 ++++++++++++++++----
>  1 file changed, 45 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panel/panel-ilitek-ili9341.c b/drivers/gpu/drm/panel/panel-ilitek-ili9341.c
> index f6082fa2a389..7cbfd739c7fd 100644
> --- a/drivers/gpu/drm/panel/panel-ilitek-ili9341.c
> +++ b/drivers/gpu/drm/panel/panel-ilitek-ili9341.c
> @@ -11,6 +11,7 @@
>  #include <linux/gpio/consumer.h>
>  #include <linux/module.h>
>  #include <linux/of_device.h>
> +#include <linux/of_graph.h>
>  #include <linux/pm.h>
>  #include <linux/property.h>
>  #include <linux/regulator/consumer.h>
> @@ -53,11 +54,13 @@
>  struct ili9341_config {
>  	const struct drm_panel_funcs *funcs;
>  	const struct drm_display_mode *mode;
> +	bool no_dpi;
>  };
>  
>  struct ili9341 {
>  	struct mipi_dbi_dev dbidev; /* This must be the first entry */
>  	struct drm_panel panel;
> +	bool use_dpi;
>  	struct regulator *regulator;
>  	struct backlight_device *backlight;
>  	const struct ili9341_config *conf;
> @@ -174,6 +177,7 @@ static const struct drm_display_mode yx240qv29_mode = {
>  static const struct ili9341_config yx240qv29_data = {
>  	.funcs = &yx240qv29_funcs,
>  	.mode = &yx240qv29_mode,
> +	.no_dpi = true,
>  };
>  
>  static int mi0283qt_prepare(struct drm_panel *panel)
> @@ -291,6 +295,7 @@ static const struct drm_display_mode mi0283qt_mode = {
>  static const struct ili9341_config mi0283qt_data = {
>  	.funcs = &mi0283qt_drm_funcs,
>  	.mode = &mi0283qt_mode,
> +	.no_dpi = true,
>  };
>  
>  /* Legacy, DRM driver name is ABI */
> @@ -303,6 +308,7 @@ static int ili9341_probe(struct spi_device *spi)
>  	const struct spi_device_id *spi_id;
>  	struct device *dev = &spi->dev;
>  	struct drm_driver *driver;
> +	struct device_node *port;
>  	struct mipi_dbi *dbi;
>  	struct gpio_desc *dc;
>  	struct ili9341 *ili;
> @@ -357,21 +363,44 @@ static int ili9341_probe(struct spi_device *spi)
>  	ili->panel.dev = dev;
>  	ili->panel.funcs = ili->conf->funcs;
>  
> -	if (ili->conf == &mi0283qt_data)
> -		driver = &mi0283qt_drm_driver;
> -	else
> -		driver = &ili9341_drm_driver;
>  
> -	return drm_mipi_dbi_panel_register(&ili->panel, &ili->dbidev, driver,
> -					   ili->conf->mode, rotation);
> +	port = of_get_child_by_name(dev->of_node, "port");
> +	if (port) {
> +		of_node_put(port);
> +		ili->use_dpi = true;
> +	}
> +
> +	if (ili->conf->no_dpi)
> +		ili->use_dpi = false;
> +
> +	if (ili->use_dpi) {
> +		ret = drm_panel_add(&ili->panel);
> +	} else {
> +		if (ili->conf == &mi0283qt_data)
> +			driver = &mi0283qt_drm_driver;
> +		else
> +			driver = &ili9341_drm_driver;
> +
> +		ret = drm_mipi_dbi_panel_register(&ili->panel, &ili->dbidev, driver,
> +						  ili->conf->mode, rotation);
> +	}
> +
> +	return ret;
>  }
>  
>  static int ili9341_remove(struct spi_device *spi)
>  {
>  	struct ili9341 *ili = spi_get_drvdata(spi);
>  
> -	drm_dev_unplug(&ili->dbidev.drm);
> -	drm_atomic_helper_shutdown(&ili->dbidev.drm);
> +	if (ili->use_dpi) {
> +		drm_panel_remove(&ili->panel);
> +		drm_panel_disable(&ili->panel);
> +		drm_panel_unprepare(&ili->panel);
> +		kfree(ili);
At first I thought - order is wrong.
But drm_panel_remove() prevents display drivers from using the driver.
And this will not invalidate the other calls.
Maybe add a short comment?

	Sam


> +	} else {
> +		drm_dev_unplug(&ili->dbidev.drm);
> +		drm_atomic_helper_shutdown(&ili->dbidev.drm);
> +	}
>  
>  	return 0;
>  }
> @@ -380,21 +409,26 @@ static void ili9341_shutdown(struct spi_device *spi)
>  {
>  	struct ili9341 *ili = spi_get_drvdata(spi);
>  
> -	drm_atomic_helper_shutdown(&ili->dbidev.drm);
> +	if (!ili->use_dpi)
> +		drm_atomic_helper_shutdown(&ili->dbidev.drm);
>  }
>  
>  static int __maybe_unused ili9341_pm_suspend(struct device *dev)
>  {
>  	struct ili9341 *ili = dev_get_drvdata(dev);
>  
> -	return drm_mode_config_helper_suspend(&ili->dbidev.drm);
> +	if (!ili->use_dpi)
> +		return drm_mode_config_helper_suspend(&ili->dbidev.drm);
> +
> +	return 0;
>  }
>  
>  static int __maybe_unused ili9341_pm_resume(struct device *dev)
>  {
>  	struct ili9341 *ili = dev_get_drvdata(dev);
>  
> -	drm_mode_config_helper_resume(&ili->dbidev.drm);
> +	if (!ili->use_dpi)
> +		drm_mode_config_helper_resume(&ili->dbidev.drm);
>  
>  	return 0;
>  }
> -- 
> 2.20.1
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/4] drm/mipi-dbi: Support command mode panel drivers

[Noralf Trønnes]

Den 11.08.2019 16.16, skrev Sam Ravnborg:
> Hi Noralf.
> 
> I really like how this allows us to have a single file for all
> the uses of a driver IC.
> And this patch-set is much easier to grasp than the first RFC.
> 
> General things:
> 
> - The current design have a tight connection between the display
>   controller and the panel. Will this hurt in the long run?
>   In other words, should we try to add a panel_bridge in-between?
>   For now I think this would just make something simple more
>   complicated.
>   So this note was to say - no I think we should not use panel_bridge.
> 

I did look at panel_bridge but it didn't give me anything I needed, it
would only be a 1:1 passthrough layer.

> - drm_panel has proper support for modes.
>   This is today duplicated in mipi_dbi.
>   Could we make it so that when a panel is used then the panel
>   has the mode info - as we then use the panel more in the way we do
>   in other cases?
>   As it is now the mode is specified in mipi_dbi_dev_init()
>   The drm_connector would then, if a panel is used, ask the panel for
>   the mode.
>   I did not really think to the end of this, but it seems wrong that
>   we introduce drm_panel and then keep modes in mipi_dbi.
> 

I considered that, but it would would just generate duplicate code for
the connector. It would make sense to refactor this when/if all mipi dbi
drivers are turned into panel drivers.

> - For backlight support please move this to drm_panel.
>   I have patches that makes it simple to use backlight with drm_panel,
>   and this will then benefit from it.
>   The drm_panel backlight supports requires that a backlight
>   phandle is specified in the DT node of the panel.
> 

I can fix that when I respin if those patches have landed by then.

> Some more specific comments in the following.
> 
> 	Sam
> 
> On Thu, Aug 01, 2019 at 03:52:46PM +0200, Noralf Trønnes wrote:
>> This adds a function that registers a DRM driver for use with MIPI DBI
>> panels in command mode. That is, framebuffer upload over DBI.
>>
>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
>> ---
>>  drivers/gpu/drm/drm_mipi_dbi.c | 92 ++++++++++++++++++++++++++++++++++
>>  include/drm/drm_mipi_dbi.h     | 34 +++++++++++++
>>  2 files changed, 126 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_mipi_dbi.c b/drivers/gpu/drm/drm_mipi_dbi.c
>> index 1961f713aaab..797a20e3a017 100644
>> --- a/drivers/gpu/drm/drm_mipi_dbi.c
>> +++ b/drivers/gpu/drm/drm_mipi_dbi.c
>> @@ -17,11 +17,13 @@
>>  #include <drm/drm_damage_helper.h>
>>  #include <drm/drm_drv.h>
>>  #include <drm/drm_gem_cma_helper.h>
>> +#include <drm/drm_fb_helper.h>
>>  #include <drm/drm_format_helper.h>
>>  #include <drm/drm_fourcc.h>
>>  #include <drm/drm_gem_framebuffer_helper.h>
>>  #include <drm/drm_mipi_dbi.h>
>>  #include <drm/drm_modes.h>
>> +#include <drm/drm_panel.h>
>>  #include <drm/drm_probe_helper.h>
>>  #include <drm/drm_rect.h>
>>  #include <drm/drm_vblank.h>
>> @@ -597,6 +599,96 @@ void mipi_dbi_release(struct drm_device *drm)
>>  }
>>  EXPORT_SYMBOL(mipi_dbi_release);
>>  
>> +static void drm_mipi_dbi_panel_pipe_enable(struct drm_simple_display_pipe *pipe,
>> +					   struct drm_crtc_state *crtc_state,
>> +					   struct drm_plane_state *plane_state)
>> +{
>> +	struct mipi_dbi_dev *dbidev = drm_to_mipi_dbi_dev(pipe->crtc.dev);
>> +	struct drm_panel *panel = dbidev->panel;
>> +	int ret, idx;
>> +
>> +	if (!drm_dev_enter(pipe->crtc.dev, &idx))
>> +		return;
>> +
>> +	DRM_DEBUG_KMS("\n");
> Still usefull?
> 

No I think this can go (it was in the code I copied from the driver).

>> +
>> +	ret = drm_panel_prepare(panel);
>> +	if (ret)
>> +		goto out_exit;
>> +
>> +	mipi_dbi_enable_flush(dbidev, crtc_state, plane_state);
>> +
>> +	drm_panel_enable(panel);
>> +out_exit:
> nit - blank line above label?
> 
>> +	drm_dev_exit(idx);
>> +}
>> +
>> +static void drm_mipi_dbi_panel_pipe_disable(struct drm_simple_display_pipe *pipe)
>> +{
>> +	struct mipi_dbi_dev *dbidev = drm_to_mipi_dbi_dev(pipe->crtc.dev);
>> +	struct drm_panel *panel = dbidev->panel;
>> +
>> +	if (!dbidev->enabled)
>> +		return;
>> +
>> +	DRM_DEBUG_KMS("\n");
> Still usefull?
>> +
>> +	dbidev->enabled = false;
>> +	drm_panel_disable(panel);
>> +	drm_panel_unprepare(panel);
>> +}
>> +
>> +static const struct drm_simple_display_pipe_funcs drm_mipi_dbi_pipe_funcs = {
>> +	.enable = drm_mipi_dbi_panel_pipe_enable,
>> +	.disable = drm_mipi_dbi_panel_pipe_disable,
>> +	.update = mipi_dbi_pipe_update,
>> +	.prepare_fb = drm_gem_fb_simple_display_pipe_prepare_fb,
>> +};
>> +
>> +/**
>> + * drm_mipi_dbi_panel_register - Register a MIPI DBI DRM driver
>> + * @panel: DRM Panel
>> + * @dbidev: MIPI DBI device structure to initialize
>> + * @mode: Display mode
>> + *
>> + * This function registeres a DRM driver with @panel attached.
>> + *
>> + * Returns:
>> + * Zero on success, negative error code on failure.
>> + */
>> +int drm_mipi_dbi_panel_register(struct drm_panel *panel, struct mipi_dbi_dev *dbidev,
>> +				struct drm_driver *driver, const struct drm_display_mode *mode,
>> +				u32 rotation)
> Can we make this use enum drm_panel_orientation - so we can use
> of_drm_get_panel_orientation() in the callers?
> of_drm_get_panel_orientation() is not merged yet, but we could do so if
> this patch-set needs it.
> 
> I know that this would require mipi_dbi_dev_init() and all users to be
> updated. But it is a simpler interface so worth it.
> 

That would break rotation on userspace that doesn't know about the panel
orientation property which is a recent addition. These panels are mostly
used in the embedded world not desktop. It also would break fbdev 90/270
rotation (see drm_client_rotation()).

Noralf.

>> +{
>> +	struct device *dev = panel->dev;
>> +	struct drm_device *drm;
>> +	int ret;
>> +
>> +	dbidev->panel = panel;
>> +
>> +	drm = &dbidev->drm;
>> +	ret = devm_drm_dev_init(dev, drm, driver);
>> +	if (ret) {
>> +		kfree(dbidev);
>> +		return ret;
>> +	}
>> +
>> +	drm_mode_config_init(drm);
>> +
>> +	ret = mipi_dbi_dev_init(dbidev, &drm_mipi_dbi_pipe_funcs, mode, rotation);
>> +
>> +	drm_mode_config_reset(drm);
>> +
>> +	ret = drm_dev_register(drm, 0);
>> +	if (ret)
>> +		return ret;
>> +
>> +	drm_fbdev_generic_setup(drm, 16);
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL(drm_mipi_dbi_panel_register);
>> +
>>  /**
>>   * mipi_dbi_hw_reset - Hardware reset of controller
>>   * @dbi: MIPI DBI structure
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/4] drm/tiny/ili9341: Move driver to drm/panel

[Noralf Trønnes]

Den 11.08.2019 17.24, skrev Sam Ravnborg:
> Hi Noralf.
> 
> Most feedback on this driver was covered in comment to 1/4.
> Only a few things caught my eye.
> 
> On Thu, Aug 01, 2019 at 03:52:47PM +0200, Noralf Trønnes wrote:
>> Move the driver to drm/panel and take advantage of the new panel support
>> in drm_mipi_dbi. Change the file name to match the naming standard in
>> drm/panel. The DRM driver name is kept since it is ABI.
>>
>> Add missing MAINTAINERS entry.
>>
>> Cc: David Lechner <david@lechnology.com>
>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
>> ---
>>  MAINTAINERS                                   |   7 +
>>  drivers/gpu/drm/panel/Kconfig                 |  12 ++
>>  drivers/gpu/drm/panel/Makefile                |   1 +
>>  .../panel-ilitek-ili9341.c}                   | 174 ++++++++++--------
>>  drivers/gpu/drm/tiny/Kconfig                  |  13 --
>>  drivers/gpu/drm/tiny/Makefile                 |   1 -
>>  6 files changed, 113 insertions(+), 95 deletions(-)
>>  rename drivers/gpu/drm/{tiny/ili9341.c => panel/panel-ilitek-ili9341.c} (66%)
>>
>> +
>> +struct ili9341 {
>> +	struct mipi_dbi_dev dbidev; /* This must be the first entry */
> 
> Can we avoid the need for this to be the first entry?
> 

That would require this driver to have a custom drm_driver->release and
pass that into the DEFINE_DRM_MIPI_DBI_PANEL_DRIVER macro.

Having it as the first entry, mipi_dbi_release() will suffice.
Simplifying things.

> 
>> -static struct drm_driver ili9341_driver = {
>> -	.driver_features	= DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC,
>> -	.fops			= &ili9341_fops,
>> -	.release		= mipi_dbi_release,
>> -	DRM_GEM_CMA_VMAP_DRIVER_OPS,
>> -	.debugfs_init		= mipi_dbi_debugfs_init,
>> -	.name			= "ili9341",
>> -	.desc			= "Ilitek ILI9341",
>> -	.date			= "20180514",
>> -	.major			= 1,
>> -	.minor			= 0,
> 
>> +DEFINE_DRM_MIPI_DBI_PANEL_DRIVER(ili9341, "Ilitek ILI9341", "20180514");
> Update the date. This is a major change so let it be refelcted in the
> date.
> 

I guess that makes sense. I don't know what this date field is used for
though.

Noralf.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/4] drm/panel/ili9341: Support DPI panels

[Noralf Trønnes]

Den 11.08.2019 18.41, skrev Sam Ravnborg:
> Hi Noralf.
> 
> On Thu, Aug 01, 2019 at 03:52:49PM +0200, Noralf Trønnes wrote:
>> Add support for panels that use the DPI interface.
>> ILI9341 has onboard RAM so the assumption made here is that all such
>> panels support pixel upload over DBI.
>>
>> The presence/absense of the Device Tree 'port' node decides which
>> interface is used for pixel transfer.
> Have you consiered if the compatible could be used to determine the use
> of a panel?
> Then it is more explicit how the HW is described in DT.
> 

Is that common to use the compatible to tell which interface to use?
I don't know what best practice is here.

Noralf.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/4] drm/panel/ili9341: Support DPI panels

[Noralf Trønnes]

Den 11.08.2019 19.02, skrev Sam Ravnborg:
> Hi Noralf.
> 
> On Thu, Aug 01, 2019 at 03:52:49PM +0200, Noralf Trønnes wrote:
>> Add support for panels that use the DPI interface.
>> ILI9341 has onboard RAM so the assumption made here is that all such
>> panels support pixel upload over DBI.
>>
>> The presence/absense of the Device Tree 'port' node decides which
>> interface is used for pixel transfer.
>>
>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
>> ---
>>  drivers/gpu/drm/panel/panel-ilitek-ili9341.c | 56 ++++++++++++++++----
>>  1 file changed, 45 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/panel/panel-ilitek-ili9341.c b/drivers/gpu/drm/panel/panel-ilitek-ili9341.c

<snip>

>>  static int ili9341_remove(struct spi_device *spi)
>>  {
>>  	struct ili9341 *ili = spi_get_drvdata(spi);
>>  
>> -	drm_dev_unplug(&ili->dbidev.drm);
>> -	drm_atomic_helper_shutdown(&ili->dbidev.drm);
>> +	if (ili->use_dpi) {
>> +		drm_panel_remove(&ili->panel);
>> +		drm_panel_disable(&ili->panel);
>> +		drm_panel_unprepare(&ili->panel);
>> +		kfree(ili);
> At first I thought - order is wrong.
> But drm_panel_remove() prevents display drivers from using the driver.
> And this will not invalidate the other calls.
> Maybe add a short comment?
> 

I just copied this code from Josef's driver, didn't actually look that
close at it. Isn't there a common pattern for this in the panel drivers?
I would assume that everyone would have to do more or less the same on
driver unbind.

Noralf.

> 	Sam
> 
> 
>> +	} else {
>> +		drm_dev_unplug(&ili->dbidev.drm);
>> +		drm_atomic_helper_shutdown(&ili->dbidev.drm);
>> +	}
>>  
>>  	return 0;
>>  }
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/4] drm/panel/ili9341: Support DPI panels

[Laurent Pinchart]

On Mon, Aug 12, 2019 at 02:13:54PM +0200, Noralf Trønnes wrote:
> Den 11.08.2019 18.41, skrev Sam Ravnborg:
> > On Thu, Aug 01, 2019 at 03:52:49PM +0200, Noralf Trønnes wrote:
> >> Add support for panels that use the DPI interface.
> >> ILI9341 has onboard RAM so the assumption made here is that all such
> >> panels support pixel upload over DBI.
> >>
> >> The presence/absense of the Device Tree 'port' node decides which
> >> interface is used for pixel transfer.
> >
> > Have you consiered if the compatible could be used to determine the use
> > of a panel? Then it is more explicit how the HW is described in DT.
> 
> Is that common to use the compatible to tell which interface to use?
> I don't know what best practice is here.

Usually the compatible identifies the device, not the interface.
Additional properties are preferred to describe the interface.

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/4] drm/panel/ili9341: Support DPI panels

[Sam Ravnborg]

Hi Laurent/Noralf.

On Mon, Aug 12, 2019 at 06:35:42PM +0300, Laurent Pinchart wrote:
> On Mon, Aug 12, 2019 at 02:13:54PM +0200, Noralf Trønnes wrote:
> > Den 11.08.2019 18.41, skrev Sam Ravnborg:
> > > On Thu, Aug 01, 2019 at 03:52:49PM +0200, Noralf Trønnes wrote:
> > >> Add support for panels that use the DPI interface.
> > >> ILI9341 has onboard RAM so the assumption made here is that all such
> > >> panels support pixel upload over DBI.
> > >>
> > >> The presence/absense of the Device Tree 'port' node decides which
> > >> interface is used for pixel transfer.
> > >
> > > Have you consiered if the compatible could be used to determine the use
> > > of a panel? Then it is more explicit how the HW is described in DT.
> > 
> > Is that common to use the compatible to tell which interface to use?
> > I don't know what best practice is here.
> 
> Usually the compatible identifies the device, not the interface.
> Additional properties are preferred to describe the interface.
Thanks Laurent.
Then the ports idea as implmented by the patch seems to fly.

	Sam
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/4] drm/mipi-dbi: Support command mode panel drivers

[Sam Ravnborg]

Hi Noralf.

> > - drm_panel has proper support for modes.
> >   This is today duplicated in mipi_dbi.
> >   Could we make it so that when a panel is used then the panel
> >   has the mode info - as we then use the panel more in the way we do
> >   in other cases?
> >   As it is now the mode is specified in mipi_dbi_dev_init()
> >   The drm_connector would then, if a panel is used, ask the panel for
> >   the mode.
> >   I did not really think to the end of this, but it seems wrong that
> >   we introduce drm_panel and then keep modes in mipi_dbi.
> > 
> 
> I considered that, but it would would just generate duplicate code for
> the connector. It would make sense to refactor this when/if all mipi dbi
> drivers are turned into panel drivers.

The objective should be that all mipi dbi drivers could be refactored.
And so it makes sense to do it right from the beginning.
It will be some duplicated code until all are migrated.
But as the number of mipi dbi drivers are low it is doable if a few
people throw some time after it.
I volunteer to assist.

In drm_mipi_dbi.c we could just add:

static int mipi_dbi_connector_get_modes(struct drm_connector *connector)
{
	...
        if (dbidev->panel)
                return drm_panel_get_modes(dbidev->panel);


Then if there is a panel we would use the mode specified by the panel.
To make this work we would need a drm_panel_attach() in
drm_mipi_dbi_panel_register() to give the panel access to the connector.
I have patches that makes connector an argument to drm_panel_get_modes()
but they need a bit more baking, so you cannot benefit from them yet.

Maybe this is the same argument as backlight?
We can introduce this when the drm_panel core is better prepared.

> >> +int drm_mipi_dbi_panel_register(struct drm_panel *panel, struct mipi_dbi_dev *dbidev,
> >> +				struct drm_driver *driver, const struct drm_display_mode *mode,
> >> +				u32 rotation)
> > Can we make this use enum drm_panel_orientation - so we can use
> > of_drm_get_panel_orientation() in the callers?
> > of_drm_get_panel_orientation() is not merged yet, but we could do so if
> > this patch-set needs it.
> > 
> > I know that this would require mipi_dbi_dev_init() and all users to be
> > updated. But it is a simpler interface so worth it.
> > 
> 
> That would break rotation on userspace that doesn't know about the panel
> orientation property which is a recent addition. These panels are mostly
> used in the embedded world not desktop. It also would break fbdev 90/270
> rotation (see drm_client_rotation()).

I think it is possible to move most of drm over to one way to spicify
rotation.
But let's wait with that battle until another day.
It should not hinder this series.

	Sam
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/4] drm/mipi-dbi: Support command mode panel drivers

[Noralf Trønnes]

Den 12.08.2019 20.49, skrev Sam Ravnborg:
> Hi Noralf.
> 
>>> - drm_panel has proper support for modes.
>>>   This is today duplicated in mipi_dbi.
>>>   Could we make it so that when a panel is used then the panel
>>>   has the mode info - as we then use the panel more in the way we do
>>>   in other cases?
>>>   As it is now the mode is specified in mipi_dbi_dev_init()
>>>   The drm_connector would then, if a panel is used, ask the panel for
>>>   the mode.
>>>   I did not really think to the end of this, but it seems wrong that
>>>   we introduce drm_panel and then keep modes in mipi_dbi.
>>>
>>
>> I considered that, but it would would just generate duplicate code for
>> the connector. It would make sense to refactor this when/if all mipi dbi
>> drivers are turned into panel drivers.
> 
> The objective should be that all mipi dbi drivers could be refactored.

Not all can be converted, easily at least. ili9225 and st7586 are not
true MIPI DBI controllers, they are just similar enough to benefit from
the helper.

> And so it makes sense to do it right from the beginning.
> It will be some duplicated code until all are migrated.
> But as the number of mipi dbi drivers are low it is doable if a few
> people throw some time after it.
> I volunteer to assist.
> 
> In drm_mipi_dbi.c we could just add:
> 
> static int mipi_dbi_connector_get_modes(struct drm_connector *connector)
> {
> 	...
>         if (dbidev->panel)
>                 return drm_panel_get_modes(dbidev->panel);
> 
> 

This would make sense if the display mode was used as-is in the panel
driver. But the mode can be rotated so that would neeed to be taken care
of somehow.

I have started to work on a driver now so I can spend time on complex
refactorings. I'm not against changing this, but maybe a change like
this should happen after drivers have been converted and the picture is
more clear.

And Thierry hasn't voiced his opinion on this either, so no point in
spending more time on this if he disagrees with this "panel driver as
full drm driver" move.

Noralf.

> Then if there is a panel we would use the mode specified by the panel.
> To make this work we would need a drm_panel_attach() in
> drm_mipi_dbi_panel_register() to give the panel access to the connector.
> I have patches that makes connector an argument to drm_panel_get_modes()
> but they need a bit more baking, so you cannot benefit from them yet.
> 
> Maybe this is the same argument as backlight?
> We can introduce this when the drm_panel core is better prepared.
> 
>>>> +int drm_mipi_dbi_panel_register(struct drm_panel *panel, struct mipi_dbi_dev *dbidev,
>>>> +				struct drm_driver *driver, const struct drm_display_mode *mode,
>>>> +				u32 rotation)
>>> Can we make this use enum drm_panel_orientation - so we can use
>>> of_drm_get_panel_orientation() in the callers?
>>> of_drm_get_panel_orientation() is not merged yet, but we could do so if
>>> this patch-set needs it.
>>>
>>> I know that this would require mipi_dbi_dev_init() and all users to be
>>> updated. But it is a simpler interface so worth it.
>>>
>>
>> That would break rotation on userspace that doesn't know about the panel
>> orientation property which is a recent addition. These panels are mostly
>> used in the embedded world not desktop. It also would break fbdev 90/270
>> rotation (see drm_client_rotation()).
> 
> I think it is possible to move most of drm over to one way to spicify
> rotation.
> But let's wait with that battle until another day.
> It should not hinder this series.
> 
> 	Sam
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel