Re: [PATCH 1/2] drm/panel: Add Raydium RM67191 DSI Panel
On Vi, 2018-04-27 at 11:08 +, Thierry Reding wrote:
> On Fri, Apr 27, 2018 at 10:37:16AM +, Robert Chiras wrote:
> >
> >
> > Hi Thierry,
> >
> > Thanks a lot for reviewing this. Your suggestions are very
> > valuable.
> > Please see my detailed answers inline.
> >
> > Best regards,
> > Robert
> Couple of comments regarding email replies. Please use proper quoting
> of
> what you're replying to. Your mail user agent should be able to do
> that
> automatically. If not, you might want to consider switching to a
> better
> one.
>
> Also, please try to stick to 78 columns of text. It's difficult to
> read
> something that is wider than that and may get wrapped. It's also
> difficult to reply to paragraphs that are too wide.
>
> Other than than, some technical comments below.
Sorry about the replies, I was using the web version of Office365 (not
like the Windows app is much better). I switched to Evolution, now.
Also, sorry for the late reply, I waited to include a fix regarding DSI
init sequence. This fix was needed after a client complained about the
fact that there are DSI signals on the data lanes during the reset of
the panel, so I had to separate the GPIO reset from the DSI init.
All your comments should be addressed in the v2 of this patch I just
sent.
>
> >
> >
> > From: Thierry Reding
> > Sent: Thursday, April 26, 2018 5:54 PM
> > To: Robert Chiras
> > Cc: dl-linux-imx; dri-devel@lists.freedesktop.org
> > Subject: Re: [PATCH 1/2] drm/panel: Add Raydium RM67191 DSI Panel
> >
> > On Tue, Apr 03, 2018 at 01:30:00PM +0300, Robert Chiras wrote:
> > >
> > > Add support for the OLED display based on MIPI-DSI protocol from
> > > Raydium:
> > > RM67191.
> > >
> > > Signed-off-by: Robert Chiras
> > > ---
> > > .../bindings/display/panel/raydium,rm67191.txt | 55 ++
> > > drivers/gpu/drm/panel/Kconfig | 9 +
> > > drivers/gpu/drm/panel/Makefile | 1 +
> > > drivers/gpu/drm/panel/panel-raydium-rm67191.c | 645
> > > +
> > > 4 files changed, 710 insertions(+)
> > > create mode 100644
> > > Documentation/devicetree/bindings/display/panel/raydium,rm67191.t
> > > xt
> > > create mode 100644 drivers/gpu/drm/panel/panel-raydium-rm67191.c
> > >
> > > diff --git
> > > a/Documentation/devicetree/bindings/display/panel/raydium,rm67191
> > > .txt
> > > b/Documentation/devicetree/bindings/display/panel/raydium,rm67191
> > > .txt
> > > new file mode 100644
> > > index 000..18a57de
> > > --- /dev/null
> > > +++
> > > b/Documentation/devicetree/bindings/display/panel/raydium,rm67191
> > > .txt
> > > @@ -0,0 +1,55 @@
> > > +Raydium RM67171 OLED LCD panel with MIPI-DSI protocol
> > > +
> > > +Required properties:
> > > +- compatible: "raydium,rm67191"
> > > +- reg: virtual channel for MIPI-DSI
> > > protocol
> > > + must be <0>
> > > +- dsi-lanes: number of DSI lanes to be used
> > > + must be <3> or <4>
> > > +- port: input port node with endpoint definition
> > > as
> > > + defined in
> > > Documentation/devicetree/bindings/graph.txt;
> > > + the input port should be connected to a
> > > MIPI-DSI device
> > > + driver
> > > +
> > > +Optional properties:
> > > +- reset-gpio:a GPIO spec for the RST_B GPIO pin
> > > +- display-timings: timings for the connected panel according
> > > to [1]
> > > +- pinctrl-0 phandle to the pin settings for the reset
> > > pin
> > > +- panel-width-mm:physical panel width [mm]
> > > +- panel-height-mm: physical panel height [mm]
> > > +
> > > +[1]: Documentation/devicetree/bindings/display/display-
> > > timing.txt
> > > +
> > > +Example:
> > > +
> > > + panel@0 {
> > > + compatible = "raydium,rm67191";
> > > + reg = <0>;
> > > + pinctrl-0 = <&pinctrl_mipi_dsi_0_1_en>;
> > > + reset-gpio = <&gpio1 7 GPIO_ACTIVE_HIGH>;
> > > + dsi-lanes = <4>;
> > > + panel-w
Re: [PATCH 1/2] drm/panel: Add Raydium RM67191 DSI Panel
Hi Thierry,
Thanks a lot for reviewing this. Your suggestions are very valuable.
Please see my detailed answers inline.
Best regards,
Robert
From: Thierry Reding
Sent: Thursday, April 26, 2018 5:54 PM
To: Robert Chiras
Cc: dl-linux-imx; dri-devel@lists.freedesktop.org
Subject: Re: [PATCH 1/2] drm/panel: Add Raydium RM67191 DSI Panel
On Tue, Apr 03, 2018 at 01:30:00PM +0300, Robert Chiras wrote:
> Add support for the OLED display based on MIPI-DSI protocol from Raydium:
> RM67191.
>
> Signed-off-by: Robert Chiras
> ---
> .../bindings/display/panel/raydium,rm67191.txt | 55 ++
> drivers/gpu/drm/panel/Kconfig | 9 +
> drivers/gpu/drm/panel/Makefile | 1 +
> drivers/gpu/drm/panel/panel-raydium-rm67191.c | 645
> +
> 4 files changed, 710 insertions(+)
> create mode 100644
> Documentation/devicetree/bindings/display/panel/raydium,rm67191.txt
> create mode 100644 drivers/gpu/drm/panel/panel-raydium-rm67191.c
>
> diff --git
> a/Documentation/devicetree/bindings/display/panel/raydium,rm67191.txt
> b/Documentation/devicetree/bindings/display/panel/raydium,rm67191.txt
> new file mode 100644
> index 000..18a57de
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/panel/raydium,rm67191.txt
> @@ -0,0 +1,55 @@
> +Raydium RM67171 OLED LCD panel with MIPI-DSI protocol
> +
> +Required properties:
> +- compatible: "raydium,rm67191"
> +- reg: virtual channel for MIPI-DSI protocol
> + must be <0>
> +- dsi-lanes: number of DSI lanes to be used
> + must be <3> or <4>
> +- port: input port node with endpoint definition as
> + defined in Documentation/devicetree/bindings/graph.txt;
> + the input port should be connected to a MIPI-DSI device
> + driver
> +
> +Optional properties:
> +- reset-gpio:a GPIO spec for the RST_B GPIO pin
> +- display-timings: timings for the connected panel according to [1]
> +- pinctrl-0 phandle to the pin settings for the reset pin
> +- panel-width-mm:physical panel width [mm]
> +- panel-height-mm: physical panel height [mm]
> +
> +[1]: Documentation/devicetree/bindings/display/display-timing.txt
> +
> +Example:
> +
> + panel@0 {
> + compatible = "raydium,rm67191";
> + reg = <0>;
> + pinctrl-0 = <&pinctrl_mipi_dsi_0_1_en>;
> + reset-gpio = <&gpio1 7 GPIO_ACTIVE_HIGH>;
> + dsi-lanes = <4>;
> + panel-width-mm = <68>;
> + panel-height-mm = <121>;
> + display-timings {
> + timing {
> + clock-frequency = <13200>;
> + hactive = <1080>;
> + vactive = <1920>;
> + hback-porch = <11>;
> + hfront-porch = <4>;
> + vback-porch = <48>;
> + vfront-porch = <20>;
> + hsync-len = <5>;
> + vsync-len = <12>;
> + hsync-active = <0>;
> + vsync-active = <0>;
> + de-active = <0>;
> + pixelclk-active = <0>;
> + };
> + };
This shouldn't be necessary. You already have the timings in your
driver, why the extra work of getting it from DT?
[Robert] I added this as optional in the DT in case someone would like to use
the panel with different timings than ones from driver. Anyway, after some
tests I saw that the blanking timings are kind of fixed, so only the
clock-frequency could be changed. For example, if someone wants to use this
panel at 30Hz (66MHz pixel clock) instead of the default one of 60Hz (132MHz
pixel clock). But, I think you are right, I could get rid of the
display-timings and just add an optional property for changing the pixel clock
in driver.
> + port {
> + panel1_in: endpoint {
> + remote-endpoint = <&mipi1_out>;
> + };
> + };
> + };
Please split device tree bindings patches off into a separate patch and
make sure you Cc the devicet...@vger.kernel.org mailing list so that
they can be reviewed by the respective maintainers.
Also make sure that you put maintainers on To:
Re: [PATCH 1/2] drm/panel: Add Raydium RM67191 DSI Panel
On Fri, Apr 27, 2018 at 10:37:16AM +, Robert Chiras wrote:
>
> Hi Thierry,
>
> Thanks a lot for reviewing this. Your suggestions are very valuable.
> Please see my detailed answers inline.
>
> Best regards,
> Robert
Couple of comments regarding email replies. Please use proper quoting of
what you're replying to. Your mail user agent should be able to do that
automatically. If not, you might want to consider switching to a better
one.
Also, please try to stick to 78 columns of text. It's difficult to read
something that is wider than that and may get wrapped. It's also
difficult to reply to paragraphs that are too wide.
Other than than, some technical comments below.
>
> From: Thierry Reding
> Sent: Thursday, April 26, 2018 5:54 PM
> To: Robert Chiras
> Cc: dl-linux-imx; dri-devel@lists.freedesktop.org
> Subject: Re: [PATCH 1/2] drm/panel: Add Raydium RM67191 DSI Panel
>
> On Tue, Apr 03, 2018 at 01:30:00PM +0300, Robert Chiras wrote:
> > Add support for the OLED display based on MIPI-DSI protocol from Raydium:
> > RM67191.
> >
> > Signed-off-by: Robert Chiras
> > ---
> > .../bindings/display/panel/raydium,rm67191.txt | 55 ++
> > drivers/gpu/drm/panel/Kconfig | 9 +
> > drivers/gpu/drm/panel/Makefile | 1 +
> > drivers/gpu/drm/panel/panel-raydium-rm67191.c | 645
> > +
> > 4 files changed, 710 insertions(+)
> > create mode 100644
> > Documentation/devicetree/bindings/display/panel/raydium,rm67191.txt
> > create mode 100644 drivers/gpu/drm/panel/panel-raydium-rm67191.c
> >
> > diff --git
> > a/Documentation/devicetree/bindings/display/panel/raydium,rm67191.txt
> > b/Documentation/devicetree/bindings/display/panel/raydium,rm67191.txt
> > new file mode 100644
> > index 000..18a57de
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/display/panel/raydium,rm67191.txt
> > @@ -0,0 +1,55 @@
> > +Raydium RM67171 OLED LCD panel with MIPI-DSI protocol
> > +
> > +Required properties:
> > +- compatible: "raydium,rm67191"
> > +- reg: virtual channel for MIPI-DSI protocol
> > + must be <0>
> > +- dsi-lanes: number of DSI lanes to be used
> > + must be <3> or <4>
> > +- port: input port node with endpoint definition as
> > + defined in
> > Documentation/devicetree/bindings/graph.txt;
> > + the input port should be connected to a MIPI-DSI
> > device
> > + driver
> > +
> > +Optional properties:
> > +- reset-gpio:a GPIO spec for the RST_B GPIO pin
> > +- display-timings: timings for the connected panel according to [1]
> > +- pinctrl-0 phandle to the pin settings for the reset pin
> > +- panel-width-mm:physical panel width [mm]
> > +- panel-height-mm: physical panel height [mm]
> > +
> > +[1]: Documentation/devicetree/bindings/display/display-timing.txt
> > +
> > +Example:
> > +
> > + panel@0 {
> > + compatible = "raydium,rm67191";
> > + reg = <0>;
> > + pinctrl-0 = <&pinctrl_mipi_dsi_0_1_en>;
> > + reset-gpio = <&gpio1 7 GPIO_ACTIVE_HIGH>;
> > + dsi-lanes = <4>;
> > + panel-width-mm = <68>;
> > + panel-height-mm = <121>;
> > + display-timings {
> > + timing {
> > + clock-frequency = <13200>;
> > + hactive = <1080>;
> > + vactive = <1920>;
> > + hback-porch = <11>;
> > + hfront-porch = <4>;
> > + vback-porch = <48>;
> > + vfront-porch = <20>;
> > + hsync-len = <5>;
> > + vsync-len = <12>;
> > + hsync-active = <0>;
> > + vsync-active = <0>;
> > + de-active = <0>;
> > + pixelclk-active = <0>;
> > + };
> > + };
>
> This shouldn't be necessary. You already have the timings in your
> driver, why the extra work of gett
Re: [PATCH 1/2] drm/panel: Add Raydium RM67191 DSI Panel
On Tue, Apr 03, 2018 at 01:30:00PM +0300, Robert Chiras wrote:
> Add support for the OLED display based on MIPI-DSI protocol from Raydium:
> RM67191.
>
> Signed-off-by: Robert Chiras
> ---
> .../bindings/display/panel/raydium,rm67191.txt | 55 ++
> drivers/gpu/drm/panel/Kconfig | 9 +
> drivers/gpu/drm/panel/Makefile | 1 +
> drivers/gpu/drm/panel/panel-raydium-rm67191.c | 645
> +
> 4 files changed, 710 insertions(+)
> create mode 100644
> Documentation/devicetree/bindings/display/panel/raydium,rm67191.txt
> create mode 100644 drivers/gpu/drm/panel/panel-raydium-rm67191.c
>
> diff --git
> a/Documentation/devicetree/bindings/display/panel/raydium,rm67191.txt
> b/Documentation/devicetree/bindings/display/panel/raydium,rm67191.txt
> new file mode 100644
> index 000..18a57de
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/panel/raydium,rm67191.txt
> @@ -0,0 +1,55 @@
> +Raydium RM67171 OLED LCD panel with MIPI-DSI protocol
> +
> +Required properties:
> +- compatible:"raydium,rm67191"
> +- reg: virtual channel for MIPI-DSI protocol
> + must be <0>
> +- dsi-lanes: number of DSI lanes to be used
> + must be <3> or <4>
> +- port: input port node with endpoint definition as
> + defined in Documentation/devicetree/bindings/graph.txt;
> + the input port should be connected to a MIPI-DSI device
> + driver
> +
> +Optional properties:
> +- reset-gpio:a GPIO spec for the RST_B GPIO pin
> +- display-timings: timings for the connected panel according to [1]
> +- pinctrl-0 phandle to the pin settings for the reset pin
> +- panel-width-mm:physical panel width [mm]
> +- panel-height-mm: physical panel height [mm]
> +
> +[1]: Documentation/devicetree/bindings/display/display-timing.txt
> +
> +Example:
> +
> + panel@0 {
> + compatible = "raydium,rm67191";
> + reg = <0>;
> + pinctrl-0 = <&pinctrl_mipi_dsi_0_1_en>;
> + reset-gpio = <&gpio1 7 GPIO_ACTIVE_HIGH>;
> + dsi-lanes = <4>;
> + panel-width-mm = <68>;
> + panel-height-mm = <121>;
> + display-timings {
> + timing {
> + clock-frequency = <13200>;
> + hactive = <1080>;
> + vactive = <1920>;
> + hback-porch = <11>;
> + hfront-porch = <4>;
> + vback-porch = <48>;
> + vfront-porch = <20>;
> + hsync-len = <5>;
> + vsync-len = <12>;
> + hsync-active = <0>;
> + vsync-active = <0>;
> + de-active = <0>;
> + pixelclk-active = <0>;
> + };
> + };
This shouldn't be necessary. You already have the timings in your
driver, why the extra work of getting it from DT?
> + port {
> + panel1_in: endpoint {
> + remote-endpoint = <&mipi1_out>;
> + };
> + };
> + };
Please split device tree bindings patches off into a separate patch and
make sure you Cc the devicet...@vger.kernel.org mailing list so that
they can be reviewed by the respective maintainers.
Also make sure that you put maintainers on To: or at least Cc: so that
they have a better chance of seeing your patch and don't have to go
find them.
> diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
> index 6ba4031..769cba7 100644
> --- a/drivers/gpu/drm/panel/Kconfig
> +++ b/drivers/gpu/drm/panel/Kconfig
> @@ -158,4 +158,13 @@ config DRM_PANEL_SITRONIX_ST7789V
> Say Y here if you want to enable support for the Sitronix
> ST7789V controller for 240x320 LCD panels
>
> +config DRM_PANEL_RAYDIUM_RM67191
> + tristate "Raydium RM67191 FHD 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.
> +
These should be sorted alphabetically.
> endmenu
> diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
> index 6d251eb..838d5c6 100644
> --- a/drivers/gpu/drm/panel/Makefile
> +++ b/drivers/gpu/drm/panel/Makefile
> @@ -16,3 +16,4 @@ obj-$(CONFIG_DRM_PANEL_SEIKO_43WVF1G) +=
> panel-seiko-43wvf1g.o
> obj-$(CONFIG_DRM_PANEL_SHARP_LQ101R1SX01) += panel-sharp-lq101r1sx01.o
> obj-$(CONFIG_DRM_PANEL_SHARP_LS043T1LE01) += panel-sharp-ls043t1le01.o
> obj-$(CONFIG_DRM_PANEL_SITRONIX_ST7789V) += panel-sitronix-st7789v.o
> +ob
[PATCH 1/2] drm/panel: Add Raydium RM67191 DSI Panel
Add support for the OLED display based on MIPI-DSI protocol from Raydium:
RM67191.
Signed-off-by: Robert Chiras
---
.../bindings/display/panel/raydium,rm67191.txt | 55 ++
drivers/gpu/drm/panel/Kconfig | 9 +
drivers/gpu/drm/panel/Makefile | 1 +
drivers/gpu/drm/panel/panel-raydium-rm67191.c | 645 +
4 files changed, 710 insertions(+)
create mode 100644
Documentation/devicetree/bindings/display/panel/raydium,rm67191.txt
create mode 100644 drivers/gpu/drm/panel/panel-raydium-rm67191.c
diff --git
a/Documentation/devicetree/bindings/display/panel/raydium,rm67191.txt
b/Documentation/devicetree/bindings/display/panel/raydium,rm67191.txt
new file mode 100644
index 000..18a57de
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/panel/raydium,rm67191.txt
@@ -0,0 +1,55 @@
+Raydium RM67171 OLED LCD panel with MIPI-DSI protocol
+
+Required properties:
+- compatible: "raydium,rm67191"
+- reg: virtual channel for MIPI-DSI protocol
+ must be <0>
+- dsi-lanes: number of DSI lanes to be used
+ must be <3> or <4>
+- port:input port node with endpoint definition as
+ defined in Documentation/devicetree/bindings/graph.txt;
+ the input port should be connected to a MIPI-DSI device
+ driver
+
+Optional properties:
+- reset-gpio: a GPIO spec for the RST_B GPIO pin
+- display-timings: timings for the connected panel according to [1]
+- pinctrl-0phandle to the pin settings for the reset pin
+- panel-width-mm: physical panel width [mm]
+- panel-height-mm: physical panel height [mm]
+
+[1]: Documentation/devicetree/bindings/display/display-timing.txt
+
+Example:
+
+ panel@0 {
+ compatible = "raydium,rm67191";
+ reg = <0>;
+ pinctrl-0 = <&pinctrl_mipi_dsi_0_1_en>;
+ reset-gpio = <&gpio1 7 GPIO_ACTIVE_HIGH>;
+ dsi-lanes = <4>;
+ panel-width-mm = <68>;
+ panel-height-mm = <121>;
+ display-timings {
+ timing {
+ clock-frequency = <13200>;
+ hactive = <1080>;
+ vactive = <1920>;
+ hback-porch = <11>;
+ hfront-porch = <4>;
+ vback-porch = <48>;
+ vfront-porch = <20>;
+ hsync-len = <5>;
+ vsync-len = <12>;
+ hsync-active = <0>;
+ vsync-active = <0>;
+ de-active = <0>;
+ pixelclk-active = <0>;
+ };
+ };
+ port {
+ panel1_in: endpoint {
+ remote-endpoint = <&mipi1_out>;
+ };
+ };
+ };
diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
index 6ba4031..769cba7 100644
--- a/drivers/gpu/drm/panel/Kconfig
+++ b/drivers/gpu/drm/panel/Kconfig
@@ -158,4 +158,13 @@ config DRM_PANEL_SITRONIX_ST7789V
Say Y here if you want to enable support for the Sitronix
ST7789V controller for 240x320 LCD panels
+config DRM_PANEL_RAYDIUM_RM67191
+ tristate "Raydium RM67191 FHD 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.
+
endmenu
diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
index 6d251eb..838d5c6 100644
--- a/drivers/gpu/drm/panel/Makefile
+++ b/drivers/gpu/drm/panel/Makefile
@@ -16,3 +16,4 @@ obj-$(CONFIG_DRM_PANEL_SEIKO_43WVF1G) += panel-seiko-43wvf1g.o
obj-$(CONFIG_DRM_PANEL_SHARP_LQ101R1SX01) += panel-sharp-lq101r1sx01.o
obj-$(CONFIG_DRM_PANEL_SHARP_LS043T1LE01) += panel-sharp-ls043t1le01.o
obj-$(CONFIG_DRM_PANEL_SITRONIX_ST7789V) += panel-sitronix-st7789v.o
+obj-$(CONFIG_DRM_PANEL_RAYDIUM_RM67191) += panel-raydium-rm67191.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..07b0bd4
--- /dev/null
+++ b/drivers/gpu/drm/panel/panel-raydium-rm67191.c
@@ -0,0 +1,645 @@
+/*
+ * 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 i
