Re: [PATCH 01/18] drm/i915/dsi: Fix swapping of MIPI_SEQ_DEASSERT_RESET / MIPI_SEQ_ASSERT_RESET

From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: Hans de Goede <hdegoede@redhat.com>
Cc: Jani Nikula <jani.nikula@intel.com>,
	Daniel Vetter <daniel.vetter@intel.com>,
	intel-gfx <intel-gfx@lists.freedesktop.org>,
	dri-devel@lists.freedesktop.org
Subject: Re: [PATCH 01/18] drm/i915/dsi: Fix swapping of MIPI_SEQ_DEASSERT_RESET / MIPI_SEQ_ASSERT_RESET
Date: Fri, 2 Dec 2016 15:45:50 +0200	[thread overview]
Message-ID: <20161202134550.GY31595@intel.com> (raw)
In-Reply-To: <2dc9a727-a6c2-cc67-f133-04c6d1dc291c@redhat.com>

On Fri, Dec 02, 2016 at 02:34:01PM +0100, Hans de Goede wrote:
> 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@intel.com>
> >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >> Signed-off-by: Hans de Goede <hdegoede@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 ?

Sure, I could live with that. We definitely need to have that
comment somewhere.

> 
> 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
> >

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