[Intel-gfx] [PATCH 5/5] drm/i915/gmbus: Reset the controller on initialisation
chris at chris-wilson.co.uk
Tue Apr 5 14:26:55 PDT 2011
On Tue, 05 Apr 2011 13:59:58 -0700, Keith Packard <keithp at keithp.com> wrote:
> On Tue, 5 Apr 2011 10:24:18 +0100, Chris Wilson <chris at chris-wilson.co.uk> wrote:
> > Toggle the Software Clear Interrupt bit which resets the controller to
> > clear any prior BUS_ERROR condition before we begin to use the
> > controller in earnest.
> We do this in two places now, do we want to share the code?
> > + int reg_offset;
> > +
> > + reg_offset = 0;
> > if (HAS_PCH_SPLIT(dev))
> > - I915_WRITE(PCH_GMBUS0, 0);
> > - else
> > - I915_WRITE(GMBUS0, 0);
> > + reg_offset = PCH_GMBUS0 - GMBUS0;
> > +
> > + /* First reset the controller by toggling the Sw Clear Interrupt. */
> > + I915_WRITE(GMBUS1 + reg_offset, GMBUS_SW_CLR_INT);
> > + I915_WRITE(GMBUS1 + reg_offset, 0);
> > +
> > + /* Then mark the controller as disabled. */
> > + I915_WRITE(GMBUS0 + reg_offset, 0);
> That's really ugly register addressing, but it looks like a common idiom
> in this file...
I'd change the lot for a cleaner method, the best I thought of was a
change of names for the constants/variables.
static void i915_gmbus_write(struct drm_device *dev, int reg, int value)
struct drm_i915_private *dev_priv = dev->dev_private;
I915_WRITE(reg + (HAS_PCH_SPLIT(dev) ? PCH_GMBUS0 : GMBUS0), value);
expands to something dreadful.
But with a
#define GMBUS_WRITE(reg, value) i915_gmbus_write(dev, reg, value)
we go from
I915_WRITE(GMBUS0 + reg_offset, 0);
I would still prefer GMBUS_WRITE(GMBUS0, 0); though.
As the patch only addresses a theoretical bug, we can punt the meat of the
patch till later and attack the stylistic points first. (Obviously through
Chris Wilson, Intel Open Source Technology Centre
More information about the Intel-gfx