[PATCH 00/13 v4] fix gmbus writes and related issues

[PATCH 00/13 v4] fix gmbus writes and related issues

[Daniel Kurtz]

This patchset addresses a couple of issues with the i915 gmbus implementation:
 * fixes misassigned pin port pair for HDMI-D
 * fixes write transactions when they are the only transaction requested
   (including large >4-byte writes) by terminating every transaction with a
   WAIT cycle.
 * returns -ENXIO and -ETIMEDOUT as appropriate so upper layers can handled
   i2c transaction failures
 * optimizes the typical read transaction case by using the INDEX cycle
v3:
 * rebased onto Daniel Vetter's drm-intel-next-queued branch
   at git://people.freedesktop.org/~danvet/drm-intel
 * replace intel_i2c_quirk_xfer with pre/post_xfer i2c routines
 * pre-allocate gmbus array
 * drop interrupt approach since I could not make it stable, probably due to
   difficulty in clearing and resetting the GMBUS interrupt which is buffered
   behind the SDE's PCH interrupt.
 * Fix zero-length writes
 * Wait for IDLE before clearing NAK

v4:
 * refactored gmbus_xfer (great idea, danvet!)
 * fixed refactoring braino (swapped [new] patches 6 & 7)
 * added URLs for docs to commit message
 * fixed checkpatch warnings in the "reuse GMBUS2" patch
 * dropped the 'functionaltiy' part of v3 patch 3
 * use WARN_ON instead of BUG_ON when an invalid port is requested

Daniel Kurtz (13):
  drm/i915/intel_i2c: refactor gmbus_xfer
  drm/i915/intel_i2c: cleanup error messages and comments
  drm/i915/intel_i2c: assign HDMI port D to pin pair 6
  drm/i915/intel_i2c: use i2c pre/post_xfer functions to setup gpio
    xfers
  drm/i915/intel_i2c: refactor using intel_gmbus_get_adapter
  drm/i915/intel_i2c: gmbus disabled and reserved ports are invalid
  drm/i915/intel_i2c: allocate gmbus array as part of drm_i915_private
  drm/i915/intel_i2c: handle zero-length writes
  drm/i915/intel_i2c: use double-buffered writes
  drm/i915/intel_i2c: always wait for IDLE before clearing NAK
  drm/i915/intel_i2c: use WAIT cycle, not STOP
  drm/i915/intel_i2c: use INDEX cycles for i2c read transactions
  drm/i915/intel_i2c: reuse GMBUS2 value read in polling loop

 drivers/gpu/drm/i915/i915_drv.h    |   10 +-
 drivers/gpu/drm/i915/i915_reg.h    |    6 +-
 drivers/gpu/drm/i915/intel_bios.c  |    4 +-
 drivers/gpu/drm/i915/intel_crt.c   |   14 +-
 drivers/gpu/drm/i915/intel_dvo.c   |    6 +-
 drivers/gpu/drm/i915/intel_hdmi.c  |    9 +-
 drivers/gpu/drm/i915/intel_i2c.c   |  356 ++++++++++++++++++++++--------------
 drivers/gpu/drm/i915/intel_lvds.c  |    7 +-
 drivers/gpu/drm/i915/intel_modes.c |    3 +-
 drivers/gpu/drm/i915/intel_sdvo.c  |    9 +-
 10 files changed, 261 insertions(+), 163 deletions(-)

-- 
1.7.7.3

[PATCH 01/13 v4] drm/i915/intel_i2c: refactor gmbus_xfer

[Daniel Kurtz]

Split out gmbus_xfer_read/write() helper functions.

Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
---
 drivers/gpu/drm/i915/intel_i2c.c |  151 +++++++++++++++++++++++---------------
 1 files changed, 92 insertions(+), 59 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
index 0713cc2..30675ce 100644
--- a/drivers/gpu/drm/i915/intel_i2c.c
+++ b/drivers/gpu/drm/i915/intel_i2c.c
@@ -198,6 +198,82 @@ intel_i2c_quirk_xfer(struct intel_gmbus *bus,
 }
 
 static int
+gmbus_xfer_read(struct drm_i915_private *dev_priv, struct i2c_msg *msg,
+		bool last)
+{
+	int reg_offset = dev_priv->gpio_mmio_base;
+	u16 len = msg->len;
+	u8 *buf = msg->buf;
+
+	I915_WRITE(GMBUS1 + reg_offset,
+		   GMBUS_CYCLE_WAIT |
+		   (last ? GMBUS_CYCLE_STOP : 0) |
+		   (len << GMBUS_BYTE_COUNT_SHIFT) |
+		   (msg->addr << GMBUS_SLAVE_ADDR_SHIFT) |
+		   GMBUS_SLAVE_READ | GMBUS_SW_RDY);
+	POSTING_READ(GMBUS2 + reg_offset);
+	do {
+		u32 val, loop = 0;
+
+		if (wait_for(I915_READ(GMBUS2 + reg_offset) &
+			     (GMBUS_SATOER | GMBUS_HW_RDY),
+			     50))
+			return -ETIMEDOUT;
+		if (I915_READ(GMBUS2 + reg_offset) & GMBUS_SATOER)
+			return -ENXIO;
+
+		val = I915_READ(GMBUS3 + reg_offset);
+		do {
+			*buf++ = val & 0xff;
+			val >>= 8;
+		} while (--len && ++loop < 4);
+	} while (len);
+
+	return 0;
+}
+
+static int
+gmbus_xfer_write(struct drm_i915_private *dev_priv, struct i2c_msg *msg,
+		bool last)
+{
+	int reg_offset = dev_priv->gpio_mmio_base;
+	u16 len = msg->len;
+	u8 *buf = msg->buf;
+	u32 val, loop;
+
+	val = loop = 0;
+	do {
+		val |= *buf++ << (8 * loop);
+	} while (--len && ++loop < 4);
+
+	I915_WRITE(GMBUS3 + reg_offset, val);
+	I915_WRITE(GMBUS1 + reg_offset,
+		   GMBUS_CYCLE_WAIT |
+		   (last ? GMBUS_CYCLE_STOP : 0) |
+		   (msg->len << GMBUS_BYTE_COUNT_SHIFT) |
+		   (msg->addr << GMBUS_SLAVE_ADDR_SHIFT) |
+		   GMBUS_SLAVE_WRITE | GMBUS_SW_RDY);
+	POSTING_READ(GMBUS2 + reg_offset);
+	while (len) {
+		if (wait_for(I915_READ(GMBUS2 + reg_offset) &
+			     (GMBUS_SATOER | GMBUS_HW_RDY),
+			     50))
+			return -ETIMEDOUT;
+		if (I915_READ(GMBUS2 + reg_offset) & GMBUS_SATOER)
+			return -ENXIO;
+
+		val = loop = 0;
+		do {
+			val |= *buf++ << (8 * loop);
+		} while (--len && ++loop < 4);
+
+		I915_WRITE(GMBUS3 + reg_offset, val);
+		POSTING_READ(GMBUS2 + reg_offset);
+	}
+	return 0;
+}
+
+static int
 gmbus_xfer(struct i2c_adapter *adapter,
 	   struct i2c_msg *msgs,
 	   int num)
@@ -220,65 +296,22 @@ gmbus_xfer(struct i2c_adapter *adapter,
 	I915_WRITE(GMBUS0 + reg_offset, 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,
-				   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);
-			do {
-				u32 val, loop = 0;
-
-				if (wait_for(I915_READ(GMBUS2 + reg_offset) & (GMBUS_SATOER | GMBUS_HW_RDY), 50))
-					goto timeout;
-				if (I915_READ(GMBUS2 + reg_offset) & GMBUS_SATOER)
-					goto clear_err;
-
-				val = I915_READ(GMBUS3 + reg_offset);
-				do {
-					*buf++ = val & 0xff;
-					val >>= 8;
-				} while (--len && ++loop < 4);
-			} while (len);
-		} else {
-			u32 val, loop;
-
-			val = loop = 0;
-			do {
-				val |= *buf++ << (8 * loop);
-			} while (--len && ++loop < 4);
-
-			I915_WRITE(GMBUS3 + reg_offset, val);
-			I915_WRITE(GMBUS1 + reg_offset,
-				   GMBUS_CYCLE_WAIT |
-				   (i + 1 == num ? GMBUS_CYCLE_STOP : 0) |
-				   (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);
-
-			while (len) {
-				if (wait_for(I915_READ(GMBUS2 + reg_offset) & (GMBUS_SATOER | GMBUS_HW_RDY), 50))
-					goto timeout;
-				if (I915_READ(GMBUS2 + reg_offset) & GMBUS_SATOER)
-					goto clear_err;
-
-				val = loop = 0;
-				do {
-					val |= *buf++ << (8 * loop);
-				} while (--len && ++loop < 4);
-
-				I915_WRITE(GMBUS3 + reg_offset, val);
-				POSTING_READ(GMBUS2+reg_offset);
-			}
-		}
-
-		if (i + 1 < num && wait_for(I915_READ(GMBUS2 + reg_offset) & (GMBUS_SATOER | GMBUS_HW_WAIT_PHASE), 50))
+		bool last = i + 1 == num;
+
+		if (msgs[i].flags & I2C_M_RD)
+			ret = gmbus_xfer_read(dev_priv, &msgs[i], last);
+		else
+			ret = gmbus_xfer_write(dev_priv, &msgs[i], last);
+
+		if (ret == -ETIMEDOUT)
+			goto timeout;
+		if (ret == -ENXIO)
+			goto clear_err;
+
+		if (!last &&
+		    wait_for(I915_READ(GMBUS2 + reg_offset) &
+			     (GMBUS_SATOER | GMBUS_HW_WAIT_PHASE),
+			     50))
 			goto timeout;
 		if (I915_READ(GMBUS2 + reg_offset) & GMBUS_SATOER)
 			goto clear_err;
-- 
1.7.7.3

Re: [PATCH 01/13 v4] drm/i915/intel_i2c: refactor gmbus_xfer

[Chris Wilson]

On Wed, 28 Mar 2012 02:36:10 +0800, Daniel Kurtz <djkurtz@chromium.org> wrote:
> Split out gmbus_xfer_read/write() helper functions.
> 
> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH 02/13 v4] drm/i915/intel_i2c: cleanup error messages and comments

