[PATCH 0/4] Generic MIPI Panel driver

[PATCH 0/4] Generic MIPI Panel driver

[Shobhit Kumar]

Hi,
This series enabled generic MIPI panel driver support, the ground for which
has been built up from the patches - 

http://lists.freedesktop.org/archives/intel-gfx/2014-February/040764.html
http://lists.freedesktop.org/archives/intel-gfx/2014-April/043646.html

With a version of these patches Asus T100A was found to be working by Arjan
who has a device. I do not have the device to test directly. Will be getting
one and verifying on that. But ideally should work. I have verified these patches 
on couple of internal panels.

Also some minor changes in intel_dsi.c to support the generic panel driver
are done.

Regards
Shobhit

Shobhit Kumar (4):
  drm/i915: Correct MIPI operation mode as per expected values from VBT
  drm/i915: MIPI init count programming as generic parameter
  drm/i915: MIPI PPS delays added
  drm/i915: Add support for Generic MIPI panel driver

 drivers/gpu/drm/i915/Makefile              |   1 +
 drivers/gpu/drm/i915/dsi_mod_vbt_generic.c | 598 +++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h           |   4 +-
 drivers/gpu/drm/i915/intel_dsi.c           |  17 +-
 drivers/gpu/drm/i915/intel_dsi.h           |  15 +-
 5 files changed, 630 insertions(+), 5 deletions(-)
 create mode 100644 drivers/gpu/drm/i915/dsi_mod_vbt_generic.c

-- 
1.8.3.2

[PATCH 1/4] drm/i915: Correct MIPI operation mode as per expected values from VBT

[Shobhit Kumar]

In VBT fields operation mode is 0 for Video mode and 1 for command mode.
This field will be directly used as is in generic panel driver. So
adjust accordingly.

Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
---
 drivers/gpu/drm/i915/intel_drv.h | 4 ++--
 drivers/gpu/drm/i915/intel_dsi.c | 4 ++--
 drivers/gpu/drm/i915/intel_dsi.h | 4 +++-
 3 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index c551472..fca11f3 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -106,8 +106,8 @@
 #define INTEL_DVO_CHIP_TMDS 2
 #define INTEL_DVO_CHIP_TVOUT 4
 
