Re: [PATCH 2/2] drm/i915: abstract i2c bit banging fallback in gmbus xfer

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Tue, Dec 01, 2015 at 05:38:30PM +0200, Jani Nikula wrote:
> On Tue, 01 Dec 2015, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > On Tue, Dec 01, 2015 at 04:29:26PM +0200, Jani Nikula wrote:
> >> Choose between i2c bit banging and gmbus in a new higher level function,
> >> and let the i2c core retry the first time we fall back to bit banging.
> >> 
> >> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/intel_i2c.c | 39 +++++++++++++++++++++++++--------------
> >>  1 file changed, 25 insertions(+), 14 deletions(-)
> >> 
> >> diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
> >> index ccb522c176bd..e26e22a72e3b 100644
> >> --- a/drivers/gpu/drm/i915/intel_i2c.c
> >> +++ b/drivers/gpu/drm/i915/intel_i2c.c
> >> @@ -472,9 +472,7 @@ gmbus_xfer_index_read(struct drm_i915_private *dev_priv, struct i2c_msg *msgs)
> >>  }
> >>  
> >>  static int
> >> -gmbus_xfer(struct i2c_adapter *adapter,
> >> -	   struct i2c_msg *msgs,
> >> -	   int num)
> >> +do_gmbus_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs, int num)
> >>  {
> >>  	struct intel_gmbus *bus = container_of(adapter,
> >>  					       struct intel_gmbus,
> >> @@ -483,14 +481,6 @@ gmbus_xfer(struct i2c_adapter *adapter,
> >>  	int i = 0, inc, try = 0;
> >>  	int ret = 0;
> >>  
> >> -	intel_display_power_get(dev_priv, POWER_DOMAIN_GMBUS);
> >> -	mutex_lock(&dev_priv->gmbus_mutex);
> >> -
> >> -	if (bus->force_bit) {
> >> -		ret = i2c_bit_algo.master_xfer(adapter, msgs, num);
> >> -		goto out;
> >> -	}
> >> -
> >>  retry:
> >>  	I915_WRITE(GMBUS0, bus->reg0);
> >>  
> >> @@ -585,13 +575,34 @@ timeout:
> >>  		 bus->adapter.name, bus->reg0 & 0xff);
> >>  	I915_WRITE(GMBUS0, 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. Use EAGAIN to have i2c core retry.
> >> +	 */
> >>  	bus->force_bit = 1;
> >> -	ret = i2c_bit_algo.master_xfer(adapter, msgs, num);
> >> +	ret = -EAGAIN;
> >
> > This does mean we're left to the mercy of the timeout handling in the
> > i2c core. That is if our gmbus attempt takes too long, we may never
> > get a chance to retry with bit banging. Not sure if that's a real
> > concern or not.
> 
> IIUC the longest it can take to transfer 4 bytes without timing out is
> just under 50 ms. The i2c core use a default timeout of 1000 ms. So if
> we first succesfully, but slowly manage to transfer quite a bit (well,
> at least 80 bytes, almost but not quite timing out), and then time out,
> i2c core won't try again.
> 
> I do not think this is a real concern, but if it is, we can increase the
> adapter->timeout in our gmbus setup code.

Hmm. I was thinking it's shorter than that, but I guess I was mixing it
up with the i2c-bit-algo timeout which is 2.2ms (per some DDC spec
IIRC).

1 second seems plenty enough to me.

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> 
> BR,
> Jani.
> 
> 
> 
> >
> > Otherwise lgtm.
> >
> >>  
> >>  out:
> >> -	mutex_unlock(&dev_priv->gmbus_mutex);
> >> +	return ret;
> >> +}
> >> +
> >> +static int
> >> +gmbus_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs, int num)
> >> +{
> >> +	struct intel_gmbus *bus = container_of(adapter, struct intel_gmbus,
> >> +					       adapter);
> >> +	struct drm_i915_private *dev_priv = bus->dev_priv;
> >> +	int ret;
> >> +
> >> +	intel_display_power_get(dev_priv, POWER_DOMAIN_GMBUS);
> >> +	mutex_lock(&dev_priv->gmbus_mutex);
> >> +
> >> +	if (bus->force_bit)
> >> +		ret = i2c_bit_algo.master_xfer(adapter, msgs, num);
> >> +	else
> >> +		ret = do_gmbus_xfer(adapter, msgs, num);
> >>  
> >> +	mutex_unlock(&dev_priv->gmbus_mutex);
> >>  	intel_display_power_put(dev_priv, POWER_DOMAIN_GMBUS);
> >>  
> >>  	return ret;
> >> -- 
> >> 2.1.4
> >> 
> >> _______________________________________________
> >> Intel-gfx mailing list
> >> Intel-gfx@lists.freedesktop.org
> >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx