Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
In-Reply-To: <20180515014349.26226-4-david@lechnology.com>
References: <20180515014349.26226-1-david@lechnology.com> <20180515014349.26226-4-david@lechnology.com>
From:   Andy Shevchenko <andy.shevchenko@gmail.com>
Date:   Wed, 16 May 2018 00:43:21 +0300
Message-ID: <CAHp75VekNQyJa4KrezVq6dpr7NG3--mdZS9qka6CZjH9BdJxXQ@mail.gmail.com>
Subject: Re: [PATCH 3/3] drm/tinydrm: new driver for ILI9341 display panels
To:     David Lechner <david@lechnology.com>
Cc:     dri-devel@lists.freedesktop.org,
        devicetree <devicetree@vger.kernel.org>,
        =?UTF-8?Q?Noralf_Tr=C3=B8nnes?= <noralf@tronnes.org>,
        Rob Herring <robh+dt@kernel.org>,
        Mark Rutland <mark.rutland@arm.com>, limor@ladyada.net,
        Nitin Patil <nitin@mindsensors.com>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue, May 15, 2018 at 4:43 AM, David Lechner <david@lechnology.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@gmail.com>

>
> Signed-off-by: David Lechner <david@lechnology.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@lechnology.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/Kc=
onfig
> 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/M=
akefile
> 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)          +=3D mipi-dbi.o
>
>  # Displays
>  obj-$(CONFIG_TINYDRM_ILI9225)          +=3D ili9225.o
> +obj-$(CONFIG_TINYDRM_ILI9341)          +=3D ili9341.o
>  obj-$(CONFIG_TINYDRM_MI0283QT)         +=3D mi0283qt.o
>  obj-$(CONFIG_TINYDRM_REPAPER)          +=3D repaper.o
>  obj-$(CONFIG_TINYDRM_ST7586)           +=3D 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@lechnology.com>
> + *
> + * Based on mi0283qt.c:
> + * Copyright 2016 Noralf Tr=C3=B8nnes
> + */
> +
> +#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 =3D pipe_to_tinydrm(pipe);
> +       struct mipi_dbi *mipi =3D mipi_dbi_from_tinydrm(tdev);
> +       u8 addr_mode;
> +       int ret;
> +
> +       DRM_DEBUG_KMS("\n");
> +
> +       ret =3D mipi_dbi_poweron_conditional_reset(mipi);
> +       if (ret < 0)
> +               return;
> +       if (ret =3D=3D 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, 0=
x02);
> +       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 =3D ILI9341_MADCTL_MX;
> +               break;
> +       case 90:
> +               addr_mode =3D ILI9341_MADCTL_MV;
> +               break;
> +       case 180:
> +               addr_mode =3D ILI9341_MADCTL_MY;
> +               break;
> +       case 270:
> +               addr_mode =3D ILI9341_MADCTL_MV | ILI9341_MADCTL_MY |
> +                           ILI9341_MADCTL_MX;
> +               break;
> +       }
> +       addr_mode |=3D 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 =3D=
 {
> +       .enable =3D yx240qv29_enable,
> +       .disable =3D mipi_dbi_pipe_disable,
> +       .update =3D tinydrm_display_pipe_update,
> +       .prepare_fb =3D drm_gem_fb_simple_display_pipe_prepare_fb,
> +};
> +
> +static const struct drm_display_mode yx240qv29_mode =3D {
> +       TINYDRM_MODE(240, 320, 37, 49),
> +};
> +
> +DEFINE_DRM_GEM_CMA_FOPS(ili9341_fops);
> +
> +static struct drm_driver ili9341_driver =3D {

> +       .driver_features        =3D DRIVER_GEM | DRIVER_MODESET | DRIVER_=
PRIME |
> +                                 DRIVER_ATOMIC,

I would rather keep one per line, or all at one line disregard 80
character limit.

> +       .fops                   =3D &ili9341_fops,
> +       TINYDRM_GEM_DRIVER_OPS,
> +       .lastclose              =3D drm_fb_helper_lastclose,
> +       .debugfs_init           =3D mipi_dbi_debugfs_init,
> +       .name                   =3D "ili9341",
> +       .desc                   =3D "Ilitek ILI9341",
> +       .date                   =3D "20180514",
> +       .major                  =3D 1,
> +       .minor                  =3D 0,
> +};
> +
> +static const struct of_device_id ili9341_of_match[] =3D {
> +       { .compatible =3D "noname,yx240qv29" },
> +       {},

Terminators better w/o comma.

> +};
> +MODULE_DEVICE_TABLE(of, ili9341_of_match);
> +
> +static const struct spi_device_id ili9341_id[] =3D {
> +       { "yx240qv29", 0 },
> +       { },

Ditto.

> +};
> +MODULE_DEVICE_TABLE(spi, ili9341_id);
> +
> +static int ili9341_probe(struct spi_device *spi)
> +{
> +       struct device *dev =3D &spi->dev;
> +       struct mipi_dbi *mipi;
> +       struct gpio_desc *dc;
> +       u32 rotation =3D 0;
> +       int ret;
> +
> +       mipi =3D devm_kzalloc(dev, sizeof(*mipi), GFP_KERNEL);
> +       if (!mipi)
> +               return -ENOMEM;
> +
> +       mipi->reset =3D devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_H=
IGH);
> +       if (IS_ERR(mipi->reset)) {
> +               DRM_DEV_ERROR(dev, "Failed to get gpio 'reset'\n");
> +               return PTR_ERR(mipi->reset);
> +       }
> +
> +       dc =3D 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 =3D devm_regulator_get(dev, "power");
> +       if (IS_ERR(mipi->regulator))
> +               return PTR_ERR(mipi->regulator);
> +
> +       mipi->backlight =3D devm_of_find_backlight(dev);
> +       if (IS_ERR(mipi->backlight))
> +               return PTR_ERR(mipi->backlight);
> +

> +       device_property_read_u32(dev, "rotation", &rotation);


> +
> +       ret =3D mipi_dbi_spi_init(spi, mipi, dc);
> +       if (ret)
> +               return ret;
> +
> +       ret =3D 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 =3D spi_get_drvdata(spi);
> +
> +       tinydrm_shutdown(&mipi->tinydrm);
> +}
> +
> +static struct spi_driver ili9341_spi_driver =3D {
> +       .driver =3D {
> +               .name =3D "ili9341",
> +               .of_match_table =3D ili9341_of_match,
> +       },
> +       .id_table =3D ili9341_id,
> +       .probe =3D ili9341_probe,
> +       .shutdown =3D ili9341_shutdown,
> +};
> +module_spi_driver(ili9341_spi_driver);
> +
> +MODULE_DESCRIPTION("Ilitek ILI9341 DRM driver");
> +MODULE_AUTHOR("David Lechner <david@lechnology.com>");
> +MODULE_LICENSE("GPL");
> --
> 2.17.0
>


--=20
With Best Regards,
Andy Shevchenko