| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <d0de831b-a7a3-eab3-2508-da50c346edae@tronnes.org>
Date: Sat, 19 May 2018 16:00:06 +0200
From: Noralf Trønnes <noralf@...nnes.org>
To: Andy Shevchenko <andy.shevchenko@...il.com>,
David Lechner <david@...hnology.com>
Cc: dri-devel@...ts.freedesktop.org,
devicetree <devicetree@...r.kernel.org>,
Rob Herring <robh+dt@...nel.org>,
Mark Rutland <mark.rutland@....com>, limor@...yada.net,
Nitin Patil <nitin@...dsensors.com>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 3/3] drm/tinydrm: new driver for ILI9341 display panels
Den 15.05.2018 23.43, skrev Andy Shevchenko:
> On Tue, May 15, 2018 at 4:43 AM, David Lechner <david@...hnology.com> wrote:
>> This adds a new driver for display panels that use the Ilitek ILI9341
>> controller. It currently supports a single display panel, namely
>> the YX240QV29-T (e.g. Adafruit 2.4" TFT).
>>
>> The init sequence is from the Adafruit Python library for the ILI9341
>> controller. https://github.com/adafruit/Adafruit_Python_ILI9341
> Some minor style nitpicks, otherwise LGTM
>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@...il.com>
>
>> Signed-off-by: David Lechner <david@...hnology.com>
>> ---
>> MAINTAINERS | 6 +
>> drivers/gpu/drm/tinydrm/Kconfig | 10 ++
>> drivers/gpu/drm/tinydrm/Makefile | 1 +
>> drivers/gpu/drm/tinydrm/ili9341.c | 239 ++++++++++++++++++++++++++++++
>> 4 files changed, 256 insertions(+)
>> create mode 100644 drivers/gpu/drm/tinydrm/ili9341.c
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index bc219de9cbee..ffa099abbd79 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -4480,6 +4480,12 @@ S: Maintained
>> F: drivers/gpu/drm/tinydrm/ili9225.c
>> F: Documentation/devicetree/bindings/display/ilitek,ili9225.txt
>>
>> +DRM DRIVER FOR ILITEK ILI9341 PANELS
>> +M: David Lechner <david@...hnology.com>
>> +S: Maintained
>> +F: drivers/gpu/drm/tinydrm/ili9341.c
>> +F: Documentation/devicetree/bindings/display/ilitek,ili9341.txt
>> +
>> DRM DRIVER FOR INTEL I810 VIDEO CARDS
>> S: Orphan / Obsolete
>> F: drivers/gpu/drm/i810/
>> diff --git a/drivers/gpu/drm/tinydrm/Kconfig b/drivers/gpu/drm/tinydrm/Kconfig
>> index 4592a5e3f20b..7a8008b0783f 100644
>> --- a/drivers/gpu/drm/tinydrm/Kconfig
>> +++ b/drivers/gpu/drm/tinydrm/Kconfig
>> @@ -20,6 +20,16 @@ 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_TINYDRM && SPI
> Can't we do something like
>
> if SPI
>
> ...
>
> endif
>
> ?
>
>> + select TINYDRM_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 ili9341.
>> +
>> config TINYDRM_MI0283QT
>> tristate "DRM support for MI0283QT"
>> depends on DRM_TINYDRM && SPI
>> diff --git a/drivers/gpu/drm/tinydrm/Makefile b/drivers/gpu/drm/tinydrm/Makefile
>> index 49a111929724..14d99080665a 100644
>> --- a/drivers/gpu/drm/tinydrm/Makefile
>> +++ b/drivers/gpu/drm/tinydrm/Makefile
>> @@ -5,6 +5,7 @@ obj-$(CONFIG_TINYDRM_MIPI_DBI) += mipi-dbi.o
>>
>> # Displays
>> 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
>> diff --git a/drivers/gpu/drm/tinydrm/ili9341.c b/drivers/gpu/drm/tinydrm/ili9341.c
>> new file mode 100644
>> index 000000000000..2ce4244a68c3
>> --- /dev/null
>> +++ b/drivers/gpu/drm/tinydrm/ili9341.c
>> @@ -0,0 +1,239 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/*
>> + * DRM driver for Ilitek ILI9341 panels
>> + *
>> + * Copyright 2018 David Lechner <david@...hnology.com>
>> + *
>> + * Based on mi0283qt.c:
>> + * 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_fb_helper.h>
>> +#include <drm/drm_modeset_helper.h>
>> +#include <drm/drm_gem_framebuffer_helper.h>
> Can it be in order?
>
>> +#include <drm/tinydrm/mipi-dbi.h>
>> +#include <drm/tinydrm/tinydrm-helpers.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 yx240qv29_enable(struct drm_simple_display_pipe *pipe,
>> + struct drm_crtc_state *crtc_state,
>> + struct drm_plane_state *plane_state)
>> +{
>> + struct tinydrm_device *tdev = pipe_to_tinydrm(pipe);
>> + struct mipi_dbi *mipi = mipi_dbi_from_tinydrm(tdev);
>> + u8 addr_mode;
>> + int ret;
>> +
>> + DRM_DEBUG_KMS("\n");
>> +
>> + ret = mipi_dbi_poweron_conditional_reset(mipi);
>> + if (ret < 0)
>> + return;
>> + if (ret == 1)
>> + goto out_enable;
>> +
>> + mipi_dbi_command(mipi, MIPI_DCS_SET_DISPLAY_OFF);
>> +
>> + mipi_dbi_command(mipi, ILI9341_PWCTRLB, 0x00, 0xc1, 0x30);
>> + mipi_dbi_command(mipi, ILI9341_PWRSEQ, 0x64, 0x03, 0x12, 0x81);
>> + mipi_dbi_command(mipi, ILI9341_DTCTRLA, 0x85, 0x00, 0x78);
>> + mipi_dbi_command(mipi, ILI9341_PWCTRLA, 0x39, 0x2c, 0x00, 0x34, 0x02);
>> + mipi_dbi_command(mipi, ILI9341_PUMPCTRL, 0x20);
>> + mipi_dbi_command(mipi, ILI9341_DTCTRLB, 0x00, 0x00);
>> +
>> + /* Power Control */
>> + mipi_dbi_command(mipi, ILI9341_PWCTRL1, 0x23);
>> + mipi_dbi_command(mipi, ILI9341_PWCTRL2, 0x10);
>> + /* VCOM */
>> + mipi_dbi_command(mipi, ILI9341_VMCTRL1, 0x3e, 0x28);
>> + mipi_dbi_command(mipi, ILI9341_VMCTRL2, 0x86);
>> +
>> + /* Memory Access Control */
>> + mipi_dbi_command(mipi, MIPI_DCS_SET_PIXEL_FORMAT, MIPI_DCS_PIXEL_FMT_16BIT);
>> +
>> + /* Frame Rate */
>> + mipi_dbi_command(mipi, ILI9341_FRMCTR1, 0x00, 0x1b);
>> +
>> + /* Gamma */
>> + mipi_dbi_command(mipi, ILI9341_EN3GAM, 0x00);
>> + mipi_dbi_command(mipi, MIPI_DCS_SET_GAMMA_CURVE, 0x01);
>> + mipi_dbi_command(mipi, ILI9341_PGAMCTRL,
>> + 0x0f, 0x31, 0x2b, 0x0c, 0x0e, 0x08, 0x4e, 0xf1,
>> + 0x37, 0x07, 0x10, 0x03, 0x0e, 0x09, 0x00);
>> + mipi_dbi_command(mipi, ILI9341_NGAMCTRL,
>> + 0x00, 0x0e, 0x14, 0x03, 0x11, 0x07, 0x31, 0xc1,
>> + 0x48, 0x08, 0x0f, 0x0c, 0x31, 0x36, 0x0f);
>> +
>> + /* DDRAM */
>> + mipi_dbi_command(mipi, ILI9341_ETMOD, 0x07);
>> +
>> + /* Display */
>> + mipi_dbi_command(mipi, ILI9341_DISCTRL, 0x08, 0x82, 0x27, 0x00);
>> + mipi_dbi_command(mipi, MIPI_DCS_EXIT_SLEEP_MODE);
>> + msleep(100);
>> +
>> + mipi_dbi_command(mipi, MIPI_DCS_SET_DISPLAY_ON);
>> + msleep(100);
>> +
>> +out_enable:
>> + switch (mipi->rotation) {
>> + default:
>> + addr_mode = ILI9341_MADCTL_MX;
>> + break;
>> + case 90:
>> + addr_mode = ILI9341_MADCTL_MV;
>> + break;
>> + case 180:
>> + addr_mode = ILI9341_MADCTL_MY;
>> + break;
>> + case 270:
>> + addr_mode = ILI9341_MADCTL_MV | ILI9341_MADCTL_MY |
>> + ILI9341_MADCTL_MX;
>> + break;
>> + }
>> + addr_mode |= ILI9341_MADCTL_BGR;
>> + mipi_dbi_command(mipi, MIPI_DCS_SET_ADDRESS_MODE, addr_mode);
>> + mipi_dbi_enable_flush(mipi, crtc_state, plane_state);
>> +}
>> +
>> +static const struct drm_simple_display_pipe_funcs ili9341_pipe_funcs = {
>> + .enable = yx240qv29_enable,
>> + .disable = mipi_dbi_pipe_disable,
>> + .update = tinydrm_display_pipe_update,
>> + .prepare_fb = drm_gem_fb_simple_display_pipe_prepare_fb,
>> +};
>> +
>> +static const struct drm_display_mode yx240qv29_mode = {
>> + TINYDRM_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_PRIME |
>> + DRIVER_ATOMIC,
> I would rather keep one per line, or all at one line disregard 80
> character limit.
I prefer on one line. I've started to use 100 character limit when it
makes sense, because all these linebreaks makes the code harder to read
(checkpatch.pl --max-line-length=100).
Reviewed-by: Noralf Trønnes <noralf@...nnes.org>
Noralf.
>> + .fops = &ili9341_fops,
>> + TINYDRM_GEM_DRIVER_OPS,
>> + .lastclose = drm_fb_helper_lastclose,
>> + .debugfs_init = mipi_dbi_debugfs_init,
>> + .name = "ili9341",
>> + .desc = "Ilitek ILI9341",
>> + .date = "20180514",
>> + .major = 1,
>> + .minor = 0,
>> +};
>> +
>> +static const struct of_device_id ili9341_of_match[] = {
>> + { .compatible = "noname,yx240qv29" },
>> + {},
> Terminators better w/o comma.
>
>> +};
>> +MODULE_DEVICE_TABLE(of, ili9341_of_match);
>> +
>> +static const struct spi_device_id ili9341_id[] = {
>> + { "yx240qv29", 0 },
>> + { },
> Ditto.
>
>> +};
>> +MODULE_DEVICE_TABLE(spi, ili9341_id);
>> +
>> +static int ili9341_probe(struct spi_device *spi)
>> +{
>> + struct device *dev = &spi->dev;
>> + struct mipi_dbi *mipi;
>> + struct gpio_desc *dc;
>> + u32 rotation = 0;
>> + int ret;
>> +
>> + mipi = devm_kzalloc(dev, sizeof(*mipi), GFP_KERNEL);
>> + if (!mipi)
>> + return -ENOMEM;
>> +
>> + mipi->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
>> + if (IS_ERR(mipi->reset)) {
>> + DRM_DEV_ERROR(dev, "Failed to get gpio 'reset'\n");
>> + return PTR_ERR(mipi->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);
>> + }
>> +
>> + mipi->regulator = devm_regulator_get(dev, "power");
>> + if (IS_ERR(mipi->regulator))
>> + return PTR_ERR(mipi->regulator);
>> +
>> + mipi->backlight = devm_of_find_backlight(dev);
>> + if (IS_ERR(mipi->backlight))
>> + return PTR_ERR(mipi->backlight);
>> +
>> + device_property_read_u32(dev, "rotation", &rotation);
>
>> +
>> + ret = mipi_dbi_spi_init(spi, mipi, dc);
>> + if (ret)
>> + return ret;
>> +
>> + ret = mipi_dbi_init(&spi->dev, mipi, &ili9341_pipe_funcs,
>> + &ili9341_driver, &yx240qv29_mode, rotation);
>> + if (ret)
>> + return ret;
>> +
>> + spi_set_drvdata(spi, mipi);
>> +
>> + return devm_tinydrm_register(&mipi->tinydrm);
>> +}
>> +
>> +static void ili9341_shutdown(struct spi_device *spi)
>> +{
>> + struct mipi_dbi *mipi = spi_get_drvdata(spi);
>> +
>> + tinydrm_shutdown(&mipi->tinydrm);
>> +}
>> +
>> +static struct spi_driver ili9341_spi_driver = {
>> + .driver = {
>> + .name = "ili9341",
>> + .of_match_table = ili9341_of_match,
>> + },
>> + .id_table = ili9341_id,
>> + .probe = ili9341_probe,
>> + .shutdown = ili9341_shutdown,
>> +};
>> +module_spi_driver(ili9341_spi_driver);
>> +
>> +MODULE_DESCRIPTION("Ilitek ILI9341 DRM driver");
>> +MODULE_AUTHOR("David Lechner <david@...hnology.com>");
>> +MODULE_LICENSE("GPL");
>> --
>> 2.17.0
>>
>
>
Powered by blists - more mailing lists