-#define INTEL_DSI_COMMAND_MODE	0
-#define INTEL_DSI_VIDEO_MODE	1
+#define INTEL_DSI_VIDEO_MODE	0
+#define INTEL_DSI_COMMAND_MODE	1
 
 struct intel_framebuffer {
 	struct drm_framebuffer base;
diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
index 4e271c7..2795782 100644
--- a/drivers/gpu/drm/i915/intel_dsi.c
+++ b/drivers/gpu/drm/i915/intel_dsi.c
@@ -59,12 +59,12 @@ static struct intel_dsi *intel_attached_dsi(struct drm_connector *connector)
 
 static inline bool is_vid_mode(struct intel_dsi *intel_dsi)
 {
-	return intel_dsi->dev.type == INTEL_DSI_VIDEO_MODE;
+	return intel_dsi->operation_mode == INTEL_DSI_VIDEO_MODE;
 }
 
 static inline bool is_cmd_mode(struct intel_dsi *intel_dsi)
 {
-	return intel_dsi->dev.type == INTEL_DSI_COMMAND_MODE;
+	return intel_dsi->operation_mode == INTEL_DSI_COMMAND_MODE;
 }
 
 static void intel_dsi_hot_plug(struct intel_encoder *encoder)
diff --git a/drivers/gpu/drm/i915/intel_dsi.h b/drivers/gpu/drm/i915/intel_dsi.h
index 550714c..1649422 100644
--- a/drivers/gpu/drm/i915/intel_dsi.h
+++ b/drivers/gpu/drm/i915/intel_dsi.h
@@ -31,7 +31,6 @@
 struct intel_dsi_device {
 	unsigned int panel_id;
 	const char *name;
-	int type;
 	const struct intel_dsi_dev_ops *dev_ops;
 	void *dev_priv;
 };
@@ -85,6 +84,9 @@ struct intel_dsi {
 	/* virtual channel */
 	int channel;
 
+	/* Video mode or command mode */
+	u16 operation_mode;
+
 	/* number of DSI lanes */
 	unsigned int lane_count;
 
-- 
1.8.3.2

[PATCH 2/4] drm/i915: MIPI init count programming as generic parameter

[Shobhit Kumar]

Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
---
 drivers/gpu/drm/i915/intel_dsi.c | 3 +++
 drivers/gpu/drm/i915/intel_dsi.h | 2 ++
 2 files changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
index 2795782..09b9318 100644
--- a/drivers/gpu/drm/i915/intel_dsi.c
+++ b/drivers/gpu/drm/i915/intel_dsi.c
@@ -525,6 +525,9 @@ static void intel_dsi_mode_set(struct intel_encoder *intel_encoder)
 	/* recovery disables */
 	I915_WRITE(MIPI_EOT_DISABLE(pipe), val);
 
+	/* in terms of low power clock */
+	I915_WRITE(MIPI_INIT_COUNT(pipe), intel_dsi->init_count);
+
 	/* in terms of txbyteclkhs. actual high to low switch +
 	 * MIPI_STOP_STATE_STALL * MIPI_LP_BYTECLK.
 	 *
diff --git a/drivers/gpu/drm/i915/intel_dsi.h b/drivers/gpu/drm/i915/intel_dsi.h
index 1649422..be132c5 100644
--- a/drivers/gpu/drm/i915/intel_dsi.h
+++ b/drivers/gpu/drm/i915/intel_dsi.h
@@ -114,6 +114,8 @@ struct intel_dsi {
 	u16 hs_to_lp_count;
 	u16 clk_lp_to_hs_count;
 	u16 clk_hs_to_lp_count;
+
+	u16 init_count;
 };
 
 static inline struct intel_dsi *enc_to_intel_dsi(struct drm_encoder *encoder)
-- 
1.8.3.2

[PATCH 3/4] drm/i915: MIPI PPS delays added

[Shobhit Kumar]

Added as generic parameters which will be initialized in the panel
driver and are specific to panels.

Backlight delays have also kept as placeholders and will be used used
once we have MIPI backlight enabling support

Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
---
 drivers/gpu/drm/i915/intel_dsi.c | 5 +++++
 drivers/gpu/drm/i915/intel_dsi.h | 7 +++++++
 2 files changed, 12 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
index 09b9318..0d4dd54 100644
--- a/drivers/gpu/drm/i915/intel_dsi.c
+++ b/drivers/gpu/drm/i915/intel_dsi.c
@@ -185,6 +185,8 @@ static void intel_dsi_pre_enable(struct intel_encoder *encoder)
 	/* put device in ready state */
 	intel_dsi_device_ready(encoder);
 
+	msleep(intel_dsi->panel_on_delay);
+
 	if (intel_dsi->dev.dev_ops->panel_reset)
 		intel_dsi->dev.dev_ops->panel_reset(&intel_dsi->dev);
 
@@ -301,6 +303,9 @@ static void intel_dsi_post_disable(struct intel_encoder *encoder)
 
 	if (intel_dsi->dev.dev_ops->disable_panel_power)
 		intel_dsi->dev.dev_ops->disable_panel_power(&intel_dsi->dev);
+
+	msleep(intel_dsi->panel_off_delay);
+	msleep(intel_dsi->panel_pwr_cycle_delay);
 }
 
 static bool intel_dsi_get_hw_state(struct intel_encoder *encoder,
diff --git a/drivers/gpu/drm/i915/intel_dsi.h b/drivers/gpu/drm/i915/intel_dsi.h
index be132c5..e3f4e91 100644
--- a/drivers/gpu/drm/i915/intel_dsi.h
+++ b/drivers/gpu/drm/i915/intel_dsi.h
@@ -116,6 +116,13 @@ struct intel_dsi {
 	u16 clk_hs_to_lp_count;
 
 	u16 init_count;
+
+	/* all delays in ms */
+	u16 backlight_off_delay;
+	u16 backlight_on_delay;
+	u16 panel_on_delay;
+	u16 panel_off_delay;
+	u16 panel_pwr_cycle_delay;
 };
 
 static inline struct intel_dsi *enc_to_intel_dsi(struct drm_encoder *encoder)
-- 
1.8.3.2

[PATCH 4/4] drm/i915: Add support for Generic MIPI panel driver

[Shobhit Kumar]

This driver makes use of the generic panel information from the VBT.
Panel information is classified into two - panel configuration and panel
power sequence which is unique to each panel. The generic driver uses the
panel configuration and sequence parsed from VBT block #52 and #53

v2: Address review comments by Jani
    - Move all of the things in driver c file from header
    - Make all functions static
    - Make use of video/mipi_display.c instead of redefining
    - Null checks during sequence execution

Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
---
 drivers/gpu/drm/i915/Makefile              |   1 +
 drivers/gpu/drm/i915/dsi_mod_vbt_generic.c | 598 +++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_dsi.c           |   5 +
 drivers/gpu/drm/i915/intel_dsi.h           |   2 +
 4 files changed, 606 insertions(+)
 create mode 100644 drivers/gpu/drm/i915/dsi_mod_vbt_generic.c

diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index b1445b7..756a7a4 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -55,6 +55,7 @@ i915-y += dvo_ch7017.o \
 	  intel_dsi_cmd.o \
 	  intel_dsi.o \
 	  intel_dsi_pll.o \
+	  dsi_mod_vbt_generic.o	\
 	  intel_dvo.o \
 	  intel_hdmi.o \
 	  intel_i2c.o \
diff --git a/drivers/gpu/drm/i915/dsi_mod_vbt_generic.c b/drivers/gpu/drm/i915/dsi_mod_vbt_generic.c
new file mode 100644
index 0000000..0c12ce8
--- /dev/null
+++ b/drivers/gpu/drm/i915/dsi_mod_vbt_generic.c
@@ -0,0 +1,598 @@
+/*
+ * Copyright © 2014 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
+ * DEALINGS IN THE SOFTWARE.
+ *
+ * Author: Shobhit Kumar <shobhit.kumar@intel.com>
+ *
+ */
+
+#include <drm/drmP.h>
+#include <drm/drm_crtc.h>
+#include <drm/drm_edid.h>
+#include <drm/i915_drm.h>
+#include <linux/slab.h>
+#include <video/mipi_display.h>
+#include <asm/intel-mid.h>
+#include <video/mipi_display.h>
+#include "i915_drv.h"
+#include "intel_drv.h"
+#include "intel_dsi.h"
+#include "intel_dsi_cmd.h"
+
+#define MIPI_TRANSFER_MODE_SHIFT	0
+#define MIPI_VIRTUAL_CHANNEL_SHIFT	1
+#define MIPI_PORT_SHIFT			3
+
+#define PREPARE_CNT_MAX		0x3F
+#define EXIT_ZERO_CNT_MAX	0x3F
+#define CLK_ZERO_CNT_MAX	0xFF
+#define TRAIL_CNT_MAX		0x1F
+
+#define NS_MHZ_RATIO 1000000
+
+/* This macro divides two integers and rounds fractional values up
+ * to the nearest integer value. */
+#define CEIL_DIV(X, Y) (((X)%(Y)) ? ((X)/(Y)+1) : ((X)/(Y)))
+
+#define GPI0_NC_0_HV_DDI0_HPD           0x4130
+#define GPIO_NC_0_HV_DDI0_PAD           0x4138
+#define GPIO_NC_1_HV_DDI0_DDC_SDA       0x4120
+#define GPIO_NC_1_HV_DDI0_DDC_SDA_PAD   0x4128
+#define GPIO_NC_2_HV_DDI0_DDC_SCL       0x4110
+#define GPIO_NC_2_HV_DDI0_DDC_SCL_PAD   0x4118
+#define GPIO_NC_3_PANEL0_VDDEN          0x4140
+#define GPIO_NC_3_PANEL0_VDDEN_PAD      0x4148
+#define GPIO_NC_4_PANEL0_BLKEN          0x4150
+#define GPIO_NC_4_PANEL0_BLKEN_PAD      0x4158
+#define GPIO_NC_5_PANEL0_BLKCTL         0x4160
+#define GPIO_NC_5_PANEL0_BLKCTL_PAD     0x4168
+#define GPIO_NC_6_PCONF0                0x4180
+#define GPIO_NC_6_PAD                   0x4188
+#define GPIO_NC_7_PCONF0                0x4190
+#define GPIO_NC_7_PAD                   0x4198
+#define GPIO_NC_8_PCONF0                0x4170
+#define GPIO_NC_8_PAD                   0x4178
+#define GPIO_NC_9_PCONF0                0x4100
+#define GPIO_NC_9_PAD                   0x4108
+#define GPIO_NC_10_PCONF0               0x40E0
+#define GPIO_NC_10_PAD                  0x40E8
+#define GPIO_NC_11_PCONF0               0x40F0
+#define GPIO_NC_11_PAD                  0x40F8
+
+struct gpio_table {
+	u16 function_reg;
+	u16 pad_reg;
+	u8 init;
+};
+
+static struct gpio_table gtable[] = {
+	{ GPI0_NC_0_HV_DDI0_HPD, GPIO_NC_0_HV_DDI0_PAD, 0 },
+	{ GPIO_NC_1_HV_DDI0_DDC_SDA, GPIO_NC_1_HV_DDI0_DDC_SDA_PAD, 0 },
+	{ GPIO_NC_2_HV_DDI0_DDC_SCL, GPIO_NC_2_HV_DDI0_DDC_SCL_PAD, 0 },
+	{ GPIO_NC_3_PANEL0_VDDEN, GPIO_NC_3_PANEL0_VDDEN_PAD, 0 },
+	{ GPIO_NC_4_PANEL0_BLKEN, GPIO_NC_4_PANEL0_BLKEN_PAD, 0 },
+	{ GPIO_NC_5_PANEL0_BLKCTL, GPIO_NC_5_PANEL0_BLKCTL_PAD, 0 },
+	{ GPIO_NC_6_PCONF0, GPIO_NC_6_PAD, 0 },
+	{ GPIO_NC_7_PCONF0, GPIO_NC_7_PAD, 0 },
+	{ GPIO_NC_8_PCONF0, GPIO_NC_8_PAD, 0 },
+	{ GPIO_NC_9_PCONF0, GPIO_NC_9_PAD, 0 },
+	{ GPIO_NC_10_PCONF0, GPIO_NC_10_PAD, 0},
+	{ GPIO_NC_11_PCONF0, GPIO_NC_11_PAD, 0}
+};
+
+static u8 *mipi_exec_send_packet(struct intel_dsi *intel_dsi, u8 *data)
+{
+	u8 type, byte, mode, vc, port;
+	u16 len;
+
+	byte = *data++;
+	mode = (byte >> MIPI_TRANSFER_MODE_SHIFT) & 0x1;
+	vc = (byte >> MIPI_VIRTUAL_CHANNEL_SHIFT) & 0x3;
+	port = (byte >> MIPI_PORT_SHIFT) & 0x3;
+
+	/* LP or HS mode */
+	intel_dsi->hs = mode;
+
+	/* get packet type and increment the pointer */
+	type = *data++;
+
+	len = *((u16 *) data);
+	data += 2;
+
+	switch (type) {
+	case MIPI_DSI_GENERIC_SHORT_WRITE_0_PARAM:
+		dsi_vc_generic_write_0(intel_dsi, vc);
+		break;
+	case MIPI_DSI_GENERIC_SHORT_WRITE_1_PARAM:
+		dsi_vc_generic_write_1(intel_dsi, vc, *data);
+		break;
+	case MIPI_DSI_GENERIC_SHORT_WRITE_2_PARAM:
+		dsi_vc_generic_write_2(intel_dsi, vc, *data, *(data + 1));
+		break;
+	case MIPI_DSI_GENERIC_READ_REQUEST_0_PARAM:
+	case MIPI_DSI_GENERIC_READ_REQUEST_1_PARAM:
+	case MIPI_DSI_GENERIC_READ_REQUEST_2_PARAM:
+		DRM_DEBUG_DRIVER("Generic Read not yet implemented or used\n");
+		break;
+	case MIPI_DSI_GENERIC_LONG_WRITE:
+		dsi_vc_generic_write(intel_dsi, vc, data, len);
+		break;
+	case MIPI_DSI_DCS_SHORT_WRITE:
+		dsi_vc_dcs_write_0(intel_dsi, vc, *data);
+		break;
+	case MIPI_DSI_DCS_SHORT_WRITE_PARAM:
+		dsi_vc_dcs_write_1(intel_dsi, vc, *data, *(data + 1));
+		break;
+	case MIPI_DSI_DCS_READ:
+		DRM_DEBUG_DRIVER("DCS Read not yet implemented or used\n");
+		break;
+	case MIPI_DSI_DCS_LONG_WRITE:
+		dsi_vc_dcs_write(intel_dsi, vc, data, len);
+		break;
+	};
+
+	data += len;
+
+	return data;
+}
+
+static u8 *mipi_exec_delay(struct intel_dsi *intel_dsi, u8 *data)
+{
+	u32 delay = *((u32 *) data);
+
+	DRM_DEBUG_DRIVER("MIPI: executing delay element\n");
+	usleep_range(delay, delay + 10);
+	data += 4;
+
+	return data;
+}
+
+static u8 *mipi_exec_gpio(struct intel_dsi *intel_dsi, u8 *data)
+{
+	u8 gpio, action;
+	u16 function, pad;
+	u32 val;
+	struct drm_device *dev = intel_dsi->base.base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	DRM_DEBUG_DRIVER("MIPI: executing gpio element\n");
+	gpio = *data++;
+
+	/* pull up/down */
+	action = *data++;
+
+	function = gtable[gpio].function_reg;
+	pad = gtable[gpio].pad_reg;
+
+	mutex_lock(&dev_priv->dpio_lock);
+	if (!gtable[gpio].init) {
+		/* program the function */
+		vlv_gpio_nc_write(dev_priv, function, 0x2000CC00);
+		gtable[gpio].init = 1;
+	}
+
+	val = 0x4 | action;
+
+	/* pull up/down */
+	vlv_gpio_nc_write(dev_priv, pad, val);
+	mutex_unlock(&dev_priv->dpio_lock);
+
+	return data;
+}
+
+typedef u8 * (*fn_mipi_elem_exec)(struct intel_dsi *intel_dsi, u8 *data);
+const fn_mipi_elem_exec exec_elem[] = {
+	NULL, /* reserved */
+	mipi_exec_send_packet,
+	mipi_exec_delay,
+	mipi_exec_gpio,
+	NULL, /* status read; later */
+};
+
+/*
+ * MIPI Sequence from VBT #53 parsing logic
+ * We have already separated each seqence during bios parsing
+ * Following is generic execution function for any sequence
+ */
+
+static const char *seq_name[] = {
+	"UNDEFINED",
+	"MIPI_SEQ_ASSERT_RESET",
+	"MIPI_SEQ_INIT_OTP",
+	"MIPI_SEQ_DISPLAY_ON",
+	"MIPI_SEQ_DISPLAY_OFF",
+	"MIPI_SEQ_DEASSERT_RESET"
+};
+
+static void generic_exec_sequence(struct intel_dsi *intel_dsi, char *sequence)
+{
+	u8 *data = sequence;
+	fn_mipi_elem_exec mipi_elem_exec;
+	int index;
+
+	if (!sequence)
+		return;
+
+	DRM_DEBUG_DRIVER("Starting MIPI sequence - %s\n", seq_name[*data]);
+
+	/* go to the first element of the sequence */
+	data++;
+
+	/* parse each byte till we reach end of sequence byte - 0x00 */
+	while (1) {
+		index = *data;
+		mipi_elem_exec = exec_elem[index];
+		if (!mipi_elem_exec) {
+			DRM_ERROR("Unsupported MIPI element, skipping sequence execution\n");
+			return;
+		}
+
+		/* goto element payload */
+		data++;
+
+		/* execute the element specifc rotines */
+		data = mipi_elem_exec(intel_dsi, data);
+
+		/*
+		 * After processing the element, data should point to
+		 * next element or end of sequence
+		 * check if have we reached end of sequence
+		 */
+		if (*data == 0x00)
+			break;
+	}
+}
+
+static bool generic_init(struct intel_dsi_device *dsi)
+{
+	struct intel_dsi *intel_dsi = container_of(dsi, struct intel_dsi, dev);
+	struct drm_device *dev = intel_dsi->base.base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct mipi_config *mipi_config = dev_priv->vbt.dsi.config;
+	struct mipi_pps_data *pps = dev_priv->vbt.dsi.pps;
+	struct drm_display_mode *mode = dev_priv->vbt.lfp_lvds_vbt_mode;
+	u32 bits_per_pixel = 24;
+	u32 tlpx_ns, extra_byte_count, bitrate, tlpx_ui;
+	u32 ui_num, ui_den;
+	u32 prepare_cnt, exit_zero_cnt, clk_zero_cnt, trail_cnt;
+	u32 ths_prepare_ns, tclk_trail_ns;
+	u32 tclk_prepare_clkzero, ths_prepare_hszero;
+
+	DRM_DEBUG_KMS("\n");
+
+	intel_dsi->eotp_pkt = mipi_config->eot_pkt_disabled ? 0 : 1;
+	intel_dsi->clock_stop = mipi_config->enable_clk_stop ? 1 : 0;
+	intel_dsi->lane_count = mipi_config->lane_cnt + 1;
+	intel_dsi->pixel_format = mipi_config->videomode_color_format << 7;
+
+	if (intel_dsi->pixel_format == VID_MODE_FORMAT_RGB666)
+		bits_per_pixel = 18;
+	else if (intel_dsi->pixel_format == VID_MODE_FORMAT_RGB565)
+		bits_per_pixel = 16;
+
+	bitrate = (mode->clock * bits_per_pixel) / intel_dsi->lane_count;
+
+	intel_dsi->operation_mode = mipi_config->is_cmd_mode;
+	intel_dsi->video_mode_format = mipi_config->video_transfer_mode;
+	intel_dsi->escape_clk_div = mipi_config->byte_clk_sel;
+	intel_dsi->lp_rx_timeout = mipi_config->lp_rx_timeout;
+	intel_dsi->turn_arnd_val = mipi_config->turn_around_timeout;
+	intel_dsi->rst_timer_val = mipi_config->device_reset_timer;
+	intel_dsi->init_count = mipi_config->master_init_timer;
+	intel_dsi->bw_timer = mipi_config->dbi_bw_timer;
+	intel_dsi->video_frmt_cfg_bits = mipi_config->bta_enabled ? DISABLE_VIDEO_BTA : 0;
+
+	switch (intel_dsi->escape_clk_div) {
+	case 0:
+		tlpx_ns = 50;
+		break;
+	case 1:
+		tlpx_ns = 100;
+		break;
+
+	case 2:
+		tlpx_ns = 200;
+		break;
+	default:
+		tlpx_ns = 50;
+		break;
+	}
+
+	switch (intel_dsi->lane_count) {
+	case 1:
+	case 2:
+		extra_byte_count = 2;
+		break;
+	case 3:
+		extra_byte_count = 4;
+		break;
+	case 4:
+	default:
+		extra_byte_count = 3;
+		break;
+	}
+
+	/*
+	 * ui(s) = 1/f [f in hz]
+	 * ui(ns) = 10^9/f*10^6 [f in Mhz] -> 10^3/f(Mhz)
+	 *
+	 * LP byte clock = TLPX/8ui
+	 *
+	 * Since txddrclkhs_i is 2xUI, the count values programmed in
+	 * DPHY param register are divided by 2
+	 *
+	 */
+
+	/* in Kbps */
+	ui_num = bitrate;
+	ui_den = NS_MHZ_RATIO;
+
+	tclk_prepare_clkzero = mipi_config->tclk_prepare_clkzero;
+	ths_prepare_hszero = mipi_config->ths_prepare_hszero;
+
+	/* B060 */
+	intel_dsi->lp_byte_clk = CEIL_DIV(tlpx_ns * ui_num, 8 * ui_den);
+
+	/* count values in UI = (ns value) * (bitrate / (2 * 10^6)) */
+	/* prepare count */
+	ths_prepare_ns =
+		(mipi_config->ths_prepare >  mipi_config->tclk_prepare) ?
+				mipi_config->ths_prepare :
+				mipi_config->tclk_prepare;
+
+	prepare_cnt = CEIL_DIV(ths_prepare_ns * ui_num,	ui_den * 2);
+
+	/* exit zero count */
+	exit_zero_cnt = CEIL_DIV(
+				(ths_prepare_hszero - ths_prepare_ns) * ui_num,
+				ui_den * 2
+				);
+
+	/*
+	 * Exit zero  is unified val ths_zero and ths_exit
+	 * minimum value for ths_exit = 110ns
+	 * min (exit_zero_cnt * 2) = 110/UI
+	 * exit_zero_cnt = 55/UI
+	 */
+	 if (exit_zero_cnt < (55 * ui_num / ui_den))
+		if ((55 * ui_num) % ui_den)
+			exit_zero_cnt += 1;
+
+	/* clk zero count */
+	clk_zero_cnt = CEIL_DIV(
+			(tclk_prepare_clkzero -	ths_prepare_ns)
+			* ui_num, 2 * ui_den);
+
+	/* trail count */
+	tclk_trail_ns = (mipi_config->tclk_trail > mipi_config->ths_trail) ?
+			mipi_config->tclk_trail : mipi_config->ths_trail;
+	trail_cnt = CEIL_DIV(tclk_trail_ns * ui_num, 2 * ui_den);
+
+	if (prepare_cnt > PREPARE_CNT_MAX ||
+		exit_zero_cnt > EXIT_ZERO_CNT_MAX ||
+		clk_zero_cnt > CLK_ZERO_CNT_MAX ||
+		trail_cnt > TRAIL_CNT_MAX)
+		DRM_DEBUG_DRIVER("Values crossing maximum limits\n");
+
+	if (prepare_cnt > PREPARE_CNT_MAX)
+		prepare_cnt = PREPARE_CNT_MAX;
+
+	if (exit_zero_cnt > EXIT_ZERO_CNT_MAX)
+		exit_zero_cnt = EXIT_ZERO_CNT_MAX;
+
+	if (clk_zero_cnt > CLK_ZERO_CNT_MAX)
+		clk_zero_cnt = CLK_ZERO_CNT_MAX;
+
+	if (trail_cnt > TRAIL_CNT_MAX)
+		trail_cnt = TRAIL_CNT_MAX;
+
+	/* B080 */
+	intel_dsi->dphy_reg = exit_zero_cnt << 24 | trail_cnt << 16 |
+						clk_zero_cnt << 8 | prepare_cnt;
+
+	/*
+	 * LP to HS switch count = 4TLPX + PREP_COUNT * 2 + EXIT_ZERO_COUNT * 2
+	 *					+ 10UI + Extra Byte Count
+	 *
+	 * HS to LP switch count = THS-TRAIL + 2TLPX + Extra Byte Count
+	 * Extra Byte Count is calculated according to number of lanes.
+	 * High Low Switch Count is the Max of LP to HS and
+	 * HS to LP switch count
+	 *
+	 */
+	tlpx_ui = CEIL_DIV(tlpx_ns * ui_num, ui_den);
+
+	/* B044 */
+	intel_dsi->hs_to_lp_count =
+		CEIL_DIV(
+			4 * tlpx_ui + prepare_cnt * 2 +
+			exit_zero_cnt * 2 + 10,
+			8);
+
+	intel_dsi->hs_to_lp_count += extra_byte_count;
+
+	/* B088 */
+	/* LP -> HS for clock lanes
+	 * LP clk sync + LP11 + LP01 + tclk_prepare + tclk_zero +
+	 *						extra byte count
+	 * 2TPLX + 1TLPX + 1 TPLX(in ns) + prepare_cnt * 2 + clk_zero_cnt *
+	 *					2(in UI) + extra byte count
+	 * In byteclks = (4TLPX + prepare_cnt * 2 + clk_zero_cnt *2 (in UI)) /
+	 *					8 + extra byte count
+	 */
+	intel_dsi->clk_lp_to_hs_count =
+		CEIL_DIV(
+			4 * tlpx_ui + prepare_cnt * 2 +
+			clk_zero_cnt * 2,
+			8);
+
+	intel_dsi->clk_lp_to_hs_count += extra_byte_count;
+
+	/* HS->LP for Clock Lanes
+	 * Low Power clock synchronisations + 1Tx byteclk + tclk_trail +
+	 *						Extra byte count
+	 * 2TLPX + 8UI + (trail_count*2)(in UI) + Extra byte count
+	 * In byteclks = (2*TLpx(in UI) + trail_count*2 +8)(in UI)/8 +
+	 *						Extra byte count
+	 */
+	intel_dsi->clk_hs_to_lp_count =
+		CEIL_DIV(2 * tlpx_ui + trail_cnt * 2 + 8,
+			8);
+	intel_dsi->clk_hs_to_lp_count += extra_byte_count;
+
+	DRM_DEBUG_KMS("EOT %s\n", intel_dsi->eotp_pkt ? "ENABLED" : "DISABLED");
+	DRM_DEBUG_KMS("CLOCKSTOP %s\n", intel_dsi->clock_stop ?
+						"ENABLED" : "DISABLED");
+	DRM_DEBUG_KMS("Mode %s\n", intel_dsi->operation_mode ? "COMMAND" : "VIDEO");
+	DRM_DEBUG_KMS("Pixel Format %d\n", intel_dsi->pixel_format);
+	DRM_DEBUG_KMS("TLPX %d\n", intel_dsi->escape_clk_div);
+	DRM_DEBUG_KMS("LP RX Timeout 0x%x\n", intel_dsi->lp_rx_timeout);
+	DRM_DEBUG_KMS("Turnaround Timeout 0x%x\n", intel_dsi->turn_arnd_val);
+	DRM_DEBUG_KMS("Init Count 0x%x\n", intel_dsi->init_count);
+	DRM_DEBUG_KMS("HS to LP Count 0x%x\n", intel_dsi->hs_to_lp_count);
+	DRM_DEBUG_KMS("LP Byte Clock %d\n", intel_dsi->lp_byte_clk);
+	DRM_DEBUG_KMS("DBI BW Timer 0x%x\n", intel_dsi->bw_timer);
+	DRM_DEBUG_KMS("LP to HS Clock Count 0x%x\n", intel_dsi->clk_lp_to_hs_count);
+	DRM_DEBUG_KMS("HS to LP Clock Count 0x%x\n", intel_dsi->clk_hs_to_lp_count);
+	DRM_DEBUG_KMS("BTA %s\n",
+			intel_dsi->video_frmt_cfg_bits & DISABLE_VIDEO_BTA ?
+			"DISABLED" : "ENABLED");
+	DRM_DEBUG_KMS("B060 = 0x%Xx, B080 = 0x%x, B044 = 0x%x, B088 = 0x%x\n",
+			intel_dsi->lp_byte_clk, intel_dsi->dphy_reg, intel_dsi->hs_to_lp_count,
+			(intel_dsi->clk_lp_to_hs_count << LP_HS_SSW_CNT_SHIFT) |
+			(intel_dsi->clk_hs_to_lp_count << HS_LP_PWR_SW_CNT_SHIFT));
+
+	/* delays in VBT are in unit of 100us, so need to convert
+	 * here in ms
+	 * Delay (100us) * 100 /1000 = Delay / 10 (ms) */
+	intel_dsi->backlight_off_delay = pps->bl_disable_delay / 10;
+	intel_dsi->backlight_on_delay = pps->bl_enable_delay / 10;
+	intel_dsi->panel_on_delay = pps->panel_on_delay / 10;
+	intel_dsi->panel_off_delay = pps->panel_off_delay / 10;
+	intel_dsi->panel_pwr_cycle_delay = pps->panel_power_cycle_delay / 10;
+
+	return true;
+}
+
+static int generic_mode_valid(struct intel_dsi_device *dsi,
+		   struct drm_display_mode *mode)
+{
+	return MODE_OK;
+}
+
+static bool generic_mode_fixup(struct intel_dsi_device *dsi,
+		    const struct drm_display_mode *mode,
+		    struct drm_display_mode *adjusted_mode) {
+	return true;
+}
+
+static void generic_panel_reset(struct intel_dsi_device *dsi)
+{
+	struct intel_dsi *intel_dsi = container_of(dsi, struct intel_dsi, dev);
+	struct drm_device *dev = intel_dsi->base.base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	char *sequence = dev_priv->vbt.dsi.sequence[MIPI_SEQ_ASSERT_RESET];
+
+	generic_exec_sequence(intel_dsi, sequence);
+}
+
+static void generic_disable_panel_power(struct intel_dsi_device *dsi)
+{
+	struct intel_dsi *intel_dsi = container_of(dsi, struct intel_dsi, dev);
+	struct drm_device *dev = intel_dsi->base.base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	char *sequence = dev_priv->vbt.dsi.sequence[MIPI_SEQ_DEASSERT_RESET];
+
+	generic_exec_sequence(intel_dsi, sequence);
+}
+
+static void generic_send_otp_cmds(struct intel_dsi_device *dsi)
+{
+	struct intel_dsi *intel_dsi = container_of(dsi, struct intel_dsi, dev);
+	struct drm_device *dev = intel_dsi->base.base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	char *sequence = dev_priv->vbt.dsi.sequence[MIPI_SEQ_INIT_OTP];
+
+	generic_exec_sequence(intel_dsi, sequence);
+}
+
+static void generic_enable(struct intel_dsi_device *dsi)
+{
+	struct intel_dsi *intel_dsi = container_of(dsi, struct intel_dsi, dev);
+	struct drm_device *dev = intel_dsi->base.base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	char *sequence = dev_priv->vbt.dsi.sequence[MIPI_SEQ_DISPLAY_ON];
+
+	generic_exec_sequence(intel_dsi, sequence);
+}
+
+static void generic_disable(struct intel_dsi_device *dsi)
+{
+	struct intel_dsi *intel_dsi = container_of(dsi, struct intel_dsi, dev);
+	struct drm_device *dev = intel_dsi->base.base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	char *sequence = dev_priv->vbt.dsi.sequence[MIPI_SEQ_DISPLAY_OFF];
+
+	generic_exec_sequence(intel_dsi, sequence);
+}
+
+static enum drm_connector_status generic_detect(struct intel_dsi_device *dsi)
+{
+	return connector_status_connected;
+}
+
+static bool generic_get_hw_state(struct intel_dsi_device *dev)
+{
+	return true;
+}
+
+static struct drm_display_mode *generic_get_modes(struct intel_dsi_device *dsi)
+{
+	struct intel_dsi *intel_dsi = container_of(dsi, struct intel_dsi, dev);
+	struct drm_device *dev = intel_dsi->base.base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	dev_priv->vbt.lfp_lvds_vbt_mode->type |= DRM_MODE_TYPE_PREFERRED;
+	return dev_priv->vbt.lfp_lvds_vbt_mode;
+}
+
+static void generic_destroy(struct intel_dsi_device *dsi) { }
+
+/* Callbacks. We might not need them all. */
+struct intel_dsi_dev_ops vbt_generic_dsi_display_ops = {
+	.init = generic_init,
+	.mode_valid = generic_mode_valid,
+	.mode_fixup = generic_mode_fixup,
+	.panel_reset = generic_panel_reset,
+	.disable_panel_power = generic_disable_panel_power,
+	.send_otp_cmds = generic_send_otp_cmds,
+	.enable = generic_enable,
+	.disable = generic_disable,
+	.detect = generic_detect,
+	.get_hw_state = generic_get_hw_state,
+	.get_modes = generic_get_modes,
+	.destroy = generic_destroy,
+};
diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
index 0d4dd54..7f1ddaa 100644
--- a/drivers/gpu/drm/i915/intel_dsi.c
+++ b/drivers/gpu/drm/i915/intel_dsi.c
@@ -35,6 +35,11 @@
 
 /* the sub-encoders aka panel drivers */
 static const struct intel_dsi_device intel_dsi_devices[] = {
+	{
+		.panel_id = MIPI_DSI_GENERIC_PANEL_ID,
+		.name = "vbt-generic-dsi-vid-mode-display",
+		.dev_ops = &vbt_generic_dsi_display_ops,
+	},
 };
 
 static void band_gap_reset(struct drm_i915_private *dev_priv)
diff --git a/drivers/gpu/drm/i915/intel_dsi.h b/drivers/gpu/drm/i915/intel_dsi.h
index e3f4e91..31db33d 100644
--- a/drivers/gpu/drm/i915/intel_dsi.h
+++ b/drivers/gpu/drm/i915/intel_dsi.h
@@ -133,4 +133,6 @@ static inline struct intel_dsi *enc_to_intel_dsi(struct drm_encoder *encoder)
 extern void vlv_enable_dsi_pll(struct intel_encoder *encoder);
 extern void vlv_disable_dsi_pll(struct intel_encoder *encoder);
 
+extern struct intel_dsi_dev_ops vbt_generic_dsi_display_ops;
+
 #endif /* _INTEL_DSI_H */
-- 
1.8.3.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 0/4] Generic MIPI Panel driver

[Kumar, Shobhit]

Hi Jani, Damien,

On 4/14/2014 11:18 AM, Shobhit Kumar wrote:
> Hi,
> This series enabled generic MIPI panel driver support, the ground for which
> has been built up from the patches -
>
> http://lists.freedesktop.org/archives/intel-gfx/2014-February/040764.html
> http://lists.freedesktop.org/archives/intel-gfx/2014-April/043646.html
>
> With a version of these patches Asus T100A was found to be working by Arjan
> who has a device. I do not have the device to test directly. Will be getting
> one and verifying on that. But ideally should work. I have verified these patches
> on couple of internal panels.
>
> Also some minor changes in intel_dsi.c to support the generic panel driver
> are done.

Any comments on this patch-set. There is still two issues which I found 
when we enable panel driver. PIPE is not going off during first disable 
sequence and get_pipe_config is not yet implemented for MIPI. I am 
working on both of them right now.

Jesse,
The pipe not being turned off is MIPI specific issue and not a general 
issue which I wrongly hinted to you in a call.

Regards
Shobhit

Re: [PATCH 1/4] drm/i915: Correct MIPI operation mode as per expected values from VBT

[Damien Lespiau]

On Mon, Apr 14, 2014 at 11:18:24AM +0530, Shobhit Kumar wrote:
> In VBT fields operation mode is 0 for Video mode and 1 for command mode.
> This field will be directly used as is in generic panel driver. So
> adjust accordingly.
> 
> Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>

Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>

-- 
Damien

Re: [PATCH 2/4] drm/i915: MIPI init count programming as generic parameter

[Damien Lespiau]

On Mon, Apr 14, 2014 at 11:18:25AM +0530, Shobhit Kumar wrote:
> Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>

Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>

-- 
Damien

Re: [PATCH 3/4] drm/i915: MIPI PPS delays added

[Damien Lespiau]

On Mon, Apr 14, 2014 at 11:18:26AM +0530, Shobhit Kumar wrote:
> Added as generic parameters which will be initialized in the panel
> driver and are specific to panels.
> 
> Backlight delays have also kept as placeholders and will be used used
> once we have MIPI backlight enabling support
> 
> Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>

Just a small note on patch ordering, You're using values that no code
initializes yet, doesn't matter at all this time.

Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>

-- 
Damien

Re: [PATCH 4/4] drm/i915: Add support for Generic MIPI panel driver

[Damien Lespiau]

On Mon, Apr 14, 2014 at 11:18:27AM +0530, Shobhit Kumar wrote:
> This driver makes use of the generic panel information from the VBT.
> Panel information is classified into two - panel configuration and panel
> power sequence which is unique to each panel. The generic driver uses the
> panel configuration and sequence parsed from VBT block #52 and #53
> 
> v2: Address review comments by Jani
>     - Move all of the things in driver c file from header
>     - Make all functions static
>     - Make use of video/mipi_display.c instead of redefining
>     - Null checks during sequence execution
> 
> Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>

I've done a first past on this. Overall looks reasonable. I'm missing
some documentation to double check the various LP->HS, HS->LP count and
other magic around the clocks (send you a mail about it) before I can
add my r-b tag.

I've added a few tiny comments as well along the road.

-- 
Damien

> ---
>  drivers/gpu/drm/i915/Makefile              |   1 +
>  drivers/gpu/drm/i915/dsi_mod_vbt_generic.c | 598 +++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_dsi.c           |   5 +
>  drivers/gpu/drm/i915/intel_dsi.h           |   2 +
>  4 files changed, 606 insertions(+)
>  create mode 100644 drivers/gpu/drm/i915/dsi_mod_vbt_generic.c
> 
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index b1445b7..756a7a4 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -55,6 +55,7 @@ i915-y += dvo_ch7017.o \
>  	  intel_dsi_cmd.o \
>  	  intel_dsi.o \
>  	  intel_dsi_pll.o \
> +	  dsi_mod_vbt_generic.o	\

Should probaly start with intel_dsi_ to be consistent with the other
files. intel_dsi_vbt.c ?

[...]

> +/* This macro divides two integers and rounds fractional values up
> + * to the nearest integer value. */
> +#define CEIL_DIV(X, Y) (((X)%(Y)) ? ((X)/(Y)+1) : ((X)/(Y)))

DIV_ROUND_UP()?

> +#define GPI0_NC_0_HV_DDI0_HPD           0x4130
> +#define GPIO_NC_0_HV_DDI0_PAD           0x4138
> +#define GPIO_NC_1_HV_DDI0_DDC_SDA       0x4120
> +#define GPIO_NC_1_HV_DDI0_DDC_SDA_PAD   0x4128
> +#define GPIO_NC_2_HV_DDI0_DDC_SCL       0x4110
> +#define GPIO_NC_2_HV_DDI0_DDC_SCL_PAD   0x4118
> +#define GPIO_NC_3_PANEL0_VDDEN          0x4140
> +#define GPIO_NC_3_PANEL0_VDDEN_PAD      0x4148
> +#define GPIO_NC_4_PANEL0_BLKEN          0x4150
> +#define GPIO_NC_4_PANEL0_BLKEN_PAD      0x4158
> +#define GPIO_NC_5_PANEL0_BLKCTL         0x4160
> +#define GPIO_NC_5_PANEL0_BLKCTL_PAD     0x4168
> +#define GPIO_NC_6_PCONF0                0x4180
> +#define GPIO_NC_6_PAD                   0x4188
> +#define GPIO_NC_7_PCONF0                0x4190
> +#define GPIO_NC_7_PAD                   0x4198
> +#define GPIO_NC_8_PCONF0                0x4170
> +#define GPIO_NC_8_PAD                   0x4178
> +#define GPIO_NC_9_PCONF0                0x4100
> +#define GPIO_NC_9_PAD                   0x4108
> +#define GPIO_NC_10_PCONF0               0x40E0
> +#define GPIO_NC_10_PAD                  0x40E8
> +#define GPIO_NC_11_PCONF0               0x40F0
> +#define GPIO_NC_11_PAD                  0x40F8
> +
> +struct gpio_table {
> +	u16 function_reg;
> +	u16 pad_reg;
> +	u8 init;
> +};
> +
> +static struct gpio_table gtable[] = {

const

> +	{ GPI0_NC_0_HV_DDI0_HPD, GPIO_NC_0_HV_DDI0_PAD, 0 },
> +	{ GPIO_NC_1_HV_DDI0_DDC_SDA, GPIO_NC_1_HV_DDI0_DDC_SDA_PAD, 0 },
> +	{ GPIO_NC_2_HV_DDI0_DDC_SCL, GPIO_NC_2_HV_DDI0_DDC_SCL_PAD, 0 },
> +	{ GPIO_NC_3_PANEL0_VDDEN, GPIO_NC_3_PANEL0_VDDEN_PAD, 0 },
> +	{ GPIO_NC_4_PANEL0_BLKEN, GPIO_NC_4_PANEL0_BLKEN_PAD, 0 },
> +	{ GPIO_NC_5_PANEL0_BLKCTL, GPIO_NC_5_PANEL0_BLKCTL_PAD, 0 },
> +	{ GPIO_NC_6_PCONF0, GPIO_NC_6_PAD, 0 },
> +	{ GPIO_NC_7_PCONF0, GPIO_NC_7_PAD, 0 },
> +	{ GPIO_NC_8_PCONF0, GPIO_NC_8_PAD, 0 },
> +	{ GPIO_NC_9_PCONF0, GPIO_NC_9_PAD, 0 },
> +	{ GPIO_NC_10_PCONF0, GPIO_NC_10_PAD, 0},
> +	{ GPIO_NC_11_PCONF0, GPIO_NC_11_PAD, 0}
> +};
> +
> +static u8 *mipi_exec_send_packet(struct intel_dsi *intel_dsi, u8 *data)
> +{
> +	u8 type, byte, mode, vc, port;
> +	u16 len;
> +
> +	byte = *data++;
> +	mode = (byte >> MIPI_TRANSFER_MODE_SHIFT) & 0x1;
> +	vc = (byte >> MIPI_VIRTUAL_CHANNEL_SHIFT) & 0x3;
> +	port = (byte >> MIPI_PORT_SHIFT) & 0x3;
> +
> +	/* LP or HS mode */
> +	intel_dsi->hs = mode;
> +
> +	/* get packet type and increment the pointer */
> +	type = *data++;
> +
> +	len = *((u16 *) data);
> +	data += 2;
> +
> +	switch (type) {
> +	case MIPI_DSI_GENERIC_SHORT_WRITE_0_PARAM:
> +		dsi_vc_generic_write_0(intel_dsi, vc);
> +		break;
> +	case MIPI_DSI_GENERIC_SHORT_WRITE_1_PARAM:
> +		dsi_vc_generic_write_1(intel_dsi, vc, *data);
> +		break;
> +	case MIPI_DSI_GENERIC_SHORT_WRITE_2_PARAM:
> +		dsi_vc_generic_write_2(intel_dsi, vc, *data, *(data + 1));
> +		break;
> +	case MIPI_DSI_GENERIC_READ_REQUEST_0_PARAM:
> +	case MIPI_DSI_GENERIC_READ_REQUEST_1_PARAM:
> +	case MIPI_DSI_GENERIC_READ_REQUEST_2_PARAM:
> +		DRM_DEBUG_DRIVER("Generic Read not yet implemented or used\n");
> +		break;
> +	case MIPI_DSI_GENERIC_LONG_WRITE:
> +		dsi_vc_generic_write(intel_dsi, vc, data, len);
> +		break;
> +	case MIPI_DSI_DCS_SHORT_WRITE:
> +		dsi_vc_dcs_write_0(intel_dsi, vc, *data);
> +		break;
> +	case MIPI_DSI_DCS_SHORT_WRITE_PARAM:
> +		dsi_vc_dcs_write_1(intel_dsi, vc, *data, *(data + 1));
> +		break;
> +	case MIPI_DSI_DCS_READ:
> +		DRM_DEBUG_DRIVER("DCS Read not yet implemented or used\n");
> +		break;
> +	case MIPI_DSI_DCS_LONG_WRITE:
> +		dsi_vc_dcs_write(intel_dsi, vc, data, len);
> +		break;
> +	};
> +
> +	data += len;
> +
> +	return data;
> +}
> +
> +static u8 *mipi_exec_delay(struct intel_dsi *intel_dsi, u8 *data)
> +{
> +	u32 delay = *((u32 *) data);
> +
> +	DRM_DEBUG_DRIVER("MIPI: executing delay element\n");

A general observation for your tracing of the sequences being executed.
Either it's important to trace the sequence we're executing and we may
want to add more information (for instance here the amount we're waiting
for, what we're doing with the GPIOs, ...) or it's not important and we
can skip the messages altogether?

> +	usleep_range(delay, delay + 10);
> +	data += 4;
> +
> +	return data;
> +}
> +
> +static u8 *mipi_exec_gpio(struct intel_dsi *intel_dsi, u8 *data)
> +{
> +	u8 gpio, action;
> +	u16 function, pad;
> +	u32 val;
> +	struct drm_device *dev = intel_dsi->base.base.dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +	DRM_DEBUG_DRIVER("MIPI: executing gpio element\n");
> +	gpio = *data++;
> +
> +	/* pull up/down */
> +	action = *data++;
> +
> +	function = gtable[gpio].function_reg;
> +	pad = gtable[gpio].pad_reg;
> +
> +	mutex_lock(&dev_priv->dpio_lock);
> +	if (!gtable[gpio].init) {
> +		/* program the function */
> +		vlv_gpio_nc_write(dev_priv, function, 0x2000CC00);

Any chance we can document that constant? (with defines of comment)

> +		gtable[gpio].init = 1;
> +	}
> +
> +	val = 0x4 | action;
> +
> +	/* pull up/down */
> +	vlv_gpio_nc_write(dev_priv, pad, val);
> +	mutex_unlock(&dev_priv->dpio_lock);
> +
> +	return data;
> +}
> +
> +typedef u8 * (*fn_mipi_elem_exec)(struct intel_dsi *intel_dsi, u8 *data);
> +const fn_mipi_elem_exec exec_elem[] = {

static

> +	NULL, /* reserved */
> +	mipi_exec_send_packet,
> +	mipi_exec_delay,
> +	mipi_exec_gpio,
> +	NULL, /* status read; later */
> +};
> +
> +/*
> + * MIPI Sequence from VBT #53 parsing logic
> + * We have already separated each seqence during bios parsing
> + * Following is generic execution function for any sequence
> + */
> +
> +static const char *seq_name[] = {

static const char * const

> +	"UNDEFINED",
> +	"MIPI_SEQ_ASSERT_RESET",
> +	"MIPI_SEQ_INIT_OTP",
> +	"MIPI_SEQ_DISPLAY_ON",
> +	"MIPI_SEQ_DISPLAY_OFF",
> +	"MIPI_SEQ_DEASSERT_RESET"
> +};
> +
> +static void generic_exec_sequence(struct intel_dsi *intel_dsi, char *sequence)
> +{
> +	u8 *data = sequence;
> +	fn_mipi_elem_exec mipi_elem_exec;
> +	int index;
> +
> +	if (!sequence)
> +		return;
> +
> +	DRM_DEBUG_DRIVER("Starting MIPI sequence - %s\n", seq_name[*data]);
> +
> +	/* go to the first element of the sequence */
> +	data++;
> +
> +	/* parse each byte till we reach end of sequence byte - 0x00 */
> +	while (1) {
> +		index = *data;
> +		mipi_elem_exec = exec_elem[index];
> +		if (!mipi_elem_exec) {
> +			DRM_ERROR("Unsupported MIPI element, skipping sequence execution\n");
> +			return;
> +		}
> +
> +		/* goto element payload */
> +		data++;
> +
> +		/* execute the element specifc rotines */

specific routines

> +		data = mipi_elem_exec(intel_dsi, data);
> +
> +		/*
> +		 * After processing the element, data should point to
> +		 * next element or end of sequence
> +		 * check if have we reached end of sequence
> +		 */
> +		if (*data == 0x00)
> +			break;
> +	}
> +}
> +
> +static bool generic_init(struct intel_dsi_device *dsi)
> +{
> +	struct intel_dsi *intel_dsi = container_of(dsi, struct intel_dsi, dev);
> +	struct drm_device *dev = intel_dsi->base.base.dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct mipi_config *mipi_config = dev_priv->vbt.dsi.config;
> +	struct mipi_pps_data *pps = dev_priv->vbt.dsi.pps;
> +	struct drm_display_mode *mode = dev_priv->vbt.lfp_lvds_vbt_mode;
> +	u32 bits_per_pixel = 24;
> +	u32 tlpx_ns, extra_byte_count, bitrate, tlpx_ui;
> +	u32 ui_num, ui_den;
> +	u32 prepare_cnt, exit_zero_cnt, clk_zero_cnt, trail_cnt;
> +	u32 ths_prepare_ns, tclk_trail_ns;
> +	u32 tclk_prepare_clkzero, ths_prepare_hszero;
> +
> +	DRM_DEBUG_KMS("\n");
> +
> +	intel_dsi->eotp_pkt = mipi_config->eot_pkt_disabled ? 0 : 1;
> +	intel_dsi->clock_stop = mipi_config->enable_clk_stop ? 1 : 0;
> +	intel_dsi->lane_count = mipi_config->lane_cnt + 1;
> +	intel_dsi->pixel_format = mipi_config->videomode_color_format << 7;
> +
> +	if (intel_dsi->pixel_format == VID_MODE_FORMAT_RGB666)
> +		bits_per_pixel = 18;
> +	else if (intel_dsi->pixel_format == VID_MODE_FORMAT_RGB565)
> +		bits_per_pixel = 16;
> +
> +	bitrate = (mode->clock * bits_per_pixel) / intel_dsi->lane_count;
> +
> +	intel_dsi->operation_mode = mipi_config->is_cmd_mode;
> +	intel_dsi->video_mode_format = mipi_config->video_transfer_mode;
> +	intel_dsi->escape_clk_div = mipi_config->byte_clk_sel;
> +	intel_dsi->lp_rx_timeout = mipi_config->lp_rx_timeout;
> +	intel_dsi->turn_arnd_val = mipi_config->turn_around_timeout;
> +	intel_dsi->rst_timer_val = mipi_config->device_reset_timer;
> +	intel_dsi->init_count = mipi_config->master_init_timer;
> +	intel_dsi->bw_timer = mipi_config->dbi_bw_timer;
> +	intel_dsi->video_frmt_cfg_bits = mipi_config->bta_enabled ? DISABLE_VIDEO_BTA : 0;
> +
> +	switch (intel_dsi->escape_clk_div) {
> +	case 0:
> +		tlpx_ns = 50;
> +		break;
> +	case 1:
> +		tlpx_ns = 100;
> +		break;
> +
> +	case 2:
> +		tlpx_ns = 200;
> +		break;
> +	default:
> +		tlpx_ns = 50;
> +		break;
> +	}
> +
> +	switch (intel_dsi->lane_count) {
> +	case 1:
> +	case 2:
> +		extra_byte_count = 2;
> +		break;
> +	case 3:
> +		extra_byte_count = 4;
> +		break;
> +	case 4:
> +	default:
> +		extra_byte_count = 3;
> +		break;
> +	}
> +
> +	/*
> +	 * ui(s) = 1/f [f in hz]
> +	 * ui(ns) = 10^9/f*10^6 [f in Mhz] -> 10^3/f(Mhz)
> +	 *
> +	 * LP byte clock = TLPX/8ui
> +	 *
> +	 * Since txddrclkhs_i is 2xUI, the count values programmed in
> +	 * DPHY param register are divided by 2
> +	 *
> +	 */
> +
> +	/* in Kbps */
> +	ui_num = bitrate;
> +	ui_den = NS_MHZ_RATIO;
> +
> +	tclk_prepare_clkzero = mipi_config->tclk_prepare_clkzero;
> +	ths_prepare_hszero = mipi_config->ths_prepare_hszero;
> +
> +	/* B060 */
> +	intel_dsi->lp_byte_clk = CEIL_DIV(tlpx_ns * ui_num, 8 * ui_den);
> +
> +	/* count values in UI = (ns value) * (bitrate / (2 * 10^6)) */
> +	/* prepare count */
> +	ths_prepare_ns =
> +		(mipi_config->ths_prepare >  mipi_config->tclk_prepare) ?
> +				mipi_config->ths_prepare :
> +				mipi_config->tclk_prepare;
> +
> +	prepare_cnt = CEIL_DIV(ths_prepare_ns * ui_num,	ui_den * 2);
> +
> +	/* exit zero count */
> +	exit_zero_cnt = CEIL_DIV(
> +				(ths_prepare_hszero - ths_prepare_ns) * ui_num,
> +				ui_den * 2
> +				);
> +
> +	/*
> +	 * Exit zero  is unified val ths_zero and ths_exit
> +	 * minimum value for ths_exit = 110ns
> +	 * min (exit_zero_cnt * 2) = 110/UI
> +	 * exit_zero_cnt = 55/UI
> +	 */
> +	 if (exit_zero_cnt < (55 * ui_num / ui_den))
> +		if ((55 * ui_num) % ui_den)
> +			exit_zero_cnt += 1;
> +
> +	/* clk zero count */
> +	clk_zero_cnt = CEIL_DIV(
> +			(tclk_prepare_clkzero -	ths_prepare_ns)
> +			* ui_num, 2 * ui_den);
> +
> +	/* trail count */
> +	tclk_trail_ns = (mipi_config->tclk_trail > mipi_config->ths_trail) ?
> +			mipi_config->tclk_trail : mipi_config->ths_trail;
> +	trail_cnt = CEIL_DIV(tclk_trail_ns * ui_num, 2 * ui_den);
> +
> +	if (prepare_cnt > PREPARE_CNT_MAX ||
> +		exit_zero_cnt > EXIT_ZERO_CNT_MAX ||
> +		clk_zero_cnt > CLK_ZERO_CNT_MAX ||
> +		trail_cnt > TRAIL_CNT_MAX)
> +		DRM_DEBUG_DRIVER("Values crossing maximum limits\n");
> +
> +	if (prepare_cnt > PREPARE_CNT_MAX)
> +		prepare_cnt = PREPARE_CNT_MAX;
> +
> +	if (exit_zero_cnt > EXIT_ZERO_CNT_MAX)
> +		exit_zero_cnt = EXIT_ZERO_CNT_MAX;
> +
> +	if (clk_zero_cnt > CLK_ZERO_CNT_MAX)
> +		clk_zero_cnt = CLK_ZERO_CNT_MAX;
> +
> +	if (trail_cnt > TRAIL_CNT_MAX)
> +		trail_cnt = TRAIL_CNT_MAX;
> +
> +	/* B080 */
> +	intel_dsi->dphy_reg = exit_zero_cnt << 24 | trail_cnt << 16 |
> +						clk_zero_cnt << 8 | prepare_cnt;
> +
> +	/*
> +	 * LP to HS switch count = 4TLPX + PREP_COUNT * 2 + EXIT_ZERO_COUNT * 2
> +	 *					+ 10UI + Extra Byte Count
> +	 *
> +	 * HS to LP switch count = THS-TRAIL + 2TLPX + Extra Byte Count
> +	 * Extra Byte Count is calculated according to number of lanes.
> +	 * High Low Switch Count is the Max of LP to HS and
> +	 * HS to LP switch count
> +	 *
> +	 */
> +	tlpx_ui = CEIL_DIV(tlpx_ns * ui_num, ui_den);
> +
> +	/* B044 */
> +	intel_dsi->hs_to_lp_count =
> +		CEIL_DIV(
> +			4 * tlpx_ui + prepare_cnt * 2 +
> +			exit_zero_cnt * 2 + 10,
> +			8);

The comment above says the reverse of what the code does (lp->hs Vs
hs->lp).

> +
> +	intel_dsi->hs_to_lp_count += extra_byte_count;
> +
> +	/* B088 */
> +	/* LP -> HS for clock lanes
> +	 * LP clk sync + LP11 + LP01 + tclk_prepare + tclk_zero +
> +	 *						extra byte count
> +	 * 2TPLX + 1TLPX + 1 TPLX(in ns) + prepare_cnt * 2 + clk_zero_cnt *
> +	 *					2(in UI) + extra byte count
> +	 * In byteclks = (4TLPX + prepare_cnt * 2 + clk_zero_cnt *2 (in UI)) /
> +	 *					8 + extra byte count
> +	 */
> +	intel_dsi->clk_lp_to_hs_count =
> +		CEIL_DIV(
> +			4 * tlpx_ui + prepare_cnt * 2 +
> +			clk_zero_cnt * 2,
> +			8);
> +
> +	intel_dsi->clk_lp_to_hs_count += extra_byte_count;
> +
> +	/* HS->LP for Clock Lanes
> +	 * Low Power clock synchronisations + 1Tx byteclk + tclk_trail +
> +	 *						Extra byte count
> +	 * 2TLPX + 8UI + (trail_count*2)(in UI) + Extra byte count
> +	 * In byteclks = (2*TLpx(in UI) + trail_count*2 +8)(in UI)/8 +
> +	 *						Extra byte count
> +	 */
> +	intel_dsi->clk_hs_to_lp_count =
> +		CEIL_DIV(2 * tlpx_ui + trail_cnt * 2 + 8,
> +			8);
> +	intel_dsi->clk_hs_to_lp_count += extra_byte_count;
> +
> +	DRM_DEBUG_KMS("EOT %s\n", intel_dsi->eotp_pkt ? "ENABLED" : "DISABLED");

We don't usually use capital letters for debug messages.

> +	DRM_DEBUG_KMS("CLOCKSTOP %s\n", intel_dsi->clock_stop ?
> +						"ENABLED" : "DISABLED");
> +	DRM_DEBUG_KMS("Mode %s\n", intel_dsi->operation_mode ? "COMMAND" : "VIDEO");
> +	DRM_DEBUG_KMS("Pixel Format %d\n", intel_dsi->pixel_format);
> +	DRM_DEBUG_KMS("TLPX %d\n", intel_dsi->escape_clk_div);
> +	DRM_DEBUG_KMS("LP RX Timeout 0x%x\n", intel_dsi->lp_rx_timeout);
> +	DRM_DEBUG_KMS("Turnaround Timeout 0x%x\n", intel_dsi->turn_arnd_val);
> +	DRM_DEBUG_KMS("Init Count 0x%x\n", intel_dsi->init_count);
> +	DRM_DEBUG_KMS("HS to LP Count 0x%x\n", intel_dsi->hs_to_lp_count);
> +	DRM_DEBUG_KMS("LP Byte Clock %d\n", intel_dsi->lp_byte_clk);
> +	DRM_DEBUG_KMS("DBI BW Timer 0x%x\n", intel_dsi->bw_timer);
> +	DRM_DEBUG_KMS("LP to HS Clock Count 0x%x\n", intel_dsi->clk_lp_to_hs_count);
> +	DRM_DEBUG_KMS("HS to LP Clock Count 0x%x\n", intel_dsi->clk_hs_to_lp_count);
> +	DRM_DEBUG_KMS("BTA %s\n",
> +			intel_dsi->video_frmt_cfg_bits & DISABLE_VIDEO_BTA ?
> +			"DISABLED" : "ENABLED");
> +	DRM_DEBUG_KMS("B060 = 0x%Xx, B080 = 0x%x, B044 = 0x%x, B088 = 0x%x\n",
> +			intel_dsi->lp_byte_clk, intel_dsi->dphy_reg, intel_dsi->hs_to_lp_count,
> +			(intel_dsi->clk_lp_to_hs_count << LP_HS_SSW_CNT_SHIFT) |
> +			(intel_dsi->clk_hs_to_lp_count << HS_LP_PWR_SW_CNT_SHIFT));

Can we have symbolic names instead of register offsets? if needed at
all? also lp_byte_clk and hs_to_lp_count are already printed out a few
lines before.

> +
> +	/* delays in VBT are in unit of 100us, so need to convert
> +	 * here in ms
> +	 * Delay (100us) * 100 /1000 = Delay / 10 (ms) */
> +	intel_dsi->backlight_off_delay = pps->bl_disable_delay / 10;
> +	intel_dsi->backlight_on_delay = pps->bl_enable_delay / 10;
> +	intel_dsi->panel_on_delay = pps->panel_on_delay / 10;
> +	intel_dsi->panel_off_delay = pps->panel_off_delay / 10;
> +	intel_dsi->panel_pwr_cycle_delay = pps->panel_power_cycle_delay / 10;
> +
> +	return true;
> +}
> +
> +static int generic_mode_valid(struct intel_dsi_device *dsi,
> +		   struct drm_display_mode *mode)
> +{
> +	return MODE_OK;
> +}
> +
> +static bool generic_mode_fixup(struct intel_dsi_device *dsi,
> +		    const struct drm_display_mode *mode,
> +		    struct drm_display_mode *adjusted_mode) {
> +	return true;
> +}
> +
> +static void generic_panel_reset(struct intel_dsi_device *dsi)
> +{
> +	struct intel_dsi *intel_dsi = container_of(dsi, struct intel_dsi, dev);
> +	struct drm_device *dev = intel_dsi->base.base.dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +	char *sequence = dev_priv->vbt.dsi.sequence[MIPI_SEQ_ASSERT_RESET];
> +
> +	generic_exec_sequence(intel_dsi, sequence);
> +}
> +
> +static void generic_disable_panel_power(struct intel_dsi_device *dsi)
> +{
> +	struct intel_dsi *intel_dsi = container_of(dsi, struct intel_dsi, dev);
> +	struct drm_device *dev = intel_dsi->base.base.dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +	char *sequence = dev_priv->vbt.dsi.sequence[MIPI_SEQ_DEASSERT_RESET];
> +
> +	generic_exec_sequence(intel_dsi, sequence);
> +}
> +
> +static void generic_send_otp_cmds(struct intel_dsi_device *dsi)
> +{
> +	struct intel_dsi *intel_dsi = container_of(dsi, struct intel_dsi, dev);
> +	struct drm_device *dev = intel_dsi->base.base.dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +	char *sequence = dev_priv->vbt.dsi.sequence[MIPI_SEQ_INIT_OTP];
> +
> +	generic_exec_sequence(intel_dsi, sequence);
> +}
> +
> +static void generic_enable(struct intel_dsi_device *dsi)
> +{
> +	struct intel_dsi *intel_dsi = container_of(dsi, struct intel_dsi, dev);
> +	struct drm_device *dev = intel_dsi->base.base.dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +	char *sequence = dev_priv->vbt.dsi.sequence[MIPI_SEQ_DISPLAY_ON];
> +
> +	generic_exec_sequence(intel_dsi, sequence);
> +}
> +
> +static void generic_disable(struct intel_dsi_device *dsi)
> +{
> +	struct intel_dsi *intel_dsi = container_of(dsi, struct intel_dsi, dev);
> +	struct drm_device *dev = intel_dsi->base.base.dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +	char *sequence = dev_priv->vbt.dsi.sequence[MIPI_SEQ_DISPLAY_OFF];
> +
> +	generic_exec_sequence(intel_dsi, sequence);
> +}
> +
> +static enum drm_connector_status generic_detect(struct intel_dsi_device *dsi)
> +{
> +	return connector_status_connected;
> +}
> +
> +static bool generic_get_hw_state(struct intel_dsi_device *dev)
> +{
> +	return true;
> +}

We don't seem to use struct intel_dsi_dev_ops's get_hw_state() anywhere
btw.

> +
> +static struct drm_display_mode *generic_get_modes(struct intel_dsi_device *dsi)
> +{
> +	struct intel_dsi *intel_dsi = container_of(dsi, struct intel_dsi, dev);
> +	struct drm_device *dev = intel_dsi->base.base.dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +	dev_priv->vbt.lfp_lvds_vbt_mode->type |= DRM_MODE_TYPE_PREFERRED;
> +	return dev_priv->vbt.lfp_lvds_vbt_mode;
> +}
> +
> +static void generic_destroy(struct intel_dsi_device *dsi) { }
> +
> +/* Callbacks. We might not need them all. */
> +struct intel_dsi_dev_ops vbt_generic_dsi_display_ops = {
> +	.init = generic_init,
> +	.mode_valid = generic_mode_valid,
> +	.mode_fixup = generic_mode_fixup,
> +	.panel_reset = generic_panel_reset,
> +	.disable_panel_power = generic_disable_panel_power,
> +	.send_otp_cmds = generic_send_otp_cmds,
> +	.enable = generic_enable,
> +	.disable = generic_disable,
> +	.detect = generic_detect,
> +	.get_hw_state = generic_get_hw_state,
> +	.get_modes = generic_get_modes,
> +	.destroy = generic_destroy,
> +};
> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
> index 0d4dd54..7f1ddaa 100644
> --- a/drivers/gpu/drm/i915/intel_dsi.c
> +++ b/drivers/gpu/drm/i915/intel_dsi.c
> @@ -35,6 +35,11 @@
>  
>  /* the sub-encoders aka panel drivers */
>  static const struct intel_dsi_device intel_dsi_devices[] = {
> +	{
> +		.panel_id = MIPI_DSI_GENERIC_PANEL_ID,
> +		.name = "vbt-generic-dsi-vid-mode-display",
> +		.dev_ops = &vbt_generic_dsi_display_ops,
> +	},
>  };
>  
>  static void band_gap_reset(struct drm_i915_private *dev_priv)
> diff --git a/drivers/gpu/drm/i915/intel_dsi.h b/drivers/gpu/drm/i915/intel_dsi.h
> index e3f4e91..31db33d 100644
> --- a/drivers/gpu/drm/i915/intel_dsi.h
> +++ b/drivers/gpu/drm/i915/intel_dsi.h
> @@ -133,4 +133,6 @@ static inline struct intel_dsi *enc_to_intel_dsi(struct drm_encoder *encoder)
>  extern void vlv_enable_dsi_pll(struct intel_encoder *encoder);
>  extern void vlv_disable_dsi_pll(struct intel_encoder *encoder);
>  
> +extern struct intel_dsi_dev_ops vbt_generic_dsi_display_ops;
> +
>  #endif /* _INTEL_DSI_H */
> -- 
> 1.8.3.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/4] drm/i915: MIPI PPS delays added

[Daniel Vetter]

On Thu, May 15, 2014 at 04:06:56PM +0100, Damien Lespiau wrote:
> On Mon, Apr 14, 2014 at 11:18:26AM +0530, Shobhit Kumar wrote:
> > Added as generic parameters which will be initialized in the panel
> > driver and are specific to panels.
> > 
> > Backlight delays have also kept as placeholders and will be used used
> > once we have MIPI backlight enabling support
> > 
> > Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
> 
> Just a small note on patch ordering, You're using values that no code
> initializes yet, doesn't matter at all this time.
> 
> Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>

Merged up to this one here, thanks for patches&review.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 4/4] drm/i915: Add support for Generic MIPI panel driver

