Hi GVRao, Thanks for the patch. Sorry for late reply. On Mon, 2020-04-20 at 14:20:56 -0700, Venkateshwar Rao Gannavarapu wrote: > The Xilinx MIPI DSI Tx Subsystem soft IP is used to display video > data from AXI-4 stream interface. > > It supports upto 4 lanes, optional register interface for the DPHY, I don't see the register interface for dphy support. > multiple RGB color formats, command mode and video mode. > This is a MIPI-DSI host driver and provides DSI bus for panels. > This driver also helps to communicate with its panel using panel > framework. > > Signed-off-by: Venkateshwar Rao Gannavarapu <venkateshwar.rao.gannavarapu@xxxxxxxxxx> > --- > drivers/gpu/drm/xlnx/Kconfig | 11 + > drivers/gpu/drm/xlnx/Makefile | 2 + > drivers/gpu/drm/xlnx/xlnx_dsi.c | 755 ++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 768 insertions(+) > create mode 100644 drivers/gpu/drm/xlnx/xlnx_dsi.c > > diff --git a/drivers/gpu/drm/xlnx/Kconfig b/drivers/gpu/drm/xlnx/Kconfig > index aa6cd88..73873cf 100644 > --- a/drivers/gpu/drm/xlnx/Kconfig > +++ b/drivers/gpu/drm/xlnx/Kconfig > @@ -11,3 +11,14 @@ config DRM_ZYNQMP_DPSUB > This is a DRM/KMS driver for ZynqMP DisplayPort controller. Choose > this option if you have a Xilinx ZynqMP SoC with DisplayPort > subsystem. > + > +config DRM_XLNX_DSI > + tristate "Xilinx DRM DSI Subsystem Driver" > + select DRM_MIPI_DSI > + select DRM_PANEL > + select DRM_PANEL_SIMPLE > + help > + This enables support for Xilinx MIPI-DSI. This sentence is not needed with below. Could you please rephrase the whole? > + This is a DRM/KMS driver for Xilinx programmable DSI controller. > + Choose this option if you have a Xilinx MIPI DSI-TX controller > + subsytem. These seem incorrectly indented. > diff --git a/drivers/gpu/drm/xlnx/Makefile b/drivers/gpu/drm/xlnx/Makefile > index 2b844c6..b7ee6ef 100644 > --- a/drivers/gpu/drm/xlnx/Makefile > +++ b/drivers/gpu/drm/xlnx/Makefile > @@ -1,2 +1,4 @@ > zynqmp-dpsub-objs += zynqmp_disp.o zynqmp_dpsub.o zynqmp_dp.o > obj-$(CONFIG_DRM_ZYNQMP_DPSUB) += zynqmp-dpsub.o > + > +obj-$(CONFIG_DRM_XLNX_DSI) += xlnx_dsi.o > diff --git a/drivers/gpu/drm/xlnx/xlnx_dsi.c b/drivers/gpu/drm/xlnx/xlnx_dsi.c > new file mode 100644 > index 0000000..b8cae59 > --- /dev/null > +++ b/drivers/gpu/drm/xlnx/xlnx_dsi.c > @@ -0,0 +1,755 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Xilinx FPGA MIPI DSI Tx Controller driver > + * > + * Copyright (C) 2017 - 2019 Xilinx, Inc. > + * > + * Authors: > + * - Saurabh Sengar <saurabhs@xxxxxxxxxx> > + * - Venkateshwar Rao Gannavarapu <venkateshwar.rao.gannavarapu@xxxxxxxxxx> > + */ > + > +#include <drm/drm_atomic_helper.h> > +#include <drm/drm_connector.h> > +#include <drm/drm_crtc.h> > +#include <drm/drm_crtc_helper.h> > +#include <drm/drm_device.h> > +#include <drm/drm_encoder.h> > +#include <drm/drm_fourcc.h> > +#include <drm/drm_gem_cma_helper.h> > +#include <drm/drm_mipi_dsi.h> > +#include <drm/drm_panel.h> > +#include <drm/drm_probe_helper.h> > + > +#include <linux/clk.h> > +#include <linux/component.h> > +#include <linux/device.h> > +#include <linux/iopoll.h> > +#include <linux/of_device.h> > +#include <linux/of_graph.h> > +#include <linux/phy/phy.h> > + > +#include <video/mipi_display.h> > +#include <video/videomode.h> > + > +/* DSI Tx IP registers */ > +#define XDSI_CCR 0x00 > +#define XDSI_CCR_COREENB BIT(0) > +#define XDSI_CCR_SOFTRST BIT(1) > +#define XDSI_CCR_CRREADY BIT(2) > +#define XDSI_CCR_CMDMODE BIT(3) > +#define XDSI_CCR_DFIFORST BIT(4) > +#define XDSI_CCR_CMDFIFORST BIT(5) > +#define XDSI_PCR 0x04 > +#define XDSI_PCR_VIDEOMODE(x) (((x) & 0x3) << 3) > +#define XDSI_PCR_VIDEOMODE_MASK (0x3 << 3) > +#define XDSI_PCR_VIDEOMODE_SHIFT 3 > +#define XDSI_PCR_BLLPTYPE(x) ((x) << 5) > +#define XDSI_PCR_BLLPMODE(x) ((x) << 6) > +#define XDSI_PCR_EOTPENABLE(x) ((x) << 13) > +#define XDSI_GIER 0x20 > +#define XDSI_ISR 0x24 > +#define XDSI_IER 0x28 > +#define XDSI_STR 0x2C > +#define XDSI_STR_RDY_SHPKT BIT(6) > +#define XDSI_STR_RDY_LNGPKT BIT(7) > +#define XDSI_STR_DFIFO_FULL BIT(8) > +#define XDSI_STR_DFIFO_EMPTY BIT(9) > +#define XDSI_STR_WAITFR_DATA BIT(10) > +#define XDSI_STR_CMD_EXE_PGS BIT(11) > +#define XDSI_STR_CCMD_PROC BIT(12) > +#define XDSI_STR_LPKT_MASK (0x5 << 7) > +#define XDSI_CMD 0x30 > +#define XDSI_CMD_QUEUE_PACKET(x) ((x) & GENMASK(23, 0)) > +#define XDSI_DFR 0x34 > +#define XDSI_TIME1 0x50 > +#define XDSI_TIME1_BLLP_BURST(x) ((x) & GENMASK(15, 0)) > +#define XDSI_TIME1_HSA(x) (((x) & GENMASK(15, 0)) << 16) > +#define XDSI_TIME2 0x54 > +#define XDSI_TIME2_VACT(x) ((x) & GENMASK(15, 0)) > +#define XDSI_TIME2_HACT(x) (((x) & GENMASK(15, 0)) << 16) > +#define XDSI_HACT_MULTIPLIER GENMASK(1, 0) > +#define XDSI_TIME3 0x58 > +#define XDSI_TIME3_HFP(x) ((x) & GENMASK(15, 0)) > +#define XDSI_TIME3_HBP(x) (((x) & GENMASK(15, 0)) << 16) > +#define XDSI_TIME4 0x5c > +#define XDSI_TIME4_VFP(x) ((x) & GENMASK(7, 0)) > +#define XDSI_TIME4_VBP(x) (((x) & GENMASK(7, 0)) << 8) > +#define XDSI_TIME4_VSA(x) (((x) & GENMASK(7, 0)) << 16) > +#define XDSI_LTIME 0x60 > +#define XDSI_BLLP_TIME 0x64 > +/* > + * XDSI_NUM_DATA_T represents number of data types in the > + * enum mipi_dsi_pixel_format in the MIPI DSI part of DRM framework. > + */ > +#define XDSI_NUM_DATA_T 4 > +#define XDSI_VIDEO_MODE_SYNC_PULSE 0x0 > +#define XDSI_VIDEO_MODE_SYNC_EVENT 0x1 > +#define XDSI_VIDEO_MODE_BURST 0x2 Please remove these unused 3 definitions. > + > +#define XDSI_DPHY_CLK_MIN 197000000000UL > +#define XDSI_DPHY_CLK_MAX 203000000000UL > +#define XDSI_DPHY_CLK_REQ 200000000000UL > + > +/* command timeout in usec */ > +#define XDSI_CMD_TIMEOUT_VAL (3000) > + > +/** > + * struct xlnx_dsi - Xilinx DSI-TX core > + * @encoder: DRM encoder structure > + * @dsi_host: DSI host device > + * @connector: DRM connector structure > + * @panel_node: MIPI DSI device panel node > + * @panel: DRM panel structure > + * @dev: device structure > + * @iomem: Base address of DSI subsystem > + * @lanes: number of active data lanes supported by DSI controller > + * @cmdmode: command mode support > + * @mode_flags: DSI operation mode related flags > + * @format: pixel format for video mode of DSI controller > + * @vm: videomode data structure > + * @mul_factor: multiplication factor for HACT timing parameter > + * @video_aclk: Video clock > + * @dphy_clk_200M: 200MHz DPHY clock and AXI Lite clock > + */ > +struct xlnx_dsi { > + struct drm_encoder encoder; > + struct mipi_dsi_host dsi_host; > + struct drm_connector connector; > + struct device_node *panel_node; > + struct drm_panel *panel; > + struct device *dev; > + void __iomem *iomem; > + u32 lanes; > + bool cmdmode; > + u32 mode_flags; > + enum mipi_dsi_pixel_format format; > + struct videomode vm; > + u32 mul_factor; > + struct clk *video_aclk; > + struct clk *dphy_clk_200M; > +}; > + > +#define host_to_dsi(host) container_of(host, struct xlnx_dsi, dsi_host) > +#define connector_to_dsi(c) container_of(c, struct xlnx_dsi, connector) > +#define encoder_to_dsi(e) container_of(e, struct xlnx_dsi, encoder) > + > +static inline void xlnx_dsi_writel(void __iomem *base, int offset, u32 val) > +{ > + writel(val, base + offset); > +} > + > +static inline u32 xlnx_dsi_readl(void __iomem *base, int offset) > +{ > + return readl(base + offset); > +} > + > +/** > + * xlnx_dsi_set_display_mode - Configure DSI timing registers > + * @dsi: xilinx DSI structure > + * > + * This function writes the timing parameters of DSI IP which are > + * retrieved from panel timing values. > + */ > +static void xlnx_dsi_set_display_mode(struct xlnx_dsi *dsi) This function better directly inline to xlnx_dsi_atomic_mode_set(). Then, the videomode is not really needed. Please remove it and use drm_display_mode. > +{ > + struct videomode *vm = &dsi->vm; > + u32 reg, video_mode; > + > + reg = xlnx_dsi_readl(dsi->iomem, XDSI_PCR); > + video_mode = (reg & XDSI_PCR_VIDEOMODE_MASK) >> > + XDSI_PCR_VIDEOMODE_SHIFT; > + > + /* configure the HSA value only if non_burst_sync_pluse video mode */ > + if (!video_mode && > + (dsi->mode_flags & MIPI_DSI_MODE_VIDEO_SYNC_PULSE)) { > + reg = XDSI_TIME1_HSA(vm->hsync_len); > + xlnx_dsi_writel(dsi->iomem, XDSI_TIME1, reg); > + } > + > + reg = XDSI_TIME4_VFP(vm->vfront_porch) | > + XDSI_TIME4_VBP(vm->vback_porch) | > + XDSI_TIME4_VSA(vm->vsync_len); > + xlnx_dsi_writel(dsi->iomem, XDSI_TIME4, reg); > + > + reg = XDSI_TIME3_HFP(vm->hfront_porch) | > + XDSI_TIME3_HBP(vm->hback_porch); > + xlnx_dsi_writel(dsi->iomem, XDSI_TIME3, reg); > + > + dev_dbg(dsi->dev, "mul factor for parsed datatype is = %d\n", > + (dsi->mul_factor) / 100); Remove unneeded (). > + /* > + * The HACT parameter received from panel timing values should be > + * divisible by 4. The reason for this is, the word count given as > + * input to DSI controller is HACT * mul_factor. The mul_factor is > + * 3, 2.25, 2.25, 2 respectively for RGB888, RGB666_L, RGB666_P and > + * RGB565. > + * e.g. for RGB666_L color format and 1080p, the word count is > + * 1920*2.25 = 4320 which is divisible by 4 and it is a valid input > + * to DSI controller. Based on this 2.25 mul factor, we come up with > + * the division factor of (XDSI_HACT_MULTIPLIER) as 4 for checking > + */ > + if ((vm->hactive & XDSI_HACT_MULTIPLIER) != 0) if (!(vm->hactive & XDSI_HACT_MULTIPLIER))? > + dev_warn(dsi->dev, "Incorrect HACT will be programmed\n"); dev_warn() better come with more details. Probably it can mention that hactive is not aligned along with its value. > + > + reg = XDSI_TIME2_HACT((vm->hactive) * (dsi->mul_factor) / 100) | Ditto. No (). > + XDSI_TIME2_VACT(vm->vactive); > + xlnx_dsi_writel(dsi->iomem, XDSI_TIME2, reg); > + > + dev_dbg(dsi->dev, "LCD size = %dx%d\n", vm->hactive, vm->vactive); > +} > + > +/** > + * xlnx_dsi_set_display_enable - Enables the DSI Tx IP core enable > + * register bit > + * @dsi: Xilinx DSI structure > + * > + * This function takes the DSI strucure and enables the core enable bit > + * of core configuration register. > + */ > +static void xlnx_dsi_set_display_enable(struct xlnx_dsi *dsi) > +{ > + u32 reg; > + > + reg = xlnx_dsi_readl(dsi->iomem, XDSI_CCR); > + reg |= XDSI_CCR_COREENB; > + Nit. It can be less lines by removing this empty line and setting the reg at declarartion an so on. > + xlnx_dsi_writel(dsi->iomem, XDSI_CCR, reg); > + dev_dbg(dsi->dev, "MIPI DSI Tx controller is enabled.\n"); > +} > + > +/** > + * xlnx_dsi_set_display_disable - Disable the DSI Tx IP core enable > + * register bit > + * @dsi: Xilinx DSI structure > + * > + * This function takes the DSI strucure and disables the core enable bit > + * of core configuration register. > + */ > +static void xlnx_dsi_set_display_disable(struct xlnx_dsi *dsi) > +{ > + u32 reg; > + > + reg = xlnx_dsi_readl(dsi->iomem, XDSI_CCR); > + reg &= ~XDSI_CCR_COREENB; > + Ditto. > + xlnx_dsi_writel(dsi->iomem, XDSI_CCR, reg); > + dev_dbg(dsi->dev, "DSI Tx is disabled. reset regs to default values\n"); > +} > + > +/** > + * xlnx_dsi_host_transfer - transfer command to panel > + * @host: mipi dsi host structure > + * @msg: mipi dsi msg with type, length and data > + * > + * This function is valid only in command mode. > + * It checks the command fifo empty status and writes into > + * data or cmd register and waits for the completion status. > + * > + * Return: number of bytes, on success and error number on failure Nit. Extra spaces. > + */ > +static ssize_t xlnx_dsi_host_transfer(struct mipi_dsi_host *host, > + const struct mipi_dsi_msg *msg) > +{ > + struct xlnx_dsi *dsi = host_to_dsi(host); > + u32 data0, data1, cmd0, status, val; > + const char *tx_buf = msg->tx_buf; > + > + if (!(xlnx_dsi_readl(dsi->iomem, XDSI_CCR) & (XDSI_CCR_COREENB | > + XDSI_CCR_CMDMODE))) { This passes when one of these 2 bits is set, and I don't think it's right. > + dev_err(dsi->dev, "dsi command mode not enabled\n"); > + return -EINVAL; > + } > + > + if (msg->type == MIPI_DSI_DCS_LONG_WRITE) { > + status = readl_poll_timeout(dsi->iomem + XDSI_STR, val, > + ((val & XDSI_STR_LPKT_MASK) == > + XDSI_STR_LPKT_MASK), 1, Please remove unneeded (). Same for rest below. > + XDSI_CMD_TIMEOUT_VAL); It'd be nice if these delay / timeout values are described.. > + if (status) { > + dev_err(dsi->dev, "long cmd fifo not empty!\n"); > + return -ETIMEDOUT; Isn't this checking if the controller is ready for long packet transaction? Then maybe -EBUSY is better. For timeout, the 'status' can propagate. Same for below. > + } > + data0 = tx_buf[0] | (tx_buf[1] << 8) | (tx_buf[2] << 16) | > + (tx_buf[3] << 24); > + data1 = tx_buf[4] | (tx_buf[5] << 8); > + cmd0 = msg->type | (MIPI_DSI_DCS_READ << 8); > + > + xlnx_dsi_writel(dsi->iomem, XDSI_DFR, data0); > + xlnx_dsi_writel(dsi->iomem, XDSI_DFR, data1); > + xlnx_dsi_writel(dsi->iomem, XDSI_CMD, cmd0); > + } else { I'm not familiar with DSI transaction handling, but is it safe to assume all other than long write is short packet write (param, without param)? > + data0 = tx_buf[0]; This line can be removed. > + if (msg->type == MIPI_DSI_DCS_SHORT_WRITE_PARAM) > + data0 = MIPI_DSI_DCS_SHORT_WRITE_PARAM | > + (tx_buf[0] << 8) | (tx_buf[1] << 16); > + else > + data0 = MIPI_DSI_DCS_SHORT_WRITE | (tx_buf[0] << 8); I'd put () for this if - else statments, but it may be just me. Then, this can mvoe to right before xlnx_dsi_writel() below. > + > + status = readl_poll_timeout(dsi->iomem + XDSI_STR, val, > + ((val & XDSI_STR_RDY_SHPKT) == > + XDSI_STR_RDY_SHPKT), 1, > + XDSI_CMD_TIMEOUT_VAL); > + if (status) { > + dev_err(dsi->dev, "short cmd fifo not empty\n"); > + return -ETIMEDOUT; > + } > + xlnx_dsi_writel(dsi->iomem, XDSI_CMD, data0); > + } > + > + status = readl_poll_timeout(dsi->iomem + XDSI_STR, val, > + (!(val & XDSI_STR_CMD_EXE_PGS)), 1, > + XDSI_CMD_TIMEOUT_VAL); > + if (status) { > + dev_err(dsi->dev, "cmd time out\n"); > + return -ETIMEDOUT; > + } > + > + return msg->tx_len; > +} > + > +static int xlnx_dsi_host_attach(struct mipi_dsi_host *host, > + struct mipi_dsi_device *device) > +{ > + u32 panel_lanes; > + struct xlnx_dsi *dsi = host_to_dsi(host); > + > + panel_lanes = device->lanes; I'd do this above, u32 panel_lanes = device->lanes > + dsi->mode_flags = device->mode_flags; > + dsi->panel_node = device->dev.of_node; > + > + if (panel_lanes != dsi->lanes) { > + dev_err(dsi->dev, "Mismatch of lanes. panel = %d, DSI = %d\n", > + panel_lanes, dsi->lanes); > + return -EINVAL; > + } > + > + if (device->format != dsi->format) { > + dev_err(dsi->dev, "Mismatch of format. panel = %d, DSI = %d\n", > + device->format, dsi->format); > + return -EINVAL; > + } > + > + if (dsi->connector.dev) > + drm_helper_hpd_irq_event(dsi->connector.dev); > + > + return 0; > +} > + > +static int xlnx_dsi_host_detach(struct mipi_dsi_host *host, > + struct mipi_dsi_device *device) > +{ > + struct xlnx_dsi *dsi = host_to_dsi(host); > + > + if (dsi->panel) { > + drm_panel_detach(dsi->panel); > + dsi->panel = NULL; > + } > + > + if (dsi->connector.dev) > + drm_helper_hpd_irq_event(dsi->connector.dev); > + > + return 0; > +} > + > +static const struct mipi_dsi_host_ops xlnx_dsi_ops = { > + .attach = xlnx_dsi_host_attach, > + .detach = xlnx_dsi_host_detach, > + .transfer = xlnx_dsi_host_transfer, > +}; > + > +static int xlnx_dsi_connector_dpms(struct drm_connector *connector, > + int mode) > +{ > + struct xlnx_dsi *dsi = connector_to_dsi(connector); > + int ret; > + > + dev_dbg(dsi->dev, "connector dpms state: %d\n", mode); > + > + switch (mode) { > + case DRM_MODE_DPMS_ON: > + ret = drm_panel_prepare(dsi->panel); > + if (ret < 0) { > + dev_err(dsi->dev, "DRM panel not found\n"); > + return ret; > + } > + > + ret = drm_panel_enable(dsi->panel); > + if (ret < 0) { > + drm_panel_unprepare(dsi->panel); > + dev_err(dsi->dev, "DRM panel not enabled\n"); > + return ret; > + } > + break; > + default: > + drm_panel_disable(dsi->panel); > + drm_panel_unprepare(dsi->panel); > + break; > + } > + > + return drm_helper_connector_dpms(connector, mode); > +} > + > +static enum drm_connector_status > +xlnx_dsi_detect(struct drm_connector *connector, bool force) > +{ > + struct xlnx_dsi *dsi = connector_to_dsi(connector); > + > + if (!dsi->panel) { > + dsi->panel = of_drm_find_panel(dsi->panel_node); > + if (dsi->panel) { > + drm_panel_attach(dsi->panel, &dsi->connector); Above are quite static, so can't it be done in host attach? Or do you see more dynamic use cases? > + if (dsi->cmdmode) { > + xlnx_dsi_writel(dsi->iomem, XDSI_CCR, > + XDSI_CCR_CMDMODE | > + XDSI_CCR_COREENB); > + drm_panel_prepare(dsi->panel); Just to confirm, is this not needed for non-command mode? And how is this aligned with same call in dpms(), meaning the panel will be prepared twice in command mode. > + xlnx_dsi_writel(dsi->iomem, XDSI_CCR, 0); > + } > + } > + } else if (!dsi->panel_node) { > + xlnx_dsi_connector_dpms(connector, DRM_MODE_DPMS_OFF); > + drm_panel_detach(dsi->panel); > + dsi->panel = NULL; > + } > + > + if (dsi->panel) > + return connector_status_connected; > + > + return connector_status_disconnected; > +} > + > +static void xlnx_dsi_connector_destroy(struct drm_connector *connector) > +{ > + drm_connector_unregister(connector); > + drm_connector_cleanup(connector); > + connector->dev = NULL; > +} > + > +static const struct drm_connector_funcs xlnx_dsi_connector_funcs = { > + .detect = xlnx_dsi_detect, > + .fill_modes = drm_helper_probe_single_connector_modes, > + .destroy = xlnx_dsi_connector_destroy, > + .reset = drm_atomic_helper_connector_reset, This better indent more consistently. Please do it through the entire file. > + .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, > + .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, > + .dpms = xlnx_dsi_connector_dpms, > +}; > + > +static int xlnx_dsi_get_modes(struct drm_connector *connector) > +{ > + struct xlnx_dsi *dsi = connector_to_dsi(connector); > + > + if (dsi->panel) > + return drm_panel_get_modes(dsi->panel, connector); > + > + return 0; > +} > + > +static struct drm_encoder * > +xlnx_dsi_best_encoder(struct drm_connector *connector) > +{ > + return &(connector_to_dsi(connector)->encoder); > +} > + > +static const struct drm_connector_helper_funcs > +xlnx_dsi_connector_helper_funcs = { > + .get_modes = xlnx_dsi_get_modes, > + .best_encoder = xlnx_dsi_best_encoder, > +}; > + > +static int xlnx_dsi_create_connector(struct drm_encoder *encoder) > +{ > + struct xlnx_dsi *dsi = encoder_to_dsi(encoder); > + struct drm_connector *connector = &dsi->connector; > + struct drm_device *drm = encoder->dev; > + int ret; > + > + connector->polled = DRM_CONNECTOR_POLL_HPD; > + > + ret = drm_connector_init(encoder->dev, connector, > + &xlnx_dsi_connector_funcs, > + DRM_MODE_CONNECTOR_DSI); > + if (ret) { > + dev_err(dsi->dev, "Failed to initialize connector with drm\n"); > + return ret; > + } > + > + drm_connector_helper_add(connector, &xlnx_dsi_connector_helper_funcs); > + drm_connector_attach_encoder(connector, encoder); > + if (!drm->registered) > + return 0; > + > + drm_connector_register(connector); > + > + return 0; > +} > + > +/** > + * xlnx_dsi_atomic_mode_set - derive the DSI timing parameters > + * > + * @encoder: pointer to Xilinx DRM encoder > + * @crtc_state: Pointer to drm core crtc state > + * @connector_state: DSI connector drm state > + * > + * This function derives the DSI IP timing parameters from the timing > + * values given in the attached panel driver. > + */ > +static void > +xlnx_dsi_atomic_mode_set(struct drm_encoder *encoder, > + struct drm_crtc_state *crtc_state, > + struct drm_connector_state *connector_state) > +{ > + struct xlnx_dsi *dsi = encoder_to_dsi(encoder); > + struct videomode *vm = &dsi->vm; > + struct drm_display_mode *m = &crtc_state->adjusted_mode; > + > + vm->hactive = m->hdisplay; > + vm->vactive = m->vdisplay; > + vm->vfront_porch = m->vsync_start - m->vdisplay; > + vm->vback_porch = m->vtotal - m->vsync_end; > + vm->vsync_len = m->vsync_end - m->vsync_start; > + vm->hfront_porch = m->hsync_start - m->hdisplay; > + vm->hback_porch = m->htotal - m->hsync_end; > + vm->hsync_len = m->hsync_end - m->hsync_start; > + xlnx_dsi_set_display_mode(dsi); As commented above, this doesn't have to be re-directed through videomode. Then the function can be inlined here. > +} > + > +static void xlnx_dsi_disable(struct drm_encoder *encoder) > +{ > + struct xlnx_dsi *dsi = encoder_to_dsi(encoder); > + > + xlnx_dsi_set_display_disable(dsi); xlnx_dsi_set_display_disable(encoder_to_dsi(encoder)); Maybe even this wrapper can be removed? > +} > + > +static void xlnx_dsi_enable(struct drm_encoder *encoder) > +{ > + struct xlnx_dsi *dsi = encoder_to_dsi(encoder); > + > + xlnx_dsi_set_display_enable(dsi); Ditto. > +} > + > +static const struct drm_encoder_helper_funcs > +xlnx_dsi_encoder_helper_funcs = { > + .atomic_mode_set = xlnx_dsi_atomic_mode_set, > + .enable = xlnx_dsi_enable, > + .disable = xlnx_dsi_disable, > +}; > + > +static const struct drm_encoder_funcs xlnx_dsi_encoder_funcs = { > + .destroy = drm_encoder_cleanup, > +}; > + > +static int xlnx_dsi_parse_dt(struct xlnx_dsi *dsi) > +{ > + struct device *dev = dsi->dev; > + struct device_node *node = dev->of_node; > + int ret; > + u32 datatype; > + static const int xdsi_mul_fact[XDSI_NUM_DATA_T] = {300, 225, 225, 200}; > + > + dsi->dphy_clk_200M = devm_clk_get(dev, "dphy_clk_200M"); > + if (IS_ERR(dsi->dphy_clk_200M)) { > + ret = PTR_ERR(dsi->dphy_clk_200M); > + dev_err(dev, "failed to get dphy_clk_200M %d\n", ret); > + return ret; > + } > + > + dsi->video_aclk = devm_clk_get(dev, "s_axis_aclk"); > + if (IS_ERR(dsi->video_aclk)) { > + ret = PTR_ERR(dsi->video_aclk); > + dev_err(dev, "failed to get video_clk %d\n", ret); > + return ret; > + } > + > + ret = of_property_read_u32(node, "xlnx,dsi-num-lanes", &dsi->lanes); > + if (ret < 0) { > + dev_err(dsi->dev, "missing xlnx,dsi-num-lanes property\n"); > + return ret; > + } > + if (dsi->lanes > 4 || dsi->lanes < 1) { > + dev_err(dsi->dev, "%d lanes : invalid lanes\n", dsi->lanes); > + return -EINVAL; > + } Nit. An empty line here would be helpful for readability. > + ret = of_property_read_u32(node, "xlnx,dsi-data-type", &datatype); > + if (ret < 0) { > + dev_err(dsi->dev, "missing xlnx,dsi-data-type property\n"); > + return ret; > + } > + dsi->format = datatype; > + if (datatype > MIPI_DSI_FMT_RGB565) { > + dev_err(dsi->dev, "Invalid xlnx,dsi-data-type string\n"); > + return -EINVAL; > + } > + > + /* > + * multiplication factor used for HACT, based on data type. > + * > + * e.g. for RGB666_L datatype and 1920x1080 resolution, > + * the Hact (WC) would be as follows - > + * 1920 pixels * 18 bits per pixel / 8 bits per byte > + * = 1920 pixels * 2.25 bytes per pixel = 4320 bytes. > + * > + * Data Type - Multiplication factor > + * RGB888 - 3 > + * RGB666_L - 2.25 > +- * RGB666_P - 2.25 > + * RGB565 - 2 > + * > + * Since the multiplication factor maybe a floating number, > + * a 100x multiplication factor is used. > + */ > + dsi->mul_factor = xdsi_mul_fact[datatype]; > + > + dsi->cmdmode = of_property_read_bool(node, "xlnx,dsi-cmd-mode"); > + > + dev_dbg(dsi->dev, "DSI controller num lanes = %d", dsi->lanes); > + dev_dbg(dsi->dev, "DSI controller datatype = %d\n", datatype); > + dev_dbg(dsi->dev, "DSI controller cmd mode = %d\n", dsi->cmdmode); > + > + return 0; > +} > + > +static int xlnx_dsi_bind(struct device *dev, struct device *master, > + void *data) > +{ > + struct xlnx_dsi *dsi = dev_get_drvdata(dev); > + struct drm_encoder *encoder = &dsi->encoder; > + struct drm_device *drm_dev = data; > + int ret; > + > + drm_encoder_init(drm_dev, encoder, &xlnx_dsi_encoder_funcs, > + DRM_MODE_ENCODER_DSI, NULL); > + drm_encoder_helper_add(encoder, &xlnx_dsi_encoder_helper_funcs); > + > + encoder->possible_crtcs = 1; > + > + ret = xlnx_dsi_create_connector(encoder); > + if (ret) { > + dev_err(dsi->dev, "fail creating connector, ret = %d\n", ret); > + drm_encoder_cleanup(encoder); > + return ret; > + } > + > + ret = mipi_dsi_host_register(&dsi->dsi_host); > + if (ret < 0) { Just nit. if (ret) would do. > + dev_err(dsi->dev, "failed to register DSI host: %d\n", ret); > + xlnx_dsi_connector_destroy(&dsi->connector); > + drm_encoder_cleanup(encoder); > + return ret; > + } > + > + return 0; > +} > + > +static void xlnx_dsi_unbind(struct device *dev, struct device *master, > + void *data) > +{ > + struct xlnx_dsi *dsi = dev_get_drvdata(dev); > + > + xlnx_dsi_disable(&dsi->encoder); > + mipi_dsi_host_unregister(&dsi->dsi_host); > +} > + > +static const struct component_ops xlnx_dsi_component_ops = { > + .bind = xlnx_dsi_bind, > + .unbind = xlnx_dsi_unbind, > +}; > + > +static int xlnx_dsi_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct resource *res; > + struct xlnx_dsi *dsi; > + int ret; > + unsigned long rate; > + > + dsi = devm_kzalloc(dev, sizeof(*dsi), GFP_KERNEL); > + if (!dsi) > + return -ENOMEM; > + > + dsi->dsi_host.ops = &xlnx_dsi_ops; > + dsi->dsi_host.dev = dev; > + dsi->dev = dev; > + > + ret = xlnx_dsi_parse_dt(dsi); > + if (ret) > + return ret; > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + dsi->iomem = devm_ioremap_resource(dev, res); > + if (IS_ERR(dsi->iomem)) { > + dev_err(dev, "failed to remap io region\n"); > + return PTR_ERR(dsi->iomem); > + } > + platform_set_drvdata(pdev, dsi); > + > + ret = clk_set_rate(dsi->dphy_clk_200M, XDSI_DPHY_CLK_REQ); > + if (ret) { > + dev_err(dev, "failed to set dphy clk rate %d\n", ret); > + return ret; > + } > + > + ret = clk_prepare_enable(dsi->dphy_clk_200M); > + if (ret) { > + dev_err(dev, "failed to enable dphy clk %d\n", ret); > + return ret; > + } > + > + rate = clk_get_rate(dsi->dphy_clk_200M); > + if (rate < XDSI_DPHY_CLK_MIN && rate > XDSI_DPHY_CLK_MAX) { A brief comment for this line wold be helpful. > + dev_err(dev, "Error DPHY clock = %lu\n", rate); > + ret = -EINVAL; > + goto err_disable_dphy_clk; > + } > + > + ret = clk_prepare_enable(dsi->video_aclk); > + if (ret) { > + dev_err(dev, "failed to enable video clk %d\n", ret); > + goto err_disable_dphy_clk; > + } > + > + ret = component_add(dev, &xlnx_dsi_component_ops); > + if (ret < 0) I prefer if (ret) where possible, but it may be just me. I let you decide. > + goto err_disable_video_clk; > + > + return ret; return 0;? but up to you. > + > +err_disable_video_clk: > + clk_disable_unprepare(dsi->video_aclk); > +err_disable_dphy_clk: > + clk_disable_unprepare(dsi->dphy_clk_200M); > + return ret; > +} At least, this initial one better come with a complete display pipeline, ex crtc / plane are missing. This won't be functional without those. I believe Laurent was planning to make the xlnx drm layer as a library, so hopefully this can be a good starting point and come with drivers aligned with it. > + > +static int xlnx_dsi_remove(struct platform_device *pdev) > +{ > + struct xlnx_dsi *dsi = platform_get_drvdata(pdev); > + > + component_del(&pdev->dev, &xlnx_dsi_component_ops); > + clk_disable_unprepare(dsi->video_aclk); > + clk_disable_unprepare(dsi->dphy_clk_200M); > + > + return 0; > +} > + > +static const struct of_device_id xlnx_dsi_of_match[] = { > + { .compatible = "xlnx-dsi", }, This should change. I believe it should be something like, xlnx,<dsi ip name>-<version> Thanks, -hyun > + { /* end of table */ }, > +}; > +MODULE_DEVICE_TABLE(of, xlnx_dsi_of_match); > + > +static struct platform_driver dsi_driver = { > + .probe = xlnx_dsi_probe, > + .remove = xlnx_dsi_remove, > + .driver = { > + .name = "xlnx-dsi", > + .of_match_table = xlnx_dsi_of_match, > + }, > +}; > + > +module_platform_driver(dsi_driver); > + > +MODULE_AUTHOR("Xilinx, Inc."); > +MODULE_DESCRIPTION("Xilinx FPGA MIPI DSI Tx Driver"); > +MODULE_LICENSE("GPL v2"); > -- > 2.7.4 > _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel