From mboxrd@z Thu Jan 1 00:00:00 1970 From: Shobhit Kumar Subject: Re: [PATCH 1/4] drm/i915: Add more dev ops for MIPI sub encoder Date: Tue, 22 Oct 2013 15:09:02 +0530 Message-ID: <526647B6.9000903@intel.com> References: <1382358067-5578-1-git-send-email-shobhit.kumar@intel.com> <1382358067-5578-2-git-send-email-shobhit.kumar@intel.com> <874n8a4uv2.fsf@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by gabe.freedesktop.org (Postfix) with ESMTP id 6D7C8E6AC1 for ; Tue, 22 Oct 2013 02:39:05 -0700 (PDT) In-Reply-To: <874n8a4uv2.fsf@intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org Errors-To: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org To: Jani Nikula Cc: vijayakumar.balakrishnan@intel.com, intel-gfx , yogesh.mohan.marimuthu@intel.com List-Id: intel-gfx@lists.freedesktop.org On 10/21/2013 6:57 PM, Jani Nikula wrote: > > Hi Shobhit - > > On Mon, 21 Oct 2013, Shobhit Kumar wrote: >> Also add new fields in intel_dsi to have all dphy related parameters. >> These will be useful even when we go for pure generic MIPI design > > I feel like we have a different idea of what the ideal generic design > is. For me, the goal is that we change the struct intel_dsi_device to > struct drm_bridge, and those drm_bridge drivers are all about the panel, > and have as few details about i915 or our hardware as possible. Having > the bridge drivers fill in register values to be written by the core DSI > code does not fit that. Yes, I had some of those in my bridge conversion > patches too, but I did not intend we'd keep adding more. > > I'd rather we provide generic helpers the bridge driver can call. Yeah, look like our ideas are different. In your goal with drm_bridge architecture, we will still end up having multiple bridge drivers for each different panel. But my goal is to have a single driver which can work for multiple panels. Since we already have enabled some panels with sub-encoder architecture for completeness I was planning to maintain generic driver as one sub-encoder. But actually we can do away with all sub-encoder and do not need even drm_bridge and all implementation will be in intel_dsi.c. Panel specific configurations or sequences will come from VBT which I have tried to convert as parameters. All the parameters DSI/DPHY spec specific and none of them particularly relate to our hardware. > >> Yogesh Mohan Marimuthu >> Signed-off-by: Shobhit Kumar >> --- >> drivers/gpu/drm/i915/intel_dsi.c | 9 ++++++++- >> drivers/gpu/drm/i915/intel_dsi.h | 29 +++++++++++++++++++++++++++++ >> 2 files changed, 37 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c >> index 9a2fdd2..34e19b7 100644 >> --- a/drivers/gpu/drm/i915/intel_dsi.c >> +++ b/drivers/gpu/drm/i915/intel_dsi.c >> @@ -147,6 +147,9 @@ static void intel_dsi_enable(struct intel_encoder *encoder) >> >> DRM_DEBUG_KMS("\n"); >> >> + if (intel_dsi->dev.dev_ops->panel_reset) >> + intel_dsi->dev.dev_ops->panel_reset(&intel_dsi->dev); > > Would this map to ->pre_enable in drm_bridge? I have not yet migrated to drm_bridge and need to check thes call flows for drm_bridge > >> + >> temp = I915_READ(MIPI_DEVICE_READY(pipe)); >> if ((temp & DEVICE_READY) == 0) { >> temp &= ~ULPS_STATE_MASK; >> @@ -162,6 +165,9 @@ static void intel_dsi_enable(struct intel_encoder *encoder) >> I915_WRITE(MIPI_DEVICE_READY(pipe), temp); >> } >> >> + if (intel_dsi->dev.dev_ops->send_otp_cmds) >> + intel_dsi->dev.dev_ops->send_otp_cmds(&intel_dsi->dev); > > What is otp? one time programming? Why not in ->enable? Yes. OTP is done before sending pixel stream and enable is done after we start pixel stream > >> + >> if (is_cmd_mode(intel_dsi)) >> I915_WRITE(MIPI_MAX_RETURN_PKT_SIZE(pipe), 8 * 4); >> >> @@ -176,7 +182,8 @@ static void intel_dsi_enable(struct intel_encoder *encoder) >> POSTING_READ(MIPI_PORT_CTRL(pipe)); >> } >> >> - intel_dsi->dev.dev_ops->enable(&intel_dsi->dev); >> + if (intel_dsi->dev.dev_ops->enable) >> + intel_dsi->dev.dev_ops->enable(&intel_dsi->dev); >> } >> >> static void intel_dsi_disable(struct intel_encoder *encoder) >> diff --git a/drivers/gpu/drm/i915/intel_dsi.h b/drivers/gpu/drm/i915/intel_dsi.h >> index c7765f3..b71c9b3 100644 >> --- a/drivers/gpu/drm/i915/intel_dsi.h >> +++ b/drivers/gpu/drm/i915/intel_dsi.h >> @@ -39,6 +39,13 @@ struct intel_dsi_device { >> struct intel_dsi_dev_ops { >> bool (*init)(struct intel_dsi_device *dsi); >> >> + void (*panel_reset)(struct intel_dsi_device *dsi); >> + >> + void (*disable_panel_power)(struct intel_dsi_device *dsi); > > What is the enabling counterpart to disable_panel_power? panel_reset? Yes. > >> + >> + /* send one time programmable commands */ >> + void (*send_otp_cmds)(struct intel_dsi_device *dsi); >> + >> /* This callback must be able to assume DSI commands can be sent */ >> void (*enable)(struct intel_dsi_device *dsi); >> >> @@ -89,6 +96,28 @@ struct intel_dsi { >> >> /* eot for MIPI_EOT_DISABLE register */ >> u32 eot_disable; >> + >> + u16 dsi_clock_freq; >> + u8 video_mode_type; >> + u32 data_width; >> + u8 dither; >> + u32 port_bits; >> + u8 escape_clk_div; >> + u32 lp_rx_timeout; >> + u8 turn_arnd_val; >> + u16 init_count; >> + u16 rst_timer_val; >> + u16 hs_to_lp_count; >> + u16 lp_byte_clk; >> + u32 bw_timer; >> + u16 clk_lp_to_hs_count; >> + u16 clk_hs_to_lp_count; >> + u32 video_frmt_cfg_bits; >> + u32 dphy_reg; >> + >> + u8 backlight_off_delay; /*in ms*/ >> + bool send_shutdown; >> + u8 shutdown_pkt_delay; /*in ms*/ >> }; >> >> static inline struct intel_dsi *enc_to_intel_dsi(struct drm_encoder *encoder) >> -- >> 1.7.9.5 >> > Regards Shobhit