[Shobhit Kumar]

Thanks Damien for your review

On Thursday 15 May 2014 10:18 PM, Damien Lespiau wrote:
> On Mon, Apr 14, 2014 at 11:18:27AM +0530, Shobhit Kumar wrote:
>> >This driver makes use of the generic panel information from the VBT.
>> >Panel information is classified into two - panel configuration and panel
>> >power sequence which is unique to each panel. The generic driver uses the
>> >panel configuration and sequence parsed from VBT block #52 and #53
>> >
>> >v2: Address review comments by Jani
>> >     - Move all of the things in driver c file from header
>> >     - Make all functions static
>> >     - Make use of video/mipi_display.c instead of redefining
>> >     - Null checks during sequence execution
>> >
>> >Signed-off-by: Shobhit Kumar<shobhit.kumar@intel.com>
> I've done a first past on this. Overall looks reasonable. I'm missing
> some documentation to double check the various LP->HS, HS->LP count and
> other magic around the clocks (send you a mail about it) before I can
> add my r-b tag.
>
> I've added a few tiny comments as well along the road.

All look okay to me and Will push updated patch asap.

There is one issue which I am struggling for now. If we have all these 
patches in, then disable sequence pipe off does not work and 
wait_for_pipe_off gives a warn dump but everything works. Its not this 
patch issue but DSI patches that are already merged. I know the fix is 
to actually disable port after disabling pipe and plane but doing that 
does not succeed enable in first attempt. Subsequent disable/enable 
works. Looking into that and should have a fix by next week on that.

Regards
Shobhit

Re: [PATCH 4/4] drm/i915: Add support for Generic MIPI panel driver

[Damien Lespiau]

On Thu, May 15, 2014 at 05:48:57PM +0100, Damien Lespiau wrote:
> > +static struct gpio_table gtable[] = {
> 
> const
> 

Please, disregard this comment. It's being written to to track GPIO
initialization.

--
Damien

Re: [PATCH 4/4] drm/i915: Add support for Generic MIPI panel driver

[Damien Lespiau]

On Mon, Apr 14, 2014 at 11:18:27AM +0530, Shobhit Kumar wrote:
> +#define NS_MHZ_RATIO 1000000

[...]

> +static bool generic_init(struct intel_dsi_device *dsi)
> +{

[...]

> +	/*
> +	 * ui(s) = 1/f [f in hz]
> +	 * ui(ns) = 10^9/f*10^6 [f in Mhz] -> 10^3/f(Mhz)

ui(ns) = 10^9/(f*10^6)

> +	 *
> +	 * LP byte clock = TLPX/8ui

Mind putting that comment just above the appropriate computation?
Also, LP byte clock = Tlpx / (8UI)

> +	 *
> +	 * Since txddrclkhs_i is 2xUI, the count values programmed in
> +	 * DPHY param registers are divided by 2

That looks like a general comment that apply to a bunch of calculations
below, probably worth separating it from the UI comment.

> +	 *
> +	 */
> +
> +	/* in Kbps */
> +	ui_num = bitrate;
> +	ui_den = NS_MHZ_RATIO;

I'm a bit confused here, most likely missing something, care to clarify?

- IIUC, you want the computations to happen in ns. I'm a bit puzzled by
  that NS_MHZ_RATIO constant name when we're dealing with Kbps.

  That constant is 10^6 which seems to be OK for KHz. So maybe just a
  naming problem?

- UI is a period, so is homogeneous to time (s), but ui_num being in
  s^-1 and ui_den a constant, ui_num/ui_den looks like a frequency. Or
  could it be that UI = ui_den / ui_num? would be confusing, but the
  code below would make more sense. In which case could we have UI =
  ui_num / ui_den?

> +
> +	tclk_prepare_clkzero = mipi_config->tclk_prepare_clkzero;
> +	ths_prepare_hszero = mipi_config->ths_prepare_hszero;
> +
> +	/* B060 */
> +	intel_dsi->lp_byte_clk = CEIL_DIV(tlpx_ns * ui_num, 8 * ui_den);
> +
> +	/* count values in UI = (ns value) * (bitrate / (2 * 10^6)) */
> +	/* prepare count */
> +	ths_prepare_ns =
> +		(mipi_config->ths_prepare >  mipi_config->tclk_prepare) ?
> +				mipi_config->ths_prepare :
> +				mipi_config->tclk_prepare;

That looks like max()

> +
> +	prepare_cnt = CEIL_DIV(ths_prepare_ns * ui_num,	ui_den * 2);

The formula above has a 10^6, why is that OK not to have it there? (in
which unit is bitrate in the formula? MHz?) Is this something like:

  Count in UI = count(ns) / UI(ns)
  
and then as UI = ui_den/ui_num (?!) we end up with:

  Count in UI = count(ns) * ui_num / ui_den

And then the / 2 comment applies.

> +
> +	/* exit zero count */
> +	exit_zero_cnt = CEIL_DIV(
> +				(ths_prepare_hszero - ths_prepare_ns) * ui_num,
> +				ui_den * 2
> +				);
> +
> +	/*
> +	 * Exit zero  is unified val ths_zero and ths_exit
> +	 * minimum value for ths_exit = 110ns
> +	 * min (exit_zero_cnt * 2) = 110/UI
> +	 * exit_zero_cnt = 55/UI
> +	 */
> +	 if (exit_zero_cnt < (55 * ui_num / ui_den))
> +		if ((55 * ui_num) % ui_den)
> +			exit_zero_cnt += 1;

I'm not sure what we're achieving with the +=1 here, mind explaining?

> +
> +	/* clk zero count */
> +	clk_zero_cnt = CEIL_DIV(
> +			(tclk_prepare_clkzero -	ths_prepare_ns)
> +			* ui_num, 2 * ui_den);
> +
> +	/* trail count */
> +	tclk_trail_ns = (mipi_config->tclk_trail > mipi_config->ths_trail) ?
> +			mipi_config->tclk_trail : mipi_config->ths_trail;

max() 

> +	trail_cnt = CEIL_DIV(tclk_trail_ns * ui_num, 2 * ui_den);
> +
> +	if (prepare_cnt > PREPARE_CNT_MAX ||
> +		exit_zero_cnt > EXIT_ZERO_CNT_MAX ||
> +		clk_zero_cnt > CLK_ZERO_CNT_MAX ||
> +		trail_cnt > TRAIL_CNT_MAX)
> +		DRM_DEBUG_DRIVER("Values crossing maximum limits\n");

Is that a situation that may happen in a normal case? or should we go
with a DRM_ERROR() here and potentially abort the modeset?

> +
> +	if (prepare_cnt > PREPARE_CNT_MAX)
> +		prepare_cnt = PREPARE_CNT_MAX;
> +
> +	if (exit_zero_cnt > EXIT_ZERO_CNT_MAX)
> +		exit_zero_cnt = EXIT_ZERO_CNT_MAX;
> +
> +	if (clk_zero_cnt > CLK_ZERO_CNT_MAX)
> +		clk_zero_cnt = CLK_ZERO_CNT_MAX;
> +
> +	if (trail_cnt > TRAIL_CNT_MAX)
> +		trail_cnt = TRAIL_CNT_MAX;
> +
> +	/* B080 */
> +	intel_dsi->dphy_reg = exit_zero_cnt << 24 | trail_cnt << 16 |
> +						clk_zero_cnt << 8 | prepare_cnt;
> +
> +	/*
> +	 * LP to HS switch count = 4TLPX + PREP_COUNT * 2 + EXIT_ZERO_COUNT * 2
> +	 *					+ 10UI + Extra Byte Count
> +	 *
> +	 * HS to LP switch count = THS-TRAIL + 2TLPX + Extra Byte Count
> +	 * Extra Byte Count is calculated according to number of lanes.
> +	 * High Low Switch Count is the Max of LP to HS and
> +	 * HS to LP switch count
> +	 *
> +	 */
> +	tlpx_ui = CEIL_DIV(tlpx_ns * ui_num, ui_den);
> +
> +	/* B044 */
> +	intel_dsi->hs_to_lp_count =
> +		CEIL_DIV(
> +			4 * tlpx_ui + prepare_cnt * 2 +
> +			exit_zero_cnt * 2 + 10,
> +			8);

The previous was before I tried to look at the spec too closely. Mind
explaining why we don't look at the HS to LP switch count? ie why HS to
LP switch cound is always smaller than the LP to HS one?

> +
> +	intel_dsi->hs_to_lp_count += extra_byte_count;
> +
> +	/* B088 */
> +	/* LP -> HS for clock lanes
> +	 * LP clk sync + LP11 + LP01 + tclk_prepare + tclk_zero +
> +	 *						extra byte count
> +	 * 2TPLX + 1TLPX + 1 TPLX(in ns) + prepare_cnt * 2 + clk_zero_cnt *
> +	 *					2(in UI) + extra byte count
> +	 * In byteclks = (4TLPX + prepare_cnt * 2 + clk_zero_cnt *2 (in UI)) /
> +	 *					8 + extra byte count
> +	 */
> +	intel_dsi->clk_lp_to_hs_count =
> +		CEIL_DIV(
> +			4 * tlpx_ui + prepare_cnt * 2 +
> +			clk_zero_cnt * 2,
> +			8);
> +
> +	intel_dsi->clk_lp_to_hs_count += extra_byte_count;
> +
> +	/* HS->LP for Clock Lanes
> +	 * Low Power clock synchronisations + 1Tx byteclk + tclk_trail +
> +	 *						Extra byte count
> +	 * 2TLPX + 8UI + (trail_count*2)(in UI) + Extra byte count
> +	 * In byteclks = (2*TLpx(in UI) + trail_count*2 +8)(in UI)/8 +
> +	 *						Extra byte count
> +	 */
> +	intel_dsi->clk_hs_to_lp_count =
> +		CEIL_DIV(2 * tlpx_ui + trail_cnt * 2 + 8,
> +			8);
> +	intel_dsi->clk_hs_to_lp_count += extra_byte_count;
> +
> +	DRM_DEBUG_KMS("EOT %s\n", intel_dsi->eotp_pkt ? "ENABLED" : "DISABLED");
> +	DRM_DEBUG_KMS("CLOCKSTOP %s\n", intel_dsi->clock_stop ?
> +						"ENABLED" : "DISABLED");
> +	DRM_DEBUG_KMS("Mode %s\n", intel_dsi->operation_mode ? "COMMAND" : "VIDEO");
> +	DRM_DEBUG_KMS("Pixel Format %d\n", intel_dsi->pixel_format);
> +	DRM_DEBUG_KMS("TLPX %d\n", intel_dsi->escape_clk_div);
> +	DRM_DEBUG_KMS("LP RX Timeout 0x%x\n", intel_dsi->lp_rx_timeout);
> +	DRM_DEBUG_KMS("Turnaround Timeout 0x%x\n", intel_dsi->turn_arnd_val);
> +	DRM_DEBUG_KMS("Init Count 0x%x\n", intel_dsi->init_count);
> +	DRM_DEBUG_KMS("HS to LP Count 0x%x\n", intel_dsi->hs_to_lp_count);
> +	DRM_DEBUG_KMS("LP Byte Clock %d\n", intel_dsi->lp_byte_clk);
> +	DRM_DEBUG_KMS("DBI BW Timer 0x%x\n", intel_dsi->bw_timer);
> +	DRM_DEBUG_KMS("LP to HS Clock Count 0x%x\n", intel_dsi->clk_lp_to_hs_count);
> +	DRM_DEBUG_KMS("HS to LP Clock Count 0x%x\n", intel_dsi->clk_hs_to_lp_count);
> +	DRM_DEBUG_KMS("BTA %s\n",
> +			intel_dsi->video_frmt_cfg_bits & DISABLE_VIDEO_BTA ?
> +			"DISABLED" : "ENABLED");
> +	DRM_DEBUG_KMS("B060 = 0x%Xx, B080 = 0x%x, B044 = 0x%x, B088 = 0x%x\n",
> +			intel_dsi->lp_byte_clk, intel_dsi->dphy_reg, intel_dsi->hs_to_lp_count,
> +			(intel_dsi->clk_lp_to_hs_count << LP_HS_SSW_CNT_SHIFT) |
> +			(intel_dsi->clk_hs_to_lp_count << HS_LP_PWR_SW_CNT_SHIFT));
> +
> +	/* delays in VBT are in unit of 100us, so need to convert
> +	 * here in ms
> +	 * Delay (100us) * 100 /1000 = Delay / 10 (ms) */
> +	intel_dsi->backlight_off_delay = pps->bl_disable_delay / 10;
> +	intel_dsi->backlight_on_delay = pps->bl_enable_delay / 10;
> +	intel_dsi->panel_on_delay = pps->panel_on_delay / 10;
> +	intel_dsi->panel_off_delay = pps->panel_off_delay / 10;
> +	intel_dsi->panel_pwr_cycle_delay = pps->panel_power_cycle_delay / 10;
> +
> +	return true;
> +}

Re: [PATCH 4/4] drm/i915: Add support for Generic MIPI panel driver

[Shobhit Kumar]

On Monday 19 May 2014 07:53 PM, Damien Lespiau wrote:
> On Mon, Apr 14, 2014 at 11:18:27AM +0530, Shobhit Kumar wrote:
>> +#define NS_MHZ_RATIO 1000000
>
> [...]
>
>> +static bool generic_init(struct intel_dsi_device *dsi)
>> +{
>
> [...]
>
>> +	/*
>> +	 * ui(s) = 1/f [f in hz]
>> +	 * ui(ns) = 10^9/f*10^6 [f in Mhz] -> 10^3/f(Mhz)
>
> ui(ns) = 10^9/(f*10^6)
>
>> +	 *
>> +	 * LP byte clock = TLPX/8ui
>
> Mind putting that comment just above the appropriate computation?
> Also, LP byte clock = Tlpx / (8UI)
>
>> +	 *
>> +	 * Since txddrclkhs_i is 2xUI, the count values programmed in
>> +	 * DPHY param registers are divided by 2
>
> That looks like a general comment that apply to a bunch of calculations
> below, probably worth separating it from the UI comment.
>

Will update as suggested.

>> +	 *
>> +	 */
>> +
>> +	/* in Kbps */
>> +	ui_num = bitrate;
>> +	ui_den = NS_MHZ_RATIO;
>
> I'm a bit confused here, most likely missing something, care to clarify?
>
> - IIUC, you want the computations to happen in ns. I'm a bit puzzled by
>    that NS_MHZ_RATIO constant name when we're dealing with Kbps.
>
>    That constant is 10^6 which seems to be OK for KHz. So maybe just a
>    naming problem?

Yeah, I now realize that it should be something like NS_KHZ_RATIO to 
avoid confusion

>
> - UI is a period, so is homogeneous to time (s), but ui_num being in
>    s^-1 and ui_den a constant, ui_num/ui_den looks like a frequency. Or
>    could it be that UI = ui_den / ui_num? would be confusing, but the
>    code below would make more sense. In which case could we have UI =
>    ui_num / ui_den?

I just kept ui_num and ui_den separately to take care of precision loss, 
but I see how it is adding to confusion. Actually it is ui_den / ui_num 
and we have all computations as 1/UI so it works. I think I will compute 
UI directly as UI = (NS_KHZ_RATIO * 1000) /bitrate and divide by 1000 
wherever we use to maintain precision. Sounds ok ?

>
>> +
>> +	tclk_prepare_clkzero = mipi_config->tclk_prepare_clkzero;
>> +	ths_prepare_hszero = mipi_config->ths_prepare_hszero;
>> +
>> +	/* B060 */
>> +	intel_dsi->lp_byte_clk = CEIL_DIV(tlpx_ns * ui_num, 8 * ui_den);
>> +
>> +	/* count values in UI = (ns value) * (bitrate / (2 * 10^6)) */
>> +	/* prepare count */
>> +	ths_prepare_ns =
>> +		(mipi_config->ths_prepare >  mipi_config->tclk_prepare) ?
>> +				mipi_config->ths_prepare :
>> +				mipi_config->tclk_prepare;
>
> That looks like max()
>
>> +
>> +	prepare_cnt = CEIL_DIV(ths_prepare_ns * ui_num,	ui_den * 2);
>
> The formula above has a 10^6, why is that OK not to have it there? (in
> which unit is bitrate in the formula? MHz?) Is this something like:
>
>    Count in UI = count(ns) / UI(ns)
>
> and then as UI = ui_den/ui_num (?!) we end up with:
>
>    Count in UI = count(ns) * ui_num / ui_den
>
> And then the / 2 comment applies.

Yeah actually its like this. I will correct as suggested above.

>
>> +
>> +	/* exit zero count */
>> +	exit_zero_cnt = CEIL_DIV(
>> +				(ths_prepare_hszero - ths_prepare_ns) * ui_num,
>> +				ui_den * 2
>> +				);
>> +
>> +	/*
>> +	 * Exit zero  is unified val ths_zero and ths_exit
>> +	 * minimum value for ths_exit = 110ns
>> +	 * min (exit_zero_cnt * 2) = 110/UI
>> +	 * exit_zero_cnt = 55/UI
>> +	 */
>> +	 if (exit_zero_cnt < (55 * ui_num / ui_den))
>> +		if ((55 * ui_num) % ui_den)
>> +			exit_zero_cnt += 1;
>
> I'm not sure what we're achieving with the +=1 here, mind explaining?

This is as per MIPI host controller spec to ceil the value

>
>> +
>> +	/* clk zero count */
>> +	clk_zero_cnt = CEIL_DIV(
>> +			(tclk_prepare_clkzero -	ths_prepare_ns)
>> +			* ui_num, 2 * ui_den);
>> +
>> +	/* trail count */
>> +	tclk_trail_ns = (mipi_config->tclk_trail > mipi_config->ths_trail) ?
>> +			mipi_config->tclk_trail : mipi_config->ths_trail;
>
> max()
>
>> +	trail_cnt = CEIL_DIV(tclk_trail_ns * ui_num, 2 * ui_den);
>> +
>> +	if (prepare_cnt > PREPARE_CNT_MAX ||
>> +		exit_zero_cnt > EXIT_ZERO_CNT_MAX ||
>> +		clk_zero_cnt > CLK_ZERO_CNT_MAX ||
>> +		trail_cnt > TRAIL_CNT_MAX)
>> +		DRM_DEBUG_DRIVER("Values crossing maximum limits\n");
>
> Is that a situation that may happen in a normal case? or should we go
> with a DRM_ERROR() here and potentially abort the modeset?
>

Generally it should not happen. We should not abort but clip to max 
values though there is no guarantee it will work, but there is high 
chance that it will work.

>> +
>> +	if (prepare_cnt > PREPARE_CNT_MAX)
>> +		prepare_cnt = PREPARE_CNT_MAX;
>> +
>> +	if (exit_zero_cnt > EXIT_ZERO_CNT_MAX)
>> +		exit_zero_cnt = EXIT_ZERO_CNT_MAX;
>> +
>> +	if (clk_zero_cnt > CLK_ZERO_CNT_MAX)
>> +		clk_zero_cnt = CLK_ZERO_CNT_MAX;
>> +
>> +	if (trail_cnt > TRAIL_CNT_MAX)
>> +		trail_cnt = TRAIL_CNT_MAX;
>> +
>> +	/* B080 */
>> +	intel_dsi->dphy_reg = exit_zero_cnt << 24 | trail_cnt << 16 |
>> +						clk_zero_cnt << 8 | prepare_cnt;
>> +
>> +	/*
>> +	 * LP to HS switch count = 4TLPX + PREP_COUNT * 2 + EXIT_ZERO_COUNT * 2
>> +	 *					+ 10UI + Extra Byte Count
>> +	 *
>> +	 * HS to LP switch count = THS-TRAIL + 2TLPX + Extra Byte Count
>> +	 * Extra Byte Count is calculated according to number of lanes.
>> +	 * High Low Switch Count is the Max of LP to HS and
>> +	 * HS to LP switch count
>> +	 *
>> +	 */
>> +	tlpx_ui = CEIL_DIV(tlpx_ns * ui_num, ui_den);
>> +
>> +	/* B044 */
>> +	intel_dsi->hs_to_lp_count =
>> +		CEIL_DIV(
>> +			4 * tlpx_ui + prepare_cnt * 2 +
>> +			exit_zero_cnt * 2 + 10,
>> +			8);
>
> The previous was before I tried to look at the spec too closely. Mind
> explaining why we don't look at the HS to LP switch count? ie why HS to
> LP switch cound is always smaller than the LP to HS one?

Because LP to HS uses exit_zero_count which is generally higher than 
clk_zero_count. So just directly used LP to HS which amounts to saying 
that switching from HS to LP takes lesser time than switching from LP to 
HS. I can/should add code to compute max of the two.

>
>> +
>> +	intel_dsi->hs_to_lp_count += extra_byte_count;
>> +
>> +	/* B088 */
>> +	/* LP -> HS for clock lanes
>> +	 * LP clk sync + LP11 + LP01 + tclk_prepare + tclk_zero +
>> +	 *						extra byte count
>> +	 * 2TPLX + 1TLPX + 1 TPLX(in ns) + prepare_cnt * 2 + clk_zero_cnt *
>> +	 *					2(in UI) + extra byte count
>> +	 * In byteclks = (4TLPX + prepare_cnt * 2 + clk_zero_cnt *2 (in UI)) /
>> +	 *					8 + extra byte count
>> +	 */
>> +	intel_dsi->clk_lp_to_hs_count =
>> +		CEIL_DIV(
>> +			4 * tlpx_ui + prepare_cnt * 2 +
>> +			clk_zero_cnt * 2,
>> +			8);
>> +
>> +	intel_dsi->clk_lp_to_hs_count += extra_byte_count;
>> +
>> +	/* HS->LP for Clock Lanes
>> +	 * Low Power clock synchronisations + 1Tx byteclk + tclk_trail +
>> +	 *						Extra byte count
>> +	 * 2TLPX + 8UI + (trail_count*2)(in UI) + Extra byte count
>> +	 * In byteclks = (2*TLpx(in UI) + trail_count*2 +8)(in UI)/8 +
>> +	 *						Extra byte count
>> +	 */
>> +	intel_dsi->clk_hs_to_lp_count =
>> +		CEIL_DIV(2 * tlpx_ui + trail_cnt * 2 + 8,
>> +			8);
>> +	intel_dsi->clk_hs_to_lp_count += extra_byte_count;
>> +
>> +	DRM_DEBUG_KMS("EOT %s\n", intel_dsi->eotp_pkt ? "ENABLED" : "DISABLED");
>> +	DRM_DEBUG_KMS("CLOCKSTOP %s\n", intel_dsi->clock_stop ?
>> +						"ENABLED" : "DISABLED");
>> +	DRM_DEBUG_KMS("Mode %s\n", intel_dsi->operation_mode ? "COMMAND" : "VIDEO");
>> +	DRM_DEBUG_KMS("Pixel Format %d\n", intel_dsi->pixel_format);
>> +	DRM_DEBUG_KMS("TLPX %d\n", intel_dsi->escape_clk_div);
>> +	DRM_DEBUG_KMS("LP RX Timeout 0x%x\n", intel_dsi->lp_rx_timeout);
>> +	DRM_DEBUG_KMS("Turnaround Timeout 0x%x\n", intel_dsi->turn_arnd_val);
>> +	DRM_DEBUG_KMS("Init Count 0x%x\n", intel_dsi->init_count);
>> +	DRM_DEBUG_KMS("HS to LP Count 0x%x\n", intel_dsi->hs_to_lp_count);
>> +	DRM_DEBUG_KMS("LP Byte Clock %d\n", intel_dsi->lp_byte_clk);
>> +	DRM_DEBUG_KMS("DBI BW Timer 0x%x\n", intel_dsi->bw_timer);
>> +	DRM_DEBUG_KMS("LP to HS Clock Count 0x%x\n", intel_dsi->clk_lp_to_hs_count);
>> +	DRM_DEBUG_KMS("HS to LP Clock Count 0x%x\n", intel_dsi->clk_hs_to_lp_count);
>> +	DRM_DEBUG_KMS("BTA %s\n",
>> +			intel_dsi->video_frmt_cfg_bits & DISABLE_VIDEO_BTA ?
>> +			"DISABLED" : "ENABLED");
>> +	DRM_DEBUG_KMS("B060 = 0x%Xx, B080 = 0x%x, B044 = 0x%x, B088 = 0x%x\n",
>> +			intel_dsi->lp_byte_clk, intel_dsi->dphy_reg, intel_dsi->hs_to_lp_count,
>> +			(intel_dsi->clk_lp_to_hs_count << LP_HS_SSW_CNT_SHIFT) |
>> +			(intel_dsi->clk_hs_to_lp_count << HS_LP_PWR_SW_CNT_SHIFT));
>> +
>> +	/* delays in VBT are in unit of 100us, so need to convert
>> +	 * here in ms
>> +	 * Delay (100us) * 100 /1000 = Delay / 10 (ms) */
>> +	intel_dsi->backlight_off_delay = pps->bl_disable_delay / 10;
>> +	intel_dsi->backlight_on_delay = pps->bl_enable_delay / 10;
>> +	intel_dsi->panel_on_delay = pps->panel_on_delay / 10;
>> +	intel_dsi->panel_off_delay = pps->panel_off_delay / 10;
>> +	intel_dsi->panel_pwr_cycle_delay = pps->panel_power_cycle_delay / 10;
>> +
>> +	return true;
>> +}

Re: [PATCH 4/4] drm/i915: Add support for Generic MIPI panel driver

[Damien Lespiau]

On Tue, May 20, 2014 at 09:46:01PM +0530, Shobhit Kumar wrote:
> >- UI is a period, so is homogeneous to time (s), but ui_num being in
> >   s^-1 and ui_den a constant, ui_num/ui_den looks like a frequency. Or
> >   could it be that UI = ui_den / ui_num? would be confusing, but the
> >   code below would make more sense. In which case could we have UI =
> >   ui_num / ui_den?
> 
> I just kept ui_num and ui_den separately to take care of precision
> loss, but I see how it is adding to confusion. Actually it is ui_den
> / ui_num and we have all computations as 1/UI so it works. I think I
> will compute UI directly as UI = (NS_KHZ_RATIO * 1000) /bitrate and
> divide by 1000 wherever we use to maintain precision. Sounds ok ?

I think just exchanging the two variable names (ui_num and ui_den)
should be less work for you and should be enough. It's really just about
having ui_num being the UI numerator so the reader is not too surprised

> >>+	/* B044 */
> >>+	intel_dsi->hs_to_lp_count =
> >>+		CEIL_DIV(
> >>+			4 * tlpx_ui + prepare_cnt * 2 +
> >>+			exit_zero_cnt * 2 + 10,
> >>+			8);
> >
> >The previous was before I tried to look at the spec too closely. Mind
> >explaining why we don't look at the HS to LP switch count? ie why HS to
> >LP switch cound is always smaller than the LP to HS one?
> 
> Because LP to HS uses exit_zero_count which is generally higher than
> clk_zero_count. So just directly used LP to HS which amounts to
> saying that switching from HS to LP takes lesser time than switching
> from LP to HS. I can/should add code to compute max of the two.

This could go to a separate task if you don't have time right now,

Thanks for your answers!

-- 
Damien

Re: [PATCH 4/4] drm/i915: Add support for Generic MIPI panel driver

[Kumar, Shobhit]

On 5/21/2014 2:25 AM, Damien Lespiau wrote:
> On Tue, May 20, 2014 at 09:46:01PM +0530, Shobhit Kumar wrote:
>>> - UI is a period, so is homogeneous to time (s), but ui_num being in
>>>    s^-1 and ui_den a constant, ui_num/ui_den looks like a frequency. Or
>>>    could it be that UI = ui_den / ui_num? would be confusing, but the
>>>    code below would make more sense. In which case could we have UI =
>>>    ui_num / ui_den?
>>
>> I just kept ui_num and ui_den separately to take care of precision
>> loss, but I see how it is adding to confusion. Actually it is ui_den
>> / ui_num and we have all computations as 1/UI so it works. I think I
>> will compute UI directly as UI = (NS_KHZ_RATIO * 1000) /bitrate and
>> divide by 1000 wherever we use to maintain precision. Sounds ok ?
>
> I think just exchanging the two variable names (ui_num and ui_den)
> should be less work for you and should be enough. It's really just about
> having ui_num being the UI numerator so the reader is not too surprised

Yeah. Will fix this

>
>>>> +	/* B044 */
>>>> +	intel_dsi->hs_to_lp_count =
>>>> +		CEIL_DIV(
>>>> +			4 * tlpx_ui + prepare_cnt * 2 +
>>>> +			exit_zero_cnt * 2 + 10,
>>>> +			8);
>>>
>>> The previous was before I tried to look at the spec too closely. Mind
>>> explaining why we don't look at the HS to LP switch count? ie why HS to
>>> LP switch cound is always smaller than the LP to HS one?
>>
>> Because LP to HS uses exit_zero_count which is generally higher than
>> clk_zero_count. So just directly used LP to HS which amounts to
>> saying that switching from HS to LP takes lesser time than switching
>> from LP to HS. I can/should add code to compute max of the two.
>
> This could go to a separate task if you don't have time right now,
>

Most likely I can do this as well. Will push the updated patch by 
sometime tomorrow.

Regards
Shobhit

[v2] drm/i915: Add support for Generic MIPI panel driver

[Shobhit Kumar]

This driver makes use of the generic panel information from the VBT.
Panel information is classified into two - panel configuration and panel
power sequence which is unique to each panel. The generic driver uses the
panel configuration and sequence parsed from VBT block #52 and #53

v2: Address review comments by Jani
    - Move all of the things in driver c file from header
    - Make all functions static
    - Make use of video/mipi_display.c instead of redefining
    - Null checks during sequence execution

v3: Address review comments by Damien
    - Rename the panel driver file as intel_dsi_panel_vbt.c
    - Fix style changes as suggested
    - Correct comments for lp->hs and hs->lp count calculations
    - General updating comments to have more clarity
    - using max() instead of ternary operator
    - Fix names (ui_num, ui_den) while using UI in calculations
    - compute max of lp_to_hs switch and hs_to_lp switch while computing
      hs_lp_switch_count

Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
---
 drivers/gpu/drm/i915/Makefile              |   1 +
 drivers/gpu/drm/i915/intel_dsi.c           |   5 +
 drivers/gpu/drm/i915/intel_dsi.h           |   2 +
 drivers/gpu/drm/i915/intel_dsi_panel_vbt.c | 589 +++++++++++++++++++++++++++++
 4 files changed, 597 insertions(+)
 create mode 100644 drivers/gpu/drm/i915/intel_dsi_panel_vbt.c

diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index 7b2f3be..cad1683 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -62,6 +62,7 @@ i915-y += dvo_ch7017.o \
 	  intel_dsi_cmd.o \
 	  intel_dsi.o \
 	  intel_dsi_pll.o \
+	  intel_dsi_panel_vbt.o \
 	  intel_dvo.o \
 	  intel_hdmi.o \
 	  intel_i2c.o \
diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
index 2525cdd..e73bec6 100644
--- a/drivers/gpu/drm/i915/intel_dsi.c
+++ b/drivers/gpu/drm/i915/intel_dsi.c
@@ -35,6 +35,11 @@
 
 /* the sub-encoders aka panel drivers */
 static const struct intel_dsi_device intel_dsi_devices[] = {
+	{
+		.panel_id = MIPI_DSI_GENERIC_PANEL_ID,
+		.name = "vbt-generic-dsi-vid-mode-display",
+		.dev_ops = &vbt_generic_dsi_display_ops,
+	},
 };
 
 static void band_gap_reset(struct drm_i915_private *dev_priv)
diff --git a/drivers/gpu/drm/i915/intel_dsi.h b/drivers/gpu/drm/i915/intel_dsi.h
index e3f4e91..31db33d 100644
--- a/drivers/gpu/drm/i915/intel_dsi.h
+++ b/drivers/gpu/drm/i915/intel_dsi.h
@@ -133,4 +133,6 @@ static inline struct intel_dsi *enc_to_intel_dsi(struct drm_encoder *encoder)
 extern void vlv_enable_dsi_pll(struct intel_encoder *encoder);
 extern void vlv_disable_dsi_pll(struct intel_encoder *encoder);
 
+extern struct intel_dsi_dev_ops vbt_generic_dsi_display_ops;
+
 #endif /* _INTEL_DSI_H */
diff --git a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
new file mode 100644
index 0000000..21a0d34
--- /dev/null
+++ b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
@@ -0,0 +1,589 @@
+/*
+ * Copyright © 2014 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
+ * DEALINGS IN THE SOFTWARE.
+ *
+ * Author: Shobhit Kumar <shobhit.kumar@intel.com>
+ *
+ */
+
+#include <drm/drmP.h>
+#include <drm/drm_crtc.h>
+#include <drm/drm_edid.h>
+#include <drm/i915_drm.h>
+#include <linux/slab.h>
+#include <video/mipi_display.h>
+#include <asm/intel-mid.h>
+#include <video/mipi_display.h>
+#include "i915_drv.h"
+#include "intel_drv.h"
+#include "intel_dsi.h"
+#include "intel_dsi_cmd.h"
+
+#define MIPI_TRANSFER_MODE_SHIFT	0
+#define MIPI_VIRTUAL_CHANNEL_SHIFT	1
+#define MIPI_PORT_SHIFT			3
+
+#define PREPARE_CNT_MAX		0x3F
+#define EXIT_ZERO_CNT_MAX	0x3F
+#define CLK_ZERO_CNT_MAX	0xFF
+#define TRAIL_CNT_MAX		0x1F
+
+#define NS_KHZ_RATIO 1000000
+
+#define GPI0_NC_0_HV_DDI0_HPD           0x4130
+#define GPIO_NC_0_HV_DDI0_PAD           0x4138
+#define GPIO_NC_1_HV_DDI0_DDC_SDA       0x4120
+#define GPIO_NC_1_HV_DDI0_DDC_SDA_PAD   0x4128
+#define GPIO_NC_2_HV_DDI0_DDC_SCL       0x4110
+#define GPIO_NC_2_HV_DDI0_DDC_SCL_PAD   0x4118
+#define GPIO_NC_3_PANEL0_VDDEN          0x4140
+#define GPIO_NC_3_PANEL0_VDDEN_PAD      0x4148
+#define GPIO_NC_4_PANEL0_BLKEN          0x4150
+#define GPIO_NC_4_PANEL0_BLKEN_PAD      0x4158
+#define GPIO_NC_5_PANEL0_BLKCTL         0x4160
+#define GPIO_NC_5_PANEL0_BLKCTL_PAD     0x4168
+#define GPIO_NC_6_PCONF0                0x4180
+#define GPIO_NC_6_PAD                   0x4188
+#define GPIO_NC_7_PCONF0                0x4190
+#define GPIO_NC_7_PAD                   0x4198
+#define GPIO_NC_8_PCONF0                0x4170
+#define GPIO_NC_8_PAD                   0x4178
+#define GPIO_NC_9_PCONF0                0x4100
+#define GPIO_NC_9_PAD                   0x4108
+#define GPIO_NC_10_PCONF0               0x40E0
+#define GPIO_NC_10_PAD                  0x40E8
+#define GPIO_NC_11_PCONF0               0x40F0
+#define GPIO_NC_11_PAD                  0x40F8
+
+struct gpio_table {
+	u16 function_reg;
+	u16 pad_reg;
+	u8 init;
+};
+
+static struct gpio_table gtable[] = {
+	{ GPI0_NC_0_HV_DDI0_HPD, GPIO_NC_0_HV_DDI0_PAD, 0 },
+	{ GPIO_NC_1_HV_DDI0_DDC_SDA, GPIO_NC_1_HV_DDI0_DDC_SDA_PAD, 0 },
+	{ GPIO_NC_2_HV_DDI0_DDC_SCL, GPIO_NC_2_HV_DDI0_DDC_SCL_PAD, 0 },
+	{ GPIO_NC_3_PANEL0_VDDEN, GPIO_NC_3_PANEL0_VDDEN_PAD, 0 },
+	{ GPIO_NC_4_PANEL0_BLKEN, GPIO_NC_4_PANEL0_BLKEN_PAD, 0 },
+	{ GPIO_NC_5_PANEL0_BLKCTL, GPIO_NC_5_PANEL0_BLKCTL_PAD, 0 },
+	{ GPIO_NC_6_PCONF0, GPIO_NC_6_PAD, 0 },
+	{ GPIO_NC_7_PCONF0, GPIO_NC_7_PAD, 0 },
+	{ GPIO_NC_8_PCONF0, GPIO_NC_8_PAD, 0 },
+	{ GPIO_NC_9_PCONF0, GPIO_NC_9_PAD, 0 },
+	{ GPIO_NC_10_PCONF0, GPIO_NC_10_PAD, 0},
+	{ GPIO_NC_11_PCONF0, GPIO_NC_11_PAD, 0}
+};
+
+static u8 *mipi_exec_send_packet(struct intel_dsi *intel_dsi, u8 *data)
+{
+	u8 type, byte, mode, vc, port;
+	u16 len;
+
+	byte = *data++;
+	mode = (byte >> MIPI_TRANSFER_MODE_SHIFT) & 0x1;
+	vc = (byte >> MIPI_VIRTUAL_CHANNEL_SHIFT) & 0x3;
+	port = (byte >> MIPI_PORT_SHIFT) & 0x3;
+
+	/* LP or HS mode */
+	intel_dsi->hs = mode;
+
+	/* get packet type and increment the pointer */
+	type = *data++;
+
+	len = *((u16 *) data);
+	data += 2;
+
+	switch (type) {
+	case MIPI_DSI_GENERIC_SHORT_WRITE_0_PARAM:
+		dsi_vc_generic_write_0(intel_dsi, vc);
+		break;
+	case MIPI_DSI_GENERIC_SHORT_WRITE_1_PARAM:
+		dsi_vc_generic_write_1(intel_dsi, vc, *data);
+		break;
+	case MIPI_DSI_GENERIC_SHORT_WRITE_2_PARAM:
+		dsi_vc_generic_write_2(intel_dsi, vc, *data, *(data + 1));
+		break;
+	case MIPI_DSI_GENERIC_READ_REQUEST_0_PARAM:
+	case MIPI_DSI_GENERIC_READ_REQUEST_1_PARAM:
+	case MIPI_DSI_GENERIC_READ_REQUEST_2_PARAM:
+		DRM_DEBUG_DRIVER("Generic Read not yet implemented or used\n");
+		break;
+	case MIPI_DSI_GENERIC_LONG_WRITE:
+		dsi_vc_generic_write(intel_dsi, vc, data, len);
+		break;
+	case MIPI_DSI_DCS_SHORT_WRITE:
+		dsi_vc_dcs_write_0(intel_dsi, vc, *data);
+		break;
+	case MIPI_DSI_DCS_SHORT_WRITE_PARAM:
+		dsi_vc_dcs_write_1(intel_dsi, vc, *data, *(data + 1));
+		break;
+	case MIPI_DSI_DCS_READ:
+		DRM_DEBUG_DRIVER("DCS Read not yet implemented or used\n");
+		break;
+	case MIPI_DSI_DCS_LONG_WRITE:
+		dsi_vc_dcs_write(intel_dsi, vc, data, len);
+		break;
+	};
+
+	data += len;
+
+	return data;
+}
+
+static u8 *mipi_exec_delay(struct intel_dsi *intel_dsi, u8 *data)
+{
+	u32 delay = *((u32 *) data);
+
+	usleep_range(delay, delay + 10);
+	data += 4;
+
+	return data;
+}
+
+static u8 *mipi_exec_gpio(struct intel_dsi *intel_dsi, u8 *data)
+{
+	u8 gpio, action;
+	u16 function, pad;
+	u32 val;
+	struct drm_device *dev = intel_dsi->base.base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	gpio = *data++;
+
+	/* pull up/down */
+	action = *data++;
+
+	function = gtable[gpio].function_reg;
+	pad = gtable[gpio].pad_reg;
+
+	mutex_lock(&dev_priv->dpio_lock);
+	if (!gtable[gpio].init) {
+		/* program the function */
+		/* FIXME: remove constant below */
+		vlv_gpio_nc_write(dev_priv, function, 0x2000CC00);
+		gtable[gpio].init = 1;
+	}
+
+	val = 0x4 | action;
+
+	/* pull up/down */
+	vlv_gpio_nc_write(dev_priv, pad, val);
+	mutex_unlock(&dev_priv->dpio_lock);
+
+	return data;
+}
+
+typedef u8 * (*fn_mipi_elem_exec)(struct intel_dsi *intel_dsi, u8 *data);
+static const fn_mipi_elem_exec exec_elem[] = {
+	NULL, /* reserved */
+	mipi_exec_send_packet,
+	mipi_exec_delay,
+	mipi_exec_gpio,
+	NULL, /* status read; later */
+};
+
+/*
+ * MIPI Sequence from VBT #53 parsing logic
+ * We have already separated each seqence during bios parsing
+ * Following is generic execution function for any sequence
+ */
+
+static const char * const seq_name[] = {
+	"UNDEFINED",
+	"MIPI_SEQ_ASSERT_RESET",
+	"MIPI_SEQ_INIT_OTP",
+	"MIPI_SEQ_DISPLAY_ON",
+	"MIPI_SEQ_DISPLAY_OFF",
+	"MIPI_SEQ_DEASSERT_RESET"
+};
+
+static void generic_exec_sequence(struct intel_dsi *intel_dsi, char *sequence)
+{
+	u8 *data = sequence;
+	fn_mipi_elem_exec mipi_elem_exec;
+	int index;
+
+	if (!sequence)
+		return;
+
+	DRM_DEBUG_DRIVER("Starting MIPI sequence - %s\n", seq_name[*data]);
+
+	/* go to the first element of the sequence */
+	data++;
+
+	/* parse each byte till we reach end of sequence byte - 0x00 */
+	while (1) {
+		index = *data;
+		mipi_elem_exec = exec_elem[index];
+		if (!mipi_elem_exec) {
+			DRM_ERROR("Unsupported MIPI element, skipping sequence execution\n");
+			return;
+		}
+
+		/* goto element payload */
+		data++;
+
+		/* execute the element specific rotines */
+		data = mipi_elem_exec(intel_dsi, data);
+
+		/*
+		 * After processing the element, data should point to
+		 * next element or end of sequence
+		 * check if have we reached end of sequence
+		 */
+		if (*data == 0x00)
+			break;
+	}
+}
+
+static bool generic_init(struct intel_dsi_device *dsi)
+{
+	struct intel_dsi *intel_dsi = container_of(dsi, struct intel_dsi, dev);
+	struct drm_device *dev = intel_dsi->base.base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct mipi_config *mipi_config = dev_priv->vbt.dsi.config;
+	struct mipi_pps_data *pps = dev_priv->vbt.dsi.pps;
+	struct drm_display_mode *mode = dev_priv->vbt.lfp_lvds_vbt_mode;
+	u32 bits_per_pixel = 24;
+	u32 tlpx_ns, extra_byte_count, bitrate, tlpx_ui;
+	u32 ui_num, ui_den;
+	u32 prepare_cnt, exit_zero_cnt, clk_zero_cnt, trail_cnt;
+	u32 ths_prepare_ns, tclk_trail_ns;
+	u32 tclk_prepare_clkzero, ths_prepare_hszero;
+	u32 lp_to_hs_switch, hs_to_lp_switch;
+
+	DRM_DEBUG_KMS("\n");
+
+	intel_dsi->eotp_pkt = mipi_config->eot_pkt_disabled ? 0 : 1;
+	intel_dsi->clock_stop = mipi_config->enable_clk_stop ? 1 : 0;
+	intel_dsi->lane_count = mipi_config->lane_cnt + 1;
+	intel_dsi->pixel_format = mipi_config->videomode_color_format << 7;
+
+	if (intel_dsi->pixel_format == VID_MODE_FORMAT_RGB666)
+		bits_per_pixel = 18;
+	else if (intel_dsi->pixel_format == VID_MODE_FORMAT_RGB565)
+		bits_per_pixel = 16;
+
+	bitrate = (mode->clock * bits_per_pixel) / intel_dsi->lane_count;
+
+	intel_dsi->operation_mode = mipi_config->is_cmd_mode;
+	intel_dsi->video_mode_format = mipi_config->video_transfer_mode;
+	intel_dsi->escape_clk_div = mipi_config->byte_clk_sel;
+	intel_dsi->lp_rx_timeout = mipi_config->lp_rx_timeout;
+	intel_dsi->turn_arnd_val = mipi_config->turn_around_timeout;
+	intel_dsi->rst_timer_val = mipi_config->device_reset_timer;
+	intel_dsi->init_count = mipi_config->master_init_timer;
+	intel_dsi->bw_timer = mipi_config->dbi_bw_timer;
+	intel_dsi->video_frmt_cfg_bits = mipi_config->bta_enabled ? DISABLE_VIDEO_BTA : 0;
+
+	switch (intel_dsi->escape_clk_div) {
+	case 0:
+		tlpx_ns = 50;
+		break;
+	case 1:
+		tlpx_ns = 100;
+		break;
+
+	case 2:
+		tlpx_ns = 200;
+		break;
+	default:
+		tlpx_ns = 50;
+		break;
+	}
+
+	switch (intel_dsi->lane_count) {
+	case 1:
+	case 2:
+		extra_byte_count = 2;
+		break;
+	case 3:
+		extra_byte_count = 4;
+		break;
+	case 4:
+	default:
+		extra_byte_count = 3;
+		break;
+	}
+
+	/*
+	 * ui(s) = 1/f [f in hz]
+	 * ui(ns) = 10^9 / (f*10^6) [f in Mhz] -> 10^3/f(Mhz)
+	 */
+
+	/* in Kbps */
+	ui_num = NS_KHZ_RATIO;
+	ui_den = bitrate;
+
+	tclk_prepare_clkzero = mipi_config->tclk_prepare_clkzero;
+	ths_prepare_hszero = mipi_config->ths_prepare_hszero;
+
+	/*
+	 * B060
+	 * LP byte clock = TLPX/ (8UI)
+	 */
+	intel_dsi->lp_byte_clk = DIV_ROUND_UP(tlpx_ns * ui_den, 8 * ui_num);
+
+	/* count values in UI = (ns value) * (bitrate / (2 * 10^6))
+	 *
+	 * Since txddrclkhs_i is 2xUI, all the count values programmed in
+	 * DPHY param register are divided by 2
+	 *
+	 * prepare count
+	 */
+	ths_prepare_ns = max(mipi_config->ths_prepare, mipi_config->tclk_prepare);
+	prepare_cnt = DIV_ROUND_UP(ths_prepare_ns * ui_den, ui_num * 2);
+
+	/* exit zero count */
+	exit_zero_cnt = DIV_ROUND_UP(
+				(ths_prepare_hszero - ths_prepare_ns) * ui_den,
+				ui_num * 2
+				);
+
+	/*
+	 * Exit zero  is unified val ths_zero and ths_exit
+	 * minimum value for ths_exit = 110ns
+	 * min (exit_zero_cnt * 2) = 110/UI
+	 * exit_zero_cnt = 55/UI
+	 */
+	 if (exit_zero_cnt < (55 * ui_den / ui_num))
+		if ((55 * ui_den) % ui_num)
+			exit_zero_cnt += 1;
+
+	/* clk zero count */
+	clk_zero_cnt = DIV_ROUND_UP(
+			(tclk_prepare_clkzero -	ths_prepare_ns)
+			* ui_den, 2 * ui_num);
+
+	/* trail count */
+	tclk_trail_ns = max(mipi_config->tclk_trail, mipi_config->ths_trail);
+	trail_cnt = DIV_ROUND_UP(tclk_trail_ns * ui_den, 2 * ui_num);
+
+	if (prepare_cnt > PREPARE_CNT_MAX ||
+		exit_zero_cnt > EXIT_ZERO_CNT_MAX ||
+		clk_zero_cnt > CLK_ZERO_CNT_MAX ||
+		trail_cnt > TRAIL_CNT_MAX)
+		DRM_DEBUG_DRIVER("Values crossing maximum limits, restricting to max values\n");
+
+	if (prepare_cnt > PREPARE_CNT_MAX)
+		prepare_cnt = PREPARE_CNT_MAX;
+
+	if (exit_zero_cnt > EXIT_ZERO_CNT_MAX)
+		exit_zero_cnt = EXIT_ZERO_CNT_MAX;
+
+	if (clk_zero_cnt > CLK_ZERO_CNT_MAX)
+		clk_zero_cnt = CLK_ZERO_CNT_MAX;
+
+	if (trail_cnt > TRAIL_CNT_MAX)
+		trail_cnt = TRAIL_CNT_MAX;
+
+	/* B080 */
+	intel_dsi->dphy_reg = exit_zero_cnt << 24 | trail_cnt << 16 |
+						clk_zero_cnt << 8 | prepare_cnt;
+
+	/*
+	 * LP to HS switch count = 4TLPX + PREP_COUNT * 2 + EXIT_ZERO_COUNT * 2
+	 *					+ 10UI + Extra Byte Count
+	 *
+	 * HS to LP switch count = THS-TRAIL + 2TLPX + Extra Byte Count
+	 * Extra Byte Count is calculated according to number of lanes.
+	 * High Low Switch Count is the Max of LP to HS and
+	 * HS to LP switch count
+	 *
+	 */
+	tlpx_ui = DIV_ROUND_UP(tlpx_ns * ui_den, ui_num);
+
+	/* B044 */
+	/* FIXME:
+	 * The comment above does not match with the code */
+	lp_to_hs_switch = DIV_ROUND_UP(4 * tlpx_ui + prepare_cnt * 2 +
+						exit_zero_cnt * 2 + 10, 8);
+
+	hs_to_lp_switch = DIV_ROUND_UP(mipi_config->ths_trail + 2 * tlpx_ui, 8);
+
+	intel_dsi->hs_to_lp_count = max(lp_to_hs_switch, hs_to_lp_switch);
+	intel_dsi->hs_to_lp_count += extra_byte_count;
+
+	/* B088 */
+	/* LP -> HS for clock lanes
+	 * LP clk sync + LP11 + LP01 + tclk_prepare + tclk_zero +
+	 *						extra byte count
+	 * 2TPLX + 1TLPX + 1 TPLX(in ns) + prepare_cnt * 2 + clk_zero_cnt *
+	 *					2(in UI) + extra byte count
+	 * In byteclks = (4TLPX + prepare_cnt * 2 + clk_zero_cnt *2 (in UI)) /
+	 *					8 + extra byte count
+	 */
+	intel_dsi->clk_lp_to_hs_count =
+		DIV_ROUND_UP(
+			4 * tlpx_ui + prepare_cnt * 2 +
+			clk_zero_cnt * 2,
+			8);
+
+	intel_dsi->clk_lp_to_hs_count += extra_byte_count;
+
+	/* HS->LP for Clock Lanes
+	 * Low Power clock synchronisations + 1Tx byteclk + tclk_trail +
+	 *						Extra byte count
+	 * 2TLPX + 8UI + (trail_count*2)(in UI) + Extra byte count
+	 * In byteclks = (2*TLpx(in UI) + trail_count*2 +8)(in UI)/8 +
+	 *						Extra byte count
+	 */
+	intel_dsi->clk_hs_to_lp_count =
+		DIV_ROUND_UP(2 * tlpx_ui + trail_cnt * 2 + 8,
+			8);
+	intel_dsi->clk_hs_to_lp_count += extra_byte_count;
+
+	DRM_DEBUG_KMS("Eot %s\n", intel_dsi->eotp_pkt ? "enabled" : "disabled");
+	DRM_DEBUG_KMS("Clockstop %s\n", intel_dsi->clock_stop ?
+						"disabled" : "enabled");
+	DRM_DEBUG_KMS("Mode %s\n", intel_dsi->operation_mode ? "command" : "video");
+	DRM_DEBUG_KMS("Pixel Format %d\n", intel_dsi->pixel_format);
+	DRM_DEBUG_KMS("TLPX %d\n", intel_dsi->escape_clk_div);
+	DRM_DEBUG_KMS("LP RX Timeout 0x%x\n", intel_dsi->lp_rx_timeout);
+	DRM_DEBUG_KMS("Turnaround Timeout 0x%x\n", intel_dsi->turn_arnd_val);
+	DRM_DEBUG_KMS("Init Count 0x%x\n", intel_dsi->init_count);
+	DRM_DEBUG_KMS("HS to LP Count 0x%x\n", intel_dsi->hs_to_lp_count);
+	DRM_DEBUG_KMS("LP Byte Clock %d\n", intel_dsi->lp_byte_clk);
+	DRM_DEBUG_KMS("DBI BW Timer 0x%x\n", intel_dsi->bw_timer);
+	DRM_DEBUG_KMS("LP to HS Clock Count 0x%x\n", intel_dsi->clk_lp_to_hs_count);
+	DRM_DEBUG_KMS("HS to LP Clock Count 0x%x\n", intel_dsi->clk_hs_to_lp_count);
+	DRM_DEBUG_KMS("BTA %s\n",
+			intel_dsi->video_frmt_cfg_bits & DISABLE_VIDEO_BTA ?
+			"disabled" : "enabled");
+
+	/* delays in VBT are in unit of 100us, so need to convert
+	 * here in ms
+	 * Delay (100us) * 100 /1000 = Delay / 10 (ms) */
+	intel_dsi->backlight_off_delay = pps->bl_disable_delay / 10;
+	intel_dsi->backlight_on_delay = pps->bl_enable_delay / 10;
+	intel_dsi->panel_on_delay = pps->panel_on_delay / 10;
+	intel_dsi->panel_off_delay = pps->panel_off_delay / 10;
+	intel_dsi->panel_pwr_cycle_delay = pps->panel_power_cycle_delay / 10;
+
+	return true;
+}
+
+static int generic_mode_valid(struct intel_dsi_device *dsi,
+		   struct drm_display_mode *mode)
+{
+	return MODE_OK;
+}
+
+static bool generic_mode_fixup(struct intel_dsi_device *dsi,
+		    const struct drm_display_mode *mode,
+		    struct drm_display_mode *adjusted_mode) {
+	return true;
+}
+
+static void generic_panel_reset(struct intel_dsi_device *dsi)
+{
+	struct intel_dsi *intel_dsi = container_of(dsi, struct intel_dsi, dev);
+	struct drm_device *dev = intel_dsi->base.base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	char *sequence = dev_priv->vbt.dsi.sequence[MIPI_SEQ_ASSERT_RESET];
+
+	generic_exec_sequence(intel_dsi, sequence);
+}
+
+static void generic_disable_panel_power(struct intel_dsi_device *dsi)
+{
+	struct intel_dsi *intel_dsi = container_of(dsi, struct intel_dsi, dev);
+	struct drm_device *dev = intel_dsi->base.base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	char *sequence = dev_priv->vbt.dsi.sequence[MIPI_SEQ_DEASSERT_RESET];
+
+	generic_exec_sequence(intel_dsi, sequence);
+}
+
+static void generic_send_otp_cmds(struct intel_dsi_device *dsi)
+{
+	struct intel_dsi *intel_dsi = container_of(dsi, struct intel_dsi, dev);
+	struct drm_device *dev = intel_dsi->base.base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	char *sequence = dev_priv->vbt.dsi.sequence[MIPI_SEQ_INIT_OTP];
+
+	generic_exec_sequence(intel_dsi, sequence);
+}
+
+static void generic_enable(struct intel_dsi_device *dsi)
+{
+	struct intel_dsi *intel_dsi = container_of(dsi, struct intel_dsi, dev);
+	struct drm_device *dev = intel_dsi->base.base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	char *sequence = dev_priv->vbt.dsi.sequence[MIPI_SEQ_DISPLAY_ON];
+
+	generic_exec_sequence(intel_dsi, sequence);
+}
+
+static void generic_disable(struct intel_dsi_device *dsi)
+{
+	struct intel_dsi *intel_dsi = container_of(dsi, struct intel_dsi, dev);
+	struct drm_device *dev = intel_dsi->base.base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	char *sequence = dev_priv->vbt.dsi.sequence[MIPI_SEQ_DISPLAY_OFF];
+
+	generic_exec_sequence(intel_dsi, sequence);
+}
+
+static enum drm_connector_status generic_detect(struct intel_dsi_device *dsi)
+{
+	return connector_status_connected;
+}
+
+static bool generic_get_hw_state(struct intel_dsi_device *dev)
+{
+	return true;
+}
+
+static struct drm_display_mode *generic_get_modes(struct intel_dsi_device *dsi)
+{
+	struct intel_dsi *intel_dsi = container_of(dsi, struct intel_dsi, dev);
+	struct drm_device *dev = intel_dsi->base.base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	dev_priv->vbt.lfp_lvds_vbt_mode->type |= DRM_MODE_TYPE_PREFERRED;
+	return dev_priv->vbt.lfp_lvds_vbt_mode;
+}
+
+static void generic_destroy(struct intel_dsi_device *dsi) { }
+
+/* Callbacks. We might not need them all. */
+struct intel_dsi_dev_ops vbt_generic_dsi_display_ops = {
+	.init = generic_init,
+	.mode_valid = generic_mode_valid,
+	.mode_fixup = generic_mode_fixup,
+	.panel_reset = generic_panel_reset,
+	.disable_panel_power = generic_disable_panel_power,
+	.send_otp_cmds = generic_send_otp_cmds,
+	.enable = generic_enable,
+	.disable = generic_disable,
+	.detect = generic_detect,
+	.get_hw_state = generic_get_hw_state,
+	.get_modes = generic_get_modes,
+	.destroy = generic_destroy,
+};
-- 
1.8.3.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [v2] drm/i915: Add support for Generic MIPI panel driver

[Damien Lespiau]

On Fri, May 23, 2014 at 09:35:27PM +0530, Shobhit Kumar wrote:
> This driver makes use of the generic panel information from the VBT.
> Panel information is classified into two - panel configuration and panel
> power sequence which is unique to each panel. The generic driver uses the
> panel configuration and sequence parsed from VBT block #52 and #53
> 
> v2: Address review comments by Jani
>     - Move all of the things in driver c file from header
>     - Make all functions static
>     - Make use of video/mipi_display.c instead of redefining
>     - Null checks during sequence execution
> 
> v3: Address review comments by Damien
>     - Rename the panel driver file as intel_dsi_panel_vbt.c
>     - Fix style changes as suggested
>     - Correct comments for lp->hs and hs->lp count calculations
>     - General updating comments to have more clarity
>     - using max() instead of ternary operator
>     - Fix names (ui_num, ui_den) while using UI in calculations
>     - compute max of lp_to_hs switch and hs_to_lp switch while computing
>       hs_lp_switch_count
> 
> Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>

Hopefully still works after all that :)

Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>

