diff mbox series

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

Message ID 20190801135249.28803-3-noralf@tronnes.org (mailing list archive)
State New, archived
Headers show
Series drm/mipi-dbi: Support panel drivers | expand

Commit Message

Noralf Trønnes Aug. 1, 2019, 1:52 p.m. UTC
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%)

Comments

David Lechner Aug. 1, 2019, 7:43 p.m. UTC | #1
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.
Noralf Trønnes Aug. 2, 2019, 2:19 p.m. UTC | #2
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.
Sam Ravnborg Aug. 11, 2019, 3:24 p.m. UTC | #3
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
Noralf Trønnes Aug. 12, 2019, 12:11 p.m. UTC | #4
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.
diff mbox series

Patch

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