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@xxxxxxx>
> ---
> .../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@xxxxxxxxxxxxxxx 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
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
