Re: [EXT] Re: [PATCH 2/2] drm/panel: Add support for Raydium RM67191 panel driver

[Robert Chiras]

Hi Sam,

Thank you for your suggestions.
See my reply inline.

On Vi, 2019-06-14 at 17:10 +0200, Sam Ravnborg wrote:
> Hi Robert.
> 
> On top of the feedback from Fabio here is a bit more.
> 
> > 
> > +
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> Divide include up in block in following order:
> 
> #include 
> 
> #include 
> 
> #incude 
> 
> Within each block sort alphabetically.
> Do not use the deprecated drmP.h - replace it with the necessary
> includes.
> Use an empty line between each include block.
Thanks. Will do.
> 
> > 
> > +
> > +/* Write Manufacture Command Set Control */
> > +#define WRMAUCCTR 0xFE
> > +
> > +/* Manufacturer Command Set pages (CMD2) */
> > +struct cmd_set_entry {
> > + u8 cmd;
> > + u8 param;
> > +};
> > +
> > +/*
> > + * There is no description in the Reference Manual about these
> > commands.
> > + * We received them from vendor, so just use them as is.
> > + */
> > +static const struct cmd_set_entry manufacturer_cmd_set[] = {
> > + {0xFE, 0x0B},
> > + {0x28, 0x40},
> > + {0x29, 0x4F},
> ...
> > 
> > + {0x51, 0x04},
> > +};
> > +
> > +static const u32 rad_bus_formats[] = {
> > + MEDIA_BUS_FMT_RGB888_1X24,
> > + MEDIA_BUS_FMT_RGB666_1X18,
> > + MEDIA_BUS_FMT_RGB565_1X16,
> > +};
> > +
> > +struct rad_panel {
> > + struct drm_panel base;
> In the other raydium driver we name this "panel", which is a more
> descriptive name.
> 
> > 
> > + struct mipi_dsi_device *dsi;
> > +
> > + struct gpio_desc *reset;
> > + struct backlight_device *backlight;
> > +
> > + bool prepared;
> > + bool enabled;
> > +
> > + struct videomode vm;
> > + u32 width_mm;
> > + u32 height_mm;
> > +};
> > +
> > +static int rad_panel_prepare(struct drm_panel *panel)
> > +{
> > + struct rad_panel *rad = to_rad_panel(panel);
> > +
> > + if (rad->prepared)
> > + return 0;
> > +
> > + if (rad->reset) {
> > + gpiod_set_value(rad->reset, 0);
> > + usleep_range(5000, 1);
> > + gpiod_set_value(rad->reset, 1);
> > + usleep_range(2, 25000);
> > + }
> > +
> > + rad->prepared = true;
> > +
> > + return 0;
> > +}
> > +
> > +static int rad_panel_unprepare(struct drm_panel *panel)
> > +{
> > + struct rad_panel *rad = to_rad_panel(panel);
> > + struct device *dev = &rad->dsi->dev;
> > +
> > + if (!rad->prepared)
> > + return 0;
> > +
> > + if (rad->enabled) {
> > + DRM_DEV_ERROR(dev, "Panel still enabled!\n");
> > + return -EPERM;
> > + }
> This seems like overkill, what should trigger this?
Probably, just bad written code in the DSI host driver, but you are
right, it's overkill. I will remove this.
> 
> > 
> > +
> > + if (rad->reset) {
> > + gpiod_set_value(rad->reset, 0);
> > + usleep_range(15000, 17000);
> > + gpiod_set_value(rad->reset, 1);
> > + }
> > +
> > + rad->prepared = false;
> > +
> > + return 0;
> > +}
> > +
> > +static int rad_panel_enable(struct drm_panel *panel)
> > +{
> > + struct rad_panel *rad = to_rad_panel(panel);
> > + struct mipi_dsi_device *dsi = rad->dsi;
> > + struct device *dev = &dsi->dev;
> > + int color_format = color_format_from_dsi_format(dsi->format);
> > + u16 brightness;
> > + int ret;
> > +
> > + if (rad->enabled)
> > + return 0;
> > +
> > + if (!rad->prepared) {
> > + DRM_DEV_ERROR(dev, "Panel not prepared!\n");
> > + return -EPERM;
> > + }
> Seems like overkill.
> 
> > 
> > +
> > + dsi->mode_flags |= MIPI_DSI_MODE_LPM;
> > +
> > + ret = rad_panel_push_cmd_list(dsi);
> > + if (ret < 0) {
> > + DRM_DEV_ERROR(dev, "Failed to send MCS (%d)\n", ret);
> > + goto fail;
> > + }
> > +
> > + /* Select User Command Set table (CMD1) */
> > + ret = mipi_dsi_generic_write(dsi, (u8[]){ WRMAUCCTR, 0x00 },
> > 2);
> > + if (ret < 0)
> > + goto fail;
> > +
> > + /* Software reset */
> > + ret = mipi_dsi_dcs_soft_reset(dsi);
> > + if (ret < 0) {
> > + DRM_DEV_ERROR(dev, "Failed to do Software Reset
> > (%d)\n", ret);
> > + goto fail;
> > + }
> > +
> > + usleep_range(15000, 17000);
> > +
> > + /* Set DSI mode */
> > + ret = mipi_dsi_generic_write(dsi, (u8[]){ 0xC2, 0x0B }, 2);
> > + if (ret < 0) {
> > + DRM_DEV_ERROR(dev, "Failed to set DSI mode (%d)\n",
> > ret);
> > + goto fail;
> > + }
> > + /* Set tear ON */
> > + ret = mipi_dsi_dcs_set_tear_on(dsi,
> > MIPI_DSI_DCS_TEAR_MODE_VBLANK);
> > + if (ret < 0) {
> > + DRM_DEV_ERROR(dev, "Failed to set tear ON (%d)\n",
> > ret);
> > + goto fail;
> > + }
> > + /* Set tear scanline */
> > + ret = mipi_dsi_dcs_set_t

Re: [EXT] Re: [PATCH 2/2] drm/panel: Add support for Raydium RM67191 panel driver

[Robert Chiras]

On Vi, 2019-06-14 at 10:39 -0300, Fabio Estevam wrote:
> Caution: EXT Email
> 
> On Fri, Jun 14, 2019 at 10:29 AM Robert Chiras  > wrote:
> 
> > 
> > The GPIO is active high, and the above sequence was received from
> > the
> > panel vendor in the following form:
> > SET_RESET_PIN(1);
> > MDELAY(10);
> > SET_RESET_PIN(0);
> > MDELAY(5);
> > SET_RESET_PIN(1);
> > MDELAY(20);
> > I got rid of the first transition to high since seemed redundant.
> > Also, according to the manual reference, the RSTB pin needs to be
> > active high while operating the display.
> That's exactly my point :-)
> 
> In normal operation the GPIO reset needs to be high.
> 
> During reset the GPIO reset needs to be low., which means that the
> GPIO reset is "active low".
> 
> So you should invert both the dts and the driver to behave correctly.
Now I get it. Thanks! I will update the dts and driver for the gpio.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] drm/panel: Add support for Raydium RM67191 panel driver

[Sam Ravnborg]

Hi Robert.

On top of the feedback from Fabio here is a bit more.

> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 

Divide include up in block in following order:

#include 

#include 

#incude 

Within each block sort alphabetically.
Do not use the deprecated drmP.h - replace it with the necessary
includes.
Use an empty line between each include block.

> +
> +/* Write Manufacture Command Set Control */
> +#define WRMAUCCTR 0xFE
> +
> +/* Manufacturer Command Set pages (CMD2) */
> +struct cmd_set_entry {
> + u8 cmd;
> + u8 param;
> +};
> +
> +/*
> + * There is no description in the Reference Manual about these commands.
> + * We received them from vendor, so just use them as is.
> + */
> +static const struct cmd_set_entry manufacturer_cmd_set[] = {
> + {0xFE, 0x0B},
> + {0x28, 0x40},
> + {0x29, 0x4F},
...
> + {0x51, 0x04},
> +};
> +
> +static const u32 rad_bus_formats[] = {
> + MEDIA_BUS_FMT_RGB888_1X24,
> + MEDIA_BUS_FMT_RGB666_1X18,
> + MEDIA_BUS_FMT_RGB565_1X16,
> +};
> +
> +struct rad_panel {
> + struct drm_panel base;
In the other raydium driver we name this "panel", which is a more
descriptive name.

> + struct mipi_dsi_device *dsi;
> +
> + struct gpio_desc *reset;
> + struct backlight_device *backlight;
> +
> + bool prepared;
> + bool enabled;
> +
> + struct videomode vm;
> + u32 width_mm;
> + u32 height_mm;
> +};
> +
> +static int rad_panel_prepare(struct drm_panel *panel)
> +{
> + struct rad_panel *rad = to_rad_panel(panel);
> +
> + if (rad->prepared)
> + return 0;
> +
> + if (rad->reset) {
> + gpiod_set_value(rad->reset, 0);
> + usleep_range(5000, 1);
> + gpiod_set_value(rad->reset, 1);
> + usleep_range(2, 25000);
> + }
> +
> + rad->prepared = true;
> +
> + return 0;
> +}
> +
> +static int rad_panel_unprepare(struct drm_panel *panel)
> +{
> + struct rad_panel *rad = to_rad_panel(panel);
> + struct device *dev = &rad->dsi->dev;
> +
> + if (!rad->prepared)
> + return 0;
> +
> + if (rad->enabled) {
> + DRM_DEV_ERROR(dev, "Panel still enabled!\n");
> + return -EPERM;
> + }
This seems like overkill, what should trigger this?

> +
> + if (rad->reset) {
> + gpiod_set_value(rad->reset, 0);
> + usleep_range(15000, 17000);
> + gpiod_set_value(rad->reset, 1);
> + }
> +
> + rad->prepared = false;
> +
> + return 0;
> +}
> +
> +static int rad_panel_enable(struct drm_panel *panel)
> +{
> + struct rad_panel *rad = to_rad_panel(panel);
> + struct mipi_dsi_device *dsi = rad->dsi;
> + struct device *dev = &dsi->dev;
> + int color_format = color_format_from_dsi_format(dsi->format);
> + u16 brightness;
> + int ret;
> +
> + if (rad->enabled)
> + return 0;
> +
> + if (!rad->prepared) {
> + DRM_DEV_ERROR(dev, "Panel not prepared!\n");
> + return -EPERM;
> + }
Seems like overkill.

> +
> + dsi->mode_flags |= MIPI_DSI_MODE_LPM;
> +
> + ret = rad_panel_push_cmd_list(dsi);
> + if (ret < 0) {
> + DRM_DEV_ERROR(dev, "Failed to send MCS (%d)\n", ret);
> + goto fail;
> + }
> +
> + /* Select User Command Set table (CMD1) */
> + ret = mipi_dsi_generic_write(dsi, (u8[]){ WRMAUCCTR, 0x00 }, 2);
> + if (ret < 0)
> + goto fail;
> +
> + /* Software reset */
> + ret = mipi_dsi_dcs_soft_reset(dsi);
> + if (ret < 0) {
> + DRM_DEV_ERROR(dev, "Failed to do Software Reset (%d)\n", ret);
> + goto fail;
> + }
> +
> + usleep_range(15000, 17000);
> +
> + /* Set DSI mode */
> + ret = mipi_dsi_generic_write(dsi, (u8[]){ 0xC2, 0x0B }, 2);
> + if (ret < 0) {
> + DRM_DEV_ERROR(dev, "Failed to set DSI mode (%d)\n", ret);
> + goto fail;
> + }
> + /* Set tear ON */
> + ret = mipi_dsi_dcs_set_tear_on(dsi, MIPI_DSI_DCS_TEAR_MODE_VBLANK);
> + if (ret < 0) {
> + DRM_DEV_ERROR(dev, "Failed to set tear ON (%d)\n", ret);
> + goto fail;
> + }
> + /* Set tear scanline */
> + ret = mipi_dsi_dcs_set_tear_scanline(dsi, 0x380);
> + if (ret < 0) {
> + DRM_DEV_ERROR(dev, "Failed to set tear scanline (%d)\n", ret);
> + goto fail;
> + }
> + /* Set pixel format */
> + ret = mipi_dsi_dcs_set_pixel_format(dsi, color_format);
> + DRM_DEV_DEBUG_DRIVER(dev, "Interface color format set to 0x%x\n",
> +  color_format);
> + if (ret < 0) {
> + DRM_DEV_ERROR(dev, "Failed to set pixel format (%d)\n", ret);
> + goto fail;
> + }
> + /* Set display brightness */
> + brightness = rad->backlight->props.brightness;
> + ret = mipi_dsi_dcs_set_display_

Re: [EXT] Re: [PATCH 2/2] drm/panel: Add support for Raydium RM67191 panel driver

[Fabio Estevam]

On Fri, Jun 14, 2019 at 10:29 AM Robert Chiras  wrote:

> The GPIO is active high, and the above sequence was received from the
> panel vendor in the following form:
> SET_RESET_PIN(1);
> MDELAY(10);
> SET_RESET_PIN(0);
> MDELAY(5);
> SET_RESET_PIN(1);
> MDELAY(20);
> I got rid of the first transition to high since seemed redundant.
> Also, according to the manual reference, the RSTB pin needs to be
> active high while operating the display.

That's exactly my point :-)

In normal operation the GPIO reset needs to be high.

During reset the GPIO reset needs to be low., which means that the
GPIO reset is "active low".

So you should invert both the dts and the driver to behave correctly.

Re: [EXT] Re: [PATCH 2/2] drm/panel: Add support for Raydium RM67191 panel driver

[Robert Chiras]

Hi Fabio,

On Vi, 2019-06-14 at 09:27 -0300, Fabio Estevam wrote:
> Hi Robert,
> 
> On Fri, Jun 14, 2019 at 8:52 AM Robert Chiras 
> wrote:
> 
> > 
> > --- /dev/null
> > +++ b/drivers/gpu/drm/panel/panel-raydium-rm67191.c
> > @@ -0,0 +1,730 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * i.MX drm driver - Raydium MIPI-DSI panel driver
> > + *
> > + * Copyright (C) 2017 NXP
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License
> > + * as published by the Free Software Foundation; either version 2
> > + * of the License, or (at your option) any later version.
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> No need for this text as you are using SPDX tag.
> 
> > 
> > +static int color_format_from_dsi_format(enum mipi_dsi_pixel_format
> > format)
> > +{
> > +   switch (format) {
> > +   case MIPI_DSI_FMT_RGB565:
> > +   return 0x55;
> > +   case MIPI_DSI_FMT_RGB666:
> > +   case MIPI_DSI_FMT_RGB666_PACKED:
> > +   return 0x66;
> > +   case MIPI_DSI_FMT_RGB888:
> > +   return 0x77;
> Could you use defines for these magic 0x55, 0x66 and 0x77 numbers?
Those magic numbers mean exactly what their case statements are. They
come from the panel documentation. I thought that the already existing
defines (MIPI_DSI_FMT_) are self explanatory here, so using defines
seemed redundant for me. But, if you think that using defines for them
is better, I can do that.
> 
> > 
> > +static int rad_panel_prepare(struct drm_panel *panel)
> > +{
> > +   struct rad_panel *rad = to_rad_panel(panel);
> > +
> > +   if (rad->prepared)
> > +   return 0;
> > +
> > +   if (rad->reset) {
> > +   gpiod_set_value(rad->reset, 0);
> > +   usleep_range(5000, 1);
> > +   gpiod_set_value(rad->reset, 1);
> > +   usleep_range(2, 25000);
> This does not look correct.
> 
> The correct way to do a reset with gpiod API is:
> 
>  gpiod_set_value(rad->reset, 1);
> 
> delay
> 
> gpiod_set_value(rad->reset, 0);
> 
> I don't have the datasheet for the RM67191 panel, but I assume the
> reset GPIO is active low.
> 
> Since you inverted the polarity in the dts and inside the driver, you
> got it right by accident.
The GPIO is active high, and the above sequence was received from the
panel vendor in the following form:
SET_RESET_PIN(1);
MDELAY(10);
SET_RESET_PIN(0);
MDELAY(5);
SET_RESET_PIN(1);
MDELAY(20);
I got rid of the first transition to high since seemed redundant.
Also, according to the manual reference, the RSTB pin needs to be
active high while operating the display.
> 
> You could also consider using gpiod_set_value_cansleep() variant
> instead because the GPIO reset could be provided by an I2C GPIO
> expander, for example.
Thanks, will use this in the next revision.
> 
> Also, when sleeping for more than 10ms, msleep is a better fit as per
> Documentation/timers/timers-howto.txt.
OK, I will use msleep. That documentation recommends using usleep_range
for sleeps <20m, but also using msleep for >10ms (so I followed the
recomendations from usleep_range)
> 
> > 
> > +   if (rad->reset) {
> > +   gpiod_set_value(rad->reset, 0);
> > +   usleep_range(15000, 17000);
> > +   gpiod_set_value(rad->reset, 1);
> > +   }
> Another reset?
Yes. This is tricky, since this GPIO is shared between the DSI
controller and touch controller. The Android guys needs the touch
controller to be active, while the display can be turned off. So this
is why, after the display is turned off, the reset pin is put back to
HIGH in order to keep the touch working.

Re: [PATCH 2/2] drm/panel: Add support for Raydium RM67191 panel driver

[Fabio Estevam]

Hi Robert,

On Fri, Jun 14, 2019 at 8:52 AM Robert Chiras  wrote:

> --- /dev/null
> +++ b/drivers/gpu/drm/panel/panel-raydium-rm67191.c
> @@ -0,0 +1,730 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * i.MX drm driver - Raydium MIPI-DSI panel driver
> + *
> + * Copyright (C) 2017 NXP
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version 2
> + * of the License, or (at your option) any later version.
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.

No need for this text as you are using SPDX tag.

> +static int color_format_from_dsi_format(enum mipi_dsi_pixel_format format)
> +{
> +   switch (format) {
> +   case MIPI_DSI_FMT_RGB565:
> +   return 0x55;
> +   case MIPI_DSI_FMT_RGB666:
> +   case MIPI_DSI_FMT_RGB666_PACKED:
> +   return 0x66;
> +   case MIPI_DSI_FMT_RGB888:
> +   return 0x77;

Could you use defines for these magic 0x55, 0x66 and 0x77 numbers?

> +static int rad_panel_prepare(struct drm_panel *panel)
> +{
> +   struct rad_panel *rad = to_rad_panel(panel);
> +
> +   if (rad->prepared)
> +   return 0;
> +
> +   if (rad->reset) {
> +   gpiod_set_value(rad->reset, 0);
> +   usleep_range(5000, 1);
> +   gpiod_set_value(rad->reset, 1);
> +   usleep_range(2, 25000);

This does not look correct.

The correct way to do a reset with gpiod API is:

 gpiod_set_value(rad->reset, 1);

delay

gpiod_set_value(rad->reset, 0);

I don't have the datasheet for the RM67191 panel, but I assume the
reset GPIO is active low.

Since you inverted the polarity in the dts and inside the driver, you
got it right by accident.

You could also consider using gpiod_set_value_cansleep() variant
instead because the GPIO reset could be provided by an I2C GPIO
expander, for example.

Also, when sleeping for more than 10ms, msleep is a better fit as per
Documentation/timers/timers-howto.txt.

> +   if (rad->reset) {
> +   gpiod_set_value(rad->reset, 0);
> +   usleep_range(15000, 17000);
> +   gpiod_set_value(rad->reset, 1);
> +   }

Another reset?
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] drm/panel: Add support for Raydium RM67191 panel driver

[Daniel Baluta]

Hi Robert,

Minor comment. See inline:

On Fri, Jun 14, 2019 at 2:52 PM Robert Chiras  wrote:
>
> This patch adds Raydium RM67191 TFT LCD panel driver (MIPI-DSI
> protocol).
>
> Signed-off-by: Robert Chiras 
> ---
>  drivers/gpu/drm/panel/Kconfig |   9 +
>  drivers/gpu/drm/panel/Makefile|   1 +
>  drivers/gpu/drm/panel/panel-raydium-rm67191.c | 730 
> ++
>  3 files changed, 740 insertions(+)
>  create mode 100644 drivers/gpu/drm/panel/panel-raydium-rm67191.c
>
> diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
> index d9d931a..8be1ac1 100644
> --- a/drivers/gpu/drm/panel/Kconfig
> +++ b/drivers/gpu/drm/panel/Kconfig
> @@ -159,6 +159,15 @@ config DRM_PANEL_RASPBERRYPI_TOUCHSCREEN
>   Pi 7" Touchscreen.  To compile this driver as a module,
>   choose M here.
>
> +config DRM_PANEL_RAYDIUM_RM67191
> +   tristate "Raydium RM67191 FHD 1080x1920 DSI video mode panel"
> +   depends on OF
> +   depends on DRM_MIPI_DSI
> +   depends on BACKLIGHT_CLASS_DEVICE
> +   help
> + Say Y here if you want to enable support for Raydium RM67191 FHD
> + (1080x1920) DSI panel.
> +
>  config DRM_PANEL_RAYDIUM_RM68200
> tristate "Raydium RM68200 720x1280 DSI video mode panel"
> depends on OF
> diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
> index fb0cb3a..1fc0f68 100644
> --- a/drivers/gpu/drm/panel/Makefile
> +++ b/drivers/gpu/drm/panel/Makefile
> @@ -14,6 +14,7 @@ obj-$(CONFIG_DRM_PANEL_ORISETECH_OTM8009A) += 
> panel-orisetech-otm8009a.o
>  obj-$(CONFIG_DRM_PANEL_OSD_OSD101T2587_53TS) += panel-osd-osd101t2587-53ts.o
>  obj-$(CONFIG_DRM_PANEL_PANASONIC_VVX10F034N00) += 
> panel-panasonic-vvx10f034n00.o
>  obj-$(CONFIG_DRM_PANEL_RASPBERRYPI_TOUCHSCREEN) += 
> panel-raspberrypi-touchscreen.o
> +obj-$(CONFIG_DRM_PANEL_RAYDIUM_RM67191) += panel-raydium-rm67191.o
>  obj-$(CONFIG_DRM_PANEL_RAYDIUM_RM68200) += panel-raydium-rm68200.o
>  obj-$(CONFIG_DRM_PANEL_ROCKTECH_JH057N00900) += panel-rocktech-jh057n00900.o
>  obj-$(CONFIG_DRM_PANEL_RONBO_RB070D30) += panel-ronbo-rb070d30.o
> diff --git a/drivers/gpu/drm/panel/panel-raydium-rm67191.c 
> b/drivers/gpu/drm/panel/panel-raydium-rm67191.c
> new file mode 100644
> index 000..75bfb03
> --- /dev/null
> +++ b/drivers/gpu/drm/panel/panel-raydium-rm67191.c
> @@ -0,0 +1,730 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * i.MX drm driver - Raydium MIPI-DSI panel driver
> + *
> + * Copyright (C) 2017 NXP
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version 2
> + * of the License, or (at your option) any later version.
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */

Please remove the license text once you already added the SPDX identifier.

Also preferred copyright for NXP is:

Copyright 2019 NXP

So, the file should look like this:

// SPDX-License-Identifier: GPL-2.0
/*
 * i.MX drm driver - Raydium MIPI-DSI panel driver
 *
 * Copyright 2019 NXP
 */

[PATCH 2/2] drm/panel: Add support for Raydium RM67191 panel driver

[Robert Chiras]

This patch adds Raydium RM67191 TFT LCD panel driver (MIPI-DSI
protocol).

Signed-off-by: Robert Chiras 
---
 drivers/gpu/drm/panel/Kconfig |   9 +
 drivers/gpu/drm/panel/Makefile|   1 +
 drivers/gpu/drm/panel/panel-raydium-rm67191.c | 730 ++
 3 files changed, 740 insertions(+)
 create mode 100644 drivers/gpu/drm/panel/panel-raydium-rm67191.c

diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
index d9d931a..8be1ac1 100644
--- a/drivers/gpu/drm/panel/Kconfig
+++ b/drivers/gpu/drm/panel/Kconfig
@@ -159,6 +159,15 @@ config DRM_PANEL_RASPBERRYPI_TOUCHSCREEN
  Pi 7" Touchscreen.  To compile this driver as a module,
  choose M here.
 
+config DRM_PANEL_RAYDIUM_RM67191
+   tristate "Raydium RM67191 FHD 1080x1920 DSI video mode panel"
+   depends on OF
+   depends on DRM_MIPI_DSI
+   depends on BACKLIGHT_CLASS_DEVICE
+   help
+ Say Y here if you want to enable support for Raydium RM67191 FHD
+ (1080x1920) DSI panel.
+
 config DRM_PANEL_RAYDIUM_RM68200
tristate "Raydium RM68200 720x1280 DSI video mode panel"
depends on OF
diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
index fb0cb3a..1fc0f68 100644
--- a/drivers/gpu/drm/panel/Makefile
+++ b/drivers/gpu/drm/panel/Makefile
@@ -14,6 +14,7 @@ obj-$(CONFIG_DRM_PANEL_ORISETECH_OTM8009A) += 
panel-orisetech-otm8009a.o
 obj-$(CONFIG_DRM_PANEL_OSD_OSD101T2587_53TS) += panel-osd-osd101t2587-53ts.o
 obj-$(CONFIG_DRM_PANEL_PANASONIC_VVX10F034N00) += 
panel-panasonic-vvx10f034n00.o
 obj-$(CONFIG_DRM_PANEL_RASPBERRYPI_TOUCHSCREEN) += 
panel-raspberrypi-touchscreen.o
+obj-$(CONFIG_DRM_PANEL_RAYDIUM_RM67191) += panel-raydium-rm67191.o
 obj-$(CONFIG_DRM_PANEL_RAYDIUM_RM68200) += panel-raydium-rm68200.o
 obj-$(CONFIG_DRM_PANEL_ROCKTECH_JH057N00900) += panel-rocktech-jh057n00900.o
 obj-$(CONFIG_DRM_PANEL_RONBO_RB070D30) += panel-ronbo-rb070d30.o
diff --git a/drivers/gpu/drm/panel/panel-raydium-rm67191.c 
b/drivers/gpu/drm/panel/panel-raydium-rm67191.c
new file mode 100644
index 000..75bfb03
--- /dev/null
+++ b/drivers/gpu/drm/panel/panel-raydium-rm67191.c
@@ -0,0 +1,730 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * i.MX drm driver - Raydium MIPI-DSI panel driver
+ *
+ * Copyright (C) 2017 NXP
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+/* Write Manufacture Command Set Control */
+#define WRMAUCCTR 0xFE
+
+/* Manufacturer Command Set pages (CMD2) */
+struct cmd_set_entry {
+   u8 cmd;
+   u8 param;
+};
+
+/*
+ * There is no description in the Reference Manual about these commands.
+ * We received them from vendor, so just use them as is.
+ */
+static const struct cmd_set_entry manufacturer_cmd_set[] = {
+   {0xFE, 0x0B},
+   {0x28, 0x40},
+   {0x29, 0x4F},
+   {0xFE, 0x0E},
+   {0x4B, 0x00},
+   {0x4C, 0x0F},
+   {0x4D, 0x20},
+   {0x4E, 0x40},
+   {0x4F, 0x60},
+   {0x50, 0xA0},
+   {0x51, 0xC0},
+   {0x52, 0xE0},
+   {0x53, 0xFF},
+   {0xFE, 0x0D},
+   {0x18, 0x08},
+   {0x42, 0x00},
+   {0x08, 0x41},
+   {0x46, 0x02},
+   {0x72, 0x09},
+   {0xFE, 0x0A},
+   {0x24, 0x17},
+   {0x04, 0x07},
+   {0x1A, 0x0C},
+   {0x0F, 0x44},
+   {0xFE, 0x04},
+   {0x00, 0x0C},
+   {0x05, 0x08},
+   {0x06, 0x08},
+   {0x08, 0x08},
+   {0x09, 0x08},
+   {0x0A, 0xE6},
+   {0x0B, 0x8C},
+   {0x1A, 0x12},
+   {0x1E, 0xE0},
+   {0x29, 0x93},
+   {0x2A, 0x93},
+   {0x2F, 0x02},
+   {0x31, 0x02},
+   {0x33, 0x05},
+   {0x37, 0x2D},
+   {0x38, 0x2D},
+   {0x3A, 0x1E},
+   {0x3B, 0x1E},
+   {0x3D, 0x27},
+   {0x3F, 0x80},
+   {0x40, 0x40},
+   {0x41, 0xE0},
+   {0x4F, 0x2F},
+   {0x50, 0x1E},
+   {0xFE, 0x06},
+   {0x00, 0xCC},
+   {0x05, 0x05},
+   {0x07, 0xA2},
+   {0x08, 0xCC},
+   {0x0D, 0x03},
+   {0x0F, 0xA2},
+   {0x32, 0xCC},
+   {0x37, 0x05},
+   {0x39, 0x83},
+   {0x3A, 0xCC},
+   {0x41, 0x04},
+   {0x43, 0x83},
+   {0x44, 0xCC},
+   {0x49, 0x05},
+   {0x4B, 0xA2},
+   {0x4C, 0xCC},
+   {0x51, 0x03},
+   {0x53, 0xA2},
+   {0x75, 0xCC},
+   {0x7A, 0x03},
+   {0x7C, 0x83},
+   {0x7D, 0xCC},
+   {0x82, 0x02},
+   {0x84, 0x83},
+   {0x