[Intel-gfx] [PATCH 01/18] drm/i915/dsi: Fix swapping of MIPI_SEQ_DEASSERT_RESET / MIPI_SEQ_ASSERT_RESET

[Hans de Goede]

Hi,

On 02-12-16 13:37, Ville Syrjälä wrote:
> On Thu, Dec 01, 2016 at 09:29:08PM +0100, Hans de Goede wrote:
>> Looking at the ADF code from the Android kernel sources for a
>> cherrytrail tablet I noticed that it is calling the
>> MIPI_SEQ_ASSERT_RESET sequence from the panel prepare hook.
>>
>> Until commit b1cb1bd29189 ("drm/i915/dsi: update reset and power sequences
>> in panel prepare/unprepare hooks") the mainline i915 code was doing the
>> same. That commits effectively swaps the calling of MIPI_SEQ_ASSERT_RESET /
>> MIPI_SEQ_DEASSERT_RESET.
>>
>> Looking at the naming of the sequences that is the right thing to do,
>> but the problem is, that the old mainline code and the ADF code was
>> actually calling the right sequence (tested on a cube iwork8 air tablet),
>> and the swapping of the calling breaks things.
>>
>> This breakage was likely not noticed in testing because on cherrytrail,
>> currently chv_exec_gpio ends up disabling the gpio pins rather then
>> setting them (this is fixed in the next patch in this patch-set).
>>
>> This commit fixes the swapping by fixing MIPI_SEQ_ASSERT/DEASSERT_RESET's
>> places in the enum defining them, so that their (new) names match their
>> actual use.
>>
>> Fixes: b1cb1bd29189 ("drm/i915/dsi: update reset and power sequences...")
>> Cc: Jani Nikula <jani.nikula at intel.com>
>> Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
>> Signed-off-by: Hans de Goede <hdegoede at redhat.com>
>> ---
>>  drivers/gpu/drm/i915/intel_bios.h          | 4 ++--
>>  drivers/gpu/drm/i915/intel_dsi_panel_vbt.c | 4 ++--
>>  2 files changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_bios.h b/drivers/gpu/drm/i915/intel_bios.h
>> index 8405b5a..642a5eb 100644
>> --- a/drivers/gpu/drm/i915/intel_bios.h
>> +++ b/drivers/gpu/drm/i915/intel_bios.h
>> @@ -49,11 +49,11 @@ struct edp_power_seq {
>>  /* MIPI Sequence Block definitions */
>>  enum mipi_seq {
>>  	MIPI_SEQ_END = 0,
>> -	MIPI_SEQ_ASSERT_RESET,
>> +	MIPI_SEQ_DEASSERT_RESET,
>>  	MIPI_SEQ_INIT_OTP,
>>  	MIPI_SEQ_DISPLAY_ON,
>>  	MIPI_SEQ_DISPLAY_OFF,
>> -	MIPI_SEQ_DEASSERT_RESET,
>> +	MIPI_SEQ_ASSERT_RESET,
>
> I think we'll still want to keep to the names as they are in the
> spec, and instead we'll just call them in the order that looks
> wrong + add a comment explaining why we do that.

I really want the i915 driver to use the correct names, anything
else will lead to no amount of confusion for people who have
experience with embedded stuff.

How about adding a comment here in the enum, as well as in
patch 8: "drm/i915/dsi: Document the panel enable / disable sequences from the spec"
That the spec has assert / deassert the wrong way around and that
the i915 code is using the correct names ?

Most people will not even have access to the spec, so it seems to
me that having this right in the code, with a comment to warn
people who do have access to the spec is better then the other
way around.

Regards,

Hans



>
>>  	MIPI_SEQ_BACKLIGHT_ON,		/* sequence block v2+ */
>>  	MIPI_SEQ_BACKLIGHT_OFF,		/* sequence block v2+ */
>>  	MIPI_SEQ_TEAR_ON,		/* sequence block v2+ */
>> diff --git a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
>> index 0d8ff00..579d2f5 100644
>> --- a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
>> +++ b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c
>> @@ -376,11 +376,11 @@ static const fn_mipi_elem_exec exec_elem[] = {
>>   */
>>
>>  static const char * const seq_name[] = {
>> -	[MIPI_SEQ_ASSERT_RESET] = "MIPI_SEQ_ASSERT_RESET",
>> +	[MIPI_SEQ_DEASSERT_RESET] = "MIPI_SEQ_DEASSERT_RESET",
>>  	[MIPI_SEQ_INIT_OTP] = "MIPI_SEQ_INIT_OTP",
>>  	[MIPI_SEQ_DISPLAY_ON] = "MIPI_SEQ_DISPLAY_ON",
>>  	[MIPI_SEQ_DISPLAY_OFF]  = "MIPI_SEQ_DISPLAY_OFF",
>> -	[MIPI_SEQ_DEASSERT_RESET] = "MIPI_SEQ_DEASSERT_RESET",
>> +	[MIPI_SEQ_ASSERT_RESET] = "MIPI_SEQ_ASSERT_RESET",
>>  	[MIPI_SEQ_BACKLIGHT_ON] = "MIPI_SEQ_BACKLIGHT_ON",
>>  	[MIPI_SEQ_BACKLIGHT_OFF] = "MIPI_SEQ_BACKLIGHT_OFF",
>>  	[MIPI_SEQ_TEAR_ON] = "MIPI_SEQ_TEAR_ON",
>> --
>> 2.9.3
>