<html> <head> <meta http-equiv="Content-Type" content="text/html; charset=iso-8859-1"> <style type="text/css" style="display:none;"></style> </head> <body dir="ltr"> <div id="divtagdefaultwrapper" style="font-size:12pt;color:#000000;font-family:Calibri,Helvetica,sans-serif;" dir="ltr"> <div id="divtagdefaultwrapper" style="font-size: 12pt; color: rgb(0, 0, 0); font-family: Calibri, Helvetica, sans-serif, EmojiFont, "Apple Color Emoji", "Segoe UI Emoji", NotoColorEmoji, "Segoe UI Symbol", "Android Emoji", EmojiSymbols;" dir="ltr"> Hi Thierry,</div> <div id="divtagdefaultwrapper" style="font-size: 12pt; color: rgb(0, 0, 0); font-family: Calibri, Helvetica, sans-serif, EmojiFont, "Apple Color Emoji", "Segoe UI Emoji", NotoColorEmoji, "Segoe UI Symbol", "Android Emoji", EmojiSymbols;" dir="ltr"> </div> <div id="divtagdefaultwrapper" style="font-size: 12pt; color: rgb(0, 0, 0); font-family: Calibri, Helvetica, sans-serif, EmojiFont, "Apple Color Emoji", "Segoe UI Emoji", NotoColorEmoji, "Segoe UI Symbol", "Android Emoji", EmojiSymbols;" dir="ltr"> Thanks a lot for reviewing this. Your suggestions are very valuable.</div> <div id="divtagdefaultwrapper" style="font-size: 12pt; color: rgb(0, 0, 0); font-family: Calibri, Helvetica, sans-serif, EmojiFont, "Apple Color Emoji", "Segoe UI Emoji", NotoColorEmoji, "Segoe UI Symbol", "Android Emoji", EmojiSymbols;" dir="ltr"> Please see my detailed answers inline.</div> <div id="divtagdefaultwrapper" style="font-size: 12pt; color: rgb(0, 0, 0); font-family: Calibri, Helvetica, sans-serif, EmojiFont, "Apple Color Emoji", "Segoe UI Emoji", NotoColorEmoji, "Segoe UI Symbol", "Android Emoji", EmojiSymbols;" dir="ltr"> </div> <div id="divtagdefaultwrapper" style="font-size: 12pt; color: rgb(0, 0, 0); font-family: Calibri, Helvetica, sans-serif, EmojiFont, "Apple Color Emoji", "Segoe UI Emoji", NotoColorEmoji, "Segoe UI Symbol", "Android Emoji", EmojiSymbols;" dir="ltr"> Best regards,</div> <div id="divtagdefaultwrapper" style="font-size: 12pt; color: rgb(0, 0, 0); font-family: Calibri, Helvetica, sans-serif, EmojiFont, "Apple Color Emoji", "Segoe UI Emoji", NotoColorEmoji, "Segoe UI Symbol", "Android Emoji", EmojiSymbols;" dir="ltr"> Robert <div style="color: rgb(0, 0, 0);"> <hr style="display:inline-block;width:98%" tabindex="-1"> <div id="divRplyFwdMsg" dir="ltr">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 <div> </div> </div> <div class="BodyFragment"> <div class="PlainText">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? </div> <div class="PlainText">[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.</div> <div class="PlainText"> </div> <div class="PlainText">[Robert] Sure, will do that.</div> <div class="PlainText"> > 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.</div> <div class="PlainText">[Robert] Will do. </div> <div class="PlainText"> > 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.</div> <div class="PlainText">[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:</div> <div class="PlainText"> struct cmd_set_entry { u8 command; u8 param; }; static const struct cmd_set_entry manufacturer_cmd_set[] = { ... };</div> <div class="PlainText"> </div> <div class="PlainText">[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?)</div> <div class="PlainText"> > +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.</div> <div class="PlainText"> </div> <div class="PlainText">[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()...</div> <div class="PlainText"> </div> <div class="PlainText">[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.</div> <div class="PlainText"> </div> <div class="PlainText">[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?</div> <div class="PlainText"> </div> <div class="PlainText">[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.</div> <div class="PlainText"> </div> <div class="PlainText">[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.</div> <div class="PlainText"> </div> <div class="PlainText">[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?</div> <div class="PlainText"> </div> <div class="PlainText">[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 </div> </div> </div> </div> </div> </body> </html>