dri-devel.lists.freedesktop.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2] drm/i915/dsi: Fix swapping of MIPI_SEQ_DEASSERT_RESET / MIPI_SEQ_ASSERT_RESET
@ 2016-12-02 15:01 Hans de Goede
  2016-12-07 17:44 ` Ville Syrjälä
  0 siblings, 1 reply; 2+ messages in thread
From: Hans de Goede @ 2016-12-02 15:01 UTC (permalink / raw)
  To: Daniel Vetter, Jani Nikula, Ville Syrjälä
  Cc: Jani Nikula, Hans de Goede, intel-gfx, dri-devel

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@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Acked-by: Jani Nikula <jani.nikula@intel.com>
---
Changes in v2:
-Add a comment to the enum explaining that the assert/reassert names
 are swapped in the spec
---
 drivers/gpu/drm/i915/intel_bios.h          | 12 +++++++++---
 drivers/gpu/drm/i915/intel_dsi_panel_vbt.c |  4 ++--
 2 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_bios.h b/drivers/gpu/drm/i915/intel_bios.h
index 8405b5a..7e3545f 100644
--- a/drivers/gpu/drm/i915/intel_bios.h
+++ b/drivers/gpu/drm/i915/intel_bios.h
@@ -46,14 +46,20 @@ struct edp_power_seq {
 	u16 t11_t12;
 } __packed;
 
-/* MIPI Sequence Block definitions */
+/*
+ * MIPI Sequence Block definitions
+ *
+ * Note the VBT spec has AssertReset / DeassertReset swapped from their
+ * usual naming, we use the proper names here to avoid confusion when
+ * reading the code.
+ */
 enum mipi_seq {
 	MIPI_SEQ_END = 0,
-	MIPI_SEQ_ASSERT_RESET,
+	MIPI_SEQ_DEASSERT_RESET,	/* Spec says MipiAssertResetPin */
 	MIPI_SEQ_INIT_OTP,
 	MIPI_SEQ_DISPLAY_ON,
 	MIPI_SEQ_DISPLAY_OFF,
-	MIPI_SEQ_DEASSERT_RESET,
+	MIPI_SEQ_ASSERT_RESET,		/* Spec says MipiDeassertResetPin */
 	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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 2+ messages in thread
* Re: [PATCH v2] drm/i915/dsi: Fix swapping of MIPI_SEQ_DEASSERT_RESET / MIPI_SEQ_ASSERT_RESET
  2016-12-02 15:01 [PATCH v2] drm/i915/dsi: Fix swapping of MIPI_SEQ_DEASSERT_RESET / MIPI_SEQ_ASSERT_RESET Hans de Goede
@ 2016-12-07 17:44 ` Ville Syrjälä
  0 siblings, 0 replies; 2+ messages in thread
From: Ville Syrjälä @ 2016-12-07 17:44 UTC (permalink / raw)
  To: Hans de Goede; +Cc: Jani Nikula, Daniel Vetter, intel-gfx, dri-devel

On Fri, Dec 02, 2016 at 04:01:28PM +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@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> Acked-by: Jani Nikula <jani.nikula@intel.com>
> ---
> Changes in v2:
> -Add a comment to the enum explaining that the assert/reassert names
>  are swapped in the spec

Pushed to dinq. Thanks for the patch.

I sucked in the changelog again. In the future please include it in
the actual commit message, cause that's how we like it.

> ---
>  drivers/gpu/drm/i915/intel_bios.h          | 12 +++++++++---
>  drivers/gpu/drm/i915/intel_dsi_panel_vbt.c |  4 ++--
>  2 files changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_bios.h b/drivers/gpu/drm/i915/intel_bios.h
> index 8405b5a..7e3545f 100644
> --- a/drivers/gpu/drm/i915/intel_bios.h
> +++ b/drivers/gpu/drm/i915/intel_bios.h
> @@ -46,14 +46,20 @@ struct edp_power_seq {
>  	u16 t11_t12;
>  } __packed;
>  
> -/* MIPI Sequence Block definitions */
> +/*
> + * MIPI Sequence Block definitions
> + *
> + * Note the VBT spec has AssertReset / DeassertReset swapped from their
> + * usual naming, we use the proper names here to avoid confusion when
> + * reading the code.
> + */
>  enum mipi_seq {
>  	MIPI_SEQ_END = 0,
> -	MIPI_SEQ_ASSERT_RESET,
> +	MIPI_SEQ_DEASSERT_RESET,	/* Spec says MipiAssertResetPin */
>  	MIPI_SEQ_INIT_OTP,
>  	MIPI_SEQ_DISPLAY_ON,
>  	MIPI_SEQ_DISPLAY_OFF,
> -	MIPI_SEQ_DEASSERT_RESET,
> +	MIPI_SEQ_ASSERT_RESET,		/* Spec says MipiDeassertResetPin */
>  	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

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 2+ messages in thread
end of thread, other threads:[~2016-12-07 17:44 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-12-02 15:01 [PATCH v2] drm/i915/dsi: Fix swapping of MIPI_SEQ_DEASSERT_RESET / MIPI_SEQ_ASSERT_RESET Hans de Goede
2016-12-07 17:44 ` Ville Syrjälä

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).