Messages in this thread | | | From | Daniel Kurtz <> | Date | Wed, 28 Mar 2012 19:39:17 +0800 | Subject | Re: [PATCH 13/13 v4] drm/i915/intel_i2c: reuse GMBUS2 value read in polling loop |
| |
On Wed, Mar 28, 2012 at 3:02 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote: > On Wed, 28 Mar 2012 02:36:22 +0800, Daniel Kurtz <djkurtz@chromium.org> wrote: >> Save the GMBUS2 value read while polling for state changes, and then >> reuse this value when determining for which reason the loops were exited. >> This is a small optimization which saves a couple of bus accesses for >> memory mapped IO registers. >> >> To avoid "assigning in if clause" checkpatch errors", use a ret variable >> to store the wait_for macro return value. >> >> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org> >> --- >> drivers/gpu/drm/i915/intel_i2c.c | 32 ++++++++++++++++++++------------ >> 1 files changed, 20 insertions(+), 12 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c >> index c71f3dc..174fc71 100644 >> --- a/drivers/gpu/drm/i915/intel_i2c.c >> +++ b/drivers/gpu/drm/i915/intel_i2c.c >> @@ -210,6 +210,7 @@ gmbus_xfer_read(struct drm_i915_private *dev_priv, struct i2c_msg *msg, >> int reg_offset = dev_priv->gpio_mmio_base; >> u16 len = msg->len; >> u8 *buf = msg->buf; >> + u32 gmbus2; > Does the temporary really need such broad scoping? > >> I915_WRITE(GMBUS1 + reg_offset, >> gmbus1 | >> @@ -219,13 +220,15 @@ gmbus_xfer_read(struct drm_i915_private *dev_priv, struct i2c_msg *msg, >> GMBUS_SLAVE_READ | GMBUS_SW_RDY); >> POSTING_READ(GMBUS2 + reg_offset); > Might as well shave this read as well.
Do you know why POSTING_READ() was there in the first place? As far as I can tell, these are used to ensure memory barriers are inserted between a group of writes, and subsequent reads to memory mapped io registers. However, the normal I915_READ() and I915_WRITE() macros already call readl() / writel(), which already have an explicit mb(). So, can we just get rid of all of them, or am I missing something?
If so, I propose we do that in another patch, and leave this one alone.
> >> do { >> + int ret; >> u32 val, loop = 0; >> >> - if (wait_for(I915_READ(GMBUS2 + reg_offset) & >> - (GMBUS_SATOER | GMBUS_HW_RDY), >> - 50)) >> + ret = wait_for((gmbus2 = I915_READ(GMBUS2 + reg_offset)) & >> + (GMBUS_SATOER | GMBUS_HW_RDY), >> + 50); >> + if (ret) >> return -ETIMEDOUT; >> - if (I915_READ(GMBUS2 + reg_offset) & GMBUS_SATOER) >> + if (gmbus2 & GMBUS_SATOER) >> return -ENXIO; >> >> val = I915_READ(GMBUS3 + reg_offset); >> @@ -245,6 +248,7 @@ gmbus_xfer_write(struct drm_i915_private *dev_priv, struct i2c_msg *msg) >> u16 len = msg->len; >> u8 *buf = msg->buf; >> u32 val, loop; >> + u32 gmbus2; >> >> val = loop = 0; >> while (len && loop < 4) { >> @@ -260,6 +264,7 @@ gmbus_xfer_write(struct drm_i915_private *dev_priv, struct i2c_msg *msg) >> GMBUS_SLAVE_WRITE | GMBUS_SW_RDY); >> POSTING_READ(GMBUS2 + reg_offset); >> while (len) { >> + int ret; >> val = loop = 0; >> do { >> val |= *buf++ << (8 * loop); >> @@ -268,11 +273,12 @@ gmbus_xfer_write(struct drm_i915_private *dev_priv, struct i2c_msg *msg) >> I915_WRITE(GMBUS3 + reg_offset, val); >> POSTING_READ(GMBUS2 + reg_offset); > > And here. > > -- > Chris Wilson, Intel Open Source Technology Centre -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |