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.red...@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.chi...@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 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.
[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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
