From mboxrd@z Thu Jan  1 00:00:00 1970
From: Thierry Reding <thierry.reding@gmail.com>
Subject: Re: [PATCH 1/2] drm/panel: Add Raydium RM67191 DSI Panel
Date: Thu, 26 Apr 2018 16:54:40 +0200
Message-ID: <20180426145440.GD31888@ulmo>
References: <1522751401-3381-1-git-send-email-robert.chiras@nxp.com>
 <1522751401-3381-2-git-send-email-robert.chiras@nxp.com>
Mime-Version: 1.0
Content-Type: multipart/mixed; boundary="===============0698156946=="
Return-path: <dri-devel-bounces@lists.freedesktop.org>
Received: from mail-wm0-x244.google.com (mail-wm0-x244.google.com
 [IPv6:2a00:1450:400c:c09::244])
 by gabe.freedesktop.org (Postfix) with ESMTPS id 780126E70E
 for <dri-devel@lists.freedesktop.org>; Thu, 26 Apr 2018 14:54:44 +0000 (UTC)
Received: by mail-wm0-x244.google.com with SMTP id t11so6631667wmt.0
 for <dri-devel@lists.freedesktop.org>; Thu, 26 Apr 2018 07:54:44 -0700 (PDT)
In-Reply-To: <1522751401-3381-2-git-send-email-robert.chiras@nxp.com>
List-Unsubscribe: <https://lists.freedesktop.org/mailman/options/dri-devel>,
 <mailto:dri-devel-request@lists.freedesktop.org?subject=unsubscribe>
List-Archive: <https://lists.freedesktop.org/archives/dri-devel>
List-Post: <mailto:dri-devel@lists.freedesktop.org>
List-Help: <mailto:dri-devel-request@lists.freedesktop.org?subject=help>
List-Subscribe: <https://lists.freedesktop.org/mailman/listinfo/dri-devel>,
 <mailto:dri-devel-request@lists.freedesktop.org?subject=subscribe>
Errors-To: dri-devel-bounces@lists.freedesktop.org
Sender: "dri-devel" <dri-devel-bounces@lists.freedesktop.org>
To: Robert Chiras <robert.chiras@nxp.com>
Cc: linux-imx@nxp.com, dri-devel@lists.freedesktop.org
List-Id: dri-devel@lists.freedesktop.org


--===============0698156946==
Content-Type: multipart/signed; micalg=pgp-sha256;
	protocol="application/pgp-signature"; boundary="Q0rSlbzrZN6k9QnT"
Content-Disposition: inline


--Q0rSlbzrZN6k9QnT
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

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.
>=20
> 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/raydi=
um,rm67191.txt
>  create mode 100644 drivers/gpu/drm/panel/panel-raydium-rm67191.c
>=20
> diff --git a/Documentation/devicetree/bindings/display/panel/raydium,rm67=
191.txt b/Documentation/devicetree/bindings/display/panel/raydium,rm67191.t=
xt
> 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 =3D "raydium,rm67191";
> +		reg =3D <0>;
> +		pinctrl-0 =3D <&pinctrl_mipi_dsi_0_1_en>;
> +		reset-gpio =3D <&gpio1 7 GPIO_ACTIVE_HIGH>;
> +		dsi-lanes =3D <4>;
> +		panel-width-mm =3D <68>;
> +		panel-height-mm =3D <121>;
> +		display-timings {
> +			timing {
> +				clock-frequency =3D <132000000>;
> +				hactive =3D <1080>;
> +				vactive =3D <1920>;
> +				hback-porch =3D <11>;
> +				hfront-porch =3D <4>;
> +				vback-porch =3D <48>;
> +				vfront-porch =3D <20>;
> +				hsync-len =3D <5>;
> +				vsync-len =3D <12>;
> +				hsync-active =3D <0>;
> +				vsync-active =3D <0>;
> +				de-active =3D <0>;
> +				pixelclk-active =3D <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 =3D <&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.

> 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
> =20
> +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/Makef=
ile
> 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) +=3D panel-seiko-=
43wvf1g.o
>  obj-$(CONFIG_DRM_PANEL_SHARP_LQ101R1SX01) +=3D panel-sharp-lq101r1sx01.o
>  obj-$(CONFIG_DRM_PANEL_SHARP_LS043T1LE01) +=3D panel-sharp-ls043t1le01.o
>  obj-$(CONFIG_DRM_PANEL_SITRONIX_ST7789V) +=3D panel-sitronix-st7789v.o
> +obj-$(CONFIG_DRM_PANEL_RAYDIUM_RM67191) +=3D panel-raydium-rm67191.o

Same here.

> 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[] =3D {

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[] =3D {
		...
	};

> +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 =3D sizeof(manufacturer_cmd_set) / CMD_TABLE_LEN;
> +	int ret =3D 0;
> +
> +	for (i =3D 0; i < count ; i++) {
> +		cmd =3D manufacturer_cmd_set[i];
> +		ret =3D 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 =3D 0; i < ARRAY_SIZE(manufacturer_cmd_set); i++) {
		const struct cmd_set_entry *entry =3D &manufacturer_cmd_set[i];
		u8 buffer[2] =3D { entry->cmd, entry->param };

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

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

> +static int rad_panel_prepare(struct drm_panel *panel)
> +{
> +	struct rad_panel *rad =3D to_rad_panel(panel);
> +	struct mipi_dsi_device *dsi =3D rad->dsi;
> +	struct device *dev =3D &dsi->dev;
> +	int ret;
> +
> +	if (rad->prepared)
> +		return 0;
> +
> +	DRM_DEV_DEBUG_DRIVER(dev, "\n");
> +
> +	if (rad->reset !=3D 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 |=3D MIPI_DSI_MODE_LPM;
> +
> +	ret =3D 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 =3D mipi_dsi_generic_write(dsi, (u8[]){ WRMAUCCTR, 0x00 }, 2);
> +	if (ret < 0)
> +		goto fail;
> +
> +	/* Software reset */
> +	ret =3D 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 =3D 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 =3D 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 =3D 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 =3D 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 =3D 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 =3D 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 =3D 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 =3D true;
> +
> +	return 0;
> +
> +fail:
> +	if (rad->reset !=3D NULL)
> +		gpiod_set_value(rad->reset, 0);
> +
> +	return ret;
> +}
> +
> +static int rad_panel_unprepare(struct drm_panel *panel)
> +{
> +	struct rad_panel *rad =3D to_rad_panel(panel);
> +	struct mipi_dsi_device *dsi =3D rad->dsi;
> +	struct device *dev =3D &dsi->dev;
> +	int ret;
> +
> +	if (!rad->prepared)
> +		return 0;
> +
> +	DRM_DEV_DEBUG_DRIVER(dev, "\n");
> +
> +	dsi->mode_flags |=3D MIPI_DSI_MODE_LPM;
> +
> +	ret =3D 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 =3D 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 !=3D NULL) {
> +		gpiod_set_value(rad->reset, 0);
> +		usleep_range(10000, 15000);
> +	}
> +
> +	rad->prepared =3D false;
> +
> +	return 0;
> +}
> +
> +static int rad_panel_enable(struct drm_panel *panel)
> +{
> +	struct rad_panel *rad =3D to_rad_panel(panel);
> +	struct device *dev =3D &rad->dsi->dev;
> +
> +	if (rad->enabled)
> +		return 0;
> +
> +	DRM_DEV_DEBUG_DRIVER(dev, "\n");
> +
> +	rad->backlight->props.power =3D FB_BLANK_UNBLANK;
> +	backlight_update_status(rad->backlight);

Please use the new backlight_enable()...

> +
> +	rad->enabled =3D true;
> +
> +	return 0;
> +}
> +
> +static int rad_panel_disable(struct drm_panel *panel)
> +{
> +	struct rad_panel *rad =3D to_rad_panel(panel);
> +	struct device *dev =3D &rad->dsi->dev;
> +
> +	if (!rad->enabled)
> +		return 0;
> +
> +	DRM_DEV_DEBUG_DRIVER(dev, "\n");
> +
> +	rad->backlight->props.power =3D FB_BLANK_POWERDOWN;
> +	backlight_update_status(rad->backlight);

=2E.. and backlight_disable() functions.

> +
> +	rad->enabled =3D false;
> +
> +	return 0;
> +}
> +
> +static int rad_panel_get_modes(struct drm_panel *panel)
> +{
> +	struct rad_panel *rad =3D to_rad_panel(panel);
> +	struct device *dev =3D &rad->dsi->dev;
> +	struct drm_connector *connector =3D panel->connector;
> +	struct drm_display_mode *mode;
> +	u32 bus_format =3D MEDIA_BUS_FMT_RGB888_1X24;
> +	u32 *bus_flags =3D &connector->display_info.bus_flags;
> +	int ret;
> +
> +	mode =3D 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 =3D rad->width_mm;
> +	mode->height_mm =3D rad->height_mm;
> +	connector->display_info.width_mm =3D rad->width_mm;
> +	connector->display_info.height_mm =3D rad->height_mm;
> +	mode->type =3D DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED;
> +
> +	if (rad->vm.flags & DISPLAY_FLAGS_DE_HIGH)
> +		*bus_flags |=3D DRM_BUS_FLAG_DE_HIGH;
> +	if (rad->vm.flags & DISPLAY_FLAGS_DE_LOW)
> +		*bus_flags |=3D DRM_BUS_FLAG_DE_LOW;
> +	if (rad->vm.flags & DISPLAY_FLAGS_PIXDATA_NEGEDGE)
> +		*bus_flags |=3D DRM_BUS_FLAG_PIXDATA_NEGEDGE;
> +	if (rad->vm.flags & DISPLAY_FLAGS_PIXDATA_POSEDGE)
> +		*bus_flags |=3D DRM_BUS_FLAG_PIXDATA_POSEDGE;
> +
> +	ret =3D 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 =3D bl_get_data(bl);
> +	struct rad_panel *rad =3D mipi_dsi_get_drvdata(dsi);
> +	struct device *dev =3D &dsi->dev;
> +	u16 brightness;
> +	int ret;
> +
> +	if (!rad->prepared)
> +		return 0;
> +
> +	DRM_DEV_DEBUG_DRIVER(dev, "\n");
> +
> +	dsi->mode_flags &=3D ~MIPI_DSI_MODE_LPM;
> +
> +	ret =3D mipi_dsi_dcs_get_display_brightness(dsi, &brightness);
> +	if (ret < 0)
> +		return ret;
> +
> +	bl->props.brightness =3D brightness;
> +
> +	return brightness & 0xff;
> +}
> +
> +static int rad_bl_update_status(struct backlight_device *bl)
> +{
> +	struct mipi_dsi_device *dsi =3D bl_get_data(bl);
> +	struct rad_panel *rad =3D mipi_dsi_get_drvdata(dsi);
> +	struct device *dev =3D &dsi->dev;
> +	int ret =3D 0;
> +
> +	if (!rad->prepared)
> +		return 0;
> +
> +	DRM_DEV_DEBUG_DRIVER(dev, "New brightness: %d\n", bl->props.brightness);
> +
> +	dsi->mode_flags &=3D ~MIPI_DSI_MODE_LPM;
> +
> +	ret =3D 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 =3D {
> +	.update_status =3D rad_bl_update_status,
> +	.get_brightness =3D rad_bl_get_brightness,
> +};
> +
> +static const struct drm_panel_funcs rad_panel_funcs =3D {
> +	.prepare =3D rad_panel_prepare,
> +	.unprepare =3D rad_panel_unprepare,
> +	.enable =3D rad_panel_enable,
> +	.disable =3D rad_panel_disable,
> +	.get_modes =3D 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 =3D {
> +	.pixelclock =3D { 66000000, 120000000, 132000000 },
> +	.hactive =3D { 1080, 1080, 1080 },
> +	.hfront_porch =3D { 20, 20, 20 },
> +	.hsync_len =3D { 2, 2, 2 },
> +	.hback_porch =3D { 34, 34, 34 },
> +	.vactive =3D { 1920, 1920, 1920 },
> +	.vfront_porch =3D { 10, 10, 10 },
> +	.vsync_len =3D { 2, 2, 2 },
> +	.vback_porch =3D { 4, 4, 4 },
> +	.flags =3D 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 =3D &dsi->dev;
> +	struct device_node *np =3D dev->of_node;
> +	struct device_node *timings;
> +	struct rad_panel *panel;
> +	struct backlight_properties bl_props;
> +	int ret;
> +
> +	panel =3D devm_kzalloc(&dsi->dev, sizeof(*panel), GFP_KERNEL);
> +	if (!panel)
> +		return -ENOMEM;
> +
> +	mipi_dsi_set_drvdata(dsi, panel);
> +
> +	panel->dsi =3D dsi;
> +
> +	dsi->format =3D MIPI_DSI_FMT_RGB888;
> +	dsi->mode_flags =3D  MIPI_DSI_MODE_VIDEO_HSE | MIPI_DSI_MODE_VIDEO |
> +			   MIPI_DSI_CLOCK_NON_CONTINUOUS;
> +
> +	ret =3D 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 =3D of_get_child_by_name(np, "display-timings");
> +	if (timings) {
> +		of_node_put(timings);
> +		ret =3D 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 =3D devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
> +
> +	if (IS_ERR(panel->reset))
> +		panel->reset =3D NULL;
> +	else
> +		gpiod_set_value(panel->reset, 0);
> +
> +
> +	memset(&bl_props, 0, sizeof(bl_props));
> +	bl_props.type =3D BACKLIGHT_RAW;
> +	bl_props.brightness =3D 255;

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

> +	bl_props.max_brightness =3D 255;
> +
> +	panel->backlight =3D devm_backlight_device_register(
> +				dev, dev_name(dev),
> +				dev, dsi,
> +				&rad_bl_ops, &bl_props);
> +	if (IS_ERR(panel->backlight)) {
> +		ret =3D PTR_ERR(panel->backlight);
> +		dev_err(dev, "Failed to register backlight (%d)\n", ret);
> +		return ret;
> +	}
> +
> +	drm_panel_init(&panel->base);
> +	panel->base.funcs =3D &rad_panel_funcs;
> +	panel->base.dev =3D dev;
> +
> +	ret =3D drm_panel_add(&panel->base);
> +
> +	if (ret < 0)
> +		return ret;
> +
> +	ret =3D 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 =3D mipi_dsi_get_drvdata(dsi);
> +	struct device *dev =3D &dsi->dev;
> +	int ret;
> +
> +	ret =3D rad_panel_unprepare(&rad->base);
> +	ret |=3D 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.

> +
> +	ret =3D 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.

> +
> +	if (rad->base.dev)
> +		drm_panel_remove(&rad->base);

Why this check? Under what circumstances would this fail?

Thierry

--Q0rSlbzrZN6k9QnT
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCAAdFiEEiOrDCAFJzPfAjcif3SOs138+s6EFAlrh6C4ACgkQ3SOs138+
s6Ew0BAAnpp3aqHE1Dp1uo6/LJqdZHsapAhgqz1Ig8FfnnjMqGorNrBbDiwRBZaJ
DhzOu2VbVXZthvAB5nh44cERLKRofNZMvBB0HmS6uHb4/y0O6YDlgFOdQZPIovbQ
L8EppFZHnJ/tJJQ7jP6hbciULzJgXojp9cy919Ev2lvHFEtJNLsRE10uOyODm4GJ
6thhkFKOfhT6yi0CHjWA3a64e5dE4wgagDP/BoeB1TYr8YYDO8JYtiOyFyWeCQqJ
XLH6q57aE38Lb499J7Id84H3VsNjfsJR0zZYzcsPqRdJpr1tJPyY5Y/K7u2ncRC3
jlQ2m7Idvt3Gzxw0jWeprS7scm1leUlUdPx1i2VMyiSEQpJqjv3wDYOY10Bvs3/D
/yNIFgRgdTgjlJmMGNOvtUyJZhv3ciM+RcfLVT4cwJ7nr8fLntWpXHwRy6nJhCD0
/QR8Ct6zfDWFBNW4qyrbCnRmZJtjMuU8AavxStrGhYMv+OqnSgQMVjEa+HgiCIls
x/u/0Br+Z9nUe9uIB7TubyEM+mZUVn4WCTnYe1NV4kP9q4U06s9/VImklAA+N42N
xF1wMSdXWw6OUKfo15NCPIpg+TzfuzaPD8e7cKd+Y3tlQIHMdPBFSZPD0Yf8iZ0N
QoOu/TcF/ldl/BhlU4xYW/Ewfdb4IqKreMktp6tlLExalI+1Azw=
=pH0y
-----END PGP SIGNATURE-----

--Q0rSlbzrZN6k9QnT--

--===============0698156946==
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: base64
Content-Disposition: inline

X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVs
IG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlz
dHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vZHJpLWRldmVsCg==

--===============0698156946==--