There's a small style issue I noticed with a last peek at the patch.
> +static bool generic_mode_fixup(struct intel_dsi_device *dsi,
> +		    const struct drm_display_mode *mode,
> +		    struct drm_display_mode *adjusted_mode) {
> +	return true;
> +}

-- 
Damien

Re: [v2] drm/i915: Add support for Generic MIPI panel driver

[Kumar, Shobhit]

On 5/27/2014 4:32 PM, Damien Lespiau wrote:
> On Fri, May 23, 2014 at 09:35:27PM +0530, Shobhit Kumar wrote:
>> This driver makes use of the generic panel information from the VBT.
>> Panel information is classified into two - panel configuration and panel
>> power sequence which is unique to each panel. The generic driver uses the
>> panel configuration and sequence parsed from VBT block #52 and #53
>>
>> v2: Address review comments by Jani
>>      - Move all of the things in driver c file from header
>>      - Make all functions static
>>      - Make use of video/mipi_display.c instead of redefining
>>      - Null checks during sequence execution
>>
>> v3: Address review comments by Damien
>>      - Rename the panel driver file as intel_dsi_panel_vbt.c
>>      - Fix style changes as suggested
>>      - Correct comments for lp->hs and hs->lp count calculations
>>      - General updating comments to have more clarity
>>      - using max() instead of ternary operator
>>      - Fix names (ui_num, ui_den) while using UI in calculations
>>      - compute max of lp_to_hs switch and hs_to_lp switch while computing
>>        hs_lp_switch_count
>>
>> Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
>
> Hopefully still works after all that :)

It does, at least on AsusT100 :) Thanks for the review

Regards
Shobhit

>
> Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>
>
> There's a small style issue I noticed with a last peek at the patch.
>> +static bool generic_mode_fixup(struct intel_dsi_device *dsi,
>> +		    const struct drm_display_mode *mode,
>> +		    struct drm_display_mode *adjusted_mode) {
>> +	return true;
>> +}
>

Re: [v2] drm/i915: Add support for Generic MIPI panel driver

[Daniel Vetter]

On Tue, May 27, 2014 at 04:51:55PM +0530, Kumar, Shobhit wrote:
> On 5/27/2014 4:32 PM, Damien Lespiau wrote:
> >On Fri, May 23, 2014 at 09:35:27PM +0530, Shobhit Kumar wrote:
> >>This driver makes use of the generic panel information from the VBT.
> >>Panel information is classified into two - panel configuration and panel
> >>power sequence which is unique to each panel. The generic driver uses the
> >>panel configuration and sequence parsed from VBT block #52 and #53
> >>
> >>v2: Address review comments by Jani
> >>     - Move all of the things in driver c file from header
> >>     - Make all functions static
> >>     - Make use of video/mipi_display.c instead of redefining
> >>     - Null checks during sequence execution
> >>
> >>v3: Address review comments by Damien
> >>     - Rename the panel driver file as intel_dsi_panel_vbt.c
> >>     - Fix style changes as suggested
> >>     - Correct comments for lp->hs and hs->lp count calculations
> >>     - General updating comments to have more clarity
> >>     - using max() instead of ternary operator
> >>     - Fix names (ui_num, ui_den) while using UI in calculations
> >>     - compute max of lp_to_hs switch and hs_to_lp switch while computing
> >>       hs_lp_switch_count
> >>
> >>Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
> >
> >Hopefully still works after all that :)
> 
> It does, at least on AsusT100 :) Thanks for the review

Queued for -next, thanks for the patch.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [v2] drm/i915: Add support for Generic MIPI panel driver

[Daniel Vetter]

On Tue, May 27, 2014 at 04:51:55PM +0530, Kumar, Shobhit wrote:
> On 5/27/2014 4:32 PM, Damien Lespiau wrote:
> >On Fri, May 23, 2014 at 09:35:27PM +0530, Shobhit Kumar wrote:
> >>This driver makes use of the generic panel information from the VBT.
> >>Panel information is classified into two - panel configuration and panel
> >>power sequence which is unique to each panel. The generic driver uses the
> >>panel configuration and sequence parsed from VBT block #52 and #53
> >>
> >>v2: Address review comments by Jani
> >>     - Move all of the things in driver c file from header
> >>     - Make all functions static
> >>     - Make use of video/mipi_display.c instead of redefining
> >>     - Null checks during sequence execution
> >>
> >>v3: Address review comments by Damien
> >>     - Rename the panel driver file as intel_dsi_panel_vbt.c
> >>     - Fix style changes as suggested
> >>     - Correct comments for lp->hs and hs->lp count calculations
> >>     - General updating comments to have more clarity
> >>     - using max() instead of ternary operator
> >>     - Fix names (ui_num, ui_den) while using UI in calculations
> >>     - compute max of lp_to_hs switch and hs_to_lp switch while computing
> >>       hs_lp_switch_count
> >>
> >>Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
> >
> >Hopefully still works after all that :)
> 
> It does, at least on AsusT100 :) Thanks for the review