[Daniel Kurtz]

Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
---
 drivers/gpu/drm/i915/intel_i2c.c |   11 +++++++----
 1 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
index 30675ce..e6c090b 100644
--- a/drivers/gpu/drm/i915/intel_i2c.c
+++ b/drivers/gpu/drm/i915/intel_i2c.c
@@ -333,17 +333,20 @@ done:
 	 * till then let it sleep.
 	 */
 	if (wait_for((I915_READ(GMBUS2 + reg_offset) & GMBUS_ACTIVE) == 0, 10))
-		DRM_INFO("GMBUS timed out waiting for idle\n");
+		DRM_INFO("GMBUS [%s] timed out waiting for idle\n",
+			 bus->adapter.name);
 	I915_WRITE(GMBUS0 + reg_offset, 0);
 	ret = i;
 	goto out;
 
 timeout:
-	DRM_INFO("GMBUS timed out, falling back to bit banging on pin %d [%s]\n",
-		 bus->reg0 & 0xff, bus->adapter.name);
+	DRM_INFO("GMBUS [%s] timed out, falling back to bit banging on pin %d\n",
+		 bus->adapter.name, bus->reg0 & 0xff);
 	I915_WRITE(GMBUS0 + reg_offset, 0);
 
-	/* Hardware may not support GMBUS over these pins? Try GPIO bitbanging instead. */
+	/* Hardware may not support GMBUS over these pins?
+	 * Try GPIO bitbanging instead.
+	 */
 	if (!bus->has_gpio) {
 		ret = -EIO;
 	} else {
-- 
1.7.7.3

Re: [PATCH 02/13 v4] drm/i915/intel_i2c: cleanup error messages and comments

[Chris Wilson]

On Wed, 28 Mar 2012 02:36:11 +0800, Daniel Kurtz <djkurtz@chromium.org> wrote:
> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH 03/13 v4] drm/i915/intel_i2c: assign HDMI port D to pin pair 6

[Daniel Kurtz]

According to i915 documentation [1], "Port D" (DP/HDMI Port D) is
actually gmbus pin pair 6 (gmbus0.2:0 == 110b GPIOF), not 7 (111b).
Pin pair 7 is a reserved pair.

[1] Documentation for [DevSNB+] and [DevIBX], as found on
http://intellinuxgraphics.org:

[DevSNB+]:
http://intellinuxgraphics.org/documentation/SNB/IHD_OS_Vol3_Part3.pdf
 Section 2.2.2 lists the 6 gmbus ports (gpio pin pairs):
    [ 5: HDMI/DPD, 4: HDMIB, 3: HDMI/DPC, 2: LVDS, 1: SSC, 0: VGA ]
 2.2.2.1 lists the GPIO registers to control these 6 ports.
 2.2.3.1 lists the mapping between 5 of these gmbus ports and the 3
 Pin_Pair_Select bits (of the GMBUS0 register).  This table is missing
 HDMIB (port 101).

[DevIBX]: http://intellinuxgraphics.org/IHD_OS_Vol3_Part3r2.pdf
 Section 2.2.2 lists the same 6 gmbus ports plus two 'reserved' gpio
 ports.
 2.2.2.1 lists 8 GPIO registers... however, it says the size of the
 block is 6x32, which implies that those 2 reserved GPIO registers
 (GPIO_6 & GPIO_7) don't actually exist (or are irrelevant).
 2.2.3.1 lists the mapping between the 6 named gmbus ports and the 3
 Pin_Pair_Select bits (of the GMBUS0 register).  This table has HDMIB.

Note: the "reserved" and "disabled" pairs do not actually map to a
physical pair of pins, nor GPIO regs and shouldn't be initialized or used.
Fixing this is left for a later patch.

This bug had not been noticed earlier for two reasons:
 1) Until recently, "gmbus" mode was disabled - all transfers actually
    used "bit-bang" mode on GPIO port 5 (the "HDMI/DPD CTLDATA/CLK"
    pair), at register 0x5024 (defined as GPIOF i915_reg.h).
    Since this is the correct pair of pins for HDMI1, transfers succeed.

 2) Even if gmbus mode is re-enabled, the first attempted transaction
    will fail because it tries to use the wrong ("Reserved") pin pair.
    However, the driver immediately falls back again to the bit-bang
    method, which correctly uses GPIOF, so again, transfers succeed.

However, if gmbus mode is re-enabled and the GPIO fall-back mode is
disabled, then reading an attached monitor's EDID fail.

Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
---
 drivers/gpu/drm/i915/i915_reg.h  |    6 +++---
 drivers/gpu/drm/i915/intel_i2c.c |    4 ++--
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index f3609f2..accd8ee 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -742,9 +742,9 @@
 #define   GMBUS_PORT_PANEL	3
 #define   GMBUS_PORT_DPC	4 /* HDMIC */
 #define   GMBUS_PORT_DPB	5 /* SDVO, HDMIB */
-				  /* 6 reserved */
-#define   GMBUS_PORT_DPD	7 /* HDMID */
-#define   GMBUS_NUM_PORTS       8
+#define   GMBUS_PORT_DPD	6 /* HDMID */
+#define   GMBUS_PORT_RESERVED	7 /* 7 reserved */
+#define   GMBUS_NUM_PORTS	8
 #define GMBUS1			0x5104 /* command/status */
 #define   GMBUS_SW_CLR_INT	(1<<31)
 #define   GMBUS_SW_RDY		(1<<30)
diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
index e6c090b..9347281 100644
--- a/drivers/gpu/drm/i915/intel_i2c.c
+++ b/drivers/gpu/drm/i915/intel_i2c.c
@@ -148,8 +148,8 @@ intel_gpio_setup(struct intel_gmbus *bus, u32 pin)
 		GPIOC,
 		GPIOD,
 		GPIOE,
-		0,
 		GPIOF,
+		0,
 	};
 	struct i2c_algo_bit_data *algo;
 
@@ -385,8 +385,8 @@ int intel_setup_gmbus(struct drm_device *dev)
 		"panel",
 		"dpc",
 		"dpb",
-		"reserved",
 		"dpd",
+		"reserved",
 	};
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	int ret, i;
-- 
1.7.7.3

[PATCH 04/13 v4] drm/i915/intel_i2c: use i2c pre/post_xfer functions to setup gpio xfers

[Daniel Kurtz]

Instead of rolling our own custom quirk_xfer function, use the bit_algo
pre_xfer and post_xfer functions to setup and teardown bit-banged
i2c transactions.

Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_i2c.c |   60 +++++++++++++++++++++----------------
 1 files changed, 34 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
index 9347281..1bb6362 100644
--- a/drivers/gpu/drm/i915/intel_i2c.c
+++ b/drivers/gpu/drm/i915/intel_i2c.c
@@ -137,6 +137,35 @@ static void set_data(void *data, int state_high)
 	POSTING_READ(bus->gpio_reg);
 }
 
+static int
+intel_gpio_pre_xfer(struct i2c_adapter *adapter)
+{
+	struct intel_gmbus *bus = container_of(adapter,
+					       struct intel_gmbus,
+					       adapter);
+	struct drm_i915_private *dev_priv = bus->dev_priv;
+
+	intel_i2c_reset(dev_priv->dev);
+	intel_i2c_quirk_set(dev_priv, true);
+	set_data(bus, 1);
+	set_clock(bus, 1);
+	udelay(I2C_RISEFALL_TIME);
+	return 0;
+}
+
+static void
+intel_gpio_post_xfer(struct i2c_adapter *adapter)
+{
+	struct intel_gmbus *bus = container_of(adapter,
+					       struct intel_gmbus,
+					       adapter);
+	struct drm_i915_private *dev_priv = bus->dev_priv;
+
+	set_data(bus, 1);
+	set_clock(bus, 1);
+	intel_i2c_quirk_set(dev_priv, false);
+}
+
 static bool
 intel_gpio_setup(struct intel_gmbus *bus, u32 pin)
 {
@@ -166,6 +195,8 @@ intel_gpio_setup(struct intel_gmbus *bus, u32 pin)
 	algo->setscl = set_clock;
 	algo->getsda = get_data;
 	algo->getscl = get_clock;
+	algo->pre_xfer = intel_gpio_pre_xfer;
+	algo->post_xfer = intel_gpio_post_xfer;
 	algo->udelay = I2C_RISEFALL_TIME;
 	algo->timeout = usecs_to_jiffies(2200);
 	algo->data = bus;
@@ -174,30 +205,6 @@ intel_gpio_setup(struct intel_gmbus *bus, u32 pin)
 }
 
 static int
-intel_i2c_quirk_xfer(struct intel_gmbus *bus,
-		     struct i2c_msg *msgs,
-		     int num)
-{
-	struct drm_i915_private *dev_priv = bus->dev_priv;
-	int ret;
-
-	intel_i2c_reset(dev_priv->dev);
-
-	intel_i2c_quirk_set(dev_priv, true);
-	set_data(bus, 1);
-	set_clock(bus, 1);
-	udelay(I2C_RISEFALL_TIME);
-
-	ret = i2c_bit_algo.master_xfer(&bus->adapter, msgs, num);
-
-	set_data(bus, 1);
-	set_clock(bus, 1);
-	intel_i2c_quirk_set(dev_priv, false);
-
-	return ret;
-}
-
-static int
 gmbus_xfer_read(struct drm_i915_private *dev_priv, struct i2c_msg *msg,
 		bool last)
 {
@@ -287,7 +294,7 @@ gmbus_xfer(struct i2c_adapter *adapter,
 	mutex_lock(&dev_priv->gmbus_mutex);
 
 	if (bus->force_bit) {
-		ret = intel_i2c_quirk_xfer(bus, msgs, num);
+		ret = i2c_bit_algo.master_xfer(adapter, msgs, num);
 		goto out;
 	}
 
@@ -351,8 +358,9 @@ timeout:
 		ret = -EIO;
 	} else {
 		bus->force_bit = true;
-		ret = intel_i2c_quirk_xfer(bus, msgs, num);
+		ret = i2c_bit_algo.master_xfer(adapter, msgs, num);
 	}
+
 out:
 	mutex_unlock(&dev_priv->gmbus_mutex);
 	return ret;
-- 
1.7.7.3

Re: [PATCH 04/13 v4] drm/i915/intel_i2c: use i2c pre/post_xfer functions to setup gpio xfers

[Chris Wilson]

On Wed, 28 Mar 2012 02:36:13 +0800, Daniel Kurtz <djkurtz@chromium.org> wrote:
> Instead of rolling our own custom quirk_xfer function, use the bit_algo
> pre_xfer and post_xfer functions to setup and teardown bit-banged
> i2c transactions.
> 
> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH 05/13 v4] drm/i915/intel_i2c: refactor using intel_gmbus_get_adapter

[Daniel Kurtz]

Instead of letting other modules directly access the ->gmbus array,
introduce intel_gmbus_get_adapter() for looking up an i2c_adapter
for a given gmbus port identifier.  This will enable later refactoring
of the gmbus port list.

Note: Before requesting an adapter for a given gmbus port number, the
driver must first check its validity using i2c_intel_gmbus_is_port_valid().
If this check fails, a call to intel_gmbus_get_adapter() will WARN_ON and
return NULL.  This is relevant for parts of the driver that read a port
from VBIOS, which might be improperly initialized and contain an invalid
port.  In these cases, the driver must fall back to using a safer default
port.

Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
---
 drivers/gpu/drm/i915/i915_drv.h    |    7 +++++++
 drivers/gpu/drm/i915/intel_bios.c  |    4 ++--
 drivers/gpu/drm/i915/intel_crt.c   |   14 ++++++++------
 drivers/gpu/drm/i915/intel_dvo.c   |    6 +++---
 drivers/gpu/drm/i915/intel_hdmi.c  |    9 ++++++---
 drivers/gpu/drm/i915/intel_i2c.c   |    8 ++++++++
 drivers/gpu/drm/i915/intel_lvds.c  |    7 ++++---
 drivers/gpu/drm/i915/intel_modes.c |    3 ++-
 drivers/gpu/drm/i915/intel_sdvo.c  |    9 +++++----
 9 files changed, 45 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index e11fcb0..44e6430 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1342,6 +1342,13 @@ extern int i915_restore_state(struct drm_device *dev);
 /* intel_i2c.c */
 extern int intel_setup_gmbus(struct drm_device *dev);
 extern void intel_teardown_gmbus(struct drm_device *dev);
+extern inline bool intel_gmbus_is_port_valid(unsigned port)
+{
+	return (port >= GMBUS_PORT_DISABLED && port <= GMBUS_PORT_RESERVED);
+}
+
+extern struct i2c_adapter *intel_gmbus_get_adapter(
+		struct drm_i915_private *dev_priv, unsigned port);
 extern void intel_gmbus_set_speed(struct i2c_adapter *adapter, int speed);
 extern void intel_gmbus_force_bit(struct i2c_adapter *adapter, bool force_bit);
 extern inline bool intel_gmbus_is_forced_bit(struct i2c_adapter *adapter)
diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
index e4317da..871aa27 100644
--- a/drivers/gpu/drm/i915/intel_bios.c
+++ b/drivers/gpu/drm/i915/intel_bios.c
@@ -372,11 +372,11 @@ parse_general_definitions(struct drm_i915_private *dev_priv,
 		if (block_size >= sizeof(*general)) {
 			int bus_pin = general->crt_ddc_gmbus_pin;
 			DRM_DEBUG_KMS("crt_ddc_bus_pin: %d\n", bus_pin);
-			if (bus_pin >= 1 && bus_pin <= 6)
+			if (intel_gmbus_is_port_valid(bus_pin))
 				dev_priv->crt_ddc_pin = bus_pin;
 		} else {
 			DRM_DEBUG_KMS("BDB_GD too small (%d). Invalid.\n",
-				  block_size);
+				      block_size);
 		}
 	}
 }
diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
index 4d3d736..d54d232 100644
--- a/drivers/gpu/drm/i915/intel_crt.c
+++ b/drivers/gpu/drm/i915/intel_crt.c
@@ -278,9 +278,10 @@ static bool intel_crt_detect_ddc(struct drm_connector *connector)
 	if (intel_ddc_probe(&crt->base, dev_priv->crt_ddc_pin)) {
 		struct edid *edid;
 		bool is_digital = false;
+		struct i2c_adapter *i2c;
 
-		edid = drm_get_edid(connector,
-			&dev_priv->gmbus[dev_priv->crt_ddc_pin].adapter);
+		i2c = intel_gmbus_get_adapter(dev_priv, dev_priv->crt_ddc_pin);
+		edid = drm_get_edid(connector, i2c);
 		/*
 		 * This may be a DVI-I connector with a shared DDC
 		 * link between analog and digital outputs, so we
@@ -483,15 +484,16 @@ static int intel_crt_get_modes(struct drm_connector *connector)
 	struct drm_device *dev = connector->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	int ret;
+	struct i2c_adapter *i2c;
 
-	ret = intel_ddc_get_modes(connector,
-				 &dev_priv->gmbus[dev_priv->crt_ddc_pin].adapter);
+	i2c = intel_gmbus_get_adapter(dev_priv, dev_priv->crt_ddc_pin);
+	ret = intel_ddc_get_modes(connector, i2c);
 	if (ret || !IS_G4X(dev))
 		return ret;
 
 	/* Try to probe digital port for output in DVI-I -> VGA mode. */
-	return intel_ddc_get_modes(connector,
-				   &dev_priv->gmbus[GMBUS_PORT_DPB].adapter);
+	i2c = intel_gmbus_get_adapter(dev_priv, GMBUS_PORT_DPB);
+	return intel_ddc_get_modes(connector, i2c);
 }
 
 static int intel_crt_set_property(struct drm_connector *connector,
diff --git a/drivers/gpu/drm/i915/intel_dvo.c b/drivers/gpu/drm/i915/intel_dvo.c
index 020a7d7..60ba50b9 100644
--- a/drivers/gpu/drm/i915/intel_dvo.c
+++ b/drivers/gpu/drm/i915/intel_dvo.c
@@ -243,7 +243,7 @@ static int intel_dvo_get_modes(struct drm_connector *connector)
 	 * that's not the case.
 	 */
 	intel_ddc_get_modes(connector,
-			    &dev_priv->gmbus[GMBUS_PORT_DPC].adapter);
+			    intel_gmbus_get_adapter(dev_priv, GMBUS_PORT_DPC));
 	if (!list_empty(&connector->probed_modes))
 		return 1;
 
@@ -375,7 +375,7 @@ void intel_dvo_init(struct drm_device *dev)
 		 * special cases, but otherwise default to what's defined
 		 * in the spec.
 		 */
-		if (dvo->gpio != 0)
+		if (intel_gmbus_is_port_valid(dvo->gpio))
 			gpio = dvo->gpio;
 		else if (dvo->type == INTEL_DVO_CHIP_LVDS)
 			gpio = GMBUS_PORT_SSC;
@@ -386,7 +386,7 @@ void intel_dvo_init(struct drm_device *dev)
 		 * It appears that everything is on GPIOE except for panels
 		 * on i830 laptops, which are on GPIOB (DVOA).
 		 */
-		i2c = &dev_priv->gmbus[gpio].adapter;
+		i2c = intel_gmbus_get_adapter(dev_priv, gpio);
 
 		intel_dvo->dev = *dvo;
 		if (!dvo->dev_ops->init(&intel_dvo->dev, i2c))
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index cae3e5f..1d00f61 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -334,7 +334,8 @@ intel_hdmi_detect(struct drm_connector *connector, bool force)
 	intel_hdmi->has_hdmi_sink = false;
 	intel_hdmi->has_audio = false;
 	edid = drm_get_edid(connector,
-			    &dev_priv->gmbus[intel_hdmi->ddc_bus].adapter);
+			    intel_gmbus_get_adapter(dev_priv,
+						    intel_hdmi->ddc_bus));
 
 	if (edid) {
 		if (edid->input & DRM_EDID_INPUT_DIGITAL) {
@@ -367,7 +368,8 @@ static int intel_hdmi_get_modes(struct drm_connector *connector)
 	 */
 
 	return intel_ddc_get_modes(connector,
-				   &dev_priv->gmbus[intel_hdmi->ddc_bus].adapter);
+				   intel_gmbus_get_adapter(dev_priv,
+							   intel_hdmi->ddc_bus));
 }
 
 static bool
@@ -379,7 +381,8 @@ intel_hdmi_detect_audio(struct drm_connector *connector)
 	bool has_audio = false;
 
 	edid = drm_get_edid(connector,
-			    &dev_priv->gmbus[intel_hdmi->ddc_bus].adapter);
+			    intel_gmbus_get_adapter(dev_priv,
+						    intel_hdmi->ddc_bus));
 	if (edid) {
 		if (edid->input & DRM_EDID_INPUT_DIGITAL)
 			has_audio = drm_detect_monitor_audio(edid);
diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
index 1bb6362..2f65d01 100644
--- a/drivers/gpu/drm/i915/intel_i2c.c
+++ b/drivers/gpu/drm/i915/intel_i2c.c
@@ -449,6 +449,14 @@ err:
 	return ret;
 }
 
+struct i2c_adapter *intel_gmbus_get_adapter(struct drm_i915_private *dev_priv,
+					    unsigned port)
+{
+	WARN_ON(!intel_gmbus_is_port_valid(port));
+	return (intel_gmbus_is_port_valid(port)) ?
+		&dev_priv->gmbus[port].adapter : NULL;
+}
+
 void intel_gmbus_set_speed(struct i2c_adapter *adapter, int speed)
 {
 	struct intel_gmbus *bus = to_intel_gmbus(adapter);
diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
index c5c0973..4f92a11 100644
--- a/drivers/gpu/drm/i915/intel_lvds.c
+++ b/drivers/gpu/drm/i915/intel_lvds.c
@@ -837,8 +837,8 @@ static bool lvds_is_present_in_vbt(struct drm_device *dev,
 		    child->device_type != DEVICE_TYPE_LFP)
 			continue;
 
-		if (child->i2c_pin)
-		    *i2c_pin = child->i2c_pin;
+		if (intel_gmbus_is_port_valid(child->i2c_pin))
+			*i2c_pin = child->i2c_pin;
 
 		/* However, we cannot trust the BIOS writers to populate
 		 * the VBT correctly.  Since LVDS requires additional
@@ -979,7 +979,8 @@ bool intel_lvds_init(struct drm_device *dev)
 	 * preferred mode is the right one.
 	 */
 	intel_lvds->edid = drm_get_edid(connector,
-					&dev_priv->gmbus[pin].adapter);
+					intel_gmbus_get_adapter(dev_priv,
+								pin));
 	if (intel_lvds->edid) {
 		if (drm_add_edid_modes(connector,
 				       intel_lvds->edid)) {
diff --git a/drivers/gpu/drm/i915/intel_modes.c b/drivers/gpu/drm/i915/intel_modes.c
index 2978a3f..cc682a0 100644
--- a/drivers/gpu/drm/i915/intel_modes.c
+++ b/drivers/gpu/drm/i915/intel_modes.c
@@ -55,7 +55,8 @@ bool intel_ddc_probe(struct intel_encoder *intel_encoder, int ddc_bus)
 		}
 	};
 
-	return i2c_transfer(&dev_priv->gmbus[ddc_bus].adapter, msgs, 2) == 2;
+	return i2c_transfer(intel_gmbus_get_adapter(dev_priv, ddc_bus),
+			    msgs, 2) == 2;
 }
 
 /**
diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
index 70fb275..830c8b5 100644
--- a/drivers/gpu/drm/i915/intel_sdvo.c
+++ b/drivers/gpu/drm/i915/intel_sdvo.c
@@ -1254,7 +1254,8 @@ intel_sdvo_get_analog_edid(struct drm_connector *connector)
 	struct drm_i915_private *dev_priv = connector->dev->dev_private;
 
 	return drm_get_edid(connector,
-			    &dev_priv->gmbus[dev_priv->crt_ddc_pin].adapter);
+			    intel_gmbus_get_adapter(dev_priv,
+						    dev_priv->crt_ddc_pin));
 }
 
 enum drm_connector_status
@@ -1922,12 +1923,12 @@ intel_sdvo_select_i2c_bus(struct drm_i915_private *dev_priv,
 	if (mapping->initialized)
 		pin = mapping->i2c_pin;
 
-	if (pin < GMBUS_NUM_PORTS) {
-		sdvo->i2c = &dev_priv->gmbus[pin].adapter;
+	if (intel_gmbus_is_port_valid(pin)) {
+		sdvo->i2c = intel_gmbus_get_adapter(dev_priv, pin);
 		intel_gmbus_set_speed(sdvo->i2c, GMBUS_RATE_1MHZ);
 		intel_gmbus_force_bit(sdvo->i2c, true);
 	} else {
-		sdvo->i2c = &dev_priv->gmbus[GMBUS_PORT_DPB].adapter;
+		sdvo->i2c = intel_gmbus_get_adapter(dev_priv, GMBUS_PORT_DPB);
 	}
 }
 
-- 
1.7.7.3

[PATCH 06/13 v4] drm/i915/intel_i2c: gmbus disabled and reserved ports are invalid

[Daniel Kurtz]

There is no GMBUS "disabled" port 0, nor "reserved" port 7.
For the other 6 ports there is a fixed 1:1 mapping between pin pairs and
gmbus ports, which means every real gmbus port has a gpio pin.

Given these realizations, clean up gmbus initialization.

Tested on Sandybridge (gen 6, PCH == CougarPoint) hardware.

Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
---
 drivers/gpu/drm/i915/i915_drv.h  |    3 +-
 drivers/gpu/drm/i915/i915_reg.h  |    2 +-
 drivers/gpu/drm/i915/intel_i2c.c |   70 ++++++++++++++-----------------------
 3 files changed, 29 insertions(+), 46 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 44e6430..c5ad7b9 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -303,7 +303,6 @@ struct intel_fbc_work;
 struct intel_gmbus {
 	struct i2c_adapter adapter;
 	bool force_bit;
-	bool has_gpio;
 	u32 reg0;
 	u32 gpio_reg;
 	struct i2c_algo_bit_data bit_algo;
@@ -1344,7 +1343,7 @@ extern int intel_setup_gmbus(struct drm_device *dev);
 extern void intel_teardown_gmbus(struct drm_device *dev);
 extern inline bool intel_gmbus_is_port_valid(unsigned port)
 {
-	return (port >= GMBUS_PORT_DISABLED && port <= GMBUS_PORT_RESERVED);
+	return (port >= GMBUS_PORT_SSC && port <= GMBUS_PORT_DPD);
 }
 
 extern struct i2c_adapter *intel_gmbus_get_adapter(
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index accd8ee..a8d218c 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -744,7 +744,7 @@
 #define   GMBUS_PORT_DPB	5 /* SDVO, HDMIB */
 #define   GMBUS_PORT_DPD	6 /* HDMID */
 #define   GMBUS_PORT_RESERVED	7 /* 7 reserved */
-#define   GMBUS_NUM_PORTS	8
+#define   GMBUS_NUM_PORTS	(GMBUS_PORT_DPD - GMBUS_PORT_SSC + 1)
 #define GMBUS1			0x5104 /* command/status */
 #define   GMBUS_SW_CLR_INT	(1<<31)
 #define   GMBUS_SW_RDY		(1<<30)
diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
index 2f65d01..dcde6f6 100644
--- a/drivers/gpu/drm/i915/intel_i2c.c
+++ b/drivers/gpu/drm/i915/intel_i2c.c
@@ -35,6 +35,20 @@
 #include "i915_drm.h"
 #include "i915_drv.h"
 
+struct gmbus_port {
+	const char *name;
+	int reg;
+};
+
+static const struct gmbus_port gmbus_ports[] = {
+	{ "ssc", GPIOB },
+	{ "vga", GPIOA },
+	{ "panel", GPIOC },
+	{ "dpc", GPIOD },
+	{ "dpb", GPIOE },
+	{ "dpd", GPIOF },
+};
+
 /* Intel GPIO access functions */
 
 #define I2C_RISEFALL_TIME 10
@@ -166,29 +180,16 @@ intel_gpio_post_xfer(struct i2c_adapter *adapter)
 	intel_i2c_quirk_set(dev_priv, false);
 }
 
-static bool
+static void
 intel_gpio_setup(struct intel_gmbus *bus, u32 pin)
 {
 	struct drm_i915_private *dev_priv = bus->dev_priv;
-	static const int map_pin_to_reg[] = {
-		0,
-		GPIOB,
-		GPIOA,
-		GPIOC,
-		GPIOD,
-		GPIOE,
-		GPIOF,
-		0,
-	};
 	struct i2c_algo_bit_data *algo;
 
-	if (pin >= ARRAY_SIZE(map_pin_to_reg) || !map_pin_to_reg[pin])
-		return false;
-
 	algo = &bus->bit_algo;
 
-	bus->gpio_reg = map_pin_to_reg[pin];
-	bus->gpio_reg += dev_priv->gpio_mmio_base;
+	/* -1 to map pin pair to gmbus index */
+	bus->gpio_reg = dev_priv->gpio_mmio_base + gmbus_ports[pin - 1].reg;
 
 	bus->adapter.algo_data = algo;
 	algo->setsda = set_data;
@@ -200,8 +201,6 @@ intel_gpio_setup(struct intel_gmbus *bus, u32 pin)
 	algo->udelay = I2C_RISEFALL_TIME;
 	algo->timeout = usecs_to_jiffies(2200);
 	algo->data = bus;
-
-	return true;
 }
 
 static int
@@ -351,15 +350,9 @@ timeout:
 		 bus->adapter.name, bus->reg0 & 0xff);
 	I915_WRITE(GMBUS0 + reg_offset, 0);
 
-	/* Hardware may not support GMBUS over these pins?
-	 * Try GPIO bitbanging instead.
-	 */
-	if (!bus->has_gpio) {
-		ret = -EIO;
-	} else {
-		bus->force_bit = true;
-		ret = i2c_bit_algo.master_xfer(adapter, msgs, num);
-	}
+	/* Hardware may not support GMBUS over these pins? Try GPIO bitbanging instead. */
+	bus->force_bit = true;
+	ret = i2c_bit_algo.master_xfer(adapter, msgs, num);
 
 out:
 	mutex_unlock(&dev_priv->gmbus_mutex);
@@ -386,16 +379,6 @@ static const struct i2c_algorithm gmbus_algorithm = {
  */
 int intel_setup_gmbus(struct drm_device *dev)
 {
-	static const char *names[GMBUS_NUM_PORTS] = {
-		"disabled",
-		"ssc",
-		"vga",
-		"panel",
-		"dpc",
-		"dpb",
-		"dpd",
-		"reserved",
-	};
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	int ret, i;
 
@@ -413,13 +396,14 @@ int intel_setup_gmbus(struct drm_device *dev)
 
 	for (i = 0; i < GMBUS_NUM_PORTS; i++) {
 		struct intel_gmbus *bus = &dev_priv->gmbus[i];
+		u32 port = i + 1; /* +1 to map gmbus index to pin pair */
 
 		bus->adapter.owner = THIS_MODULE;
 		bus->adapter.class = I2C_CLASS_DDC;
 		snprintf(bus->adapter.name,
 			 sizeof(bus->adapter.name),
 			 "i915 gmbus %s",
-			 names[i]);
+			 gmbus_ports[i].name);
 
 		bus->adapter.dev.parent = &dev->pdev->dev;
 		bus->dev_priv = dev_priv;
@@ -430,9 +414,9 @@ int intel_setup_gmbus(struct drm_device *dev)
 			goto err;
 
 		/* By default use a conservative clock rate */
-		bus->reg0 = i | GMBUS_RATE_100KHZ;
+		bus->reg0 = port | GMBUS_RATE_100KHZ;
 
-		bus->has_gpio = intel_gpio_setup(bus, i);
+		intel_gpio_setup(bus, port);
 	}
 
 	intel_i2c_reset(dev_priv->dev);
@@ -453,8 +437,9 @@ struct i2c_adapter *intel_gmbus_get_adapter(struct drm_i915_private *dev_priv,
 					    unsigned port)
 {
 	WARN_ON(!intel_gmbus_is_port_valid(port));
+	/* -1 to map pin pair to gmbus index */
 	return (intel_gmbus_is_port_valid(port)) ?
-		&dev_priv->gmbus[port].adapter : NULL;
+		&dev_priv->gmbus[port - 1].adapter : NULL;
 }
 
 void intel_gmbus_set_speed(struct i2c_adapter *adapter, int speed)
@@ -468,8 +453,7 @@ void intel_gmbus_force_bit(struct i2c_adapter *adapter, bool force_bit)
 {
 	struct intel_gmbus *bus = to_intel_gmbus(adapter);
 
-	if (bus->has_gpio)
-		bus->force_bit = force_bit;
+	bus->force_bit = force_bit;
 }
 
 void intel_teardown_gmbus(struct drm_device *dev)
-- 
1.7.7.3

[PATCH 07/13 v4] drm/i915/intel_i2c: allocate gmbus array as part of drm_i915_private

[Daniel Kurtz]

This memory is always allocated, and it is always a fixed size, so just
allocate it along with the rest of the driver state.

Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_drv.h  |    2 +-
 drivers/gpu/drm/i915/intel_i2c.c |   10 ----------
 2 files changed, 1 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index c5ad7b9..6983b4b 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -326,7 +326,7 @@ typedef struct drm_i915_private {
 	/** gt_lock is also taken in irq contexts. */
 	struct spinlock gt_lock;
 
-	struct intel_gmbus *gmbus;
+	struct intel_gmbus gmbus[GMBUS_NUM_PORTS];
 
 	/** gmbus_mutex protects against concurrent usage of the single hw gmbus
 	 * controller on different i2c buses. */
diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
index dcde6f6..c12db72 100644
--- a/drivers/gpu/drm/i915/intel_i2c.c
+++ b/drivers/gpu/drm/i915/intel_i2c.c
@@ -387,11 +387,6 @@ int intel_setup_gmbus(struct drm_device *dev)
 	else
 		dev_priv->gpio_mmio_base = 0;
 
-	dev_priv->gmbus = kcalloc(GMBUS_NUM_PORTS, sizeof(struct intel_gmbus),
-				  GFP_KERNEL);
-	if (dev_priv->gmbus == NULL)
-		return -ENOMEM;
-
 	mutex_init(&dev_priv->gmbus_mutex);
 
 	for (i = 0; i < GMBUS_NUM_PORTS; i++) {
@@ -428,8 +423,6 @@ err:
 		struct intel_gmbus *bus = &dev_priv->gmbus[i];
 		i2c_del_adapter(&bus->adapter);
 	}
-	kfree(dev_priv->gmbus);
-	dev_priv->gmbus = NULL;
 	return ret;
 }
 
@@ -468,7 +461,4 @@ void intel_teardown_gmbus(struct drm_device *dev)
 		struct intel_gmbus *bus = &dev_priv->gmbus[i];
 		i2c_del_adapter(&bus->adapter);
 	}
-
-	kfree(dev_priv->gmbus);
-	dev_priv->gmbus = NULL;
 }
-- 
1.7.7.3

Re: [PATCH 07/13 v4] drm/i915/intel_i2c: allocate gmbus array as part of drm_i915_private

[Daniel Vetter]

On Wed, Mar 28, 2012 at 02:36:16AM +0800, Daniel Kurtz wrote:
> This memory is always allocated, and it is always a fixed size, so just
> allocate it along with the rest of the driver state.
> 
> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

I've picked up all your patches up and including this one for d-i-n,
thanks. I'm a bit unhappy about the gmbus to gmbus pin off-by-one stuff in
the previous patch, but I could not come up with a better way to do it.
And merging these 2 enumerations into one sounds like a good idea, so I've
picked it up.
-Daniel
> ---
>  drivers/gpu/drm/i915/i915_drv.h  |    2 +-
>  drivers/gpu/drm/i915/intel_i2c.c |   10 ----------
>  2 files changed, 1 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index c5ad7b9..6983b4b 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -326,7 +326,7 @@ typedef struct drm_i915_private {
>  	/** gt_lock is also taken in irq contexts. */
>  	struct spinlock gt_lock;
>  
> -	struct intel_gmbus *gmbus;
> +	struct intel_gmbus gmbus[GMBUS_NUM_PORTS];
>  
>  	/** gmbus_mutex protects against concurrent usage of the single hw gmbus
>  	 * controller on different i2c buses. */
> diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
> index dcde6f6..c12db72 100644
> --- a/drivers/gpu/drm/i915/intel_i2c.c
> +++ b/drivers/gpu/drm/i915/intel_i2c.c
> @@ -387,11 +387,6 @@ int intel_setup_gmbus(struct drm_device *dev)
>  	else
>  		dev_priv->gpio_mmio_base = 0;
>  
> -	dev_priv->gmbus = kcalloc(GMBUS_NUM_PORTS, sizeof(struct intel_gmbus),
> -				  GFP_KERNEL);
> -	if (dev_priv->gmbus == NULL)
> -		return -ENOMEM;
> -
>  	mutex_init(&dev_priv->gmbus_mutex);
>  
>  	for (i = 0; i < GMBUS_NUM_PORTS; i++) {
> @@ -428,8 +423,6 @@ err:
>  		struct intel_gmbus *bus = &dev_priv->gmbus[i];
>  		i2c_del_adapter(&bus->adapter);
>  	}
> -	kfree(dev_priv->gmbus);
> -	dev_priv->gmbus = NULL;
>  	return ret;
>  }
>  
> @@ -468,7 +461,4 @@ void intel_teardown_gmbus(struct drm_device *dev)
>  		struct intel_gmbus *bus = &dev_priv->gmbus[i];
>  		i2c_del_adapter(&bus->adapter);
>  	}
> -
> -	kfree(dev_priv->gmbus);
> -	dev_priv->gmbus = NULL;
>  }
> -- 
> 1.7.7.3
> 

-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

[PATCH 08/13 v4] drm/i915/intel_i2c: handle zero-length writes

[Daniel Kurtz]

A common method of probing an i2c bus is trying to do a zero-length write.
Handle this case by checking the length first before decrementing it.

This is actually important, since attempting a zero-length write is one
of the ways that i2cdetect and i2c_new_probed_device detect whether
there is device present on the bus with a given address.

Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
---
 drivers/gpu/drm/i915/intel_i2c.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
index c12db72..5a94e9b 100644
--- a/drivers/gpu/drm/i915/intel_i2c.c
+++ b/drivers/gpu/drm/i915/intel_i2c.c
@@ -248,9 +248,10 @@ gmbus_xfer_write(struct drm_i915_private *dev_priv, struct i2c_msg *msg,
 	u32 val, loop;
 
 	val = loop = 0;
-	do {
+	while (len && loop < 4) {
 		val |= *buf++ << (8 * loop);
-	} while (--len && ++loop < 4);
+		len -= 1;
+	}
 
 	I915_WRITE(GMBUS3 + reg_offset, val);
 	I915_WRITE(GMBUS1 + reg_offset,
-- 
1.7.7.3

Re: [PATCH 08/13 v4] drm/i915/intel_i2c: handle zero-length writes

[Chris Wilson]

On Wed, 28 Mar 2012 02:36:17 +0800, Daniel Kurtz <djkurtz@chromium.org> wrote:
> A common method of probing an i2c bus is trying to do a zero-length write.
> Handle this case by checking the length first before decrementing it.
> 
> This is actually important, since attempting a zero-length write is one
> of the ways that i2cdetect and i2c_new_probed_device detect whether
> there is device present on the bus with a given address.
> 
> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
> ---
>  drivers/gpu/drm/i915/intel_i2c.c |    5 +++--
>  1 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
> index c12db72..5a94e9b 100644
> --- a/drivers/gpu/drm/i915/intel_i2c.c
> +++ b/drivers/gpu/drm/i915/intel_i2c.c
> @@ -248,9 +248,10 @@ gmbus_xfer_write(struct drm_i915_private *dev_priv, struct i2c_msg *msg,
>  	u32 val, loop;
>  
>  	val = loop = 0;
> -	do {
> +	while (len && loop < 4) {
while (len-- && loop < 4)

>  		val |= *buf++ << (8 * loop);
> -	} while (--len && ++loop < 4);
> +		len -= 1;
Otherwise this looks too pythonesque ;-)

> +	}

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 08/13 v4] drm/i915/intel_i2c: handle zero-length writes

[Daniel Kurtz]

On Wed, Mar 28, 2012 at 3:14 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Wed, 28 Mar 2012 02:36:17 +0800, Daniel Kurtz <djkurtz@chromium.org> wrote:
>> A common method of probing an i2c bus is trying to do a zero-length write.
>> Handle this case by checking the length first before decrementing it.
>>
>> This is actually important, since attempting a zero-length write is one
>> of the ways that i2cdetect and i2c_new_probed_device detect whether
>> there is device present on the bus with a given address.
>>
>> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
>> ---
>>  drivers/gpu/drm/i915/intel_i2c.c |    5 +++--
>>  1 files changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
>> index c12db72..5a94e9b 100644
>> --- a/drivers/gpu/drm/i915/intel_i2c.c
>> +++ b/drivers/gpu/drm/i915/intel_i2c.c
>> @@ -248,9 +248,10 @@ gmbus_xfer_write(struct drm_i915_private *dev_priv, struct i2c_msg *msg,
>>       u32 val, loop;
>>
>>       val = loop = 0;
>> -     do {
>> +     while (len && loop < 4) {
> while (len-- && loop < 4)
>
>>               val |= *buf++ << (8 * loop);
>> -     } while (--len && ++loop < 4);
>> +             len -= 1;
> Otherwise this looks too pythonesque ;-)

Unfortunately there is a bug in both my original patch (it wasn't
incrementing loop), and in your suggested cleanup (it always
decrements len, even when it is already 0... or if the loop test
fails).  How about the following; despite its pythonic nature, it
always completes with len holding the number of bytes remaining.

	val = loop = 0;
	while (len && loop < 4) {
		val |= *buf++ << (8 * loop++);
		len -= 1;
	}

>> +     }
>
> --
> Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 08/13 v4] drm/i915/intel_i2c: handle zero-length writes

[Chris Wilson]

On Wed, 28 Mar 2012 19:32:27 +0800, Daniel Kurtz <djkurtz@chromium.org> wrote:
> Unfortunately there is a bug in both my original patch (it wasn't
> incrementing loop), and in your suggested cleanup (it always
> decrements len, even when it is already 0... or if the loop test
> fails).  How about the following; despite its pythonic nature, it
> always completes with len holding the number of bytes remaining.
> 
> 	val = loop = 0;
> 	while (len && loop < 4) {
> 		val |= *buf++ << (8 * loop++);
> 		len -= 1;
> 	}

I'm getting over my aversion ;-)

As we are here, have you considered doing an optimised u32 variant whilst
len >= 4, which should be reasonably common during EDID transfer. Though
I am not sure if that loop is merely in the noise compared to the rest of
the mmio.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH 09/13 v4] drm/i915/intel_i2c: use double-buffered writes

[Daniel Kurtz]

The GMBUS controller GMBUS3 register is double-buffered.  Take advantage
of this  by writing two 4-byte words before the first wait for HW_RDY.
This helps keep the GMBUS controller from becoming idle during long writes.

Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
---
 drivers/gpu/drm/i915/intel_i2c.c |   14 +++++++-------
 1 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
index 5a94e9b..c576e02 100644
--- a/drivers/gpu/drm/i915/intel_i2c.c
+++ b/drivers/gpu/drm/i915/intel_i2c.c
@@ -262,13 +262,6 @@ 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) {
-		if (wait_for(I915_READ(GMBUS2 + reg_offset) &
-			     (GMBUS_SATOER | GMBUS_HW_RDY),
-			     50))
-			return -ETIMEDOUT;
-		if (I915_READ(GMBUS2 + reg_offset) & GMBUS_SATOER)
-			return -ENXIO;
-
 		val = loop = 0;
 		do {
 			val |= *buf++ << (8 * loop);
@@ -276,6 +269,13 @@ gmbus_xfer_write(struct drm_i915_private *dev_priv, struct i2c_msg *msg,
 
 		I915_WRITE(GMBUS3 + reg_offset, val);
 		POSTING_READ(GMBUS2 + reg_offset);
+
+		if (wait_for(I915_READ(GMBUS2 + reg_offset) &
+			     (GMBUS_SATOER | GMBUS_HW_RDY),
+			     50))
+			return -ETIMEDOUT;
+		if (I915_READ(GMBUS2 + reg_offset) & GMBUS_SATOER)
+			return -ENXIO;
 	}
 	return 0;
 }
-- 
1.7.7.3

Re: [PATCH 09/13 v4] drm/i915/intel_i2c: use double-buffered writes

[Chris Wilson]

On Wed, 28 Mar 2012 02:36:18 +0800, Daniel Kurtz <djkurtz@chromium.org> wrote:
> @@ -276,6 +269,13 @@ gmbus_xfer_write(struct drm_i915_private *dev_priv, struct i2c_msg *msg,
>  
>  		I915_WRITE(GMBUS3 + reg_offset, val);
>  		POSTING_READ(GMBUS2 + reg_offset);

You might as well squash this posting read as the write flush is
redundant given the following read of GMBUS2.

> +
> +		if (wait_for(I915_READ(GMBUS2 + reg_offset) &
> +			     (GMBUS_SATOER | GMBUS_HW_RDY),
> +			     50))
> +			return -ETIMEDOUT;
> +		if (I915_READ(GMBUS2 + reg_offset) & GMBUS_SATOER)
> +			return -ENXIO;

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH 10/13 v4] drm/i915/intel_i2c: always wait for IDLE before clearing NAK

[Daniel Kurtz]

The GMBUS controller can report a NAK condition while a transaction is
still active. If the driver is fast enough, and the bus is slow enough,
the driver may clear the NAK condition while the controller is still
busy, resulting in a confused GMBUS controller.  This will leave the
controller in a bad state such that the next transaction may fail.

Also, return -ENXIO if a device NAKs a transaction.

Note: this patch also refactors gmbus_xfer to remove the "done" label.

Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
---
 drivers/gpu/drm/i915/intel_i2c.c |   41 ++++++++++++++++++++++++++++---------
 1 files changed, 31 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
index c576e02..1a5f52a 100644
--- a/drivers/gpu/drm/i915/intel_i2c.c
+++ b/drivers/gpu/drm/i915/intel_i2c.c
@@ -324,26 +324,47 @@ gmbus_xfer(struct i2c_adapter *adapter,
 			goto clear_err;
 	}
 
-	goto done;
+	/* Mark the GMBUS interface as disabled after waiting for idle.
+	 * We will re-enable it at the start of the next xfer,
+	 * till then let it sleep.
+	 */
+	if (wait_for((I915_READ(GMBUS2 + reg_offset) & GMBUS_ACTIVE) == 0, 10))
+		DRM_INFO("GMBUS [%s] timed out waiting for idle\n",
+			 adapter->name);
+	I915_WRITE(GMBUS0 + reg_offset, 0);
+	ret = i;
+	goto out;
 
 clear_err:
+	/*
+	 * Wait for bus to IDLE before clearing NAK.
+	 * If we clear the NAK while bus is still active, then it will stay
+	 * active and the next transaction may fail.
+	 */
+	if (wait_for((I915_READ(GMBUS2 + reg_offset) & GMBUS_ACTIVE) == 0,
+		     10))
+		DRM_INFO("GMBUS [%s] timed out after NAK\n", adapter->name);
+
 	/* Toggle the Software Clear Interrupt bit. This has the effect
 	 * 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(GMBUS0 + reg_offset, 0);
 
-done:
-	/* Mark the GMBUS interface as disabled after waiting for idle.
-	 * We will re-enable it at the start of the next xfer,
-	 * till then let it sleep.
+	DRM_DEBUG_DRIVER("GMBUS [%s] NAK for addr: %04x %c(%d)\n",
+			 adapter->name, msgs[i].addr,
+			 (msgs[i].flags & I2C_M_RD) ? 'r' : 'w', msgs[i].len);
+
+	/*
+	 * If no ACK is received during the address phase of a transaction,
+	 * the adapter must report -ENXIO.
+	 * It is not clear what to return if no ACK is received at other times.
+	 * So, we always return -ENXIO in all NAK cases, to ensure we send
+	 * it at least during the one case that is specified.
 	 */
-	if (wait_for((I915_READ(GMBUS2 + reg_offset) & GMBUS_ACTIVE) == 0, 10))
-		DRM_INFO("GMBUS [%s] timed out waiting for idle\n",
-			 bus->adapter.name);
-	I915_WRITE(GMBUS0 + reg_offset, 0);
-	ret = i;
+	ret = -ENXIO;
 	goto out;
 
 timeout:
-- 
1.7.7.3

Re: [PATCH 10/13 v4] drm/i915/intel_i2c: always wait for IDLE before clearing NAK

[Chris Wilson]

On Wed, 28 Mar 2012 02:36:19 +0800, Daniel Kurtz <djkurtz@chromium.org> wrote:
> The GMBUS controller can report a NAK condition while a transaction is
> still active. If the driver is fast enough, and the bus is slow enough,
> the driver may clear the NAK condition while the controller is still
> busy, resulting in a confused GMBUS controller.  This will leave the
> controller in a bad state such that the next transaction may fail.
> 
> Also, return -ENXIO if a device NAKs a transaction.
> 
> Note: this patch also refactors gmbus_xfer to remove the "done" label.
> 
> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>

Thanks for clarifying the appropriate return codes.

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH 11/13 v4] drm/i915/intel_i2c: use WAIT cycle, not STOP

[Daniel Kurtz]

The i915 is only able to generate a STOP cycle (i.e. finalize an i2c
transaction) during a DATA or WAIT phase.  In other words, the
controller rejects a STOP requested as part of the first transaction in a
sequence.

Thus, for the first transaction we must always use a WAIT cycle, detect
when the device has finished (and is in a WAIT phase), and then either
start the next transaction, or, if there are no more transactions,
generate a STOP cycle.

Note: Theoretically, the last transaction of a multi-transaction sequence
could initiate a STOP cycle.  However, this slight optimization is left
for another patch.  We return -ETIMEDOUT if the hardware doesn't
deactivate after the STOP cycle.

Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
---
 drivers/gpu/drm/i915/intel_i2c.c |   30 +++++++++++++++---------------
 1 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
index 1a5f52a..43a3ca3 100644
--- a/drivers/gpu/drm/i915/intel_i2c.c
+++ b/drivers/gpu/drm/i915/intel_i2c.c
@@ -204,8 +204,7 @@ intel_gpio_setup(struct intel_gmbus *bus, u32 pin)
 }
 
 static int
-gmbus_xfer_read(struct drm_i915_private *dev_priv, struct i2c_msg *msg,
-		bool last)
+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;
@@ -213,7 +212,6 @@ gmbus_xfer_read(struct drm_i915_private *dev_priv, struct i2c_msg *msg,
 
 	I915_WRITE(GMBUS1 + reg_offset,
 		   GMBUS_CYCLE_WAIT |
-		   (last ? GMBUS_CYCLE_STOP : 0) |
 		   (len << GMBUS_BYTE_COUNT_SHIFT) |
 		   (msg->addr << GMBUS_SLAVE_ADDR_SHIFT) |
 		   GMBUS_SLAVE_READ | GMBUS_SW_RDY);
@@ -239,8 +237,7 @@ gmbus_xfer_read(struct drm_i915_private *dev_priv, struct i2c_msg *msg,
 }
 
 static int
-gmbus_xfer_write(struct drm_i915_private *dev_priv, struct i2c_msg *msg,
-		bool last)
+gmbus_xfer_write(struct drm_i915_private *dev_priv, struct i2c_msg *msg)
 {
 	int reg_offset = dev_priv->gpio_mmio_base;
 	u16 len = msg->len;
@@ -256,7 +253,6 @@ gmbus_xfer_write(struct drm_i915_private *dev_priv, struct i2c_msg *msg,
 	I915_WRITE(GMBUS3 + reg_offset, val);
 	I915_WRITE(GMBUS1 + reg_offset,
 		   GMBUS_CYCLE_WAIT |
-		   (last ? GMBUS_CYCLE_STOP : 0) |
 		   (msg->len << GMBUS_BYTE_COUNT_SHIFT) |
 		   (msg->addr << GMBUS_SLAVE_ADDR_SHIFT) |
 		   GMBUS_SLAVE_WRITE | GMBUS_SW_RDY);
@@ -289,7 +285,8 @@ gmbus_xfer(struct i2c_adapter *adapter,
 					       struct intel_gmbus,
 					       adapter);
 	struct drm_i915_private *dev_priv = bus->dev_priv;
-	int i, reg_offset, ret;
+	int i, reg_offset;
+	int ret = 0;
 
 	mutex_lock(&dev_priv->gmbus_mutex);
 
@@ -303,20 +300,17 @@ gmbus_xfer(struct i2c_adapter *adapter,
 	I915_WRITE(GMBUS0 + reg_offset, bus->reg0);
 
 	for (i = 0; i < num; i++) {
-		bool last = i + 1 == num;
-
 		if (msgs[i].flags & I2C_M_RD)
-			ret = gmbus_xfer_read(dev_priv, &msgs[i], last);
+			ret = gmbus_xfer_read(dev_priv, &msgs[i]);
 		else
-			ret = gmbus_xfer_write(dev_priv, &msgs[i], last);
+			ret = gmbus_xfer_write(dev_priv, &msgs[i]);
 
 		if (ret == -ETIMEDOUT)
 			goto timeout;
 		if (ret == -ENXIO)
 			goto clear_err;
 
-		if (!last &&
-		    wait_for(I915_READ(GMBUS2 + reg_offset) &
+		if (wait_for(I915_READ(GMBUS2 + reg_offset) &
 			     (GMBUS_SATOER | GMBUS_HW_WAIT_PHASE),
 			     50))
 			goto timeout;
@@ -324,15 +318,21 @@ gmbus_xfer(struct i2c_adapter *adapter,
 			goto clear_err;
 	}
 
+	/* Generate a STOP condition on the bus */
+	I915_WRITE(GMBUS1 + reg_offset, GMBUS_CYCLE_STOP | GMBUS_SW_RDY);
+
 	/* Mark the GMBUS interface as disabled after waiting for idle.
 	 * We will re-enable it at the start of the next xfer,
 	 * till then let it sleep.
 	 */
-	if (wait_for((I915_READ(GMBUS2 + reg_offset) & GMBUS_ACTIVE) == 0, 10))
+	if (wait_for((I915_READ(GMBUS2 + reg_offset) & GMBUS_ACTIVE) == 0,
+		     10)) {
 		DRM_INFO("GMBUS [%s] timed out waiting for idle\n",
 			 adapter->name);
+		ret = -ETIMEDOUT;
+	}
 	I915_WRITE(GMBUS0 + reg_offset, 0);
-	ret = i;
+	ret = ret ?: i;
 	goto out;
 
 clear_err:
-- 
1.7.7.3

[PATCH 12/13 v4] drm/i915/intel_i2c: use INDEX cycles for i2c read transactions

[Daniel Kurtz]

It is very common for an i2c device to require a small 1 or 2 byte write
followed by a read.  For example, when reading from an i2c EEPROM it is
common to write and address, offset or index followed by a reading some
values.

The i915 gmbus controller provides a special "INDEX" cycle for performing
such a small write followed by a read.  The INDEX can be either one or two
bytes long.  The advantage of using such a cycle is that the CPU has
slightly less work to do once the read with INDEX cycle is started.

Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>
---
 drivers/gpu/drm/i915/intel_i2c.c |   32 ++++++++++++++++++++++++++++++--
 1 files changed, 30 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
index 43a3ca3..c71f3dc 100644
--- a/drivers/gpu/drm/i915/intel_i2c.c
+++ b/drivers/gpu/drm/i915/intel_i2c.c
@@ -204,13 +204,15 @@ intel_gpio_setup(struct intel_gmbus *bus, u32 pin)
 }
 
 static int
-gmbus_xfer_read(struct drm_i915_private *dev_priv, struct i2c_msg *msg)
+gmbus_xfer_read(struct drm_i915_private *dev_priv, struct i2c_msg *msg,
+		u32 gmbus1)
 {
 	int reg_offset = dev_priv->gpio_mmio_base;
 	u16 len = msg->len;
 	u8 *buf = msg->buf;
 
 	I915_WRITE(GMBUS1 + reg_offset,
+		   gmbus1 |
 		   GMBUS_CYCLE_WAIT |
 		   (len << GMBUS_BYTE_COUNT_SHIFT) |
 		   (msg->addr << GMBUS_SLAVE_ADDR_SHIFT) |
@@ -300,8 +302,34 @@ gmbus_xfer(struct i2c_adapter *adapter,
 	I915_WRITE(GMBUS0 + reg_offset, bus->reg0);
 
 	for (i = 0; i < num; i++) {
+		bool last = i + 1 == num;
+		u32 gmbus5 = 0;
+		u32 gmbus1 = 0;
+
+		/*
+		 * The gmbus controller can combine a 1 or 2 byte write with a
+		 * read that immediately follows it by using an "INDEX" cycle.
+		 */
+		if (!last &&
+		    !(msgs[i].flags & I2C_M_RD) &&
+		    (msgs[i + 1].flags & I2C_M_RD) &&
+		    msgs[i].len <= 2) {
+			if (msgs[i].len == 2)
+				gmbus5 = GMBUS_2BYTE_INDEX_EN |
+					 msgs[i].buf[1] |
+					 (msgs[i].buf[0] << 8);
+			if (msgs[i].len == 1)
+				gmbus1 = GMBUS_CYCLE_INDEX |
+					 (msgs[i].buf[0] <<
+					  GMBUS_SLAVE_INDEX_SHIFT);
+			i += 1;  /* set i to the index of the read xfer */
+		}
+
+		/* GMBUS5 holds 16-bit index, but must be 0 if not used */
+		I915_WRITE(GMBUS5 + reg_offset, gmbus5);
+
 		if (msgs[i].flags & I2C_M_RD)
-			ret = gmbus_xfer_read(dev_priv, &msgs[i]);
+			ret = gmbus_xfer_read(dev_priv, &msgs[i], gmbus1);
 		else
 			ret = gmbus_xfer_write(dev_priv, &msgs[i]);
 
-- 
1.7.7.3

Re: [PATCH 12/13 v4] drm/i915/intel_i2c: use INDEX cycles for i2c read transactions

[Daniel Vetter]

On Wed, Mar 28, 2012 at 02:36:21AM +0800, Daniel Kurtz wrote:
> It is very common for an i2c device to require a small 1 or 2 byte write
> followed by a read.  For example, when reading from an i2c EEPROM it is
> common to write and address, offset or index followed by a reading some
> values.
> 
> The i915 gmbus controller provides a special "INDEX" cycle for performing
> such a small write followed by a read.  The INDEX can be either one or two
> bytes long.  The advantage of using such a cycle is that the CPU has
> slightly less work to do once the read with INDEX cycle is started.
> 
> Signed-off-by: Daniel Kurtz <djkurtz@chromium.org>

Two minor bikesheds below.


> ---
>  drivers/gpu/drm/i915/intel_i2c.c |   32 ++++++++++++++++++++++++++++++--
>  1 files changed, 30 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
> index 43a3ca3..c71f3dc 100644
> --- a/drivers/gpu/drm/i915/intel_i2c.c
> +++ b/drivers/gpu/drm/i915/intel_i2c.c
> @@ -204,13 +204,15 @@ intel_gpio_setup(struct intel_gmbus *bus, u32 pin)
>  }
>  
>  static int
> -gmbus_xfer_read(struct drm_i915_private *dev_priv, struct i2c_msg *msg)
> +gmbus_xfer_read(struct drm_i915_private *dev_priv, struct i2c_msg *msg,
> +		u32 gmbus1)

Can we call this gmbus1_index to make it clear that this is just to
specifiy an index write in gmbus1 and not the entire thing?

>  {
>  	int reg_offset = dev_priv->gpio_mmio_base;
>  	u16 len = msg->len;
>  	u8 *buf = msg->buf;
>  
>  	I915_WRITE(GMBUS1 + reg_offset,
> +		   gmbus1 |
>  		   GMBUS_CYCLE_WAIT |
>  		   (len << GMBUS_BYTE_COUNT_SHIFT) |
>  		   (msg->addr << GMBUS_SLAVE_ADDR_SHIFT) |
> @@ -300,8 +302,34 @@ gmbus_xfer(struct i2c_adapter *adapter,
>  	I915_WRITE(GMBUS0 + reg_offset, bus->reg0);
>  
>  	for (i = 0; i < num; i++) {
> +		bool last = i + 1 == num;
> +		u32 gmbus5 = 0;
> +		u32 gmbus1 = 0;
> +
> +		/*
> +		 * The gmbus controller can combine a 1 or 2 byte write with a
> +		 * read that immediately follows it by using an "INDEX" cycle.
> +		 */
> +		if (!last &&
> +		    !(msgs[i].flags & I2C_M_RD) &&
> +		    (msgs[i + 1].flags & I2C_M_RD) &&
> +		    msgs[i].len <= 2) {
> +			if (msgs[i].len == 2)
> +				gmbus5 = GMBUS_2BYTE_INDEX_EN |
> +					 msgs[i].buf[1] |
> +					 (msgs[i].buf[0] << 8);
> +			if (msgs[i].len == 1)
> +				gmbus1 = GMBUS_CYCLE_INDEX |
> +					 (msgs[i].buf[0] <<
> +					  GMBUS_SLAVE_INDEX_SHIFT);
> +			i += 1;  /* set i to the index of the read xfer */
> +		}

Thas fallthrough is imo to clever code. What about extracting
gmbus_xfer_index_read which does all this and then directly calls
gmbus_xfer_read?

i.e.

if (complated condition for index read) {
	gmbus_xfer_index_read();
	i += 1; /* set i to the index of the read xfer */
} else if (is_read) {
	gmbus_xfer_read();
} else {
	gmbus_xfer_write();
}

This way we also don't need to clobber gmbus_xfer with the new gmbus1,
gmbus5 local variables.

> +
> +		/* GMBUS5 holds 16-bit index, but must be 0 if not used */
> +		I915_WRITE(GMBUS5 + reg_offset, gmbus5);

gmbus_xfer_index_read could then also clear gmbus5 after calling
gmbux_xfer_read.

> +
>  		if (msgs[i].flags & I2C_M_RD)
> -			ret = gmbus_xfer_read(dev_priv, &msgs[i]);
> +			ret = gmbus_xfer_read(dev_priv, &msgs[i], gmbus1);
>  		else
>  			ret = gmbus_xfer_write(dev_priv, &msgs[i]);
>  
> -- 
> 1.7.7.3
> 

-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

[PATCH 13/13 v4] drm/i915/intel_i2c: reuse GMBUS2 value read in polling loop

[Daniel Kurtz]

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;
 
 	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);
 	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);
 
-		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;
 	}
 	return 0;
@@ -289,6 +295,7 @@ gmbus_xfer(struct i2c_adapter *adapter,
 	struct drm_i915_private *dev_priv = bus->dev_priv;
 	int i, reg_offset;
 	int ret = 0;
+	u32 gmbus2 = 0;
 
 	mutex_lock(&dev_priv->gmbus_mutex);
 
@@ -338,11 +345,12 @@ gmbus_xfer(struct i2c_adapter *adapter,
 		if (ret == -ENXIO)
 			goto clear_err;
 
-		if (wait_for(I915_READ(GMBUS2 + reg_offset) &
-			     (GMBUS_SATOER | GMBUS_HW_WAIT_PHASE),
-			     50))
+		ret = wait_for((gmbus2 = I915_READ(GMBUS2 + reg_offset)) &
+			       (GMBUS_SATOER | GMBUS_HW_WAIT_PHASE),
+			       50);
+		if (ret)
 			goto timeout;
-		if (I915_READ(GMBUS2 + reg_offset) & GMBUS_SATOER)
+		if (gmbus2 & GMBUS_SATOER)
 			goto clear_err;
 	}
 
-- 
1.7.7.3

Re: [PATCH 13/13 v4] drm/i915/intel_i2c: reuse GMBUS2 value read in polling loop

[Chris Wilson]

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 {
> +		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

Re: [PATCH 13/13 v4] drm/i915/intel_i2c: reuse GMBUS2 value read in polling loop

[Daniel Kurtz]

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

Re: [PATCH 13/13 v4] drm/i915/intel_i2c: reuse GMBUS2 value read in polling loop

[Chris Wilson]

On Wed, 28 Mar 2012 19:39:17 +0800, Daniel Kurtz <djkurtz@chromium.org> wrote:
> On Wed, Mar 28, 2012 at 3:02 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > 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?

They can go. They were there just as paranoia to make sure the writes
were flushed before any timing delays and across loops. Once the code
settled I never took the liberty of removing them.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre