Re: [PATCH 4/4] drm/i915: Parameterize the MIPI enabling sequnece and adjust the sequence

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Tue, Oct 22, 2013 at 02:36:18PM +0530, Shobhit Kumar wrote:
> On 10/21/2013 6:53 PM, Ville Syrjälä 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).

> 
> >
> > 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.

-- 
Ville Syrjälä
Intel OTC