Aside: checkpatch.pl complained a bit about style issues in your patch
(like unpretty alignment of continuation lines and stuff like that).
Please feed your patches to checkpatch if you don't have an editor that
simply gets this right.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [v2] drm/i915: Add support for Generic MIPI panel driver

[Kumar, Shobhit]

On 5/27/2014 5:09 PM, Daniel Vetter wrote:
> On Tue, May 27, 2014 at 04:51:55PM +0530, Kumar, Shobhit wrote:
>> On 5/27/2014 4:32 PM, Damien Lespiau wrote:
>>> On Fri, May 23, 2014 at 09:35:27PM +0530, Shobhit Kumar wrote:
>>>> This driver makes use of the generic panel information from the VBT.
>>>> Panel information is classified into two - panel configuration and panel
>>>> power sequence which is unique to each panel. The generic driver uses the
>>>> panel configuration and sequence parsed from VBT block #52 and #53
>>>>
>>>> v2: Address review comments by Jani
>>>>      - Move all of the things in driver c file from header
>>>>      - Make all functions static
>>>>      - Make use of video/mipi_display.c instead of redefining
>>>>      - Null checks during sequence execution
>>>>
>>>> v3: Address review comments by Damien
>>>>      - Rename the panel driver file as intel_dsi_panel_vbt.c
>>>>      - Fix style changes as suggested
>>>>      - Correct comments for lp->hs and hs->lp count calculations
>>>>      - General updating comments to have more clarity
>>>>      - using max() instead of ternary operator
>>>>      - Fix names (ui_num, ui_den) while using UI in calculations
>>>>      - compute max of lp_to_hs switch and hs_to_lp switch while computing
>>>>        hs_lp_switch_count
>>>>
>>>> Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
>>>
>>> Hopefully still works after all that :)
>>
>> It does, at least on AsusT100 :) Thanks for the review
>
> Aside: checkpatch.pl complained a bit about style issues in your patch
> (like unpretty alignment of continuation lines and stuff like that).
> Please feed your patches to checkpatch if you don't have an editor that
> simply gets this right.

Hmm, I will check my vim config and be more careful next time. Thanks 
for pointing out. Have you taken care of these or I will push another 
patch to fix the alignment issues ?

Regards
Shobhit

> -Daniel
>

Re: [v2] drm/i915: Add support for Generic MIPI panel driver

[Daniel Vetter]

On Tue, May 27, 2014 at 1:42 PM, Kumar, Shobhit <shobhit.kumar@intel.com> wrote:
> On 5/27/2014 5:09 PM, Daniel Vetter wrote:
>>
>> On Tue, May 27, 2014 at 04:51:55PM +0530, Kumar, Shobhit wrote:
>>>
>>> On 5/27/2014 4:32 PM, Damien Lespiau wrote:
>>>>
>>>> On Fri, May 23, 2014 at 09:35:27PM +0530, Shobhit Kumar wrote:
>>>>>
>>>>> This driver makes use of the generic panel information from the VBT.
>>>>> Panel information is classified into two - panel configuration and
>>>>> panel
>>>>> power sequence which is unique to each panel. The generic driver uses
>>>>> the
>>>>> panel configuration and sequence parsed from VBT block #52 and #53
>>>>>
>>>>> v2: Address review comments by Jani
>>>>>      - Move all of the things in driver c file from header
>>>>>      - Make all functions static
>>>>>      - Make use of video/mipi_display.c instead of redefining
>>>>>      - Null checks during sequence execution
>>>>>
>>>>> v3: Address review comments by Damien
>>>>>      - Rename the panel driver file as intel_dsi_panel_vbt.c
>>>>>      - Fix style changes as suggested
>>>>>      - Correct comments for lp->hs and hs->lp count calculations
>>>>>      - General updating comments to have more clarity
>>>>>      - using max() instead of ternary operator
>>>>>      - Fix names (ui_num, ui_den) while using UI in calculations
>>>>>      - compute max of lp_to_hs switch and hs_to_lp switch while
>>>>> computing
>>>>>        hs_lp_switch_count
>>>>>
>>>>> Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
>>>>
>>>>
>>>> Hopefully still works after all that :)
>>>
>>>
>>> It does, at least on AsusT100 :) Thanks for the review
>>
>>
>> Aside: checkpatch.pl complained a bit about style issues in your patch
>> (like unpretty alignment of continuation lines and stuff like that).
>> Please feed your patches to checkpatch if you don't have an editor that
>> simply gets this right.
>
>
> Hmm, I will check my vim config and be more careful next time. Thanks for
> pointing out. Have you taken care of these or I will push another patch to
> fix the alignment issues ?

Yeah, a quick fixup patch would be pretty. For the vim configuration I
highly recommend to use

set cinoptions=:0,(0 " no indent for case and align function arguments to (

Then you can realign function parameter lists or multi-line if
condiditions with the = command, e.g. =a(

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [v2] drm/i915: Add support for Generic MIPI panel driver

[Damien Lespiau]

On Tue, May 27, 2014 at 01:51:22PM +0200, Daniel Vetter wrote:
> > Hmm, I will check my vim config and be more careful next time. Thanks for
> > pointing out. Have you taken care of these or I will push another patch to
> > fix the alignment issues ?
> 
> Yeah, a quick fixup patch would be pretty. For the vim configuration I
> highly recommend to use
> 
> set cinoptions=:0,(0 " no indent for case and align function arguments to (
> 
> Then you can realign function parameter lists or multi-line if
> condiditions with the = command, e.g. =a(

FWIW, this is what I use:

set cinoptions=:0,t0,(0,u0,w1,m1

-- 
Damien

[PATCH] drm/i915: Fix checkpatch errors

[Shobhit Kumar]

Fix warnings introduced by the following commit -

commit 9c92da2c7c17eea79b6321b37592df0a002d24df
Author: Shobhit Kumar <shobhit.kumar@intel.com>
Date:   Fri May 23 21:35:27 2014 +0530

    drm/i915: Add support for Generic MIPI panel driver

Fixed all except the DRM logging which go beyond line 80

Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
---
 drivers/gpu/drm/i915/intel_dsi_panel_vbt.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
index 21a0d34..47c7584 100644
--- a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
+++ b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
@@ -143,7 +143,7 @@ static u8 *mipi_exec_send_packet(struct intel_dsi *intel_dsi, u8 *data)
 	case MIPI_DSI_DCS_LONG_WRITE:
 		dsi_vc_dcs_write(intel_dsi, vc, data, len);
 		break;
-	};
+	}
 
 	data += len;
 
@@ -294,7 +294,8 @@ static bool generic_init(struct intel_dsi_device *dsi)
 	intel_dsi->rst_timer_val = mipi_config->device_reset_timer;
 	intel_dsi->init_count = mipi_config->master_init_timer;
 	intel_dsi->bw_timer = mipi_config->dbi_bw_timer;
-	intel_dsi->video_frmt_cfg_bits = mipi_config->bta_enabled ? DISABLE_VIDEO_BTA : 0;
+	intel_dsi->video_frmt_cfg_bits =
+		mipi_config->bta_enabled ? DISABLE_VIDEO_BTA : 0;
 
 	switch (intel_dsi->escape_clk_div) {
 	case 0:
@@ -351,7 +352,8 @@ static bool generic_init(struct intel_dsi_device *dsi)
 	 *
 	 * prepare count
 	 */
-	ths_prepare_ns = max(mipi_config->ths_prepare, mipi_config->tclk_prepare);
+	ths_prepare_ns = max(mipi_config->ths_prepare,
+			     mipi_config->tclk_prepare);
 	prepare_cnt = DIV_ROUND_UP(ths_prepare_ns * ui_den, ui_num * 2);
 
 	/* exit zero count */
-- 
1.8.3.2

Re: [PATCH] drm/i915: Fix checkpatch errors

[Damien Lespiau]

On Tue, May 27, 2014 at 07:23:46PM +0530, Shobhit Kumar wrote:
> Fix warnings introduced by the following commit -
> 
> commit 9c92da2c7c17eea79b6321b37592df0a002d24df
> Author: Shobhit Kumar <shobhit.kumar@intel.com>
> Date:   Fri May 23 21:35:27 2014 +0530
> 
>     drm/i915: Add support for Generic MIPI panel driver
> 
> Fixed all except the DRM logging which go beyond line 80
> 
> Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>

To squash into previous commit?

Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>

-- 
Damien

Re: [PATCH] drm/i915: Fix checkpatch errors

[Kumar, Shobhit]

On 5/27/2014 7:49 PM, Damien Lespiau wrote:
> On Tue, May 27, 2014 at 07:23:46PM +0530, Shobhit Kumar wrote:
>> Fix warnings introduced by the following commit -
>>
>> commit 9c92da2c7c17eea79b6321b37592df0a002d24df
>> Author: Shobhit Kumar <shobhit.kumar@intel.com>
>> Date:   Fri May 23 21:35:27 2014 +0530
>>
>>      drm/i915: Add support for Generic MIPI panel driver
>>
>> Fixed all except the DRM logging which go beyond line 80
>>
>> Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
>
> To squash into previous commit?

Can be done. I am not sure how this is handled for queued patches

Regards
Shobhit

Re: [PATCH] drm/i915: Fix checkpatch errors

[Damien Lespiau]

On Tue, May 27, 2014 at 08:04:43PM +0530, Kumar, Shobhit wrote:
> On 5/27/2014 7:49 PM, Damien Lespiau wrote:
> >On Tue, May 27, 2014 at 07:23:46PM +0530, Shobhit Kumar wrote:
> >>Fix warnings introduced by the following commit -
> >>
> >>commit 9c92da2c7c17eea79b6321b37592df0a002d24df
> >>Author: Shobhit Kumar <shobhit.kumar@intel.com>
> >>Date:   Fri May 23 21:35:27 2014 +0530
> >>
> >>     drm/i915: Add support for Generic MIPI panel driver
> >>
> >>Fixed all except the DRM logging which go beyond line 80
> >>
> >>Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
> >
> >To squash into previous commit?
> 
> Can be done. I am not sure how this is handled for queued patches

Daniel does a fair bit of this kind of editing when applying patches.

-- 
Damien

Re: [PATCH] drm/i915: Fix checkpatch errors

[Daniel Vetter]

On Tue, May 27, 2014 at 03:39:37PM +0100, Damien Lespiau wrote:
> On Tue, May 27, 2014 at 08:04:43PM +0530, Kumar, Shobhit wrote:
> > On 5/27/2014 7:49 PM, Damien Lespiau wrote:
> > >On Tue, May 27, 2014 at 07:23:46PM +0530, Shobhit Kumar wrote:
> > >>Fix warnings introduced by the following commit -
> > >>
> > >>commit 9c92da2c7c17eea79b6321b37592df0a002d24df
> > >>Author: Shobhit Kumar <shobhit.kumar@intel.com>
> > >>Date:   Fri May 23 21:35:27 2014 +0530
> > >>
> > >>     drm/i915: Add support for Generic MIPI panel driver
> > >>
> > >>Fixed all except the DRM logging which go beyond line 80
> > >>
> > >>Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com>
> > >
> > >To squash into previous commit?
> > 
> > Can be done. I am not sure how this is handled for queued patches
> 
> Daniel does a fair bit of this kind of editing when applying patches.

I think I'll keep this one separate, as a cautionary tale ;-)

Queued for -next, thanks for the patch.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch