Re: [EXT] Re: [PATCH 2/2] drm/panel: Add support for Raydium RM67191 panel driver

[Robert Chiras]

Hi Sam,

Thank you for your suggestions.
See my reply inline.

On Vi, 2019-06-14 at 17:10 +0200, Sam Ravnborg wrote:
> Hi Robert.
> 
> On top of the feedback from Fabio here is a bit more.
> 
> > 
> > +
> > +#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>
> Divide include up in block in following order:
> 
> #include <linux/*>
> 
> #include <video/*>
> 
> #incude <drm/*>
> 
> Within each block sort alphabetically.
> Do not use the deprecated drmP.h - replace it with the necessary
> includes.
> Use an empty line between each include block.
Thanks. Will do.
> 
> > 
> > +
> > +/* Write Manufacture Command Set Control */
> > +#define WRMAUCCTR 0xFE
> > +
> > +/* Manufacturer Command Set pages (CMD2) */
> > +struct cmd_set_entry {
> > +ÂÂÂÂÂu8 cmd;
> > +ÂÂÂÂÂu8 param;
> > +};
> > +
> > +/*
> > + * There is no description in the Reference Manual about these
> > commands.
> > + * We received them from vendor, so just use them as is.
> > + */
> > +static const struct cmd_set_entry manufacturer_cmd_set[] = {
> > +ÂÂÂÂÂ{0xFE, 0x0B},
> > +ÂÂÂÂÂ{0x28, 0x40},
> > +ÂÂÂÂÂ{0x29, 0x4F},
> ...
> > 
> > +ÂÂÂÂÂ{0x51, 0x04},
> > +};
> > +
> > +static const u32 rad_bus_formats[] = {
> > +ÂÂÂÂÂMEDIA_BUS_FMT_RGB888_1X24,
> > +ÂÂÂÂÂMEDIA_BUS_FMT_RGB666_1X18,
> > +ÂÂÂÂÂMEDIA_BUS_FMT_RGB565_1X16,
> > +};
> > +
> > +struct rad_panel {
> > +ÂÂÂÂÂstruct drm_panel base;
> In the other raydium driver we name this "panel", which is a more
> descriptive name.
> 
> > 
> > +ÂÂÂÂÂ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 int rad_panel_prepare(struct drm_panel *panel)
> > +{
> > +ÂÂÂÂÂstruct rad_panel *rad = to_rad_panel(panel);
> > +
> > +ÂÂÂÂÂif (rad->prepared)
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂreturn 0;
> > +
> > +ÂÂÂÂÂif (rad->reset) {
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂgpiod_set_value(rad->reset, 0);
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂusleep_range(5000, 10000);
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂgpiod_set_value(rad->reset, 1);
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂusleep_range(20000, 25000);
> > +ÂÂÂÂÂ}
> > +
> > +ÂÂÂÂÂrad->prepared = true;
> > +
> > +ÂÂÂÂÂreturn 0;
> > +}
> > +
> > +static int rad_panel_unprepare(struct drm_panel *panel)
> > +{
> > +ÂÂÂÂÂstruct rad_panel *rad = to_rad_panel(panel);
> > +ÂÂÂÂÂstruct device *dev = &rad->dsi->dev;
> > +
> > +ÂÂÂÂÂif (!rad->prepared)
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂreturn 0;
> > +
> > +ÂÂÂÂÂif (rad->enabled) {
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂDRM_DEV_ERROR(dev, "Panel still enabled!\n");
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂreturn -EPERM;
> > +ÂÂÂÂÂ}
> This seems like overkill, what should trigger this?
Probably, just bad written code in the DSI host driver, but you are
right, it's overkill. I will remove this.
> 
> > 
> > +
> > +ÂÂÂÂÂif (rad->reset) {
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂgpiod_set_value(rad->reset, 0);
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂusleep_range(15000, 17000);
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂgpiod_set_value(rad->reset, 1);
> > +ÂÂÂÂÂ}
> > +
> > +ÂÂÂÂÂrad->prepared = false;
> > +
> > +ÂÂÂÂÂreturn 0;
> > +}
> > +
> > +static int rad_panel_enable(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 color_format = color_format_from_dsi_format(dsi->format);
> > +ÂÂÂÂÂu16 brightness;
> > +ÂÂÂÂÂint ret;
> > +
> > +ÂÂÂÂÂif (rad->enabled)
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂreturn 0;
> > +
> > +ÂÂÂÂÂif (!rad->prepared) {
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂDRM_DEV_ERROR(dev, "Panel not prepared!\n");
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂreturn -EPERM;
> > +ÂÂÂÂÂ}
> Seems like overkill.
> 
> > 
> > +
> > +ÂÂÂÂÂ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(15000, 17000);
> > +
> > +ÂÂÂÂÂ/* 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 */
> > +ÂÂÂÂÂret = mipi_dsi_dcs_set_pixel_format(dsi, color_format);
> > +ÂÂÂÂÂDRM_DEV_DEBUG_DRIVER(dev, "Interface color format set to
> > 0x%x\n",
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂcolor_format);
> > +ÂÂÂÂÂif (ret < 0) {
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂDRM_DEV_ERROR(dev, "Failed to set pixel format
> > (%d)\n", ret);
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂgoto fail;
> > +ÂÂÂÂÂ}
> > +ÂÂÂÂÂ/* Set display brightness */
> > +ÂÂÂÂÂbrightness = rad->backlight->props.brightness;
> > +ÂÂÂÂÂret = mipi_dsi_dcs_set_display_brightness(dsi, brightness);
> > +ÂÂÂÂÂif (ret < 0) {
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂDRM_DEV_ERROR(dev, "Failed to set display brightness
> > (%d)\n",
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂret);
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂgoto fail;
> > +ÂÂÂÂÂ}
> brightness is written to again when you enable backlight below.
> Seems redundant to do so twice.
I put this here from the initialization sequence received from the
panel vendor. But you are right, the backlight will be enabled later
on. I will remove this.Â
> 
> 
> > 
> > +ÂÂÂÂÂ/* 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;
> > +ÂÂÂÂÂ}
> > +
> > +ÂÂÂÂÂbacklight_enable(rad->backlight);
> > +
> > +ÂÂÂÂÂrad->enabled = true;
> > +
> > +ÂÂÂÂÂreturn 0;
> > +
> > +fail:
> > +ÂÂÂÂÂif (rad->reset)
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂgpiod_set_value(rad->reset, 0);
> > +
> > +ÂÂÂÂÂreturn ret;
> > +}
> > +
> > +static int rad_panel_disable(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->enabled)
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂreturn 0;
> > +
> > +ÂÂÂÂÂdsi->mode_flags |= MIPI_DSI_MODE_LPM;
> > +
> > +ÂÂÂÂÂbacklight_disable(rad->backlight);
> > +
> > +ÂÂÂÂÂusleep_range(10000, 15000);
> > +
> > +ÂÂÂÂÂret = mipi_dsi_dcs_set_display_off(dsi);
> > +ÂÂÂÂÂif (ret < 0) {
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂDRM_DEV_ERROR(dev, "Failed to set display OFF
> > (%d)\n", ret);
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂreturn 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);
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂreturn ret;
> > +ÂÂÂÂÂ}
> > +
> > +ÂÂÂÂÂ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_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,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂrad_bus_formats,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂARRAY_SIZE(rad_bus_for
> > mats));
> > +ÂÂÂÂÂ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, 132000000, 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;
> > +ÂÂÂÂÂu32 video_mode;
> > +
> > +ÂÂÂÂÂ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, "video-mode", &video_mode);
> > +ÂÂÂÂÂif (!ret) {
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂswitch (video_mode) {
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂcase 0:
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ/* burst mode */
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂdsi->mode_flags |= MIPI_DSI_MODE_VIDEO_BURST;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂbreak;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂcase 1:
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ/* non-burst mode with sync event */
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂbreak;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂcase 2:
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ/* non-burst mode with sync pulse */
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂdsi->mode_flags |=
> > MIPI_DSI_MODE_VIDEO_SYNC_PULSE;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂbreak;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂdefault:
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂdev_warn(dev, "invalid video mode %d\n",
> > video_mode);
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂbreak;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂ}
> > +ÂÂÂÂÂ}
> > +
> > +ÂÂÂÂÂ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, "width-mm", &panel->width_mm);
> > +ÂÂÂÂÂof_property_read_u32(np, "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;
> > +ÂÂÂÂÂ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;
> > +ÂÂÂÂÂdev_set_drvdata(dev, panel);
> > +
> > +ÂÂÂÂÂ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 = mipi_dsi_detach(dsi);
> > +ÂÂÂÂÂif (ret < 0)
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂDRM_DEV_ERROR(dev, "Failed to detach from host
> > (%d)\n",
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂret);
> > +
> > +ÂÂÂÂÂdrm_panel_remove(&rad->base);
> > +
> > +ÂÂÂÂÂreturn 0;
> > +}
> > +
> > +static void rad_panel_shutdown(struct mipi_dsi_device *dsi)
> > +{
> > +ÂÂÂÂÂstruct rad_panel *rad = mipi_dsi_get_drvdata(dsi);
> > +
> > +ÂÂÂÂÂrad_panel_disable(&rad->base);
> > +ÂÂÂÂÂrad_panel_unprepare(&rad->base);
> > +}
> > +
> > +#ifdef CONFIG_PM
> > +static int rad_panel_suspend(struct device *dev)
> > +{
> > +ÂÂÂÂÂstruct rad_panel *rad = dev_get_drvdata(dev);
> > +
> > +ÂÂÂÂÂif (!rad->reset)
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂreturn 0;
> > +
> > +ÂÂÂÂÂdevm_gpiod_put(dev, rad->reset);
> > +ÂÂÂÂÂrad->reset = NULL;
> > +
> > +ÂÂÂÂÂreturn 0;
> > +}
> > +
> > +static int rad_panel_resume(struct device *dev)
> > +{
> > +ÂÂÂÂÂstruct rad_panel *rad = dev_get_drvdata(dev);
> > +
> > +ÂÂÂÂÂif (rad->reset)
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂreturn 0;
> > +
> > +ÂÂÂÂÂrad->reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
> > +ÂÂÂÂÂif (IS_ERR(rad->reset))
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂrad->reset = NULL;
> > +
> > +ÂÂÂÂÂreturn PTR_ERR_OR_ZERO(rad->reset);
> > +}
> > +
> > +#endif
> No other panels have suspend/resume.
> Is it relevant?
This is because of the shared 'reset' pin with the touchscreen driver.
During suspend I need to release this gpio resource.
> 
> 
> > 
> > +
> > +static const struct dev_pm_ops rad_pm_ops = {
> > +ÂÂÂÂÂSET_RUNTIME_PM_OPS(rad_panel_suspend, rad_panel_resume, NULL)
> > +ÂÂÂÂÂSET_SYSTEM_SLEEP_PM_OPS(rad_panel_suspend, rad_panel_resume)
> > +};
> > +
> > +static const struct of_device_id rad_of_match[] = {
> > +ÂÂÂÂÂ{ .compatible = "raydium,rm67191", },
> > +ÂÂÂÂÂ{ }
> > +};
> > +MODULE_DEVICE_TABLE(of, rad_of_match);
> > +
> > +static struct mipi_dsi_driver rad_panel_driver = {
> > +ÂÂÂÂÂ.driver = {
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂ.name = "panel-raydium-rm67191",
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂ.of_match_table = rad_of_match,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂ.pmÂÂÂÂÂ= &rad_pm_ops,
> > +ÂÂÂÂÂ},
> > +ÂÂÂÂÂ.probe = rad_panel_probe,
> > +ÂÂÂÂÂ.remove = rad_panel_remove,
> > +ÂÂÂÂÂ.shutdown = rad_panel_shutdown,
> > +};
> > +module_mipi_dsi_driver(rad_panel_driver);
> > +
> > +MODULE_AUTHOR("Robert Chiras <robert.chiras@xxxxxxx>");
> > +MODULE_DESCRIPTION("DRM Driver for Raydium RM67191 MIPI DSI
> > panel");
> > +MODULE_LICENSE("GPL v2");
> 
> ÂÂÂÂÂÂÂÂSam