From mboxrd@z Thu Jan 1 00:00:00 1970 From: Shobhit Kumar Subject: Re: [PATCH 4/4] drm/i915: Parameterize the MIPI enabling sequnece and adjust the sequence Date: Wed, 23 Oct 2013 18:27:21 +0530 Message-ID: <5267C7B1.8080307@intel.com> References: <1382358067-5578-1-git-send-email-shobhit.kumar@intel.com> <1382358067-5578-5-git-send-email-shobhit.kumar@intel.com> <20131021132348.GX13047@intel.com> <5266400A.3010708@intel.com> <20131022104915.GF13047@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1"; Format="flowed" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by gabe.freedesktop.org (Postfix) with ESMTP id 1775CE5F32 for ; Wed, 23 Oct 2013 05:49:50 -0700 (PDT) In-Reply-To: <20131022104915.GF13047@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: =?ISO-8859-1?Q?Ville_Syrj=E4l=E4?= Cc: jani.nikula@intel.com, vijayakumar.balakrishnan@intel.com, intel-gfx , yogesh.mohan.marimuthu@intel.com List-Id: intel-gfx@lists.freedesktop.org On 10/22/2013 04:19 PM, Ville Syrj=E4l=E4 wrote: > On Tue, Oct 22, 2013 at 02:36:18PM +0530, Shobhit Kumar wrote: >> On 10/21/2013 6:53 PM, Ville Syrj=E4l=E4 wrote: >>> On Mon, Oct 21, 2013 at 05:51:07PM +0530, Shobhit Kumar wrote: >>>> Has been tested on couple of panels now. >>> >>> While it's nice to get patches, I can't say I'm very happy about the >>> shape of this one. >>> >>> The patch contains several changes in one patch. It should be split up >>> into several patches. Based on a cursory examination I would suggest >>> something like this: >>> - weird ULPS ping-pong >> >> Suggested and approved sequence by HW team for ULPS entry/exit is as >> follows during enable time - >> set DEVICE_READY --> Clear DEVICE_READY --> set DEVICE_READY >> >> And during disable time to flush all FIFOs - >> set ENTER_SLEEP --> EXIT_SLEEP --> ENTER_SLEEP >> >> I will push this is new patch >> >>> - add backlight support >> >> Ok will push in new patch >> >>> - moving the ->disable() call >> >> Earlier disable was called at the beginning even before pixel stream was >> stopped. Ideal flow would be to disable pixel stream and then follow >> panel's required disable sequence >> >>> - each of the new intel_dsi->foo/bar/etc. parameter could probably >>> be a separate patch >> >> Ok, I can break all parameter changes into a separate patch >> >>> >>> As far as the various timeout related parameters are concerned, to me >>> it would make more sense to specify them in usecs or some other real >>> world unit. Or you could provide/leave in some helper functions to >>> calculate the clock based values from some real world values. >> >> Few timeouts are as per spec. Are you referring to back-light or >> shutdown packet delays ? If yes we can change them to usecs. > > These at least: > MIPI_LP_RX_TIMEOUT > MIPI_TURN_AROUND_TIMEOUT > MIPI_DEVICE_RESET_TIMER > MIPI_INIT_COUNT > MIPI_HIGH_LOW_SWITCH_COUNT > > It's been a while since I read the spec so I don't remember anymore how > all those were specified there. If the spec defines them in some clocks, > then that would be the best choice, but if they're specified in some > time units, then I would possibly follow that. At the very least you > should add some documentation about the units in the intel_dsi struct > (or whereever we expect these things to live). > Ok, got it. All the above are defined as byte clocks. Will take care as = per your suggestions. >> >>> >>> And finally justficiation for each of these changes is missing from >>> the current patch. We want to know why the code has to change. >> >> I hope I have provided some clarifications above. I will work on >> splitting this patch into few more patches for more clarity. > > Yeah, looks like good stuff. Looking forward to seeing the split up > patches. Thanks. > WIP Regards Shobhit