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

Shobhit Kumar shobhit.kumar at intel.com
Wed Oct 23 14:57:21 CEST 2013

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 -
>> And during disable time to flush all FIFOs -
>> 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:
> 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.



More information about the Intel-gfx mailing list