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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
