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

[Shobhit Kumar <shobhit.kumar@intel.com>]

On 10/22/2013 04:19 PM, Ville Syrjälä wrote:
> 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).
>

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