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@xxxxxxxxx> > >> Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > >> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx> > >> --- > >> 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@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel