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 thierry.reding@gmail.com 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 robert.chiras@nxp.com

.../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 0000000..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 = <132000000>;
  

•                         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 devicetree@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.

[Robert] Sure, will do that.

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. [Robert] Will do.

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

Same here. [Robert] Will do.

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 0000000..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 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 <drm/drmP.h> +#include <drm/drm_crtc.h> +#include <drm/drm_mipi_dsi.h> +#include <drm/drm_panel.h> +#include <linux/gpio/consumer.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/regulator/consumer.h> +#include <video/mipi_display.h> +#include <video/of_videomode.h> +#include <video/videomode.h>

+#define CMD_TABLE_LEN 2 +typedef u8 cmd_set_table[CMD_TABLE_LEN];

This is all very confusing. The "table" isn't in fact 2 entries long, but each entry contains two bytes.

+/* Write Manufacture Command Set Control */ +#define WRMAUCCTR 0xFE

+/* Manufacturer Command Set pages (CMD2) */ +static const cmd_set_table manufacturer_cmd_set[] = {

There a lot of magic values in this table. Can you add a reference to where you got these from? Also, looking at these commands it seems like they are actually <command, parameter> pairs, so maybe a better representation would be something along the lines of:

struct cmd_set_entry { u8 command; u8 param; };

static const struct cmd_set_entry manufacturer_cmd_set[] = { ... };

[Robert] Your suggestion is good and I will change the code accordingly. Regarding the "magic values": I got them from a sample driver we received from the vendor. According to the Reference Manual, the IC on this panel has two sets of registers: Manufacture Command Set (MCS = CMD2) and User Command Set (UCS = CMD1). In the RM, there is no detailed description of the MCS, only a reference to a command on how to switch between these command sets. Now, what are those commands used in the MCS mode: we got no details for them (probably to protect their IP?)

+struct rad_panel {

• struct drm_panel base;
  

• 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 inline struct rad_panel *to_rad_panel(struct drm_panel *panel) +{

• return container_of(panel, struct rad_panel, base);
  

+}

+static int rad_panel_push_cmd_list(struct mipi_dsi_device *dsi) +{

• size_t i;
  

• const u8 *cmd;
  

• size_t count = sizeof(manufacturer_cmd_set) / CMD_TABLE_LEN;
  

• int ret = 0;
  

• for (i = 0; i < count ; i++) {
  

•         cmd = manufacturer_cmd_set[i];
  

•         ret = mipi_dsi_generic_write(dsi, cmd, CMD_TABLE_LEN);
  

•         if (ret < 0)
  

•                 return ret;
  

• }
  

• return ret;
  

+};

With the changes I suggested above, this simply becomes:

for (i = 0; i < ARRAY_SIZE(manufacturer_cmd_set); i++) { const struct cmd_set_entry *entry = &manufacturer_cmd_set[i]; u8 buffer[2] = { entry->cmd, entry->param };

ret = mipi_dsi_generic_write(dsi, buffer, sizeof(buffer)); if (ret < 0) return ret; }

which is about the same length but a lot more idiomatic.

[Robert] Sure, will do this too. Thanks!

+static int rad_panel_prepare(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 ret;
  

• if (rad->prepared)
  

•         return 0;
  

• DRM_DEV_DEBUG_DRIVER(dev, "\n");
  

• if (rad->reset != NULL) {
  

•         gpiod_set_value(rad->reset, 1);
  

•         usleep_range(10000, 15000);
  

•         gpiod_set_value(rad->reset, 0);
  

•         usleep_range(5000, 10000);
  

•         gpiod_set_value(rad->reset, 1);
  

•         usleep_range(20000, 25000);
  

• }
  

• 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(10000, 15000);
  

• /* 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 to RGB888 */
  

• ret = mipi_dsi_dcs_set_pixel_format(dsi, 0x77);
  

• if (ret < 0) {
  

•         DRM_DEV_ERROR(dev, "Failed to set pixel format (%d)\n", ret);
  

•         goto fail;
  

• }
  

• /* Set display brightness */
  

• ret = mipi_dsi_dcs_set_display_brightness(dsi, 0x20);
  

• if (ret < 0) {
  

•         DRM_DEV_ERROR(dev, "Failed to set display brightness (%d)\n",
  

•                       ret);
  

•         goto fail;
  

• }
  

• /* Exit sleep mode */
  

• ret = mipi_dsi_dcs_exit_sleep_mode(dsi);
  

• if (ret < 0) {
  

•         DRM_DEV_ERROR(dev, "Failed to exit sleep mode (%d)\n", ret);
  

•         goto fail;
  

• }
  

• usleep_range(5000, 10000);
  

• ret = mipi_dsi_dcs_set_display_on(dsi);
  

• if (ret < 0) {
  

•         DRM_DEV_ERROR(dev, "Failed to set display ON (%d)\n", ret);
  

•         goto fail;
  

• }
  

• rad->prepared = true;
  

• return 0;
  

+fail:

• if (rad->reset != NULL)
  

•         gpiod_set_value(rad->reset, 0);
  

• return ret;
  

+}

+static int rad_panel_unprepare(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 ret;
  

• if (!rad->prepared)
  

•         return 0;
  

• DRM_DEV_DEBUG_DRIVER(dev, "\n");
  

• dsi->mode_flags |= MIPI_DSI_MODE_LPM;
  

• ret = mipi_dsi_dcs_set_display_off(dsi);
  

• if (ret < 0)
  

•         DRM_DEV_ERROR(dev, "Failed to set display OFF (%d)\n", ret);
  

• usleep_range(5000, 10000);
  

• ret = mipi_dsi_dcs_enter_sleep_mode(dsi);
  

• if (ret < 0)
  

•         DRM_DEV_ERROR(dev, "Failed to enter sleep mode (%d)\n", ret);
  

• usleep_range(10000, 15000);
  

• if (rad->reset != NULL) {
  

•         gpiod_set_value(rad->reset, 0);
  

•         usleep_range(10000, 15000);
  

• }
  

• rad->prepared = false;
  

• return 0;
  

+}

+static int rad_panel_enable(struct drm_panel *panel) +{

• struct rad_panel *rad = to_rad_panel(panel);
  

• struct device *dev = &rad->dsi->dev;
  

• if (rad->enabled)
  

•         return 0;
  

• DRM_DEV_DEBUG_DRIVER(dev, "\n");
  

• rad->backlight->props.power = FB_BLANK_UNBLANK;
  

• backlight_update_status(rad->backlight);
  

Please use the new backlight_enable()...

[Robert] Will do.

• rad->enabled = true;
  

• return 0;
  

+}

+static int rad_panel_disable(struct drm_panel *panel) +{

• struct rad_panel *rad = to_rad_panel(panel);
  

• struct device *dev = &rad->dsi->dev;
  

• if (!rad->enabled)
  

•         return 0;
  

• DRM_DEV_DEBUG_DRIVER(dev, "\n");
  

• rad->backlight->props.power = FB_BLANK_POWERDOWN;
  

• backlight_update_status(rad->backlight);
  

... and backlight_disable() functions.

[Robert] Same here.

• rad->enabled = false;
  

• return 0;
  

+}

+static int rad_panel_get_modes(struct drm_panel *panel) +{

• struct rad_panel *rad = to_rad_panel(panel);
  

• struct device *dev = &rad->dsi->dev;
  

• struct drm_connector *connector = panel->connector;
  

• struct drm_display_mode *mode;
  

• u32 bus_format = MEDIA_BUS_FMT_RGB888_1X24;
  

• u32 *bus_flags = &connector->display_info.bus_flags;
  

• int ret;
  

• mode = drm_mode_create(connector->dev);
  

• if (!mode) {
  

•         DRM_DEV_ERROR(dev, "Failed to create display mode!\n");
  

•         return 0;
  

• }
  

• drm_display_mode_from_videomode(&rad->vm, mode);
  

• mode->width_mm = rad->width_mm;
  

• mode->height_mm = rad->height_mm;
  

• connector->display_info.width_mm = rad->width_mm;
  

• connector->display_info.height_mm = rad->height_mm;
  

• mode->type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED;
  

• if (rad->vm.flags & DISPLAY_FLAGS_DE_HIGH)
  

•         *bus_flags |= DRM_BUS_FLAG_DE_HIGH;
  

• if (rad->vm.flags & DISPLAY_FLAGS_DE_LOW)
  

•         *bus_flags |= DRM_BUS_FLAG_DE_LOW;
  

• if (rad->vm.flags & DISPLAY_FLAGS_PIXDATA_NEGEDGE)
  

•         *bus_flags |= DRM_BUS_FLAG_PIXDATA_NEGEDGE;
  

• if (rad->vm.flags & DISPLAY_FLAGS_PIXDATA_POSEDGE)
  

•         *bus_flags |= DRM_BUS_FLAG_PIXDATA_POSEDGE;
  

• ret = drm_display_info_set_bus_formats(&connector->display_info,
  

•                                        &bus_format, 1);
  

• if (ret)
  

•         return ret;
  

• drm_mode_probed_add(panel->connector, mode);
  

• return 1;
  

+}

+static int rad_bl_get_brightness(struct backlight_device *bl) +{

• struct mipi_dsi_device *dsi = bl_get_data(bl);
  

• struct rad_panel *rad = mipi_dsi_get_drvdata(dsi);
  

• struct device *dev = &dsi->dev;
  

• u16 brightness;
  

• int ret;
  

• if (!rad->prepared)
  

•         return 0;
  

• DRM_DEV_DEBUG_DRIVER(dev, "\n");
  

• dsi->mode_flags &= ~MIPI_DSI_MODE_LPM;
  

• ret = mipi_dsi_dcs_get_display_brightness(dsi, &brightness);
  

• if (ret < 0)
  

•         return ret;
  

• bl->props.brightness = brightness;
  

• return brightness & 0xff;
  

+}

+static int rad_bl_update_status(struct backlight_device *bl) +{

• struct mipi_dsi_device *dsi = bl_get_data(bl);
  

• struct rad_panel *rad = mipi_dsi_get_drvdata(dsi);
  

• struct device *dev = &dsi->dev;
  

• int ret = 0;
  

• if (!rad->prepared)
  

•         return 0;
  

• DRM_DEV_DEBUG_DRIVER(dev, "New brightness: %d\n", bl->props.brightness);
  

• dsi->mode_flags &= ~MIPI_DSI_MODE_LPM;
  

• ret = mipi_dsi_dcs_set_display_brightness(dsi, bl->props.brightness);
  

• if (ret < 0)
  

•         return ret;
  

• return 0;
  

+}

+static const struct backlight_ops rad_bl_ops = {

• .update_status = rad_bl_update_status,
  

• .get_brightness = rad_bl_get_brightness,
  

+};

+static const struct drm_panel_funcs rad_panel_funcs = {

• .prepare = rad_panel_prepare,
  

• .unprepare = rad_panel_unprepare,
  

• .enable = rad_panel_enable,
  

• .disable = rad_panel_disable,
  

• .get_modes = rad_panel_get_modes,
  

+};

+/*

• • The clock might range from 66MHz (30Hz refresh rate)
• • to 132MHz (60Hz refresh rate)
• */

+static const struct display_timing rad_default_timing = {

• .pixelclock = { 66000000, 120000000, 132000000 },
  

• .hactive = { 1080, 1080, 1080 },
  

• .hfront_porch = { 20, 20, 20 },
  

• .hsync_len = { 2, 2, 2 },
  

• .hback_porch = { 34, 34, 34 },
  

• .vactive = { 1920, 1920, 1920 },
  

• .vfront_porch = { 10, 10, 10 },
  

• .vsync_len = { 2, 2, 2 },
  

• .vback_porch = { 4, 4, 4 },
  

• .flags = DISPLAY_FLAGS_HSYNC_LOW |
  

•          DISPLAY_FLAGS_VSYNC_LOW |
  

•          DISPLAY_FLAGS_DE_LOW |
  

•          DISPLAY_FLAGS_PIXDATA_NEGEDGE,
  

+};

+static int rad_panel_probe(struct mipi_dsi_device *dsi) +{

• struct device *dev = &dsi->dev;
  

• struct device_node *np = dev->of_node;
  

• struct device_node *timings;
  

• struct rad_panel *panel;
  

• struct backlight_properties bl_props;
  

• int ret;
  

• panel = devm_kzalloc(&dsi->dev, sizeof(*panel), GFP_KERNEL);
  

• if (!panel)
  

•         return -ENOMEM;
  

• mipi_dsi_set_drvdata(dsi, panel);
  

• panel->dsi = dsi;
  

• dsi->format = MIPI_DSI_FMT_RGB888;
  

• dsi->mode_flags =  MIPI_DSI_MODE_VIDEO_HSE | MIPI_DSI_MODE_VIDEO |
  

•                    MIPI_DSI_CLOCK_NON_CONTINUOUS;
  

• ret = of_property_read_u32(np, "dsi-lanes", &dsi->lanes);
  

• if (ret < 0) {
  

•         dev_err(dev, "Failed to get dsi-lanes property (%d)\n", ret);
  

•         return ret;
  

• }
  

• /*
  

•  * 'display-timings' is optional, so verify if the node is present
  

•  * before calling of_get_videomode so we won't get console error
  

•  * messages
  

•  */
  

• timings = of_get_child_by_name(np, "display-timings");
  

• if (timings) {
  

•         of_node_put(timings);
  

•         ret = of_get_videomode(np, &panel->vm, 0);
  

• } else {
  

•         videomode_from_timing(&rad_default_timing, &panel->vm);
  

• }
  

• if (ret < 0)
  

•         return ret;
  

• of_property_read_u32(np, "panel-width-mm", &panel->width_mm);
  

• of_property_read_u32(np, "panel-height-mm", &panel->height_mm);
  

• panel->reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
  

• if (IS_ERR(panel->reset))
  

•         panel->reset = NULL;
  

• else
  

•         gpiod_set_value(panel->reset, 0);
  

• memset(&bl_props, 0, sizeof(bl_props));
  

• bl_props.type = BACKLIGHT_RAW;
  

• bl_props.brightness = 255;
  

Maybe this should read the current brightness from the panel to make sure it's correct?

[Robert] In order to read the current brightness from the panel, we need to be able to send DSI commands. Since the DSI commands can be sent by means of the DSI host controller, at this moment is impossible to do this. Anyway, during the panel_prepare, when the panel is initialized, the backlight value is defaulted there, so I can update the current brightness there.

• bl_props.max_brightness = 255;
  

• panel->backlight = devm_backlight_device_register(
  

•                         dev, dev_name(dev),
  

•                         dev, dsi,
  

•                         &rad_bl_ops, &bl_props);
  

• 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->base);
  

• panel->base.funcs = &rad_panel_funcs;
  

• panel->base.dev = dev;
  

• ret = drm_panel_add(&panel->base);
  

• if (ret < 0)
  

•         return ret;
  

• ret = mipi_dsi_attach(dsi);
  

• if (ret < 0)
  

•         drm_panel_remove(&panel->base);
  

• return ret;
  

+}

+static int rad_panel_remove(struct mipi_dsi_device *dsi) +{

• struct rad_panel *rad = mipi_dsi_get_drvdata(dsi);
  

• struct device *dev = &dsi->dev;
  

• int ret;
  

• ret = rad_panel_unprepare(&rad->base);
  

• ret |= rad_panel_disable(&rad->base);
  

• if (ret < 0)
  

•         DRM_DEV_ERROR(dev, "Failed to disable panel (%d)\n", ret);
  

This is the wrong way around. First disable, then unprepare. Also, this should not be necessary because the panel should've been disabled by the time you get here.

[Robert] Oh, you're right here. I will remove them.

• ret = mipi_dsi_detach(dsi);
  

• if (ret < 0)
  

•         DRM_DEV_ERROR(dev, "Failed to detach from host (%d)\n",
  

•                 ret);
  

• drm_panel_detach(&rad->base);
  

Please don't do that. I've just merged a set of patches which document this as being wrong and which remove similar calls from other panel drivers.

[Robert] I am sorry, but I don't understand here. Don't do what? mipi_dsi_detach or drm_panel_detach? Or are you referring to the fact that I'm still calling drm_panel_detach even though mipi_dsi_detach fails?

• if (rad->base.dev)
  

•         drm_panel_remove(&rad->base);
  

Why this check? Under what circumstances would this fail?

[Robert] Well, I thought that if probe fails, base will be NULL, but you are right about this. If probe fails there won't be any remove calls. Will fix this too.

Thierry

On Vi, 2018-04-27 at 11:08 +0000, Thierry Reding wrote:

On Fri, Apr 27, 2018 at 10:37:16AM +0000, 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 thierry.reding@gmail.com 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 robert.chiras@nxp.com

.../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 0000000..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 = <132000000>; +                             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.

To be honest, I wouldn't bother with that. Let someone else add that if they want to drive the panel at a different mode. Chances are nobody will, in which case adding it now would just be dead code that we need to maintain for no good reason.

[...]

...

...

+/* Write Manufacture Command Set Control */ +#define WRMAUCCTR 0xFE

+/* Manufacturer Command Set pages (CMD2) */ +static const cmd_set_table manufacturer_cmd_set[] = {

There a lot of magic values in this table. Can you add a reference to where you got these from? Also, looking at these commands it seems like they are actually <command, parameter> pairs, so maybe a better representation would be something along the lines of:

struct cmd_set_entry {                 u8 command;                 u8 param;         };

static const struct cmd_set_entry manufacturer_cmd_set[] = {                 ...         };

[Robert] Your suggestion is good and I will change the code accordingly. Regarding the "magic values": I got them from a sample driver we received from the vendor. According to the Reference Manual, the IC on this panel has two sets of registers: Manufacture Command Set (MCS = CMD2) and User Command Set (UCS = CMD1). In the RM, there is no detailed description of the MCS, only a reference to a command on how to switch between these command sets. Now, what are those commands used in the MCS mode: we got no details for them (probably to protect their IP?)

Okay, not much you can do about it, then.

...

...

+static const struct backlight_ops rad_bl_ops = { +     .update_status = rad_bl_update_status, +     .get_brightness = rad_bl_get_brightness, +};

[...]

...

...

+static int rad_panel_probe(struct mipi_dsi_device *dsi) +{

[...]

...

...

+     memset(&bl_props, 0, sizeof(bl_props)); +     bl_props.type = BACKLIGHT_RAW; +     bl_props.brightness = 255;

Maybe this should read the current brightness from the panel to make sure it's correct?

[Robert] In order to read the current brightness from the panel, we need to be able to send DSI commands. Since the DSI commands can be sent by means of the DSI host controller, at this moment is impossible to do this. Anyway, during the panel_prepare, when the panel is initialized, the backlight value is defaulted there, so I can update the current brightness there.

Couldn't you do it right after the call to mipi_dsi_attach()? It seems like the panel should be able to execute DSI commands at that time. It may have to call ->prepare() and ->unprepare() around those, though.

I guess another possibility would be to do it as part of the ->prepare() callback.

Looking a little closer at drivers/video/backlight/backlight.c it seems like maybe even that wouldn't be necessary. Drivers that implement the ->get_brightness() callback don't really need to set .brightness, so maybe just drop that line?

...

...

+     ret = mipi_dsi_detach(dsi); +     if (ret < 0) +             DRM_DEV_ERROR(dev, "Failed to detach from host (%d)\n", +                     ret);

+     drm_panel_detach(&rad->base);

Please don't do that. I've just merged a set of patches which document this as being wrong and which remove similar calls from other panel drivers.

[Robert] I am sorry, but I don't understand here. Don't do what? mipi_dsi_detach or drm_panel_detach? Or are you referring to the fact that I'm still calling drm_panel_detach even though mipi_dsi_detach fails?

drm_panel_detach() should not be called from panel drivers. It should be called from display drivers to detach from the panel.

Thierry

On Tue, Apr 03, 2018 at 01:30:01PM +0300, Robert Chiras wrote:

From: Mirela Rabulea mirela.rabulea@nxp.com

Do not hardcode pixel_format to 0x77 but calculate it from dsi->format. Report all the supported bus formats in get_modes: MEDIA_BUS_FMT_RGB888_1X24 MEDIA_BUS_FMT_RGB666_1X18 MEDIA_BUS_FMT_RGB565_1X16 Change pixelclock from 120 to 132 MHz, or 16 bpp formats will not work.

Signed-off-by: Mirela Rabulea mirela.rabulea@nxp.com

drivers/gpu/drm/panel/panel-raydium-rm67191.c | 33 +++++++++++++++++++++++---- 1 file changed, 28 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-raydium-rm67191.c b/drivers/gpu/drm/panel/panel-raydium-rm67191.c index 07b0bd4..6331fef 100644 --- a/drivers/gpu/drm/panel/panel-raydium-rm67191.c +++ b/drivers/gpu/drm/panel/panel-raydium-rm67191.c @@ -178,6 +178,12 @@ static const cmd_set_table manufacturer_cmd_set[] = { {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; struct mipi_dsi_device *dsi; @@ -215,11 +221,27 @@ static int rad_panel_push_cmd_list(struct mipi_dsi_device *dsi) return ret; };

+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;
  

• default:

• return 0x77; /* for backward compatibility */
  

• }

+};

static int rad_panel_prepare(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); int ret;

  if (rad->prepared)

@@ -276,8 +298,10 @@ static int rad_panel_prepare(struct drm_panel *panel) DRM_DEV_ERROR(dev, "Failed to set tear scanline (%d)\n", ret); goto fail; }

• /* Set pixel format to RGB888 */
• ret = mipi_dsi_dcs_set_pixel_format(dsi, 0x77);

• /* 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;

@@ -393,7 +417,6 @@ static int rad_panel_get_modes(struct drm_panel *panel) struct device *dev = &rad->dsi->dev; struct drm_connector *connector = panel->connector; struct drm_display_mode *mode;

• u32 bus_format = MEDIA_BUS_FMT_RGB888_1X24; u32 *bus_flags = &connector->display_info.bus_flags; int ret;

@@ -420,7 +443,7 @@ static int rad_panel_get_modes(struct drm_panel *panel) *bus_flags |= DRM_BUS_FLAG_PIXDATA_POSEDGE;

ret = drm_display_info_set_bus_formats(&connector->display_info,

• 			       &bus_format, 1);
  

• 	rad_bus_formats, ARRAY_SIZE(rad_bus_formats));
  

  if (ret) return ret;

@@ -492,7 +515,7 @@ static const struct drm_panel_funcs rad_panel_funcs = {

• to 132MHz (60Hz refresh rate)

*/ static const struct display_timing rad_default_timing = {

• .pixelclock = { 66000000, 120000000, 132000000 },

• .pixelclock = { 66000000, 132000000, 132000000 },

This seems like a fix that should be squashed into patch 1.

Thierry

On Fri, Apr 27, 2018 at 10:38:24AM +0000, Robert Chiras wrote:

---

From: Thierry Reding thierry.reding@gmail.com Sent: Thursday, April 26, 2018 5:56 PM To: Robert Chiras Cc: dl-linux-imx; dri-devel@lists.freedesktop.org Subject: Re: [PATCH 2/2] drm/panel: rm67191: Add support for new bus formats

On Tue, Apr 03, 2018 at 01:30:01PM +0300, Robert Chiras wrote:

...

From: Mirela Rabulea mirela.rabulea@nxp.com

Do not hardcode pixel_format to 0x77 but calculate it from dsi->format. Report all the supported bus formats in get_modes: MEDIA_BUS_FMT_RGB888_1X24 MEDIA_BUS_FMT_RGB666_1X18 MEDIA_BUS_FMT_RGB565_1X16 Change pixelclock from 120 to 132 MHz, or 16 bpp formats will not work.

Signed-off-by: Mirela Rabulea mirela.rabulea@nxp.com

drivers/gpu/drm/panel/panel-raydium-rm67191.c | 33 +++++++++++++++++++++++---- 1 file changed, 28 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-raydium-rm67191.c b/drivers/gpu/drm/panel/panel-raydium-rm67191.c index 07b0bd4..6331fef 100644 --- a/drivers/gpu/drm/panel/panel-raydium-rm67191.c +++ b/drivers/gpu/drm/panel/panel-raydium-rm67191.c @@ -178,6 +178,12 @@ static const cmd_set_table manufacturer_cmd_set[] = { {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; struct mipi_dsi_device *dsi; @@ -215,11 +221,27 @@ static int rad_panel_push_cmd_list(struct mipi_dsi_device *dsi) return ret; };

+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;
  

• default:
  

•         return 0x77; /* for backward compatibility */
  

• }
  

+};

static int rad_panel_prepare(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);
   int ret;
  
   if (rad->prepared)
  

@@ -276,8 +298,10 @@ static int rad_panel_prepare(struct drm_panel *panel) DRM_DEV_ERROR(dev, "Failed to set tear scanline (%d)\n", ret); goto fail; }

• /* Set pixel format to RGB888 */
  

• ret = mipi_dsi_dcs_set_pixel_format(dsi, 0x77);
  

• /* 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;
  

@@ -393,7 +417,6 @@ static int rad_panel_get_modes(struct drm_panel *panel) struct device *dev = &rad->dsi->dev; struct drm_connector *connector = panel->connector; struct drm_display_mode *mode;

• u32 bus_format = MEDIA_BUS_FMT_RGB888_1X24;
   u32 *bus_flags = &connector->display_info.bus_flags;
   int ret;
  
  

@@ -420,7 +443,7 @@ static int rad_panel_get_modes(struct drm_panel *panel) *bus_flags |= DRM_BUS_FLAG_PIXDATA_POSEDGE;

   ret = drm_display_info_set_bus_formats(&connector->display_info,

•                                        &bus_format, 1);
  

•                 rad_bus_formats, ARRAY_SIZE(rad_bus_formats));
   if (ret)
           return ret;
  
  

@@ -492,7 +515,7 @@ static const struct drm_panel_funcs rad_panel_funcs = {

• to 132MHz (60Hz refresh rate)

*/ static const struct display_timing rad_default_timing = {

• .pixelclock = { 66000000, 120000000, 132000000 },
  

• .pixelclock = { 66000000, 132000000, 132000000 },
  

This seems like a fix that should be squashed into patch 1.

[Robert] I did that to preserve the author, but I can squash it into patch 1 and add her there.

If you want to preserve authorship, just split it off into a separate patch. We don't usually do that for initial submissions of a driver, so you can usually squash such subsequent fixes into the initial patch for an initial submission and mention contributors in the commit message. I don't have any strong objections if you want to split the patch out separately, though.

Thierry