[PATCH 1/2] drm/panel: Add Raydium RM67191 DSI Panel
Thierry Reding
thierry.reding at gmail.com
Thu Apr 26 14:54:40 UTC 2018
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 at 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 at 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 at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20180426/33d695be/attachment-0001.sig>
More information about the dri-devel
mailing list