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?

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

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.

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.

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

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

+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()...

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

• 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?

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

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

• if (rad->base.dev)

• drm_panel_remove(&rad->base);
  

Why this check? Under what circumstances would this fail?

Thierry