On Sun, Nov 01, 2015 at 04:33:57PM +0100, Lukas Wunner wrote:
> Hi Ville,
>
> On Fri, Sep 18, 2015 at 08:03:45PM +0300, ville.syrjala@xxxxxxxxxxxxxxx wrote:
> > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> >
> > Keep single 'lvds_reg' and 'lvds' variable around in
> > intel_lvds_init(), and read it just once at the start.
>
> Hm, is it intentional that you didn't also replace this register readout
> at the end of the function?
>
> lvds_encoder->a3_power = I915_READ(lvds_encoder->reg) &
> LVDS_A3_POWER_MASK;
Simply didn't notice that bit.
>
> (Sorry, I only noticed this today, post-merge, while rebasing my LVDS
> reprobing stuff.)
>
> Best regards,
>
> Lukas
>
> >
> > Also intel_lvds_get_config() doesn't need to figure out which reg to use
> > since it can just consult lvds_encoder->reg.
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/intel_lvds.c | 30 ++++++++++++++----------------
> > 1 file changed, 14 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
> > index 2c2d1f0..35bad71 100644
> > --- a/drivers/gpu/drm/i915/intel_lvds.c
> > +++ b/drivers/gpu/drm/i915/intel_lvds.c
> > @@ -98,15 +98,11 @@ static void intel_lvds_get_config(struct intel_encoder *encoder,
> > {
> > struct drm_device *dev = encoder->base.dev;
> > struct drm_i915_private *dev_priv = dev->dev_private;
> > - u32 lvds_reg, tmp, flags = 0;
> > + struct intel_lvds_encoder *lvds_encoder = to_lvds_encoder(&encoder->base);
> > + u32 tmp, flags = 0;
> > int dotclock;
> >
> > - if (HAS_PCH_SPLIT(dev))
> > - lvds_reg = PCH_LVDS;
> > - else
> > - lvds_reg = LVDS;
> > -
> > - tmp = I915_READ(lvds_reg);
> > + tmp = I915_READ(lvds_encoder->reg);
> > if (tmp & LVDS_HSYNC_POLARITY)
> > flags |= DRM_MODE_FLAG_NHSYNC;
> > else
> > @@ -944,6 +940,7 @@ void intel_lvds_init(struct drm_device *dev)
> > struct drm_display_mode *downclock_mode = NULL;
> > struct edid *edid;
> > struct drm_crtc *crtc;
> > + u32 lvds_reg;
> > u32 lvds;
> > int pipe;
> > u8 pin;
> > @@ -966,8 +963,15 @@ void intel_lvds_init(struct drm_device *dev)
> > if (dmi_check_system(intel_no_lvds))
> > return;
> >
> > + if (HAS_PCH_SPLIT(dev))
> > + lvds_reg = PCH_LVDS;
> > + else
> > + lvds_reg = LVDS;
> > +
> > + lvds = I915_READ(lvds_reg);
> > +
> > if (HAS_PCH_SPLIT(dev)) {
> > - if ((I915_READ(PCH_LVDS) & LVDS_DETECTED) == 0)
> > + if ((lvds & LVDS_DETECTED) == 0)
> > return;
> > if (dev_priv->vbt.edp_support) {
> > DRM_DEBUG_KMS("disable LVDS for eDP support\n");
> > @@ -977,8 +981,7 @@ void intel_lvds_init(struct drm_device *dev)
> >
> > pin = GMBUS_PIN_PANEL;
> > if (!lvds_is_present_in_vbt(dev, &pin)) {
> > - u32 reg = HAS_PCH_SPLIT(dev) ? PCH_LVDS : LVDS;
> > - if ((I915_READ(reg) & LVDS_PORT_EN) == 0) {
> > + if ((lvds & LVDS_PORT_EN) == 0) {
> > DRM_DEBUG_KMS("LVDS is not present in VBT\n");
> > return;
> > }
> > @@ -1055,11 +1058,7 @@ void intel_lvds_init(struct drm_device *dev)
> > connector->interlace_allowed = false;
> > connector->doublescan_allowed = false;
> >
> > - if (HAS_PCH_SPLIT(dev)) {
> > - lvds_encoder->reg = PCH_LVDS;
> > - } else {
> > - lvds_encoder->reg = LVDS;
> > - }
> > + lvds_encoder->reg = lvds_reg;
> >
> > /* create the scaling mode property */
> > drm_mode_create_scaling_mode_property(dev);
> > @@ -1140,7 +1139,6 @@ void intel_lvds_init(struct drm_device *dev)
> > if (HAS_PCH_SPLIT(dev))
> > goto failed;
> >
> > - lvds = I915_READ(LVDS);
> > pipe = (lvds & LVDS_PIPEB_SELECT) ? 1 : 0;
> > crtc = intel_get_crtc_for_pipe(dev, pipe);
> >
> > --
> > 2.4.6
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
