Re: [EXT] Re: [PATCH v3 2/2] drm/panel: Add support for Raydium RM67191 panel driver
On Lu, 2019-06-24 at 13:12 -0300, Fabio Estevam wrote: > Caution: EXT Email > > Hi Robert, > > On Mon, Jun 24, 2019 at 4:44 AM Robert Chiras > wrote: > > > > > > > > > You did not handle the "power" regulator. > > There is no need for a power regulator. > I am sure the panel requires power to be applied so that it can work > :-) Yes, of course there are power lines to the panel. I will add them to the next revision. > > > > > > > > > Can't you simply remove them? > > The reset gpio pin is shared between the DSI display controller and > Looking at the imx8mq-evk schematics it is not clear for me what pin > in shared between the OLED display and touch screen. > > Could you please clarify which pin you are referring to? It's about the DSI_EN pin on the DSI LCD IF physical port J1501. This goes into RST_B_1V8 on the panel port which is used for both display and touch resets. > > > > > touch controller, both found on the same panel. Since, the touch > > driver > > also acceses this gpio pin, in some cases the display can be put to > > sleep, but the touch is still active. This is why, during suspend I > > release the gpio resource, and I have to take it back in resume. > > Without this release/acquire mechanism during suspend/resume, > > stress- > > tests showed some failures: the resume freezes completly. > Looking at the imx8mq-evk dts from the vendor tree I see that the > touchscreen is not supported in mainline yet. > > Maybe there is a better way to solve this, so what if you don't > introduce the suspend/resume hooks for now and then we revisit this > after the touchscreen driver is added? You are right. We can do this too. I will remove it for now.
Re: [EXT] Re: [PATCH v3 2/2] drm/panel: Add support for Raydium RM67191 panel driver
Hi Robert, On Mon, Jun 24, 2019 at 4:44 AM Robert Chiras wrote: > > You did not handle the "power" regulator. > There is no need for a power regulator. I am sure the panel requires power to be applied so that it can work :-) > > Can't you simply remove them? > The reset gpio pin is shared between the DSI display controller and Looking at the imx8mq-evk schematics it is not clear for me what pin in shared between the OLED display and touch screen. Could you please clarify which pin you are referring to? > touch controller, both found on the same panel. Since, the touch driver > also acceses this gpio pin, in some cases the display can be put to > sleep, but the touch is still active. This is why, during suspend I > release the gpio resource, and I have to take it back in resume. > Without this release/acquire mechanism during suspend/resume, stress- > tests showed some failures: the resume freezes completly. Looking at the imx8mq-evk dts from the vendor tree I see that the touchscreen is not supported in mainline yet. Maybe there is a better way to solve this, so what if you don't introduce the suspend/resume hooks for now and then we revisit this after the touchscreen driver is added?
Re: [EXT] Re: [PATCH v3 2/2] drm/panel: Add support for Raydium RM67191 panel driver
Hi Fabio,
Thanks for your feedback. I will handle them all, but for the pm_ops I
have some comments. See inline.
On Vi, 2019-06-21 at 10:59 -0300, Fabio Estevam wrote:
> Hi Robert,
>
> On Thu, Jun 20, 2019 at 10:31 AM Robert Chiras > wrote:
>
> >
> > +fail:
> > + if (rad->reset)
> > + gpiod_set_value_cansleep(rad->reset, 1);
> gpiod_set_value_cansleep() can handle NULL, so no need for the if()
> check.
>
> >
> > +static const struct display_timing rad_default_timing = {
> > + .pixelclock = { 13200, 13200, 13200 },
> Having the same information listed three times does not seem useful.
>
> You could use a drm_display_mode structure with a single entry
> instead.
>
> >
> > + videomode_from_timing(_default_timing, >vm);
> > +
> > + of_property_read_u32(np, "width-mm", >width_mm);
> > + of_property_read_u32(np, "height-mm", >height_mm);
> > +
> > + panel->reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
> Since this is optional it would be better to use
> devm_gpiod_get_optional() instead.
>
>
> >
> > +
> > + if (IS_ERR(panel->reset))
> > + panel->reset = NULL;
> > + else
> > + gpiod_set_value_cansleep(panel->reset, 1);
> This is not handling defer probing, so it would be better to do like
> this:
>
> panel->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
> if (IS_ERR(panel->reset))
> return PTR_ERR(panel->reset);
>
> >
> > + memset(_props, 0, sizeof(bl_props));
> > + bl_props.type = BACKLIGHT_RAW;
> > + bl_props.brightness = 255;
> > + bl_props.max_brightness = 255;
> > +
> > + panel->backlight = devm_backlight_device_register(dev,
> > dev_name(dev),
> > + dev, dsi,
> > + _bl_o
> > ps,
> > + _props
> > );
> Could you put more parameters into the same line?
>
> Using 4 lines seems excessive.
>
> >
> > + if (IS_ERR(panel->backlight)) {
> > + ret = PTR_ERR(panel->backlight);
> > + dev_err(dev, "Failed to register backlight (%d)\n",
> > ret);
> > + return ret;
> > + }
> > +
> > + drm_panel_init(>panel);
> > + panel->panel.funcs = _panel_funcs;
> > + panel->panel.dev = dev;
> > + dev_set_drvdata(dev, panel);
> > +
> > + ret = drm_panel_add(>panel);
> > +
> Unneeded blank line
>
> >
> > + if (ret < 0)
> > + return ret;
> > +
> > + ret = mipi_dsi_attach(dsi);
> > + if (ret < 0)
> > + drm_panel_remove(>panel);
> > +
> > + return ret;
> You did not handle the "power" regulator.
There is no need for a power regulator.
>
> >
> > +static int __maybe_unused rad_panel_suspend(struct device *dev)
> > +{
> > + struct rad_panel *rad = dev_get_drvdata(dev);
> > +
> > + if (!rad->reset)
> > + return 0;
> > +
> > + devm_gpiod_put(dev, rad->reset);
> > + rad->reset = NULL;
> > +
> > + return 0;
> > +}
> > +
> > +static int __maybe_unused rad_panel_resume(struct device *dev)
> > +{
> > + struct rad_panel *rad = dev_get_drvdata(dev);
> > +
> > + if (rad->reset)
> > + return 0;
> > +
> > + rad->reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
> Why do you call devm_gpiod_get() once again?
>
> I am having a hard time to understand the need for this
> suspend/resume hooks.
>
> Can't you simply remove them?
The reset gpio pin is shared between the DSI display controller and
touch controller, both found on the same panel. Since, the touch driver
also acceses this gpio pin, in some cases the display can be put to
sleep, but the touch is still active. This is why, during suspend I
release the gpio resource, and I have to take it back in resume.
Without this release/acquire mechanism during suspend/resume, stress-
tests showed some failures: the resume freezes completly.
Re: [PATCH v3 2/2] drm/panel: Add support for Raydium RM67191 panel driver
Hi Robert,
On Thu, Jun 20, 2019 at 10:31 AM Robert Chiras wrote:
> +fail:
> + if (rad->reset)
> + gpiod_set_value_cansleep(rad->reset, 1);
gpiod_set_value_cansleep() can handle NULL, so no need for the if() check.
> +static const struct display_timing rad_default_timing = {
> + .pixelclock = { 13200, 13200, 13200 },
Having the same information listed three times does not seem useful.
You could use a drm_display_mode structure with a single entry instead.
> + videomode_from_timing(_default_timing, >vm);
> +
> + of_property_read_u32(np, "width-mm", >width_mm);
> + of_property_read_u32(np, "height-mm", >height_mm);
> +
> + panel->reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
Since this is optional it would be better to use
devm_gpiod_get_optional() instead.
> +
> + if (IS_ERR(panel->reset))
> + panel->reset = NULL;
> + else
> + gpiod_set_value_cansleep(panel->reset, 1);
This is not handling defer probing, so it would be better to do like this:
panel->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
if (IS_ERR(panel->reset))
return PTR_ERR(panel->reset);
> + memset(_props, 0, sizeof(bl_props));
> + bl_props.type = BACKLIGHT_RAW;
> + bl_props.brightness = 255;
> + bl_props.max_brightness = 255;
> +
> + panel->backlight = devm_backlight_device_register(dev, dev_name(dev),
> + dev, dsi,
> + _bl_ops,
> + _props);
Could you put more parameters into the same line?
Using 4 lines seems excessive.
> + if (IS_ERR(panel->backlight)) {
> + ret = PTR_ERR(panel->backlight);
> + dev_err(dev, "Failed to register backlight (%d)\n", ret);
> + return ret;
> + }
> +
> + drm_panel_init(>panel);
> + panel->panel.funcs = _panel_funcs;
> + panel->panel.dev = dev;
> + dev_set_drvdata(dev, panel);
> +
> + ret = drm_panel_add(>panel);
> +
Unneeded blank line
> + if (ret < 0)
> + return ret;
> +
> + ret = mipi_dsi_attach(dsi);
> + if (ret < 0)
> + drm_panel_remove(>panel);
> +
> + return ret;
You did not handle the "power" regulator.
> +static int __maybe_unused rad_panel_suspend(struct device *dev)
> +{
> + struct rad_panel *rad = dev_get_drvdata(dev);
> +
> + if (!rad->reset)
> + return 0;
> +
> + devm_gpiod_put(dev, rad->reset);
> + rad->reset = NULL;
> +
> + return 0;
> +}
> +
> +static int __maybe_unused rad_panel_resume(struct device *dev)
> +{
> + struct rad_panel *rad = dev_get_drvdata(dev);
> +
> + if (rad->reset)
> + return 0;
> +
> + rad->reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
Why do you call devm_gpiod_get() once again?
I am having a hard time to understand the need for this suspend/resume hooks.
Can't you simply remove them?
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v3 2/2] drm/panel: Add support for Raydium RM67191 panel driver
This patch adds Raydium RM67191 TFT LCD panel driver (MIPI-DSI
protocol).
Signed-off-by: Robert Chiras
Reviewed-by: Sam Ravnborg
---
MAINTAINERS | 6 +
drivers/gpu/drm/panel/Kconfig | 9 +
drivers/gpu/drm/panel/Makefile| 1 +
drivers/gpu/drm/panel/panel-raydium-rm67191.c | 690 ++
4 files changed, 706 insertions(+)
create mode 100644 drivers/gpu/drm/panel/panel-raydium-rm67191.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 7a2f487..cd93030e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5089,6 +5089,12 @@ S: Maintained
F: drivers/gpu/drm/qxl/
F: include/uapi/drm/qxl_drm.h
+DRM DRIVER FOR RAYDIUM RM67191 PANELS
+M: Robert Chiras
+S: Maintained
+F: drivers/gpu/drm/panel/panel-raydium-rm67191.c
+F: Documentation/devicetree/bindings/display/panel/raydium,rm67191.txt
+
DRM DRIVER FOR RAGE 128 VIDEO CARDS
S: Orphan / Obsolete
F: drivers/gpu/drm/r128/
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..c4d7dfd
--- /dev/null
+++ b/drivers/gpu/drm/panel/panel-raydium-rm67191.c
@@ -0,0 +1,690 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Raydium RM67191 MIPI-DSI panel driver
+ *
+ * Copyright 2019 NXP
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include
+#include
+#include
+
+#include
+#include
+#include
+#include
+
+/* Panel specific color-format bits */
+#define COL_FMT_16BPP 0x55
+#define COL_FMT_18BPP 0x66
+#define COL_FMT_24BPP 0x77
+
+/* 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},
