Re: [PATCH 3/4] drm/i915: Restore GMBUS operation after a failed bit-banging fallback

[Jani Nikula <jani.nikula@linux.intel.com>]

On Mon, 07 Mar 2016, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> When the GMBUS based i2c transfer times out, we try to fall back to
> bit-banging and retry the operation that way. However if the bit-banging
> attempt also fails, we should probably go back to the GMBUS method for
> the next attempt. Maybe there simply wasn't anyone one the bus at this
> time.
>
> There's also a bit of a mess going on with the force_bit handling.
> It's supposed to be a ref count actually, and it is as far as
> intel_gmbus_force_bit() is concerned. But it's treated as just a
> flag by the timeout based bit-banging fallback. I suppose that's
> fine since we should never end up in the timeout fallback case
> if force_bit was already non-zero. However now that we want to restore
> things back to where they were after the bit-banging attempt failed,
> we're going to have to do things a bit differently to avoid clobbering
> the force_bit count as set up by intel_gmbus_force_bit(). So let's
> dedicate the high bit as a flag for the low level timeout based fallback
> and treat the rest of the bits as a ref count just as before.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h  |  1 +
>  drivers/gpu/drm/i915/intel_i2c.c | 10 +++++++---
>  2 files changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index f37ac120a29d..2348fea59592 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1043,6 +1043,7 @@ struct intel_fbc_work;
>  
>  struct intel_gmbus {
>  	struct i2c_adapter adapter;
> +#define GMBUS_FORCE_BIT_RETRY (1U << 31)
>  	u32 force_bit;
>  	u32 reg0;
>  	i915_reg_t gpio_reg;
> diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
> index 7bf8a485e18f..5d4b3604afd2 100644
> --- a/drivers/gpu/drm/i915/intel_i2c.c
> +++ b/drivers/gpu/drm/i915/intel_i2c.c
> @@ -579,7 +579,6 @@ timeout:
>  	 * Hardware may not support GMBUS over these pins? Try GPIO bitbanging
>  	 * instead. Use EAGAIN to have i2c core retry.
>  	 */
> -	bus->force_bit = 1;
>  	ret = -EAGAIN;
>  
>  out:
> @@ -597,10 +596,15 @@ gmbus_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs, int num)
>  	intel_display_power_get(dev_priv, POWER_DOMAIN_GMBUS);
>  	mutex_lock(&dev_priv->gmbus_mutex);
>  
> -	if (bus->force_bit)
> +	if (bus->force_bit) {
>  		ret = i2c_bit_algo.master_xfer(adapter, msgs, num);
> -	else
> +		if (ret < 0)
> +			bus->force_bit &= ~GMBUS_FORCE_BIT_RETRY;
> +	} else {
>  		ret = do_gmbus_xfer(adapter, msgs, num);
> +		if (ret == -EAGAIN)
> +			bus->force_bit |= GMBUS_FORCE_BIT_RETRY;

Hmm, would this all be simpler if we did the first bit-banging retry
here ourselves after all, and set ->force_bit only if bit-banging
succeeds after gmbus -EAGAIN? I think moving the retry out of
do_gmbus_xfer() was the right thing to do to, but maybe I went too far
by pushing it all the way to i2c core?

Anyway, this patch looks good, but it's just a bit subtle with the
-EAGAIN and one retry and all.

Up to you.

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

> +	}
>  
>  	mutex_unlock(&dev_priv->gmbus_mutex);
>  	intel_display_power_put(dev_priv, POWER_DOMAIN_GMBUS);

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx