On 2012-09-06 00:09, Daniel Vetter wrote:
> The gmbus interrupt generation is rather fiddly: We can only ever
> enable one interrupt source (but we always want to check for NAK
> in addition to the real bit). And the bits in the gmbus status
> register don't map at all to the bis in the irq register.
>
> To prepare for this mess, start by extracting the hw status wait
> loop into it's own function, consolidate the NAK error handling a
> bit. To keep things flexible, pass in the status bit we care about
> (in addition to any NAK signalling).
>
> v2: I've failed to notice that the sens of GMBUS_ACTIVE is inverted,
> Chris Wilson gladly pointed that out for me. To keep things simple,
> ignore that case for now (we only need to idle the gmbus controller
> at the end of an entire i2c transaction, not after every message).
>
> Signed-off-by: Daniel Vetter <daniel.vetter at ffwll.ch>
> ---
> drivers/gpu/drm/i915/intel_i2c.c | 46
> ++++++++++++++++++++++------------------
> 1 file changed, 25 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_i2c.c
> b/drivers/gpu/drm/i915/intel_i2c.c
> index b9755f6..57decac 100644
> --- a/drivers/gpu/drm/i915/intel_i2c.c
> +++ b/drivers/gpu/drm/i915/intel_i2c.c
> @@ -204,6 +204,24 @@ intel_gpio_setup(struct intel_gmbus *bus, u32
> pin)
> }
>
> static int
> +gmbus_wait_hw_status(struct drm_i915_private *dev_priv,
> + u32 gmbus2_status)
> +{
> + int ret;
> + int reg_offset = dev_priv->gpio_mmio_base;
> + u32 gmbus2;
> +
> + ret = wait_for((gmbus2 = I915_READ(GMBUS2 + reg_offset)) &
> + (GMBUS_SATOER | gmbus2_status),
> + 50);
> +
> + if (gmbus2 & GMBUS_SATOER)
> + return -ENXIO;
> +
> + return ret;
> +}
> +
> +static int
> gmbus_xfer_read(struct drm_i915_private *dev_priv, struct i2c_msg
> *msg,
> u32 gmbus1_index)
> {
I know this isn't new to your patch, but the consolidation leaves a
good opportunity for clarification of the timeout via a comment.
I started digging in to the math a bit here for the 50ms timeout and
was left confused. I believe we're off by a factor of 10 here for a
worst case which would be a 50KHz bus speed. Comments?
> @@ -220,15 +238,10 @@ gmbus_xfer_read(struct drm_i915_private
> *dev_priv, struct i2c_msg *msg,
> while (len) {
> int ret;
> u32 val, loop = 0;
> - u32 gmbus2;
>
> - ret = wait_for((gmbus2 = I915_READ(GMBUS2 + reg_offset)) &
> - (GMBUS_SATOER | GMBUS_HW_RDY),
> - 50);
> + ret = gmbus_wait_hw_status(dev_priv, GMBUS_HW_RDY);
> if (ret)
> - return -ETIMEDOUT;
> - if (gmbus2 & GMBUS_SATOER)
> - return -ENXIO;
> + return ret;
>
> val = I915_READ(GMBUS3 + reg_offset);
> do {
> @@ -262,7 +275,6 @@ gmbus_xfer_write(struct drm_i915_private
> *dev_priv, struct i2c_msg *msg)
> GMBUS_SLAVE_WRITE | GMBUS_SW_RDY);
> while (len) {
> int ret;
> - u32 gmbus2;
>
> val = loop = 0;
> do {
> @@ -271,13 +283,9 @@ gmbus_xfer_write(struct drm_i915_private
> *dev_priv, struct i2c_msg *msg)
>
> I915_WRITE(GMBUS3 + reg_offset, val);
>
> - ret = wait_for((gmbus2 = I915_READ(GMBUS2 + reg_offset)) &
> - (GMBUS_SATOER | GMBUS_HW_RDY),
> - 50);
> + ret = gmbus_wait_hw_status(dev_priv, GMBUS_HW_RDY);
> if (ret)
> - return -ETIMEDOUT;
> - if (gmbus2 & GMBUS_SATOER)
> - return -ENXIO;
> + return ret;
> }
> return 0;
> }
> @@ -346,8 +354,6 @@ gmbus_xfer(struct i2c_adapter *adapter,
> I915_WRITE(GMBUS0 + reg_offset, bus->reg0);
>
> for (i = 0; i < num; i++) {
> - u32 gmbus2;
> -
> if (gmbus_is_index_read(msgs, i, num)) {
> ret = gmbus_xfer_index_read(dev_priv, &msgs[i]);
> i += 1; /* set i to the index of the read xfer */
> @@ -362,13 +368,11 @@ gmbus_xfer(struct i2c_adapter *adapter,
> if (ret == -ENXIO)
> goto clear_err;
>
> - ret = wait_for((gmbus2 = I915_READ(GMBUS2 + reg_offset)) &
> - (GMBUS_SATOER | GMBUS_HW_WAIT_PHASE),
> - 50);
> + ret = gmbus_wait_hw_status(dev_priv, GMBUS_HW_WAIT_PHASE);
> + if (ret == -ENXIO)
> + goto clear_err;
> if (ret)
> goto timeout;
> - if (gmbus2 & GMBUS_SATOER)
> - goto clear_err;
> }
>
> /* Generate a STOP condition on the bus. Note that gmbus can't
> generata
--
Ben Widawsky, Intel Open Source Technology Center
