Message-ID: <20180228191611.GC22932@mithrandir>
Date:   Wed, 28 Feb 2018 20:16:13 +0100
From:   Thierry Reding <thierry.reding@...il.com>
To:     Philippe Cornu <philippe.cornu@...com>
Cc:     David Airlie <airlied@...ux.ie>, Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        dri-devel@...ts.freedesktop.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, Andrzej Hajda <a.hajda@...sung.com>,
        Yannick Fertre <yannick.fertre@...com>,
        Benjamin Gaignard <benjamin.gaignard@...aro.org>,
        Vincent Abriou <vincent.abriou@...com>,
        Alexandre Torgue <alexandre.torgue@...com>
Subject: Re: [PATCH v1 2/2] drm/panel: Add support for Raydium rm68200 panel
 driver

On Thu, Feb 08, 2018 at 03:30:26PM +0100, Philippe Cornu wrote:
> This patch adds Raydium Semiconductor Corporation rm68200
> 5.5" 720x1280 TFT LCD panel driver (MIPI-DSI video mode).
> 
> Signed-off-by: Philippe Cornu <philippe.cornu@...com>
> ---
>  drivers/gpu/drm/panel/Kconfig                 |   8 +
>  drivers/gpu/drm/panel/Makefile                |   1 +
>  drivers/gpu/drm/panel/panel-raydium-rm68200.c | 464 ++++++++++++++++++++++++++
>  3 files changed, 473 insertions(+)
>  create mode 100755 drivers/gpu/drm/panel/panel-raydium-rm68200.c
> 
> diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
> index 988048ebcc22..08d99dd46765 100644
> --- a/drivers/gpu/drm/panel/Kconfig
> +++ b/drivers/gpu/drm/panel/Kconfig
> @@ -108,6 +108,14 @@ config DRM_PANEL_RASPBERRYPI_TOUCHSCREEN
>  	  Pi 7" Touchscreen.  To compile this driver as a module,
>  	  choose M here.
>  
> +config DRM_PANEL_RAYDIUM_RM68200
> +	tristate "Raydium rm68200 720x1280 dsi 2dl video mode panel"

What's 2dl? Either this is something already implied by the RM68200
model or if there are multiple variants of the RM68200 you'll probably
want to ensure that's reflected in the compatible string.

> +	depends on OF
> +	depends on DRM_MIPI_DSI
> +	help
> +	  Say Y here if you want to enable support for Raydium rm68200
> +	  720x1280 dsi 2dl video mode panel
> +
>  config DRM_PANEL_SAMSUNG_S6E3HA2
>  	tristate "Samsung S6E3HA2 DSI video mode panel"
>  	depends on OF
> diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
> index 3d2a88d0e965..f26efc11d746 100644
> --- a/drivers/gpu/drm/panel/Makefile
> +++ b/drivers/gpu/drm/panel/Makefile
> @@ -9,6 +9,7 @@ obj-$(CONFIG_DRM_PANEL_LG_LG4573) += panel-lg-lg4573.o
>  obj-$(CONFIG_DRM_PANEL_ORISETECH_OTM8009A) += panel-orisetech-otm8009a.o
>  obj-$(CONFIG_DRM_PANEL_PANASONIC_VVX10F034N00) += panel-panasonic-vvx10f034n00.o
>  obj-$(CONFIG_DRM_PANEL_RASPBERRYPI_TOUCHSCREEN) += panel-raspberrypi-touchscreen.o
> +obj-$(CONFIG_DRM_PANEL_RAYDIUM_RM68200) += panel-raydium-rm68200.o
>  obj-$(CONFIG_DRM_PANEL_SAMSUNG_LD9040) += panel-samsung-ld9040.o
>  obj-$(CONFIG_DRM_PANEL_SAMSUNG_S6E3HA2) += panel-samsung-s6e3ha2.o
>  obj-$(CONFIG_DRM_PANEL_SAMSUNG_S6E63J0X03) += panel-samsung-s6e63j0x03.o
> diff --git a/drivers/gpu/drm/panel/panel-raydium-rm68200.c b/drivers/gpu/drm/panel/panel-raydium-rm68200.c
> new file mode 100755
> index 000000000000..f3e15873d05a
> --- /dev/null
> +++ b/drivers/gpu/drm/panel/panel-raydium-rm68200.c
> @@ -0,0 +1,464 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) STMicroelectronics SA 2017
> + *
> + * Authors: Philippe Cornu <philippe.cornu@...com>
> + *          Yannick Fertre <yannick.fertre@...com>
> + */
> +
> +#include <drm/drmP.h>
> +#include <drm/drm_mipi_dsi.h>
> +#include <drm/drm_panel.h>
> +#include <linux/backlight.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/regulator/consumer.h>
> +#include <video/mipi_display.h>
> +
> +#define DRV_NAME "raydium_rm68200"

Not needed, see below.

> +
> +/*** Manufacturer Command Set ***/
> +#define MCS_CMD_MODE_SW	0xFE /* CMD Mode Switch */
> +#define MCS_CMD1_UCS	0x00 /* User Command Set (UCS = CMD1) */
> +#define MCS_CMD2_P0	0x01 /* Manufacture Command Set Page0 (CMD2 P0) */
> +#define MCS_CMD2_P1	0x02 /* Manufacture Command Set Page1 (CMD2 P1) */
> +#define MCS_CMD2_P2	0x03 /* Manufacture Command Set Page2 (CMD2 P2) */
> +#define MCS_CMD2_P3	0x04 /* Manufacture Command Set Page3 (CMD2 P3) */
> +
> +/* CMD2 P0 commands (Display Options and Power) */
> +#define MCS_STBCTR	0x12 /* TE1 Output Setting Zig-Zag Connection */
> +#define MCS_SGOPCTR	0x16 /* Source Bias Current */
> +#define MCS_SDCTR	0x1A /* Source Output Delay Time */
> +#define MCS_INVCTR	0x1B /* Inversion Type */
> +#define MCS_EXT_PWR_IC	0x24 /* External PWR IC Control */
> +#define MCS_SETAVDD	0x27 /* PFM Control for AVDD Output */
> +#define MCS_SETAVEE	0x29 /* PFM Control for AVEE Output */
> +#define MCS_BT2CTR	0x2B /* DDVDL Charge Pump Control */
> +#define MCS_BT3CTR	0x2F /* VGH Charge Pump Control */
> +#define MCS_BT4CTR	0x34 /* VGL Charge Pump Control */
> +#define MCS_VCMCTR	0x46 /* VCOM Output Level Control */
> +#define MCS_SETVGN	0x52 /* VG M/S N Control */
> +#define MCS_SETVGP	0x54 /* VG M/S P Control */
> +#define MCS_SW_CTRL	0x5F /* Interface Control for PFM and MIPI */
> +
> +/* CMD2 P2 commands (GOA Timing Control) - no description in datasheet */
> +#define GOA_VSTV1	0x00
> +#define GOA_VSTV2	0x07
> +#define GOA_VCLK1	0x0E
> +#define GOA_VCLK2	0x17
> +#define GOA_VCLK_OPT1	0x20
> +#define GOA_BICLK1	0x2A
> +#define GOA_BICLK2	0x37
> +#define GOA_BICLK3	0x44
> +#define GOA_BICLK4	0x4F
> +#define GOA_BICLK_OPT1	0x5B
> +#define GOA_BICLK_OPT2	0x60
> +#define MCS_GOA_GPO1	0x6D
> +#define MCS_GOA_GPO2	0x71
> +#define MCS_GOA_EQ	0x74
> +#define MCS_GOA_CLK_GALLON 0x7C
> +#define MCS_GOA_FS_SEL0	0x7E
> +#define MCS_GOA_FS_SEL1	0x87
> +#define MCS_GOA_FS_SEL2	0x91
> +#define MCS_GOA_FS_SEL3	0x9B
> +#define MCS_GOA_BS_SEL0	0xAC
> +#define MCS_GOA_BS_SEL1	0xB5
> +#define MCS_GOA_BS_SEL2	0xBF
> +#define MCS_GOA_BS_SEL3	0xC9
> +#define MCS_GOA_BS_SEL4	0xD3
> +
> +/* CMD2 P3 commands (Gamma) */
> +#define MCS_GAMMA_VP	0x60 /* Gamma VP1~VP16 */
> +#define MCS_GAMMA_VN	0x70 /* Gamma VN1~VN16 */
> +
> +struct rm68200 {
> +	struct device *dev;
> +	struct drm_panel panel;
> +	struct gpio_desc *reset_gpio;
> +	struct regulator *supply;
> +	struct backlight_device *bl_dev;
> +	bool prepared;
> +	bool enabled;
> +};
> +
> +static const struct drm_display_mode default_mode = {
> +	.clock = 52582,
> +	.hdisplay = 720,
> +	.hsync_start = 720 + 38,
> +	.hsync_end = 720 + 38 + 8,
> +	.htotal = 720 + 38 + 8 + 38,
> +	.vdisplay = 1280,
> +	.vsync_start = 1280 + 12,
> +	.vsync_end = 1280 + 12 + 4,
> +	.vtotal = 1280 + 12 + 4 + 12,
> +	.vrefresh = 50,
> +	.flags = 0,
> +	.width_mm = 68,
> +	.height_mm = 122,
> +};
> +
> +static inline struct rm68200 *panel_to_rm68200(struct drm_panel *panel)
> +{
> +	return container_of(panel, struct rm68200, panel);
> +}
> +
> +static void rm68200_dcs_write_buf(struct rm68200 *ctx, const void *data,
> +				  size_t len)
> +{
> +	struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev);
> +
> +	if (mipi_dsi_dcs_write_buffer(dsi, data, len) < 0)
> +		DRM_WARN("mipi dsi dcs write buffer failed\n");
> +}
> +
> +static void rm68200_dcs_write_cmd(struct rm68200 *ctx, u8 cmd, u8 value)
> +{
> +	struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev);
> +
> +	if (mipi_dsi_dcs_write(dsi, cmd, &value, 1) < 0)
> +		DRM_WARN("mipi dsi dcs write failed\n");
> +}

I question the usefulness of your error reporting here. You're not
giving the user any meaningful feedback. Also, you'll be warning about
every single failure. It's likely that if one of the writes fails, then
subsequent writes will also fail, which means that you'll give the user
a flood of DRM_WARN() with exactly the same text.

> +/*
> + * This panel is not able to auto-increment all cmd addresses so for some of
> + * them, we need to send them one by one...
> + */
> +#define dcs_write_cmd_seq(ctx, cmd, seq...)			\
> +({								\
> +	static const u8 d[] = { seq };				\
> +	static int i;						\
> +	for (i = 0; i < ARRAY_SIZE(d) ; i++)			\
> +		rm68200_dcs_write_cmd(ctx, cmd + i, d[i]);	\
> +})
> +
> +static void rm68200_init_sequence(struct rm68200 *ctx)
> +{
> +	/* Enter CMD2 with page 0 */
> +	dcs_write_seq(ctx, MCS_CMD_MODE_SW, MCS_CMD2_P0);
> +	dcs_write_cmd_seq(ctx, MCS_EXT_PWR_IC, 0xC0, 0x53, 0x00);
> +	dcs_write_seq(ctx, MCS_BT2CTR, 0xE5);
> +	dcs_write_seq(ctx, MCS_SETAVDD, 0x0A);
> +	dcs_write_seq(ctx, MCS_SETAVEE, 0x0A);
> +	dcs_write_seq(ctx, MCS_SGOPCTR, 0x52);
> +	dcs_write_seq(ctx, MCS_BT3CTR, 0x53);
> +	dcs_write_seq(ctx, MCS_BT4CTR, 0x5A);
> +	dcs_write_seq(ctx, MCS_INVCTR, 0x00);
> +	dcs_write_seq(ctx, MCS_STBCTR, 0x0A);
> +	dcs_write_seq(ctx, MCS_SDCTR, 0x06);
> +	dcs_write_seq(ctx, MCS_VCMCTR, 0x56);
> +	dcs_write_seq(ctx, MCS_SETVGN, 0xA0, 0x00);
> +	dcs_write_seq(ctx, MCS_SETVGP, 0xA0, 0x00);
> +	dcs_write_seq(ctx, MCS_SW_CTRL, 0x11); /* 2 data lanes, see doc */
> +
> +	dcs_write_seq(ctx, MCS_CMD_MODE_SW, MCS_CMD2_P2);
> +	dcs_write_seq(ctx, GOA_VSTV1, 0x05);
> +	dcs_write_seq(ctx, 0x02, 0x0B);
> +	dcs_write_seq(ctx, 0x03, 0x0F);
> +	dcs_write_seq(ctx, 0x04, 0x7D, 0x00, 0x50);
> +	dcs_write_cmd_seq(ctx, GOA_VSTV2, 0x05, 0x16, 0x0D, 0x11, 0x7D, 0x00,
> +			  0x50);
> +	dcs_write_cmd_seq(ctx, GOA_VCLK1, 0x07, 0x08, 0x01, 0x02, 0x00, 0x7D,
> +			  0x00, 0x85, 0x08);
> +	dcs_write_cmd_seq(ctx, GOA_VCLK2, 0x03, 0x04, 0x05, 0x06, 0x00, 0x7D,
> +			  0x00, 0x85, 0x08);
> +	dcs_write_seq(ctx, GOA_VCLK_OPT1, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> +		      0x00, 0x00, 0x00, 0x00);
> +	dcs_write_cmd_seq(ctx, GOA_BICLK1, 0x07, 0x08);
> +	dcs_write_seq(ctx, 0x2D, 0x01);
> +	dcs_write_seq(ctx, 0x2F, 0x02, 0x00, 0x40, 0x05, 0x08, 0x54, 0x7D,
> +		      0x00);
> +	dcs_write_cmd_seq(ctx, GOA_BICLK2, 0x03, 0x04, 0x05, 0x06, 0x00);
> +	dcs_write_seq(ctx, 0x3D, 0x40);
> +	dcs_write_seq(ctx, 0x3F, 0x05, 0x08, 0x54, 0x7D, 0x00);
> +	dcs_write_seq(ctx, GOA_BICLK3, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> +		      0x00, 0x00, 0x00, 0x00, 0x00);
> +	dcs_write_seq(ctx, GOA_BICLK4, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> +		      0x00, 0x00);
> +	dcs_write_seq(ctx, 0x58, 0x00, 0x00, 0x00);
> +	dcs_write_seq(ctx, GOA_BICLK_OPT1, 0x00, 0x00, 0x00, 0x00, 0x00);
> +	dcs_write_seq(ctx, GOA_BICLK_OPT2, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> +		      0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00);
> +	dcs_write_seq(ctx, MCS_GOA_GPO1, 0x00, 0x00, 0x00, 0x00);
> +	dcs_write_seq(ctx, MCS_GOA_GPO2, 0x00, 0x20, 0x00);
> +	dcs_write_seq(ctx, MCS_GOA_EQ, 0x08, 0x08, 0x08, 0x08, 0x08, 0x08,
> +		      0x00, 0x00);
> +	dcs_write_seq(ctx, MCS_GOA_CLK_GALLON, 0x00, 0x00);
> +	dcs_write_cmd_seq(ctx, MCS_GOA_FS_SEL0, 0xBF, 0x02, 0x06, 0x14, 0x10,
> +			  0x16, 0x12, 0x08, 0x3F);
> +	dcs_write_cmd_seq(ctx, MCS_GOA_FS_SEL1, 0x3F, 0x3F, 0x3F, 0x3F, 0x0C,
> +			  0x0A, 0x0E, 0x3F, 0x3F, 0x00);
> +	dcs_write_cmd_seq(ctx, MCS_GOA_FS_SEL2, 0x04, 0x3F, 0x3F, 0x3F, 0x3F,
> +			  0x05, 0x01, 0x3F, 0x3F, 0x0F);
> +	dcs_write_cmd_seq(ctx, MCS_GOA_FS_SEL3, 0x0B, 0x0D, 0x3F, 0x3F, 0x3F,
> +			  0x3F);
> +	dcs_write_cmd_seq(ctx, 0xA2, 0x3F, 0x09, 0x13, 0x17, 0x11, 0x15);
> +	dcs_write_cmd_seq(ctx, 0xA9, 0x07, 0x03, 0x3F);
> +	dcs_write_cmd_seq(ctx, MCS_GOA_BS_SEL0, 0x3F, 0x05, 0x01, 0x17, 0x13,
> +			  0x15, 0x11, 0x0F, 0x3F);
> +	dcs_write_cmd_seq(ctx, MCS_GOA_BS_SEL1, 0x3F, 0x3F, 0x3F, 0x3F, 0x0B,
> +			  0x0D, 0x09, 0x3F, 0x3F, 0x07);
> +	dcs_write_cmd_seq(ctx, MCS_GOA_BS_SEL2, 0x03, 0x3F, 0x3F, 0x3F, 0x3F,
> +			  0x02, 0x06, 0x3F, 0x3F, 0x08);
> +	dcs_write_cmd_seq(ctx, MCS_GOA_BS_SEL3, 0x0C, 0x0A, 0x3F, 0x3F, 0x3F,
> +			  0x3F, 0x3F, 0x0E, 0x10, 0x14);
> +	dcs_write_cmd_seq(ctx, MCS_GOA_BS_SEL4, 0x12, 0x16, 0x00, 0x04, 0x3F);
> +	dcs_write_seq(ctx, 0xDC, 0x02);
> +	dcs_write_seq(ctx, 0xDE, 0x12);
> +
> +	dcs_write_seq(ctx, MCS_CMD_MODE_SW, 0x0E); /* No documentation */
> +	dcs_write_seq(ctx, 0x01, 0x75);
> +
> +	dcs_write_seq(ctx, MCS_CMD_MODE_SW, MCS_CMD2_P3);
> +	dcs_write_cmd_seq(ctx, MCS_GAMMA_VP, 0x00, 0x0C, 0x12, 0x0E, 0x06,
> +			  0x12, 0x0E, 0x0B, 0x15, 0x0B, 0x10, 0x07, 0x0F,
> +			  0x12, 0x0C, 0x00);
> +	dcs_write_cmd_seq(ctx, MCS_GAMMA_VN, 0x00, 0x0C, 0x12, 0x0E, 0x06,
> +			  0x12, 0x0E, 0x0B, 0x15, 0x0B, 0x10, 0x07, 0x0F,
> +			  0x12, 0x0C, 0x00);
> +
> +	/* Exit CMD2 */
> +	dcs_write_seq(ctx, MCS_CMD_MODE_SW, MCS_CMD1_UCS);
> +}
> +
> +static int rm68200_disable(struct drm_panel *panel)
> +{
> +	struct rm68200 *ctx = panel_to_rm68200(panel);
> +
> +	if (!ctx->enabled)
> +		return 0;
> +
> +	if (ctx->bl_dev) {
> +		ctx->bl_dev->props.power = FB_BLANK_POWERDOWN;
> +		ctx->bl_dev->props.state |= BL_CORE_FBBLANK;
> +		backlight_update_status(ctx->bl_dev);
> +	}

This should use the new backlight_disable() function.

> +
> +	ctx->enabled = false;
> +
> +	return 0;
> +}
> +
> +static int rm68200_unprepare(struct drm_panel *panel)
> +{
> +	struct rm68200 *ctx = panel_to_rm68200(panel);
> +	struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev);
> +	int ret;
> +
> +	if (!ctx->prepared)
> +		return 0;
> +
> +	ret = mipi_dsi_dcs_set_display_off(dsi);
> +	if (ret)
> +		DRM_WARN("failed to set display off: %d\n", ret);
> +
> +	ret = mipi_dsi_dcs_enter_sleep_mode(dsi);
> +	if (ret)
> +		DRM_WARN("failed to enter sleep mode: %d\n", ret);
> +
> +	msleep(120);
> +
> +	if (ctx->reset_gpio) {
> +		gpiod_set_value_cansleep(ctx->reset_gpio, 1);
> +		msleep(20);
> +	}
> +
> +	regulator_disable(ctx->supply);
> +
> +	ctx->prepared = false;
> +
> +	return 0;
> +}
> +
> +static int rm68200_prepare(struct drm_panel *panel)
> +{
> +	struct rm68200 *ctx = panel_to_rm68200(panel);
> +	struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev);
> +	int ret;
> +
> +	if (ctx->prepared)
> +		return 0;
> +
> +	ret = regulator_enable(ctx->supply);
> +	if (ret < 0) {
> +		DRM_ERROR("failed to enable supply: %d\n", ret);
> +		return ret;
> +	}
> +
> +	if (ctx->reset_gpio) {
> +		gpiod_set_value_cansleep(ctx->reset_gpio, 0);

Why do you need this? If the panel is already in reset, shouldn't the
below still work? Why take it out of reset only to reset it immediately
afterwards again?

> +		gpiod_set_value_cansleep(ctx->reset_gpio, 1);
> +		msleep(20);
> +		gpiod_set_value_cansleep(ctx->reset_gpio, 0);
> +		msleep(100);
> +	}
> +
> +	rm68200_init_sequence(ctx);
> +
> +	ret = mipi_dsi_dcs_exit_sleep_mode(dsi);
> +	if (ret)
> +		return ret;
> +
> +	msleep(125);
> +
> +	ret = mipi_dsi_dcs_set_display_on(dsi);
> +	if (ret)
> +		return ret;
> +
> +	msleep(20);
> +
> +	ctx->prepared = true;
> +
> +	return 0;
> +}
> +
> +static int rm68200_enable(struct drm_panel *panel)
> +{
> +	struct rm68200 *ctx = panel_to_rm68200(panel);
> +
> +	if (ctx->enabled)
> +		return 0;
> +
> +	if (ctx->bl_dev) {
> +		ctx->bl_dev->props.state &= ~BL_CORE_FBBLANK;
> +		ctx->bl_dev->props.power = FB_BLANK_UNBLANK;
> +		backlight_update_status(ctx->bl_dev);
> +	}

backlight_enable(), please.

> +
> +	ctx->enabled = true;
> +
> +	return 0;
> +}
> +
> +static int rm68200_get_modes(struct drm_panel *panel)
> +{
> +	struct drm_display_mode *mode;
> +
> +	mode = drm_mode_duplicate(panel->drm, &default_mode);
> +	if (!mode) {
> +		DRM_ERROR("failed to add mode %ux%ux@%u\n",
> +			  default_mode.hdisplay, default_mode.vdisplay,
> +			  default_mode.vrefresh);
> +		return -ENOMEM;
> +	}
> +
> +	drm_mode_set_name(mode);
> +
> +	mode->type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED;
> +	drm_mode_probed_add(panel->connector, mode);
> +
> +	panel->connector->display_info.width_mm = mode->width_mm;
> +	panel->connector->display_info.height_mm = mode->height_mm;
> +
> +	return 1;
> +}
> +
> +static const struct drm_panel_funcs rm68200_drm_funcs = {
> +	.disable   = rm68200_disable,
> +	.unprepare = rm68200_unprepare,
> +	.prepare   = rm68200_prepare,
> +	.enable    = rm68200_enable,
> +	.get_modes = rm68200_get_modes,
> +};
> +
> +static int rm68200_probe(struct mipi_dsi_device *dsi)
> +{
> +	struct device *dev = &dsi->dev;
> +	struct device_node *backlight;
> +	struct rm68200 *ctx;
> +	int ret;
> +
> +	ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
> +	if (!ctx)
> +		return -ENOMEM;
> +
> +	ctx->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
> +	if (IS_ERR(ctx->reset_gpio)) {
> +		dev_err(dev, "cannot get reset-gpio\n");
> +		return PTR_ERR(ctx->reset_gpio);
> +	}
> +
> +	ctx->supply = devm_regulator_get(dev, "power");
> +	if (IS_ERR(ctx->supply)) {
> +		dev_err(dev, "cannot get regulator\n");
> +		return PTR_ERR(ctx->supply);
> +	}
> +
> +	backlight = of_parse_phandle(dev->of_node, "backlight", 0);
> +	if (backlight) {
> +		ctx->bl_dev = of_find_backlight_by_node(backlight);
> +		of_node_put(backlight);
> +
> +		if (!ctx->bl_dev)
> +			return -EPROBE_DEFER;
> +	}

devm_of_find_backlight()

> +
> +	mipi_dsi_set_drvdata(dsi, ctx);
> +
> +	ctx->dev = dev;
> +
> +	dsi->lanes = 2;
> +	dsi->format = MIPI_DSI_FMT_RGB888;
> +	dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_BURST |
> +			  MIPI_DSI_MODE_LPM;
> +
> +	drm_panel_init(&ctx->panel);
> +	ctx->panel.dev = dev;
> +	ctx->panel.funcs = &rm68200_drm_funcs;
> +
> +	drm_panel_add(&ctx->panel);
> +
> +	ret = mipi_dsi_attach(dsi);
> +	if (ret < 0) {
> +		dev_err(dev, "mipi_dsi_attach failed. Is host ready?\n");
> +		drm_panel_remove(&ctx->panel);
> +		if (ctx->bl_dev)
> +			put_device(&ctx->bl_dev->dev);
> +		return ret;
> +	}
> +
> +	DRM_INFO(DRV_NAME "_panel %ux%u@%u %ubpp dsi %udl - ready\n",
> +		 default_mode.hdisplay, default_mode.vdisplay,
> +		 default_mode.vrefresh,
> +		 mipi_dsi_pixel_format_to_bpp(dsi->format), dsi->lanes);

No need to brag about this being successful. That's what it's supposed
to be. You should let the user know if things *don't* happen the way
they're supposed to.

> +
> +	return 0;
> +}
> +
> +static int rm68200_remove(struct mipi_dsi_device *dsi)
> +{
> +	struct rm68200 *ctx = mipi_dsi_get_drvdata(dsi);
> +
> +	if (ctx->bl_dev)
> +		put_device(&ctx->bl_dev->dev);
> +
> +	mipi_dsi_detach(dsi);
> +	drm_panel_remove(&ctx->panel);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id raydium_rm68200_of_match[] = {
> +	{ .compatible = "raydium,rm68200" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, raydium_rm68200_of_match);
> +
> +static struct mipi_dsi_driver raydium_rm68200_driver = {
> +	.probe  = rm68200_probe,
> +	.remove = rm68200_remove,
> +	.driver = {
> +		.name = DRV_NAME "_panel",

This is the only occurrence so you can just drop DRV_NAME and put the
name here.

> +		.of_match_table = raydium_rm68200_of_match,
> +	},
> +};
> +module_mipi_dsi_driver(raydium_rm68200_driver);
> +
> +MODULE_AUTHOR("Philippe Cornu <philippe.cornu@...com>");
> +MODULE_AUTHOR("Yannick Fertre <yannick.fertre@...com>");
> +MODULE_DESCRIPTION("DRM Driver for Raydium RM68200 MIPI DSI panel");

You use RM68200 here but in other places you use rm68200. Please be
consistent. It seems like RM68200 is the "official" spelling, so stick
to that in prose.

Thierry

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)

Powered by blists - more mailing lists