diff mbox

[08/16] drm/i915/i2c: Convert from using GMBUS1 + reg_offset idiom to reg + 0

Message ID 1305235044-9159-9-git-send-email-chris@chris-wilson.co.uk
State New, archived
Headers show

Commit Message

Chris Wilson May 12, 2011, 9:17 p.m. UTC
Keith complained that GMBUSx + reg_offset was ugly. An alternative
naming scheme which is more consistent with the reset of the code base
is to store the address of the GMBUS0 and then reference each of the
GMBUSx registers as an offset from GMBUS0.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_i2c.c |   51 +++++++++++++++++++++----------------
 1 files changed, 29 insertions(+), 22 deletions(-)

Comments

Keith Packard May 13, 2011, 12:36 a.m. UTC | #1
On Thu, 12 May 2011 22:17:16 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:

> -	I915_WRITE(GMBUS0 + reg_offset, 0);
> +	intel_i2c_reset(dev_priv->dev);

This looks like an unrelated change. Please split this out.
Keith Packard May 13, 2011, 12:40 a.m. UTC | #2
On Thu, 12 May 2011 22:17:16 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:

> Keith complained that GMBUSx + reg_offset was ugly. An alternative
> naming scheme which is more consistent with the reset of the code base
> is to store the address of the GMBUS0 and then reference each of the
> GMBUSx registers as an offset from GMBUS0.

This looks completely wrong -- GMBUS1 is GMBUS0 + 4, not GMBUS0 + 1.

How about a simple function that computes the GMBUS register address
based on the device and a number? like:

static int intel_gmbus_reg(struct drm_device *dev, int reg) {
        int     base = HAS_PCH_SPLIT(dev) ? PCH_GMBUS0 : GMBUS0;

        return base + reg * 4;
}
Chris Wilson May 13, 2011, 9:28 a.m. UTC | #3
On Thu, 12 May 2011 17:40:42 -0700, Keith Packard <keithp@keithp.com> wrote:
> On Thu, 12 May 2011 22:17:16 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> 
> > Keith complained that GMBUSx + reg_offset was ugly. An alternative
> > naming scheme which is more consistent with the reset of the code base
> > is to store the address of the GMBUS0 and then reference each of the
> > GMBUSx registers as an offset from GMBUS0.
> 
> This looks completely wrong -- GMBUS1 is GMBUS0 + 4, not GMBUS0 + 1.

That was shameful.

> How about a simple function that computes the GMBUS register address
> based on the device and a number? like:
> 
> static int intel_gmbus_reg(struct drm_device *dev, int reg) {
>         int     base = HAS_PCH_SPLIT(dev) ? PCH_GMBUS0 : GMBUS0;
> 
>         return base + reg * 4;

And how about something like:

#define I915_GMBUS_WRITE(reg, val) \
   I915_WRITE(intel_gmbus_reg(dev_priv->dev, reg), val)

I915_GMBUS_WRITE(0, val);

For the body?
-Chris
Keith Packard May 13, 2011, 3 p.m. UTC | #4
On Fri, 13 May 2011 10:28:34 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:

> And how about something like:
> 
> #define I915_GMBUS_WRITE(reg, val) \
>    I915_WRITE(intel_gmbus_reg(dev_priv->dev, reg), val)
> 
> I915_GMBUS_WRITE(0, val);

Yes, that's very pretty! And do give it a try this time :-)
Keith Packard June 3, 2011, 8:55 p.m. UTC | #5
On Fri, 13 May 2011 08:00:40 -0700, Keith Packard <keithp@keithp.com> wrote:
Non-text part: multipart/signed
> On Fri, 13 May 2011 10:28:34 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> 
> > And how about something like:
> > 
> > #define I915_GMBUS_WRITE(reg, val) \
> >    I915_WRITE(intel_gmbus_reg(dev_priv->dev, reg), val)
> > 
> > I915_GMBUS_WRITE(0, val);

I haven't seen a follow-up patch with this in place.
Chris Wilson June 3, 2011, 11:09 p.m. UTC | #6
On Fri, 03 Jun 2011 13:55:10 -0700, Keith Packard <keithp@keithp.com> wrote:
> On Fri, 13 May 2011 08:00:40 -0700, Keith Packard <keithp@keithp.com> wrote:
> Non-text part: multipart/signed
> > On Fri, 13 May 2011 10:28:34 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > 
> > > And how about something like:
> > > 
> > > #define I915_GMBUS_WRITE(reg, val) \
> > >    I915_WRITE(intel_gmbus_reg(dev_priv->dev, reg), val)
> > > 
> > > I915_GMBUS_WRITE(0, val);
> 
> I haven't seen a follow-up patch with this in place.

I need to address the broader concerns raised by Jean Delvare first.
Once I have our i2c adapter to his liking, I can then get the code to
yours.
-Chris
Keith Packard June 3, 2011, 11:48 p.m. UTC | #7
On Sat, 04 Jun 2011 00:09:29 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:

> I need to address the broader concerns raised by Jean Delvare first.
> Once I have our i2c adapter to his liking, I can then get the code to
> yours.

Sounds good.
Keith Packard July 13, 2011, 6:33 p.m. UTC | #8
On Sat, 04 Jun 2011 00:09:29 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:

> I need to address the broader concerns raised by Jean Delvare first.
> Once I have our i2c adapter to his liking, I can then get the code to
> yours.

Are you still planning on cleaning up the i2c stuff at some point?
Chris Wilson July 13, 2011, 7:22 p.m. UTC | #9
On Wed, 13 Jul 2011 11:33:22 -0700, Keith Packard <keithp@keithp.com> wrote:
Non-text part: multipart/signed
> On Sat, 04 Jun 2011 00:09:29 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> 
> > I need to address the broader concerns raised by Jean Delvare first.
> > Once I have our i2c adapter to his liking, I can then get the code to
> > yours.
> 
> Are you still planning on cleaning up the i2c stuff at some point?

Still planning on doing so, yes. The cost of reading the EDID every 10s is
irksome.
-Chris
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
index d3b903b..ed11523 100644
--- a/drivers/gpu/drm/i915/intel_i2c.c
+++ b/drivers/gpu/drm/i915/intel_i2c.c
@@ -51,6 +51,11 @@  struct intel_gpio {
 	u32 reg;
 };
 
+static int intel_gmbus_reg0(struct drm_device *dev)
+{
+	return HAS_PCH_SPLIT(dev) ? PCH_GMBUS0 : GMBUS0;
+}
+
 void
 intel_i2c_reset(struct drm_device *dev)
 {
@@ -232,36 +237,36 @@  gmbus_xfer(struct i2c_adapter *adapter,
 					       struct intel_gmbus,
 					       adapter);
 	struct drm_i915_private *dev_priv = adapter->algo_data;
-	int i, reg_offset;
+	int i, reg;
 
 	if (bus->force_bit)
 		return intel_i2c_quirk_xfer(dev_priv,
 					    bus->force_bit, msgs, num);
 
-	reg_offset = HAS_PCH_SPLIT(dev_priv->dev) ? PCH_GMBUS0 - GMBUS0 : 0;
+	reg = intel_gmbus_reg0(dev_priv->dev);
 
-	I915_WRITE(GMBUS0 + reg_offset, bus->reg0);
+	I915_WRITE(reg + 0, bus->reg0);
 
 	for (i = 0; i < num; i++) {
 		u16 len = msgs[i].len;
 		u8 *buf = msgs[i].buf;
 
 		if (msgs[i].flags & I2C_M_RD) {
-			I915_WRITE(GMBUS1 + reg_offset,
+			I915_WRITE(reg + 1,
 				   GMBUS_CYCLE_WAIT | (i + 1 == num ? GMBUS_CYCLE_STOP : 0) |
 				   (len << GMBUS_BYTE_COUNT_SHIFT) |
 				   (msgs[i].addr << GMBUS_SLAVE_ADDR_SHIFT) |
 				   GMBUS_SLAVE_READ | GMBUS_SW_RDY);
-			POSTING_READ(GMBUS2+reg_offset);
+			POSTING_READ(reg + 2);
 			do {
 				u32 val, loop = 0;
 
-				if (wait_for(I915_READ(GMBUS2 + reg_offset) & (GMBUS_SATOER | GMBUS_HW_RDY), 50))
+				if (wait_for(I915_READ(reg + 2) & (GMBUS_SATOER | GMBUS_HW_RDY), 50))
 					goto timeout;
-				if (I915_READ(GMBUS2 + reg_offset) & GMBUS_SATOER)
+				if (I915_READ(reg + 2) & GMBUS_SATOER)
 					goto clear_err;
 
-				val = I915_READ(GMBUS3 + reg_offset);
+				val = I915_READ(reg + 3);
 				do {
 					*buf++ = val & 0xff;
 					val >>= 8;
@@ -275,18 +280,18 @@  gmbus_xfer(struct i2c_adapter *adapter,
 				val |= *buf++ << (8 * loop);
 			} while (--len && ++loop < 4);
 
-			I915_WRITE(GMBUS3 + reg_offset, val);
-			I915_WRITE(GMBUS1 + reg_offset,
+			I915_WRITE(reg + 3, val);
+			I915_WRITE(reg + 1,
 				   (i + 1 == num ? GMBUS_CYCLE_STOP : GMBUS_CYCLE_WAIT) |
 				   (msgs[i].len << GMBUS_BYTE_COUNT_SHIFT) |
 				   (msgs[i].addr << GMBUS_SLAVE_ADDR_SHIFT) |
 				   GMBUS_SLAVE_WRITE | GMBUS_SW_RDY);
-			POSTING_READ(GMBUS2+reg_offset);
+			POSTING_READ(reg + 2);
 
 			while (len) {
-				if (wait_for(I915_READ(GMBUS2 + reg_offset) & (GMBUS_SATOER | GMBUS_HW_RDY), 50))
+				if (wait_for(I915_READ(reg + 2) & (GMBUS_SATOER | GMBUS_HW_RDY), 50))
 					goto timeout;
-				if (I915_READ(GMBUS2 + reg_offset) & GMBUS_SATOER)
+				if (I915_READ(reg + 2) & GMBUS_SATOER)
 					goto clear_err;
 
 				val = loop = 0;
@@ -294,14 +299,15 @@  gmbus_xfer(struct i2c_adapter *adapter,
 					val |= *buf++ << (8 * loop);
 				} while (--len && ++loop < 4);
 
-				I915_WRITE(GMBUS3 + reg_offset, val);
-				POSTING_READ(GMBUS2+reg_offset);
+				I915_WRITE(reg + 3, val);
+				POSTING_READ(reg + 2);
 			}
 		}
 
-		if (i + 1 < num && wait_for(I915_READ(GMBUS2 + reg_offset) & (GMBUS_SATOER | GMBUS_HW_WAIT_PHASE), 50))
+		if (i + 1 < num &&
+		    wait_for(I915_READ(reg + 2) & (GMBUS_SATOER | GMBUS_HW_WAIT_PHASE), 50))
 			goto timeout;
-		if (I915_READ(GMBUS2 + reg_offset) & GMBUS_SATOER)
+		if (I915_READ(reg + 2) & GMBUS_SATOER)
 			goto clear_err;
 	}
 
@@ -312,22 +318,23 @@  clear_err:
 	 * of resetting the GMBUS controller and so clearing the
 	 * BUS_ERROR raised by the slave's NAK.
 	 */
-	I915_WRITE(GMBUS1 + reg_offset, GMBUS_SW_CLR_INT);
-	I915_WRITE(GMBUS1 + reg_offset, 0);
+	I915_WRITE(reg + 1, GMBUS_SW_CLR_INT);
+	I915_WRITE(reg + 1, 0);
 
 done:
 	/* Mark the GMBUS interface as disabled. We will re-enable it at the
 	 * start of the next xfer, till then let it sleep.
 	 */
-	I915_WRITE(GMBUS0 + reg_offset, 0);
+	I915_WRITE(reg + 0, 0);
 	return i;
 
 timeout:
 	DRM_INFO("GMBUS timed out, falling back to bit banging on pin %d [%s]\n",
 		 bus->reg0 & 0xff, bus->adapter.name);
-	I915_WRITE(GMBUS0 + reg_offset, 0);
+	intel_i2c_reset(dev_priv->dev);
 
-	/* Hardware may not support GMBUS over these pins? Try GPIO bitbanging instead. */
+	/* Hardware may not support GMBUS over these pins?
+	 * Try GPIO bitbanging instead. */
 	bus->force_bit = intel_gpio_create(dev_priv, bus->reg0 & 0xff);
 	if (!bus->force_bit)
 		return -ENOMEM;